sneak/vaultik
1
0
Fork 0
Code Issues 17 Pull Requests 1 Actions Packages Projects Releases Wiki Activity

pruneLocalSnapshots writes prose to stdout unconditionally, so prune --json | jq still fails after the banner fix #108

New Issue
Closed
opened 2026-08-09 19:03:16 +02:00 by clawbot · 2 comments
clawbot commented 2026-08-09 19:03:16 +02:00
Collaborator
Copy Link
Copy Source

Found while verifying
issue #106 end to end.
Filing rather than fixing drive-by, for the same reason #106 was filed
out of #82: this is a different writer on a different code path, and it
is not the banner.

Problem

pruneLocalSnapshots (internal/vaultik/snapshot.go:845-878) is the
only stdout writer on a --json path with no --json awareness at all.
Every other one is gated — Prune's blob phase checks opts.JSON
before each of its five writes (internal/vaultik/prune.go:113-150),
RemoveSnapshot returns early on opts.JSON
(internal/vaultik/snapshot.go:1042), remote info wraps its whole
human report in if !jsonOutput (internal/vaultik/info.go:216). This
function does not, and Prune calls it before the blob phase, so its
line lands ahead of the document:

$ vaultik --config /tmp/demo/config.yml prune --json
No stale local snapshots found.
{
  "blobs_found": 1,
  "blobs_deleted": 1,
  "bytes_freed": 246
}

$ vaultik --config /tmp/demo/config.yml prune --json | jq .
jq: parse error: Invalid numeric literal at line 1, column 3

Three writes are affected, covering every branch of the function, so
there is no input that avoids it:

  • v.stdoutf("Removing stale local record: %s\n", id) (line 858)
  • v.printlnStdout("No stale local snapshots found.") (line 873)
  • v.stdoutf("Removed %d stale local snapshot record(s).\n", removed)
    (line 875)

Not the same defect as #106, and not fixed by it

  • It reproduces identically on main before the #106 change and after
    it. #106 removes the three banner lines; these remain.
  • -q does not suppress it either, so it is not covered by the
    workaround #106 retired. printlnStdout and stdoutf write straight
    to v.Stdout and never consult v.UI, which is what SetQuiet
    affects. vaultik prune -q --json | jq fails today for this reason.
  • prune is the only affected command: snapshot list --json,
    snapshot verify --json, snapshot remove --json and
    remote info --json all pipe into jq cleanly once the banner is
    gone. Verified against a real file:// destination store with a real
    snapshot.

Expected

prune --json emits its PruneBlobsResult and nothing else, the same
contract issue #82
established for the logger and #106 for the banner.

Notes

The narrow fix is to thread opts.JSON into pruneLocalSnapshots the
way the surrounding code already does. Worth considering instead
whether these three lines want to be log.Info records — they are
progress narration, not output the caller asked for, and the logger is
already on stderr. That choice is what this issue needs decided; the
count of stale records is not currently in PruneBlobsResult, so if it
should survive into the document that is a third option.

