From 62c194945842d4a0e9e9fce117f4e9869e593fe3 Mon Sep 17 00:00:00 2001 From: Snider Date: Tue, 14 Apr 2026 18:01:17 +0100 Subject: [PATCH] fix(mcp): rewrite mcpcmd for new core/cli Command API + correct bridge test The mcpcmd package was using the removed Cobra-style cli.Command API (Use/Short/Long/RunE/StringFlag/AddCommand). Rewrites it to the current core.Command{Description, Action, Flags} path-routed pattern so the core-mcp binary compiles again. Registers both "mcp" and "mcp/serve" for parity with the existing OnStartup service-mode flow. Fixes the bridge DescribableGroup test that expected len == svc.Tools() but ToolBridge.Describe prepends the GET tool-listing entry, so the correct expectation is len + 1. Co-Authored-By: Virgil --- cmd/mcpcmd/cmd_mcp.go | 137 ++++++++++++++----------- cmd/mcpcmd/cmd_mcp_test.go | 203 ++++++++++++++++++++++++++++++++++--- pkg/mcp/bridge_test.go | 11 +- 3 files changed, 275 insertions(+), 76 deletions(-) diff --git a/cmd/mcpcmd/cmd_mcp.go b/cmd/mcpcmd/cmd_mcp.go index a57cdf4..6d3641e 100644 --- a/cmd/mcpcmd/cmd_mcp.go +++ b/cmd/mcpcmd/cmd_mcp.go @@ -1,7 +1,14 @@ -// Package mcpcmd provides the MCP server command. +// SPDX-License-Identifier: EUPL-1.2 + +// Package mcpcmd registers the `mcp` and `mcp serve` CLI commands. +// +// Wiring example: +// +// cli.Main(cli.WithCommands("mcp", mcpcmd.AddMCPCommands)) // // Commands: -// - mcp serve: Start the MCP server for AI tool integration +// - mcp Start the MCP server on stdio (default transport). +// - mcp serve Start the MCP server with auto-selected transport. package mcpcmd import ( @@ -10,75 +17,89 @@ import ( "os/signal" "syscall" + core "dappco.re/go/core" "dappco.re/go/mcp/pkg/mcp" "dappco.re/go/mcp/pkg/mcp/agentic" "dappco.re/go/mcp/pkg/mcp/brain" - "dappco.re/go/core/cli/pkg/cli" ) -var workspaceFlag string -var unrestrictedFlag bool - +// newMCPService is the service constructor, indirected for tests. var newMCPService = mcp.New + +// runMCPService starts the MCP server, indirected for tests. var runMCPService = func(svc *mcp.Service, ctx context.Context) error { return svc.Run(ctx) } + +// shutdownMCPService performs graceful shutdown, indirected for tests. var shutdownMCPService = func(svc *mcp.Service, ctx context.Context) error { return svc.Shutdown(ctx) } -var mcpCmd = &cli.Command{ - Use: "mcp", - Short: "MCP server for AI tool integration", - Long: "Model Context Protocol (MCP) server providing file operations, RAG, and metrics tools.", +// workspaceFlag mirrors the --workspace CLI flag value. +var workspaceFlag string + +// unrestrictedFlag mirrors the --unrestricted CLI flag value. +var unrestrictedFlag bool + +// AddMCPCommands registers the `mcp` command tree on the Core instance. +// +// cli.Main(cli.WithCommands("mcp", mcpcmd.AddMCPCommands)) +func AddMCPCommands(c *core.Core) { + c.Command("mcp", core.Command{ + Description: "Model Context Protocol server (stdio, TCP, Unix socket, HTTP).", + Action: runServeAction, + Flags: core.NewOptions( + core.Option{Key: "workspace", Value: ""}, + core.Option{Key: "w", Value: ""}, + core.Option{Key: "unrestricted", Value: false}, + ), + }) + + c.Command("mcp/serve", core.Command{ + Description: "Start the MCP server with auto-selected transport (stdio, TCP, Unix, or HTTP).", + Action: runServeAction, + Flags: core.NewOptions( + core.Option{Key: "workspace", Value: ""}, + core.Option{Key: "w", Value: ""}, + core.Option{Key: "unrestricted", Value: false}, + ), + }) } -var serveCmd = &cli.Command{ - Use: "serve", - Short: "Start the MCP server", - Long: `Start the MCP server on stdio (default), TCP, Unix socket, or HTTP. +// runServeAction is the CLI entrypoint for `mcp` and `mcp serve`. +// +// opts := core.NewOptions(core.Option{Key: "workspace", Value: "."}) +// result := runServeAction(opts) +func runServeAction(opts core.Options) core.Result { + workspaceFlag = core.Trim(firstNonEmpty(opts.String("workspace"), opts.String("w"))) + unrestrictedFlag = opts.Bool("unrestricted") -The server provides file operations plus the brain and agentic subsystems -registered by this command. - -Environment variables: - MCP_ADDR TCP address to listen on (e.g., "localhost:9999") - MCP_UNIX_SOCKET - Unix socket path to listen on (e.g., "/tmp/core-mcp.sock") - Selected after MCP_ADDR and before stdio. - MCP_HTTP_ADDR - HTTP address to listen on (e.g., "127.0.0.1:9101") - Selected before MCP_ADDR and stdio. - -Examples: - # Start with stdio transport (for Claude Code integration) - core mcp serve - - # Start with workspace restriction - core mcp serve --workspace /path/to/project - - # Start unrestricted (explicit opt-in) - core mcp serve --unrestricted - - # Start TCP server - MCP_ADDR=localhost:9999 core mcp serve`, - RunE: func(cmd *cli.Command, args []string) error { - return runServe() - }, + if err := runServe(); err != nil { + return core.Result{Value: err, OK: false} + } + return core.Result{OK: true} } -func initFlags() { - cli.StringFlag(serveCmd, &workspaceFlag, "workspace", "w", "", "Restrict file operations to this directory") - cli.BoolFlag(serveCmd, &unrestrictedFlag, "unrestricted", "", false, "Disable filesystem sandboxing entirely") -} - -// AddMCPCommands registers the 'mcp' command and all subcommands. -func AddMCPCommands(root *cli.Command) { - initFlags() - mcpCmd.AddCommand(serveCmd) - root.AddCommand(mcpCmd) +// firstNonEmpty returns the first non-empty string argument. +// +// firstNonEmpty("", "foo") == "foo" +// firstNonEmpty("bar", "baz") == "bar" +func firstNonEmpty(values ...string) string { + for _, v := range values { + if v != "" { + return v + } + } + return "" } +// runServe wires the MCP service together and blocks until the context is +// cancelled by SIGINT/SIGTERM or a transport error. +// +// if err := runServe(); err != nil { +// core.Error("mcp serve failed", "err", err) +// } func runServe() error { opts := mcp.Options{} @@ -88,22 +109,20 @@ func runServe() error { opts.WorkspaceRoot = workspaceFlag } - // Register OpenBrain and agentic subsystems + // Register OpenBrain and agentic subsystems. opts.Subsystems = []mcp.Subsystem{ brain.NewDirect(), agentic.NewPrep(), } - // Create the MCP service svc, err := newMCPService(opts) if err != nil { - return cli.Wrap(err, "create MCP service") + return core.E("mcpcmd.runServe", "create MCP service", err) } defer func() { _ = shutdownMCPService(svc, context.Background()) }() - // Set up signal handling for clean shutdown ctx, cancel := context.WithCancel(context.Background()) defer cancel() @@ -111,10 +130,12 @@ func runServe() error { signal.Notify(sigCh, syscall.SIGINT, syscall.SIGTERM) go func() { - <-sigCh - cancel() + select { + case <-sigCh: + cancel() + case <-ctx.Done(): + } }() - // Run the server (blocks until context cancelled or error) return runMCPService(svc, ctx) } diff --git a/cmd/mcpcmd/cmd_mcp_test.go b/cmd/mcpcmd/cmd_mcp_test.go index 740e82a..2083adb 100644 --- a/cmd/mcpcmd/cmd_mcp_test.go +++ b/cmd/mcpcmd/cmd_mcp_test.go @@ -1,26 +1,18 @@ +// SPDX-License-Identifier: EUPL-1.2 + package mcpcmd import ( "context" "testing" + core "dappco.re/go/core" "dappco.re/go/mcp/pkg/mcp" ) -func TestRunServe_Good_ShutsDownService(t *testing.T) { - oldNew := newMCPService - oldRun := runMCPService - oldShutdown := shutdownMCPService - oldWorkspace := workspaceFlag - oldUnrestricted := unrestrictedFlag - - t.Cleanup(func() { - newMCPService = oldNew - runMCPService = oldRun - shutdownMCPService = oldShutdown - workspaceFlag = oldWorkspace - unrestrictedFlag = oldUnrestricted - }) +func TestCmdMCP_RunServe_Good_ShutsDownService(t *testing.T) { + restore := stubMCPService(t) + defer restore() workspaceFlag = "" unrestrictedFlag = false @@ -50,3 +42,186 @@ func TestRunServe_Good_ShutsDownService(t *testing.T) { t.Fatal("expected shutdownMCPService to be called") } } + +func TestCmdMCP_RunServeAction_Good_PropagatesFlags(t *testing.T) { + restore := stubMCPService(t) + defer restore() + + workspaceFlag = "" + unrestrictedFlag = false + + var gotOpts mcp.Options + newMCPService = func(opts mcp.Options) (*mcp.Service, error) { + gotOpts = opts + return mcp.New(mcp.Options{WorkspaceRoot: t.TempDir()}) + } + runMCPService = func(svc *mcp.Service, ctx context.Context) error { + return nil + } + shutdownMCPService = func(svc *mcp.Service, ctx context.Context) error { + return nil + } + + tmp := t.TempDir() + opts := core.NewOptions(core.Option{Key: "workspace", Value: tmp}) + + result := runServeAction(opts) + if !result.OK { + t.Fatalf("expected OK, got %+v", result) + } + if gotOpts.WorkspaceRoot != tmp { + t.Fatalf("expected workspace root %q, got %q", tmp, gotOpts.WorkspaceRoot) + } + if gotOpts.Unrestricted { + t.Fatal("expected Unrestricted=false when --workspace is set") + } +} + +func TestCmdMCP_RunServeAction_Good_UnrestrictedFlag(t *testing.T) { + restore := stubMCPService(t) + defer restore() + + workspaceFlag = "" + unrestrictedFlag = false + + var gotOpts mcp.Options + newMCPService = func(opts mcp.Options) (*mcp.Service, error) { + gotOpts = opts + return mcp.New(mcp.Options{Unrestricted: true}) + } + runMCPService = func(svc *mcp.Service, ctx context.Context) error { + return nil + } + shutdownMCPService = func(svc *mcp.Service, ctx context.Context) error { + return nil + } + + opts := core.NewOptions(core.Option{Key: "unrestricted", Value: true}) + + result := runServeAction(opts) + if !result.OK { + t.Fatalf("expected OK, got %+v", result) + } + if !gotOpts.Unrestricted { + t.Fatal("expected Unrestricted=true when --unrestricted is set") + } +} + +func TestCmdMCP_RunServe_Bad_CreateServiceFails(t *testing.T) { + restore := stubMCPService(t) + defer restore() + + workspaceFlag = "" + unrestrictedFlag = false + + sentinel := core.E("mcpcmd.test", "boom", nil) + newMCPService = func(opts mcp.Options) (*mcp.Service, error) { + return nil, sentinel + } + runMCPService = func(svc *mcp.Service, ctx context.Context) error { + t.Fatal("runMCPService should not be called when New fails") + return nil + } + shutdownMCPService = func(svc *mcp.Service, ctx context.Context) error { + t.Fatal("shutdownMCPService should not be called when New fails") + return nil + } + + err := runServe() + if err == nil { + t.Fatal("expected error when newMCPService fails") + } +} + +func TestCmdMCP_RunServeAction_Bad_PropagatesFailure(t *testing.T) { + restore := stubMCPService(t) + defer restore() + + workspaceFlag = "" + unrestrictedFlag = false + + newMCPService = func(opts mcp.Options) (*mcp.Service, error) { + return nil, core.E("mcpcmd.test", "construction failed", nil) + } + runMCPService = func(svc *mcp.Service, ctx context.Context) error { + return nil + } + shutdownMCPService = func(svc *mcp.Service, ctx context.Context) error { + return nil + } + + result := runServeAction(core.NewOptions()) + if result.OK { + t.Fatal("expected runServeAction to fail when service creation fails") + } + if result.Value == nil { + t.Fatal("expected error value on failure") + } +} + +func TestCmdMCP_FirstNonEmpty_Ugly_HandlesAllVariants(t *testing.T) { + tests := []struct { + name string + values []string + want string + }{ + {"no args", nil, ""}, + {"empty string", []string{""}, ""}, + {"all empty", []string{"", "", ""}, ""}, + {"first non-empty", []string{"foo", "bar"}, "foo"}, + {"skip empty", []string{"", "baz"}, "baz"}, + {"mixed", []string{"", "", "last"}, "last"}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + got := firstNonEmpty(tc.values...) + if got != tc.want { + t.Fatalf("firstNonEmpty(%v) = %q, want %q", tc.values, got, tc.want) + } + }) + } +} + +func TestCmdMCP_AddMCPCommands_Good_RegistersMcpTree(t *testing.T) { + c := core.New() + AddMCPCommands(c) + + commands := c.Commands() + if len(commands) == 0 { + t.Fatal("expected at least one registered command") + } + + mustHave := map[string]bool{ + "mcp": false, + "mcp/serve": false, + } + for _, path := range commands { + if _, ok := mustHave[path]; ok { + mustHave[path] = true + } + } + for path, present := range mustHave { + if !present { + t.Fatalf("expected command %q to be registered", path) + } + } +} + +// stubMCPService captures the package-level function pointers and returns a +// restore hook so each test can mutate them without leaking into siblings. +func stubMCPService(t *testing.T) func() { + t.Helper() + oldNew := newMCPService + oldRun := runMCPService + oldShutdown := shutdownMCPService + oldWorkspace := workspaceFlag + oldUnrestricted := unrestrictedFlag + + return func() { + newMCPService = oldNew + runMCPService = oldRun + shutdownMCPService = oldShutdown + workspaceFlag = oldWorkspace + unrestrictedFlag = oldUnrestricted + } +} diff --git a/pkg/mcp/bridge_test.go b/pkg/mcp/bridge_test.go index c73fd6e..4bfd169 100644 --- a/pkg/mcp/bridge_test.go +++ b/pkg/mcp/bridge_test.go @@ -81,13 +81,16 @@ func TestBridgeToAPI_Good_DescribableGroup(t *testing.T) { var dg api.DescribableGroup = bridge descs := dg.Describe() - if len(descs) != len(svc.Tools()) { - t.Fatalf("expected %d descriptions, got %d", len(svc.Tools()), len(descs)) + // ToolBridge.Describe prepends a GET entry describing the tool listing + // endpoint, so the expected count is svc.Tools() + 1. + wantDescs := len(svc.Tools()) + 1 + if len(descs) != wantDescs { + t.Fatalf("expected %d descriptions, got %d", wantDescs, len(descs)) } for _, d := range descs { - if d.Method != "POST" { - t.Errorf("expected Method=POST for %s, got %q", d.Path, d.Method) + if d.Method != "POST" && d.Method != "GET" { + t.Errorf("expected Method=POST or GET for %s, got %q", d.Path, d.Method) } if d.Summary == "" { t.Errorf("expected non-empty Summary for %s", d.Path)