← all branches

feat/metrics-agg

needs attention
6faff4b · incrementalPR #291reviewed 2026-07-10 00:01 UTC0H · 2M · 6L · 1I
The branch
Purpose
Make a site-level total (e.g. total_demand = feeder_a + feeder_b + feeder_c) a first-class readable stream, computed at read and never materialized — the cross-device-totals requirement from the electricity-data-api port.
Goal
Read-time derivation engine: a derived stream whose active source is internal:derivation resolves input streams recursively and evaluates the formula slot-wise. Derived-at-read means late corrections self-heal; a materialized total would go stale (billing-grade bug).
Sub-goals
  • SG-1 — pure evaluator: restricted arithmetic grammar (+ - * /, parens, literals, identifiers) → AST → slot-wise eval. Never eval, zero deps. strict/partial policy.
  • SG-2 — read-path integration: route derivation source → recursive input resolution → evaluate. Depth + cycle guards at read. StreamWindowReader port for unit testability.
  • SG-3 — seam guard: derived total flows through SiteMetrics billing seam with zero seam change; numeric test pins coincident peak and honest data-quality.
  • SG-N — docs: derivation read-engine documented in domains/metrics/CLAUDE.md; site-metrics-seam.md updated.
The changes (whole branch)
What
Memoization layer over the derivation coordinator (diamond DAG dedup), stale-input skip (unreferenced formula variables no longer resolved), improved derivation-error messages at the SiteMetrics seam, and two new tests covering these behaviors.
Why
Previous review (d75e7386) identified that a diamond DAG (SHARED reached via two paths) would read the physical stream twice, and that a stale 'c: <deleted-stream>' entry in inputs would cause a hard failure even if the formula never used 'c'. Both are correctness issues for real-world multi-feeder site configurations.
Areas
domains/metrics+1994−75domains/cross-domain+14−3apps/platform+19−22.claude/rules+23−0packages/integration-manifests+3−2
Blast
16 files, ~2050 net additions across domains/metrics (new derivation engine + tests), cross-domain shell (error msg), platform handler. Public API signatures unchanged.
new-subsystem billing-path mutable-shared-memo
✅typecheck/lint· tsc + lint green across metrics, cross-domain, platform (per PR description)✅unit-tests· 50+ tests green; 2 new tests added in this diff➖ci· CI status API inaccessible with current token➖coderabbit· no .coderabbit.yaml in repo

Findings · 9

correctness1

low

Depth-guard test comment says d(MAX+1) trips but guard fires at d(MAX)

domains/metrics/src/metric/__tests__/metric.derivation.queries.test.ts

With MAX_DERIVATION_DEPTH=10, guard is ctx.depth >= 10, so it fires at depth=10 (d(MAX)), not d11. Production guard correct; comment misleads.

conventions3

medium

Pre-existing: TouScheduleFCIS.findByTariffAndZone throws instead of returning Result

domains/cross-domain/src/site-energy-metrics.shells.ts

Shell try/catches correctly, but underlying query violates no-throw contract. Not introduced in this PR — follow-up needed in tou-schedule.queries.ts.

low

MAX_DERIVATION_DEPTH imported cross-entity from metric-source into metric.queries

domains/metrics/src/metric/metric.queries.ts

Constant governs derivation semantics; natural home is derivation/derivation.errors.ts, not metric-source/metric-source.errors.ts.

info

readonly memo: Map vs readonly ancestors: ReadonlySet intentional asymmetry

domains/metrics/src/metric/metric.queries.ts

Documented in JSDoc. ancestors is per-branch immutable; memo is shared-mutable. No action needed.

tests3

low

Stale-input comment overstates: reads[] tracks pipe calls, not resolveStream calls

domains/metrics/src/metric/__tests__/metric.derivation.queries.test.ts

Comment says GHOST was 'never resolved'; reads[] only proves it was never physically read via the pipe. Behavior correct.

low

No test for memo suppressing false DepthExceeded on a shared sub-stream

domains/metrics/src/metric/__tests__/metric.derivation.queries.test.ts

Code comment explicitly states this property; no dedicated test. A graph TOTAL→{A→S, B→C→D→…→S} near MAX_DERIVATION_DEPTH would cover it.

low

No test combining stale entry + referenced-but-unbound variable

domains/metrics/src/metric/__tests__/metric.derivation.queries.test.ts

formula='m1+m2', inputs={m1:A, c:GHOST} (m2 unbound, c stale) — confirms skip doesn't mask DerivationInputNotFound.

improvement2

medium

parseFormula called twice per derivation node (variable extraction + evaluation)

domains/metrics/src/metric/metric.queries.ts

resolveDerivationWindow calls parseFormula to get referenced vars; evaluateDerivation calls it again internally. Fix: accept optional pre-parsed ParsedFormula in EvaluateDerivationInput.

low

reads.toHaveLength(2) fragile in stale-input test — prefer source-ID assertion

domains/metrics/src/metric/__tests__/metric.derivation.queries.test.ts

Diamond test does this correctly (filters by 'SHARED_src'). Prefer: reads.some(r => r.sourceIds.includes('GHOST_DELETED_STREAM_src')) === false.

History · 5 commits

  1. 6faff4bneeds attentionincremental0H · 2M · 6L2026-07-10 00:01current
  2. d75e738needs attentionincremental0H · 2M · 5L2026-07-09 19:39
  3. 17b6060needs attentionfull2H · 10M · 8L2026-07-09 18:48
  4. 6580b91needs attentionincremental2H · 9M · 11L2026-07-09 02:06
  5. 4849d9aneeds attentionfull6H · 8M · 6L2026-07-07 18:54