feat/energia-est-tune

Shell does not catch insert() throw — violates Result contract

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

findingQueries.insert() throws a FindingDatabaseError object (not returns err(...)) when INSERT ... RETURNING yields zero rows (queries.ts line 316). detectFindingsForOrgShell has no try/catch around the transaction body, so that throw propagates as an unhandled rejection. The function's declared return type is Promise<Result<DetectFindingsResult, DetectFindingsError>>, but the promise will reject rather than resolve to err(...). Callers (scripts/energia/detect-findings.ts line 105) use result.ok without any .catch(), so the script crashes with an unhandled-rejection stack trace rather than logging a typed error. Fixing this requires either (a) converting insert() to return Result<Finding, FindingDatabaseError> and propagating it, or (b) wrapping the transaction body in try/catch and mapping thrown FindingDatabaseError to err(...).

detectVatRecompute scans the full 24-month fetch window, not the 12-month detector window

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

Every other detector calls takeRecentPerRpu(input, DETECTOR_WINDOW_MONTHS) to slice to the last 12 months before iterating. detectVatRecompute iterates directly over the raw input array, which contains up to 24 months per RPU (FETCH_WINDOW_MONTHS = 24). This means VAT anomalies on bills from 13-24 months ago are flagged, while the same period is excluded from every other detector. The docstring does not document this intentional deviation from the 12-month window. In practice this produces findings anchored to old periods and inflates the finding count beyond what the corpus SQL produced. Deduplication by (contract:type:period) prevents duplicates on re-runs, but the scope inconsistency causes confusion about which window each detector covers.

detectConsumptionYoySpike distinctMonths count uses the full non-legado slice, not just 12 most-recent months

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

The guard 'if (distinctMonths < 12) continue' counts distinct year_month values across all non-legado rows in the fetched window (up to 24 rows), not just the most-recent 12. An RPU with data in months 2-13 (relative to latest) passes the >= 12 distinct-months guard, but when the prior-year same-month row is found at index ~13 in the array, it was included in the median/window calculations for the OTHER detectors via takeRecentPerRpu(input, 12). For yoy_spike this is intentional (the prior-year row must be reachable), but the '>= 12 months of history' claim in the docstring is imprecise — a bimonthly RPU could have 12 distinct months spanning 24 calendar months, which is 2 years of history, not the 12-month window the comment implies. More critically, the distinctMonths count is derived from 'desc' (non-legado rows, uncapped), while the median and window calculations inside detect* siblings only use the most-recent 12. This asymmetry means a bimonthly RPU with exactly 6 bills in its last 12 months but 12 bills total in 24 months passes the guard with fewer actual recent months.

Float comparison for estimatedImpact can cause spurious 'update' decisions on high-precision numerics

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

decideUpsertFinding converts the stored estimatedImpact (a string from the Postgres numeric column) to a JS number via Number(existing.estimatedImpact) and compares it to command.estimatedImpact (number). Postgres numeric can store arbitrary precision (e.g. '100.123456789012345'), while Number() truncates to 53-bit float. If the detector emits round2(value) (2 decimal places) but a previous run stored a slightly different precision value, the round-trip Number(string) comparison may produce a spurious 'changed = true' and trigger an unnecessary update every run. This wastes an update per affected finding on every detection pass and emits an unnecessary outbox event, breaking the idempotent-double-run guarantee described in the spec. The fix is to compare at the same precision: either store as round2 explicitly on insert (it already does round2 in detectors), or compare the string representations: String(round2(existingEstimated)) !== String(round2(command.estimatedImpact)).

detectFpRegimeChange: qualifying array is not bounded to the 12-month window

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

detectFpRegimeChange filters from 'all' (all rows for the RPU from groupByRpuDesc, which is the full 24-month fetch window) rather than from a slice bounded to DETECTOR_WINDOW_MONTHS. Unlike detectFpChronicPenalty (which explicitly does .slice(0, DETECTOR_WINDOW_MONTHS) on its non-legado subset), detectFpRegimeChange has no equivalent cap. The qualifying rows must be >= '2026-04', which provides a hard date floor (not month-count), so this does not produce stale detections. However, chargedSinceApr and the annualized estimatedImpact grow proportionally with the count of qualifying rows, which in the 24-month case covers the full post-April 2026 period in the fetch window. As the product matures (April 2027+), the 24-month window will include > 12 qualifying months, making the estimate > 12 months of data, while the docstring says 'since 2026-04' — not an error per se, but the unbounded-window behaviour is undocumented and inconsistent with sibling detectors.

