Skip to content

Implement the free and flag-gated latency audit findings - #1377

Merged
BigSimmo merged 37 commits into
mainfrom
claude/latency-findings-impl-s8g01v
Jul 29, 2026
Merged

Implement the free and flag-gated latency audit findings#1377
BigSimmo merged 37 commits into
mainfrom
claude/latency-findings-impl-s8g01v

Conversation

@BigSimmo

@BigSimmo BigSimmo commented Jul 29, 2026

Copy link
Copy Markdown
Owner

Summary

  • Implements the free- and flag-gated findings from the 2026-07-28 latency audit, which was proposed as PR chore: organize dirty work from claude/latency-audit-f1cbcd #1312 and closed unmerged. That PR was closed because its additive-index migration lacked synchronized schema/drift proof; the closing note also said "compatible latency work is already on main", but verification on 6021f6d found none of the six applied changes had landed, and the audit document itself was absent, so its follow-ups were untracked anywhere.
  • Adds Server-Timing preamble stages (auth/ratelimit/scope). This was the repo's largest measurement gap: /api/answer/stream — the route the UI actually calls — emitted no header at all, and /api/search emitted none either. Only pre-header stages appear on the stream route, because headers flush before the first SSE frame and routing in-stream durations through the SSE contract would put instrumentation inside a governed clinical payload (answer-stream-contract.ts:18-21 whitelists only progress/final/error).
  • L1-2 — corrected after review. /api/answer threads signal: request.signal into resolveSearchScope, fixing a pre-existing defect where a client disconnect could not cancel scope's paginated queries even though search-scope.ts:200,328 always supported .abortSignal(...). The concurrency part of this finding was written, reviewed, and removed — see "Correction" below. Scope resolution runs behind rate-limit admission.
  • L1-1: the shared-cache-hit path — the fastest path in the system — no longer awaits setCachedAnswer before responding, so serving a cached answer no longer requires a fresh documents query first. forceRefresh: true is deliberately kept: it is the mid-request staleness guard that discards the write when the corpus moves, and that constraint is now documented on the function itself.
  • L2-6: three select("*") calls on document_table_facts narrowed to explicit projections, reusing the existing tableFactDetailProjection rather than duplicating it. This keeps the generated search_tsv tsvector and owner_id off the wire. The PATCH response shape is unchanged — the projection matches the TableFactRow DTO field for field.
  • L2-9: /api/medications builds the governance map and the fields=index projection once at module scope instead of mapping every record on every anonymous request. Ranking still runs per query.
  • L3-4 / L3-5: ten ssr: false dashboard surfaces rendered nothing between HTML arrival and chunk execution and now use the shared LoadingPanel primitive (role="status" plus an accessible label); the Supabase origin gains preconnect and dns-prefetch, since AuthProvider awaits a cross-origin getUser() on mount that every auth-gated fetch queues behind. Neither is held behind ledger #017, which governs payload decisions — a loading fallback ships zero bytes and a resource hint ships about sixty.
  • Deliberately not implemented: the six canary-gated findings (each needs a live eval pair and explicit approval), the seven #017-gated findings (need live Web-Vitals evidence first), and the four the audit retired during verification. All are filed as ledger #098#105.
  • L2-3 / L2-5 SQL is authored, not applied. No migration file and no supabase/** change: the three CREATE INDEX CONCURRENTLY statements live in docs/operator-apply-performance-latency-remediation.md with the apply → mirror → regenerate → register ordering spelled out. Registering the index names in required_indexes first would turn the live health check red, and shipping the migration without the schema mirror is precisely what closed PR chore: organize dirty work from claude/latency-audit-f1cbcd #1312.
  • L4-2 is retracted. The audit called the worker's triple image read "CONFIRMED with no fix evidence"; it is in fact a deliberate peak-memory trade-off already documented in place at worker/main.ts:866-869 and dispositioned as such by the 2026-07-01 audit. Recorded as a correction in #104 so a third audit does not resurrect it.

RAG impact: no retrieval behaviour change — the only src/lib/rag/** edits defer a process-local cache write off the response path and add a doc comment; no scoring, ordering, selection, alias, or citation logic is touched, and the mid-request staleness guard is preserved.

Correction — L1-2's concurrency was refuted on review (e52c25c)

An earlier revision of this PR started resolveSearchScope concurrently with the rate-limit RPC and aborted it on deny, and this description claimed a throttled caller "still costs nothing". That claim was wrong, as Codex review raised at P1 and I confirmed against the code.

resolveSearchScope returns without touching the database only when there are no filters and no explicit document ids (the early returns at search-scope.ts:242 and :253). With either present it enters the paginated documents loop at :269 plus the nested label loop — and an AbortSignal cancels the client request without un-executing a statement Postgres has already begun. Because filters is caller-controlled, a throttled caller could keep spending database capacity while collecting 429s. That is the opposite of what admission control is for, and the wrong direction against capacity-review.md:106-113, which names Postgres CPU under concurrency the first soft failure — the very argument this PR uses for why round-trip reduction has capacity value.

Scope now sits behind admission. What is kept from the original change is the threaded abort signal and the scope timing stage, both of which are independent of the overlap and correct on their own. tests/answer-route-preamble.test.ts is the guard the reviewer asked for: no scope call may begin while the limiter is pending, and a denied request sent with filters must dispatch none at all. Both cases were checked to fail against the overlapping shape rather than pass vacuously. The refutation and its precondition — a non-database admission gate ahead of the durable limiter — are recorded in the audit's L1-2 section and ledger #099 so a later latency pass does not rediscover it as an obvious win.

Verification

  • npm run verify:pr-local — exit 0. Format, lint, typecheck, Test Files 418 passed (418) / Tests 4244 passed | 4 skipped (4248), production build ✓ Compiled successfully in 59s, Client bundle secret surface check passed., Offline RAG fixture and manifest validation passed (36 golden cases, 21 suites).
  • npm run verify:cheap — exit 0 after the main merge and the L1-2 correction: Test Files 423 passed (423) / Tests 4278 passed | 4 skipped (4282).
  • npm run check:branch-review-ledger — passed after the main merge, guarding the merge=union duplication risk that ledger #088 watches for.

UI verification not run: the UI change is ten loading fallbacks built from the existing shared LoadingPanel primitive plus two <link> resource hints, with no new markup, styling, layout, or motion. Per the agreed scope for this pass, npm run verify:ui is deferred and tracked in ledger #105, which also asks for confirmation that the preconnect reaches <head> on a live page.

npm run eval:retrieval:quality, eval:rag, eval:quality, check:supabase-project and verify:release were not run: all are provider-backed, and no retrieval, ranking, selection, chunking, scoring, or answer-generation behaviour changed. check:drift and drift:manifest were not run because supabase/** is untouched, so there is nothing to re-derive.

Note on the red PR required runs: the job log shows COVERAGE_RESULT: success, BUILD_RESULT: success, STATIC_RESULT: success, SAFETY_RESULT: success, DB_RESULT: success, and UI_RESULT: cancelled##[error]production-ui result was cancelled. That is ledger #095 — the aggregate calls require_success on production-ui, so a push that supersedes an in-flight run reports failure indistinguishably from a real one. No substantive job failed.

Risk and rollout

  • Risk: Low. After the correction the /api/answer preamble is fully sequential, so this PR adds no concurrency to the answer path and no work to denied requests; the ordering is pinned by a test that fails against the overlapping shape. The cache-write deferral cannot change what is served — it moves a write that was already discarded on a staleness mismatch off the response path, and the discard condition is untouched. The narrowed projections were checked field-by-field against the consuming DTO. The medication memo caches only query-independent derivations of an already-memoised snapshot.
  • Rollback: revert the commits. No migration, no schema change, no data change, no configuration change, so revert is complete and immediate.
  • Provider or production effects: None. No OpenAI, Supabase, or hosted-CI call was made in producing or verifying this change. The operator SQL is documentation only and applies nothing.

Clinical Governance Preflight

  • Source-backed claims still require linked source verification before clinical use
  • No patient-identifiable document workflow was introduced or expanded without explicit governance approval
  • Supabase target remains Clinical KB Database (sjrfecxgysukkwxsowpy)
  • Service-role keys and private document access remain server-only
  • Demo/synthetic content remains clearly separated from real clinical sources
  • Source metadata, review status, and outdated/unknown-source behavior remain conservative
  • Deployment classification/TGA SaMD impact was checked when clinical decision-support behavior changed

Notes on the above: no answer content, citation, verification, or governance logic is touched, so source-backed claims and conservative source-metadata behaviour are unchanged by construction. Narrowing the table-facts projections strictly reduces what leaves the server — it stops owner_id being returned on the PATCH response. The preconnect reads only the already-public NEXT_PUBLIC_SUPABASE_URL and emits nothing when that variable is absent, so demo mode is unaffected and no credential enters the client graph; the client bundle secret surface check passed. The Supabase target is unchanged because no Supabase configuration is touched at all. There is no change to clinical decision-support behaviour, so the SaMD classification is unaffected.

Notes

  • Ledger #098#105 are new and replace the #085#092 numbering the original audit draft used; those IDs were taken by unrelated items before this landed. #016 gains the L3-1/L3-2/L3-3/L3-6/L3-7 detail and a correction to its Therapy Compass framing: those files are unversioned and served with an ETag, so repeat visits pay revalidation round trips rather than the full 3.16 MB, and the fix is content-hashed filenames rather than a bare Cache-Control line.
  • The #017 exemption for L3-4/L3-5 is a deliberate reading, stated in both the audit and #105: #017 gates payload decisions, and a zero-byte fallback cannot be justified or refuted by a Lighthouse number.

🤖 Generated with Claude Code

https://claude.ai/code/session_01361jh3eYVjJCzXWjAhdZiF

Summary by CodeRabbit

  • Performance
    • Improved anonymous medication catalogue responses by reusing precomputed index projections and governance mappings.
    • Added browser network warming (preconnect/dns-prefetch) and clarified therapy catalogue JSON behavior with ETag revalidation.
  • Reliability
    • Strengthened /api/answer request admission, abort propagation, and error handling to avoid unnecessary downstream work.
    • Updated several ssr:false rendering paths to consistently show an accessible loading fallback.
  • Tests
    • Expanded automated coverage for preamble behavior, abort signaling, and Server-Timing composition/order.
  • Documentation
    • Updated latency audit findings, latency remediation runbook, branch review ledger, and outstanding-issues tracking with revised guidance and execution status.

The 2026-07-28 latency audit was proposed as PR #1312 and closed unmerged
because its additive-index migration lacked synchronized schema/drift proof.
The closing note also said "compatible latency work is already on main" —
verification on 6021f6d found none of the six applied changes had landed, and
the audit document itself was absent, so its follow-ups were untracked.

This re-lands the free- and flag-gated work with tests, authors the operator
SQL without touching supabase/**, and files the remainder in the ledger.

Applied:
- Server-Timing preamble stages (auth/ratelimit/scope). /api/answer/stream —
  the route the UI actually calls — emitted no header at all; /api/search
  emitted none either. Only pre-header stages can appear on the stream route,
  since routing in-stream durations through the SSE contract would put
  instrumentation inside a governed clinical payload.
- L1-2: /api/answer resolves scope concurrently with the rate-limit RPC,
  aborts it on deny, and threads AbortSignal.any so a client disconnect
  cancels scope's paginated queries. The promise is settled, never floating.
- L1-1: the shared-cache-hit path no longer awaits setCachedAnswer before
  responding. forceRefresh stays — it is the mid-request staleness guard, now
  documented on the function.
- L2-6: three select("*") on document_table_facts narrowed to explicit
  projections, reusing tableFactDetailProjection. Keeps the generated
  search_tsv and owner_id off the wire; response shape unchanged.
- L2-9: /api/medications builds the governance map and index projection once
  instead of per anonymous request.
- L3-4/L3-5: ten ssr:false surfaces gain LoadingPanel fallbacks (role=status);
  Supabase preconnect/dns-prefetch added. Both ship ~zero bytes, so neither is
  gated behind #17, which governs payload decisions.

Not applied, deliberately: the six canary-gated findings, the seven #17-gated
findings, the four retired during verification, and the L2-3/L2-5 indexes —
authored as reviewed CREATE INDEX CONCURRENTLY statements in the operator
runbook with the apply/mirror/regenerate/register ordering spelled out.

L4-2 is retracted: the worker's triple image read is a deliberate peak-memory
trade-off already documented at worker/main.ts:866-869.

RAG impact: no retrieval behaviour change — the only src/lib/rag/** edits defer
a process-local cache write off the response path and add a doc comment; no
scoring, ordering, selection, alias, or citation logic is touched, and the
mid-request staleness guard is preserved.

Verification: verify:cheap exit 0 (418 files, 4244 passed / 4 skipped);
verify:pr-local exit 0 (same suite, production build compiled in 59s, client
bundle secret surface check passed, 36 golden cases / 21 suites validated).
tests/answer-route-preamble.test.ts fails against the pre-change serial shape.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01361jh3eYVjJCzXWjAhdZiF
@coderabbitai

coderabbitai Bot commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Warning

Review limit reached

You’ve reached a temporary PR review limit under our Fair Usage Limits Policy.

Your recent review volume is higher than typical usage, so adaptive limits are currently applied.

Next review available in: 37 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 @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: decf7af4-7816-4312-ae00-f77f919457bc

📥 Commits

Reviewing files that changed from the base of the PR and between 9f7a629 and ed41d6c.

📒 Files selected for processing (2)
  • docs/branch-review-ledger.md
  • docs/outstanding-issues.md
📝 Walkthrough

Walkthrough

The change corrects latency-audit evidence and remediation documentation, adds module-level caching for anonymous medication payload derivations, and adds tests covering answer-route admission, abort propagation, error handling, and Server-Timing composition.

Changes

Latency remediation and verification

Layer / File(s) Summary
Audit findings and measurement updates
docs/audit/latency-audit-2026-07-28.md
Corrects latency findings, delivery-path classifications, applied changes, measurement plans, and methodology limitations.
Operator rollout and cached payload derivation
docs/operator-apply-performance-latency-remediation.md, src/app/api/medications/route.ts
Documents concurrent index rollout and rollback sequencing, and memoizes anonymous medication index and governance projections at module scope.
Answer preamble and Server-Timing validation
tests/answer-route-preamble.test.ts, tests/server-timing.test.ts
Tests limiter-before-scope ordering, denial behavior, abort-signal forwarding, scope errors, and preamble timing composition.
Audit and review tracking records
docs/branch-review-ledger.md, docs/outstanding-issues.md
Adds review synchronization records and refreshes remediation queues and open-item statuses.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related PRs

Suggested reviewers: cursoragent

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the PR’s main purpose: implementing latency-audit findings with free and flag-gated work.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

Comment @coderabbitai help to get the list of available commands.

@supabase

supabase Bot commented Jul 29, 2026

Copy link
Copy Markdown

This pull request has been ignored for the connected project sjrfecxgysukkwxsowpy because there are no changes detected in supabase directory. You can change this behaviour in Project Integrations Settings ↗︎.


Preview Branches by Supabase.
Learn more about Supabase Branching ↗︎.

@BigSimmo
BigSimmo marked this pull request as ready for review July 29, 2026 04:08
@BigSimmo

Copy link
Copy Markdown
Owner Author

@cursoragent Inspect the PR and repository state, identify the root cause of every failing CI check, unresolved review comment, merge conflict, and related defect. Apply the smallest robust fix using existing project patterns and best practices, without unrelated changes. Run Bugbot and all relevant lint, type-check, test, build, and CI-equivalent commands, fix any issues found, then resolve only the review comments that are fully addressed. Summarise the root causes, changes made, validation results, and any remaining blockers.

@cursor

cursor Bot commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

PR #1377 is cleaned up and mergeable again. Tip: 9f21672c (auto-merge still armed; hosted CI re-running).

Root causes

Issue Cause
GitHub DIRTY / CONFLICTING Mostly staleness (branch behind main). Later, real conflicts when #1376 landed with overlapping latency work.
Codex P1 Scope overlapped with rate-limit; abort cannot undo in-flight PostgREST pages → denied callers still burned DB.
Codex P2 (docs) Stale audit measurement-plan claims after the L1-2 retraction.
Bugbot No cursor[bot] findings.
CI reds mid-babysit #095-style cancel churn from tip moves, not product failures. Prior tip d7aa4c6c had full PR required green.

Changes made

  • Synced main repeatedly (including after #1378).
  • Kept admission-before-scope + preamble tests; retracted overlap+abort.
  • Resolved Codex threads (0 unresolved).
  • Merged #1376 with real conflicts: kept this branch’s L1-2 REFUTED docs; took #1376 cache invalidation epochs, empty-scope Server-Timing, stream-scope signal.
  • Ledger rows appended for the babysit.

Validation

  • Local: verify:cheap earlier (423 files / 4278 passed); after reconcile: preamble + rag-cache-invalidation 7/7, check:rag:fixtures 36/21.
  • Hosted: green on d7aa4c6c / 4927410e; re-running on 9f21672c.

Remaining blocker

Wait for hosted CI on 9f21672c. Auto-merge should land when required checks go green, unless main moves again with another overlapping tip.

Open in Web Open in Cursor 

@BigSimmo

Copy link
Copy Markdown
Owner Author

@codex review PR

@BigSimmo

Copy link
Copy Markdown
Owner Author

@CodeRabbit Review PR

@coderabbitai

coderabbitai Bot commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

@BigSimmo: I’ll review PR #1377.

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@BigSimmo
BigSimmo enabled auto-merge (squash) July 29, 2026 04:09
Resolve GitHub DIRTY/CONFLICTING staleness; merge-tree was clean.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 1b11fe98c9

ℹ️ 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 thread src/app/api/answer/route.ts Outdated
claude and others added 4 commits July 29, 2026 04:20
Codex review raised this as P1 on PR #1377 and it is correct. The L1-2 change
started resolveSearchScope concurrently with the rate-limit RPC and aborted it
on deny, on the claim that a throttled caller "still costs nothing". That claim
is false.

resolveSearchScope only returns without touching the database when there are no
filters and no explicit document ids (search-scope.ts:242,253). With either
present it enters the paginated `documents` loop at :269 plus the nested label
loop. An AbortSignal cancels the client request; it does not un-execute a
statement Postgres has already begun. `filters` is caller-controlled, so a
throttled caller could keep spending database capacity while collecting 429s —
the opposite of what admission control is for, and the wrong direction against
capacity-review.md:106-113, which names Postgres CPU under concurrency the
first soft failure.

Scope now sits behind admission again. Two parts of the original change are
kept because they are independent of the overlap and unambiguously correct:

- `signal: request.signal` is threaded into resolveSearchScope, so a client
  disconnect finally cancels its paginated queries. search-scope.ts:200,328
  always supported .abortSignal(...); this route never passed one.
- the `scope` stage is still reported in Server-Timing.

tests/answer-route-preamble.test.ts is inverted to the guard the reviewer
asked for: no scope query may begin before the limiter admits, and a denied
request (sent with filters, the shape that reaches the paginated loop)
dispatches none at all. Both cases fail against the overlapping shape.

The audit's L1-2 section and ledger #99 record the refutation so a later
latency pass does not rediscover the overlap; re-attempting it requires a
non-database admission gate ahead of the durable limiter.

Verification: verify:cheap exit 0 on the merged tree (423 files, 4278 passed /
4 skipped).

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01361jh3eYVjJCzXWjAhdZiF
Keep the PR tip current after the CLAUDE.md orientation landing.
Capture the main sync, Codex P1 admission-order fix, and local verification
on tip e353e1d.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e52c25c79a

ℹ️ 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 thread docs/audit/latency-audit-2026-07-28.md Outdated
Comment thread docs/audit/latency-audit-2026-07-28.md Outdated
@BigSimmo

Copy link
Copy Markdown
Owner Author

@codex resolve actionable Codex review findings for this pull request and current head using the repository instructions. This is the pull request's single automatic repair pass: do not perform a fresh review, create new standalone findings, or request another review. Work only the existing unresolved Codex threads on the current head. Always fix P0 and P1 findings. For P2 and lower findings, fix only clear, scoped, low-risk issues; otherwise disposition them with a concise reason. After fixing or dispositioning a thread, reply in that thread with as the first line, followed by a concise summary; that marker authorizes the workflow to close that exact thread. If human input or new authorization is required, do not use the marker and leave the thread open with the blocker. Finish only after every actionable thread is fixed or dispositioned and closed, or explicitly left open for a human decision. Do not update the branch from main, address unrelated reviews, broaden scope, or create more than one scoped fix commit. Do not use external APIs, paid services, credentials, dependency changes, or broad refactors unless explicitly authorized. Add targeted tests where behavior changes and run the narrowest relevant validation.

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Keep them coming!

Reviewed commit: 11b88dd3b2

ℹ️ 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".

claude and others added 5 commits July 29, 2026 04:26
Both raised at P2 by Codex review on PR #1377 and both verified against the
code before acting.

1. The measurement plan still described L1-2's REFUTED contract — "scope must
   start before the limiter settles". The L1-2 section was corrected when the
   overlap was removed but this paragraph was missed, so the doc contradicted
   both the route and tests/answer-route-preamble.test.ts. Anyone generalising
   the harness under #98 would have enforced the opposite invariant and
   reintroduced database work for throttled requests. It now states the actual
   admission-cost contract: scope starts only after admission, and a denial
   dispatches zero scope queries.

2. The provider-free wall-clock step claimed the typeahead route plus demo mode
   times "auth + rate-limit + scope + RPC". False on both counts:
   src/app/api/search/universal/route.ts:137 returns on isDemoMode() ||
   isLocalNoAuthMode() BEFORE createAdminClient (:150), publicAccessContext
   (:151) and consumeSubjectApiRateLimit (:153), and resolveSearchScope is
   never called from that route. A demo-mode run would measure local fixture
   search and appear to clear an L1 finding it never exercised. The step now
   says to run it non-demo, and to time scope separately via /api/answer.

Docs only; no source or test behaviour changes.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01361jh3eYVjJCzXWjAhdZiF
Sync after #1378 landed; merge-tree was clean (GitHub DIRTY = staleness).
Keep admission-before-scope and the L1-2 refutation docs from this branch;
take #1376's deferred-cache invalidation epochs, empty-scope Server-Timing,
and stream-scope signal. Real content conflicts after #1376 landed on main.
@BigSimmo

Copy link
Copy Markdown
Owner Author

@cursoragent Inspect the PR and repository state, identify the root cause of every failing CI check, unresolved review comment, merge conflict, and related defect. Apply the smallest robust fix using existing project patterns and best practices, without unrelated changes. Run Bugbot and all relevant lint, type-check, test, build, and CI-equivalent commands, fix any issues found, then resolve only the review comments that are fully addressed. Summarise the root causes, changes made, validation results, and any remaining blockers.

@cursor

cursor Bot commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

PR #1377 is MERGEABLE again at tip 82d6b47c. Auto-merge is armed; hosted CI is re-running after the latest main sync.

Root causes

Issue Cause
GitHub DIRTY / BEHIND Mostly staleness as main moved (#1375, #1382, #1383, #1381).
Real conflict Only docs/outstanding-issues.md vs #1383 (new #106/#107 vs this branch’s corrected #098#105).
Failing CI earlier Cancel churn (#095) from tip moves — not product failures. Prior tips 9f21672c / 07c6aaeb / 4b4f9810 had full PR required green.
Review threads All already fixed/dispositioned (Codex P1 admission order + P2 audit docs).
Bugbot No new cursor[bot] findings.

Changes this pass

  • Merged #1375, #1382/#1383, #1381.
  • Resolved #1383 conflict: kept corrected latency #098#105 (L1-2 REFUTED / #104 correction); added #106/#107; next-id=108.
  • No product code changes required beyond the merge.
  • Ledger updated for the babysit (earlier commits); skipped a final ledger push after #1381 to avoid more CI cancel churn.

Validation

  • Local: preamble / server-timing / rag-cache-invalidation / calculator-scoring / private-access-routes green; typecheck exit 0; check:branch-review-ledger pass.
  • Hosted: PR required SUCCESS on 4b4f9810; CI queued/in progress on tip 82d6b47c.
  • Unresolved review threads: 0.

Remaining blocker

Wait for hosted CI on 82d6b47c. If it goes green and main stays still, armed squash auto-merge should land. If main advances again, another clean sync may be needed (unique tip delta vs main is now mostly docs + medications memo + preamble/Server-Timing tests — most latency product work already landed via #1376).

Open in Web Open in Cursor 

Sync after #1375 (Clinical Sky design tokens); merge-tree was clean.
Resolve outstanding-issues conflict from #1383: keep this branch's
corrected #98#105 latency ledger (including L1-2 REFUTED / #104
correction) and take main's new #106/#107 coverage recommendations.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b66d41d0a4

ℹ️ 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 thread docs/operator-apply-performance-latency-remediation.md Outdated
claude added 2 commits July 29, 2026 13:17
Apply step 5 and rollback phase A both described the required_indexes
change as an edit to `search_schema_health()` at supabase/schema.sql:3177.
schema.sql is a mirror, so that edit never reaches the hosted function.

search_schema_health() is redefined by `create or replace function` in
eleven migrations; 20260705180000_reconcile_search_health_indexes.sql is
the precedent -- it creates indexes and carries the updated
required_indexes array (:62) in the same migration.

As written the procedure left the three new indexes unmonitored on live,
and rollback was worse: phase A retracted only in the mirror, so phase B
would drop indexes the hosted function still required and turn the health
check red -- the exact failure phase A exists to prevent.

Apply now authors one migration carrying both the index creates and the
create-or-replace-function registration, mirrors both into schema.sql, and
deploys last. Phase A gets its own retraction migration plus matching
mirror and regenerated drift manifest.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01361jh3eYVjJCzXWjAhdZiF

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c11fdde972

ℹ️ 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 thread docs/outstanding-issues.md Outdated
claude added 2 commits July 29, 2026 13:24
Three places described the RAG-path index as canary-gated "until"/"unless"/
"or" fetchDocumentTitleAliasRows' unordered .limit(12) is made deterministic,
which reads as ordering lifting the gate.

It does not. An unordered LIMIT has no stable selection to preserve, so
imposing an order can pick a different twelve than the database happens to
return today -- that is an ordering behaviour change on a retrieval surface,
which AGENTS.md already requires its own live eval-canary pair for.
Sequencing the ordering fix first is still worthwhile, since an unordered
LIMIT feeding retrieval candidates is latent nondeterminism regardless of
this index, but it yields two canary-gated changes rather than one gate that
ordering unlocks.

The audit's L2-3 body already said this correctly; its opening clause and
the two outstanding-issues rows and the runbook bullet did not.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01361jh3eYVjJCzXWjAhdZiF

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 0ab16977d5

ℹ️ 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 thread docs/outstanding-issues.md Outdated
claude added 6 commits July 29, 2026 13:30
#103's outcome is that the migration chain and schema.sql agree on
document_table_facts trigram indexes, but the row offered
drift-allowlist.json as an alternative to mirroring. It is not one.

The allowlist's own header scopes it to "Known live-vs-schema.sql
divergence" -- it suppresses a live drift finding and cannot make the two
schema sources agree. A fresh `supabase db reset` still runs
20260714190000 and creates document_table_facts_text_trgm_idx while
schema.sql still omits it, so the divergence survives the allowlist
entirely.

There are exactly two routes: mirror it into schema.sql and regenerate the
manifest if retained, or drop it via a forward migration if the live scan
evidence shows it redundant.

Also records that no offline gate catches this -- the migration/schema.sql
parity test only asserts one migration's schema_drift_snapshot function
definition, not an index inventory.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01361jh3eYVjJCzXWjAhdZiF
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01361jh3eYVjJCzXWjAhdZiF
Merge 9f7a629 brought origin/main into this branch, and merge=union kept
both copies of four records this branch already carried from an earlier
main merge. Static PR checks went red on the ledger guard:

  - 4 exact duplicate review record(s) found at line(s) 1277, 1279, 1280, 1281.
  - 4 record(s) repeat the same ref/HEAD/scope (line pairs 1270 and 1277;
    1275 and 1279; 1272 and 1280; 1273 and 1281).

This is the #88 watch condition exactly. All four pairs are byte-identical,
so only the later copies are removed -- the one mutation the append-only
contract permits. No record is edited and none is lost: 4 deletions, 0
additions, and the unique-row sets before and after are identical.

The rows belong to three other branches (clinical-design-system-update-e34ca9,
test-coverage-analysis-2vcd8a, document-reader-condensed-view), not to this
one.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01361jh3eYVjJCzXWjAhdZiF
Resolved docs/outstanding-issues.md by keeping this branch's updated #98,
#102, #104 and #105 entries (the ones this PR rewrites) and adding main's new
#108 and #109 rows; the rest of the conflict was prettier re-padding column
widths after a summary-cell length change. next-id stays at 110.

Removed four exact-duplicate branch-review-ledger rows produced by the union
merge driver keeping both copies of rows present on this branch and on main -
the duplication risk tracked as #88.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d66138c4de

ℹ️ 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 thread docs/outstanding-issues.md Outdated
claude added 2 commits July 29, 2026 14:03
The #98 row still named test-cache-path.mjs and check-rag-fixtures.mjs as
the fixtures to build the round-trip counting harness over. Neither can
count a RAG request: the first computes Vitest/TypeScript cache paths, the
second only validates fixture manifests. A harness wired to them would have
pinned nothing.

The audit document already carried this retraction (:358); this row did not,
so the stale references survived in the one place an implementer would
actually read them.

Now names scripts/eval-rag-offline.mjs, scripts/test-rag-offline.mjs,
scripts/rag-offline-contract.mjs and scripts/fixtures/rag-offline-contract-tests.json,
all four verified present, and records the correction inline so it is not
reintroduced a third time.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01361jh3eYVjJCzXWjAhdZiF
@github-actions

Copy link
Copy Markdown
Contributor

CI triage

CI failed on this PR. Automated classification of the 2 failed job(s):

  • Production UIneeds investigation: inspect the failing step and uploaded diagnostics; rerun only after classifying the cause.
  • PR requiredneeds investigation: inspect the failing step and uploaded diagnostics; rerun only after classifying the cause.

Compared with main CI run #6558 (success).

Classification is evidence routing, not permission to ignore a failure. Exact quarantined Playwright identities remain governed by the flake ledger.

Copy link
Copy Markdown
Owner Author

Production UI failure on 14e0232 — diagnosed, not caused by this PR

Three failures, 325 passed. I am not pushing a fix, because this PR provably cannot cause any of them. Evidence rather than assertion:

1. The entire diff against origin/main is seven files.

docs/audit/latency-audit-2026-07-28.md
docs/branch-review-ledger.md
docs/operator-apply-performance-latency-remediation.md
docs/outstanding-issues.md
src/app/api/medications/route.ts     ← the only source file
tests/answer-route-preamble.test.ts
tests/server-timing.test.ts

Every other source file is byte-identical to main — the rest of the latency work already landed there via #1376. So the only behaviour this PR can change is /api/medications.

2. Neither failing test can reach it. git diff origin/main...HEAD -- tests/ui-phone-scroll.spec.ts tests/ui-tools.spec.ts is empty — both specs are byte-identical to main. ui-phone-scroll.spec.ts contains no medication reference at all, and ui-tools.spec.ts:127 network-stubs the endpoint:

await page.route(/\/api\/medications(?:\?.*)?$/, async (route) => {  })

so the handler I changed never executes during that test.

3. The same code passed this job 67 minutes earlier. #1389's own Production UI ran success (12:55:36 → 13:11:16). It is the PR that introduced both these tests and the implementation they exercise (use-hide-on-scroll.ts, master-search-header.tsx, +232 lines of ui-phone-scroll.spec.ts). Between that run and this one, main gained only #1384, #1386 and #1387 — all docs/ledger.

On the triage bot's comparison

Compared with main CI run #6558 (success).

That run skipped Production UI — along with Build, Unit coverage, Migration replay and Advisory UI. It was a docs-only push (f505ab4), so risk-scoped CI ran only Change scope and Static PR checks. The comparison is green because the job never executed, not because these tests pass on main. Worth knowing before it is used as a baseline again.

What the failures actually are

# Test Failure
1, 2 ui-phone-scroll.spec.ts:416 (browser + PWA) after dragScrollBy(page, 720, 24), universal-header-collapse never gains data-scroll-hidden="true"24 × locator resolved, attribute stays null for the full 10 s
3 ui-tools.spec.ts:945 /favourites?q=lithium at tablet getByTestId('search-query-ribbon') resolved to 2 elements, one under mobile-composer-reserve-pad

Failure 3 matches open issue #093 exactly — "Next streaming S: clone causes Playwright strict-mode violations under CI load; a getByTestId that is unique locally resolves to 2 elements in CI."

Failures 1–2 are the interesting ones. The element resolves consistently, so it is not a duplicate or a missing-node problem; the scroll-hide simply never fires. The most likely reading is that the drag gesture produced no actual scroll movement under load, so the listener never ran — but I want to be careful not to over-claim: on the evidence available these are new tests that have run green exactly once and red exactly once, which is not enough to call either way.

What I am not doing, and why

  • Not quarantining. docs/testing.md:59 requires reproducing three times on the same SHA, an owner, a repro command and an expiry, and says explicitly to "treat repeatable failures as regressions". A one-off red does not qualify, and the flake ledger governs these identities.
  • Not fixing the phone-chrome behaviour. It is main's code, outside this PR's scope, and docs/search-chrome-behaviour.md governs that surface — changing it needs that document read first and verify:phone-chrome run, not an opportunistic patch from a latency PR.
  • Not re-running the job unilaterally. Hosted CI is confirmation-required here, and the triage bot's own instruction is to rerun only after classifying the cause. The cause is now classified; a re-run is the natural next step and I will do it on request.

The substantive point for the repo, independent of this PR: #1389 landed a phone-chrome change whose tests have been executed exactly twice, and main has not run them since the merge because every push to main afterwards was docs-only. If these are flaky, they will keep reddening unrelated PRs; if they are real, main is currently broken on the document-detail phone surface. Either way it wants three same-SHA reproductions to settle, which is a decision for the repo owner rather than something to resolve from inside this PR.


Generated by Claude Code

Copy link
Copy Markdown
Owner Author

Production UI red on 14e0232a — diagnosed as flake, not a regression from this PR

Recording the evidence rather than asserting it, because this is not the #095 cancellation false-red: the job ran for 12m53s and concluded failure with three genuinely failed tests, and classify-playwright-failures.mjs marked all three "needs investigation" rather than recognising them.

3 failed
  ui-phone-scroll.spec.ts:416 › document detail header overlay and footer follow browser document scrolling together
  ui-phone-scroll.spec.ts:416 › document detail header overlay and footer follow standalone PWA main scrolling together
  ui-tools.spec.ts:945 › search route keeps the correct composer at tablet width on /favourites?q=lithium&focus=1&run=1
325 passed (10.5m)

Why this is not attributable to this PR. The diff against main is four docs files, src/app/api/medications/route.ts, and two unit test files. It touches no component, no route under (search-app), no CSS and no phone-chrome code. None of the three failing journeys can reach the medications memo.

The controlled comparison. PR #1374 ran the identical Chromium production suite at 13:52 on a base differing from this one only by #1387 — which is docs-only — and passed:

Production UI: success, Chromium production journeys: success (14m24s), all 12 jobs green, PR required: success

So the suite itself is healthy on this base. Two branches, same base, same suite: the one with no UI changes failed and the one with UI changes passed. That is the shape of a flake, not a regression.

The failure signature matches a tracked issue. The ui-tools failure is a strict-mode violation where getByTestId('search-query-ribbon') resolved to two identical elements — same aria-label, same data-status, one of them nested under mobile-composer-reserve-pad:

strict mode violation: getByTestId('search-query-ribbon') resolved to 2 elements
  1) <section … aria-label="Search results for lithium"> aka getByTestId('mobile-composer-reserve-pad').getByTestId('search-query-ribbon')
  2) <section … aria-label="Search results for lithium"> aka getByTestId('search-query-ribbon').nth(1)

A transient duplicate of the same subtree is exactly ledger #093"Next streaming S: clone causes Playwright strict-mode violations under CI load". Worth noting the classifier did not catch it, which suggests #093's recognised signature list needs this locator added; I have not changed the classifier here since that is outside this PR's scope.

Next step: this branch needs a main sync regardless, which re-runs CI on a fresh head. I am treating that as the re-run. If the same three fail again on a clean head I will stop and investigate properly rather than re-running a third time — a red required check that keeps being re-rolled is precisely the habit #095 warns teaches people to discount red.


Generated by Claude Code

@BigSimmo
BigSimmo merged commit 64f4cb6 into main Jul 29, 2026
19 checks passed
@BigSimmo
BigSimmo deleted the claude/latency-findings-impl-s8g01v branch July 29, 2026 15:34
BigSimmo pushed a commit that referenced this pull request Jul 29, 2026
Resolves the `docs/outstanding-issues.md` conflict. Two independent causes:

1. Rows #98#105 diverged. `main` carries the newer text (PR #1377 landed
   "Done 2026-07-29" outcomes and rewrote several summaries); this branch
   carried the pre-#1377 wording. Took `main`'s rows wholesale — the branch
   made no edits to that range, it simply forked before them.

2. `#108`/`#109` were allocated twice. `main` used them for the verified-landed
   branch deletions and the shallow-clone finding; this branch had used the same
   two ids for the design-system token manifest and the `ui-overlap` flake. The
   `issues:next-id` marker has no concurrency protection, which is exactly the
   failure the branch's own PR notes predicted. Renumbered this branch's two rows
   to #110/#111 and bumped the marker to 112. No content was dropped from either
   side; all 111 rows are unique.

Also records #111 as done, since this branch is what fixes it: the ui-overlap
inset measurement now retries inside `toPass` with the 2px symmetry tolerance
and the assertions unchanged. Leaving it open with a "Next: apply the retry
shape" action would have re-queued work this PR already did.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants