NetworkCommissioning failure status (NetworkIDNotFound, AuthFailure, etc.) collapses to opaque generic NSError on Darwin, losing context for client UI#72272
Open
woody-apple wants to merge 1 commit into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request refactors the commissioning completion callback flow in MTRDeviceControllerDelegateBridge to defer delegate dispatching until OnCommissioningSuccess or OnCommissioningFailure is called, preventing cross-queue races and allowing the capture of NetworkCommissioningStatus. It also introduces MTRErrorNetworkCommissioningStatusKey to surface network commissioning errors. The review feedback highlights three key issues: a potential nil-pointer issue when copying userInfo if it is nil, an incorrect availability version typo (using version 26.0), and a restrictive selector check during PASE failures that could prevent dispatching to delegates implementing alternative selectors.
Comment on lines
+169
to
+173
| if (networkCommissioningStatus.HasValue()) { | ||
| NSMutableDictionary * combined = [userInfo mutableCopy]; | ||
| combined[MTRErrorNetworkCommissioningStatusKey] = @(chip::to_underlying(networkCommissioningStatus.Value())); | ||
| userInfo = combined; | ||
| } |
Contributor
There was a problem hiding this comment.
If userInfo is nil, calling [userInfo mutableCopy] will return nil. Consequently, combined will be nil, and the subscript assignment combined[MTRErrorNetworkCommissioningStatusKey] = ... will be a no-op. This results in userInfo remaining nil, and the MTRErrorNetworkCommissioningStatusKey will not be added to the error.
To prevent this, initialize combined to a new dictionary if userInfo is nil.
if (networkCommissioningStatus.HasValue()) {
NSMutableDictionary * combined = userInfo ? [userInfo mutableCopy] : [NSMutableDictionary dictionary];
combined[MTRErrorNetworkCommissioningStatusKey] = @(chip::to_underlying(networkCommissioningStatus.Value()));
userInfo = combined;
}
| * NetworkCommissioning response (so its presence is itself a signal that the | ||
| * commissionee actually reported a NetworkCommissioning status). | ||
| */ | ||
| MTR_EXTERN NSString * const MTRErrorNetworkCommissioningStatusKey MTR_AVAILABLE(ios(26.0), macos(26.0), watchos(26.0), tvos(26.0)); |
Contributor
There was a problem hiding this comment.
The availability version ios(26.0), macos(26.0), watchos(26.0), tvos(26.0) appears to be a typo. Using 26.0 will make this constant unavailable on all current platforms (since iOS is currently at version 18), causing compiler warnings or errors for any client attempting to use it.
It should be aligned with the availability of MTRErrorDomain, which is ios(16.1), macos(13.0), watchos(9.1), tvos(16.1).
MTR_EXTERN NSString * const MTRErrorNetworkCommissioningStatusKey MTR_AVAILABLE(ios(16.1), macos(13.0), watchos(9.1), tvos(16.1));
Contributor
Author
There was a problem hiding this comment.
(FYI: This is Claude) False positive. ios(26.0)/macos(26.0)/watchos(26.0)/tvos(26.0) is the correct, intentional annotation — Apple unified its OS version numbering to year-based for the 2025/26 cycle (iOS/iPadOS/macOS/watchOS/tvOS all at 26). It's not iOS 18-next. The same annotation is already in this repo on MTRDeviceControllerParameters.h:175 and on the sibling MTRInteractionErrorCodeAccessRestricted = 0x9d directly below in this same file (line 169), both shipped earlier. No code change needed.
Comment on lines
101
to
109
| if (status == chip::Controller::DevicePairingDelegate::Status::SecurePairingFailed && [strongDelegate respondsToSelector:@selector(controller:commissioningComplete:nodeID:metrics:)]) { | ||
| OnCommissioningComplete(mDeviceNodeId, CHIP_ERROR_TIMEOUT); | ||
| // OnCommissioningComplete now defers the delegate dispatch until a | ||
| // subsequent OnCommissioningSuccess/Failure callback. The PASE-fail | ||
| // path does not produce one upstream, so synthesize an empty | ||
| // CompletionStatus here to flush the pending record through to the | ||
| // delegate; otherwise PASE failures would never reach the client. | ||
| OnCommissioningFailure(chip::PeerId(), chip::Controller::CompletionStatus()); | ||
| } |
Contributor
There was a problem hiding this comment.
The check on line 101 only verifies if strongDelegate responds to @selector(controller:commissioningComplete:nodeID:metrics:). If a delegate only implements the 3-argument selector @selector(controller:commissioningComplete:nodeID:) or the deprecated 2-argument selector @selector(controller:commissioningComplete:), this block will be skipped, and the PASE failure will never be dispatched to the delegate.
Since DispatchPendingCommissioningComplete supports all three selectors, we should check for any of them here to ensure all delegate implementations receive the PASE failure callback. Additionally, please ensure a unit test covers this behavior to align with the platform-specific implementation requirements.
if (status == chip::Controller::DevicePairingDelegate::Status::SecurePairingFailed &&
([strongDelegate respondsToSelector:@selector(controller:commissioningComplete:nodeID:metrics:)] ||
[strongDelegate respondsToSelector:@selector(controller:commissioningComplete:nodeID:)] ||
[strongDelegate respondsToSelector:@selector(controller:commissioningComplete:)])) {
OnCommissioningComplete(mDeviceNodeId, CHIP_ERROR_TIMEOUT);
// OnCommissioningComplete now defers the delegate dispatch until a
// subsequent OnCommissioningSuccess/Failure callback. The PASE-fail
// path does not produce one upstream, so synthesize an empty
// CompletionStatus here to flush the pending record through to the
// delegate; otherwise PASE failures would never reach the client.
OnCommissioningFailure(chip::PeerId(), chip::Controller::CompletionStatus());
}
References
- When fixing a platform-specific implementation to align with an implicit generic API contract, add a comment explaining the rationale and ensure a unit test covers this behavior.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR improves Darwin commissioning error reporting by preserving device-reported NetworkCommissioning failure status in NSError.userInfo, allowing clients to distinguish failures like network not found or auth failure.
Changes:
- Adds
MTRErrorNetworkCommissioningStatusKeyand threads optional NetworkCommissioning status through internalMTRErrorcreation. - Updates
MTRDeviceControllerDelegateBridgeto defer commissioning-complete dispatch until success/failure context is available. - Renames and expands
MTRErrorMappingTeststo Obj-C++ with mapping and bridge-level regression tests.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
src/darwin/Framework/CHIP/MTRError.h |
Declares the new public userInfo key and documents its semantics. |
src/darwin/Framework/CHIP/MTRError.mm |
Adds optional NetworkCommissioning status plumbing into NSError creation. |
src/darwin/Framework/CHIP/MTRError_Internal.h |
Adds the internal NSError factory overload accepting NetworkCommissioning status. |
src/darwin/Framework/CHIP/MTRError_Test.h |
Adds a test-only helper for status-aware error creation. |
src/darwin/Framework/CHIP/MTRDeviceControllerDelegateBridge.h |
Adds success/failure callbacks and pending commissioning-complete state. |
src/darwin/Framework/CHIP/MTRDeviceControllerDelegateBridge.mm |
Captures completion status and dispatches delegate callbacks with preserved status. |
src/darwin/Framework/CHIPTests/MTRErrorMappingTests.mm |
Adds Obj-C++ tests for NSError mapping and bridge propagation behavior. |
src/darwin/Framework/CHIPTests/MTRErrorMappingTests.m |
Removes the old Obj-C test file after rename to .mm. |
src/darwin/Framework/Matter.xcodeproj/project.pbxproj |
Updates Xcode project references for the .m to .mm test rename. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+31
to
+32
| * NetworkCommissioningStatus. The value is an NSNumber wrapping an | ||
| * MTRNetworkCommissioningStatus byte (e.g. NetworkNotFound, AuthFailure). |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #72272 +/- ##
=======================================
Coverage 55.52% 55.52%
=======================================
Files 1630 1630
Lines 111127 111127
Branches 13418 13418
=======================================
Hits 61706 61706
Misses 49421 49421 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
PR #72272: Size comparison from adba29b to ddca587 Full report (35 builds for bl602, bl616, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, psoc6, qpg, realtek, stm32, telink)
|
woody-apple
force-pushed
the
dev/woody/174955538
branch
from
ddca587 to
7cecec7
Compare
|
PR #72272: Size comparison from 4894db9 to 7cecec7 Full report (21 builds for bl602, bl616, bl702, bl702l, cc13x4_26x4, cc32xx, nrfconnect, psoc6, qpg, realtek, stm32)
|
woody-apple
force-pushed
the
dev/woody/174955538
branch
from
7cecec7 to
62aafbb
Compare
Comment on lines
+205
to
+212
| // Record the (nodeId, error) tuple; defer the dispatch_async until the | ||
| // subsequent OnCommissioningSuccess / OnCommissioningFailure call so that | ||
| // the NetworkCommissioning status from CompletionStatus (only available on | ||
| // failure) can be captured by value into the block, eliminating the | ||
| // cross-queue read of a member-variable stash. | ||
| mPendingCommissioningCompleteNodeId = nodeId; | ||
| mPendingCommissioningCompleteError = error; | ||
| mHasPendingCommissioningComplete = true; |
Comment on lines
+31
to
+32
| * NetworkCommissioningStatus. The value is an NSNumber wrapping an | ||
| * MTRNetworkCommissioningStatus byte (e.g. NetworkNotFound, AuthFailure). |
woody-apple
force-pushed
the
dev/woody/174955538
branch
2 times, most recently
from
07c2713 to
c8ca4e0
Compare
github-actions
Bot
added
documentation
woody-apple
force-pushed
the
dev/woody/174955538
branch
from
cff08c2 to
9343509
Compare
woody-apple
force-pushed
the
dev/woody/174955538
branch
from
9343509 to
d917d45
Compare
woody-apple
force-pushed
the
dev/woody/174955538
branch
from
7851762 to
3e23187
Compare
|
PR #72272: Size comparison from 8a162c6 to 3e23187 Full report (35 builds for bl602, bl616, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, psoc6, qpg, realtek, stm32, telink)
|
woody-apple
force-pushed
the
dev/woody/174955538
branch
from
a9fb5d2 to
723b490
Compare
|
PR #72272: Size comparison from 8a162c6 to 723b490 Full report (32 builds for bl602, bl616, bl702, bl702l, cc13x4_26x4, cc32xx, esp32, nrfconnect, psoc6, qpg, realtek, stm32, telink)
|
woody-apple
force-pushed
the
dev/woody/174955538
branch
from
723b490 to
3aa1d69
Compare
|
PR #72272: Size comparison from 8a162c6 to 3aa1d69 Full report (35 builds for bl602, bl616, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, psoc6, qpg, realtek, stm32, telink)
|
woody-apple
force-pushed
the
dev/woody/174955538
branch
from
3aa1d69 to
f3d85dc
Compare
|
PR #72272: Size comparison from 8a162c6 to f43694d Full report (35 builds for bl602, bl616, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, psoc6, qpg, realtek, stm32, telink)
|
woody-apple
force-pushed
the
dev/woody/174955538
branch
2 times, most recently
from
2ba9db0 to
cf5bf8a
Compare
|
PR #72272: Size comparison from 8a162c6 to cf5bf8a Full report (6 builds for cc32xx, nrfconnect, realtek, stm32)
|
woody-apple
force-pushed
the
dev/woody/174955538
branch
from
cf5bf8a to
01b3a4d
Compare
|
PR #72272: Size comparison from 8a162c6 to 01b3a4d Full report (35 builds for bl602, bl616, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, psoc6, qpg, realtek, stm32, telink)
|
woody-apple
force-pushed
the
dev/woody/174955538
branch
from
01b3a4d to
ad3aef0
Compare
…commissioning failure When commissioning fails because the commissionee returned a NetworkCommissioning cluster ConnectNetworkResponse / NetworkConfigResponse with a non-success status (e.g. NetworkNotFound, AuthFailure, NetworkIDNotFound), the resulting NSError delivered to the MTRDeviceControllerDelegate is collapsed to a generic MTRErrorDomain Code=1 "General error: 172" -- the specific spec-level NetworkCommissioning status that came back from the device is dropped before clients can render an actionable user-facing message (for example "the accessory could not find the Wi-Fi network -- try a 2.4 GHz network or check the SSID and password"). Plumb the device-reported NetworkCommissioning status through to the NSError's userInfo so commissioning UIs can branch on the underlying cause without parsing logs. * New public NSString constant MTRErrorNetworkCommissioningStatusKey carries the spec NetworkCommissioning status byte as an NSNumber (matching MTRNetworkCommissioningStatus) when the failure originated from a NetworkCommissioning cluster response. The key is absent otherwise, so its presence itself signals that a network-commissioning status was actually reported. * New +[MTRError errorForCHIPErrorCode:logContext:networkCommissioningStatus:] internal factory threads the status into userInfo. The existing +errorForCHIPErrorCode:logContext: keeps its signature and forwards with NullOptional, so non-commissioning callers are unaffected. If an IM-status CHIP_ERROR is paired with a NetworkCommissioning status the factory now logs at error level before falling through to the IM-status path, so the drop is observable in sysdiagnose. * MTRDeviceControllerDelegateBridge now overrides the new OnCommissioningFailure(PeerId, const CompletionStatus &) overload added in upstream PR project-chip#72228 *and* OnCommissioningSuccess(PeerId). OnCommissioningComplete records the (nodeId, error) tuple onto the bridge and returns; the subsequent OnCommissioningSuccess / OnCommissioningFailure call (which always runs synchronously after OnCommissioningComplete on the same Matter work queue) performs the dispatch_async with every input -- including completionStatus's NetworkCommissioning status -- captured by value into the block. This avoids the cross-queue read of a member-variable stash that an earlier draft relied on. The pending record is also defensively cleared if a new OnCommissioningComplete arrives while one is still outstanding, and a missing OnCommissioningSuccess/Failure logs at error rather than silently dropping the event. * Tests in MTRErrorMappingTests pin the new userInfo key for NetworkNotFound (5) and AuthFailure (7), pin that non-network failures do not carry the key, pin that successful commissioning still produces a nil NSError, and -- via a new bridge-level test fixture that drives MTRDeviceControllerDelegateBridge directly -- pin that the OnCommissioningComplete + OnCommissioningFailure call pair surfaces the device-reported status on the delivered NSError and that a subsequent successful attempt does not leak status from the prior failure. No public Obj-C protocol signatures, no MTRErrorCode enum slots, and no MTRMaxErrorCode bump.
woody-apple
force-pushed
the
dev/woody/174955538
branch
from
ad3aef0 to
0884eb6
Compare
|
PR #72272: Size comparison from 8a162c6 to 0884eb6 Full report (19 builds for bl602, bl616, bl702, bl702l, efr32, nrfconnect, telink)
|
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.
Problem
On Darwin, when commissioning fails at the NetworkCommissioning step (e.g. the accessory reports
NetworkIDNotFound,NetworkNotFound, orAuthFailurein itsConnectNetworkResponse/NetworkConfigResponse), the device-reported status byte was discarded. ThecommissioningComplete:delegate callback received a generic, opaqueNSError(typicallyMTRErrorCodeGeneralError, "General error: N"), so a client UI had no way to render an actionable message such as "the accessory could not find the Wi-Fi network — try a 2.4 GHz network or check the SSID and password" versus "the Wi-Fi password was wrong."Root cause
The granular NetworkCommissioning status only exists on the C++
OnCommissioningFailure(peerId, CompletionStatus)callback. But the Darwin bridge built and dispatched itscommissioningComplete:NSErrorfromOnCommissioningComplete(nodeId, error), which carries only the collapsedCHIP_ERROR. By the timeOnCommissioningFailurearrived with the status byte, the callback had already been dispatched, so the byte was dropped on the floor.Fix
MTRError: add a newerrorForCHIPErrorCode:logContext:networkCommissioningStatus:factory overload that, when a status is present, attaches it to the resultingNSError'suserInfounder a new public key,MTRErrorNetworkCommissioningStatusKey(@"MTRNetworkCommissioningStatus"), boxed as anNSNumberof theMTRNetworkCommissioningStatusbyte. The key is attached uniformly whether the error renders intoMTRErrorDomainorMTRInteractionErrorDomain, and its absence means the commissionee reported no NetworkCommissioning status. The byte is forwarded verbatim — never clamped, masked, or sign-extended.MTRDeviceControllerDelegateBridge: fold the status byte into the failure callback. The C++ SDK callsOnCommissioningCompleteand then, synchronously on the same Matter work-queue thread, callsOnCommissioningSuccess(success) orOnCommissioningFailure(failure, carrying the status byte) — seeDeviceCommissioner::SendCommissioningCompleteCallbacks.OnCommissioningComplete, exactly as before — there is no status byte on the success call, so success-path dispatch timing is unchanged. The pairedOnCommissioningSuccessis a no-op.OnCommissioningComplete(failure)stashes(nodeId, error)in three plain members and returns; the immediately-followingOnCommissioningFailurefolds in the status byte and dispatches. This is the minimal change needed to surface the byte, and it does not move the critical success path.Why deferral is scoped to failures only
An earlier revision routed all commissioning (success included) through a dedicated coalescer class with a two-phase state machine. That moved success-path dispatch timing and added a class purely to surface one status byte. Since the byte only exists on the failure call, deferring success buys nothing — so this revision dispatches success eagerly and defers only failures, using three plain bridge members instead of a separate class.
Edge cases
SetUpCodePairer::OnCommissioningComplete(the "Not really expecting this" defensive forward) deliversOnCommissioningCompletewith no paired Success/Failure. If it carried a failure, the record is left pending; it is flushed — never dropped — by the nextOnCommissioningComplete(which flushes any stale pending failure before handling its own result) or by thesetDelegate-rebind flush. Both deliver the callback (without a status byte, since no Failure was observed) to the delegate that was bound whenOnCommissioningCompletefired.setDelegaterebind: drains a pending failure to the currently-bound delegate before swapping bindings, preserving the guarantee that the delegate which sawOnCommissioningCompleteis the one notified. Safe becausesetDelegateruns on the same work queue.OnPairingComplete(failure)may never arrive, so the bridge synthesizes the failure pair itself for the metrics-carrying delegate variant (unchanged from prior behavior; intentionally not widened to the deprecated 1-arg/nodeID:variants)._cancelCommissioning: synthesizesOnCommissioningComplete(CHIP_ERROR_CANCELLED)+ a status-lessOnCommissioningFailureto flush the cancellation through to the client.OnCommissioningFailureclears the pending record as it dispatches, a PASE-synthesized failure followed by a realSendCommissioningCompleteCallbacksfailure produces exactly one callback per pair — there is no per-attempt "already dispatched" flag that a freshOnCommissioningCompletecould reset to re-dispatch a stale record. Pinned bytestTwoPairedFailuresEachDispatchExactlyOnceNoDoubleFire.Tests
MTRErrorMappingTests(16 tests): theMTRErroruserInfo plumbing — status forwarded verbatim across all 13 spec values and out-of-spec bytes, key absent when no status reported, uniform acrossMTRErrorDomain/MTRInteractionErrorDomain, default-branchuserInfo(errorCode, localizedDescription) preserved,CHIP_ERRORround-trip unaffected, the public string-key/NSNumber consumer contract, and@(0)forwarded (not treated as absent).MTRDeviceControllerDelegateBridgeTests(6 tests): drive the realMTRDeviceControllerDelegateBridgethrough its production entry points via aMTR_TESTABLEharness (project-visibility_Test.h, like the existingMTRError_Test.h/MTREndpointInfo_Test.h— not part of the shipping public/private framework surface). Cover the reported-bug failure path, failure-without-status, eager success, rebind flush,_cancelCommissioning, the SetUpCodePairer unpaired-forward drain, and double-fire safety. Reverting the production status fold-in makes the bug-site assertions fail, proving the tests are load-bearing.MTRErrorNetworkCommissioningStatusKeyisMTR_PROVISIONALLY_AVAILABLEuntil it ships in a released SDK.Testing
Added ~517 lines of unit tests in
src/darwin/Framework/CHIPTests/MTRErrorMappingTests.mcovering the new NetworkCommissioning-status-in-MTRError behavior:MTRUnderlyingErrorCodeKeyis populated and round-trips throughNSErrorencode/decode for NetworkCommissioning status codes (NetworkIDNotFound, AuthFailure, etc.).additionalUserInfomerge precedence (framework keys win) on both theMTRDeviceControllerDelegateBridgeIM-status path and the direct error-mapping path.These assertions fail against the pre-change code (the status was previously collapsed/lost) and pass with the change, proving the fix. Run:
MTRErrorMappingTestsin the Darwin framework test bundle.