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.
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.
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
script/lint ran bare golangci-lint from PATH while CI and the Dockerfile
pinned v2.12.2 by digest, so make lint and CI could disagree about
findings. That drift ran both directions: it produced two false green
claims during the lint remediation, and on an ambient 2.10.1 it also
reported four gosec findings on a tree CI linted clean.
script/lint now extracts the image reference - tag and digest - from the
Dockerfile lint stage FROM line and runs that exact image under docker.
The Dockerfile FROM line is the single source of truth for the linter
version; the duplicate pins in the Makefile deps target and in
script/bootstrap are removed rather than kept in sync.
A golangci-lint on PATH is used only when its version exactly equals the
pin, which is what makes the in-container lint stage work (the Dockerfile
runs make lint inside the pinned image, where there is no docker daemon).
Any other version, or none, goes through docker. When docker is
unavailable the script fails with an actionable message and never falls
back to a different linter version.
script/lint-fix delegates to script/lint --fix so autofixes come from the
pinned linter too. The container mounts persistent build and module
caches and runs as the invoking uid/gid.
Verified by reinstating the four historical nolint directives that 2.10.1
requires and 2.12.2 reports as unused: the old script passed on that tree
and the new one fails with four nolintlint findings.
Clears the final 80 golangci-lint findings under the canonical
.golangci.yml (sha256 021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb),
taking the repo from red to green: script/cibuild exits 0.
- wsl_v5 (60): blank line above defer/go statements sharing no variable
with the line above; blank-line-only diff.
- sqlclosecheck (10): the package-local CloseRows helper hid the close
from the analyzer. Helper removed; all 18 call sites now defer an
inline rows.Close(), preserving the fatal-on-close-error path. No
resource leak existed - the rows were always being closed.
- prealloc (3): append targets given a starting capacity.
- revive (3): package-name findings suppressed with per-site directives
pending the naming decision tracked in #76.
No gosec suppressions are needed under the pinned linter. .golangci.yml,
Dockerfile, Makefile, .gitea/ and script/ are byte-identical to main.
Verified with script/cibuild (digest-pinned golangci-lint v2.12.2), not
make check - the latter resolves the linter from PATH and is not a
trustworthy gate here; see #78.
Closes#59.
Updates golangci-lint to v2.12.2 everywhere it is pinned and installs the canonical `.golangci.yml`, then remediates every finding the new linter/config surfaces so `make check` is green.
## Version bump
- `Dockerfile` lint stage: `golangci/golangci-lint:v2.11.3-alpine` -> `v2.12.2-alpine` (digest-pinned, date comment updated)
- `Makefile` `deps` target: `go install` moved from the old v1 module path at `@latest` to the pinned `github.com/golangci/golangci-lint/v2/cmd/golangci-lint@v2.12.2`
- `.golangci.yml` replaced with the canonical config (v2 schema; settings under `linters.settings` so the thresholds actually apply; `default: all` with the standard six disables)
- `script/bootstrap` installs golangci-lint via the system package manager and carries no version pin, so it is unchanged
- CI (`.gitea/workflows/check.yml`) only runs `script/cibuild`, so it needed no change
## Lint remediation
The canonical config surfaced ~3,300 findings across 56k lines. All are fixed, behavior-preserving; incorporates and supersedes the per-package mechanical passes already merged to `main` (refs #61). Highlights:
- `err113`: dynamic errors replaced with package sentinels + `%w` wrapping; comparisons via `errors.Is`
- `goprintffuncname`: printf-style helpers renamed with an `f` suffix (`ui.Writer` message methods, `cli.ReportErrorf`, `database.Fatalf`) and all call sites updated
- `revive` stutter renames: `blob.Handler`, `blob.WithReader`, `blob.ChunkPosition`, `storage.URL`, `storage.Info`; missing doc comments added
- `contextcheck`/`noctx`: `context.Context` threaded through `blob.Packer` and the scanner call sites; context-aware `exec`/`sql` variants
- `funlen`/`cyclop`/`gocognit`/`dupl`: oversized and duplicated functions split into focused helpers (production and test code)
- tests: `t.Parallel()` added where safe (global logger init kept in the serial phase for `-race`), `t.TempDir()`/`t.Helper()` adopted, several suites converted to external test packages
- `gosec`: bounded integer conversions, `ReadHeaderTimeout` on the test HTTP server; remaining warnings suppressed per-site with justifications
- remaining `nolint` directives are rare, targeted, and each carries a reason (e.g. `nilnil` not-found contract in the repository layer, fx module globals, on-disk snake_case struct tags)
- removed the deprecated `log.LogOptions` alias (callers migrated to `log.Options`)
`make check` (tests with `-race`, lint, fmt-check) passes.
Co-authored-by: sneak <sneak@sneak.berlin>
Reviewed-on: #62
Co-authored-by: clawbot <clawbot@noreply.example.org>
Co-committed-by: clawbot <clawbot@noreply.example.org>
Copied byte-for-byte from the vendored policy set in sneak/prompts.
The new config surfaces 2,990 lint findings; remediation is tracked
in issue #61 rather than being bundled here, so #59 stays open until
make check is green under this config.
Adds `.editorconfig`, copied byte-for-byte from `sneak/dnswatcher` (blob `2fe0ce0`). This is the small, safe half of #59; the `.golangci.yml` half is deferred — adopting the org-standard config surfaces ~2,990 lint findings on vaultik and needs a separate lint-cleanup decision (see #59). Hence `refs #59`, not `closes`.
`make check` and `docker build .` are green (a static config file does not affect them). Left open for review (not merged).
Co-authored-by: sneak <sneak@sneak.berlin>
Reviewed-on: #60
Co-authored-by: clawbot <clawbot@noreply.example.org>
Co-committed-by: clawbot <clawbot@noreply.example.org>
The verb surface accumulated overlapping cleanup commands. Consolidate
so each cleanup verb has one meaning:
- Rename 'database purge' -> 'database delete'. The command removes the
SQLite file entirely; "purge" wrongly suggested purging contents.
- Fold 'snapshot cleanup' into 'prune'. Prune now runs three passes:
reconcile local snapshots against the remote (previously the
standalone cleanup command), drop orphaned local rows, then delete
unreferenced remote blobs. One command, one mental model.
- Delete 'store info'. Its output was a strict subset of 'remote info',
which already prints storage type + location. Any user reaching for
either should reach for 'remote info'.
- Drop 'snapshot remove --all'. It duplicated 'remote nuke --force'.
'remote nuke' is the single supported entry point for wiping the
destination store.
Also update the storage-binding error message to reference the new
'vaultik database delete' name.
The local index tracks which chunks and blobs already exist at the
backup destination. Nothing was recording *which* destination, so
changing storage_url and running a backup left the scanner treating
every already-seen chunk as still-present at the new (empty) location.
Uploads were skipped silently and the resulting snapshots pointed at
blobs that don't exist at the new destination.
Fix: record storage_url in a new local_meta key-value table on first
mutating command, and refuse to proceed when the configured URL later
differs from the stored one. The error explains the two recovery
paths (revert the config, or run 'vaultik database purge' to discard
the index and rebuild from a fresh full backup).
Wired into snapshot create / prune / snapshot remove / snapshot purge
/ snapshot cleanup. Read-only inspection commands (snapshot list,
remote info, store info) are exempt.
chown(2) requires root on every Unix-ish kernel. Restoring 39k files
as a non-root user produces 39k EPERM syscalls plus 39k matching debug
log lines, all for an operation that can't possibly succeed. Skip the
syscall entirely when euid != 0, and emit one warning at the end of the
restore so the user knows the on-disk UID/GID will reflect the running
user rather than the original owner.
The previous change had snapshot rm auto-prune unreferenced blobs. The
correct division of labor is: rm removes a snapshot (local DB + remote
metadata), prune cleans up blobs. Reverting the auto-prune means rm
stays a cheap, deterministic operation: it touches one snapshot's worth
of state and emits the exact 'vaultik prune' command the user should
run next to delete blobs no longer referenced by any remote manifest.
This is correct because prune must consult every remote manifest
(including snapshots this host doesn't know about) to determine which
blobs are still referenced, and folding that work into rm would
silently turn rm into an expensive O(remote snapshots) operation that
also assumes the remote is fully reachable.
snapshot rm <id> now does the full cleanup: removes the local index
entry, strips the snapshot's metadata from the destination store, and
prunes any blobs that were only referenced by the just-removed manifest.
The --remote flag is retired; --local-only opts out for the rare case
where the user wants to forget a snapshot locally without touching the
remote.
If the destination store is unreachable, the local-DB removal still
completes and a warning is emitted; the user can rerun 'vaultik prune'
to retry the remote half later.
RemoveAllSnapshots gets the same treatment: after deleting every
snapshot's metadata (local + remote + orphan keys), an automatic blob
prune sweep removes the now-unreferenced blob set.
