feat/energia-api

Concurrent detection runs can throw unhandled 23505 unique-constraint violation

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

The db.transaction() block in detectFindingsForOrgShell has no try/catch and the insert query has no ON CONFLICT clause. If two concurrent runs process the same org simultaneously, both see no existing finding for a given dedupeKey (step 3, outside the transaction), both decide 'insert', and the second transaction throws a postgres 23505 unique-constraint violation on uq_ucf_dedupe_key. This exception propagates as an unhandled rejection — not wrapped in err() — breaking the declared Promise<Result<..., DetectFindingsError>> contract. Fix: add ON CONFLICT (dedupe_key) DO NOTHING to the insert (and skip the outbox event on conflict), or wrap the transaction body in try/catch and convert 23505 to a typed DetectFindingsError.

detectInactiveServiceCharges over-estimates annual impact 2× for bimonthly RPUs

domains/utility/src/finding/finding.decisions.ts:909

estimatedImpact = round2(12 * avgNet) assumes monthly billing. Bimonthly RPUs (accounts 01–40) produce 6 bills/year, not 12. For avgNet=$500/bill, the reported impact is $6,000 when actual is $3,000. The over-estimate is 2×. The corpus SQL source has the same limitation; at minimum document the assumption in a JSDoc comment.

verified→verified self-transition permitted despite verified being terminal

domains/utility/src/finding/finding.decisions.ts:183

isValidFindingStatusTransition returns true when from===to. FINDING_STATUS_TRANSITIONS.verified is [] (terminal). This allows re-calling the transition endpoint with toStatus='verified' and a new verifiedAmount on an already-verified finding (a version bump still occurs). If verified is truly terminal in PR2, the from===to short-circuit should be conditional: skip the early-true for 'verified'.

Unenforced findByPublicId exported alongside org-scoped variant — future misuse risk

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

findingQueries exports findByPublicId(db, publicId) with NO org scoping. A future handler that reaches for this simpler function (instead of findByPublicIdForOrgEnriched) would return findings from any org to any authenticated energia-entitled caller. Same footgun documented for bills in domains/utility/CLAUDE.md §SG-19. Consider removing from the exported object or renaming to signal unsafe scope.

Handler calls findingQueries directly, bypassing shell abstraction

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

Both handlers call FindingFCIS.findingQueries.listByOrgEnriched and .findByPublicIdForOrgEnriched directly. The canonical form rule states: 'Handlers never call queries directly — always through shells.' For read-only paths this is common in practice, but the convention is to expose reads through thin shell-level functions rather than surfacing the raw findingQueries object. The queries ARE behind the FCIS namespace (borderline compliant), but a thin withFindingOrg() shell would enforce the pattern and enable future pre/post hooks.

insert query throws on null row instead of returning a Result

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

if (!row) throw FindingErrors.databaseError('insert') violates the FCIS rule 'never throw — return Result<T, E>'. The thrown value (a domain error object, not an Error instance) would bubble as an unhandled rejection through db.transaction(). The shell's comment acknowledges the case but the fix should be Result-based, not a throw — return null and let the shell map it to err().

mapFindingReadError accepts only FindingNotFoundError — the exhaustiveness guard is vacuous

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

The function parameter type is the single FindingNotFoundError, making the default: never branch unreachable by construction. The comment says this is intentional for PR2, but the never annotation misleads future readers into thinking the switch is genuinely exhaustive over a union. Use the full FindingReadError union as the parameter type so the switch IS exhaustive — adding a new read-path error then requires a new case.

FindingsListQuerySchema missing satisfies z.ZodType<FindingsListQuery> annotation

packages/api/src/schemas/finding.schemas.ts:139

All response schemas use 'satisfies z.ZodType<T>' per api-patterns.md. FindingsListQuerySchema is missing this annotation. A new filter field added to FindingsListQuery but forgotten in the schema would not be caught at compile time.

No integration test for shell's UPDATE path on re-detection with changed numbers

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

The idempotent double-run test only covers the noop path. There is no integration test that runs detectFindingsForOrgShell twice with different bill data (changed kwhPerDay, subtotal) and verifies value.updated===1 and the finding row is mutated correctly. The update path in finding.shells.ts is a currently untested branch — a column-mapping bug would let unit tests pass and the double-run noop test pass, but real re-detections silently fail to persist updated impact numbers.

Noop guard for non-null estimatedImpact (string→number round-trip) untested

domains/utility/src/finding/__tests__/finding.decisions.test.ts:170

Finding.estimatedImpact is string|null (Postgres numeric stored as string); UpsertFindingCommand.estimatedImpact is number|null. The noop comparison converts existing.estimatedImpact via Number() before comparing. The only noop test uses null for both sides. No test covers existing='100.00' vs command=100 (correct noop) or floating-point precision edge cases. If Number('100.000') !== 100 due to any rounding, every re-detection of a finding with non-null impact produces a spurious update and outbox event.

dismissed→detected re-open path via detectFindingsForOrgShell never integration-tested

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

