diff --git a/packages/core/api/client.ts b/packages/core/api/client.ts index d3ced64a18..73ff7bf788 100644 --- a/packages/core/api/client.ts +++ b/packages/core/api/client.ts @@ -202,6 +202,7 @@ import { AutopilotRunSchema, FALLBACK_AUTOPILOT_RUN, ListIssuesResponseSchema, + CreateIssueResponseSchema, ListWebhookDeliveriesResponseSchema, RuntimeHourlyActivityListSchema, RuntimeUsageByAgentListSchema, @@ -658,10 +659,26 @@ export class ApiClient { } async createIssue(data: CreateIssueRequest): Promise { - return this.fetch("/api/issues", { + // Parse through a schema (not a raw cast): the create modal keys its + // label-attach compatibility fallback off `labels` being absent vs a + // validated Label[], so an unvalidated wrong shape must not slip through. + // Unlike list endpoints, a create that returns an unusable body is a + // FAILED mutation, not a safe-empty read: fall back to null and reject so + // the modal keeps the draft and shows a failure toast instead of a blank + // "created" card pointing at an empty issue id. parseWithFallback already + // logged the schema issues + raw payload; the empty message lets the modal + // render its localized "failed to create" toast. + const raw = await this.fetch("/api/issues", { method: "POST", body: JSON.stringify(data), }); + const issue = parseWithFallback(raw, CreateIssueResponseSchema, null, { + endpoint: "POST /api/issues", + }); + if (!issue) { + throw new Error(); + } + return issue; } async quickCreateIssue(data: { diff --git a/packages/core/api/schema.test.ts b/packages/core/api/schema.test.ts index 605e12b955..79e96a9f05 100644 --- a/packages/core/api/schema.test.ts +++ b/packages/core/api/schema.test.ts @@ -91,6 +91,91 @@ describe("ApiClient schema fallback", () => { }); }); + describe("createIssue", () => { + // The create modal decides whether to run its label-attach fallback by + // reading `labels` off the parsed response, and treats a rejection as a + // failed create (keep the draft, failure toast). So: a valid issue with + // any labels shape resolves (labels absent → undefined, valid → Label[], + // malformed → undefined), but a body that isn't a usable issue rejects + // rather than fabricating a blank "success". + const validIssue = { + id: "issue-1", + workspace_id: "ws-1", + number: 1, + identifier: "MUL-1", + title: "Created", + description: null, + status: "todo", + priority: "none", + assignee_type: null, + assignee_id: null, + creator_type: "member", + creator_id: "user-1", + parent_issue_id: null, + project_id: null, + position: 0, + stage: null, + start_date: null, + due_date: null, + metadata: {}, + properties: {}, + created_at: "2025-01-01T00:00:00Z", + updated_at: "2025-01-01T00:00:00Z", + }; + const label = { + id: "label-1", + workspace_id: "ws-1", + name: "bug", + color: "#ef4444", + created_at: "2025-01-01T00:00:00Z", + updated_at: "2025-01-01T00:00:00Z", + }; + + it("keeps labels undefined when the backend omits the field (older backend)", async () => { + stubFetchJson(validIssue, 201); + const client = new ApiClient("https://api.example.test"); + const issue = await client.createIssue({ title: "Created" }); + expect(issue.id).toBe("issue-1"); + expect(issue.labels).toBeUndefined(); + }); + + it("validates a well-formed labels array", async () => { + stubFetchJson({ ...validIssue, labels: [label] }, 201); + const client = new ApiClient("https://api.example.test"); + const issue = await client.createIssue({ title: "Created" }); + expect(issue.labels?.map((l) => l.id)).toEqual(["label-1"]); + }); + + it("degrades a null labels field to undefined so the client falls back", async () => { + stubFetchJson({ ...validIssue, labels: null }, 201); + const client = new ApiClient("https://api.example.test"); + const issue = await client.createIssue({ title: "Created" }); + // The issue itself still parses; only the malformed labels degrade. + expect(issue.id).toBe("issue-1"); + expect(issue.labels).toBeUndefined(); + }); + + it("degrades a labels array of the wrong element shape to undefined", async () => { + stubFetchJson({ ...validIssue, labels: [{ nope: true }] }, 201); + const client = new ApiClient("https://api.example.test"); + const issue = await client.createIssue({ title: "Created" }); + expect(issue.id).toBe("issue-1"); + expect(issue.labels).toBeUndefined(); + }); + + it("rejects when the whole response body is not a usable issue (no fake success)", async () => { + stubFetchJson({ not: "an issue" }, 201); + const client = new ApiClient("https://api.example.test"); + await expect(client.createIssue({ title: "Created" })).rejects.toThrow(); + }); + + it("rejects when the created issue has an empty id", async () => { + stubFetchJson({ ...validIssue, id: "" }, 201); + const client = new ApiClient("https://api.example.test"); + await expect(client.createIssue({ title: "Created" })).rejects.toThrow(); + }); + }); + describe("searchIssues", () => { it("falls back to an empty result when the response is malformed", async () => { stubFetchJson({ issues: "not-an-array", total: 0 }); diff --git a/packages/core/api/schemas.ts b/packages/core/api/schemas.ts index 4a8eb135de..f1390ba8fc 100644 --- a/packages/core/api/schemas.ts +++ b/packages/core/api/schemas.ts @@ -469,6 +469,25 @@ export const ListIssuesResponseSchema = z.object({ total: z.number().default(0), }).loose(); +// Response schema for POST /api/issues. Two tightenings over IssueSchema: +// +// - `id` must be non-empty. A created issue always carries a real id, so an +// empty/absent id means the create effectively failed. createIssue turns a +// schema failure into a rejection (not a fabricated success), so tightening +// id here routes an id-less body to that same failure path. +// - `labels` is the backend-compatibility signal the create modal reads to +// decide whether the backend attached labels in the create transaction +// (present) or predates that (absent → fall back to per-label attach). +// Validate it strictly as Label[] and degrade a malformed value to +// `undefined` — the same as an absent field — so a wrong shape (null, +// object, a garbage array) can never masquerade as "handled" and suppress +// the fallback. Unlike the loose IssueSchema.labels (z.array(z.unknown())), +// the elements are fully validated. See packages/views/modals/create-issue.tsx. +export const CreateIssueResponseSchema = IssueSchema.extend({ + id: z.string().min(1), + labels: z.array(LabelSchema).optional().catch(undefined), +}).loose(); + export const EMPTY_LIST_ISSUES_RESPONSE: ListIssuesResponse = { issues: [], total: 0, diff --git a/packages/core/issues/ws-updaters.test.ts b/packages/core/issues/ws-updaters.test.ts index 1bb4364807..5002ae4d32 100644 --- a/packages/core/issues/ws-updaters.test.ts +++ b/packages/core/issues/ws-updaters.test.ts @@ -264,6 +264,23 @@ describe("project progress invalidation", () => { }); }); +describe("onIssueCreated — carries the label snapshot into list cache", () => { + it("keeps the created issue's labels so members other than the creator render it already labeled", () => { + // The backend now attaches labels in the create transaction and echoes + // them on the issue:created event. Guard that the cache insert doesn't + // strip them — otherwise online members would see the new issue blank + // until a refetch (staleTime: Infinity means no self-heal). + const qc = new QueryClient(); + qc.setQueryData(issueKeys.list(WS_ID), makeListCache()); + + onIssueCreated(qc, WS_ID, { ...baseIssue, labels: [labelA, labelB] }); + + const cache = qc.getQueryData(issueKeys.list(WS_ID)); + const cached = cache?.byStatus.todo?.issues.find((i) => i.id === ISSUE_ID); + expect(cached?.labels).toEqual([labelA, labelB]); + }); +}); + describe("onIssueUpdated — position move is surgical, not a list refetch", () => { let qc: QueryClient; diff --git a/packages/core/types/api.ts b/packages/core/types/api.ts index 798779ab5e..8daaec69b3 100644 --- a/packages/core/types/api.ts +++ b/packages/core/types/api.ts @@ -17,6 +17,9 @@ export interface CreateIssueRequest { start_date?: string; due_date?: string; attachment_ids?: string[]; + /** Issue-scoped label IDs to attach in the same transaction as the create. + * Unknown or non-issue ids are rejected by the server with 400. */ + label_ids?: string[]; } export interface UpdateIssueRequest { diff --git a/packages/views/modals/create-issue.test.tsx b/packages/views/modals/create-issue.test.tsx index 41713af81b..cc1a599c68 100644 --- a/packages/views/modals/create-issue.test.tsx +++ b/packages/views/modals/create-issue.test.tsx @@ -23,6 +23,7 @@ function I18nWrapper({ children }: { children: ReactNode }) { const mockPush = vi.hoisted(() => vi.fn()); const mockCreateIssue = vi.hoisted(() => vi.fn()); +const mockAttachLabel = vi.hoisted(() => vi.fn()); const mockSetDraft = vi.hoisted(() => vi.fn()); const mockClearDraft = vi.hoisted(() => vi.fn()); const mockSetLastAssignee = vi.hoisted(() => vi.fn()); @@ -140,6 +141,10 @@ vi.mock("@multica/core/issues/mutations", () => ({ useUpdateIssue: () => ({ mutate: vi.fn() }), })); +vi.mock("@multica/core/labels", () => ({ + useAttachLabelToIssue: () => ({ mutateAsync: mockAttachLabel }), +})); + vi.mock("@multica/core/hooks/use-file-upload", () => ({ useFileUpload: () => ({ uploadWithToast: mockUploadWithToast }), })); @@ -455,7 +460,12 @@ describe("CreateIssueModal", () => { identifier: "TES-123", title: "Ship create issue regression coverage", status: "todo", + // Current backend echoes the attached labels, so the create flow skips + // the legacy per-label attach fallback. Empty is enough — what matters + // is that the field is present (not undefined). + labels: [], }); + mockAttachLabel.mockResolvedValue({ labels: [] }); }); it("shows success feedback with a direct path to the new issue", async () => { @@ -505,6 +515,71 @@ describe("CreateIssueModal", () => { expect(mockToastDismiss).toHaveBeenCalledWith("toast-1"); }); + it("forwards selected labels in the create payload so they attach in the same transaction", async () => { + const user = userEvent.setup(); + mockDraftStore.draft.labelIds = [ + "aaaaaaaa-1111-2222-3333-444444444444", + "bbbbbbbb-1111-2222-3333-444444444444", + ]; + + renderModal(); + + fireEvent.change(screen.getByPlaceholderText("Issue title"), { + target: { value: "Labeled issue" }, + }); + await user.click(screen.getByRole("button", { name: "Create Issue" })); + + await waitFor(() => { + expect(mockCreateIssue).toHaveBeenCalledWith( + expect.objectContaining({ + title: "Labeled issue", + label_ids: [ + "aaaaaaaa-1111-2222-3333-444444444444", + "bbbbbbbb-1111-2222-3333-444444444444", + ], + }), + ); + }); + // Backend echoed `labels`, so the atomic path handled it — no legacy + // per-label attach fallback should run. + expect(mockAttachLabel).not.toHaveBeenCalled(); + }); + + it("falls back to per-label attach when an older backend omits labels from the create response", async () => { + const user = userEvent.setup(); + // Older backend: ignores label_ids and returns an issue with no `labels` + // field (the rolling-deploy window where web is ahead of the backend). + mockCreateIssue.mockResolvedValueOnce({ + id: "issue-123", + identifier: "TES-123", + title: "Labeled issue", + status: "todo", + }); + mockDraftStore.draft.labelIds = [ + "aaaaaaaa-1111-2222-3333-444444444444", + "bbbbbbbb-1111-2222-3333-444444444444", + ]; + + renderModal(); + + fireEvent.change(screen.getByPlaceholderText("Issue title"), { + target: { value: "Labeled issue" }, + }); + await user.click(screen.getByRole("button", { name: "Create Issue" })); + + await waitFor(() => { + expect(mockAttachLabel).toHaveBeenCalledTimes(2); + }); + expect(mockAttachLabel).toHaveBeenCalledWith({ + issueId: "issue-123", + labelId: "aaaaaaaa-1111-2222-3333-444444444444", + }); + expect(mockAttachLabel).toHaveBeenCalledWith({ + issueId: "issue-123", + labelId: "bbbbbbbb-1111-2222-3333-444444444444", + }); + }); + it("keeps manual mode open and clears content when create another is enabled", async () => { const user = userEvent.setup(); const onClose = vi.fn(); diff --git a/packages/views/modals/create-issue.tsx b/packages/views/modals/create-issue.tsx index d89255a25d..ab0effafa3 100644 --- a/packages/views/modals/create-issue.tsx +++ b/packages/views/modals/create-issue.tsx @@ -362,6 +362,12 @@ export function ManualCreatePanel({ start_date: startDate || undefined, due_date: dueDate || undefined, attachment_ids: activeAttachmentIds.length > 0 ? activeAttachmentIds : undefined, + // The server attaches these in the same transaction as the create and + // echoes them back as `issue.labels`, so a stale selection fails the + // create instead of leaving a committed-but-unlabeled issue. A legacy + // backend that predates this ignores the field — handled by the + // compatibility fallback below. + label_ids: labelIds.length > 0 ? labelIds : undefined, parent_issue_id: parentIssueId, // Stage is only meaningful for a sub-issue (relative to its siblings). stage: parentIssueId && stage != null ? stage : undefined, @@ -402,11 +408,15 @@ export function ManualCreatePanel({ } } - // Attach the labels chosen in the dialog. Like the sub-issue links - // above, this is deferred to after create because the new issue's ID - // doesn't exist yet, and the create endpoint takes no labels. Partial - // failures don't roll back the committed issue. - if (labelIds.length > 0) { + // Backend-compatibility fallback for the rolling deploy window: the web + // app auto-deploys on merge but the backend deploys manually, so a newer + // web build can briefly talk to a backend that predates atomic label + // creation. That backend silently ignores `label_ids` and returns an + // issue with no `labels` field. Only then do we fall back to the legacy + // per-label attach so the user's labels aren't silently dropped. When + // `labels` is present (current backend) the atomic path already ran, so + // we skip this — no double-write, no per-label fan-out. + if (labelIds.length > 0 && issue.labels === undefined) { const results = await Promise.allSettled( labelIds.map((labelId) => attachLabelMutation.mutateAsync({ issueId: issue.id, labelId }), @@ -416,7 +426,7 @@ export function ManualCreatePanel({ for (const result of results) { if (result.status === "rejected") { labelsFailed += 1; - console.error("[create-issue] label attach failed", result.reason); + console.error("[create-issue] label attach fallback failed", result.reason); } } if (labelsFailed > 0) { diff --git a/server/internal/handler/issue.go b/server/internal/handler/issue.go index 1ccccadcf1..13849c9d69 100644 --- a/server/internal/handler/issue.go +++ b/server/internal/handler/issue.go @@ -2185,6 +2185,10 @@ type CreateIssueRequest struct { StartDate *string `json:"start_date"` DueDate *string `json:"due_date"` AttachmentIDs []string `json:"attachment_ids,omitempty"` + // LabelIDs are issue-scoped labels to attach to the new issue in the same + // transaction as the create. Unknown or non-issue ids are rejected with + // 400 (service.ErrIssueLabelNotFound) rather than silently dropped. + LabelIDs []string `json:"label_ids,omitempty"` // OriginType / OriginID stamp the new issue with its provenance so // platform-internal flows can deterministically locate it later. Only // trusted callers should set these — currently the daemon CLI passes @@ -2287,6 +2291,11 @@ func (h *Handler) CreateIssue(w http.ResponseWriter, r *http.Request) { return } + labelIDs, ok := parseUUIDSliceOrBadRequest(w, req.LabelIDs, "label_ids") + if !ok { + return + } + var startDate pgtype.Date if req.StartDate != nil && *req.StartDate != "" { d, err := util.ParseCalendarDate(*req.StartDate) @@ -2405,14 +2414,21 @@ func (h *Handler) CreateIssue(w http.ResponseWriter, r *http.Request) { OriginID: originID, Stage: ptrToInt4(req.Stage), AttachmentIDs: attachmentIDs, + LabelIDs: labelIDs, AllowDuplicate: req.AllowDuplicate, }, service.IssueCreateOpts{ ActorID: actualCreatorID, AnalyticsAgentID: analyticsAgentID, Platform: func() string { p, _, _ := middleware.ClientMetadataFromContext(r.Context()); return p }(), - BroadcastPayload: func(issue db.Issue, atts []db.Attachment) map[string]any { + BroadcastPayload: func(issue db.Issue, atts []db.Attachment, labels []db.IssueLabel) map[string]any { payload := issueToResponse(issue, prefix) payload.Attachments = buildAttachmentResponses(atts) + // Carry the authoritative label snapshot so every online client + // renders the new issue already labeled. Non-nil (even empty) + // pointer = authoritative list; the old flow's separate + // issue_labels:changed broadcast is gone. + labelResponses := labelsToResponse(labels) + payload.Labels = &labelResponses return map[string]any{"issue": payload} }, }) @@ -2435,6 +2451,10 @@ func (h *Handler) CreateIssue(w http.ResponseWriter, r *http.Request) { writeError(w, http.StatusBadRequest, "project not found in this workspace") return } + if errors.Is(err, service.ErrIssueLabelNotFound) { + writeError(w, http.StatusBadRequest, "one or more labels not found in this workspace") + return + } if err != nil { slog.Warn("create issue failed", append(logger.RequestAttrs(r), "error", err, "workspace_id", workspaceID)...) writeError(w, http.StatusInternalServerError, "failed to create issue: "+err.Error()) @@ -2446,6 +2466,11 @@ func (h *Handler) CreateIssue(w http.ResponseWriter, r *http.Request) { resp := issueToResponse(issue, prefix) resp.Attachments = buildAttachmentResponses(res.Attachments) + // Echo the authoritative labels attached in the create transaction. Always + // non-nil (empty slice when none) so a newer client can tell the backend + // understood label_ids and skip its legacy post-create attach fallback. + labelResponses := labelsToResponse(res.Labels) + resp.Labels = &labelResponses writeJSON(w, http.StatusCreated, resp) } diff --git a/server/internal/handler/issue_create_labels_test.go b/server/internal/handler/issue_create_labels_test.go new file mode 100644 index 0000000000..faf2a5f63c --- /dev/null +++ b/server/internal/handler/issue_create_labels_test.go @@ -0,0 +1,268 @@ +package handler + +import ( + "context" + "encoding/json" + "net/http" + "net/http/httptest" + "testing" + "time" + + "github.com/google/uuid" + "github.com/multica-ai/multica/server/internal/events" + "github.com/multica-ai/multica/server/pkg/protocol" +) + +// createTestIssueLabel creates an issue-scoped label in the test workspace via +// the public handler and registers cleanup. It returns the new label id. +func createTestIssueLabel(t *testing.T, name string) string { + t.Helper() + w := httptest.NewRecorder() + req := newRequest("POST", "/api/labels", map[string]any{ + "name": name, + "color": "#ef4444", + }) + testHandler.CreateLabel(w, req) + if w.Code != http.StatusCreated { + t.Fatalf("CreateLabel %q: expected 201, got %d: %s", name, w.Code, w.Body.String()) + } + var created LabelResponse + if err := json.NewDecoder(w.Body).Decode(&created); err != nil { + t.Fatalf("decode label: %v", err) + } + t.Cleanup(func() { + testPool.Exec(context.Background(), `DELETE FROM issue_label WHERE id = $1`, created.ID) + }) + return created.ID +} + +func countIssueLabel(t *testing.T, issueID, labelID string) int { + t.Helper() + var n int + if err := testPool.QueryRow(context.Background(), + `SELECT COUNT(*) FROM issue_to_label WHERE issue_id = $1 AND label_id = $2`, + issueID, labelID, + ).Scan(&n); err != nil { + t.Fatalf("count issue_to_label: %v", err) + } + return n +} + +func countIssuesWithTitle(t *testing.T, title string) int { + t.Helper() + var n int + if err := testPool.QueryRow(context.Background(), + `SELECT COUNT(*) FROM issue WHERE workspace_id = $1 AND title = $2`, + testWorkspaceID, title, + ).Scan(&n); err != nil { + t.Fatalf("count issues: %v", err) + } + return n +} + +// TestCreateIssueAttachesLabelsAtomically verifies that labels passed in the +// create request are written in the same transaction as the issue, so a +// created issue already carries its labels without a second round-trip. +func TestCreateIssueAttachesLabelsAtomically(t *testing.T) { + labelA := createTestIssueLabel(t, "cl-a-"+uuid.NewString()[:8]) + labelB := createTestIssueLabel(t, "cl-b-"+uuid.NewString()[:8]) + + w := httptest.NewRecorder() + req := newRequest("POST", "/api/issues?workspace_id="+testWorkspaceID, map[string]any{ + "title": "create-labels attaches", + "status": "todo", + "priority": "low", + "label_ids": []string{labelA, labelB}, + }) + testHandler.CreateIssue(w, req) + if w.Code != http.StatusCreated { + t.Fatalf("CreateIssue: expected 201, got %d: %s", w.Code, w.Body.String()) + } + var issue IssueResponse + json.NewDecoder(w.Body).Decode(&issue) + t.Cleanup(func() { deleteTestIssue(t, issue.ID) }) + + if got := countIssueLabel(t, issue.ID, labelA); got != 1 { + t.Errorf("label A: expected 1 attachment, got %d", got) + } + if got := countIssueLabel(t, issue.ID, labelB); got != 1 { + t.Errorf("label B: expected 1 attachment, got %d", got) + } + + // The create response must echo the authoritative labels so every online + // client (not just the creator) renders the new issue already labeled and + // a newer client can tell the backend understood label_ids. + if issue.Labels == nil { + t.Fatalf("create response must include a labels field") + } + got := map[string]bool{} + for _, l := range *issue.Labels { + got[l.ID] = true + } + if !got[labelA] || !got[labelB] { + t.Errorf("response labels %v missing labelA=%s or labelB=%s", got, labelA, labelB) + } +} + +// TestCreateIssueResponseAlwaysIncludesLabelsField verifies that even when no +// labels are requested the create response carries an explicit (empty) labels +// list — the signal a newer client uses to know the backend handled labels and +// skip its legacy per-label attach fallback. +func TestCreateIssueResponseAlwaysIncludesLabelsField(t *testing.T) { + w := httptest.NewRecorder() + req := newRequest("POST", "/api/issues?workspace_id="+testWorkspaceID, map[string]any{ + "title": "create-labels no-label response", + "status": "todo", + "priority": "low", + }) + testHandler.CreateIssue(w, req) + if w.Code != http.StatusCreated { + t.Fatalf("CreateIssue: expected 201, got %d: %s", w.Code, w.Body.String()) + } + var issue IssueResponse + json.NewDecoder(w.Body).Decode(&issue) + t.Cleanup(func() { deleteTestIssue(t, issue.ID) }) + + if issue.Labels == nil { + t.Fatalf("create response must include a labels field even when empty") + } + if len(*issue.Labels) != 0 { + t.Errorf("expected empty labels, got %d", len(*issue.Labels)) + } +} + +// TestCreateIssueEventCarriesLabels verifies that the issue:created websocket +// event carries the attached labels, so online members other than the creator +// render the new issue already labeled instead of blank until a refetch (the +// old flow relied on a separate issue_labels:changed broadcast this PR removed). +func TestCreateIssueEventCarriesLabels(t *testing.T) { + label := createTestIssueLabel(t, "cl-evt-"+uuid.NewString()[:8]) + + gotEvent := make(chan events.Event, 1) + testHandler.Bus.Subscribe(protocol.EventIssueCreated, func(e events.Event) { + select { + case gotEvent <- e: + default: + } + }) + + w := httptest.NewRecorder() + req := newRequest("POST", "/api/issues?workspace_id="+testWorkspaceID, map[string]any{ + "title": "create-labels event carries", + "status": "todo", + "priority": "low", + "label_ids": []string{label}, + }) + testHandler.CreateIssue(w, req) + if w.Code != http.StatusCreated { + t.Fatalf("CreateIssue: expected 201, got %d: %s", w.Code, w.Body.String()) + } + var issue IssueResponse + json.NewDecoder(w.Body).Decode(&issue) + t.Cleanup(func() { deleteTestIssue(t, issue.ID) }) + + select { + case ev := <-gotEvent: + pm, ok := ev.Payload.(map[string]any) + if !ok { + t.Fatalf("issue:created payload has unexpected type %T", ev.Payload) + } + resp, ok := pm["issue"].(IssueResponse) + if !ok { + t.Fatalf("issue:created payload issue has unexpected type %T", pm["issue"]) + } + if resp.Labels == nil { + t.Fatalf("issue:created payload must include a labels field") + } + found := false + for _, l := range *resp.Labels { + if l.ID == label { + found = true + } + } + if !found { + t.Errorf("issue:created labels %+v missing %s", *resp.Labels, label) + } + case <-time.After(2 * time.Second): + t.Fatal("did not receive issue:created event") + } +} + +// TestCreateIssueDedupesDuplicateLabelIDs verifies that a repeated label id in +// the request attaches exactly once (ON CONFLICT DO NOTHING) and still 201s. +func TestCreateIssueDedupesDuplicateLabelIDs(t *testing.T) { + label := createTestIssueLabel(t, "cl-dup-"+uuid.NewString()[:8]) + + w := httptest.NewRecorder() + req := newRequest("POST", "/api/issues?workspace_id="+testWorkspaceID, map[string]any{ + "title": "create-labels dedupe", + "status": "todo", + "priority": "low", + "label_ids": []string{label, label}, + }) + testHandler.CreateIssue(w, req) + if w.Code != http.StatusCreated { + t.Fatalf("CreateIssue: expected 201, got %d: %s", w.Code, w.Body.String()) + } + var issue IssueResponse + json.NewDecoder(w.Body).Decode(&issue) + t.Cleanup(func() { deleteTestIssue(t, issue.ID) }) + + if got := countIssueLabel(t, issue.ID, label); got != 1 { + t.Errorf("expected label attached exactly once, got %d", got) + } +} + +// TestCreateIssueRejectsUnknownLabelWithoutCreating verifies that an unknown +// label id fails the whole create with 400 and leaves no issue behind — the +// atomicity guarantee the old post-create attach flow could not offer. +func TestCreateIssueRejectsUnknownLabelWithoutCreating(t *testing.T) { + const title = "create-labels unknown rejected" + w := httptest.NewRecorder() + req := newRequest("POST", "/api/issues?workspace_id="+testWorkspaceID, map[string]any{ + "title": title, + "status": "todo", + "priority": "low", + "label_ids": []string{uuid.NewString()}, + }) + testHandler.CreateIssue(w, req) + if w.Code != http.StatusBadRequest { + t.Fatalf("CreateIssue: expected 400, got %d: %s", w.Code, w.Body.String()) + } + if got := countIssuesWithTitle(t, title); got != 0 { + t.Errorf("expected no issue created on invalid label, found %d", got) + } +} + +// TestCreateIssueRejectsNonIssueScopedLabel verifies that a label scoped to a +// non-issue resource (e.g. agent) cannot be attached to an issue at create, +// mirroring the resource_type guard baked into AttachLabelToIssue. +func TestCreateIssueRejectsNonIssueScopedLabel(t *testing.T) { + var agentLabelID string + if err := testPool.QueryRow(context.Background(), + `INSERT INTO issue_label (workspace_id, resource_type, name, color) + VALUES ($1, 'agent', $2, '#000000') RETURNING id`, + testWorkspaceID, "create-labels agent "+uuid.NewString(), + ).Scan(&agentLabelID); err != nil { + t.Fatalf("insert agent label: %v", err) + } + t.Cleanup(func() { + testPool.Exec(context.Background(), `DELETE FROM issue_label WHERE id = $1`, agentLabelID) + }) + + const title = "create-labels wrong scope rejected" + w := httptest.NewRecorder() + req := newRequest("POST", "/api/issues?workspace_id="+testWorkspaceID, map[string]any{ + "title": title, + "status": "todo", + "priority": "low", + "label_ids": []string{agentLabelID}, + }) + testHandler.CreateIssue(w, req) + if w.Code != http.StatusBadRequest { + t.Fatalf("CreateIssue: expected 400, got %d: %s", w.Code, w.Body.String()) + } + if got := countIssuesWithTitle(t, title); got != 0 { + t.Errorf("expected no issue created on wrong-scope label, found %d", got) + } +} diff --git a/server/internal/service/issue.go b/server/internal/service/issue.go index 3e48376ac6..124d8c5ab9 100644 --- a/server/internal/service/issue.go +++ b/server/internal/service/issue.go @@ -6,6 +6,7 @@ import ( "fmt" "log/slog" + "github.com/jackc/pgx/v5" "github.com/jackc/pgx/v5/pgtype" "github.com/multica-ai/multica/server/internal/analytics" "github.com/multica-ai/multica/server/internal/events" @@ -50,22 +51,28 @@ func NewIssueService(q *db.Queries, tx TxStarter, bus *events.Bus, ac analytics. // to IssueService.Create. The handler owns the parsing step that turns its // request payload into this struct; the service stays transport-agnostic. type IssueCreateParams struct { - WorkspaceID pgtype.UUID - Title string - Description pgtype.Text - Status string - Priority string - AssigneeType pgtype.Text - AssigneeID pgtype.UUID - CreatorType string // "agent" or "member" - CreatorID pgtype.UUID - ParentIssueID pgtype.UUID - ProjectID pgtype.UUID - StartDate pgtype.Date - DueDate pgtype.Date - OriginType pgtype.Text - OriginID pgtype.UUID - AttachmentIDs []pgtype.UUID + WorkspaceID pgtype.UUID + Title string + Description pgtype.Text + Status string + Priority string + AssigneeType pgtype.Text + AssigneeID pgtype.UUID + CreatorType string // "agent" or "member" + CreatorID pgtype.UUID + ParentIssueID pgtype.UUID + ProjectID pgtype.UUID + StartDate pgtype.Date + DueDate pgtype.Date + OriginType pgtype.Text + OriginID pgtype.UUID + AttachmentIDs []pgtype.UUID + // LabelIDs are the issue-scoped labels to attach to the new issue. They + // are validated and written inside the create transaction (see Create), + // so the issue is never committed with a partial or wrong label set. An + // unknown or non-issue label id fails the whole create with + // ErrIssueLabelNotFound rather than being silently dropped. + LabelIDs []pgtype.UUID AllowDuplicate bool // Stage groups this issue into an ordered barrier group under its parent // (NULL = unstaged). See issue_child_done.go for the staged-barrier wake. @@ -82,8 +89,11 @@ type IssueCreateOpts struct { // package to depend on handler-layer types. If nil, the service // emits a minimal `{"issue_id": }` payload — enough for cache // invalidation, but front-ends that expect the full response shape - // must provide BroadcastPayload. - BroadcastPayload func(issue db.Issue, attachments []db.Attachment) map[string]any + // must provide BroadcastPayload. The labels argument is the authoritative + // snapshot attached in the create transaction, so the emitted payload can + // carry the new issue's labels and every online client renders it already + // labeled instead of blank until a refetch. + BroadcastPayload func(issue db.Issue, attachments []db.Attachment, labels []db.IssueLabel) map[string]any // ActorID overrides the actor ID used for broadcast + analytics // when it differs from the creator on the row. Agent-created issues @@ -124,6 +134,12 @@ var ErrParentIssueNotFound = errors.New("parent issue not found in this workspac // having to remember it. Callers translate this into 400. var ErrProjectNotFound = errors.New("project not found in this workspace") +// ErrIssueLabelNotFound signals that one of the supplied LabelIDs does not +// exist in the issue's workspace or is not an issue-scoped label. The whole +// create is rejected so a new issue is never born with a partial or wrong +// label set. Callers translate this into their transport's 400. +var ErrIssueLabelNotFound = errors.New("issue label not found in this workspace") + // IssueCreateResult is the typed return from IssueService.Create. // // - On the happy path: Issue is the new row, Attachments lists the @@ -131,8 +147,14 @@ var ErrProjectNotFound = errors.New("project not found in this workspace") // - On ErrActiveDuplicate: DuplicateIssue is the row that blocked the // create; Issue and Attachments are zero. type IssueCreateResult struct { - Issue db.Issue - Attachments []db.Attachment + Issue db.Issue + Attachments []db.Attachment + // Labels is the authoritative set of labels attached to the new issue in + // the create transaction (empty when none were requested). Callers echo it + // on the create response + issue:created event so every client renders the + // new issue already labeled and a new client can detect that the backend + // understood label_ids (see the create handler's compatibility contract). + Labels []db.IssueLabel DuplicateIssue *db.Issue } @@ -194,6 +216,15 @@ func (s *IssueService) Create(ctx context.Context, p IssueCreateParams, opts Iss } } + // Validate labels before we increment the issue counter so a stale or + // wrong-scope selection fails the create cheaply. The de-duplicated rows + // are attached to the issue below, inside this same transaction, and + // echoed back as the authoritative snapshot in the result. + labels, err := validateIssueLabels(ctx, qtx, p.WorkspaceID, p.LabelIDs) + if err != nil { + return IssueCreateResult{}, err + } + duplicate, found, err := issueguard.LockAndFindActiveDuplicate(ctx, qtx, p.WorkspaceID, projectID, p.ParentIssueID, p.Title, p.AllowDuplicate) if err != nil { return IssueCreateResult{}, fmt.Errorf("duplicate guard: %w", err) @@ -268,6 +299,22 @@ func (s *IssueService) Create(ctx context.Context, p IssueCreateParams, opts Iss return IssueCreateResult{}, fmt.Errorf("create issue: %w", err) } + // Attach labels inside the create transaction so the issue and its + // labels commit together — the old flow created the issue first and + // attached labels in a second, non-atomic round-trip whose partial + // failure left the issue mis-categorized. AttachLabelToIssue is + // workspace/resource_type-guarded and ON CONFLICT DO NOTHING, and the + // ids were already validated above. + for _, label := range labels { + if err := qtx.AttachLabelToIssue(ctx, db.AttachLabelToIssueParams{ + IssueID: issue.ID, + LabelID: label.ID, + WorkspaceID: p.WorkspaceID, + }); err != nil { + return IssueCreateResult{}, fmt.Errorf("attach issue label: %w", err) + } + } + if err := tx.Commit(ctx); err != nil { return IssueCreateResult{}, fmt.Errorf("commit: %w", err) } @@ -279,11 +326,54 @@ func (s *IssueService) Create(ctx context.Context, p IssueCreateParams, opts Iss actorID = util.UUIDToString(issue.CreatorID) } - s.publishIssueCreated(issue, attachments, p.CreatorType, actorID, opts) + s.publishIssueCreated(issue, attachments, labels, p.CreatorType, actorID, opts) s.captureCreatedAnalytics(issue, p.CreatorType, actorID, opts) s.maybeEnqueueOnAssign(ctx, issue, p.CreatorType, actorID) - return IssueCreateResult{Issue: issue, Attachments: attachments}, nil + return IssueCreateResult{Issue: issue, Attachments: attachments, Labels: labels}, nil +} + +// validateIssueLabels checks that every requested label exists in the +// workspace and is issue-scoped, returning the de-duplicated label rows to +// attach. Returning the full rows (not just ids) lets Create echo an +// authoritative label snapshot on the create response + issue:created event +// without a second query. It mirrors the workspace + resource_type='issue' +// guard already enforced by AttachLabelToIssue so an unknown or wrong-scope id +// surfaces as ErrIssueLabelNotFound instead of a silent no-op insert. Invalid +// (zero) UUIDs are skipped. The label count per issue is small, so a GetLabel +// per distinct id is fine and avoids introducing a new batch query. +func validateIssueLabels(ctx context.Context, qtx *db.Queries, workspaceID pgtype.UUID, labelIDs []pgtype.UUID) ([]db.IssueLabel, error) { + if len(labelIDs) == 0 { + return nil, nil + } + seen := make(map[string]struct{}, len(labelIDs)) + deduped := make([]db.IssueLabel, 0, len(labelIDs)) + for _, labelID := range labelIDs { + if !labelID.Valid { + continue + } + key := util.UUIDToString(labelID) + if _, ok := seen[key]; ok { + continue + } + seen[key] = struct{}{} + + label, err := qtx.GetLabel(ctx, db.GetLabelParams{ + ID: labelID, + WorkspaceID: workspaceID, + }) + if err != nil { + if errors.Is(err, pgx.ErrNoRows) { + return nil, ErrIssueLabelNotFound + } + return nil, fmt.Errorf("get issue label: %w", err) + } + if label.ResourceType != "issue" { + return nil, ErrIssueLabelNotFound + } + deduped = append(deduped, label) + } + return deduped, nil } // linkAttachments links the given attachment IDs to the newly created @@ -318,13 +408,13 @@ func (s *IssueService) linkAttachments(ctx context.Context, issue db.Issue, ids return list } -func (s *IssueService) publishIssueCreated(issue db.Issue, attachments []db.Attachment, creatorType, actorID string, opts IssueCreateOpts) { +func (s *IssueService) publishIssueCreated(issue db.Issue, attachments []db.Attachment, labels []db.IssueLabel, creatorType, actorID string, opts IssueCreateOpts) { if s.Bus == nil { return } var payload map[string]any if opts.BroadcastPayload != nil { - payload = opts.BroadcastPayload(issue, attachments) + payload = opts.BroadcastPayload(issue, attachments, labels) } else { // Minimal fallback so cache invalidations still fire even if the // caller forgot to supply a builder. Front-ends that expect the