fix(audit): finish #598's triage — five more audit-first, and the rule gets a test - #612
Merged
Merged
Conversation
…e gets a test The owner's #598 decision was that a path which CAN audit before it mutates should. #607/#608 took the plain reorders; this finishes the sweep's output, with every remaining candidate opened rather than named from the tool's summary. **Five flipped, each needing a one-field seam change:** `createMeasure`, segment create, segment UPDATE, segment delete, `uploadEvidence`. `CreateMeasureInput`, `CreateSegmentInput` and `InsertEvidenceInput` now accept the value the event keys on — optional, minted by the store when absent, so every other caller is unchanged. That was the whole obstacle: the store minted the id, or for evidence the `uploadedAt` the payload reports as `payload.timestamp`. Segment UPDATE needed three writes moved rather than one (`updateSegment`, `setMeasures`, `setOverrides`), so a failure after the first left a partly-updated segment with no event at all; its 404 became an explicit pre-read, because `updateSegment` returning null WAS the not-found signal, which is what made the old order unavoidable. `uploadEvidence` audits before the BUCKET write too — an object in storage the ledger never mentions is harder to notice than a missing row. **`src/audit/audit-order.test.ts` exists because nothing tested the rule.** Nine call sites had been flipped across three commits and no test could tell: every existing test asserts the event EXISTS after a SUCCEEDING operation, which is equally true in either order, so a reorder back was silent. Each case makes the MUTATION fail and requires the event anyway — the only externally visible difference between the orders. Mutation-checked on two. Still mutate-first, with the reason now at each call site: the run-created transition and the import-driven finalize (deliberate — the event is best-effort at the run boundary); `dispatchOutreach`, which dispatches a message before any ledger entry and builds its payload from the delivery result, so it needs ADR-073 d4's intent-then-completion pair and that adds an event type consumers read; the three identity-link writes, whose obstacle is sharper than "the store mints the id" — `upsertLink` returns the EXISTING row's id on conflict, so keying those events on the PAIR is the fix and it changes what `entity_id` means; and the two backfill scripts, which are seeding tools rather than operator surfaces. Checked and NOT violations, every one a matcher artifact: `audit-packet` (a hash), `materialize-run` and `backfill-trend-history` (reads), evidence download, `measure-seed` (itself audit-first), subject-list create (its audit is a `beforeComplete` callback that runs before the list becomes visible), and panel assignment, which audits before the mapping and records each per-case event before `assignCases`. The sweep is fully triaged, which is not #598 closing: what remains is the cross-store `applyCaseAction` primitive plus the outreach and identity decisions. §4 and CLAUDE.md say exactly that. Backend 2,856 tests: 2,832 pass, 23 skip, 1 pre-existing local failure (`corpus-membership`).
![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: 525835649f
ℹ️ 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".
| enabled: (body.enabled as boolean | undefined) ?? before.enabled, | ||
| measureIds: (body.measureIds as string[] | undefined) ?? before.measureIds, | ||
| }); | ||
| await store.updateSegment(putId, { |
There was a problem hiding this comment.
If another request deletes the segment after getSegment succeeds but before this call, updateSegment returns null, but the result is ignored and the final json(await store.getSegment(...)) responds 200 null instead of the route's documented 404. This previously returned 404 from the mutation result; preserve that check even though the preceding audit event is intentionally allowed to over-claim.
Useful? React with 👍 / 👎.
Comment on lines
+223
to
+226
| await audit(stores.events, "SEGMENT_UPDATED", putId, actor, { | ||
| name: (body.name as string | undefined) ?? before.name, | ||
| enabled: (body.enabled as boolean | undefined) ?? before.enabled, | ||
| measureIds: (body.measureIds as string[] | undefined) ?? before.measureIds, |
There was a problem hiding this comment.
When two admins update the same segment concurrently, omitted fields can change between the pre-read and the mutation. For example, if this request reads enabled: true, another request changes it to false, and this request changes only the name, updateSegment preserves the newer false value while this event reports true. The previous post-write hydration reported the actual result, so use a CAS/transactional snapshot or limit the audit payload to values this request actually supplies.
Useful? React with 👍 / 👎.
Comment on lines
+34
to
+38
| Case actions, the measure/segment/value-set/waiver/appointment/evidence paths already do; the | ||
| run-created case transition and the import-driven finalize deliberately do not, and outreach cannot | ||
| without a new event pair. `DATA_MODEL_CONTRACTS` §4 has the whole triage and | ||
| `backend-ts/scripts/audit-order-sweep.py` re-derives it — **but #598 does not close on §4, because | ||
| what is still missing is the cross-store `applyCaseAction` PRIMITIVE, not the ordering.** |
There was a problem hiding this comment.
The referenced sweep still reports untriaged mutations: runResolve calls upsertResolvedValueSet before appendAudit in both success and error paths, while backfillTrendHistory creates runs and records outcomes before TREND_HISTORY_SEEDED (despite §4 labeling that path as reads). Those writes can therefore persist without their audit event, so the single-source operating rule should not say ordering is fully triaged until these paths are fixed or their exceptions and rationale are recorded.
AGENTS.md reference: AGENTS.md:L5-L9
Useful? React with 👍 / 👎.
added 3 commits
September 21, 2026 17:10
…ts that could not fail Review of this branch found seven things. One is a regression this branch introduced; three are tests asserting a weaker property than their titles claim. **The PUT's relocated 404 dropped a guard that also protected the two later writes.** `updateSegment` returning null was the not-found signal, and moving the 404 to an explicit pre-read discarded it — so a row vanishing between the check and the write gave either a 500 (`setMeasures` violating the `segment_measures` foreign key) or an HTTP 200 whose body is `null`, where the old order returned a clean 404 for both. Two concurrent admin requests reach it. The return value is checked again, before the child writes. **All three new segments tests passed against the pre-change code.** The reason was a false claim in the test file's own header: that the route's ordering is unreachable because it resolves its stores from `env`. It is reachable — the store is a class, and patching its prototype makes a write fail against the real fixture. The DELETE case asserted only that the payload name came from a pre-read, which was true before the change too. Both cases now make the write fail and require the event to survive, and the PUT gets one for the vanished-row 404. **The event reported a measure list the row would never hold.** `setMeasures` writes `[...new Set(...)]` and `hydrate` reads back ordered, so a payload built from the request array named something the segment never contained. Harmless while the audit came second; a payload-accuracy regression once it comes first, in the direction #598 exists to close. **None of the three new seams was exercised by the store contract**, so the deployed Postgres ceiling was asserted nowhere: deleting `input.id ??` from the SQLite adapter failed a test, and the identical edit to the Pg adapter failed nothing. Three contract cases now, on both stores. **Four existing audit-first paths had no ordering test** despite §4 saying one belongs — `transitionStatus`, `createTerminologyMapping`, value-set attach and detach. **§4's completeness claim was wrong for the third time.** `backfill-trend-history` was filed under "not a violation (reads)" on the strength of two of its four hits; the other two are writes. `recover-stuck-runs`, `resolve-valuesets` and `batch-evaluate-scale` were missing altogether, and the PUT's own writes now surface as matcher artifacts against the DELETE's audit. Each time the prose was plausible and the arithmetic was not done — so §4 now carries the count, 55 hits across 20 files, plus the one-line command that re-derives it. Recorded rather than fixed: EVIDENCE_UPLOADED now reaches the case TIMELINE (`audit_events WHERE ref_case_id`), so a failed bucket write leaves a permanent "Evidence uploaded — <filename>" row with nothing to download. The rule picks the over-claim side for the ledger; whether a clinical-ops read surface should inherit it for a named file is an owner call, and §4 says so now. Backend 2,863 tests: 2,839 pass, 23 skip, 1 pre-existing local failure. Mutations killed: reverting the PUT ordering, dropping the null check, reverting the dedupe, and re-minting each of the three seam values — each failing only its own case.
…uessed post-state (Codex, #612) Merging the request over a pre-read produced a post-state, and under concurrency that post-state is a guess: read `enabled: true`, let another admin set it false, change only the name, and `updateSegment` preserves the newer false while the event reports true. The post-write hydration this replaced could not be wrong about it, because it re-read — and an audit-first event cannot re-read. So the payload is now what THIS REQUEST changes: each field the body supplies, plus a `changed` list naming exactly that set, and nothing about the fields it does not set. `measureIds` stays deduped, because that is what the row will hold. A consumer wanting the resulting state reads the row; what the ledger is for is who changed what, and every value here is knowable before the write and true after it. Pinned by a deterministic race rather than an argument: another writer flips `enabled` between this request's pre-read and its write, and the assertions are that the event says nothing about `enabled` and that the other writer's value survives — which is what the merged form would have mis-reported. Codex's other two findings on this PR were already closed by the previous commit: rechecking `updateSegment`'s result, and §4's incomplete audit-order triage. Backend 2,864 tests: 2,840 pass, 23 skip, 1 pre-existing local failure. Mutation-checked: restoring the merged payload fails the race case.
# Conflicts: # docs/JOURNAL.md
Taleef7
added a commit
that referenced
this pull request
Sep 23, 2026
…e gets a test (#612) * fix(audit): finish #598's triage — five more audit-first, and the rule gets a test The owner's #598 decision was that a path which CAN audit before it mutates should. #607/#608 took the plain reorders; this finishes the sweep's output, with every remaining candidate opened rather than named from the tool's summary. **Five flipped, each needing a one-field seam change:** `createMeasure`, segment create, segment UPDATE, segment delete, `uploadEvidence`. `CreateMeasureInput`, `CreateSegmentInput` and `InsertEvidenceInput` now accept the value the event keys on — optional, minted by the store when absent, so every other caller is unchanged. That was the whole obstacle: the store minted the id, or for evidence the `uploadedAt` the payload reports as `payload.timestamp`. Segment UPDATE needed three writes moved rather than one (`updateSegment`, `setMeasures`, `setOverrides`), so a failure after the first left a partly-updated segment with no event at all; its 404 became an explicit pre-read, because `updateSegment` returning null WAS the not-found signal, which is what made the old order unavoidable. `uploadEvidence` audits before the BUCKET write too — an object in storage the ledger never mentions is harder to notice than a missing row. **`src/audit/audit-order.test.ts` exists because nothing tested the rule.** Nine call sites had been flipped across three commits and no test could tell: every existing test asserts the event EXISTS after a SUCCEEDING operation, which is equally true in either order, so a reorder back was silent. Each case makes the MUTATION fail and requires the event anyway — the only externally visible difference between the orders. Mutation-checked on two. Still mutate-first, with the reason now at each call site: the run-created transition and the import-driven finalize (deliberate — the event is best-effort at the run boundary); `dispatchOutreach`, which dispatches a message before any ledger entry and builds its payload from the delivery result, so it needs ADR-073 d4's intent-then-completion pair and that adds an event type consumers read; the three identity-link writes, whose obstacle is sharper than "the store mints the id" — `upsertLink` returns the EXISTING row's id on conflict, so keying those events on the PAIR is the fix and it changes what `entity_id` means; and the two backfill scripts, which are seeding tools rather than operator surfaces. Checked and NOT violations, every one a matcher artifact: `audit-packet` (a hash), `materialize-run` and `backfill-trend-history` (reads), evidence download, `measure-seed` (itself audit-first), subject-list create (its audit is a `beforeComplete` callback that runs before the list becomes visible), and panel assignment, which audits before the mapping and records each per-case event before `assignCases`. The sweep is fully triaged, which is not #598 closing: what remains is the cross-store `applyCaseAction` primitive plus the outreach and identity decisions. §4 and CLAUDE.md say exactly that. Backend 2,856 tests: 2,832 pass, 23 skip, 1 pre-existing local failure (`corpus-membership`). * fix(audit): the review's corrections — a 404 I dropped, and three tests that could not fail Review of this branch found seven things. One is a regression this branch introduced; three are tests asserting a weaker property than their titles claim. **The PUT's relocated 404 dropped a guard that also protected the two later writes.** `updateSegment` returning null was the not-found signal, and moving the 404 to an explicit pre-read discarded it — so a row vanishing between the check and the write gave either a 500 (`setMeasures` violating the `segment_measures` foreign key) or an HTTP 200 whose body is `null`, where the old order returned a clean 404 for both. Two concurrent admin requests reach it. The return value is checked again, before the child writes. **All three new segments tests passed against the pre-change code.** The reason was a false claim in the test file's own header: that the route's ordering is unreachable because it resolves its stores from `env`. It is reachable — the store is a class, and patching its prototype makes a write fail against the real fixture. The DELETE case asserted only that the payload name came from a pre-read, which was true before the change too. Both cases now make the write fail and require the event to survive, and the PUT gets one for the vanished-row 404. **The event reported a measure list the row would never hold.** `setMeasures` writes `[...new Set(...)]` and `hydrate` reads back ordered, so a payload built from the request array named something the segment never contained. Harmless while the audit came second; a payload-accuracy regression once it comes first, in the direction #598 exists to close. **None of the three new seams was exercised by the store contract**, so the deployed Postgres ceiling was asserted nowhere: deleting `input.id ??` from the SQLite adapter failed a test, and the identical edit to the Pg adapter failed nothing. Three contract cases now, on both stores. **Four existing audit-first paths had no ordering test** despite §4 saying one belongs — `transitionStatus`, `createTerminologyMapping`, value-set attach and detach. **§4's completeness claim was wrong for the third time.** `backfill-trend-history` was filed under "not a violation (reads)" on the strength of two of its four hits; the other two are writes. `recover-stuck-runs`, `resolve-valuesets` and `batch-evaluate-scale` were missing altogether, and the PUT's own writes now surface as matcher artifacts against the DELETE's audit. Each time the prose was plausible and the arithmetic was not done — so §4 now carries the count, 55 hits across 20 files, plus the one-line command that re-derives it. Recorded rather than fixed: EVIDENCE_UPLOADED now reaches the case TIMELINE (`audit_events WHERE ref_case_id`), so a failed bucket write leaves a permanent "Evidence uploaded — <filename>" row with nothing to download. The rule picks the over-claim side for the ledger; whether a clinical-ops read surface should inherit it for a named file is an owner call, and §4 says so now. Backend 2,863 tests: 2,839 pass, 23 skip, 1 pre-existing local failure. Mutations killed: reverting the PUT ordering, dropping the null check, reverting the dedupe, and re-minting each of the three seam values — each failing only its own case. * fix(audit): SEGMENT_UPDATED reports what the request changes, not a guessed post-state (Codex, #612) Merging the request over a pre-read produced a post-state, and under concurrency that post-state is a guess: read `enabled: true`, let another admin set it false, change only the name, and `updateSegment` preserves the newer false while the event reports true. The post-write hydration this replaced could not be wrong about it, because it re-read — and an audit-first event cannot re-read. So the payload is now what THIS REQUEST changes: each field the body supplies, plus a `changed` list naming exactly that set, and nothing about the fields it does not set. `measureIds` stays deduped, because that is what the row will hold. A consumer wanting the resulting state reads the row; what the ledger is for is who changed what, and every value here is knowable before the write and true after it. Pinned by a deterministic race rather than an argument: another writer flips `enabled` between this request's pre-read and its write, and the assertions are that the event says nothing about `enabled` and that the other writer's value survives — which is what the merged form would have mis-reported. Codex's other two findings on this PR were already closed by the previous commit: rechecking `updateSegment`'s result, and §4's incomplete audit-order triage. Backend 2,864 tests: 2,840 pass, 23 skip, 1 pre-existing local failure. Mutation-checked: restoring the merged payload fails the race case. --------- Co-authored-by: Taleef <taleef@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The owner's #598 decision was that a path which can audit before it mutates should — the ledger
errs toward an over-claim rather than a silent state change. #607/#608 took the plain reorders. This
finishes the sweep's output, with every remaining candidate opened rather than named from the
tool's summary.
Five flipped, each needing a one-field seam change
createMeasure· segment create · segment update · segment delete ·uploadEvidenceCreateMeasureInput,CreateSegmentInputandInsertEvidenceInputnow accept the value the eventkeys on — optional, minted by the store when absent, so every other caller is unchanged. That was the
whole obstacle these shared, and it is the same one
createTerminologyMappingandgrantWaiveralready didn't have: the store minted the id, or for evidence the
uploadedAtthe payload reports aspayload.timestamp.Two details worth stating:
updateSegment,setMeasures,setOverrides— so a failure after the first left a partly-updated segment with no event at all.Its 404 became an explicit pre-read, because
updateSegmentreturning null was the not-foundsignal, which is precisely what made the old order unavoidable: the route could not know the segment
existed until it had already tried to change it. The pre-read leaves a window where the row could
vanish between check and write — an event for a change that then did not happen, which is the side
A case action's event and its patch are two writes with no transaction — and the run path can lose the event #598's rule deliberately picks.
uploadEvidenceaudits before the BUCKET write too. An object in storage the ledger nevermentions is harder to notice than a missing row.
The rule finally has a test, and it did not before
Nine call sites had been flipped across three commits and not one test could tell. Every existing
test asserts the event EXISTS after a succeeding operation — equally true in either order — so a
reorder back was silent.
src/audit/audit-order.test.tsmakes the mutation fail and requires the event anyway. That is theonly externally visible difference between the two orders, and it is exactly what the rule promises.
Mutation-checked: reversing
createMeasureanduploadEvidenceeach fails its own case and nothingelse. A new audit-first path belongs in that file.
The route-level surfaces resolve their stores from
envrather than taking them injected, so theirordering is not reachable that way;
segments.test.tspins the enabling half instead — that theaudited entity id is the id the row is created under, and that
SEGMENT_UPDATEDreports the post-statemerged from the pre-state and the request rather than from a re-read.
Still mutate-first, with the reason at each call site
WARN.dispatchOutreachchannel.send()dispatches the message and the payload is built from the delivery result (status,messageId,provider,sentAt) — nothing to record beforehand, nothing to retract after. Needs ADR-073 d4's intent-then-completion pair, which adds an event type consumers read: an owner decision, not a reorder.upsertLinkreturns the existing row's id on conflict, so a caller-minted id is not the id the event would name. Keying these events on the pair — which is known beforehand — would work, and changes whatentity_idmeans for a consumer.backfill-scale·backfill-quality-historyChecked and NOT violations — every one a matcher artifact
audit-packet(a hash) ·materialize-runandbackfill-trend-history(reads) · evidence download(
arrayBuffer) ·measure-seed(itself audit-first, flagged against a different write's audit) ·subject-list create (its audit is a
beforeCompletecallback that runs before the list becomesvisible) · panel assignment, which audits before
upsertPanelAssignmentand records eachper-case event before
assignCases— the mapping-then-consequences order is by design.Which is the same lesson
feedback_a_sweep_is_worth_its_matcheralready carries, measured again: six ofthe eleven remaining candidates were not defects, and none of them could be told apart from the output.
This does not close #598
The sweep is fully triaged. What remains is the missing primitive — there is no
applyCaseAction({ patch, action, audit }), and there cannot be one inside a single store, because theaction and audit rows belong to
CaseEventStorewhile the patch belongs toCaseStore— plus the twodecisions above.
DATA_MODEL_CONTRACTS§4 andCLAUDE.mdnow say exactly that, in place of the older"an untriaged set remains".
Verification
backend-tssuitetsc --noEmitThe one failure is
corpus-membership.test.ts— this host's.official-contentsparse checkout,pre-existing and unrelated.