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

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

New Issue
Closed
opened 2026-08-09 18:20:13 +02:00 by clawbot · 3 comments
clawbot commented 2026-08-09 18:20:13 +02:00
Collaborator
Copy Link

Found while fixing
issue #82. Filing rather
than fixing drive-by: #82 is about the logger's sink, and this is a
different writer on a different code path.

Problem

internal/cli/entry.go:20-27 prints the startup banner to stdout before
cobra parses anything:

if !bannerSuppressedInArgs(os.Args[1:]) {
    writeStartupBanner(ui.New(os.Stdout), time.Now().UTC(), short)
}

bannerSuppressedInArgs recognizes only --quiet, -q and --cron.
It does not recognize --json, so every --json subcommand emits two
banner lines and a blank line onto stdout ahead of its document.

--json paths that pass opts.JSON as extraQuiet to runVaultikApp
(e.g. snapshot remove) do not help: that calls UI.SetQuiet(true) in
an fx OnStart hook, which runs long after Entry has already printed
the banner.

Reproduction

With a config at any permissions and the binary from main:

$ vaultik --config /tmp/demo/config.yml snapshot list --json
vaultik dev-... by Jeffrey Paul <sneak@sneak.berlin> (commit ..., built on ...) starting up at ....
https://sneak.berlin/go/vaultik

[]

$ vaultik --config /tmp/demo/config.yml snapshot list --json | python3 -c 'import json,sys; json.load(sys.stdin)'
json.decoder.JSONDecodeError: Expecting value: line 1 column 1 (char 0)

The workaround is -q, which suppresses the banner as a side effect —
but a machine consumer should not have to know that, and --json
already communicates the intent.

Affected

snapshot list --json, snapshot verify --json, snapshot remove --json, prune --json, remote info --json.

Expected

--json implies the banner is suppressed. stdout carries the document
and nothing else, which is the same contract
issue #82 established for
the logger.

Notes

  • Once #82 lands, the logger is no longer a source of stdout
    contamination, so the banner is the remaining one.
  • bannerSuppressedInArgs already scans raw os.Args for flags because
    the banner precedes cobra parsing; adding --json there is consistent
    with how --quiet/--cron are handled, though it does mean a
    subcommand-level flag is matched anywhere in the argument vector.
    Whether that is acceptable or whether the banner should instead move
    after parsing is the design question this issue needs answered.
  • A regression test should assert that stdout from a --json
    invocation parses as exactly one JSON document. The existing
    TestListSnapshots_JSONStdoutIsOnlyTheDocument covers the library
    call and cannot see Entry, so this one belongs in internal/cli.
