Convention alignment refactor across the monorepo — no behavior change.
Helpers adoption:
- adopt isEmpty() helper for emptiness checks (9 sites); OCPPServiceUtils.ts:1113 preserved with anchor comment (already-trimmed input; isEmpty would redundantly re-trim)
- adopt Constants.UNIT_DIVIDER_KILO for kW→W conversion (4 sites)
- use JSONStringify for unknown MCP tool payload (Map/Set-safe replacer)
Constants promotion:
- promote inline literals to named constants (MAX_ITEMS_PER_REPORT_MESSAGE with OCPP 2.0.1 §2.1.16/§2.1.18 spec-anchor comment; AVG_ENTRY_SIZE_BYTES; DEFAULT_EXPONENTIAL_BACKOFF_BASE_DELAY_MS; DEFAULT_AUTH_CACHE_TTL_SECONDS; DEFAULT_PERSIST_STATE)
- coverage gap: isNotEmptyArray adoption at ConfigurationValidation.ts:124
Imports discipline:
- add .js extension to relative import in ui-web/vitest.config (ESM/NodeNext)
- use component barrels instead of deep imports in tests (10 sites)
Test literals:
- use DEFAULT_HOST/DEFAULT_PORT constants in ui-common tests (72 sites: 32 in config.test.ts + 40 in WebSocketClient.test.ts)
StationHelpers.ts is now a pure re-export barrel preserving the public API,
so all 77 consumer test files keep importing from './StationHelpers.js'
without change. Mechanical move only: no logic, signature, or behavior
change.
determineEvseUsage is now exported (previously module-private) because it
is consumed by the factory across module boundaries.
Two optional EvProfile refinements. Both default to current behavior so
existing profiles and golden tests are unchanged.
- powerFactor (0, 1], absent => 1: cos phi factor. Per-phase current
becomes I = P / (V . phases . powerFactor); INV-1 extends to
P_active = V.I.phases.powerFactor.
- rampShape 'linear'|'sigmoid', absent => 'linear': sigmoid uses a
shifted-scaled logistic (k=10) pinned at f(0)=0 and f(1)=1 exactly
after endpoint normalization.
* refactor(charging-station): extract charging-profile helpers to HelpersChargingProfile (#1936 f-2)
Split Helpers.ts (Phase 2 of 5). Moves 14 charging-profile symbols into
sibling HelpersChargingProfile.ts and re-exports the public ones from
Helpers.ts so the barrel import path is preserved. Zero behavior change:
same signatures, same code, same log content. The moduleName local in
the new file switches log prefixes from 'Helpers.<fn>' to
'HelpersChargingProfile.<fn>', reflecting the actual source location.
Moved (private inside new file):
- getChargingStationChargingProfiles, buildChargingProfilesLimit,
ChargingProfilesLimit interface, getChargingProfileId,
getChargingProfilesLimit, canProceedRecurringChargingProfile,
prepareRecurringChargingProfile, checkRecurringChargingProfileDuration
Moved (exported from new file):
- getChargingStationChargingProfilesLimit, getConnectorChargingProfiles,
getConnectorChargingProfilesLimit, prepareChargingProfileKind,
canProceedChargingProfile (all re-exported from Helpers.ts)
- getSingleChargingSchedule (exported for Phase 3 direct import; not
re-exported from Helpers.ts to keep the public API unchanged)
Follows the barrel pattern established by HelpersReservation.ts in
Phase 1 (PR #1946).
* refactor(charging-station): extract connector-status helpers to HelpersConnectorStatus (#1936 f-3)
Split Helpers.ts (Phase 3 of 5). Moves 8 connector-status symbols into
sibling HelpersConnectorStatus.ts and re-exports the public ones from
Helpers.ts so the barrel import path is preserved. Zero behavior change:
same signatures, same code, same log content. The moduleName local in
the new file switches log prefixes from 'Helpers.<fn>' to
'HelpersConnectorStatus.<fn>', reflecting the actual source location.
Moved (private inside new file):
- initializeConnectorStatus
Moved (exported from new file, re-exported from Helpers.ts):
- buildConnectorsMap, initializeConnectorsMapStatus, resetConnectorStatus,
resetAuthorizeConnectorStatus, prepareConnectorStatus,
checkStationInfoConnectorStatus, getBootConnectorStatus
Follows the barrel pattern established by HelpersReservation.ts in
Phase 1 (PR #1946).
* refactor(charging-station): extract serial-number/id helpers to HelpersId (#1936 f-4)
The coherent path's `resolveTemplates` at `CoherentMeterValueBuilder.ts`
read connector-level `MeterValues` only. A station that defined
`MeterValues` at EVSE level (via the `Evses` template section) and
enabled `coherentMeterValues=true` would emit an empty MeterValue
because the EVSE-level array was never consulted — a pre-existing TODO
documented in the prior `resolveTemplates` JSDoc: "Adding EVSE-level
template inheritance to the coherent path requires extending the
context interface with `getEvseStatus`."
Fix: mirror the random/fixed path's `getSampledValueTemplate` semantics
(README section "MeterValues at EVSE level": "EVSE-level definitions
apply to all connectors of the EVSE and override connector-level
definitions"). EVSE-level `MeterValues`, when defined and non-empty,
override connector-level `MeterValues` for every connector under the
EVSE; connector-level `MeterValues` are used when the connector is not
grouped under an EVSE (flat `Connectors` map station layout) or when
the EVSE-level array is empty.
- Extend `ICoherentContext` with two accessors mirroring the existing
`ChargingStation` public API: `getEvseIdByConnectorId(connectorId)`
and `getEvseStatus(evseId)`. Requires importing `EvseStatus` from
the shared types barrel.
- Rewrite `resolveTemplates` in `CoherentMeterValueBuilder.ts` to
perform the EVSE-first lookup and fall back to connector-level when
the EVSE is undefined or its `MeterValues` array is missing/empty.
JSDoc updated to reflect the new contract.
- Extend the in-file `buildContext` test factory with an optional
`evseMeterValues` override (implies `getEvseIdByConnectorId`
resolves to EVSE id 1 and `getEvseStatus(1)` returns
`{ MeterValues: overrides.evseMeterValues, ... }`; falsy override
leaves `getEvseIdByConnectorId` returning `undefined`).
- Add an `EVSE-level template fallback` describe block with two
tests: (a) EVSE-level MeterValues override connector-level when
defined, (b) connector-level MeterValues used when no EVSE
grouping exists.
- Update README section "Template resolution scope" to remove the
"connector-level definition only" caveat and document the new
precedence rules.
The shared mock in `tests/charging-station/helpers/StationHelpers.ts`
already exposes both accessors; downstream test files routing through
the mock require no change.
R1 Lane A (Oracle) surfaced two follow-ups on the initial R1 rebase
implementation of the EVSE-first template lookup:
1. The `resolveTemplates` JSDoc and README wording claimed "the same
precedence as ... `getSampledValueTemplate`". That claim was
inaccurate on a subtle edge case: the random/fixed reference at
`OCPPServiceUtils.ts:1546-1563` aggregates `MeterValues` across all
sibling connectors under the same EVSE when `EvseStatus.MeterValues`
is empty. The coherent path deliberately does NOT do this - a
connector without its own `MeterValues` returns `undefined` under
the coherent path, whereas the reference would leak sibling
templates. This stricter isolation is desirable design (per-connector
ownership), but the parity claim overstated the match.
Wording weakened accordingly: JSDoc + README now explicitly state
that the coherent generator emits templates from exactly one source
(EVSE-level or the queried connector) and does not aggregate across
sibling connectors, calling out the divergence from the reference
path.
2. The initial R1 rebase covered "EVSE-level overrides connector-level
when defined" and "connector-level used when no EVSE grouping" but
left the third semantic branch untested: EVSE grouping present +
`EvseStatus.MeterValues` is an empty array. The `resolveTemplates`
guard `evseTemplates != null && evseTemplates.length > 0` explicitly
handles this case by falling back to connector-level, but no test
asserted the branch.
New test added: `should fall back to connector-level MeterValues
when EVSE grouping exists but EVSE-level MeterValues array is
empty`. Configures `evseMeterValues: []` in `buildContext` (which
makes `getEvseIdByConnectorId` return `1` and
`getEvseStatus(1).MeterValues = []`), asserts connector-level
template emits.
No code change to `resolveTemplates`; the empty-EVSE branch was
already correct.
R2 Lane B (adversarial) noted that the previous 3-test coverage in the
`EVSE-level template fallback` describe block conflated two distinct
cases: "no EVSE grouping" and "EVSE grouping present with
`MeterValues` undefined". Both eventually fall back to connector-level,
but through different code paths in `resolveTemplates`:
- "No EVSE grouping": `getEvseIdByConnectorId` returns `undefined` -
the outer `if (evseId != null)` guard fails, fallback immediate.
- "EVSE grouping + MeterValues undefined": `getEvseIdByConnectorId`
returns `1`, `getEvseStatus(1).MeterValues` is `undefined`, so the
inner `evseTemplates != null && evseTemplates.length > 0` guard
fails (both undefined and empty-array branches share the fallback).
The prior mock factory heuristic `overrides.evseMeterValues != null ?
1 : undefined` could express the first two cases plus "grouping +
empty array" but NOT "grouping + undefined MeterValues" - those
required distinct mock states.
Redesigned factory:
- Add an explicit `groupUnderEvse?: boolean` knob that overrides the
`evseMeterValues != null` heuristic when set.
- `groupUnderEvse === true` forces `getEvseIdByConnectorId` to return
`1` regardless of `evseMeterValues`, enabling the "grouping +
undefined MeterValues" state via `groupUnderEvse: true` alone.
- `groupUnderEvse === false` forces flat-connector layout even with
`evseMeterValues` defined (fully explicit override).
- `groupUnderEvse === undefined` preserves the prior heuristic
(backward-compat for existing tests).
New test added: `should fall back to connector-level MeterValues when
EVSE grouping exists but EVSE-level MeterValues is undefined`. Uses
`groupUnderEvse: true` without `evseMeterValues` to exercise the
distinct-from-empty-array branch.
R8 (comprehensive final review, user-directed expanded criteria) surfaced
2 tightly related follow-ups on the R1-established `resolveTemplates`
implementation:
1. Lane C MINOR (JSDoc precision drift): the divergence NOTE narrowed
the condition under which the random/fixed path aggregates
sibling-connector templates to "both EVSE-level and the queried
connector's `MeterValues` are empty". The reference
`getSampledValueTemplate` (`OCPPServiceUtils.ts:1546-1559`) actually
aggregates whenever `isNotEmptyArray(evseStatus.MeterValues)` is
false, which covers `undefined` and `[]` regardless of the queried
connector's state. The NOTE was strictly narrower than reality; the
README and PR body already used the broader wording, leaving only
the JSDoc drifted.
NOTE rewritten: "when EVSE-level `MeterValues` is undefined or
empty" (matches reference guard exactly), with the source clause
correspondingly rephrased "EVSE-level when non-empty, otherwise the
queried connector".
2. Lane A NIT (harmonization with cross-linked sibling): `resolveTemplates`
used inline `evseTemplates != null && evseTemplates.length > 0`; the
explicitly cross-linked sibling `getSampledValueTemplate` uses
`isNotEmptyArray(evseStatus.MeterValues)` for the identical guard.
`isNotEmptyArray` is a repo-wide type guard (`Utils.ts:411`,
`value is NonEmptyArray<T> | ReadonlyNonEmptyArray<T>`) that also
narrows the return type. Behavioral equivalence, better locality
with sibling code, one less inline predicate.
Guard rewritten to `if (isNotEmptyArray(evseTemplates))`; the utils
barrel import now includes `isNotEmptyArray`.
No behavioral change. Test invariant `fail: 0, skipped: 6` preserved.
R9 (post-R8-fix comprehensive re-review, user-directed expanded criteria)
surfaced 3 follow-ups on the R8-committed branch state:
1. Lane B MINOR (regression test gap): the documented divergence from
`getSampledValueTemplate` (coherent path does NOT aggregate
sibling-connector `MeterValues` when EVSE-level is undefined or
empty) was described in JSDoc, README, and PR body but not
contractually locked by a test. The prior test harness had a
single-connector EVSE (`connectors: Map([[1, connectorStatus]])`),
which precluded exercising the sibling-aggregation branch.
`buildContext` extended with `siblingConnectorMeterValues?:
SampledValueTemplate[]` knob: when defined, the mock's
`getEvseStatus(1).connectors` gains a second connector (id 2) with
the given `MeterValues`. New test
`should NOT aggregate sibling-connector MeterValues when EVSE-level
MeterValues is undefined or empty` asserts the coherent path emits
zero samples when queried connector's `MeterValues` is `[]`,
EVSE-level `MeterValues` is `[]`, and a sibling connector carries a
Power template. The reference path would emit that Power template;
the coherent path must not.
2. Lane B NIT (stale test comment): the comment at
`CoherentMeterValues.test.ts:1500-1503` still named the pre-R8
inline guard `evseTemplates != null && length > 0`. Updated to
reference `isNotEmptyArray(evseTemplates)` matching the current
implementation at `CoherentMeterValueBuilder.ts:335-337`.
3. Lane B SUB-THRESHOLD (README hyphenation): `README.md:367,369,409`
used `EVSE level` / `connector level` unhyphenated, drifting from
the hyphenated `EVSE-level` / `connector-level` used at
`README.md:489` and throughout the JSDoc/PR body/tests. Normalized
the 3 remaining occurrences to the hyphenated form for repo-wide
terminology consistency.
Local report `.omo/reviews/PR-1944.md` also refreshed post-R8 append
drift (Convergence Summary claimed "7 rounds" when 8 were now
documented; R8 consolidation arithmetic `12+ + 6 + 9 = 27+` was
written as `21+`).
Test invariant `fail: 0, skipped: 6` preserved (37/37 pass in target
file including the new sibling-aggregation regression test).
feat(meter-values): honor RegisterValuesWithoutPhases in coherent path (issue #1936 k) (#1942)
* feat(meter-values): honor RegisterValuesWithoutPhases in coherent path (issue #1936 k)
OCPP 2.0.1 `SampledDataCtrlr.RegisterValuesWithoutPhases` suppresses
per-phase L-N emission of `Energy.Active.Import.Register` when set to
`true`. The coherent path previously emitted per-phase register based
solely on the connector template's phase qualifier, ignoring this
config variable (documented at `CoherentMeterValueBuilder.ts` L-N
branch as "not consulted").
Wire the variable through:
- Add `OCPP20OptionalVariableName.RegisterValuesWithoutPhases` to the
OCPP 2.0.1 optional-variable enum (registry entry already existed).
- Extend `buildCoherentMeterValue` with an optional
`registerValuesWithoutPhases` parameter (defaults to `false` when
omitted, preserving current behavior for OCPP 1.6 callers).
- Extend `resolvePhasedValue` with the same flag; the
`ENERGY_ACTIVE_IMPORT_REGISTER` L-N branch returns `undefined` when
the flag is `true` so the caller log-and-skips the per-phase
template, leaving only the aggregate register to emit.
- Wire the flag resolution from `OCPPServiceUtils.buildMeterValue`'s
coherent strategy gate using the existing `isOCPP20FlagEnabled`
helper. Safe on OCPP 1.6 stations because the component-scoped
configuration key never resolves there (returns `false`, preserving
current behavior).
- Move `OCPP20OptionalVariableName` from the type-only import block
to the value import block in `OCPPServiceUtils.ts` (previously
imported as type only; now used as a runtime value).
Three tests added exercising the new behavior:
- L-N per-phase `Energy.Active.Import.Register` templates suppressed
and only the aggregate register emitted when the flag is `true`.
- Default per-phase L-N emission preserved when the flag is unset.
- `Power.Active.Import` per-phase emission unaffected (flag scoped to
`Energy.Active.Import.Register`).
README section on coherent-mode phase emission updated to reflect the
new semantics.
Test invariant `fail: 0, skipped: 6` preserved.
Spec citation: OCPP 2.0.1 edition 3 part 2, `SampledDataCtrlr`
component: "If this variable reports a value of true, then meter
values of measurand `Energy.Active.Import.Register` will only report
the total energy over all phases without reporting the individual
phase values."
The wire-site comment in `OCPPServiceUtils.ts` at the coherent strategy
gate previously stated:
> "OCPP 1.6 stations never resolve the component-scoped key (returns
> false), preserving current behavior."
`isOCPP20FlagEnabled` reads the raw configuration store via
`getConfigurationKey`. An OCPP 1.6 station whose template literally
set a key named `SampledDataCtrlr.RegisterValuesWithoutPhases` to
`"true"` WOULD resolve it. In practice this is impossible under normal
use (the key is not standard for 1.6), so the safety outcome is
correct, but the "never resolve" wording is not strictly accurate.
Weakened to: "OCPP 1.6 stations do not carry the component-scoped key
by default: `getConfigurationKey` returns `undefined`,
`convertToBoolean(undefined) === false`, so current behavior is
preserved."
Same intent (documenting OCPP 1.6 safety), more precise wording
matching the actual semantic of `isOCPP20FlagEnabled`.
No code change. Test invariant `fail: 0, skipped: 6` preserved.
* refactor(meter-values): apply RegisterValuesWithoutPhases at bucket level (issue #1936 k)
R2 Lane B found two spec-conformance issues in the initial threaded
implementation of `SampledDataCtrlr.RegisterValuesWithoutPhases`:
1. When only per-phase L-N `Energy.Active.Import.Register` templates are
configured on a connector and the flag is `true`, the previous
implementation suppressed all three L-N templates and emitted
nothing for the measurand. This violates the spec:
> "will only report the total energy over all phases without
> reporting the individual phase values."
The spec mandates that the total IS reported; it does not permit a
silent no-op when the config lacks an aggregate template.
2. `resolvePhasedValue` returned `undefined` for suppressed L-N
templates, and `buildCoherentMeterValue`'s log-and-skip path treated
this as an unsupported `(measurand, phase)` combination, emitting a
spurious `WARN` for every configured L-N register template on
OCPP 2.0.1 stations with the flag set. This is a legitimate,
config-driven skip, not an error condition.
Redesign the suppression as a bucket-level pre-filter in
`buildCoherentMeterValue` (before the emit loop) instead of a
per-call return-value negotiation with `resolvePhasedValue`. Both
issues collapse to zero:
- The new helper `applyRegisterValuesWithoutPhases` filters L-N
templates out of the `Energy.Active.Import.Register` bucket before
iteration.
- When the connector configured only per-phase L-N templates (no
aggregate), the helper synthesizes an aggregate template from the
first suppressed L-N by shallow-cloning it with `phase` cleared;
`unit`, `measurand`, `location`, and `context` fields inherit from
the L-N template so the emitted aggregate matches the operator's
intent for encoding and scale.
- Because L-N templates are removed before iteration, the
log-and-skip path fires only for genuinely unsupported combinations,
never for a configured spec suppression.
`resolvePhasedValue` reverts to its pre-`a46c0189` signature (5
parameters, no `suppressPerPhaseRegister` flag) since the flag is now
handled entirely at the bucket boundary. The exported
`buildCoherentMeterValue` retains its `registerValuesWithoutPhases`
parameter and the strategy-gate wire in `OCPPServiceUtils.buildMeterValue`
is unchanged.
One test added exercising the synthesis path:
- `should synthesize aggregate Energy.Active.Import.Register when only
per-phase L-N templates configured and registerValuesWithoutPhases=true`
asserts 1 sample survives, the surviving sample has no phase
qualifier, the value equals the total register (not
`register / phases`), and the synthesized template inherits the unit
from the first suppressed L-N.
README updated to describe the pre-filter + synthesis semantic and to
weaken the OCPP 1.6 wording (`getConfigurationKey` returns `undefined`,
`convertToBoolean(undefined) === false`) matching the `4dcb6213`
comment fix.
Test invariant `fail: 0, skipped: 6` preserved.
Spec citation: OCPP 2.0.1 edition 3 part 2, `SampledDataCtrlr`,
`RegisterValuesWithoutPhases`.
R5 (maintainer perspective) flagged missing OCPP 2.0 service-boundary
regression tests: the existing suite covered `buildCoherentMeterValue`
directly but not the `buildMeterValue` strategy gate that resolves the
`SampledDataCtrlr.RegisterValuesWithoutPhases` variable and threads it
into the coherent builder on an OCPP 2.0.1 station.
Two tests added to `StrategyDispatch.test.ts`:
1. `should synthesize aggregate register at buildMeterValue boundary
when the SampledDataCtrlr variable resolves to true`. Uses
`addConfigurationKey(station, buildConfigKey(...), 'true')` to set
the component-scoped key exactly as the runtime does, injects a
3-phase coherent session, configures only per-phase L-N Energy
templates, calls `buildMeterValue`, and asserts exactly one
synthesized aggregate sample survives with no phase qualifier.
2. `should preserve per-phase L-N emission when the SampledDataCtrlr
variable is absent`. Same setup without the configuration key;
asserts all 3 per-phase L-N samples emit (default behavior
preserved when the variable does not resolve).
These tests exercise the full wire path: `isOCPP20FlagEnabled` reads
the config store, threads the boolean into `buildCoherentMeterValue`'s
`registerValuesWithoutPhases` parameter, which triggers
`applyRegisterValuesWithoutPhases` bucket pre-filtering and (when
appropriate) aggregate synthesis.
Test invariant `fail: 0, skipped: 6` preserved.
* refactor(meter-values): partition RegisterValuesWithoutPhases by identity family (issue #1936 k)
R8 Lane B (OCPP spec + physics + math adversarial review) surfaced a
correctness gap in the previous `applyRegisterValuesWithoutPhases`
helper: it keyed suppression/synthesis by measurand only, so a single
aggregate `Energy.Active.Import.Register` template anywhere in the
bucket satisfied `hasAggregate` and suppressed per-phase L-N
templates in every other identity family. Example: connector
configured with per-phase INLET templates plus an aggregate OUTLET
template would drop the INLET family entirely and synthesize nothing
for it, violating the spec requirement to report the total energy
per configured family.
Redesign the helper to partition templates into identity families
keyed by `(context, format, location, unit)` before applying
suppression/synthesis:
- Per-phase L-N templates in each family are filtered out (no
behavioral change per family).
- Within each family that had per-phase L-N templates but no
aggregate, an aggregate is synthesized from the first suppressed
L-N template of that family (`phase` cleared, other identity
fields inherited via shallow spread).
- Families that had no per-phase L-N templates are untouched.
- Result is re-sorted by `PHASE_RANK` to preserve stable emit order.
Extract the `phaseFamily(t.phase) === 'LineToNeutral'` predicate
into `isLineToNeutralTemplate` per R8 Lane A NIT (DRY, drops
redundant `t.phase != null` guard because `phaseFamily(undefined)`
is always `'Aggregate'`, never `'LineToNeutral'`).
Normalize terminology `L-N per-phase` -> `per-phase L-N` in JSDoc
and wire comments per R8 Lane C NIT (single term per concept).
One regression test added exercising the mixed-family case:
INLET per-phase L-N templates plus an OUTLET aggregate template
with the flag enabled must emit two aggregate samples (INLET
synthesized, OUTLET preserved), not one.
Test invariant `fail: 0, skipped: 6` preserved.
Spec citation: OCPP 2.0.1 DMD
`docs/ocpp2/Appendices_CSV_v1.4/dm_components_vars.csv:213`:
`SampledDataCtrlr;RegisterValuesWithoutPhases;;no;boolean` -
"will only report the total energy over all phases".
* refactor(meter-values): use Map.groupBy in RegisterValuesWithoutPhases helper (issue #1936 k)
Redesign `applyRegisterValuesWithoutPhases` per session-established
criteria for elegance, harmonization, and TS/JS state-of-the-art:
- Replace the imperative two-Maps-plus-double-loop dance
(`perPhaseLNByFamily` + `otherByFamily`) with a single
`Map.groupBy(bucket, templateFamilyKey)` call, harmonizing with the
sibling `groupTemplatesByMeasurand` helper in the same file. Both
now use the same ES2024 grouping primitive for the same conceptual
operation.
- Extract `templateFamilyKey` to module scope alongside the sibling
`phaseFamily` and `isLineToNeutralTemplate` helpers, matching the
file's helper-placement convention (private module-scope pure
functions above the exported builder).
- Inline the synthesis into a single `surviving.push(...)` call
(removes the intermediate `synthesized` local).
- Net -4 lines with clearer control flow: per-family filter for
L-N vs non-L-N, three explicit branches (no L-N to suppress,
aggregate already configured, synthesize from first L-N),
followed by a single stable sort.
No behavioral change: the mixed-family regression test at
`CoherentMeterValues.test.ts:1256+` (INLET per-phase L-N +
OUTLET aggregate) still asserts both families emit an aggregate
(INLET synthesized, OUTLET preserved). All prior R1-R8 tests
pass unchanged.
Test invariant `fail: 0, skipped: 6` preserved.
* docs(meter-values): describe per-family synthesis in README and JSDoc (issue #1936 k)
R10 Lane B cross-artifact alignment audit caught 2 MINOR wording drifts
carried over from the pre-R8 (pre-family-partitioning) design that R9
Lane C had implicitly accepted as converged:
1. `README.md:496` Energy.Active.Import.Register bullet described
synthesis as connector-global ("if no aggregate register template
is configured, one is synthesized...") without the per-family
qualifier. HEAD's `applyRegisterValuesWithoutPhases` groups
templates into identity families keyed by `(context, format,
location, unit)` and synthesizes an aggregate per family that
lacks one, so the wording was implying a single global aggregate
where the code emits one per family.
2. `CoherentMeterValueBuilder.ts` `buildCoherentMeterValue` JSDoc
`@param registerValuesWithoutPhases` used the same pre-family
phrasing ("if the connector configures only per-phase L-N
templates (no aggregate), an aggregate template is synthesized").
The helper JSDoc was already accurate; only the exported API
documentation lagged the redesign.
Both descriptions rewritten to state the identity-family key
explicitly and the per-family synthesis semantic. No code change.
refactor(meter-values): rename Prng.ts to PRNG.ts (issue #1936 c) (#1940)
* refactor(meter-values): rename Prng.ts to PRNG.ts (issue #1936 c)
PRNG is the canonical uppercase abbreviation (Pseudo-Random Number
Generator). Uppercase acronyms in file names align with the codebase's
TypeScript PascalCase-for-modules convention (e.g. `OCPPServiceUtils.ts`).
`Prng.ts` was the outlier.
Rename performed via two-step `git mv` to survive case-insensitive
filesystems (macOS APFS default). Updates:
- Path imports (4 sites): `CoherentSampleComputer.ts`, `CoherentSession.ts`,
`CoherentMeterValues.test.ts`, `PRNG.test.ts`.
- JSDoc `{@link}` module references (3 sites): `CoherentSession.ts` (2),
`index.ts` (1).
- JSDoc backtick module references (2 sites): `CoherentSession.ts` (1),
`CoherentMeterValues.test.ts` (1).
- Test suite description string: `describe('Prng', ...)` to
`describe('PRNG', ...)` in `PRNG.test.ts`.
Cosmetic; zero runtime change. Test invariant `fail: 0, skipped: 6`
preserved (2895 pass / 2901 total). Build passes.
Closes issue #1936 item (c).
* style(meter-values): normalize prose em-dash to ASCII on PR-C-touched lines (issue #1936 c)
R1 Lane C flagged em-dash (U+2014) in `+` lines of PR-C delta:
- `src/charging-station/meter-values/CoherentSession.ts:71` (JSDoc backtick reference line
updated by rename)
- `src/charging-station/meter-values/index.ts:23` (JSDoc `{@link}` reference line updated
by rename)
Both lines were modified by the PR-C rename (`Prng.ts` -> `PRNG.ts` reference updates) and
therefore appear in the branch `+` diff. Session ASCII rule permits only the section sign
(U+00A7) plus pre-existing physics/math notation in JSDoc as non-ASCII exceptions; em-dash
as prose punctuation is not covered.
Fix: replace both `--` (U+2014) with ASCII `-`. Semantic content preserved.
Em-dashes on lines NOT modified by PR-C (CoherentSession.ts:15, index.ts:14/18/21,
PRNG.test.ts x3, CoherentMeterValues.test.ts x1) remain SUB-THRESHOLD per pre-existing
outside line-level delta convention.
Test invariant `fail: 0, skipped: 6` preserved. Build passes.
`CoherentSample.energyRegisterWh` (in `CoherentSampleComputer.ts`) is
computed from `connectorStatus.transactionEnergyActiveImportRegisterValue
+ deltaEnergyWh` (transaction-scoped) at sample-compute time, but the
emitted `Energy.Active.Import.Register` measurand reads
`connectorStatus.energyActiveImportRegisterValue` (station-scoped) via
`resolvePhasedValue` in `CoherentMeterValueBuilder.ts`. The two counters
diverge whenever the station has non-zero pre-transaction register
history, converging only when the transaction begins on a station whose
register was previously zero.
Documents the divergence on the field's JSDoc so tests and in-process
introspection paths reading `sample.energyRegisterWh` understand the
scope distinction. No runtime change.
* fix(meter-values): tighten L-L voltage guard to 3-phase-only (issue #1936 h.1)
`resolvePhasedValue`'s Line-to-Line voltage branch (in
`CoherentMeterValueBuilder.ts`) previously used `numberOfPhases <= 1` as
the skip guard, which meant that `numberOfPhases === 2` silently emitted
`Math.sqrt(2) * voltageV` -- a physically meaningless value.
2-phase AC is unsupported by contract across the codebase:
`Helpers.getPhaseRotationValue` branches only on `{0, 1, 3}` (AC 1-phase,
AC 3-phase, and DC), and the station-template validator enforces
numberOfPhases in `{0, 1, 3}`. The `<= 1` guard was written assuming the
enum-open case would default to the 3-phase computation; the 2-phase gap
escaped review because no valid station configuration reaches it in
production.
Tightens the guard to `numberOfPhases !== 3` so any non-3-phase
configuration (0, 1, 2, or hypothetical future values) returns
`undefined`, letting the caller log-and-skip rather than emit
`sqrt(2) * V_LN` nonsense. Same behavior for 3-phase (still emits
`sqrt(3) * V_LN`) and for 1-phase (still skipped). Adds a WHY comment
documenting the `V_LL = sqrt(3) * V_LN` contract.
The h.1 guard tightened `numberOfPhases <= 1` to `numberOfPhases !== 3`,
widening the L-L voltage skip predicate from "1-phase only" to "any
non-3-phase count". The `resolvePhasedValue` JSDoc header still
described the pre-h.1 skip case as "`sqrt(phases)` collapses to 1 on
single-phase, in which case L-L has no physical meaning and the
template is skipped" -- factually incomplete post-h.1 (N=0/2/4+ also
skip through the same predicate).
Updates the JSDoc to reflect the current predicate: "L-L is defined
only for balanced 3-phase AC; skipped for any other phase count".
Documentation-atomicity fix per AGENTS.md instruction to "update code,
tests, and documentation atomically".
No runtime change. Test invariant `fail: 0, skipped: 6` preserved
(2925 pass / 2931 total).
* style(meter-values): normalize prose em-dash and ellipsis to ASCII (issue #1936 h)
The session ASCII rule permits only the section sign (U+00A7) plus
pre-existing physics/math notation in JSDoc as non-ASCII exceptions.
It does not permit em-dash (U+2014) or horizontal ellipsis (U+2026)
as prose punctuation. Both symbols pre-existed in the 2 files touched
by PR-H (5 em-dashes in `CoherentMeterValueBuilder.ts`, 14 em-dashes
and 1 ellipsis in `CoherentSampleComputer.ts`) and were classified
through R1-R8 as SUB-THRESHOLD per scope-discipline convention
(pre-existing outside the line-level PR-H delta).
Under explicit user directive to normalize (Option B follow-up),
replaces all 20 prose Unicode occurrences with ASCII (U+2014 to `-`,
U+2026 to `...`) in the 2 touched files. Physics/math notation
(sqrt, multiplication dot, implication arrow, sigma, phi, element-of,
delta, less-or-equal, epsilon) preserved per pre-existing JSDoc
convention.
OCPP 2.0.1 F06.FR.06: when a CSMS sends a TriggerMessage with
MessageTrigger.MeterValues, the Charging Station SHALL respond with a
MeterValuesRequest carrying the requested Measurand values.
The current OCPP20IncomingRequestService.handleRequestTriggerMessage
handler for MessageTrigger.MeterValues iterated all connectors with
active transactions and sent TransactionEventRequest(Updated) — a
spec violation introduced by PR #1726. It only fell back to sending
a MeterValuesRequest when no active transactions existed.
Fix: always emit MeterValuesRequest for MessageTrigger.MeterValues.
- Iterate target EVSEs (specific from evse.id, or all if unspecified).
- Per EVSE, build one MeterValue per connector with an active
transaction using the OCPP 2.0.1 AlignedDataCtrlr.Measurands
allow-list and TRIGGER ReadingContext (matches F06 semantics for
aligned/triggered data). Sub-schema MeterValues with empty
sampledValue arrays are skipped per MeterValueType.sampledValue's
1..* cardinality.
- Emit one MeterValuesRequest per target EVSE aggregating those
MeterValues. When an EVSE has no active transactions, emit one
request with a schema-conforming placeholder so the CSMS observes
the response.
Test updates:
- Remove the MeterValues entry from the shared parameterized 'fires
requestHandler once' loop (that assertion baked in the pre-fix
1-call fallback behavior).
- Add a dedicated F06.FR.06 broadcast test asserting one
MeterValuesRequest per EVSE (3 EVSEs → 3 requests) — symmetric
with the existing StatusNotification broadcast test.
Test invariant fail: 0, skipped: 6 preserved.
Closes issue #1936 item (j).
* fix(ocpp20): tag trigger MV placeholder with Trigger context and measurand (issue #1936 j)
The placeholder SampledValue emitted when an EVSE has no active transaction
previously omitted both `context` and `measurand`, silently defaulting to
`Sample.Periodic` and `Energy.Active.Import.Register` per the OCPP 2.0.1
`MeterValuesRequest` schema. TC_F_12_CS (F06.FR.06 certification) asserts
`meterValue[0].sampledValue[0].context = 'Trigger'` on trigger-elicited
MeterValuesRequests, and reporting `Energy.Active.Import.Register = 0`
misleads the CSMS into recording a spurious energy register reset.
Sets `context = OCPP20ReadingContextEnumType.TRIGGER` and
`measurand = OCPP20MeasurandEnumType.POWER_ACTIVE_IMPORT` on the placeholder
so the wire payload is truthful (`0 W` when no charging is in progress)
and consistent with the surrounding trigger emission's declared context.
Extends the broadcast test to assert per-EVSE payload shape (command,
`evseId > 0`, `meterValue` non-empty, `sampledValue[0].context = Trigger`,
request options) mirroring the sibling StatusNotification broadcast test.
* test(ocpp20): use numeric comparator on trigger meterValues broadcast assertion (issue #1936 j)
`Array.prototype.sort()` sorts as strings by default; the coincidence with numeric
ordering only holds for the current fixture's single-digit EVSE ids `{1, 2, 3}`.
If a future fixture bumps `evsesCount` beyond 9, the assertion would silently
reorder (`['1','10','2','3']`) and mask a real bug.
Uses an explicit `(a, b) => a - b` numeric comparator so the sort behavior is
fixture-count-agnostic.
Extends the F06.FR.06 broadcast test to assert
`sampledValue[0].measurand = Power.Active.Import` on the placeholder path,
locking the design tradeoff: the placeholder emits
`Power.Active.Import = 0 W` (truthful when idle) rather than reading the
first entry from `AlignedDataCtrlr.Measurands` (whose default value
`Energy.Active.Import.Register = 0 Wh` would imply a cumulative register
reset).
F06.FR.09 requires "current information only"; a 0-value energy register
is a stronger lie than a 0-value power flow when no transaction is
running. The measurand assertion prevents a future regression that
swaps the placeholder for the schema-default measurand.
Harmonises the OCPP 2.0.1 TriggerMessage(MeterValues) case with the sibling
`triggerStatusNotification` / `triggerAllEvseStatusNotifications` pattern
already established in the same class. The 66-line inline case is replaced
by a single-line delegation, matching the visual rhythm of every other
switch case.
Design refinements enabled by the extraction:
- `emitEvseMeterValues` now consumes the `evseStatus` yielded directly by
`iterateEvses(true)`, removing a redundant Map lookup on every broadcast
target (matches the sibling `triggerAllEvseStatusNotifications` idiom).
- The unreachable `evseStatus == null` guard in the broadcast path is
eliminated; the type is narrowed once at the top of the specific-EVSE
branch.
- The F06.FR.06 spec-citation comment block is lifted to the helper's
JSDoc, composing F06.FR.10 (Accepted messages SHALL be sent) and
F06.FR.11 (evse absent -> broadcast to all EVSEs) references.
* fix(ocpp20): guard buildMeterValue against synchronous throw in trigger path (issue #1936 j)
`buildMeterValue` can throw synchronously - the sibling
`OCPP20ServiceUtils.buildTransactionStartedMeterValues` (lines 165-183)
wraps it in try/catch and logs on failure. `emitEvseMeterValues` was
calling the same builder unguarded; a synchronous throw would have
unwound through the EventEmitter listener without ever reaching the
downstream `.catch(errorHandler)` (which only observes Promise
rejection).
Mirrors the sibling defensive pattern: wraps the per-connector
`buildMeterValue` call in try/catch, logs the error with the module
scope name and forwarded reason, and continues the outer loop so that
one bad connector does not prevent the remaining connectors' meter
values from being aggregated into the outgoing MeterValuesRequest.
feat(ocpp16): wire signed meter values into trigger + broadcast paths (issue #1936 d) (#1945)
* feat(ocpp16): wire signed meter values into trigger + broadcast paths (issue #1936 d)
OCPP 1.6 Signed Meter Values whitepaper v1.0 requires that when
SampledDataSignReadings + SampledDataSignUpdatedReadings are enabled
and a signing key is configured, a paired SignedData SampledValue
accompanies the Raw SampledValue in every Sample.Periodic-context
MeterValue. The periodic loop (OCPP16ServiceUtils.startUpdatedMeterValues)
already applies POST-HOC signing after buildMeterValue returns, so
periodic MeterValues are signed today.
Two callsites were missing the signing wrapper:
1. OCPP 1.6 TriggerMessage(MeterValues) handler in
OCPP16IncomingRequestService.ts (both the specific-connectorId
branch and the broadcast-to-all-connectors branch).
2. Worker broadcast channel in
ChargingStationWorkerBroadcastChannel.handleMeterValues
(cross-version handler; signing is 1.6-only per the whitepaper).
Both paths silently emitted unsigned Raw values instead of the
whitepaper-mandated paired SignedData SampledValue when signing was
enabled.
Fix: consolidate the signing block from startUpdatedMeterValues into
a new public static helper OCPP16ServiceUtils.appendSignedUpdatedReadings
that mutates the MeterValue in place. The helper is a no-op when
signing is disabled or the connector's signing config is missing.
Apply the helper at the three missing callsites and refactor
startUpdatedMeterValues to call it too.
Spec citation: OCA Application Note 'Signed Meter Values for OCPP 1.6'
v1.0 §3.2.1 (paired SignedData SampledValue).
Closes issue #1936 item (d).
* docs(ocpp16): apply round-1 review fixes on signed MV wire-up (issue #1936 d)
Address cross-validated R1 findings:
- Naming coherence in TriggerMessage broadcast-to-all branch: rename
loop-local variables from abbreviated 'id'/'cs'/'txId'/'mv' to
full 'connectorId'/'connectorStatus'/'transactionId'/'meterValue'
to match the specific-connector sibling branch style. Per AGENTS.md
naming coherence: two adjacent branches doing the same thing with
different name styles is the 'synonym creates ambiguity' pattern.
- Rewrite the broadcast channel guard comment to correctly attribute
the '!isOcpp2' skip. Previous wording claimed 'the whitepaper is OCPP
1.6 specific', but the whitepaper §4 explicitly covers OCPP 2.x. The
actual reason for the skip is that OCPP 2.0.x signing is applied
inline inside buildMeterValue via the versioned dispatcher's signing
hook, so post-hoc wrapping is the 1.6 pattern only.
- Tighten the appendSignedUpdatedReadings @description to drop
refactor-history narrative ('Consolidates the signing block used by
every trigger/broadcast path...'). Replace with operational spec
citing whitepaper §3.3.6 SampledDataSignUpdatedReadings and the
mutation contract on connectorStatus.publicKeySentInTransaction
(per PublicKeyWithSignedMeterValue = OncePerTransaction).
* fix(ocpp16): harden signed MV helper and thread reading context (issue #1936 d)
Enforce the transactionStarted invariant inside `appendSignedUpdatedReadings` so callsites
with looser guards cannot leak signed emissions past `resetConnectorTransactionStatus`.
Accept an optional reading `context` parameter (default `Sample.Periodic`);
`TriggerMessage(MeterValues)` callsites pass `Trigger` per OCPP 1.6 Core Table 30 so the
signed payload's context field matches its emission source.
Downgrade the `readSigningConfigForConnector` disabled-state log from `warn` to `debug`;
the helper is a hot-path probe whose `undefined` return already conveys the disabled state
to callers.
refactor(charging-station): extract reservation helpers from Helpers.ts (issue #1936 f Phase 1) (#1946)
* refactor(charging-station): extract reservation helpers from Helpers.ts (issue #1936 f)
Phase 1 of the Helpers.ts file split tracked in issue #1936 item (f).
Helpers.ts is 1389 LOC — 5.5x the 250 LOC ceiling documented in
AGENTS.md's programming skill. This PR extracts the smallest cohesive
block (the 4 reservation helpers) into a dedicated file as an initial
step, proving the barrel-preservation pattern so the remaining slices
can follow in subsequent PRs without breaking any callers.
Helpers.ts (1389 -> 1336 LOC) keeps the public API via a barrel
re-export block. Every external caller ('import { ... } from
"./Helpers.js"') continues to work unchanged. Now-unused imports
in Helpers.ts (isPast, Reservation, ReservationTerminationReason)
are dropped.
Apply round-1 review findings on HelpersReservation.ts:
- Banner alignment: 'Copyright ... 2021-2026' → 'Partial Copyright ...
2021-2025' to match sibling files in src/charging-station/ that carry
banners (ChargingStation.ts, Bootstrap.ts, BootstrapStateUtils.ts,
CoherentMeterValuesManager.ts, AutomaticTransactionGenerator.ts,
ChargingStationWorker.ts). A repo-wide year bump to 2026 is out of
scope for this refactor.
- Strip historical narrative from @description per AGENTS.md
documentation convention ('document current state; exclude historical
evolution'). Old text narrated the extraction event ('Extracted from
./Helpers as the first slice of the issue #1936 (item f) file split'),
which the commit message + git blame already carry permanently.
Replace with an operational spec describing the module's current
responsibility: reservation-state predicates + bulk expired-reservation
cleanup.
- Add missing @returns on removeExpiredReservations. Every other
exported helper documents its return; the async cleanup did not.
New wording also encodes the WHY (Promise.allSettled → never
rethrows, individual failures logged) that was previously only
implicit in the body.
* docs(helpers-reservation): harmonize JSDoc voice across predicates (issue #1936 f)
Align the three reservation predicates to a single JSDoc voice:
'hasPendingReservation' and 'hasPendingReservations' were 'Checks
if …'; 'hasReservationExpired' was already 'Determines whether …'
after the R1 doc-fix. Standardize the two predicates to
'Determines whether …' so all three read consistently. Leave
'removeExpiredReservations' as imperative ('Removes every …') since
it is a mutation, not a predicate — different verb class,
appropriate.
Per AGENTS.md 'Naming coherence' — semantically accurate names
across code and documentation, avoid synonyms that create
ambiguity.
Nine inline comments in OCPP20IncomingRequestService.ts (7) and its
sibling test file OCPP20IncomingRequestService-UpdateFirmware.test.ts
(2) carried internal audit-round finding numbers (H9, H10, H11, C10,
C12) as if they were code identifiers.
Production-file markers (7) originate from commit 3a6e89f634
'fix(ocpp20): remediate all OCPP 2.0.1 audit findings (#1726)' which
predates PR #1935. Test-file markers (2 H11) originate from commit b50f9e5f 'refactor(tests): separate handler/listener tests and remove
setTimeout hacks' — same audit-round family, later commit.
The audit markers had the shape //<letter+><digits>[:-)] which is
distinct from the legitimate OCPP spec references elsewhere in the
repo (B11 - Reset, F06.FR.06, A02.FR.06, E05.FR.09, L01.FR.30,
C10.FR.04, C12.FR.05, ...) that all follow <UC>.FR.<NN>.
Each rewrite replaces the marker with a WHY-rationale or BDD-style
expectation describing the current-state behavior. See the PR body
table for the full before/after list.
Verification (POSIX character classes; portable across all git-grep -E
builds — the \s shorthand is a PCRE extension not supported by POSIX
ERE and silently returns no match on macOS git):
refactor(meter-values): split CoherentMeterValuesGenerator into three modules (issue #1936 item i) (#1938)
* refactor(meter-values): split CoherentMeterValuesGenerator into three modules (issue #1936 item i)
Split the 820 LOC `CoherentMeterValuesGenerator.ts` (introduced by PR
#1935) into three focused modules per issue #1936 item (i):
- `CoherentSampleComputer.ts` (311 LOC): physics chain V→P→I→ΔE→SoC with
INV-1/2/3 by construction (guard bundle, ramp factor, voltage noise,
EVSE/EV/capacity clamps, integer-rounded emission, energy accrual,
monotone SoC saturation), plus `advanceEnergyRegister` which the
builder invokes once per sample so `meterStop` stays correct even when
`Energy.Active.Import.Register` is not in the configured MeterValues.
- `CoherentMeterValueBuilder.ts` (364 LOC): OCPP {@link MeterValue}
assembly — phase family classifier, cross-measurand emit order
(SoC→Voltage→Power→Current→Energy), within-measurand phase rank
(no-phase → L1/L1-N → L2/L2-N → L3/L3-N → L1-L2 → L2-L3 → L3-L1 → N),
kilo/unit divider, template resolution, and the `buildCoherentMeterValue`
entry point that orchestrates `computeCoherentSample` +
`advanceEnergyRegister` + per-template emission.
- `CoherentMeterValuesGenerator.ts` (204 LOC, shrunk from 820): session
lifecycle (`createCoherentSession`, `CreateSessionOptions`), PRNG
helpers (`createStreamPrng` with the `tx:`-namespaced deriveSeed leg,
`resolveRootSeed`), the `isCoherentModeActive` type guard, and the
module-scope WeakMap runtime state (`SessionRuntime`,
`getSessionRuntime`, `disposeCoherentSessionRuntime`) accessed by the
computer.
The three modules stack: Generator (WeakMap + PRNG helpers) ← Computer
(imports `getSessionRuntime`, `createStreamPrng`) ← Builder (imports
`computeCoherentSample`, `advanceEnergyRegister`, `ROUNDING_SCALE`,
`CoherentSample`, `ComputeSampleOptions`). No cycles.
The barrel (`meter-values/index.ts`) re-exports `buildCoherentMeterValue`
+ `BuildVersionedSampledValue` from the builder and
`createCoherentSession` / `isCoherentModeActive` / `resolveRootSeed`
from the generator so external consumers
(`CoherentMeterValuesManager`, `OCPPServiceUtils`, `OCPP16ServiceUtils`)
keep their existing imports.
Preserved invariants (per mission constraints):
- `__injectCoherentSession` NODE_ENV production-guard throw
(unchanged — lives on `CoherentMeterValuesManager`).
- Session-snapshot reads for `voltageOutNominal` / `currentType` /
`numberOfPhases` (still read from `session.*` inside
`computeCoherentSample`).
- `tx:` PRNG namespace on the transactionId leg of `createStreamPrng`
(preserves the XOR self-inverse guard).
- non-finite-`intervalMs` defensive zero-sample guard
preserved verbatim in `computeCoherentSample`.
Test file imports updated to point at the new module boundaries; no test
logic changes. Test invariant `fail: 0, skipped: 6` holds.
Closes issue #1936 item (i).
* refactor(meter-values): align banner and tighten JSDoc on new split modules (issue #1936 i)
Revert the copyright banner on the three files introduced by the split
(CoherentSampleComputer.ts, CoherentMeterValueBuilder.ts, index.ts) from
'Copyright ... 2021-2026' to 'Partial Copyright ... 2021-2025' to match
the sibling verbatim convention (AutomaticTransactionGenerator.ts,
ChargingStation.ts, CoherentMeterValuesManager.ts, PerformanceStatistics.ts).
Wider repo-wide banner year sweep to 2021-2026 is deferred to a separate
chore commit.
Tighten two hedge-worded JSDoc lines in CoherentSampleComputer.ts:
- 'Sample timestamp in milliseconds (typically Date.now())' now spells
out 'callers pass Date.now() in production and a test-controlled clock
in unit tests'.
- Defensive-guard rationale 'a template override or future dynamic
supply could return 0' tightened to 'a template override may set
voltageOut to 0'.
CoherentMeterValuesGenerator.ts is intentionally not touched here to
avoid four-way merge collisions with #1940 (Prng rename), #1941
(physics refinements h), #1942 (RegisterValuesWithoutPhases k), and
#1944 (EVSE-level template fallback g). Design-MAJOR findings (inverted
Computer → Generator dependency, widened intra-package exports,
'Generator' semantic misnomer) are documented in the PR body as a
post-stack-merge follow-up.
Drop the 'Partial' prefix on the three files introduced by the split.
Round-1 aligned to parent-folder siblings (ChargingStation.ts,
AutomaticTransactionGenerator.ts, CoherentMeterValuesManager.ts) which
use 'Partial Copyright ... 2021-2025', but same-folder siblings
(EvProfiles.ts, Prng.ts, types.ts) use 'Copyright ... 2021-2025'
without the prefix. Aligning to same-folder convention restores
banner uniformity inside src/charging-station/meter-values/.
CoherentMeterValuesGenerator.ts (2021-2026 outlier) remains untouched
to avoid the four-way merge collision with #1940 / #1941 / #1942 /
#1944 — repo-wide banner sweep is deferred as documented.
* refactor(meter-values): unidirectional DAG for coherent split (issue #1936 i)
Address the design findings raised by post-split review by relocating
runtime state and PRNG plumbing to their responsibility owners:
- Move SessionRuntime, sessionRuntimes, getSessionRuntime to
CoherentSampleComputer.ts as module-private (they exist only to cache
the voltage-noise PRNG closure across samples, which is a physics
concern, not a session-lifecycle concern).
- Move disposeCoherentSessionRuntime to CoherentSampleComputer.ts
(exported for CoherentMeterValuesManager). Update the manager's
direct import path accordingly.
- Move createStreamPrng to Prng.ts alongside deriveSeed / mulberry32 /
hashLabel. It composes those three helpers and is a PRNG primitive,
not a session helper.
- Rename CoherentMeterValuesGenerator.ts to CoherentSession.ts. After
the moves the file owns only session identity (createCoherentSession,
CreateSessionOptions), the strategy-gate type guard
(isCoherentModeActive), and the root-seed resolver (resolveRootSeed).
The 'Generator' name no longer describes the file — the entry point
buildCoherentMeterValue lives in the builder.
- Align banner on the renamed file to same-folder convention (Copyright
... 2021-2025, matching EvProfiles / Prng / types).
- Update barrel and test imports.
Result: strictly unidirectional import DAG within meter-values/:
CoherentSession no longer depends on any coherent sibling; Computer no
longer depends on Session (the pre-refactor Computer → Session inversion
is eliminated). Three previously-widened intra-package exports revert to
module-private (SessionRuntime, sessionRuntimes, getSessionRuntime);
ROUNDING_SCALE stays exported (Computer → Builder is the correct
direction). Barrel public surface unchanged.
Wire behavior and physics chain byte-identical — the move preserves
every function body verbatim, only relocating them. Test invariant
fail:0, skipped:6 preserved (2925 pass / 2931 total).
The four open PRs touching CoherentMeterValuesGenerator.ts (#1940 c,
#1941 h, #1942 k, #1944 g) will need manual rebase resolution on top
of this refactor — accepted trade-off per the maintainer's directive
to do the design work now rather than defer.
- Retarget the stale '{@link ./CoherentMeterValuesGenerator}' reference
in CoherentSampleComputer.ts @file JSDoc to describe the current state
(physics chain + session-runtime WeakMap + disposeCoherentSessionRuntime
teardown hook) instead of the pre-split historical evolution, per the
AGENTS.md 'document current state; exclude historical evolution' rule.
- Restore the getSessionRuntime JSDoc dropped during the Generator →
Computer move. Documents the load-bearing invariant that the
voltage-noise PRNG closure must be cached across samples so the
stream advances rather than restarting from the same seed each draw.
- Rename CoherentMeterValuesGenerator.test.ts to CoherentMeterValues.test.ts.
Update @file header and describe() label to match the new subject —
the test file now exercises CoherentSession, CoherentSampleComputer,
CoherentMeterValueBuilder, and Prng; no single 'generator' module
remains post-refactor.
- Fix dangling '{@link ../../CoherentMeterValuesManager...}' at
CoherentMeterValueBuilder.ts:287. From src/charging-station/meter-values/,
'../../' resolves to src/ (target does not exist there); correct
path is '../CoherentMeterValuesManager' (src/charging-station/). This
matches the fixes already applied to the sibling occurrences in
CoherentSampleComputer.ts:9 and CoherentSession.ts:37; Builder was
missed during the round-3 sweep.
- Add 'satisfies readonly MeterValueMeasurand[]' to MEASURAND_EMIT_ORDER
in CoherentMeterValueBuilder.ts. Matches the 'as const satisfies
Record<...>' idiom already used by PHASE_FAMILY and PHASE_RANK.
Gates against a typo-introduced non-measurand value at compile time.
- Expand the label-style '// EV acceptance from the curve at running
SoC.' comment in CoherentSampleComputer.ts to document the load-bearing
physics invariant: the taper MUST interpolate at session.socPercent
(live state advanced by advanceEnergyRegister) rather than a constant
or the initial SoC. Without this, a future maintainer could replace
the live read with a constant, breaking the taper (P would not
decrease as SoC rises, over-charging past the capacity clamp,
violating INV-3).
The round-4 physics comment at CoherentSampleComputer.ts:299 stated
that session.socPercent is advanced 'by the previous
advanceEnergyRegister tick', but advanceEnergyRegister (line 180)
only mutates connectorStatus energy registers — it does not touch
session.socPercent. The actual mutation happens at the end of
computeCoherentSample itself (line 354).
Retarget the comment's mutation-site reference to computeCoherentSample
and inline-reference the assignment below. This preserves the
load-bearing invariant that round-4 documented (taper must interpolate
at live SoC, not initial) while pointing the future maintainer to the
correct code location — a maintainer following the wrong pointer to
advanceEnergyRegister would find no session mutation and might
delete the 'stale' comment or replace the live read with a constant,
silently breaking the taper (P would not decrease as SoC rises →
over-charging past capacity clamp → violation of INV-3).
* docs(coherent): strip historical narrative from JSDoc + test prose (issue #1936 i)
Apply AGENTS.md 'document current state; exclude historical evolution'
across the coherent-mode surface (both this PR's meter-values files
and the CoherentMeterValuesManager landed in PR #1937):
- CoherentMeterValuesManager.ts:@file — 'Extracted from ChargingStation'
narrated origin. Rewrite to 'Sits alongside ChargingStation' — same
design rationale (keeping the strictly opt-in coherent surface off
the main class body) without the historical framing.
- CoherentMeterValuesManager.ts:injectSession JSDoc — 'mirroring the
seam previously exposed on ChargingStation' claimed the seam was
no longer on ChargingStation, but __injectCoherentSession is still
a live delegator on ChargingStation (points at manager.injectSession).
Drop 'previously' — the seam mirrors the ChargingStation delegator.
- CoherentMeterValueBuilder.ts:@file — 'Extracted from the original
single-file generator as part of the issue #1936 (item i) file
split to keep each module under the 250 LOC ceiling' was pure
historical narrative; drop the sentence entirely.
- CoherentMeterValueBuilder.ts:resolveTemplates JSDoc — 'tracked as a
follow-up in issue #1936' was TODO-adjacent but redundant with
git-blame trail. Keep the current-state limitation description
(EVSE-level template inheritance needs getEvseStatus on the port).
- meter-values/index.ts:@description — 'Module layout after the
issue #1936 (item i) split:' → 'Module layout:'.
- CoherentMeterValues.test.ts:@file — 'Exercises the split modules
together' → 'Cross-module tests spanning'.
- CoherentMeterValues.test.ts:797 assertion — '(moved to module-scope
runtime state)' → '(owned by module-scope runtime state in
CoherentSampleComputer)'. Same invariant, current-state phrasing.
Extract per-station coherent MeterValues lifecycle owner from
ChargingStation into a dedicated singleton-per-station manager,
mirroring the multiton pattern of AutomaticTransactionGenerator /
IdTagsCache / SharedLRUCache (keyed by stationInfo.hashId).
The manager owns the EV profile file, the active-session Map, and
the create/destroy/inject lifecycle. ChargingStation keeps the four
public methods as thin delegators for API stability — external OCPP
handler call sites, the test helper mock, and existing tests are
unchanged.
Behavior is preserved:
- Sessions are still created only when coherentMeterValues=true AND
a valid EV profile file loads.
- The __injectCoherentSession NODE_ENV production guard is preserved
on the manager side (BaseError throw).
- Session runtime state is disposed at every reset/stop/disconnect
path via destroySession, and at station stop/delete via dispose /
deleteInstance.
Drop `getCoherentSession` from `ICoherentContext`: the port now
exposes only what the physics chain needs to query about the station
itself. Session lookup moves to the caller (the strategy gate), which
looks up via `ChargingStation.getCoherentSession` — the A1 delegator
that routes through `CoherentMeterValuesManager` in production and
through the test-mock's own Map in tests.
Signature changes:
- `isCoherentModeActive(session): session is CoherentSession` — pure
type guard replacing the two-tier (stationInfo + session-lookup)
predicate. The stationInfo gate is implied by session existence
(sessions are only created via the manager when
`coherentMeterValues=true`).
- `buildCoherentMeterValue(context, session, ...)` — takes the session
directly. The internal `context.getCoherentSession` lookup and the
"missing session" warning branch collapse to a single connector-lookup
guard.
Strategy gate call sites in `OCPPServiceUtils.buildMeterValue` and
`OCPP16ServiceUtils.buildTransactionBeginMeterValue` are threaded
accordingly.
Behavior preserved: sessions are still only routed to the coherent path
when they exist; random/fixed path is untouched. Test suite invariant
`fail: 0, skipped: 6` holds.
Split OCPP16ServiceUtils.buildTransactionBeginMeterValue's dual
responsibility. The coherent short-circuit (route through
buildMeterValue with the vendor StartTxnSampledData override) moves to
a named private static helper; the outer function keeps only the
random/fixed default path plus the strategy gate.
The strategy gate in OCPP 1.6 now lives at a single well-labeled
boundary in this file, mirroring the pattern established by
OCPPServiceUtils.buildMeterValue for the periodic MeterValues path.
Behavior is unchanged: same measurand-key resolution, same
buildMeterValue re-dispatch, same TRANSACTION_BEGIN context.
PR-A round-1 Oracle review (lanes A + D converged on this MAJOR
finding): the strategy gate in `OCPPServiceUtils.buildMeterValue`
reaches `ChargingStation.getCoherentSession` unconditionally on every
MeterValue tick, and the delegator was routed through
`CoherentMeterValuesManager.getInstance` — a lazy-create factory.
Result: every station that ever emits a MeterValue allocated a
`CoherentMeterValuesManager` + Map, regardless of the `coherentMeterValues`
opt-in flag. This contradicted the file-header design contract that
'stations with the option off never allocate a manager'.
Fix: add `CoherentMeterValuesManager.peekInstance(cs)` — a lookup-only
sibling of `getInstance` that never constructs. Route the four
read/destroy/dispose sites through `peekInstance`:
- `ChargingStation.createCoherentSession` (opt-in station's manager
is warmed up at initialize; non-opt-in stations return `undefined`
without allocating).
- `ChargingStation.destroyCoherentSession`
- `ChargingStation.getCoherentSession` (the strategy-gate hot path).
- `ChargingStation.stop()` finally-block dispose.
Keep `getInstance` (create-if-missing) at:
- `ChargingStation.__injectCoherentSession` — production-guard
BaseError throw must remain reachable if this test seam is
accidentally called in prod.
- `ChargingStation.initialize` — the opt-in eager warm-up that surfaces
EV-profile-file warnings at startup rather than at first transaction.
Also tighten `getInstance` JSDoc to state precisely that `undefined` is
returned iff `stationInfo.hashId` is not yet resolved (the sole failure
mode), and cross-reference `peekInstance` for read paths.
Behavior for opt-in stations is unchanged (warm-up allocates the same
manager, subsequent reads find it in the cache). Non-opt-in stations
no longer allocate. Test invariant `fail: 0, skipped: 6` holds.
Closes issue #1936 item (a) sub-finding from round-1 review.
* docs(meter-values): tighten getInstance/peekInstance JSDoc post round-2 review (issue #1936)
PR-A round-2 Oracle review (lanes A + B converged on JSDoc precision):
- Lane A: `getInstance` JSDoc still listed `createSession` as a caller,
but the round-1 fix routed `createSession` through `peekInstance`.
A future reader following the JSDoc could reintroduce the phantom-
allocation bug at the exact site it was just patched.
- Lane B: JSDoc labeled `destroySession` and `stop()` dispose as
"read-only paths". Both are actually mutations (Map.delete +
PRNG-closure disposal). Reword to "paths that must not allocate a
manager on behalf of non-opt-in stations".
- Lane A NIT: the eager-init invariant is load-bearing — if
`ChargingStation.initialize` stops warming up the manager for opt-in
stations, `createCoherentSession` silently no-ops. The invariant was
implicit; make it explicit in `peekInstance` JSDoc.
Docs-only. No runtime change. Test invariant `fail: 0, skipped: 6` holds.
* fix(meter-values): propagate template reload to coherent manager (issue #1936)
The template file watcher and reset() path already flush sharedLRUCache,
idTagsCache, and OCPPAuthServiceFactory before calling initialize(). The
coherent manager was left behind: getInstance returned the cached
manager for the unchanged hashId, whose evProfiles were snapshotted at
first-construction, so runtime mutations of evProfilesFile or its file
contents did not take effect until process restart.
Wire reloadEvProfiles() into every initialize() invocation so template
mutations propagate. Symmetrically drop the manager entirely when the
coherentMeterValues opt-in flips true to false so cached sessions and
stale profile data do not leak across the config change.
* refactor(meter-values): gate injectSession, harmonize banner and JSDoc (issue #1936)
injectSession test seam now honors the stationInfo.coherentMeterValues
opt-in gate. Pre-refactor, isCoherentModeActive checked the flag on
every tick; post-refactor it trusts session existence. Without the gate,
a test could inject a session on a non-opt-in station and activate the
coherent wire path — contradicting the type-guard precondition.
Production is already protected by the NODE_ENV production throw.
Align copyright banner to the sibling verbatim form
('Partial Copyright Jerome Benoit. 2021-2025').
Harmonize JSDoc: peekInstance MUST-use list now includes createSession
(mirrors getInstance JSDoc); reloadEvProfiles fail-soft description
covers non-opt-in stations and any error; 'cache miss' prose replaced
with 'instances-map miss' to match the field name.
* refactor(meter-values): preserve in-flight coherent sessions across opt-in flag flip (issue #1936)
The previous initialize() logic dropped the CoherentMeterValuesManager
via deleteInstance() when coherentMeterValues flipped true to false.
That immediately disposed every in-flight coherent session — and, in
the template file watcher flow where initialize() runs before
stopAutomaticTransactionGenerator(), that produced mixed-provenance
TransactionEvent frames on OCPP 2.0.x: the Begin frame was coherent,
the Update/End frames after the flip fell through the strategy gate
to random path, and the CSMS saw a discontinuous transaction.
Remove the else branch. New sessions are already blocked by the
coherentMeterValues gate in createSession; existing in-flight
sessions drain naturally via destroyCoherentSession on transaction
end. Provenance is preserved for transactions started before the
flag flip, and the reloadEvProfiles propagation on the true branch
(the round-1 MAJOR fix) is unaffected.
* docs(meter-values): calibrate injectSession JSDoc for mock-helper reality (issue #1936)
The prior JSDoc claimed the opt-in guard means isCoherentModeActive
cannot be tricked into activating the coherent wire path by
injecting on a non-opt-in station. That is true only for the
production-backed injection path (through the real
CoherentMeterValuesManager.injectSession). Tests that mock
ChargingStation write to their own session store bypassing this
seam entirely, so the mock is responsible for enforcing its own
opt-in invariant where relevant. Calibrate the JSDoc scope
accordingly.
Implements physics-coherent MeterValues (V->P->I->dE->SoC) gated by template
flag coherentMeterValues. Session lifecycle on ChargingStation with txId
snapshotted before resetConnectorStatus. Strategy gate after versioned
sampled-value dispatcher, before legacy random measurand generation.
Deterministic Mulberry32 PRNG with per-label stream splitting. New module
under src/charging-station/meter-values/. Golden invariants harness green.
Refs: #40
* test(meter-values): regression tests for Phase 4 findings (issue #40)
RED phase for M1..M4 findings from /tmp/issue-40/review-consolidated.md:
- M1: voltage PRNG must advance state across samples
- M2: deltaEnergyWh must be clamped to remaining battery capacity at 100% SoC
- M3-OCPP16: coherent session destroyed even if station stops during postTransactionDelay
- M3-OCPP20: same for OCPP 2.0 sibling path
- M4: stopEnergyWh assertion strengthened to remove self-reference tautology
- M1: cache voltage PRNG on CoherentSession (was reconstructed each
sample, producing a stalled seed sequence). PRNG state now advances
across samples as documented.
- M2: clamp powerW to remaining battery capacity so a sample crossing
100 % SoC cannot over-charge the register. Everything downstream
(I, ΔE, register) is recomputed from clamped power, preserving
INV-1 and INV-3.
- M3-OCPP16 / M3-OCPP20: destroy the coherent session BEFORE awaiting
postTransactionDelay so an intervening station stop cannot leak the
session. destroyCoherentSession is idempotent so the post-sleep
path remains valid.
Regression tests (M1..M4): pass 23/23.
Full suite: pass 2908/0/6 skipped.
Consolidated fixes for all 30 findings from the multi-agent code review of
PR #1935. Preserves the coherent MeterValues feature scope; adds OCPP 2.0
end-to-end wiring; hardens correctness and idiomaticity.
Blocking (2):
- B1 fix INV-1 breach in capacity-clamp branch: derive current as exact
fraction (P / (V·phases)), round at emit, then derive emitted P from
rounded V·I·phases. Prior integer-amp rounding could inflate V·I·phases
above the capacity-clamped power by up to V·phases·0.5 W. New AC 3-phase
and DC regression tests lock the invariant.
- B2 wire OCPP 2.0.1 support: add createCoherentSession call in
OCPP20ResponseService.handleResponseTransactionEvent (Started case),
mirroring OCPP 1.6. New 5-scenario integration test covering Accepted,
implicit Accept, rejected idToken, force-override, and non-Started events.
High (4):
- H1 clear coherentSessions in ChargingStation.stop() finally; dispose
runtime state per session before delete.
- H2 README: add three template-parameter rows (coherentMeterValues,
evProfilesFile, randomSeed) and a new 'EV profile file format' subsection
documenting the ev-profiles-template.json schema.
- H3 strip process residue: remove /tmp/issue-40/* references (3 files),
'Phase 2 merged finding #N' and 'Fix Phase 4 M-N' markers (8+ locations);
replace with technical rationale.
- H4 label INV-1/INV-2/INV-3 in the class-level JSDoc using the
PR-body-canonical numbering (INV-1=V·I=P, INV-2=SoC monotone,
INV-3=ΔE=P·Δt). Remove undefined INV-4 reference.
Medium (13):
- M1 DRY resolveRootSeed via hashLabel (byte-equivalent, test-locked).
- M2 move voltagePrng runtime state off CoherentSession to a module-scope
WeakMap keyed on session identity (no cross-station coupling); add
disposeCoherentSessionRuntime wired into destroyCoherentSession + stop().
- M3 collapse five identical rounding scales into a single ROUNDING_SCALE.
- M4 add explanatory comment on the boundary 'as MeterValue' cast
(OCPP16/20 SampledValue.context enums structurally diverge).
- M5 correct Prng.ts JSDoc: 'SplitMix32-derived' -> 'Mulberry32 + FNV-1a'.
- M6 remove 'byte-identical' over-claims; use 'reproducible' / 'identical'.
- M7 defensive early-return zero-sample when intervalMs <= 0 to prevent
NaN contamination when SoC has already saturated.
- M8 trim meter-values/index.ts barrel from 22 to 11 externally-consumed
symbols.
- M9 mark 7 immutable CoherentSession fields readonly.
- M10 add ChargingStation.injectCoherentSession() public method and use it
in 3 test sites, replacing 'station as unknown as { coherentSessions }'
private-field injection.
- M11 fix 'transaction id' -> 'transactionId' in Prng.ts comment.
- M12 replace global Date.now monkey-patch with per-iteration
mock.method(Date, 'now', ...) from node:test.
- M13 remove dead chargingProfileLimitW parameter
(getConnectorMaximumAvailablePower already folds in charging profiles).
Low / Nit:
- C4 clear-on-stop (covered under H1).
- C5 XOR-commutativity in deriveSeed: deferred with explanatory comment
(birthday bound ~2^16 well beyond simulator scale; a non-commutative mix
would desync existing golden tests).
- D-7 rename prng.ts -> Prng.ts (and prng.test.ts -> Prng.test.ts) to
match repo PascalCase filename convention.
- D-8 move getEvProfilesFile from EvProfiles.ts to Helpers.ts next to
getIdTagsFile.
- D-9 fix resolveTemplates JSDoc: remove false 'mirrors EVSE lookup' claim.
- D-11 CoherentMeterValuesDefaults now exposes all tunable constants.
- D-18 use AvailabilityType.Operative enum instead of string cast.
- I1 auto-resolved by B1 (ROUNDING_SCALE=2 now semantically meaningful
for current).
- T2 use getErrorMessage() (repo convention) instead of (error as Error).
- S2 simplify templatesFor test helper — remove 'as unknown as { unit }'
casts.
- S5 consolidate duplicate BuildVersionedSampledValueFn type into the
canonical BuildVersionedSampledValue exported from meter-values.
Consolidated fixes for all Medium+Low+Nit findings from the second
multi-agent review round of PR #1935. Two Blocking findings (S1 OCPP 1.6
signed-meter-values wrapper, S2 begin MeterValue routing) — S2 implemented,
S1 deferred to a follow-up (post-hoc signing in startUpdatedMeterValues
already covers the OCPP 1.6 periodic path; the remaining gap is only
TriggerMessage/broadcast callsites, best served by a dedicated PR with a
proper signing-key test fixture).
## Blocking (1 of 2 addressed; other deferred)
- **S2 (implemented)** — `Transaction.Started` / `Transaction.Begin`
MeterValue is now generated by the coherent path. Made
`ChargingStation.createCoherentSession` idempotent so early
request-builder creation is safe; reordered OCPP 2.0 flows
(`OCPP20ServiceUtils.startTransactionOnConnector`,
`OCPP20IncomingRequestService` RequestStartTransaction handler) to
create the session BEFORE `buildTransactionStartedMeterValues`;
reordered OCPP 1.6 flow (`OCPP16ResponseService.handleResponseStartTransaction`)
and added a coherent-mode branch to
`OCPP16ServiceUtils.buildTransactionBeginMeterValue` that routes
through `buildMeterValue` when a session is live.
- **S1 (deferred)** — OCPP 1.6 signed-meter-values wrapper for the
coherent path. Rationale: `startUpdatedMeterValues` in
`OCPP16ServiceUtils` applies post-hoc signing after `buildMeterValue`
returns, so the OCPP 1.6 periodic coherent path IS signed today. The
actual gap is only the TriggerMessage / worker-broadcast callsites
where signing is skipped. Fixing it cleanly requires a signing-key
test fixture that is best set up in a dedicated PR.
## Medium (8)
- **M-01/M-02/M-03/M-07 combined defensive-guard block**
(`computeCoherentSample`): early-return zero-sample when `intervalMs ≤ 0`
(existing), `batteryCapacityWh ≤ 0` or non-finite (M-01), or
`voltageOut ≤ 0` or non-finite (M-02). `rampUpDurationMs` guard now
requires `> 0 && Number.isFinite(...)` (M-03). Prevents NaN poisoning
and INV-1/INV-3 incoherence across the four sources.
- **M-04** — Reverted the proposed Zod refinement for sorted `chargingCurve`
after design analysis showed `loadEvProfilesFile`'s in-place sort is
the authoritative defense and the refinement would break the loader's
"accepts unsorted, sorts in place" contract. Documented rationale in
`EvProfileSchema` JSDoc.
- **M-06** — Renamed `ChargingStation.injectCoherentSession` →
`__injectCoherentSession` (dunder-prefix test-seam convention);
tightened mock parameter type from `unknown` to `CoherentSession`;
updated the 3 test call sites.
- **M-07** — Fixed the determinism claim in README.md and PR body:
`interval` is a physics parameter, not a PRNG seed input. New prose
makes this explicit.
- **M-08** — Coherent path now respects OCPP 2.0.1 `SampledDataCtrlr.TxUpdatedMeasurands`
/ `TxEndedMeasurands` / `TxStartedMeasurands` and OCPP 1.6
`MeterValuesSampledData`. Added `resolveEnabledMeasurands` helper in
`OCPPServiceUtils.ts` and threaded a `ReadonlySet<MeterValueMeasurand>`
allow-list into `buildCoherentMeterValue`. Governs J02.FR.11 / E02.FR.09
/ E06.FR.11.
## Deferred to follow-up issue (3)
- **M-05** — Extract `CoherentMeterValuesManager.getInstance(chargingStation)`.
Sibling per-station concerns (AutomaticTransactionGenerator, IdTagsCache,
SharedLRUCache) use the multiton pattern; the coherent module currently
keeps state on `ChargingStation`. Not a correctness issue; architectural
refactor best done standalone.
- **N-03** — Blocked on M-05 (semantic circularity in `ICoherentContext.getCoherentSession`
cleaned up naturally when the manager owns the session store).
- **N-04** — `Prng.ts` filename kept as-is; renaming to `PRNG.ts` on a
case-insensitive macOS FS would require a second two-step rename with
no functional benefit.
## Low/Nit (10)
- **N-01** — Dropped `disposeCoherentSessionRuntime` from the
`meter-values/index.ts` barrel; direct sub-module import in
`ChargingStation.ts` (barrel exposes only externally-consumed symbols).
- **N-02** — Rephrased the `Fix B1:` process-comment in
`CoherentMeterValuesGenerator.ts` to invariant-only technical rationale
(H3 miss from the previous round).
- **N-05** — Renamed voltage locals in `computeCoherentSample`:
`voltageNominal`/`voltageV`/`roundedVoltage` →
`nominalV`/`sampledV`/`roundedV` (fewer names for the same quantity).
- **N-06** — Added defensive `destroyCoherentSession` in the OCPP 2.0
`handleResponseTransactionEvent` `Ended` branch when connectorId is
unknown (symmetric with OCPP 1.6 defensive destroy in
`handleResponseStopTransaction`).
- **N-07** — Tightened `deriveSeed` JSDoc with quantified birthday
bound: expected collisions ≈ N²/2^33; at simulator scale (≤ 5×10⁴
pairs) ≈ 0.3 — negligible.
- **N-08** — Tightened AC1/AC3/DC test tolerances from ≤ 1/3/1 W to
≤ 0.01 W (post-Option D tight bound is 0.005 W).
- **N-09** — Uniform `getErrorMessage(error)` in EvProfiles.ts
ZodError branch (was direct `error.message`, inconsistent with the
else branch).
- **N-10** — Removed dead exports `CoherentMeterValuesDefaults` and
`mixSeed` (grep-verified zero external usages).
- **N-11 F-04** — `@file` tag "MeterValue generator" (singular) →
"MeterValues generator" (plural OCPP term).
- **N-11 F-07** — README schema example id aligned with the actual
`ev-profiles-template.json` (`compact-ev-40kwh` → `city-ev-40kWh`).
- **N-11 F-08** — README documents the `initialSocPercentMin` ≤
`initialSocPercentMax` swap-and-warn behavior.
Consolidated fixes for all findings from the third multi-agent review round.
No blocking findings remain; two content-lane blocks (barrel count, README
example) plus 4 cross-validated HIGH nits addressed.
## HIGH (4)
- **H1** — Correct false JSDoc claim at `ChargingStation.ts:319`. The
`__injectCoherentSession` docstring stated the `__` prefix was enforced
by a `no-restricted-syntax` ESLint rule; no such rule exists. Reworded
to "convention only — not currently enforced by a lint rule".
- **H2** — Trim 4 dead type re-exports from `src/charging-station/index.ts`
(`ChargingCurvePoint`, `EvProfile`, `EvProfilesFile`, `ICoherentContext`).
Grep-verified zero external consumers. `CoherentSession` kept (used by
4 test files). Barrel is now honest about its "only externally-consumed
symbols" contract.
- **H3** — Sync README `city-ev-40kWh` schema example to
`src/assets/ev-profiles-template.json` exactly: `maxPowerW` 7400→11000,
`weight` 1.0→3, `initialSocPercentMin` 10→15, `initialSocPercentMax`
80→55, 3-point curve → 4-point taper. First-time readers now see the
same values in both docs.
- **H4** — Validate CSV entries in `resolveEnabledMeasurands`
(`OCPPServiceUtils.ts`). `Object.values(MeterValueMeasurand)`-membership
guard drops unknown entries and logs a per-station-debounced warning.
Prevents silent typo tolerance (e.g. `Voltege` in config CSV) while
accepting every OCPP-defined measurand (unlike the narrower
`isMeasurandSupported` allowlist).
## MED (3)
- **M1** — Thread `TxUpdatedMeasurands` into `buildMeterValue` at both
OCPP 2.0 sites that previously bypassed it: `OCPP20ServiceUtils.ts`
`startUpdatedMeterValues` (periodic path) and
`OCPP20IncomingRequestService.ts` `TriggerMessage(MeterValues)` handler.
Closes the J02.FR.11 spec gap where CSMS-configured measurand filters
were ignored on the periodic Updated path.
- **M2** — Debug-only sortedness assertion in `interpolateChargingCurve`
(`EvProfiles.ts`). Production path (`loadEvProfilesFile`) sorts in
place; the assertion catches misuse from the `__inject*` test seam
(unsorted curves silently returned the tail power-fraction — a hidden
test footgun). `process.env.NODE_ENV !== 'production'` gate keeps
runtime cost at zero in production.
- **M3** — Session-leak safety net at 2 OCPP 2.0 request-time create sites:
`OCPP20ServiceUtils.ts` `startTransactionOnConnector` wraps
`sendTransactionEvent` in `try/catch`; `OCPP20IncomingRequestService.ts`
`RequestStartTransaction` handler augments the existing `.catch` chain.
Both call `destroyCoherentSession(txId)` before re-throwing/logging,
symmetric with the OCPP 1.6 `resetConnectorOnStartTransactionError`
pattern. Prevents session Map growth on WebSocket-send rejection.
## Content + Low/Nit
- **P-01** — Extend the defensive guard block in `computeCoherentSample`
with `!Number.isFinite(options.nowMs)`. Matches the pattern for the
other 4 guarded inputs. Not reachable in production (`Date.now()`
always finite) but closes the test-injection hole.
- **P-02** — Document `rampUpDurationMs = Number.EPSILON` equivalence
with 0 (both collapse to immediate full-power). Comment only.
- **D-05** — Add `logger.warn` to the OCPP 2.0
`handleResponseTransactionEvent` Ended-with-unknown-connector branch,
mirroring the OCPP 1.6 diagnostic parity at
`OCPP16ResponseService.ts:534-536`.
- **N-03** — Expand `ComputeSampleOptions.voltageNoise` inline comment
into a full JSDoc with `@remarks` and `@defaultValue`.
- **N-04** — Document the WHY for `createCoherentSession` idempotency
(OCPP 2.0.x has two call sites per transaction from S2 fix).
- **N-05** — Document the `<quantity><Unit>` naming convention on
`CoherentSample` (all fields follow `currentA`/`powerW`/`voltageV`).
## Deferred with rationale (documented decisions)
- **A6** — `buildTransactionBeginMeterValue` reads
`connectorStatus.transactionId` internally rather than accepting an
explicit param. Single call site, safe mutation ordering already
documented; adding a param would add noise for zero safety gain.
- **D-03** — `buildTransactionBeginMeterValue` dual-responsibility
(coherent short-circuit + legacy path) tracked in follow-up issue
#1936 as new M-06 item.
Consolidated fixes for all findings from the fourth multi-agent review round
(5 parallel reviewers: Oracle elegance/TS · Oracle math/physics · general
OCPP-spec-via-qmd · explore harmonization · general content/terminology).
Design phase cross-validated 5 architecturally-loaded decisions via Oracle.
## HIGH (6)
- **H1 [R4C]** — Thread `OCPP20ReadingContextEnumType.TRIGGER` into the
`TriggerMessage(MeterValues)` handler at `OCPP20IncomingRequestService.ts:611`.
Emitted `SampledValue.context` now correctly labels values taken in
response to `TriggerMessage` per OCPP 2.0.1 §3.66 ReadingContextEnumType;
previously defaulted to `Sample.Periodic`.
- **H2 [R4C]** — Presence-aware `resolveEnabledMeasurands` semantics in
`OCPPServiceUtils.ts`. Discriminates key-absent (defaults to
`{Energy.Active.Import.Register}` for ergonomic parity) from key-present
(honors the CSV verbatim, including empty ⇒ empty allow-list per
OCPP 2.0.1 J02.FR.11). Removes the unconditional force-add at line 1115.
- **H3 [R4C]** — Per-phase measurand emission in `buildCoherentMeterValue`.
Restructured to iterate all templates per measurand (was: first-matching
only) with phase-aware value resolution:
`Voltage L-N ⇒ V`; `L-L ⇒ √phases × V`.
`Power L-N ⇒ P/phases`; L-L unsupported (log-and-skip).
`Current line ⇒ sample.currentA` (balanced 3-φ Y).
`SoC / Energy.Active.Import.Register` phase-qualified templates rejected.
Emit order: `SoC → V → P → I → Energy` across measurands; within a
measurand: `no-phase → L1/L1-N → L2/L2-N → L3/L3-N → L1-L2 → L2-L3 →
L3-L1 → N → other`. Closes legacy-parity regression for 3-phase stations
with phase-qualified templates.
- **H4 [R4D]** — `throw new BaseError(...)` in `EvProfiles.ts`
`interpolateChargingCurve` (was: bare `Error`), aligning with AGENTS.md
TypeScript conventions on typed errors.
- **H5 [R4D]** — `assert.ok(cs != null, 'Expected connector 1 to exist')`
in `OCPP20ServiceUtils-PostTransactionDelay.test.ts` (was: `throw new
Error`), matching the pattern used elsewhere in the test suite.
- **H6 [R4E]** — README §EV profile file format now documents the
connector-level-only `MeterValues` template resolution scope under
coherent mode (EVSE-level inheritance not applied; tracked as follow-up).
## MED (12)
- **M1 [R4A]** — `computeCoherentSample` now takes the resolved
`session` as a parameter (was: fetched twice via `context.getCoherentSession`).
Removes 2 unreachable defensive branches (\~25 LOC) and tightens the
contract into a pure physics function.
- **M2 [R4C]** — OCPP 1.6 `buildTransactionBeginMeterValue` threads
vendor param `StartTxnSampledData` when configured, falling back to
`MeterValuesSampledData` when absent — per the OCPP 1.6 Signed Meter
Values whitepaper.
- **M3 [R4D]** — Move `MS_PER_HOUR` and `UNIT_DIVIDER_KILO` to
`Constants` class (`src/utils/Constants.ts`). Was duplicated as
file-scope constants in `OCPPServiceUtils.ts` and
`CoherentMeterValuesGenerator.ts`. All references now use
`Constants.MS_PER_HOUR` / `Constants.UNIT_DIVIDER_KILO`.
- **M4 [R4D]** — Move `VOLTAGE_NOISE_PERCENT` to
`Constants.DEFAULT_COHERENT_VOLTAGE_NOISE_PERCENT`. Tunables belong in
the canonical defaults map per AGENTS.md.
- **M5 [R4D]** — Move `DEFAULT_RAMP_UP_DURATION_MS` to
`Constants.DEFAULT_COHERENT_RAMP_UP_DURATION_MS`. Same rationale.
- **M6 [R4D]** — Merge same-specifier `import type` statements in
`types.ts` and `EvProfiles.ts` (was: 2 statements each for the same
module). Aligns with the project's `import/no-duplicates` /
`verbatimModuleSyntax` convention.
- **M8/M11 [R4D]** — `describe('Prng', ...)` and
`describe('StrategyDispatch', ...)` renames to match the
module-name-only convention in `tests/TEST_STYLE_GUIDE.md`.
- **M9 [R4E]** — README precision: "emitted measurand list" no longer
claims `ΔE` is emitted (it is an internal per-sample intermediate);
INV-1 spelled out per current-type (`P = V·I·phases` for AC, `P = V·I`
for DC).
- **M10 [R4E]** — Rewrite forward-looking comment at
`OCPP16ServiceUtils.ts:152-158` to describe the current-branch semantics
(`StartTxnSampledData` override with fallback to
`MeterValuesSampledData`) instead of the deferred S1 signing wiring.
- **R4A-LOW-02** — 3 near-identical `CoherentSample` zero-literal returns
in `computeCoherentSample` collapsed to a single `buildZeroSample`
helper. Two of the three defensive branches also removed by M1.
- **R4A-NIT-01** — `findTemplate` replaced by `groupTemplatesByMeasurand`
(needed for H3 per-phase iteration anyway); legacy `for..of` scan is now
gone.
- **R4B-LOW-01/02** — INV-1/INV-3 docstring precision: emit-time rounding
bound stated as "`ROUNDING_SCALE` half-width (\~0.005 W scalar)"; INV-3
divergence bound quantified (\~0.12 Wh over 24 h at 1 Hz).
- **R4B-LOW-05** — `EvProfileSchema` JSDoc documents monotone-non-increasing
`powerFraction` as a caller responsibility (not schema-enforced;
real curves are flat through CC before tapering).
- **R4B-NIT-06** — AC `numberOfPhases <= 0` now triggers the zero-sample
defensive branch (was: silently produced `P = 0` via
`divisor = V × 0 = 0`).
- **R4B-NIT-07** — `interpolateChargingCurve` JSDoc documents the
closed-closed interval semantic (left segment wins at interior nodes).
- **R4E-LOW-05** — `Prng.ts` header no longer claims a specific LOC
bound ("kept intentionally small").
- **R4E-LOW-06** — Expanded JSDoc on `ComputeSampleOptions` and
`CreateSessionOptions` interfaces (per-field semantics, units, defaults).
## Deferred to follow-up #1936 (documented rationale)
- **M7 [R4D]** — `StationHelpers.ts` (954 LOC) modular split. Structural
refactor with 30+ test file consumers; TODO comment added at the top of
the file linking to #1936. Detailed split sketch documented in the
design phase.
- **H6 code-side** — Extend `ICoherentContext` with `getEvseStatus` to
restore EVSE-level template inheritance. Round-4 lands docs-only.
- **R4B-LOW-03/04** — Physics model design notes (`cos φ = 1` assumption,
linear-vs-S-curve ramp shape). Not blockers.
## Non-findings (verified compliant)
- OCPP 2.0.1 `TxUpdatedInterval` correctly wired for periodic scheduling.
- `Transaction.Begin` / `Transaction.End` enum literals correct.
- Coherent path does not spoof `signedMeterValue`; signing gate intact.
- Coherent path does not bleed into `AlignedDataCtrlr` /
`ClockAlignedDataInterval` handling.
- Existing hyphenated test file names (`OCPP16-CoherentMeterValues.test.ts`
etc.) match the `ChargingStation-Configuration.test.ts` sub-domain
precedent — not a divergence.
- `TransactionEvent(Updated)` no longer serializes an empty `meterValue`
wrapper when `TxUpdatedMeasurands` resolves to an empty allow-list.
Periodic `startUpdatedMeterValues` short-circuits; TriggerMessage
(MeterValues) active-tx branch sends the event without the `meterValue`
field. Matches OCPP 2.0.1 J02.FR.11 ("no meter values are sent") and
fixes the JSON-schema `minItems: 1` violation on empty sampledValue.
- `OCPP16ResponseService` gates the extra `MeterValues.req` sends at
both the start (`beginEndMeterValues`) and end (`outOfOrderEndMeterValues`)
branches on `isNotEmptyArray(sampledValue)`. Composes cleanly with the
Signed Meter Values whitepaper §3.3.4 rule (`StartTxnSampledData`
present + non-empty) and the legacy `MeterValuesSampledData` fallback:
both empty ⇒ no send, avoiding a schema-suspect empty sampledValue
wrapper.
## Coherent path — per-phase physics polish
- **Per-phase Energy.Active.Import.Register**: L-N templates now emit
`register / phases` (balanced 3-φ Y contribution per phase); L-L and
`N` phases skipped with a warn. OCPP 2.0.1
`SampledDataCtrlr.RegisterValuesWithoutPhases` config-driven suppression
not consulted (documented follow-up).
- **Neutral phase**: new `Neutral` phase-family classifier distinguishes
bare `N` from `LineToNeutral` (`L1`/`L1-N`/etc.). `N`-qualified
Voltage and Current templates emit 0 (balanced 3-φ Y: I_N = 0 by
phasor sum, V_N = 0 by reference-node definition).
- **L-L voltage on 1-phase**: log-and-skip. Previously the `√1 × V`
degeneracy silently emitted nominal V under an L-L label.
- **L-N power on phases≤0**: log-and-skip (previously fell through to
aggregate power under a per-phase label — unreachable in practice due
to the outer defensive guard but semantically inconsistent).
- **Register clamp sync**: `preRegisterWh` in `computeCoherentSample`
applies `Math.max(0, ... ?? 0)` matching `advanceEnergyRegister`;
reported `sample.energyRegisterWh` and post-advance persisted state
now agree even under corrupted negative register state.
## Elegance + TS state-of-the-art
- Extract `resolveUnitDivider(measurand, unit)` +
`KILO_UNIT_BY_MEASURAND` at module scope, removing the inline
`(A && B) || (C && D)` boolean at the emit site.
- `PHASE_RANK` converted from `ReadonlyMap` to object literal with
`as const satisfies Record<MeterValuePhase, number>` for
compile-time exhaustiveness; `phaseRank`'s runtime fallback dropped.
- `resolvePhasedValue` returns exact fractions (no internal rounding);
rounding happens once at the emit site after unit-divider scaling.
Removes double-rounding asymmetry between aggregate and per-phase paths.
- `groupTemplatesByMeasurand` uses `Map.groupBy` (Node 22+) in place of
the hand-rolled loop; per-bucket phase sort preserved.
- Merged split `import type { ConnectorStatus }` with sibling type-import
from the same specifier.
## Documentation
- README §Phase-qualified measurands: "nominal V" → "sampled V"
everywhere (the coherent path emits `sample.voltageV`, i.e.
post-noise rounded voltage — not the station's nominal); documents
`N` phase behavior and per-phase Energy register emission.
- `resolvePhasedValue` JSDoc updated to match implementation.
- `ComputeSampleOptions` and `CreateSessionOptions` JSDoc converted
from bullet-list style to inline per-field JSDoc (matching
`CoherentSession` and `ICoherentContext` in `types.ts`).
- `types.ts` file header rewritten to state the current interface
contract instead of deferred tooling-contingency rationale.
- `Constants` docstrings for coherent tunables /
`MS_PER_HOUR` / `UNIT_DIVIDER_KILO` shortened to one-liners matching
the concision of sibling entries.
- `CoherentMeterValuesGenerator.ts` header carries a follow-up TODO
for the 250-LOC ceiling exceedance (split scope documented).
- Explanatory comment on the module-scope `warnedInvalidMeasurands`
WeakMap in `OCPPServiceUtils.ts` distinguishing its GC-keyed intent
from a true singleton.
- Internal-only comment on `VersionedSampledValueDispatch` interface.
## Tests
- New per-phase emission integration test: 3-phase AC session with
L-N/L-L voltage, aggregate + per-phase Power, L1/N Current, and L-N
Energy templates; asserts V_L1_N=230, V_L1_L2≈√3·230, Σ per-phase P
≈ aggregate P, I_N=0, L-L voltage skipped on 1-phase, L-N energy
register emitted as `register / phases`.
- `describe('OCPP 1.6 coherent MeterValues integration')` renamed to
`describe('OCPP16CoherentMeterValues')` (module-name convention).
- OCPP 1.6 integration test replaces its local `MS_PER_HOUR = 3_600_000`
literal with a `Constants.MS_PER_HOUR` alias (canonical source).
- Removed leaked internal-review annotations from 9 describe/it labels
(`(regression: ...)` suffixes) — these violated documentation
timeliness by embedding non-behavioral session metadata into the
behavioral test tree.
- Periodic `TransactionEvent(Updated)` no longer drops the whole message
when `TxUpdatedMeasurands` resolves empty. Sends the event with the
`meterValue` field omitted, matching the R5 fix already in place on the
`TriggerMessage(MeterValues)` active-tx branch. Preserves the periodic
heartbeat cadence at the OCPP level while honoring J02.FR.11
("no meter values are sent") — the message is still delivered, only
the sampled-value payload is elided.
## Coherent path — phase-degeneracy symmetry
- `Energy.Active.Import.Register` L-N templates on `phases <= 0` now
log-and-skip (return `undefined`), matching `Power.Active.Import`'s
behavior for the same misconfiguration. Previously the register branch
fell through to emit the aggregate under a per-phase template label.
## Elegance + TS state-of-the-art
- `resolvePhasedValue` signature narrowed from
`(measurand, template, sample, phases, connectorStatus)` to
`(measurand, phase, sample, phases, connectorStatus)`. The function
only reads `template.phase`; taking the whole template widened the
coupling. Caller passes `template.phase` at the loop head.
- `resolveUnitDivider` gains a JSDoc block (sole helper in the file that
previously lacked one) and reorders the guard to
`unit != null && KILO_UNIT_BY_MEASURAND.get(measurand) === unit` for
intent-first reading ("only kilo-divide when a unit is present").
- `LEGACY_EMIT_ORDER` renamed to `MEASURAND_EMIT_ORDER`. The constant
is the canonical emission order of the coherent path, not a
compatibility shim; the JSDoc note preserves the "mirrors the legacy
path" provenance.
- `buildZeroSample` now owns all rounding for the zero-sample path.
Callers pass raw `socPercent` / `voltageV`; the helper applies
`roundTo` internally. Rounding-responsibility is unified in one place.
## Documentation
- INV-1 docstring documents both the aggregate residual (≤ 0.005 W) and
the per-phase L-N residual (≤ 0.01 W = 2 × `ROUNDING_SCALE` half-width),
quantifying the two-step rounding bound (aggregate emit + per-phase
division).
- `resolvePhasedValue` JSDoc documents the per-phase Energy register
conservation bound: Σ across all L-N templates equals the aggregate
register within emit-unit rounding granularity (Wh: ≤ phases · 0.005 Wh;
kWh: ≤ phases · 5 Wh).
- `VersionedSampledValueDispatch.signingState` JSDoc: unresolvable
`{@link buildVersionedSampledValue}` replaced with plain prose
(the reference is a local closure, not an exported symbol).
- `resolveEnabledMeasurands` `@param measurandsKey` prose clarified:
`undefined` (or omitted) defaults to `StandardParametersKey.`
`MeterValuesSampledData` for OCPP 1.6; returns `undefined` (no filter)
for all other versions.
- `meter-values/index.ts` barrel comment simplified. Dropped the
enumerated intentionally-omitted list (drifts with internals); kept
the intent sentence only.
- `ChargingStation.createCoherentSession` JSDoc: dropped the
request-builder/response-handler call-site narrative; kept the API
contract ("idempotent; returns `undefined` when coherent mode is
disabled or no valid EV profile file is loaded").
## README
- `§Template resolution scope`: dropped the internal
`getSampledValueTemplate` helper name and the backlog-tracking sentence.
Documents current behavior only (connector-level templates, no
EVSE-level inheritance under coherent mode).
- `§Phase-qualified measurands`: dropped the "tracked as a follow-up"
tail on the `RegisterValuesWithoutPhases` note.
- Charging station template configuration table: `three phased` →
`three-phase`, `line to line voltage` → `line-to-line voltage`
(hyphenation consistent with the rest of the docs).
## Tests
- Added mandatory `afterEach(() => standardCleanup())` block to
`CoherentMeterValuesGenerator.test.ts`, `EvProfiles.test.ts`, and
`Prng.test.ts` per `tests/TEST_STYLE_GUIDE.md` §Mandatory Cleanup.
- `describe('OCPP20ServiceUtils — PostTransactionDelay')` renamed to
`describe('OCPP20ServiceUtilsPostTransactionDelay')` — module-name
concatenated form matching the existing coherent-test naming.
- `describe('B2 - OCPP 2.0.1 TransactionEvent Started → coherent session
wiring')` renamed to `describe('OCPP20ResponseServiceCoherentSession')`
— module-name-only form; the descriptive/spec-prefix suffix was
redundant with the inner `it` labels.
* refactor(meter-values): phase-family Record + shared test interval constant (issue #40)
## phaseFamily as an exhaustive Record
- Convert `phaseFamily`'s switch statement to a `PHASE_FAMILY` object
literal with `as const satisfies Record<MeterValuePhase, ...>`,
matching the `PHASE_RANK` pattern already established for phase
ranking. Adding a new `MeterValuePhase` enum value now fails compile
at the table declaration until the value is classified — the same
compile-time gate PHASE_RANK enforces.
- The `phaseFamily` function becomes a one-liner:
`phase == null ? 'Aggregate' : PHASE_FAMILY[phase]`.
`Aggregate` remains the sentinel for the undefined-phase case.
- Removes the defensive `default: return 'Aggregate'` branch that
silently absorbed unclassified enum values.
## phaseRank inlined
- Drop the `phaseRank` helper (used at exactly one call site) and
inline the null-check + `PHASE_RANK` lookup directly into the
`groupTemplatesByMeasurand` bucket-sort comparator. The
`PHASE_RANK` table is now the visible source of truth at the sort
site, matching the `PHASE_FAMILY` pattern above.
## Shared TEST_METER_VALUES_INTERVAL_MS constant
- Add `TEST_METER_VALUES_INTERVAL_MS = 30_000` to
`tests/charging-station/ChargingStationTestConstants.ts` as the
canonical test interval.
- Replace 27 inline `30_000` / `30000` literals across
`CoherentMeterValuesGenerator.test.ts` (20 sites),
`OCPP16-CoherentMeterValues.test.ts` (4 sites, including the local
`const INTERVAL_MS = 30_000` that was itself a duplicate), and
`StrategyDispatch.test.ts` (3 sites). Preserves value; consolidates
the semantic "meter-values sample interval used across coherent
tests" into one importable constant.
Removed 19 in-code review-round prefixes (`M1:`, `M2:`, `M3:`, `M4:`,
`B1`, `B2`, `Regression M4:`, `Regression B2:`) from test assertion
messages, JSDoc `@description`, and inline comments across 5 test files:
- `CoherentMeterValuesGenerator.test.ts` — 7 sites (voltage-noise,
capacity-clamp, INV-1 residual assertions).
- `OCPP16-CoherentMeterValues.test.ts` — 6 sites (stop-energy
reconstruction and coherent-session-leak assertions + comment).
- `OCPP20ServiceUtils-PostTransactionDelay.test.ts` — 1 site
(coherent-session-leak assertion).
- `OCPP20ResponseService-CoherentSession.test.ts` — 4 sites (`@description`
+ three eventType/idToken assertions).
- `OCPP16ResponseService-ForceTxOnInvalid.test.ts` — 1 comment block
("exercises the regression:" process language → behavioral "exercises
the pre-Start guard").
Assertion messages now describe the invariant being checked in plain
English without carrying review-round bookkeeping. This is a follow-up to
the earlier cleanup that stripped `(regression: XX)` suffixes from
`describe`/`it` labels but missed codes inside assertion messages and
`@description` blocks.
## JSDoc / doc hygiene
- `PHASE_FAMILY` (`CoherentMeterValuesGenerator.ts`) — deleted the stale
first JSDoc block that documented the pre-refactor `phaseFamily`
function; kept the block that documents the current Record with
compile-time exhaustiveness gate.
- `VersionedSampledValueDispatch` (`OCPPServiceUtils.ts`) — converted
the `//` block comment above the interface to a proper `/** */` JSDoc
block matching the sibling-interface convention. Body documents the
internal-only dispatch-bag role with `@link` cross-references.
- `warnedInvalidMeasurands` and `KNOWN_MEASURANDS` (`OCPPServiceUtils.ts`)
— moved above the `resolveEnabledMeasurands` JSDoc block so the JSDoc
is now adjacent to the function it documents (was previously separated
by the two `const` declarations, breaking the visual JSDoc→function
association).
- `template.unit as MeterValueUnit | undefined` cast (`CoherentMeter`
`ValuesGenerator.ts`) — added an intent comment documenting the OCPP
2.0 open-string-branch narrowing at the Map-lookup boundary and the
fallback semantics (non-enum strings return `undefined` from the Map
and fall through to `divider = 1`).
- `MEASURAND_EMIT_ORDER` (`CoherentMeterValuesGenerator.ts`) — dropped
the explicit `readonly MeterValueMeasurand[]` annotation; kept `as const`
for a narrow tuple type matching the sibling `PHASE_FAMILY`/`PHASE_RANK`
pattern (immutability via const-assertion, coherent style across the
three module-scope constants).
- `ChargingStation.__injectCoherentSession` JSDoc — dropped the
"not currently enforced by a lint rule" sentence (build-tool trivia,
not API behavior).
## Test constants
- `TEST_METER_VALUES_INTERVAL_MS` rationale documented on the
`Timer Intervals` block header (`_MS` intervals are intentionally
half of `Constants.DEFAULT_*_INTERVAL_MS` for bounded test wall-clock).
- `ChargingStationTestConstants.ts` file header rewritten from the
narrative-prose + `@see` form to standard `@file` / `@description`
JSDoc matching the rest of the test suite.
- OCPP20ServiceUtils.ts / OCPP20IncomingRequestService.ts: replace loose
J02.FR.11 shorthand on the meterValue-field-omission branches with the
actual grounding — MeterValueType.sampledValue cardinality 1..* combined
with TransactionEventRequest.meterValue cardinality 0..* — so a reader
sees why the empty case yields {} rather than { meterValue: [] }.
- CoherentMeterValuesGenerator.test.ts: capacity-clamp failure messages
now report 2 × ROUNDING_SCALE (0.01 W) to match the assertion tolerance
(the two rounding steps: aggregate P = V·I·phases plus post-clamp
re-round each contribute one half-width).
* refactor(meter-values): use session snapshot for voltage, currentType, numberOfPhases (issue #40)
computeCoherentSample now reads voltageOutNominal, currentType, and
numberOfPhases from the session object (immutable snapshot captured at
createCoherentSession) instead of the live context. The three fields
were already declared readonly on CoherentSession but only two were
consulted at runtime, leaving voltageOutNominal dead. Using the snapshot
makes the physics chain deterministic against hypothetical mid-session
station-config drift and reduces per-sample context calls from three to
zero (getConnectorMaximumAvailablePower stays live — the EVSE cap is a
genuinely dynamic quantity that can change during a transaction).
* fix(meter-values): namespace transactionId in createStreamPrng to prevent XOR self-inverse (issue #40)
createStreamPrng chained two XOR-based deriveSeed calls, so
String(transactionId) === label collapsed the derived stream seed back
to the root seed (deriveSeed(deriveSeed(r, X), X) === r). Trigger
required transactionId to match a literal stream label
('VOLTAGE_NOISE', 'POWER_NOISE', 'PROFILE_PICK', 'INITIAL_SOC') —
extremely unlikely with OCPP-numeric transactionIds but reachable via
test seams. Namespace the transactionId leg with a 'tx:' prefix that
labels never carry so the two hash inputs cannot algebraically cancel.
Prng.ts deriveSeed docstring updated to reference the namespacing in
createStreamPrng and drop the 'Known limitation (deferred)' language.
* chore(charging-station): guard __injectCoherentSession against production use (issue #40)
The public __injectCoherentSession method is a test seam whose only
prior protection was the '__' naming convention. Add a runtime guard
that throws a typed BaseError when NODE_ENV === 'production', mirroring
the pattern used by interpolateChargingCurve in EvProfiles.ts for the
unsorted-curve defensive check. Tests run with NODE_ENV unset or 'test'
so the guard is transparent to them; accidental production use now
fails loudly instead of silently corrupting the session store.
* fix(meter-values): tighten defensive guard against non-finite intervalMs (issue #40)
The computeCoherentSample defensive guard applied Number.isFinite to
batteryCapacityWh, nominalV, and nowMs but only tested intervalMs <= 0.
Since NaN <= 0 and +Infinity <= 0 both evaluate to false in JS, either
pathological value escaped the guard, propagated through
maxPowerFromCapacityW = remainingWh * MS_PER_HOUR / intervalMs, and
permanently poisoned session.socPercent to NaN (NaN + x === NaN for
every subsequent sample). Reachability was low in practice — legitimate
callers pass integer intervals and convertToInt throws on NaN — but a
non-compliant CSMS setting MeterValueSampleInterval to a pathological
value via ChangeConfiguration could reach the coherent path.
Add !Number.isFinite(options.intervalMs) to the OR chain, mirroring the
sibling checks. Docstring bullet rewritten to reflect the tightened
coverage. Two new test cases lock the behavior: intervalMs=NaN and
intervalMs=+Infinity both short-circuit to a zero sample with session
state intact, matching the existing intervalMs=0 semantics. The
describe block is renamed from 'intervalMs=0 defensive guard' to
'intervalMs defensive guard' to reflect the broader coverage.
* chore(helpers): drop caller-coordination note on resetConnectorStatus (issue #40)
The 'NOTE: transactionId is deleted here. Callers that need to identify
the just-stopped transaction MUST snapshot ... BEFORE invoking this
function.' comment described a caller-side coordination concern rather
than the WHY of the delete. Every caller that needs the transactionId
already snapshots it before the reset; the note added no local WHY and
duplicated a responsibility that belongs in the caller, not in
resetConnectorStatus.
* docs(meter-values): drop 'legacy' framing for the random/fixed simulation mode (issue #40)
The random/fixed measurand generation is not deprecated — it is one of
two peer simulation modes exposed by the simulator, selected via the
coherentMeterValues station flag. The word 'legacy' throughout the
newly-introduced comments, JSDoc, README, and test descriptions
inaccurately implied a deprecation path.
Neutralize 13 sites across 6 files: 'legacy' either dropped where it
was purely decorative (e.g. 'mirroring the legacy X path' → 'mirroring
the X path') or replaced with 'random/fixed' / 'default' where the
descriptor was substantive. No code changes, no test-logic changes,
purely prose.
F12-A retitled the enclosing it() from 'via the legacy path' to 'via
the random/fixed path' but the inline WHY comment two lines below still
said 'Legacy path' — same author, same commit, same test block.
Missed by the case-sensitive verification grep used at F12-A landing;
this closes the neutralization loop.
Jérôme Benoit [Mon, 29 Jun 2026 22:34:53 +0000 (00:34 +0200)]
refactor: project-wide audit of helper/util usage in src/ + OCPP 2.0.1 §F01.FR.13 spec fix (#1931)
* refactor(utils): replace bare structuredClone with clone helper
Adopts the existing `clone` helper from `src/utils/Utils.ts` at 7 sites in 4
files where `structuredClone` was called directly. `clone` is a thin wrapper
around `structuredClone` (`return structuredClone(object)`); the swap is
behaviour-preserving DRY consolidation.
The `as Record<string, unknown>` casts at ConfigurationValidation.ts:103 and
TemplateValidation.ts:62 are preserved on the call sites.
Signed-off-by: Jérôme Benoit <jerome.benoit@sap.com>
* refactor(ocpp): replace (error as Error).message with getErrorMessage
The `(error as Error).message` cast is unsound — when the thrown value is
not an `Error` instance, `.message` is `undefined` and the log silently
emits 'undefined', obscuring potentially security-relevant failures (notably
on crypto / signing paths). The `getErrorMessage` helper from
`src/utils/ErrorUtils.ts` handles both `Error` instances and non-`Error`
thrown values via `ensureError`, producing a meaningful string in every
case.
The inline `getConfigurationKey(...)?.value === 'true'` pattern only matches
the literal lowercase string `'true'` and silently treats `'True'`, `'1'`,
`true` (boolean) and any non-string value as false. `convertToBoolean` from
`src/utils/Utils.ts` handles the full set of truthy representations
consistently across the codebase.
Sites converted (7):
- src/charging-station/ocpp/OCPPServiceUtils.ts (3 sites in OCPP 2.0 signed
meter value generation: SignReadings, SignStartedReadings,
SignUpdatedReadings flags)
- src/charging-station/ocpp/1.6/OCPP16ServiceUtils.ts (3 sites: isSigningEnabled,
buildStartedMeterValues and buildUpdatedMeterValues signing guards)
- src/charging-station/Bootstrap.ts (1 site: ENV_SIMULATOR_COLD_START env var)
Consolidates inline emptiness checks on typed strings, arrays and Sets to
use the existing `isEmpty` and `isNotEmptyString` helpers from
`src/utils/Utils.ts`. The helpers cover string, array, Set, Map and plain
object emptiness uniformly; `isNotEmptyString` also trims internally so
`workerScript.trim().length === 0` becomes `!isNotEmptyString(workerScript)`
without behavior change.
isEmpty sites (7):
- src/charging-station/ui-server/UIServerAccessPolicy.ts:230,268,297,407
(4× .length === 0 on string[] from splitHeaderList / getAllowedHosts)
- src/charging-station/ui-server/UIServerAccessPolicy.ts:508
(.size === 0 on ReadonlySet<string> trustedProxies)
- src/charging-station/ui-server/AbstractUIServer.ts:1317,1326
(2× .length === 0 on accessPolicy string[] in UI server constructor warn
paths)
Imports updated: UIServerAccessPolicy.ts adds isEmpty; OCPPSignedMeterValueUtils.ts
extends to isNotEmptyString; WorkerAbstract.ts adds a fresh utils import.
AbstractUIServer.ts already had both helpers imported.
Signed-off-by: Jérôme Benoit <jerome.benoit@sap.com>
* refactor: adopt convertToInt and has helpers for type conversions and property checks
Consolidates inline `Number.parseInt(_, 10)` / `Number(_)` coercions on string
inputs to use `convertToInt`, and `Object.hasOwn` property checks to use
`has` from `src/utils/Utils.ts`. `has` accepts `(property, object)` argument
order (unlike `Object.hasOwn(object, property)`) and adds a null-guard on the
object.
convertToInt sites (3):
- src/charging-station/ui-server/UIServerNet.ts:97
(`Number.parseInt(port, 10)` on validated digit-string port)
- src/charging-station/ocpp/1.6/OCPP16IncomingRequestService.ts:785
(`Number(value)` on OCPP config integer-key value)
- src/charging-station/TemplateSchema.ts:268
(`Number(evseKey)` on Object.entries(template.Evses) record key)
has sites (2):
- src/utils/ConfigurationSchema.ts:210
(`!Object.hasOwn(value as object, 'accessPolicy')` in Zod refine —
the `as object` cast is no longer needed since `has` accepts `unknown`)
- src/charging-station/ui-server/UIServerAccessPolicy.ts:355
(`Object.hasOwn(params, key)` in Forwarded header parameter dedup)
Signed-off-by: Jérôme Benoit <jerome.benoit@sap.com>
* refactor: adopt isNotEmptyArray and buildConfigKey for collection checks and log formatting
isNotEmptyArray sites (4):
- src/charging-station/ui-server/AbstractUIServer.ts:1438 (countConnectors
fallback on data.connectors)
- src/charging-station/ui-server/AbstractUIServer.ts:1451 (iterateConnectors
guard on data.connectors)
- src/charging-station/ui-server/UIServerAccessPolicy.ts:438 (allowedOrigins
presence check)
- src/charging-station/ui-server/UIServerAccessPolicy.ts:443 (allowedHosts
presence check in fallback)
buildConfigKey site (1):
- src/charging-station/ocpp/2.0/OCPP20ServiceUtils.ts:518
(readVariableAsInteger warn log: unify the displayed key format with the
canonical `buildConfigKey(component, variable)` output instead of inline
`${component}.${variable}` interpolation)
Imports updated: AbstractUIServer.ts extends to isNotEmptyArray;
UIServerAccessPolicy.ts extends to isNotEmptyArray.
Signed-off-by: Jérôme Benoit <jerome.benoit@sap.com>
* fix(refactor): avoid TDZ cycles via utils barrel for new helper imports
The previous commits introduced fresh imports from `src/utils/index.js` in
three modules that participate in barrel-induced TDZ cycles via
`ConfigurationSchema.ts`:
- src/charging-station/ui-server/UIServerNet.ts is imported by
ConfigurationSchema.ts, so importing back through utils/index.js produces
`ReferenceError: Cannot access 'isHostLiteralWithoutPort' before
initialization`.
- src/worker/WorkerAbstract.ts is exported via worker/index.js, which is
consumed by ConfigurationSchema.ts (`WorkerProcessType`), so importing
back through utils/index.js breaks Zod enum initialization with
`TypeError: Cannot convert undefined or null to object` in
`getEnumValues`.
- src/charging-station/TemplateSchema.ts uses the same cycle-prone barrel
defensively, preempting future Zod-init breakage.
Resolution: import `convertToInt` and `isNotEmptyString` directly from
`./Utils.js` (the leaf module), mirroring the repo-established
cycle-avoidance pattern at `src/utils/ConfigurationMigrations.ts:3-4`
(`import { BaseError } from '../exception/BaseError.js'` with the same kind
of inline note).
Tests: previously failing
- tests/charging-station/ui-server/UIServerNet.test.ts
- tests/worker/WorkerUtils.test.ts
both pass; full suite 2857/2857.
Reverts the convertToInt swap at OCPP16IncomingRequestService.ts:785 (M4 in
the helpers audit). The downstream check
`!Number.isInteger(numValue) || numValue < 0 → REJECTED`
intentionally relies on `Number(value)` returning `NaN` for invalid input
to fail `Number.isInteger`. `convertToInt` breaks this contract in four
distinct ways verified empirically:
value Number(value) convertToInt(value) effect on rejection check
-------- ------------- ------------------- ----------------------
undefined NaN 0 silently ACCEPTED
'' 0 throws uncaught exception
'1.5' 1.5 1 silently ACCEPTED
'abc' NaN throws uncaught exception
Two of these (`''` and `'abc'`) produce uncaught exceptions that propagate
out of the handler instead of returning Rejected. The other two silently
accept invalid values. The audit recommendation to adopt `convertToInt`
here was incorrect; the call site needs the NaN-on-invalid contract that
only `Number()` provides.
Issue flagged by hyperspace-insights review bot on PR #1931 (only the
`undefined → 0` case was highlighted; the full divergence is broader).
Inline note added to prevent re-introduction by future cleanups.
Tests: tests/charging-station/ocpp/1.6/OCPP16IncomingRequestService-Configuration.test.ts
and tests/charging-station/ocpp/1.6/OCPP16Integration-Configuration.test.ts —
14/14 pass.
Applies four review nits from the post-audit self-review, plus reverts an
architectural misuse:
F1 — OCPPServiceUtils.ts (OCPP 2.0 signed-meter-value block, 3 sites):
extracts a private `isOCPP20FlagEnabled(chargingStation, component, variable)`
helper consuming the existing `convertToBoolean(getConfigurationKey(_,
buildConfigKey(_, _))?.value)` chain. Reduces 5-level indented blocks at
lines 938, 950, 963 to a single helper call, ~12 lines saved.
F2 — OCPP16ServiceUtils.ts (2 sites): adds `isSigningStartedReadingsEnabled`
and `isSigningUpdatedReadingsEnabled` static methods next to the existing
`isSigningEnabled`, all three following the same shape. Replaces the inline
`convertToBoolean(getConfigurationKey(_, OCPP16VendorParametersKey.X)?.value)`
chains at lines 173 and 826 with named-intent calls. Semantic naming
> generic helper since these are vendor-key-specific.
F3 — UIServerNet.ts, WorkerAbstract.ts, TemplateSchema.ts: compresses the
multi-line TDZ-cycle workaround comments to single-line format, matching
the established repo convention at `ConfigurationMigrations.ts:3`
(`// Direct path: the exception/index.js barrel re-exports OCPPError,
causing a TDZ cycle.`).
F4 — OCPP16IncomingRequestService.ts:786: shortens the 5-line `// NB:`
comment about `Number(value)` preservation to a 1-line concise note while
preserving the regression-prevention intent.
Architectural revert — WorkerAbstract.ts: removes the `isNotEmptyString`
import from `../utils/Utils.js` added in 7fe3e2a10 (M3). `src/worker/` is
a standalone sub-project and must not import from elsewhere in the source
tree. Reverted the call site to inline `workerScript.trim().length === 0`.
Same architectural rule that justifies deferring the worker-side
`mergeDeepRight`/`sleep`/`secureRandom` consolidations.
Tests: 37/37 pass on the affected suites (UIServerNet, WorkerUtils,
WorkerAbstract, OCPP16 Configuration, OCPP16 Configuration integration).
Brings OCPP 2.0 incoming-request handlers to parity with the OCPP 1.6
equivalent (OCPP16IncomingRequestService.ts) by routing all catch-and-return
patterns through the shared `handleIncomingRequestError<T>` helper from
`src/utils/ErrorUtils.ts`.
4 conditional sites (G — nested + outer catches in handleRequestStartTransaction,
lines 2567/2608/2673/2737): each catch builds a typed
OCPP20RequestStartTransactionResponse local const (status: Rejected with the
appropriate additionalInfo, transactionId: generateUUID()) and passes it both
as errorResponse and as the `??` typing-narrowing fallback. The
`await this.resetConnectorOnStartTransactionError(...)` side-effect on the
outer catch is preserved before the helper call.
Behavioral note: the log message format changes from
`OCPP20IncomingRequestService.handleRequestX: ...` to
`ErrorUtils.handleIncomingRequestError: Incoming request command 'X' error: ...`
— matches the OCPP 1.6 service. The error itself is still logged (via the
helper's internal logger.error call); no error is swallowed.
Tests: 336/336 pass on `tests/charging-station/ocpp/2.0/OCPP20IncomingRequestService-*.test.ts`.
Signed-off-by: Jérôme Benoit <jerome.benoit@sap.com>
* refactor(validation): adopt assertIsJsonObject for JSON-object type guards
Replaces the inline guard 'if (parsed == null || typeof parsed !== object ||
Array.isArray(parsed)) throw new BaseError(...)' with the canonical
'assertIsJsonObject(parsed, new BaseError(...))' helper from
'src/utils/Utils.ts'. The helper:
- Narrows the value type to JsonObject via 'asserts value is JsonObject'
(type predicate gain).
- Accepts a custom Error second argument that is thrown verbatim, preserving
the existing BaseError message and stack trace.
Replaces 14 bare 'JSON.stringify(_, undefined/null, 2)' call sites with the
shared 'JSONStringify' helper from 'src/utils/Utils.ts'. The helper:
- Serializes Maps and Sets safely. Bare 'JSON.stringify' silently drops Map
entries (correctness fix for sites with potential Map values).
- Accepts an optional 'MapStringifyFormat' parameter to control how Maps are
rendered (default: array of pairs; '.object': key-value object).
Sites with Map values (use 'MapStringifyFormat.object' to match the existing
JSON config-schema representation of station data):
- src/charging-station/ChargingStation.ts:2595 (config file write,
ChargingStationConfiguration)
- src/charging-station/ui-server/mcp/MCPResources.ts:22 (listChargingStationData)
- src/charging-station/ui-server/mcp/MCPResources.ts:44 (station data by id)
Also loosens the 'JSONStringify' generic constraint to 'object: unknown'
(matching 'JSON.stringify' standard signature). The previous restrictive
generic (already marked 'no-unnecessary-type-parameters') prevented passing
values like 'ChargingStationConfiguration' that lack TS index signatures
but are JSON-serializable at runtime. Removes unused 'JsonType' import.
Skipped (architectural):
- src/worker/WorkerSet.ts:191 — 'src/worker/' is a standalone sub-project
and must not import from elsewhere (same rule as the WorkerAbstract revert
in 2f190a4cf).
Tests: 512/512 pass on Utils + ChargingStation + Bootstrap + OCPP + UI server suites.
Signed-off-by: Jérôme Benoit <jerome.benoit@sap.com>
* refactor(bootstrap): adopt promiseWithTimeout in waitChargingStationsStopped
Replaces the manual 'new Promise((resolve, reject) => { setTimeout(...);
waitChargingStationEvents(...).then(resolve).catch(reject) })' construct
with the shared 'promiseWithTimeout' helper from 'src/utils/Utils.ts'.
The refactor is behaviour-preserving:
- The timeout BaseError is now constructed once upfront and passed to
'promiseWithTimeout' (matching the helper's pre-built error contract).
- The 'logger.warn' is emitted only when the caught error is referentially
the timeoutError (i.e., only on actual timeout, not on errors propagated
from 'waitChargingStationEvents'). Same semantic as the original which
only logged inside the setTimeout callback.
- 'clearTimeout' on success path is handled internally by 'promiseWithTimeout'
via 'promise.finally(() => clearTimeout(timer))'.
Linear try/await/catch flow replaces the nested Promise constructor +
.then/.catch/.finally chain (~5 LOC saved, less indirection).
Imports updated: Bootstrap.ts adds 'promiseWithTimeout' alphabetically to
the existing utils barrel import.
Tests: 11/11 pass on 'tests/charging-station/Bootstrap.test.ts'.
F1 — OCPP20IncomingRequestService.ts (2 sites): converts the
constant-inlined-twice pattern at handleRequestClearCache:815 and
handleRequestSendLocalList:961 to the local-const pattern used by the other
11 sites in the same file. The error-response constant is now bound to a
typed local const and referenced once for both the helper's 'errorResponse'
parameter and the '?? errorResponse' TS-narrowing fallback, eliminating the
double reference to the same constant.
F3 — Bootstrap.ts waitChargingStationsStopped: adds a 3-line comment above
the 'if (error === timeoutError)' identity check documenting the contract
relied upon — 'promiseWithTimeout' must reject with the same Error instance
passed in. If a future maintainer wraps the error inside the helper, the
identity check would silently break and the timeout-specific 'logger.warn'
would no longer fire. The comment also clarifies that downstream errors
from 'waitChargingStationEvents' intentionally propagate unlogged
(preserves the original behaviour).
No behavioural change. Tests: 347/347 pass on OCPP20IncomingRequest +
Bootstrap suites.
Applies 9 fixes from the 4-agent cross-validation review (2 oracles + 1
librarian + 1 explore agent on the same scope).
HIGH-CONFIDENCE fixes (>=2 agents converged):
CV1 — type-safety: 'isOCPP20FlagEnabled' parameter tightened from
'variable: string' to 'OCPP20OptionalVariableName | OCPP20RequiredVariableName
| OCPP20VendorVariableName'. The merged-runtime 'StandardParametersKey' /
'VendorParametersKey' objects only had OCPP 1.6 typedefs, masking the OCPP
2.0 call-site values; the explicit union closes the gap. (OCPPServiceUtils.ts)
CV2 — semantic preservation: 'convertToInt(evseKey)' in the Zod
'.superRefine' callback would throw on non-numeric keys, where the original
'Number(evseKey)' returned NaN and silently fell through. Reverted to
'convertToIntOrNaN(evseKey)' to preserve the NaN-on-invalid contract the
constraint check downstream relies on. Same class of bug the bot
identified at OCPP16IncomingRequestService:785. (TemplateSchema.ts)
CV3 — discriminator robustness: replaced 'error === timeoutError' identity
check with an 'instanceof TimeoutError' check against a 'BaseError'
subclass. The subclass discriminator survives any future wrapping of the
error inside 'promiseWithTimeout'; the identity check silently broke on
any error-cloning refactor of the helper. (Bootstrap.ts)
CV4 — cross-version parity: aligned the 4 remaining 'handleIncomingRequestError'
sites in OCPP 1.6 (cancelReservation, dataTransfer, getDiagnostics, reserveNow)
to the local-const pattern that OCPP 2.0 adopted in the audit, eliminating
the duplicated constant reference at each call site. (OCPP16IncomingRequestService.ts)
OBSERVABILITY fix:
S1 — reverted the 4 nested + outer catches in OCPP 2.0
'handleRequestStartTransaction' (Authorization / Group authorization /
Charging profile validation / outer 'Error starting transaction') from
'handleIncomingRequestError' adoption back to inline 'catch → logger.error →
return typed-rejection'. The helper's unified log message dropped the
phase-specific context (truncated idToken values, distinct phase names).
OCPP 1.6 'handleRequestRemoteStartTransaction' itself does NOT use
'handleIncomingRequestError', so this revert restores both observability
AND cross-version parity. The other 10 single-catch sites adopted by the
audit remain unchanged. (OCPP20IncomingRequestService.ts)
Doc/style nits:
S4 — README.md: 'SIMULATOR_COLD_START=true' documented as 'a truthy value
(true, True, TRUE, 1, optionally surrounded by whitespace)' to reflect
the actual 'convertToBoolean' semantics adopted in the audit.
S5 — Bootstrap.ts:649: added 'MapStringifyFormat.object' to the
'JSONStringify(data, 2)' call on worker-message error data. Consistent
with the format choice at 'ChargingStation.ts:2597' and 'MCPResources.ts'
since worker message data can carry Maps.
S6 — TemplateSchema.ts:3: TDZ-cycle direct-path comment realigned to the
declarative form used by 'ConfigurationMigrations.ts:3' ('the X barrel
re-exports Y, causing a TDZ cycle.').
S7 — OCPP16ServiceUtils.ts:683,694: JSDoc terminology aligned with the
spec ('signing of meter values at transaction start' / 'during transaction
updates' instead of 'transaction-started readings' / 'transaction-updated
readings').
Applies 10 fixes from the v4 4-agent cross-validation review (2 oracles + 1
librarian + 1 explore) on the post-v3 state of PR #1931.
CORRECTNESS:
BUG — Configuration.ts:293: 'convertToInt(env.PORT ?? "")' would throw an
uncaught exception when PORT is unset (?? '' returns empty string, on which
convertToInt throws). Same class of bug as the CV2 fix in TemplateSchema.
Guards with isNotEmptyString(env.PORT) and only converts when set.
DESIGN:
HC1 + Q2(d) — TimeoutError relocated from Bootstrap.ts to a dedicated
src/exception/TimeoutError.ts file, exported from src/exception/index.js.
Bootstrap.ts now imports TimeoutError alongside BaseError. Matches the
established repo pattern (BaseError, OCPPError each in their own file
under src/exception/). The class declaration is no longer interleaved
with imports in Bootstrap.ts.
S1 — extract rejectedInternalError(additionalInfo) helper in
OCPP20IncomingRequestService.ts. Collapses 4 near-identical 8-line
'{ status: Rejected, statusInfo: { additionalInfo, reasonCode:
InternalError }, transactionId: generateUUID() }' literals at the 4
nested + outer catches of handleRequestStartTransaction. Each catch
becomes a 1-line 'return rejectedInternalError("...")'.
S2 — wrap 'logger.error(..., error)' with 'ensureError(error)' at the
4 reverted G-sites. Aligns with the 10 sibling sites in the same file
that already route through ensureError. Non-Error throwables are
normalized before reaching the logger.
Q3(a) — TemplateSchema.ts: add 'Number.isNaN(evseId)' guard in the
.superRefine loop over Object.entries(template.Evses). Closes the
validation gap where 'Evses: { "abc": {...} }' silently passed
because NaN === 0 / NaN > 0 are both false. The guard pushes a
'ctx.addIssue' for non-numeric keys with a clear message citing
OCPP 2.0.1 §7.2.
POLISH:
S3 — Bootstrap.ts: 'logger.warn(..., error.message)' instead of the
closed-over 'timeoutError.message'. After the instanceof TimeoutError
narrowing, 'error' is correctly typed as TimeoutError. If a future
wrap of promiseWithTimeout passes through a different instance,
error.message still reflects what was actually caught.
S4 — Bootstrap.ts: 'behaviour' (British) → 'behavior' (American)
to match the 30+ occurrences elsewhere in src/. Also resolves the
cspell flag.
S5 — Bootstrap.ts: catch comment compressed from 3 lines to 1 line.
Drops the partial redundancy with the TimeoutError JSDoc and removes
the 'unlogged' cspell flag.
HC2 — README.md: SIMULATOR_COLD_START wording tightened from a
non-exhaustive list ('true, True, TRUE, 1') to the exhaustive rule
'case-insensitive true or 1, optionally surrounded by whitespace'.
Matches the actual convertToBoolean semantics (trim + toLowerCase).
HC-A TemplateSchema.ts: replace NaN-via-arithmetic guard with explicit
Number.isInteger check for evseId validation (clearer intent, same semantics).
HC-B + HC-C TimeoutError.ts: compress JSDoc to single line; add SPDX
copyright header to match peer OCPPError.ts.
L-A + HC-D OCPP20IncomingRequestService.ts: rename rejectedInternalError
helper to buildRejectedResponse(additionalInfo, reasonCode) and migrate
all 11 RequestStartTransaction rejection sites (4 helper calls + 7 inline
literals) through it. Drop fabricated transactionId: generateUUID() from
rejection responses per OCPP 2.0.1 §E07: that field signals a transaction
already started by the Charging Station before the request arrived
(cable-first scenarios), so fabricating a UUID on a pure rejection
misleads CSMS implementations that map remoteStartId -> transactionId.
V5-6 Bootstrap.ts: relocate catch-block ordering comment below instanceof
check and reword for clarity.
Test: update §E07 regression assertion in
OCPP20IncomingRequestService-RequestStartTransaction.test.ts to expect
response.transactionId === undefined on TxInProgress rejection (was
incorrectly asserting notStrictEqual against the now-fixed bug).
All quality gates pass: 2857/2857 tests, typecheck, lint, format.
The 'transactionId-on-rejection' fix in commit 23beb7ea1 cited OCPP 2.0.1
§E07, which is wrong: §E07 is 'Transaction locally stopped by IdToken'.
The actual clause governing transactionId echo on RequestStartTransaction-
Response is §F01.FR.13: 'When a transaction is created on the Charging
Station, but has not been authorized. AND RequestStartTransactionRequest
is received → The Charging Station SHALL return the transactionId in the
RequestStartTransactionResponse.'
This is the cable-plugin-first scenario. On pure rejections F01.FR.13 does
not apply, so transactionId is correctly omitted. The semantics of the fix
are unchanged; only the spec citation in the comments is corrected.
Verified against OCPP-2-0-1-edition3-part2-specification.md via local
spec collection.
V6-S2 + V6-Q1 (HIGH, both Oracle agents): RequestStartTransaction
TxInProgress rejection regressed OCPP 2.0.1 part 2 §F01.FR.13 when the
connector held a transactionPending state. The v5 fix dropped the
fabricated UUID (correct) but also dropped the real connectorStatus.
transactionId (incorrect). Per §F01.FR.13: 'When a transaction is created
on the Charging Station, but has not been authorized AND
RequestStartTransactionRequest is received, the Charging Station SHALL
return the transactionId in the RequestStartTransactionResponse.'
Split the TxInProgress check into two branches:
- transactionPending === true AND typeof connectorStatus.transactionId
=== 'string' → buildRejectedResponse(TxStarted, ..., transactionId).
The appendix distinguishes TxStarted (cable-plugin-first, not yet
authorized) from TxInProgress (authorized and running) — TxStarted
matches the §F01.FR.13 precondition.
- Fallthrough (transactionStarted || transactionPending-without-tx-id ||
locked) → buildRejectedResponse(TxInProgress, ...) without echo.
Extended buildRejectedResponse signature: added optional transactionId?:
UUIDv4 parameter and swapped to (reasonCode, additionalInfo,
transactionId?) — required-first/optional-last per V6-X3+Q11. All 11
existing callsites updated; the new branch is the 12th call.
V6-S1 (M, Oracle1, qmd-verified): TemplateSchema.ts cited OCPP 2.0.1
§7.2 (Connector numbering) for EVSE-key validation. The correct clause
is §7.1 (EVSE numbering) — qmd against
OCPP-2.0.1_edition3_part1_architecture_topology.md:269 confirms §7.1
covers 'EVSEs MUST be sequentially numbered starting from 1' and 'evseId
0 is reserved'. Two citation occurrences corrected.
V6-S4 (L, Oracle1): F01.FR.11 violation (ChargingProfile.transactionId
set in RequestStartTransaction) used reasonCode InvalidValue. F01.FR.26
hints InvalidProfile/InvalidSchedule for invalid ChargingProfile.
Changed to InvalidProfile for family coherence with the other two
ChargingProfile rejection sites.
V6-X1 (L, Librarian): Helper comment overstated the spec — said
'fabricating a UUID misleads CSMS' as if §F01.FR.13 mandated MUST NOT.
Reworded to distinguish the positive obligation (SHALL echo when
precondition holds) from the practical argument (fabrication misleads).
Documents the new optional transactionId? parameter semantics.
V6-X14 (M, Librarian): TimeoutError extends BaseError {} was an empty
subclass — TypeScript structural typing made BaseError and TimeoutError
indistinguishable in the type system, so the single instanceof check in
Bootstrap.ts relied solely on the runtime prototype chain. Added
'public override readonly name = TimeoutError as const' for nominal
distinction (matches the OCPPError pattern in the same codebase).
V6-Q2 (M, Oracle2): Test coverage gap — only 1 of the 11 v5 rejection
paths asserted transactionId === undefined. Added the assertion to the
6 remaining unprotected Rejected tests (InvalidIdToken, MasterPassGroup,
invalid ChargingProfile purpose, ChargingProfile-with-transactionId,
authorization throw, all-EVSEs-inoperative). The V6-S2 fix test now
asserts transactionId === pendingTransactionId AND reasonCode ===
TxStarted (regression-locks the §F01.FR.13 echo path).
Deferred to follow-up: V6-Q3 (mixed catch pattern — intentional per
v3 S1), V6-E11/E12/E15 (ensureError missing at 18 logger.error sites
in OCPP 2.0 .catch callbacks + 2 OCPP 1.6 catches), V6-E19 (extract
helper for RequestStopTransaction rejection literals — needs different
return type), V6-X3 cosmetic polish items.
Verified via qmd ocpp-specs collection: §F01.FR.13, §7.1 vs §7.2,
§2.10, B09.FR.31 (errata 2025-09 §2.12 — exists as cited), TxStarted
vs TxInProgress appendix distinction. V6-E3 (Explorer's HIGH flag on
B09.FR.31) was a false positive — citation is valid.
All quality gates pass: typecheck, lint, format, 2857/2857 tests.
V7 review pass — 4 agents (oracle ×2, librarian, explore) with v6 lessons
integrated as new criteria: regression-chain audit, type-cast scrutiny,
test-gap blast-radius, helper-signature swap audit, false-positive
detection via qmd. Convergent findings reconciled into 11 in-scope fixes.
False positives rejected (lesson from V6-E3/E4):
- V7-E2/E17 (Explore HIGH unique): claimed startTransactionOnConnector
misses transactionPending=true. REJECTED — ATG path sends
TransactionEvent(Started) with triggerReason=Authorized, not cable-
plugin-first. Setting transactionPending=true there would create
spurious echoes on already-authorized connectors.
Fixes applied:
V7-Q1+X5 (idiom): swap ternary spread '?: {}' to '&& { x }' to match
the dominant codebase pattern (ChargingStation.ts, OCPP16TestUtils.ts,
AbstractUIService.ts).
V7-Q4+S1 (helper docstring): trim from 6 lines to 3 + reword to avoid
implying §F01.FR.13 mandates status=Rejected (spec is silent on status).
V7-Q5 (test comment): trim P4 duplicate of helper docstring to 1-line
anchor — assertions already self-document.
V7-Q7+S2+X6 (UUIDv4 cast safety): add inline justification comment at
the 'as UUIDv4' cast site pointing to the source-of-truth write site
(line ~2740 generateUUID() assignment).
V7-Q2 (reasonCode lock): add statusInfo.reasonCode assertions to 6 v6-Q2
tests so a future bug swapping codes (e.g., InvalidIdToken vs
InvalidValue) cannot pass: InvalidIdToken×2, InvalidProfile×2,
InternalError×1, NotFound×1.
V7-Q3 (residual TxInProgress test): add test exercising the fallthrough
branch via connectorStatus.locked = true — verifies reasonCode=TxInProgress
and transactionId=undefined (the V6-S2 split's other branch).
V7-E8 (citation qualifier): TemplateSchema.ts:285,295 inline error
messages use 'part 1 §7.2' for consistency with v6-S1 correction style.
V7-E9 (empty subclass discriminants): add 'public override readonly
name = ... as const' to 5 error subclasses (OCPPError,
TemplateValidationError, PayloadTooLargeError,
ConfigurationValidationError, AuthenticationError — last had non-readonly
non-as-const override). Extends V6-X14 TimeoutError pattern uniformly.
Deferred (rationale documented):
- V7-Q6+X4 — TimeoutError commit-body typo and 'matches OCPPError'
inaccuracy — git history immutable, factual note belongs in PR desc.
- V7-E1 — ConnectorStatus.transactionId: number|string union (OCPP 1.6
numeric IDs share the field). Narrowing requires OCPP 1.6 refactor.
- V7-E3+E6+E10+E12+E13+E14+E15+E16+E18 — verified clean / not in scope
(defensive code correct, no echo fields, plausible 1.6 citations).
Verified via qmd ocpp-specs (OCA FINAL schema, lorenzodonini/ocpp-go,
mobilityhouse/ocpp, citrineos-core, EVerest/libocpp): TxStarted vs
TxInProgress appendix distinction matches v6 split; transactionId on
status=Rejected is schema-valid; no reference impl contradicts v6;
typeof===string + as UUIDv4 cast is safe given OCPP 2.0 invariant.
All quality gates pass: typecheck, lint, format, 2852/2852 tests.
False positive rejected (lesson from V6-E3, V7-E2):
- V8-E2 (Explore L) — claimed BaseError has no name override and would
emit name='Error' on direct instantiation. REJECTED: BaseError's
constructor already does this.name = new.target.name, so direct
instantiation yields name='BaseError' correctly. Caller-trace
verification (same v7 criterion) eliminated this.
Fixes applied:
V8-E1 (M) — Extend V7-E9 pattern to ui/common package missed entirely
by v7. ConnectionError and ServerFailureError in ui/common/src/errors.ts
used mutable constructor-assignment 'this.name = ...' instead of the
class-field pattern. Replaced with 'public override readonly name =
"<ClassName>" as const' class fields, removed the constructor
assignments. Now all 8 error subclasses across src/ and ui/common share
the same nominal-discriminant pattern.
V8-E3 (L) — handleRequestStartTransaction had 1 remaining inline
rejection literal at line 2516 (the resolvedEvseId == null branch with
reasonCode=NotFound) bypassing the buildRejectedResponse helper used by
all 11 other rejection returns in the same function. Replaced with
buildRejectedResponse(ReasonCodeEnumType.NotFound, 'No available EVSE
found for remote start') for invariant coherence.
V8-S1 (L) — RequestStopTransaction test 'should reject stop transaction
for non-existent transaction ID' (line 133) used 'non-existent-
transaction-id' as input, which fails UUID format validation at line
2783 and never reaches the TxNotFound branch (line 2796). The test
covered InvalidValue (format-rejection), not TxNotFound. Renamed test
to '... for invalid transaction ID format - non-UUID string' to match
actual coverage + added new test 'should reject stop transaction for
valid UUID with no matching transaction' that uses
'00000000-0000-4000-8000-000000000000' (valid UUIDv4 format, no
matching transaction) and asserts reasonCode=TxNotFound. The real
TxNotFound branch is now covered.
Deferred (rationale documented):
- V8-S2/Q1 — line-number pointer '~2740' in the as UUIDv4 cast comment
is fragile but acceptable; tilde already signals approximation.
- V8-S3/Q2 — §F01.FR.13 anchor appears at 4 sites (helper docstring,
branch comment, V6-S2 test comment, V7-Q3 test name). Each lives at a
distinct cognitive level (helper API vs handler decision vs test
intent); not collapsible.
- V8-E4 (echo of V7-S7) — handleRequestStopTransaction inline literals
return a different response type (OCPP20RequestStopTransactionResponse);
buildRejectedResponse cannot be reused. Helper-extraction follow-up
out of scope.
- V8-E5 (echo) — 3 ?: {} sites in test-helper files. Already noted.
Saturation declaration:
- v8 found 3 substantive findings (V8-E1, V8-E3, V8-S1). v9 sweep would
target: (a) ui/common error subclasses (now all readonly as const),
(b) buildRejectedResponse call sites (now all 13 use the helper), (c)
RequestStopTransaction test coverage (now exercises both InvalidValue
and TxNotFound branches). Expected v9 finding count: 0.
Verified via qmd ocpp-specs (5 audits cumulative), empirical Node.js
v24 testing (V7-E9 stack trace + JSON.stringify + util.inspect +
winston format paths), and external references (5 OSS OCPP impls + 5
major TS error-class repos for readonly-name canonical confirmation).
All quality gates pass: typecheck (src + ui/common), lint (src +
ui/common), format, tests (src 2859/2865, ui/common 120/120, 0 fail).
Aligns OCPP 2.0 ResponseService test wiring with the established pattern used
by RequestService and VariableManager:
- New src/charging-station/ocpp/2.0/__testable__/OCPP20ResponseServiceTestable.ts
exporting createTestableResponseService + TestableOCPP20ResponseService;
mirrors OCPP20RequestServiceTestable.ts structure (function declarations,
file-level JSDoc with @example, double-cast through unknown).
- __testable__/index.ts re-exports the new module.
- OCPP20ResponseService-TransactionEvent.test.ts and -ForceTxOnInvalid.test.ts
now import createTestableResponseService from __testable__/index.js;
call sites renamed (drops redundant OCPP20 prefix, consistent with
createTestableRequestService).
- OCPP20ResponseServiceTestUtils.ts now contains only the test-only
buildTransactionEventRequest payload builder; adopts @file header and
arrow-style export to harmonize with MockFactories.ts neighbour.
Test-only refactor; no production behavior change.
Centralizes the two patterns used by OCPP 2.0 IncomingRequest tests to
inject mock OCPPAuthService instances into the factory cache:
- injectMockAuthService(station, service) — wraps the station-id lookup +
OCPPAuthServiceFactory.setInstanceForTesting plumbing; replaces the
duplicated boilerplate in ClearCache.test.ts and LocalAuthList.test.ts.
- withThrowingAuthServiceFactory(errorMessage, fn) — scoped monkey-patch +
try/finally restore for the 3 sites that must exercise a failing
getInstance (factory unavailable, no Authorization Cache support).
Required because setInstanceForTesting only injects a returned instance;
it cannot make getInstance itself throw.
Side effect of the helper adoption:
- LocalAuthList.test.ts drops the suite-scoped originalGetInstance save
and afterEach restore — no longer needed since every monkey-patch is
now scoped to its own test via withThrowingAuthServiceFactory.
- ClearCache.test.ts drops the local try/finally on the no-cache-support
test for the same reason.
Test-only refactor; no production behavior change.
Signed-off-by: Jérôme Benoit <jerome.benoit@sap.com>
* test(ocpp): align auth mock helper verbs and drop dead return type
V2-1: harmonize JSDoc verbs across the auth injection helpers — `inject*`
docstrings now start with 'Inject' to match the function name, and the local
`setupMockAuthService` wrappers (which build the mock + inject it) now start
with 'Configure and inject' to reflect their broader role. Avoids the verb
collision between the shared `injectMockAuthService` and the local
`setupMockAuthService` helpers.
V2-2: drop the dead `OCPPAuthService` return type from LocalAuthList's local
`setupMockAuthService` — no call site captured the return value. Also drops
the now-unused `OCPPAuthService` type import.
Test-only refactor; no production behavior change.
Signed-off-by: Jérôme Benoit <jerome.benoit@sap.com>
* test(ocpp): align ClearCache test hook ordering with siblings
Moves the `afterEach` block below `beforeEach` in ClearCache.test.ts to match
the before-then-after ordering used by LocalAuthList, TransactionEvent and
ForceTxOnInvalid test files. Cosmetic; no behavior change.
- Promote 'internal' | 'transport' string-literal union to enum
UIRequestOrigin { INTERNAL, TRANSPORT } hosted in src/types/UIProtocol.ts.
Matches SCREAMING_SNAKE_CASE convention used across the file
(ProcedureName, ResponseStatus, BroadcastChannelProcedureName).
- Promote UIRequestContext interface to src/types/UIProtocol.ts.
- Widen canonical ProtocolRequestHandler with optional context? parameter
(backwards-compatible).
- Re-export UIRequestOrigin and UIRequestContext from src/types/index.ts.
- Drop the local UIServiceProtocolRequestHandler shadow in AbstractUIService.ts;
consume the canonical ProtocolRequestHandler.
- Replace inline 'internal' / 'transport' literals at the two AbstractUIService
defaults and the AbstractUIServer.sendInternalRequest call site.
- Tighten the canonical ProtocolRequestHandler in src/types/UIProtocol.ts:
required params (uuid, procedureName, payload) match the runtime invariant
guaranteed by the ProtocolRequest tuple destructure in requestHandler();
return union collapses to Promise<ResponsePayload | undefined> | ... | undefined.
- Tighten handleProtocolRequest signature accordingly and drop the dead
'Invalid protocol request' runtime throw (unreachable: requestHandler only
calls it after the tuple has been destructured into non-null values).
- Rename interface BroadcastResponseLogContext to
BroadcastChannelResponseLogContext for symmetry with the
BroadcastChannel{ProcedureName, RequestContext, ...} family, and export
it so test helpers can type-check the structured log payload.
- Align log strings in logBroadcastResponseWithoutHandler from
'transport handler' to 'response handler' to match the public API
(hasResponseHandler / responseHandlers). Test names and regex matchers
updated atomically.
Signed-off-by: Jérôme Benoit <jerome.benoit@sap.com>
* style(ui-server): rename active to stopped and document rollback invariants
- Rename AbstractUIService.active boolean to stopped (default false, flipped
by stop()). The branch label 'late broadcast response after UI service
stop' now matches the field name; the read site at
logBroadcastResponseWithoutHandler reads as 'if (this.stopped) debug,
else warn' without needing a comment.
- Invert the two log branches under requestContext == null accordingly:
the stopped branch leads with debug, the active branch with warn.
- Add a 1-line rationale comment on the try/catch in
sendBroadcastChannelRequest documenting the bookkeeping/dispatch
atomicity invariant (rollback the just-recorded context if the worker
dispatch throws).
- Add a 1-line rationale comment on the try/finally in
UIServiceWorkerBroadcastChannel.responseHandler documenting the
aggregation-state release invariant under a sendResponse throw.
- Add expectSingleLog(mocks, level, pattern, contextShape?) to
UIServerTestUtils.ts. Collapses the repeated
'warnMock.length === 0 / debugMock.length === 1 + typeof guard +
assert.match' block previously duplicated across six broadcast-response
tests. Accepts an optional structured contextShape that, when provided,
deep-equals the second logger argument against the
BroadcastChannelResponseLogContext payload.
- Rewrite the six broadcast-response classification tests to invoke the
helper. Each test now asserts the full structured log context (origin,
procedureName, status, uuid, hashIds{Failed,Succeeded}), not just the
message string — catching regressions that swap origin, drop
procedureName, or omit hashIds propagation into the log payload.
- Add a new regression test 'should rollback expected responses when
broadcast dispatch throws' that stubs uiServiceWorkerBroadcastChannel
.sendRequest to throw, calls service.requestHandler with a broadcast
procedure, and asserts service.getBroadcastChannelExpectedResponses
rolled back to 0 — locking the try/catch invariant introduced in this
PR.
- tests: replace Reflect.get + double 'as never' chain in the broadcast
dispatch rollback regression test with a typed cast to
UIServiceWorkerBroadcastChannel. Kills both 'as never' casts and the
'(): never' return annotation. Field rename now fails the test loudly
via TS instead of silently mis-testing.
- tests: rename 'should warn on untracked broadcast responses while
service is active' to '... before service stop' so the test name
matches the post-rename 'stopped' field vocabulary.
- types: add JSDoc on the new public surface introduced by this PR -
UIRequestOrigin, UIRequestContext, ProtocolRequestHandler widening
(UIProtocol.ts), and BroadcastChannelResponseLogContext
(AbstractUIService.ts). Docs describe contract, not mechanism.
Drop the `#### Migration notes` block under the worker process model
section that documented the `uiServer.metrics.enabled=true` listener-
inheritance change. Operational guidance lives with the feature
documentation; this stand-alone block was redundant.
Jérôme Benoit [Tue, 23 Jun 2026 22:35:14 +0000 (00:35 +0200)]
feat(ui-server): expose Prometheus /metrics on ws and mcp transports (#1921)
The opt-in Prometheus `/metrics` endpoint introduced by #1912 was wired
only into `UIHttpServer`. With `uiServer.type` set to `ws` or `mcp`, the
endpoint was silently disabled with a startup warning.
Move the metrics registry, the request handler, and the soft-cap
accounting from `UIHttpServer` up to `AbstractUIServer`, then expose
`/metrics` on the shared `httpServer` from `UIWebSocketServer` and
`UIMCPServer` via a single `tryServeMetrics` template method that
encapsulates the `metrics.enabled` gate, the path predicate, the
authentication step, and the scrape handler. The
`uiServer.metrics.{enabled,softSampleCap}` configuration block is
preserved; the gauge registry is re-used unchanged; and `accessPolicy`,
rate-limiting, and `authentication` apply identically on all three
transports.
The lifecycle is bracketed by a public `start()` template-method on
`AbstractUIServer`; subclasses implement the `attachTransport()`
protected hook and may override the symmetric `detachTransport()` hook.
A one-shot `transportAttached` guard is set BEFORE `attachTransport()`
runs so a throwing hook cannot leave a half-attached server admitting a
silent retry; on failure any partially-registered `'request'` /
`'upgrade'` listeners on `httpServer` are stripped and the guard is
rolled back. `metricsScrapeChain` is reseated to `Promise.resolve()`
at every `start()` so cycle N+1 never awaits cycle N's scheduled
registry clear; in `runMetricsScrape`, the per-scrape
`metricsSampleCount` is captured into a local snapshot BEFORE
`await registry.metrics()` yields, so a peer scrape dispatched after
a sync `stop()`/`start()` cannot overwrite the count the soft-cap
branch reads. The corresponding `stop()` is symmetrically defensive:
both `detachTransport()` and each `uiService.stop()` are wrapped in
per-iteration `try/catch` so a throwing override does not abort
downstream teardown (`stopHttpServer`, remaining services, handlers,
caches), and the `transportAttached = false` reset runs in a `finally`
block so a subsequent `start()` is never permanently locked out — any
other unexpected throw still propagates. A shared
`renderNotFoundAndDestroy(req, res)` helper consolidates the 404
fallback used by the WebSocket and MCP request listeners; the helper
delegates socket teardown to `destroyHttp1SocketIfPending(req)` which
skips `req.destroy()` on HTTP/2 streams (lifecycle owned by the
`Http2Stream`) and on already-complete HTTP/1.1 requests (keep-alive
pool).
No new configuration surface, no new listener: `/metrics` is served on
the same `Http2Server | Server` instance that already backs the
WebSocket protocol upgrade in `UIWebSocketServer` and the `/mcp` route
in `UIMCPServer`. HEAD responses emit an explicit `Content-Length`
equal to the GET body byte length (`Buffer.byteLength(body, 'utf8')`)
and an empty body, per RFC 9110 §9.3.2. A frozen
`METRICS_ALLOWED_LABEL_NAMES` tuple pins the canonical PII surface; the
`isMetricsAllowedLabelName` type-guard predicate is the O(1) lookup,
and a guardian spec asserts every gauge label name in the rendered
registry is admitted.
Operator-visible change: `uiServer.metrics.enabled=true` with
`type='ws'` or `'mcp'` now serves `/metrics` whereas the previous
release silently disabled it. Operators relying on the silent disable
must set `metrics.enabled=false` (or omit the block) and audit firewall
posture on the UI server port. `HEAD /metrics` (e.g. `curl -I`) returns
identical headers to `GET` (including `Content-Length`) with an empty
body. README updated.
Expose simulator state on GET /metrics when uiServer.metrics.enabled=true.
The endpoint is grafted into UIHttpServer.requestListener after the
existing runRequestPrologue (access policy + rate limit) and authenticate
calls, so it inherits per-client rate limiting, host/origin/proxy
validation and basic-auth credentials with no additional surface.
Prometheus is HTTP-only by spec; the flag is honoured only on
uiServer.type=http and AbstractUIServer.warnIfMisconfigured logs a
warning otherwise.
Metric families (exhaustive within the data already exposed via
ChargingStationData and Bootstrap.getState()):
PII reject-list (HARD): supervisionUrl, chargeBoxSerialNumber,
chargePointSerialNumber, meterSerialNumber, iccid, imsi,
authorizeIdTag/localAuthorizeIdTag/transactionIdTag/transactionGroupIdToken
and OCPP customData. transactionId / remoteStartId never used as labels
(unbounded cardinality). Adversarial label values are escaped per
prom-client's exposition-format rules so synthesized # HELP / # TYPE
lines cannot be injected.
Cardinality soft cap: a single warning log per scrape when total
samples exceed METRICS_SOFT_SAMPLE_CAP=5000. The response is still
served in full so operators retain observability while being alerted
to fleet growth.
Adds prom-client@^15.1.3 and 17 new tests under
tests/charging-station/ui-server/UIMetricsEndpoint.test.ts covering
end-to-end exposition, security inheritance (access policy / rate
limit / basic-auth), the PII reject-list, exposition-format escaping
and the soft-cap warning. README.md updated with a "Metrics endpoint
(Prometheus)" subsection (config example, sample output, basic-auth
compatibility note, privacy/cardinality note, scrape_config snippet).
Refs: #851
* fix(ui-server): rebuild metrics registry on start to recover from stop/start cycle (M1)
The metrics registry was built only in the constructor and cleared on
stop(); after Bootstrap.stopUIServer() then startUIServer() (live-reload
path on uiServer.enabled toggle), metricsRegistry was undefined and
/metrics silently fell through to the JSON-RPC parser as a 400
Malformed URL with no log to indicate misconfiguration.
Move the registry build into start() guarded by an idempotency check
so the endpoint recovers across restart cycles. The constructor no
longer touches metricsRegistry.
Refs: #851 (Phase 4 review M1)
* feat(ui-server): expose metrics.softSampleCap in canonical defaults (M3)
The soft cardinality cap was documented as something operators may
'scale' but was not in the canonical defaults map. Add an optional
softSampleCap to UIServerMetricsConfigurationSchema (default 5000 via
METRICS_SOFT_SAMPLE_CAP) so operators can tune the threshold without
patching code, per AGENTS.md options-and-configuration rule.
Two concurrent GET /metrics requests interleaved on the shared
metricsSampleCount field and shared Gauge state, making the soft-cap
warning unreliable and the body content racy. Serialize scrapes via a
per-server promise chain; stop() now awaits the in-flight scrape
before clearing the registry, so a request mid-await no longer sees
undefined gauges.
Also adds the headersSent / writableEnded guard on the resolve path
so a client that closes mid-await does not trigger an emit-after-end.
The cap value is now read from uiServerConfiguration.metrics?.softSampleCap
(falls back to METRICS_SOFT_SAMPLE_CAP), wiring up the M3 schema field.
Refs: #851 (Phase 4 review M2 + m6 + m5)
* fix(ui-server): align iterateConnectors with simulator_station_connectors_total either-or (m2)
The per-connector iteration helper unconditionally yielded entries from
both data.connectors and data.evses, while simulator_station_connectors_total
uses an either-or rule. In current production these arrays are mutually
exclusive (buildConnectorEntries returns [] when hasEvses=true), so the
collision was unreachable, but the inconsistency made the helper fragile
to future invariant changes. Align it with the gauge's logic.
Refs: #851 (Phase 4 review m2)
* fix(ui-server): accept HEAD on /metrics for liveness-probe scrapers (n3)
Some Prometheus tooling HEAD-probes the endpoint before scraping.
Treat HEAD identically to GET in isMetricsRequest; Node drops the body
automatically on HEAD responses.
Refs: #851 (Phase 4 review n3)
* style: replace 'honoured' with 'honored' for spelling consistency (n2)
Aligns the new metrics docstring/log with American spelling used
elsewhere in the README and source.
Refs: #851 (Phase 4 review n2)
* docs(readme): fix metrics sample output to match actual exposition (m4)
The previous sample showed simulator_charging_stations_configured_total
with a 'template' label, but that metric is global with no labels.
The per-template counter is the separate simulator_template_configured
gauge. Replace the sample with the actual exposition shape.
Refs: #851 (Phase 4 review m4)
* test(ui-server): add wsState=undefined and EVSE-mode coverage (M4)
The new tests close two regression-detection gaps identified in Phase 4
review: (a) removing the !== undefined guard on data.wsState would
silently emit NaN — now caught by an absence assertion; (b) the OCPP
2.0.x code path (evses populated, connectors empty, evseStatus.connectors
Map iteration) was entirely untested — now exercised end-to-end.
Refs: #851 (Phase 4 review M4)
* test(ui-server): add soft-cap boundary tests for off-by-one detection (m3)
Probe-then-verify pattern: first scrape with a very high cap to count
actual samples produced; then assert no warn fires when cap equals the
sample count (strict-greater-than semantics) and one warn fires when
cap is one below. This regression test would fail if '>' became '>='
on the soft-cap comparison.
Refs: #851 (Phase 4 review m3)
* test(ui-server): retarget rate-limit burst at allowed loopback path (m1)
T9 previously hit a non-loopback denied path, exercising the rate
limiter on a 403 response rather than on /metrics. Switch to a
loopback request that is allowed through the access policy and assert
that the rate limit fires on /metrics directly.
Refs: #851 (Phase 4 review m1)
* style(test): rename tests per TEST_STYLE_GUIDE and drop redundant assertion (n1, n4)
Renamed all tests from 'T<N>: <verb>...' to 'should <verb>...' format
per tests/TEST_STYLE_GUIDE.md. Also dropped the redundant pre-stop
assertion in 'should clear registry on stop()' (the post-stop
assertion already proves the stop() effect).
Refs: #851 (Phase 4 review n1, n4)
* refactor(ui-server): add HEAD to HttpMethod enum (n1)
Avoid the bare 'HEAD' string literal in UIHttpServer.isMetricsRequest by
extending HttpMethod with an explicit HEAD member, matching the AGENTS.md
guidance to prefer enumerations over string literals when one exists.
Phase 6 review feedback consolidated into one coherent surgery on the
metrics path:
- M1: defineGauge now returns Gauge<L> with auto-injected registers and a
string-literal label-name generic. Every collect() callback is non-arrow
with typed 'this: Gauge<L>', per the prom-client documentation. Eliminates
~20 unsafe 'as Gauge | undefined' casts and ~30 dead null-guards.
- m1: collapse the four global aggregate gauges into a single tuple-loop,
mirroring the per-template loop pattern.
- m2: append a terminal '.catch(() => undefined)' to the metricsScrapeChain
reassignment in stop() so the field always points to a handled promise.
- m3: document the metricsSampleCount/metricsScrapeChain concurrency
invariants; explicitly forbid async collect callbacks.
- n2: factor 'ChargingStationDataProvider' type alias at module scope.
- n3: drop the redundant outer 'async' on handleMetricsRequest.
- n4: replace box-drawing dividers with plain JSDoc section markers.
- n5: rename 'PII whitelist invariant' to 'PII allowlist invariant'.
- n6: document the OCPP either-or rule on iterateConnectors.
The HTTP /metrics contract is unchanged; existing tests pass without
modification.
* docs(readme): align metrics documentation with code (m4, m5)
- m4: replace the two stale '# HELP' lines in the sample exposition block
with the strings actually emitted by the registry (commit 1bac5c23d
fixed two of the four; the other two had drifted again).
- m5: add the 'metrics' sub-key to the uiServer row of the configuration
table: bullet under the Description column documenting metrics.enabled
and metrics.softSampleCap, and corresponding shape in the Value type
column. AGENTS.md 'Documentation conventions / Exhaustivity' treats the
table as the authoritative tunable list.
* test(ui-server): regression for concurrent /metrics scrapes (R1, R2)
Locks the metricsScrapeChain serialization invariant against future drift:
- R1: two concurrent scrapes against a registry whose sample count equals
the configured softSampleCap; honest serialized execution produces zero
warns. A regression that removes the chain (or makes any collect()
callback async) would either spuriously warn (counter shared between
scrapes) or corrupt the body, both detected by this test.
- R2: same test indirectly guards against async collect callbacks; the
invariant is also documented as a JSDoc on buildMetricsRegistry.
- M1: collapse 3 inline per-station gauges (ws_state, connectors_total,
boot_status_info) into existing helpers (~40 LOC less duplication).
- n2: simulator_station_connectors_total now counts via iterateConnectors,
making the iterateConnectors JSDoc 'shared with' claim literally true.
- n3: replace 'const self = this' with a typed 'provider:
IChargingStationDataProvider' built from .bind(this) method captures.
Drops the @typescript-eslint/no-this-alias suppression and narrows the
type surface accessed by collect() callbacks.
- n4: justify defineGauge<L extends string = never> default in the JSDoc
(stricter than prom-client's 'Gauge<T extends string = string>').
- n5: addPerStationInfoLabel hard-codes 'status' (all callers used it);
addConnectorOneHot narrows labelName to a literal union of valid keys —
both eliminate the theoretical 'hash_id'/'connector_id' clobber risk.
- n6: rename ChargingStationDataProvider to IChargingStationDataProvider
(interface form, repo I-prefix convention) and extend with
getChargingStationsCount.
- n8: tighten handleMetricsRequest and iterateConnectors @param
descriptions to remove low-signal restatements.
- n9: drop the explanatory '// Explicit return required by
promise/always-return lint rule' comment; the lint rule speaks for
itself when the line is removed.
* test(ui-server): tighten concurrent-scrape regression with sample-count assertion (n1)
Replace the weak 'bodyA === bodyB' check (which would pass even if the
metricsScrapeChain serialization were removed, since both scrapes
collect against the same Registry instance) with an exact sample-line
count assertion against the probed value. Locks two distinct invariants
that bodyA===bodyB did not: no truncation, and no double-count from
interleaved 'collect()' calls under prom-client's internal Promise.all.
* docs(readme): add trailing semicolon to metrics value type (n7)
Align the new 'metrics?: { ... }' member of the uiServer Value type
column with the surrounding style (every other nested-object member
in the same type literal terminates with ';').
- n2: rename addPerStationInfoLabel to addPerStationStatusInfo to match
the helper's actual responsibility (it hard-codes the 'status' label
per Phase 7 n5).
- n3: drop the I-prefix on ChargingStationDataProvider — most interfaces
in src/ are unprefixed, only IBootstrap uses I.
- n4: extend the ChargingStationDataProvider JSDoc to acknowledge that
the inline simulator_ui_server_known_stations_total gauge consumes the
getChargingStationsCount method (not only the helpers).
- n5: extract countConnectors as a single source of truth for the OCPP
either-or rule and have simulator_station_connectors_total consume it.
Also restructure the iterateConnectors JSDoc to cross-reference
countConnectors instead of the gauge.
- n7: tighten @param res on handleMetricsRequest to 'HTTP response to
end with the exposition body.' (restores the direction Phase 7 n8
shortened away).
- n1 (addConnectorOneHot generic threading) is deferred: TypeScript
cannot narrow the dynamic computed property '[labelName]: v' to
'Record<L, string>' without an 'as' cast, which AGENTS.md 'Type
safety' forbids. The runtime literal-union on labelName is kept as
the type-safety contract; a 3-line comment documents the trade-off.
- n1: thread Gauge<...> end-to-end on addConnectorOneHot via a
ConnectorOneHotLabel type + typed-init + property mutation pattern.
Phase 9 oracle B proved the cast-free pattern compiles cleanly under
strict TS 6.0.3 (probe with 8 alternatives, only this one passes).
Replaces the Phase 8 deferral comment, which mis-attributed an 'as'
ban to AGENTS.md (AGENTS.md only forbids '!' and 'any').
- n2: drop the self-{@link} on countConnectors JSDoc opening — the
block documents countConnectors itself, so the cross-reference back
to it reads awkwardly. Asymmetric pattern with iterateConnectors
preserved.
- n3: drop the duplicate '(see {@link countConnectors} for the
invariant source)' parenthetical on iterateConnectors JSDoc — the
leading 'same...rule as {@link countConnectors}' already directs the
reader, and 'invariant source' was jargon.
* docs(ui-server): harmonize JSDoc prose with repo cadence
Three Phase 9-pass-2 audit findings against the rest of the codebase:
- Drop the coined phrases 'either-or rule' and 'OCPP-version-driven'
from countConnectors and iterateConnectors JSDoc; the repo-wide
prose simply names 'OCPP 1.6' / 'OCPP 2.0.x' inline (see ChargingStation.ts
'$OCPP 2.0.1 §4.2.3', Helpers.ts 'OCPP 2.0 chargingSchedule',
TemplateSchema.ts 'OCPP 2.0.1 §7.2'). Use 'mutually exclusive' for the
source-split semantic.
- Drop the bold-stress '**Invariant**:' prose markers from the
metricsSampleCount, stop() and buildMetricsRegistry JSDoc; `grep -rn '\*\*'
src/` returns 0 such markers in JSDoc anywhere else. Inline the
invariant prose into the surrounding sentences without a section
marker.
* docs(test): harmonize 'soft cap' spelling and @description cadence
Two outliers found in the test file's prose during repo-wide audit:
- 'soft-cap' (3 occurrences: @description, 1 test name, 1 inline comment)
was hyphenated while the production warning string and ConfigurationSchema
JSDoc both spell it 'soft cap' / 'soft sample cap' (no hyphen). Tests'
string-includes('soft cap') matches were already correct; only the prose
drifted.
- @description was multi-line whereas every other test file in tests/
uses a single-line @description (verified across ChargingStation*.test.ts,
ConfigurationKeyUtils.test.ts, TemplateValidation.test.ts, etc.).
* docs(ui-server): harmonize misconfiguration warning style with repo cadence
The warnIfMisconfigured warning used an em-dash + uppercase 'NOT' to
separate the condition from the consequence:
metrics.enabled=true is only honored when uiServer.type='http';
current type='X' — the /metrics endpoint will NOT be served.
Sibling warnings in AbstractUIServer (host-not-allowed and tls-required
at lines 441-457) use semicolon-and-period separators with normal-case
prose and a final action sentence. Match that cadence:
metrics.enabled=true is honored only when uiServer.type='http'; current
type='X'. The /metrics endpoint will not be served. Set uiServer.type='http'
to expose metrics.
Test substring matches ('metrics' / 'http') are preserved.
* test(ui-server): rename M-prefix fixtures to T-prefix convention
The fixture station hash IDs 'station-M{evse,boundary,concurrent}' inherited
the 'M' prefix from the Phase 4 review's MAJOR finding IDs (M1..M4) and
were tokenized by cspell as unknown words 'Mevse', 'Mboundary',
'Mconcurrent' — emitting 7 lint warnings.
The rest of the metrics test file already follows the 'station-T<n>'
numeric convention ('station-T5', 'station-T12', 'station-T13',
'station-T16'), where <n> matches the test ordinal. Map the M-prefixed
fixtures to their actual test ordinals:
- 'Mevse' -> 'T18' (EVSE-mode test, the 18th 'await it()')
- 'Mboundary-*' -> 'T19-*' (soft-cap boundary, 19th)
- 'Mconcurrent-*'-> 'T20-*' (concurrent-scrape regression, 20th)
Quality gates now report 0 errors AND 0 warnings; previous baseline
was 0 errors / 7 warnings.
* docs(readme): drop redundant Metrics endpoint section, harmonize uiServer.metrics bullet
The dedicated '### Metrics endpoint (Prometheus)' section duplicated
information that is already authoritative elsewhere:
- The /metrics behavior, configuration shape and defaults are documented
in the uiServer row of the configuration table (single source of
truth, per AGENTS.md 'No duplication' rule).
- The PII reject-list and softSampleCap semantics are documented in
UIServerMetricsConfigurationSchema JSDoc.
- The HTTP-only constraint and the warning behavior are documented in
the AbstractUIServer.warnIfMisconfigured warning string itself.
Drop the section, the TOC entry, and the now-dangling cross-link from
the table bullet. Restructure the _metrics_ bullet to mirror the
_accessPolicy_ cadence in the same row (top-level intro + indented
sub-bullets per sub-key), so the table description is self-sufficient
and harmonized with its sibling.
.codegraph is a symlink to .omo/codegraph/projects/<hash>/ used by the
oh-my-opencode tooling, on the same lifecycle as .omo/ and .sisyphus/.
Group it under the existing 'oh-my-opencode' section.
Jérôme Benoit [Wed, 17 Jun 2026 17:22:45 +0000 (19:22 +0200)]
style(test): fix lint warnings in OCPP 2.0 ForceTxOnInvalid test
Three pre-existing post-merge warnings introduced by the squash-merge
of #1907 on the file:
`tests/charging-station/ocpp/2.0/OCPP20ResponseService-ForceTxOnInvalid.test.ts`
- Lines 67-68: `jsdoc/require-param-description` — fill the empty
`@param idToken.idToken` and `@param idToken.type` sub-property
descriptions auto-inserted by eslint-plugin-jsdoc.
- Line 298: `@cspell/spellchecker` — replace "remock" with "re-mock"
(English-correct hyphenation, matches existing repo idioms like
"re-entrant", "re-export").
Verification: pnpm lint exit 0 with 0 errors / 0 warnings; focused
`pnpm test` on the file: 16/16 GREEN (substring assertions unaffected).
Jérôme Benoit [Wed, 17 Jun 2026 16:49:22 +0000 (18:49 +0200)]
feat(simulator): add forceTransactionOnInvalidIdToken template flag (#1907)
* feat(simulator): add forceTransactionOnInvalidIdToken template flag
Allow station-initiated transactions to continue even when CSMS replies
with a non-Accepted IdToken status (OCPP 1.6 idTagInfo.status, OCPP 2.0.1
idTokenInfo.status). Default false preserves spec-compliant behavior.
The flag is non-spec-compliant by design (violates OCPP 2.0.1 E05.FR.09,
E05.FR.10, E06.FR.04 when enabled) and is intended for testing edge-case
charging station implementations that ignore CSMS rejection. JSDoc on the
field warns explicitly; README entry flags it as a non-spec-compliant test
override.
Scope in OCPP 2.0.1 is limited to TransactionEvent eventType=Started;
mid-transaction revocation (Updated/Ended event types) still triggers
deauthorization regardless of the flag. The OCPP 2.0.1 \`StopTxOnInvalidId\`
device-model variable is left untouched — the template flag short-circuits
ahead of the variable's accounting branch.
Tests: 16 new test cases across both OCPP namespaces covering force-on/off,
mid-tx revocation preservation, override marker log presence, auth cache
non-relaxation, pre-Start guard preservation, and full status-enum parity
(8 OCPP 2.0.1 statuses).
Closes #1826
* fix(types): restore fixedName and pin endedConnector non-null
The forceTransactionOnInvalidIdToken JSDoc rewrite in 73e3358a accidentally
deleted the adjacent fixedName?: boolean field on ChargingStationTemplate,
breaking 30+ TS errors in CI (ChargingStationNameTemplate Pick keyed on
fixedName). Local pnpm test does not run tsc --noEmit, so the regression
was invisible until CI.
Also fix a CI-only error in OCPP20ResponseService-ForceTxOnInvalid.test.ts:
endedConnector is typed as ConnectorStatus | undefined; replace the
optional-chain assertions with an explicit if/fail guard so TS narrows
the type for the subsequent strict-equal assertions.
* test(2.0): document deferred MV-pump fence and ConcurrentTx scope
Phase-4 review-C MINOR-1: the 2.0-T2 test asserts that
\`startUpdatedMeterValues\` is called but stubs the helper, so a wire-level
regression where the interval binds to the wrong connector would not be
caught. Phase 6 golden-set with the live mock CSMS will close that gap;
add a TODO comment to make the deferral explicit.
Phase-4 review-C NIT-1: the T7 enum-parity loop omits ConcurrentTx; this
is correct (ConcurrentTx is not an IdToken rejection in OCPP 2.0.1) but
deserves an inline comment to prevent a future contributor from adding it
naively.
* test(ocpp): tighten forceTransactionOnInvalidIdToken coverage and clarify docs
Follow-up to #1907 applying post-review fixes from a 3-angle audit
(production / types-schema-readme / tests-adversarial).
Production (OCPP 2.0):
- Simplify defensive `else if (overrideRejection && status !== Accepted)`
to `else if (overrideRejection)` in handleResponseTransactionEvent;
the second predicate is entailed by the enclosing if/else.
- Trim duplicated 5-line comment block down to a 2-line pointer to the
canonical JSDoc on ChargingStationTemplate.forceTransactionOnInvalidIdToken.
Documentation:
- ChargingStationTemplate JSDoc: disambiguate from the OCPP variables
StopTransactionOnInvalidId (1.6) and StopTxOnInvalidId (2.0.1) which
control mid-transaction stop on revocation and have inverse polarity;
state independence from ocppStrictCompliance.
- README row: same disambiguation appended to the cell.
Tests (OCPP 1.6 ForceTxOnInvalid):
- T5 (pre-Start guard): change response status from Accepted to Invalid
so the test exercises the flag-vs-guard interaction it claims to lock.
- T6 (new): status-enum parity loop via Object.values(OCPP16AuthorizationStatus)
excluding Accepted and ConcurrentTx (3 instances: Blocked, Expired, Invalid).
- T2: replicate the Phase-6 fake-timer-fence TODO from the sibling 2.0 file
(MV pump observability disclosure).
- Rename test titles to should [verb] per tests/TEST_STYLE_GUIDE.md \xa71.
Tests (OCPP 2.0 ForceTxOnInvalid):
- buildTransactionEventRequest: add optional idToken parameter to exercise
the auth-cache update path at handleResponseTransactionEvent.
- T8 (new): C10.FR.01/04/05 auth-cache invariant test, parametric over
[flag=true, flag=false] (2 instances) asserting updateAuthorizationCache
is called with the supplied idToken and the CSMS-replied idTokenInfo.
- T4 (Ended): add deauth call-count assertion (=== 0); locks the
cleanup-runs-before-deauth-gate ordering invariant. The deauth path is
reached but no-ops because cleanupEndedTransaction clears transactionId
first, so getConnectorIdByTransactionId returns null. A regression that
reorders cleanup after the gate (or preserves transactionId on Ended)
flips this to 1.
- T7 (parity): replace static 8-element array with
Object.values(OCPP20AuthorizationStatusEnumType).filter(...). Same 8
instances today; future enum additions are auto-covered.
- T6: split into 2 it() blocks (flag-on / flag-off) eliminating the
mid-test standardCleanup+remock pattern. Each block additionally asserts
that the override-marker warn-log is NOT emitted on null idTokenInfo,
locking the invariant against an A6-style regression where the
override-marker else-if is hoisted outside the outer null guard.
- Rename test titles to should [verb].
Verification:
- pnpm typecheck exit 0
- pnpm lint exit 0
- pnpm test: 0 fail across the full suite
- 23/23 GREEN on the two ForceTxOnInvalid files (was 17/17; +6 new)
- Three mutation experiments confirm regression-detection strength:
* Drop `|| forceTransactionOnInvalidIdToken` from OCPP16 gate (line 427):
T2 + 3 parity tests fail with `false !== true` on transactionStarted.
* Replace `if (requestPayload.idToken != null)` with `if (false)` in
OCPP20 cache update (line 519): T8 (both flag states) fails `0 !== 1`.
* Bypass the OCPP16 unauthorized-remote-start guard (lines 315-329):
T5 only fails (regression-localized to that scenario).
* docs(ocpp): note ocppStrictCompliance independence and clarify Started log
Follow-up to review v2 of #1907 closing the two actionable findings
(B4 Low, N1 nit). Two remaining v2 nits (B5 multi-line JSDoc, R3 test
cast) are intentionally deferred — rationale in
/tmp/pr-1907-review-v2/fixes-design.md.
B4 — ChargingStationTemplate.forceTransactionOnInvalidIdToken JSDoc and
the matching README row both gain a one-clause disambiguation: "Independent
of `ocppStrictCompliance` (operates on response handling, not schema
validation)". Mirrors the pattern used by the sibling `outOfOrderEndMeterValues`
row (which cites ITS `ocppStrictCompliance` coupling), preventing readers
from inferring a non-existent coupling for the new flag.
N1 — OCPP20ResponseService.handleResponseTransactionEvent override warn
log now reads "...on eventType=Started despite..." (was "...on Started
despite..."). The `eventType=` prefix removes ambiguity with OCPP
connector state names (Charging, Available) and aligns the log vocabulary
with the JSDoc, the inline reference comment, and the test fixture
(`requestPayload.eventType === OCPP20TransactionEventEnumType.Started`).
The override-marker substring `forceTransactionOnInvalidIdToken=true` is
preserved verbatim, so the four test assertions that grep for it remain
unaffected.
Verification:
- pnpm typecheck exit 0
- pnpm lint exit 0
- pnpm test: 0 fail across the full suite
- 23/23 GREEN on the two ForceTxOnInvalid files (count unchanged; doc-only changes)
- README table re-aligned by prettier on save (column padding shifted
421 -> 460 chars to fit the longer description); diff is mechanical
whitespace, content change is one clause.
Jérôme Benoit [Tue, 16 Jun 2026 17:02:22 +0000 (19:02 +0200)]
fix(bootstrap): make start/stop idempotent and signal handler re-entrant safe (#1905)
- Memoize startPromise/stopPromise so concurrent callers await the same
in-flight transition instead of silently no-op'ing.
- Extract start()/stop() bodies into private doStart()/doStop() so the
public methods are thin memoization wrappers.
- Demote idempotent guard hits from error to warn (matches sister
ChargingStation convention) for "Cannot start/stop already ..." paths
and to debug for "Awaiting in-flight ..." concurrency observations.
- Make gracefulShutdown re-entrant safe via shuttingDown flag --
multiple SIGTERM/SIGINT/SIGQUIT no longer race the in-flight stop.
- On the no-op stop path with reason=user, ensure
.simulator-state.json still reflects the authoritative state (writes
started:false if file says started:true). Fixes UI clients reading
stale started:true after a UI-driven stop on an already-stopped sim.
- Document Bootstrap.stop coalescing semantics: the FIRST caller's
reason controls the in-flight transition; later callers' reasons are
ignored.
- Add 'entrancy' to cspell dictionary (re-entrancy is the standard
concurrency-engineering spelling).
Tests: tests/charging-station/Bootstrap.test.ts (new file, 11 tests)
- concurrent stop() / start() callers observe the same in-flight
transition
- gracefulShutdown is re-entrant: 3 synchronous calls invoke stop()
exactly once (direct unit test on Bootstrap.prototype, runs on every
platform including Windows)
- multiple SIGTERM produce a single 'Graceful shutdown' log line
(POSIX-only spawn-based integration smoke; skipped on Windows where
child.kill('SIGTERM') maps to TerminateProcess and bypasses the
handler -- coverage for that platform comes from the unit test
above)
- state-file consistency on the no-op stop path (3 sub-cases:
reason=user, reason=shutdown, real-stop-then-no-op-stop)
- idempotent guards log at warn or debug, not error (4 sub-cases)
Reproduction: 5x kill -TERM in tight loop, before fix produces 'Cannot
stop an already stopping' error log; after fix produces a single
'Graceful shutdown' info log with no error.
Refs: review feedback on PR #1905 -- 1 BLOCKER, 4 MAJOR, 4 MINOR
findings, all addressed in this amended commit.
Audit pass over the describe('buildStatusNotificationPayload') block
added by PR #1902. Adds the only two production-callsite-tied cases the
existing 9 tests miss. No production code change.
T2.1 — OCPP 1.6 with errorCode and evseId together:
- Tied to ui/cli/src/commands/ocpp.ts:191-203 (1.6 requires --error-code,
passes evseId) and ui/web/src/skins/classic/components/charging-stations
/CSConnector.vue:172-173 (selectedErrorCode defaults to NO_ERROR).
- Existing tests cover errorCode-without-evseId and evseId-without-
errorCode but never both flags together — the actual production path.
T2.2 — undefined ocppVersion with errorCode and evseId:
- Tied to ui/web/src/core/UIClient.ts:141-155 +
ui/web/src/shared/composables/useConnectorActions.ts:113-122 where
setConnectorStatus is unconditionally invoked with both options even
when props.ocppVersion is OCPPVersion | undefined.
- Existing 'undefined version' test omits both options; this asserts the
1.6 fallthrough preserves them.
3-angle audit found other proposals (cross-enum mismatch guard, marking
existing cases [SYNTHETIC], structural refactor into nested describes)
but they did not survive cross-validation: the first requires a runtime
guard in payloadBuilders.ts (out of scope per task spec), the second is
subjective, the third is cosmetic per AGENTS.md 'small verifiable
changes'.
* test(ui-common): align buildStatusNotificationPayload test names with style guide
Address review feedback on #1904.
Both new test names violated tests/TEST_STYLE_GUIDE.md "should [verb]
in lowercase" describing observable behavior:
- T2.1 carried a callsite traceability annotation
'(CLI + Web UI 1.6 path)' in the title.
- T2.2 used the implementation-internal term 'fall through' plus a
'(Web UI undefined-version path)' annotation.
Renamed to mirror the existing sibling tests in the same describe
block:
- T2.1: 'should pass errorCode and evseId through for OCPP 1.6 when
both are provided' (mirrors 'should pass evseId through for OCPP 1.6
without errorCode').
- T2.2: 'should default to OCPP 1.6 shape with errorCode and evseId
when ocppVersion is undefined' (mirrors 'should default to OCPP 1.6
shape when ocppVersion is undefined').
Callsite traceability lives in the commit message + PR body, not the
test title — matches the convention of all 9 prior cases in the same
describe. No assertion change. 116/116 tests pass.
Jérôme Benoit [Mon, 15 Jun 2026 19:20:06 +0000 (21:20 +0200)]
fix(config): enforce Zod simulator-config validation at boot (#1874) (#1903)
* fix(config): enforce Zod simulator-config validation at boot (#1874)
Audit pass over uiServer.* primitives that PR #1901 left under-constrained.
Each constraint closes a concrete fail-fast gap surfaced by a 3-angle design
review (schema completeness, fail-fast wiring, adversarial risk).
UIServerAuthenticationSchema:
- username: z.string().min(1).optional() — empty username currently passes;
AbstractUIServer falls back to '' and authenticates against the empty
configured value, silently bypassing Basic-Auth.
- password: z.string().min(1).optional() — same hole on password.
- username .refine(no ':') (RFC 7617) — lifts the runtime check at
UIServerFactory.ts:48-54 into the schema layer.
- object-level .refine: enabled === true requires both username and password.
Closes the silent-bypass when authentication is enabled with no creds.
UIServerAccessPolicySchema:
- object-level .refine: allowLoopbackProxy === true requires
trustedProxies.length >= 1. With allowLoopbackProxy: true and an empty
trustedProxies list, the proxy-trust check at runtime always evaluates
against the empty allowlist, contradicting the operator's intent.
No wiring change: Configuration.ts:160 (boot, process.exit(1)) and
Configuration.ts:386-431 (hot-reload, rollback) already route
ConfigurationValidationError correctly. Schema tightening lands at parse
time without rewiring.
Test fixture update: tests/utils/ConfigurationSchema.test.ts unknown-key
test for uiServer.authentication now includes valid username/password
alongside bogusAuthKey so the unknown-key intent stays unambiguous after
the new object-level superRefine.
9 new test cases (5 reject + 3 accept regression-anchors + 1 cross-field
reject) covering rejection paths and the docker/config.json-shape
acceptance.
Designs that did NOT survive cross-validation are documented as
out-of-scope follow-ups: type×version coupling (coupled to in-place
mutation bug at UIServerFactory.ts:69), type×auth coupling (defense in
depth at runtime is intentional), options.{path,readableAll,writableAll,
ipv6Only} refinements (no shipped config exercises these), and the
TLS-non-loopback wildcard refine (would break docker/config.json:18).
* fix(config): refine auth schema error path and tighten test path checks
Address review feedback on #1903:
src/utils/ConfigurationSchema.ts
- UIServerAuthenticationSchema object-level .refine now carries
path: ['username'], producing an error path of
'uiServer.authentication.username' rather than 'uiServer.authentication'.
Mirrors the path: ['allowLoopbackProxy'] convention already used on
UIServerAccessPolicySchema and yields a more actionable message.
- JSDoc clarifies that the field-level constraints (min(1), no ':')
apply unconditionally even when authentication.enabled is false:
the schema is intentionally stricter than the runtime guard in
UIServerFactory so a credentials value cannot ship under
enabled: false and become a bypass the moment enabled flips on
(e.g. via hot-reload).
tests/utils/ConfigurationSchema.test.ts
- Three .includes('uiServer.authentication') substring checks tightened
to .startsWith('uiServer.authentication') so they cannot match a
hypothetical sibling key like 'uiServer.authenticationFoo'. Affects
the two object-level coupling tests and the unknown-key fixture
assertion. Exact-element checks at lines 630/651 (specific leaf
paths) and the leaf-specific .includes at line 671 are left as-is.
Rewrite the JSDoc as a single factual paragraph matching the file
convention (LogConfigurationSchema, WorkerConfigurationSchema,
UIServerListenOptionsObjectSchema, ...) — 12 lines collapsed to 7,
no redundant phrasing, structured as: constraint definitions →
required-when-enabled coupling → unconditional-field-level rationale.
Per AGENTS.md 'Documentation conventions' (concision, structure).
* fix(config): emit per-field error paths on auth-required refine
Address sub-agent review feedback on #1903.
src/utils/ConfigurationSchema.ts
- Replace the .refine() with a fixed path: ['username'] by a
.superRefine() that emits one issue per actually-missing field with
the correct path. Configurations supplying only 'username' now
surface the issue at 'uiServer.authentication.password' rather than
'uiServer.authentication.username', and vice versa. Configurations
missing both produce two issues, one per field.
tests/utils/ConfigurationSchema.test.ts
- 'should reject authentication enabled without username and password'
now asserts both 'uiServer.authentication.username' and
'uiServer.authentication.password' paths are present, locking the
per-field issue contract.
- 'should reject authentication enabled with username but no password'
now asserts 'uiServer.authentication.password' specifically — would
catch the prior path-misanchoring regression.
- The 'should reject unknown key in uiServer.authentication section'
regression-anchor now asserts i.code === 'unrecognized_keys' at the
'uiServer.authentication' path, locking the unknown-key intent so a
hypothetical future refine cannot satisfy the assertion by accident.
* fix(ui-common): align authenticationConfigSchema with simulator auth schema
Apply the same defense-in-depth + per-field-error-path pattern shipped
on the simulator-side UIServerAuthenticationSchema in this PR. The Web
UI client schema (consumed by ui/web/src/main.ts and transitively by
ui/cli/src/config/loader.ts) carried the same set of defects:
- password lacked min(1) at the field level
- username regex /^[^:]*$/ accepted the empty string
- a single .refine combined type-check + presence + length, producing
a generic error at the auth object root with no path
- empty creds shipped under enabled: false would become a Basic-Auth
bypass the moment enabled is flipped on
ui/common/src/config/schema.ts
- password: z.string().min(1).optional()
- username: z.string().min(1).refine(no ':' RFC 7617).optional()
(replaces the regex which silently accepted '')
- .refine(...) replaced by .superRefine(...) emitting one issue per
actually-missing field with the correct path, only when
enabled === true && type === PROTOCOL_BASIC_AUTH (existing
type-check short-circuit preserved as future-proofing).
- JSDoc mirrors the simulator-side sibling, documenting the
intentional stricter-than-runtime semantics.
ui/common/tests/config.test.ts
- 4 new tests: empty password rejected (path 'authentication.password'),
username with ':' rejected (RFC 7617 message at
'authentication.username'), password-only and username-only enabled
configs each rejected at the missing field's path.
- 2 existing tests tightened: 'missing credentials' now asserts both
username and password paths; 'empty username' now asserts the
'authentication.username' path.
- 6 redundant 'if (result.success) return' narrowing guards removed —
assert.strictEqual already narrows the result type.
118/118 ui-common tests pass, typecheck + lint clean. No production
behavior change for the only valid-config consumer ('admin'/'admin'
shape regression-anchored).
* fix(config): point accessPolicy refine error path at the actionable field
Address review feedback on #1903. The .refine() rejecting
'allowLoopbackProxy: true' with empty 'trustedProxies' attached its
issue at path ['allowLoopbackProxy'] — the trigger of the rejection,
but not the field the operator must edit. Move the path to
['trustedProxies'] so structured log scrapers and IDE jump-to-field
tooling steer the operator to the field that needs populating. The
message wording (which already names both fields) is unchanged.
Test 'should reject allowLoopbackProxy=true with empty trustedProxies'
tightened from a permissive substring check
('allowLoopbackProxy or trustedProxies') to an exact-path check on
'uiServer.accessPolicy.trustedProxies' so the path contract is locked.
A second review suggestion (gate the auth credentials-required
superRefine on type === PROTOCOL_BASIC_AUTH) was investigated and
declined: the server-side AuthenticationType enum has BASIC_AUTH AND
PROTOCOL_BASIC_AUTH (unlike the Web UI client's single-member enum),
both runtime paths require username/password
(AbstractUIServer.ts:358-381), and adding the type-gate would re-open
the silent Basic-Auth bypass on the legacy 'basic-auth' path that
this PR is closing on the modern path.
src/utils/ConfigurationSchema.ts + ui/common/src/config/schema.ts
- Drop the 'across hot-reload toggles of enabled' phrasing in the
UIServerAuthenticationSchema and authenticationConfigSchema JSDoc.
Hot-reload of uiServer.* is documented as a no-op in the PR body
(R4: Bootstrap.ts:103,161 instantiates the UI server once), so the
unconditional field-level constraints are an early-detection signal
rather than a hot-reload mitigation. The accurate rationale is that
empty placeholders cannot ship under enabled: false and become a
Basic-Auth bypass on the next boot with enabled: true.
- The asymmetry between the two superRefines (server gates only on
enabled; Web UI also gates on type === PROTOCOL_BASIC_AUTH) is
preserved and intentional — server-side AuthenticationType has
legacy BASIC_AUTH which is also credential-required at runtime
(AbstractUIServer.ts isBasicAuthEnabled / isValidBasicAuth). The
rationale is captured in this commit body and the prior 86a6d4a2 commit body for git blame discoverability.
tests/utils/ConfigurationSchema.test.ts
- RFC 7617 username-rejection test now matches the issue path with
i.path.join('.') === 'uiServer.authentication.username' (exact)
instead of String.prototype.includes (substring), aligning with
every other test in the file and with the ui-common sibling.
Jérôme Benoit [Mon, 15 Jun 2026 16:40:57 +0000 (18:40 +0200)]
fix(config): tighten Zod validation on uiServer.options.port (#1874) (#1901)
* fix(config): tighten Zod validation on uiServer.options.port (#1874)
Replace the permissive z.custom<ListenOptions> bridge under
uiServer.options with a typed object schema validating known primitive
fields, so bad transport-level values (e.g. port: "not-a-number")
fail at boot time in ConfigurationSchema.safeParse instead of later
inside node:net.Server.listen with ERR_SOCKET_BAD_PORT.
The existing accessPolicy refinement is preserved via .pipe(); unknown
keys are still passed through (.loose()) to keep the ListenOptions
extension surface (e.g. signal: AbortSignal) usable.
Tests: 10 new cases in tests/utils/ConfigurationSchema.test.ts and
1 integration assertion in tests/utils/ConfigurationValidation.test.ts
covering the structured Zod error path uiServer.options.port routed
through ConfigurationValidationError.
* docs(config): align JSDoc with composite uiServer.options schema
Address review comment on #1901: the JSDoc above
UIServerListenOptionsObjectSchema was named after the composite
UIServerListenOptionsSchema, leaving the helper described under the
wrong name and the composite undocumented. UIServerConfigurationSchema
also still referenced the now-removed permissive z.custom<ListenOptions>
bridge.
- UIServerListenOptionsObjectSchema gets its own JSDoc describing the
typed object layer (.loose() + primitive constraints).
- UIServerListenOptionsSchema gets a JSDoc describing the full chain:
object guard → accessPolicy refinement → pipe to typed object.
- UIServerConfigurationSchema JSDoc updated to reference the new
composite schema instead of the obsolete custom bridge.
Adds a describe('buildStatusNotificationPayload') block matching the
existing payloadBuilders.test.ts style. Cases (9):
OCPP 1.6
- explicit errorCode: payload carries {connectorId, status, errorCode}
- errorCode omitted: payload omits errorCode (no NoError default)
- evseId pass-through with errorCode
- ocppVersion=undefined falls through to 1.6 shape
- unsupported version (e.g. '3.0') throws via assertOCPP16OrUndefined
OCPP 2.0.x
- VERSION_201 + evseId: {connectorId, connectorStatus, evseId}, no
status / errorCode fields
- VERSION_20 without evseId: {connectorId, connectorStatus}
- errorCode option ignored (1.6-only field)
Cross-version
- same input, different ocppVersion: shapes differ in the documented
way (status vs connectorStatus, errorCode/evseId presence)
Uses OCPP enum constants directly (OCPP16ChargePointStatus,
OCPP16ChargePointErrorCode, OCPP20ConnectorStatusEnumType,
OCPPVersion) per the OCPP enumeration-naming convention. Production
code in payloadBuilders.ts is unchanged.
* test(ui-common): exercise evseId-only path for OCPP 1.6 StatusNotification
Address review comment on #1902: the prior test 'should pass evseId
through for OCPP 1.6 when provided' passed both errorCode and evseId
together, so the evseId-only OCPP 1.6 branch was never independently
exercised. The errorCode-only path is already covered by the case
'should build OCPP 1.6 payload with status and explicit errorCode',
and the combined case is trivial composition of the two via the
spread syntax in the function body.
Test renamed to 'should pass evseId through for OCPP 1.6 without
errorCode' and now only passes options.evseId, asserting errorCode is
absent from the resulting payload.
Add moduleName constant and prefix the SENT command log with
'OCPPRequestService.internalSendMessage:' to match the format of the
3 received-message logs in ChargingStation.ts (handleErrorMessage,
handleIncomingMessage, handleResponseMessage).
Jérôme Benoit [Sun, 14 Jun 2026 20:07:08 +0000 (22:07 +0200)]
fix(ui-server): post-#1891 access policy and WS upgrade hardening (#1898)
* fix(ui-server): reject ports in allowedHosts, fix host normalization edges
- ConfigurationSchema: reject allowedHosts entries that carry a port
(e.g., 'gateway.example.com:8080', '[::1]:8080'). Runtime host
matching strips ports, so a port-bearing entry was silently
equivalent to its host-only form. Operators are now forced to
spell out host-only entries at config load time, matching how
matching actually works.
- UIServerNet: normalizeHost strips a trailing dot after the colon
split rather than from the whole input, so a Host header like
'localhost.:80' now resolves to 'localhost' instead of leaving
the trailing dot intact and failing the allowlist match.
- UIServerAccessPolicy: nonEmpty treats whitespace-only forwarded
values as absent, and hasForwardedHeaders consumes nonEmpty so
the two helpers agree. A whitespace-only X-Forwarded-Proto no
longer flips forwardedHeadersPresent to true and triggers an
Ambiguous* denial.
The onSocketError listener was previously attached only after the
prologue and bad-header guards had already written their rejection
responses, and was removed before the auth-failure rejection write.
A client TCP-reset between an upgrade event and the destroy callback
could fire 'error' on the Duplex socket with no listener, which Node
escalates to an unhandled exception and crashes the process.
The listener is now attached at the top of the upgrade handler and
only removed once webSocketServer.handleUpgrade takes ownership of
the socket. All three rejection paths (bad upgrade headers, prologue
failure, auth failure) keep the listener until socket.destroy(),
neutralising the DoS vector.
- Schema: split UIServerListenOptions validation so non-object values
surface 'must be a non-array object' instead of the misleading
'accessPolicy must be configured under uiServer' message.
- Forwarded parser: unescape RFC 7230 quoted-pair sequences inside
double-quoted Forwarded parameter values.
- getForwardedClientAddress: propagate forwarded.kind === 'error' before
the trustedProxy gate, matching getForwardedProtocol and
getForwardedHost (no behavioral change today; closes a latent
asymmetry if the upstream untrusted-peer gate is reordered).
- UIServerAccessCache: document identity-based cache invalidation for
trustedProxies; reload flows must construct a new configuration.
- getForwardedProtocol / getForwardedHost: distinguish length === 0
(new InvalidForwardedProtocol / InvalidForwardedHost reasons) from
length > 1 (existing Ambiguous reasons), matching the existing
InvalidForwardedClient / AmbiguousForwardedClient pattern.
* feat(ui-server): authorize requests before authentication
* feat(ui-server): gate HTTP requests by access policy
* feat(ui-server): gate MCP requests by access policy
* feat(ui-server): gate WebSocket upgrades by access policy
* chore(docker): align UI access policy for local compose
* docs(ui): document gateway access policy
* [autofix.ci] apply automated fixes
* refactor(ui-server): type access decisions and unify request prologue
- Replace stringly-typed denial reason with UIServerAccessDenialReason enum and
DENIAL_MESSAGES rendering map; UIServerAccessDecision is a discriminated union
narrowed by 'allowed'.
- Memoize the per-request access decision via WeakMap so the policy is evaluated
once per request.
- Add runRequestPrologue template method on AbstractUIServer; rate-limit now
accounts denied requests, closing pre-auth flood amplification.
- Validate accessPolicy.trustedProxies entries as IPv4/IPv6 literals at the
schema layer; precompile the normalized set per UIServerConfiguration.
- Drop the dead httpServer.on('connect') listener in UIWebSocketServer; use
StatusCodes consistently in UIMCPServer.
- README clarifies the binary loopback-vs-non-loopback model, single-hop XFF,
TLS termination via reverse proxy only, and adds a migration note for the
default requireTlsForNonLoopback: true.
* refactor(ui-server): split access policy module and tighten its surface
- Split UIServerUtils.ts into UIServerAccessPolicy (decision, forwarded
parsing, host/origin allowlists) and UIServerNet (IP/host normalization,
loopback, header tokenization). UIServerUtils retains auth tokens and WS
subprotocol negotiation only.
- Make evaluateUIServerAccess private; tests and runtime callers go through
resolveUIServerAccess so the decision cache is always exercised.
- Move accessDecisionCache and trustedProxiesCache from module scope to a
per-AbstractUIServer instance cache injected into resolveUIServerAccess.
- Quote-aware splitHeaderList honors RFC 7239 / RFC 7230 double-quoted
values; commas inside "…" are preserved.
- Replace the {error?, value?} dialects in forwarded-header parsers with a
discriminated ParseOutcome<T>; close the Forwarded params type to
by/for/host/proto.
- Drop the unreachable isDirectTLSRequest helper and its TLSSocket import.
- Set HTTP server requestTimeout=30s and headersTimeout=5s on
AbstractUIServer to bound slow-loris exposure on rejected upgrades.
- Ensure UIMCPServer.handleMcpRequest releases the transport and the MCP
server in every error branch (idempotent cleanup).
- Promote MockUpgradeSocket to UIServerTestUtils.ts; replace the duplicate
test factory createAccessPolicyConfiguration with the canonical
createMockUIServerConfiguration.
- Cover the Docker dual-stack case where an IPv4-mapped IPv6 remote address
matches an IPv4 trustedProxies entry.
- README accessPolicy cell switches to per-field sub-bullets, aligned with
the log and worker rows.
Signed-off-by: Jérôme Benoit <jerome.benoit@sap.com>
* refactor(ui-server): drop @file headers from new policy modules
@file JSDoc is the test-suite convention (TEST_STYLE_GUIDE.md) and is
absent from the rest of src/. Align the new UIServerAccessPolicy and
UIServerNet modules with the existing source-tree style.
- Drop README migration note and the duplicate access policy paragraph;
the configuration table cell already specifies the policy.
- Tighten the Docker section access policy paragraph.
- Trim narrative meta-commentary in UIServerAccessPolicy and UIServerNet
JSDoc; let constant and type names carry the rest.
- isHostAllowed: when the immediate peer is a trusted proxy and
X-Forwarded-Host is present, match it (not the immediate Host) against
allowedHosts, so reverse proxies that rewrite Host to an internal
upstream name are not rejected.
- isOriginAllowed: compare allowedOrigins via parsed URL protocol+host
rather than literal string equality, so entries with a trailing slash or
with a protocol-default port match the canonical browser-emitted Origin.
- UIMCPServer: destroy the response and request sockets after a denied
prologue, mirroring UIHttpServer.
- evaluateUIServerAccess: parse the Forwarded header once and pass the
outcome to getForwardedProtocol and getForwardedClientAddress.
- hasDuplicateHeaders: count rawHeaders in a single pass.
Signed-off-by: Jérôme Benoit <jerome.benoit@sap.com>
* fix(ui-server): include prologue headers on WebSocket upgrade denial
The WebSocket upgrade denial path wrote only the HTTP/1.1 status line, so
rate-limit denials reached clients without the Retry-After header that
the HTTP and MCP transports already emit.
- UIMCPServer: destroy the response and request sockets after a denied
authentication, mirroring UIHttpServer.
- UIWebSocketServer: include the WWW-Authenticate header on auth-denied
upgrades so RFC 7235 clients receive the Basic challenge that the HTTP
and MCP transports already emit.
Signed-off-by: Jérôme Benoit <jerome.benoit@sap.com>
* fix(ui-server): close denied connections cleanly and harden allowedOrigins
- AbstractUIServer: also call setTimeout() on the http server so the
HTTP/2 transport (Http2Server lacks requestTimeout/headersTimeout) gets
the same idle bound.
- UIHttpServer / UIMCPServer: emit Connection: close on prologue and auth
denials and drop the explicit res.destroy()/req.destroy() calls; the
body now flushes before the connection closes.
- UIWebSocketServer: destroy the upgrade socket from the write callback
so the response bytes are flushed before teardown.
- UIServerAccessPolicySchema: refine allowedOrigins entries to reject
paths, query strings, and fragments at config load instead of silently
ignoring them at runtime.
- UIServerAccessPolicy: parse the Forwarded host parameter symmetrically
with for and proto. Both Forwarded host=... and X-Forwarded-Host now
feed isHostAllowed when the immediate peer is trusted, with a new
AmbiguousForwardedHost denial reason when they conflict.
- UIWebSocketServer: build upgrade rejection responses through a single
helper that always emits Connection: close and Content-Length: 0,
matching the HTTP and MCP transports.
Signed-off-by: Jérôme Benoit <jerome.benoit@sap.com>
* fix(test): wait on response finish event in gzip tests
Replace the fixed 50 ms waitForStreamFlush delay with await once(res,
'finish') in the UIHttpServer gzip tests. MockServerResponse already
emits 'finish' from end(), so the wait is now event-driven and stops
flaking on slow CI runners (notably Windows / Node 24.x).
Drop the now-unused waitForStreamFlush helper and GZIP_STREAM_FLUSH_DELAY_MS
constant.
Signed-off-by: Jérôme Benoit <jerome.benoit@sap.com>
* fix(ui-server): scope HTTP timeouts to slow-loris and tighten Forwarded parser
- AbstractUIServer: drop the connection-wide setTimeout call. requestTimeout
and headersTimeout already cover slow-loris on HTTP/1.1, and HTTP/2 streams
are per-request and not vulnerable to the same pattern. The dropped 30 s
socket-inactivity cap was killing legitimate long-running responses on
the deprecated HTTP transport.
- UIServerAccessPolicy: split Forwarded pairs with quote awareness so that
RFC 7239 quoted-string values containing semicolons are not truncated.
Mirrors the quote handling already in splitHeaderList.
- UIWebSocketServer: place Connection: close after the extraHeaders spread
in buildUpgradeRejectionResponse to match the HTTP and MCP transports
and keep the close header non-overridable by callers.
Signed-off-by: Jérôme Benoit <jerome.benoit@sap.com>
* fix(ui-server): omit Connection header on HTTP/2 and prioritize trusted-peer denial reason
- AbstractUIServer: add getConnectionCloseHeader() returning an empty
header set when the underlying server is HTTP/2. HTTP/2 forbids the
Connection header as a connection-specific header; Node otherwise emits
an UnsupportedWarning and silently drops the value. Streams are closed
by res.end() on that path. HTTP/1.1 keeps the explicit Connection: close
to terminate keep-alive on denial.
- UIHttpServer, UIMCPServer: route prologue-denial and auth-denial
responses through the helper instead of hardcoding Connection: close.
- UIServerAccessPolicy: hoist the trusted-peer check above the protocol
and host ambiguity checks. An untrusted peer sending forwarded headers
is now denied as ForwardedFromUntrustedPeer regardless of duplicate
proto/host metadata, so operator logs reflect the trust violation
rather than a parser-level symptom. Duplicate-headers retains absolute
priority. Tests added for the protocol- and host-ambiguity paths from
an untrusted peer.
Empty forwarded-header values were treated as present-with-empty-string,
which caused two misbehaviors when the immediate peer was a trusted proxy:
- An empty 'host=' parameter inside a Forwarded header (or an empty
X-Forwarded-Host) shadowed the real Host header. The policy resolved
the host to '' and denied the request as HostNotAllowed even though
the actual Host was in allowedHosts.
- An empty X-Forwarded-Proto was rejected as AmbiguousForwardedProtocol
(via splitHeaderList returning an empty array) instead of being
treated as missing.
A nonEmpty() helper now normalizes empty/undefined header values to
undefined at parse time. parseSingleForwardedHeader skips empty
parameter values rather than recording them. The two getters fall
through to the next source when the upstream value is empty.
Tests cover the host fallback paths (Forwarded host= and
X-Forwarded-Host) and the protocol absent-vs-ambiguous distinction.
Signed-off-by: Jérôme Benoit <jerome.benoit@sap.com>
* fix(ui-server): accept RFC 7239 hidden-identity forms and tighten Forwarded parser
- UIServerAccessPolicy: recognize the RFC 7239 nodename forms 'unknown'
and obfuscated identifiers ('_' followed by token characters) in the
Forwarded 'for=' parameter and X-Forwarded-For header. Hidden-identity
forms now resolve to ABSENT, so the access decision falls back to the
trusted proxy's own remote address as the client identifier (used as
the rate-limit bucket and the logged identity). Previously these
values were rejected as InvalidForwardedClient even though the rest
of the request (TLS, host, origin, trust) was valid.
- UIServerAccessPolicy: strip Forwarded parameter quotes only when the
pair is balanced. The previous '/^"|"$/g' replace stripped the
leading or trailing quote independently and silently accepted
unbalanced inputs such as 'for="203.0.113.10'. The new
'/^"(.*)"$/' anchors both ends in a single match.
- README: clarify that allowLoopbackProxy requires the loopback peer
to also be listed in trustedProxies. allowLoopbackProxy alone has
no effect because the trusted-peer check denies forwarded headers
from peers absent from the trustedProxies list.
- Tests: positive path for allowLoopbackProxy=true; identity-hidden
paths for Forwarded for=unknown, Forwarded for=_obfuscated, and
X-Forwarded-For: unknown.
Signed-off-by: Jérôme Benoit <jerome.benoit@sap.com>
* fix(ui-server): normalize empty X-Forwarded-For and align MCP auth with continuation pattern
- UIServerAccessPolicy: normalize an empty X-Forwarded-For header to
undefined so the access decision falls back to the trusted proxy's own
remote address rather than denying the request as InvalidForwardedClient
or as AmbiguousForwardedClient when a Forwarded for= parameter is also
present. Restores symmetry with X-Forwarded-Host and X-Forwarded-Proto
empty-value handling.
- UIMCPServer: rewrite the auth path as a callback continuation. The
previous capture-then-check pattern relied on authenticate() being
synchronous and on the last next() call winning. The new shape
matches UIHttpServer and UIWebSocketServer and continues to handle
MCP requests only after a successful authentication callback.
Signed-off-by: Jérôme Benoit <jerome.benoit@sap.com>
* fix(ui-server): align MCP error responses and treat empty forwarded headers as absent
- UIMCPServer: emit Connection: close on the 404 not-found and on the
centralized sendErrorResponse helper (BAD_REQUEST, METHOD_NOT_ALLOWED,
INTERNAL_SERVER_ERROR), aligning these paths with the post-prologue
and auth denial responses. The connection-close header is omitted on
HTTP/2 by the existing getConnectionCloseHeader() helper.
- UIServerAccessPolicy: hasForwardedHeaders now ignores empty header
values, restoring symmetry with the downstream forwarded-header
parsers that already map empty values to absent. Empty X-Forwarded-*
headers from an untrusted peer no longer trigger
ForwardedFromUntrustedPeer; an empty X-Forwarded-Proto from a
non-loopback untrusted peer now denies as TlsRequired (no proxy
proof of TLS). Empty headers from a loopback peer no longer block
the request.
- README: clarify that requireTlsForNonLoopback honors X-Forwarded-Proto
/ Forwarded: proto= from a trusted proxy and does not detect native
direct TLS; note that a compromised trustedProxies entry can bypass
per-client rate limiting by varying X-Forwarded-For.
Signed-off-by: Jérôme Benoit <jerome.benoit@sap.com>
* chore(ui-server): trim redundant access-policy comments and JSDoc
- Drop module-level JSDoc on UIServerAccessPolicy and UIServerNet to
match the codebase convention (no module-level JSDoc on peer files).
- Drop redundant inline comments whose content is already documented in
the README or self-evident from the surrounding code: the empty
X-Forwarded-For malformed branch, the allowedOrigins fallback, and
the loopback-aliases derivation.
- Trim the verbose 'Discriminated by ...' prose on the UIServerAccessDecision
and ParseOutcome types and remove the JSDoc on resolveUIServerAccess
(function name and signature are self-explanatory; memoization is
visible from the cache lookup).
- UIServerNet: factor the quote-aware splitter into splitQuoted(value,
delimiter); splitHeaderList becomes a thin wrapper. The previously
duplicated state machine in UIServerAccessPolicy (splitForwardedPairs)
is replaced by a direct splitQuoted call on ';'.
- UIServerAccessPolicy: factor the X-Forwarded-* / Forwarded ambiguity
pick into pickForwardedValue. The three getForwarded* helpers now
share a single source of truth for the present/absent/ambiguous
decision; transport-specific tail logic (multi-hop X-Forwarded-For
rejection, X-Forwarded-Proto multi-value rejection, lowercasing)
stays local.
- UIServerAccessPolicy: drop the unreachable string-trim fallback in
normalizeIPAddress (every input that fails normalizeHost also fails
the downstream IP checks), narrow isSecureForwardedProtocol's
parameter to string | undefined (no caller passes null), and skip
the redundant pre-pass through normalizeHost in isSameHost (the
IP-address branch already calls normalizeHost internally).
- UIServerAccessPolicy: replace Reflect.get with direct property
access on IncomingMessage.headersDistinct and rawHeaders (typed
members on Node 18+).
- Tests: extend the createMockIncomingMessage and the policy test's
createAccessPolicyRequest factories with headersDistinct and
rawHeaders defaults so direct property access works against the
mocks.
Signed-off-by: Jérôme Benoit <jerome.benoit@sap.com>
* refactor(ui-server): unify denial rendering and switch authenticate to boolean
- AbstractUIServer: introduce renderDenial(res, payload) and
getUnauthorizedDenial() so HTTP and MCP route every prologue, 401,
and post-handler error response through the same code path. The
helper centralizes Content-Type, Connection-close (HTTP/2-aware),
and 'res.headersSent' guarding.
- AbstractUIServer: change authenticate(req) to return a boolean
instead of invoking a continuation with an Error. The CPS shape was
fully synchronous in practice — every caller did
'if (err != null)' — and the BaseError allocation was dead. The
three transports collapse to 'if (!this.authenticate(req)) { … }'.
- AbstractUIServer + UIServerUtils: replace hand-typed reason phrases
with getReasonPhrase() from http-status-codes; the helper functions
isValidBasicAuth / isValidProtocolBasicAuth and
getUsernameAndPasswordFromAuthorizationToken drop their continuation
parameter accordingly.
- UIServerSecurity: replace the chunk-counter createBodySizeLimiter()
with a shared readLimitedBody(req, maxBytes): Promise<Buffer> helper
and a typed PayloadTooLargeError. UIHttpServer reads the request
body via the helper instead of an event-based loop, and UIMCPServer
detects oversized payloads via 'instanceof PayloadTooLargeError'
rather than string-matching the error message.
- UIMCPServer: route sendErrorResponse through renderDenial so all
4xx/5xx error responses share the same headers and Connection
semantics; preserve the path-filter-before-authenticate ordering
with a comment so unknown paths return 404 without revealing
whether authentication would have succeeded.
- UIWebSocketServer: use getUnauthorizedDenial() for the upgrade
rejection on auth failure instead of inlining the WWW-Authenticate
string and the 'Unauthorized' literal.
- Tests: align UIServerUtils.test, UIServerSecurity.test, and
UIMCPServer.test with the new APIs (boolean signatures, new
PayloadTooLargeError, new readLimitedBody).
- ConfigurationSchema: introduce UI_SERVER_ACCESS_POLICY_DEFAULTS as the
single source of truth for the access-policy field defaults
(requireTlsForNonLoopback=true, allowLoopbackProxy=false, and the
empty allowlists). The defaults previously lived inline in
evaluateUIServerAccess.
- UIServerAccessPolicy: consume UI_SERVER_ACCESS_POLICY_DEFAULTS instead
of the hardcoded literals so a default change propagates uniformly
across the policy code, the schema, and the README.
- ConfigurationSchema: validate allowedHosts entries via normalizeHost
so paths, queries, fragments, and other malformed Host values are
rejected at config load. The previous z.string().min(1) accepted
any non-empty string.
Signed-off-by: Jérôme Benoit <jerome.benoit@sap.com>
* test(ui-server): factor access-policy test fixtures and address constants
- UIServerTestUtils: introduce TRUSTED_PROXY_IP, EXTERNAL_CLIENT_IP, and
GATEWAY_HOST constants from RFC 5737 / RFC 3849 reserved ranges;
factor the recurrent gateway access-policy configurations into
createGatewayConfigWithTrustedProxy() and
createGatewayConfigWithoutTrustedProxies() so subsequent test
additions inherit a single source of truth for the gateway shape.
- UIServerAccessPolicy.test: cover the previously untested denial
reasons AmbiguousForwardedProtocol (multi-value X-Forwarded-Proto),
AmbiguousForwardedHeader (multi-entry Forwarded), and
AmbiguousForwardedParameter (duplicate parameter inside one entry);
cover the InvalidForwardedClient branch for non-IP, non-hidden 'for='
values; cover AmbiguousForwardedClient when 'Forwarded: for=unknown'
collides with X-Forwarded-For; cover the headersDistinct path of
hasDuplicateHeaders.
- UIServerAccessPolicy.test: cover the malformed Origin URL branch in
isOriginAllowed and the allowedHosts-fallback branch when
allowedOrigins is empty; pin the design choices for whitespace-padded
Host normalization, uppercase X-Forwarded-Proto values, IPv4-mapped
IPv6 loopback proxies under allowLoopbackProxy, and the policy's
intentional non-detection of req.socket.encrypted without forwarded
protocol headers.
- UIServerAccessPolicy.test: tighten the previously weak assertions on
the accessPolicy-undefined paths (now check the denial reason and
the resolved client address); rename the misleading
'should reject empty-but-present X-Forwarded-For' test to reflect
what it actually exercises (no parseable addresses); replace the
trivial cache-isolation assertion with a real
cacheA.decisions.has(req) / cacheB.decisions.has(req) check.
- UIMCPServer.test: drop the two malformed-host tests that re-tested
policy logic already covered by UIServerAccessPolicy.test; the
Connection: close tests in the same describe still anchor the
transport-level rendering contract.
Signed-off-by: Jérôme Benoit <jerome.benoit@sap.com>
* test(ui-server): close coverage gaps and tighten weak assertions identified in second-pass audit
- ConfigurationSchema.test: validate the allowedHosts schema refine
rejects malformed entries (excess colons, non-numeric port, port out
of range, empty input) and accepts canonical hostnames and IP
literals; lock the canonical UI_SERVER_ACCESS_POLICY_DEFAULTS map
shape so a default change cannot drift silently.
- UIServerNet.test: cover normalizeHost directly (rejection branch is
the boundary of the allowedHosts schema refine; previously only
exercised indirectly through transport tests).
- UIServerSecurity.test: assert readLimitedBody propagates upstream
stream errors (security-relevant fail-closed behavior).
- UIServerAccessPolicy.test: rename the misleading
for=unknown collision test to reflect the actual code path
(collision is generic, not unknown-specific); drop the duplicate
encrypted-proxy plaintext rejection (the encrypted flag is ignored
by the policy by design and a dedicated test already pins it);
tighten the whitespace-padded Host test to use an explicit
allowedHosts list so the trim path is genuinely exercised rather
than the loopback alias fallback.
- UIServerTestUtils: drop the dead EXTERNAL_CLIENT_IP export and
correct the RFC reference comment (RFC 5737 IPv4 / RFC 2606
example.com — RFC 3849 covers IPv6 and is irrelevant here).
- UIWebSocketServer: add a brief comment documenting why
buildUpgradeRejectionResponse stays separate from
AbstractUIServer.renderDenial — pre-handshake WS rejections write
raw HTTP/1.1 to a Duplex socket while renderDenial targets
ServerResponse.
- src/utils/index: re-export UI_SERVER_ACCESS_POLICY_DEFAULTS so the
defaults can be asserted from the schema test layer.
- UIServerAccessPolicy: getForwardedHost rejects multi-value
X-Forwarded-Host as AmbiguousForwardedHost, mirroring the symmetric
guards in getForwardedProtocol and getForwardedClientAddress. The PR
description's claim that 'multi-value X-Forwarded-* are rejected'
now applies uniformly across the three forwarded dimensions.
- UIServerNet: normalizeHost validates the hostname charset against a
conservative [a-z0-9._-]+ allowlist on the dot path. Inputs with
commas, spaces, or unbalanced brackets are rejected at config-load
time when used as allowedHosts entries, and at runtime as a
defense-in-depth check on the Host header.
- UIServerNet: isValidPort rejects port 0 (RFC 6335 reserved). Port 0
in a client-side Host header has no useful meaning; tightening to
1..65535 matches common practice and the prior config-bind path is
unaffected.
- README: requireTlsForNonLoopback wording corrected — non-loopback
requests without forwarded protocol headers are denied as
tls-required, not merely 'not detected', even when the underlying
socket is encrypted.
- UIServerAccessPolicy: parseSingleForwardedHeader normalises an empty
Forwarded header value (e.g., 'Forwarded: ') to ABSENT instead of
rejecting it as AmbiguousForwardedHeader. The PR description claim
that 'empty forwarded values are treated as absent' now applies to
the header value itself, not only to parameters inside the header.
- UIMCPServer: sendErrorResponse accepts an optional headers argument
so 405 Method Not Allowed responses advertise 'Allow: GET, POST,
DELETE' per RFC 9110 §15.5.6.
- AbstractUIServer: warn at startup when the UI server is bound to a
wildcard host ('', '0.0.0.0', '::') with an empty
accessPolicy.allowedHosts; the previously silent lockout (every
request denied as host-not-allowed) now produces an actionable log
line pointing at the configuration to adjust.
- AbstractUIServer: warnIfMisconfigured now also fires when the UI
server is bound to a non-loopback specific host with
requireTlsForNonLoopback=true and no accessPolicy.trustedProxies;
the previously silent path (every plaintext request denied as
tls-required) now produces an actionable startup log line. Wildcard
binding with empty allowedHosts retains its dedicated warning.
- AbstractUIServer: rate-limit denials now emit a warn log symmetric
to the access-policy 403 path so operators can detect probing.
Previously the 429 branch returned silently, which made abusive
clients invisible from the logs.
- README: drop the 'even when the underlying socket is encrypted'
caveat from the requireTlsForNonLoopback description; the simulator
only listens via plaintext or h2c, so the scenario described was
unreachable. The runtime behaviour is unchanged and the test that
pins the design choice stays as defense in depth.
Jérôme Benoit [Sun, 14 Jun 2026 01:29:02 +0000 (03:29 +0200)]
fix(ui-web): skip dev-only diagnostics under Vitest (#1897)
Extract isDev() helper in core/env.ts that returns true only in real
DEV (excludes production build and Vitest). Migrate the 7 existing
import.meta.env.DEV check sites to the helper.
Primary motivation: validateTokenContract schedules a
requestAnimationFrame whose body calls console.warn for missing CSS
tokens. jsdom does not resolve --color-* tokens, so every theme/skin
switch under Vitest 4 queued a console.warn that fired after
environment teardown — surfacing as EnvironmentTeardownError on
Linux/Node 22; passes on macOS/Node 24.
Side effect: silences console.debug noise from storage.ts,
providers.ts, ToggleButton.vue under Vitest. dev/build behaviour
unchanged.
Adds regression test in tests/unit/shared/tokens/contract.test.ts.
Jérôme Benoit [Sat, 13 Jun 2026 23:46:21 +0000 (01:46 +0200)]
test(ui-web): drain dynamic imports globally for async components (#1893)
Add vi.dynamicImportSettled() to the global afterEach hook so pending
dynamic imports (defineAsyncComponent loaders, lazy routes) settle
before Vitest tears down the test environment. Prevents
EnvironmentTeardownError on transitive .vue import chains observed
under filtered runs (e.g. -t "should open authorize dialog").
Covers App.vue, ModernLayout.vue, and any future component using
defineAsyncComponent or lazy-loaded routes.
Jérôme Benoit [Thu, 4 Jun 2026 17:17:35 +0000 (19:17 +0200)]
test(ui-web): mock useTheme/useSkin in SimulatorBar.test.ts to fix vitest 4.x teardown RPC leak (#1884)
* test(ui-web): mock useTheme/useSkin in SimulatorBar.test.ts to fix vitest 4.x teardown leak
The two final tests ('should call switchTheme/switchSkin when … select changes')
mounted SimulatorBar with the real useTheme/useSkin composables. Both code paths
schedule console.warn output AFTER the synchronous test body resolves:
1. validateTokenContract() (src/shared/tokens/contract.ts) wraps work in
requestAnimationFrame and warns when CSS variables are missing. jsdom does
not resolve --color-* CSS variables, so the contract check logs ~24 warnings
via rAF on every theme/skin switch.
2. switchSkin() returns Promise<boolean>, but the @change handler discards the
promise and trigger('change') only awaits Vue's nextTick. The unawaited
loadStyles() dynamic import resolves later and may also call console.warn.
Vitest 4.x tightened teardown to fail rather than swallow pending RPC calls,
surfacing this latent bug as:
EnvironmentTeardownError: [vitest-worker]: Closing rpc while
"onUserConsoleLog" was pending
Reproducibly fails on Linux/Node 22 in CI; passes on macOS/Node 24 because rAF
+ dynamic-import timing differs.
Fix mirrors the existing precedent in tests/unit/router.test.ts: vi.mock both
composables at the top of the test file, returning shapes that match the real
contract (THEME_IDS / SKIN_IDS imported from ui-common rather than hand-rolled
subsets). The mocked switchSkin resolves immediately so no teardown leak occurs.
Also tightens the two affected tests to actually assert what their names claim
(switchTheme/switchSkin called with the selected value), instead of merely
checking the <select> exists.
* test(ui-web): use THEME_IDS/SKIN_IDS indices instead of hardcoded values
Address PR review: hardcoding 'dracula' and 'classic' as the target select
values would silently break the assertions if THEME_IDS or SKIN_IDS are
reordered or renamed in ui-common (the DOM $lt;select$gt; would keep its
current option, switchTheme/switchSkin would be called with that stale
value, and the toHaveBeenCalledWith assertion would compare against a
constant that no longer matches the dispatched event).
Pick targets that are guaranteed to differ from the mocked active values:
THEME_IDS[1] (≠ activeThemeId = THEME_IDS[0]) and the first SKIN_IDS entry
that is not 'modern' (≠ activeSkinId). The assertions then reference the
same constants, keeping the test coupled to the actual contract surface.
Note: the third reviewer comment about adding beforeEach(mockClear) is not
needed — vitest.config.ts already sets clearMocks: true and
restoreMocks: true, which run before each test.
* Revert "test(ui-web): use THEME_IDS/SKIN_IDS indices instead of hardcoded values"
* test(ui-web): add explicit afterEach mock cleanup in SimulatorBar test
Aligns SimulatorBar.test.ts with the dominant convention in ui/web's test
suite (Dialogs, StationCard, ConnectorRow, ClassicLayout, Actions, …),
which all add an explicit `afterEach(() => { vi.clearAllMocks() })`
alongside vitest.config.ts's global `clearMocks: true` / `restoreMocks: true`.
Belt-and-braces: the explicit reset keeps mock lifecycle visible next to
the mock declarations, even though it is technically redundant with the
global flags.
Jérôme Benoit [Tue, 2 Jun 2026 19:53:05 +0000 (21:53 +0200)]
chore(release-please): remove release-as override after 4.8.0 release
The 4.8.0 override has shipped via PR #1880 (merge commit 44ab6cf8).
Remove the 'release-as' field so that subsequent release-please runs
resume normal Conventional Commits version computation.
Without this cleanup, subsequent runs would keep proposing 4.8.0,
producing no-op release PRs or duplicate-tag errors.
Jérôme Benoit [Tue, 2 Jun 2026 19:48:39 +0000 (21:48 +0200)]
chore(release-please): override next release to 4.8.0
The BREAKING CHANGE footer in 8bb806d describes a log message wording
change consumed by external monitors, not a SemVer-breaking API change.
This override forces release-please to ship 4.8.0 (minor) instead of 5.0.0.
Cleanup: this 'release-as' field MUST be removed in a follow-up commit
once the 4.8.0 release PR merges, otherwise subsequent runs will
re-propose 4.8.0.
Jérôme Benoit [Wed, 27 May 2026 22:33:50 +0000 (00:33 +0200)]
refactor: relocate simulator config to src/utils and enforce barrel discipline
- Move ConfigurationSchema/Migrations/Validation from charging-station to utils
- Rename ConfigurationUtils.logPrefix to configurationLogPrefix (barrel anti-collision)
- Expose public APIs through component barrels (charging-station, ocpp, utils, worker)
- Migrate 38 test imports from deep paths to barrels
- Document TDZ-cycle exceptions in OCPPError and ConfigurationMigrations
* feat(config): scaffold simulator configuration schema and validator skeleton
- Add ConfigurationMigrations.ts: CURRENT_CONFIGURATION_SCHEMA_VERSION=1,
coerceConfigurationVersion, applyConfigurationMigration, migrateV0ToV1
with DEPRECATED_KEY_REMAPPINGS (~25 legacy keys)
- Add ConfigurationSchema.ts: strict Zod v4 schema for all config sections
(log, worker, performanceStorage, uiServer, stationTemplateUrls) with
deprecated keys as .optional()
- Add ConfigurationValidation.ts: validateConfiguration pipeline
(guard→clone→coerce→migrate→safeParse→transform) + ConfigurationValidationError
extends BaseError with structured fieldErrors
- Add $schemaVersion: 1 to config-template.json
- Add ConfigurationFixtures.ts test helpers
- Add ConfigurationSchema.test.ts (63 tests), ConfigurationMigrations.test.ts (44 tests)
- Wire validateConfiguration into Configuration.getConfigurationData() with
hard-throw at boot (console.error+chalk+process.exit(1))
- Hot-reload snapshot rollback: pre-clear snapshot, restore on failure
- Replace ConfigurationData interface with z.infer<typeof ConfigurationSchema>
- Migrate all consumers atomically (SimulatorState, Configuration, types barrel)
- Delete checkWorkerProcessType and checkWorkerElementsPerWorker (subsumed by schema)
- Delete ConfigurationMigration.ts (logic moved to ConfigurationMigrations.ts)
- Add ConfigurationValidation.test.ts (39 tests), Configuration-hot-reload.test.ts
- Add ConfigurationValidation-perf.test.ts (p99 < 50ms budget)
- Add error message snapshot test
- Update README.md with $schemaVersion documentation
* feat(config): remove legacy ConfigurationMigration.ts and stale call sites
- Delete src/utils/ConfigurationMigration.ts (deprecated-key logic now
owned by ConfigurationMigrations.ts v0→v1 migration step)
- Remove checkDeprecatedConfigurationKeys import and call site from
Configuration.getStationTemplateUrls() — validation pipeline in
getConfigurationData() already handles deprecated key remapping
* chore(config): silence lint warnings on configuration migrations
- Add JSDoc descriptions for setAtPath() params
- Add JSDoc descriptions and @returns for migrateV0ToV1
- Whitelist 'emerg' (syslog level) and 'REMAPPINGS' in cspell dictionary
- Extract deprecated-key sweep into pure remapDeprecatedKeys() that
reports warnings and field errors via return value instead of side
effects. Runs unconditionally regardless of $schemaVersion so v1
configs still containing deprecated keys never silently drop user
values (B3).
- Replace silent-drop setAtPath with collision- and intermediate-aware
variant: equal-value writes are idempotent no-ops, unequal values
produce a typed field error (B4), non-object intermediates are
reported and stop traversal instead of overwriting user data (N7).
- Type DEPRECATED_KEY_REMAPPINGS as Record<string, string | null>;
null marks deprecated keys with no canonical destination, replacing
the autoReconnectMaxRetries self-mapping that silently dropped the
user value (B2). Add 'worker.elementStartDelay' as a dotted source
key so nested deprecations live in the same single source of truth.
- Refactor ConfigurationValidationError: primary constructor takes
FieldError[] + a context with explicit phase ('migration' | 'schema');
static fromZodError() factory wraps Zod failures. Error messages now
carry the phase in their tag for clearer diagnostics.
- Switch deprecation-warning channel from logger.warn to console.warn
to break a re-entrant boot path where the Logger proxy lazily
resolved Configuration.getConfigurationSection('log'), recursing into
the validation pipeline (B1). transformConfiguration is now a pure
deep clone — its previous warning loop moved upstream to the sweep.
- Delete dead post-migration remapping in Configuration.ts:
deprecatedLogKeyMap, deprecatedWorkerKeyMap, the
delete configurationData.workerPoolStrategy mutation, the cast hiding
the legacy 'supervisionURLs' read, and the worker.elementStartDelay
fallback (B6). All deprecation handling now flows through the
migration's single source of truth.
- Make hot-reload reload loop async end-to-end. Awaits the change
callback inside the same lock so subsequent reloads cannot interleave
with an in-flight callback. Adds configurationFileReloadPending so
events arriving during a reload coalesce into exactly one drain
reload after the current one completes (N8).
- Polish schema: use z.enum(NativeEnum) directly for ApplicationProtocol
and ApplicationProtocolVersion to preserve literal-type narrowing on
UIServerConfiguration (N4); drop the BaseConfigurationSchema alias
(N5).
* test(config): cover migration edge cases and hot-reload rollback
Tests aligned with the rebuilt validation pipeline:
- Split migration tests into a remapDeprecatedKeys block (per-key sweep,
null-destination removal for autoReconnectMaxRetries, equal-value
collision idempotency, unequal-value collision field error, non-object
intermediate field error, nested worker.elementStartDelay) and a lean
applyConfigurationMigration block (version-bump only, immutability).
- Replace the empty self-mapping branch with explicit hasOwnProperty
removal assertions to defeat the previously vacuous test.
- ConfigurationValidation tests: switch deprecation-warning channel
spies from logger.warn to console.warn (B1 regression also asserts
logger.warn is never called from the pipeline), tolerate
schema-incompatible remap targets so the warning fires before the
downstream throw, add B3 sweep / B6 SSOT / future-version pipeline /
ConfigurationValidationError shape (FieldError[] + phase, fromZodError
factory, schema-phase aggregation) tests, and update the error-message
snapshot to include the new [phase] tag.
- transformConfiguration immutability test verifies a fresh validation
is unaffected by mutating a prior return value.
- ConfigurationSchema tests: strict parity for performanceStorage and
uiServer.authentication; StationTemplateUrl entry constraints
(empty file, negative numberOfStations, deprecated numberOfStation,
unknown key); worker.elementsPerWorker and poolMaxSize/MinSize
positive-integer constraints; log.statisticsInterval non-negative
integer constraints; bidirectional schema/DEPRECATED_KEY_REMAPPINGS
sync meta-test that walks both top-level and nested @deprecated
describe markers.
- Hot-reload tests rebuilt around the async runReloadLoop: validation
failure asserts the logged error is a ConfigurationValidationError;
JSON parse error path asserts configurationData and section cache are
fully restored (Gap 7); a sentinel watcher survives a failed reload;
N8 rapid double-save coalesces into exactly one drain reload reflecting
the latest content; flag reset is exercised on both paths.
- Configuration validation perf test: relative p99 budget (20× median
with a 1ms floor) plus an absolute 500ms catastrophic ceiling,
replacing the flaky absolute 50ms threshold (N1).
- Fixtures: new buildInvalidJsonString, buildV1WithDeprecatedKey
(handles top-level and dotted source keys), and
buildV0WithDeprecatedKeyCollision builders for the new test cases.
* docs(config): trim verbose JSDocs and harmonize with Template* conventions
Polish pass after PR audit: bring comments and docstrings in line with
the existing TemplateMigrations / TemplateValidation / TemplateSchema
patterns, drop ceremonial AI-generated narration, and fix two precision
defects.
- ConfigurationMigrations.ts: trim getAtPath / setAtPath / remapDeprecatedKeys /
migrateV0ToV1 / applyConfigurationMigration JSDocs to match Template*
density; drop the historical "now handled unconditionally upstream"
paragraph (AGENTS.md "exclude historical evolution").
- ConfigurationValidation.ts: collapse the duplicated pipeline narration
(the function-level JSDoc already enumerates stages, the inline
// Stage N — markers were removing them); shorten the
ConfigurationValidationError class docstring; tighten transformConfiguration
JSDoc and drop the aspirational "future cross-field invariants" sentence;
collapse the 5-line re-entrancy comment to a 2-line rationale.
- ConfigurationSchema.ts: fix two precision defects in JSDoc — stale
module name (ConfigurationMigration → ConfigurationMigrations) and wrong
subject (numberOfStation is not in the remap table; nothing auto-migrates
it). Disambiguate WorkerConfigurationSchema description (resourceLimits
is bridged, not deprecated; elementStartDelay is the deprecated alias).
- Configuration.ts: shrink runReloadLoop and performReload JSDocs to two-
line summaries matching the surrounding private-method conventions in
the same file; collapse the 4-line ESLint-disable rationale to a single
-- suffix on the disable directive.
- Test fixtures and headers: drop internal review codes (B1/B3/B4/B6/N7/N8/
Gap-7/RG-4) from JSDocs and test names — they had no lookup table and
would be opaque to future maintainers; trim test-file headers to the
one-line Template* shape.
* refactor(config): address post-audit findings on validation pipeline
* fix(config): drop incompatible v0 remaps and tighten validation invariants
Addresses 4 PR review findings cross-validated by 2 oracles.
- ConfigurationMigrations: drop `useWorkerPool` (boolean→enum),
`distributeStationToTenantEqually` and `distributeStationsToTenantsEqually`
(boolean→enum), and `uiWebSocketServer` (legacy shape→strict object) to
`null` (warn-and-delete) — same pattern as `workerPoolStrategy` and
`autoReconnectMaxRetries`. v0 configurations carrying any of these
legacy keys would otherwise auto-write a value the strict schema
rejects, killing the simulator at boot. Migration now warns and asks
the user to set the canonical key explicitly. Fixes the BLOCKING
finding from the PR review (regression on 'v0 configs remain valid').
- ConfigurationMigrations: remove redundant `out.$schemaVersion = 1`
from `migrateV0ToV1`. The `applyConfigurationMigration` loop is the
single owner of version stamping; per-step writes are silently
overwritten and become misleading once a second migration is added.
- ConfigurationSchema: tighten `StationTemplateUrlSchema.numberOfStations`
from `.nonnegative()` to `.positive()` — `0` was schema-valid but
semantically meaningless (a template entry that spawns no station)
and conflicts with `isValidNumberOfStations()` which rejects `<= 0`.
Deprecated `numberOfStation` and `provisionedNumberOfStations` keep
`.nonnegative()` (back-compat / 'none provisioned' is meaningful).
- ConfigurationValidation: import `isEmpty` directly from
`utils/Utils.js` instead of the `utils/index.js` barrel, which
re-exports `Configuration` and creates the cycle
`ConfigurationValidation → utils/index → Configuration → ConfigurationValidation`.
Mirrors the existing direct-path `BaseError` import in
`ConfigurationMigrations.ts` for the same TDZ-cycle-avoidance reason.
* docs(config): fix precision defects in schema JSDocs and README
- ConfigurationSchema.ts: correct StorageConfigurationSchema JSDoc —
'URI' is accepted but not auto-migrated (not in DEPRECATED_KEY_REMAPPINGS);
narrow ConfigurationSchema JSDoc meta-test scope claim to 'top-level
and worker.* keys' to match actual test coverage.
- README.md: clarify that deprecated-key remapping is unconditional
(runs on every load, not only on v0 migration).
* fix(config): keep numberOfStations as .nonnegative() to preserve disabled-entry convention
Revert the .positive() tightening from 95cd26dd: docker/config.json ships
5 stationTemplateUrls entries with `numberOfStations: 0` (used as a
'keep on disk but disabled' convention), and this file is copied to
src/assets/config.json by docker/Dockerfile at image build time. With
.positive(), Docker images built from this branch would fail to start
on validateConfiguration's strict parse → process.exit(1).
Bootstrap loops already tolerate 0 (no-op, no crash). The original
.nonnegative() correctly models the on-disk convention; isValidNumberOfStations()
in UIServerSecurity.ts is for runtime UI add-station requests, a
separate concern from config-file parsing.
* docs(config): trim re-entrancy and clone rationales to one-liners
Jérôme Benoit [Tue, 26 May 2026 17:40:45 +0000 (19:40 +0200)]
ci(renovate): enforce 3-day minimum release age for npm packages
Extend the Renovate config with the official 'security:minimumReleaseAgeNpm'
preset so that Renovate waits 3 days after publication before creating PRs
for any npm/pnpm dependency. This adds a buffer against unpublished or
freshly-broken releases (e.g. malicious packages, npm unpublish window,
transient registry/lockfile resolution issues).
Add `atomicWriteFile` and `atomicWriteFileSync` to `src/utils/FileUtils.ts`,
implementing the canonical write-then-rename pattern with optional fsync
durability and best-effort temp file cleanup on failure. Errors are funnelled
through the existing `handleFileException` helper so callers stay aligned
with the project-wide file error reporting contract.
Migrate the five non-atomic disk writes uncovered by the audit:
- `BootstrapStateUtils.writeStateFile` replaces its inline tmp+rename with
the new primitive (single source of truth, gains fsync durability).
- `ChargingStation.saveConfiguration` replaces `writeFileSync` so charging
station OCPP configuration JSON cannot be torn by a crash mid-write.
- `JsonFileStorage.storePerformanceStatistics` drops the persistent
`openSync('w')` file descriptor design (which truncated the file at byte
zero on every sample) and uses the atomic primitive instead. Also fixes
the previous fire-and-forget `runExclusive(...).catch()` pattern by
awaiting the lock.
- `OCPP20CertificateManager` writes installed PEM certificates atomically.
Add `FileType.Certificate` so PEM writes can flow through
`handleFileException` with an accurate file type label.
Concurrent writers to the same path must still be serialized externally
(typically via `AsyncLock`); the primitive does not implement an internal
queue, matching how every existing call site already locks. The
`createWriteStream` diagnostics archive in OCPP 1.6 `GetDiagnostics` is
intentionally left as-is since the file is ephemeral (FTP-uploaded then
discarded).
Targets Node >= 22 so `writeFile`/`writeFileSync` natively expose the
`flush: true` option used for the fsync step.
* fix(utils): align atomic write call sites on a single error path
Address review feedback on PR #1871. The atomic write primitive logs and
re-throws by default via `handleFileException`. Three call sites kept
their pre-migration outer error wrappers, producing double handling:
- `ChargingStation.saveConfiguration`: the `.catch` handler attached to
the fire-and-forget `AsyncLock.runExclusive(...).catch(...)` chain
re-threw inside the catch callback, leaking an unhandled promise
rejection on every config write failure (disk full, EACCES, EROFS,
...). Pass `{ throwError: false }` to that `handleFileException`
call so the rejection is fully absorbed. The retry semantics are
preserved: when `atomicWriteFileSync` throws, the `endMeasure` and
`configurationFileHash` updates inside the lock callback are skipped,
so the next `saveConfiguration` invocation will retry the write.
- `JsonFileStorage.storePerformanceStatistics`: drop the redundant
outer `try/catch` and pass `{ throwError: false }` to the primitive,
matching the `BootstrapStateUtils.writeStateFile` template. Failures
now produce a single error log instead of one error log followed by
a second warn-level log.
- `OCPP20CertificateManager.storeCertificate`: replace the empty
`logPrefix` with a string carrying `stationHashId` and the module
origin so the new error log carries actionable context. The outer
`try/catch` in `storeCertificate` only stringifies the error into
the structured `{ success: false, error }` result and does not call
`handleFileException`, so there is no double handling here — only
the missing context to fix.
* refactor(utils): consolidate atomic write options and address audit findings
- FileUtils: include threadId in temp filename for cross-worker uniqueness;
fold errorParams into AtomicWriteOptions (single trailing options bag);
document SIGKILL leak, durability scope, and per-field defaults; drop
redundant 'utf8' as BufferEncoding cast.
- BootstrapStateUtils.writeStateFile: drop the undefined placeholder.
- ChargingStation.saveConfiguration: route fs failures (already logged at
error by handleFileException) to debug, and surface non-fs failures from
the lock body at error level via a typeof-guarded ErrnoException check.
- OCPP20CertificateManager.storeCertificate: take a required logPrefix from
the caller (replaces the synthetic prefix); drop the redundant manual
mkdir+pathExists block (atomicWriteFile's default ensureDir handles it);
write certificates with mode 0o600; document the per-path serialization
rationale (paths keyed by certificate serial number). Both
OCPP20IncomingRequestService callers updated to pass
chargingStation.logPrefix(); the manager unit tests are migrated.
- JsonFileStorage.storePerformanceStatistics: migrate to AtomicWriteOptions
with errorParams; set flush: false on this hot telemetry path (durability
across crashes is not required for performance records); add 5 focused
unit tests covering happy path, overwrite, Map serialization, runtime
storage directory removal, and parallel writes.
- FileUtils tests: tighten assert.throws/rejects with { code: 'ENOENT' } and
add a deterministic test that asserts the destination remains intact and
the temp file is cleaned up when the rename step fails.
* fix(performance-storage): decode file URIs into native filesystem paths
JsonFileStorage parsed the storage URI via new URL(uri) and used
.pathname directly as a filesystem path. On Windows, file: URLs yield
WHATWG-formatted pathnames such as '/C:/Users/...' which mkdirSync
interprets relative to the current drive, producing corrupt paths like
'D:\\C:\\Users\\...' and breaking JsonFileStorage on windows-latest CI.
Use fileURLToPath() (the standard Node.js inverse of pathToFileURL) to
decode file: URIs into the native path on every platform. Non-file
schemes (typically jsonfile:./relative-path) keep .pathname semantics
for backward compatibility with existing user configurations.
The new tests in tests/performance/storage/JsonFileStorage.test.ts
exercise this code path with pathToFileURL(absolutePath), which now
round-trips correctly across POSIX and Windows.
Jérôme Benoit [Fri, 22 May 2026 01:07:50 +0000 (03:07 +0200)]
chore(deps): update @ai-hero/sandcastle to 0.5.11
Refresh the patch via 'pnpm patch'/'pnpm patch-commit' and pin the
patch key to the exact version so context drift on future bumps
fails loudly instead of applying silently.
Patch semantics are unchanged: it still adds the 'thinking' option
to PiOptions and threads '--thinking <level>' into the pi agent
provider's print command. Only context lines and blob hashes are
refreshed for 0.5.11 (DEFAULT_MODEL bumped to claude-opus-4-7
upstream).
Jérôme Benoit [Mon, 18 May 2026 21:49:47 +0000 (23:49 +0200)]
refactor(bootstrap): factorize UI server start/stop logic
Extract idempotent helpers and remove duplication across the four
UI server lifecycle call sites:
- public startUIServer() now contains the actual logic instead of
delegating to a private wrapper; start() calls it directly.
- new private stopUIServer() helper replaces the two inline stop
blocks in gracefulShutdown() and restart().
- restart() guards syncUIServerTemplates() on uiServerStarted to
avoid redundant template sync (twice when re-enabling, wasted
call when the UI server is disabled or stopped). startUIServer()
performs the sync itself when it actually starts the server.
Jérôme Benoit [Mon, 18 May 2026 00:54:16 +0000 (02:54 +0200)]
fix(sandcastle): resolve TDZ in strategies/index.ts module init
Move the file-private validation regexes `STRATEGY_KEY_PATTERN` and
`CONTROL_TAG_PATTERN` (with their JSDoc verbatim) above the eager call
`STRATEGY_BY_KEY = indexByKey(STRATEGY_REGISTRY)`, so they are
initialized before `indexByKey` reads them at module evaluation.
repositories / e-mobility-charging-stations-simulator.git / log
