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); + }); +});