fix/dup-contracts
safea15a13a · incrementalPR #298reviewed 2026-07-14 00:21 UTC0H · 0M · 2L · 3I- Purpose
- Fix a bug where CFE bill persist pipeline derived contract evidence from bills[0] (arbitrary array position), causing old-regime XMLs to terminate active contracts ('inverted replacement'). 18 contracts wrongly deposed in prod Apr–Jun 2026.
- Goal
- Newest-evidence guard for terminate-and-replace + 23505 SAVEPOINT race recovery
- Sub-goals
- SG-1: persistBatchBillsShell — evidence from newest bill by periodEnd with a tariffCode
- SG-2: decideEnsureContractFromBills — TerminateAndReplace only when evidence is at least as recent; older evidence → CreateHistoricalContract (already-terminated, active contract untouched)
- SG-3: 23505 race recovery — contract inserts in SAVEPOINT; unique-violation adopts concurrently-created row
- SG-4: Test hardening — pin race-suppression test to create branch and verify spy intercept count
- What
- Added expect(result.value.action).toBe('created') and expect(spy).toHaveBeenCalledTimes(2) to the race-adopted event-suppression test; condensed the setup comment.
- Why
- Previous version of the test had a false-positive gap: if the findByCompoundKey spy failed to intercept, the shell routed UseExistingContract (action='found'), which also produces 0 events — passing the test without ever exercising the if (!adopted) guard.
- Areas
- domains/utility/src/utility-contract+620−48domains/utility/src/bill+503−10domains/utility/src/events+15−0docs/development+87−0
- Blast
- 12 files, +1405/−58 total; all changes in domains/utility (contract decisions/shells/tests, bill decisions/shells/tests, events) and docs. No schema changes, no API contract changes, no cross-domain or infra changes.
Findings · 5
correctness2
spy call-count is fragile if preResolvedCtx path is ever used
domains/utility/src/utility-contract/__tests__/ensure-contract-evidence.integration.test.ts:386
expect(spy).toHaveBeenCalledTimes(2) correctly models the current code: 1 call in the fallback FETCH path + 1 recovery re-fetch inside insertContractRaceSafe. However, if the shell is ever invoked with a preResolvedCtx (the fast path), the FETCH call to findByCompoundKey is skipped — the spy would be called only ONCE, breaking this test with no obvious connection to the race-suppression intent. The test does not pass preResolvedCtx today, so this is a latent fragility. A brief inline note — 'no preResolvedCtx passed, so fallback path fires findByCompoundKey once in FETCH' — would make the 2-call invariant self-explanatory to future readers.
Shortened comment omits causal role of 'terminated' status in routing
domains/utility/src/utility-contract/__tests__/ensure-contract-evidence.integration.test.ts:349
The original comment noted 'terminated, so the active-RPU lookup misses too' — explaining why the shell routes to CreateContractFromBills rather than TerminateAndReplace. The new comment mentions 'terminated' but drops this causal link. A regression reader might not immediately understand why the terminated status matters to the decision branch taken.
conventions1
Assertion ordering: spy count (internal mechanics) sits between observable-result assertions
domains/utility/src/utility-contract/__tests__/ensure-contract-evidence.integration.test.ts:386
Current order: result.ok → action → contract.id → spy count → DB events. Grouping observable-result assertions (action, contract.id) together before the mechanistic assertion (spy count) would improve readability. Not a defect — style preference only.
tests2
action === 'created' meaningfully closes a real false-positive gap
domains/utility/src/utility-contract/__tests__/ensure-contract-evidence.integration.test.ts:383
Without this assertion, if the spy failed to intercept (e.g. mockResolvedValueOnce consumed early), the shell routes to UseExistingContract (action='found'), which also writes 0 events — making the final events check pass silently. Pinning action='created' ensures the test only passes via the create→23505→adopted path it is designed to exercise.
spy.toHaveBeenCalledTimes(2) is accurate and non-flaky under current shell
domains/utility/src/utility-contract/__tests__/ensure-contract-evidence.integration.test.ts:386
Maps precisely to: (1) FETCH-phase findByCompoundKey (mocked to null) + (2) insertContractRaceSafe recovery re-fetch (passthrough). Spy is scoped and restored in try/finally — no bleed across tests.