Handler calls ensureModuleEntitled before resolveCurrentOrg — duplicate org resolution

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

Both listFindingsHandler and getFindingHandler call ensureModuleEntitled(database, authReq.auth, nextRequest, 'energia') then resolveCurrentOrg(authReq.auth.memberships, nextRequest) independently. ensureModuleEntitled internally must resolve the org to check module enablement, meaning the membership/org lookup executes twice per request. This is a performance inefficiency, not a functional bug. It also means if the caller's membership changes between the two calls (an extremely narrow race), the two could resolve different orgs. The canonical pattern used in other handlers is to resolve org once, then pass orgId to both entitlement check and query.

decideTransitionFindingStatus: self-transition to 'verified' requires re-supplying verifiedAmount

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

isValidFindingStatusTransition('verified', 'verified') returns true (self-transitions are always valid per line 183). But the verifiedAmount guard (lines 241-244) fires for ANY toStatus === 'verified', including the self-transition case. A caller sending { toStatus: 'verified' } on an already-verified finding (idempotency retry) must also supply verifiedAmount or get a VerificationEvidenceMissingError (400). Worse, if they supply verifiedAmount: X, the patch will set verifiedAmount = X, potentially overwriting the stored value with a different number. This is not currently exposed by any handler (PR2 is read-only), but when a status-transition handler ships (PR3+), this edge case will need explicit handling: idempotent self-transition to 'verified' should be a noop that preserves the existing verifiedAmount, not an overwrite.

detectFpChronicPenalty: annualPenalty sums the last-12 non-legado window but streak uses asc ordering of that window

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

The 'asc' array for streak detection is derived by reversing 'desc' (line 615: [...desc].reverse()), where desc = descNonLegado.slice(0, DETECTOR_WINDOW_MONTHS). This is correct. However, annualPenalty (line 635) uses 'desc' (the 12-row slice), while the streak uses 'asc' (also 12 rows, same data). If DETECTOR_WINDOW_MONTHS is changed asymmetrically between uses this could diverge, but as written the two are consistent. No correctness bug, but the dual iteration is a maintenance hazard: if a future dev changes the window for the streak but forgets to update annualPenalty, the MXN total and streak length will be computed over different windows, producing an incorrect payback estimate.

Bill-facts SQL: hired_demand_kw resolution prefers latest bill line-item over contract column

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

rpu_hired selects DISTINCT ON (rpu) the latest non-null hiredDemand bill line-item value, and bill_facts COALESCEs it over uc.hired_demand_kw (the contract column). This means ALL bills for the RPU use the same latest-bill hired demand, not each bill's own period demand. For isFpLargeUser() the threshold is 1000 kW, so if a client downsized from 1200 kW to 800 kW recently, historical bills at 1200 kW would still use the new 800 kW value, changing the umbral retroactively for past periods (from 95/97% back to 90%). This could cause detectFpRecompute to miss charges that were legitimate under the old umbral. The approach is documented (the SQL comment says 'latest bill hired kw'), but reviewers should be aware this is a known approximation, not an exact per-period lookup.

Platform-admin bypass skips org-scoping in finding reads

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

ensureModuleEntitled returns null immediately for isPlatformAdmin, then resolveCurrentOrg is still called and the resulting orgId is passed to the query layer. This means a platform admin is still scoped to their current membership org — they cannot inspect findings for other orgs from this endpoint. That is likely intentional parity with the tariff-rates handler, but it is undocumented and the handler comment says admins 'bypass the gate' which could mislead a future writer into removing the resolveCurrentOrg call. The real concern is the asymmetry: if a platform admin has no membership in an org, resolveCurrentOrg returns an error and they get 401 rather than being able to read findings org-by-org (e.g. for support triage). The script at scripts/energia/detect-findings.ts exists for that use case but is not the same interface. A clearer design would either (a) document that isPlatformAdmin only bypasses the commercial gate, not the org scope, or (b) add an optional ?orgPublicId admin override param.

Unscoped finding query functions exported from FCIS namespace are accessible without an orgId guard

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

