Standardize CLI Error Handling (#318)

* Standardize CLI error handling and deprecate cli.Fatal

- Updated `pkg/cli/output.go` to send error and warning output to `os.Stderr`.
- Added `ErrorWrap`, `ErrorWrapVerb`, and `ErrorWrapAction` helpers to `pkg/cli/output.go`.
- Deprecated `cli.Fatal` family of functions in `pkg/cli/errors.go`.
- Introduced `cli.ExitError` and `cli.Exit` helper to allow commands to return specific exit codes.
- Updated `pkg/cli/app.go` to silence Cobra errors and handle error printing and process exit in `Main`.
- Refactored multiple commands (QA, SDK, CI, Updater) to return errors instead of exiting abruptly.
- Replaced direct `os.Stderr` writes with standardized CLI or log helpers across the codebase.
- Updated tests to accommodate changes in output destination.

* Fix CI failure: remove unused fmt import in pkg/mcp/transport_tcp.go

- Removed unused "fmt" import in `pkg/mcp/transport_tcp.go` that was causing CI failure.
- Verified build and relevant tests pass.

* Standardize CLI error handling and fix formatting issues

- Updated `pkg/cli/output.go` to send error and warning output to `os.Stderr`.
- Added `ErrorWrap`, `ErrorWrapVerb`, and `ErrorWrapAction` helpers to `pkg/cli/output.go`.
- Deprecated `cli.Fatal` family of functions in `pkg/cli/errors.go`.
- Introduced `cli.ExitError` and `cli.Exit` helper to allow commands to return specific exit codes.
- Updated `pkg/cli/app.go` to silence Cobra errors and handle error printing and process exit in `Main`.
- Refactored multiple commands (QA, SDK, CI, Updater) to return errors instead of exiting abruptly.
- Replaced direct `os.Stderr` writes with standardized CLI or log helpers across the codebase.
- Updated tests to accommodate changes in output destination.
- Fixed formatting in `pkg/io/local/client.go`.
- Removed unused `fmt` import in `pkg/mcp/transport_tcp.go`.

* Standardize CLI error handling and fix CI issues

- Updated `pkg/cli/output.go` to send error and warning output to `os.Stderr`.
- Added `ErrorWrap`, `ErrorWrapVerb`, and `ErrorWrapAction` helpers to `pkg/cli/output.go`.
- Deprecated `cli.Fatal` family of functions in `pkg/cli/errors.go`.
- Introduced `cli.ExitError` and `cli.Exit` helper to allow commands to return specific exit codes.
- Updated `pkg/cli/app.go` to silence Cobra errors and handle error printing and process exit in `Main`.
- Refactored multiple commands (QA, SDK, CI, Updater) to return errors instead of exiting abruptly.
- Replaced direct `os.Stderr` writes with standardized CLI or log helpers across the codebase.
- Updated tests to accommodate changes in output destination.
- Fixed formatting in `pkg/io/local/client.go`.
- Removed unused `fmt` import in `pkg/mcp/transport_tcp.go`.
- Fixed potential `gh` context issue in `.github/workflows/auto-merge.yml` by providing `GH_REPO`.

---------

Co-authored-by: Claude <developers@lethean.io>

internal/cmd/go/cmd_qa.go

@@ -308,7 +308,7 @@ func runGoQA(cmd *cli.Command, args []string) error {
 	}
 
 	if failed > 0 {
-		os.Exit(1)
+		return cli.Err("QA checks failed: %d passed, %d failed", passed, failed)
 	}
 	return nil
 }

internal/cmd/php/cmd_ci.go

@@ -189,7 +189,7 @@ func runPHPCI() error {
 			return err
 		}
 		if !result.Passed {
-			os.Exit(result.ExitCode)
+			return cli.Exit(result.ExitCode, cli.Err("CI pipeline failed"))
 		}
 		return nil
 	}

internal/cmd/qa/cmd_docblock.go

@@ -167,7 +167,7 @@ func CheckDocblockCoverage(patterns []string) (*DocblockResult, error) {
 		}, parser.ParseComments)
 		if err != nil {
 			// Log parse errors but continue to check other directories
-			fmt.Fprintf(os.Stderr, "warning: failed to parse %s: %v\n", dir, err)
+			cli.Warnf("failed to parse %s: %v", dir, err)
 			continue
 		}
 

