Unknown subcommands of vault, unlocker, version and generate fail (closes #183)

Given a subcommand they do not have, `secret vault`, `secret unlocker`,
`secret version` and `secret generate` printed their help and exited 0,
so a mistyped command looked like a successful one. Each now exits 1
with `unknown command "<name>" for "secret <parent>"`, and with no
arguments still prints its help.

Cobra checks a command's Args only when it has a RunE, so each parent
gets `Args: cobra.NoArgs` and a RunE that prints its help. Args alone
changes nothing. TestNoVaultImportCommand required the old answer to
`secret vault import` and now requires the error.

Judgement call: their help now shows a `secret <parent> [flags]` usage
line, which cobra adds for any command that runs.
Judgement call: `secret vault help` now fails as an unknown subcommand;
`secret help vault` and `secret vault --help` still work.

Model: opus-5-5

README.md

@@ -142,11 +142,7 @@ value may be over 100 MiB, whatever the limit.
 
 #### `secret add <secret-name> [--force]`
 
-Adds a secret to the current vault. Reads the secret value from stdin. When the
-value ends in a newline, that one newline is dropped, so
-`echo value | secret add name` stores `value`; from `printf 'a\n\n'` it stores
-`a` and one newline. To keep a file's final newline, as in a PEM key, use
-`secret import`, which stores the file's bytes unchanged.
+Adds a secret to the current vault. Reads the secret value from stdin.
 
 - `--force, -f`: Overwrite existing secret
 
@@ -159,13 +155,13 @@ contain `//`, or have `..` as a path segment.
 
 #### `secret get <secret-name> [--version <version>]`
 
-Writes the secret's value to stdout exactly as stored, with no newline added
-after it. A failed write of the value, such as to a file on a full disk, prints
-the error and exits 1, with two exceptions. If the program reading from a pipe
-has already exited, as in `secret get s | true`, the broken-pipe signal
-(SIGPIPE) ends `secret` with no message and the shell reports exit status 141.
-If the shell closed stdout before `secret` started (`secret get s >&-`), the
-value is written to `/dev/null` and `secret get` exits 0.
+Retrieves and outputs a secret value to stdout. A failed write of the value,
+such as to a file on a full disk, prints the error and exits 1, with two
+exceptions. If the program reading from a pipe has already exited, as in
+`secret get s | true`, the broken-pipe signal (SIGPIPE) ends `secret` with no
+message and the shell reports exit status 141. If the shell closed stdout before
+`secret` started (`secret get s >&-`), the value is written to `/dev/null` and
+`secret get` exits 0.
 
 - `--version, -v`: Get a specific version (default: current)
 
@@ -304,7 +300,7 @@ Selects an unlocker as the current default for operations.
 #### `secret import <secret-name> --source <filename>`
 
 Imports a secret from a file and stores it in the current vault under the given
-name. The file's bytes are stored unchanged, a final newline included.
+name.
 
 ### Encryption Operations
 

internal/cli/trailing_newline_test.go

@@ -1,47 +0,0 @@
-//nolint:testpackage // white-box test of unexported internals
-package cli
-
-import (
-	"bytes"
-	"testing"
-
-	"github.com/spf13/afero"
-	"github.com/spf13/cobra"
-	"github.com/stretchr/testify/assert"
-	"github.com/stretchr/testify/require"
-)
-
-// TestAddDropsOneTrailingNewlineImportKeepsIt pins the rule the README
-// gives: `secret add` drops one trailing newline from stdin, and
-// `secret import` stores the file's bytes unchanged.
-func TestAddDropsOneTrailingNewlineImportKeepsIt(t *testing.T) {
-	t.Parallel()
-
-	fs, vlt := newSizeTestVault(t)
-	value := []byte("a\n\n")
-
-	cmd := &cobra.Command{}
-	cmd.SetIn(bytes.NewReader(value))
-
-	cli := NewCLIInstanceWithStateDir(fs, testStateDir)
-	cli.cmd = cmd
-
-	require.NoError(t, cli.AddSecret("added", false))
-
-	sourceFile := "/test/value"
-	require.NoError(t, afero.WriteFile(fs, sourceFile, value, 0o600))
-	require.NoError(t, cli.ImportSecret(cmd, "imported", sourceFile, false))
-
-	added, err := vlt.GetSecret("added")
-	require.NoError(t, err)
-
-	defer added.Destroy()
-
-	imported, err := vlt.GetSecret("imported")
-	require.NoError(t, err)
-
-	defer imported.Destroy()
-
-	assert.Equal(t, []byte("a\n"), added.Bytes())
-	assert.Equal(t, []byte("a\n\n"), imported.Bytes())
-}

internal/secret/pgpunlocker.go

@@ -388,8 +388,7 @@ func ResolveGPGKeyFingerprint(keyID string) (string, error) {
 			return "", fmt.Errorf("%w: %s", ErrGPGKeyNotFound, keyID)
 		}
 
-		return "", fmt.Errorf(
-			"failed to resolve GPG key fingerprint: %w", withGPGStderr(err))
+		return "", fmt.Errorf("failed to resolve GPG key fingerprint: %w", err)
 	}
 
 	// Parse the output to extract the fingerprint
