fix(cost,db): bound runaway AI spend, and stop reporting zero for a metric that has been broken since it shipped - #9146
Merged
Merged
Conversation
…etric that has been broken since it shipped #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
Contributor
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
Contributor
Logic backtestReplayed 0 historical case(s) for Backtest comparison:
|
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
loopover-ui | 1108e82 | Commit Preview URL Branch Preview URL |
Jul 27 2026, 04:15 AM |
Bundle ReportChanges will increase total bundle size by 61 bytes (0.0%) ⬆️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: loopover-uiAssets Changed:
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #9146 +/- ##
==========================================
+ Coverage 90.56% 92.68% +2.12%
==========================================
Files 96 815 +719
Lines 22490 80939 +58449
Branches 3884 24586 +20702
==========================================
+ Hits 20367 75015 +54648
- Misses 1945 4844 +2899
- Partials 178 1080 +902
Flags with carried forward coverage won't be shown. Click here to find out more.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
#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:listPullRequestFiles, 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-churnfix 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 here:That is the exact inverse of
aiReviewLockContendedResult, which ispersistable: falseprecisely because a concurrent pass writes the real result within seconds. Here nothing else is coming, which is why the cooldown must.disabledandunavailableare deliberately excluded — the operator switched AI review off, or no provider is bound. Those are configuration states, not failed attempts, and must keep returningundefinedrather than holding PRs for a review nobody configured.Two compounding parts, both named in the issue:
estimatedNeurons: 0. So RAG and impact-map spend never moved the governor's counter — meaning the ceiling could never converge and the loop never self-limited, no matter how many times it ran. They 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 — and the two counters now share one implementation, since thebyok:%model filter was their only difference and was the whole gap.Also: with
AI_EMBEDunset, embeddings route onto the frontier review chain and bill at frontier rates. The fallback stays (removing it would silently disable RAG for every existing deployment relying on it) but it now warns, so an operator knows what they are paying for.#9084 — the review-effort metric has been silently zero on Postgres
Verified live:
avg((metadata_json::jsonb ->> 'reviewEffortMinutes'))→ERROR: function avg(text) does not exist.json_extracttranslates to->>, which yields text, so the enclosingAVGresolved toavg(text). Both call sites swallow the error, so a published number reported nothing and nobody was told. Fixed with an explicit numeric cast valid in both dialects, plusNULLIFfor the empty string Postgres would otherwise reject outright.Same family, same file.
target_keyis not uniformly two-segment —regateRepairTargetKeymintsrepo#pr#headSha. On SQLite the INTEGER cast ofpr#shais 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 withlength()/replace()rather than a nestedinstr()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
instrtranslation itself. The regex stopped its haystack at the first comma, so a nestedinstr(substr(a, instr(a,'#')+1), '#')left the outer call untranslated, producing SQL that fails on Postgres with the exact "function instr does not exist" that 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. Nothing nestsinstrtoday; this makes sure the first thing that does is not silently broken on the self-host.Policy reversal — please read
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
undefinedis precisely what stopped the failure from ever being recorded, which is what let the loop re-run forever.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 exactly that emptiness, so a human-readable apology there would read as a real assessment and silently suppress the audit plus Sentry signal an operator needs. The hold reaches the contributor through the finding instead.Verification
npm run test:ci— exit 0, full gate green.src/lines, verified by intersectinggit diff -U0againstlcov.infoDA:/BRDA:records.selfhost:env-referencewas regenerated and committed.Closes #9060
Closes #9061
Closes #9084