README says secret add drops one trailing newline and import keeps it (closes #158)

`secret add` drops one trailing newline from stdin, so
`echo value | secret add name` stores `value`. `secret import` stores a
file's bytes unchanged, and `secret get` writes the value with no
newline added. The README said none of this, so a PEM key piped into
`add` came back one byte short with no hint why. Its entries for `add`,
`get` and `import` now state each rule. A new test stores `a\n\n`
through `add` and through `import` and checks that `add` kept two bytes
and `import` three. No program code changes.

The new test passes on the code before this change; it fails when `add` keeps the newline.

Model: opus-5-5

README.md

@@ -142,7 +142,11 @@ 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.
+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.
 
 - `--force, -f`: Overwrite existing secret
 
@@ -155,13 +159,13 @@ contain `//`, or have `..` as a path segment.
 
 #### `secret get <secret-name> [--version <version>]`
 
-Retrieves and outputs a secret value to stdout. A failed write of the value,
+Writes the secret's value to stdout exactly as stored, with no newline added
-such as to a file on a full disk, prints the error and exits 1, with two
+after it. A failed write of the value, such as to a file on a full disk, prints
-exceptions. If the program reading from a pipe has already exited, as in
+the error and exits 1, with two exceptions. If the program reading from a pipe
-`secret get s | true`, the broken-pipe signal (SIGPIPE) ends `secret` with no
+has already exited, as in `secret get s | true`, the broken-pipe signal
-message and the shell reports exit status 141. If the shell closed stdout before
+(SIGPIPE) ends `secret` with no message and the shell reports exit status 141.
-`secret` started (`secret get s >&-`), the value is written to `/dev/null` and
+If the shell closed stdout before `secret` started (`secret get s >&-`), the
-`secret get` exits 0.
+value is written to `/dev/null` and `secret get` exits 0.
 
 - `--version, -v`: Get a specific version (default: current)
 
@@ -204,6 +208,12 @@ Moves or renames a secret within the current vault.
 Lists all versions of a secret showing creation time, status, and validity
 period.
 
+`STATUS` is `current` for the secret's current version, `expired` for another
+version whose `NOT_AFTER` has passed, and `inactive` for any other version.
+Adding a version sets `NOT_AFTER` on the version that was current;
+`secret version promote` sets no times. A version that is not current therefore
+shows `inactive` when no version was added while it was current.
+
 #### `secret version promote <secret-name> <version>`
 
 Promotes a specific version to current by rewriting the secret's `current` file
@@ -294,7 +304,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.
+name. The file's bytes are stored unchanged, a final newline included.
 
 ### Encryption Operations
 

internal/cli/trailing_newline_test.go

@@ -0,0 +1,47 @@
+//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/cli/unlockers.go

@@ -314,7 +314,8 @@ func (cli *Instance) UnlockersList(jsonOutput bool) error {
 		return err
 	}
 
-	var unlockers []UnlockerInfo
+	// Not nil, so that a vault with no unlockers prints [] in JSON, not null
+	unlockers := make([]UnlockerInfo, 0, len(unlockerMetadata))
 
 	for _, unlockerID := range slices.Sorted(maps.Keys(unlockerMetadata)) {
 		metadata := unlockerMetadata[unlockerID]

internal/cli/unlockers_list_test.go

@@ -1,13 +1,14 @@
 // Unlocker List Tests
 //
-// Tests for `secret unlocker list` behavior when an unlocker's metadata
+// Tests for `secret unlocker list`:
-// cannot be read or used:
 //
 //   - TestUnlockersListToleratesCorruptMetadata: one unlocker's corrupt
 //     metadata does not stop the others from being listed.
 //   - TestUnlockersListSkipsUnreadableMetadata: an unlocker whose metadata
 //     file cannot be checked for or read is left out, and the other is
 //     still listed.
+//   - TestUnlockersListEmptyJSON: a vault with no unlockers lists them as
+//     an empty JSON list.
 
 //nolint:testpackage // white-box test of unexported internals
 package cli
@@ -321,3 +322,36 @@ func TestUnlockersListSkipsUnreadableMetadata(t *testing.T) {
 		})
 	}
 }
+
+// TestUnlockersListEmptyJSON is a regression test for
+// https://git.eeqj.de/sneak/secret/issues/148: listing the unlockers of a
+// vault that has none, as after its last unlocker is removed, printed
+// "unlockers": null in JSON form instead of an empty list.
+func TestUnlockersListEmptyJSON(t *testing.T) {
+	t.Parallel()
+
+	fs := afero.NewMemMapFs()
+	vaultDir := filepath.Join(listTestStateDir, "vaults.d", listTestVaultName)
+
+	require.NoError(t, afero.WriteFile(
+		fs, filepath.Join(listTestStateDir, "currentvault"),
+		[]byte(listTestVaultName), listTestFilePerm,
+	))
+	require.NoError(t, fs.MkdirAll(
+		filepath.Join(vaultDir, listTestUnlockersDirName), listTestDirPerm,
+	))
+
+	var buf bytes.Buffer
+
+	cmd := &cobra.Command{}
+	cmd.SetOut(&buf)
+
+	instance := &Instance{fs: fs, stateDir: listTestStateDir, cmd: cmd}
+	require.NoError(t, instance.UnlockersList(true))
+
+	var decoded map[string]any
+
+	require.NoError(t, json.Unmarshal(buf.Bytes(), &decoded))
+	assert.Equal(t, []any{}, decoded["unlockers"],
+		"no unlockers must be printed as [], not %s", buf.String())
+}

internal/cli/vault_list_test.go

