Slim AsertoError: single wrapped error, fixed grpc/http correlation, drop dead methods

.github/workflows/ci.yaml

@@ -17,38 +17,35 @@ on:
 env:
   PRE_RELEASE: ${{ github.ref == 'refs/heads/main' && 'development' || '' }}
   GO_VER: "1.26"
-  GO_LANGCI_LINT_VER: v2.11.4
-  GO_TESTSUM_VER: "1.13.0"
+  GO_LANGCI_LINT_VER: "v2.13.1"
 
 jobs:
   test:
     runs-on: ubuntu-latest
     steps:
       - 
-        uses: actions/checkout@v6
+        uses: actions/checkout@v7
         with:
           fetch-depth: 0
       - 
         name: Setup Go
-        uses: actions/setup-go@v6
+        uses: actions/setup-go@v7
         with:
           go-version-file: 'go.mod'
+      -
+        name: Test Setup
+        run: |
+          make install-gotestsum
       -
         name: Lint
         uses: golangci/golangci-lint-action@v9
         with:
           version: ${{ env.GO_LANGCI_LINT_VER }}
-          verify: false  # `golangci-lint config verify` checks against the latest schema instead of its own version.
           args: --timeout=30m
-      -
-        name: Test Setup
-        uses: gertd/action-gotestsum@v3.0.0
-        with:
-          gotestsum_version: ${{ env.GO_TESTSUM_VER }}
       -
         name: Test
         run: |
-          gotestsum --format short-verbose -- -count=1 -parallel=1 -v -timeout=240s -coverprofile=cover.out -coverpkg=./... ./...
+          .ext/bin/gotestsum --format short-verbose -- -count=1 -parallel=1 -v -timeout=240s -coverprofile=cover.out -coverpkg=./... ./...
       -
         name: Upload code coverage
         uses: shogo82148/actions-goveralls@v1

.golangci.yaml

@@ -13,6 +13,7 @@ linters:
     - depguard
     - exhaustruct
     - gochecknoglobals # no configuration options
+    - gomodguard # deprecated (since v2.12.0) due to: new major version
     - nlreturn # redundant with wsl
     - noinlineerr
     - paralleltest

DESIGN.md

