fix(node): default server exports no longer inherit base implementation features - #4159
Open
Apollon77 wants to merge 5 commits into
Open
fix(node): default server exports no longer inherit base implementation features#4159Apollon77 wants to merge 5 commits into
Apollon77 wants to merge 5 commits into
Conversation
…on features A cluster's own feature table must not enable features. Only a device type requirement or an explicit implementation decision may do so. The spec's feature-table fallback column was ingested as the model's "supported" encoding, and `ClusterBehavior.for()` silently stopped reverting feature selection when cluster namespaces lost `supportedFeatures`, so eleven default server exports published the features their base implementation enables internally. - codegen no longer maps the feature fallback column to a feature default, and the stale defaults are removed from Groups and LevelControl - default server exports use `.with()` as the single feature-drop mechanism - `FeatureSelectionErrors()` assesses a selection against FeatureMap conformance; adding a server cluster to an endpoint fails when it does not conform - `EnergyEvseServer` selects its unconditionally mandatory ChargingPreferences feature, and a LongIdleTimeSupport ICD must also select CheckInProtocolSupport and UserActiveModeTrigger - `ClusterBehavior.with()` rejects a feature the cluster does not define - the behavior cache keys on the namespace, feature variants converge on the schema they derive from, and the deprecated `Cluster.with()` shim memoizes Fixes #4145 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes feature-selection leakage from default server exports (notably LevelControlServer) by ensuring cluster features are never implicitly enabled by a cluster’s own feature table, and by restoring effective feature-reset behavior in behavior exports. It also adds validation and tests to prevent regressions across codegen, model, and node behavior layers.
Changes:
- Codegen/model: stop mapping spec “fallback” feature values into model defaults; add
FeatureSelectionErrors()and assert standard clusters enable no features by default. - Node behaviors: make default
XxxServerexports featureless via.with(); enforce invalid feature names and validate feature-map conformance at endpoint construction. - Caching/tests: strengthen namespace/schema caching behavior and add broad sweeps to ensure only allowlisted server exports enable features unconditionally.
Reviewed changes
Copilot reviewed 40 out of 40 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| support/codegen/src/mom/spec/translate-cluster.ts | Removes spec fallback/default mapping for features during cluster translation. |
| packages/types/test/cluster/ClusterTypeWithCompatTest.ts | Updates compat-shim expectations to “one clone per feature selection”. |
| packages/types/src/cluster/ClusterType.ts | Memoizes Cluster.with() clones per selection (compat shim). |
| packages/node/test/node/ServerSubscriptionTest.ts | Updates LIT ICD feature selection to include UserActiveModeTrigger. |
| packages/node/test/node/client/ClientNodePhysicalPropertiesIcdTest.ts | Updates ICD feature selection to include UserActiveModeTrigger. |
| packages/node/test/node/client/ClientNodeInteractionIcdTest.ts | Updates ICD feature selection to include UserActiveModeTrigger. |
| packages/node/test/devices/DeviceFeatureSelectionTest.ts | Adds sweep test asserting device default behaviors have conformant feature selections. |
| packages/node/test/behaviors/icd-management/IcdManagementServerTest.ts | Updates ICD tests for mandatory UserActiveModeTrigger under LITS/DSLS. |
| packages/node/test/behaviors/general-diagnostics/GeneralDiagnosticsFeaturesTest.ts | Adds test coverage for advisory log tied to DataModelTest + maxPathsPerInvoke. |
| packages/node/test/behavior/system/icd/litSupportedTest.ts | Updates LIT ICD selection to include UserActiveModeTrigger. |
| packages/node/test/behavior/system/icd/IcdClientTest.ts | Updates ICD feature selections to include UserActiveModeTrigger. |
| packages/node/test/behavior/cluster/ClusterBehaviorFeaturesTest.ts | Adds comprehensive tests for with()/for() feature semantics and export invariants. |
| packages/node/test/behavior/cluster/ClusterBehaviorCacheTest.ts | Expands cache tests to cover namespace-aware caching and schema/behavior sharing. |
| packages/node/src/behaviors/window-covering/WindowCoveringServer.ts | Switches default server export to .with() to drop base-enabled features. |
| packages/node/src/behaviors/thermostat/ThermostatServer.ts | Switches default server export to .with() to drop base-enabled features. |
| packages/node/src/behaviors/switch/SwitchServer.ts | Switches default server export to .with() to drop base-enabled features. |
| packages/node/src/behaviors/smoke-co-alarm/SmokeCoAlarmServer.ts | Switches default server export to .with() to drop base-enabled features. |
| packages/node/src/behaviors/service-area/ServiceAreaServer.ts | Switches default server export to .with() to drop base-enabled features. |
| packages/node/src/behaviors/power-topology/PowerTopologyServer.ts | Switches default server export to .with() to drop base-enabled features. |
| packages/node/src/behaviors/power-source/PowerSourceServer.ts | Switches default server export to .with() to drop base-enabled features. |
| packages/node/src/behaviors/on-off/OnOffServer.ts | Switches default server export to .with() to drop base-enabled features. |
| packages/node/src/behaviors/mode-select/ModeSelectServer.ts | Switches default server export to .with() to drop base-enabled features. |
| packages/node/src/behaviors/level-control/LevelControlServer.ts | Fixes LevelControlServer export to be featureless via .with(). |
| packages/node/src/behaviors/icd-management/IcdManagementServer.ts | Corrects LITS guidance to include mandatory UserActiveModeTrigger. |
| packages/node/src/behaviors/general-diagnostics/GeneralDiagnosticsServer.ts | Adds runtime advisory when DataModelTest is enabled but not required. |
| packages/node/src/behaviors/energy-evse/EnergyEvseServer.ts | Ensures mandatory ChargingPreferences feature is selected. |
| packages/node/src/behaviors/electrical-energy-measurement/ElectricalEnergyMeasurementServer.ts | Switches default server export to .with() to drop base-enabled features. |
| packages/node/src/behaviors/door-lock/DoorLockServer.ts | Switches default server export to .with() to drop base-enabled features. |
| packages/node/src/behaviors/color-control/ColorControlServer.ts | Switches default server export to .with() to drop base-enabled features. |
| packages/node/src/behavior/internal/ServerBehaviorBacking.ts | Adds endpoint-time feature selection validation (FeatureSelectionError). |
| packages/node/src/behavior/cluster/ClusterBehaviorType.ts | Fixes feature sync, improves schema caching, and rejects unknown feature selections. |
| packages/node/src/behavior/cluster/ClusterBehaviorCache.ts | Makes cache namespace-aware and honors client/server distinction. |
| packages/node/src/behavior/cluster/ClusterBehavior.ts | Clarifies for() feature-selection semantics in docs. |
| packages/model/test/standard/MatterDefinitionTest.ts | Adds assertion that standard clusters enable no features by default. |
| packages/model/test/logic/FeatureSelectionErrorsTest.ts | Adds unit tests for FeatureSelectionErrors() behavior. |
| packages/model/src/standard/elements/level-control.element.ts | Removes incorrect feature “default” values from LevelControl FeatureMap. |
| packages/model/src/standard/elements/groups.element.ts | Removes incorrect feature “default” value from Groups FeatureMap. |
| packages/model/src/logic/index.ts | Exports FeatureSelectionErrors() from model logic index. |
| packages/model/src/logic/FeatureSelectionErrors.ts | Implements feature selection conformance checking (choices, dependencies, disallow). |
| CHANGELOG.md | Documents behavior/model fixes and new validation. |
…lones A repeated feature produced a distinct cache key, so `Cluster.with(A, A)` and `Cluster.with(A)` yielded separate clones of identical content and split the behavior cache keyed on namespace identity. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
|
Tick the box to add this pull request to the merge queue (same as
|
The shim memoizes a clone per selection, so an unvalidated name grew the cache without bound and only surfaced when the endpoint was constructed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Selection now records against `Model.operationalIsSupported`, so a feature's `default` conveys only the specification's fallback value. The spec parser keeps parsing that column and the model keeps carrying it; it has no effect. A feature the specification mandates without condition is not an application choice, so server behaviors select it regardless. Client behaviors defer to the features their peer reports. - `FeatureSelectionErrors()` reports against `IllegalFeatureCombinations` so runtime cannot diverge from the assessment codegen uses, and surfaces through `ValidatedElements` memoized per schema rather than per behavior instance - `FeatureSet.resolve()` replaces four feature-name resolvers that accepted three different vocabularies - fix the OR case in `IllegalFeatureCombinations`, which emitted stringified keys and omitted its own feature, so `DoorLock.User` was never required - General Diagnostics asserts DataModelTest above a maxPathsPerInvoke of one, and its state getters no longer name their own type, which made any feature selection unconstructible Co-Authored-By: Claude Opus 5 (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.
Fixes #4145.
Problem
A cluster's own feature table was enabling features, and the mechanism that reverted a feature selection had silently stopped working.
Two independent causes:
Spec fallback ingested as "supported".
translate-cluster.tsmapped the spec feature table'sdef/fallbackcolumn into the model'sdefault, whichClusterModel.supportedFeaturesreads as "this feature is on". The spec lists| 0 | OO | OnOff | O | … | 1 |for LevelControl — that trailing1is a fallback, not a default selection.Groups.GNcarried a hand-set default from an old bulk commit.ClusterBehavior.for()stopped reverting features.syncFeatures()readnamespace.supportedFeatures, but since780d1d05b(2026-03-22, the ClusterNamespace codegen refactor) cluster namespaces no longer carry that property, so the check became a permanent no-op. ElevenXxxServer extends XxxBaseServer.for(Xxx)exports therefore published the features their base enables internally. First released in 0.17.0; 0.16.x is unaffected.The reporter's symptom:
LevelControlServerleakedLighting, so aPumpDeviceenforced the[LT]MinLevel 1–254 constraint and rejectedminLevel: 0.Rule
Enforced at three layers: codegen can't reintroduce the fallback;
MatterDefinitionTestasserts no standard cluster enables a feature by default; a sweep over all behavior exports asserts only nine allowlisted features are on.Changes
Model / codegen
groupsandlevel-controlFeatureSelectionErrors()assesses a selection against FeatureMap conformance — choice groups (O.a,.a+,.a-), mandatory-given-others, disallowed-given-othersBehaviors
.with()as the single feature-drop mechanismClusterBehavior.with()throws on a feature the cluster does not define instead of silently selecting nothingEnergyEvseServerselectsChargingPreferences— the only unconditionally mandatory feature in the model, and without itEnergyEvseDevicecould not be instantiated at allCheckInProtocolSupportandUserActiveModeTrigger(spec:UAT: LITS, O); the previous docstring recommended an invalid combinationValidation —
FeatureSelectionErrorat endpoint init. It cannot run at type-construction time: 177 behavior exports and 59 device-type requirement exports legitimately ship an incomplete selection because the application chooses.Caches —
ClusterBehaviorCachekeys on the namespace (two namespaces sharing a schema no longer collide) and honorsforClient; feature variants converge on the schema they derive from; the deprecatedCluster.with()shim memoizes; both schema caches areWeakMap.Behavior changes
.with(...). Their persisted cluster state resets once, as it is keyed by feature selection.ClusterTypeWithCompatTestpreviously asserted the shim returns a fresh clone per call; that freshness is what defeated the behavior cache, so the invariant is now one clone per selection.Verification
Clean build, full monorepo suite,
format-verifyandlintall green. +49 tests (model +22, node +26, types +1).Each production change was verified to be detected by its tests: reverting the feature work fails 5 tests, reverting the cache work fails 4.
Two rounds of independent adversarial review are folded in. The second round found the dead
EnergyEvseDevice, an unreachableLevelControl.Frequency(provisional features assess as inapplicable and were being rejected), and a cache regression introduced by the namespace key — all fixed, with a device-type sweep test that would have caught the first.🤖 Generated with Claude Code