diff --git a/apps/backend/internal/agentctl/server/process/workspace_poll_mode_grace_test.go b/apps/backend/internal/agentctl/server/process/workspace_poll_mode_grace_test.go index 09124d354c..756a26c76f 100644 --- a/apps/backend/internal/agentctl/server/process/workspace_poll_mode_grace_test.go +++ b/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) { diff --git a/apps/backend/internal/office/skills/handler.go b/apps/backend/internal/office/skills/handler.go index 35e9c0c743..2c8060a575 100644 --- a/apps/backend/internal/office/skills/handler.go +++ b/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 } diff --git a/apps/backend/internal/office/skills/handler_test.go b/apps/backend/internal/office/skills/handler_test.go new file mode 100644 index 0000000000..e2a93f4c45 --- /dev/null +++ b/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") + } +} diff --git a/apps/backend/internal/office/skills/service.go b/apps/backend/internal/office/skills/service.go index f1ade825d0..0d53f18397 100644 --- a/apps/backend/internal/office/skills/service.go +++ b/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 { diff --git a/apps/backend/internal/office/skills/service_slug_update_test.go b/apps/backend/internal/office/skills/service_slug_update_test.go new file mode 100644 index 0000000000..f53393f611 --- /dev/null +++ b/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) + } +} diff --git a/apps/backend/internal/office/skills/service_test.go b/apps/backend/internal/office/skills/service_test.go index 6720c9c78e..07912e1c46 100644 --- a/apps/backend/internal/office/skills/service_test.go +++ b/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") } }