fix(office): reject empty slug on skill update (#3509)

Co-authored-by: nova28 <17953305+nova28@users.noreply.github.com>
Co-authored-by: Carlos Florencio <carlosmflorencio@gmail.com>

apps/backend/internal/agentctl/server/process/workspace_poll_mode_grace_test.go

@@ -187,6 +187,11 @@ func TestPollModeGrace_StopDisarmsTimer(t *testing.T) {
 // it to return, and prevent the callback from changing the mode afterward.
 func TestPollModeGrace_StopJoinsFinalScan(t *testing.T) {
 	wt := newGraceTestTracker(t, graceFiresQuickly)
+	// The test covers the grace callback's cancellation and wait-group
+	// ownership. Disable the independent polling loops so their real Git work
+	// cannot change poll mode while this lifecycle boundary is under test (the
+	// Windows race runner can otherwise report an unrelated paused transition).
+	wt.gitIndexPath = ""
 	finalScanStarted := make(chan struct{})
 	finalScanFinished := make(chan struct{})
 	wt.gitStatusObserver = func(ctx context.Context) (types.GitStatusUpdate, error) {

apps/backend/internal/office/skills/handler.go

@@ -98,7 +98,7 @@ func (h *Handler) updateSkill(c *gin.Context) {
 		return
 	}
 	applySkillUpdates(skill, &req)
-	if err := h.svc.ValidateSkillUpdate(ctx, skill); err != nil {
+	if err := h.svc.ValidateSkillUpdate(ctx, skill, req.Slug != nil); err != nil {
 		c.JSON(http.StatusBadRequest, gin.H{"error": err.Error()})
 		return
 	}

apps/backend/internal/office/skills/handler_test.go

@@ -0,0 +1,190 @@
+package skills_test
+
+import (
+	"context"
+	"encoding/json"
+	"net/http"
+	"net/http/httptest"
+	"strings"
+	"testing"
+
+	"github.com/gin-gonic/gin"
+
+	"github.com/kandev/kandev/internal/office/models"
+	"github.com/kandev/kandev/internal/office/skills"
+)
+
+// newTestSkillRouter mounts the skills routes over a fresh service.
+func newTestSkillRouter(t *testing.T) (*gin.Engine, *skills.SkillService) {
+	t.Helper()
+	gin.SetMode(gin.TestMode)
+	svc := newTestSkillService(t)
+	router := gin.New()
+	skills.NewHandler(svc).RegisterRoutes(router.Group("/api/v1"))
+	return router, svc
+}
+
+func doSkillRequest(t *testing.T, router *gin.Engine, method, path, body string) *httptest.ResponseRecorder {
+	t.Helper()
+	req := httptest.NewRequest(method, path, strings.NewReader(body))
+	req.Header.Set("Content-Type", "application/json")
+	rec := httptest.NewRecorder()
+	router.ServeHTTP(rec, req)
+	return rec
+}
+
+func TestUpdateSkillHandler_RejectsEmptySlug(t *testing.T) {
+	router, svc := newTestSkillRouter(t)
+	ctx := context.Background()
+
+	skill := &models.Skill{WorkspaceID: "ws-1", Name: "Existing", Slug: "kandev-existing", SourceType: "inline"}
+	if err := svc.ValidateAndPrepareSkill(ctx, skill); err != nil {
+		t.Fatalf("validate: %v", err)
+	}
+	if err := svc.CreateSkill(ctx, skill); err != nil {
+		t.Fatalf("create: %v", err)
+	}
+
+	rec := doSkillRequest(t, router, http.MethodPatch, "/api/v1/skills/"+skill.ID, `{"slug":""}`)
+	if rec.Code != http.StatusBadRequest {
+		t.Fatalf("status: got %d, want %d (body: %s)", rec.Code, http.StatusBadRequest, rec.Body.String())
+	}
+
+	reloaded, err := svc.GetSkillFromConfig(ctx, skill.ID)
+	if err != nil {
+		t.Fatalf("reload: %v", err)
+	}
+	if reloaded.Slug != "kandev-existing" {
+		t.Errorf("stored slug = %q, want unchanged %q", reloaded.Slug, "kandev-existing")
+	}
+}
+
+func TestUpdateSkillHandler_RejectsNotWellFormedSlug(t *testing.T) {
+	router, svc := newTestSkillRouter(t)
+	ctx := context.Background()
+
+	skill := &models.Skill{WorkspaceID: "ws-1", Name: "Existing", Slug: "kandev-existing", SourceType: "inline"}
+	if err := svc.ValidateAndPrepareSkill(ctx, skill); err != nil {
+		t.Fatalf("validate: %v", err)
+	}
+	if err := svc.CreateSkill(ctx, skill); err != nil {
+		t.Fatalf("create: %v", err)
+	}
+
+	rec := doSkillRequest(t, router, http.MethodPatch, "/api/v1/skills/"+skill.ID, `{"slug":"not a valid slug!"}`)
+	if rec.Code != http.StatusBadRequest {
+		t.Fatalf("status: got %d, want %d (body: %s)", rec.Code, http.StatusBadRequest, rec.Body.String())
+	}
+
+	reloaded, err := svc.GetSkillFromConfig(ctx, skill.ID)
+	if err != nil {
+		t.Fatalf("reload: %v", err)
+	}
+	if reloaded.Slug != "kandev-existing" {
+		t.Errorf("stored slug = %q, want unchanged %q", reloaded.Slug, "kandev-existing")
+	}
+}
+
+func TestUpdateSkillHandler_ContentOnlyPatchHealsStoredEmptySlug(t *testing.T) {
+	router, svc := newTestSkillRouter(t)
+	ctx := context.Background()
+
+	// Bypasses ValidateAndPrepareSkill, mirroring config-import's CreateSkill
+	// call, to reproduce a durable row with an empty stored slug.
+	skill := &models.Skill{WorkspaceID: "ws-1", Name: "Existing Skill", Slug: "", SourceType: "inline"}
+	if err := svc.CreateSkill(ctx, skill); err != nil {
+		t.Fatalf("create: %v", err)
+	}
+
+	rec := doSkillRequest(t, router, http.MethodPatch, "/api/v1/skills/"+skill.ID, `{"content":"new content"}`)
+	if rec.Code != http.StatusOK {
+		t.Fatalf("status: got %d, want %d (body: %s)", rec.Code, http.StatusOK, rec.Body.String())
+	}
+
+	var resp skills.SkillResponse
+	if err := json.Unmarshal(rec.Body.Bytes(), &resp); err != nil {
+		t.Fatalf("decode body %q: %v", rec.Body.String(), err)
+	}
+	if resp.Skill.Slug == "" {
+		t.Errorf("response slug still empty after content-only PATCH")
+	}
+
+	reloaded, err := svc.GetSkillFromConfig(ctx, skill.ID)
+	if err != nil {
+		t.Fatalf("reload: %v", err)
+	}
+	if reloaded.Slug == "" {
+		t.Errorf("stored slug still empty after content-only PATCH")
+	}
+}
+
+func TestUpdateSkillHandler_ContentOnlyPatchDoesNotFailOnSlugCollision(t *testing.T) {
+	router, svc := newTestSkillRouter(t)
+	ctx := context.Background()
+
+	occupant := &models.Skill{WorkspaceID: "ws-1", Name: "Existing Skill", SourceType: "inline"}
+	if err := svc.ValidateAndPrepareSkill(ctx, occupant); err != nil {
+		t.Fatalf("validate occupant: %v", err)
+	}
+	if err := svc.CreateSkill(ctx, occupant); err != nil {
+		t.Fatalf("create occupant: %v", err)
+	}
+
+	// Bypasses ValidateAndPrepareSkill, mirroring config-import's CreateSkill
+	// call, to reproduce a durable row with an empty stored slug whose
+	// name-derived candidate collides with occupant's slug.
+	broken := &models.Skill{WorkspaceID: "ws-1", Name: "Existing Skill", Slug: "", SourceType: "inline"}
+	if err := svc.CreateSkill(ctx, broken); err != nil {
+		t.Fatalf("create broken: %v", err)
+	}
+
+	rec := doSkillRequest(t, router, http.MethodPatch, "/api/v1/skills/"+broken.ID, `{"content":"new content"}`)
+	if rec.Code != http.StatusOK {
+		t.Fatalf("status: got %d, want %d (body: %s)", rec.Code, http.StatusOK, rec.Body.String())
+	}
+
+	reloaded, err := svc.GetSkillFromConfig(ctx, broken.ID)
+	if err != nil {
+		t.Fatalf("reload: %v", err)
+	}
+	if reloaded.Slug != "" {
+		t.Errorf("stored slug = %q, want left empty on collision", reloaded.Slug)
+	}
+	if reloaded.Content != "new content" {
+		t.Errorf("content = %q, want %q", reloaded.Content, "new content")
+	}
+}
+
+func TestUpdateSkillHandler_OmittedSlugLeavesItUnchanged(t *testing.T) {
+	router, svc := newTestSkillRouter(t)
+	ctx := context.Background()
+
+	skill := &models.Skill{WorkspaceID: "ws-1", Name: "Existing", Slug: "kandev-existing", SourceType: "inline"}
+	if err := svc.ValidateAndPrepareSkill(ctx, skill); err != nil {
+		t.Fatalf("validate: %v", err)
+	}
+	if err := svc.CreateSkill(ctx, skill); err != nil {
+		t.Fatalf("create: %v", err)
+	}
+
+	rec := doSkillRequest(t, router, http.MethodPatch, "/api/v1/skills/"+skill.ID, `{"content":"new content"}`)
+	if rec.Code != http.StatusOK {
+		t.Fatalf("status: got %d, want %d (body: %s)", rec.Code, http.StatusOK, rec.Body.String())
+	}
+
+	var resp skills.SkillResponse
+	if err := json.Unmarshal(rec.Body.Bytes(), &resp); err != nil {
+		t.Fatalf("decode body %q: %v", rec.Body.String(), err)
+	}
+	if resp.Skill.Slug != "kandev-existing" {
+		t.Errorf("response slug = %q, want unchanged %q", resp.Skill.Slug, "kandev-existing")
+	}
+
+	reloaded, err := svc.GetSkillFromConfig(ctx, skill.ID)
+	if err != nil {
+		t.Fatalf("reload: %v", err)
+	}
+	if reloaded.Slug != "kandev-existing" {
+		t.Errorf("stored slug = %q, want unchanged %q", reloaded.Slug, "kandev-existing")
+	}
+}

apps/backend/internal/office/skills/service.go

@@ -169,11 +169,20 @@ func prepareSkillPackageMetadata(skill *models.Skill) {
 }
 
 // ValidateSkillUpdate validates a skill update for slug uniqueness. Slug
-// handling mirrors ValidateAndPrepareSkill: reject a not-well-formed slug
-// outright (AC-001.11), otherwise normalize to canonical before the
-// uniqueness check (AC-001.12).
-func (s *SkillService) ValidateSkillUpdate(ctx context.Context, skill *models.Skill) error {
-	if skill.Slug == "" {
+// handling mirrors ValidateAndPrepareSkill: an empty slug is not
+// well-formed and is rejected like any other not-well-formed slug when the
+// caller supplies one; the caller omits the field entirely (slugRequested
+// false) to leave the slug unchanged. A well-formed slug is normalized to
+// canonical form before the uniqueness check runs.
+//
+// A request that never mentions slug must never fail on slug grounds. The
+// config-import path can create a row with an empty stored slug outside
+// this service's own validation, so an unrelated update to such a row
+// heals it to a free name-derived slug rather than rejecting the caller's
+// edit; if every candidate collides, the stored empty value is left as-is.
+func (s *SkillService) ValidateSkillUpdate(ctx context.Context, skill *models.Skill, slugRequested bool) error {
+	if !slugRequested && skill.Slug == "" {
+		s.healEmptySlug(ctx, skill)
 		return nil
 	}
 	if !skillslug.WellFormed(skill.Slug) {
@@ -183,6 +192,15 @@ func (s *SkillService) ValidateSkillUpdate(ctx context.Context, skill *models.Sk
 	return s.validateSlugUnique(ctx, skill.WorkspaceID, skill.Slug, skill.ID)
 }
 
+// healEmptySlug assigns skill a name-derived slug when one is free in its
+// workspace, and leaves its stored empty slug untouched otherwise.
+func (s *SkillService) healEmptySlug(ctx context.Context, skill *models.Skill) {
+	candidate := skillslug.Normalize(GenerateSlug(skill.Name))
+	if s.validateSlugUnique(ctx, skill.WorkspaceID, candidate, skill.ID) == nil {
+		skill.Slug = candidate
+	}
+}
+
 func (s *SkillService) validateSlugUnique(ctx context.Context, workspaceID, slug, excludeID string) error {
 	skills, err := s.repo.ListSkills(ctx, workspaceID)
 	if err != nil {

apps/backend/internal/office/skills/service_slug_update_test.go

@@ -0,0 +1,76 @@
+package skills_test
+
+import (
+	"context"
+	"testing"
+
+	"github.com/kandev/kandev/internal/office/models"
+)
+
+func TestValidateSkillUpdate_RejectsEmptySlug(t *testing.T) {
+	svc := newTestSkillService(t)
+	ctx := context.Background()
+
+	skill := &models.Skill{WorkspaceID: "ws-1", Name: "Existing", Slug: "kandev-existing", SourceType: "inline"}
+	if err := svc.ValidateAndPrepareSkill(ctx, skill); err != nil {
+		t.Fatalf("validate: %v", err)
+	}
+	if err := svc.CreateSkill(ctx, skill); err != nil {
+		t.Fatalf("create: %v", err)
+	}
+
+	skill.Slug = ""
+	if err := svc.ValidateSkillUpdate(ctx, skill, true); err == nil {
+		t.Fatal("expected error for empty slug")
+	}
+	if skill.Slug != "" {
+		t.Errorf("slug = %q, want unchanged empty string, not coerced", skill.Slug)
+	}
+}
+
+func TestValidateSkillUpdate_HealsEmptyStoredSlugWhenNotRequested(t *testing.T) {
+	svc := newTestSkillService(t)
+	ctx := context.Background()
+
+	// Bypasses ValidateAndPrepareSkill, mirroring config-import's CreateSkill
+	// call, to reproduce a durable row with an empty stored slug.
+	skill := &models.Skill{WorkspaceID: "ws-1", Name: "Existing Skill", SourceType: "inline"}
+	if err := svc.CreateSkill(ctx, skill); err != nil {
+		t.Fatalf("create: %v", err)
+	}
+
+	if err := svc.ValidateSkillUpdate(ctx, skill, false); err != nil {
+		t.Fatalf("update: %v", err)
+	}
+	if skill.Slug == "" {
+		t.Error("slug still empty after healing an unrequested update")
+	}
+}
+
+func TestValidateSkillUpdate_LeavesEmptyStoredSlugOnCollisionWhenNotRequested(t *testing.T) {
+	svc := newTestSkillService(t)
+	ctx := context.Background()
+
+	occupant := &models.Skill{WorkspaceID: "ws-1", Name: "Existing Skill", SourceType: "inline"}
+	if err := svc.ValidateAndPrepareSkill(ctx, occupant); err != nil {
+		t.Fatalf("validate occupant: %v", err)
+	}
+	if err := svc.CreateSkill(ctx, occupant); err != nil {
+		t.Fatalf("create occupant: %v", err)
+	}
+
+	// Bypasses ValidateAndPrepareSkill, mirroring config-import's CreateSkill
+	// call, to reproduce a durable row with an empty stored slug whose
+	// name-derived candidate collides with an existing skill.
+	broken := &models.Skill{WorkspaceID: "ws-1", Name: "Existing Skill", SourceType: "inline"}
+	if err := svc.CreateSkill(ctx, broken); err != nil {
+		t.Fatalf("create broken: %v", err)
+	}
+
+	if err := svc.ValidateSkillUpdate(ctx, broken, false); err != nil {
+		t.Fatalf("update: %v", err)
+	}
+	if broken.Slug != "" {
+		t.Errorf("slug = %q, want left empty on collision", broken.Slug)
+	}
+}

apps/backend/internal/office/skills/service_test.go

@@ -247,7 +247,7 @@ func TestValidateSkillUpdate_NormalizesWellFormedSlugToCanonical(t *testing.T) {
 	}
 
 	skill.Slug = "renamed"
-	if err := svc.ValidateSkillUpdate(ctx, skill); err != nil {
+	if err := svc.ValidateSkillUpdate(ctx, skill, true); err != nil {
 		t.Fatalf("update: %v", err)
 	}
 	if skill.Slug != "kandev-renamed" {
@@ -268,7 +268,7 @@ func TestValidateSkillUpdate_RejectsNotWellFormedSlug(t *testing.T) {
 	}
 
 	skill.Slug = "not a valid slug!"
-	if err := svc.ValidateSkillUpdate(ctx, skill); err == nil {
+	if err := svc.ValidateSkillUpdate(ctx, skill, true); err == nil {
 		t.Fatal("expected error for not-well-formed slug")
 	}
 }