internal/cmd/sdk/cmd_sdk.go

@@ -96,8 +96,7 @@ func runSDKDiff(basePath, specPath string) error {
 
 	result, err := Diff(basePath, specPath)
 	if err != nil {
-		fmt.Printf("%s %v\n", sdkErrorStyle.Render(i18n.Label("error")), err)
-		os.Exit(2)
+		return cli.Exit(2, cli.Wrap(err, i18n.Label("error")))
 	}
 
 	if result.Breaking {
@@ -105,7 +104,7 @@ func runSDKDiff(basePath, specPath string) error {
 		for _, change := range result.Changes {
 			fmt.Printf("  - %s\n", change)
 		}
-		os.Exit(1)
+		return cli.Exit(1, cli.Err("%s", result.Summary))
 	}
 
 	fmt.Printf("%s %s\n", sdkSuccessStyle.Render(i18n.T("cmd.sdk.label.ok")), result.Summary)

internal/cmd/updater/cmd.go

@@ -3,7 +3,6 @@ package updater
 import (
 	"context"
 	"fmt"
-	"os"
 	"runtime"
 
 	"github.com/host-uk/core/pkg/cli"
@@ -133,8 +132,6 @@ func runUpdate(cmd *cobra.Command, args []string) error {
 	cli.Print("%s Updated to %s\n", cli.SuccessStyle.Render(cli.Glyph(":check:")), release.TagName)
 	cli.Print("%s Restarting...\n", cli.DimStyle.Render("→"))
 
-	// Exit so the watcher can restart us
-	os.Exit(0)
 	return nil
 }
 
@@ -179,7 +176,6 @@ func handleDevUpdate(currentVersion string) error {
 	cli.Print("%s Updated to %s\n", cli.SuccessStyle.Render(cli.Glyph(":check:")), release.TagName)
 	cli.Print("%s Restarting...\n", cli.DimStyle.Render("→"))
 
-	os.Exit(0)
 	return nil
 }
 
@@ -216,6 +212,5 @@ func handleDevTagUpdate(currentVersion string) error {
 	cli.Print("%s Updated to latest dev build\n", cli.SuccessStyle.Render(cli.Glyph(":check:")))
 	cli.Print("%s Restarting...\n", cli.DimStyle.Render("→"))
 
-	os.Exit(0)
 	return nil
 }

pkg/ansible/executor.go

@@ -120,7 +120,7 @@ func (e *Executor) runPlay(ctx context.Context, play *Play) error {
 			if err := e.gatherFacts(ctx, host, play); err != nil {
 				// Non-fatal
 				if e.Verbose > 0 {
-					fmt.Fprintf(os.Stderr, "Warning: gather facts failed for %s: %v\n", host, err)
+					log.Warn("gather facts failed", "host", host, "err", err)
 				}
 			}
 		}

pkg/cli/app.go

@@ -33,14 +33,23 @@ func Main() {
 			})),
 		},
 	}); err != nil {
-		Fatal(err)
+		Error(err.Error())
+		os.Exit(1)
 	}
 	defer Shutdown()
 
 	// Add completion command to the CLI's root
 	RootCmd().AddCommand(completionCmd)
 
-	Fatal(Execute())
+	if err := Execute(); err != nil {
+		code := 1
+		var exitErr *ExitError
+		if As(err, &exitErr) {
+			code = exitErr.Code
+		}
+		Error(err.Error())
+		os.Exit(code)
+	}
 }
 
 // completionCmd generates shell completion scripts.

pkg/cli/errors.go

@@ -77,41 +77,75 @@ func Join(errs ...error) error {
 	return errors.Join(errs...)
 }
 
