From eb8805dd2966f20e174c52b87a282b74e7e7fd95 Mon Sep 17 00:00:00 2001 From: Sebastian Mendel Date: Sun, 2 Aug 2026 12:18:42 +0200 Subject: [PATCH 1/3] fix(cli): exit non-zero when a command fails MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every failing invocation exited 0: ofelia validate --config=/does/not/exist -> 0 ofelia validate -> 0 ofelia bogus-command -> 0 So `ofelia validate --config=… || exit 1` never fired. Any pipeline, Makefile target or deploy gate that ran validate to stop a broken config let it through, and the only sign was a log line nothing was reading. The cause is not a design decision, despite the comment saying so. The line came in with 242cf6d, a linting cleanup, and "Exit gracefully instead of os.Exit(1)" reads as a rationalisation of that fix. Nothing enforces it now: os.Exit(1) in that position produces no findings under the current .golangci.yml, since revive's deep-exit rule is not enabled. main() now does nothing but exit with what run() returns, so the status is a value rather than a process-ending side effect — which is also what makes it assertable. Help and --version stay 0; asking for information and receiving it is not a failure. Everything else is 1, and the error now travels with the log line instead of being dropped. The e2e test for the malformed-INI case discarded the exit status with `_ = err` and a note calling exit 0 intentional, so it documented the defect rather than catching it. It now asserts the status, as do the missing-file and happy-path cases. Signed-off-by: Sebastian Mendel --- e2e/config_validation_test.go | 20 +++++++++++++------- ofelia.go | 34 ++++++++++++++++++++++++++++------ 2 files changed, 41 insertions(+), 13 deletions(-) diff --git a/e2e/config_validation_test.go b/e2e/config_validation_test.go index 39f4a3df12..dec1bb123e 100644 --- a/e2e/config_validation_test.go +++ b/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"`} { diff --git a/ofelia.go b/ofelia.go index 1d08de20e8..81dae7e5c2 100644 --- a/ofelia.go +++ b/ofelia.go @@ -43,14 +43,28 @@ 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 @@ -58,7 +72,6 @@ func main() { 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 } From 824da36c668f7b6492f2dfb5f2dddfce84ab0f6f Mon Sep 17 00:00:00 2001 From: Sebastian Mendel Date: Sun, 2 Aug 2026 12:18:57 +0200 Subject: [PATCH 2/3] test: assert the exit status of the user-facing commands MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The exit status is the only part of a CLI a shell, a Makefile or a CI step can act on, and nothing asserted it. That is how a validate command that could not fail survived: the tests read its output, which was correct, and never looked at what it handed back. e2e covers the process boundary, which is where the contract lives — a unit test can inspect an error value, but only the binary can be wrong about the status it returns. Added: unknown command fails; version, the --version flag and --help succeed; strict validation rejects a bad config; and validate used the way a deploy gate uses it — run it over a good and a bad config and branch on the status — must tell the two apart. That last one would have caught the original defect on its own. The unit tests move from main() to run() so they can assert the code too, and two of them were fixed rather than kept: they invoked `--log-level`/`--config` before the subcommand, where the top-level parser rejects them as unknown flags, and only passed because everything returned 0. They now use the position that works. That placement asymmetry is a separate defect and is deliberately not pinned here: both flags are pre-parsed from anywhere in argv, but the top-level parser declares neither, so `ofelia --config=x validate` fails while `ofelia validate --config=x` works. Signed-off-by: Sebastian Mendel --- e2e/cli_exit_codes_test.go | 123 +++++++++++++++++++++++++++++++++++++ e2e/helpers_test.go | 23 +++++++ ofelia_main_test.go | 104 +++++++++++++++++++------------ 3 files changed, 210 insertions(+), 40 deletions(-) create mode 100644 e2e/cli_exit_codes_test.go diff --git a/e2e/cli_exit_codes_test.go b/e2e/cli_exit_codes_test.go new file mode 100644 index 0000000000..1ffb8b6cf2 --- /dev/null +++ b/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") + } +} diff --git a/e2e/helpers_test.go b/e2e/helpers_test.go index dbbe203d73..6e2cee476f 100644 --- a/e2e/helpers_test.go +++ b/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) + } +} diff --git a/ofelia_main_test.go b/ofelia_main_test.go index 3e9a944f24..1c7046a4b0 100644 --- a/ofelia_main_test.go +++ b/ofelia_main_test.go @@ -5,31 +5,29 @@ package main import ( "os" + "path/filepath" "strings" "testing" ) -// main() is reachable from a test because every one of its exits is a plain -// return: the flag-error path was deliberately changed to return instead of -// calling os.Exit(1) (see the comment at its final return). Exercising it here -// covers the command wiring — a command dropped from the parser, or a -// constructor that starts panicking, fails these tests instead of only showing -// up when a user runs the binary. +// run() is what main() does, minus the call to os.Exit, so a test can assert +// on the exit code the process would have produced. Exercising it here covers +// the command wiring — a command dropped from the parser, or a constructor +// that starts panicking, fails these tests instead of only showing up when a +// user runs the binary — and, since the exit code is the only thing a shell +// reads, that each outcome maps to the right status. // // These tests never pass a real command such as `daemon`, which would start a // scheduler. Only paths that parse and return are used. -// runMain invokes main() with the given argv while redirecting stdout, so the -// parser's help output does not drown the test log. It restores both os.Args -// and os.Stdout before returning and reports what main() printed. -func runMain(t *testing.T, argv ...string) string { +// runMain invokes run() with the given argv while redirecting stdout, so the +// parser's help output does not drown the test log. It restores os.Stdout +// before returning and reports both what was printed and the exit code. +func runMain(t *testing.T, argv ...string) (string, int) { t.Helper() - origArgs, origStdout := os.Args, os.Stdout - t.Cleanup(func() { - os.Args = origArgs - os.Stdout = origStdout - }) + origStdout := os.Stdout + t.Cleanup(func() { os.Stdout = origStdout }) r, w, err := os.Pipe() if err != nil { @@ -38,7 +36,7 @@ func runMain(t *testing.T, argv ...string) string { // A pipe's buffer is finite and the help text is long, so drain it // concurrently; writing more than the buffer holds would otherwise block - // main() forever. + // run() forever. captured := make(chan string, 1) go func() { var sb strings.Builder @@ -55,15 +53,14 @@ func runMain(t *testing.T, argv ...string) string { captured <- sb.String() }() - os.Args = append([]string{"ofelia"}, argv...) os.Stdout = w - main() + code := run(argv) _ = w.Close() out := <-captured _ = r.Close() - return out + return out, code } // TestMain_VersionFlag covers the short-circuit before the parser is built: @@ -72,10 +69,13 @@ func runMain(t *testing.T, argv ...string) string { //nolint:paralleltest // mutates os.Args and os.Stdout, which are process-global func TestMain_VersionFlag(t *testing.T) { // No t.Parallel(): os.Args and os.Stdout are process-global. - out := runMain(t, "--version") + out, code := runMain(t, "--version") if strings.TrimSpace(out) == "" { t.Error("--version printed nothing") } + if code != exitOK { + t.Errorf("--version exit code = %d, want %d", code, exitOK) + } } // TestMain_Help covers the full command-registration path and the @@ -84,10 +84,15 @@ func TestMain_VersionFlag(t *testing.T) { // //nolint:paralleltest // mutates os.Args and os.Stdout, which are process-global func TestMain_Help(t *testing.T) { - out := runMain(t, "--help") + out, code := runMain(t, "--help") + + // Asking for help and getting it is the command succeeding, not failing. + if code != exitOK { + t.Errorf("--help exit code = %d, want %d", code, exitOK) + } // Each registered command should appear in the help output. This is what - // turns the test from "main did not panic" into a check that the command + // turns the test from "run did not panic" into a check that the command // set is intact. for _, cmd := range []string{"daemon", "validate", "config", "init", "doctor", "hash-password", "version"} { if !strings.Contains(out, cmd) { @@ -101,10 +106,14 @@ func TestMain_Help(t *testing.T) { // //nolint:paralleltest // mutates os.Args and os.Stdout, which are process-global func TestMain_UnknownCommand(t *testing.T) { - out := runMain(t, "definitely-not-a-command") + out, code := runMain(t, "definitely-not-a-command") if !strings.Contains(out, "daemon") { t.Errorf("an unknown command should print the help listing; got %q", out) } + // The status is the only part a shell can act on. + if code != exitFailure { + t.Errorf("unknown command exit code = %d, want %d", code, exitFailure) + } } // TestMain_NoArguments covers the same error branch reached with no command at @@ -112,34 +121,49 @@ func TestMain_UnknownCommand(t *testing.T) { // //nolint:paralleltest // mutates os.Args and os.Stdout, which are process-global func TestMain_NoArguments(t *testing.T) { - out := runMain(t) + out, code := runMain(t) if strings.TrimSpace(out) == "" { t.Error("a bare invocation printed nothing; expected the help listing") } + if code != exitFailure { + t.Errorf("bare invocation exit code = %d, want %d (no command was given)", code, exitFailure) + } } -// TestMain_LogLevelFlagIsPreParsed pins that --log-level is consumed by the -// pre-parser before the real parser runs, so an early log level applies to -// everything the commands log. It reaches the same help path, but with the -// pre-parse branch populated. +// TestMain_LogLevelFromConfigFile covers the pre-parse branch that reads the +// log level out of the INI when no --log-level flag was given, which is how a +// containerised deployment configures it. +// +// The flag is passed after the subcommand on purpose: --config and --log-level +// are pre-parsed from anywhere in argv, but the top-level parser does not +// declare them, so `ofelia --config=x validate` is rejected as an unknown flag +// while `ofelia validate --config=x` works. That placement asymmetry is a +// separate defect, not something this test should bake in. // //nolint:paralleltest // mutates os.Args and os.Stdout, which are process-global -func TestMain_LogLevelFlagIsPreParsed(t *testing.T) { - out := runMain(t, "--log-level", "debug", "--help") - if !strings.Contains(out, "daemon") { - t.Errorf("expected help output with a log level set; got %q", out) +func TestMain_LogLevelFromConfigFile(t *testing.T) { + iniPath := filepath.Join(t.TempDir(), "ofelia.ini") + body := "[global]\n log-level = debug\n\n[job-local \"noop\"]\n schedule = @every 1h\n command = true\n" + if err := os.WriteFile(iniPath, []byte(body), 0o600); err != nil { + t.Fatalf("writing config: %v", err) + } + + out, code := runMain(t, "validate", "--config="+iniPath) + if code != exitOK { + t.Errorf("validating a good config exited %d, want %d; output:\n%s", code, exitOK, out) } } -// TestMain_ConfigFlagMissingFileIsTolerated pins that a --config path which -// does not exist does not stop startup: the INI load is best-effort and only -// supplies a log level. +// TestMain_ValidateMissingConfigFails pins the status a scripted caller reads: +// a config file that is not there is a failure, and `ofelia validate … || …` +// has to be able to see it. // //nolint:paralleltest // mutates os.Args and os.Stdout, which are process-global -func TestMain_ConfigFlagMissingFileIsTolerated(t *testing.T) { - missing := t.TempDir() + "/no-such-config.ini" - out := runMain(t, "--config", missing, "--help") - if !strings.Contains(out, "daemon") { - t.Errorf("a missing --config file should be tolerated; got %q", out) +func TestMain_ValidateMissingConfigFails(t *testing.T) { + missing := filepath.Join(t.TempDir(), "no-such-config.ini") + + _, code := runMain(t, "validate", "--config="+missing) + if code != exitFailure { + t.Errorf("validating a missing config exited %d, want %d", code, exitFailure) } } From 698d1b9e81a3d1b63f9a914823ad0cd4e3a2f91b Mon Sep 17 00:00:00 2001 From: Sebastian Mendel Date: Sun, 2 Aug 2026 12:28:01 +0200 Subject: [PATCH 3/3] test: correct the paralleltest suppression reason Switching these tests from main() to run(argv) removed the os.Args mutation, but the suppression comments still named it. They are suppressed because they replace os.Stdout, which is process-global on its own; the reason now says only that. Review finding from Copilot on PR #771. Signed-off-by: Sebastian Mendel --- ofelia_main_test.go | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/ofelia_main_test.go b/ofelia_main_test.go index 1c7046a4b0..76f1789cb2 100644 --- a/ofelia_main_test.go +++ b/ofelia_main_test.go @@ -66,9 +66,9 @@ func runMain(t *testing.T, argv ...string) (string, int) { // TestMain_VersionFlag covers the short-circuit before the parser is built: // --version must print and return without constructing any command. // -//nolint:paralleltest // mutates os.Args and os.Stdout, which are process-global +//nolint:paralleltest // replaces os.Stdout, which is process-global func TestMain_VersionFlag(t *testing.T) { - // No t.Parallel(): os.Args and os.Stdout are process-global. + // No t.Parallel(): os.Stdout is process-global. out, code := runMain(t, "--version") if strings.TrimSpace(out) == "" { t.Error("--version printed nothing") @@ -82,7 +82,7 @@ func TestMain_VersionFlag(t *testing.T) { // flags.WroteHelp branch: every AddCommand call runs before the parser reports // that it wrote help. // -//nolint:paralleltest // mutates os.Args and os.Stdout, which are process-global +//nolint:paralleltest // replaces os.Stdout, which is process-global func TestMain_Help(t *testing.T) { out, code := runMain(t, "--help") @@ -104,7 +104,7 @@ func TestMain_Help(t *testing.T) { // TestMain_UnknownCommand covers the flags.Error branch, which prints help plus // the version string and returns rather than exiting the process. // -//nolint:paralleltest // mutates os.Args and os.Stdout, which are process-global +//nolint:paralleltest // replaces os.Stdout, which is process-global func TestMain_UnknownCommand(t *testing.T) { out, code := runMain(t, "definitely-not-a-command") if !strings.Contains(out, "daemon") { @@ -119,7 +119,7 @@ func TestMain_UnknownCommand(t *testing.T) { // TestMain_NoArguments covers the same error branch reached with no command at // all, which is what a bare `ofelia` invocation does. // -//nolint:paralleltest // mutates os.Args and os.Stdout, which are process-global +//nolint:paralleltest // replaces os.Stdout, which is process-global func TestMain_NoArguments(t *testing.T) { out, code := runMain(t) if strings.TrimSpace(out) == "" { @@ -140,7 +140,7 @@ func TestMain_NoArguments(t *testing.T) { // while `ofelia validate --config=x` works. That placement asymmetry is a // separate defect, not something this test should bake in. // -//nolint:paralleltest // mutates os.Args and os.Stdout, which are process-global +//nolint:paralleltest // replaces os.Stdout, which is process-global func TestMain_LogLevelFromConfigFile(t *testing.T) { iniPath := filepath.Join(t.TempDir(), "ofelia.ini") body := "[global]\n log-level = debug\n\n[job-local \"noop\"]\n schedule = @every 1h\n command = true\n" @@ -158,7 +158,7 @@ func TestMain_LogLevelFromConfigFile(t *testing.T) { // a config file that is not there is a failure, and `ofelia validate … || …` // has to be able to see it. // -//nolint:paralleltest // mutates os.Args and os.Stdout, which are process-global +//nolint:paralleltest // replaces os.Stdout, which is process-global func TestMain_ValidateMissingConfigFails(t *testing.T) { missing := filepath.Join(t.TempDir(), "no-such-config.ini")