findByPublicId (line 42), findByDedupeKey (line 47), findByContract (line 57), and findManyByDedupeKeys (line 75) all accept a raw DbOrTx with no orgId parameter, returning any finding regardless of org membership. These are exported via the FCIS namespace (finding/index.ts). Currently only the shell (detectFindingsForOrgShell) and future write paths call them, but they are visible to any handler author who imports FindingFCIS. If a future handler naively calls FindingFCIS.findingQueries.findByPublicId(database, publicId) without an org check, it leaks cross-org findings with no RLS backstop (the table runs via service-role). The risk is low today (no misuse exists) but the footgun is live in the exported surface. These functions should either not be exported from the FCIS namespace, or carry a prominent doc comment stating they are internal/shell-only and must never be called from a handler without org-scoping.

details JSONB blob is passed through to the API response without sanitisation

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

The response mapper passes finding.details (a Record<string, unknown> JSONB column) verbatim into the API response as FindingResponse.details. The wire schema in finding.schemas.ts declares it as z.record(z.string(), z.unknown()) — fully opaque. While the current detector implementations only store numeric/string evidence values (tariff codes, power factors, kWh values), the schema enforces nothing. A future detector could inadvertently store an internal UUID, an org identifier, or a sensitive reference in the details blob, and it would leak directly to clients. Consider either (a) restricting the details schema to a discriminated union of known detector evidence shapes, or (b) adding a strip step in toFindingResponse that removes any keys that match known internal-ID naming patterns.

Dedicated detect-findings runner script lacks access-level warning for --all mode

scripts/energia/detect-findings.ts:73

With --all, the script queries every non-deleted organization and runs detectFindingsForOrgShell against each. The script uses a DATABASE_URL that is typically a service-role connection (no RLS, no auth context). The script is a developer tool, documented as 'never prod', but there is no runtime guard that prevents it from being pointed at a production DATABASE_URL. A misfire with --all on a production database would silently upsert findings (dedupe makes it idempotent for re-runs, but it may trigger outbox events). The risk is operational rather than a security vulnerability per se, but the --all path deserves a 'this will write to ALL orgs — are you sure?' confirmation prompt to reduce the blast radius of an accidental prod run.

The open type filter is passed as a free-text exact match directly to a Drizzle eq() predicate

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

The list-findings API accepts type as a free-text string (documented as an open registry). It is passed through the handler to buildOrgScopedFindingsWhere where eq(findings.type, filters.type) is applied. Drizzle's parameterised queries prevent SQL injection here, so this is not an injection risk. However, because type is unrestricted, a caller can probe for undocumented internal detector type strings (e.g. speculating on type names like 'internal_audit'). This is a low-severity information-disclosure concern: the filter either returns results (confirming the type exists) or returns an empty list (no confirmation). Given the detector registry is in code, not truly secret, the practical impact is minimal. Documenting the known type strings in the API schema (or optionally restricting to a closed enum once the registry is stable) would harden this.

RLS select policy on utility_contract_findings does not filter deleted contracts or sites

packages/database/src/schema/utility-contract-findings.ts:116

The ucf_select_member RLS policy (line 116-119) joins site_utility_contracts to sites without filtering on sites.deleted_at or siteUtilityContracts.deleted_at. This mirrors the known pattern on bills/utility_contracts (SG-19), where the RLS policy is not the primary isolation gate anyway (the handler uses service-role + manual org-scope). For the ucf_select_member policy, a soft-deleted site's SUC rows could still grant authenticated read access to findings via the RLS sub-query, even after the site is removed from the org. The handler-layer query (buildOrgScopedFindingsWhere) also does not filter deleted sites. The handler is the real gate, and its org-scope subquery has the same gap. The consequence is that findings for contracts that were linked to a now-soft-deleted site remain visible to org members after the site is deleted. Whether this is intentional (the finding is still on the contract, just the site link went away) should be explicitly decided.

Service-role database use in finding handler is correctly justified (SG-19 pattern)

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

The handler correctly documents why it uses the raw service-role database (utility_contract_findings has no org_id; RLS gives nothing on the sites→SUC→contract JOIN for the same reason bills/utility-contracts use service-role). Org isolation is enforced manually via the org-scoped subquery in buildOrgScopedFindingsWhere, which mirrors the established SG-19 bill.queries.ts pattern. The approach is sound and well-documented. The ensureModuleEntitled gate is applied before the org resolution, providing an additional commercial gate on top of the data isolation.

