Skip to content

refactor(zwave): extract scenes groups and associations#4732

Draft
AlCalzone wants to merge 30 commits into
alcalzone-backend-refactor-schedules-templatesfrom
alcalzone-backend-refactor-scenes-groups-associati
Draft

refactor(zwave): extract scenes groups and associations#4732
AlCalzone wants to merge 30 commits into
alcalzone-backend-refactor-schedules-templatesfrom
alcalzone-backend-refactor-scenes-groups-associati

Conversation

@AlCalzone

@AlCalzone AlCalzone commented Jul 11, 2026

Copy link
Copy Markdown
Member

Extracts scene, group, virtual-node, and association ownership from ZwaveClient.

  • Keeps public facade methods, persistence, socket projections, and association behavior compatible.
  • Uses live ports and generation fencing across driver and virtual-registry replacement.
  • Review focus: stale async work, virtual-node identity, and scene/group persistence ordering.

Part of #4722. Stacked on #4731.

@coveralls

coveralls commented Jul 11, 2026

Copy link
Copy Markdown

Coverage Report for CI Build 29574997057

Warning

No base build found for commit ed906c3 on alcalzone-backend-refactor-schedules-templates.
Coverage changes can't be calculated without a base build.
If a base build is processing, this comment will update automatically when it completes.

Coverage: 52.094%

Details

  • Patch coverage: 43 uncovered changes across 1 file (338 of 381 lines covered, 88.71%).

Uncovered Changes

File Changed Covered %
api/lib/ZwaveClient.ts 73 30 41.1%
Total (5 files) 381 338 88.71%

Coverage Regressions

Requires a base build to compare against. How to fix this →


Coverage Stats

Coverage Status
Relevant Lines: 7384
Covered Lines: 4030
Line Coverage: 54.58%
Relevant Branches: 4625
Covered Branches: 2226
Branch Coverage: 48.13%
Branches in Coverage %: Yes
Coverage Strength: 33.38 hits per line

💛 - Coveralls