@@ -419,24 +418,6 @@ func checkGPGAvailable() error {
 	return nil
 }
 
-// withGPGStderr appends gpg's stderr, which says why gpg failed, to err
-// from cmd.Output. cmd.Output keeps that stderr in the *exec.ExitError.
-// err is returned unchanged when gpg wrote nothing to stderr.
-func withGPGStderr(err error) error {
-	var exitErr *exec.ExitError
-
-	if !errors.As(err, &exitErr) {
-		return err
-	}
-
-	stderr := strings.TrimSpace(string(exitErr.Stderr))
-	if stderr == "" {
-		return err
-	}
-
-	return fmt.Errorf("%w: %s", err, stderr)
-}
-
 // gpgEncryptDefault is the default implementation of GPG encryption
 func gpgEncryptDefault(data *memguard.LockedBuffer, keyID string) ([]byte, error) {
 	if data == nil {
@@ -456,7 +437,7 @@ func gpgEncryptDefault(data *memguard.LockedBuffer, keyID string) ([]byte, error
 
 	output, err := cmd.Output()
 	if err != nil {
-		return nil, fmt.Errorf("GPG encryption failed: %w", withGPGStderr(err))
+		return nil, fmt.Errorf("GPG encryption failed: %w", err)
 	}
 
 	return output, nil
@@ -469,7 +450,7 @@ func gpgDecryptDefault(encryptedData []byte) (*memguard.LockedBuffer, error) {
 
 	output, err := cmd.Output()
 	if err != nil {
-		return nil, fmt.Errorf("GPG decryption failed: %w", withGPGStderr(err))
+		return nil, fmt.Errorf("GPG decryption failed: %w", err)
 	}
 
 	// Create a secure buffer for the decrypted data

internal/secret/pgpunlocker_test.go

@@ -102,27 +102,3 @@ func TestPGPUnlockerAddedTwiceKeepsFirst(t *testing.T) {
 	assert.NotEqual(t, first.GetDirectory(), second.GetDirectory())
 	assert.Equal(t, firstFiles, dirFiles(t, fs, first.GetDirectory()))
 }
-
-// TestGPGDecryptErrorHasGPGReason runs the real gpg on data that is not an
-// OpenPGP message and checks that the error carries the reason gpg writes
-// to stderr.
-func TestGPGDecryptErrorHasGPGReason(t *testing.T) {
-	// The empty GNUPGHOME keeps gpg away from the user's own keyring;
-	// LC_ALL=C keeps gpg's message in English.
-	t.Setenv("GNUPGHOME", t.TempDir())
-	t.Setenv("LC_ALL", "C")
-
-	_, err := secret.GPGDecryptFunc([]byte("not an OpenPGP message"))
-	require.ErrorContains(t, err, "gpg: no valid OpenPGP data found")
-}
-
-// TestGPGDecryptErrorWithoutGPGStderr checks that when gpg fails without
-// writing to stderr, the error ends with the exit status and nothing after it.
-//
-//nolint:paralleltest // installFakeGPG uses t.Setenv
-func TestGPGDecryptErrorWithoutGPGStderr(t *testing.T) {
-	installFakeGPG(t)
-
-	_, err := secret.GPGDecryptFunc([]byte("not an OpenPGP message"))
-	require.EqualError(t, err, "GPG decryption failed: exit status 1")
-}