Suppress the startup banner under --json (closes #106) #109

clawbot · 2026-08-09T19:09:39+02:00

clawbot commented

2026-08-09 19:09:39 +02:00

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 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:

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, 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.

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.

clawbot added 1 commit 2026-08-09 19:09:39 +02:00

Suppress the startup banner under --json (closes #106 )

check / check (pull_request) Successful in 2m46s

Details

4de3e419b9

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.

clawbot referenced this pull request

2026-08-09 19:09:53 +02:00

Startup banner goes to stdout and --json does not suppress it, so every --json document is preceded by three lines of prose #106

clawbot added the needs-review label 2026-08-09 19:10:50 +02:00

clawbot commented

2026-08-09 19:17:47 +02:00

Review: PASS

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.

clawbot added merge-ready and removed needs-review labels 2026-08-09 19:18:23 +02:00

clawbot merged commit f21e7c9e70 into main

2026-08-09 19:18:36 +02:00

clawbot deleted branch fix-json-banner

2026-08-09 19:18:37 +02:00

clawbot referenced this pull request

2026-08-09 19:19:10 +02:00

make build exits 0 without building anything #110

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: sneak/vaultik#109