Skip to content

fix: accept boolean flag values written with a space - #1037

Open
JonJagger wants to merge 2 commits into
mainfrom
fix-boolean-flag-space-separator
Open

fix: accept boolean flag values written with a space#1037
JonJagger wants to merge 2 commits into
mainfrom
fix-boolean-flag-space-separator

Conversation

@JonJagger

Copy link
Copy Markdown
Contributor

"kosli attest generic Dockerfile --compliant false" failed with
"accepts at most 1 arg(s), received 2". pflag's NoOptDefVal makes a
boolean flag never consume the next token, so "false" was left behind
as a positional argument. The bare "--compliant" form and the space
form are gated by that same single field in opposite directions, so no
custom pflag.Value type can fix it, because the consume-or-not decision
is made before Set() is ever called on the value.

Normalize the argument slice before pflag sees it, rewriting the space
form into the "=" form for boolean flags only. Both places that feed
args to cobra do this: innerMain for the normal path, and getMultiOpts
for the multi-host path, where the stray positional previously emptied
MultiOpts and silently downgraded a multi-host call to a single host.

The golden test harness normalizes too, so tests exercise the same args
path as the real CLI instead of asserting the pre-fix behaviour.

Deliberately left alone, each still failing loudly with the link to the
boolean flags FAQ: grouped shorthands such as "-qC false", literals
pflag accepts via "=" but Kosli does not document such as "TRUE" and
"1", and anything following a "--" terminator.

Refs kosli-dev/server#6235

  "kosli attest generic Dockerfile --compliant false" failed with
  "accepts at most 1 arg(s), received 2". pflag's NoOptDefVal makes a
  boolean flag never consume the next token, so "false" was left behind
  as a positional argument. The bare "--compliant" form and the space
  form are gated by that same single field in opposite directions, so no
  custom pflag.Value type can fix it, because the consume-or-not decision
  is made before Set() is ever called on the value.

  Normalize the argument slice before pflag sees it, rewriting the space
  form into the "=" form for boolean flags only. Both places that feed
  args to cobra do this: innerMain for the normal path, and getMultiOpts
  for the multi-host path, where the stray positional previously emptied
  MultiOpts and silently downgraded a multi-host call to a single host.

  The golden test harness normalizes too, so tests exercise the same args
  path as the real CLI instead of asserting the pre-fix behaviour.

  Deliberately left alone, each still failing loudly with the link to the
  boolean flags FAQ: grouped shorthands such as "-qC false", literals
  pflag accepts via "=" but Kosli does not document such as "TRUE" and
  "1", and anything following a "--" terminator.

  Refs kosli-dev/server#6235
@claude

claude Bot commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

Claude finished @JonJagger's task in 2m 28s —— View job


Review: accept boolean flag values written with a space

  • Static analysis (build, vet, mod tidy)
  • Review normalizeBoolFlagArgs.go
  • Review call sites (main.go, multiHost.go)
  • Review tests
  • Post review feedback

Verdict: LGTM. This is a careful, well-reasoned fix. The core insight in the PR description — that pflag's NoOptDefVal makes the consume-or-not decision before Set() is ever called, so no pflag.Value type can fix it — is correct, and normalizing the arg slice before pflag sees it is the right layer to intervene at.

Static analysis

Check Result
go build ./... ✅ pass
go vet ./cmd/kosli/ ✅ pass
go.mod / go.sum ✅ unchanged (no new deps)

What's good

  • root.Find → merged flags. Relying on Find to trigger mergePersistentFlags so inherited flags (--debug, --flow) get rewritten is subtle, but it's pinned by TestNormalizeBoolFlagArgsJoinsInheritedBoolFlag, so a future cobra change can't regress it silently. Good instinct locking that down.
  • Idempotent by construction. Once --compliant false becomes --compliant=false, the token no longer matches boolFlagTokens, so the second pass on the multi-host path (getMultiOpts then innerMain) is a no-op. The double-normalization is safe.
  • The multi-host regression test is stronger than it looks. TestRunDoubledHostAcceptsSpaceSeparatedBoolFlag calls runMultiHost directly, which calls getMultiOpts internally. Without the fix, the stray false positional makes cmd.Execute() return a genuine (non-fake) error, so getMultiOpts returns MultiOpts{} with hosts == nil — and runMultiHost would then panic on opts.hosts[0]. So the test genuinely guards the "silently downgraded to single host" path described in the PR body, not just the happy output.
  • Harness parity. Routing executeCommandC through normalizeBoolFlagArgs (testHelpers.go:71) so golden tests exercise the same args path as the real CLI is the right call — otherwise the tests would assert pre-fix behaviour that no user gets.
  • The accepted ambiguities are pinned, not hidden. Every deliberate non-rewrite (grouped shorthands -qC, undocumented literals TRUE/1, post--- terminator) has a dedicated test asserting the args pass through unchanged, plus doc comments explaining why. The two "captured" ambiguities (positional literally named true, bool-token-as-flag-value) are likewise documented and tested. This is exactly the discipline that keeps a heuristic maintainable.

Observations (non-blocking)

  1. Un-normalized args in the error-recovery path. innerMain executes with normalized args but the unknown-flag recovery branch calls cmd.Traverse(args[1:]) (main.go:99) on the original args. This is harmless — normalization only joins bool tokens and never creates/removes an unknown flag, so it can't change which branch is taken or the resulting message — just noting it so the asymmetry is a known, deliberate one rather than a surprise later.

  2. Prior review points are addressed. The two pathological cases raised in the earlier review — positional named true/false and the --name --compliant false bool-token-as-value case — are now both documented in the normalizeBoolFlagArgs doc comment and pinned by TestNormalizeBoolFlagArgsCapturesPositionalNamedTrue and TestNormalizeBoolFlagArgsCapturesBoolFlagTokenUsedAsFlagValue. The boundary is fully pinned. 👍

No changes requested. Nicely done.
· fix-boolean-flag-space-separator

Comment thread cmd/kosli/normalizeBoolFlagArgs.go
Comment thread cmd/kosli/normalizeBoolFlagArgs.go
  Review of #1037 spotted an input the rewrite reads differently from
  pflag and that nothing recorded: "--name --compliant false". pflag
  gives --name the value "--compliant" and leaves "false" positional,
  whereas the rewrite sees a space-form boolean and joins it.

  Left as is. It sits in the same family as the already-accepted
  positional named "true", and telling the two apart would mean tracking
  which tokens pflag consumes as values, turning a token pre-pass into a
  second parser. What was missing was the boundary being written down,
  so a later reader does not rediscover it as a bug.

  The doc comment now names the rewrite as positional and lists both
  inputs, and a test pins the behaviour the way the sibling ambiguity
  already is.

  Comment and test only, no behaviour change.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant