diff --git a/TODO.md b/TODO.md index 3612f96..37a4c1c 100644 --- a/TODO.md +++ b/TODO.md @@ -25,6 +25,15 @@ Bring the repo into policy compliance in one commit: # Completed Steps +- 2026-10-04: A failed command prints its error once, without the usage + text after it (https://git.eeqj.de/sneak/secret/issues/41). Usage is + still printed for a command called wrongly: wrong number of arguments, + unknown flag, bad flag value, missing required flag, or flags that + break a flag group (mutually exclusive, required together, one + required). The root command's `PersistentPreRunE` turns usage off. + Cobra checks arguments and flag values before that hook but required + flags and flag groups only after it, so the hook checks those two + first. Root `SilenceUsage` would have hidden usage for all of these. - 2026-10-04: `secret get` keeps the secret in locked memory until it writes it out (https://git.eeqj.de/sneak/secret/issues/37): `Vault.GetSecret` and `Vault.GetSecretVersion` return a @@ -224,8 +233,6 @@ Bring the repo into policy compliance in one commit: 209-216); non-constant-time public key compare (vault.go:95-100). - High priority: - Secure temporary file handling and cleanup. - - Print cobra usage only for argument errors, not internal - failures. - Initialize a default unlock key at vault creation. - Confirmation prompts for destructive operations (keys rm, vault deletion). diff --git a/internal/cli/root.go b/internal/cli/root.go index c911991..2756cf6 100644 --- a/internal/cli/root.go +++ b/internal/cli/root.go @@ -46,9 +46,30 @@ func newRootCmd() *cobra.Command { Short: "A simple secrets manager", Long: `A simple secrets manager to store and retrieve sensitive ` + `information securely.`, - // Ensure usage is shown after errors - SilenceUsage: false, + // Cobra prints the error a command returns; Entry does not. SilenceErrors: false, + // Usage belongs only to a command called wrongly. Cobra has + // checked its arguments and flag values before this runs, but + // checks required flags (ValidateRequiredFlags) and flag groups + // (ValidateFlagGroups) only after it, so both are checked here + // to keep usage for them. An error after that comes from running + // the command, and usage would only bury it. A subcommand that + // sets its own PersistentPreRun replaces this one. + PersistentPreRunE: func(cmd *cobra.Command, _ []string) error { + err := cmd.ValidateRequiredFlags() + if err != nil { + return err + } + + err = cmd.ValidateFlagGroups() + if err != nil { + return err + } + + cmd.SilenceUsage = true + + return nil + }, } secret.Debug("Adding subcommands to root command") diff --git a/internal/cli/usage_test.go b/internal/cli/usage_test.go new file mode 100644 index 0000000..185cd9d --- /dev/null +++ b/internal/cli/usage_test.go @@ -0,0 +1,46 @@ +package cli_test + +import ( + "strings" + "testing" + + "git.eeqj.de/sneak/secret/internal/cli" + "git.eeqj.de/sneak/secret/internal/secret" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// usageHeading starts the usage text cobra prints after an error. +const usageHeading = "Usage:" + +// A command called wrongly gets usage after its error; a command that +// fails while running gets its error alone. Either way the command fails +// and its error is shown exactly once. +// +//nolint:paralleltest // executes the CLI in-process and sets the environment +func TestUsageOnlyForCallErrors(t *testing.T) { + // No vault in the state directory, so `get x` fails while running. + env := map[string]string{secret.EnvStateDir: t.TempDir()} + + tests := []struct { + call string + wantUsage bool + }{ + {call: "get", wantUsage: true}, + {call: "get x y", wantUsage: true}, + {call: "get --no-such-flag x", wantUsage: true}, + {call: "generate secret x --length abc", wantUsage: true}, + {call: "import x", wantUsage: true}, + {call: "get x", wantUsage: false}, + } + + for _, tt := range tests { + output, err := cli.ExecuteCommandInProcess(strings.Fields(tt.call), "", env) + require.Error(t, err, "%q should fail", tt.call) + + assert.Equal(t, 1, strings.Count(output, err.Error()), + "%q should show its error once:\n%s", tt.call, output) + assert.Equal(t, tt.wantUsage, strings.Contains(output, usageHeading), + "usage shown for %q:\n%s", tt.call, output) + } +}