Cross-org detection gap: findings carry no org_id and cross-org probe correctly returns 404

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

The GET /findings/:publicId handler correctly maps a cross-org or missing finding to 404 (not 403), preventing existence oracle leaks. The query findByPublicIdForOrgEnriched ANDs the publicId match with the org-scope subquery so a cross-org probe is indistinguishable from a missing row. This is correct per the tenant isolation rule (SITE_NOT_FOUND pattern).

Migration does not wrap CREATE POLICY in idempotency block

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

The migration emits bare CREATE POLICY statements (lines 40-41) without the DO $$ BEGIN ... EXCEPTION WHEN duplicate_object THEN null; END $$; wrapper documented in .claude/rules/migrations.md. On a fresh preview DB this is not a problem (first-run), but if migrations.md documents this as required for drizzle-kit's re-emit behaviour, these bare statements could fail on a branch DB that received the schema via another path. This is an operational nuisance rather than a security issue. The RLS policies themselves are correct in content.

Handler calls queries directly, bypassing shell layer

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

Both listFindingsHandler and getFindingHandler call FindingFCIS.findingQueries.listByOrgEnriched and findByPublicIdForOrgEnriched directly. The canonical rule is 'Handlers never call queries directly — always through shells.' The justification given (SG-19: findings have no org_id, service-role + manual org scoping) is valid from a security standpoint and mirrors the bills handler precedent, but the bills handler was explicitly flagged as a known gap pending SG-19 resolution. A thin read shell (e.g. listFindingsShell / getFindingShell) wrapping these queries would maintain the layering invariant and let the handler stay query-free, without requiring the full SG-19 fix. This is a convention deviation, not a correctness bug, but it sets a precedent that bypasses the FCIS boundary in a new entity instead of enforcing the pattern.

Mixed discriminant field between decision types in the same file

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

UpsertFindingDecision uses 'action' as its discriminant field (action: 'insert' | 'update' | 'noop'), while TransitionFindingStatusDecision uses '_tag' (the canonical convention for all discriminated types in this codebase). Within the same file, the two decision unions use different discriminant keys. The established pattern across the entire domain — error types, other decision types (see utility-contract.decisions.ts, bill.decisions.ts) — consistently uses '_tag'. This inconsistency will be confusing when the transition shell ships and the two union types are used side by side.

GET /findings (list) contract declares 404 but handler never returns it

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

The list route (GET /findings) declares a 404 response in the contract. The handler never returns 404 — a list over an empty result set returns 200 with an empty array. Per the api-patterns.md anti-pattern: 'Returning a status code not declared in the contract responses map' causes ts-rest to collapse it to 500; the inverse (declaring a status code the handler never returns) is benign but adds noise and misleads API consumers about the possible responses. The 404 makes sense on GET /findings/:publicId (detail) but not on the list route.

decideTransitionFindingStatus decision function is not backed by a shell

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

finding.decisions.ts defines decideTransitionFindingStatus with a full state machine (detected → in_action → verified | dismissed) and the corresponding Command/State/Patch/Decision types. These are exported from the index barrel. However, no transitionFindingStatusShell exists in finding.shells.ts — the shell file has only detectFindingsForOrgShell. This is acknowledged as intentional for PR2 (read-only surface), but the decision function is public API in the barrel, suggesting it was built ahead of time without the corresponding imperative shell or handler. When PR3 ships the write path, the shell will need to be added. For now, having a decision function that leads nowhere is a minor conventions concern (the FCIS model expects each decision to have a corresponding shell).

isValidFindingStatusTransition allows self-transitions from terminal state 'verified'

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

The guard 'if (from === to) return true' allows 'verified → verified' as a valid transition. The FINDING_STATUS_TRANSITIONS constant declares verified: [] (no allowed transitions), making 'verified' a terminal state by design. The self-transition bypass silently overrides this terminal guarantee — a shell calling decideTransitionFindingStatus with toStatus='verified' on an already-verified finding would succeed instead of returning an error. This could be intentional (idempotent re-verification) but it is not documented as such and conflicts with the comment 'verified → (terminal for PR2)'.

Detector functions are in finding.decisions.ts but exceed the pure-decision role

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