@@ -0,0 +1,251 @@
+# AsertoError & Logging Redesign
+
+Status: **Draft — for discussion, no code changes yet.**
+
+## 1. Motivation
+
+`AsertoError` was built to do three jobs at once: a sentinel-error registry, a
+structured-context builder (mirroring zerolog's `Event` API), and a
+grpc\<->HTTP status bridge. Reviewing the type against how it's actually used
+in the three consumers that matter (`go-directory/pkg/derr`, `topaz`, `azm`)
+turned up two confirmed bugs and a design mismatch between what the package
+offers and what's actually used:
+
+- `Copy()` shallow-copies the `errs []error` slice (`errors.go:62-77`), so two
+  errors derived from the same parent via `.Err()` can share a backing array
+  and silently overwrite each other once Go's slice growth leaves capacity
+  slack. Reproduced directly — a 3-deep chain branched into two siblings and
+  one silently absorbed the other's appended error. `azm` already does
+  `derr.ErrX.Err(derr.ErrY.Msgf(...))` (`safe/relation.go` and others), so
+  this is live exposure, not a lab curiosity.
+- `FromGRPCStatus` (`errors.go:280-301`) mutates the package-level singleton
+  returned by `asertoErrors[code]` in place (`result.data = t.Metadata`)
+  instead of operating on a copy. Reproduced directly — calling it once
+  permanently changes the shared `data` on every future use of that error
+  code, and it's an unsynchronized concurrent write on top of that.
+- The package's own `GRPCStatus()` (`errors.go:249-260`) is never called by
+  any of the three consumers, and it's the broken half of the design: it
+  never writes `HTTPCode` into the `errdetails.ErrorInfo` metadata that
+  `CustomErrorHandler` (`custom_error_handler.go`) reads back out. The actual
+  grpc\<->HTTP correlation in production is hand-rolled a second time in
+  `topaz/internal/grpc/middlewares/gerr/gerr_middleware.go`, which manually
+  calls `.Int(aerr.HTTPStatusErrorMetadata, asertoErr.HTTPCode)` before
+  building the status — the one thing this package exists to provide is
+  duplicated and has drifted from the library's own version of it.
+- Usage sweep across `go-directory`, `topaz`, `azm` (grep for real
+  `AsertoError`-typed call chains, not zerolog's identically-named builder
+  methods): `NewAsertoError`, `.Msg`/`.Msgf`, and `.Err` are the workhorses.
+  `.Str`/`.Int32`/`.Int64`/`.Bool`/`.Duration`/`.Time`/`.FromReader`/
+  `.Interface`/`SameAs`/`Equals`/`WithGRPCStatus`/`WithHTTPStatus` have zero
+  real call sites in any of the three repos. `aerr.Logger`/`.Data()`/
+  `.Fields()`/`ContextError` have exactly one call site — `gerr_middleware.go`
+  — but it's the single choke point every gRPC error response in `topaz`
+  passes through, so it's load-bearing despite the low count. `azm` never
+  touches the logging surface at all, yet imports it transitively (zerolog,
+  `grpc-ecosystem/grpc-gateway/v2/runtime`) for zero benefit.
+
+This is why the fix isn't a patch to `Copy()` — the logging/gRPC-gateway
+coupling and the wrapping mechanism are the same code paths that need to
+change shape anyway.
+
+## 2. Non-goals
+
+- Not rewriting `topaz`'s or `azm`'s own general-purpose logging (their
+  existing `.Str().Msg()` zerolog call sites outside the errors-package
+  boundary are untouched).
+- Not touching the error *registries* (`derr`, `internal/eds/pkg/ds/error.go`)
+  — they keep calling `NewAsertoError(code, grpcCode, httpCode, msg)`
+  unchanged.
+- Not picking a new wire format for cross-service error propagation —
+  `errdetails.ErrorInfo` inside the grpc `Status` stays; it's the right
+  mechanism for the stated goal (survives the grpc-gateway boundary,
+  standard idiom).
+
+## 3. Proposed design
+
+### P1 — Slim the core type
+
+Keep exactly what's used; deprecate the rest for one release cycle before
+removal (see §5).
+
+```go
+type AsertoError struct {
+    Code     string
+    GRPCCode codes.Code
+    HTTPCode int
+    Message  string
+    data     map[string]string // copy-on-write, as today
+    wrapped  error              // see P3 — was []error
+}
+```
+
+Kept: `NewAsertoError`, `Msg`/`Msgf`, `Err`, `Str` (used to attach a single
+metadata field — keep, low cost, unlike the rest of the typed setters),
+`Data`/`Fields`, `Error`/`Unwrap`, `Copy`, `Ctx`.
+
+Deprecated for removal: `Int`/`Int32`/`Int64`/`Bool`/`Duration`/`Time`/
+`FromReader`/`Interface`, `SameAs`, `Equals`, `WithGRPCStatus`,
+`WithHTTPStatus`. Zero real call sites found across all three consumers.
+
+### P2 — One source of truth for grpc\<->HTTP correlation
+
+Replace the broken `GRPCStatus()` with a helper that both the library and
+`gerr_middleware.go` can use, including the `Reason` field the middleware
+currently sets itself (a correlation/error ID, which is caller-specific and
+shouldn't be generated by this package):
+
+```go
+// ErrorInfo returns the errdetails.ErrorInfo for this error, with HTTPCode
+// folded into Metadata under HTTPStatusErrorMetadata.
+func (e *AsertoError) ErrorInfo(reason string) *errdetails.ErrorInfo
+
+// GRPCStatus builds the grpc Status via ErrorInfo(""). Satisfies the
+// GRPCStatus() *status.Status interface some grpc tooling expects.
+func (e *AsertoError) GRPCStatus() *status.Status
+```
+
+`gerr_middleware.go` then becomes:
+
+```go
+errResult, err := status.New(asertoErr.GRPCCode, asertoErr.Error()).
+    WithDetails(asertoErr.ErrorInfo(errID.String()))
+```
+
+removing its hand-rolled duplicate of `WithDetails`/`ErrorInfo` construction,
+and guaranteeing `CustomErrorHandler` always finds the HTTP status key,
+because the same code path is now the only path.
+
+### P3 — One wrapping mechanism
+
+`errs []error` plus custom `Error()`/`Unwrap()` string-joining coexists today
+with `pkg/errors`-style wrapping (`ContextError.Cause`) and native
+`errors.Unwrap`. Nothing in `go-directory`, `topaz`, or `azm` chains more than
+one `.Err()` call in practice (the multi-error chain is only exercised by
+this package's own tests). Replace the slice with a single `wrapped error`
+field:
+
+```go
+func (e *AsertoError) Err(err error) *AsertoError {
+    if err == nil {
+        return e
+    }
+    c := e.Copy()
+    c.wrapped = err
+    return c
+}
+```
+
+This removes the aliasing bug by construction — there's no shared backing
+array left to alias — rather than papering over it with a deep copy that
+still leaves a subtler version of the same hazard if the slice comes back
+later. If genuine multi-error wrapping turns out to be needed somewhere, the
+stdlib `errors.Join` is the right tool for that, orthogonal to this type.
+
+*This changes `Error()`'s multi-wrap output format (`TestDoubleCerr`,
+`TestError`'s 4-chain case) — those tests get rewritten, not preserved.*
+
+### P4 — Logging: `slog` at the boundary, zerolog stays the engine in `topaz`
+
+- `errors` core module drops the `zerolog` dependency entirely.
+- `AsertoError` implements `slog.LogValuer`:
+
+  ```go
+  func (e *AsertoError) LogValue() slog.Value
+  ```
+
+  replacing `MarshalZerologObject`.
+- A new `ctxlog` package owns the "logger in context" convention that
+  `ContextError`/`extractLogger` already half-implement today, but as a
+  standalone utility rather than only reachable by wrapping an error
+  (`log/slog` deliberately ships no such helper — this has to be owned):
+
+  ```go
+  package ctxlog
+
+  func With(ctx context.Context, logger *slog.Logger) context.Context
+  func From(ctx context.Context) *slog.Logger
+  ```
+
+- `Logger(err error)` walks the error chain like today but returns
+  `*slog.Logger`, sourced via `ctxlog.From`.
+- `topaz` keeps zerolog as its actual encoder by writing (or adopting) a
+  small `slog.Handler` backed by a `zerolog.Logger` — the 4-method interface
+  (`Enabled`/`Handle`/`WithAttrs`/`WithGroup`) is cheap to own directly rather
+  than take on a third-party bridge dependency. Two things this bridge must
+  handle deliberately, or they silently regress:
+  - `gerr_middleware.go`'s `.Stack()` call (via `pkg/errors`) — no native
+    `slog` stack-trace attr convention exists; the handler needs to recognize
+    and format it.
+  - Allocation behavior — zerolog's whole pitch is zero-allocation logging;
+    routing everything through generic `slog.Record`/`Attr` conversion adds
+    some. Almost certainly noise next to network/DB calls on topaz's request
+    path, but worth one benchmark on the per-decision logging path before
+    assuming it's free, not after.
+
+### P5 — Dependency isolation needs module boundaries, not just packages
+
+`azm` importing `errors` today pulls in zerolog and
+`grpc-ecosystem/grpc-gateway/v2/runtime` transitively even though it uses
+neither. Moving `CustomErrorHandler` and the zerolog bridge into
+*subpackages* of the same module doesn't fully fix this — Go's module graph
+still resolves those requirements for anything that imports the module at
+all. Getting `azm` genuinely free of zerolog/grpc-gateway requires **separate
+`go.mod` files**:
+
+- `github.com/aserto-dev/errors` — core type, no zerolog, no grpc-gateway.
+- `github.com/aserto-dev/errors/zlog` (or similar) — the `slog.Handler`
+  zerolog bridge + `ctxlog`. Only `topaz` imports it.
+- `github.com/aserto-dev/errors/httpgw` — `CustomErrorHandler`. Only whatever
+  actually runs a grpc-gateway `ServeMux` imports it.
+
+This is a real increase in release/versioning overhead (three modules to tag
+instead of one) — flagged as an open question in §6, not a foregone
+conclusion.
+
+## 4. What doesn't change
+
+- `NewAsertoError(code, grpcCode, httpCode, msg)` registry pattern — used
+  ~45+ times in `go-directory/derr` alone, untouched.
+- `errdetails.ErrorInfo` inside grpc `Status` as the wire mechanism.
+- Context-carried logger as the standard, replacing the current
+  inconsistent mix of direct-pass and context-pass in `topaz` (per prior
+  discussion) — this is what `ctxlog` formalizes.
+
+## 5. Sequencing
+
+| Phase | Scope | Depends on |
+|---|---|---|
+| 0 | Decide open questions below | — |
+| 1 | `errors`: P1 (slim type) + P3 (single `wrapped`) + P2 (`ErrorInfo`/`GRPCStatus` fix) — pure logic, no logging changes | Phase 0 |
+| 2 | `errors`: split `zlog`/`httpgw` into separate modules (P5), drop zerolog/grpc-gateway from core `go.mod` | Phase 0 (module-split decision) |
+| 3 | `errors/zlog`: `ctxlog` + zerolog-backed `slog.Handler` + `LogValue()` on `AsertoError` (P4) | Phase 2 |
+| 4 | `topaz`: migrate `gerr_middleware.go` to `ErrorInfo()`/`ctxlog`, removing its duplicate grpc-status construction | Phase 1, 3 |
+| 5 | `topaz`/`azm`: bump to the new `errors` version; deprecated methods removed once the same repo-search technique used in this review confirms zero remaining call sites | Phase 4 |
+
+Both `go-directory/derr` (`v0.34.0` pinned in `topaz`) and `azm`
+(`v0.0.17` pinned — notably far behind `topaz`'s pin) are still pre-1.0, so
+Go's semver convention allows breaking changes on a minor bump without a
+`/v2` module path. That gives more room to move than a post-1.0 package
+would have, but `azm`'s stale pin is worth checking — it may mean nobody
+has attempted an upgrade in a while for a reason not visible from usage
+grep alone.
+
+## 6. Open questions
+
+1. **Module split (P5).** Worth the added release overhead of three
+   versioned modules instead of one, to fully isolate `azm` from
+   zerolog/grpc-gateway? Or is "core module free of those deps, `topaz`-only
+   subpackages that `azm` simply never imports" (accepting the module-graph
+   resolution cost, not the compile cost) good enough in practice?
+2. **Urgency of the `Copy()` fix.** P3 removes the aliasing bug by
+   construction, but it's bundled with the larger redesign. Given `azm`
+   already exercises the vulnerable pattern in production, does the
+   `errs`→`wrapped` change (or a minimal interim deep-copy patch) need to
+   ship on its own first, ahead of the rest of this doc?
+3. **Deprecation window.** How long do the dead builder methods stay present
+   (marked `Deprecated:`) before hard removal — tied to how quickly `topaz`/
+   `azm` can bump their pins.
+4. **Where `ctxlog` lives.** It's a generically useful "logger in context"
+   helper with no inherent connection to errors. Keep it inside this module
+   (even if split per P5) since it grew out of `ContextError`, or should it
+   be its own small standalone module from the start?