Found while verifying [issue #106](https://git.eeqj.de/sneak/vaultik/issues/106) end to end. Filing rather than fixing drive-by, for the same reason #106 was filed out of #82: this is a different writer on a different code path, and it is not the banner. ## Problem `pruneLocalSnapshots` (`internal/vaultik/snapshot.go:845-878`) is the only stdout writer on a `--json` path with no `--json` awareness at all. Every other one is gated — `Prune`'s blob phase checks `opts.JSON` before each of its five writes (`internal/vaultik/prune.go:113-150`), `RemoveSnapshot` returns early on `opts.JSON` (`internal/vaultik/snapshot.go:1042`), `remote info` wraps its whole human report in `if !jsonOutput` (`internal/vaultik/info.go:216`). This function does not, and `Prune` calls it before the blob phase, so its line lands ahead of the document: ``` $ vaultik --config /tmp/demo/config.yml prune --json No stale local snapshots found. { "blobs_found": 1, "blobs_deleted": 1, "bytes_freed": 246 } ``` ``` $ vaultik --config /tmp/demo/config.yml prune --json | jq . jq: parse error: Invalid numeric literal at line 1, column 3 ``` Three writes are affected, covering every branch of the function, so there is no input that avoids it: - `v.stdoutf("Removing stale local record: %s\n", id)` (line 858) - `v.printlnStdout("No stale local snapshots found.")` (line 873) - `v.stdoutf("Removed %d stale local snapshot record(s).\n", removed)` (line 875) ## Not the same defect as #106, and not fixed by it - It reproduces identically on `main` before the #106 change and after it. #106 removes the three banner lines; these remain. - `-q` does not suppress it either, so it is not covered by the workaround #106 retired. `printlnStdout` and `stdoutf` write straight to `v.Stdout` and never consult `v.UI`, which is what `SetQuiet` affects. `vaultik prune -q --json | jq` fails today for this reason. - `prune` is the only affected command: `snapshot list --json`, `snapshot verify --json`, `snapshot remove --json` and `remote info --json` all pipe into `jq` cleanly once the banner is gone. Verified against a real `file://` destination store with a real snapshot. ## Expected `prune --json` emits its `PruneBlobsResult` and nothing else, the same contract [issue #82](https://git.eeqj.de/sneak/vaultik/issues/82) established for the logger and #106 for the banner. ## Notes The narrow fix is to thread `opts.JSON` into `pruneLocalSnapshots` the way the surrounding code already does. Worth considering instead whether these three lines want to be `log.Info` records — they are progress narration, not output the caller asked for, and the logger is already on stderr. That choice is what this issue needs decided; the count of stale records is not currently in `PruneBlobsResult`, so if it should survive into the document that is a third option.
clawbot added this to the 1.0.0 milestone 2026-08-09 19:10:42 +02:00
clawbot commented 2026-08-09 19:23:41 +02:00
Author
Collaborator
Copy Link
Copy Source

Plan

Implementing this together with
issue #110 as one PR
against main; they are unrelated in the code but both small.

The decision this issue asks for

Of the three options in the description I am taking the first — thread
opts.JSON into the function — and explicitly rejecting the other two.

Not log.Info. The logger's default level is slog.LevelWarn
(internal/log/log.go:68), so moving these three lines to log.Info
would not relocate them to stderr, it would delete them from a plain
vaultik prune. "Removing stale local record: <id>" narrates the
deletion of rows from the local index; a user who runs prune with no
flags should see that it happened, and making it visible only under
--verbose is a behaviour regression rather than a stream fix.

Not PruneBlobsResult. Every field of that struct is blob-scoped
(blobs_found, blobs_deleted, blobs_failed, bytes_freed) and it
is the result of the blob phase, which runs after this reconciliation.
Adding a stale-record count to it would make the document's name
inaccurate and would change a published --json schema as a side
effect of a stream-hygiene fix. If the count should be
machine-readable, the right shape is a prune document covering both
phases, and that is a separate design question, not this patch.

So: CleanupLocalSnapshots takes opts *PruneOptions, symmetric with
its sibling phase PruneBlobs(opts), and each of the three writes is
gated on !opts.JSON exactly as prune.go:113-150 already gates its
own. The two informational events additionally get log.Info records,
mirroring PruneBlobs, which already logs and prints in parallel.

Tests

Failing-before, at two layers:

  1. internal/vaultik: CleanupLocalSnapshots with JSON: true leaves
    stdout empty in both branches (stale records present, and none), and
    with JSON: false still emits all three lines — so the guard cannot
    be satisfied by deleting the output.
  2. internal/cli: prune --json run end to end through Entry, cobra
    and fx over the process's real stdout descriptor against a
    hermetic file:// store, asserting stdout holds exactly one JSON
    document. Both branches, by seeding the index database with a
    snapshot that has no remote manifest. This is the assertion the
    issue makes — vaultik prune --json | jq . with no other flags —
    with the pipe replaced by a decoder.

Plus a manual end-to-end run of the built binary against a real config
and file:// destination store, in both branches, and a re-check that
the other four --json commands still pipe cleanly.

README.md:142 will be adjusted: it currently describes the banner as
"the other thing that writes to stdout", which this issue contradicts.

## Plan Implementing this together with [issue #110](https://git.eeqj.de/sneak/vaultik/issues/110) as one PR against `main`; they are unrelated in the code but both small. ### The decision this issue asks for Of the three options in the description I am taking the first — thread `opts.JSON` into the function — and explicitly rejecting the other two. **Not `log.Info`.** The logger's default level is `slog.LevelWarn` (`internal/log/log.go:68`), so moving these three lines to `log.Info` would not relocate them to stderr, it would delete them from a plain `vaultik prune`. "Removing stale local record: &lt;id&gt;" narrates the deletion of rows from the local index; a user who runs `prune` with no flags should see that it happened, and making it visible only under `--verbose` is a behaviour regression rather than a stream fix. **Not `PruneBlobsResult`.** Every field of that struct is blob-scoped (`blobs_found`, `blobs_deleted`, `blobs_failed`, `bytes_freed`) and it is the result of the blob phase, which runs after this reconciliation. Adding a stale-record count to it would make the document's name inaccurate and would change a published `--json` schema as a side effect of a stream-hygiene fix. If the count should be machine-readable, the right shape is a prune document covering both phases, and that is a separate design question, not this patch. So: `CleanupLocalSnapshots` takes `opts *PruneOptions`, symmetric with its sibling phase `PruneBlobs(opts)`, and each of the three writes is gated on `!opts.JSON` exactly as `prune.go:113-150` already gates its own. The two informational events additionally get `log.Info` records, mirroring `PruneBlobs`, which already logs and prints in parallel. ### Tests Failing-before, at two layers: 1. `internal/vaultik`: `CleanupLocalSnapshots` with `JSON: true` leaves stdout empty in both branches (stale records present, and none), and with `JSON: false` still emits all three lines — so the guard cannot be satisfied by deleting the output. 2. `internal/cli`: `prune --json` run end to end through `Entry`, cobra and fx over the process's real stdout descriptor against a hermetic `file://` store, asserting stdout holds exactly one JSON document. Both branches, by seeding the index database with a snapshot that has no remote manifest. This is the assertion the issue makes — `vaultik prune --json | jq .` with no other flags — with the pipe replaced by a decoder. Plus a manual end-to-end run of the built binary against a real config and `file://` destination store, in both branches, and a re-check that the other four `--json` commands still pipe cleanly. `README.md:142` will be adjusted: it currently describes the banner as "the other thing that writes to stdout", which this issue contradicts.
clawbot commented 2026-08-09 19:37:00 +02:00
Author
Collaborator
Copy Link
Copy Source

Implemented in
PR #111, on branch
fix-prune-json-and-build.

What was built

CleanupLocalSnapshots now takes *PruneOptions and gates all three
writes on !opts.JSON, matching PruneBlobs — its sibling phase, which
already takes the same struct — and RemoveSnapshot and remote info.
The two informational events are additionally emitted as log.Info
records, the pattern PruneBlobs already uses, so they still exist on
stderr for anyone running --verbose.

The decision the issue asked for, restated with the reasons in full in
the PR body: the log.Info-only option was rejected because the
logger's default level is slog.LevelWarn, so it would not move those
lines to stderr, it would delete them from a plain vaultik prune —
narrating the deletion of local index rows only under --verbose is a
behaviour regression, not a stream fix. The PruneBlobsResult option
was rejected because every field of that struct is blob-scoped and it is
produced by the phase that runs after this reconciliation; adding a
stale-record count would change a published --json schema as a side
effect. Per the issue's request, that is flagged rather than silently
added — if the count should be machine-readable, the right shape is a
prune document covering both phases, which is its own design question.

How it was verified

Against a real config and a file:// destination store with a real
snapshot, using the binary from make build, with no other flags and
no -q:

  • No stale records: vaultik prune --json | jq . exits 0. od -c on
    the raw stdout shows the document and nothing else.
  • Stale record present (destination metadata/ removed under a live
    local record): vaultik prune --json | jq . exits 0,
    {"blobs_found": 1, "blobs_deleted": 1, "bytes_freed": 229}, and the
    record is gone from the index afterwards.
  • Without --json, all three lines are still printed — confirmed on the
    same store: "Removing stale local record: ...", "Removed 1 stale local
    snapshot record(s).", and "No stale local snapshots found." on the
    other branch.
  • The other four commands still pipe cleanly on the same binary:
    snapshot list --json, snapshot verify &lt;id&gt; --json,
    snapshot remove &lt;id&gt; --json, remote info --json, jq exit 0
    for each.

Tests, each confirmed to fail with the fix reverted rather than assumed
to: unit coverage of CleanupLocalSnapshots asserting stdout stays
empty under --json in all three branches (stale records, none, empty
index) and that the human output is retained without it, so the guard
cannot be satisfied by deleting the lines; plus an end-to-end
prune --json through Entry, cobra and fx over the process's real
stdout descriptor against a hermetic file:// store, asserting exactly
one JSON document, in both branches.

make check green; script/cibuild exit 0 with the check layers
confirmed to have executed rather than replayed from cache.

Implemented in [PR #111](https://git.eeqj.de/sneak/vaultik/pulls/111), on branch `fix-prune-json-and-build`. ## What was built `CleanupLocalSnapshots` now takes `*PruneOptions` and gates all three writes on `!opts.JSON`, matching `PruneBlobs` — its sibling phase, which already takes the same struct — and `RemoveSnapshot` and `remote info`. The two informational events are additionally emitted as `log.Info` records, the pattern `PruneBlobs` already uses, so they still exist on stderr for anyone running `--verbose`. The decision the issue asked for, restated with the reasons in full in the PR body: the `log.Info`-only option was rejected because the logger's default level is `slog.LevelWarn`, so it would not move those lines to stderr, it would delete them from a plain `vaultik prune` — narrating the deletion of local index rows only under `--verbose` is a behaviour regression, not a stream fix. The `PruneBlobsResult` option was rejected because every field of that struct is blob-scoped and it is produced by the phase that runs after this reconciliation; adding a stale-record count would change a published `--json` schema as a side effect. Per the issue's request, that is flagged rather than silently added — if the count should be machine-readable, the right shape is a prune document covering both phases, which is its own design question. ## How it was verified Against a real config and a `file://` destination store with a real snapshot, using the binary from `make build`, **with no other flags and no `-q`**: - No stale records: `vaultik prune --json | jq .` exits 0. `od -c` on the raw stdout shows the document and nothing else. - Stale record present (destination `metadata/` removed under a live local record): `vaultik prune --json | jq .` exits 0, `{"blobs_found": 1, "blobs_deleted": 1, "bytes_freed": 229}`, and the record is gone from the index afterwards. - Without `--json`, all three lines are still printed — confirmed on the same store: "Removing stale local record: ...", "Removed 1 stale local snapshot record(s).", and "No stale local snapshots found." on the other branch. - The other four commands still pipe cleanly on the same binary: `snapshot list --json`, `snapshot verify &lt;id&gt; --json`, `snapshot remove &lt;id&gt; --json`, `remote info --json`, `jq` exit 0 for each. Tests, each confirmed to fail with the fix reverted rather than assumed to: unit coverage of `CleanupLocalSnapshots` asserting stdout stays empty under `--json` in all three branches (stale records, none, empty index) and that the human output is retained without it, so the guard cannot be satisfied by deleting the lines; plus an end-to-end `prune --json` through `Entry`, cobra and fx over the process's real stdout descriptor against a hermetic `file://` store, asserting exactly one JSON document, in both branches. `make check` green; `script/cibuild` exit 0 with the check layers confirmed to have executed rather than replayed from cache.
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
merge-ready
All checks pass, reviewed, rebased, ready for merge
 needs-checks
make check does not pass cleanly
 needs-rebase
PR has merge conflicts or is behind main
 needs-review
Code review not yet done
 needs-rework
Code review found issues to fix
No labels
Milestone
No items
No Milestone 1.0.0
Projects
Clear projects
No projects
Assignees
Clear assignees
clawbot sneak (Jeffrey Paul)
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: sneak/vaultik#108