feat/energia-api

Count query in `listByOrgEnriched` omits the INNER JOIN on `utilityContracts` — count and page results can diverge

domains/utility/src/finding/finding.queries.ts:233

The paginated rows query INNER JOINs `utilityContracts` (enrichment); the count query runs a plain `.from(findings).where(orgScopedWhere)` without that join. If a finding's FK row is ever dangling, the rows query silently drops the finding while the count query still counts it, causing `total` to exceed what can actually be paginated. Edge-case today (FK has ON DELETE CASCADE), but structurally fragile. Add the same contract existence guard to the count subquery.

Lost-update race in 'update' branch silently swallowed — no log, no signal to operator

domains/utility/src/finding/finding.shells.ts:223

When `updateEstimate()` returns null (optimistic lock rejected), the fix correctly skips `updated += 1` and the outbox event. But the lost update is swallowed silently — no `log.warn`. For a re-entrant detector run this is safe (next run will retry), but if the concurrent writer is a human-triggered status transition, the detector's evidence update is permanently lost without any signal. Add at minimum a warn log: `log.warn('finding update lost-update race', { findingId })`.

No composite indexes on `(utility_contract_id, category)` or `(utility_contract_id, severity)`

packages/database/drizzle/0062_sloppy_ma_gnuci.sql:38

Migration provides `idx_ucf_contract_status (contract_id, status)` but not indexes for `category` or `severity` filters. `listByOrgEnriched` with those filters will scan the org's findings via `utilityContractId IN (subquery)` without a covering index. At current volume this is acceptable; flag as a follow-up before the table reaches production scale.

Unscoped `findByPublicId` and `findByContract` exported in `FindingFCIS` — footgun for future handler authors

domains/utility/src/finding/index.ts

Both queries perform no org-scope check. Current handlers correctly use `findByPublicIdForOrgEnriched` and `listByOrgEnriched`. Any future handler author who accidentally uses `findByPublicId` or `findByContract` via `FindingFCIS.findingQueries` silently bypasses org isolation. Consider removing them from the public export or adding an `_unscoped` suffix to signal intent.

`details` JSONB verbatim in API response — no documented invariant preventing internal ID leakage

apps/platform/src/api/mappers/finding.mapper.ts:33

Current detectors store only business metrics (amounts, percentages, reading types) in `details`. But the schema types it as `z.record(z.string(), z.unknown())` with no further constraint. Add a comment on the detector base type or the schema documenting the 'no internal UUIDs/ARNs in details' invariant so future detectors don't inadvertently leak internal IDs.

RLS policy correct — `ucf_select_member` uses sites→SUC chain (better than the broken bills model)

packages/database/drizzle/0062_sloppy_ma_gnuci.sql:41

The policy gates SELECT via `site_utility_contracts → sites → get_user_org_ids()`. This is the correct, populated path — notably better than `bills`, whose RLS uses `entity_relationships` (documented as never populated). Cross-org isolation under RLS is actually functioning for this table. Integration tests confirm with the positive-control RLS pattern (owner can see own data before asserting cross-org returns empty).

No authenticated INSERT/UPDATE/DELETE RLS — correct by design; shell runs as service_role

packages/database/drizzle/0062_sloppy_ma_gnuci.sql:40

RLS is enabled. With only `ucf_service_role_all` (write) and `ucf_select_member` (read), PostgreSQL default-denies all unmatched operations. An authenticated Supabase JWT cannot insert/update/delete findings via PostgREST. Detection shell runs via service-role `database` handle (bypasses RLS), which is correct.

SQL injection fix in `detect-findings.ts` is correct

scripts/energia/detect-findings.ts:130

Replaced `sql.raw(ARRAY[...uuid...])` (string interpolation of DB-sourced ids) with `sql.join(orgs.map(o => sql\`${o.id}::uuid\`), sql\`, \`)`. Drizzle's `sql` template tag parameterizes interpolated values as `$N` placeholders — org UUIDs never splice into the query text. The `::uuid` is a SQL fragment suffix on the placeholder, not part of the value.

`mapFindingReadError` parameter typed as concrete `FindingNotFoundError` — exhaustiveness guard in default branch is inert

apps/platform/src/api/mappers/finding.mapper.ts:44

