Standardize API error handling

docs/error-handling.md

@@ -23,10 +23,51 @@ There are two acceptable ways to construct errors in ToolHive:
 
 ToolHive provides a typed error system for common error scenarios. Each error type has an associated HTTP status code for API responses.
 
-Error types are defined in:
-- `pkg/errors/errors.go` - Core application errors (CLI, proxy, etc.)
+### Creating Errors with HTTP Status Codes
+
+Use `errors.WithCode()` to associate HTTP status codes with errors:
+
+```go
+import (
+    "errors"
+    "net/http"
+
+    thverrors "github.com/stacklok/toolhive/pkg/errors"
+)
+
+// Define an error with a status code
+var ErrWorkloadNotFound = thverrors.WithCode(
+    errors.New("workload not found"),
+    http.StatusNotFound,
+)
+
+// Create a new error inline with a status code
+return thverrors.WithCode(
+    fmt.Errorf("invalid request: %w", err),
+    http.StatusBadRequest,
+)
+```
+
+### Extracting Status Codes
+
+Use `errors.Code()` to extract the HTTP status code from an error:
+
+```go
+code := thverrors.Code(err)  // Returns 500 if no code is found
+```
+
+### Error Definitions
+
+Error types with HTTP status codes are defined in:
+- `pkg/errors/errors.go` - Core error utilities (`WithCode`, `Code`, `CodedError`)
+- `pkg/groups/errors.go` - Group-related errors
+- `pkg/container/runtime/types.go` - Runtime errors (`ErrWorkloadNotFound`)
+- `pkg/workloads/types/validate.go` - Workload validation errors
+- `pkg/secrets/factory.go` - Secrets provider errors
+- `pkg/transport/session/errors.go` - Transport session errors
 - `pkg/vmcp/errors.go` - Virtual MCP Server domain errors
 
+In general, define errors near the code that produces the error.
 
 ## Error Wrapping Guidelines
 
@@ -54,23 +95,41 @@ which particular step is failing. Consider using `errors.WithStack` or `errors.W
 
 ## API Error Handling
 
-### Response Format
-
-API errors are returned as plain text using `http.Error()`:
+### Handler Pattern
 
-Response codes are derived from unwrapping the error and this happens in a common middleware layer.
+API handlers return errors instead of calling `http.Error()` directly. The `ErrorHandler` decorator in `pkg/api/errors/handler.go` converts errors to HTTP responses:
 
-See pkg/api/errors/ for more details. 
-TODO: implement common middleware for error and panic handling.
-TODO: integrate handler into setupDefaultRoutes.
-TODO: update documentation on APIs.
+```go
+// Define a handler that returns an error
+func (s *Routes) getWorkload(w http.ResponseWriter, r *http.Request) error {
+    workload, err := s.manager.GetWorkload(ctx, name)
+    if err != nil {
+        return err  // ErrWorkloadNotFound already has 404 status code
+    }
+
+    // For errors without a status code, wrap with WithCode
+    if someCondition {
+        return thverrors.WithCode(
+            fmt.Errorf("invalid input"),
+            http.StatusBadRequest,
+        )
+    }
+
+    // Success case - write response
+    return json.NewEncoder(w).Encode(workload)
+}
 
