fix(cli): exit non-zero when a command fails

e2e/cli_exit_codes_test.go

@@ -0,0 +1,123 @@
+//go:build e2e && unix
+// +build e2e,unix
+
+// Copyright (c) 2025-2026 Netresearch DTT GmbH
+// SPDX-License-Identifier: MIT
+
+package e2e
+
+import (
+	"path/filepath"
+	"strings"
+	"testing"
+)
+
+// Everything a user reaches for before ofelia ever schedules anything — the
+// version banner, the help listing, `validate` in a deploy gate — is judged by
+// its exit status long before anyone reads its output. These tests run the
+// real binary and assert that status, because the process boundary is where
+// the contract lives: a unit test can call a function and inspect its error,
+// but only the binary can be wrong about what it hands back to the shell.
+//
+// This surface previously exited 0 for every failure, which made
+// `ofelia validate --config=… || exit 1` a no-op in any pipeline that used it.
+
+// TestE2E_ExitCode_UnknownCommand pins that a typo is a failure. A shell
+// wrapper that dispatches to ofelia has nothing else to go on.
+func TestE2E_ExitCode_UnknownCommand(t *testing.T) {
+	t.Parallel()
+
+	stdout, stderr, err := runCommand(t, "definitely-not-a-command")
+	assertExitCode(t, err, 1, stdout, stderr)
+
+	// The listing is what tells the user what they should have typed.
+	if !strings.Contains(stdout+stderr, "daemon") {
+		t.Errorf("an unknown command should print the available commands, got:\nstdout=%s\nstderr=%s",
+			stdout, stderr)
+	}
+}
+
+// TestE2E_ExitCode_SuccessPaths covers the other direction: asking for
+// information succeeded, so these must not report failure. Getting this
+// backwards would break every CI step that runs `ofelia version` as a probe.
+func TestE2E_ExitCode_SuccessPaths(t *testing.T) {
+	t.Parallel()
+
+	cases := []struct {
+		name string
+		args []string
+	}{
+		{name: "version subcommand", args: []string{"version"}},
+		{name: "version flag", args: []string{"--version"}},
+		{name: "help flag", args: []string{"--help"}},
+	}
+
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			t.Parallel()
+
+			stdout, stderr, err := runCommand(t, tc.args...)
+			assertExitCode(t, err, 0, stdout, stderr)
+
+			if strings.TrimSpace(stdout+stderr) == "" {
+				t.Errorf("%v printed nothing", tc.args)
+			}
+		})
+	}
+}
+
+// TestE2E_ExitCode_StrictValidationFails complements the INI-syntax case in
+// config_validation_test.go with a file that parses but does not satisfy
+// strict validation. Both have to fail, and for a deploy gate the distinction
+// does not matter — what matters is that neither is silently accepted.
+//
+// Strict validation is opt-in (`enable-strict-validation`, default false), so
+// the config turns it on. Without it ofelia accepts semantically broken jobs
+// here — including an unparsable schedule, which the daemon then logs as a
+// warning while starting anyway, leaving a job that never fires. That is a
+// separate question from exit codes and is not pinned here.
+func TestE2E_ExitCode_StrictValidationFails(t *testing.T) {
+	t.Parallel()
+
+	configPath := writeConfig(t, `[global]
+  enable-strict-validation = true
+
+[job-local "broken"]
+  schedule = not-a-schedule
+  command = echo hi
+`)
+
+	stdout, stderr, err := runCommand(t, "validate", "--config="+configPath)
+	assertExitCode(t, err, 1, stdout, stderr)
+
+	if !strings.Contains(stdout+stderr, "validation failed") {
+		t.Errorf("expected a validation failure message, got:\nstdout=%s\nstderr=%s", stdout, stderr)
+	}
+}
+
+// TestE2E_ExitCode_ValidateIsUsableAsAGate is the test that would have caught
+// the original defect on its own: it uses validate exactly as a deployment
+// pipeline does — run it, branch on the status — over a good and a bad config,
+// and requires the two to be distinguishable.
+func TestE2E_ExitCode_ValidateIsUsableAsAGate(t *testing.T) {
+	t.Parallel()
+
+	good := writeConfig(t, `[global]
+  log-level = info
+
+[job-local "hello"]
+  schedule = @every 30s
+  command = echo hello
+`)
+	bad := filepath.Join(t.TempDir(), "missing.ini")
+
+	_, _, goodErr := runCommand(t, "validate", "--config="+good)
+	_, _, badErr := runCommand(t, "validate", "--config="+bad)
+
+	if goodErr != nil {
+		t.Errorf("a valid config was rejected: %v", goodErr)
+	}
+	if badErr == nil {
+		t.Error("a missing config was accepted; validate cannot be used as a gate")
+	}
+}

e2e/config_validation_test.go

