Launch-readiness: privacy notice + /privacy page, health SLO probe, deps, runbooks#513
Conversation
…notice APP-5 collection/overseas-disclosure notice at the answer composer plus a /privacy transparency page (closes the PIA-5 content gap). The OpenAI DPA/ZDR remains the operator/legal PIA-1 step. Copy centralized in ui-copy.ts; sitemap regenerated. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ep /api/health probe New src/lib/observability/answer-slo.ts aggregates rag_queries metadata (counts only) behind the existing secret-gated deep probe; kept out of checks so a telemetry failure never flips liveness to 503. Surfaces the silent-degradation guard from docs/observability-slos.md. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…guard (PIA-4) Adds migration 20260708120000 to the preview-branch cron-guard assertion loop so the misses-retention purge is held to the same discipline as the rag_queries/retrieval-logs purges. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
supabase-js 2.110.2, openai 6.46.0, lucide-react 1.24.0, prettier 3.9.5, tsx 4.23.0, postcss 8.5.16, fast-check 4.9.0, @types/node 24.13.3, eslint 9.39.5. Held majors (eslint 10 / typescript 7 / @types/node 26). Verified: lint, typecheck, format, 1652 unit tests, npm audit 0 vulns. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…-order launch-operator-runbook.md sequences the operator-gated launch steps (migrations, release gate, staging, Railway deploy, conn cap, worker) with approval markers. forward-codify-retrieval-rpcs-workorder.md locks the 2026-07-12 live RPC fingerprints + the byte-faithful codification procedure (drift backlog #0), blocked on Docker + a quiescent live DB. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe changes add a privacy page and composer disclosure, expose Answer SLO telemetry through deep health checks, record hybrid RPC errors, document launch and retrieval codification procedures, enforce a workflow permission, and expand Supabase retention replay coverage. ChangesPrivacy transparency
Answer SLO health telemetry
Launch operations documentation
Codex workflow permission guard
Schema retention replay coverage
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant HealthRoute
participant Supabase
participant AnswerSloSnapshot
HealthRoute->>Supabase: Probe deep health
HealthRoute->>AnswerSloSnapshot: Request SLO snapshot when healthy
AnswerSloSnapshot->>Supabase: Query rag_queries counts
AnswerSloSnapshot-->>HealthRoute: Return counts and rates
HealthRoute-->>HealthRoute: Include optional slo response field
Possibly related PRs
✅ Pre-merge checks override appliedThe pre-merge checks have been overridden successfully. You can now proceed with the merge. Overridden by ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (9 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ffa8523c7e
ℹ️ 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".
|
@codex resolve all issues and CI and enable merge please $comment |
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (4)
src/app/api/health/route.ts (1)
43-51: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winSwallowed SLO fetch failures leave no diagnostic trail.
catch { slo = null; }correctly keeps telemetry failures out of the readiness gate, but drops the error entirely. For a probe whose entire purpose is surfacing silent degradation, a persistently-failing SLO fetch (e.g. bad RPC/query shape, permission issue) would itself degrade silently with no log line to explain the missingslofield.🔎 Suggested minimal logging
try { slo = await answerSloSnapshot(admin); - } catch { + } catch (error) { + console.error("answer SLO snapshot failed", error); slo = null; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/api/health/route.ts` around lines 43 - 51, Update the SLO fetch catch block in the health route’s health.ok branch to log the caught error with appropriate diagnostic context before setting slo to null. Preserve the existing behavior that SLO fetch failures do not affect readiness or return a 503.tests/health-route.test.ts (1)
55-65: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the deep-health SLO happy path
This file only exercises the unauthorizeddeep=1branch. Add one test for an authorized deep probe that returns a populatedslo, and another foranswerSloSnapshotthrowing while the response still stays200withsloomitted.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/health-route.test.ts` around lines 55 - 65, Add tests alongside the existing deep-health test covering the authorized deep probe: mock a valid token and verify a successful response includes a populated slo snapshot, then mock answerSloSnapshot to throw and verify the response remains 200 with slo omitted. Reuse the existing healthRequest, mockEnv, GET import, and payload helpers, and preserve the current unauthorized coverage.src/components/clinical-dashboard/answer-status.tsx (1)
95-98: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winDrop
aria-labelon the notice<p>.
aria-labelon a plain<p>overrides its accessible name/content for AT, which can suppress the linear announcement of the actual notice text and link. Since the text is already visible and meaningful, an explicit label is unnecessary here.♿ Suggested fix
- <p - className="text-center text-2xs leading-4 text-[color:var(--text-muted)]" - aria-label={privacyCopy.noticeAriaLabel} - > + <p className="text-center text-2xs leading-4 text-[color:var(--text-muted)]"> {privacyCopy.composerNotice}{" "}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/clinical-dashboard/answer-status.tsx` around lines 95 - 98, Remove the aria-label prop from the notice paragraph in the answer-status component, leaving the visible notice text and link content unchanged.src/app/privacy/page.tsx (1)
116-126: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCentralize this warning-card copy in
ui-copy.tstoo.This inline PHI-reminder text is hardcoded here, while the equivalent composer notice is centralized via
privacyCopy. Per the stated design goal of centralizing privacy copy, having two independently-maintained strings for the same compliance message risks drift.♻️ Suggested consolidation
export const privacyCopy = { composerNotice: "Answers are generated by OpenAI in the US — don't enter identifiable patient details.", + pageWarningNotice: + "Do not enter identifiable patient details (names, dates of birth, record numbers) into your questions. Your question text is sent to OpenAI in the United States to generate an answer.", composerLinkLabel: "Privacy & data handling",- <p className="min-w-0 text-sm leading-6 text-[color:var(--text-heading)]"> - Do not enter identifiable patient details (names, dates of birth, record numbers) into your questions. - Your question text is sent to OpenAI in the United States to generate an answer. - </p> + <p className="min-w-0 text-sm leading-6 text-[color:var(--text-heading)]"> + {privacyCopy.pageWarningNotice} + </p>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/privacy/page.tsx` around lines 116 - 126, Move the inline PHI reminder text in the privacy page warning card into the existing privacyCopy definition in ui-copy.ts, then render that centralized value in the paragraph while preserving the current wording and formatting. Reuse the existing privacyCopy symbol used by the composer notice rather than introducing a second copy constant.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/forward-codify-retrieval-rpcs-workorder.md`:
- Around line 45-62: Update the retrieval RPC capture and verification procedure
to include run_visual_eval_case, run_all_visual_eval_cases, and
repair_enrichment_quality_batch alongside get_visual_evidence_cards in the
function query, fingerprint table, recapture set, and all parity checks. Keep
these live-only functions in the codification scope and ensure each is validated
and emitted as CREATE OR REPLACE.
- Line 9: Update the capture and validation steps in the workorder to use one
identical canonical function-definition representation before hashing, including
the referenced sections at lines 42-43 and 65-68. Ensure both sides apply the
same whitespace normalization and MD5 process, and revise the “byte-identically”
wording to match the normalized comparison.
- Around line 55-64: Update the retrieval-RPC work order to explicitly capture
each function’s ACLs using proacl or routine privileges, alongside
pg_get_functiondef. Require replay/application of the captured grants in the
migration and schema reconciliation, and compare the resulting ACLs, with
particular coverage for the live-only security-definer RPCs.
In `@docs/launch-operator-runbook.md`:
- Around line 66-71: Update the “1b. Drift-codify apply” section to keep the
step blocked until the migration artifact is committed and execution-time live
fingerprint recapture is completed. Remove the unresolved <drift-codify-forward>
reference, document the exact committed artifact, and require verification that
recaptured fingerprints match before applying; abort on mismatch.
- Around line 50-55: Update the runbook’s R17 manual CREATE UNIQUE INDEX
CONCURRENTLY path to either remove it or explicitly identify it as an approved
exception to the live-change guardrail. If retained, require recording the
migration/history entry and reconciling schema.sql so the manual change cannot
leave untracked drift; apply the same guidance to the corresponding instructions
at the other referenced section.
- Around line 140-141: Update the Worker redeploy instruction to reference
migration 20260708130000 directly without labeling it “step 1b,” avoiding
confusion with the separate drift-codify procedure described earlier in the
runbook.
- Line 18: Update the fenced code block in the launch operator runbook to
include an appropriate language identifier, such as text, immediately after the
opening fence while preserving its contents.
In `@src/app/privacy/page.tsx`:
- Around line 1-25: Update the privacy page type declaration by importing
ReactNode as a type from react and changing Section.body from React.ReactNode to
ReactNode; leave the existing Section shape and page behavior unchanged.
In `@src/lib/observability/answer-slo.ts`:
- Around line 50-58: Update the rag_queries persistence flow used by
answerSloSnapshot so hybrid RPC failures are written into the existing metadata
field under hybrid_rpc_errors before the SLO query runs. Reuse the same failure
data already captured by search telemetry, preserve fallback_reason persistence,
and ensure the metadata value is available to
base().not("metadata->hybrid_rpc_errors", "is", null).
---
Nitpick comments:
In `@src/app/api/health/route.ts`:
- Around line 43-51: Update the SLO fetch catch block in the health route’s
health.ok branch to log the caught error with appropriate diagnostic context
before setting slo to null. Preserve the existing behavior that SLO fetch
failures do not affect readiness or return a 503.
In `@src/app/privacy/page.tsx`:
- Around line 116-126: Move the inline PHI reminder text in the privacy page
warning card into the existing privacyCopy definition in ui-copy.ts, then render
that centralized value in the paragraph while preserving the current wording and
formatting. Reuse the existing privacyCopy symbol used by the composer notice
rather than introducing a second copy constant.
In `@src/components/clinical-dashboard/answer-status.tsx`:
- Around line 95-98: Remove the aria-label prop from the notice paragraph in the
answer-status component, leaving the visible notice text and link content
unchanged.
In `@tests/health-route.test.ts`:
- Around line 55-65: Add tests alongside the existing deep-health test covering
the authorized deep probe: mock a valid token and verify a successful response
includes a populated slo snapshot, then mock answerSloSnapshot to throw and
verify the response remains 200 with slo omitted. Reuse the existing
healthRequest, mockEnv, GET import, and payload helpers, and preserve the
current unauthorized coverage.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 248211e5-aa69-444c-9e89-8e2318b73347
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (11)
docs/forward-codify-retrieval-rpcs-workorder.mddocs/launch-operator-runbook.mddocs/site-map.mdsrc/app/api/health/route.tssrc/app/privacy/page.tsxsrc/components/clinical-dashboard/answer-status.tsxsrc/lib/observability/answer-slo.tssrc/lib/ui-copy.tstests/answer-slo.test.tstests/health-route.test.tstests/supabase-schema.test.ts
|
Summary
Git / PR
Testing
|
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 4 file(s) based on 9 unresolved review comments. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 4 file(s) based on 9 unresolved review comments. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
|
@copilot resolve the merge conflicts in this pull request |
|
@copilot resolve the merge conflicts in this pull request |
Head branch was pushed to by a user without write access
|
@copilot resolve the merge conflicts on this branch. |
Head branch was pushed to by a user without write access
CodeRabbit's autofix (7a3b9e7) edited this doc to address its review comments but left it un-prettier-formatted, which failed the static-pr format:check gate. Reformat only; content unchanged. lint + typecheck + targeted format all green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3f7d6d76f5
ℹ️ 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".
| */ | ||
| export async function answerSloSnapshot(client: SloProbeClient, windowMinutes = 60): Promise<AnswerSloSnapshot> { | ||
| const sinceIso = new Date(Date.now() - windowMinutes * 60_000).toISOString(); | ||
| const base = () => client.from("rag_queries").select("*", { count: "exact", head: true }).gt("created_at", sinceIso); |
There was a problem hiding this comment.
Filter search telemetry out of answer SLO counts
When users run /api/search, logSearchObservation writes rows into the same rag_queries table with answer: null, model: "search", and metadata.event_type = "private_search" (src/app/api/search/route.ts). Because this base query only filters by created_at, the new answer-pipeline SLO denominator includes those search-only rows while the degraded and hybrid-error numerators are answer-specific, so search-heavy periods can make /api/health?deep=1 report artificially low rates and miss the outage this probe is meant to surface. Add an answer-row predicate such as answer IS NOT NULL/model != 'search' (applied consistently to all three counts).
Useful? React with 👍 / 👎.
…cile Resolves the leftover merge-conflict markers in §11 (kept main's PIA-2 "enforced in code" language alongside the PIA-3 durable-log scoping). Codex (P2): rag_response_cache TTL gates reads only — sharedCacheSelector filters on expires_at, replaceSharedCacheRow overwrites only the same key, and there is no purge cron (per §8). Reword the residual/data-table/APP-11/Overall entries so they no longer describe a read-TTL as a retention bound; the row persists at rest until a same-key overwrite or manual flush. CodeRabbit (Major): finish the APP 1/5 reconciliation the autofix started — align the §1 register (PIA-1, PIA-5) and the PIA-5 detail (add a Progress note) with the §9 matrix, so the whole doc reflects the live privacy page + composer notice (PR #513). Residual is the APP 8 DPA/ZDR basis plus broader data-handling docs. Docs only; no behaviour change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…log (#535) * docs(privacy): scope PIA-3 closure to the durable rag_queries.answer log Review fix: the earlier PIA-3 wording overclaimed that generated answer text is "dropped at rest by default." The durable rag_queries.answer log is gated by RAG_PERSIST_ANSWER_TEXT, but rag_response_cache.payload still holds the full answer (owner-scoped, ~5-min TTL). Tighten the register/APP-11/recommendation wording to the durable log and add an explicit "Residual (scoped out)" note for the short-lived response cache, which is intentionally not gated (nulling its payload would defeat caching). No behaviour change; documentation accuracy only. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix: apply CodeRabbit auto-fixes Fixed 1 file(s) based on 1 unresolved review comment. Co-authored-by: CodeRabbit <noreply@coderabbit.ai> * fix: resolve merge conflict in docs/privacy-impact-assessment.md Combine updated PIA-2 HMAC wording from main (#532) with this branch's scoped PIA-3 closure wording (durable rag_queries.answer log rather than answer text broadly). * docs(privacy): correct cache-retention wording + finish APP 1/5 reconcile Resolves the leftover merge-conflict markers in §11 (kept main's PIA-2 "enforced in code" language alongside the PIA-3 durable-log scoping). Codex (P2): rag_response_cache TTL gates reads only — sharedCacheSelector filters on expires_at, replaceSharedCacheRow overwrites only the same key, and there is no purge cron (per §8). Reword the residual/data-table/APP-11/Overall entries so they no longer describe a read-TTL as a retention bound; the row persists at rest until a same-key overwrite or manual flush. CodeRabbit (Major): finish the APP 1/5 reconciliation the autofix started — align the §1 register (PIA-1, PIA-5) and the PIA-5 detail (add a Progress note) with the §9 matrix, so the whole doc reflects the live privacy page + composer notice (PR #513). Residual is the APP 8 DPA/ZDR basis plus broader data-handling docs. Docs only; no behaviour change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: CodeRabbit <noreply@coderabbit.ai> Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
…#536) * feat(observability): add retrieval cache hit-rate counter to /api/health deep probe Completes runbook item 3.2 (observability/alerts/rollback). The answer-SLO snapshot from #513 already exposes the hybrid_rpc_errors and degraded/source-only rates; the one remaining silent-degradation counter — cache hit-rate — is now instrumented in-process at the retrieval hot path and surfaced on the deep probe. - New src/lib/observability/cache-metrics.ts: cumulative process-start hit/miss counters (Prometheus-style; a scraper derives a windowed rate from deltas). - getCachedSearch records exactly one hit/miss per cache-enabled lookup (skipCache / TTL-or-size-0 records neither). - Health route exposes a `cache` block for any authorized deep probe (in-process, so it works in demo mode too); like `slo`, it never flips 200/503 liveness. - Refresh docs/observability-slos.md: the /api/health extension is now shipped (both `slo` and `cache`); only alert-channel wiring + host metrics remain. Tests: cache-metrics unit test + a health-route deep-probe case; full vitest suite green (1682 passed, 1 skipped). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(observability): count shared-cache hits in the cache hit-rate counter Addresses Codex P2 on #536. The counter was incremented inside getCachedSearch, which only sees the process-local cache — a local miss followed by a shared (rag_response_cache) hit was recorded as a miss, so a cold process or a multi-instance deployment with a warm shared cache would show a deflated hit-rate and could trip a false degradation alert. Move the recording to the two-layer cache orchestration in searchChunksWithTelemetry, recording exactly once with full knowledge of both layers via a pure classifySearchCacheOutcome(): a request served by either the local or the shared cache is a hit; a miss is counted only when a layer was consulted and missed; disabled/skipped lookups (shared returns null) record neither. getCachedSearch reverts to its original form. Tests: classifySearchCacheOutcome unit tests covering the local-miss/shared-hit regression, plus the disabled and both-miss cases. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix: format health/ready route.ts with Prettier --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Summary
Launch-readiness "next steps" — verified, additive changes toward production.
/privacydata-handling page + an APP-5 collection notice & PHI reminder at the answer composer (copy centralized inui-copy.ts). Closes the PIA-5 content gap; the OpenAI DPA/ZDR remains the operator/legal PIA-1 step./api/health?deep=1now exposeshybrid_rpc_errors+ degraded SLO rates — aggregate counts fromrag_queries, behind the existing deep-probe secret, kept out ofchecksso a telemetry failure never flips liveness to 503.rag_query_missesretention pg_cron preview-guard.Verification
lint / typecheck / prettier (repo-wide) all green; unit tests 1652 passed (+4 new);
npm audit --omit=dev --audit-level=high0 vulnerabilities. Security-reviewed: no HIGH/MEDIUM findings.Local
verify:cheapis red only on the pre-existingcheck:type-scale --strict(arbitrary rem sizes inmode-home-template.tsxfrom #496 — not touched here; tracked separately).Clinical governance / privacy preflight
docs/privacy-impact-assessment.md; it does not alter data flows, retention, or storage.rag_queries.metadata(no raw query text / PII) behind the existing secret gate.Not in this PR (staged, operator-gated)
Forward-codify retrieval RPC drift (1.2) is blocked on Docker (byte-faithful replay) + a quiescent live DB — see
docs/forward-codify-retrieval-rpcs-workorder.md. Live migration applies, staging, and the Railway deploy remain operator steps perdocs/launch-operator-runbook.md.🤖 Generated with Claude Code