The canonical form for decisions.ts is: 'Pure business logic → Result<Decision, Error>'. The detector functions (detectEstimatedReadings, detectFpRecompute, detectFpChronicPenalty, etc.) are pure (no I/O) but they produce FindingCandidate[] rather than Result<Decision, Error>. They are neither operation-scoped decisions (decideCreate*, decideUpdate*) nor do they return Result types. Placing them in finding.decisions.ts is pragmatic and the file is clearly documented, but it blurs the convention boundary. A dedicated finding.detectors.ts (the test file __tests__/finding.detectors.test.ts already implies this split exists) would keep decisions.ts scoped to the two canonical operations (decideUpsertFinding, decideTransitionFindingStatus) and the detector logic in a clearly named module. This is a mild organization concern.

mapFindingReadError signature accepts only FindingNotFoundError, not the full union

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

mapFindingReadError is typed as accepting FindingFCIS.FindingNotFoundError (a single error type), while the exhaustive default branch references 'never'. When PR3 ships a status-transition handler that reuses this mapper, the author must widen the parameter type or create a new mapper. The comment says 'kept so a future write path that reuses this mapper is forced to handle every _tag', but a narrow parameter type is the opposite of future-proof — it will require an explicit widening. The mapper for read errors should either accept the narrowest possible type without a default branch (since there is only one read error) or accept the full TransitionFindingError union upfront. The current shape is misleading.

FCIS namespace is created at domain barrel level, entity index.ts uses flat re-exports

domains/utility/src/finding/index.ts:1

The canonical form says 'index.ts — Barrel export ({Entity}FCIS namespace)'. In this codebase the FCIS namespace is always created at the DOMAIN barrel (domains/utility/src/index.ts: export * as FindingFCIS from './finding'), not inside the entity's index.ts. The entity index.ts uses flat named re-exports. This is consistent with every other entity in the utility domain (BillFCIS, UtilityContractFCIS, etc.) and is the correct established pattern. The canonical-form.md description is slightly misleading on where the namespace is created, but the implementation is correct.

Event schema co-locates Zod schema with TS interface (ADR-018 exception documented)

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

finding.events.ts exports both a TypeScript interface (FindingDetectedEvent) and a Zod schema (FindingDetectedEventSchema) in the same file. The comment acknowledges this as an 'ADR-018 pragmatic exception'. This is consistent with how other domain event files are written (e.g. utility.subscription.events.ts). The satisfies z.ZodType<FindingDetectedEvent> annotation ensures type alignment. No action needed — the approach is documented and intentional.

Public ID prefix 'fnd' (3 chars) is correct per naming conventions

packages/database/src/schema/utility-contract-findings.ts:33

FINDING_PREFIX = 'fnd' follows the 3-char prefix convention. The ontology.md and domain CLAUDE.md do not list findings yet (new entity), but the prefix is consistent with the established pattern. No issue.

finding.type-check.ts AssertEqual will flag jsonb/numeric type drift correctly

domains/utility/src/finding/finding.type-check.ts:16

The type check asserts AssertEqual<DrizzleFinding, Finding>. Drizzle infers numeric columns as 'string | null' and jsonb with .$type<Record<string,unknown>>() as that exact type. Finding.type.ts has estimatedImpact: string | null, verifiedAmount: string | null, and details: Record<string, unknown> — these match the Drizzle inference. The type check is sound.

Shell integration test never verifies the @1→@2 detector-version upgrade path

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

The idempotent double-run test seeds a fresh estimated-reading bill and expects `inserted >= 1, updated == 0` on the second run — but this only exercises the fully-identical-row noop path. There is no test that (1) inserts an existing finding with `detectorVersion: 'estimated_reading@1'`, (2) re-runs `detectFindingsForOrgShell`, and (3) asserts `updated == 1` and the persisted finding now carries `detectorVersion: 'estimated_reading@2'` with `severity: 'info'`. This matters because the latest commit (feat(energia): demote generic estimated_reading to informational) bumped the version to @2 specifically to trigger re-detection of existing @1 findings in production databases. The upgrade path is the primary migration mechanism, yet it has zero test coverage.

Shell integration outbox test does not assert finding severity or detectorVersion

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

After calling `detectFindingsForOrgShell` and reading back the inserted finding, the outbox test only asserts `event.eventData.publicId`, `event.eventData.type`, and `event.eventData.period`. It never checks `finding.severity` or `finding.detectorVersion`. With the @2 demotion in place, an estimated-reading bill with no over-estimate history should produce `severity: 'info'`. Without asserting this end-to-end, a regression in the shell that hardcodes `severity: 'warning'` (as the @1 version did) would not be caught.

