Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions apps/loopover-ui/src/lib/selfhost-env-reference.ts
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,10 @@ export const SELFHOST_ENV_REFERENCE_ROWS: SelfHostEnvReferenceRow[] = [
name: "AI_DAILY_NEURON_BUDGET",
firstReference: "src/services/ai-review.ts",
},
{
name: "AI_DAILY_REPO_CALL_LIMIT",
firstReference: "src/services/ai-review.ts",
},
{
name: "AI_DUAL_REVIEW",
firstReference: "src/selfhost/ai.ts",
Expand Down Expand Up @@ -638,6 +642,7 @@ export const SELFHOST_ENV_REFERENCE_MARKDOWN = [
"| `AI_BYOK_DAILY_REPO_LIMIT` | `src/services/ai-review.ts` |",
"| `AI_COMBINE` | `src/selfhost/ai.ts` |",
"| `AI_DAILY_NEURON_BUDGET` | `src/services/ai-review.ts` |",
"| `AI_DAILY_REPO_CALL_LIMIT` | `src/services/ai-review.ts` |",
"| `AI_DUAL_REVIEW` | `src/selfhost/ai.ts` |",
"| `AI_EMBED_API_KEY` | `src/server.ts` |",
"| `AI_EMBED_BASE_URL` | `src/selfhost/ai.ts` |",
Expand Down
16 changes: 15 additions & 1 deletion src/db/repositories.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3977,7 +3977,21 @@ const BYOK_SPEND_ATTEMPT_STATUSES = ["ok", "error"] as const;
* count; excluding attempted-but-failed calls would turn a flaky or misconfigured provider into a way to
* bypass this cap entirely via forced failures. See BYOK_SPEND_ATTEMPT_STATUSES for why this is an allowlist.
*/
/** #9061: count ALL AI spend attempts for a repo since `sinceIso`, BYOK or not. Its sibling
* countByokAiEventsForRepoSince is filtered to `byok:%` models, which is why the per-repo ceiling it powers has
* only ever bound the BYOK path -- on the self-host, where reviews run on the free/default chain, one runaway
* repo could consume the entire instance-wide allowance with no per-repo limit anywhere. */
export async function countAiEventsForRepoSince(env: Env, repoFullName: string, sinceIso: string): Promise<number> {
return countRepoAiEventsSince(env, repoFullName, sinceIso, false);
}

export async function countByokAiEventsForRepoSince(env: Env, repoFullName: string, sinceIso: string): Promise<number> {
return countRepoAiEventsSince(env, repoFullName, sinceIso, true);
}

/** Shared body for the two per-repo AI-spend counters above — identical apart from the `byok:%` model filter,
* which is exactly the difference that left the free/default chain with no per-repo ceiling (#9061). */
async function countRepoAiEventsSince(env: Env, repoFullName: string, sinceIso: string, byokOnly: boolean): Promise<number> {
const db = getDb(env.DB);
const [row] = await db
.select({ total: sql<number>`count(*)` })
Expand All @@ -3986,7 +4000,7 @@ export async function countByokAiEventsForRepoSince(env: Env, repoFullName: stri
and(
gte(aiUsageEvents.createdAt, sinceIso),
inArray(aiUsageEvents.status, BYOK_SPEND_ATTEMPT_STATUSES),
sql`${aiUsageEvents.model} like 'byok:%'`,
...(byokOnly ? [sql`${aiUsageEvents.model} like 'byok:%'`] : []),
sql`json_extract(${aiUsageEvents.metadataJson}, '$.repoFullName') = ${repoFullName}`,
),
);
Expand Down
4 changes: 4 additions & 0 deletions src/env.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,10 @@ declare global {
AI_DAILY_NEURON_BUDGET?: string;
/** Per-repository/day cap for maintainer-paid BYOK AI review provider calls. */
AI_BYOK_DAILY_REPO_LIMIT?: string;
/** #9061: per-repository/day cap on AI calls for the FREE/default chain — the path the self-host actually
* runs on, which had no per-repo ceiling at all, so one runaway repo could drain the whole instance-wide
* allowance. Unset ⇒ DEFAULT_DAILY_REPO_AI_CALL_LIMIT; "0" disables the per-repo ceiling. */
AI_DAILY_REPO_CALL_LIMIT?: string;
AI_MAX_OUTPUT_TOKENS?: string;
/** Optional Cloudflare AI Gateway id for legacy env.AI-compatible adapters. Self-host review execution should
* prefer provider-specific AI_* configuration instead. */
Expand Down
93 changes: 91 additions & 2 deletions src/queue/ai-review-orchestration.ts
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,8 @@ import {
runLoopOverAiReview,
type ImprovementMagnitude,
type InlineFinding,
isAiDailyBudgetExhausted,
isRepoDailyAiLimitReached,
} from "../services/ai-review";
import { shouldRenderFindingCategories, shouldRequestInlineFindings } from "../review/inline-comments";
import { buildReviewGroundingText, isGroundingEnabled } from "../review/grounding-wire";
Expand Down Expand Up @@ -173,6 +175,63 @@ export function aiReviewLockContendedResult(
};
}

/**
* #9060 — the shape a pass returns when the AI review ATTEMPT failed, so the failure itself gets a cooldown.
*
* Both failure exits used to return `undefined`, and the caller only writes a cache row for a defined result.
* So a pass that threw inside the try (a DB hiccup, a GitHub 5xx during grounding, an enrichment timeout) or
* came back non-ok wrote NOTHING — and the next tick, two minutes later, missed the cache and re-executed the
* entire prologue: list files, fetch up to 96k characters of file content for grounding, RAG embeddings,
* impact-map embeddings, culture profile, the external enrichment POST, then the model call. Forever. That is
* the exact shape and cadence of the 259-calls-in-24h incident; the #regate-churn fix bounded re-spend for
* DISPUTED verdicts and never covered failures.
*
* `cacheable: false, persistable: true` is the whole point, and the pair means something specific here. Not
* cacheable: this is not a verdict and must never be served as one, so the PR is re-reviewed properly on the
* next attempt. Persistable: the row still gets WRITTEN, so the non-cacheable retry cooldown applies and the
* expensive prologue runs once per cooldown window instead of once per tick. That is the exact opposite of
* {@link aiReviewLockContendedResult}, which is `persistable: false` because a concurrent pass is about to
* write the real result within seconds — here nothing else is coming, which is precisely why the cooldown must.
*
* The finding is the same inconclusive hold every other unresolvable-review path produces, so a repo requiring
* blocking AI review holds for a human rather than passing on deterministic checks alone.
*/
/** Review statuses that mean AI review was never going to produce anything here — the operator switched it off,
* or no provider is bound. A configuration state, not a failed attempt (#9060): these keep returning
* `undefined` so nothing is recorded and no PR is held for a review that was never expected to run. */
const AI_NOT_CONFIGURED_STATUSES: ReadonlySet<string> = new Set(["disabled", "unavailable"]);

export function aiReviewAttemptFailedResult(
advisory: Pick<Awaited<ReturnType<typeof buildPullRequestAdvisory>>, "findings">,
reason: string,
// NonNullable: this helper never returns undefined, and saying so lets callers read the fields without a
// narrowing dance. The declared union on runAiReviewForAdvisory itself is what carries the absent case.
): NonNullable<Awaited<ReturnType<typeof runAiReviewForAdvisory>>> {
const findings: AdvisoryFinding[] = [
{
code: "ai_review_inconclusive",
severity: "warning",
title: "AI review could not complete for this PR head",
detail: `The AI review attempt did not produce a result (${reason}). The gate is held for a human rather than passed automatically.`,
action: "The review is retried automatically after a short cooldown; a maintainer can review manually in the meantime.",
},
];
advisory.findings.push(...findings);
return {
// Empty on purpose. There IS no public review text -- the attempt did not produce one -- and the downstream
// "required AI review produced no public summary" audit keys on exactly that emptiness. Putting a
// human-readable apology here would read as a real assessment to that check and silently suppress the
// audit + Sentry signal an operator needs. The hold reaches the contributor through the finding below.
notes: "",
reviewerCount: 0,
inlineFindings: [],
findings,
// Not a verdict -- never served as one. But WRITTEN, so the retry cooldown bounds the prologue re-spend.
cacheable: false,
persistable: true,
};
}

export async function shouldStartAiReviewForAdvisory(
env: Env,
args: {
Expand Down Expand Up @@ -497,6 +556,28 @@ export async function runAiReviewForAdvisory(
})))
)
return undefined;
// #9060 / #9061: the spend ceilings, checked before any spend -- before listing files, before grounding fetches up to
// 96k characters of file content from GitHub, before RAG and impact-map embeddings, before the culture
// profile, before the external enrichment POST. The full budget gate inside runLoopOverAiReview needs this
// call's estimated cost, which needs the assembled prompt, so it necessarily sits AFTER all of that: on an
// exhausted budget every tick paid for the whole prologue and then declined to make the one call the prologue
// existed to support. And because embeddings are booked at zero estimated neurons, that spend never moved the
// counter either, so the ceiling could not converge and the loop never self-limited.
//
// Placed AFTER the "is AI review even supposed to run here" short-circuits above (paused, mode off,
// unreviewable author, reputation skip) and BEFORE the lock claim and the prologue: a repo that was never
// going to spend anything must not pay two ledger reads to find that out, and must keep returning `undefined`
// rather than a held-for-review finding.
//
// Neither pre-check needs the prompt, so both can run here. Returning the cooldown-bearing failure result
// (rather than undefined) means the exhausted state is itself recorded, so the next tick is a cache hit
// instead of another full prologue.
if (await isAiDailyBudgetExhausted(env)) {
return aiReviewAttemptFailedResult(args.advisory, "the daily AI budget is exhausted");
}
if (await isRepoDailyAiLimitReached(env, args.repoFullName)) {
return aiReviewAttemptFailedResult(args.advisory, "this repository reached its daily AI-call limit");
}
// Per-(repo, PR, head SHA, mode) advisory lock (#confirmed-bug, mirrors #2129/#2368's claimPrActuationLock):
// a webhook pass and an agent-regate-pr sweep pass can independently reach this point for the SAME PR at the
// SAME head, both miss the cache (neither has written yet), and both fire a real, wasteful LLM call that can
Expand Down Expand Up @@ -734,7 +815,13 @@ export async function runAiReviewForAdvisory(
// the caller resolved the feature on for this repo. Absent/false ⇒ byte-identical prompt.
improvementSignal: args.improvementSignal === true,
});
if (result.status !== "ok") return undefined;
// #9060: a cooldown-bearing failure row, not `undefined` -- see aiReviewAttemptFailedResult. But only for a
// genuine FAILURE. `disabled` and `unavailable` mean AI review was never going to produce anything here
// (the operator switched it off, or no provider is bound), which is a configuration state, not a failed
// attempt: it must keep returning `undefined` so nothing is recorded and no PR is held for a review that
// was never expected to run. `quota_exceeded` is the one that matters -- that is the runaway loop's exit.
if (AI_NOT_CONFIGURED_STATUSES.has(result.status)) return undefined;
if (result.status !== "ok") return aiReviewAttemptFailedResult(args.advisory, `status=${result.status}`);
// #8229 stage 0: persist each reviewer's stance for the provider track records — best-effort like every
// calibration write (a vote-store failure must never affect the review), one audit event per reviewer,
// attribution already swap-proof from the runner (votes attach at leg production time).
Expand Down Expand Up @@ -947,7 +1034,9 @@ export async function runAiReviewForAdvisory(
pr: args.pr.number,
head_sha: args.advisory.headSha,
}, "ai_review_failed");
return undefined;
// #9060: same cooldown as the non-ok exit above. A crash inside the try is exactly the case that used to
// re-run the whole expensive prologue every two minutes with nothing recording that it had already failed.
return aiReviewAttemptFailedResult(args.advisory, "the review attempt threw");
} finally {
// #regate-dup-prep: only release a lock THIS call actually claimed. A caller-supplied
// preAcquiredAiReviewLock must keep covering the caller's own post-return work (e.g. persisting the fresh
Expand Down
43 changes: 41 additions & 2 deletions src/review/adapters.ts
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,30 @@ export function reviewVectorAdapter(vectorize: Vectorize): VectorAdapter {
// returns real `usage` (provider/model/tokens) for embeddings, so this is recorded for free via
// `coerceAiUsage`; Workers AI's binding has no such `usage` field, so the call is still recorded (feature +
// the model actually requested), just without token/cost detail. ──
/**
* #9060 — a real, non-zero cost estimate for an embedding call.
*
* Every embedding was booked at `estimatedNeurons: 0`, which made RAG and impact-map spend invisible to the
* daily budget governor. That is not merely under-reporting: it is why the ceiling could never converge. The
* governor's counter never moved for embedding work, so a review loop re-running the prologue every two
* minutes accumulated real provider spend while the budget it was supposed to be bounded by stayed flat.
*
* Deliberately a coarse heuristic, matching how `estimateNeurons` already treats chat calls: the unit is a
* Workers-AI holdover applied provider-agnostically, so precision here would be false precision. What matters
* is that repeated embedding work moves the counter at all, so the ceiling can actually bind.
*/
export function estimateEmbeddingNeurons(options: unknown): number {
const text = (options as { text?: unknown } | null | undefined)?.text;
const chars = Array.isArray(text)
? text.reduce<number>((sum, entry) => sum + (typeof entry === "string" ? entry.length : 0), 0)
: typeof text === "string"
? text.length
: 0;
// ~4 chars per token, and a floor of 1 so a call is never free -- a batch of empty strings is still a
// round trip, and "free" is exactly the accounting that let this spend hide.
return Math.max(1, Math.ceil(chars / 4 / 100));
}

export function reviewInferenceAdapter(env: Env, ai: Ai): InferenceAdapter {
const runner = ai as unknown as { run(m: string, o: Record<string, unknown>): Promise<unknown> };
return {
Expand All @@ -77,7 +101,7 @@ export function reviewInferenceAdapter(env: Env, ai: Ai): InferenceAdapter {
provider: usage?.provider,
effort: usage?.effort,
status: "ok",
estimatedNeurons: 0,
estimatedNeurons: estimateEmbeddingNeurons(options),
inputTokens: usage?.inputTokens,
outputTokens: usage?.outputTokens,
totalTokens: usage?.totalTokens,
Expand All @@ -90,7 +114,8 @@ export function reviewInferenceAdapter(env: Env, ai: Ai): InferenceAdapter {
route: "review.embeddings",
model,
status: "error",
estimatedNeurons: 0,
// A failed embedding still cost a round trip; booking it at zero is how a retry loop stays invisible.
estimatedNeurons: estimateEmbeddingNeurons(options),
detail: error instanceof Error ? error.message : "embedding_failed",
});
throw error;
Expand All @@ -114,7 +139,21 @@ export function createReviewAdapters(env: Env): RagInfra {
if (env.VECTORIZE) infra.vector = reviewVectorAdapter(env.VECTORIZE);
// Embeddings use the DEDICATED embed provider (env.AI_EMBED) when configured — keeping the review chat chain
// frontier-only — and fall back to env.AI otherwise (byte-identical to before).
// #9061: falling back to env.AI routes embeddings onto the FRONTIER review chain. createChainAi rejects
// embeds for CLI providers, but an openai-compatible or anthropic link serves them at frontier pricing --
// booked, until #9060, at zero. The fallback is kept (removing it would silently disable RAG for every
// existing deployment that relies on it) but it is now loud: an operator running RAG without a dedicated
// embed provider should know they are paying frontier rates for embeddings.
const embedAi = env.AI_EMBED ?? env.AI;
if (!env.AI_EMBED && env.AI) {
console.warn(
JSON.stringify({
level: "warn",
event: "review_embeddings_using_review_chain",
message: "AI_EMBED is not configured; embeddings route onto the review chain and may bill at frontier rates",
}),
);
}
if (embedAi) infra.inference = reviewInferenceAdapter(env, embedAi);
return infra;
}
20 changes: 18 additions & 2 deletions src/review/public-stats.ts
Original file line number Diff line number Diff line change
Expand Up @@ -281,7 +281,8 @@ export const PUBLISHED_PR_KEYS = `
CAST(substr(target_key, instr(target_key, '#') + 1) AS INTEGER) AS number,
created_at
FROM audit_events
WHERE event_type = 'github_app.pr_public_surface_published' AND instr(target_key, '#') > 0`;
WHERE event_type = 'github_app.pr_public_surface_published' AND instr(target_key, '#') > 0
AND length(target_key) - length(replace(target_key, '#', '')) = 1`;

/** Assemble the public-safe payload from the LIVE review ledger: distinct PRs the bot published a review for
* (audit_events) joined to their terminal disposition (pull_requests state). Realtime behind the 60s HTTP cache
Expand Down Expand Up @@ -330,6 +331,7 @@ export async function getPublicStats(
FROM audit_events
WHERE event_type IN ('reversal_reopened', 'reversal_reverted', 'reversal_superseded')
AND outcome = 'completed' AND instr(target_key, '#') > 0
AND length(target_key) - length(replace(target_key, '#', '')) = 1
) ev
WHERE LOWER(ev.project) IN (${inList})
GROUP BY project`,
Expand All @@ -353,6 +355,19 @@ export async function getPublicStats(
),
// review-effort minutes (#1955/#2070): sum each distinct published PR's persisted estimate, using
// MINUTES_SAVED_PER_PR only for PRs whose metadata lacks reviewEffortMinutes (mixed-rollout safe).
// #9084: two dialect hazards this SQL used to walk straight into on the Postgres self-host, both silent.
//
// json_extract translates to `->>`, which yields TEXT, so the enclosing AVG resolved to `avg(text)` — a
// function Postgres does not have. The error was swallowed by the fail-safe read wrapper, so the published
// "review effort / minutes saved" number was permanently zero and nothing said so. CAST(... AS REAL) is
// valid in both dialects; NULLIF guards the empty string, which Postgres would otherwise reject outright.
//
// And target_key is not uniformly two-segment: regateRepairTargetKey mints `repo#pr#headSha`. On SQLite the
// INTEGER cast of `pr#sha` is lenient garbage; on Postgres it aborts the WHOLE query, so a single
// three-segment row among the filtered event types took the entire public-stats read to [] and the homepage
// counters silently to zero. Excluding those keys before the cast keeps one row from erasing every number.
// The separator count is written as length()-length(replace()) rather than a nested instr(): `length` and
// `replace` mean the same thing in both dialects and need no translation at all.
safeAll<{ totalMinutes: number | null }>(
env,
`SELECT SUM(COALESCE(minutes, ?)) AS totalMinutes
Expand All @@ -361,11 +376,12 @@ export async function getPublicStats(
FROM (
SELECT LOWER(substr(target_key, 1, instr(target_key, '#') - 1)) AS repo,
CAST(substr(target_key, instr(target_key, '#') + 1) AS INTEGER) AS number,
json_extract(metadata_json, '$.reviewEffortMinutes') AS minutes
CAST(NULLIF(json_extract(metadata_json, '$.reviewEffortMinutes'), '') AS REAL) AS minutes
FROM audit_events
WHERE event_type = 'github_app.pr_public_surface_published'
AND LOWER(substr(target_key, 1, instr(target_key, '#') - 1)) IN (${inList})
AND instr(target_key, '#') > 0
AND length(target_key) - length(replace(target_key, '#', '')) = 1
)
GROUP BY repo, number
)`,
Expand Down
Loading
Loading