← all branches

fix/job-active

needs attentionviewing older commit
c7b4e31 · incrementalPR #314reviewed 2026-07-16 01:03 UTC0H · 1M · 5L · 3I
The branch
Purpose
Fix a cluster of correctness bugs in utility contract lifecycle management: active-first RPU resolution, evidence-driven reactivation, OCC WHERE enforcement, and now the OCC diagnostic branching.
Goal
Make contract shell operations race-safe with correct optimistic concurrency control — including the diagnostic re-read that distinguishes retryable conflicts (live row) from terminal failures (missing or soft-deleted row).
Sub-goals
  • SG-1: active-first RPU resolution + evidence-driven reactivation
  • SG-2: OCC enforcement in updateWithVersion WHERE clause
  • SG-3: updateContractShell surfaces OCC null as ContractVersionConflict
  • SG-4: guard demotion write + close review-round-1 test gaps
  • SG-5: only a LIVE row is a retryable version conflict + test both diagnostic branches
The changes (whole branch)
What
Added !current.deletedAt guard at two OCC diagnostic re-read sites (updateContractShell and statusWriteError). Added two integration tests for the live→409 and null→500 paths. Removed superseded OCC test from ensure-contract-evidence.
Why
Soft-deleted contracts were producing retryable 409 responses, which would send callers into infinite retry loops against a dead contract row. The fix ensures only live contracts are retryable.
Areas
apps/platform+19−5docs/development+205−0domains/cross-domain+389−10domains/utility/src/utility-contract+746−19domains/utility/src/bill + monitoring+110−4services/utility/bills/cfe+3−1
Blast
20 files, +1472/-39 across domains/utility (utility-contract core), domains/cross-domain (job intent + contract wizard shells), apps/platform (CFE job handlers), services/utility (CFE lambda). No public API schema changes.
➖CI· No check results available➖CodeRabbit· No .coderabbit.yaml✅correctness✅tests· 409 + 500 paths tested; one untested branch (soft-deleted → 500)✅conventions✅security✅improvement➖seo

Findings · 10

correctness1

info

!current.deletedAt is correct for Date | null type

UtilityContract.deletedAt is Drizzle timestamp() → Date | null. A non-null Date is always truthy; no truthy-but-non-Date scenario exists.

security2

low

409/500 discrimination is a weak oracle (pre-existing, not introduced here)

domains/utility/src/utility-contract/utility-contract.shells.ts

An authorized caller racing an update can infer soft-delete state from the error code. Exploitable only within a narrow window by an already-authenticated actor. Pre-existing behavior.

low

versionConflict 409 exposes actualVersion (pre-existing)

The 409 response body includes expectedVersion and actualVersion. Low sensitivity but broader than a strict retry signal requires. Not introduced by this diff.

conventions1

low

Branch name in test comment will rot post-merge

domains/utility/src/utility-contract/__tests__/utility-contract.integration.test.ts:395

Comment reads 'With OCC enforced in updateWithVersion's WHERE (fix/job-active)'. Replace with 'with the version guard in updateWithVersion's WHERE clause'.

tests5

medium

Soft-deleted row → 500 branch untested

domains/utility/src/utility-contract/__tests__/utility-contract.integration.test.ts:420

Three diagnostic outcomes exist: null → 500 (tested), live → 409 (tested), soft-deleted → 500 (not tested). A future refactor returning a distinct error for the soft-deleted case would be silently uncovered.

low

originalFind passthrough fragile w.r.t. this-binding

domains/utility/src/utility-contract/__tests__/utility-contract.integration.test.ts:421

mockImplementationOnce(originalFind) works today as findByPublicId is a plain function. A (...args) => originalFind(...args) wrapper makes the delegation explicit and future-safe.

low

409 test has implicit ordering assumption (testContract must be live)

domains/utility/src/utility-contract/__tests__/utility-contract.integration.test.ts:398

Diagnostic re-read is not mocked in the 409 test — hits real DB and relies on testContract not being soft-deleted by a prior test in the same describe block.

info

Consolidation from ensure-contract-evidence is structurally correct

The removed test lived in a file scoped to ensureContractFromBills and imported updateContractShell as a guest — odd placement. Moving both OCC diagnostic tests to utility-contract.integration.test.ts is the right structural call.

info

findSpy.toHaveBeenCalledTimes(2) correctly counts both shell calls

The shell calls contractQueries.findByPublicId twice in the OCC null path (step-1 fetch + diagnostic re-read). Both go through the same spied function. The assertion accurately pins this.

improvement1

low

500 test: pair of mockResolvedValueOnce cleaner than originalFind capture

domains/utility/src/utility-contract/__tests__/utility-contract.integration.test.ts:421

Pre-fetch the contract value, then use .mockResolvedValueOnce(contract).mockResolvedValueOnce(null) to avoid function-identity coupling.

History · 6 commits

  1. 3e1361aneeds attentionincremental0H · 1M · 0L2026-07-16 01:14
  2. c7b4e31needs attentionincremental0H · 1M · 5L2026-07-16 01:03current
  3. 2099ea7needs attentionincremental0H · 3M · 5L2026-07-16 00:53
  4. ce2dc3eneeds attentionincremental0H · 1M · 1L2026-07-16 00:44
  5. df0ef09needs attentionincremental0H · 2M · 3L2026-07-16 00:23
  6. d4a8d84needs attentionfull0H · 3M · 5L2026-07-16 00:04