← all branches

fix/dup-contracts

needs attentionviewing older commit
ec4a791 · fullPR #298reviewed 2026-07-13 19:42 UTC3H · 8M · 8L · 2I
The branch
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
The changes (whole branch)
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+51216domains/utility/src/bill+629docs/development+870
Blast
7 files, +661/-25 lines. All in domains/utility. No schema or API contract changes.
production-bug-fix backwards-compatible no-migration-required
ci· No CI checks availablecoderabbit· No .coderabbit.yaml

Findings · 21

correctness3

medium

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).

medium

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.

low

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

medium

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.

low

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.

low

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.

low

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

medium

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.

medium

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.

low

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.

low

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

high

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.

high

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.

high

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.

medium

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.

medium

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.

medium

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

low

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.

low

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.

info

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.

info

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.

History · 4 commits

  1. a15a13asafeincremental0H · 0M · 2L2026-07-14 00:21
  2. 1521c70needs attentionincremental2H · 4M · 6L2026-07-14 00:00
  3. 5d0ec62needs attentionincremental2H · 4M · 3L2026-07-13 20:31
  4. ec4a791needs attentionfull3H · 8M · 8L2026-07-13 19:42current