fix/dup-contracts
needs attentionviewing older commitec4a791 · fullPR #298reviewed 2026-07-13 19:42 UTC3H · 8M · 8L · 2I- Purpose
- Fix production bug: 18 active contracts wrongly terminated by stale-evidence backfill batches (bills[0] as evidence source, June 2026)
- Goal
- Newest-evidence guard prevents historical backfills from deposing active contracts; 23505 SAVEPOINT recovery eliminates insert races
- Sub-goals
- SG-1: Sort bills by periodEnd desc, pick newest with tariffCode as evidence
- SG-2: CreateHistoricalContract decision for stale-evidence batches
- SG-3: SAVEPOINT-based 23505 race recovery on all contract inserts
- SG-4: Historical contract inherits active contract site instead of minting new one
- SG-5: 6 unit decision cases + 3 integration scenarios
- What
- bill.shells.ts: evidence selection; utility-contract.decisions.ts: CreateHistoricalContractDecision + evidencePeriodEnd guard; utility-contract.shells.ts: insertContractRaceSafe + CreateHistoricalContract branch; bill.queries.ts: findNewestXmlPeriodEnd; new integration test file; expanded decision unit tests
- Why
- 18 contracts wrongly terminated on prod. Fix must ship before Phase 2 data repair script, or re-inverted RPUs would undo the repair.
- Areas
- domains/utility/src/utility-contract+512−16domains/utility/src/bill+62−9docs/development+87−0
- Blast
- 7 files, +661/-25 lines. All in domains/utility. No schema or API contract changes.
Findings · 21
correctness3
Duplicate outbox event when race-recovery adopts competing transaction's contract
domains/utility/src/utility-contract/utility-contract.shells.ts:815
When `insertContractRaceSafe` catches a 23505 in the TerminateAndReplace or CreateHistoricalContract branch, the outer code still emits `utility.contract.created` for the adopted contract ID. The competing transaction that won the race has already committed its own event for the same `aggregateId`, resulting in two `utility.contract.created` events for the same contract. If downstream consumers are not idempotent on this event type they will double-process. Fix: skip the outbox insert when the contract was adopted from a race recovery (detectable by tracking whether the row was freshly inserted or pre-existed).
Historical contract may spawn a new site when active contract has no site links
domains/utility/src/bill/bill.shells.ts:861
When `activeSites` is empty (active contract exists but has no site in this org — possible for wizard-created contracts), `linkedToActiveSite` stays `false` and execution falls through to `createSiteShell`, minting a new site for the historical contract. The subsequent pipeline run for the same RPU's active contract will then create yet another site, producing two sites for one physical location. For `createdHistorical` actions where the active contract has no site, the correct behavior is to skip site creation rather than mint an orphaned one.
Staleness guard silently bypassed for legacy callers omitting evidencePeriodEnd
domains/utility/src/utility-contract/utility-contract.decisions.ts:556
`evidenceIsStale` requires `input.evidencePeriodEnd !== undefined`. When omitted, the guard evaluates false regardless of `activeNewestXmlPeriodEnd` and falls through to TerminateAndReplace — exactly the bug being fixed. The current production caller always passes it, but any future direct caller of `ensureContractFromBillsShell` that omits `evidencePeriodEnd` silently gets the old broken behavior. Consider making the field required, or documenting the bypass explicitly in the JSDoc.
security4
findActiveByContractNumber is not org-scoped — foreign-org contract can influence staleness guard
domains/utility/src/utility-contract/utility-contract.shells.ts:649
`findActiveByContractNumber` resolves an active contract purely by RPU (no org filter). In a multi-tenant system a single RPU can be associated with multiple orgs. If Org B's pipeline processes a batch for an RPU that Org A also holds, `activeNewestXmlPeriodEnd` is fetched from Org A's contract, and the staleness guard gates Org B's decision on Org A's bill timeline. The active contract resolution should use the org-scoped `findByRpuAndOrgId` that already exists in the queries file.
findNewestXmlPeriodEnd carries no org boundary — relies on caller to pass org-scoped contract ID
domains/utility/src/bill/bill.queries.ts:1178
The function aggregates `MAX(period_end)` for any contract ID without an org predicate. It is safe only when the passed `contractId` was already resolved within the current org's scope. Combined with the unscoped `findActiveByContractNumber` finding above, this creates a path where bill metadata from another org's contract influences a decision in this org's pipeline. Adding an optional `orgId` parameter (join through SUC→sites) or enforcing the invariant at the query level would eliminate the dependency on call-site discipline.
TOCTOU: activeNewestXmlPeriodEnd fetch and contract insert are not atomic
domains/utility/src/utility-contract/utility-contract.shells.ts:664
The newest XML period is fetched at FETCH time, a decision is made, and then the insert happens later. Under READ COMMITTED isolation, a concurrent transaction inserting a newer XML bill for the active contract between these steps would leave the guard stale, potentially allowing a TerminateAndReplace that should have been blocked. The window is narrow and the consequence is recoverable and audit-logged, but this is a new gate introduced by this PR — the race window is now meaningful.
Zero UUID sentinel for system actor degrades audit trail on site_utility_contracts rows
domains/utility/src/bill/bill.shells.ts:868
`createdBy: actor.actorId ?? '00000000-0000-0000-0000-000000000000'` records a nil UUID when the actor is a system actor (actorId=null). Unlike NULL, which signals 'unknown', the nil UUID is a valid-looking but unresolvable FK, polluting audit queries. The new `createdHistorical` site-linking path at line 868 is new code extending this pre-existing pattern. Consider using NULL where the schema allows it or a named system-actor sentinel that is clearly not a profile UUID.
conventions4
CreateHistoricalContractDecision not exported from the barrel
domains/utility/src/utility-contract/index.ts
All other `EnsureContractFromBillsDecision` arm types (UseExistingContractFromBillsDecision, CreateContractFromBillsDecision, EnrichWizardContractDecision, TerminateAndReplaceDecision) are individually exported. `CreateHistoricalContractDecision` is defined but absent from the barrel export. Any coordinator or consumer that wants to import it by name cannot, forcing them to import the full union or re-declare the type.
utility.contract.created outbox event for historical contract is missing serviceName
domains/utility/src/utility-contract/utility-contract.shells.ts:757
Every other `utility.contract.created` outbox payload in this file includes `serviceName`. The new `CreateHistoricalContract` branch omits it. If `ContractCreatedEventSchema` declares `serviceName` as required, downstream EventBridge consumers will fail schema validation and silently drop the event. Add `serviceName: historical.serviceName` to the eventData block.
EnsureContractFromBillsState.activeNewestXmlPeriodEnd is optional but semantically required when there's an active contract
domains/utility/src/utility-contract/utility-contract.decisions.ts:428
Declaring the field `readonly activeNewestXmlPeriodEnd?: string | null` (doubly-optional) makes 'absent' and 'null (no XML evidence)' indistinguishable in the decision. The shell always resolves and passes it, so `string | null` (required, no `?`) would tighten the contract and prevent a future caller from accidentally omitting it and silently falling through to TerminateAndReplace.
No assertNever guard on EnsureContractFromBillsDecision dispatch chain
domains/utility/src/utility-contract/utility-contract.shells.ts:684
The shell dispatches via `if (d._tag === ...)` blocks without a final `assertNever(d)` call. TypeScript cannot verify exhaustiveness on this pattern. Adding an `assertNever` at the end of the dispatch chain would turn a silent runtime miss into a compile-time error when a future arm is added to the union.
tests6
No outbox event verification in integration tests — shell atomicity guarantee untested
domains/utility/src/utility-contract/__tests__/ensure-contract-evidence.integration.test.ts:141
The new integration tests verify returned contract state but none query `domain_events` to assert that `utility.contract.created` was written atomically. The ADR-016 shell contract requires the outbox write in the same transaction as the entity mutation. If the outbox insert were accidentally removed or the transaction rolled back, no test would catch it. The `CreateHistoricalContract` branch emits a distinct event with `historicalBackfill: true` and `activeContractId` — these fields should be asserted.
Site-linking for historical contracts is completely untested
domains/utility/src/bill/bill.shells.ts:861
The new `createdHistorical` site-linking branch (bill.shells.ts ~861-874) lives in `persistBatchBillsShell`, not in `ensureContractFromBillsShell`. The new integration tests only call the shell directly, so this path is never exercised. A `persistBatchBillsShell` integration test with a pre-existing site-linked active contract and stale-evidence batch is needed to validate site inheritance, org scoping, and idempotency.
No test for activeSites empty branch — silent fallthrough to createSiteShell
domains/utility/src/bill/bill.shells.ts:864
When the active contract has no site links in this org, `activeSite` is undefined and execution silently falls to `createSiteShell`. There is no test exercising `createdHistorical` with an active contract that has zero org-scoped site links, so the wrong-site or duplicate-site regression described in the correctness findings would not be caught.
Idempotency test does not verify active contract is untouched after second stale call
domains/utility/src/utility-contract/__tests__/ensure-contract-evidence.integration.test.ts:205
The 'repeated stale backfill' test verifies `second.value.action === 'found'` and contract id reuse, but does not re-fetch and assert the active contract's status and version after the second call. A regression that deposes the active contract during the second call would not be caught by this test.
No test for evidence selection when newest bill lacks tariffCode
domains/utility/src/bill/bill.shells.ts:721
`billsNewestFirst.find((b) => b.tariffCode)` falls back to an older bill when the newest lacks a tariffCode. In that case `evidencePeriodEnd` would be an older period — which could incorrectly trigger the stale-evidence guard and route a legitimate TerminateAndReplace to `CreateHistoricalContract`. This scenario is not tested in bill.shells.test.ts.
No unit tests for findNewestXmlPeriodEnd edge cases
domains/utility/src/bill/bill.queries.ts:1178
The gating function has no direct tests: (1) contract with zero XML bills → should return null; (2) contract with both xml and inferred/payment_check bills → only xml rows counted; (3) contract with multiple xml bills → returns MAX, not first inserted. A schema change to the `source` enum value would silently break the guard.
improvement4
Redundant dual-null guard on evidenceBill/tariffCode
domains/utility/src/bill/bill.shells.ts:724
`!tariffCode || !evidenceBill` is redundant: `tariffCode` is derived as `evidenceBill?.tariffCode`, so both are falsy iff `evidenceBill` is absent. A single `!evidenceBill` (or a `!evidenceBill?.tariffCode` guard) would suffice and avoid implying they can be independently falsy.
Unnecessary ?? null coercion before !== null check
domains/utility/src/utility-contract/utility-contract.decisions.ts:555
`const activeNewest = state.activeNewestXmlPeriodEnd ?? null` then `activeNewest !== null` can be simplified to `state.activeNewestXmlPeriodEnd != null` (loose inequality), eliminating the intermediate variable. The current form adds a step that obscures intent.
TxLike could use the exported Transaction type from @batu/shared-kernel
domains/utility/src/utility-contract/utility-contract.shells.ts:47
`type TxLike = Parameters<Parameters<Database['transaction']>[0]>[0]` is opaque. `@batu/shared-kernel` exports `Transaction` (and `DbOrTx`). Using `import type { Transaction } from '@batu/shared-kernel'` is more readable and grep-able.
findNewestXmlPeriodEnd could be batched into the context-loader query
domains/utility/src/utility-contract/utility-contract.shells.ts:664
This is a separate round-trip inside the FETCH phase. At current volumes negligible, but worth noting if the contract load query is ever revisited for batching.