refactor: audit-driven cleanup (OCPP 2.0 spec conformance + 5 low-risk refactors...

refactor: audit-driven cleanup (OCPP 2.0 spec conformance + 5 low-risk refactors) (#1959)

* 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).

Replace MAX_VARIABLE_VALUE_LENGTH with two spec-cited constants:
- MAX_CONFIGURATION_VALUE_SIZE = 1000 (SetVariable path, §2.1.20)
- MAX_REPORTING_VALUE_SIZE     = 2500 (GetBaseReport / reporting truncation, §2.1.21)

Remap all consumers:
- OCPP20VariableRegistry: ConfigurationValueSize maxLimit -> MAX_CONFIGURATION_VALUE_SIZE;
  ReportingValueSize + OCPP 2.1 ValueSize maxLimit -> MAX_REPORTING_VALUE_SIZE.
- OCPP20VariableManager readAttributeValue reporting truncation -> MAX_REPORTING_VALUE_SIZE.
- OCPP20VariableManager setVariable effective-limit fallback + absolute cap ->
  MAX_CONFIGURATION_VALUE_SIZE.
- Test fixtures resetReportingValueSize / resetValueSizeLimits use each spec constant
  according to which variable they seed.

Public API surface is not affected (constants are internal to ocpp/2.0).

* refactor: dedupe zod field-error mapping, centralize ui/web skin/theme keys

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.

All gates pass (format / typecheck / lint / build / test).

* 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:

  export const TEST_SUPERVISION_URL =
    `ws://${Constants.DEFAULT_UI_SERVER_HOST}:${Constants.DEFAULT_UI_SERVER_PORT.toString()}`

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.

Anchored one-offs:
- src/types/ocpp/ErrorType.ts @cspell/spellchecker
- src/charging-station/ChargingStation.ts @typescript-eslint/no-deprecated
- src/charging-station/ChargingStation.ts promise/no-promise-in-callback
- src/charging-station/broadcast-channel/ChargingStationWorkerBroadcastChannel.ts
    three @typescript-eslint/no-invalid-void-type + one promise/catch-or-return
- src/charging-station/TemplateMigrations.ts @typescript-eslint/no-dynamic-delete
- src/charging-station/ocpp/1.6/OCPP16IncomingRequestService.ts no-fallthrough
- src/charging-station/ocpp/OCPPServiceUtils.ts @typescript-eslint/no-unsafe-assignment
- src/charging-station/ocpp/OCPPRequestService.ts @typescript-eslint/no-this-alias
- src/charging-station/Bootstrap.ts two @typescript-eslint/unbound-method
- src/worker/WorkerSet.ts promise/no-promise-in-callback
- ui/web/src/shared/composables/useTheme.ts no-void
- ui/web/src/shared/composables/useAsyncAction.ts no-void

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.