Conversation
…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>
Contributor
There was a problem hiding this comment.
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
NameResolverto preferstruct[attributeId]when resolving by property name, falling back tostruct[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
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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Related to matter-js/matterjs-server#589
Summary
BooleanStateConfiguration.currentSensitivityLevelwith constraintmax supportedSensitivityLevels - 1). The validator'sNameResolverread the stale name-keyed default (0) instead of the subscription-delivered id-keyed value (e.g. 3), so value 2 failed againstmax = -1withConstraint "all": Value 2 is not within bounds defined by constraint.Datasource.RootReference.change()clone now also copies id-keyed entries that#fields(property-names-only) would otherwise drop;NameResolver.createDirectResolvernow prefersstruct[id]when present and falls back tostruct[name].Why this happens
ClientBehaviorBackingsetsprimaryKey: "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
Notesbelow.Tests
packages/node/test/behavior/state/validation/constraintTest.ts— 4 new validator-layer cases undermax 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 theNameResolverchange (verified by temporary revert).packages/node/test/node/ClientNodeTest.ts— 3 new full-stack cases underwrites attribute whose constraint references a sibling attributecovering all three client write APIs:agent.stateassignment,setStateOf, andendpoint.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,StructManagergetter fallback,integrateExternalChange) warrants its own PR.🤖 Generated with Claude Code