From 4e729fb66fc47a74f6281eac94af8d7393257658 Mon Sep 17 00:00:00 2001 From: JonJagger Date: Mon, 27 Jul 2026 16:22:44 +0100 Subject: [PATCH 1/4] fix: accept boolean flag values written with a space "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 --- cmd/kosli/attestGeneric_test.go | 40 +++--- cmd/kosli/main.go | 6 + cmd/kosli/multiHost.go | 7 ++ cmd/kosli/multiHost_test.go | 24 ++++ cmd/kosli/normalizeBoolFlagArgs.go | 64 ++++++++++ cmd/kosli/normalizeBoolFlagArgs_test.go | 155 ++++++++++++++++++++++++ cmd/kosli/root_test.go | 36 ++++++ cmd/kosli/testHelpers.go | 2 +- 8 files changed, 314 insertions(+), 20 deletions(-) create mode 100644 cmd/kosli/normalizeBoolFlagArgs.go create mode 100644 cmd/kosli/normalizeBoolFlagArgs_test.go diff --git a/cmd/kosli/attestGeneric_test.go b/cmd/kosli/attestGeneric_test.go index 8daf389c9..3e9f16f4e 100644 --- a/cmd/kosli/attestGeneric_test.go +++ b/cmd/kosli/attestGeneric_test.go @@ -42,10 +42,24 @@ func (suite *AttestGenericCommandTestSuite) TestAttestGenericCmd() { golden: "Error: accepts at most 1 arg(s), received 2 [foo bar]\n", }, { - wantError: true, - name: "fails when artifact-name is provided and there is an --artifact-type flag and --compliant is not set with =", - cmd: fmt.Sprintf("attest generic testdata/file1 %s --artifact-type file --compliant false", suite.defaultKosliArguments), - golden: "Error: accepts at most 1 arg(s), received 2 [testdata/file1 false]\nSee https://docs.kosli.com//faq/#boolean-flags\n", + name: "reports a non-compliant attestation when --compliant is given with a space", + cmd: fmt.Sprintf("attest generic testdata/file1 --artifact-type file --name foo --commit HEAD --origin-url http://example.com --compliant false %s", suite.defaultKosliArguments), + golden: "generic attestation 'foo' is reported to trail: test-123\n", + }, + { + name: "passes through a non-boolean flag whose value is the literal false", + cmd: fmt.Sprintf("attest generic testdata/file1 --artifact-type file --name false --commit HEAD --origin-url http://example.com %s", suite.defaultKosliArguments), + golden: "generic attestation 'false' is reported to trail: test-123\n", + }, + { + name: "reports a non-compliant attestation when the -C shorthand is given with a space", + cmd: fmt.Sprintf("attest generic testdata/file1 --artifact-type file --name foo --commit HEAD --origin-url http://example.com -C false %s", suite.defaultKosliArguments), + golden: "generic attestation 'foo' is reported to trail: test-123\n", + }, + { + name: "reports a non-compliant attestation when --compliant is given with =", + cmd: fmt.Sprintf("attest generic testdata/file1 --artifact-type file --name foo --commit HEAD --origin-url http://example.com --compliant=false %s", suite.defaultKosliArguments), + golden: "generic attestation 'foo' is reported to trail: test-123\n", }, { wantError: true, @@ -55,9 +69,9 @@ func (suite *AttestGenericCommandTestSuite) TestAttestGenericCmd() { }, { wantError: true, - name: "fails when artifact-name is provided (as _unused_ boolean 'space' arg) and there is no --artifact-type and no --fingerprint", - cmd: fmt.Sprintf("attest generic %s --compliant false", suite.defaultKosliArguments), - golden: "Error: --artifact-type or --fingerprint must be specified when artifact name ('false') argument is supplied.\nSee https://docs.kosli.com//faq/#boolean-flags\nUsage: kosli attest generic [IMAGE-NAME | FILE-PATH | DIR-PATH] [flags]\n", + name: "links to the boolean flags FAQ when a stray true|false argument remains", + cmd: fmt.Sprintf("attest generic foo false --artifact-type file --name bar %s", suite.defaultKosliArguments), + golden: "Error: accepts at most 1 arg(s), received 2 [foo false]\nSee https://docs.kosli.com//faq/#boolean-flags\n", }, { wantError: true, @@ -65,18 +79,6 @@ func (suite *AttestGenericCommandTestSuite) TestAttestGenericCmd() { cmd: fmt.Sprintf("attest generic wibble %s", suite.defaultKosliArguments), golden: "Error: --artifact-type or --fingerprint must be specified when artifact name ('wibble') argument is supplied.\nUsage: kosli attest generic [IMAGE-NAME | FILE-PATH | DIR-PATH] [flags]\n", }, - { - wantError: true, - name: "fails when there are extra args and gives custom help message when an argument is true|false", - cmd: fmt.Sprintf("attest generic foo -t file %s --compliant false", suite.defaultKosliArguments), - golden: "Error: accepts at most 1 arg(s), received 2 [foo false]\nSee https://docs.kosli.com//faq/#boolean-flags\n", - }, - { - wantError: true, - name: "fails when there are extra args and gives custom help message when an argument is true|false", - cmd: fmt.Sprintf("attest generic %s --compliant false", suite.defaultKosliArguments), - golden: "Error: --artifact-type or --fingerprint must be specified when artifact name ('false') argument is supplied.\nSee https://docs.kosli.com//faq/#boolean-flags\nUsage: kosli attest generic [IMAGE-NAME | FILE-PATH | DIR-PATH] [flags]\n", - }, { wantError: true, name: "fails when both --fingerprint and --artifact-type", diff --git a/cmd/kosli/main.go b/cmd/kosli/main.go index 1b49ec976..09174a360 100644 --- a/cmd/kosli/main.go +++ b/cmd/kosli/main.go @@ -61,7 +61,13 @@ func enrichError(cmd *cobra.Command, err error) error { return fmt.Errorf("[%s] %w", strings.Join(parts, " "), err) } +// innerMain runs cmd against args (args[0] being the program name) and turns +// the outcome into the process-level error, printing the update notice on the +// --version path and reporting errors in the friendliest available form. func innerMain(cmd *cobra.Command, args []string) error { + if len(args) > 1 { + cmd.SetArgs(normalizeBoolFlagArgs(cmd, args[1:])) + } executedCmd, err := cmd.ExecuteC() if err == nil { // Cobra handles --version internally and bypasses all hooks, so we print diff --git a/cmd/kosli/multiHost.go b/cmd/kosli/multiHost.go index 977461f3e..1036dd060 100644 --- a/cmd/kosli/multiHost.go +++ b/cmd/kosli/multiHost.go @@ -161,6 +161,13 @@ func getMultiOpts() MultiOpts { cmd.SetOut(writer) cmd.SetErr(writer) + // Parse the same normalized args innerMain will run with, so a boolean flag + // written in the space form (eg --debug false) does not leave a stray + // positional argument that fails arg validation and empties MultiOpts. + if len(os.Args) > 1 { + cmd.SetArgs(normalizeBoolFlagArgs(cmd, os.Args[1:])) + } + fakeError := errors.New("") cmd.PersistentPreRunE = func(cmd *cobra.Command, args []string) error { diff --git a/cmd/kosli/multiHost_test.go b/cmd/kosli/multiHost_test.go index 383c00a07..bea710b27 100644 --- a/cmd/kosli/multiHost_test.go +++ b/cmd/kosli/multiHost_test.go @@ -134,6 +134,30 @@ func (suite *MultiHostTestSuite) TestRunDoubledHost() { } } +// TestRunDoubledHostAcceptsSpaceSeparatedBoolFlag checks that a boolean flag +// written in the space form is honoured on the multi-host path too. --debug is +// false here, so the output must be the bare fingerprint: no per-host [debug] +// prefix and no secondary-call output. fingerprint is used because it needs no +// server, so the two hosts are never contacted. +func (suite *MultiHostTestSuite) TestRunDoubledHostAcceptsSpaceSeparatedBoolFlag() { + args := []string{ + "kosli", "fingerprint", "testdata/person-schema.json", + "--artifact-type", "file", + "--debug", "false", + fmt.Sprintf("--host=%s,%s", localHost, localHost), + fmt.Sprintf("--api-token=%s,%s", apiToken, apiToken), + fmt.Sprintf("--org=%s", orgName), + } + want := []string{"1bef738d0bb1e690500f99a5b57d958caf3a5eb3e00d9012e1f4369fc6812e01", ""} + + defer func(original []string) { os.Args = original }(os.Args) + os.Args = args + output, err := runMultiHost(args) + + assert.Equal(suite.T(), error(nil), err) + assert.Equal(suite.T(), "", diff(want, strings.Split(output, "\n"))) +} + func (suite *MultiHostTestSuite) TestRunTripledHost() { multiHost := fmt.Sprintf("--host=%s,%s,%s", localHost, localHost, localHost) diff --git a/cmd/kosli/normalizeBoolFlagArgs.go b/cmd/kosli/normalizeBoolFlagArgs.go new file mode 100644 index 000000000..4f3ceac2f --- /dev/null +++ b/cmd/kosli/normalizeBoolFlagArgs.go @@ -0,0 +1,64 @@ +package main + +import ( + "github.com/spf13/cobra" + "github.com/spf13/pflag" +) + +// normalizeBoolFlagArgs rewrites boolean flags written in the space form +// ("--compliant false") into the "=" form ("--compliant=false"), which is the +// only form pflag accepts for an explicitly-valued boolean flag. All other +// tokens are returned unchanged, as is everything following a "--" terminator. +func normalizeBoolFlagArgs(root *cobra.Command, args []string) []string { + cmd, _, err := root.Find(args) + if err != nil { + return args + } + boolFlags := boolFlagTokens(cmd) + normalized := make([]string, 0, len(args)) + for i := 0; i < len(args); i++ { + if args[i] == "--" { + // pflag stops parsing flags here, so everything left is a + // positional argument and must be passed through untouched. + normalized = append(normalized, args[i:]...) + break + } + if boolFlags[args[i]] && i+1 < len(args) && isBoolLiteral(args[i+1]) { + normalized = append(normalized, args[i]+"="+args[i+1]) + i++ + continue + } + normalized = append(normalized, args[i]) + } + return normalized +} + +// boolFlagTokens returns the command-line tokens ("--compliant", "-C") of every +// boolean flag known to cmd. +// +// Shorthands are listed individually, so a grouped token such as "-qC" is not a +// key here and "-qC false" is left alone: it keeps failing with the arg-count +// error and its link to the boolean flags FAQ. That is a deliberate choice. +// Grouping raises questions this rewrite has no good answer to, such as which +// member of the group the value belongs to and what to do when a group mixes +// boolean and non-boolean shorthands. Rescuing the plain forms is worth a +// low-level rewrite; guessing intent inside a grouped token is not. +func boolFlagTokens(cmd *cobra.Command) map[string]bool { + tokens := map[string]bool{} + cmd.Flags().VisitAll(func(flag *pflag.Flag) { + if flag.Value.Type() != "bool" { + return + } + tokens["--"+flag.Name] = true + if flag.Shorthand != "" { + tokens["-"+flag.Shorthand] = true + } + }) + return tokens +} + +// isBoolLiteral reports whether token is one of the two values a rewritten +// boolean flag may be given. +func isBoolLiteral(token string) bool { + return token == "true" || token == "false" +} diff --git a/cmd/kosli/normalizeBoolFlagArgs_test.go b/cmd/kosli/normalizeBoolFlagArgs_test.go new file mode 100644 index 000000000..99bda8182 --- /dev/null +++ b/cmd/kosli/normalizeBoolFlagArgs_test.go @@ -0,0 +1,155 @@ +package main + +import ( + "io" + "testing" + + "github.com/stretchr/testify/require" +) + +// TestNormalizeBoolFlagArgsLeavesTrailingBareBoolFlagAlone covers the boundary +// where a boolean flag is the final token, so there is no following token to +// consider joining. The bare form keeps its NoOptDefVal of true. +func TestNormalizeBoolFlagArgsLeavesTrailingBareBoolFlagAlone(t *testing.T) { + args := []string{ + "fingerprint", "testdata/person-schema.json", + "--artifact-type", "file", + "--debug", + } + root, err := newRootCmd(io.Discard, io.Discard, args) + require.NoError(t, err) + + normalized := normalizeBoolFlagArgs(root, args) + + require.Equal(t, args, normalized) +} + +// TestNormalizeBoolFlagArgsLeavesGroupedShorthandsAlone pins the deliberate +// decision not to rewrite a grouped shorthand token, so that "-qC false" keeps +// failing with the arg-count error and its link to the boolean flags FAQ. See +// boolFlagTokens for why. This test exists so that widening the rewrite to +// cover groups is a visible choice rather than a silent one. +func TestNormalizeBoolFlagArgsLeavesGroupedShorthandsAlone(t *testing.T) { + args := []string{ + "attest", "generic", "testdata/file1", + "--artifact-type", "file", + "-qC", "false", + "--name", "foo", + } + root, err := newRootCmd(io.Discard, io.Discard, args) + require.NoError(t, err) + + normalized := normalizeBoolFlagArgs(root, args) + + require.Equal(t, args, normalized) +} + +// TestNormalizeBoolFlagArgsStopsAtTerminator checks that nothing after the "--" +// terminator is rewritten. pflag stops parsing flags there, so every later +// token is a positional argument, however much it looks like a flag. Rewriting +// past the terminator would alter arguments the flag parser never inspects. +func TestNormalizeBoolFlagArgsStopsAtTerminator(t *testing.T) { + args := []string{ + "fingerprint", "--artifact-type", "file", + "--", "--debug", "false", + } + root, err := newRootCmd(io.Discard, io.Discard, args) + require.NoError(t, err) + + normalized := normalizeBoolFlagArgs(root, args) + + require.Equal(t, args, normalized) +} + +// TestNormalizeBoolFlagArgsLeavesUndocumentedBoolLiteralsAlone pins the +// deliberate restriction to the two literals Kosli documents. pflag's bool +// values go through strconv.ParseBool, so the "=" form also accepts 1, 0, t, f, +// TRUE, False and friends, but the space form is rewritten only for "true" and +// "false". Widening this would capture positionals named "0" or "1", which are +// far more plausible artifact names than "true", and would mean guessing intent +// on input no Kosli documentation describes. The undocumented forms therefore +// keep failing loudly instead of being interpreted. +func TestNormalizeBoolFlagArgsLeavesUndocumentedBoolLiteralsAlone(t *testing.T) { + for _, literal := range []string{"TRUE", "True", "FALSE", "False", "1", "0", "t", "f"} { + t.Run(literal, func(t *testing.T) { + args := []string{ + "attest", "generic", "testdata/file1", + "--artifact-type", "file", + "--compliant", literal, + "--name", "foo", + } + root, err := newRootCmd(io.Discard, io.Discard, args) + require.NoError(t, err) + + normalized := normalizeBoolFlagArgs(root, args) + + require.Equal(t, args, normalized) + }) + } +} + +// TestNormalizeBoolFlagArgsCapturesPositionalNamedTrue pins the one accepted +// ambiguity of the rewrite: a positional argument whose literal value is "true" +// or "false" immediately after a bare boolean flag is taken as that flag's +// value. Here the artifact being fingerprinted is a file named "true", and it +// is swallowed by --debug. This is deliberate: Kosli positionals are artifact +// names, fingerprints and file paths, for which such a name is pathological, +// and distinguishing the two cases would mean predicting each command's +// expected argument count. +func TestNormalizeBoolFlagArgsCapturesPositionalNamedTrue(t *testing.T) { + args := []string{"fingerprint", "--debug", "true", "--artifact-type", "file"} + root, err := newRootCmd(io.Discard, io.Discard, args) + require.NoError(t, err) + + normalized := normalizeBoolFlagArgs(root, args) + + require.Equal(t, []string{"fingerprint", "--debug=true", "--artifact-type", "file"}, normalized) +} + +// TestNormalizeBoolFlagArgsJoinsInheritedBoolFlag checks that a boolean flag +// inherited from a parent command is rewritten too, not just one declared on +// the subcommand itself. +func TestNormalizeBoolFlagArgsJoinsInheritedBoolFlag(t *testing.T) { + args := []string{ + "attest", "generic", "Dockerfile", + "--artifact-type", "file", + "--debug", "false", + "--flow", "my-flow", + } + root, err := newRootCmd(io.Discard, io.Discard, args) + require.NoError(t, err) + + normalized := normalizeBoolFlagArgs(root, args) + + require.Equal(t, []string{ + "attest", "generic", "Dockerfile", + "--artifact-type", "file", + "--debug=false", + "--flow", "my-flow", + }, normalized) +} + +// TestNormalizeBoolFlagArgsJoinsSpaceSeparatedBoolValue checks that a boolean +// flag written in the space form is rewritten into the "=" form, so that +// `--compliant false` stops leaving "false" behind as a positional argument. +func TestNormalizeBoolFlagArgsJoinsSpaceSeparatedBoolValue(t *testing.T) { + args := []string{ + "attest", "generic", "Dockerfile", + "--artifact-type", "file", + "--compliant", "false", + "--flow", "my-flow", + "--trail", "my-trail", + } + root, err := newRootCmd(io.Discard, io.Discard, args) + require.NoError(t, err) + + normalized := normalizeBoolFlagArgs(root, args) + + require.Equal(t, []string{ + "attest", "generic", "Dockerfile", + "--artifact-type", "file", + "--compliant=false", + "--flow", "my-flow", + "--trail", "my-trail", + }, normalized) +} diff --git a/cmd/kosli/root_test.go b/cmd/kosli/root_test.go index 9fd753b6f..38873a0b4 100644 --- a/cmd/kosli/root_test.go +++ b/cmd/kosli/root_test.go @@ -66,6 +66,42 @@ func (suite *RootCommandTestSuite) TestInnerMainEnrichesError() { suite.ErrorContains(err, "[kosli attest snyk flow=cyber-dojo trail=live-snyk-scan]") } +// TestExecuteCommandCNormalizesSpaceSeparatedBoolFlag checks that the golden +// test harness normalizes args the same way innerMain does. Without this the +// harness sets args on the command itself and bypasses normalization, so golden +// tests would assert behaviour that real CLI users no longer get. +func (suite *RootCommandTestSuite) TestExecuteCommandCNormalizesSpaceSeparatedBoolFlag() { + runTestCmd(suite.T(), []cmdTestCase{ + { + name: "a bool flag written in the space form is accepted", + cmd: "fingerprint testdata/person-schema.json --artifact-type file --debug false", + golden: "1bef738d0bb1e690500f99a5b57d958caf3a5eb3e00d9012e1f4369fc6812e01\n", + }, + }) +} + +// TestInnerMainAcceptsSpaceSeparatedBoolFlag drives a command whose bool flag +// is written in the space form ("--debug false") through innerMain, which is +// where the args are normalized. Without that normalization pflag leaves +// "false" behind as a second positional argument and the command fails with +// "accepts 1 arg(s), received 2". Note that this test deliberately does not +// call cmd.SetArgs: setting the normalized args is innerMain's job. +func (suite *RootCommandTestSuite) TestInnerMainAcceptsSpaceSeparatedBoolFlag() { + args := []string{ + "fingerprint", "testdata/person-schema.json", + "--artifact-type", "file", + "--debug", "false", + } + out := new(bytes.Buffer) + cmd, err := newRootCmd(out, io.Discard, args) + suite.Require().NoError(err) + + err = innerMain(cmd, append([]string{"kosli"}, args...)) + + suite.Require().NoError(err) + suite.Equal("1bef738d0bb1e690500f99a5b57d958caf3a5eb3e00d9012e1f4369fc6812e01\n", out.String()) +} + // In order for 'go test' to run this suite, we need to create // a normal test function and pass our suite to suite.Run func TestRootCommandTestSuite(t *testing.T) { diff --git a/cmd/kosli/testHelpers.go b/cmd/kosli/testHelpers.go index 2036b82a3..96b7f05ea 100644 --- a/cmd/kosli/testHelpers.go +++ b/cmd/kosli/testHelpers.go @@ -68,7 +68,7 @@ func executeCommandC(cmd string) (*cobra.Command, string, string, string, error) root.SetOut(outWriter) root.SetErr(errWriter) root.SetIn(new(bytes.Buffer)) - root.SetArgs(args) + root.SetArgs(normalizeBoolFlagArgs(root, args)) c, err := root.ExecuteC() From c3e1f7bf2fd45c19da05a336bc6d2dfdcbc5d7a0 Mon Sep 17 00:00:00 2001 From: JonJagger Date: Mon, 27 Jul 2026 16:47:21 +0100 Subject: [PATCH 2/4] docs: pin the second accepted ambiguity of the bool flag rewrite 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. --- cmd/kosli/normalizeBoolFlagArgs.go | 11 +++++++++++ cmd/kosli/normalizeBoolFlagArgs_test.go | 26 +++++++++++++++++++++++++ 2 files changed, 37 insertions(+) diff --git a/cmd/kosli/normalizeBoolFlagArgs.go b/cmd/kosli/normalizeBoolFlagArgs.go index 4f3ceac2f..d1fcaafa5 100644 --- a/cmd/kosli/normalizeBoolFlagArgs.go +++ b/cmd/kosli/normalizeBoolFlagArgs.go @@ -9,6 +9,17 @@ import ( // ("--compliant false") into the "=" form ("--compliant=false"), which is the // only form pflag accepts for an explicitly-valued boolean flag. All other // tokens are returned unchanged, as is everything following a "--" terminator. +// +// The rewrite is positional: it matches on the tokens themselves and does not +// track which of them pflag will consume as the value of an earlier flag. Two +// pathological inputs are therefore read differently from the way pflag reads +// them, and both are accepted. A positional argument literally named "true" or +// "false" directly after a bare boolean flag becomes that flag's value. And in +// "--name --compliant false", where pflag gives --name the value "--compliant" +// and leaves "false" positional, this rewrite instead produces +// "--compliant=false". Kosli positionals are artifact names, fingerprints and +// file paths, and flag values are not flag tokens, so both inputs are far +// enough outside real usage to be worth the plain forms this rescues. func normalizeBoolFlagArgs(root *cobra.Command, args []string) []string { cmd, _, err := root.Find(args) if err != nil { diff --git a/cmd/kosli/normalizeBoolFlagArgs_test.go b/cmd/kosli/normalizeBoolFlagArgs_test.go index 99bda8182..44268883e 100644 --- a/cmd/kosli/normalizeBoolFlagArgs_test.go +++ b/cmd/kosli/normalizeBoolFlagArgs_test.go @@ -106,6 +106,32 @@ func TestNormalizeBoolFlagArgsCapturesPositionalNamedTrue(t *testing.T) { require.Equal(t, []string{"fingerprint", "--debug=true", "--artifact-type", "file"}, normalized) } +// TestNormalizeBoolFlagArgsCapturesBoolFlagTokenUsedAsFlagValue pins the second +// accepted ambiguity of the rewrite: a boolean flag token given as the value of +// a preceding value-expecting flag is still rewritten. Here pflag would give +// --name the value "--compliant" and leave "false" as a positional argument, +// whereas the rewrite reads the same tokens as a space-form boolean. This is +// deliberate: telling the two apart would mean tracking which tokens pflag +// consumes as values, and a flag value that is itself a flag token is +// pathological. +func TestNormalizeBoolFlagArgsCapturesBoolFlagTokenUsedAsFlagValue(t *testing.T) { + args := []string{ + "attest", "generic", "Dockerfile", + "--artifact-type", "file", + "--name", "--compliant", "false", + } + root, err := newRootCmd(io.Discard, io.Discard, args) + require.NoError(t, err) + + normalized := normalizeBoolFlagArgs(root, args) + + require.Equal(t, []string{ + "attest", "generic", "Dockerfile", + "--artifact-type", "file", + "--name", "--compliant=false", + }, normalized) +} + // TestNormalizeBoolFlagArgsJoinsInheritedBoolFlag checks that a boolean flag // inherited from a parent command is rewritten too, not just one declared on // the subcommand itself. From 672b241c71804a463065fcd312f8ded0529adf13 Mon Sep 17 00:00:00 2001 From: JonJagger Date: Tue, 28 Jul 2026 11:46:29 +0100 Subject: [PATCH 3/4] fix: normalize bool flag values written before the subcommand Marko pointed out on #1037 that the space-form fix only covered flags placed after the subcommand. Written before it, "kosli --debug false list flows" printed the root help and exited 0: the command silently never ran, yet the caller was told it had succeeded. The flags in scope depend on where the token sits, so a single pass cannot cover both positions. root.Find cannot resolve the command while a leading bool flag is still in the space form, because its value is left as a stray positional that cobra reads as an unknown subcommand. Normalizing from root's own flags first is what lets Find succeed, so the second pass can still reach the flags declared on the resolved command. Refs #1037 --- cmd/kosli/normalizeBoolFlagArgs.go | 24 ++++++++++++++++++++---- cmd/kosli/normalizeBoolFlagArgs_test.go | 15 +++++++++++++++ 2 files changed, 35 insertions(+), 4 deletions(-) diff --git a/cmd/kosli/normalizeBoolFlagArgs.go b/cmd/kosli/normalizeBoolFlagArgs.go index d1fcaafa5..dc8e40412 100644 --- a/cmd/kosli/normalizeBoolFlagArgs.go +++ b/cmd/kosli/normalizeBoolFlagArgs.go @@ -20,12 +20,28 @@ import ( // "--compliant=false". Kosli positionals are artifact names, fingerprints and // file paths, and flag values are not flag tokens, so both inputs are far // enough outside real usage to be worth the plain forms this rescues. +// The rewrite runs in two passes because the flags in scope depend on where a +// token sits. A flag before the subcommand can only be one of root's own, and +// root.Find cannot resolve the command while such a flag is still in the space +// form: the value is left as a stray positional, which cobra reads as an unknown +// subcommand. The first pass therefore works from root's flags alone, which is +// what lets the second pass resolve the command and reach the flags declared on +// it. Joining is idempotent, since a joined token no longer matches any flag +// token, so the passes cannot rewrite the same token twice. func normalizeBoolFlagArgs(root *cobra.Command, args []string) []string { + args = joinBoolFlagValues(boolFlagTokens(root.LocalFlags()), args) cmd, _, err := root.Find(args) if err != nil { return args } - boolFlags := boolFlagTokens(cmd) + return joinBoolFlagValues(boolFlagTokens(cmd.Flags()), args) +} + +// joinBoolFlagValues rewrites every " " pair in args, where flag +// is a token in boolFlags and literal is a boolean literal, into the single +// "=" token. All other tokens are returned unchanged, as is +// everything following a "--" terminator. +func joinBoolFlagValues(boolFlags map[string]bool, args []string) []string { normalized := make([]string, 0, len(args)) for i := 0; i < len(args); i++ { if args[i] == "--" { @@ -45,7 +61,7 @@ func normalizeBoolFlagArgs(root *cobra.Command, args []string) []string { } // boolFlagTokens returns the command-line tokens ("--compliant", "-C") of every -// boolean flag known to cmd. +// boolean flag in flags. // // Shorthands are listed individually, so a grouped token such as "-qC" is not a // key here and "-qC false" is left alone: it keeps failing with the arg-count @@ -54,9 +70,9 @@ func normalizeBoolFlagArgs(root *cobra.Command, args []string) []string { // member of the group the value belongs to and what to do when a group mixes // boolean and non-boolean shorthands. Rescuing the plain forms is worth a // low-level rewrite; guessing intent inside a grouped token is not. -func boolFlagTokens(cmd *cobra.Command) map[string]bool { +func boolFlagTokens(flags *pflag.FlagSet) map[string]bool { tokens := map[string]bool{} - cmd.Flags().VisitAll(func(flag *pflag.Flag) { + flags.VisitAll(func(flag *pflag.Flag) { if flag.Value.Type() != "bool" { return } diff --git a/cmd/kosli/normalizeBoolFlagArgs_test.go b/cmd/kosli/normalizeBoolFlagArgs_test.go index 44268883e..1d243d996 100644 --- a/cmd/kosli/normalizeBoolFlagArgs_test.go +++ b/cmd/kosli/normalizeBoolFlagArgs_test.go @@ -155,6 +155,21 @@ func TestNormalizeBoolFlagArgsJoinsInheritedBoolFlag(t *testing.T) { }, normalized) } +// TestNormalizeBoolFlagArgsJoinsGlobalBoolFlagBeforeSubcommand checks that a +// global boolean flag written in the space form before the subcommand is +// rewritten too. Left alone, the stray "false" stops cobra resolving `list +// flows`, and the CLI prints the root help text and exits 0, so the command +// silently never runs. +func TestNormalizeBoolFlagArgsJoinsGlobalBoolFlagBeforeSubcommand(t *testing.T) { + args := []string{"--debug", "false", "list", "flows"} + root, err := newRootCmd(io.Discard, io.Discard, args) + require.NoError(t, err) + + normalized := normalizeBoolFlagArgs(root, args) + + require.Equal(t, []string{"--debug=false", "list", "flows"}, normalized) +} + // TestNormalizeBoolFlagArgsJoinsSpaceSeparatedBoolValue checks that a boolean // flag written in the space form is rewritten into the "=" form, so that // `--compliant false` stops leaving "false" behind as a positional argument. From ce0917e44eb07690100186094995aea6c0ec4c20 Mon Sep 17 00:00:00 2001 From: JonJagger Date: Tue, 28 Jul 2026 12:34:50 +0100 Subject: [PATCH 4/4] docs: drop the boolean-flags FAQ link from two flag descriptions The CLI now accepts boolean flag values written with a space, so the --compliant and --new-compliance-status help text no longer needs to send users to the FAQ to learn the --flag=value form. Pointing at the FAQ from these two flags alone was also inconsistent: none of the other ~23 boolean flags carried the link. --- cmd/kosli/root.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cmd/kosli/root.go b/cmd/kosli/root.go index c3fd77f7e..88d8c0d3b 100644 --- a/cmd/kosli/root.go +++ b/cmd/kosli/root.go @@ -261,7 +261,7 @@ The ^.kosli_ignore^ will be treated as part of the artifact like any other file, attestationRedactCommitInfoFlag = "[optional] The list of commit info to be redacted before sending to Kosli. Allowed values are one or more of [author, message, branch]." attestationOriginUrlFlag = "[optional] The url pointing to where the attestation came from or is related. (defaulted to the CI url in some CIs: https://docs.kosli.com/integrations/ci_cd/#defaulted-kosli-command-flags-from-ci-variables )." attestationNameFlag = "The name of the attestation as declared in the flow or trail yaml template." - attestationCompliantFlag = "[defaulted] Whether the attestation is compliant or not. A boolean flag https://docs.kosli.com/faq/#boolean-flags" + attestationCompliantFlag = "[defaulted] Whether the attestation is compliant or not." attestationRepoRootFlag = "[defaulted] The directory where the source git repository is available. Only used if --commit is used or defaulted in CI, see https://docs.kosli.com/integrations/ci_cd/#defaulted-kosli-command-flags-from-ci-variables ." attestationCustomTypeNameFlag = "The name of the custom attestation type." attestationCustomDataFileFlag = "The filepath of a json file containing the custom attestation data." @@ -275,7 +275,7 @@ The ^.kosli_ignore^ will be treated as part of the artifact like any other file, annotationFlag = "[optional] Annotate the attestation with data using key=value." attestationDescription = "[optional] attestation description" attestationOverrideReasonFlag = "The reason for overriding the attestation." - newComplianceStatusFlag = "The new compliance status to set on the attestation. A boolean flag https://docs.kosli.com/faq/#boolean-flags" + newComplianceStatusFlag = "The new compliance status to set on the attestation." originalAttestationTypeFlag = "The original attestation type being overridden (e.g. generic, snyk, junit, sonar, jira, pull_request, custom)." attestationDecisionControlFlag = "The control identifier being evaluated (e.g. RCTL-043)." excludeScalingFlag = "[optional] Exclude scaling events for snapshots. Snapshots with scaling changes will not result in new environment records."