Entry writes the banner to stdout before cobra parses anything, and
the scan deciding whether to write it knew --quiet, -q and --cron
but not --json, so every --json document arrived behind two lines of
prose and a blank line. --json and --json= now join that list.
The design question, answered
Extend the raw-argv scan. Do not move the banner after parsing.
The banner is printed before parsing deliberately, and root.go says
why: it must still appear when cobra rejects the arguments
(requires at least 2 arg(s)) and on --help. After parsing there is
no single place that runs on all of those paths, so "after parsing"
means reimplementing the banner in several handlers, or accepting that
it vanishes from the failure paths where a human most wants to know
which build they just ran. That is a real regression traded for a
cosmetic gain.
On whether the persistent/subcommand distinction matters here: it does
not, because the precedent already crosses it. --cron is in the
suppression list and is also a subcommand flag — it exists only on snapshot create (internal/cli/snapshot.go:137), not on the root
command. So matching --json anywhere in the vector adds another
instance of an imprecision the code already accepts, rather than a new
kind of one.
The two error directions are also not symmetric. A false positive loses
a decorative banner. A false negative corrupts a document on stdout and
breaks a pipeline. A scan that cannot be exact should err toward
suppression, and this one does — which is also why --json=false
suppresses the banner, exactly as --quiet=false already does. Making
the value parse honest for --json alone would make it inconsistent
with its two neighbours; the doc comment states the bias instead.
Tests
Three, in internal/cli, where TestListSnapshots_JSONStdoutIsOnlyTheDocument cannot reach:
TestEntryJSONStdoutIsExactlyOneDocument runs Entry itself with a
real argument vector over the process's real stdout file descriptor,
through cobra and the fx graph to the document, and asserts the
capture decodes as one JSON value with nothing after it. It is
hermetic because file:// storage is exempt from the S3 credential
checks and snapshot list treats a destination store with no metadata/ as an empty list rather than a failure.
TestJSONInvocationStdoutIsExactlyOneDocument covers the argument
vectors of all five --json commands, plus --json before the
subcommand, --json=true, and a flag-with-value ahead of it.
TestBannerStillPrintedWithoutSuppressingFlag asserts the banner is
still printed without a suppressing flag, so the first two cannot be
satisfied by deleting the banner.
Verified to fail without the fix by reverting bannerSuppressedInArgs,
not by assumption: all three fail, the end-to-end one included, with Should be empty, but was vaultik dev by Jeffrey Paul ... starting up at ....
AGENTS.md policy 9
Re-worded to key on the log stream rather than stdout. The operative
rule — structured output when the log stream is not a terminal — is
unchanged; only the stream it names was wrong after issue #82 moved the
logger and its format test to stderr.
The two smaller observations
bytesAttrKey: fixed rather than commented, being three lines.
Keys reaching writeAttr are group-qualified, so bytes logged under
an open group arrived as transfer.bytes and fell back to a bare
number. The match is now made on the final dot-separated segment,
tested grouped and ungrouped.
Vaultik.Stderr: kept. The comment still reads true as a
statement of intent, and now also says outright that nothing writes to
it today, so the next reader does not go hunting for a writer that
does not exist. listEnv.stderr in snapshot_list_test.go was
genuinely dead — assigned, never read, since those tests began
capturing the process's stderr — and is removed.
prune --json is still broken, for a different reason
Filed as issue #108, not fixed
here. pruneLocalSnapshots (internal/vaultik/snapshot.go:845-878)
writes three lines of prose to stdout with no --json awareness at all,
unlike every other writer on a --json path. It reproduces identically
on main before this change, -q never suppressed it either, and it is
a different writer on a different code path — the same reason #106 was
filed out of #82 rather than fixed inside it.
Verification
End to end against a real config with a file:// destination store and
a real snapshot, with no other flags and no -q:
script/cibuild exit 0: fresh CHECK_EPOCH per invocation, fmt-check, lint and test layers all executed (10 CACHED layers, all dependency
and module layers), 15 ok packages, 0 (cached), 134s wall.
.golangci.yml unchanged, sha256 021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb.
No tags created; origin still has zero.
Closes [issue #106](https://git.eeqj.de/sneak/vaultik/issues/106).
## The fix
`Entry` writes the banner to stdout before cobra parses anything, and
the scan deciding whether to write it knew `--quiet`, `-q` and `--cron`
but not `--json`, so every `--json` document arrived behind two lines of
prose and a blank line. `--json` and `--json=` now join that list.
## The design question, answered
**Extend the raw-argv scan. Do not move the banner after parsing.**
The banner is printed before parsing deliberately, and `root.go` says
why: it must still appear when cobra rejects the arguments
(`requires at least 2 arg(s)`) and on `--help`. After parsing there is
no single place that runs on all of those paths, so "after parsing"
means reimplementing the banner in several handlers, or accepting that
it vanishes from the failure paths where a human most wants to know
which build they just ran. That is a real regression traded for a
cosmetic gain.
On whether the persistent/subcommand distinction matters here: it does
not, because the precedent already crosses it. `--cron` is in the
suppression list and is **also** a subcommand flag — it exists only on
`snapshot create` (`internal/cli/snapshot.go:137`), not on the root
command. So matching `--json` anywhere in the vector adds another
instance of an imprecision the code already accepts, rather than a new
kind of one.
The two error directions are also not symmetric. A false positive loses
a decorative banner. A false negative corrupts a document on stdout and
breaks a pipeline. A scan that cannot be exact should err toward
suppression, and this one does — which is also why `--json=false`
suppresses the banner, exactly as `--quiet=false` already does. Making
the value parse honest for `--json` alone would make it inconsistent
with its two neighbours; the doc comment states the bias instead.
## Tests
Three, in `internal/cli`, where
`TestListSnapshots_JSONStdoutIsOnlyTheDocument` cannot reach:
- `TestEntryJSONStdoutIsExactlyOneDocument` runs `Entry` itself with a
real argument vector over the process's real stdout file descriptor,
through cobra and the fx graph to the document, and asserts the
capture decodes as one JSON value with nothing after it. It is
hermetic because `file://` storage is exempt from the S3 credential
checks and `snapshot list` treats a destination store with no
`metadata/` as an empty list rather than a failure.
- `TestJSONInvocationStdoutIsExactlyOneDocument` covers the argument
vectors of all five `--json` commands, plus `--json` before the
subcommand, `--json=true`, and a flag-with-value ahead of it.
- `TestBannerStillPrintedWithoutSuppressingFlag` asserts the banner is
still printed without a suppressing flag, so the first two cannot be
satisfied by deleting the banner.
Verified to fail without the fix by reverting `bannerSuppressedInArgs`,
not by assumption: all three fail, the end-to-end one included, with
`Should be empty, but was vaultik dev by Jeffrey Paul ... starting up
at ...`.
## `AGENTS.md` policy 9
Re-worded to key on the **log stream** rather than stdout. The operative
rule — structured output when the log stream is not a terminal — is
unchanged; only the stream it names was wrong after
[issue #82](https://git.eeqj.de/sneak/vaultik/issues/82) moved the
logger and its format test to stderr.
## The two smaller observations
- **`bytesAttrKey`**: fixed rather than commented, being three lines.
Keys reaching `writeAttr` are group-qualified, so `bytes` logged under
an open group arrived as `transfer.bytes` and fell back to a bare
number. The match is now made on the final dot-separated segment,
tested grouped and ungrouped.
- **`Vaultik.Stderr`**: kept. The comment still reads true as a
statement of intent, and now also says outright that nothing writes to
it today, so the next reader does not go hunting for a writer that
does not exist. `listEnv.stderr` in `snapshot_list_test.go` was
genuinely dead — assigned, never read, since those tests began
capturing the process's stderr — and is removed.
## `prune --json` is still broken, for a different reason
Filed as
[issue #108](https://git.eeqj.de/sneak/vaultik/issues/108), not fixed
here. `pruneLocalSnapshots` (`internal/vaultik/snapshot.go:845-878`)
writes three lines of prose to stdout with no `--json` awareness at all,
unlike every other writer on a `--json` path. It reproduces identically
on `main` before this change, `-q` never suppressed it either, and it is
a different writer on a different code path — the same reason #106 was
filed out of #82 rather than fixed inside it.
## Verification
End to end against a real config with a `file://` destination store and
a real snapshot, **with no other flags** and no `-q`:
| Command | `\| jq .` |
|---|---|
| `snapshot list --json` | exit 0 |
| `snapshot verify <id> --json` | exit 0 |
| `snapshot remove <id> --json` | exit 0 |
| `remote info --json` | exit 0 |
| `prune --json` | **fails** — [issue #108](https://git.eeqj.de/sneak/vaultik/issues/108), not the banner |
`script/cibuild` exit 0: fresh `CHECK_EPOCH` per invocation, `fmt-check`,
`lint` and `test` layers all executed (10 CACHED layers, all dependency
and module layers), 15 `ok` packages, 0 `(cached)`, 134s wall.
`.golangci.yml` unchanged, sha256
`021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb`.
No tags created; origin still has zero.
Entry writes the banner to stdout before cobra parses anything, and the
scan that decides whether to write it knew --quiet, -q and --cron but
not --json. Every --json document therefore arrived behind two lines of
prose and a blank line, and `vaultik snapshot list --json | jq` failed.
Passing opts.JSON as extraQuiet could not help: that reaches UI.SetQuiet
through an fx OnStart hook, long after the banner is already written.
With the logger moved to stderr in #82, this was the last writer that
could put something on stdout the caller did not ask for.
The design question the issue raised is answered in favour of extending
the raw-argv scan rather than moving the banner after parsing. The
banner is printed first deliberately, so that it still appears when
cobra rejects the arguments and on --help; after parsing there is no
single place that covers those paths, so "after parsing" means either
reimplementing the banner in several handlers or losing it exactly where
a human most wants to know which build just ran. The objection to the
scan is that --json is a subcommand flag matched anywhere in the vector,
but --cron is already in the list and is also a subcommand flag: it
exists only on `snapshot create`. So this adds another instance of an
imprecision the code already accepts, not a new kind of one. The two
error directions are not symmetric either — a false positive loses a
decorative banner, a false negative corrupts a document — so the scan
errs toward suppression, and --json=false suppresses it exactly as
--quiet=false already does.
Three tests at the CLI layer, where internal/vaultik's existing guard
cannot reach. TestEntryJSONStdoutIsExactlyOneDocument runs Entry itself
over the process's real stdout descriptor, through cobra and the fx
graph to the document, and asserts the capture decodes as one JSON value
with nothing after it; it is hermetic because file:// storage needs no
credentials and `snapshot list` treats a destination store with no
metadata/ as an empty list rather than a failure. A second covers the
argument vectors of all five --json commands plus the pre-subcommand and
--json=true forms. A third asserts the banner is still printed without a
suppressing flag, so the first cannot be satisfied by deleting it.
AGENTS.md policy 9 still keyed the structured-log format on stdout's
TTY-ness after #82 moved that decision to stderr; it now names the log
stream. A rules file that misdescribes the code misleads exactly the
readers who trust it most.
Two smaller findings from the same review. bytesAttrKey's human-readable
byte formatting stopped applying under an open group, because the key
reaching the comparison is group-qualified: "bytes" logged under a group
arrives as "transfer.bytes" and fell back to a bare number. The match is
now made on the final dot-separated segment, tested both grouped and
ungrouped. And listEnv.stderr in snapshot_list_test.go, assigned but
never read since those tests began capturing the process's stderr, is
removed. Vaultik.Stderr is kept — nothing writes to it today, which its
comment now says outright rather than leaving the next reader to hunt
for a writer that does not exist.
`prune --json` still does not survive jq, for an unrelated reason found
while verifying this: pruneLocalSnapshots writes three lines of prose to
stdout with no --json awareness, on main and after this change alike,
and -q never suppressed them either. Filed as #108 rather than fixed
here, being a different writer on a different code path.
Independent review of PR #109 at 4de3e41
against issue #106. No
blocking defects.
Verified
Definition of done met (4 of 5 --json commands, with the fifth
correctly excluded — see below); CI green on 4de3e41; head contains c16ef47, fast-forwardable; make fmt-check clean; container lint 0 issues. executed fresh (60.3s, fresh CHECK_EPOCH); no
Claude/Anthropic references or trailers anywhere in the diff or the
commit message; commit title ends (closes #106); .golangci.yml
sha256 matches, and Dockerfile, script/, REPO_POLICIES.md, .gitea/, Makefile are byte-identical to main; origin still has 0
tags; inclusive terminology clean; no existing test weakened; nothing
live deleted (listEnv.stderr confirmed write-only before removal).
Fail-without-fix reproduced independently, not taken on trust. With --json/--json= reverted in bannerSuppressedInArgs and isBytesAttr
neutered, make test fails exactly and only: TestEntryJSONStdoutIsExactlyOneDocument (with the real banner text
ahead of [] on the captured fd), TestJSONInvocationStdoutIsExactlyOneDocument, TestBannerSuppressedInArgs, and TestTTYHandlerByteFormattingSurvivesGrouping/grouped
(transfer.bytes=1536 instead of transfer.bytes=1.5 KB). The grouped subtest fails while ungrouped passes, which is the right
discrimination. TestBannerStillPrintedWithoutSuppressingFlag keeps
passing under the mutation, confirming it is a genuine anti-deletion
guard rather than a second copy of the same assertion.
End to end, built binary, real config, file:// store, real snapshot,
no -q and no other flags:snapshot list --json, remote info --json, snapshot verify <id> --json and snapshot remove <id> --json all pipe through jq -c . at exit 0. prune --json fails, with No stale local snapshots found. ahead of the
object — matching issue #108 exactly. internal/vaultik/snapshot.go is untouched by this diff, so that is
pre-existing by construction, and the PR body correctly claims four of
five rather than all five.
Argv-scan attack surface. No shorthand exists for --json on any of
the five registrations (all BoolVar), so there is no -j hole; no env
or config binding sets it. --json=false suppresses (verified: banner
gone, table printed) — the accepted false positive, consistent with --quiet=false and documented in the doc comment. --config --json ...
is likewise a false positive only. snapshot list -- --json prints the
banner and produces no JSON document (verified), so honouring the
terminator is correct. Design decision to keep the banner pre-parse is
sound: --help verified to still carry it, and --cron is confirmed
subcommand-only at internal/cli/snapshot.go:137, so this is another
instance of an accepted imprecision rather than a new kind. The error
asymmetry argument holds.
AGENTS.md policy 9's operative rule (jsonl when the log stream is not
a terminal) is preserved verbatim; only the stream name changed. No //nolint was added for goconst — the two directives added
(testpackage, gochecknoglobals) are both well-precedented on main,
and the hoisted command-word constants are sensible.
Non-blocking
One fail-open exists, and it is pre-existing.--config
consumes -- as its value in pflag, but bannerSuppressedInArgs
treats the same -- as the terminator and returns false. Verified
against the built binary from a directory containing a config file
literally named --: vaultik --config -- snapshot list --json emits the two banner lines,
the blank line, and then []. It requires a config file named --,
the identical hole already exists for --quiet on main, and
honouring the terminator is deliberate and tested
(entry_banner_test.go:152). Recording it because it is the one
banner-on-a---json-run case that exists; not a reason to hold this
PR.
README.md:142 is inaccurate in the way this PR argues against.
"The startup banner is the other thing that writes to stdout" reads
as the last such writer, but pruneLocalSnapshots writes three
prose lines to stdout on a --json path — the PR's own issue #108. The
commit message carries the same claim ("this was the last writer that
could put something on stdout the caller did not ask for"), though it
is qualified later in the same message. The actionable sentence that
follows it — snapshot list --json | jq . works on its own — is true,
which is why this is a nit rather than a defect. A clause such as "the
banner was the last one on the snapshot list path" would carry the
PR's own standard for a docs file that describes the code.
entry_banner_test.go:119 uses a bare "restore" while every
other command word in the file is hoisted to a constant — the exact
thing the constant block's comment and AGENTS.md policy 10 ask for.
entry_banner_test.go:101-104 is tautological: the test writes placeholderJSONDocument into the buffer itself and then asserts the
buffer holds one JSON document. The load-bearing assertion is the require.Empty above it (confirmed by the mutation run). Harmless,
but it reads as more coverage than it is.
cibuild
script/cibuild exit 0, 2m43s wall. Fresh CHECK_EPOCH forced the fmt-check, lint, test and build layers to execute (14 CACHED
layers, all dependency/module/base layers). 15 ok packages, 0 (cached) markers — script/test passes -count=1, so no result-cache
hit is possible. Lint reported 0 issues. in-container, with only the
known gomodguard deprecation warning.
## Review: PASS
Independent review of
[PR #109](https://git.eeqj.de/sneak/vaultik/pulls/109) at `4de3e41`
against [issue #106](https://git.eeqj.de/sneak/vaultik/issues/106). No
blocking defects.
### Verified
Definition of done met (4 of 5 `--json` commands, with the fifth
correctly excluded — see below); CI green on `4de3e41`; head contains
`c16ef47`, fast-forwardable; `make fmt-check` clean; container lint
`0 issues.` executed fresh (60.3s, fresh `CHECK_EPOCH`); no
Claude/Anthropic references or trailers anywhere in the diff or the
commit message; commit title ends ` (closes #106)`; `.golangci.yml`
sha256 matches, and `Dockerfile`, `script/`, `REPO_POLICIES.md`,
`.gitea/`, `Makefile` are byte-identical to `main`; origin still has 0
tags; inclusive terminology clean; no existing test weakened; nothing
live deleted (`listEnv.stderr` confirmed write-only before removal).
**Fail-without-fix reproduced independently**, not taken on trust. With
`--json`/`--json=` reverted in `bannerSuppressedInArgs` and `isBytesAttr`
neutered, `make test` fails exactly and only:
`TestEntryJSONStdoutIsExactlyOneDocument` (with the real banner text
ahead of `[]` on the captured fd),
`TestJSONInvocationStdoutIsExactlyOneDocument`,
`TestBannerSuppressedInArgs`, and
`TestTTYHandlerByteFormattingSurvivesGrouping/grouped`
(`transfer.bytes=1536` instead of `transfer.bytes=1.5 KB`). The
`grouped` subtest fails while `ungrouped` passes, which is the right
discrimination. `TestBannerStillPrintedWithoutSuppressingFlag` keeps
passing under the mutation, confirming it is a genuine anti-deletion
guard rather than a second copy of the same assertion.
**End to end, built binary, real config, `file://` store, real snapshot,
no `-q` and no other flags:** `snapshot list --json`,
`remote info --json`, `snapshot verify <id> --json` and
`snapshot remove <id> --json` all pipe through `jq -c .` at exit 0.
`prune --json` fails, with `No stale local snapshots found.` ahead of the
object — matching
[issue #108](https://git.eeqj.de/sneak/vaultik/issues/108) exactly.
`internal/vaultik/snapshot.go` is untouched by this diff, so that is
pre-existing by construction, and the PR body correctly claims four of
five rather than all five.
**Argv-scan attack surface.** No shorthand exists for `--json` on any of
the five registrations (all `BoolVar`), so there is no `-j` hole; no env
or config binding sets it. `--json=false` suppresses (verified: banner
gone, table printed) — the accepted false positive, consistent with
`--quiet=false` and documented in the doc comment. `--config --json ...`
is likewise a false positive only. `snapshot list -- --json` prints the
banner and produces no JSON document (verified), so honouring the
terminator is correct. Design decision to keep the banner pre-parse is
sound: `--help` verified to still carry it, and `--cron` is confirmed
subcommand-only at `internal/cli/snapshot.go:137`, so this is another
instance of an accepted imprecision rather than a new kind. The error
asymmetry argument holds.
`AGENTS.md` policy 9's operative rule (jsonl when the log stream is not
a terminal) is preserved verbatim; only the stream name changed. No
`//nolint` was added for `goconst` — the two directives added
(`testpackage`, `gochecknoglobals`) are both well-precedented on `main`,
and the hoisted command-word constants are sensible.
### Non-blocking
1. **One fail-open exists, and it is pre-existing.** `--config`
consumes `--` as its value in pflag, but `bannerSuppressedInArgs`
treats the same `--` as the terminator and returns false. Verified
against the built binary from a directory containing a config file
literally named `--`:
`vaultik --config -- snapshot list --json` emits the two banner lines,
the blank line, and then `[]`. It requires a config file named `--`,
the identical hole already exists for `--quiet` on `main`, and
honouring the terminator is deliberate and tested
(`entry_banner_test.go:152`). Recording it because it is the one
banner-on-a-`--json`-run case that exists; not a reason to hold this
PR.
2. **`README.md:142` is inaccurate in the way this PR argues against.**
"The startup banner is the other thing that writes to stdout" reads
as *the last* such writer, but `pruneLocalSnapshots` writes three
prose lines to stdout on a `--json` path — the PR's own
[issue #108](https://git.eeqj.de/sneak/vaultik/issues/108). The
commit message carries the same claim ("this was the last writer that
could put something on stdout the caller did not ask for"), though it
is qualified later in the same message. The actionable sentence that
follows it — `snapshot list --json | jq .` works on its own — is true,
which is why this is a nit rather than a defect. A clause such as "the
banner was the last one on the `snapshot list` path" would carry the
PR's own standard for a docs file that describes the code.
3. **`entry_banner_test.go:119`** uses a bare `"restore"` while every
other command word in the file is hoisted to a constant — the exact
thing the constant block's comment and `AGENTS.md` policy 10 ask for.
4. **`entry_banner_test.go:101-104`** is tautological: the test writes
`placeholderJSONDocument` into the buffer itself and then asserts the
buffer holds one JSON document. The load-bearing assertion is the
`require.Empty` above it (confirmed by the mutation run). Harmless,
but it reads as more coverage than it is.
### cibuild
`script/cibuild` exit 0, 2m43s wall. Fresh `CHECK_EPOCH` forced the
`fmt-check`, `lint`, `test` and `build` layers to execute (14 CACHED
layers, all dependency/module/base layers). 15 `ok` packages, 0
`(cached)` markers — `script/test` passes `-count=1`, so no result-cache
hit is possible. Lint reported `0 issues.` in-container, with only the
known `gomodguard` deprecation warning.
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Closes issue #106.
The fix
Entrywrites the banner to stdout before cobra parses anything, andthe scan deciding whether to write it knew
--quiet,-qand--cronbut not
--json, so every--jsondocument arrived behind two lines ofprose and a blank line.
--jsonand--json=now join that list.The design question, answered
Extend the raw-argv scan. Do not move the banner after parsing.
The banner is printed before parsing deliberately, and
root.gosayswhy: it must still appear when cobra rejects the arguments
(
requires at least 2 arg(s)) and on--help. After parsing there isno single place that runs on all of those paths, so "after parsing"
means reimplementing the banner in several handlers, or accepting that
it vanishes from the failure paths where a human most wants to know
which build they just ran. That is a real regression traded for a
cosmetic gain.
On whether the persistent/subcommand distinction matters here: it does
not, because the precedent already crosses it.
--cronis in thesuppression list and is also a subcommand flag — it exists only on
snapshot create(internal/cli/snapshot.go:137), not on the rootcommand. So matching
--jsonanywhere in the vector adds anotherinstance of an imprecision the code already accepts, rather than a new
kind of one.
The two error directions are also not symmetric. A false positive loses
a decorative banner. A false negative corrupts a document on stdout and
breaks a pipeline. A scan that cannot be exact should err toward
suppression, and this one does — which is also why
--json=falsesuppresses the banner, exactly as
--quiet=falsealready does. Makingthe value parse honest for
--jsonalone would make it inconsistentwith its two neighbours; the doc comment states the bias instead.
Tests
Three, in
internal/cli, whereTestListSnapshots_JSONStdoutIsOnlyTheDocumentcannot reach:TestEntryJSONStdoutIsExactlyOneDocumentrunsEntryitself with areal argument vector over the process's real stdout file descriptor,
through cobra and the fx graph to the document, and asserts the
capture decodes as one JSON value with nothing after it. It is
hermetic because
file://storage is exempt from the S3 credentialchecks and
snapshot listtreats a destination store with nometadata/as an empty list rather than a failure.TestJSONInvocationStdoutIsExactlyOneDocumentcovers the argumentvectors of all five
--jsoncommands, plus--jsonbefore thesubcommand,
--json=true, and a flag-with-value ahead of it.TestBannerStillPrintedWithoutSuppressingFlagasserts the banner isstill printed without a suppressing flag, so the first two cannot be
satisfied by deleting the banner.
Verified to fail without the fix by reverting
bannerSuppressedInArgs,not by assumption: all three fail, the end-to-end one included, with
Should be empty, but was vaultik dev by Jeffrey Paul ... starting up at ....AGENTS.mdpolicy 9Re-worded to key on the log stream rather than stdout. The operative
rule — structured output when the log stream is not a terminal — is
unchanged; only the stream it names was wrong after
issue #82 moved the
logger and its format test to stderr.
The two smaller observations
bytesAttrKey: fixed rather than commented, being three lines.Keys reaching
writeAttrare group-qualified, sobyteslogged underan open group arrived as
transfer.bytesand fell back to a barenumber. The match is now made on the final dot-separated segment,
tested grouped and ungrouped.
Vaultik.Stderr: kept. The comment still reads true as astatement of intent, and now also says outright that nothing writes to
it today, so the next reader does not go hunting for a writer that
does not exist.
listEnv.stderrinsnapshot_list_test.gowasgenuinely dead — assigned, never read, since those tests began
capturing the process's stderr — and is removed.
prune --jsonis still broken, for a different reasonFiled as
issue #108, not fixed
here.
pruneLocalSnapshots(internal/vaultik/snapshot.go:845-878)writes three lines of prose to stdout with no
--jsonawareness at all,unlike every other writer on a
--jsonpath. It reproduces identicallyon
mainbefore this change,-qnever suppressed it either, and it isa different writer on a different code path — the same reason #106 was
filed out of #82 rather than fixed inside it.
Verification
End to end against a real config with a
file://destination store anda real snapshot, with no other flags and no
-q:| jq .snapshot list --jsonsnapshot verify <id> --jsonsnapshot remove <id> --jsonremote info --jsonprune --jsonscript/cibuildexit 0: freshCHECK_EPOCHper invocation,fmt-check,lintandtestlayers all executed (10 CACHED layers, all dependencyand module layers), 15
okpackages, 0(cached), 134s wall..golangci.ymlunchanged, sha256021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb.No tags created; origin still has zero.
Review: PASS
Independent review of
PR #109 at
4de3e41against issue #106. No
blocking defects.
Verified
Definition of done met (4 of 5
--jsoncommands, with the fifthcorrectly excluded — see below); CI green on
4de3e41; head containsc16ef47, fast-forwardable;make fmt-checkclean; container lint0 issues.executed fresh (60.3s, freshCHECK_EPOCH); noClaude/Anthropic references or trailers anywhere in the diff or the
commit message; commit title ends
(closes #106);.golangci.ymlsha256 matches, and
Dockerfile,script/,REPO_POLICIES.md,.gitea/,Makefileare byte-identical tomain; origin still has 0tags; inclusive terminology clean; no existing test weakened; nothing
live deleted (
listEnv.stderrconfirmed write-only before removal).Fail-without-fix reproduced independently, not taken on trust. With
--json/--json=reverted inbannerSuppressedInArgsandisBytesAttrneutered,
make testfails exactly and only:TestEntryJSONStdoutIsExactlyOneDocument(with the real banner textahead of
[]on the captured fd),TestJSONInvocationStdoutIsExactlyOneDocument,TestBannerSuppressedInArgs, andTestTTYHandlerByteFormattingSurvivesGrouping/grouped(
transfer.bytes=1536instead oftransfer.bytes=1.5 KB). Thegroupedsubtest fails whileungroupedpasses, which is the rightdiscrimination.
TestBannerStillPrintedWithoutSuppressingFlagkeepspassing under the mutation, confirming it is a genuine anti-deletion
guard rather than a second copy of the same assertion.
End to end, built binary, real config,
file://store, real snapshot,no
-qand no other flags:snapshot list --json,remote info --json,snapshot verify <id> --jsonandsnapshot remove <id> --jsonall pipe throughjq -c .at exit 0.prune --jsonfails, withNo stale local snapshots found.ahead of theobject — matching
issue #108 exactly.
internal/vaultik/snapshot.gois untouched by this diff, so that ispre-existing by construction, and the PR body correctly claims four of
five rather than all five.
Argv-scan attack surface. No shorthand exists for
--jsonon any ofthe five registrations (all
BoolVar), so there is no-jhole; no envor config binding sets it.
--json=falsesuppresses (verified: bannergone, table printed) — the accepted false positive, consistent with
--quiet=falseand documented in the doc comment.--config --json ...is likewise a false positive only.
snapshot list -- --jsonprints thebanner and produces no JSON document (verified), so honouring the
terminator is correct. Design decision to keep the banner pre-parse is
sound:
--helpverified to still carry it, and--cronis confirmedsubcommand-only at
internal/cli/snapshot.go:137, so this is anotherinstance of an accepted imprecision rather than a new kind. The error
asymmetry argument holds.
AGENTS.mdpolicy 9's operative rule (jsonl when the log stream is nota terminal) is preserved verbatim; only the stream name changed. No
//nolintwas added forgoconst— the two directives added(
testpackage,gochecknoglobals) are both well-precedented onmain,and the hoisted command-word constants are sensible.
Non-blocking
One fail-open exists, and it is pre-existing.
--configconsumes
--as its value in pflag, butbannerSuppressedInArgstreats the same
--as the terminator and returns false. Verifiedagainst the built binary from a directory containing a config file
literally named
--:vaultik --config -- snapshot list --jsonemits the two banner lines,the blank line, and then
[]. It requires a config file named--,the identical hole already exists for
--quietonmain, andhonouring the terminator is deliberate and tested
(
entry_banner_test.go:152). Recording it because it is the onebanner-on-a-
--json-run case that exists; not a reason to hold thisPR.
README.md:142is inaccurate in the way this PR argues against."The startup banner is the other thing that writes to stdout" reads
as the last such writer, but
pruneLocalSnapshotswrites threeprose lines to stdout on a
--jsonpath — the PR's ownissue #108. The
commit message carries the same claim ("this was the last writer that
could put something on stdout the caller did not ask for"), though it
is qualified later in the same message. The actionable sentence that
follows it —
snapshot list --json | jq .works on its own — is true,which is why this is a nit rather than a defect. A clause such as "the
banner was the last one on the
snapshot listpath" would carry thePR's own standard for a docs file that describes the code.
entry_banner_test.go:119uses a bare"restore"while everyother command word in the file is hoisted to a constant — the exact
thing the constant block's comment and
AGENTS.mdpolicy 10 ask for.entry_banner_test.go:101-104is tautological: the test writesplaceholderJSONDocumentinto the buffer itself and then asserts thebuffer holds one JSON document. The load-bearing assertion is the
require.Emptyabove it (confirmed by the mutation run). Harmless,but it reads as more coverage than it is.
cibuild
script/cibuildexit 0, 2m43s wall. FreshCHECK_EPOCHforced thefmt-check,lint,testandbuildlayers to execute (14 CACHEDlayers, all dependency/module/base layers). 15
okpackages, 0(cached)markers —script/testpasses-count=1, so no result-cachehit is possible. Lint reported
0 issues.in-container, with only theknown
gomodguarddeprecation warning.