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>
This commit is contained in:
@@ -85,8 +85,12 @@ func (e *MxAccessError) Error() string {
|
|||||||
}
|
}
|
||||||
|
|
||||||
// Unwrap returns the wrapped CommandError, when one is present.
|
// 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 {
|
func (e *MxAccessError) Unwrap() error {
|
||||||
if e == nil {
|
if e == nil || e.Command == nil {
|
||||||
return nil
|
return nil
|
||||||
}
|
}
|
||||||
return e.Command
|
return e.Command
|
||||||
|
|||||||
@@ -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)
|
||||||
|
}
|
||||||
|
}
|
||||||
@@ -7,7 +7,7 @@
|
|||||||
| Review date | 2026-05-18 |
|
| Review date | 2026-05-18 |
|
||||||
| Commit reviewed | `3cc53a8` |
|
| Commit reviewed | `3cc53a8` |
|
||||||
| Status | Reviewed |
|
| Status | Reviewed |
|
||||||
| Open findings | 10 |
|
| Open findings | 9 |
|
||||||
|
|
||||||
## Checklist coverage
|
## Checklist coverage
|
||||||
|
|
||||||
@@ -33,13 +33,13 @@
|
|||||||
| Severity | High |
|
| Severity | High |
|
||||||
| Category | Correctness & logic bugs |
|
| Category | Correctness & logic bugs |
|
||||||
| Location | `clients/go/mxgateway/errors.go:88-93`, `clients/go/mxgateway/errors.go:117-128` |
|
| 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.
|
**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`.
|
**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
|
### Client.Go-002
|
||||||
|
|
||||||
|
|||||||
Reference in New Issue
Block a user