No test for the detector-version registry (resolveDetectorVersion)

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

The `DETECTOR_VERSIONS` map in `finding.shells.ts` is the sole source of detector version strings written to the database. It currently maps `estimated_reading` to `@2` (changed from `@1` in this PR). There is no unit test that asserts `resolveDetectorVersion('estimated_reading') === 'estimated_reading@2'` or that the fallback `${type}@1` fires for unknown types. A typo here silently writes the wrong version string for every finding, breaking the update-triggering mechanism on re-runs.

No unit tests for the finding response mapper (toFindingResponse / mapFindingReadError)

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

The contract test (`finding.contract.test.ts`) validates the Zod schema shape but does not exercise `toFindingResponse` or `mapFindingReadError`. The mapper is the boundary between domain types and the wire format. Key things left unverified: (a) `estimatedImpact` numeric-to-string conversion from the domain's Decimal type, (b) `verifiedAmount` null passthrough, (c) `createdAt`/`updatedAt` ISO string formatting, (d) that `mapFindingReadError`'s exhaustive switch returns a 404 for `FindingNotFound`. All other sibling mappers in this path (e.g., `cfe-jobs.mapper.test.ts`, `authorization.mapper.test.ts`) have dedicated tests — the finding mapper is the gap.

No handler-level tests for the findings handler

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

There are no tests for `listFindingsHandler` or `getFindingHandler`. The handler wires together `ensureModuleEntitled` (the 403 gate), `resolveCurrentOrg`, query calls, and the mapper. Untested paths include: (a) the 403 path when the org lacks `energiaEntitled`, (b) the 404 path when `findByPublicIdForOrgEnriched` returns null, (c) the 401 path when `profileId` is absent. Other handlers in this codebase also lack dedicated tests and rely on contract+integration coverage, but the commercial entitlement gate for a paid module is high-risk to leave entirely unverified.

detect-findings.ts script has no unit tests for its CLI parsing logic

scripts/energia/detect-findings.ts:31

The `parseArgs` function handles `--org`, `--org=<value>`, and `--all` with an early-exit guard. This is the entry point for manual operator runs and has no tests. The most relevant edge cases: (a) `--org` with no following argument (increments `i` past end of array, returns `org: null`), (b) missing `--` prefix on an unknown arg silently ignored, (c) both `--all` and `--org` provided simultaneously (no conflict check — `all` wins in the shell loop but `org` is also set, which the main function then ignores). Scripts are not normally unit-tested in this codebase, and this is in-line with other operational scripts, so this is low severity.

Contract test does not validate the 400/401/403/404 response body schemas

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

The test asserts `list.responses[400]` is `toBeDefined()` but never calls `.parse()` on a sample 400/401/403/404 body. The contract declares these codes as `JSendFailSchema(['INVALID_INPUT'])`, `JSendFailSchema(['UNAUTHORIZED'])`, etc. — but a drift in the error code string (e.g. a code not registered in `codes.ts`) would only fail at typecheck, not at the contract test. Comparing to sibling contract tests (`utility-contracts.contract.test.ts`), the pattern of only asserting existence rather than schema-parsing is consistent, so this is a systemic gap rather than a findings-specific one.

Integration test uses hard-coded 'estimated_reading@1' in directly-seeded findings

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

Several direct `findingQueries.insert()` calls throughout the integration test use `detectorVersion: 'estimated_reading@1'`. This is correct for testing the query layer directly (the insert accepts any string), but it means those fixtures are permanently one version behind the shell's `@2`. If a future test were added asserting a version string from a directly-inserted row, it would need updating. This is a documentation/clarity note rather than a bug.

All 8 detector types are covered by unit tests with firing, boundary, legado, and no-finding cases

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

Positive finding: all 8 corpus-verified detectors have dedicated unit tests in finding.detectors.test.ts covering the fire case, critical vs warning severity thresholds, legado exclusion, and no-finding (below-threshold) cases. The decisions test additionally covers detectEstimatedReadings and detectFpRecompute with boundary cases (exact +20% boundary, straddle month 2026-04, pf within 0.05 of umbral). The BillFactRow → SQL mapping is pinned by the integration-level fetchBillFactRowsForOrg test.

