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..8d545be 100644 --- a/cmd/browsers.go +++ b/cmd/browsers.go @@ -216,20 +216,23 @@ 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 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 - } 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, provided } - return tags + return tags, provided } // formatTags renders tags as a deterministic "k=v, k2=v2" string with keys @@ -302,7 +305,7 @@ type BrowsersUpdateInput struct { SetName bool ClearName bool Tags map[string]string - SetTags bool + TagsProvided bool ClearTags bool Output string } @@ -708,16 +711,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 +2764,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 +2817,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 +2989,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 +3008,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..bf29b82 100644 --- a/cmd/browsers_test.go +++ b/cmd/browsers_test.go @@ -693,18 +693,67 @@ 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 := 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 := tagsCmdWithArgs(t) + + tags, provided := tagsFromFlag(cmd, "tag") + + assert.False(t, provided) + assert.Nil(t, tags) +} + +func TestTagsFromFlag_AllMalformed_ReportsProvidedWithNoTags(t *testing.T) { + setupStdoutCapture(t) + + cmd := tagsCmdWithArgs(t, "--tag=foo") + + tags, provided := tagsFromFlag(cmd, "tag") + + assert.True(t, provided) + assert.Empty(t, tags) + 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 @@ -2239,12 +2288,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 +2329,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 +2369,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 +2387,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)