+// ExitError represents an error that should cause the CLI to exit with a specific code.
+type ExitError struct {
+	Code int
+	Err  error
+}
+
+func (e *ExitError) Error() string {
+	if e.Err == nil {
+		return ""
+	}
+	return e.Err.Error()
+}
+
+func (e *ExitError) Unwrap() error {
+	return e.Err
+}
+
+// Exit creates a new ExitError with the given code and error.
+// Use this to return an error from a command with a specific exit code.
+func Exit(code int, err error) error {
+	if err == nil {
+		return nil
+	}
+	return &ExitError{Code: code, Err: err}
+}
+
 // ─────────────────────────────────────────────────────────────────────────────
-// Fatal Functions (print and exit)
+// Fatal Functions (Deprecated - return error from command instead)
 // ─────────────────────────────────────────────────────────────────────────────
 
-// Fatal prints an error message and exits with code 1.
+// Fatal prints an error message to stderr and exits with code 1.
+//
+// Deprecated: return an error from the command instead.
 func Fatal(err error) {
 	if err != nil {
-		fmt.Println(ErrorStyle.Render(Glyph(":cross:") + " " + err.Error()))
+		fmt.Fprintln(os.Stderr, ErrorStyle.Render(Glyph(":cross:")+" "+err.Error()))
 		os.Exit(1)
 	}
 }
 
-// Fatalf prints a formatted error message and exits with code 1.
+// Fatalf prints a formatted error message to stderr and exits with code 1.
+//
+// Deprecated: return an error from the command instead.
 func Fatalf(format string, args ...any) {
 	msg := fmt.Sprintf(format, args...)
-	fmt.Println(ErrorStyle.Render(Glyph(":cross:") + " " + msg))
+	fmt.Fprintln(os.Stderr, ErrorStyle.Render(Glyph(":cross:")+" "+msg))
 	os.Exit(1)
 }
 
-// FatalWrap prints a wrapped error message and exits with code 1.
+// FatalWrap prints a wrapped error message to stderr and exits with code 1.
 // Does nothing if err is nil.
 //
+// Deprecated: return an error from the command instead.
+//
 //	cli.FatalWrap(err, "load config")  // Prints "✗ load config: <error>" and exits
 func FatalWrap(err error, msg string) {
 	if err == nil {
 		return
 	}
 	fullMsg := fmt.Sprintf("%s: %v", msg, err)
-	fmt.Println(ErrorStyle.Render(Glyph(":cross:") + " " + fullMsg))
+	fmt.Fprintln(os.Stderr, ErrorStyle.Render(Glyph(":cross:")+" "+fullMsg))
 	os.Exit(1)
 }
 
-// FatalWrapVerb prints a wrapped error using i18n grammar and exits with code 1.
+// FatalWrapVerb prints a wrapped error using i18n grammar to stderr and exits with code 1.
 // Does nothing if err is nil.
 //
