fix(cmd): exit non-zero when a command handler returns an error (#4255)

pkg/gofr/cmd.go

@@ -34,30 +34,36 @@ func (e ErrCommandNotFound) Error() string {
 	return fmt.Sprintf("'%s' is not a valid command.", e.Command)
 }
 
-func (cmd *cmd) Run(c *container.Container) {
+// Run executes the sub-command parsed from os.Args and returns true if the
+// handler (or command resolution) responded with an error, so the caller can
+// exit with a non-zero status.
+func (cmd *cmd) Run(c *container.Container) bool {
 	args := os.Args[1:] // First one is command itself
 	subCommand, showHelp, firstArg := parseArgs(args)
 
 	if showHelp && subCommand == "" {
 		cmd.printHelp()
-		return
+		return false
 	}
 
 	r := cmd.handler(subCommand)
-	ctx := newCMDContext(&cmd2.Responder{}, cmd2.NewRequest(args), c, cmd.out)
+	responder := &cmd2.Responder{}
+	ctx := newCMDContext(responder, cmd2.NewRequest(args), c, cmd.out)
 
 	commandForError := getCommandForError(subCommand, firstArg)
 
 	if cmd.noCommandResponse(r, ctx, commandForError) {
-		return
+		return responder.Errored()
 	}
 
 	if showHelp {
 		cmd.out.Println(r.help)
-		return
+		return false
 	}
 
 	ctx.responder.Respond(r.handler(ctx))
+
+	return responder.Errored()
 }
 
 // parseArgs parses command line arguments and returns subCommand, showHelp flag, and firstArg.

pkg/gofr/cmd/responder.go

@@ -5,15 +5,29 @@ import (
 	"os"
 )
 
-type Responder struct{}
+// Responder writes a command's output to stdout and its error to stderr.
+// It records whether an error was responded with, so the caller can set a
+// non-zero process exit code (shells and CI rely on the exit status to detect
+// a failed command).
+type Responder struct {
+	errored bool
+}
 
-func (*Responder) Respond(data any, err error) {
-	// TODO - provide proper exit codes here. Using os.Exit directly is a problem for tests.
+// Respond writes data to stdout and err to stderr. If err is non-nil, it marks
+// the responder as errored so the process can later exit with a non-zero status.
+func (r *Responder) Respond(data any, err error) {
 	if data != nil {
 		fmt.Fprintln(os.Stdout, data)
 	}
 
 	if err != nil {
 		fmt.Fprintln(os.Stderr, err)
+
+		r.errored = true
 	}
 }
+
+// Errored reports whether Respond was called with a non-nil error.
+func (r *Responder) Errored() bool {
+	return r.errored
+}

pkg/gofr/cmd/responder_test.go

@@ -24,3 +24,25 @@ func TestResponder_Respond(t *testing.T) {
 
 	assert.Equal(t, "error\n", err, "TEST Failed.\n", "Responder stderr output")
 }
+
+func TestResponder_Errored(t *testing.T) {
+	t.Run("no error responded", func(t *testing.T) {
+		r := Responder{}
+
+		_ = testutil.StdoutOutputForFunc(func() {
+			r.Respond("data", nil)
+		})
+
+		assert.False(t, r.Errored(), "Errored should be false when Respond is called without an error")
+	})
+
+	t.Run("error responded", func(t *testing.T) {
+		r := Responder{}
+
+		_ = testutil.StderrOutputForFunc(func() {
+			r.Respond(nil, errors.New("boom")) //nolint:err113 // dynamic error is fine for this test.
+		})
+
+		assert.True(t, r.Errored(), "Errored should be true after Respond is called with an error")
+	})
+}

pkg/gofr/cmd_test.go

@@ -351,3 +351,60 @@ func Test_Run_handler_help(t *testing.T) {
 	// check that only help for the hello subcommand is printed
 	assert.Equal(t, "this a helper string for hello sub command\n", out)
 }
+
+// Test_Run_ReturnsErroredTrueOnHandlerError asserts cmd.Run reports failure when the
+// handler returns an error, which is what App.runCMD uses to set a non-zero exit code.
+func Test_Run_ReturnsErroredTrueOnHandlerError(t *testing.T) {
+	os.Args = []string{"", "fail"}
+
+	c := cmd{}
+	c.addRoute("fail", func(*Context) (any, error) {
+		return nil, errTest
+	})
+
+	var failed bool
+
+	_ = testutil.StderrOutputForFunc(func() {
+		failed = c.Run(container.NewContainer(config.NewEnvFile("", logging.NewMockLogger(logging.DEBUG))))
+	})
+
+	assert.True(t, failed, "cmd.Run should report failure when the handler returns an error")
+}
+
+// Test_Run_ReturnsErroredFalseOnSuccess asserts cmd.Run reports success when the
+// handler returns no error.
+func Test_Run_ReturnsErroredFalseOnSuccess(t *testing.T) {
+	os.Args = []string{"", "ok"}
+
+	c := cmd{}
+	c.addRoute("ok", func(*Context) (any, error) {
+		return "done", nil
+	})
+
+	var failed bool
+
+	_ = testutil.StdoutOutputForFunc(func() {
+		failed = c.Run(container.NewContainer(config.NewEnvFile("", logging.NewMockLogger(logging.DEBUG))))
+	})
+
+	assert.False(t, failed, "cmd.Run should report success when the handler returns no error")
+}
+
+// Test_Run_ReturnsErroredTrueOnUnknownCommand asserts cmd.Run reports failure when the
+// requested subcommand does not exist.
+func Test_Run_ReturnsErroredTrueOnUnknownCommand(t *testing.T) {
+	os.Args = []string{"", "does-not-exist"}
+
+	c := cmd{}
+	c.addRoute("ok", func(*Context) (any, error) {
+		return "done", nil
+	})
+
+	var failed bool
+
+	_ = testutil.StderrOutputForFunc(func() {
+		failed = c.Run(container.NewContainer(config.NewEnvFile("", logging.NewMockLogger(logging.DEBUG))))
+	})
+
+	assert.True(t, failed, "cmd.Run should report failure for an unknown command")
+}

pkg/gofr/run.go

@@ -9,6 +9,7 @@ import (
 	"os/signal"
 	"sync"
 	"syscall"
+	"testing"
 	"time"
 )
 
@@ -21,25 +22,43 @@ const telemetryFlushTimeout = 10 * time.Second
 // final metric window and the pending span batch would otherwise be dropped when
 // the process exits, which for a CLI invocation is every window and every batch.
 // The flush is bounded by telemetryFlushTimeout so an unreachable collector
-// cannot hang the invocation.
+// cannot hang the invocation. If the command failed, it exits non-zero after the
+// flush so shells and CI can detect the failure.
 func (a *App) runCMD() {
-	a.cmd.Run(a.container)
+	failed := a.cmd.Run(a.container)
 
-	if a.container != nil {
-		flushCtx, cancel := context.WithTimeout(context.Background(), telemetryFlushTimeout)
-		defer cancel()
+	a.flushCMDTelemetry()
 
-		if err := a.container.ShutdownMetrics(flushCtx); err != nil {
-			a.Logger().Errorf("failed to flush metrics: %v", err)
-		}
+	if closer, ok := a.container.Logger.(io.Closer); ok {
+		closer.Close()
+	}
 
-		if err := a.shutdownTraces(flushCtx); err != nil {
-			a.Logger().Errorf("failed to flush traces: %v", err)
-		}
+	// Exit non-zero (after telemetry is flushed and the logger is closed) so a failed
+	// command is detectable by shells and CI. Skipped under `go test` so in-process
+	// tests that invoke Run — including apps' own main() tests — are not terminated.
+	if failed && !testing.Testing() {
+		//nolint:revive // exit status 1 signals the failed command to shells and CI
+		os.Exit(1)
 	}
+}
 
-	if closer, ok := a.container.Logger.(io.Closer); ok {
-		closer.Close()
+// flushCMDTelemetry flushes the final metric window and pending span batch after a
+// CMD app's handler returns. Kept separate so its deferred cancel runs before
+// runCMD's os.Exit on the failure path.
+func (a *App) flushCMDTelemetry() {
+	if a.container == nil {
+		return
+	}
+
+	flushCtx, cancel := context.WithTimeout(context.Background(), telemetryFlushTimeout)
+	defer cancel()
+
+	if err := a.container.ShutdownMetrics(flushCtx); err != nil {
+		a.Logger().Errorf("failed to flush metrics: %v", err)
+	}
+
+	if err := a.shutdownTraces(flushCtx); err != nil {
+		a.Logger().Errorf("failed to flush traces: %v", err)
 	}
 }