fix/rpu-unique
needs attentionviewing older commitcc8af77 · incrementalPR #321reviewed 2026-07-17 00:34 UTC0H · 1M · 3L · 1I- Purpose
- Add the agreements-model DB backstop (partial unique index uq_utility_contracts_one_active_per_rpu) so a concurrent double-activation of an RPU agreement chapter surfaces as a 23505 instead of silently forking the RPU's active state. Gated on clean census: prod (339 dup groups, all NO_ACTION_OK) and staging (13 dup groups, all NO_ACTION_OK) verified 2026-07-16.
- Goal
- At most one active agreement chapter per RPU, enforced at DB level. The decision layer already guarantees this; the index makes a concurrent race an explicit error.
- Sub-goals
- SG-1: Partial unique index migration (0061) — CREATE UNIQUE INDEX WHERE status='active' AND deleted_at IS NULL
- SG-2: Write-order fix in ReactivateContract — demote-first then reactivate-second to avoid holding two active rows at a statement boundary
- SG-3 (this commit): Race error identity — activeChapterRace() constructor, ONE_ACTIVE_PER_RPU_CONSTRAINT constant, fresh-create race test, demoted contract state assertions, doc cleanup
- What
- Round-2 fixes: (1) new ContractErrors.activeChapterRace() constructor returning ContractVersionConflict shape with RPU as publicId and version sentinels 0/0, replacing versionConflict(rpu,-1,-1); (2) export COMPOUND_KEY_CONSTRAINT and ONE_ACTIVE_PER_RPU_CONSTRAINT constants from shells to eliminate string-literal duplication; (3) two new tests — insertContractRaceSafe propagates ONE_ACTIVE_PER_RPU 23505 instead of adopting, and fresh-create path maps race to 409; (4) demoted contract post-update state assertions (status + version); (5) ctx.skip() for DB backstop test on pre-migration DBs; (6) doc update removing decideEnsureContractForTariff (zero callers, removed 2026-07-16), clarifying reactivation path priority.
- Why
- Round-1 review identified: race error was using versionConflict(-1,-1) (misleading sentinels), constraint names were string literals (typo risk), no test verified that insertContractRaceSafe propagates the one-active violation, and the backstop test silently greened on pre-migration DBs.
- Areas
- domains/utility/src/utility-contract+282−280packages/database+10−0domains/cross-domain/src/__tests__+2−2.claude/rules + domains/utility/CLAUDE.md+14−8
- Blast
- 10 files, +308/-290 across utility-contract domain layer, DB migration, cross-domain integration test, and docs. No API surface changes, no handler changes, no UI changes. The DB migration adds a partial unique index and is backward-compatible on clean data.
Findings · 5
conventions2
activeChapterRace shares _tag with versionConflict — breaks discriminated-union identity
domains/utility/src/utility-contract/utility-contract.errors.ts:164
ADR-016 discriminated unions require _tag to uniquely identify the error shape. activeChapterRace emits _tag: 'ContractVersionConflict' with expectedVersion: 0 / actualVersion: 0 as sentinels — callers cannot distinguish a real version mismatch from a concurrent-index-violation by _tag alone. A distinct tag (e.g. 'ContractActiveChapterRace') preserves the discriminated-union invariant while both can still map to a retryable 409 at the handler layer. The current approach works but will silently mislead any future caller that branches on the error shape post-_tag.
DB constraint name constants exported from shells layer
domains/utility/src/utility-contract/utility-contract.shells.ts:47
COMPOUND_KEY_CONSTRAINT and ONE_ACTIVE_PER_RPU_CONSTRAINT are infrastructure constants (DB index names from migration 0061). Canonical form assigns shells the role of 'Transaction orchestrators: fetch → decide → write + outbox' — constraint name literals are query-layer details and belong in utility-contract.queries.ts. The integration-test-only justification is valid for the export, but the location couples schema awareness to the orchestration layer.
tests2
TerminateAndReplace race path not tested for 409 mapping
domains/utility/src/utility-contract/__tests__/ensure-contract-evidence.integration.test.ts:640
The new 'maps a one-active-per-RPU race on fresh-create to ContractVersionConflict (409)' test only exercises the CreateContractFromBills branch. The shell's outer catch also covers the TerminateAndReplace insert path — a concurrent activation between the terminate-old write and insert-new write would also hit ONE_ACTIVE_PER_RPU_CONSTRAINT and must map to 409. Without a test: set up an active contract (routes to T&R), mock contractQueries.insert to throw the constraint violation, assert result.error._tag === 'ContractVersionConflict' with statusCode 409. A regression removing the outer catch would surface as a 500 on this path.
compoundSpy guard is redundant with the rejects assertion
domains/utility/src/utility-contract/__tests__/ensure-contract-evidence.integration.test.ts:613
In 'insertContractRaceSafe PROPAGATES', compoundSpy = vi.spyOn(contractQueries, 'findByCompoundKey') has no mock implementation and calls the real function. The not.toHaveBeenCalled() assertion only fires after the rejects assertion already passed — it provides no independent signal. If findByCompoundKey were accidentally called, the rejects assertion would fail first. The guard is not wrong, but is not an independent test gate.
improvement1
Unreachable `return` after `ctx.skip()`
domains/utility/src/utility-contract/__tests__/ensure-contract-evidence.integration.test.ts:555
ctx.skip() throws in Vitest to signal a skip — the return on the following line is dead code. Remove it to avoid misleading readers into thinking return is the skip mechanism.