List remote snapshots without requiring the private key (closes #64) #83

clawbot · 2026-08-09T05:16:58+02:00

clawbot commented

2026-08-09 05:16:58 +02:00

Fixes #64.

ListSnapshots built its table entirely from the local SQLite index.
The only remote access, reportRemoteDrift, was gated on
AgeSecretKey != "" — so on a correctly configured host, which by
design holds no private key, snapshot list never contacted the
destination store at all. A user who lost their local index could not
see their own backups, and the <remote only> cell the README
documents was unreachable dead code.

Review rework (head `9a45221`)

The first review returned FAIL with three blocking findings. All three
are fixed in 9a45221, each with a test that fails without the fix.
No JSON shape change, so nothing here breaks a machine consumer.

1. The TIMESTAMP column mixed two timezones. Remote-only rows
rendered timestamp.UTC(); local rows came from ListRecent, whose
scanner (internal/database/snapshots.go:767) decoded Unix seconds in
the host's local zone — unlike its two siblings at lines 200 and 639,
which decode in UTC. Both render through the same zone-less format
string, so on a non-UTC host the same snapshot showed one time when
locally tracked and a different time when remote-only, in the same
column, with nothing to indicate why.

Fixed at the point the timestamp enters the domain, not at the display
layer: scanSnapshotRows now decodes in UTC like its siblings, and
GetIncompleteByHostname's inline copy of that loop was folded onto the
shared scanner, so the call sites cannot drift apart again. (That fold
was also required by dupl, which correctly started firing once the two
loops became token-identical.)

Covered by TestSnapshotTimestampsDecodeAsUTC
(internal/database/snapshots_test.go), which checks every snapshot
reader and compares *time.Location pointers, so it is equally strict
on a UTC host — time.Unix returns time.Local, which is never the
same Location value as time.UTC; and by
TestListSnapshots_TimestampsAreUTCOnNonUTCHost, which pins
time.Local to +07:13 and asserts a local row and a remote-only row
built from the same instant render the same wall clock in the table and
the same string in --json. Without the fix the local row renders
2026-03-01 17:13:00 against the remote row's 2026-03-01 10:00:00.

2. --json silently truncated. The early return skipped
reportListDrift, so neither the maxRemoteOnlyRows = 1000 cap nor the
unreadable-manifest count reached a machine consumer: past 1000
remote-only snapshots the document was short with no signal at all.

Both counts now go to v.Stderr — the same stream the
unreachable-destination warning already uses — via
reportJSONListingLimits. The document's shape is deliberately
unchanged, so existing consumers keep parsing; a consumer that must
react to truncation can treat any output on stderr as "this listing is
not the whole picture".

Covered by TestListSnapshots_JSONReportsTruncation (1001 remote-only
snapshots, asserts 1000 rows plus the truncation notice) and
TestListSnapshots_JSONReportsUnreadableManifests.

3. The --json stderr workaround was half-applied. Two
per-snapshot log.Warn calls on the same new path were left unguarded,
and internal/log builds its logger over os.Stdout at default level
Warn, so one corrupt manifest or one bad manifest timestamp put a JSON
log line on stdout ahead of the document and broke | jq.

Both now route through warnWhileListing, which picks v.Stderr in
--json mode and log.Warn otherwise. They are also collected during
the concurrent manifest reads and emitted afterwards in key order, since
v.Stderr is not safe for concurrent writes — that also makes warning
order deterministic. The workaround stays local and still carries the
comment saying to remove it when #82 lands. #82 itself is untouched.

Covered by TestListSnapshots_JSONStdoutIsOnlyTheDocument, which
redirects the process's own os.Stdout to a pipe and rebuilds the
logger over it, then points the JSON encoder and the UI at the same
pipe. That is what snapshot list --json | jq actually sees, and it is
the only way a test can observe the defect — log.Initialize binds to
os.Stdout, not to any writer a test can inject. With the fix reverted
the test captures both log lines ahead of the array and the parse fails.

Nits from the review, all three taken: the destination-listing
failure is no longer printed twice in table mode (UI.Warningf alone;
quiet mode still shows warnings, so nothing is lost); formatRemoteOnlyID
is no longer computed and discarded for locally tracked rows; and the
merge sort is sort.SliceStable, so rows sharing the zero-timestamp
fallback keep a deterministic order as a local property rather than an
incidental one.

Rework verification. The review was right that the earlier
script/cibuild exit code proved nothing — that build resolved from
layer cache. Forced uncached both ways this time:

GOFLAGS=-count=1 make check → EXIT=0. 14 ok package lines,
none marked (cached); lint printed 0 issues.
script/cibuild with BUILDKIT_PROGRESS=plain → EXIT=0, and
the source COPY invalidated both stages, so neither was CACHED:
#16 [lint 8/8] RUN make lint ran 57.7s and printed 0 issues.;
#23 [builder 8/9] RUN make test ran 69.1s and printed the ok lines
with none (cached).

.golangci.yml still hashes to
021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb.
Dockerfile, Makefile, .gitea/ and script/ remain byte-identical
to main. No existing test was weakened or deleted; the only change to
an existing test is one added field on the --json row struct and an
addRemote helper split so a manifest can carry a raw timestamp string.
TODO.md's Next Step remains unrotated, per the review.

What changed

The listing is now the union of the local index and the destination
store, with no age_secret_key gate. The manifest is unencrypted, so a
host holding only the public key can enumerate what it has backed up.

The naming constraint from the manager comment is honored exactly: a
remote-only snapshot's hostname and name are not recovered and
nothing new is written to remote storage. RemoteSnapshotKey is
one-way and the manifest stores the hash rather than the human ID, so
those live only in the local index and the encrypted db.zst.age.
Remote-only rows are labelled <remote only:<12 hex chars>>, carry the
real timestamp and total_compressed_size from the manifest, and
show <remote only> in the two columns that genuinely require the
local index.

Most of the listing code moved from the 1857-line snapshot.go into a
new internal/vaultik/snapshot_list.go, so the whole feature reads in
one place.

Verification

See the rework verification above for the current head. The original
end-to-end run against a file:// destination is unchanged and is kept
below.

The no-private-key property

Two independent checks.

Unit. TestListSnapshots_RemoteWithoutSecretKey builds a Vaultik
with AgeSecretKey: "" (asserted, so the test cannot silently stop
testing what it claims) against a storer that counts prefix listings
and records every fetched key. It asserts the destination was listed
exactly once, that no key containing .age was ever read, that the
remote-only row renders, and that the human ID is neither shown nor
fabricated. If the remote listing is ever gated on the secret key
again, this fails.

End to end, with the real binary against a file:// destination, a
config containing no age_secret_key, and no VAULTIK_AGE_SECRET_KEY
in the environment. Backup, then delete the local index, then list:

REMOTE SNAPSHOTS:
SNAPSHOT ID                  TIMESTAMP             COMPRESSED SIZE   UNCOMPRESSED SIZE   NEW CHUNK SIZE
───────────                  ─────────             ───────────────   ─���───────────────   ──────────────
&lt;remote only:5e5370c1480b&gt;   2026-08-09 03:09:26   195.6 KB          &lt;remote only&gt;       &lt;remote only&gt;
》 NOTE: 1 snapshot(s) on the backup destination store are not in the local index. Their hostname and snapshot name cannot be recovered without the age secret key, so they are listed by remote key.

With the index intact the same snapshot renders as a normal named row
with real numbers in all three size columns, confirming both branches:

sandcastle_demo_2026-08-09T03:09:26Z   2026-08-09 03:09:26   195.6 KB   195.3 KB   195.3 KB

--json on the same remote-only state:

[
  {
    "id": "",
    "remote_key": "5e5370c1480bf282d42aa95c94a7c7bc8ff6fd5a9b3603660a745d0462604137",
    "timestamp": "2026-08-09T03:09:26Z",
    "compressed_size": 200293,
    "locally_tracked": false,
    "remote_present": true
  }
]

So requirement 2 is verified, not assumed: remoteOnlyCell and the
LocallyTracked == false branch render correctly in both the table and
JSON, in tests and against the real binary.

Requirements

6 — the vaultik snapshot cleanup string: renamed to vaultik prune. The issue asks whether the command was removed or never
added. It was removed and the message orphaned:
internal/vaultik/prune.go:80-82 calls CleanupLocalSnapshots and its
comment says so outright — "This used to be the separate 'snapshot
cleanup' command and is now folded in". So CleanupLocalSnapshots is
not unreachable; it is prune's first pass. Re-adding a snapshot cleanup command would restore the duplicate entry point that the
2026-07-02 CLI consolidation deliberately removed, so the message now
names vaultik prune, and a test asserts the string snapshot cleanup
does not appear in the output.

9 — reportRemoteDrift collapses. Its remote-only half is fully
subsumed by the merged table: those snapshots are rows now, with real
timestamps and sizes, rather than a bare count in a footnote. Its
local-only half is still meaningful but no longer needs a remote
listing of its own — it became reportListDrift, which reads the merge
ListSnapshots already computed. Net effect: snapshot list lists the
destination exactly once per invocation, where the old code would have
listed it twice had both halves ever run.

10 — the stale branch needs nothing folded in. origin/fix/sync-snapshot-cleanup
(tip 332ea26) changes exactly one line: v.Repositories.Snapshots.Delete(...)
to v.deleteSnapshotFromLocalDB(...) in syncWithRemote. That change
is already on main at internal/vaultik/snapshot.go:1186, having
landed independently through the deleteSnapshotFromLocalDB
error-propagation work (ddc23f8 / 597b560). The three-dot diff still
shows it only because the merge base (c24e7e6) predates both. There is
nothing to fold and nothing to collide with; the branch is redundant and
can be deleted under #71.

7 — single manifest reader. downloadManifestByKey is now the only
place in the codebase that reads a remote manifest. verify.go and
info.go each opened and decoded
metadata/<key>/manifest.json.zst inline; both were routed through the
helper in the first commit, and its doc comment now states the
invariant and why #81 depends on it.

8 — bounding. One streamed listing of the metadata/ prefix, so
request count does not scale with snapshot count. Manifest reads happen
only for keys the local index does not already account for, capped at
maxRemoteOnlyRows (1000) with the overflow reported as a count — in
table mode and now in --json too — and run through an errgroup
limited to 8 in flight. Nothing beyond the per-row summaries is
retained.

3 — local-only drift is still reported below the table, and is now
also visible in --json via remote_present: false.

4 — degradation. An unreachable destination is a warning plus
local-only output and a zero exit code. remote_present is null
rather than false, so "absent" and "unknown" stay distinguishable, and
no drift is claimed on the basis of a listing that never happened.

5 — --json gains remote_key (full 64-character key) and
remote_present, alongside the existing locally_tracked.

One thing to flag

In --json mode every warning on the listing path is written to
v.Stderr directly rather than through log or v.UI. Both of those
write to stdout (internal/log/log.go:73-78), which would corrupt
the JSON document. That is a pre-existing bug affecting every --json
command — an 0644 config file alone is enough to break snapshot list --json | jq on main today — so it is filed as #82 rather than fixed
drive-by. The workaround here should be removed once #82 lands.

Tests

internal/vaultik/snapshot_list_test.go, all against a config with no
age secret key:

TestListSnapshots_RemoteWithoutSecretKey — the regression guard
described above.
TestListSnapshots_RemoteOnlyRowRendering — pins the remote-only row:
identifier column leads with the abbreviated key, real timestamp and
size, exactly two <remote only> cells.
TestListSnapshots_MergesLocalAndRemote — both sources in one table;
the locally tracked row keeps its human ID and is not marked
remote-only.
TestListSnapshots_LocalOnlyReportedAsDrift — drift reported, hint
names vaultik prune, output does not contain snapshot cleanup.
TestListSnapshots_UnreachableRemoteDegrades — warning, local rows
still printed, nil return (exit 0), and no drift claimed.
TestListSnapshots_UnreadableManifestDoesNotHideOthers — one corrupt
manifest cannot suppress the rest.
TestListSnapshots_JSONMergedView — all three states in one document.
TestListSnapshots_JSONUnreachableRemote — stdout stays parseable,
remote_present is null, warning on stderr.
TestListSnapshots_TimestampsAreUTCOnNonUTCHost — new; blocking
finding 1.
TestListSnapshots_JSONReportsUnreadableManifests and
TestListSnapshots_JSONReportsTruncation — new; blocking finding 2.
TestListSnapshots_JSONStdoutIsOnlyTheDocument — new; blocking
finding 3.

And internal/database/snapshots_test.go:

TestSnapshotTimestampsDecodeAsUTC — new; blocking finding 1, at the
normalization point itself.

No existing assertions were weakened or removed.

Docs

README's snapshot list section and the ListSnapshots doc comment
both rewritten to match implemented behavior. TODO.md updated in the
same commit as the work.

Note on TODO.md: I added a Completed Steps entry but left Next
Step as it was ("Triage the stale remote branches, issue #71"),
because that is not the work I did — rotating it would have recorded a
task as done that isn't. This PR does resolve one of #71's branches:
fix/sync-snapshot-cleanup is redundant and can be deleted.

Commits

0952925 Route every remote manifest read through
downloadManifestByKey
0e2929d List remote snapshots without requiring the private key
(closes #64)
9a45221 Fix timezone drift and --json truncation in snapshot list
(closes #64)

Fixes #64. `ListSnapshots` built its table entirely from the local SQLite index. The only remote access, `reportRemoteDrift`, was gated on `AgeSecretKey != ""` — so on a correctly configured host, which by design holds no private key, `snapshot list` never contacted the destination store at all. A user who lost their local index could not see their own backups, and the `<remote only>` cell the README documents was unreachable dead code. ## Review rework (head `9a45221`) The first review returned FAIL with three blocking findings. All three are fixed in `9a45221`, each with a test that fails without the fix. **No JSON shape change**, so nothing here breaks a machine consumer. **1. The TIMESTAMP column mixed two timezones.** Remote-only rows rendered `timestamp.UTC()`; local rows came from `ListRecent`, whose scanner (`internal/database/snapshots.go:767`) decoded Unix seconds in the host's local zone — unlike its two siblings at lines 200 and 639, which decode in UTC. Both render through the same zone-less format string, so on a non-UTC host the same snapshot showed one time when locally tracked and a different time when remote-only, in the same column, with nothing to indicate why. Fixed at the point the timestamp enters the domain, not at the display layer: `scanSnapshotRows` now decodes in UTC like its siblings, and `GetIncompleteByHostname`'s inline copy of that loop was folded onto the shared scanner, so the call sites cannot drift apart again. (That fold was also required by `dupl`, which correctly started firing once the two loops became token-identical.) Covered by `TestSnapshotTimestampsDecodeAsUTC` (`internal/database/snapshots_test.go`), which checks every snapshot reader and compares `*time.Location` pointers, so it is equally strict on a UTC host — `time.Unix` returns `time.Local`, which is never the same `Location` value as `time.UTC`; and by `TestListSnapshots_TimestampsAreUTCOnNonUTCHost`, which pins `time.Local` to `+07:13` and asserts a local row and a remote-only row built from the same instant render the same wall clock in the table and the same string in `--json`. Without the fix the local row renders `2026-03-01 17:13:00` against the remote row's `2026-03-01 10:00:00`. **2. `--json` silently truncated.** The early return skipped `reportListDrift`, so neither the `maxRemoteOnlyRows` = 1000 cap nor the unreadable-manifest count reached a machine consumer: past 1000 remote-only snapshots the document was short with no signal at all. Both counts now go to `v.Stderr` — the same stream the unreachable-destination warning already uses — via `reportJSONListingLimits`. The document's shape is deliberately unchanged, so existing consumers keep parsing; a consumer that must react to truncation can treat any output on stderr as "this listing is not the whole picture". Covered by `TestListSnapshots_JSONReportsTruncation` (1001 remote-only snapshots, asserts 1000 rows plus the truncation notice) and `TestListSnapshots_JSONReportsUnreadableManifests`. **3. The `--json` stderr workaround was half-applied.** Two per-snapshot `log.Warn` calls on the same new path were left unguarded, and `internal/log` builds its logger over `os.Stdout` at default level `Warn`, so one corrupt manifest or one bad manifest timestamp put a JSON log line on stdout ahead of the document and broke `| jq`. Both now route through `warnWhileListing`, which picks `v.Stderr` in `--json` mode and `log.Warn` otherwise. They are also collected during the concurrent manifest reads and emitted afterwards in key order, since `v.Stderr` is not safe for concurrent writes — that also makes warning order deterministic. The workaround stays local and still carries the comment saying to remove it when #82 lands. **#82 itself is untouched.** Covered by `TestListSnapshots_JSONStdoutIsOnlyTheDocument`, which redirects the *process's own* `os.Stdout` to a pipe and rebuilds the logger over it, then points the JSON encoder and the UI at the same pipe. That is what `snapshot list --json | jq` actually sees, and it is the only way a test can observe the defect — `log.Initialize` binds to `os.Stdout`, not to any writer a test can inject. With the fix reverted the test captures both log lines ahead of the array and the parse fails. **Nits from the review, all three taken:** the destination-listing failure is no longer printed twice in table mode (`UI.Warningf` alone; quiet mode still shows warnings, so nothing is lost); `formatRemoteOnlyID` is no longer computed and discarded for locally tracked rows; and the merge sort is `sort.SliceStable`, so rows sharing the zero-timestamp fallback keep a deterministic order as a local property rather than an incidental one. **Rework verification.** The review was right that the earlier `script/cibuild` exit code proved nothing — that build resolved from layer cache. Forced uncached both ways this time: - `GOFLAGS=-count=1 make check` → **`EXIT=0`**. 14 `ok` package lines, **none** marked `(cached)`; lint printed `0 issues.` - `script/cibuild` with `BUILDKIT_PROGRESS=plain` → **`EXIT=0`**, and the source `COPY` invalidated both stages, so neither was `CACHED`: `#16 [lint 8/8] RUN make lint` ran 57.7s and printed `0 issues.`; `#23 [builder 8/9] RUN make test` ran 69.1s and printed the `ok` lines with none `(cached)`. `.golangci.yml` still hashes to `021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb`. `Dockerfile`, `Makefile`, `.gitea/` and `script/` remain byte-identical to `main`. No existing test was weakened or deleted; the only change to an existing test is one added field on the `--json` row struct and an `addRemote` helper split so a manifest can carry a raw timestamp string. `TODO.md`'s **Next Step** remains unrotated, per the review. ## What changed The listing is now the union of the local index and the destination store, with no `age_secret_key` gate. The manifest is unencrypted, so a host holding only the public key can enumerate what it has backed up. The naming constraint from the manager comment is honored exactly: a remote-only snapshot's hostname and name are **not** recovered and nothing new is written to remote storage. `RemoteSnapshotKey` is one-way and the manifest stores the hash rather than the human ID, so those live only in the local index and the encrypted `db.zst.age`. Remote-only rows are labelled `<remote only:<12 hex chars>>`, carry the real `timestamp` and `total_compressed_size` from the manifest, and show `<remote only>` in the two columns that genuinely require the local index. Most of the listing code moved from the 1857-line `snapshot.go` into a new `internal/vaultik/snapshot_list.go`, so the whole feature reads in one place. ## Verification See the rework verification above for the current head. The original end-to-end run against a `file://` destination is unchanged and is kept below. ### The no-private-key property Two independent checks. **Unit.** `TestListSnapshots_RemoteWithoutSecretKey` builds a `Vaultik` with `AgeSecretKey: ""` (asserted, so the test cannot silently stop testing what it claims) against a storer that counts prefix listings and records every fetched key. It asserts the destination was listed exactly once, that no key containing `.age` was ever read, that the remote-only row renders, and that the human ID is neither shown nor fabricated. If the remote listing is ever gated on the secret key again, this fails. **End to end,** with the real binary against a `file://` destination, a config containing no `age_secret_key`, and no `VAULTIK_AGE_SECRET_KEY` in the environment. Backup, then delete the local index, then list: ``` REMOTE SNAPSHOTS: SNAPSHOT ID TIMESTAMP COMPRESSED SIZE UNCOMPRESSED SIZE NEW CHUNK SIZE ─────────── ───────── ─────────────── ─��─────────────── ────────────── <remote only:5e5370c1480b> 2026-08-09 03:09:26 195.6 KB <remote only> <remote only> 》 NOTE: 1 snapshot(s) on the backup destination store are not in the local index. Their hostname and snapshot name cannot be recovered without the age secret key, so they are listed by remote key. ``` With the index intact the same snapshot renders as a normal named row with real numbers in all three size columns, confirming both branches: ``` sandcastle_demo_2026-08-09T03:09:26Z 2026-08-09 03:09:26 195.6 KB 195.3 KB 195.3 KB ``` `--json` on the same remote-only state: ```json [ { "id": "", "remote_key": "5e5370c1480bf282d42aa95c94a7c7bc8ff6fd5a9b3603660a745d0462604137", "timestamp": "2026-08-09T03:09:26Z", "compressed_size": 200293, "locally_tracked": false, "remote_present": true } ] ``` So requirement 2 is verified, not assumed: `remoteOnlyCell` and the `LocallyTracked == false` branch render correctly in both the table and JSON, in tests and against the real binary. ## Requirements **6 — the `vaultik snapshot cleanup` string: renamed to `vaultik prune`.** The issue asks whether the command was removed or never added. It was removed and the message orphaned: `internal/vaultik/prune.go:80-82` calls `CleanupLocalSnapshots` and its comment says so outright — "This used to be the separate 'snapshot cleanup' command and is now folded in". So `CleanupLocalSnapshots` is not unreachable; it is `prune`'s first pass. Re-adding a `snapshot cleanup` command would restore the duplicate entry point that the 2026-07-02 CLI consolidation deliberately removed, so the message now names `vaultik prune`, and a test asserts the string `snapshot cleanup` does not appear in the output. **9 — `reportRemoteDrift` collapses.** Its remote-only half is fully subsumed by the merged table: those snapshots are rows now, with real timestamps and sizes, rather than a bare count in a footnote. Its local-only half is still meaningful but no longer needs a remote listing of its own — it became `reportListDrift`, which reads the merge `ListSnapshots` already computed. Net effect: `snapshot list` lists the destination exactly once per invocation, where the old code would have listed it twice had both halves ever run. **10 — the stale branch needs nothing folded in.** `origin/fix/sync-snapshot-cleanup` (tip `332ea26`) changes exactly one line: `v.Repositories.Snapshots.Delete(...)` to `v.deleteSnapshotFromLocalDB(...)` in `syncWithRemote`. That change is **already on `main`** at `internal/vaultik/snapshot.go:1186`, having landed independently through the `deleteSnapshotFromLocalDB` error-propagation work (`ddc23f8` / `597b560`). The three-dot diff still shows it only because the merge base (`c24e7e6`) predates both. There is nothing to fold and nothing to collide with; the branch is redundant and can be deleted under #71. **7 — single manifest reader.** `downloadManifestByKey` is now the only place in the codebase that reads a remote manifest. `verify.go` and `info.go` each opened and decoded `metadata/<key>/manifest.json.zst` inline; both were routed through the helper in the first commit, and its doc comment now states the invariant and why #81 depends on it. **8 — bounding.** One streamed listing of the `metadata/` prefix, so request count does not scale with snapshot count. Manifest reads happen only for keys the local index does not already account for, capped at `maxRemoteOnlyRows` (1000) with the overflow reported as a count — in table mode **and now in `--json` too** — and run through an `errgroup` limited to 8 in flight. Nothing beyond the per-row summaries is retained. **3 — local-only drift** is still reported below the table, and is now also visible in `--json` via `remote_present: false`. **4 — degradation.** An unreachable destination is a warning plus local-only output and a zero exit code. `remote_present` is `null` rather than `false`, so "absent" and "unknown" stay distinguishable, and no drift is claimed on the basis of a listing that never happened. **5 — `--json`** gains `remote_key` (full 64-character key) and `remote_present`, alongside the existing `locally_tracked`. ## One thing to flag In `--json` mode every warning on the listing path is written to `v.Stderr` directly rather than through `log` or `v.UI`. Both of those write to **stdout** (`internal/log/log.go:73-78`), which would corrupt the JSON document. That is a pre-existing bug affecting every `--json` command — an 0644 config file alone is enough to break `snapshot list --json | jq` on `main` today — so it is filed as #82 rather than fixed drive-by. The workaround here should be removed once #82 lands. ## Tests `internal/vaultik/snapshot_list_test.go`, all against a config with no age secret key: - `TestListSnapshots_RemoteWithoutSecretKey` — the regression guard described above. - `TestListSnapshots_RemoteOnlyRowRendering` — pins the remote-only row: identifier column leads with the abbreviated key, real timestamp and size, exactly two `<remote only>` cells. - `TestListSnapshots_MergesLocalAndRemote` — both sources in one table; the locally tracked row keeps its human ID and is not marked remote-only. - `TestListSnapshots_LocalOnlyReportedAsDrift` — drift reported, hint names `vaultik prune`, output does not contain `snapshot cleanup`. - `TestListSnapshots_UnreachableRemoteDegrades` — warning, local rows still printed, nil return (exit 0), and no drift claimed. - `TestListSnapshots_UnreadableManifestDoesNotHideOthers` — one corrupt manifest cannot suppress the rest. - `TestListSnapshots_JSONMergedView` — all three states in one document. - `TestListSnapshots_JSONUnreachableRemote` — stdout stays parseable, `remote_present` is null, warning on stderr. - `TestListSnapshots_TimestampsAreUTCOnNonUTCHost` — new; blocking finding 1. - `TestListSnapshots_JSONReportsUnreadableManifests` and `TestListSnapshots_JSONReportsTruncation` — new; blocking finding 2. - `TestListSnapshots_JSONStdoutIsOnlyTheDocument` — new; blocking finding 3. And `internal/database/snapshots_test.go`: - `TestSnapshotTimestampsDecodeAsUTC` — new; blocking finding 1, at the normalization point itself. No existing assertions were weakened or removed. ## Docs README's `snapshot list` section and the `ListSnapshots` doc comment both rewritten to match implemented behavior. `TODO.md` updated in the same commit as the work. Note on `TODO.md`: I added a Completed Steps entry but left **Next Step** as it was ("Triage the stale remote branches, issue #71"), because that is not the work I did — rotating it would have recorded a task as done that isn't. This PR does resolve one of #71's branches: `fix/sync-snapshot-cleanup` is redundant and can be deleted. ## Commits - `0952925` Route every remote manifest read through `downloadManifestByKey` - `0e2929d` List remote snapshots without requiring the private key (closes #64) - `9a45221` Fix timezone drift and `--json` truncation in snapshot list (closes #64)

clawbot added 2 commits 2026-08-09 05:16:59 +02:00

Route every remote manifest read through downloadManifestByKey 0952925453

internal/vaultik/verify.go and internal/vaultik/info.go each built the
metadata/<remote-key>/manifest.json.zst path and decoded the manifest
inline, duplicating downloadManifestByKey. Both now call the helper.

The manifest is currently stored compressed but unencrypted, which is
what will let `snapshot list` enumerate the destination store on a host
holding no private key. Whether to encrypt it is still open (#81), and
a single reader means that decision has one call site to change rather
than four.

Refs #81

List remote snapshots without requiring the private key (closes #64 )

check / check (pull_request) Successful in 2m13s

Details

0e2929d75d

ListSnapshots built its table entirely from the local SQLite index. The
only remote access, reportRemoteDrift, was gated on AgeSecretKey being
non-empty — so on a correctly configured host, which by design holds no
private key, `snapshot list` never contacted the destination store at
all. A user who lost their local index could not see their own backups,
and the "<remote only>" cell the README documents was unreachable dead
code.

The listing is now the union of the local index and the destination
store, with no age_secret_key gate. The manifest is unencrypted, so a
host holding only the public key can enumerate what it has backed up:
one streamed listing of the metadata/ prefix, then a manifest read per
remote key the local index does not already account for, bounded by
maxRemoteOnlyRows and run with bounded concurrency.

A remote-only snapshot's hostname and name are deliberately NOT
recovered. RemoteSnapshotKey is one-way and the manifest stores the
hash rather than the human ID, so they are recoverable only from the
encrypted per-snapshot database; making them readable from remote
storage would undo a deliberate privacy property (#81). Such rows are
labelled "<remote only:<12 hex chars>>", carry the real timestamp and
compressed size from the manifest, and show "<remote only>" in the two
columns that genuinely require the local index. --json carries the full
64-character key in remote_key, and remote_present distinguishes seen
(true), missing (false) and not-listable (null).

Local records with no counterpart on the destination store are still
surfaced as drift, and the remediation hint now names `vaultik prune`,
which exists, rather than `vaultik snapshot cleanup`, which does not:
CleanupLocalSnapshots is already wired as prune's first pass, and
re-adding a second entry point would undo the CLI consolidation.

reportRemoteDrift collapses. Its remote-only half is subsumed by the
table — those snapshots are rows now, not a footnote count — and its
local-only half reads the merge ListSnapshots already computed, so the
command lists the destination exactly once per invocation.

An unreachable destination stays a warning plus local-only output and a
zero exit code, as the doc comment always claimed. In --json mode that
warning goes to stderr, because the logger and the UI writer both emit
on stdout and would otherwise corrupt the document.

Tests cover remote-only rendering, the no-private-key property (a
storer that counts prefix listings and records fetched keys, asserting
the destination is read and nothing encrypted is touched), graceful
degradation on an unreachable destination in both output modes,
local-only drift, and an unreadable manifest not hiding other
snapshots.

clawbot added the needs-review label 2026-08-09 06:55:45 +02:00

clawbot self-assigned this 2026-08-09 06:55:46 +02:00

clawbot added this to the 1.0.0 milestone 2026-08-09 06:55:46 +02:00

clawbot commented

2026-08-09 07:03:51 +02:00

Review: PR #83 — FAIL (`needs-rework`)

Head 0e2929d, base main af607e3. Mergeable (base is an ancestor of
head; no conflicts). CI green on the head commit (check / check (pull_request), success, 2m13s).

The design is right, the privacy constraint is honored exactly, and the
tests are real. Three defects in the newly-merged output path block it.

Gate

script/cibuild → EXIT=0.

That result is worthless on its own: every stage resolved from the layer
cache (#17 [lint 8/8] RUN make lint → CACHED, #18 [builder 8/9] RUN make test → CACHED), so neither the linter nor the test suite
actually executed. I re-ran the checks uncached through the repo
entrypoints (GOFLAGS=-count=1 make check, which is script/check →
script/test with -race, script/lint against the digest-pinned
image, script/fmt-check):

14 ok package lines, none marked (cached).
Lint: 0 issues.
EXIT=0

So the gate is genuinely green; the author's claim is true, it just
could not be confirmed from the cached build alone. (The
gomodguard-deprecation warning in the lint output is pre-existing from
the #61 rollout, not this PR.)

Blocking findings

1. The merged TIMESTAMP column mixes two timezones

internal/vaultik/snapshot_list.go:291 — remote-only rows:
Timestamp: timestamp.UTC()
internal/vaultik/snapshot_list.go:336 — local rows:
Timestamp: ls.StartedAt
internal/database/snapshots.go:767 (scanSnapshotRows, the scanner
ListRecent uses): snapshot.StartedAt = time.Unix(startedAtUnix, 0)
— no .UTC(), unlike the sibling scanners at lines 200 and 639
which do call .UTC().
internal/vaultik/snapshot_list.go:476 renders both with
snap.Timestamp.Format("2006-01-02 15:04:05") — no zone suffix.

On any host whose TZ is not UTC, locally-tracked rows print local wall
time and remote-only rows print UTC, in the same column, with nothing to
distinguish them. A snapshot taken at 12:00 CEST shows as 12:00:00
while the identical snapshot, once the local index is gone, shows as
10:00:00. This is precisely the reconciliation the feature exists to
support, and the table silently lies about it. The --json timestamp
field inherits the same split (RFC3339 offset +02:00 for local rows,
Z for remote-only), so string comparison across rows breaks too.

Neither the unit tests nor the author's end-to-end run could catch this:
every fixture timestamp is constructed in time.UTC, and the
file:// verification appears to have run on a UTC host.

Why it matters: the merged table is the deliverable of this issue, and
its primary column is not comparable between the two sources it merges.

Acceptable: normalize at one place. Either force local rows to UTC in
snapshotInfoFromLocal (or fix scanSnapshotRows to match its two
siblings), or convert at render time in printSnapshotTable. Sorting is
unaffected either way (time.Time comparison is absolute), so this is a
display/serialization fix only. A test with a non-UTC local timestamp
asserting both rows render on the same clock would pin it.

2. `--json` silently truncates and silently drops rows

internal/vaultik/snapshot_list.go:102-107 returns immediately after
encoding, so reportListDrift (:354) never runs in JSON mode. Every
signal it carries is therefore JSON-invisible:

listing.omitted — the maxRemoteOnlyRows = 1000 cap
(:200-203). Past 1000 remote-only snapshots the JSON document is
truncated with no indication whatsoever. In table mode this is
correctly reported (:388-391); in machine-readable mode it is not.
listing.unreadable — a snapshot whose manifest is corrupt simply
does not appear in the JSON array, indistinguishable from one that
does not exist on the destination.

A consumer piping snapshot list --json into automation cannot
distinguish "these are all the snapshots" from "these are the first 1000
of N, and M more were unreadable." Silent truncation of a listing whose
entire purpose is disaster recovery is the wrong failure mode.

Acceptable: surface both counts in the JSON path — either as a
top-level envelope alongside the rows, or (if the bare-array shape must
be preserved for compatibility) written to v.Stderr the same way the
unreachable-destination warning already is. Add a test asserting a
truncated/partial listing is detectable from --json output.

3. The `--json` stderr workaround is not scoped correctly, and the gap is on the path it was written for

warnRemoteListingFailed (:132-144) correctly routes the
listing-failure warning to v.Stderr in JSON mode, because
internal/log/log.go:72-78 builds the logger over os.Stdout in
both the TTY and JSON-handler branches, and internal/log/log.go:62
sets the default level to slog.LevelWarn. So log.Warn is emitted on
stdout by default, with no flags.

But two log.Warn calls on the very same remote-only path were left
unguarded:

internal/vaultik/snapshot_list.go:229-230 — log.Warn("Could not describe remote snapshot", ...)
internal/vaultik/snapshot_list.go:283-284 — log.Warn("Remote manifest has an unparseable timestamp", ...)

vaultik snapshot list --json with one corrupt manifest or one bad
manifest timestamp therefore emits a JSON log line onto stdout ahead
of the JSON document, and | jq fails. That is exactly the corruption
the v.Stderr workaround at :134 exists to prevent, on exactly the
degradation scenario the PR treats as first-class —
TestListSnapshots_UnreadableManifestDoesNotHideOthers covers it in
table mode only, and the tests cannot see it because
log.Initialize(log.Config{}) points the logger at the test process's
real stdout rather than env.stdout.

I accept the v.Stderr workaround itself as reasonable given #82, and
filing rather than fixing #82 drive-by was the right call. The objection
is narrower: within the one function the workaround was added to, the
same hazard was left in two places. Half-applied, it is the kind of
inconsistency that rots — the next reader sees log.Warn used freely
here and concludes it is safe.

Acceptable: make the JSON-mode discipline uniform on this path — thread
the jsonOutput flag into describeRemoteOnlySnapshots /
remoteSnapshotInfo, or collect these failures and emit them through
the same writer warnRemoteListingFailed already chooses. A test that
points the logger at a captured buffer and asserts stdout is
byte-for-byte a JSON document would pin it.

(Noted, not blocking: the pre-existing log.Warn calls in
snapshotInfoFromLocal at :317, :325, :330 have the same hazard
and are unchanged from main. They are #82's problem, not this PR's.)

Verified clean

Everything below I checked directly rather than taking from the PR body.

Definition of done. All eight items satisfied, plus the five extra
requirements from the manager comment.

The core property (DoD 1).
TestListSnapshots_RemoteWithoutSecretKey
(internal/vaultik/snapshot_list_test.go:211-255) is a genuine
regression guard, not a name. It asserts Config.AgeSecretKey is empty
up front (so it cannot silently stop testing its premise), asserts
exactly one prefix listing was issued, asserts no fetched key contains
.age, and asserts the remote-only row rendered. Reinstating the
AgeSecretKey == "" early return would drive listStreamCalls to 0 and
remove the row — it fails on both counts. The AgeSecretKey gate is
gone from the code entirely.

Privacy constraint (the one that matters most). Fully honored.

No writes to remote storage on the listing path: the only storage
calls are ListStream and Get (no Put anywhere in
snapshot_list.go).
The human ID is never fabricated, guessed, or brute-forced.
remoteSnapshotInfo (:272-295) leaves ID zero, with a doc comment
stating why. --json asserts assert.Empty(t, remoteOnly.ID) at
test line 497.
internal/snapshot/remotekey.go and internal/snapshot/manifest.go
are byte-identical to main — RemoteSnapshotKey and the manifest
format are unchanged.
The manifest is read and never written by this code. The only
snapshot.DecodeManifest call site in the entire repo is inside
downloadManifestByKey (internal/vaultik/snapshot.go:905).

Concurrency. No data race. describeRemoteOnlySnapshots
(:217-260) writes only to distinct indices found[i] / ok[i], and
go.mod declares go 1.26.1 so loop variables are per-iteration. The
full suite passes under -race (script/test sets it). One manifest
read cannot abort the listing: the goroutine body returns nil
unconditionally (:232, :238) and failures are recorded in ok, so
errgroup never cancels — TestListSnapshots_UnreadableManifestDoesNotHideOthers
genuinely verifies this (a real non-zstd payload, asserting the good row
is present and the bad one is counted). Ordering is deterministic:
unknown is sorted before both truncation and dispatch (:196-198),
results are reassembled in index order, and the final sort key is
absolute time. The 1000 cap is reported in table mode — but see
blocking finding 2 for JSON mode.

Degradation (DoD 3). ListSnapshots returns nil on remote
failure, verified by TestListSnapshots_UnreachableRemoteDegrades and
TestListSnapshots_JSONUnreachableRemote. markRemotePresence
(:301-306) is called only in the remoteErr == nil branch, so
RemotePresent stays a nil *bool → null, never false. The
*bool type makes "absent" and "unknown" structurally
distinguishable. reportListDrift is gated on remoteErr == nil
(:114-116), so no drift is claimed from a listing that never
happened — asserted directly at test line 396.

Behavior preservation. I read the 275 deleted lines against the new
file. printSnapshotTable and snapshotInfoFromLocal moved verbatim;
the only table change is the identifier cell, which now falls back to
formatRemoteOnlyID when !LocallyTracked (previously it printed
snap.ID unconditionally, which was always populated because
LocallyTracked was hardcoded true). Locally-tracked rows render
identically. reportRemoteDrift → reportListDrift drops no working
case: the local-only warning, the per-ID list, and the remediation hint
all survive; the remote-only branch was a bare count and is now real
rows. Both unreadable and omitted are new additions.

PR body claim — origin/fix/sync-snapshot-cleanup is redundant.
True. Its tip 332ea26 changes exactly one line in syncWithRemote
(Repositories.Snapshots.Delete → deleteSnapshotFromLocalDB), and
main already carries that change at
internal/vaultik/snapshot.go:1186. Confirmed by reading
git show origin/main:internal/vaultik/snapshot.go. It shows in the
three-dot diff only because the merge base c24e7e6 predates it. No
real fix is being discarded.

PR body claim — single manifest reader. True. DecodeManifest
has exactly one call site repo-wide (snapshot.go:905, inside
downloadManifestByKey). The remaining manifest.json.zst string
occurrences are: the upload in internal/snapshot/snapshot.go:508 (a
Put, not a read), two key-name filters in listing loops
(prune.go:223, snapshot.go:1247), tests, and doc comments.
verify.go and info.go were genuinely routed through the helper.

snapshot cleanup string. Gone from all Go source; the only
remaining occurrences are the TODO entry describing the removal and the
test asserting its absence (snapshot_list_test.go:367).
CleanupLocalSnapshots is reachable: Prune calls it at
internal/vaultik/prune.go:82 as its first pass, with the comment
saying so. Not dead. Renaming rather than re-adding a duplicate entry
point was the right call.

Nothing weakened. .golangci.yml hashes to
021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb as
required. Dockerfile, Makefile, .gitea/, script/ are
byte-identical to main (empty git diff). No deleted tests, no
t.Skip, no weakened assertions, no file-level //nolint. One new
//nolint (helpers.go:76, tagliatelle) is per-site, justified, and
matches the directive already on that struct on main. No new
dependency: golang.org/x/sync was already a direct require.

Repo policy. No Claude/Anthropic references in commits, PR body,
code, or comments. No attribution trailers (git log --format='%(trailers)'
empty on both commits). Landing commit title ends with (closes #64).
No scope creep — the downloadManifestByKey consolidation is a separate
commit and is explicitly requested by the issue's manager comment.
Inclusive terminology clean. make fmt-check clean.

TODO.md. Updated in the same commit as the work and factually
accurate against the diff. Leaving Next Step as "Triage the stale
remote branches (issue #71)" was correct — the workflow says to
rotate the step you did, and this was not it. Recording #71 as done
would have been a false claim; the entry going to Completed Steps with
Next Step untouched is the honest bookkeeping.

Nits (non-blocking)

internal/vaultik/snapshot_list.go:141-143 — in non-JSON mode
warnRemoteListingFailed calls both log.Warn and v.UI.Warningf
with the same message. Since the logger writes to stdout at default
level, the user sees the warning twice. reportRemoteDrift on main
emitted it once.
internal/vaultik/snapshot_list.go:464 — formatRemoteOnlyID is
computed for every row and discarded for locally-tracked ones. Free
to move inside the if.
internal/vaultik/snapshot_list.go:98 — sort.Slice is not stable.
Output is still deterministic here because the input order is, but
sort.SliceStable would make that property local rather than
incidental, which matters once many remote-only rows share the zero
timestamp from the unparseable-timestamp fallback at :286.

## Review: PR #83 — FAIL (`needs-rework`) Head `0e2929d`, base `main` `af607e3`. Mergeable (base is an ancestor of head; no conflicts). CI green on the head commit (`check / check (pull_request)`, success, 2m13s). The design is right, the privacy constraint is honored exactly, and the tests are real. Three defects in the newly-merged output path block it. --- ### Gate `script/cibuild` → **`EXIT=0`**. That result is worthless on its own: every stage resolved from the layer cache (`#17 [lint 8/8] RUN make lint` → `CACHED`, `#18 [builder 8/9] RUN make test` → `CACHED`), so neither the linter nor the test suite actually executed. I re-ran the checks uncached through the repo entrypoints (`GOFLAGS=-count=1 make check`, which is `script/check` → `script/test` with `-race`, `script/lint` against the digest-pinned image, `script/fmt-check`): - 14 `ok` package lines, **none** marked `(cached)`. - Lint: `0 issues.` - `EXIT=0` So the gate is genuinely green; the author's claim is true, it just could not be confirmed from the cached build alone. (The `gomodguard`-deprecation warning in the lint output is pre-existing from the #61 rollout, not this PR.) --- ### Blocking findings #### 1. The merged TIMESTAMP column mixes two timezones - `internal/vaultik/snapshot_list.go:291` — remote-only rows: `Timestamp: timestamp.UTC()` - `internal/vaultik/snapshot_list.go:336` — local rows: `Timestamp: ls.StartedAt` - `internal/database/snapshots.go:767` (`scanSnapshotRows`, the scanner `ListRecent` uses): `snapshot.StartedAt = time.Unix(startedAtUnix, 0)` — **no** `.UTC()`, unlike the sibling scanners at lines 200 and 639 which do call `.UTC()`. - `internal/vaultik/snapshot_list.go:476` renders both with `snap.Timestamp.Format("2006-01-02 15:04:05")` — no zone suffix. On any host whose `TZ` is not UTC, locally-tracked rows print local wall time and remote-only rows print UTC, in the same column, with nothing to distinguish them. A snapshot taken at 12:00 CEST shows as `12:00:00` while the identical snapshot, once the local index is gone, shows as `10:00:00`. This is precisely the reconciliation the feature exists to support, and the table silently lies about it. The `--json` `timestamp` field inherits the same split (RFC3339 offset `+02:00` for local rows, `Z` for remote-only), so string comparison across rows breaks too. Neither the unit tests nor the author's end-to-end run could catch this: every fixture timestamp is constructed in `time.UTC`, and the `file://` verification appears to have run on a UTC host. Why it matters: the merged table is the deliverable of this issue, and its primary column is not comparable between the two sources it merges. Acceptable: normalize at one place. Either force local rows to UTC in `snapshotInfoFromLocal` (or fix `scanSnapshotRows` to match its two siblings), or convert at render time in `printSnapshotTable`. Sorting is unaffected either way (`time.Time` comparison is absolute), so this is a display/serialization fix only. A test with a non-UTC local timestamp asserting both rows render on the same clock would pin it. #### 2. `--json` silently truncates and silently drops rows `internal/vaultik/snapshot_list.go:102-107` returns immediately after encoding, so `reportListDrift` (`:354`) never runs in JSON mode. Every signal it carries is therefore JSON-invisible: - `listing.omitted` — the `maxRemoteOnlyRows` = 1000 cap (`:200-203`). Past 1000 remote-only snapshots the JSON document is truncated with **no** indication whatsoever. In table mode this is correctly reported (`:388-391`); in machine-readable mode it is not. - `listing.unreadable` — a snapshot whose manifest is corrupt simply does not appear in the JSON array, indistinguishable from one that does not exist on the destination. A consumer piping `snapshot list --json` into automation cannot distinguish "these are all the snapshots" from "these are the first 1000 of N, and M more were unreadable." Silent truncation of a listing whose entire purpose is disaster recovery is the wrong failure mode. Acceptable: surface both counts in the JSON path — either as a top-level envelope alongside the rows, or (if the bare-array shape must be preserved for compatibility) written to `v.Stderr` the same way the unreachable-destination warning already is. Add a test asserting a truncated/partial listing is detectable from `--json` output. #### 3. The `--json` stderr workaround is not scoped correctly, and the gap is on the path it was written for `warnRemoteListingFailed` (`:132-144`) correctly routes the listing-failure warning to `v.Stderr` in JSON mode, because `internal/log/log.go:72-78` builds the logger over **`os.Stdout`** in both the TTY and JSON-handler branches, and `internal/log/log.go:62` sets the default level to `slog.LevelWarn`. So `log.Warn` is emitted on stdout by default, with no flags. But two `log.Warn` calls on the very same remote-only path were left unguarded: - `internal/vaultik/snapshot_list.go:229-230` — `log.Warn("Could not describe remote snapshot", ...)` - `internal/vaultik/snapshot_list.go:283-284` — `log.Warn("Remote manifest has an unparseable timestamp", ...)` `vaultik snapshot list --json` with one corrupt manifest or one bad manifest timestamp therefore emits a JSON *log line* onto stdout ahead of the JSON *document*, and `| jq` fails. That is exactly the corruption the `v.Stderr` workaround at `:134` exists to prevent, on exactly the degradation scenario the PR treats as first-class — `TestListSnapshots_UnreadableManifestDoesNotHideOthers` covers it in table mode only, and the tests cannot see it because `log.Initialize(log.Config{})` points the logger at the test process's real stdout rather than `env.stdout`. I accept the `v.Stderr` workaround itself as reasonable given #82, and filing rather than fixing #82 drive-by was the right call. The objection is narrower: within the one function the workaround was added to, the same hazard was left in two places. Half-applied, it is the kind of inconsistency that rots — the next reader sees `log.Warn` used freely here and concludes it is safe. Acceptable: make the JSON-mode discipline uniform on this path — thread the `jsonOutput` flag into `describeRemoteOnlySnapshots` / `remoteSnapshotInfo`, or collect these failures and emit them through the same writer `warnRemoteListingFailed` already chooses. A test that points the logger at a captured buffer and asserts stdout is byte-for-byte a JSON document would pin it. (Noted, not blocking: the pre-existing `log.Warn` calls in `snapshotInfoFromLocal` at `:317`, `:325`, `:330` have the same hazard and are unchanged from `main`. They are #82's problem, not this PR's.) --- ### Verified clean Everything below I checked directly rather than taking from the PR body. **Definition of done.** All eight items satisfied, plus the five extra requirements from the manager comment. **The core property (DoD 1).** `TestListSnapshots_RemoteWithoutSecretKey` (`internal/vaultik/snapshot_list_test.go:211-255`) is a genuine regression guard, not a name. It asserts `Config.AgeSecretKey` is empty up front (so it cannot silently stop testing its premise), asserts exactly one prefix listing was issued, asserts no fetched key contains `.age`, and asserts the remote-only row rendered. Reinstating the `AgeSecretKey == ""` early return would drive `listStreamCalls` to 0 and remove the row — it fails on both counts. The `AgeSecretKey` gate is gone from the code entirely. **Privacy constraint (the one that matters most).** Fully honored. - No writes to remote storage on the listing path: the only storage calls are `ListStream` and `Get` (no `Put` anywhere in `snapshot_list.go`). - The human ID is never fabricated, guessed, or brute-forced. `remoteSnapshotInfo` (`:272-295`) leaves `ID` zero, with a doc comment stating why. `--json` asserts `assert.Empty(t, remoteOnly.ID)` at test line 497. - `internal/snapshot/remotekey.go` and `internal/snapshot/manifest.go` are byte-identical to `main` — `RemoteSnapshotKey` and the manifest format are unchanged. - The manifest is read and never written by this code. The only `snapshot.DecodeManifest` call site in the entire repo is inside `downloadManifestByKey` (`internal/vaultik/snapshot.go:905`). **Concurrency.** No data race. `describeRemoteOnlySnapshots` (`:217-260`) writes only to distinct indices `found[i]` / `ok[i]`, and `go.mod` declares `go 1.26.1` so loop variables are per-iteration. The full suite passes under `-race` (`script/test` sets it). One manifest read cannot abort the listing: the goroutine body returns `nil` unconditionally (`:232`, `:238`) and failures are recorded in `ok`, so `errgroup` never cancels — `TestListSnapshots_UnreadableManifestDoesNotHideOthers` genuinely verifies this (a real non-zstd payload, asserting the good row is present and the bad one is counted). Ordering is deterministic: `unknown` is sorted before both truncation and dispatch (`:196-198`), results are reassembled in index order, and the final sort key is absolute time. The 1000 cap **is** reported in table mode — but see blocking finding 2 for JSON mode. **Degradation (DoD 3).** `ListSnapshots` returns `nil` on remote failure, verified by `TestListSnapshots_UnreachableRemoteDegrades` and `TestListSnapshots_JSONUnreachableRemote`. `markRemotePresence` (`:301-306`) is called only in the `remoteErr == nil` branch, so `RemotePresent` stays a nil `*bool` → `null`, never `false`. The `*bool` type makes "absent" and "unknown" structurally distinguishable. `reportListDrift` is gated on `remoteErr == nil` (`:114-116`), so no drift is claimed from a listing that never happened — asserted directly at test line 396. **Behavior preservation.** I read the 275 deleted lines against the new file. `printSnapshotTable` and `snapshotInfoFromLocal` moved verbatim; the only table change is the identifier cell, which now falls back to `formatRemoteOnlyID` when `!LocallyTracked` (previously it printed `snap.ID` unconditionally, which was always populated because `LocallyTracked` was hardcoded `true`). Locally-tracked rows render identically. `reportRemoteDrift` → `reportListDrift` drops no working case: the local-only warning, the per-ID list, and the remediation hint all survive; the remote-only branch was a bare count and is now real rows. Both `unreadable` and `omitted` are new additions. **PR body claim — `origin/fix/sync-snapshot-cleanup` is redundant.** **True.** Its tip `332ea26` changes exactly one line in `syncWithRemote` (`Repositories.Snapshots.Delete` → `deleteSnapshotFromLocalDB`), and `main` already carries that change at `internal/vaultik/snapshot.go:1186`. Confirmed by reading `git show origin/main:internal/vaultik/snapshot.go`. It shows in the three-dot diff only because the merge base `c24e7e6` predates it. No real fix is being discarded. **PR body claim — single manifest reader.** **True.** `DecodeManifest` has exactly one call site repo-wide (`snapshot.go:905`, inside `downloadManifestByKey`). The remaining `manifest.json.zst` string occurrences are: the upload in `internal/snapshot/snapshot.go:508` (a `Put`, not a read), two key-name filters in listing loops (`prune.go:223`, `snapshot.go:1247`), tests, and doc comments. `verify.go` and `info.go` were genuinely routed through the helper. **`snapshot cleanup` string.** Gone from all Go source; the only remaining occurrences are the TODO entry describing the removal and the test asserting its absence (`snapshot_list_test.go:367`). `CleanupLocalSnapshots` is reachable: `Prune` calls it at `internal/vaultik/prune.go:82` as its first pass, with the comment saying so. Not dead. Renaming rather than re-adding a duplicate entry point was the right call. **Nothing weakened.** `.golangci.yml` hashes to `021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb` as required. `Dockerfile`, `Makefile`, `.gitea/`, `script/` are byte-identical to `main` (empty `git diff`). No deleted tests, no `t.Skip`, no weakened assertions, no file-level `//nolint`. One new `//nolint` (`helpers.go:76`, `tagliatelle`) is per-site, justified, and matches the directive already on that struct on `main`. No new dependency: `golang.org/x/sync` was already a direct require. **Repo policy.** No Claude/Anthropic references in commits, PR body, code, or comments. No attribution trailers (`git log --format='%(trailers)'` empty on both commits). Landing commit title ends with ` (closes #64)`. No scope creep — the `downloadManifestByKey` consolidation is a separate commit and is explicitly requested by the issue's manager comment. Inclusive terminology clean. `make fmt-check` clean. **`TODO.md`.** Updated in the same commit as the work and factually accurate against the diff. Leaving **Next Step** as "Triage the stale remote branches (issue #71)" was **correct** — the workflow says to rotate the step you did, and this was not it. Recording #71 as done would have been a false claim; the entry going to Completed Steps with Next Step untouched is the honest bookkeeping. --- ### Nits (non-blocking) 1. `internal/vaultik/snapshot_list.go:141-143` — in non-JSON mode `warnRemoteListingFailed` calls both `log.Warn` and `v.UI.Warningf` with the same message. Since the logger writes to stdout at default level, the user sees the warning twice. `reportRemoteDrift` on `main` emitted it once. 2. `internal/vaultik/snapshot_list.go:464` — `formatRemoteOnlyID` is computed for every row and discarded for locally-tracked ones. Free to move inside the `if`. 3. `internal/vaultik/snapshot_list.go:98` — `sort.Slice` is not stable. Output is still deterministic here because the input order is, but `sort.SliceStable` would make that property local rather than incidental, which matters once many remote-only rows share the zero timestamp from the unparseable-timestamp fallback at `:286`.

clawbot added needs-rework and removed needs-review labels 2026-08-09 07:04:32 +02:00

clawbot commented

2026-08-09 07:04:48 +02:00

Manager note on the review above. Verdict accepted: FAIL, label set to
needs-rework. All three blocking findings are real and worth fixing
before this lands.

Two things about the review method are worth recording, because they keep
paying off in this repo:

The gate result was a false positive and the reviewer caught it.
script/cibuild returned a literal EXIT=0, but the whole Docker build
resolved from layer cache — RUN make lint and RUN make test both
CACHED, zero stage output. An exit code from a build that ran nothing
is not evidence. The reviewer re-ran uncached and confirmed the tree is
genuinely green. This repo has now produced three separate flavors of
false green (wrong linter version twice, and now a cached build), so
treating a bare exit code as proof is not a safe habit here.

Finding 1 is the kind of bug that survives review. Remote-only rows
render timestamp.UTC() while local rows come from ListRecent, whose
scanner at internal/database/snapshots.go:767 does
time.Unix(startedAtUnix, 0) without .UTC() — unlike its two
siblings at lines 200 and 639, which do. Both paths then print through
the same zone-less format string. On a non-UTC host the same snapshot
shows one time when locally tracked and a different time when
remote-only, in the same column, with nothing to indicate why. Every
fixture is time.UTC and the end-to-end run was evidently on a UTC host,
so nothing caught it. For a backup tool where the timestamp is how a user
identifies which snapshot to restore, a silently wrong time is worse than
a missing one.

Note that snapshots.go:767 is a pre-existing inconsistency this PR
merely surfaces. Fix it at a single normalization point rather than
patching the display layer, so the other two call sites cannot drift back.

Finding 2 is the "no silent caps" rule: table mode reports both the
1000-row cap and the unreadable-manifest count, but --json returns early
before reportListDrift and reports neither. A machine consumer past
1000 remote-only snapshots gets a truncated document with no signal. That
is precisely the consumer least able to notice.

Finding 3 is narrow: the --json stderr workaround is right, and
filing #82 instead of fixing the logger drive-by was the right call, but
two log.Warn calls on the same new path were left unguarded, so a single
corrupt manifest still breaks | jq. The tests cannot see it because
log.Initialize points at the real process stdout rather than the test's
capture buffer — worth fixing so the test can actually observe stdout.

Everything else the reviewer checked came back clean, including both
factual claims I asked it to verify independently rather than trust:
fix/sync-snapshot-cleanup really is already on main (nothing is being
discarded), and downloadManifestByKey really is the only remote-manifest
reader. The privacy constraint holds — no writes on the listing path, the
human ID never fabricated, remotekey.go and manifest.go byte-identical.
Leaving TODO.md's Next Step unrotated was also correct.

Going back to an implementer now; a fresh reviewer will look at the rework.

Manager note on the review above. Verdict accepted: **FAIL**, label set to `needs-rework`. All three blocking findings are real and worth fixing before this lands. Two things about the review method are worth recording, because they keep paying off in this repo: **The gate result was a false positive and the reviewer caught it.** `script/cibuild` returned a literal `EXIT=0`, but the whole Docker build resolved from layer cache — `RUN make lint` and `RUN make test` both `CACHED`, zero stage output. An exit code from a build that ran nothing is not evidence. The reviewer re-ran uncached and confirmed the tree is genuinely green. This repo has now produced three separate flavors of false green (wrong linter version twice, and now a cached build), so treating a bare exit code as proof is not a safe habit here. **Finding 1 is the kind of bug that survives review.** Remote-only rows render `timestamp.UTC()` while local rows come from `ListRecent`, whose scanner at `internal/database/snapshots.go:767` does `time.Unix(startedAtUnix, 0)` **without** `.UTC()` — unlike its two siblings at lines 200 and 639, which do. Both paths then print through the same zone-less format string. On a non-UTC host the same snapshot shows one time when locally tracked and a different time when remote-only, in the same column, with nothing to indicate why. Every fixture is `time.UTC` and the end-to-end run was evidently on a UTC host, so nothing caught it. For a backup tool where the timestamp is how a user identifies which snapshot to restore, a silently wrong time is worse than a missing one. Note that `snapshots.go:767` is a **pre-existing** inconsistency this PR merely surfaces. Fix it at a single normalization point rather than patching the display layer, so the other two call sites cannot drift back. **Finding 2** is the "no silent caps" rule: table mode reports both the 1000-row cap and the unreadable-manifest count, but `--json` returns early before `reportListDrift` and reports neither. A machine consumer past 1000 remote-only snapshots gets a truncated document with no signal. That is precisely the consumer least able to notice. **Finding 3** is narrow: the `--json` stderr workaround is right, and filing #82 instead of fixing the logger drive-by was the right call, but two `log.Warn` calls on the same new path were left unguarded, so a single corrupt manifest still breaks `| jq`. The tests cannot see it because `log.Initialize` points at the real process stdout rather than the test's capture buffer — worth fixing so the test can actually observe stdout. Everything else the reviewer checked came back clean, including both factual claims I asked it to verify independently rather than trust: `fix/sync-snapshot-cleanup` really is already on `main` (nothing is being discarded), and `downloadManifestByKey` really is the only remote-manifest reader. The privacy constraint holds — no writes on the listing path, the human ID never fabricated, `remotekey.go` and `manifest.go` byte-identical. Leaving `TODO.md`'s Next Step unrotated was also correct. Going back to an implementer now; a fresh reviewer will look at the rework.

clawbot added 1 commit 2026-08-09 07:21:48 +02:00

Fix timezone drift and --json truncation in snapshot list (closes #64 )

check / check (pull_request) Successful in 2m24s

Details

9a45221b79

Three defects in the merged listing path, all found in review.

1. The TIMESTAMP column mixed two timezones. Remote-only rows render
   timestamp.UTC(); local rows come from ListRecent, whose scanner
   decoded Unix seconds in the host's local zone, unlike the two other
   snapshot scanners in that file. Both render through the same
   zone-less format string, so on a non-UTC host the same snapshot
   showed one time when locally tracked and another when remote-only,
   in the same column, with nothing to indicate why.

   Normalized at the point the timestamp enters the domain rather than
   at the display layer: scanSnapshotRows now decodes in UTC like its
   siblings, and GetIncompleteByHostname's copy of that loop was folded
   onto the shared scanner so the three call sites cannot drift apart
   again.

2. --json silently truncated. The early return skipped reportListDrift,
   so neither the 1000-row cap nor the unreadable-manifest count reached
   a machine consumer: past the cap the document was short with no
   signal at all. Both counts now go to stderr, where the
   unreachable-destination warning already goes. The document's shape is
   deliberately unchanged, so existing consumers keep parsing.

3. The --json stderr workaround was half-applied. Two per-snapshot
   log.Warn calls on the same new path were left unguarded, and the
   logger writes to stdout at default level, so one corrupt manifest put
   a log line ahead of the document and broke `| jq`. Both now route
   through the same JSON-aware writer. They are also collected during
   the concurrent manifest reads and emitted afterwards in key order,
   since that writer is not safe for concurrent use. Still a local
   workaround; the logger itself is issue #82.

Also from review, non-blocking: the destination-listing failure is no
longer printed twice in table mode, the identifier cell is no longer
computed and discarded for locally tracked rows, and the merge sort is
stable so that rows sharing the zero-timestamp fallback keep a
deterministic order.

Tests: each fix has a test that fails without it. The timezone tests
pin time.Local to a non-UTC zone and assert Location identity, so they
would have caught this on the UTC host where it was missed. The JSON
stdout test redirects the process's own stdout to a pipe and rebuilds
the logger over it, so it can actually observe a log line landing on
stdout ahead of the document rather than asserting on an injected
buffer the log never reaches.

clawbot commented

2026-08-09 07:23:55 +02:00

Rework pushed as 9a45221. All three blocking findings are addressed,
each with a test I verified fails without its fix. No finding is
silently skipped and none is rebutted — all three were correct. The PR
body has a matching "Review rework" section.

Gate — forced uncached, both ways

The review was right that the previous EXIT=0 proved nothing: that
build resolved entirely from layer cache. This time:

1. GOFLAGS=-count=1 make check → literal EXIT=0.
-count=1 disables the test result cache, so every package genuinely
ran. 14 ok package lines, none marked (cached); the lint stage
printed 0 issues.; fmt-check clean.

2. BUILDKIT_PROGRESS=plain script/cibuild → literal EXIT=0.
The source COPY layer was invalidated by the new commit, so neither
gate stage could be cached, and the plain progress output shows both
actually executing:

#16 [lint 8/8] RUN make lint
#16 52.95 0 issues.
#16 DONE 57.7s
...
#23 [builder 8/9] RUN make test
#23 68.32 ok  	sneak.berlin/go/vaultik/internal/vaultik	7.456s
#23 DONE 69.1s

Every ok line in that stage carries a real duration; none is
(cached). The only warning is the pre-existing gomodguard
deprecation notice from the #61 rollout.

.golangci.yml is unchanged and still hashes to
021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb.
git diff origin/main over Dockerfile, Makefile, .gitea/ and
script/ is empty.

Finding 1 — mixed timezones in the TIMESTAMP column

Fixed at the scanner, not the display layer, as directed.
scanSnapshotRows (internal/database/snapshots.go) now decodes
started_at / completed_at with .UTC(), matching the two sibling
scanners it had drifted from, with a comment stating why the zone is a
decode choice here. Nothing in snapshot_list.go was patched to
compensate.

While doing it I also folded GetIncompleteByHostname onto
scanSnapshotRows: it selects the identical column set and carried a
verbatim copy of the loop. That was forced by dupl, which correctly
began firing the moment the two loops became token-identical — and it
is the right outcome anyway, since it removes the last place the
normalization could drift back out of sync. Three readers, one scanner.

Tests. Two, at both levels:

TestSnapshotTimestampsDecodeAsUTC (internal/database/snapshots_test.go)
exercises GetByID, ListRecent, GetIncompleteSnapshots and
GetIncompleteByHostname, and compares *time.Location pointers
rather than offsets. That makes it host-independent in the strongest
sense: time.Unix returns time.Local, which is never the same
Location value as time.UTC even on a host whose offset is zero.
It would have failed on the UTC machine where this was missed.
TestListSnapshots_TimestampsAreUTCOnNonUTCHost pins time.Local to
a fixed +07:13 zone via a documented helper (restored in
t.Cleanup, and the test is deliberately non-parallel — Go runs
non-parallel tests to completion before resuming any parallel one, so
the mutation is not observable elsewhere). It builds a locally tracked
row and a remote-only row from the same instant and asserts they
render the same wall clock in the table and the same string in
--json.

Reverting the .UTC() makes both fail, and the second one prints the
bug verbatim:

"testhost_home_2026-03-01T10:00:00Z   2026-03-01 17:13:00   ..."
does not contain "2026-03-01 10:00:00"

Finding 2 — `--json` silently truncating

Took the stderr route, as recommended. The JSON shape is unchanged,
so this is not a breaking change for consumers. New
reportJSONListingLimits runs before the encoder (so the warnings
survive even if encoding fails) and writes to v.Stderr, the same
stream the unreachable-destination warning already uses:

Warning: N remote snapshot(s) could not be described: manifest missing
or unreadable. They are missing from this listing.
Warning: listing truncated: N further remote-only snapshot(s) not shown
(limit 1000 per listing).

Gated on remoteErr == nil, exactly like the table-mode path, so
nothing is claimed from a listing that never happened.

Tests. TestListSnapshots_JSONReportsTruncation builds 1001
remote-only snapshots and asserts the document has exactly 1000 rows
and that the truncation is announced — that is the case the review
called out as invisible. TestListSnapshots_JSONReportsUnreadableManifests
covers the corrupt-manifest count. Both fail without the fix ("" does not contain "listing truncated"). The 1001-manifest test costs about
4s; the suite still finishes well inside the 30s per-package timeout.

Finding 3 — half-applied `--json` stderr workaround

Both stragglers now route through one JSON-aware writer,
warnWhileListing, which picks v.Stderr in --json mode and
log.Warn otherwise. Its doc comment states the reason and says to
delete it, along with the branch in warnRemoteListingFailed, once #82
lands. The logger itself is untouched — #82 remains out of scope.

One structural change was needed to do this safely: those two warnings
fire inside the errgroup workers, and v.Stderr is not safe for
concurrent writes (a bytes.Buffer in tests, and -race would have
caught it). So each worker now records at most one listingWarning in
its own slot and the warnings are emitted after group.Wait() in key
order, from a single goroutine. That also makes warning order
deterministic run to run. remoteSnapshotInfo returns its warning
instead of logging it, for the same reason.

Test. TestListSnapshots_JSONStdoutIsOnlyTheDocument does what the
review asked for: it redirects the process's own os.Stdout to a
pipe, rebuilds the logger over it, and points the JSON encoder and the
UI writer at the same pipe. That is precisely what snapshot list --json | jq sees, and it is the only way a test can observe this — as
the review noted, log.Initialize binds to os.Stdout, not to any
writer the test can inject. The fixture has both a manifest that is not
a zstd stream and a manifest whose timestamp will not parse, so both
call sites are covered.

With the fix reverted it captures the defect exactly — two log lines on
stdout ahead of the array, and the parse fails:

stdout must be parseable JSON:
"{\"time\":...,\"level\":\"WARN\",\"msg\":\"Could not describe remote
snapshot\",...}\n{\"time\":...,\"msg\":\"Remote manifest has an
unparseable timestamp\",...}\n[\n  {\n    \"id\": \"\", ...

It also asserts the good row and the bad-timestamp row are both present
and the corrupt one is absent, so the degradation behavior is pinned
alongside the parseability.

Optional nits — all three taken

Duplicate warning (:141-143): the log.Warn is gone;
UI.Warningf alone. Checked before removing it that this loses
nothing in --cron / --quiet: that path is UI.SetQuiet(true),
and quiet mode silences everything except Warning and Error. (The
Vaultik.UI doc comment claiming cron swaps in a discarding writer
is stale — the mechanism is SetQuiet. Not touched here.)
formatRemoteOnlyID computed and discarded (:464): the row
cells are now filled in an if/else, so neither branch's formatting
runs for the other.
sort.Slice → sort.SliceStable (:98), with a comment noting
the zero-timestamp fallback makes ties realistic.

Scope

Nothing outside the three findings and the three nits. No existing test
weakened or deleted — the only edits to existing tests are one added
field on the --json row struct and splitting addRemote so a manifest
can carry a raw timestamp string; every prior assertion is intact. Two
new //nolint directives, both per-site and justified: nilerr on the
deliberate return nil in the errgroup worker (returning the error
would cancel the group and hide every other snapshot), and
gosmopolitan on the time.Local helper, where pinning a non-UTC zone
is the entire point. Nothing filed as a new issue; nothing discovered
that warranted one.

TODO.md's Completed Steps entry for #64 now describes the rework;
Next Step is still unrotated, per the review.

Rework pushed as `9a45221`. All three blocking findings are addressed, each with a test I verified fails without its fix. No finding is silently skipped and none is rebutted — all three were correct. The PR body has a matching "Review rework" section. ## Gate — forced uncached, both ways The review was right that the previous `EXIT=0` proved nothing: that build resolved entirely from layer cache. This time: **1. `GOFLAGS=-count=1 make check` → literal `EXIT=0`.** `-count=1` disables the test result cache, so every package genuinely ran. 14 `ok` package lines, **none** marked `(cached)`; the lint stage printed `0 issues.`; `fmt-check` clean. **2. `BUILDKIT_PROGRESS=plain script/cibuild` → literal `EXIT=0`.** The source `COPY` layer was invalidated by the new commit, so neither gate stage could be cached, and the plain progress output shows both actually executing: ``` #16 [lint 8/8] RUN make lint #16 52.95 0 issues. #16 DONE 57.7s ... #23 [builder 8/9] RUN make test #23 68.32 ok sneak.berlin/go/vaultik/internal/vaultik 7.456s #23 DONE 69.1s ``` Every `ok` line in that stage carries a real duration; none is `(cached)`. The only warning is the pre-existing `gomodguard` deprecation notice from the #61 rollout. `.golangci.yml` is unchanged and still hashes to `021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb`. `git diff origin/main` over `Dockerfile`, `Makefile`, `.gitea/` and `script/` is empty. ## Finding 1 — mixed timezones in the TIMESTAMP column **Fixed at the scanner, not the display layer**, as directed. `scanSnapshotRows` (`internal/database/snapshots.go`) now decodes `started_at` / `completed_at` with `.UTC()`, matching the two sibling scanners it had drifted from, with a comment stating why the zone is a decode choice here. Nothing in `snapshot_list.go` was patched to compensate. While doing it I also folded `GetIncompleteByHostname` onto `scanSnapshotRows`: it selects the identical column set and carried a verbatim copy of the loop. That was forced by `dupl`, which correctly began firing the moment the two loops became token-identical — and it is the right outcome anyway, since it removes the last place the normalization could drift back out of sync. Three readers, one scanner. **Tests.** Two, at both levels: - `TestSnapshotTimestampsDecodeAsUTC` (`internal/database/snapshots_test.go`) exercises `GetByID`, `ListRecent`, `GetIncompleteSnapshots` and `GetIncompleteByHostname`, and compares `*time.Location` **pointers** rather than offsets. That makes it host-independent in the strongest sense: `time.Unix` returns `time.Local`, which is never the same `Location` value as `time.UTC` even on a host whose offset is zero. It would have failed on the UTC machine where this was missed. - `TestListSnapshots_TimestampsAreUTCOnNonUTCHost` pins `time.Local` to a fixed `+07:13` zone via a documented helper (restored in `t.Cleanup`, and the test is deliberately non-parallel — Go runs non-parallel tests to completion before resuming any parallel one, so the mutation is not observable elsewhere). It builds a locally tracked row and a remote-only row from the same instant and asserts they render the same wall clock in the table and the same string in `--json`. Reverting the `.UTC()` makes both fail, and the second one prints the bug verbatim: ``` "testhost_home_2026-03-01T10:00:00Z 2026-03-01 17:13:00 ..." does not contain "2026-03-01 10:00:00" ``` ## Finding 2 — `--json` silently truncating Took the stderr route, as recommended. **The JSON shape is unchanged**, so this is not a breaking change for consumers. New `reportJSONListingLimits` runs before the encoder (so the warnings survive even if encoding fails) and writes to `v.Stderr`, the same stream the unreachable-destination warning already uses: ``` Warning: N remote snapshot(s) could not be described: manifest missing or unreadable. They are missing from this listing. Warning: listing truncated: N further remote-only snapshot(s) not shown (limit 1000 per listing). ``` Gated on `remoteErr == nil`, exactly like the table-mode path, so nothing is claimed from a listing that never happened. **Tests.** `TestListSnapshots_JSONReportsTruncation` builds 1001 remote-only snapshots and asserts the document has exactly 1000 rows **and** that the truncation is announced — that is the case the review called out as invisible. `TestListSnapshots_JSONReportsUnreadableManifests` covers the corrupt-manifest count. Both fail without the fix (`"" does not contain "listing truncated"`). The 1001-manifest test costs about 4s; the suite still finishes well inside the 30s per-package timeout. ## Finding 3 — half-applied `--json` stderr workaround Both stragglers now route through one JSON-aware writer, `warnWhileListing`, which picks `v.Stderr` in `--json` mode and `log.Warn` otherwise. Its doc comment states the reason and says to delete it, along with the branch in `warnRemoteListingFailed`, once #82 lands. **The logger itself is untouched** — #82 remains out of scope. One structural change was needed to do this safely: those two warnings fire inside the `errgroup` workers, and `v.Stderr` is not safe for concurrent writes (a `bytes.Buffer` in tests, and `-race` would have caught it). So each worker now records at most one `listingWarning` in its own slot and the warnings are emitted after `group.Wait()` in key order, from a single goroutine. That also makes warning order deterministic run to run. `remoteSnapshotInfo` returns its warning instead of logging it, for the same reason. **Test.** `TestListSnapshots_JSONStdoutIsOnlyTheDocument` does what the review asked for: it redirects the **process's own** `os.Stdout` to a pipe, rebuilds the logger over it, and points the JSON encoder and the UI writer at the same pipe. That is precisely what `snapshot list --json | jq` sees, and it is the only way a test can observe this — as the review noted, `log.Initialize` binds to `os.Stdout`, not to any writer the test can inject. The fixture has both a manifest that is not a zstd stream and a manifest whose timestamp will not parse, so both call sites are covered. With the fix reverted it captures the defect exactly — two log lines on stdout ahead of the array, and the parse fails: ``` stdout must be parseable JSON: "{\"time\":...,\"level\":\"WARN\",\"msg\":\"Could not describe remote snapshot\",...}\n{\"time\":...,\"msg\":\"Remote manifest has an unparseable timestamp\",...}\n[\n {\n \"id\": \"\", ... ``` It also asserts the good row and the bad-timestamp row are both present and the corrupt one is absent, so the degradation behavior is pinned alongside the parseability. ## Optional nits — all three taken - **Duplicate warning** (`:141-143`): the `log.Warn` is gone; `UI.Warningf` alone. Checked before removing it that this loses nothing in `--cron` / `--quiet`: that path is `UI.SetQuiet(true)`, and quiet mode silences everything **except** Warning and Error. (The `Vaultik.UI` doc comment claiming cron swaps in a discarding writer is stale — the mechanism is `SetQuiet`. Not touched here.) - **`formatRemoteOnlyID` computed and discarded** (`:464`): the row cells are now filled in an `if`/`else`, so neither branch's formatting runs for the other. - **`sort.Slice` → `sort.SliceStable`** (`:98`), with a comment noting the zero-timestamp fallback makes ties realistic. ## Scope Nothing outside the three findings and the three nits. No existing test weakened or deleted — the only edits to existing tests are one added field on the `--json` row struct and splitting `addRemote` so a manifest can carry a raw timestamp string; every prior assertion is intact. Two new `//nolint` directives, both per-site and justified: `nilerr` on the deliberate `return nil` in the errgroup worker (returning the error would cancel the group and hide every other snapshot), and `gosmopolitan` on the `time.Local` helper, where pinning a non-UTC zone is the entire point. Nothing filed as a new issue; nothing discovered that warranted one. `TODO.md`'s Completed Steps entry for #64 now describes the rework; **Next Step is still unrotated**, per the review.

clawbot added needs-review and removed needs-rework labels 2026-08-09 07:24:34 +02:00

clawbot referenced this pull request

2026-08-09 07:25:17 +02:00

Vaultik.UI doc comment misdescribes how --cron suppresses output #84

clawbot commented

2026-08-09 07:33:24 +02:00

Re-review: PR #83 — PASS (`merge-ready`)

Fresh reviewer, independent of the earlier review. Head 9a45221, base
main af607e3. git merge-base HEAD origin/main is af607e3, which
is the current tip of main, so the branch is a fast-forward with no
conflicts. CI on the head commit: check / check (pull_request),
success, 2m24s.

I re-derived every finding below rather than carrying anything forward
from the prior review, and I re-checked the items it had marked clean,
since a new commit landed after it.

Gate — the first result was another false green, as warned

BUILDKIT_PROGRESS=plain script/cibuild → EXIT=0. Worthless. Every
stage resolved from layer cache, including the two that matter:

#22 [builder 8/9] RUN make test
#22 CACHED
#25 [lint 8/8] RUN make lint
#25 CACHED

Zero stage output, so neither the linter nor the suite executed. That is
the fourth false green this repo has produced.

Forced uncached through the repo entrypoints — GOFLAGS=-count=1 make check, i.e. script/check → script/test (go test -race -timeout 30s ./...), script/lint (digest-pinned image from the Dockerfile
lint stage), script/fmt-check:

14 ok package lines, none marked (cached). Longest:
internal/database 7.773s, internal/vaultik 7.988s.
Lint: 0 issues.
fmt-check: clean.
EXIT=0

So the tree is genuinely green, under -race, at the pinned linter
version. The gomodguard deprecation warning is pre-existing from the
#61 rollout and is not this PR's.

The three blocking findings — all genuinely fixed, and the tests genuinely catch regressions

I did not take the author's "fails without the fix" claim on trust. I
reverted each fix in a scratch worktree and ran the suite through
make test, then restored (git status --porcelain empty afterwards).

1. Timezone normalization — fixed at the scanner, correctly.

internal/database/snapshots.go:733-745 — scanSnapshotRows now decodes
both started_at and completed_at with .UTC(), with a comment
explaining that the column is a bare Unix second so the zone is a decode
choice. Nothing in snapshot_list.go was patched to compensate, which is
the right layer.

All three read paths are genuinely normalized. GetByID (:200-204)
already did .UTC() on both fields; ListRecent (:234),
GetIncompleteSnapshots (:585) and GetIncompleteByHostname (:614)
now all return r.scanSnapshotRows(rows).

The fold of GetIncompleteByHostname onto the shared scanner changed
nothing beyond removing the duplicate:

Column order: its SELECT list is byte-identical to
GetIncompleteSnapshots' and to what scanSnapshotRows scans
(id, hostname, vaultik_version, vaultik_git_revision, started_at, completed_at, file_count, chunk_count, blob_count, total_size, blob_size, compression_ratio). Verified field by field.
NULL handling: identical — completedAtUnix *int64, nil check
preserved.
Error paths: identical — same "scanning snapshot: %w" wrap, same
trailing rows.Err(), and the caller's defer rows.Close() with
Fatalf is untouched.
The deleted inline loop already had .UTC() on both fields, so this
call site's behavior is bit-for-bit unchanged.

Revert check: with .UTC() removed from scanSnapshotRows,
TestSnapshotTimestampsDecodeAsUTC FAILs and
TestListSnapshots_TimestampsAreUTCOnNonUTCHost FAILs. Note this
review host is itself UTC — the database test still failed, because it
compares *time.Location pointers rather than offsets, exactly as
claimed. That is the property that makes it host-independent.

2. --json truncation reporting — correct, and the shape really is unchanged.

reportJSONListingLimits (internal/vaultik/snapshot_list.go:168-182)
writes both counts to v.Stderr, and is called at :107-109 gated on
remoteErr == nil — so no nil deref (collectRemoteSnapshots returns a
nil listing on error) and no claim made from a listing that never
happened. It runs before encoder.Encode, so the notices survive an
encoding failure.

JSON shape: the rework commit 9a45221 touches no struct tag and adds
no field. git diff 0e2929d 9a45221 -- internal/vaultik/helpers.go is
empty. Stdout stays a bare array. Confirmed parseable by
decodeListJSON, which json.Unmarshals the whole of stdout and fails
loudly on any prefix.

Revert check: with the reportJSONListingLimits call removed,
TestListSnapshots_JSONReportsTruncation FAILs,
TestListSnapshots_JSONReportsUnreadableManifests FAILs, and
TestListSnapshots_JSONStdoutIsOnlyTheDocument FAILs.

3. The concurrent warning collection — I looked hard at this and it is correct.

describeRemoteOnlySnapshots (:308-365):

Slots are disjoint. found, ok and warnings are all
pre-allocated to len(keys); each worker writes only found[i],
warnings[i], ok[i]. No append, no shared map, no resize.
i and key are per-iteration — go.mod declares go 1.26.1,
well past the 1.22 loop-variable change.
Happens-before is established by group.Wait() at :345 before
any slot is read at :350-362.
No warning is dropped. Both branches assign warnings[i]: the
error branch builds its own, the success branch stores whatever
remoteSnapshotInfo returned (possibly nil). The emit loop is over
for i := range keys and runs before the !ok[i] continue, so
a warning belonging to an unreadable key is still emitted.
No warning is duplicated — exactly one emit per index, and
remoteSnapshotInfo no longer logs on its own (:397-404 returns the
warning instead).
Ordering is deterministic. unknown is sort.Strings-sorted at
:277 before both the truncation cut and dispatch, and emission is in
index order from the single calling goroutine.
The suite passes under -race (script/test sets -race), which is
what would have caught concurrent bytes.Buffer writes had the
structural change not been made.

TestListSnapshots_JSONStdoutIsOnlyTheDocument does redirect the
process's stdout, not the env buffer: captureProcessStdout sets
os.Stdout = writer and then calls log.Initialize(log.Config{}).
internal/log/log.go:73-78 reads os.Stdout at call time in both the
TTY and JSON-handler branches, so the rebuilt logger genuinely writes to
the pipe. The encoder and the UI are then pointed at the same pipe. So
the assertion is against what snapshot list --json | jq actually sees.

Revert check: forcing warnWhileListing to always call log.Warn
makes that test — and only that test — FAIL.

The five `//nolint` directives

All five are per-site, none file-level, and each justification is
accurate:

internal/vaultik/snapshot_list.go:332 //nolint:nilerr — correct,
not a swallow. The error is not discarded: it is recorded in
warnings[i] (emitted verbatim, key and error, at :352) and ok[i]
stays false, which increments listing.unreadable, which is reported
in table mode (reportListDrift, :503-506) and in --json mode
(reportJSONListingLimits, :169-174). Returning it would cancel the
errgroup and let one corrupt manifest hide every other snapshot —
which TestListSnapshots_UnreadableManifestDoesNotHideOthers pins.
internal/vaultik/helpers.go:76 //nolint:tagliatelle — pre-existing
on main, unchanged.
internal/vaultik/snapshot_list_test.go:444 //nolint:tagliatelle —
the test's mirror of the wire format; asserting on snake_case is the
point.
snapshot_list_test.go:564 //nolint:gosmopolitan — pinning
time.Local is literally the mechanism under test.
snapshot_list_test.go:586 and :756, both //nolint:paralleltest —
both accurate. One mutates
time.Local, the other os.Stdout plus the global logger. Go runs
non-parallel top-level tests to completion before resuming paused
parallel ones, so neither mutation is observable elsewhere; -race
agrees.

Previously-clean items, re-verified at the new head

No-private-key guard. TestListSnapshots_RemoteWithoutSecretKey
asserts Config.AgeSecretKey is empty up front, asserts exactly one
prefix listing, asserts no fetched key contains .age, asserts the
remote-only row renders and that neither otherhost nor media
appears in the output. Reinstating an AgeSecretKey == "" gate breaks
it on several counts. No such gate exists anywhere in the code.
Privacy constraint. internal/snapshot/remotekey.go and
internal/snapshot/manifest.go are byte-identical to main (empty
git diff). No Put, Delete or upload appears anywhere in
snapshot_list.go — the listing path is read-only. The human ID is
left zero for remote-only rows with a doc comment saying why, and
TestListSnapshots_JSONMergedView asserts Empty(remoteOnly.ID).
errgroup correctness — covered above.
RemotePresent *bool. markRemotePresence is called only in the
remoteErr == nil branch (:93-96), so on an unreachable destination
it stays nil and serializes as null
(json:"remote_present", no omitempty).
TestListSnapshots_JSONUnreachableRemote asserts Nil;
TestListSnapshots_JSONMergedView asserts a real false for a
local-only row. "Absent" and "unknown" stay distinguishable.
Single remote-manifest reader. snapshot.DecodeManifest has
exactly one non-test call site repo-wide:
internal/vaultik/snapshot.go:905, inside downloadManifestByKey.
The remaining manifest.json.zst occurrences are the upload in
internal/snapshot/snapshot.go:508, two key-name filters
(prune.go:223, snapshot.go:1247), and doc comments. verify.go
and info.go are genuinely routed through the helper, and
info.go's path string was previously built identically, so behavior
is preserved.

The optional nits from the prior review

All three taken, and the reasoning behind the first one checks out:

The duplicate log.Warn in warnRemoteListingFailed is gone.
Nothing is lost in --cron/--quiet. Vaultik.UI is always
ui.New(os.Stdout) (internal/vaultik/vaultik.go:109) — there is no
discarding-writer swap anywhere — and quiet mode is SetQuiet(true),
which gates Beginf/Completef/Infof/Noticef/Detailf/Progressf/Bannerf
but not Warningf or Errorf (internal/ui/ui.go:69-207). The
warning still reaches a cron mailbox; only the Infof follow-up line
is suppressed, as it should be.
formatRemoteOnlyID is now inside the else branch (:584-594).
sort.SliceStable at :102 with a comment naming the zero-timestamp
tie source.

Nothing weakened

.golangci.yml → 021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb.
git diff origin/main over Dockerfile, Makefile, .gitea/,
script/ is empty.
git diff origin/main --stat -- '*_test.go' is insertions only
(+113, +801, 0 deletions). grep '^-func Test' over the test diff is
empty — no test removed. No new t.Skip (the three in the tree are
pre-existing and untouched). No weakened assertion. No file-level
//nolint.

Repo policy

No Claude/Anthropic reference in any commit message, commit body, code,
comment, doc, or the PR body. No attribution trailers —
git log --format='%(trailers)' is empty on all three commits.
Tip commit title: Fix timezone drift and --json truncation in snapshot list (closes #64). Ends with (closes #64).
Inclusive terminology: clean.
make fmt-check: clean.
No scope creep. The downloadManifestByKey consolidation is its own
commit and is explicitly requested by the issue's manager comment
("read the manifest through a single helper"). Issue #82's logger fix
is correctly left alone; the workaround carries a comment naming #82 as
its removal condition.
TODO.md is updated in the same commit as the work and is factually
accurate against the diff, including the rework paragraph. Next Step
left unrotated is still correct: the workflow rotates the step you
did, and this was issue #64, not #71. Rotating would have recorded
#71 as done when it is not.
Definition of done: all eight items satisfied, plus the five extra
requirements from the manager comment on #64.

Nits (non-blocking, no action required to merge)

internal/vaultik/snapshot_list_test.go — captureProcessStdout has
no defer restoring os.Stdout. The one require.NoError inside the
fn closure would runtime.Goexit() past the restore and past
writer.Close(), leaving the package's os.Stdout pointing at an
unread pipe and leaking the drain goroutine — turning a clean failure
into a probable 30s package timeout. Only reachable when the test is
already failing, so it costs diagnosability rather than correctness.
defer func() { os.Stdout = previous }() would close it.
internal/vaultik/snapshot_list.go:186 — kvPairSize is a
package-scoped const in internal/vaultik named for a general
structured-logging concern but declared mid-file between two listing
functions. A local const inside warnWhileListing, or a name tied
to that function, would keep the package namespace tidier.
internal/vaultik/snapshot_list.go:277-282 — truncation cuts
unknown in remote-key hex order, so the 1000 rows shown are
arbitrary with respect to time rather than the newest 1000. This is
inherent (timestamps require the very manifest reads being capped) and
the omission is reported, so it is fine — but the cap is not mentioned
in the README's snapshot list section, which is otherwise thorough.
Both 9a45221 and 0e2929d end with (closes #64). The repo's
default merge style is squash, so exactly one commit lands and the
duplicate is harmless. Worth avoiding on future reworks only because
it would double-close if the branch were ever merged unsquashed.

Verdict

PASS. The three blocking findings are fixed at the right layer, each
is pinned by a test I confirmed fails when the fix is reverted, the new
concurrent code is race-free with disjoint slots and deterministic
ordering, nothing was weakened, and the tree is green under an uncached
run at the pinned linter version. Label merge-ready, assign sneak.

## Re-review: PR #83 — PASS (`merge-ready`) Fresh reviewer, independent of the earlier review. Head `9a45221`, base `main` `af607e3`. `git merge-base HEAD origin/main` is `af607e3`, which is the current tip of `main`, so the branch is a fast-forward with no conflicts. CI on the head commit: `check / check (pull_request)`, **success**, 2m24s. I re-derived every finding below rather than carrying anything forward from the prior review, and I re-checked the items it had marked clean, since a new commit landed after it. --- ### Gate — the first result was another false green, as warned `BUILDKIT_PROGRESS=plain script/cibuild` → `EXIT=0`. **Worthless.** Every stage resolved from layer cache, including the two that matter: ``` #22 [builder 8/9] RUN make test #22 CACHED #25 [lint 8/8] RUN make lint #25 CACHED ``` Zero stage output, so neither the linter nor the suite executed. That is the fourth false green this repo has produced. Forced uncached through the repo entrypoints — `GOFLAGS=-count=1 make check`, i.e. `script/check` → `script/test` (`go test -race -timeout 30s ./...`), `script/lint` (digest-pinned image from the `Dockerfile` lint stage), `script/fmt-check`: - **14 `ok` package lines, none marked `(cached)`.** Longest: `internal/database` 7.773s, `internal/vaultik` 7.988s. - Lint: **`0 issues.`** - `fmt-check`: clean. - **`EXIT=0`** So the tree is genuinely green, under `-race`, at the pinned linter version. The `gomodguard` deprecation warning is pre-existing from the #61 rollout and is not this PR's. --- ### The three blocking findings — all genuinely fixed, and the tests genuinely catch regressions I did not take the author's "fails without the fix" claim on trust. I reverted each fix in a scratch worktree and ran the suite through `make test`, then restored (`git status --porcelain` empty afterwards). **1. Timezone normalization — fixed at the scanner, correctly.** `internal/database/snapshots.go:733-745` — `scanSnapshotRows` now decodes both `started_at` and `completed_at` with `.UTC()`, with a comment explaining that the column is a bare Unix second so the zone is a decode choice. Nothing in `snapshot_list.go` was patched to compensate, which is the right layer. All three read paths are genuinely normalized. `GetByID` (`:200-204`) already did `.UTC()` on both fields; `ListRecent` (`:234`), `GetIncompleteSnapshots` (`:585`) and `GetIncompleteByHostname` (`:614`) now all return `r.scanSnapshotRows(rows)`. The fold of `GetIncompleteByHostname` onto the shared scanner changed **nothing** beyond removing the duplicate: - Column order: its `SELECT` list is byte-identical to `GetIncompleteSnapshots`' and to what `scanSnapshotRows` scans (`id, hostname, vaultik_version, vaultik_git_revision, started_at, completed_at, file_count, chunk_count, blob_count, total_size, blob_size, compression_ratio`). Verified field by field. - NULL handling: identical — `completedAtUnix *int64`, nil check preserved. - Error paths: identical — same `"scanning snapshot: %w"` wrap, same trailing `rows.Err()`, and the caller's `defer rows.Close()` with `Fatalf` is untouched. - The deleted inline loop already had `.UTC()` on both fields, so this call site's behavior is bit-for-bit unchanged. Revert check: with `.UTC()` removed from `scanSnapshotRows`, `TestSnapshotTimestampsDecodeAsUTC` **FAILs** and `TestListSnapshots_TimestampsAreUTCOnNonUTCHost` **FAILs**. Note this review host is itself UTC — the database test still failed, because it compares `*time.Location` pointers rather than offsets, exactly as claimed. That is the property that makes it host-independent. **2. `--json` truncation reporting — correct, and the shape really is unchanged.** `reportJSONListingLimits` (`internal/vaultik/snapshot_list.go:168-182`) writes both counts to `v.Stderr`, and is called at `:107-109` gated on `remoteErr == nil` — so no nil deref (`collectRemoteSnapshots` returns a nil `listing` on error) and no claim made from a listing that never happened. It runs **before** `encoder.Encode`, so the notices survive an encoding failure. JSON shape: the rework commit `9a45221` touches no struct tag and adds no field. `git diff 0e2929d 9a45221 -- internal/vaultik/helpers.go` is empty. Stdout stays a bare array. Confirmed parseable by `decodeListJSON`, which `json.Unmarshal`s the whole of stdout and fails loudly on any prefix. Revert check: with the `reportJSONListingLimits` call removed, `TestListSnapshots_JSONReportsTruncation` **FAILs**, `TestListSnapshots_JSONReportsUnreadableManifests` **FAILs**, and `TestListSnapshots_JSONStdoutIsOnlyTheDocument` **FAILs**. **3. The concurrent warning collection — I looked hard at this and it is correct.** `describeRemoteOnlySnapshots` (`:308-365`): - **Slots are disjoint.** `found`, `ok` and `warnings` are all pre-allocated to `len(keys)`; each worker writes only `found[i]`, `warnings[i]`, `ok[i]`. No append, no shared map, no resize. - **`i` and `key` are per-iteration** — `go.mod` declares `go 1.26.1`, well past the 1.22 loop-variable change. - **Happens-before is established** by `group.Wait()` at `:345` before any slot is read at `:350-362`. - **No warning is dropped.** Both branches assign `warnings[i]`: the error branch builds its own, the success branch stores whatever `remoteSnapshotInfo` returned (possibly nil). The emit loop is over `for i := range keys` and runs **before** the `!ok[i]` `continue`, so a warning belonging to an unreadable key is still emitted. - **No warning is duplicated** — exactly one emit per index, and `remoteSnapshotInfo` no longer logs on its own (`:397-404` returns the warning instead). - **Ordering is deterministic.** `unknown` is `sort.Strings`-sorted at `:277` before both the truncation cut and dispatch, and emission is in index order from the single calling goroutine. - The suite passes under `-race` (`script/test` sets `-race`), which is what would have caught concurrent `bytes.Buffer` writes had the structural change not been made. `TestListSnapshots_JSONStdoutIsOnlyTheDocument` does redirect the **process's** stdout, not the env buffer: `captureProcessStdout` sets `os.Stdout = writer` and then calls `log.Initialize(log.Config{})`. `internal/log/log.go:73-78` reads `os.Stdout` at call time in both the TTY and JSON-handler branches, so the rebuilt logger genuinely writes to the pipe. The encoder and the UI are then pointed at the same pipe. So the assertion is against what `snapshot list --json | jq` actually sees. Revert check: forcing `warnWhileListing` to always call `log.Warn` makes that test — and only that test — **FAIL**. --- ### The five `//nolint` directives All five are per-site, none file-level, and each justification is accurate: - `internal/vaultik/snapshot_list.go:332` `//nolint:nilerr` — **correct, not a swallow.** The error is not discarded: it is recorded in `warnings[i]` (emitted verbatim, key and error, at `:352`) and `ok[i]` stays false, which increments `listing.unreadable`, which is reported in table mode (`reportListDrift`, `:503-506`) and in `--json` mode (`reportJSONListingLimits`, `:169-174`). Returning it would cancel the errgroup and let one corrupt manifest hide every other snapshot — which `TestListSnapshots_UnreadableManifestDoesNotHideOthers` pins. - `internal/vaultik/helpers.go:76` `//nolint:tagliatelle` — pre-existing on `main`, unchanged. - `internal/vaultik/snapshot_list_test.go:444` `//nolint:tagliatelle` — the test's mirror of the wire format; asserting on snake_case is the point. - `snapshot_list_test.go:564` `//nolint:gosmopolitan` — pinning `time.Local` is literally the mechanism under test. - `snapshot_list_test.go:586` and `:756`, both `//nolint:paralleltest` — both accurate. One mutates `time.Local`, the other `os.Stdout` plus the global logger. Go runs non-parallel top-level tests to completion before resuming paused parallel ones, so neither mutation is observable elsewhere; `-race` agrees. --- ### Previously-clean items, re-verified at the new head - **No-private-key guard.** `TestListSnapshots_RemoteWithoutSecretKey` asserts `Config.AgeSecretKey` is empty up front, asserts exactly one prefix listing, asserts no fetched key contains `.age`, asserts the remote-only row renders and that neither `otherhost` nor `media` appears in the output. Reinstating an `AgeSecretKey == ""` gate breaks it on several counts. No such gate exists anywhere in the code. - **Privacy constraint.** `internal/snapshot/remotekey.go` and `internal/snapshot/manifest.go` are byte-identical to `main` (empty `git diff`). No `Put`, `Delete` or upload appears anywhere in `snapshot_list.go` — the listing path is read-only. The human ID is left zero for remote-only rows with a doc comment saying why, and `TestListSnapshots_JSONMergedView` asserts `Empty(remoteOnly.ID)`. - **errgroup correctness** — covered above. - **`RemotePresent *bool`.** `markRemotePresence` is called only in the `remoteErr == nil` branch (`:93-96`), so on an unreachable destination it stays nil and serializes as `null` (`json:"remote_present"`, no `omitempty`). `TestListSnapshots_JSONUnreachableRemote` asserts `Nil`; `TestListSnapshots_JSONMergedView` asserts a real `false` for a local-only row. "Absent" and "unknown" stay distinguishable. - **Single remote-manifest reader.** `snapshot.DecodeManifest` has exactly one non-test call site repo-wide: `internal/vaultik/snapshot.go:905`, inside `downloadManifestByKey`. The remaining `manifest.json.zst` occurrences are the upload in `internal/snapshot/snapshot.go:508`, two key-name filters (`prune.go:223`, `snapshot.go:1247`), and doc comments. `verify.go` and `info.go` are genuinely routed through the helper, and `info.go`'s path string was previously built identically, so behavior is preserved. --- ### The optional nits from the prior review All three taken, and the reasoning behind the first one checks out: - The duplicate `log.Warn` in `warnRemoteListingFailed` is gone. **Nothing is lost in `--cron`/`--quiet`.** `Vaultik.UI` is always `ui.New(os.Stdout)` (`internal/vaultik/vaultik.go:109`) — there is no discarding-writer swap anywhere — and quiet mode is `SetQuiet(true)`, which gates `Beginf/Completef/Infof/Noticef/Detailf/Progressf/Bannerf` but **not** `Warningf` or `Errorf` (`internal/ui/ui.go:69-207`). The warning still reaches a cron mailbox; only the `Infof` follow-up line is suppressed, as it should be. - `formatRemoteOnlyID` is now inside the `else` branch (`:584-594`). - `sort.SliceStable` at `:102` with a comment naming the zero-timestamp tie source. --- ### Nothing weakened - `.golangci.yml` → `021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb`. - `git diff origin/main` over `Dockerfile`, `Makefile`, `.gitea/`, `script/` is **empty**. - `git diff origin/main --stat -- '*_test.go'` is **insertions only** (+113, +801, 0 deletions). `grep '^-func Test'` over the test diff is empty — no test removed. No new `t.Skip` (the three in the tree are pre-existing and untouched). No weakened assertion. No file-level `//nolint`. ### Repo policy - No Claude/Anthropic reference in any commit message, commit body, code, comment, doc, or the PR body. No attribution trailers — `git log --format='%(trailers)'` is empty on all three commits. - Tip commit title: `Fix timezone drift and --json truncation in snapshot list (closes #64)`. Ends with ` (closes #64)`. - Inclusive terminology: clean. - `make fmt-check`: clean. - No scope creep. The `downloadManifestByKey` consolidation is its own commit and is explicitly requested by the issue's manager comment ("read the manifest through a single helper"). Issue #82's logger fix is correctly left alone; the workaround carries a comment naming #82 as its removal condition. - `TODO.md` is updated in the same commit as the work and is factually accurate against the diff, including the rework paragraph. **Next Step left unrotated is still correct**: the workflow rotates the step you did, and this was issue #64, not #71. Rotating would have recorded #71 as done when it is not. - Definition of done: all eight items satisfied, plus the five extra requirements from the manager comment on #64. --- ### Nits (non-blocking, no action required to merge) 1. `internal/vaultik/snapshot_list_test.go` — `captureProcessStdout` has no `defer` restoring `os.Stdout`. The one `require.NoError` inside the `fn` closure would `runtime.Goexit()` past the restore and past `writer.Close()`, leaving the package's `os.Stdout` pointing at an unread pipe and leaking the drain goroutine — turning a clean failure into a probable 30s package timeout. Only reachable when the test is already failing, so it costs diagnosability rather than correctness. `defer func() { os.Stdout = previous }()` would close it. 2. `internal/vaultik/snapshot_list.go:186` — `kvPairSize` is a package-scoped const in `internal/vaultik` named for a general structured-logging concern but declared mid-file between two listing functions. A local `const` inside `warnWhileListing`, or a name tied to that function, would keep the package namespace tidier. 3. `internal/vaultik/snapshot_list.go:277-282` — truncation cuts `unknown` in remote-key hex order, so the 1000 rows shown are arbitrary with respect to time rather than the newest 1000. This is inherent (timestamps require the very manifest reads being capped) and the omission is reported, so it is fine — but the cap is not mentioned in the README's `snapshot list` section, which is otherwise thorough. 4. Both `9a45221` and `0e2929d` end with ` (closes #64)`. The repo's default merge style is squash, so exactly one commit lands and the duplicate is harmless. Worth avoiding on future reworks only because it would double-close if the branch were ever merged unsquashed. --- ### Verdict **PASS.** The three blocking findings are fixed at the right layer, each is pinned by a test I confirmed fails when the fix is reverted, the new concurrent code is race-free with disjoint slots and deterministic ordering, nothing was weakened, and the tree is green under an uncached run at the pinned linter version. Label `merge-ready`, assign `sneak`.

clawbot added merge-ready and removed needs-review labels 2026-08-09 07:34:01 +02:00

clawbot merged commit 50e20b460e into main

2026-08-09 07:34:15 +02:00

clawbot deleted branch fix-snapshot-list-remote

2026-08-09 07:34:15 +02:00

clawbot referenced this pull request

2026-08-09 07:35:21 +02:00

snapshot list must list remote snapshots without requiring the private key #64

clawbot referenced this pull request

2026-08-09 07:41:30 +02:00

script/cibuild reports a green it did not earn when the tree is unchanged #85

clawbot referenced this pull request