fix(restyutil): apply 3-reviewer findings (drop WithRetries, add OnError + WithTransport, log hygiene)

Three reviewers (bug / code-quality / senior-engineer) consolidated
the same actionable items:

- Drop WithRetries entirely. Resty's default retry condition only
  fires on transport errors, not 5xx, so the option was misleading
  without an accompanying retry-condition helper. YAGNI-applied:
  no current call site needs retries; bring it back when a real
  caller has a concrete retry policy in mind.
- Rename WithRetries' max parameter -> moot (option dropped). The
  shadow of the Go 1.21 builtin `max` is no longer present.
- Add OnError hook so transport errors (connect refused, DNS, ctx
  deadline, TLS) log too. Without it the helper REGRESSED
  observability vs. the raw net/http boilerplate it replaces.
- Add WithTransport(http.RoundTripper) Option. Wraps the supplied
  transport with otelhttp so OTel propagation is preserved by
  construction, even with custom TLS / proxy / dialer. Required
  before pkg/oidc (which needs InsecureSkipVerify in dev) can
  migrate.
- Strip query string from URL in slog log fields. URLs can carry
  ?token=... or ?api_key=... in the wild; CLAUDE.md "Never log
  tokens, passwords, or full message bodies".
- Propagate request_id from ctx into the slog line via
  natsutil.RequestIDFromContext. CLAUDE.md "include in all log
  lines".

Tests cover OnError firing on transport error, WithTransport
preserving the custom round-tripper, request_id propagation, and
query-string stripping.

Author: claude

pkg/restyutil/restyutil.go

