{/* Version row */}
-
-
-
-
-
+
+
+
Version
-
Installed Codra release
-
+
v{pkg.version}
-
+
- {/* Changelog / links row */}
+ {/* License row */}
-
-
-
-
Releases
-
Browse all versions and release notes on GitHub
+
License
+
{pkg.license}
+
+
+
+
+ {/* Links */}
+
+ {[
+ { href: `${pkg.repository.url.replace(/\.git$/, '')}/releases/`, label: 'Releases', sub: 'Version history & notes' },
+ { href: pkg.homepage, label: 'Homepage', sub: 'codra.run' },
+ { href: pkg.bugs.url, label: 'Report an issue', sub: 'GitHub issue tracker' },
+ ].map(({ href, label, sub }) => (
- View releases
-
+
+
+ {label}
+
+
+ {sub}
+
-
-
+ ))}
diff --git a/src/client/pages/stats.tsx b/src/client/pages/stats.tsx
index bd7e555..f0c9963 100644
--- a/src/client/pages/stats.tsx
+++ b/src/client/pages/stats.tsx
@@ -5,24 +5,11 @@ import {
Bar,
BarChart,
CartesianGrid,
- Line,
- LineChart,
ResponsiveContainer,
Tooltip,
XAxis,
YAxis,
} from 'recharts';
-import {
- Activity,
- BarChart3,
- Bot,
- Gauge,
- GitBranch,
- RefreshCw,
- Server,
- TrendingUp,
-} from 'lucide-react';
-
import { TimeRangeSelect } from '@client/components/features/stats/time-range-select';
import { PageHeaderActions } from '@client/components/shared/page-header-actions';
import { PageHeader } from '@client/components/layout/page-header';
@@ -36,21 +23,16 @@ import { fmtNumber } from '@client/lib/utils';
import type { StatsPayload } from '@shared/schema';
const CHART = {
- primary: '#84cc16',
+ primary: '#65a30d',
primaryDark: '#e0fe56',
comments: '#0ea5e9',
- warning: '#f59e0b',
- danger: '#ef4444',
- quiet: '#64748b',
- violet: '#8b5cf6',
-};
-
-const axisProps = {
- fontSize: 11,
- tickLine: false,
- tickMargin: 10,
- axisLine: false,
- tick: { fontFamily: 'var(--font-sans)', fill: 'var(--muted-foreground)' } as const,
+ commentsDark: '#38bdf8',
+ warning: '#d97706',
+ warningDark: '#fbbf24',
+ danger: '#dc2626',
+ dangerDark: '#f87171',
+ quiet: '#94a3b8',
+ quietDark: '#64748b',
};
function formatDay(value: string) {
@@ -63,11 +45,6 @@ function formatCompact(value: number) {
return value >= 1000 ? fmtNumber(value) : value.toLocaleString();
}
-function ratio(numerator: number, denominator: number, decimals = 1) {
- if (!denominator) return '0';
- return (numerator / denominator).toFixed(decimals);
-}
-
function percent(numerator: number, denominator: number) {
if (!denominator) return 0;
return Math.round((numerator / denominator) * 100);
@@ -111,135 +88,163 @@ function GraphSectionTitle() {
function GraphShell({
title,
- eyebrow,
- icon: Icon,
children,
className = '',
}: {
title: string;
- eyebrow: string;
- icon: typeof Activity;
children: ReactNode;
className?: string;
}) {
return (
-
-
-
-
-
-
+
+
+
{title}
- {children}
+ {children}
);
}
-function GraphCardGallery({ stats, days, isDark }: { stats: StatsPayload; days: number; isDark: boolean }) {
+function MetricsGrid({ stats, isDark }: { stats: StatsPayload; isDark: boolean }) {
const color = isDark ? CHART.primaryDark : CHART.primary;
- const tokenTrend = stats.trend.map((day) => ({
- ...day,
- tokens: day.inputTokens + day.outputTokens,
- tokenDensity: Math.round((day.inputTokens + day.outputTokens) / Math.max(day.jobs, 1)),
- commentRate: Number(ratio(day.comments, Math.max(day.jobs, 1), 1)),
- }));
- const cumulativeTrend = stats.trend.reduce<{ day: string; reviews: number; comments: number }[]>((acc, day) => {
- const prev = acc[acc.length - 1];
- acc.push({
- day: day.day,
- reviews: (prev?.reviews ?? 0) + day.jobs,
- comments: (prev?.comments ?? 0) + day.comments,
- });
- return acc;
- }, []);
+ const commentsColor = isDark ? CHART.commentsDark : CHART.comments;
+ const inputColor = isDark ? CHART.commentsDark : CHART.comments;
+ const outputColor = isDark ? CHART.warningDark : CHART.warning;
+ const dangerColor = isDark ? CHART.dangerDark : CHART.danger;
+ const quietColor = isDark ? CHART.quietDark : CHART.quiet;
const repoMax = Math.max(...stats.topRepos.map((repo) => repo.jobs), 1);
const modelMax = Math.max(...stats.models.map((model) => model.calls), 1);
+ // Theme-aware chart chrome. CSS variables don't reliably resolve inside
+ // Recharts SVG text, so use explicit colors keyed off the active theme.
+ const axisColor = isDark ? 'rgba(228,228,231,0.62)' : 'rgba(63,63,70,0.78)';
+ const gridColor = isDark ? 'rgba(255,255,255,0.08)' : 'rgba(0,0,0,0.08)';
+ const cursorColor = isDark ? 'rgba(255,255,255,0.06)' : 'rgba(0,0,0,0.05)';
+ const axisProps = {
+ fontSize: 11,
+ tickLine: false,
+ tickMargin: 8,
+ axisLine: false,
+ tick: { fontFamily: 'var(--font-sans)', fill: axisColor },
+ } as const;
+ // Let Recharts thin the labels by available space (respecting minTickGap)
+ // rather than a fixed stride keyed off the range — the trend array can have
+ // far fewer points than `days`, which would hide every label but the first.
+ const xAxisProps = {
+ dataKey: 'day',
+ tickFormatter: formatDay,
+ interval: 'preserveStartEnd' as const,
+ minTickGap: 24,
+ };
+
+ const STATUS_COLOR: Record
= {
+ done: color,
+ running: commentsColor,
+ queued: quietColor,
+ failed: dangerColor,
+ superseded: quietColor,
+ cancelled: quietColor,
+ };
+ const statusTotal = Math.max(stats.statuses.reduce((sum, s) => sum + s.count, 0), 1);
+
return (
-
+
-
-
-
-
-
-
-
-
- } />
-
-
-
-
-
-
-
-
-
+
+
+
-
+
-
-
-
+
+
+
+
+
-
-
-
- } />
-
+
+
+
+ } cursor={{ stroke: color, strokeDasharray: '4 4' }} />
+
+
-
-
-
-
+
+
-
-
-
-
- } />
-
-
+
+
+
+
+ } cursor={{ fill: cursorColor }} />
+
+
+
-
-
-
-
-
- } />
-
-
-
+
+
+
+
+ {stats.statuses.map((s) => (
+
+ ))}
+
+
+ {stats.statuses.map((s) => (
+
+
+
+ {s.status}
+
+
{s.count}
+
+ ))}
+
-
-
-
-
- {stats.topRepos.slice(0, 7).map((repo) => (
+
+
+ {stats.topRepos.slice(0, 8).map((repo) => (
-
+
{repo.owner}/{repo.repo}
-
@@ -249,16 +254,16 @@ function GraphCardGallery({ stats, days, isDark }: { stats: StatsPayload; days:
-
-
- {stats.models.slice(0, 7).map((model) => (
+
+
+ {stats.models.slice(0, 8).map((model) => (
-
+
{modelName(model.modelUsed)}
-
{model.calls}
@@ -267,176 +272,6 @@ function GraphCardGallery({ stats, days, isDark }: { stats: StatsPayload; days:
-
-
- );
-}
-
-function PanelHeader({
- label,
- value,
- detail,
-}: {
- label: string;
- value?: string;
- detail?: string;
-}) {
- return (
-
-
-
-
-
-
-
{label}
- {detail &&
{detail}
}
-
-
- {value && (
-
- {value}
-
- )}
-
- );
-}
-
-function ReviewFlowCard({
- stats,
- days,
- isDark,
-}: {
- stats: StatsPayload;
- days: number;
- isDark: boolean;
-}) {
- const color = isDark ? CHART.primaryDark : CHART.primary;
- const totalReviews = stats.trend.reduce((sum, day) => sum + day.jobs, 0);
- const maxDay = stats.trend.reduce(
- (best, day) => (day.jobs > best.jobs ? day : best),
- stats.trend[0] ?? { day: '', jobs: 0 },
- );
-
- return (
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
- } cursor={{ stroke: color, strokeDasharray: '4 4' }} />
-
-
-
-
-
-
-
-
-
- );
-}
-
-function ReviewFlowSkeleton() {
- return (
-
);
}
@@ -444,14 +279,10 @@ function ReviewFlowSkeleton() {
function GraphCardSkeleton({ className = '' }: { className?: string }) {
return (
-
-
-
-
-
-
+
+
-
+
@@ -461,15 +292,11 @@ function GraphCardSkeleton({ className = '' }: { className?: string }) {
function GraphBarCardSkeleton({ className = '' }: { className?: string }) {
return (
-
-
-
-
-
-
+
+
-
- {Array.from({ length: 5 }).map((_, i) => (
+
+ {Array.from({ length: 8 }).map((_, i) => (
@@ -483,21 +310,18 @@ function GraphBarCardSkeleton({ className = '' }: { className?: string }) {
);
}
-function GraphCardGallerySkeleton() {
+function MetricsGridSkeleton() {
return (
-
+
-
-
-
-
-
-
-
+
+
+
-
);
@@ -546,15 +370,9 @@ export function StatsPage() {
{error &&
{error}}
{stats ? (
- <>
-
-
-
+
) : (
- <>
-
-
-
+
)}
);
diff --git a/src/server/app.ts b/src/server/app.ts
index 3a28be2..4265f70 100644
--- a/src/server/app.ts
+++ b/src/server/app.ts
@@ -11,6 +11,7 @@ import { createJobsRouter } from '@server/routes/api/jobs';
import { createReposRouter } from '@server/routes/api/repos';
import { createStatsRouter } from '@server/routes/api/stats';
import { createModelsRouter } from '@server/routes/api/models';
+import { createSettingsRouter } from '@server/routes/api/settings';
async function serveIndex(c: Context
) {
return c.env.ASSETS.fetch(new URL('/index.html', c.req.url));
@@ -34,6 +35,7 @@ export function createApp() {
app.route('/api/repos', createReposRouter());
app.route('/api/stats', createStatsRouter());
app.route('/api/models', createModelsRouter());
+ app.route('/api/settings', createSettingsRouter());
app.get('/login', serveIndex);
app.get('/', serveIndex); // Unauthenticated landing page
diff --git a/src/server/core/diff.ts b/src/server/core/diff.ts
index 64a5641..76e6331 100644
--- a/src/server/core/diff.ts
+++ b/src/server/core/diff.ts
@@ -32,15 +32,22 @@ const defaultSkipMatchers = ['**/*.lock', '**/package-lock.json', '**/pnpm-lock.
picomatch(pattern, { dot: true }),
);
-export function parseUnifiedDiff(rawDiff: string): FileDiff[] {
- const lines = rawDiff.replace(/\r\n/g, '\n').split('\n');
+export function isReviewableFile(path: string, customMatchers: ReturnType[]) {
+ if (defaultSkipMatchers.some((matcher) => matcher(path))) return false;
+ if (customMatchers.some((matcher) => matcher(path))) return false;
+ return true;
+}
+
+export function parseUnifiedDiff(rawDiff: string, reviewConfig?: RepoConfig['review']): FileDiff[] {
const files: FileDiff[] = [];
+ const customMatchers = reviewConfig?.skip_files?.map((pattern) => picomatch(pattern, { dot: true })) ?? [];
let currentFile: FileDiff | null = null;
let currentHunk: DiffHunk | null = null;
let oldLine = 0;
let newLine = 0;
let position = 0;
+ let isIgnored = false;
const pushCurrentFile = () => {
if (currentFile) {
@@ -51,13 +58,29 @@ export function parseUnifiedDiff(rawDiff: string): FileDiff[] {
oldLine = 0;
newLine = 0;
position = 0;
+ isIgnored = false;
};
- for (const line of lines) {
+ let startIndex = 0;
+ const length = rawDiff.length;
+
+ while (startIndex < length) {
+ let endIndex = rawDiff.indexOf('\n', startIndex);
+ if (endIndex === -1) {
+ endIndex = length;
+ }
+
+ let line = rawDiff.substring(startIndex, endIndex);
+ if (line.charCodeAt(line.length - 1) === 13) {
+ line = line.slice(0, -1);
+ }
+
+ startIndex = endIndex + 1;
+
if (line.startsWith('diff --git ')) {
pushCurrentFile();
- const parts = line.split(' ');
- const bPath = parts[parts.length - 1];
+ const lastSpace = line.lastIndexOf(' ');
+ const bPath = line.substring(lastSpace + 1);
const path = bPath.startsWith('b/') ? bPath.slice(2) : bPath;
currentFile = {
@@ -69,6 +92,10 @@ export function parseUnifiedDiff(rawDiff: string): FileDiff[] {
lineCount: 0,
hunks: [],
};
+
+ if (reviewConfig) {
+ isIgnored = !isReviewableFile(path, customMatchers);
+ }
continue;
}
@@ -77,12 +104,16 @@ export function parseUnifiedDiff(rawDiff: string): FileDiff[] {
}
if (line.startsWith('rename from ')) {
- currentFile.previousPath = line.slice('rename from '.length);
+ currentFile.previousPath = line.slice(12);
continue;
}
if (line.startsWith('rename to ')) {
- currentFile.path = line.slice('rename to '.length);
+ const nextPath = line.slice(10);
+ currentFile.path = nextPath.startsWith('b/') ? nextPath.slice(2) : nextPath;
+ if (reviewConfig) {
+ isIgnored = !isReviewableFile(currentFile.path, customMatchers);
+ }
continue;
}
@@ -93,21 +124,30 @@ export function parseUnifiedDiff(rawDiff: string): FileDiff[] {
if (line.startsWith('deleted file mode ')) {
currentFile.isDeleted = true;
+ isIgnored = true;
continue;
}
if (line.startsWith('Binary files ') || line.startsWith('GIT binary patch')) {
currentFile.isBinary = true;
- continue;
- }
-
- if (line.startsWith('--- ')) {
+ isIgnored = true;
continue;
}
if (line.startsWith('+++ ')) {
const nextPath = line.slice(4);
currentFile.path = nextPath.startsWith('b/') ? nextPath.slice(2) : nextPath;
+ if (reviewConfig) {
+ isIgnored = !isReviewableFile(currentFile.path, customMatchers);
+ }
+ continue;
+ }
+
+ if (isIgnored) {
+ continue;
+ }
+
+ if (line.startsWith('--- ')) {
continue;
}
@@ -132,7 +172,7 @@ export function parseUnifiedDiff(rawDiff: string): FileDiff[] {
}
const prefix = line[0];
- if (![' ', '+', '-'].includes(prefix)) {
+ if (prefix !== ' ' && prefix !== '+' && prefix !== '-') {
continue;
}
@@ -282,3 +322,62 @@ export function truncateFileDiff(file: FileDiff, maxLines: number): FileDiff {
originalLineCount: file.lineCount,
};
}
+
+export function chunkFileDiff(file: FileDiff, maxLinesPerChunk: number): FileDiff[] {
+ if (file.lineCount <= maxLinesPerChunk) {
+ return [file];
+ }
+
+ const chunks: FileDiff[] = [];
+ let currentHunks: DiffHunk[] = [];
+ let currentLines = 0;
+
+ for (const hunk of file.hunks) {
+ let linesRemainingInHunk = hunk.lines;
+
+ while (linesRemainingInHunk.length > 0) {
+ const roomInChunk = maxLinesPerChunk - currentLines;
+
+ if (roomInChunk <= 0) {
+ chunks.push({
+ ...file,
+ hunks: currentHunks,
+ lineCount: currentLines,
+ isTruncated: true,
+ originalLineCount: file.lineCount,
+ });
+ currentHunks = [];
+ currentLines = 0;
+ continue;
+ }
+
+ if (linesRemainingInHunk.length <= roomInChunk) {
+ currentHunks.push({
+ ...hunk,
+ lines: linesRemainingInHunk,
+ });
+ currentLines += linesRemainingInHunk.length;
+ linesRemainingInHunk = [];
+ } else {
+ currentHunks.push({
+ ...hunk,
+ lines: linesRemainingInHunk.slice(0, roomInChunk),
+ });
+ currentLines += roomInChunk;
+ linesRemainingInHunk = linesRemainingInHunk.slice(roomInChunk);
+ }
+ }
+ }
+
+ if (currentHunks.length > 0) {
+ chunks.push({
+ ...file,
+ hunks: currentHunks,
+ lineCount: currentLines,
+ isTruncated: true,
+ originalLineCount: file.lineCount,
+ });
+ }
+
+ return chunks;
+}
diff --git a/src/server/core/github.ts b/src/server/core/github.ts
index 28dee67..57b4dc4 100644
--- a/src/server/core/github.ts
+++ b/src/server/core/github.ts
@@ -196,9 +196,22 @@ export class GitHubClient {
private readonly tracker?: { incrementSubrequests(count?: number): void },
) {}
+ // In-memory token cache scoped to this client instance (i.e. one Worker invocation). Without it,
+ // every GitHub request re-read the token from KV -- a wasted subrequest per call. A finalize or
+ // review invocation makes many GitHub calls, so that repeated KV read pushed the invocation toward
+ // the Workers-Free 50-subrequest cap (finalize could tip over it right before posting the review).
+ private memoToken: InstallationTokenCacheRecord | null = null;
+
async getInstallationToken(): Promise {
+ // Reuse the in-memory token while it's comfortably unexpired (invocations are < ~120s; tokens
+ // last ~1h, so this holds for the whole invocation) -- no KV read, no network call.
+ if (this.memoToken?.token && new Date(this.memoToken.expiresAt).getTime() > Date.now() + 60_000) {
+ return this.memoToken.token;
+ }
+
const cached = await readCachedInstallationToken(this.env, this.installationId, this.tracker);
if (cached?.token) {
+ this.memoToken = cached;
return cached.token;
}
@@ -229,10 +242,12 @@ export class GitHubClient {
}
const data = (await response.json()) as { token: string; expires_at: string };
- await writeCachedInstallationToken(this.env, this.installationId, {
+ const record: InstallationTokenCacheRecord = {
token: data.token,
expiresAt: data.expires_at,
- }, this.tracker);
+ };
+ await writeCachedInstallationToken(this.env, this.installationId, record, this.tracker);
+ this.memoToken = record;
return data.token;
});
@@ -462,7 +477,7 @@ export class GitHubClient {
title: string;
summary: string;
status?: 'in_progress' | 'completed';
- conclusion?: 'success' | 'neutral' | 'failure';
+ conclusion?: 'success' | 'neutral' | 'failure' | 'cancelled';
},
) {
return withRetry(`updateCheckRun ${owner}/${repo} ${checkRunId}`, async () => {
diff --git a/src/server/core/job-recovery.ts b/src/server/core/job-recovery.ts
index 83ec21f..5cf0405 100644
--- a/src/server/core/job-recovery.ts
+++ b/src/server/core/job-recovery.ts
@@ -13,6 +13,10 @@ export async function recoverJobs(env: AppBindings) {
jobId,
deliveryId: crypto.randomUUID(),
phase: 'review',
+ // The job's previous Workflow instance (keyed on jobId) is dead but still exists, so a
+ // same-id create() would be dropped as a duplicate. Force a fresh instance keyed on the new
+ // deliveryId so the recovered job actually resumes instead of climbing recovery_count.
+ forceFreshInstance: true,
});
}
@@ -28,19 +32,43 @@ export async function recoverJobs(env: AppBindings) {
}
export async function completeTerminalCheckRuns(env: AppBindings) {
- // Limit to 5 to avoid Cloudflare's 50 subrequest limit per invocation.
- // Each job requires 1 GitHub API request and 1 DB update.
- const jobs = await getTerminalJobsNeedingCheckRunCompletion(env, 5);
+ // Limit to 1 to avoid Cloudflare's 50 subrequest limit per invocation,
+ // especially when called opportunistically via waitUntil during API polling.
+ // Each job requires multiple subrequests (KV, GitHub API, Hyperdrive).
+ const jobs = await getTerminalJobsNeedingCheckRunCompletion(env, 1);
for (const job of jobs) {
if (!job.check_run_id) continue;
try {
const github = new GitHubService(env, job.installation_id);
+
+ let conclusion: 'success' | 'neutral' | 'failure' | 'cancelled';
+ let title: string;
+ let summary: string;
+ if (job.status === 'done') {
+ // A completed review whose inline check-run update didn't land (e.g. finalize ran out of
+ // subrequest budget). Reconstruct the same conclusion finalize would have posted.
+ const partial = (job.error_msg ?? '').startsWith('Partial review');
+ conclusion = partial ? 'failure' : (job.verdict === 'approve' ? 'success' : 'neutral');
+ title = partial ? 'Review partially failed' : (job.verdict === 'approve' ? 'LGTM' : 'Comments posted');
+ summary = job.error_msg ?? `${job.comment_count ?? 0} inline comments across ${job.file_count ?? 0} files.`;
+ } else {
+ const checkRunPresentation = {
+ superseded: { conclusion: 'neutral' as const, title: 'Review superseded', summary: 'Superseded by a newer commit or job.' },
+ cancelled: { conclusion: 'cancelled' as const, title: 'Review stopped', summary: 'Stopped by user.' },
+ failed: { conclusion: 'failure' as const, title: 'Review failed', summary: 'Review failed.' },
+ };
+ const presentation = checkRunPresentation[job.status as keyof typeof checkRunPresentation] ?? checkRunPresentation.failed;
+ conclusion = presentation.conclusion;
+ title = presentation.title;
+ summary = job.error_msg ?? presentation.summary;
+ }
+
await github.updateCheckRun(job.owner, job.repo, job.check_run_id, {
status: 'completed',
- conclusion: job.status === 'superseded' ? 'neutral' : 'failure',
- title: job.status === 'superseded' ? 'Review superseded' : 'Review failed',
- summary: job.error_msg ?? (job.status === 'superseded' ? 'Superseded by a newer commit or job.' : 'Review failed.'),
+ conclusion,
+ title,
+ summary,
});
await markJobCheckRunCompleted(env, job.id);
} catch (error) {
diff --git a/src/server/core/model-output.ts b/src/server/core/model-output.ts
index d2298d5..3ad16f8 100644
--- a/src/server/core/model-output.ts
+++ b/src/server/core/model-output.ts
@@ -398,7 +398,7 @@ export function parseFileReviewResponse(raw: string, file: FileDiff): {
while (current !== prev) {
prev = current;
current = current
- .replace(/^([\u{1F300}-\u{1F9FF}]|\[QUALITY\]|\[SECURITY\]|\[BUG\]|\[PERFORMANCE\]|\[CORRECTNESS\]|\[P[0-3]\]|\[NIT\]|QUALITY|SECURITY|BUG|P[0-3]|NIT|[:\-\s\uFE0F]|[^\w\s])+/giu, '')
+ .replace(/^(?:[^\w\s]+|(?:QUALITY|SECURITY|BUG|PERFORMANCE|CORRECTNESS|P[0-3]|NIT)\b)+/giu, '')
.replace(/\n\s*/g, ' ') // Flatten newlines in titles/snippets
.trim();
}
diff --git a/src/server/core/review.ts b/src/server/core/review.ts
index 4a4d1b3..e49656a 100644
--- a/src/server/core/review.ts
+++ b/src/server/core/review.ts
@@ -1,8 +1,9 @@
import { logger } from './logger';
import { isSupportedGitHubWebhookEvent, type GitHubWebhookEventName, type GitHubWebhookPayload, type IssueCommentWebhookPayload, type PullRequestWebhookPayload } from '@shared/github';
import { defaultRepoConfig, normalizeModelId, type ParsedReviewComment, type RepoConfig, type ReviewJobMessage } from '@shared/schema';
+import { isTimeoutMessage, matchesAnyTransientSubstring } from '@shared/transient-errors';
import type { AppBindings } from '@server/env';
-import { getFileReviewsForJobs, recordRetryableFileReviewFailure, upsertFileReview } from '@server/db/file-reviews';
+import { bulkInheritFileReviews, bulkMarkFilesFailed, getFileReviewsForJobs, recordRetryableFileReviewFailure, upsertFileReview } from '@server/db/file-reviews';
import { getResolvedModelConfig } from '@server/db/model-configs';
import {
claimJobLease,
@@ -17,7 +18,10 @@ import {
mapJob,
markJobCheckRunCompleted,
markJobContinuationQueued,
+ resetJobContinuationCount,
releaseJobLease,
+ setJobPullRequestMeta,
+ setJobWorkflowInstance,
supersedeOlderJobs,
updateJobCheckRun,
updateJobStep,
@@ -32,42 +36,119 @@ import { TokenTracker } from './token-tracker';
import { loadRepoConfig } from './config';
import { getWebhookDelivery } from '@server/db/webhook-deliveries';
import { sendTelemetryEvent } from './telemetry';
+import { getReviewSettings } from '@server/db/app-settings';
+import { REVIEW_CONCURRENCY_LIMITS } from '@shared/schema';
type PersistedReviewJob = ReturnType;
-export type ReviewJobRunResult =
- | { action: 'ack' }
+export type ReviewJobRunResult =
+ | { action: 'ack' }
| { action: 'retry'; delaySeconds: number }
| { action: 'next_phase'; phase: 'prepare' | 'review' | 'finalize'; delaySeconds: number };
-const REVIEW_CHUNK_FILE_LIMIT = 3;
const REVIEW_CHUNK_WALL_CLOCK_MS = 12 * 60 * 1000;
-const MAX_CONCURRENT_JOBS = 4;
const JOB_LEASE_SECONDS = 15 * 60;
const BUSY_RETRY_SECONDS = 60;
-const RETRYABLE_MODEL_FAILURE_RETRY_DELAYS_SECONDS = [60, 5 * 60, 15 * 60];
+// Backoff between deferred retries of a file that hit a transient model/provider failure.
+// Kept short at first: most observed failures are momentary provider load (Gemini 500/503
+// "high demand") or self-inflicted connection queuing, both of which clear within seconds,
+// so a long first delay just makes reviews grind. Later attempts back off harder in case the
+// provider really is having an outage.
+const RETRYABLE_MODEL_FAILURE_RETRY_DELAYS_SECONDS = [30, 2 * 60, 5 * 60];
+// Yield used when a review chunk / phase transition MUST run in a fresh Worker invocation to get a
+// fresh per-invocation subrequest budget (Workers Free: 50/invocation). Cloudflare only hibernates
+// a Workflow -- running -> waiting -> resume in a NEW invocation -- when the step.sleep is long
+// enough; a "very short" sleep keeps the instance warm in the SAME invocation, so the real
+// subrequest budget accumulates across every chunk until it's exhausted and the whole review loops
+// in one invocation until "Too many subrequests" (the observed failure). This yield is deliberately
+// long enough to force hibernation so each continuation starts with a clean budget. It is only
+// applied when a fresh budget is actually needed (multi-chunk reviews, budget-pressured finalize),
+// so small PRs that fit in a single invocation stay fast.
+const FRESH_INVOCATION_YIELD_SECONDS = 60;
+// Delay between polls of an in-flight Workers AI async batch review. Batches typically complete
+// within a few minutes, so poll on a short cadence; a stuck batch is bounded by the shared
+// MAX_JOB_CONTINUATIONS ceiling (each poll reschedule counts as a no-progress continuation).
+const ASYNC_BATCH_POLL_DELAY_SECONDS = 20;
const MAX_RETRYABLE_FILE_REVIEW_FAILURES = 3;
+// Belt-and-suspenders ceiling on how many times a job may reschedule the *same* phase without
+// completing a single file (see markJobContinuationQueued / resetJobContinuationCount). The
+// per-file cap above is the primary bound and resets this counter on any progress, so a healthy
+// job never approaches this; it only fires when a job is genuinely wedged (e.g. a provider is
+// down for the whole backoff window) and stops it from churning for hours before finalizing.
+const MAX_JOB_CONTINUATIONS = 20;
+// A job's commit (and therefore its diff) never changes, so the raw diff can be
+// cached for the job's entire lifetime instead of being re-fetched from GitHub on
+// every prepare/review-chunk/finalize phase. 6h comfortably covers even a job that
+// hits every retryable-failure backoff (up to 15 min each, several times over).
+const DIFF_CACHE_TTL_SECONDS = 6 * 60 * 60;
+// Estimated subrequest cost of reviewing one file, used only to size how many files can
+// safely be reviewed concurrently in a chunk given the job's remaining subrequest budget for
+// this invocation (see budgetAwareChunkFileLimit below). A file walks a fallback chain of up
+// to ~3 models, but the per-model model-config lookup is now cached per invocation
+// (ModelService.resolveModel), so the recurring cost per file is ~1 provider call per model
+// tried plus the persisted-review write -- roughly 5 in the worst case rather than 9. Lower
+// estimate => more files reviewed in parallel per chunk within the same 50-subrequest cap.
+//
+// Sized to the ~5 worst-case figure above (not padded higher): with the TokenTracker's
+// SAFE_MARGIN reserve of 25 the fresh-budget headroom is 25, and 25 / 5 == 5 keeps even the
+// highest configured concurrency level (max == 4) fully honored at a healthy budget. Padding
+// this to 8 would make floor(25 / 8) == 3 silently cap the "max" slider to 3 -- the exact
+// "concurrency slider is dead above medium" regression pinned by chunk-concurrency.spec.ts.
+const ESTIMATED_SUBREQUESTS_PER_FILE = 5;
+
+/**
+ * How many files a single review chunk may process concurrently: the configured concurrency
+ * level, capped only by what the invocation's remaining subrequest budget can safely cover.
+ *
+ * The cap is deliberately sized so it does NOT silently override the user's chosen concurrency
+ * at a healthy budget -- that would make the concurrency setting a no-op above the cap. It
+ * only throttles once earlier failures in this invocation have actually eaten into the budget;
+ * if there is not enough safe budget for one more file, the chunk yields and resumes in a fresh
+ * invocation instead of gambling past the margin. Any files a throttled chunk can't reach roll
+ * into the next chunk. The
+ * chunk-file-limit-honors-configured-level invariant is pinned by a regression test.
+ */
+export function budgetAwareFileLimit(remainingSafeBudget: number, configuredChunkFileLimit: number) {
+ const budgetLimit = Math.floor(remainingSafeBudget / ESTIMATED_SUBREQUESTS_PER_FILE);
+ return Math.min(configuredChunkFileLimit, budgetLimit);
+}
function isRetryableFileReviewErrorMessage(message: string | null | undefined) {
if (!message) return false;
const lower = message.toLowerCase();
+
+ // Explicitly fail fast for timeouts so they don't loop endlessly, aligning with
+ // isTransientModelFailure which prevents timeouts from being retried.
+ if (isTimeoutMessage(lower)) {
+ return false;
+ }
+
return (
+ matchesAnyTransientSubstring(lower) ||
lower.includes('all configured review models failed') ||
lower.includes('retrying later') ||
lower.includes('google request failed with 5') ||
- lower.includes('cloudflare') ||
- lower.includes('timeout') ||
- lower.includes('timed out') ||
- lower.includes('internal error') ||
- lower.includes('unavailable') ||
- lower.includes('high demand') ||
lower.includes('temporary') ||
- lower.includes('[redacted]') ||
- lower.includes('returned no review content') ||
- lower.includes('empty response')
+ // Older jobs may have persisted subrequest-budget failures before budget exhaustion became
+ // a pure chunk-level deferral. Keep retrying those rows instead of treating them as handled.
+ lower.includes('subrequest')
);
}
+/**
+ * Detects Cloudflare's per-invocation subrequest-limit error (Workers Free plan: 50
+ * subrequests/invocation). Unlike a provider outage, this clears completely on the next
+ * invocation, so the correct response is never to fail the whole job or permanently abandon
+ * a file -- it is to persist whatever progress was made and reschedule the same phase, which
+ * runs in a fresh invocation with a fresh budget. Because each review chunk reviews and
+ * persists only a few files (see reviewChunkFileLimit), rescheduling reliably makes forward
+ * progress and the review grinds to completion instead of dying mid-way.
+ */
+function isSubrequestBudgetError(error: unknown): boolean {
+ const message = error instanceof Error ? error.message : String(error ?? '');
+ return message.toLowerCase().includes('subrequest');
+}
+
function retryableModelFailureDelaySeconds(failureCount: number | null | undefined) {
if (!failureCount || failureCount < 1) return RETRYABLE_MODEL_FAILURE_RETRY_DELAYS_SECONDS[0];
const index = Math.min(failureCount - 1, RETRYABLE_MODEL_FAILURE_RETRY_DELAYS_SECONDS.length - 1);
@@ -91,10 +172,31 @@ function countsAsHandledFileReview(review: { file_status: string; error_msg: str
return !shouldRetryExistingFileReview(review);
}
+/**
+ * A file whose review was submitted to the Workers AI async batch queue and is still
+ * queued/running. Such a row is persisted as 'pending' with the queue request_id; it is neither
+ * "handled" (it must be polled to completion) nor a failure to retry from scratch.
+ */
+function isAwaitingAsyncReview(review: { file_status: string; async_request_id?: string | null }) {
+ return review.file_status === 'pending' && !!review.async_request_id;
+}
+
+// Reduces a model identifier to its bare name, ignoring the optional `provider:` prefix.
+// A completed file review stores the bare model id (e.g. `gemini-3.1-flash-lite`), while the
+// configured strategy stores the provider-qualified id (e.g. `google:gemini-3.1-flash-lite`).
+// Comparing the bare form on both sides is what lets a retry recognise an already-reviewed file
+// as inheritable -- without it, no completed review ever matches the config and every retry
+// re-reviews every file from scratch.
+function bareModelId(model: string): string {
+ const normalized = normalizeModelId(model);
+ const colon = normalized.indexOf(':');
+ return colon === -1 ? normalized : normalized.slice(colon + 1);
+}
+
function configuredModelSet(config: RepoConfig) {
const models = new Set();
const addModel = (model: string | null | undefined) => {
- if (model) models.add(normalizeModelId(model));
+ if (model) models.add(bareModelId(model));
};
addModel(config.model?.main);
@@ -112,7 +214,7 @@ function configuredModelSet(config: RepoConfig) {
}
function canInheritParentFileReview(config: RepoConfig, review: { model_used: string }) {
- return configuredModelSet(config).has(normalizeModelId(review.model_used));
+ return configuredModelSet(config).has(bareModelId(review.model_used));
}
async function resolveModelProviderName(env: Pick, modelId: string | null | undefined) {
@@ -213,10 +315,20 @@ export async function runReviewJob(env: AppBindings, message: ReviewJobMessage):
return { action: 'ack' };
}
- const runningCount = await getOtherRunningJobsCount(env, resolved.job.id);
- if (runningCount >= MAX_CONCURRENT_JOBS) {
- logger.info(`Throttling job ${resolved.job.id}: ${runningCount} other jobs are currently running.`);
- return { action: 'retry', delaySeconds: 30 };
+ // Concurrency admission control: only throttle a job that has NOT started yet (status 'queued').
+ // A job that is already 'running' is mid-flight; re-gating its phase continuations would mean that
+ // lowering the concurrency limit (or any transient over-count from the check-then-claim race)
+ // makes every in-flight job retry forever -- that path returns before markJobContinuationQueued,
+ // so MAX_JOB_CONTINUATIONS never trips, the lease goes stale, and recovery force-fails the job.
+ // Gating only admission also avoids a second getReviewSettings fetch on review/finalize invocations.
+ if (resolved.job.status === 'queued') {
+ const { concurrencyLevel } = await getReviewSettings(env);
+ const maxConcurrentJobs = REVIEW_CONCURRENCY_LIMITS[concurrencyLevel];
+ const runningCount = await getOtherRunningJobsCount(env, resolved.job.id);
+ if (runningCount >= maxConcurrentJobs) {
+ logger.info(`Throttling admission of job ${resolved.job.id}: ${runningCount} other jobs are currently running.`);
+ return { action: 'retry', delaySeconds: 30 };
+ }
}
const leaseOwner = crypto.randomUUID();
@@ -235,6 +347,19 @@ export async function runReviewJob(env: AppBindings, message: ReviewJobMessage):
}
const job = mapJob(claim.row);
+
+ // Bind this job row to the ACTUAL Workflow instance id so job control (stop/delete/rerun) can
+ // terminate the right instance. The bind-workflow-id step can't do this for webhook-triggered
+ // jobs -- their instance is keyed on deliveryId while the job row (created later, in prepare) has
+ // a different id -- so we (re)bind here now that the real job is resolved. Cheap and idempotent.
+ if (message.workflowInstanceId && job.workflowInstanceId !== message.workflowInstanceId) {
+ try {
+ await setJobWorkflowInstance(env, job.id, message.workflowInstanceId);
+ } catch (error) {
+ logger.warn(`Failed to bind workflow instance id for job ${job.id}`, error instanceof Error ? error : new Error(String(error)));
+ }
+ }
+
const phase = resolved.phase;
const tracker = new TokenTracker();
const github = new GitHubService(env, job.installationId, tracker);
@@ -247,7 +372,7 @@ export async function runReviewJob(env: AppBindings, message: ReviewJobMessage):
} else if (phase === 'finalize') {
await runFinalizePhase(env, job, leaseOwner, github, formatter);
} else {
- await runReviewPhase(env, job, leaseOwner, github, model);
+ await runReviewPhase(env, job, leaseOwner, github, model, tracker);
}
await releaseJobLease(env, job.id, leaseOwner);
@@ -272,9 +397,27 @@ export async function runReviewJob(env: AppBindings, message: ReviewJobMessage):
phase,
delaySeconds,
});
- await markJobContinuationQueued(env, job.id, delaySeconds);
- await releaseJobLease(env, job.id, leaseOwner);
- return { action: 'next_phase', phase, delaySeconds }; // Retry same phase
+ return continueOrFailWedgedJob(env, job, github, leaseOwner, phase, delaySeconds, 'transient model/provider failures');
+ }
+
+ // Running out of this invocation's subrequest budget is not a job failure: the prepare and
+ // review phases are idempotent (already-reviewed files are persisted and skipped next time),
+ // so reschedule the same phase to resume on a fresh budget instead of terminally failing the
+ // whole review. Finalize is intentionally excluded -- it posts the GitHub review and isn't
+ // safe to re-run, and with no AI calls it does not realistically approach the cap anyway.
+ if (phase !== 'finalize' && isSubrequestBudgetError(error)) {
+ // A fresh Worker invocation is what fixes budget exhaustion -- but only a long-enough sleep
+ // actually hibernates the workflow into one. Yield long enough to force that hibernation.
+ const record = error && typeof error === 'object' ? error as { retryAfterSeconds?: unknown } : null;
+ const delaySeconds = typeof record?.retryAfterSeconds === 'number'
+ ? record.retryAfterSeconds
+ : FRESH_INVOCATION_YIELD_SECONDS;
+ logger.warn(`Review job hit the per-invocation subrequest limit; rescheduling ${phase} on a fresh budget: ${job.owner}/${job.repo} PR #${job.prNumber}`, {
+ error: messageText,
+ phase,
+ delaySeconds,
+ });
+ return continueOrFailWedgedJob(env, job, github, leaseOwner, phase, delaySeconds, 'per-invocation subrequest limits');
}
logger.error(`Review job failed: ${job.owner}/${job.repo} PR #${job.prNumber}`, error);
@@ -284,6 +427,66 @@ export async function runReviewJob(env: AppBindings, message: ReviewJobMessage):
}
}
+// Records a same-phase continuation and enforces the MAX_JOB_CONTINUATIONS ceiling. As long as
+// the job keeps completing files, resetJobContinuationCount() keeps this counter near zero; once
+// it has rescheduled MAX_JOB_CONTINUATIONS times without a single file completing, the job is
+// genuinely wedged (e.g. a provider is down for the entire backoff window), so we fail it
+// terminally instead of letting it churn indefinitely.
+async function continueOrFailWedgedJob(
+ env: AppBindings,
+ job: PersistedReviewJob,
+ github: GitHubService,
+ leaseOwner: string,
+ phase: 'prepare' | 'review' | 'finalize',
+ delaySeconds: number,
+ reason: string,
+): Promise {
+ const continuationCount = await markJobContinuationQueued(env, job.id, delaySeconds);
+
+ if (continuationCount > MAX_JOB_CONTINUATIONS) {
+ if (phase === 'review') {
+ // Degrade to a partial review rather than throwing away the work done so far. NOTE: we must
+ // RETURN the finalize transition here, not call enqueueJobPhase() -- that helper throws
+ // NextPhaseError, and because continueOrFailWedgedJob runs inside runReviewJob's catch
+ // block that throw would escape the function uncaught instead of being turned into a result.
+ logger.error(`Review job exceeded the continuation ceiling; degrading to a partial review: ${job.owner}/${job.repo} PR #${job.prNumber}`, {
+ phase,
+ continuationCount,
+ reason,
+ });
+ // Any file still awaiting an async batch result would otherwise be finalized as an empty
+ // "successful" review (its 'pending' row isn't 'failed', so finalize maps it to verdict
+ // 'comment'/'' with no findings). Mark them failed first -- mirrors the async-poll degrade path.
+ const stillPending = (await getFileReviewsForJobs(env, [job.id])).filter(isAwaitingAsyncReview);
+ for (const review of stillPending) {
+ await persistFailedFileReview(env, job.id, {
+ filePath: review.file_path,
+ modelUsed: review.async_model ?? review.model_used,
+ diffLineCount: review.diff_line_count,
+ errorMessage: 'Async batch review did not complete before the job wedged.',
+ clearAsync: true,
+ });
+ }
+ await releaseJobLease(env, job.id, leaseOwner);
+ // Finalize on a fresh invocation/budget -- this invocation is the one that hit the wall.
+ return { action: 'next_phase', phase: 'finalize', delaySeconds: FRESH_INVOCATION_YIELD_SECONDS };
+ } else {
+ const message = `Review could not make progress after ${continuationCount} continuation attempts (${reason}). Failing the job to avoid an endless retry loop; re-run it once the underlying provider issue clears.`;
+ logger.error(`Review job exceeded the continuation ceiling; failing terminally: ${job.owner}/${job.repo} PR #${job.prNumber}`, {
+ phase,
+ continuationCount,
+ reason,
+ });
+ await failJobAndCheckRun(env, job, github, message);
+ await releaseJobLease(env, job.id, leaseOwner);
+ return { action: 'ack' };
+ }
+ }
+
+ await releaseJobLease(env, job.id, leaseOwner);
+ return { action: 'next_phase', phase, delaySeconds }; // Resume same phase
+}
+
async function resolveQueuedJob(
env: AppBindings,
message: ReviewJobMessage,
@@ -426,6 +629,17 @@ async function runPreparePhase(
const pr = await github.getPullRequest(job.owner, job.repo, job.prNumber);
const config = (job.configSnapshot ?? defaultRepoConfig) as RepoConfig;
+ // Refresh the cached PR title/author from the live PR: these are snapshotted at job creation and
+ // copied onto retries, so a title edited on GitHub afterwards would otherwise stay stale.
+ try {
+ await setJobPullRequestMeta(env, job.id, {
+ prTitle: pr.title ?? null,
+ prAuthor: pr.user?.login ?? null,
+ });
+ } catch (error) {
+ logger.warn(`Failed to refresh PR metadata for job ${job.id}`, error instanceof Error ? error : new Error(String(error)));
+ }
+
let checkRunId = job.checkRunId;
if (!checkRunId) {
const checkRun = await github.createCheckRun(job.owner, job.repo, {
@@ -437,8 +651,7 @@ async function runPreparePhase(
await updateJobCheckRun(env, job.id, checkRun.id);
}
- const rawDiff = await github.getPullRequestDiff(job.owner, job.repo, job.prNumber);
- const files = filterReviewableFiles(parseUnifiedDiff(rawDiff), config.review);
+ const files = await getDiffFiles(env, job, github, config);
await completePreparationStep(env, job.id, files.length);
await heartbeatJobLease(env, job.id, leaseOwner, JOB_LEASE_SECONDS);
@@ -449,10 +662,16 @@ async function runPreparePhase(
}
if (checkRunId) {
- await github.updateCheckRun(job.owner, job.repo, checkRunId, {
- title: `Reviewing (0/${files.length})`,
- summary: 'Codra is analyzing changed files.',
- });
+ // Best-effort progress cosmetics only (see runReviewPhase): don't let a failed check-run
+ // update block enqueuing the review phase that does the actual work.
+ try {
+ await github.updateCheckRun(job.owner, job.repo, checkRunId, {
+ title: `Reviewing (0/${files.length})`,
+ summary: 'Codra is analyzing changed files.',
+ });
+ } catch (error) {
+ logger.warn(`Failed to update initial progress check run for job ${job.id}; continuing to the review phase anyway`, error instanceof Error ? error : new Error(String(error)));
+ }
}
await enqueueJobPhase(env, job.id, 'review');
}
@@ -463,6 +682,7 @@ async function runReviewPhase(
leaseOwner: string,
github: GitHubService,
model: ModelService,
+ tracker: TokenTracker,
) {
if (!hasCompletedStep(job, 'Preparation')) {
await runPreparePhase(env, job, leaseOwner, github);
@@ -479,9 +699,18 @@ async function runReviewPhase(
failureModelProviderPromise ??= resolveModelProviderName(env, failureModelId);
return failureModelProviderPromise;
};
- const rawDiff = await github.getPullRequestDiff(job.owner, job.repo, job.prNumber);
- const files = filterReviewableFiles(parseUnifiedDiff(rawDiff), config.review);
+ const files = await getDiffFiles(env, job, github, config);
const totalLineCount = files.reduce((sum, file) => sum + file.lineCount, 0);
+ const { concurrencyLevel } = await getReviewSettings(env);
+ const configuredChunkFileLimit = REVIEW_CONCURRENCY_LIMITS[concurrencyLevel];
+ // Cap this chunk's concurrency by the invocation's remaining subrequest budget so a run of
+ // model/provider failures can't push it over Cloudflare's per-invocation cap (Workers Free
+ // plan: 50) -- but sized (see budgetAwareFileLimit) so the configured concurrency level is
+ // honored in full at a healthy budget and only throttled once the budget is actually spent.
+ const reviewChunkFileLimit = budgetAwareFileLimit(tracker.remainingSafeBudget(), configuredChunkFileLimit);
+ if (reviewChunkFileLimit <= 0) {
+ throw new Error('Subrequest budget for this invocation was exhausted before starting the next review chunk.');
+ }
const startedAt = Date.now();
let processedThisChunk = 0;
@@ -492,23 +721,122 @@ async function runReviewPhase(
const parentReviews = new Map(allExistingReviews.filter((review) => review.job_id !== job.id && review.file_status === 'done').map((review) => [review.file_path, review]));
const reviewTasks: Array> = [];
+ // Counters shared across the concurrent review tasks below (single-threaded JS, so ++ is safe):
+ // `terminalProgress` counts files that reached a terminal state this chunk (reviewed, inherited,
+ // or permanently failed); `awaitingAsync` counts files still queued/running on the async batch.
+ let terminalProgress = 0;
+ let awaitingAsync = 0;
+
+ // Fast path for retries: bulk-copy every reusable parent review in one cheap DB pass instead of
+ // re-persisting them one-per-budget-slot through the throttled loop below. Only files with no row
+ // yet in this job are bulk-inherited; anything with an existing row falls through to the loop
+ // (which handles its own inherit/re-review decision). This lets a fully-inheritable retry finish
+ // the whole review phase in a single invocation rather than crawling through ~12 hibernated chunks.
+ if (job.retryOfJobId && parentReviews.size > 0) {
+ const inheritablePaths = files
+ .filter((file) => {
+ if (currentReviews.has(file.path)) return false;
+ const parent = parentReviews.get(file.path);
+ return Boolean(parent && canInheritParentFileReview(config, parent));
+ })
+ .map((file) => file.path);
+
+ if (inheritablePaths.length > 0) {
+ const inheritedPaths = await bulkInheritFileReviews(env, {
+ jobId: job.id,
+ parentJobId: job.retryOfJobId,
+ filePaths: inheritablePaths,
+ });
+ // Mark the just-copied files handled so the loop below skips them (no per-file re-work).
+ for (const path of inheritedPaths) {
+ const parent = parentReviews.get(path);
+ if (parent) currentReviews.set(path, parent);
+ }
+ terminalProgress += inheritedPaths.length;
+ if (inheritedPaths.length > 0) {
+ logger.info(`Bulk-inherited ${inheritedPaths.length} parent file reviews for job ${job.id} in one pass`);
+ }
+ }
+ }
for (const file of files) {
const existingReview = currentReviews.get(file.path);
- if (existingReview && countsAsHandledFileReview(existingReview)) {
+ // An in-flight async submission must be polled (not skipped as "handled" and not resubmitted).
+ const awaitingReview = existingReview && isAwaitingAsyncReview(existingReview) ? existingReview : null;
+ if (existingReview && countsAsHandledFileReview(existingReview) && !awaitingReview) {
continue;
}
const inherited = parentReviews.get(file.path);
const reviewTask = async () => {
+ // (0) Poll an already-submitted async batch review.
+ if (awaitingReview) {
+ const poll = await model.pollReviewBatch({
+ model: awaitingReview.async_model ?? awaitingReview.model_used,
+ requestId: awaitingReview.async_request_id!,
+ file,
+ });
+ if (poll.status === 'pending') {
+ awaitingAsync += 1;
+ return;
+ }
+ if (poll.status === 'failed') {
+ // The batch errored/expired -- fall back to a synchronous review so the file still gets done.
+ logger.warn(`Async batch poll failed for ${file.path}; falling back to synchronous review`, {
+ error: poll.error instanceof Error ? poll.error.message : String(poll.error),
+ });
+ await reviewAndPersistFile(env, job, file, pr, config, totalLineCount, model, resolveFailureModelProvider, existingReview);
+ terminalProgress += 1;
+ return;
+ }
+ await persistCompletedReview(env, job, file, poll.response);
+ terminalProgress += 1;
+ return;
+ }
+
if (!inherited) {
+ // (1) Try the async batch queue first; on any unavailability fall back to sync review.
+ const submitted = await model.submitReviewBatch({
+ file,
+ prTitle: pr.title ?? null,
+ prDescription: pr.body ?? null,
+ config,
+ totalLineCount,
+ compactPrompt: (existingReview?.transient_error_count ?? 0) > 0,
+ });
+ if (submitted) {
+ await upsertFileReview(env, job.id, {
+ filePath: file.path,
+ fileStatus: 'pending',
+ modelUsed: submitted.model,
+ modelProvider: null,
+ diffLineCount: file.lineCount,
+ diffInput: null,
+ rawAiOutput: null,
+ parsedComments: [],
+ inputTokens: null,
+ outputTokens: null,
+ durationMs: null,
+ verdict: null,
+ fileSummary: null,
+ overallCorrectness: null,
+ confidenceScore: null,
+ errorMessage: null,
+ asyncRequestId: submitted.requestId,
+ asyncModel: submitted.model,
+ });
+ awaitingAsync += 1;
+ return;
+ }
await reviewAndPersistFile(env, job, file, pr, config, totalLineCount, model, resolveFailureModelProvider, existingReview);
+ terminalProgress += 1;
return;
}
if (!canInheritParentFileReview(config, inherited)) {
logger.info(`Ignoring inherited review for ${file.path}; parent model ${inherited.model_used} is not in the current model strategy`);
await reviewAndPersistFile(env, job, file, pr, config, totalLineCount, model, resolveFailureModelProvider, existingReview);
+ terminalProgress += 1;
} else {
await upsertFileReview(env, job.id, {
filePath: file.path,
@@ -529,13 +857,14 @@ async function runReviewPhase(
errorMessage: null,
});
currentReviews.set(file.path, inherited);
+ terminalProgress += 1;
}
};
reviewTasks.push(reviewTask());
processedThisChunk += 1;
- if (processedThisChunk >= REVIEW_CHUNK_FILE_LIMIT || Date.now() - startedAt >= REVIEW_CHUNK_WALL_CLOCK_MS) {
+ if (processedThisChunk >= reviewChunkFileLimit || Date.now() - startedAt >= REVIEW_CHUNK_WALL_CLOCK_MS) {
break;
}
}
@@ -543,17 +872,27 @@ async function runReviewPhase(
const results = await Promise.allSettled(reviewTasks);
await heartbeatAndCheckSuperseded(env, job.id, leaseOwner);
+ // Terminal progress means a file reached a terminal state this chunk (reviewed, inherited, or
+ // marked permanently failed). Clear the no-progress continuation counter so a slow-but-advancing
+ // job never trips the MAX_JOB_CONTINUATIONS safety net. A chunk that only *submitted* or *polled*
+ // still-pending async batches made no terminal progress, so it must NOT reset the counter --
+ // that's what bounds polling of a batch that never completes.
+ if (terminalProgress > 0) {
+ await resetJobContinuationCount(env, job.id);
+ }
+
const rejected = results.filter((result): result is PromiseRejectedResult => result.status === 'rejected');
if (rejected.length > 0) {
rejected.forEach((result, index) => {
logger.error(`Review chunk task ${index + 1}/${rejected.length} failed`, result.reason);
});
- // If any of the rejected tasks were transient model errors, we must throw a RetryableModelError
- // so that the job orchestrator retries the chunk, instead of failing the job with an AggregateError.
- const retryableError = rejected.map(r => r.reason).find(isRetryableModelError);
- if (retryableError) {
- throw retryableError;
+ // If any rejected task was a transient model error or a per-invocation subrequest-budget
+ // hit, surface that single error so the job orchestrator reschedules the chunk on a fresh
+ // budget, instead of failing the job with an AggregateError.
+ const deferrableError = rejected.map(r => r.reason).find(r => isRetryableModelError(r) || isSubrequestBudgetError(r));
+ if (deferrableError) {
+ throw deferrableError;
}
throw rejected.length === 1
@@ -562,22 +901,150 @@ async function runReviewPhase(
}
const latestReviews = await getFileReviewsForJobs(env, [job.id]);
- const reviewedPaths = new Set(latestReviews.filter(countsAsHandledFileReview).map((review) => review.file_path));
+ // A file still awaiting its async batch result is NOT complete yet -- exclude it so the job
+ // doesn't finalize with pending reviews.
+ const reviewedPaths = new Set(
+ latestReviews.filter((review) => countsAsHandledFileReview(review) && !isAwaitingAsyncReview(review)).map((review) => review.file_path),
+ );
const completedCount = files.filter((file) => reviewedPaths.has(file.path)).length;
if (completedCount >= files.length) {
await updateJobStep(env, job.id, 'Reviewing Files', { status: 'done' });
- await enqueueJobPhase(env, job.id, 'finalize');
+ // Finalize (post the GitHub review, labels, check run) needs its OWN fresh subrequest budget.
+ // Always hibernate into a new invocation first: the review phase that just finished spent this
+ // invocation's budget, and TokenTracker under-reports real usage (it doesn't see Hyperdrive/
+ // GitHub subrequests), so a conditional yield let finalize run in the exhausted invocation and
+ // die with "Too many subrequests". Unconditional yield trades a one-time delay for reliability.
+ await enqueueJobPhase(env, job.id, 'finalize', FRESH_INVOCATION_YIELD_SECONDS);
return;
}
+ // If the only thing left is in-flight async batches (no synchronous work remains to advance),
+ // poll again after a short delay rather than immediately re-running. Bound the polling with the
+ // shared continuation ceiling: markJobContinuationQueued returns the post-increment count, and
+ // because a pending-only chunk never reset it (terminalProgress === 0), a batch that never
+ // completes will eventually cross MAX_JOB_CONTINUATIONS and degrade to a partial review instead
+ // of polling forever.
+ if (awaitingAsync > 0 && terminalProgress === 0) {
+ const pollCount = await markJobContinuationQueued(env, job.id, ASYNC_BATCH_POLL_DELAY_SECONDS);
+ if (pollCount > MAX_JOB_CONTINUATIONS) {
+ logger.error(`Async batch reviews did not complete after ${pollCount} polls; degrading to a partial review: ${job.owner}/${job.repo} PR #${job.prNumber}`);
+ for (const review of latestReviews.filter(isAwaitingAsyncReview)) {
+ await persistFailedFileReview(env, job.id, {
+ filePath: review.file_path,
+ modelUsed: review.async_model ?? review.model_used,
+ diffLineCount: review.diff_line_count,
+ errorMessage: 'Async batch review did not complete in time.',
+ clearAsync: true,
+ });
+ }
+ await updateJobStep(env, job.id, 'Reviewing Files', { status: 'done' });
+ throw new NextPhaseError('finalize', FRESH_INVOCATION_YIELD_SECONDS);
+ }
+ throw new NextPhaseError('review', ASYNC_BATCH_POLL_DELAY_SECONDS);
+ }
+
if (job.checkRunId) {
- await github.updateCheckRun(job.owner, job.repo, job.checkRunId, {
- title: `Reviewing (${completedCount}/${files.length})`,
- summary: 'Codra is continuing this review in the next queue chunk.',
- });
+ // Best-effort progress cosmetics only: the file reviews for this chunk are already
+ // persisted, so a failure here (e.g. this invocation's subrequest budget is spent) must
+ // not stop us from enqueuing the next chunk that finishes the job.
+ try {
+ await github.updateCheckRun(job.owner, job.repo, job.checkRunId, {
+ title: `Reviewing (${completedCount}/${files.length})`,
+ summary: 'Codra is continuing this review in the next queue chunk.',
+ });
+ } catch (error) {
+ logger.warn(`Failed to update progress check run for job ${job.id}; continuing to the next chunk anyway`, error instanceof Error ? error : new Error(String(error)));
+ }
}
- await enqueueJobPhase(env, job.id, 'review');
+ // More files remain -- the next chunk needs a fresh subrequest budget, so yield long enough to
+ // force the workflow to hibernate into a new invocation rather than looping in this one (which
+ // would accumulate subrequests across chunks until the cap is hit).
+ await enqueueJobPhase(env, job.id, 'review', FRESH_INVOCATION_YIELD_SECONDS);
+}
+
+/**
+ * Persist a completed review produced by the async batch poll path. Mirrors the success branch of
+ * reviewAndPersistFile and clears the async bookkeeping columns so the row is terminal ('done').
+ */
+async function persistCompletedReview(
+ env: AppBindings,
+ job: PersistedReviewJob,
+ file: ReturnType[number],
+ response: {
+ modelUsed: string;
+ provider: string;
+ inputTokens: number;
+ outputTokens: number;
+ rawText: string;
+ userPrompt: string;
+ parsed: {
+ comments: ParsedReviewComment[];
+ verdict: 'approve' | 'comment';
+ fileSummary: string;
+ overallCorrectness?: string;
+ confidenceScore?: number;
+ };
+ },
+) {
+ await upsertFileReview(env, job.id, {
+ filePath: file.path,
+ fileStatus: 'done',
+ modelUsed: response.modelUsed,
+ modelProvider: response.provider,
+ diffLineCount: file.lineCount,
+ diffInput: response.userPrompt,
+ rawAiOutput: response.rawText,
+ parsedComments: response.parsed.comments,
+ inputTokens: response.inputTokens,
+ outputTokens: response.outputTokens,
+ durationMs: null,
+ verdict: response.parsed.verdict,
+ fileSummary: response.parsed.fileSummary,
+ overallCorrectness: response.parsed.overallCorrectness,
+ confidenceScore: response.parsed.confidenceScore,
+ errorMessage: null,
+ asyncRequestId: null,
+ asyncModel: null,
+ });
+}
+
+/**
+ * Persist a file review as terminally 'failed' with the shared "mostly-null" shape. Collapses the
+ * several near-identical failure upserts (transient-retry exhaustion, hard provider limit, async
+ * batch giving up, finalize backfilling missing files) into one place. `clearAsync` wipes the
+ * async batch bookkeeping columns for rows that had been submitted to the queue.
+ */
+async function persistFailedFileReview(
+ env: AppBindings,
+ jobId: string,
+ input: {
+ filePath: string;
+ modelUsed: string;
+ modelProvider?: string | null;
+ diffLineCount: number;
+ durationMs?: number | null;
+ errorMessage: string;
+ clearAsync?: boolean;
+ },
+) {
+ await upsertFileReview(env, jobId, {
+ filePath: input.filePath,
+ fileStatus: 'failed',
+ modelUsed: input.modelUsed,
+ modelProvider: input.modelProvider ?? null,
+ diffLineCount: input.diffLineCount,
+ diffInput: '',
+ rawAiOutput: null,
+ parsedComments: [],
+ inputTokens: null,
+ outputTokens: null,
+ durationMs: input.durationMs ?? null,
+ verdict: null,
+ fileSummary: null,
+ errorMessage: input.errorMessage,
+ ...(input.clearAsync ? { asyncRequestId: null, asyncModel: null } : {}),
+ });
}
async function reviewAndPersistFile(
@@ -626,6 +1093,22 @@ async function reviewAndPersistFile(
const modelId = config.model?.main ?? 'unconfigured';
const modelProvider = await resolveFailureModelProvider();
+ // Per-invocation subrequest pressure clears on the next Worker invocation, so do not count
+ // it as a per-file provider outage. Let the job-level no-progress continuation ceiling bound
+ // a genuinely wedged job while this file remains pending for the fresh-budget retry.
+ if (isSubrequestBudgetError(error)) {
+ logger.warn(`File review deferred for ${file.path}; subrequest budget will retry in a fresh invocation`, {
+ error: errorMessage,
+ });
+ Object.defineProperty(error, 'retryAfterSeconds', {
+ value: FRESH_INVOCATION_YIELD_SECONDS,
+ configurable: true,
+ });
+ throw error;
+ }
+
+ // Transient model/provider outages count against the file so a single unrecoverable file
+ // eventually becomes a partial-review failure instead of blocking the entire job forever.
if (isRetryableModelError(error)) {
const failureCount = await recordRetryableFileReviewFailure(env, job.id, {
filePath: file.path,
@@ -639,20 +1122,12 @@ async function reviewAndPersistFile(
if (failureCount >= MAX_RETRYABLE_FILE_REVIEW_FAILURES) {
const finalError = `Review skipped after ${failureCount} repeated model provider outages.`;
- await upsertFileReview(env, job.id, {
+ await persistFailedFileReview(env, job.id, {
filePath: file.path,
- fileStatus: 'failed',
modelUsed: modelId,
modelProvider,
diffLineCount: file.lineCount,
- diffInput: '',
- rawAiOutput: null,
- parsedComments: [],
- inputTokens: null,
- outputTokens: null,
durationMs: Date.now() - startedAt,
- verdict: null,
- fileSummary: null,
errorMessage: finalError,
});
logger.error(`File review failed permanently for ${file.path} after transient retries`, {
@@ -675,36 +1150,63 @@ async function reviewAndPersistFile(
logger.error(`File review failed for ${file.path}`, { error });
+ // A genuine provider allocation exhaustion (e.g. Cloudflare Workers AI daily free
+ // allocation, error 4006) won't clear by retrying within this job, so mark the file failed
+ // and let the PR review complete as a partial review. (Per-invocation subrequest limits are
+ // NOT hard limits -- they're handled as deferrals above and retried on a fresh budget.)
const isHardLimit =
- errorMessage.toLowerCase().includes('subrequest') ||
errorMessage.includes('4006') ||
errorMessage.toLowerCase().includes('allocation');
if (isHardLimit) {
- logger.warn(`File review hit hard limit for ${file.path}, marking as failed to allow partial PR review.`, { error: errorMessage });
+ logger.warn(`File review hit hard provider allocation limit for ${file.path}, marking as failed to allow partial PR review.`, { error: errorMessage });
// We don't throw here; we just fall through and let it be marked as failed
// so the PR review can continue and complete as a partial review.
}
- await upsertFileReview(env, job.id, {
+ await persistFailedFileReview(env, job.id, {
filePath: file.path,
- fileStatus: 'failed',
modelUsed: modelId,
modelProvider,
diffLineCount: file.lineCount,
- diffInput: '',
- rawAiOutput: null,
- parsedComments: [],
- inputTokens: null,
- outputTokens: null,
durationMs: Date.now() - startedAt,
- verdict: null,
- fileSummary: null,
errorMessage,
});
}
}
+/**
+ * Assemble and fire the anonymous per-review telemetry event. The shared aggregate fields
+ * (line/token counts, models, file extensions, duration) are computed once here; the three fields
+ * that differ between the success and all-failed paths are passed as `overrides`. Never throws.
+ */
+async function sendReviewTelemetry(
+ env: AppBindings,
+ job: PersistedReviewJob,
+ files: Array<{ path: string; lineCount: number }>,
+ reviews: Array<{ input_tokens: number | null; output_tokens: number | null; model_used: string }>,
+ overrides: { findingsReported: number; verdict: string; severityDistribution: Record },
+) {
+ try {
+ await sendTelemetryEvent(env, {
+ linesReviewed: files.reduce((sum, file) => sum + file.lineCount, 0),
+ inputTokens: reviews.reduce((sum, r) => sum + (r.input_tokens ?? 0), 0),
+ outputTokens: reviews.reduce((sum, r) => sum + (r.output_tokens ?? 0), 0),
+ modelsUsed: Array.from(new Set(reviews.map((r) => r.model_used).filter(Boolean))),
+ fileExtensions: Array.from(new Set(files.map((f) => {
+ const parts = f.path.split('.');
+ return parts.length > 1 ? parts.pop() || '' : '';
+ }).filter(Boolean))),
+ triggerType: job.trigger,
+ reviewDurationMs: Math.max(0, Date.now() - new Date(job.createdAt).getTime()),
+ filesReviewed: files.length,
+ ...overrides,
+ });
+ } catch (e) {
+ logger.error('Failed to send telemetry', e instanceof Error ? e : new Error(String(e)));
+ }
+}
+
async function runFinalizePhase(
env: AppBindings,
job: PersistedReviewJob,
@@ -716,14 +1218,34 @@ async function runFinalizePhase(
const pr = await github.getPullRequest(job.owner, job.repo, job.prNumber);
const config = (job.configSnapshot ?? defaultRepoConfig) as RepoConfig;
- const rawDiff = await github.getPullRequestDiff(job.owner, job.repo, job.prNumber);
- const files = filterReviewableFiles(parseUnifiedDiff(rawDiff), config.review);
- const reviews = await getFileReviewsForJobs(env, [job.id]);
+ const files = await getDiffFiles(env, job, github, config);
+ let reviews = await getFileReviewsForJobs(env, [job.id]);
if (reviews.length < files.length) {
- await updateJobStep(env, job.id, 'Reviewing Files', { status: 'running' });
- await enqueueJobPhase(env, job.id, 'review');
- return;
+ const reviewedPaths = new Set(reviews.map((r) => r.file_path));
+ const missingFiles = files.filter((f) => !reviewedPaths.has(f.path));
+
+ if (missingFiles.length > 0) {
+ logger.warn(`Job ${job.id} reached finalize phase with ${missingFiles.length} missing file reviews. Forcing them to failed state.`);
+ // Batch the backfill into one INSERT. Doing it per-file (a transaction each) scales the
+ // subrequest cost with the number of missing files and, on a large/growing PR, exhausts the
+ // per-invocation budget right before the review is posted (finalize can't safely hibernate --
+ // it posts the GitHub review -- so it must stay within one invocation's budget).
+ await bulkMarkFilesFailed(
+ env,
+ job.id,
+ missingFiles.map((file) => ({ filePath: file.path, diffLineCount: file.lineCount })),
+ { modelUsed: config.model?.main ?? 'unconfigured', errorMessage: 'Review could not be completed after repeated infrastructure limits' },
+ );
+
+ // Refresh reviews list after inserting the missing ones
+ reviews = await getFileReviewsForJobs(env, [job.id]);
+ } else {
+ await updateJobStep(env, job.id, 'Reviewing Files', { status: 'running' });
+ // Bounce back to review on a fresh invocation/budget (finalize already spent this one).
+ await enqueueJobPhase(env, job.id, 'review', FRESH_INVOCATION_YIELD_SECONDS);
+ return;
+ }
}
const reviewedComments = reviews.flatMap((review) => review.parsed_comments as ParsedReviewComment[]);
@@ -737,34 +1259,12 @@ async function runFinalizePhase(
if (fileSummaries.length > 0 && fileSummaries.every((file) => file.verdict === 'failed')) {
await updateJobStep(env, job.id, 'Generating Summary', { status: 'failed', error: 'All files failed to review' });
-
- // Send anonymous telemetry for the failed job
- const fileInputTokens = reviews.reduce((sum, review) => sum + (review.input_tokens ?? 0), 0);
- const fileOutputTokens = reviews.reduce((sum, review) => sum + (review.output_tokens ?? 0), 0);
- const modelsUsed = Array.from(new Set(reviews.map(r => r.model_used).filter(Boolean)));
- const fileExtensions = Array.from(new Set(files.map(f => {
- const parts = f.path.split('.');
- return parts.length > 1 ? parts.pop() || '' : '';
- }).filter(Boolean)));
- const reviewDurationMs = Math.max(0, Date.now() - new Date(job.createdAt).getTime());
-
- try {
- await sendTelemetryEvent(env, {
- linesReviewed: files.reduce((sum, file) => sum + file.lineCount, 0),
- findingsReported: 0,
- inputTokens: fileInputTokens,
- outputTokens: fileOutputTokens,
- modelsUsed,
- fileExtensions,
- triggerType: job.trigger,
- reviewDurationMs,
- filesReviewed: files.length,
- verdict: 'failed',
- severityDistribution: {},
- });
- } catch (e) {
- logger.error('Failed to send telemetry', e instanceof Error ? e : new Error(String(e)));
- }
+
+ await sendReviewTelemetry(env, job, files, reviews, {
+ findingsReported: 0,
+ verdict: 'failed',
+ severityDistribution: {},
+ });
throw new Error('All files failed to review');
}
@@ -773,13 +1273,15 @@ async function runFinalizePhase(
const failedFileCount = fileSummaries.filter((file) => file.verdict === 'failed').length;
const severityRanks: Record = { P0: 0, P1: 1, P2: 2, P3: 3, nit: 4 };
const minRank = severityRanks[config.review.min_severity] ?? 4;
-
+ const { maxComments: globalMaxComments } = await getReviewSettings(env);
+ const effectiveMaxComments = Math.min(config.review.max_comments, globalMaxComments);
+
let finalComments = reviewedComments.filter(c => (severityRanks[c.severity] ?? 4) <= minRank);
finalComments.sort((a, b) => (severityRanks[a.severity] ?? 4) - (severityRanks[b.severity] ?? 4));
-
- const omittedCount = reviewedComments.length - Math.min(finalComments.length, config.review.max_comments);
- if (finalComments.length > config.review.max_comments) {
- finalComments = finalComments.slice(0, config.review.max_comments);
+
+ const omittedCount = reviewedComments.length - Math.min(finalComments.length, effectiveMaxComments);
+ if (finalComments.length > effectiveMaxComments) {
+ finalComments = finalComments.slice(0, effectiveMaxComments);
}
const verdictSummary = formatter.summarizeVerdict(finalComments, hasFailures);
@@ -787,9 +1289,9 @@ async function runFinalizePhase(
await heartbeatAndCheckSuperseded(env, job.id, leaseOwner);
let formattedSummary = formatter.formatReviewOverview(pr.head.sha, env.BOT_USERNAME);
-
+
if (omittedCount > 0) {
- formattedSummary += `\n\n> [!NOTE]\n> **${omittedCount} comments were omitted** from this review to reduce noise and respect the configured \`max_comments\` limit (${config.review.max_comments}). Showing the most critical issues.`;
+ formattedSummary += `\n\n> [!NOTE]\n> **${omittedCount} comments were omitted** from this review to reduce noise and respect the configured \`max_comments\` limit (${effectiveMaxComments}). Showing the most critical issues.`;
}
await updateJobStep(env, job.id, 'Completing', { status: 'running' });
@@ -804,43 +1306,8 @@ async function runFinalizePhase(
})),
});
- if (config.review.labels !== false) {
- const labels = config.review.labels;
- const labelMap = {
- comment: { name: labels.p1, color: 'f79009' },
- approve: { name: labels.p2, color: '027a48' },
- } as const;
- const label = labelMap[verdictSummary.verdict];
-
- await github.removeIssueLabelsIfPresent(
- job.owner,
- job.repo,
- job.prNumber,
- [labels.p1, labels.p2, labels.p3].filter(possibleLabel => possibleLabel !== label.name),
- );
-
- await github.ensureLabel(job.owner, job.repo, label.name, label.color);
- await github.addIssueLabels(job.owner, job.repo, job.prNumber, [label.name]);
- }
-
- if (job.checkRunId) {
- await github.updateCheckRun(job.owner, job.repo, job.checkRunId, {
- status: 'completed',
- conclusion: hasFailures ? 'failure' : (verdictSummary.verdict === 'approve' ? 'success' : 'neutral'),
- title: hasFailures ? 'Review partially failed' : (verdictSummary.verdict === 'approve' ? 'LGTM' : 'Comments posted'),
- summary: `${finalComments.length} inline comments across ${files.length} files.${hasFailures ? ` ${failedFileCount} file${failedFileCount === 1 ? '' : 's'} could not be reviewed after repeated provider outages.` : ''}`,
- });
- }
-
const fileInputTokens = reviews.reduce((sum, review) => sum + (review.input_tokens ?? 0), 0);
const fileOutputTokens = reviews.reduce((sum, review) => sum + (review.output_tokens ?? 0), 0);
-
- const modelsUsed = Array.from(new Set(reviews.map(r => r.model_used).filter(Boolean)));
- const fileExtensions = Array.from(new Set(files.map(f => {
- const parts = f.path.split('.');
- return parts.length > 1 ? parts.pop() || '' : '';
- }).filter(Boolean)));
- const reviewDurationMs = Math.max(0, Date.now() - new Date(job.createdAt).getTime());
const severityDistribution: Record = {};
for (const comment of finalComments) {
@@ -851,6 +1318,10 @@ async function runFinalizePhase(
const partialErrorMessage = hasFailures
? `Partial review: ${failedFileCount} of ${files.length} file${files.length === 1 ? '' : 's'} could not be reviewed after repeated model/provider outages.`
: null;
+ // Record the posted review and mark the job done IMMEDIATELY after createReview -- before the
+ // best-effort cosmetics below. The review is already on GitHub at this point; if the remaining
+ // label/check-run calls exhaust this invocation's subrequest budget (large PR), we must not leave
+ // the job stranded as 'failed' with review_id null. This is the critical, must-not-lose write.
await completeJob(env, job.id, {
verdict: verdictSummary.verdict,
fileCount: files.length,
@@ -864,24 +1335,52 @@ async function runFinalizePhase(
});
logger.info(`Review job completed: ${job.owner}/${job.repo} PR #${job.prNumber}`);
- // Send anonymous telemetry
+ // Cosmetics: labels and the check-run conclusion. Best-effort -- the review is posted and the job
+ // is already 'done', so a failure here (e.g. subrequest budget spent, GitHub blip) must not fail
+ // the job. completeTerminalCheckRuns / a re-run can reconcile a check run left un-updated.
try {
- await sendTelemetryEvent(env, {
- linesReviewed: files.reduce((sum, file) => sum + file.lineCount, 0),
- findingsReported: finalComments.length,
- inputTokens: fileInputTokens,
- outputTokens: fileOutputTokens,
- modelsUsed,
- fileExtensions,
- triggerType: job.trigger,
- reviewDurationMs,
- filesReviewed: files.length,
- verdict: verdictSummary.verdict,
- severityDistribution,
- });
- } catch (e) {
- logger.error('Failed to send telemetry', e instanceof Error ? e : new Error(String(e)));
+ // Check-run conclusion first: it drives the PR's status badge, so it matters more than labels
+ // if the budget only allows one of them.
+ if (job.checkRunId) {
+ await github.updateCheckRun(job.owner, job.repo, job.checkRunId, {
+ status: 'completed',
+ conclusion: hasFailures ? 'failure' : (verdictSummary.verdict === 'approve' ? 'success' : 'neutral'),
+ title: hasFailures ? 'Review partially failed' : (verdictSummary.verdict === 'approve' ? 'LGTM' : 'Comments posted'),
+ summary: `${finalComments.length} inline comments across ${files.length} files.${hasFailures ? ` ${failedFileCount} file${failedFileCount === 1 ? '' : 's'} could not be reviewed after repeated provider outages.` : ''}`,
+ });
+ // Only now is the check run genuinely completed -- record it so the maintenance sweep doesn't
+ // redo it. If the update above threw, this line is skipped and completeTerminalCheckRuns will
+ // finish the check run on a later invocation with a fresh budget.
+ await markJobCheckRunCompleted(env, job.id);
+ }
+
+ if (config.review.labels !== false) {
+ const labels = config.review.labels;
+ const labelMap = {
+ comment: { name: labels.p1, color: 'f79009' },
+ approve: { name: labels.p2, color: '027a48' },
+ } as const;
+ const label = labelMap[verdictSummary.verdict];
+
+ await github.removeIssueLabelsIfPresent(
+ job.owner,
+ job.repo,
+ job.prNumber,
+ [labels.p1, labels.p2, labels.p3].filter(possibleLabel => possibleLabel !== label.name),
+ );
+
+ await github.ensureLabel(job.owner, job.repo, label.name, label.color);
+ await github.addIssueLabels(job.owner, job.repo, job.prNumber, [label.name]);
+ }
+ } catch (error) {
+ logger.warn(`Post-review labels/check-run update failed for job ${job.id}; review is posted and job is completed, so leaving it best-effort`, error instanceof Error ? error : new Error(String(error)));
}
+
+ await sendReviewTelemetry(env, job, files, reviews, {
+ findingsReported: finalComments.length,
+ verdict: verdictSummary.verdict,
+ severityDistribution,
+ });
}
async function heartbeatAndCheckSuperseded(env: AppBindings, jobId: string, leaseOwner: string) {
@@ -912,14 +1411,56 @@ function hasCompletedStep(job: PersistedReviewJob, stepName: string) {
return job.steps.some((step) => step.name === stepName && step.status === 'done');
}
-async function failJobAndCheckRun(
+function diffCacheKey(jobId: string) {
+ return `diff:${jobId}`;
+}
+
+/**
+ * Returns the job's reviewable files, fetching and parsing the PR diff from
+ * GitHub only once per job (cached in KV) instead of once per phase invocation.
+ */
+export async function getDiffFiles(
env: AppBindings,
- job: PersistedReviewJob,
- github: GitHubService,
+ job: Pick,
+ github: Pick,
+ config: RepoConfig,
+) {
+ const cacheKey = diffCacheKey(job.id);
+ let rawDiff = await env.APP_KV.get(cacheKey);
+
+ if (!rawDiff) {
+ rawDiff = await github.getPullRequestDiff(job.owner, job.repo, job.prNumber);
+ try {
+ await env.APP_KV.put(cacheKey, rawDiff, { expirationTtl: DIFF_CACHE_TTL_SECONDS });
+ } catch (error) {
+ logger.warn(`Failed to cache PR diff for job ${job.id}; it will be re-fetched on the next phase`, error instanceof Error ? error : new Error(String(error)));
+ }
+ }
+
+ return filterReviewableFiles(parseUnifiedDiff(rawDiff, config.review), config.review);
+}
+
+export async function failJobAndCheckRun(
+ env: AppBindings,
+ job: Pick,
+ github: Pick,
message: string,
) {
+ // Marking the job failed in the DB is the critical, must-not-lose write: it's what
+ // makes the job terminal so it stops being retried, and it's what makes it eligible
+ // for completeTerminalCheckRuns() to pick up later if the GitHub call below fails.
try {
await failJob(env, job.id, message);
+ } catch (dbError) {
+ logger.error(`Critical: failed to mark job ${job.id} as failed in the DB; it may remain stuck until lease-expiry recovery reclaims it`, dbError);
+ return;
+ }
+
+ // Updating the GitHub check run is best-effort here. If it fails (e.g. the Worker's
+ // subrequest budget for this invocation is already exhausted from the review itself),
+ // the job is still durably marked failed above, and the opportunistic maintenance sweep
+ // (completeTerminalCheckRuns) will retry this update on a later invocation with a fresh budget.
+ try {
const latest = await getJobForProcessing(env, job.id);
const checkRunId = latest?.check_run_id ?? job.checkRunId;
if (checkRunId) {
@@ -931,7 +1472,7 @@ async function failJobAndCheckRun(
});
await markJobCheckRunCompleted(env, job.id);
}
- } catch (innerError) {
- logger.error('Failed to record job failure in DB/GitHub', innerError);
+ } catch (checkRunError) {
+ logger.warn(`Failed to update GitHub check run for failed job ${job.id}; opportunistic maintenance will retry it`, checkRunError);
}
}
diff --git a/src/server/core/telemetry.ts b/src/server/core/telemetry.ts
index 95c12fd..28be312 100644
--- a/src/server/core/telemetry.ts
+++ b/src/server/core/telemetry.ts
@@ -58,9 +58,18 @@ export async function sendTelemetryEvent(
},
): Promise {
try {
+ // Opt-out for self-hosters/forks: set TELEMETRY_DISABLED=true (or 1) to send nothing.
+ const disabled = String((env as any).TELEMETRY_DISABLED ?? '').toLowerCase();
+ if (disabled === 'true' || disabled === '1') {
+ return;
+ }
+
const instanceId = await getInstanceId(env);
// Use an environment variable if available, otherwise default to the hosted backend
const telemetryUrl = (env as any).TELEMETRY_API_URL ?? 'https://codra.run/api/telemetry';
+ // Allow the ingestion secret to be overridden via env so it isn't pinned to the value committed
+ // in this (public) source tree.
+ const telemetrySecret = (env as any).TELEMETRY_SECRET ?? TELEMETRY_SECRET;
// Fire and forget using standard fetch with a timeout
const controller = new AbortController();
@@ -70,7 +79,7 @@ export async function sendTelemetryEvent(
method: 'POST',
headers: {
'Content-Type': 'application/json',
- 'Authorization': `Bearer ${TELEMETRY_SECRET}`,
+ 'Authorization': `Bearer ${telemetrySecret}`,
},
body: JSON.stringify({
instanceId,
diff --git a/src/server/core/token-tracker.ts b/src/server/core/token-tracker.ts
index c7951f1..74a6397 100644
--- a/src/server/core/token-tracker.ts
+++ b/src/server/core/token-tracker.ts
@@ -14,7 +14,10 @@ export class TokenTracker {
private usage: Map = new Map();
private subrequests = 0;
private readonly MAX_SUBREQUESTS = 50;
- private readonly SAFE_MARGIN = 22; // Increased even further to handle finalization overhead
+ // Increased from 10 to 25 to account for the untracked Hyperdrive queries that happen per chunk
+ // (e.g. lease heartbeats, getting reviews, upserting reviews, etc.) which can easily add ~15
+ // subrequests that the tracker doesn't see.
+ private readonly SAFE_MARGIN = 25;
incrementSubrequests(count = 1) {
this.subrequests += count;
@@ -32,6 +35,18 @@ export class TokenTracker {
return this.subrequests >= this.MAX_SUBREQUESTS - this.SAFE_MARGIN;
}
+ /**
+ * How many more subrequests can safely be spent right now before crossing into the
+ * reserved safety margin below Cloudflare's hard per-invocation cap (Workers Free plan:
+ * 50 subrequests/invocation). Callers that can start a variable amount of concurrent work
+ * (e.g. how many files to review at once) should size that work against this number
+ * instead of a fixed constant, so throughput stays high while the budget is healthy and
+ * automatically shrinks as it's spent.
+ */
+ remainingSafeBudget() {
+ return Math.max(0, this.MAX_SUBREQUESTS - this.SAFE_MARGIN - this.subrequests);
+ }
+
/**
* Records token usage for a specific model call.
*/
diff --git a/src/server/db/app-settings.ts b/src/server/db/app-settings.ts
new file mode 100644
index 0000000..5c496e6
--- /dev/null
+++ b/src/server/db/app-settings.ts
@@ -0,0 +1,48 @@
+import type { AppBindings } from '@server/env';
+import { queryRows } from './client';
+import { logger } from '@server/core/logger';
+import { reviewConcurrencyLevels, reviewMaxCommentsOptions, reviewSettingsSchema, type ReviewSettings } from '@shared/schema';
+
+const CONCURRENCY_KEY = 'review_concurrency_level';
+const MAX_COMMENTS_KEY = 'review_max_comments';
+
+const DEFAULT_REVIEW_SETTINGS: ReviewSettings = reviewSettingsSchema.parse({});
+const CONCURRENCY_LEVELS = new Set(reviewConcurrencyLevels);
+const MAX_COMMENTS_OPTIONS = new Set(reviewMaxCommentsOptions);
+
+export async function getReviewSettings(env: Pick): Promise {
+ try {
+ const rows = await queryRows<{ key: string; value: string }>(
+ env,
+ 'SELECT key, value FROM global_settings WHERE key = ANY($1)',
+ [[CONCURRENCY_KEY, MAX_COMMENTS_KEY]],
+ );
+ const map = new Map(rows.map((row) => [row.key, row.value]));
+ const storedConcurrency = map.get(CONCURRENCY_KEY);
+ const storedMaxComments = map.get(MAX_COMMENTS_KEY);
+ const parsedMaxComments = storedMaxComments === undefined ? NaN : Number(storedMaxComments);
+
+ return reviewSettingsSchema.parse({
+ concurrencyLevel: storedConcurrency && CONCURRENCY_LEVELS.has(storedConcurrency)
+ ? storedConcurrency
+ : DEFAULT_REVIEW_SETTINGS.concurrencyLevel,
+ maxComments: MAX_COMMENTS_OPTIONS.has(parsedMaxComments)
+ ? parsedMaxComments
+ : DEFAULT_REVIEW_SETTINGS.maxComments,
+ });
+ } catch (error) {
+ logger.warn('Failed to load review settings, using defaults', {
+ error: error instanceof Error ? error.message : String(error),
+ });
+ return DEFAULT_REVIEW_SETTINGS;
+ }
+}
+
+export async function updateReviewSettings(env: Pick, settings: ReviewSettings): Promise {
+ await queryRows(
+ env,
+ `INSERT INTO global_settings (key, value) VALUES ($1, $2), ($3, $4)
+ ON CONFLICT (key) DO UPDATE SET value = EXCLUDED.value`,
+ [CONCURRENCY_KEY, settings.concurrencyLevel, MAX_COMMENTS_KEY, String(settings.maxComments)],
+ );
+}
diff --git a/src/server/db/client.ts b/src/server/db/client.ts
index 7a41254..ab4341e 100644
--- a/src/server/db/client.ts
+++ b/src/server/db/client.ts
@@ -62,8 +62,29 @@ export function runWithDb(env: DbEnv, fn: () => T): T {
return dbStorage.run(createDbClient(env), fn);
}
+// Reused clients for the no-runWithDb() fallback path below. Keyed by connection string so a caller
+// that queries outside a runWithDb() scope shares one bounded pool instead of leaking a fresh pool
+// (up to `max` connections) on every single query.
+const fallbackClients = new Map();
+
export function getDb(env: DbEnv) {
- return dbStorage.getStore() ?? createDbClient(env);
+ const store = dbStorage.getStore();
+ if (store) return store;
+
+ // Outside a runWithDb() scope. In production this never happens -- fetch, queue, scheduled, and
+ // the workflow all wrap their work in runWithDb() -- so this branch is effectively test-only (the
+ // Hono test client calls app.fetch() directly, and tests call db helpers directly). Creating a new
+ // pool per query there leaked connections across the whole test process and exhausted CI's Postgres
+ // (masked locally by Neon's pooler). Reuse one client per connection string instead. Keeping this
+ // strictly to the store-less path preserves the per-request client production relies on to satisfy
+ // Cloudflare's "no I/O across request contexts" rule.
+ const connectionString = env.HYPERDRIVE.connectionString;
+ let client = fallbackClients.get(connectionString);
+ if (!client) {
+ client = createDbClient(env);
+ fallbackClients.set(connectionString, client);
+ }
+ return client;
}
export async function queryRows(env: DbEnv, sqlText: string, params: unknown[] = []) {
diff --git a/src/server/db/file-reviews.ts b/src/server/db/file-reviews.ts
index 641d4f9..1877d4f 100644
--- a/src/server/db/file-reviews.ts
+++ b/src/server/db/file-reviews.ts
@@ -111,6 +111,10 @@ export async function upsertFileReview(
overallCorrectness?: string | null;
confidenceScore?: number | null;
errorMessage: string | null;
+ // Async batch bookkeeping: set when a review is submitted to the Workers AI queue (status
+ // 'pending'), cleared (null) once the batch completes and a terminal review is persisted.
+ asyncRequestId?: string | null;
+ asyncModel?: string | null;
},
) {
await queryTransaction(env, async (tx) => {
@@ -132,9 +136,11 @@ export async function upsertFileReview(
overall_correctness,
confidence_score,
error_msg,
- model_provider
+ model_provider,
+ async_request_id,
+ async_model
)
- VALUES ($1::uuid, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12, $13, $14, $15, $16)
+ VALUES ($1::uuid, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12, $13, $14, $15, $16, $17, $18)
ON CONFLICT (job_id, file_path) DO UPDATE SET
file_status = EXCLUDED.file_status,
model_used = EXCLUDED.model_used,
@@ -150,6 +156,8 @@ export async function upsertFileReview(
confidence_score = EXCLUDED.confidence_score,
error_msg = EXCLUDED.error_msg,
model_provider = EXCLUDED.model_provider,
+ async_request_id = EXCLUDED.async_request_id,
+ async_model = EXCLUDED.async_model,
transient_error_count = 0
RETURNING id
`,
@@ -170,6 +178,8 @@ export async function upsertFileReview(
input.confidenceScore ?? null,
input.errorMessage,
input.modelProvider ?? null,
+ input.asyncRequestId ?? null,
+ input.asyncModel ?? null,
],
);
@@ -270,151 +280,110 @@ export async function recordRetryableFileReviewFailure(
});
}
-export async function getModelUsageStats(env: Pick) {
- return queryRows<{
- model_used: string;
- model_provider: string | null;
- calls: number;
- input_tokens: number | null;
- output_tokens: number | null;
- }>(
- env,
- `
- SELECT
- model_used,
- MIN(model_provider) AS model_provider,
- COUNT(*)::int AS calls,
- COALESCE(SUM(input_tokens), 0)::int AS input_tokens,
- COALESCE(SUM(output_tokens), 0)::int AS output_tokens
- FROM file_reviews
- GROUP BY model_used
- ORDER BY calls DESC, model_used ASC
- `,
- );
-}
-
-export async function batchInsertFileReviews(
+/**
+ * Copy every still-needed, inheritable parent review (and its comments) into `jobId` in a single
+ * cheap DB pass. A retry that can reuse the parent's completed reviews would otherwise re-persist
+ * them one file at a time through the budget-limited review loop (~5 files/chunk, hibernating a
+ * fresh invocation between chunks), turning a fully-inheritable retry into a many-minute crawl.
+ * This collapses all of them into one transaction (a couple of subrequests total, regardless of
+ * file count) so the review phase finishes in a single invocation. `filePaths` must already be
+ * filtered to files that are inheritable under the current model strategy and have no row yet in
+ * the target job. Returns the file paths that were actually inserted.
+ */
+export async function bulkInheritFileReviews(
env: Pick,
- jobId: string,
- reviews: Array<{
- filePath: string;
- fileStatus: 'pending' | 'done' | 'skipped' | 'failed';
- modelUsed: string;
- modelProvider?: string | null;
- diffLineCount: number;
- diffInput: string | null;
- rawAiOutput: string | null;
- parsedComments: ParsedReviewComment[];
- inputTokens: number | null;
- outputTokens: number | null;
- durationMs: number | null;
- verdict: 'approve' | 'comment' | null;
- fileSummary: string | null;
- overallCorrectness?: string | null;
- confidenceScore?: number | null;
- errorMessage: string | null;
- }>,
-) {
- if (reviews.length === 0) return;
+ input: { jobId: string; parentJobId: string; filePaths: string[] },
+): Promise {
+ if (input.filePaths.length === 0) return [];
- // 1. Insert file reviews and get their IDs
- // Since we want to handle comments too, we'll do this in a single query with UNNEST and RETURNING
- const filePaths = reviews.map(r => r.filePath);
- const fileStatuses = reviews.map(r => r.fileStatus);
- const modelsUsed = reviews.map(r => r.modelUsed);
- const diffLineCounts = reviews.map(r => r.diffLineCount);
- const diffInputs = reviews.map(r => r.diffInput);
- const rawAiOutputs = reviews.map(r => r.rawAiOutput);
- const inputTokens = reviews.map(r => r.inputTokens);
- const outputTokens = reviews.map(r => r.outputTokens);
- const durationMs = reviews.map(r => r.durationMs);
- const verdicts = reviews.map(r => r.verdict);
- const fileSummaries = reviews.map(r => r.fileSummary);
- const overallCorrectness = reviews.map(r => r.overallCorrectness ?? null);
- const confidenceScores = reviews.map(r => r.confidenceScore ?? null);
- const errorMessages = reviews.map(r => r.errorMessage);
- const modelProviders = reviews.map(r => r.modelProvider ?? null);
-
- await queryTransaction(env, async (tx) => {
- const insertedRows = await tx.query<{ id: string; file_path: string }>(
+ return await queryTransaction(env, async (tx) => {
+ const inserted = await tx.query<{ id: string; file_path: string }>(
`
INSERT INTO file_reviews (
job_id, file_path, file_status, model_used, diff_line_count, diff_input,
raw_ai_output, input_tokens, output_tokens, duration_ms, verdict,
file_summary, overall_correctness, confidence_score, error_msg, model_provider
)
- SELECT $1::uuid, * FROM UNNEST(
- $2::text[], $3::text[], $4::text[], $5::int[], $6::text[],
- $7::text[], $8::int[], $9::int[], $10::int[], $11::text[],
- $12::text[], $13::text[], $14::real[], $15::text[], $16::text[]
- )
+ SELECT $1::uuid, file_path, file_status, model_used, diff_line_count, diff_input,
+ raw_ai_output, input_tokens, output_tokens, duration_ms, verdict,
+ file_summary, overall_correctness, confidence_score, error_msg, model_provider
+ FROM file_reviews
+ WHERE job_id = $2::uuid AND file_status = 'done' AND file_path = ANY($3::text[])
+ ON CONFLICT (job_id, file_path) DO NOTHING
RETURNING id, file_path
`,
- [
- jobId, filePaths, fileStatuses, modelsUsed, diffLineCounts, diffInputs,
- rawAiOutputs, inputTokens, outputTokens, durationMs, verdicts,
- fileSummaries, overallCorrectness, confidenceScores, errorMessages, modelProviders
- ]
+ [input.jobId, input.parentJobId, input.filePaths],
);
- const allComments: Array<{
- fileReviewId: string;
- path: string;
- line: number | null;
- position: number | null;
- severity: string;
- category: string;
- title: string;
- body: string;
- codeSuggestion: string | null;
- }> = [];
-
- for (const review of reviews) {
- const inserted = insertedRows.find(r => r.file_path === review.filePath);
- if (!inserted || review.parsedComments.length === 0) continue;
-
- for (const comment of review.parsedComments) {
- allComments.push({
- fileReviewId: inserted.id,
- path: comment.path,
- line: comment.line ?? null,
- position: comment.position ?? null,
- severity: comment.severity,
- category: comment.category,
- title: comment.title,
- body: comment.body,
- codeSuggestion: comment.codeSuggestion ?? null,
- });
- }
- }
-
- if (allComments.length > 0) {
+ if (inserted.length > 0) {
+ // Re-attach each inherited review's comments, mapping the parent's rows to the new ids by path.
await tx.query(
`
INSERT INTO review_comments (
file_review_id, path, line, position, severity, category, title, body, code_suggestion
)
- SELECT * FROM UNNEST(
- $1::uuid[], $2::text[], $3::int[], $4::int[], $5::text[], $6::text[], $7::text[], $8::text[], $9::text[]
- )
+ SELECT nw.new_id, rc.path, rc.line, rc.position, rc.severity, rc.category, rc.title, rc.body, rc.code_suggestion
+ FROM UNNEST($1::uuid[], $2::text[]) AS nw(new_id, file_path)
+ JOIN file_reviews pf ON pf.job_id = $3::uuid AND pf.file_path = nw.file_path
+ JOIN review_comments rc ON rc.file_review_id = pf.id
`,
- [
- allComments.map(c => c.fileReviewId),
- allComments.map(c => c.path),
- allComments.map(c => c.line),
- allComments.map(c => c.position),
- allComments.map(c => c.severity),
- allComments.map(c => c.category),
- allComments.map(c => c.title),
- allComments.map(c => c.body),
- allComments.map(c => c.codeSuggestion),
- ]
+ [inserted.map((r) => r.id), inserted.map((r) => r.file_path), input.parentJobId],
);
}
+
+ return inserted.map((r) => r.file_path);
});
}
+/**
+ * Mark many files 'failed' in a single INSERT. Finalize backfills reviews for files that never got
+ * one (e.g. files that appeared mid-review, or unrecoverable ones); doing that one-by-one through
+ * upsertFileReview runs a transaction per file (several Hyperdrive round-trips each), which for a
+ * large/growing PR can blow the per-invocation subrequest budget right before the review is posted.
+ * This collapses the whole backfill into one statement (one subrequest). Skips files that already
+ * have a row (ON CONFLICT DO NOTHING) so it never clobbers a real review.
+ */
+export async function bulkMarkFilesFailed(
+ env: Pick,
+ jobId: string,
+ files: Array<{ filePath: string; diffLineCount: number }>,
+ opts: { modelUsed: string; errorMessage: string },
+): Promise {
+ if (files.length === 0) return;
+ await queryRows(
+ env,
+ `
+ INSERT INTO file_reviews (job_id, file_path, file_status, model_used, diff_line_count, diff_input, error_msg, duration_ms)
+ SELECT $1::uuid, u.file_path, 'failed', $2, u.diff_line_count, '', $3, 0
+ FROM UNNEST($4::text[], $5::int[]) AS u(file_path, diff_line_count)
+ ON CONFLICT (job_id, file_path) DO NOTHING
+ `,
+ [jobId, opts.modelUsed, opts.errorMessage, files.map((f) => f.filePath), files.map((f) => f.diffLineCount)],
+ );
+}
+export async function getModelUsageStats(env: Pick) {
+ return queryRows<{
+ model_used: string;
+ model_provider: string | null;
+ calls: number;
+ input_tokens: number | null;
+ output_tokens: number | null;
+ }>(
+ env,
+ `
+ SELECT
+ model_used,
+ MIN(model_provider) AS model_provider,
+ COUNT(*)::int AS calls,
+ COALESCE(SUM(input_tokens), 0)::int AS input_tokens,
+ COALESCE(SUM(output_tokens), 0)::int AS output_tokens
+ FROM file_reviews
+ GROUP BY model_used
+ ORDER BY calls DESC, model_used ASC
+ `,
+ );
+}
export async function getFileReviewsForJobs(env: Pick, jobIds: string[]) {
if (jobIds.length === 0) return [];
@@ -439,6 +408,8 @@ export async function getFileReviewsForJobs(env: Pick
error_msg: string | null;
model_provider: string | null;
transient_error_count: number;
+ async_request_id: string | null;
+ async_model: string | null;
}>(
env,
`
diff --git a/src/server/db/jobs.ts b/src/server/db/jobs.ts
index 9f263b4..301277d 100644
--- a/src/server/db/jobs.ts
+++ b/src/server/db/jobs.ts
@@ -15,7 +15,7 @@ export type JobRow = {
commit_sha: ByteaValue;
base_sha: ByteaValue;
trigger: 'auto' | 'mention' | 'retry';
- status: 'queued' | 'running' | 'done' | 'failed' | 'superseded';
+ status: 'queued' | 'running' | 'done' | 'failed' | 'superseded' | 'cancelled';
config_snapshot: { review?: RepoConfig['review']; model?: RepoConfig['model'] } | string | null;
check_run_id: number | null;
check_run_completed_at: string | null;
@@ -26,6 +26,7 @@ export type JobRow = {
lease_expires_at: string | null;
heartbeat_at: string | null;
recovery_count: number | null;
+ continuation_count: number | null;
last_queue_message_at: string | null;
total_input_tokens: number | null;
total_output_tokens: number | null;
@@ -154,8 +155,66 @@ export async function setJobWorkflowInstance(env: Pick,
+ jobId: string,
+ meta: { prTitle: string | null; prAuthor: string | null },
+) {
+ await queryRows(
+ env,
+ `
+ UPDATE jobs
+ SET pr_title = $2,
+ pr_author = COALESCE($3, pr_author)
+ WHERE id = $1
+ `,
+ [jobId, meta.prTitle, meta.prAuthor],
+ );
+}
+
+export async function markSystemActive(env: Pick) {
+ try {
+ await env.APP_KV.put('system:active_jobs', '1', { expirationTtl: 20 * 60 });
+ } catch (error) {
+ // Ignore KV errors to avoid failing the DB transaction
+ }
+}
+
+export async function clearSystemActive(env: Pick) {
+ try {
+ await env.APP_KV.delete('system:active_jobs');
+ } catch (error) {
+ // Best-effort: the 20-minute TTL on the flag is the backstop if this delete fails.
+ }
+}
+
+/**
+ * Whether the scheduled maintenance loop still has anything to do: any job that could still be
+ * running/recoverable (queued/running) or any terminal job whose GitHub check run hasn't been
+ * completed yet. When this is false the cron can clear the `system:active_jobs` flag so subsequent
+ * ticks skip the DB entirely and the serverless Postgres is allowed to suspend.
+ */
+export async function hasPendingMaintenanceWork(env: Pick): Promise {
+ const rows = await queryRows<{ has_work: boolean }>(
+ env,
+ `
+ SELECT EXISTS (
+ SELECT 1 FROM jobs
+ WHERE status IN ('queued', 'running')
+ OR (status IN ('done', 'failed', 'superseded', 'cancelled') AND check_run_id IS NOT NULL AND check_run_completed_at IS NULL)
+ ) AS has_work
+ `,
+ );
+ return rows[0]?.has_work === true;
+}
+
+export async function insertJob(
+ env: Pick,
input: {
installationId: string;
owner: string;
@@ -218,6 +277,7 @@ export async function insertJob(
],
);
+ await markSystemActive(env);
return mapJob(row);
}
@@ -394,35 +454,8 @@ export async function getJobDetail(env: Pick, jobId:
});
}
-export async function startJobProcessing(env: Pick, jobId: string, stepName: string) {
- const now = new Date().toISOString();
- const rows = await queryRows<{ id: string }>(
- env,
- `
- UPDATE jobs
- SET status = 'running',
- started_at = COALESCE(started_at, now()),
- steps = COALESCE(steps, '[]'::jsonb) || jsonb_build_array(
- jsonb_build_object(
- 'name', $2::text,
- 'status', 'running',
- 'startedAt', $3::text,
- 'finishedAt', NULL,
- 'error', NULL
- )
- )
- WHERE id = $1
- AND status = 'queued'
- RETURNING id
- `,
- [jobId, stepName, now],
- );
-
- return rows.length > 0;
-}
-
export async function claimJobLease(
- env: Pick,
+ env: Pick,
jobId: string,
leaseOwner: string,
leaseSeconds: number,
@@ -461,6 +494,7 @@ export async function claimJobLease(
);
if (claimed) {
+ await markSystemActive(env);
return { status: 'claimed', row: claimed };
}
@@ -485,7 +519,7 @@ export async function claimJobLease(
}
export async function heartbeatJobLease(
- env: Pick,
+ env: Pick,
jobId: string,
leaseOwner: string,
leaseSeconds: number,
@@ -502,6 +536,7 @@ export async function heartbeatJobLease(
`,
[jobId, leaseOwner, String(leaseSeconds)],
);
+ await markSystemActive(env);
}
export async function releaseJobLease(env: Pick, jobId: string, leaseOwner: string) {
@@ -518,21 +553,45 @@ export async function releaseJobLease(env: Pick, jobI
);
}
+// Records that a job is rescheduling the same phase (a continuation) and returns the resulting
+// no-progress continuation count. The counter is bumped here and cleared by
+// resetJobContinuationCount() whenever a chunk actually completes a file, so a healthy job that
+// keeps making headway stays near zero while a job that can never progress climbs toward the
+// MAX_JOB_CONTINUATIONS ceiling and is failed terminally.
export async function markJobContinuationQueued(env: Pick, jobId: string, delaySeconds = 0) {
- await queryRows(
+ const rows = await queryRows<{ continuation_count: number }>(
env,
`
UPDATE jobs
SET heartbeat_at = now(),
+ continuation_count = continuation_count + 1,
last_queue_message_at = CASE
WHEN $2::int > 0 THEN now() + ($2::text || ' seconds')::interval
ELSE now()
END
WHERE id = $1
AND status = 'running'
+ RETURNING continuation_count
`,
[jobId, delaySeconds],
);
+ return rows[0]?.continuation_count ?? 0;
+}
+
+// Clears the no-progress continuation counter after a chunk completes at least one file review,
+// so slow-but-progressing jobs never trip the MAX_JOB_CONTINUATIONS safety net.
+export async function resetJobContinuationCount(env: Pick, jobId: string) {
+ await queryRows(
+ env,
+ `
+ UPDATE jobs
+ SET continuation_count = 0
+ WHERE id = $1
+ AND status = 'running'
+ AND continuation_count <> 0
+ `,
+ [jobId],
+ );
}
export async function updateJobCheckRun(env: Pick, jobId: string, checkRunId: number) {
@@ -570,7 +629,11 @@ export async function completeJob(
UPDATE jobs
SET status = 'done',
finished_at = now(),
- check_run_completed_at = now(),
+ -- NOTE: check_run_completed_at is intentionally NOT set here. It's set only once the
+ -- GitHub check run has actually been updated (markJobCheckRunCompleted, called after the
+ -- best-effort updateCheckRun in finalize). That way, if finalize couldn't update the
+ -- check run (e.g. subrequest budget spent on a huge PR), it stays NULL and the maintenance
+ -- sweep (completeTerminalCheckRuns) reconciles it -- the check run always ends 'completed'.
lease_owner = NULL,
lease_expires_at = NULL,
verdict = $2,
@@ -623,7 +686,7 @@ export async function completeJob(
);
}
-export async function failJob(env: Pick, jobId: string, errorMessage: string) {
+export async function failJob(env: Pick, jobId: string, errorMessage: string) {
await queryRows(
env,
`
@@ -649,6 +712,59 @@ export async function failJob(env: Pick, jobId: strin
`,
[jobId, errorMessage],
);
+ await markSystemActive(env);
+}
+
+/**
+ * Stop an ongoing (queued/running) job at the user's request: mark it terminal as 'cancelled',
+ * clear the lease (so lease-recovery won't requeue it) and mark any running steps failed. Returns
+ * true if a job was actually transitioned (false if it was already terminal). The caller is
+ * responsible for terminating the Cloudflare Workflow instance.
+ */
+export async function cancelJob(env: Pick, jobId: string): Promise {
+ const rows = await queryRows<{ id: string }>(
+ env,
+ `
+ UPDATE jobs
+ SET status = 'cancelled',
+ finished_at = now(),
+ lease_owner = NULL,
+ lease_expires_at = NULL,
+ error_msg = COALESCE(error_msg, 'Stopped by user.'),
+ steps = CASE
+ WHEN steps IS NOT NULL THEN (
+ SELECT jsonb_agg(
+ CASE
+ WHEN s->>'status' = 'running'
+ THEN s || jsonb_build_object('status', 'failed', 'finishedAt', now(), 'error', 'Stopped by user.')
+ ELSE s
+ END
+ ) FROM jsonb_array_elements(steps) s
+ )
+ ELSE steps
+ END
+ WHERE id = $1 AND status IN ('queued', 'running')
+ RETURNING id
+ `,
+ [jobId],
+ );
+ // Keep the maintenance flag set so the cron completes the GitHub check run for the cancelled job.
+ if (rows.length > 0) await markSystemActive(env);
+ return rows.length > 0;
+}
+
+/**
+ * Permanently delete a job. file_reviews and review_comments cascade automatically (ON DELETE
+ * CASCADE); child retry jobs have their retry_of_job_id nulled (ON DELETE SET NULL). Returns true
+ * if a row was deleted.
+ */
+export async function deleteJob(env: Pick, jobId: string): Promise {
+ const rows = await queryRows<{ id: string }>(
+ env,
+ `DELETE FROM jobs WHERE id = $1 RETURNING id`,
+ [jobId],
+ );
+ return rows.length > 0;
}
export async function markJobCheckRunCompleted(env: Pick, jobId: string) {
@@ -751,8 +867,14 @@ export async function updateJobStep(
WHEN s->>'name' = $2
THEN s || jsonb_build_object(
'status', $3::text,
- 'startedAt', COALESCE($4::text, s->>'startedAt'),
- 'finishedAt', COALESCE($5::text, s->>'finishedAt'),
+ -- Preserve the FIRST start time. A phase (e.g. "Reviewing Files") re-enters
+ -- 'running' once per hibernated chunk; overwriting startedAt each time would make
+ -- the displayed duration reflect only the last chunk (~seconds) instead of the full
+ -- multi-minute wall-clock. Keep the existing start; only seed it when absent.
+ 'startedAt', COALESCE(s->>'startedAt', $4::text),
+ -- Clear any stale finish when a step is (re-)entered as 'running' so its duration
+ -- isn't computed against an old finishedAt while it is back in progress.
+ 'finishedAt', CASE WHEN $3::text = 'running' THEN NULL ELSE COALESCE($5::text, s->>'finishedAt') END,
'error', COALESCE($6::text, s->>'error')
)
ELSE s
@@ -775,23 +897,6 @@ export async function updateJobStep(
);
}
-export async function recoverStaleJobs(
- env: Pick,
- thresholdMinutes = 20,
-): Promise {
- const rows = await queryRows<{ id: string }>(env, `
- UPDATE jobs
- SET status = 'failed',
- finished_at = now(),
- error_msg = 'Job timed out: worker crashed or was evicted.'
- WHERE status = 'running'
- AND started_at < now() - ($1 || ' minutes')::interval
- RETURNING id
- `, [String(thresholdMinutes)]);
-
- return rows.length;
-}
-
export async function recoverExpiredJobLeases(
env: Pick,
maxRecoveryCount = 3,
@@ -902,7 +1007,7 @@ export async function getTerminalJobsNeedingCheckRunCompletion(
SELECT j.*, r.owner, r.repo, r.installation_id
FROM jobs j
JOIN repositories r ON j.repository_id = r.id
- WHERE j.status IN ('failed', 'superseded')
+ WHERE j.status IN ('done', 'failed', 'superseded', 'cancelled')
AND j.check_run_id IS NOT NULL
AND j.check_run_completed_at IS NULL
ORDER BY COALESCE(j.finished_at, j.started_at, j.created_at) ASC
diff --git a/src/server/db/model-configs.ts b/src/server/db/model-configs.ts
index 453f51e..51e90b8 100644
--- a/src/server/db/model-configs.ts
+++ b/src/server/db/model-configs.ts
@@ -26,9 +26,6 @@ type ModelConfigRow = {
provider_name: string;
api_format: LlmApiFormat;
model_name: string;
- rpm: number | null;
- tpm: number | null;
- rpd: number | null;
updated_at: string;
};
@@ -69,9 +66,6 @@ function mapModelConfig(row: ModelConfigRow): ModelConfig {
providerName: row.provider_name,
apiFormat: row.api_format,
modelName: row.model_name,
- rpm: row.rpm,
- tpm: row.tpm,
- rpd: row.rpd,
updatedAt: row.updated_at,
});
}
@@ -83,9 +77,6 @@ const MODEL_SELECT = `
p.name AS provider_name,
p.api_format,
mc.model_name,
- mc.rpm,
- mc.tpm,
- mc.rpd,
mc.updated_at
FROM model_configs mc
JOIN llm_providers p ON p.id = mc.provider_id
@@ -248,9 +239,6 @@ export async function getResolvedModelConfig(
p.name AS provider_name,
p.api_format,
mc.model_name,
- mc.rpm,
- mc.tpm,
- mc.rpd,
mc.updated_at,
p.enabled AS provider_enabled,
p.base_url,
@@ -279,20 +267,17 @@ export async function updateModelConfig(
env,
`
WITH upserted AS (
- INSERT INTO model_configs (model_id, provider_id, model_name, rpm, tpm, rpd, provider, updated_at)
- SELECT $1, p.id, $3, $4, $5, $6, p.api_format, now()
+ INSERT INTO model_configs (model_id, provider_id, model_name, provider, updated_at)
+ SELECT $1, p.id, $3, p.api_format, now()
FROM llm_providers p
WHERE p.id = $2
ON CONFLICT (model_id)
DO UPDATE SET
provider_id = EXCLUDED.provider_id,
model_name = EXCLUDED.model_name,
- rpm = EXCLUDED.rpm,
- tpm = EXCLUDED.tpm,
- rpd = EXCLUDED.rpd,
provider = EXCLUDED.provider,
updated_at = now()
- RETURNING model_id, provider_id, model_name, rpm, tpm, rpd, updated_at
+ RETURNING model_id, provider_id, model_name, updated_at
)
SELECT
u.model_id,
@@ -300,14 +285,11 @@ export async function updateModelConfig(
p.name AS provider_name,
p.api_format,
u.model_name,
- u.rpm,
- u.tpm,
- u.rpd,
u.updated_at
FROM upserted u
JOIN llm_providers p ON p.id = u.provider_id
`,
- [config.modelId, config.providerId, config.modelName, config.rpm, config.tpm, config.rpd],
+ [config.modelId, config.providerId, config.modelName],
);
return row ? mapModelConfig(row) : null;
}
@@ -352,9 +334,6 @@ export async function upsertDiscoveredModelConfigs(
model_id: string;
provider_id: string;
model_name: string;
- rpm: number | null;
- tpm: number | null;
- rpd: number | null;
provider: LlmApiFormat;
}> = [];
@@ -374,9 +353,6 @@ export async function upsertDiscoveredModelConfigs(
model_id: candidate,
provider_id: input.providerId,
model_name: modelName,
- rpm: null,
- tpm: null,
- rpd: null,
provider: input.apiFormat,
});
}
@@ -386,9 +362,6 @@ export async function upsertDiscoveredModelConfigs(
const modelIds = rowsToInsert.map(row => row.model_id);
const providerIds = rowsToInsert.map(row => row.provider_id);
const modelNames = rowsToInsert.map(row => row.model_name);
- const rpms = rowsToInsert.map(row => row.rpm);
- const tpms = rowsToInsert.map(row => row.tpm);
- const rpds = rowsToInsert.map(row => row.rpd);
const providers = rowsToInsert.map(row => row.provider);
const rows = await queryRows(
@@ -400,18 +373,15 @@ export async function upsertDiscoveredModelConfigs(
$1::text[],
$2::uuid[],
$3::text[],
- $4::integer[],
- $5::integer[],
- $6::integer[],
- $7::text[]
- ) AS item(model_id, provider_id, model_name, rpm, tpm, rpd, provider)
+ $4::text[]
+ ) AS item(model_id, provider_id, model_name, provider)
),
inserted AS (
- INSERT INTO model_configs (model_id, provider_id, model_name, rpm, tpm, rpd, provider, updated_at)
- SELECT model_id, provider_id, model_name, rpm, tpm, rpd, provider, now()
+ INSERT INTO model_configs (model_id, provider_id, model_name, provider, updated_at)
+ SELECT model_id, provider_id, model_name, provider, now()
FROM incoming
ON CONFLICT (model_id) DO NOTHING
- RETURNING model_id, provider_id, model_name, rpm, tpm, rpd, updated_at
+ RETURNING model_id, provider_id, model_name, updated_at
)
SELECT
i.model_id,
@@ -419,15 +389,12 @@ export async function upsertDiscoveredModelConfigs(
p.name AS provider_name,
p.api_format,
i.model_name,
- i.rpm,
- i.tpm,
- i.rpd,
i.updated_at
FROM inserted i
JOIN llm_providers p ON p.id = i.provider_id
ORDER BY i.model_id ASC
`,
- [modelIds, providerIds, modelNames, rpms, tpms, rpds, providers],
+ [modelIds, providerIds, modelNames, providers],
);
return rows.map(mapModelConfig);
diff --git a/src/server/db/stats.ts b/src/server/db/stats.ts
index 42d1175..457aa65 100644
--- a/src/server/db/stats.ts
+++ b/src/server/db/stats.ts
@@ -1,13 +1,18 @@
import type { AppBindings } from '@server/env';
import { queryRows } from './client';
-import { statsSchema } from '@shared/schema';
+import { statsSchema, jobStatuses, reviewTriggers, reviewSeverities, reviewCategories } from '@shared/schema';
import { getModelUsageStats } from './file-reviews';
+const jobStatusSet = new Set(jobStatuses);
+const reviewTriggerSet = new Set(reviewTriggers);
+const reviewSeveritySet = new Set(reviewSeverities);
+const reviewCategorySet = new Set(reviewCategories);
+
export async function getStats(env: Pick, days = 30) {
const parsedDays = Number(days);
const safeDays = Number.isFinite(parsedDays) ? Math.trunc(parsedDays) : 30;
const clampedDays = Math.min(Math.max(safeDays, 1), 365);
- const [[totals], dailyRows, verdictRows, topRepos, modelRows] = await Promise.all([
+ const [[totals], dailyRows, verdictRows, topRepos, modelRows, statusRows, triggerRows, severityRows, categoryRows, [performanceRow]] = await Promise.all([
queryRows<{
jobs: number;
input_tokens: number;
@@ -61,6 +66,64 @@ export async function getStats(env: Pick, days = 30)
`,
),
getModelUsageStats(env),
+ queryRows<{ status: string; count: number }>(
+ env,
+ `
+ SELECT status, COUNT(*)::int AS count
+ FROM jobs
+ WHERE created_at >= now() - ($1::int * interval '1 day')
+ GROUP BY status
+ ORDER BY count DESC
+ `,
+ [clampedDays],
+ ),
+ queryRows<{ trigger: string; count: number }>(
+ env,
+ `
+ SELECT trigger, COUNT(*)::int AS count
+ FROM jobs
+ WHERE created_at >= now() - ($1::int * interval '1 day')
+ GROUP BY trigger
+ ORDER BY count DESC
+ `,
+ [clampedDays],
+ ),
+ queryRows<{ severity: string; count: number }>(
+ env,
+ `
+ SELECT rc.severity, COUNT(*)::int AS count
+ FROM review_comments rc
+ JOIN file_reviews fr ON fr.id = rc.file_review_id
+ WHERE fr.created_at >= now() - ($1::int * interval '1 day')
+ GROUP BY rc.severity
+ ORDER BY count DESC
+ `,
+ [clampedDays],
+ ),
+ queryRows<{ category: string; count: number }>(
+ env,
+ `
+ SELECT rc.category, COUNT(*)::int AS count
+ FROM review_comments rc
+ JOIN file_reviews fr ON fr.id = rc.file_review_id
+ WHERE fr.created_at >= now() - ($1::int * interval '1 day')
+ GROUP BY rc.category
+ ORDER BY count DESC
+ `,
+ [clampedDays],
+ ),
+ queryRows<{ avg_duration_ms: number | null; p95_duration_ms: number | null; avg_confidence: number | null }>(
+ env,
+ `
+ SELECT
+ AVG(EXTRACT(EPOCH FROM (finished_at - started_at)) * 1000) AS avg_duration_ms,
+ PERCENTILE_CONT(0.95) WITHIN GROUP (ORDER BY EXTRACT(EPOCH FROM (finished_at - started_at)) * 1000) AS p95_duration_ms,
+ AVG(overall_confidence_score) AS avg_confidence
+ FROM jobs
+ WHERE finished_at IS NOT NULL AND started_at IS NOT NULL AND created_at >= now() - ($1::int * interval '1 day')
+ `,
+ [clampedDays],
+ ),
]);
return statsSchema.parse({
@@ -86,5 +149,17 @@ export async function getStats(env: Pick, days = 30)
outputTokens: row.output_tokens ?? 0,
})),
topRepos: topRepos.map((row) => ({ owner: row.owner, repo: row.repo, jobs: row.jobs })),
+ // Drop any rows whose enum-typed column holds an unexpected value (e.g. legacy
+ // rows back-migrated from JSON, where severity/category have no DB CHECK
+ // constraint). Keeping them would fail statsSchema.parse and 500 the endpoint.
+ statuses: statusRows.filter((row) => jobStatusSet.has(row.status)).map((row) => ({ status: row.status as (typeof jobStatuses)[number], count: row.count })),
+ triggers: triggerRows.filter((row) => reviewTriggerSet.has(row.trigger)).map((row) => ({ trigger: row.trigger as (typeof reviewTriggers)[number], count: row.count })),
+ severities: severityRows.filter((row) => reviewSeveritySet.has(row.severity)).map((row) => ({ severity: row.severity as (typeof reviewSeverities)[number], count: row.count })),
+ categories: categoryRows.filter((row) => reviewCategorySet.has(row.category)).map((row) => ({ category: row.category as (typeof reviewCategories)[number], count: row.count })),
+ performance: {
+ avgDurationMs: performanceRow?.avg_duration_ms != null ? Math.round(performanceRow.avg_duration_ms) : null,
+ p95DurationMs: performanceRow?.p95_duration_ms != null ? Math.round(performanceRow.p95_duration_ms) : null,
+ avgConfidence: performanceRow?.avg_confidence ?? null,
+ },
});
}
diff --git a/src/server/env.ts b/src/server/env.ts
index 0ab9fc6..e15e2a3 100644
--- a/src/server/env.ts
+++ b/src/server/env.ts
@@ -1,7 +1,7 @@
import type { ReviewJobMessage } from '@shared/schema';
export interface WorkersAiBinding {
- run(model: string, input: Record): Promise;
+ run(model: string, input: Record, options?: { signal?: AbortSignal }): Promise;
}
export interface QueueProducer {
diff --git a/src/server/index.ts b/src/server/index.ts
index 94ce8a2..3356b86 100644
--- a/src/server/index.ts
+++ b/src/server/index.ts
@@ -1,11 +1,10 @@
import { createApp } from './app';
-import { runReviewJob } from './core/review';
import { ReviewWorkflow } from './workflows/review';
import type { AppBindings } from './env';
import { reviewJobMessageSchema } from '@shared/schema';
import { logger } from '@server/core/logger';
import { runWithDb } from '@server/db/client';
-import { failJob } from '@server/db/jobs';
+import { failJob, hasPendingMaintenanceWork, clearSystemActive } from '@server/db/jobs';
import { runBestEffortJobMaintenance } from '@server/core/job-recovery';
const app = createApp();
@@ -17,6 +16,36 @@ export default {
return runWithDb(env, () => app.fetch(request, env, ctx));
},
+ async scheduled(_controller: ScheduledController, env: AppBindings, _ctx: ExecutionContext) {
+ // The cron fires every 2 minutes, but its only job is maintenance (recovering stuck jobs and
+ // finishing check runs). Touching Postgres on every tick would keep the serverless DB awake
+ // 24/7. So gate the DB work on a KV flag that is set whenever a job is created/claimed and
+ // cleared below once there is genuinely nothing left to maintain. When the flag is absent we
+ // return without ever opening a DB connection, letting Postgres suspend.
+ try {
+ const active = await env.APP_KV.get('system:active_jobs');
+ if (!active) {
+ return;
+ }
+ } catch (error) {
+ logger.warn('Failed to read active jobs flag from KV, proceeding with maintenance', error instanceof Error ? error : new Error(String(error)));
+ }
+
+ return runWithDb(env, async () => {
+ await runBestEffortJobMaintenance(env);
+ // As soon as no jobs are running/recoverable and no check runs are outstanding, drop the
+ // flag so the next tick skips Postgres entirely (instead of waiting out the 20-minute TTL).
+ // A new job re-sets the flag on insert/claim, so this only trims the idle tail.
+ try {
+ if (!(await hasPendingMaintenanceWork(env))) {
+ await clearSystemActive(env);
+ }
+ } catch (error) {
+ logger.warn('Failed to evaluate pending maintenance work; leaving active-jobs flag to expire via TTL', error instanceof Error ? error : new Error(String(error)));
+ }
+ });
+ },
+
async queue(batch: MessageBatch, env: AppBindings, _ctx: ExecutionContext) {
return runWithDb(env, async () => {
try {
@@ -33,12 +62,29 @@ export default {
body: message.body,
error: parseResult.error.flatten(),
});
- message.ack(); // Drop invalid schema messages
+ // A malformed message can't be processed and retrying won't help, so ack it -- but if it
+ // still carries a recognizable jobId, fail that job so it doesn't sit 'queued' forever
+ // (lease recovery only revives 'running' rows).
+ const strandedId = (message.body as { jobId?: unknown })?.jobId;
+ if (typeof strandedId === 'string' && /^[0-9a-f-]{36}$/i.test(strandedId)) {
+ try {
+ await failJob(env, strandedId, 'Review dropped: the queue message failed schema validation.');
+ } catch (failError) {
+ logger.error('Failed to fail job stranded by an invalid queue message', failError instanceof Error ? failError : new Error(String(failError)));
+ }
+ }
+ message.ack();
continue;
}
try {
- const id = parseResult.data.jobId ?? parseResult.data.deliveryId;
+ // Recovery re-enqueues a stuck job under its original jobId; keying the instance on jobId
+ // would collide with the dead instance (instance.already_exists) and get dropped as a
+ // duplicate, so recovery sets forceFreshInstance to key the new instance on the (fresh)
+ // deliveryId. deliveryId is a UUID, matching the workflow_instance_id column type.
+ const id = parseResult.data.forceFreshInstance
+ ? parseResult.data.deliveryId
+ : (parseResult.data.jobId ?? parseResult.data.deliveryId);
if (!id) {
logger.error('Message missing identifiers; dropping', { body: message.body });
message.ack();
diff --git a/src/server/models/anthropic.ts b/src/server/models/anthropic.ts
index dab3da4..423bf3d 100644
--- a/src/server/models/anthropic.ts
+++ b/src/server/models/anthropic.ts
@@ -2,7 +2,7 @@ import { logger } from '@server/core/logger';
import { withTimeout } from '@server/core/timeout';
import { ProviderRequestError, providerErrorMessage, type ModelResponse } from './types';
-const ANTHROPIC_TIMEOUT_MS = 180_000;
+const ANTHROPIC_TIMEOUT_MS = 80_000;
const ANTHROPIC_MAX_OUTPUT_TOKENS = 4096;
const DEFAULT_ANTHROPIC_BASE_URL = 'https://api.anthropic.com/v1';
@@ -15,16 +15,17 @@ export interface AnthropicResponse {
}
export async function reviewWithAnthropic(
- config: { apiKey: string; baseUrl?: string | null; providerName: string },
+ config: { apiKey: string; baseUrl?: string | null; providerName: string; timeoutMs?: number },
model: string,
input: { systemPrompt: string; userPrompt: string },
tracker?: { incrementSubrequests(count?: number): void },
): Promise {
logger.info(`Calling Anthropic model: ${model}`);
const baseUrl = (config.baseUrl || DEFAULT_ANTHROPIC_BASE_URL).replace(/\/+$/, '');
+ const timeoutMs = config.timeoutMs ?? ANTHROPIC_TIMEOUT_MS;
if (tracker) tracker.incrementSubrequests(1);
- const response = await withTimeout('Anthropic API', ANTHROPIC_TIMEOUT_MS, (signal) =>
+ const response = await withTimeout('Anthropic API', timeoutMs, (signal) =>
fetch(`${baseUrl}/messages`, {
method: 'POST',
signal,
diff --git a/src/server/models/cloudflare.ts b/src/server/models/cloudflare.ts
index 8436c75..9b0c50f 100644
--- a/src/server/models/cloudflare.ts
+++ b/src/server/models/cloudflare.ts
@@ -1,10 +1,17 @@
import { logger } from '@server/core/logger';
import type { AppBindings } from '@server/env';
import { TimeoutError } from '@server/core/timeout';
-import { ProviderRequestError, type ModelResponse } from './types';
+import { ProviderRequestError, UnparseableModelResponseError, type ModelResponse } from './types';
-/** Max wall-clock time allowed for a single Workers-AI call. */
-const CLOUDFLARE_TIMEOUT_MS = 180_000;
+/**
+ * Default max wall-clock time allowed for a single Workers-AI call when the caller doesn't
+ * supply a diff-size-aware budget. Kept well under the review workflow's 15-minute step
+ * timeout: a model that hasn't answered a code-review prompt in this long (reasoning models
+ * under strict-JSON decoding are the usual offenders -- they burn the whole token budget
+ * "thinking" and never emit the JSON) is not going to, so we fail fast and let the file defer
+ * to a fresh invocation instead of stalling the whole review.
+ */
+const CLOUDFLARE_TIMEOUT_MS = 45_000;
const CLOUDFLARE_MAX_RETRIES = 0;
const CLOUDFLARE_MAX_OUTPUT_TOKENS = 8192;
const REVIEW_RESPONSE_SCHEMA = {
@@ -71,12 +78,6 @@ function getRecord(value: unknown, key: string): UnknownRecord | null {
return isRecord(child) ? child : null;
}
-function getText(value: unknown, key: string): string | null {
- if (!isRecord(value)) return null;
- const child = value[key];
- return isText(child) ? child.trim() : null;
-}
-
function getNumber(value: unknown, key: string) {
if (!isRecord(value)) return null;
const child = value[key];
@@ -89,16 +90,9 @@ function isLocalWorkersAiBindingError(error: unknown) {
return normalized.includes('binding ai') && normalized.includes('run remotely');
}
-function synthesizeInconclusiveReview(model: string, reason: string): string {
- logger.warn(`Cloudflare model ${model} returned no parseable review content; synthesizing inconclusive review JSON`, {
- reason,
- });
- return JSON.stringify({
- findings: [],
- overall_correctness: 'patch is incorrect',
- overall_explanation: `Cloudflare model ${model} returned no parseable review content (${reason}). The file review is inconclusive.`,
- overall_confidence_score: 0,
- });
+function failUnparseable(model: string, reason: string): never {
+ logger.warn(`Cloudflare model ${model} returned no parseable review content; failing the file review`, { reason });
+ throw new UnparseableModelResponseError(model, reason);
}
function extractMessageContent(content: unknown): string | null {
@@ -119,13 +113,32 @@ function extractMessageContent(content: unknown): string | null {
return null;
}
+// A model's completion arrives in `response`, but the type varies by model: most return a
+// plain string, while models honoring `response_format`/structured output (e.g.
+// @cf/qwen/qwen2.5-coder-32b-instruct) return an already-parsed JSON object or array. Accept
+// both -- stringify structured values so the downstream repair/parse pipeline can consume them
+// instead of the extractor discarding a perfectly good review as an "empty response".
+function extractResponseField(container: unknown): string | null {
+ if (!isRecord(container)) return null;
+ const value = container.response;
+ if (isText(value)) return value.trim();
+ if (value && typeof value === 'object') {
+ try {
+ return JSON.stringify(value);
+ } catch {
+ return null;
+ }
+ }
+ return null;
+}
+
function extractCloudflareText(result: unknown, model: string): string {
if (isText(result)) return result.trim();
- const response = getText(result, 'response');
+ const response = extractResponseField(result);
if (response) return response;
const nestedResult = getRecord(result, 'result');
- const nestedResponse = getText(nestedResult, 'response');
+ const nestedResponse = extractResponseField(nestedResult);
if (nestedResponse) return nestedResponse;
const choices = isRecord(result) && Array.isArray(result.choices) ? result.choices : null;
@@ -137,14 +150,14 @@ function extractCloudflareText(result: unknown, model: string): string {
const finishReason = isRecord(choice) ? choice.finish_reason ?? choice.stop_reason : null;
const reasoning = isText(message?.reasoning) ? message.reasoning : isText(message?.reasoning_content) ? message.reasoning_content : null;
if (reasoning) {
- return synthesizeInconclusiveReview(model, `reasoning-only response${finishReason ? `, finish_reason=${String(finishReason)}` : ''}`);
+ return failUnparseable(model, `reasoning-only response${finishReason ? `, finish_reason=${String(finishReason)}` : ''}`);
}
if (finishReason) {
- return synthesizeInconclusiveReview(model, `finish_reason=${String(finishReason)}`);
+ return failUnparseable(model, `finish_reason=${String(finishReason)}`);
}
- return synthesizeInconclusiveReview(model, 'empty response');
+ return failUnparseable(model, 'empty response');
}
function extractCloudflareUsage(result: unknown) {
@@ -155,21 +168,155 @@ function extractCloudflareUsage(result: unknown) {
};
}
+/**
+ * The single-request inference payload sent to Workers AI. Shared by the synchronous path and
+ * the asynchronous batch path so both send an identical prompt/schema/decoding configuration.
+ */
+function buildCloudflareInferenceRequest(input: { systemPrompt: string; userPrompt: string }) {
+ return {
+ messages: [
+ {
+ role: 'system',
+ content: `${input.systemPrompt}\n\nReturn only the JSON object. Do not include chain-of-thought, analysis, markdown, code fences, or explanatory prose.`,
+ },
+ { role: 'user', content: `${input.userPrompt}\n\nRespond with the required JSON object only.` },
+ ],
+ max_completion_tokens: CLOUDFLARE_MAX_OUTPUT_TOKENS,
+ response_format: {
+ type: 'json_schema',
+ json_schema: {
+ name: 'codra_file_review',
+ strict: true,
+ schema: REVIEW_RESPONSE_SCHEMA,
+ },
+ },
+ temperature: 0,
+ top_p: 0.1,
+ } as const;
+}
+
+/**
+ * Result of polling an async batch request. `pending` means the batch is still queued/running
+ * on Workers AI (poll again later); `done` carries the extracted review.
+ */
+export type CloudflareBatchPollResult =
+ | { status: 'pending' }
+ | { status: 'done'; response: ModelResponse };
+
+function extractBatchStatus(result: unknown): string | null {
+ if (!isRecord(result)) return null;
+ const status = result.status ?? getRecord(result, 'result')?.status;
+ return typeof status === 'string' ? status.toLowerCase() : null;
+}
+
+/**
+ * Finds the single inference result inside a completed batch poll response. Workers AI has
+ * returned a few shapes for this over time (a top-level `responses` array, a nested
+ * `result.responses`, or a bare single result), so probe defensively and fall back to treating
+ * the whole payload as one result.
+ */
+function extractBatchInnerResult(result: unknown): unknown {
+ const containers = [result, isRecord(result) ? result.result : undefined];
+ for (const container of containers) {
+ if (!isRecord(container)) continue;
+ const responses = container.responses ?? container.results;
+ if (Array.isArray(responses) && responses.length > 0) {
+ const first = responses[0];
+ // Each entry may wrap the model output under `result`/`response`, or be it directly.
+ if (isRecord(first)) return first.result ?? first;
+ return first;
+ }
+ }
+ return result;
+}
+
+/**
+ * Submit a single review as an asynchronous batch request. Returns the queue `request_id` to
+ * poll later. Throws if the model/account does not support async queueing (the caller is
+ * expected to fall back to the synchronous path on any failure).
+ */
+export async function submitCloudflareBatch(
+ env: Pick,
+ model: string,
+ input: { systemPrompt: string; userPrompt: string },
+ tracker?: { incrementSubrequests(count?: number): void },
+): Promise {
+ if (tracker) tracker.incrementSubrequests(1);
+ logger.info(`Submitting async batch request to Cloudflare model: ${model}`);
+ const result = await env.AI.run(
+ model as any,
+ { requests: [buildCloudflareInferenceRequest(input)] } as any,
+ { queueRequest: true } as any,
+ );
+
+ const requestId = isRecord(result)
+ ? (result.request_id ?? getRecord(result, 'result')?.request_id)
+ : undefined;
+ if (typeof requestId !== 'string' || !requestId) {
+ throw new Error(`Cloudflare model ${model} did not return an async batch request_id (async queueing unsupported).`);
+ }
+ return requestId;
+}
+
+/**
+ * Poll a previously submitted async batch request by its `request_id`. Returns `pending` while
+ * the batch is still queued/running, or `done` with the extracted review once complete.
+ */
+export async function pollCloudflareBatch(
+ env: Pick,
+ model: string,
+ requestId: string,
+ tracker?: { incrementSubrequests(count?: number): void },
+ providerName = 'Cloudflare',
+): Promise {
+ if (tracker) tracker.incrementSubrequests(1);
+ const result = await env.AI.run(model as any, { request_id: requestId } as any);
+
+ const status = extractBatchStatus(result);
+ if (status === 'queued' || status === 'running') {
+ return { status: 'pending' };
+ }
+
+ const inner = extractBatchInnerResult(result);
+ const rawText = extractCloudflareText(inner, model);
+ const usage = extractCloudflareUsage(inner);
+ return {
+ status: 'done',
+ response: {
+ rawText,
+ inputTokens: usage.inputTokens,
+ outputTokens: usage.outputTokens,
+ modelUsed: model,
+ provider: providerName,
+ },
+ };
+}
+
export async function reviewWithCloudflare(
env: Pick,
model: string,
input: { systemPrompt: string; userPrompt: string },
tracker?: { incrementSubrequests(count?: number): void },
providerName = 'Cloudflare',
+ options?: { timeoutMs?: number },
): Promise {
const maxRetries = CLOUDFLARE_MAX_RETRIES;
+ const timeoutMs = options?.timeoutMs ?? CLOUDFLARE_TIMEOUT_MS;
let lastError: unknown;
for (let attempt = 0; attempt <= maxRetries; attempt++) {
let timer: ReturnType | undefined;
+ // Abort the underlying Workers-AI request when the timeout fires. Promise.race on its own
+ // only stops *us* awaiting -- the subrequest would keep running in the background, holding
+ // this invocation's wall-clock and pushing the workflow toward its 15-minute step cap long
+ // after we've given up. Aborting via the AI binding's signal actually cancels it.
+ const controller = new AbortController();
const timeoutPromise = new Promise((_, reject) => {
- timer = setTimeout(() => reject(new TimeoutError(`Cloudflare (${model})`, CLOUDFLARE_TIMEOUT_MS)), CLOUDFLARE_TIMEOUT_MS);
+ timer = setTimeout(() => {
+ controller.abort();
+ reject(new TimeoutError(`Cloudflare (${model})`, timeoutMs));
+ }, timeoutMs);
});
try {
@@ -182,29 +329,11 @@ export async function reviewWithCloudflare(
logger.info(`Calling Cloudflare model: ${model}`);
const startTime = Date.now();
- const result = await Promise.race([
- env.AI.run(model as any, {
- messages: [
- {
- role: 'system',
- content: `${input.systemPrompt}\n\nReturn only the JSON object. Do not include chain-of-thought, analysis, markdown, code fences, or explanatory prose.`,
- },
- { role: 'user', content: `${input.userPrompt}\n\nRespond with the required JSON object only.` },
- ],
- max_completion_tokens: CLOUDFLARE_MAX_OUTPUT_TOKENS,
- response_format: {
- type: 'json_schema',
- json_schema: {
- name: 'codra_file_review',
- strict: true,
- schema: REVIEW_RESPONSE_SCHEMA,
- },
- },
- temperature: 0,
- top_p: 0.1,
- }),
- timeoutPromise,
- ]);
+ const runPromise = env.AI.run(model as any, buildCloudflareInferenceRequest(input), { signal: controller.signal });
+ // Once the timeout wins the race the aborted run still settles (as a rejection); attach a
+ // no-op handler so that late rejection can't surface as an unhandled promise rejection.
+ runPromise.catch(() => {});
+ const result = await Promise.race([runPromise, timeoutPromise]);
const durationMs = Date.now() - startTime;
logger.info(`AI model ${model} responded in ${durationMs}ms`);
diff --git a/src/server/models/google.ts b/src/server/models/google.ts
index 085d182..f7d8932 100644
--- a/src/server/models/google.ts
+++ b/src/server/models/google.ts
@@ -1,15 +1,58 @@
import { logger } from '@server/core/logger';
import { withTimeout } from '@server/core/timeout';
-import { ProviderRequestError, providerErrorMessage, type ModelResponse } from './types';
-
-/** Max wall-clock time allowed for a single Google AI Studio call. */
-const GEMINI_TIMEOUT_MS = 180_000;
-const GEMINI_MAX_RETRIES = 1;
-const GEMINI_MAX_OUTPUT_TOKENS = 4096;
+import { ProviderRequestError, UnparseableModelResponseError, providerErrorMessage, type ModelResponse } from './types';
+
+/** Default max wall-clock time for a single Google AI Studio call when the caller doesn't
+ * supply a diff-size-aware budget. */
+const GEMINI_TIMEOUT_MS = 45_000;
+// Retry transient upstream failures (Gemini's frequent 5xx "Internal error encountered.") a
+// couple of times with backoff so a momentary blip doesn't fail an otherwise-fine gemma review.
+const GEMINI_MAX_RETRIES = 2;
+// Headroom so reasoning/"thinking" models (the gemma-4 family) can spend tokens thinking and
+// still emit the JSON answer instead of getting truncated at the limit with an empty body.
+const GEMINI_MAX_OUTPUT_TOKENS = 8192;
+// Hard cap on any single in-call retry sleep. A 429 can carry a `retry-after` of 30-60s on the
+// Free tier; sleeping that long in-call would pin a model-call-gate slot and burn the chunk's
+// wall-clock budget for a retry that will just 429 again. Cap it low -- if the provider needs a
+// longer cool-off, the file is deferred and resumes in a fresh invocation (which is the real,
+// budget-resetting backoff), so a long in-call sleep buys nothing.
+const GEMINI_MAX_RETRY_DELAY_MS = 5_000;
const DEFAULT_GEMINI_BASE_URL = 'https://generativelanguage.googleapis.com/v1beta';
function isRetryableGeminiStatus(status: number) {
- return status === 408 || status === 500 || status === 502 || status === 503 || status === 504 || status === 524;
+ return status === 408 || status === 429 || status === 500 || status === 502 || status === 503 || status === 504 || status === 524;
+}
+
+function defaultRetryDelayMs(attempt: number) {
+ // Snappy backoff: a transient Gemini 5xx usually clears within a second or two, and long sleeps
+ // just eat into the caller's wall-clock and subrequest budget. ~0.8s then ~1.6s for the retries.
+ return Math.pow(2, attempt) * 800 + Math.random() * 400;
+}
+
+function retryAfterDelayMs(value: string | null) {
+ if (!value) return null;
+ const seconds = Number(value);
+ if (Number.isFinite(seconds) && seconds >= 0) {
+ return seconds * 1000;
+ }
+
+ const dateMs = Date.parse(value);
+ if (Number.isFinite(dateMs)) {
+ return Math.max(0, dateMs - Date.now());
+ }
+
+ return null;
+}
+
+function isRetryableTransportError(error: unknown) {
+ if (!(error instanceof Error)) return false;
+ // Deliberately do NOT retry timeouts: the caller already grants a diff-size-aware budget (up to
+ // 2 minutes), so a call that blows it is a genuinely slow/stuck generation -- retrying just
+ // burns more wall-clock and subrequests into the same wall. Fail fast and let the fallback chain
+ // (or a fresh-budget continuation) take over. Only genuine transport blips are worth a retry.
+ if (error.name === 'TimeoutError' || error.message.toLowerCase().includes('timed out')) return false;
+ if (error.message.includes('fetch failed')) return true;
+ return error instanceof TypeError;
}
function isPrivateIP(hostname: string) {
@@ -39,11 +82,12 @@ function isValidPublicUrl(urlString: string) {
}
export async function reviewWithGoogle(
- config: { apiKey: string; baseUrl?: string | null; providerName?: string },
+ config: { apiKey: string; baseUrl?: string | null; providerName?: string; timeoutMs?: number },
model: string,
input: { systemPrompt: string; userPrompt: string },
tracker?: { incrementSubrequests(count?: number): void },
): Promise {
+ const timeoutMs = config.timeoutMs ?? GEMINI_TIMEOUT_MS;
logger.info(`Calling Google model: ${model}`);
if (config.baseUrl && !isValidPublicUrl(config.baseUrl)) {
@@ -55,17 +99,19 @@ export async function reviewWithGoogle(
const url = `${baseUrl}/models/${encodeURIComponent(model)}:generateContent?key=${encodeURIComponent(config.apiKey)}`;
const maxRetries = GEMINI_MAX_RETRIES;
let lastError: unknown;
+ let delayBeforeAttemptMs = 0;
for (let attempt = 0; attempt <= maxRetries; attempt++) {
+ if (delayBeforeAttemptMs > 0) {
+ logger.info(`Retrying Gemini request (attempt ${attempt}/${maxRetries}) in ${Math.round(delayBeforeAttemptMs)}ms`);
+ await new Promise(resolve => setTimeout(resolve, delayBeforeAttemptMs));
+ delayBeforeAttemptMs = 0;
+ }
+
+ let response: Response;
try {
if (tracker) tracker.incrementSubrequests(1);
- if (attempt > 0) {
- const delay = Math.pow(2, attempt) * 1000 + Math.random() * 1000;
- logger.info(`Retrying Gemini request (attempt ${attempt}/${maxRetries}) in ${Math.round(delay)}ms`);
- await new Promise(resolve => setTimeout(resolve, delay));
- }
-
- const response = await withTimeout('Gemini API', GEMINI_TIMEOUT_MS, (signal) =>
+ response = await withTimeout('Gemini API', timeoutMs, (signal) =>
fetch(url, {
method: 'POST',
signal,
@@ -84,64 +130,81 @@ export async function reviewWithGoogle(
},
],
generationConfig: {
- responseMimeType: 'application/json',
+ ...(model.toLowerCase().includes('gemma') ? {} : { responseMimeType: 'application/json' }),
maxOutputTokens: GEMINI_MAX_OUTPUT_TOKENS,
},
}),
}),
);
-
- if (!response.ok) {
- const errorText = await response.text();
- const message = providerErrorMessage(errorText);
- const isRetryable = isRetryableGeminiStatus(response.status);
-
- const logData = {
- error: message,
- attempt,
- willRetry: isRetryable && attempt < maxRetries,
- };
- if (isRetryable && attempt < maxRetries) {
- logger.warn(`Gemini request failed with ${response.status}; retrying`, logData);
- lastError = new ProviderRequestError(config.providerName ?? 'Google', response.status, message);
- continue;
- }
-
- logger.error(`Gemini request failed with ${response.status}`, logData);
- throw new ProviderRequestError(config.providerName ?? 'Google', response.status, message);
+ } catch (error) {
+ lastError = error;
+ if (isRetryableTransportError(error) && attempt < maxRetries) {
+ delayBeforeAttemptMs = defaultRetryDelayMs(attempt);
+ continue;
}
+ throw error;
+ }
- const durationMs = Date.now() - startTime;
- logger.info(`AI model ${model} responded in ${durationMs}ms`);
+ if (!response.ok) {
+ const errorText = await response.text();
+ const message = providerErrorMessage(errorText);
+ const isRetryable = isRetryableGeminiStatus(response.status);
+ const retryDelayMs = Math.min(
+ GEMINI_MAX_RETRY_DELAY_MS,
+ response.status === 429
+ ? retryAfterDelayMs(response.headers.get('retry-after')) ?? defaultRetryDelayMs(attempt)
+ : defaultRetryDelayMs(attempt),
+ );
- const data = (await response.json()) as {
- candidates?: Array<{ content?: { parts?: Array<{ text?: string }> } }>;
- usageMetadata?: {
- promptTokenCount?: number;
- candidatesTokenCount?: number;
- };
+ const logData = {
+ error: message,
+ attempt,
+ willRetry: isRetryable && attempt < maxRetries,
+ retryDelayMs: isRetryable && attempt < maxRetries ? retryDelayMs : undefined,
};
-
- const rawText = data.candidates?.[0]?.content?.parts?.map((part) => part.text ?? '').join('')?.trim();
- if (!rawText) {
- throw new Error('Gemini returned an empty response.');
+ if (isRetryable && attempt < maxRetries) {
+ logger.warn(`Gemini request failed with ${response.status}; retrying`, logData);
+ lastError = new ProviderRequestError(config.providerName ?? 'Google', response.status, message);
+ delayBeforeAttemptMs = retryDelayMs;
+ continue;
}
- return {
- rawText,
- inputTokens: data.usageMetadata?.promptTokenCount ?? 0,
- outputTokens: data.usageMetadata?.candidatesTokenCount ?? 0,
- modelUsed: model,
- provider: config.providerName ?? 'Google',
+ logger.error(`Gemini request failed with ${response.status}`, logData);
+ throw new ProviderRequestError(config.providerName ?? 'Google', response.status, message);
+ }
+
+ const durationMs = Date.now() - startTime;
+ logger.info(`AI model ${model} responded in ${durationMs}ms`);
+
+ const data = (await response.json()) as {
+ candidates?: Array<{ content?: { parts?: Array<{ text?: string }> }; finishReason?: string }>;
+ usageMetadata?: {
+ promptTokenCount?: number;
+ candidatesTokenCount?: number;
};
- } catch (error) {
- lastError = error;
- const isTimeout = error instanceof Error && (error.name === 'TimeoutError' || error.message.includes('timeout'));
- if (isTimeout && attempt < maxRetries) {
- continue;
+ };
+
+ const candidate = data.candidates?.[0];
+ const rawText = candidate?.content?.parts?.map((part) => part.text ?? '').join('')?.trim();
+ if (!rawText) {
+ const finishReason = candidate?.finishReason;
+ // A reasoning/"thinking" model can consume its whole output budget before emitting any
+ // answer text, returning empty parts with finishReason MAX_TOKENS (or a safety/RECITATION
+ // block). That's deterministic, so fail the file permanently rather than deferring it for a
+ // retry that would hit the same wall. A truly empty STOP response is treated as transient.
+ if (finishReason && finishReason !== 'STOP') {
+ throw new UnparseableModelResponseError(model, `finishReason=${finishReason}`);
}
- throw error;
+ throw new Error('Gemini returned an empty response.');
}
+
+ return {
+ rawText,
+ inputTokens: data.usageMetadata?.promptTokenCount ?? 0,
+ outputTokens: data.usageMetadata?.candidatesTokenCount ?? 0,
+ modelUsed: model,
+ provider: config.providerName ?? 'Google',
+ };
}
throw lastError;
diff --git a/src/server/models/limits.ts b/src/server/models/limits.ts
new file mode 100644
index 0000000..07bb711
--- /dev/null
+++ b/src/server/models/limits.ts
@@ -0,0 +1,99 @@
+/**
+ * Shared throttling/timeout policy for outbound model calls.
+ *
+ * Two hard Cloudflare Workers Free-plan constraints shape everything here:
+ *
+ * 1. Each Worker invocation may have at most SIX connections simultaneously waiting for
+ * response headers. Anything beyond that (fetch, AI binding, KV, Hyperdrive) is silently
+ * QUEUED by the runtime -- it does not error, it just doesn't start. A model call that sits
+ * queued behind other long-running model calls will burn its own client-side timeout without
+ * the request ever being dispatched, which shows up in logs as a provider "timing out" at
+ * exactly the configured timeout on every attempt.
+ * 2. 50 subrequests per invocation, so retries are expensive and every queued-then-timed-out
+ * call is a wasted subrequest.
+ */
+
+/** Base wall-clock budget for a model call reviewing a small diff. A fast, suitable model
+ * (e.g. a Gemini flash model) answers in ~1-5s, so 20s is generous headroom; anything slower is a
+ * stuck/overloaded/queued model and we fail over quickly. */
+export const MODEL_TIMEOUT_BASE_MS = 20_000;
+/** Extra time granted per diff line beyond MODEL_TIMEOUT_FREE_LINES -- large diffs legitimately
+ * need longer generations. */
+export const MODEL_TIMEOUT_PER_LINE_MS = 100;
+/** Diff lines included in the base budget before per-line scaling kicks in. */
+export const MODEL_TIMEOUT_FREE_LINES = 100;
+/**
+ * Hard ceiling for a single model call. CRITICAL: this must stay well under the Cloudflare Worker
+ * *invocation* wall-clock limit (~120s). If one call is allowed to run near 120s (as the old
+ * 120_000 ceiling permitted for large diffs), a hung/queued call makes the whole workflow
+ * invocation get killed as `exceededCpu` -- losing all progress and looping -- instead of the call
+ * timing out gracefully and failing over. 40s leaves room for a couple of sequential fallback
+ * attempts within one invocation while never approaching the platform limit.
+ */
+export const MODEL_TIMEOUT_MAX_MS = 40_000;
+
+/**
+ * Wall-clock budget for a single file's entire fallback chain within one invocation. Even with a
+ * short per-call ceiling, a file that fails over through many configured models could otherwise
+ * run several calls back-to-back and push the invocation past the ~120s platform limit. Once a
+ * file's chain has spent this long, stop trying more models and defer it -- it resumes from the
+ * (fast) primary model in a fresh invocation. Keeps one file's chain safely under the invocation cap.
+ */
+export const MODEL_FALLBACK_CHAIN_BUDGET_MS = 55_000;
+
+/**
+ * Wall-clock timeout for one model call, scaled by the size of the (already truncated) diff
+ * the model has to review. Small diffs fail over fast; large diffs get up to MODEL_TIMEOUT_MAX_MS.
+ */
+export function adaptiveModelTimeoutMs(diffLineCount: number | null | undefined): number {
+ const lines = typeof diffLineCount === 'number' && Number.isFinite(diffLineCount) ? Math.max(0, diffLineCount) : 0;
+ const scaled = MODEL_TIMEOUT_BASE_MS + Math.max(0, lines - MODEL_TIMEOUT_FREE_LINES) * MODEL_TIMEOUT_PER_LINE_MS;
+ return Math.min(MODEL_TIMEOUT_MAX_MS, scaled);
+}
+
+/**
+ * Max model calls in flight at once for a single invocation. Kept below the runtime's
+ * 6-connection cap so short-lived KV/Hyperdrive/GitHub requests issued by concurrent file
+ * reviews still have free connection slots and model calls are never queued behind each other
+ * by the runtime (queued calls burn their timeout without ever being dispatched).
+ */
+export const MAX_CONCURRENT_MODEL_CALLS = 3;
+
+/**
+ * Tiny FIFO semaphore. Callers wait *before* their provider timeout starts, so waiting for a
+ * slot never eats into a model call's own time budget.
+ */
+export class ModelCallGate {
+ private active = 0;
+ private readonly waiters: Array<() => void> = [];
+
+ constructor(private readonly limit = MAX_CONCURRENT_MODEL_CALLS) {}
+
+ async run(fn: () => Promise): Promise {
+ await this.acquire();
+ try {
+ return await fn();
+ } finally {
+ this.release();
+ }
+ }
+
+ private acquire(): Promise {
+ if (this.active < this.limit) {
+ this.active++;
+ return Promise.resolve();
+ }
+ return new Promise((resolve) => this.waiters.push(resolve));
+ }
+
+ private release() {
+ // Hand the slot directly to the next waiter (active count unchanged) so a newly arriving
+ // caller can't sneak in between the release and the waiter resuming.
+ const next = this.waiters.shift();
+ if (next) {
+ next();
+ } else {
+ this.active--;
+ }
+ }
+}
diff --git a/src/server/models/openai.ts b/src/server/models/openai.ts
index 11f4540..aa13ab6 100644
--- a/src/server/models/openai.ts
+++ b/src/server/models/openai.ts
@@ -2,7 +2,7 @@ import { logger } from '@server/core/logger';
import { withTimeout } from '@server/core/timeout';
import { ProviderRequestError, providerErrorMessage, type ModelResponse } from './types';
-const OPENAI_TIMEOUT_MS = 180_000;
+const OPENAI_TIMEOUT_MS = 80_000;
const OPENAI_MAX_OUTPUT_TOKENS = 4096;
export interface OpenAIResponse {
@@ -63,12 +63,13 @@ function isValidPublicUrl(urlString: string) {
}
export async function reviewWithOpenAI(
- config: { apiKey: string | null; baseUrl: string; providerName: string },
+ config: { apiKey: string | null; baseUrl: string; providerName: string; timeoutMs?: number },
model: string,
input: { systemPrompt: string; userPrompt: string },
tracker?: { incrementSubrequests(count?: number): void },
): Promise {
logger.info(`Calling OpenAI-format model: ${model}`);
+ const timeoutMs = config.timeoutMs ?? OPENAI_TIMEOUT_MS;
if (!isValidPublicUrl(config.baseUrl)) {
throw new ProviderRequestError(config.providerName, 400, 'Invalid provider base URL.');
@@ -77,7 +78,7 @@ export async function reviewWithOpenAI(
const url = `${config.baseUrl.replace(/\/+$/, '')}/chat/completions`;
if (tracker) tracker.incrementSubrequests(1);
- const response = await withTimeout('OpenAI API', OPENAI_TIMEOUT_MS, (signal) =>
+ const response = await withTimeout('OpenAI API', timeoutMs, (signal) =>
fetch(url, {
method: 'POST',
signal,
diff --git a/src/server/models/types.ts b/src/server/models/types.ts
index ece7258..189a24e 100644
--- a/src/server/models/types.ts
+++ b/src/server/models/types.ts
@@ -17,6 +17,20 @@ export class ProviderRequestError extends Error {
}
}
+/**
+ * Thrown when a model responds but produces no reviewable output -- reasoning/thinking only, a
+ * response truncated at the token limit, or an empty body. The file was NOT actually reviewed, so
+ * rather than synthesizing a fake "inconclusive" pass we throw: the fallback chain tries the next
+ * model, and if none succeed the file is honestly marked `failed`. Treated as a PERMANENT failure
+ * (not transient) because the outcome is deterministic -- retrying the same model just burns quota.
+ */
+export class UnparseableModelResponseError extends Error {
+ constructor(public readonly model: string, public readonly reason: string) {
+ super(`Model ${model} produced no reviewable output (${reason}); the file review failed.`);
+ this.name = 'UnparseableModelResponseError';
+ }
+}
+
export function providerErrorMessage(errorText: string) {
try {
const parsed = JSON.parse(errorText) as unknown;
diff --git a/src/server/routes/api/jobs.ts b/src/server/routes/api/jobs.ts
index e25fd35..ecd8a6a 100644
--- a/src/server/routes/api/jobs.ts
+++ b/src/server/routes/api/jobs.ts
@@ -2,10 +2,30 @@ import { Hono } from 'hono';
import type { Context } from 'hono';
import { defaultRepoConfig, jobsQuerySchema } from '@shared/schema';
import type { AppEnv } from '@server/env';
-import { bytesToHex, getJobDetail, getJobForProcessing, insertJob, listJobs, mapJob, supersedeOlderJobs } from '@server/db/jobs';
+import { bytesToHex, cancelJob, deleteJob, getJobDetail, getJobForProcessing, insertJob, listJobs, mapJob, supersedeOlderJobs } from '@server/db/jobs';
import { jsonError } from '@server/core/http';
import { scheduleBestEffortJobMaintenance } from '@server/core/job-recovery';
import { loadRepoConfig } from '@server/core/config';
+import { logger } from '@server/core/logger';
+import type { AppBindings } from '@server/env';
+
+/**
+ * Best-effort termination of a job's Cloudflare Workflow instance. The instance id is the one we
+ * stored (workflowInstanceId) or, as a fallback, the job id we passed to REVIEW_WORKFLOW.create().
+ * .get() throws if the instance doesn't exist and .terminate() throws if it's already terminal --
+ * both are non-fatal here (there's simply nothing left to stop).
+ */
+async function terminateJobWorkflow(env: AppBindings, job: { id: string; workflowInstanceId?: string | null }) {
+ const instanceId = job.workflowInstanceId ?? job.id;
+ try {
+ const instance = await env.REVIEW_WORKFLOW.get(instanceId);
+ await instance.terminate();
+ } catch (error) {
+ logger.info(`Could not terminate workflow for job ${job.id} (already finished or never started)`, {
+ error: error instanceof Error ? error.message : String(error),
+ });
+ }
+}
function jobEtag(input: { id: string; status: string; updatedAt: string; fileCount: number; commentCount: number }) {
return `"job-${input.id}-${input.status}-${input.fileCount}-${input.commentCount}-${new Date(input.updatedAt).getTime()}"`;
@@ -59,11 +79,11 @@ export function createJobsRouter() {
return response;
});
- app.post('/:id/retry', async (c) => {
- const rawSource = await getJobForProcessing(c.env, c.req.param('id'));
- if (!rawSource) {
- return jsonError('Job not found.', 404);
- }
+ // Shared logic for "re-run" (retry) and "rerun from start". Creates a fresh job for the same PR,
+ // supersedes any older queued/running jobs, and enqueues the prepare phase. When inherit=true the
+ // new job links to its parent (retryOfJobId) and reuses already-`done` file reviews; when false
+ // it starts from scratch (no inheritance) so every file is reviewed again.
+ async function startReplacementJob(c: Context, rawSource: NonNullable>>, options: { inherit: boolean }) {
const source = mapJob(rawSource);
let configSnapshot;
try {
@@ -90,7 +110,7 @@ export function createJobsRouter() {
headRef: rawSource.head_ref,
baseRef: rawSource.base_ref,
configSnapshot,
- retryOfJobId: source.id,
+ ...(options.inherit ? { retryOfJobId: source.id } : {}),
});
// Supersede any older pending/running jobs for this PR
@@ -110,8 +130,65 @@ export function createJobsRouter() {
requestId: c.get('requestId'),
});
+ return job;
+ }
+
+ // Re-run: reuse the parent's already-completed file reviews where the model strategy still matches.
+ app.post('/:id/retry', async (c) => {
+ const rawSource = await getJobForProcessing(c.env, c.req.param('id'));
+ if (!rawSource) {
+ return jsonError('Job not found.', 404);
+ }
+ const job = await startReplacementJob(c, rawSource, { inherit: true });
return c.json({ job }, 202);
});
+ // Rerun from start: review every file again from scratch (no inheritance). Stops the current run
+ // first so two workflows don't race on the same PR.
+ app.post('/:id/rerun', async (c) => {
+ const rawSource = await getJobForProcessing(c.env, c.req.param('id'));
+ if (!rawSource) {
+ return jsonError('Job not found.', 404);
+ }
+ const source = mapJob(rawSource);
+ if (source.status === 'queued' || source.status === 'running') {
+ await terminateJobWorkflow(c.env, source);
+ }
+ const job = await startReplacementJob(c, rawSource, { inherit: false });
+ return c.json({ job }, 202);
+ });
+
+ // Stop an ongoing job: terminate its workflow and mark it 'cancelled'.
+ app.post('/:id/stop', async (c) => {
+ const id = c.req.param('id');
+ const raw = await getJobForProcessing(c.env, id);
+ if (!raw) {
+ return jsonError('Job not found.', 404);
+ }
+ const job = mapJob(raw);
+ if (job.status !== 'queued' && job.status !== 'running') {
+ return jsonError('Only a queued or running job can be stopped.', 409);
+ }
+ await terminateJobWorkflow(c.env, job);
+ await cancelJob(c.env, id);
+ const updated = await getJobForProcessing(c.env, id);
+ return c.json({ job: updated ? mapJob(updated) : job }, 200);
+ });
+
+ // Delete a job (cascades to its file reviews and comments). Stops the workflow first if running.
+ app.delete('/:id', async (c) => {
+ const id = c.req.param('id');
+ const raw = await getJobForProcessing(c.env, id);
+ if (!raw) {
+ return jsonError('Job not found.', 404);
+ }
+ const job = mapJob(raw);
+ if (job.status === 'queued' || job.status === 'running') {
+ await terminateJobWorkflow(c.env, job);
+ }
+ await deleteJob(c.env, id);
+ return c.body(null, 204);
+ });
+
return app;
}
diff --git a/src/server/routes/api/models.ts b/src/server/routes/api/models.ts
index 8d8423e..897c291 100644
--- a/src/server/routes/api/models.ts
+++ b/src/server/routes/api/models.ts
@@ -28,7 +28,6 @@ import { ProviderRequestError } from '@server/models/types';
const apiFormatSchema = z.enum(llmApiFormats);
const positiveIntegerSchema = z.number().int().positive().finite();
-const optionalLimitSchema = positiveIntegerSchema.nullable();
const modelIdSchema = z.string().trim().min(1);
const optionalUrlSchema = z.string().trim().url().nullable().optional();
const providerIdSchema = z.string().uuid();
@@ -48,9 +47,6 @@ const providerUpdateSchema = providerCreateSchema.extend({
const modelConfigUpdateSchema = z.object({
providerId: providerIdSchema,
modelName: z.string().trim().min(1),
- rpm: optionalLimitSchema,
- tpm: optionalLimitSchema,
- rpd: optionalLimitSchema,
}).strict();
const globalModelConfigSchema = z.object({
diff --git a/src/server/routes/api/settings.ts b/src/server/routes/api/settings.ts
new file mode 100644
index 0000000..774ffdc
--- /dev/null
+++ b/src/server/routes/api/settings.ts
@@ -0,0 +1,41 @@
+import { Hono } from 'hono';
+import { z } from 'zod';
+import type { AppEnv } from '@server/env';
+import { getReviewSettings, updateReviewSettings } from '@server/db/app-settings';
+import { jsonError } from '@server/core/http';
+import { reviewConcurrencyLevels, reviewMaxCommentsOptions, reviewSettingsSchema } from '@shared/schema';
+
+const reviewSettingsPatchSchema = z.object({
+ concurrencyLevel: z.enum(reviewConcurrencyLevels).optional(),
+ maxComments: z.number().int().refine(
+ (value) => (reviewMaxCommentsOptions as readonly number[]).includes(value),
+ 'Invalid max comments value.',
+ ).optional(),
+}).strict().refine(
+ (settings) => settings.concurrencyLevel !== undefined || settings.maxComments !== undefined,
+ 'At least one setting must be provided.',
+);
+
+export function createSettingsRouter() {
+ const app = new Hono();
+
+ app.get('/', async (c) => {
+ const settings = await getReviewSettings(c.env);
+ return c.json({ settings });
+ });
+
+ app.patch('/', async (c) => {
+ const body = await c.req.json().catch(() => null);
+ const parsed = reviewSettingsPatchSchema.safeParse(body);
+ if (!parsed.success) {
+ return jsonError('Invalid review settings.', 400);
+ }
+
+ const current = await getReviewSettings(c.env);
+ const next = reviewSettingsSchema.parse({ ...current, ...parsed.data });
+ await updateReviewSettings(c.env, next);
+ return c.json({ ok: true, settings: next });
+ });
+
+ return app;
+}
diff --git a/src/server/services/github.ts b/src/server/services/github.ts
index f8e9150..8d88917 100644
--- a/src/server/services/github.ts
+++ b/src/server/services/github.ts
@@ -20,7 +20,7 @@ export class GitHubService {
return this.client.createCheckRun(owner, repo, params);
}
- async updateCheckRun(owner: string, repo: string, checkRunId: number, params: { title: string; summary: string; status?: 'in_progress' | 'completed'; conclusion?: 'success' | 'neutral' | 'failure' }) {
+ async updateCheckRun(owner: string, repo: string, checkRunId: number, params: { title: string; summary: string; status?: 'in_progress' | 'completed'; conclusion?: 'success' | 'neutral' | 'failure' | 'cancelled' }) {
return this.client.updateCheckRun(owner, repo, checkRunId, params);
}
diff --git a/src/server/services/model.ts b/src/server/services/model.ts
index 144a9a2..46a83a3 100644
--- a/src/server/services/model.ts
+++ b/src/server/services/model.ts
@@ -1,19 +1,21 @@
import type { AppBindings } from '../env';
import { reviewWithGoogle } from '../models/google';
-import { reviewWithCloudflare } from '../models/cloudflare';
+import { reviewWithCloudflare, submitCloudflareBatch, pollCloudflareBatch } from '../models/cloudflare';
import { reviewWithOpenAI } from '../models/openai';
import { reviewWithAnthropic } from '../models/anthropic';
import { buildFileReviewPrompts } from '../prompts/file-review';
import { buildSummaryPrompt, SUMMARY_SYSTEM_PROMPT } from '../prompts/summary';
import { parseFileReviewResponse } from '../core/model-output';
-import { truncateFileDiff } from '../core/diff';
+import { truncateFileDiff, chunkFileDiff } from '../core/diff';
import type { RepoConfig } from '@shared/schema';
import type { TokenTracker } from '../core/token-tracker';
-import type { ModelResponse } from '../models/types';
+import { UnparseableModelResponseError, type ModelResponse } from '../models/types';
import { logger } from '../core/logger';
import { normalizeModelId } from '@shared/schema';
+import { isTimeoutMessage, matchesAnyTransientSubstring } from '@shared/transient-errors';
import { getResolvedModelConfig, type ResolvedModelConfig } from '@server/db/model-configs';
import { decryptLlmApiKey } from '@server/core/llm-crypto';
+import { ModelCallGate, adaptiveModelTimeoutMs, MODEL_FALLBACK_CHAIN_BUDGET_MS } from '../models/limits';
const PROVIDER_UNAVAILABLE_TTL_SECONDS = 24 * 60 * 60;
const COMPACT_REVIEW_PROMPT_LINE_CAP = 400;
@@ -57,33 +59,74 @@ function isCloudflareAllocationError(error: unknown) {
function isGoogleRateLimitError(error: unknown) {
const message = error instanceof Error ? error.message : String(error);
- return message.includes('429') || message.includes('RESOURCE_EXHAUSTED') || message.toLowerCase().includes('quota exceeded');
+ const lower = message.toLowerCase();
+
+ if (lower.includes('timed out') || lower.includes('timeout')) {
+ return false;
+ }
+
+ return lower.includes('429') || lower.includes('resource_exhausted') || lower.includes('quota exceeded');
}
function isTransientModelFailure(error: unknown) {
if (isRetryableModelError(error)) return true;
+ // No reviewable output (reasoning-only / truncated / empty) is deterministic -- never retry it.
+ if (error instanceof UnparseableModelResponseError) return false;
if (isCloudflareAllocationError(error)) return false;
const message = error instanceof Error ? error.message : String(error);
const lower = message.toLowerCase();
+ // Explicitly fail fast for timeouts so they don't loop endlessly
+ if (isTimeoutMessage(lower)) {
+ return false;
+ }
+
return (
isGoogleRateLimitError(error) ||
- /\b50[0-9]\b/.test(message) ||
- lower.includes('internal error') ||
- lower.includes('unavailable') ||
- lower.includes('high demand') ||
- lower.includes('timeout') ||
- lower.includes('timed out') ||
+ matchesAnyTransientSubstring(lower) ||
lower.includes('fetch failed') ||
lower.includes('network') ||
lower.includes('temporar') ||
- lower.includes('returned no review content') ||
- lower.includes('empty response') ||
- lower.includes('[redacted]')
+ // Upstream 5xx (e.g. Gemini's frequent "request failed with 500: Internal error encountered")
+ // is a transient server-side outage, not a deterministic client error. Without this a sustained
+ // 5xx run makes every model in the chain throw a non-transient error, so the file is marked
+ // permanently failed instead of being deferred and retried once the provider recovers.
+ /\b50[0-9]\b/.test(lower) ||
+ lower.includes('internal error')
);
}
export class ModelService {
+ // Model configs don't change during a single review invocation, but resolveModel() is called
+ // once per file *and* once per fallback model. Left uncached that's a Hyperdrive round-trip
+ // (a counted subrequest) for every one of those, which both burns the per-invocation
+ // subrequest budget (shrinking how many files a chunk can review in parallel) and floods the
+ // connection pool. Memoize per ModelService instance (one instance == one invocation/chunk)
+ // so each distinct model is resolved from the DB at most once. Cache the in-flight promise
+ // (not just the settled value) so concurrent resolveModel() calls for the same model made
+ // before the first DB round-trip completes all await the same request instead of each firing
+ // their own.
+ private readonly resolvedModelCache = new Map>();
+
+ // The Workers runtime allows only 6 simultaneous connections per invocation; anything beyond
+ // that is queued without starting. When several files review in parallel, un-gated model
+ // calls queue behind each other and burn their entire client timeout before the request is
+ // even dispatched (observed as a provider "timing out" at exactly the configured timeout on
+ // every attempt). Gate all outbound model calls for this invocation so a call's timeout only
+ // starts once it actually has a connection slot.
+ private readonly callGate = new ModelCallGate();
+
+ // Provider-unavailable markers live in KV and every read is a counted subrequest. The marker
+ // can't flip from set back to unset within one invocation, so cache lookups per instance
+ // (one instance == one invocation) instead of re-reading KV for every file in the chunk.
+ private readonly providerUnavailableCache = new Map>();
+
+ // Models proven (this invocation) not to support the async batch queue. try-async-then-fallback
+ // means the first file probes async; if that fails, every later file in the same chunk skips the
+ // probe and goes straight to the synchronous path, so a non-async model isn't charged an extra
+ // (potentially full-inference) submit attempt per file.
+ private readonly asyncUnsupportedModels = new Set();
+
constructor(
private env: AppBindings,
private tracker?: TokenTracker,
@@ -94,25 +137,37 @@ export class ModelService {
return this.options.jobId ? `jobs:${this.options.jobId}:provider-unavailable:${providerId}` : null;
}
- private async isProviderUnavailable(providerId: string) {
+ private isProviderUnavailable(providerId: string): Promise {
const key = this.providerUnavailableKey(providerId);
- if (!key) return false;
+ if (!key) return Promise.resolve(false);
- try {
- return (await this.env.APP_KV.get(key)) !== null;
- } catch (error) {
- logger.warn(`Failed to read unavailable provider marker for ${providerId}`, {
- error: error instanceof Error ? error.message : String(error),
- });
- return false;
+ let pending = this.providerUnavailableCache.get(providerId);
+ if (!pending) {
+ pending = (async () => {
+ try {
+ this.tracker?.incrementSubrequests(1);
+ return (await this.env.APP_KV.get(key)) !== null;
+ } catch (error) {
+ logger.warn(`Failed to read unavailable provider marker for ${providerId}`, {
+ error: error instanceof Error ? error.message : String(error),
+ });
+ return false;
+ }
+ })();
+ this.providerUnavailableCache.set(providerId, pending);
}
+ return pending;
}
private async markProviderUnavailable(providerId: string, reason: string) {
const key = this.providerUnavailableKey(providerId);
if (!key) return;
+ // Keep the in-invocation cache consistent with what we just wrote.
+ this.providerUnavailableCache.set(providerId, Promise.resolve(true));
+
try {
+ this.tracker?.incrementSubrequests(1);
await this.env.APP_KV.put(
key,
JSON.stringify({
@@ -161,7 +216,17 @@ export class ModelService {
private async resolveModel(model: string) {
const normalized = normalizeModel(model);
- const resolved = await getResolvedModelConfig(this.env, normalized);
+ let pending = this.resolvedModelCache.get(normalized);
+ if (!pending) {
+ // Cache the DB answer -- including a null "not configured" result -- so a missing or
+ // repeatedly-used model isn't re-queried for every file in the chunk.
+ pending = getResolvedModelConfig(this.env, normalized);
+ this.resolvedModelCache.set(normalized, pending);
+ // A failed lookup (e.g. a transient DB error) shouldn't poison the cache for the rest of
+ // the invocation -- drop it so the next call retries instead of rejecting immediately.
+ pending.catch(() => this.resolvedModelCache.delete(normalized));
+ }
+ const resolved = await pending;
if (!resolved) {
throw new Error(`Model ${normalized} is not configured. Add it in Settings before using it in a route.`);
}
@@ -183,43 +248,59 @@ export class ModelService {
private async callResolvedModel(
config: ResolvedModelConfig,
input: { systemPrompt: string; userPrompt: string },
+ timeoutMs?: number,
): Promise {
+ // Resolve credentials *before* taking a gate slot so slow KV/crypto work never occupies a
+ // model-call slot, then run the actual provider request under the gate. The provider's
+ // timeout only starts inside the gated call, so time spent waiting for a slot is free.
if (config.apiFormat === 'cloudflare-workers-ai') {
- return reviewWithCloudflare(this.env, config.modelName, input, this.tracker, config.providerName);
+ return this.callGate.run(() =>
+ reviewWithCloudflare(this.env, config.modelName, input, this.tracker, config.providerName, { timeoutMs }),
+ );
}
if (config.apiFormat === 'gemini') {
- return reviewWithGoogle(
- { apiKey: await this.decryptApiKey(config), baseUrl: config.baseUrl, providerName: config.providerName },
- config.modelName,
- input,
- this.tracker,
+ const apiKey = await this.decryptApiKey(config);
+ return this.callGate.run(() =>
+ reviewWithGoogle(
+ { apiKey, baseUrl: config.baseUrl, providerName: config.providerName, timeoutMs },
+ config.modelName,
+ input,
+ this.tracker,
+ ),
);
}
if (config.apiFormat === 'openai') {
- return reviewWithOpenAI(
- {
- apiKey: await this.decryptApiKey(config),
- baseUrl: config.baseUrl || 'https://api.openai.com/v1',
- providerName: config.providerName,
- },
- config.modelName,
- input,
- this.tracker,
+ const apiKey = await this.decryptApiKey(config);
+ return this.callGate.run(() =>
+ reviewWithOpenAI(
+ {
+ apiKey,
+ baseUrl: config.baseUrl || 'https://api.openai.com/v1',
+ providerName: config.providerName,
+ timeoutMs,
+ },
+ config.modelName,
+ input,
+ this.tracker,
+ ),
);
}
- return reviewWithAnthropic(
- { apiKey: await this.decryptApiKey(config), baseUrl: config.baseUrl, providerName: config.providerName },
- config.modelName,
- input,
- this.tracker,
+ const apiKey = await this.decryptApiKey(config);
+ return this.callGate.run(() =>
+ reviewWithAnthropic(
+ { apiKey, baseUrl: config.baseUrl, providerName: config.providerName, timeoutMs },
+ config.modelName,
+ input,
+ this.tracker,
+ ),
);
}
- private async callModel(model: string, input: { systemPrompt: string; userPrompt: string }): Promise {
- return this.callResolvedModel(await this.resolveModel(model), input);
+ private async callModel(model: string, input: { systemPrompt: string; userPrompt: string }, timeoutMs?: number): Promise {
+ return this.callResolvedModel(await this.resolveModel(model), input, timeoutMs);
}
async reviewFile(params: {
@@ -234,10 +315,170 @@ export class ModelService {
const modelLineCap = params.compactPrompt
? Math.min(configuredLineCap, COMPACT_REVIEW_PROMPT_LINE_CAP)
: configuredLineCap;
- const reviewFile = truncateFileDiff(params.file, modelLineCap);
+
+ let chunks = chunkFileDiff(params.file, modelLineCap);
+ // Remember the pre-cap chunk count so wasPromptTruncated doesn't have to re-run chunkFileDiff.
+ const totalChunkCount = chunks.length;
+
+ // Cap chunks to prevent single files from burning all subrequests and getting stuck.
+ const MAX_CHUNKS = 4;
+ if (chunks.length > MAX_CHUNKS) {
+ chunks = chunks.slice(0, MAX_CHUNKS);
+ }
+
+ if (chunks.length === 1) {
+ return this.reviewFileChunk({ ...params, file: chunks[0] });
+ }
+
+ const results: Array, reviewedLineCount: number, wasPromptTruncated: boolean, userPrompt: string }> = [];
+
+ for (const chunk of chunks) {
+ // Don't start a new chunk if we are dangerously close to the 50 subrequest limit.
+ if (results.length > 0 && this.tracker?.isNearLimit()) {
+ logger.warn(`Stopping chunk processing for ${params.file.path} early due to subrequest budget limits.`);
+ break;
+ }
+
+ try {
+ const res = await this.reviewFileChunk({ ...params, file: chunk });
+ results.push(res as any);
+ } catch (error) {
+ if (results.length === 0) {
+ throw error; // First chunk failed, let it defer/fail properly
+ }
+ logger.warn(`Chunk review failed for ${params.file.path}, returning partial results to avoid stalling the job.`, { error: error instanceof Error ? error.message : String(error) });
+ break;
+ }
+ }
+
+ const combinedFindings = results.flatMap(r => r.parsed.comments);
+ // Report the file with the most serious chunk's verdict/summary/correctness, not just the last
+ // chunk's: taking `results[results.length - 1]` would let a clean final chunk mask real findings
+ // from an earlier chunk of the same file (reporting verdict 'approve' while carrying its comments).
+ const primaryResult = results.find(r => r.parsed.verdict === 'comment') ?? results[results.length - 1];
+
+ return {
+ ...primaryResult,
+ inputTokens: results.reduce((sum, r) => sum + r.inputTokens, 0),
+ outputTokens: results.reduce((sum, r) => sum + r.outputTokens, 0),
+ parsed: {
+ ...primaryResult.parsed,
+ comments: combinedFindings,
+ },
+ reviewedLineCount: results.reduce((sum, r) => sum + r.reviewedLineCount, 0),
+ wasPromptTruncated: chunks.length < totalChunkCount || results.length < chunks.length,
+ };
+ }
+
+ /**
+ * Try to submit a file's review to the Workers AI asynchronous batch queue. Returns the queue
+ * request_id and the model it was submitted to, or null when async batching isn't usable for
+ * the primary model (non-Cloudflare provider, or the model/account doesn't support queueing) --
+ * in which case the caller falls back to the synchronous reviewFile path. This decouples slow
+ * (e.g. reasoning) model inference from the per-invocation timeout and subrequest cap.
+ */
+ async submitReviewBatch(params: {
+ file: any;
+ prTitle: string | null;
+ prDescription: string | null;
+ config: RepoConfig;
+ totalLineCount: number;
+ compactPrompt?: boolean;
+ }): Promise<{ requestId: string; model: string } | null> {
+ const { primary } = this.selectModel({ totalLineCount: params.totalLineCount, config: params.config });
+
+ let resolved: ResolvedModelConfig;
+ try {
+ resolved = await this.resolveModel(primary);
+ } catch {
+ return null;
+ }
+ // Only Cloudflare Workers AI exposes the async batch queue; other providers use the sync path.
+ if (resolved.apiFormat !== 'cloudflare-workers-ai') return null;
+ // Skip the probe for a model already shown not to support async queueing this invocation.
+ if (this.asyncUnsupportedModels.has(resolved.modelName)) return null;
+
+ const configuredLineCap = params.config.review.max_diff_lines_per_file;
+ const modelLineCap = params.compactPrompt
+ ? Math.min(configuredLineCap, COMPACT_REVIEW_PROMPT_LINE_CAP)
+ : configuredLineCap;
+ const file = truncateFileDiff(params.file, modelLineCap);
const { systemPrompt, userPrompt } = buildFileReviewPrompts({
...params,
- file: reviewFile,
+ file,
+ config: params.config.review,
+ });
+
+ try {
+ const requestId = await this.callGate.run(() =>
+ submitCloudflareBatch(this.env, resolved.modelName, { systemPrompt, userPrompt }, this.tracker),
+ );
+ return { requestId, model: resolved.modelName };
+ } catch (error) {
+ // Any failure here (async unsupported, transient submit error) is non-fatal: the caller
+ // reviews the file synchronously instead. Remember the model so sibling files this
+ // invocation don't each pay the failed probe.
+ this.asyncUnsupportedModels.add(resolved.modelName);
+ logger.warn(`Async batch submit unavailable for ${resolved.modelName}; using synchronous review`, {
+ error: error instanceof Error ? error.message : String(error),
+ });
+ return null;
+ }
+ }
+
+ /**
+ * Poll a previously submitted async batch review. Returns 'pending' while still queued/running,
+ * 'done' with the parsed review once complete, or 'failed' if the poll or parse errored.
+ */
+ async pollReviewBatch(params: { model: string; requestId: string; file: any }): Promise<
+ | { status: 'pending' }
+ | { status: 'done'; response: ModelResponse & { parsed: ReturnType; reviewedLineCount: number; wasPromptTruncated: boolean; userPrompt: string } }
+ | { status: 'failed'; error: unknown }
+ > {
+ let resolved: ResolvedModelConfig;
+ try {
+ resolved = await this.resolveModel(params.model);
+ } catch (error) {
+ return { status: 'failed', error };
+ }
+
+ try {
+ const poll = await this.callGate.run(() =>
+ pollCloudflareBatch(this.env, resolved.modelName, params.requestId, this.tracker, resolved.providerName),
+ );
+ if (poll.status === 'pending') return { status: 'pending' };
+
+ const response = poll.response;
+ if (this.tracker) {
+ this.tracker.record(response.modelUsed, response.inputTokens, response.outputTokens);
+ }
+ const parsed = parseFileReviewResponse(response.rawText, params.file);
+ return {
+ status: 'done',
+ response: {
+ ...response,
+ parsed,
+ userPrompt: '',
+ reviewedLineCount: params.file.lineCount,
+ wasPromptTruncated: params.file.isTruncated === true,
+ },
+ };
+ } catch (error) {
+ return { status: 'failed', error };
+ }
+ }
+
+ private async reviewFileChunk(params: {
+ file: any;
+ prTitle: string | null;
+ prDescription: string | null;
+ config: RepoConfig;
+ totalLineCount: number;
+ compactPrompt?: boolean;
+ }) {
+ const { systemPrompt, userPrompt } = buildFileReviewPrompts({
+ ...params,
+ file: params.file,
config: params.config.review,
});
@@ -247,10 +488,52 @@ export class ModelService {
});
const modelsToTry = [primary, ...fallbacks];
+ // Size the per-call timeout to the diff the model actually sees: small
+ // files fail over to the next model fast; large diffs get a proportionally longer budget.
+ const timeoutMs = adaptiveModelTimeoutMs(params.file.lineCount);
+
let lastError: unknown;
let lastTransientError: unknown;
let sawTransientFailure = false;
- for (const currentModel of modelsToTry) {
+ const chainStartedAt = Date.now();
+ for (const [modelIndex, currentModel] of modelsToTry.entries()) {
+ // Always allow the first (primary) model a shot even if the shared job budget is
+ // already tight, so a file isn't punished for other files' earlier failures. But once
+ // we're into the fallback chain, each additional attempt costs more subrequests
+ // (config lookup + provider call, sometimes a provider-availability check too) that
+ // could tip this whole invocation over Cloudflare's per-invocation subrequest cap
+ // (Workers Free plan: 50). Defer the file for a later retry instead of gambling the
+ // rest of the invocation's budget on a low-probability extra fallback.
+ if (modelIndex > 0 && this.tracker?.isNearLimit()) {
+ logger.warn(`Skipping remaining fallback models for ${params.file.path}; subrequest budget for this invocation is nearly exhausted`, {
+ skippedModels: modelsToTry.slice(modelIndex),
+ });
+
+ // If we haven't seen any transient failures (e.g. they were all permanent timeouts),
+ // don't force this to become a transient failure. Just break and let the last permanent error propagate.
+ if (sawTransientFailure) {
+ lastTransientError = lastTransientError ?? lastError ?? new Error('Subrequest budget for this invocation was nearly exhausted before trying all configured fallback models');
+ }
+ break;
+ }
+
+ // Stop walking the fallback chain once this file has consumed its wall-clock budget: a long
+ // chain of slow/timing-out models could otherwise run several calls back-to-back and push the
+ // whole workflow invocation past Cloudflare's ~120s limit (killing it as `exceededCpu` and
+ // losing all progress). Defer instead -- the file resumes from the fast primary model in a
+ // fresh invocation. Always let the primary (modelIndex 0) run first.
+ if (modelIndex > 0 && Date.now() - chainStartedAt > MODEL_FALLBACK_CHAIN_BUDGET_MS) {
+ logger.warn(`Deferring ${params.file.path}: fallback chain exceeded its per-invocation time budget`, {
+ elapsedMs: Date.now() - chainStartedAt,
+ skippedModels: modelsToTry.slice(modelIndex),
+ });
+ // Treat as a transient/deferrable outcome so the file is retried on a fresh budget rather
+ // than marked permanently failed.
+ sawTransientFailure = true;
+ lastTransientError = lastTransientError ?? lastError ?? new Error(`Model fallback chain for ${params.file.path} exceeded its time budget; deferring for retry.`);
+ break;
+ }
+
let resolved: ResolvedModelConfig;
try {
resolved = await this.resolveModel(currentModel);
@@ -267,52 +550,40 @@ export class ModelService {
continue;
}
- let attempts = 0;
- const maxAttempts = 1;
+ // One shot per model: a failed call is never retried against the same model (a retryable
+ // outage is handled by deferring the whole file to a fresh invocation), so on failure we just
+ // fall through to the next model in the fallback chain.
+ try {
+ const response = await this.callResolvedModel(resolved, { systemPrompt, userPrompt }, timeoutMs);
- while (attempts < maxAttempts) {
- try {
- const response = await this.callResolvedModel(resolved, { systemPrompt, userPrompt });
-
- if (this.tracker) {
- this.tracker.record(response.modelUsed, response.inputTokens, response.outputTokens);
- }
-
- const parsed = parseFileReviewResponse(response.rawText, params.file);
- return {
- ...response,
- parsed,
- userPrompt,
- reviewedLineCount: reviewFile.lineCount,
- wasPromptTruncated: reviewFile.isTruncated === true,
- };
- } catch (error) {
- lastError = error;
- if (isTransientModelFailure(error)) {
- sawTransientFailure = true;
- lastTransientError = error;
- }
- attempts++;
- if (resolved.apiFormat === 'cloudflare-workers-ai' && isCloudflareAllocationError(error)) {
- await this.markProviderUnavailable(resolved.providerId, error instanceof Error ? error.message : String(error));
- }
-
- const isRateLimit = isGoogleRateLimitError(error);
- const isRetryable = false;
- const errorMessage = error instanceof Error ? error.message : String(error);
-
- logger.warn(`Model ${currentModel} failed for ${params.file.path} (attempt ${attempts}/${maxAttempts})`, {
- error: errorMessage,
- rateLimited: isRateLimit,
- willRetrySameModel: isRetryable,
- willTryFallback: !isRetryable && modelsToTry.indexOf(currentModel) < modelsToTry.length - 1
- });
+ if (this.tracker) {
+ this.tracker.record(response.modelUsed, response.inputTokens, response.outputTokens);
+ }
- if (isRetryable) {
- continue;
- }
- break; // Move to next model in fallbacks
+ const parsed = parseFileReviewResponse(response.rawText, params.file);
+ return {
+ ...response,
+ parsed,
+ userPrompt,
+ reviewedLineCount: params.file.lineCount,
+ wasPromptTruncated: params.file.isTruncated === true,
+ };
+ } catch (error) {
+ lastError = error;
+ if (isTransientModelFailure(error)) {
+ sawTransientFailure = true;
+ lastTransientError = error;
}
+ if (resolved.apiFormat === 'cloudflare-workers-ai' && isCloudflareAllocationError(error)) {
+ await this.markProviderUnavailable(resolved.providerId, error instanceof Error ? error.message : String(error));
+ }
+
+ logger.warn(`Model ${currentModel} failed for ${params.file.path}`, {
+ error: error instanceof Error ? error.message : String(error),
+ rateLimited: isGoogleRateLimitError(error),
+ willTryFallback: modelIndex < modelsToTry.length - 1,
+ });
+ // Fall through to the next model in the fallback chain.
}
}
@@ -361,7 +632,7 @@ export class ModelService {
const response = await this.callResolvedModel(resolved, {
systemPrompt: SUMMARY_SYSTEM_PROMPT,
userPrompt: buildSummaryPrompt(params),
- });
+ }, adaptiveModelTimeoutMs(0));
if (this.tracker) {
this.tracker.record(response.modelUsed, response.inputTokens, response.outputTokens);
diff --git a/src/server/workflows/review.ts b/src/server/workflows/review.ts
index 7ffddf9..77cf844 100644
--- a/src/server/workflows/review.ts
+++ b/src/server/workflows/review.ts
@@ -5,9 +5,18 @@ import { type ReviewJobMessage } from '@shared/schema';
import { setJobWorkflowInstance } from '@server/db/jobs';
import { logger } from '@server/core/logger';
import { runBestEffortJobMaintenance } from '@server/core/job-recovery';
+import { runWithDb } from '@server/db/client';
export class ReviewWorkflow extends WorkflowEntrypoint {
async run(event: WorkflowEvent, step: WorkflowStep) {
+ // Share one DB client/connection for this entire invocation instead of opening a
+ // new Hyperdrive connection per query (the default behavior of getDb() outside any
+ // runWithDb() context). On workflow replay after a step.sleep or a resumed retry,
+ // this simply runs again and creates a fresh client for that new invocation.
+ return runWithDb(this.env, () => this.execute(event, step));
+ }
+
+ private async execute(event: WorkflowEvent, step: WorkflowStep) {
const params = event.payload;
const env = this.env;
@@ -50,7 +59,7 @@ export class ReviewWorkflow extends WorkflowEntrypoint {
- return await runReviewJob(env, { ...params, phase: currentPhase });
+ return await runReviewJob(env, { ...params, phase: currentPhase, workflowInstanceId: event.instanceId });
});
} catch (error) {
await step.do(`telemetry-failure-${currentPhase}-${attempt}`, async () => {
@@ -86,6 +95,11 @@ export class ReviewWorkflow extends WorkflowEntrypoint {
await runBestEffortJobMaintenance(env);
diff --git a/src/shared/schema.ts b/src/shared/schema.ts
index 0e39d13..8cfd897 100644
--- a/src/shared/schema.ts
+++ b/src/shared/schema.ts
@@ -1,7 +1,7 @@
import { z } from 'zod';
export const reviewTriggers = ['auto', 'mention', 'retry'] as const;
-export const jobStatuses = ['queued', 'running', 'done', 'failed', 'superseded'] as const;
+export const jobStatuses = ['queued', 'running', 'done', 'failed', 'superseded', 'cancelled'] as const;
export const fileStatuses = ['pending', 'done', 'skipped', 'failed'] as const;
export const reviewVerdicts = ['approve', 'comment'] as const;
export const reviewSeverities = ['P0', 'P1', 'P2', 'P3', 'nit'] as const;
@@ -160,6 +160,12 @@ export const reviewJobMessageSchema = z.object({
commitSha: z.string().min(1).optional(),
trigger: z.enum(reviewTriggers).optional(),
requestId: z.string().optional(),
+ // The actual Cloudflare Workflow instance id, injected by the workflow so runReviewJob can bind
+ // it to the resolved job row (webhook jobs can't be bound at instance-create time).
+ workflowInstanceId: z.string().optional(),
+ // Set by lease recovery so the queue consumer creates a FRESH instance (keyed on deliveryId)
+ // instead of colliding with the dead instance that is still keyed on jobId.
+ forceFreshInstance: z.boolean().optional(),
}).superRefine((message, ctx) => {
if (message.jobId || message.eventName) {
return;
@@ -302,6 +308,35 @@ export const statsSchema = z.object({
jobs: z.number().int(),
}),
),
+ statuses: z.array(
+ z.object({
+ status: z.enum(jobStatuses),
+ count: z.number().int(),
+ }),
+ ),
+ triggers: z.array(
+ z.object({
+ trigger: z.enum(reviewTriggers),
+ count: z.number().int(),
+ }),
+ ),
+ severities: z.array(
+ z.object({
+ severity: z.enum(reviewSeverities),
+ count: z.number().int(),
+ }),
+ ),
+ categories: z.array(
+ z.object({
+ category: z.enum(reviewCategories),
+ count: z.number().int(),
+ }),
+ ),
+ performance: z.object({
+ avgDurationMs: z.number().nullable(),
+ p95DurationMs: z.number().nullable(),
+ avgConfidence: z.number().nullable(),
+ }),
});
export type ParsedReviewComment = z.infer;
@@ -365,9 +400,6 @@ export const modelConfigSchema = z.object({
providerName: z.string(),
apiFormat: z.enum(llmApiFormats),
modelName: z.string(),
- rpm: z.number().int().nullable(),
- tpm: z.number().int().nullable(),
- rpd: z.number().int().nullable(),
updatedAt: dateStringSchema,
});
@@ -377,3 +409,20 @@ export type ModelConfig = z.infer;
export type StatsPayload = z.infer;
export const defaultRepoConfig = repoConfigSchema.parse({});
+
+export const reviewConcurrencyLevels = ['low', 'medium', 'high', 'max'] as const;
+export type ReviewConcurrencyLevel = typeof reviewConcurrencyLevels[number];
+export const REVIEW_CONCURRENCY_LIMITS: Record = {
+ low: 1,
+ medium: 2,
+ high: 3,
+ max: 4,
+};
+
+export const reviewMaxCommentsOptions = [5, 10, 15, 20] as const;
+
+export const reviewSettingsSchema = z.object({
+ concurrencyLevel: z.enum(reviewConcurrencyLevels).default('medium'),
+ maxComments: z.union([z.literal(5), z.literal(10), z.literal(15), z.literal(20)]).default(10),
+});
+export type ReviewSettings = z.infer;
diff --git a/src/shared/transient-errors.ts b/src/shared/transient-errors.ts
new file mode 100644
index 0000000..ee2dd2a
--- /dev/null
+++ b/src/shared/transient-errors.ts
@@ -0,0 +1,24 @@
+// Substrings that both the server's "should this persisted failed file-review be retried?" check
+// (isRetryableFileReviewErrorMessage, review.ts) and its "is this live model error transient?"
+// check (isTransientModelFailure, model.ts) treat as transient. Keeping the common core in one
+// place stops the two lists from silently drifting apart. Each classifier still appends its own
+// layer-specific extras (e.g. 'all configured review models failed', 'fetch failed').
+export const SHARED_TRANSIENT_ERROR_SUBSTRINGS = [
+ 'unavailable',
+ 'high demand',
+ 'returned no review content',
+ 'empty response',
+ '[redacted]',
+] as const;
+
+/** Timeouts are deliberately NOT transient here -- both classifiers fail fast on them. */
+export function isTimeoutMessage(lowerMessage: string): boolean {
+ return lowerMessage.includes('timed out') || lowerMessage.includes('timeout');
+}
+
+export function matchesAnyTransientSubstring(
+ lowerMessage: string,
+ substrings: readonly string[] = SHARED_TRANSIENT_ERROR_SUBSTRINGS,
+): boolean {
+ return substrings.some((substring) => lowerMessage.includes(substring));
+}
diff --git a/test/api.spec.ts b/test/api.spec.ts
index 27e3d15..fd40d8d 100644
--- a/test/api.spec.ts
+++ b/test/api.spec.ts
@@ -1,6 +1,7 @@
import { createApp } from '@server/app';
import { getJobForProcessing, insertJob } from '@server/db/jobs';
import { insertFileReview } from '@server/db/file-reviews';
+import { queryRows } from '@server/db/client';
import { getRepoConfigRecord } from '@server/db/repo-configs';
import { loadRepoConfig, updateGlobalConfig } from '@server/core/config';
import { GitHubClient } from '@server/core/github';
@@ -155,6 +156,126 @@ describe('Dashboard API Suite', () => {
expect(response.status).toBe(404);
});
+ it('preserves omitted review settings when patching a single setting', async () => {
+ const env = createTestEnv();
+ const token = await getAuthCookie(env);
+ const headers = {
+ Cookie: `codra_session=${token}`,
+ 'x-requested-with': 'XMLHttpRequest',
+ 'content-type': 'application/json',
+ };
+
+ try {
+ const seed = await app.request('/api/settings', {
+ method: 'PATCH',
+ headers,
+ body: JSON.stringify({ concurrencyLevel: 'low', maxComments: 5 }),
+ }, env);
+ expect(seed.status).toBe(200);
+
+ const commentsOnly = await app.request('/api/settings', {
+ method: 'PATCH',
+ headers,
+ body: JSON.stringify({ maxComments: 20 }),
+ }, env);
+ expect(commentsOnly.status).toBe(200);
+
+ const afterCommentsOnly = await app.request('/api/settings', {
+ headers: { Cookie: `codra_session=${token}` },
+ }, env);
+ expect(afterCommentsOnly.status).toBe(200);
+ await expect(afterCommentsOnly.json()).resolves.toMatchObject({
+ settings: { concurrencyLevel: 'low', maxComments: 20 },
+ });
+
+ const concurrencyOnly = await app.request('/api/settings', {
+ method: 'PATCH',
+ headers,
+ body: JSON.stringify({ concurrencyLevel: 'high' }),
+ }, env);
+ expect(concurrencyOnly.status).toBe(200);
+
+ const afterConcurrencyOnly = await app.request('/api/settings', {
+ headers: { Cookie: `codra_session=${token}` },
+ }, env);
+ expect(afterConcurrencyOnly.status).toBe(200);
+ await expect(afterConcurrencyOnly.json()).resolves.toMatchObject({
+ settings: { concurrencyLevel: 'high', maxComments: 20 },
+ });
+ } finally {
+ await app.request('/api/settings', {
+ method: 'PATCH',
+ headers,
+ body: JSON.stringify({ concurrencyLevel: 'medium', maxComments: 10 }),
+ }, env);
+ }
+ });
+
+ it('returns 400 for malformed review settings JSON', async () => {
+ const env = createTestEnv();
+ const token = await getAuthCookie(env);
+
+ const response = await app.request('/api/settings', {
+ method: 'PATCH',
+ headers: {
+ Cookie: `codra_session=${token}`,
+ 'x-requested-with': 'XMLHttpRequest',
+ 'content-type': 'application/json',
+ },
+ body: '{',
+ }, env);
+
+ expect(response.status).toBe(400);
+ await expect(response.json()).resolves.toMatchObject({ error: 'Invalid review settings.' });
+ });
+
+ it('falls back invalid stored review settings independently', async () => {
+ const env = createTestEnv();
+ const token = await getAuthCookie(env);
+
+ try {
+ await queryRows(
+ env,
+ `INSERT INTO global_settings (key, value) VALUES
+ ('review_concurrency_level', 'turbo'),
+ ('review_max_comments', '20')
+ ON CONFLICT (key) DO UPDATE SET value = EXCLUDED.value`,
+ );
+
+ const invalidConcurrency = await app.request('/api/settings', {
+ headers: { Cookie: `codra_session=${token}` },
+ }, env);
+ expect(invalidConcurrency.status).toBe(200);
+ await expect(invalidConcurrency.json()).resolves.toMatchObject({
+ settings: { concurrencyLevel: 'medium', maxComments: 20 },
+ });
+
+ await queryRows(
+ env,
+ `INSERT INTO global_settings (key, value) VALUES
+ ('review_concurrency_level', 'high'),
+ ('review_max_comments', '999')
+ ON CONFLICT (key) DO UPDATE SET value = EXCLUDED.value`,
+ );
+
+ const invalidComments = await app.request('/api/settings', {
+ headers: { Cookie: `codra_session=${token}` },
+ }, env);
+ expect(invalidComments.status).toBe(200);
+ await expect(invalidComments.json()).resolves.toMatchObject({
+ settings: { concurrencyLevel: 'high', maxComments: 10 },
+ });
+ } finally {
+ await queryRows(
+ env,
+ `INSERT INTO global_settings (key, value) VALUES
+ ('review_concurrency_level', 'medium'),
+ ('review_max_comments', '10')
+ ON CONFLICT (key) DO UPDATE SET value = EXCLUDED.value`,
+ );
+ }
+ });
+
it('rejects logout without the CSRF header', async () => {
const env = createTestEnv();
const token = await getAuthCookie(env);
@@ -375,9 +496,7 @@ describe('Dashboard API Suite', () => {
'content-type': 'application/json',
},
body: JSON.stringify({
- rpm: -1,
- tpm: Number.NaN,
- rpd: 100,
+ providerId: 'not-a-uuid',
provider: 'unknown',
}),
}, env);
@@ -491,7 +610,7 @@ describe('Dashboard API Suite', () => {
expect(response.status).toBe(200);
const data = await response.json() as ModelConfigsResponse;
const discoveredGoogleModel = data.configs.find(config => config.modelName === discoveredModelName);
- expect(discoveredGoogleModel).toMatchObject({ rpm: null, rpd: null, tpm: null });
+ expect(discoveredGoogleModel).toMatchObject({ modelName: discoveredModelName, apiFormat: 'gemini' });
expect(data.configs.some(config => config.providerName === 'Cloudflare' && config.modelName === '@cf/openai/gpt-oss-120b')).toBe(true);
expect(data.syncErrors).toEqual([]);
});
@@ -519,7 +638,7 @@ describe('Dashboard API Suite', () => {
const env = createTestEnv();
const token = await getAuthCookie(env);
await saveTestProviderApiKey(env);
- vi.spyOn(globalThis, 'fetch').mockResolvedValue(Response.json({
+ vi.spyOn(globalThis, 'fetch').mockImplementation(async () => Response.json({
error: {
code: 429,
message: 'Quota exceeded. Please retry later.',
@@ -585,7 +704,8 @@ describe('Dashboard API Suite', () => {
}, env);
expect(response.status).toBe(502);
- expect(fetchMock).toHaveBeenCalledTimes(2);
+ // GEMINI_MAX_RETRIES = 2, so a persistent 5xx is attempted 3 times before giving up.
+ expect(fetchMock).toHaveBeenCalledTimes(3);
const data = await response.json() as { error: string };
expect(data.error).toContain('Internal error encountered.');
});
@@ -840,6 +960,76 @@ describe('Dashboard API Suite', () => {
});
});
+ it('stops an ongoing job: marks it cancelled and terminates the workflow', async () => {
+ const env = createTestEnv();
+ const token = await getAuthCookie(env);
+ const job = await insertJob(env, {
+ installationId: '123', owner: 'api-test-owner', repo: `stop-${Date.now()}`, prNumber: 1,
+ prTitle: 'Stop', prAuthor: 'author', commitSha: 'a'.repeat(40), baseSha: 'b'.repeat(40),
+ trigger: 'auto', headRef: 'feature', baseRef: 'main',
+ });
+
+ const response = await app.request(`/api/jobs/${job.id}/stop`, {
+ method: 'POST',
+ headers: { Cookie: `codra_session=${token}`, 'x-requested-with': 'XMLHttpRequest' },
+ }, env);
+
+ expect(response.status).toBe(200);
+ const body = await response.json() as { job: { status: string } };
+ expect(body.job.status).toBe('cancelled');
+ expect((env.REVIEW_WORKFLOW as any).terminated).toContain(job.id);
+
+ const row = await getJobForProcessing(env, job.id);
+ expect(row?.status).toBe('cancelled');
+
+ // Stopping an already-terminal job is a 409.
+ const second = await app.request(`/api/jobs/${job.id}/stop`, {
+ method: 'POST',
+ headers: { Cookie: `codra_session=${token}`, 'x-requested-with': 'XMLHttpRequest' },
+ }, env);
+ expect(second.status).toBe(409);
+ });
+
+ it('deletes a job (and it is gone afterwards)', async () => {
+ const env = createTestEnv();
+ const token = await getAuthCookie(env);
+ const job = await insertJob(env, {
+ installationId: '123', owner: 'api-test-owner', repo: `delete-${Date.now()}`, prNumber: 1,
+ prTitle: 'Delete', prAuthor: 'author', commitSha: 'a'.repeat(40), baseSha: 'b'.repeat(40),
+ trigger: 'auto', headRef: 'feature', baseRef: 'main',
+ });
+
+ const response = await app.request(`/api/jobs/${job.id}`, {
+ method: 'DELETE',
+ headers: { Cookie: `codra_session=${token}`, 'x-requested-with': 'XMLHttpRequest' },
+ }, env);
+
+ expect(response.status).toBe(204);
+ expect(await getJobForProcessing(env, job.id)).toBeNull();
+ });
+
+ it('reruns a job from start: creates a fresh job that does NOT inherit the parent (no retryOfJobId)', async () => {
+ const env = createTestEnv();
+ const token = await getAuthCookie(env);
+ const source = await insertJob(env, {
+ installationId: '123', owner: 'api-test-owner', repo: `rerun-${Date.now()}`, prNumber: 1,
+ prTitle: 'Rerun', prAuthor: 'author', commitSha: 'a'.repeat(40), baseSha: 'b'.repeat(40),
+ trigger: 'auto', headRef: 'feature', baseRef: 'main',
+ });
+
+ const response = await app.request(`/api/jobs/${source.id}/rerun`, {
+ method: 'POST',
+ headers: { Cookie: `codra_session=${token}`, 'x-requested-with': 'XMLHttpRequest' },
+ }, env);
+
+ expect(response.status).toBe(202);
+ const body = await response.json() as { job: { id: string } };
+ expect(body.job.id).not.toBe(source.id);
+ const fresh = await getJobForProcessing(env, body.job.id);
+ // Rerun-from-start must not inherit parent file reviews, so retry_of_job_id stays null.
+ expect(fresh?.retry_of_job_id ?? null).toBeNull();
+ });
+
it('accepts legacy jobId-only queue messages during schema transition', () => {
const parsed = reviewJobMessageSchema.safeParse({
jobId: crypto.randomUUID(),
diff --git a/test/async-batch-review.spec.ts b/test/async-batch-review.spec.ts
new file mode 100644
index 0000000..871b19c
--- /dev/null
+++ b/test/async-batch-review.spec.ts
@@ -0,0 +1,128 @@
+import { runReviewJob } from '@server/core/review';
+import { createTestEnv, generateMockDiff, hasConfiguredTestDatabaseUrl } from './helpers';
+import { vi, expect } from 'vitest';
+import { findExistingJobForHead, getJobForProcessing } from '@server/db/jobs';
+import { getFileReviewsForJobs } from '@server/db/file-reviews';
+import { runWithDb, queryRows } from '@server/db/client';
+
+const sha = (char: string) => char.repeat(40);
+
+vi.mock('@server/db/jobs', async (importOriginal) => {
+ const mod = await importOriginal();
+ return { ...mod, getOtherRunningJobsCount: vi.fn().mockResolvedValue(0) };
+});
+
+vi.mock('@server/services/github', () => {
+ class MockGitHubService {
+ async getPullRequest() {
+ return { title: 'Test PR', body: 'Test Body', head: { sha: 'headsha', ref: 'feature' }, base: { sha: 'basesha', ref: 'main' }, user: { login: 'author' } };
+ }
+ async getPullRequestDiff() { return generateMockDiff([{ path: 'src/app.ts', content: 'console.log(1);' }]); }
+ async createCheckRun() { return { id: 123 }; }
+ async updateCheckRun() { return {}; }
+ async createReview() { return { id: 456 }; }
+ async ensureLabel() { return {}; }
+ async addIssueLabels() { return {}; }
+ async removeIssueLabelsIfPresent() { return {}; }
+ }
+ return { GitHubService: MockGitHubService };
+});
+
+// Controllable async-batch model: submit hands back a request_id; the first poll is still
+// pending, the second completes. reviewFile must NOT be called on the async path.
+const pollCalls = { count: 0 };
+const reviewFileSpy = vi.fn();
+vi.mock('@server/services/model', () => {
+ class MockModelService {
+ async submitReviewBatch() {
+ return { requestId: 'req-async-1', model: '@cf/moonshotai/kimi-k2.6' };
+ }
+ async pollReviewBatch() {
+ pollCalls.count += 1;
+ if (pollCalls.count < 2) return { status: 'pending' as const };
+ return {
+ status: 'done' as const,
+ response: {
+ modelUsed: '@cf/moonshotai/kimi-k2.6',
+ provider: 'Cloudflare',
+ inputTokens: 11,
+ outputTokens: 7,
+ rawText: '{"findings":[]}',
+ userPrompt: '',
+ parsed: { comments: [], verdict: 'approve' as const, fileSummary: 'ok', overallCorrectness: 'patch is correct', confidenceScore: 0.9 },
+ },
+ };
+ }
+ async reviewFile() { reviewFileSpy(); throw new Error('sync reviewFile should not be called on the async path'); }
+ async generateSummary() { return { modelUsed: 'm', provider: 'p', rawText: '{"summary":"s"}', inputTokens: 1, outputTokens: 1 }; }
+ }
+ return { ModelService: MockModelService, isRetryableModelError: (e: unknown) => Boolean(e && typeof e === 'object' && (e as any).retryable === true) };
+});
+
+const dbDescribe = hasConfiguredTestDatabaseUrl() ? describe : describe.skip;
+
+dbDescribe('Async batch review flow', () => {
+ const env = createTestEnv();
+
+ // A delayed reschedule sets last_queue_message_at into the future; claimJobLease refuses to
+ // claim until then (in prod the workflow's step.sleep waits it out). Backdate it to simulate
+ // that scheduled delay having elapsed so the next poll can claim immediately.
+ async function simulateScheduledDelayElapsed(jobId: string) {
+ await queryRows(env, `UPDATE jobs SET last_queue_message_at = now() - interval '1 second' WHERE id = $1`, [jobId]);
+ }
+
+ it('submits to the async queue, stays pending across polls, then completes and finalizes', async () => {
+ pollCalls.count = 0;
+ const repo = `test-repo-${Date.now()}-async`;
+ const headSha = sha('c');
+
+ await runWithDb(env, async () => {
+ // Phase 1: prepare (creates the job, enqueues review).
+ const prep = await runReviewJob(env, {
+ deliveryId: `delivery-async-${Date.now()}`,
+ eventName: 'pull_request',
+ payload: {
+ action: 'opened',
+ installation: { id: 123 },
+ repository: { owner: { login: 'test-owner' }, name: repo },
+ pull_request: { number: 1, head: { sha: headSha, ref: 'feature' }, base: { sha: sha('d'), ref: 'main' }, title: 'Test PR', user: { login: 'author' }, draft: false },
+ },
+ } as any);
+ expect(prep).toMatchObject({ action: 'next_phase', phase: 'review' });
+
+ const job = await findExistingJobForHead(env, { owner: 'test-owner', repo, prNumber: 1, commitSha: headSha, trigger: 'auto' });
+ const jobId = job!.id;
+
+ // Phase 2: first review invocation -> submits the async batch, persists a 'pending' row.
+ const submitResult = await runReviewJob(env, { jobId, phase: 'review' } as any);
+ expect(submitResult).toMatchObject({ action: 'next_phase', phase: 'review' });
+ let reviews = await getFileReviewsForJobs(env, [jobId]);
+ expect(reviews).toHaveLength(1);
+ expect(reviews[0].file_status).toBe('pending');
+ expect(reviews[0].async_request_id).toBe('req-async-1');
+
+ // Phase 3: poll returns pending -> stays in review phase.
+ await simulateScheduledDelayElapsed(jobId);
+ const pollPending = await runReviewJob(env, { jobId, phase: 'review' } as any);
+ expect(pollPending).toMatchObject({ action: 'next_phase', phase: 'review' });
+ reviews = await getFileReviewsForJobs(env, [jobId]);
+ expect(reviews[0].file_status).toBe('pending');
+
+ // Phase 4: poll returns done -> persists 'done', clears async bookkeeping, moves to finalize.
+ await simulateScheduledDelayElapsed(jobId);
+ const pollDone = await runReviewJob(env, { jobId, phase: 'review' } as any);
+ expect(pollDone).toMatchObject({ action: 'next_phase', phase: 'finalize' });
+ reviews = await getFileReviewsForJobs(env, [jobId]);
+ expect(reviews[0].file_status).toBe('done');
+ expect(reviews[0].async_request_id).toBeNull();
+ expect(reviews[0].model_used).toBe('@cf/moonshotai/kimi-k2.6');
+
+ // The synchronous reviewFile path must never have been used.
+ expect(reviewFileSpy).not.toHaveBeenCalled();
+ // Sanity: the batch was polled until it completed.
+ expect(pollCalls.count).toBeGreaterThanOrEqual(2);
+
+ expect(await getJobForProcessing(env, jobId)).toBeTruthy();
+ });
+ }, 60_000);
+});
diff --git a/test/chunk-concurrency.spec.ts b/test/chunk-concurrency.spec.ts
new file mode 100644
index 0000000..87a12d2
--- /dev/null
+++ b/test/chunk-concurrency.spec.ts
@@ -0,0 +1,39 @@
+import { describe, expect, it } from 'vitest';
+import { budgetAwareFileLimit } from '@server/core/review';
+import { TokenTracker } from '@server/core/token-tracker';
+import { REVIEW_CONCURRENCY_LIMITS, reviewConcurrencyLevels } from '@shared/schema';
+
+// Regression guard for the "concurrency slider is dead above medium" incident: the per-chunk
+// budget cap must NOT silently override the user's configured concurrency at a healthy budget.
+// These assertions exercise the REAL TokenTracker (so MAX_SUBREQUESTS / SAFE_MARGIN are in play)
+// and the REAL REVIEW_CONCURRENCY_LIMITS, so bumping SAFE_MARGIN or ESTIMATED_SUBREQUESTS_PER_FILE
+// back into a slider-defeating range fails this test.
+
+const maxLevel = Math.max(...reviewConcurrencyLevels.map((level) => REVIEW_CONCURRENCY_LIMITS[level]));
+
+describe('budgetAwareFileLimit', () => {
+ it('honors every configured concurrency level at a fresh budget', () => {
+ const fresh = new TokenTracker().remainingSafeBudget();
+ for (const level of reviewConcurrencyLevels) {
+ const configured = REVIEW_CONCURRENCY_LIMITS[level];
+ expect(budgetAwareFileLimit(fresh, configured)).toBe(configured);
+ }
+ });
+
+ it('still honors the highest level after the getPullRequest preamble spends a few subrequests', () => {
+ const tracker = new TokenTracker();
+ tracker.incrementSubrequests(3); // token read + getPullRequest + a little slack
+ expect(budgetAwareFileLimit(tracker.remainingSafeBudget(), maxLevel)).toBe(maxLevel);
+ });
+
+ it('throttles below the configured level only once the budget has actually been eaten into', () => {
+ // Deep into a troubled invocation the cap should shrink to protect the 50-subrequest ceiling.
+ expect(budgetAwareFileLimit(4, maxLevel)).toBe(0);
+ expect(budgetAwareFileLimit(0, maxLevel)).toBe(0);
+ expect(budgetAwareFileLimit(10, maxLevel)).toBeLessThan(maxLevel);
+ });
+
+ it('never exceeds the configured level even with a huge budget', () => {
+ expect(budgetAwareFileLimit(10_000, 2)).toBe(2);
+ });
+});
diff --git a/test/cloudflare-model.spec.ts b/test/cloudflare-model.spec.ts
new file mode 100644
index 0000000..6ff14d0
--- /dev/null
+++ b/test/cloudflare-model.spec.ts
@@ -0,0 +1,117 @@
+import { describe, it, expect, vi } from 'vitest';
+import { reviewWithCloudflare, submitCloudflareBatch, pollCloudflareBatch } from '@server/models/cloudflare';
+
+// Regression coverage for the "qwen returned no parseable review content (empty response)"
+// incident: some Workers AI models (notably @cf/qwen/qwen2.5-coder-32b-instruct honoring
+// response_format) return the completion in `response` as an already-parsed JSON object/array
+// rather than a string. extractCloudflareText used to only accept a string, so a perfectly good
+// review was discarded as an "empty response" and synthesized into an inconclusive verdict.
+
+const REVIEW_JSON = {
+ findings: [],
+ overall_correctness: 'patch is correct',
+ overall_explanation: 'Looks good.',
+ overall_confidence_score: 0.9,
+};
+
+function envReturning(result: unknown) {
+ return { AI: { async run() { return result; } } } as any;
+}
+
+const input = { systemPrompt: 'sys', userPrompt: 'user' };
+
+describe('reviewWithCloudflare response extraction', () => {
+ it('accepts a structured object response (parsed JSON) and passes it through verbatim', async () => {
+ const res = await reviewWithCloudflare(
+ envReturning({ response: REVIEW_JSON, usage: { prompt_tokens: 3, completion_tokens: 4 } }),
+ '@cf/qwen/qwen2.5-coder-32b-instruct',
+ input,
+ );
+ // Must be the real review JSON, not a synthesized "no parseable review content" fallback.
+ expect(JSON.parse(res.rawText)).toMatchObject({ overall_correctness: 'patch is correct' });
+ expect(res.rawText).not.toContain('no parseable review content');
+ expect(res.inputTokens).toBe(3);
+ expect(res.outputTokens).toBe(4);
+ });
+
+ it('accepts a structured object under a nested result.response', async () => {
+ const res = await reviewWithCloudflare(
+ envReturning({ result: { response: REVIEW_JSON } }),
+ '@cf/qwen/qwen2.5-coder-32b-instruct',
+ input,
+ );
+ expect(JSON.parse(res.rawText)).toMatchObject({ overall_explanation: 'Looks good.' });
+ expect(res.rawText).not.toContain('no parseable review content');
+ });
+
+ it('still accepts a plain string response (existing behavior)', async () => {
+ const res = await reviewWithCloudflare(
+ envReturning({ response: JSON.stringify(REVIEW_JSON) }),
+ '@cf/meta/llama-3.3-70b-instruct-fp8-fast',
+ input,
+ );
+ expect(JSON.parse(res.rawText)).toMatchObject({ overall_correctness: 'patch is correct' });
+ });
+
+ it('throws (fails the file) instead of synthesizing a fake review when the model returns nothing usable', async () => {
+ await expect(
+ reviewWithCloudflare(envReturning({ something_unexpected: true }), '@cf/qwen/qwen2.5-coder-32b-instruct', input),
+ ).rejects.toThrow(/no reviewable output/i);
+ });
+
+ it('throws on a reasoning-only / token-truncated response (marks the file failed, not inconclusive)', async () => {
+ const reasoningOnly = { choices: [{ finish_reason: 'length', message: { content: null, reasoning: 'thinking, thinking, never answering...' } }] };
+ await expect(
+ reviewWithCloudflare(envReturning(reasoningOnly), '@cf/moonshotai/kimi-k2.6', input),
+ ).rejects.toThrow(/no reviewable output/i);
+ });
+});
+
+describe('Cloudflare async batch submit/poll', () => {
+ it('submits a batch request and returns the queue request_id', async () => {
+ const run = vi.fn().mockResolvedValue({ status: 'queued', request_id: 'req-123', model: '@cf/moonshotai/kimi-k2.6' });
+ const env = { AI: { run } } as any;
+ const id = await submitCloudflareBatch(env, '@cf/moonshotai/kimi-k2.6', input);
+ expect(id).toBe('req-123');
+ // Must send a `requests` array with queueRequest option.
+ expect(run.mock.calls[0][1]).toHaveProperty('requests');
+ expect(run.mock.calls[0][2]).toMatchObject({ queueRequest: true });
+ });
+
+ it('throws when the model does not return a request_id (async unsupported → caller falls back to sync)', async () => {
+ const env = { AI: { async run() { return { response: '{"findings":[]}' }; } } } as any;
+ await expect(submitCloudflareBatch(env, '@cf/meta/llama-3.1-8b-instruct', input)).rejects.toThrow(/async queueing unsupported|did not return/i);
+ });
+
+ it('reports pending while the batch is queued or running', async () => {
+ for (const status of ['queued', 'running']) {
+ const env = { AI: { async run() { return { status, request_id: 'req-1' }; } } } as any;
+ const res = await pollCloudflareBatch(env, '@cf/moonshotai/kimi-k2.6', 'req-1');
+ expect(res.status).toBe('pending');
+ }
+ });
+
+ it('extracts the review from a completed batch (responses[] with string response)', async () => {
+ const env = { AI: { async run() {
+ return { responses: [{ id: 0, external_reference: 'src/app.ts', result: { response: JSON.stringify(REVIEW_JSON), usage: { prompt_tokens: 5, completion_tokens: 6 } } }] };
+ } } } as any;
+ const res = await pollCloudflareBatch(env, '@cf/moonshotai/kimi-k2.6', 'req-1');
+ expect(res.status).toBe('done');
+ if (res.status === 'done') {
+ expect(JSON.parse(res.response.rawText)).toMatchObject({ overall_correctness: 'patch is correct' });
+ expect(res.response.inputTokens).toBe(5);
+ expect(res.response.outputTokens).toBe(6);
+ }
+ });
+
+ it('extracts the review from a completed batch whose entry carries an object response', async () => {
+ const env = { AI: { async run() {
+ return { result: { responses: [{ id: 0, response: REVIEW_JSON }] } };
+ } } } as any;
+ const res = await pollCloudflareBatch(env, '@cf/moonshotai/kimi-k2.6', 'req-1');
+ expect(res.status).toBe('done');
+ if (res.status === 'done') {
+ expect(JSON.parse(res.response.rawText)).toMatchObject({ overall_explanation: 'Looks good.' });
+ }
+ });
+});
diff --git a/test/e2e/dashboard.spec.tsx b/test/e2e/dashboard.spec.tsx
index 4bc68dc..1245255 100644
--- a/test/e2e/dashboard.spec.tsx
+++ b/test/e2e/dashboard.spec.tsx
@@ -51,7 +51,12 @@ describe('Frontend UI Flows (JSDOM)', () => {
trend: [],
verdicts: [],
models: [],
- topRepos: []
+ topRepos: [],
+ statuses: [],
+ triggers: [],
+ severities: [],
+ categories: [],
+ performance: { avgDurationMs: null, p95DurationMs: null, avgConfidence: null },
}
});
diff --git a/test/helpers.ts b/test/helpers.ts
index 1e85f4b..b451445 100644
--- a/test/helpers.ts
+++ b/test/helpers.ts
@@ -58,10 +58,19 @@ export class MockQueue {
export class MockWorkflow {
public readonly created: any[] = [];
+ public readonly terminated: string[] = [];
async create(opts: any) {
this.created.push(opts);
}
+
+ async get(id: string) {
+ return {
+ terminate: async () => {
+ this.terminated.push(id);
+ },
+ };
+ }
}
function usableEnvValue(value: string | undefined) {
@@ -116,7 +125,6 @@ export function createTestEnv(overrides: Partial = {}): AppBindings
get ENVIRONMENT() { return unusedEnv('ENVIRONMENT'); },
get CF_API_TOKEN() { return unusedEnv('CF_API_TOKEN'); },
get CF_ACCOUNT_ID() { return unusedEnv('CF_ACCOUNT_ID'); },
- get CF_DLQ_ID() { return unusedEnv('CF_DLQ_ID'); },
...overrides,
};
}
diff --git a/test/model-config-cache.spec.ts b/test/model-config-cache.spec.ts
new file mode 100644
index 0000000..c0dae19
--- /dev/null
+++ b/test/model-config-cache.spec.ts
@@ -0,0 +1,75 @@
+import { beforeEach, describe, expect, it, vi } from 'vitest';
+import { createTestEnv } from './helpers';
+
+// Isolated in its own file: mocking @server/db/model-configs module-wide would break the
+// other model-service tests that resolve configs against the real test DB.
+const getResolvedModelConfigMock = vi.hoisted(() => vi.fn());
+
+vi.mock('@server/db/model-configs', async (importOriginal) => {
+ const mod = await importOriginal();
+ return { ...mod, getResolvedModelConfig: getResolvedModelConfigMock };
+});
+
+import { ModelService } from '@server/services/model';
+
+const cloudflareConfig = (modelId: string) => ({
+ modelId,
+ providerId: 'cf',
+ providerName: 'Cloudflare',
+ apiFormat: 'cloudflare-workers-ai' as const,
+ modelName: modelId,
+ updatedAt: new Date().toISOString(),
+ providerEnabled: true,
+ baseUrl: null,
+ encryptedApiKey: null,
+});
+
+describe('ModelService model-config caching', () => {
+ beforeEach(() => {
+ getResolvedModelConfigMock.mockReset();
+ });
+
+ it('resolves a given model config from the DB at most once per invocation', async () => {
+ getResolvedModelConfigMock.mockImplementation(async (_env: any, modelId: string) => cloudflareConfig(modelId));
+ const service = new ModelService(createTestEnv());
+
+ // The same model is resolved repeatedly across a chunk (once per file); only the first
+ // should hit the DB.
+ await (service as any).resolveModel('@cf/zai-org/glm-4.7-flash');
+ await (service as any).resolveModel('@cf/zai-org/glm-4.7-flash');
+ await (service as any).resolveModel('@cf/zai-org/glm-4.7-flash');
+
+ expect(getResolvedModelConfigMock).toHaveBeenCalledTimes(1);
+ });
+
+ it('keeps a separate cache entry per distinct model id', async () => {
+ getResolvedModelConfigMock.mockImplementation(async (_env: any, modelId: string) => cloudflareConfig(modelId));
+ const service = new ModelService(createTestEnv());
+
+ await (service as any).resolveModel('gemma-4-31b-it');
+ await (service as any).resolveModel('gemma-4-26b-a4b-it');
+ await (service as any).resolveModel('gemma-4-31b-it');
+
+ expect(getResolvedModelConfigMock).toHaveBeenCalledTimes(2);
+ });
+
+ it('caches a null "not configured" result so it is not re-queried every file', async () => {
+ getResolvedModelConfigMock.mockResolvedValue(null);
+ const service = new ModelService(createTestEnv());
+
+ await expect((service as any).resolveModel('does-not-exist')).rejects.toThrow('is not configured');
+ await expect((service as any).resolveModel('does-not-exist')).rejects.toThrow('is not configured');
+
+ expect(getResolvedModelConfigMock).toHaveBeenCalledTimes(1);
+ });
+
+ it('does not share a cache across ModelService instances (one instance == one invocation)', async () => {
+ getResolvedModelConfigMock.mockImplementation(async (_env: any, modelId: string) => cloudflareConfig(modelId));
+ const env = createTestEnv();
+
+ await (new ModelService(env) as any).resolveModel('@cf/zai-org/glm-4.7-flash');
+ await (new ModelService(env) as any).resolveModel('@cf/zai-org/glm-4.7-flash');
+
+ expect(getResolvedModelConfigMock).toHaveBeenCalledTimes(2);
+ });
+});
diff --git a/test/model-limits.spec.ts b/test/model-limits.spec.ts
new file mode 100644
index 0000000..e713cc8
--- /dev/null
+++ b/test/model-limits.spec.ts
@@ -0,0 +1,63 @@
+import { describe, expect, it } from 'vitest';
+import {
+ ModelCallGate,
+ adaptiveModelTimeoutMs,
+ MODEL_TIMEOUT_BASE_MS,
+ MODEL_TIMEOUT_MAX_MS,
+} from '../src/server/models/limits';
+
+describe('adaptiveModelTimeoutMs', () => {
+ it('uses the base budget for small diffs', () => {
+ expect(adaptiveModelTimeoutMs(0)).toBe(MODEL_TIMEOUT_BASE_MS);
+ expect(adaptiveModelTimeoutMs(100)).toBe(MODEL_TIMEOUT_BASE_MS);
+ expect(adaptiveModelTimeoutMs(undefined)).toBe(MODEL_TIMEOUT_BASE_MS);
+ expect(adaptiveModelTimeoutMs(null)).toBe(MODEL_TIMEOUT_BASE_MS);
+ });
+
+ it('scales with diff size beyond the free-line allowance', () => {
+ // Use line counts that stay below the MAX cap so the linear scaling is observable.
+ expect(adaptiveModelTimeoutMs(200)).toBe(MODEL_TIMEOUT_BASE_MS + 100 * 100);
+ expect(adaptiveModelTimeoutMs(250)).toBeGreaterThan(adaptiveModelTimeoutMs(150));
+ });
+
+ it('caps at the maximum regardless of diff size', () => {
+ expect(adaptiveModelTimeoutMs(100_000)).toBe(MODEL_TIMEOUT_MAX_MS);
+ });
+});
+
+describe('ModelCallGate', () => {
+ it('never runs more than the limit concurrently and eventually runs everything', async () => {
+ const gate = new ModelCallGate(2);
+ let active = 0;
+ let peak = 0;
+ const done: number[] = [];
+
+ const task = (id: number) =>
+ gate.run(async () => {
+ active++;
+ peak = Math.max(peak, active);
+ // Yield a couple of microtasks so tasks genuinely overlap.
+ await Promise.resolve();
+ await Promise.resolve();
+ active--;
+ done.push(id);
+ });
+
+ await Promise.all([task(1), task(2), task(3), task(4), task(5)]);
+
+ expect(peak).toBeLessThanOrEqual(2);
+ expect(done).toHaveLength(5);
+ });
+
+ it('releases the slot when a gated call rejects', async () => {
+ const gate = new ModelCallGate(1);
+
+ await expect(gate.run(async () => {
+ throw new Error('boom');
+ })).rejects.toThrow('boom');
+
+ // The slot must be free again for the next caller.
+ const result = await gate.run(async () => 'ok');
+ expect(result).toBe('ok');
+ });
+});
diff --git a/test/model-service.spec.ts b/test/model-service.spec.ts
index 00dfea6..2b250a2 100644
--- a/test/model-service.spec.ts
+++ b/test/model-service.spec.ts
@@ -4,6 +4,7 @@ import { reviewWithCloudflare } from '@server/models/cloudflare';
import { reviewWithGoogle } from '@server/models/google';
import { createTestEnv, saveTestProviderApiKey } from './helpers';
import { defaultRepoConfig } from '@shared/schema';
+import { TokenTracker } from '@server/core/token-tracker';
describe('ModelService', () => {
afterEach(() => {
@@ -60,7 +61,7 @@ describe('ModelService', () => {
})).toThrow('No review model strategy is configured');
});
- it('turns Cloudflare reasoning-only responses into inconclusive review JSON', async () => {
+ it('fails (throws) on a Cloudflare reasoning-only response instead of faking an inconclusive review', async () => {
const env = createTestEnv({
AI: {
async run() {
@@ -80,18 +81,14 @@ describe('ModelService', () => {
} as any,
});
- const response = await reviewWithCloudflare(env, '@cf/moonshotai/kimi-k2.6', {
- systemPrompt: 'system',
- userPrompt: 'user',
- });
- const parsed = JSON.parse(response.rawText);
-
- expect(parsed.findings).toEqual([]);
- expect(parsed.overall_correctness).toBe('patch is incorrect');
- expect(parsed.overall_explanation).toContain('inconclusive');
+ // The file was not actually reviewed, so this must surface as a failure (to be marked failed
+ // after the fallback chain), not a synthesized "inconclusive" pass.
+ await expect(
+ reviewWithCloudflare(env, '@cf/moonshotai/kimi-k2.6', { systemPrompt: 'system', userPrompt: 'user' }),
+ ).rejects.toThrow(/no reviewable output.*reasoning-only/i);
});
- it('does not parse Cloudflare reasoning as review JSON when final content is missing', async () => {
+ it('throws when Cloudflare final content is missing (does not parse reasoning as review JSON)', async () => {
const env = createTestEnv({
AI: {
async run() {
@@ -111,14 +108,9 @@ describe('ModelService', () => {
} as any,
});
- const response = await reviewWithCloudflare(env, '@cf/zai-org/glm-4.7-flash', {
- systemPrompt: 'system',
- userPrompt: 'user',
- });
- const parsed = JSON.parse(response.rawText);
-
- expect(parsed.findings).toEqual([]);
- expect(parsed.overall_explanation).toContain('reasoning-only response');
+ await expect(
+ reviewWithCloudflare(env, '@cf/zai-org/glm-4.7-flash', { systemPrompt: 'system', userPrompt: 'user' }),
+ ).rejects.toThrow(/no reviewable output/i);
});
it('asks Cloudflare chat models for strict review JSON', async () => {
@@ -187,6 +179,69 @@ describe('ModelService', () => {
expect(response.rawText).toContain('"findings"');
});
+ it('caps the Retry-After sleep for Google 429 responses at the max in-call retry delay', async () => {
+ // A 429 carries retry-after: 7s, but the in-call sleep is capped at GEMINI_MAX_RETRY_DELAY_MS
+ // (5s) -- a longer cool-off is handled by deferring the file to a fresh invocation, not by
+ // pinning a gate slot in-call. So the retry must fire at 5s, not 7s.
+ vi.useFakeTimers();
+ try {
+ const fetchMock = vi.spyOn(globalThis, 'fetch')
+ .mockResolvedValueOnce(
+ new Response(
+ JSON.stringify({ error: { code: 429, message: 'Rate limited.' } }),
+ { status: 429, headers: { 'content-type': 'application/json', 'retry-after': '7' } },
+ ),
+ )
+ .mockResolvedValueOnce(
+ new Response(
+ JSON.stringify({
+ candidates: [{ content: { parts: [{ text: '{"findings":[],"overall_correctness":"patch is correct","overall_explanation":"ok","overall_confidence_score":0.9}' }] } }],
+ usageMetadata: { promptTokenCount: 1, candidatesTokenCount: 1 },
+ }),
+ { status: 200, headers: { 'content-type': 'application/json' } },
+ ),
+ );
+
+ const promise = reviewWithGoogle(
+ { apiKey: 'test-key' },
+ 'gemma-4-31b-it',
+ { systemPrompt: 'system', userPrompt: 'user' },
+ );
+ promise.catch(() => {});
+
+ await vi.waitFor(() => expect(fetchMock).toHaveBeenCalledTimes(1));
+ await vi.advanceTimersByTimeAsync(4_999);
+ expect(fetchMock).toHaveBeenCalledTimes(1);
+
+ await vi.advanceTimersByTimeAsync(1);
+ const response = await promise;
+
+ expect(fetchMock).toHaveBeenCalledTimes(2);
+ expect(response.rawText).toContain('"findings"');
+ } finally {
+ vi.useRealTimers();
+ }
+ });
+
+ it('does not retry TypeErrors thrown after a successful Google response', async () => {
+ const fetchMock = vi.spyOn(globalThis, 'fetch').mockResolvedValue({
+ ok: true,
+ json: async () => {
+ throw new TypeError('parser exploded after response');
+ },
+ } as unknown as Response);
+
+ await expect(
+ reviewWithGoogle(
+ { apiKey: 'test-key' },
+ 'gemma-4-31b-it',
+ { systemPrompt: 'system', userPrompt: 'user' },
+ ),
+ ).rejects.toThrow('parser exploded after response');
+
+ expect(fetchMock).toHaveBeenCalledTimes(1);
+ });
+
it('does not spend an extra queue slice retrying the same Cloudflare model inline', async () => {
let attempts = 0;
const env = createTestEnv({
@@ -207,33 +262,50 @@ describe('ModelService', () => {
expect(attempts).toBe(1);
});
+ it('aborts and fails fast (as a retryable timeout) when a Cloudflare model hangs past the timeout', async () => {
+ vi.useFakeTimers();
+ try {
+ let capturedSignal: AbortSignal | undefined;
+ const env = createTestEnv({
+ AI: {
+ run(_model: string, _request: any, options?: { signal?: AbortSignal }) {
+ capturedSignal = options?.signal;
+ // Model never responds -- only the timeout can end this call.
+ return new Promise(() => {});
+ },
+ } as any,
+ });
+
+ const promise = reviewWithCloudflare(env, '@cf/zai-org/glm-4.7-flash', {
+ systemPrompt: 'system',
+ userPrompt: 'user',
+ });
+ // Prevent an unhandled-rejection warning while the timer is still pending.
+ promise.catch(() => {});
+
+ await vi.advanceTimersByTimeAsync(45_000);
+
+ await expect(promise).rejects.toThrow('timed out after 45000ms');
+ // The underlying Workers-AI request was actually cancelled, not just abandoned.
+ expect(capturedSignal?.aborted).toBe(true);
+ } finally {
+ vi.useRealTimers();
+ }
+ });
+
it('tries the smaller Google fallback after the primary Google model fails', async () => {
let cloudflareCalls = 0;
+ const gemini500 = () =>
+ new Response(
+ JSON.stringify({ error: { code: 500, message: 'Internal error encountered.', status: 'INTERNAL' } }),
+ { status: 500, headers: { 'content-type': 'application/json' } },
+ );
+ // GEMINI_MAX_RETRIES = 2, so the primary model makes 3 attempts (initial + 2 retries) before
+ // failing over; then the smaller fallback succeeds on the 4th call.
const fetchMock = vi.spyOn(globalThis, 'fetch')
- .mockResolvedValueOnce(
- new Response(
- JSON.stringify({
- error: {
- code: 500,
- message: 'Internal error encountered.',
- status: 'INTERNAL',
- },
- }),
- { status: 500, headers: { 'content-type': 'application/json' } },
- ),
- )
- .mockResolvedValueOnce(
- new Response(
- JSON.stringify({
- error: {
- code: 500,
- message: 'Internal error encountered.',
- status: 'INTERNAL',
- },
- }),
- { status: 500, headers: { 'content-type': 'application/json' } },
- ),
- )
+ .mockResolvedValueOnce(gemini500())
+ .mockResolvedValueOnce(gemini500())
+ .mockResolvedValueOnce(gemini500())
.mockResolvedValueOnce(
new Response(
JSON.stringify({
@@ -285,14 +357,142 @@ describe('ModelService', () => {
totalLineCount: 1,
});
- expect(fetchMock).toHaveBeenCalledTimes(3);
+ expect(fetchMock).toHaveBeenCalledTimes(4);
expect(String(fetchMock.mock.calls[0][0])).toContain('/models/gemma-4-31b-it:generateContent');
expect(String(fetchMock.mock.calls[1][0])).toContain('/models/gemma-4-31b-it:generateContent');
- expect(String(fetchMock.mock.calls[2][0])).toContain('/models/gemma-4-26b-a4b-it:generateContent');
+ expect(String(fetchMock.mock.calls[2][0])).toContain('/models/gemma-4-31b-it:generateContent');
+ expect(String(fetchMock.mock.calls[3][0])).toContain('/models/gemma-4-26b-a4b-it:generateContent');
expect(cloudflareCalls).toBe(0);
expect(response.modelUsed).toBe('gemma-4-26b-a4b-it');
});
+ it('classifies an exhausted run of Google 5xx failures as retryable (not a permanent file failure)', async () => {
+ // A sustained upstream 5xx outage across every configured model must be deferred and retried on
+ // a fresh budget, not marked permanently failed. Regression guard for isTransientModelFailure
+ // dropping 5xx / "internal error" detection.
+ // Fresh Response per call -- a Response body can only be read once, so a shared instance would
+ // make the 2nd+ fetch fail on an already-consumed body instead of on the 500 under test.
+ const fetchMock = vi.spyOn(globalThis, 'fetch').mockImplementation(async () =>
+ new Response(
+ JSON.stringify({ error: { code: 500, message: 'Internal error encountered.', status: 'INTERNAL' } }),
+ { status: 500, headers: { 'content-type': 'application/json' } },
+ ),
+ );
+ const env = createTestEnv();
+ await saveTestProviderApiKey(env);
+ const service = new ModelService(env);
+
+ await expect(
+ service.reviewFile({
+ file: { path: 'src/app.ts', lineCount: 1, hunks: [], isDeleted: false, isBinary: false, isNew: false, previousPath: null },
+ prTitle: 'Test',
+ prDescription: null,
+ config: {
+ ...defaultRepoConfig,
+ model: { main: 'gemma-4-31b-it', fallbacks: ['gemma-4-26b-a4b-it'], size_overrides: [] },
+ },
+ totalLineCount: 1,
+ }),
+ ).rejects.toSatisfy(isRetryableModelError);
+ expect(fetchMock).toHaveBeenCalled();
+ });
+
+ it('still tries the primary model even when the shared job budget is already near the subrequest limit', async () => {
+ const fetchMock = vi.spyOn(globalThis, 'fetch').mockResolvedValueOnce(
+ new Response(
+ JSON.stringify({
+ candidates: [{ content: { parts: [{ text: '{"findings":[],"overall_correctness":"patch is correct","overall_explanation":"ok","overall_confidence_score":0.9}' }] } }],
+ usageMetadata: { promptTokenCount: 1, candidatesTokenCount: 1 },
+ }),
+ { status: 200, headers: { 'content-type': 'application/json' } },
+ ),
+ );
+ const env = createTestEnv();
+ await saveTestProviderApiKey(env);
+ const tracker = new TokenTracker();
+ tracker.incrementSubrequests(40); // above the near-limit threshold (MAX_SUBREQUESTS 50 - SAFE_MARGIN 25)
+ const service = new ModelService(env, tracker);
+
+ const response = await service.reviewFile({
+ file: {
+ path: 'src/app.ts',
+ lineCount: 1,
+ hunks: [],
+ isDeleted: false,
+ isBinary: false,
+ isNew: false,
+ previousPath: null,
+ },
+ prTitle: 'Test',
+ prDescription: null,
+ config: {
+ ...defaultRepoConfig,
+ model: {
+ main: 'gemma-4-31b-it',
+ fallbacks: ['gemma-4-26b-a4b-it'],
+ size_overrides: [],
+ },
+ },
+ totalLineCount: 1,
+ });
+
+ expect(fetchMock).toHaveBeenCalledTimes(1);
+ expect(response.modelUsed).toBe('gemma-4-31b-it');
+ });
+
+ it('skips remaining fallback models (instead of spending more of the shared budget) once near the subrequest limit', async () => {
+ // Google's client retries a 5xx once internally before giving up on a model, so the
+ // primary model alone can issue more than one raw fetch call; return a fresh Response for
+ // every call so retries do not reuse an already-consumed body. Use a 503/"unavailable"
+ // (a genuinely transient failure) so the near-limit skip produces a retryable deferral --
+ // a 500 "internal error" is now treated as a permanent failure and would not defer.
+ const fetchMock = vi.spyOn(globalThis, 'fetch').mockImplementation(async () =>
+ new Response(
+ JSON.stringify({ error: { code: 503, message: 'The model is overloaded and currently unavailable.', status: 'UNAVAILABLE' } }),
+ { status: 503, headers: { 'content-type': 'application/json' } },
+ ),
+ );
+ const env = createTestEnv();
+ await saveTestProviderApiKey(env);
+ const tracker = new TokenTracker();
+ tracker.incrementSubrequests(40); // above the near-limit threshold (MAX_SUBREQUESTS 50 - SAFE_MARGIN 25)
+ const service = new ModelService(env, tracker);
+
+ await expect(
+ service.reviewFile({
+ file: {
+ path: 'src/app.ts',
+ lineCount: 1,
+ hunks: [],
+ isDeleted: false,
+ isBinary: false,
+ isNew: false,
+ previousPath: null,
+ },
+ prTitle: 'Test',
+ prDescription: null,
+ config: {
+ ...defaultRepoConfig,
+ model: {
+ main: 'gemma-4-31b-it',
+ fallbacks: ['gemma-4-26b-a4b-it'],
+ size_overrides: [],
+ },
+ },
+ totalLineCount: 1,
+ }),
+ ).rejects.toSatisfy(isRetryableModelError);
+
+ // Only the primary model was attempted (possibly with its own internal retry); the
+ // fallback model was skipped rather than risking tipping the shared invocation over
+ // Cloudflare's subrequest cap. The file is deferred for a later retry (via the
+ // RetryableModelError) instead of being burned through here.
+ expect(fetchMock.mock.calls.length).toBeGreaterThan(0);
+ for (const call of fetchMock.mock.calls) {
+ expect(String(call[0])).toContain('/models/gemma-4-31b-it:generateContent');
+ }
+ });
+
it('marks exhausted transient provider failures as retryable for the queue', async () => {
const env = createTestEnv({
AI: {
@@ -406,10 +606,10 @@ describe('ModelService', () => {
expect(fetchMock).toHaveBeenCalledTimes(2);
});
- it('uses the configured Gemma prompt cap and output token budget on the first attempt', async () => {
- let requestBody: any = null;
+ it('splits an oversized Gemma diff into capped chunks and reviews each in its own call', async () => {
+ const requestBodies: any[] = [];
const fetchMock = vi.spyOn(globalThis, 'fetch').mockImplementation(async (_url, init) => {
- requestBody = JSON.parse(String(init?.body));
+ requestBodies.push(JSON.parse(String(init?.body)));
return new Response(
JSON.stringify({
candidates: [{ content: { parts: [{ text: '{"findings":[],"overall_correctness":"patch is correct","overall_explanation":"ok","overall_confidence_score":0.9}' }] } }],
@@ -456,20 +656,27 @@ describe('ModelService', () => {
totalLineCount: 500,
});
- const userPrompt = requestBody.contents[0].parts[0].text as string;
- expect(fetchMock).toHaveBeenCalledOnce();
- expect(requestBody.generationConfig.maxOutputTokens).toBe(4096);
- expect(userPrompt).toContain('[NOTE: This diff has been truncated from 900 lines to 800 lines for brevity.]');
- expect(userPrompt).toContain('const value799 = 799;');
- expect(userPrompt).not.toContain('const value800 = 800;');
- expect(response.reviewedLineCount).toBe(800);
- expect(response.wasPromptTruncated).toBe(true);
+ // A 900-line file with the configured 800-line cap is split into two chunks (800 + 100)
+ // and each chunk is reviewed in its own model call instead of the tail being truncated away.
+ expect(fetchMock).toHaveBeenCalledTimes(2);
+ for (const body of requestBodies) {
+ expect(body.generationConfig.maxOutputTokens).toBe(8192);
+ }
+ const firstPrompt = requestBodies[0].contents[0].parts[0].text as string;
+ expect(firstPrompt).toContain('const value799 = 799;');
+ expect(firstPrompt).not.toContain('const value800 = 800;');
+ const secondPrompt = requestBodies[1].contents[0].parts[0].text as string;
+ expect(secondPrompt).toContain('const value800 = 800;');
+ expect(secondPrompt).toContain('const value899 = 899;');
+ // The whole file is covered across the chunks, so nothing is dropped as truncated.
+ expect(response.reviewedLineCount).toBe(900);
+ expect(response.wasPromptTruncated).toBe(false);
});
- it('uses a compact Gemma prompt only after a prior transient failure', async () => {
- let requestBody: any = null;
+ it('applies the compact Gemma prompt cap by producing smaller chunks after a prior transient failure', async () => {
+ const requestBodies: any[] = [];
vi.spyOn(globalThis, 'fetch').mockImplementation(async (_url, init) => {
- requestBody = JSON.parse(String(init?.body));
+ requestBodies.push(JSON.parse(String(init?.body)));
return new Response(
JSON.stringify({
candidates: [{ content: { parts: [{ text: '{"findings":[],"overall_correctness":"patch is correct","overall_explanation":"ok","overall_confidence_score":0.9}' }] } }],
@@ -517,11 +724,14 @@ describe('ModelService', () => {
compactPrompt: true,
});
- const userPrompt = requestBody.contents[0].parts[0].text as string;
- expect(userPrompt).toContain('[NOTE: This diff has been truncated from 900 lines to 400 lines for brevity.]');
- expect(userPrompt).toContain('const value399 = 399;');
- expect(userPrompt).not.toContain('const value400 = 400;');
- expect(response.reviewedLineCount).toBe(400);
- expect(response.wasPromptTruncated).toBe(true);
+ // compactPrompt lowers the per-call cap to COMPACT_REVIEW_PROMPT_LINE_CAP (400), so the
+ // 900-line file is split into three chunks (400 + 400 + 100) rather than the two chunks
+ // the full 800-line cap would produce -- the first chunk stops exactly at the compact cap.
+ expect(requestBodies.length).toBe(3);
+ const firstPrompt = requestBodies[0].contents[0].parts[0].text as string;
+ expect(firstPrompt).toContain('const value399 = 399;');
+ expect(firstPrompt).not.toContain('const value400 = 400;');
+ expect(response.reviewedLineCount).toBe(900);
+ expect(response.wasPromptTruncated).toBe(false);
});
});
diff --git a/test/review-flow.spec.ts b/test/review-flow.spec.ts
index 7af87de..1f29928 100644
--- a/test/review-flow.spec.ts
+++ b/test/review-flow.spec.ts
@@ -1,10 +1,10 @@
import { runReviewJob } from '@server/core/review';
import { createTestEnv, generateMockDiff, hasConfiguredTestDatabaseUrl } from './helpers';
import { vi } from 'vitest';
-import { findExistingJobForHead, getJobForProcessing, insertJob, updateJobFileCount, updateJobStep } from '@server/db/jobs';
+import { findExistingJobForHead, getJobForProcessing, getTerminalJobsNeedingCheckRunCompletion, insertJob, updateJobFileCount, updateJobStep } from '@server/db/jobs';
import { getFileReviewsForJobs, upsertFileReview } from '@server/db/file-reviews';
-import { defaultRepoConfig } from '@shared/schema';
-import { runWithDb } from '@server/db/client';
+import { defaultRepoConfig, type ParsedReviewComment } from '@shared/schema';
+import { runWithDb, queryRows } from '@server/db/client';
const sha = (char: string) => char.repeat(40);
@@ -44,6 +44,14 @@ vi.mock('@server/services/github', () => {
vi.mock('@server/services/model', () => {
class MockModelService {
+ // Return null so the review phase uses the synchronous reviewFile path these tests exercise.
+ // (A real request_id here would route through the async batch submit/poll flow instead.)
+ async submitReviewBatch() {
+ return null;
+ }
+ async pollReviewBatch() {
+ return { status: 'pending' as const };
+ }
async reviewFile() {
return {
parsed: {
@@ -102,6 +110,17 @@ dbDescribe('Review Flow Lifecycle', () => {
if (result.action === 'next_phase') {
currentMessage = { ...currentMessage, phase: result.phase };
retries = 0;
+ // Phase/chunk transitions now yield long enough to hibernate into a fresh invocation,
+ // which schedules the next delivery into the future (last_queue_message_at). In-process
+ // we don't actually wait, so backdate it to simulate the delay elapsing -- otherwise the
+ // next claim would report 'busy'.
+ const jobId = (currentMessage as any).jobId;
+ const repo = (currentMessage as any).payload?.repository?.name;
+ if (jobId) {
+ await queryRows(env, `UPDATE jobs SET last_queue_message_at = now() - interval '5 seconds' WHERE id = $1`, [jobId]);
+ } else if (repo) {
+ await queryRows(env, `UPDATE jobs SET last_queue_message_at = now() - interval '5 seconds' WHERE repository_id IN (SELECT id FROM repositories WHERE repo = $1)`, [repo]);
+ }
} else if (result.action === 'retry') {
if (++retries > MAX_RETRIES) throw new Error('Max retries exceeded');
// In test environments, if we get throttled or told to retry, just break to prevent infinite loops.
@@ -203,6 +222,112 @@ dbDescribe('Review Flow Lifecycle', () => {
expect(finalJob?.verdict).toBeNull();
}, REVIEW_FLOW_TIMEOUT_MS);
+ it('throttles a new (queued) job at the concurrency limit but never a running continuation', async () => {
+ const jobsMod = await import('@server/db/jobs');
+ const repo = `test-repo-${Date.now()}-admission`;
+ const baseSha = sha('0');
+ const base = {
+ installationId: '123', owner: 'test-owner', repo, prAuthor: 'author',
+ baseSha, trigger: 'auto' as const, headRef: 'feature', baseRef: 'main',
+ configSnapshot: defaultRepoConfig,
+ };
+
+ const queued = await insertJob(env, { ...base, prNumber: 30, prTitle: 'Admission Queued', commitSha: sha('c') });
+ const running = await insertJob(env, { ...base, prNumber: 31, prTitle: 'Admission Running', commitSha: sha('d') });
+ // Report far over any concurrency limit for the whole test. (The running case never calls this --
+ // the gate is skipped by status -- so restore the module-mock default afterwards to avoid leaking.)
+ vi.mocked(jobsMod.getOtherRunningJobsCount).mockResolvedValue(99);
+ try {
+ // A brand-new (queued) job IS gated at the limit -> retry (admission control).
+ await runWithDb(env, async () => {
+ const res = await runReviewJob(env, { jobId: queued.id, deliveryId: 'delivery-adm-queued', phase: 'prepare' });
+ expect(res.action).toBe('retry');
+ });
+
+ // A job already 'running' must NOT be re-gated on its continuations, even far over the limit --
+ // that is the starvation bug (every in-flight job retries forever and gets lease-recovery-failed).
+ await runWithDb(env, async () => {
+ await queryRows(env, `UPDATE jobs SET status = 'running' WHERE id = $1`, [running.id]);
+ const res = await runReviewJob(env, { jobId: running.id, deliveryId: 'delivery-adm-running', phase: 'review' });
+ expect(res.action).not.toBe('retry');
+ });
+ } finally {
+ vi.mocked(jobsMod.getOtherRunningJobsCount).mockResolvedValue(0);
+ }
+ }, REVIEW_FLOW_TIMEOUT_MS);
+
+ it('bulk-marks missing files failed in a single pass without clobbering existing rows', async () => {
+ const { bulkMarkFilesFailed } = await import('@server/db/file-reviews');
+ const job = await insertJob(env, {
+ installationId: '123', owner: 'test-owner', repo: `test-repo-${Date.now()}-bulk-failed`,
+ prNumber: 40, prTitle: 'Bulk failed', prAuthor: 'author', commitSha: sha('e'), baseSha: sha('0'),
+ trigger: 'auto', headRef: 'feature', baseRef: 'main', configSnapshot: defaultRepoConfig,
+ });
+
+ await bulkMarkFilesFailed(env, job.id, [
+ { filePath: 'src/a.ts', diffLineCount: 10 },
+ { filePath: 'src/b.ts', diffLineCount: 20 },
+ ], { modelUsed: 'gemini-3.1-flash-lite', errorMessage: 'infra limit' });
+
+ // Second call including an existing path must not duplicate or overwrite it (ON CONFLICT DO NOTHING).
+ await bulkMarkFilesFailed(env, job.id, [
+ { filePath: 'src/a.ts', diffLineCount: 10 },
+ { filePath: 'src/c.ts', diffLineCount: 5 },
+ ], { modelUsed: 'other-model', errorMessage: 'second call' });
+
+ const reviews = await getFileReviewsForJobs(env, [job.id]);
+ expect(reviews).toHaveLength(3);
+ expect(reviews.every((r) => r.file_status === 'failed')).toBe(true);
+ // a.ts keeps its first values (not clobbered by the second call).
+ expect(reviews.find((r) => r.file_path === 'src/a.ts')?.error_msg).toBe('infra limit');
+ expect(reviews.find((r) => r.file_path === 'src/c.ts')?.error_msg).toBe('second call');
+ });
+
+ it('completes the job with the review recorded even if post-review check-run/label updates fail', async () => {
+ // Regression: the GitHub review is posted mid-finalize; if the subsequent (cosmetic) check-run
+ // or label calls throw -- e.g. a large PR exhausting the invocation's subrequest budget -- the
+ // job must still finish 'done' with review_id set, not be stranded 'failed' with the review
+ // already live on the PR.
+ const { GitHubService } = await import('@server/services/github');
+ const checkRunSpy = vi.spyOn(GitHubService.prototype, 'updateCheckRun' as any)
+ .mockRejectedValue(new Error('Too many subrequests by single Worker invocation'));
+
+ const job = await insertJob(env, {
+ installationId: '123', owner: 'test-owner', repo: `test-repo-${Date.now()}-besteffort`,
+ prNumber: 41, prTitle: 'Best effort', prAuthor: 'author', commitSha: sha('f'), baseSha: sha('0'),
+ trigger: 'auto', headRef: 'feature', baseRef: 'main', configSnapshot: defaultRepoConfig,
+ });
+
+ await runAndDrain({ jobId: job.id, deliveryId: 'delivery-besteffort' });
+
+ const final = await getJobForProcessing(env, job.id);
+ expect(final?.status).toBe('done');
+ expect(final?.review_id).not.toBeNull();
+ // The check-run update failed, so it must NOT be marked completed -- it stays pending so the
+ // maintenance sweep can finish it (the check run always ends up 'completed', never stuck).
+ expect(final?.check_run_completed_at).toBeNull();
+ const pending = await getTerminalJobsNeedingCheckRunCompletion(env, 500);
+ expect(pending.some((j) => j.id === job.id)).toBe(true);
+ checkRunSpy.mockRestore();
+ }, REVIEW_FLOW_TIMEOUT_MS);
+
+ it('marks the check-run completed on a successful finalize (no maintenance needed)', async () => {
+ const job = await insertJob(env, {
+ installationId: '123', owner: 'test-owner', repo: `test-repo-${Date.now()}-checkrun-ok`,
+ prNumber: 42, prTitle: 'Check run ok', prAuthor: 'author', commitSha: sha('a'), baseSha: sha('0'),
+ trigger: 'auto', headRef: 'feature', baseRef: 'main', configSnapshot: defaultRepoConfig,
+ });
+
+ await runAndDrain({ jobId: job.id, deliveryId: 'delivery-checkrun-ok' });
+
+ const final = await getJobForProcessing(env, job.id);
+ expect(final?.status).toBe('done');
+ // The inline check-run update succeeded, so it's marked complete and won't be re-done by maintenance.
+ expect(final?.check_run_completed_at).not.toBeNull();
+ const pending = await getTerminalJobsNeedingCheckRunCompletion(env, 500);
+ expect(pending.some((j) => j.id === job.id)).toBe(false);
+ }, REVIEW_FLOW_TIMEOUT_MS);
+
it('processes a pre-created retry job from a queue message', async () => {
const repo = `test-repo-${Date.now()}-retry`;
const sourceHeadSha = sha('1');
@@ -330,6 +455,99 @@ dbDescribe('Review Flow Lifecycle', () => {
reviewSpy.mockRestore();
}, REVIEW_FLOW_TIMEOUT_MS);
+ it('inherits a parent review when the config model id is provider-prefixed but the stored model_used is bare', async () => {
+ // Regression: file reviews persist the bare model id (e.g. `gemini-3.1-flash-lite`) while the
+ // configured strategy stores the provider-qualified id (e.g. `google:gemini-3.1-flash-lite`).
+ // Inheritance must match on the bare name; otherwise every retry re-reviews every file.
+ const { ModelService } = await import('@server/services/model');
+ const reviewSpy = vi.spyOn(ModelService.prototype, 'reviewFile');
+ const repo = `test-repo-${Date.now()}-retry-prefix`;
+ const sourceHeadSha = sha('a');
+ const retryHeadSha = sha('b');
+ const baseSha = sha('0');
+
+ const prefixedConfig = {
+ ...defaultRepoConfig,
+ model: {
+ main: 'google:gemini-3.1-flash-lite',
+ fallbacks: ['google:gemini-2.5-flash-lite'],
+ size_overrides: [],
+ },
+ };
+
+ const source = await insertJob(env, {
+ installationId: '123',
+ owner: 'test-owner',
+ repo,
+ prNumber: 7,
+ prTitle: 'Retry Prefix Match',
+ prAuthor: 'author',
+ commitSha: sourceHeadSha,
+ baseSha,
+ trigger: 'auto',
+ headRef: 'feature',
+ baseRef: 'main',
+ configSnapshot: prefixedConfig,
+ });
+
+ await upsertFileReview(env, source.id, {
+ filePath: 'src/app.ts',
+ fileStatus: 'done',
+ modelUsed: 'gemini-3.1-flash-lite', // bare, as the model service actually stores it
+ modelProvider: 'google',
+ diffLineCount: 1,
+ diffInput: 'old diff',
+ rawAiOutput: '{}',
+ parsedComments: [{
+ path: 'src/app.ts',
+ line: 1,
+ position: 1,
+ severity: 'P2',
+ category: 'quality',
+ title: 'Inherited finding',
+ body: 'This comment must survive inheritance',
+ }],
+ inputTokens: 1,
+ outputTokens: 1,
+ durationMs: 1,
+ verdict: 'comment',
+ fileSummary: 'inherited-summary',
+ errorMessage: null,
+ });
+
+ const retry = await insertJob(env, {
+ installationId: '123',
+ owner: 'test-owner',
+ repo,
+ prNumber: 7,
+ prTitle: 'Retry Prefix Match',
+ prAuthor: 'author',
+ commitSha: retryHeadSha,
+ baseSha,
+ trigger: 'retry',
+ headRef: 'feature',
+ baseRef: 'main',
+ configSnapshot: prefixedConfig,
+ retryOfJobId: source.id,
+ });
+
+ await runAndDrain({
+ jobId: retry.id,
+ deliveryId: 'delivery-retry-prefix-match',
+ });
+
+ // The file must be inherited verbatim (bare model id + parent summary preserved), not re-reviewed.
+ expect(reviewSpy).not.toHaveBeenCalled();
+ const reviews = await getFileReviewsForJobs(env, [retry.id]);
+ const inherited = reviews.find((review) => review.file_path === 'src/app.ts');
+ expect(inherited?.model_used).toBe('gemini-3.1-flash-lite');
+ expect(inherited?.file_summary).toBe('inherited-summary');
+ // The parent's comments must be carried over by the bulk-inherit copy, not lost.
+ expect(inherited?.parsed_comments).toHaveLength(1);
+ expect((inherited?.parsed_comments as ParsedReviewComment[])[0]?.title).toBe('Inherited finding');
+ reviewSpy.mockRestore();
+ }, REVIEW_FLOW_TIMEOUT_MS);
+
it('resumes an existing queued duplicate job instead of stranding it', async () => {
const repo = `test-repo-${Date.now()}-duplicate`;
const headSha = sha('4');
@@ -405,7 +623,7 @@ dbDescribe('Review Flow Lifecycle', () => {
phase: 'review',
});
- expect(result).toEqual({ action: 'next_phase', phase: 'review', delaySeconds: 60 });
+ expect(result).toEqual({ action: 'next_phase', phase: 'review', delaySeconds: 30 });
expect(reviewSpy).toHaveBeenCalled();
expect((env.REVIEW_QUEUE as any).sent).toHaveLength(0);
});
@@ -427,7 +645,6 @@ dbDescribe('Review Flow Lifecycle', () => {
generateMockDiff([
{ path: 'src/one.ts', content: 'console.log(1);' },
{ path: 'src/two.ts', content: 'console.log(2);' },
- { path: 'src/three.ts', content: 'console.log(3);' },
]),
);
let active = 0;
@@ -468,7 +685,7 @@ dbDescribe('Review Flow Lifecycle', () => {
baseRef: 'main',
configSnapshot: defaultRepoConfig,
});
- await updateJobFileCount(env, job.id, 3);
+ await updateJobFileCount(env, job.id, 2);
await updateJobStep(env, job.id, 'Preparation', { status: 'done' });
await runWithDb(env, async () => {
@@ -479,13 +696,16 @@ dbDescribe('Review Flow Lifecycle', () => {
phase: 'review',
});
- expect(result).toEqual({ action: 'next_phase', phase: 'finalize', delaySeconds: 0 });
- expect(maxActive).toBe(3);
+ // Finalize always yields long enough to hibernate into a fresh invocation (fresh subrequest
+ // budget), so the delay is the hibernation yield, not 0.
+ expect(result).toEqual({ action: 'next_phase', phase: 'finalize', delaySeconds: expect.any(Number) });
+ expect(result.action === 'next_phase' && result.delaySeconds).toBeGreaterThan(0);
+ expect(maxActive).toBe(2);
expect((env.REVIEW_QUEUE as any).sent).toHaveLength(0);
});
const reviews = await getFileReviewsForJobs(env, [job.id]);
- expect(reviews.filter((review) => review.file_status === 'done')).toHaveLength(3);
+ expect(reviews.filter((review) => review.file_status === 'done')).toHaveLength(2);
reviewSpy.mockRestore();
getDiffSpy.mockRestore();
diff --git a/test/review-resilience.spec.ts b/test/review-resilience.spec.ts
new file mode 100644
index 0000000..37f5c7c
--- /dev/null
+++ b/test/review-resilience.spec.ts
@@ -0,0 +1,132 @@
+import { describe, it, expect, vi, beforeEach } from 'vitest';
+import { getDiffFiles, failJobAndCheckRun } from '@server/core/review';
+import { createTestEnv, generateMockDiff } from './helpers';
+import { defaultRepoConfig } from '@shared/schema';
+
+// Regression coverage for the subrequest-exhaustion incident (job bb9cf692...): a large PR's
+// review workflow re-fetched the PR diff from GitHub on every phase/chunk and, once the
+// Worker's subrequest budget was exhausted, the failure-reporting path silently swallowed
+// errors and could leave a job's DB failure state undone. These tests pin down the fixes for
+// both without requiring a real Postgres connection or live GitHub/Workflow infrastructure.
+
+const { failJobMock, getJobForProcessingMock, markJobCheckRunCompletedMock } = vi.hoisted(() => ({
+ failJobMock: vi.fn(),
+ getJobForProcessingMock: vi.fn(),
+ markJobCheckRunCompletedMock: vi.fn(),
+}));
+
+vi.mock('@server/db/jobs', async (importOriginal) => {
+ const mod = await importOriginal();
+ return {
+ ...mod,
+ failJob: failJobMock,
+ getJobForProcessing: getJobForProcessingMock,
+ markJobCheckRunCompleted: markJobCheckRunCompletedMock,
+ };
+});
+
+describe('getDiffFiles', () => {
+ const baseJob = { owner: 'test-owner', repo: 'test-repo', prNumber: 26 };
+
+ it('fetches the PR diff from GitHub once and reuses the cached copy on later calls for the same job', async () => {
+ const env = createTestEnv();
+ const job = { ...baseJob, id: `diff-cache-hit-${Date.now()}` };
+ const rawDiff = generateMockDiff([{ path: 'src/app.ts', content: 'console.log(1);' }]);
+ const github = { getPullRequestDiff: vi.fn().mockResolvedValue(rawDiff) };
+
+ const first = await getDiffFiles(env, job, github, defaultRepoConfig);
+ const second = await getDiffFiles(env, job, github, defaultRepoConfig);
+ const third = await getDiffFiles(env, job, github, defaultRepoConfig);
+
+ expect(github.getPullRequestDiff).toHaveBeenCalledTimes(1);
+ expect(first.map((f) => f.path)).toEqual(['src/app.ts']);
+ expect(second.map((f) => f.path)).toEqual(['src/app.ts']);
+ expect(third.map((f) => f.path)).toEqual(['src/app.ts']);
+ });
+
+ it('does not share cached diffs across different jobs', async () => {
+ const env = createTestEnv();
+ const jobA = { ...baseJob, id: `diff-cache-job-a-${Date.now()}` };
+ const jobB = { ...baseJob, id: `diff-cache-job-b-${Date.now()}` };
+ const githubA = { getPullRequestDiff: vi.fn().mockResolvedValue(generateMockDiff([{ path: 'src/one.ts', content: 'a' }])) };
+ const githubB = { getPullRequestDiff: vi.fn().mockResolvedValue(generateMockDiff([{ path: 'src/two.ts', content: 'b' }])) };
+
+ const filesA = await getDiffFiles(env, jobA, githubA, defaultRepoConfig);
+ const filesB = await getDiffFiles(env, jobB, githubB, defaultRepoConfig);
+
+ expect(githubA.getPullRequestDiff).toHaveBeenCalledTimes(1);
+ expect(githubB.getPullRequestDiff).toHaveBeenCalledTimes(1);
+ expect(filesA.map((f) => f.path)).toEqual(['src/one.ts']);
+ expect(filesB.map((f) => f.path)).toEqual(['src/two.ts']);
+ });
+
+ it('still returns the parsed files if caching the diff in KV fails', async () => {
+ const env = createTestEnv();
+ (env.APP_KV as any).put = vi.fn().mockRejectedValue(new Error('KV unavailable'));
+ const job = { ...baseJob, id: `diff-cache-put-fail-${Date.now()}` };
+ const github = { getPullRequestDiff: vi.fn().mockResolvedValue(generateMockDiff([{ path: 'src/app.ts', content: 'console.log(1);' }])) };
+
+ const files = await getDiffFiles(env, job, github, defaultRepoConfig);
+
+ expect(files.map((f) => f.path)).toEqual(['src/app.ts']);
+ // The next phase would simply re-fetch from GitHub since the cache write failed; it must
+ // not throw and break the job.
+ });
+});
+
+describe('failJobAndCheckRun', () => {
+ const job = { id: 'job-fail-1', owner: 'test-owner', repo: 'test-repo', checkRunId: 42 };
+
+ beforeEach(() => {
+ failJobMock.mockReset();
+ getJobForProcessingMock.mockReset();
+ markJobCheckRunCompletedMock.mockReset();
+ });
+
+ it('durably records the DB failure even when the GitHub check-run update fails (e.g. subrequest limit exhausted)', async () => {
+ const env = createTestEnv();
+ failJobMock.mockResolvedValue(undefined);
+ getJobForProcessingMock.mockResolvedValue({ check_run_id: job.checkRunId });
+ const updateCheckRun = vi.fn().mockRejectedValue(new Error('Too many subrequests by single Worker invocation.'));
+
+ await expect(failJobAndCheckRun(env, job, { updateCheckRun }, 'boom')).resolves.toBeUndefined();
+
+ // Use expect.anything() rather than the literal env: env's APP_PRIVATE_KEY getter
+ // deliberately throws for unused test secrets, and toHaveBeenCalledWith's deep-equality
+ // check would otherwise trigger it while walking env's own properties.
+ expect(failJobMock).toHaveBeenCalledWith(expect.anything(), job.id, 'boom');
+ expect(updateCheckRun).toHaveBeenCalledTimes(1);
+ // Deliberately not marked complete: completeTerminalCheckRuns() picks this up and
+ // retries the GitHub update later, once a fresh invocation has its own subrequest budget.
+ expect(markJobCheckRunCompletedMock).not.toHaveBeenCalled();
+ });
+
+ it('does not attempt the GitHub call at all if the DB write itself fails', async () => {
+ const env = createTestEnv();
+ failJobMock.mockRejectedValue(new Error('Too many subrequests by single Worker invocation.'));
+ const updateCheckRun = vi.fn();
+
+ await expect(failJobAndCheckRun(env, job, { updateCheckRun }, 'boom')).resolves.toBeUndefined();
+
+ expect(failJobMock).toHaveBeenCalledWith(expect.anything(), job.id, 'boom');
+ expect(getJobForProcessingMock).not.toHaveBeenCalled();
+ expect(updateCheckRun).not.toHaveBeenCalled();
+ });
+
+ it('marks the check run completed once the GitHub update succeeds', async () => {
+ const env = createTestEnv();
+ failJobMock.mockResolvedValue(undefined);
+ getJobForProcessingMock.mockResolvedValue({ check_run_id: job.checkRunId });
+ const updateCheckRun = vi.fn().mockResolvedValue(undefined);
+
+ await failJobAndCheckRun(env, job, { updateCheckRun }, 'boom');
+
+ expect(updateCheckRun).toHaveBeenCalledWith(
+ job.owner,
+ job.repo,
+ job.checkRunId,
+ expect.objectContaining({ status: 'completed', conclusion: 'failure', summary: 'boom' }),
+ );
+ expect(markJobCheckRunCompletedMock).toHaveBeenCalledWith(expect.anything(), job.id);
+ });
+});
diff --git a/test/review-subrequest-completion.spec.ts b/test/review-subrequest-completion.spec.ts
new file mode 100644
index 0000000..2c3ed72
--- /dev/null
+++ b/test/review-subrequest-completion.spec.ts
@@ -0,0 +1,169 @@
+import { describe, it, expect, vi, beforeEach } from 'vitest';
+import { createTestEnv } from './helpers';
+
+// Regression coverage for the subrequest-exhaustion incident (PR #26 / job 9dda151e...): a
+// review phase that hit Cloudflare's per-invocation subrequest cap (Workers Free plan: 50)
+// terminally FAILED the whole job instead of resuming on a fresh budget, so large PRs could
+// never finish. These tests pin down that a per-invocation subrequest-limit error reschedules
+// the same phase (which the workflow re-runs in a fresh invocation with a fresh budget),
+// while an unrelated error still fails the job as before.
+
+const {
+ getJobForProcessingMock,
+ mapJobMock,
+ getOtherRunningJobsCountMock,
+ claimJobLeaseMock,
+ releaseJobLeaseMock,
+ markJobContinuationQueuedMock,
+ updateJobStepMock,
+ failJobMock,
+ markJobCheckRunCompletedMock,
+ getPullRequestMock,
+} = vi.hoisted(() => ({
+ getJobForProcessingMock: vi.fn(),
+ mapJobMock: vi.fn(),
+ getOtherRunningJobsCountMock: vi.fn(),
+ claimJobLeaseMock: vi.fn(),
+ releaseJobLeaseMock: vi.fn(),
+ markJobContinuationQueuedMock: vi.fn(),
+ updateJobStepMock: vi.fn(),
+ failJobMock: vi.fn(),
+ markJobCheckRunCompletedMock: vi.fn(),
+ getPullRequestMock: vi.fn(),
+}));
+
+vi.mock('@server/db/jobs', async (importOriginal) => {
+ const mod = await importOriginal();
+ return {
+ ...mod,
+ getJobForProcessing: getJobForProcessingMock,
+ mapJob: mapJobMock,
+ getOtherRunningJobsCount: getOtherRunningJobsCountMock,
+ claimJobLease: claimJobLeaseMock,
+ releaseJobLease: releaseJobLeaseMock,
+ markJobContinuationQueued: markJobContinuationQueuedMock,
+ updateJobStep: updateJobStepMock,
+ failJob: failJobMock,
+ markJobCheckRunCompleted: markJobCheckRunCompletedMock,
+ };
+});
+
+vi.mock('@server/db/app-settings', async (importOriginal) => {
+ const mod = await importOriginal();
+ return {
+ ...mod,
+ getReviewSettings: vi.fn().mockResolvedValue({ concurrencyLevel: 'low', maxComments: 20 }),
+ };
+});
+
+vi.mock('@server/services/github', () => ({
+ GitHubService: class {
+ getPullRequest = getPullRequestMock;
+ updateCheckRun = vi.fn().mockResolvedValue(undefined);
+ },
+}));
+
+// Imported after the mocks are registered.
+import { runReviewJob } from '@server/core/review';
+
+const JOB_ID = '9dda151e-0c61-4205-9cba-497027706698';
+
+const reviewJob = {
+ id: JOB_ID,
+ installationId: '123',
+ owner: 'test-owner',
+ repo: 'test-repo',
+ prNumber: 26,
+ checkRunId: null as number | null,
+ retryOfJobId: null as string | null,
+ trigger: 'auto' as const,
+ createdAt: new Date().toISOString(),
+ // Preparation already done so runReviewPhase proceeds to the review work (and thus to the
+ // getPullRequest call we make throw) instead of falling back into runPreparePhase.
+ steps: [{ name: 'Preparation', status: 'done' }],
+ configSnapshot: undefined,
+};
+
+describe('runReviewJob subrequest-budget handling', () => {
+ beforeEach(() => {
+ vi.clearAllMocks();
+ // A claimable, non-terminal job every time.
+ getJobForProcessingMock.mockResolvedValue({ check_run_id: null });
+ mapJobMock.mockReturnValue(reviewJob);
+ getOtherRunningJobsCountMock.mockResolvedValue(0);
+ claimJobLeaseMock.mockResolvedValue({ status: 'claimed', row: {} });
+ releaseJobLeaseMock.mockResolvedValue(undefined);
+ markJobContinuationQueuedMock.mockResolvedValue(undefined);
+ updateJobStepMock.mockResolvedValue(undefined);
+ failJobMock.mockResolvedValue(undefined);
+ });
+
+ it('reschedules the same phase (fresh budget) instead of failing the job when it hits the per-invocation subrequest limit', async () => {
+ const env = createTestEnv();
+ getPullRequestMock.mockRejectedValue(new Error('Too many subrequests by single Worker invocation.'));
+
+ const result = await runReviewJob(env, { jobId: JOB_ID, phase: 'review' } as any);
+
+ expect(result).toEqual({ action: 'next_phase', phase: 'review', delaySeconds: expect.any(Number) });
+ // Queued for continuation and the lease released so the fresh invocation can re-claim it...
+ expect(markJobContinuationQueuedMock).toHaveBeenCalledTimes(1);
+ expect(releaseJobLeaseMock).toHaveBeenCalledTimes(1);
+ // ...and crucially the job was NOT marked failed.
+ expect(failJobMock).not.toHaveBeenCalled();
+ });
+
+ it('still fails the job terminally for an unrelated (non-subrequest, non-retryable) error', async () => {
+ const env = createTestEnv();
+ getPullRequestMock.mockRejectedValue(new Error('totally unexpected boom'));
+
+ const result = await runReviewJob(env, { jobId: JOB_ID, phase: 'review' } as any);
+
+ expect(result).toEqual({ action: 'ack' });
+ expect(failJobMock).toHaveBeenCalledWith(expect.anything(), JOB_ID, 'totally unexpected boom');
+ expect(markJobContinuationQueuedMock).not.toHaveBeenCalled();
+ });
+
+ it('keeps rescheduling while the continuation count is still under the ceiling', async () => {
+ const env = createTestEnv();
+ // markJobContinuationQueued returns the post-increment count; a value at/under the ceiling
+ // (MAX_JOB_CONTINUATIONS = 20) must still reschedule rather than give up.
+ markJobContinuationQueuedMock.mockResolvedValue(20);
+ getPullRequestMock.mockRejectedValue(new Error('Too many subrequests by single Worker invocation.'));
+
+ const result = await runReviewJob(env, { jobId: JOB_ID, phase: 'review' } as any);
+
+ expect(result).toEqual({ action: 'next_phase', phase: 'review', delaySeconds: expect.any(Number) });
+ expect(failJobMock).not.toHaveBeenCalled();
+ });
+
+ it('degrades a wedged review phase to a partial review (finalize) once it exceeds the continuation ceiling', async () => {
+ const env = createTestEnv();
+ // Post-increment count above MAX_JOB_CONTINUATIONS (20): the review phase is wedged, so
+ // rather than discarding all completed file reviews it must hand off to finalize and post a
+ // partial review. It must NOT fail the job terminally, and must return (not throw) that
+ // transition -- enqueueJobPhase would throw NextPhaseError and escape the catch block.
+ markJobContinuationQueuedMock.mockResolvedValue(21);
+ getPullRequestMock.mockRejectedValue(new Error('Too many subrequests by single Worker invocation.'));
+
+ const result = await runReviewJob(env, { jobId: JOB_ID, phase: 'review' } as any);
+
+ expect(result).toEqual({ action: 'next_phase', phase: 'finalize', delaySeconds: expect.any(Number) });
+ expect(failJobMock).not.toHaveBeenCalled();
+ expect(releaseJobLeaseMock).toHaveBeenCalled();
+ });
+
+ it('fails a non-review phase terminally once it exceeds the continuation ceiling without making progress', async () => {
+ const env = createTestEnv();
+ // The graceful degrade-to-finalize path only applies to the review phase; a wedged prepare
+ // phase has no partial result to salvage, so it must still fail terminally instead of churning.
+ markJobContinuationQueuedMock.mockResolvedValue(21);
+ getPullRequestMock.mockRejectedValue(new Error('Too many subrequests by single Worker invocation.'));
+
+ const result = await runReviewJob(env, { jobId: JOB_ID, phase: 'prepare' } as any);
+
+ expect(result).toEqual({ action: 'ack' });
+ expect(failJobMock).toHaveBeenCalledTimes(1);
+ expect(failJobMock.mock.calls[0][2]).toMatch(/could not make progress after 21 continuation attempts/);
+ expect(releaseJobLeaseMock).toHaveBeenCalled();
+ });
+});
diff --git a/test/scheduled-maintenance.spec.ts b/test/scheduled-maintenance.spec.ts
new file mode 100644
index 0000000..c18b3be
--- /dev/null
+++ b/test/scheduled-maintenance.spec.ts
@@ -0,0 +1,63 @@
+import { describe, it, expect, vi, beforeEach } from 'vitest';
+import { createTestEnv } from './helpers';
+
+// The cron must keep the serverless Postgres asleep when idle: it only opens a DB connection when
+// the `system:active_jobs` KV flag is set, and it clears that flag as soon as there is no pending
+// maintenance work so the next tick skips the DB entirely.
+
+const { runBestEffortJobMaintenanceMock, hasPendingMaintenanceWorkMock } = vi.hoisted(() => ({
+ runBestEffortJobMaintenanceMock: vi.fn().mockResolvedValue(undefined),
+ hasPendingMaintenanceWorkMock: vi.fn(),
+}));
+
+vi.mock('@server/core/job-recovery', async (importOriginal) => ({
+ ...(await importOriginal()),
+ runBestEffortJobMaintenance: runBestEffortJobMaintenanceMock,
+}));
+
+vi.mock('@server/db/jobs', async (importOriginal) => ({
+ ...(await importOriginal()),
+ hasPendingMaintenanceWork: hasPendingMaintenanceWorkMock,
+}));
+
+import worker from '@server/index';
+
+const controller = {} as ScheduledController;
+const ctx = { waitUntil: () => {}, passThroughOnException: () => {} } as unknown as ExecutionContext;
+
+describe('scheduled() cron maintenance gating', () => {
+ beforeEach(() => {
+ vi.clearAllMocks();
+ });
+
+ it('skips all DB work (no maintenance) when the active-jobs flag is absent', async () => {
+ const env = createTestEnv();
+ await worker.scheduled(controller, env, ctx);
+ expect(runBestEffortJobMaintenanceMock).not.toHaveBeenCalled();
+ expect(hasPendingMaintenanceWorkMock).not.toHaveBeenCalled();
+ });
+
+ it('runs maintenance and clears the flag when no pending work remains', async () => {
+ const env = createTestEnv();
+ await env.APP_KV.put('system:active_jobs', '1');
+ hasPendingMaintenanceWorkMock.mockResolvedValue(false);
+
+ await worker.scheduled(controller, env, ctx);
+
+ expect(runBestEffortJobMaintenanceMock).toHaveBeenCalledTimes(1);
+ // Flag cleared -> the next tick will early-return without touching Postgres.
+ expect(await env.APP_KV.get('system:active_jobs')).toBeNull();
+ });
+
+ it('runs maintenance but keeps the flag while work is still pending', async () => {
+ const env = createTestEnv();
+ await env.APP_KV.put('system:active_jobs', '1');
+ hasPendingMaintenanceWorkMock.mockResolvedValue(true);
+
+ await worker.scheduled(controller, env, ctx);
+
+ expect(runBestEffortJobMaintenanceMock).toHaveBeenCalledTimes(1);
+ // Flag retained -> the cron keeps maintaining the still-active job(s).
+ expect(await env.APP_KV.get('system:active_jobs')).toBe('1');
+ });
+});
diff --git a/test/setup.ts b/test/setup.ts
index e7a505b..eda9c3b 100644
--- a/test/setup.ts
+++ b/test/setup.ts
@@ -103,6 +103,14 @@ if (typeof window !== 'undefined' && !window.matchMedia) {
});
}
+if (typeof window !== 'undefined' && !window.ResizeObserver) {
+ window.ResizeObserver = class ResizeObserver {
+ observe() {}
+ unobserve() {}
+ disconnect() {}
+ };
+}
+
const originalConsoleWarn = console.warn;
console.warn = (...args: any[]) => {
if (typeof args[0] === 'string' && args[0].includes('The width(-1) and height(-1) of chart should be greater than 0')) {
diff --git a/test/token-tracker.spec.ts b/test/token-tracker.spec.ts
new file mode 100644
index 0000000..852e25b
--- /dev/null
+++ b/test/token-tracker.spec.ts
@@ -0,0 +1,47 @@
+import { describe, it, expect } from 'vitest';
+import { TokenTracker } from '@server/core/token-tracker';
+
+// Regression coverage for the subrequest-exhaustion incident (job bb9cf692...): the review
+// workflow could burn through Cloudflare's per-invocation subrequest cap (Workers Free plan:
+// 50 subrequests/invocation) without ever checking how much budget was left. TokenTracker
+// already tracked a running count but exposed no way to ask "how much is safely left", so
+// nothing consulted it before starting more concurrent work. These tests pin down the new
+// remainingSafeBudget() accessor that review.ts and model.ts now use to throttle themselves.
+
+describe('TokenTracker.remainingSafeBudget', () => {
+ it('starts with the full margin below the hard cap available', () => {
+ const tracker = new TokenTracker();
+ // MAX_SUBREQUESTS (50) - SAFE_MARGIN (25) = 25, with nothing spent yet.
+ expect(tracker.remainingSafeBudget()).toBe(25);
+ });
+
+ it('shrinks by exactly what has been spent so far', () => {
+ const tracker = new TokenTracker();
+ tracker.incrementSubrequests(10);
+ expect(tracker.remainingSafeBudget()).toBe(15);
+
+ tracker.incrementSubrequests(5);
+ expect(tracker.remainingSafeBudget()).toBe(10);
+ });
+
+ it('never goes negative once spending exceeds the safe margin', () => {
+ const tracker = new TokenTracker();
+ tracker.incrementSubrequests(45);
+ expect(tracker.remainingSafeBudget()).toBe(0);
+
+ tracker.incrementSubrequests(100);
+ expect(tracker.remainingSafeBudget()).toBe(0);
+ });
+
+ it('agrees with isNearLimit at the same threshold', () => {
+ const tracker = new TokenTracker();
+ // Near-limit / zero-safe-budget threshold is MAX_SUBREQUESTS (50) - SAFE_MARGIN (25) = 25.
+ tracker.incrementSubrequests(24);
+ expect(tracker.isNearLimit()).toBe(false);
+ expect(tracker.remainingSafeBudget()).toBeGreaterThan(0);
+
+ tracker.incrementSubrequests(1);
+ expect(tracker.isNearLimit()).toBe(true);
+ expect(tracker.remainingSafeBudget()).toBe(0);
+ });
+});
diff --git a/wrangler.jsonc b/wrangler.jsonc
index 77def02..1de962c 100644
--- a/wrangler.jsonc
+++ b/wrangler.jsonc
@@ -66,6 +66,11 @@
"class_name": "ReviewWorkflow"
}
],
+ "triggers": {
+ "crons": [
+ "*/2 * * * *"
+ ]
+ },
"assets": {
"directory": "./dist/client",
"binding": "ASSETS",