fix(caring-contacts): consolidate privacy, validation, and shell reliability - #2705
fix(caring-contacts): consolidate privacy, validation, and shell reliability#2705BigSimmo wants to merge 19 commits into
Conversation
…inician safety (#HDCF2B, #LM33K2, #Q8NMM3, #8K9W2B, #M6P1QQ, #99W2X1) - #HDCF2B: Refactor caseload search query to use POST body payloads and opaque session filter tokens with TTL expiration and automatic invalid token removal, eliminating PHI from URLs and access logs. - #LM33K2: Fix off-by-one boundary condition in caseload pagination query and normalize fractional page and page sizes. - #Q8NMM3: Extend CaringContactsAuditEntry and AuditEvent to capture actorRole signatures, updating export serialization and schema validation. - #8K9W2B: Synchronize notification delivery retry queue backoff ladder [1m, 5m, 15m, 1h, 6h], clamping attempts and handling dead-letter state. - #M6P1QQ: Add optimistic locking with version checks in draft-store to prevent stale draft overwrites, prevent resurrecting deleted drafts, and preserve clinician attempted content during conflicts. - #99W2X1: Add confirmation modal barrier with keyboard navigation and focus management before transitioning active contact plans to inactive.
|
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: Advanced 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_1fc7edda-1a01-4a43-ad5b-3680b85f62cb) |
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. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 957fa2f7cf
ℹ️ 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 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. The only repository destination is BigSimmo/Database, and the only branch destination is the pull request head branch task/caring-contacts-data-and-privacy at starting commit 0ad317d; never publish fixes to a detached or synthetic work branch and never create a stacked pull request. Use the authenticated GitHub connector to commit each approved fix to BigSimmo/Database:task/caring-contacts-data-and-privacy, then verify that the pull request head contains the published commit before reporting success. 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. For a fixed thread, reply with as the first line and as the second line. For a no-code disposition, use followed by . These result markers authorize the workflow to close that exact thread only after it verifies a fixed commit is the pull request head; a local-only commit is not a fix. If publication or verification fails, do not use either result marker, do not claim success, and leave the thread open with the blocker. If human input or new authorization is required, do the same. 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. |
CI triageCI failed on this PR. Automated classification of the 3 failed job(s):
Compared with main CI run #16436 (failure). Classification is evidence routing, not permission to ignore a failure. Exact quarantined Playwright identities remain governed by the flake ledger. |
…ectory client bundle
CI Build failed with webpack's UnhandledSchemeError on node:crypto. The chain was:
patients-directory-client.tsx ("use client") -> patients-directory-filter.ts ->
caseload-search-token.ts (imports node:crypto for the sft_ token store), because
patients-directory-filter.ts mixed a client-safe piece (state order, filter parsing)
with the one server-only function that resolves a filter token,
readPatientsDirectoryAddress.
Split readPatientsDirectoryAddress and its PatientsDirectoryAddress type out into a
new src/lib/caring-contacts/patients-directory-address.ts, marked `import "server-only"`
(the existing repo convention for this exact class of module, e.g.
caring-contacts-server/config.ts). patients-directory-filter.ts no longer imports
caseload-search-token.ts at all, so it stays safely importable from the client
component. Updated the two call sites (page.tsx, and the two test files that imported
readPatientsDirectoryAddress) to the new module path; no behavioural change.
Also regenerated data/repo-awareness-snapshot.json (npm run snapshot:repo-awareness),
which was behind on the new POST /api/caring-contacts/patients/search route this PR
already registered in site-map.md, fixing the check:repo-awareness-snapshot failure in
Static PR checks.
Verified: npm run build (full production build, webpack compiles clean), npm run
typecheck, eslint on the changed files, and the PR's caring-contacts test files
(116/116 passing, including caring-contacts-search-privacy.test.ts and
caring-contacts-patients-directory.dom.test.tsx which exercise
readPatientsDirectoryAddress directly).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01L8jaqEKUUgnjPrQgR18igS
|
Codex Review: Didn't find any major issues. Can't wait for the next one! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
…sion resolveSearchFilterToken previously accepted any bearer token and resolved it to the plaintext search query before authorization was checked, so a filterToken obtained from browser history, a Referer header, or an access log let anyone open the patients directory with the original searcher's query pre-filled, regardless of role. Tokens now store the minting actor's id and team alongside the query, and resolution requires the CURRENT actor to be that same actor AND to currently hold viewPatientRecord (READ_ACTIONS.patientName) -- the same capability the page already checks for mayViewPatientNames. A token replayed by anyone else, or redeemed after the grant is gone, resolves to null exactly like an expired token. readPatientsDirectoryAddress and the POST search route now thread the resolved actor through; the patients page resolves the actor before reading the address instead of after, since the address read itself now needs it (still ahead of the store and every audited read, preserving the existing "redirect before anything is recorded" guarantee). Addresses PR #2705 review thread PRRT_kwDOSh5Fis6f00qC. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01L8jaqEKUUgnjPrQgR18igS
…lve-month simulation DEFAULT_CONTACT_RETRY_POLICY (three attempts, 45 minutes apart) and the new notification retry backoff ladder in retry-queue.ts (1m/5m/15m/1h/6h, dead-letter after 5) were two separate, un-reconciled policies for the same clinical action. driveTwelveMonthSimulation -- the only non-test consumer of a retry policy in this codebase -- kept using the old flat-interval policy, so the advertised ladder never governed any real (simulated) contact retry. - retry-queue.ts: calculateRetryDelayMs/calculateNextRetryTime now take an optional ladder parameter (defaulting to RETRY_BACKOFF_LADDER_MS), so a different governed caller can share the same function instead of a parallel copy of the arithmetic. - service-rules.ts: ContactRetryPolicy now carries maxAttempts plus a backoffMs ladder (defaulting to retry-queue's own MAX_RETRY_ATTEMPTS / RETRY_BACKOFF_LADDER_MS) in place of a flat retryIntervalMinutes. - simulation.ts: driveTwelveMonthSimulation computes each attempt's offset from sendAt via calculateRetryDelayMs against the policy's ladder (attempt 1 carries the ladder's own first-attempt delay, matching NotificationRetryQueue's own enqueue-to-first-attempt behaviour), instead of a flat (attempt-1)*interval. isWithinApprovedSendWindow still refuses any attempt that would land outside the approved send hours or roll into the next AWST day -- that rule is unchanged and senior to the retry cadence, exactly as before. - tests/caring-contacts-simulation.test.ts updated for the new default (5 attempts, the real ladder offsets) and the new ContactRetryPolicy override shape, including a corrected window-boundary scenario (the ladder's much shorter early offsets mean the fourth attempt, not the third, is now the one that lands outside the window). Addresses PR #2705 review thread PRRT_kwDOSh5Fis6f00qM. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01L8jaqEKUUgnjPrQgR18igS
… domain CI's caring-contacts-domain-isolation test flagged the new patients-directory-address.ts for importing the "server" + "only" marker package: tests/caring-contacts-domain-isolation.test.ts holds every file under src/lib/caring-contacts/ to an allowlist of import specifiers (relative imports and bare node: builtins only), as proof the whole domain is self-contained and provider-free. That marker package is neither, and caseload-search-token.ts (which already imports node:crypto directly) never carried it either, so this file shouldn't be the first to. Removed the import; the module note now explains why. The boundary this file exists to enforce is structural, not marker-based: patients-directory-filter.ts (imported by the "use client" directory component) never imports this file or caseload-search-token.ts, so the client bundle cannot reach node:crypto regardless. Verified: tests/caring-contacts-domain-isolation.test.ts and the full caring-contacts test set from the prior commit still pass (132/132), typecheck clean, eslint clean, Prettier clean. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01L8jaqEKUUgnjPrQgR18igS
…the real plan-wizard draft path DraftStore/DraftConcurrencyError were only imported by their own test; the live plan wizard writes through writePlanDraft in plan-wizard/plan-draft.ts, whose stored PlanDraft shape had no version and whose writes went straight to storage with no check at all -- a genuine, if narrow, lost-update bug for two writes built from the same stale render (see PlanDraft.version's module note for the exact shape: the stage-4 auto-mint effect and an ordinary field edit can both close over the same pre-commit draft in one React commit). Note on scope: sessionStorage is tab-isolated, so the "modified in another tab or session" scenario DraftConcurrencyError's own message describes cannot literally happen for this consumer -- there is no cross-tab writer to race. The real, reachable race this closes is intra-tab: two writes from the same stale render closure. I wired the real optimistic-locking mechanism in rather than reaching for a bespoke check, because it's the correct tool for that narrower race and reuses code instead of duplicating the pattern. - draft-store.ts: DraftConcurrencyError is now generic over the stored draft's shape (defaulting to DraftMessage, so every existing caller keeps resolving unchanged), since PlanDraft shares no fields with DraftMessage beyond a version number. - plan-draft.ts: PlanDraft gains a `version` field (0 on a fresh draft, an absent field in an old stored draft parses as 0 -- non-clinical bookkeeping, safe to default, unlike this parser's strict-refuse rule for stage 3/4 fields). writePlanDraft now always re-reads the version actually held (never trusts the caller's own copy) and, when `options.expectedVersion` is given and does not match, throws DraftConcurrencyError<PlanDraft> carrying the live draft instead of silently overwriting it. - plan-wizard.tsx: a new writeDraftWithRetry wraps every write the wizard makes (update(), the stage-4 submission-mint effect, recordOnLiveDraft()). On a conflict it re-applies the SAME change onto the live draft from the thrown error and retries once -- provably sufficient since writePlanDraft is synchronous start to finish, so nothing can interleave between the retry's read and its write. Also includes `npm run format`'s corrections to two files from an earlier commit on this branch (caseload-search-token.ts, patients-directory.dom.test) that were not run through it at the time. Test: extended tests/caring-contacts-plan-draft.dom.test.tsx with cases proving writePlanDraft on the real module increments version on every write, throws DraftConcurrencyError on a stale expectedVersion, that the thrown error carries the live draft rather than the failed write's own copy, and that re-basing onto it (the exact pattern writeDraftWithRetry follows) loses neither write. Updated existing version-sensitive assertions in that file and in tests/caring-contacts-plan-wizard.dom.test.tsx. All of caring-contacts-plan-draft.dom.test.tsx, caring-contacts-draft-store.test.ts, and caring-contacts-plan-wizard.dom.test.tsx pass (126/126), plus the wider test:cc-guards suite (1074 tests, one pre-existing unrelated failure from review thread 2's still-open finding), a clean tsc --noEmit, and eslint. Addresses PR #2705 review thread PRRT_kwDOSh5Fis6f00qR. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01L8jaqEKUUgnjPrQgR18igS
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_5bb89b4e-f81d-4e55-bdcf-78d47376e4d6) |
…alidated helper
CI's tests/api-validation-contract.test.ts flagged the invalid-JSON-body branch
in patients/search/route.ts for a route-local NextResponse.json({ error }, ...)
envelope; every sibling Caring Contacts route uses the shared, schema-validated
invalidRequestResponse() helper for exactly this case (see access-trail/route.ts).
This line predates my changes to this file (it was already in the branch tip
before this task started) -- the earlier edits just shifted its line number by
adding code above it, which is presumably why the ratchet only now caught it.
Switched to invalidRequestResponse(); no test asserted the old literal string,
so nothing else needed to change.
Verified: tests/api-validation-contract.test.ts and
tests/caring-contacts-search-privacy.test.ts pass (49/49), clean tsc --noEmit
and eslint on the changed file.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01L8jaqEKUUgnjPrQgR18igS
…ct decision PlanStatusToggle (#99W2X1) was never wired into any production screen (review thread PRRT_kwDOSh5Fis6f00qG) and, being an unreachable Client Component under the Caring Contacts workspace shell, tripped two real CI gates on its own: - check:design-system-contract: hardcoded motion/z-index/color utilities bypassing the repo's design tokens. - tests/caring-contacts-explained-automation.dom.test.tsx: any new Client Component under this tree must be deliberately added to ALLOWED_CLIENT_COMPONENTS, since the shell hands that subtree a whole ServiceState (including patient-identifying `note`) and a client boundary serialises props into the RSC payload. The safety property this component claims to add already exists, more rigorously, on the existing "Hold this plan" (pause) action -- see the review thread reply for the full comparison. Wiring it in as a second, less rigorous control for the same action would misrepresent that open thread as resolved. Removing it here so the six other real fixes in this PR (search-token privacy, pagination, audit roles, retry ladder, draft concurrency) can land clean; the deactivation-UX question is deferred to its own follow-up once the design direction is decided. Verified: tests/caring-contacts-explained-automation.dom.test.tsx (28/28), the full Caring Contacts + related contract test set (340/340), check:design-system-contract, typecheck, lint, and check:diff-integrity (net test count 197 -> 255, deletion covered by the aggregate) all pass. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01L8jaqEKUUgnjPrQgR18igS
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_62ca7b93-11bf-4bcc-a84c-1fe8c9ce3b06) |
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_deef3907-7c1a-4b0a-8078-345163e8fcc3) |
…ion, and plan-store validation
Resolve 4 quick-win styling, configuration, doc, and validation tasks:
- #NKHVRY (P2): Dark specimen tile paints dead compat palette. In src/components/caring-contacts/mockups/component-state-specimens.tsx:128, add ckb-v2 alongside dark on <article> mount so CSS custom properties inherit the v2 dark palette instead of legacy fallback.
- #42M061 (P2): Tailwind does not scan mockups tree. Add src/mockups/**/*.{ts,tsx,html} and src/components/caring-contacts/mockups/**/*.{ts,tsx,html} to tailwind.config.ts content array.
- #Q33JV6 (P2): Crisis-line re-verification cadence contradiction. Reconcile doc contradiction between audit (12 months) and spec (6 months) in docs/care-plan/crisis-lines-verification.md and create docs/caring-contacts-crisis-lines.md establishing canonical 6-month verification.
- #V6CDEV (P3): createPlan accepts blank patient name. In src/lib/caring-contacts/plan-store.ts (and in-memory and postgres repositories), enforce name.trim().length > 0, throwing a validation error on blank names. Add unit tests in tests/caring-contacts-plan-store.test.ts and contract tests in tests/helpers/caring-contacts-repository-contract.ts.
- Queue done requests for #NKHVRY, #42M061, #Q33JV6, and #V6CDEV in docs/outstanding-issues-inbox/.
…testids
Resolves #2NRB8V, #57QDCS, #XXH42K, #NHGFXR, #1BHXEF, #QX7TP2, #JZA0XK
- #2NRB8V: Scope three caring-contacts-guidance assertions in tests/ui-caring-contacts-workspace.spec.ts to the active boundary section (section[aria-labelledby='caring-contacts-guidance-boundary'])
- #57QDCS: Add isMountedRef unmount tracking and async state guard in PlanWizard; scope reload draft test in tests/ui-caring-contacts-activation.spec.ts to :visible
- #XXH42K: Settle openWorkspace helper in tests/ui-caring-contacts-workspace.spec.ts using expect.poll to wait for both caring-contacts-rail and caring-contacts-phone-dock simultaneously
- #NHGFXR: Differentiate loading synthetic marker testId ('caring-contacts-loading-synthetic-marker') in src/app/caring-contacts/loading.tsx and src/components/caring-contacts/workspace/synthetic-marker.tsx
- #1BHXEF: Freeze demo clock mockup test in tests/ui-ward-roles.spec.ts using page.clock.pauseAt(new Date('2026-08-26T10:00:00Z'))
- #QX7TP2: Add unit test suite in tests/caring-contacts-wizard.test.ts covering the plan wizard stage transition matrix, definitions, implementations, and traversal helpers
- #JZA0XK: Add browser assertions in tests/ui-caring-contacts-activation.spec.ts verifying disabled submission controls during in-flight activation and idempotent handling in the created-not-started two-write middle state
- Queue ledger inbox done requests for all 7 issues
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_6352b9c6-2fe6-479a-b139-c6c0d7a54bc9) |
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_325e78e3-7d50-4340-8d4a-5f0be372c0fc) |
BigSimmo
changed the title
2 participants
Summary
Consolidates patient-search privacy, audit/retry persistence, plan validation, and workspace shell fixes. Requires caller-supplied assurances and write context, keeps patient details out of write results, and retains the unresolved SQL blank-name constraint as tracked work. Crisis-line values are explicitly unverified.
Consolidates #2705 (1d99af1), #2739 (cbe04f8), #2743 (eb78c46). Original heads are retained as ancestors; absorbed PRs are closed only after publication is verified.
Verification
npm run test -- tests/caring-contacts-plan-store.test.ts tests/caring-contacts-repository.test.ts tests/caring-contacts-plan-wizard.dom.test.tsx tests/caring-contacts-plan-patient-detail.test.ts tests/caring-contacts-plan-activation.test.ts tests/caring-contacts-wizard.test.ts— 306 passed in 6 files.npm run format— passed; formatting committed.git diff --check— passed.Verification not run: full local PR/release suites; focused integration checks above were selected, and GitHub remains the merge gate. No live provider evaluation, deployment, migration or production data access was performed.
UI verification not run: local browser matrix; applicable hosted UI checks must pass before merge.
Current-base CI limitation: Make the outstanding-issues snapshot merge-safe (v2), and record the measured assessment of two other rows #2735 run 34495136858 failed only the aged ranking fixture provenance test (32 days old). clinical(governance): resolve forms permissions, test guards, and alert tiers (Bundle 1) #2763 carries the existing refresh; these bundles must not be merged past failed required checks.
Follow-up after integration: cancelled duplicate #42M061 completion request;
npm run check:outstanding-issuespassed with both explicit cancellation decisions.npm run test -- tests/caring-contacts-plan-store.test.ts— 12 passed after the fixture type correction. The subsequent normal push guard passed lint/typecheck.Risk and rollout
npm run check:production-readinesson the shared current-base governance integration stopped at six existing privacy release blockers (five pending and one partial). The production-readiness stage did not execute. These approvals remain outstanding.Clinical Governance Preflight
Clinical KB Database(sjrfecxgysukkwxsowpy)