feat/victron

Window cap check uses `alignedFrom`, which can reject valid at-cap requests

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

windowDays is computed as (toUtc − alignedFrom) / 86_400_000. Since alignWindowStart always floors the start backward (by up to 14m59s for 15m, 59m59s for 1h), a request of exactly 31.0 days can become 31.01+ days after alignment and get rejected with 'exceeds VRM cap' even though the original window was within bounds. A correct check would compare against the original fromUtc span, or subtract the alignment delta from the cap. Most impactful for the 1h granularity where up to 59 minutes are added.

Future `last_timestamp` from VRM produces negative secsAgo and falsely reports device online

services/metrics/integrations/victron/src/handlers/connection.lambda.ts

secsAgo = nowEpochSec − last. If VRM returns a last_timestamp in the future (device clock skew), secsAgo is negative, making `secsAgo <= 900` true. The device would be reported reachable:true with a misleading 'last update -N seconds ago' detail. Given that VRM is a cloud intermediary that normalizes timestamps, this is unlikely but possible under device clock drift.

Partial channel success emits no signal to coordinator that some channels were skipped

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

When some channels project successfully (payloads.length > 0) and some fail (channel-not-found), the handler returns InvocationSuccess with only the successful payloads — logs the skipped channels but the coordinator has no structured way to know grid_import data is missing from the collection window. Consistent with the documented design ('Only fail if ALL fail'), but a consistently-absent channel produces no alert — only log noise. An observability gap that could mask a misconfigured MetricSource.

IAM wildcard allows worker to read any org's Victron secret

infra/cdk/src/stacks/services/metrics/integrations/victron/iam.ts

The policy grants GetSecretValue on `{env}/victron/*` and `batu/{env}/metrics/victron/*`. Both are per-env wildcards covering every organisation's Victron credential. A compromised worker Lambda could retrieve another org's VRM token by passing any matching ARN. In the normal SFN path this cannot be triggered by end-user input (secretConfigArn is DB-sourced), but defence-in-depth is absent at the IAM layer. Consider per-org path segments (e.g. `batu/{env}/metrics/victron/{orgId}/*`) or explicitly document the intentional cross-org trust boundary.

`secretConfigArn` is not validated against the expected prefix before use

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

