feat/enphase
needs attention27da24f · incrementalPR #282reviewed 2026-07-10 00:23 UTC0H · 3M · 7L · 2I- Purpose
- Port Enphase Enlighten v4 into the v2 metrics engine with a ground-up auth redesign: OAuth refresh rotation moves off secret write-back into a DDB token store (secrets become read-only), eliminating grant contention with the live legacy pipeline.
- Goal
- Enphase integration live in preview — 15m native (production_meter, exact parity vs vendor lifetime and legacy DDB), grant-safe token rotation, CDK stacks, and multi-granularity validation (15m+1d disjoint tiers, both grains parity-exact).
- Sub-goals
- SG-1: Probe + worker + 53 unit tests (13 rotation state machine)
- SG-2: CDK AuthCache (no-TTL token store) + Lambda stacks; read-only IAM
- SG-3: Preview data plane + sentinel grant-protection + IAM quota unblock
- SG-4: Live SFN + Tinybird + exact parity (Δ=0 3/3 days) + dead-grant live-validation
- SG-N: Framework learnings #27–32 fold-back to scope.md + integrations CLAUDE.md
- SG-6: Rebase onto feat/int-base (multi-gran engine, §9 standard)
- SG-7: Live probe confirms NO native monthly; worker → §9 shape; 5m micro re-rejected
- SG-8: Live multi-gran validation — 15m+1d disjoint, parity exact both grains, read path proven
- What
- SG-8 closes the branch: adds provision-multigrain-sources.ts (a dev-only script for provisioning coarser-grain MetricSources and flipping source statuses for disjoint-coverage collection), updates scope.md to mark SG-8 complete, and appends intent_e8h1 evidence to intent.md.
- Why
- SG-8 is the live multi-granularity validation milestone — proves that the §9 worker collects 15m+1d from the same stream with disjoint tiers, dedupe is stable per grain, parity is exact on both grains vs vendor lifetime, and the real read path resolves the correct cadence per window. Closes all planned SGs; branch is merge-ready.
- Areas
- scripts/metrics+223−2.branch+50−1
- Blast
- This commit: 4 files, +274/−2. Whole branch: ~70 files across services/metrics/integrations/enphase/ (new), infra/cdk/src/stacks/services/metrics/integrations/enphase/ (new), domains/metrics, domains/cross-domain, packages/integration-manifests, scripts/metrics, .branch/. No changes to platform app, API handlers, or DB schema.
Findings · 13
correctness4
null externalId coerced to '' instead of failing loudly
scripts/metrics/provision-multigrain-sources.ts:167
When assetRow.externalId is null, the script passes '' to provisionAssetShell. The shell uses (orgId, makeId, externalId) as a natural-key fallback; an empty string is a degenerate key that could match multiple assets in the same org/make. A null externalId likely indicates missing seed data — throw rather than coerce.
TOCTOU: version read and shell call not in a transaction
scripts/metrics/provision-multigrain-sources.ts:192
metricSources.version is read in one query and passed to changeMetricSourceStatusShell in a separate call. A concurrent writer between the read and the shell will produce a spurious version-conflict error. Low-risk in practice on dev, but the resulting error message ('version conflict') will be opaque without context.
split(':') truncates on multi-colon values
scripts/metrics/provision-multigrain-sources.ts:65
Both parsers use destructuring after split(':'), discarding anything after the second segment. Public IDs (msr_…) are ULID-safe today, but the pattern is fragile. Use split(':', 2) or an indexOf-based split to make the intent explicit.
Op1 failure does not prevent Op2 — may mask a prerequisite dependency
scripts/metrics/provision-multigrain-sources.ts:178
When provisionAssetShell returns !result.ok, the script increments failures but continues to Op2 (status flips). If a status flip targets a source that was supposed to be created in Op1, the flip silently no-ops or errors on a missing source. The ops are documented as independent, but a comment clarifying the isolation intent would prevent confusion.
security3
No runtime prod guard — wrong POSTGRES_URL silently mutates any DB
scripts/metrics/provision-multigrain-sources.ts:65
The header warns 'Preview/dev only' but there is no enforcement. A mispointed env var (wrong terminal, CI secret leak, --env flag missing) writes to whichever DB POSTGRES_URL resolves to. A minimal guard — reject connection strings matching known prod hostnames, or require an explicit --env=dev flag — prevents the most likely accident.
DATABASE_URL fallback silently widens credential scope
scripts/metrics/provision-multigrain-sources.ts:65
POSTGRES_URL ?? DATABASE_URL silently accepts a broader, potentially higher-privilege connection string. In CI/CD environments where DATABASE_URL points to staging/prod, the script connects without warning. Prefer a single explicit env var and fail loudly if absent.
publicId accepted without msr_ format validation
scripts/metrics/provision-multigrain-sources.ts:47
The --set-status parser accepts any non-empty string as publicId and forwards it to a parameterized Drizzle query (no injection risk). However, no check against the msr_ ULID prefix means a typo or wrong entity id produces 'not found' rather than a clear error. Add a regex check at parse time.
conventions4
Redundant type assertion on already-narrowed status union
scripts/metrics/provision-multigrain-sources.ts:200
change.status is typed 'active' | 'inactive' by both the Args interface and parseArgs validation. The cast `as MetricSourceFCIS.MetricSourceStatus` is redundant and will mask a future divergence if MetricSourceStatus gains new variants.
Error message omits DATABASE_URL fallback
scripts/metrics/provision-multigrain-sources.ts:95
throw new Error('Missing POSTGRES_URL') is misleading when the script also accepts DATABASE_URL. Prefer 'Missing POSTGRES_URL (or DATABASE_URL)' to match the actual accepted env vars.
as unknown as Database cast is established script-layer convention
scripts/metrics/provision-multigrain-sources.ts:97
Identical pattern used in copy-legacy-secrets.ts, migrate-legacy-registry.ts, seed-metrics-test.ts, backfill-orphan-pdfs.ts. This is the established workaround for Drizzle's concrete generic type not satisfying the Database branded interface in script contexts. No violation.
No RLS wrapping — intentional and correctly scoped
scripts/metrics/provision-multigrain-sources.ts:97
Scripts in scripts/metrics/ operate outside the handler stack with a superuser/service-role credential bypassing RLS by design. The 'Preview/dev only' header, POSTGRES_URL requirement, and sibling-script precedent confirm this is the expected pattern.
tests1
parseArgs inlined rather than extracted to lib/ (diverges from sibling pattern)
scripts/metrics/provision-multigrain-sources.ts
Sibling scripts (copy-legacy-secrets.ts, migrate-legacy-registry.ts) extract pure-core logic into lib/ modules with unit tests. parseArgs here is inlined, making it untestable without running the script. Low-risk given dry-run-by-default and one-off use, but worth noting for consistency.
improvement1
Exit code swallowed if connection.end() throws in finally
scripts/metrics/provision-multigrain-sources.ts:215
If connection.end() throws inside finally, the error propagates to the rejection handler (exit 1), masking the actual failure count that main() would have returned. Minor edge case, but the exit code would be correct by accident rather than by design.