Fixes #41: a failed command printed the whole usage text after its error, which buried the error. A wrong passphrase and an unreadable vault looked alike.
The root command now has a PersistentPreRunE that turns usage off for the command being run. Cobra runs that hook after it has parsed the flags and checked the number of arguments. So wrong arity, an unknown flag and a bad flag value still get usage, and an error from running the command gets none. Cobra checks required flags and flag groups (mutually exclusive, required together, one required) only after the hook, so the hook checks both first: secret import x without --source still gets usage.
Cobra still prints the error, once. Entry() is unchanged, so the order stays: print the error, purge memory, exit with code 1.
The new test runs each kind of wrong call plus one failure while running (secret get x with no vault). It checks that the command fails, that its error appears exactly once, and that usage appears only for the wrong calls.
Deviation: root SilenceUsage: true, which the issue suggests, is not used. In cobra v1.9.1, argument and flag errors take the same path as errors from running the command, so it would hide usage for them too.
Judgement call: a missing required flag or a broken flag group counts as a flag error, so it keeps usage.
Unverified: no command uses flag groups yet, so the test does not cover that case.
Trap: a subcommand that defines its own PersistentPreRun replaces the root one, and would get usage after every failure again. No subcommand does today.
Model: opus-5-5
Fixes https://git.eeqj.de/sneak/secret/issues/41: a failed command printed the whole usage text after its error, which buried the error. A wrong passphrase and an unreadable vault looked alike.
The root command now has a `PersistentPreRunE` that turns usage off for the command being run. Cobra runs that hook after it has parsed the flags and checked the number of arguments. So wrong arity, an unknown flag and a bad flag value still get usage, and an error from running the command gets none. Cobra checks required flags and flag groups (mutually exclusive, required together, one required) only after the hook, so the hook checks both first: `secret import x` without `--source` still gets usage.
Cobra still prints the error, once. `Entry()` is unchanged, so the order stays: print the error, purge memory, exit with code 1.
The new test runs each kind of wrong call plus one failure while running (`secret get x` with no vault). It checks that the command fails, that its error appears exactly once, and that usage appears only for the wrong calls.
- Deviation: root `SilenceUsage: true`, which the issue suggests, is not used. In cobra v1.9.1, argument and flag errors take the same path as errors from running the command, so it would hide usage for them too.
- Judgement call: a missing required flag or a broken flag group counts as a flag error, so it keeps usage.
- Unverified: no command uses flag groups yet, so the test does not cover that case.
- Trap: a subcommand that defines its own `PersistentPreRun` replaces the root one, and would get usage after every failure again. No subcommand does today.
Model: opus-5-5
internal/cli/root.go lines 51-66, the PersistentPreRunE hook and its comment: the comment says cobra has checked the arguments and flags before the hook runs, except for required flags. Cobra v1.9.1 also checks flag groups (flags marked mutually exclusive, required together, or one required) only after the hook. A wrong call that breaks one of those would print its error without usage, and the comment tells the next reader that cannot happen. No command uses flag groups today, so nothing misbehaves now, but the comment is wrong and hides that gap. Acceptable: the hook also calls cmd.ValidateFlagGroups() after cmd.ValidateRequiredFlags(), and the comment names both. Make the same sentence in the TODO.md entry and the commit message match.
Note: TODO.md conflicts with current next (both add a Completed Steps entry). Keep both entries when rebasing.
Model: opus-5-5
FAIL: needs rework.
1. `internal/cli/root.go` lines 51-66, the `PersistentPreRunE` hook and its comment: the comment says cobra has checked the arguments and flags before the hook runs, except for required flags. Cobra v1.9.1 also checks flag groups (flags marked mutually exclusive, required together, or one required) only after the hook. A wrong call that breaks one of those would print its error without usage, and the comment tells the next reader that cannot happen. No command uses flag groups today, so nothing misbehaves now, but the comment is wrong and hides that gap. Acceptable: the hook also calls `cmd.ValidateFlagGroups()` after `cmd.ValidateRequiredFlags()`, and the comment names both. Make the same sentence in the `TODO.md` entry and the commit message match.
Note: `TODO.md` conflicts with current `next` (both add a Completed Steps entry). Keep both entries when rebasing.
Model: opus-5-5
A failed command printed the whole usage text after its error, burying
it. The root command's PersistentPreRunE now turns usage off, so an
error from running the command is printed once on its own. Wrong arity,
an unknown flag, a bad flag value, a missing required flag and broken
flag groups still get usage: cobra checks arguments and flag values
before that hook but required flags and flag groups only after it, so
the hook checks those two first. Root SilenceUsage was not used: in
this cobra version it hides usage for argument and flag errors too.
Cobra still prints the error; Entry is unchanged.
Model: opus-5-5
The root PersistentPreRunE now calls cmd.ValidateFlagGroups() after cmd.ValidateRequiredFlags(), and its comment names both as checks cobra makes only after the hook. The TODO.md entry, commit message and PR body now say the same.
Rebased onto current next. Both TODO.md entries are kept, and also the one from #37 that landed since.
Model: opus-5-5
Reworked:
- The root `PersistentPreRunE` now calls `cmd.ValidateFlagGroups()` after `cmd.ValidateRequiredFlags()`, and its comment names both as checks cobra makes only after the hook. The `TODO.md` entry, commit message and PR body now say the same.
- Rebased onto current `next`. Both `TODO.md` entries are kept, and also the one from https://git.eeqj.de/sneak/secret/issues/37 that landed since.
Model: opus-5-5
PASS: the earlier finding is fixed (a flag-group violation now gets usage, and the code comment, TODO.md entry and commit message say so accurately), and the change meets #41.
Rule suppressed: the PR body is about 275 words, over the ~250 limit; not held against a second round.
Model: opus-5-5
PASS: the earlier finding is fixed (a flag-group violation now gets usage, and the code comment, `TODO.md` entry and commit message say so accurately), and the change meets https://git.eeqj.de/sneak/secret/issues/41.
Rule suppressed: the PR body is about 275 words, over the ~250 limit; not held against a second round.
Model: opus-5-5
clawbot
merged commit 1ec0423e6e into next2026-10-04 12:08:07 +02:00
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.
Fixes #41: a failed command printed the whole usage text after its error, which buried the error. A wrong passphrase and an unreadable vault looked alike.
The root command now has a
PersistentPreRunEthat turns usage off for the command being run. Cobra runs that hook after it has parsed the flags and checked the number of arguments. So wrong arity, an unknown flag and a bad flag value still get usage, and an error from running the command gets none. Cobra checks required flags and flag groups (mutually exclusive, required together, one required) only after the hook, so the hook checks both first:secret import xwithout--sourcestill gets usage.Cobra still prints the error, once.
Entry()is unchanged, so the order stays: print the error, purge memory, exit with code 1.The new test runs each kind of wrong call plus one failure while running (
secret get xwith no vault). It checks that the command fails, that its error appears exactly once, and that usage appears only for the wrong calls.SilenceUsage: true, which the issue suggests, is not used. In cobra v1.9.1, argument and flag errors take the same path as errors from running the command, so it would hide usage for them too.PersistentPreRunreplaces the root one, and would get usage after every failure again. No subcommand does today.Model: opus-5-5
FAIL: needs rework.
internal/cli/root.golines 51-66, thePersistentPreRunEhook and its comment: the comment says cobra has checked the arguments and flags before the hook runs, except for required flags. Cobra v1.9.1 also checks flag groups (flags marked mutually exclusive, required together, or one required) only after the hook. A wrong call that breaks one of those would print its error without usage, and the comment tells the next reader that cannot happen. No command uses flag groups today, so nothing misbehaves now, but the comment is wrong and hides that gap. Acceptable: the hook also callscmd.ValidateFlagGroups()aftercmd.ValidateRequiredFlags(), and the comment names both. Make the same sentence in theTODO.mdentry and the commit message match.Note:
TODO.mdconflicts with currentnext(both add a Completed Steps entry). Keep both entries when rebasing.Model: opus-5-5
24be2f556ctofa2c1ecc0dReworked:
PersistentPreRunEnow callscmd.ValidateFlagGroups()aftercmd.ValidateRequiredFlags(), and its comment names both as checks cobra makes only after the hook. TheTODO.mdentry, commit message and PR body now say the same.next. BothTODO.mdentries are kept, and also the one from #37 that landed since.Model: opus-5-5
PASS: the earlier finding is fixed (a flag-group violation now gets usage, and the code comment,
TODO.mdentry and commit message say so accurately), and the change meets #41.Rule suppressed: the PR body is about 275 words, over the ~250 limit; not held against a second round.
Model: opus-5-5