The estimated_reading demotion logic is well-covered at the decision unit level

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

Positive finding: the @2 severity model for estimated_reading (over-estimate warning, all else info) is thoroughly tested with 8 cases: (a) non-legado over-estimate → warning, (b) legado over-estimate → info, (b2) null isLegado → info, (c) null readingType → no finding, (d) non-legado under-estimate → info, (e) ESTIM* vocabulary variants, (f) exact +20% boundary → info, (g) no median available → info, (h) per-RPU independence. The demotion rationale is documented inline. The gap is at the integration and shell level (see high-severity findings above).

Outbox insert block duplicated verbatim for insert vs update paths

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

The `outboxQueries.insert(...)` call with identical event shape appears twice — once for the 'insert' action (line 190) and once for the 'update' action (line 226). The `eventData` object is structurally identical in both cases, differing only in that both already have `finding` in scope. Extracting a small `buildFindingEvent(finding, candidate, orgId)` helper would remove the duplication and make future event-shape changes a single-site edit. This is a maintenance risk: the two blocks could drift (e.g. PR3 adds a new field to one and forgets the other).

Custom isJsonEqual could be replaced with JSON.stringify comparison

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

`isJsonEqual` is a hand-rolled recursive deep-equality function (15 lines) used in a single place (`decideUpsertFinding`) to compare the `details` JSONB blob. Because `details` is a `Record<string, unknown>` that round-trips through Postgres JSONB — which normalises to a JSON-serialisable structure with deterministic key order per Postgres — `JSON.stringify(a) === JSON.stringify(b)` would be functionally equivalent for this use case and far simpler. The custom implementation has an edge-case correctness gap too: it sorts keys before comparing (`aKeys.sort()`), but does not verify that `bKeys[i]` matches `aKeys[i]` before recursing — it checks `key === bKeys[i]` in the same `every` predicate, which is correct but non-obvious. Consider replacing with a one-liner: `JSON.stringify(existing.details) === JSON.stringify(command.details)`. If key-ordering non-determinism from app-level construction is a concern, add a canonical serialiser utility shared across the domain.

groupByRpuDesc called redundantly: once in takeRecentPerRpu, once again per detector

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

Eight detectors that use the per-RPU window each call `groupByRpuDesc(input)` independently. Two detectors (`detectEstimatedReadings`, `detectFpRecompute`) do so indirectly via `takeRecentPerRpu`, and six more call it directly. Since every detector in the shell receives the same `input` array, the Map is rebuilt 8 times per detection run. For a typical org with dozens of RPUs and hundreds of rows this is O(n) per detector call (not O(n²)) and practically negligible, but passing the already-grouped Map as a parameter — or having the shell pre-group once — would make the cost explicit and testable in isolation. This matters more as detectors are added.

round2 and round4 are module-private duplicates of a pattern that exists in other domain files

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

`round4` (line 328) and `round2` (line 508) are file-private helpers. Identical parameterised `round(n, dp)` helpers already exist in `domains/cross-domain/src/bill-charts.shells.ts` (line 27) and `domains/metrics/src/metric/metric.fingerprint.ts` (line 92). These are good candidates for a shared utility in `domains/utility/src/lib/` (or a cross-domain math util). Low priority since they are trivial, but three independent implementations of the same 1-liner is a maintenance smell.

priorYearMonth helper duplicated across two domain files

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

`priorYearMonth(yearMonth: string): string | null` at line 351 is substantively identical to `priorYearMonth(ym: string): string` in `domains/cross-domain/src/savings-report-compute.shells.ts` (line 143). The only difference is the null-safety on a parse failure. This is a candidate for a shared `domains/utility/src/lib/year-month.ts` utility alongside the existing `lib/ulid.ts`.

pfs spread into Math.min/Math.max can stack-overflow on very large qualifying arrays

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

In `detectFpRegimeChange`, `Math.min(...pfs)` / `Math.max(...pfs)` spreads the `pfs` array into variadic arguments. For orgs with many RPUs and many qualifying months this can cause a call-stack overflow (JS engine limit on argument count is typically 65k–256k but varies). This is a theoretical concern for production data scales today, but `pfs.reduce((m, v) => Math.min(m, v), Infinity)` is the idiomatic safe form. Additionally, the `.filter((v) => v !== null)` on line 965 is redundant — the `qualifying` filter at line 951–959 already ensures `r.powerFactor !== null` via the `r.powerFactor !== null && r.powerFactor >= 95` guard — though TypeScript still needs the non-null assertion.