@@ -1,4 +1,4 @@
-// Package restyutil constructs a resty client wired with codebase defaults: OTel transport, slog response logging, 30s timeout.
+// Package restyutil constructs a resty client with codebase defaults: OTel transport, slog request/response logging, 30s timeout.
 package restyutil
 
 import (
@@ -8,8 +8,11 @@ import (
 
 	"github.com/go-resty/resty/v2"
 	"go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp"
+
+	"github.com/hmchangw/chat/pkg/natsutil"
 )
 
+// 30s suits arbitrary downstream HTTP, distinct from the 5s probe in mongoutil/valkeyutil.
 const defaultTimeout = 30 * time.Second
 
 type Option func(*resty.Client)
@@ -26,11 +29,9 @@ func WithBearerToken(token string) Option {
 	return func(c *resty.Client) { c.SetAuthToken(token) }
 }
 
-// WithRetries configures n retries with exponential backoff between wait and max.
-func WithRetries(n int, wait, max time.Duration) Option {
-	return func(c *resty.Client) {
-		c.SetRetryCount(n).SetRetryWaitTime(wait).SetRetryMaxWaitTime(max)
-	}
+// WithTransport replaces the default transport; OTel is wrapped on for you.
+func WithTransport(rt http.RoundTripper) Option {
+	return func(c *resty.Client) { c.SetTransport(otelhttp.NewTransport(rt)) }
 }
 
 func New(baseURL string, opts ...Option) *resty.Client {
@@ -39,18 +40,47 @@ func New(baseURL string, opts ...Option) *resty.Client {
 		SetTimeout(defaultTimeout).
 		SetTransport(otelhttp.NewTransport(http.DefaultTransport))
 
-	c.OnAfterResponse(func(_ *resty.Client, resp *resty.Response) error {
-		slog.Debug("http response",
-			"method", resp.Request.Method,
-			"url", resp.Request.URL,
-			"status", resp.StatusCode(),
-			"duration_ms", resp.Time().Milliseconds(),
-		)
-		return nil
-	})
+	c.OnAfterResponse(logResponse)
+	c.OnError(logError)
 
 	for _, opt := range opts {
 		opt(c)
 	}
 	return c
 }
+
+func logResponse(_ *resty.Client, resp *resty.Response) error {
+	slog.Debug("http response", logFields(resp.Request, resp.StatusCode(), resp.Time(), nil)...)
+	return nil
+}
+
+func logError(req *resty.Request, err error) {
+	slog.Debug("http error", logFields(req, 0, time.Since(req.Time), err)...)
+}
+
+func logFields(req *resty.Request, status int, dur time.Duration, err error) []any {
+	// Strip RawQuery from URL — query strings can carry tokens (CLAUDE.md: never log tokens).
+	host, path := "", req.URL
+	if rr := req.RawRequest; rr != nil && rr.URL != nil {
+		host = rr.URL.Host
+		path = rr.URL.Path
+	}
+	fields := []any{
+		"method", req.Method,
+		"host", host,
+		"path", path,
+		"duration_ms", dur.Milliseconds(),
+	}
+	if rr := req.RawRequest; rr != nil {
+		if id := natsutil.RequestIDFromContext(rr.Context()); id != "" {
+			fields = append(fields, "request_id", id)
+		}
+	}
+	if status > 0 {
+		fields = append(fields, "status", status)
+	}
+	if err != nil {
+		fields = append(fields, "error", err.Error())
+	}
+	return fields
+}

pkg/restyutil/restyutil_test.go

@@ -1,14 +1,22 @@
 package restyutil
 
 import (
+	"bytes"
+	"context"
+	"log/slog"
+	"net"
 	"net/http"
 	"net/http/httptest"
+	"strings"
+	"sync/atomic"
 	"testing"
 	"time"
 
 	"github.com/go-resty/resty/v2"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
+
+	"github.com/hmchangw/chat/pkg/natsutil"
 )
 
 func TestNew_DefaultTimeout(t *testing.T) {
@@ -44,11 +52,21 @@ func TestWithBearerToken(t *testing.T) {
 	assert.Equal(t, "Bearer abc123", got)
 }
 
-func TestWithRetries(t *testing.T) {
-	c := New("http://example", WithRetries(3, 10*time.Millisecond, 100*time.Millisecond))
-	assert.Equal(t, 3, c.RetryCount)
-	assert.Equal(t, 10*time.Millisecond, c.RetryWaitTime)
-	assert.Equal(t, 100*time.Millisecond, c.RetryMaxWaitTime)
+func TestWithTransport_PreservesOTelWrap(t *testing.T) {
+	var hits atomic.Int32
+	custom := roundTripperFunc(func(r *http.Request) (*http.Response, error) {
+		hits.Add(1)
+		return http.DefaultTransport.RoundTrip(r)
+	})
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
+		w.WriteHeader(http.StatusOK)
+	}))
+	defer srv.Close()
+
+	c := New(srv.URL, WithTransport(custom))
+	_, err := c.R().Get("/")
+	require.NoError(t, err)
+	assert.Equal(t, int32(1), hits.Load(), "custom transport must run")
 }
 
 // End-to-end: real httptest server, GET, JSON decode into a struct.
@@ -74,22 +92,75 @@ func TestNew_RoundTrip(t *testing.T) {
 	assert.Equal(t, "pong", got.Msg)
 }
 
-// Verify retries actually fire on 5xx responses when WithRetries is set.
-func TestWithRetries_FiresOn5xx(t *testing.T) {
-	var hits int
+// Verify the Debug log line includes request_id when the ctx carries it.
+func TestLog_PropagatesRequestID(t *testing.T) {
+	var buf bytes.Buffer
+	prev := slog.Default()
+	slog.SetDefault(slog.New(slog.NewTextHandler(&buf, &slog.HandlerOptions{Level: slog.LevelDebug})))
+	defer slog.SetDefault(prev)
+
 	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
-		hits++
-		w.WriteHeader(http.StatusInternalServerError)
+		w.WriteHeader(http.StatusOK)
 	}))
 	defer srv.Close()
 
-	c := New(srv.URL, WithRetries(2, time.Millisecond, 5*time.Millisecond)).
-		AddRetryCondition(func(r *resty.Response, _ error) bool {
-			return r.StatusCode() >= 500
-		})
+	ctx := natsutil.WithRequestID(context.Background(), "req-abc-123")
+	c := New(srv.URL)
+	_, err := c.R().SetContext(ctx).Get("/x?token=secret")
+	require.NoError(t, err)
+
+	out := buf.String()
+	assert.Contains(t, out, "request_id=req-abc-123")
+	assert.NotContains(t, out, "secret", "query string must be stripped from logs")
+}
+
+// Verify OnError fires on transport failure (not just OnAfterResponse).
+func TestLog_FiresOnTransportError(t *testing.T) {
+	var buf bytes.Buffer
+	prev := slog.Default()
+	slog.SetDefault(slog.New(slog.NewTextHandler(&buf, &slog.HandlerOptions{Level: slog.LevelDebug})))
+	defer slog.SetDefault(prev)
 
-	resp, err := c.R().Get("/")
+	// Bind+close a port to guarantee connect-refused.
+	l, err := net.Listen("tcp", "127.0.0.1:0")
 	require.NoError(t, err)
-	assert.Equal(t, http.StatusInternalServerError, resp.StatusCode())
-	assert.Equal(t, 3, hits, "1 initial + 2 retries")
+	addr := l.Addr().String()
+	require.NoError(t, l.Close())
+
+	c := New("http://"+addr, WithTimeout(500*time.Millisecond))
+	_, err = c.R().Get("/")
+	require.Error(t, err)
+	assert.Contains(t, buf.String(), "http error")
+}
+
+type roundTripperFunc func(*http.Request) (*http.Response, error)
+
+func (f roundTripperFunc) RoundTrip(r *http.Request) (*http.Response, error) { return f(r) }
+
+// Sanity: log fields construction doesn't panic on a nil RawRequest (rare; defensive guard).
+func TestLogFields_NilRawRequest(t *testing.T) {
+	req := (&resty.Client{}).R()
+	fields := logFields(req, 200, 5*time.Millisecond, nil)
+	require.NotEmpty(t, fields)
+	assert.Equal(t, "method", fields[0])
+	// Should NOT panic and should not include path-from-RawRequest (since RawRequest is nil).
+	joined := strings.Join(toStrings(fields), " ")
+	assert.NotContains(t, joined, "panic")
+}
+
+func toStrings(in []any) []string {
+	out := make([]string, len(in))
+	for i, v := range in {
+		out[i] = fmtAny(v)
+	}
+	return out
+}
+
+func fmtAny(v any) string {
+	switch s := v.(type) {
+	case string:
+		return s
+	default:
+		return ""
+	}
 }