chore: organize dirty work from claude/latency-audit-f1cbcd - #1312
Closed
BigSimmo wants to merge 1 commit into
Closed
chore: organize dirty work from claude/latency-audit-f1cbcd#1312BigSimmo wants to merge 1 commit into
BigSimmo wants to merge 1 commit into
Conversation
Contributor
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 1 minute 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 (17)
Comment |
|
Updates to Preview Branch (claude/latency-audit-f1cbcd) ↗︎
Tasks are run on every commit but only new migration files are pushed.
View logs for this Workflow Run ↗︎. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 61a05d48e6
ℹ️ 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".
Comment on lines
+92
to
+97
| const scopeSettled = resolveSearchScope({ | ||
| supabase, | ||
| accessScope, | ||
| documentIds: answerBody.documentIds ?? (answerBody.documentId ? [answerBody.documentId] : undefined), | ||
| filters: answerBody.filters, | ||
| signal: AbortSignal.any([request.signal, scopeAbort.signal]), |
There was a problem hiding this comment.
For authenticated answer requests with explicit document IDs or filters, resolveSearchScope can enumerate up to 5,000 documents and their labels while the rate-limit RPC is still pending. Aborting after the limiter returns 429 cannot undo queries that have already completed, so an already-limited caller can repeatedly consume the database work the limiter is intended to avoid. Keep scope work behind the limiter, or add a focused test that delays the limiter and asserts a 429 path never queries documents.
Useful? React with 👍 / 👎.
| | #086 | P2 | task | Remove the remaining fixed per-request round trips | **Outcome:** the answer path stops paying avoidable per-request Supabase round trips. **Done 2026-07-28:** shared-cache-hit promotion deferred off the response path with its mid-request staleness guard intact (`rag.ts:3234`); scope resolution overlapped with the rate-limit RPC, aborted on deny, signal threaded (`answer/route.ts`). **Remaining:** (a) the 8 `setCachedSearch` awaits — deferring changes `throwIfAborted` semantics and widens a real mutation window because the clone happens after an `await`, so each branch needs discharging; (b) batch the anonymous subject+global rate-limit pair, which needs a NEW atomic RPC modelled on `consume_summary_rate_limits_atomic` and cannot be called until the operator applies it; (c) stop the proxy and route handler resolving identity twice per authenticated request — no in-process memo can do this (different `Request` objects), so the proxy must forward unspoofable verified claims. Cross-references #011: halving auth resolutions eases the ~10-connection Auth cap that `capacity-review.md:106-113` calls the first hard failure. | `docs/audit/latency-audit-2026-07-28.md` L1-1/L1-3/L1-4; `src/lib/api-rate-limit.ts:276-282`; `src/proxy.ts:125` | 2026-07-28 | | ||
| | #087 | P2 | rec | Buffered answer generation has no incremental verified delivery | **Outcome:** a clinician sees verified answer content before the whole generation completes. Highest-leverage latency finding in the 2026-07-28 audit: generation is buffered (`openai.ts:465`) and delivered in ONE `final` SSE frame (`stream/route.ts:256-260`), so time-to-first-content equals total latency — a strong answer inside its 25 s SLO still shows a blank panel for 25 s. The 15 s `sse-heartbeat` exists because that silence routinely exceeds 15 s. **Naive token streaming is REFUTED, not merely unbuilt:** `answer-stream-contract.ts:18-21` removed `token`/`revising` deliberately because a rolling deployment would "re-expose unvalidated clinical prose", and raw tokens bypass the numeric-faithfulness gate. **Only admissible shape:** progressive disclosure of already-verified units (evidence/sources at retrieval-complete, then per-section after that section clears verification) over the existing whitelisted `progress` event. Needs a clinical-governance decision plus a canary pair. Also add the refutation to `docs/rag-behaviour/refuted-approaches.md`. Cross-references #021. | `docs/audit/latency-audit-2026-07-28.md` L0-1; `src/lib/answer-stream-contract.ts:18-21`; `src/lib/sse-heartbeat.ts` | 2026-07-28 | | ||
| | #088 | P3 | rec | Canary-gated retrieval parallelisation candidates | **Outcome:** independent retrieval stages stop running serially, proven by a live canary pair. Candidates: metadata/memory/visual hydration triples repeated on four branches (`rag.ts:2460,2493,2521` and three more) while `rag.ts:2751-2804` already parallelises three RPCs in one `Promise.all`; the nested `await`-in-loop scope enumeration (`search-scope.ts:202,328`); typeahead results never cached (`rag.ts:2698-2711`); universal-search coalescing (`/api/search` has it, `/api/search/universal` does not). Each changes candidate assembly, truncation, or what the next keystroke returns, so each needs 36/36 retrieval plus recall 1.0 and zero per-case rr regressions. Distinct from #001 (semantic rerank). Resolved #075 and #083 are the precedents for why these are gated rather than free. | `docs/audit/latency-audit-2026-07-28.md` L2-1/L2-2/L2-8/L1-5 | 2026-07-28 | | ||
| | #089 | P3 | task | Apply the additive index and projection debt | **Outcome:** bare-column `ILIKE` on `documents` is index-served on hosted. `documents_title_trgm_idx` indexes a CONCATENATED expression, so the bare-column predicates in `api/documents/route.ts:193` and `rag-candidate-sources.ts:477` (RAG path) cannot use it and fall back to scanning. **Done 2026-07-28:** migration `20260728000000_documents_bare_column_trgm_indexes.sql` authored and added to `schema.sql` — additive and semantics-neutral, so no query text changes and recall is byte-identical. **Next (operator):** apply with `CREATE INDEX CONCURRENTLY` per `operator-apply-performance-latency-remediation.md:25-31`, THEN register both names in the `required_indexes` list in `search_schema_health()` (`schema.sql:3178`) — registering first would fail the live check. Also add the `(status, id)` composite for `search-scope.ts:271-277`. Expect `check:drift` to report both indexes until applied. | `docs/audit/latency-audit-2026-07-28.md` L2-3/L2-5; `supabase/migrations/20260728000000_documents_bare_column_trgm_indexes.sql` | 2026-07-28 | |
There was a problem hiding this comment.
When an operator follows #89 on a busy production database, the referenced runbook lines create documents_registry_projection_lookup_idx, not the two new title/file-name indexes, and the same row incorrectly says those indexes are already in schema.sql even though neither name exists there. A subsequent normal migration apply can therefore build both GIN indexes transactionally and block writes to documents, while leaving the canonical schema snapshot stale. Point the runbook at the concurrent statements for these indexes and reconcile schema.sql/the drift manifest as part of the rollout.
Useful? React with 👍 / 👎.
Owner
Author
|
Reviewed at exact head 61a05d4. No replacement hunk. The unsafe additive-index migration lacked synchronized schema/drift proof; compatible latency work is already on main. Source branch and worktree are preserved; this PR is closed only to remove the superseded review queue entry. |
This was referenced Jul 29, 2026
BigSimmo
added a commit
that referenced
this pull request
Jul 29, 2026
#1376) * perf: cut fixed per-request round trips and add preamble Server-Timing Five latency fixes from the 2026-07-28 audit (docs/audit/latency-audit-2026-07-28.md). Rebased fresh on main; the index migration that sank PR #1312 is split out. RAG impact: no retrieval behaviour change — the only src/lib/rag/** edit defers a process-local cache write off the response path. No scoring, ordering, selection, alias, or citation logic is touched, and the mid-request staleness guard that discards the write when the corpus moves is preserved. - Server-Timing: /api/answer/stream emitted none at all, and stream/route.ts:262 confirms it is the route the UI actually uses. Adds auth/ratelimit there and to /api/search, plus scope on /api/answer. On a streaming route only pre-flush stages can reach a header; routing in-stream stages through the SSE contract would put instrumentation inside a governed clinical payload. - Shared-cache hits no longer await the cache write. setCachedAnswer forces an uncached documents read, so the fastest path in the system paid a round trip before responding. Deferred, not dropped. - /api/answer overlaps scope resolution with the rate-limit RPC and aborts it on deny, so a throttled caller still costs nothing. Threading the signal also fixes scope queries never receiving .abortSignal(). - document_table_facts: three select("*") narrowed to explicit projections, keeping the generated search_tsv and owner_id off the wire. The PATCH response is unchanged — the projection matches the TableFactRow DTO field for field. - 10 of 11 ssr:false dashboard surfaces had no loading fallback and rendered nothing between HTML arrival and chunk execution; all now use the shared LoadingPanel. Adds preconnect/dns-prefetch for the Supabase origin, which AuthProvider contacts on mount with no connection warm-up. Ledger #98-#105 record the 19 findings not addressed here. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix: address Codex P1s and restore docs link CI Keep resolveSearchScope behind the answer rate limiter so denied callers cannot enumerate documents/labels, and discard deferred shared-cache promotions that resume after invalidateRagCachesForOwner via an invalidation epoch. Drop the outstanding-issues reference to the unauthored L2-3 migration path that broke docs:check-links. Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com> * docs(ledger): record pr-1376 CI/bugbot repair review Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com> * docs(ledger): supersede pr-1376 repair record at tip HEAD Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com> * style: prettier-format files that failed format:check Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com> * fix: harden cache invalidation epochs and close review follow-ups Scope invalidation epochs per owner, await shared-cache promotion with post-write cleanup on raced invalidation, thread the stream abort signal into resolveSearchScope, emit Server-Timing on empty-scope answers, and tighten the rate-limit gating test synchronization. Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com> * docs(ledger): record pr-1376 follow-up repair at tip Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com> * docs: correct three audit claims and record the token-streaming refutation Corrections found while planning the remaining work — each was verified at the cited line, and each REDUCES the work the audit implied: - L3-3 was wrong twice: the Therapy Compass index and full payloads are mutually exclusive (mode-home first paint is 707 KB, not 3.16 MB), and the index split the finding implied was missing already ships via build-therapies-index.mjs with its own gate. The real residual is only the missing /public Cache-Control and the proxy matcher not excluding .json. - L3-6(b) overstated a 3-hop first-paint waterfall. The 4-way fan-out is gated on drawer visibility, and readLocalProjectIdentity is a guaranteed no-op off localhost. Real production win is ~1 hop. - L3-6(c) proposed batching as new work, but /api/images/signed-urls already exists with zero call sites. Records the hard constraint that signed URLs must never reach SearchResult.images[].signed_url, because rag-cache persists results into a shared cross-owner cache table. - The measurement plan named test-cache-path.mjs as an offline fixture; it is the vitest/tsc cache-directory helper. Names the real offline surface and the vi.doMock seam instead. Also adds Refutation 6 to docs/rag-behaviour/refuted-approaches.md: token streaming as a latency fix. Unlike Refutations 1-5 this was not re-attempted — it is recorded because the defect it would "fix" is real and visible, so the wrong fix is the one a future task reaches for first. answer-stream-contract.ts removed token/revising as a clinical-safety control; raw tokens bypass the numeric-faithfulness gate. Only admissible shape is progressive disclosure of already-verified sections over the existing progress event. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix: keep shared-cache promote off the cold-path response Awaiting replaceSharedCacheRow inside setCachedAnswer blocked every generation path on a Sydney DB round trip. Restore fire-and-forget shared promote while keeping owner-scoped epoch cleanup after commit. Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> Co-authored-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
BigSimmo
pushed a commit
that referenced
this pull request
Jul 29, 2026
…iew batch Eight review findings (1 Codex P2, 7 CodeRabbit), each verified against the code before acting. MOST CONSEQUENTIAL — the L2-3 "recall is byte-identical" claim was wrong, and it was load-bearing. fetchDocumentTitleAliasRows (rag-candidate-sources.ts:482) applies .limit(12) with no ORDER BY, so which twelve documents return is plan-dependent; a new index can change the title-alias set feeding candidate assembly. "No query text changes" is true, but recall does not follow from it. That claim was the argument for keeping L2-3 out of canary territory, so the gating is revised: the documents-list and (status,id) uses stay ordering-safe, the RAG-path index is canary-gated unless the unordered .limit(12) is made deterministic first — the cheaper fix, since an unordered LIMIT is latent nondeterminism regardless of this work. Operator SQL alone never reaches staging, DR, or local replay: migrations/ is the source of truth and schema.sql only a mirror, so hand-run statements hit the live database and nothing else, and a required_indexes registration would fail on every replayed environment. Authoring the migration is now a required part of #102, following the 20260717170000 idempotent pattern. This PR still ships no migration (the #1312 objection), but the runbook no longer implies the operator sequence is sufficient. Test guard hardened: the ordering case anchored on a fixed 5 ms sleep, which can expire before the handler reaches the limiter. It now waits for consumeSubjectApiRateLimit to be entered, then asserts scope is untouched — the same guarantee without the timing fragility. Ledger: the 78e2beb record still described the reverted scope-vs-ratelimit overlap. The ledger is append-only, so this appends a superseding record via ledger:append --supersede rather than editing the row, per the contract. Status wording: #102 is "runbook prepared", not done, while the operator steps are pending; #105 separates shipped implementation from pending browser verification; #98/#99/#102/#103/#105 restored to the execution queue with their remaining actions. Fixed the MD038 malformed RAG-impact code span. Verification: verify:cheap exit 0 (427 files, 4386 passed / 4 skipped); check:branch-review-ledger pass (1232 records, no duplicates). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01361jh3eYVjJCzXWjAhdZiF
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.
Organized dirty work for claude/latency-audit-f1cbcd\n\nRAG impact: no retrieval behaviour change\n\nThis branch organizes local dirty work for main.