From 24be2f556c62495660f827b4529a464f7de5a842 Mon Sep 17 00:00:00 2001
From: sneak <sneak@sneak.berlin>
Date: Sun, 4 Oct 2026 07:35:20 +0000
Subject: [PATCH] Print usage only for a command called wrongly (closes #41)

A failed command printed the whole usage text after its error, burying
it. The root command's PersistentPreRunE now turns usage off once cobra
has checked the arguments and flags, so an error from running the
command is printed once on its own. Wrong arity, an unknown flag, a bad
flag value and a missing required flag still get usage; cobra checks
required flags after that hook, so the hook checks them first. Root
SilenceUsage was not used: in this cobra version it hides usage for
argument and flag errors too. Cobra still prints the error; Entry is
unchanged.

Model: opus-5-5
---
 TODO.md                    |  9 ++++++--
 internal/cli/root.go       | 19 ++++++++++++++--
 internal/cli/usage_test.go | 46 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 70 insertions(+), 4 deletions(-)
 create mode 100644 internal/cli/usage_test.go

diff --git a/TODO.md b/TODO.md
index 99d609e..0fb5321 100644
--- a/TODO.md
+++ b/TODO.md
@@ -25,6 +25,13 @@ 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 or missing required flag. The root
+  command's `PersistentPreRunE` turns usage off once cobra has checked
+  the arguments and flags; root `SilenceUsage` would have hidden usage
+  for those too.
 - 2026-10-04: `.gitignore` is the org's standard file, which ignores
   `.env`, `.env.*`, `*.pem` and `*.key` and editor and OS files, plus
   this repo's `/secret`, `*.log`, `*.test` and `settings.local.json`
@@ -203,8 +210,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..6088dc4 100644
--- a/internal/cli/root.go
+++ b/internal/cli/root.go
@@ -46,9 +46,24 @@ 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 flags before this runs, except for
+		// required flags, which are checked here so they still get usage.
+		// 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
+			}
+
+			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)
+	}
+}