diff --git a/server/internal/mcp/registry.go b/server/internal/mcp/registry.go index 7906d2d..cc756c2 100644 --- a/server/internal/mcp/registry.go +++ b/server/internal/mcp/registry.go @@ -34,6 +34,42 @@ type Caller struct { TokenName string } +// ArgType is the JSON type of one declared tool argument. The set is closed +// deliberately: these are the only shapes the argument helpers in this package +// (stringArg, stringSliceArg, tagArg, pageLimit) can actually decode, so a +// schema promising anything else would advertise an argument no handler could +// read. +type ArgType string + +const ( + ArgString ArgType = "string" + ArgInteger ArgType = "integer" + ArgBoolean ArgType = "boolean" + ArgStringArray ArgType = "string_array" + // ArgTagMap is a flat object of string tag keys to string values, which is + // what tagArg decodes. + ArgTagMap ArgType = "tag_map" + // ArgObject is a free-form object whose inner shape the tool documents in + // the argument description — create_monitor's target, whose fields differ + // per monitor type. + ArgObject ArgType = "object" +) + +// ToolArg declares one argument a tool actually reads. +// +// Without this, a tool's arguments existed only in prose inside its +// Description and in the handler's args[...] lookups: no client could discover +// limit, tags, cursor, confirm, server_ids or any of the rest, so a model had +// to guess them from the description or not use them at all. Declaring them +// here also gets the SDK to validate and reject a malformed call before the +// handler runs, which is where a required-argument check belongs. +type ToolArg struct { + Name string + Type ArgType + Description string + Required bool +} + // Tool is one registered capability. type Tool struct { Name string @@ -44,8 +80,63 @@ type Tool struct { Scope string // Write marks a tool that changes something. A write tool is omitted from // the listing for a caller without mcp:write. - Write bool - Handler ToolFunc + Write bool + // Args declares every argument the handler reads, in the order a client + // should see them. A tool taking none declares an empty slice, which is + // distinct from "nobody has written the schema yet" — see the registry + // tests, which require the declaration to be deliberate. + Args []ToolArg + // TouchesServers marks a tool that returns or acts on server-derived data: + // a hostname, a server ID, a package list, a run's per-server output. Such + // a tool must apply Caller.TokenScope, through GetServerScoped, + // ResolveTargetsScoped, ListServersFiltered or VisibleServerIDs. + // + // Like serverScopedRoutes in the api package, this can only ever assert + // that a declaration exists, never that the handler honours it — get_run_logs + // proved the run's instance and the server's membership in the run and then + // read production stdout for a staging token. What it does buy is that + // adding a tool forces an answer to "does this touch server data?", and the + // registry test names every tool that says yes, so the set cannot grow + // without a reviewer seeing it. + TouchesServers bool + Handler ToolFunc +} + +// InputSchema renders the tool's declared arguments as a JSON Schema object, +// which is what a client reads from tools/list to know what to send. +// +// It returns a map rather than a typed schema so this file stays free of the +// MCP SDK; the transport hands it straight to the SDK, which remarshals it. +// additionalProperties is left open: several handlers accept confirm on top of +// their own arguments through CheckFanOut, and a strict object would refuse a +// call the fan-out guard is there to handle. +func (t Tool) InputSchema() map[string]any { + props := map[string]any{} + var required []string + for _, a := range t.Args { + p := map[string]any{"description": a.Description} + switch a.Type { + case ArgStringArray: + p["type"] = "array" + p["items"] = map[string]any{"type": "string"} + case ArgTagMap: + p["type"] = "object" + p["additionalProperties"] = map[string]any{"type": "string"} + case ArgObject: + p["type"] = "object" + default: + p["type"] = string(a.Type) + } + props[a.Name] = p + if a.Required { + required = append(required, a.Name) + } + } + schema := map[string]any{"type": "object", "properties": props} + if len(required) > 0 { + schema["required"] = required + } + return schema } // Registry holds the tool set in registration order, which is the order a diff --git a/server/internal/mcp/registry_schema_test.go b/server/internal/mcp/registry_schema_test.go new file mode 100644 index 0000000..d5dc90f --- /dev/null +++ b/server/internal/mcp/registry_schema_test.go @@ -0,0 +1,148 @@ +package mcp + +import ( + "encoding/json" + "strings" + "testing" +) + +// Every tool must declare its arguments. A nil Args is "nobody wrote the +// schema", which is what the whole tool set looked like before: descriptions +// promised limit, tags, confirm, server_ids and the rest, and tools/list +// advertised none of them, so no client could discover an argument and a model +// had to guess. An empty (but non-nil) slice is the deliberate "takes none". +func TestEveryToolDeclaresArgs(t *testing.T) { + for _, tool := range All().Tools() { + if tool.Args == nil { + t.Errorf("tool %q declares no Args; use []ToolArg{} if it truly takes none", tool.Name) + } + } +} + +func TestToolArgsAreWellFormed(t *testing.T) { + valid := map[ArgType]bool{ + ArgString: true, ArgInteger: true, ArgBoolean: true, + ArgStringArray: true, ArgTagMap: true, ArgObject: true, + } + for _, tool := range All().Tools() { + seen := map[string]bool{} + for _, a := range tool.Args { + if a.Name == "" { + t.Errorf("tool %q has an argument with no name", tool.Name) + } + if seen[a.Name] { + t.Errorf("tool %q declares argument %q twice", tool.Name, a.Name) + } + seen[a.Name] = true + if !valid[a.Type] { + t.Errorf("tool %q argument %q has unknown type %q", tool.Name, a.Name, a.Type) + } + if strings.TrimSpace(a.Description) == "" { + t.Errorf("tool %q argument %q has no description; the description is what a model reads", tool.Name, a.Name) + } + } + } +} + +// The schema has to survive JSON marshalling, because that is the only form a +// client ever sees it in. +func TestInputSchemaMarshals(t *testing.T) { + for _, tool := range All().Tools() { + schema := tool.InputSchema() + if schema["type"] != "object" { + t.Errorf("tool %q schema is not an object", tool.Name) + } + b, err := json.Marshal(schema) + if err != nil { + t.Errorf("tool %q schema does not marshal: %v", tool.Name, err) + continue + } + props, _ := schema["properties"].(map[string]any) + for _, a := range tool.Args { + if _, ok := props[a.Name]; !ok { + t.Errorf("tool %q declares argument %q but the schema omits it", tool.Name, a.Name) + } + } + if len(tool.Args) > 0 && !strings.Contains(string(b), tool.Args[0].Name) { + t.Errorf("tool %q schema lost argument %q in marshalling", tool.Name, tool.Args[0].Name) + } + } +} + +// serverTouchingTools names every tool that returns or acts on server-derived +// data. The test below pins the registry against it, so a tool added that +// reads a hostname, a server ID, a package list or a run's per-server output +// fails until somebody declares TouchesServers and — the point of the exercise +// — decides how it applies Caller.TokenScope. +// +// This is the assertion that would have caught get_run_logs, which proved the +// run's instance and the named server's membership in the run and then read +// production stdout for a token restricted to staging. Declaring the flag does +// not prove the handler is scoped; it puts the question in front of a reviewer +// at the moment the tool is written, which is the same bargain +// api.serverScopedRoutes makes. +var serverTouchingTools = map[string]bool{ + "list_servers": true, + "get_server": true, + "list_monitors": true, // Runner is a server ID; redacted out of scope. + "get_monitor_status": true, // same. + "list_workflows": true, // target server IDs. + "get_workflow": true, // same. + "get_run": true, // per-server run status. + "get_run_logs": true, // a named server's stdout. + "list_pending_updates": true, + "list_vulnerabilities": true, // affected-host counts. + "get_server_packages": true, + "search_fleet": true, + "run_workflow": true, + "apply_updates": true, + "update_agent": true, + "assign_key": true, + "create_workflow": true, // saves a target server list. +} + +func TestServerTouchingToolsAreDeclared(t *testing.T) { + for _, tool := range All().Tools() { + want := serverTouchingTools[tool.Name] + if tool.TouchesServers != want { + if want { + t.Errorf("tool %q is listed as touching server data but does not declare TouchesServers", tool.Name) + } else { + t.Errorf("tool %q declares TouchesServers but is not in serverTouchingTools; "+ + "add it there, having first checked it applies Caller.TokenScope", tool.Name) + } + } + } + registered := map[string]bool{} + for _, tool := range All().Tools() { + registered[tool.Name] = true + } + for name := range serverTouchingTools { + if !registered[name] { + t.Errorf("serverTouchingTools names %q, which is not a registered tool", name) + } + } +} + +// A tool that touches server data and takes a server_ids or tags selector must +// also offer confirm, or the fan-out guard has no way to be satisfied and a +// legitimate fleet-wide call is unrefusable rather than merely confirmed. +func TestFanOutToolsOfferConfirm(t *testing.T) { + for _, tool := range All().Tools() { + if !tool.Write { + continue + } + selector, confirm := false, false + for _, a := range tool.Args { + switch a.Name { + case "server_ids", "tags": + selector = true + case "confirm": + confirm = true + } + } + if selector && !confirm { + t.Errorf("write tool %q takes a server selector but declares no confirm argument", tool.Name) + } + } +} diff --git a/server/internal/mcp/tools_create.go b/server/internal/mcp/tools_create.go index f2b83fe..27f03f4 100644 --- a/server/internal/mcp/tools_create.go +++ b/server/internal/mcp/tools_create.go @@ -166,7 +166,13 @@ func buildMonitor(args map[string]any) (models.Monitor, error) { func init() { All().Register(Tool{ - Name: "create_step", + Name: "create_step", + Args: []ToolArg{ + {Name: "name", Type: ArgString, Description: "Name for the step.", Required: true}, + {Name: "interpreter", Type: ArgString, Description: "Interpreter to run the script with, e.g. bash or powershell.", Required: true}, + {Name: "script", Type: ArgString, Description: "The script body. It is parsed and scanned exactly as the UI does; secret_refs are refused.", Required: true}, + {Name: "description", Type: ArgString, Description: "What the step does, for a human reading the library later."}, + }, Write: true, Scope: "workflows:write", Description: "Create a reusable workflow step: a named script with an interpreter. " + @@ -192,9 +198,17 @@ func init() { }) All().Register(Tool{ - Name: "create_workflow", - Write: true, - Scope: "workflows:write", + Name: "create_workflow", + Args: []ToolArg{ + {Name: "name", Type: ArgString, Description: "Name for the workflow.", Required: true}, + {Name: "step_ids", Type: ArgStringArray, Description: "IDs of existing steps, in the order they should run.", Required: true}, + {Name: "server_ids", Type: ArgStringArray, Description: "Server IDs to target. Combined with tags as a union; at least one of the two is required."}, + {Name: "tags", Type: ArgTagMap, Description: "Tag key/value pairs a server must carry to be included; ANDed across keys."}, + {Name: "confirm", Type: ArgBoolean, Description: "Set true to proceed when this would affect more servers than the fan-out limit (25)."}, + }, + TouchesServers: true, + Write: true, + Scope: "workflows:write", Description: "Create a workflow from existing step IDs, in the order given, targeting " + "servers by ID or by tags. The workflow is SAVED but not run and cannot be " + "created with a schedule; run it explicitly with run_workflow.", @@ -240,7 +254,14 @@ func init() { }) All().Register(Tool{ - Name: "create_monitor", + Name: "create_monitor", + Args: []ToolArg{ + {Name: "name", Type: ArgString, Description: "Name for the monitor.", Required: true}, + {Name: "type", Type: ArgString, Description: "Check type: http, tcp, icmp or tls.", Required: true}, + {Name: "target", Type: ArgObject, Description: "What to check. http/tls take url; tcp/icmp take host, and tcp also port. Optional: method, keyword, expected_status, tls_warn_days, insecure.", Required: true}, + {Name: "group", Type: ArgString, Description: "Optional group name to file the monitor under."}, + {Name: "interval_sec", Type: ArgInteger, Description: "Seconds between checks; defaults to 60."}, + }, Write: true, Scope: "monitors:write", Description: "Create a monitor. It is SAVED DISABLED and will not check anything or " + diff --git a/server/internal/mcp/tools_fleet.go b/server/internal/mcp/tools_fleet.go index e513bb3..dc4efb8 100644 --- a/server/internal/mcp/tools_fleet.go +++ b/server/internal/mcp/tools_fleet.go @@ -78,8 +78,13 @@ type listServersResult struct { func init() { All().Register(Tool{ - Name: "list_servers", - Scope: "servers:read", + Name: "list_servers", + Args: []ToolArg{ + {Name: "limit", Type: ArgInteger, Description: "Maximum rows to return (default 50, capped at 200)."}, + {Name: "tags", Type: ArgTagMap, Description: "Tag key/value pairs a server must carry to be included; ANDed across keys."}, + }, + TouchesServers: true, + Scope: "servers:read", Description: "List the servers in this Vantage fleet, optionally filtered by tags. " + "Returns a compact summary per server; use get_server for full detail on one.", Handler: func(ctx context.Context, c Caller, args map[string]any) (any, error) { @@ -108,8 +113,12 @@ func init() { }) All().Register(Tool{ - Name: "get_server", - Scope: "servers:read", + Name: "get_server", + Args: []ToolArg{ + {Name: "server_id", Type: ArgString, Description: "The server's ID.", Required: true}, + }, + TouchesServers: true, + Scope: "servers:read", Description: "Get detail for one server by ID: OS, online state and tags. " + "Use list_pending_updates for that server's outstanding package updates.", Handler: func(ctx context.Context, c Caller, args map[string]any) (any, error) { diff --git a/server/internal/mcp/tools_health.go b/server/internal/mcp/tools_health.go index 0d2c33f..ab7de40 100644 --- a/server/internal/mcp/tools_health.go +++ b/server/internal/mcp/tools_health.go @@ -92,8 +92,13 @@ const maxSampleLimit = 500 func init() { All().Register(Tool{ - Name: "list_monitors", - Scope: "monitors:read", + Name: "list_monitors", + Args: []ToolArg{ + {Name: "state", Type: ArgString, Description: "Only monitors in this state: up, down or pending."}, + {Name: "limit", Type: ArgInteger, Description: "Maximum rows to return (default 50, capped at 200)."}, + }, + TouchesServers: true, + Scope: "monitors:read", Description: "List the monitors on this instance with their current state. " + "Pass state:\"down\" to see only what is currently failing.", Handler: func(ctx context.Context, c Caller, args map[string]any) (any, error) { @@ -125,8 +130,12 @@ func init() { }) All().Register(Tool{ - Name: "get_monitor_status", - Scope: "monitors:read", + Name: "get_monitor_status", + Args: []ToolArg{ + {Name: "monitor_id", Type: ArgString, Description: "The monitor's ID.", Required: true}, + }, + TouchesServers: true, + Scope: "monitors:read", Description: "Get one monitor's current state: up, down or pending, the last check " + "time, and the last error message if it is failing.", Handler: func(ctx context.Context, c Caller, args map[string]any) (any, error) { @@ -150,7 +159,11 @@ func init() { }) All().Register(Tool{ - Name: "list_incidents", + Name: "list_incidents", + Args: []ToolArg{ + {Name: "monitor_id", Type: ArgString, Description: "Only incidents for this monitor; omit for every monitor."}, + {Name: "limit", Type: ArgInteger, Description: "Maximum rows to return (default 50, capped at 200)."}, + }, Scope: "monitors:read", Description: "List monitor incidents (outages), most recent first. Pass monitor_id to " + "scope to one monitor, or omit it to see incidents across every monitor.", @@ -211,7 +224,11 @@ func init() { }) All().Register(Tool{ - Name: "get_monitor_samples", + Name: "get_monitor_samples", + Args: []ToolArg{ + {Name: "monitor_id", Type: ArgString, Description: "The monitor's ID.", Required: true}, + {Name: "limit", Type: ArgInteger, Description: "Maximum rows to return (default 50, capped at 200)."}, + }, Scope: "monitors:read", Description: "Get one monitor's recent raw check results (timestamp, ok/fail, latency). " + "Samples are numerous and expire after 48 hours; use list_incidents for a longer view.", diff --git a/server/internal/mcp/tools_work.go b/server/internal/mcp/tools_work.go index 36099a1..ebc66cf 100644 --- a/server/internal/mcp/tools_work.go +++ b/server/internal/mcp/tools_work.go @@ -157,8 +157,12 @@ type listSecretNamesResult struct { func init() { All().Register(Tool{ - Name: "list_workflows", - Scope: "workflows:read", + Name: "list_workflows", + Args: []ToolArg{ + {Name: "limit", Type: ArgInteger, Description: "Maximum rows to return (default 50, capped at 200)."}, + }, + TouchesServers: true, + Scope: "workflows:read", Description: "List the workflows defined on this instance: step count, target count, " + "and whether each is on a schedule. Use get_workflow for the ordered step list.", Handler: func(ctx context.Context, c Caller, args map[string]any) (any, error) { @@ -200,8 +204,12 @@ func init() { }) All().Register(Tool{ - Name: "get_workflow", - Scope: "workflows:read", + Name: "get_workflow", + Args: []ToolArg{ + {Name: "workflow_id", Type: ArgString, Description: "The workflow's ID.", Required: true}, + }, + TouchesServers: true, + Scope: "workflows:read", Description: "Get one workflow's full definition: ordered steps, targets and schedule. " + "Use get_run for what happened the last time it ran.", Handler: func(ctx context.Context, c Caller, args map[string]any) (any, error) { @@ -245,8 +253,12 @@ func init() { }) All().Register(Tool{ - Name: "get_run", - Scope: "workflows:read", + Name: "get_run", + Args: []ToolArg{ + {Name: "run_id", Type: ArgString, Description: "The run's ID.", Required: true}, + }, + TouchesServers: true, + Scope: "workflows:read", Description: "Get one workflow run's status: overall state, start/finish time, and a " + "count of servers by their per-server status. Use get_run_logs for the output of one server.", Handler: func(ctx context.Context, c Caller, args map[string]any) (any, error) { @@ -285,8 +297,14 @@ func init() { }) All().Register(Tool{ - Name: "get_run_logs", - Scope: "workflows:read", + Name: "get_run_logs", + Args: []ToolArg{ + {Name: "run_id", Type: ArgString, Description: "The run's ID.", Required: true}, + {Name: "server_id", Type: ArgString, Description: "Which server within the run to read output for.", Required: true}, + {Name: "limit", Type: ArgInteger, Description: "Maximum rows to return (default 50, capped at 200)."}, + }, + TouchesServers: true, + Scope: "workflows:read", Description: "Get the ordered log lines for one server within one workflow run. " + "Capped at 200 lines by default; ask for a higher limit if you need more.", Handler: func(ctx context.Context, c Caller, args map[string]any) (any, error) { @@ -346,8 +364,14 @@ func init() { }) All().Register(Tool{ - Name: "list_pending_updates", - Scope: "servers:read", + Name: "list_pending_updates", + Args: []ToolArg{ + {Name: "server_id", Type: ArgString, Description: "One server to report on; omit to report across the fleet."}, + {Name: "tags", Type: ArgTagMap, Description: "Tag key/value pairs a server must carry to be included; ANDed across keys."}, + {Name: "limit", Type: ArgInteger, Description: "Maximum rows to return (default 50, capped at 200)."}, + }, + TouchesServers: true, + Scope: "servers:read", Description: "List outstanding package updates across the fleet, or for one server. " + "Pass server_id for one server, or tags to filter by, respecting the token's own scope.", Handler: func(ctx context.Context, c Caller, args map[string]any) (any, error) { @@ -396,8 +420,14 @@ func init() { }) All().Register(Tool{ - Name: "list_vulnerabilities", - Scope: "vulns:read", + Name: "list_vulnerabilities", + Args: []ToolArg{ + {Name: "severity", Type: ArgString, Description: "Only this severity: critical, high, medium or low."}, + {Name: "status", Type: ArgString, Description: "Only findings in this state: open (default) or accepted."}, + {Name: "limit", Type: ArgInteger, Description: "Maximum rows to return (default 50, capped at 200)."}, + }, + TouchesServers: true, + Scope: "vulns:read", Description: "List known CVEs affecting this fleet, one row per CVE/package pair with " + "how many servers are affected. Filter by severity or status (open/accepted).", Handler: func(ctx context.Context, c Caller, args map[string]any) (any, error) { @@ -461,8 +491,14 @@ func init() { }) All().Register(Tool{ - Name: "get_server_packages", - Scope: "vulns:read", + Name: "get_server_packages", + Args: []ToolArg{ + {Name: "server_id", Type: ArgString, Description: "The server's ID.", Required: true}, + {Name: "name", Type: ArgString, Description: "Substring match on the package name. A host can carry ~2000 packages, so pass this."}, + {Name: "limit", Type: ArgInteger, Description: "Maximum rows to return (default 50, capped at 200)."}, + }, + TouchesServers: true, + Scope: "vulns:read", Description: "List installed packages on one server, optionally filtered by name. " + "A server can carry ~2000 packages, so pass name to search rather than listing them all.", Handler: func(ctx context.Context, c Caller, args map[string]any) (any, error) { @@ -497,8 +533,14 @@ func init() { }) All().Register(Tool{ - Name: "search_fleet", - Scope: "vulns:read", + Name: "search_fleet", + Args: []ToolArg{ + {Name: "name", Type: ArgString, Description: "Exact package name to search for across the fleet.", Required: true}, + {Name: "version_below", Type: ArgString, Description: "Not supported and refused if supplied: version ordering is per-distribution and cannot be resolved here."}, + {Name: "limit", Type: ArgInteger, Description: "Maximum rows to return (default 50, capped at 200)."}, + }, + TouchesServers: true, + Scope: "vulns:read", Description: "Search every server's installed packages by name across the whole fleet — " + "answers questions like \"which hosts still run OpenSSL 1.1\". version_below is not " + "currently supported: filtering package versions correctly requires knowing each " + @@ -544,7 +586,11 @@ func init() { }) All().Register(Tool{ - Name: "list_audit_events", + Name: "list_audit_events", + Args: []ToolArg{ + {Name: "event_type", Type: ArgString, Description: "Event type prefix to filter by, e.g. \"workflow\", \"key\", \"server\"."}, + {Name: "limit", Type: ArgInteger, Description: "Maximum rows to return (default 50, capped at 200)."}, + }, Scope: "settings:read", Description: "List recent audit log events on this instance: who did what, and when. " + "Filter by event_type prefix (e.g. \"workflow\", \"key\", \"server\").", @@ -566,7 +612,9 @@ func init() { }) All().Register(Tool{ - Name: "list_secret_names", + Name: "list_secret_names", + // This tool reads no arguments at all. + Args: []ToolArg{}, Scope: "secrets:read", Description: "List secret group and key names on this instance. Metadata only — no " + "tool ever returns a secret's plaintext value to a model.", diff --git a/server/internal/mcp/tools_write.go b/server/internal/mcp/tools_write.go index 6d14eee..47e9eff 100644 --- a/server/internal/mcp/tools_write.go +++ b/server/internal/mcp/tools_write.go @@ -52,9 +52,14 @@ type runStartedResult struct { // REST route makes, so there is exactly one path that starts a run. func init() { All().Register(Tool{ - Name: "run_workflow", - Write: true, - Scope: "workflows:write", + Name: "run_workflow", + Args: []ToolArg{ + {Name: "workflow_id", Type: ArgString, Description: "The workflow to run. Its saved targets are used; this call cannot pick different ones.", Required: true}, + {Name: "confirm", Type: ArgBoolean, Description: "Set true to proceed when this would affect more servers than the fan-out limit (25)."}, + }, + TouchesServers: true, + Write: true, + Scope: "workflows:write", Description: "Run a workflow against the servers it is already configured to target " + "(its saved server list and tags — this call does not let you pick different " + "targets). This EXECUTES COMMANDS on real machines and cannot be undone from " + @@ -121,7 +126,10 @@ type cancelledResult struct { // server. func init() { All().Register(Tool{ - Name: "cancel_run", + Name: "cancel_run", + Args: []ToolArg{ + {Name: "run_id", Type: ArgString, Description: "The run to cancel.", Required: true}, + }, Write: true, Scope: "workflows:write", Description: "Cancel an in-progress workflow run. This stops further steps from " + @@ -160,9 +168,15 @@ type updateBatchResult struct { // instead of hardcoded to one server_id from the URL. func init() { All().Register(Tool{ - Name: "apply_updates", - Write: true, - Scope: "servers:write", + Name: "apply_updates", + Args: []ToolArg{ + {Name: "server_ids", Type: ArgStringArray, Description: "Server IDs to target. Combined with tags as a union; at least one of the two is required."}, + {Name: "tags", Type: ArgTagMap, Description: "Tag key/value pairs a server must carry to be included; ANDed across keys."}, + {Name: "confirm", Type: ArgBoolean, Description: "Set true to proceed when this would affect more servers than the fan-out limit (25)."}, + }, + TouchesServers: true, + Write: true, + Scope: "servers:write", Description: "Apply pending OS package updates on real servers, selected by " + "server_ids and/or tags. This installs packages on real machines right now and " + "cannot be undone from here. A server may need a reboot afterward, which this " + @@ -208,9 +222,15 @@ type agentUpdateResult struct { // over the resolved, scoped target set. func init() { All().Register(Tool{ - Name: "update_agent", - Write: true, - Scope: "servers:write", + Name: "update_agent", + Args: []ToolArg{ + {Name: "server_ids", Type: ArgStringArray, Description: "Server IDs to target. Combined with tags as a union; at least one of the two is required."}, + {Name: "tags", Type: ArgTagMap, Description: "Tag key/value pairs a server must carry to be included; ANDed across keys."}, + {Name: "confirm", Type: ArgBoolean, Description: "Set true to proceed when this would affect more servers than the fan-out limit (25)."}, + }, + TouchesServers: true, + Write: true, + Scope: "servers:write", Description: "Trigger the Vantage agent on real servers, selected by server_ids " + "and/or tags, to download and replace itself with the latest version. This " + "restarts the agent process on real machines and cannot be undone from here.", @@ -266,9 +286,16 @@ type assignKeyResult struct { // server. func init() { All().Register(Tool{ - Name: "assign_key", - Write: true, - Scope: "keys:write", + Name: "assign_key", + Args: []ToolArg{ + {Name: "key_id", Type: ArgString, Description: "The SSH key to assign.", Required: true}, + {Name: "server_ids", Type: ArgStringArray, Description: "Server IDs to target. Combined with tags as a union; at least one of the two is required."}, + {Name: "tags", Type: ArgTagMap, Description: "Tag key/value pairs a server must carry to be included; ANDed across keys."}, + {Name: "confirm", Type: ArgBoolean, Description: "Set true to proceed when this would affect more servers than the fan-out limit (25)."}, + }, + TouchesServers: true, + Write: true, + Scope: "keys:write", Description: "Assign an SSH key to real servers, selected by server_ids and/or " + "tags. The agent rewrites /root/.ssh/authorized_keys on each targeted machine " + "and this cannot be undone from here — use revoke to remove it afterward.", diff --git a/server/internal/mcp/transport.go b/server/internal/mcp/transport.go index 5df7c97..a49121f 100644 --- a/server/internal/mcp/transport.go +++ b/server/internal/mcp/transport.go @@ -67,9 +67,19 @@ func Handler() gin.HandlerFunc { // trusted from Visible, because listing and calling are separate requests and a // token's scopes are re-read on each. func registerSDKTool(srv *sdk.Server, tool Tool, caller Caller) { + // InputSchema is set explicitly rather than inferred from the handler's + // argument type. The SDK can infer one from a typed In parameter, which is + // cleaner where it fits — but every ToolFunc here takes map[string]any, and + // inference over that yields a bare open object saying nothing. Giving each + // tool its own Go argument struct would mean twenty-odd structs and a + // generic registry that could no longer hold them in one map, losing the + // gate logic and the audit wrapper this function exists to apply. The + // declared Args are the same information without that cost, and the SDK + // validates against the schema either way. sdk.AddTool(srv, &sdk.Tool{ Name: tool.Name, Description: tool.Description, + InputSchema: tool.InputSchema(), }, func(ctx context.Context, req *sdk.CallToolRequest, args map[string]any) (*sdk.CallToolResult, any, error) { return callTool(ctx, tool, caller, args) }) diff --git a/server/internal/mcp/transport_schema_test.go b/server/internal/mcp/transport_schema_test.go new file mode 100644 index 0000000..15d8313 --- /dev/null +++ b/server/internal/mcp/transport_schema_test.go @@ -0,0 +1,25 @@ +package mcp + +import ( + "testing" + + sdk "github.com/modelcontextprotocol/go-sdk/mcp" +) + +// sdk.AddTool panics on a schema it cannot resolve, and the only place that +// would otherwise happen is inside a live request. Registering every tool onto +// a real server here moves that failure to the test run. +func TestEveryToolRegistersWithTheSDK(t *testing.T) { + srv := sdk.NewServer(&sdk.Implementation{Name: "vantage", Version: "test"}, nil) + caller := Caller{InstanceID: "i", Scopes: []string{"mcp:write"}} + for _, tool := range All().Tools() { + func() { + defer func() { + if r := recover(); r != nil { + t.Errorf("tool %q: SDK rejected its input schema: %v", tool.Name, r) + } + }() + registerSDKTool(srv, tool, caller) + }() + } +}