fix/rpu-unique
needs attentionviewing older commit6352bc8 · incrementalPR #321reviewed 2026-07-17 18:44 UTC0H · 3M · 5L · 1I- Purpose
- Enforce the agreements-model invariant — at most one active contract chapter per RPU — at the DB level, turning silent concurrent double-activations into catchable 23505 errors.
- Goal
- Add uq_utility_contracts_one_active_per_rpu partial unique index, fix ReactivateContract write order, discriminate constraint in race-safe insert, map to 409, remove dead decideEnsureContractForTariff, and achieve full integration test coverage.
- Sub-goals
- SG-1: Migration + schema — partial unique index uq_utility_contracts_one_active_per_rpu
- SG-2: ReactivateContract write order — demote-first / reactivate-second (mirrors T&R's terminate-then-insert)
- SG-3: insertContractRaceSafe — adopt only on compound-key 23505; propagate one-active 23505
- SG-4: Shell boundary catch — map one-active 23505 to ContractVersionConflict (409)
- SG-5: Remove decideEnsureContractForTariff (zero callers; would have created second active rows)
- SG-6: Integration test coverage — DB backstop, fresh-create race, T&R race, Reactivation OCC rollback
- What
- Adds a partial unique index on utility_contracts.contract_number WHERE status='active' AND deleted_at IS NULL. Rewrites ReactivateContract to demote-first/reactivate-second. Discriminates the constraint name in insertContractRaceSafe to separate adoption (compound-key race) from conflict propagation (one-active race). Maps the propagated 23505 to ContractVersionConflict (409) in the shell boundary catch. Removes dead decideEnsureContractForTariff/ensureContractForTariff. Adds 5 new integration tests covering every race path.
- Why
- Census validation on 2026-07-16 confirmed zero duplicate active groups in prod and staging, making the migration safe. The existing decision-layer guarantee needed a DB backstop to catch concurrent races that bypass per-transaction state checks.
- Areas
- domains/utility/src/utility-contract/+258−276domains/utility/CLAUDE.md+7−4packages/database/drizzle/+8−0packages/database/src/schema/utility-contracts.ts+9−0.claude/rules/ontology.md+7−4domains/cross-domain/src/__tests__/cfe-job-intent.integration.test.ts+2−2
- Blast
- 12 files, +362/-106 lines. Focused on domains/utility/src/utility-contract/ and packages/database/. No API surface changes — the new error maps to an existing 409 response type. No handler/contract changes required.
Findings · 10
correctness2
Mock bypasses the real SAVEPOINT-through-constraint path it documents
domains/utility/src/utility-contract/__tests__/ensure-contract-evidence.integration.test.ts
The test comment claims the one-active violation 'propagates to the shell's boundary catch' via the SAVEPOINT, but the mock throws a JS Error before any SQL runs — no real Postgres 23505 inside a nested SAVEPOINT is exercised. What is actually tested is the isUniqueViolation check on a synthetic error object and the outer-tx rollback. The SAVEPOINT-recovery path (that a real DB 23505 leaves the outer transaction usable) is not covered. This is a shared gap with the fresh-create race test; not new here, but the comment overstates the guarantee.
Early-return guard silently skips the atomicity assertion when result.ok is unexpectedly true
domains/utility/src/utility-contract/__tests__/ensure-contract-evidence.integration.test.ts
if (result.ok) return; inside the try block exits before the rollback assertion. If the shell erroneously returns ok, expect(result.ok).toBe(false) would fail — but the atomicity check (findById?.status) is never reached. This makes the unique correctness property of the test contingent on the earlier assertion catching the regression. Replace with: expect(result.ok).toBe(false); expect(result.error._tag).toBe('ContractVersionConflict'); then run the DB check unconditionally.
security2
Module-level spy on contractQueries creates inter-file isolation risk in concurrent workers
domains/utility/src/utility-contract/__tests__/ensure-contract-evidence.integration.test.ts
vi.spyOn(contractQueries, 'insert') mutates the shared module export. If Vitest runs this file in the same worker as another file importing contractQueries, mockRejectedValueOnce could be consumed by an unrelated call. mockRejectedValueOnce limits blast radius to one call and finally guarantees restore, so real-DB state is not corrupted — but a parallel test could get a spurious error. Mitigated by restoreAllMocks: true (see finding above) and confirmed worker isolation in vitest.config.ts.
Confirm shell does not surface raw constraint_name to HTTP callers
domains/utility/src/utility-contract/utility-contract.shells.ts
The synthetic error carries constraint_name = ONE_ACTIVE_PER_RPU_CONSTRAINT. The test correctly checks that the message only contains the RPU string. This is already correct in ContractErrors.activeChapterRace — confirmed that no internal schema details (pg code, constraint name) leak through to API responses.
conventions1
Missing call-count assertion for findByCompoundKey / findActiveByContractNumber
domains/utility/src/utility-contract/__tests__/ensure-contract-evidence.integration.test.ts
The insertContractRaceSafe PROPAGATES test asserts compoundSpy was not called to prove the error did not silently fall through to the adopt branch. The T&R race test omits this pin. A spy on findByCompoundKey (or findActiveByContractNumber) with a .not.toHaveBeenCalled() assertion would guard against a future regression where the shell re-enters a recovery branch after catching the constraint error.
improvement2
try/finally spy teardown boilerplate — use restoreAllMocks instead
domains/utility/src/utility-contract/__tests__/ensure-contract-evidence.integration.test.ts
The file has 7+ spy declarations each wrapped in try/finally for mockRestore. Vitest's restoreAllMocks: true (or afterEach(() => vi.restoreAllMocks())) would auto-restore every vi.spyOn after each test, removing all the try/finally boilerplate. The current pattern is error-prone: a future early return inside a try block silently leaves the spy active, poisoning subsequent tests in the sequential run. Set restoreAllMocks: true in the package vitest.config.ts.
oneActiveViolation object constructed identically in three separate tests
domains/utility/src/utility-contract/__tests__/ensure-contract-evidence.integration.test.ts
Object.assign(new Error('duplicate key value'), { code: '23505', constraint_name: ONE_ACTIVE_PER_RPU_CONSTRAINT }) appears verbatim in three adjacent tests. Extract as a describe-block-level const makeOneActiveViolation() factory so the constraint name is a single point of change.
test-coverage3
Missing outbox atomicity assertion for the rolled-back termination event
domains/utility/src/utility-contract/__tests__/ensure-contract-evidence.integration.test.ts
The test checks active.status is still 'active' (row-level rollback) but does not assert that no utility.contract.status_changed event was written to domainEvents for active.id. The shell writes the termination outbox event in step 3a — before the failing insert in step 3b. Both OCC rollback tests in the same file assert events.toHaveLength(0) for all affected IDs. Add: const events = await database.select().from(domainEvents).where(eq(domainEvents.aggregateId, active.id)); expect(events).toHaveLength(0);
active.version not asserted after rollback
domains/utility/src/utility-contract/__tests__/ensure-contract-evidence.integration.test.ts
Only status is checked post-rollback. If step 3a's updateWithVersion incremented the version column before the transaction rolled back (partial write), the status check alone would not detect it. Both analogous rollback tests assert version === original.version for every affected row. Add: expect((await contractQueries.findById(database, active.id))?.version).toBe(active.version).
Mock error uses top-level code/constraint_name only — .cause-nested Postgres driver path untested
domains/utility/src/utility-contract/__tests__/ensure-contract-evidence.integration.test.ts
The actual Postgres driver wraps PG fields under e.cause.code / e.cause.constraint_name. The mock uses Object.assign(new Error(...), { code, constraint_name }) at the top level. isUniqueViolation handles both branches, but the .cause path is exercised by neither this test nor the fresh-create race test. A second variant of the mock with { cause: { code: '23505', constraint_name: ONE_ACTIVE_PER_RPU_CONSTRAINT } } would complete coverage of both driver shapes.