@@ -0,0 +1,42 @@
+package cli_test
+
+import (
+	"bytes"
+	"encoding/json"
+	"path/filepath"
+	"testing"
+
+	"github.com/spf13/afero"
+	"github.com/spf13/cobra"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"sneak.berlin/go/secret/internal/cli"
+	"sneak.berlin/go/secret/internal/secret"
+)
+
+// TestVaultListEmptyJSON is a regression test for
+// https://git.eeqj.de/sneak/secret/issues/148: listing the vaults when
+// vaults.d holds none printed "vaults": null in JSON form instead of an
+// empty list.
+func TestVaultListEmptyJSON(t *testing.T) {
+	t.Parallel()
+
+	fs := afero.NewMemMapFs()
+	require.NoError(t, fs.MkdirAll(
+		filepath.Join(testStateDir, "vaults.d"), secret.DirPerms,
+	))
+
+	var buf bytes.Buffer
+
+	cmd := &cobra.Command{}
+	cmd.SetOut(&buf)
+
+	c := cli.NewCLIInstanceWithStateDir(fs, testStateDir)
+	require.NoError(t, c.ListVaults(cmd, true))
+
+	var decoded map[string]any
+
+	require.NoError(t, json.Unmarshal(buf.Bytes(), &decoded))
+	assert.Equal(t, []any{}, decoded["vaults"],
+		"no vaults must be printed as [], not %s", buf.String())
+}

internal/cli/version.go

@@ -229,9 +229,13 @@ func printVersionRow(
 	}
 
 	// Determine status
-	status := "expired"
+	status := "inactive"
-	if version == currentVersion {
+
+	switch {
+	case version == currentVersion:
 		status = "current"
+	case sv.Metadata.NotAfter != nil && sv.Metadata.NotAfter.Before(time.Now()):
+		status = "expired"
 	}
 
 	// Format timestamps

internal/cli/version_test.go

@@ -5,6 +5,8 @@
 // - TestListVersionsCommand: Tests `secret version list` command output
 // - TestListVersionsNonExistentSecret: Tests error handling for missing secrets
 // - TestPromoteVersionCommand: Tests `secret version promote` command
+// - TestListVersionsStatusAfterPromote: Tests each version's status after
+//   promoting an older version
 // - TestPromoteNonExistentVersion: Tests error handling for invalid promotion
 // - TestPromoteHelpSaysCurrentFileIsRewritten: Tests `secret version promote
 //   --help` text
@@ -244,6 +246,66 @@ func TestPromoteVersionCommand(t *testing.T) {
 	assert.Equal(t, []byte("version-1"), promoted.Bytes())
 }
 
+func TestListVersionsStatusAfterPromote(t *testing.T) {
+	t.Parallel()
+
+	fs := afero.NewMemMapFs()
+	cli := NewCLIInstanceWithStateDir(fs, testStateDir)
+	cli.Mnemonic = testMnemonicBuffer(t)
+
+	setupTestVault(t, fs)
+
+	vlt, err := vault.GetCurrentVault(fs, testStateDir)
+	require.NoError(t, err)
+
+	vlt.Mnemonic = cli.Mnemonic
+
+	addTestSecret(t, vlt, []byte("version-1"), false)
+	addTestSecret(t, vlt, []byte("version-2"), true)
+	addTestSecret(t, vlt, []byte("version-3"), true)
+
+	vaultDir, err := vlt.GetDirectory()
+	require.NoError(t, err)
+
+	// Newest first
+	versions, err := secret.ListVersions(fs,
+		filepath.Join(vaultDir, "secrets.d", "test%secret"))
+	require.NoError(t, err)
+	require.Len(t, versions, 3)
+
+	newest, middle, oldest := versions[0], versions[1], versions[2]
+
+	cmd := newRootCmd()
+
+	var buf bytes.Buffer
+	cmd.SetOut(&buf)
+	cmd.SetErr(&buf)
+
+	err = cli.PromoteVersion(cmd, "test/secret", oldest)
+	require.NoError(t, err)
+
+	buf.Reset()
+
+	err = cli.ListVersions(cmd, "test/secret")
+	require.NoError(t, err)
+
+	// STATUS is the fourth field of a row, since CREATED is a date and a time
+	statuses := map[string]string{}
+
+	for line := range strings.SplitSeq(buf.String(), "\n") {
+		fields := strings.Fields(line)
+		if len(fields) > 3 {
+			statuses[fields[0]] = fields[3]
+		}
+	}
+
+	assert.Equal(t, "current", statuses[oldest])
+	// Its NOT_AFTER was set when the newest version was added
+	assert.Equal(t, "expired", statuses[middle])
+	// It has no NOT_AFTER
+	assert.Equal(t, "inactive", statuses[newest])
+}
+
 func TestPromoteNonExistentVersion(t *testing.T) {
 	t.Parallel()
 

internal/secret/version.go

@@ -32,7 +32,8 @@ type VersionMetadata struct {
 	ID        string     `json:"id"`                  // ULID
 	CreatedAt *time.Time `json:"createdAt,omitempty"` // When version was created
 	NotBefore *time.Time `json:"notBefore,omitempty"` // When this version becomes active
-	NotAfter  *time.Time `json:"notAfter,omitempty"`  // Expiry (nil = current)
+	// NotAfter is set when another version is added while this one is current
+	NotAfter *time.Time `json:"notAfter,omitempty"`
 }
 
 // Version represents a version of a secret

internal/vault/management.go

@@ -140,8 +140,9 @@ func ListVaults(fs afero.Fs, stateDir string) ([]string, error) {
 		return nil, fmt.Errorf("failed to read vaults directory: %w", err)
 	}
 
-	// Extract vault names
+	// Extract vault names. Not nil, so that no vaults prints [] in JSON,
-	var vaults []string
+	// not null.
+	vaults := make([]string, 0, len(entries))
 
 	for _, entry := range entries {
 		if entry.IsDir() {