From 7e8f02a1af2e83221092cc698e0ae52587bf586b Mon Sep 17 00:00:00 2001 From: Kyle Felter Date: Wed, 27 May 2026 13:40:50 -0500 Subject: [PATCH 1/7] feat: Add nico-mcp server mode to nicocli Signed-off-by: Kyle Felter --- AGENTS.md | 2 +- cli/README.md | 59 ++++++ cli/cmd/cli/main.go | 2 + cli/mcp/cmd.go | 198 ++++++++++++++++++ cli/mcp/config.go | 184 +++++++++++++++++ cli/mcp/config_test.go | 295 +++++++++++++++++++++++++++ cli/mcp/schema.go | 113 +++++++++++ cli/mcp/schema_test.go | 123 +++++++++++ cli/mcp/server.go | 226 +++++++++++++++++++++ cli/mcp/server_test.go | 193 ++++++++++++++++++ cli/mcp/transport_test.go | 416 ++++++++++++++++++++++++++++++++++++++ go.mod | 3 + go.sum | 6 + 13 files changed, 1819 insertions(+), 1 deletion(-) create mode 100644 cli/mcp/cmd.go create mode 100644 cli/mcp/config.go create mode 100644 cli/mcp/config_test.go create mode 100644 cli/mcp/schema.go create mode 100644 cli/mcp/schema_test.go create mode 100644 cli/mcp/server.go create mode 100644 cli/mcp/server_test.go create mode 100644 cli/mcp/transport_test.go diff --git a/AGENTS.md b/AGENTS.md index 1bd638a9c..cea598708 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -32,7 +32,7 @@ infra-controller-rest/ ├── api/ # Main REST API server (Echo-based) ├── auth/ # Authentication (Keycloak, JWT, service accounts) ├── cert-manager/ # Native PKI certificate management (credsmgr) -├── cli/ # CLI client (nicocli) with TUI +├── cli/ # CLI client (nicocli) with TUI and MCP server mode ├── common/ # Shared utilities and configuration ├── db/ # Database layer (Bun ORM, pgx, migrations) ├── deploy/ # Kubernetes deployment (Kind, Kustomize, Helm) diff --git a/cli/README.md b/cli/README.md index 6f2589196..9422998dd 100644 --- a/cli/README.md +++ b/cli/README.md @@ -333,6 +333,65 @@ To skip the config selector and connect to a specific environment directly: nicocli --config ~/.nico/config.prod.yaml tui ``` +## MCP Server Mode + +`nicocli mcp serve` exposes the NICo REST read surface (every `GET` operation in the embedded OpenAPI spec) as Model Context Protocol tools over streamable-HTTP. It is intended to run behind an MCP-aware gateway such as the [Latinum Agent Gateway](https://agentgateway.dev), but works standalone for local development. + +```bash +nicocli mcp serve \ + --listen :8080 \ + --path /mcp \ + --base-url https://nico.example.com \ + --org tester +``` + +### Properties + +- **Read-only.** Only `GET` operations are exposed. Mutating routes (`POST`, `PATCH`, `PUT`, `DELETE`) are intentionally excluded. +- **Tool naming.** Tools are named `nico_` (e.g. `nico_get_all_site`, `nico_validate_rack`). +- **Stateless and request/response only.** The server sets `Stateless: true` and `JSONResponse: true` on the MCP streamable-HTTP handler -- responses are always `Content-Type: application/json`, never `text/event-stream`, and the server retains no per-session state. This is compatible with deployment behind a NATS-bridged shard proxy. +- **JWT passthrough.** The `Authorization: Bearer ` header on the inbound MCP request is forwarded unchanged to NICo REST. NICo REST validates the JWT, resolves the caller org, and enforces role-based authorisation. The MCP layer never makes the authz decision itself. + +### Flags + +| Flag | Env Var | Description | +|------|---------|-------------| +| `--listen` | `NICO_MCP_LISTEN` | Listen address (default `:8080`) | +| `--path` | `NICO_MCP_PATH` | HTTP path prefix the MCP handler is mounted at (default `/mcp`) | +| `--shutdown-timeout` | `NICO_MCP_SHUTDOWN_TIMEOUT` | Graceful shutdown timeout (default `10s`) | + +`--base-url`, `--org`, `--api-name`, `--token`, and `--token-command` are inherited from the root command and provide the server-side defaults. Like every other `nicocli` command, the server loads `~/.nico/config.yaml` at startup; unset flags fall back to config values. + +### Per-call config overrides + +Every typical config value can also be passed as an argument on each MCP tool call, layered on top of the server defaults: + +| Tool arg | Equivalent flag | Config field | +|----------|-----------------|--------------| +| `org` | `--org` | `api.org` | +| `base_url` | `--base-url` | `api.base` | +| `api_name` | `--api-name` | `api.name` | +| `token` | `--token` | `auth.token` | + +Precedence per tool call (first non-empty wins): tool argument → inbound `Authorization` header (token only) → server startup flag → server-loaded config file → `token_command` refresh on a 401. `token_command`, OIDC credentials, and NGC api_key settings are NOT exposed as tool arguments -- they are login-flow inputs configured server-side. + +### Probing the server + +```bash +# List the tool catalogue +curl -sS http://localhost:8080/mcp \ + -H 'Content-Type: application/json' \ + -H 'Accept: application/json, text/event-stream' \ + -d '{"jsonrpc":"2.0","id":1,"method":"tools/list","params":{}}' | jq + +# Call a specific tool +curl -sS http://localhost:8080/mcp \ + -H 'Content-Type: application/json' \ + -H 'Accept: application/json, text/event-stream' \ + -H "Authorization: Bearer $TOKEN" \ + -d '{"jsonrpc":"2.0","id":2,"method":"tools/call","params":{"name":"nico_get_all_site","arguments":{}}}' | jq +``` + ## Troubleshooting If `nicocli` is not found after install, make sure `$(go env GOPATH)/bin` is in your PATH: diff --git a/cli/cmd/cli/main.go b/cli/cmd/cli/main.go index e90b720d2..a9b7b29d0 100644 --- a/cli/cmd/cli/main.go +++ b/cli/cmd/cli/main.go @@ -21,6 +21,7 @@ import ( "fmt" "os" + "github.com/NVIDIA/infra-controller-rest/cli/mcp" appcli "github.com/NVIDIA/infra-controller-rest/cli/pkg" "github.com/NVIDIA/infra-controller-rest/cli/tui" "github.com/NVIDIA/infra-controller-rest/openapi" @@ -41,6 +42,7 @@ func main() { return tui.RunTUI(c.String("config")) }, }) + app.Commands = append(app.Commands, mcp.Command(openapi.Spec)) if err := app.Run(os.Args); err != nil { fmt.Fprintf(os.Stderr, "Error: %v\n", err) os.Exit(1) diff --git a/cli/mcp/cmd.go b/cli/mcp/cmd.go new file mode 100644 index 000000000..af5708659 --- /dev/null +++ b/cli/mcp/cmd.go @@ -0,0 +1,198 @@ +/* + * SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. + * SPDX-License-Identifier: Apache-2.0 + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package mcp + +import ( + "context" + "errors" + "fmt" + "net/http" + "os" + "os/signal" + "syscall" + "time" + + "github.com/sirupsen/logrus" + urfave "github.com/urfave/cli/v2" + + appcli "github.com/NVIDIA/infra-controller-rest/cli/pkg" +) + +// Command returns the "mcp" urfave/cli command tree for nicocli. Wire +// it into the binary's command list from cmd/cli/main.go alongside the +// dynamically-generated commands the rest of the CLI ships with. +// +// specData is the OpenAPI YAML the rest of the CLI is built from; the +// command's "serve" action passes it to BuildServer so the MCP tool +// catalogue stays in lockstep with every nicocli build. +func Command(specData []byte) *urfave.Command { + return &urfave.Command{ + Name: "mcp", + Usage: "Run an MCP server that exposes the NICo REST read surface over streamable-HTTP", + Subcommands: []*urfave.Command{ + serveCommand(specData), + }, + } +} + +func serveCommand(specData []byte) *urfave.Command { + return &urfave.Command{ + Name: "serve", + Usage: "Start the streamable-HTTP MCP server", + Description: "Serves the NICo REST read surface as MCP tools at /mcp on the\n" + + "configured listen address. The server is stateless and never emits\n" + + "text/event-stream responses; every tool/call returns a single JSON\n" + + "body. In production, place an MCP-aware gateway in front and rely on\n" + + "the inbound Authorization header for per-call authentication.", + Flags: []urfave.Flag{ + &urfave.StringFlag{ + Name: "listen", + Usage: "address:port to listen on", + EnvVars: []string{"NICO_MCP_LISTEN"}, + Value: ":8080", + }, + &urfave.StringFlag{ + Name: "path", + Usage: "HTTP path prefix the MCP handler is mounted at", + EnvVars: []string{"NICO_MCP_PATH"}, + Value: "/mcp", + }, + &urfave.DurationFlag{ + Name: "shutdown-timeout", + Usage: "graceful shutdown timeout when SIGINT/SIGTERM arrives", + EnvVars: []string{"NICO_MCP_SHUTDOWN_TIMEOUT"}, + Value: 10 * time.Second, + }, + }, + Action: func(c *urfave.Context) error { + return runServe(c, specData) + }, + } +} + +// runServe wires the urfave context into Options, builds the MCP server, +// and runs an http.Server until SIGINT/SIGTERM. It is split out from the +// urfave Action closure so tests can drive it directly. +func runServe(c *urfave.Context, specData []byte) error { + opts, err := buildServeOptions(c) + if err != nil { + return err + } + + server, err := BuildServer(specData, opts) + if err != nil { + return fmt.Errorf("building MCP server: %w", err) + } + + listen := c.String("listen") + path := c.String("path") + shutdownTimeout := c.Duration("shutdown-timeout") + + mux := http.NewServeMux() + mux.Handle(path, NewHandler(server)) + + httpServer := &http.Server{ + Addr: listen, + Handler: mux, + ReadHeaderTimeout: 10 * time.Second, + } + + opts.Log.Infof("nico-mcp: listening on %s, MCP at %s (stateless, JSONResponse)", listen, path) + + errCh := make(chan error, 1) + go func() { + errCh <- httpServer.ListenAndServe() + }() + + sigCh := make(chan os.Signal, 1) + signal.Notify(sigCh, syscall.SIGINT, syscall.SIGTERM) + + select { + case err := <-errCh: + if err != nil && !errors.Is(err, http.ErrServerClosed) { + return fmt.Errorf("http server: %w", err) + } + return nil + case sig := <-sigCh: + opts.Log.Infof("nico-mcp: received %s, shutting down", sig) + ctx, cancel := context.WithTimeout(context.Background(), shutdownTimeout) + defer cancel() + if err := httpServer.Shutdown(ctx); err != nil { + return fmt.Errorf("graceful shutdown: %w", err) + } + return nil + } +} + +// buildServeOptions resolves the urfave context into Options by layering +// app-global flags on top of the loaded config file, mirroring what the +// dynamically-generated commands do via clientFromContext. +func buildServeOptions(c *urfave.Context) (Options, error) { + cfg, _ := appcli.LoadConfig() + appcli.ApplyEnvOverrides(cfg) + + baseURL := cfg.API.Base + if c.IsSet("base-url") { + baseURL = c.String("base-url") + } + if baseURL == "" { + baseURL = c.String("base-url") + } + + org := cfg.API.Org + if c.IsSet("org") { + org = c.String("org") + } + + apiName := cfg.API.Name + if c.IsSet("api-name") { + apiName = c.String("api-name") + } + if apiName == "" { + apiName = "nico" + } + + token := "" + if c.IsSet("token") { + token = c.String("token") + } else if cfg.Auth.Token != "" { + token = cfg.Auth.Token + } + + tokenCommand := "" + if c.IsSet("token-command") { + tokenCommand = c.String("token-command") + } else if cfg.Auth.TokenCommand != "" { + tokenCommand = cfg.Auth.TokenCommand + } + + log := logrus.NewEntry(logrus.StandardLogger()) + if c.Bool("debug") { + log.Logger.SetLevel(logrus.DebugLevel) + } + + return Options{ + BaseURL: baseURL, + Org: org, + APIName: apiName, + Token: token, + TokenCommand: tokenCommand, + Debug: c.Bool("debug"), + Log: log, + }, nil +} diff --git a/cli/mcp/config.go b/cli/mcp/config.go new file mode 100644 index 000000000..c49637600 --- /dev/null +++ b/cli/mcp/config.go @@ -0,0 +1,184 @@ +/* + * SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. + * SPDX-License-Identifier: Apache-2.0 + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package mcp + +import ( + "fmt" + "strings" + + "github.com/modelcontextprotocol/go-sdk/mcp" + "github.com/sirupsen/logrus" + + appcli "github.com/NVIDIA/infra-controller-rest/cli/pkg" +) + +// Options carry the server-side defaults that every tool invocation +// starts from. Individual tool calls override these per request through +// resolveCallConfig. +// +// Token and TokenCommand are mutually compatible: Token is the static +// fallback, TokenCommand is the dynamic refresh hook fired on a 401 from +// NICo REST. The server never persists tokens back to disk -- the +// refresh hook wires to appcli.ExecuteTokenCommand, not the login +// flow's write-through helper LoginWithTokenCommand. +type Options struct { + // BaseURL is the NICo REST base URL (e.g. https://nico.example.com). + BaseURL string + // Org is the default organisation used in /v2/org//... paths. + Org string + // APIName is the API path segment between org and resource (default + // "nico", overridable via api.name in config). + APIName string + // Token is the static bearer used when no inbound bearer or tool + // arg token is provided. + Token string + // TokenCommand, if non-empty, is a shell command that prints a + // fresh bearer to stdout. Used as a 401-refresh hook only; never + // pre-fetched and never persisted. + TokenCommand string + // Debug enables logrus debug-level HTTP request/response logging + // through to the appcli.Client. + Debug bool + // Log is the logrus entry used for client request/response logging. + // If nil, a default entry wrapping the standard logger is used. + Log *logrus.Entry +} + +// withDefaults returns a copy of opts with empty optional fields filled +// in with package defaults. APIName falls back to "nico" and Log to +// logrus.StandardLogger() so callers can leave them unset. +func (o Options) withDefaults() Options { + if o.APIName == "" { + o.APIName = "nico" + } + if o.Log == nil { + o.Log = logrus.NewEntry(logrus.StandardLogger()) + } + return o +} + +// resolvedConfig is the result of merging Options with the per-call +// overrides for one tool invocation. It is consumed by registerGET to +// construct a fresh appcli.Client. +type resolvedConfig struct { + BaseURL string + Org string + APIName string + Token string + TokenRefresh func() (string, error) +} + +// resolveCallConfig implements the precedence chain documented in the +// design plan: +// +// 1. Tool-call argument (org, base_url, api_name, token) +// 2. Inbound Authorization header (token only) +// 3. Server startup flag / Options (BaseURL, Org, APIName, Token) +// 4. token_command (token only; wired as TokenRefresh, fires on 401) +// +// The function returns an error when a required field (org, base_url) +// ends up empty so the tool handler can surface a JSON-RPC error +// instead of letting the call go out with an invalid URL. +func resolveCallConfig(in map[string]any, req *mcp.CallToolRequest, opts Options) (resolvedConfig, error) { + cfg := resolvedConfig{ + BaseURL: pickString(stringArg(in, "base_url"), opts.BaseURL), + Org: pickString(stringArg(in, "org"), opts.Org), + APIName: pickString(stringArg(in, "api_name"), opts.APIName), + } + + cfg.Token = pickString( + stringArg(in, "token"), + bearerFromExtra(req), + opts.Token, + ) + + if opts.TokenCommand != "" { + tokenCommand := opts.TokenCommand + cfg.TokenRefresh = func() (string, error) { + return appcli.ExecuteTokenCommand(tokenCommand) + } + } + + if err := cfg.requireNonEmpty(); err != nil { + return resolvedConfig{}, err + } + return cfg, nil +} + +// requireNonEmpty returns a descriptive error when org or BaseURL are +// blank. Token can be empty -- NICo REST will reject the request with +// 401 and the response surfaces to the caller as an MCP error result; +// that path is exercised by the bearer-passthrough integration test. +func (c resolvedConfig) requireNonEmpty() error { + missing := []string{} + if c.Org == "" { + missing = append(missing, "org") + } + if c.BaseURL == "" { + missing = append(missing, "base_url") + } + if len(missing) == 0 { + return nil + } + return fmt.Errorf("missing required config value(s): %s; pass via tool args, server flags, or %s", + strings.Join(missing, ", "), appcli.ConfigPath()) +} + +func stringArg(in map[string]any, key string) string { + if in == nil { + return "" + } + v, ok := in[key] + if !ok { + return "" + } + s, ok := v.(string) + if !ok { + return "" + } + return strings.TrimSpace(s) +} + +func pickString(values ...string) string { + for _, v := range values { + if v != "" { + return v + } + } + return "" +} + +// bearerFromExtra extracts the bearer from a *mcp.CallToolRequest's +// inbound HTTP headers. The streamable-HTTP handler stamps every JSON-RPC +// request with req.Extra.Header from the HTTP request. Returns the bare +// token without the "Bearer " prefix; returns "" for any value the SDK +// did not stash or that does not look like a bearer. +func bearerFromExtra(req *mcp.CallToolRequest) string { + if req == nil || req.Extra == nil || req.Extra.Header == nil { + return "" + } + auth := req.Extra.Header.Get("Authorization") + if auth == "" { + return "" + } + const prefix = "Bearer " + if len(auth) <= len(prefix) || !strings.EqualFold(auth[:len(prefix)], prefix) { + return "" + } + return strings.TrimSpace(auth[len(prefix):]) +} diff --git a/cli/mcp/config_test.go b/cli/mcp/config_test.go new file mode 100644 index 000000000..f3586137b --- /dev/null +++ b/cli/mcp/config_test.go @@ -0,0 +1,295 @@ +/* + * SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. + * SPDX-License-Identifier: Apache-2.0 + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package mcp + +import ( + "net/http" + "os" + "path/filepath" + "testing" + + "github.com/modelcontextprotocol/go-sdk/mcp" + "github.com/stretchr/testify/require" +) + +func TestResolveCallConfig_PrecedenceChain(t *testing.T) { + type expect struct { + baseURL, org, apiName, token string + hasRefresh bool + wantErr bool + } + cases := []struct { + name string + in map[string]any + req *mcp.CallToolRequest + opts Options + expected expect + }{ + { + name: "tool_args_win_every_field", + in: map[string]any{ + "base_url": "https://from-arg.example.com", + "org": "arg-org", + "api_name": "arg-name", + "token": "arg-token", + }, + req: requestWithBearer("inbound-bearer"), + opts: Options{BaseURL: "https://opts.example.com", Org: "opts-org", APIName: "opts-name", Token: "opts-token"}, + expected: expect{ + baseURL: "https://from-arg.example.com", + org: "arg-org", + apiName: "arg-name", + token: "arg-token", + }, + }, + { + name: "inbound_bearer_wins_when_no_token_arg", + in: map[string]any{}, + req: requestWithBearer("from-header"), + opts: Options{BaseURL: "https://opts.example.com", Org: "opts-org", APIName: "nico", Token: "opts-token"}, + expected: expect{ + baseURL: "https://opts.example.com", + org: "opts-org", + apiName: "nico", + token: "from-header", + }, + }, + { + name: "opts_token_used_when_no_arg_and_no_inbound", + in: map[string]any{}, + req: nil, + opts: Options{BaseURL: "https://opts.example.com", Org: "opts-org", APIName: "nico", Token: "opts-token"}, + expected: expect{ + baseURL: "https://opts.example.com", + org: "opts-org", + apiName: "nico", + token: "opts-token", + }, + }, + { + name: "token_command_wired_as_refresh_hook", + in: map[string]any{}, + req: nil, + opts: Options{ + BaseURL: "https://opts.example.com", + Org: "opts-org", + APIName: "nico", + TokenCommand: "echo refreshed-token", + }, + expected: expect{ + baseURL: "https://opts.example.com", + org: "opts-org", + apiName: "nico", + token: "", + hasRefresh: true, + }, + }, + { + name: "missing_org_errors", + in: map[string]any{}, + req: nil, + opts: Options{BaseURL: "https://opts.example.com"}, + expected: expect{ + wantErr: true, + }, + }, + { + name: "missing_base_url_errors", + in: map[string]any{}, + req: nil, + opts: Options{Org: "opts-org"}, + expected: expect{ + wantErr: true, + }, + }, + { + name: "missing_both_errors", + in: map[string]any{}, + req: nil, + opts: Options{}, + expected: expect{ + wantErr: true, + }, + }, + { + name: "api_name_defaults_when_unset", + in: map[string]any{}, + req: nil, + opts: Options{BaseURL: "https://opts.example.com", Org: "opts-org", Token: "t"}.withDefaults(), + expected: expect{ + baseURL: "https://opts.example.com", + org: "opts-org", + apiName: "nico", + token: "t", + }, + }, + { + name: "empty_string_arg_falls_through_to_opts", + in: map[string]any{ + "org": "", + "token": " ", + }, + req: requestWithBearer("inbound"), + opts: Options{ + BaseURL: "https://opts.example.com", + Org: "opts-org", + APIName: "nico", + Token: "opts-token", + }, + expected: expect{ + baseURL: "https://opts.example.com", + org: "opts-org", + apiName: "nico", + token: "inbound", + }, + }, + } + + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + cfg, err := resolveCallConfig(c.in, c.req, c.opts) + if c.expected.wantErr { + require.Error(t, err) + return + } + require.NoError(t, err) + require.Equal(t, c.expected.baseURL, cfg.BaseURL) + require.Equal(t, c.expected.org, cfg.Org) + require.Equal(t, c.expected.apiName, cfg.APIName) + require.Equal(t, c.expected.token, cfg.Token) + require.Equal(t, c.expected.hasRefresh, cfg.TokenRefresh != nil) + }) + } +} + +func TestResolveCallConfig_TokenRefreshIsPerCall(t *testing.T) { + // Sanity-check: the TokenRefresh closure is rebuilt on every call + // to resolveCallConfig. Two adjacent invocations produce two + // distinct closures, so a refresh on call A cannot leak state into + // call B. + opts := Options{ + BaseURL: "https://opts.example.com", + Org: "opts-org", + TokenCommand: "echo t", + } + cfgA, err := resolveCallConfig(nil, nil, opts) + require.NoError(t, err) + cfgB, err := resolveCallConfig(nil, nil, opts) + require.NoError(t, err) + require.NotNil(t, cfgA.TokenRefresh) + require.NotNil(t, cfgB.TokenRefresh) + // Closures themselves are distinct values. + require.NotSame(t, &cfgA.TokenRefresh, &cfgB.TokenRefresh) +} + +func TestStringArg(t *testing.T) { + in := map[string]any{ + "a": "hello", + "b": " hello ", + "c": 42, + "d": nil, + "e": "", + } + require.Equal(t, "hello", stringArg(in, "a")) + require.Equal(t, "hello", stringArg(in, "b")) + require.Equal(t, "", stringArg(in, "c")) + require.Equal(t, "", stringArg(in, "d")) + require.Equal(t, "", stringArg(in, "e")) + require.Equal(t, "", stringArg(in, "missing")) + require.Equal(t, "", stringArg(nil, "any")) +} + +func TestPickString(t *testing.T) { + require.Equal(t, "a", pickString("a", "b", "c")) + require.Equal(t, "b", pickString("", "b", "c")) + require.Equal(t, "c", pickString("", "", "c")) + require.Equal(t, "", pickString("", "", "")) + require.Equal(t, "", pickString()) +} + +func TestBearerFromExtra(t *testing.T) { + cases := []struct { + name string + hdr http.Header + want string + }{ + {"nil_req", nil, ""}, + {"empty_header", http.Header{}, ""}, + {"bearer", http.Header{"Authorization": []string{"Bearer abc.def"}}, "abc.def"}, + {"bearer_lowercase_scheme", http.Header{"Authorization": []string{"bearer abc.def"}}, "abc.def"}, + {"bearer_with_padding", http.Header{"Authorization": []string{"Bearer spaced "}}, "spaced"}, + {"non_bearer_basic", http.Header{"Authorization": []string{"Basic dXNlcjpwYXNz"}}, ""}, + {"empty_value", http.Header{"Authorization": []string{""}}, ""}, + {"bearer_alone", http.Header{"Authorization": []string{"Bearer "}}, ""}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + var req *mcp.CallToolRequest + if c.hdr != nil { + req = &mcp.CallToolRequest{Extra: &mcp.RequestExtra{Header: c.hdr}} + } + require.Equal(t, c.want, bearerFromExtra(req)) + }) + } +} + +// TestOptions_WithDefaults_PreservesCustomLogger asserts that +// withDefaults does not clobber a caller-supplied Log entry. +func TestOptions_WithDefaults(t *testing.T) { + o := Options{}.withDefaults() + require.Equal(t, "nico", o.APIName) + require.NotNil(t, o.Log) +} + +// TestNoConfigWriteBack runs the runServe-relevant config helpers (just +// LoadConfig + ApplyEnvOverrides + the package's TokenRefresh closure) +// against a temp config file and verifies the file on disk is byte- +// identical before and after. This is the design guarantee from the +// "Statelessness invariants" section: the MCP server never writes back +// to the on-disk config, even when a token_command refresh fires. +func TestNoConfigWriteBack(t *testing.T) { + tmpDir := t.TempDir() + cfgPath := filepath.Join(tmpDir, "config.yaml") + original := []byte("api:\n base: https://example.test\n org: tester\nauth:\n token_command: echo new-token\n") + require.NoError(t, os.WriteFile(cfgPath, original, 0o600)) + + opts := Options{ + BaseURL: "https://example.test", + Org: "tester", + APIName: "nico", + TokenCommand: "echo new-token", + } + cfg, err := resolveCallConfig(nil, nil, opts) + require.NoError(t, err) + require.NotNil(t, cfg.TokenRefresh) + token, err := cfg.TokenRefresh() + require.NoError(t, err) + require.Equal(t, "new-token", token) + + after, err := os.ReadFile(cfgPath) + require.NoError(t, err) + require.Equal(t, original, after, "TokenRefresh must not rewrite the on-disk config file") +} + +func requestWithBearer(token string) *mcp.CallToolRequest { + return &mcp.CallToolRequest{ + Extra: &mcp.RequestExtra{ + Header: http.Header{"Authorization": []string{"Bearer " + token}}, + }, + } +} diff --git a/cli/mcp/schema.go b/cli/mcp/schema.go new file mode 100644 index 000000000..fe8262a56 --- /dev/null +++ b/cli/mcp/schema.go @@ -0,0 +1,113 @@ +/* + * SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. + * SPDX-License-Identifier: Apache-2.0 + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package mcp + +import ( + "sort" + + "github.com/google/jsonschema-go/jsonschema" + + appcli "github.com/NVIDIA/infra-controller-rest/cli/pkg" +) + +// commonConfigDescriptions documents the four per-call config overrides +// that are merged into every tool's input schema. Kept as a slice (not +// a map) so the schema render order is stable. +var commonConfigDescriptions = []struct { + Name string + Desc string +}{ + {"org", "Override the server's default org (api.org) for this call. If unset, the inbound bearer's claims plus server-side config decide the org."}, + {"base_url", "Override the server's default base URL (api.base) for this call. Useful when one MCP server fronts multiple NICo REST deployments."}, + {"api_name", "Override the API path segment used in /v2/org///... (api.name; default \"nico\")."}, + {"token", "Bearer token for this call. Overrides the inbound Authorization header. Omit in production behind agentgateway; the gateway-injected JWT is passed through automatically."}, +} + +// buildInputSchema produces a JSON Schema describing a tool's input: +// OpenAPI path and query parameters merged with the four common config +// override fields (org, base_url, api_name, token). Path parameters are +// marked required; OpenAPI-required query parameters are marked +// required; the config overrides are always optional. +func buildInputSchema(item appcli.PathItem, op *appcli.Operation) *jsonschema.Schema { + props := map[string]*jsonschema.Schema{} + var required []string + + allParams := append([]appcli.Parameter{}, item.Parameters...) + allParams = append(allParams, op.Parameters...) + for _, p := range allParams { + if p.Name == "org" { + // Resolved from the config layer (org override or server + // default). The OpenAPI {org} segment is filled in by + // appcli.Client.Do. + continue + } + if p.In != "path" && p.In != "query" { + continue + } + props[p.Name] = paramToJSONSchema(p) + if p.In == "path" || p.Required { + required = append(required, p.Name) + } + } + + for _, c := range commonConfigDescriptions { + if _, exists := props[c.Name]; exists { + continue + } + props[c.Name] = &jsonschema.Schema{ + Type: "string", + Description: c.Desc, + } + } + + sort.Strings(required) + return &jsonschema.Schema{ + Type: "object", + Properties: props, + Required: required, + } +} + +// paramToJSONSchema converts a single OpenAPI parameter to a JSON +// schema fragment. Types are normalised to integer/boolean/number/ +// string; everything else falls back to string. Enums are preserved +// where present. +func paramToJSONSchema(p appcli.Parameter) *jsonschema.Schema { + s := &jsonschema.Schema{Description: p.Description} + if p.Schema == nil { + s.Type = "string" + return s + } + switch p.Schema.Type { + case "integer": + s.Type = "integer" + case "boolean": + s.Type = "boolean" + case "number": + s.Type = "number" + default: + s.Type = "string" + } + if len(p.Schema.Enum) > 0 { + s.Enum = make([]any, 0, len(p.Schema.Enum)) + for _, e := range p.Schema.Enum { + s.Enum = append(s.Enum, e) + } + } + return s +} diff --git a/cli/mcp/schema_test.go b/cli/mcp/schema_test.go new file mode 100644 index 000000000..8937f9216 --- /dev/null +++ b/cli/mcp/schema_test.go @@ -0,0 +1,123 @@ +/* + * SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. + * SPDX-License-Identifier: Apache-2.0 + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package mcp + +import ( + "testing" + + "github.com/stretchr/testify/require" + + appcli "github.com/NVIDIA/infra-controller-rest/cli/pkg" +) + +func TestBuildInputSchema_OnlyConfigFields(t *testing.T) { + schema := buildInputSchema(appcli.PathItem{}, &appcli.Operation{OperationID: "get-metadata"}) + require.Equal(t, "object", schema.Type) + for _, c := range commonConfigDescriptions { + require.Contains(t, schema.Properties, c.Name, "missing common config field %s", c.Name) + require.Equal(t, "string", schema.Properties[c.Name].Type) + } + require.Empty(t, schema.Required, "common config fields should never be required") +} + +func TestBuildInputSchema_PathAndQuery(t *testing.T) { + item := appcli.PathItem{ + Parameters: []appcli.Parameter{ + {Name: "org", In: "path", Required: true, Schema: &appcli.Schema{Type: "string"}}, + {Name: "siteId", In: "path", Required: true, Schema: &appcli.Schema{Type: "string"}}, + }, + } + op := &appcli.Operation{ + OperationID: "get-site-status-history", + Parameters: []appcli.Parameter{ + {Name: "pageNumber", In: "query", Schema: &appcli.Schema{Type: "integer"}}, + {Name: "pageSize", In: "query", Schema: &appcli.Schema{Type: "integer"}}, + {Name: "status", In: "query", Schema: &appcli.Schema{Type: "string", Enum: []string{"ACTIVE", "INACTIVE"}}, Required: true}, + }, + } + + schema := buildInputSchema(item, op) + + require.Equal(t, "object", schema.Type) + // org as a path parameter is dropped because Client.Do fills the + // {org} segment from cfg.Org. It is then re-added by the common + // config layer as an optional override, so the schema does carry + // "org" -- but it must NOT be required and must carry the config- + // override description, not the OpenAPI path-param description. + require.Contains(t, schema.Properties, "org") + require.NotContains(t, schema.Required, "org") + require.Equal(t, commonConfigDescriptions[0].Desc, schema.Properties["org"].Description) + require.Contains(t, schema.Properties, "siteId") + require.Equal(t, "string", schema.Properties["siteId"].Type) + require.Contains(t, schema.Properties, "pageNumber") + require.Equal(t, "integer", schema.Properties["pageNumber"].Type) + require.Contains(t, schema.Properties, "pageSize") + require.Equal(t, "integer", schema.Properties["pageSize"].Type) + require.Contains(t, schema.Properties, "status") + require.Equal(t, []any{"ACTIVE", "INACTIVE"}, schema.Properties["status"].Enum) + + // Path params + Required:true query params are required; pure + // query params are not. + require.ElementsMatch(t, []string{"siteId", "status"}, schema.Required) + + // Config-layer fields still merged in. + for _, c := range commonConfigDescriptions { + require.Contains(t, schema.Properties, c.Name) + } +} + +func TestBuildInputSchema_ConfigArgDoesNotOverrideOpenAPIParam(t *testing.T) { + // If an OpenAPI spec accidentally declares a query param named + // "token", the OpenAPI definition wins -- we never overwrite a + // real parameter with the common-config placeholder. + op := &appcli.Operation{ + OperationID: "get-foo", + Parameters: []appcli.Parameter{ + {Name: "token", In: "query", Description: "API-specific token query param", Schema: &appcli.Schema{Type: "string"}}, + }, + } + schema := buildInputSchema(appcli.PathItem{}, op) + require.Contains(t, schema.Properties, "token") + require.Equal(t, "API-specific token query param", schema.Properties["token"].Description) +} + +func TestParamToJSONSchema_TypeMapping(t *testing.T) { + cases := []struct { + openapiType string + want string + }{ + {"string", "string"}, + {"integer", "integer"}, + {"number", "number"}, + {"boolean", "boolean"}, + {"unknown", "string"}, + {"", "string"}, + } + for _, c := range cases { + t.Run(c.openapiType, func(t *testing.T) { + s := paramToJSONSchema(appcli.Parameter{Name: "x", Schema: &appcli.Schema{Type: appcli.SchemaType(c.openapiType)}}) + require.Equal(t, c.want, s.Type) + }) + } +} + +func TestParamToJSONSchema_NoSchemaDefaultsToString(t *testing.T) { + s := paramToJSONSchema(appcli.Parameter{Name: "x", Description: "no schema"}) + require.Equal(t, "string", s.Type) + require.Equal(t, "no schema", s.Description) +} diff --git a/cli/mcp/server.go b/cli/mcp/server.go new file mode 100644 index 000000000..aedf40eae --- /dev/null +++ b/cli/mcp/server.go @@ -0,0 +1,226 @@ +/* + * SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. + * SPDX-License-Identifier: Apache-2.0 + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +// Package mcp serves the NICo REST read surface as MCP tools over +// streamable-HTTP. Tools are projected 1:1 from the embedded OpenAPI +// spec's GET operations. The server is stateless and never emits SSE: +// every tool/call returns a single application/json body. +package mcp + +import ( + "context" + "encoding/json" + "fmt" + "net/http" + "sort" + "strings" + + "github.com/modelcontextprotocol/go-sdk/mcp" + + appcli "github.com/NVIDIA/infra-controller-rest/cli/pkg" +) + +// BuildServer constructs an *mcp.Server with one tool registered for +// every GET operation in the supplied OpenAPI spec. Tool names follow +// the SDD: nico_. Each tool handler builds a +// fresh appcli.Client per call from resolveCallConfig and forwards the +// bearer token from the inbound MCP request to NICo REST unchanged. +// +// BuildServer does not start a listener; callers wrap the result with +// NewHandler to get an *http.Handler ready for ListenAndServe. +func BuildServer(specData []byte, opts Options) (*mcp.Server, error) { + spec, err := appcli.ParseSpec(specData) + if err != nil { + return nil, fmt.Errorf("parsing spec: %w", err) + } + opts = opts.withDefaults() + + server := mcp.NewServer(&mcp.Implementation{ + Name: "nico-mcp", + Title: "NVIDIA Infrastructure Controller (NICo) MCP", + Version: spec.Info.Version, + }, nil) + + for _, path := range sortedPaths(spec) { + item := spec.Paths[path] + if item.Get == nil || item.Get.OperationID == "" { + continue + } + registerGET(server, spec, path, item, opts) + } + return server, nil +} + +// NewHandler wraps an *mcp.Server in a streamable-HTTP handler +// configured for stateless, request/response-only operation: +// +// - Stateless: true -- the SDK does not validate Mcp-Session-Id +// and rejects server->client requests. initialize is a no-op. +// - JSONResponse: true -- every tool/call response uses +// Content-Type: application/json; the SDK never opens an SSE +// stream. The data-hall deployment behind the Latinum Agent +// Gateway Shard Proxy (NATS) requires this. +func NewHandler(server *mcp.Server) http.Handler { + return mcp.NewStreamableHTTPHandler( + func(*http.Request) *mcp.Server { return server }, + &mcp.StreamableHTTPOptions{ + Stateless: true, + JSONResponse: true, + }, + ) +} + +// sortedPaths returns spec.Paths keys in deterministic order so the +// resulting tool list is stable across server restarts. +func sortedPaths(spec *appcli.Spec) []string { + keys := make([]string, 0, len(spec.Paths)) + for k := range spec.Paths { + keys = append(keys, k) + } + sort.Strings(keys) + return keys +} + +func registerGET(server *mcp.Server, spec *appcli.Spec, path string, item appcli.PathItem, opts Options) { + op := item.Get + allParams := append([]appcli.Parameter{}, item.Parameters...) + allParams = append(allParams, op.Parameters...) + + tool := &mcp.Tool{ + Name: toolName(op.OperationID), + Description: toolDescription(op), + InputSchema: buildInputSchema(item, op), + Annotations: &mcp.ToolAnnotations{ + ReadOnlyHint: true, + Title: op.Summary, + }, + } + + mcp.AddTool(server, tool, func(ctx context.Context, req *mcp.CallToolRequest, in map[string]any) (*mcp.CallToolResult, any, error) { + cfg, err := resolveCallConfig(in, req, opts) + if err != nil { + return errorResult(err), nil, nil + } + client := appcli.NewClient(cfg.BaseURL, cfg.Org, cfg.Token, opts.Log, opts.Debug) + client.APIName = cfg.APIName + if cfg.TokenRefresh != nil { + client.TokenRefresh = cfg.TokenRefresh + } + + pathParams, queryParams := splitArgs(in, allParams) + body, _, err := client.Do(http.MethodGet, path, pathParams, queryParams, nil) + if err != nil { + return errorResult(err), nil, nil + } + return jsonResult(body), nil, nil + }) +} + +// toolName converts an operationId like get-all-site to the SDD's +// canonical MCP tool name nico_get_all_site by replacing hyphens with +// underscores and prefixing with nico_. +func toolName(operationID string) string { + return "nico_" + strings.ReplaceAll(operationID, "-", "_") +} + +func toolDescription(op *appcli.Operation) string { + parts := make([]string, 0, 2) + if s := strings.TrimSpace(op.Summary); s != "" { + parts = append(parts, s) + } + if s := strings.TrimSpace(op.Description); s != "" { + parts = append(parts, s) + } + if len(parts) == 0 { + return op.OperationID + } + return strings.Join(parts, "\n\n") +} + +// splitArgs maps the tool input map onto path and query parameters +// using the OpenAPI parameter definitions. Common config keys (org, +// base_url, api_name, token) and any unrecognised keys are dropped: +// they are consumed by resolveCallConfig, not the URL builder. The +// "org" path parameter is intentionally skipped here because +// appcli.Client.Do substitutes {org} from Client.Org, which +// resolveCallConfig sets from the per-call override or config layer. +func splitArgs(in map[string]any, params []appcli.Parameter) (pathParams, queryParams map[string]string) { + pathParams = map[string]string{} + queryParams = map[string]string{} + for _, p := range params { + if p.Name == "org" { + continue + } + raw, ok := in[p.Name] + if !ok { + continue + } + s := coerceToString(raw) + if s == "" { + continue + } + switch p.In { + case "path": + pathParams[p.Name] = s + case "query": + queryParams[p.Name] = s + } + } + return pathParams, queryParams +} + +func coerceToString(v any) string { + switch t := v.(type) { + case nil: + return "" + case string: + return t + case bool: + if t { + return "true" + } + return "false" + case float64: + // JSON numbers decode to float64; format integers without the + // decimal point so they round-trip through query strings. + if t == float64(int64(t)) { + return fmt.Sprintf("%d", int64(t)) + } + return fmt.Sprintf("%g", t) + case int: + return fmt.Sprintf("%d", t) + case int64: + return fmt.Sprintf("%d", t) + case json.Number: + return t.String() + default: + return "" + } +} + +func errorResult(err error) *mcp.CallToolResult { + return &mcp.CallToolResult{ + IsError: true, + Content: []mcp.Content{&mcp.TextContent{Text: err.Error()}}, + } +} + +func jsonResult(body []byte) *mcp.CallToolResult { + return &mcp.CallToolResult{ + Content: []mcp.Content{&mcp.TextContent{Text: string(body)}}, + } +} diff --git a/cli/mcp/server_test.go b/cli/mcp/server_test.go new file mode 100644 index 000000000..3b7c0cfb9 --- /dev/null +++ b/cli/mcp/server_test.go @@ -0,0 +1,193 @@ +/* + * SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. + * SPDX-License-Identifier: Apache-2.0 + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package mcp + +import ( + "encoding/json" + "testing" + + "github.com/stretchr/testify/require" + + appcli "github.com/NVIDIA/infra-controller-rest/cli/pkg" +) + +func TestToolName(t *testing.T) { + cases := []struct { + operationID string + want string + }{ + {"get-metadata", "nico_get_metadata"}, + {"get-all-site", "nico_get_all_site"}, + {"get-site-status-history", "nico_get_site_status_history"}, + {"get-current-tenant", "nico_get_current_tenant"}, + {"validate-rack", "nico_validate_rack"}, + {"validate-trays", "nico_validate_trays"}, + } + for _, c := range cases { + t.Run(c.operationID, func(t *testing.T) { + require.Equal(t, c.want, toolName(c.operationID)) + }) + } +} + +func TestToolDescription(t *testing.T) { + t.Run("summary and description", func(t *testing.T) { + op := &appcli.Operation{ + OperationID: "get-foo", + Summary: "Retrieve Foo", + Description: "More detail on Foo.", + } + require.Equal(t, "Retrieve Foo\n\nMore detail on Foo.", toolDescription(op)) + }) + t.Run("summary only", func(t *testing.T) { + op := &appcli.Operation{OperationID: "get-foo", Summary: "Retrieve Foo"} + require.Equal(t, "Retrieve Foo", toolDescription(op)) + }) + t.Run("operationID fallback", func(t *testing.T) { + op := &appcli.Operation{OperationID: "get-foo"} + require.Equal(t, "get-foo", toolDescription(op)) + }) +} + +func TestSplitArgs(t *testing.T) { + params := []appcli.Parameter{ + {Name: "org", In: "path"}, + {Name: "siteId", In: "path"}, + {Name: "pageNumber", In: "query"}, + {Name: "pageSize", In: "query"}, + } + in := map[string]any{ + "siteId": "abc-123", + "pageNumber": float64(5), + "pageSize": float64(50), + "org": "should-not-appear-here", + "token": "should-be-ignored-by-splitArgs", + "base_url": "should-be-ignored", + "unknown": "ignored", + } + pathParams, queryParams := splitArgs(in, params) + require.Equal(t, map[string]string{"siteId": "abc-123"}, pathParams) + require.Equal(t, map[string]string{"pageNumber": "5", "pageSize": "50"}, queryParams) +} + +func TestCoerceToString(t *testing.T) { + cases := []struct { + name string + in any + want string + }{ + {"string", "foo", "foo"}, + {"empty_string", "", ""}, + {"int_as_float64", float64(42), "42"}, + {"negative_int_as_float64", float64(-3), "-3"}, + {"float_with_fraction", float64(3.14), "3.14"}, + {"bool_true", true, "true"}, + {"bool_false", false, "false"}, + {"int", 7, "7"}, + {"int64", int64(99), "99"}, + {"json_number", json.Number("12345"), "12345"}, + {"nil", nil, ""}, + {"unsupported_slice", []int{1, 2}, ""}, + {"unsupported_map", map[string]any{"a": 1}, ""}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + require.Equal(t, c.want, coerceToString(c.in)) + }) + } +} + +func TestSortedPaths(t *testing.T) { + spec := &appcli.Spec{ + Paths: map[string]appcli.PathItem{ + "/z": {}, "/a": {}, "/m": {}, + }, + } + require.Equal(t, []string{"/a", "/m", "/z"}, sortedPaths(spec)) +} + +// TestBuildServer_SyntheticSpec exercises BuildServer end-to-end on a +// hand-crafted YAML spec to assert tool registration does not panic and +// no error escapes for any combination of GET, POST, and parameterless +// path items. The downstream tool list is verified at the HTTP layer +// in transport_test.go. +func TestBuildServer_SyntheticSpec(t *testing.T) { + specYAML := []byte(` +openapi: 3.0.0 +info: + title: Test + version: 0.0.1 +paths: + /v2/org/{org}/nico/foo: + parameters: + - name: org + in: path + required: true + schema: {type: string} + get: + operationId: get-all-foo + summary: List foos + parameters: + - name: pageSize + in: query + schema: {type: integer} + /v2/org/{org}/nico/foo/{fooId}: + parameters: + - name: org + in: path + required: true + schema: {type: string} + - name: fooId + in: path + required: true + schema: {type: string} + get: + operationId: get-foo + summary: Retrieve a foo + /v2/org/{org}/nico/foo/{fooId}/status-history: + parameters: + - name: org + in: path + required: true + schema: {type: string} + - name: fooId + in: path + required: true + schema: {type: string} + get: + operationId: get-foo-status-history + summary: Foo status history + /v2/org/{org}/nico/skip: + parameters: + - name: org + in: path + required: true + post: + operationId: create-skip + summary: Excluded mutation +`) + + server, err := BuildServer(specYAML, Options{BaseURL: "http://example.test", Org: "demo"}) + require.NoError(t, err) + require.NotNil(t, server) +} + +func TestBuildServer_RejectsInvalidSpec(t *testing.T) { + _, err := BuildServer([]byte("not: valid: yaml: ::"), Options{}) + require.Error(t, err) +} diff --git a/cli/mcp/transport_test.go b/cli/mcp/transport_test.go new file mode 100644 index 000000000..bd02255cd --- /dev/null +++ b/cli/mcp/transport_test.go @@ -0,0 +1,416 @@ +/* + * SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. + * SPDX-License-Identifier: Apache-2.0 + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package mcp + +import ( + "bytes" + "encoding/json" + "io" + "net/http" + "net/http/httptest" + "os" + "path/filepath" + "strings" + "sync" + "sync/atomic" + "testing" + + "github.com/stretchr/testify/require" +) + +// synthSpec is a minimal three-operation OpenAPI document that drives +// the integration tests. The org path param is required because that's +// the real shape of NICo REST routes (/v2/org/{org}/...). +const synthSpec = ` +openapi: 3.0.0 +info: + title: SynthNICo + version: 0.0.1 +paths: + /v2/org/{org}/nico/foo: + parameters: + - {name: org, in: path, required: true, schema: {type: string}} + get: + operationId: get-all-foo + summary: List foos + parameters: + - {name: pageSize, in: query, schema: {type: integer}} + /v2/org/{org}/nico/foo/{fooId}: + parameters: + - {name: org, in: path, required: true, schema: {type: string}} + - {name: fooId, in: path, required: true, schema: {type: string}} + get: + operationId: get-foo + summary: Retrieve a foo + /v2/org/{org}/nico/foo/{fooId}/status-history: + parameters: + - {name: org, in: path, required: true, schema: {type: string}} + - {name: fooId, in: path, required: true, schema: {type: string}} + get: + operationId: get-foo-status-history + summary: Foo status history +` + +func TestHandler_RejectsLongPollGET(t *testing.T) { + server, err := BuildServer([]byte(synthSpec), Options{BaseURL: "http://example.test", Org: "x"}) + require.NoError(t, err) + ts := httptest.NewServer(NewHandler(server)) + defer ts.Close() + + req, err := http.NewRequest(http.MethodGet, ts.URL, nil) + require.NoError(t, err) + req.Header.Set("Accept", "text/event-stream") + resp, err := http.DefaultClient.Do(req) + require.NoError(t, err) + defer resp.Body.Close() + + // In Stateless mode the SDK rejects long-poll GETs because there is + // no session for the server to push notifications onto. The exact + // status code is an SDK choice; we assert it is a non-success + // response so a future SDK change is caught. + require.GreaterOrEqual(t, resp.StatusCode, http.StatusBadRequest, + "long-poll GET on /mcp must be rejected in stateless mode (got %d)", resp.StatusCode) +} + +func TestHandler_ToolsListAndJSONResponse(t *testing.T) { + server, err := BuildServer([]byte(synthSpec), Options{BaseURL: "http://example.test", Org: "x"}) + require.NoError(t, err) + ts := httptest.NewServer(NewHandler(server)) + defer ts.Close() + + resp := mcpPost(t, ts.URL, "", jsonrpcRequest(1, "tools/list", map[string]any{})) + defer resp.Body.Close() + + require.Equal(t, http.StatusOK, resp.StatusCode) + ctype := resp.Header.Get("Content-Type") + require.True(t, strings.HasPrefix(ctype, "application/json"), + "response Content-Type must be application/json, never text/event-stream (got %q)", ctype) + + body, err := io.ReadAll(resp.Body) + require.NoError(t, err) + tools := decodeToolList(t, body) + + wantNames := []string{ + "nico_get_all_foo", + "nico_get_foo", + "nico_get_foo_status_history", + } + gotNames := make([]string, 0, len(tools)) + for _, tool := range tools { + gotNames = append(gotNames, tool.Name) + } + require.ElementsMatch(t, wantNames, gotNames) +} + +func TestHandler_ToolsCall_BearerPassthrough(t *testing.T) { + var ( + recordedAuth atomic.Value + recordedPath atomic.Value + ) + upstream := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + recordedAuth.Store(r.Header.Get("Authorization")) + recordedPath.Store(r.URL.Path) + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"id":"foo-1","name":"Foo One"}`)) + })) + defer upstream.Close() + + server, err := BuildServer([]byte(synthSpec), Options{ + BaseURL: upstream.URL, + Org: "tester", + APIName: "nico", + }) + require.NoError(t, err) + ts := httptest.NewServer(NewHandler(server)) + defer ts.Close() + + resp := mcpPost(t, ts.URL, "Bearer caller-jwt-xyz", jsonrpcRequest(2, "tools/call", map[string]any{ + "name": "nico_get_foo", + "arguments": map[string]any{"fooId": "foo-1"}, + })) + defer resp.Body.Close() + + require.Equal(t, http.StatusOK, resp.StatusCode) + body, err := io.ReadAll(resp.Body) + require.NoError(t, err) + + result := decodeToolCallResult(t, body) + require.False(t, result.IsError, "tool call should succeed: %s", body) + require.Equal(t, `{"id":"foo-1","name":"Foo One"}`, firstText(result)) + + require.Equal(t, "Bearer caller-jwt-xyz", recordedAuth.Load()) + require.Equal(t, "/v2/org/tester/nico/foo/foo-1", recordedPath.Load()) +} + +func TestHandler_ToolsCall_TokenArgWins(t *testing.T) { + var recordedAuth atomic.Value + upstream := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + recordedAuth.Store(r.Header.Get("Authorization")) + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`[]`)) + })) + defer upstream.Close() + + server, err := BuildServer([]byte(synthSpec), Options{ + BaseURL: upstream.URL, + Org: "tester", + APIName: "nico", + }) + require.NoError(t, err) + ts := httptest.NewServer(NewHandler(server)) + defer ts.Close() + + resp := mcpPost(t, ts.URL, "Bearer inbound-bearer", jsonrpcRequest(3, "tools/call", map[string]any{ + "name": "nico_get_all_foo", + "arguments": map[string]any{ + "token": "explicit-tool-arg-token", + }, + })) + defer resp.Body.Close() + + require.Equal(t, http.StatusOK, resp.StatusCode) + require.Equal(t, "Bearer explicit-tool-arg-token", recordedAuth.Load()) +} + +// TestHandler_ConcurrentCallersDoNotBleedTokens proves the +// statelessness invariant: two callers hitting the same handler with +// different bearers each get their own bearer forwarded to NICo REST. +// Run in parallel to also stress for shared-state races. +func TestHandler_ConcurrentCallersDoNotBleedTokens(t *testing.T) { + t.Parallel() + + var ( + mu sync.Mutex + perPath = map[string]string{} + ) + upstream := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + mu.Lock() + perPath[r.URL.Path] = r.Header.Get("Authorization") + mu.Unlock() + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{}`)) + })) + defer upstream.Close() + + server, err := BuildServer([]byte(synthSpec), Options{ + BaseURL: upstream.URL, + Org: "tester", + APIName: "nico", + }) + require.NoError(t, err) + ts := httptest.NewServer(NewHandler(server)) + defer ts.Close() + + const callers = 8 + wg := sync.WaitGroup{} + for i := 0; i < callers; i++ { + wg.Add(1) + go func(i int) { + defer wg.Done() + id := i + 100 + resp := mcpPost(t, ts.URL, + "Bearer caller-"+itoa(i), + jsonrpcRequest(id, "tools/call", map[string]any{ + "name": "nico_get_foo", + "arguments": map[string]any{ + "fooId": "foo-" + itoa(i), + }, + })) + defer resp.Body.Close() + require.Equal(t, http.StatusOK, resp.StatusCode) + }(i) + } + wg.Wait() + + mu.Lock() + defer mu.Unlock() + require.Len(t, perPath, callers, "each caller should have hit a distinct path") + for path, auth := range perPath { + // path looks like /v2/org/tester/nico/foo/foo-; auth must + // match its caller's bearer. + idx := strings.TrimPrefix(path, "/v2/org/tester/nico/foo/foo-") + require.Equal(t, "Bearer caller-"+idx, auth, "bearer leaked between callers on %s", path) + } +} + +// TestHandler_TokenCommandRefreshIsPerCall drives two unrelated tool +// calls through a server configured with a token_command but no static +// token. The upstream returns 401 whenever no Authorization header is +// present, so each call's 401 fires Client.TokenRefresh exactly once. +// The shell counter file proves token_command ran twice -- once per +// tool call -- with no cross-call reuse. +func TestHandler_TokenCommandRefreshIsPerCall(t *testing.T) { + t.Parallel() + + upstream := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Header.Get("Authorization") == "" { + w.WriteHeader(http.StatusUnauthorized) + return + } + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"ok":true}`)) + })) + defer upstream.Close() + + tmpDir := t.TempDir() + counterPath := filepath.Join(tmpDir, "counter") + // printf >> appends one line per invocation; we read line count + // after the test to learn how many times the shell ran. + tokenCmd := "printf 'x\\n' >> " + shellQuote(counterPath) + " && printf '%s' refreshed-token" + + server, err := BuildServer([]byte(synthSpec), Options{ + BaseURL: upstream.URL, + Org: "tester", + APIName: "nico", + TokenCommand: tokenCmd, + }) + require.NoError(t, err) + ts := httptest.NewServer(NewHandler(server)) + defer ts.Close() + + for i := 0; i < 2; i++ { + resp := mcpPost(t, ts.URL, "", jsonrpcRequest(i+1, "tools/call", map[string]any{ + "name": "nico_get_foo", + "arguments": map[string]any{"fooId": "foo-" + itoa(i)}, + })) + _ = resp.Body.Close() + } + + count := countLines(t, counterPath) + require.GreaterOrEqual(t, count, 2, + "token_command must run at least once per tool call (got %d invocations)", count) +} + +// --- helpers below --- + +func mcpPost(t *testing.T, base string, authorization string, body []byte) *http.Response { + t.Helper() + req, err := http.NewRequest(http.MethodPost, base, bytes.NewReader(body)) + require.NoError(t, err) + req.Header.Set("Content-Type", "application/json") + req.Header.Set("Accept", "application/json, text/event-stream") + if authorization != "" { + req.Header.Set("Authorization", authorization) + } + resp, err := http.DefaultClient.Do(req) + require.NoError(t, err) + return resp +} + +func jsonrpcRequest(id int, method string, params map[string]any) []byte { + type wrapper struct { + JSONRPC string `json:"jsonrpc"` + ID int `json:"id"` + Method string `json:"method"` + Params map[string]any `json:"params"` + } + b, err := json.Marshal(wrapper{ + JSONRPC: "2.0", + ID: id, + Method: method, + Params: params, + }) + if err != nil { + panic(err) + } + return b +} + +type rpcTool struct { + Name string `json:"name"` + Description string `json:"description"` +} + +func decodeToolList(t *testing.T, body []byte) []rpcTool { + t.Helper() + var env struct { + Result struct { + Tools []rpcTool `json:"tools"` + } `json:"result"` + } + require.NoError(t, json.Unmarshal(body, &env)) + return env.Result.Tools +} + +type rpcContent struct { + Type string `json:"type"` + Text string `json:"text"` +} + +type rpcToolCallResult struct { + IsError bool `json:"isError"` + Content []rpcContent `json:"content"` +} + +func decodeToolCallResult(t *testing.T, body []byte) rpcToolCallResult { + t.Helper() + var env struct { + Result rpcToolCallResult `json:"result"` + } + require.NoError(t, json.Unmarshal(body, &env)) + return env.Result +} + +func firstText(r rpcToolCallResult) string { + for _, c := range r.Content { + if c.Type == "text" { + return c.Text + } + } + return "" +} + +func itoa(i int) string { + if i == 0 { + return "0" + } + var b strings.Builder + if i < 0 { + b.WriteByte('-') + i = -i + } + digits := []byte{} + for i > 0 { + digits = append([]byte{byte('0' + i%10)}, digits...) + i /= 10 + } + b.Write(digits) + return b.String() +} + +// shellQuote returns a single-quoted shell string. Inputs from t.TempDir +// are safe paths, but quoting keeps the test resilient to environments +// that may sneak whitespace into tmp paths. +func shellQuote(s string) string { + return "'" + strings.ReplaceAll(s, "'", `'\''`) + "'" +} + +func countLines(t *testing.T, path string) int { + t.Helper() + data, err := os.ReadFile(path) + if err != nil { + if os.IsNotExist(err) { + return 0 + } + require.NoError(t, err) + } + if len(data) == 0 { + return 0 + } + return strings.Count(string(data), "\n") +} diff --git a/go.mod b/go.mod index b613ca5b4..9b1782ea4 100644 --- a/go.mod +++ b/go.mod @@ -37,6 +37,7 @@ require ( github.com/gogo/status v1.1.1 github.com/golang-jwt/jwt/v5 v5.3.0 github.com/golang/mock v1.6.0 + github.com/google/jsonschema-go v0.3.0 github.com/google/uuid v1.6.0 github.com/gorilla/mux v1.8.1 github.com/grpc-ecosystem/go-grpc-middleware v1.4.0 @@ -52,6 +53,7 @@ require ( github.com/lib/pq v1.10.9 github.com/metal-stack/v v1.0.3 github.com/mitchellh/mapstructure v1.5.0 + github.com/modelcontextprotocol/go-sdk v0.8.0 github.com/pkg/errors v0.9.1 github.com/prometheus/client_golang v1.23.2 github.com/prometheus/client_model v0.6.2 @@ -225,6 +227,7 @@ require ( github.com/xdg-go/scram v1.1.2 // indirect github.com/xdg-go/stringprep v1.0.4 // indirect github.com/xrash/smetrics v0.0.0-20240521201337-686a1a2994c1 // indirect + github.com/yosida95/uritemplate/v3 v3.0.2 // indirect github.com/youmark/pkcs8 v0.0.0-20240726163527-a2c0da244d78 // indirect github.com/yusufpapurcu/wmi v1.2.4 // indirect go.etcd.io/etcd/api/v3 v3.6.7 // indirect diff --git a/go.sum b/go.sum index d9bf6de43..efd540c35 100644 --- a/go.sum +++ b/go.sum @@ -170,6 +170,8 @@ github.com/google/go-cmp v0.7.0/go.mod h1:pXiqmnSA92OHEEa9HXL2W4E7lf9JzCmGVUdgjX github.com/google/go-querystring v1.1.0 h1:AnCroh3fv4ZBgVIf1Iwtovgjaw/GiKJo8M8yD/fhyJ8= github.com/google/go-querystring v1.1.0/go.mod h1:Kcdr2DB4koayq7X8pmAG4sNG59So17icRSOU623lUBU= github.com/google/gofuzz v1.0.0/go.mod h1:dBl0BpW6vV/+mYPU4Po3pmUjxk6FQPldtuIdl/M65Eg= +github.com/google/jsonschema-go v0.3.0 h1:6AH2TxVNtk3IlvkkhjrtbUc4S8AvO0Xii0DxIygDg+Q= +github.com/google/jsonschema-go v0.3.0/go.mod h1:r5quNTdLOYEz95Ru18zA0ydNbBuYoo9tgaYcxEYhJVE= github.com/google/pprof v0.0.0-20250403155104-27863c87afa6 h1:BHT72Gu3keYf3ZEu2J0b1vyeLSOYI8bm5wbJM/8yDe8= github.com/google/pprof v0.0.0-20250403155104-27863c87afa6/go.mod h1:boTsfXsheKC2y+lKOCMpSfarhxDeIzfZG1jqGcPl3cA= github.com/google/uuid v1.6.0 h1:NIvaJDMOsjHA8n1jAhLSgzrAzy1Hgr+hNrb57e+94F0= @@ -290,6 +292,8 @@ github.com/moby/sys/userns v0.1.0 h1:tVLXkFOxVu9A64/yh59slHVv9ahO9UIev4JZusOLG/g github.com/moby/sys/userns v0.1.0/go.mod h1:IHUYgu/kao6N8YZlp9Cf444ySSvCmDlmzUcYfDHOl28= github.com/moby/term v0.5.0 h1:xt8Q1nalod/v7BqbG21f8mQPqH+xAaC9C3N3wfWbVP0= github.com/moby/term v0.5.0/go.mod h1:8FzsFHVUBGZdbDsJw/ot+X+d5HLUbvklYLJ9uGfcI3Y= +github.com/modelcontextprotocol/go-sdk v0.8.0 h1:jdsBtGzBLY287WKSIjYovOXAqtJkP+HtFQFKrZd4a6c= +github.com/modelcontextprotocol/go-sdk v0.8.0/go.mod h1:nYtYQroQ2KQiM0/SbyEPUWQ6xs4B95gJjEalc9AQyOs= github.com/modern-go/concurrent v0.0.0-20180228061459-e0a39a4cb421/go.mod h1:6dJC0mAP4ikYIbvyc7fijjWJddQyLn8Ig3JB5CqoB9Q= github.com/modern-go/concurrent v0.0.0-20180306012644-bacd9c7ef1dd h1:TRLaZ9cD/w8PVh93nsPXa1VrQ6jlwL5oN8l14QlcNfg= github.com/modern-go/concurrent v0.0.0-20180306012644-bacd9c7ef1dd/go.mod h1:6dJC0mAP4ikYIbvyc7fijjWJddQyLn8Ig3JB5CqoB9Q= @@ -437,6 +441,8 @@ github.com/xdg-go/stringprep v1.0.4 h1:XLI/Ng3O1Atzq0oBs3TWm+5ZVgkq2aqdlvP9JtoZ6 github.com/xdg-go/stringprep v1.0.4/go.mod h1:mPGuuIYwz7CmR2bT9j4GbQqutWS1zV24gijq1dTyGkM= github.com/xrash/smetrics v0.0.0-20240521201337-686a1a2994c1 h1:gEOO8jv9F4OT7lGCjxCBTO/36wtF6j2nSip77qHd4x4= github.com/xrash/smetrics v0.0.0-20240521201337-686a1a2994c1/go.mod h1:Ohn+xnUBiLI6FVj/9LpzZWtj1/D6lUovWYBkxHVV3aM= +github.com/yosida95/uritemplate/v3 v3.0.2 h1:Ed3Oyj9yrmi9087+NczuL5BwkIc4wvTb5zIM+UJPGz4= +github.com/yosida95/uritemplate/v3 v3.0.2/go.mod h1:ILOh0sOhIJR3+L/8afwt/kE++YT040gmv5BQTMR2HP4= github.com/youmark/pkcs8 v0.0.0-20240726163527-a2c0da244d78 h1:ilQV1hzziu+LLM3zUTJ0trRztfwgjqKnBWNtSRkbmwM= github.com/youmark/pkcs8 v0.0.0-20240726163527-a2c0da244d78/go.mod h1:aL8wCCfTfSfmXjznFBSZNN13rSJjlIOI1fUNAtF7rmI= github.com/yuin/goldmark v1.1.27/go.mod h1:3hX8gzYuyVAZsxl0MRgGTJEmQBFcNTphYh9decYSb74= From 4cfc3ba956597cad3640c1118f19c9dff612f491 Mon Sep 17 00:00:00 2001 From: Kyle Felter Date: Mon, 1 Jun 2026 09:56:21 -0500 Subject: [PATCH 2/7] fix: Address CodeRabbit review feedback for MCP mode --- cli/mcp/cmd.go | 9 +++++++- cli/mcp/schema.go | 36 +++++++++++++++++++++++++---- cli/mcp/schema_test.go | 17 ++++++++++++++ cli/mcp/server.go | 42 ++++++++++++++++++++-------------- cli/mcp/server_test.go | 52 +++++++++++++++++++++++++++--------------- go.mod | 6 +++-- go.sum | 16 ++++++++----- 7 files changed, 129 insertions(+), 49 deletions(-) diff --git a/cli/mcp/cmd.go b/cli/mcp/cmd.go index af5708659..1985785c6 100644 --- a/cli/mcp/cmd.go +++ b/cli/mcp/cmd.go @@ -101,6 +101,9 @@ func runServe(c *urfave.Context, specData []byte) error { listen := c.String("listen") path := c.String("path") + if path == "" || path[0] != '/' { + return fmt.Errorf("invalid --path %q: must be non-empty and start with '/'", path) + } shutdownTimeout := c.Duration("shutdown-timeout") mux := http.NewServeMux() @@ -121,6 +124,7 @@ func runServe(c *urfave.Context, specData []byte) error { sigCh := make(chan os.Signal, 1) signal.Notify(sigCh, syscall.SIGINT, syscall.SIGTERM) + defer signal.Stop(sigCh) select { case err := <-errCh: @@ -143,7 +147,10 @@ func runServe(c *urfave.Context, specData []byte) error { // app-global flags on top of the loaded config file, mirroring what the // dynamically-generated commands do via clientFromContext. func buildServeOptions(c *urfave.Context) (Options, error) { - cfg, _ := appcli.LoadConfig() + cfg, err := appcli.LoadConfig() + if err != nil { + return Options{}, fmt.Errorf("loading config: %w", err) + } appcli.ApplyEnvOverrides(cfg) baseURL := cfg.API.Base diff --git a/cli/mcp/schema.go b/cli/mcp/schema.go index fe8262a56..9db6e6d54 100644 --- a/cli/mcp/schema.go +++ b/cli/mcp/schema.go @@ -43,13 +43,34 @@ var commonConfigDescriptions = []struct { // override fields (org, base_url, api_name, token). Path parameters are // marked required; OpenAPI-required query parameters are marked // required; the config overrides are always optional. +type paramKey struct { + in string + name string +} + +// mergeParameters combines path-item and operation parameters, with +// operation-level definitions overriding path-item-level ones that +// share the same {in,name} tuple per OpenAPI override semantics. +func mergeParameters(item appcli.PathItem, op *appcli.Operation) []appcli.Parameter { + merged := map[paramKey]appcli.Parameter{} + for _, p := range item.Parameters { + merged[paramKey{in: p.In, name: p.Name}] = p + } + for _, p := range op.Parameters { + merged[paramKey{in: p.In, name: p.Name}] = p + } + out := make([]appcli.Parameter, 0, len(merged)) + for _, p := range merged { + out = append(out, p) + } + return out +} + func buildInputSchema(item appcli.PathItem, op *appcli.Operation) *jsonschema.Schema { props := map[string]*jsonschema.Schema{} - var required []string + requiredSet := map[string]struct{}{} - allParams := append([]appcli.Parameter{}, item.Parameters...) - allParams = append(allParams, op.Parameters...) - for _, p := range allParams { + for _, p := range mergeParameters(item, op) { if p.Name == "org" { // Resolved from the config layer (org override or server // default). The OpenAPI {org} segment is filled in by @@ -61,10 +82,15 @@ func buildInputSchema(item appcli.PathItem, op *appcli.Operation) *jsonschema.Sc } props[p.Name] = paramToJSONSchema(p) if p.In == "path" || p.Required { - required = append(required, p.Name) + requiredSet[p.Name] = struct{}{} } } + required := make([]string, 0, len(requiredSet)) + for name := range requiredSet { + required = append(required, name) + } + for _, c := range commonConfigDescriptions { if _, exists := props[c.Name]; exists { continue diff --git a/cli/mcp/schema_test.go b/cli/mcp/schema_test.go index 8937f9216..ae6ac4641 100644 --- a/cli/mcp/schema_test.go +++ b/cli/mcp/schema_test.go @@ -81,6 +81,23 @@ func TestBuildInputSchema_PathAndQuery(t *testing.T) { } } +func TestBuildInputSchema_OperationOverridesPathItemParam(t *testing.T) { + item := appcli.PathItem{ + Parameters: []appcli.Parameter{ + {Name: "filter", In: "query", Required: true, Schema: &appcli.Schema{Type: "string"}}, + }, + } + op := &appcli.Operation{ + OperationID: "get-foo", + Parameters: []appcli.Parameter{ + {Name: "filter", In: "query", Required: false, Schema: &appcli.Schema{Type: "string"}}, + }, + } + + schema := buildInputSchema(item, op) + require.NotContains(t, schema.Required, "filter") +} + func TestBuildInputSchema_ConfigArgDoesNotOverrideOpenAPIParam(t *testing.T) { // If an OpenAPI spec accidentally declares a query param named // "token", the OpenAPI definition wins -- we never overwrite a diff --git a/cli/mcp/server.go b/cli/mcp/server.go index aedf40eae..c70997f35 100644 --- a/cli/mcp/server.go +++ b/cli/mcp/server.go @@ -97,8 +97,7 @@ func sortedPaths(spec *appcli.Spec) []string { func registerGET(server *mcp.Server, spec *appcli.Spec, path string, item appcli.PathItem, opts Options) { op := item.Get - allParams := append([]appcli.Parameter{}, item.Parameters...) - allParams = append(allParams, op.Parameters...) + allParams := mergeParameters(item, op) tool := &mcp.Tool{ Name: toolName(op.OperationID), @@ -121,7 +120,10 @@ func registerGET(server *mcp.Server, spec *appcli.Spec, path string, item appcli client.TokenRefresh = cfg.TokenRefresh } - pathParams, queryParams := splitArgs(in, allParams) + pathParams, queryParams, err := splitArgs(in, allParams) + if err != nil { + return errorResult(err), nil, nil + } body, _, err := client.Do(http.MethodGet, path, pathParams, queryParams, nil) if err != nil { return errorResult(err), nil, nil @@ -158,7 +160,10 @@ func toolDescription(op *appcli.Operation) string { // "org" path parameter is intentionally skipped here because // appcli.Client.Do substitutes {org} from Client.Org, which // resolveCallConfig sets from the per-call override or config layer. -func splitArgs(in map[string]any, params []appcli.Parameter) (pathParams, queryParams map[string]string) { +// +// TODO: full OpenAPI style/explode serialization for array and object +// parameters is intentionally deferred; unsupported shapes fail fast. +func splitArgs(in map[string]any, params []appcli.Parameter) (pathParams, queryParams map[string]string, err error) { pathParams = map[string]string{} queryParams = map[string]string{} for _, p := range params { @@ -169,7 +174,10 @@ func splitArgs(in map[string]any, params []appcli.Parameter) (pathParams, queryP if !ok { continue } - s := coerceToString(raw) + s, ok := coerceToString(raw) + if !ok { + return nil, nil, fmt.Errorf("unsupported argument type for %q: %T", p.Name, raw) + } if s == "" { continue } @@ -180,35 +188,35 @@ func splitArgs(in map[string]any, params []appcli.Parameter) (pathParams, queryP queryParams[p.Name] = s } } - return pathParams, queryParams + return pathParams, queryParams, nil } -func coerceToString(v any) string { +func coerceToString(v any) (string, bool) { switch t := v.(type) { case nil: - return "" + return "", true case string: - return t + return t, true case bool: if t { - return "true" + return "true", true } - return "false" + return "false", true case float64: // JSON numbers decode to float64; format integers without the // decimal point so they round-trip through query strings. if t == float64(int64(t)) { - return fmt.Sprintf("%d", int64(t)) + return fmt.Sprintf("%d", int64(t)), true } - return fmt.Sprintf("%g", t) + return fmt.Sprintf("%g", t), true case int: - return fmt.Sprintf("%d", t) + return fmt.Sprintf("%d", t), true case int64: - return fmt.Sprintf("%d", t) + return fmt.Sprintf("%d", t), true case json.Number: - return t.String() + return t.String(), true default: - return "" + return "", false } } diff --git a/cli/mcp/server_test.go b/cli/mcp/server_test.go index 3b7c0cfb9..f61b1e4b2 100644 --- a/cli/mcp/server_test.go +++ b/cli/mcp/server_test.go @@ -80,34 +80,50 @@ func TestSplitArgs(t *testing.T) { "base_url": "should-be-ignored", "unknown": "ignored", } - pathParams, queryParams := splitArgs(in, params) + pathParams, queryParams, err := splitArgs(in, params) + require.NoError(t, err) require.Equal(t, map[string]string{"siteId": "abc-123"}, pathParams) require.Equal(t, map[string]string{"pageNumber": "5", "pageSize": "50"}, queryParams) } +func TestSplitArgs_UnsupportedType(t *testing.T) { + params := []appcli.Parameter{ + {Name: "tags", In: "query"}, + } + in := map[string]any{ + "tags": []string{"a", "b"}, + } + _, _, err := splitArgs(in, params) + require.Error(t, err) + require.Contains(t, err.Error(), "tags") +} + func TestCoerceToString(t *testing.T) { cases := []struct { - name string - in any - want string + name string + in any + want string + wantOK bool }{ - {"string", "foo", "foo"}, - {"empty_string", "", ""}, - {"int_as_float64", float64(42), "42"}, - {"negative_int_as_float64", float64(-3), "-3"}, - {"float_with_fraction", float64(3.14), "3.14"}, - {"bool_true", true, "true"}, - {"bool_false", false, "false"}, - {"int", 7, "7"}, - {"int64", int64(99), "99"}, - {"json_number", json.Number("12345"), "12345"}, - {"nil", nil, ""}, - {"unsupported_slice", []int{1, 2}, ""}, - {"unsupported_map", map[string]any{"a": 1}, ""}, + {"string", "foo", "foo", true}, + {"empty_string", "", "", true}, + {"int_as_float64", float64(42), "42", true}, + {"negative_int_as_float64", float64(-3), "-3", true}, + {"float_with_fraction", float64(3.14), "3.14", true}, + {"bool_true", true, "true", true}, + {"bool_false", false, "false", true}, + {"int", 7, "7", true}, + {"int64", int64(99), "99", true}, + {"json_number", json.Number("12345"), "12345", true}, + {"nil", nil, "", true}, + {"unsupported_slice", []int{1, 2}, "", false}, + {"unsupported_map", map[string]any{"a": 1}, "", false}, } for _, c := range cases { t.Run(c.name, func(t *testing.T) { - require.Equal(t, c.want, coerceToString(c.in)) + got, ok := coerceToString(c.in) + require.Equal(t, c.wantOK, ok) + require.Equal(t, c.want, got) }) } } diff --git a/go.mod b/go.mod index 9b1782ea4..1c4c8026b 100644 --- a/go.mod +++ b/go.mod @@ -37,7 +37,7 @@ require ( github.com/gogo/status v1.1.1 github.com/golang-jwt/jwt/v5 v5.3.0 github.com/golang/mock v1.6.0 - github.com/google/jsonschema-go v0.3.0 + github.com/google/jsonschema-go v0.4.2 github.com/google/uuid v1.6.0 github.com/gorilla/mux v1.8.1 github.com/grpc-ecosystem/go-grpc-middleware v1.4.0 @@ -53,7 +53,7 @@ require ( github.com/lib/pq v1.10.9 github.com/metal-stack/v v1.0.3 github.com/mitchellh/mapstructure v1.5.0 - github.com/modelcontextprotocol/go-sdk v0.8.0 + github.com/modelcontextprotocol/go-sdk v1.4.1 github.com/pkg/errors v0.9.1 github.com/prometheus/client_golang v1.23.2 github.com/prometheus/client_model v0.6.2 @@ -207,6 +207,8 @@ require ( github.com/russross/blackfriday/v2 v2.1.0 // indirect github.com/ryanuber/go-glob v1.0.0 // indirect github.com/sagikazarmark/locafero v0.11.0 // indirect + github.com/segmentio/asm v1.1.3 // indirect + github.com/segmentio/encoding v0.5.4 // indirect github.com/segmentio/ksuid v1.0.4 // indirect github.com/shirou/gopsutil/v4 v4.25.6 // indirect github.com/sourcegraph/conc v0.3.1-0.20240121214520-5f936abd7ae8 // indirect diff --git a/go.sum b/go.sum index efd540c35..d1b034ef6 100644 --- a/go.sum +++ b/go.sum @@ -170,8 +170,8 @@ github.com/google/go-cmp v0.7.0/go.mod h1:pXiqmnSA92OHEEa9HXL2W4E7lf9JzCmGVUdgjX github.com/google/go-querystring v1.1.0 h1:AnCroh3fv4ZBgVIf1Iwtovgjaw/GiKJo8M8yD/fhyJ8= github.com/google/go-querystring v1.1.0/go.mod h1:Kcdr2DB4koayq7X8pmAG4sNG59So17icRSOU623lUBU= github.com/google/gofuzz v1.0.0/go.mod h1:dBl0BpW6vV/+mYPU4Po3pmUjxk6FQPldtuIdl/M65Eg= -github.com/google/jsonschema-go v0.3.0 h1:6AH2TxVNtk3IlvkkhjrtbUc4S8AvO0Xii0DxIygDg+Q= -github.com/google/jsonschema-go v0.3.0/go.mod h1:r5quNTdLOYEz95Ru18zA0ydNbBuYoo9tgaYcxEYhJVE= +github.com/google/jsonschema-go v0.4.2 h1:tmrUohrwoLZZS/P3x7ex0WAVknEkBZM46iALbcqoRA8= +github.com/google/jsonschema-go v0.4.2/go.mod h1:r5quNTdLOYEz95Ru18zA0ydNbBuYoo9tgaYcxEYhJVE= github.com/google/pprof v0.0.0-20250403155104-27863c87afa6 h1:BHT72Gu3keYf3ZEu2J0b1vyeLSOYI8bm5wbJM/8yDe8= github.com/google/pprof v0.0.0-20250403155104-27863c87afa6/go.mod h1:boTsfXsheKC2y+lKOCMpSfarhxDeIzfZG1jqGcPl3cA= github.com/google/uuid v1.6.0 h1:NIvaJDMOsjHA8n1jAhLSgzrAzy1Hgr+hNrb57e+94F0= @@ -292,8 +292,8 @@ github.com/moby/sys/userns v0.1.0 h1:tVLXkFOxVu9A64/yh59slHVv9ahO9UIev4JZusOLG/g github.com/moby/sys/userns v0.1.0/go.mod h1:IHUYgu/kao6N8YZlp9Cf444ySSvCmDlmzUcYfDHOl28= github.com/moby/term v0.5.0 h1:xt8Q1nalod/v7BqbG21f8mQPqH+xAaC9C3N3wfWbVP0= github.com/moby/term v0.5.0/go.mod h1:8FzsFHVUBGZdbDsJw/ot+X+d5HLUbvklYLJ9uGfcI3Y= -github.com/modelcontextprotocol/go-sdk v0.8.0 h1:jdsBtGzBLY287WKSIjYovOXAqtJkP+HtFQFKrZd4a6c= -github.com/modelcontextprotocol/go-sdk v0.8.0/go.mod h1:nYtYQroQ2KQiM0/SbyEPUWQ6xs4B95gJjEalc9AQyOs= +github.com/modelcontextprotocol/go-sdk v1.4.1 h1:M4x9GyIPj+HoIlHNGpK2hq5o3BFhC+78PkEaldQRphc= +github.com/modelcontextprotocol/go-sdk v1.4.1/go.mod h1:Bo/mS87hPQqHSRkMv4dQq1XCu6zv4INdXnFZabkNU6s= github.com/modern-go/concurrent v0.0.0-20180228061459-e0a39a4cb421/go.mod h1:6dJC0mAP4ikYIbvyc7fijjWJddQyLn8Ig3JB5CqoB9Q= github.com/modern-go/concurrent v0.0.0-20180306012644-bacd9c7ef1dd h1:TRLaZ9cD/w8PVh93nsPXa1VrQ6jlwL5oN8l14QlcNfg= github.com/modern-go/concurrent v0.0.0-20180306012644-bacd9c7ef1dd/go.mod h1:6dJC0mAP4ikYIbvyc7fijjWJddQyLn8Ig3JB5CqoB9Q= @@ -364,6 +364,10 @@ github.com/sagikazarmark/locafero v0.11.0 h1:1iurJgmM9G3PA/I+wWYIOw/5SyBtxapeHDc github.com/sagikazarmark/locafero v0.11.0/go.mod h1:nVIGvgyzw595SUSUE6tvCp3YYTeHs15MvlmU87WwIik= github.com/samber/lo v1.52.0 h1:Rvi+3BFHES3A8meP33VPAxiBZX/Aws5RxrschYGjomw= github.com/samber/lo v1.52.0/go.mod h1:4+MXEGsJzbKGaUEQFKBq2xtfuznW9oz/WrgyzMzRoM0= +github.com/segmentio/asm v1.1.3 h1:WM03sfUOENvvKexOLp+pCqgb/WDjsi7EK8gIsICtzhc= +github.com/segmentio/asm v1.1.3/go.mod h1:Ld3L4ZXGNcSLRg4JBsZ3//1+f/TjYl0Mzen/DQy1EJg= +github.com/segmentio/encoding v0.5.4 h1:OW1VRern8Nw6ITAtwSZ7Idrl3MXCFwXHPgqESYfvNt0= +github.com/segmentio/encoding v0.5.4/go.mod h1:HS1ZKa3kSN32ZHVZ7ZLPLXWvOVIiZtyJnO1gPH1sKt0= github.com/segmentio/ksuid v1.0.4 h1:sBo2BdShXjmcugAMwjugoGUdUV0pcxY5mW4xKRn3v4c= github.com/segmentio/ksuid v1.0.4/go.mod h1:/XUiZBD3kVx5SmUOl55voK5yeAbBNNIed+2O73XgrPE= github.com/shirou/gopsutil/v4 v4.25.6 h1:kLysI2JsKorfaFPcYmcJqbzROzsBWEOAtw6A7dIfqXs= @@ -609,8 +613,8 @@ golang.org/x/tools v0.0.0-20200619180055-7c47624df98f/go.mod h1:EkVYQZoAsY45+roY golang.org/x/tools v0.0.0-20210106214847-113979e3529a/go.mod h1:emZCQorbCU4vsT4fOWvOPXz4eW1wZW4PmDk9uLelYpA= golang.org/x/tools v0.1.1/go.mod h1:o0xws9oXOQQZyjljx8fwUC0k7L1pTE6eaCbjGeHmOkk= golang.org/x/tools v0.1.12/go.mod h1:hNGJHUnrk76NpqgfD5Aqm5Crs+Hm0VOH/i9J2+nxYbc= -golang.org/x/tools v0.40.0 h1:yLkxfA+Qnul4cs9QA3KnlFu0lVmd8JJfoq+E41uSutA= -golang.org/x/tools v0.40.0/go.mod h1:Ik/tzLRlbscWpqqMRjyWYDisX8bG13FrdXp3o4Sr9lc= +golang.org/x/tools v0.41.0 h1:a9b8iMweWG+S0OBnlU36rzLp20z1Rp10w+IY2czHTQc= +golang.org/x/tools v0.41.0/go.mod h1:XSY6eDqxVNiYgezAVqqCeihT4j1U2CCsqvH3WhQpnlg= golang.org/x/xerrors v0.0.0-20190717185122-a985d3407aa7/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0= golang.org/x/xerrors v0.0.0-20191011141410-1b5146add898/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0= golang.org/x/xerrors v0.0.0-20191204190536-9bdfabe68543/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0= From eec5d995b6f6953991497b87099a626c8412c8b2 Mon Sep 17 00:00:00 2001 From: Kyle Felter Date: Mon, 1 Jun 2026 10:41:55 -0500 Subject: [PATCH 3/7] refactor: Make MCP serve param-driven and drop config-file loading --- cli/README.md | 4 +-- cli/mcp/cmd.go | 72 +++++++++--------------------------------- cli/mcp/config.go | 4 +-- cli/mcp/config_test.go | 3 +- cli/mcp/server.go | 8 +++++ 5 files changed, 28 insertions(+), 63 deletions(-) diff --git a/cli/README.md b/cli/README.md index 9422998dd..0e172fccc 100644 --- a/cli/README.md +++ b/cli/README.md @@ -360,7 +360,7 @@ nicocli mcp serve \ | `--path` | `NICO_MCP_PATH` | HTTP path prefix the MCP handler is mounted at (default `/mcp`) | | `--shutdown-timeout` | `NICO_MCP_SHUTDOWN_TIMEOUT` | Graceful shutdown timeout (default `10s`) | -`--base-url`, `--org`, `--api-name`, `--token`, and `--token-command` are inherited from the root command and provide the server-side defaults. Like every other `nicocli` command, the server loads `~/.nico/config.yaml` at startup; unset flags fall back to config values. +`--base-url`, `--org`, `--api-name`, `--token`, and `--token-command` are inherited from the root command and provide optional server-side defaults; each also reads its `NICO_*` environment variable. Unlike the other `nicocli` commands, `mcp serve` does **not** read `~/.nico/config.yaml` -- the server is stateless and entirely parameter-driven, so `nicocli mcp serve` starts cleanly with no config file present and every connection detail is supplied per tool call (see below), falling back to these flags only when an argument is omitted. ### Per-call config overrides @@ -373,7 +373,7 @@ Every typical config value can also be passed as an argument on each MCP tool ca | `api_name` | `--api-name` | `api.name` | | `token` | `--token` | `auth.token` | -Precedence per tool call (first non-empty wins): tool argument → inbound `Authorization` header (token only) → server startup flag → server-loaded config file → `token_command` refresh on a 401. `token_command`, OIDC credentials, and NGC api_key settings are NOT exposed as tool arguments -- they are login-flow inputs configured server-side. +Precedence per tool call (first non-empty wins): tool argument → inbound `Authorization` header (token only) → server startup flag/env → `token_command` refresh on a 401. The MCP server does not read the on-disk config file. `token_command`, OIDC credentials, and NGC api_key settings are NOT exposed as tool arguments -- they are login-flow inputs configured server-side via flags/env. ### Probing the server diff --git a/cli/mcp/cmd.go b/cli/mcp/cmd.go index 1985785c6..4fcc0cd73 100644 --- a/cli/mcp/cmd.go +++ b/cli/mcp/cmd.go @@ -29,8 +29,6 @@ import ( "github.com/sirupsen/logrus" urfave "github.com/urfave/cli/v2" - - appcli "github.com/NVIDIA/infra-controller-rest/cli/pkg" ) // Command returns the "mcp" urfave/cli command tree for nicocli. Wire @@ -89,10 +87,7 @@ func serveCommand(specData []byte) *urfave.Command { // and runs an http.Server until SIGINT/SIGTERM. It is split out from the // urfave Action closure so tests can drive it directly. func runServe(c *urfave.Context, specData []byte) error { - opts, err := buildServeOptions(c) - if err != nil { - return err - } + opts := buildServeOptions(c) server, err := BuildServer(specData, opts) if err != nil { @@ -143,63 +138,26 @@ func runServe(c *urfave.Context, specData []byte) error { } } -// buildServeOptions resolves the urfave context into Options by layering -// app-global flags on top of the loaded config file, mirroring what the -// dynamically-generated commands do via clientFromContext. -func buildServeOptions(c *urfave.Context) (Options, error) { - cfg, err := appcli.LoadConfig() - if err != nil { - return Options{}, fmt.Errorf("loading config: %w", err) - } - appcli.ApplyEnvOverrides(cfg) - - baseURL := cfg.API.Base - if c.IsSet("base-url") { - baseURL = c.String("base-url") - } - if baseURL == "" { - baseURL = c.String("base-url") - } - - org := cfg.API.Org - if c.IsSet("org") { - org = c.String("org") - } - - apiName := cfg.API.Name - if c.IsSet("api-name") { - apiName = c.String("api-name") - } - if apiName == "" { - apiName = "nico" - } - - token := "" - if c.IsSet("token") { - token = c.String("token") - } else if cfg.Auth.Token != "" { - token = cfg.Auth.Token - } - - tokenCommand := "" - if c.IsSet("token-command") { - tokenCommand = c.String("token-command") - } else if cfg.Auth.TokenCommand != "" { - tokenCommand = cfg.Auth.TokenCommand - } - +// buildServeOptions resolves the MCP server's start-up defaults from the +// process flags (each of which also reads its NICO_* environment +// variable). Unlike the dynamically-generated commands, mcp serve does +// NOT read ~/.nico/config.yaml: the server is stateless and entirely +// parameter-driven, so every connection detail is supplied per tool call +// via resolveCallConfig, with these flag values as the only fallback. +// This lets "nicocli mcp serve" start cleanly with no config file present. +func buildServeOptions(c *urfave.Context) Options { log := logrus.NewEntry(logrus.StandardLogger()) if c.Bool("debug") { log.Logger.SetLevel(logrus.DebugLevel) } return Options{ - BaseURL: baseURL, - Org: org, - APIName: apiName, - Token: token, - TokenCommand: tokenCommand, + BaseURL: c.String("base-url"), + Org: c.String("org"), + APIName: c.String("api-name"), + Token: c.String("token"), + TokenCommand: c.String("token-command"), Debug: c.Bool("debug"), Log: log, - }, nil + } } diff --git a/cli/mcp/config.go b/cli/mcp/config.go index c49637600..d1a06d6d8 100644 --- a/cli/mcp/config.go +++ b/cli/mcp/config.go @@ -135,8 +135,8 @@ func (c resolvedConfig) requireNonEmpty() error { if len(missing) == 0 { return nil } - return fmt.Errorf("missing required config value(s): %s; pass via tool args, server flags, or %s", - strings.Join(missing, ", "), appcli.ConfigPath()) + return fmt.Errorf("missing required config value(s): %s; pass via tool-call arguments, server flags, or NICO_* environment variables", + strings.Join(missing, ", ")) } func stringArg(in map[string]any, key string) string { diff --git a/cli/mcp/config_test.go b/cli/mcp/config_test.go index f3586137b..8f82a930b 100644 --- a/cli/mcp/config_test.go +++ b/cli/mcp/config_test.go @@ -256,8 +256,7 @@ func TestOptions_WithDefaults(t *testing.T) { require.NotNil(t, o.Log) } -// TestNoConfigWriteBack runs the runServe-relevant config helpers (just -// LoadConfig + ApplyEnvOverrides + the package's TokenRefresh closure) +// TestNoConfigWriteBack exercises the package's TokenRefresh closure // against a temp config file and verifies the file on disk is byte- // identical before and after. This is the design guarantee from the // "Statelessness invariants" section: the MCP server never writes back diff --git a/cli/mcp/server.go b/cli/mcp/server.go index c70997f35..c3157e5e6 100644 --- a/cli/mcp/server.go +++ b/cli/mcp/server.go @@ -74,6 +74,14 @@ func BuildServer(specData []byte, opts Options) (*mcp.Server, error) { // Content-Type: application/json; the SDK never opens an SSE // stream. The data-hall deployment behind the Latinum Agent // Gateway Shard Proxy (NATS) requires this. +// +// DNS-rebinding (localhost) protection and cross-origin protection are +// deliberately left at the SDK's secure defaults (go-sdk v1.4.1+): +// browser cross-origin requests and localhost DNS-rebinding attempts are +// rejected, while non-browser MCP/gateway clients -- which send no Origin +// or Sec-Fetch-Site header -- pass through unaffected. Do not set +// DisableLocalhostProtection or a permissive CrossOriginProtection here +// without understanding the security trade-off. func NewHandler(server *mcp.Server) http.Handler { return mcp.NewStreamableHTTPHandler( func(*http.Request) *mcp.Server { return server }, From ef427170521736908fa87d8d5f59e58c07dce9e0 Mon Sep 17 00:00:00 2001 From: Kyle Felter Date: Tue, 2 Jun 2026 09:27:19 -0500 Subject: [PATCH 4/7] feat: Surface NICo REST pagination metadata in MCP tool results --- cli/README.md | 13 ++++++++----- cli/mcp/server.go | 34 ++++++++++++++++++++++++++++++---- cli/mcp/server_test.go | 30 ++++++++++++++++++++++++++++++ 3 files changed, 68 insertions(+), 9 deletions(-) diff --git a/cli/README.md b/cli/README.md index 0e172fccc..c2b456d80 100644 --- a/cli/README.md +++ b/cli/README.md @@ -338,11 +338,14 @@ nicocli --config ~/.nico/config.prod.yaml tui `nicocli mcp serve` exposes the NICo REST read surface (every `GET` operation in the embedded OpenAPI spec) as Model Context Protocol tools over streamable-HTTP. It is intended to run behind an MCP-aware gateway such as the [Latinum Agent Gateway](https://agentgateway.dev), but works standalone for local development. ```bash -nicocli mcp serve \ - --listen :8080 \ - --path /mcp \ +# --base-url/--org/--api-name/--token/--token-command are root-level flags +# and must be passed BEFORE the `mcp serve` subcommand. +nicocli \ --base-url https://nico.example.com \ - --org tester + --org tester \ + mcp serve \ + --listen :8080 \ + --path /mcp ``` ### Properties @@ -360,7 +363,7 @@ nicocli mcp serve \ | `--path` | `NICO_MCP_PATH` | HTTP path prefix the MCP handler is mounted at (default `/mcp`) | | `--shutdown-timeout` | `NICO_MCP_SHUTDOWN_TIMEOUT` | Graceful shutdown timeout (default `10s`) | -`--base-url`, `--org`, `--api-name`, `--token`, and `--token-command` are inherited from the root command and provide optional server-side defaults; each also reads its `NICO_*` environment variable. Unlike the other `nicocli` commands, `mcp serve` does **not** read `~/.nico/config.yaml` -- the server is stateless and entirely parameter-driven, so `nicocli mcp serve` starts cleanly with no config file present and every connection detail is supplied per tool call (see below), falling back to these flags only when an argument is omitted. +`--base-url`, `--org`, `--api-name`, `--token`, and `--token-command` are inherited from the root command and provide optional server-side defaults; each also reads its `NICO_*` environment variable. Because they are root-level flags, pass them **before** the `mcp serve` subcommand (e.g. `nicocli --org tester mcp serve --listen :8080`). Unlike the other `nicocli` commands, `mcp serve` does **not** read `~/.nico/config.yaml` -- the server is stateless and entirely parameter-driven, so `nicocli mcp serve` starts cleanly with no config file present and every connection detail is supplied per tool call (see below), falling back to these flags only when an argument is omitted. ### Per-call config overrides diff --git a/cli/mcp/server.go b/cli/mcp/server.go index c3157e5e6..a58b386f9 100644 --- a/cli/mcp/server.go +++ b/cli/mcp/server.go @@ -132,11 +132,11 @@ func registerGET(server *mcp.Server, spec *appcli.Spec, path string, item appcli if err != nil { return errorResult(err), nil, nil } - body, _, err := client.Do(http.MethodGet, path, pathParams, queryParams, nil) + body, respHeader, err := client.Do(http.MethodGet, path, pathParams, queryParams, nil) if err != nil { return errorResult(err), nil, nil } - return jsonResult(body), nil, nil + return jsonResult(body, respHeader), nil, nil }) } @@ -235,8 +235,34 @@ func errorResult(err error) *mcp.CallToolResult { } } -func jsonResult(body []byte) *mcp.CallToolResult { - return &mcp.CallToolResult{ +// jsonResult wraps a successful REST response body as a single JSON text +// content block. When the upstream response carries pagination metadata +// (the X-Pagination header NICo REST sets on list endpoints), it is +// surfaced under the result's _meta.pagination so MCP clients can page +// without the metadata polluting the tool's primary JSON payload. +func jsonResult(body []byte, header http.Header) *mcp.CallToolResult { + res := &mcp.CallToolResult{ Content: []mcp.Content{&mcp.TextContent{Text: string(body)}}, } + if meta := paginationMeta(header); meta != nil { + res.Meta = meta + } + return res +} + +// paginationMeta extracts NICo REST's X-Pagination response header into an +// MCP _meta map. The header value is JSON (e.g. +// {"pageNumber":1,"pageSize":50,"total":1234,"orderBy":null}); it is +// parsed so clients get structured fields, falling back to the raw string +// if it is not valid JSON. Returns nil when the header is absent. +func paginationMeta(header http.Header) mcp.Meta { + raw := header.Get("X-Pagination") + if raw == "" { + return nil + } + var parsed any + if err := json.Unmarshal([]byte(raw), &parsed); err != nil { + parsed = raw + } + return mcp.Meta{"pagination": parsed} } diff --git a/cli/mcp/server_test.go b/cli/mcp/server_test.go index f61b1e4b2..63c80f511 100644 --- a/cli/mcp/server_test.go +++ b/cli/mcp/server_test.go @@ -19,6 +19,7 @@ package mcp import ( "encoding/json" + "net/http" "testing" "github.com/stretchr/testify/require" @@ -137,6 +138,35 @@ func TestSortedPaths(t *testing.T) { require.Equal(t, []string{"/a", "/m", "/z"}, sortedPaths(spec)) } +func TestPaginationMeta(t *testing.T) { + t.Run("absent header is nil", func(t *testing.T) { + require.Nil(t, paginationMeta(http.Header{})) + require.Nil(t, paginationMeta(nil)) + }) + t.Run("json header parsed into structured fields", func(t *testing.T) { + h := http.Header{"X-Pagination": []string{`{"pageNumber":1,"pageSize":50,"total":1234,"orderBy":null}`}} + m := paginationMeta(h) + require.NotNil(t, m) + p, ok := m["pagination"].(map[string]any) + require.True(t, ok) + require.Equal(t, float64(1234), p["total"]) + require.Equal(t, float64(1), p["pageNumber"]) + }) + t.Run("non-json header falls back to raw string", func(t *testing.T) { + h := http.Header{"X-Pagination": []string{"weird-value"}} + require.Equal(t, "weird-value", paginationMeta(h)["pagination"]) + }) +} + +func TestJSONResult_AttachesPagination(t *testing.T) { + res := jsonResult([]byte(`[{"id":"a"}]`), http.Header{"X-Pagination": []string{`{"total":7}`}}) + require.Len(t, res.Content, 1) + require.Equal(t, float64(7), res.Meta["pagination"].(map[string]any)["total"]) + + // No pagination header -> no _meta attached. + require.Nil(t, jsonResult([]byte(`[]`), http.Header{}).Meta) +} + // TestBuildServer_SyntheticSpec exercises BuildServer end-to-end on a // hand-crafted YAML spec to assert tool registration does not panic and // no error escapes for any combination of GET, POST, and parameterless From 1f2d0e5aeda9ebfdfd64eaed41f4bcc8e6b14955 Mon Sep 17 00:00:00 2001 From: Kyle Felter Date: Tue, 2 Jun 2026 09:44:54 -0500 Subject: [PATCH 5/7] fix: Harden MCP path and schema validation --- cli/mcp/cmd.go | 14 +++++++++++++- cli/mcp/cmd_test.go | 38 ++++++++++++++++++++++++++++++++++++++ cli/mcp/schema.go | 33 ++++++++++++++++++++++++++------- cli/mcp/schema_test.go | 33 +++++++++++++++++++++++++++++++++ cli/mcp/transport_test.go | 31 +++++++++++++++++++++++++++++++ 5 files changed, 141 insertions(+), 8 deletions(-) create mode 100644 cli/mcp/cmd_test.go diff --git a/cli/mcp/cmd.go b/cli/mcp/cmd.go index 4fcc0cd73..dfff5c230 100644 --- a/cli/mcp/cmd.go +++ b/cli/mcp/cmd.go @@ -102,7 +102,9 @@ func runServe(c *urfave.Context, specData []byte) error { shutdownTimeout := c.Duration("shutdown-timeout") mux := http.NewServeMux() - mux.Handle(path, NewHandler(server)) + if err := registerHandler(mux, path, NewHandler(server)); err != nil { + return err + } httpServer := &http.Server{ Addr: listen, @@ -138,6 +140,16 @@ func runServe(c *urfave.Context, specData []byte) error { } } +func registerHandler(mux *http.ServeMux, path string, handler http.Handler) (err error) { + defer func() { + if r := recover(); r != nil { + err = fmt.Errorf("invalid --path %q: %v", path, r) + } + }() + mux.Handle(path, handler) + return nil +} + // buildServeOptions resolves the MCP server's start-up defaults from the // process flags (each of which also reads its NICO_* environment // variable). Unlike the dynamically-generated commands, mcp serve does diff --git a/cli/mcp/cmd_test.go b/cli/mcp/cmd_test.go new file mode 100644 index 000000000..b68ebdcc0 --- /dev/null +++ b/cli/mcp/cmd_test.go @@ -0,0 +1,38 @@ +/* + * SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. + * SPDX-License-Identifier: Apache-2.0 + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package mcp + +import ( + "net/http" + "testing" + + "github.com/stretchr/testify/require" +) + +func TestRegisterHandler_ValidPath(t *testing.T) { + mux := http.NewServeMux() + err := registerHandler(mux, "/mcp", http.HandlerFunc(func(http.ResponseWriter, *http.Request) {})) + require.NoError(t, err) +} + +func TestRegisterHandler_InvalidPatternReturnsError(t *testing.T) { + mux := http.NewServeMux() + err := registerHandler(mux, "/{", http.HandlerFunc(func(http.ResponseWriter, *http.Request) {})) + require.Error(t, err) + require.Contains(t, err.Error(), "invalid --path") +} diff --git a/cli/mcp/schema.go b/cli/mcp/schema.go index 9db6e6d54..6a26d57c3 100644 --- a/cli/mcp/schema.go +++ b/cli/mcp/schema.go @@ -18,6 +18,7 @@ package mcp import ( + "encoding/json" "sort" "github.com/google/jsonschema-go/jsonschema" @@ -32,8 +33,8 @@ var commonConfigDescriptions = []struct { Name string Desc string }{ - {"org", "Override the server's default org (api.org) for this call. If unset, the inbound bearer's claims plus server-side config decide the org."}, - {"base_url", "Override the server's default base URL (api.base) for this call. Useful when one MCP server fronts multiple NICo REST deployments."}, + {"org", "Org used in /v2/org//... paths for this call. Overrides the server startup flag/env default when set."}, + {"base_url", "NICo REST base URL for this call. Overrides the server startup flag/env default when set; useful when one MCP server fronts multiple NICo REST deployments."}, {"api_name", "Override the API path segment used in /v2/org///... (api.name; default \"nico\")."}, {"token", "Bearer token for this call. Overrides the inbound Authorization header. Omit in production behind agentgateway; the gateway-injected JWT is passed through automatically."}, } @@ -72,9 +73,8 @@ func buildInputSchema(item appcli.PathItem, op *appcli.Operation) *jsonschema.Sc for _, p := range mergeParameters(item, op) { if p.Name == "org" { - // Resolved from the config layer (org override or server - // default). The OpenAPI {org} segment is filled in by - // appcli.Client.Do. + // Resolved from per-call args or server startup defaults. + // The OpenAPI {org} segment is filled in by appcli.Client.Do. continue } if p.In != "path" && p.In != "query" { @@ -111,14 +111,17 @@ func buildInputSchema(item appcli.PathItem, op *appcli.Operation) *jsonschema.Sc // paramToJSONSchema converts a single OpenAPI parameter to a JSON // schema fragment. Types are normalised to integer/boolean/number/ -// string; everything else falls back to string. Enums are preserved -// where present. +// string; everything else falls back to string. Scalar validation hints +// such as format, min/max, length bounds, defaults, and enums are +// preserved where present so MCP clients get the same guardrails as the +// generated CLI flags. func paramToJSONSchema(p appcli.Parameter) *jsonschema.Schema { s := &jsonschema.Schema{Description: p.Description} if p.Schema == nil { s.Type = "string" return s } + openapiSchema := p.Schema switch p.Schema.Type { case "integer": s.Type = "integer" @@ -135,5 +138,21 @@ func paramToJSONSchema(p appcli.Parameter) *jsonschema.Schema { s.Enum = append(s.Enum, e) } } + s.Format = openapiSchema.Format + s.MinLength = openapiSchema.MinLength + s.MaxLength = openapiSchema.MaxLength + if openapiSchema.Minimum != nil { + v := float64(*openapiSchema.Minimum) + s.Minimum = &v + } + if openapiSchema.Maximum != nil { + v := float64(*openapiSchema.Maximum) + s.Maximum = &v + } + if openapiSchema.Default != nil { + if b, err := json.Marshal(openapiSchema.Default); err == nil { + s.Default = b + } + } return s } diff --git a/cli/mcp/schema_test.go b/cli/mcp/schema_test.go index ae6ac4641..c9b7b2231 100644 --- a/cli/mcp/schema_test.go +++ b/cli/mcp/schema_test.go @@ -18,6 +18,7 @@ package mcp import ( + "encoding/json" "testing" "github.com/stretchr/testify/require" @@ -138,3 +139,35 @@ func TestParamToJSONSchema_NoSchemaDefaultsToString(t *testing.T) { require.Equal(t, "string", s.Type) require.Equal(t, "no schema", s.Description) } + +func TestParamToJSONSchema_PreservesScalarValidationHints(t *testing.T) { + minLen := 3 + maxLen := 64 + min := 1 + max := 100 + s := paramToJSONSchema(appcli.Parameter{ + Name: "pageSize", + Schema: &appcli.Schema{ + Type: "integer", + Format: "int32", + MinLength: &minLen, + MaxLength: &maxLen, + Minimum: &min, + Maximum: &max, + Default: 20, + }, + }) + + require.Equal(t, "integer", s.Type) + require.Equal(t, "int32", s.Format) + require.Equal(t, &minLen, s.MinLength) + require.Equal(t, &maxLen, s.MaxLength) + require.NotNil(t, s.Minimum) + require.Equal(t, float64(1), *s.Minimum) + require.NotNil(t, s.Maximum) + require.Equal(t, float64(100), *s.Maximum) + + var defaultValue int + require.NoError(t, json.Unmarshal(s.Default, &defaultValue)) + require.Equal(t, 20, defaultValue) +} diff --git a/cli/mcp/transport_test.go b/cli/mcp/transport_test.go index bd02255cd..a8779dc98 100644 --- a/cli/mcp/transport_test.go +++ b/cli/mcp/transport_test.go @@ -117,6 +117,37 @@ func TestHandler_ToolsListAndJSONResponse(t *testing.T) { require.ElementsMatch(t, wantNames, gotNames) } +func TestHandler_ToolsCall_RejectsOutOfRangeParam(t *testing.T) { + specYAML := strings.Replace(synthSpec, + `{name: pageSize, in: query, schema: {type: integer}}`, + `{name: pageSize, in: query, schema: {type: integer, minimum: 1, maximum: 100}}`, + 1, + ) + server, err := BuildServer([]byte(specYAML), Options{BaseURL: "http://example.test", Org: "x"}) + require.NoError(t, err) + ts := httptest.NewServer(NewHandler(server)) + defer ts.Close() + + resp := mcpPost(t, ts.URL, "", jsonrpcRequest(2, "tools/call", map[string]any{ + "name": "nico_get_all_foo", + "arguments": map[string]any{"pageSize": 101}, + })) + defer resp.Body.Close() + require.Equal(t, http.StatusOK, resp.StatusCode) + + body, err := io.ReadAll(resp.Body) + require.NoError(t, err) + var env struct { + Error *struct { + Message string `json:"message"` + } `json:"error"` + } + require.NoError(t, json.Unmarshal(body, &env)) + require.NotNil(t, env.Error) + require.Contains(t, env.Error.Message, "invalid params") + require.Contains(t, env.Error.Message, "pageSize") +} + func TestHandler_ToolsCall_BearerPassthrough(t *testing.T) { var ( recordedAuth atomic.Value From dce5e270d61a3443210d5382fa2a07d6ca9971bd Mon Sep 17 00:00:00 2001 From: Kyle Felter Date: Tue, 2 Jun 2026 09:57:26 -0500 Subject: [PATCH 6/7] docs: Clarify MCP serve path flag behavior --- cli/README.md | 2 +- cli/mcp/cmd.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/cli/README.md b/cli/README.md index c2b456d80..04c69fd81 100644 --- a/cli/README.md +++ b/cli/README.md @@ -360,7 +360,7 @@ nicocli \ | Flag | Env Var | Description | |------|---------|-------------| | `--listen` | `NICO_MCP_LISTEN` | Listen address (default `:8080`) | -| `--path` | `NICO_MCP_PATH` | HTTP path prefix the MCP handler is mounted at (default `/mcp`) | +| `--path` | `NICO_MCP_PATH` | HTTP path the MCP handler is mounted at (default `/mcp`) | | `--shutdown-timeout` | `NICO_MCP_SHUTDOWN_TIMEOUT` | Graceful shutdown timeout (default `10s`) | `--base-url`, `--org`, `--api-name`, `--token`, and `--token-command` are inherited from the root command and provide optional server-side defaults; each also reads its `NICO_*` environment variable. Because they are root-level flags, pass them **before** the `mcp serve` subcommand (e.g. `nicocli --org tester mcp serve --listen :8080`). Unlike the other `nicocli` commands, `mcp serve` does **not** read `~/.nico/config.yaml` -- the server is stateless and entirely parameter-driven, so `nicocli mcp serve` starts cleanly with no config file present and every connection detail is supplied per tool call (see below), falling back to these flags only when an argument is omitted. diff --git a/cli/mcp/cmd.go b/cli/mcp/cmd.go index dfff5c230..f4dc584df 100644 --- a/cli/mcp/cmd.go +++ b/cli/mcp/cmd.go @@ -66,7 +66,7 @@ func serveCommand(specData []byte) *urfave.Command { }, &urfave.StringFlag{ Name: "path", - Usage: "HTTP path prefix the MCP handler is mounted at", + Usage: "HTTP path the MCP handler is mounted at", EnvVars: []string{"NICO_MCP_PATH"}, Value: "/mcp", }, From 3776f573a01cb636ec5f01f780c0f302aaa5b7fa Mon Sep 17 00:00:00 2001 From: Kyle Felter Date: Tue, 2 Jun 2026 15:07:05 -0500 Subject: [PATCH 7/7] fix: Tighten MCP tool argument validation --- cli/mcp/config.go | 18 +++++++++++++++--- cli/mcp/config_test.go | 18 ++++++++++++++++-- cli/mcp/schema.go | 11 ++++++++--- cli/mcp/schema_test.go | 2 ++ cli/mcp/transport_test.go | 26 ++++++++++++++++++++++++++ 5 files changed, 67 insertions(+), 8 deletions(-) diff --git a/cli/mcp/config.go b/cli/mcp/config.go index d1a06d6d8..9828c87ec 100644 --- a/cli/mcp/config.go +++ b/cli/mcp/config.go @@ -96,16 +96,16 @@ type resolvedConfig struct { // instead of letting the call go out with an invalid URL. func resolveCallConfig(in map[string]any, req *mcp.CallToolRequest, opts Options) (resolvedConfig, error) { cfg := resolvedConfig{ - BaseURL: pickString(stringArg(in, "base_url"), opts.BaseURL), + BaseURL: normalizeBaseURL(pickString(stringArg(in, "base_url"), opts.BaseURL)), Org: pickString(stringArg(in, "org"), opts.Org), APIName: pickString(stringArg(in, "api_name"), opts.APIName), } - cfg.Token = pickString( + cfg.Token = normalizeToken(pickString( stringArg(in, "token"), bearerFromExtra(req), opts.Token, - ) + )) if opts.TokenCommand != "" { tokenCommand := opts.TokenCommand @@ -154,6 +154,18 @@ func stringArg(in map[string]any, key string) string { return strings.TrimSpace(s) } +func normalizeBaseURL(v string) string { + return strings.TrimRight(v, "/") +} + +func normalizeToken(v string) string { + const prefix = "Bearer " + if len(v) > len(prefix) && strings.EqualFold(v[:len(prefix)], prefix) { + return strings.TrimSpace(v[len(prefix):]) + } + return v +} + func pickString(values ...string) string { for _, v := range values { if v != "" { diff --git a/cli/mcp/config_test.go b/cli/mcp/config_test.go index 8f82a930b..fbc59d986 100644 --- a/cli/mcp/config_test.go +++ b/cli/mcp/config_test.go @@ -43,10 +43,10 @@ func TestResolveCallConfig_PrecedenceChain(t *testing.T) { { name: "tool_args_win_every_field", in: map[string]any{ - "base_url": "https://from-arg.example.com", + "base_url": "https://from-arg.example.com/", "org": "arg-org", "api_name": "arg-name", - "token": "arg-token", + "token": "Bearer arg-token", }, req: requestWithBearer("inbound-bearer"), opts: Options{BaseURL: "https://opts.example.com", Org: "opts-org", APIName: "opts-name", Token: "opts-token"}, @@ -222,6 +222,20 @@ func TestPickString(t *testing.T) { require.Equal(t, "", pickString()) } +func TestNormalizeBaseURL(t *testing.T) { + require.Equal(t, "https://api.example.com", normalizeBaseURL("https://api.example.com/")) + require.Equal(t, "https://api.example.com", normalizeBaseURL("https://api.example.com///")) + require.Equal(t, "https://api.example.com/v2", normalizeBaseURL("https://api.example.com/v2/")) + require.Equal(t, "", normalizeBaseURL("")) +} + +func TestNormalizeToken(t *testing.T) { + require.Equal(t, "abc.def", normalizeToken("Bearer abc.def")) + require.Equal(t, "abc.def", normalizeToken("bearer abc.def")) + require.Equal(t, "abc.def", normalizeToken("Bearer abc.def ")) + require.Equal(t, "abc.def", normalizeToken("abc.def")) +} + func TestBearerFromExtra(t *testing.T) { cases := []struct { name string diff --git a/cli/mcp/schema.go b/cli/mcp/schema.go index 6a26d57c3..5e12ba217 100644 --- a/cli/mcp/schema.go +++ b/cli/mcp/schema.go @@ -103,12 +103,17 @@ func buildInputSchema(item appcli.PathItem, op *appcli.Operation) *jsonschema.Sc sort.Strings(required) return &jsonschema.Schema{ - Type: "object", - Properties: props, - Required: required, + Type: "object", + Properties: props, + Required: required, + AdditionalProperties: falseJSONSchema(), } } +func falseJSONSchema() *jsonschema.Schema { + return &jsonschema.Schema{Not: &jsonschema.Schema{}} +} + // paramToJSONSchema converts a single OpenAPI parameter to a JSON // schema fragment. Types are normalised to integer/boolean/number/ // string; everything else falls back to string. Scalar validation hints diff --git a/cli/mcp/schema_test.go b/cli/mcp/schema_test.go index c9b7b2231..6437cd56f 100644 --- a/cli/mcp/schema_test.go +++ b/cli/mcp/schema_test.go @@ -71,6 +71,8 @@ func TestBuildInputSchema_PathAndQuery(t *testing.T) { require.Equal(t, "integer", schema.Properties["pageSize"].Type) require.Contains(t, schema.Properties, "status") require.Equal(t, []any{"ACTIVE", "INACTIVE"}, schema.Properties["status"].Enum) + require.NotNil(t, schema.AdditionalProperties) + require.NotNil(t, schema.AdditionalProperties.Not) // Path params + Required:true query params are required; pure // query params are not. diff --git a/cli/mcp/transport_test.go b/cli/mcp/transport_test.go index a8779dc98..e6f2306b2 100644 --- a/cli/mcp/transport_test.go +++ b/cli/mcp/transport_test.go @@ -148,6 +148,32 @@ func TestHandler_ToolsCall_RejectsOutOfRangeParam(t *testing.T) { require.Contains(t, env.Error.Message, "pageSize") } +func TestHandler_ToolsCall_RejectsUnknownArg(t *testing.T) { + server, err := BuildServer([]byte(synthSpec), Options{BaseURL: "http://example.test", Org: "x"}) + require.NoError(t, err) + ts := httptest.NewServer(NewHandler(server)) + defer ts.Close() + + resp := mcpPost(t, ts.URL, "", jsonrpcRequest(2, "tools/call", map[string]any{ + "name": "nico_get_all_foo", + "arguments": map[string]any{"page_size": 1}, + })) + defer resp.Body.Close() + require.Equal(t, http.StatusOK, resp.StatusCode) + + body, err := io.ReadAll(resp.Body) + require.NoError(t, err) + var env struct { + Error *struct { + Message string `json:"message"` + } `json:"error"` + } + require.NoError(t, json.Unmarshal(body, &env)) + require.NotNil(t, env.Error) + require.Contains(t, env.Error.Message, "invalid params") + require.Contains(t, env.Error.Message, "page_size") +} + func TestHandler_ToolsCall_BearerPassthrough(t *testing.T) { var ( recordedAuth atomic.Value