Stop the caseload list read fetching patient columns it discards (#RZVMPD) - #2533
Conversation
…cards #RZVMPD. `listPlans` selected `PLAN_COLUMNS` verbatim, so every render of the Patients directory pulled every patient's name, mobile number and identifier list for the whole team into the process -- and `toPlanRecord` discarded all three. Nothing was released (`PlanRecord` excludes `patientDetail` structurally, and that guarantee held), but the data need never have been fetched. - New `PLAN_LIST_COLUMNS`, used by `listPlans` only. `PLAN_COLUMNS` is unchanged for `readPlanRecord` and `selectPlanForUpdate`, whose callers genuinely need the patient detail -- narrowing the shared constant in place would leave `getEpisode` projecting `undefined` for three fields, which no type checks. - `patient_name` is dropped alongside the mobile number and identifiers. Names have their own read with its own capability check and its own `patientNameDirectory` access-audit object type (Ruling 91), so pulling names inside a read audited as `plan` under-counted the "who read patients' names" trail. Two guards, because neither alone is sufficient: - A static scan of `PLAN_LIST_COLUMNS` and its wiring, beside the existing `first_contact_reason` and `preferred_name` scans. It also asserts the constant is actually used, so a correctly narrowed list that was never wired up cannot read as a fix. - A wire-level test recording every statement `listPlans` issues, which a scan cannot replace: a second hand-written list query added later would never name the constant at all. Both were confirmed to fail against the pre-fix code and pass after it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014X6aJ6PgN26a8XiDm4FVHF
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Team Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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. |
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. |
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_7e3904d2-ec07-4820-b848-87e0f6f6ba59) |
…-contacts-rules-r7r2ih-2
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_9eeeb85a-128f-4921-8c12-29133cbea41c) |
…2ih' into claude/caring-contacts-rules-r7r2ih-2
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_fa9efb5a-e1ea-4bf0-b308-d5dedc9f7b10) |
PRs #2533 and #2535 were squash-merged into their base branches rather than into main, which left this branch behind its own base. Its diff had started to show unrelated main work as deletions -- the Phase 3 plan document, a ledger inbox record, the ward-flow roadmap and the docs-link checker -- and the base had stopped merging cleanly. Bringing the base forward fixes both. The only conflict was at end of file: the base's squashed caseload-read test ends the file, and this branch appends the mid-read clearance race test that Codex review asked for. Kept the test. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014X6aJ6PgN26a8XiDm4FVHF
… not run Codex review finding (P2) on PR #2572, and it was right. All four review records named individual hosted CI jobs as green and then ended with "No provider-backed gate run" -- but AGENTS.md classifies hosted CI as provider-backed, so the record contradicted itself. A later reader could not tell whether those results were observed or inherited from someone else's report, which is exactly what a review record exists to settle. Both halves were true and the sentence conflated them. The hosted results WERE observed: this session read them from the GitHub check runs via the MCP GitHub tools, under a standing instruction to babysit these PRs. What was not run is the set of gates that call OpenAI or Supabase. Each record now separates LOCAL OFFLINE GATES from HOSTED CI, states who observed the hosted result, and names the specific provider-backed gates that were not run rather than denying provider contact wholesale. Two records gained a correction beyond the wording: #2533 and #2535 had NO hosted CI of their own, because repo CI is scoped to branches [main, release/**] and their base was another feature branch. Their records now say that plainly and point at the main-based head whose CI actually covered them, instead of implying a pipeline ran on them. Record filenames are a sha256 of the row, so these were regenerated through ledger:append rather than edited in place; the four superseded files were never merged, so no immutable history is rewritten and the net diff against main is unchanged at four added records. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014X6aJ6PgN26a8XiDm4FVHF
2 participants
Summary
listPlansgets its ownPLAN_LIST_COLUMNS, droppingpatient_name,patient_mobile_numberandpatient_identifiers(#RZVMPD).PLAN_COLUMNSunchanged forreadPlanRecordandselectPlanForUpdate, whose callers genuinely need the patient detail.listPlansissues.listPatientNamesandschedule-viewdoc comments, which stated the old behaviour as fact.What was happening
Every render of the Patients directory ran
select ${PLAN_COLUMNS} from caring_contacts.planswith noWHERE, pulling every patient's name, mobile number and identifier list for the whole team into the application process — andtoPlanRecordmapped none of them.Nothing was released.
PlanRecordexcludespatientDetailstructurally and that guarantee held throughout; the narrowing was in the mapping, which releases nothing but fetches everything. It is now in the query.Why
patient_namegoes tooThe issue named two columns; this drops three. Names have their own read with its own capability check and its own
patientNameDirectoryaccess-audit object type (Ruling 91), which exists so "who read patients' names, and when" is answerable. Fetching names inside a read audited asplanunder-counted that trail.Why a separate constant rather than a narrowed
PLAN_COLUMNSgetEpisodeandmarkRetentionClearedlegitimately need those columns viareadPlanRecord. Narrowing the shared constant in place would leavegetEpisodeprojectingundefinedfor three patient fields — a worse defect than this one, and one no type would catch.Why two guards
Neither is sufficient alone. A static scan cannot see a second hand-written list query added later that never names the constant — which is the shape the original defect had. A runtime test alone would not catch the constant being widened for a path no test exercises. The static scan also asserts the constant is actually used, so a correctly narrowed list that was never wired up cannot read as a fix.
Both were confirmed to fail against the pre-fix code and pass after it.
Verification
npm run test— full offline unit suite,Test Files 949 passed (949),Tests 12292 passed | 1 skipped (12293)npm run lint— eslint at--max-warnings 0, exit 0npm run typecheck—tsc --noEmit, exit 0npm run format(committed)npm run caring-contacts:db:testagainst a local disposable Postgres 16 —Tests 214 passed (214), up from 213 by the new wire-level guardnode scripts/run-vitest.mjs run tests/caring-contacts-domain-isolation.test.ts—Tests 12 passed (12)listPlanstoPLAN_COLUMNSturns both new guards redThe database suite ran against a throwaway local cluster started from
/usr/lib/postgresql/16/bin. It is not the live Supabase project, andassertNotClinicalKbProject()refuses that ref by construction. No provider-backed gate was run.npm run verify:pr-localnot run: this remote container has no confirmed Chromium/Playwright provisioning, and the offline gates above cover the changed scope. GitHub remains the authoritative merge gate.UI verification not run: no UI, routing, styling, or browser behaviour changed.
Risk and rollout
listPlansrow;toPlanRecordis the sole mapper and reads none of them, which the shared contract suite and the database suite both exercise.SELECTlist has no schema footprint.Clinical Governance Preflight
Completed voluntarily. The classifier returns
clinicalRisk: falsefor these paths, but this is a change to how patient mobile numbers and identifiers are handled for a suicide-prevention cohort, which is squarely inside the repository's own "PR risk detection" list. The mechanicalfalseis a substring accident, not a judgement.Clinical KB Database(sjrfecxgysukkwxsowpy)Notes
#59JT7Wbranch, so this PR's diff is only its own change; merge in sequence.🤖 Generated with Claude Code
https://claude.ai/code/session_014X6aJ6PgN26a8XiDm4FVHF
Generated by Claude Code
Note
Low Risk
This removes PHI from a hot-path SELECT with no API shape change (
PlanRecordnever exposed those fields); risk is low unless an undocumented consumer read patient columns off raw query rows.Overview
Narrows the Postgres caseload query so
listPlansno longer selectspatient_name,patient_mobile_number, orpatient_identifierson every Patients-directory render. A newPLAN_LIST_COLUMNSconstant drives that read; fullPLAN_COLUMNSstays for single-plan paths likereadPlanRecord/selectPlanForUpdatethat still need patient detail forgetEpisodeand retention clearance.Privacy and audit alignment: names remain available only through
listPatientNames(separate capability andpatientNameDirectoryaudit). Comments inlistPatientNamesandschedule-vieware updated to reflect query-level narrowing, not justPlanRecordtyping.Regression guards: a static source scan asserts
PLAN_LIST_COLUMNSexcludes patient fields and is wired intolistPlans; a Postgres integration test records every SQL statement fromlistPlansand fails if any names a patient column.Reviewed by Cursor Bugbot for commit bdf3001. Configure here.