refactor: audit-driven cleanup — OCPP conformance fixes, physics, harmonization (#1960)
* fix(ocpp/1.6): correct GetDiagnostics log directory resolution
`handleRequestGetDiagnostics` called `resolve()` with the comma operator
by accident:
resolve((fileURLToPath(import.meta.url), '../', dirname(logFile)))
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:
resolve(dirname(fileURLToPath(import.meta.url)), '../', diagnosticsArchive)
Both call sites now resolve paths relative to the module's parent
directory. Gates pass (typecheck / lint).
* fix(physics): use sqrt(3) for line-to-line voltage derivation
Two sites derived the line-to-line nominal voltage from Math.sqrt(numberOfPhases) * V_LN:
- src/charging-station/ocpp/OCPPServiceUtils.ts:1255
- src/charging-station/meter-values/CoherentMeterValueBuilder.ts:282
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.
Gates pass (typecheck / test, 2955 pass / 0 fail).
* 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)
65 occurrences updated across 5 files. Files touched:
- adapters/OCPP20AuthAdapter.ts (31)
- types/AuthTypes.ts (21)
- strategies/CertificateAuthStrategy.ts (7)
- utils/AuthValidators.ts (5)
- strategies/RemoteAuthStrategy.ts (1)
Zero code behavior change; comment/JSDoc content only.
Gates pass (typecheck / lint).
* 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:
- src/utils/Constants.ts: `60000` -> `60_000` (6 sites),
`30000` -> `30_000`, `
281474976710655` -> `281_474_976_710_655`,
`
2147483647` -> `2_147_483_647`.
- src/charging-station/ocpp/OCPPConstants.ts:
`OCPP_WEBSOCKET_TIMEOUT_MS = 60000` -> `60_000`.
- src/charging-station/ui-server/UIServerSecurity.ts:
`DEFAULT_MAX_PAYLOAD_SIZE_BYTES =
1048576` -> `1_048_576`,
`DEFAULT_RATE_WINDOW_MS = 60000` -> `60_000`,
`DEFAULT_MAX_TRACKED_IPS = 10000` -> `10_000`.
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.
Zero code behavior change. Gates pass (typecheck / lint / test, 2955
pass / 0 fail).
* chore: expose WILDCARD_HOSTS + document Authorize in OCPP 2.0.x README table
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.
Gates pass (typecheck / test, 2955 pass / 0 fail).
* 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:
sleep(...)
requestHandler(FIRMWARE_STATUS_NOTIFICATION, { status: Installed })
stationInfo.firmwareStatus = Installed
if (reset === true) { sleep(...); 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).
Gates pass (typecheck / test, 2935 pass / 0 fail).
* 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.
Gates pass (typecheck / test, 2945 pass / 0 fail).
* 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.
Gates pass (typecheck / lint / test, 2955 pass / 0 fail).
* 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:
- src/charging-station/ocpp/2.0/**/*.ts (56 occurrences)
- src/charging-station/ocpp/OCPPServiceUtils.ts (3)
- src/charging-station/ui-server/UIMCPServer.ts (2)
- ui/common/tests/payloadBuilders.test.ts (2)
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/:
- src/charging-station/ConfigurationKeyUtils.ts:
user-facing warning 'use OCPP 2.0 variable ... instead'
- src/charging-station/meter-values/CoherentMeterValueBuilder.ts:
code comment 'Narrow the OCPP 2.0 SampledValueTemplate.unit ...'
- src/charging-station/TemplateSchema.ts (2 sites):
JSDoc references to 'OCPP 2.0 unit-of-measure descriptor' and
'OCPP 2.0 namespaced keys'
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.