Single transaction wraps N sequential awaits — potential for long-held lock on large orgs

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

The write phase opens a single `db.transaction()` and executes one `findingQueries.insert` (or `updateEstimate`) plus one `outboxQueries.insert` per candidate sequentially inside it. For an org with 50 RPUs and 10 active detectors this could yield ~500 round-trips inside a single transaction, holding locks on `utility_contract_findings` and the outbox table for the full duration. A batched write approach — collecting all insert/update payloads and issuing bulk `INSERT ... VALUES (...)` — would reduce lock time significantly. This is a performance concern for larger enterprise orgs and is worth noting for the PR3 lambda trigger design.

detectFindingsForOrgShell declares DetectFindingsError return but never actually returns an error

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

The function signature is `Promise<Result<DetectFindingsResult, DetectFindingsError>>` and `DetectFindingsError = FindingDatabaseError`. However, the shell never returns `err(...)` — all DB failures are allowed to throw (the transaction's implicit throw path), not returned as typed errors. The `decideUpsertFinding` path that could `err` is `Result<_, never>` so it never fires. The `err` branch exists for the status-transition shell (not wired here). The signature is aspirationally correct (a future PR might surface DB errors as typed results), but currently the `DetectFindingsError` type is dead in this function. Consider documenting this as intentional or switching to `Result<DetectFindingsResult, never>` + a top-level `try/catch` until the error surface is wired.

Handler checks !authReq.auth.profileId before the module gate, but withAuth already guarantees profileId for dashboard routes

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

Both `listFindingsHandler` and `getFindingHandler` explicitly check `if (!authReq.auth.profileId) return unauthorized(...)` before calling `ensureModuleEntitled`. The `tariff-rates.handler.ts` follows the same pattern, so this is consistent with the established convention. However, `withAuth` on dashboard routes already validates the session and populates `profileId` — if `withAuth` resolved, `profileId` should always be present for human actors. The guard is defensive boilerplate. It is harmless and consistent with the codebase pattern, but worth flagging as potential simplification if `withAuth`'s contract is ever tightened to guarantee `profileId` for cookie-authenticated routes.

FindingCandidate and UpsertFindingCommand share almost identical shapes with structural duplication

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

`FindingCandidate` (line 297) and `UpsertFindingCommand` (line 56) share 8 of their 10 fields with identical names and types. The only differences are that `UpsertFindingCommand` adds `detectorVersion` and `dedupeKey`, while `FindingCandidate` adds `rpu`. The shell (finding.shells.ts, line 146–158) constructs a `UpsertFindingCommand` from a `FindingCandidate` by copying all shared fields. Consider defining `UpsertFindingCommand` as an extension of `FindingCandidate` (or vice versa via `Pick`/`Omit` + intersection) to make the relationship explicit and reduce the risk of the two types drifting when a field is added to one.

DETECTOR_VERSIONS registry and resolveDetectorVersion live in finding.shells.ts but are logically detector metadata

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

The `DETECTOR_VERSIONS` map and `resolveDetectorVersion` function encode detector-version metadata that conceptually belongs with the detector functions themselves in `finding.decisions.ts`. The current placement means adding a new detector requires changes in two files: the decision function in `finding.decisions.ts` and the version entry in `finding.shells.ts`. Co-locating the version string as a named export alongside each detector (e.g. `export const ESTIMATED_READING_DETECTOR_VERSION = 'estimated_reading@2'`) would make the coupling explicit and the registry auto-complete.

SQL result row type coercion pattern is repeated identically in two places

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

The expression `(result as unknown as { rows?: BillFactSqlRow[] }).rows ?? (result as unknown as BillFactSqlRow[])` at line 549–550 handles the Drizzle/postgres.js dual result shape. The same pattern appears in `scripts/energia/detect-findings.ts` (lines 77, 86, 153) and other files. This is a known Drizzle raw-SQL result shape inconsistency across adapters. A small typed helper `extractRows<T>(result: unknown): T[]` in a shared utility would prevent the cast from being copy-pasted across the codebase and give one place to update when the Drizzle adapter stabilises.