fix/tariff-cov

Catch on Finalize + oversized $.perRateType could defeat the very protection it adds

infra/cdk/src/stacks/services/utility/tariffs/cfe/stepfunctions.stack.ts:403

When Finalize catches an error, `ResultPath: '$.error'` merges the error into the existing state, which still carries `$.perRateType` (the full Map output). The Finalize comment explicitly names 'an aggregated payload over the state-size limit' as a scenario being protected against. But if the perRateType payload was what pushed state past SFN's 256 KB limit, the Catch execution could itself exceed the limit before reaching FailJob — leaving the job stuck in `running`. Common failure modes (DB down) are well under 256 KB, so this is an edge-within-an-edge, but worth a note in the runbook.

Empty `divisions: []` silently falls back to survivor denominator

services/utility/tariffs/cfe/src/handlers/tariff-transition.lambda.ts:244

`divisions: []` (empty array, distinct from null) yields `resolveTargets([])` → targets=[], so `expectedPairs = 0 * rateTypes.length = 0`. The fallback is `totals.divisionsDiscovered ?? null` — the survivor-denominated value the commit fixes. This is likely unreachable if the job schema rejects empty arrays at creation (null = all, non-empty array = subset), but that guard should be confirmed. If reachable, the fallback silently reintroduces the bug.

UNKNOWN_DIVISION 'division' field carries the requested name, not a zone name

services/utility/tariffs/cfe/src/handlers/tariff-collector.lambda.ts

For matched-but-failed divisions, failures carry the individual zone name. For UNKNOWN_DIVISION, the failure carries the raw requested name (e.g., 'ATLANTIS'). Intentional and tested, but noteworthy: any downstream that joins failures on zone name will not find a match for UNKNOWN_DIVISION entries.

Unmatched names carry the un-trimmed raw string in failure messages

services/utility/tariffs/cfe/src/lib/divisions.ts:73

`wanted = requested.map(r => ({ raw: r, key: r.trim().toUpperCase() }))`. A name with trailing whitespace ('BAJIO ') will match via `key` but appear in `unmatched` and failure messages as the un-trimmed 'BAJIO ' (with space). Low risk given operator-controlled inputs, but trimming `raw` too would be more consistent.

Operator-supplied division name reflected verbatim in failure messages stored in SFN state

services/utility/tariffs/cfe/src/handlers/tariff-collector.lambda.ts

The raw operator-supplied division name is embedded in `UNKNOWN_DIVISION` failure messages, which flow into SFN execution state and CloudWatch Logs. This is an internal operator input path with no injection risk, but an oversized string could bloat SFN state and logs. Capping the name at a reasonable length (e.g., `name.slice(0, 200)`) before embedding it is a low-cost guard.

Catch on States.ALL writes Lambda error objects (including stack traces) into SFN execution history

infra/cdk/src/stacks/services/utility/tariffs/cfe/stepfunctions.stack.ts

`ErrorEquals: ["States.ALL"]` captures Lambda service errors, timeouts, and throttles, putting raw error objects (potentially including stack traces or internal ARNs) into `$.error` in SFN execution history. Ensure `states:GetExecutionHistory` on this state machine is restricted to ops/admin IAM roles. This is an IAM review item, not a code change.

Re-export of lib symbols from handler file is a leaky abstraction

services/utility/tariffs/cfe/src/handlers/tariff-collector.lambda.ts:64

`export { resolveTargets, type DivisionConfig } from '../lib/divisions'` is a transitional shim so collector tests (which import from the handler) keep working. But a Lambda handler is an entry-point boundary, not a library surface. The comment already admits this is a crutch. Tests in collector.test.ts should import from `../src/lib/divisions` directly, and the re-export should be removed. Leaving it creates an implicit API contract on the handler file that makes future removal a breaking change.

`expectedPairs > 0` fallback path is untested

services/utility/tariffs/cfe/__tests__/transition.test.ts

