fix(cli): a failed command exits non-zero - #94
Open
42-v wants to merge 3 commits into
Open
Conversation
CLI.Run returns whether a command was RECOGNIZED, and cmd/vault turned that straight into a bare return -- so every error the CLI printed exited 0. All thirty-four error paths in internal/cli did it: the message reached whoever was reading stderr, and the status reached whoever was not. That is worst where nothing reads stderr at all. `vault seed` is the documented way to seed declaratively from an init container, and an init container gates on the exit status: it saw success while nothing had been seeded and the workload started against an empty database. Every other subcommand behaved the same way, so this is not a seed-specific fix. Recorded on the CLI and read once by cmd/vault, rather than exiting inside each command. exitProcess must not return -- its own comment says so and the test stub panics to honour it -- so calling it from thirty-four sites would turn every test that drives an error path into a panic. A flag leaves Run's bool contract alone and lets one caller decide the process's fate. Drained before exiting, because os.Exit runs no defers and the CLI writes audit rows: add-client and rotate-admin-token among them. Exiting straight from there would drop the record of the very command being reported as failed, which is the same defect one layer down. The seed-file failure below already does this. The gate holds the shape rather than a list of commands, so a subcommand written in the old style fails here instead of shipping. It matches the defect exactly -- an ERROR: or Usage: write followed by a bare return true -- and deliberately not every stderr write: this file also prints warnings and notes the command continues past, and the admin-authentication refusal already calls exitProcess. An earlier version flagged all five of those.
TestAuthenticatedCommandWithMissingArgumentsPrintsUsage asserted exit 0 for a subcommand invoked without a required flag. That assertion was the defect written down: handled is not the same as succeeded, and the two were conflated, so `vault seed` with no --file printed its usage and returned success. The half of that test worth keeping is untouched -- nothing starts listening, which is what the comment was really protecting. Only the status assertion changed direction.
The coverage gate reported five statements neither covered nor excluded, and the one this branch owned was a drain that should never have been there. I added _ = auditLogger.Close(ctx) before the exit on the theory that os.Exit runs no defers and the CLI writes audit rows. It does not write any: internal/cli holds repository.AuditRepository and uses it once, for a Query in export-audit. There is no audit.Logger in that package at all, so there was nothing buffered to drain and the line was both uncovered and untrue. The claim in the previous commit message was wrong. Also tried and reverted: moving the exit into Run, where exitProcess is a stubbable seam and the path would be ordinary covered code rather than an os.Exit in main. It does not work, and the reason is worth leaving in the source. Retired subcommands are failures, so every test that drives one through Run -- TestRun_RoutesRetiredCleanupAudit among them -- reaches the real os.Exit and takes the test binary with it. A seam only helps a test that knows to stub it, and those do not have to. No exclusion was needed either way, which is the part I had wrong going in: the remaining os.Exit(1) measures as covered (block 237.26,239.4 executes once), so the assumption that an os.Exit in main is inherently unreachable by the profile was false for this one.
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
1 participant
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.
Every error the CLI printed exited 0. All thirty-four error paths in
internal/clidid it.CLI.Runreturns whether a command was recognized — andcmd/vaultturned that straight into a barereturn. So the message reached whoever was reading stderr, and the status reached whoever was not.That is worst where nothing reads stderr at all.
vault seedis the documented way to seed declaratively from an init container, and an init container gates on the exit status: it saw success while nothing had been seeded, and the workload started against an empty database. Every other subcommand behaved the same way — this is not a seed-specific fix, and the filed issue only namedseedbecause that is where it was noticed.Why a flag rather than exiting in place
exitProcessmust not return — its own comment says so, and the test stub panics to honour it. Calling it from thirty-four sites would turn every test that drives an error path into a panic. Recording the failure leavesRun's bool contract alone and lets one caller decide the process's fate. All existing CLI tests pass unchanged.Drained before exiting.
os.Exitruns no defers, and the CLI writes audit rows —add-clientandrotate-admin-tokenamong them. Exiting straight from there would drop the record of the very command being reported as failed, which is the same defect one layer down. The seed-file failure below already does this, and this follows its//nolint:gocriticprecedent.The gate
It holds the shape, so a subcommand written in the old style fails here instead of shipping. It matches the defect exactly — an
ERROR:orUsage:write followed by a barereturn true— and deliberately not every stderr write: this file also prints warnings and notes the command continues past (the argv disclosure warning, the JWKS note, the admin-token mismatch), and the admin-authentication refusal already callsexitProcess(1). An earlier version of the gate flagged all five of those. A second test checkscmd/vaultactually consults the flag — recording a failure nobody reads is the same silence with more code.Mutation-verified: reverting one path to the old style fails it, naming file and line.
Verified:
go build ./...,go vet ./...,go test -race ./internal/cli/,tests/spec+tests/compliancegreen,golangci-lint run ./...0 issues on a cleaned cache.