@AlCalzone
AlCalzone marked this pull request as draft July 11, 2026 22:06
AlCalzone added a commit that referenced this pull request Jul 13, 2026
Layer 10 cross-stack comment audit for the scene/group/association
refactor (PR #4732). Removes decorative section dividers, restated
JSDoc that duplicates adjacent code, and history/changelog framing
(finding numbers, 'used to be', 'original ZwaveClient did X').

Preserves and in several spots restores non-obvious domain facts:
generation-fencing/cancellation semantics, stale-driver-across-await
re-resolution, virtual-node lifecycle ownership, deep-equality
comparison rationale for object-valued CCs, and the still-unresolved
Delegates to X comments trimmed to their misleading-signature facts
(getGroups' void-return-but-mutates-in-place quirk) while pure
one-line-body restatements are removed.

Two comments on ports.ts (hasPhysicalNode, getZWaveNode) are corrected
for factual accuracy against their actual ZwaveClient.ts implementations.

No runtime/logic changes; comment-only.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@AlCalzone
AlCalzone force-pushed the alcalzone-backend-refactor-schedules-templates branch from 89a3ab8 to d09a565 Compare July 16, 2026 14:19
AlCalzone added a commit that referenced this pull request Jul 16, 2026
Layer 10 cross-stack comment audit for the scene/group/association
refactor (PR #4732). Removes decorative section dividers, restated
JSDoc that duplicates adjacent code, and history/changelog framing
(finding numbers, 'used to be', 'original ZwaveClient did X').

Preserves and in several spots restores non-obvious domain facts:
generation-fencing/cancellation semantics, stale-driver-across-await
re-resolution, virtual-node lifecycle ownership, deep-equality
comparison rationale for object-valued CCs, and the still-unresolved
Delegates to X comments trimmed to their misleading-signature facts
(getGroups' void-return-but-mutates-in-place quirk) while pure
one-line-body restatements are removed.

Two comments on ports.ts (hasPhysicalNode, getZWaveNode) are corrected
for factual accuracy against their actual ZwaveClient.ts implementations.

No runtime/logic changes; comment-only.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@AlCalzone
AlCalzone force-pushed the alcalzone-backend-refactor-scenes-groups-associati branch from 77beb46 to abf39e8 Compare July 16, 2026 16:43
@AlCalzone
AlCalzone force-pushed the alcalzone-backend-refactor-schedules-templates branch from d09a565 to 978a891 Compare July 16, 2026 20:12
AlCalzone added a commit that referenced this pull request Jul 16, 2026
Layer 10 cross-stack comment audit for the scene/group/association
refactor (PR #4732). Removes decorative section dividers, restated
JSDoc that duplicates adjacent code, and history/changelog framing
(finding numbers, 'used to be', 'original ZwaveClient did X').

Preserves and in several spots restores non-obvious domain facts:
generation-fencing/cancellation semantics, stale-driver-across-await
re-resolution, virtual-node lifecycle ownership, deep-equality
comparison rationale for object-valued CCs, and the still-unresolved
Delegates to X comments trimmed to their misleading-signature facts
(getGroups' void-return-but-mutates-in-place quirk) while pure
one-line-body restatements are removed.

Two comments on ports.ts (hasPhysicalNode, getZWaveNode) are corrected
for factual accuracy against their actual ZwaveClient.ts implementations.

No runtime/logic changes; comment-only.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@AlCalzone
AlCalzone force-pushed the alcalzone-backend-refactor-scenes-groups-associati branch from efea0b6 to ed70f11 Compare July 16, 2026 20:49
@AlCalzone
AlCalzone force-pushed the alcalzone-backend-refactor-schedules-templates branch from 978a891 to ed906c3 Compare July 17, 2026 09:57
AlCalzone and others added 18 commits July 17, 2026 12:11
…ociation services

Adds lazy-resolved port interfaces (persistence, driver, node store,
virtual node registry, socket, utils, logger) that SceneService,
GroupService, and AssociationService will depend on. No runtime
behavior changes yet - ZwaveClient still owns all scene/group/
association state and logic until the following commits wire it up.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Introduces SceneService, a strict-clean class that owns scene
collection state and all scene lifecycle logic: create/remove scene,
bulk set/get, per-value get/add/remove, and activation (including the
per-value delayed-write timeout scheduling and error handling).
Structurally/behaviorally identical to the logic currently still living
in ZwaveClient.ts - ZwaveClient is wired to delegate to this service in
a later commit.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Introduces GroupService, a strict-clean class that owns the
user-defined ("multi-target"/virtual) group collection and its full
lifecycle: CRUD with physical-node validation (rejects broadcast/
long-range/own-node/duplicate/virtual ids, requires >= 2 valid physical
nodes), virtual node materialization/reuse/teardown against the
driver's multicast group, per-node virtual-node refresh throttling,
group-aware value projection for virtual nodes, and the associated
socket emissions. Structurally/behaviorally identical to the logic
currently still living in ZwaveClient.ts - ZwaveClient is wired to
delegate to this service in a later commit.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Introduces AssociationService, a strict-clean class that owns all
device-native (Association CC) association flows: reading/refreshing a
node's association groups, checking/adding/removing single or bulk
associations, removing all associations from a group, and removing a
node from every other node's associations (used on node removal).
Preserves the endpoint/lifeline/multi-channel-capability handling,
driver-null guards, and error logging exactly as ZwaveClient's original
implementation. ZwaveClient is wired to delegate to this service in a
later commit.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…tion services

Replaces every scene, group, and association method body in
ZwaveClient with a thin delegate to _sceneService/_groupService/
_associationService, constructed lazily in init() so ports always
resolve the current driver/controller/nodes/value registry/socket/
store across a restart. Every public and underscore-prefixed method
keeps its exact existing signature and position in the 110-entry
allowedApis list - this is a pure internal refactor with no wire/API/
socket/MQTT/HA/persistence behavior changes.

Removes now-unused imports (AssociationGroup, AssociationCheckResult)
and now-duplicated module-level constants (GROUP_NAME_MAX_LENGTH,
GROUP_ID_MIN) that moved into GroupService.

ZwaveClient.ts: 8185 -> 7605 lines (-580).

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…tions

Adds deterministic, isolated-fake-port unit tests for SceneService (22
tests), GroupService (35 tests), and AssociationService (27 tests),
covering success paths, error/edge paths, stale-driver/no-driver
guards, persistence rollback on failure, virtual node materialization/
reuse/teardown, throttling, and concurrency-sensitive scenarios (stale
scene/group replacement mid-flight). All three exceed the 90/85/90/90
statement/branch/function/line coverage bar.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…cade extraction

callApi.test.ts, inboundApis.test.ts, and outboundProducers.test.ts
seeded scene/group state by poking the now-removed zwave.scenes/
zwave.groups public fields directly. Since TypeScript doesn't
type-check test/ files against tsconfig's api/**-only include, these
silently compiled as no-op ad-hoc property assignments and stopped
seeding anything once SceneService/GroupService encapsulated that
state, without failing to compile.

Repoints the seeding at the private service instances' internal arrays
(zwave._sceneService._scenes / zwave._groupService._groups) instead of
the real _setScenes/_createGroup delegates, preserving the original
in-memory-only, disk-independent seeding semantics each test relied on
(using the real persisting delegates would leak seeded state across
tests sharing the same isolated on-disk store).

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Several test files imported store-dependent modules (jsonStore,
Gateway, MqttDiscoveryManager, mqttScopedSubscription, configuration
template routes/services) via static top-of-file imports, which
resolve and start reading module-level state before per-test
STORE_DIR/env isolation is established. Under parallel or shuffled
test runs, this raced against the real repo store/ directory and
could read/write actual on-disk artifacts.

Convert the risky static imports to type-only imports plus dynamic
import() calls inside beforeAll, strictly after ensureTestEnv() (or
an equivalent harness that calls it) has redirected STORE_DIR/env for
that file. Confirmed via repeated shuffled-seed runs that no repo
store artifacts are generated and behavior is unchanged.

Files:
- test/lib/Constants.test.ts
- test/lib/Gateway.test.ts
- test/lib/hass/MqttDiscoveryManager.test.ts
- test/lib/hass/mqttScopedSubscription.test.ts
- test/lib/http/configurationTemplates.test.ts
- test/lib/jsonStore.test.ts
- test/lib/zwave/ConfigurationTemplateService.test.ts

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Found while proving the full backend suite is order-independent across
shuffled seeds (see logger order-independence work):

- settingsConstructorBoundary.test.ts: the mocked ZnifferManager was
  missing a close() method. POST /api/restart awaits
  oldZniffer.close() on whatever zniffer a prior test in this shared
  harness left attached, so whichever test happened to run before
  another restart call - order dependent under shuffle - threw
  'oldZniffer.close is not a function'. Add the missing close() mock.

- auth.test.ts: 'fails with a generic error when there is no
  logged-in session user' relied on the route's
  users.find(u => u.username === user.username) throwing on the
  missing session user, but Array.prototype.find never invokes its
  predicate for an empty users array - so under default declaration
  order (an earlier sibling describe happens to seed a user first)
  the test passed for an accidental reason. Under shuffle this test
  can run before any user is ever seeded, short-circuiting to the
  same observable failure for the WRONG reason. Seed an unrelated user
  directly so the real precondition holds regardless of run order.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…endence

module()/customTransports() memoize the transports list and register
every named logger in the module-level logContainer singleton -
correct for production (a single process never needs to un-create a
logger) but this leaked across describe/it blocks that exercise
several module names/configs in the same test process, making
logger.test.ts order-dependent. Under --sequence.shuffle seed 4732
this manifested as an assertion failure on a test that expected
default logging to be enabled, because an earlier block's config had
already mutated the shared/cached logger state.

Extract closeCachedTransports() out of setupAll() and add a
production-inert __testHooks.reset() seam (closes cached transports,
stops the clean job, and closes every registered winston logger via
logContainer.close()) so tests can reset to a clean slate between
suites. Nothing in the production entrypoint calls __testHooks.

Rewrite test/lib/logger.test.ts to call __testHooks.reset() in
beforeEach/afterEach. Reproduced the pre-fix seed-4732 failure via
git stash before implementing the fix, and confirmed seed 4732 plus
10+ other shuffled seeds now pass consistently.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…ciationService

Several methods cached this._driver.getDriver() into a local BEFORE an
await (CC-value refresh, a preceding add/remove call, a loop
iteration), then kept using that captured reference afterwards. If the
driver is torn down and replaced by a restart while the method is
suspended, the stale reference would silently keep operating against a
destroyed driver instead of the new one.

Add a private _requireDriver() helper and call it fresh at every point
the original code re-read this._driver, including immediately after
every await boundary (refreshCCValues, a prior addAssociations, each
association-array loop iteration) across all 7 public methods:
getGroups, getAssociations, checkAssociation, addAssociations,
removeAssociations, removeAllAssociations,
removeNodeFromAllAssociations.

Extend AssociationService.test.ts (24 -> 35 tests) with mutable-port
deterministic deferred-driver-swap regressions for get/add/removeAll,
asserting the exact old-driver-then-new-driver call sequence.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…g to GroupService

GroupService received ZwaveClient's _virtualNodes Map by reference at
construction time. ZwaveClient.init() replaces _virtualNodes with a
brand-new Map on every restart, so a GroupService instance built
before a restart kept reading/writing an orphaned, abandoned Map
instead of the one the rest of ZwaveClient (and the next GroupService
generation) actually uses.

Replace the direct Map reference with a closure-backed
groupVirtualNodeRegistryPort (has/get/set/delete) in ZwaveClient.ts
that resolves this._virtualNodes fresh on every call, matching the
pattern already used for the other Group*Port objects.

Also add GroupServiceGeneration, a cancellation-token class:
ZwaveClient.init() cancels the previous generation and mints a new one
before constructing the next GroupService. createGroup/updateGroup/
deleteGroup/removeNodeFromGroups check the generation immediately
after their persistence await and abort further virtual-node mutation
if it was cancelled while suspended - so an in-flight call left over
from a pre-restart GroupService instance can no longer mix its
bookkeeping with the new generation's driver/node state.

Extend GroupService.test.ts (35 -> 40 tests) with deferred-persistence
generation-fencing regressions for restart-during-create/update/delete
and current-registry-restoration assertions.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…acade

ScenePersistencePort.put() and SceneService's addSceneValue()/
removeSceneValue() were typed to return Promise<unknown>, discarding
the actual Promise<ZUISceneRecord<V>[]> shape produced by every code
path (jsonStore.put<T>() resolves to the T it was given, and both
methods only ever return that or throw). ZwaveClient's
_addSceneValue()/_removeSceneValue() facade methods pick up the
correct Promise<ZUIScene[]> return type now that the underlying chain
is exact end-to-end, matching their doc comments and their callers'
expectations.

Add expectTypeOf compile-time assertions plus runtime result-shape
checks to SceneService.test.ts proving the return type is exactly
ZUISceneRecord<V>[], not a wider/unknown type.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…age floor

Adds zwaveClientServiceWiring.test.ts (20 tests): production-adapter
integration coverage for the ZwaveClient wiring touched by this PR's
review findings - association delegate methods, group create/update/
delete/get through the real facade, buildVirtualValueId, startup group
restoration, node-removal group-index delegation, and group
virtual-node socket projection using valid real VirtualValueID shapes.
Exercises the actual ZwaveClient class and production ports/adapters
end-to-end rather than the extracted services in isolation (already
covered by their own dedicated unit-test files) - deliberately not a
broad api/** coverage pool.

Add an exact-file 'api/lib/ZwaveClient.ts' coverage threshold entry to
BOTH vitest.config.server.ts and vitest.config.ts, set to the measured
baseline with this file's 20 tests (22.81/17.02/31.14/22.91 statements/
branches/functions/lines) as a non-regression floor - separate from,
and below, the 90/85/90/90 bar the fully-extracted services keep.
Empirically proved the gate actually fails by temporarily raising the
thresholds above the measured baseline (confirmed npm run
coverage:server fails with explicit per-dimension errors), then
restored the passing baseline values.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Layer 10 cross-stack comment audit for the scene/group/association
refactor (PR #4732). Removes decorative section dividers, restated
JSDoc that duplicates adjacent code, and history/changelog framing
(finding numbers, 'used to be', 'original ZwaveClient did X').

Preserves and in several spots restores non-obvious domain facts:
generation-fencing/cancellation semantics, stale-driver-across-await
re-resolution, virtual-node lifecycle ownership, deep-equality
comparison rationale for object-valued CCs, and the still-unresolved
Delegates to X comments trimmed to their misleading-signature facts
(getGroups' void-return-but-mutates-in-place quirk) while pure
one-line-body restatements are removed.

Two comments on ports.ts (hasPhysicalNode, getZWaveNode) are corrected
for factual accuracy against their actual ZwaveClient.ts implementations.

No runtime/logic changes; comment-only.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Layer 10 cross-stack comment audit, test side. Removes decorative
section dividers, restated JSDoc/inline comments that duplicate the
assertions directly below them, and history framing ('Finding #3',
'mirrors the assertion above'). Condenses repeated store-isolation
rationale (dynamic import after ensureTestEnv(), since production
modules read STORE_DIR at module-evaluation time) to one sentence per
call site while keeping each file's unique nuance (Gateway.ts's
fs.watch(), MqttDiscoveryManager's CustomDeviceRegistry constructor-arg
asymmetry, jsonStore/Constants' import-alone-triggers-the-write case).

Removes 2 comments in callApi.test.ts that stated a case the code
doesn't handle ('scenes default to [] - no seeding needed'), which is
a documented CLAUDE.md style violation, not a lossy trim.

zwaveClientServiceWiring.test.ts's 82-line file header is condensed to
~22 lines: most of the removed per-section rationale already lives
unchanged on that section's own describe-block title (verified via
diff), and the store-isolation mechanics are delegated to
outboundProducers.test.ts's own doc comment, which already covers them
in full.

No test subjects any harness/fake/fixture/builder/cleanup-helper
mechanics in isolation; no test removals. Every changed/added test
still exercises production entry points (SceneService, AssociationService,
GroupService, ZwaveClient facades). All 1089 server tests / 1141
combined tests pass unchanged.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
An independent code-review pass flagged two precision issues in the
comment audit (both informational, not semantic loss):

- GroupService.ts's _virtualNodes field comment established that
  broadcast virtual-node lifecycle belongs to ZwaveClient, but didn't
  name where - add a pointer to _updateBroadcastNodeValues() so the
  fact is locally discoverable without cross-file searching.
- zwaveClientServiceWiring.test.ts's file header described the
  nodeId -> groupIds index as 'only rebuilt from _groups at
  construction', which reads as though construction were the index's
  only rebuild trigger. It's actually rebuilt after every mutating
  call too; reworded to state precisely why the node-removal test
  seeds via disk (a direct post-construction _groups overwrite leaves
  the index stale until the next mutation), without the ambiguity.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
A second independent code-review pass caught a factual regression the
audit itself introduced: Constants.test.ts, Gateway.test.ts,
jsonStore.test.ts, and logger.test.ts originally correctly said their
production file 'statically imports storeDir/logsDir from config/app.ts'
(each does - verified via direct grep of their import statements), but
condensation had swapped this to 'transitively imports' across all four,
apparently applied as a uniform substitution without re-checking each
file's actual import graph. Revert to 'statically' in all four.

logger.test.ts also had two more issues from the same review pass:

- Its second paragraph originally described two distinct module-level
  singletons (the transports-list array and the winston.Container), but
  the condensed wording read as if the transports list lived inside the
  Container. Restore the distinguishing 'and' clause; verified via
  logger.ts that transportsList (a standalone array) and logContainer
  (new winston.Container()) are in fact separate structures.
- Restore that setupCleanJob()'s un-awaited clean() promise makes its
  own logger.info(...) calls through this same file's shared transports
  cache - a non-obvious link between the rotation-cleanup background
  work and the exact cache this file's tests reset, dropped as part of
  the DISABLE_LOG_ROTATION paragraph's condensation.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
AlCalzone and others added 12 commits July 17, 2026 12:18
…tion

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…file

The write-order comment restored in 3d35ef3 was deliberately removed by
base's own comment audit (ce123bc, 87220f8, 8af7ee9), which trimmed
this shared file to a single non-obvious comment. Re-adding it broke
consistency with 5 sibling toHaveBeenNthCalledWith blocks in the same
file that never had such a comment. Base owns this file's content.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…le reset

Removes the __testHooks.reset() export added to api/lib/logger.ts
during review fixes. It existed solely so tests could clear the
module-level winston.Container/transports-list/cleanJob singletons
between cases - a production-only test seam per PR #4723's accepted
design feedback.

logger.ts's singletons are genuine process-lifetime state (issue
refactor, so instead test/lib/logger.test.ts now calls vi.resetModules()
per describe block and re-imports logger.ts (and winston, so
instanceof checks compare classes from the same generation) fresh,
giving each block its own isolated module instance instead of
resetting shared state through a productio-only API.

Verified order-independent: full file passes in default order and
under --sequence.shuffle with seed 4732 plus 6 other random seeds.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Rewrite zwaveClientServiceWiring.test.ts per PR #4723's accepted review
rules, replacing delegation/identity-mechanics tests with tests that
exercise the real, unmocked AssociationService/GroupService through
ZwaveClient's real public facade:

- Association tests: replace 6 tests asserting
  _associationService.X spy calls (toHaveBeenCalledWith) with 7 tests
  that drive a stateful fake controller association table and assert
  the real observable association state returned by
  getAssociations()/checkAssociation()/addAssociations()/
  removeAssociations()/removeAllAssociations()/
  removeNodeFromAllAssociations() - including a genuine
  Forbidden_SelfAssociation branch, not a canned OK mock.
- Group CRUD facade: remove a duplicate _getGroups() 'same array
  reference' identity test already covered by GroupService.test.ts.
- _buildVirtualValueId tests: rewrite 3 of 5 through _createGroup()/
  _updateGroup() using a fake driver's real getDefinedValueIDs(),
  asserting the real emitted ZUIValueId fields instead of calling the
  private method directly. The remaining 'existing field preservation
  merge' test is removed: both real call sites
  (GroupService._updateVirtualNodeValues,
  ZwaveClient._doUpdateBroadcastNodeValues) reset the target node's
  values to {} immediately before the rebuild loop reads 'existing'
  from that same map, so the merge branch is unreachable via any real
  path today - confirmed by the rewritten test failing until removed.
  Documented in a code comment rather than silently dropped.
- Node removal: remove the removeNodeFromGroupsSpy
  toHaveBeenCalledOnce/toHaveBeenCalledWith assertions; keep the real,
  observable in-memory and on-disk group/node state assertions.
- Socket projection: replace a double (valueId as any).id /
  (node as any).id cast with full type inference through
  TypedEventEmitter.once()'s generic signature, typing only the
  Promise's own resolved shape.

No production code changed. Test count: 20 -> 18 (net -2, reconciled
above). Remaining private-field (as any) pokes for driver/service
fixture setup match the established, inherited, pre-Layer-10
convention already used in callApi.test.ts/outboundProducers.test.ts/
inboundApis.test.ts/hass/server.test.ts.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…test

Per PR #4723's accepted review rules, audited AssociationService.test.ts/
GroupService.test.ts/SceneService.test.ts for delegation-mechanics framing
and raw-crash-text locking:

- GroupService.test.ts: 3 rollback/propagation tests asserted the exact
  text of an arbitrary test-fixture error ('persist failed' from
  createPersistencePort()'s fake, 'rejected node set' from an inline
  driver.getMulticastGroup override) via .rejects.toThrow('<text>'). The
  real, semantic contract under test is that a downstream failure
  propagates and rolls back in-memory state - already covered by the
  surrounding assertions - not the incidental message text a test author
  happened to pick. Loosened to argument-less .rejects.toThrow().
- AssociationService.test.ts: retitled 'delegates to
  driver.controller.checkAssociation' to 'returns the real driver check
  result for the given source/group/association' - the assertions
  already tested real observable behavior (the real return value plus
  the real external-driver-boundary call), only the title's framing
  language was mechanics-flavored.

No other changes: all other toThrow(...)/toHaveBeenCalledWith(...) text
in these 3 files was verified against the real production throw sites
(AssociationService.ts/GroupService.ts/SceneService.ts) and matches
deliberate, user-facing semantic error messages, or asserts the
external zwave-js driver boundary (the correct level for an
adapter/wrapper service with no other observable side effect) - left
unchanged. Driver/ZWaveNode/VirtualNode "as unknown as X" casts audited
and confirmed structurally required by TypeScript when faking large
third-party zwave-js SDK interfaces with minimal objects (empirically
confirmed narrower alternatives still require the same "unknown" hop,
and the established ScheduleService.test.ts precedent's alternative
pattern actually produces latent, currently-ungated tsc diagnostics
that this pattern avoids) - left unchanged. GroupServiceGeneration
fencing tests already exercise real restart/persistence/notification
invariants through public behavior, never asserting ".cancelled"
directly - left unchanged. No production code changed.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…tles

Two describe-block titles carried forward an internal development-time
"finding #N" bookkeeping label with no meaning outside that process:

- SceneService.test.ts: 'facade return type precision (Finding #3)' ->
  'facade return type precision'.
- zwaveClientServiceWiring.test.ts: 'group CRUD facade (...) - real
  GroupService + jsonStore wiring (the closure-backed registry finding
  #2 fixed)' -> '... wiring, with _getGroups() reading the live
  registry after each mutation' - keeps the real, still-useful
  behavioral fact the title was gesturing at (each test in the block
  reads back via _getGroups()/disk after a mutation, so it can't pass
  against a stale registry snapshot) without the process reference.

No assertions changed. Confirmed via a full diff of this PR's Layer 10
delta against its pre-Layer-10 base that these were the only two such
references anywhere in the delta.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Use structural upstream handles in the extracted-service ports so tests can exercise public service behavior without nominal external-class casts. Remove accessor, identity, type-only, and collaborator-mechanics assertions while retaining restart, persistence, projection, and association behavior coverage.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Delete the integration suite that drove private service and driver state. Preserve the observable scene and group facade compatibility checks through public underscore APIs, isolated persistence, and socket payloads.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…th session

req.session.user can be undefined with auth disabled and no prior login;
isAuthenticated still calls next() in that case. The unguarded lookup let
.find()'s predicate dereference user.username on undefined once users.json
was non-empty, throwing instead of falling through to the existing
"User not found" response. This mirrors a fix already established and
tested elsewhere in this stack's history (1c23c2f, predating this
rewritten base) that the base's own comment-only-labeled pass silently
undid.

Also drops a stale seedUser() call and its pre-guard-era comment from the
corresponding test: an earlier conflict resolution in this rewrite grafted
6100f49's shuffle-order workaround (written against an unguarded
predecessor of this handler) onto this already-guarded test, where the
ternary short-circuits before .find() ever runs and the seed is moot.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…f Layer 10's accepted delta

api/routes/auth.ts aside, these were the only 2 files where this rebase's
cumulative diff against the new base didn't match the original PR's
cumulative diff against its own base (verified file-by-file across the
full touched set).

Root cause: the replayed range includes non-merge commits from a
'merge updated layer 9 base' step whose own merge commit is (correctly)
excluded from a non -p rebase. One of those commits' store-isolation fix
touched MqttDiscoveryManager.test.ts and mqttScopedSubscription.test.ts,
but the original merge's resolution discarded that change for these two
Layer9-owned files, keeping the newer parent's version outright - a
choice this base's own tip already matches. Replaying the commit's raw
diff without that merge's resolution silently reintroduced the discarded
change. Confirmed by diffing this PR's original base against its own
final head: both files are byte-identical there, across all 29 original
commits.

Restores both files to this base's content, matching the original PR's
true net effect of zero change to either file.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…suites

test/lib/Constants.test.ts, test/lib/jsonStore.test.ts, and
test/lib/logger.test.ts each imported ensureTestEnv/cleanupTestEnv from a
'./http/env.ts' that has never existed on this branch; the real helper
lives at './shared/env.ts'. All 3 files failed at import time with zero
tests running, surfaced only by actually executing a shuffled-seed run
rather than by diff-size comparison (a same-size, wrong-target import
edit doesn't show up as a line-count anomaly). Traced to 2 of this
rebase's own earlier conflict-resolution commits.

Constants.test.ts and jsonStore.test.ts have byte-identical blobs at both
the old base and this new base, so no adaptation is legitimate for either
- both are now byte-identical to the original PR's final content.
logger.test.ts's base genuinely evolved independently between the two
branches, so only its broken env.ts references were corrected in place,
preserving its otherwise-legitimate freshLogger() adaptation.

test/lib test/runtime: 50/50 files, 842/842 tests passing after this fix.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Remove the out-of-scope HTTP fix added during Layer 10 replay.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@AlCalzone
AlCalzone force-pushed the alcalzone-backend-refactor-scenes-groups-associati branch from ed70f11 to de200c3 Compare July 17, 2026 10:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants