diff --git a/cmd/pad/main.go b/cmd/pad/main.go index 4858a908..0268fe30 100644 --- a/cmd/pad/main.go +++ b/cmd/pad/main.go @@ -3645,13 +3645,19 @@ func nextCmd() *cobra.Command { return err } - if formatFlag == "json" { - return cli.PrintJSON(dashJSON) - } - + // Decode once; both the JSON branch and the human-readable + // branch use the same suggested_next slice. + // + // BUG-987 bug 6: previously the JSON branch dumped the + // entire dashboard, making `project next --format json` + // indistinguishable from `project dashboard --format json`. + // Now it emits ONLY the recommended-next array (with the + // item-ref + reason fields agents need), matching the human + // branch's framing. var dash struct { SuggestedNext []struct { ItemSlug string `json:"item_slug"` + ItemRef string `json:"item_ref,omitempty"` ItemTitle string `json:"item_title"` Collection string `json:"collection"` Reason string `json:"reason"` @@ -3662,6 +3668,10 @@ func nextCmd() *cobra.Command { return err } + if formatFlag == "json" { + return cli.PrintJSON(dash.SuggestedNext) + } + if len(dash.SuggestedNext) == 0 { fmt.Println("No suggestions — all tasks may be complete or no active plans found.") return nil diff --git a/internal/mcp/bug987_test.go b/internal/mcp/bug987_test.go new file mode 100644 index 00000000..30e8b12e --- /dev/null +++ b/internal/mcp/bug987_test.go @@ -0,0 +1,188 @@ +package mcp + +import ( + "context" + "errors" + "strings" + "testing" + + "github.com/PerpetualSoftware/pad/internal/cmdhelp" +) + +// Tests covering BUG-987's MCP-layer fixes (Bugs 11, 12; Bugs 6, 8, +// 13, 14 land in their respective package tests). + +// TestStripCobraUsageBlock verifies the helper that scrubs cobra's +// auto-emitted "Usage: ..." block from CLI stderr before it reaches +// MCP error envelopes (BUG-987 bug 11). The Usage block leaks old +// CLI verb names (e.g. `pad item block`) into responses agents see +// — confusing for agents using the v0.2 catalog and fragile against +// future CLI flag changes. +func TestStripCobraUsageBlock(t *testing.T) { + cases := []struct { + name string + in string + want string + }{ + { + name: "strips standard cobra usage block", + in: `Error: cannot link an item to itself +Usage: + pad item block [flags] + +Flags: + -h, --help help for block + +Global Flags: + --format string output format`, + want: "Error: cannot link an item to itself", + }, + { + name: "passes through when no Usage block present", + in: "Error: item TASK-99 not found", + want: "Error: item TASK-99 not found", + }, + { + name: "Usage marker as substring of word — not stripped", + in: "Error: this misUsage: case is rare but possible", + want: "Error: this misUsage: case is rare but possible", + }, + { + name: "trims trailing whitespace before Usage", + in: "Error: bad input\n\n \nUsage:\n pad item show ", + want: "Error: bad input", + }, + { + name: "empty input", + in: "", + want: "", + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + if got := stripCobraUsageBlock(tc.in); got != tc.want { + t.Errorf("got %q\nwant %q", got, tc.want) + } + }) + } +} + +// TestClassifyExecError_StripsUsageBlock confirms classifyExecError +// removes Usage text BEFORE pattern matching and BEFORE placing the +// remaining stderr into the envelope. Without this, agents see +// `pad item block ...` references in error responses despite the +// MCP catalog using `pad_item action=link link_type=blocks`. +func TestClassifyExecError_StripsUsageBlock(t *testing.T) { + stderr := `Error: cannot link an item to itself +Usage: + pad item block [flags] + +Flags: + -h, --help help for block` + + res := classifyExecError(context.Background(), + []string{"item", "block"}, + errors.New("exit 1"), + stderr, + nil, + ) + body := textOf(res) + if strings.Contains(body, "Usage:") { + t.Errorf("envelope leaked Usage block: %s", body) + } + if strings.Contains(body, "pad item block ") { + t.Errorf("envelope leaked old CLI verb help: %s", body) + } + if !strings.Contains(body, "cannot link an item to itself") { + t.Errorf("envelope dropped the actual error message: %s", body) + } +} + +// TestValidationFailedFromBuildErr verifies that BuildCLIArgs error +// strings are wrapped as structured validation_failed envelopes +// instead of bare-text results (BUG-987 bug 12). +func TestValidationFailedFromBuildErr(t *testing.T) { + t.Run("missing required argument extracts field name", func(t *testing.T) { + err := errors.New(`missing required argument "title"`) + res := validationFailedFromBuildErr("item create", err) + if !res.IsError { + t.Errorf("IsError = false, want true") + } + env := decodeEnvelope(t, res) + if env.Error.Code != ErrValidationFailed { + t.Errorf("Code = %q, want %q", env.Error.Code, ErrValidationFailed) + } + if env.Error.Field != "title" { + t.Errorf("Field = %q, want title", env.Error.Field) + } + if !strings.Contains(env.Error.Message, "item create") { + t.Errorf("Message should reference cmdPath; got %q", env.Error.Message) + } + }) + + t.Run("flag type mismatch extracts field name", func(t *testing.T) { + err := errors.New(`flag "limit": expected number, got string`) + res := validationFailedFromBuildErr("item list", err) + env := decodeEnvelope(t, res) + if env.Error.Code != ErrValidationFailed { + t.Errorf("Code = %q, want %q", env.Error.Code, ErrValidationFailed) + } + if env.Error.Field != "limit" { + t.Errorf("Field = %q, want limit", env.Error.Field) + } + }) + + t.Run("unrecognized error message still produces validation envelope", func(t *testing.T) { + err := errors.New("totally novel error format") + res := validationFailedFromBuildErr("item show", err) + env := decodeEnvelope(t, res) + if env.Error.Code != ErrValidationFailed { + t.Errorf("Code = %q, want %q", env.Error.Code, ErrValidationFailed) + } + // Field stays empty when the regex misses; the underlying + // message text still carries the detail. + if env.Error.Field != "" { + t.Errorf("Field = %q, want empty", env.Error.Field) + } + if !strings.Contains(env.Error.Message, "totally novel error") { + t.Errorf("Message should preserve underlying text; got %q", env.Error.Message) + } + }) +} + +// TestEnvDispatch_ValidationErrorIsStructured drives env.Dispatch +// with a missing-arg input and confirms the resulting error result is +// a structured validation_failed envelope, not a bare-text result. +// This is the integration counterpart to TestValidationFailedFromBuildErr. +func TestEnvDispatch_ValidationErrorIsStructured(t *testing.T) { + doc := &cmdhelp.Document{ + Binary: "pad", + Commands: map[string]cmdhelp.Command{ + "item show": { + Args: []cmdhelp.Arg{{Name: "ref", Required: true}}, + }, + }, + } + env := ActionEnv{ + Doc: doc, + Workspace: NewWorkspaceState(""), + Dispatcher: &fakeDispatcher{}, + } + // Missing required `ref` — BuildCLIArgs returns an error, + // env.Dispatch must wrap it as validation_failed. + res, err := env.Dispatch(context.Background(), []string{"item", "show"}, map[string]any{}) + if err != nil { + t.Fatalf("Dispatch returned protocol error: %v", err) + } + if !res.IsError { + t.Fatalf("expected IsError, got success: %s", textOf(res)) + } + env2 := decodeEnvelope(t, res) + if env2.Error.Code != ErrValidationFailed { + t.Errorf("Code = %q, want %q (full envelope: %+v)", + env2.Error.Code, ErrValidationFailed, env2.Error) + } + if env2.Error.Field != "ref" { + t.Errorf("Field = %q, want ref", env2.Error.Field) + } +} diff --git a/internal/mcp/catalog.go b/internal/mcp/catalog.go index 2ac78df7..34c82b47 100644 --- a/internal/mcp/catalog.go +++ b/internal/mcp/catalog.go @@ -155,7 +155,13 @@ func (env ActionEnv) Dispatch(ctx context.Context, cmdPath []string, input map[s } cliArgs, err := BuildCLIArgs(cmdInfo, input, env.Workspace.Get(), env.RootFlags) if err != nil { - return mcp.NewToolResultErrorf("%s", err.Error()), nil + // BUG-987 bug 12: BuildCLIArgs returns plain Go errors for + // missing required args / bad types. Previously those came + // out as bare-text MCP results, breaking the structured + // envelope contract that every other error path follows. + // Wrap as validation_failed so agents see a consistent + // shape across the surface and can branch on error.code. + return validationFailedFromBuildErr(pathStr, err), nil } ctx = WithDispatchInput(ctx, mergeDispatchInput(input, env.Workspace.Get(), env.RootFlags)) return env.Dispatcher.Dispatch(ctx, cmdPath, cliArgs) diff --git a/internal/mcp/dispatch_http.go b/internal/mcp/dispatch_http.go index c344da7d..f13878c9 100644 --- a/internal/mcp/dispatch_http.go +++ b/internal/mcp/dispatch_http.go @@ -283,6 +283,8 @@ func (d *HTTPHandlerDispatcher) Dispatch(ctx context.Context, cmdPath, _ []strin return d.dispatchProjectReady(ctx, input, user) case "project stale": return d.dispatchProjectStale(ctx, input, user) + case "project next": + return d.dispatchProjectNext(ctx, input, user) case "project standup": return d.dispatchProjectStandup(ctx, input, user) case "project changelog": diff --git a/internal/mcp/dispatch_http_project_test.go b/internal/mcp/dispatch_http_project_test.go index 372123c8..80b464ad 100644 --- a/internal/mcp/dispatch_http_project_test.go +++ b/internal/mcp/dispatch_http_project_test.go @@ -12,21 +12,80 @@ import ( // --- project next / ready / stale --- -func TestRouteTable_ProjectNextAliasesDashboard(t *testing.T) { - // `pad project next --format json` returns the FULL dashboard - // JSON verbatim (cmd/pad/main.go nextCmd's `cli.PrintJSON(dashJSON)` - // path). The MCP route-table entry is a straight alias; this test - // pins that the URL is the dashboard endpoint and the agent gets - // the same payload they'd get from `project dashboard`. - m, p, _, err := routeTable["project next"](map[string]any{"workspace": "docapp"}) - if err != nil { - t.Fatalf("routeTable[project next]: %v", err) +// TestDispatch_ProjectNext_SlicesToSuggestedNext is the post-BUG-987 +// regression test. Pre-fix, `project next` was a route-table alias +// for /dashboard and returned the entire dashboard payload — making +// the action indistinguishable from `project dashboard`. Now it's +// dispatched as a method on HTTPHandlerDispatcher that fetches the +// dashboard then slices to suggested_next, matching the CLI's +// post-fix `pad project next --format json` behaviour. +func TestDispatch_ProjectNext_SlicesToSuggestedNext(t *testing.T) { + mux := http.NewServeMux() + mux.HandleFunc("/api/v1/workspaces/docapp/dashboard", func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{ + "summary": {"total_items": 99}, + "active_items": [{"slug":"x"}], + "suggested_next": [ + {"item_ref":"TASK-1","item_title":"First","reason":"high priority"}, + {"item_ref":"TASK-2","item_title":"Second","reason":"in_progress"} + ] + }`)) + }) + d := &HTTPHandlerDispatcher{Handler: mux, UserResolver: fixedUserResolver(&models.User{ID: "u"})} + res, err := d.Dispatch( + WithDispatchInput(context.Background(), map[string]any{"workspace": "docapp"}), + []string{"project", "next"}, nil, + ) + if err != nil || res.IsError { + t.Fatalf("Dispatch err=%v IsError=%v: %#v", err, res != nil && res.IsError, res) } - if m != http.MethodGet { - t.Errorf("method = %q", m) + // Wrapped as {items: [...]} per BUG-985 fix. + wrapped, ok := res.StructuredContent.(map[string]any) + if !ok { + t.Fatalf("structuredContent = %T, want map[string]any", res.StructuredContent) } - if p != "/api/v1/workspaces/docapp/dashboard" { - t.Errorf("path = %q", p) + items, ok := wrapped["items"].([]any) + if !ok { + t.Fatalf("items field missing or wrong type: %#v", wrapped) + } + if len(items) != 2 { + t.Errorf("expected 2 suggestions, got %d", len(items)) + } + // Critical: dashboard-only fields must NOT appear at the top level + // of the structured content (the WHOLE point of project.next is to + // be smaller than the dashboard). + for _, leaked := range []string{"summary", "active_items"} { + if _, present := wrapped[leaked]; present { + t.Errorf("project.next leaked dashboard field %q at top level: %#v", leaked, wrapped) + } + } +} + +// TestDispatch_ProjectNext_EmptyDashboardYieldsEmptyArray covers the +// "no candidates" path — the response must still produce a valid +// items envelope, not return an error or a missing field. +func TestDispatch_ProjectNext_EmptyDashboardYieldsEmptyArray(t *testing.T) { + mux := http.NewServeMux() + mux.HandleFunc("/api/v1/workspaces/docapp/dashboard", func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"suggested_next": []}`)) + }) + d := &HTTPHandlerDispatcher{Handler: mux, UserResolver: fixedUserResolver(&models.User{ID: "u"})} + res, err := d.Dispatch( + WithDispatchInput(context.Background(), map[string]any{"workspace": "docapp"}), + []string{"project", "next"}, nil, + ) + if err != nil || res.IsError { + t.Fatalf("Dispatch err=%v IsError=%v: %#v", err, res != nil && res.IsError, res) + } + wrapped, _ := res.StructuredContent.(map[string]any) + items, _ := wrapped["items"].([]any) + if items == nil { + t.Errorf("expected empty items array, got %#v", wrapped) + } + if len(items) != 0 { + t.Errorf("expected 0 suggestions, got %d", len(items)) } } diff --git a/internal/mcp/dispatch_http_routes.go b/internal/mcp/dispatch_http_routes.go index ebcb0435..6178c63c 100644 --- a/internal/mcp/dispatch_http_routes.go +++ b/internal/mcp/dispatch_http_routes.go @@ -312,10 +312,13 @@ func init() { // returns dashJSON verbatim). `ready` and `stale` get custom // dispatchers because their CLI JSON output is `{count, results}` // post-filter, not the raw dashboard. - "project next": routeSpec{ - method: http.MethodGet, - pathTemplate: "/api/v1/workspaces/{workspace}/dashboard", - }.toRouteMapper(), + // `project next` is dispatched as a method on + // HTTPHandlerDispatcher (see dispatch_http.go's switch) so it + // can fetch the dashboard then slice to suggested_next only — + // matching the CLI behaviour after BUG-987 bug 6's fix. + // Routing this entry to the bare /dashboard endpoint would + // re-introduce the full-dashboard regression on the HTTP + // transport. // --- Admin: collections --- "collection create": mapCollectionCreate, diff --git a/internal/mcp/dispatch_http_slice4.go b/internal/mcp/dispatch_http_slice4.go index 0d1f0e7f..52867b88 100644 --- a/internal/mcp/dispatch_http_slice4.go +++ b/internal/mcp/dispatch_http_slice4.go @@ -19,6 +19,39 @@ import ( // --- project standup --- +// dispatchProjectNext reproduces `pad project next --format json` +// after BUG-987 bug 6's fix: fetch the dashboard, return ONLY the +// suggested_next array. Without this method, "project next" routed +// straight to /dashboard via the route table — making the HTTP +// transport's response indistinguishable from project dashboard, +// which the CLI no longer does. Catalog actions must produce the +// same shape on both transports. +func (d *HTTPHandlerDispatcher) dispatchProjectNext( + ctx context.Context, + input map[string]any, + user *models.User, +) (*mcp.CallToolResult, error) { + const cmdKey = "project next" + dash, errRes := d.fetchDashboardJSON(ctx, input, user, cmdKey) + if errRes != nil { + return errRes, nil + } + suggestions := dashboardArrayField(dash, "suggested_next") + if suggestions == nil { + // Distinguish "no suggestions" from a totally absent field — + // emit an empty slice so consumers see a stable shape. + suggestions = []map[string]any{} + } + // Re-encode the slice to drive packageJSONResult's + // array-wrap-as-{items: [...]} path (BUG-985 fix). Same wire + // shape MCP host validators expect. + body, err := json.Marshal(suggestions) + if err != nil { + return mcp.NewToolResultErrorf("%s: marshal suggestions: %s", cmdKey, err.Error()), nil + } + return packageJSONResult(string(body)), nil +} + // dispatchProjectStandup reproduces `pad project standup --format // json`: fetches the dashboard for blockers + suggested-next, lists // items in each terminal status to find recently completed work, @@ -121,14 +154,22 @@ func (d *HTTPHandlerDispatcher) dispatchProjectStandup( blockers := make([]standupItem, 0) for _, a := range dashboardArrayField(dash, "attention") { + // BUG-987 bug 8: previously omitted Ref, leaving agents + // unable to link the blocker entry back to the blocked item. + // The dashboard's attention[].item_ref is the canonical issue + // ref (e.g. TASK-7), already populated by the dashboard handler. blockers = append(blockers, standupItem{ + Ref: stringFromMap(a, "item_ref"), Title: stringFromMap(a, "item_title"), Reason: stringFromMap(a, "reason"), }) } suggested := make([]standupItem, 0) for _, s := range dashboardArrayField(dash, "suggested_next") { + // Same Ref-omission as blockers above. dashboard's + // suggested_next[].item_ref carries the canonical issue ref. suggested = append(suggested, standupItem{ + Ref: stringFromMap(s, "item_ref"), Title: stringFromMap(s, "item_title"), Reason: stringFromMap(s, "reason"), }) diff --git a/internal/mcp/errors.go b/internal/mcp/errors.go index 4f051247..5b55dc2a 100644 --- a/internal/mcp/errors.go +++ b/internal/mcp/errors.go @@ -250,6 +250,14 @@ func envelopeFrom(res *mcp.CallToolResult) ErrorEnvelope { // structured envelope. lookup is optional — when supplied, no_workspace // errors get available_workspaces enrichment. func classifyExecError(ctx context.Context, cmdPath []string, runErr error, stderr string, lookup WorkspaceLister) *mcp.CallToolResult { + // BUG-987 bug 11: cobra automatically appends a "Usage: ..." block + // to stderr when a command fails with a runtime error. That help + // text uses the OLD CLI verb names (e.g. `pad item block`) which + // confuses agents using the v0.2 catalog (`pad_item action=link + // link_type=blocks`) and bloats error messages. Strip the Usage + // block before classification so neither matchers nor envelope + // content carry it. + stderr = stripCobraUsageBlock(stderr) stderr = strings.TrimSpace(stderr) lower := strings.ToLower(stderr) @@ -349,6 +357,99 @@ func extractUnknownWorkspaceSlug(stderr string) string { return m[1] } +// validationFailedFromBuildErr wraps a BuildCLIArgs error (typically +// "missing required argument %q" or "flag %q: ") as a +// structured validation_failed envelope. Best-effort extraction of +// the offending field name out of Go's error wrapping conventions — +// when the regex misses, the message itself still carries the +// underlying text. +// +// BUG-987 bug 12: previously BuildCLIArgs failures came out of +// env.Dispatch as bare mcp.NewToolResultErrorf strings, breaking the +// structured-envelope invariant that every other error path follows. +func validationFailedFromBuildErr(cmdPath string, err error) *mcp.CallToolResult { + msg := err.Error() + field := extractValidationField(msg) + payload := ErrorPayload{ + Code: ErrValidationFailed, + Message: fmt.Sprintf("validation failed for `%s`: %s", cmdPath, msg), + Field: field, + } + return NewErrorResult(payload) +} + +// reValidationField matches the field-name token in BuildCLIArgs's +// error strings. Both `argument "x"` and `flag "x"` formats cover +// ~all of its err paths (see internal/mcp/dispatch.go's BuildCLIArgs). +var reValidationField = regexp.MustCompile(`(?:argument|flag)\s+"([^"]+)"`) + +// extractValidationField pulls the field name out of a BuildCLIArgs +// error message, returning empty string when no match is found. +func extractValidationField(msg string) string { + m := reValidationField.FindStringSubmatch(msg) + if len(m) < 2 { + return "" + } + return m[1] +} + +// stripCobraUsageBlock removes cobra's auto-appended "Usage: ..." +// help block from a stderr string. cobra emits this block on any +// runtime error from a RunE handler — useful for human users running +// the CLI directly, but noise in MCP error envelopes (and worse, +// references CLI verb names like `pad item block` that aren't part +// of the v0.2 MCP catalog at all, BUG-987 bug 11). +// +// The block is recognizable: a line containing exactly "Usage:" +// (with optional surrounding whitespace) followed by the help text. +// Truncate at the first such line. If no Usage block is present +// (the typical no-cobra-help error path), the input is returned +// unchanged. +func stripCobraUsageBlock(stderr string) string { + idx := indexOfUsageLine(stderr) + if idx < 0 { + return stderr + } + return strings.TrimRight(stderr[:idx], " \t\r\n") +} + +// indexOfUsageLine returns the byte offset of the line containing +// "Usage:" (cobra's help-block prefix), or -1 when absent. The match +// is anchored to a line start (preceded by '\n' or string start) so +// in-message mentions of the word "Usage:" don't accidentally +// truncate. +func indexOfUsageLine(s string) int { + const marker = "Usage:" + idx := 0 + for { + rel := strings.Index(s[idx:], marker) + if rel < 0 { + return -1 + } + abs := idx + rel + // Anchor: must be at the start of a line. A leading newline + // (with optional whitespace before the marker) qualifies. + if abs == 0 || isLineStart(s, abs) { + return abs + } + idx = abs + len(marker) + } +} + +// isLineStart returns true when the character at byte position i is +// preceded by a newline (with arbitrary leading whitespace allowed +// between the newline and i). +func isLineStart(s string, i int) bool { + for j := i - 1; j >= 0; j-- { + c := s[j] + if c == ' ' || c == '\t' { + continue + } + return c == '\n' + } + return true +} + // ───────────────────────────────────────────────────────────────────── // HTTPHandlerDispatcher classification // diff --git a/internal/models/item.go b/internal/models/item.go index 785c01e7..3d1996f7 100644 --- a/internal/models/item.go +++ b/internal/models/item.go @@ -202,6 +202,16 @@ func ExtractItemConventionMetadata(fieldsJSON string) *ItemConventionMetadata { var metadata ItemConventionMetadata hasMetadata := false + // hasConventionShape tracks whether we've found a Convention- + // SPECIFIC marker — the structured convention field, or one of + // trigger / surfaces / scope / commands / direct enforcement. + // `category` alone is NOT a Convention marker (Ideas, Bugs, Roadmap + // items also use category). Used to gate the priority→enforcement + // legacy fallback below; without this gate every Task/Idea with a + // `priority` field got a phantom `convention.enforcement` surfaced + // on its response (BUG-987 bug 13). + hasConventionShape := false + if raw, ok := fieldsMap[ItemFieldConvention]; ok { payload, err := json.Marshal(raw) if err == nil { @@ -215,6 +225,7 @@ func ExtractItemConventionMetadata(fieldsJSON string) *ItemConventionMetadata { Commands: append([]string(nil), structured.Commands...), } hasMetadata = true + hasConventionShape = true } } } @@ -223,45 +234,71 @@ func ExtractItemConventionMetadata(fieldsJSON string) *ItemConventionMetadata { if category, ok := fieldsMap["category"].(string); ok { metadata.Category = category hasMetadata = true + // Note: category alone does NOT flip hasConventionShape — + // many non-Convention collections legitimately use it. } } if metadata.Trigger == "" { if trigger, ok := fieldsMap["trigger"].(string); ok { metadata.Trigger = trigger hasMetadata = true + hasConventionShape = true } } + // Direct enforcement only — the priority fallback runs at the + // END so surfaces/scope/commands have a chance to flip + // hasConventionShape first. Without that ordering, a legacy + // Convention like `{scope:"all", priority:"must"}` (no trigger) + // would silently drop enforcement because the fallback ran + // before scope set hasConventionShape. if metadata.Enforcement == "" { - switch value := fieldsMap["enforcement"].(type) { - case string: + if value, ok := fieldsMap["enforcement"].(string); ok { metadata.Enforcement = value hasMetadata = true - default: - if priority, ok := fieldsMap["priority"].(string); ok { - metadata.Enforcement = priority - hasMetadata = true - } + hasConventionShape = true } } if len(metadata.Surfaces) == 0 { if surfaces := extractStringList(fieldsMap["surfaces"]); len(surfaces) > 0 { metadata.Surfaces = surfaces hasMetadata = true + hasConventionShape = true } else if scope, ok := fieldsMap["scope"].(string); ok && scope != "" { metadata.Surfaces = []string{scope} hasMetadata = true + hasConventionShape = true } } if len(metadata.Commands) == 0 { if commands := extractStringList(fieldsMap["commands"]); len(commands) > 0 { metadata.Commands = commands hasMetadata = true + hasConventionShape = true + } + } + + // Legacy priority→enforcement fallback. Runs AFTER all other + // markers because hasConventionShape only flips once we've seen + // a Convention-specific signal. Without this ordering, a legacy + // Convention with only `{scope, priority}` would lose its + // enforcement value because scope hadn't been processed yet + // (Codex review on PR #361 caught this). + if metadata.Enforcement == "" && hasConventionShape { + if priority, ok := fieldsMap["priority"].(string); ok { + metadata.Enforcement = priority } } if !hasMetadata { return nil } + // Final guard: if we ONLY matched on `category` (no Convention- + // specific markers), the item isn't a Convention. Suppress the + // metadata entirely — surfacing { category } on a non-Convention + // item just for category alone produced confusing responses. + if !hasConventionShape { + return nil + } return normalizeItemConventionMetadata(&metadata) } diff --git a/internal/models/item_test.go b/internal/models/item_test.go index d4cf9227..b854386a 100644 --- a/internal/models/item_test.go +++ b/internal/models/item_test.go @@ -65,6 +65,86 @@ func TestExtractItemConventionMetadataFallsBackToLegacyFields(t *testing.T) { } } +// TestExtractItemConventionMetadata_NoLeakOnNonConventionItems is the +// regression test for BUG-987 bug 13. Previously every Task / Idea / +// Plan with a `priority` field got a phantom +// `convention.enforcement: ` surfaced on its response, +// because the legacy fallback in ExtractItemConventionMetadata +// unconditionally treated `priority` as the Convention enforcement +// tier. Tasks have priority but aren't Conventions; the metadata +// must NOT be synthesized for them. +func TestExtractItemConventionMetadata_NoLeakOnNonConventionItems(t *testing.T) { + cases := []struct { + name string + fields string + }{ + {"task with priority", `{"status":"open","priority":"high"}`}, + {"task with priority and category", `{"status":"open","priority":"high","category":"frontend"}`}, + {"idea with priority", `{"status":"new","priority":"medium","impact":"high"}`}, + {"plan with start_date and priority", `{"status":"active","priority":"high","start_date":"2026-01-01"}`}, + {"category alone is not a Convention signal", `{"category":"agent-integration","status":"new"}`}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got := ExtractItemConventionMetadata(tc.fields) + if got != nil { + t.Errorf("expected nil metadata for non-Convention item; got %+v", got) + } + }) + } +} + +// TestExtractItemConventionMetadata_ConventionWithLegacyPriority +// exercises the path where priority→enforcement legacy fallback IS +// expected to fire — items that carry Convention-specific markers +// (trigger, scope, etc.) but use the legacy `priority` field for +// enforcement. The bug 13 fix preserves this path. +func TestExtractItemConventionMetadata_ConventionWithLegacyPriority(t *testing.T) { + got := ExtractItemConventionMetadata(`{"status":"active","trigger":"on-commit","scope":"all","priority":"must"}`) + if got == nil { + t.Fatal("expected metadata for Convention with legacy priority field") + } + if got.Enforcement != "must" { + t.Errorf("Enforcement = %q, want must (priority legacy fallback)", got.Enforcement) + } + if got.Trigger != "on-commit" { + t.Errorf("Trigger = %q, want on-commit", got.Trigger) + } +} + +// TestExtractItemConventionMetadata_LegacyConvention_ScopeOnly is +// the regression test for Codex's PR #361 round-1 finding: a legacy +// Convention carrying only `{scope, priority}` (no trigger, no +// commands, no structured convention field) must still resolve +// priority→enforcement. Pre-fix, the fallback ran BEFORE scope had +// flipped hasConventionShape, so enforcement got silently dropped. +func TestExtractItemConventionMetadata_LegacyConvention_ScopeOnly(t *testing.T) { + got := ExtractItemConventionMetadata(`{"status":"active","scope":"all","priority":"must"}`) + if got == nil { + t.Fatal("expected metadata for legacy Convention with scope+priority") + } + if got.Enforcement != "must" { + t.Errorf("Enforcement = %q, want must (priority fallback after scope flips shape)", + got.Enforcement) + } + if len(got.Surfaces) != 1 || got.Surfaces[0] != "all" { + t.Errorf("Surfaces = %v, want [all]", got.Surfaces) + } +} + +// TestExtractItemConventionMetadata_LegacyConvention_CommandsOnly +// covers the equivalent path for the commands marker. +func TestExtractItemConventionMetadata_LegacyConvention_CommandsOnly(t *testing.T) { + got := ExtractItemConventionMetadata(`{"status":"active","commands":["go test"],"priority":"should"}`) + if got == nil { + t.Fatal("expected metadata for legacy Convention with commands+priority") + } + if got.Enforcement != "should" { + t.Errorf("Enforcement = %q, want should (priority fallback after commands flips shape)", + got.Enforcement) + } +} + func TestExtractItemImplementationNotes(t *testing.T) { notes := ExtractItemImplementationNotes(`{"status":"open","implementation_notes":[{"id":"note-1","summary":"Used SSE refresh","details":"Reload phase tasks on visibility resume","created_at":"2026-04-02T15:00:00Z","created_by":"agent"}]}`) if len(notes) != 1 { diff --git a/internal/store/agent_roles.go b/internal/store/agent_roles.go index d32246f2..17c6f28e 100644 --- a/internal/store/agent_roles.go +++ b/internal/store/agent_roles.go @@ -244,7 +244,15 @@ func (s *Store) GetRoleBreakdown(workspaceID string) ([]RoleBreakdown, error) { }) } - // Add unassigned + // Add unassigned. BUG-987 bug 14: previously the unassigned row + // was emitted with empty role_name + role_slug, which downstream + // consumers parsed as a "phantom" entry — visually misleading + // (appeared as a blank row with item_count > 0) and forced clients + // to special-case empty strings as "unassigned." Use explicit + // "Unassigned" / "unassigned" so the entry is self-describing, + // while still keeping role_id null so it's distinguishable from + // a real role with that slug (none can exist — `unassigned` is + // reserved by virtue of role_id being nil). if unassigned.count > 0 { var userList []string if unassigned.users != "" { @@ -252,8 +260,8 @@ func (s *Store) GetRoleBreakdown(workspaceID string) ([]RoleBreakdown, error) { } result = append(result, RoleBreakdown{ RoleID: nil, - RoleName: "", - RoleSlug: "", + RoleName: "Unassigned", + RoleSlug: "unassigned", RoleIcon: "", ItemCount: unassigned.count, Users: userList, diff --git a/internal/store/agent_roles_test.go b/internal/store/agent_roles_test.go new file mode 100644 index 00000000..d6a45c40 --- /dev/null +++ b/internal/store/agent_roles_test.go @@ -0,0 +1,60 @@ +package store + +import ( + "testing" + + "github.com/PerpetualSoftware/pad/internal/models" +) + +// TestGetRoleBreakdown_UnassignedRowHasExplicitLabels is the +// regression test for BUG-987 bug 14. Previously the unassigned-items +// row was emitted with empty role_name + role_slug, so dashboard +// consumers saw a "phantom" entry: a row with item_count > 0 but no +// identifying label. Now the row is explicitly labelled "Unassigned" +// / "unassigned" while still keeping role_id null (the marker that +// distinguishes an unassigned bucket from a real role with that slug). +func TestGetRoleBreakdown_UnassignedRowHasExplicitLabels(t *testing.T) { + s := testStore(t) + ws := newTestWorkspace(t, s, "rb-bug987") + + coll := createTestCollection(t, s, ws.ID, "tasks") + // One item with no agent role — should land in the unassigned row. + createTestItem(t, s, ws.ID, coll.ID, "Unassigned task", "") + + got, err := s.GetRoleBreakdown(ws.ID) + if err != nil { + t.Fatalf("GetRoleBreakdown: %v", err) + } + if len(got) != 1 { + t.Fatalf("expected 1 row (unassigned only); got %d", len(got)) + } + row := got[0] + if row.RoleID != nil { + t.Errorf("RoleID = %v, want nil for unassigned row", row.RoleID) + } + if row.RoleName != "Unassigned" { + t.Errorf("RoleName = %q, want \"Unassigned\"", row.RoleName) + } + if row.RoleSlug != "unassigned" { + t.Errorf("RoleSlug = %q, want \"unassigned\"", row.RoleSlug) + } + if row.ItemCount != 1 { + t.Errorf("ItemCount = %d, want 1", row.ItemCount) + } +} + +// newTestWorkspace creates a workspace bound to the test store with +// the given slug. Helper for store-level tests that don't already use +// the larger setup harness (e.g. permission tests). Kept minimal — +// just enough state for GetRoleBreakdown to succeed. +func newTestWorkspace(t *testing.T, s *Store, slug string) *models.Workspace { + t.Helper() + ws, err := s.CreateWorkspace(models.WorkspaceCreate{ + Slug: slug, + Name: slug, + }) + if err != nil { + t.Fatalf("CreateWorkspace: %v", err) + } + return ws +}