diff --git a/cmd/routing/main.go b/cmd/routing/main.go index 2ece8bc..a93160e 100644 --- a/cmd/routing/main.go +++ b/cmd/routing/main.go @@ -103,12 +103,17 @@ func main() { })) if cfg.GiteaMCPURL != "" { + mcpC, err := mcpclient.New(cfg.GiteaMCPURL, cfg.GiteaMCPToken) + if err != nil { + logger.Error("mcpclient init for project_create — GITEA_MCP_URL is set but GITEA_MCP_TOKEN is empty (check routing-secrets)", "err", err) + os.Exit(1) + } var ghClient *githubclient.Client if cfg.GitHubPAT != "" { ghClient = githubclient.New(cfg.GitHubPAT) } reg.Register(project.New(project.Config{ - Client: mcpclient.New(cfg.GiteaMCPURL, cfg.GiteaMCPToken), + Client: mcpC, GitHub: ghClient, GiteaOwner: cfg.GiteaOwner, GitHubOwner: cfg.GitHubOwner, diff --git a/internal/mcpclient/client.go b/internal/mcpclient/client.go index 135da15..1f0c7f4 100644 --- a/internal/mcpclient/client.go +++ b/internal/mcpclient/client.go @@ -7,6 +7,7 @@ import ( "bytes" "context" "encoding/json" + "errors" "fmt" "io" "net/http" @@ -20,13 +21,27 @@ type Client struct { http *http.Client } -// New returns a Client. token may be empty for unauthenticated servers. -func New(url, token string) *Client { +// ErrTokenRequired is returned by New when token is empty. Empty token +// causes mcpclient to omit the Authorization header at request time, +// which is silently misread as 401 by bearer-auth servers — see +// hyperguild #13 and the brain entry on the failure mode. +var ErrTokenRequired = errors.New("mcpclient: token required") + +// New returns a Client. Returns ErrTokenRequired when token is empty: +// every MCP server we talk to today is bearer-protected, and an empty +// token is always a configuration bug (typically a Kubernetes Secret +// missing the expected key, see hyperguild #13). Callers that genuinely +// need an unauthenticated client should construct &Client{} directly in +// tests, not call New. +func New(url, token string) (*Client, error) { + if token == "" { + return nil, ErrTokenRequired + } return &Client{ url: url, token: token, http: &http.Client{Timeout: 60 * time.Second}, - } + }, nil } // WithHTTPClient overrides the underlying HTTP client (test injection). diff --git a/internal/mcpclient/client_test.go b/internal/mcpclient/client_test.go index 2302fc4..ea0ae40 100644 --- a/internal/mcpclient/client_test.go +++ b/internal/mcpclient/client_test.go @@ -14,6 +14,13 @@ import ( "github.com/stretchr/testify/require" ) +func TestNew_EmptyTokenFailsFast(t *testing.T) { + c, err := mcpclient.New("http://example.invalid", "") + require.Error(t, err) + require.Nil(t, c) + require.ErrorIs(t, err, mcpclient.ErrTokenRequired) +} + func TestCallTool_Success(t *testing.T) { srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { assert.Equal(t, http.MethodPost, r.Method) @@ -30,13 +37,13 @@ func TestCallTool_Success(t *testing.T) { })) defer srv.Close() - c := mcpclient.New(srv.URL, "tok") + c, err := mcpclient.New(srv.URL, "tok") + require.NoError(t, err) var out struct { OK bool `json:"ok"` N int `json:"n"` } - err := c.CallTool(context.Background(), "x_y", map[string]any{"a": 1}, &out) - require.NoError(t, err) + require.NoError(t, c.CallTool(context.Background(), "x_y", map[string]any{"a": 1}, &out)) assert.True(t, out.OK) assert.Equal(t, 7, out.N) } @@ -48,8 +55,9 @@ func TestCallTool_RPCError(t *testing.T) { })) defer srv.Close() - c := mcpclient.New(srv.URL, "") - err := c.CallTool(context.Background(), "x", nil, nil) + c, err := mcpclient.New(srv.URL, "test") + require.NoError(t, err) + err = c.CallTool(context.Background(), "x", nil, nil) require.Error(t, err) var me *mcpclient.Error require.True(t, errors.As(err, &me)) @@ -64,8 +72,9 @@ func TestCallTool_HTTPError(t *testing.T) { })) defer srv.Close() - c := mcpclient.New(srv.URL, "") - err := c.CallTool(context.Background(), "x", nil, nil) + c, err := mcpclient.New(srv.URL, "test") + require.NoError(t, err) + err = c.CallTool(context.Background(), "x", nil, nil) require.Error(t, err) assert.Contains(t, err.Error(), "401") } @@ -77,6 +86,7 @@ func TestCallTool_NilResult(t *testing.T) { })) defer srv.Close() - c := mcpclient.New(srv.URL, "") + c, err := mcpclient.New(srv.URL, "test") + require.NoError(t, err) require.NoError(t, c.CallTool(context.Background(), "x", nil, nil)) } diff --git a/internal/skills/project/handlers_test.go b/internal/skills/project/handlers_test.go index e1de228..1999e5b 100644 --- a/internal/skills/project/handlers_test.go +++ b/internal/skills/project/handlers_test.go @@ -125,7 +125,7 @@ func newSkill(t *testing.T, f *fakeGiteaMCP) (*project.Skill, *fakeGitHub) { t.Cleanup(ghSrv.Close) return project.New(project.Config{ - Client: mcpclient.New(srv.URL, ""), + Client: mustClient(t, srv.URL), GitHub: githubclient.New("ghp_test").WithBaseURL(ghSrv.URL), GiteaOwner: "mathias", GitHubOwner: "mathiasb", @@ -141,13 +141,23 @@ func newSkillNoGitHub(t *testing.T, f *fakeGiteaMCP) *project.Skill { srv := httptest.NewServer(f.handler()) t.Cleanup(srv.Close) return project.New(project.Config{ - Client: mcpclient.New(srv.URL, ""), + Client: mustClient(t, srv.URL), GiteaOwner: "mathias", GitHubOwner: "mathiasb", InfraRepo: "infra", }) } +// mustClient builds an mcpclient against an httptest server. Uses a +// non-empty dummy token because httptest servers don't enforce bearer +// auth, but mcpclient.New now requires non-empty token (see #13). +func mustClient(t *testing.T, url string) *mcpclient.Client { + t.Helper() + c, err := mcpclient.New(url, "test-token") + require.NoError(t, err) + return c +} + func happyArgs() json.RawMessage { return json.RawMessage(`{ "name":"my-experiment",