From 23722783b38e261c289bcf23844d8775e19aa550 Mon Sep 17 00:00:00 2001 From: espadonne Date: Sun, 17 May 2026 16:25:10 -0400 Subject: [PATCH 1/2] label/create: tighten 422 classification to uniqueness-only (C6) --- pkg/cmd/label/create/create.go | 31 +++++++++++++++++++++++++------ 1 file changed, 25 insertions(+), 6 deletions(-) diff --git a/pkg/cmd/label/create/create.go b/pkg/cmd/label/create/create.go index 0743520..90fa592 100644 --- a/pkg/cmd/label/create/create.go +++ b/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 } From 72495d1431957745dd5535808afc9aaac766f17a Mon Sep 17 00:00:00 2001 From: espadonne Date: Sun, 17 May 2026 16:25:11 -0400 Subject: [PATCH 2/2] label/create: cover length-422 vs uniqueness-422 paths --- pkg/cmd/label/create/create_test.go | 77 +++++++++++++++++++++++++++-- 1 file changed, 72 insertions(+), 5 deletions(-) diff --git a/pkg/cmd/label/create/create_test.go b/pkg/cmd/label/create/create_test.go index a52ee69..54bf205 100644 --- a/pkg/cmd/label/create/create_test.go +++ b/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) } }