fix/tariff-cov

formState delete list must stay in sync with cascade steps or retries leak state

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

scrapeDivision deletes NAMES.state/municipality/division from the shared formState before each attempt, then re-sets them via post(). This is correct today — the deletes at the top of every call clear any stale values from a prior attempt. But the invariant is implicit: if a future maintainer adds a cascade step (e.g. a 'region' dropdown) and adds a post() call without also adding the corresponding delete at the top, stale values from a failed attempt pollute the next retry. The pattern is correct but fragile — the delete list and the post list are a paired contract with no enforcement.

lastError is undefined if DIVISION_SCRAPE_ATTEMPTS is ever reduced to 0

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

lastError is declared without initializer. If DIVISION_SCRAPE_ATTEMPTS were 0 the loop body never runs, scraped stays null, and the rethrow becomes `new Error('undefined')`. Currently impossible since it's a constant set to 3, but the post-loop check relies on an implicit loop-entry invariant the type system cannot enforce. Initialising as `let lastError: unknown = new Error('no attempts made')` makes the contract self-documenting.

formState accumulates year and month keys permanently — intentional

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

Year and month keys are set once before the division loop and never deleted. Every division postback includes them. This is correct — ASP.NET requires all form fields and the month must stay selected across the cascade. Confirmed intentional.

Total sleep budget is implicit — Lambda timeout could be silently consumed by retry backoff

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

sleep(RETRY_BACKOFF_MS * attempt) with RETRY_BACKOFF_MS=250 and DIVISION_SCRAPE_ATTEMPTS=3 adds at most 250+500=750ms per division. With 15+ divisions this is ~11s of additional sleep in the worst case, on top of real request latency. At current constants this is well within any reasonable Lambda timeout. Worth noting if either constant grows.

Error wrapping in lastError rethrow discards non-Error throwables

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

String(lastError) on a non-Error thrown value is safe here — the only source of thrown values is the HTML parser or HTTP client, neither of which carries credentials or sensitive data.

Multi-paragraph block comments are high-volume for a single Lambda handler

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

CLAUDE.md prohibits multi-paragraph docstrings. The file now has ~8 multi-paragraph /** */ blocks. Each documents a non-obvious constraint or prior failure mode — which is exactly what the rule's WHY exception is for. The content is load-bearing (bug archaeology). No change strictly required, but the density is notable.

sleep in Lambda handler follows established codebase precedent — no violation

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

10+ identical patterns in other Lambda handlers and API clients (fronius, solark, ABB). The CLAUDE.md intent is to prevent sleep in domain decision functions. This is an imperative I/O shell — the usage is correct.

Retry boundary (DIVISION_SCRAPE_ATTEMPTS=3) is untested — only 1-fail-then-success path covered

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

failTimes=1 proves the retry path works, but two boundaries are uncovered: (a) exactly 2 failures then success — the last-attempt edge — and (b) all 3 attempts exhausted — which should record the division as a failure. Without (b), reducing DIVISION_SCRAPE_ATTEMPTS from 3 to 1 would leave the transient test green while breaking the retry budget guarantee.

No test for first or last division failure — only a middle-of-list division (BAJIO, index 2)

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

The containment test uses BAJIO (3rd of 15 configs). The last-division case (Valle de México, a multi-zone config) is structurally different: permanent failure there should produce 3 zone entries in failures and divisionsCollected=14 — never tested. A first-division failure would also make the cascade-infection scenario impossible to demonstrate (nothing precedes it), but the counter handling is worth pinning.

Real sleep (up to 750ms) in tests — no fake timers, RETRY_BACKOFF_MS not injectable

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

RETRY_BACKOFF_MS is a module-private constant. The flaky tests incur real wall-clock sleeps. Tests stay within the timeout but are noticeably slower. Exporting RETRY_BACKOFF_MS or accepting it as injected config would let tests pass 0 and run instantly — also makes the retry constant auditable from the test.

FlakyStubClient attempt counter is instance-global — coupling is undocumented

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

private attempts counts only the target division's postbacks, so non-target divisions don't consume the budget. This is correct but implicit. A per-divisionId Map would make the counter scope self-documenting and robust if a test ever drives multiple rate types through the same client instance.

Test 1 does not assert attempt count — retry success is inferred, not pinned

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

The test proves BAJIO is collected and failures is empty — sufficient to establish that a retry occurred. It does not assert how many attempts were made (e.g. via client.urls length). Low severity because the containment test provides complementary structural proof.

FlakyStubClient __VIEWSTATE check correctly models the infection mechanism, not just luck

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

A broken page has no __VIEWSTATE. Any postback built from it is rejected immediately (another broken page). The fix (baseResp baseline) ensures all division attempts start from a page that has a viewstate. The stub enforces this exactly — the test proves structural containment, not coincidental success.

divisionsDiscovered is not asserted in the two new tests

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

Both tests assert divisionsCollected. Asserting divisionsDiscovered alongside (expected 17 in both) would pin the accounting separately and catch a regression where failures are silently suppressed into divisionsCollected. The existing 'counts zones, not division configs' test at line 261 checks both — establishing the pattern.

data.rates[0]! non-null assertion is safe for test inputs but would throw silently if rates were empty

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

Both new tests have at least one rate type configured and at least one successful division, so rates[0] always exists. Adding expect(data.rates).toHaveLength(1) before the chain would produce a clearer failure message if the invariant ever broke.

scrapeDivision closes over shared mutable formState without documenting the contract

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

The function signature shows only `cfg` in, but it also reads/writes the outer formState and captures baseResp, post, and NAMES. This hidden coupling is correct today (serial loop, deletes at top of each call) but is invisible at the call site. A one-line contract comment at the function boundary — naming that it resets formState to year+month baseline then replays state→municipality→division off baseResp — would make the invariant explicit without restructuring.

Asymmetric delete counts before state vs. municipality POST are undocumented

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

Before the state POST, three keys are deleted (state, municipality, division). Before the municipality POST, only division is deleted. This correctly mirrors ASP.NET cascade semantics (selecting a new state invalidates both downstream keys; selecting a new municipality only invalidates division). The asymmetry is silent — a brief inline comment explaining the cascade rule would prevent a symmetry 'fix' from introducing a bug.

Inline retry loop could be a small named helper

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

The 12-line attempt/lastError/sleep pattern is used once. A module-local `withRetry<T>(fn, attempts, backoffMs)` would collapse it to one call, eliminate the nested try/catch, and make the retry policy readable at the call site. Not urgent at this size; useful if a second caller is added (e.g. the year/month postbacks also fail under load).

scrapeDivision altitude: inner closure is correct — parameters vs. closure is a stylistic tradeoff

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

Making scrapeDivision a module-level function would require passing baseResp, formState, post, and NAMES as parameters — noisier than the closure at this size. The existing JSDoc at lines 470-473 already explains the isolation rationale. No change needed.