inv.secretConfigArn is passed directly to loadCredentialFromArn → GetSecretValueCommand without a code-level check that it matches the expected victron/* prefix. IAM mitigates this (the Lambda role rejects reads outside those paths), but if the IAM policy were ever widened, a crafted Invocation could cause the worker to fetch an unrelated secret. Defence-in-depth runtime prefix check would be independent of IAM policy correctness. Same in connection.lambda.ts.

Silent `catch {}` in `loadCredentialFromArn` masks IAM/permission failures as `MissingSecret`

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

The catch block swallows all Secrets Manager errors — including AccessDeniedException (misconfigured/revoked IAM policy) and ResourceNotFoundException — surfacing them identically as MissingSecret. An IAM regression stripping GetSecretValue from the Lambda role would be indistinguishable from a genuinely absent secret, delaying diagnosis. Consider distinguishing AccessDeniedException (log a distinct warning) from ResourceNotFoundException (current null treatment is correct).

`fail.exception` log may echo fetch-stack error messages

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

The outer catch logs e.message from any unhandled exception. Currently this will be runtime errors, not VRM network errors (caught inside vrmGet). However, if a future refactor moves credential handling outside the try block or if Node.js fetch surfaces the request URL in an exception message, the token in x-authorization could theoretically appear in logs. The explicit comment 'NEVER log the payload' in loadCredentialFromArn is good hygiene and should extend to the catch-all handler.

`TranslationFailed` always maps to statusCode:500 — input validation errors classified as infrastructure errors

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

translationFailed() hard-codes statusCode:500, so validation failures like 'invalid window', 'window exceeds VRM cap', and 'missing timezone' are emitted with status:500 and become InvocationFailure.status='error' (not 'fail'). These are logical precondition failures, which the JSend contract classifies as 'fail' (4xx). The established ecosystem does this the same way (eGauge identical) — so this is a contract-level inherited semantic choice, not a Victron deviation. However, it means the coordinator's retry/alerting logic cannot distinguish a misconfigured invocation from a genuine infrastructure breakdown. This semantic mismatch should be addressed upstream before 11 more ports inherit it.

Manifest `externalVariableId: 'solarGeneration'` is a legacy vestige — never used in routing

packages/integration-manifests/src/manifests/victron.ts

The victron manifest declares solar_generation.externalVariableId as 'solarGeneration' (the legacy VRM channel name). Per metrics-pipeline.md gotcha #1 and resolveExternalVariableId(), what gets stored in MetricSource.source_config.externalVariableId is the manifest VARIABLE KEY ('solar_generation'), not this inner field. The worker routes on that key against CHANNEL_ATTRIBUTE_CODES. The inner externalVariableId field is structurally unused in routing. A future maintainer could misinterpret it as the routing key. A clarifying comment or normalization to the canonical key would prevent confusion.

Mixed-granularity invocation (15m + 1d channels) — two VRM fetches not tested

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

metrics.lambda.ts groups channels by granularity and calls fetchStats ONCE PER DISTINCT GRANULARITY (the for-loop over `granularities`). Every test uses only same-granularity channels, so the multi-fetch path is never exercised. A bug in how `recordsByGranularity` is populated or keyed for the second granularity would go undetected. Add a test with sources: [{…, granularity:'15m'}, {…, granularity:'1d'}] and assert fetchStats is called TWICE with the correct `interval` each time and both payloads returned.

`resolveChannels` scalar-params fallback path is untested

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

When `params.sources` is absent or empty, `resolveChannels` constructs a single-channel spec from scalar params (params.metricSourcePublicId, params.externalVariableId, params.granularity), falling back to DEFAULT_CHANNEL ('solar_generation') and DEFAULT_GRANULARITY ('15m'). No test exercises this branch. If the fallback defaults or the type cast to MetricChannelSpec are wrong, older or direct-invoke callers silently get the wrong channel. This path will be copied into 11 more ports.

Zero-length window (`fromUtc === toUtc`) not tested — boundary in guard

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

The handler guards `fromUtc >= toUtc`, which rejects both the equal case and reversed windows. No test directly covers `from === to`. A direct test with an equal window should confirm the TranslationFailed path. The equal case is most likely to appear from a misconfigured aggregator emitting a zero-width window.

`loadCredentialFromArn` returning null not tested in the connection handler

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

The metrics handler tests the null-credential path (MissingSecret). The connection handler has identical logic but the connection-handler test file has no corresponding case. A regression where the connection handler throws instead of returning a clean MissingSecret on null credential would go undetected.

`fetchOwnUserId` failure path not tested in connection handler

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

The connection handler calls fetchOwnUserId when credential.userId is absent and, on failure, returns a success-probe with probeFailurePayload. The failure path (fetchOwnUserId returns {ok:false}) is never tested. Test needed: loadCredentialFromArn returns {token:'tok'} (no userId) AND fetchOwnUserId returns {ok:false, reason:'auth-failed', …} → result should be success (probe) with credentialValid:false.

`probeFailurePayload` — only 2 of 7 VRM failure reasons exercised

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

probeFailurePayload has branching logic: reason==='auth-failed' produces a specific detail string; all others use the details field verbatim. The remaining VRM failure reasons (rate-limited, timeout, http-error, vendor-rejected, unparseable) are neither tested via the handler mock nor directly as a unit test of the exported pure function. Direct unit tests for each reason would pin the auth-failed branch and guard against regressions.

`kwhToMeanWatts` 1h test uses trivially-satisfied value

services/metrics/integrations/victron/src/__tests__/points-to-batu.test.ts:37

The 1h test passes `1 kWh` and expects `1000 W` — trivially satisfied by almost any formula that divides by slotSeconds (1×1000×3600/3600=1000). The 15m test uses `0.25 kWh → 1000 W` (non-trivial) and the 1d test adds `48.897 kWh → …`. The 1h case should get a non-trivial value (e.g. 0.5 kWh → 500 W) to differentiate a correct formula.

1d window cap (180 days) not tested — only the 31-day 15m cap is covered

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

MAX_WINDOW_DAYS defines different caps: 15m/1h → 31 days, 1d → 180 days. Only the 15m 31-day breach is tested. There is no test confirming (a) a 1d request within 180 days succeeds; (b) a 1d request > 180 days is rejected. The 180-day cap path is unexercised.

`vendor-rejected` fetch reason not tested via metrics handler end-to-end

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

mapFetchError unit tests cover vendor-rejected → UpstreamUnavailable, but no test mocks fetchStats returning {ok:false, reason:'vendor-rejected'} and asserts on the handler's overall status/errors. The unit test pins the error mapping but not the handler-level wiring for this reason.

`grid_export` channel not exercised end-to-end through the metrics handler

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

The handler tests use only solar_generation and grid_import. The pointsToBatu unit tests confirm grid_export shares Pg with solar_generation by reference but do not test a full projection for grid_export (codes: Pg+Bg). Through the handler, grid_export has never been requested — a typo in CHANNEL_ATTRIBUTE_CODES.grid_export would not be caught.

Envelope helpers (`success`/`failure`) duplicated 6 ways, heading for 28 with remaining 11 ports

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

success<T>() and failure() are byte-for-byte identical across growatt/metrics, growatt/explore, egauge/metrics, egauge/connection, victron/metrics, victron/connection (6 files). With 11 more ports, this reaches 28 copies. @batu/metrics-engine already exports InvocationSuccess, InvocationFailure, Invocation — the building blocks. Exporting invocationSuccess()/invocationFailure() helpers from services/metrics/engine/src/lib/ eliminates this before it replicates. Concrete divergence risk: TraceFields has a workerRequestId? field absent from all 6 current helpers — they can diverge independently.

`resolveChannels` duplicated across 3 metrics handlers with no shared home

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

resolveChannels(params, fallbackSourceId) is verbatim in growatt, egauge, and victron metrics handlers. This BAT-189 boilerplate will appear in every metrics worker. A shared resolveChannelsFallback() in @batu/metrics-engine/lib means one deletion point when the scalar-params legacy path is retired, instead of 14. The eGauge extension (egaugeColumn spread) can be handled via an options object.

`alignWindowStart` called twice with identical arguments in the channel projection loop

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

alignWindowStart(fromUtc, granularity, timezone ?? 'UTC') is computed at line 145 (stored as alignedFrom for the cap check and fetchStats call) and recomputed inside the for (const ch of channels) loop for every channel projection. All channels sharing a granularity call it with the same three inputs. Fix: populate an alignedFromByGranularity: Map alongside recordsByGranularity in the fetch loop and read it in the projection loop. alignWindowStart uses Intl.DateTimeFormat on the 1d path — non-trivial cost. This pattern will be copied into 11 more ports.

Sequential granularity fetches; 90s timeout sized exactly to worst case with no margin

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

The fetch loop awaits each granularity serially. For a mixed 15m+1d collection, two serial worst-case fetches consume 60s of the 90s budget. Promise.all(granularities.map(...fetchStats)) halves worst-case latency. If sequential is intentional (e.g. VRM per-org rate-limit), document it explicitly — otherwise each of the 11 remaining ports will independently debate and decide.

`resolveChannels` comment says 'older callers' but Victron has none in the TypeScript system

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

The JSDoc copies 'keeps direct invokes / older callers working' from eGauge/Growatt, where it's accurate (live legacy paths being migrated). Victron's prior callers are in the Python monorepo — there are no TypeScript 'older callers'. As the template for 11 ports, copying this comment as-is makes it impossible to audit which integrations genuinely have migration debt vs. which adopted the pattern speculatively.

Window cap validation mixed into fetch loop — partial VRM calls before validation failure

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

The per-granularity window cap check sits inside the fetch loop. For a 2-granularity request where 15m is valid and 1d is oversized, the 15m VRM call fires before the 1d cap error is returned. Extracting all cap checks into a validation pass before the try block (alongside jobType/asset/window/timezone checks) makes 'validate all before any I/O' the explicit contract. For 11 ports copying this pattern, structural separation reduces the risk of partial-network-call validation failures becoming entrenched.