fix(mcp): six surgical fixes for BUG-987 (bugs 6, 8, 11, 12, 13, 14) (#361)

* fix(mcp): six surgical fixes for BUG-987 (bugs 6, 8, 11, 12, 13, 14)

Hotfix follow-up to v0.1.0-rc.3's Claude Desktop dogfood. Six
surgical fixes; bigger items (5, 7, 9, 10) deferred to separate
tasks.

- Bug 6: `pad project next --format json` was emitting the entire
  dashboard, indistinguishable from `pad project dashboard --format
  json`. Now slices to suggested_next only. cmd/pad/main.go.

- Bug 8: standup blockers carried empty `ref` strings, blocking
  agent linkback to the actually-blocked items. dashboard's
  attention[].item_ref is canonical; the standup composer in
  internal/mcp/dispatch_http_slice4.go just wasn't propagating it.
  Same fix applied to suggested_next entries.

- Bug 11: cobra's auto-emitted "Usage: pad item block ..." help
  block leaked into MCP error envelopes via classifyExecError. The
  Usage text references OLD CLI verb names (pre-v0.2 catalog) that
  agents using the new surface have no business seeing, and bloats
  every error response. New stripCobraUsageBlock helper truncates
  stderr at the first line-anchored "Usage:" marker before
  classification + envelope construction.

- Bug 12: BuildCLIArgs validation errors (missing required arg, type
  mismatch) came out of env.Dispatch as bare-text NewToolResultErrorf
  results, breaking the structured envelope contract. New helper
  validationFailedFromBuildErr wraps them as ErrValidationFailed
  envelopes with the field name extracted via regex from the
  underlying message.

- Bug 13: every Task / Idea / Plan with a `priority` field got a
  phantom `convention: { enforcement: "<priority>" }` surfaced on
  its response, because ExtractItemConventionMetadata's legacy
  fallback treated `priority` as the Convention enforcement tier
  unconditionally. Restructured to track hasConventionShape
  separately from hasMetadata — only Convention-specific markers
  (structured convention field, trigger, scope, surfaces, commands,
  direct enforcement) flip the shape flag. category alone is
  insufficient (Ideas / Bugs / Roadmap items legitimately use it).
  Final guard returns nil when only category was matched.

- Bug 14: GetRoleBreakdown's unassigned row was emitted with empty
  role_name + role_slug, presenting as a "phantom" entry in the
  dashboard. Now explicitly labelled "Unassigned" / "unassigned"
  while keeping role_id null so it's still distinguishable from a
  real role.

Tests:
- internal/mcp/bug987_test.go (new) — stripCobraUsageBlock + classify
  + validation envelope wrapping + env.Dispatch integration.
- internal/models/item_test.go — three cases covering non-Convention
  items (Task, Idea, Plan with priority) returning nil metadata, and
  one preservation test for legacy Conventions with priority field.
- internal/store/agent_roles_test.go (new) — confirms unassigned row
  carries explicit "Unassigned" / "unassigned" labels.

Live verified: pad_project action=next returns just the suggestions
array; pad_item action=create with no fields returns validation_failed
with field=collection; pad_item action=link with self-target returns
without Usage-block leakage.

Deferred to separate items (per BUG-987 triage):
- Bug 5: text vs JSON returns across note, decide, star, unstar,
  delete, bulk-update — needs CLI-side handler updates per command.
- Bug 7: suggested_next algorithm — needs to consider in-progress
  items, not just open ones; behavior change needs design.
- Bug 9: fields/tags double-stringified — potentially breaking for
  web UI/CLI consumers.
- Bug 10: decision_log/notes embedded in fields blob duplicating
  top-level arrays — might require data migration.

Parent: BUG-987.

* fix(mcp): HTTP transport equivalence + ordering for BUG-987 per Codex review (round 1)

Two findings from Codex review of PR #361:

1. project.next on HTTP transport still returned the full dashboard.
   The route table mapped "project next" directly to /dashboard, so
   the CLI fix (slice to suggested_next) didn't reach OAuth-authed
   agents going through HTTPHandlerDispatcher. Catalog actions must
   produce equivalent shapes on stdio and HTTP — that's the contract
   that lets agents be transport-agnostic.

   Fix: new dispatchProjectNext method on HTTPHandlerDispatcher that
   fetches the dashboard via the existing fetchDashboardJSON helper,
   slices to suggested_next[], re-encodes, and runs through
   packageJSONResult so it gets the same {items: [...]} wrap as
   other list responses.

   Also retires the broken route-table entry — replaced with a
   comment pointing at the new method so future contributors don't
   re-add a passthrough.

   Test: TestDispatch_ProjectNext_SlicesToSuggestedNext + the empty-
   array case. Asserts dashboard-only top-level fields (summary,
   active_items) don't leak into the response — that's the whole
   point of project.next being distinct from project.dashboard.

2. ExtractItemConventionMetadata's priority→enforcement legacy
   fallback ran BEFORE surfaces/scope/commands had a chance to flip
   hasConventionShape, so a Convention with only `{scope, priority}`
   would silently drop enforcement.

   Fix: move the priority fallback to AFTER all marker checks. Direct
   `enforcement` still resolves first; the legacy priority fallback
   runs at the bottom once shape detection is complete.

   Tests: two new cases covering scope-only and commands-only legacy
   Conventions — both must resolve enforcement via the priority
   fallback.

Parent: BUG-987.

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

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 <source-ref> <target-ref> [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 <ref>",
+			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 <source-ref> <target-ref> [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 <source-ref>") {
+		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)
+	}
+}

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)

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":

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))
 	}
 }
 

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,

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"),
 		})

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: <type-mismatch>") 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
 //

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)
 }
 

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: <priority>` 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 {

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,

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
+}