diff --git a/server/cmd/migrate/main.go b/server/cmd/migrate/main.go index ee75cef99..0e3cfa7bf 100644 --- a/server/cmd/migrate/main.go +++ b/server/cmd/migrate/main.go @@ -11,6 +11,7 @@ import ( "syscall" "github.com/jackc/pgx/v5" + "github.com/jackc/pgx/v5/pgconn" "github.com/jackc/pgx/v5/pgxpool" "github.com/multica-ai/multica/server/internal/attributionbackfill" "github.com/multica-ai/multica/server/internal/chatoriginbackfill" @@ -754,7 +755,13 @@ func main() { } startupSettings := dbstartup.SettingsFromEnv() - pool, err := dbstartup.NewPool(context.Background(), dbURL, startupSettings.ConnectTimeout) + poolConfig, err := dbstartup.ParsePoolConfig(dbURL, startupSettings.ConnectTimeout) + if err != nil { + slog.Error("unable to connect to database", "error", err) + os.Exit(1) + } + poolConfig.ConnConfig.OnNotice = logMigrationNotice + pool, err := pgxpool.NewWithConfig(context.Background(), poolConfig) if err != nil { slog.Error("unable to connect to database", "error", err) os.Exit(1) @@ -817,6 +824,33 @@ func main() { fmt.Println("Done.") } +// migrationReportPrefix marks a notice a migration raises on purpose to say +// what it changed, e.g. +// +// RAISE NOTICE 'migration report: renamed % row(s)', n; +// +// Only these reach the migration log. The server's own notices cannot be told +// apart from a deliberate RAISE by condition code — "does not exist, skipping" +// and the rename notice of ADD CONSTRAINT ... USING INDEX both carry SQLSTATE +// 00000 — so the marker is explicit. +const migrationReportPrefix = "migration report: " + +// migrationReport returns the text of a notice raised with +// migrationReportPrefix, and false for every other notice. +func migrationReport(notice *pgconn.Notice) (string, bool) { + return strings.CutPrefix(notice.Message, migrationReportPrefix) +} + +// logMigrationNotice forwards migration reports to the migration log. pgx +// drops server notices unless a handler is set, so without it the per-row +// report a data-repair migration prints (e.g. 476) never reached whoever ran +// the deploy. +func logMigrationNotice(_ *pgconn.PgConn, notice *pgconn.Notice) { + if report, ok := migrationReport(notice); ok { + slog.Info("migration report", "message", report) + } +} + // runMigrations applies (direction="up") or rolls back (direction="down") // the given file list against the supplied pool, serialized through a // Postgres session-level advisory lock so multiple concurrent runners diff --git a/server/cmd/migrate/migrate_report_notice_test.go b/server/cmd/migrate/migrate_report_notice_test.go new file mode 100644 index 000000000..43b137b1d --- /dev/null +++ b/server/cmd/migrate/migrate_report_notice_test.go @@ -0,0 +1,63 @@ +package main + +import ( + "context" + "slices" + "testing" + "time" + + "github.com/jackc/pgx/v5" + "github.com/jackc/pgx/v5/pgconn" +) + +// TestMigrationReportForwardsOnlyMarkedNotices runs the notices a migration +// run really produces through the runner's filter. The server's own chatter +// cannot be filtered by condition code — "does not exist, skipping" and the +// USING INDEX rename carry SQLSTATE 00000 just like a plain RAISE — so only +// the explicit report marker may pass. (MUL-7212) +func TestMigrationReportForwardsOnlyMarkedNotices(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + + cfg, err := pgx.ParseConfig(testDatabaseURL()) + if err != nil { + t.Fatalf("parse database url: %v", err) + } + var reports, dropped []string + cfg.OnNotice = func(_ *pgconn.PgConn, notice *pgconn.Notice) { + if report, ok := migrationReport(notice); ok { + reports = append(reports, report) + return + } + dropped = append(dropped, notice.Code+" "+notice.Message) + } + conn, err := pgx.ConnectConfig(ctx, cfg) + if err != nil { + t.Skipf("could not connect to %s: %v", testDatabaseURL(), err) + } + defer conn.Close(context.Background()) + + // Temp objects only: nothing outlives the connection. + if _, err := conn.Exec(ctx, ` + CREATE TEMP TABLE IF NOT EXISTS migrate_report_probe (id int); + CREATE TEMP TABLE IF NOT EXISTS migrate_report_probe (id int); + DROP TABLE IF EXISTS migrate_report_probe_missing; + CREATE UNIQUE INDEX migrate_report_probe_id ON migrate_report_probe (id); + ALTER TABLE migrate_report_probe + ADD CONSTRAINT migrate_report_probe_pkey PRIMARY KEY USING INDEX migrate_report_probe_id; + DO $$ BEGIN + RAISE NOTICE 'unmarked chatter'; + RAISE NOTICE 'migration report: renamed % row(s)', 2; + END $$; + `); err != nil { + t.Fatalf("produce notices: %v", err) + } + + if want := []string{"renamed 2 row(s)"}; !slices.Equal(reports, want) { + t.Errorf("forwarded reports = %q, want %q", reports, want) + } + // The probe is only meaningful if the server really sent the chatter. + if len(dropped) != 4 { + t.Errorf("dropped notices = %q, want the 4 unmarked ones", dropped) + } +} diff --git a/server/cmd/multica/cmd_issue.go b/server/cmd/multica/cmd_issue.go index 2173c4413..968bd4061 100644 --- a/server/cmd/multica/cmd_issue.go +++ b/server/cmd/multica/cmd_issue.go @@ -223,7 +223,9 @@ var issueStatusCmd = &cobra.Command{ Long: "Change an issue's status. The argument is a status KEY, not its display name.\n" + "Built-in keys: backlog, todo, in_progress, in_review, done, blocked, cancelled.\n" + "A workspace may define custom statuses on top of these; their keys are shown in\n" + - "Workspace Settings > Issue Statuses, and an unknown value errors with the full list.", + "Workspace Settings > Issue Statuses, and an unknown value errors with the full list.\n" + + "The key triage is reserved for issues waiting in Triage: it cannot be set here,\n" + + "and an issue in Triage keeps its status until it is accepted.", Args: exactArgs(2), RunE: runIssueStatus, } @@ -393,7 +395,9 @@ var issueSearchCmd = &cobra.Command{ // validIssueStatuses are the 7 BUILT-IN status keys, present in every // workspace. Since MUL-6243 a workspace may define additional custom statuses, // so this is the list shown in help text and error messages, not the set of -// accepted values — see validateIssueStatus. +// accepted values — see validateIssueStatus. The reserved `triage` key is +// left out on purpose: it is readable (`issue list --status triage`) but never +// a value a write may set. var validIssueStatuses = []string{ "backlog", "todo", "in_progress", "in_review", "done", "blocked", "cancelled", } diff --git a/server/cmd/server/notification_listeners.go b/server/cmd/server/notification_listeners.go index ccec27a27..f683ff10f 100644 --- a/server/cmd/server/notification_listeners.go +++ b/server/cmd/server/notification_listeners.go @@ -30,6 +30,7 @@ var statusLabels = map[string]string{ "done": "Done", "blocked": "Blocked", "cancelled": "Cancelled", + "triage": "Triage", } // priorityLabels maps DB priority values to human-readable labels for notifications. diff --git a/server/internal/featureflags/keys.go b/server/internal/featureflags/keys.go index c9af98d73..d84db107f 100644 --- a/server/internal/featureflags/keys.go +++ b/server/internal/featureflags/keys.go @@ -23,6 +23,14 @@ const ( // gate pinned Task/Run execution: disabling discovery and management must not // mutate an immutable execution manifest that is already in flight. PluginsV1 = "plugins_v1" + // TriageV1 gates the Triage inbox (MUL-7189): creating issues into Triage, + // triage runs, and the Triage page. It is a GLOBAL switch — per-workspace + // targeting has no production wiring — which is enough because a + // workspace with no triager configured and no Triage issues sees nothing + // either way. Turning it off stops new intake and triage runs but leaves + // existing Triage issues workable; the reserved `triage` status itself is + // not gated. + TriageV1 = "triage_v1" // agentBuilderCompat is no longer a release flag. Keep publishing the key // as enabled so installed desktop clients that still gate the AI creation // entry on this config decision receive the permanently enabled behavior. @@ -58,6 +66,10 @@ func PluginsV1Enabled(ctx context.Context, flags *featureflag.Service) bool { return flags.IsEnabled(ctx, PluginsV1, false) } +func TriageV1Enabled(ctx context.Context, flags *featureflag.Service) bool { + return flags.IsEnabled(ctx, TriageV1, false) +} + func EvaluateFrontendPublicFlags(ctx context.Context, flags *featureflag.Service) map[string]bool { out := make(map[string]bool, len(frontendPublicFlags)+3) for _, key := range frontendPublicFlags { diff --git a/server/internal/handler/github.go b/server/internal/handler/github.go index 3b79f3ec6..fde2b8d97 100644 --- a/server/internal/handler/github.go +++ b/server/internal/handler/github.go @@ -1919,6 +1919,11 @@ func (h *Handler) lookupIssueByIdentifier(ctx context.Context, workspaceID pgtyp } func (h *Handler) advanceIssueToDone(ctx context.Context, issue db.Issue, workspaceID string) { + // An issue leaves Triage only by being accepted; a merged "Closes" PR + // links to it but must not move it out. (MUL-7189 §2.2) + if issue.Status == issuestatus.Triage { + return + } updated, err := h.Queries.UpdateIssueStatus(ctx, db.UpdateIssueStatusParams{ ID: issue.ID, Status: "done", diff --git a/server/internal/handler/issue.go b/server/internal/handler/issue.go index 017fdced7..8d04795b7 100644 --- a/server/internal/handler/issue.go +++ b/server/internal/handler/issue.go @@ -162,6 +162,10 @@ func (h *Handler) resolveIssueStatusKey(w http.ResponseWriter, r *http.Request, func (h *Handler) resolveIssueStatusKeyKind(w http.ResponseWriter, r *http.Request, workspaceID pgtype.UUID, status string) (string, bool, bool) { entry, err := issuestatus.Resolve(r.Context(), h.Queries, workspaceID, status) if err != nil { + if errors.Is(err, issuestatus.ErrReservedStatus) { + writeStatusReservedForTriage(w) + return "", false, false + } if errors.Is(err, issuestatus.ErrUnknownStatus) { // Labels, not bare keys: a derived key says nothing about what the // status means, so listing `in_review_2` alone leaves the caller no @@ -3161,6 +3165,10 @@ func (h *Handler) CreateIssue(w http.ResponseWriter, r *http.Request) { "the target status was archived while this request was in flight; reload the status list and retry") return } + if errors.Is(err, service.ErrStatusReservedForTriage) { + writeStatusReservedForTriage(w) + return + } if writeIssueLimitReached(w, err) { return } @@ -3434,6 +3442,13 @@ func (h *Handler) UpdateIssue(w http.ResponseWriter, r *http.Request) { var rawFields map[string]json.RawMessage json.Unmarshal(bodyBytes, &rawFields) + if prevIssue.Status == issuestatus.Triage { + if field := triageLockedField(req.Status != nil, rawFields); field != "" { + writeIssueInTriage(w, field) + return + } + } + // Pre-fill nullable fields (bare sqlc.narg) with current values params := db.UpdateIssueParams{ ID: prevIssue.ID, @@ -4180,6 +4195,9 @@ func (h *Handler) BatchUpdateIssues(w http.ResponseWriter, r *http.Request) { return } } + if !h.validateBatchTriageLocks(w, r, wsUUID, req.IssueIDs, req.Updates.Status != nil, rawUpdates) { + return + } // The batch shares one project_id, so it is checked once here rather than // per issue, and rejected instead of skipped like the per-item guards in // the loop: a foreign project invalidates the whole request. diff --git a/server/internal/handler/issue_table_group.go b/server/internal/handler/issue_table_group.go index 384eced10..95c4c49e3 100644 --- a/server/internal/handler/issue_table_group.go +++ b/server/internal/handler/issue_table_group.go @@ -114,8 +114,10 @@ func statusValueCategoryExpr(valueExpr string, customKeys map[string]string, add return b.String() } +// statusOrderExpression ranks a category in board order. Triage, which has no +// board column, sorts ahead of Unstarted: it is work nobody has taken on yet. func statusOrderExpression(categoryExpr string) string { - return "CASE " + categoryExpr + " WHEN 'unstarted' THEN 0 WHEN 'started' THEN 1 WHEN 'done' THEN 2 WHEN 'closed' THEN 3 ELSE 4 END" + return "CASE " + categoryExpr + " WHEN 'triage' THEN -1 WHEN 'unstarted' THEN 0 WHEN 'started' THEN 1 WHEN 'done' THEN 2 WHEN 'closed' THEN 3 ELSE 4 END" } // resolveStatusCategoryMaps derives BOTH shapes a category grouping needs from diff --git a/server/internal/handler/issue_triage_guard.go b/server/internal/handler/issue_triage_guard.go new file mode 100644 index 000000000..d3dc2bd68 --- /dev/null +++ b/server/internal/handler/issue_triage_guard.go @@ -0,0 +1,93 @@ +package handler + +import ( + "encoding/json" + "log/slog" + "net/http" + + "github.com/jackc/pgx/v5/pgtype" + "github.com/multica-ai/multica/server/internal/issuestatus" + "github.com/multica-ai/multica/server/internal/logger" + "github.com/multica-ai/multica/server/internal/util" + db "github.com/multica-ai/multica/server/pkg/db/generated" +) + +// Write protection for the reserved Triage status (MUL-7189 §2.2). +// +// An issue enters Triage only through Triage intake and leaves it only by being +// accepted, so ordinary issue writes are held to two rules: +// +// - no write may name `triage` as its target status. issuestatus.Resolve +// refuses the key, and resolveIssueStatusKeyKind renders that refusal, so +// every create, update and batch path is covered by the one resolver; +// - an issue in Triage keeps its status, project and parent. Those are what +// accept decides, so writing them here would bypass it. Assignee, priority, +// labels and content stay editable. +// +// A field counts as written when the request carries it, even with the value +// the issue already has. The rule stays one a client can predict — "these +// fields are read-only in Triage" — instead of depending on the current value, +// and a client showing an issue in Triage has no reason to send them. + +// triageLockedField returns the first Triage-locked field a write carries, or +// "" when it carries none. statusSet reports whether the request sets a status +// (a null status leaves it unchanged); project_id and parent_issue_id count +// when present at all, because null clears them. +func triageLockedField(statusSet bool, raw map[string]json.RawMessage) string { + if statusSet { + return "status" + } + for _, field := range []string{"project_id", "parent_issue_id"} { + if _, ok := raw[field]; ok { + return field + } + } + return "" +} + +func writeStatusReservedForTriage(w http.ResponseWriter) { + writeErrorCode(w, http.StatusBadRequest, "status_reserved_for_triage", + `status "triage" is reserved: an issue enters Triage only through Triage intake and leaves it only by being accepted`) +} + +func writeIssueInTriage(w http.ResponseWriter, field string) { + writeErrorCode(w, http.StatusBadRequest, "issue_in_triage", + "the issue is in Triage, so its "+field+" cannot be changed; accept it out of Triage first") +} + +// validateBatchTriageLocks rejects a batch that would write a Triage-locked +// field on any issue in Triage, before anything is written. A silent per-issue +// skip would report a short `{"updated": N}` with no reason attached, which is +// the same failure the batch status check rejects up front for. +func (h *Handler) validateBatchTriageLocks(w http.ResponseWriter, r *http.Request, workspaceID pgtype.UUID, issueIDs []string, statusSet bool, rawUpdates map[string]json.RawMessage) bool { + field := triageLockedField(statusSet, rawUpdates) + if field == "" { + return true + } + ids := make([]pgtype.UUID, 0, len(issueIDs)) + for _, id := range issueIDs { + // Unparseable ids are skipped by the batch loop as well. + if parsed, err := util.ParseUUID(id); err == nil { + ids = append(ids, parsed) + } + } + if len(ids) == 0 { + return true + } + rows, err := h.Queries.ListIssueGCStatuses(r.Context(), db.ListIssueGCStatusesParams{ + WorkspaceID: workspaceID, + IssueIds: ids, + }) + if err != nil { + slog.Warn("batch update issues: load statuses for triage check", append(logger.RequestAttrs(r), "error", err)...) + writeError(w, http.StatusInternalServerError, "failed to validate issues") + return false + } + for _, row := range rows { + if row.Status == issuestatus.Triage { + writeIssueInTriage(w, field) + return false + } + } + return true +} diff --git a/server/internal/handler/issue_triage_guard_test.go b/server/internal/handler/issue_triage_guard_test.go new file mode 100644 index 000000000..6a0345b0c --- /dev/null +++ b/server/internal/handler/issue_triage_guard_test.go @@ -0,0 +1,222 @@ +package handler + +import ( + "context" + "errors" + "net/http" + "testing" + + "github.com/multica-ai/multica/server/internal/issuestatus" + "github.com/multica-ai/multica/server/internal/service" + "github.com/multica-ai/multica/server/internal/testutil" +) + +// Triage write protection (MUL-7212). A triage issue can only be made by +// Triage intake, which does not exist yet, so these tests insert one directly. + +// The number comes from the workspace counter, not the fixture's MAX+1, so an +// HTTP create later in the same test cannot be handed the same number. +func triageIssueForTest(t *testing.T, title string) string { + t.Helper() + return dbfx.Issue(t, title, testutil.Cols{ + "status": issuestatus.Triage, + "number": nextWorkspaceIssueNumber(t), + }) +} + +func issueStatusOf(t *testing.T, issueID string) string { + t.Helper() + var status string + dbfx.QueryRow(t, `SELECT status FROM issue WHERE id = $1`, issueID).Scan(&status) + return status +} + +func wantErrorCode(t *testing.T, resp *testutil.Response, code string) { + t.Helper() + if got := resp.Want(http.StatusBadRequest).Map()["code"]; got != code { + t.Fatalf("error code = %v, want %q: %s", got, code, resp.Text()) + } +} + +func TestIssueWritesRefuseTheReservedTriageStatus(t *testing.T) { + seedTestCatalog(t) + + t.Run("create", func(t *testing.T) { + for _, status := range []string{"triage", " TRIAGE "} { + resp := testutil.Call(t, testHandler.CreateIssue, newRequest(http.MethodPost, "/api/issues", map[string]any{ + "title": "create into triage", "status": status, + })) + wantErrorCode(t, resp, "status_reserved_for_triage") + } + }) + + t.Run("update", func(t *testing.T) { + issueID := dbfx.Issue(t, "update into triage") + resp := testutil.Call(t, testHandler.UpdateIssue, withURLParam( + newRequest(http.MethodPut, "/api/issues/"+issueID, map[string]any{"status": "triage"}), + "id", issueID)) + wantErrorCode(t, resp, "status_reserved_for_triage") + if got := issueStatusOf(t, issueID); got != "todo" { + t.Errorf("status after refused update = %q, want todo", got) + } + }) + + t.Run("batch", func(t *testing.T) { + issueID := dbfx.Issue(t, "batch into triage") + resp := testutil.Call(t, testHandler.BatchUpdateIssues, newRequest(http.MethodPatch, + "/api/issues/batch?workspace_id="+testWorkspaceID, map[string]any{ + "issue_ids": []string{issueID}, + "updates": map[string]any{"status": "triage"}, + })) + wantErrorCode(t, resp, "status_reserved_for_triage") + if got := issueStatusOf(t, issueID); got != "todo" { + t.Errorf("status after refused batch = %q, want todo", got) + } + }) + + // Every create entry shares the rule, not only the HTTP handler that + // resolves the status first. + t.Run("service create", func(t *testing.T) { + _, err := testHandler.IssueService.Create(context.Background(), service.IssueCreateParams{ + WorkspaceID: parseUUID(testWorkspaceID), + Title: "service create into triage", + Status: issuestatus.Triage, + Priority: "none", + CreatorType: "member", + CreatorID: parseUUID(testUserID), + }, service.IssueCreateOpts{}) + if !errors.Is(err, service.ErrStatusReservedForTriage) { + t.Fatalf("IssueService.Create(triage) = %v, want ErrStatusReservedForTriage", err) + } + }) +} + +// A custom status can neither take the key nor claim Triage as its category: +// either would give the catalog a row that resolves to the reserved status. +func TestCustomStatusCannotClaimTriage(t *testing.T) { + seedTestCatalog(t) + cases := map[string]map[string]any{ + "triage category": {"name": "Foo", "key": "foo", "category": "triage", "color": "#123456"}, + "triage key": {"name": "Mine", "key": "triage", "category": "todo", "color": "#123456"}, + "name slugging onto triage": {"name": "Triage", "category": "backlog", "color": "#123456"}, + } + for name, body := range cases { + t.Run(name, func(t *testing.T) { + testutil.Call(t, testHandler.CreateIssueStatus, + newRequest(http.MethodPost, "/api/issue-statuses", body)).Want(http.StatusBadRequest) + }) + } + if n := dbfx.Count(t, `SELECT count(*) FROM issue_status WHERE workspace_id = $1 AND (key IN ('foo', 'triage') OR category = 'triage')`, testWorkspaceID); n != 0 { + t.Fatalf("%d catalog row(s) claimed triage", n) + } +} + +func TestIssueInTriageKeepsStatusProjectAndParent(t *testing.T) { + issueID := triageIssueForTest(t, "waiting in triage") + projectID := dbfx.Project(t, "triage target project") + parentID := dbfx.Issue(t, "triage target parent") + + update := func(body map[string]any) *testutil.Response { + return testutil.Call(t, testHandler.UpdateIssue, withURLParam( + newRequest(http.MethodPut, "/api/issues/"+issueID, body), "id", issueID)) + } + + for name, body := range map[string]map[string]any{ + "status": {"status": "todo"}, + "status unchanged": {"status": "triage"}, + "project": {"project_id": projectID}, + "clear project": {"project_id": nil}, + "parent": {"parent_issue_id": parentID}, + "status and title": {"status": "done", "title": "sneaky"}, + } { + t.Run(name, func(t *testing.T) { + wantErrorCode(t, update(body), "issue_in_triage") + }) + } + var status, title string + var projectSet, parentSet bool + dbfx.QueryRow(t, `SELECT status, title, project_id IS NOT NULL, parent_issue_id IS NOT NULL FROM issue WHERE id = $1`, issueID). + Scan(&status, &title, &projectSet, &parentSet) + if status != issuestatus.Triage || title != "waiting in triage" || projectSet || parentSet { + t.Fatalf("refused writes changed the issue: status=%q title=%q project=%v parent=%v", status, title, projectSet, parentSet) + } + + // Everything accept does not decide stays editable. + var resp IssueResponse + update(map[string]any{"priority": "high", "title": "retitled in triage"}).Want(http.StatusOK).JSON(&resp) + if resp.Status != issuestatus.Triage || resp.Priority != "high" || resp.Title != "retitled in triage" { + t.Fatalf("allowed update = {status:%q priority:%q title:%q}", resp.Status, resp.Priority, resp.Title) + } + // A reserved key is its own category, with no catalog read. + if resp.StatusCategory != issuestatus.Triage { + t.Errorf("status_category = %q, want triage", resp.StatusCategory) + } +} + +// A batch that would move an issue out of Triage is refused whole, before any +// write, rather than updating the rest and skipping it silently. +func TestBatchUpdateRefusesIssuesInTriage(t *testing.T) { + triageID := triageIssueForTest(t, "batch triage member") + todoID := dbfx.Issue(t, "batch todo member") + + batch := func(updates map[string]any) *testutil.Response { + return testutil.Call(t, testHandler.BatchUpdateIssues, newRequest(http.MethodPatch, + "/api/issues/batch?workspace_id="+testWorkspaceID, map[string]any{ + "issue_ids": []string{todoID, triageID}, + "updates": updates, + })) + } + + wantErrorCode(t, batch(map[string]any{"status": "done"}), "issue_in_triage") + wantErrorCode(t, batch(map[string]any{"project_id": nil}), "issue_in_triage") + if got := issueStatusOf(t, todoID); got != "todo" { + t.Errorf("refused batch still updated its other issue: status = %q", got) + } + + var out struct { + Updated int `json:"updated"` + } + batch(map[string]any{"priority": "urgent"}).Want(http.StatusOK).JSON(&out) + if out.Updated != 2 { + t.Errorf("priority batch updated %d issue(s), want 2", out.Updated) + } + if got := issueStatusOf(t, triageID); got != issuestatus.Triage { + t.Errorf("triage issue status after priority batch = %q, want triage", got) + } +} + +// An item waiting in Triage has not been taken on, so it must not block anyone +// filing the same work by hand. +func TestTriageIssueIsNotAnActiveDuplicate(t *testing.T) { + triageIssueForTest(t, "Duplicate guard ignores triage") + var created IssueResponse + testutil.Call(t, testHandler.CreateIssue, newRequest(http.MethodPost, "/api/issues", map[string]any{ + "title": "Duplicate guard ignores triage", + })).Want(http.StatusCreated).JSON(&created) + dbfx.Cleanup(t, `DELETE FROM issue WHERE id = $1`, parseUUID(created.ID)) +} + +// A merged "Closes" PR links to a triage issue but must not move it out. +func TestPullRequestMergeDoesNotAdvanceTriageIssue(t *testing.T) { + issueID := triageIssueForTest(t, "closed by a PR while in triage") + issue, err := testHandler.Queries.GetIssue(context.Background(), parseUUID(issueID)) + if err != nil { + t.Fatalf("load issue: %v", err) + } + testHandler.advanceIssueToDone(context.Background(), issue, testWorkspaceID) + if got := issueStatusOf(t, issueID); got != issuestatus.Triage { + t.Fatalf("status after merged PR = %q, want triage", got) + } +} + +// Triage has no board column; in a status sort it leads, ahead of Backlog. +func TestStatusOrderRanksTriageFirst(t *testing.T) { + rank := func(status string) int { + var got int + dbfx.QueryRow(t, `SELECT `+statusOrderExpression("$1::text"), status).Scan(&got) + return got + } + if triage, backlog := rank(issuestatus.Triage), rank(issuestatus.Backlog); triage >= backlog { + t.Fatalf("status rank triage=%d backlog=%d, want triage first", triage, backlog) + } +} diff --git a/server/internal/handler/source_context.go b/server/internal/handler/source_context.go index 6f687fb48..c86c9e60d 100644 --- a/server/internal/handler/source_context.go +++ b/server/internal/handler/source_context.go @@ -893,6 +893,9 @@ func (h *Handler) writeSourceContextError(w http.ResponseWriter, err error, limi case errors.Is(err, service.ErrParentIssueNotFound), errors.Is(err, service.ErrProjectNotFound): status = http.StatusBadRequest message = err.Error() + case errors.Is(err, service.ErrStatusReservedForTriage): + status, code = http.StatusBadRequest, "status_reserved_for_triage" + message = err.Error() case errors.Is(err, errSourceContextBadRequest): status, code = http.StatusBadRequest, "invalid_request" message = err.Error() diff --git a/server/internal/issueguard/duplicate.go b/server/internal/issueguard/duplicate.go index 75c0b1081..ee4b1ade9 100644 --- a/server/internal/issueguard/duplicate.go +++ b/server/internal/issueguard/duplicate.go @@ -42,6 +42,18 @@ func NewActiveDuplicateError(issue db.Issue, issuePrefix string) *ActiveDuplicat } } +// inactiveStatusKeys are the status keys the duplicate guards never count as an +// active duplicate: the terminal categories, plus Triage. An issue waiting in +// Triage has not been taken on yet, so it must not block anyone filing the same +// work; a duplicate there is resolved by merging it from Triage. +func inactiveStatusKeys(ctx context.Context, q *db.Queries, workspaceID pgtype.UUID) ([]string, error) { + return issuestatus.ExpandCategories(ctx, q, workspaceID, []string{ + issuestatus.CategoryDone, + issuestatus.CategoryClosed, + issuestatus.Triage, + }) +} + func LockAndFindActiveDuplicate( ctx context.Context, q *db.Queries, @@ -61,17 +73,14 @@ func LockAndFindActiveDuplicate( if allowDuplicate { return db.Issue{}, false, nil } - terminalStatusKeys, err := issuestatus.ExpandCategories(ctx, q, workspaceID, []string{ - issuestatus.CategoryDone, - issuestatus.CategoryClosed, - }) + inactiveKeys, err := inactiveStatusKeys(ctx, q, workspaceID) if err != nil { return db.Issue{}, false, err } duplicate, err := q.FindActiveDuplicateIssue(ctx, db.FindActiveDuplicateIssueParams{ WorkspaceID: workspaceID, - TerminalStatusKeys: terminalStatusKeys, + TerminalStatusKeys: inactiveKeys, ProjectID: projectID, ParentIssueID: parentIssueID, NormalizedTitle: normalizedTitle, @@ -101,17 +110,14 @@ func LockAndFindRecentAutopilotDuplicate( if err := q.LockIssueDuplicateKey(ctx, recentAutopilotLockKey(workspaceID, autopilotID, projectID, normalizedTitle)); err != nil { return db.Issue{}, false, err } - terminalStatusKeys, err := issuestatus.ExpandCategories(ctx, q, workspaceID, []string{ - issuestatus.CategoryDone, - issuestatus.CategoryClosed, - }) + inactiveKeys, err := inactiveStatusKeys(ctx, q, workspaceID) if err != nil { return db.Issue{}, false, err } duplicate, err := q.FindRecentAutopilotDuplicateIssue(ctx, db.FindRecentAutopilotDuplicateIssueParams{ WorkspaceID: workspaceID, - TerminalStatusKeys: terminalStatusKeys, + TerminalStatusKeys: inactiveKeys, OriginID: autopilotID, ProjectID: projectID, NormalizedTitle: normalizedTitle, diff --git a/server/internal/issuestatus/issuestatus.go b/server/internal/issuestatus/issuestatus.go index 8ff39bf04..20e77244e 100644 --- a/server/internal/issuestatus/issuestatus.go +++ b/server/internal/issuestatus/issuestatus.go @@ -8,6 +8,9 @@ // to done/cancelled; nonterminal custom keys remain distinct. Category resolves // lifecycle independently. WireCategory adapts response enums for installed // clients without persisting or granting legacy behavior. +// +// One key sits outside the catalog and outside the four categories: Triage, +// reserved for issues waiting in Triage — see Triage. package issuestatus import ( @@ -34,6 +37,21 @@ const ( Cancelled = "cancelled" ) +// Triage is the RESERVED status key of an issue waiting in Triage (MUL-7189). +// +// It is platform-owned like the 7 canonical keys — no workspace can mint a +// custom status with it — but it is deliberately not one of them: it has no +// catalog row, it is not one of the four lifecycle categories, and it is not a +// board column. An issue enters it only through Triage intake and leaves it +// only by being accepted, so no ordinary write may set it or move an issue out +// of it. It is its own category, which is what lets Category answer for it +// without a catalog read. +// +// Because no custom key can resolve to it, the SQL literal `status = 'triage'` +// is exactly equivalent to the Go-side check, and read paths can filter on the +// bare indexed column. +const Triage = "triage" + // The four stored lifecycle categories. const ( CategoryUnstarted = "unstarted" @@ -82,6 +100,10 @@ var categoryRank = func() map[string]int { // catalog, or present but archived. var ErrUnknownStatus = errors.New("unknown issue status") +// ErrReservedStatus is returned by Resolve for Triage: the key is valid to +// read but never a target an ordinary write may set. +var ErrReservedStatus = errors.New("issue status is reserved for triage") + // keyPattern mirrors the issue_status.key CHECK constraint. Keys are lowercase // so `multica issue status human_review` is unambiguous to type. var keyPattern = regexp.MustCompile(`^[a-z0-9][a-z0-9_]{0,31}$`) @@ -95,17 +117,21 @@ type Querier interface { ListIssueStatusKeysByCategories(ctx context.Context, arg db.ListIssueStatusKeysByCategoriesParams) ([]string, error) } -// Canonical returns the 7 built-in status keys in display order. +// Canonical returns the 7 canonical status keys in display order. Triage is +// not among them: it is not a board column and not a settings row. func Canonical() []string { out := make([]string, len(canonicalOrder)) copy(out, canonicalOrder) return out } -// IsBuiltIn reports whether key is one of the 7 canonical statuses. +// IsBuiltIn reports whether key is platform-owned: one of the 7 canonical +// statuses or the reserved Triage key. Each is its own category, so callers +// can read the category off the key without a catalog lookup, and none can be +// reused as a custom key. func IsBuiltIn(key string) bool { _, ok := canonicalRank[key] - return ok + return ok || key == Triage } // Categories returns the four lifecycle categories in display order. @@ -142,6 +168,10 @@ func CategoryForBehavior(behavior string) (string, bool) { return CategoryDone, true case Cancelled: return CategoryClosed, true + case Triage: + // Not one of the four stored categories: no catalog row may hold it, + // and the pair below keeps Triage answerable without a catalog read. + return Triage, true default: return "", false } @@ -159,13 +189,23 @@ func BehaviorsForCategory(category string) []string { return []string{Done} case CategoryClosed: return []string{Cancelled} + case Triage: + return []string{Triage} default: return nil } } // ParseCategory accepts previous API enum spellings at the boundary only. +// +// Triage is refused: CategoryForBehavior answers for it so a Triage issue can +// still be rendered and grouped, but it is not a category a caller may name — +// accepting it here would let `{key: "foo", category: "triage"}` reach the +// catalog, whose CHECK holds only the four stored categories. func ParseCategory(value string) (string, bool) { + if value == Triage { + return "", false + } if IsCategory(value) { return value, true } @@ -312,17 +352,23 @@ func DeriveKey(name, category string, taken map[string]bool) (string, error) { // base_ that is. n starts at 2 so the series reads as "the second one". // // The scan is bounded by the CATALOG, not by a policy number. Every candidate -// it tests is distinct, and at most len(taken)+7 keys can be occupied (the -// workspace's own plus the built-ins), so by the pigeonhole principle a free -// one has to turn up within that many attempts plus one. Picking a round -// constant instead would invent a cap on custom statuses that exists nowhere -// else in the product, and would fail with "provide one explicitly" — the very -// error this package was changed to stop showing a UI that has no key field. +// it tests is distinct, and only len(taken) plus the reserved names (the 7 +// canonical keys, the 4 category names and Triage) can be occupied, so by the +// pigeonhole +// principle a free one has to turn up within that many attempts plus one. +// Picking a round constant instead would invent a cap on custom statuses that +// exists nowhere else in the product, and would fail with "provide one +// explicitly" — the very error this package was changed to stop showing a UI +// that has no key field. +// +// Migration 476 applies this same rule in SQL to rename a pre-existing custom +// `triage` key; keep the two in step. func firstFreeKey(base string, taken map[string]bool) (string, error) { if !keyOccupied(base, taken) { return ValidateKey(base) } - limit := len(taken) + len(canonicalOrder) + len(categoryOrder) + 2 + // +1 for Triage, which keyOccupied also treats as taken. + limit := len(taken) + len(canonicalOrder) + len(categoryOrder) + 1 + 2 for n := 2; n <= limit; n++ { suffix := "_" + strconv.Itoa(n) candidate := truncateForSuffix(base, len(suffix)) + suffix @@ -369,7 +415,8 @@ func Ensure(ctx context.Context, q Querier, workspaceID pgtype.UUID) error { // carries. This is THE function that keeps existing logic correct: // // - a built-in key returns itself, unchanged, WITHOUT touching the database, -// so no existing code path gains a query or changes behavior; +// so no existing code path gains a query or changes behavior. Triage +// takes that path too: it has no catalog row to read; // - a custom terminal key returns done/cancelled; other custom keys stay raw. // // On an unresolvable key it returns the key unchanged. That is the fail-safe @@ -450,11 +497,18 @@ func CategoryAndName(ctx context.Context, q Querier, workspaceID pgtype.UUID, st // feature, or mid-rollout before migration 339 runs — could not create or // update an issue at all. Failing open here is limited precisely to the set // that was valid before this feature existed; anything else still needs a row. +// +// Triage never reaches the catalog: it resolves to ErrReservedStatus, so every +// write path that validates through here refuses it without having to know +// about it. func Resolve(ctx context.Context, q Querier, workspaceID pgtype.UUID, status string) (db.IssueStatus, error) { key := strings.ToLower(strings.TrimSpace(status)) if key == "" { return db.IssueStatus{}, ErrUnknownStatus } + if key == Triage { + return db.IssueStatus{}, ErrReservedStatus + } entry, err := q.GetIssueStatusEntryByKey(ctx, db.GetIssueStatusEntryByKeyParams{ WorkspaceID: workspaceID, Key: key, @@ -671,10 +725,17 @@ func (r *Resolver) Name(ctx context.Context, q Querier, status string) string { // category's column. // // An unseeded workspace yields no rows; the concrete built-in behavior keys are -// added explicitly so filtering remains complete. +// added explicitly so filtering remains complete. The reserved Triage key +// expands to exactly itself — no custom status can carry it. func ExpandCategories(ctx context.Context, q Querier, workspaceID pgtype.UUID, categories []string) ([]string, error) { behaviors := make([]string, 0, len(categories)*2) + triage := false for _, c := range categories { + if c == Triage { + // The reserved key owns no catalog row, so it expands to itself. + triage = true + continue + } if IsCategory(c) { behaviors = append(behaviors, c) continue @@ -687,6 +748,9 @@ func ExpandCategories(ctx context.Context, q Querier, workspaceID pgtype.UUID, c } } if len(behaviors) == 0 { + if triage { + return []string{Triage}, nil + } return nil, nil } seenBehaviors := make(map[string]bool, len(behaviors)) @@ -722,6 +786,9 @@ func ExpandCategories(ctx context.Context, q Querier, workspaceID pgtype.UUID, c } } } + if triage { + out = append(out, Triage) + } return out, nil } diff --git a/server/internal/issuestatus/issuestatus_test.go b/server/internal/issuestatus/issuestatus_test.go index 0baa09c2d..27841fbf9 100644 --- a/server/internal/issuestatus/issuestatus_test.go +++ b/server/internal/issuestatus/issuestatus_test.go @@ -3,6 +3,7 @@ package issuestatus import ( "context" "errors" + "slices" "strconv" "strings" "testing" @@ -17,10 +18,11 @@ var testWorkspace = pgtype.UUID{Bytes: [16]byte{1}, Valid: true} // fakeQuerier is an in-memory catalog keyed by (workspace, key). It records // lookups so a test can assert that the built-in fast path issues NO query. type fakeQuerier struct { - entries map[string]db.IssueStatus - lookups int - lists int - err error + entries map[string]db.IssueStatus + lookups int + lists int + keyLists int + err error } func newFakeQuerier(entries ...db.IssueStatus) *fakeQuerier { @@ -63,6 +65,7 @@ func (f *fakeQuerier) SeedIssueStatusEntries(_ context.Context, _ pgtype.UUID) e } func (f *fakeQuerier) ListIssueStatusKeysByCategories(_ context.Context, arg db.ListIssueStatusKeysByCategoriesParams) ([]string, error) { + f.keyLists++ if f.err != nil { return nil, f.err } @@ -208,6 +211,109 @@ func TestCategoriesCollapseBuiltInsIntoFourLifecycleGroups(t *testing.T) { } } +// Triage is reserved like a built-in but is not one of the four lifecycle +// categories. Were it one, `{key: "foo", category: "triage"}` would pass +// custom-status validation and give the catalog a row that resolves to it, +// breaking the equivalence of `status = 'triage'` with the Go check. (MUL-7212) +func TestTriageIsReservedButNotACategory(t *testing.T) { + if !IsBuiltIn(Triage) { + t.Error("triage must be built-in so no custom status can reuse the key") + } + if IsCategory(Triage) { + t.Error("triage must not be a stored category a custom status can declare") + } + if slices.Contains(Canonical(), Triage) { + t.Error("triage must not be a canonical status: it is not a board column or a settings row") + } + if slices.Contains(Categories(), Triage) { + t.Error("triage must not appear among the four lifecycle categories") + } + if _, err := ValidateKey("Triage"); err == nil { + t.Error("ValidateKey must refuse triage as a custom key") + } + if _, err := DeriveKey("Triage", CategoryUnstarted, takenSet()); err == nil { + t.Error("DeriveKey must refuse a name that slugs onto triage") + } + if _, err := DeriveKey("待分诊", Triage, takenSet()); err == nil { + t.Error("DeriveKey must refuse triage as the fallback category") + } + // A name that merely starts with it is unrelated and keeps its slug. + if got, err := DeriveKey("Triage Later", CategoryUnstarted, takenSet()); err != nil || got != "triage_later" { + t.Errorf("DeriveKey(Triage Later) = %q, %v; want triage_later", got, err) + } +} + +// Triage resolves to itself on every read path without a catalog read, and is +// never a write target. (MUL-7212) +func TestTriageResolvesWithoutTheCatalog(t *testing.T) { + ctx := context.Background() + q := newFakeQuerier(custom("human_review", InReview)) + + if got := Effective(ctx, q, testWorkspace, Triage); got != Triage { + t.Errorf("Effective(triage) = %q, want triage", got) + } + behavior, name := EffectiveAndName(ctx, q, testWorkspace, Triage) + if behavior != Triage || name != "" { + t.Errorf("EffectiveAndName(triage) = (%q, %q), want (triage, \"\") — clients localize it from the key", behavior, name) + } + // Triage is its own category, so the lifecycle question stays answerable + // for an issue whose status has no catalog row. + if got, ok := CategoryForBehavior(Triage); !ok || got != Triage { + t.Errorf("CategoryForBehavior(triage) = %q, %v; want triage, true", got, ok) + } + if got := Category(ctx, q, testWorkspace, Triage); got != Triage { + t.Errorf("Category(triage) = %q, want triage", got) + } + // ParseCategory is the input side: naming triage as a category must fail, + // or a custom status could declare it and land a row the catalog's CHECK + // rejects. + if got, ok := ParseCategory(Triage); ok { + t.Errorf("ParseCategory(triage) = %q, %v; want refused", got, ok) + } + if got := WireCategory(Triage, Triage); got != Triage { + t.Errorf("WireCategory(triage) = %q, want triage", got) + } + r := NewResolver(testWorkspace) + if got := r.Effective(ctx, q, Triage); got != Triage { + t.Errorf("Resolver.Effective(triage) = %q, want triage", got) + } + if got := r.Category(ctx, q, Triage); got != Triage { + t.Errorf("Resolver.Category(triage) = %q, want triage", got) + } + if got := r.Name(ctx, q, Triage); got != "" { + t.Errorf("Resolver.Name(triage) = %q, want empty", got) + } + for _, input := range []string{Triage, " TRIAGE "} { + if _, err := Resolve(ctx, q, testWorkspace, input); !errors.Is(err, ErrReservedStatus) { + t.Errorf("Resolve(%q) = %v, want ErrReservedStatus", input, err) + } + } + if q.lookups != 0 || q.lists != 0 { + t.Errorf("triage resolution touched the catalog: %d lookup(s), %d list(s)", q.lookups, q.lists) + } + + // A key that merely starts with triage still needs a catalog row: the + // reservation covers the exact key, not a prefix. + if _, err := Resolve(ctx, q, testWorkspace, "triage_2"); !errors.Is(err, ErrUnknownStatus) { + t.Errorf("Resolve(triage_2) with no row = %v, want ErrUnknownStatus", err) + } +} + +// With Triage occupying its key, disambiguation lands on triage_2 — the same +// replacement migration 476 picks for a workspace that already owned a custom +// `triage`. The two must agree, or a renamed status and a newly derived one +// could disagree about which key is next. +func TestFirstFreeKeySkipsTriage(t *testing.T) { + got, err := firstFreeKey(Triage, takenSet()) + if err != nil || got != "triage_2" { + t.Errorf("firstFreeKey(triage) = %q, %v; want triage_2", got, err) + } + got, err = firstFreeKey(Triage, takenSet("triage_2")) + if err != nil || got != "triage_3" { + t.Errorf("firstFreeKey(triage) with triage_2 taken = %q, %v; want triage_3", got, err) + } +} + func TestCategoryRankUsesFourLifecycleOrder(t *testing.T) { want := []string{"unstarted", "started", "done", "closed"} got := Categories() @@ -579,4 +685,27 @@ func TestExpandCategories(t *testing.T) { t.Errorf("expand of a non-category = %v, want nil", got) } }) + + t.Run("triage expands to exactly itself", func(t *testing.T) { + q := newFakeQuerier(custom("human_review", InReview)) + got, err := ExpandCategories(ctx, q, testWorkspace, []string{Triage, Triage}) + if err != nil { + t.Fatalf("expand: %v", err) + } + if !slices.Equal(got, []string{Triage}) { + t.Errorf("expand(triage) = %v, want [triage] — no custom status can carry it", got) + } + }) + + t.Run("triage rides along with real categories", func(t *testing.T) { + q := newFakeQuerier(custom("shipped", CategoryDone)) + got, err := ExpandCategories(ctx, q, testWorkspace, []string{CategoryDone, CategoryClosed, Triage}) + if err != nil { + t.Fatalf("expand: %v", err) + } + slices.Sort(got) + if want := []string{Cancelled, Done, "shipped", Triage}; !slices.Equal(got, want) { + t.Errorf("expand(done, closed, triage) = %v, want %v", got, want) + } + }) } diff --git a/server/internal/migrations/reserve_triage_status_key_migration_test.go b/server/internal/migrations/reserve_triage_status_key_migration_test.go new file mode 100644 index 000000000..afaa66781 --- /dev/null +++ b/server/internal/migrations/reserve_triage_status_key_migration_test.go @@ -0,0 +1,355 @@ +package migrations + +import ( + "context" + "os" + "slices" + "testing" + "time" + + "github.com/jackc/pgx/v5/pgxpool" +) + +const ( + triageWSBoth = "00000000-0000-0000-0000-00000000a001" // owns triage and triage_2 + triageWSOnly = "00000000-0000-0000-0000-00000000a002" // owns triage only + triageWSClean = "00000000-0000-0000-0000-00000000a003" // owns neither +) + +// triageMigrationSandbox holds a pool whose connections all work in one +// isolated schema, holding only the columns migrations 475-476 read or write. +type triageMigrationSandbox struct { + pool *pgxpool.Pool + schema string +} + +func newTriageMigrationSandbox(t *testing.T, schema string) *triageMigrationSandbox { + t.Helper() + dbURL := os.Getenv("DATABASE_URL") + if dbURL == "" { + t.Skip("integration test requires Postgres at DATABASE_URL") + } + ctx := context.Background() + cfg, err := pgxpool.ParseConfig(dbURL) + if err != nil { + t.Fatalf("parse DATABASE_URL: %v", err) + } + // Every pooled connection, not just the first, must resolve the + // migrations' unqualified table names inside the sandbox. + cfg.ConnConfig.RuntimeParams["search_path"] = schema + pool, err := pgxpool.NewWithConfig(ctx, cfg) + if err != nil { + t.Fatalf("connect to Postgres: %v", err) + } + cleanup := func() { + _, _ = pool.Exec(context.Background(), "DROP SCHEMA IF EXISTS "+schema+" CASCADE") + } + cleanup() + t.Cleanup(func() { + cleanup() + pool.Close() + }) + if _, err := pool.Exec(ctx, "CREATE SCHEMA "+schema); err != nil { + t.Fatalf("create isolated migration schema: %v", err) + } + if _, err := pool.Exec(ctx, ` + CREATE TABLE issue_status ( + id UUID NOT NULL DEFAULT gen_random_uuid(), + workspace_id UUID NOT NULL, + key TEXT NOT NULL, + category TEXT NOT NULL, + archived_at TIMESTAMPTZ, + updated_at TIMESTAMPTZ NOT NULL DEFAULT now(), + UNIQUE (workspace_id, key) + ); + CREATE TABLE issue ( + id UUID NOT NULL DEFAULT gen_random_uuid(), + workspace_id UUID NOT NULL, + title TEXT NOT NULL, + status TEXT NOT NULL, + revision BIGINT NOT NULL DEFAULT 1 + ); + CREATE TABLE issue_view ( + id UUID NOT NULL DEFAULT gen_random_uuid(), + workspace_id UUID NOT NULL, + name TEXT NOT NULL, + query JSONB NOT NULL, + revision INTEGER NOT NULL DEFAULT 1, + updated_at TIMESTAMPTZ NOT NULL DEFAULT now() + ); + `); err != nil { + t.Fatalf("create status tables: %v", err) + } + return &triageMigrationSandbox{pool: pool, schema: schema} +} + +// reserve applies the two reservation migrations the way the runner does: each +// file on its own, so 475's barrier commits before 476 scans. +func (s *triageMigrationSandbox) reserve(t *testing.T, ctx context.Context) { + t.Helper() + applyMigrationFile(t, ctx, s.pool, "475_issue_status_key_not_reserved.up.sql") + applyMigrationFile(t, ctx, s.pool, "476_reserve_triage_status_key.up.sql") +} + +func (s *triageMigrationSandbox) catalog(t *testing.T, ctx context.Context, workspaceID string) []string { + t.Helper() + var keys []string + if err := s.pool.QueryRow(ctx, ` + SELECT array_agg(key ORDER BY key) FROM issue_status WHERE workspace_id = $1 + `, workspaceID).Scan(&keys); err != nil { + t.Fatalf("read catalog: %v", err) + } + return keys +} + +func (s *triageMigrationSandbox) constraintValidated(t *testing.T, ctx context.Context) bool { + t.Helper() + var validated bool + if err := s.pool.QueryRow(ctx, ` + SELECT convalidated FROM pg_constraint + WHERE conname = 'issue_status_key_not_reserved' + AND conrelid = (quote_ident($1) || '.issue_status')::regclass + `, s.schema).Scan(&validated); err != nil { + t.Fatalf("read reservation constraint: %v", err) + } + return validated +} + +// TestReserveTriageStatusKeyMigration covers the upgrade conflict of MUL-7212: +// a workspace that already owns a custom `triage` status must come out with a +// replacement key picked like issuestatus.firstFreeKey, and its catalog, issues +// and saved views must all agree on it afterwards. +func TestReserveTriageStatusKeyMigration(t *testing.T) { + ctx := context.Background() + s := newTriageMigrationSandbox(t, "reserve_triage_status_key_migration_test") + + for _, seed := range []string{` + INSERT INTO issue_status (workspace_id, key, category, archived_at) VALUES + ($1, 'triage', 'backlog', NULL), + -- Archived rows still own their key, so triage_2 is not free. + ($1, 'triage_2', 'todo', now()), + ($2, 'triage', 'in_review', NULL), + ($3, 'todo_later', 'todo', NULL) + `, ` + INSERT INTO issue (workspace_id, title, status, revision) VALUES + ($1, 'on triage A', 'triage', 3), + ($1, 'on triage B', 'triage', 1), + ($1, 'on triage_2', 'triage_2', 1), + ($2, 'on triage C', 'triage', 1), + ($3, 'clean todo', 'todo', 1) + `, ` + INSERT INTO issue_view (workspace_id, name, query) VALUES + ($1, 'mixed', '{"statusFilters": ["todo", "triage", "triage_2"], "priorityFilters": ["high"]}'), + ($1, 'no status filter', '{"priorityFilters": ["high"]}'), + ($2, 'only triage', '{"statusFilters": ["triage"]}'), + -- A workspace without a custom triage never had a status by that + -- key, so its views are not the migration's to reinterpret. + ($3, 'stale value', '{"statusFilters": ["triage"]}') + `} { + if _, err := s.pool.Exec(ctx, seed, triageWSBoth, triageWSOnly, triageWSClean); err != nil { + t.Fatalf("seed conflicting workspaces: %v", err) + } + } + + s.reserve(t, ctx) + + if got, want := s.catalog(t, ctx, triageWSBoth), []string{"triage_2", "triage_3"}; !slices.Equal(got, want) { + t.Errorf("catalog with triage and archived triage_2 = %v, want %v", got, want) + } + if got, want := s.catalog(t, ctx, triageWSOnly), []string{"triage_2"}; !slices.Equal(got, want) { + t.Errorf("catalog with triage only = %v, want %v", got, want) + } + if !s.constraintValidated(t, ctx) { + t.Error("476 left the reservation NOT VALID") + } + + type issueState struct { + status string + revision int64 + } + issues := map[string]issueState{} + rows, err := s.pool.Query(ctx, `SELECT title, status, revision FROM issue`) + if err != nil { + t.Fatalf("read issues: %v", err) + } + for rows.Next() { + var title string + var state issueState + if err := rows.Scan(&title, &state.status, &state.revision); err != nil { + t.Fatalf("scan issue: %v", err) + } + issues[title] = state + } + rows.Close() + for title, want := range map[string]issueState{ + // Moved issues bump their revision so a stale editor conflicts. + "on triage A": {"triage_3", 4}, + "on triage B": {"triage_3", 2}, + "on triage_2": {"triage_2", 1}, + "on triage C": {"triage_2", 2}, + "clean todo": {"todo", 1}, + } { + if got := issues[title]; got != want { + t.Errorf("issue %q = %+v, want %+v", title, got, want) + } + } + + type viewState struct { + query string + revision int + } + views := map[string]viewState{} + rows, err = s.pool.Query(ctx, `SELECT name, query::text, revision FROM issue_view`) + if err != nil { + t.Fatalf("read views: %v", err) + } + for rows.Next() { + var name string + var state viewState + if err := rows.Scan(&name, &state.query, &state.revision); err != nil { + t.Fatalf("scan view: %v", err) + } + views[name] = state + } + rows.Close() + for name, want := range map[string]viewState{ + "mixed": {`{"statusFilters": ["todo", "triage_3", "triage_2"], "priorityFilters": ["high"]}`, 2}, + "no status filter": {`{"priorityFilters": ["high"]}`, 1}, + "only triage": {`{"statusFilters": ["triage_2"]}`, 2}, + "stale value": {`{"statusFilters": ["triage"]}`, 1}, + } { + if got := views[name]; got != want { + t.Errorf("view %q = %+v, want %+v", name, got, want) + } + } + + // The reservation now holds in storage: an old pod that still accepts + // `triage` as a custom key cannot recreate the conflict. + assertInsertCheckViolation(t, ctx, s.pool, + `INSERT INTO issue_status (workspace_id, key, category) VALUES ($1, 'triage', 'todo')`, triageWSClean) + + applyMigrationFile(t, ctx, s.pool, "476_reserve_triage_status_key.down.sql") + applyMigrationFile(t, ctx, s.pool, "475_issue_status_key_not_reserved.down.sql") + if _, err := s.pool.Exec(ctx, + `INSERT INTO issue_status (workspace_id, key, category) VALUES ($1, 'triage', 'todo')`, triageWSClean); err != nil { + t.Fatalf("insert after rollback dropped the reservation: %v", err) + } + // Re-applying handles the row the rollback window let in. + s.reserve(t, ctx) + if got, want := s.catalog(t, ctx, triageWSClean), []string{"todo_later", "triage_2"}; !slices.Equal(got, want) { + t.Errorf("catalog after re-apply = %v, want %v", got, want) + } +} + +// TestReserveTriageStatusKeySurvivesAConcurrentCreate is the rollout race: an +// old pod, which still accepts `triage` as a custom key, creates one in a +// workspace that had no conflict while the migrations run. Scanning first and +// adding the CHECK last let that row fail the final validation and abort the +// deploy. With the barrier committed before the scan, the in-flight create is +// either repaired by 476 or refused outright — the deploy never fails on it. +func TestReserveTriageStatusKeySurvivesAConcurrentCreate(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() + s := newTriageMigrationSandbox(t, "reserve_triage_status_key_race_test") + + migrator, err := s.pool.Acquire(ctx) + if err != nil { + t.Fatalf("acquire migrator connection: %v", err) + } + defer migrator.Release() + oldPod, err := s.pool.Acquire(ctx) + if err != nil { + t.Fatalf("acquire old-pod connection: %v", err) + } + defer oldPod.Release() + var migratorPID int32 + if err := migrator.QueryRow(ctx, `SELECT pg_backend_pid()`).Scan(&migratorPID); err != nil { + t.Fatalf("read migrator pid: %v", err) + } + + // The old pod is mid-create when the migration starts. + tx, err := oldPod.Begin(ctx) + if err != nil { + t.Fatalf("begin old-pod create: %v", err) + } + defer tx.Rollback(context.Background()) + if _, err := tx.Exec(ctx, + `INSERT INTO issue_status (workspace_id, key, category) VALUES ($1, 'triage', 'todo')`, triageWSClean); err != nil { + t.Fatalf("old-pod create: %v", err) + } + + barrierSQL := readMigrationFile(t, "475_issue_status_key_not_reserved.up.sql") + barrier := make(chan error, 1) + go func() { + _, err := migrator.Exec(ctx, barrierSQL) + barrier <- err + }() + + // The barrier needs the table to itself, so it waits for the create. + for { + var waiting bool + if err := s.pool.QueryRow(ctx, + `SELECT wait_event_type = 'Lock' FROM pg_stat_activity WHERE pid = $1`, migratorPID).Scan(&waiting); err != nil { + t.Fatalf("watch migrator: %v", err) + } + if waiting { + break + } + select { + case err := <-barrier: + t.Fatalf("barrier finished before the in-flight create committed: %v", err) + case <-ctx.Done(): + t.Fatal("migrator never waited on the in-flight create") + case <-time.After(20 * time.Millisecond): + } + } + if err := tx.Commit(ctx); err != nil { + t.Fatalf("commit old-pod create: %v", err) + } + if err := <-barrier; err != nil { + t.Fatalf("apply barrier after a concurrent create: %v", err) + } + + // From here on the old pod cannot create another one anywhere. + assertInsertCheckViolation(t, ctx, oldPod, + `INSERT INTO issue_status (workspace_id, key, category) VALUES ($1, 'triage', 'todo')`, triageWSOnly) + + applyMigrationFile(t, ctx, migrator, "476_reserve_triage_status_key.up.sql") + if got, want := s.catalog(t, ctx, triageWSClean), []string{"triage_2"}; !slices.Equal(got, want) { + t.Errorf("catalog after the race = %v, want the in-flight create renamed to [triage_2]", got) + } + if !s.constraintValidated(t, ctx) { + t.Error("476 left the reservation NOT VALID") + } +} + +// TestIssueEffectiveStatusTriageMigration pins the SQL mirror of +// issuestatus.Effective: `triage` resolves to itself on the fast path, and +// custom keys still resolve through the catalog. +func TestIssueEffectiveStatusTriageMigration(t *testing.T) { + ctx := context.Background() + s := newTriageMigrationSandbox(t, "issue_effective_status_triage_migration_test") + if _, err := s.pool.Exec(ctx, + `INSERT INTO issue_status (workspace_id, key, category) VALUES ($1, 'shipped', 'done')`, triageWSBoth); err != nil { + t.Fatalf("seed catalog: %v", err) + } + + applyMigrationFile(t, ctx, s.pool, "477_issue_effective_status_triage.up.sql") + + cases := map[string]string{"triage": "triage", "todo": "todo", "shipped": "done", "ghost": "ghost"} + for status, want := range cases { + var got string + if err := s.pool.QueryRow(ctx, `SELECT issue_effective_status($1, $2)`, triageWSBoth, status).Scan(&got); err != nil { + t.Fatalf("issue_effective_status(%q): %v", status, err) + } + if got != want { + t.Errorf("issue_effective_status(%q) = %q, want %q", status, got, want) + } + } + + applyMigrationFile(t, ctx, s.pool, "477_issue_effective_status_triage.down.sql") + var got string + if err := s.pool.QueryRow(ctx, `SELECT issue_effective_status($1, 'shipped')`, triageWSBoth).Scan(&got); err != nil || got != "done" { + t.Fatalf("after rollback issue_effective_status(shipped) = %q, %v; want done", got, err) + } +} diff --git a/server/internal/service/issue.go b/server/internal/service/issue.go index 6e1e1253c..ce02e9898 100644 --- a/server/internal/service/issue.go +++ b/server/internal/service/issue.go @@ -164,6 +164,11 @@ var ErrIssueLabelNotFound = errors.New("issue label not found in this workspace" // it arrived, so retrying against the refreshed catalog is the remedy. var ErrIssueStatusUnavailable = errors.New("issue status is no longer available") +// ErrStatusReservedForTriage signals a create that asked for the reserved +// `triage` status. An issue enters Triage only through Triage intake, never +// through an ordinary create; callers translate this into a 400. +var ErrStatusReservedForTriage = errors.New("status triage is reserved for Triage intake") + var ErrSourceContextAlreadyAttached = errors.New("source context is already attached") // IssueCreateResult is the typed return from IssueService.Create. @@ -211,6 +216,11 @@ type IssueCreateResult struct { // Caller-owned validation is limited to transport-shaped checks: title // required, RFC3339 date format, assignee pair sanity. func (s *IssueService) Create(ctx context.Context, p IssueCreateParams, opts IssueCreateOpts) (IssueCreateResult, error) { + // Checked here as well as at the transport so every create entry shares + // the rule; the built-in skip below would otherwise let the key through. + if p.Status == issuestatus.Triage { + return IssueCreateResult{}, ErrStatusReservedForTriage + } issueCountPolicy := ResolveIssueCountPolicy(ctx, s.Entitlements, p.WorkspaceID) tx, err := s.TxStarter.Begin(ctx) if err != nil { diff --git a/server/migrations/475_issue_status_key_not_reserved.down.sql b/server/migrations/475_issue_status_key_not_reserved.down.sql new file mode 100644 index 000000000..30230ca04 --- /dev/null +++ b/server/migrations/475_issue_status_key_not_reserved.down.sql @@ -0,0 +1 @@ +ALTER TABLE issue_status DROP CONSTRAINT IF EXISTS issue_status_key_not_reserved; diff --git a/server/migrations/475_issue_status_key_not_reserved.up.sql b/server/migrations/475_issue_status_key_not_reserved.up.sql new file mode 100644 index 000000000..4a827770f --- /dev/null +++ b/server/migrations/475_issue_status_key_not_reserved.up.sql @@ -0,0 +1,21 @@ +-- Reserve the `triage` status key, step 1 of 2: the write barrier (MUL-7212). +-- +-- `triage` becomes a platform-reserved key: the status of an issue waiting in +-- Triage. It has no catalog row, it is not a category, and the server refuses +-- it as a custom key. Before this change nothing reserved it, so a workspace +-- may already own a CUSTOM status keyed `triage`; migration 476 renames those. +-- +-- The CHECK lands first, NOT VALID and in a file of its own, so it commits +-- before 476 scans for conflicts. From then on no pod — including an old one +-- still running during the rollout, which accepts `triage` as a custom key — +-- can create or keep writing a `triage` row, and the conflict set 476 scans +-- cannot grow behind its back. A single migration that scanned first and added +-- the CHECK last left that window open: a row created in between failed the +-- final validation and aborted the deploy. +-- +-- NOT VALID skips the existing rows, which still hold the key until 476 +-- renames them and validates the constraint. Until then an old pod updating one +-- of those rows (rename, archive, reorder) is refused by the CHECK; the window +-- is the gap between two files of one migration run. +ALTER TABLE issue_status + ADD CONSTRAINT issue_status_key_not_reserved CHECK (key <> 'triage') NOT VALID; diff --git a/server/migrations/476_reserve_triage_status_key.down.sql b/server/migrations/476_reserve_triage_status_key.down.sql new file mode 100644 index 000000000..866e9dfce --- /dev/null +++ b/server/migrations/476_reserve_triage_status_key.down.sql @@ -0,0 +1,4 @@ +-- Intentionally empty. Workspaces whose custom `triage` status was renamed keep +-- the replacement key: restoring `triage` would hand their issues back to a key +-- the Triage feature owns. The constraint this migration validated is dropped +-- by 475's down migration. diff --git a/server/migrations/476_reserve_triage_status_key.up.sql b/server/migrations/476_reserve_triage_status_key.up.sql new file mode 100644 index 000000000..7c3ee7153 --- /dev/null +++ b/server/migrations/476_reserve_triage_status_key.up.sql @@ -0,0 +1,90 @@ +-- Reserve the `triage` status key, step 2 of 2: repair the conflicts and +-- validate the barrier (MUL-7212, design MUL-7189 §2.1). +-- +-- Migration 475 already refuses any new `triage` row, so the set of workspaces +-- that own a custom `triage` is fixed by the time this file scans for it. Once +-- the new server reads that key as "in Triage", every issue on such a custom +-- status would silently change meaning, so this migration runs before any +-- server that reserves the key starts (the entrypoint finishes migrations +-- first). The file executes as one implicit transaction, and for every +-- workspace that owns a custom `triage` it: +-- +-- 1. picks a replacement key with the same rule as issuestatus.firstFreeKey: +-- `triage_2`, `triage_3`, ... — the first one this workspace does not own, +-- counting archived rows, because the (workspace_id, key) index is not +-- partial. A fixed name such as `triage_legacy` could collide; +-- 2. renames the catalog row, moves the workspace's issues on `triage` to +-- the replacement, and rewrites `triage` inside saved views' +-- query.statusFilters — the only other place a status key is stored as +-- live configuration. activity_log / inbox_item details are history and +-- are left as written; +-- 3. reports the counts for the workspace in the migration log. +-- +-- Each workspace's catalog is taken under the same EXCLUSIVE advisory lock +-- that archive holds. Old pods still running during the rollout take the +-- SHARED side and re-resolve a custom status before writing an issue onto it, +-- so an in-flight write either commits before the rename (and is moved with +-- the rest) or re-resolves after it and is refused. No issue can be left on +-- the old key. +-- +-- VALIDATE then proves no `triage` row is left. It takes SHARE UPDATE +-- EXCLUSIVE, so it does not block reads or writes of the catalog. +-- +-- issue.revision is bumped on the moved issues, so a client editing with the +-- old status in hand gets a revision conflict instead of writing it back. +-- The rename is not reversed by the down migration; the replacement key is an +-- ordinary custom key the admin can keep or rename. + +DO $$ +DECLARE + legacy RECORD; + n INT; + replacement TEXT; + moved_issues BIGINT; + rewritten_views BIGINT; +BEGIN + FOR legacy IN + SELECT id, workspace_id FROM issue_status WHERE key = 'triage' ORDER BY workspace_id + LOOP + PERFORM pg_advisory_xact_lock(hashtextextended(legacy.workspace_id::text || ':issue_status', 0)); + + n := 2; + WHILE EXISTS ( + SELECT 1 FROM issue_status + WHERE workspace_id = legacy.workspace_id AND key = 'triage_' || n + ) LOOP + n := n + 1; + END LOOP; + replacement := 'triage_' || n; + + UPDATE issue_status + SET key = replacement, updated_at = now() + WHERE id = legacy.id; + + UPDATE issue + SET status = replacement, revision = revision + 1 + WHERE workspace_id = legacy.workspace_id AND status = 'triage'; + GET DIAGNOSTICS moved_issues = ROW_COUNT; + + UPDATE issue_view + SET query = jsonb_set(query, '{statusFilters}', ( + SELECT jsonb_agg( + CASE WHEN f.value = to_jsonb('triage'::text) THEN to_jsonb(replacement) ELSE f.value END + ORDER BY f.ordinality) + FROM jsonb_array_elements(query -> 'statusFilters') WITH ORDINALITY AS f(value, ordinality) + )), + revision = revision + 1, + updated_at = now() + WHERE workspace_id = legacy.workspace_id + AND jsonb_typeof(query -> 'statusFilters') = 'array' + AND query -> 'statusFilters' @> '["triage"]'::jsonb; + GET DIAGNOSTICS rewritten_views = ROW_COUNT; + + -- The "migration report: " prefix is what cmd/migrate forwards to its + -- log; other server notices are dropped. + RAISE NOTICE 'migration report: reserve triage status key: workspace % renamed custom status triage to %, moved % issue(s), rewrote % saved view(s)', + legacy.workspace_id, replacement, moved_issues, rewritten_views; + END LOOP; +END $$; + +ALTER TABLE issue_status VALIDATE CONSTRAINT issue_status_key_not_reserved; diff --git a/server/migrations/477_issue_effective_status_triage.down.sql b/server/migrations/477_issue_effective_status_triage.down.sql new file mode 100644 index 000000000..12c410cb2 --- /dev/null +++ b/server/migrations/477_issue_effective_status_triage.down.sql @@ -0,0 +1,12 @@ +-- Restore migration 469's body, whose fast path knows only the 7 built-in keys. +CREATE OR REPLACE FUNCTION issue_effective_status(p_workspace_id UUID, p_status TEXT) +RETURNS TEXT LANGUAGE sql STABLE PARALLEL SAFE AS $function$ + SELECT CASE + WHEN p_status IN ('backlog', 'todo', 'in_progress', 'in_review', 'done', 'blocked', 'cancelled') THEN p_status + ELSE COALESCE((SELECT CASE s.category + WHEN 'done' THEN 'done' + WHEN 'closed' THEN 'cancelled' + ELSE p_status END + FROM issue_status s WHERE s.workspace_id = p_workspace_id AND s.key = p_status), p_status) + END +$function$; diff --git a/server/migrations/477_issue_effective_status_triage.up.sql b/server/migrations/477_issue_effective_status_triage.up.sql new file mode 100644 index 000000000..471a83518 --- /dev/null +++ b/server/migrations/477_issue_effective_status_triage.up.sql @@ -0,0 +1,17 @@ +-- Teach the SQL mirror of issuestatus.Effective about the reserved `triage` +-- key (MUL-7212, design MUL-7189 §2.1). Like the Go resolver, it is its own +-- category and never looked up: migrations 475-476 guarantee no catalog row can +-- hold it, so the fast path is exact rather than a guess. The rest of the body +-- is migration 469's, which projects custom terminal categories onto +-- done/cancelled. +CREATE OR REPLACE FUNCTION issue_effective_status(p_workspace_id UUID, p_status TEXT) +RETURNS TEXT LANGUAGE sql STABLE PARALLEL SAFE AS $function$ + SELECT CASE + WHEN p_status IN ('backlog', 'todo', 'in_progress', 'in_review', 'done', 'blocked', 'cancelled', 'triage') THEN p_status + ELSE COALESCE((SELECT CASE s.category + WHEN 'done' THEN 'done' + WHEN 'closed' THEN 'cancelled' + ELSE p_status END + FROM issue_status s WHERE s.workspace_id = p_workspace_id AND s.key = p_status), p_status) + END +$function$;