The parameter is typed as `FindingNotFoundError` (single concrete type, not a union). The `switch(error._tag)` default branch assigns to `never`, but TypeScript already narrowed `_tag` to `'FindingNotFound'` before the switch — so the default is dead code with no compile-time enforcement. The canonical peer mappers (bill, contract) type the parameter as the full error union so the switch is genuinely exhaustive. Either: (a) type the parameter as the full `FindingReadError` union (today that's only `FindingNotFoundError`, but the guard then works when the union grows), or (b) remove the misleading `default: never` guard and document the deliberate single-error narrowing.

GET /findings list contract declares `404` response code the list handler never emits

apps/platform/src/api/contracts/finding.contract.ts:45

The list route declares `404: JSendFailSchema(...)`. The list handler only returns 200/401/403. A list returning 'not found' is semantically wrong (empty list is the correct shape). This will advertise a spurious 404 in the generated OpenAPI spec, misleading API consumers. Remove the 404 from the list route responses.

Unrelated exports piggybacked in `domains/utility/src/index.ts`

domains/utility/src/index.ts

Added: `TAX_REGIMES`, `TAX_REGIME_RATES`, `TaxRegime` (billing IVA regime, PR #302) and `TaxRateFCIS`, `ContractActiveChapterRaceError` (contracts race error, PR #323). These were merged separately and their barrel re-exports surfaced here via branch merge. No correctness issue, but the PR description should acknowledge them so reviewers know what they are.

Shell `updated` counter fix has no regression test — a revert would be invisible

domains/utility/src/finding/__tests__/finding.integration.test.ts:418

The only assertion on `result.value.updated` is `expect(second.value.updated).toBe(0)` on idempotent re-run. There is no test that drives the UPDATE path and asserts `updated ≥ 1`. The commit that fixed the counter placement (`updated += 1` moved inside `if (finding)`) has no green test that would red if it were reverted. Add: insert an estimated-reading bill, run the shell (inserted ≥ 1), mutate the bill data, re-run the shell, assert `updated ≥ 1 && inserted === 0`.

`insertEstimatedReadingBill` uses `Math.random() * 1e6` for publicId — birthday collision risk in parallel CI

domains/utility/src/finding/__tests__/finding.integration.test.ts:207

`publicId = 'bil_' + testTimestamp + Math.floor(Math.random() * 1e6)`. `testTimestamp` is fixed at module load, so uniqueness rests on 6 random digits across all calls. With N=10+ calls the birthday-collision probability is ~0.005% — low but non-zero under parallel Vitest workers sharing the same DB. `insertFullFactBill` already uses `1e9` (1000× safer). Fix: use `randomUUID()` (already imported) for the publicId suffix in `insertEstimatedReadingBill`.

No handler-level tests for `listFindingsHandler`/`getFindingHandler` — entitlement gate (403) is untested

apps/platform/src/api/handlers/finding.handler.ts

The `ensureModuleEntitled(database, auth, request, 'energia')` gate, the `resolveCurrentOrg` 401 path, and the `mapFindingReadError` 404 path from `getFindingHandler` have no integration or E2E tests. The contract tests are schema-only. The 403 contract declaration is load-bearing (PR description) but the gate itself has no test confirming it fires correctly.

Outbox test relies on sequential test ordering rather than structural per-test isolation

domains/utility/src/finding/__tests__/finding.integration.test.ts:460

`findingsForContract[0]` is taken as the anchor for the event lookup. If `detectFindingsForOrgShell` inserts multiple findings (race or setup side-effect), `[0]` may not point to the expected single finding. Use `findingsForContract.length === 1` assertion or query the outbox directly by `orgId + eventType` without going through `findByContract`.

CONCEPT_NAMES manually maintained — no compile-time or runtime drift detection vs LATERAL

domains/utility/src/finding/__tests__/finding.integration.test.ts:91

The comment says the array must stay in lockstep with the `bcc.concept_name IN (...)` list in `fetchBillFactRowsForOrg`'s LATERAL. If a new concept is added to the LATERAL but not to `CONCEPT_NAMES`, `ensureConcepts()` won't seed it, and the LATERAL will silently return null for that column — the detector will never fire in tests. No structural enforcement. Consider extracting the concept list to a shared constant imported by both the query and the test, or add a runtime assertion in the test setup.

`insert()` throws a domain error object instead of returning null — breaks ADR-016 Result contract

domains/utility/src/finding/finding.queries.ts:316

`if (!row) throw FindingErrors.databaseError('insert')` throws a plain discriminated-union object (not a JS `Error` instance) from inside a query. The shell (`detectFindingsForOrgShell`) has no try/catch around its `db.transaction()` block and declares `Promise<Result<DetectFindingsResult, DetectFindingsError>>` as its return type. When the throw propagates out of the transaction, the shell rejects instead of resolving to `err(…)`. ADR-016: 'never throw exceptions for business errors'. The canonical peer (`bill.queries.ts`) returns `null` from `insert()` and lets the shell return `err(BillErrors.databaseError('insert'))` after testing the null. Fix: make `finding.queries.insert()` return `Promise<Finding | null>` and handle the null in the shell.

Duplicate test helpers `addContractToOrg` / `createContractForOrg` — one should be deleted

domains/utility/src/finding/__tests__/finding.integration.test.ts:180

Both helpers insert a utility-contract row + SUC wired to a fixture site and push the id onto `createdContractIds`. The only difference: `addContractToOrg` generates the contract number via `uniq('RPU_'+label)` internally; `createContractForOrg` accepts the full number from the caller. Consolidate to one helper with an optional `contractNumber` override or just call `createContractForOrg(org, uniq('BYCONTRACT_C2'))` at the one `addContractToOrg` site.