From e967e8597360dd5c2383e6afab496bfd2877ab3d Mon Sep 17 00:00:00 2001
From: Joseph Doherty <dohejw01@gmail.com>
Date: Mon, 18 May 2026 20:46:12 -0400
Subject: [PATCH] Resolve Client.Go-001 code-review finding

MxAccessError.Unwrap returned e.Command directly; on the HRESULT-only path
Command is a nil *CommandError, so Unwrap returned a non-nil error wrapping
a typed nil and errors.As bound a nil *CommandError. Unwrap now returns an
untyped nil when Command is nil. Added errors_test.go regression coverage
for the HRESULT-only and populated-Command paths.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
---
 clients/go/mxgateway/errors.go      |  6 ++++-
 clients/go/mxgateway/errors_test.go | 42 +++++++++++++++++++++++++++++
 code-reviews/Client.Go/findings.md  |  6 ++---
 3 files changed, 50 insertions(+), 4 deletions(-)
 create mode 100644 clients/go/mxgateway/errors_test.go

diff --git a/clients/go/mxgateway/errors.go b/clients/go/mxgateway/errors.go
index 92dd486..45d114c 100644
--- a/clients/go/mxgateway/errors.go
+++ b/clients/go/mxgateway/errors.go
@@ -85,8 +85,12 @@ func (e *MxAccessError) Error() string {
 }
 
 // Unwrap returns the wrapped CommandError, when one is present.
+//
+// When Command is nil (the HRESULT / MxStatusProxy path) it returns an
+// untyped nil rather than a typed-nil *CommandError, so errors.As does not
+// bind a nil pointer that a caller would then panic on.
 func (e *MxAccessError) Unwrap() error {
-	if e == nil {
+	if e == nil || e.Command == nil {
 		return nil
 	}
 	return e.Command
diff --git a/clients/go/mxgateway/errors_test.go b/clients/go/mxgateway/errors_test.go
new file mode 100644
index 0000000..a664b0a
--- /dev/null
+++ b/clients/go/mxgateway/errors_test.go
@@ -0,0 +1,42 @@
+package mxgateway
+
+import (
+	"errors"
+	"testing"
+)
+
+// TestMxAccessErrorUnwrapHResultPathNoTypedNilCommandError reproduces
+// Client.Go-001: an MxAccessError built via the HRESULT / MxStatusProxy path
+// leaves Command nil. Unwrap must not hand back a typed-nil *CommandError,
+// because errors.As would then succeed while binding a nil pointer and a
+// caller dereferencing it would panic.
+func TestMxAccessErrorUnwrapHResultPathNoTypedNilCommandError(t *testing.T) {
+	hresult := int32(-2147467259) // 0x80004005, a failing HRESULT.
+	reply := &MxCommandReply{Hresult: &hresult}
+
+	err := EnsureMxAccessSuccess("invoke", reply)
+	if err == nil {
+		t.Fatal("expected MxAccessError for a failing HRESULT, got nil")
+	}
+
+	var ce *CommandError
+	if errors.As(err, &ce) {
+		t.Fatalf("errors.As bound *CommandError from an HRESULT-only MxAccessError (ce=%v); "+
+			"a caller dereferencing ce.Status would panic", ce)
+	}
+}
+
+// TestMxAccessErrorUnwrapPopulatedCommand confirms the non-nil Command path
+// still unwraps to the wrapped *CommandError.
+func TestMxAccessErrorUnwrapPopulatedCommand(t *testing.T) {
+	command := &CommandError{Op: "invoke"}
+	err := &MxAccessError{Command: command}
+
+	var ce *CommandError
+	if !errors.As(err, &ce) {
+		t.Fatal("errors.As failed to bind the populated *CommandError")
+	}
+	if ce != command {
+		t.Fatalf("errors.As bound an unexpected *CommandError: got %v want %v", ce, command)
+	}
+}
diff --git a/code-reviews/Client.Go/findings.md b/code-reviews/Client.Go/findings.md
index 943f9f5..212a8a3 100644
--- a/code-reviews/Client.Go/findings.md
+++ b/code-reviews/Client.Go/findings.md
@@ -7,7 +7,7 @@
 | Review date | 2026-05-18 |
 | Commit reviewed | `3cc53a8` |
 | Status | Reviewed |
-| Open findings | 10 |
+| Open findings | 9 |
 
 ## Checklist coverage
 
@@ -33,13 +33,13 @@
 | Severity | High |
 | Category | Correctness & logic bugs |
 | Location | `clients/go/mxgateway/errors.go:88-93`, `clients/go/mxgateway/errors.go:117-128` |
-| Status | Open |
+| Status | Resolved |
 
 **Description:** `MxAccessError.Unwrap` returns `e.Command` directly. `EnsureMxAccessSuccess` constructs `&MxAccessError{Reply: reply}` with `Command` left nil (the HRESULT / failing-`MxStatusProxy` path). When `Command` is a nil `*CommandError`, `Unwrap()` returns a non-nil `error` interface wrapping a nil pointer. Consequently `errors.As(err, &ce)` for `*CommandError` returns `true` while setting `ce` to nil — a caller writing the idiomatic `if errors.As(err, &commandErr) { use commandErr.Status }` nil-dereferences and panics. Verified empirically; the existing test only exercises the populated-`Command` path.
 
 **Recommendation:** Make `Unwrap` return an untyped nil when `Command` is nil: `if e == nil || e.Command == nil { return nil }; return e.Command`. Add a test for the HRESULT-only `MxAccessError` asserting `errors.As(err, &ce)` is `false`.
 
-**Resolution:** _(open)_
+**Resolution:** Resolved 2026-05-18: `MxAccessError.Unwrap` now returns an untyped nil when `Command` is nil, so `errors.As` no longer binds a typed-nil `*CommandError`; added `errors_test.go` regression coverage for the HRESULT-only and populated-`Command` paths.
 
 ### Client.Go-002