diff --git a/internal/mcp/catalog.go b/internal/mcp/catalog.go index 5b735516..88ae755a 100644 --- a/internal/mcp/catalog.go +++ b/internal/mcp/catalog.go @@ -83,10 +83,10 @@ type ToolSchema struct { // ParamDef declares one parameter on a tool's input schema. Type maps // to mcp-go's helper functions (WithString, WithNumber, WithBoolean, -// WithArray). Enum, when non-empty, constrains string parameters. +// WithArray, WithObject). Enum, when non-empty, constrains string parameters. type ParamDef struct { Name string - Type string // "string" | "number" | "bool" | "array" + Type string // "string" | "number" | "bool" | "array" | "object" Description string Enum []string } @@ -316,6 +316,14 @@ func paramDefToToolOption(p ParamDef) mcp.ToolOption { return mcp.WithBoolean(p.Name, propOpts...) case "array": return mcp.WithArray(p.Name, append(propOpts, mcp.WithStringItems())...) + case "object": + // Structured JSON parameter. The MCP host sees a generic object; + // per-tool semantics are encoded in the Description. Callers + // constructing the value can pass a native JSON object — + // BuildCLIArgs json-encodes it before handing it to the CLI as + // a string flag value (which the CLI's --schema flag accepts as + // inline JSON). + return mcp.WithObject(p.Name, propOpts...) default: return mcp.WithString(p.Name, propOpts...) } diff --git a/internal/mcp/catalog_collection.go b/internal/mcp/catalog_collection.go index 9dd705be..79e7bb1f 100644 --- a/internal/mcp/catalog_collection.go +++ b/internal/mcp/catalog_collection.go @@ -27,7 +27,12 @@ var padCollectionTool = ToolDef{ { Name: "fields", Type: "string", - Description: "Field DSL: \"key:type[:options]; ...\". Optional for action=create. Example: \"status:select:open,done; priority:select:high,medium,low\".", + Description: "Compact field DSL: \"key:type[:options]; ...\". Optional for action=create. Example: \"status:select:open,done; priority:select:high,medium,low\". Use `schema` instead when you need terminal_options, custom defaults, computed fields, suffixes, or relation collections — the DSL cannot express those. Mutually exclusive with `schema`.", + }, + { + Name: "schema", + Type: "object", + Description: "Structured CollectionSchema: {\"fields\":[{\"key\":\"...\",\"label\":\"...\",\"type\":\"...\",\"options\":[...],\"terminal_options\":[...],\"default\":\"...\",\"required\":bool,\"computed\":bool,\"suffix\":\"...\",\"collection\":\"...\"}]}. Optional for action=create. Use instead of `fields` when you need terminal_options or any FieldDef property the DSL cannot express. Missing `label` values are auto-filled from `key` using Title Case (e.g. due_date → Due Date). Mutually exclusive with `fields`.", }, { Name: "icon", @@ -71,8 +76,14 @@ Actions: Required: workspace. create — Create a new collection. Required: workspace, name. - Optional: fields, icon, description, layout, default_view, board_group_by. - Field DSL example: "status:select:open,done; priority:select:high,medium,low". + Optional: fields OR schema (mutually exclusive), icon, description, + layout, default_view, board_group_by. + DSL example: fields="status:select:open,done; priority:select:high,medium,low" + Schema example: schema={"fields":[{"key":"status","type":"select","options":["new","done"],"terminal_options":["done"]}]} + Prefer schema when terminal_options matters (driving dashboard + active/complete counts) or when fields need defaults, computed + flags, suffixes, or relation collections — the DSL cannot express + those. Schema for an individual collection is included in the list response — read it from there rather than calling list again. v0.2 does not expose a dedicated diff --git a/internal/mcp/dispatch.go b/internal/mcp/dispatch.go index cdd434b2..6d78b7c7 100644 --- a/internal/mcp/dispatch.go +++ b/internal/mcp/dispatch.go @@ -446,7 +446,11 @@ func BuildCLIArgs( } continue } - flagArgs = append(flagArgs, "--"+name, fmt.Sprint(val)) + encoded, err := encodeFlagValue(val) + if err != nil { + return nil, fmt.Errorf("flag %q: %w", name, err) + } + flagArgs = append(flagArgs, "--"+name, encoded) } } @@ -530,6 +534,35 @@ func toStringSlice(v any) ([]string, error) { return nil, fmt.Errorf("expected array or string, got %T", v) } +// encodeFlagValue stringifies an MCP input value for inclusion as a CLI +// flag argument. Strings and primitives pass through fmt.Sprint as before; +// structured values (maps, slices) are JSON-encoded so that flags accepting +// JSON literals — like `pad collection create --schema ''` — receive +// a valid JSON string rather than Go's map-print syntax. +// +// exec.CommandContext takes []string args directly, so no shell-quoting is +// involved; the JSON string travels intact to the subprocess and is parsed +// by the CLI flag's own decoder. +func encodeFlagValue(v any) (string, error) { + switch t := v.(type) { + case nil: + return "", nil + case string: + return t, nil + case bool, int, int32, int64, uint, uint32, uint64, float32, float64, json.Number: + return fmt.Sprint(v), nil + default: + // map[string]any, []any, or any other structured type: JSON-encode. + // Compact JSON keeps the CLI process-arg list small and avoids any + // embedded-newline weirdness. + b, err := json.Marshal(v) + if err != nil { + return "", fmt.Errorf("encode value: %w", err) + } + return string(b), nil + } +} + func toBool(v any) (bool, error) { switch t := v.(type) { case bool: diff --git a/internal/mcp/dispatch_http_project_test.go b/internal/mcp/dispatch_http_project_test.go index 29069de6..df83a724 100644 --- a/internal/mcp/dispatch_http_project_test.go +++ b/internal/mcp/dispatch_http_project_test.go @@ -383,6 +383,196 @@ func TestParseCollectionFieldsDSL_EmptyReturnsEmptyFields(t *testing.T) { } } +// TestMapCollectionCreate_AcceptsStructuredSchema verifies that the MCP +// dispatcher accepts a typed `schema` object input (the path TASK-1335 +// adds for BUG-1284) and passes it through to the create-collection +// body with terminal_options preserved — which the DSL path cannot +// express. +func TestMapCollectionCreate_AcceptsStructuredSchema(t *testing.T) { + _, _, body, err := mapCollectionCreate(map[string]any{ + "workspace": "docapp", + "name": "Marketing", + "schema": map[string]any{ + "fields": []any{ + map[string]any{ + "key": "status", + "label": "Status", + "type": "select", + "options": []any{"idea", "drafting", "published", "archived"}, + "terminal_options": []any{"published", "archived"}, + "default": "idea", + "required": true, + }, + }, + }, + }) + if err != nil { + t.Fatalf("mapCollectionCreate: %v", err) + } + var payload map[string]any + if err := json.Unmarshal(body, &payload); err != nil { + t.Fatalf("decode body: %v", err) + } + schemaStr, _ := payload["schema"].(string) + var schema map[string]any + if err := json.Unmarshal([]byte(schemaStr), &schema); err != nil { + t.Fatalf("decode schema: %v", err) + } + fields, _ := schema["fields"].([]any) + if len(fields) != 1 { + t.Fatalf("fields length = %d, want 1", len(fields)) + } + field, _ := fields[0].(map[string]any) + termOpts, _ := field["terminal_options"].([]any) + if len(termOpts) != 2 || termOpts[0] != "published" || termOpts[1] != "archived" { + t.Errorf("terminal_options not preserved through MCP body: %v", termOpts) + } + if field["default"] != "idea" { + t.Errorf("default not preserved: %v", field["default"]) + } + if field["required"] != true { + t.Errorf("required not preserved: %v", field["required"]) + } +} + +// TestMapCollectionCreate_SchemaBackfillsMissingLabel mirrors the CLI's +// label-backfill heuristic — a schema field omitting `label` should get +// one auto-filled from `key` so the web UI doesn't render blank headers. +func TestMapCollectionCreate_SchemaBackfillsMissingLabel(t *testing.T) { + _, _, body, err := mapCollectionCreate(map[string]any{ + "workspace": "docapp", + "name": "Marketing", + "schema": map[string]any{ + "fields": []any{ + map[string]any{ + "key": "due_date", + "type": "date", + }, + }, + }, + }) + if err != nil { + t.Fatalf("mapCollectionCreate: %v", err) + } + var payload map[string]any + if err := json.Unmarshal(body, &payload); err != nil { + t.Fatalf("decode body: %v", err) + } + schemaStr, _ := payload["schema"].(string) + var schema map[string]any + if err := json.Unmarshal([]byte(schemaStr), &schema); err != nil { + t.Fatalf("decode schema: %v", err) + } + fields, _ := schema["fields"].([]any) + if len(fields) != 1 { + t.Fatalf("fields length = %d, want 1", len(fields)) + } + field, _ := fields[0].(map[string]any) + if field["label"] != "Due Date" { + t.Errorf("label not backfilled: got %v, want \"Due Date\"", field["label"]) + } +} + +// TestMapCollectionCreate_SchemaAcceptsStringifiedJSON exercises the +// fallback shape for clients that can only pass strings — schema arrives +// as a JSON-encoded string and still round-trips through the body. +func TestMapCollectionCreate_SchemaAcceptsStringifiedJSON(t *testing.T) { + _, _, body, err := mapCollectionCreate(map[string]any{ + "workspace": "docapp", + "name": "Marketing", + "schema": `{"fields":[{"key":"status","type":"select","options":["a","b"],"terminal_options":["b"]}]}`, + }) + if err != nil { + t.Fatalf("mapCollectionCreate: %v", err) + } + var payload map[string]any + if err := json.Unmarshal(body, &payload); err != nil { + t.Fatalf("decode body: %v", err) + } + schemaStr, _ := payload["schema"].(string) + var schema map[string]any + if err := json.Unmarshal([]byte(schemaStr), &schema); err != nil { + t.Fatalf("decode schema: %v", err) + } + fields, _ := schema["fields"].([]any) + field, _ := fields[0].(map[string]any) + termOpts, _ := field["terminal_options"].([]any) + if len(termOpts) != 1 || termOpts[0] != "b" { + t.Errorf("terminal_options not preserved from stringified schema: %v", termOpts) + } +} + +// TestMapCollectionCreate_FieldsAndSchemaMutuallyExclusive ensures the +// dispatcher rejects requests that set both flags — mirrors the CLI-side +// guard so MCP clients see the same error shape. +func TestMapCollectionCreate_FieldsAndSchemaMutuallyExclusive(t *testing.T) { + _, _, _, err := mapCollectionCreate(map[string]any{ + "workspace": "docapp", + "name": "Both", + "fields": "status:select:open,done", + "schema": map[string]any{"fields": []any{}}, + }) + if err == nil { + t.Fatal("expected error when both fields and schema set, got nil") + } + if !strings.Contains(err.Error(), "mutually exclusive") { + t.Errorf("expected mutually-exclusive error, got: %v", err) + } +} + +// TestMapCollectionCreate_EmptySchemaFallsThroughToFields verifies the +// transport-symmetry fix per Codex round 1: when a client sends +// schema=null or schema="" alongside fields, the dispatcher treats the +// empty schema as absent and uses the fields DSL — matching how the +// CLI handles an empty --schema value. +func TestMapCollectionCreate_EmptySchemaFallsThroughToFields(t *testing.T) { + cases := []struct { + name string + schema any + }{ + {name: "nil-schema", schema: nil}, + {name: "empty-string-schema", schema: ""}, + {name: "whitespace-string-schema", schema: " "}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + _, _, body, err := mapCollectionCreate(map[string]any{ + "workspace": "docapp", + "name": "Falls", + "fields": "status:select:open,done", + "schema": tc.schema, + }) + if err != nil { + t.Fatalf("expected fall-through to --fields, got error: %v", err) + } + var payload map[string]any + if err := json.Unmarshal(body, &payload); err != nil { + t.Fatalf("decode body: %v", err) + } + schemaStr, _ := payload["schema"].(string) + if !strings.Contains(schemaStr, `"key":"status"`) { + t.Errorf("expected DSL-parsed status field, got schema: %q", schemaStr) + } + }) + } +} + +// TestMapCollectionCreate_SchemaMalformedJSON ensures a clear error when +// the stringified schema is invalid JSON. +func TestMapCollectionCreate_SchemaMalformedJSON(t *testing.T) { + _, _, _, err := mapCollectionCreate(map[string]any{ + "workspace": "docapp", + "name": "Bad", + "schema": `{not valid json`, + }) + if err == nil { + t.Fatal("expected error for malformed schema JSON, got nil") + } + if !strings.Contains(err.Error(), "invalid schema JSON") { + t.Errorf("expected 'invalid schema JSON' in error, got: %v", err) + } +} + // --- library list --- func TestDispatch_LibraryList_BothEndpoints(t *testing.T) { diff --git a/internal/mcp/dispatch_http_routes.go b/internal/mcp/dispatch_http_routes.go index 6178c63c..8454dc5a 100644 --- a/internal/mcp/dispatch_http_routes.go +++ b/internal/mcp/dispatch_http_routes.go @@ -9,6 +9,7 @@ import ( "strings" "github.com/PerpetualSoftware/pad/internal/collections" + "github.com/PerpetualSoftware/pad/internal/models" ) // routeSpec is the declarative description of a CLI→HTTP mapping. @@ -361,13 +362,46 @@ func mapCollectionCreate(input map[string]any) (string, string, []byte, error) { } dsl, _ := input["fields"].(string) - schema, err := parseCollectionFieldsDSL(dsl) - if err != nil { - return "", "", nil, fmt.Errorf("parse --fields: %w", err) + rawSchema, hasSchema := input["schema"] + // Normalize "schema present but empty" — null and empty string — to + // absent. Without this, MCP clients sending `schema: null` (or `""`) + // while also setting `fields` would hit the mutually-exclusive guard, + // even though the CLI side treats an empty --schema value as a + // fall-through to --fields. Keeps the two transports symmetric. + if hasSchema { + switch v := rawSchema.(type) { + case nil: + hasSchema = false + case string: + if strings.TrimSpace(v) == "" { + hasSchema = false + } + } } - schemaJSON, err := json.Marshal(schema) - if err != nil { - return "", "", nil, fmt.Errorf("encode schema: %w", err) + if hasSchema && dsl != "" { + return "", "", nil, fmt.Errorf("fields and schema are mutually exclusive") + } + + var schemaJSON []byte + if hasSchema { + // Schema may arrive as a map (typed object param), as a string + // (agent passed a stringified JSON), or — defensively — as nil + // when omitted but the key was still set. + encoded, err := encodeSchemaForBody(rawSchema) + if err != nil { + return "", "", nil, err + } + schemaJSON = encoded + } else { + schema, err := parseCollectionFieldsDSL(dsl) + if err != nil { + return "", "", nil, fmt.Errorf("parse --fields: %w", err) + } + b, err := json.Marshal(schema) + if err != nil { + return "", "", nil, fmt.Errorf("encode schema: %w", err) + } + schemaJSON = b } layout, _ := input["layout"].(string) @@ -470,6 +504,44 @@ func parseCollectionFieldsDSL(dsl string) (map[string]any, error) { return out, nil } +// encodeSchemaForBody converts a `schema` MCP input value into a +// JSON-encoded CollectionSchema string suitable for the create-collection +// HTTP body's `schema` field. Accepts either a structured object (the +// typical typed-param shape an MCP client sends) or a string containing +// inline JSON (fallback for clients that can't construct typed objects). +// +// Backfills missing `label` values on each field using the same +// Title-Case-of-key heuristic as the CLI (`cmd/pad/main.go`'s +// collectionSchemaJSONFromFlags) so a schema constructed in either +// surface renders identically in the web UI. +func encodeSchemaForBody(raw any) ([]byte, error) { + if raw == nil { + return json.Marshal(models.CollectionSchema{}) + } + var blob []byte + switch t := raw.(type) { + case string: + blob = []byte(t) + default: + b, err := json.Marshal(raw) + if err != nil { + return nil, fmt.Errorf("encode schema: %w", err) + } + blob = b + } + + var schema models.CollectionSchema + if err := json.Unmarshal(blob, &schema); err != nil { + return nil, fmt.Errorf("invalid schema JSON: %w", err) + } + for i := range schema.Fields { + if schema.Fields[i].Label == "" && schema.Fields[i].Key != "" { + schema.Fields[i].Label = titleCaseLabel(schema.Fields[i].Key) + } + } + return json.Marshal(schema) +} + // titleCaseLabel converts a snake_case key into a Title Case label // the same way the CLI does ("due_date" → "Due Date"). Avoids // pulling in golang.org/x/text/cases for a one-line transformation. diff --git a/internal/mcp/dispatch_test.go b/internal/mcp/dispatch_test.go index 5862c778..ef53ab83 100644 --- a/internal/mcp/dispatch_test.go +++ b/internal/mcp/dispatch_test.go @@ -411,3 +411,70 @@ func TestExecDispatcher_NonzeroExitReturnsErrorResult(t *testing.T) { t.Errorf("expected IsError result for non-zero exit") } } + +// TestBuildCLIArgs_ObjectFlagValueIsJSONEncoded covers TASK-1335: when an +// MCP client passes a structured object as a flag value (e.g. `schema` +// for `pad collection create`), BuildCLIArgs must JSON-encode it so the +// CLI receives a parseable string rather than Go's default `map[...]` print. +func TestBuildCLIArgs_ObjectFlagValueIsJSONEncoded(t *testing.T) { + cmd := cmdhelp.Command{ + Flags: map[string]cmdhelp.Flag{ + "schema": {Type: "string"}, + }, + } + got, err := BuildCLIArgs(cmd, map[string]any{ + "schema": map[string]any{ + "fields": []any{ + map[string]any{"key": "status", "type": "select"}, + }, + }, + }, "", nil) + if err != nil { + t.Fatalf("BuildCLIArgs: %v", err) + } + // Find the --schema value. + var schemaVal string + for i := 0; i < len(got)-1; i++ { + if got[i] == "--schema" { + schemaVal = got[i+1] + break + } + } + if schemaVal == "" { + t.Fatalf("--schema flag not emitted; got %v", got) + } + // Must be valid JSON. + if !strings.HasPrefix(schemaVal, "{") { + t.Errorf("expected JSON-encoded value, got %q", schemaVal) + } + if !strings.Contains(schemaVal, `"status"`) || !strings.Contains(schemaVal, `"select"`) { + t.Errorf("encoded value does not preserve field shape: %q", schemaVal) + } +} + +// TestBuildCLIArgs_ArrayFlagValueIsJSONEncoded covers the array branch +// of the same object-encoding path — confirms a top-level array (not just +// objects) gets JSON-encoded too. +func TestBuildCLIArgs_ArrayFlagValueIsJSONEncoded(t *testing.T) { + cmd := cmdhelp.Command{ + Flags: map[string]cmdhelp.Flag{ + "raw": {Type: "string"}, + }, + } + got, err := BuildCLIArgs(cmd, map[string]any{ + "raw": []any{"a", "b", "c"}, + }, "", nil) + if err != nil { + t.Fatalf("BuildCLIArgs: %v", err) + } + var rawVal string + for i := 0; i < len(got)-1; i++ { + if got[i] == "--raw" { + rawVal = got[i+1] + break + } + } + if rawVal != `["a","b","c"]` { + t.Errorf("expected JSON array, got %q", rawVal) + } +}