+// Wire up with the ErrorHandler decorator
+r.Get("/{name}", apierrors.ErrorHandler(routes.getWorkload))
+```
 
 ### Error Response Behavior
 
-1. **First matching error code wins** - Check specific errors first, then fall back to generic internal server error.
-2. **Hide internal details** - For 500 errors, log the full error but return a generic message to the user
-3. **Include context for client errors** - For 400/404 errors, include helpful context in the message
+1. **Status codes from errors** - The `ErrorHandler` extracts status codes using `errors.Code()`. Errors without codes default to 500.
+2. **Hide internal details** - For 5xx errors, the full error is logged but only a generic message is returned to the user.
+3. **Include context for client errors** - For 4xx errors, the error message is returned to the client.
+
+See `pkg/api/errors/handler.go` for implementation details.
 
 
 ## CLI Error Handling

pkg/api/errors/handler.go

@@ -0,0 +1,49 @@
+// Package errors provides HTTP error handling utilities for the API.
+package errors
+
+import (
+	"net/http"
+
+	"github.com/stacklok/toolhive/pkg/errors"
+	"github.com/stacklok/toolhive/pkg/logger"
+)
+
+// HandlerWithError is an HTTP handler that can return an error.
+// This signature allows handlers to return errors instead of manually
+// writing error responses, enabling centralized error handling.
+type HandlerWithError func(http.ResponseWriter, *http.Request) error
+
+// ErrorHandler wraps a HandlerWithError and converts returned errors
+// into appropriate HTTP responses.
+//
+// The decorator:
+//   - Returns early if no error is returned (handler already wrote response)
+//   - Extracts HTTP status code from the error using errors.Code()
+//   - For 5xx errors: logs full error details, returns generic message to client
+//   - For 4xx errors: returns error message to client
+//
+// Usage:
+//
+//	r.Get("/{name}", apierrors.ErrorHandler(routes.getWorkload))
+func ErrorHandler(fn HandlerWithError) http.HandlerFunc {
+	return func(w http.ResponseWriter, r *http.Request) {
+		err := fn(w, r)
+		if err == nil {
+			// No error returned, handler already wrote the response
+			return
+		}
+
+		// Extract HTTP status code from the error
+		code := errors.Code(err)
+
+		// For 5xx errors, log the full error but return a generic message
+		if code >= http.StatusInternalServerError {
+			logger.Errorf("Internal server error: %v", err)
+			http.Error(w, http.StatusText(code), code)
+			return
+		}
+
+		// For 4xx errors, return the error message to the client
+		http.Error(w, err.Error(), code)
+	}
+}

pkg/api/errors/handler_test.go

@@ -0,0 +1,168 @@
+package errors
+
+import (
+	"errors"
+	"fmt"
+	"net/http"
+	"net/http/httptest"
+	"strings"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+
+	thverrors "github.com/stacklok/toolhive/pkg/errors"
+)
+
+func TestErrorHandler(t *testing.T) {
+	t.Parallel()
+
+	t.Run("passes through successful response", func(t *testing.T) {
+		t.Parallel()
+
+		handler := ErrorHandler(func(w http.ResponseWriter, _ *http.Request) error {
+			w.WriteHeader(http.StatusOK)
+			_, _ = w.Write([]byte("success"))
+			return nil
+		})
+
+		req := httptest.NewRequest(http.MethodGet, "/", nil)
+		rec := httptest.NewRecorder()
+
+		handler.ServeHTTP(rec, req)
+
+		require.Equal(t, http.StatusOK, rec.Code)
+		require.Equal(t, "success", rec.Body.String())
+	})
+
+	t.Run("converts 400 error to HTTP response with message", func(t *testing.T) {
+		t.Parallel()
+
+		handler := ErrorHandler(func(_ http.ResponseWriter, _ *http.Request) error {
+			return thverrors.WithCode(
+				fmt.Errorf("invalid input"),
+				http.StatusBadRequest,
+			)
+		})
+
+		req := httptest.NewRequest(http.MethodGet, "/", nil)
+		rec := httptest.NewRecorder()
+
+		handler.ServeHTTP(rec, req)
+
+		require.Equal(t, http.StatusBadRequest, rec.Code)
+		require.Contains(t, rec.Body.String(), "invalid input")
+	})
+
+	t.Run("converts 404 error to HTTP response with message", func(t *testing.T) {
+		t.Parallel()
+
+		handler := ErrorHandler(func(_ http.ResponseWriter, _ *http.Request) error {
+			return thverrors.WithCode(
+				fmt.Errorf("resource not found"),
+				http.StatusNotFound,
+			)
+		})
+
+		req := httptest.NewRequest(http.MethodGet, "/", nil)
+		rec := httptest.NewRecorder()
+
+		handler.ServeHTTP(rec, req)
+
+		require.Equal(t, http.StatusNotFound, rec.Code)
+		require.Contains(t, rec.Body.String(), "resource not found")
+	})
+
+	t.Run("converts 409 error to HTTP response with message", func(t *testing.T) {
+		t.Parallel()
+
+		handler := ErrorHandler(func(_ http.ResponseWriter, _ *http.Request) error {
+			return thverrors.WithCode(
+				fmt.Errorf("resource already exists"),
+				http.StatusConflict,
+			)
+		})
+
+		req := httptest.NewRequest(http.MethodPost, "/", nil)
+		rec := httptest.NewRecorder()
+
+		handler.ServeHTTP(rec, req)
+
+		require.Equal(t, http.StatusConflict, rec.Code)
+		require.Contains(t, rec.Body.String(), "resource already exists")
+	})
+
+	t.Run("converts 500 error to generic HTTP response", func(t *testing.T) {
+		t.Parallel()
+
+		handler := ErrorHandler(func(_ http.ResponseWriter, _ *http.Request) error {
+			return thverrors.WithCode(
+				fmt.Errorf("sensitive database error details"),
+				http.StatusInternalServerError,
+			)
+		})
+
+		req := httptest.NewRequest(http.MethodGet, "/", nil)
+		rec := httptest.NewRecorder()
+
+		handler.ServeHTTP(rec, req)
+
+		require.Equal(t, http.StatusInternalServerError, rec.Code)
+		// Should NOT contain the sensitive error details
+		require.False(t, strings.Contains(rec.Body.String(), "sensitive"))
+		// Should contain generic message
+		require.Contains(t, rec.Body.String(), "Internal Server Error")
+	})
+
+	t.Run("error without code defaults to 500 with generic message", func(t *testing.T) {
+		t.Parallel()
+
+		handler := ErrorHandler(func(_ http.ResponseWriter, _ *http.Request) error {
+			return errors.New("plain error without code")
+		})
+
+		req := httptest.NewRequest(http.MethodGet, "/", nil)
+		rec := httptest.NewRecorder()
+
+		handler.ServeHTTP(rec, req)
+
+		require.Equal(t, http.StatusInternalServerError, rec.Code)
+		// Should NOT contain the original error details
+		require.False(t, strings.Contains(rec.Body.String(), "plain error"))
+		// Should contain generic message
+		require.Contains(t, rec.Body.String(), "Internal Server Error")
+	})
+
+	t.Run("handles wrapped error with code", func(t *testing.T) {
+		t.Parallel()
+
+		sentinelErr := thverrors.WithCode(
+			errors.New("not found"),
+			http.StatusNotFound,
+		)
+
+		handler := ErrorHandler(func(_ http.ResponseWriter, _ *http.Request) error {
+			return fmt.Errorf("workload lookup failed: %w", sentinelErr)
+		})
+
+		req := httptest.NewRequest(http.MethodGet, "/", nil)
+		rec := httptest.NewRecorder()
+
+		handler.ServeHTTP(rec, req)
+
+		require.Equal(t, http.StatusNotFound, rec.Code)
+		require.Contains(t, rec.Body.String(), "workload lookup failed")
+	})
+}
+
+func TestHandlerWithError_Type(t *testing.T) {
+	t.Parallel()
+
+	// Ensure HandlerWithError can be used as expected
+	var handler HandlerWithError = func(w http.ResponseWriter, _ *http.Request) error {
+		w.WriteHeader(http.StatusOK)
+		return nil
+	}
+
+	wrapped := ErrorHandler(handler)
+	require.NotNil(t, wrapped)
+}