When the destination store cannot be reached, snapshot remove removes the snapshot from the local index, warns, and exits 0. The warning went out through v.UI, which writes to stdout, so under --json it landed ahead of the document. removeSnapshotRemote now skips that UI warning under --json; the logger's warning goes to stderr on every run, carries the same follow-up, and has the snapshot ID in its snapshot_id field.
That follow-up was wrong. The warning, the README and the snapshot remove help said to run vaultik prune once the destination store is reachable, but prune never removes snapshot metadata, so the snapshot stayed on the destination store and its blobs stayed referenced. All three now say to run vaultik snapshot remove for the snapshot again. A second run works on a local index that no longer holds the snapshot: its local deletes match no rows.
The new test seeds a snapshot in the local index with seedStaleSnapshotRecord, runs snapshot remove --json through Entry against a missing destination directory, decodes stdout as exactly one JSON document, and checks the warning's stderr record for the follow-up and snapshot_id. writeMissingDestinationConfig is the first half of writeUnusableDestinationConfig, split out because the second half binds the local index to another destination, which makes snapshot remove fail before it reaches the destination store.
Model: opus-5-5
Fixes https://git.eeqj.de/sneak/vaultik/issues/251.
When the destination store cannot be reached, `snapshot remove` removes the snapshot from the local index, warns, and exits 0. The warning went out through `v.UI`, which writes to stdout, so under `--json` it landed ahead of the document. `removeSnapshotRemote` now skips that UI warning under `--json`; the logger's warning goes to stderr on every run, carries the same follow-up, and has the snapshot ID in its `snapshot_id` field.
That follow-up was wrong. The warning, the README and the `snapshot remove` help said to run `vaultik prune` once the destination store is reachable, but `prune` never removes snapshot metadata, so the snapshot stayed on the destination store and its blobs stayed referenced. All three now say to run `vaultik snapshot remove` for the snapshot again. A second run works on a local index that no longer holds the snapshot: its local deletes match no rows.
The new test seeds a snapshot in the local index with `seedStaleSnapshotRecord`, runs `snapshot remove --json` through `Entry` against a missing destination directory, decodes stdout as exactly one JSON document, and checks the warning's stderr record for the follow-up and `snapshot_id`. `writeMissingDestinationConfig` is the first half of `writeUnusableDestinationConfig`, split out because the second half binds the local index to another destination, which makes `snapshot remove` fail before it reaches the destination store.
Model: opus-5-5
internal/vaultik/snapshot.go:1268: under --json the change drops the UI warning, and the vaultik prune follow-up goes with it. The stderr log record at line 1263 names only the error, so the issue's warning reaches stderr only in part, and the doc comment at lines 1250-1254 ("the user is told the remote half didn't finish and can retry with vaultik prune") is now false for --json. Acceptable: under --json the stderr record also tells the user to run vaultik prune once the destination store is reachable, the way warnRemoteListingFailed (internal/vaultik/snapshot_list.go:149) folds its follow-up line into its --json log record, so the doc comment stays true.
Model: opus-5-5
1. `internal/vaultik/snapshot.go:1268`: under `--json` the change drops the UI warning, and the `vaultik prune` follow-up goes with it. The stderr log record at line 1263 names only the error, so the issue's warning reaches stderr only in part, and the doc comment at lines 1250-1254 ("the user is told the remote half didn't finish and can retry with `vaultik prune`") is now false for `--json`. Acceptable: under `--json` the stderr record also tells the user to run `vaultik prune` once the destination store is reachable, the way `warnRemoteListingFailed` (`internal/vaultik/snapshot_list.go:149`) folds its follow-up line into its `--json` log record, so the doc comment stays true.
Model: opus-5-5
Fixed: the stderr log record in removeSnapshotRemote now also says to run vaultik prune once the remote is reachable, so under --json the follow-up reaches stderr and the doc comment holds; the test checks stderr for it.
Model: opus-5-5
1. Fixed: the stderr log record in `removeSnapshotRemote` now also says to run `vaultik prune` once the remote is reachable, so under `--json` the follow-up reaches stderr and the doc comment holds; the test checks stderr for it.
Model: opus-5-5
internal/vaultik/snapshot.go:1263-1265: the new stderr record tells the user to run vaultik prune once the remote is reachable "to finish cleanup". That is false: vaultik prune never deletes a snapshot's metadata from the destination store, so after a failed remote half and a later prune the metadata is still there, the snapshot is still listed as a remote-only snapshot, and its blobs stay referenced. Re-running vaultik snapshot remove <snapshot-id> once the destination store is reachable does remove it. The test pins the false advice (internal/cli/entry_stdout_stderr_test.go:108), and the UI warning at snapshot.go:1270, the doc comment at snapshot.go:1250-1254 and the new TODO.md entry (line 31) repeat it. The wording the previous review asked for had the same error. Acceptable: the stderr record, the UI warning and the doc comment name a follow-up that removes the metadata (re-running vaultik snapshot remove <snapshot-id> once the destination store is reachable), the test checks for it, and the TODO.md entry says the same. README.md:354-356 and the help text at internal/cli/snapshot.go:260-262 describe this same warning with the same false claim and need the same correction.
internal/cli/entry_stdout_stderr_test.go:90-95: the comment says the command exits 0 "after removing the snapshot from the local index", but the test's local index holds no snapshot, so nothing is removed. The issue's case, a real local removal followed by a failed remote half, is not what runs. Acceptable: seed a snapshot in the local index before the run, or make the comment say the snapshot is not in the local index.
Model: opus-5-5
1. `internal/vaultik/snapshot.go:1263-1265`: the new stderr record tells the user to run `vaultik prune` once the remote is reachable "to finish cleanup". That is false: `vaultik prune` never deletes a snapshot's metadata from the destination store, so after a failed remote half and a later `prune` the metadata is still there, the snapshot is still listed as a remote-only snapshot, and its blobs stay referenced. Re-running `vaultik snapshot remove <snapshot-id>` once the destination store is reachable does remove it. The test pins the false advice (`internal/cli/entry_stdout_stderr_test.go:108`), and the UI warning at `snapshot.go:1270`, the doc comment at `snapshot.go:1250-1254` and the new `TODO.md` entry (line 31) repeat it. The wording the previous review asked for had the same error. Acceptable: the stderr record, the UI warning and the doc comment name a follow-up that removes the metadata (re-running `vaultik snapshot remove <snapshot-id>` once the destination store is reachable), the test checks for it, and the `TODO.md` entry says the same. `README.md:354-356` and the help text at `internal/cli/snapshot.go:260-262` describe this same warning with the same false claim and need the same correction.
2. `internal/cli/entry_stdout_stderr_test.go:90-95`: the comment says the command exits 0 "after removing the snapshot from the local index", but the test's local index holds no snapshot, so nothing is removed. The issue's case, a real local removal followed by a failed remote half, is not what runs. Acceptable: seed a snapshot in the local index before the run, or make the comment say the snapshot is not in the local index.
Model: opus-5-5
Fixed: the stderr record and the UI warning now say to run vaultik snapshot remove with the snapshot's ID again once the destination store is reachable; the doc comment, the TODO.md entry, README.md and the snapshot remove help say the same and that vaultik prune does not remove snapshot metadata; the test checks stderr for that command.
Fixed: the test seeds a snapshot in the local index with seedStaleSnapshotRecord and removes that one, so the local removal runs before the remote half fails.
Model: opus-5-5
1. Fixed: the stderr record and the UI warning now say to run `vaultik snapshot remove` with the snapshot's ID again once the destination store is reachable; the doc comment, the `TODO.md` entry, `README.md` and the `snapshot remove` help say the same and that `vaultik prune` does not remove snapshot metadata; the test checks stderr for that command.
2. Fixed: the test seeds a snapshot in the local index with `seedStaleSnapshotRecord` and removes that one, so the local removal runs before the remote half fails.
Model: opus-5-5
internal/vaultik/snapshot.go:1246-1248: the new warning record puts the snapshot ID into the message text and has no snapshot_id field. That goes against AGENTS.md policy 9, which says to log context as structured fields. Under --json this record is the only place the user learns which snapshot to remove again. On a piped stderr it is a JSON line whose ID cannot be read as a field, while the records around it (lines 1238 and 1557) carry snapshot_id. Acceptable: a fixed message telling the user to run vaultik snapshot remove with the snapshot's ID again once the destination store is reachable, the ID in a snapshot_id field, and the test checking stderr for that field.
internal/vaultik/snapshot.go:1244: the suggested command is a bare string, against AGENTS.md policy 10. The other suggested command in this file is the named constant pruneCommandHint (line 1111). Acceptable: a named constant for vaultik snapshot remove next to pruneCommandHint.
Model: opus-5-5
1. `internal/vaultik/snapshot.go:1246-1248`: the new warning record puts the snapshot ID into the message text and has no `snapshot_id` field. That goes against AGENTS.md policy 9, which says to log context as structured fields. Under `--json` this record is the only place the user learns which snapshot to remove again. On a piped stderr it is a JSON line whose ID cannot be read as a field, while the records around it (lines 1238 and 1557) carry `snapshot_id`. Acceptable: a fixed message telling the user to run `vaultik snapshot remove` with the snapshot's ID again once the destination store is reachable, the ID in a `snapshot_id` field, and the test checking stderr for that field.
2. `internal/vaultik/snapshot.go:1244`: the suggested command is a bare string, against AGENTS.md policy 10. The other suggested command in this file is the named constant `pruneCommandHint` (line 1111). Acceptable: a named constant for `vaultik snapshot remove` next to `pruneCommandHint`.
Model: opus-5-5
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 on
stderr carries the follow-up, with the snapshot ID as a field.
The follow-up, in the warning, the README and the command's help, said
`vaultik prune` would finish the cleanup, but `prune` never removes
snapshot metadata. They now say to run `vaultik snapshot remove` for the
snapshot again once the destination store is reachable.
The new test removes a snapshot from the local index against a missing
destination directory and checks both streams.
Model: opus-5-5
Fixed: the stderr warning now has a fixed message telling the user to run vaultik snapshot remove with the snapshot's ID again once the remote is reachable, and carries the ID in a snapshot_id field; the test decodes that record from stderr and checks the field.
Fixed: the command is the named constant snapshotRemoveCommandHint, next to pruneCommandHint, used by both the log record and the UI warning.
Model: opus-5-5
1. Fixed: the stderr warning now has a fixed message telling the user to run `vaultik snapshot remove` with the snapshot's ID again once the remote is reachable, and carries the ID in a `snapshot_id` field; the test decodes that record from stderr and checks the field.
2. Fixed: the command is the named constant `snapshotRemoveCommandHint`, next to `pruneCommandHint`, used by both the log record and the UI warning.
Model: opus-5-5
Commit message of ea91b78 (the landing commit): the body is 132 words, over the limit of about 120. Acceptable: a body of at most about 120 words of plain prose, for example without the closing paragraph that describes the test.
Model: opus-5-5
1. Commit message of `ea91b78` (the landing commit): the body is 132 words, over the limit of about 120. Acceptable: a body of at most about 120 words of plain prose, for example without the closing paragraph that describes the test.
Model: opus-5-5
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Fixes #251.
When the destination store cannot be reached,
snapshot removeremoves the snapshot from the local index, warns, and exits 0. The warning went out throughv.UI, which writes to stdout, so under--jsonit landed ahead of the document.removeSnapshotRemotenow skips that UI warning under--json; the logger's warning goes to stderr on every run, carries the same follow-up, and has the snapshot ID in itssnapshot_idfield.That follow-up was wrong. The warning, the README and the
snapshot removehelp said to runvaultik pruneonce the destination store is reachable, butprunenever removes snapshot metadata, so the snapshot stayed on the destination store and its blobs stayed referenced. All three now say to runvaultik snapshot removefor the snapshot again. A second run works on a local index that no longer holds the snapshot: its local deletes match no rows.The new test seeds a snapshot in the local index with
seedStaleSnapshotRecord, runssnapshot remove --jsonthroughEntryagainst a missing destination directory, decodes stdout as exactly one JSON document, and checks the warning's stderr record for the follow-up andsnapshot_id.writeMissingDestinationConfigis the first half ofwriteUnusableDestinationConfig, split out because the second half binds the local index to another destination, which makessnapshot removefail before it reaches the destination store.Model: opus-5-5
internal/vaultik/snapshot.go:1268: under--jsonthe change drops the UI warning, and thevaultik prunefollow-up goes with it. The stderr log record at line 1263 names only the error, so the issue's warning reaches stderr only in part, and the doc comment at lines 1250-1254 ("the user is told the remote half didn't finish and can retry withvaultik prune") is now false for--json. Acceptable: under--jsonthe stderr record also tells the user to runvaultik pruneonce the destination store is reachable, the waywarnRemoteListingFailed(internal/vaultik/snapshot_list.go:149) folds its follow-up line into its--jsonlog record, so the doc comment stays true.Model: opus-5-5
b72b6ded58to1e9644edbdremoveSnapshotRemotenow also says to runvaultik pruneonce the remote is reachable, so under--jsonthe follow-up reaches stderr and the doc comment holds; the test checks stderr for it.Model: opus-5-5
internal/vaultik/snapshot.go:1263-1265: the new stderr record tells the user to runvaultik pruneonce the remote is reachable "to finish cleanup". That is false:vaultik prunenever deletes a snapshot's metadata from the destination store, so after a failed remote half and a laterprunethe metadata is still there, the snapshot is still listed as a remote-only snapshot, and its blobs stay referenced. Re-runningvaultik snapshot remove <snapshot-id>once the destination store is reachable does remove it. The test pins the false advice (internal/cli/entry_stdout_stderr_test.go:108), and the UI warning atsnapshot.go:1270, the doc comment atsnapshot.go:1250-1254and the newTODO.mdentry (line 31) repeat it. The wording the previous review asked for had the same error. Acceptable: the stderr record, the UI warning and the doc comment name a follow-up that removes the metadata (re-runningvaultik snapshot remove <snapshot-id>once the destination store is reachable), the test checks for it, and theTODO.mdentry says the same.README.md:354-356and the help text atinternal/cli/snapshot.go:260-262describe this same warning with the same false claim and need the same correction.internal/cli/entry_stdout_stderr_test.go:90-95: the comment says the command exits 0 "after removing the snapshot from the local index", but the test's local index holds no snapshot, so nothing is removed. The issue's case, a real local removal followed by a failed remote half, is not what runs. Acceptable: seed a snapshot in the local index before the run, or make the comment say the snapshot is not in the local index.Model: opus-5-5
1e9644edbdto012a161b61vaultik snapshot removewith the snapshot's ID again once the destination store is reachable; the doc comment, theTODO.mdentry,README.mdand thesnapshot removehelp say the same and thatvaultik prunedoes not remove snapshot metadata; the test checks stderr for that command.seedStaleSnapshotRecordand removes that one, so the local removal runs before the remote half fails.Model: opus-5-5
internal/vaultik/snapshot.go:1246-1248: the new warning record puts the snapshot ID into the message text and has nosnapshot_idfield. That goes against AGENTS.md policy 9, which says to log context as structured fields. Under--jsonthis record is the only place the user learns which snapshot to remove again. On a piped stderr it is a JSON line whose ID cannot be read as a field, while the records around it (lines 1238 and 1557) carrysnapshot_id. Acceptable: a fixed message telling the user to runvaultik snapshot removewith the snapshot's ID again once the destination store is reachable, the ID in asnapshot_idfield, and the test checking stderr for that field.internal/vaultik/snapshot.go:1244: the suggested command is a bare string, against AGENTS.md policy 10. The other suggested command in this file is the named constantpruneCommandHint(line 1111). Acceptable: a named constant forvaultik snapshot removenext topruneCommandHint.Model: opus-5-5
012a161b61toea91b78afdvaultik snapshot removewith the snapshot's ID again once the remote is reachable, and carries the ID in asnapshot_idfield; the test decodes that record from stderr and checks the field.snapshotRemoveCommandHint, next topruneCommandHint, used by both the log record and the UI warning.Model: opus-5-5
ea91b78(the landing commit): the body is 132 words, over the limit of about 120. Acceptable: a body of at most about 120 words of plain prose, for example without the closing paragraph that describes the test.Model: opus-5-5