Found while fixing [issue #82](https://git.eeqj.de/sneak/vaultik/issues/82). Filing rather than fixing drive-by: #82 is about the logger's sink, and this is a different writer on a different code path. ## Problem `internal/cli/entry.go:20-27` prints the startup banner to stdout before cobra parses anything: ```go if !bannerSuppressedInArgs(os.Args[1:]) { writeStartupBanner(ui.New(os.Stdout), time.Now().UTC(), short) } ``` `bannerSuppressedInArgs` recognizes only `--quiet`, `-q` and `--cron`. It does not recognize `--json`, so every `--json` subcommand emits two banner lines and a blank line onto stdout ahead of its document. `--json` paths that pass `opts.JSON` as `extraQuiet` to `runVaultikApp` (e.g. `snapshot remove`) do not help: that calls `UI.SetQuiet(true)` in an fx `OnStart` hook, which runs long after `Entry` has already printed the banner. ## Reproduction With a config at any permissions and the binary from `main`: ``` $ vaultik --config /tmp/demo/config.yml snapshot list --json vaultik dev-... by Jeffrey Paul <sneak@sneak.berlin> (commit ..., built on ...) starting up at .... https://sneak.berlin/go/vaultik [] ``` ``` $ vaultik --config /tmp/demo/config.yml snapshot list --json | python3 -c 'import json,sys; json.load(sys.stdin)' json.decoder.JSONDecodeError: Expecting value: line 1 column 1 (char 0) ``` The workaround is `-q`, which suppresses the banner as a side effect — but a machine consumer should not have to know that, and `--json` already communicates the intent. ## Affected `snapshot list --json`, `snapshot verify --json`, `snapshot remove --json`, `prune --json`, `remote info --json`. ## Expected `--json` implies the banner is suppressed. stdout carries the document and nothing else, which is the same contract [issue #82](https://git.eeqj.de/sneak/vaultik/issues/82) established for the logger. ## Notes - Once #82 lands, the logger is no longer a source of stdout contamination, so the banner is the remaining one. - `bannerSuppressedInArgs` already scans raw `os.Args` for flags because the banner precedes cobra parsing; adding `--json` there is consistent with how `--quiet`/`--cron` are handled, though it does mean a subcommand-level flag is matched anywhere in the argument vector. Whether that is acceptable or whether the banner should instead move after parsing is the design question this issue needs answered. - A regression test should assert that stdout from a `--json` invocation parses as exactly one JSON document. The existing `TestListSnapshots_JSONStdoutIsOnlyTheDocument` covers the library call and cannot see `Entry`, so this one belongs in `internal/cli`.
clawbot added this to the 1.0.0 milestone 2026-08-09 18:30:48 +02:00
clawbot commented 2026-08-09 18:44:19 +02:00
Author
Collaborator
Copy Link

Scope addition — one more item lands with this, from the
PR #107 review.

AGENTS.md policy 9 (lines 86-87) is now false. It states the log
format is keyed on stdout's TTY-ness. As of #82 the logger writes to
stderr and the format choice tests os.Stderr. The rule's operative
content is preserved — structured output when the log stream is not a
terminal — but the stream it names is wrong.

Fix it in the same change as the banner, since both are about which
stream carries what. One-line amendment: key the wording on the log
stream rather than stdout.

Worth doing rather than deferring: this repo has now corrected false
claims in snapshot.go's manifest comment (#81), the Vaultik.UI
--cron comment (#84), TTYHandler's WithAttrs doc (#97), and the
--cron flag help (#87). A rules file that misdescribes the code is the
same defect in the place agents are most likely to trust it.

Revised definition of done

  1. --json suppresses the startup banner; stdout carries the document
    and nothing else.
  2. AGENTS.md policy 9 names the log stream, not stdout.
  3. A regression test in internal/cli asserting a --json invocation's
    stdout parses as exactly one JSON document — the existing
    TestListSnapshots_JSONStdoutIsOnlyTheDocument covers the library
    call and cannot see Entry.
  4. The design question in the original body is answered explicitly:
    either bannerSuppressedInArgs scans raw os.Args for --json as it
    already does for --quiet/--cron (accepting that a subcommand-level
    flag is matched anywhere in the vector), or the banner moves after
    parsing. Say which and why.
  5. script/cibuild exits 0.

Two smaller observations from the same review, fix if cheap:

  • Vaultik.Stderr now has zero production writers and env.stderr zero
    readers. vaultik.go documents keeping it deliberately — confirm that
    still reads true, or remove it.
  • bytesAttrKey's human-readable byte formatting does not survive
    grouping (x.bytes no longer matches the key check). Unreachable
    today; worth a comment if not a fix.
Scope addition — one more item lands with this, from the [PR #107](https://git.eeqj.de/sneak/vaultik/pulls/107) review. **`AGENTS.md` policy 9 (lines 86-87) is now false.** It states the log format is keyed on **stdout**'s TTY-ness. As of #82 the logger writes to stderr and the format choice tests `os.Stderr`. The rule's operative content is preserved — structured output when the log stream is not a terminal — but the stream it names is wrong. Fix it in the same change as the banner, since both are about which stream carries what. One-line amendment: key the wording on the **log stream** rather than stdout. Worth doing rather than deferring: this repo has now corrected false claims in `snapshot.go`'s manifest comment (#81), the `Vaultik.UI` `--cron` comment (#84), `TTYHandler`'s `WithAttrs` doc (#97), and the `--cron` flag help (#87). A rules file that misdescribes the code is the same defect in the place agents are most likely to trust it. ## Revised definition of done 1. `--json` suppresses the startup banner; stdout carries the document and nothing else. 2. `AGENTS.md` policy 9 names the log stream, not stdout. 3. A regression test in `internal/cli` asserting a `--json` invocation's stdout parses as exactly one JSON document — the existing `TestListSnapshots_JSONStdoutIsOnlyTheDocument` covers the library call and cannot see `Entry`. 4. The design question in the original body is answered explicitly: either `bannerSuppressedInArgs` scans raw `os.Args` for `--json` as it already does for `--quiet`/`--cron` (accepting that a subcommand-level flag is matched anywhere in the vector), or the banner moves after parsing. Say which and why. 5. `script/cibuild` exits 0. Two smaller observations from the same review, fix if cheap: - `Vaultik.Stderr` now has zero production writers and `env.stderr` zero readers. `vaultik.go` documents keeping it deliberately — confirm that still reads true, or remove it. - `bytesAttrKey`'s human-readable byte formatting does not survive grouping (`x.bytes` no longer matches the key check). Unreachable today; worth a comment if not a fix.
clawbot commented 2026-08-09 18:56:03 +02:00
Author
Collaborator
Copy Link

Implementation plan, on branch fix-json-banner off main.

1. The design question — extend bannerSuppressedInArgs, do not move the banner.

Moving the banner after parsing costs more than it buys. root.go
already documents why it is printed first: it must appear even 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 disappears from the failure
paths where a human most wants to know which build they just ran.

The stated objection to the scan is that --json is a subcommand flag
matched anywhere in the vector. That objection applies equally to
--cron, which is already in the list and is also a subcommand flag —
it exists only on snapshot create
(internal/cli/snapshot.go:137). So this adds no new class of
imprecision, only another instance of one the code already accepts.
And the two failure directions are not symmetric: a false positive
loses a decorative banner, a false negative corrupts a document on
stdout. The scan should err toward suppression, and does.

2. --json and --json= join --quiet/-q/--cron in the scan.
--json=false will suppress the banner, exactly as --quiet=false
already does. Making the value parse honest for --json alone would
make it inconsistent with its two neighbours, and fixing all three is a
behaviour change nobody asked for; the doc comment states the bias
instead.

3. Tests, in internal/cli. Two layers:

  • TestEntryJSONStdoutIsExactlyOneDocument runs Entry itself with a
    real argument vector over the process's real stdout descriptor, all
    the way through cobra and fx to the document, and asserts the capture
    decodes as exactly one JSON value with nothing after it. It is
    hermetic because storage_url: file:// 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 — so the
    end-to-end path needs no network and no credentials.
  • A table over the argument vectors of all five --json commands
    (plus --json before the subcommand, --json=true, and a
    flag-with-value ahead of it), and a paired test asserting the banner
    is still printed without a suppressing flag, so the first test cannot
    be satisfied by deleting the banner.

4. AGENTS.md policy 9 re-worded to key on the log stream rather
than stdout, preserving the operative rule.

5. The two smaller observations.

  • Vaultik.Stderr: kept, comment amended to say outright that nothing
    writes to it today, so the next reader does not go looking for the
    writer. listEnv.stderr in snapshot_list_test.go is genuinely dead
    — assigned, never read, because those tests capture the process's
    stderr instead — and is removed.
  • bytesAttrKey: fixed rather than commented, since the fix is three
    lines. The key reaching writeAttr is group-qualified, so the match
    is made against the final dot-separated segment. Covered by a test
    over both the grouped and ungrouped renderings.

Verification: script/cibuild, plus vaultik snapshot list --json | jq .
against a real config with no other flags, and the same for snapshot verify, snapshot remove, prune and remote info. The new test will
be confirmed to fail against the unfixed bannerSuppressedInArgs by
reverting it, not by assumption.

Implementation plan, on branch `fix-json-banner` off `main`. **1. The design question — extend `bannerSuppressedInArgs`, do not move the banner.** Moving the banner after parsing costs more than it buys. `root.go` already documents why it is printed first: it must appear even 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 disappears from the failure paths where a human most wants to know which build they just ran. The stated objection to the scan is that `--json` is a subcommand flag matched anywhere in the vector. That objection applies equally to `--cron`, which is already in the list and is also a subcommand flag — it exists only on `snapshot create` (`internal/cli/snapshot.go:137`). So this adds no new class of imprecision, only another instance of one the code already accepts. And the two failure directions are not symmetric: a false positive loses a decorative banner, a false negative corrupts a document on stdout. The scan should err toward suppression, and does. **2. `--json` and `--json=` join `--quiet`/`-q`/`--cron` in the scan.** `--json=false` will suppress the banner, exactly as `--quiet=false` already does. Making the value parse honest for `--json` alone would make it inconsistent with its two neighbours, and fixing all three is a behaviour change nobody asked for; the doc comment states the bias instead. **3. Tests, in `internal/cli`.** Two layers: - `TestEntryJSONStdoutIsExactlyOneDocument` runs `Entry` itself with a real argument vector over the process's real stdout descriptor, all the way through cobra and fx to the document, and asserts the capture decodes as exactly one JSON value with nothing after it. It is hermetic because `storage_url: file://` 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 — so the end-to-end path needs no network and no credentials. - A table over the argument vectors of all five `--json` commands (plus `--json` before the subcommand, `--json=true`, and a flag-with-value ahead of it), and a paired test asserting the banner is still printed without a suppressing flag, so the first test cannot be satisfied by deleting the banner. **4. `AGENTS.md` policy 9** re-worded to key on the log stream rather than stdout, preserving the operative rule. **5. The two smaller observations.** - `Vaultik.Stderr`: kept, comment amended to say outright that nothing writes to it today, so the next reader does not go looking for the writer. `listEnv.stderr` in `snapshot_list_test.go` is genuinely dead — assigned, never read, because those tests capture the process's stderr instead — and is removed. - `bytesAttrKey`: fixed rather than commented, since the fix is three lines. The key reaching `writeAttr` is group-qualified, so the match is made against the final dot-separated segment. Covered by a test over both the grouped and ungrouped renderings. Verification: `script/cibuild`, plus `vaultik snapshot list --json | jq .` against a real config with no other flags, and the same for `snapshot verify`, `snapshot remove`, `prune` and `remote info`. The new test will be confirmed to fail against the unfixed `bannerSuppressedInArgs` by reverting it, not by assumption.
clawbot commented 2026-08-09 19:09:53 +02:00
Author
Collaborator
Copy Link

Implemented in
PR #109 (branch
fix-json-banner, one commit).

Built. --json and --json= join --quiet/-q/--cron in
bannerSuppressedInArgs. The design question is answered in favour of
extending the raw-argv scan: the banner is printed before parsing
deliberately, so that it survives cobra rejecting the arguments and
--help, and after parsing no single place covers those paths. The
subcommand-vs-persistent distinction does not decide it, because
--cron is already in the list and is also a subcommand flag — it
exists only on snapshot create. Full reasoning in the PR body.
AGENTS.md policy 9 now names the log stream. bytesAttrKey matches on
the final dot-separated segment, so byte formatting survives grouping.
Vaultik.Stderr kept with the comment amended to say outright that
nothing writes to it; listEnv.stderr removed as dead.

Verified. The three new internal/cli tests were confirmed to fail
against the unfixed scan by reverting it, not by assumption — including
TestEntryJSONStdoutIsExactlyOneDocument, which runs Entry itself over
the process's real stdout descriptor through cobra and fx to the
document. End to end against a real config and a real snapshot, with no
other flags and no -q: snapshot list --json | jq .,
snapshot verify --json, snapshot remove --json and
remote info --json all exit 0. script/cibuild exit 0, 15 ok
packages, 0 (cached), 134s.

One item of the definition of done is not met, deliberately.
prune --json | jq still fails, for a reason that is not the banner:
pruneLocalSnapshots writes three lines of prose to stdout with no
--json awareness, it reproduces identically on main before this
change, and -q never suppressed it either. Filed as
issue #108 rather than
fixed here — a different writer on a different code path, the same
reason this issue was filed out of
issue #82 instead of
being folded into it. It also needs a decision this issue does not
contain: whether those lines belong in the document, on the logger, or
suppressed outright.

Implemented in [PR #109](https://git.eeqj.de/sneak/vaultik/pulls/109) (branch `fix-json-banner`, one commit). **Built.** `--json` and `--json=` join `--quiet`/`-q`/`--cron` in `bannerSuppressedInArgs`. The design question is answered in favour of extending the raw-argv scan: the banner is printed before parsing deliberately, so that it survives cobra rejecting the arguments and `--help`, and after parsing no single place covers those paths. The subcommand-vs-persistent distinction does not decide it, because `--cron` is already in the list and is also a subcommand flag — it exists only on `snapshot create`. Full reasoning in the PR body. `AGENTS.md` policy 9 now names the log stream. `bytesAttrKey` matches on the final dot-separated segment, so byte formatting survives grouping. `Vaultik.Stderr` kept with the comment amended to say outright that nothing writes to it; `listEnv.stderr` removed as dead. **Verified.** The three new `internal/cli` tests were confirmed to fail against the unfixed scan by reverting it, not by assumption — including `TestEntryJSONStdoutIsExactlyOneDocument`, which runs `Entry` itself over the process's real stdout descriptor through cobra and fx to the document. End to end against a real config and a real snapshot, with no other flags and no `-q`: `snapshot list --json | jq .`, `snapshot verify --json`, `snapshot remove --json` and `remote info --json` all exit 0. `script/cibuild` exit 0, 15 `ok` packages, 0 `(cached)`, 134s. **One item of the definition of done is not met, deliberately.** `prune --json | jq` still fails, for a reason that is not the banner: `pruneLocalSnapshots` writes three lines of prose to stdout with no `--json` awareness, it reproduces identically on `main` before this change, and `-q` never suppressed it either. Filed as [issue #108](https://git.eeqj.de/sneak/vaultik/issues/108) rather than fixed here — a different writer on a different code path, the same reason this issue was filed out of [issue #82](https://git.eeqj.de/sneak/vaultik/issues/82) instead of being folded into it. It also needs a decision this issue does not contain: whether those lines belong in the document, on the logger, or suppressed outright.
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
merge-ready
All checks pass, reviewed, rebased, ready for merge
 needs-checks
make check does not pass cleanly
 needs-rebase
PR has merge conflicts or is behind main
 needs-review
Code review not yet done
 needs-rework
Code review found issues to fix
No Label
Milestone
No items
No Milestone 1.0.0
Projects
Clear projects
No project
Assignees
Clear assignees
clawbot sneak
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: sneak/vaultik#106
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?