decideUpsertFinding does NOT update status on re-detection (never-regress-status guarantee). A dismissed finding that re-fires should keep its status='dismissed'. There is no test that (a) inserts a finding, (b) transitions it to 'dismissed', (c) runs the shell again, (d) asserts status stays 'dismissed'. If the shell were ever changed to reset status, no test would catch the regression — a billing-domain invariant with no guard.

No unit test for isJsonEqual with nested arrays in details

domains/utility/src/finding/__tests__/finding.decisions.test.ts:194

The noop-with-key-ordering test only tests flat object reordering. The isJsonEqual implementation branches on Array.isArray. No test verifies array-valued details fields (e.g. {periods: ['2026-01','2026-02']}). An off-by-one in the array comparison branch would produce phantom updates on every re-detection run, emitting spurious outbox events.

detectFpChronicPenalty: no test for 12-month window truncation

domains/utility/src/finding/__tests__/finding.detectors.test.ts:76

Tests only pass 3-4 rows. No test verifies that a 13th-month bill is dropped from the streak and annual-penalty calculations. A bug in the takeRecentPerRpu sort or slice would over-count the penalty — inflating estimatedImpact and possibly escalating severity to critical on billing-domain data.

No integration test for fetchBillFactRowsForOrg XML-only source filtering

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

The existing test verifies source='pdf' bills are excluded, but not source='payment_check' or source='inferred'. These stub types must not appear in detector input. A refactor that accidentally widens the WHERE to OR source='inferred' would pass existing tests but feed fabricated historical periods to detectors, generating false billing-error findings.

detectFpRegimeChange: no boundary test at the $1,000 threshold

domains/utility/src/finding/__tests__/finding.detectors.test.ts:410

The isDeadPaid predicate uses chargedSinceApr > 1000 (strict). Tests use 500 (clearly below). No boundary test at exactly 1000 (excluded) vs 1001 (fires). Standard practice for billing thresholds.

detectConsumptionYoySpike: trendVsRecentPct (3-month proxy) never asserted

domains/utility/src/finding/__tests__/finding.detectors.test.ts:142

Tests assert yoyPct, severity, period, billId but never details.trendVsRecentPct. This field uses desc.slice(1,4) — a subtle off-by-one or slice boundary error would produce wrong/null trendVsRecentPct silently. While not used for severity, it is a customer-visible diagnostic field.

detectInactiveServiceCharges: no boundary test at totalNet=$200

domains/utility/src/finding/__tests__/finding.detectors.test.ts:346

isDeadPaid requires totalNet > 200 (strict). Test uses 150. No boundary test at 200 (excluded) vs 201 (fires). A billing-critical threshold for 'dead meter' classification.

No contract test for type filter vocabulary — 10 detector type strings untested

apps/platform/src/api/contracts/__tests__/finding.contract.test.ts:27

The type filter is a free-string (no closed enum at contract layer), so an unknown type silently returns 200+empty instead of 400. Tests only use 'estimated_reading'. None of the other 9 detector type strings are verified to round-trip through the filter correctly.

detectDapPresent: no test verifying that a DAP charge in month 13 (outside window) is excluded

domains/utility/src/finding/__tests__/finding.detectors.test.ts:476

detectDapPresent uses all.slice(0, 12). No test verifies that a DAP charge only in month 13 does NOT produce a finding, or that estimatedImpact reflects only the windowed months. If the slice were accidentally removed, impact would be inflated.

FP_ELIGIBLE_TARIFFS is redundant — equals HT ∪ MT union

domains/utility/src/finding/finding.decisions.ts:466

FP_ELIGIBLE_TARIFFS is provably identical to FP_LARGE_USER_HT_TARIFFS ∪ FP_LARGE_USER_MT_TARIFFS. A future addition to one of the sub-sets without adding it to FP_ELIGIBLE_TARIFFS would silently skip the recompute check for that tariff. Derive FP_ELIGIBLE_TARIFFS from the other two sets or remove it and inline the union check.

powerFactorBonusMxn fetched, mapped, typed — but read by no detector

domains/utility/src/finding/finding.decisions.ts:280

BillFactRow declares powerFactorBonusMxn; the SQL query projects it and the query mapper maps it. No detector references it. If reserved for a future detector, add a comment. Otherwise, removing it reduces the SQL projection, type surface, and mapper noise — the BillFactRow contract should be honest about what detectors actually consume.

Handler auth prologue duplicated across list and get handlers

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

Both handlers share a 4-step prologue: withAuth → !profileId → ensureModuleEntitled('energia') → resolveCurrentOrg. A small withEnergiaOrg(request, db, callback) helper would collapse ~10 lines into 1 per handler and make the entitlement+org-resolution always-paired. Low cost now (2 handlers), high value when write handlers (status transitions, notes) land in PR3+.

Math.min/max spread over qualifying array is fragile for very large arrays

domains/utility/src/finding/finding.decisions.ts:956

Math.min(...pfs) / Math.max(...pfs) hit the JS argument-count limit on very large arrays (RangeError). In practice the fetch cap is 24 months and qualifying filters to post-2026-04 large users (tiny array). Defensive pattern: pfs.reduce((m,v) => Math.min(m,v), Infinity).