From d6ecf4bc8a99345be85d5b3612e49d52bedae3b2 Mon Sep 17 00:00:00 2001 From: Bohan Jiang <52446949+Bohan-J@users.noreply.github.com> Date: Fri, 28 Aug 2026 14:24:32 +0800 Subject: [PATCH] fix(issues): derive a usable status key from non-ASCII display names (MUL-6749) (#7638) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Creating a custom issue status whose display name is written entirely in a non-Latin script — 客户确认, 고객 확인, 確認待ち — failed with "cannot derive a status key from that name; provide one explicitly". The key alphabet is ASCII, so such a name slugified to nothing, and the settings form exposes no field for an explicit key: there was no way to create the status through the UI at all. The key stays an ASCII machine handle — it is what issue.status stores, what the CLI takes as an argument, and what the column CHECK permits — so there is NO migration. When the name has nothing to slug the key falls back to the status's CATEGORY plus an ordinal (in_review_2), which keeps saying which platform behavior the status inherits. Also fixed in the same path: - Two names that collapse onto one slug ("待客户 Review" and "待供应商 Review" both slug to "review") no longer make the second create fail with a conflict blaming a display name nobody took. Disambiguation counts ARCHIVED rows, since idx_issue_status_workspace_key is not partial. - Every create takes the exclusive catalog lock, not just the deriving one, so an explicit-key insert cannot land between a derive's catalog read and its insert and hand a 409 to a form with no key field. - The ordinal scan is bounded by the catalog rather than a constant. A derived key is opaque, so the places that printed a key alone now carry the name: issue payloads gain status_name (HTTP and background events alike, from one catalog read), the unknown-status 400 lists `key (Name)`, creating a status surfaces the key it minted, and `multica issue status --help` says the argument is a key rather than a display name. Fixes #7627. Supersedes #7636. --- packages/core/api/schemas.test.ts | 29 ++ packages/core/api/schemas.ts | 12 + packages/core/types/issue.ts | 9 + packages/views/locales/en/settings.json | 1 + packages/views/locales/ja/settings.json | 1 + packages/views/locales/ko/settings.json | 1 + packages/views/locales/zh-Hans/settings.json | 1 + .../components/issue-statuses-tab.tsx | 17 +- server/cmd/multica/cmd_issue.go | 6 +- server/internal/handler/issue.go | 40 +- server/internal/handler/issue_status.go | 93 +++- server/internal/handler/issue_status_test.go | 454 +++++++++++++++++- server/internal/issuestatus/issuestatus.go | 245 +++++++++- .../internal/issuestatus/issuestatus_test.go | 198 +++++++- server/internal/service/autopilot.go | 2 +- server/internal/service/issue.go | 2 +- server/internal/service/task.go | 27 +- 17 files changed, 1066 insertions(+), 72 deletions(-) diff --git a/packages/core/api/schemas.test.ts b/packages/core/api/schemas.test.ts index 779f04b92..fcf92453e 100644 --- a/packages/core/api/schemas.test.ts +++ b/packages/core/api/schemas.test.ts @@ -167,6 +167,35 @@ describe("ChatSessionSchema", () => { }); }); describe("IssueSchema (via ListIssuesResponseSchema)", () => { + // A custom status key can be derived rather than readable — "客户确认" becomes + // `in_review_2` — so the display name travels with it. The field has to + // survive a server that predates it, since an issue that fails validation + // degrades to a stub rather than losing one field. (MUL-6749) + it("carries a custom status's display name", () => { + const parsed = ListIssuesResponseSchema.parse({ + issues: [{ ...baseIssue, status: "in_review_2", status_name: "客户确认" }], + total: 1, + }); + expect(parsed.issues[0]?.status_name).toBe("客户确认"); + }); + it("drops only a malformed status_name, keeping the issue and the list", () => { + for (const bad of [42, { name: "x" }, ["x"], true]) { + const parsed = ListIssuesResponseSchema.parse({ + issues: [{ ...baseIssue, status: "in_review_2", status_name: bad }], + total: 1, + }); + expect(parsed.issues).toHaveLength(1); + expect(parsed.issues[0]?.id).toBe(baseIssue.id); + expect(parsed.issues[0]?.status).toBe("in_review_2"); + expect(parsed.issues[0]?.status_name).toBeUndefined(); + } + }); + it("still parses an issue from a server that does not send status_name", () => { + const { status_name: _omitted, ...withoutName } = { ...baseIssue, status_name: "x" }; + const parsed = ListIssuesResponseSchema.parse({ issues: [withoutName], total: 1 }); + expect(parsed.issues[0]?.id).toBe(baseIssue.id); + expect(parsed.issues[0]?.status_name).toBeUndefined(); + }); it("keeps the issue while independently dropping a malformed source context", () => { const parsed = ListIssuesResponseSchema.parse({ issues: [{ ...baseIssue, source_context: { snapshot: "bad" } }], diff --git a/packages/core/api/schemas.ts b/packages/core/api/schemas.ts index 99fbcbebc..d8eb94485 100644 --- a/packages/core/api/schemas.ts +++ b/packages/core/api/schemas.ts @@ -1206,6 +1206,18 @@ export const IssueSchema = z.object({ // consumers must fall back to `status` rather than treat "" as a category. // (MUL-6243) status_category: z.string().optional(), + // A CUSTOM status's display name; "" for a built-in, which clients localize + // from the key. Optional so a response from a server that predates the field + // still validates. + // + // .catch(undefined) because this is ADDITIVE display data and the failure it + // guards against is disproportionate: a parse failure anywhere in IssueSchema + // takes the whole response through parseWithFallback to EMPTY_LIST_ISSUES_RESPONSE, + // so one malformed name from a mixed-version deploy would blank an entire + // issue list. Same treatment as source_context and labels above. Nothing reads + // this field to make a decision — useStatusLabel resolves the label from the + // catalog — so dropping it costs a fallback to the key. (MUL-6749) + status_name: z.string().optional().catch(undefined), priority: z.string(), assignee_type: z.string().nullable(), assignee_id: z.string().nullable(), diff --git a/packages/core/types/issue.ts b/packages/core/types/issue.ts index bbe833e1c..6f24af6fa 100644 --- a/packages/core/types/issue.ts +++ b/packages/core/types/issue.ts @@ -172,6 +172,15 @@ export interface Issue { * (MUL-6243) */ status_category?: IssueStatusCategory; + /** + * A CUSTOM status's display name, carried beside the key. Empty for the 7 + * built-ins, which are localized from the key — prefer `useStatusLabel`, + * which handles both and stays correct when an admin renames a status. + * + * Optional only for compatibility with a server that predates it; a current + * server always sends the field. (MUL-6749) + */ + status_name?: string; priority: IssuePriority; assignee_type: IssueAssigneeType | null; assignee_id: string | null; diff --git a/packages/views/locales/en/settings.json b/packages/views/locales/en/settings.json index eeb7fa178..c7fc28205 100644 --- a/packages/views/locales/en/settings.json +++ b/packages/views/locales/en/settings.json @@ -21,6 +21,7 @@ "name": "Name", "name_placeholder": "e.g. Code Review", "key_hint": "Referred to as {{key}} by the API and the CLI.", + "created": "Status created. The API and the CLI refer to it as {{key}}.", "category": "Category", "category_locked": "Category cannot be changed — issues already on this status depend on it.", "description": "Description", diff --git a/packages/views/locales/ja/settings.json b/packages/views/locales/ja/settings.json index 1fced4f59..d344e446b 100644 --- a/packages/views/locales/ja/settings.json +++ b/packages/views/locales/ja/settings.json @@ -21,6 +21,7 @@ "name": "名前", "name_placeholder": "例: Code Review", "key_hint": "API と CLI では {{key}} として参照されます。", + "created": "ステータスを作成しました。API と CLI では {{key}} として参照されます。", "category": "カテゴリ", "category_locked": "カテゴリは変更できません。すでにこのステータスのタスクが依存しています。", "description": "説明", diff --git a/packages/views/locales/ko/settings.json b/packages/views/locales/ko/settings.json index d37591e79..c131e7b02 100644 --- a/packages/views/locales/ko/settings.json +++ b/packages/views/locales/ko/settings.json @@ -21,6 +21,7 @@ "name": "이름", "name_placeholder": "예: Code Review", "key_hint": "API와 CLI에서는 {{key}}로 참조합니다.", + "created": "상태를 만들었습니다. API와 CLI에서는 {{key}}로 참조합니다.", "category": "카테고리", "category_locked": "카테고리는 변경할 수 없습니다. 이미 이 상태인 태스크가 이에 의존합니다.", "description": "설명", diff --git a/packages/views/locales/zh-Hans/settings.json b/packages/views/locales/zh-Hans/settings.json index 136aefa27..58da2b3ea 100644 --- a/packages/views/locales/zh-Hans/settings.json +++ b/packages/views/locales/zh-Hans/settings.json @@ -21,6 +21,7 @@ "name": "名称", "name_placeholder": "例如：Code Review", "key_hint": "API 和 CLI 中通过 {{key}} 引用。", + "created": "状态已创建。API 和 CLI 中通过 {{key}} 引用。", "category": "分类", "category_locked": "分类不可修改 —— 已经处于该状态的任务依赖它。", "description": "描述", diff --git a/packages/views/settings/components/issue-statuses-tab.tsx b/packages/views/settings/components/issue-statuses-tab.tsx index 6b9ba38d4..719e152eb 100644 --- a/packages/views/settings/components/issue-statuses-tab.tsx +++ b/packages/views/settings/components/issue-statuses-tab.tsx @@ -524,7 +524,22 @@ function StatusEditorDialog({ category: draft.category, color: draft.color, }, - { onSuccess: () => onOpenChange(false), onError }, + { + onSuccess: (created) => { + onOpenChange(false); + // The key is derived server-side and is the only handle the API and + // the CLI accept, so creation has to say what it minted. A name with + // no ASCII to slug gets one that cannot be guessed back from the name + // — "客户确认" becomes `in_review_2` (MUL-6749) — so staying silent + // would leave the admin no way to learn it short of reopening the + // row. The dialog is already closing; a toast is the one surface + // still visible. + if (created?.key) { + toast.success(t(($) => $.issue_statuses.editor.created, { key: created.key })); + } + }, + onError, + }, ); }; diff --git a/server/cmd/multica/cmd_issue.go b/server/cmd/multica/cmd_issue.go index 9f5ccdf99..26f437309 100644 --- a/server/cmd/multica/cmd_issue.go +++ b/server/cmd/multica/cmd_issue.go @@ -220,8 +220,10 @@ var issueAssignCmd = &cobra.Command{ var issueStatusCmd = &cobra.Command{ Use: "status ", Short: "Change issue status", - Long: "Change an issue's status. Valid statuses: " + - "backlog, todo, in_progress, in_review, done, blocked, cancelled.", + 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.", Args: exactArgs(2), RunE: runIssueStatus, } diff --git a/server/internal/handler/issue.go b/server/internal/handler/issue.go index e9790341a..bd615656c 100644 --- a/server/internal/handler/issue.go +++ b/server/internal/handler/issue.go @@ -46,15 +46,29 @@ type IssueResponse struct { // category for a custom status. Omitted when the endpoint does not resolve // it, so consumers must fall back to Status rather than assume a blank // value means "no category". (MUL-6243) - StatusCategory string `json:"status_category,omitempty"` - Priority string `json:"priority"` - AssigneeType *string `json:"assignee_type"` - AssigneeID *string `json:"assignee_id"` - CreatorType string `json:"creator_type"` - CreatorID string `json:"creator_id"` - ParentIssueID *string `json:"parent_issue_id"` - ProjectID *string `json:"project_id"` - Position float64 `json:"position"` + StatusCategory string `json:"status_category,omitempty"` + // StatusName is a CUSTOM status's display name, carried beside the key so a + // consumer that only ever sees `status` is not left holding a bare handle. + // A key derived from a non-Latin name is opaque by construction + // (`in_review_2`), and an agent reading an issue has nothing else to match + // against the status a human named for it. + // + // Always emitted, unlike StatusCategory. Empty is a MEANING here — "this is + // a built-in, localize it from the key" — not the "this endpoint did not + // resolve it" that an absent status_category signals. Keeping the key + // present is also what lets TestIssueToMap_KeysMatchIssueResponse see the + // field at all: with omitempty a built-in fixture hides it from BOTH + // renderings, and the drift guard goes green on a payload that has drifted. + // (MUL-6749) + StatusName string `json:"status_name"` + Priority string `json:"priority"` + AssigneeType *string `json:"assignee_type"` + AssigneeID *string `json:"assignee_id"` + CreatorType string `json:"creator_type"` + CreatorID string `json:"creator_id"` + ParentIssueID *string `json:"parent_issue_id"` + ProjectID *string `json:"project_id"` + Position float64 `json:"position"` // Stage groups sub-issues under the same parent into ordered barrier // groups (null = unstaged). See issue_child_done.go for how a closed // stage gates the child-done -> parent wake. @@ -129,7 +143,10 @@ func (h *Handler) resolveIssueStatusKeyKind(w http.ResponseWriter, r *http.Reque entry, err := issuestatus.Resolve(r.Context(), h.Queries, workspaceID, status) if err != nil { if errors.Is(err, issuestatus.ErrUnknownStatus) { - allowed, listErr := issuestatus.ActiveKeys(r.Context(), h.Queries, workspaceID) + // Labels, not bare keys: a derived key says nothing about what the + // status means, so listing `in_review_2` alone leaves the caller no + // way to find the one they were told to use. (MUL-6749) + allowed, listErr := issuestatus.ActiveKeyLabels(r.Context(), h.Queries, workspaceID) if listErr != nil || len(allowed) == 0 { allowed = issuestatus.Canonical() } @@ -260,6 +277,9 @@ func (h *Handler) newStatusCategoryFiller(ctx context.Context, wsID pgtype.UUID) return } resp.StatusCategory = resolver.Effective(ctx, h.Queries, resp.Status) + // Same Resolver, same single catalog read, so the name rides along for + // free. Built-ins return "" and stay omitted. (MUL-6749) + resp.StatusName = resolver.Name(ctx, h.Queries, resp.Status) } } diff --git a/server/internal/handler/issue_status.go b/server/internal/handler/issue_status.go index fb5cdd4e2..39d9e209e 100644 --- a/server/internal/handler/issue_status.go +++ b/server/internal/handler/issue_status.go @@ -1,6 +1,7 @@ package handler import ( + "context" "encoding/json" "errors" "log/slog" @@ -155,28 +156,31 @@ func (h *Handler) CreateIssueStatus(w http.ResponseWriter, r *http.Request) { return } - // An explicit key wins; otherwise derive one from the name so the common - // case is a single field. Either way it is validated against the reserved - // built-in keys and the storage pattern. - var key string + // An explicit key wins and is checked here, against the reserved built-in + // names and the storage pattern. Without one the key is DERIVED from the + // display name, which needs the catalog and so happens under the lock in + // createIssueStatusEntry. + var explicitKey string if strings.TrimSpace(req.Key) != "" { - key, err = issuestatus.ValidateKey(req.Key) - } else { - key, err = issuestatus.SlugifyKey(name) - } - if err != nil { - writeError(w, http.StatusBadRequest, err.Error()) - return + explicitKey, err = issuestatus.ValidateKey(req.Key) + if err != nil { + writeError(w, http.StatusBadRequest, err.Error()) + return + } } - entry, err := h.Queries.CreateIssueStatusEntry(r.Context(), db.CreateIssueStatusEntryParams{ + entry, badRequest, err := h.createIssueStatusEntry(r.Context(), wsUUID, db.CreateIssueStatusEntryParams{ WorkspaceID: wsUUID, - Key: key, + Key: explicitKey, Name: name, Description: req.Description, Category: req.Category, Color: strings.ToLower(color), }) + if badRequest != "" { + writeError(w, http.StatusBadRequest, badRequest) + return + } if err != nil { if isUniqueViolation(err) { writeError(w, http.StatusConflict, "a status with this key or name already exists") @@ -190,6 +194,69 @@ func (h *Handler) CreateIssueStatus(w http.ResponseWriter, r *http.Request) { writeJSON(w, http.StatusCreated, issueStatusToResponse(entry)) } +// createIssueStatusEntry writes one catalog row, deriving arg.Key from the +// display name when it arrives empty. +// +// Derivation READS the catalog to choose a key nothing already owns, so the +// read and the insert have to be a single atomic step: two admins creating a +// Chinese-named in_review status at the same instant would otherwise both +// compute `in_review_2`, and the loser would be told a key they never typed was +// already taken. The EXCLUSIVE catalog lock — the same one archive takes — +// serializes them. +// +// EVERY create takes that lock, including one that supplies its own key. +// Excluding those would leave the race half-closed: an explicit-key insert of +// `in_review_2` could still land between a derive's catalog read and its +// insert, and the derive — a UI request with no key field to blame — would come +// back 409. The lock is only contended by catalog writes, which are rare admin +// actions, so serializing them costs nothing worth keeping the hole for. +// +// A non-empty second return is a caller error the handler reports as 400, +// distinct from a nil-error success and from an infrastructure failure. +func (h *Handler) createIssueStatusEntry(ctx context.Context, workspaceID pgtype.UUID, arg db.CreateIssueStatusEntryParams) (db.IssueStatus, string, error) { + tx, err := h.TxStarter.Begin(ctx) + if err != nil { + return db.IssueStatus{}, "", err + } + defer tx.Rollback(ctx) + qtx := h.Queries.WithTx(tx) + + if err := qtx.LockIssueStatusCatalog(ctx, workspaceID); err != nil { + return db.IssueStatus{}, "", err + } + + if arg.Key == "" { + // IncludeArchived, because idx_issue_status_workspace_key is NOT a + // partial index: a retired status still owns its key, so reusing it + // would fail on insert instead of producing a second candidate. + entries, err := qtx.ListIssueStatusEntries(ctx, db.ListIssueStatusEntriesParams{ + WorkspaceID: workspaceID, + IncludeArchived: true, + }) + if err != nil { + return db.IssueStatus{}, "", err + } + taken := make(map[string]bool, len(entries)) + for _, e := range entries { + taken[e.Key] = true + } + key, err := issuestatus.DeriveKey(arg.Name, arg.Category, taken) + if err != nil { + return db.IssueStatus{}, err.Error(), nil + } + arg.Key = key + } + + entry, err := qtx.CreateIssueStatusEntry(ctx, arg) + if err != nil { + return db.IssueStatus{}, "", err + } + if err := tx.Commit(ctx); err != nil { + return db.IssueStatus{}, "", err + } + return entry, "", nil +} + // UpdateIssueStatus edits a custom status's presentation. Built-in statuses are // immutable in v1 — name and color included — so the default workspace looks // and behaves identically for everyone who never opens this settings page. diff --git a/server/internal/handler/issue_status_test.go b/server/internal/handler/issue_status_test.go index 9fbe662b6..d51c1a861 100644 --- a/server/internal/handler/issue_status_test.go +++ b/server/internal/handler/issue_status_test.go @@ -7,6 +7,8 @@ import ( "net/http" "net/http/httptest" "slices" + "strings" + "sync" "testing" "time" @@ -1196,7 +1198,7 @@ func TestListEndpointsCarryStatusCategory(t *testing.T) { // TestBackgroundEventCarriesCustomStatusCategory pins the background-event // payload. IssueToMap fills a category only for built-ins; the publishers that -// can emit a CUSTOM status go through IssueToMapWithCategory so clients receive +// can emit a CUSTOM status go through IssueToMapResolved so clients receive // an authoritative one instead of a blank they would have to refetch to resolve. func TestBackgroundEventCarriesCustomStatusCategory(t *testing.T) { ctx := context.Background() @@ -1216,9 +1218,9 @@ func TestBackgroundEventCarriesCustomStatusCategory(t *testing.T) { plain["status_category"]) } - authoritative := service.IssueToMapWithCategory(ctx, testHandler.Queries, issue, "MUL") + authoritative := service.IssueToMapResolved(ctx, testHandler.Queries, issue, "MUL") if authoritative["status_category"] != "in_review" { - t.Errorf("IssueToMapWithCategory status_category = %v, want in_review", + t.Errorf("IssueToMapResolved status_category = %v, want in_review", authoritative["status_category"]) } if authoritative["status"] != "human_review_bg" { @@ -1330,3 +1332,449 @@ func TestCatalogWritesAnnounceThemselves(t *testing.T) { } expectNoChange(t) } + +// TestCreateIssueStatusAcceptsANonLatinName is the regression for MUL-6749 / +// GitHub #7627. A display name written entirely in a non-Latin script has no +// characters in the key alphabet, so key derivation used to fail the create +// outright — and the settings form has no field for an explicit key, which left +// no way to create the status at all. +func TestCreateIssueStatusAcceptsANonLatinName(t *testing.T) { + seedTestCatalog(t) + + create := func(t *testing.T, name, category string) IssueStatusResponse { + t.Helper() + var created IssueStatusResponse + testutil.Call(t, testHandler.CreateIssueStatus, + newRequest(http.MethodPost, "/api/issue-statuses", map[string]any{ + "name": name, "category": category, "color": "#123456", + })).Want(http.StatusCreated).JSON(&created) + t.Cleanup(func() { + testPool.Exec(context.Background(), `DELETE FROM issue_status WHERE id = $1`, parseUUID(created.ID)) + }) + return created + } + + first := create(t, "客户确认", issuestatus.InReview) + if first.Key != "in_review_2" { + t.Errorf("derived key = %q, want %q", first.Key, "in_review_2") + } + if first.Name != "客户确认" { + t.Errorf("display name = %q, want it stored verbatim", first.Name) + } + + // A second one in the same category takes the next ordinal instead of + // colliding with the first. + second := create(t, "供应商确认", issuestatus.InReview) + if second.Key != "in_review_3" { + t.Errorf("second derived key = %q, want %q", second.Key, "in_review_3") + } + + // A name that CAN be slugged is untouched by the fallback. + english := create(t, "Human Review Zh", issuestatus.InReview) + if english.Key != "human_review_zh" { + t.Errorf("sluggable name derived %q, want %q", english.Key, "human_review_zh") + } +} + +// TestCreateIssueStatusDisambiguatesCollidingSlugs covers the sharper half of +// #7627. The bug report's suggested workaround was to mix ASCII into the +// display name, but the non-ASCII part is dropped, so two DIFFERENT names +// collapse onto one key — and the second create used to fail with a conflict +// that blamed a display name nobody had taken. +func TestCreateIssueStatusDisambiguatesCollidingSlugs(t *testing.T) { + seedTestCatalog(t) + + create := func(t *testing.T, name string) IssueStatusResponse { + t.Helper() + var created IssueStatusResponse + testutil.Call(t, testHandler.CreateIssueStatus, + newRequest(http.MethodPost, "/api/issue-statuses", map[string]any{ + "name": name, "category": issuestatus.Todo, "color": "#123456", + })).Want(http.StatusCreated).JSON(&created) + t.Cleanup(func() { + testPool.Exec(context.Background(), `DELETE FROM issue_status WHERE id = $1`, parseUUID(created.ID)) + }) + return created + } + + if got := create(t, "待客户 Zzreview").Key; got != "zzreview" { + t.Fatalf("first key = %q, want %q", got, "zzreview") + } + if got := create(t, "待供应商 Zzreview").Key; got != "zzreview_2" { + t.Errorf("colliding key = %q, want %q", got, "zzreview_2") + } +} + +// TestDerivedKeyAvoidsAnArchivedKey pins the storage constraint that makes the +// "taken" set wider than the visible catalog: idx_issue_status_workspace_key is +// NOT partial, so an archived status still owns its key and handing it out +// again would fail on insert. +func TestDerivedKeyAvoidsAnArchivedKey(t *testing.T) { + entry := createTestCustomStatus(t, "zzarchived", issuestatus.Todo) + if _, err := testHandler.Queries.ArchiveIssueStatusEntry(context.Background(), db.ArchiveIssueStatusEntryParams{ + ID: entry.ID, + WorkspaceID: parseUUID(testWorkspaceID), + }); err != nil { + t.Fatalf("archive: %v", err) + } + + var created IssueStatusResponse + testutil.Call(t, testHandler.CreateIssueStatus, + newRequest(http.MethodPost, "/api/issue-statuses", map[string]any{ + "name": "Zzarchived", "category": issuestatus.Todo, "color": "#123456", + })).Want(http.StatusCreated).JSON(&created) + t.Cleanup(func() { + testPool.Exec(context.Background(), `DELETE FROM issue_status WHERE id = $1`, parseUUID(created.ID)) + }) + + if created.Key == "zzarchived" { + t.Fatal("derivation reused an archived status's key") + } + if created.Key != "zzarchived_2" { + t.Errorf("derived key = %q, want %q", created.Key, "zzarchived_2") + } +} + +// TestUnknownStatusErrorNamesCustomStatuses covers the other half of what makes +// a derived key usable: on its own `in_review_2` says nothing, so the error a +// caller gets after writing a bad status has to carry the display name or there +// is no way to find the status they were told to use. (MUL-6749) +func TestUnknownStatusErrorNamesCustomStatuses(t *testing.T) { + seedTestCatalog(t) + entry, err := testHandler.Queries.CreateIssueStatusEntry(context.Background(), db.CreateIssueStatusEntryParams{ + WorkspaceID: parseUUID(testWorkspaceID), + Key: "in_review_9", + Name: "客户确认", + Description: "", + Category: issuestatus.InReview, + Color: "#123456", + }) + if err != nil { + t.Fatalf("create custom status: %v", err) + } + t.Cleanup(func() { + testPool.Exec(context.Background(), `DELETE FROM issue_status WHERE id = $1`, entry.ID) + }) + + rec := httptest.NewRecorder() + testHandler.CreateIssue(rec, newRequest(http.MethodPost, "/api/issues", map[string]any{ + "title": "unknown status error body", + "status": "not_a_status", + })) + if rec.Code != http.StatusBadRequest { + t.Fatalf("expected 400, got %d: %s", rec.Code, rec.Body.String()) + } + body := rec.Body.String() + if !strings.Contains(body, "in_review_9 (客户确认)") { + t.Errorf("error body does not pair the key with its name: %s", body) + } + // Built-ins stay bare — clients localize those from the key, so echoing the + // seeded English name would be the one string a Chinese workspace ignores. + if strings.Contains(body, "todo (Todo)") { + t.Errorf("built-in statuses should be listed as bare keys: %s", body) + } +} + +// TestIssueResponseCarriesCustomStatusName pins the field an agent reads an +// issue through. `status` alone is a bare handle, and a derived key carries no +// meaning, so the display name travels beside it. (MUL-6749) +func TestIssueResponseCarriesCustomStatusName(t *testing.T) { + seedTestCatalog(t) + entry, err := testHandler.Queries.CreateIssueStatusEntry(context.Background(), db.CreateIssueStatusEntryParams{ + WorkspaceID: parseUUID(testWorkspaceID), + Key: "in_review_8", + Name: "客户确认", + Description: "", + Category: issuestatus.InReview, + Color: "#123456", + }) + if err != nil { + t.Fatalf("create custom status: %v", err) + } + t.Cleanup(func() { + testPool.Exec(context.Background(), `DELETE FROM issue_status WHERE id = $1`, entry.ID) + }) + + var custom IssueResponse + testutil.Call(t, testHandler.CreateIssue, + newRequest(http.MethodPost, "/api/issues", map[string]any{ + "title": "custom status name", "status": "in_review_8", + })).Want(http.StatusCreated).JSON(&custom) + t.Cleanup(func() { + testPool.Exec(context.Background(), `DELETE FROM issue WHERE id = $1`, parseUUID(custom.ID)) + }) + if custom.StatusName != "客户确认" { + t.Errorf("status_name = %q, want %q", custom.StatusName, "客户确认") + } + if custom.StatusCategory != issuestatus.InReview { + t.Errorf("status_category = %q, want %q", custom.StatusCategory, issuestatus.InReview) + } + + // A built-in carries no name: every client renders those from the key + // through i18n, so the seeded English one would be noise at best. + var builtIn IssueResponse + testutil.Call(t, testHandler.CreateIssue, + newRequest(http.MethodPost, "/api/issues", map[string]any{ + "title": "built-in status name", "status": issuestatus.Todo, + })).Want(http.StatusCreated).JSON(&builtIn) + t.Cleanup(func() { + testPool.Exec(context.Background(), `DELETE FROM issue WHERE id = $1`, parseUUID(builtIn.ID)) + }) + if builtIn.StatusName != "" { + t.Errorf("built-in status_name = %q, want it empty", builtIn.StatusName) + } +} + +// TestExplicitKeyCreateAlsoTakesTheCatalogLock closes the half-open race found +// in review of MUL-6749. Derivation reads the catalog and then inserts; if a +// create that supplies its OWN key were allowed to skip the lock, it could land +// on the key the derive just chose in exactly that gap. The derive would then +// fail on the unique index and return 409 to a settings form that has no key +// field — the same dead end this issue exists to remove. +// +// The assertion is that an explicit-key create PARKS while the lock is held. +// Before the fix it completed immediately. +func TestExplicitKeyCreateAlsoTakesTheCatalogLock(t *testing.T) { + seedTestCatalog(t) + ctx := context.Background() + + tx, err := testPool.Begin(ctx) + if err != nil { + t.Fatalf("begin: %v", err) + } + defer tx.Rollback(ctx) + if _, err := tx.Exec(ctx, + `SELECT pg_advisory_xact_lock(hashtextextended($1::uuid::text || ':issue_status', 0))`, + parseUUID(testWorkspaceID)); err != nil { + t.Fatalf("take exclusive lock: %v", err) + } + + done := make(chan *httptest.ResponseRecorder, 1) + go func() { + rec := httptest.NewRecorder() + testHandler.CreateIssueStatus(rec, newRequest(http.MethodPost, "/api/issue-statuses", map[string]any{ + "name": "Zzlockprobe", "key": "zzlockprobe", "category": issuestatus.Todo, "color": "#123456", + })) + done <- rec + }() + + select { + case rec := <-done: + t.Fatalf("an explicit-key create completed (%d) while the catalog lock was held; "+ + "it can still insert between a derived create's catalog read and its insert", rec.Code) + case <-time.After(400 * time.Millisecond): + // Parked on the lock, which is the point. + } + + if err := tx.Commit(ctx); err != nil { + t.Fatalf("release lock: %v", err) + } + + select { + case rec := <-done: + if rec.Code != http.StatusCreated { + t.Fatalf("explicit-key create after the lock released: %d %s", rec.Code, rec.Body.String()) + } + var created IssueStatusResponse + if err := json.Unmarshal(rec.Body.Bytes(), &created); err != nil { + t.Fatalf("decode: %v", err) + } + t.Cleanup(func() { + testPool.Exec(context.Background(), `DELETE FROM issue_status WHERE id = $1`, parseUUID(created.ID)) + }) + case <-time.After(10 * time.Second): + t.Fatal("explicit-key create never completed after the lock released") + } +} + +// TestConcurrentDerivedCreatesNeverConflict is the behavioral half of the same +// fix: whatever else is writing the catalog, a create that supplied no key must +// never come back 409. A caller who typed a key CAN legitimately be told it is +// taken; a caller who typed only a display name has nothing to correct. +// +// Every result carries WHICH kind of writer produced it. Counting successes +// without that — the first version of this test did — lets an explicit writer's +// 201 stand in for a derived writer's 409 and passes on exactly the regression +// it exists to catch. +// +// This is a probabilistic net, not a proof: the window it hunts for is the few +// microseconds between a derive's catalog read and its insert, and in-process +// it does not reliably open. TestExplicitKeyCreateAlsoTakesTheCatalogLock is +// the deterministic guard on the lock itself — this one guards the OUTCOME, so +// that any future path which lets a keyless create conflict shows up here even +// if the lock is still nominally taken. Do not delete one for the other. +func TestConcurrentDerivedCreatesNeverConflict(t *testing.T) { + seedTestCatalog(t) + + const derivedWriters = 6 + const rounds = 4 + type result struct { + derived bool + label string + code int + key string + body string + } + + // Every derived name must be ALL non-ASCII, or it slugs to something and + // never reaches the fallback the test is about: "客户确认0-1" keeps its ASCII + // digits and derives `0_1`, which contends with nothing. Uniqueness comes + // from a distinct base per writer and a repeated character per round, so the + // names stay collision-free without smuggling in a digit. + bases := []string{"客户确认", "供应商确认", "财务确认", "法务确认", "安全确认", "运营确认"} + if len(bases) != derivedWriters { + t.Fatalf("need one non-ASCII base per derived writer, got %d for %d", len(bases), derivedWriters) + } + + for round := range rounds { + t.Run(fmt.Sprintf("round-%d", round), func(t *testing.T) { + results := make(chan result, derivedWriters+2) + start := make(chan struct{}) + var wg sync.WaitGroup + + post := func(derived bool, label string, body map[string]any) { + defer wg.Done() + <-start + rec := httptest.NewRecorder() + testHandler.CreateIssueStatus(rec, newRequest(http.MethodPost, "/api/issue-statuses", body)) + var created IssueStatusResponse + json.Unmarshal(rec.Body.Bytes(), &created) + results <- result{derived, label, rec.Code, created.Key, rec.Body.String()} + } + + // Admins naming a status in a non-Latin script, all in one category, + // so every one of them derives from the same base. + for i := range derivedWriters { + name := bases[i] + strings.Repeat("确", round+1) + wg.Add(1) + go post(true, name, map[string]any{ + "name": name, "category": issuestatus.Blocked, "color": "#123456", + }) + } + // Explicit writers aiming straight at the ordinals the derives want. + for _, key := range []string{"blocked_2", "blocked_3"} { + wg.Add(1) + go post(false, key, map[string]any{ + "name": fmt.Sprintf("Zz %s %d", key, round), "key": key, + "category": issuestatus.Blocked, "color": "#123456", + }) + } + + close(start) + wg.Wait() + close(results) + + t.Cleanup(func() { + testPool.Exec(context.Background(), + `DELETE FROM issue_status WHERE workspace_id = $1 AND category = 'blocked' AND is_system = FALSE`, + parseUUID(testWorkspaceID)) + }) + + seen := map[string]bool{} + derivedSucceeded := 0 + for r := range results { + switch { + case r.derived && r.code != http.StatusCreated: + // The regression this test exists for: a writer with no key + // field to correct was told to correct one. + t.Errorf("derived create %q returned %d, want 201 — a create with no key must never conflict: %s", + r.label, r.code, r.body) + continue + case !r.derived && r.code != http.StatusCreated && r.code != http.StatusConflict: + t.Errorf("explicit create %q returned %d, want 201 or 409: %s", r.label, r.code, r.body) + continue + case r.code == http.StatusConflict: + // Only reachable for an explicit writer, by the branch above. + if !strings.Contains(r.body, "already exists") { + t.Errorf("unexpected 409 body for %q: %s", r.label, r.body) + } + continue + } + if seen[r.key] { + t.Errorf("two statuses were created with the same key %q", r.key) + } + seen[r.key] = true + if issuestatus.IsBuiltIn(r.key) { + t.Errorf("a custom status shadowed the built-in key %q", r.key) + } + if r.derived { + // Pins the PREMISE: a derived name that slugs to anything at + // all takes the slug path and contends with nothing, which + // would leave this test green while exercising nothing. + if !strings.HasPrefix(r.key, issuestatus.Blocked+"_") { + t.Errorf("derived create %q produced key %q; the name must slug to nothing "+ + "so it lands on the _ fallback these writers contend for", + r.label, r.key) + } + derivedSucceeded++ + } + } + if derivedSucceeded != derivedWriters { + t.Errorf("%d of %d DERIVED creates succeeded", derivedSucceeded, derivedWriters) + } + }) + } +} + +// TestCustomStatusPayloadsAgreeAcrossRenderings pins the contract +// TestIssueToMap_KeysMatchIssueResponse cannot reach with a built-in fixture: +// an issue on a CUSTOM status must describe itself identically whether it +// arrives over HTTP or on a background event. A field present in one and blank +// in the other reads back undefined depending on which entry point produced the +// issue. (MUL-6749) +func TestCustomStatusPayloadsAgreeAcrossRenderings(t *testing.T) { + ctx := context.Background() + seedTestCatalog(t) + entry, err := testHandler.Queries.CreateIssueStatusEntry(ctx, db.CreateIssueStatusEntryParams{ + WorkspaceID: parseUUID(testWorkspaceID), + Key: "in_review_7", + Name: "客户确认", + Description: "", + Category: issuestatus.InReview, + Color: "#123456", + }) + if err != nil { + t.Fatalf("create custom status: %v", err) + } + t.Cleanup(func() { + testPool.Exec(context.Background(), `DELETE FROM issue_status WHERE id = $1`, entry.ID) + }) + + var fromHTTP IssueResponse + testutil.Call(t, testHandler.CreateIssue, + newRequest(http.MethodPost, "/api/issues", map[string]any{ + "title": "payload parity", "status": "in_review_7", + })).Want(http.StatusCreated).JSON(&fromHTTP) + t.Cleanup(func() { + testPool.Exec(context.Background(), `DELETE FROM issue WHERE id = $1`, parseUUID(fromHTTP.ID)) + }) + + issue, err := testHandler.Queries.GetIssue(ctx, parseUUID(fromHTTP.ID)) + if err != nil { + t.Fatalf("reload issue: %v", err) + } + fromEvent := service.IssueToMapResolved(ctx, testHandler.Queries, issue, "MUL") + + for field, want := range map[string]string{ + "status": "in_review_7", + "status_category": issuestatus.InReview, + "status_name": "客户确认", + } { + got, ok := fromEvent[field].(string) + if !ok { + t.Errorf("event payload is missing %q; clients treat it as a complete issue", field) + continue + } + if got != want { + t.Errorf("event %s = %q, want %q", field, got, want) + } + } + if fromHTTP.StatusName != fromEvent["status_name"] { + t.Errorf("status_name differs by entry point: HTTP %q, event %q", + fromHTTP.StatusName, fromEvent["status_name"]) + } + if fromHTTP.StatusCategory != fromEvent["status_category"] { + t.Errorf("status_category differs by entry point: HTTP %q, event %q", + fromHTTP.StatusCategory, fromEvent["status_category"]) + } +} diff --git a/server/internal/issuestatus/issuestatus.go b/server/internal/issuestatus/issuestatus.go index ba3c46515..e07b89f23 100644 --- a/server/internal/issuestatus/issuestatus.go +++ b/server/internal/issuestatus/issuestatus.go @@ -27,6 +27,7 @@ import ( "errors" "fmt" "regexp" + "strconv" "strings" "github.com/jackc/pgx/v5" @@ -131,9 +132,17 @@ func ValidateKey(key string) (string, error) { return key, nil } -// SlugifyKey derives a candidate key from a display name, for callers that let -// an admin type only a name. Returns an error when nothing usable survives. -func SlugifyKey(name string) (string, error) { +// maxKeyLen mirrors the 32-character ceiling in keyPattern and the issue_status +// CHECK. Suffixing a key has to stay under it, so the base gets truncated +// rather than the suffix dropped — a truncated base is still unique-able, a +// dropped suffix is not. +const maxKeyLen = 32 + +// slugify reduces a display name to the ASCII key alphabet, returning "" when +// nothing survives. Lowercase because `multica issue status human_review` +// has to be unambiguous to type; runs of everything else collapse to a single +// underscore so "Gate — Approved!" does not become "gate___approved". +func slugify(name string) string { var b strings.Builder lastUnderscore := false for _, r := range strings.ToLower(strings.TrimSpace(name)) { @@ -147,13 +156,109 @@ func SlugifyKey(name string) (string, error) { } } slug := strings.Trim(b.String(), "_") - if len(slug) > 32 { - slug = strings.Trim(slug[:32], "_") + if len(slug) > maxKeyLen { + slug = strings.Trim(slug[:maxKeyLen], "_") } - if slug == "" { - return "", errors.New("cannot derive a status key from that name; provide one explicitly") + return slug +} + +// DeriveKey picks the stored key for a status whose creator supplied only a +// display name (MUL-6749 / GitHub #7627). +// +// The key is an ASCII machine handle — it is what issue.status stores, what the +// CLI takes as an argument, and what the column's CHECK constraint permits — +// while the display name is free Unicode. Deriving one from the other therefore +// has to survive names that share no alphabet with it: +// +// - a name that slugs to something usable keeps that slug, so an +// English-named status still gets the readable `human_review` it always +// got; +// - a name written entirely in a non-Latin script slugs to nothing, and falls +// back to its CATEGORY plus an ordinal. Category, not a random suffix: it +// is already the anchor an agent reasons from, so `in_review_2` still says +// which platform behavior the status inherits, and it degrades into the +// right neighborhood when a model confuses it with its category. The +// meaning of the status travels with its name and description, which the +// agent brief prints beside the key. +// +// Either way the result is disambiguated against keys the workspace already +// owns, so two names that collapse onto one slug ("待客户 Review" and +// "待供应商 Review" both slug to "review") no longer make the second create +// fail with a conflict that blames the display name. +// +// taken reports whether a key already exists in this workspace and MUST count +// ARCHIVED statuses: idx_issue_status_workspace_key is not partial, so an +// archived row still owns its key. Built-in keys are treated as taken here +// regardless of what taken says, which both stops a custom status from +// shadowing one and is what makes the fallback start at `_2`. +// +// Derivation is expected to succeed for any name that reaches it. The one +// refusal left is a name that slugs ONTO a built-in key: that is a naming +// collision the admin should see rather than have quietly renamed, and it is +// unchanged from before. +func DeriveKey(name, category string, taken map[string]bool) (string, error) { + if slug := slugify(name); slug != "" { + if IsBuiltIn(slug) { + return "", fmt.Errorf("%q is a built-in status key and cannot be reused; rename the status or pass an explicit key", slug) + } + return firstFreeKey(slug, taken) } - return ValidateKey(slug) + // Nothing in the name belongs to the key alphabet. Category is a valid key + // by construction, and its built-in always occupies it, so this lands on + // _2 for the first such status in that category. + if !IsCategory(category) { + return "", fmt.Errorf("category must be one of: %s", strings.Join(canonicalOrder, ", ")) + } + return firstFreeKey(category, taken) +} + +// firstFreeKey returns base when it is unclaimed, otherwise the first +// 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. +func firstFreeKey(base string, taken map[string]bool) (string, error) { + if !keyOccupied(base, taken) { + return ValidateKey(base) + } + limit := len(taken) + len(canonicalOrder) + 2 + for n := 2; n <= limit; n++ { + suffix := "_" + strconv.Itoa(n) + candidate := truncateForSuffix(base, len(suffix)) + suffix + if !keyOccupied(candidate, taken) { + return ValidateKey(candidate) + } + } + // Unreachable by the argument above; kept so a future change to candidate + // generation surfaces as an error rather than an infinite loop. + return "", errors.New("could not derive an unused status key from that name; provide one explicitly") +} + +// keyOccupied folds the built-in set into the workspace's own keys. A built-in +// counts as occupied even in a workspace whose catalog rows have not been +// seeded yet, so an unseeded workspace cannot mint a custom status that +// shadows one. +func keyOccupied(key string, taken map[string]bool) bool { + if IsBuiltIn(key) { + return true + } + return taken[key] +} + +// truncateForSuffix shortens base so base+suffix fits maxKeyLen. The trailing +// underscore trim keeps "human_review" from becoming "human_" + "_2"; base +// always starts with an alphanumeric, so trimming can never empty it. +func truncateForSuffix(base string, suffixLen int) string { + limit := maxKeyLen - suffixLen + if len(base) <= limit { + return base + } + return strings.TrimRight(base[:limit], "_") } // Ensure idempotently seeds a workspace's 7 built-in statuses. Safe to call @@ -192,6 +297,33 @@ func Effective(ctx context.Context, q Querier, workspaceID pgtype.UUID, status s return entry.Category } +// EffectiveAndName resolves a status to BOTH its category and its display name +// in one catalog read, for payload builders that need the pair. +// +// Two separate calls would double the query on every background event carrying +// a custom status. Same fail-safe direction as Effective: an unresolvable key +// yields the key unchanged and an empty name. +// +// The name is empty for a built-in on purpose — clients localize those from the +// key, so echoing the seeded English one would be the single string a +// non-English workspace has to ignore. (MUL-6749) +func EffectiveAndName(ctx context.Context, q Querier, workspaceID pgtype.UUID, status string) (string, string) { + if IsBuiltIn(status) { + return status, "" + } + entry, err := q.GetIssueStatusEntryByKey(ctx, db.GetIssueStatusEntryByKeyParams{ + WorkspaceID: workspaceID, + Key: status, + }) + if err != nil { + return status, "" + } + if !IsCategory(entry.Category) { + return status, entry.Name + } + return entry.Category, entry.Name +} + // Resolve validates that status is usable in this workspace, returning the // catalog entry. Write paths use this; it is the application-layer replacement // for the enum CHECK that migration 337 dropped. @@ -267,6 +399,42 @@ func ActiveKeys(ctx context.Context, q Querier, workspaceID pgtype.UUID) ([]stri return keys, nil } +// ActiveKeyLabels is ActiveKeys with each CUSTOM status's display name attached, +// as `key (Name)`. It backs the error a caller sees after writing an unknown +// status, where a bare key list is the least useful thing to hand back: a key +// derived from a non-Latin name carries no meaning on its own (`in_review_2`), +// so the reader — a person or an agent — has nothing to match against the name +// they were told to use. +// +// Built-ins stay bare. Their key IS their category, their seeded name is +// English, and every client localizes them from the key anyway, so printing +// `todo (Todo)` would add noise rather than information. +func ActiveKeyLabels(ctx context.Context, q Querier, workspaceID pgtype.UUID) ([]string, error) { + entries, err := q.ListIssueStatusEntries(ctx, db.ListIssueStatusEntriesParams{ + WorkspaceID: workspaceID, + IncludeArchived: false, + }) + if err != nil { + return nil, err + } + labels := make([]string, 0, len(entries)+len(canonicalOrder)) + seen := make(map[string]bool, len(entries)) + for _, e := range entries { + seen[e.Key] = true + if IsBuiltIn(e.Key) || strings.TrimSpace(e.Name) == "" { + labels = append(labels, e.Key) + continue + } + labels = append(labels, e.Key+" ("+e.Name+")") + } + for _, key := range canonicalOrder { + if !seen[key] { + labels = append(labels, key) + } + } + return labels, nil +} + // Resolver resolves many statuses against ONE workspace's catalog using at most // one query, for list endpoints that would otherwise issue a lookup per row. // @@ -281,6 +449,7 @@ func ActiveKeys(ctx context.Context, q Querier, workspaceID pgtype.UUID) ([]stri type Resolver struct { workspaceID pgtype.UUID categories map[string]string + names map[string]string loaded bool } @@ -289,6 +458,29 @@ func NewResolver(workspaceID pgtype.UUID) *Resolver { return &Resolver{workspaceID: workspaceID} } +// load fetches the catalog once, on the first call that needs it. A failed read +// leaves the maps nil, which makes every lookup below fall back to its +// key-unchanged branch — the same fail-safe the single-shot resolver applies. +func (r *Resolver) load(ctx context.Context, q Querier) { + if r.loaded { + return + } + r.loaded = true + entries, err := q.ListIssueStatusEntries(ctx, db.ListIssueStatusEntriesParams{ + WorkspaceID: r.workspaceID, + IncludeArchived: true, + }) + if err != nil { + return + } + r.categories = make(map[string]string, len(entries)) + r.names = make(map[string]string, len(entries)) + for _, e := range entries { + r.categories[e.Key] = e.Category + r.names[e.Key] = e.Name + } +} + // Effective mirrors the package-level Effective, but amortizes the catalog read // across every call. Same fail-safe direction: an unresolvable key is returned // unchanged rather than guessed at. @@ -296,22 +488,7 @@ func (r *Resolver) Effective(ctx context.Context, q Querier, status string) stri if IsBuiltIn(status) { return status } - if !r.loaded { - r.loaded = true - entries, err := q.ListIssueStatusEntries(ctx, db.ListIssueStatusEntriesParams{ - WorkspaceID: r.workspaceID, - IncludeArchived: true, - }) - if err != nil { - // Leave the map nil; every custom key then falls back to itself, - // which is the same fail-safe the single-shot resolver applies. - return status - } - r.categories = make(map[string]string, len(entries)) - for _, e := range entries { - r.categories[e.Key] = e.Category - } - } + r.load(ctx, q) category, ok := r.categories[status] if !ok || !IsCategory(category) { return status @@ -319,6 +496,26 @@ func (r *Resolver) Effective(ctx context.Context, q Querier, status string) stri return category } +// Name returns a CUSTOM status's display name, or "" for a built-in and for a +// key the catalog does not hold. +// +// Built-ins return "" on purpose rather than their seeded English name: every +// client renders those from the key through i18n, so handing back "In Progress" +// would be the one string a Chinese workspace must ignore. Custom names are +// user-authored and untranslatable, so they are the label everywhere — which is +// also why a payload carrying a custom key should carry this beside it. A +// generated key like `in_review_2` says nothing on its own. (MUL-6749) +// +// Shares the Resolver's single catalog read with Effective, so adding the name +// to a response costs no extra query. +func (r *Resolver) Name(ctx context.Context, q Querier, status string) string { + if IsBuiltIn(status) { + return "" + } + r.load(ctx, q) + return r.names[status] +} + // ExpandCategories turns a set of categories into the status keys that belong // to them, for use as an INDEXED `status = ANY(...)` predicate. // diff --git a/server/internal/issuestatus/issuestatus_test.go b/server/internal/issuestatus/issuestatus_test.go index 07f6c787b..9221e0e55 100644 --- a/server/internal/issuestatus/issuestatus_test.go +++ b/server/internal/issuestatus/issuestatus_test.go @@ -3,6 +3,7 @@ package issuestatus import ( "context" "errors" + "strconv" "strings" "testing" @@ -240,7 +241,20 @@ func TestValidateKeyRejectsReservedAndMalformed(t *testing.T) { } } -func TestSlugifyKey(t *testing.T) { +// takenSet builds the DeriveKey callback from a literal list of keys the +// workspace already owns. +func takenSet(keys ...string) map[string]bool { + owned := make(map[string]bool, len(keys)) + for _, k := range keys { + owned[k] = true + } + return owned +} + +// TestDeriveKeyKeepsTheSlugForSluggableNames pins the behavior an English +// workspace already had: the key stays a readable slug of the name, and the +// fallback introduced for MUL-6749 must not reach names that never needed it. +func TestDeriveKeyKeepsTheSlugForSluggableNames(t *testing.T) { cases := map[string]string{ "Human Review": "human_review", "Gate Approved!": "gate_approved", @@ -248,23 +262,189 @@ func TestSlugifyKey(t *testing.T) { "Waiting — 客户": "waiting", } for name, want := range cases { - got, err := SlugifyKey(name) + got, err := DeriveKey(name, InReview, takenSet()) if err != nil { - t.Errorf("SlugifyKey(%q) failed: %v", name, err) + t.Errorf("DeriveKey(%q) failed: %v", name, err) continue } if got != want { - t.Errorf("SlugifyKey(%q) = %q, want %q", name, got, want) + t.Errorf("DeriveKey(%q) = %q, want %q", name, got, want) + } + } +} + +// TestDeriveKeyFallsBackToCategoryForNonLatinNames is the bug this package was +// changed for (MUL-6749 / GitHub #7627): a display name written entirely in a +// non-Latin script has nothing to slug, and used to be REFUSED outright — with +// no field in the settings form to supply a key instead, that made the status +// impossible to create at all. +// +// The fallback is the category plus an ordinal rather than a random suffix, so +// the key still says which platform behavior the status inherits. It starts at +// 2 because the category's own built-in already owns the bare key. +func TestDeriveKeyFallsBackToCategoryForNonLatinNames(t *testing.T) { + for _, name := range []string{"客户确认", "고객 확인", "確認待ち", "تأكيد العميل", "客戶確認"} { + got, err := DeriveKey(name, InReview, takenSet()) + if err != nil { + t.Fatalf("DeriveKey(%q) failed: %v", name, err) + } + if got != "in_review_2" { + t.Errorf("DeriveKey(%q) = %q, want %q", name, got, "in_review_2") } } - // A name that slugifies onto a built-in key must be refused, not silently - // shadow the built-in. - if _, err := SlugifyKey("In Progress"); err == nil { + // A second non-Latin status in the same category takes the next ordinal + // rather than colliding with the first. + got, err := DeriveKey("供应商确认", InReview, takenSet("in_review_2")) + if err != nil { + t.Fatalf("DeriveKey on a second non-Latin name failed: %v", err) + } + if got != "in_review_3" { + t.Errorf("second non-Latin in_review status = %q, want %q", got, "in_review_3") + } + + // The ordinal is per category, so a different category starts over. + got, err = DeriveKey("待排期", Todo, takenSet("in_review_2", "in_review_3")) + if err != nil { + t.Fatalf("DeriveKey in another category failed: %v", err) + } + if got != "todo_2" { + t.Errorf("first non-Latin todo status = %q, want %q", got, "todo_2") + } +} + +// TestDeriveKeyEvenWhenTheWorkspaceIsUnseeded covers the rolling-deploy case. +// A workspace whose catalog rows have not landed reports NO keys as taken, so +// the fallback would hand a custom status the bare category key — which is a +// built-in — and let it shadow one. Built-ins count as occupied regardless of +// what the workspace reports. +func TestDeriveKeyEvenWhenTheWorkspaceIsUnseeded(t *testing.T) { + got, err := DeriveKey("客户确认", InReview, takenSet()) + if err != nil { + t.Fatalf("DeriveKey on an unseeded workspace failed: %v", err) + } + if IsBuiltIn(got) { + t.Fatalf("DeriveKey returned the built-in key %q on an unseeded workspace", got) + } +} + +// TestDeriveKeyDisambiguatesCollidingSlugs covers the sharper half of #7627: +// the reported workaround was to mix ASCII into the display name, but the +// non-ASCII part is dropped, so two DIFFERENT names collapse onto one slug. The +// second create used to fail on the key's unique index with a message blaming +// the display name, which was not taken at all. +func TestDeriveKeyDisambiguatesCollidingSlugs(t *testing.T) { + first, err := DeriveKey("待客户 Review", InReview, takenSet()) + if err != nil { + t.Fatalf("DeriveKey failed: %v", err) + } + if first != "review" { + t.Fatalf("DeriveKey(\"待客户 Review\") = %q, want %q", first, "review") + } + second, err := DeriveKey("待供应商 Review", InReview, takenSet(first)) + if err != nil { + t.Fatalf("DeriveKey on the colliding name failed: %v", err) + } + if second != "review_2" { + t.Errorf("colliding slug resolved to %q, want %q", second, "review_2") + } + third, err := DeriveKey("待财务 Review", InReview, takenSet(first, second)) + if err != nil { + t.Fatalf("DeriveKey on the third colliding name failed: %v", err) + } + if third != "review_3" { + t.Errorf("third colliding slug resolved to %q, want %q", third, "review_3") + } +} + +// TestDeriveKeyCountsArchivedKeysAsTaken is a storage constraint, not a +// preference: idx_issue_status_workspace_key is NOT partial, so an archived +// status still owns its key. Handing it out again would fail on insert. +func TestDeriveKeyCountsArchivedKeysAsTaken(t *testing.T) { + got, err := DeriveKey("Human Review", InReview, takenSet("human_review")) + if err != nil { + t.Fatalf("DeriveKey failed: %v", err) + } + if got != "human_review_2" { + t.Errorf("DeriveKey around an archived key = %q, want %q", got, "human_review_2") + } +} + +// TestDeriveKeyStillRefusesABuiltInSlug keeps the pre-existing guard: a name +// that slugifies ONTO a built-in key is a collision the admin should see, not +// something to silently renumber into a near-duplicate of a built-in status. +func TestDeriveKeyStillRefusesABuiltInSlug(t *testing.T) { + if _, err := DeriveKey("In Progress", InProgress, takenSet()); err == nil { t.Error("a name slugifying to a built-in key should be rejected") } - if _, err := SlugifyKey("客户"); err == nil { - t.Error("a name with no slug-able characters should be rejected") +} + +// TestDeriveKeyRejectsAnUnknownCategory guards the fallback's own input: it +// builds the key OUT of the category, so an unvalidated one would mint a key +// that the storage CHECK may not even accept. +func TestDeriveKeyRejectsAnUnknownCategory(t *testing.T) { + if _, err := DeriveKey("客户确认", "started", takenSet()); err == nil { + t.Error("a non-Latin name in an unknown category should be rejected") + } +} + +// TestDeriveKeyKeepsSuffixedKeysWithinTheStorageLimit pins the truncation: the +// key column caps at 32 characters, so the BASE gives way to the suffix rather +// than the suffix being dropped — dropping it would return a key already taken. +func TestDeriveKeyKeepsSuffixedKeysWithinTheStorageLimit(t *testing.T) { + long := strings.Repeat("a", 40) + base, err := DeriveKey(long, InReview, takenSet()) + if err != nil { + t.Fatalf("DeriveKey on a long name failed: %v", err) + } + if len(base) != 32 { + t.Fatalf("a 40-character name produced a %d-character key %q, want 32", len(base), base) + } + suffixed, err := DeriveKey(long, InReview, takenSet(base)) + if err != nil { + t.Fatalf("DeriveKey on a long colliding name failed: %v", err) + } + if len(suffixed) > 32 { + t.Errorf("suffixed key %q is %d characters, over the 32 the column allows", suffixed, len(suffixed)) + } + if suffixed == base { + t.Error("the suffixed key collided with the key it was supposed to avoid") + } + if _, err := ValidateKey(suffixed); err != nil { + t.Errorf("suffixed key %q fails the storage pattern: %v", suffixed, err) + } +} + +// TestDeriveKeyScanIsBoundedByTheCatalogNotAConstant pins that disambiguation +// keeps going as long as the workspace has keys to collide with. An arbitrary +// ceiling would fail with "provide one explicitly" — the exact error a UI with +// no key field cannot act on, which is the whole reason this package changed. +func TestDeriveKeyScanIsBoundedByTheCatalogNotAConstant(t *testing.T) { + // Every candidate `zz`, `zz_2` … `zz_1200` is already taken, so a fixed + // 1000-ish bound would give up here. + keys := []string{"zz"} + for n := 2; n <= 1200; n++ { + keys = append(keys, "zz_"+strconv.Itoa(n)) + } + got, err := DeriveKey("ZZ", Todo, takenSet(keys...)) + if err != nil { + t.Fatalf("DeriveKey gave up on a large catalog: %v", err) + } + if got != "zz_1201" { + t.Errorf("DeriveKey = %q, want %q", got, "zz_1201") + } + + // Same for the non-Latin fallback: the ordinal walks past any fixed cap. + keys = nil + for n := 2; n <= 1100; n++ { + keys = append(keys, "todo_"+strconv.Itoa(n)) + } + got, err = DeriveKey("待排期", Todo, takenSet(keys...)) + if err != nil { + t.Fatalf("non-Latin fallback gave up on a large catalog: %v", err) + } + if got != "todo_1101" { + t.Errorf("non-Latin fallback = %q, want %q", got, "todo_1101") } } diff --git a/server/internal/service/autopilot.go b/server/internal/service/autopilot.go index 427beba85..29735db0e 100644 --- a/server/internal/service/autopilot.go +++ b/server/internal/service/autopilot.go @@ -771,7 +771,7 @@ func (s *AutopilotService) dispatchCreateIssue(ctx context.Context, ap db.Autopi ActorType: "agent", ActorID: util.UUIDToString(leader.ID), Payload: map[string]any{ - "issue": IssueToMapWithCategory(ctx, s.Queries, issue, prefix), + "issue": IssueToMapResolved(ctx, s.Queries, issue, prefix), }, }) s.captureIssueCreatedFromAutopilot(ap, run, issue, leader.ID) diff --git a/server/internal/service/issue.go b/server/internal/service/issue.go index 2494e2ae7..ac0f8ab6e 100644 --- a/server/internal/service/issue.go +++ b/server/internal/service/issue.go @@ -647,7 +647,7 @@ func (s *IssueService) PublishAttachmentsChanged(ctx context.Context, issue db.I ActorType: "member", ActorID: util.UUIDToString(actorID), Payload: map[string]any{ - "issue": IssueToMapWithCategory(ctx, s.Queries, current, workspace.IssuePrefix), + "issue": IssueToMapResolved(ctx, s.Queries, current, workspace.IssuePrefix), "assignee_changed": false, "status_changed": false, "project_changed": false, diff --git a/server/internal/service/task.go b/server/internal/service/task.go index 017ef0157..9064dfc14 100644 --- a/server/internal/service/task.go +++ b/server/internal/service/task.go @@ -6751,7 +6751,7 @@ func (s *TaskService) broadcastIssueUpdated(ctx context.Context, issue db.Issue, ActorType: "system", ActorID: "", Payload: map[string]any{ - "issue": IssueToMapWithCategory(ctx, s.Queries, issue, prefix), + "issue": IssueToMapResolved(ctx, s.Queries, issue, prefix), "status_changed": prevStatus != issue.Status, "prev_status": prevStatus, }, @@ -6912,13 +6912,19 @@ func builtInStatusCategory(status string) string { return "" } -// IssueToMapWithCategory is IssueToMap with an AUTHORITATIVE status_category, -// resolved through the catalog so a custom status is not emitted with a blank -// one. Background events go through here; clients treat this payload as a -// complete issue and bucket it by category. (MUL-6243) -func IssueToMapWithCategory(ctx context.Context, q issuestatus.Querier, issue db.Issue, issuePrefix string) map[string]any { +// IssueToMapResolved is IssueToMap with an AUTHORITATIVE status_category and +// status_name, both resolved through the catalog so a custom status is not +// emitted with blanks. Background events go through here; clients treat this +// payload as a complete issue and bucket it by category. (MUL-6243) +// +// Both fields come from ONE catalog read. Resolving them separately would +// double the query on every event carrying a custom status, and the HTTP +// rendering already shares a single read through its Resolver. (MUL-6749) +func IssueToMapResolved(ctx context.Context, q issuestatus.Querier, issue db.Issue, issuePrefix string) map[string]any { m := IssueToMap(issue, issuePrefix) - m["status_category"] = issuestatus.Effective(ctx, q, issue.WorkspaceID, issue.Status) + category, name := issuestatus.EffectiveAndName(ctx, q, issue.WorkspaceID, issue.Status) + m["status_category"] = category + m["status_name"] = name return m } @@ -6934,7 +6940,12 @@ func IssueToMap(issue db.Issue, issuePrefix string) map[string]any { // Mirrors handler.IssueResponse.StatusCategory: a built-in status IS // its own category, so this resolves with no catalog lookup. Empty for // a custom status, which consumers resolve via the catalog. (MUL-6243) - "status_category": builtInStatusCategory(issue.Status), + "status_category": builtInStatusCategory(issue.Status), + // Mirrors handler.IssueResponse.StatusName. A built-in carries no name + // — clients localize those from the key — and a CUSTOM one is filled in + // by IssueToMapResolved, which has the catalog. Emitted unconditionally + // so this rendering cannot lose a key the HTTP one carries. (MUL-6749) + "status_name": "", "priority": issue.Priority, "assignee_type": util.TextToPtr(issue.AssigneeType), "assignee_id": util.UUIDToPtr(issue.AssigneeID),