Jérôme Benoit [Sun, 23 Aug 2026 15:33:04 +0000 (17:33 +0200)]
chore: remove Sandcastle integration (#2093)
* chore: remove Sandcastle integration
- delete .sandcastle agent harness, Docker sandbox and scheduled workflow
- drop @ai-hero/sandcastle devDependency and 'sandcastle' script
- remove .sandcastle from lint/format globs, tsconfig include and .cfignore
- drop unused cspell words and pnpm minimumReleaseAgeExclude entry
Jérôme Benoit [Fri, 14 Aug 2026 22:08:21 +0000 (00:08 +0200)]
refactor(webui): order station action buttons by CSO lifecycle (#2084)
* refactor(webui): order station action buttons by CSO lifecycle
Reorder the station-card action buttons in both skins to follow the
Charging Station Operator lifecycle (bring-up, provisioning, exploitation,
observability):
- modern StationCard: Start/Stop, Connect/Disconnect, Configuration,
Authorize, Details
- classic CSData: Start/Stop, Connection, Set Supervision Url,
Change Configuration, Show Details
This fixes Authorize (runtime) preceding Configuration (provisioning) and
Details being wedged between active commands. Action sets, labels, handlers,
routes, styling and the isolated Delete button are unchanged.
Make the classic CSData show-details toggle assertions look the button up by
id instead of a fixed index, so they stay robust to ordering.
Add id-based navigation tests for the change-configuration toggle in
classic CSData, mirroring the show-details coverage: assert on() pushes
the change-configuration route with hashId/chargingStationId params and
off() pushes back to charging-stations. Closes the coverage gap for the
toggle reordered in this change.
* test(webui): select set-supervision-url toggle by id for consistency
Align the set-supervision-url toggle navigation tests with the
change-configuration and show-details ones by looking the toggle up via
its id instead of a fixed index, so the whole toggle-navigation suite is
order-independent and uses a single selection convention.
Jérôme Benoit [Fri, 14 Aug 2026 18:03:20 +0000 (20:03 +0200)]
fix(webui): keep modern station-card Delete action aligned on the footer row (#2083)
The modern station-card footer laid the 5 action buttons in a non-wrapping
inline-flex group beside the Delete button, inside a wrapping footer. Once the
group's max-content width exceeded the card, the footer broke the line between
the group and Delete, dropping Delete onto its own row where margin-left:auto
pinned it bottom-right (orphaned) instead of aligned with the other actions.
Make the footer a single flex line (flex-wrap: nowrap) and let the action
group wrap its buttons internally (flex-wrap: wrap) so it reflows across rows
on narrow cards while Delete stays on the same row, pinned right. align-self:
flex-start top-aligns Delete with the first action row when the group wraps.
Presentational modern-skin CSS only; no markup or handler change.
Jérôme Benoit [Fri, 14 Aug 2026 13:39:35 +0000 (15:39 +0200)]
fix(simulator): seed autoRegister and ocppProtocol defaults into stationInfo (#2082)
* fix(simulator): seed autoRegister and ocppProtocol defaults into stationInfo
autoRegister and ocppProtocol had no default, so a template omitting them
left stationInfo carrying undefined for both. Raw consumers of stationInfo
— the UI data payload (buildChargingStationDataPayload) and the persisted
configuration — therefore received undefined, so the Web UI station details
showed an empty placeholder for Auto Register and OCPP Protocol.
Both are static defaults (no derivation), so seed them in DEFAULT_STATION_INFO
(autoRegister: false, ocppProtocol: OCPPProtocol.JSON) alongside currentOutType
and ocppVersion. getStationInfo applies them via
mergeDeepRight(DEFAULT_STATION_INFO, stationInfo), so an explicit template/file
value or option still wins (idempotent, no clobber) and a persisted config
predating the fields is backfilled on reload. Export OCPPProtocol from the
types barrel to keep Constants.ts importing types from a single source, as
OCPPVersion already does.
Runtime-neutral: ocppProtocol has no runtime read; the autoRegister === true
guards are unaffected by false vs undefined; the getHeartbeatInterval warn
guarded by === false is unreachable in the initialized pipeline because
initializeOcppConfiguration always seeds the HeartbeatInterval key first.
* refactor(test): extract persisted-config resolution into realStation helpers
The persisted configuration file/dir resolution from a real-station template
was inlined and duplicated across three suites (AutoRegisterOcppProtocol,
NumberOfPhases, ResetIdentity). Extract persistedConfigurationDir and
resolvePersistedConfigurationFile into StationHelpers.realStation.ts (single
source of truth for the temp-dir layout) and migrate all three call sites.
resolvePersistedConfigurationFile throws a descriptive error when no config
exists yet, replacing the silent `?? ''` fallback (which degraded into EISDIR).
Group the helper by concern (temp-dir lifecycle / construction / persisted-config
resolution) with section headers, and document the two new helpers in
TEST_STYLE_GUIDE.md.
* docs(test): harmonize helper-table param placeholders in TEST_STYLE_GUIDE
Jérôme Benoit [Thu, 13 Aug 2026 20:13:58 +0000 (22:13 +0200)]
test(simulator): drop manual test counters from worker broadcast-channel test (#2081)
The group-header comments carried hand-maintained (N tests) counters that rot
as tests are added or removed; the test runner already reports the count.
Remove them, keeping the descriptive group headers.
Jérôme Benoit [Thu, 13 Aug 2026 19:37:03 +0000 (21:37 +0200)]
feat(webui): allow editing charging station configuration (#2077)
* feat(webui): allow editing charging station configuration
Add a generic, data-driven editor for a charging station's current OCPP
configurationKey values in the Web UI (both classic and modern skins),
closing #1828.
A new CHANGE_CONFIGURATION UI protocol verb clones the SET_SUPERVISION_URL
chain end-to-end (UIClient -> ProcedureName/BroadcastChannelProcedureName ->
AbstractUIService mapping -> worker handler). To stay OCPP spec-faithful, the
change is applied through a new version-agnostic
ChargingStation.changeConfiguration seam that reuses the existing OCPP 1.6
ChangeConfiguration and OCPP 2.0.1 SetVariables handler logic (readonly
rejection, integer/bounds validation, heartbeat/WS-ping restarts, reboot
signalling) without emitting a CSMS response, then emits
ChargingStationEvents.updated so the UI refreshes.
OCPP 2.0.1 configurationKey entries are now seeded with their registry
mutability/rebootRequired flags, so readonly keys are correctly disabled in
the UI and reboot keys are flagged.
Address PR review coverage gaps:
- assert ChargingStation.changeConfiguration emits ChargingStationEvents.updated
only on Accepted/RebootRequired (drives the read-view refresh)
- assert the OCPP 1.6 seam applies changes without emitting a CSMS response
- add OCPP 2.0.1 seam read-only rejection and reboot-required cases
* refactor(webui): address review findings for change-configuration
- OCPP 2.0.1 seam: set the resolved `instance` only on the SetVariables
`component` (drop the redundant `variable.instance`); the registry models
the instance as component-scoped and internal resolution reads
`variable.instance ?? component.instance`, so this is behavior-neutral.
Add an instance-scoped non-regression test (TariffCostCtrlr.Enabled.Cost).
- useStationDetails: docstring no longer claims "read-only" now that it
exposes editableConfigurationKeys.
- ui/web README: "edit configuration" -> "change configuration" to match the
UI/code terminology.
- ModernLayout: order the ChangeConfigurationDialog async declaration
alphabetically and put :hash-id before :charging-station-id, matching the
sibling dialogs.
* refactor(webui): hoist change-configuration draft state into the shared composable
Address the second-round review findings:
- M1 (DRY): move the per-key draft values, seeding watch and save() from both
skin components into useChangeConfigurationForm (now takes the editable keys
ref and exposes { draftValues, pending, save }), mirroring useSetUrlForm which
owns its form state. The classic action and modern dialog become pure UI.
- M2: add direct unit tests for the worker CHANGE_CONFIGURATION handler
(delegation, empty-value accepted, missing/empty/non-string key or value
rejected with BaseError).
- M3: align residual "edit"/"editing" wording with the "change configuration"
terminology in docstrings (kept editableConfigurationKeys as a property name).
* refactor(webui): validate required broadcast-channel string fields with isNotEmptyString
Replace the raw `typeof x !== 'string' || isEmpty(x)` guards in the
CHANGE_CONFIGURATION and SET_SUPERVISION_URL worker handlers with the
`!isNotEmptyString(x)` type guard (the repo-wide idiom, ~78 usages), which also
narrows the field to a non-empty string. Both handlers are switched together so
the file keeps a single convention. `isEmpty` remains used for the
empty-response status checks; the `value` field stays `typeof value !== 'string'`
since an empty value is a legitimate configuration value.
* refactor(webui): tidy change-configuration naming, reverse-map and MCP schema
Third-round review nits:
- M1: rename `editableConfigurationKeys` -> `visibleConfigurationKeys` (the
collection includes read-only keys; the name now matches its source util
getVisibleConfigurationKeys). Propagated across useStationDetails,
useChangeConfigurationForm, both skin components and the tests.
- M2: build OCPP20VariableManager's flat-key reverse map as a declarative
private instance field (like #validComponentNames) instead of a mutable
module-level for-loop; the dedup guard was inert (composite key names are
unique).
- T1: inline the single-caller private `submit` into `save`.
- T2: enforce a non-empty `key` at the MCP option layer (z.string().min(1));
`value` stays unconstrained (empty is a valid configuration value).
Add component tests for the classic `ChangeConfiguration.vue` action and the
modern `ChangeConfigurationDialog.vue`, which were shipped without component
tests and dropped ui/web coverage below the CI thresholds (the failing
`Build dashboard / Node 24.x / ubuntu-latest` cell runs `pnpm test:coverage`).
Tests exercise the observable contracts already verified in-browser: not-found
panel, empty-state, non-visible key exclusion, read-only input/button disabling,
editable save invoking `changeConfiguration`, reboot-required notice, read-only
save guard and backend-rejection error toast. Adds the missing
`changeConfiguration` method to the shared `MockUIClient`.
Refs: #1828
* fix(ocpp): reject empty value for integer 1.6 configuration keys
Address the initial-review findings on PR #2077:
- T1 (fix): the OCPP 1.6 ChangeConfiguration handler accepted an empty/blank
value for integer keys (Number('') === 0 passed the non-negative-integer
check) and persisted the raw empty string. Reject it explicitly via
isNotEmptyString, matching the spec (values not conforming to the expected
integer format are Rejected). Covers both the UI seam and the OCPP wire path.
Add a discriminating test (empty value -> REJECTED, value preserved).
- T2 (fix): correct the misleading worker error message
"'value' field is required" -> "'value' field must be a string" (an empty
string is a legitimate value; only a non-string is rejected).
- T3 (test): rename the composable test description "editable keys" ->
"visible keys" to match the renamed visibleConfigurationKeys parameter.
- M1 (docs): document, on OCPP20 changeConfiguration, that the resolved
instance is carried on component.instance and is internal-only (never
emitted to a CSMS), so the component- vs variable-instance distinction is
immaterial here.
- M2 (docs): add the missing 'changeConfiguration' ProcedureName block to the
README UI protocol reference.
Kept as-is with rationale: the version-agnostic seam reuses the 1.6-named
ChangeConfigurationResponse type (established CommandResponse union convention,
type-safe alias); the modern dialog table style follows the skin's
per-component scoped convention. Cleartext config values in the edit UI are
pre-existing (already exposed via LIST_CHARGING_STATIONS / MCP) and out of
scope.
Refs: #1828
* docs: clarify changeConfiguration value validation in the README protocol reference
Address review finding TR1: the changeConfiguration block claimed "an empty
string is allowed", which is only true at the message-envelope level. A value
invalid for the target key (e.g. a non-integer or empty value for a numeric
1.6 key) is rejected. Reword the value clause to state that invalid values are
rejected and keep it version-agnostic (the verb serves both OCPP 1.6 and
2.0.1). No code change.
Refs: #1828
* test(webui): factor duplicated configuration-keys station fixture into a shared factory
The `stationWithKeys` helper was duplicated verbatim in the classic and modern
skin test files. Mirroring the test-factorization convention consolidated in
main (PR #2078: shared test helpers, no per-file scaffolding duplication),
extract it into the canonical fixture home `tests/unit/constants.ts` as
`createStationWithConfigurationKeys`, and migrate both skin test suites to it.
Removes the now-unused local helpers and `ConfigurationKey` imports. No behavior
change (ui/web 592/592).
Refs: #1828
* test(webui): migrate remaining inline config-key fixtures to the shared factory
Address review finding TR1: complete the DRY migration started with
createStationWithConfigurationKeys. Replace the 13 remaining inline
`createChargingStationData({ ocppConfiguration: { configurationKey: … } })`
sole-override fixtures across the stationDetails, useStationDetails, ShowDetails
and ShowDetailsDialog test blocks with the shared factory. The empty-
ocppConfiguration case (stationDetails.test.ts, tests the absent-key fallback)
and all sites carrying additional overrides are intentionally left inline. No
behavior change (ui/web 592/592).
- OCPP16 integer-key guard: consolidate the two lines commenting the same guard
into one coherent, accurate rationale — why Number() over convertToInt
(truncates '1.5' → 1, throws on ''/'abc') and why the explicit isNotEmptyString
check. Number() yields a non-integer float ('1.5' → 1.5) or NaN ('abc'), both
caught by !Number.isInteger; isNotEmptyString rejects '' (Number('') === 0 would
otherwise pass).
- OCPP20 changeConfiguration JSDoc: remove redundancy with the delegate's JSDoc
and compress the instance-placement caveat, keeping every non-derivable fact.
Comment-only change; no behavior change.
Refs: #1828
* refactor(webui): single-source config-key formatting and align a11y state
- Export the shared formatBoolean helper and reuse it for the Readonly/Reboot
cells in both change-configuration skins (drop the 4 inline Yes/No literals).
- Classic Save button: expose aria-busy while a change is in flight, matching
the modern ActionButton (no visual spinner: classic has no such token).
- Drop the redundant aria-labelledby (and its useId) on the modern table whose
<caption> already names it, and the partial aria-disabled on the inputs whose
native disabled is authoritative.
- Add tests: Readonly/Reboot cell rendering and in-flight aria-busy in both
skins, plus modern parity for reboot-notice, read-only-not-submitted and
error-toast (mirroring the classic suite).
Refs: #1828
* docs: clarify metadata-driven reboot notice and safe 2.0.1 status collapse
- useChangeConfigurationForm: note that the reboot notice derives from the key
metadata, not the runtime status, because the worker response collapses
ACCEPTED|REBOOT_REQUIRED into a boolean success.
- OCPP20 status mapping: note that UnknownComponent/UnknownVariable are
unreachable via changeConfiguration (resolveConfigurationKeyName gates unknown
keys) and that Record exhaustiveness is compile-enforced.
- Yes/No cell test: use column-asymmetric keys (readonly-only vs reboot-only)
so an accidental swap of the Readonly/Reboot columns fails the assertion.
- Add a guard that the OCPP parameters table is named via its <caption> in
both skins (the modern table dropped its redundant aria-labelledby).
Refs: #1828
* refactor(ocpp): use isEmpty for the SetVariables result check
Replace `response.setVariableResult.length === 0` with `isEmpty(...)` in the
2.0.1 changeConfiguration seam, matching the repo convention (isEmpty is
already imported and used for arrays in this file).
Refs: #1828
* test(webui): fit CHANGE_CONFIGURATION worker tests into the group structure
The two CHANGE_CONFIGURATION describes (status collapse + handler) were wedged
between Group 1 and Group 2 without a group banner. Relocate them after Group 2
under a numbered "Group 3" banner (restoring ascending group order and filling
the pre-existing gap), and correct the stale Group 4 count (8 -> 9 tests).
Jérôme Benoit [Tue, 11 Aug 2026 22:09:54 +0000 (00:09 +0200)]
fix(simulator): seed numberOfPhases default into stationInfo (#2078)
* fix(simulator): seed numberOfPhases default into stationInfo
The derived numberOfPhases default (AC: 3, DC: 0) lived only in the
getNumberOfPhases getter and was never written to stationInfo. Raw
consumers of stationInfo — the UI data payload (buildChargingStationDataPayload)
and the persisted configuration file — therefore received an undefined
numberOfPhases, so the Web UI showed an empty placeholder instead of the
effective phase count.
Seed stationInfo.numberOfPhases via the existing getNumberOfPhases getter
in getStationInfo, post-merge and source-agnostic, so both freshly
template-derived and already-persisted (legacy) configurations are fixed.
Idempotent: an explicit AC template value is preserved (?? 3), DC pins 0.
Backend consumers keep reading the getter and are invariant.
* test(simulator): align numberOfPhases test names with should-prefix convention
* test(simulator): cover DC phase pinning and persisted-config backfill
Add two discriminant cases to the numberOfPhases seeding suite:
- DC pins numberOfPhases to 0 even when the template sets a value
- a persisted configuration predating the field is backfilled on reload
(the file-sourced path that motivated seeding in the orchestrator)
- reuse the canonical flushMicrotasks test helper instead of a local
node:timers/promises import
- expand the test @file description to cover DC pinning and legacy backfill
- align the seed comment terminology on "backfill"
* test(simulator): dedupe test baseName literal into a constant
* docs(simulator): fix backfill comment accuracy and align seed wording
* test(simulator): inline baseName to match sibling station-test convention
* test(simulator): consolidate real-station-from-template scaffolding into a shared helper
Six charging-station tests each hand-rolled the same temp-dir template
scaffolding to build a real ChargingStation (the mock factory bypasses
initialize()/getStationInfo()). Extract writeStationTemplate/copyStationTemplate/
createStationFromTemplate/cleanupStationTemplates into StationHelpers.realStation,
migrate all six tests, and document the helper in TEST_STYLE_GUIDE.
* test(simulator): share temp-file and singleton-reset test helpers
Extract the mkdtemp/write/cleanup boilerplate into tests/helpers/TempFiles.ts
(createTempDir/writeTempFile/cleanupTempDirs) and a resetSingleton() helper into
TestLifecycleHelpers, then migrate the file-I/O tests (IdTagsCache, EvProfiles,
JsonFileStorage, Configuration-HotReload, FileUtils, UIMCPServer integration) and
the singleton-reset tests (Bootstrap, SharedLRUCache, IdTagsCache) onto them.
* docs(test): document TempFiles helpers and resetSingleton in the style guide
Jérôme Benoit [Mon, 10 Aug 2026 17:38:42 +0000 (19:38 +0200)]
fix(webui): polish the station details view (panel placement + section/key-value separation) (#2076)
CSS/Vue-only polish of the #993 Show details view: classic panel-placement fix + section/key-value separation; modern section panels, promoted titles and tighter symmetric row separators; not-found escape; toggle-navigation test coverage.
Jérôme Benoit [Mon, 10 Aug 2026 15:51:27 +0000 (17:51 +0200)]
feat(webui): add show details action for charging stations (#2072)
* feat(webui): add show details action for charging stations
Add a read-only "Show details" action in both Web UI skins displaying a
charging station's stationInfo, OCPP configuration parameters and other
relevant ChargingStationData fields. Frontend only: the data is already
shipped to the client via the charging station data payload.
- Shared pure util `stationDetails.ts` (single source of truth for section
selection, field formatting and OCPP key visibility filtering), matching
the existing `stationStatus.ts` pure-util convention.
- Classic skin: `show-details` router action + `ShowDetails.vue` panel,
triggered by a shared ToggleButton in the station actions cell.
- Modern skin: `StationDetailsDialog.vue` modal, triggered by a footer
"Details" button on the station card, wired through ModernLayout.
- Supervision password is always masked; supervision URL is host-only.
Closes #993
* [autofix.ci] apply automated fixes
* refactor(webui): centralize OCPP row formatting and address review
Review-round-1 fixes for the "Show details" action:
- Move OCPP cell formatting (readonly/reboot/value) into the shared
`buildConfigurationRows` util so both skins render identical, single-
sourced rows instead of duplicating the ternaries.
- Format Boot Notification "Current Time" via toLocaleString (consistent
with "Last Update") and drop the unsafe double cast.
- Give the modern OCPP table an accessible name (aria-labelledby).
- Tests: assert password masking in the classic rendered panel, cover the
OCPP readonly/reboot/value cell formatting in both skins and the util.
* test(webui): tighten details tests and unify empty-value formatting
Review-round-2 fixes for the "Show details" action:
- Format the OCPP parameter value via the shared `formatValue` so an
empty-string value renders as the empty placeholder, consistent with
every other field (was `value ?? Ø`, which left '' blank).
- Strengthen the modern password-masking test to also assert the masked
placeholder, add coverage for the empty-string value case, and assert
the modern OCPP table's aria-labelledby accessible name.
* refactor(webui): address initial-review findings for show details
- M1 (DRY): extract shared `useStationDetails(hashId)` composable consumed by
both skins, removing the duplicated station/sections/configurationRows
computed (mirrors the shared `useSetUrlForm` precedent).
- M2 (terminology): unify the feature label on "Show Details" across the
classic header, the modern dialog title and the README; the modern card
keeps its terse "Details" button per the card convention; route id
unchanged.
- M3: drop the duplicated Boot Notification "Status" entry (kept as
"Registration Status" under General).
- N1: factor a private `formatDate` helper and guard both dates.
- N3: give each modern detail `<dl>` section an accessible name via useId().
- N2 (OCPP terminology) intentionally left as "OCPP Parameters" to match the
issue wording; documented in review.
- Tests: new `useStationDetails` composable tests; assert the modern section
aria-labelledby wiring; update the "Show Details" heading/title assertions.
* style(webui): harmonize modern show-details with skin conventions
- H3: extract the shared `.modern-section-label` primitive (renamed from
`.modern-card__section-label`, single consumer migrated) and use it for the
modern dialog section headings instead of a one-off `.station-details__title`.
- H1: restyle the detail key/value list to the modern spec typography
(uppercase muted `dt`, strong `dd`) in a left-aligned two-column grid fit
for the wider dialog (drops the ad-hoc space-between/right-align/hairlines).
- H2: restyle the OCPP table to the modern low-chrome table aesthetic
(border-collapse, no per-cell borders, muted uppercase headers, subtle row
separators) matching the existing `.modern-connector__tx-table` precedent;
scope word-break to values/keys so column headers no longer break mid-word.
Classic skin unchanged (already reuses the shared data-table system). No test
changes: DOM hooks (.station-details__list/__table), aria-labelledby and the
OCPP row formatting are preserved. Card rendering is visually unchanged.
* style(webui): resolve exhaustive-review nits for show details
- MIN-1: left-align the classic detail table cells (target th/td so the
shared `.data-table` center rule no longer wins), fixing centered values.
- MIN-2: modern dialog title to sentence-case "Show details — {id}" to match
the sibling dialog titles (classic header keeps Title Case per its convention).
- NIT-1: derive the OCPP heading id from useId() instead of a hardcoded id,
consistent with the section headings.
- NIT-2: extract a private `formatBoolean` helper reused by
buildConfigurationRows and formatValue's boolean branch.
- NIT-3: align the shared "Supervision Url" label with the existing majority
spelling used across the classic surface.
- NIT-4: rename StationDetailsDialog.vue -> ShowDetailsDialog.vue (matches the
classic ShowDetails.vue and the feature name); update wiring + tests.
"OCPP Parameters" kept (issue #993 wording). Tests updated for the new title,
useId-based aria and the rename. 567 tests green.
* style(webui): order ShowDetailsDialog async import alphabetically
* docs(webui): fix show details JSDoc wording and uniformize empty-state punctuation
The HTTP UI transport (UIHttpServer / ApplicationProtocol.HTTP) is
deprecated in favor of the MCP transport and will be removed. Drop its
Insomnia requests collection and the README pointer to it; the WebSocket
collection and the deprecation notice are kept.
Jérôme Benoit [Tue, 4 Aug 2026 21:20:36 +0000 (23:20 +0200)]
chore(opencode): stop Serena MCP from auto-opening the web dashboard (#2066)
Pass --open-web-dashboard false to serena start-mcp-server in .opencode/opencode.jsonc so the dashboard is not opened in a browser on every OpenCode startup.
Jérôme Benoit [Tue, 4 Aug 2026 20:04:07 +0000 (22:04 +0200)]
feat(simulator): add AC/DC conversion efficiency template support (#2063)
Add optional template tunable conversionEfficiency (float, (0, 1], default 1) reducing available charging power on DC stations only (currentOutType === DC). Applied at runtime in getConnectorMaximumAvailablePower to both power-derived bounds; amperage and charging-profile limits unchanged; no reduced value persisted. Absent field keeps existing behavior.
fix(ui-server): reconcile evses wire type so /metrics works on OCPP 2.0.x (#2048)
* fix(ui-server): reconcile evses wire type so /metrics works on OCPP 2.0.x
The worker->main producer `buildEvseEntries` emits `evseStatus.connectors`
as a `ConnectorEntry[]` array, but `ChargingStationData.evses` was typed
with the in-memory `EvseStatus` (connectors: Map), hidden by an
`as unknown as EvseEntry[]` cast. On a live scrape the metrics consumer
`iterateConnectors` tuple-destructured each array element as a Map entry,
throwing `TypeError: ... is not iterable` inside prom-client's
`Registry.metrics()` -> HTTP 500 for every OCPP 2.0.x station.
`countConnectors` also read `.size` on the array -> NaN. OCPP 1.6 was
unaffected because its `data.connectors` path already yields ConnectorEntry
objects without destructuring.
Introduce dedicated wire types `EvseEntryData`/`EvseStatusData`
(connectors: ConnectorEntry[]), type `ChargingStationData.evses` and
`buildEvseEntries` with them, and drop the cast so the compiler enforces
the array shape end to end. `iterateConnectors` now yields the entry
directly and `countConnectors` uses `.length`. The array shape matches what
the Web UI and CLI already consume over the JSON wire; a Map would not
survive JSON serialization. The in-memory `EvseStatus` (connectors: Map)
is unchanged.
Add a regression test feeding the actual `buildEvseEntries` output through
the /metrics scrape (fails pre-fix with 500/NaN, passes post-fix) plus
zero-id and empty-evses edge cases, and migrate the existing EVSE-mode test
off its synthetic Map fixture that masked the defect.
Add JSDoc on the wire types introduced for #2046:
- EvseEntryData: anchor its relation to the in-memory EvseEntry (Map) and to
the structurally-identical ui-common EvseEntry (duplicated, no cross-package
re-export), removing the naming ambiguity.
- EvseStatusData: document that MeterValues is intentionally omitted (the
producer never emits it, no UI-facing consumer reads it off the wire),
resolving the PR review comment on the omission.
Address review findings on the #2046 regression tests:
- Convert the "empty evses" test to a populated EVSE with an empty connectors
array. The zero-EVSE case short-circuits `reduce` (returns 0 even pre-fix),
so it guarded nothing; a populated EVSE with `connectors: []` is the case
that pre-fix reads `.size` on an array -> undefined -> NaN. Verified: fails
pre-fix (connectors_total Nan), passes post-fix (0).
- Type the buildEvseEntries stub as `Pick<ChargingStation, 'iterateEvses'>`
instead of `as unknown as`, so `iterateEvses`/`evseStatus` stay type-checked
(resolves the PR review comment on the stub cast).
- Use `satisfies` instead of `as` on the migrated EVSE-mode fixture, so a
Map/array shape drift is caught at compile time (the unchecked-cast class
that masked the original bug).
* refactor(types): make EvseStatusData an immutable wire snapshot
Mark EvseStatusData.availability and connectors readonly (and connectors a
readonly ConnectorEntry[]), matching the readonly EvseEntryData wrapper. The
wire projection is a produced-once snapshot never mutated by any consumer
(countConnectors reads .length, iterateConnectors iterates read-only), so
immutability is the accurate contract and removes the wrapper/payload
readonly asymmetry. In-memory EvseStatus (mutable Map) is unchanged.
N1/INFO - server16.py: add Unix 128+signal comment before sys.exit;
expand boot_index one-liner to full NOTE (shared across ALL stations,
not per-station)
N2 - test_server.py: extend _patch_main with **args_overrides (same
pattern as test_server16.py); add test_boot_status_sequence_parsed in
TestMainGracefulShutdown — covers server.py:1134-1149
N3 - test_server16.py: add test_boot_status_single_value — covers
server16.py:786-787 (elif --boot-status single value branch)
N4 - pyproject.toml: update description to cover both 1.6 and 2.0.1
N5 - README.md: remove duplicated sentence at line 38
N6 - test_server16.py: fix RuntimeWarning (coroutine never awaited)
in test_commands_sequence_scheduled_on_connect — mock_cp.send_commands
was AsyncMock, its coroutine was passed to mocked create_task unrun;
replace with MagicMock() to prevent the orphan coroutine
MAJOR-1 - README.md 2.0.1: add ReserveNow and CancelReservation to
Outgoing Commands list; add --reservation-id, --reserve-id-token,
--reserve-evse-id flags to Command-specific options; add example command
MAJOR-2a - test_server.py: add caplog assertions to 4 signed MeterValue
tests (started/updated/ended/meter_values) — verify _log_signed_meter_values
actually logs, not just that the handler returns correct type
MAJOR-2b - test_server.py: rewrite test_unsupported_command_logs_warning
with caplog + assert 'not supported' in log output
MAJOR-2c - test_server.py: align 7 empty-response handlers on equality
pattern (== EmptyResult()) on top of isinstance — matches test_server16
MINOR-1 - _common.py: add generic parse_commands(str, type[ActionT])
factored from the two identical _parse_commands; both servers now use
a one-line wrapper; remove import math from server16/server (now in
_common only); coverage 96.76% (+0.81pp)
MAJOR-A - README.md 2.0.1: replace invalid --trigger-message value
SignCertificate with the 11 exact MessageTriggerEnumType members
(SignChargingStationCertificate/SignV2GCertificate/SignCombinedCertificate
+ TransactionEvent + PublishFirmwareStatusNotification); the 1.6 list was
fixed in round-3, this is its 2.0.1 counterpart
MAJOR-B - test_server.py: add TestLogSignedMeterValues (3 tests) adapted
to 2.0.1 semantics (signed_meter_value field, not format=SignedData);
covers the previously-uncovered unsigned branch server.py:97->95
MINOR-C - test_server.py + test_server16.py: 6 error-handling tests
(timeout/ocpp_error/unexpected) upgraded from swallowing-only to caplog
assertions on level ERROR + message substring
MINOR-D - DRY: extract negotiate_subprotocol + install_signal_handlers_and_wait
to _common.py (version-agnostic lifecycle, logger passed explicitly to
preserve observability); both servers use them; remove import signal from
both servers; retarget 4 signal.signal patches to _common. Instance methods
(_call_and_log/_send_command/...) and DEFAULT_RESERVATION_* constants left
in place (OCPP coupling / locality cost > DRY gain for a test mock)
MINOR-E - _common.py to 100%: add non-numeric-delay and empty-entry tests
for _parse_commands in both suites
INFO MORT-1 - remove unreachable dead code (if not boot_sequence) in both
servers' main()
* test(ocpp-server): close round-5 parity + config findings
MINOR parity - test_server.py: assert len(config.charge_points) == 0 on
both on_connect rejection tests (missing subprotocol / protocol mismatch),
mirroring test_server16.py; guards against a regression registering a
ChargePoint on a rejected connection in 2.0.1
MINOR parity - test_server.py: strengthen Heartbeat test_returns_current_time
with a freshness window (delta < 60s) + import timezone, mirroring the 1.6
suite (was isinstance + tzinfo only)
INFO I4 - pyproject.toml: mypy python_version 3.14 -> 3.12 to match the
requires-python floor (>=3.12); mypy stays clean (verified live)
INFO I5 - README.md: add the --boot-status shorthand sentence to the 1.6
section for parity with 2.0.1 (identical behavior)
Deliberately NOT changed (accepted debt, evidence-based):
- DRY mixin for send_command/send_commands/handle_connection_closed: moving
them to _common would emit lifecycle logs under _common instead of
server/server16 (silent observability regression) for ~24 lines in a mock
- mypy test override kept: real guard (test files carry untyped defs)
- docstrings on _common helpers: 0/4 documented, keeping style consistent
* test(ocpp-server): close round-6 parity + hygiene findings
MINOR-1 - test_server16.py: add test_parent_id_tag_absent_by_default,
mirroring 2.0.1 test_authorize_no_enrichment_by_default; asserts
parent_id_tag is absent from id_tag_info under the default AuthConfig
(closes the last inter-suite rigor asymmetry)
MINOR-2 - server.py: harmonize _call_and_log docstring with server16.py
(both now '...based on its status')
MINOR-3 - test_server.py: add test_parse_set_variable_specs_skips_empty_entries
and test_parse_get_variable_specs_invalid_no_dot, covering the two
previously-uncovered branches of _parse_variable_specs (empty-entry skip,
require_value=False missing-dot); server.py 875/885 now covered
I1 - server.py + server16.py: derive --auth-mode choices/default from the
local AuthMode StrEnum ([mode.value for mode in AuthMode] / AuthMode.normal.value)
instead of string literals (AGENTS.md: avoid string literals when an
enumeration exists); order/content identical (verified live), no behavior change
* test(ocpp-server): close round-7 style + DRY findings
- test_server.py: fix stale docstring (20 -> 24 _send_* methods)
- server.py/server16.py: --auth-mode type=str -> type=AuthMode to match
the sibling enum-typed CLI args (--boot-status, --reset-type, ...); keep
choices for unchanged help/UX
- server.py/server16.py: extract DEFAULT_WHITELIST/DEFAULT_BLACKLIST module
constants (single source of truth; removes __init__/argparse duplication)
- test_server.py/test_server16.py: TestHandlerCoverage now asserts @on
registration (_on_action) so an undecorated handler is caught
* test(ocpp-server): close round-8 findings (harmonize auth-mode, DRY, handler routing)
- _common.py: host DEFAULT_WHITELIST/DEFAULT_BLACKLIST as the single source
of truth; server.py and server16.py now import them (removes cross-file
literal duplication)
- server.py/server16.py: drop now-dead choices=list(AuthMode) from --auth-mode
and move valid values into the help text, matching the sibling enum-typed
args (--boot-status, --reset-type, ...); uniform invalid-value error message
- server.py/server16.py: mode=args.auth_mode (args value is already an AuthMode
via type=AuthMode; drop redundant re-wrap)
- test_server.py/test_server16.py: TestHandlerCoverage now asserts each handler
is registered for the CORRECT OCPP action (_on_action == expected), not merely
decorated with some @on
* test(ocpp-server): address bot review comments on server16
- test_server16.py: use supported CSMS->CS commands (TriggerMessage/Reset)
instead of ClearCache/Heartbeat in the send_commands sequencing and
_parse_commands tests, so the fixtures reflect commands actually in
_COMMAND_HANDLERS rather than unsupported outgoing actions
- server.py/server16.py: replace the empty `except KeyboardInterrupt: pass`
with contextlib.suppress(KeyboardInterrupt) (clears the empty-except finding
without a comment; both servers for parity)
- server.py/server16.py: _resolve_auth_status returns default_status via an
unconditional trailing return instead of a wildcard `case _`, making the
control flow unambiguous to static analysis (both servers for parity)
Add an opt-in `uiServer.securityHeaders.strictTransportSecurity` config
knob (`string | false`) that, when set to a non-empty string, emits a
`Strict-Transport-Security` (HSTS) response header on the UI server.
Emission is gated on secure transport per RFC 6797 §7.2 (which forbids
sending HSTS over non-secure transport): the header is emitted only when
the response travels over direct TLS (`req.socket.encrypted`) or a trusted
reverse proxy that forwarded a secure protocol (`https`/`wss`), resolved by
a new `isRequestEffectivelySecure` predicate that reuses the access-policy
trusted-proxy/forwarded-protocol machinery. Over plaintext (e.g. loopback
development) the header is omitted.
The header is spread — via `getSecurityHeaders(isSecure)` and, for the
async HTTP success path, a per-uuid `secureResponses` map mirroring
`acceptsGzip` — across every UI-server HTTP response path that this code
writes: shared denials (`renderDenial`, now request-aware), HTTP success
responses (gzip and non-gzip), WebSocket upgrade rejections, and the
`/metrics` success and error responses, on the http/ws/mcp transports. MCP
JSON-RPC success bodies are written by the MCP SDK transport and are not
covered.
Mirrors the `metrics` opt-in sub-schema: one `.strict()` leaf schema, the
derived `z.infer` type, and header spreads. No new dependency; no OCPP
PDU/message-format change (HTTP transport header only).
The header is absent by default (knob unset, `false`, or empty string) and
over non-secure transport, so the default response behavior is unchanged.
Recommended production value behind a TLS-terminating reverse proxy or
native TLS: "max-age=31536000; includeSubDomains".
Closes only slice C of #1980; the identity-aware proxy mode, local-interface
spoofing guard, security audit CLI, and dangerously* naming slices remain open.
test: use shared constants and enums instead of hardcoded literals (#2040)
Replace hardcoded literals in recent tests with the shared constants/enums they duplicate (single source of truth, no behavior change; values are byte-equivalent):
- OCPP20 PostStopResurrection: GenericStatus.Accepted / ReportBaseEnumType.FullInventory instead of raw 'Accepted' / 'FullInventory'
- OCPP20 RequestStartTransaction: OCPP20ChargingRateUnitEnumType.A instead of 'A' as OCPP20ChargingRateUnitEnumType casts
- OCPP16 SmartCharging: TEST_ONE_HOUR_SECONDS instead of the 3600 duration literal
- UIHttpServer: ProcedureName.* instead of raw procedure-name strings
- Remove the now-unused TEST_PROCEDURES test constant (ProcedureName is the canonical source)
The deprecated HTTP UI transport emitted a synthetic `success` the instant the
request handler resolved `undefined` (the broadcast-pending signal), without
awaiting worker replies. PR #2020's Set-based aggregation therefore covered the
WebSocket and MCP transports but not HTTP: a fan-out command reported
`200`/`success` before any worker acted, and the aggregated
`hashIdsFailed`/`responsesFailed` never reached the client.
Align HTTP with the WebSocket and MCP transports (issue #2028 option 1): only
synchronous, non-broadcast procedures respond inline. A broadcast keeps its
already-registered response handler open so the later aggregated `sendResponse`
writes the real status and `hashIdsSucceeded`/`hashIdsFailed`, reusing the
existing 60s safety-net timeout (`UI_SERVER_BROADCAST_CHANNEL_REQUEST_TIMEOUT_MS`)
so a never-arriving aggregation yields a bounded failure with no hung socket.
Client-disconnect cleanup and server-side uuid minting are unchanged; no new
timeout or aggregation path is introduced.
This is a UI SRPC transport-layer fix only; no OCPP PDU/message format changes,
and no change to WebSocket/MCP behavior or the shared aggregation semantics.
feat(worker): terminate a deleted station's worker once it hosts zero elements (#2027) (#2038)
* feat(worker): terminate a deleted station's worker once it hosts zero elements (#2027)
Add an element-granular removeElement primitive to the worker abstraction and
wire it into the station-delete path so a deleted charging station's hosting
worker thread is terminated once it no longer hosts any station, without
disrupting sibling stations that share the worker (elementsPerWorker > 1).
- WorkerAbstract: new abstract removeElement(elementKey: PropertyKey).
- WorkerSet: internal PropertyKey -> WorkerSetElement map fed by an injected
generic elementKey selector; decrement numberOfWorkerElements and reuse a
factored terminateWorker helper (shared with stop()) to terminate only at
zero elements with no in-flight addition; purge the map on element removal.
- WorkerFixedPool/WorkerDynamicPool: documented no-op (poolifier exposes no safe
single-element eviction without destroying the worker and its siblings).
- Bootstrap: pass stationInfo => stationInfo.hashId; call removeElement from
workerEventDeleted.
Set-vs-pool decision: element-granular termination is workerSet-only; pool
worker threads are a bounded, reused resource and are not reclaimed per delete.
Cleanup/timeout contract: termination reuses the existing worker exit handler
for pending-promise rejection and set/map cleanup; any in-flight broadcast
request still resolves via the existing 60s aggregation timeout backstop.
Part A (#2031 cancellable reset) and bulk stop() are untouched.
* fix(worker): harden element-granular termination against concurrency races
Cross-validated review of the removeElement primitive surfaced two reachable
defects and one latent robustness issue, now fixed:
- Phantom-count leak on re-adding the same element key: numberOfWorkerElements
was incremented unconditionally, so a duplicate/re-added key inflated the
count and the worker was never terminated. Count is now key-aware (distinct
keys per worker); factored into trackAddedWorkerElement.
- New addition routed onto a terminating worker: getWorkerSetElement could
select a worker parked in terminateWorker, losing the message and rejecting
an unrelated add. Terminating workers are now flagged and excluded from
selection.
- getWorkerSetElementByWorker matched by threadId, which collapses to -1 after
terminate(); switched to worker object identity.
Tests: add coverage for the in-flight-sibling gate, drain-to-last-sibling,
add-during-terminate, same-key re-add, and unknown-key no-op; consolidate the
silent worker fixture into echoWorker via a `hold` flag. Mutation-verified.
* fix(worker): make element termination robust and tighten its types
Second cross-validated review round hardened the removeElement primitive:
- terminateWorker no longer awaits a re-attached 'exit' listener (redundant,
since worker.terminate() fulfils once the worker has exited, and it could
deadlock if 'exit' fired first). It now catches a rejected terminate()
(surfaced via the error event, not rethrown so the removal still succeeds)
and performs pending-promise rejection + set/map cleanup in a finally, closing
the zombie-on-reject leak, the orphaned-pending edge, and the stop-concurrent-
exit deadlock in one place.
- Drop the dead, semantically fictional migration branch in
trackAddedWorkerElement: duplicate live element keys cannot occur (identities
are deduplicated upstream), so a key is counted once per worker and a same-key
re-add is a no-op.
- Make WorkerSetElement.terminating a required boolean (initialized false) and
narrow the element key from PropertyKey to string, matching the sole caller
(hashId) and avoiding non-serializable keys.
Tests: add reject-path coverage (removeElement resolves and cleans up when
terminate() rejects; an in-flight addition is still rejected on stop when
terminate() rejects). Mutation-verified. All project gates green.
* test(worker): align WorkerSet internals view element key type to string
Coherence with the production narrowing of the element key from PropertyKey
to string; test-only, no runtime change.
* docs(worker): drop the worker-termination note from the README
Client-supplied WebSocket UI request ids were validated for format only.
A second request reusing a still-in-flight id overwrote the prior request's
response handler (cross-delivered reply, dropped second response) and its
broadcast tracking (leaked safety-net timer firing against the wrong context).
Guard the transport ingest: when responseHandlers already holds the request id,
reject the duplicate with a typed BaseError failure on its own socket instead of
overwriting the in-flight request. A completed request releases its handler, so a
legitimate sequential reuse of the same id is still accepted. HTTP and MCP mint
server-side UUIDs and are unaffected.
Wrap the ws.send in rejectInFlightRequestId() in try/catch, matching the
sendResponse() send discipline. The helper runs in the synchronous 'message'
listener, so an uncaught send throw would escape unhandled; log and swallow it
instead. No behavior change to the in-flight guard.
Signed-off-by: Jérôme Benoit <jerome.benoit@sap.com>
* test(ui-server): fail parseSentResponse with a descriptive assertion
When no message is captured at the requested index, fail with an explicit
assertion naming the index and sentMessages length instead of letting
JSON.parse throw an opaque SyntaxError. Use a length check rather than a
nullish guard, since MockWebSocket.sentMessages is typed string[] (indexed
access is not nullable under the project's tsconfig).
Signed-off-by: Jérôme Benoit <jerome.benoit@sap.com>
* test(ui-server): reuse shared UI-server test helpers and constants
Hoist emitWorkerResponse into UIServerTestUtils as the single source of truth
and consume it from both AbstractUIService and UIWebSocketServer tests, removing
the duplicated worker-response injection helper (with divergent argument order).
In the WebSocket test, build the protocol request via createProtocolRequest
instead of an inline tuple, and drop the redundant explicit 'ui0.0.1' argument
(it is createMockUIWebSocket's default).
fix(ui-server): dedup duplicate station identity on add (#2026) (#2032)
* fix(ui-server): dedup duplicate station identity on add (#2026)
hashId is a deterministic content hash of template identity fields + composed
chargingStationId, with no uniqueness check on add. Two stations from a
template with identical identity fields (e.g. fixedName: true) collide on one
hashId. AbstractUIServer.setChargingStationData was last-writer-wins by
timestamp keyed by hashId, so the twin silently overwrote the first station in
the UI registry; a targeted broadcast command then completed success on the
first worker's reply while the collided twin was orphaned.
Add post-creation identity dedup at the registry write choke point:
setChargingStationData returns a discriminated 'set' | 'stale' | 'collision'
outcome. A write whose hashId already maps to a station with a different
templateIndex is rejected as 'collision' without overwriting, guarding every
worker event (added/started/stopped/updated) uniformly. A restart re-emit
reuses the same templateIndex and still updates by timestamp, so there is no
regression. workerEventAdded logs the rejected collision at error level
instead of silently reporting the twin as added.
getHashId output is unchanged (byte-identical), so persisted <hashId>.json
configuration, the registry map key, broadcast routing and stats resolve
untouched. No OCPP PDU / message-format change; simulator station-registry
lifecycle only.
Tests: a twin (same hashId, different templateIndex) is rejected and does not
overwrite; a restart re-emit (same identity, newer timestamp) still updates; a
stale same-identity re-emit is dropped; a targeted broadcast reports no false
success after a collision. Mutation-verified: reverting the guard fails the
twin/false-success tests while the restart/stale tests stay green.
Duplicate-identity aggregation rework and hash-algorithm / per-instance-salt
changes are out of scope (per issue). Causally linked to #2027: an orphaned
twin is exactly the worker #2027 cannot individually terminate.
Closes #2026.
Signed-off-by: Jérôme Benoit <jerome.benoit@sap.com> Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
* fix(ui-server): discriminate station identity by (templateName, templateIndex) (#2026)
Strengthen the add-time identity dedup after a multi-reviewer cross-validation.
The initial guard discriminated on templateIndex alone, but getHashId does not
hash the template file name and the index space is per-templateName: two
identity-clone template files (copy + rename, both fixedName, same baseName and
identity fields) can produce the same hashId AND the same templateIndex. The
templateIndex-only guard treated them as the same station and silently
overwrote one, reintroducing the last-writer-wins bug for cross-template
collisions.
Discriminate on the full physical-station identity, the (templateName,
templateIndex) pair: a cached entry differing in either field is a collision. A
legitimate restart re-emit keeps both fields and still updates by timestamp (no
regression). Sync the SetChargingStationDataOutcome doc to the pair, add
templateName to the rejection log so an equal-index clone collision is
diagnosable, and add a test for the same-templateIndex / different-templateName
case.
Mutation-verified: reverting the templateName clause fails only the new
clone-file test while the same-template twin, restart, stale, and
false-success tests stay green.
Signed-off-by: Jérôme Benoit <jerome.benoit@sap.com> Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
---------
Signed-off-by: Jérôme Benoit <jerome.benoit@sap.com> Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
fix(charging-station): cancel a pending reset when the station is deleted (#2027) (#2031)
reset() stopped the station, slept resetTime, then re-initialized and
reconnected without checking whether the station had been deleted during
the sleep. A station deleted mid-reset was resurrected and reconnected to
the CSMS (the "zombie" reconnect that triggers #2017).
Add an instance AbortController tripped by delete(); reset() now awaits
interruptibleSleep(resetTime, signal) and rechecks the aborted state
before re-initializing, bailing out cleanly on abort. The dispose signal
is kept distinct from started/stopping, which reset() itself toggles via
stop() and therefore cannot be used to detect deletion.
The OCPP Reset response stays Accepted and the handler still invokes
reset() fire-and-forget; only the reset/delete lifecycle changes.
Part B (element-granular worker-thread termination) remains open.
Daniel [Fri, 17 Jul 2026 22:17:54 +0000 (00:17 +0200)]
fix(ui-server): make UI broadcast-channel commands complete reliably (#2018) (#2020)
* fix(ui-server): time out broadcast-channel requests so UI commands cannot hang
A UI control command (start/stop/delete a station) is dispatched over the
worker broadcast channel and the UI service waits for a fixed number of
worker responses, sampled at send time. If a targeted worker never replies
-- e.g. the station is deleted while the command is in flight -- that count
is never reached, so the request is never completed or released and the
client waits forever. Read commands keep working, which masks the wedge.
Arm a per-request safety-net timeout when a broadcast-channel request is
sent. On expiry the request is completed with a failure (reporting the
charging stations that did reply successfully) and both the request and
response aggregation state are released, so the caller gets an answer
instead of hanging. The timeout is cleared on normal completion, on a
dispatch failure, and on service stop.
Closes #2018.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Daniel <7558512+DerGenaue@users.noreply.github.com>
* fix(ui-server): complete broadcast requests by target set and reconcile on station deletion (#2018)
Broadcast-channel request completion was gated on a frozen integer count
sampled at send time, blind to which stations were targeted. When the set
of stations changed mid-flight the aggregator could not reconcile the
drift, producing failure modes on top of the hang the safety-net timeout
already backstops:
- Hang until timeout when a targeted station is deleted mid-flight (the
count is never reached).
- Late double-completion: after release the count reads 0, so a late reply
re-enters and 1 >= 0 re-completes the request.
- False success: an empty explicit hashIds array degraded to a
broadcast-to-all instead of failing.
Track the outstanding responders as a Set of target hashIds and make the
Set the single source of truth for completion:
1. Snapshot the resolved targets (validated explicit hashIds, or the live
station set for a broadcast) into the request context; complete when the
Set empties. AbstractUIServer.deleteChargingStationData fans out
AbstractUIService.reconcileDeletedStation, which drops the departed
hashId from every in-flight request and completes any that empty with
the truthful aggregated payload. A DELETE_CHARGING_STATIONS request is
left untouched by reconciliation: its targets self-delete yet still post
their command reply on a separate transport, so that reply (not the
racing `deleted` event) is the completion source; a worker that dies
mid-delete is covered by the timeout.
2. Drop untracked responses (unknown/released request, or a hashId that is
not an outstanding responder) in the response handler, closing the late
double-completion re-entry. A reply without a hashId for a still-tracked
request is dropped and logged distinctly so the resulting timeout is
diagnosable.
3. Reject unknown and empty explicit targets instead of degrading to a
broadcast-to-all.
The 60 s safety-net timeout stays as a pure backstop for genuine worker
crash/deadlock; normal and reconciled completion clear its timer. The
completion accessor is named getBroadcastChannelOutstandingResponseCount to
reflect that it returns the responses still outstanding, not a frozen total.
Duplicate-identity false success and orphan-worker termination are not
addressed here (a hashId-keyed set cannot disambiguate two workers sharing
an identity) and are deferred to follow-up changes. The deprecated HTTP
transport bypasses aggregation and is likewise out of scope.
Tests cover reconcile-on-delete, the untracked-response guard, unknown and
empty target rejection, the DELETE self-target ordering (reconcile-first
completes via the reply; no reply falls back to the timeout), reconcile
that does not empty the set, two concurrent requests reconciled by one
deletion, a hashId-less reply, and the previously untested timeout
behaviors (partial-success payload, dispatch-failure and stop() timer
clearing, and the completion-race guard).
Closes #2018.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Jérôme Benoit <jerome.benoit@sap.com>
---------
Signed-off-by: Daniel <7558512+DerGenaue@users.noreply.github.com> Signed-off-by: Jérôme Benoit <jerome.benoit@sap.com> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Co-authored-by: Jérôme Benoit <jerome.benoit@sap.com>
fix(ocpp): reject charging profiles by EVSE existence and isolate non-persistent station configuration (#2022, #2024) (#2025)
* fix(ocpp): reject charging profiles by EVSE existence, not EVSE count (#2022)
OCPP 2.0 validateChargingProfile() guarded evseId with a count comparison
(evseId > getNumberOfEvses()). getNumberOfEvses() counts EVSEs (excluding
EVSE 0) and is not a maximum id, so a station with non-contiguous EVSE ids
{0,1,3} false-rejected a valid profile for EVSE 3 (3 > 2). Use the existing
hasEvse() existence check instead, mirroring the OCPP 1.6 hasConnector twin,
and align the message to "EVSE does not exist".
RequestStartTransaction carries the TxProfile (F01.FR.08-10 / F02.FR.16-18);
existence-based rejection mirrors K01.FR.28 (conformance test TC_K_14_CS).
Test: a charging profile for the existing, non-contiguous EVSE 3 is accepted
(a count-based guard would reject it, 3 > 2). Mutation-verified: reverting the
guard makes the test fail.
getOcppConfigurationFromTemplate() returned the SharedLRUCache-cached
template's Configuration by reference. For non-persistent stations
(ocppPersistentConfiguration=false) this aliased the shared configurationKey
array, so an in-place setConfigurationKeyValue() mutation polluted the cached
template. Clone the Configuration, mirroring the existing connector/EVSE clone
pattern (clone(undefined) === undefined).
Test: a non-persistent station's Configuration is an independent copy of the
cached template, and mutating a key on it leaves that cached template unchanged.
Mutation-verified: reverting the clone makes the test fail. Retires the
now-redundant SharedLRUCache-clearing afterEach workaround in
ChargingStation-ResetIdentity.test.ts (file still passes).
Daniel [Fri, 17 Jul 2026 17:10:37 +0000 (19:10 +0200)]
fix(charging-station): retain creation options so a reset keeps station identity (#2019)
* fix(charging-station): retain creation options so a reset keeps station identity
On an OCPP Reset (and on a template-file reload) the station was
re-initialized via initialize() with no arguments, dropping the
creation-time options. The restarted station reverted to the template
defaults, losing its fixed identity (reappearing as CS-BASIC-000N with a
new hashId) and its configured supervision URL; the original station
never came back.
Retain the creation options on the instance and re-pass them in reset()
and in the template-watcher reload. setSupervisionUrl() also updates the
retained snapshot so a runtime supervision-URL change survives a reboot
(but not a full simulator restart, which reconstructs the worker from the
original options). The snapshot is in-memory only: it is never persisted
and never written to the template.
Closes #2017.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Daniel <7558512+DerGenaue@users.noreply.github.com>
* fix(charging-station): clone the retained creation options
setSupervisionUrl updates the retained options in place so a later reset
re-applies the current supervision URL. Clone them at construction so that
in-place update never mutates the shared worker data passed from the worker
thread.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Daniel <7558512+DerGenaue@users.noreply.github.com>
* test(charging-station): cover identity retention across a reset
A non-persistent station keeps its configured identity across a reset only
when the creation options are re-applied; a persistent one restores it from
its saved configuration without them. This locks the behavior and scopes the
options fix to the non-persistent case.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Daniel <7558512+DerGenaue@users.noreply.github.com>
* refactor(charging-station): re-apply retained options on reset only when non-persistent
Address review feedback on the identity-retention fix:
- Rename the retained-options field to creationOptions (clearer; no longer
shadows the options parameters).
- Re-apply the creation options on reset/template-reload only for a
non-persistent station, via the reinitializeOptions getter. A persistent
station restores from its saved config, which stays the source of truth, so
re-applying them would needlessly override it (and collapse an OCPP-config
station's supervision URL). The unconditional setSupervisionUrl mirror is kept,
with a sharpened comment explaining why it must run for both branches.
- Cover the reset -> initialize wiring (both persistence modes) and the
setSupervisionUrl-across-reset retention with unit tests.
No change to the fixed behavior: a reset still keeps the configured identity.
* refactor(charging-station): factor setSupervisionUrl credential updates into a helper
Deduplicate the supervisionUser/supervisionPassword null-checks that were
applied twice (to stationInfo and to the retained creation options) into a
single applyCredentials() closure. No behavior change.
* test(charging-station): cover OCPP-config supervision URL retention across reset
- Add a regression test for the non-persistent supervisionUrlOcppConfiguration
path: after setSupervisionUrl(), a real reset() must still dial the new URL
(the branch the setSupervisionUrl mirror comment reasons about, previously
untested).
- Prefix all test titles with "should" per tests/TEST_STYLE_GUIDE.md.
- Clear the real SharedLRUCache singleton in afterEach so a cached template
cannot bleed between tests.
- Support optional template field overrides in the makeTemplate helper.
* refactor(charging-station): rename reinitialization getter and fix its mirror comment
- Rename the reinitializeOptions getter to reinitializationOptions (noun form,
consistent with the wsConnectionUrl/hasEvses getters).
- Correct the setSupervisionUrl mirror comment: the retained-options mirror is
load-bearing on a cache-cold reinitialization (template reload), not on a plain
warm-cache reset() (whose in-place OCPP-key cache mutation already carries the
URL). No behavior change.
* test(charging-station): add a genuine OCPP-config supervision-URL reload guard
The previous OCPP-config test drove a warm-cache reset() and passed even with the
retained-options mirror deleted (a false guard): the cached template still carried
the mutated OCPP key. Add a cache-cold reload test (clear SharedLRUCache, then
reinitialize) that re-seeds the key from configuredSupervisionUrl and therefore
fails if the mirror is removed. Relabel the warm-cache reset test and correct its
comment to reflect that it exercises the cache-mutation path, not the mirror.
* docs(charging-station): tighten the setSupervisionUrl mirror comment
Condense the retained-options mirror comment (~11 -> 8 lines) while preserving
every load-bearing detail: it runs for both branches, a cache-cold reload
re-seeds the OCPP key from configuredSupervisionUrl, a warm reset() survives via
the cached template mutation, and the mirror is in-memory only.
---------
Signed-off-by: Daniel <7558512+DerGenaue@users.noreply.github.com> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Co-authored-by: Jérôme Benoit <jerome.benoit@sap.com>
Daniel [Fri, 17 Jul 2026 15:22:18 +0000 (17:22 +0200)]
fix(charging-station): re-dial after a server-initiated connection close (#2021)
* fix(charging-station): re-dial after a server-initiated connection close
When the CSMS or a proxy closed a station's WebSocket with a normal
(clean) close, the station stayed "started but not connected" instead of
reconnecting. onClose only re-dialed on abnormal close codes and treated
clean closes as terminal, but a clean close cannot be told apart by code
from the station's own closeWSConnection() (the UI disconnect action,
which must stay down). Real charge-point hardware re-dials after losing
the connection regardless of the close code.
Mark only explicitly-requested closes (the UI disconnect) as terminal via
a byRequest flag on closeWSConnection(), and reconnect on any close the
station did not request while it is still started -- clean or abnormal.
This also fixes certificate rotation, which closes the socket to force a
re-dial with the new certificate and previously never reconnected.
Closes #2016.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Daniel <7558512+DerGenaue@users.noreply.github.com>
* refactor(charging-station): pass closeWSConnection intent via an options object
Match the paired openWSConnection signature and make the call site
self-documenting: closeWSConnection({ byRequest: true }) rather than a bare
positional boolean.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Daniel <7558512+DerGenaue@users.noreply.github.com>
* test(charging-station): cover the onClose reconnect decision
Drive onClose directly with a spied reconnect: the station re-dials after a
server-initiated normal close while started, and stays disconnected after an
operator-requested close. Removes a stale test that only asserted the socket
reached CLOSED, which held regardless of the reconnect decision.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Daniel <7558512+DerGenaue@users.noreply.github.com>
* docs(charging-station): harmonize reconnect terminology and tighten close comments
- Use the codebase's "reconnect" term consistently (drop "re-dial"), and
"server-initiated"/"requested" to match the PR title and the byRequest option.
- Tighten the closeWSConnection JSDoc and onClose comments; align the JSDoc
@param style (no trailing period) with the sibling openWSConnection.
- Prefix the reconnect test titles with "should" per tests/TEST_STYLE_GUIDE.md.
No behavior change.
---------
Signed-off-by: Daniel <7558512+DerGenaue@users.noreply.github.com> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Co-authored-by: Jérôme Benoit <jerome.benoit@sap.com>
fix(ocpp): support TriggerMessage(TransactionEvent) per F06.FR.07 (#2013)
TriggerMessage(requestedMessage=TransactionEvent) returned NotImplemented
although TransactionEvent is a fully implemented outgoing message, violating
OCPP 2.0.1 F06.FR.07/08.
Wire the trigger to the existing TransactionEvent send path across the two
touchpoints:
- handleRequestTriggerMessage: add a TransactionEvent case that validates the
evse, returns Accepted when an ongoing transaction exists in scope, else
Rejected (F06.FR.05); it no longer falls through to NotImplemented.
- TRIGGER_MESSAGE dispatch listener: add a TransactionEvent case that sends a
TransactionEventRequest(eventType=Updated, triggerReason=Trigger) with the
TxUpdatedMeasurands meterValue for each active transaction in scope; when the
evse field is absent it fans out to all EVSEs (F06.FR.11).
Reuses sendTransactionEvent and the TxUpdatedMeasurands meter-value builder;
chargingState is populated by buildTransactionEvent for Updated events. The 6
existing trigger cases are unchanged.
- Backtick 49 JSDoc @returns boolean literals across 17 files (lowercase True/False).
- Align !== undefined -> != null on number|undefined guards in OCPP20IncomingRequestService.ts (13 tokens); conditional-spread inclusion guards preserved as !== undefined.
- Extend != null idiom to OCPP20VariableManager min/max ternaries + presence guards and the negated EVSE guard.
- Fix OCPP20AuthAdapter.validateConfiguration @returns (synchronous boolean; was falsely 'Promise resolving to').
Gap C (String(previousRequestId) -> .toString()) dropped: premise false — operand is number|undefined, .toString() would fail typecheck (TS18048); String() is the only behavior-preserving form.
All gates green (format/typecheck/lint/build/test, fail=0). Reviewed across multiple multi-agent rounds with cross-validation.
refactor(ocpp): remove as-unknown-as double-casts from request/response dispatch (#2003)
Give each builder its precise input contract instead of laundering casts
through phantom generics or JsonType:
- buildRequestPayload drops its unsound <Request> generic and returns honest
JsonType; each dispatch branch narrows with a single cast at a genuine
JsonType boundary.
- Builder inputs (StatusNotificationOptions, OCPP20TransactionEventOptions,
SignCertificateOptions) are JsonObject subtypes; StatusNotification build
validates connectorStatus via the shared isOCPP20ConnectorStatus guard.
- Handler bridges (toRequestHandler/toResponseHandler) use a single cast that
preserves the handler's async identity, fixing an unawaited-handler
regression from a plain wrapper defeating isAsyncFunction.
- CAT-C string-to-enum narrowing becomes a type guard / typed Record.
refactor(utils): swap getRandomFloat(Rounded) params to (min, max) for consistency (#1998)
Swap getRandomFloat and getRandomFloatRounded params from (max, min[, scale]) to (min, max[, scale]) for consistency with the sibling bounds helpers randomInt (node:crypto) and isValidRandomIntBounds; migrate all 11 production + 5 test call sites; harmonize JSDoc across the random-float family.
Bundles a correctness fix: getRandomFloat now rejects a non-finite interval width (max - min), closing a latent overflow where finite endpoints such as (-MAX_VALUE, MAX_VALUE) returned Infinity/NaN, silently violating the documented [min, max] contract.
Internal API refactor: package.json exports only ./dist/start.js, so getRandomFloat is app-internal and this is not a semver-major public break.
fix(ocpp): guard OCPP 1.6 UpdateFirmware deferred-timer schedule against sealed state writes (#1994)
The .on(UPDATE_FIRMWARE, ...) listener's deferred branch obtains state via
getOrCreateStationState() and then writes stationState.deferredFirmwareUpdateTimer
= setTimeout(...). After PR #1992 sealed the base-plumbing behavior,
getOrCreateStationState() returns the stopped state instead of lazy-initializing
a fresh entry once stop() has marked stationsState.get(cs)?.stopped === true. A
late UPDATE_FIRMWARE dispatch after ChargingStation.stop() runs
ocppIncomingRequestService.stop(this) at ChargingStation.ts:1273 (before
this.started = false at :1280) therefore writes a fresh Timeout onto the sealed
state.
The callback body is safe (unref'd, checkChargingStationState gate), but the
write is inconsistent with the OCPP 2.0.1 GUARD sites shipped in PR #1992
(getCertSigningRetryManager, sendSecurityEventNotification,
simulateFirmwareUpdateLifecycle, simulateLogUploadLifecycle) and leaves a Timeout
closure-referenced entry pending in Node's queue until GC reclaims the
ChargingStation.
Insert a stopped GUARD immediately after getOrCreateStationState, before
cancelDeferredFirmwareUpdate — mirroring the exact syntax of the OCPP 2.0.1
GUARD sites (bare 'return' after 'if (stationState.stopped === true) {'). The
GUARD short-circuits the schedule branch before any write reaches the sealed
state.
Add a regression test covering the post-stop() deferred-schedule path in
tests/charging-station/ocpp/1.6/OCPP16IncomingRequestService-Firmware.test.ts.
References:
- PR #1992 — source of the 'stopped' sentinel + OCPP 2.0.1 GUARD sites
- PR #1984 — source of 'deferredFirmwareUpdateTimer'
This is purely additive; no observable OCPP behavior change (the pre-existing
fire-time checkChargingStationState gate inside the setTimeout callback remains
as defense-in-depth).
fix(ocpp): guard post-stop() handler dispatch against WeakMap resurrection (#1992)
Closes #1970
OCPP 2.0.1 handlers dispatched from queued microtasks, unref'd retry
timers, or EventEmitter-dispatched `.on(...)` listeners could run
AFTER `stop()` and resurrect the `stationsState` WeakMap entry via
`getOrCreateStationState` lazy-init. `sendQueuedSecurityEvents` also
scheduled a retry `setTimeout(...)` whose handle was never stored, so
`stop()` had no way to cancel a pending retry, and the recursive
retry callback re-entered `sendQueuedSecurityEvents` — resurrecting
state.
Three coordinated parts.
Part 1 — base plumbing (`OCPPIncomingRequestService.ts`):
- Generic bound widened to
`TStationState extends { stopped?: boolean } = { stopped?: boolean }`.
- `stop()` template: after `resetStationState`, marks
`stationState.stopped = true` instead of `stationsState.delete(cs)`.
Deletion would re-enable resurrection via `getOrCreateStationState`
lazy-init on any late dispatch. Keeping the sealed entry lets the
getter return the sealed state and raw `.get()` null-guarded callers
observe the marker and drop. The WeakMap entry is naturally
collected when the ChargingStation reference is dropped.
- `getOrCreateStationState` returns the sealed stopped state when
`state?.stopped === true`; no fresh entry is lazy-init'd post-stop.
- Both concrete state interfaces (`OCPP16StationState`,
`OCPP20StationState`) get an alphabetized `stopped?: boolean` field.
Part 2 — OCPP 2.0.1 call-site audit (11 sites):
Two sites converted to raw `stationsState.get(cs)` + `stopped === true`
null-guard, silent-drop:
- `sendNotifyReportRequest` — dispatched from
`.on(GET_BASE_REPORT).catch(...)` microtask; state usually created
by sibling `handleRequestGetBaseReport` but external `emit(...)`
paths and post-stop dispatch are covered by the null-guard.
- `sendQueuedSecurityEvents` — dispatched from unref'd retry
`setTimeout`; guard is the last line of defense against a
fired-before-cancel race.
Four sites use `getOrCreateStationState` + explicit `stopped` check
(state may not exist yet on the pre-stop happy path):
- `sendSecurityEventNotification` — lifecycle-entry on the
invalid-cert-on-first-request edge case
(`handleRequestCertificateSigned` X.509-invalid branch,
`handleRequestUpdateFirmware` invalid-PEM branch).
- `getCertSigningRetryManager` — public accessor; return type widened
to `OCPP20CertSigningRetryManager | undefined`; two callers
(`handleRequestCertificateSigned`, `OCPP20ResponseService`) updated
with `?.` to tolerate the undefined return. Without this guard, a
late `handleResponseSignCertificate` dispatch (a SignCertificate
response can arrive between `ocppIncomingRequestService.stop(cs)`
and `closeWSConnection()` in `ChargingStation.stop()`) would
materialize a fresh manager on the sealed state and schedule a
non-`.unref()`'d retry `setTimeout` holding the ChargingStation
reference.
- `simulateFirmwareUpdateLifecycle` — dispatched from
`.on(UPDATE_FIRMWARE).catch(...)` microtask; guards against driving
the full async lifecycle on a sealed state (which would send OCPP
requests through a closed WebSocket and clobber the abort-controller
fields).
- `simulateLogUploadLifecycle` — symmetric,
`.on(GET_LOG).catch(...)`.
Part 3 — retry timer discipline in `sendQueuedSecurityEvents`:
- Store the retry `setTimeout` handle on
`OCPP20StationState.securityEventRetryTimer`.
- Cancel any previously scheduled retry before scheduling a new one.
- Self-clear the handle before the recursive
`sendQueuedSecurityEvents` call inside the callback.
- New `cancelSecurityEventRetryTimer` helper is called from
`resetStationState` (after abort-controller aborts and cert-signing
retry cancel, before the two `resetActive*State` helpers).
- `.unref()` preserved.
Design rationale — silent-drop over `StationStoppedError` throw:
matches the "post-stop silently drop" semantic and avoids catch-block
sprawl at every handler.
Design rationale — return `undefined` from `getCertSigningRetryManager`
over convert-with-explicit-null-manager: two callers already handle
optional chaining cleanly; return-type widening is the minimum change.
Verification: pnpm format / typecheck / lint pass; pnpm test 3029
pass, 0 fail; pnpm build exit 0. No `stationsState.delete` remains in
`src/`; `securityEventRetryTimer` lives in exactly three logical
places (interface field, retry-site store + self-clear, cancel helper
called from `resetStationState`).
fix(ocpp): implement OCPP 1.6 GetDiagnostics supersession via AbortController (#1991)
Closes #1971
The entry guard added by PR #1987 at handleRequestGetDiagnostics
prevented concurrent FTP uploads racing on the same
${chargingStationId}_logs.tar.gz archive by silently dropping the
second GetDiagnostics.req with an empty GetDiagnostics.conf. That
mitigated the concrete race but was not the OCPP 1.6 supersession
semantic: the new request needed to abort the in-flight upload and
start a fresh one, with a terminal DiagnosticsStatusNotification
emitted for the superseded upload.
Refactor the entry guard from skip-second into abort-then-install:
- Add `activeDiagnosticsAbortController?: AbortController` to
`OCPP16StationState` (alphabetical, above the existing fields).
- On supersession, the entry guard calls
`stationState.activeDiagnosticsAbortController?.abort()` and
installs a fresh controller. `diagnosticsUploadInProgress` stays
`true` across the handoff so the §4.4 / §7.24 trigger cross-check
never observes a false-idle window.
- `basic-ftp` v6 does not natively consume `AbortSignal`; the
documented interruption path is `Client.close()`, which invokes
`FtpContext.close()` and rejects the pending `uploadFrom` task
with `User closed client during task`. The handler wires
`signal.addEventListener('abort', () => ftpClient?.close(), { once: true })`.
- The existing `catch` at L1390 already emits
`DiagnosticsStatusNotification(UploadFailed)` for any thrown error,
so a close-triggered rejection flows through unchanged — no second
emission needed. OCPP 1.6 `DiagnosticsStatusNotification.req` (§6.17)
does not carry a `requestId`; the terminal for the superseded upload
is uncorrelated by design, only its temporal position tells the CSMS
which upload it belonged to.
- The `finally` clause identity-guards its cleanup
(`if (stationState.activeDiagnosticsAbortController === abortController)`)
so a superseded handler's late unwind cannot clobber the new
lifecycle's state. Cleaner than the microtask-yield primitive
sketched in the initial design because it removes the ordering
dependency between the two handlers' async continuations.
- `resetStationState` aborts the in-flight controller before the base
template deletes the WeakMap entry, following the cancel-before-delete
ordering documented on `OCPP20IncomingRequestService.resetStationState`.
Mirrors the OCPP 2.0.1 log-upload supersession pattern (commits 29a8330f, ac6ed218, 64f384fa, be29b4c8) but does not import
`AcceptedCanceled` — that response semantic is a 2.0.1 concept absent
from OCPP 1.6 `GetDiagnostics.conf` (§6.26).
fix(ocpp): cross-check OCPP 1.6 lifecycle flags in trigger, re-entry, and simulation entry (#1987)
The OCPP 1.6 TriggerMessage handler cases for DiagnosticsStatusNotification and
FirmwareStatusNotification read stationInfo.diagnosticsStatus and
stationInfo.firmwareStatus directly. When an exception, abort, or lifecycle
drift left stationInfo.*Status at a stale non-terminal value, the trigger
faithfully reported that stale value to the CSMS instead of Idle.
The identical anti-pattern was present in handleRequestUpdateFirmware's re-entry
guard (which suppressed legitimate new UpdateFirmware.req when
stationInfo.firmwareStatus was stuck at Downloading / Downloaded / Installing)
and in the base-class event dispatcher's unconditional fan-out of
UPDATE_FIRMWARE emits (which spawned duplicate concurrent
updateFirmwareSimulation invocations emitting duplicate progress notifications
to the CSMS).
Track lifecycle progress with two per-station boolean flags on
OCPP16StationState (diagnosticsUploadInProgress, firmwareUpdateInProgress), set
on lifecycle entry and cleared in a finally block on every exit path (happy,
return, throw). The flags are consumed at four sites: the two TriggerMessage
cases, the handleRequestUpdateFirmware re-entry guard, and the
updateFirmwareSimulation entry guard.
OCPP 1.6 GetDiagnostics.req and UpdateFirmware.req core-profile payloads do not
carry a requestId (unlike OCPP 2.0.1 GetLog and UpdateFirmware), so lifecycle
progress is tracked with boolean flags rather than requestId-presence
sentinels.
Satisfies OCPP 1.6 SHALL clauses in section 4.4 (Diagnostics) and section 4.5
(Firmware), and the Idle enumeration definitions in section 7.24 and
section 7.25.
fix(ocpp): cancel deferred OCPP 1.6 firmware setTimeout on stop() (#1984)
The OCPP 1.6 UPDATE_FIRMWARE event listener schedules
updateFirmwareSimulation via setTimeout(...).unref() when retrieveDate
is in the future. .unref() prevents the timer from blocking process
exit, but the callback still fires after stop(): if the station
restarts, the deferred simulation runs against the new connection and
can emit FirmwareStatusNotification messages the CSMS did not request.
Store the timer handle on OCPP16StationState.deferredFirmwareUpdateTimer
(per-station state introduced by #1963 / PR #1983) and release it via
a shared cancelDeferredFirmwareUpdate helper called from both the
schedule site (to supersede a prior pending schedule) and
resetStationState (invoked by the inherited stop() template before the
base deletes the WeakMap entry). The callback clears the handle before
awaiting updateFirmwareSimulation so resetStationState never targets a
fired timer.
Mirrors the OCPP 2.0.1 pattern
(OCPP20IncomingRequestService.resetStationState cancels
certSigningRetryManager.cancelRetryTimer, commit c0b25553).
refactor(ocpp): extract per-station state plumbing into shared OCPPIncomingRequestService base (#1983)
Closes #1963. Companion issues #1971, #1972, #1973 will consume this base.
Pure structural refactor + OCPP 1.6 concretization. Zero observable
behavior change on OCPP 2.0.1: every field currently cleared in the
former OCPP20IncomingRequestService.stop() body is still cleared, in
the same order, at the same lifecycle point.
Touchpoints:
A. src/charging-station/ocpp/OCPPIncomingRequestService.ts
Base becomes generic <TStationState extends object = object>. Adds
shared 'stationsState' WeakMap (L86), 'getOrCreateStationState'
lazy-init, concrete 'stop()' template (bound in the constructor at
L94 to prevent detach-and-call regressions), and abstract hooks
'createStationState' / 'resetStationState'. Documents the
exception-safety contract on the template: a throw in
'resetStationState' skips WeakMap eviction and any subclass
extension after 'super.stop()' — matches pre-refactor semantics.
The '= object' default on the generic parameter is load-bearing
(removing it would break the static registry Map and the
getInstance constraint with TS2314).
B. src/charging-station/ocpp/2.0/OCPP20IncomingRequestService.ts
Extends OCPPIncomingRequestService<OCPP20StationState>. Removes
local 'stationsState' field and 'getOrCreateStationState'. Adds
'createStationState' factory and 'resetStationState' override with
the 5-statement ordering invariant of the pre-refactor stop() body.
The abort-before-clear ordering matters because clearing first
makes the subsequent '?.abort()' short-circuit on the nulled field,
leaving the in-flight operation un-signaled. 'stop()' override
calls super first, then keeps the unconditional
OCPP20VariableManager cleanup. Class-level JSDoc documents OCPP 2.1
subclass path (extends OCPP20IncomingRequestService inherits state,
reset, and stop() unchanged; widening the generic parameter for new
fields requires a preparatory refactor with a factory cast per
TS2352, or subclass override of createStationState).
C. src/charging-station/ocpp/1.6/OCPP16IncomingRequestService.ts
Adds empty 'OCPP16StationState' interface. Extends
OCPPIncomingRequestService<OCPP16StationState>. Adds no-op
'createStationState' + 'resetStationState' overrides. Removes the
'/* no-op for OCPP 1.6 */' stop() override (now inherited from the
base template). The 'resetStationState' JSDoc prescribes the
abort-before-clear ordering companion issues MUST follow.
D. eslint.config.js
Adds 'no-restricted-syntax' rule enforcing the INVARIANT: forbids
direct '.set/.delete/.clear' calls on 'stationsState' from outside
the base class file, forbids aliasing 'stationsState' to a local
binding, and forbids destructuring 'stationsState'. Covers the AST
escape hatches identified during hostile-adversarial review.
Enforced by 'pnpm lint' in CI on every companion PR.
E. src/charging-station/ocpp/OCPPServiceUtils.ts
Adds bidirectional cross-reference in the comment on
'warnedInvalidMeasurands' distinguishing the module-scope warn-once
diagnostic cache from the class-scope lifecycle-state WeakMap on
OCPPIncomingRequestService.stationsState.
Companion issues consume this base infrastructure in order:
- #1971 GetDiagnostics supersession -> activeDiagnosticsAbortController + Id
- #1972 firmware setTimeout cancel -> deferred firmware timer handle
- #1973 trigger cross-check -> activeFirmwareUpdateRequestId
#1971 and #1973 share 'activeDiagnosticsRequestId': whichever lands
first adds the (optional) field to OCPP16StationState; the second
lands as-is.
Origin of the OCPP 2.0.1 pattern being harmonized:
- 47cdf2bbe refactor(ocpp20): isolate per-station state with
WeakMap instead of singleton properties (introduces the WeakMap
pattern with initial names 'OCPP20PerStationState' / 'stationStates'
/ 'getStationState')
- f1e33ea42 refactor(ocpp20): harmonize per-station state naming
(renames to 'OCPP20StationState' / 'stationsState')
- 17396c1c4 refactor(ocpp20): rename getStationState to
getOrCreateStationState (lazy-getter naming split)
- c0b25553 fix(ocpp20): cancel cert-signing retry timer in stop()
- be29b4c8 fix(ocpp20): add AbortController to log-upload lifecycle
+ stop() abort
- d2e9b8b0 refactor(ocpp20): extract resetActiveFirmwareUpdateState
helper
- ce875dae refactor(ocpp20): harmonize firmware-state cleanup + log
superseded
Tests: 7 new plumbing tests in
tests/charging-station/ocpp/OCPPIncomingRequestService-StationState.test.ts
covering lazy-init idempotency (with createStationState spy verifying
call count = 1 after two getOrCreate calls), WeakMap eviction on
stop(), resetStationState invocation count, no-op guard when no state
exists, two-station isolation, the OCPP 1.6 default resetStationState
no-op (with reference identity check), and the exception-safety
contract lock (throw in resetStationState skips WeakMap.delete, and
subsequent stop() re-invokes on same state before evicting). Zero
edits to existing OCPP 2.0.1 tests.
The OCPP 2.0.1 5-statement ordering invariant is enforced by four
independent mechanisms: (1) JSDoc rationale on
OCPP20IncomingRequestService.resetStationState documenting the
'?.abort()' short-circuit consequence of reordering, (2) the
'no-restricted-syntax' ESLint rule preventing direct 'stationsState'
mutations from subclass code (with selector coverage for aliasing and
destructuring bypasses; enforced in CI), (3) constructor
'stop.bind(this)' defense-in-depth against detach-and-call
regressions, and (4) byte-identical preservation of the pre-refactor
stop() body.
fix(ocpp): drive FirmwareStatusNotification trigger from last-sent notification (L01.FR.26) (#1981)
TriggerMessage(FirmwareStatusNotification) was gated on
`activeFirmwareUpdateRequestId != null && hasFirmwareUpdateInProgress()`,
falling back to Idle otherwise. Since `hasFirmwareUpdateInProgress` returns
false for every terminal status (`DownloadFailed`, `InvalidSignature`,
`InstallationFailed`, `InstallVerificationFailed`, `Installed`), the
station returned Idle after every non-Installed terminal, violating OCPP
2.0.1 L01.FR.26 (SHALL, mirrored by L02.FR.17).
Persist the last-sent `(requestId, status)` on `OCPP20StationState` and
drive the trigger from it. `stationInfo.firmwareStatus` is retained
because `hasFirmwareUpdateInProgress` and its 3 consumers (reset
rejection, isChargingStationIdle, isEvseIdle) depend on its
"in-progress predicate" semantics — deliberate dual-source, not
conflation.
Add `src/assets/ev-profiles*.json` with negation for
`ev-profiles-template.json`, alongside the existing `config` and
`idtags` patterns. The three pairs are semantically identical:
user-mutable local instance + committed template.
The `ev-profiles.json` user file was previously untracked and
surfaced as noise in `git status`; the template
(`ev-profiles-template.json`) is documented in the README as the
canonical starting point for the `evProfilesFile` template field
consumed by the coherent MeterValues generator.
Verified with `git check-ignore -v`:
- `src/assets/ev-profiles.json` → matched by .gitignore:7
- `src/assets/ev-profiles-template.json` → NOT ignored (negation)
fix(ocpp): reset stationInfo.firmwareStatus on abort/exception in simulateFirmwareUpdateLifecycle (#1976)
simulateFirmwareUpdateLifecycle wrote stationInfo.firmwareStatus at every
status transition via sendFirmwareStatusNotification, but its finally block
only cleared the per-station WeakMap state (activeFirmwareUpdateAbortController,
activeFirmwareUpdateRequestId). On exception or abort (supersession, stop()),
stationInfo.firmwareStatus stayed at the last-emitted non-terminal value
(e.g. 'Downloading'), leaving a latent data-model split any future
consumer that does not cross-check activeFirmwareUpdateRequestId would
observe.
Track the current lifecycle stage inside the lifecycle scope and, in the
existing finally, reset stationInfo.firmwareStatus in-place when the
lifecycle did not reach Installed. Per OCPP 2.0.1 FirmwareStatusEnumType,
Idle SHALL only be used in TriggerMessage-triggered notifications, so as
a persistent local state Idle is only valid before install starts:
- clean download-phase abort -> Idle
- clean install-phase abort -> InstallationFailed
- exception during download stage -> DownloadFailed
- exception during install stage -> InstallationFailed
Per OCPP 2.0.1 L01.FR.24 Note the terminal *Failed FirmwareStatusNotification
on supersession is optional; L02.FR.15 Note omits any such clause. Emitting
no FirmwareStatusNotification from finally is safe under both profiles.
The field is reset in-place; no FirmwareStatusNotification is emitted
from finally, so the simulator does not race the caller driving the
supersession.
Stage transitions are placed only after successful advancement (after the
Installing / Installed notification awaits resolve), never on error/abort
branches, so the terminal-value selection in finally is unambiguous. The
finally guards mirror clearActiveFirmwareUpdate's requestId-supersession
pattern to avoid clobbering a superseder's live status, and preserve any
explicit terminal already emitted from inside try (DownloadFailed /
InvalidSignature / InstallationFailed).
Stage->terminal mapping uses `Record<Exclude<FirmwareStage, 'installed'>,
OCPP20FirmwareStatusEnumType>` with `as const satisfies` for
compile-time exhaustiveness over the failure-eligible stages. Naming
convention (SCREAMING_SNAKE_CASE) matches CoherentMeterValueBuilder's
PHASE_FAMILY/PHASE_RANK pattern for private module-scope Record lookup
tables.
Add OCPP20VariableManager.getInstance().resetRuntimeOverrides() to the
shared standardCleanup test helper so per-station variable overrides do
not leak between tests (all mock stations share a hardcoded hashId).
fix(atg): invalidate configurationValidationResult on ChargingStationEvents.updated (#1977)
Subscribe to ChargingStationEvents.updated in AutomaticTransactionGenerator's
constructor so in-flight configuration mutations that reach any charging
station mutation emit cannot leave a stale memoized validation decision
behind. stop() still clears the cache as a redundant safety net.
deleteInstance unsubscribes the handler before removing the entry so the
retired instance is not retained via the charging station's listener array
across a getInstance/deleteInstance churn cycle.
* docs: fill JSDoc gaps on public/protected abstract methods
Nine public/protected abstract methods across five base classes lacked
JSDoc, forcing new implementers to read existing subclasses to
reconstruct the contract. Add minimal-accurate JSDoc (one-line
description, @param, @returns) matching the style used by neighboring
already-documented methods in the same files.
Covered:
- Storage.close / open / storePerformanceStatistics
(contract for MongoDB / MariaDB / MySQL / SQLite / JSON storage
backends)
- OCPPIncomingRequestService.stop
- OCPPIncomingRequestService.isIncomingRequestCommandSupported
- OCPPRequestService.requestHandler
(contract for OCPP 1.6 / 2.0 request services)
- OCPPResponseService.isRequestCommandSupported
- AbstractUIServer.sendRequest / sendResponse
(contract for HTTP / WebSocket / stdio UI transports)
No behavior change. Gates pass (format / typecheck / lint).
* fix(physics): warn on voltageOut=400 V likely intended as line-to-line
voltageOut is line-to-neutral (V_LN) throughout the simulator. V_LL is
derived as sqrt(3) * V_LN in a balanced 3-phase Y system (see
OCPPServiceUtils.buildVoltageMeasurandValue and
CoherentMeterValueBuilder). The Voltage enum offers four closed values:
110, 230, 400, 800.
Users familiar with EU 3-phase infrastructure often configure
voltageOut=400 V thinking of the standard 400 V line-to-line nominal,
unaware that the simulator interprets it as line-to-neutral. Downstream
this yields an unrealistic simulated L-L ~= 693 V that only surfaces
when a meter value is inspected.
Emit a targeted logger.warn once per station init when
(currentOutType === AC && numberOfPhases === 3 && voltageOut === 400)
so the ambiguity surfaces at station start and the message tells the
user how to reach 400 V L-L (set voltageOut=230, the closest enum L-N
value).
No behavior change beyond the log line. Voltage.VOLTAGE_400 remains a
valid L-N configuration for the fraction of deployments that truly
want a 400 V L-N (rare but not unphysical), so the warning is not an
error. Gates pass (format / typecheck / lint).
* fix(atg): make in-flight transaction wait AbortSignal-aware
The AutomaticTransactionGenerator run loop at internalStartConnector
uses two long sleeps drawn from randomInt-clamped uniform distributions:
- Line 319: await sleep(wait), up to maxDelayBetweenTwoTransactions
- Line 345: await sleep(waitTrxEnd), up to maxDuration (potentially
hours for a realistic charging session)
Neither observed the connectorsStatus.get(connectorId)?.start flag while
sleeping. stopConnector() flipped start=false immediately, but the run
loop only saw it AFTER the current sleep expired — so a maxDuration=3600
config could keep the loop alive for an entire hour after a stop request.
Introduce a small module-scope interruptibleSleep(ms, signal) primitive
that races setTimeout against an AbortSignal, cleaning up whichever
listener does not win. Add a per-connector AbortController map on the
class, wire stopConnector() to abort the current controller, and thread
the signal through both long sleeps in internalStartConnector. The
controller is recreated per invocation of internalStartConnector so a
stale abort from a previous run cannot short-circuit a fresh loop.
The polling helpers (waitChargingStationAvailable /
waitConnectorAvailable / waitRunningTransactionStopped) keep their
DEFAULT_ATG_WAIT_TIME_MS sleeps — they iterate on much shorter
timescales and are not the audit target.
Gates pass (format / typecheck / lint).
* fix(atg): validate min/max delay and duration bounds before starting the run loop
The AutomaticTransactionGenerator run loop draws its between-transaction
wait and in-transaction duration from randomInt(min, max + 1) of
node:crypto. That primitive throws RangeError when min >= max, so a
mis-configured template with minDelay > maxDelay or minDuration >
maxDuration kills the async loop at the first iteration with an
unhandled rejection that surfaces only as a generic 'Error while
starting connector' log line — the actual mis-config value is lost.
Add a private validateConfiguration() that checks both min/max
invariants up front and, on violation, logs a targeted error naming
the offending pair and refuses to schedule internalStartConnector.
Called from startConnector() before the internalStartConnector spawn
so external callers of startConnector() also benefit; start() reaches
startConnectors() → startConnector() and inherits the guard.
Config absence is not a violation (returns true); the run loop already
defaults to 0 for missing min/max fields and no randomInt call fires.
No behavior change for a valid configuration. Gates pass
(format / typecheck / lint).
The audit's M4-M6 finding covered ~63 imperative/redundant/narrative
comments across the codebase, roughly a quarter of which sat in
ChargingStation.ts as one-line WHAT-restatements directly above a
self-documenting method call:
// Start heartbeat
this.startHeartbeat()
or above a specifically-named event handler:
// Handle WebSocket close
this.wsConnection.on('close', this.onClose.bind(this))
Delete such restatements when the next line already conveys the same
information via method / event / status name. Comments that carry a
non-obvious WHY, a spec reference, a race condition note, or a subtle
behavioural invariant are preserved unchanged (e.g. the response-
handling deferred-promise reject-is-no-op explanation at line 2472).
Continue the M4-M6 comment sweep in the OCPP request/incoming-request
dispatch layer:
OCPPRequestService.ts (8 deletions)
- // Send error message (above sendError → internalSendMessage)
- // Send response message (above sendResponse → internalSendMessage)
- // Build request (above JSON.stringify of OutgoingRequest)
- // Build response (above JSON.stringify of Response)
- // Send a message through wsConnection (above the WebSocket Promise)
- // Handle the request's response (above responseHandler call)
- // Remove request from the cache (above requests.delete)
- // Check if wsConnection opened (above isWebSocketConnectionOpened())
OCPPIncomingRequestService.ts (2 deletions)
- // Log (above logger.error)
- // Send the built response (above sendResponse call)
The '// Build Error Message per OCPP-J §4.2.3: [4, messageId, errorCode,
errorDescription, errorDetails]' comment carries a spec reference and
the tuple shape, and stays. The '// Emit command name event to allow
delayed handling only if there are listeners' comment carries the
listener-count WHY and stays.
No code change. Gates pass (format / typecheck / lint).
* refactor(auth): remove 14 redundant restatement comments in OCPPAuthServiceImpl
Continue the M4-M6 comment sweep in the auth service. Delete pure
WHAT-restatement comments that immediately precede a self-descriptive
line (method call, property assignment, or self-explaining condition):
- // Initialize metrics tracking / Initialize default configuration
(above obvious property assignments in the constructor)
- // Update request metrics / Update failure metrics
(above metrics counter increments)
- // Update metrics based on result
(above updateMetricsForResult call)
- // Create a minimal request to check applicability
(above testRequest object literal)
- // Check if adapter reports remote availability
(above adapter.isRemoteAvailable() call)
- // Merge new config with existing / Validate merged configuration
/ Apply validated configuration
(updateConfiguration WHAT-narration)
- // Check for specific error patterns that indicate critical issues
(above criticalPatterns array literal)
- // Track successful vs failed authentication / Track strategy usage
/ Track cache hits/misses based on method
(updateMetricsForResult WHAT-narration)
Preserved for their WHY / non-obvious context:
- // Note: Adapter and strategies will be initialized async via initialize()
(async lifecycle note)
- // Try each strategy in priority order (section marker in a 100+ line method)
- // Continue to next strategy unless it's a critical error (has WHY)
- // Get rate limiting stats from cache via remote strategy
(describes routing indirection)
- // Try local strategy first for quick cache/list lookup (fast-path rationale)
- // Create a type guard to check if strategy has configure method
/ Use type guard instead of any cast (non-obvious TS pattern)
No code change. Gates pass (format / typecheck / lint).
* refactor(charging-station): remove 6 residual restatement comments in ATG and HelpersChargingProfile
Final M4-M6 comment sweep pass on the two remaining files with clear
pure-restatement comments:
AutomaticTransactionGenerator.ts (2 deletions)
- // Start transaction (above await this.startTransaction(connectorId))
- // Wait until end of transaction
(above const waitTrxEnd = secondsToMilliseconds(randomInt(minDur, maxDur+1)) — the
waitTrxEnd variable name is the comment)
HelpersChargingProfile.ts (4 deletions)
- // Check if the charging profile is active
(above isWithinInterval(currentDate, {...}) condition)
- // Check if the first schedule period startPeriod property is equal to 0
(above the equality check on chargingSchedulePeriod[0].startPeriod)
- // Handle only one schedule period
(above chargingSchedule.chargingSchedulePeriod.length === 1 branch)
- // Handle the last schedule period within the charging profile duration
(above the composite last-period + duration-overflow condition)
Preserved WHY-comments in these two files: the § references, physics /
priority-order rationale, race-condition notes, and the ATG
interruptibleSleep + AbortController design comments introduced by fa2bea32 / 5a066a51.
No code change. Gates pass (format / typecheck / lint).
* docs(atg): document onAbort cleanup semantics in interruptibleSleep
The empty JSDoc bloc above the onAbort function inside interruptibleSleep
was an auto-generated placeholder from the jsdoc/require-jsdoc lint rule
firing on function declarations. It read like a TODO left in place.
Replace with a targeted comment covering the two non-obvious invariants
this cleanup function relies on:
- clearTimeout(timeout) prevents the setTimeout callback from firing
after the promise has already resolved on the abort path
- addEventListener({ once: true }) auto-removes the abort listener so
no explicit removeEventListener is needed on the abort path
No behavior change. Gates pass (format / typecheck / lint).
* fix(physics): broaden voltageOut warning to Voltage.VOLTAGE_800 and harmonize with CoherentSession
Round-1 review of this PR surfaced a harmonization violation: my M13
warning at ChargingStation.getStationInfoFromTemplate covered only
Voltage.VOLTAGE_400 and gated on numberOfPhases===3, while the
pre-existing per-session warning at CoherentSession.createCoherentSession
covered Voltage.VOLTAGE_400 or Voltage.VOLTAGE_800 in AC without a phase
gate.
Broaden the station-init warning:
- Add Voltage.VOLTAGE_800 (common L-L nominal in DC HPC / industrial
systems; users configuring it as L-N in AC produce sqrt(3) * 800
~= 1386 V simulated L-L).
- Drop the numberOfPhases === 3 gate so single-phase AC configurations
with 400 V / 800 V voltageOut also surface at station init (matches
CoherentSession coverage).
- Message shows the actual derived L-L (sqrt(3) * voltageOut) so users
see the physical implausibility of both enum values.
- Suggestion 'set voltageOut=230' is emitted only for the 400 case;
the 800 case has no clean L-N enum alternative (462 V is not in the
Voltage enum), so the message stops at the diagnostic.
The two warning sites remain complementary:
- ChargingStation station-init: fires once per station lifetime,
regardless of coherentMeterValues flag.
- CoherentSession per-session: fires per transaction, only when
coherentMeterValues=true.
Users with coherentMeterValues=false now get consistent enum coverage
without depending on the session-level flag.
Gates pass (format / typecheck / lint).
* refactor(atg): memoize validateConfiguration result to dedupe per-connector log noise
Round-1 review noted that validateConfiguration() fires N times for a
station with N connectors (once per startConnector call inside the
startConnectors loop), so an invalid ATG configuration produces N
identical error log lines instead of one.
Add a private configurationValidationResult field cached on first
invocation and reset in stop() so a station with N connectors emits the
diagnostic log line at most once per session, while still allowing a
subsequent start() with a mutated configuration to re-validate from
scratch.
Cache lifecycle:
- Initialized to undefined in the constructor.
- Set to true or false on the first validateConfiguration() call after
a start() (or after a fresh instance).
- Cleared back to undefined at the end of stop() so the next start()
observes any config changes and re-emits the log line if invalid.
No behavior change for the valid-configuration path. For the invalid
path, log-line count drops from N (per-connector) to 1 (per-session).
Gates pass (format / typecheck / lint).
* refactor(utils): extract isRandomIntBoundsValid predicate and apply in ATG
Round-1 review noted that the min > max guard in
AutomaticTransactionGenerator.validateConfiguration is a specific
instance of a general concern: any caller of randomInt(min, max + 1)
from node:crypto risks a RangeError when configuration-driven min/max
values are mis-ordered.
Extract a pure predicate isRandomIntBoundsValid(min, max) into
src/utils/Utils.ts (co-located with other random primitives:
secureRandom, getRandomFloat, getRandomFloatFluctuatedRounded,
getRandomFloatRounded). The predicate carries the +1 semantics contract
in its JSDoc so consumers cannot misuse it. Zero logger dependency,
zero side effects, testable in isolation.
Apply in ATG's validateConfiguration where the same check now goes
through the named predicate for readability and reuse. The two log
lines remain in ATG with their full field-name context (which the
predicate intentionally does not embed to stay pure).
Other randomInt(min, max + 1) sites in the codebase remain unchanged,
each documented as low-risk:
- OCPP16IncomingRequestService.updateFirmwareSimulation lines 1860,
1871, 1908, 1925, 1936, 1947: min/max are function-parameter
defaults (maxDelay=30, minDelay=15, hardcoded and safe by
construction); the internal callers respect the default ordering.
- OCPPServiceUtils.buildSocMeasurandValue line 270: socMaximumValue is
the Constants.SOC_MAXIMUM_PERCENT (100), socMinimumValue is the Zod-
schema-validated template field (bounded by the schema). The Zod
schema guarantees the invariant at load time.
These sites can adopt the predicate in a follow-up config-validation
sweep PR if the constraints ever loosen. No behavior change in this
commit. Gates pass (format / typecheck / lint).
* fix(atg): guarantee AbortController cleanup and prevent per-connector race
Round-3 review (subagents #1 design/impl + #2 algo) surfaced two
concurrency bugs in internalStartConnector's per-connector controller
lifecycle:
1. Leak on abort/exception path: the delete at line 412 was only reached
when the while-loop exited normally. Any await inside the loop that
rejected (waitChargingStationAvailable, startTransaction,
stopTransaction, etc.) propagated through the caller's .catch
handler in startConnector — bypassing the delete and leaving the
controller in connectorAbortControllers indefinitely.
2. Race in stopConnector→startConnector→cleanup ordering: if the old
loop was still draining (e.g. resolving an aborted
interruptibleSleep) when a new startConnector fired, the new run
set(controller_new) at line 349, and shortly after the old loop
reached the unconditional delete(connectorId), wiping the successor
controller. A subsequent stopConnector could then not abort the new
loop.
Fix both with a single try/finally + conditional delete pattern:
- try { while-loop } finally { conditional delete } guarantees
cleanup on every exit path (normal, break, throw) without leaking.
- Delete only when this.connectorAbortControllers.get(connectorId)
=== abortController (identity check on the run-local reference)
so a successor controller cannot be wiped by a predecessor's finally.
No behavior change on the happy path. Gates pass
(format / typecheck / lint).
* fix(atg): unconditionally reset validation cache in stop() to enable recovery
Round-3 review (subagents #1 + #2) found a stuck-state bug:
The cache reset was at the END of stop(), inside the branch guarded by
'if (!this.started) { return }'. So if a caller invoked
startConnector() directly (without going through start()), this.started
never became true, and any subsequent stop() early-returned without
resetting the cache — the user was stuck with a cached-false
configurationValidationResult and no path to recover.
Move the reset to the TOP of stop(), before the started/stopping
guards. The reset is idempotent (undefined -> undefined is a no-op),
so unconditional execution has no side effect on the already-stopped
path. This lets external startConnector() callers recover by calling
stop() to clear the cache.
Gates pass (format / typecheck / lint).
* refactor(utils): rename to isValidRandomIntBounds and strengthen input validation
Round-3 review (subagents #1 + #2 + harmonization #6) found three
issues with the extracted predicate:
1. Naming inversion — neighbors follow subject-first (isValidDate,
isNotEmptyString, isNotEmptyArray) but isRandomIntBoundsValid put
the operation before the subject. Rename to isValidRandomIntBounds.
2. Under-validation — the predicate only checked min <= max, so it
returned true for NaN/Infinity/non-integers/negatives even though
node:crypto.randomInt throws RangeError on all of them (safe integer
0 <= min < 2^48 <= max required). NaN was accidentally rejected
because 'NaN <= NaN' is false. Strengthen to reject explicitly:
Number.isSafeInteger(minValue) && Number.isSafeInteger(maxValue) &&
minValue >= 0 && minValue <= maxValue.
3. JSDoc leaked ATG-specific '+1' convention into a general utility
contract. Trim @param maxValue to just 'Upper bound (inclusive).'
The +1 semantics belong in the description (kept there) and at the
call site.
Log messages in ATG updated to match:
- randomInt(min, max) -> randomInt(min, max + 1) to reflect the actual
call
- Semicolon comma joining consequence clauses replaced with em-dash
for readability
Gates pass (format / typecheck / lint).
* refactor(utils): export interruptibleSleep and apply to ATG polling helpers
Round-3 review (subagent #4 harmonization) found interruptibleSleep
was private to ATG and could not be reused, plus the three ATG
polling helpers (waitChargingStationAvailable / waitConnectorAvailable
/ waitRunningTransactionStopped) called plain sleep() and were blind
to the abort signal.
Changes:
1. Move interruptibleSleep from AutomaticTransactionGenerator.ts to
src/utils/Utils.ts next to sleep(). Export from utils/index.ts.
2. Reorder to declare 'timeout' before the 'onAbort' arrow function so
there is no forward reference (subagent #1 finding).
3. Expand JSDoc: describe both cleanup paths symmetrically (timer-path
explicit removeEventListener, abort-path clearTimeout + { once: true }
auto-removal). Also document that the promise resolves (does not
reject) on abort so callers can drop try/catch noise.
4. Add signal: AbortSignal parameter to the three ATG wait helpers.
5. Replace sleep(DEFAULT_ATG_WAIT_TIME_MS) with
interruptibleSleep(DEFAULT_ATG_WAIT_TIME_MS, signal) in each helper.
6. Include !signal.aborted in each while-loop condition so the poll
exits immediately when the connector is stopped mid-wait.
7. Thread abortController.signal from internalStartConnector into all
three helper calls.
8. Add 'interruptible' to cspell.config.yaml.
Impact: a stopConnector() call during an availability-wait now unblocks
the poll immediately rather than after DEFAULT_ATG_WAIT_TIME_MS. Gates
pass (format / typecheck / lint).
* fix(physics): scope L-L derivation to 3-phase and harmonize voltageOut warning
Round-3 review (subagents #1 impl elegance + #2 physics + #4
harmonization + #5 content) found the M13 warning had three related
issues:
1. Message inaccuracy for single-phase AC: the phrase '3-phase Y-derived
line-to-line = X V' fired for single-phase AC stations too (F1 had
intentionally dropped the numberOfPhases===3 gate for enum coverage
symmetry with CoherentSession). But sqrt(3) only applies to balanced
3-phase Y — for single-phase there is no L-L, so the number cited
was physically meaningless.
2. Missing VOLTAGE_800 suggestion: only the VOLTAGE_400 case emitted a
remediation hint. VOLTAGE_800 warned but did not tell the operator
what to do next; 800 V L-L has no clean L-N enum equivalent, so the
suggestion should say so.
3. Terminology drift vs CoherentSession: this site said
'line-to-neutral' while CoherentSession said 'line-to-neutral (phase
voltage)'; missing space before V in CoherentSession's log message.
Fix:
- Gate the 3-phase-Y clause behind getNumberOfPhases===3 (conditional
string, still fires for all AC phase counts on enum match). For
1-phase and DC, the derived-L-L clause is elided.
- Emit a VOLTAGE_800 remediation: 'no standard L-N enum value exists;
verify template voltageOut intent (~462 V L-N)'.
- Adopt 'line-to-neutral (phase voltage)' terminology matching
CoherentSession. Add space before V in CoherentSession log message.
- Trim the 10-line explanatory comment block to 6 lines: keep the
invariant (voltageOut is L-N; VOLTAGE_400/800 commonly mis-configured);
drop the cross-site harmonization prose (belongs in commit history).
Round-3 review (subagent #1 design/impl) found the M4-M6 comment sweep
had over-deleted one non-restatement comment at HelpersChargingProfile
line ~404: '// Handle the last schedule period within the charging
profile duration'. The composite condition below (last-in-array OR
next-period-would-exceed-duration) is not self-evident from the
boolean expression alone; the comment aids navigation through this
OCPP-specific boundary case.
Restore with a tighter version: 'Last period: last in array OR next
period would exceed the charging profile duration.'
No code change. Gates pass (format / typecheck / lint).
* docs(ocpp): use backtick-quoted `true` in @returns for consistency
Round-3 review (subagent #5 content finding 3) found @returns lines
in the M11 JSDocs used capitalized 'True when' while neighboring
JSDocs used backtick-quoted `true`. Normalize both:
No behavior change. Gates pass (format / typecheck / lint).
* docs: remove/trim redundant comments per DRY (AGENTS.md no-duplication rule)
User feedback: comments that repeat info already conveyed by log
messages, method names, or field names violate concision + AGENTS.md
'No duplication: maintain single authoritative documentation source;
reference other sources rather than copying'.
Changes:
1. ChargingStation.ts:1676-1681 - DELETE the 6-line voltage warning
block comment. Every fact it stated (voltageOut is L-N, 400/800 are
L-L nominals) is already in the log message immediately below
('matches a line-to-line nominal value', 'treats voltageOut as
line-to-neutral (phase voltage)'). The 'harmonized with
CoherentSession' cross-reference is trivially discoverable via
'rg Voltage.VOLTAGE_400'.
2. AutomaticTransactionGenerator.ts stop() - trim 4-line 'Reset
unconditionally' comment to 3 lines. Drop the redundant
'configurationValidationResult' field name (visible on next line)
and 'this.started never transitioned to true' (obvious from
the guard structure).
3. AutomaticTransactionGenerator.ts stopConnector() - trim 5-line
'Wake' comment to 3 lines. Drop the redundant second sentence about
controller lifecycle (visible from the field name
'connectorAbortControllers' and its usage in internalStartConnector).
4. AutomaticTransactionGenerator.ts internalStartConnector() - trim
4-line 'Fresh AbortController' comment to 3 lines. The 'new
AbortController()' on the next line already conveys freshness; only
the abort-before-replace ordering (unblock lingering sleep) needs
documenting.
5. AutomaticTransactionGenerator.ts try/finally - trim 5-line 'Only
clear' comment to 2 lines. The try/finally structure self-documents
the 'guarantees cleanup on all paths' claim; only the identity
check for concurrent stopConnector→startConnector needs explanation.
No behavior change. Gates pass (format / typecheck / lint).
* fix(atg): close two ATG concurrency correctness gaps
1. **Concurrent-loop race on back-to-back stopConnector→startConnector**
(subagent #2 finding 3): after stopConnector aborts the controller,
the old internalStartConnector loop resumes from its interruptible
sleep. When startConnector fires immediately after and resets
connectorStatus.start=true, the old loop's while condition (which
only reads the shared 'start' flag) sees true again and continues
running with an already-aborted signal — subsequent
interruptibleSleep calls resolve synchronously → tight-loop, or
worse, the old loop calls stopConnector again and aborts the NEW
controller.
Fix: add !abortController.signal.aborted to the while condition. The
old loop now exits immediately after abort regardless of the shared
'start' flag.
2. **waitRunningTransactionStopped stale connectorStatus snapshot**
(subagent #1 finding 7): the helper fetched connectorStatus once
before the loop and re-read the same object on each iteration. This
works today because getConnectorStatus returns a Map reference that
mutates in place, but the pattern was inconsistent with
waitChargingStationAvailable / waitConnectorAvailable which call
their predicates inline, and would silently break if
getConnectorStatus ever returned a new object per call.
Fix: inline the getConnectorStatus call in the while condition and
inline it again for the one-time log message. Matches the pattern of
the two sibling wait helpers.
Gates pass (format / typecheck / lint).
* fix(atg): move post-loop bookkeeping inside finally for exception paths
Round-4 review (subagent #1 design/impl MEDIUM finding 3): the
connectorStatus.stoppedDate assignment and the two 'Stopped on
connector' logger calls at the end of internalStartConnector sat
OUTSIDE the try/finally block. If any await inside the while loop
rejected (waitChargingStationAvailable, startTransaction, stopTransaction,
etc.), the finally block ran the controller cleanup — but the throw
propagated past the post-loop block, leaving connectorStatus.stoppedDate
undefined and skipping the 'Stopped on connector' + 'Stopped with
connector status' log lines. The connector observably appeared to be
still running for a stopped station.
Move the four post-loop statements inside the finally block so the
connector's stoppedDate is always set and the operator always sees the
'Stopped' log lines, regardless of exit path (normal exit, break,
throw). The ChargingStationEvents.updated emit also relocates so the
UI reflects the stopped state on every path.
Gates pass (format / typecheck / lint).
* docs: harmonize voltage warnings and @returns JSDoc across OCPP layer
Round-4 review (subagents #3 OCPP+TS, #4 harmonization, #5 content)
surfaced three harmonization drifts in JSDoc/log content:
1. ChargingStation.ts:1697 voltage warning said 'voltageOut=X V with
AC output matches…' — 'with AC output' is redundant (the warning
already gates on currentOutType === AC) and not spec terminology.
Drop the qualifier.
2. CoherentSession.ts:103 voltage warning framed the diagnostic
differently than ChargingStation.ts ("is treated as line-to-neutral
by ACElectricUtils" vs "matches a line-to-line nominal value").
Also attributed the interpretation to ACElectricUtils, an
implementation detail that leaked into a user-facing log. Align
framing to match ChargingStation.ts and drop the ACElectricUtils
attribution.
3. AutomaticTransactionGenerator.ts:302-304 abort-before-replace
comment ended with 'before installing the new controller' — the
'new AbortController()' + 'connectorAbortControllers.set()' calls
on the immediately following two lines restate that fact. Trim.
4. Four OCPP validate*Payload JSDocs used "@returns True if payload
validation succeeds, false otherwise" — capitalized bare 'True'/'false'
without backticks and no trailing period. Established pattern
elsewhere in the same files uses "\ when … \ otherwise."
Normalize in OCPPIncomingRequestService, OCPPRequestService,
OCPPResponseService, and OCPPServiceUtils.
Gates pass (format / typecheck / lint).
* docs: polish batch from round-4 review
Round-4 subagents flagged five small content-quality items, none
blocking:
1. Utils.ts interruptibleSleep: delete two inline comment blocks that
duplicate the JSDoc (timer-path cleanup and abort-path cleanup are
already documented symmetrically in the JSDoc body).
2. Utils.ts isValidRandomIntBounds JSDoc: drop the last sentence
that leaked call-site usage guidance into a general-utility
predicate contract. Document the >= 0 constraint in @param.
3. AutomaticTransactionGenerator.ts validateConfiguration JSDoc: trim
the memoization-mechanism sentence to a single clause. The full
rationale already lives in the stop() reset comment.
4. AutomaticTransactionGenerator.ts validateConfiguration log messages:
the > description understated the predicate rejection surface (it
also rejects NaN/Infinity/non-integer/negative). Rewrite as invalid
bounds matching the predicate contract.
5. Storage.ts three JSDoc @returns: backtick-quote void to match the
codebase convention of quoting type names.
No behavior change. Gates pass (format / typecheck / lint).
* fix(ocpp/1.6): guard updateFirmwareSimulation against invalid delay bounds
Round-4 review (subagent #4 harmonization finding 3): six sites in
updateFirmwareSimulation call randomInt(minDelay, maxDelay + 1) using
function-parameter bounds. Current callers (constructor lines 586 and
596) pass no arguments so the safe defaults (maxDelay=30, minDelay=15)
apply. But the parameters are exposed, and any future caller passing
config-driven values could hit a randomInt RangeError inside the async
firmware simulation loop that surfaces only as a generic caught error.
Add an early isValidRandomIntBounds guard immediately after the
existing checkChargingStationState guard. On invalid bounds, log an
error naming the offending pair and return without scheduling any
simulation. Defensive-only — no behavior change for the current
default-only callers.
Gates pass (format / typecheck / lint).
* fix(ocpp/2.0): thread abort signal through simulateFirmwareUpdateLifecycle sleeps
Round-4 review (subagent #4 harmonization finding 1): the OCPP 2.0
firmware simulation lifecycle owns an AbortController
(activeFirmwareUpdateAbortController set at line 3703) so a concurrent
UpdateFirmware request can cancel an in-progress simulation (line
3091 aborts the previous controller). However, the eight
await sleep(...) calls inside simulateFirmwareUpdateLifecycle did not
observe that signal. The pattern was:
await sleep(delayMs)
if (checkAborted()) return
which correctly exits after abort but only ONCE the sleep expires. A
long sleep (retrieveDateTime scheduling, InstallDateTime scheduling)
could keep the simulation running for hours after the abort.
Replace each of the eight sleeps inside simulateFirmwareUpdateLifecycle
with interruptibleSleep(delay, abortController.signal). The abort now
wakes the sleep immediately; the subsequent checkAborted() guard
observes the aborted state and returns. Callback semantics identical
on the non-abort path.
The ninth sleep at line 3902 (simulateLogUploadLifecycle) is left
unchanged - that function does not own an AbortController and log
uploads are not user-cancellable in this simulator.
Gates pass (format / typecheck / lint).
* fix(utils): enforce node:crypto randomInt 2^48 range limit in predicate
Round-5 review (subagent OCPP+TS finding 8, HIGH): Number.isSafeInteger
allows values up to 2^53 - 1, but node:crypto.randomInt rejects any
call where max - min > 2^48 - 1 with a RangeError. The predicate's
documented contract ('safe for randomInt') was therefore incorrect
for wide ranges — the false-positive latent because current callers
use small integer delays (15-30s) and SoC bounds (0-100).
Add the 2^48 range check to the predicate and document it in the
JSDoc. Empirically verified against Node 22:
randomInt(0, 2**48) throws RangeError
Number.isSafeInteger(2**48) === true
Gates pass (format / typecheck / lint).
* fix(ocpp): guard SOC randomInt against invalid template bounds
The SoC MeterValues fallback path in buildSocMeasurandValue drew a
random integer via randomInt(socMinimumValue, socMaximumValue + 1)
where socMinimumValue comes from the (user-supplied) template
minimumValue field and socMaximumValue is Constants.SOC_MAXIMUM_PERCENT
(=100). If a template set minimumValue >= 101, randomInt would throw
RangeError inside a MeterValues emission — surfacing only as a caught
error in the OCPP path.
Guard with isValidRandomIntBounds before the randomInt call. On
invalid bounds, log a warn line naming the offending pair and fall
back to socMaximumValue (100) so the measurand still produces a
value.
Gates pass (format / typecheck / lint).
* fix(atg): gate all post-loop bookkeeping on controller ownership
Round-5 review (design/impl finding 3, MEDIUM): the round-4 finally
block gated only the connectorAbortControllers.delete() on the
identity check (map.get === abortController). It then wrote
connectorStatus.stoppedDate unconditionally, emitted the Stopped log
lines, and fired the updated event.
On a back-to-back stopConnector -> startConnector, the sequence is:
t0: old loop finally fires
t1: identity check: map.get === NEW controller (the successor
already installed by startConnector); check fails, delete
correctly skipped
t2: connectorStatus.stoppedDate = new Date() <-- corrupts the
new run's status object (shared reference in connectorsStatus)
t3: 'Stopped on connector' log fires for a connector the operator
just re-started
Gate all four post-loop operations (delete, stoppedDate, info log,
debug log, updated event) on a single isOwner boolean. Non-owner
paths silently exit — the successor run owns the status object and
will emit its own Stopped log when it eventually completes.
Gates pass (format / typecheck / lint).
* fix(ocpp/2.0): guarantee clearActiveFirmwareUpdate on every exit path
Round-5 review (harmonization finding C-2, MEDIUM):
simulateFirmwareUpdateLifecycle called clearActiveFirmwareUpdate only
on three specific paths (empty location, signature failure, happy
completion). Any other early-return path (7 abort returns) or a
thrown await left activeFirmwareUpdateAbortController set. A
subsequent UpdateFirmware.req would then abort() a non-existent
in-progress update, and stationState.activeFirmwareUpdateRequestId
would never re-populate — the station appeared stuck with a phantom
in-progress firmware update.
Wrap the entire body in try/finally with a single
clearActiveFirmwareUpdate call in finally. Delete the three now-redundant
site-local calls. Also correct the abort-controller store comment
from 'cancel' to 'abort' (codebase terminology finding B1/D4 from the
round-5 content review).
Gates pass (format / typecheck / lint).
* docs: polish batch from round-5 review
Four small content-quality items surfaced by round-5 review:
1. AutomaticTransactionGenerator.waitRunningTransactionStopped: the
first-iteration log line called getConnectorStatus a second time
(separate from the loop condition) to read transactionId. Between
the two reads the transaction could theoretically end and a new
one start, logging a different id than the one that triggered the
wait. Snapshot transactionId into a local const before the log.
2. AutomaticTransactionGenerator.validateConfiguration error messages
and OCPP16IncomingRequestService.updateFirmwareSimulation abort
message both suffixed 'for randomInt(min, max + 1)' — misleading
because isValidRandomIntBounds validates raw bounds, not the +1
form. Drop the suffix; the 'invalid bounds' clause is sufficient.
3. CoherentSession voltage warning lacked the actionable remediation
hint present in the ChargingStation counterpart. Append 'Set
voltageOut to the L-N equivalent in the station template to
resolve.'
No behavior change. Gates pass (format / typecheck / lint).
* fix(utils): tighten isValidRandomIntBounds boundary by one
The predicate validated maxValue - minValue < 2^48, but every call site
passes randomInt(min, maxValue + 1). node:crypto's randomInt requires
max_exclusive - min <= 2^48 - 1, so the effective constraint on the
inclusive bounds is maxValue - minValue <= 2^48 - 2. The old boundary
permitted maxValue - minValue = 2^48 - 1, which after the +1 offset
would throw RangeError at the randomInt call.
Practical impact is nil (all real config paths are far below the limit)
but the predicate is now nominally correct. Also trim JSDoc
overexplanation of caller convention.
* fix(ocpp): skip SoC measurand emission on invalid bounds
buildSocMeasurandValue previously fabricated a synthetic 100 % SoC
reading when the template's minimumValue was misconfigured (e.g. a
negative float that fails isValidRandomIntBounds). Emitting 100 %
looks like a valid meter reading to the CSMS and could distort
billing, session-end triggers, and analytics on the operator side.
Skip the measurand entirely instead — consistent with the existing
null-return path when the SoC template itself is absent (line 261).
The warn log still surfaces the misconfiguration.
handleRequestUpdateFirmware's abort-and-replace path directly wrote
undefined to the two activeFirmwareUpdate* fields, while the lifecycle
finally routed through clearActiveFirmwareUpdate. Any future logic
added to the helper (logging, metrics, invariants) would silently
diverge for the abort-and-replace path.
Extract resetActiveFirmwareUpdateState as the single source of truth
for the field reset. clearActiveFirmwareUpdate still guards on
requestId identity (protecting a successor's state from being cleared
by a stale finally); handleRequestUpdateFirmware calls the reset
unconditionally because the abort-and-replace semantics require it.
* feat(atg): log displaced-loop exit at debug level
The isOwner gate silently returned from the finally block when a
concurrent stopConnector→startConnector had already installed a
successor controller. Diagnosing 'why did the old loop stop?' under
concurrency required knowing the silent path existed.
Emit a debug line on the non-owner path so displaced-loop exits are
observable without adding noise at higher log levels. Also trim the
redundant second half of the ownership comment (the consequence is
already implied by the WHY sentence above).
* docs(coherent): align voltage warning voice with ChargingStation
CoherentSession's L-L/L-N mismatch warning used passive voice
('If this value is meant as line-to-line') while the parallel warning
in ChargingStation.ts uses direct second-person ('If you intended').
Align to the second-person form for cross-file consistency.
The description body stated 'maxValue - minValue <= 2^48 - 2' while
@param stated 'maxValue - minValue < 2^48 - 1'. Both are equivalent
for integers but forced a reader to verify equivalence. Also drop the
derivation showing how the node:crypto max_exclusive - min <= 2^48 - 1
constraint reduces to maxValue - minValue <= 2^48 - 2 — the derivation
belongs in commit history, not in the live docblock.
@param and description now use the same notation (<= 2^48 - 2).
* docs(logs): align tense and specificity in round-6 messages
Two log-message polish items surfaced by round-7:
ATG.internalStartConnector displaced-loop debug log used present
participle 'Exiting' while the sibling info log at line 391 uses past
tense 'Stopped'. Both fire from the finally block reporting a
completed event. Align to 'Exited'.
OCPPServiceUtils.buildSocMeasurandValue skip-log action clause read
'skipping measurand'. Sibling messages in ATG and OCPP16 use fully
qualified action noun phrases ('aborting connector startup',
'aborting firmware update simulation'). Add the 'SoC' qualifier for
parallelism: 'skipping SoC measurand'.
Three coherence gaps identified by round-7 harmonization review:
stop() aborted the active firmware update but wrote the delete without
routing through resetActiveFirmwareUpdateState. Latent-only because
the stationsState.delete makes the fields unreachable, but the pattern
diverged from every other cleanup site. Route through the helper.
clearActiveFirmwareUpdate silently returned when the requestId did not
match the active one — the exact 'displaced' pattern the ATG round-6
fix addressed. A stale finally firing after a successor took over is
diagnostically important. Emit a debug line for the superseded path.
Naming distinction (reset = unconditional, clear = identity-guarded)
is now self-documenting via the guard in clearActiveFirmwareUpdate.
* fix(ocpp20): prevent state resurrection in clearActiveFirmwareUpdate
getStationState is a lazy-init getter: on cache miss it creates an
empty entry and adds it to stationsState. clearActiveFirmwareUpdate
runs in the finally of simulateFirmwareUpdateLifecycle. If stop() has
already deleted the station entry before the aborted lifecycle's
finally fires, the lazy-init would resurrect a fresh empty entry —
leaking state past shutdown.
Use stationsState.get() directly and no-op silently when the entry is
absent. The non-owner debug log now only fires for genuine
supersession (state present, requestId mismatched).
* docs: revert isValidRandomIntBounds JSDoc to match code + fix typo
Round-7 aligned the JSDoc @param to '<= 2^48 - 2' while the code kept
'< 2 ** 48 - 1'. Both forms are equivalent for integers but the JSDoc
notation deviated from the code's literal. Revert JSDoc to '< 2^48 - 1'
which mirrors the code and stays close to node:crypto's own docs
phrasing ('range must be less than 2^48').
Also fix a pre-existing typo in the Constants.MAX_RANDOM_INTEGER
comment: 'randomInit()' -> 'randomInt()'.
* fix(ocpp20): use raw stationsState.get() in FirmwareStatusNotification trigger
The TriggerMessage/FirmwareStatusNotification case at line 564 is a
reporting path — it reads activeFirmwareUpdateRequestId to build the
notification payload but does not otherwise mutate station state.
Calling getStationState (which lazy-inits on cache miss) here means
a TriggerMessage arriving for a station that has never had a firmware
update creates a spurious empty state entry, and a TriggerMessage
arriving between stop() and WS close would resurrect state after
shutdown — same pattern as the round-8 clearActiveFirmwareUpdate fix.
Use stationsState.get() directly and optional-chain the requestId
read. When no state exists the notification carries requestId
undefined, which is spec-compliant for status = Idle (OCPP 2.0.1
L01.FR.20: requestId is mandatory only for non-Idle statuses).
* docs(utils): drop @param maxValue range-width restatement
The @param maxValue clause restated 'with maxValue - minValue <
2^48 - 1', which is already stated in the description body. Trim to
just the intrinsic bound (inclusive safe integer >= minValue) and
let the range-width constraint live where it belongs (the body).
* refactor(ocpp20): rename getStationState to getOrCreateStationState
The private getter lazily creates a station state entry on cache miss.
Callers seeing 'getStationState(cs)' vs 'stationsState.get(cs)' side
by side could not tell that the former creates state while the latter
does not — the very split rounds 8 and 9 introduced to fix
state-resurrection bugs (clearActiveFirmwareUpdate, TriggerMessage/
FirmwareStatusNotification).
Rename the lazy-init getter so its name carries the semantics:
this.getOrCreateStationState(cs) // creates on miss - handler paths
this.stationsState.get(cs) // undefined on miss - cleanup/reporting
Nine call sites + one definition. Zero behavior change.
* docs(utils): make @param minValue structurally symmetric with @param maxValue
Round-9 trimmed @param maxValue to 'safe integer >= minValue' but left
@param minValue as 'non-negative safe integer'. Both bounds now state
'safe integer >= X' — @param minValue relative to 0, @param maxValue
relative to minValue. The description body still carries the rejection
list (NaN, Infinity, floats, negatives, range-width).
* fix(ocpp20): cross-check firmware status against active requestId in trigger
FirmwareStatusNotification TriggerMessage read from two independent
sources: hasFirmwareUpdateInProgress reads chargingStation.stationInfo
.firmwareStatus, activeFirmwareUpdateRequestId lives on stationsState.
They can diverge in real windows:
1. simulateFirmwareUpdateLifecycle sets activeFirmwareUpdateRequestId
at line 3716 before its first sendFirmwareStatusNotification (line
3467). During any await between the two — notably the retrieveTime
sleep at 3730 — a trigger would emit
{ requestId: X, status: Idle/undefined }.
2. On exception paths the finally resets activeFirmwareUpdateRequestId
but leaves stationInfo.firmwareStatus at its last-set value. A
trigger fired between the exception and the next UpdateFirmware.req
would emit { requestId: undefined, status: <non-Idle> } — an OCPP
2.0.1 L01.FR.20 violation (requestId mandatory for non-Idle).
Gate the non-Idle firmwareStatus branch on requestId being defined —
activeFirmwareUpdateRequestId is the authoritative signal for 'an
update is in progress'. When it is undefined we always emit Idle,
which is spec-compliant regardless of the stationInfo staleness.
The resolveEnabledMeasurands warn-once path had a 4-line inline
lazy-init block on warnedInvalidMeasurands (WeakMap<ChargingStation,
Set<string>>). Extract it as a module-level helper mirroring the
round-10 getOrCreateStationState naming.
Same semantic distinction as OCPP20IncomingRequestService:
- getOrCreate* — lazy-init, always returns a live reference
- .get() — raw WeakMap read, may return undefined
* fix(ocpp20): apply dual-gate to LogStatusNotification trigger
The LogStatusNotification TriggerMessage case sent
{ status: Idle } unconditionally, with no requestId — losing the
signal when a log upload is genuinely in progress and mirroring the
same pre-fix gap round-11 closed for FirmwareStatusNotification.
Round-11 established the dual-gate pattern: an active requestId in
stationsState is the authoritative 'operation in progress' signal;
the status is emitted from the same source of truth to guarantee
consistency between requestId and status.
Log upload has no stationInfo.logStatus equivalent (unlike firmware's
stationInfo.firmwareStatus), so store the status alongside the
requestId in the stationState. Wrap simulateLogUploadLifecycle in
try/finally to guarantee cleanup on exception. The trigger now
reports Uploading/Uploaded with the active requestId when in-flight,
Idle otherwise — matching the firmware pattern structurally.
* docs(ocpp20): enumerate all 8 in-progress statuses in hasFirmwareUpdateInProgress @returns
The JSDoc @returns listed 3 statuses (Downloading, Downloaded,
Installing) but the body checks 8 (also DownloadScheduled,
DownloadPaused, InstallScheduled, InstallRebooting, SignatureVerified).
Enumerate all 8 to match the implementation.
1. simulateLogUploadLifecycle mutated activeLogUploadStatus =
Uploaded without an identity guard. If a concurrent GetLog
request installed a new lifecycle during the sleep, the old
lifecycle's post-sleep resume would overwrite the new lifecycle's
status with a stale Uploaded, causing the TriggerMessage handler
to report the wrong signal for the in-flight upload.
Guard the mid-lifecycle status mutation with the same identity
check used in the finally. Extract clearActiveLogUpload as a
helper mirroring clearActiveFirmwareUpdate — same raw-.get() +
null-guard + identity-check + debug-log-on-superseded pattern
for observability parity with the firmware peer.
2. hasFirmwareUpdateInProgress @returns clause was 183 chars on one
line and used unbacktick'd 'true'. Wrap the enumeration across
three lines and restore the `true` backtick style established
by 92a8dac1.
* fix(ocpp20): implement log-upload supersession per OCPP 2.0.1 N01.FR.12
Round-14 subagent C surfaced a HIGH spec-conformance gap: when a new
GetLog request arrives with a prior upload in progress, the CS was
returning Accepted and letting the old lifecycle run to completion
(sending a spurious Uploaded for the superseded requestId). This
violates N01.FR.12 + FR.20 and fails TC_N_36_CSMS step 7 which
expects LogStatusNotification(AcceptedCanceled) for the old id.
handleRequestGetLog now mirrors handleRequestUpdateFirmware:
- detect active upload, capture the previous requestId
- reset log state so the old lifecycle exits gracefully
- send LogStatusNotification(AcceptedCanceled) for the old id
- return GetLogResponse with status AcceptedCanceled
The GET_LOG on-hook now accepts both Accepted and AcceptedCanceled
to start the new lifecycle. The mid-lifecycle Uploaded emission is
now co-located inside the identity guard: a superseded lifecycle
skips both the state mutation AND the wire notification, preventing
the spurious Uploaded that would follow an already-emitted
AcceptedCanceled.
Extracted resetActiveLogUploadState mirroring
resetActiveFirmwareUpdateState (round-14 subagent A finding 2) so
clearActiveLogUpload delegates to the two-field wipe rather than
inlining it — full structural parity with the firmware peer.
Three findings from the same review round, one file:
1. clearActiveFirmwareUpdate and clearActiveLogUpload logged the debug
'superseded requestId X (active: Y)' message even when the active
field was undefined (the state was reset, not superseded). The
post-supersession trace fires clearActive* with the old requestId
after resetActive*State cleared the active field — the log said
'active: undefined' which reads as a bug. Tighten the else branch
to fire only when a DIFFERENT non-null requestId is active; drop
the String() wrapper since only the number path can reach it.
2. stop() aborted the firmware controller and reset firmware state
before deleting the stationsState entry, but skipped the log upload
state. The delete makes the fields unreachable so it is not a
memory leak, but the asymmetry breaks the structural mirror with
firmware and the aborted-but-still-running log lifecycle would see
inconsistent state post-stop. Add resetActiveLogUploadState for
parity.
3. handleRequestGetLog @returns still said 'Accepted status' only —
stale since the round-14 supersession fix. Extend to mention both
Accepted (no prior upload) and AcceptedCanceled (superseded prior
upload) with a description sentence on the supersession behavior.
Three findings from round-16 review of the round-15 changes:
1. MEDIUM: OCPP20StationState.certSigningRetryManager holds a live
setTimeout whose callback closes over chargingStation. The
stationsState.delete() in stop() drops the WeakMap entry but the
timer keeps chargingStation alive until it fires. Add
certSigningRetryManager?.cancelRetryTimer() alongside the firmware
abort() call, before the resets and the delete — closes the leak
and matches the abort-before-reset pattern established for
firmware.
2. LOW: 'simulates a Uploading' — wrong article. Fix to 'simulates
an Uploading' (starts with vowel sound).
3. LOW: The @returns clause exceeded 140 chars on a single line.
Wrap onto two lines to match the ~100-char convention.
* fix(ocpp20): add AbortController to log-upload lifecycle + canceled spelling
Round-17 findings from rounds 13-17 flagged the log-upload/firmware
structural asymmetry repeatedly: firmware had AbortController + reset,
cert-signing had cancelRetryTimer + reset, log had only identity-guard
+ state reset. A superseded lifecycle woke from sleep on its own
schedule, wasted the delay, and the aborted-but-still-running
coroutine emitted spurious wire notifications until the identity
guard caught it. Round-17's harmonization subagent explicitly said
'worth fixing in this PR while the pattern is fresh'.
Add activeLogUploadAbortController to OCPP20StationState, create it
in simulateLogUploadLifecycle, replace sleep() with interruptibleSleep
threading the signal, and gate the mid-lifecycle status write and
send on both signal.aborted and the identity check. abort() the
controller in handleRequestGetLog supersession, in stop(), and reset
it via resetActiveLogUploadState. sleep import is now unused —
removed.
Also fix pre-existing British/American spelling drift surfaced by
round-17 content review: 'Retry timer cancelled' -> 'canceled', for
consistency with the cancelRetryTimer method name and OCPP enum
values (AcceptedCanceled).
fix(ocpp): from-zero audit — Critical + High spec conformance and safety fixes (#1961)
Eleven commits from the from-zero audit (three review rounds).
Fixes:
- C2 (§4.2) forbid non-triggered outbound messages in Pending state under non-strict compliance
- H2 emit StatusNotification(Finishing) before StopTransaction.req in the non-remote stop path
- H6 (§5.11) RemoteStart auto-select skips connectors reserved for a different idTag
- H9 partial: .unref() on 5 lifecycle timer sites (3 setTimeout + 2 setInterval) so they no longer block node.js shutdown; per-site safety-invariant comments
- H10 partial: initialize 5 string ChargingStation fields in the constructor; drop their definite-assignment markers
- H11 log the errors previously swallowed by .catch(() => undefined) in AbstractUIServer
Review follow-ups:
- Correct method name in stop() metrics-cleanup log prefix
- Clarify runMetricsScrape catch-log wording
- Correct spec-citation in stopTransactionOnConnector test title
Gates: format / typecheck / lint / build / test (2951 pass / 0 fail / 6 skipped) all pass at tip ca6b0182. Skott circular-dependency count preserved at 61.
The extra parentheses wrap a comma-operator expression that evaluates to
`dirname(logFile)` only, so `readdirSync` was invoked with a
directory resolved from CWD instead of relative to the module URL. When
the process CWD is not the project root (e.g. a systemd service, a docker
container that mounts logs elsewhere, a background worker with a chdir'd
parent), the log-file discovery scanned the wrong directory and produced
an incomplete diagnostics archive.
Align with the sibling call at line 1183 which already uses the intended
pattern:
The ratio V_LL / V_LN in a balanced 3-phase Y system is a fixed sqrt(3),
which comes from the 30-degree phase separation between line and neutral
voltages (V_LL = 2 * V_LN * sin(60 deg) = sqrt(3) * V_LN). It is NOT a
function of the phase count.
Both call sites are currently reachable only when numberOfPhases === 3
(one guarded explicitly at CoherentMeterValueBuilder.ts:281; the other
implicitly via the phaseLineToLineVoltageMeterValues + phase-rotation
plumbing that only supports 3-phase). Numerically the emitted value is
unchanged, but the formula was a trap: any future relaxation of the
3-phase guard would silently emit sqrt(2) * V_LN at N=2, which is
physically meaningless.
Replace both call sites with Math.sqrt(3) and add a code comment stating
the physics derivation so the constant cannot be re-parameterized on the
phase count.
* refactor(auth): standardize on OCPP 2.0.1 terminology in auth subsystem
The recently-added auth subsystem used bare 'OCPP 2.0' throughout its
comments and JSDoc while the rest of the codebase (README, AGENTS.md,
all other source files) uses 'OCPP 2.0.1' as the canonical spec version
and 'OCPP 2.0.x' as the family. The auth subsystem also used non-existent
forms 'OCPP 2.0+' and 'OCPP 2.0/2.1' that misrepresented the supported
spec range: OCPP 2.1 is not implemented.
Bulk-replace across src/charging-station/ocpp/auth/:
- 'OCPP 2.0+' -> 'OCPP 2.0.1'
- 'OCPP 2.0/2.1' -> 'OCPP 2.0.1'
- bare 'OCPP 2.0' -> 'OCPP 2.0.1' (negative lookahead on '.digit' so
existing 'OCPP 2.0.1' and 'OCPP 2.0.x' occurrences are preserved)
* refactor(ocpp/1.6): harmonize 'Central System' capitalization for logs and docs
Two mismatches surfaced against OCPP 2.0 counterparts:
1. `csmsName` field values in OCPP16IncomingRequestService.ts:176 and
OCPP16ResponseService.ts:129 held the lowercase two-word form
'central system'. The abstract base classes interpolate this field
into runtime error messages, producing 'not registered on the central
system' for OCPP 1.6 versus 'not registered on the CSMS' for OCPP
2.0. Two spec-correct forms are involved (OCPP 1.6 uses 'Central
System'; OCPP 2.0.1 uses 'CSMS' for 'Charging Station Management
System'), so keep the spec-correct term per version but capitalize
the 1.6 value to match OCPP 1.6 spec convention.
2. JSDoc prose across OCPP 1.6 files ('sends X to the central system',
'response from the central system', etc.) mixed the lowercase form
with the capitalized 'Central System (CS)' used in the module-level
docstring of OCPP16IncomingRequestService.ts. Normalize to the
capitalized form throughout src/charging-station/ocpp/1.6/.
Zero code behavior change; only logged strings and comment prose shift
casing. Gates pass (typecheck / lint).
* refactor: apply numeric-literal underscore separators to 5-digit-plus constants
Numeric-literal style was inconsistent: OCPP20Constants.ts uses
underscore separators for readability (e.g. `30_000`, `5_000`),
while src/utils/Constants.ts and other constants files used bare
literals (`60000`, `30000`, `1048576`).
Apply underscore separators uniformly to 5+ digit numeric literals
across the constants files, matching the OCPP20Constants.ts precedent:
4-digit values (1000, 3600, 5000, 8080) are intentionally left unchanged;
they include port literals (`DEFAULT_UI_SERVER_PORT = 8080`) where the
underscore form is unusual. AGENTS.md numeric-literal style is a
readability convention rather than an enforced rule; applying it only
above the 5-digit threshold matches the existing OCPP20Constants.ts
pattern.
Two low-risk harmonization findings from the from-zero audit.
1. README OCPP 2.0.x commands table missing 'Authorize' (Lane 5 M2)
The 'Authorize' command is fully implemented for OCPP 2.0.x:
- OCPP20RequestCommand.AUTHORIZE registered
- OCPP20RequestService.ts / OCPP20ResponseService.ts handlers wired
- OCPP20ServiceUtils.ts sending helper
but the README '#### C. Authorization' section listed only 'ClearCache'.
Add ':white_check_mark: Authorize' alongside 'ClearCache' so the docs
match the actual implementation surface.
2. Extract WILDCARD_HOSTS set (Lane 4 M20)
AbstractUIServer.ts:1325 inlined the three literals '', '0.0.0.0', '::'
as a triple '===' chain to detect wildcard host configuration, while
UIServerAccessPolicy.ts:23 already held the same three values in a
module-scoped 'WILDCARD_HOSTS' set. Two independent definitions of
the same domain concept.
Export WILDCARD_HOSTS (typed 'ReadonlySet<string>') from
UIServerAccessPolicy.ts, import it in AbstractUIServer.ts, and
replace the inline OR chain with 'WILDCARD_HOSTS.has(configuredHost)'.
Zero behavior change. Gates pass (typecheck / lint).
* fix(ocpp/1.6): return Accepted for DataTransfer with matching vendor and messageId
Per OCPP 1.6 §4.3, the DataTransfer response statuses are:
- `UnknownVendorId` when the vendorId is not recognized
- `UnknownMessageId` when the vendorId is recognized but the messageId
is not
- `Accepted` when both vendor and message are recognized
`handleRequestDataTransfer` previously returned `UnknownMessageId` for
*any* non-null messageId, even when the vendorId matched the station's
configured `chargePointVendor`:
if (vendorId !== chargingStation.stationInfo?.chargePointVendor) {
return ... UNKNOWN_VENDOR_ID
}
if (messageId != null) {
return ... UNKNOWN_MESSAGE_ID // wrong: any non-null messageId
}
That rejected every legitimate DataTransfer with a messageId, regardless
of whether the messageId would have been supported. The simulator does
not maintain a per-vendor messageId registry (it has no way to know
which custom messageIds a real charge point would accept), so the
spec-correct behaviour is to accept any messageId once the vendor
matches.
Drop the erroneous `messageId != null` branch and return `Accepted`
whenever the vendor matches; the `UnknownVendorId` path is unchanged.
Add a source comment citing OCPP 1.6 §4.3 to prevent regression of the
buggy check.
Test update: the existing 'should return UnknownMessageId for matching
vendor with messageId' test asserted the old buggy behaviour. Rewritten
as 'should return Accepted for matching vendor with messageId (no
messageId registry per §4.3)' to lock in the spec-correct outcome.
* fix(ocpp/1.6): emit FirmwareStatusNotification(Installed) after Installing
Per OCPP 1.6 §4.5, the Charge Point SHALL notify the CSMS of firmware
update progress via FirmwareStatusNotification. `updateFirmwareSimulation`
already emitted the intermediate statuses (Downloading -> Downloaded ->
Installing) but never signalled successful completion — after emitting
`Installing`, the simulation went straight to the optional reset step
(if configured) or returned, leaving the CSMS with no confirmation
that installation actually finished.
Insert an `Installed` status emission between the InstallationFailed
early-return branch and the optional reset:
Behaviour on the failure path is unchanged: the InstallationFailed
branch still returns early without emitting `Installed`. Behaviour
when `reset` is disabled is unchanged apart from the additional
notification (which is the whole point of the fix).
* fix(ocpp/1.6): return Failed (not NotSupported) when LocalAuthList is disabled or manager missing
Per OCPP 1.6 §5.15, the SendLocalList response statuses have distinct
semantics:
- NotSupported: the LocalAuthListManagement feature profile is not
implemented by the Charge Point
- Failed: the feature is implemented but the specific update failed
(disabled, unavailable, or otherwise unable to apply the change)
`handleRequestSendLocalList` correctly returned NotSupported when the
LocalAuthListManagement profile is absent (line 1534). But two
subsequent guards also returned NotSupported when:
- `LocalAuthListEnabled` config is false (feature supported but disabled)
- `getLocalAuthListManager()` returns null (feature supported but manager
not wired)
Both scenarios describe a Charge Point that supports the feature — the
LocalAuthListManagement profile is present — but cannot apply this
particular update. Per spec, that is Failed, not NotSupported.
Change both guards to return
`OCPP16Constants.OCPP_SEND_LOCAL_LIST_RESPONSE_FAILED`. The profile-
absent guard (line 1534) is unchanged — it correctly reports
NotSupported when the feature is not implemented at all.
Test updates: two existing tests locked in the old NotSupported result.
Renamed and updated to assert Failed with an inline reference to §5.15
so the spec-correct outcome is regression-locked.
* chore(auth): remove OCPPAuthServiceImpl from public barrel
`OCPPAuthServiceImpl` is the concrete implementation of the
`OCPPAuthService` interface. External code should construct instances
through `OCPPAuthServiceFactory` (which internally uses the class)
rather than instantiating it directly. Exporting the concrete class
from the auth barrel published an implementation detail that:
- Adds public API surface with no consumer benefit
- Encourages callers to bypass the factory's per-station singleton
- Complicates future refactoring of the concrete class
Grep across src/ and ui/ confirms zero consumers of
`OCPPAuthServiceImpl` via the barrel (`ocpp/auth/index.js`); the
factory imports it via the direct file path within the same subtree.
Two test files (`OCPP20ResponseService-CacheUpdate.test.ts`,
`OCPP20ServiceUtils-AuthCache.test.ts`) did import it via the barrel;
routed to the direct path so they still exercise the concrete class
where explicitly needed.
Also correct the module-level JSDoc from 'OCPP 1.6 and 2.0' to
'OCPP 1.6 and 2.0.1' to match the canonical spec-version convention
used throughout the codebase.
* refactor: extend OCPP 2.0.1 terminology to non-auth paths and compress physics comment
Two round-3 review findings from PR #1960 self-review addressed together.
1. Round-3 High: terminology scope creep
Prior commit dbc3f3ff bulk-replaced OCPP 2.0 -> OCPP 2.0.1 in
src/charging-station/ocpp/auth/ only (65 occurrences). Identical drift
existed across the rest of the OCPP 2.0.x code paths, including the
invalid OCPP 2.0+ form that dbc3f3ff had explicitly corrected in auth/
but left intact in ~10 sites of OCPP20IncomingRequestService JSDoc.
After dbc3f3ff, auth/ said OCPP 2.0.1 while the rest of the OCPP 2.0.x
subtree still said OCPP 2.0 / OCPP 2.0+ — the terminology divergence
was worse, not better.
Apply the same three-pass perl regex (strip +, collapse /2.1,
negative-lookahead on .digit for bare 2.0) to:
63 total substitutions. Zero code behavior change; comment/JSDoc/test
description content only. Zero remaining bare OCPP 2.0 refs under the
scanned scope.
2. Round-3 Low: physics comment verbosity
The sqrt(3) physics anchor comment added in commit 34a574f9 spanned 8
lines with tangential explanations. Compress to 3 lines that keep the
essential physics anchor (V_LL = sqrt(3) * V_LN, 30-degree phase
separation, 3-phase-only constraint).
Documented as still-deferred:
- Narrative comment at OCPP16IncomingRequestService.ts:276 — kept under
M5 bulk cleanup deferral.
- OCPP 2.0.x handleRequestDataTransfer UnknownVendorId design — a
different intentional choice, not the same bug as OCPP 1.6 C3.
All gates green (format / typecheck / lint / build / test) across root,
ui/cli, ui/web. Circular-deps baseline of 61 preserved.
* chore: complete OCPP 2.0.1 terminology sweep and drop redundant physics comment line
Round-4 review findings from PR #1960 self-review addressed together.
1. Terminology sweep completion (Low)
The round-3 extension (commit e03368ff) covered
src/charging-station/ocpp/2.0/**, OCPPServiceUtils.ts, UIMCPServer.ts,
and payloadBuilders.test.ts, but the audit's terminology finding
applied to the whole codebase. Four bare 'OCPP 2.0' refs survived in
charging-station/ files outside ocpp/2.0/:
Apply the same negative-lookahead perl regex; 4 substitutions, zero
code behaviour change. The user-facing warning in ConfigurationKeyUtils
now cites the spec version consistently with the rest of the codebase.
2. Physics comment redundancy (Info)
The compressed sqrt(3) anchor comment from commit e03368ff sat
directly below a preexisting one-liner ('Line-to-line voltage is only
defined for 3-phase AC') that duplicated the L-L === 3-phase constraint
already stated in the compressed block ('Defined only for numberOfPhases
=== 3'). Delete the preexisting redundant line; the compressed anchor
carries the full physics rationale (V_LL = sqrt(3) * V_LN, 30-degree
phase separation, 3-phase-only constraint).
Test files (~15 refs of 'OCPP 2.0' as describe/it group labels) are
intentionally left as-is: those are test-grouping labels rather than
spec-version references, and renaming them would break dev-side test
regex filters without functional benefit.
Gates green (format / typecheck / lint / build / test) across root;
ui/cli and ui/web untouched. Circular-deps baseline of 61 preserved.
* fix(ocpp/2.0): correct spec conformance for value size constants
OCPP 2.0.1 mandates two distinct value size caps that the codebase collapsed
into a single MAX_VARIABLE_VALUE_LENGTH = 2500 constant, which caused the
SetVariable code path to accept payloads up to 2.5x the spec-defined limit
(§2.1.20: ConfigurationValueSize.maxLimit = 1000; SetVariableDataType.attributeValue
is string[0..1000]) and mislabeled the reporting cap (§2.1.21: ReportingValueSize.maxLimit = 2500).
Three independent cleanups from the from-zero audit, bundled to share quality gates.
1. Extract shared Zod-issue to FieldError helpers (z-A1-F1)
Two identical inline duplications (in ConfigurationValidation and
TemplateValidation) mapped ZodError.issues to { message, path } records
and joined them into the same ' - <path>: <message>' field summary.
Extract two exported helpers in ConfigurationMigrations:
- mapZodIssuesToFieldErrors(zodError)
- formatFieldErrorsSummary(fieldErrors)
Update both call sites; TemplateValidationError.fieldErrors is retyped
from an inline object array to the shared FieldError (structurally
identical, public surface preserved).
2. Centralize ui/web skin/theme storage keys in core/Constants (z-A2-F3)
SKIN_STORAGE_KEY, DEFAULT_THEME, and THEME_STORAGE_KEY previously lived
in the composables (useSkin.ts, useTheme.ts) even though main.ts and
SkinLoadError.vue imported them from there for storage bootstrap,
mirroring the already-centralized DEFAULT_SKIN. Move all three to
ui/web/src/core/Constants.ts, re-export via the @/core barrel, and update
the composables plus the two external consumers to import from @/core.
Behaviour and localStorage keys are unchanged.
3. Adopt ui-common extractErrorMessage at 3 sites (z-A2-F4)
Replace three copies of the 'error instanceof Error ? error.message :
String(error)' pattern with extractErrorMessage(error) (ui-common already
used this helper in ui/cli/src/config/loader.ts, output/json.ts,
output/formatter.ts, and ui/web/src/skins/modern/utils/errors.ts):
- ui/cli/src/commands/action.ts
- ui/cli/src/commands/skill.ts
- ui/web/src/shared/composables/useSkin.ts
Rationale for a deferred audit item (z-A1-F2, UIServerSecurity DEFAULT_*
constants to global Constants): declined. The six constants
(DEFAULT_MAX_PAYLOAD_SIZE_BYTES, DEFAULT_RATE_LIMIT, DEFAULT_RATE_WINDOW_MS,
DEFAULT_MAX_STATIONS, DEFAULT_MAX_TRACKED_IPS,
DEFAULT_COMPRESSION_THRESHOLD_BYTES) are ui-server-security tunables used
only within src/charging-station/ui-server/. Promoting them into the
charging-station-wide Constants class would blur module boundaries with
no consumer benefit; co-location with the security domain is intentional.
Public API surface is unchanged. All gates pass (format / typecheck / lint /
build / test) across root, ui/cli, and ui/web; skott circular-deps baseline
of 62 preserved.
* refactor(ui/web): align default-skin fallback and extract host:port literal
Two independent slop cleanups in ui/web from the from-zero audit.
1. Use canonical DEFAULT_SKIN in place of the 'classic' literal fallback (z-A2-F6)
`ui/web/src/main.ts:62` used a hardcoded 'classic' as the fallback for
`config.skin ?? 'classic'` even though the codebase declares the canonical
default in `ui/web/src/core/Constants.ts`:
export const DEFAULT_SKIN = 'modern'
and `SkinLoadError.vue` already uses DEFAULT_SKIN as the recovery target.
The magic string kept the two out of sync: a fresh boot with neither
localStorage nor config.skin would land on 'classic' while the rest of
the app treated 'modern' as the default. Route the fallback through
DEFAULT_SKIN so there is a single source of truth for the default skin.
Behavior change (intentional): first boot with no persisted skin and no
`config.skin` now initializes on 'modern' (the declared default) instead
of 'classic'. Users who explicitly set a skin (via config or the skin
selector) are unaffected because their choice is honored before the
fallback kicks in.
2. Extract 'host:port' literal in UIClient WebSocket adapter (z-A2-F7)
Three interpolations of `${config.host}:${config.port.toString()}` in the
onerror/onopen handlers of `createClientWithAbort` are collapsed into a
single `uiServerAddress` local computed once at the top of the closure.
Zero behavior change; just removes the triplicated interpolation.
Skipped audit item (z-A2-F5, adopt `nonEmptyStringOrUndefined` in
`useSetUrlForm` for supervisionUser/supervisionPassword): declined.
Per the WebSocket protocol contract documented in README.md ('setSupervisionUrl'
procedure), an empty string `""` for user/password explicitly clears the
existing CSMS auth, while `undefined` preserves it. Wrapping the form values
with `nonEmptyStringOrUndefined` would silently change the UX: leaving a
password field blank would preserve the old password instead of clearing it,
with no user-facing indication of the change. Keeping the raw pass-through
matches the documented protocol semantics.
Gates green (format / typecheck / lint / build / test:coverage) for ui/web;
root and ui/cli untouched.
* refactor: prune unused barrel exports and downgrade internal-only symbols
Five symbols were exported from src/utils/index.ts and src/types/index.ts but
have zero consumers in src/, tests/, ui/common/, ui/cli/, or ui/web/. Prune
them from the barrels; since each is also unused outside its defining file,
downgrade the file-level 'export' keyword so the internal-only status is
enforced at compile time (except ElementsPerWorkerType which had no consumer
at all and is fully removed).
Pruned barrel exports (root package has no library API surface — package.json
'exports' points only to dist/start.js — so unused barrel entries are dead):
- DEFAULT_PERSIST_STATE (src/utils/index.ts):
used only inside Configuration.ts:196; barrel entry removed; definition
downgraded from 'export const' to 'const'.
- UIServerAccessPolicySchema (src/utils/index.ts):
used only inside ConfigurationSchema.ts:243 as .optional() nested schema;
barrel entry removed; 'export const' -> 'const'.
- UIServerAuthenticationSchema (src/utils/index.ts):
used only inside ConfigurationSchema.ts:244 as .optional() nested schema;
barrel entry removed; 'export const' -> 'const'.
- AtomicWriteOptions (src/utils/index.ts):
used only inside FileUtils.ts as parameter type for atomicWriteFile /
atomicWriteFileSync; barrel entry removed; 'export interface' -> 'interface'.
- ElementsPerWorkerType (src/types/index.ts + ConfigurationData.ts):
no consumer anywhere; both barrel entry and type alias definition removed.
Kept (audit misclassified as dead):
- isCertificateBased, isOCPP16Type, isOCPP20Type, requiresAdditionalInfo:
all four exercised by tests/charging-station/ocpp/auth/types/AuthTypes.test.ts
— pruning would drop test coverage.
- StrictTemplateSchema (src/charging-station/index.ts):
documented escape hatch ('For CI strict mode' per TemplateSchema.ts:309);
intentional public surface for ad-hoc strict validation with no in-repo
consumer. Kept to honor documented intent.
All gates pass (format / typecheck / lint / build / test across root, ui/cli,
ui/web); circular-deps baseline preserved.
* refactor(auth): rename ConfigValidator to AuthConfigValidator for name coherence
The file src/charging-station/ocpp/auth/utils/ConfigValidator.ts exports a
single object under the name AuthConfigValidator and uses the string
'AuthConfigValidator' as its moduleName for log prefixes, but the file itself
was named after a more generic 'ConfigValidator' concept. Two consumers and
the test file already refer to the symbol as AuthConfigValidator, and the
generic file name overlaps semantically with the unrelated
src/utils/ConfigurationValidation.ts (application config validation) —
grep 'ConfigValidator' surfaces both, which is easy to misread.
Rename via 'git mv' to preserve history:
- src/charging-station/ocpp/auth/utils/ConfigValidator.ts
-> src/charging-station/ocpp/auth/utils/AuthConfigValidator.ts
- tests/charging-station/ocpp/auth/utils/ConfigValidator.test.ts
-> tests/charging-station/ocpp/auth/utils/AuthConfigValidator.test.ts
Update imports in AuthComponentFactory and OCPPAuthServiceImpl to reference
the new file path. No source content changes; exported symbol name and
module identity are unchanged.
Skipped audit items in this tier:
- z-A1-F3 (OCPP_WEBSOCKET_TIMEOUT_MS -> global Constants): declined. The
constant already lives in src/charging-station/ocpp/OCPPConstants.ts, which
is the OCPP-domain constants class. Promoting it to the charging-station-
wide Constants would blur module boundaries with no consumer benefit —
consistent with the same rationale that deferred UIServerSecurity DEFAULT_*
centralization.
- z-A1-F4 (rename DEFAULT_ATG_WAIT_TIME_MS -> DEFAULT_ATG_RETRY_DELAY_MS):
declined. All three call sites in AutomaticTransactionGenerator
(waitChargingStationAvailable, waitConnectorAvailable,
waitRunningTransactionStopped) are sleep() delays inside 'wait for
condition' polling while-loops, not retry loops on a failing operation.
'RETRY_DELAY' would misdescribe the semantics; the current 'WAIT_TIME' is
the accurate label.
* test: use TEST_SUPERVISION_URL constant instead of hardcoded ws://localhost:8080
Six test files repeated the literal 'ws://localhost:8080' 15 times as a
supervision URL fixture. tests/utils/TestNetworkConstants.ts already provides
the canonical TEST_SUPERVISION_URL:
Route each fixture through that constant so a change to the production UI
server host/port defaults propagates to every test without a search/replace
sweep. Files updated:
- tests/charging-station/TemplateValidation.test.ts (6 sites)
- tests/charging-station/TemplateMigrations.test.ts (3 sites)
- tests/charging-station/TemplateSchema.test.ts (1 site)
- tests/utils/ConfigurationSchema.test.ts (3 sites — including the
first entry of the ['ws://localhost:8080', 'ws://localhost:8081'] array;
the second literal is intentionally distinct and stays hardcoded)
- tests/utils/ConfigurationValidation.test.ts (1 site)
- tests/performance/storage/MikroOrmStorage.test.ts (1 site)
The JSDoc example in tests/charging-station/mocks/MockWebSocket.ts is
preserved as-is (documentation, not a fixture).
Fixture values, test assertions, and behavior are unchanged.
All gates pass (format / typecheck / lint / build / test).
* refactor: reword narrative comments and anchor non-systemic eslint-disable directives
Two audit tiers bundled to share the gate run.
Tier 8 - narrative/imperative comments -> state-describing form:
Three comments in touched files were rewritten from developer-narrative to
state-describing per the repo comment convention.
- src/charging-station/ocpp/2.0/OCPP20CertificateManager.ts
'We need the directory: basePath/ChargingStationCertificate' is replaced
with a two-line state description of what dirPath is and how it relates
to the getCertificatePath return value. Anchors the non-obvious
resolve(certFilePath, '..') derivation to the surrounding directory check.
- src/charging-station/ocpp/auth/strategies/CertificateAuthStrategy.ts
'Should not reach here due to canHandle check, but handle gracefully'
becomes 'Unreachable when the canHandle contract holds; defensive fallback
for unsupported OCPP versions'.
'Must have certificate data in the identifier' becomes
'The identifier carries certificate data' (paired with a matching rewrite
of 'Certificate authentication must be enabled' -> '...is enabled per
configuration').
Tier 9 - '-- reason' anchors on non-systemic eslint-disable directives:
Sixteen one-off eslint-disable-next-line directives now carry the '-- reason'
anchor already used elsewhere in the repo (e.g. UIServerFactory.ts,
OCPPResponseService.ts). Systemic directives (repeated idioms such as
@typescript-eslint/no-redeclare for the enum + type declaration-merging
pattern, restrict-template-expressions for logger calls, no-extraneous-class
for the Constants classes, no-unnecessary-type-parameters for contravariant
handler bridges) are left unchanged in this pass — those are pattern-level
choices that belong in a repo-wide rationale, not per-site.
Behavior is unchanged; every directive still disables the same rule at the
same line, only the rationale is now recorded inline.
Gates green (format / typecheck / lint / build / test) across root and ui/web;
circular-deps baseline of 62 preserved.
* refactor(utils): extract FieldError into a single-purpose module
Two review findings from PR #1959 addressed together — both are internal
tidying with no behavior change.
1. Extract Zod field-error helpers out of ConfigurationMigrations
ConfigurationMigrations owned the FieldError interface plus
mapZodIssuesToFieldErrors and formatFieldErrorsSummary, but those helpers
are Zod-issue formatters used by TemplateValidation and
ConfigurationValidation — neither of which is a configuration-migration
concern. Move the interface and both helpers into a dedicated single-
purpose module src/utils/FieldError.ts, re-export via the utils barrel,
and route all three consumers to import from ./FieldError.js:
- src/utils/ConfigurationMigrations.ts now imports the FieldError type
it still uses in remapDeprecatedKeys and RemapDeprecatedKeysResult.
- src/utils/ConfigurationValidation.ts imports the type plus both helpers.
- src/charging-station/TemplateValidation.ts switches from the deep
import of ConfigurationMigrations to the deep import of FieldError.
The utils/index.ts barrel gains a FieldError export block placed
alphabetically between ErrorUtils and FileUtils.
2. Add TEST_SUPERVISION_URL_ALT_PORT for two-URL fixtures
tests/utils/ConfigurationSchema.test.ts had a mixed literal/constant
array — [TEST_SUPERVISION_URL, 'ws://localhost:8081'] — used to verify
supervisionUrls accepts a string array. Both entries now come from
TestNetworkConstants: the alternate port is DEFAULT_UI_SERVER_PORT + 1,
keeping the same 'derived from production defaults' contract. The
TestNetworkConstants file also factors HOST/PORT into local constants
to keep the two URL definitions DRY.
Additional review-finding dispositions:
- Finding #3 (visual QA on the DEFAULT_SKIN fallback change): already
regression-locked by existing Vitest coverage —
tests/unit/skins/registry.test.ts:12 pins DEFAULT_SKIN === 'modern',
tests/unit/shared/composables/useSkin.test.ts exercises
switchSkin(DEFAULT_SKIN) and asserts activeSkinId defaults to
DEFAULT_SKIN. No additional test needed.
- Finding #4 (Validation vs Validator terminology): withdrawn on
re-inspection. *Validation.ts modules host the validation process
(validateConfiguration, ConfigurationValidationError). AuthConfigValidator
is a validator entity object with a .validate() method. The suffix
distinguishes 'process module' from 'entity object' — intentional and
consistent, not divergent.
- Finding #5 (spec coverage for VariableCharacteristics.valueList and
EventData.actualValue): pre-existing feature-not-implemented gaps in the
simulator (grep confirms neither valueList nor NotifyEvent code paths
exist under src/charging-station/ocpp/2.0/). Bounding these would
require implementing the underlying features first; out of scope for
this review-response commit.
Public API surface unchanged. Gates green (format / typecheck / lint /
build / test); circular-deps baseline of 62 preserved.
* chore(charging-station): unify TemplateValidation utils imports through the barrel
Round-2 review finding: TemplateValidation.ts mixed two import styles for
the same target directory. Lines 6-10 pulled FieldError + helpers via a
deep import ('../utils/FieldError.js') while line 11 pulled other utilities
via the barrel ('../utils/index.js'). Both symbol groups originate from
src/utils/ and the barrel already re-exports the FieldError trio (utils/
index.ts:43-47).
Merge both into a single barrel import block, alphabetically ordered
(case-insensitive) per the repo's perfectionist convention:
assertIsJsonObject, clone, type FieldError, formatFieldErrorsSummary,
isEmpty, isNotEmptyString, logger, mapZodIssuesToFieldErrors.
The 'type FieldError' inline qualifier is kept in the mixed named-import
(same pattern used by utils/index.ts's own FieldError re-export block).
Behavior unchanged; only import mechanics shift.
All gates pass (format / typecheck / lint / build / test); skott circular-
deps baseline of 62 preserved.
Module-scope constants:
- WS_DEFLATE_{CONCURRENCY_LIMIT,SERVER_MAX_WINDOW_BITS,ZLIB_*} in UIWebSocketServer
- STATISTICS_PERCENTILE in PerformanceStatistics
- AUTH_CACHE_LIFETIME_{MIN,MAX}_SECONDS, AUTH_CACHE_MIN_ENTRIES,
AUTH_TIMEOUT_{MIN,MAX}_SECONDS in ConfigValidator
- JITTER_PERCENT local to src/worker/ (module standalone; no cross-dep)
- SIGINT/SIGTERM_EXIT_CODE, MISSING_VALUE_PLACEHOLDER,
TEMPLATE_NAME_SUFFIX, TRUNCATED_HASH_ID_LENGTH in ui/cli output
- SKIN_ERROR_RELOAD_COUNT_KEY in ui/web core
Helper adoption:
- millisecondsToSeconds (date-fns) at AbstractUIServer / InMemoryAuthCache / test
- isNotEmptyString in TemplateValidation
- WebSocketReadyState enum (ui-common) at CLI wsIcon/renderers and web stationStatus
- OCPP16ChargePointStatus / OCPP20ConnectorStatusEnumType enum keys instead
of string literals in CLI STATUS_ABBREVIATIONS and web CONNECTOR_STATUS_VARIANT
- nonEmptyStringOrUndefined shared helper (ui/web/src/shared/utils) adopted
by 3 composables + skin error extractor
- Extracted OCPPRequestService.logRequestHandlerError() consumed by OCPP16/20
RequestService (byte-identical catch shell)
Barrel/module hygiene:
- Close barrel gap: meter-values/index.ts exposes disposeCoherentSessionRuntime
(JSDoc already advertised it)
- UIServerAccessPolicy imports UI_SERVER_ACCESS_POLICY_DEFAULTS via utils/index
barrel (drop deep import)
- ui/web skins route 8 imports through @/shared/utils/index barrel
- UIServiceWorkerBroadcastChannel type-only imports AbstractUIService via
ui-server/index barrel (safe by type erasure; comment documents the constraint)
Dead barrel exports pruned (0 external consumers):
- src/utils: FieldError, applyConfigurationMigration, coerceConfigurationVersion,
queueMicrotaskErrorThrowing (tests updated to import from source)
- charging-station/meter-values: ChargingCurvePoint
- ui-server/mcp: MCPToolSchema
- ocpp/auth: AuthStats, CacheStats, CertificateAuthProvider, CertificateInfo
- ocpp/2.0/__testable__: SendMessageFn, TestableRequestServiceOptions/Result
re-exports; TestableOCPP20IncomingRequestService downgraded to module-private;
TestableOCPP20VariableManager type re-export dropped
Test constants:
- Heartbeat interval sites use Constants.DEFAULT_HEARTBEAT_INTERVAL_MS /
secondsToMilliseconds
- New tests/utils/TestNetworkConstants.ts: TEST_SUPERVISION_URL derived from
Constants.DEFAULT_UI_SERVER_{HOST,PORT}; adopted at MessageChannelUtils.test
+ StorageTestHelpers + ConfigurationFixtures
- Rate-limit / mock-timers-tick 60000 literals replaced by minutesToMilliseconds(1)
in UIServerSecurity / OCPP20CertSigningRetryManager / SignedMeterValues tests
Deliberately not applied (documented rationale):
- src/worker/WorkerAbstract isNotEmptyString adoption: src/worker/ is a
standalone module with zero cross-module dependencies; retracted rather
than adding an import to ../utils
- AbstractUIService → broadcast-channel/index value-import via barrel:
empirically amplifies circular dependencies 62 → 71 because the barrel also
re-exports ChargingStationWorkerBroadcastChannel whose transitive graph
closes a runtime cycle back through ui-server/index; deep import kept
Fixes 9 findings surfaced by fan-out self-review of commit a25f4ba9.
Behavior-preserving; no public API change.
Findings addressed:
- H1 (DRY, Constants.ts) — `DEFAULT_AUTH_CACHE_MAX_ABSOLUTE_LIFETIME_MS` and
`MS_PER_DAY` both held the literal `86_400_000` (same value, same commit).
Class-static forward-reference is illegal in TypeScript (TS2729:
"Property 'B' is used before its initialization"), so the shared literal is
hoisted to module-scope `DAY_IN_MS` / `DAY_IN_SECONDS` helpers before the
class and referenced from both derived defaults and canonical time-unit
constants. Single source of truth restored.
- H2 (DRY, ConfigValidator.ts) — `AUTH_CACHE_LIFETIME_MAX_SECONDS = 86_400`
duplicated `Constants.SECONDS_PER_DAY = 86_400`. Fixed cross-file by
importing `Constants` (already reachable via `../../../../utils/index.js`)
and referencing `Constants.SECONDS_PER_DAY`.
- M1 (helper adoption, ui/cli format.ts:102) — `fuzzyTime` still used
`diff / 1000` while the same PR adopted `millisecondsToSeconds` from
`date-fns` at 4 other sites. Replaced by `millisecondsToSeconds(diff)`
for consistency.
- L1 (naming coherence, ConfigValidator.ts) — Renamed
`AUTH_CACHE_LIFETIME_{MIN,MAX}_SECONDS` to
`AUTH_CACHE_TTL_{MIN,MAX}_SECONDS` to align with the pre-existing
`DEFAULT_AUTH_CACHE_TTL_SECONDS` naming family. Two words for one concept
eliminated within the auth module.
- L2 (naming coherence, ui/cli output) — Renamed cli
`MISSING_VALUE_PLACEHOLDER` to `EMPTY_VALUE_PLACEHOLDER` to align with
ui/web's pre-existing `EMPTY_VALUE_PLACEHOLDER`. Values remain
medium-specific (`'–'` cli / `'Ø'` web), only the semantic role name is
shared. 6 sites updated.
- L3 (stranded export, format.ts) — `TRUNCATED_HASH_ID_LENGTH` had zero
external consumers (only the in-file default arg on `truncateId`).
Dropped `export` keyword.
- L4 (stranded export, format.ts) — `TEMPLATE_NAME_SUFFIX` had zero external
consumers (only the in-file `stripTemplateSuffix` helper). Dropped
`export` keyword; the helper itself keeps `export` (2 external consumers).
- L5 (documented duplication, WorkerUtils.ts) — JSDoc on `JITTER_PERCENT`
now explicitly cross-references `Constants.DEFAULT_RECONNECT_JITTER_PERCENT`
and documents the maintenance invariant. The two constants must stay
synchronized manually because `src/worker/` is a standalone module (no
cross-module value imports).
- I1 (anchor accuracy, StatisticUtils.ts:52) — The
`no-unnecessary-condition` disable reason is reworded from a plausible-
but-imprecise "JSON.parse violation" framing to the actual root cause:
`baseIndex+1` can equal `sortedDataSet.length`, and TypeScript indexes
as `number` (no `noUncheckedIndexedAccess`), so the runtime `!= null`
guard is genuinely needed for out-of-bounds protection.
Empirical retraction verifications:
- B1-5 (cycle amplification 62 → 71) — Re-verified by applying only the
retracted change on top of `daad8a9d` and running `pnpm circular-deps`:
62 baseline, 71 after B1-5, 62 after revert. Retraction rationale holds.
- A1-17 (`src/worker/` standalone) — Re-verified by `grep -rE
"^import.*\.\./" src/worker/`: zero cross-module value imports; the new
L5 JSDoc documents the exception rationale in-source.
Doc-only follow-up to commit 6ba8503c. Behavior unchanged.
- G1 (JITTER doc accuracy) — Reworded both `Constants.DEFAULT_RECONNECT_JITTER_PERCENT`
and worker-local `JITTER_PERCENT` JSDoc from the empirically false claim
"symmetric ±20 %" to "bounds the jitter within ±20 %". The `randomizeDelay`
algorithm couples sign and magnitude through a single uniform draw, so with
`random ∈ [0, 1)` and `sign = random < 0.5 ? -1 : +1` the output range is
`(0.9·delay, delay] ∪ [1.1·delay, 1.2·delay)` — asymmetric, with a gap at
`[delay, 1.1·delay)`. The new wording accurately caps the magnitude without
overclaiming symmetry; the WorkerUtils docstring adds an explicit distribution
note pointing to `randomizeDelay`. Algorithm itself pre-dates the PR and is
behavior-preserved.
- A1 (InMemoryAuthCache @param defaults) — Three JSDoc `@param default:` fields
were pointing at raw literals (3600, 86400000, 60000) instead of the promoted
Constants members. Aligned with the pattern already used for `maxEntries`
(which cites `Constants.DEFAULT_AUTH_CACHE_MAX_ENTRIES`): now all six
`@param default:` values reference the canonical `Constants.*` symbol.
- A2 (DEFAULT_REMOTE_AUTH_CACHE_TTL_SECONDS wording) — JSDoc used the phrase
"distinct from local `_TTL_SECONDS = 3600`" where `_TTL_SECONDS` is not an
actual symbol. Replaced by explicit sibling reference
`DEFAULT_AUTH_CACHE_TTL_SECONDS (3600, local cache default)`.
Restores web test suite to full green after documented quality gates surfaced
regressions the prior review rounds missed. Behavior-preserving vs the intent
of round-1 refactor.
Regressions fixed (QG-1/QG-2/QG-3 — blockers surfaced by `pnpm --filter web
test:coverage`):
- `stationStatus.ts` — round-1 finding A2-5 replaced `switch (status?.toLowerCase())`
by a Record keyed by `OCPP16ChargePointStatus.*` / `OCPP20ConnectorStatusEnumType.*`
enum values. That change (a) dropped the case-insensitive contract asserted by
`stationStatus.test.ts:74`, and (b) introduced a module-scope runtime access
to `ui-common` enum values, which fails at module init when the containing
test suite mocks `ui-common` (2 file-level FAILs in `useAddStationsForm.test.ts`
and `useStartTxForm.test.ts`).
Design chosen (Option F in review v4 report): keep the Record structure but
key it by lowercase string literals and normalize input via `.toLowerCase()`
on lookup. Retains the refactor benefit (Record > switch for O(1) lookup,
clearer add/remove semantics), restores case-insensitive input, removes the
module-scope enum dependency, and preserves behaviour for the 9 status values
the switch covered. Enum imports dropped since no other value site remains.
Web test suite: 31 files / 532 tests pass (previously 3 files failed / 1
test failed).
Review v4 low-severity findings:
- v4-A1 `Constants.ts:77` — JSDoc "bounds within ±20 %" for
`DEFAULT_RECONNECT_JITTER_PERCENT` implied a bidirectional range, but algebra
of the sole consumer `computeExponentialBackOffDelay` (`jitter = delay ×
jitterPercent × secureRandom()` with `secureRandom() ∈ [0, 1)`) yields
`jitter ∈ [0, +20 %)` strictly non-negative. Reworded to describe the
additive-uniform-draw semantic and cross-reference the two distinct consumer
distributions.
- v4-A2 `WorkerUtils.ts:12` — "shared jitter policy" overclaimed behavioural
parity between the two consumers (positive-only vs asymmetric with a
probability-zero gap). Softened to "shared jitter magnitude cap".
- v4-B1 `ConfigurationFixtures.ts:67` — `uiServer.options` still hardcoded
`host: 'localhost', port: 8080` after the sibling `supervisionUrls` field on
the same object literal was migrated to `TEST_SUPERVISION_URL`. Extended
`tests/utils/TestNetworkConstants.ts` to also export `TEST_UI_SERVER_HOST`
and `TEST_UI_SERVER_PORT` (re-exports of `Constants.DEFAULT_UI_SERVER_HOST` /
`Constants.DEFAULT_UI_SERVER_PORT`).
- v4-B4 `ConfigurationMigrations.test.ts:130,141` — string literals
`'ws://localhost:8080'` in a file the PR already touched (barrel-to-source
import migration) but where the URL literals were skipped in round 2.
Migrated to `TEST_SUPERVISION_URL`.
- `cspell.config.yaml` — added `subclassing` (used in
`src/charging-station/ui-server/index.ts:10`, pre-existing since #1954). Root
lint now reports 0 errors AND 0 warnings.
Deferred (documented in review v4 report, out of this PR's scope):
- v4-B2 `ConfigValidator.ts` MIN/MAX bounds harmonization (already tracked as M2)
- v4-B3 asymmetry between `1.6/__testable__/index.ts` (exports
`TestableOCPP16IncomingRequestService`) and `2.0/__testable__/index.ts`
(interface demoted to non-exported in round 2). Requires either restoring
the 2.0 export or migrating 1.6 consumers to `ReturnType<typeof …>` —
larger touch, kept for a dedicated follow-up.
- Pre-existing gap/asymmetry in `randomizeDelay`'s distribution
(`(−10 %, 0] ∪ [+10 %, +20 %)` — magnitude and sign both derive from a single
uniform draw). Documented in-source, not fixed by this refactor.
Quality gates (documented per `.serena/memories/task_completion_checklist`):
* docs: tighten prose in 5 comments — state-describing, not narrative
Removes historical/imperative framing ("kept as", "hoisted to", "if a future
edit converts...", "must not access... because") in favour of concise
statements of what is.
Applies the 3 actionable findings from review round 5. Behavior-preserving.
- v5-A1 `SkinLoadError.vue:28-32` — Reworded `resetToDefault` JSDoc from 4-line
imperative `NOTE: ...should clear the reload-count sessionStorage key...`
to 1-line state-describing `Counter is reset by useSkin.switchSkin on
successful load.` Missed by the round-4 comment tightening sweep.
- v5-B1 `TestNetworkConstants.ts` / `ConfigurationFixtures.ts` — Retired
`TEST_UI_SERVER_HOST` / `TEST_UI_SERVER_PORT` exports (1 external consumer
each, both at the same `ConfigurationFixtures.ts:71` expression — criterion 10
requires ≥2 consumers). Inlined `Constants.DEFAULT_UI_SERVER_HOST` /
`Constants.DEFAULT_UI_SERVER_PORT` at the single call site.
`TEST_SUPERVISION_URL` remains exported (5 consumers).
- v5-B2 `ui/web/src/shared/utils/stationStatus.ts` — Removed the module-scope
value import `import { WebSocketReadyState } from 'ui-common'` and switched
`getWebSocketStateVariant` to 4 module-local numeric constants
`WS_STATE_{CONNECTING,OPEN,CLOSING,CLOSED} = 0/1/2/3`. Eliminates the same
ui-common runtime module-scope dependency that the round-4 fix removed for
`CONNECTOR_STATUS_VARIANT`. No behavior change (values match WHATWG WebSocket
readyState spec).
* docs: drop narrative comments in stationStatus.ts
Removes two comment blocks that narrated rationale ("mirror of...",
"module-local to keep...", "test suites mock...") instead of describing
what is. The `WS_STATE_*` constants and lowercase Record keys are
self-documenting.
repositories / e-mobility-charging-stations-simulator.git / log
