test(rag): pin Supabase round-trip budgets on the answer path - #1450
Conversation
Ledger #98, the enabler it sequences before #99 and #101. Every finding in the 2026-07-28 latency audit was argued from reading code, and the fixes in PR #1377 were verified the same way. Nothing pinned the resulting round-trip counts, so a later refactor could add one to a hot path and no gate would notice. This makes that a red gate. `countSupabaseRoundTrips` wraps whatever stub a suite already builds and records every `.rpc(name)` and `.from(table)`. Builder methods are deliberately NOT counted: they are fluent and issue nothing, so counting them would measure query style rather than network cost and would churn whenever someone added a filter. One chain is one trip. Measured, not chosen: the offline source-only answer path issues 14 round trips. from:rag_aliases 1 rpc:match_document_chunks_text_v2 3 rpc:get_related_document_metadata_v2 2 from:document_index_quality 1 from:document_memory_cards 1 from:document_images 3 rpc:match_document_table_facts_text_v2 3 The budget does not endorse that count. Three calls each to the text-chunk, table-fact and image lookups for one query is the repeated-triple shape #101 already records, so this gives #101 a number to improve against rather than an inference. Second scenario guards shape rather than magnitude: five sources must cost the same 14 trips as one. A per-source round trip is the regression that a single-source budget cannot see, and it is measured to hold today. Non-vacuity was checked rather than assumed, which is the lesson from this session's other guards. Each budget first asserts the scenario produced a grounded answer and issued at least one trip, so a path that silently stopped running cannot pass as "within budget". A third test pins the mechanism: one extra `.from()` must move the count, and wrapping must not mutate the caller's stub (spread, not mutate, or two scenarios in one file share state). Registered in the offline contract list and its fixture so it runs as a standing guard, not an orphan test — `eval-rag-offline` is now 22 files / 570 tests. RAG impact: no retrieval behaviour change — this adds a test helper and a test, and appends one suite to the offline contract list. No scoring, ordering, selection, alias, citation or RPC logic is touched, and no file under src/lib/rag/** changes. The wrapper only observes: it delegates every call to the original client unmodified. Gates: verify:cheap exit 0, Test Files 436 passed (436), Tests 4572 passed | 4 skipped. eval-rag-offline 22 files / 570 tests. typecheck and prettier clean. No providers, no DB, per #98's own scope. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01361jh3eYVjJCzXWjAhdZiF
This pull request has been ignored for the connected project Preview Branches by Supabase. |
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in:40 minutes Your organization has reached its usage spending cap. Adjust your spending cap in the billing tab. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:d40f95b8fe
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Both Codex P2s on PR #1450. Both were right, and the first exposed a real over-count in the budget I had just measured. 1) Count at execution, not construction. A Supabase builder issues its request when awaited, not when `.from()` creates it. Counting at `.from()` charged a trip for a query that was built and abandoned, and charged only one for a builder awaited twice — which really is two requests. The counter now records when the returned thenable executes, via a proxy that re-wraps fluent links so the pending trip stays attached however long the chain gets. This moved the measured budget 14 -> 13. The line that disappeared is `from:document_memory_cards`: on this path that query is built but never executed, so it sends nothing and must not be charged. The old number counted traffic that does not exist. Two proofs added, exactly the ones the review asked for: an unexecuted builder costs zero, and a builder executed twice costs two. 2) Batch across documents, not just chunks. The five-source fixture inherited `document_id: "clozapine-doc"` and varied only the chunk id, so a regression hydrating once per distinct document would still have been a single call — the "no scaling" claim could pass over an N+1 path on realistic cross-document results. Each source now has a distinct document_id and page. Re-measured: still 13, so cross-document batching genuinely holds. That is now a real result rather than an artefact of a shared id. Also fixed en route, both mine: `MinimalSupabaseClient` used property syntax with `unknown[]` params, which rejects every concrete stub because a parameter typed `unknown` is not assignable to one typed `string` — method syntax with `never[]` is bivariant and accepts them. And the mechanism stubs now use their `table` / `name` parameters rather than prefixing them with an underscore, since this repo's ESLint has no argsIgnorePattern and ran at --max-warnings 0. Gates: verify:cheap exit 0, Tests 4574 passed | 4 skipped. lint, typecheck, prettier clean. eval-rag-offline 22 files. RAG impact: no retrieval behaviour change — tests and a test helper only. No file under src/lib/rag/** changes, and the counter still delegates every call to the original client unmodified. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01361jh3eYVjJCzXWjAhdZiF
…01v' into claude/latency-findings-impl-s8g01v
Adopts the one idea from Codex's parallel fix that mine lacked: assert the fixture actually spans five distinct documents before asserting the budget. Without it, someone tidying the fixture back to five chunks of one document would take the "no scaling" assertion straight back to proving nothing — the exact vacuity the cross-document change was made to remove. Same discipline as the budgets themselves: check the scenario is real before trusting its number. Gates: verify:cheap exit 0; lint exit 0; typecheck clean; the suite's 5 tests pass. RAG impact: no retrieval behaviour change — test-only. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01361jh3eYVjJCzXWjAhdZiF
Adopts the refinement from Codex's parallel fix, which handled this better than mine did. A promise memoises: awaiting it twice performs one request but invokes `then` twice, so counting promises on `then` would double-count a re-awaited one — a limitation I had documented rather than fixed. An async stub has already issued its request by the time it returns a promise, so the call is the right moment to record it. Only a lazy thenable — a real PostgrestFilterBuilder, which re-requests on each execution — is counted when it executes. The rule is now simply "count when the request is issued", and both shapes satisfy it. The measured budget is unchanged at 13, which is the useful part: two independently written counters, mine and Codex's, converge on the same number for this path. Gates: verify:cheap exit 0; the suite's 5 tests pass, including the abandoned builder at zero and the twice-executed builder at two. RAG impact: no retrieval behaviour change — test helper only. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01361jh3eYVjJCzXWjAhdZiF
Uh oh!
There was an error while loading. Please reload this page.
Sync main, then update two rows against evidence rather than adding new ones. #98: PR #1450 landed the counting proxy and answer-path budgets while this branch was open. Verified rather than assumed — the helper counts on execution not construction, tests/rag-round-trip-budget.test.ts pins two answer-path scenarios plus three counter self-tests, and it is registered in the offline contract fixture so it runs there. Ran it: Test Files 1 passed (1), Tests 5 passed (5). The row stays open with its Next narrowed to the two real gaps: /api/search has no budget, and eval-rag-offline/test-rag-offline were not wired. Also records the helper's own blind spot — it sees only traffic through the wrapped client. #130: already owns the unfiled pre-paint/cold-load guard, so its design goes there instead of a new row. Records what the guard must test (the pre-paint reserve seed, sampled before and after hydration rather than once after), why a zero-inset profile is required for it to be able to fail at all, and that it must be proven against the broken shape first. Also records the environment blocker: browser gates cannot launch here per #121, and the symlink bridge writes under /opt, which the sandbox refuses. No new ids allocated; both are updates to rows that already own the work. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018K7sEKH35KZkWxvCnQcNN2
…gn (#1455) * issues: close#122, capture the container Playwright pin mismatch Three related ledger items, each independently revertible. Close#122 (`ci/circleci: verify` fails on every branch). Its outcome allowed either "trustworthy signal again, or it stops reporting"; the second happened. `.circleci/config.yml` was deleted by 9779828 (PR #1412), and PR #1452's head reported 21 check runs with none named `ci/circleci: verify`, so the status no longer reports on new PRs. No operator log read is needed and the quota hypothesis is retired unproven. Capture #145: the remote container ships Chromium 1194 while the repo's Playwright pin wants 1234, so every browser test dies at launch and zero assertions run while the output reads like product breakage. This has cost time twice — the 2026-07-30 handoff records 13 launch failures read as a code defect, and #120 was filed on a gate reading taken under the same condition. The row gives the start-of-session check and keeps the existing "never run npx playwright install" stop rule. Fix a stale rule found while verifying #122: AGENTS.md cited `ci/circleci: verify` as a check that fails on unformatted files. It cannot report again, so the rule now names `Static PR checks` and records the CircleCI failures as history. The outstanding-issues diff is 4 insertions / 3 deletions ignoring whitespace; the rest is Prettier re-padding the archive table, because #122's original summary is wider than that column and was kept verbatim. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018K7sEKH35KZkWxvCnQcNN2 * issues: fold the Playwright pin evidence into #121, drop duplicate #145 Codex review was right: #121 ("Container Playwright browser build lags the pinned client") already tracks this exact condition — client 1234 versus container 1194, every browser test failing at launch — so #145 created a second canonical action for one problem. The row was allocated without first searching the open table, which is the dedupe step the issues skill requires. #145 is removed and its distinct content folded into #121: the reproduction on main at c5c1a86, the fact that the condition has now been misread twice (the handoff's 13 launch failures, and #120 filed as a gate defect under it), the detection command to run before trusting a browser gate, and the stop rule against filing a gate defect from a run whose tests never launched. #121's own workaround, PLAYWRIGHT_SKIP_BROWSER_DOWNLOAD note and open Next decision are unchanged. The id marker rolls back 146 -> 145 because #145 was never used by a live row. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018K7sEKH35KZkWxvCnQcNN2 * issues: record #98 delivery and #130 pre-paint guard design Sync main, then update two rows against evidence rather than adding new ones. #98: PR #1450 landed the counting proxy and answer-path budgets while this branch was open. Verified rather than assumed — the helper counts on execution not construction, tests/rag-round-trip-budget.test.ts pins two answer-path scenarios plus three counter self-tests, and it is registered in the offline contract fixture so it runs there. Ran it: Test Files 1 passed (1), Tests 5 passed (5). The row stays open with its Next narrowed to the two real gaps: /api/search has no budget, and eval-rag-offline/test-rag-offline were not wired. Also records the helper's own blind spot — it sees only traffic through the wrapped client. #130: already owns the unfiled pre-paint/cold-load guard, so its design goes there instead of a new row. Records what the guard must test (the pre-paint reserve seed, sampled before and after hydration rather than once after), why a zero-inset profile is required for it to be able to fail at all, and that it must be proven against the broken shape first. Also records the environment blocker: browser gates cannot launch here per #121, and the symlink bridge writes under /opt, which the sandbox refuses. No new ids allocated; both are updates to rows that already own the work. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018K7sEKH35KZkWxvCnQcNN2 --------- Co-authored-by: Claude <noreply@anthropic.com>
Closes the half of #98 that PR #1450 left open: /api/search had no budget, so an added round trip there was still an inference rather than a red gate. tests/search-round-trip-budget.test.ts pins searchChunksWithTelemetry — the function src/app/api/search/route.ts actually calls — using the existing counting proxy, and is registered in both the offline contract fixture and rag-offline-contract.mjs so it runs in the contract rather than on demand. Two scenarios. A lexical clinical search currently costs 11 round trips: rag_aliases 1, match_document_chunks_text_v2 3, match_document_table_facts_text_v2 3, get_related_document_metadata_v2 1, document_index_quality 1, document_images 2. Both the total and the breakdown are pinned, because a refactor swapping one probe for an unrelated query would hold the total while changing the traffic. Whether 3+3 text RPCs per search is intended is not settled here — the budget makes it visible and #98 now carries it as an open retrieval decision. The second scenario pins zero Supabase traffic for a query refused as adversarial, matching rag.ts's claim that prompt-injection intent is refused before any query issues. Both guards were proven against the broken shape rather than assumed: - The first control spent 0 trips because the module registry was cached between runs, so it would have passed while proving nothing. Fixed with vi.resetModules() inside the harness. - The refusal check initially failed on the results assertion rather than the round-trip one, demonstrating only that the test catches the regression, not that the budget can fail. The counter assertion is now ordered first; with a non-refused query it fails as "expected 11 to be +0". Counts are deterministic across three consecutive runs. No src/lib/rag file is edited; this observes the path, it does not change retrieval behaviour. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018K7sEKH35KZkWxvCnQcNN2
Summary
Ledger
#098— the enabler it sequences before#099and#101. Tests and harness registration only; no production code changes.Every finding in the 2026-07-28 latency audit was argued from reading code, and the fixes in PR #1377 were verified the same way. Nothing pinned the resulting round-trip counts, so a later refactor could add one to a hot path and no gate would notice.
#098's stated outcome is "per-scenario Supabase round-trip counts are pinned by a test, so an extra round trip on a hot path is a red gate rather than an inference" — this delivers that.tests/helpers/supabase-round-trip-counter.ts— wraps whatever Supabase stub a suite already builds and records every.rpc(name)and.from(table). Builder methods (.select,.eq,.order, …) are deliberately not counted: they are fluent and issue nothing, so counting them would measure query style rather than network cost and would churn whenever someone added a filter. One chain is one trip. The wrapper only observes — it delegates every call to the original unmodified.tests/rag-round-trip-budget.test.ts— drivesanswerQuestionWithScopethrough the counted client on the offline source-only path and pins the counts.scripts/rag-offline-contract.mjs+scripts/fixtures/rag-offline-contract-tests.json— registers the suite so it runs as a standing guard rather than an orphan test.The measurement
Measured, not chosen. The offline source-only answer path issues 14 round trips:
The budget does not endorse that count. Three calls each to the text-chunk, table-fact and image lookups for a single query is the repeated-triple shape
#101already records — hydration repeated across branches whilerag.ts:2751already parallelises three RPCs elsewhere. So this gives#101a number to improve against instead of an inference. If someone reduces it, the test fails and they lower the budget in the same commit; that is the intended workflow, and the file header says so.Second scenario guards shape, not magnitude: five sources must cost the same 14 trips as one. A per-source round trip is precisely the regression a single-source budget cannot see, and it is measured to hold today.
Non-vacuity — checked, not assumed
This session produced two guards that passed while proving nothing, so each budget here first asserts the scenario actually ran:
grounded, andWithout those, a path that silently stopped running would pass as "within budget" — the exact failure mode. Confirmed live: with the constant at
0the assertion failed atexpected 14 to be +0and printed the full breakdown, so the comparison is real and its failure message is actionable.A third test pins the mechanism, because a budget guard is only worth having if an added round trip moves it: one extra
.from()must increment the count, and wrapping must not mutate the caller's stub (spread, not mutate — otherwise two scenarios in one file share counting state). Proving that on the real path would mean editingsrc/lib/rag/**to inject a call, which is not worth doing to a protected surface for a mechanism this simple.RAG impact: no retrieval behaviour change — this adds a test helper and a test, and appends one suite to the offline contract list. No scoring, ordering, selection, alias, citation or RPC logic is touched, and no file under
src/lib/rag/**changes. The counter delegates every call to the original client unmodified, so the suites' existing stub behaviour is identical with or without it.Verification
npm run verify:cheap— exit 0,Test Files 436 passed (436),Tests 4572 passed | 4 skipped (4576).node scripts/eval-rag-offline.mjs—Test Files 22 passed (22),Tests 570 passed (570),Offline RAG fixture and production-contract checks passed.(21 → 22 suites, confirming the registration took effect.)validateOfflineContractTestsagainst the fixture —contract list and fixture agree (22 suites), so the required list and the JSON cannot drift.npm run typecheck,npx prettier --writethen clean.0, plus the dedicated mechanism test.Not run, and why: no browser or build gate applies to a test-only change.
check:driftnot run —supabase/**untouched. Every provider-backed gate (eval:*,verify:release,check:supabase-project,test:live) not run and not implicated:#098is explicitly scoped "no providers, no DB", and this runs entirely against the suite's existing offline stub.Risk and rollout
14is a measured constant, so any intended change to the answer path's traffic will fail this test until the budget is updated. That is the point, but it means the number must be maintained deliberately rather than reflexively relaxed — stated in the file header.Clinical Governance Preflight
Clinical KB Database(sjrfecxgysukkwxsowpy)Notes on the above: this PR adds tests and registers one suite, so every item is satisfied by construction rather than by inspection of new behaviour. No answer generation, citation, verification, source-governance or document-access logic changes; the fixture source is the existing synthetic clozapine record already used by
tests/rag-offline-answer.test.ts, clearly separated from real clinical content. No credential or Supabase value appears in the diff and no live client is constructed — the pinned target is referenced only as unchanged context. Clinical decision-support behaviour is unaffected, so the SaMD classification is unchanged.Notes
origin/mainafter docs(issues): re-apply #102's canary-gated correction lost to a merge #1440 merged, keeping the same name, per the merged-PR rule.#098's row named the correct files thanks to its own earlier correction: an earlier version pointed attest-cache-path.mjsandcheck-rag-fixtures.mjs, neither of which exercises a RAG request, so a harness built on them "would have counted nothing". Building onrag-offline-contractinstead is what makes these counts real.#101can now be attempted against a measured baseline, and#099's remaining round trips have a place to be pinned as they are removed.🤖 Generated with Claude Code
https://claude.ai/code/session_01361jh3eYVjJCzXWjAhdZiF
Generated by Claude Code