The production handler uses `expectedPairs > 0 ? expectedPairs : (totals.divisionsDiscovered ?? null)`. No test exercises the `expectedPairs = 0` branch (e.g., a job created with `rateTypes: []` or all divisions unrecognized). In that branch, the handler silently falls back to the survivor-denominated value — exactly the bug this commit fixes. If this branch is legitimately unreachable, a guard or assertion at the call site would make that explicit. If reachable, a test should cover it.

No test exercises the finalize handler wiring end-to-end

services/utility/tariffs/cfe/__tests__/transition.test.ts

The denominator arithmetic is tested as a pure calculation against the library functions (`zoneCount(resolveTargets(...))`) but the full handler path — receiving `results`, calling `aggregatePersistResults`, computing `expectedPairs` from `job.config`, and passing it to `finalizeTariffJobShell` — is never exercised with a stubbed job object. A wiring error (e.g., reading `job.config.rateTypes` from the wrong field) would pass the arithmetic tests. An integration test of the `finalize` branch with a real job config would close this gap.

CRON_RATE_TYPES.length pinned implicitly via `17 * 9` magic literal

services/utility/tariffs/cfe/__tests__/transition.test.ts

`expect(allZones * CRON_RATE_TYPES.length).toBe(17 * 9)` — the `9` is a magic literal. Adding a tenth rate type to rate-pages.json produces a confusing arithmetic failure rather than a clear `expect(CRON_RATE_TYPES.length).toBe(9)`. Splitting into two assertions (explicit length + the product) would make failures self-diagnosing.

UNKNOWN_DIVISION test doesn't assert distinct rateType per failure

services/utility/tariffs/cfe/__tests__/collector.test.ts

The test asserts `unknown.toHaveLength(2)` and `unknown.every(f => f.division === 'ATLANTIS')`, confirming one failure per rate type. But it doesn't assert that the two failures carry distinct `rateType` values ('GDMTH' and 'GDMTO'). An implementation that emits two failures for the same rate type would pass. Adding `expect(unknown.map(f => f.rateType).sort()).toEqual(['GDMTH', 'GDMTO'])` (or checking `f.message` prefix) would close this.

Three behavioral changes each covered by a focused, well-named test

services/utility/tariffs/cfe/__tests__/collector.test.ts

All three stated behavioral changes (unmatched surfacing, UNKNOWN_DIVISION failures, honest denominator) have direct test coverage with specific assertions and clear descriptions. The accented 'VALLE DE MÉXICO' test case is particularly good at pinning the exact bug scenario.

Double iteration over DIVISIONS in resolveTargets — minor but asymmetric

services/utility/tariffs/cfe/src/lib/divisions.ts:65

`targets` is built with `DIVISIONS.filter(...)` and `unmatched` is built with `wanted.filter(w => !DIVISIONS.some(...))` — two passes duplicating the same predicate in opposite polarity. A single pass building a Set of matched wanted keys while collecting targets, then deriving unmatched from the difference, would be both cleaner and marginally more efficient. At 17 divisions this is trivial, but the current structure makes the logic harder to follow at a glance.

`matches` inner function closes over nothing — could be module-level

services/utility/tariffs/cfe/src/lib/divisions.ts:68

The `matches` predicate is defined inside `resolveTargets` on every invocation but captures no variables from the enclosing scope. Hoisting it to module level would make it directly testable, clarify that it's a pure predicate (not a closure), and avoid a per-call function allocation.

Zero-pairs fallback silently proceeds where a loud error would be clearer

services/utility/tariffs/cfe/src/handlers/tariff-transition.lambda.ts:249

`expectedPairs > 0 ? expectedPairs : (totals.divisionsDiscovered ?? null)` — if `expectedPairs` is 0 (unreachable if job creation validates), the handler silently falls through with a potentially-wrong denominator rather than throwing. A `throw new Error('finalize: expectedPairs resolved to 0 — job config has empty rateTypes or all divisions unrecognized')` would be both safer and easier to debug.