Make a plan's own state decide what may be sent, overturning Ruling [129] (#PAMATF) - #2535
Conversation
#PAMATF. `createPlan` writes every planned contact in state `scheduled` AT CREATION -- the schedule is built when the plan is, not when it is activated -- and no plan lifecycle write touches them. `listSendableContacts` filtered on the contact alone, so a plan nobody had started, and one a coordinator had paused, presented ten messages as work about to go out. Not a live send hole: both stores already refuse to OPEN a dispatch on a non-active plan (`requiresActivePlan`), there is no dispatcher in the tree, and the only reader is the simulation driver. What was wrong is the read a future dispatcher will trust. THIS OVERTURNS RULING [129], with the owner's explicit approval. That ruling declined exactly this change for a good reason -- the write gate is real and correctly placed, and a second gate means "two places to keep in step where there is now one" -- and offered a rename or a plan-state-aware sibling instead. Every fact in [129] was re-verified and still holds. What changed is the weighing of the residual it named itself: a rename or a sibling leaves the misleading behaviour reachable and merely relabelled, which is the same "nothing is obliged to call the safe one" shape as #59JT7W. Gating the existing read makes the existing name true and leaves nothing to pick up by mistake. Recorded as Ruling [129A] in the build record, and the HANDOVER entry is closed rather than left contradicting the code. The cost [129] predicted is real, and is now pinned rather than accepted: the shared contract asserts the read gate and `requiresActivePlan` agree for every plan state where the plan is what decides. The ended states are excluded deliberately -- withdrawal cancels every contact, so both gates would agree there even if the plan gate were deleted. - `planSendingHold` MOVES from `schedule-view.ts` to `model.ts`, beside `contactSendability`. It already existed and was already exhaustive over `PlanState`; its own doc there called it "THE GATE listSendableContacts DOES NOT HAVE". `schedule-view.ts` re-exports both names. - Both stores gate on it. Neither needs a new query or join: the in-memory store holds `stored.plan.state` and the Postgres store already selected the plan row. A read racing an activation sees the older state and returns nothing -- the fail-closed direction. - `validateGovernedMessage` gains `plan-not-dispatchable` for draft and paused, which previously passed. The ended states keep their code. - A load-time invariant ties `planSendingHold` to `TERMINAL_PLAN_STATES`. Two pinned assertions change deliberately: - "holds without cancelling for a readmission" expected a paused plan to still offer 10 sendable contacts. It was pinning the defect. It now expects zero and additionally asserts all ten contacts still exist and are still `scheduled`, so the reversibility [129] cared about is proven more directly than before and could not be satisfied by cancelling them. - The simulation's paused-plan case now sees the refusal at the sendable read rather than the dispatch write. Its guarantee -- the store decides, not the driver -- is unchanged, and the write-side guard stays pinned independently by "refuses to begin a dispatch unless the plan is active". Also: the absorbed-contact contract test now activates its plan, so it measures absorption rather than reading a sendable list off a draft; the death-correction case gained a per-contact state assertion, because the plan gate had made its `[]` check pass regardless of contact state; and three comments that documented the missing gate as a live fact are corrected. 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. |
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_528a3ec0-0e4b-495f-b07d-f5aaf66d56a8) |
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. |
…ng-contacts-rules-r7r2ih-4
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_dc36a5d5-f43b-4c80-9b53-159d4d70db80) |
…ng-contacts-rules-r7r2ih-4
…ng-contacts-rules-r7r2ih-4
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_e639dbaa-cac3-4936-9692-201735465086) |
c82d6b6
into
claude/caring-contacts-rules-r7r2ih-3
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
Summary
listSendableContactsnow gate on the owning plan before the contact (#PAMATF).planSendingHoldfromschedule-view.tstomodel.ts, besidecontactSendability, and re-export it so every existing import still resolves.plan-not-dispatchableissue tovalidateGovernedMessage, covering the draft and paused plans that previously passed the chokepoint carrying a message.planSendingHoldtoTERMINAL_PLAN_STATES, beside the two that already guardcontactSendability.The defect
createPlanwrites every planned contact in statescheduledat creation — the schedule is built when the plan is, not when it is activated — and no plan lifecycle write touches them.listSendableContactsfiltered on the contact alone, so a plan nobody had started, and one a coordinator had paused, presented ten messages as work about to go out.This is not a live send hole
Both stores already refuse to open a dispatch on a non-active plan (
requiresActivePlan), there is no dispatcher or SMS provider in the tree, and the only reader is the simulation driver. What was wrong is the read a future dispatcher will trust — which is what Ruling [129] said to fix before anything is built that dispatches.This overturns Ruling [129], and says so in the documents
Ruling [129] (
docs/caring-contacts/phase-2b-build-record.md) considered exactly this change and declined it:Every fact in that ruling was re-verified before this change and every one still holds. What changed is the weighing of the residual it named itself: a rename or a sibling leaves the misleading behaviour reachable and merely relabelled — a safe alternative nothing obliges a future sender to choose. That is the identical shape as
#59JT7W, one PR down this stack, where the closing-message refusal sat in a function nothing was obliged to call.The overturn is recorded as Ruling [129A] in the build record, and the HANDOVER entry is closed rather than left contradicting the code. This is an owner decision, taken explicitly, not an incidental consequence of a bug fix.
And the cost [129] predicted is now pinned rather than accepted. There are two gates —
planSendingHoldon the read andrequiresActivePlanon the write — and the shared contract asserts they agree for every plan state where the plan is what decides. The ended states are excluded deliberately: withdrawal cancels every contact, so both gates would agree there even if the plan gate were deleted, and including them would make the assertion pass for the wrong reason.Reuse, not a second predicate
planSendingHoldalready existed, already exhaustive overPlanState, already distinguishing not-started from paused from ended — and its own doc called it "THE GATElistSendableContactsDOES NOT HAVE". It was a rule about the domain living in a view module, which the read that needed it could not import. Both now consult the one function.Neither store needs a new query or a join: the in-memory store holds
stored.plan.stateand the Postgres store already selected the plan row. A read racing an activation sees the older state and returns nothing — the fail-closed direction.Two pinned assertions change deliberately
Flagging these explicitly rather than letting them pass as incidental churn:
scheduled, so the hold's reversibility (what that test is really about) is still proven and could not be satisfied by cancelling them.not-sendable) rather than at the dispatch write. Its guarantee — that the store decides, not the driver — is unchanged, and refusing before a dispatch is opened is the better order. The write-side guard remains pinned independently, against both stores, by the contract test "refuses to begin a dispatch unless the plan is active", which takes its target before pausing and so still reaches the write.The absorbed-contact contract test now activates its plan, so it measures absorption rather than reading a sendable list off a draft.
A review round found a third: the death-correction case in
simulation.test.tsassertedlistSendableContacts → []to prove contacts 5-10 stayed cancelled. The plan gate now returns[]from the plan state alone, so that assertion would pass even if every contact were back inscheduled— which is what "nothing resumes" denies. It gained a per-contact state assertion. The other three→ []sites were checked and each already carries an independent contact-level or access-level assertion.Rejected alternative
Creating contacts in a new pre-scheduled state and transitioning them on activation. It needs a
contacts.stateCHECK change (a migration), a newContactStatemember with reclassification in two load-time invariants, a new transition carrying its own audit event, and it stores a second copy of a fact the plan row already holds.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 test:cc-guards—Test Files 42 passed (42),Tests 1054 passed (1054)npm run caring-contacts:db:testagainst a local disposable Postgres 16 —Tests 217 passed (217)planSendingHoldadmit a paused plan (so the read and write gates disagree) turns the new Ruling [129A] agreement assertion redThe database suite ran against a throwaway local cluster started from
/usr/lib/postgresql/16/bin. It is not the live Supabase project. No provider-backed gate was run.UI verification not run:
npm run verify:uineeds a dev server plus Chromium journeys this remote container has not confirmed provisioning for. The classifier reportsui: trueonly because a component file is touched, and that touch is a doc comment — no rendered output changes in this PR. Please run it locally before merge if you want the full Chromium gate.Risk and rollout
Clinical Governance Preflight
Clinical KB Database(sjrfecxgysukkwxsowpy)Notes
schedule-view.ts, the schedule API route, and the plan-activation wizard copy rationale. The user-facing activation copy itself is unchanged and still correct — creating a plan really does schedule its contacts; what is no longer true is that anything would treat them as ready to go out.#J7PZQPbranch; merge in sequence.🤖 Generated with Claude Code
https://claude.ai/code/session_014X6aJ6PgN26a8XiDm4FVHF
Generated by Claude Code
Note
Medium Risk
Changes caring-contacts send/dispatch semantics and shared repository contracts in a conservative (stricter) direction; write paths are unchanged but dual read/write gates must stay aligned via new tests.
Overview
Plan state now decides what counts as sendable, overturning Ruling [129] with owner approval (documented as Ruling [129A]).
planSendingHoldmoves from the schedule view intomodel.ts, and both repository implementations apply it before filteringscheduledcontacts inlistSendableContacts— draft and paused plans return an empty sendable list while contacts stayscheduled.validateGovernedMessageadds aplan-not-dispatchableissue (withhold: not-started vs paused) instead of letting draft/paused plans pass the message chokepoint; ended plans keepterminated-contact-dispatch-refusal.The write gate (
requiresActivePlanon dispatch) is unchanged. A shared repository contract now asserts read and write gates agree for draft, active, and paused. Tests and docs are updated (simulation refuses atlistSendableContactswhen paused; readmission case expects zero sendable but ten stillscheduled; death-correction checks contact state, not only an empty sendable list).Reviewed by Cursor Bugbot for commit 3e7d010. Configure here.