From 5c4f95077bb0f87b895113a9c39fbf9964d50347 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E6=9D=A8=E6=88=90=E9=94=B4?= Date: Tue, 18 Aug 2026 08:53:02 +0000 Subject: [PATCH] fix(server): stop counting disabled conventions as completed work Standup, changelog, and project report treated a disabled convention as completed work because they used the global DefaultTerminalStatuses union (or counted every schema terminal as throughput). Honor per-collection terminal_options and skip negative terminals, including disabled. --- cmd/pad/cmd_project.go | 74 ++++++------ internal/models/terminal.go | 61 ++++++++++ internal/models/terminal_test.go | 65 +++++++++++ internal/server/handlers_project_intel.go | 80 +++++++++++-- .../server/handlers_project_intel_test.go | 107 ++++++++++++++++++ internal/store/reports.go | 23 +--- internal/store/reports_test.go | 39 +++++++ 7 files changed, 380 insertions(+), 69 deletions(-) diff --git a/cmd/pad/cmd_project.go b/cmd/pad/cmd_project.go index d7a7221c2..d7f814e8a 100644 --- a/cmd/pad/cmd_project.go +++ b/cmd/pad/cmd_project.go @@ -265,6 +265,40 @@ func nextCmd() *cobra.Command { } } +// listCompletedWorkSince fetches items that reached a *positive* terminal +// since cutoff. Terminal values are resolved per collection via +// models.CollectionCompletedWorkValues — KEEP IN SYNC with +// server.listTerminalItemsSince (BUG-1049). Best-effort per collection +// and status: a list error skips that query rather than failing the +// whole report. +func listCompletedWorkSince(client *cli.Client, ws string, cutoff time.Time, limit int) []models.Item { + colls, err := client.ListCollections(ws) + if err != nil { + return nil + } + var out []models.Item + for _, c := range colls { + field, values := models.CollectionCompletedWorkValues(c.Schema, c.Settings) + for _, status := range values { + params := url.Values{ + field: {status}, + "sort": {"updated_at:desc"}, + "limit": {strconv.Itoa(limit)}, + } + items, err := client.ListCollectionItems(ws, c.Slug, params) + if err != nil { + continue + } + for _, item := range items { + if item.UpdatedAt.After(cutoff) { + out = append(out, item) + } + } + } + } + return out +} + // --- standup --- func standupCmd() *cobra.Command { @@ -315,26 +349,8 @@ func standupCmd() *cobra.Command { return fmt.Errorf("parsing dashboard: %w", err) } - // Fetch recently completed items (terminal statuses) - doneStatuses := models.DefaultTerminalStatuses - var completedItems []models.Item cutoff := time.Now().AddDate(0, 0, -days) - - for _, status := range doneStatuses { - items, err := client.ListItems(ws, url.Values{ - "status": {status}, - "sort": {"updated_at:desc"}, - "limit": {"20"}, - }) - if err != nil { - continue - } - for _, item := range items { - if item.UpdatedAt.After(cutoff) { - completedItems = append(completedItems, item) - } - } - } + completedItems := listCompletedWorkSince(client, ws, cutoff, 20) // Fetch in-progress items inProgressItems, err := client.ListItems(ws, url.Values{ @@ -689,25 +705,7 @@ func changelogCmd() *cobra.Command { cutoff = time.Now().AddDate(0, 0, -days) } - // Fetch completed items across all terminal statuses - doneStatuses := models.DefaultTerminalStatuses - var allItems []models.Item - - for _, status := range doneStatuses { - items, err := client.ListItems(ws, url.Values{ - "status": {status}, - "sort": {"updated_at:desc"}, - "limit": {"100"}, - }) - if err != nil { - continue - } - for _, item := range items { - if item.UpdatedAt.After(cutoff) { - allItems = append(allItems, item) - } - } - } + allItems := listCompletedWorkSince(client, ws, cutoff, 100) // Filter by parent if specified filterParent := parentRef diff --git a/internal/models/terminal.go b/internal/models/terminal.go index 5c791eb31..a58327d4d 100644 --- a/internal/models/terminal.go +++ b/internal/models/terminal.go @@ -1,6 +1,7 @@ package models import ( + "encoding/json" "regexp" "strings" ) @@ -89,6 +90,66 @@ func TerminalValuesForDoneField( return fieldKey, DefaultTerminalStatuses } +// NegativeTerminals are terminal values that represent a NON-shipping close +// (the work didn't complete positively). Project report throughput and the +// standup/changelog "completed" lists exclude these so a rejected idea, +// cancelled task, or disabled convention is not counted as completed work. +// Matched case-insensitively. +// +// A future task can make this per-collection configurable; for now it's a +// sensible global default (PLAN-1628). "disabled" is included so collections +// whose only schema-declared terminal is "disabled" (stock Conventions) do +// not report turning a rule off as throughput (BUG-1049). +var NegativeTerminals = map[string]bool{ + "rejected": true, + "cancelled": true, + "canceled": true, + "wontfix": true, + "won't fix": true, + "duplicate": true, + "declined": true, + "abandoned": true, + "disabled": true, +} + +// IsNegativeTerminal reports whether value is a non-shipping terminal. +func IsNegativeTerminal(value string) bool { + return NegativeTerminals[strings.ToLower(strings.TrimSpace(value))] +} + +// PositiveTerminalValuesForDoneField is TerminalValuesForDoneField minus +// NegativeTerminals — the values that count as completed *work*. +func PositiveTerminalValuesForDoneField( + schema CollectionSchema, + settings CollectionSettings, +) (fieldKey string, values []string) { + fieldKey, terminals := TerminalValuesForDoneField(schema, settings) + values = make([]string, 0, len(terminals)) + for _, v := range terminals { + if IsNegativeTerminal(v) { + continue + } + values = append(values, v) + } + return fieldKey, values +} + +// CollectionCompletedWorkValues unmarshals a collection's persisted schema +// and settings JSON (best-effort; parse failures fall back the same way +// TerminalValuesForDoneField does) and returns the done-field key plus the +// positive terminal values that count as completed work. +func CollectionCompletedWorkValues(schemaJSON, settingsJSON string) (fieldKey string, values []string) { + var schema CollectionSchema + var settings CollectionSettings + if schemaJSON != "" { + _ = json.Unmarshal([]byte(schemaJSON), &schema) + } + if settingsJSON != "" { + _ = json.Unmarshal([]byte(settingsJSON), &settings) + } + return PositiveTerminalValuesForDoneField(schema, settings) +} + // TerminalPlaceholdersForDoneField is a SQL-layer convenience that returns // the done-field key plus the placeholder + args pair needed for an IN // clause. All values are lowercased to match the WHERE clause pattern used diff --git a/internal/models/terminal_test.go b/internal/models/terminal_test.go index a434627bd..182ec3b7a 100644 --- a/internal/models/terminal_test.go +++ b/internal/models/terminal_test.go @@ -276,3 +276,68 @@ func TestIsTerminalStatusDefault(t *testing.T) { t.Fatal("expected 'in-progress' to NOT be default-terminal") } } + +func TestIsNegativeTerminal(t *testing.T) { + if !IsNegativeTerminal("disabled") { + t.Fatal("expected 'disabled' to be a negative terminal (BUG-1049)") + } + if !IsNegativeTerminal("Rejected") { + t.Fatal("expected case-insensitive negative-terminal match") + } + if IsNegativeTerminal("done") { + t.Fatal("expected 'done' to remain a positive terminal") + } +} + +func TestPositiveTerminalValuesForDoneField_DropsNegatives(t *testing.T) { + // Stock Conventions: the only schema-declared terminal is "disabled", + // which is a non-shipping close. Completed-work queries must see an + // empty positive set rather than treating the disabled rule as work. + schema := CollectionSchema{ + Fields: []FieldDef{ + { + Key: "status", + Type: "select", + Options: []string{"active", "draft", "disabled"}, + TerminalOptions: []string{"disabled"}, + }, + }, + } + key, values := PositiveTerminalValuesForDoneField(schema, CollectionSettings{}) + if key != "status" { + t.Fatalf("expected key 'status', got %q", key) + } + if len(values) != 0 { + t.Fatalf("expected no positive terminals for Conventions, got %v", values) + } +} + +func TestPositiveTerminalValuesForDoneField_KeepsDone(t *testing.T) { + schema := CollectionSchema{ + Fields: []FieldDef{ + { + Key: "status", + Type: "select", + Options: []string{"open", "done", "cancelled"}, + TerminalOptions: []string{"done", "cancelled"}, + }, + }, + } + _, values := PositiveTerminalValuesForDoneField(schema, CollectionSettings{}) + if !reflect.DeepEqual(values, []string{"done"}) { + t.Fatalf("expected only 'done' (cancelled is negative), got %v", values) + } +} + +func TestCollectionCompletedWorkValues_ParsesJSON(t *testing.T) { + key, values := CollectionCompletedWorkValues( + `{"fields":[{"key":"status","type":"select","options":["open","shipped"],"terminal_options":["shipped"]}]}`, + `{}`, + ) + if key != "status" { + t.Fatalf("expected key 'status', got %q", key) + } + if !reflect.DeepEqual(values, []string{"shipped"}) { + t.Fatalf("expected [shipped], got %v", values) + } +} diff --git a/internal/server/handlers_project_intel.go b/internal/server/handlers_project_intel.go index f6dda393e..6c3707965 100644 --- a/internal/server/handlers_project_intel.go +++ b/internal/server/handlers_project_intel.go @@ -122,23 +122,81 @@ func (s *Server) projectIntelVisibility(r *http.Request, workspaceID string) (co return collIDs, itemIDs, visibleIDs, nil } -// listTerminalItemsSince fetches items in each of models.DefaultTerminalStatuses -// (one ListItems call per status — mirrors the CLI's loop rather than OR-ing -// statuses into a single query), keeping only items updated after cutoff, up -// to limit items considered per status. A store error for one status is -// swallowed and the loop continues — matches the CLI's best-effort -// semantics: a transient failure on one status must not blank out the whole -// report. The MCP transport inherits this by proxying here (TASK-1916) -// rather than replicating the loop. +// listTerminalItemsSince fetches items that reached a *positive* terminal +// since cutoff. Terminal values are resolved per collection via +// models.CollectionCompletedWorkValues (schema terminal_options, minus +// models.NegativeTerminals) rather than iterating the global +// DefaultTerminalStatuses union — so a collection that only declares +// "shipped" is not scanned for "done", and a disabled convention is not +// counted as completed work (BUG-1049). +// +// One ListItems call per (done-field, value) pair, scoped to the +// collections that share that pair (mirrors the CLI's per-status +// best-effort loop rather than OR-ing values into a single query). A +// store error for one pair is swallowed and the loop continues: a +// transient failure on one status must not blank out the whole report. +// The MCP transport inherits this by proxying here (TASK-1916) rather +// than replicating the loop. +// +// KEEP IN SYNC with cmd/pad/cmd_project.go's listCompletedWorkSince. func (s *Server) listTerminalItemsSince( workspaceID string, collIDs, itemIDs []string, cutoff time.Time, limit int, ) []models.Item { + colls, err := s.store.ListCollections(workspaceID) + if err != nil { + return nil + } + + // Resolve terminals for EVERY collection, including ones the caller + // only has an item-level grant on. ListItems treats CollectionIDs OR + // ItemIDs as the visibility filter; if we skipped those collections + // here we would never query their done-field values and a granted + // child in an item-grant-only collection would vanish from changelog + // (see TestProjectChangelogEndpoint_GuestParentFilter_ItemGrantOnlyCollection). + allowed := map[string]bool{} + restrict := collIDs != nil + if restrict { + for _, id := range collIDs { + allowed[id] = true + } + } + + type queryKey struct { + field string + value string + } + groups := map[queryKey][]string{} + var order []queryKey + for _, c := range colls { + field, values := models.CollectionCompletedWorkValues(c.Schema, c.Settings) + for _, value := range values { + k := queryKey{field: field, value: value} + if _, exists := groups[k]; !exists { + order = append(order, k) + } + groups[k] = append(groups[k], c.ID) + } + } + var out []models.Item - for _, status := range models.DefaultTerminalStatuses { + for _, k := range order { + var queryCollIDs []string + if restrict { + for _, id := range groups[k] { + if allowed[id] { + queryCollIDs = append(queryCollIDs, id) + } + } + if len(queryCollIDs) == 0 && len(itemIDs) == 0 { + continue + } + } else { + queryCollIDs = groups[k] + } items, err := s.store.ListItems(workspaceID, models.ItemListParams{ - CollectionIDs: collIDs, + CollectionIDs: queryCollIDs, ItemIDs: itemIDs, - Fields: map[string]string{"status": status}, + Fields: map[string]string{k.field: k.value}, Sort: "updated_at:desc", Limit: limit, }) diff --git a/internal/server/handlers_project_intel_test.go b/internal/server/handlers_project_intel_test.go index 465b09d7f..f96fa51a7 100644 --- a/internal/server/handlers_project_intel_test.go +++ b/internal/server/handlers_project_intel_test.go @@ -641,3 +641,110 @@ func TestProjectChangelogEndpoint_GuestParentFilter_ItemGrantOnlyCollection(t *t t.Fatalf("expected only the granted child %s, got %+v", child.Ref, resp.Groups) } } + +// TestProjectStandupEndpoint_DisabledConventionNotCompleted pins BUG-1049: +// disabling a convention is not completed work. The stock Conventions +// schema declares terminal_options: ["disabled"]; standup must not list +// those items under completed just because "disabled" is in the global +// DefaultTerminalStatuses union. +func TestProjectStandupEndpoint_DisabledConventionNotCompleted(t *testing.T) { + srv := testServer(t) + slug := createWSWithCollections(t, srv) + + disabled := createItem(t, srv, slug, "conventions", map[string]interface{}{ + "title": "Never push directly to main", + "fields": `{"status":"disabled"}`, + }) + done := createItem(t, srv, slug, "tasks", map[string]interface{}{ + "title": "Shipped it", + "fields": `{"status":"done"}`, + }) + + rr := doRequest(srv, "GET", "/api/v1/workspaces/"+slug+"/standup", nil) + if rr.Code != http.StatusOK { + t.Fatalf("standup: expected 200, got %d: %s", rr.Code, rr.Body.String()) + } + var resp StandupResponse + parseJSON(t, rr, &resp) + + for _, item := range resp.Completed { + if item.Ref == disabled.Ref || item.Status == "disabled" { + t.Fatalf("disabled convention %s must not appear in standup.completed, got %+v", disabled.Ref, resp.Completed) + } + } + if len(resp.Completed) != 1 || resp.Completed[0].Ref != done.Ref { + t.Fatalf("expected only the done task (%s) in completed, got %+v", done.Ref, resp.Completed) + } +} + +// TestProjectStandupEndpoint_HonorsCollectionTerminalOptions pins the +// mechanical half of BUG-1049: listTerminalItemsSince must resolve +// terminal values per collection via TerminalValuesForDoneField, not the +// global DefaultTerminalStatuses union. A collection that only declares +// "shipped" as terminal must not report a "done" item as completed. +func TestProjectStandupEndpoint_HonorsCollectionTerminalOptions(t *testing.T) { + srv := testServer(t) + slug := createWSWithCollections(t, srv) + + rr := doRequest(srv, "POST", "/api/v1/workspaces/"+slug+"/collections", map[string]interface{}{ + "name": "Ships", + "slug": "ships", + "prefix": "SHIP", + "schema": `{"fields":[{"key":"status","type":"select","options":["open","done","shipped"],"terminal_options":["shipped"],"default":"open"}]}`, + }) + if rr.Code != http.StatusCreated { + t.Fatalf("create ships collection: expected 201, got %d: %s", rr.Code, rr.Body.String()) + } + + shipped := createItem(t, srv, slug, "ships", map[string]interface{}{ + "title": "Actually shipped", + "fields": `{"status":"shipped"}`, + }) + createItem(t, srv, slug, "ships", map[string]interface{}{ + "title": "Mislabelled done", + "fields": `{"status":"done"}`, + }) + + rr = doRequest(srv, "GET", "/api/v1/workspaces/"+slug+"/standup", nil) + if rr.Code != http.StatusOK { + t.Fatalf("standup: expected 200, got %d: %s", rr.Code, rr.Body.String()) + } + var resp StandupResponse + parseJSON(t, rr, &resp) + + if len(resp.Completed) != 1 || resp.Completed[0].Ref != shipped.Ref { + t.Fatalf("expected only the shipped item (%s) in completed (done is not terminal on this collection), got %+v", shipped.Ref, resp.Completed) + } +} + +func TestProjectChangelogEndpoint_DisabledConventionNotCompleted(t *testing.T) { + srv := testServer(t) + slug := createWSWithCollections(t, srv) + + createItem(t, srv, slug, "conventions", map[string]interface{}{ + "title": "Never push directly to main", + "fields": `{"status":"disabled"}`, + }) + done := createItem(t, srv, slug, "tasks", map[string]interface{}{ + "title": "Shipped it", + "fields": `{"status":"done"}`, + }) + + rr := doRequest(srv, "GET", "/api/v1/workspaces/"+slug+"/changelog", nil) + if rr.Code != http.StatusOK { + t.Fatalf("changelog: expected 200, got %d: %s", rr.Code, rr.Body.String()) + } + var resp ChangelogResponse + parseJSON(t, rr, &resp) + if resp.Total != 1 { + t.Fatalf("expected 1 completed item (the done task), got %d (%+v)", resp.Total, resp) + } + if len(resp.Groups) != 1 || resp.Groups[0].Items[0].Ref != done.Ref { + t.Fatalf("expected only the done task (%s), got %+v", done.Ref, resp.Groups) + } + for _, g := range resp.Groups { + if g.Collection == "Conventions" { + t.Fatalf("changelog must not group disabled conventions as completed: %+v", resp.Groups) + } + } +} diff --git a/internal/store/reports.go b/internal/store/reports.go index 635ce460c..006320c92 100644 --- a/internal/store/reports.go +++ b/internal/store/reports.go @@ -18,29 +18,12 @@ import ( // throughput bucketed over a window, net flow, completed-by-collection, and a // current status-distribution snapshot. "Completed" is a status_transitions // row INTO a *positive* terminal value (a terminal option that isn't a -// negative outcome like rejected/cancelled — see negativeTerminals), counted -// per the collection's done field. Created counts items.created_at. +// negative outcome like rejected/cancelled — see models.NegativeTerminals), +// counted per the collection's done field. Created counts items.created_at. // // Everything routes date math through dialect.DateBucket so the same query // runs on SQLite and Postgres. -// negativeTerminals are terminal status values that represent a NON-shipping -// close (the work didn't complete positively). They're excluded from the -// "completed" throughput so a rejected idea or cancelled task isn't counted as -// a completion. Matched case-insensitively against a collection's terminal -// options. (A future task can make this per-collection configurable; for now -// it's a sensible global default — PLAN-1628 decision.) -var negativeTerminals = map[string]bool{ - "rejected": true, - "cancelled": true, - "canceled": true, - "wontfix": true, - "won't fix": true, - "duplicate": true, - "declined": true, - "abandoned": true, -} - // ReportOptions parameterizes GetReport. type ReportOptions struct { // Window is one of "day", "week", "2wk", "month". Invalid/empty defaults @@ -692,7 +675,7 @@ func (s *Store) resolveReportCollections(workspaceID string, opts ReportOptions) var positives []string for _, v := range terminals { - if negativeTerminals[strings.ToLower(strings.TrimSpace(v))] { + if models.IsNegativeTerminal(v) { continue } positives = append(positives, v) diff --git a/internal/store/reports_test.go b/internal/store/reports_test.go index ea055f631..af8b71a48 100644 --- a/internal/store/reports_test.go +++ b/internal/store/reports_test.go @@ -697,3 +697,42 @@ func TestBackfillStatusTransitions_SeedSeqBelowHop(t *testing.T) { t.Fatalf("create-seed seq (%d) must be below the hop seq (%d)", seedSeq, hopSeq) } } + +func TestGetReport_DisabledConventionNotCompleted(t *testing.T) { + s := testStore(t) + u, err := s.CreateUser(models.UserCreate{Name: "C", Email: "c@example.com"}) + if err != nil { + t.Fatalf("create user: %v", err) + } + ws, err := s.CreateWorkspace(models.WorkspaceCreate{Name: "Conv", Slug: "conv", OwnerID: u.ID}) + if err != nil { + t.Fatalf("create workspace: %v", err) + } + col, err := s.CreateCollection(ws.ID, models.CollectionCreate{ + Name: "Conventions", + Slug: "conventions", + Prefix: "CONVE", + Schema: `{"fields":[{"key":"status","label":"Status","type":"select","options":["active","draft","disabled"],"terminal_options":["disabled"],"default":"active","required":true}]}`, + }) + if err != nil { + t.Fatalf("create conventions: %v", err) + } + item, err := s.CreateItem(ws.ID, col.ID, models.ItemCreate{ + Title: "Never push directly to main", + Fields: `{"status":"active"}`, + }) + if err != nil { + t.Fatalf("create item: %v", err) + } + if _, err := s.UpdateItem(item.ID, models.ItemUpdate{Fields: strPtr(`{"status":"disabled"}`)}); err != nil { + t.Fatalf("disable: %v", err) + } + + rep, err := s.GetReport(ws.ID, ReportOptions{Window: "week", Now: time.Now().UTC()}) + if err != nil { + t.Fatalf("GetReport: %v", err) + } + if rep.Totals.Completed != 0 { + t.Fatalf("disabled convention must not count as completed work, got %d", rep.Totals.Completed) + } +}