From 49f26e55d3cdb22c7aa69db2e1298713085def01 Mon Sep 17 00:00:00 2001 From: Behroz Saadat Date: Thu, 11 Jun 2026 16:54:02 -0400 Subject: [PATCH 1/2] cli: collapse --tag provided/value signals into one in browsers update tagsFromFlag now reports whether the --tag flag was provided (independent of whether its values parsed), so runBrowsersUpdate no longer rebuilds that signal via cmd.Flags().Changed("tag"). Renames BrowsersUpdateInput.SetTags to TagsProvided, sourced from the helper, and updates the three other call sites to discard the new bool. Behavior is unchanged and the name side is left as-is. Non-blocking follow-up to PR #184. Co-Authored-By: Claude Opus 4.8 --- cmd/browser_pools.go | 2 +- cmd/browsers.go | 30 +++++++++++----------- cmd/browsers_test.go | 60 +++++++++++++++++++++++++++++++------------- 3 files changed, 59 insertions(+), 33 deletions(-) diff --git a/cmd/browser_pools.go b/cmd/browser_pools.go index 5afe8bc..acd8287 100644 --- a/cmd/browser_pools.go +++ b/cmd/browser_pools.go @@ -742,7 +742,7 @@ func runBrowserPoolsAcquire(cmd *cobra.Command, args []string) error { client := getKernelClient(cmd) timeout, _ := cmd.Flags().GetInt64("timeout") name, _ := cmd.Flags().GetString("name") - tags := tagsFromFlag(cmd, "tag") + tags, _ := tagsFromFlag(cmd, "tag") output, _ := cmd.Flags().GetString("output") c := BrowserPoolsCmd{client: &client.BrowserPools} return c.Acquire(cmd.Context(), BrowserPoolsAcquireInput{ diff --git a/cmd/browsers.go b/cmd/browsers.go index fde9229..f894139 100644 --- a/cmd/browsers.go +++ b/cmd/browsers.go @@ -216,20 +216,22 @@ func parseKeyValueSpecs(specs []string) (map[string]string, []string) { } // tagsFromFlag reads a repeated KEY=VALUE flag and parses it into a map, -// warning about any malformed entries. Returns nil when no valid tags are set. -func tagsFromFlag(cmd *cobra.Command, flagName string) map[string]string { +// warning about any malformed entries. It returns the parsed tags (nil when no +// valid pairs are set) and whether the flag was provided at least once, +// independent of whether its values parsed. +func tagsFromFlag(cmd *cobra.Command, flagName string) (map[string]string, bool) { specs, _ := cmd.Flags().GetStringArray(flagName) if len(specs) == 0 { - return nil + return nil, false } tags, malformed := parseKeyValueSpecs(specs) for _, invalid := range malformed { pterm.Warning.Printf("Ignoring malformed tag: %s\n", invalid) } if len(tags) == 0 { - return nil + return nil, true // provided, but all values malformed } - return tags + return tags, true } // formatTags renders tags as a deterministic "k=v, k2=v2" string with keys @@ -302,7 +304,7 @@ type BrowsersUpdateInput struct { SetName bool ClearName bool Tags map[string]string - SetTags bool + TagsProvided bool ClearTags bool Output string } @@ -708,16 +710,16 @@ func (b BrowsersCmd) Update(ctx context.Context, in BrowsersUpdateInput) error { return fmt.Errorf("cannot specify both --name and --clear-name") } - // Cannot specify both --tag and --clear-tags. Use SetTags (the raw flag - // signal) as well as the parsed map so the check still fires when every + // Cannot specify both --tag and --clear-tags. TagsProvided (whether the flag + // was passed) is the authoritative signal so the check still fires when every // --tag value was malformed and dropped to an empty map. - if (in.SetTags || len(in.Tags) > 0) && in.ClearTags { + if (in.TagsProvided || len(in.Tags) > 0) && in.ClearTags { return fmt.Errorf("cannot specify both --tag and --clear-tags") } // --tag was provided but parsed to zero valid pairs (every value malformed). // Treat as a user error rather than silently leaving tags unchanged. - if in.SetTags && len(in.Tags) == 0 { + if in.TagsProvided && len(in.Tags) == 0 { return fmt.Errorf("no valid --tag KEY=VALUE pairs provided") } @@ -2761,7 +2763,7 @@ func runBrowsersList(cmd *cobra.Command, args []string) error { limit, _ := cmd.Flags().GetInt("limit") offset, _ := cmd.Flags().GetInt("offset") query, _ := cmd.Flags().GetString("query") - tags := tagsFromFlag(cmd, "tag") + tags, _ := tagsFromFlag(cmd, "tag") return b.List(cmd.Context(), BrowsersListInput{ Output: out, IncludeDeleted: includeDeleted, @@ -2814,7 +2816,7 @@ func runBrowsersCreate(cmd *cobra.Command, args []string) error { poolName, _ := cmd.Flags().GetString("pool-name") telemetry, _ := cmd.Flags().GetString("telemetry") name, _ := cmd.Flags().GetString("name") - tags := tagsFromFlag(cmd, "tag") + tags, _ := tagsFromFlag(cmd, "tag") chromePolicy, _ := cmd.Flags().GetString("chrome-policy") chromePolicyFile, _ := cmd.Flags().GetString("chrome-policy-file") output, _ := cmd.Flags().GetString("output") @@ -2986,7 +2988,7 @@ func runBrowsersUpdate(cmd *cobra.Command, args []string) error { telemetry, _ := cmd.Flags().GetString("telemetry") name, _ := cmd.Flags().GetString("name") clearName, _ := cmd.Flags().GetBool("clear-name") - tags := tagsFromFlag(cmd, "tag") + tags, tagsProvided := tagsFromFlag(cmd, "tag") clearTags, _ := cmd.Flags().GetBool("clear-tags") svc := client.Browsers @@ -3005,7 +3007,7 @@ func runBrowsersUpdate(cmd *cobra.Command, args []string) error { SetName: cmd.Flags().Changed("name"), ClearName: clearName, Tags: tags, - SetTags: cmd.Flags().Changed("tag"), + TagsProvided: tagsProvided, ClearTags: clearTags, Output: out, }) diff --git a/cmd/browsers_test.go b/cmd/browsers_test.go index aecf979..4407da4 100644 --- a/cmd/browsers_test.go +++ b/cmd/browsers_test.go @@ -699,12 +699,36 @@ func TestTagsFromFlag_WarnsOnMalformed(t *testing.T) { cmd := &cobra.Command{} cmd.Flags().StringArray("tag", []string{"team=backend", "oops", "env=staging"}, "") - tags := tagsFromFlag(cmd, "tag") + tags, provided := tagsFromFlag(cmd, "tag") + assert.True(t, provided) assert.Equal(t, map[string]string{"team": "backend", "env": "staging"}, tags) assert.Contains(t, outBuf.String(), "Ignoring malformed tag: oops") } +func TestTagsFromFlag_NotProvided_ReportsFalse(t *testing.T) { + cmd := &cobra.Command{} + cmd.Flags().StringArray("tag", nil, "") + + tags, provided := tagsFromFlag(cmd, "tag") + + assert.False(t, provided) + assert.Nil(t, tags) +} + +func TestTagsFromFlag_AllMalformed_ReportsProvidedWithNoTags(t *testing.T) { + setupStdoutCapture(t) + + cmd := &cobra.Command{} + cmd.Flags().StringArray("tag", []string{"foo"}, "") + + tags, provided := tagsFromFlag(cmd, "tag") + + assert.True(t, provided) + assert.Empty(t, tags) + assert.Contains(t, outBuf.String(), "Ignoring malformed tag: foo") +} + func TestBrowsersGet_JSONOutput_IncludesNameAndTags(t *testing.T) { setupStdoutCapture(t) oldStdout := os.Stdout @@ -2239,12 +2263,12 @@ func TestBrowsersUpdate_NameAndTagsWithProxy_AllForwarded(t *testing.T) { b := BrowsersCmd{browsers: fake} err := b.Update(context.Background(), BrowsersUpdateInput{ - Identifier: "session123", - ProxyID: "proxy-123", - Name: "combo", - SetName: true, - Tags: map[string]string{"k": "v"}, - SetTags: true, + Identifier: "session123", + ProxyID: "proxy-123", + Name: "combo", + SetName: true, + Tags: map[string]string{"k": "v"}, + TagsProvided: true, }) assert.NoError(t, err) @@ -2280,10 +2304,10 @@ func TestBrowsersUpdate_ClearNameWithSetTags_BothForwarded(t *testing.T) { b := BrowsersCmd{browsers: fake} err := b.Update(context.Background(), BrowsersUpdateInput{ - Identifier: "session123", - ClearName: true, - Tags: map[string]string{"env": "prod"}, - SetTags: true, + Identifier: "session123", + ClearName: true, + Tags: map[string]string{"env": "prod"}, + TagsProvided: true, }) assert.NoError(t, err) @@ -2320,10 +2344,10 @@ func TestBrowsersUpdate_MalformedTagWithClearTags_Errors(t *testing.T) { b := BrowsersCmd{browsers: fake} err := b.Update(context.Background(), BrowsersUpdateInput{ - Identifier: "session123", - SetTags: true, // --tag was provided... - Tags: nil, // ...but every value was malformed and dropped - ClearTags: true, + Identifier: "session123", + TagsProvided: true, // --tag was provided... + Tags: nil, // ...but every value was malformed and dropped + ClearTags: true, }) assert.Error(t, err) @@ -2338,9 +2362,9 @@ func TestBrowsersUpdate_AllMalformedTags_Errors(t *testing.T) { b := BrowsersCmd{browsers: fake} err := b.Update(context.Background(), BrowsersUpdateInput{ - Identifier: "session123", - SetTags: true, - Tags: nil, + Identifier: "session123", + TagsProvided: true, + Tags: nil, }) assert.Error(t, err) From 282203acc9a8ca70fec049af2106dfd8c959038b Mon Sep 17 00:00:00 2001 From: Behroz Saadat Date: Thu, 11 Jun 2026 17:09:54 -0400 Subject: [PATCH 2/2] cli: detect empty --tag= as provided (fix tagsFromFlag regression) An empty `--tag=` leaves pflag's StringArray slice empty but still marks the flag Changed, so deriving "provided" from len(specs) > 0 regressed validation: `--tag= --clear-tags` silently cleared tags and a lone `--tag=` fell through to the generic "must specify at least one" error instead of being rejected. Source the provided signal from cmd.Flags().Changed (matching pre-refactor behavior), and exercise tagsFromFlag through real flag parsing plus an empty-value regression test instead of default-value injection. Co-Authored-By: Claude Opus 4.8 --- cmd/browsers.go | 15 ++++++++------- cmd/browsers_test.go | 37 +++++++++++++++++++++++++++++++------ 2 files changed, 39 insertions(+), 13 deletions(-) diff --git a/cmd/browsers.go b/cmd/browsers.go index f894139..8d545be 100644 --- a/cmd/browsers.go +++ b/cmd/browsers.go @@ -217,21 +217,22 @@ func parseKeyValueSpecs(specs []string) (map[string]string, []string) { // tagsFromFlag reads a repeated KEY=VALUE flag and parses it into a map, // warning about any malformed entries. It returns the parsed tags (nil when no -// valid pairs are set) and whether the flag was provided at least once, -// independent of whether its values parsed. +// valid pairs are set) and whether the flag was provided on the command line. +// +// "Provided" keys off Changed, not len(specs): pflag records an empty `--tag=` +// as Changed-but-empty, and that must still count as provided so the update +// path rejects it (or `--tag= --clear-tags`) instead of silently ignoring it. func tagsFromFlag(cmd *cobra.Command, flagName string) (map[string]string, bool) { + provided := cmd.Flags().Changed(flagName) specs, _ := cmd.Flags().GetStringArray(flagName) - if len(specs) == 0 { - return nil, false - } tags, malformed := parseKeyValueSpecs(specs) for _, invalid := range malformed { pterm.Warning.Printf("Ignoring malformed tag: %s\n", invalid) } if len(tags) == 0 { - return nil, true // provided, but all values malformed + return nil, provided } - return tags, true + return tags, provided } // formatTags renders tags as a deterministic "k=v, k2=v2" string with keys diff --git a/cmd/browsers_test.go b/cmd/browsers_test.go index 4407da4..bf29b82 100644 --- a/cmd/browsers_test.go +++ b/cmd/browsers_test.go @@ -693,11 +693,22 @@ func TestParseKeyValueSpecs(t *testing.T) { assert.Equal(t, []string{"bad", "=novalue"}, malformed) } +// tagsCmdWithArgs builds a command with a StringArray --tag flag and parses the +// given args through pflag, so cmd.Flags().Changed("tag") reflects real +// command-line usage (the default-value injection pattern would leave Changed +// false and not exercise the provided signal). +func tagsCmdWithArgs(t *testing.T, args ...string) *cobra.Command { + t.Helper() + cmd := &cobra.Command{} + cmd.Flags().StringArray("tag", nil, "") + require.NoError(t, cmd.Flags().Parse(args)) + return cmd +} + func TestTagsFromFlag_WarnsOnMalformed(t *testing.T) { setupStdoutCapture(t) - cmd := &cobra.Command{} - cmd.Flags().StringArray("tag", []string{"team=backend", "oops", "env=staging"}, "") + cmd := tagsCmdWithArgs(t, "--tag=team=backend", "--tag=oops", "--tag=env=staging") tags, provided := tagsFromFlag(cmd, "tag") @@ -707,8 +718,7 @@ func TestTagsFromFlag_WarnsOnMalformed(t *testing.T) { } func TestTagsFromFlag_NotProvided_ReportsFalse(t *testing.T) { - cmd := &cobra.Command{} - cmd.Flags().StringArray("tag", nil, "") + cmd := tagsCmdWithArgs(t) tags, provided := tagsFromFlag(cmd, "tag") @@ -719,8 +729,7 @@ func TestTagsFromFlag_NotProvided_ReportsFalse(t *testing.T) { func TestTagsFromFlag_AllMalformed_ReportsProvidedWithNoTags(t *testing.T) { setupStdoutCapture(t) - cmd := &cobra.Command{} - cmd.Flags().StringArray("tag", []string{"foo"}, "") + cmd := tagsCmdWithArgs(t, "--tag=foo") tags, provided := tagsFromFlag(cmd, "tag") @@ -729,6 +738,22 @@ func TestTagsFromFlag_AllMalformed_ReportsProvidedWithNoTags(t *testing.T) { assert.Contains(t, outBuf.String(), "Ignoring malformed tag: foo") } +// Regression (PR #186 review): an empty `--tag=` leaves pflag's StringArray +// slice empty while marking the flag Changed. tagsFromFlag must report it as +// provided so the update path rejects a lone `--tag=` and `--tag= --clear-tags` +// instead of silently ignoring them — matching the prior Changed("tag") signal. +func TestTagsFromFlag_EmptyValue_ReportsProvided(t *testing.T) { + setupStdoutCapture(t) + + cmd := tagsCmdWithArgs(t, "--tag=") + + tags, provided := tagsFromFlag(cmd, "tag") + + assert.True(t, provided) + assert.Empty(t, tags) + assert.NotContains(t, outBuf.String(), "Ignoring malformed tag") +} + func TestBrowsersGet_JSONOutput_IncludesNameAndTags(t *testing.T) { setupStdoutCapture(t) oldStdout := os.Stdout