Wire the developer hub environment strip to real environment facts - #2495
Conversation
…rant timeout Both shell out to a Node child process. Measured on this worktree: the child finishes in ~7 s standalone and the tests take ~9 s and ~10 s, so the global 30 s budget looks generous in isolation. It is not under the full run — the node suite is CPU-bound on two workers across 900+ files, and on 2026-09-01 the parity test exceeded 30 000 ms inside `npm run test` and failed the gate for a diff that touched nothing under `.design-sync/**`. Deliberately not quarantined: it did not fail an assertion, and a wrong time budget is a different defect from an unstable test. `tests/flake-ledger.json` is untouched, and the repo's three-reproductions rule does not apply here. Proven rather than assumed: setting the same option to 1 ms makes the test fail with "Test timed out in 1ms", so the option is read rather than silently ignored. Committed with SKIP_DOCS_SYNC_HOOK=1 because the pre-commit generator refuses a partially staged tree; the sibling commit that follows runs it in full on the final tree, which is what gets pushed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…acts The strip shipped in Phase 1 with `demoMode`, `documentCount` and `email` all `null`, rendering "environment unknown · document count unavailable · account unknown". Phase 2's design deferred them to whichever phase owned each value's data source and reserved any Supabase read from the hub for its own approval; that approval was given on 2026-09-01. `resolveHubEnvironmentFacts` gathers all three in one Supabase round trip. Two of its rules are load-bearing, and each is pinned by a test proven to fail without it: - The count goes through the cookie-bound user client, so row-level security (`documents owner read`, `owner_id = auth.uid()`) scopes it to the caller's own documents. The service-role admin client bypasses RLS and would report every owner's total to whoever was signed in; a source assertion fails if that import ever appears. - Every failure path returns `null`, never `0`, and an unauthenticated request skips the query rather than reporting the zero rows RLS would correctly hand it. Zero is a true answer here, so a failed read must not be able to impersonate an empty corpus. Reading data makes the page an async Server Component, so the dom tests and the shared in-page-nav route table now await their fixtures. `next build` classifies `/mockups/development` as dynamic, which it already was via the layout's `headers()` call, so nothing regressed there. The Phase 2 handoff document is corrected here rather than left claiming the strip is unwired. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Team Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_a6396642-a586-4d14-a4a2-c16913444c67) |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fdb93a1e9a
ℹ️ 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".
CI triageCI failed on this PR. Automated classification of the 2 failed job(s):
Compared with main CI run #14661 (success). That run's conclusion is an aggregate and did not exercise Classification is evidence routing, not permission to ignore a failure. Exact quarantined Playwright identities remain governed by the flake ledger. |
…d rejects Raised in review of #2495. The helper handled a returned `{ error }` but not a rejected promise, and the installed client rejects rather than resolves when a request is aborted or exhausts its network retries. An unhandled rejection there fails the whole developer hub page instead of degrading one line of it — during exactly the Supabase outage that makes the page worth opening, and contrary to the module's own stated contract. Both awaits are now inside one guard, not just the count: an auth read can fail the same way. `demoMode` is resolved before either of them and survives both, because it never depended on the network, so an outage cannot make the strip claim it is unable to tell demo data from live. Proven, not assumed: two tests whose mocked `select` and `getUser` reject both fail without the guard and pass with it, and they assert the whole returned shape rather than merely that nothing threw. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…' into claude/dev-hub-environment-strip
* test(design-sync): give the two subprocess contract tests a load-tolerant timeout Both shell out to a Node child process. Measured on this worktree: the child finishes in ~7 s standalone and the tests take ~9 s and ~10 s, so the global 30 s budget looks generous in isolation. It is not under the full run — the node suite is CPU-bound on two workers across 900+ files, and on 2026-09-01 the parity test exceeded 30 000 ms inside `npm run test` and failed the gate for a diff that touched nothing under `.design-sync/**`. Deliberately not quarantined: it did not fail an assertion, and a wrong time budget is a different defect from an unstable test. `tests/flake-ledger.json` is untouched, and the repo's three-reproductions rule does not apply here. Proven rather than assumed: setting the same option to 1 ms makes the test fail with "Test timed out in 1ms", so the option is read rather than silently ignored. Committed with SKIP_DOCS_SYNC_HOOK=1 because the pre-commit generator refuses a partially staged tree; the sibling commit that follows runs it in full on the final tree, which is what gets pushed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * feat(developer-hub): wire the environment strip to real environment facts The strip shipped in Phase 1 with `demoMode`, `documentCount` and `email` all `null`, rendering "environment unknown · document count unavailable · account unknown". Phase 2's design deferred them to whichever phase owned each value's data source and reserved any Supabase read from the hub for its own approval; that approval was given on 2026-09-01. `resolveHubEnvironmentFacts` gathers all three in one Supabase round trip. Two of its rules are load-bearing, and each is pinned by a test proven to fail without it: - The count goes through the cookie-bound user client, so row-level security (`documents owner read`, `owner_id = auth.uid()`) scopes it to the caller's own documents. The service-role admin client bypasses RLS and would report every owner's total to whoever was signed in; a source assertion fails if that import ever appears. - Every failure path returns `null`, never `0`, and an unauthenticated request skips the query rather than reporting the zero rows RLS would correctly hand it. Zero is a true answer here, so a failed read must not be able to impersonate an empty corpus. Reading data makes the page an async Server Component, so the dom tests and the shared in-page-nav route table now await their fixtures. `next build` classifies `/mockups/development` as dynamic, which it already was via the layout's `headers()` call, so nothing regressed there. The Phase 2 handoff document is corrected here rather than left claiming the strip is unwired. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(developer-hub): degrade the environment strip when a Supabase read rejects Raised in review of #2495. The helper handled a returned `{ error }` but not a rejected promise, and the installed client rejects rather than resolves when a request is aborted or exhausts its network retries. An unhandled rejection there fails the whole developer hub page instead of degrading one line of it — during exactly the Supabase outage that makes the page worth opening, and contrary to the module's own stated contract. Both awaits are now inside one guard, not just the count: an auth read can fail the same way. `demoMode` is resolved before either of them and survives both, because it never depended on the network, so an outage cannot make the strip claim it is unable to tell demo data from live. Proven, not assumed: two tests whose mocked `select` and `getUser` reject both fail without the guard and pass with it, and they assert the whole returned shape rather than merely that nothing threw. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * feat(developer-hub): surface clinical answer failures the ledger already records The task ledger already records which clinical questions the answer engine gets wrong -- today, three open items naming five questions between them, including antipsychotic metabolic monitoring returning a placeholder instead of an answer. Those facts sit unread among eighty-one open items. This puts them on the hub. `resolveClinicalAnswerFailures` intersects two records the repository already maintains: open ledger items, and the eval case list that names each clinical question. Derived rather than curated on purpose -- a hand-kept list of ledger ids is exact the day it is written and wrong a month later, with nothing to catch the drift. Matching is whole-token, and that is the load-bearing part rather than a detail. Case ids nest: `discharge-documentation` and `patient-safety-plan` are both real cases AND substrings of other real ones, so a plain `includes` reported five questions where three were real -- two clinical questions on screen that nothing had reported as broken. Proven by mutation: reverting the matcher to `includes` turns both the synthetic nesting test and the real-data property test red. The panel names itself for its evidence ("Open problems recorded against a named clinical question"), and states above the list that a recorded problem which does not name a case is not shown. On a clinical surface the reassuring-looking empty state is the dangerous one, so the empty branch says in words that a quiet ledger is not a well answer engine. Both are pinned by tests. A hub band links straight to the panel, in warning rather than danger colours: these are recorded problems, not blocking work, and painting them the same red as the blocking band would teach the reader to discount both. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(developer-hub): a named eval case is a reference, not a verdict on that case Raised in review of #2498, and confirmed against the real ledger text rather than taken on trust. `#J8SJQ9` is about `quality-antipsychotic-metabolic-monitoring` and names `quality-discharge-documentation` only as the CONTRAST -- the case that "deliberately drops mustContainAny" because a source pointer is a legitimate answer there. The panel listed it among the questions and counted it, stating on a clinical surface the opposite of what the ledger says. Two tempting fixes were tried against the real data and rejected: - Matching only the `source` field fixes `#J8SJQ9` and HIDES both genuinely broken questions in `#S4R2W3`, which names them only in its detail prose. On this panel a false negative is worse than a loose one. - Guessing intent from surrounding words is the fragile heuristic the panel exists to avoid; it would be wrong quietly, later. So the assertion moves to the level the data supports. The item-level claim was always sound -- these are recorded problems about clinical answers -- and the named cases are now presented as references: the section reads "Questions this item names", the count is "questions referenced" (`affectedQuestionCount` becomes `referencedQuestionCount`), and a second caveat says in words that an item may name a question as the contrast that is behaving correctly. Tests: the reviewer's requested fixture reproduces the real `#J8SJQ9` detail and pins that BOTH cases come back as references, with a comment saying a later change may only narrow that via an explicit per-case association in the ledger, never a heuristic. A DOM test pins the wording, and fails when the caveat is removed -- proven by mutation, then restored byte-identical. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Summary
demoMode,documentCountandemailallnull, rendering "environment unknown / document count unavailable / account unknown" on every visit. A new server-only helper,resolveHubEnvironmentFacts, gathers all three in one Supabase round trip, which makes the hub page an async Server Component. Two of its rules are load-bearing, and each is pinned by a test proven to fail without it: the count goes through the cookie-bound user client, so row-level security (documents owner read,owner_id = auth.uid()) scopes it to the caller's own documents rather than application code doing the scoping; and every failure path returnsnullrather than0, so an empty corpus can never be confused with a read that failed. The Phase 2 handoff document is corrected in the same commit instead of being left claiming the strip is unwired.verify:pr-localrun for this very branch, failing the gate for a diff that touches nothing under.design-sync/**. Deliberately not quarantined: it did not fail an assertion, and a wrong time budget is a different defect from an unstable test.tests/flake-ledger.jsonis untouched.Verification
npm run verify:pr-localReported precisely rather than as a clean pass. The run completed
check:runtime,check:installed-lock-parity,format:changed,sitemap:check,check:repo-awareness-snapshot,docs:check-index,docs:check-inventory,docs:check-scripts,docs:check-links,check:branch-review-ledger,check:outstanding-issues,check:ledger-write-discipline,lintandtypecheck, then reportedTests 1 failed | 11707 passed | 75 skipped (11783). The single failure wastests/design-sync-contract.test.ts > keeps sources, entry exports, previews and published props in parity, withError: Test timed out in 30000ms. No assertion failed, and that test reads only.design-sync/**, which this diff does not touch; it passes in isolation on the same worktree. That failure is fixed by the second commit in this PR.Because the suite failed, the run never reached
build,check:rag:fixtures,check:medication-interactionsorcheck:medication-lexicon-report. All four were then run separately, and all four passed:npm run build— exit 0,Compiled successfully in 100s,Client bundle secret surface check passed. This is the gate that matters most here: making the page fetch data changes its render mode, which typecheck, lint and Vitest are all blind to. The build classifies/mockups/developmentas dynamic, which it already was via the layout'sheaders()call, so nothing regressed.npm run check:rag:fixtures—Offline RAG fixture and manifest validation passed (36 golden cases, 26 suites).npm run check:medication-interactions—data/medication-interaction-index.json is up to date (531 rows).npm run check:medication-lexicon-report—docs/medication-interaction-lexicon-review.md is up to date (37 catalogue terms).Both behavioural rules in the new module were mutation-checked rather than assumed. Coercing a failed count to
0turns "keeps an empty corpus distinct from a count it could not read" and "reports a missing count as unavailable rather than as zero" red; removing the unauthenticated early return turns "does not count anything for a request with no signed-in user" red. The new 60 s timeout was proven to be read rather than silently ignored by setting the same option to 1 ms and observingTest timed out in 1ms. Every mutated file was restored and confirmed byte-identical by SHA-256.UI verification not run:
npm run verify:uiwas not run because no shared UI foundation changed — this alters the data behind one line of text on one administrator-only page, with no layout, chrome, routing or composer change. Instead the changed surface was proven directly:npm run ensureon this worktree,/api/local-project-idconfirmed as this project, and/mockups/developmentrendered and inspected in a browser. The strip readsDemo corpus / document count unavailable / build unknown / account unknown, which is the correct local-development rendering: demo mode is detected and reported, and the three facts that genuinely cannot be read without a live signed-in session name their own gaps instead of inventing values. The signed-in email and a non-null document count cannot be exercised locally, and are covered by unit tests rather than by a browser.npm run verify:releasenot run: not a release or handoff-confidence claim.npm run eval:retrieval:qualitynot applicable: no retrieval, ranking, selection, chunking or scoring behaviour changed.npm run check:production-readinessnot run: no clinical workflow, environment, source-governance or deployment behaviour changed. The Supabase read added here is a count of the caller's own rows through the existing user-session client, on a page that already authenticates through that same client on every request.Risk and rollout
nullprops and the synchronous page; reverting the second restores the 30 s timeout. Neither leaves migration, data or configuration state behind.documentscount query issued at render time on/mockups/development, which is gated to a signed-in administrator in production. No migration, no schema change, no write path, no new environment variable, and no service-role usage.Clinical Governance Preflight
scripts/pr-policy.mjsclassifies this diff asclinicalRisk: false,operationalRisk: false,ragRanking: false,ui: true, so this section is not required. It is completed anyway, because the change reads thedocumentstable and the signed-in account, and understating that would be the wrong instinct in this repository.Clinical KB Database(sjrfecxgysukkwxsowpy) — no environment or project configuration was touched.import "server-only"and usescreateSupabaseServerClient, the cookie-bound user client that is subject to RLS. It never importscreateAdminClient, and a source assertion intests/developer-hub-environment-facts.test.tsfails if that import or the stringsupabase/adminever appears in it, because the service-role client bypasses RLS and would report every owner's total to whoever was signed in.Notes
The two commits are bundled deliberately under the repository's PR-bundling rule: both are independently low-risk by
classifyPullRequestFiles, each is a separately revertible commit, and each has its own Summary bullet. They are also causally linked — the timeout fix repairs the failure that stopped this branch's own verification run, so landing them together avoids handing the same red to the next unrelated pull request.The second commit was made with
SKIP_DOCS_SYNC_HOOK=1, which the pre-commit hook itself documents as its bypass. The hook refuses a partially staged tree, so two disjoint commits are impossible in one pass. The bypass was used only for the intermediate commit; the final commit ran the hook in full on the complete tree, regeneratingdocs/site-map.md, the codebase-index coverage check and the design-system adoption manifest, all of which came back unchanged.One thing the push guard could not do: it reported
static: run coordinator busy — lint and typecheck NOT run, because another worktree held the heavy-run lease. Both were run locally before the commit and both passed, and CI enforces them regardless.Note
Low Risk
Changes are confined to an administrator-only developer hub page with conservative null-on-failure semantics and RLS-scoped reads; the main operational effect is one extra Supabase count query per hub load.
Overview
Wires the developer hub environment strip so demo/live mode, signed-in email, and document count come from real reads instead of always showing “unknown” placeholders.
Adds server-only
resolveHubEnvironmentFacts(syncisDemoMode(), one Supabase auth + owner-scopeddocumentscount via the user session client). The hub page becomes an async Server Component and passes those values intoEnvironmentStrip. Load-bearing rules are tested: RLS-scoped count (no admin client),nullon failures (not0), and no count query without a signed-in user.Test and docs follow-through: Phase 2 handoff marks the strip as closed; hub and in-page-nav DOM tests
awaitthe page and mock/stub the helper; new unit tests cover the resolver. Unrelated: design-sync contract subprocess tests get an explicit 60s timeout so full-suite CPU contention does not hit the global 30s limit.Reviewed by Cursor Bugbot for commit fdb93a1. Configure here.