Fixes and updates (#31)

* Handle errors in a single place (stop Cobra automatically outputting them)
* Remove duplicate logger creation code
* Update README
* Improved parsing of stderr for MCP server output

Author: peteski22

README.md

@@ -85,7 +85,7 @@ make docs-nav
 
 ## 🤝 Contributing
 
-Coming soon.
+Please see our [Contributing to mcpd](CONTRIBUTING.md) guide for more information. 
 
 ## 📄 License
 

cmd/add.go

@@ -101,7 +101,10 @@ func (c *AddCmd) run(cmd *cobra.Command, args []string) error {
 		return fmt.Errorf("server name cannot be empty")
 	}
 
-	logger := c.Logger()
+	logger, err := c.Logger()
+	if err != nil {
+		return err
+	}
 
 	reg, err := c.registryBuilder.Build()
 	if err != nil {

cmd/daemon.go

@@ -85,12 +85,19 @@ func (c *DaemonCmd) run(cmd *cobra.Command, args []string) error {
 	//	c.Logger.Info("Dev API key", "value", "dev-api-key-12345")   // TODO: Generate local key
 	//	c.Logger.Info("Secrets file", "path", "~/.mcpd/secrets.dev") // TODO: Configurable?
 	//	c.Logger.Info("Press Ctrl+C to stop.")
+	logger, err := c.Logger()
+	if err != nil {
+		return err
+	}
+
+	daemonCtx, daemonCtxCancel := context.WithCancel(context.Background())
+	defer daemonCtxCancel()
 
-	d, err := daemon.NewDaemon(c.Logger(), c.cfgLoader, addr)
+	d, err := daemon.NewDaemon(logger, c.cfgLoader, addr)
 	if err != nil {
 		return fmt.Errorf("failed to create mcpd daemon instance: %w", err)
 	}
-	if err := d.StartAndManage(context.Background()); err != nil {
+	if err := d.StartAndManage(daemonCtx); err != nil {
 		return fmt.Errorf("daemon start failed: %w", err)
 	}
 

cmd/init.go

@@ -51,7 +51,10 @@ The configuration file path can be overridden using the '--%s' flag or the '%s'
 }
 
 func (c *InitCmd) run(cmd *cobra.Command, _ []string) error {
-	logger := c.Logger()
+	logger, err := c.Logger()
+	if err != nil {
+		return err
+	}
 
 	var initFilePath string
 

cmd/remove.go

@@ -53,7 +53,10 @@ func (c *RemoveCmd) run(cmd *cobra.Command, args []string) error {
 		return fmt.Errorf("server name is required and cannot be empty")
 	}
 
-	logger := c.Logger()
+	logger, err := c.Logger()
+	if err != nil {
+		return err
+	}
 
 	name := strings.TrimSpace(args[0])
 	if name == "" {

cmd/root.go

@@ -2,11 +2,7 @@ package cmd
 
 import (
 	"fmt"
-	"io"
-	"os"
-	"strings"
 
-	"github.com/hashicorp/go-hclog"
 	"github.com/spf13/cobra"
 
 	"github.com/mozilla-ai/mcpd/v2/internal/cmd"
@@ -15,8 +11,6 @@ import (
 	"github.com/mozilla-ai/mcpd/v2/internal/printer"
 )
 
-var version = "dev" // Set at build time using -ldflags
-
 // createCmdFunc aliases the signature for a new command function.
 type createCmdFunc func(baseCmd *cmd.BaseCmd, opt ...options.CmdOption) (*cobra.Command, error)
 
@@ -27,7 +21,7 @@ type RootCmd struct {
 // Global variable to hold the root command instance
 var rootCmdInstance *RootCmd
 
-func Execute() {
+func Execute() error {
 	// Create the root command instance
 	rootCmdInstance = &RootCmd{
 		BaseCmd: &cmd.BaseCmd{},
@@ -36,34 +30,24 @@ func Execute() {
 	// Create cobra command.
 	rootCmd, err := NewRootCmd(rootCmdInstance)
 	if err != nil {
-		// TODO: Handle top level error.
-	}
-
-	// Add hook to update loggers after flag parsing
-	rootCmd.PersistentPreRun = func(cmd *cobra.Command, args []string) {
-		// Configure logger with parsed flags
-		logger, err := configureLogger()
-		if err != nil {
-			fmt.Fprintf(os.Stderr, "error configuring logger: %s\n", err)
-			os.Exit(1)
-		}
-
-		// Update the root command instance
-		rootCmdInstance.SetLogger(logger)
+		return fmt.Errorf("could not create root command: %w", err)
 	}
 
 	if err := rootCmd.Execute(); err != nil {
-		os.Exit(1)
+		return err
 	}
+
+	return nil
 }
 
 func NewRootCmd(c *RootCmd) (*cobra.Command, error) {
 	rootCmd := &cobra.Command{
-		Use:          "mcpd <command> [sub-command] [args]",
-		Short:        "'mcpd' CLI is the primary interface for developers to interact with mcpd.",
-		Long:         c.longDescription(),
-		SilenceUsage: true,
-		Version:      version,
+		Use:           "mcpd <command> [sub-command] [args]",
+		Short:         "'mcpd' CLI is the primary interface for developers to interact with mcpd.",
+		Long:          c.longDescription(),
+		SilenceUsage:  true,
+		SilenceErrors: true,
+		Version:       cmd.Version(),
 	}
 
 	// Global flags
@@ -100,43 +84,6 @@ func NewRootCmd(c *RootCmd) (*cobra.Command, error) {
 	return rootCmd, nil
 }
 
-// TODO: Remove and call RootCmd.Logger()?
-func configureLogger() (hclog.Logger, error) {
-	// Use flags first, then fall back to env vars
-	logPath := flags.LogPath
-	if logPath == "" {
-		logPath = strings.TrimSpace(os.Getenv(flags.EnvVarLogPath))
-	}
-
-	// If log path is empty, don't log to a file
-	logOutput := io.Discard
-
-	if logPath != "" {
-		f, err := os.OpenFile(logPath, os.O_CREATE|os.O_APPEND|os.O_WRONLY, 0o644)
-		if err != nil {
-			return nil, fmt.Errorf("failed to open log file (%s): %w", logPath, err)
-		}
-		logOutput = f
-	}
-
-	// Use flags first, then fall back to env vars
-	logLevel := flags.LogLevel
-	if logLevel == "" {
-		logLevel = strings.ToLower(os.Getenv(flags.EnvVarLogLevel))
-		if logLevel == "" {
-			logLevel = flags.DefaultLogLevel
-		}
-	}
-
-	logger := hclog.New(&hclog.LoggerOptions{
-		Name:   "mcpd",
-		Level:  hclog.LevelFromString(logLevel),
-		Output: logOutput,
-	})
-
-	return logger, nil
-}
-
 func (c *RootCmd) longDescription() string {
 	return `The 'mcpd' CLI is the primary interface for developers to interact with the
 mcpd Control Plane, define their agent projects, and manage MCP server dependencies.`

internal/cmd/basecmd.go

@@ -14,6 +14,18 @@ import (
 	"github.com/mozilla-ai/mcpd/v2/internal/runtime"
 )
 
+var version = "dev" // Set at build time using -ldflags
+
+// Version is used by other packages to retrieve the build version of mcpd.
+func Version() string {
+	return version
+}
+
+// AppName returns the name of the mcpd application.
+func AppName() string {
+	return "mcpd"
+}
+
 var _ registry.Builder = (*BaseCmd)(nil)
 
 type BaseCmd struct {
@@ -26,9 +38,9 @@ func (c *BaseCmd) SetLogger(logger hclog.Logger) {
 }
 
 // Logger returns the current logger for the command
-func (c *BaseCmd) Logger() hclog.Logger {
+func (c *BaseCmd) Logger() (hclog.Logger, error) {
 	if c.logger != nil {
-		return c.logger
+		return c.logger, nil
 	}
 
 	// Get log level from flags first, then environment, then default
@@ -46,32 +58,35 @@ func (c *BaseCmd) Logger() hclog.Logger {
 		logPath = strings.TrimSpace(os.Getenv(flags.EnvVarLogPath))
 	}
 
-	// Configure logger output
-	output := io.Discard // os.Stderr
+	// Configure logger output based on the log file path
+	output := io.Discard // Default to discarding log output.
 	if logPath != "" {
 		f, err := os.OpenFile(logPath, os.O_CREATE|os.O_APPEND|os.O_WRONLY, 0o644)
 		if err != nil {
-			fmt.Fprintf(os.Stderr, "Failed to open log file (%s): %v, using stderr\n", logPath, err)
+			return nil, fmt.Errorf("failed to open log file (%s): %w", logPath, err)
 		} else {
 			output = f
 		}
 	}
 
-	// Using flags/env for fallback logger
 	c.logger = hclog.New(&hclog.LoggerOptions{
-		Name:   "mcpd-default",
+		Name:   AppName(),
 		Level:  hclog.LevelFromString(logLevel),
 		Output: output,
 	})
 
-	return c.logger
+	return c.logger, nil
 }
 
 func (c *BaseCmd) Build() (registry.PackageProvider, error) {
-	l := c.Logger().Named("registry")
+	logger, err := c.Logger()
+	if err != nil {
+		return nil, err
+	}
 
 	supportedRuntimes := c.MCPDSupportedRuntimes()
 	opts := runtime.WithSupportedRuntimes(supportedRuntimes...)
+	l := logger.Named("registry")
 
 	// TODO: Should we be using a hardcoded URL
 	mcpm, err := mcpm.NewRegistry(l, "https://getmcp.io/api/servers.json", opts)