From 1e9644edbdc9e930117385b9c0857365e98c41bf Mon Sep 17 00:00:00 2001 From: sneak Date: Tue, 6 Oct 2026 23:24:14 +0000 Subject: [PATCH] Write only the document to stdout from snapshot remove --json (closes #251) When the destination store cannot be reached, `snapshot remove` still removes the snapshot from the local index and warns. The warning went through the UI, which writes to stdout, so under `--json` it landed ahead of the document and `| jq` failed on a command that exited 0. Under `--json` the UI warning is now skipped. The logger's warning, which goes to stderr in every mode, now also says to run `vaultik prune` once the destination store is reachable. The new test runs the command through `Entry` against a missing destination directory, decodes stdout as exactly one JSON document, and finds the warning and the `vaultik prune` follow-up on stderr. Model: opus-5-5 --- TODO.md | 9 +++++ internal/cli/entry_stdout_stderr_test.go | 46 ++++++++++++++++++++---- internal/vaultik/snapshot.go | 13 ++++--- 3 files changed, 56 insertions(+), 12 deletions(-) diff --git a/TODO.md b/TODO.md index b2e6cb7..38a94f0 100644 --- a/TODO.md +++ b/TODO.md @@ -22,6 +22,15 @@ the tag exists and is exercised; what is left is merging `next` to # Completed Steps +- 2026-10-06: Made `snapshot remove --json` write only its document to + stdout when the destination store cannot be reached + ([issue #251](https://git.eeqj.de/sneak/vaultik/issues/251)). Its + warning that the snapshot's metadata was left on the destination store + went to stdout ahead of the document, breaking `| jq` on a command that + exited 0. Under `--json` the warning now reaches stderr only, through + the logger, whose record now also says to run `vaultik prune` once the + destination store is reachable. + - 2026-10-06: Made command output follow the README's stdout and stderr rules ([issue #224](https://git.eeqj.de/sneak/vaultik/issues/224)). The startup banner went to stdout, so a `completion` script or a diff --git a/internal/cli/entry_stdout_stderr_test.go b/internal/cli/entry_stdout_stderr_test.go index 43a6e42..946c6d4 100644 --- a/internal/cli/entry_stdout_stderr_test.go +++ b/internal/cli/entry_stdout_stderr_test.go @@ -87,12 +87,31 @@ func TestEntryJSONFailureIsReportedOnStderr(t *testing.T) { } } -// writeUnusableDestinationConfig builds a config whose destination -// directory does not exist, which fails `remote info`, and whose local -// index is bound to another destination, which fails `prune` and -// `snapshot remove` (a missing destination alone only makes `snapshot -// remove` warn). Returns the config path. -func writeUnusableDestinationConfig(t *testing.T) string { +// TestEntrySnapshotRemoveJSONWarningIsOnStderr runs `snapshot remove +// --json` against a destination directory that does not exist. The +// command still exits 0 after removing the snapshot from the local +// index. Its stdout must hold the document alone, with the warning +// about the destination store, and the `vaultik prune` follow-up, on +// stderr. +// +//nolint:paralleltest // replaces os.Args, os.Stdout, os.Stderr and the xdg globals +func TestEntrySnapshotRemoveJSONWarningIsOnStderr(t *testing.T) { + configPath, _ := writeMissingDestinationConfig(t) + + code, stdout, stderr := runEntry(t, flagConfig, configPath, + cmdSnapshot, cmdRemove, someSnapshotID, flagJSON) + + require.Equal(t, 0, code) + requireExactlyOneJSONDocument(t, stdout) + assert.Contains(t, stderr, + "Could not remove snapshot metadata from remote storage") + assert.Contains(t, stderr, "run 'vaultik prune'") +} + +// writeMissingDestinationConfig builds a config whose destination +// directory does not exist. Returns the config path and the path of +// its local index, which is not created here. +func writeMissingDestinationConfig(t *testing.T) (string, string) { t.Helper() dir := t.TempDir() @@ -114,6 +133,19 @@ func writeUnusableDestinationConfig(t *testing.T) string { xdg.Reload() t.Cleanup(xdg.Reload) + return configPath, indexPath +} + +// writeUnusableDestinationConfig builds a config whose destination +// directory does not exist, which fails `remote info`, and whose local +// index is bound to another destination, which fails `prune` and +// `snapshot remove` (a missing destination alone only makes `snapshot +// remove` warn). Returns the config path. +func writeUnusableDestinationConfig(t *testing.T) string { + t.Helper() + + configPath, indexPath := writeMissingDestinationConfig(t) + ctx := context.Background() db, err := database.New(ctx, indexPath) @@ -122,7 +154,7 @@ func writeUnusableDestinationConfig(t *testing.T) string { defer func() { require.NoError(t, db.Close()) }() require.NoError(t, database.NewRepositories(db).LocalMeta.Set(ctx, - database.LocalMetaKeyStorageURL, "file://"+filepath.Join(dir, "other"))) + database.LocalMetaKeyStorageURL, "file://"+t.TempDir())) return configPath } diff --git a/internal/vaultik/snapshot.go b/internal/vaultik/snapshot.go index 7c948f7..b7b75dd 100644 --- a/internal/vaultik/snapshot.go +++ b/internal/vaultik/snapshot.go @@ -1167,7 +1167,7 @@ func (v *Vaultik) RemoveSnapshot( } if !opts.LocalOnly { - result.RemoteRemoved = v.removeSnapshotRemote(snapshotID) + result.RemoteRemoved = v.removeSnapshotRemote(snapshotID, opts) } if v.SnapshotManager != nil { @@ -1252,7 +1252,7 @@ func (v *Vaultik) confirmRemoveSnapshot(snapshotID string, opts *RemoveOptions) // removal has already happened, so the user is told the remote half // didn't finish and can retry with `vaultik prune` once the destination // store is reachable. Returns true when the remote removal succeeded. -func (v *Vaultik) removeSnapshotRemote(snapshotID string) bool { +func (v *Vaultik) removeSnapshotRemote(snapshotID string, opts *RemoveOptions) bool { log.Info("Removing snapshot metadata from remote storage", "snapshot_id", snapshotID) @@ -1260,10 +1260,13 @@ func (v *Vaultik) removeSnapshotRemote(snapshotID string) bool { err := v.deleteRemoteSnapshotByKey(remoteKey) if err != nil { - log.Warn("Could not remove snapshot metadata from remote storage", - "error", err) + log.Warn("Could not remove snapshot metadata from remote storage; "+ + "run '"+pruneCommandHint+"' once the remote is reachable "+ + "to finish cleanup", "error", err) - if v.UI != nil { + // The UI writes to stdout, which under --json holds only the + // document; the log record above is the warning on stderr. + if v.UI != nil && !opts.JSON { v.UI.Warningf("Could not remove snapshot metadata from remote: "+ "%v. Run '%s' once the remote is reachable to finish cleanup.", err, pruneCommandHint)