fix/dup-contracts
needs attentionviewing older commit1521c70 · incrementalPR #298reviewed 2026-07-14 00:00 UTC2H · 4M · 6L · 2I- Purpose
- Fix CFE pipeline deriving contract-classification evidence from bills[0] (arbitrary position), causing inverted TerminateAndReplace when backfill batches were led by old-regime XML. 18 contracts wrongly deposed Apr-Jun 2026.
- Goal
- Newest-evidence-wins guard: evidence bill = newest by periodEnd with a tariffCode; older evidence creates a terminated historical row rather than terminating the active contract. Plus SAVEPOINT recovery for 23505 race on concurrent contract inserts.
- Sub-goals
- SG-1: selectEvidenceBill — newest tariff-carrying bill drives classification
- SG-2: decideEnsureContractFromBills — CreateHistoricalContract path for stale evidence
- SG-3: insertContractRaceSafe — SAVEPOINT 23505 recovery
- SG-4: Event enrichment — add missing serviceName/status to all ContractCreated emission paths
- SG-5: Test coverage — unit matrix + integration suite for the guard + race recovery
- What
- Round-3: adds missing serviceName+status fields to WizardEnrich event emission; exports insertContractRaceSafe for test access; adds SAVEPOINT recovery integration tests; adds stable-sort tie-break test; fixes siteDisplayName to use billsNewestFirst[0] not input.bills[0].
- Why
- Round-2 loop-review identified missing required event fields and insufficient race-path test coverage.
- Areas
- domains/utility/src/utility-contract+235−48domains/utility/src/bill+396−10domains/utility/src/events+15−0docs/development+87−0
- Blast
- 12 files, +1347/-58 cumulative; utility domain only (contract shell, bill shell, events schema, tests). No API surface, no schema migration, no UI change.
Findings · 14
correctness5
status_changed outbox shape diverges from ContractStatusChangedEvent schema (pre-existing)
domains/utility/src/utility-contract/utility-contract.shells.ts:812
TerminateAndReplace emits { changes: { status: { old, new } }, reason } but schema declares { oldStatus, newStatus }. Three emission sites produce incompatible shapes; consumers parsing via ContractStatusChangedEventSchema silently miss oldStatus/newStatus.
adopted=true + contract=null permanently suppresses created event
domains/utility/src/utility-contract/utility-contract.shells.ts
If winning tx was rolled back, findByCompoundKey returns null → { contract: null, adopted: true }. The !adopted guard already skipped the event. No utility.contract.created is ever emitted for that row.
TOCTOU: findNewestXmlPeriodEnd races with concurrent bill inserts under READ COMMITTED
domains/utility/src/utility-contract/utility-contract.shells.ts:674
activeNewestXmlPeriodEnd read early in outer tx; concurrent newer bill committed between read and insert is invisible, causing stale evidence to fire TerminateAndReplace instead of CreateHistoricalContract.
siteDisplayName uses billsNewestFirst[0] instead of evidenceBill
domains/utility/src/bill/bill.shells.ts:880
billsNewestFirst[0] may differ from evidenceBill when the most-recent bill has no tariffCode. Prefer evidenceBill?.contractFields?.serviceName fallback.
Tie-break comment understates regime classification impact
domains/utility/src/bill/__tests__/bill.decisions.test.ts:1725
Comment notes tie does not change the depose decision but omits that the winner also determines which tariffCode drives regime classification.
security2
@internal export of insertContractRaceSafe is unenforced
domains/utility/src/utility-contract/utility-contract.shells.ts:71
Any deep import of utility-contract.shells can call the function without decision-layer pre-flight. @internal JSDoc is a convention, not a boundary.
CI status unavailable (PAT scope)
gh pr checks returned no data; status = na.
tests5
Race-adopted path: event suppression is untested
domains/utility/src/utility-contract/__tests__/ensure-contract-evidence.integration.test.ts:331
insertContractRaceSafe tests verify adopted=true and contract.id but never assert zero domainEvents for the adopted contract — the if (!adopted) guard preventing double-delivery is unverified.
Termination event for old contract not asserted in TerminateAndReplace test
domains/utility/src/utility-contract/__tests__/ensure-contract-evidence.integration.test.ts:207
Test adds assertions on replacement contract's created event but never queries the utility.contract.status_changed event for the terminated old contract.
No test for non-23505 error re-throw path in insertContractRaceSafe
domains/utility/src/utility-contract/__tests__/ensure-contract-evidence.integration.test.ts:297
The throw e branch for non-unique-violation errors is entirely untested; a regression that swallows all DB errors would be silent.
Unsafe cast before event field assertions
domains/utility/src/utility-contract/__tests__/ensure-contract-evidence.integration.test.ts:194
as Record<string, unknown> on eventData; if events[0] is undefined, assertions silently read undefined. Add expect(events[0]?.eventData).toBeTruthy() guard first.
Test suite title says SAVEPOINT but code uses Drizzle nested tx
domains/utility/src/utility-contract/__tests__/ensure-contract-evidence.integration.test.ts:297
Implementation uses tx.transaction() (Drizzle nested tx) not raw SAVEPOINT SQL; comment may mislead future maintainers.
improvement2
createdFrom and source are semantically redundant event fields
domains/utility/src/events/utility-contract.events.ts:32
WizardEnrich uses source; three other paths use createdFrom. Same concept, two spellings. Consolidate before schema stabilizes.
Four near-identical utility.contract.created outbox insertions
domains/utility/src/utility-contract/utility-contract.shells.ts
Extract emitContractCreated(tx, contract, provenance, actor) helper to avoid repeating six core fields four times.