fix(cli): exit non-zero when a command fails - #771
Merged
Conversation
Every failing invocation exited 0:
ofelia validate --config=/does/not/exist -> 0
ofelia validate <malformed ini> -> 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 <github@sebastianmendel.de>
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 <github@sebastianmendel.de>
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Automated approval for maintainer PR
All automated quality gates passed. See SECURITY_CONTROLS.md for compensating controls.
|
There was a problem hiding this comment.
Pull request overview
This PR fixes Ofelia’s CLI contract so failures (invalid config, malformed INI, unknown commands, command execution errors) return a non-zero process exit code, making ofelia validate … usable as a deployment/CI gate as intended.
Changes:
- Refactor
main()intorun(args []string) intand havemain()exit with the returned status code. - Ensure help/version paths exit
0, while all other parse/execute errors exit1(and log the underlying error). - Add/adjust unit + e2e tests to assert exit-status behavior at the process boundary.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
ofelia.go |
Introduces run() returning exit codes and makes main() exit with that code; returns non-zero on command failures. |
ofelia_main_test.go |
Updates unit tests to call run() and assert exit codes for version/help/unknown/no-args/validate paths. |
e2e/helpers_test.go |
Adds assertExitCode helper to reliably assert subprocess exit status in e2e tests. |
e2e/config_validation_test.go |
Stops discarding subprocess errors; asserts that invalid configs exit non-zero and valid configs exit zero. |
e2e/cli_exit_codes_test.go |
New e2e coverage for exit-code behavior across unknown commands, help/version, strict validation failures, and validate-as-gate scenarios. |
Suppressed comments (5)
ofelia_main_test.go:85
- This
paralleltestsuppression comment still mentionsos.Args, but these tests now callrun(argv)and only redirectos.Stdout. Please update the justification to reflect the actual shared global state.
//nolint:paralleltest // mutates os.Args and os.Stdout, which are process-global
ofelia_main_test.go:107
- This
paralleltestsuppression comment still mentionsos.Args, but these tests now callrun(argv)and only redirectos.Stdout. Please update the justification to reflect the actual shared global state.
//nolint:paralleltest // mutates os.Args and os.Stdout, which are process-global
ofelia_main_test.go:122
- This
paralleltestsuppression comment still mentionsos.Args, but these tests now callrun(argv)and only redirectos.Stdout. Please update the justification to reflect the actual shared global state.
//nolint:paralleltest // mutates os.Args and os.Stdout, which are process-global
ofelia_main_test.go:143
- This
paralleltestsuppression comment still mentionsos.Args, but these tests now callrun(argv)and only redirectos.Stdout. Please update the justification to reflect the actual shared global state.
//nolint:paralleltest // mutates os.Args and os.Stdout, which are process-global
ofelia_main_test.go:161
- This
paralleltestsuppression comment still mentionsos.Args, but these tests now callrun(argv)and only redirectos.Stdout. Please update the justification to reflect the actual shared global state.
//nolint:paralleltest // mutates os.Args and os.Stdout, which are process-global
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #771 +/- ##
==========================================
- Coverage 89.06% 89.03% -0.03%
==========================================
Files 90 90
Lines 12059 12061 +2
==========================================
- Hits 10740 10739 -1
- Misses 1022 1025 +3
Partials 297 297
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. |
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 <github@sebastianmendel.de>
|
There was a problem hiding this comment.
Automated approval for maintainer PR
All automated quality gates passed. See SECURITY_CONTROLS.md for compensating controls.
This was referenced Aug 2, 2026
Closed
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
ofelia validatecould not failEvery failing invocation exited 0:
ofelia validate --config=/does/not/existofelia validateon a malformed INIofelia bogus-commandofelia version/--version/--helpSo
ofelia validate --config=… || exit 1never fired. Every pipeline, Makefile target and deploy gate that ran validate to stop a broken config let it through, and the only sign was a log line nothing reads.It was not a design decision, despite the comment saying so. The line came in with
242cf6d— a linting cleanup ("comprehensive linting resolution") — and "Exit gracefully instead of os.Exit(1)" reads as a rationalisation of that fix. Nothing enforces it now either:os.Exit(1)in that position produces zero findings under the current.golangci.yml, because revive'sdeep-exitrule is not enabled. Verified by putting it there and running the linter.And it had been documented into permanence.
e2e/config_validation_test.godiscarded the status with_ = errunder a note calling exit 0 intentional — a test that recorded the defect instead of catching it.main()now does nothing but exit with whatrun()returns, so the status is a value rather than a process-ending side effect, which is also what makes it assertable.Tests
The exit status is the only part of a CLI that a shell can act on, and nothing asserted it — which is exactly how this survived: the tests read the output, which was correct all along.
New e2e coverage at the process boundary, where the contract actually lives: unknown command fails;
version,--versionand--helpsucceed; strict validation rejects a bad config; andvalidateused the way a deploy gate uses it — run over a good and a bad config, branch on the status — must tell the two apart. That last test would have caught the original defect on its own.The unit tests move from
main()torun()so they can assert the code too. Two of them were fixed rather than kept: they passed--log-level/--configbefore the subcommand and only passed because everything returned 0.Three findings this turned up, deliberately not fixed here
Each is a separate contract change and belongs in its own PR:
Global flags are rejected before the subcommand.
--log-leveland--configare pre-parsed from anywhere in argv, but the top-level parser declares neither, soofelia --config=x validatefails withunknown flag `config'whileofelia validate --config=xworks.A bad schedule passes validation and the daemon starts anyway. With
enable-strict-validationoff (the default),schedule = not-a-schedulevalidates clean; the daemon then logsWARN Failed to register job "broken"and starts withjobCount=1, leaving a job that never fires and nothing that stops a deployment.Strict validation demands web-auth fields regardless. Turning it on for a config with no web UI fails on
web-password-hashandweb-secret-keybeing "required".Behaviour change to be aware of
Pipelines that today run past a broken config will start failing. That is the point, but it will surface.
Test plan
go test ./...— green, coverage 90.09%go test -race -tags=e2e ./e2e/...— greengolangci-lint runincl.--build-tags="e2e unix"— 0 issueslefthook run pre-push— exit 0