C6 [CRIT]: unwind A13 over-fit on label create 422

pkg/cmd/label/create/create.go

@@ -9,6 +9,7 @@ import (
 	"context"
 	"errors"
 	"fmt"
+	"strings"
 
 	"github.com/spf13/cobra"
 
@@ -126,16 +127,34 @@ func Run(ctx context.Context, opts *options) error {
 }
 
 // isAlreadyExists reports whether the server's response indicates a
-// uniqueness conflict on label name. The server historically returned
-// 422 (validation failed) for that case; the labels handler also
-// returns 409 Conflict on the unique-name violation now. Accept either
-// so the friendly-error path is robust across server versions.
+// uniqueness conflict on label name.
+//
+// C6 (audit regression of A13): the previous implementation treated
+// *any* 422 as a uniqueness conflict, which made length / charset /
+// color-format validation 422s render as "already exists; pass
+// --force" — actively misleading. Tightened to:
+//   - 409 always means uniqueness (clear status semantics)
+//   - 422 only if the server message names the uniqueness case
+//     ("already taken", "already exists", or "duplicate")
+//
+// Other 422s fall through to the raw server message via the default
+// error path, which is the correct behavior for length / charset /
+// color shape failures.
 func isAlreadyExists(err error) bool {
 	var ae *api.APIError
-	if errors.As(err, &ae) && (ae.StatusCode == 422 || ae.StatusCode == 409) {
+	if !errors.As(err, &ae) {
+		return false
+	}
+	if ae.StatusCode == 409 {
 		return true
 	}
-	return false
+	if ae.StatusCode != 422 {
+		return false
+	}
+	msg := strings.ToLower(ae.Message)
+	return strings.Contains(msg, "already taken") ||
+		strings.Contains(msg, "already exists") ||
+		strings.Contains(msg, "duplicate")
 }
 
 func ptr[T any](v T) *T { return &v }

pkg/cmd/label/create/create_test.go

@@ -65,7 +65,7 @@ func TestCreateForceFallsBackToEdit(t *testing.T) {
 	tf.Server.Handle(http.MethodPost, "/api/v1/repos/o/r/labels", func(w http.ResponseWriter, _ *http.Request) {
 		w.Header().Set("Content-Type", "application/json")
 		w.WriteHeader(http.StatusUnprocessableEntity)
-		_, _ = w.Write([]byte(`{"message":"validation failed"}`))
+		_, _ = w.Write([]byte(`{"error":"label name already taken on this repo"}`))
 	})
 	var patchBody json.RawMessage
 	tf.Server.Handle(http.MethodPatch, "/api/v1/repos/o/r/labels/bug", func(w http.ResponseWriter, r *http.Request) {
@@ -93,12 +93,15 @@ func TestCreateForceFallsBackToEdit(t *testing.T) {
 	}
 }
 
-func TestCreateNoForcePropagatesConflict(t *testing.T) {
+// TestCreateUniquenessConflictNoForce: 422 whose message indicates a
+// uniqueness conflict ("already taken") should surface the friendly
+// "already exists; pass --force" error.
+func TestCreateUniquenessConflictNoForce(t *testing.T) {
 	tf := cmdutiltest.New(t)
 	tf.Server.Handle(http.MethodPost, "/api/v1/repos/o/r/labels", func(w http.ResponseWriter, _ *http.Request) {
 		w.Header().Set("Content-Type", "application/json")
 		w.WriteHeader(http.StatusUnprocessableEntity)
-		_, _ = w.Write([]byte(`{"message":"validation failed"}`))
+		_, _ = w.Write([]byte(`{"error":"label name already taken on this repo"}`))
 	})
 
 	opts := &options{
@@ -109,7 +112,71 @@ func TestCreateNoForcePropagatesConflict(t *testing.T) {
 		Repo:        "o/r",
 		Color:       "ff0000",
 	}
-	if err := Run(context.Background(), opts); err == nil {
-		t.Fatal("expected error: conflict without --force")
+	err := Run(context.Background(), opts)
+	if err == nil {
+		t.Fatal("expected error on uniqueness conflict")
+	}
+	if !strings.Contains(err.Error(), "already exists") || !strings.Contains(err.Error(), "--force") {
+		t.Errorf("expected friendly 'already exists; --force' error, got: %v", err)
+	}
+}
+
+// TestCreate409TreatedAsConflict: the new server contract uses 409 for
+// uniqueness. Confirm we still map it to the friendly error.
+func TestCreate409TreatedAsConflict(t *testing.T) {
+	tf := cmdutiltest.New(t)
+	tf.Server.Handle(http.MethodPost, "/api/v1/repos/o/r/labels", func(w http.ResponseWriter, _ *http.Request) {
+		w.Header().Set("Content-Type", "application/json")
+		w.WriteHeader(http.StatusConflict)
+		_, _ = w.Write([]byte(`{"message":"label name already taken"}`))
+	})
+
+	opts := &options{
+		IO:          tf.IOStreams,
+		HTTPClient:  tf.Factory.HTTPClient,
+		DefaultHost: tf.Factory.DefaultHost,
+		Name:        "bug",
+		Repo:        "o/r",
+		Color:       "ff0000",
+	}
+	err := Run(context.Background(), opts)
+	if err == nil {
+		t.Fatal("expected error on 409")
+	}
+	if !strings.Contains(err.Error(), "already exists") {
+		t.Errorf("expected friendly 'already exists' error, got: %v", err)
+	}
+}
+
+// TestCreate422LengthErrorNotMisclassified is the C6 regression test:
+// a 422 about length / charset / color shape MUST NOT be rendered as
+// "already exists". Audit C6 caught this happening for "name length
+// 1-50", "name too short", "color must be 3 or 6 hex digits", etc.
+// The raw server message should reach the user.
+func TestCreate422LengthErrorNotMisclassified(t *testing.T) {
+	tf := cmdutiltest.New(t)
+	tf.Server.Handle(http.MethodPost, "/api/v1/repos/o/r/labels", func(w http.ResponseWriter, _ *http.Request) {
+		w.Header().Set("Content-Type", "application/json")
+		w.WriteHeader(http.StatusUnprocessableEntity)
+		_, _ = w.Write([]byte(`{"error":"issues: label name length 1-50"}`))
+	})
+
+	opts := &options{
+		IO:          tf.IOStreams,
+		HTTPClient:  tf.Factory.HTTPClient,
+		DefaultHost: tf.Factory.DefaultHost,
+		Name:        "x", // single char triggers the server's length check in the fixture
+		Repo:        "o/r",
+		Color:       "ff0000",
+	}
+	err := Run(context.Background(), opts)
+	if err == nil {
+		t.Fatal("expected error on length-422")
+	}
+	if strings.Contains(err.Error(), "already exists") || strings.Contains(err.Error(), "--force") {
+		t.Errorf("C6 regression: length-422 misclassified as already-exists; got: %v", err)
+	}
+	if !strings.Contains(err.Error(), "length") {
+		t.Errorf("expected raw server message containing 'length'; got: %v", err)
 	}
 }