fix(prompts): warn on duplicate custom prompt name instead of 500 (#859)

* fix(backend): return 409 on duplicate prompt name

Pre-check by name in CreatePrompt and rename paths and surface
ErrPromptAlreadyExists as 409 with the underlying error message.
Previously the SQLite UNIQUE constraint bubbled up as a 500 with
a generic body, so the frontend had nothing actionable to show.

* feat(web): toast on prompt save errors with e2e coverage

Wire useToast in the Prompts settings page so create/update/delete
failures surface a warning instead of being swallowed silently. Add
data-testid hooks plus Playwright specs covering duplicate-name
create, rename-to-existing, and the happy path.

* fix(backend): close TOCTOU + tighten prompt error handling

Address PR review:
- log client errors (4xx) at Info, reserve Error for unexpected 500s
- switch handler error mapping to errors.Is so wrapped sentinels still
  resolve to the right HTTP status
- drop the unreachable existing==nil branch in assertNameAvailable
- translate SQLite UNIQUE constraint errors that slip through the
  pre-check race into ErrPromptAlreadyExists so concurrent dup-name
  inserts return 409 instead of 500
This commit is contained in:
José Almeida
2026-05-10 17:04:56 +01:00
committed by GitHub
parent 21202ef437
commit 5223a1cd17
7 changed files with 428 additions and 24 deletions
@@ -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))
@@ -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())
}
}
@@ -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
}
@@ -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)
}
}
@@ -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 (
<div className="rounded-lg border border-border/70 bg-background p-4 space-y-3">
<div
className="rounded-lg border border-border/70 bg-background p-4 space-y-3"
data-testid="prompt-create-form"
>
<div className="text-sm font-medium text-foreground">Add prompt</div>
<Input
value={formState.name}
onChange={(event) => onFormChange({ name: event.target.value })}
placeholder="Prompt name"
data-testid="prompt-name-input"
/>
<Textarea
value={formState.content}
@@ -59,9 +68,10 @@ function PromptCreateForm({
placeholder="Prompt content"
rows={5}
className="resize-y max-h-60 overflow-auto"
data-testid="prompt-content-input"
/>
<div className="flex items-center gap-2">
<Button onClick={onSubmit} disabled={!isValid || isBusy}>
<Button onClick={onSubmit} disabled={!isValid || isBusy} data-testid="prompt-submit">
Add prompt
</Button>
<Button variant="ghost" onClick={onCancel} disabled={isBusy}>
@@ -109,6 +119,8 @@ function PromptListItem({
<div
className="rounded-lg border border-border/70 bg-background p-4 flex flex-col gap-3"
ref={isEditing ? editingRef : null}
data-testid="prompt-list-item"
data-prompt-name={prompt.name}
>
<div className="flex items-center justify-between gap-2">
<div className="flex items-center gap-2">
@@ -127,6 +139,7 @@ function PromptListItem({
onClick={() => onStartEditing(prompt)}
disabled={isBusy || showCreate}
className="cursor-pointer"
data-testid="prompt-edit-button"
>
<IconEdit className="h-4 w-4" />
</Button>
@@ -147,6 +160,7 @@ function PromptListItem({
value={formState.name}
onChange={(event) => onFormChange({ name: event.target.value })}
placeholder="Prompt name"
data-testid="prompt-name-input"
/>
<Textarea
value={formState.content}
@@ -154,9 +168,10 @@ function PromptListItem({
placeholder="Prompt content"
rows={5}
className="resize-y max-h-60 overflow-auto"
data-testid="prompt-content-input"
/>
<div className="flex items-center gap-2">
<Button onClick={onUpdate} disabled={!isValid || isBusy}>
<Button onClick={onUpdate} disabled={!isValid || isBusy} data-testid="prompt-submit">
Save changes
</Button>
<Button variant="ghost" onClick={onCancel} disabled={isBusy}>
@@ -319,6 +334,7 @@ function usePromptsActions(state: ReturnType<typeof usePromptsState>) {
deleteTarget,
formState,
} = state;
const { toast } = useToast();
const resetForm = useCallback(() => {
setEditingId(null);
@@ -364,13 +380,15 @@ function usePromptsActions(state: ReturnType<typeof usePromptsState>) {
});
const isBusy = createRequest.isLoading || updateRequest.isLoading || deleteRequest.isLoading;
const toastError = (title: string) => (err: unknown) =>
toast({ title, description: errorMessage(err), variant: "error" });
const handleCreate = () => {
if (!isValid || isBusy) return;
createRequest.run(formState).catch(() => undefined);
createRequest.run(formState).catch(toastError("Couldn't create prompt"));
};
const handleUpdate = () => {
if (!isValid || isBusy || !editingId) return;
updateRequest.run(editingId, formState).catch(() => undefined);
updateRequest.run(editingId, formState).catch(toastError("Couldn't save prompt"));
};
const startEditing = (prompt: CustomPrompt) => {
setEditingId(prompt.id);
@@ -390,7 +408,7 @@ function usePromptsActions(state: ReturnType<typeof usePromptsState>) {
};
const confirmDelete = () => {
if (!deleteTarget) return;
deleteRequest.run(deleteTarget.id).catch(() => undefined);
deleteRequest.run(deleteTarget.id).catch(toastError("Couldn't delete prompt"));
closeDeleteDialog();
};
@@ -455,7 +473,11 @@ export function PromptsSettings() {
<div className="space-y-6 mt-4">
<div className="flex items-center justify-between">
<div className="text-sm font-medium text-foreground">Custom prompts</div>
<Button onClick={startCreate} disabled={isBusy || isEditing || showCreate}>
<Button
onClick={startCreate}
disabled={isBusy || isEditing || showCreate}
data-testid="prompt-create-button"
>
Add prompt
</Button>
</div>
+17
View File
@@ -294,6 +294,23 @@ export class ApiClient {
await this.request("PATCH", `/api/v1/agent-profiles/${profileId}`, patch);
}
async listPrompts(): Promise<{
prompts: Array<{ id: string; name: string; content: string; builtin: boolean }>;
}> {
return this.request("GET", "/api/v1/prompts");
}
async createPrompt(
name: string,
content: string,
): Promise<{ id: string; name: string; content: string; builtin: boolean }> {
return this.request("POST", "/api/v1/prompts", { name, content });
}
async deletePrompt(promptId: string): Promise<void> {
await this.request("DELETE", `/api/v1/prompts/${promptId}`);
}
async createTaskWithAgent(
workspaceId: string,
title: string,
@@ -0,0 +1,92 @@
import { test, expect } from "../../fixtures/test-base";
// Custom prompts share a UNIQUE name index with built-in prompts. Prior to the
// 409-mapping fix, posting a duplicate name returned 500 and the frontend
// silently swallowed the error. These tests guard the toast contract so a
// regression cannot ship unnoticed.
test.describe("Prompts settings — duplicate name handling", () => {
test.afterEach(async ({ apiClient }) => {
const { prompts } = await apiClient.listPrompts();
for (const p of prompts) {
if (!p.builtin) {
await apiClient.deletePrompt(p.id).catch(() => undefined);
}
}
});
test("creating a prompt with a duplicate name shows an error toast and keeps the form open", async ({
testPage,
apiClient,
}) => {
test.setTimeout(60_000);
await apiClient.createPrompt("dupe-prompt", "first content");
await testPage.goto("/settings/prompts");
await testPage.getByTestId("prompt-create-button").click();
const form = testPage.getByTestId("prompt-create-form");
await expect(form).toBeVisible();
await form.getByTestId("prompt-name-input").fill("dupe-prompt");
await form.getByTestId("prompt-content-input").fill("second content");
await form.getByTestId("prompt-submit").click();
const toast = testPage.getByTestId("toast-message");
await expect(toast).toBeVisible({ timeout: 5_000 });
await expect(toast).toContainText(/already exists/i);
// Form must remain open (no silent reset) so the user can fix the name.
await expect(form).toBeVisible();
await expect(form.getByTestId("prompt-name-input")).toHaveValue("dupe-prompt");
});
test("renaming a prompt to an existing name shows an error toast and does not persist the rename", async ({
testPage,
apiClient,
}) => {
test.setTimeout(60_000);
await apiClient.createPrompt("alpha", "a");
await apiClient.createPrompt("beta", "b");
await testPage.goto("/settings/prompts");
const betaRow = testPage.locator('[data-testid="prompt-list-item"][data-prompt-name="beta"]');
await expect(betaRow).toBeVisible();
await betaRow.getByTestId("prompt-edit-button").click();
const nameInput = betaRow.getByTestId("prompt-name-input");
await nameInput.fill("alpha");
await betaRow.getByTestId("prompt-submit").click();
const toast = testPage.getByTestId("toast-message");
await expect(toast).toBeVisible({ timeout: 5_000 });
await expect(toast).toContainText(/already exists/i);
// Backend rejected — the row must still exist under its original name.
await testPage.reload();
await expect(
testPage.locator('[data-testid="prompt-list-item"][data-prompt-name="beta"]'),
).toBeVisible();
});
test("creating a prompt with a unique name succeeds and appears in the list", async ({
testPage,
}) => {
test.setTimeout(60_000);
await testPage.goto("/settings/prompts");
await testPage.getByTestId("prompt-create-button").click();
const form = testPage.getByTestId("prompt-create-form");
await form.getByTestId("prompt-name-input").fill("e2e-fresh-prompt");
await form.getByTestId("prompt-content-input").fill("hello world");
await form.getByTestId("prompt-submit").click();
await expect(
testPage.locator('[data-testid="prompt-list-item"][data-prompt-name="e2e-fresh-prompt"]'),
).toBeVisible({ timeout: 10_000 });
// Form should be reset / closed on success.
await expect(testPage.getByTestId("prompt-create-form")).toHaveCount(0);
});
});