cli: collapse --tag provided/value signals into one in browsers update

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{

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,
 	})

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)