diff --git a/README.md b/README.md index 432541f434..3b7ea94f71 100644 --- a/README.md +++ b/README.md @@ -636,6 +636,8 @@ Structured repository results use compact DTOs rather than raw REST objects. Rel For `get_repository_tree`, `list_gists`, `get_gist`, `create_gist`, and `update_gist`, gist results retain file names, contents, sizes, language/type information, timestamps, visibility, and compact owner profiles, while omitting API/node and Git transport URLs. Tree entries omit their derivable API URLs. Create/update gist results contain the gist ID and HTML URL. Modern JSON text and structured content serialize the same compact result. Older or unknown protocol versions retain byte-for-byte legacy text without an output schema or structured content. +List-returning tools `list_commits` and the `pull_request_read` methods `get_files`, `get_commits`, `get_reviews`, and `get_comments` expose their arrays inside a named object field (`commits`, `files`, `reviews`, `comments`) because MCP requires `structuredContent` to be a JSON object rather than a top-level array. Modern JSON text serializes the same object; older and unknown protocol versions keep the bare array text unchanged. + The issue metadata (`list_issue_types`, `list_issue_fields`), comment (`add_issue_comment`, `update_issue_comment`), dependency (`issue_dependency_read`, `issue_dependency_write`), and duplicate-detection (`find_duplicate`) tools provide typed `outputSchema` and `structuredContent` for negotiated supported protocol versions `2026-07-28` or newer. Older or unknown protocol versions retain the same text responses without these fields. Tool errors do not return structured content. Dependency and duplicate-detection tools retain their existing feature gates. For negotiated supported protocol versions `2026-07-28` or newer, the consolidated Actions tools (`actions_list`, `actions_get`, `actions_run_trigger`) return structured objects with a `method` discriminant and a method-specific typed field; JSON text contains the same compact projection. Workflow, job, and artifact projections omit API/hypermedia URLs while retaining browser links and useful identifiers, states, timestamps, and usage durations. Run usage lists billable runner environments in deterministic name order. Older and unknown-protocol text responses remain unchanged; `get_job_logs` retains its typed content, URL, or failed-job collection variants. diff --git a/docs/typed-tool-schemas.md b/docs/typed-tool-schemas.md index 2065872be0..a8407ab9b6 100644 --- a/docs/typed-tool-schemas.md +++ b/docs/typed-tool-schemas.md @@ -63,6 +63,11 @@ malformed versions also retain the legacy text result. Normal `Inventory.Registe protocol era is already known before server construction, such as a stateless request-scoped server. +The MCP schema for `structuredContent` is an object, so a typed output must not +be a top-level array. Wrap list results in a named field (for example +`ListCommitsOutput{Commits: [...]}`) and give the output schema the matching +object shape; the handler text may still be a bare array for legacy clients. + If a handler intentionally returns non-JSON text or content blocks, set `PreserveHandlerContent` in `TypedSchemaOptions`, or call `inventory.PreserveToolHandlerContent(ctx)` from the handler middleware for diff --git a/pkg/github/__toolsnaps__/list_commits.snap b/pkg/github/__toolsnaps__/list_commits.snap index ddd55bac91..7f25644d87 100644 --- a/pkg/github/__toolsnaps__/list_commits.snap +++ b/pkg/github/__toolsnaps__/list_commits.snap @@ -83,119 +83,152 @@ }, "name": "list_commits", "outputSchema": { - "items": { - "additionalProperties": false, - "properties": { - "author": { + "additionalProperties": false, + "properties": { + "commits": { + "items": { "additionalProperties": false, "properties": { - "avatar_url": { - "type": "string" - }, - "details": { + "author": { "additionalProperties": false, "properties": { - "bio": { - "type": "string" - }, - "blog": { - "type": "string" - }, - "company": { - "type": "string" - }, - "created_at": { - "description": "Account creation time in RFC3339 format.", - "type": "string" - }, - "email": { - "type": "string" - }, - "followers": { - "type": "integer" - }, - "following": { - "type": "integer" - }, - "hireable": { - "type": "boolean" - }, - "location": { + "avatar_url": { "type": "string" }, - "name": { - "type": "string" - }, - "owned_private_repos": { - "type": "integer" - }, - "private_gists": { - "type": "integer" - }, - "public_gists": { - "type": "integer" - }, - "public_repos": { - "type": "integer" - }, - "total_private_repos": { + "details": { + "additionalProperties": false, + "properties": { + "bio": { + "type": "string" + }, + "blog": { + "type": "string" + }, + "company": { + "type": "string" + }, + "created_at": { + "description": "Account creation time in RFC3339 format.", + "type": "string" + }, + "email": { + "type": "string" + }, + "followers": { + "type": "integer" + }, + "following": { + "type": "integer" + }, + "hireable": { + "type": "boolean" + }, + "location": { + "type": "string" + }, + "name": { + "type": "string" + }, + "owned_private_repos": { + "type": "integer" + }, + "private_gists": { + "type": "integer" + }, + "public_gists": { + "type": "integer" + }, + "public_repos": { + "type": "integer" + }, + "total_private_repos": { + "type": "integer" + }, + "twitter_username": { + "type": "string" + }, + "updated_at": { + "description": "Last profile update time in RFC3339 format.", + "type": "string" + } + }, + "required": [ + "public_repos", + "public_gists", + "followers", + "following", + "created_at", + "updated_at" + ], + "type": [ + "null", + "object" + ] + }, + "id": { "type": "integer" }, - "twitter_username": { + "login": { "type": "string" }, - "updated_at": { - "description": "Last profile update time in RFC3339 format.", + "profile_url": { "type": "string" } }, "required": [ - "public_repos", - "public_gists", - "followers", - "following", - "created_at", - "updated_at" + "login" ], "type": [ "null", "object" ] }, - "id": { - "type": "integer" - }, - "login": { - "type": "string" - }, - "profile_url": { - "type": "string" - } - }, - "required": [ - "login" - ], - "type": [ - "null", - "object" - ] - }, - "commit": { - "additionalProperties": false, - "properties": { - "author": { + "commit": { "additionalProperties": false, "properties": { - "date": { - "type": "string" - }, - "email": { - "type": "string" - }, - "name": { + "author": { + "additionalProperties": false, + "properties": { + "date": { + "type": "string" + }, + "email": { + "type": "string" + }, + "name": { + "type": "string" + } + }, + "type": [ + "null", + "object" + ] + }, + "committer": { + "additionalProperties": false, + "properties": { + "date": { + "type": "string" + }, + "email": { + "type": "string" + }, + "name": { + "type": "string" + } + }, + "type": [ + "null", + "object" + ] + }, + "message": { "type": "string" } }, + "required": [ + "message" + ], "type": [ "null", "object" @@ -204,145 +237,121 @@ "committer": { "additionalProperties": false, "properties": { - "date": { - "type": "string" - }, - "email": { - "type": "string" - }, - "name": { - "type": "string" - } - }, - "type": [ - "null", - "object" - ] - }, - "message": { - "type": "string" - } - }, - "required": [ - "message" - ], - "type": [ - "null", - "object" - ] - }, - "committer": { - "additionalProperties": false, - "properties": { - "avatar_url": { - "type": "string" - }, - "details": { - "additionalProperties": false, - "properties": { - "bio": { - "type": "string" - }, - "blog": { - "type": "string" - }, - "company": { - "type": "string" - }, - "created_at": { - "description": "Account creation time in RFC3339 format.", - "type": "string" - }, - "email": { - "type": "string" - }, - "followers": { - "type": "integer" - }, - "following": { - "type": "integer" - }, - "hireable": { - "type": "boolean" - }, - "location": { - "type": "string" - }, - "name": { + "avatar_url": { "type": "string" }, - "owned_private_repos": { + "details": { + "additionalProperties": false, + "properties": { + "bio": { + "type": "string" + }, + "blog": { + "type": "string" + }, + "company": { + "type": "string" + }, + "created_at": { + "description": "Account creation time in RFC3339 format.", + "type": "string" + }, + "email": { + "type": "string" + }, + "followers": { + "type": "integer" + }, + "following": { + "type": "integer" + }, + "hireable": { + "type": "boolean" + }, + "location": { + "type": "string" + }, + "name": { + "type": "string" + }, + "owned_private_repos": { + "type": "integer" + }, + "private_gists": { + "type": "integer" + }, + "public_gists": { + "type": "integer" + }, + "public_repos": { + "type": "integer" + }, + "total_private_repos": { + "type": "integer" + }, + "twitter_username": { + "type": "string" + }, + "updated_at": { + "description": "Last profile update time in RFC3339 format.", + "type": "string" + } + }, + "required": [ + "public_repos", + "public_gists", + "followers", + "following", + "created_at", + "updated_at" + ], + "type": [ + "null", + "object" + ] + }, + "id": { "type": "integer" }, - "private_gists": { - "type": "integer" - }, - "public_gists": { - "type": "integer" - }, - "public_repos": { - "type": "integer" - }, - "total_private_repos": { - "type": "integer" - }, - "twitter_username": { + "login": { "type": "string" }, - "updated_at": { - "description": "Last profile update time in RFC3339 format.", + "profile_url": { "type": "string" } }, "required": [ - "public_repos", - "public_gists", - "followers", - "following", - "created_at", - "updated_at" + "login" ], "type": [ "null", "object" ] }, - "id": { - "type": "integer" - }, - "login": { - "type": "string" + "html_url": { + "type": [ + "null", + "string" + ] }, - "profile_url": { - "type": "string" + "sha": { + "type": [ + "null", + "string" + ] } }, - "required": [ - "login" - ], - "type": [ - "null", - "object" - ] + "type": "object" }, - "html_url": { - "type": [ - "null", - "string" - ] - }, - "sha": { - "type": [ - "null", - "string" - ] - } - }, - "type": "object" + "type": [ + "null", + "array" + ] + } }, - "type": [ - "null", - "array" - ] + "required": [ + "commits" + ], + "type": "object" } } \ No newline at end of file diff --git a/pkg/github/__toolsnaps__/pull_request_read.snap b/pkg/github/__toolsnaps__/pull_request_read.snap index c99d4276e2..10e8dbe85e 100644 --- a/pkg/github/__toolsnaps__/pull_request_read.snap +++ b/pkg/github/__toolsnaps__/pull_request_read.snap @@ -653,8 +653,8 @@ "type": "object" }, { - "anyOf": [ - { + "properties": { + "files": { "items": { "additionalProperties": false, "properties": { @@ -702,8 +702,16 @@ "null", "array" ] - }, - { + } + }, + "required": [ + "files" + ], + "type": "object" + }, + { + "properties": { + "commits": { "items": { "additionalProperties": false, "properties": { @@ -746,8 +754,16 @@ "null", "array" ] - }, - { + } + }, + "required": [ + "commits" + ], + "type": "object" + }, + { + "properties": { + "reviews": { "items": { "additionalProperties": false, "properties": { @@ -890,8 +906,16 @@ "null", "array" ] - }, - { + } + }, + "required": [ + "reviews" + ], + "type": "object" + }, + { + "properties": { + "comments": { "items": { "additionalProperties": false, "properties": { @@ -994,8 +1018,11 @@ "array" ] } + }, + "required": [ + "comments" ], - "type": "array" + "type": "object" } ] } diff --git a/pkg/github/consolidated_pull_request_types.go b/pkg/github/consolidated_pull_request_types.go index 6d6abaed63..dc8f65a039 100644 --- a/pkg/github/consolidated_pull_request_types.go +++ b/pkg/github/consolidated_pull_request_types.go @@ -133,15 +133,15 @@ func (out PullRequestReadOutput) MarshalJSON() ([]byte, error) { case out.Status != nil: return json.Marshal(out.Status) case out.Files != nil: - return json.Marshal(out.Files) + return json.Marshal(map[string]any{"files": out.Files}) case out.Commits != nil: - return json.Marshal(out.Commits) + return json.Marshal(map[string]any{"commits": out.Commits}) case out.ReviewThreads != nil: return json.Marshal(out.ReviewThreads) case out.Reviews != nil: - return json.Marshal(out.Reviews) + return json.Marshal(map[string]any{"reviews": out.Reviews}) case out.Comments != nil: - return json.Marshal(out.Comments) + return json.Marshal(map[string]any{"comments": out.Comments}) case out.CheckRuns != nil: return json.Marshal(out.CheckRuns) default: @@ -168,8 +168,15 @@ func pullRequestReadOutputSchema() *jsonschema.Schema { for _, array := range arrays { array.Defs = nil } - // An empty array is valid for every list method, so the list shapes form - // one anyOf array variant instead of ambiguous oneOf branches. + // MCP requires structuredContent to be a JSON object, so each list method + // returns its array under a named key rather than as a top-level array. + wrapped := func(name string, items *jsonschema.Schema) *jsonschema.Schema { + return &jsonschema.Schema{ + Type: "object", + Properties: map[string]*jsonschema.Schema{name: items}, + Required: []string{name}, + } + } return repositoryUnionSchema( &jsonschema.Schema{Type: "null"}, pullRequestOutputSchema[MinimalPullRequest](), @@ -177,7 +184,10 @@ func pullRequestReadOutputSchema() *jsonschema.Schema { pullRequestOutputSchema[MinimalCombinedStatus](), pullRequestOutputSchema[MinimalReviewThreadsResponse](), pullRequestOutputSchema[MinimalCheckRunsResult](), - &jsonschema.Schema{Type: "array", AnyOf: arrays}, + wrapped("files", arrays[0]), + wrapped("commits", arrays[1]), + wrapped("reviews", arrays[2]), + wrapped("comments", arrays[3]), ) } diff --git a/pkg/github/repositories.go b/pkg/github/repositories.go index b884f4888d..cb3cb1652e 100644 --- a/pkg/github/repositories.go +++ b/pkg/github/repositories.go @@ -163,7 +163,13 @@ type ListCommitOutput struct { Committer *MinimalUser `json:"committer,omitempty"` } -func structuredListCommitsOutput(commits []MinimalCommit, fields []string) []ListCommitOutput { +// ListCommitsOutput envelopes the commit list because MCP requires +// structuredContent to be a JSON object, not a top-level array. +type ListCommitsOutput struct { + Commits []ListCommitOutput `json:"commits"` +} + +func structuredListCommitsOutput(commits []MinimalCommit, fields []string) ListCommitsOutput { output := make([]ListCommitOutput, 0, len(commits)) selected := func(field string) bool { return len(fields) == 0 || slices.Contains(fields, field) @@ -187,7 +193,7 @@ func structuredListCommitsOutput(commits []MinimalCommit, fields []string) []Lis } output = append(output, item) } - return output + return ListCommitsOutput{Commits: output} } // ListCommits creates a tool to get the list of commits of a branch in a GitHub @@ -233,7 +239,7 @@ func ListCommits(t translations.TranslationHelperFunc) inventory.ServerTool { ) WithPagination(schema) - return NewToolWithSchemaOptions[ListCommitsInput, []ListCommitOutput]( + return NewToolWithSchemaOptions[ListCommitsInput, ListCommitsOutput]( ToolsetMetadataRepos, mcp.Tool{ Name: "list_commits", @@ -246,12 +252,12 @@ func ListCommits(t translations.TranslationHelperFunc) inventory.ServerTool { }, scopes.PublicRead(scopes.Repo), inventory.TypedSchemaOptions{ValidationInputSchema: relaxedPaginationValidationSchema(schema)}, - func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, input ListCommitsInput) (*mcp.CallToolResult, []ListCommitOutput, error) { + func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, input ListCommitsInput) (*mcp.CallToolResult, ListCommitsOutput, error) { if input.Owner == "" { - return utils.NewToolResultError("missing required parameter: owner"), nil, nil + return utils.NewToolResultError("missing required parameter: owner"), ListCommitsOutput{}, nil } if input.Repo == "" { - return utils.NewToolResultError("missing required parameter: repo"), nil, nil + return utils.NewToolResultError("missing required parameter: repo"), ListCommitsOutput{}, nil } pagination := PaginationParams{Page: 1, PerPage: 30} if input.Page != nil { @@ -275,21 +281,21 @@ func ListCommits(t translations.TranslationHelperFunc) inventory.ServerTool { if input.Since != "" { sinceTime, err := parseISOTimestamp(input.Since) if err != nil { - return utils.NewToolResultError(fmt.Sprintf("invalid since timestamp: %s", err)), nil, nil + return utils.NewToolResultError(fmt.Sprintf("invalid since timestamp: %s", err)), ListCommitsOutput{}, nil } opts.Since = sinceTime } if input.Until != "" { untilTime, err := parseISOTimestamp(input.Until) if err != nil { - return utils.NewToolResultError(fmt.Sprintf("invalid until timestamp: %s", err)), nil, nil + return utils.NewToolResultError(fmt.Sprintf("invalid until timestamp: %s", err)), ListCommitsOutput{}, nil } opts.Until = untilTime } client, err := deps.GetClient(ctx) if err != nil { - return nil, nil, fmt.Errorf("failed to get GitHub client: %w", err) + return nil, ListCommitsOutput{}, fmt.Errorf("failed to get GitHub client: %w", err) } commits, resp, err := client.Repositories.ListCommits(ctx, input.Owner, input.Repo, opts) if err != nil { @@ -297,16 +303,16 @@ func ListCommits(t translations.TranslationHelperFunc) inventory.ServerTool { fmt.Sprintf("failed to list commits: %s", input.SHA), resp, err, - ), nil, nil + ), ListCommitsOutput{}, nil } defer func() { _ = resp.Body.Close() }() if resp.StatusCode != 200 { body, err := io.ReadAll(resp.Body) if err != nil { - return nil, nil, fmt.Errorf("failed to read response body: %w", err) + return nil, ListCommitsOutput{}, fmt.Errorf("failed to read response body: %w", err) } - return ghErrors.NewGitHubAPIStatusErrorResponse(ctx, "failed to list commits", resp, body), nil, nil + return ghErrors.NewGitHubAPIStatusErrorResponse(ctx, "failed to list commits", resp, body), ListCommitsOutput{}, nil } // Convert to minimal commits @@ -320,7 +326,7 @@ func ListCommits(t translations.TranslationHelperFunc) inventory.ServerTool { if len(input.Fields) > 0 { filteredCommits, err := filterEachField(minimalCommits, input.Fields) if err != nil { - return utils.NewToolResultErrorFromErr("failed to filter commits", err), nil, nil + return utils.NewToolResultErrorFromErr("failed to filter commits", err), ListCommitsOutput{}, nil } payload = filteredCommits filtered = true @@ -328,7 +334,7 @@ func ListCommits(t translations.TranslationHelperFunc) inventory.ServerTool { r, err := json.Marshal(payload) if err != nil { - return nil, nil, fmt.Errorf("failed to marshal response: %w", err) + return nil, ListCommitsOutput{}, fmt.Errorf("failed to marshal response: %w", err) } recordFieldsUsageFor(ctx, deps, "list_commits", minimalCommits, filtered, len(r)) diff --git a/pkg/github/typed_compatibility_test.go b/pkg/github/typed_compatibility_test.go index d1e68a6b1f..adc52f5143 100644 --- a/pkg/github/typed_compatibility_test.go +++ b/pkg/github/typed_compatibility_test.go @@ -155,7 +155,12 @@ func TestTypedCompatibilityPullRequestAfter(t *testing.T) { assert.Contains(t, getErrorResult(t, result).Text, "after") } else { require.False(t, result.IsError, mustMarshalJSON(t, result)) - assert.Equal(t, "[]", getTextResult(t, result).Text) + expected := "[]" + if protocol != "2025-11-25" { + // Modern text serializes the enveloped DTO. + expected = `{"files":[]}` + } + assert.Equal(t, expected, getTextResult(t, result).Text) } } diff --git a/pkg/github/typed_consolidated_pull_request_outputs_test.go b/pkg/github/typed_consolidated_pull_request_outputs_test.go index 60afd2f1f1..638a12be7f 100644 --- a/pkg/github/typed_consolidated_pull_request_outputs_test.go +++ b/pkg/github/typed_consolidated_pull_request_outputs_test.go @@ -218,6 +218,22 @@ type typedPullRequestCase struct { text string } +// pullRequestListOutputKey names the object field that envelopes a list +// method's array in structuredContent. Legacy text keeps the bare array. +func pullRequestListOutputKey(method any) string { + switch method { + case "get_files": + return "files" + case "get_commits": + return "commits" + case "get_reviews": + return "reviews" + case "get_comments": + return "comments" + } + return "" +} + func runTypedPullRequestCases(t *testing.T, session *mcp.ClientSession, schemas map[string]*jsonschema.Resolved, cases []typedPullRequestCase) { t.Helper() for _, tc := range cases { @@ -242,6 +258,10 @@ func runTypedPullRequestCases(t *testing.T, session *mcp.ClientSession, schemas continue } require.NotNil(t, result.StructuredContent, "%s %v", tc.tool, tc.args) + expected := tc.text + if key := pullRequestListOutputKey(tc.args["method"]); key != "" { + expected = `{"` + key + `":` + tc.text + `}` + } var value any require.NoError(t, json.Unmarshal([]byte(mustMarshalJSON(t, result.StructuredContent)), &value)) require.NoError(t, schema.Validate(value), "%s %v", tc.tool, tc.args) @@ -267,7 +287,7 @@ func runTypedPullRequestCases(t *testing.T, session *mcp.ClientSession, schemas assert.JSONEq(t, `{"id":"77"}`, mustMarshalJSON(t, value)) } default: - assert.JSONEq(t, tc.text, mustMarshalJSON(t, value)) + assert.JSONEq(t, expected, mustMarshalJSON(t, value)) } } } @@ -537,8 +557,8 @@ func TestConsolidatedPullRequestOutputSchemas(t *testing.T) { }{ { pullRequestReadOutputSchema(), - []string{`null`, `[]`, `{"diff":""}`, `{"number":1,"title":"","state":"","draft":false,"merged":false,"html_url":""}`, `{"state":"","sha":"","total_count":0,"statuses":[]}`, `{"total_count":0,"check_runs":[]}`, `{"review_threads":[],"totalCount":0,"pageInfo":{"hasNextPage":false,"hasPreviousPage":false}}`, `[{"filename":"f"}]`, `[{"id":1,"html_url":""}]`}, - []string{`{}`, `"diff"`, `{"diff":1}`, `[{"unknown":1}]`, `{"number":1}`, `{"number":1,"title":"","state":"unknown","draft":false,"merged":false,"html_url":""}`}, + []string{`null`, `{"files":[]}`, `{"commits":[]}`, `{"reviews":[]}`, `{"comments":[]}`, `{"diff":""}`, `{"number":1,"title":"","state":"","draft":false,"merged":false,"html_url":""}`, `{"state":"","sha":"","total_count":0,"statuses":[]}`, `{"total_count":0,"check_runs":[]}`, `{"review_threads":[],"totalCount":0,"pageInfo":{"hasNextPage":false,"hasPreviousPage":false}}`, `{"files":[{"filename":"f"}]}`, `{"comments":[{"id":1,"html_url":""}]}`}, + []string{`{}`, `"diff"`, `{"diff":1}`, `[{"unknown":1}]`, `[]`, `[{"filename":"f"}]`, `{"number":1}`, `{"number":1,"title":"","state":"unknown","draft":false,"merged":false,"html_url":""}`}, }, { pullRequestWriteOutputSchema(), diff --git a/pkg/github/typed_repository_commit_outputs_test.go b/pkg/github/typed_repository_commit_outputs_test.go index 59b5f207ff..324f4ddfa0 100644 --- a/pkg/github/typed_repository_commit_outputs_test.go +++ b/pkg/github/typed_repository_commit_outputs_test.go @@ -192,7 +192,12 @@ func TestTypedRepositoryCommitOutputs(t *testing.T) { require.NotNil(t, result.StructuredContent, "%s must return structured content", call.name) structuredJSON := mustMarshalJSON(t, result.StructuredContent) - assert.JSONEq(t, call.text, text) + // list_commits envelopes its array so structuredContent is an object. + expected := call.text + if call.name == "list_commits" { + expected = `{"commits":` + call.text + `}` + } + assert.JSONEq(t, expected, text) assert.JSONEq(t, text, structuredJSON, "modern text must serialize the typed DTO") var schema jsonschema.Schema