@@ -24,11 +24,11 @@ func TestE2E_Validate_MalformedINI(t *testing.T) {
 
 	stdout, stderr, err := runCommand(t, "validate", "--config="+configPath)
 
-	// Note on exit code: ofelia.go intentionally calls `return` instead of
-	// os.Exit(1) after go-flags reports the error (see ofelia.go ~L132),
-	// so the process exits 0. We therefore assert on the human-readable
-	// error text — that's what the user actually sees in CI logs.
-	_ = err
+	// The exit status is what a pipeline reads. This used to be discarded here
+	// with a note calling the exit-0 behavior intentional, which meant the
+	// test documented the defect instead of catching it: `ofelia validate …
+	// || exit 1` could never fire on a broken config.
+	assertExitCode(t, err, 1, stdout, stderr)
 
 	combined := stdout + stderr
 	for _, needle := range []string{
@@ -49,7 +49,9 @@ func TestE2E_Validate_MissingConfigFile(t *testing.T) {
 	t.Parallel()
 
 	missingPath := filepath.Join(t.TempDir(), "does-not-exist.ini")
-	stdout, stderr, _ := runCommand(t, "validate", "--config="+missingPath)
+	stdout, stderr, err := runCommand(t, "validate", "--config="+missingPath)
+
+	assertExitCode(t, err, 1, stdout, stderr)
 
 	combined := stdout + stderr
 	for _, needle := range []string{
@@ -84,7 +86,11 @@ func TestE2E_Validate_AcceptsValidConfig(t *testing.T) {
 `
 
 	configPath := writeConfig(t, configBody)
-	stdout, stderr, _ := runCommand(t, "validate", "--config="+configPath)
+	stdout, stderr, err := runCommand(t, "validate", "--config="+configPath)
+
+	// The counterpart to the failure cases: a good config must exit 0, or a
+	// pipeline that gates on validate would reject every deployment.
+	assertExitCode(t, err, 0, stdout, stderr)
 
 	// JSON dump should mention both jobs we defined.
 	for _, needle := range []string{`"hello"`, `"world"`, `"Image": "alpine:3.20"`} {

e2e/helpers_test.go

@@ -345,3 +345,26 @@ func dockerRemove(t *testing.T, name string) {
 	cmd.Stderr = io.Discard
 	_ = cmd.Run()
 }
+
+// assertExitCode fails the test unless the command exited with want.
+//
+// The exit status is the only thing a shell, a Makefile or a CI step can act
+// on, so it is asserted separately from the human-readable output: a command
+// that prints a clear error and still exits 0 is indistinguishable from
+// success to everything except a person reading the log.
+func assertExitCode(t *testing.T, runErr error, want int, stdout, stderr string) {
+	t.Helper()
+
+	got := 0
+	if runErr != nil {
+		var exitErr *exec.ExitError
+		if !errors.As(runErr, &exitErr) {
+			t.Fatalf("command failed to run at all: %v", runErr)
+		}
+		got = exitErr.ExitCode()
+	}
+
+	if got != want {
+		t.Errorf("exit code = %d, want %d\nstdout=%s\nstderr=%s", got, want, stdout, stderr)
+	}
+}

ofelia.go

@@ -43,22 +43,35 @@ func buildLogger(level string) (*slog.Logger, *slog.LevelVar) {
 	return slog.New(handler), levelVar
 }
 
+// Exit codes. A command that failed has to say so in the only channel a shell
+// reads, otherwise `ofelia validate … || exit 1` in a pipeline never fires and
+// a broken config sails through the gate that exists to stop it.
+const (
+	exitOK      = 0
+	exitFailure = 1
+)
+
 func main() {
+	os.Exit(run(os.Args[1:]))
+}
+
+// run holds what main used to do and returns the process exit code instead of
+// ending the process, so the exit status is a value tests can assert on.
+func run(args []string) int {
 	cli.Version = version
 	cli.Build = build
 
 	// Handle --version flag before parser setup
-	if slices.Contains(os.Args[1:], "--version") {
+	if slices.Contains(args, "--version") {
 		_, _ = fmt.Fprintln(os.Stdout, cli.VersionString())
-		return
+		return exitOK
 	}
 
 	// Pre-parse log-level flag to configure logger early
 	var pre struct {
 		LogLevel   string `long:"log-level"`
 		ConfigFile string `long:"config" default:"/etc/ofelia/config.ini"`
 	}
-	args := os.Args[1:]
 	preParser := flags.NewParser(&pre, flags.IgnoreUnknown)
 	_, _ = preParser.ParseArgs(args)
 
@@ -118,8 +131,10 @@ func main() {
 	)
 
 	if _, err := parser.ParseArgs(args); err != nil {
+		// Help was asked for and printed. That is the command doing its job,
+		// not a failure.
 		if flags.WroteHelp(err) {
-			return
+			return exitOK
 		}
 
 		var flagErr *flags.Error
@@ -128,7 +143,14 @@ func main() {
 			_, _ = fmt.Fprintf(os.Stdout, "\n%s\n", cli.VersionString())
 		}
 
-		logger.Error("Command failed to execute")
-		return // Exit gracefully instead of os.Exit(1)
+		// Every other error — an unusable config, a subcommand that returned
+		// an error, an unknown command — is a failure and has to leave a
+		// non-zero status behind. This used to return 0 with a logged message,
+		// which meant `ofelia validate … || exit 1` could not fire and a
+		// broken config passed the gate meant to catch it.
+		logger.Error("Command failed to execute", "error", err)
+		return exitFailure
 	}
+
+	return exitOK
 }