fix(mcp): strip inherited chi.RouteCtxKey before synthesizing dispatched requests (TASK-1075) (#384)

Every production /mcp tool call was returning 404 from the dispatcher's
synthesized /api/v1/... request, surfacing in Claude Desktop /
Cursor / ChatGPT as the generic
\"{code:'item_not_found', hint:'404 page not found'}\" envelope on
pad_workspace_list, pad_item_show, pad_project_dashboard, and every
other tool. Codex review caught the actual cause.

## Root cause

chi's Mux.ServeHTTP short-circuits when the inbound request context
already carries a chi.RouteCtxKey (chi/v5/mux.go:71-75):

    rctx, _ := r.Context().Value(RouteCtxKey).(*Context)
    if rctx != nil {
        mx.handler.ServeHTTP(w, r)  // bypass fresh routing
        return
    }

That's the right behavior for chi's own Sub() / Mount() patterns
(running as a sub-router under a parent), but wrong for our case:
we synthesize a brand-new HTTP request that needs to route from
scratch against the ROOT mux.

In production every MCP call enters via chi's /mcp route — chi
attaches a RouteCtxKey to the inbound request context, mcp-go
threads that context through to the tool handler, and the dispatcher
inherits it via http.NewRequestWithContext(ctx, ...). The synthesized
/api/v1/workspaces request then runs through srv.ServeHTTP carrying
the stale RouteCtxKey from /mcp — chi takes the short-circuit branch,
skips its rctx.Reset() + RoutePath = \"/api/v1/...\" setup, the route
table lookup runs against contaminated routing state, and the request
falls through to chi's default NotFound handler. The body of that
handler is the literal \"404 page not found\\n\" the user reported.

## Why tests passed pre-fix

Existing dispatcher tests called Dispatch with context.Background()
— no chi RouteCtxKey to inherit, no contamination. The bug was
specific to the production path where requests enter via the chi-
mounted /mcp endpoint.

## Fix

In buildHTTPRequest (the central path EVERY synthesized request
flows through — main writes, RMW prefetches, bulk-update PATCHes,
link-create POSTs), shadow chi.RouteCtxKey with a typed nil before
constructing the new request:

    ctx = context.WithValue(ctx, chi.RouteCtxKey, (*chi.Context)(nil))

chi's Value(RouteCtxKey).(*Context) on a typed-nil returns
(nil, false), the `rctx != nil` check fails, and chi takes the
fresh-routing branch as intended.

We deliberately do NOT strip pad's own context values
(WithCurrentUser, WithAPITokenAuth, TokenScopes,
TokenAllowedWorkspaces) — those carry the authenticated user
identity and OAuth scope/allow-list state the synthesized request
needs. Only the chi-specific routing key is stripped.

## Tests

Two added (both fail without the fix, pass with it — verified via
git stash negative-control):

  - TestHTTPHandlerDispatcher_StripsChiRouteCtx_ProductionPath:
    full integration shape — chi router with /mcp route whose
    handler invokes the dispatcher, which synthesizes a
    /api/v1/workspaces request that MUST reach the workspace
    handler. Pre-fix returns 405 Method Not Allowed (chi remembers
    /mcp's registered methods). Post-fix returns 200 with the
    workspace data round-tripped.

  - TestBuildHTTPRequest_StripsChiRouteCtx: unit-level pin on the
    strip itself — feeds buildHTTPRequest a context carrying a
    non-nil chi RouteCtx, asserts the resulting request's context
    type-asserts to nil at chi.RouteCtxKey.

The integration test also pins (\"test setup\") that the inbound
context DOES carry a RouteCtxKey under chi v5.2.5 — if chi ever
changes that semantic the test fails loudly rather than silently
passing for the wrong reason.

## Credit

Found by Codex under /codex ask after my own initial trailing-slash
hypothesis was empirically disproved.

internal/mcp/dispatch_http.go

@@ -11,6 +11,7 @@ import (
 	"net/url"
 	"strings"
 
+	"github.com/go-chi/chi/v5"
 	"github.com/mark3labs/mcp-go/mcp"
 
 	"github.com/PerpetualSoftware/pad/internal/collections"
@@ -442,6 +443,49 @@ func (d *HTTPHandlerDispatcher) buildAuthedRequest(
 // chain treats the call as authenticated. Pulled out so tests can
 // inspect / decorate it cheaply.
 func buildHTTPRequest(ctx context.Context, method, urlPath string, body []byte, user *models.User) (*http.Request, error) {
+	// Strip any inherited chi.RouteCtxKey from the inbound context
+	// before synthesizing the new request. Without this, every
+	// production /mcp tool call 404s on the synthesized /api/v1/...
+	// request because chi's Mux.ServeHTTP short-circuits when it
+	// detects an existing RouteCtxKey:
+	//
+	//   // chi/v5/mux.go:71-75
+	//   rctx, _ := r.Context().Value(RouteCtxKey).(*Context)
+	//   if rctx != nil {
+	//       mx.handler.ServeHTTP(w, r)  // bypass fresh routing
+	//       return
+	//   }
+	//
+	// chi assumes "if there's already a route context, I'm being
+	// invoked as a sub-router from a parent — don't reset state."
+	// That's correct for chi's own Sub() / Mount() patterns, but
+	// here we're synthesizing a brand-new request that needs to
+	// route from scratch against the ROOT mux. The stale RouteCtxKey
+	// from the inbound /mcp request causes chi to skip its
+	// rctx.Reset() + RoutePath = "/api/v1/..." setup; the route
+	// table lookup runs against contaminated routing state and
+	// falls through to chi's default NotFound handler — whose body
+	// is the literal "404 page not found\n" the production user
+	// reported on every dispatcher call.
+	//
+	// In tests this never fired because Dispatch was always called
+	// with context.Background() (no RouteCtxKey to inherit). In
+	// production every call enters via /mcp's chi-routed handler,
+	// so the contamination is universal.
+	//
+	// Setting the value to a typed nil shadows the parent's value:
+	// chi's `.(*Context)` type assertion on a context.Value of nil
+	// returns (nil, false), the `rctx != nil` check fails, and
+	// chi takes the fresh-routing branch as intended.
+	//
+	// Critically we DON'T strip pad's own context values
+	// (WithCurrentUser, WithAPITokenAuth, TokenScopes,
+	// TokenAllowedWorkspaces) — those are added below / preserved
+	// from the inbound request and are exactly what the synthesized
+	// request needs to authenticate as the same user. We only strip
+	// the chi-specific routing key.
+	ctx = context.WithValue(ctx, chi.RouteCtxKey, (*chi.Context)(nil))
+
 	var bodyReader io.Reader
 	if len(body) > 0 {
 		bodyReader = bytes.NewReader(body)

internal/mcp/dispatch_http_chi_route_ctx_test.go

@@ -0,0 +1,144 @@
+package mcp
+
+// Regression test for the bug Codex review surfaced (TASK-1075):
+// every production /mcp tool call returned 404 from the dispatcher's
+// synthesized /api/v1/... request because chi short-circuits routing
+// when the context already carries a RouteCtxKey from the parent
+// /mcp request. Pre-fix this only manifested in production (real
+// chi-routed traffic); existing tests passed because they used
+// context.Background().
+//
+// This test pins the production-shaped path: a chi router with a
+// /mcp route whose handler invokes the dispatcher, which in turn
+// synthesizes a /api/v1/workspaces request that MUST reach the
+// workspace-list handler and not get short-circuited to chi's
+// default NotFound.
+
+import (
+	"context"
+	"encoding/json"
+	"net/http"
+	"net/http/httptest"
+	"strings"
+	"testing"
+
+	"github.com/go-chi/chi/v5"
+
+	"github.com/PerpetualSoftware/pad/internal/models"
+)
+
+// TestHTTPHandlerDispatcher_StripsChiRouteCtx_ProductionPath wires a
+// chi router with the same shape pad uses in production (a /mcp route
+// at the top level + an /api/v1/workspaces handler the dispatcher
+// would target), then drives traffic through /mcp's handler so the
+// dispatcher inherits the chi route context. Pre-fix this test fails
+// with a 404. Post-fix it succeeds.
+func TestHTTPHandlerDispatcher_StripsChiRouteCtx_ProductionPath(t *testing.T) {
+	// What we're proving was reached. Set true ONLY by the
+	// /api/v1/workspaces handler — if the bug regresses, this stays
+	// false and the test reports the failure mode (the dispatcher
+	// returned an error envelope because chi 404'd).
+	apiHit := false
+
+	// Build a chi router that mirrors the production shape: a /mcp
+	// endpoint at the top + an /api/v1/workspaces endpoint the
+	// dispatcher synthesizes a request for. We construct the
+	// dispatcher inside the /mcp handler so it sees the same chi-
+	// contaminated context production sees.
+	root := chi.NewRouter()
+
+	root.Get("/api/v1/workspaces", func(w http.ResponseWriter, r *http.Request) {
+		apiHit = true
+		w.Header().Set("Content-Type", "application/json")
+		_, _ = w.Write([]byte(`[{"slug":"test","name":"Test"}]`))
+	})
+
+	root.Post("/mcp", func(w http.ResponseWriter, r *http.Request) {
+		// Sanity: the inbound request DOES carry a chi RouteCtxKey,
+		// matching the production scenario. If chi ever changes this
+		// behavior the test would silently start passing for the wrong
+		// reason; pin it.
+		if r.Context().Value(chi.RouteCtxKey) == nil {
+			t.Fatal("test setup: expected chi.RouteCtxKey in /mcp handler context — chi may have changed routing semantics")
+		}
+
+		// Build the dispatcher here so it points at the same root
+		// router (mimicking production where srv is both the chi root
+		// AND the dispatcher's Handler).
+		d := &HTTPHandlerDispatcher{
+			Handler:      root,
+			UserResolver: fixedUserResolver(&models.User{ID: "u-1", Name: "Tester"}),
+		}
+
+		// Drive Dispatch with the inbound request's context — this is
+		// exactly what mcp-go does when invoking a tool handler.
+		ctx := WithDispatchInput(r.Context(), map[string]any{})
+		res, err := d.Dispatch(ctx, []string{"workspace", "list"}, nil)
+		if err != nil {
+			t.Errorf("dispatch error: %v", err)
+		}
+		if res == nil {
+			t.Fatal("nil result from dispatcher")
+		}
+		if res.IsError {
+			dumped, _ := json.Marshal(res)
+			t.Errorf("dispatch returned error envelope (chi 404'd?); full=%s", string(dumped))
+		}
+		// Echo dispatcher result back so the outer test can sanity-check
+		// the body round-tripped (proves we hit the real handler, not
+		// a stub upstream).
+		w.Header().Set("Content-Type", "application/json")
+		dumped, _ := json.Marshal(res)
+		_, _ = w.Write(dumped)
+	})
+
+	// Drive the production-shaped path: external /mcp POST.
+	req := httptest.NewRequest("POST", "/mcp", strings.NewReader("{}"))
+	rec := httptest.NewRecorder()
+	root.ServeHTTP(rec, req)
+
+	if !apiHit {
+		t.Fatalf("api/v1/workspaces handler was NOT reached — chi route ctx contamination likely. /mcp response body: %s", rec.Body.String())
+	}
+	if rec.Code != http.StatusOK {
+		t.Errorf("/mcp returned %d; body=%s", rec.Code, rec.Body.String())
+	}
+	if !strings.Contains(rec.Body.String(), "Test") {
+		t.Errorf("expected workspace name to round-trip through dispatcher; body=%s", rec.Body.String())
+	}
+}
+
+// TestBuildHTTPRequest_StripsChiRouteCtx is the unit-level pin for the
+// strip itself: feed a context carrying a chi RouteCtxKey, confirm the
+// resulting request's context returns nil for that key. Cheaper to run
+// than the integration test above and pinpoints exactly where the
+// strip happens if regression hits.
+func TestBuildHTTPRequest_StripsChiRouteCtx(t *testing.T) {
+	// Seed a parent context with a chi RouteCtx (non-nil — matches
+	// what chi's Mux.ServeHTTP attaches before invoking handlers).
+	parentRctx := chi.NewRouteContext()
+	parentRctx.RoutePath = "/mcp" // mimic production state
+	parent := context.WithValue(context.Background(), chi.RouteCtxKey, parentRctx)
+
+	req, err := buildHTTPRequest(parent, "GET", "/api/v1/workspaces", nil, &models.User{ID: "u"})
+	if err != nil {
+		t.Fatalf("buildHTTPRequest: %v", err)
+	}
+
+	// chi's check at mux.go:71 type-asserts to (*chi.Context). When the
+	// strip works correctly, that assertion against our typed-nil
+	// returns (nil, false) and chi falls through to fresh routing.
+	got, ok := req.Context().Value(chi.RouteCtxKey).(*chi.Context)
+	if got != nil {
+		t.Errorf("request context still carries non-nil chi RouteCtx after strip; got=%+v ok=%v", got, ok)
+	}
+	// And the value lookup itself should yield a nil interface (or
+	// typed-nil) — NOT the parent's non-nil RouteCtx.
+	if rawValue := req.Context().Value(chi.RouteCtxKey); rawValue != nil {
+		// typed-nil interface != nil; check via the assertion path
+		// that chi actually uses.
+		if rctx, _ := rawValue.(*chi.Context); rctx != nil {
+			t.Errorf("expected typed-nil after strip; got non-nil RouteCtx %+v", rctx)
+		}
+	}
+}