diff --git a/apps/backend/internal/prompts/handlers/handlers.go b/apps/backend/internal/prompts/handlers/handlers.go index f1b8448751..111d14aa9a 100644 --- a/apps/backend/internal/prompts/handlers/handlers.go +++ b/apps/backend/internal/prompts/handlers/handlers.go @@ -1,6 +1,7 @@ package handlers import ( + "errors" "net/http" "github.com/gin-gonic/gin" @@ -51,12 +52,15 @@ func (h *Handlers) httpCreatePrompt(c *gin.Context) { } resp, err := h.controller.CreatePrompt(c.Request.Context(), req) if err != nil { - status := http.StatusInternalServerError - if err == service.ErrInvalidPrompt { - status = http.StatusBadRequest + status, message := http.StatusInternalServerError, "failed to create prompt" + switch { + case errors.Is(err, service.ErrInvalidPrompt): + status, message = http.StatusBadRequest, err.Error() + case errors.Is(err, service.ErrPromptAlreadyExists): + status, message = http.StatusConflict, err.Error() } - h.logger.Error("failed to create prompt", zap.Error(err)) - c.JSON(status, gin.H{"error": "failed to create prompt"}) + logRejection(h.logger, "create prompt rejected", "failed to create prompt", err, status) + c.JSON(status, gin.H{"error": message}) return } c.JSON(http.StatusOK, resp) @@ -70,20 +74,32 @@ func (h *Handlers) httpUpdatePrompt(c *gin.Context) { } resp, err := h.controller.UpdatePrompt(c.Request.Context(), c.Param("id"), req) if err != nil { - status := http.StatusInternalServerError - switch err { - case service.ErrInvalidPrompt: - status = http.StatusBadRequest - case service.ErrPromptNotFound: - status = http.StatusNotFound + status, message := http.StatusInternalServerError, "failed to update prompt" + switch { + case errors.Is(err, service.ErrInvalidPrompt): + status, message = http.StatusBadRequest, err.Error() + case errors.Is(err, service.ErrPromptNotFound): + status, message = http.StatusNotFound, err.Error() + case errors.Is(err, service.ErrPromptAlreadyExists): + status, message = http.StatusConflict, err.Error() } - h.logger.Error("failed to update prompt", zap.Error(err)) - c.JSON(status, gin.H{"error": "failed to update prompt"}) + logRejection(h.logger, "update prompt rejected", "failed to update prompt", err, status) + c.JSON(status, gin.H{"error": message}) return } c.JSON(http.StatusOK, resp) } +// logRejection keeps the error-rate dashboard clean by logging client-driven +// 4xx outcomes at Info level and reserving Error for unexpected 500s. +func logRejection(log *logger.Logger, infoMsg, errorMsg string, err error, status int) { + if status >= http.StatusInternalServerError { + log.Error(errorMsg, zap.Error(err)) + return + } + log.Info(infoMsg, zap.Error(err), zap.Int("status", status)) +} + func (h *Handlers) httpDeletePrompt(c *gin.Context) { if err := h.controller.DeletePrompt(c.Request.Context(), c.Param("id")); err != nil { h.logger.Error("failed to delete prompt", zap.Error(err)) diff --git a/apps/backend/internal/prompts/handlers/handlers_test.go b/apps/backend/internal/prompts/handlers/handlers_test.go new file mode 100644 index 0000000000..eb4f04347a --- /dev/null +++ b/apps/backend/internal/prompts/handlers/handlers_test.go @@ -0,0 +1,114 @@ +package handlers + +import ( + "bytes" + "encoding/json" + "net/http" + "net/http/httptest" + "path/filepath" + "testing" + + "github.com/gin-gonic/gin" + "github.com/jmoiron/sqlx" + + "github.com/kandev/kandev/internal/common/logger" + "github.com/kandev/kandev/internal/db" + "github.com/kandev/kandev/internal/prompts/controller" + "github.com/kandev/kandev/internal/prompts/service" + promptstore "github.com/kandev/kandev/internal/prompts/store" +) + +func newTestRouter(t *testing.T) (*gin.Engine, func()) { + t.Helper() + gin.SetMode(gin.TestMode) + + tmpDir := t.TempDir() + dbConn, err := db.OpenSQLite(filepath.Join(tmpDir, "test.db")) + if err != nil { + t.Fatalf("open sqlite: %v", err) + } + sqlxDB := sqlx.NewDb(dbConn, "sqlite3") + repo, repoCleanup, err := promptstore.Provide(sqlxDB, sqlxDB) + if err != nil { + t.Fatalf("provide repo: %v", err) + } + svc := service.NewService(repo) + ctrl := controller.NewController(svc) + log, _ := logger.NewLogger(logger.LoggingConfig{Level: "error", Format: "json"}) + + router := gin.New() + RegisterRoutes(router, ctrl, log) + + cleanup := func() { + if err := sqlxDB.Close(); err != nil { + t.Errorf("close sqlite: %v", err) + } + if err := repoCleanup(); err != nil { + t.Errorf("close repo: %v", err) + } + } + return router, cleanup +} + +func postJSON(t *testing.T, router *gin.Engine, path string, body any) *httptest.ResponseRecorder { + t.Helper() + buf, err := json.Marshal(body) + if err != nil { + t.Fatalf("marshal body: %v", err) + } + req := httptest.NewRequest(http.MethodPost, path, bytes.NewReader(buf)) + req.Header.Set("Content-Type", "application/json") + rec := httptest.NewRecorder() + router.ServeHTTP(rec, req) + return rec +} + +// Duplicate prompt name on POST /api/v1/prompts must surface as 409 with a +// human-readable error message — not a generic 500 — so the frontend can show +// a useful warning rather than crashing the user out of the create form. +func TestHTTPCreatePrompt_DuplicateName_Returns409(t *testing.T) { + router, cleanup := newTestRouter(t) + defer cleanup() + + first := postJSON(t, router, "/api/v1/prompts", map[string]string{ + "name": "dupe", "content": "first", + }) + if first.Code != http.StatusOK { + t.Fatalf("seed prompt: status %d body %s", first.Code, first.Body.String()) + } + + second := postJSON(t, router, "/api/v1/prompts", map[string]string{ + "name": "dupe", "content": "second", + }) + if second.Code != http.StatusConflict { + t.Fatalf("expected 409, got %d body %s", second.Code, second.Body.String()) + } + + var resp struct { + Error string `json:"error"` + } + if err := json.Unmarshal(second.Body.Bytes(), &resp); err != nil { + t.Fatalf("unmarshal response: %v", err) + } + if resp.Error == "" { + t.Fatal("expected non-empty error message") + } + if !bytes.Contains([]byte(resp.Error), []byte("already exists")) { + t.Fatalf("expected 'already exists' in message, got %q", resp.Error) + } +} + +// Built-in prompts (seeded with names like "code-review") share the unique +// name space with user-created prompts, so creating a custom prompt that +// collides with a built-in must also return 409, not 500. +func TestHTTPCreatePrompt_BuiltinName_Returns409(t *testing.T) { + router, cleanup := newTestRouter(t) + defer cleanup() + + rec := postJSON(t, router, "/api/v1/prompts", map[string]string{ + "name": "code-review", "content": "shadow", + }) + if rec.Code != http.StatusConflict { + t.Fatalf("expected 409, got %d body %s", rec.Code, rec.Body.String()) + } +} diff --git a/apps/backend/internal/prompts/service/service.go b/apps/backend/internal/prompts/service/service.go index f2047f4a75..aaa275e338 100644 --- a/apps/backend/internal/prompts/service/service.go +++ b/apps/backend/internal/prompts/service/service.go @@ -2,6 +2,7 @@ package service import ( "context" + "database/sql" "errors" "strings" @@ -10,8 +11,9 @@ import ( ) var ( - ErrPromptNotFound = errors.New("prompt not found") - ErrInvalidPrompt = errors.New("invalid prompt") + ErrPromptNotFound = errors.New("prompt not found") + ErrInvalidPrompt = errors.New("invalid prompt") + ErrPromptAlreadyExists = errors.New("prompt with this name already exists") ) type Service struct { @@ -32,16 +34,48 @@ func (s *Service) CreatePrompt(ctx context.Context, name, content string) (*mode if name == "" || content == "" { return nil, ErrInvalidPrompt } + if err := s.assertNameAvailable(ctx, name, ""); err != nil { + return nil, err + } prompt := &models.Prompt{ Name: name, Content: content, } if err := s.repo.CreatePrompt(ctx, prompt); err != nil { - return nil, err + return nil, translateNameConflict(err) } return prompt, nil } +// translateNameConflict closes the TOCTOU window between assertNameAvailable +// and the write: the SQLite UNIQUE index on custom_prompts.name is the only +// authoritative guard, and a concurrent write that loses the race surfaces a +// "UNIQUE constraint failed" driver error which would otherwise fall through +// to a generic 500. +func translateNameConflict(err error) error { + if err != nil && strings.Contains(err.Error(), "UNIQUE constraint failed") { + return ErrPromptAlreadyExists + } + return err +} + +// assertNameAvailable returns ErrPromptAlreadyExists if a different prompt with +// the given name already exists. excludeID lets callers exclude the prompt +// being updated so unchanged-name saves do not falsely trip. +func (s *Service) assertNameAvailable(ctx context.Context, name, excludeID string) error { + existing, err := s.repo.GetPromptByName(ctx, name) + if err != nil { + if errors.Is(err, sql.ErrNoRows) { + return nil + } + return err + } + if existing.ID == excludeID { + return nil + } + return ErrPromptAlreadyExists +} + func (s *Service) UpdatePrompt(ctx context.Context, promptID string, name *string, content *string) (*models.Prompt, error) { prompt, err := s.repo.GetPromptByID(ctx, promptID) if err != nil { @@ -52,6 +86,11 @@ func (s *Service) UpdatePrompt(ctx context.Context, promptID string, name *strin if trimmed == "" { return nil, ErrInvalidPrompt } + if trimmed != prompt.Name { + if err := s.assertNameAvailable(ctx, trimmed, prompt.ID); err != nil { + return nil, err + } + } prompt.Name = trimmed } if content != nil { @@ -62,7 +101,7 @@ func (s *Service) UpdatePrompt(ctx context.Context, promptID string, name *strin prompt.Content = trimmed } if err := s.repo.UpdatePrompt(ctx, prompt); err != nil { - return nil, err + return nil, translateNameConflict(err) } return prompt, nil } diff --git a/apps/backend/internal/prompts/service/service_test.go b/apps/backend/internal/prompts/service/service_test.go index b7a22dc197..5b60ca372e 100644 --- a/apps/backend/internal/prompts/service/service_test.go +++ b/apps/backend/internal/prompts/service/service_test.go @@ -2,11 +2,14 @@ package service import ( "context" + "database/sql" + "errors" "path/filepath" "testing" "github.com/jmoiron/sqlx" "github.com/kandev/kandev/internal/db" + "github.com/kandev/kandev/internal/prompts/models" promptstore "github.com/kandev/kandev/internal/prompts/store" ) @@ -69,3 +72,104 @@ func TestService_UpdatePrompt(t *testing.T) { t.Fatalf("expected content %q, got %q", content, updated.Content) } } + +func TestService_CreatePromptDuplicateName(t *testing.T) { + svc, cleanup := createService(t) + defer cleanup() + ctx := context.Background() + + if _, err := svc.CreatePrompt(ctx, "shared-name", "first"); err != nil { + t.Fatalf("seed prompt: %v", err) + } + if _, err := svc.CreatePrompt(ctx, "shared-name", "second"); err != ErrPromptAlreadyExists { + t.Fatalf("expected ErrPromptAlreadyExists, got %v", err) + } + // Trimmed input still detected. + if _, err := svc.CreatePrompt(ctx, " shared-name ", "third"); err != ErrPromptAlreadyExists { + t.Fatalf("expected ErrPromptAlreadyExists for trimmed name, got %v", err) + } +} + +func TestService_UpdatePromptRenameToExisting(t *testing.T) { + svc, cleanup := createService(t) + defer cleanup() + ctx := context.Background() + + if _, err := svc.CreatePrompt(ctx, "alpha", "a"); err != nil { + t.Fatalf("seed alpha: %v", err) + } + beta, err := svc.CreatePrompt(ctx, "beta", "b") + if err != nil { + t.Fatalf("seed beta: %v", err) + } + + rename := "alpha" + if _, err := svc.UpdatePrompt(ctx, beta.ID, &rename, nil); err != ErrPromptAlreadyExists { + t.Fatalf("expected ErrPromptAlreadyExists, got %v", err) + } +} + +// Saving a prompt without changing its name (e.g. content-only edit, or sending +// the same name through the PATCH) must not trip the duplicate-name guard. +func TestService_UpdatePromptSameName(t *testing.T) { + svc, cleanup := createService(t) + defer cleanup() + ctx := context.Background() + + prompt, err := svc.CreatePrompt(ctx, "stable", "v1") + if err != nil { + t.Fatalf("seed prompt: %v", err) + } + + sameName := "stable" + newContent := "v2" + updated, err := svc.UpdatePrompt(ctx, prompt.ID, &sameName, &newContent) + if err != nil { + t.Fatalf("update with same name: %v", err) + } + if updated.Content != newContent { + t.Fatalf("expected content %q, got %q", newContent, updated.Content) + } +} + +// raceRepo simulates a TOCTOU loss against the SQLite UNIQUE index: the +// pre-check sees no row, but the write fails because a concurrent insert +// landed first. The service must translate that into ErrPromptAlreadyExists +// rather than letting the raw driver error fall through to a 500. +type raceRepo struct { + promptstore.Repository + createErr error + updateErr error +} + +func (r *raceRepo) GetPromptByID(_ context.Context, id string) (*models.Prompt, error) { + return &models.Prompt{ID: id, Name: "old", Content: "x"}, nil +} + +func (r *raceRepo) GetPromptByName(_ context.Context, _ string) (*models.Prompt, error) { + return nil, sql.ErrNoRows +} + +func (r *raceRepo) CreatePrompt(_ context.Context, _ *models.Prompt) error { return r.createErr } +func (r *raceRepo) UpdatePrompt(_ context.Context, _ *models.Prompt) error { return r.updateErr } + +func TestService_CreatePrompt_TranslatesUniqueConstraintRace(t *testing.T) { + svc := NewService(&raceRepo{ + createErr: errors.New("UNIQUE constraint failed: custom_prompts.name"), + }) + + if _, err := svc.CreatePrompt(context.Background(), "any", "content"); err != ErrPromptAlreadyExists { + t.Fatalf("expected ErrPromptAlreadyExists, got %v", err) + } +} + +func TestService_UpdatePrompt_TranslatesUniqueConstraintRace(t *testing.T) { + svc := NewService(&raceRepo{ + updateErr: errors.New("UNIQUE constraint failed: custom_prompts.name"), + }) + + rename := "new-name" + if _, err := svc.UpdatePrompt(context.Background(), "id-1", &rename, nil); err != ErrPromptAlreadyExists { + t.Fatalf("expected ErrPromptAlreadyExists, got %v", err) + } +} diff --git a/apps/web/components/settings/prompts-settings.tsx b/apps/web/components/settings/prompts-settings.tsx index a5b7cd3d19..a2dc0d1f46 100644 --- a/apps/web/components/settings/prompts-settings.tsx +++ b/apps/web/components/settings/prompts-settings.tsx @@ -15,6 +15,7 @@ import { import { Input } from "@kandev/ui/input"; import { Textarea } from "@kandev/ui/textarea"; import { SettingsPageTemplate } from "@/components/settings/settings-page-template"; +import { useToast } from "@/components/toast-provider"; import { useCustomPrompts } from "@/hooks/domains/settings/use-custom-prompts"; import { useAppStore } from "@/components/state-provider"; import { createPrompt, deletePrompt, updatePrompt } from "@/lib/api"; @@ -26,6 +27,10 @@ const defaultFormState = { content: "", }; +function errorMessage(error: unknown): string { + return error instanceof Error ? error.message : "Request failed"; +} + type PromptFormState = typeof defaultFormState; type PromptCreateFormProps = { @@ -46,12 +51,16 @@ function PromptCreateForm({ isBusy, }: PromptCreateFormProps) { return ( -