+// Deprecated: return an error from the command instead.
+//
 //	cli.FatalWrapVerb(err, "load", "config")  // Prints "✗ Failed to load config: <error>" and exits
 func FatalWrapVerb(err error, verb, subject string) {
 	if err == nil {
@@ -119,6 +153,6 @@ func FatalWrapVerb(err error, verb, subject string) {
 	}
 	msg := i18n.ActionFailed(verb, subject)
 	fullMsg := fmt.Sprintf("%s: %v", msg, err)
-	fmt.Println(ErrorStyle.Render(Glyph(":cross:") + " " + fullMsg))
+	fmt.Fprintln(os.Stderr, ErrorStyle.Render(Glyph(":cross:")+" "+fullMsg))
 	os.Exit(1)
 }

pkg/cli/output.go

@@ -2,6 +2,7 @@ package cli
 
 import (
 	"fmt"
+	"os"
 	"strings"
 
 	"github.com/host-uk/core/pkg/i18n"
@@ -45,22 +46,48 @@ func Successf(format string, args ...any) {
 	Success(fmt.Sprintf(format, args...))
 }
 
-// Error prints an error message with cross (red).
+// Error prints an error message with cross (red) to stderr.
 func Error(msg string) {
-	fmt.Println(ErrorStyle.Render(Glyph(":cross:") + " " + msg))
+	fmt.Fprintln(os.Stderr, ErrorStyle.Render(Glyph(":cross:")+" "+msg))
 }
 
-// Errorf prints a formatted error message.
+// Errorf prints a formatted error message to stderr.
 func Errorf(format string, args ...any) {
 	Error(fmt.Sprintf(format, args...))
 }
 
-// Warn prints a warning message with warning symbol (amber).
-func Warn(msg string) {
-	fmt.Println(WarningStyle.Render(Glyph(":warn:") + " " + msg))
+// ErrorWrap prints a wrapped error message to stderr.
+func ErrorWrap(err error, msg string) {
+	if err == nil {
+		return
+	}
+	Error(fmt.Sprintf("%s: %v", msg, err))
 }
 
-// Warnf prints a formatted warning message.
+// ErrorWrapVerb prints a wrapped error using i18n grammar to stderr.
+func ErrorWrapVerb(err error, verb, subject string) {
+	if err == nil {
+		return
+	}
+	msg := i18n.ActionFailed(verb, subject)
+	Error(fmt.Sprintf("%s: %v", msg, err))
+}
+
+// ErrorWrapAction prints a wrapped error using i18n grammar to stderr.
+func ErrorWrapAction(err error, verb string) {
+	if err == nil {
+		return
+	}
+	msg := i18n.ActionFailed(verb, "")
+	Error(fmt.Sprintf("%s: %v", msg, err))
+}
+
+// Warn prints a warning message with warning symbol (amber) to stderr.
+func Warn(msg string) {
+	fmt.Fprintln(os.Stderr, WarningStyle.Render(Glyph(":warn:")+" "+msg))
+}
+
+// Warnf prints a formatted warning message to stderr.
 func Warnf(format string, args ...any) {
 	Warn(fmt.Sprintf(format, args...))
 }

pkg/cli/output_test.go

@@ -8,14 +8,17 @@ import (
 )
 
 func captureOutput(f func()) string {
-	old := os.Stdout
+	oldOut := os.Stdout
+	oldErr := os.Stderr
 	r, w, _ := os.Pipe()
 	os.Stdout = w
+	os.Stderr = w
 
 	f()
 
 	_ = w.Close()
-	os.Stdout = old
+	os.Stdout = oldOut
+	os.Stderr = oldErr
 
 	var buf bytes.Buffer
 	_, _ = io.Copy(&buf, r)

pkg/cli/runtime.go

@@ -58,8 +58,10 @@ func Init(opts Options) error {
 
 		// Create root command
 		rootCmd := &cobra.Command{
-			Use:     opts.AppName,
-			Version: opts.Version,
+			Use:           opts.AppName,
+			Version:       opts.Version,
+			SilenceErrors: true,
+			SilenceUsage:  true,
 		}
 
 		// Attach all registered commands

pkg/mcp/transport_tcp.go

@@ -3,11 +3,10 @@ package mcp
 import (
 	"bufio"
 	"context"
-	"fmt"
 	"io"
 	"net"
-	"os"
 
+	"github.com/host-uk/core/pkg/log"
 	"github.com/modelcontextprotocol/go-sdk/jsonrpc"
 	"github.com/modelcontextprotocol/go-sdk/mcp"
 )
@@ -49,7 +48,7 @@ func (s *Service) ServeTCP(ctx context.Context, addr string) error {
 	if addr == "" {
 		addr = t.listener.Addr().String()
 	}
-	fmt.Fprintf(os.Stderr, "MCP TCP server listening on %s\n", addr)
+	log.Info("MCP TCP server listening", "addr", addr)
 
 	for {
 		conn, err := t.listener.Accept()
@@ -58,7 +57,7 @@ func (s *Service) ServeTCP(ctx context.Context, addr string) error {
 			case <-ctx.Done():
 				return nil
 			default:
-				fmt.Fprintf(os.Stderr, "Accept error: %v\n", err)
+				log.Error("Accept error", "err", err)
 				continue
 			}
 		}
@@ -84,7 +83,7 @@ func (s *Service) handleConnection(ctx context.Context, conn net.Conn) {
 	// Run server (blocks until connection closed)
 	// Server.Run calls Connect, then Read loop.
 	if err := server.Run(ctx, transport); err != nil {
-		fmt.Fprintf(os.Stderr, "Connection error: %v\n", err)
+		log.Error("Connection error", "err", err)
 	}
 }