Skip to content

fix(node): resolve id-keyed sibling values during client constraint validation#3676

Merged
Apollon77 merged 1 commit intomainfrom
fix/clientnode-constraint-sibling-resolution
Apr 30, 2026
Merged

fix(node): resolve id-keyed sibling values during client constraint validation#3676
Apollon77 merged 1 commit intomainfrom
fix/clientnode-constraint-sibling-resolution

Conversation

@Apollon77
Copy link
Copy Markdown
Collaborator

@Apollon77 Apollon77 commented Apr 30, 2026

Related to matter-js/matterjs-server#589

Summary

  • ClientNode mirrors rejected valid attribute writes whose constraint referenced a sibling attribute (e.g. BooleanStateConfiguration.currentSensitivityLevel with constraint max supportedSensitivityLevels - 1). The validator's NameResolver read the stale name-keyed default (0) instead of the subscription-delivered id-keyed value (e.g. 3), so value 2 failed against max = -1 with Constraint "all": Value 2 is not within bounds defined by constraint.
  • Two compensating fixes: Datasource.RootReference.change() clone now also copies id-keyed entries that #fields (property-names-only) would otherwise drop; NameResolver.createDirectResolver now prefers struct[id] when present and falls back to struct[name].

Why this happens

ClientBehaviorBacking sets primaryKey: "id" so subscription deliveries land at numeric attribute ids while initial defaults from the schema sit under property names. The two coexist in the same struct with conflicting values — id-keyed is "live", name-keyed is stale. The constraint validator's sibling lookup hit the stale slot.

A deeper fix (consolidate to single key per attribute) is tracked separately as a refactor; see Notes below.

Tests

  • packages/node/test/behavior/state/validation/constraintTest.ts — 4 new validator-layer cases under max with reference (sibling keyed by id) covering id-only sibling, accept and reject paths, and id-key preference over a stale name-key default. These fail without the NameResolver change (verified by temporary revert).
  • packages/node/test/node/ClientNodeTest.ts — 3 new full-stack cases under writes attribute whose constraint references a sibling attribute covering all three client write APIs: agent.state assignment, setStateOf, and endpoint.set.

Notes

The two fixes patch consequences of the dual-keying scheme rather than the scheme itself. A follow-up refactor to normalize ClientNode mirror storage to a single key per attribute is captured locally; once that lands, both compensations in this PR can be reverted. Filed as a separate task because the blast radius (name-keyed reads in ValueValidator.createStructValidator, StructManager getter fallback, integrateExternalChange) warrants its own PR.

🤖 Generated with Claude Code

…alidation

ClientNode mirrors store subscription-delivered values under numeric attribute
ids (primaryKey: "id") while initial defaults sit under property names.  When a
constraint referenced a sibling attribute (e.g. BooleanStateConfiguration's
currentSensitivityLevel constraint "max supportedSensitivityLevels - 1"), the
validator's NameResolver read the stale name-keyed default instead of the live
id-keyed value, rejecting otherwise valid writes.

- Datasource clone now also copies id-keyed entries (previously dropped because
  `#fields` only enumerates property names).
- NameResolver.createDirectResolver prefers `struct[id]` when present and falls
  back to `struct[name]` for name-keyed datasources.

Adds focused validator-layer tests for id-keyed siblings and full-stack
ClientNode tests covering all three client-write paths (agent.state assignment,
setStateOf, endpoint.set).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings April 30, 2026 08:32
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes client-side constraint validation when a constraint references a sibling attribute that is stored id-keyed (live subscription state) while a stale name-keyed default also exists in the same struct. This aligns validator sibling lookups with how ClientBehaviorBacking stores mirrored state (primaryKey: "id").

Changes:

  • Update NameResolver to prefer struct[attributeId] when resolving by property name, falling back to struct[propertyName].
  • Update Datasource.RootReference.change() cloning to retain id-keyed entries that would otherwise be dropped when cloning from #fields (property-name-only).
  • Add validator-layer and full-stack ClientNode tests covering sibling-reference constraints with id-keyed sibling values.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.

Show a summary per file
File Description
packages/node/src/behavior/state/managed/NameResolver.ts Prefer id-keyed struct entries (when available) for name-based resolution to avoid reading stale name-keyed defaults.
packages/node/src/behavior/state/managed/Datasource.ts Preserve id-keyed entries when cloning root values during change(), preventing loss of live mirror state across transactional clones.
packages/node/test/behavior/state/validation/validation-test-utils.ts Extend Fields() helper to allow specifying id for test schemas.
packages/node/test/behavior/state/validation/constraintTest.ts Add constraint validation cases proving id-keyed sibling lookup works and is preferred over stale name-key defaults.
packages/node/test/node/ClientNodeTest.ts Add end-to-end tests ensuring all client write paths succeed when constraints reference id-keyed sibling attributes.

@Apollon77 Apollon77 merged commit 11b356f into main Apr 30, 2026
41 checks passed
@Apollon77 Apollon77 deleted the fix/clientnode-constraint-sibling-resolution branch April 30, 2026 08:57
Apollon77 added a commit that referenced this pull request May 1, 2026
…cache (#3678)

* fix(general): surface original commit-phase-2 errors to transaction caller

Tx#executeCommit2 used to wrap any phase-2 participant failure in a
generic FinalizationError carrying only the participant name. The
original error (e.g. a StatusResponseError returned by a remote device
declining a write) was logged but never reached the caller, so
ClientNode setStateOf / agent.state / endpoint.set callers had no way
to detect the device's status code.

throwIfErrored now rethrows the original error when a single participant
fails, and aggregates with MatterAggregateError when multiple do. The
rollback path's mislabeled "in commit phase 2" message is corrected.

Adds direct unit tests for the new contract (TransactionTest) and
end-to-end ClientNode tests covering single rejection, partial multi-
attribute rejection, and full multi-attribute rejection across two
clusters, plus a server-side regression test for the preCommit path.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* feat(node): roll back local client cache on declined remote writes

After PR #3676 the original StatusResponseError reaches the caller of
ClientNode setStateOf / agent.state / endpoint.set, but the local
client-side cache still held the value the user attempted to write.
Subscriptions only deliver server-side changes, so the divergence
persisted until restart.

RemoteWriteParticipant now tracks a per-cluster snapshot
(previousValues + writtenValues + compensator) per set() call. On
commit2 failure the new RemoteWriter onFailure callback delivers
per-attribute statuses; the participant groups failures by
endpoint+cluster and invokes the compensator with the failed ids.

DatasourceCache implements the compensator: it restores the pre-write
value via the existing externalSet path (so persistence + listeners
fire as if a subscription update arrived) but only when:
  - a baseline previous value was captured for the key, AND
  - the current local value still equals what was just written
    (skip restoration if a concurrent subscription update changed it).

Adds three end-to-end tests covering single decline, partial decline
across two clusters, and full decline.  The tests fail when the
compensate body is disabled, so they verify compensation rather than
relying on subscription delivery.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* refactor(node): use reference identity for write-decline compensation guard

Datasource keeps user-written values by reference and integrateExternalChange
replaces a reference only when a subscription actually delivers a changed
value (its own no-op deep-equal filter strips identical re-deliveries).  The
guard in DatasourceCache.compensate therefore only needs to check `current[id]
!== writtenValues[id]` instead of running a full deep-equal on every key — the
ref check captures real concurrent updates without the extra walk.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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