feat/int-base

AccessDeniedException re-throw path not covered by any of the 4 brands' loader tests

services/metrics/integrations/abb/src/__tests__/abb-client.test.ts

The fix correctly re-throws every error except ResourceNotFoundException. AccessDeniedException (IAM policy denial) re-throws to UpstreamUnavailable(502) — semantically correct but the intent is only documented by the ThrottlingException test. Adding AccessDenied would make the distinction explicit: 'only ResourceNotFoundException is caught; all other errors propagate'.

InvalidRequestException (malformed ARN) re-throws as UpstreamUnavailable — perpetual retry risk

services/metrics/integrations/abb/src/engine/abb-client.ts:199

A syntactically malformed ARN in secretConfigArn triggers InvalidRequestException, which now re-throws to UpstreamUnavailable(502) — a retryable code. The SFN will exhaust retries on what is actually a provisioning bug. This is not a regression (the old code returned null→MissingSecret(400) which was also wrong), and a malformed ARN is extremely unlikely given coordinator validation. But classifying it as UpstreamUnavailable silently burns retry budget on an unresolvable error.

Handler catch chain verified: re-thrown SDK faults → UpstreamUnavailable(502) in all 4

services/metrics/integrations/abb/src/handlers/metrics.lambda.ts:258

All four handlers wrap the credential-load + fetch block in a try/catch that maps any thrown Error to { _tag: 'UpstreamUnavailable', statusCode: 502 }. The fix is complete end-to-end. A ThrottlingException re-thrown from loadCredentialFromArn propagates to this catch and surfaces a 502 retryable error, not null→MissingSecret(400).

AWS SDK v3 error name 'ResourceNotFoundException' check is correct (no namespace prefix)

services/metrics/integrations/abb/src/engine/abb-client.ts:200

In AWS SDK v3, service exceptions have their `name` set to the bare class name without namespace prefix. The check `e.name === 'ResourceNotFoundException'` is correct across all four brands.

Victron userId numeric coercion correctly implemented and tested

services/metrics/integrations/victron/src/engine/vrm-client.ts:218

typeof userId === 'number' → String(userId) works correctly for numeric userIds (e.g. 123456 → '123456'). Test at vrm-client.test.ts:9 confirms.

Solark credentialDigest NUL separator is a JS escape (\0), not a raw binary byte

services/metrics/integrations/solark/src/engine/solark-client.ts:125

The source file is clean text. The \0 escape is correctly interpreted as U+0000 at runtime. The PR's concern about the old version embedding a raw NUL (making the file binary) has been addressed.

Re-thrown SDK error message may embed ARN/account-ID in SFN execution state

services/metrics/integrations/abb/src/engine/abb-client.ts:201

When SecretsManager throws AccessDeniedException, its message typically contains the full role ARN and account ID (e.g. 'User: arn:aws:sts::706877...'). The handler embeds e.message verbatim in UpstreamUnavailable.message, which flows into SFN execution history. Exposure is internal-to-AWS-account only (not customer-facing), but it leaks infra details to anyone with SFN console access. Same in sma, solark, victron. Mitigation: strip ARN/account patterns from e.message before embedding, or use a fixed generic string and log the raw error to CloudWatch only.

SSRF: none of the 4 clients follow vendor-supplied redirect URLs — clean

services/metrics/integrations/victron/src/engine/vrm-client.ts

All four clients use hardcoded HOST/BASE_URL constants for all outbound requests. No response Location header is extracted and used as a next request URL. Confirmed clean as stated in the SG-14 review.

Secret fields (token, subscriptionKey, etc.) are never logged in any of the 4 loaders

services/metrics/integrations/abb/src/engine/abb-client.ts:191

All four loadCredentialFromArn implementations honor the 'NEVER log the payload' contract. The re-thrown Error is a raw AWS SDK exception — it contains the secret ARN in metadata but NOT the SecretString value. No credential field leaks through any error path.

ABB JSDoc says 'ResourceNotFound' but the error name checked is 'ResourceNotFoundException'

services/metrics/integrations/abb/src/engine/abb-client.ts:183

The JSDoc uses the abbreviated 'ResourceNotFound' term; the code at line 200 checks e.name === 'ResourceNotFoundException'. Minor inaccuracy — code is correct, comment is slightly imprecise.

Victron JSDoc omits that absent userId is NOT a null condition (it's an optional field)

services/metrics/integrations/victron/src/engine/vrm-client.ts:186

The JSDoc correctly lists the null conditions (ResourceNotFound, empty, non-JSON, missing token) but a reader could wonder if absent userId also returns null — it does not. Adding '(userId is optional)' to the return-type description would clarify.

All 4 implementations structurally identical to fronius reference — no deviations

services/metrics/integrations/abb/src/engine/abb-client.ts

ABB, SMA, Victron, and Sol-Ark loadCredentialFromArn are structurally identical to the fronius reference. Error classification contract correct in all four. Handler outer try-catch correctly maps re-thrown SDK errors to _tag: 'UpstreamUnavailable' in each case.

Discriminated union propagation correct in all 4 handlers

services/metrics/integrations/victron/src/handlers/metrics.lambda.ts:138

In all four handlers (ABB:155, SMA:132, Victron:138, Sol-Ark:168), loadCredentialFromArn is inside the outer try. A re-thrown transient SDK error propagates to the catch(e) block which maps it to _tag:'UpstreamUnavailable'/502. Never a raw unhandled throw to the SFN.

No handler-level test: loadCredentialFromArn throws → UpstreamUnavailable(502)

services/metrics/integrations/abb/src/__tests__/metrics-handler.test.ts

The SG-14 fix's entire purpose is: when loadCredentialFromArn re-throws (ThrottlingException, AccessDenied, network), the handler's outer catch must surface UpstreamUnavailable(502) instead of an unhandled Lambda failure. The loader-level unit tests verify the re-throw, but none of the four brands' handler tests mock loadCredentialFromArn to throw and then assert the result is UpstreamUnavailable. The null→MissingSecret path IS tested in handler tests, but the new throw→UpstreamUnavailable path is not. Closing this gap is the highest-value test to add before merge.

ABB: empty subscription_key and empty app_id not individually tested in null-returns case

services/metrics/integrations/abb/src/__tests__/abb-client.test.ts:165

The test covers missing app_id and empty token, but neither empty subscription_key nor empty app_id is explicitly asserted → null. The implementation guards all three fields with .length===0 checks; both branches are live untested code paths.

SMA: empty client_secret not tested; only empty client_id is asserted

services/metrics/integrations/sma/src/__tests__/sma-client.test.ts:158

The null-returns test checks missing client_secret (only client_id present) and empty client_id, but never asserts that client_secret='' returns null. The implementation guards both fields symmetrically (clientSecret.length===0 → null).

Solark: AccessDenied now re-throws but no test documents this intentional change

services/metrics/integrations/solark/src/__tests__/solark-client.test.ts:237

The old solark test explicitly asserted AccessDenied→null (encoding the bug). The fix is correct (only ResourceNotFoundException→null, everything else re-throws), but no test names AccessDenied to document the intent. The ThrottlingException test establishes the pattern; adding a named AccessDenied case would close the regression guard on the old bug's specific error type.

Victron: string userId passthrough (typeof string branch) not tested

services/metrics/integrations/victron/src/__tests__/vrm-client.test.ts:9

The numeric-userId-coercion test is present. The implementation also passes through string userId unchanged (typeof userId === 'string' branch) — this branch has no test.

rejects.toThrow assertion doesn't prevent a test passing trivially if mock is accidentally swapped

services/metrics/integrations/abb/src/__tests__/abb-client.test.ts:180

All four brands use `await expect(loadCredentialFromArn(...)).rejects.toThrow('Rate exceeded')`. The mock is correctly set to mockRejectedValue; the assertion is sound. But `.rejects.toThrow(throttled)` (passing the Error instance rather than the string) would be more precise — it verifies the exact error is propagated, not just any error with a matching message substring.

credentialDigest NUL separator: pin test confirms hash but doesn't assert the separator identity in code

services/metrics/integrations/solark/src/__tests__/solark-client.test.ts:246

The pinned-hash test is load-bearing (a separator change would break it), but the separator's identity is documented only in the comment. An explicit `expect(createHash('sha256').update('\0').digest('hex')).toBe(...)` check would make the NUL nature machine-verifiable.

No handler-level test for token-only Victron credential (fetchOwnUserId fallback path)

services/metrics/integrations/victron/src/__tests__/metrics-handler.test.ts

The loader test correctly verifies that a token-only payload (no userId) returns {token} without the userId field. But the handler test only sets up a mock with {token, userId} — the fallback path (credential.userId absent → call /users/me) has no handler-level coverage.

loadCredentialFromArn SM-fetch block duplicated across 11 integration clients

services/metrics/integrations/abb/src/engine/abb-client.ts:191

The two-try split pattern (SM send → ResourceNotFoundException filter → JSON.parse) is identical across 11 vendor clients (abb, apsystems, egauge, enphase, fronius, hoymiles, shelly, sma, solark, solis, victron). Only the field-extraction block differs per vendor. A `loadSecretJson(client, secretId): Promise<Record<string,unknown>|null>` helper in @batu/metrics-engine would collapse this to one place. The SG-14 fix had to land in 4 files this round; a future SM SDK change needs 11 edits.

Solark credentialDigest NUL separator (\0) is undocumented — could be broken by well-meaning cleanup

services/metrics/integrations/solark/src/engine/solark-client.ts:125

The \0 prevents boundary-shift collisions (user=ab,pw=c vs user=a,pw=bc hashing identically). Without a comment, a maintainer could replace it with ':' or remove it, silently breaking memo isolation. One line: '// \0 separator prevents boundary-shift collisions' closes this.

cachedSecretsClient singleton also duplicated across 11 clients

services/metrics/integrations/sma/src/engine/sma-client.ts:313

The `let cachedSecretsClient: SecretsManagerClient | null = null; export function secretsClient() { … }` block is copy-pasted alongside loadCredentialFromArn in every client. A shared export from @batu/metrics-engine would eliminate this without affecting the per-Lambda warm-reuse property (module scope is per-bundle).

`e instanceof Error` check before `.name === 'ResourceNotFoundException'` is always true in AWS SDK v3

services/metrics/integrations/victron/src/engine/vrm-client.ts:205

AWS SDK v3 exceptions always extend Error, so the instanceof guard is redundant noise. Affects all 5 implementations (abb, sma, solark, victron, fronius). No behavior change — purely cosmetic, worth noting if a shared helper is ever extracted.

Split try/catch verbosity — a narrow isResourceNotFound() type guard would be cleaner

services/metrics/integrations/abb/src/engine/abb-client.ts:196

An alternative: a narrow `isResourceNotFound(e: unknown): boolean` guard lets the SM call live in a single try with a typed discriminant: `catch(e) { if (!isResourceNotFound(e)) throw e; return null; }`. Non-blocking — the current form is correct and readable. Worth noting if a shared helper is extracted.