Route the closing-message refusal through the validateGovernedMessage chokepoint (#59JT7W) - #2532
Conversation
…GovernedMessage #59JT7W. The "refuse loudly rather than send nothing" rule for closing messages lived only in `resolveClosingContactMessageBody`, a standalone function nothing was obliged to call. A sender that resolved a closing body some other way met no refusal at all -- unlike the A1 fictional-contact check, which rides `validateGovernedMessage`, the chokepoint every future sender must pass. The rule now lives in that chokepoint: - `GovernedMessageInput.text` widens to `string | undefined`, so "nothing was authored" can be expressed at all. Previously a sender holding no body could only pass `""` -- refused for the wrong reason -- or skip the chokepoint and decide for itself. - An unauthored body is refused for EVERY message type, not only `closing`. Refusing only closing would have left the chokepoint answering `valid: true` for a standard message with no body at all -- a new permission granted by the very change meant to close a bypass, and reachable only because of the widening above. `standard` and `first` report `message-body-not-authored`; `closing` keeps its own code, which carries the specific A4 meaning that no closing wording has ever been clinically authored. - The refusal is reported instead of, not alongside, the text checks. Accumulating `closing-message-missing-ending-statement` would say "the body you wrote is wrong" about a body nobody wrote, and send a maintainer looking for wording to fix. - The record-level refusals are still reported when there is no body. `appendStateIssues` is shared by both exits, so a cancelled plan with no body no longer says only "write a body" -- the recoverable condition must not mask the unrecoverable one. - `resolveClosingContactMessageBody` keeps its signature and return type and becomes a thin adapter over the chokepoint, reading only its own issue code. Its failure branch still never returns a body. No behaviour reaches a patient: there is no dispatcher, no SMS provider and no send path in the tree, and `validateGovernedMessage` has no production caller today. This fixes a module contract a future sender inherits. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014X6aJ6PgN26a8XiDm4FVHF
|
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. |
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_d4cb0c6c-bbf8-4dda-b98f-1e9b8436e4e3) |
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_1e66cd70-43c1-4d23-999f-50d6ab5c7c03) |
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_26efcd2b-5191-44f7-b598-b584bc5fc177) |
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_fd3eed15-10f0-4f81-9311-bf535232e2bc) |
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_d73ea964-3b8b-4f3e-a960-7e4ada84f587) |
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_dddfe611-a576-4894-82e8-390a55a2fd83) |
PR #2532 squash-merged as 94a14a8, so this branch was auto-retargeted from that branch to main and went mergeable_state: dirty against the squashed history. The only conflict was at end of file, as on every previous sync: main's version of the 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
Sync only: main advanced by #2532 (Caring Contacts message-policy chokepoint), which touches no ledger, inbox, or snapshot file, so the single reconcile transaction on this branch is unchanged and still audits against the new base. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AcjoJHKAPfBPKYgwTF3wTS
Summary
validateGovernedMessage, the chokepoint every future sender must pass, so it can no longer be bypassed by resolving a closing body some other way (#59JT7W).GovernedMessageInput.texttostring | undefined, so "nothing was authored" can be expressed at the chokepoint at all.closing-message-body-not-authoredissue code, reported alone via an early return, keeping "no body exists" distinct from "the body is wrong".closing.resolveClosingContactMessageBodyto a thin adapter over the chokepoint; its signature and return type are unchanged.Why this was a hole
The "refuse loudly rather than send nothing" rule for closing messages lived only in
resolveClosingContactMessageBody— a standalone function nothing was obliged to call. Unlike the A1 fictional-contact check, which ridesvalidateGovernedMessage, a sender that obtained a closing body any other way met no refusal at all.The trap this deliberately avoids
Coercing an unauthored body to
""also refuses — but withclosing-message-missing-ending-statement, which means "the body you wrote is wrong" and sends a maintainer looking for wording to correct. There is none to correct. The early return is what keeps the two codes meaning different things, as the module's own doc comment requires. The failure branch still never returns a body.Two things a review round caught in the first draft of this fix
Both are recorded here rather than silently folded in, because each is the fix reintroducing a smaller version of the problem it exists to solve.
Refusing only
closingopened a new permission. Wideningtexttostring | undefinedmade "nothing was authored" expressible for the first time — and with a closing-only rule,validateGovernedMessage({ text: undefined, messageType: "standard" })returnedvalid: true: an explicit "this may be sent" for a message with no body at all. That is a new permission granted by the very change meant to close a bypass. The refusal now covers every type.standardandfirstreportmessage-body-not-authored;closingkeeps its own code, which carries the specific A4 meaning that no closing wording has ever been clinically authored.The early return hid the more serious refusal. A cancelled plan with no closing body reported only "write a body" — the recoverable condition masking the unrecoverable one, which is the mirror image of the false diagnosis above. A future operator surface keying on the code would have invited someone to author a closing message for a withdrawn plan and refused them on the second attempt.
appendStateIssuesis now shared by both exits; those checks read no text, so a missing body makes none of them unanswerable.Scope note
Nothing reaches a patient. There is no dispatcher, no SMS provider and no send path in the tree, and
validateGovernedMessagehas no production caller today. This fixes a module contract a future sender inherits.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)node scripts/run-vitest.mjs run tests/caring-contacts-message-policy.test.ts tests/caring-contacts-interface-vocabulary.test.ts—Tests 67 passed (67)npm run verify:pr-localnot run: this remote container has no Chromium/Playwright provisioning confirmed, and the changed scope is one pure, provider-free module plus its unit test. The offline gates above cover it; GitHub remains the authoritative merge gate.UI verification not run: no UI, routing, styling, or browser behaviour changed.
Risk and rollout
Clinical Governance Preflight
Clinical KB Database(sjrfecxgysukkwxsowpy)Notes
main.🤖 Generated with Claude Code
https://claude.ai/code/session_014X6aJ6PgN26a8XiDm4FVHF
Generated by Claude Code
Note
Medium Risk
Narrows PHI fetched on a hot list-read path and changes the governed-message contract (new refusal codes and optional
text), though no production send path calls it yet; regressions would mainly affect future senders or screens that assumedlistPlansstill loaded patient columns in-process.Overview
Two privacy and governance fixes in the caring-contacts domain: caseload reads stop loading patient fields at the database, and outgoing-message validation can no longer be bypassed when no body exists.
Caseload list read (#RZVMPD).
listPlansnow selects a newPLAN_LIST_COLUMNSset (nopatient_name, mobile, identifiers, etc.) instead of fullPLAN_COLUMNS, so schedule/caseload renders no longer pull every team's patient identifiers into the process only to drop them intoPlanRecord. Patient names stay on the auditedlistPatientNamespath. Source scans and a Postgres integration test assert the constant is wired and that issued SQL never names those columns.Message policy (#59JT7W).
GovernedMessageInput.textisstring | undefined, andvalidateGovernedMessagerefuses missing/blank bodies for all message types (closing-message-body-not-authoredvsmessage-body-not-authored), without running “wrong body” checks on empty text. Terminal contact/plan refusals still apply via sharedappendStateIssueseven when there is no body.resolveClosingContactMessageBodydelegates to the chokepoint instead of owning the rule alone.Reviewed by Cursor Bugbot for commit 3cba802. Configure here.