feat: matter decommission command with RemoveFabric over CASE#56
feat: matter decommission command with RemoveFabric over CASE#56
Conversation
Adds the over-the-air counterpart to `matter fabric remove`. The new command reads OperationalCredentials.CurrentFabricIndex from the device, sends RemoveFabric, and only then deletes the node from the local store. Reuses the session daemon's cached CASE session for both round-trips when available. When the device is unreachable, prompts for confirmation before doing a local-only delete; `--force` skips the prompt. The `matter fabric remove` command now clarifies in its description and output that it is local-only and the device is not notified. Includes unit tests for the rawstruct NOCResponse decoder covering OK, error with debug text, empty input, and unknown-tag cases. Closes #46
Adds the new command to the CLI usage examples alongside the existing `matter fabric remove`, clarifying that one notifies the device and the other does not.
There was a problem hiding this comment.
Pull request overview
Adds a new matter decommission command to properly remove a device from the fabric over CASE (RemoveFabric) before deleting the local node record, and updates existing UX/docs to clearly distinguish local-only removal from full decommissioning.
Changes:
- Introduces
matter decommission [@target]with--forcefallback to local-only deletion on failure. - Updates
matter fabric removehelp text/output to explicitly warn it is local-only and point users tomatter decommission. - Adds unit tests for decoding/parsing
OperationalCredentials.NOCResponsefields and updates CLI usage docs.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
cli/decommission.go |
Implements the new decommission flow (read CurrentFabricIndex, invoke RemoveFabric, then delete locally with optional prompt/force). |
cli/decommission_test.go |
Adds tests for NOCResponse decoding/parsing used by RemoveFabric result handling. |
cli/fabric.go |
Clarifies fabric remove is local-only via help text and runtime warning output. |
CLAUDE.md |
Updates examples to include decommission vs local-only fabric removal. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for { | ||
| if err := r.Next(); err != nil { | ||
| break | ||
| } | ||
| if r.Type() == tlv.TypeEndOfContainer { | ||
| break | ||
| } | ||
| switch r.TagValue().TagNum { | ||
| case 0: | ||
| if v, ok := r.Value().(uint64); ok { | ||
| status = uint8(v) | ||
| } | ||
| case 2: | ||
| if v, ok := r.Value().(string); ok { | ||
| debug = v | ||
| } | ||
| } | ||
| } | ||
| return status, debug, nil |
There was a problem hiding this comment.
decodeNOCResponse currently breaks the loop on any Reader.Next() error and still returns (status, debug, nil). This means malformed/truncated TLV (or container mismatch) can be silently treated as status=0 (OK), causing RemoveFabric failures to be reported as success. Return an error for any Next() error other than io.EOF, and consider tracking whether the mandatory StatusCode (tag 0) was actually present (error if missing) to avoid defaulting to OK.
| resp.StatusCode, interaction.StatusCode(resp.StatusCode)) | ||
| } | ||
| if resp.HasData { | ||
| data, _ := daemon.DecodeFields(resp.Data) |
There was a problem hiding this comment.
In the daemon path, the error from daemon.DecodeFields(resp.Data) is ignored. If decoding fails you’ll currently pass nil/empty fields into parseNOCResponse, which can produce a misleading error (or even success if the NOCResponse decoder is made more permissive). Please handle and wrap the DecodeFields error explicitly so failures are reported accurately.
| data, _ := daemon.DecodeFields(resp.Data) | |
| data, err := daemon.DecodeFields(resp.Data) | |
| if err != nil { | |
| return fmt.Errorf("decoding RemoveFabric response: %w", err) | |
| } |
| stepper.Fail(cause.Error()) | ||
|
|
||
| if force { | ||
| fmt.Fprintf(cmd.ErrOrStderr(), "%s Device is unreachable; --force given, deleting local record anyway.\n", |
There was a problem hiding this comment.
The --force warning text always says "Device is unreachable", but confirmForceDelete() is also used for cases where the device responded and explicitly rejected RemoveFabric (e.g. InvalidFabricIndex). Please reword this message to reflect a generic RemoveFabric failure rather than assuming unreachable.
| fmt.Fprintf(cmd.ErrOrStderr(), "%s Device is unreachable; --force given, deleting local record anyway.\n", | |
| fmt.Fprintf(cmd.ErrOrStderr(), "%s RemoveFabric failed; --force given, deleting local record anyway.\n", |
| fmt.Fprint(cmd.ErrOrStderr(), "Remove from local store anyway? [y/N] ") | ||
|
|
||
| scanner := bufio.NewScanner(cmd.InOrStdin()) | ||
| if !scanner.Scan() { |
There was a problem hiding this comment.
If scanner.Scan() returns false, confirmForceDelete() currently returns false without checking scanner.Err(). That can hide real I/O errors (e.g. broken stdin) and makes debugging harder. Consider checking scanner.Err() and returning it to the caller (e.g. change the helper to return (bool, error)), or at least surface the error to stderr before aborting.
| if !scanner.Scan() { | |
| if !scanner.Scan() { | |
| if err := scanner.Err(); err != nil { | |
| fmt.Fprintf(cmd.ErrOrStderr(), "%s Failed to read confirmation input: %v\n", | |
| output.WarningIcon(), err) | |
| } |
| func contains(s, substr string) bool { | ||
| for i := 0; i+len(substr) <= len(s); i++ { | ||
| if s[i:i+len(substr)] == substr { | ||
| return true | ||
| } | ||
| } | ||
| return false |
There was a problem hiding this comment.
The test helper contains() duplicates strings.Contains from the standard library. Using strings.Contains would simplify the test and remove custom substring-scanning logic.
Summary
matter decommission @N— the over-the-air counterpart tomatter fabric remove. ReadsOperationalCredentials.CurrentFabricIndex, sendsRemoveFabric, then deletes the node locally. Reuses the session daemon's cached CASE session for both round-trips when available.--forceskips the prompt.matter fabric removeis local-only by updating its description and printing a warning pointing atmatter decommissionfor full removals.Test plan
mise run test— all packages pass, including newTestDecodeNOCResponse/TestParseNOCResponsecovering OK, error-with-debug, empty, and unknown-tag cases.matter decommission @1sends RemoveFabric, device drops our fabric, local store entry removed.Closes #46