fix(observability): keep /readyz controller-backed when health disabled (MCP-32)

CodexReviewer finding: passing a non-nil observability manager made the HTTP
API skip its controller-backed health block entirely, so /readyz (and the
/livez,/health aliases) either 404'd or fell back to the observability health
manager's vacuous always-ready handler — a config-gated feature must be a no-op
for readiness when disabled.

- httpapi: register /metrics independently of the health endpoints; only let
  the observability health manager own /healthz+/readyz when it is actually
  enabled, otherwise keep the controller-backed handlers authoritative.
- server: disable the observability health manager in buildObservabilityConfig
  (out of scope for MCP-32; its readiness is vacuous) so controller readiness
  stays authoritative even when metrics are enabled.
- tests: regression test that with metrics ON / health OFF, /readyz reflects
  controller readiness (503 when not ready) and the liveness aliases + /metrics
  are all served; assert buildObservabilityConfig keeps health off.

Related MCP-32

Co-Authored-By: Paperclip <noreply@paperclip.ing>

Author: Dumbris

internal/httpapi/observability_readiness_test.go

@@ -0,0 +1,58 @@
+package httpapi
+
+import (
+	"net/http"
+	"net/http/httptest"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"go.uber.org/zap"
+
+	"github.com/smart-mcp-proxy/mcpproxy-go/internal/observability"
+)
+
+// notReadyController reports IsReady()==false so we can distinguish the
+// controller-backed readiness handler (503) from the observability health
+// manager's vacuous always-ready handler (200).
+type notReadyController struct{ MockServerController }
+
+func (m *notReadyController) IsReady() bool { return false }
+
+// MCP-32 regression: enabling the metrics exporter must NOT take over /readyz.
+// When the observability health manager is not active, /readyz must keep
+// reflecting controller readiness (a config-gated feature is a no-op for
+// readiness). Previously passing a non-nil observability manager skipped the
+// controller health block entirely.
+func TestReadyz_StaysControllerBackedWhenObservabilityHasNoHealth(t *testing.T) {
+	obsCfg := observability.Config{
+		Metrics: observability.MetricsConfig{Enabled: true},
+		Health:  observability.HealthConfig{Enabled: false},
+		Tracing: observability.TracingConfig{Enabled: false},
+	}
+	mgr, err := observability.NewManager(zap.NewNop().Sugar(), &obsCfg)
+	require.NoError(t, err)
+	require.NotNil(t, mgr.Metrics())
+	require.Nil(t, mgr.Health())
+
+	srv := NewServer(&notReadyController{}, zap.NewNop().Sugar(), mgr)
+
+	// /readyz reflects the controller (not ready -> 503), not a vacuous 200.
+	rec := httptest.NewRecorder()
+	srv.router.ServeHTTP(rec, httptest.NewRequest(http.MethodGet, "/readyz", http.NoBody))
+	assert.Equal(t, http.StatusServiceUnavailable, rec.Code)
+	assert.Contains(t, rec.Body.String(), `"ready":false`)
+
+	// Liveness aliases remain registered too.
+	for _, path := range []string{"/healthz", "/livez", "/health"} {
+		r := httptest.NewRecorder()
+		srv.router.ServeHTTP(r, httptest.NewRequest(http.MethodGet, path, http.NoBody))
+		assert.Equalf(t, http.StatusOK, r.Code, "liveness endpoint %s should be registered", path)
+	}
+
+	// And the metrics exporter is still served.
+	rm := httptest.NewRecorder()
+	srv.router.ServeHTTP(rm, httptest.NewRequest(http.MethodGet, "/metrics", http.NoBody))
+	assert.Equal(t, http.StatusOK, rm.Code)
+	assert.Contains(t, rm.Body.String(), "mcpproxy_uptime_seconds")
+}

internal/httpapi/server.go

@@ -560,17 +560,23 @@ func (s *Server) setupRoutes() {
 		_, _ = w.Write([]byte(`{"ready":false}`))
 	}
 
-	// Observability endpoints (registered first to avoid conflicts)
+	// Observability /metrics endpoint (MCP-32). Independent of the health
+	// endpoints below: enabling metrics must not change readiness semantics.
 	if s.observability != nil {
-		if health := s.observability.Health(); health != nil {
-			s.router.Get("/healthz", health.HealthzHandler())
-			s.router.Get("/readyz", health.ReadyzHandler())
-		}
 		if metrics := s.observability.Metrics(); metrics != nil {
 			s.router.Handle("/metrics", metrics.Handler())
 		}
+	}
+
+	// Health and readiness endpoints. The observability health manager only
+	// takes over when it is actually enabled; otherwise the controller-backed
+	// handlers remain authoritative. A config-gated feature (metrics/tracing)
+	// must be a no-op for readiness when health is not enabled (MCP-32).
+	if s.observability != nil && s.observability.Health() != nil {
+		health := s.observability.Health()
+		s.router.Get("/healthz", health.HealthzHandler())
+		s.router.Get("/readyz", health.ReadyzHandler())
 	} else {
-		// Register custom health endpoints only if observability is not available
 		for _, path := range []string{"/livez", "/healthz", "/health"} {
 			s.router.Get(path, livenessHandler)
 		}

internal/server/observability_config_test.go

@@ -31,6 +31,19 @@ func TestBuildObservabilityConfig_DefaultConfigOff(t *testing.T) {
 	assert.False(t, out.Tracing.Enabled)
 }
 
+// MCP-32 regression: the observability health manager is never wired (its
+// readiness is vacuous), so /readyz stays controller-backed even with metrics on.
+func TestBuildObservabilityConfig_HealthAlwaysDisabled(t *testing.T) {
+	off := buildObservabilityConfig(config.DefaultConfig())
+	assert.False(t, off.Health.Enabled, "health manager must stay off (metrics off)")
+
+	cfg := config.DefaultConfig()
+	cfg.Observability.Metrics = &config.MetricsExporterConfig{Enabled: true}
+	on := buildObservabilityConfig(cfg)
+	assert.True(t, on.Metrics.Enabled)
+	assert.False(t, on.Health.Enabled, "health manager must stay off even with metrics on")
+}
+
 func TestBuildObservabilityConfig_EnablesAndMapsTracing(t *testing.T) {
 	cfg := config.DefaultConfig()
 	cfg.Observability.Metrics = &config.MetricsExporterConfig{Enabled: true}

internal/server/server.go

@@ -101,6 +101,10 @@ func buildObservabilityConfig(cfg *config.Config) observability.Config {
 	// DefaultConfig enables metrics; flip to opt-in per the MCP-32 directive.
 	out.Metrics.Enabled = false
 	out.Tracing.Enabled = false
+	// The observability package's health manager is out of scope for MCP-32 and
+	// its readiness is vacuous (no registered checkers). Keep the existing
+	// controller-backed /healthz and /readyz authoritative in all cases.
+	out.Health.Enabled = false
 
 	obs := cfg.Observability
 	if obs == nil {