Ensure we explicitly map untracked server health errors (and tools not found) (#161)

* Fix bug #158 by explicitly mapping the server health not tracked error
* Add tests for API level and health tracker level code
* Add pre-emptive fix for unhandled tools not found error (and tests)
* Update code comments for clarity and to aid future devs

Author: peteski22

internal/api/health_test.go

@@ -3,10 +3,12 @@ package api
 import (
 	"fmt"
 	"testing"
+	"time"
 
 	"github.com/stretchr/testify/require"
 
 	"github.com/mozilla-ai/mcpd/v2/internal/domain"
+	"github.com/mozilla-ai/mcpd/v2/internal/errors"
 )
 
 func TestParseHealthStatus_ValidCases(t *testing.T) {
@@ -57,3 +59,72 @@ func TestParseHealthStatus_InvalidCase(t *testing.T) {
 	require.Error(t, err)
 	require.EqualError(t, err, fmt.Sprintf("unknown health status: %s", input))
 }
+
+// mockHealthMonitor implements the MCPHealthMonitor interface for testing.
+type mockHealthMonitor struct {
+	servers map[string]domain.ServerHealth
+}
+
+func newMockHealthMonitor() *mockHealthMonitor {
+	return &mockHealthMonitor{
+		servers: make(map[string]domain.ServerHealth),
+	}
+}
+
+func (m *mockHealthMonitor) Status(name string) (domain.ServerHealth, error) {
+	if health, ok := m.servers[name]; ok {
+		return health, nil
+	}
+	return domain.ServerHealth{}, fmt.Errorf("%w: %s", errors.ErrHealthNotTracked, name)
+}
+
+func (m *mockHealthMonitor) List() []domain.ServerHealth {
+	servers := make([]domain.ServerHealth, 0, len(m.servers))
+	for _, server := range m.servers {
+		servers = append(servers, server)
+	}
+	return servers
+}
+
+func (m *mockHealthMonitor) Update(name string, status domain.HealthStatus, latency *time.Duration) error {
+	m.servers[name] = domain.ServerHealth{
+		Name:    name,
+		Status:  status,
+		Latency: latency,
+	}
+	return nil
+}
+
+func TestHandleHealthServer_ServerNotTracked(t *testing.T) {
+	t.Parallel()
+
+	monitor := newMockHealthMonitor()
+
+	// Try to get health for a server that doesn't exist.
+	result, err := handleHealthServer(monitor, "nonexistent-server")
+	require.Error(t, err)
+	require.Nil(t, result)
+
+	require.ErrorIs(t, err, errors.ErrHealthNotTracked)
+}
+
+func TestHandleHealthServer_ServerExists(t *testing.T) {
+	t.Parallel()
+
+	monitor := newMockHealthMonitor()
+
+	// Add a server to the monitor.
+	latency := 100 * time.Millisecond
+	err := monitor.Update("test-server", domain.HealthStatusOK, &latency)
+	require.NoError(t, err)
+
+	// Get health for existing server.
+	result, err := handleHealthServer(monitor, "test-server")
+	require.NoError(t, err)
+	require.NotNil(t, result)
+
+	require.Equal(t, "test-server", result.Body.Name)
+	require.Equal(t, HealthStatusOK, result.Body.Status)
+	require.NotNil(t, result.Body.Latency)
+	require.Equal(t, "100ms", *result.Body.Latency)
+}

internal/daemon/api_server.go

@@ -153,13 +153,35 @@ func (a *APIServer) applyCORS(mux *chi.Mux) {
 	mux.Use(cors.Handler(corsOptions))
 }
 
-// mapError maps application domain errors to API errors.
+// mapError maps application domain errors to appropriate HTTP status codes.
+//
+// This function is the central place where domain errors from internal/errors are converted to HTTP responses.
+// When adding new errors to internal/errors/errors.go, you MUST add them here to prevent them from falling
+// through to the default case which returns HTTP 500.
+//
+// NOTE: Keep this function in sync with internal/errors/errors.go.
+// Every error defined there should have an explicit case here otherwise it will default to 500.
+//
+// Mapping guidelines:
+//   - 400: Client errors (bad input, invalid requests)
+//   - 403: Authorization/permission errors
+//   - 404: Resource not found errors
+//   - 502: External service/dependency failures
+//   - 500: Unexpected internal errors (default case)
+//
+// Don't forget to:
+// 1. Add test cases to TestMapError (internal/daemon/api_server_test.go)
+// 2. Update the documentation in internal/errors/errors.go
 func mapError(logger hclog.Logger, err error) huma.StatusError {
 	switch {
 	case stdErrors.Is(err, errors.ErrBadRequest):
 		return huma.Error400BadRequest(err.Error())
 	case stdErrors.Is(err, errors.ErrServerNotFound):
 		return huma.Error404NotFound(err.Error())
+	case stdErrors.Is(err, errors.ErrToolsNotFound):
+		return huma.Error404NotFound(err.Error())
+	case stdErrors.Is(err, errors.ErrHealthNotTracked):
+		return huma.Error404NotFound(err.Error())
 	case stdErrors.Is(err, errors.ErrToolForbidden):
 		return huma.Error403Forbidden(err.Error())
 	case stdErrors.Is(err, errors.ErrToolListFailed):

internal/daemon/api_server_test.go

@@ -1,12 +1,15 @@
 package daemon
 
 import (
+	"fmt"
 	"testing"
 	"time"
 
 	"github.com/go-chi/chi/v5"
 	"github.com/hashicorp/go-hclog"
 	"github.com/stretchr/testify/require"
+
+	"github.com/mozilla-ai/mcpd/v2/internal/errors"
 )
 
 func TestNewAPIServer_AppliesDefaults(t *testing.T) {
@@ -284,3 +287,70 @@ func testNewChiMux(t *testing.T) *chi.Mux {
 	t.Helper()
 	return chi.NewMux()
 }
+
+func TestMapError(t *testing.T) {
+	t.Parallel()
+
+	logger := hclog.NewNullLogger()
+
+	tests := []struct {
+		name           string
+		err            error
+		expectedStatus int
+	}{
+		{
+			name:           "ErrBadRequest maps to 400",
+			err:            errors.ErrBadRequest,
+			expectedStatus: 400,
+		},
+		{
+			name:           "ErrServerNotFound maps to 404",
+			err:            errors.ErrServerNotFound,
+			expectedStatus: 404,
+		},
+		{
+			name:           "ErrToolsNotFound maps to 404",
+			err:            errors.ErrToolsNotFound,
+			expectedStatus: 404,
+		},
+		{
+			name:           "ErrHealthNotTracked maps to 404",
+			err:            errors.ErrHealthNotTracked,
+			expectedStatus: 404,
+		},
+		{
+			name:           "ErrToolForbidden maps to 403",
+			err:            errors.ErrToolForbidden,
+			expectedStatus: 403,
+		},
+		{
+			name:           "ErrToolListFailed maps to 502",
+			err:            errors.ErrToolListFailed,
+			expectedStatus: 502,
+		},
+		{
+			name:           "ErrToolCallFailed maps to 502",
+			err:            errors.ErrToolCallFailed,
+			expectedStatus: 502,
+		},
+		{
+			name:           "ErrToolCallFailedUnknown maps to 502",
+			err:            errors.ErrToolCallFailedUnknown,
+			expectedStatus: 502,
+		},
+		{
+			name:           "Unknown error maps to 500",
+			err:            fmt.Errorf("unknown error"),
+			expectedStatus: 500,
+		},
+	}
+
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			t.Parallel()
+
+			statusErr := mapError(logger, tc.err)
+			require.Equal(t, tc.expectedStatus, statusErr.GetStatus())
+		})
+	}
+}

internal/errors/errors.go

@@ -1,16 +1,60 @@
+// Package errors defines domain-level errors used throughout the application.
+// These errors represent business logic failures and are mapped to appropriate HTTP status codes at the API boundary.
+//
+// NOTE: Important for developers
+// When adding a new error here, you MUST consider how it should be handled when returned from API endpoints.
+//
+// Unmapped errors will default to HTTP 500 Internal Server Error.
+//
+// Don't forget to:
+// 1. Add your error to mapError (internal/daemon/api_server.go)
+// 2. Add a test case to TestMapError (internal/daemon/api_server_test.go)
+// 3. Consider if existing handler tests need updates
 package errors
 
 import (
 	"errors"
 )
 
 var (
-	ErrBadRequest            = errors.New("bad request")
-	ErrServerNotFound        = errors.New("server not found")
-	ErrToolsNotFound         = errors.New("tools not found")
-	ErrToolForbidden         = errors.New("tool not allowed")
-	ErrToolListFailed        = errors.New("tool list failed")
-	ErrToolCallFailed        = errors.New("tool call failed")
+	// ErrBadRequest indicates that the client provided invalid input or made a malformed request.
+	// This typically results from validation failures or incorrect request parameters.
+	// Recommended to map to HTTP 400 Bad Request.
+	ErrBadRequest = errors.New("bad request")
+
+	// ErrServerNotFound indicates that the requested MCP server does not exist or is not configured.
+	// This occurs when trying to access operations on a server that hasn't been registered.
+	// Recommended to map to HTTP 404 Not Found.
+	ErrServerNotFound = errors.New("server not found")
+
+	// ErrToolsNotFound indicates that no tools are configured or available for the specified server.
+	// This can happen when a server exists but has no tools defined.
+	// Recommended to map to HTTP 404 Not Found.
+	ErrToolsNotFound = errors.New("tools not found")
+
+	// ErrToolForbidden indicates that the requested tool either does not exist for the MCP server,
+	// or exists but is not allowed to be called.
+	// This occurs when a tool is not in the server's allowed tools list.
+	// Recommended to map to HTTP 403 Forbidden.
+	ErrToolForbidden = errors.New("tool not allowed")
+
+	// ErrToolListFailed indicates that listing tools from an MCP server failed.
+	// This represents a communication or protocol error with the external MCP server.
+	// Recommended to map to HTTP 502 Bad Gateway.
+	ErrToolListFailed = errors.New("tool list failed")
+
+	// ErrToolCallFailed indicates that calling a tool on an MCP server failed.
+	// This represents a communication or execution error with the external MCP server.
+	// Recommended to map to HTTP 502 Bad Gateway.
+	ErrToolCallFailed = errors.New("tool call failed")
+
+	// ErrToolCallFailedUnknown indicates that calling a tool failed for an unknown/unexpected reason.
+	// This is used when the exact cause of the tool call failure cannot be determined.
+	// Recommended to map to HTTP 502 Bad Gateway.
 	ErrToolCallFailedUnknown = errors.New("tool call failed (unknown error)")
-	ErrHealthNotTracked      = errors.New("server health is not being tracked")
+
+	// ErrHealthNotTracked indicates that health monitoring is not enabled for the specified server.
+	// This occurs when trying to get health status for a server that isn't being monitored.
+	// Recommended to map to HTTP 404 Not Found.
+	ErrHealthNotTracked = errors.New("server health is not being tracked")
 )