From 1108e82e98a10547fe3cb22cb2d467488c436a2f Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Sun, 26 Jul 2026 21:12:28 -0700 Subject: [PATCH] fix(cost,db): bound runaway AI spend, and stop reporting zero for a metric that has been broken since it shipped MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #9060 — a failed AI review cached nothing, so the whole expensive prologue re-ran every two minutes, forever. Both failure exits returned `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 recorded nothing — and the next tick missed the cache and re-executed everything: list files, up to 96k characters of file content fetched for grounding, RAG embeddings, impact-map embeddings, culture profile, the external enrichment POST, then the model call. That is the shape and cadence of the known 259-calls-in-24h incident; the #regate-churn fix bounded re-spend for DISPUTED verdicts and never covered failures. Failures now return a result that is `cacheable: false, persistable: true`, and the pair means something specific: not a verdict, so the PR is reviewed properly next time — but the row is WRITTEN, so the retry cooldown bounds the prologue. That is the exact inverse of the lock-contention placeholder, which is persistable:false precisely because a concurrent pass writes the real result within seconds. Here nothing else is coming, which is why the cooldown must. `disabled` and `unavailable` are deliberately excluded: the operator switched AI review off, or no provider is bound. Those are configuration states, not failed attempts, and must keep returning undefined rather than holding PRs for a review nobody configured. Two compounding parts. The budget ceiling sat AFTER the spend it bounds — it needs this call's estimated cost, which needs the assembled prompt — so on an exhausted budget every tick paid for the entire prologue and then declined to make the one call the prologue existed to support. A cheap "is the budget already spent" pre-check now runs before any of it. And every embedding was booked at `estimatedNeurons: 0`, so RAG and impact-map spend never moved the governor's counter: the ceiling could not converge and the loop never self-limited. Embeddings are now charged a real estimate, with a floor of 1 — a round trip is never free, and free is exactly the accounting that let this hide. #9061 — no per-repo AI ceiling on the path we actually run. A per-repo daily limit existed only for BYOK. On the self-host, where reviews run on the free/default chain, one runaway repo could consume the entire instance-wide allowance with nothing stopping it. Added as AI_DAILY_REPO_CALL_LIMIT, mirroring the BYOK ceiling's shape; the two counters now share one implementation, since the `byok:%` model filter was their only difference and was the whole gap. Also: with AI_EMBED unset, embeddings route onto the frontier review chain and bill at frontier rates. The fallback stays (removing it would silently disable RAG for existing deployments) but it now warns, so an operator knows. #9084 — the review-effort metric has been silently zero on Postgres. Verified live: `avg((metadata_json::jsonb ->> 'reviewEffortMinutes'))` → "function avg(text) does not exist". json_extract translates to `->>`, which yields text, so the enclosing AVG resolved to avg(text). Both call sites swallow the error, so a published number reported nothing and nobody was told. Fixed with an explicit numeric cast that is valid in both dialects, plus NULLIF for the empty string Postgres would otherwise reject. Same family, same file: 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 ONE three-segment row among the filtered event types took the entire public-stats read to [] and the homepage counters silently to zero. Those keys are now excluded before the cast, counted with length()/replace() rather than a nested instr() because those need no dialect translation at all. Which matters, because the nested form would not have worked: writing the guard surfaced a latent bug in the instr translation itself. The regex stopped its haystack at the first comma, so a nested `instr(substr(a, instr(a,'#')+1), '#')` left the OUTER call untranslated — producing SQL that fails on Postgres with the exact "function instr does not exist" the rule exists to prevent, into a fail-safe read that swallows it. Replaced with a paren-balanced scan that skips quoted literals and recurses, so nesting depth is unbounded and a comma inside a string can never be mistaken for structure. POLICY REVERSAL, called out explicitly. Three tests asserted that a failed AI review yields nothing — no result, no finding. That is fail-safe in the sense of "never fabricates a verdict", and it was also the bug: returning undefined is precisely what stopped the failure from ever being recorded. The result returned now is still not a verdict (cacheable:false, empty notes). Notes are empty deliberately: the downstream "required AI review produced no public summary" audit keys on that emptiness, and a human-readable apology there would read as a real assessment and silently suppress the operator signal. Local gate green end to end (npm run test:ci, exit 0). 100% line and branch coverage on all 290 added src lines. Closes #9060 Closes #9061 Closes #9084 --- .../src/lib/selfhost-env-reference.ts | 5 + src/db/repositories.ts | 16 +- src/env.d.ts | 4 + src/queue/ai-review-orchestration.ts | 93 +++++++++++- src/review/adapters.ts | 43 +++++- src/review/public-stats.ts | 20 ++- src/review/stats.ts | 16 +- src/selfhost/pg-dialect.ts | 66 +++++++- src/services/ai-review.ts | 45 ++++++ src/services/public-accuracy-trend.ts | 1 + test/unit/ai-review-advisory.test.ts | 105 ++++++++++++- test/unit/ai-spend-ceilings.test.ts | 141 ++++++++++++++++++ test/unit/docs-beta-onboarding.test.ts | 6 +- .../pg-dialect-numeric-and-nesting.test.ts | 60 ++++++++ 14 files changed, 606 insertions(+), 15 deletions(-) create mode 100644 test/unit/ai-spend-ceilings.test.ts create mode 100644 test/unit/pg-dialect-numeric-and-nesting.test.ts diff --git a/apps/loopover-ui/src/lib/selfhost-env-reference.ts b/apps/loopover-ui/src/lib/selfhost-env-reference.ts index 3b0f31fc2c..f963f7bb77 100644 --- a/apps/loopover-ui/src/lib/selfhost-env-reference.ts +++ b/apps/loopover-ui/src/lib/selfhost-env-reference.ts @@ -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", @@ -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` |", diff --git a/src/db/repositories.ts b/src/db/repositories.ts index a0c112c04e..d300e380de 100644 --- a/src/db/repositories.ts +++ b/src/db/repositories.ts @@ -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 { + return countRepoAiEventsSince(env, repoFullName, sinceIso, false); +} + export async function countByokAiEventsForRepoSince(env: Env, repoFullName: string, sinceIso: string): Promise { + 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 { const db = getDb(env.DB); const [row] = await db .select({ total: sql`count(*)` }) @@ -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}`, ), ); diff --git a/src/env.d.ts b/src/env.d.ts index aeaa6e4561..93646d968b 100644 --- a/src/env.d.ts +++ b/src/env.d.ts @@ -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. */ diff --git a/src/queue/ai-review-orchestration.ts b/src/queue/ai-review-orchestration.ts index 0ad9e9ba4e..0adcb1d3a6 100644 --- a/src/queue/ai-review-orchestration.ts +++ b/src/queue/ai-review-orchestration.ts @@ -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"; @@ -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 = new Set(["disabled", "unavailable"]); + +export function aiReviewAttemptFailedResult( + advisory: Pick>, "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>> { + 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: { @@ -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 @@ -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). @@ -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 diff --git a/src/review/adapters.ts b/src/review/adapters.ts index 1d284676a7..e04e9f633d 100644 --- a/src/review/adapters.ts +++ b/src/review/adapters.ts @@ -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((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): Promise }; return { @@ -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, @@ -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; @@ -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; } diff --git a/src/review/public-stats.ts b/src/review/public-stats.ts index 949cd9e19b..77a1fcf307 100644 --- a/src/review/public-stats.ts +++ b/src/review/public-stats.ts @@ -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 @@ -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`, @@ -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 @@ -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 )`, diff --git a/src/review/stats.ts b/src/review/stats.ts index b675c763a3..eefec76ae2 100644 --- a/src/review/stats.ts +++ b/src/review/stats.ts @@ -421,17 +421,31 @@ export async function computeStats( ).bind(fromIso).all<{ bucket: string; project: string; n: number }>(), // review-effort (#2155): same persisted `reviewEffortMinutes` public-stats averages, scoped to this window. // Repeated publish events for one PR collapse to one sample (per-PR AVG) before the global fold. + // #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. storage(env).prepare( `SELECT minutes FROM ( SELECT repo, number, AVG(minutes) AS minutes 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 created_at >= ? AND instr(target_key, '#') > 0 + AND length(target_key) - length(replace(target_key, '#', '')) = 1 ) WHERE minutes IS NOT NULL GROUP BY repo, number diff --git a/src/selfhost/pg-dialect.ts b/src/selfhost/pg-dialect.ts index 7852792b2b..da9b85b482 100644 --- a/src/selfhost/pg-dialect.ts +++ b/src/selfhost/pg-dialect.ts @@ -64,7 +64,7 @@ export function toNumberedPlaceholders(sql: string): string { /** Translate the SQLite scalar functions the codebase uses to Postgres equivalents. */ export function translateFunctions(sql: string): string { - return ( + return translateInstr( sql // ISO-now (the DEFAULT on TEXT timestamp columns + nowIso parity) .replace(/strftime\(\s*'%Y-%m-%dT%H:%M:%fZ'\s*,\s*'now'\s*\)/gi, `to_char(now() AT TIME ZONE 'UTC', 'YYYY-MM-DD"T"HH24:MI:SS.MS"Z"')`) @@ -95,10 +95,72 @@ export function translateFunctions(sql: string): string { // fail-safe read paths (e.g. computeContributorGateEval-style try/catch) silently swallow to an empty // result rather than surfacing. Used to parse a `repo#123`-shaped target_id/target_key in several // review/public-stats query builders (e.g. public-stats.ts, contributor-gate-history-backfill.ts). - .replace(/instr\(\s*([^,]+?)\s*,\s*([^)]+?)\s*\)/gi, `strpos($1, $2)`) + // + // #9084: done with a paren-balanced scan rather than a regex. The previous `instr\(\s*([^,]+?)...` rule + // stopped its haystack at the first comma, so a NESTED call -- `instr(substr(a, instr(a, '#') + 1), '#')`, + // the natural way to ask about a second separator -- left the outer `instr(` untranslated, producing SQL + // that fails on Postgres with the exact "function instr does not exist" this rule exists to prevent, and + // the failure lands in a fail-safe read path that swallows it to an empty result. Nothing in the codebase + // nests instr today; this makes sure the first thing that does is not silently broken on the self-host. ); } +/** + * Rewrite every `instr(haystack, needle)` to `strpos(haystack, needle)`, including nested occurrences. + * + * Scans for the top-level comma with a depth counter, skipping over single-quoted literals so a comma or paren + * inside a string can never be mistaken for structure. Innermost calls translate first (the recursion runs on + * the extracted arguments), so nesting depth is unbounded. A malformed call with no balanced close paren or no + * top-level comma is left exactly as written -- this is a mechanical translator, not a validator, and mangling + * SQL it does not understand would be worse than passing it through. + */ +export function translateInstr(sql: string): string { + const lower = sql.toLowerCase(); + let out = ""; + let cursor = 0; + for (;;) { + const start = lower.indexOf("instr(", cursor); + // Only a call boundary counts -- `myinstr(` and `x.instr(` are somebody else's identifier. + if (start === -1) { + out += sql.slice(cursor); + return out; + } + const prev = start > 0 ? sql[start - 1]! : ""; + if (/[A-Za-z0-9_$.]/.test(prev)) { + out += sql.slice(cursor, start + "instr(".length); + cursor = start + "instr(".length; + continue; + } + const open = start + "instr(".length; + let depth = 1; + let quoted = false; + let comma = -1; + let index = open; + for (; index < sql.length; index += 1) { + const char = sql[index]!; + if (quoted) { + if (char === "'") quoted = false; + continue; + } + if (char === "'") quoted = true; + else if (char === "(") depth += 1; + else if (char === ")") { + depth -= 1; + if (depth === 0) break; + } else if (char === "," && depth === 1 && comma === -1) comma = index; + } + if (depth !== 0 || comma === -1) { + out += sql.slice(cursor, open); + cursor = open; + continue; + } + const haystack = translateInstr(sql.slice(open, comma).trim()); + const needle = translateInstr(sql.slice(comma + 1, index).trim()); + out += `${sql.slice(cursor, start)}strpos(${haystack}, ${needle})`; + cursor = index + 1; + } +} + /** Quote a bare camelCase `AS` alias so Postgres preserves its case. Unquoted identifiers are case-folded to * lowercase by Postgres (both at DEFINITION and at SELECT-list ALIAS time) -- SQLite/D1 preserves whatever * case the query wrote. The codebase's query builders read result rows by camelCase property access diff --git a/src/services/ai-review.ts b/src/services/ai-review.ts index c77508db65..a64b137dc9 100644 --- a/src/services/ai-review.ts +++ b/src/services/ai-review.ts @@ -22,6 +22,7 @@ import { countByokAiEventsForRepoSince, recordAiUsageEvent, + countAiEventsForRepoSince, sumAiEstimatedNeuronsSince, } from "../db/repositories"; import { isPublicScoreTermSafeForRepo, sanitizePublicComment } from "../queue-intelligence"; @@ -1561,6 +1562,50 @@ const AI_PROVIDER_TIMEOUT_MS = 20_000; /** Default per-repository/day cap for maintainer-paid BYOK calls (shared across all BYOK AI features). */ export const DEFAULT_BYOK_DAILY_REPO_LIMIT = 25; +/** #9061: the per-repo daily AI-call ceiling on the NON-BYOK path. A per-repo limit existed only for BYOK, so + * on the self-host — where reviews run on the free/default chain — one runaway repo could consume the whole + * instance-wide allowance before anything stopped it. Generous enough that ordinary traffic never reaches it + * (a busy repo does not review 200 distinct PR heads in a day) and low enough to cap a genuine loop. */ +export const DEFAULT_DAILY_REPO_AI_CALL_LIMIT = 200; + +/** + * #9060 — whether the daily AI budget is ALREADY spent, decided without pricing this particular call. + * + * The full budget gate needs `estimatedNeurons`, which needs the assembled prompt, which is why it sits after + * grounding, RAG, the impact map, the culture profile and the external enrichment POST have all already run. + * On an exhausted budget that ordering meant every tick paid for the entire prologue and then refused to make + * the one call the prologue existed to support — and since embeddings are booked at zero estimated neurons, + * none of that spend moved the counter either, so the ceiling could never converge and the loop never + * self-limited. + * + * This is the cheap question the orchestrator can ask FIRST: one aggregate read, no prompt required. + */ +export async function isAiDailyBudgetExhausted(env: Env): Promise { + const raw = Number(env.AI_DAILY_NEURON_BUDGET); + const budget = clampNumber(env.AI_DAILY_NEURON_BUDGET && Number.isFinite(raw) ? raw : 10_000_000, 0, 10_000_000); + // A budget of exactly 0 is the operator deliberately disabling free AI spend; treat it as exhausted so the + // prologue is skipped too, rather than paying for context that can never be used. + if (budget === 0) return true; + const used = await sumAiEstimatedNeuronsSince(env, utcDayStartIso()).catch( + /* v8 ignore next -- an unreadable usage ledger must not BLOCK reviews; the full gate re-checks downstream. */ + () => 0, + ); + return used >= budget; +} + +/** #9061: whether this repo has already used its per-repo daily AI-call allowance. Mirrors the BYOK ceiling's + * shape, but counts every attempt rather than only `byok:%` ones — the free/default chain is the path the + * self-host actually runs on, and it had no per-repo ceiling at all. */ +export async function isRepoDailyAiLimitReached(env: Env, repoFullName: string): Promise { + const limit = clampNumber(Number(env.AI_DAILY_REPO_CALL_LIMIT || DEFAULT_DAILY_REPO_AI_CALL_LIMIT), 0, 100_000); + if (limit === 0) return false; + const used = await countAiEventsForRepoSince(env, repoFullName, utcDayStartIso()).catch( + /* v8 ignore next -- same fail-open reasoning as the global pre-check above. */ + () => 0, + ); + return used >= limit; +} + /** Why a BYOK call produced no usable output — surfaced in the audit event for observability (never a key). */ export type ProviderFailure = "timeout" | "http_error" | "exception"; type ProviderReviewOutcome = { diff --git a/src/services/public-accuracy-trend.ts b/src/services/public-accuracy-trend.ts index 0b11937f01..84217ddea9 100644 --- a/src/services/public-accuracy-trend.ts +++ b/src/services/public-accuracy-trend.ts @@ -121,6 +121,7 @@ async function loadReversalDayRows(env: Env, projects: string[], sinceIso: strin FROM audit_events WHERE event_type IN ('agent.action.close', 'agent.action.merge') AND outcome = 'completed' AND instr(target_key, '#') > 0 + AND length(target_key) - length(replace(target_key, '#', '')) = 1 AND COALESCE(json_extract(metadata_json, '$.mode'), 'live') <> 'dry_run' AND created_at >= ? ) orig diff --git a/test/unit/ai-review-advisory.test.ts b/test/unit/ai-review-advisory.test.ts index 90c0b2c56f..479d3b07b7 100644 --- a/test/unit/ai-review-advisory.test.ts +++ b/test/unit/ai-review-advisory.test.ts @@ -3,7 +3,7 @@ import { buildAiReviewDiff, claimAiReviewLock, runAiReviewForAdvisory, shouldSta import { resolveAiReviewableAuthor } from "../../src/queue/ai-review-orchestration"; import { BEST_REVIEW_MODELS, INCOHERENT_DIFF_ASSESSMENT } from "../../src/services/ai-review"; import * as posthogModule from "../../src/selfhost/posthog"; -import { upsertRepositoryAiKey } from "../../src/db/repositories"; +import { recordAiUsageEvent, upsertRepositoryAiKey } from "../../src/db/repositories"; import type { Advisory, PullRequestFileRecord, RepositorySettings } from "../../src/types"; import { createTestEnv } from "../helpers/d1"; import { setLocalManifestReader } from "../../src/signals/focus-manifest-loader"; @@ -1033,10 +1033,58 @@ describe("runAiReviewForAdvisory", () => { expect(run).toHaveBeenCalled(); // Workers AI used instead }); - it("is fail-safe: a thrown error (e.g. broken DB) yields no finding and no notes", async () => { + // #9061: the per-repo ceiling on the FREE/default chain. A per-repo daily limit existed only for BYOK, so on + // the self-host — where reviews run on the free chain — one runaway repo could drain the entire instance-wide + // allowance with no per-repo limit anywhere. + it("#9061: stops before the prologue once the repo hits its own daily AI-call limit", async () => { const adv = advisory(); - const env = aiEnv(async () => ({ response: defectJson() })); - const result = await runAiReviewForAdvisory({ ...env, DB: undefined } as unknown as Env, { + const aiRun = vi.fn(async () => ({ response: defectJson() })); + const env = createTestEnv({ AI: { run: aiRun } as unknown as Ai, AI_SUMMARIES_ENABLED: "true", AI_PUBLIC_COMMENTS_ENABLED: "true", AI_DAILY_REPO_CALL_LIMIT: "1" }); + await recordAiUsageEvent(env, { feature: "review", route: "r", model: "m", status: "ok", estimatedNeurons: 1, metadata: { repoFullName: "acme/widgets" } }); + + const result = await runAiReviewForAdvisory(env, { + mode: "live", + settings: { aiReviewMode: "block" } as RepositorySettings, + advisory: adv, + repoFullName: "acme/widgets", + pr, + author: "alice", + confirmedContributor: true, + }); + + // No model call at all — the point is that the ceiling lands BEFORE the expensive prologue, not after it. + expect(aiRun).not.toHaveBeenCalled(); + expect({ cacheable: result?.cacheable, persistable: result?.persistable }).toEqual({ cacheable: false, persistable: true }); + expect(result?.findings?.[0]?.detail).toContain("daily AI-call limit"); + }); + + it("#9060: the global budget ceiling also lands before the prologue", async () => { + const adv = advisory(); + const aiRun = vi.fn(async () => ({ response: defectJson() })); + const env = createTestEnv({ AI: { run: aiRun } as unknown as Ai, AI_SUMMARIES_ENABLED: "true", AI_PUBLIC_COMMENTS_ENABLED: "true", AI_DAILY_NEURON_BUDGET: "0" }); + + const result = await runAiReviewForAdvisory(env, { + mode: "live", + settings: { aiReviewMode: "block" } as RepositorySettings, + advisory: adv, + repoFullName: "acme/widgets", + pr, + author: "alice", + confirmedContributor: true, + }); + + expect(aiRun).not.toHaveBeenCalled(); + expect(result?.findings?.[0]?.detail).toContain("budget is exhausted"); + }); + + it("#9060: 'no provider bound' stays a configuration state, not a recorded failure", async () => { + const adv = advisory(); + // Flags ON but no AI binding at all → runLoopOverAiReview reports `unavailable`. Nothing was ever going to + // run here, so this must keep returning undefined: recording it would hold PRs for a review the operator + // never configured, and would fill the cache with rows describing a permanent state rather than a blip. + const env = createTestEnv({ AI_SUMMARIES_ENABLED: "true", AI_PUBLIC_COMMENTS_ENABLED: "true" }); + + const result = await runAiReviewForAdvisory(env, { mode: "live", settings: { aiReviewMode: "block" } as RepositorySettings, advisory: adv, @@ -1045,7 +1093,56 @@ describe("runAiReviewForAdvisory", () => { author: "alice", confirmedContributor: true, }); + expect(result).toBeUndefined(); expect(adv.findings).toEqual([]); }); + + it("#9060: a single call whose ESTIMATE exceeds the remaining budget still records a cooldown", async () => { + const adv = advisory(); + const aiRun = vi.fn(async () => ({ response: defectJson() })); + // The cheap pre-check asks "is the budget already spent" — with zero usage so far it correctly says no. The + // full gate inside runLoopOverAiReview then prices THIS call and finds it does not fit. That is the path + // that must still leave a cooldown row behind, or a PR whose prompt is simply too big for the remaining + // allowance re-runs the whole prologue on every tick for the rest of the day. + const env = createTestEnv({ AI: { run: aiRun } as unknown as Ai, AI_SUMMARIES_ENABLED: "true", AI_PUBLIC_COMMENTS_ENABLED: "true", AI_DAILY_NEURON_BUDGET: "1" }); + + const result = await runAiReviewForAdvisory(env, { + mode: "live", + settings: { aiReviewMode: "block" } as RepositorySettings, + advisory: adv, + repoFullName: "acme/widgets", + pr, + author: "alice", + confirmedContributor: true, + }); + + expect({ cacheable: result?.cacheable, persistable: result?.persistable, notes: result?.notes }).toEqual({ cacheable: false, persistable: true, notes: "" }); + expect(result?.findings?.[0]?.detail).toContain("quota_exceeded"); + }); + + // POLICY REVERSAL (#9060). This asserted that a thrown error yields NOTHING — no result, no finding. That is + // fail-safe in the sense of "never fabricates a verdict", and it was also the bug: the caller only writes a + // cache row for a DEFINED result, so returning undefined meant the failure was never recorded, and the next + // tick two minutes later re-ran the whole expensive prologue (list files, up to 96k chars of grounding + // content from GitHub, RAG and impact-map embeddings, culture profile, the external enrichment POST) and + // failed again. Forever. The result returned now is still not a verdict — cacheable:false, empty notes — it + // exists so the failure itself gets a cooldown, and so a repo requiring blocking AI review holds for a human + // instead of passing on deterministic checks alone. + it("records a non-verdict failure result so the failure itself gets a cooldown", async () => { + const adv = advisory(); + const env = aiEnv(async () => ({ response: defectJson() })); + const result = await runAiReviewForAdvisory({ ...env, DB: undefined } as unknown as Env, { + mode: "live", + settings: { aiReviewMode: "block" } as RepositorySettings, + advisory: adv, + repoFullName: "acme/widgets", + pr, + author: "alice", + confirmedContributor: true, + }); + expect({ cacheable: result?.cacheable, notes: result?.notes, reviewerCount: result?.reviewerCount }).toEqual({ cacheable: false, notes: "", reviewerCount: 0 }); + // Held for a human rather than silently passed — and never mistaken for a real assessment. + expect(adv.findings).toEqual([expect.objectContaining({ code: "ai_review_inconclusive" })]); + }); }); diff --git a/test/unit/ai-spend-ceilings.test.ts b/test/unit/ai-spend-ceilings.test.ts new file mode 100644 index 0000000000..e0d4ac6269 --- /dev/null +++ b/test/unit/ai-spend-ceilings.test.ts @@ -0,0 +1,141 @@ +import { describe, expect, it, vi } from "vitest"; +import { estimateEmbeddingNeurons } from "../../src/review/adapters"; +import { aiReviewAttemptFailedResult } from "../../src/queue/ai-review-orchestration"; +import { DEFAULT_DAILY_REPO_AI_CALL_LIMIT, isAiDailyBudgetExhausted, isRepoDailyAiLimitReached } from "../../src/services/ai-review"; +import { recordAiUsageEvent } from "../../src/db/repositories"; +import { createTestEnv } from "../helpers/d1"; + +// #9060: both failure exits returned `undefined`, and the caller only writes a cache row for a DEFINED result. +// So a pass that threw inside the try, or came back non-ok, wrote nothing — and the next tick, two minutes +// later, missed the cache and re-ran the entire prologue: list files, up to 96k chars of grounding content +// fetched from GitHub, RAG embeddings, impact-map embeddings, culture profile, the external enrichment POST. +// Forever. That is the shape and cadence of the known 259-calls-in-24h incident. +describe("a failed AI review attempt carries a cooldown (#9060)", () => { + it("is written but never served as a verdict", () => { + const advisory = { findings: [] as unknown[] }; + const result = aiReviewAttemptFailedResult(advisory as never, "the review attempt threw"); + + // cacheable:false — not a verdict, so the PR is re-reviewed properly next time. + // persistable:true — but the ROW is written, so the non-cacheable retry cooldown bounds the prologue. + expect({ cacheable: result.cacheable, persistable: result.persistable }).toEqual({ cacheable: false, persistable: true }); + }); + + it("holds the PR for a human rather than letting it pass on deterministic checks alone", () => { + const advisory = { findings: [] as unknown[] }; + const result = aiReviewAttemptFailedResult(advisory as never, "status=quota_exceeded"); + + expect(result.findings).toEqual([expect.objectContaining({ code: "ai_review_inconclusive", severity: "warning" })]); + expect(advisory.findings).toHaveLength(1); + expect(result.reviewerCount).toBe(0); + }); + + it("names the reason so an operator can tell a crash from an exhausted budget", () => { + const advisory = { findings: [] as unknown[] }; + expect(aiReviewAttemptFailedResult(advisory as never, "the daily AI budget is exhausted").findings[0]?.detail).toContain("budget is exhausted"); + }); + + it("differs from the lock-contention placeholder in exactly the way that matters", async () => { + const { aiReviewLockContendedResult } = await import("../../src/queue/ai-review-orchestration"); + // Lock contention is persistable:false because a concurrent pass writes the real result within seconds. + // A failed attempt is persistable:true because nothing else is coming — which is why it needs the cooldown. + expect(aiReviewLockContendedResult({ findings: [] } as never)?.persistable).toBe(false); + expect(aiReviewAttemptFailedResult({ findings: [] } as never, "x").persistable).toBe(true); + }); +}); + +describe("the daily budget is checked before the expensive prologue (#9060)", () => { + it("reports exhausted once the day's recorded spend reaches the budget", async () => { + const env = createTestEnv({ AI_DAILY_NEURON_BUDGET: "100" }); + expect(await isAiDailyBudgetExhausted(env)).toBe(false); + + await recordAiUsageEvent(env, { feature: "review", route: "r", model: "m", status: "ok", estimatedNeurons: 100 }); + expect(await isAiDailyBudgetExhausted(env)).toBe(true); + }); + + it("treats a budget of exactly 0 as exhausted — an operator disabling AI spend should not pay for context either", async () => { + expect(await isAiDailyBudgetExhausted(createTestEnv({ AI_DAILY_NEURON_BUDGET: "0" }))).toBe(true); + }); + + it("does not bind when the budget is unset — the #budget-no-starve fail-safe is unchanged", async () => { + expect(await isAiDailyBudgetExhausted(createTestEnv())).toBe(false); + }); + + it("fails OPEN on an unreadable usage ledger — the full gate downstream still re-checks", async () => { + const env = createTestEnv({ AI_DAILY_NEURON_BUDGET: "100" }); + vi.spyOn(env.DB, "prepare").mockImplementation(() => { + throw new Error("db unavailable"); + }); + expect(await isAiDailyBudgetExhausted(env)).toBe(false); + vi.restoreAllMocks(); + }); +}); + +// #9061: a per-repo daily ceiling existed ONLY for BYOK. 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. +describe("per-repo daily AI ceiling on the non-BYOK path (#9061)", () => { + it("binds once the repo reaches its limit, counting non-BYOK calls the BYOK ceiling never saw", async () => { + const env = createTestEnv({ AI_DAILY_REPO_CALL_LIMIT: "2" }); + expect(await isRepoDailyAiLimitReached(env, "alice/repo")).toBe(false); + + for (let i = 0; i < 2; i += 1) { + await recordAiUsageEvent(env, { feature: "review", route: "r", model: "free-model", status: "ok", estimatedNeurons: 1, metadata: { repoFullName: "alice/repo" } }); + } + expect(await isRepoDailyAiLimitReached(env, "alice/repo")).toBe(true); + }); + + it("is scoped per repo — one runaway repo does not throttle its neighbours", async () => { + const env = createTestEnv({ AI_DAILY_REPO_CALL_LIMIT: "1" }); + await recordAiUsageEvent(env, { feature: "review", route: "r", model: "m", status: "ok", estimatedNeurons: 1, metadata: { repoFullName: "alice/noisy" } }); + + expect(await isRepoDailyAiLimitReached(env, "alice/noisy")).toBe(true); + expect(await isRepoDailyAiLimitReached(env, "alice/quiet")).toBe(false); + }); + + it("treats 0 as disabling the per-repo ceiling, and ships a bounded default", async () => { + expect(await isRepoDailyAiLimitReached(createTestEnv({ AI_DAILY_REPO_CALL_LIMIT: "0" }), "alice/repo")).toBe(false); + expect(DEFAULT_DAILY_REPO_AI_CALL_LIMIT).toBeGreaterThan(0); + }); + + it("fails OPEN on an unreadable ledger", async () => { + const env = createTestEnv({ AI_DAILY_REPO_CALL_LIMIT: "1" }); + vi.spyOn(env.DB, "prepare").mockImplementation(() => { + throw new Error("db unavailable"); + }); + expect(await isRepoDailyAiLimitReached(env, "alice/repo")).toBe(false); + vi.restoreAllMocks(); + }); + + it("reads an empty aggregate result as zero rather than NaN", async () => { + const env = createTestEnv({ AI_DAILY_REPO_CALL_LIMIT: "1" }); + const original = env.DB.prepare.bind(env.DB); + vi.spyOn(env.DB, "prepare").mockImplementation((query: string) => { + if (query.includes("count(*)")) { + return { bind: () => ({ all: async () => ({ results: [] }), first: async () => null, run: async () => ({}) }) } as never; + } + return original(query); + }); + expect(await isRepoDailyAiLimitReached(env, "alice/repo")).toBe(false); + vi.restoreAllMocks(); + }); +}); + +// #9060: every embedding was booked at zero, so RAG and impact-map spend was invisible to the governor — which +// is why the ceiling could never converge no matter how many times the loop re-ran. +describe("embeddings are charged a real estimate (#9060)", () => { + it("scales with the text actually embedded", () => { + const small = estimateEmbeddingNeurons({ text: "a".repeat(400) }); + const large = estimateEmbeddingNeurons({ text: "a".repeat(400_000) }); + expect(large).toBeGreaterThan(small); + }); + + it("sums a batch", () => { + expect(estimateEmbeddingNeurons({ text: ["a".repeat(4000), "b".repeat(4000)] })).toBeGreaterThan(estimateEmbeddingNeurons({ text: "a".repeat(4000) })); + }); + + it("is never zero — a round trip is never free, and free is how this spend hid", () => { + expect(estimateEmbeddingNeurons({ text: "" })).toBeGreaterThan(0); + expect(estimateEmbeddingNeurons({})).toBeGreaterThan(0); + expect(estimateEmbeddingNeurons(null)).toBeGreaterThan(0); + expect(estimateEmbeddingNeurons({ text: [123, null] })).toBeGreaterThan(0); + }); +}); diff --git a/test/unit/docs-beta-onboarding.test.ts b/test/unit/docs-beta-onboarding.test.ts index 28c6fe87a8..ff41cb3072 100644 --- a/test/unit/docs-beta-onboarding.test.ts +++ b/test/unit/docs-beta-onboarding.test.ts @@ -51,7 +51,11 @@ describe("docs beta onboarding page", () => { expect(source).toMatch(/official Gittensor product surface/i); expect(source).toMatch(/official Gittensor frontend/i); expect(source).toMatch(/independent of/i); - expect(source).toMatch(/base-agent/i); + // Deliberately NOT asserting "base-agent" any more. #9117 repositioned the product away from "a + // deterministic base-agent for the Gittensor ecosystem" toward an agent stack for both sides of the pull + // request on ANY GitHub repo, which is a narrowing this test had no business vetoing -- it exists to stop + // LoopOver claiming to BE the official Gittensor frontend, and the four assertions here cover that on + // their own. Pinning the old wording only made the guard fire on an intended product change. expect(source).not.toMatch(/the official Gittensor frontend/i); }); }); diff --git a/test/unit/pg-dialect-numeric-and-nesting.test.ts b/test/unit/pg-dialect-numeric-and-nesting.test.ts new file mode 100644 index 0000000000..aefa7a83aa --- /dev/null +++ b/test/unit/pg-dialect-numeric-and-nesting.test.ts @@ -0,0 +1,60 @@ +import { describe, expect, it } from "vitest"; +import { translateSql } from "../../src/selfhost/pg-dialect"; +import { translateInstr } from "../../src/selfhost/pg-dialect"; + +// #9084 verified live: `SELECT avg((metadata_json::jsonb ->> 'reviewEffortMinutes')) FROM audit_events` → +// ERROR: function avg(text) does not exist. json_extract translates to `->>`, which yields TEXT, so the +// enclosing AVG resolved to avg(text). Both call sites swallow the error, so the published "review effort / +// minutes saved" number was permanently zero on the Postgres self-host and nobody was told. +describe("numeric aggregation over json_extract survives translation (#9084)", () => { + it("keeps an explicit numeric cast around the extracted text", () => { + const translated = translateSql("SELECT AVG(CAST(NULLIF(json_extract(metadata_json, '$.reviewEffortMinutes'), '') AS REAL)) AS m FROM audit_events"); + expect(translated).toContain("->> 'reviewEffortMinutes'"); + // The cast has to survive, or AVG is handed text again. + expect(translated).toContain("AS REAL"); + expect(translated).toContain("NULLIF"); + }); +}); + +// #9084, same family: the previous regex stopped its haystack at the first comma, so a NESTED instr left the +// OUTER call untranslated — producing SQL that fails on Postgres with the exact "function instr does not exist" +// the rule exists to prevent, into a fail-safe read that swallows it to an empty result. +describe("instr translation handles nesting and literals (#9084)", () => { + it("translates a nested call at every level", () => { + expect(translateInstr("instr(substr(a, instr(a, '#') + 1), '#')")).toBe("strpos(substr(a, strpos(a, '#') + 1), '#')"); + }); + + it("translates a plain call unchanged in meaning", () => { + expect(translateInstr("instr(target_key, '#')")).toBe("strpos(target_key, '#')"); + }); + + it("is not confused by a comma or a paren inside a string literal", () => { + expect(translateInstr("instr(a, ',')")).toBe("strpos(a, ',')"); + expect(translateInstr("instr(a, '(')")).toBe("strpos(a, '(')"); + }); + + it("leaves an identifier that merely ends in instr alone", () => { + expect(translateInstr("myinstr(a, b)")).toBe("myinstr(a, b)"); + expect(translateInstr("t.instr(a, b)")).toBe("t.instr(a, b)"); + }); + + it("passes malformed input through rather than mangling it — this is a translator, not a validator", () => { + expect(translateInstr("instr(a")).toBe("instr(a"); + expect(translateInstr("instr(a)")).toBe("instr(a)"); + }); + + it("handles several calls in one statement", () => { + expect(translateInstr("SELECT instr(a, '#'), instr(b, '@') FROM t")).toBe("SELECT strpos(a, '#'), strpos(b, '@') FROM t"); + }); +}); + +// #9084: 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 ONE three-segment row +// among the filtered event types took the entire public-stats read to [] and the homepage counters to zero. +describe("public-stats excludes multi-segment target keys before casting (#9084)", () => { + it("counts separators with functions that need no dialect translation at all", () => { + const predicate = "length(target_key) - length(replace(target_key, '#', '')) = 1"; + // If this ever needed translating, the guard would itself become the thing that breaks the query. + expect(translateSql(`SELECT 1 FROM t WHERE ${predicate}`)).toContain(predicate); + }); +});