Vaultik.UI doc comment misdescribes how --cron suppresses output #84
Reference in New Issue
Block a user
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?
Found while reworking PR #83. Small, but the wrong-comment class of bug is
worth closing out rather than leaving to mislead the next reader.
The
Vaultik.UIfield's doc comment states that the CLI swaps in adiscarding writer in
--cronmode. That is not the mechanism. The actualbehavior is
UI.SetQuiet(true), which silences informational output butstill permits Warning and Error.
The distinction matters. A discarding writer would mean nothing reaches
the user in cron mode;
SetQuietmeans warnings and errors still do.Someone trusting the comment would reasonably assume diagnostics vanish
under
--cronand could either suppress something that should surface, orskip handling output that in fact still appears.
This is the same failure mode as the false comment corrected in #81
(
snapshot.go:842claimed the public manifest bytes carry no timestamp,while the next statement wrote one) and it is adjacent to #82 (the logger
writes to stdout, so those still-permitted warnings can corrupt
--jsonoutput). A comment that misstates which output survives is exactly the
kind of thing that produces bugs like PR #83's finding 3.
Definition of done
Vaultik.UIdoc comment names the real mechanism —UI.SetQuiet(true)— and states explicitly that Warning and Error arestill emitted.
--crondiscards orsilences all output, and correct those too. Do not fix only the one
instance.
to match the corrected description either, stop and report rather than
changing behavior under a docs issue.
script/cibuildexits 0.Docs-only, so per repo policy this can skip adversarial review and merge
directly once the gate is green.
Implementation plan (branch
fix-ui-cron-commentoffmain):I verified the claim against the code before writing anything:
internal/vaultik/vaultik.go:109— theUIfield is constructed asui.New(os.Stdout).internal/cli/app.go:58-60—setupGlobalscallsv.UI.SetQuiet(true)whenopts.Cron || opts.Quiet. No writer is ever swapped; the field is never reassigned outsideNew.internal/ui/ui.go—quietis checked byBeginf,Completef,Infof,Noticef,Detailf,Progressf, andBannerfonly.Warningf(line 158) andErrorf(line 167) have no quiet check and always write.So the issue's description is correct and the existing comment is false. The only
io.DiscardUI writer in the tree isinternal/snapshot/scanner.go:176, a fallback for a nil scanner UI — unrelated to--cron, which is likely where the "discarding writer" wording came from.Planned changes (comments/docs only, no behavior change):
Vaultik.UIdoc comment to nameUI.SetQuiet(true)in the cli layer, note it applies to--cronand--quietalike, and state explicitly that Warning and Error are still emitted.README.mdline 171 documents--cronas "Silent unless error", which understates what survives — warnings are emitted too. Correcting that line to name warnings as well.--cronmentions and found them already accurate, so they stay untouched:internal/log/log.go:53-55,internal/cli/snapshot.go:104,internal/vaultik/snapshot.go:116-120.TODO.mdentry in the same commit.Out of scope and deliberately not touched: #82 (logger to stdout), the
--jsonstderr workaround insnapshot_list.go, and the--croncobra flag help string atinternal/cli/snapshot.go:138("silent unless error") — that one carries the same imprecision as the README line, but it is program output rather than a comment, so changing it would violate the no-behavior-change constraint of this issue. Reporting it for separate triage.Gate:
script/cibuild, forced uncached.