From cdeb1508b28d724c190ef9b7a7902f724126070a Mon Sep 17 00:00:00 2001 From: olaservo Date: Fri, 24 Jul 2026 20:55:12 -0700 Subject: [PATCH 1/6] feat: add output schemas for tools with polymorphic outputs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds feature-gated MCP `outputSchema` and `structuredContent`, targeting the tools whose response shape varies by their `method` argument — the ones both existing upstream attempts (#2382, #2468) explicitly set aside as "multi-method tool returning different shapes". Protocol revision 2026-07-28 (SEP-2106) is what makes these expressible. 2025-11-25 typed `outputSchema` as a closed object shape restricted to `type: "object"` at the root, and `structuredContent` as a JSON object; the draft widens both to any valid JSON Schema 2020-12 and any JSON value. Two orthogonal gates: - `output_schemas` feature flag controls rollout. - The negotiated protocol version, read per request, controls which schema shapes are legal. Object-root schemas ship to every client; non-object roots and non-object structuredContent are withheld below 2026-07-28. Unions use `anyOf`, never `oneOf`. `oneOf` requires exactly one branch to match and provably fails here: actions_run_trigger has four structurally identical branches, an empty array satisfies every array branch, and an issue_read `get` on a sub-issue also satisfies the `get_parent` branch. A test asserts no schema ever contains "oneOf". Handlers are unchanged. Rather than thread typed values out of ~20 call sites, structuredContent is mirrored from the serialized-JSON text block each handler already emits — the relationship the spec itself describes. This is byte-exact, so it cannot drift from the text content and preserves number formatting a re-marshal would lose. It applies only to tools that declare a schema, leaving the other ~120 tools byte-identical on the wire. Because the schema lives on ServerTool and is copied onto a duplicate mcp.Tool at registration, Tool.OutputSchema stays nil in the default surface and none of the 123 committed toolsnaps change. Tools covered: actions_get, actions_list, actions_run_trigger, discussion_comment_write, issue_dependency_read (single shape, not a union), issue_read, pull_request_read. Also fixes three adjacent bugs found along the way: - GetSubIssues could emit the literal `null`: go-github declares `var subIssues []*SubIssue` and leaves it nil on an empty body, which marshals to null rather than []. Every sibling method normalises; this one did not. - sub_issue_write's `method` property had no `enum`, so its values existed only in prose while dispatch accepted exactly add/remove/reprioritize. - convertJSONTextResultToCSV cleared StructuredContent, which would have made csv_output silently disable output_schemas for every list_* tool. Co-Authored-By: Claude Opus 5 --- docs/feature-flags.md | 27 + pkg/github/__toolsnaps__/sub_issue_write.snap | 5 + pkg/github/actions.go | 6 +- pkg/github/csv_output.go | 7 +- pkg/github/discussions.go | 2 +- pkg/github/feature_flags.go | 13 + pkg/github/issue_dependencies.go | 2 +- pkg/github/issues.go | 12 +- pkg/github/output_schema.go | 135 ++++ pkg/github/output_schema_test.go | 165 +++++ pkg/github/output_schemas/actions_get.json | 546 ++++++++++++++++ pkg/github/output_schemas/actions_list.json | 564 +++++++++++++++++ .../output_schemas/actions_run_trigger.json | 95 +++ .../discussion_comment_write.json | 45 ++ .../output_schemas/issue_dependency_read.json | 72 +++ pkg/github/output_schemas/issue_read.json | 467 ++++++++++++++ .../output_schemas/pull_request_read.json | 595 ++++++++++++++++++ pkg/github/output_schemas_polymorphic.go | 71 +++ pkg/github/output_schemas_polymorphic_test.go | 239 +++++++ pkg/github/pullrequests.go | 2 +- pkg/github/server.go | 5 + pkg/inventory/builder.go | 16 +- pkg/inventory/output_schema_gate.go | 117 ++++ pkg/inventory/output_schema_gate_test.go | 146 +++++ .../output_schema_registration_test.go | 145 +++++ pkg/inventory/registry.go | 7 +- pkg/inventory/server_tool.go | 50 ++ pkg/inventory/structured_mirror.go | 83 +++ pkg/inventory/structured_mirror_test.go | 116 ++++ 29 files changed, 3742 insertions(+), 13 deletions(-) create mode 100644 pkg/github/output_schema.go create mode 100644 pkg/github/output_schema_test.go create mode 100644 pkg/github/output_schemas/actions_get.json create mode 100644 pkg/github/output_schemas/actions_list.json create mode 100644 pkg/github/output_schemas/actions_run_trigger.json create mode 100644 pkg/github/output_schemas/discussion_comment_write.json create mode 100644 pkg/github/output_schemas/issue_dependency_read.json create mode 100644 pkg/github/output_schemas/issue_read.json create mode 100644 pkg/github/output_schemas/pull_request_read.json create mode 100644 pkg/github/output_schemas_polymorphic.go create mode 100644 pkg/github/output_schemas_polymorphic_test.go create mode 100644 pkg/inventory/output_schema_gate.go create mode 100644 pkg/inventory/output_schema_gate_test.go create mode 100644 pkg/inventory/output_schema_registration_test.go create mode 100644 pkg/inventory/structured_mirror.go create mode 100644 pkg/inventory/structured_mirror_test.go diff --git a/docs/feature-flags.md b/docs/feature-flags.md index c83e0c74be..e80cb69fb5 100644 --- a/docs/feature-flags.md +++ b/docs/feature-flags.md @@ -22,6 +22,33 @@ end users. Insiders-only flags are not user-toggleable. --- +## `output_schemas` + +Advertises MCP [`outputSchema`](https://modelcontextprotocol.io/specification/draft/server/tools) on tools that declare one, and returns a matching `structuredContent` alongside the existing text result. It gives clients and models a machine-readable contract for a tool's response — needed for code-execution workflows, which cannot generate typed bindings without it. + +The flag does not change any tool's inventory or input schema, so it does not appear in the generated list below. It also never changes the text content a tool returns: `structuredContent` carries the exact bytes of the existing serialized-JSON text block, which is the relationship the spec already describes ("a tool that returns structured content SHOULD also return the serialized JSON in a TextContent block"). + +Tools that currently declare a schema are the ones whose response shape varies by their `method` argument: `actions_get`, `actions_list`, `actions_run_trigger`, `discussion_comment_write`, `issue_dependency_read`, `issue_read`, and `pull_request_read`. Their schemas live in [`pkg/github/output_schemas/`](../pkg/github/output_schemas/). + +### Interaction with the negotiated protocol version + +This flag controls *rollout*. It is independent of which schema shapes are *legal* for a given client, which is decided per request from the negotiated protocol version. + +Protocol revision 2025-11-25 typed `outputSchema` as a closed object shape, restricted to `type: "object"` at the root, and typed `structuredContent` as a JSON object. [SEP-2106](https://github.com/modelcontextprotocol/modelcontextprotocol) lifted both in 2026-07-28: an output schema may now be any valid JSON Schema 2020-12, and structured content may be any JSON value. + +So a union spanning objects and arrays — which is what `issue_read` and `pull_request_read` need — is only expressible from 2026-07-28 onward. The server handles this automatically: + +| Negotiated version | Object-root schemas | Non-object-root schemas | +|--------------------|---------------------|-------------------------| +| `>= 2026-07-28` | advertised | advertised | +| older, or unknown | advertised | withheld | + +Non-object `structuredContent` is withheld from older clients on the same basis. The text content is unaffected in every case, so no client loses data — older ones simply do not gain the structured channel for those tools. + +Schemas use `anyOf`, never `oneOf`. `oneOf` requires exactly one branch to match, which fails on real payloads here: `actions_run_trigger` has four structurally identical branches, an empty array satisfies every array branch at once, and an `issue_read` `get` on a sub-issue also satisfies the `get_parent` branch. + +--- + ## Tools affected by each flag The list below is regenerated from the Go source. For each user-controllable diff --git a/pkg/github/__toolsnaps__/sub_issue_write.snap b/pkg/github/__toolsnaps__/sub_issue_write.snap index 9dc2776c32..192d0dcfc2 100644 --- a/pkg/github/__toolsnaps__/sub_issue_write.snap +++ b/pkg/github/__toolsnaps__/sub_issue_write.snap @@ -21,6 +21,11 @@ }, "method": { "description": "The action to perform on a single sub-issue\nOptions are:\n- 'add' - add a sub-issue to a parent issue in a GitHub repository.\n- 'remove' - remove a sub-issue from a parent issue in a GitHub repository.\n- 'reprioritize' - change the order of sub-issues within a parent issue in a GitHub repository. Use either 'after_id' or 'before_id' to specify the new position.\nWrites issue hierarchy. To move a sub-issue to a new parent, use `add` with `replace_parent=true`; there is no writable parent field.\n", + "enum": [ + "add", + "remove", + "reprioritize" + ], "type": "string" }, "owner": { diff --git a/pkg/github/actions.go b/pkg/github/actions.go index c16efa0f18..e25b036c70 100644 --- a/pkg/github/actions.go +++ b/pkg/github/actions.go @@ -401,7 +401,7 @@ Use this tool to list workflows in a repository, or list workflow runs, jobs, an } }, ) - return tool + return tool.WithOutputSchema(actionsListOutputSchema) } // ActionsGet returns the tool and handler for getting GitHub Actions resources. @@ -523,7 +523,7 @@ Use this tool to get details about individual workflows, workflow runs, jobs, an } }, ) - return tool + return tool.WithOutputSchema(actionsGetOutputSchema) } // ActionsRunTrigger returns the tool and handler for triggering GitHub Actions workflows. @@ -640,7 +640,7 @@ func ActionsRunTrigger(t translations.TranslationHelperFunc) inventory.ServerToo } }, ) - return tool + return tool.WithOutputSchema(actionsRunTriggerOutputSchema) } // ActionsGetJobLogs returns the tool and handler for getting workflow job logs. diff --git a/pkg/github/csv_output.go b/pkg/github/csv_output.go index 6acb8b2fdb..05e490ffee 100644 --- a/pkg/github/csv_output.go +++ b/pkg/github/csv_output.go @@ -101,7 +101,12 @@ func convertJSONTextResultToCSV(result *mcp.CallToolResult) *mcp.CallToolResult } result.Content = []mcp.Content{&mcp.TextContent{Text: csvText}} - result.StructuredContent = nil + // StructuredContent is deliberately preserved. content and + // structuredContent are independent channels: the CSV is the compact + // rendering for the model, while structuredContent remains the + // machine-readable result a client validates against the tool's + // outputSchema. Clearing it here would make csv_output silently disable + // output_schemas for every list_* tool. return result } diff --git a/pkg/github/discussions.go b/pkg/github/discussions.go index 8643acc7ef..cf6b922815 100644 --- a/pkg/github/discussions.go +++ b/pkg/github/discussions.go @@ -678,7 +678,7 @@ Options are: default: return utils.NewToolResultError("invalid method, must be one of: 'add', 'reply', 'update', 'delete', 'mark_answer', 'unmark_answer'"), nil, nil } - }) + }).WithOutputSchema(discussionCommentWriteOutputSchema) } func addDiscussionComment(ctx context.Context, client *githubv4.Client, args map[string]any) (*mcp.CallToolResult, any, error) { diff --git a/pkg/github/feature_flags.go b/pkg/github/feature_flags.go index b0652c3346..a40027425d 100644 --- a/pkg/github/feature_flags.go +++ b/pkg/github/feature_flags.go @@ -35,6 +35,18 @@ const FeatureFlagIssueDependencies = "issue_dependencies" // a redeploy. const FeatureFlagFieldsParam = "fields_param" +// FeatureFlagOutputSchemas is the feature flag name for MCP tool output +// schemas. When enabled, tools that declare one advertise `outputSchema` and +// return a matching `structuredContent` alongside the existing text result. +// It is gated so the added tools/list payload is opt-in, and so it can be +// switched off as a kill switch without a redeploy. +// +// Note this flag controls *rollout*, not protocol legality: which schema +// shapes may be advertised to a given client is decided separately, per +// request, from the negotiated protocol version. See +// inventory.OutputSchemaVersionGate. +const FeatureFlagOutputSchemas = "output_schemas" + // AllowedFeatureFlags is the allowlist of feature flags that can be enabled // by users via --features CLI flag or X-MCP-Features HTTP header. // Only flags in this list are accepted; unknown flags are silently ignored. @@ -49,6 +61,7 @@ var AllowedFeatureFlags = []string{ FeatureFlagFileBlame, FeatureFlagIssueDependencies, FeatureFlagFieldsParam, + FeatureFlagOutputSchemas, } // InsidersFeatureFlags is the list of feature flags that insiders mode enables. diff --git a/pkg/github/issue_dependencies.go b/pkg/github/issue_dependencies.go index 49533232d5..b5e7bc71f7 100644 --- a/pkg/github/issue_dependencies.go +++ b/pkg/github/issue_dependencies.go @@ -104,7 +104,7 @@ Options are: } }) st.FeatureFlagEnable = FeatureFlagIssueDependencies - return st + return st.WithOutputSchema(issueDependencyReadOutputSchema) } // GetIssueBlockedBy lists the issues that block the given issue. diff --git a/pkg/github/issues.go b/pkg/github/issues.go index 0f12804a59..191ef2cbb9 100644 --- a/pkg/github/issues.go +++ b/pkg/github/issues.go @@ -709,7 +709,7 @@ func IssueRead(t translations.TranslationHelperFunc) inventory.ServerTool { default: return utils.NewToolResultError(fmt.Sprintf("unknown method: %s", method)), nil, nil } - }) + }).WithOutputSchema(issueReadOutputSchema) } func GetIssue(ctx context.Context, client *github.Client, deps ToolDependencies, owner string, repo string, issueNumber int) (*mcp.CallToolResult, error) { @@ -931,6 +931,15 @@ func GetSubIssues(ctx context.Context, client *github.Client, deps ToolDependenc subIssues = filteredSubIssues } + // go-github declares `var subIssues []*SubIssue` and leaves it nil when the + // response body is empty or null, which json.Marshal renders as the literal + // `null` rather than `[]`. Every sibling method normalises (they build with + // make), so normalise here too: callers parsing the result expect an array, + // and null does not conform to this tool's declared output schema. + if subIssues == nil { + subIssues = []*github.SubIssue{} + } + r, err := json.Marshal(subIssues) if err != nil { return nil, fmt.Errorf("failed to marshal response: %w", err) @@ -1388,6 +1397,7 @@ func SubIssueWrite(t translations.TranslationHelperFunc) inventory.ServerTool { "- 'remove' - remove a sub-issue from a parent issue in a GitHub repository.\n" + "- 'reprioritize' - change the order of sub-issues within a parent issue in a GitHub repository. Use either 'after_id' or 'before_id' to specify the new position.\n" + "Writes issue hierarchy. To move a sub-issue to a new parent, use `add` with `replace_parent=true`; there is no writable parent field.\n", + Enum: []any{"add", "remove", "reprioritize"}, }, "owner": { Type: "string", diff --git a/pkg/github/output_schema.go b/pkg/github/output_schema.go new file mode 100644 index 0000000000..276786bb23 --- /dev/null +++ b/pkg/github/output_schema.go @@ -0,0 +1,135 @@ +package github + +import ( + "bytes" + "context" + "encoding/json" + "fmt" + + "github.com/google/jsonschema-go/jsonschema" + "github.com/modelcontextprotocol/go-sdk/mcp" + + "github.com/github/github-mcp-server/pkg/inventory" + "github.com/github/github-mcp-server/pkg/utils" +) + +// MustOutputSchema infers an output schema for T, panicking during package +// initialization if inference fails. +// +// Unlike input schemas, an output schema root need not be `{"type":"object"}`: +// from protocol version 2026-07-28 (SEP-2106) it may be any valid JSON Schema +// 2020-12, including a bare array or a bare anyOf. Schemas whose root is not +// an object are stripped per-request for older clients — see +// inventory.OutputSchemaVersionGate — so inferring one here is safe. +func MustOutputSchema[T any]() *jsonschema.Schema { + schema, err := jsonschema.For[T](nil) + if err != nil { + var zero T + panic(fmt.Sprintf("failed to infer output schema for %T: %v", zero, err)) + } + return schema +} + +// AnyOfSchema builds a union output schema over the given branches. +// +// It deliberately emits `anyOf` and never `oneOf`. `oneOf` requires that +// EXACTLY ONE branch match, which is wrong for essentially every tool in this +// package whose output shape varies by method: +// +// - Branches are frequently structurally identical. All four of +// actions_run_trigger's non-run_workflow methods return the same +// {message, run_id, status, status_code} map, so a oneOf over them matches +// four branches and therefore always fails. +// - Empty collections are ambiguous. issue_read method=get_comments on an +// issue with no comments returns [], which vacuously satisfies every array +// branch. +// - Optional fields overlap. issue_read method=get on a sub-issue populates +// `parent`, which also satisfies the get_parent branch. +// +// anyOf ("at least one") accepts all three while still rejecting values that +// match no branch, which is the useful half of the validation. +func AnyOfSchema(branches ...*jsonschema.Schema) *jsonschema.Schema { + return &jsonschema.Schema{AnyOf: branches} +} + +// MustRawOutputSchema wraps a hand-authored JSON Schema document, panicking +// during package initialization if it does not parse or does not resolve. +// +// Hand-authored schemas are kept as json.RawMessage rather than +// *jsonschema.Schema so they round-trip byte-for-byte to the client and +// produce deterministic toolsnaps output, independent of jsonschema-go's +// struct field coverage and marshaling order. +// +// Resolution is checked here (not just parsing) so a dangling $ref fails the +// build rather than the request. +func MustRawOutputSchema(raw string) json.RawMessage { + var parsed jsonschema.Schema + if err := json.Unmarshal([]byte(raw), &parsed); err != nil { + panic(fmt.Sprintf("invalid output schema JSON: %v\nschema: %s", err, raw)) + } + if _, err := parsed.Resolve(nil); err != nil { + panic(fmt.Sprintf("output schema does not resolve: %v\nschema: %s", err, raw)) + } + return json.RawMessage(raw) +} + +// outputSchemasEnabled reports whether the output_schemas feature is on for +// this request. This is the rollout gate only; see canSendStructuredContent +// for the protocol-legality gate. +func outputSchemasEnabled(ctx context.Context, deps ToolDependencies) bool { + return deps.IsFeatureEnabled(ctx, FeatureFlagOutputSchemas) +} + +// isJSONObject reports whether the marshaled form of v is a JSON object. +func isJSONObject(marshaled []byte) bool { + return bytes.HasPrefix(bytes.TrimLeft(marshaled, " \t\r\n"), []byte("{")) +} + +// canSendStructuredContent reports whether a structuredContent value of the +// given marshaled shape may legally be sent to this client. +// +// Under 2025-11-25 and earlier, structuredContent is typed +// `{ [key: string]: unknown }` — a JSON object. 2026-07-28 widened it to +// `unknown`, explicitly "any JSON value (object, array, string, number, +// boolean, or null)". So an object is always safe; anything else requires the +// newer protocol. req may be nil in tests, in which case only objects are sent. +func canSendStructuredContent(req *mcp.CallToolRequest, marshaled []byte) bool { + if isJSONObject(marshaled) { + return true + } + if req == nil { + return false + } + return req.ProtocolVersion() >= inventory.ProtocolVersionNonObjectOutputSchemas +} + +// structuredTextResult builds a tool result whose text content is the +// serialized textValue — byte-identical to what the tool returned before +// output schemas existed — and which additionally carries structured as +// structuredContent when both gates allow it. +// +// The text block is always populated regardless of the gates. The spec calls +// for this independently: "For backwards compatibility, a tool that returns +// structured content SHOULD also return the serialized JSON in a TextContent +// block." +func structuredTextResult(ctx context.Context, deps ToolDependencies, req *mcp.CallToolRequest, textValue, structured any) (*mcp.CallToolResult, error) { + data, err := json.Marshal(textValue) + if err != nil { + return nil, fmt.Errorf("failed to marshal response: %w", err) + } + + result := utils.NewToolResultText(string(data)) + if structured == nil || !outputSchemasEnabled(ctx, deps) { + return result, nil + } + + structuredJSON, err := json.Marshal(structured) + if err != nil { + return nil, fmt.Errorf("failed to marshal structured content: %w", err) + } + if !canSendStructuredContent(req, structuredJSON) { + return result, nil + } + result.StructuredContent = structured + return result, nil +} diff --git a/pkg/github/output_schema_test.go b/pkg/github/output_schema_test.go new file mode 100644 index 0000000000..ba876892f9 --- /dev/null +++ b/pkg/github/output_schema_test.go @@ -0,0 +1,165 @@ +package github + +import ( + "context" + "encoding/json" + "testing" + + "github.com/google/jsonschema-go/jsonschema" + "github.com/modelcontextprotocol/go-sdk/mcp" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/github/github-mcp-server/pkg/inventory" +) + +type outputSchemaTestPayload struct { + Message string `json:"message"` +} + +func TestMustOutputSchemaInfersObject(t *testing.T) { + schema := MustOutputSchema[outputSchemaTestPayload]() + require.NotNil(t, schema) + assert.Equal(t, "object", schema.Type) + assert.Contains(t, schema.Properties, "message") +} + +// The 2025-11-25 schema typed outputSchema as a closed object shape; 2026-07-28 +// (SEP-2106) allows any valid 2020-12 schema. Non-object roots must therefore +// be inferable rather than a panic — this is the constraint being lifted. +func TestMustOutputSchemaAllowsNonObjectRoots(t *testing.T) { + assert.NotPanics(t, func() { + schema := MustOutputSchema[[]outputSchemaTestPayload]() + // Inference emits the nullable form {"type":["null","array"]} because a + // Go slice can be nil, so the single-valued Type field is empty and the + // union lands in Types. + assert.Empty(t, schema.Type) + assert.ElementsMatch(t, []string{"null", "array"}, schema.Types) + }, "a bare array output schema is legal from 2026-07-28") + + assert.NotPanics(t, func() { + schema := MustOutputSchema[string]() + assert.Equal(t, "string", schema.Type) + }, "a bare string output schema is legal from 2026-07-28") +} + +// Both of the above roots must be gated for pre-2026-07-28 clients: a type +// array is not the {"type":"object"} shape those revisions required. +func TestInferredNonObjectRootsAreGated(t *testing.T) { + assert.False(t, inventory.HasObjectRootOutputSchema(MustOutputSchema[[]outputSchemaTestPayload]())) + assert.False(t, inventory.HasObjectRootOutputSchema(MustOutputSchema[string]())) + assert.True(t, inventory.HasObjectRootOutputSchema(MustOutputSchema[outputSchemaTestPayload]())) +} + +func TestAnyOfSchemaNeverEmitsOneOf(t *testing.T) { + schema := AnyOfSchema( + MustOutputSchema[outputSchemaTestPayload](), + MustOutputSchema[[]outputSchemaTestPayload](), + ) + require.Len(t, schema.AnyOf, 2) + assert.Nil(t, schema.OneOf, "oneOf requires exactly one branch to match and is wrong for these unions") + + raw, err := json.Marshal(schema) + require.NoError(t, err) + assert.Contains(t, string(raw), `"anyOf"`) + assert.NotContains(t, string(raw), `"oneOf"`) +} + +// Guards the reason AnyOfSchema exists: an empty array satisfies every array +// branch, so oneOf ("exactly one") fails where anyOf ("at least one") succeeds. +func TestAnyOfAcceptsAmbiguousPayloadsButStillRejectsGarbage(t *testing.T) { + schema := MustRawOutputSchema(`{"anyOf":[ + {"type":"array","items":{"type":"object","properties":{"id":{"type":"integer"}}}}, + {"type":"array","items":{"type":"object","properties":{"name":{"type":"string"}}}} + ]}`) + var parsed = mustResolveSchema(t, schema) + + assert.NoError(t, parsed.Validate([]any{}), "empty array matches both branches; anyOf must accept it") + assert.Error(t, parsed.Validate(map[string]any{"junk": 1}), "anyOf must still reject values matching no branch") +} + +func TestMustRawOutputSchemaRejectsBadInput(t *testing.T) { + assert.Panics(t, func() { MustRawOutputSchema(`{"type":`) }, "malformed JSON must fail the build") + assert.Panics(t, func() { + MustRawOutputSchema(`{"$ref":"#/$defs/missing"}`) + }, "a dangling $ref must fail the build, not the request") + assert.NotPanics(t, func() { + MustRawOutputSchema(`{"$ref":"#/$defs/ok","$defs":{"ok":{"type":"object"}}}`) + }) +} + +func depsWithOutputSchemas(enabled bool) BaseDeps { + return BaseDeps{featureChecker: func(_ context.Context, flag string) (bool, error) { + return enabled && flag == FeatureFlagOutputSchemas, nil + }} +} + +func callToolReqAtVersion(version string) *mcp.CallToolRequest { + params := &mcp.CallToolParamsRaw{} + if version != "" { + params.Meta = mcp.Meta{mcp.MetaKeyProtocolVersion: version} + } + return &mcp.CallToolRequest{Params: params} +} + +func TestStructuredTextResultGating(t *testing.T) { + objectValue := outputSchemaTestPayload{Message: "structured"} + arrayValue := []outputSchemaTestPayload{{Message: "a"}} + + tests := []struct { + name string + flagEnabled bool + version string + structured any + wantStructured bool + }{ + {"flag off means no structured content", false, "2026-07-28", objectValue, false}, + {"object ships on 2025-11-25", true, "2025-11-25", objectValue, true}, + {"object ships on 2026-07-28", true, "2026-07-28", objectValue, true}, + {"array is withheld from 2025-11-25", true, "2025-11-25", arrayValue, false}, + {"array ships on 2026-07-28", true, "2026-07-28", arrayValue, true}, + {"array is withheld when version is unknown", true, "", arrayValue, false}, + {"nil structured is always omitted", true, "2026-07-28", nil, false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + textValue := map[string]string{"message": "text"} + result, err := structuredTextResult( + context.Background(), depsWithOutputSchemas(tt.flagEnabled), + callToolReqAtVersion(tt.version), textValue, tt.structured, + ) + require.NoError(t, err) + require.NotNil(t, result) + + // The text block is populated unconditionally and is byte-identical + // to the pre-output-schema behaviour. + textContent := getTextResult(t, result) + var gotText map[string]string + require.NoError(t, json.Unmarshal([]byte(textContent.Text), &gotText)) + assert.Equal(t, textValue, gotText) + + if tt.wantStructured { + assert.Equal(t, tt.structured, result.StructuredContent) + } else { + assert.Nil(t, result.StructuredContent) + } + }) + } +} + +func TestStructuredTextResultMarshalError(t *testing.T) { + _, err := structuredTextResult(context.Background(), BaseDeps{}, nil, + map[string]any{"invalid": func() {}}, nil) + require.Error(t, err) + assert.Contains(t, err.Error(), "failed to marshal response") +} + +func mustResolveSchema(t *testing.T, raw json.RawMessage) *jsonschema.Resolved { + t.Helper() + var s jsonschema.Schema + require.NoError(t, json.Unmarshal(raw, &s)) + resolved, err := s.Resolve(nil) + require.NoError(t, err) + return resolved +} diff --git a/pkg/github/output_schemas/actions_get.json b/pkg/github/output_schemas/actions_get.json new file mode 100644 index 0000000000..cd2e9fc6f0 --- /dev/null +++ b/pkg/github/output_schemas/actions_get.json @@ -0,0 +1,546 @@ +{ + "$schema": "https://json-schema.org/draft/2020-12/schema", + "type": "object", + "description": "Result of actions_get. The concrete shape depends on the `method` argument: each branch below corresponds to one method.", + "anyOf": [ + { + "title": "get_workflow", + "description": "A single GitHub Actions workflow (GET /repos/{owner}/{repo}/actions/workflows/{workflow_id}). All fields are omitted when absent from the API response.", + "type": "object", + "properties": { + "id": { + "type": "integer", + "description": "Workflow ID." + }, + "node_id": { + "type": "string" + }, + "name": { + "type": "string", + "description": "Workflow name." + }, + "path": { + "type": "string", + "description": "Path of the workflow file in the repository, e.g. .github/workflows/ci.yml." + }, + "state": { + "type": "string", + "description": "Workflow state, e.g. active, disabled_manually." + }, + "created_at": { + "$ref": "#/$defs/timestamp" + }, + "updated_at": { + "$ref": "#/$defs/timestamp" + }, + "url": { + "type": "string" + }, + "html_url": { + "type": "string" + }, + "badge_url": { + "type": "string" + } + } + }, + { + "title": "get_workflow_run", + "description": "A single workflow run (GET /repos/{owner}/{repo}/actions/runs/{run_id}).", + "type": "object", + "properties": { + "id": { + "type": "integer", + "description": "Workflow run ID." + }, + "name": { + "type": "string" + }, + "node_id": { + "type": "string" + }, + "head_branch": { + "type": "string" + }, + "head_sha": { + "type": "string" + }, + "path": { + "type": "string" + }, + "run_number": { + "type": "integer" + }, + "run_attempt": { + "type": "integer" + }, + "event": { + "type": "string", + "description": "Event that triggered the run, e.g. push, pull_request, workflow_dispatch." + }, + "display_title": { + "type": "string" + }, + "status": { + "type": "string", + "description": "e.g. queued, in_progress, completed." + }, + "conclusion": { + "type": "string", + "description": "e.g. success, failure, cancelled, skipped. Omitted while the run is not complete." + }, + "workflow_id": { + "type": "integer" + }, + "check_suite_id": { + "type": "integer" + }, + "check_suite_node_id": { + "type": "string" + }, + "url": { + "type": "string" + }, + "html_url": { + "type": "string" + }, + "pull_requests": { + "type": "array", + "items": { + "type": [ + "object", + "null" + ], + "description": "Pull request associated with the run." + } + }, + "created_at": { + "$ref": "#/$defs/timestamp" + }, + "updated_at": { + "$ref": "#/$defs/timestamp" + }, + "run_started_at": { + "$ref": "#/$defs/timestamp" + }, + "jobs_url": { + "type": "string" + }, + "logs_url": { + "type": "string" + }, + "check_suite_url": { + "type": "string" + }, + "artifacts_url": { + "type": "string" + }, + "cancel_url": { + "type": "string" + }, + "rerun_url": { + "type": "string" + }, + "previous_attempt_url": { + "type": "string" + }, + "head_commit": { + "$ref": "#/$defs/headCommit" + }, + "workflow_url": { + "type": "string" + }, + "repository": { + "$ref": "#/$defs/repository" + }, + "head_repository": { + "$ref": "#/$defs/repository" + }, + "actor": { + "$ref": "#/$defs/user" + }, + "triggering_actor": { + "$ref": "#/$defs/user" + }, + "referenced_workflows": { + "type": "array", + "items": { + "$ref": "#/$defs/referencedWorkflow" + } + } + } + }, + { + "title": "get_workflow_job", + "description": "A single workflow job (GET /repos/{owner}/{repo}/actions/jobs/{job_id}).", + "type": "object", + "properties": { + "id": { + "type": "integer", + "description": "Job ID." + }, + "run_id": { + "type": "integer" + }, + "run_url": { + "type": "string" + }, + "node_id": { + "type": "string" + }, + "head_branch": { + "type": "string" + }, + "head_sha": { + "type": "string" + }, + "url": { + "type": "string" + }, + "html_url": { + "type": "string" + }, + "status": { + "type": "string", + "description": "e.g. queued, in_progress, completed." + }, + "conclusion": { + "type": "string", + "description": "e.g. success, failure, cancelled, skipped." + }, + "created_at": { + "$ref": "#/$defs/timestamp" + }, + "started_at": { + "$ref": "#/$defs/timestamp" + }, + "completed_at": { + "$ref": "#/$defs/timestamp" + }, + "name": { + "type": "string" + }, + "steps": { + "type": "array", + "items": { + "$ref": "#/$defs/taskStep" + } + }, + "check_run_url": { + "type": "string" + }, + "labels": { + "type": "array", + "items": { + "type": "string" + }, + "description": "Runner labels from the runs-on key of the workflow." + }, + "runner_id": { + "type": "integer" + }, + "runner_name": { + "type": "string" + }, + "runner_group_id": { + "type": "integer" + }, + "runner_group_name": { + "type": "string" + }, + "run_attempt": { + "type": "integer" + }, + "workflow_name": { + "type": "string" + } + } + }, + { + "title": "download_workflow_run_artifact", + "description": "Temporary download location for a workflow run artifact. Built inline by the server, not returned verbatim by the GitHub API.", + "type": "object", + "properties": { + "download_url": { + "type": "string", + "description": "Short-lived URL that serves the artifact as a ZIP archive." + }, + "message": { + "type": "string" + }, + "note": { + "type": "string" + }, + "artifact_id": { + "type": "integer", + "description": "The artifact ID that was requested." + } + }, + "required": [ + "download_url", + "message", + "note", + "artifact_id" + ] + }, + { + "title": "get_workflow_run_logs_url", + "description": "Temporary download location for the complete logs of a workflow run. Built inline by the server, not returned verbatim by the GitHub API.", + "type": "object", + "properties": { + "logs_url": { + "type": "string", + "description": "Short-lived URL that serves all run logs as a ZIP archive." + }, + "message": { + "type": "string" + }, + "note": { + "type": "string" + }, + "warning": { + "type": "string" + }, + "optimization_tip": { + "type": "string" + } + }, + "required": [ + "logs_url", + "message", + "note", + "warning", + "optimization_tip" + ] + }, + { + "title": "get_workflow_run_usage", + "description": "Billable time for a workflow run (GET /repos/{owner}/{repo}/actions/runs/{run_id}/timing). Both fields are omitted when the API reports no usage, so an empty object is a valid response.", + "type": "object", + "properties": { + "billable": { + "type": [ + "object", + "null" + ], + "description": "Billable time keyed by runner environment, e.g. UBUNTU, MACOS, WINDOWS.", + "additionalProperties": { + "$ref": "#/$defs/workflowRunBill" + } + }, + "run_duration_ms": { + "type": "integer" + } + } + } + ], + "$defs": { + "timestamp": { + "type": "string", + "description": "RFC 3339 timestamp.", + "format": "date-time" + }, + "user": { + "type": [ + "object", + "null" + ], + "description": "GitHub user. Additional GitHub user fields may be present.", + "properties": { + "login": { + "type": "string" + }, + "id": { + "type": "integer" + }, + "node_id": { + "type": "string" + }, + "avatar_url": { + "type": "string" + }, + "html_url": { + "type": "string" + }, + "url": { + "type": "string" + }, + "type": { + "type": "string" + } + } + }, + "repository": { + "type": [ + "object", + "null" + ], + "description": "GitHub repository. Additional GitHub repository fields may be present.", + "properties": { + "id": { + "type": "integer" + }, + "node_id": { + "type": "string" + }, + "name": { + "type": "string" + }, + "full_name": { + "type": "string" + }, + "private": { + "type": "boolean" + }, + "url": { + "type": "string" + }, + "html_url": { + "type": "string" + }, + "owner": { + "$ref": "#/$defs/user" + } + } + }, + "headCommit": { + "type": [ + "object", + "null" + ], + "description": "Head commit of the workflow run.", + "properties": { + "id": { + "type": "string" + }, + "tree_id": { + "type": "string" + }, + "sha": { + "type": "string" + }, + "message": { + "type": "string" + }, + "url": { + "type": "string" + }, + "distinct": { + "type": "boolean" + }, + "timestamp": { + "$ref": "#/$defs/timestamp" + }, + "author": { + "type": [ + "object", + "null" + ] + }, + "committer": { + "type": [ + "object", + "null" + ] + }, + "added": { + "type": "array", + "items": { + "type": "string" + }, + "description": "Only populated by webhook events; not returned by the workflow runs API." + }, + "removed": { + "type": "array", + "items": { + "type": "string" + }, + "description": "Only populated by webhook events; not returned by the workflow runs API." + }, + "modified": { + "type": "array", + "items": { + "type": "string" + }, + "description": "Only populated by webhook events; not returned by the workflow runs API." + } + } + }, + "referencedWorkflow": { + "type": [ + "object", + "null" + ], + "properties": { + "path": { + "type": "string" + }, + "sha": { + "type": "string" + }, + "ref": { + "type": "string" + } + } + }, + "taskStep": { + "type": [ + "object", + "null" + ], + "description": "One step of a workflow job.", + "properties": { + "name": { + "type": "string" + }, + "status": { + "type": "string" + }, + "conclusion": { + "type": "string" + }, + "number": { + "type": "integer" + }, + "started_at": { + "$ref": "#/$defs/timestamp" + }, + "completed_at": { + "$ref": "#/$defs/timestamp" + } + } + }, + "workflowRunBill": { + "type": [ + "object", + "null" + ], + "description": "Billable time for one runner environment.", + "properties": { + "total_ms": { + "type": "integer" + }, + "jobs": { + "type": "integer" + }, + "job_runs": { + "type": "array", + "items": { + "type": [ + "object", + "null" + ], + "properties": { + "job_id": { + "type": "integer" + }, + "duration_ms": { + "type": "integer" + } + } + } + } + } + } + } +} diff --git a/pkg/github/output_schemas/actions_list.json b/pkg/github/output_schemas/actions_list.json new file mode 100644 index 0000000000..dbd957f934 --- /dev/null +++ b/pkg/github/output_schemas/actions_list.json @@ -0,0 +1,564 @@ +{ + "$schema": "https://json-schema.org/draft/2020-12/schema", + "type": "object", + "description": "Output of the actions_list tool. The shape depends on the `method` argument: `list_workflows`, `list_workflow_runs` and `list_workflow_run_artifacts` return the GitHub API list envelope directly, while `list_workflow_jobs` nests the envelope under a `jobs` key. Every field of every envelope is optional: go-github tags them `omitempty`, so an empty collection is reported as {\"total_count\":0} with the item array absent rather than as an empty array. Each branch forbids the other branches' discriminator keys so that a payload is checked against the branch it actually belongs to.", + "anyOf": [ + { + "title": "list_workflows", + "description": "method=list_workflows. Serialized github.Workflows.", + "type": "object", + "properties": { + "total_count": { + "type": "integer", + "description": "Total number of workflows in the repository." + }, + "workflows": { + "type": "array", + "items": { + "$ref": "#/$defs/workflow" + } + }, + "workflow_runs": false, + "jobs": false, + "artifacts": false + } + }, + { + "title": "list_workflow_runs", + "description": "method=list_workflow_runs. Serialized github.WorkflowRuns.", + "type": "object", + "properties": { + "total_count": { + "type": "integer", + "description": "Total number of workflow runs matching the query." + }, + "workflow_runs": { + "type": "array", + "items": { + "$ref": "#/$defs/workflowRun" + } + }, + "workflows": false, + "jobs": false, + "artifacts": false + } + }, + { + "title": "list_workflow_jobs", + "description": "method=list_workflow_jobs. The github.Jobs envelope wrapped in an outer object under the `jobs` key, so the payload is double-nested: {\"jobs\":{\"total_count\":N,\"jobs\":[...]}}. `jobs` is null if the API returned an empty body.", + "type": "object", + "required": [ + "jobs" + ], + "properties": { + "jobs": { + "type": [ + "object", + "null" + ], + "properties": { + "total_count": { + "type": "integer", + "description": "Total number of jobs in the workflow run." + }, + "jobs": { + "type": "array", + "items": { + "$ref": "#/$defs/workflowJob" + } + } + } + }, + "workflows": false, + "workflow_runs": false, + "artifacts": false + } + }, + { + "title": "list_workflow_run_artifacts", + "description": "method=list_workflow_run_artifacts. Serialized github.ArtifactList.", + "type": "object", + "properties": { + "total_count": { + "type": "integer", + "description": "Total number of artifacts for the workflow run." + }, + "artifacts": { + "type": "array", + "items": { + "$ref": "#/$defs/artifact" + } + }, + "workflows": false, + "workflow_runs": false, + "jobs": false + } + } + ], + "$defs": { + "workflow": { + "type": "object", + "description": "A repository workflow definition (github.Workflow).", + "properties": { + "id": { + "type": "integer" + }, + "node_id": { + "type": "string" + }, + "name": { + "type": "string" + }, + "path": { + "type": "string", + "description": "Path to the workflow file, e.g. .github/workflows/ci.yml" + }, + "state": { + "type": "string", + "description": "e.g. active, disabled_manually, disabled_inactivity" + }, + "created_at": { + "type": "string", + "format": "date-time" + }, + "updated_at": { + "type": "string", + "format": "date-time" + }, + "url": { + "type": "string" + }, + "html_url": { + "type": "string" + }, + "badge_url": { + "type": "string" + } + } + }, + "workflowRun": { + "type": "object", + "description": "A single workflow run (github.WorkflowRun).", + "properties": { + "id": { + "type": "integer" + }, + "name": { + "type": "string" + }, + "node_id": { + "type": "string" + }, + "head_branch": { + "type": "string" + }, + "head_sha": { + "type": "string" + }, + "path": { + "type": "string" + }, + "run_number": { + "type": "integer" + }, + "run_attempt": { + "type": "integer" + }, + "event": { + "type": "string" + }, + "display_title": { + "type": "string" + }, + "status": { + "type": "string", + "description": "e.g. queued, in_progress, completed" + }, + "conclusion": { + "type": "string", + "description": "e.g. success, failure, cancelled, skipped; omitted while the run is not complete" + }, + "workflow_id": { + "type": "integer" + }, + "check_suite_id": { + "type": "integer" + }, + "check_suite_node_id": { + "type": "string" + }, + "url": { + "type": "string" + }, + "html_url": { + "type": "string" + }, + "pull_requests": { + "type": "array", + "items": { + "type": "object", + "description": "Pull request associated with the run (github.PullRequest)." + } + }, + "created_at": { + "type": "string", + "format": "date-time" + }, + "updated_at": { + "type": "string", + "format": "date-time" + }, + "run_started_at": { + "type": "string", + "format": "date-time" + }, + "jobs_url": { + "type": "string" + }, + "logs_url": { + "type": "string" + }, + "check_suite_url": { + "type": "string" + }, + "artifacts_url": { + "type": "string" + }, + "cancel_url": { + "type": "string" + }, + "rerun_url": { + "type": "string" + }, + "previous_attempt_url": { + "type": "string" + }, + "head_commit": { + "$ref": "#/$defs/headCommit" + }, + "workflow_url": { + "type": "string" + }, + "repository": { + "$ref": "#/$defs/repository" + }, + "head_repository": { + "$ref": "#/$defs/repository" + }, + "actor": { + "$ref": "#/$defs/user" + }, + "triggering_actor": { + "$ref": "#/$defs/user" + }, + "referenced_workflows": { + "type": "array", + "items": { + "$ref": "#/$defs/referencedWorkflow" + } + } + } + }, + "workflowJob": { + "type": "object", + "description": "A single job within a workflow run (github.WorkflowJob).", + "properties": { + "id": { + "type": "integer" + }, + "run_id": { + "type": "integer" + }, + "run_url": { + "type": "string" + }, + "node_id": { + "type": "string" + }, + "head_branch": { + "type": "string" + }, + "head_sha": { + "type": "string" + }, + "url": { + "type": "string" + }, + "html_url": { + "type": "string" + }, + "status": { + "type": "string", + "description": "e.g. queued, in_progress, completed" + }, + "conclusion": { + "type": "string", + "description": "e.g. success, failure, cancelled, skipped; omitted while the job is not complete" + }, + "created_at": { + "type": "string", + "format": "date-time" + }, + "started_at": { + "type": "string", + "format": "date-time" + }, + "completed_at": { + "type": "string", + "format": "date-time" + }, + "name": { + "type": "string" + }, + "steps": { + "type": "array", + "items": { + "$ref": "#/$defs/taskStep" + } + }, + "check_run_url": { + "type": "string" + }, + "labels": { + "type": "array", + "description": "Runner labels from the `runs-on:` key of the workflow.", + "items": { + "type": "string" + } + }, + "runner_id": { + "type": "integer" + }, + "runner_name": { + "type": "string" + }, + "runner_group_id": { + "type": "integer" + }, + "runner_group_name": { + "type": "string" + }, + "run_attempt": { + "type": "integer" + }, + "workflow_name": { + "type": "string" + } + } + }, + "taskStep": { + "type": "object", + "description": "A step within a workflow job (github.TaskStep).", + "properties": { + "name": { + "type": "string" + }, + "status": { + "type": "string" + }, + "conclusion": { + "type": "string" + }, + "number": { + "type": "integer" + }, + "started_at": { + "type": "string", + "format": "date-time" + }, + "completed_at": { + "type": "string", + "format": "date-time" + } + } + }, + "artifact": { + "type": "object", + "description": "An artifact produced by a workflow run (github.Artifact).", + "properties": { + "id": { + "type": "integer" + }, + "node_id": { + "type": "string" + }, + "name": { + "type": "string" + }, + "size_in_bytes": { + "type": "integer" + }, + "url": { + "type": "string" + }, + "archive_download_url": { + "type": "string" + }, + "expired": { + "type": "boolean" + }, + "created_at": { + "type": "string", + "format": "date-time" + }, + "updated_at": { + "type": "string", + "format": "date-time" + }, + "expires_at": { + "type": "string", + "format": "date-time" + }, + "digest": { + "type": "string", + "description": "SHA256 digest; only present for artifacts uploaded with upload-artifact v4 or newer." + }, + "workflow_run": { + "$ref": "#/$defs/artifactWorkflowRun" + } + } + }, + "artifactWorkflowRun": { + "type": "object", + "description": "The workflow run an artifact belongs to (github.ArtifactWorkflowRun).", + "properties": { + "id": { + "type": "integer" + }, + "repository_id": { + "type": "integer" + }, + "head_repository_id": { + "type": "integer" + }, + "head_branch": { + "type": "string" + }, + "head_sha": { + "type": "string" + } + } + }, + "referencedWorkflow": { + "type": "object", + "description": "A reusable workflow referenced by a run (github.ReferencedWorkflow).", + "properties": { + "path": { + "type": "string" + }, + "sha": { + "type": "string" + }, + "ref": { + "type": "string" + } + } + }, + "headCommit": { + "type": "object", + "description": "The head commit of a workflow run (github.HeadCommit).", + "properties": { + "id": { + "type": "string" + }, + "sha": { + "type": "string" + }, + "message": { + "type": "string" + }, + "tree_id": { + "type": "string" + }, + "timestamp": { + "type": "string", + "format": "date-time" + }, + "url": { + "type": "string" + }, + "distinct": { + "type": "boolean" + }, + "author": { + "type": "object" + }, + "committer": { + "type": "object" + }, + "added": { + "type": "array", + "items": { + "type": "string" + } + }, + "removed": { + "type": "array", + "items": { + "type": "string" + } + }, + "modified": { + "type": "array", + "items": { + "type": "string" + } + } + } + }, + "repository": { + "type": "object", + "description": "Repository the run belongs to (full github.Repository payload; only the most useful fields are described).", + "properties": { + "id": { + "type": "integer" + }, + "node_id": { + "type": "string" + }, + "name": { + "type": "string" + }, + "full_name": { + "type": "string" + }, + "private": { + "type": "boolean" + }, + "html_url": { + "type": "string" + }, + "url": { + "type": "string" + }, + "owner": { + "$ref": "#/$defs/user" + } + } + }, + "user": { + "type": "object", + "description": "A GitHub account (full github.User payload; only the most useful fields are described).", + "properties": { + "login": { + "type": "string" + }, + "id": { + "type": "integer" + }, + "node_id": { + "type": "string" + }, + "type": { + "type": "string" + }, + "avatar_url": { + "type": "string" + }, + "html_url": { + "type": "string" + } + } + } + } +} diff --git a/pkg/github/output_schemas/actions_run_trigger.json b/pkg/github/output_schemas/actions_run_trigger.json new file mode 100644 index 0000000000..ec8d69ae9b --- /dev/null +++ b/pkg/github/output_schemas/actions_run_trigger.json @@ -0,0 +1,95 @@ +{ + "$schema": "https://json-schema.org/draft/2020-12/schema", + "type": "object", + "description": "Result of a GitHub Actions run-trigger operation. Every method returns a JSON object whose shape depends on the method.", + "$defs": { + "message": { + "type": "string", + "description": "Human-readable confirmation of the operation that was performed." + }, + "httpStatus": { + "type": "string", + "description": "HTTP status line returned by the GitHub API, e.g. \"204 No Content\" or \"202 Accepted\"." + }, + "httpStatusCode": { + "type": "integer", + "description": "HTTP status code returned by the GitHub API, e.g. 204 or 202." + } + }, + "anyOf": [ + { + "title": "run_workflow", + "type": "object", + "description": "Returned by method=run_workflow: a workflow_dispatch event was created.", + "properties": { + "message": { + "$ref": "#/$defs/message" + }, + "workflow_type": { + "type": "string", + "description": "How the workflow was addressed: \"workflow_id\" when the workflow_id argument parsed as a numeric ID, \"workflow_file\" when it was treated as a workflow file name.", + "enum": [ + "workflow_id", + "workflow_file" + ] + }, + "workflow_id": { + "type": "string", + "description": "The workflow ID or workflow file name exactly as supplied by the caller." + }, + "ref": { + "type": "string", + "description": "The git reference (branch or tag) the workflow was dispatched against." + }, + "inputs": { + "type": [ + "object", + "null" + ], + "description": "The workflow inputs supplied by the caller, echoed back verbatim. Null when no inputs were provided." + }, + "status": { + "$ref": "#/$defs/httpStatus" + }, + "status_code": { + "$ref": "#/$defs/httpStatusCode" + } + }, + "required": [ + "message", + "workflow_type", + "workflow_id", + "ref", + "inputs", + "status", + "status_code" + ] + }, + { + "title": "rerun_workflow_run, rerun_failed_jobs, cancel_workflow_run, delete_workflow_run_logs", + "type": "object", + "description": "Returned by method=rerun_workflow_run, rerun_failed_jobs, cancel_workflow_run and delete_workflow_run_logs. All four return the same acknowledgement shape; only the message text differs.", + "properties": { + "message": { + "$ref": "#/$defs/message" + }, + "run_id": { + "type": "integer", + "description": "The workflow run ID the operation was applied to." + }, + "status": { + "$ref": "#/$defs/httpStatus" + }, + "status_code": { + "$ref": "#/$defs/httpStatusCode" + } + }, + "required": [ + "message", + "run_id", + "status", + "status_code" + ] + } + ] +} diff --git a/pkg/github/output_schemas/discussion_comment_write.json b/pkg/github/output_schemas/discussion_comment_write.json new file mode 100644 index 0000000000..dffe96ef0f --- /dev/null +++ b/pkg/github/output_schemas/discussion_comment_write.json @@ -0,0 +1,45 @@ +{ + "$schema": "https://json-schema.org/draft/2020-12/schema", + "type": "object", + "description": "Result of discussion_comment_write. The shape depends on the 'method' argument: comment mutations ('add', 'reply', 'update', 'delete') return the affected comment's node ID and URL; answer mutations ('mark_answer', 'unmark_answer') return the parent discussion's node ID and URL.", + "anyOf": [ + { + "title": "add | reply | update | delete", + "type": "object", + "description": "Minimal identity of the discussion comment that was created, replied to, updated, or deleted.", + "properties": { + "id": { + "type": "string", + "description": "GraphQL node ID of the discussion comment." + }, + "url": { + "type": "string", + "description": "HTML URL of the discussion comment." + } + }, + "required": [ + "id", + "url" + ] + }, + { + "title": "mark_answer | unmark_answer", + "type": "object", + "description": "Minimal identity of the discussion whose answer was marked or unmarked.", + "properties": { + "discussionID": { + "type": "string", + "description": "GraphQL node ID of the discussion that contains the comment." + }, + "discussionURL": { + "type": "string", + "description": "HTML URL of the discussion that contains the comment." + } + }, + "required": [ + "discussionID", + "discussionURL" + ] + } + ] +} diff --git a/pkg/github/output_schemas/issue_dependency_read.json b/pkg/github/output_schemas/issue_dependency_read.json new file mode 100644 index 0000000000..1837f7a0d5 --- /dev/null +++ b/pkg/github/output_schemas/issue_dependency_read.json @@ -0,0 +1,72 @@ +{ + "$schema": "https://json-schema.org/draft/2020-12/schema", + "title": "issue_dependency_read result", + "description": "Result of issue_dependency_read. Both methods (get_blocked_by, get_blocking) return the same shape: the related issues projected into the compact MinimalIssueRef form, plus page-based pagination info.", + "type": "object", + "properties": { + "issues": { + "type": "array", + "description": "The related issues. For get_blocked_by these are the issues blocking the subject issue; for get_blocking these are the issues the subject issue blocks. Empty when there are none.", + "items": { + "$ref": "#/$defs/minimalIssueRef" + } + }, + "pageInfo": { + "type": "object", + "description": "Page-based pagination info derived from the REST Link header.", + "properties": { + "hasNextPage": { + "type": "boolean", + "description": "True when the API advertised a next page." + }, + "nextPage": { + "type": "integer", + "description": "The next page number, or 0 when there is no next page." + } + }, + "required": [ + "hasNextPage", + "nextPage" + ] + } + }, + "required": [ + "issues", + "pageInfo" + ], + "$defs": { + "minimalIssueRef": { + "type": "object", + "title": "MinimalIssueRef", + "description": "Compact reference to a related issue.", + "properties": { + "number": { + "type": "integer", + "description": "The issue number." + }, + "title": { + "type": "string", + "description": "The issue title." + }, + "state": { + "type": "string", + "description": "The issue state, upper-cased to match the GraphQL-sourced state used elsewhere (e.g. OPEN, CLOSED)." + }, + "url": { + "type": "string", + "description": "The issue's HTML URL." + }, + "repository": { + "type": "string", + "description": "The issue's repository as \"owner/repo\". Omitted when it could not be derived from the issue's repository URL." + } + }, + "required": [ + "number", + "title", + "state", + "url" + ] + } + } +} diff --git a/pkg/github/output_schemas/issue_read.json b/pkg/github/output_schemas/issue_read.json new file mode 100644 index 0000000000..3b65824c96 --- /dev/null +++ b/pkg/github/output_schemas/issue_read.json @@ -0,0 +1,467 @@ +{ + "$schema": "https://json-schema.org/draft/2020-12/schema", + "description": "Output of the issue_read tool. The shape depends on the `method` argument: `get` returns a single minimal issue object, `get_comments` and `get_sub_issues` return bare JSON arrays, `get_parent` and `get_labels` return small wrapper objects.", + "anyOf": [ + { + "title": "method=get", + "description": "A single issue as MinimalIssue, enriched with hierarchy signals and GraphQL field values. The verbose REST `issue_field_values` array is always dropped for this method.", + "type": "object", + "properties": { + "number": { + "type": "integer" + }, + "title": { + "type": "string" + }, + "body": { + "type": "string" + }, + "state": { + "type": "string" + }, + "state_reason": { + "type": "string" + }, + "draft": { + "type": "boolean" + }, + "locked": { + "type": "boolean" + }, + "html_url": { + "type": "string" + }, + "user": { + "$ref": "#/$defs/minimalUser" + }, + "author_association": { + "type": "string" + }, + "labels": { + "type": "array", + "items": { + "type": "string" + } + }, + "assignees": { + "type": "array", + "items": { + "type": "string" + } + }, + "milestone": { + "type": "string" + }, + "comments": { + "type": "integer" + }, + "reactions": { + "$ref": "#/$defs/minimalReactions" + }, + "created_at": { + "type": "string" + }, + "updated_at": { + "type": "string" + }, + "closed_at": { + "type": "string" + }, + "closed_by": { + "type": "string" + }, + "issue_type": { + "type": "string" + }, + "field_values": { + "type": "array", + "description": "Custom issue field values resolved via GraphQL.", + "items": { + "type": "object", + "properties": { + "field": { + "type": "string" + }, + "value": { + "type": "string" + }, + "values": { + "type": "array", + "items": { + "type": "string" + } + } + }, + "required": [ + "field" + ] + } + }, + "has_parent": { + "type": "boolean" + }, + "has_children": { + "type": "boolean" + }, + "parent": { + "$ref": "#/$defs/issueRef" + }, + "sub_issues_summary": { + "type": "object", + "properties": { + "total": { + "type": "integer" + }, + "completed": { + "type": "integer" + }, + "percent_completed": { + "type": "integer" + } + }, + "required": [ + "total", + "completed", + "percent_completed" + ] + } + }, + "required": [ + "number", + "title", + "state" + ] + }, + { + "title": "method=get_comments", + "description": "A bare array of trimmed issue comments (MinimalIssueComment). Always an array; empty when the issue has no comments or lockdown filtering removed them all.", + "type": "array", + "items": { + "type": "object", + "properties": { + "id": { + "type": "integer" + }, + "body": { + "type": "string" + }, + "html_url": { + "type": "string" + }, + "user": { + "$ref": "#/$defs/minimalUser" + }, + "author_association": { + "type": "string" + }, + "reactions": { + "$ref": "#/$defs/minimalReactions" + }, + "created_at": { + "type": "string" + }, + "updated_at": { + "type": "string" + } + }, + "required": [ + "id", + "html_url" + ] + } + }, + { + "title": "method=get_sub_issues", + "description": "A bare array of full GitHub REST issue objects. go-github declares `type SubIssue Issue` (a defined type over Issue, not an alias), so items carry the complete REST issue payload rather than a trimmed type; every field is omitempty, so an item may be sparse. Always an array — empty when the issue has no sub-issues or lockdown filtering removed them all.", + "type": "array", + "items": { + "type": "object", + "properties": { + "id": { + "type": "integer" + }, + "node_id": { + "type": "string" + }, + "number": { + "type": "integer" + }, + "state": { + "type": "string" + }, + "state_reason": { + "type": "string" + }, + "locked": { + "type": "boolean" + }, + "active_lock_reason": { + "type": "string" + }, + "title": { + "type": "string" + }, + "body": { + "type": "string" + }, + "draft": { + "type": "boolean" + }, + "author_association": { + "type": "string" + }, + "user": { + "type": "object" + }, + "assignee": { + "type": "object" + }, + "assignees": { + "type": "array", + "items": { + "type": "object" + } + }, + "closed_by": { + "type": "object" + }, + "labels": { + "type": "array", + "items": { + "type": "object" + } + }, + "milestone": { + "type": "object" + }, + "type": { + "type": "object" + }, + "reactions": { + "type": "object" + }, + "pull_request": { + "type": "object" + }, + "repository": { + "type": "object" + }, + "pinned_comment": { + "type": "object" + }, + "performed_via_github_app": { + "type": "object" + }, + "issue_dependencies_summary": { + "type": "object" + }, + "sub_issues_summary": { + "type": "object" + }, + "issue_field_values": { + "type": "array", + "items": { + "type": "object" + } + }, + "text_matches": { + "type": "array", + "items": { + "type": "object" + } + }, + "comments": { + "type": "integer" + }, + "created_at": { + "type": "string" + }, + "updated_at": { + "type": "string" + }, + "closed_at": { + "type": "string" + }, + "url": { + "type": "string" + }, + "html_url": { + "type": "string" + }, + "comments_url": { + "type": "string" + }, + "events_url": { + "type": "string" + }, + "labels_url": { + "type": "string" + }, + "repository_url": { + "type": "string" + }, + "parent_issue_url": { + "type": "string" + } + } + } + }, + { + "title": "method=get_parent", + "description": "Wrapper object whose single `parent` key is null when the issue has no parent, or when lockdown mode cannot verify the parent as safe content.", + "type": "object", + "properties": { + "parent": { + "anyOf": [ + { + "type": "null" + }, + { + "$ref": "#/$defs/issueRef" + } + ] + } + }, + "required": [ + "parent" + ] + }, + { + "title": "method=get_labels", + "description": "Wrapper object with the labels currently applied to the issue (GraphQL, first 100) and the total label count.", + "type": "object", + "properties": { + "labels": { + "type": "array", + "items": { + "type": "object", + "properties": { + "id": { + "type": "string" + }, + "name": { + "type": "string" + }, + "color": { + "type": "string" + }, + "description": { + "type": "string" + } + }, + "required": [ + "id", + "name", + "color", + "description" + ] + } + }, + "totalCount": { + "type": "integer" + } + }, + "required": [ + "labels", + "totalCount" + ] + } + ], + "$defs": { + "minimalUser": { + "type": "object", + "description": "Trimmed user reference (MinimalUser).", + "properties": { + "login": { + "type": "string" + }, + "id": { + "type": "integer" + }, + "profile_url": { + "type": "string" + }, + "avatar_url": { + "type": "string" + }, + "details": { + "type": "object" + } + }, + "required": [ + "login" + ] + }, + "minimalReactions": { + "type": "object", + "description": "Reaction counts (MinimalReactions). Every count key is always present when the object is emitted.", + "properties": { + "total_count": { + "type": "integer" + }, + "+1": { + "type": "integer" + }, + "-1": { + "type": "integer" + }, + "laugh": { + "type": "integer" + }, + "confused": { + "type": "integer" + }, + "heart": { + "type": "integer" + }, + "hooray": { + "type": "integer" + }, + "rocket": { + "type": "integer" + }, + "eyes": { + "type": "integer" + } + }, + "required": [ + "total_count", + "+1", + "-1", + "laugh", + "confused", + "heart", + "hooray", + "rocket", + "eyes" + ] + }, + "issueRef": { + "type": "object", + "description": "Compact reference to a related issue. Used for `parent` in both method=get (MinimalIssueRef, where `repository` is omitted when empty) and method=get_parent (an inline map that always sets `repository`).", + "properties": { + "number": { + "type": "integer" + }, + "title": { + "type": "string" + }, + "state": { + "type": "string" + }, + "url": { + "type": "string" + }, + "repository": { + "type": "string" + } + }, + "required": [ + "number", + "title", + "state", + "url" + ] + } + } +} diff --git a/pkg/github/output_schemas/pull_request_read.json b/pkg/github/output_schemas/pull_request_read.json new file mode 100644 index 0000000000..b3fa1ddb55 --- /dev/null +++ b/pkg/github/output_schemas/pull_request_read.json @@ -0,0 +1,595 @@ +{ + "$schema": "https://json-schema.org/draft/2020-12/schema", + "description": "Output of the pull_request_read tool. The shape depends on the `method` argument: each anyOf branch is titled with the method that produces it. anyOf (never oneOf) is required because branches overlap: an empty array satisfies every array branch, and objects whose optional fields are all absent satisfy several object branches. Note: method=get_diff returns the raw unified diff as text content only and emits no structuredContent, so no string branch appears here.", + "anyOf": [ + { + "title": "get", + "description": "method=get: a single pull request, trimmed to MinimalPullRequest.", + "type": "object", + "properties": { + "number": { + "type": "integer" + }, + "title": { + "type": "string" + }, + "body": { + "type": "string" + }, + "state": { + "type": "string" + }, + "draft": { + "type": "boolean" + }, + "merged": { + "type": "boolean" + }, + "mergeable_state": { + "type": "string" + }, + "html_url": { + "type": "string" + }, + "user": { + "$ref": "#/$defs/minimalUser" + }, + "labels": { + "type": "array", + "items": { + "type": "string" + } + }, + "assignees": { + "type": "array", + "items": { + "type": "string" + } + }, + "requested_reviewers": { + "type": "array", + "items": { + "type": "string" + } + }, + "merged_by": { + "type": "string" + }, + "head": { + "$ref": "#/$defs/minimalPRBranch" + }, + "base": { + "$ref": "#/$defs/minimalPRBranch" + }, + "additions": { + "type": "integer" + }, + "deletions": { + "type": "integer" + }, + "changed_files": { + "type": "integer" + }, + "commits": { + "type": "integer" + }, + "comments": { + "type": "integer" + }, + "created_at": { + "type": "string" + }, + "updated_at": { + "type": "string" + }, + "closed_at": { + "type": "string" + }, + "merged_at": { + "type": "string" + }, + "milestone": { + "type": "string" + } + }, + "required": [ + "number", + "title", + "state", + "draft", + "merged", + "html_url" + ] + }, + { + "title": "get_status", + "description": "method=get_status: the GitHub combined commit status for the pull request head SHA, marshalled from *github.CombinedStatus with no minimal-type trimming. Every field is optional, so an empty object is valid. additionalProperties is false because the payload is a fixed Go struct with no custom marshaller: unknown GitHub API fields are dropped at unmarshal time and can never reach the client.", + "type": "object", + "properties": { + "state": { + "type": "string" + }, + "name": { + "type": "string" + }, + "sha": { + "type": "string" + }, + "total_count": { + "type": "integer" + }, + "statuses": { + "type": "array", + "items": { + "$ref": "#/$defs/repoStatus" + } + }, + "commit_url": { + "type": "string" + }, + "repository_url": { + "type": "string" + } + }, + "additionalProperties": false + }, + { + "title": "get_files", + "description": "method=get_files: files changed in the pull request, trimmed to MinimalPRFile. Empty when the requested page has no files.", + "type": "array", + "items": { + "$ref": "#/$defs/minimalPRFile" + } + }, + { + "title": "get_commits", + "description": "method=get_commits: commits on the pull request, trimmed to MinimalPullRequestCommit. Empty when the requested page has no commits.", + "type": "array", + "items": { + "$ref": "#/$defs/minimalPullRequestCommit" + } + }, + { + "title": "get_review_comments", + "description": "method=get_review_comments: review threads with their comments, plus GraphQL cursor pagination info.", + "type": "object", + "properties": { + "review_threads": { + "type": "array", + "items": { + "$ref": "#/$defs/minimalReviewThread" + } + }, + "totalCount": { + "type": "integer" + }, + "pageInfo": { + "$ref": "#/$defs/minimalPageInfo" + } + }, + "required": [ + "review_threads", + "totalCount", + "pageInfo" + ] + }, + { + "title": "get_reviews", + "description": "method=get_reviews: reviews on the pull request, trimmed to MinimalPullRequestReview. Empty when the requested page has no reviews.", + "type": "array", + "items": { + "$ref": "#/$defs/minimalPullRequestReview" + } + }, + { + "title": "get_comments", + "description": "method=get_comments: issue-style comments on the pull request, trimmed to MinimalIssueComment. Empty when the requested page has no comments.", + "type": "array", + "items": { + "$ref": "#/$defs/minimalIssueComment" + } + }, + { + "title": "get_check_runs", + "description": "method=get_check_runs: check runs for the pull request head SHA, trimmed to MinimalCheckRunsResult.", + "type": "object", + "properties": { + "total_count": { + "type": "integer" + }, + "check_runs": { + "type": "array", + "items": { + "$ref": "#/$defs/minimalCheckRun" + } + } + }, + "required": [ + "total_count", + "check_runs" + ] + } + ], + "$defs": { + "minimalUser": { + "type": "object", + "description": "Trimmed user (MinimalUser).", + "properties": { + "login": { + "type": "string" + }, + "id": { + "type": "integer" + }, + "profile_url": { + "type": "string" + }, + "avatar_url": { + "type": "string" + }, + "details": { + "type": "object" + } + }, + "required": [ + "login" + ] + }, + "minimalPRBranch": { + "type": "object", + "description": "Trimmed pull request head/base reference (MinimalPRBranch).", + "properties": { + "ref": { + "type": "string" + }, + "sha": { + "type": "string" + }, + "repo": { + "type": "object", + "properties": { + "full_name": { + "type": "string" + }, + "description": { + "type": "string" + } + }, + "required": [ + "full_name" + ] + } + }, + "required": [ + "ref", + "sha" + ] + }, + "minimalCommitAuthor": { + "type": "object", + "description": "Trimmed commit author (MinimalCommitAuthor); every field is optional.", + "properties": { + "name": { + "type": "string" + }, + "email": { + "type": "string" + }, + "date": { + "type": "string" + } + } + }, + "minimalReactions": { + "type": "object", + "description": "Trimmed reaction summary (MinimalReactions).", + "properties": { + "total_count": { + "type": "integer" + }, + "+1": { + "type": "integer" + }, + "-1": { + "type": "integer" + }, + "laugh": { + "type": "integer" + }, + "confused": { + "type": "integer" + }, + "heart": { + "type": "integer" + }, + "hooray": { + "type": "integer" + }, + "rocket": { + "type": "integer" + }, + "eyes": { + "type": "integer" + } + } + }, + "minimalPRFile": { + "type": "object", + "properties": { + "filename": { + "type": "string" + }, + "status": { + "type": "string" + }, + "additions": { + "type": "integer" + }, + "deletions": { + "type": "integer" + }, + "changes": { + "type": "integer" + }, + "patch": { + "type": "string" + }, + "previous_filename": { + "type": "string" + } + }, + "required": [ + "filename" + ] + }, + "minimalPullRequestCommit": { + "type": "object", + "properties": { + "sha": { + "type": "string" + }, + "html_url": { + "type": "string" + }, + "message": { + "type": "string" + }, + "author": { + "$ref": "#/$defs/minimalCommitAuthor" + } + }, + "required": [ + "sha" + ] + }, + "minimalPullRequestReview": { + "type": "object", + "properties": { + "id": { + "type": "integer" + }, + "state": { + "type": "string" + }, + "body": { + "type": "string" + }, + "html_url": { + "type": "string" + }, + "user": { + "$ref": "#/$defs/minimalUser" + }, + "commit_id": { + "type": "string" + }, + "submitted_at": { + "type": "string" + }, + "author_association": { + "type": "string" + } + }, + "required": [ + "id", + "state", + "html_url" + ] + }, + "minimalIssueComment": { + "type": "object", + "properties": { + "id": { + "type": "integer" + }, + "body": { + "type": "string" + }, + "html_url": { + "type": "string" + }, + "user": { + "$ref": "#/$defs/minimalUser" + }, + "author_association": { + "type": "string" + }, + "reactions": { + "$ref": "#/$defs/minimalReactions" + }, + "created_at": { + "type": "string" + }, + "updated_at": { + "type": "string" + } + }, + "required": [ + "id", + "html_url" + ] + }, + "minimalReviewComment": { + "type": "object", + "properties": { + "body": { + "type": "string" + }, + "path": { + "type": "string" + }, + "line": { + "type": [ + "integer", + "null" + ] + }, + "author": { + "type": "string" + }, + "created_at": { + "type": "string" + }, + "updated_at": { + "type": "string" + }, + "html_url": { + "type": "string" + } + }, + "required": [ + "path", + "html_url" + ] + }, + "minimalReviewThread": { + "type": "object", + "properties": { + "id": { + "type": "string" + }, + "is_resolved": { + "type": "boolean" + }, + "is_outdated": { + "type": "boolean" + }, + "is_collapsed": { + "type": "boolean" + }, + "comments": { + "type": "array", + "items": { + "$ref": "#/$defs/minimalReviewComment" + } + }, + "total_count": { + "type": "integer" + } + }, + "required": [ + "id", + "is_resolved", + "is_outdated", + "is_collapsed", + "comments", + "total_count" + ] + }, + "minimalPageInfo": { + "type": "object", + "properties": { + "hasNextPage": { + "type": "boolean" + }, + "hasPreviousPage": { + "type": "boolean" + }, + "startCursor": { + "type": "string" + }, + "endCursor": { + "type": "string" + } + }, + "required": [ + "hasNextPage", + "hasPreviousPage" + ] + }, + "minimalCheckRun": { + "type": "object", + "properties": { + "id": { + "type": "integer" + }, + "name": { + "type": "string" + }, + "status": { + "type": "string" + }, + "conclusion": { + "type": "string" + }, + "html_url": { + "type": "string" + }, + "details_url": { + "type": "string" + }, + "started_at": { + "type": "string" + }, + "completed_at": { + "type": "string" + } + }, + "required": [ + "id", + "name", + "status" + ] + }, + "repoStatus": { + "type": "object", + "description": "An individual commit status inside the combined status, marshalled from *github.RepoStatus. Like the parent it is a fixed Go struct, so no unlisted fields can appear.", + "properties": { + "id": { + "type": "integer" + }, + "node_id": { + "type": "string" + }, + "url": { + "type": "string" + }, + "state": { + "type": "string" + }, + "target_url": { + "type": "string" + }, + "description": { + "type": "string" + }, + "context": { + "type": "string" + }, + "avatar_url": { + "type": "string" + }, + "creator": { + "type": "object" + }, + "created_at": { + "type": "string" + }, + "updated_at": { + "type": "string" + } + }, + "additionalProperties": false + } + } +} diff --git a/pkg/github/output_schemas_polymorphic.go b/pkg/github/output_schemas_polymorphic.go new file mode 100644 index 0000000000..68189cf6bf --- /dev/null +++ b/pkg/github/output_schemas_polymorphic.go @@ -0,0 +1,71 @@ +package github + +import ( + "bytes" + "embed" + "encoding/json" + "fmt" +) + +// Output schemas for tools whose response shape varies by the `method` +// argument. +// +// These are hand-authored rather than inferred from Go types, because no +// single Go type describes a method-dispatched tool's output. They live as +// .json files so they stay reviewable and diffable, and are embedded rather +// than pasted into Go string literals (their descriptions contain markdown +// backticks, which raw string literals cannot hold). +// +// Every union uses anyOf, never oneOf. oneOf requires EXACTLY ONE branch to +// match, which is provably wrong here: actions_run_trigger has four +// structurally identical branches, an empty array vacuously satisfies every +// array branch, and an issue_read `get` on a sub-issue also satisfies the +// `get_parent` branch. See AnyOfSchema in output_schema.go. +// +// Schemas whose root is not {"type":"object"} are legal only from protocol +// version 2026-07-28 (SEP-2106); inventory.OutputSchemaVersionGate strips them +// per-request for older clients. TestPolymorphicOutputSchemaRootKinds pins +// which schemas fall on which side of that line. +// +//go:embed output_schemas/*.json +var outputSchemaFS embed.FS + +var ( + // Object-root schemas: legal under every protocol revision that supports + // outputSchema at all, so these ship to every client ungated even though + // they use anyOf internally. + actionsGetOutputSchema = mustLoadOutputSchema("actions_get") + actionsListOutputSchema = mustLoadOutputSchema("actions_list") + actionsRunTriggerOutputSchema = mustLoadOutputSchema("actions_run_trigger") + discussionCommentWriteOutputSchema = mustLoadOutputSchema("discussion_comment_write") + + // Not actually a union: both methods of issue_dependency_read return the + // same shape, so this is a single object schema. + issueDependencyReadOutputSchema = mustLoadOutputSchema("issue_dependency_read") + + // Non-object roots (bare anyOf spanning objects and arrays): gated to + // clients speaking 2026-07-28 or later. + issueReadOutputSchema = mustLoadOutputSchema("issue_read") + pullRequestReadOutputSchema = mustLoadOutputSchema("pull_request_read") +) + +// mustLoadOutputSchema reads an embedded schema, compacts it, and verifies it +// resolves — panicking during package initialization on any failure so a +// malformed schema or dangling $ref fails the build rather than a request. +// +// Compacting is not cosmetic: it strips inter-token whitespace, so a checkout +// that rewrote the files' line endings (git's core.autocrlf does this on +// Windows, and it is what corrupts pkg/octicons' embedded data URIs) cannot +// leak stray carriage returns into what is sent to clients. It preserves +// string contents exactly, so descriptions are untouched. +func mustLoadOutputSchema(name string) json.RawMessage { + raw, err := outputSchemaFS.ReadFile(fmt.Sprintf("output_schemas/%s.json", name)) + if err != nil { + panic(fmt.Sprintf("output schema %q: %v", name, err)) + } + var compact bytes.Buffer + if err := json.Compact(&compact, raw); err != nil { + panic(fmt.Sprintf("output schema %q: %v", name, err)) + } + return MustRawOutputSchema(compact.String()) +} diff --git a/pkg/github/output_schemas_polymorphic_test.go b/pkg/github/output_schemas_polymorphic_test.go new file mode 100644 index 0000000000..18803266b6 --- /dev/null +++ b/pkg/github/output_schemas_polymorphic_test.go @@ -0,0 +1,239 @@ +package github + +import ( + "context" + "encoding/json" + "net/http" + "strings" + "testing" + + gogithub "github.com/google/go-github/v89/github" + "github.com/google/jsonschema-go/jsonschema" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/github/github-mcp-server/pkg/inventory" + "github.com/github/github-mcp-server/pkg/translations" +) + +// polymorphicSchemas is the full set declared by this package, keyed by tool. +func polymorphicSchemas() map[string]json.RawMessage { + return map[string]json.RawMessage{ + "actions_get": actionsGetOutputSchema, + "actions_list": actionsListOutputSchema, + "actions_run_trigger": actionsRunTriggerOutputSchema, + "discussion_comment_write": discussionCommentWriteOutputSchema, + "issue_dependency_read": issueDependencyReadOutputSchema, + "issue_read": issueReadOutputSchema, + "pull_request_read": pullRequestReadOutputSchema, + } +} + +// oneOf requires EXACTLY ONE branch to match. These unions have structurally +// identical branches, empty collections that satisfy several array branches at +// once, and optional fields that overlap between branches — so oneOf would +// reject valid output. Guard against a well-meaning future "tightening". +func TestPolymorphicOutputSchemasNeverUseOneOf(t *testing.T) { + for tool, schema := range polymorphicSchemas() { + t.Run(tool, func(t *testing.T) { + assert.NotContains(t, string(schema), `"oneOf"`, + "use anyOf; oneOf requires exactly one branch to match and breaks on ambiguous payloads") + }) + } +} + +func TestPolymorphicOutputSchemasResolve(t *testing.T) { + for tool, schema := range polymorphicSchemas() { + t.Run(tool, func(t *testing.T) { + var s jsonschema.Schema + require.NoError(t, json.Unmarshal(schema, &s)) + _, err := s.Resolve(nil) + require.NoError(t, err, "schema must resolve; a dangling $ref would fail at request time") + }) + } +} + +// Pins which schemas the protocol-version gate will withhold from clients +// older than 2026-07-28. A root of {"type":"object"} is representable under +// 2025-11-25 and ships to everyone; anything else is gated. +func TestPolymorphicOutputSchemaRootKinds(t *testing.T) { + wantObjectRoot := map[string]bool{ + "actions_get": true, + "actions_list": true, + "actions_run_trigger": true, + "discussion_comment_write": true, + "issue_dependency_read": true, + // Bare anyOf spanning objects and arrays — not representable before + // 2026-07-28, so these two are the gated ones. + "issue_read": false, + "pull_request_read": false, + } + for tool, schema := range polymorphicSchemas() { + t.Run(tool, func(t *testing.T) { + assert.Equal(t, wantObjectRoot[tool], inventory.HasObjectRootOutputSchema(schema)) + }) + } +} + +// Every schema must reject values that match no branch. A union that accepts +// anything documents nothing. +func TestPolymorphicOutputSchemasRejectGarbage(t *testing.T) { + for tool, schema := range polymorphicSchemas() { + t.Run(tool, func(t *testing.T) { + var s jsonschema.Schema + require.NoError(t, json.Unmarshal(schema, &s)) + resolved, err := s.Resolve(nil) + require.NoError(t, err) + assert.Error(t, resolved.Validate("a bare string")) + assert.Error(t, resolved.Validate(float64(42))) + assert.Error(t, resolved.Validate(nil)) + }) + } +} + +func resolveToolSchema(t *testing.T, schema json.RawMessage) *jsonschema.Resolved { + t.Helper() + var s jsonschema.Schema + require.NoError(t, json.Unmarshal(schema, &s)) + resolved, err := s.Resolve(nil) + require.NoError(t, err) + return resolved +} + +// The end-to-end guarantee: what the real handler actually emits must validate +// against the schema the tool advertises. The mirror sets structuredContent to +// the exact bytes of the text block, so validating the text block here is +// equivalent to validating the structured content a client would receive. +func TestIssueReadOutputValidatesAgainstDeclaredSchema(t *testing.T) { + t.Parallel() + + serverTool := IssueRead(translations.NullTranslationHelper) + resolved := resolveToolSchema(t, issueReadOutputSchema) + + mockIssue := &gogithub.Issue{ + Number: gogithub.Ptr(1), + Title: gogithub.Ptr("Test issue"), + State: gogithub.Ptr("open"), + HTMLURL: gogithub.Ptr("https://github.com/octocat/repo/issues/1"), + User: &gogithub.User{Login: gogithub.Ptr("octocat")}, + } + + tests := []struct { + name string + method string + handlers map[string]http.HandlerFunc + }{ + { + name: "get", + method: "get", + handlers: map[string]http.HandlerFunc{ + GetReposIssuesByOwnerByRepoByIssueNumber: mockResponse(t, http.StatusOK, mockIssue), + }, + }, + { + // A fully sparse issue: every optional field absent. Exercises the + // branch's required set rather than a convenient fixture. + name: "get with all optionals absent", + method: "get", + handlers: map[string]http.HandlerFunc{ + GetReposIssuesByOwnerByRepoByIssueNumber: mockResponse(t, http.StatusOK, &gogithub.Issue{}), + }, + }, + { + name: "get_comments", + method: "get_comments", + handlers: map[string]http.HandlerFunc{ + GetReposIssuesCommentsByOwnerByRepoByIssueNumber: mockResponse(t, http.StatusOK, + []*gogithub.IssueComment{{ + ID: gogithub.Ptr(int64(1)), + Body: gogithub.Ptr("hello"), + HTMLURL: gogithub.Ptr("https://github.com/octocat/repo/issues/1#issuecomment-1"), + User: &gogithub.User{Login: gogithub.Ptr("octocat")}, + }}), + }, + }, + { + // The ambiguous case that makes oneOf unusable: [] satisfies every + // array branch at once. + name: "get_comments with no comments", + method: "get_comments", + handlers: map[string]http.HandlerFunc{ + GetReposIssuesCommentsByOwnerByRepoByIssueNumber: mockResponse(t, http.StatusOK, []*gogithub.IssueComment{}), + }, + }, + { + name: "get_sub_issues", + method: "get_sub_issues", + handlers: map[string]http.HandlerFunc{ + GetReposIssuesSubIssuesByOwnerByRepoByIssueNumber: mockResponse(t, http.StatusOK, + []*gogithub.Issue{mockIssue}), + }, + }, + { + name: "get_sub_issues with none", + method: "get_sub_issues", + handlers: map[string]http.HandlerFunc{ + GetReposIssuesSubIssuesByOwnerByRepoByIssueNumber: mockResponse(t, http.StatusOK, []*gogithub.Issue{}), + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + deps := BaseDeps{Client: mustNewGHClient(t, MockHTTPClientWithHandlers(tt.handlers))} + handler := serverTool.Handler(deps) + + request := createMCPRequest(map[string]any{ + "method": tt.method, + "owner": "octocat", + "repo": "repo", + "issue_number": float64(1), + }) + result, err := handler(ContextWithDeps(context.Background(), deps), &request) + require.NoError(t, err) + require.False(t, result.IsError, "handler should succeed") + + text := getTextResult(t, result) + var payload any + require.NoError(t, json.Unmarshal([]byte(text.Text), &payload), + "handler output must be JSON for the mirror to publish it as structuredContent") + + require.NoError(t, resolved.Validate(payload), + "real handler output for method=%s must conform to the advertised outputSchema", tt.method) + }) + } +} + +// go-github declares `var subIssues []*SubIssue` and leaves it nil on an empty +// body, which marshals to the literal `null` rather than `[]`. The schema +// rejects null, deliberately: the right fix is to normalise in the handler, +// not to widen the contract for every caller. +func TestGetSubIssuesNeverEmitsNull(t *testing.T) { + t.Parallel() + + serverTool := IssueRead(translations.NullTranslationHelper) + deps := BaseDeps{Client: mustNewGHClient(t, MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + // A 200 whose body is JSON null — what go-github leaves the slice nil on. + GetReposIssuesSubIssuesByOwnerByRepoByIssueNumber: func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte(`null`)) + }, + }))} + + request := createMCPRequest(map[string]any{ + "method": "get_sub_issues", "owner": "octocat", "repo": "repo", "issue_number": float64(1), + }) + result, err := serverTool.Handler(deps)(ContextWithDeps(context.Background(), deps), &request) + require.NoError(t, err) + require.False(t, result.IsError) + + text := getTextResult(t, result) + assert.NotEqual(t, "null", strings.TrimSpace(text.Text), + "a nil slice must be normalised to [] before marshalling, or structuredContent violates the schema") + + require.NoError(t, resolveToolSchema(t, issueReadOutputSchema).Validate(func() any { + var v any + require.NoError(t, json.Unmarshal([]byte(text.Text), &v)) + return v + }())) +} diff --git a/pkg/github/pullrequests.go b/pkg/github/pullrequests.go index daf3b97331..ecb0250fa6 100644 --- a/pkg/github/pullrequests.go +++ b/pkg/github/pullrequests.go @@ -157,7 +157,7 @@ Possible options: default: return utils.NewToolResultError(fmt.Sprintf("unknown method: %s", method)), nil, nil } - }) + }).WithOutputSchema(pullRequestReadOutputSchema) } func GetPullRequest(ctx context.Context, client *github.Client, deps ToolDependencies, owner, repo string, pullNumber int) (*mcp.CallToolResult, error) { diff --git a/pkg/github/server.go b/pkg/github/server.go index 43e0940017..eb1a3ada6d 100644 --- a/pkg/github/server.go +++ b/pkg/github/server.go @@ -101,6 +101,11 @@ func NewMCPServer(ctx context.Context, cfg *MCPServerConfig, deps ToolDependenci // Add middlewares. Order matters - for example, the error context middleware should be applied last so that it runs FIRST (closest to the handler) to ensure all errors are captured, // and any middleware that needs to read or modify the context should be before it. ghServer.AddReceivingMiddleware(middleware...) + // Runs outermost of the three so it sees the final tools/list result: it + // removes output schemas whose root is not `{"type":"object"}` from + // responses to clients older than 2026-07-28, which cannot represent them. + // No-op when the output_schemas feature is off, since nothing declares one. + ghServer.AddReceivingMiddleware(inventory.OutputSchemaVersionGate()) ghServer.AddReceivingMiddleware(InjectDepsMiddleware(deps)) ghServer.AddReceivingMiddleware(addGitHubAPIErrorToContext) diff --git a/pkg/inventory/builder.go b/pkg/inventory/builder.go index 9ecaca1f57..12797152d9 100644 --- a/pkg/inventory/builder.go +++ b/pkg/inventory/builder.go @@ -14,10 +14,18 @@ var ( ErrUnknownTools = errors.New("unknown tools specified in WithTools") ) -// mcpAppsFeatureFlag is the feature flag name that controls MCP Apps UI metadata. -// This is defined here to avoid importing pkg/github (which imports pkg/inventory). -// The value must match github.MCPAppsFeatureFlag. -const mcpAppsFeatureFlag = "remote_mcp_ui_apps" +// These are defined here to avoid importing pkg/github (which imports +// pkg/inventory). The values must match their github package counterparts. +const ( + // mcpAppsFeatureFlag is the feature flag name that controls MCP Apps UI metadata. + // The value must match github.MCPAppsFeatureFlag. + mcpAppsFeatureFlag = "remote_mcp_ui_apps" + + // outputSchemasFeatureFlag controls whether tools advertise outputSchema + // and return structuredContent. The value must match + // github.FeatureFlagOutputSchemas. + outputSchemasFeatureFlag = "output_schemas" +) // ToolFilter is a function that determines if a tool should be included. // Returns true if the tool should be included, false to exclude it. diff --git a/pkg/inventory/output_schema_gate.go b/pkg/inventory/output_schema_gate.go new file mode 100644 index 0000000000..c8b288f6b8 --- /dev/null +++ b/pkg/inventory/output_schema_gate.go @@ -0,0 +1,117 @@ +package inventory + +import ( + "context" + "encoding/json" + + "github.com/modelcontextprotocol/go-sdk/mcp" +) + +// ProtocolVersionNonObjectOutputSchemas is the first MCP protocol revision that +// permits a tool's outputSchema to have a root other than `{"type":"object"}`, +// and correspondingly permits structuredContent to be any JSON value rather +// than only a JSON object (SEP-2106). +// +// Under 2025-11-25 the normative schema typed outputSchema as a closed shape: +// +// outputSchema?: { $schema?: string; type: "object"; +// properties?: { [key: string]: object }; required?: string[]; } +// +// with the doc comment "Currently restricted to type: \"object\" at the root +// level." Both the restriction and the closed shape were removed in +// 2026-07-28, which types it as `{ $schema?: string; [key: string]: unknown }` +// and describes it as "any valid JSON Schema 2020-12". +const ProtocolVersionNonObjectOutputSchemas = "2026-07-28" + +// HasObjectRootOutputSchema reports whether schema marshals to a JSON Schema +// whose root declares `"type": "object"`. Such a schema is legal under every +// protocol revision that supports outputSchema at all, so it needs no gating — +// even when it uses composition keywords like anyOf internally. +// +// A schema that fails to marshal, or that declares any other root (a bare +// anyOf, `"type": "array"`, a $ref) is reported as non-object-root and is +// therefore gated. Failing closed is deliberate: an unmarshalable schema +// should not be advertised to a client that may reject the whole tools/list. +func HasObjectRootOutputSchema(schema any) bool { + if schema == nil { + return false + } + raw, err := json.Marshal(schema) + if err != nil { + return false + } + var root struct { + Type any `json:"type"` + } + if err := json.Unmarshal(raw, &root); err != nil { + return false + } + // 2020-12 permits `"type"` to be an array of types. Only a bare "object" + // satisfies the 2025-11-25 shape, so anything else is gated. + t, ok := root.Type.(string) + return ok && t == "object" +} + +// OutputSchemaVersionGate returns receiving middleware that removes +// non-object-root output schemas from tools/list responses sent to clients +// speaking a protocol revision older than 2026-07-28. +// +// Object-root schemas pass through untouched at every version. +// +// The middleware copies on write. The SDK's listTools appends the *same* +// *mcp.Tool pointers it holds in its registry (mcp/server.go:936-939), so +// mutating a tool in place here would strip the schema from the server's +// stored definition and leak that to every later session on the same server. +// Only tools that actually need gating are copied. +func OutputSchemaVersionGate() mcp.Middleware { + return func(next mcp.MethodHandler) mcp.MethodHandler { + return func(ctx context.Context, method string, req mcp.Request) (mcp.Result, error) { + res, err := next(ctx, method, req) + if err != nil || method != "tools/list" { + return res, err + } + listRes, ok := res.(*mcp.ListToolsResult) + if !ok || listRes == nil { + return res, err + } + listReq, ok := req.(*mcp.ListToolsRequest) + if !ok { + return res, err + } + // ProtocolVersion reads the per-request _meta for >= 2026-07-28 + // clients (which no longer send initialize at all, per SEP-2575) + // and falls back to the session's InitializeParams for older ones. + // An empty version means we could not determine it; gate in that + // case, since only a client we know is new can be trusted with a + // non-object root. + if listReq.ProtocolVersion() >= ProtocolVersionNonObjectOutputSchemas { + return res, err + } + listRes.Tools = stripNonObjectRootOutputSchemas(listRes.Tools) + return listRes, err + } + } +} + +// stripNonObjectRootOutputSchemas returns tools with non-object-root output +// schemas cleared, copying only the entries it changes and leaving the +// caller's slice untouched. +func stripNonObjectRootOutputSchemas(tools []*mcp.Tool) []*mcp.Tool { + var out []*mcp.Tool + for i, t := range tools { + if t == nil || t.OutputSchema == nil || HasObjectRootOutputSchema(t.OutputSchema) { + continue + } + if out == nil { + out = make([]*mcp.Tool, len(tools)) + copy(out, tools) + } + toolCopy := *t + toolCopy.OutputSchema = nil + out[i] = &toolCopy + } + if out == nil { + return tools + } + return out +} diff --git a/pkg/inventory/output_schema_gate_test.go b/pkg/inventory/output_schema_gate_test.go new file mode 100644 index 0000000000..baf170b024 --- /dev/null +++ b/pkg/inventory/output_schema_gate_test.go @@ -0,0 +1,146 @@ +package inventory + +import ( + "context" + "encoding/json" + "testing" + + "github.com/modelcontextprotocol/go-sdk/mcp" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestHasObjectRootOutputSchema(t *testing.T) { + tests := []struct { + name string + schema any + want bool + }{ + {"nil", nil, false}, + {"object root", json.RawMessage(`{"type":"object"}`), true}, + { + "object root with inner anyOf is still object root", + json.RawMessage(`{"type":"object","anyOf":[{"required":["a"]},{"required":["b"]}]}`), + true, + }, + {"bare anyOf", json.RawMessage(`{"anyOf":[{"type":"object"},{"type":"array"}]}`), false}, + {"array root", json.RawMessage(`{"type":"array","items":{"type":"object"}}`), false}, + {"string root", json.RawMessage(`{"type":"string"}`), false}, + {"bare $ref", json.RawMessage(`{"$ref":"#/$defs/x","$defs":{"x":{"type":"object"}}}`), false}, + // 2020-12 permits a type array; that is not the 2025-11-25 shape. + {"type array containing object", json.RawMessage(`{"type":["object","null"]}`), false}, + {"map form", map[string]any{"type": "object"}, true}, + {"unmarshalable fails closed", make(chan int), false}, + {"non-schema JSON fails closed", json.RawMessage(`"just a string"`), false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, HasObjectRootOutputSchema(tt.schema)) + }) + } +} + +// The SDK's listTools hands back the *same* *mcp.Tool pointers it stores in +// its registry, so a gate that mutated them in place would strip the schema +// from the server's own definition and leak that to every later session. +func TestStripNonObjectRootOutputSchemasDoesNotMutateInput(t *testing.T) { + objectRoot := json.RawMessage(`{"type":"object"}`) + bareAnyOf := json.RawMessage(`{"anyOf":[{"type":"array"},{"type":"object"}]}`) + + registry := []*mcp.Tool{ + {Name: "keeps", OutputSchema: objectRoot}, + {Name: "gets_stripped", OutputSchema: bareAnyOf}, + {Name: "no_schema"}, + } + + got := stripNonObjectRootOutputSchemas(registry) + + // The returned view is gated... + require.Len(t, got, 3) + assert.Equal(t, objectRoot, got[0].OutputSchema, "object roots survive the gate") + assert.Nil(t, got[1].OutputSchema, "non-object root is stripped from the returned view") + + // ...but the caller's tools are untouched. + assert.Equal(t, bareAnyOf, registry[1].OutputSchema, "registry tool must not be mutated") + assert.NotSame(t, registry[1], got[1], "the stripped entry must be a copy") + assert.Same(t, registry[0], got[0], "unchanged entries are not copied") +} + +func TestStripNonObjectRootOutputSchemasNoOp(t *testing.T) { + tools := []*mcp.Tool{ + {Name: "a", OutputSchema: json.RawMessage(`{"type":"object"}`)}, + {Name: "b"}, + } + got := stripNonObjectRootOutputSchemas(tools) + // Nothing needed gating, so the original slice is returned as-is. + assert.Equal(t, &tools, &got, "no-op should not allocate a new slice") +} + +func listToolsReqAtVersion(version string) *mcp.ListToolsRequest { + params := &mcp.ListToolsParams{} + if version != "" { + params.Meta = mcp.Meta{mcp.MetaKeyProtocolVersion: version} + } + return &mcp.ListToolsRequest{Params: params} +} + +func TestOutputSchemaVersionGate(t *testing.T) { + bareAnyOf := json.RawMessage(`{"anyOf":[{"type":"array"},{"type":"object"}]}`) + objectRoot := json.RawMessage(`{"type":"object"}`) + + newResult := func() *mcp.ListToolsResult { + return &mcp.ListToolsResult{Tools: []*mcp.Tool{ + {Name: "object_root", OutputSchema: objectRoot}, + {Name: "bare_anyof", OutputSchema: bareAnyOf}, + }} + } + + tests := []struct { + name string + version string + wantAnyOfStopped bool + }{ + {"2026-07-28 gets the non-object root", "2026-07-28", false}, + {"a later revision also gets it", "2027-01-01", false}, + {"2025-11-25 does not", "2025-11-25", true}, + {"2025-06-18 does not", "2025-06-18", true}, + {"unknown version fails closed", "", true}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var handled string + next := func(_ context.Context, method string, _ mcp.Request) (mcp.Result, error) { + handled = method + return newResult(), nil + } + res, err := OutputSchemaVersionGate()(next)(context.Background(), "tools/list", listToolsReqAtVersion(tt.version)) + require.NoError(t, err) + require.Equal(t, "tools/list", handled) + + list, ok := res.(*mcp.ListToolsResult) + require.True(t, ok) + assert.Equal(t, objectRoot, list.Tools[0].OutputSchema, "object roots are never gated") + if tt.wantAnyOfStopped { + assert.Nil(t, list.Tools[1].OutputSchema) + } else { + assert.Equal(t, bareAnyOf, list.Tools[1].OutputSchema) + } + }) + } +} + +func TestOutputSchemaVersionGateIgnoresOtherMethods(t *testing.T) { + called := false + next := func(_ context.Context, _ string, _ mcp.Request) (mcp.Result, error) { + called = true + return &mcp.CallToolResult{}, nil + } + // A non-tools/list method must pass straight through, including its + // result type, which is not a ListToolsResult. + res, err := OutputSchemaVersionGate()(next)(context.Background(), "tools/call", listToolsReqAtVersion("2025-11-25")) + require.NoError(t, err) + assert.True(t, called) + _, ok := res.(*mcp.CallToolResult) + assert.True(t, ok) +} diff --git a/pkg/inventory/output_schema_registration_test.go b/pkg/inventory/output_schema_registration_test.go new file mode 100644 index 0000000000..877a03e974 --- /dev/null +++ b/pkg/inventory/output_schema_registration_test.go @@ -0,0 +1,145 @@ +package inventory + +import ( + "context" + "encoding/json" + "slices" + "testing" + + "github.com/modelcontextprotocol/go-sdk/mcp" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// toolReturning builds a ServerTool whose handler always emits body as its +// single text block. +func toolReturning(name, body string) ServerTool { + return NewServerTool( + mcp.Tool{Name: name, InputSchema: json.RawMessage(`{"type":"object","properties":{}}`)}, + testToolsetMetadata("toolset1"), + func(context.Context, *mcp.CallToolRequest) (*mcp.CallToolResult, error) { + return &mcp.CallToolResult{Content: []mcp.Content{&mcp.TextContent{Text: body}}}, nil + }, + ) +} + +// connectToInventory registers the inventory on a server and returns a +// connected client session. +func connectToInventory(t *testing.T, inv *Inventory) *mcp.ClientSession { + t.Helper() + ctx := context.Background() + + srv := mcp.NewServer(&mcp.Implementation{Name: "test-server"}, nil) + inv.RegisterTools(ctx, srv, nil) + + st, ct := mcp.NewInMemoryTransports() + type result struct { + session *mcp.ClientSession + err error + } + ch := make(chan result, 1) + go func() { + s, err := mcp.NewClient(&mcp.Implementation{Name: "test-client"}, nil).Connect(ctx, ct, nil) + ch <- result{s, err} + }() + serverSession, err := srv.Connect(ctx, st, nil) + require.NoError(t, err) + t.Cleanup(func() { _ = serverSession.Close() }) + + got := <-ch + require.NoError(t, got.err) + t.Cleanup(func() { _ = got.session.Close() }) + return got.session +} + +func featureCheckerFor(flags ...string) FeatureFlagChecker { + return func(_ context.Context, flag string) (bool, error) { + return slices.Contains(flags, flag), nil + } +} + +// The whole feature is opt-in: with the flag off, the advertised tool surface +// and the call result must be exactly what they were before output schemas +// existed. +func TestOutputSchemaRegistrationIsFeatureGated(t *testing.T) { + schema := json.RawMessage(`{"type":"object","properties":{"id":{"type":"integer"}}}`) + + tests := []struct { + name string + checker FeatureFlagChecker + wantSchema bool + wantStructured bool + }{ + {"flag off", nil, false, false}, + {"unrelated flag on", featureCheckerFor("something_else"), false, false}, + {"flag on", featureCheckerFor(outputSchemasFeatureFlag), true, true}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + tool := toolReturning("t", `{"id":1}`).WithOutputSchema(schema) + inv := mustBuild(t, NewBuilder().SetTools([]ServerTool{tool}). + WithToolsets([]string{"all"}).WithFeatureChecker(tt.checker)) + session := connectToInventory(t, inv) + ctx := context.Background() + + listed, err := session.ListTools(ctx, nil) + require.NoError(t, err) + require.Len(t, listed.Tools, 1) + if tt.wantSchema { + require.NotNil(t, listed.Tools[0].OutputSchema) + } else { + require.Nil(t, listed.Tools[0].OutputSchema) + } + + called, err := session.CallTool(ctx, &mcp.CallToolParams{Name: "t"}) + require.NoError(t, err) + if tt.wantStructured { + require.NotNil(t, called.StructuredContent, "a schema'd tool should mirror its text into structuredContent") + } else { + require.Nil(t, called.StructuredContent) + } + + // The text block is identical either way — this feature never + // changes what an existing client reads. + require.Len(t, called.Content, 1) + text, ok := called.Content[0].(*mcp.TextContent) + require.True(t, ok) + assert.JSONEq(t, `{"id":1}`, text.Text) + }) + } +} + +// A tool that declares no schema must be completely untouched even with the +// feature on — that is what keeps the other ~120 tools byte-identical. +func TestToolsWithoutSchemaAreUnaffected(t *testing.T) { + inv := mustBuild(t, NewBuilder(). + SetTools([]ServerTool{toolReturning("plain", `{"id":1}`)}). + WithToolsets([]string{"all"}). + WithFeatureChecker(featureCheckerFor(outputSchemasFeatureFlag))) + session := connectToInventory(t, inv) + ctx := context.Background() + + listed, err := session.ListTools(ctx, nil) + require.NoError(t, err) + assert.Nil(t, listed.Tools[0].OutputSchema) + + called, err := session.CallTool(ctx, &mcp.CallToolParams{Name: "plain"}) + require.NoError(t, err) + assert.Nil(t, called.StructuredContent) +} + +// Registration must never write the schema back onto the shared ServerTool, or +// a second registration with the feature off would still carry it. +func TestRegistrationDoesNotMutateTheInventory(t *testing.T) { + schema := json.RawMessage(`{"type":"object"}`) + tool := toolReturning("t", `{}`).WithOutputSchema(schema) + inv := mustBuild(t, NewBuilder().SetTools([]ServerTool{tool}). + WithToolsets([]string{"all"}). + WithFeatureChecker(featureCheckerFor(outputSchemasFeatureFlag))) + + connectToInventory(t, inv) + assert.Nil(t, inv.AllTools()[0].Tool.OutputSchema, + "the schema belongs on ServerTool.OutputSchema until registration copies it onto a duplicate") + assert.Equal(t, schema, inv.AllTools()[0].OutputSchema) +} diff --git a/pkg/inventory/registry.go b/pkg/inventory/registry.go index 915ed0aa1c..713fd49112 100644 --- a/pkg/inventory/registry.go +++ b/pkg/inventory/registry.go @@ -219,9 +219,14 @@ func shouldStripMCPAppsMetadata(ctx context.Context, featureFlagEnabled bool) bo // user identity from ctx would otherwise see context.Background() and // falsely report the flag off, even when the actual request arrived on the // /insiders route. +// Output schemas are likewise attached only when the output_schemas feature +// flag is enabled for this request, for the same per-request-context reason. func (r *Inventory) RegisterTools(ctx context.Context, s *mcp.Server, deps any, middleware ...ToolHandlerMiddleware) { + opts := RegisterToolOptions{ + IncludeOutputSchema: r.checkFeatureFlag(ctx, outputSchemasFeatureFlag), + } for _, tool := range r.ToolsForRegistration(ctx) { - tool.RegisterFunc(s, deps, middleware...) + tool.RegisterFuncWithOptions(s, deps, opts, middleware...) } } diff --git a/pkg/inventory/server_tool.go b/pkg/inventory/server_tool.go index 44a062ba2e..e2d5c6a3d6 100644 --- a/pkg/inventory/server_tool.go +++ b/pkg/inventory/server_tool.go @@ -58,6 +58,21 @@ type ServerTool struct { // Tool is the MCP tool definition containing name, description, schema, etc. Tool mcp.Tool + // OutputSchema is copied onto Tool.OutputSchema at registration time, but + // only when the output_schemas feature is enabled. Keeping it off + // Tool.OutputSchema until registration means the default tool surface — + // and the committed toolsnaps — stay unchanged for clients that have not + // opted in. + // + // Any value that JSON-marshals to a valid JSON Schema is accepted; + // json.RawMessage is preferred for hand-authored schemas because it + // round-trips byte-for-byte and keeps $defs/anyOf/$ref exactly as written. + // + // A schema whose root is not `{"type":"object"}` is only legal from + // protocol version 2026-07-28 onward (SEP-2106); such schemas are stripped + // per-request for older clients. See NonObjectRootOutputSchema. + OutputSchema any + // Toolset contains metadata about which toolset this tool belongs to. Toolset ToolsetMetadata @@ -110,17 +125,52 @@ func (st *ServerTool) Handler(deps any) mcp.ToolHandler { return st.HandlerFunc(deps) } +// WithOutputSchema returns a copy of the tool carrying a feature-gated output +// schema. Chainable off the NewTool constructors at tool definition sites. +func (st ServerTool) WithOutputSchema(schema any) ServerTool { + st.OutputSchema = schema + return st +} + +// RegisterToolOptions controls optional, feature-gated registration behaviour. +type RegisterToolOptions struct { + // IncludeOutputSchema attaches ServerTool.OutputSchema to the registered + // tool. Off by default so the tool surface is unchanged unless the + // output_schemas feature is enabled. + IncludeOutputSchema bool +} + // RegisterFunc registers the tool with the server using the provided dependencies. // Icons are automatically applied from the toolset metadata if not already set. // A shallow copy of the tool is made to avoid mutating the original ServerTool. // Panics if the tool has no handler - all tools should have handlers. func (st *ServerTool) RegisterFunc(s *mcp.Server, deps any, middleware ...ToolHandlerMiddleware) { + st.RegisterFuncWithOptions(s, deps, RegisterToolOptions{}, middleware...) +} + +// RegisterFuncWithOptions is RegisterFunc with feature-gated metadata applied. +func (st *ServerTool) RegisterFuncWithOptions(s *mcp.Server, deps any, opts RegisterToolOptions, middleware ...ToolHandlerMiddleware) { handler := st.Handler(deps) // This will panic if HandlerFunc is nil + // Mirror the serialized text result into structuredContent for tools that + // declare an output schema. Wrapped before the caller's middleware so that + // middleware still sees, and can rewrite, the final result. + if opts.IncludeOutputSchema && st.OutputSchema != nil { + handler = mirrorStructuredContent(handler) + } for i := len(middleware) - 1; i >= 0; i-- { handler = middleware[i](handler) } // Make a shallow copy of the tool to avoid mutating the original toolCopy := st.Tool + // Attach the output schema only when the feature is on. The else branch is + // not redundant: Tool literals never set OutputSchema themselves, but + // clearing it here keeps the invariant explicit and makes an accidental + // literal assignment fail closed rather than leak to every client. + if opts.IncludeOutputSchema { + toolCopy.OutputSchema = st.OutputSchema + } else { + toolCopy.OutputSchema = nil + } // Apply icons from toolset metadata if tool doesn't have icons set if len(toolCopy.Icons) == 0 { toolCopy.Icons = st.Toolset.Icons() diff --git a/pkg/inventory/structured_mirror.go b/pkg/inventory/structured_mirror.go new file mode 100644 index 0000000000..d843791cc9 --- /dev/null +++ b/pkg/inventory/structured_mirror.go @@ -0,0 +1,83 @@ +package inventory + +import ( + "context" + "encoding/json" + + "github.com/modelcontextprotocol/go-sdk/mcp" +) + +// mirrorStructuredContent wraps a tool handler so that a tool which declares an +// OutputSchema also returns structuredContent, without the handler having to +// produce it separately. +// +// This exploits the relationship the spec already defines between the two +// fields: "For backwards compatibility, a tool that returns structured content +// SHOULD also return the serialized JSON in a TextContent block." Every tool in +// this package already emits exactly that — one text block holding +// json.Marshal of the result — so the structured value is recoverable from it +// exactly, with no re-marshaling and therefore no risk of changing number +// formatting or key order. +// +// The mirrored value is the raw bytes of the text block, so content and +// structuredContent are byte-identical by construction and cannot drift. +// +// It applies only to tools that declared an OutputSchema. Tools without one are +// untouched, so the wire format of the other ~120 tools is unchanged. +func mirrorStructuredContent(next mcp.ToolHandler) mcp.ToolHandler { + return func(ctx context.Context, req *mcp.CallToolRequest) (*mcp.CallToolResult, error) { + res, err := next(ctx, req) + if err != nil || res == nil { + return res, err + } + // Never attach structured content to an error result: outputSchema + // describes the success shape, and a client validating an error + // payload against it would fail. + if res.IsError || res.StructuredContent != nil { + return res, err + } + raw, ok := singleJSONTextBlock(res) + if !ok { + return res, err + } + // Under 2025-11-25 and earlier, structuredContent is typed as a JSON + // object. Only send a non-object value to a client that can represent + // it. The text block is unaffected either way, so nothing is lost. + if !isJSONObjectBytes(raw) && req.ProtocolVersion() < ProtocolVersionNonObjectOutputSchemas { + return res, err + } + res.StructuredContent = json.RawMessage(raw) + return res, err + } +} + +// singleJSONTextBlock returns the bytes of the result's sole text content when +// that content is valid JSON. Results that are not a single JSON text block — +// raw diffs, logs, file contents, embedded resources, multi-part results — are +// left alone, since there is no structured value to mirror. +func singleJSONTextBlock(res *mcp.CallToolResult) ([]byte, bool) { + if len(res.Content) != 1 { + return nil, false + } + text, ok := res.Content[0].(*mcp.TextContent) + if !ok { + return nil, false + } + raw := []byte(text.Text) + if !json.Valid(raw) { + return nil, false + } + return raw, true +} + +func isJSONObjectBytes(raw []byte) bool { + for _, b := range raw { + switch b { + case ' ', '\t', '\r', '\n': + continue + default: + return b == '{' + } + } + return false +} diff --git a/pkg/inventory/structured_mirror_test.go b/pkg/inventory/structured_mirror_test.go new file mode 100644 index 0000000000..bad8b7ddfc --- /dev/null +++ b/pkg/inventory/structured_mirror_test.go @@ -0,0 +1,116 @@ +package inventory + +import ( + "context" + "encoding/json" + "testing" + + "github.com/modelcontextprotocol/go-sdk/mcp" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func textResult(text string) *mcp.CallToolResult { + return &mcp.CallToolResult{Content: []mcp.Content{&mcp.TextContent{Text: text}}} +} + +func runMirror(t *testing.T, version string, res *mcp.CallToolResult) *mcp.CallToolResult { + t.Helper() + params := &mcp.CallToolParamsRaw{} + if version != "" { + params.Meta = mcp.Meta{mcp.MetaKeyProtocolVersion: version} + } + req := &mcp.CallToolRequest{Params: params} + out, err := mirrorStructuredContent(func(context.Context, *mcp.CallToolRequest) (*mcp.CallToolResult, error) { + return res, nil + })(context.Background(), req) + require.NoError(t, err) + return out +} + +func TestMirrorStructuredContent(t *testing.T) { + tests := []struct { + name string + version string + result *mcp.CallToolResult + want string // expected structuredContent bytes, "" for none + }{ + { + name: "object mirrors on any version", version: "2025-11-25", + result: textResult(`{"number":1,"title":"x"}`), want: `{"number":1,"title":"x"}`, + }, + { + // Non-object structuredContent is only representable from 2026-07-28. + name: "array is withheld from an older client", version: "2025-11-25", + result: textResult(`[{"id":1}]`), want: "", + }, + { + name: "array mirrors on 2026-07-28", version: "2026-07-28", + result: textResult(`[{"id":1}]`), want: `[{"id":1}]`, + }, + { + name: "empty array mirrors on 2026-07-28", version: "2026-07-28", + result: textResult(`[]`), want: `[]`, + }, + { + name: "unknown version withholds a non-object", version: "", + result: textResult(`[]`), want: "", + }, + { + // A raw diff or log body is not structured data. + name: "non-JSON text is left alone", version: "2026-07-28", + result: textResult("diff --git a/x b/x\n@@ -1 +1 @@\n-a\n+b"), want: "", + }, + { + name: "error results never carry structured content", version: "2026-07-28", + result: func() *mcp.CallToolResult { + r := textResult(`{"message":"boom"}`) + r.IsError = true + return r + }(), want: "", + }, + { + name: "multi-part results are left alone", version: "2026-07-28", + result: &mcp.CallToolResult{Content: []mcp.Content{ + &mcp.TextContent{Text: `{"a":1}`}, &mcp.TextContent{Text: `{"b":2}`}, + }}, want: "", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := runMirror(t, tt.version, tt.result) + if tt.want == "" { + assert.Nil(t, got.StructuredContent) + return + } + raw, ok := got.StructuredContent.(json.RawMessage) + require.True(t, ok, "structured content should be raw bytes, not a re-marshaled value") + assert.JSONEq(t, tt.want, string(raw)) + }) + } +} + +// The mirror must not overwrite a value a handler set deliberately. +func TestMirrorStructuredContentPreservesExplicitValue(t *testing.T) { + res := textResult(`{"from":"text"}`) + res.StructuredContent = map[string]any{"from": "handler"} + got := runMirror(t, "2026-07-28", res) + assert.Equal(t, map[string]any{"from": "handler"}, got.StructuredContent) +} + +// Mirroring must be byte-exact: no re-marshaling, so large integers and number +// formatting survive untouched. A round-trip through any/float64 would not. +func TestMirrorStructuredContentIsByteExact(t *testing.T) { + const body = `{"id":9007199254740993,"ratio":1.10,"pad":"0042"}` + got := runMirror(t, "2026-07-28", textResult(body)) + raw, ok := got.StructuredContent.(json.RawMessage) + require.True(t, ok) + assert.Equal(t, body, string(raw), "structured content must be the text block's exact bytes") + + // And it survives serialization to the wire unchanged. + encoded, err := json.Marshal(map[string]any{"structuredContent": got.StructuredContent}) + require.NoError(t, err) + assert.Contains(t, string(encoded), `9007199254740993`) + assert.Contains(t, string(encoded), `1.10`) +} From 23278a9cf6d01a5c7e3ea0c00db293aac33189f0 Mon Sep 17 00:00:00 2001 From: olaservo Date: Sat, 25 Jul 2026 14:33:47 -0700 Subject: [PATCH 2/6] feat: drop redundant text content for clients that read structuredContent MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds `structured_content_only`, which turns the main cost of output schemas into a saving. With `output_schemas` alone, a schema-bearing tool sends its payload twice: once as the serialized-JSON text block and once as structuredContent. That duplication works against csv_output, minimal_output and the fields param, all of which exist to make responses smaller. The spec's "a tool that returns structured content SHOULD also return the serialized JSON in a TextContent block" is a backwards-compatibility clause. The Go SDK is explicit about this where it synthesises that block on its typed path: the fallback exists "so that pre-SEP-2106 clients can recover the structured payload from unstructured content". A client that negotiated 2026-07-28 is not such a client, so for those the block is pure duplication. Measured on a small payload (TestStructuredContentOnlyRoughlyHalvesTheResult): no output schema 272 bytes output_schemas 405 bytes (+49%) output_schemas,structured_content_only 254 bytes (-7%) The result is smaller than having no output schema at all, because a text block embeds JSON as an escaped string — every quote becomes \" — while structuredContent carries it raw. The saving grows with payload size. `content` stays present as an empty array: the draft schema still lists it in CallToolResult's required set, and the SDK normalises an empty slice to `[]` rather than `null`. The text is kept whenever it is not genuinely redundant: older or unknown protocol version, error results, non-JSON or multi-part results (raw diffs, logs, CSV-converted output), and results where the handler set structuredContent itself — in that last case the server did not author the text/structured pairing and cannot assume the two match. Deliberately a separate opt-in, conjoined with output_schemas rather than independent. Negotiating 2026-07-28 does not prove a client reads structuredContent — no capability advertises it — and a client that ignored it would see an empty result. Co-Authored-By: Claude Opus 5 --- docs/feature-flags.md | 29 +++++++ pkg/github/feature_flags.go | 17 ++++ pkg/inventory/builder.go | 6 ++ .../output_schema_registration_test.go | 74 +++++++++++++++++ pkg/inventory/registry.go | 8 +- pkg/inventory/server_tool.go | 10 ++- pkg/inventory/structured_mirror.go | 40 ++++++++- pkg/inventory/structured_mirror_test.go | 81 ++++++++++++++++++- 8 files changed, 259 insertions(+), 6 deletions(-) diff --git a/docs/feature-flags.md b/docs/feature-flags.md index e80cb69fb5..052425737e 100644 --- a/docs/feature-flags.md +++ b/docs/feature-flags.md @@ -49,6 +49,35 @@ Schemas use `anyOf`, never `oneOf`. `oneOf` requires exactly one branch to match --- +## `structured_content_only` + +Requires `output_schemas`. On its own it does nothing. + +With `output_schemas` alone, a schema-bearing tool sends its payload **twice** — once as the serialized-JSON text block and once as `structuredContent`. That is what the spec asks for by default, but the reason is backwards compatibility: the text block exists so that clients which cannot read `structuredContent` can still recover the payload. + +A client that negotiated 2026-07-28 is not such a client. This flag drops the redundant text block for those clients, so the payload travels once: + +| Configuration | Serialized result | +|---------------|-------------------| +| no output schema | 272 bytes | +| `output_schemas` | 405 bytes (+49%) | +| `output_schemas,structured_content_only` | 254 bytes (−7%) | + +(Measured by `TestStructuredContentOnlyRoughlyHalvesTheResult` on a small payload.) The result is *smaller* than having no output schema at all, because a text block embeds JSON as an escaped string — every `"` becomes `\"` — while `structuredContent` carries it raw. The saving grows with payload size. + +`content` remains present as an empty array, since the schema still requires the field. + +The text block is kept, and nothing is dropped, whenever it is not genuinely redundant: + +- the client negotiated anything older than 2026-07-28, or its version could not be determined +- the result is an error — error messages are never dropped +- the tool returned something other than a single JSON text block (a raw diff, logs, file contents, a CSV-converted result, or a multi-part result) +- the handler set `structuredContent` itself, so the server did not author the text/structured pairing and cannot assume they match + +This is a separate opt-in rather than automatic behaviour because negotiating 2026-07-28 does not *prove* a client reads `structuredContent` — no capability advertises it, and a client that ignored it would see an empty result. Enable it where the consumer is known to read structured output; code-execution hosts are the motivating case. + +--- + ## Tools affected by each flag The list below is regenerated from the Go source. For each user-controllable diff --git a/pkg/github/feature_flags.go b/pkg/github/feature_flags.go index a40027425d..f0d8649a73 100644 --- a/pkg/github/feature_flags.go +++ b/pkg/github/feature_flags.go @@ -47,6 +47,22 @@ const FeatureFlagFieldsParam = "fields_param" // inventory.OutputSchemaVersionGate. const FeatureFlagOutputSchemas = "output_schemas" +// FeatureFlagStructuredContentOnly drops the serialized-JSON text block from +// results whose structuredContent already carries the identical bytes, for +// clients speaking protocol 2026-07-28 or later. Without it, a schema-bearing +// tool sends the same JSON twice; with it, such a response is roughly halved. +// +// It requires FeatureFlagOutputSchemas — on its own it does nothing, because +// without a declared schema no structuredContent is produced to replace the +// text. +// +// This is a separate opt-in rather than automatic behaviour: negotiating +// 2026-07-28 does not prove a client actually reads structuredContent (there +// is no capability that advertises it), and a client that ignored it would +// see an empty result. Enable it only where the consumer is known to read +// structured output — code-execution hosts being the motivating case. +const FeatureFlagStructuredContentOnly = "structured_content_only" + // AllowedFeatureFlags is the allowlist of feature flags that can be enabled // by users via --features CLI flag or X-MCP-Features HTTP header. // Only flags in this list are accepted; unknown flags are silently ignored. @@ -62,6 +78,7 @@ var AllowedFeatureFlags = []string{ FeatureFlagIssueDependencies, FeatureFlagFieldsParam, FeatureFlagOutputSchemas, + FeatureFlagStructuredContentOnly, } // InsidersFeatureFlags is the list of feature flags that insiders mode enables. diff --git a/pkg/inventory/builder.go b/pkg/inventory/builder.go index 12797152d9..ae0c90cfb4 100644 --- a/pkg/inventory/builder.go +++ b/pkg/inventory/builder.go @@ -25,6 +25,12 @@ const ( // and return structuredContent. The value must match // github.FeatureFlagOutputSchemas. outputSchemasFeatureFlag = "output_schemas" + + // structuredContentOnlyFeatureFlag controls whether the now-duplicated + // serialized-JSON text block is dropped for clients that can read + // structuredContent. The value must match + // github.FeatureFlagStructuredContentOnly. + structuredContentOnlyFeatureFlag = "structured_content_only" ) // ToolFilter is a function that determines if a tool should be included. diff --git a/pkg/inventory/output_schema_registration_test.go b/pkg/inventory/output_schema_registration_test.go index 877a03e974..8dba4c0dfd 100644 --- a/pkg/inventory/output_schema_registration_test.go +++ b/pkg/inventory/output_schema_registration_test.go @@ -143,3 +143,77 @@ func TestRegistrationDoesNotMutateTheInventory(t *testing.T) { "the schema belongs on ServerTool.OutputSchema until registration copies it onto a duplicate") assert.Equal(t, schema, inv.AllTools()[0].OutputSchema) } + +// structured_content_only is meaningless alone: with no schema there is no +// structuredContent to replace the text with, so stripping it would leave the +// client with nothing. +func TestStructuredContentOnlyRequiresOutputSchemas(t *testing.T) { + // A payload big enough that halving it is visible. + body := `{"number":1,"title":"a reasonably long issue title","state":"open"}` + + tests := []struct { + name string + flags []string + wantText bool + }{ + {"neither flag", nil, true}, + {"only structured_content_only", []string{structuredContentOnlyFeatureFlag}, true}, + {"only output_schemas", []string{outputSchemasFeatureFlag}, true}, + {"both", []string{outputSchemasFeatureFlag, structuredContentOnlyFeatureFlag}, false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + tool := toolReturning("t", body). + WithOutputSchema(json.RawMessage(`{"type":"object"}`)) + inv := mustBuild(t, NewBuilder().SetTools([]ServerTool{tool}). + WithToolsets([]string{"all"}). + WithFeatureChecker(featureCheckerFor(tt.flags...))) + + called, err := connectToInventory(t, inv). + CallTool(context.Background(), &mcp.CallToolParams{Name: "t"}) + require.NoError(t, err) + + if tt.wantText { + require.Len(t, called.Content, 1, "the payload must be reachable somewhere") + assert.JSONEq(t, body, called.Content[0].(*mcp.TextContent).Text) + } else { + assert.Empty(t, called.Content, "text is redundant once structuredContent carries it") + require.NotNil(t, called.StructuredContent, "...but only because structuredContent has it") + } + }) + } +} + +// Substantiates the claim: the same call carries the payload once rather than +// twice. Measures the serialized result, which is what actually crosses the +// wire. The in-memory client negotiates the SDK's latest version (2026-07-28), +// so this exercises the new-client path. +func TestStructuredContentOnlyRoughlyHalvesTheResult(t *testing.T) { + body := `{"number":1,"title":"a reasonably long issue title","state":"open","html_url":"https://github.com/o/r/issues/1"}` + + sizeWith := func(flags ...string) int { + tool := toolReturning("t", body).WithOutputSchema(json.RawMessage(`{"type":"object"}`)) + inv := mustBuild(t, NewBuilder().SetTools([]ServerTool{tool}). + WithToolsets([]string{"all"}).WithFeatureChecker(featureCheckerFor(flags...))) + called, err := connectToInventory(t, inv). + CallTool(context.Background(), &mcp.CallToolParams{Name: "t"}) + require.NoError(t, err) + encoded, err := json.Marshal(called) + require.NoError(t, err) + return len(encoded) + } + + doubled := sizeWith(outputSchemasFeatureFlag) + once := sizeWith(outputSchemasFeatureFlag, structuredContentOnlyFeatureFlag) + baseline := sizeWith() + + t.Logf("baseline (no schema): %d bytes", baseline) + t.Logf("output_schemas only: %d bytes (%+d vs baseline)", doubled, doubled-baseline) + t.Logf("+ structured_only: %d bytes (%+d vs baseline)", once, once-baseline) + + assert.Greater(t, doubled, baseline, "mirroring alone adds a second copy of the payload") + assert.Less(t, once, doubled, "dropping the redundant text must shrink the result") + assert.Less(t, once, baseline+len(body)/2, + "with the text gone the result should be close to a single copy of the payload") +} diff --git a/pkg/inventory/registry.go b/pkg/inventory/registry.go index 713fd49112..3e7d29cbf7 100644 --- a/pkg/inventory/registry.go +++ b/pkg/inventory/registry.go @@ -222,8 +222,14 @@ func shouldStripMCPAppsMetadata(ctx context.Context, featureFlagEnabled bool) bo // Output schemas are likewise attached only when the output_schemas feature // flag is enabled for this request, for the same per-request-context reason. func (r *Inventory) RegisterTools(ctx context.Context, s *mcp.Server, deps any, middleware ...ToolHandlerMiddleware) { + includeOutputSchema := r.checkFeatureFlag(ctx, outputSchemasFeatureFlag) opts := RegisterToolOptions{ - IncludeOutputSchema: r.checkFeatureFlag(ctx, outputSchemasFeatureFlag), + IncludeOutputSchema: includeOutputSchema, + // Meaningless without a schema to produce structuredContent from, so + // it is deliberately conjoined rather than independent: enabling only + // structured_content_only must never strip text with nothing to + // replace it. + OmitRedundantTextContent: includeOutputSchema && r.checkFeatureFlag(ctx, structuredContentOnlyFeatureFlag), } for _, tool := range r.ToolsForRegistration(ctx) { tool.RegisterFuncWithOptions(s, deps, opts, middleware...) diff --git a/pkg/inventory/server_tool.go b/pkg/inventory/server_tool.go index e2d5c6a3d6..a97de03d94 100644 --- a/pkg/inventory/server_tool.go +++ b/pkg/inventory/server_tool.go @@ -138,6 +138,14 @@ type RegisterToolOptions struct { // tool. Off by default so the tool surface is unchanged unless the // output_schemas feature is enabled. IncludeOutputSchema bool + + // OmitRedundantTextContent drops the serialized-JSON text block from + // results whose structuredContent already carries the identical bytes, + // for clients speaking 2026-07-28 or later. Halves the response for + // schema-bearing tools instead of doubling it. Requires + // IncludeOutputSchema; on its own it does nothing, since without a schema + // no structuredContent is produced to replace the text. + OmitRedundantTextContent bool } // RegisterFunc registers the tool with the server using the provided dependencies. @@ -155,7 +163,7 @@ func (st *ServerTool) RegisterFuncWithOptions(s *mcp.Server, deps any, opts Regi // declare an output schema. Wrapped before the caller's middleware so that // middleware still sees, and can rewrite, the final result. if opts.IncludeOutputSchema && st.OutputSchema != nil { - handler = mirrorStructuredContent(handler) + handler = mirrorStructuredContent(handler, opts.OmitRedundantTextContent) } for i := len(middleware) - 1; i >= 0; i-- { handler = middleware[i](handler) diff --git a/pkg/inventory/structured_mirror.go b/pkg/inventory/structured_mirror.go index d843791cc9..5933bee791 100644 --- a/pkg/inventory/structured_mirror.go +++ b/pkg/inventory/structured_mirror.go @@ -24,7 +24,11 @@ import ( // // It applies only to tools that declared an OutputSchema. Tools without one are // untouched, so the wire format of the other ~120 tools is unchanged. -func mirrorStructuredContent(next mcp.ToolHandler) mcp.ToolHandler { +// +// When omitRedundantText is set, the now-duplicated text block is dropped for +// clients new enough to read structuredContent, halving the response instead of +// doubling it. See dropRedundantTextContent. +func mirrorStructuredContent(next mcp.ToolHandler, omitRedundantText bool) mcp.ToolHandler { return func(ctx context.Context, req *mcp.CallToolRequest) (*mcp.CallToolResult, error) { res, err := next(ctx, req) if err != nil || res == nil { @@ -43,14 +47,46 @@ func mirrorStructuredContent(next mcp.ToolHandler) mcp.ToolHandler { // Under 2025-11-25 and earlier, structuredContent is typed as a JSON // object. Only send a non-object value to a client that can represent // it. The text block is unaffected either way, so nothing is lost. - if !isJSONObjectBytes(raw) && req.ProtocolVersion() < ProtocolVersionNonObjectOutputSchemas { + newEnough := req.ProtocolVersion() >= ProtocolVersionNonObjectOutputSchemas + if !isJSONObjectBytes(raw) && !newEnough { return res, err } res.StructuredContent = json.RawMessage(raw) + if omitRedundantText && newEnough { + dropRedundantTextContent(res) + } return res, err } } +// dropRedundantTextContent removes the text block that structuredContent now +// duplicates byte-for-byte. +// +// The spec's "a tool that returns structured content SHOULD also return the +// serialized JSON in a TextContent block" is a backwards-compatibility clause. +// The Go SDK says as much where it synthesises that block on its typed path: +// the fallback exists "so that pre-SEP-2106 clients can recover the structured +// payload from unstructured content" (mcp/server.go). A client that negotiated +// 2026-07-28 is not such a client, so for those the block is pure duplication — +// this server otherwise sends the same JSON twice, which works against +// csv_output, minimal_output and the fields param, all of which exist to make +// responses smaller. +// +// Callers must only reach here when structuredContent was mirrored from this +// exact text, so nothing is lost: the bytes survive, they just travel once. +// +// content stays present as an empty array rather than being unset — the draft +// schema still lists it in CallToolResult's required set, and the SDK +// normalises an empty slice to `[]` rather than `null`. +// +// This is opt-in (see RegisterToolOptions.OmitRedundantTextContent) rather than +// automatic, because negotiating 2026-07-28 does not prove a client actually +// reads structuredContent — there is no capability that says so, and a client +// that ignored it would see an empty result. +func dropRedundantTextContent(res *mcp.CallToolResult) { + res.Content = []mcp.Content{} +} + // singleJSONTextBlock returns the bytes of the result's sole text content when // that content is valid JSON. Results that are not a single JSON text block — // raw diffs, logs, file contents, embedded resources, multi-part results — are diff --git a/pkg/inventory/structured_mirror_test.go b/pkg/inventory/structured_mirror_test.go index bad8b7ddfc..e04bd7dca0 100644 --- a/pkg/inventory/structured_mirror_test.go +++ b/pkg/inventory/structured_mirror_test.go @@ -14,7 +14,7 @@ func textResult(text string) *mcp.CallToolResult { return &mcp.CallToolResult{Content: []mcp.Content{&mcp.TextContent{Text: text}}} } -func runMirror(t *testing.T, version string, res *mcp.CallToolResult) *mcp.CallToolResult { +func runMirrorOpt(t *testing.T, version string, omitRedundantText bool, res *mcp.CallToolResult) *mcp.CallToolResult { t.Helper() params := &mcp.CallToolParamsRaw{} if version != "" { @@ -23,11 +23,16 @@ func runMirror(t *testing.T, version string, res *mcp.CallToolResult) *mcp.CallT req := &mcp.CallToolRequest{Params: params} out, err := mirrorStructuredContent(func(context.Context, *mcp.CallToolRequest) (*mcp.CallToolResult, error) { return res, nil - })(context.Background(), req) + }, omitRedundantText)(context.Background(), req) require.NoError(t, err) return out } +func runMirror(t *testing.T, version string, res *mcp.CallToolResult) *mcp.CallToolResult { + t.Helper() + return runMirrorOpt(t, version, false, res) +} + func TestMirrorStructuredContent(t *testing.T) { tests := []struct { name string @@ -114,3 +119,75 @@ func TestMirrorStructuredContentIsByteExact(t *testing.T) { assert.Contains(t, string(encoded), `9007199254740993`) assert.Contains(t, string(encoded), `1.10`) } + +// The point of the feature: for a client that can read structuredContent, the +// serialized-JSON text block is pure duplication, so the response carries the +// bytes once instead of twice. +func TestMirrorDropsRedundantTextForNewClients(t *testing.T) { + const body = `{"number":1,"title":"x"}` + got := runMirrorOpt(t, "2026-07-28", true, textResult(body)) + + raw, ok := got.StructuredContent.(json.RawMessage) + require.True(t, ok) + assert.Equal(t, body, string(raw), "the payload must survive intact in structuredContent") + + assert.Empty(t, got.Content, "the duplicated text block should be gone") + assert.NotNil(t, got.Content, "content is required by the schema, so it must be [] and not null") +} + +// A client that cannot read structuredContent must keep its text, or the +// result would be empty for it. +func TestMirrorKeepsTextForOlderClients(t *testing.T) { + const body = `{"number":1}` + got := runMirrorOpt(t, "2025-11-25", true, textResult(body)) + + require.Len(t, got.Content, 1, "an older client must still get the text block") + assert.Equal(t, body, got.Content[0].(*mcp.TextContent).Text) + require.NotNil(t, got.StructuredContent, "an object is still legal structuredContent for it") +} + +func TestMirrorKeepsTextWhenVersionUnknown(t *testing.T) { + got := runMirrorOpt(t, "", true, textResult(`{"a":1}`)) + require.Len(t, got.Content, 1, "unknown version must fail safe and keep the text") +} + +// Text is only dropped when structuredContent actually replaced it. If the +// mirror declined for any reason, the text is the only copy and must survive. +func TestMirrorNeverDropsTextItDidNotReplace(t *testing.T) { + t.Run("non-JSON text", func(t *testing.T) { + got := runMirrorOpt(t, "2026-07-28", true, textResult("diff --git a/x b/x")) + require.Len(t, got.Content, 1) + assert.Nil(t, got.StructuredContent) + }) + + t.Run("error result", func(t *testing.T) { + res := textResult(`{"message":"boom"}`) + res.IsError = true + got := runMirrorOpt(t, "2026-07-28", true, res) + require.Len(t, got.Content, 1, "an error message must never be silently dropped") + assert.Nil(t, got.StructuredContent) + }) + + t.Run("handler set its own structured content", func(t *testing.T) { + res := textResult(`{"from":"text"}`) + res.StructuredContent = map[string]any{"from": "handler"} + got := runMirrorOpt(t, "2026-07-28", true, res) + require.Len(t, got.Content, 1, + "the mirror did not author this pairing, so it must not assume the text is redundant") + }) + + t.Run("multi-part result", func(t *testing.T) { + res := &mcp.CallToolResult{Content: []mcp.Content{ + &mcp.TextContent{Text: `{"a":1}`}, &mcp.TextContent{Text: `{"b":2}`}, + }} + got := runMirrorOpt(t, "2026-07-28", true, res) + require.Len(t, got.Content, 2) + }) +} + +// Opting out leaves the doubled-payload behaviour untouched. +func TestMirrorKeepsTextWhenOptionDisabled(t *testing.T) { + got := runMirrorOpt(t, "2026-07-28", false, textResult(`{"a":1}`)) + require.Len(t, got.Content, 1) + require.NotNil(t, got.StructuredContent) +} From 7e3327522b26cc7fad14140075a6b11faa2506fe Mon Sep 17 00:00:00 2001 From: olaservo Date: Sat, 25 Jul 2026 15:30:44 -0700 Subject: [PATCH 3/6] fix: make output schema branches non-vacuous with anyOf-over-required MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four of the seven schemas accepted arbitrary objects, so the unions documented and validated nothing: actions_get accepted {} and {"junk":1} actions_list accepted {} and {"junk":1} issue_read accepted [{}] and [{"junk":1}] pull_request_read accepted {} The cause is that these branches describe go-github structs whose every field is omitempty, leaving no required key anywhere. JSON Schema 2020-12 has no dedicated "at least one of these keys" keyword; the idiom is anyOf over singleton required: {"anyOf":[{"required":["id"]},{"required":["number"]}, ...]} which reads as "must carry at least one recognised field". It is the right tool here precisely because it says nothing about UNKNOWN properties: a go-github bump that adds a struct field cannot make the server emit a key its own schema forbids. That is also why additionalProperties:false is now removed from pull_request_read's get_status branch and its repoStatus $def. It achieved non-vacuity but coupled the schema to go-github's exact field set. get_status needed one extra step, because an all-nil *github.CombinedStatus (every field pointer+omitempty) really does marshal to {}. Rather than leave {} globally acceptable, that branch is anyOf over a documented {"maxProperties":0} arm plus the singleton required arms, so {} stays valid only through the branch that can actually produce it while {"junk":1} is rejected. For array branches the constraint goes inside "items", so an empty array still validates — get_comments on an issue with no comments returns [], and that must keep working. Adds three permanent guards: - TestPolymorphicOutputSchemasAreNotVacuous, which whitelists the single legitimate {} by tool and requires a justification rather than allowing it silently - TestPolymorphicOutputSchemasDoNotForbidUnknownFields, which keeps additionalProperties:false from creeping back - the existing NeverUseOneOf guard, unchanged Verified that nothing was over-tightened: real handler output still conforms (including sparse issues with every optional absent, and empty collections), unknown fields alongside recognised ones still validate, and [] still validates for every array branch. Co-Authored-By: Claude Opus 5 --- pkg/github/output_schemas/actions_get.json | 374 ++++++++- pkg/github/output_schemas/actions_list.json | 758 +++++++++++++++++- pkg/github/output_schemas/issue_read.json | 41 + .../output_schemas/pull_request_read.json | 107 ++- pkg/github/output_schemas_polymorphic_test.go | 51 +- 5 files changed, 1311 insertions(+), 20 deletions(-) diff --git a/pkg/github/output_schemas/actions_get.json b/pkg/github/output_schemas/actions_get.json index cd2e9fc6f0..27503a7611 100644 --- a/pkg/github/output_schemas/actions_get.json +++ b/pkg/github/output_schemas/actions_get.json @@ -5,7 +5,7 @@ "anyOf": [ { "title": "get_workflow", - "description": "A single GitHub Actions workflow (GET /repos/{owner}/{repo}/actions/workflows/{workflow_id}). All fields are omitted when absent from the API response.", + "description": "A single GitHub Actions workflow (GET /repos/{owner}/{repo}/actions/workflows/{workflow_id}). All fields are omitted when absent from the API response. Every field of the go-github struct is a pointer or slice with `omitempty`, so no key is guaranteed to survive marshalling and a plain `required` list would be wrong. Instead this branch uses anyOf over singleton `required` -- the object must carry at least one recognised key. That rejects `{}` and `{\"junk\":1}` while staying permissive about UNKNOWN keys, so a go-github bump that adds a struct field cannot make the server violate its own schema. The branch list is exactly the property set above, so no sparse-but-real payload can be rejected.", "type": "object", "properties": { "id": { @@ -42,11 +42,63 @@ "badge_url": { "type": "string" } - } + }, + "anyOf": [ + { + "required": [ + "id" + ] + }, + { + "required": [ + "node_id" + ] + }, + { + "required": [ + "name" + ] + }, + { + "required": [ + "path" + ] + }, + { + "required": [ + "state" + ] + }, + { + "required": [ + "created_at" + ] + }, + { + "required": [ + "updated_at" + ] + }, + { + "required": [ + "url" + ] + }, + { + "required": [ + "html_url" + ] + }, + { + "required": [ + "badge_url" + ] + } + ] }, { "title": "get_workflow_run", - "description": "A single workflow run (GET /repos/{owner}/{repo}/actions/runs/{run_id}).", + "description": "A single workflow run (GET /repos/{owner}/{repo}/actions/runs/{run_id}). Every field of the go-github struct is a pointer or slice with `omitempty`, so no key is guaranteed to survive marshalling and a plain `required` list would be wrong. Instead this branch uses anyOf over singleton `required` -- the object must carry at least one recognised key. That rejects `{}` and `{\"junk\":1}` while staying permissive about UNKNOWN keys, so a go-github bump that adds a struct field cannot make the server violate its own schema. The branch list is exactly the property set above, so no sparse-but-real payload can be rejected.", "type": "object", "properties": { "id": { @@ -168,11 +220,188 @@ "$ref": "#/$defs/referencedWorkflow" } } - } + }, + "anyOf": [ + { + "required": [ + "id" + ] + }, + { + "required": [ + "name" + ] + }, + { + "required": [ + "node_id" + ] + }, + { + "required": [ + "head_branch" + ] + }, + { + "required": [ + "head_sha" + ] + }, + { + "required": [ + "path" + ] + }, + { + "required": [ + "run_number" + ] + }, + { + "required": [ + "run_attempt" + ] + }, + { + "required": [ + "event" + ] + }, + { + "required": [ + "display_title" + ] + }, + { + "required": [ + "status" + ] + }, + { + "required": [ + "conclusion" + ] + }, + { + "required": [ + "workflow_id" + ] + }, + { + "required": [ + "check_suite_id" + ] + }, + { + "required": [ + "check_suite_node_id" + ] + }, + { + "required": [ + "url" + ] + }, + { + "required": [ + "html_url" + ] + }, + { + "required": [ + "pull_requests" + ] + }, + { + "required": [ + "created_at" + ] + }, + { + "required": [ + "updated_at" + ] + }, + { + "required": [ + "run_started_at" + ] + }, + { + "required": [ + "jobs_url" + ] + }, + { + "required": [ + "logs_url" + ] + }, + { + "required": [ + "check_suite_url" + ] + }, + { + "required": [ + "artifacts_url" + ] + }, + { + "required": [ + "cancel_url" + ] + }, + { + "required": [ + "rerun_url" + ] + }, + { + "required": [ + "previous_attempt_url" + ] + }, + { + "required": [ + "head_commit" + ] + }, + { + "required": [ + "workflow_url" + ] + }, + { + "required": [ + "repository" + ] + }, + { + "required": [ + "head_repository" + ] + }, + { + "required": [ + "actor" + ] + }, + { + "required": [ + "triggering_actor" + ] + }, + { + "required": [ + "referenced_workflows" + ] + } + ] }, { "title": "get_workflow_job", - "description": "A single workflow job (GET /repos/{owner}/{repo}/actions/jobs/{job_id}).", + "description": "A single workflow job (GET /repos/{owner}/{repo}/actions/jobs/{job_id}). Every field of the go-github struct is a pointer or slice with `omitempty`, so no key is guaranteed to survive marshalling and a plain `required` list would be wrong. Instead this branch uses anyOf over singleton `required` -- the object must carry at least one recognised key. That rejects `{}` and `{\"junk\":1}` while staying permissive about UNKNOWN keys, so a go-github bump that adds a struct field cannot make the server violate its own schema. The branch list is exactly the property set above, so no sparse-but-real payload can be rejected.", "type": "object", "properties": { "id": { @@ -254,7 +483,124 @@ "workflow_name": { "type": "string" } - } + }, + "anyOf": [ + { + "required": [ + "id" + ] + }, + { + "required": [ + "run_id" + ] + }, + { + "required": [ + "run_url" + ] + }, + { + "required": [ + "node_id" + ] + }, + { + "required": [ + "head_branch" + ] + }, + { + "required": [ + "head_sha" + ] + }, + { + "required": [ + "url" + ] + }, + { + "required": [ + "html_url" + ] + }, + { + "required": [ + "status" + ] + }, + { + "required": [ + "conclusion" + ] + }, + { + "required": [ + "created_at" + ] + }, + { + "required": [ + "started_at" + ] + }, + { + "required": [ + "completed_at" + ] + }, + { + "required": [ + "name" + ] + }, + { + "required": [ + "steps" + ] + }, + { + "required": [ + "check_run_url" + ] + }, + { + "required": [ + "labels" + ] + }, + { + "required": [ + "runner_id" + ] + }, + { + "required": [ + "runner_name" + ] + }, + { + "required": [ + "runner_group_id" + ] + }, + { + "required": [ + "runner_group_name" + ] + }, + { + "required": [ + "run_attempt" + ] + }, + { + "required": [ + "workflow_name" + ] + } + ] }, { "title": "download_workflow_run_artifact", @@ -315,7 +661,7 @@ }, { "title": "get_workflow_run_usage", - "description": "Billable time for a workflow run (GET /repos/{owner}/{repo}/actions/runs/{run_id}/timing). Both fields are omitted when the API reports no usage, so an empty object is a valid response.", + "description": "Billable time for a workflow run (GET /repos/{owner}/{repo}/actions/runs/{run_id}/timing). `run_duration_ms` is omitted when the API reports no duration, and `billable` may itself be an empty object when nothing was billable -- but the REST response always carries `billable`, and go-github marshals a present-but-empty map as `\"billable\":{}`, so this branch never marshals to `{}`. The anyOf over singleton `required` below therefore demands at least one of the two keys, without forbidding unknown keys.", "type": "object", "properties": { "billable": { @@ -331,7 +677,19 @@ "run_duration_ms": { "type": "integer" } - } + }, + "anyOf": [ + { + "required": [ + "billable" + ] + }, + { + "required": [ + "run_duration_ms" + ] + } + ] } ], "$defs": { diff --git a/pkg/github/output_schemas/actions_list.json b/pkg/github/output_schemas/actions_list.json index dbd957f934..425b3f23c2 100644 --- a/pkg/github/output_schemas/actions_list.json +++ b/pkg/github/output_schemas/actions_list.json @@ -1,12 +1,24 @@ { "$schema": "https://json-schema.org/draft/2020-12/schema", "type": "object", - "description": "Output of the actions_list tool. The shape depends on the `method` argument: `list_workflows`, `list_workflow_runs` and `list_workflow_run_artifacts` return the GitHub API list envelope directly, while `list_workflow_jobs` nests the envelope under a `jobs` key. Every field of every envelope is optional: go-github tags them `omitempty`, so an empty collection is reported as {\"total_count\":0} with the item array absent rather than as an empty array. Each branch forbids the other branches' discriminator keys so that a payload is checked against the branch it actually belongs to.", + "description": "Output of the actions_list tool. The shape depends on the `method` argument: `list_workflows`, `list_workflow_runs` and `list_workflow_run_artifacts` return the GitHub API list envelope directly, while `list_workflow_jobs` nests the envelope under a `jobs` key. Every field of every envelope is optional: go-github tags them `omitempty`, so an empty collection is reported as {\"total_count\":0} with the item array absent rather than as an empty array. Each branch forbids the other branches' discriminator keys so that a payload is checked against the branch it actually belongs to. Because every field is optional, each branch and each item type also carries an anyOf over singleton `required` — \"at least one recognised field must be present\" — so that the union is not vacuous. That idiom deliberately says nothing about UNKNOWN fields, so a go-github bump that adds a struct field cannot make the server emit a payload its own schema rejects; additionalProperties:false would, and is never used here.", "anyOf": [ { "title": "list_workflows", "description": "method=list_workflows. Serialized github.Workflows.", "type": "object", + "anyOf": [ + { + "required": [ + "total_count" + ] + }, + { + "required": [ + "workflows" + ] + } + ], "properties": { "total_count": { "type": "integer", @@ -27,6 +39,18 @@ "title": "list_workflow_runs", "description": "method=list_workflow_runs. Serialized github.WorkflowRuns.", "type": "object", + "anyOf": [ + { + "required": [ + "total_count" + ] + }, + { + "required": [ + "workflow_runs" + ] + } + ], "properties": { "total_count": { "type": "integer", @@ -56,6 +80,22 @@ "object", "null" ], + "description": "The github.Jobs envelope, or null when go-github left the *Jobs pointer nil (empty response body). When it is an object it carries at least one recognised field, or is empty; maxProperties:0 keeps the genuinely-empty envelope reachable while still rejecting an object made only of unrecognised keys.", + "anyOf": [ + { + "required": [ + "total_count" + ] + }, + { + "required": [ + "jobs" + ] + }, + { + "maxProperties": 0 + } + ], "properties": { "total_count": { "type": "integer", @@ -78,6 +118,18 @@ "title": "list_workflow_run_artifacts", "description": "method=list_workflow_run_artifacts. Serialized github.ArtifactList.", "type": "object", + "anyOf": [ + { + "required": [ + "total_count" + ] + }, + { + "required": [ + "artifacts" + ] + } + ], "properties": { "total_count": { "type": "integer", @@ -99,6 +151,58 @@ "workflow": { "type": "object", "description": "A repository workflow definition (github.Workflow).", + "anyOf": [ + { + "required": [ + "id" + ] + }, + { + "required": [ + "node_id" + ] + }, + { + "required": [ + "name" + ] + }, + { + "required": [ + "path" + ] + }, + { + "required": [ + "state" + ] + }, + { + "required": [ + "created_at" + ] + }, + { + "required": [ + "updated_at" + ] + }, + { + "required": [ + "url" + ] + }, + { + "required": [ + "html_url" + ] + }, + { + "required": [ + "badge_url" + ] + } + ], "properties": { "id": { "type": "integer" @@ -139,6 +243,183 @@ "workflowRun": { "type": "object", "description": "A single workflow run (github.WorkflowRun).", + "anyOf": [ + { + "required": [ + "id" + ] + }, + { + "required": [ + "name" + ] + }, + { + "required": [ + "node_id" + ] + }, + { + "required": [ + "head_branch" + ] + }, + { + "required": [ + "head_sha" + ] + }, + { + "required": [ + "path" + ] + }, + { + "required": [ + "run_number" + ] + }, + { + "required": [ + "run_attempt" + ] + }, + { + "required": [ + "event" + ] + }, + { + "required": [ + "display_title" + ] + }, + { + "required": [ + "status" + ] + }, + { + "required": [ + "conclusion" + ] + }, + { + "required": [ + "workflow_id" + ] + }, + { + "required": [ + "check_suite_id" + ] + }, + { + "required": [ + "check_suite_node_id" + ] + }, + { + "required": [ + "url" + ] + }, + { + "required": [ + "html_url" + ] + }, + { + "required": [ + "pull_requests" + ] + }, + { + "required": [ + "created_at" + ] + }, + { + "required": [ + "updated_at" + ] + }, + { + "required": [ + "run_started_at" + ] + }, + { + "required": [ + "jobs_url" + ] + }, + { + "required": [ + "logs_url" + ] + }, + { + "required": [ + "check_suite_url" + ] + }, + { + "required": [ + "artifacts_url" + ] + }, + { + "required": [ + "cancel_url" + ] + }, + { + "required": [ + "rerun_url" + ] + }, + { + "required": [ + "previous_attempt_url" + ] + }, + { + "required": [ + "head_commit" + ] + }, + { + "required": [ + "workflow_url" + ] + }, + { + "required": [ + "repository" + ] + }, + { + "required": [ + "head_repository" + ] + }, + { + "required": [ + "actor" + ] + }, + { + "required": [ + "triggering_actor" + ] + }, + { + "required": [ + "referenced_workflows" + ] + } + ], "properties": { "id": { "type": "integer" @@ -197,7 +478,44 @@ "type": "array", "items": { "type": "object", - "description": "Pull request associated with the run (github.PullRequest)." + "description": "Pull request associated with the run (github.PullRequest). The API returns a reduced form of the pull request here, so only the keys it is known to carry are listed; unknown keys are still accepted.", + "anyOf": [ + { + "required": [ + "id" + ] + }, + { + "required": [ + "number" + ] + }, + { + "required": [ + "url" + ] + }, + { + "required": [ + "html_url" + ] + }, + { + "required": [ + "node_id" + ] + }, + { + "required": [ + "head" + ] + }, + { + "required": [ + "base" + ] + } + ] } }, "created_at": { @@ -262,6 +580,123 @@ "workflowJob": { "type": "object", "description": "A single job within a workflow run (github.WorkflowJob).", + "anyOf": [ + { + "required": [ + "id" + ] + }, + { + "required": [ + "run_id" + ] + }, + { + "required": [ + "run_url" + ] + }, + { + "required": [ + "node_id" + ] + }, + { + "required": [ + "head_branch" + ] + }, + { + "required": [ + "head_sha" + ] + }, + { + "required": [ + "url" + ] + }, + { + "required": [ + "html_url" + ] + }, + { + "required": [ + "status" + ] + }, + { + "required": [ + "conclusion" + ] + }, + { + "required": [ + "created_at" + ] + }, + { + "required": [ + "started_at" + ] + }, + { + "required": [ + "completed_at" + ] + }, + { + "required": [ + "name" + ] + }, + { + "required": [ + "steps" + ] + }, + { + "required": [ + "check_run_url" + ] + }, + { + "required": [ + "labels" + ] + }, + { + "required": [ + "runner_id" + ] + }, + { + "required": [ + "runner_name" + ] + }, + { + "required": [ + "runner_group_id" + ] + }, + { + "required": [ + "runner_group_name" + ] + }, + { + "required": [ + "run_attempt" + ] + }, + { + "required": [ + "workflow_name" + ] + } + ], "properties": { "id": { "type": "integer" @@ -349,6 +784,38 @@ "taskStep": { "type": "object", "description": "A step within a workflow job (github.TaskStep).", + "anyOf": [ + { + "required": [ + "name" + ] + }, + { + "required": [ + "status" + ] + }, + { + "required": [ + "conclusion" + ] + }, + { + "required": [ + "number" + ] + }, + { + "required": [ + "started_at" + ] + }, + { + "required": [ + "completed_at" + ] + } + ], "properties": { "name": { "type": "string" @@ -375,6 +842,68 @@ "artifact": { "type": "object", "description": "An artifact produced by a workflow run (github.Artifact).", + "anyOf": [ + { + "required": [ + "id" + ] + }, + { + "required": [ + "node_id" + ] + }, + { + "required": [ + "name" + ] + }, + { + "required": [ + "size_in_bytes" + ] + }, + { + "required": [ + "url" + ] + }, + { + "required": [ + "archive_download_url" + ] + }, + { + "required": [ + "expired" + ] + }, + { + "required": [ + "created_at" + ] + }, + { + "required": [ + "updated_at" + ] + }, + { + "required": [ + "expires_at" + ] + }, + { + "required": [ + "digest" + ] + }, + { + "required": [ + "workflow_run" + ] + } + ], "properties": { "id": { "type": "integer" @@ -421,6 +950,33 @@ "artifactWorkflowRun": { "type": "object", "description": "The workflow run an artifact belongs to (github.ArtifactWorkflowRun).", + "anyOf": [ + { + "required": [ + "id" + ] + }, + { + "required": [ + "repository_id" + ] + }, + { + "required": [ + "head_repository_id" + ] + }, + { + "required": [ + "head_branch" + ] + }, + { + "required": [ + "head_sha" + ] + } + ], "properties": { "id": { "type": "integer" @@ -442,6 +998,23 @@ "referencedWorkflow": { "type": "object", "description": "A reusable workflow referenced by a run (github.ReferencedWorkflow).", + "anyOf": [ + { + "required": [ + "path" + ] + }, + { + "required": [ + "sha" + ] + }, + { + "required": [ + "ref" + ] + } + ], "properties": { "path": { "type": "string" @@ -457,6 +1030,68 @@ "headCommit": { "type": "object", "description": "The head commit of a workflow run (github.HeadCommit).", + "anyOf": [ + { + "required": [ + "id" + ] + }, + { + "required": [ + "sha" + ] + }, + { + "required": [ + "message" + ] + }, + { + "required": [ + "tree_id" + ] + }, + { + "required": [ + "timestamp" + ] + }, + { + "required": [ + "url" + ] + }, + { + "required": [ + "distinct" + ] + }, + { + "required": [ + "author" + ] + }, + { + "required": [ + "committer" + ] + }, + { + "required": [ + "added" + ] + }, + { + "required": [ + "removed" + ] + }, + { + "required": [ + "modified" + ] + } + ], "properties": { "id": { "type": "string" @@ -481,10 +1116,10 @@ "type": "boolean" }, "author": { - "type": "object" + "$ref": "#/$defs/commitAuthor" }, "committer": { - "type": "object" + "$ref": "#/$defs/commitAuthor" }, "added": { "type": "array", @@ -506,9 +1141,92 @@ } } }, + "commitAuthor": { + "type": "object", + "description": "Author or committer of the head commit (github.CommitAuthor). `username` is go-github's rename of the Login field and only appears on webhook-sourced payloads.", + "anyOf": [ + { + "required": [ + "name" + ] + }, + { + "required": [ + "email" + ] + }, + { + "required": [ + "date" + ] + }, + { + "required": [ + "username" + ] + } + ], + "properties": { + "name": { + "type": "string" + }, + "email": { + "type": "string" + }, + "date": { + "type": "string", + "format": "date-time" + }, + "username": { + "type": "string" + } + } + }, "repository": { "type": "object", "description": "Repository the run belongs to (full github.Repository payload; only the most useful fields are described).", + "anyOf": [ + { + "required": [ + "id" + ] + }, + { + "required": [ + "node_id" + ] + }, + { + "required": [ + "name" + ] + }, + { + "required": [ + "full_name" + ] + }, + { + "required": [ + "private" + ] + }, + { + "required": [ + "html_url" + ] + }, + { + "required": [ + "url" + ] + }, + { + "required": [ + "owner" + ] + } + ], "properties": { "id": { "type": "integer" @@ -539,6 +1257,38 @@ "user": { "type": "object", "description": "A GitHub account (full github.User payload; only the most useful fields are described).", + "anyOf": [ + { + "required": [ + "login" + ] + }, + { + "required": [ + "id" + ] + }, + { + "required": [ + "node_id" + ] + }, + { + "required": [ + "type" + ] + }, + { + "required": [ + "avatar_url" + ] + }, + { + "required": [ + "html_url" + ] + } + ], "properties": { "login": { "type": "string" diff --git a/pkg/github/output_schemas/issue_read.json b/pkg/github/output_schemas/issue_read.json index 3b65824c96..42dd4d8f9d 100644 --- a/pkg/github/output_schemas/issue_read.json +++ b/pkg/github/output_schemas/issue_read.json @@ -176,6 +176,47 @@ "type": "array", "items": { "type": "object", + "description": "One sub-issue. Because every go-github Issue field is omitempty there is no single key guaranteed to survive marshalling, so instead of a `required` list this uses anyOf over singleton `required` — the item must carry at least one recognised issue key. That rejects `{}` and `{\"junk\":1}` without forbidding unknown keys, so a go-github bump that adds a struct field cannot make the server violate its own schema. The branch list is exactly the property set below, so no sparse-but-real sub-issue can be rejected.", + "anyOf": [ + { "required": ["id"] }, + { "required": ["node_id"] }, + { "required": ["number"] }, + { "required": ["state"] }, + { "required": ["state_reason"] }, + { "required": ["locked"] }, + { "required": ["active_lock_reason"] }, + { "required": ["title"] }, + { "required": ["body"] }, + { "required": ["draft"] }, + { "required": ["author_association"] }, + { "required": ["user"] }, + { "required": ["assignee"] }, + { "required": ["assignees"] }, + { "required": ["closed_by"] }, + { "required": ["labels"] }, + { "required": ["milestone"] }, + { "required": ["type"] }, + { "required": ["reactions"] }, + { "required": ["pull_request"] }, + { "required": ["repository"] }, + { "required": ["pinned_comment"] }, + { "required": ["performed_via_github_app"] }, + { "required": ["issue_dependencies_summary"] }, + { "required": ["sub_issues_summary"] }, + { "required": ["issue_field_values"] }, + { "required": ["text_matches"] }, + { "required": ["comments"] }, + { "required": ["created_at"] }, + { "required": ["updated_at"] }, + { "required": ["closed_at"] }, + { "required": ["url"] }, + { "required": ["html_url"] }, + { "required": ["comments_url"] }, + { "required": ["events_url"] }, + { "required": ["labels_url"] }, + { "required": ["repository_url"] }, + { "required": ["parent_issue_url"] } + ], "properties": { "id": { "type": "integer" diff --git a/pkg/github/output_schemas/pull_request_read.json b/pkg/github/output_schemas/pull_request_read.json index b3fa1ddb55..00e05c12ac 100644 --- a/pkg/github/output_schemas/pull_request_read.json +++ b/pkg/github/output_schemas/pull_request_read.json @@ -103,7 +103,7 @@ }, { "title": "get_status", - "description": "method=get_status: the GitHub combined commit status for the pull request head SHA, marshalled from *github.CombinedStatus with no minimal-type trimming. Every field is optional, so an empty object is valid. additionalProperties is false because the payload is a fixed Go struct with no custom marshaller: unknown GitHub API fields are dropped at unmarshal time and can never reach the client.", + "description": "method=get_status: the GitHub combined commit status for the pull request head SHA, marshalled from *github.CombinedStatus with no minimal-type trimming. Every field is optional, so an empty object is valid: every field of that struct is a pointer or slice tagged omitempty, so an all-nil value really does marshal to {} and no single key can be required. The anyOf below therefore reads \"empty, or carrying at least one recognised key\" — enough to reject an object made only of unrecognised keys, without forbidding unknown keys the way additionalProperties:false would. Forbidding them would couple this schema to go-github's exact struct layout, so the next version bump that adds a field would make the server emit a key its own schema rejects.", "type": "object", "properties": { "state": { @@ -131,7 +131,47 @@ "type": "string" } }, - "additionalProperties": false + "anyOf": [ + { + "description": "An all-nil *github.CombinedStatus marshals to {}. This branch keeps that reachable payload valid; it is the only reason {} validates against the whole union.", + "maxProperties": 0 + }, + { + "required": [ + "state" + ] + }, + { + "required": [ + "name" + ] + }, + { + "required": [ + "sha" + ] + }, + { + "required": [ + "total_count" + ] + }, + { + "required": [ + "statuses" + ] + }, + { + "required": [ + "commit_url" + ] + }, + { + "required": [ + "repository_url" + ] + } + ] }, { "title": "get_files", @@ -553,7 +593,7 @@ }, "repoStatus": { "type": "object", - "description": "An individual commit status inside the combined status, marshalled from *github.RepoStatus. Like the parent it is a fixed Go struct, so no unlisted fields can appear.", + "description": "An individual commit status inside the combined status, marshalled from *github.RepoStatus. Like the parent it is all pointer fields tagged omitempty, so no key is guaranteed and an all-nil element marshals to {}. Same idiom as the parent: empty, or at least one recognised key. Unknown keys stay allowed so a go-github field addition cannot invalidate real output.", "properties": { "id": { "type": "integer" @@ -589,7 +629,66 @@ "type": "string" } }, - "additionalProperties": false + "anyOf": [ + { + "maxProperties": 0 + }, + { + "required": [ + "id" + ] + }, + { + "required": [ + "node_id" + ] + }, + { + "required": [ + "url" + ] + }, + { + "required": [ + "state" + ] + }, + { + "required": [ + "target_url" + ] + }, + { + "required": [ + "description" + ] + }, + { + "required": [ + "context" + ] + }, + { + "required": [ + "avatar_url" + ] + }, + { + "required": [ + "creator" + ] + }, + { + "required": [ + "created_at" + ] + }, + { + "required": [ + "updated_at" + ] + } + ] } } } diff --git a/pkg/github/output_schemas_polymorphic_test.go b/pkg/github/output_schemas_polymorphic_test.go index 18803266b6..6198ad9592 100644 --- a/pkg/github/output_schemas_polymorphic_test.go +++ b/pkg/github/output_schemas_polymorphic_test.go @@ -80,10 +80,7 @@ func TestPolymorphicOutputSchemaRootKinds(t *testing.T) { func TestPolymorphicOutputSchemasRejectGarbage(t *testing.T) { for tool, schema := range polymorphicSchemas() { t.Run(tool, func(t *testing.T) { - var s jsonschema.Schema - require.NoError(t, json.Unmarshal(schema, &s)) - resolved, err := s.Resolve(nil) - require.NoError(t, err) + resolved := resolveToolSchema(t, schema) assert.Error(t, resolved.Validate("a bare string")) assert.Error(t, resolved.Validate(float64(42))) assert.Error(t, resolved.Validate(nil)) @@ -91,6 +88,52 @@ func TestPolymorphicOutputSchemasRejectGarbage(t *testing.T) { } } +// A branch whose properties are all optional accepts any object at all, which +// makes the whole union vacuous. The fix is anyOf over singleton required +// ("at least one recognised field"), which stays permissive about UNKNOWN +// fields — so a go-github bump that adds one cannot break it — while still +// rejecting objects that look nothing like the documented shape. +// +// Deliberately not additionalProperties:false, which would reject unknown +// fields and couple these schemas to go-github's exact struct layout. +func TestPolymorphicOutputSchemasAreNotVacuous(t *testing.T) { + // An empty object is legitimately reachable for one tool: an all-nil + // *github.CombinedStatus (every field pointer+omitempty) marshals to {}, + // so pull_request_read method=get_status can really return it. Anywhere + // else, {} means the schema is not constraining anything. + emptyObjectIsReal := map[string]bool{"pull_request_read": true} + + for tool, schema := range polymorphicSchemas() { + t.Run(tool, func(t *testing.T) { + resolved := resolveToolSchema(t, schema) + + assert.Error(t, resolved.Validate(map[string]any{"junk": 1}), + "an object with no recognised field must not validate") + assert.Error(t, resolved.Validate([]any{map[string]any{"junk": 1}}), + "an array of unrecognised objects must not validate") + + if !emptyObjectIsReal[tool] { + assert.Error(t, resolved.Validate(map[string]any{}), + "{} must not validate unless a real payload can be empty") + assert.Error(t, resolved.Validate([]any{map[string]any{}}), + "[{}] must not validate unless a real payload can be empty") + } + }) + } +} + +// additionalProperties:false would make these schemas reject payloads the +// server can legally emit the moment go-github adds a struct field. Use +// anyOf-over-required for non-vacuity instead. +func TestPolymorphicOutputSchemasDoNotForbidUnknownFields(t *testing.T) { + for tool, schema := range polymorphicSchemas() { + t.Run(tool, func(t *testing.T) { + assert.NotContains(t, string(schema), `"additionalProperties": false`) + assert.NotContains(t, string(schema), `"additionalProperties":false`) + }) + } +} + func resolveToolSchema(t *testing.T, schema json.RawMessage) *jsonschema.Resolved { t.Helper() var s jsonschema.Schema From 9b7d725d5a25008074bb8de476bf90f55a3e5fe4 Mon Sep 17 00:00:00 2001 From: olaservo Date: Sat, 25 Jul 2026 15:41:40 -0700 Subject: [PATCH 4/6] docs: trim over-long comments on the output schema code Comment-to-code ratio across the four new files was 0.89, with the worst offender being 24 comment lines above a one-line function body. Cuts the "what" and keeps the non-obvious "why": that oneOf is wrong for these unions, that the version gate must copy on write because the SDK shares tool pointers, that dropping text is opt-in because no capability advertises structuredContent support. Much of what went was duplicating docs/feature-flags.md, which did not exist when these were written. Ratio is now 0.48, against 0.62 for pkg/inventory/registry.go and 1.23 for pkg/ifc/ifc.go. Blocks of 7+ lines drop from 12 to 4. No code changed. Co-Authored-By: Claude Opus 5 --- pkg/github/output_schema.go | 73 ++++++++---------------- pkg/github/output_schemas_polymorphic.go | 32 +++-------- pkg/inventory/output_schema_gate.go | 51 ++++++----------- pkg/inventory/structured_mirror.go | 54 +++++------------- 4 files changed, 60 insertions(+), 150 deletions(-) diff --git a/pkg/github/output_schema.go b/pkg/github/output_schema.go index 276786bb23..45ed6fa29b 100644 --- a/pkg/github/output_schema.go +++ b/pkg/github/output_schema.go @@ -16,11 +16,9 @@ import ( // MustOutputSchema infers an output schema for T, panicking during package // initialization if inference fails. // -// Unlike input schemas, an output schema root need not be `{"type":"object"}`: -// from protocol version 2026-07-28 (SEP-2106) it may be any valid JSON Schema -// 2020-12, including a bare array or a bare anyOf. Schemas whose root is not -// an object are stripped per-request for older clients — see -// inventory.OutputSchemaVersionGate — so inferring one here is safe. +// Unlike input schemas, an output schema root need not be `{"type":"object"}`, +// so T may be a slice or a scalar. Non-object roots are stripped per-request +// for pre-2026-07-28 clients by inventory.OutputSchemaVersionGate. func MustOutputSchema[T any]() *jsonschema.Schema { schema, err := jsonschema.For[T](nil) if err != nil { @@ -32,36 +30,21 @@ func MustOutputSchema[T any]() *jsonschema.Schema { // AnyOfSchema builds a union output schema over the given branches. // -// It deliberately emits `anyOf` and never `oneOf`. `oneOf` requires that -// EXACTLY ONE branch match, which is wrong for essentially every tool in this -// package whose output shape varies by method: -// -// - Branches are frequently structurally identical. All four of -// actions_run_trigger's non-run_workflow methods return the same -// {message, run_id, status, status_code} map, so a oneOf over them matches -// four branches and therefore always fails. -// - Empty collections are ambiguous. issue_read method=get_comments on an -// issue with no comments returns [], which vacuously satisfies every array -// branch. -// - Optional fields overlap. issue_read method=get on a sub-issue populates -// `parent`, which also satisfies the get_parent branch. -// -// anyOf ("at least one") accepts all three while still rejecting values that -// match no branch, which is the useful half of the validation. +// Always anyOf, never oneOf: oneOf requires EXACTLY ONE branch to match, and +// these unions have structurally identical branches, empty arrays that satisfy +// every array branch, and overlapping optional fields. anyOf still rejects +// values matching no branch, which is the useful half of the validation. +// TestAnyOfAcceptsAmbiguousPayloadsButStillRejectsGarbage demonstrates both. func AnyOfSchema(branches ...*jsonschema.Schema) *jsonschema.Schema { return &jsonschema.Schema{AnyOf: branches} } -// MustRawOutputSchema wraps a hand-authored JSON Schema document, panicking -// during package initialization if it does not parse or does not resolve. -// -// Hand-authored schemas are kept as json.RawMessage rather than -// *jsonschema.Schema so they round-trip byte-for-byte to the client and -// produce deterministic toolsnaps output, independent of jsonschema-go's -// struct field coverage and marshaling order. +// MustRawOutputSchema wraps a hand-authored JSON Schema document as raw bytes, +// so it reaches the client exactly as written rather than through +// jsonschema-go's struct coverage and field ordering. // -// Resolution is checked here (not just parsing) so a dangling $ref fails the -// build rather than the request. +// Resolution is checked, not just parsing, so a dangling $ref fails the build +// rather than a request. func MustRawOutputSchema(raw string) json.RawMessage { var parsed jsonschema.Schema if err := json.Unmarshal([]byte(raw), &parsed); err != nil { @@ -73,9 +56,8 @@ func MustRawOutputSchema(raw string) json.RawMessage { return json.RawMessage(raw) } -// outputSchemasEnabled reports whether the output_schemas feature is on for -// this request. This is the rollout gate only; see canSendStructuredContent -// for the protocol-legality gate. +// outputSchemasEnabled is the rollout gate; canSendStructuredContent is the +// separate protocol-legality gate. func outputSchemasEnabled(ctx context.Context, deps ToolDependencies) bool { return deps.IsFeatureEnabled(ctx, FeatureFlagOutputSchemas) } @@ -85,14 +67,10 @@ func isJSONObject(marshaled []byte) bool { return bytes.HasPrefix(bytes.TrimLeft(marshaled, " \t\r\n"), []byte("{")) } -// canSendStructuredContent reports whether a structuredContent value of the -// given marshaled shape may legally be sent to this client. -// -// Under 2025-11-25 and earlier, structuredContent is typed -// `{ [key: string]: unknown }` — a JSON object. 2026-07-28 widened it to -// `unknown`, explicitly "any JSON value (object, array, string, number, -// boolean, or null)". So an object is always safe; anything else requires the -// newer protocol. req may be nil in tests, in which case only objects are sent. +// canSendStructuredContent reports whether a structuredContent value of this +// shape may legally be sent to this client. An object is always safe; anything +// else needs 2026-07-28, which widened the field from an object to any JSON +// value. A nil req (tests) sends objects only. func canSendStructuredContent(req *mcp.CallToolRequest, marshaled []byte) bool { if isJSONObject(marshaled) { return true @@ -103,15 +81,10 @@ func canSendStructuredContent(req *mcp.CallToolRequest, marshaled []byte) bool { return req.ProtocolVersion() >= inventory.ProtocolVersionNonObjectOutputSchemas } -// structuredTextResult builds a tool result whose text content is the -// serialized textValue — byte-identical to what the tool returned before -// output schemas existed — and which additionally carries structured as -// structuredContent when both gates allow it. -// -// The text block is always populated regardless of the gates. The spec calls -// for this independently: "For backwards compatibility, a tool that returns -// structured content SHOULD also return the serialized JSON in a TextContent -// block." +// structuredTextResult returns a result whose text content is the serialized +// textValue — byte-identical to the pre-output-schema behaviour, and populated +// regardless of the gates — plus structured as structuredContent when both the +// feature flag and the client's protocol version allow it. func structuredTextResult(ctx context.Context, deps ToolDependencies, req *mcp.CallToolRequest, textValue, structured any) (*mcp.CallToolResult, error) { data, err := json.Marshal(textValue) if err != nil { diff --git a/pkg/github/output_schemas_polymorphic.go b/pkg/github/output_schemas_polymorphic.go index 68189cf6bf..77eb49957e 100644 --- a/pkg/github/output_schemas_polymorphic.go +++ b/pkg/github/output_schemas_polymorphic.go @@ -8,24 +8,9 @@ import ( ) // Output schemas for tools whose response shape varies by the `method` -// argument. -// -// These are hand-authored rather than inferred from Go types, because no -// single Go type describes a method-dispatched tool's output. They live as -// .json files so they stay reviewable and diffable, and are embedded rather -// than pasted into Go string literals (their descriptions contain markdown -// backticks, which raw string literals cannot hold). -// -// Every union uses anyOf, never oneOf. oneOf requires EXACTLY ONE branch to -// match, which is provably wrong here: actions_run_trigger has four -// structurally identical branches, an empty array vacuously satisfies every -// array branch, and an issue_read `get` on a sub-issue also satisfies the -// `get_parent` branch. See AnyOfSchema in output_schema.go. -// -// Schemas whose root is not {"type":"object"} are legal only from protocol -// version 2026-07-28 (SEP-2106); inventory.OutputSchemaVersionGate strips them -// per-request for older clients. TestPolymorphicOutputSchemaRootKinds pins -// which schemas fall on which side of that line. +// argument. Hand-authored, because no single Go type describes a +// method-dispatched tool's output, and kept as .json so they stay reviewable +// and diffable. Every union uses anyOf, never oneOf — see AnyOfSchema. // //go:embed output_schemas/*.json var outputSchemaFS embed.FS @@ -50,14 +35,11 @@ var ( ) // mustLoadOutputSchema reads an embedded schema, compacts it, and verifies it -// resolves — panicking during package initialization on any failure so a -// malformed schema or dangling $ref fails the build rather than a request. +// resolves, panicking at init so a malformed schema fails the build. // -// Compacting is not cosmetic: it strips inter-token whitespace, so a checkout -// that rewrote the files' line endings (git's core.autocrlf does this on -// Windows, and it is what corrupts pkg/octicons' embedded data URIs) cannot -// leak stray carriage returns into what is sent to clients. It preserves -// string contents exactly, so descriptions are untouched. +// Compacting also strips inter-token whitespace, so a checkout that rewrote +// line endings (core.autocrlf on Windows, which is what corrupts +// pkg/octicons' embedded data URIs) cannot leak carriage returns to clients. func mustLoadOutputSchema(name string) json.RawMessage { raw, err := outputSchemaFS.ReadFile(fmt.Sprintf("output_schemas/%s.json", name)) if err != nil { diff --git a/pkg/inventory/output_schema_gate.go b/pkg/inventory/output_schema_gate.go index c8b288f6b8..15bddcea6a 100644 --- a/pkg/inventory/output_schema_gate.go +++ b/pkg/inventory/output_schema_gate.go @@ -7,31 +7,18 @@ import ( "github.com/modelcontextprotocol/go-sdk/mcp" ) -// ProtocolVersionNonObjectOutputSchemas is the first MCP protocol revision that -// permits a tool's outputSchema to have a root other than `{"type":"object"}`, -// and correspondingly permits structuredContent to be any JSON value rather -// than only a JSON object (SEP-2106). -// -// Under 2025-11-25 the normative schema typed outputSchema as a closed shape: -// -// outputSchema?: { $schema?: string; type: "object"; -// properties?: { [key: string]: object }; required?: string[]; } -// -// with the doc comment "Currently restricted to type: \"object\" at the root -// level." Both the restriction and the closed shape were removed in -// 2026-07-28, which types it as `{ $schema?: string; [key: string]: unknown }` -// and describes it as "any valid JSON Schema 2020-12". +// ProtocolVersionNonObjectOutputSchemas is the first MCP protocol revision +// permitting a tool's outputSchema to have a root other than +// `{"type":"object"}`, and structuredContent to be any JSON value rather than +// only an object. Before it, the normative schema restricted outputSchema to +// "type: \"object\" at the root level" (SEP-2106 lifted both). const ProtocolVersionNonObjectOutputSchemas = "2026-07-28" -// HasObjectRootOutputSchema reports whether schema marshals to a JSON Schema -// whose root declares `"type": "object"`. Such a schema is legal under every -// protocol revision that supports outputSchema at all, so it needs no gating — -// even when it uses composition keywords like anyOf internally. -// -// A schema that fails to marshal, or that declares any other root (a bare -// anyOf, `"type": "array"`, a $ref) is reported as non-object-root and is -// therefore gated. Failing closed is deliberate: an unmarshalable schema -// should not be advertised to a client that may reject the whole tools/list. +// HasObjectRootOutputSchema reports whether schema's root declares +// `"type": "object"`, which needs no gating at any version — even when it uses +// anyOf internally. Anything else (a bare anyOf, an array, a $ref) is gated, +// as is a schema that fails to marshal: failing closed keeps an unparseable +// schema from breaking a client's whole tools/list. func HasObjectRootOutputSchema(schema any) bool { if schema == nil { return false @@ -58,11 +45,9 @@ func HasObjectRootOutputSchema(schema any) bool { // // Object-root schemas pass through untouched at every version. // -// The middleware copies on write. The SDK's listTools appends the *same* -// *mcp.Tool pointers it holds in its registry (mcp/server.go:936-939), so -// mutating a tool in place here would strip the schema from the server's -// stored definition and leak that to every later session on the same server. -// Only tools that actually need gating are copied. +// Copies on write: the SDK's listTools hands back the *same* *mcp.Tool +// pointers it holds in its registry, so mutating one here would strip the +// schema from the server's stored definition for every later session. func OutputSchemaVersionGate() mcp.Middleware { return func(next mcp.MethodHandler) mcp.MethodHandler { return func(ctx context.Context, method string, req mcp.Request) (mcp.Result, error) { @@ -78,12 +63,10 @@ func OutputSchemaVersionGate() mcp.Middleware { if !ok { return res, err } - // ProtocolVersion reads the per-request _meta for >= 2026-07-28 - // clients (which no longer send initialize at all, per SEP-2575) - // and falls back to the session's InitializeParams for older ones. - // An empty version means we could not determine it; gate in that - // case, since only a client we know is new can be trusted with a - // non-object root. + // Reads the per-request _meta for >= 2026-07-28 clients (which no + // longer send initialize at all, per SEP-2575), falling back to + // the session's InitializeParams. Empty means undeterminable, so + // it gates. if listReq.ProtocolVersion() >= ProtocolVersionNonObjectOutputSchemas { return res, err } diff --git a/pkg/inventory/structured_mirror.go b/pkg/inventory/structured_mirror.go index 5933bee791..cc60d70869 100644 --- a/pkg/inventory/structured_mirror.go +++ b/pkg/inventory/structured_mirror.go @@ -7,27 +7,16 @@ import ( "github.com/modelcontextprotocol/go-sdk/mcp" ) -// mirrorStructuredContent wraps a tool handler so that a tool which declares an -// OutputSchema also returns structuredContent, without the handler having to -// produce it separately. +// mirrorStructuredContent gives tools that declare an OutputSchema a +// structuredContent value, taken as the raw bytes of the serialized-JSON text +// block they already emit. Reusing those bytes rather than re-marshaling keeps +// the two fields byte-identical, so number formatting and key order cannot +// drift between them. // -// This exploits the relationship the spec already defines between the two -// fields: "For backwards compatibility, a tool that returns structured content -// SHOULD also return the serialized JSON in a TextContent block." Every tool in -// this package already emits exactly that — one text block holding -// json.Marshal of the result — so the structured value is recoverable from it -// exactly, with no re-marshaling and therefore no risk of changing number -// formatting or key order. +// Only tools declaring a schema are wrapped, leaving the rest unchanged. // -// The mirrored value is the raw bytes of the text block, so content and -// structuredContent are byte-identical by construction and cannot drift. -// -// It applies only to tools that declared an OutputSchema. Tools without one are -// untouched, so the wire format of the other ~120 tools is unchanged. -// -// When omitRedundantText is set, the now-duplicated text block is dropped for -// clients new enough to read structuredContent, halving the response instead of -// doubling it. See dropRedundantTextContent. +// omitRedundantText additionally drops the now-duplicated text block; see +// dropRedundantTextContent. func mirrorStructuredContent(next mcp.ToolHandler, omitRedundantText bool) mcp.ToolHandler { return func(ctx context.Context, req *mcp.CallToolRequest) (*mcp.CallToolResult, error) { res, err := next(ctx, req) @@ -60,29 +49,12 @@ func mirrorStructuredContent(next mcp.ToolHandler, omitRedundantText bool) mcp.T } // dropRedundantTextContent removes the text block that structuredContent now -// duplicates byte-for-byte. -// -// The spec's "a tool that returns structured content SHOULD also return the -// serialized JSON in a TextContent block" is a backwards-compatibility clause. -// The Go SDK says as much where it synthesises that block on its typed path: -// the fallback exists "so that pre-SEP-2106 clients can recover the structured -// payload from unstructured content" (mcp/server.go). A client that negotiated -// 2026-07-28 is not such a client, so for those the block is pure duplication — -// this server otherwise sends the same JSON twice, which works against -// csv_output, minimal_output and the fields param, all of which exist to make -// responses smaller. -// -// Callers must only reach here when structuredContent was mirrored from this -// exact text, so nothing is lost: the bytes survive, they just travel once. -// -// content stays present as an empty array rather than being unset — the draft -// schema still lists it in CallToolResult's required set, and the SDK -// normalises an empty slice to `[]` rather than `null`. +// duplicates byte-for-byte. The spec's "SHOULD also return the serialized JSON +// in a TextContent block" is a backwards-compatibility clause, so for a client +// that reads structuredContent the block is pure duplication. // -// This is opt-in (see RegisterToolOptions.OmitRedundantTextContent) rather than -// automatic, because negotiating 2026-07-28 does not prove a client actually -// reads structuredContent — there is no capability that says so, and a client -// that ignored it would see an empty result. +// Callers must only reach here having mirrored from this exact text. Empty +// rather than unset: content is in CallToolResult's required set. func dropRedundantTextContent(res *mcp.CallToolResult) { res.Content = []mcp.Content{} } From 53cb8d2b9c417a06d02435d93e1a06b7ab09caa4 Mon Sep 17 00:00:00 2001 From: olaservo Date: Sat, 25 Jul 2026 18:21:22 -0700 Subject: [PATCH 5/6] test: prove real handler output conforms to every declared output schema MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Extends TestIssueReadOutputValidatesAgainstDeclaredSchema to the remaining six tools, taking method coverage from 3 of 28 to 28 of 28. Each tool gets its own file; each subtest runs the real handler against the existing mock harness, then validates the emitted payload against the schema the tool advertises. Validating the text block is equivalent to validating what a client receives, because the structured-content mirror publishes those exact bytes as structuredContent. Beyond the happy path, the tests cover the cases that actually exercise a schema's required sets: empty collections, sparse objects with every optional field absent, and — for actions_get — a different single surviving key per method, so a non-first branch of each anyOf is exercised rather than always the first. Fixes a latent bug the tests surfaced. go-github decodes GetWorkflowRunByID, GetWorkflowJobByID and GetWorkflowRunUsageByID into a *T, so a 200 carrying a null body leaves the pointer nil and json.Marshal emits the literal `null`, which cannot validate against an object-rooted schema. Same class as the sub-issues bug already fixed here; these three now report the anomaly instead of returning a payload no caller can use. Co-Authored-By: Claude Opus 5 --- pkg/github/actions.go | 12 + .../output_conformance_actions_get_test.go | 439 +++++++++++++++++ .../output_conformance_actions_list_test.go | 461 +++++++++++++++++ ...ut_conformance_actions_run_trigger_test.go | 221 +++++++++ ...nformance_discussion_comment_write_test.go | 465 ++++++++++++++++++ ..._conformance_issue_dependency_read_test.go | 285 +++++++++++ ...put_conformance_issue_read_graphql_test.go | 277 +++++++++++ ...tput_conformance_pull_request_read_test.go | 460 +++++++++++++++++ 8 files changed, 2620 insertions(+) create mode 100644 pkg/github/output_conformance_actions_get_test.go create mode 100644 pkg/github/output_conformance_actions_list_test.go create mode 100644 pkg/github/output_conformance_actions_run_trigger_test.go create mode 100644 pkg/github/output_conformance_discussion_comment_write_test.go create mode 100644 pkg/github/output_conformance_issue_dependency_read_test.go create mode 100644 pkg/github/output_conformance_issue_read_graphql_test.go create mode 100644 pkg/github/output_conformance_pull_request_read_test.go diff --git a/pkg/github/actions.go b/pkg/github/actions.go index e25b036c70..c5702de5bd 100644 --- a/pkg/github/actions.go +++ b/pkg/github/actions.go @@ -802,6 +802,12 @@ func getWorkflowRun(ctx context.Context, client *github.Client, owner, repo stri return ghErrors.NewGitHubAPIErrorResponse(ctx, "failed to get workflow run", resp, err), nil, nil } defer func() { _ = resp.Body.Close() }() + // go-github decodes into a *WorkflowRun, so a 200 carrying a null body + // leaves it nil and json.Marshal would emit the literal `null`. Report the + // anomaly rather than returning a payload no caller can use. + if workflowRun == nil { + return utils.NewToolResultError("workflow run response was empty"), nil, nil + } r, err := json.Marshal(workflowRun) if err != nil { return nil, nil, fmt.Errorf("failed to marshal workflow run: %w", err) @@ -815,6 +821,9 @@ func getWorkflowJob(ctx context.Context, client *github.Client, owner, repo stri return ghErrors.NewGitHubAPIErrorResponse(ctx, "failed to get workflow job", resp, err), nil, nil } defer func() { _ = resp.Body.Close() }() + if workflowJob == nil { + return utils.NewToolResultError("workflow job response was empty"), nil, nil + } r, err := json.Marshal(workflowJob) if err != nil { return nil, nil, fmt.Errorf("failed to marshal workflow job: %w", err) @@ -1007,6 +1016,9 @@ func getWorkflowRunUsage(ctx context.Context, client *github.Client, owner, repo } defer func() { _ = resp.Body.Close() }() + if usage == nil { + return utils.NewToolResultError("workflow run usage response was empty"), nil, nil + } r, err := json.Marshal(usage) if err != nil { return nil, nil, fmt.Errorf("failed to marshal response: %w", err) diff --git a/pkg/github/output_conformance_actions_get_test.go b/pkg/github/output_conformance_actions_get_test.go new file mode 100644 index 0000000000..e4e31eda41 --- /dev/null +++ b/pkg/github/output_conformance_actions_get_test.go @@ -0,0 +1,439 @@ +package github + +import ( + "context" + "encoding/json" + "net/http" + "strings" + "testing" + "time" + + gogithub "github.com/google/go-github/v89/github" + "github.com/google/jsonschema-go/jsonschema" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/github/github-mcp-server/pkg/translations" +) + +// Endpoints actions_get reaches that helper_test.go has no constant for. +// Named with the tool prefix so they cannot collide with a constant another +// test file adds later. +const ( + actionsgetEndpointJobByID = "GET /repos/{owner}/{repo}/actions/jobs/{job_id}" + actionsgetEndpointArtifactZip = "GET /repos/{owner}/{repo}/actions/artifacts/{artifact_id}/zip" +) + +// actionsgetRedirect reproduces the 302 + Location dance GitHub uses for the +// two endpoints that hand back a short-lived download URL rather than a body. +// go-github returns the Location verbatim without following it (maxRedirects +// only follows 301), so this is the whole of the real protocol. +func actionsgetRedirect(location string) http.HandlerFunc { + return func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Location", location) + w.WriteHeader(http.StatusFound) + } +} + +// The end-to-end guarantee for actions_get: what the real handler emits must +// validate against the outputSchema the tool advertises. The structured-content +// mirror publishes the exact bytes of the single text block as +// structuredContent, so validating the text block here is equivalent to +// validating what a client receives. +// +// Every value of the tool's `method` enum is covered, and the enum itself is +// pinned below, so adding a method without adding a case fails the test rather +// than silently shipping an unvalidated branch. +func TestActionsGetOutputValidatesAgainstDeclaredSchema(t *testing.T) { + t.Parallel() + + serverTool := ActionsGet(translations.NullTranslationHelper) + resolved := resolveToolSchema(t, actionsGetOutputSchema) + + ts := gogithub.Timestamp{Time: time.Date(2024, 3, 4, 5, 6, 7, 0, time.UTC)} + user := &gogithub.User{ + Login: gogithub.Ptr("octocat"), + ID: gogithub.Ptr(int64(583231)), + NodeID: gogithub.Ptr("MDQ6VXNlcjU4MzIzMQ=="), + AvatarURL: gogithub.Ptr("https://avatars.githubusercontent.com/u/583231?v=4"), + HTMLURL: gogithub.Ptr("https://github.com/octocat"), + URL: gogithub.Ptr("https://api.github.com/users/octocat"), + Type: gogithub.Ptr("User"), + } + repo := &gogithub.Repository{ + ID: gogithub.Ptr(int64(1296269)), + NodeID: gogithub.Ptr("MDEwOlJlcG9zaXRvcnkxMjk2MjY5"), + Name: gogithub.Ptr("repo"), + FullName: gogithub.Ptr("owner/repo"), + Private: gogithub.Ptr(false), + URL: gogithub.Ptr("https://api.github.com/repos/owner/repo"), + HTMLURL: gogithub.Ptr("https://github.com/owner/repo"), + Owner: user, + } + + // Every field the get_workflow branch declares, so a property whose type + // drifts from the go-github struct is caught rather than skipped over. + fullWorkflow := &gogithub.Workflow{ + ID: gogithub.Ptr(int64(161335)), + NodeID: gogithub.Ptr("MDg6V29ya2Zsb3cxNjEzMzU="), + Name: gogithub.Ptr("CI"), + Path: gogithub.Ptr(".github/workflows/ci.yml"), + State: gogithub.Ptr("active"), + CreatedAt: &ts, + UpdatedAt: &ts, + URL: gogithub.Ptr("https://api.github.com/repos/owner/repo/actions/workflows/161335"), + HTMLURL: gogithub.Ptr("https://github.com/owner/repo/blob/main/.github/workflows/ci.yml"), + BadgeURL: gogithub.Ptr("https://github.com/owner/repo/workflows/CI/badge.svg"), + } + + fullWorkflowRun := &gogithub.WorkflowRun{ + ID: gogithub.Ptr(int64(12345)), + Name: gogithub.Ptr("CI"), + NodeID: gogithub.Ptr("MDEyOldvcmtmbG93IFJ1bjEyMzQ1"), + HeadBranch: gogithub.Ptr("main"), + HeadSHA: gogithub.Ptr("acb5820ced9479c074f688cc328bf03f341a511d"), + Path: gogithub.Ptr(".github/workflows/ci.yml"), + RunNumber: gogithub.Ptr(42), + RunAttempt: gogithub.Ptr(1), + Event: gogithub.Ptr("push"), + DisplayTitle: gogithub.Ptr("Fix the thing"), + Status: gogithub.Ptr("completed"), + Conclusion: gogithub.Ptr("success"), + WorkflowID: gogithub.Ptr(int64(161335)), + CheckSuiteID: gogithub.Ptr(int64(42)), + CheckSuiteNodeID: gogithub.Ptr("MDEwOkNoZWNrU3VpdGU0Mg=="), + URL: gogithub.Ptr("https://api.github.com/repos/owner/repo/actions/runs/12345"), + HTMLURL: gogithub.Ptr("https://github.com/owner/repo/actions/runs/12345"), + PullRequests: []*gogithub.PullRequest{{Number: gogithub.Ptr(3), Title: gogithub.Ptr("Fix the thing")}}, + CreatedAt: &ts, + UpdatedAt: &ts, + RunStartedAt: &ts, + JobsURL: gogithub.Ptr("https://api.github.com/repos/owner/repo/actions/runs/12345/jobs"), + LogsURL: gogithub.Ptr("https://api.github.com/repos/owner/repo/actions/runs/12345/logs"), + CheckSuiteURL: gogithub.Ptr("https://api.github.com/repos/owner/repo/check-suites/42"), + ArtifactsURL: gogithub.Ptr("https://api.github.com/repos/owner/repo/actions/runs/12345/artifacts"), + CancelURL: gogithub.Ptr("https://api.github.com/repos/owner/repo/actions/runs/12345/cancel"), + RerunURL: gogithub.Ptr("https://api.github.com/repos/owner/repo/actions/runs/12345/rerun"), + PreviousAttemptURL: gogithub.Ptr("https://api.github.com/repos/owner/repo/actions/runs/12344"), + WorkflowURL: gogithub.Ptr("https://api.github.com/repos/owner/repo/actions/workflows/161335"), + Repository: repo, + HeadRepository: repo, + Actor: user, + TriggeringActor: user, + ReferencedWorkflows: []*gogithub.ReferencedWorkflow{{Path: gogithub.Ptr("owner/repo/.github/workflows/reusable.yml@main"), SHA: gogithub.Ptr("acb5820"), Ref: gogithub.Ptr("refs/heads/main")}}, + HeadCommit: &gogithub.HeadCommit{ + ID: gogithub.Ptr("acb5820ced9479c074f688cc328bf03f341a511d"), + TreeID: gogithub.Ptr("d23f6eedb1e1b9610bbc754ddb5197bfe7271223"), + Message: gogithub.Ptr("Fix the thing"), + Timestamp: &ts, + Author: &gogithub.CommitAuthor{Name: gogithub.Ptr("Mona"), Email: gogithub.Ptr("mona@github.com")}, + Committer: &gogithub.CommitAuthor{Name: gogithub.Ptr("Mona"), Email: gogithub.Ptr("mona@github.com")}, + }, + } + + fullWorkflowJob := &gogithub.WorkflowJob{ + ID: gogithub.Ptr(int64(399444496)), + RunID: gogithub.Ptr(int64(12345)), + RunURL: gogithub.Ptr("https://api.github.com/repos/owner/repo/actions/runs/12345"), + NodeID: gogithub.Ptr("MDg6Q2hlY2tSdW4zOTk0NDQ0OTY="), + HeadBranch: gogithub.Ptr("main"), + HeadSHA: gogithub.Ptr("acb5820ced9479c074f688cc328bf03f341a511d"), + URL: gogithub.Ptr("https://api.github.com/repos/owner/repo/actions/jobs/399444496"), + HTMLURL: gogithub.Ptr("https://github.com/owner/repo/runs/399444496"), + Status: gogithub.Ptr("completed"), + Conclusion: gogithub.Ptr("success"), + CreatedAt: &ts, + StartedAt: &ts, + CompletedAt: &ts, + Name: gogithub.Ptr("build"), + Steps: []*gogithub.TaskStep{{ + Name: gogithub.Ptr("Set up job"), + Status: gogithub.Ptr("completed"), + Conclusion: gogithub.Ptr("success"), + Number: gogithub.Ptr(int64(1)), + StartedAt: &ts, + CompletedAt: &ts, + }}, + CheckRunURL: gogithub.Ptr("https://api.github.com/repos/owner/repo/check-runs/399444496"), + Labels: []string{"ubuntu-latest"}, + RunnerID: gogithub.Ptr(int64(1)), + RunnerName: gogithub.Ptr("my runner"), + RunnerGroupID: gogithub.Ptr(int64(2)), + RunnerGroupName: gogithub.Ptr("my runner group"), + RunAttempt: gogithub.Ptr(int64(1)), + WorkflowName: gogithub.Ptr("CI"), + } + + tests := []struct { + name string + method string + resourceID string + handlers map[string]http.HandlerFunc + }{ + { + name: "get_workflow", + method: actionsMethodGetWorkflow, + resourceID: "161335", + handlers: map[string]http.HandlerFunc{ + GetReposActionsWorkflowsByOwnerByRepoByWorkflowID: mockResponse(t, http.StatusOK, fullWorkflow), + }, + }, + { + // get_workflow accepts a file name as well as a numeric ID, which + // dispatches to GetWorkflowByFileName instead of GetWorkflowByID. + // Same declared branch, different code path into it. + name: "get_workflow by file name", + method: actionsMethodGetWorkflow, + resourceID: "ci.yml", + handlers: map[string]http.HandlerFunc{ + GetReposActionsWorkflowsByOwnerByRepoByWorkflowID: mockResponse(t, http.StatusOK, fullWorkflow), + }, + }, + { + // Sparsest payload the branch can legally accept: every field of + // go-github's Workflow is a pointer with omitempty, so all but one + // drop out of the marshalled object. `state` rather than `id` on + // purpose, so a non-first singleton-required branch is exercised. + // A fully empty {} is deliberately NOT accepted by the schema + // (see TestPolymorphicOutputSchemasAreNotVacuous) and GitHub never + // returns one for this endpoint. + name: "get_workflow with all optionals absent", + method: actionsMethodGetWorkflow, + resourceID: "161335", + handlers: map[string]http.HandlerFunc{ + GetReposActionsWorkflowsByOwnerByRepoByWorkflowID: mockResponse(t, http.StatusOK, + &gogithub.Workflow{State: gogithub.Ptr("disabled_manually")}), + }, + }, + { + name: "get_workflow_run", + method: actionsMethodGetWorkflowRun, + resourceID: "12345", + handlers: map[string]http.HandlerFunc{ + GetReposActionsRunsByOwnerByRepoByRunID: mockResponse(t, http.StatusOK, fullWorkflowRun), + }, + }, + { + // A run still in flight: `conclusion` is absent, which is the one + // optional the schema documents as routinely missing. + name: "get_workflow_run in progress without conclusion", + method: actionsMethodGetWorkflowRun, + resourceID: "12345", + handlers: map[string]http.HandlerFunc{ + GetReposActionsRunsByOwnerByRepoByRunID: mockResponse(t, http.StatusOK, &gogithub.WorkflowRun{ + ID: gogithub.Ptr(int64(12345)), + Status: gogithub.Ptr("in_progress"), + RunStartedAt: &ts, + }), + }, + }, + { + // Empty collections upstream: pull_requests and + // referenced_workflows come back as [], and go-github's omitempty + // drops them entirely on the way out. Proves the round trip still + // conforms rather than emitting `null` for either. + name: "get_workflow_run with empty collections", + method: actionsMethodGetWorkflowRun, + resourceID: "12345", + handlers: map[string]http.HandlerFunc{ + GetReposActionsRunsByOwnerByRepoByRunID: mockResponse(t, http.StatusOK, + `{"id":12345,"status":"queued","pull_requests":[],"referenced_workflows":[]}`), + }, + }, + { + name: "get_workflow_run with all optionals absent", + method: actionsMethodGetWorkflowRun, + resourceID: "12345", + handlers: map[string]http.HandlerFunc{ + GetReposActionsRunsByOwnerByRepoByRunID: mockResponse(t, http.StatusOK, + &gogithub.WorkflowRun{Status: gogithub.Ptr("queued")}), + }, + }, + { + name: "get_workflow_job", + method: actionsMethodGetWorkflowJob, + resourceID: "399444496", + handlers: map[string]http.HandlerFunc{ + actionsgetEndpointJobByID: mockResponse(t, http.StatusOK, fullWorkflowJob), + }, + }, + { + // The other empty-collection case: a job with zero steps and zero + // runner labels. + name: "get_workflow_job with empty steps and labels", + method: actionsMethodGetWorkflowJob, + resourceID: "399444496", + handlers: map[string]http.HandlerFunc{ + actionsgetEndpointJobByID: mockResponse(t, http.StatusOK, + `{"id":399444496,"status":"queued","steps":[],"labels":[]}`), + }, + }, + { + name: "get_workflow_job with all optionals absent", + method: actionsMethodGetWorkflowJob, + resourceID: "399444496", + handlers: map[string]http.HandlerFunc{ + actionsgetEndpointJobByID: mockResponse(t, http.StatusOK, + &gogithub.WorkflowJob{Conclusion: gogithub.Ptr("cancelled")}), + }, + }, + { + name: "download_workflow_run_artifact", + method: actionsMethodDownloadWorkflowArtifact, + resourceID: "11", + handlers: map[string]http.HandlerFunc{ + actionsgetEndpointArtifactZip: actionsgetRedirect("https://pipelines.actions.githubusercontent.com/artifact/11.zip"), + }, + }, + { + // Degenerate but reachable: a 302 carrying no Location parses to + // the empty URL, so download_url is "". The branch's required set + // still has to hold — the server builds this object itself, so + // every key is present regardless. + name: "download_workflow_run_artifact with empty location", + method: actionsMethodDownloadWorkflowArtifact, + resourceID: "11", + handlers: map[string]http.HandlerFunc{ + actionsgetEndpointArtifactZip: actionsgetRedirect(""), + }, + }, + { + name: "get_workflow_run_usage", + method: actionsMethodGetWorkflowRunUsage, + resourceID: "12345", + handlers: map[string]http.HandlerFunc{ + GetReposActionsRunsTimingByOwnerByRepoByRunID: mockResponse(t, http.StatusOK, &gogithub.WorkflowRunUsage{ + Billable: &gogithub.WorkflowRunBillMap{ + "UBUNTU": { + TotalMS: gogithub.Ptr(int64(180000)), + Jobs: gogithub.Ptr(1), + JobRuns: []*gogithub.WorkflowRunJobRun{{JobID: gogithub.Ptr(1), DurationMS: gogithub.Ptr(int64(180000))}}, + }, + }, + RunDurationMS: gogithub.Ptr(int64(500000)), + }), + }, + }, + { + // The empty-collection case that actually survives marshalling: + // `billable` is a *pointer* to a map, so omitempty keeps it and an + // empty map emits `"billable":{}`. run_duration_ms is absent, so + // the payload leans on the second singleton-required branch alone. + name: "get_workflow_run_usage with nothing billable", + method: actionsMethodGetWorkflowRunUsage, + resourceID: "12345", + handlers: map[string]http.HandlerFunc{ + GetReposActionsRunsTimingByOwnerByRepoByRunID: mockResponse(t, http.StatusOK, + &gogithub.WorkflowRunUsage{Billable: &gogithub.WorkflowRunBillMap{}}), + }, + }, + { + // A billable entry with every optional absent, plus an empty + // job_runs array upstream. + name: "get_workflow_run_usage with sparse billable entry", + method: actionsMethodGetWorkflowRunUsage, + resourceID: "12345", + handlers: map[string]http.HandlerFunc{ + GetReposActionsRunsTimingByOwnerByRepoByRunID: mockResponse(t, http.StatusOK, + `{"billable":{"MACOS":{"job_runs":[]}}}`), + }, + }, + { + name: "get_workflow_run_logs_url", + method: actionsMethodGetWorkflowRunLogsURL, + resourceID: "12345", + handlers: map[string]http.HandlerFunc{ + GetReposActionsRunsLogsByOwnerByRepoByRunID: actionsgetRedirect("https://pipelines.actions.githubusercontent.com/logs/12345.zip"), + }, + }, + { + name: "get_workflow_run_logs_url with empty location", + method: actionsMethodGetWorkflowRunLogsURL, + resourceID: "12345", + handlers: map[string]http.HandlerFunc{ + GetReposActionsRunsLogsByOwnerByRepoByRunID: actionsgetRedirect(""), + }, + }, + } + + // Pin the enum: a new method must gain a case above, not slip through. + covered := map[string]bool{} + for _, tt := range tests { + covered[tt.method] = true + } + inputSchema, ok := serverTool.Tool.InputSchema.(*jsonschema.Schema) + require.True(t, ok, "input schema should be a *jsonschema.Schema") + for _, m := range inputSchema.Properties["method"].Enum { + assert.True(t, covered[m.(string)], + "method %q is advertised in the enum but has no output-conformance case", m) + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + deps := BaseDeps{Client: mustNewGHClient(t, MockHTTPClientWithHandlers(tt.handlers))} + handler := serverTool.Handler(deps) + + request := createMCPRequest(map[string]any{ + "method": tt.method, + "owner": "owner", + "repo": "repo", + "resource_id": tt.resourceID, + }) + result, err := handler(ContextWithDeps(context.Background(), deps), &request) + require.NoError(t, err) + require.False(t, result.IsError, "handler should succeed") + + text := getTextResult(t, result) + + var payload any + require.NoError(t, json.Unmarshal([]byte(text.Text), &payload), + "handler output must be JSON for the mirror to publish it as structuredContent") + + require.NoError(t, resolved.Validate(payload), + "real handler output for method=%s must conform to the advertised outputSchema", tt.method) + }) + } +} + +// go-github decodes these endpoints into a *T, so a 200 whose body is JSON +// null leaves the pointer nil and json.Marshal emits the literal `null` — +// which cannot validate against an object-rooted schema. Same class of bug as +// TestGetSubIssuesNeverEmitsNull. +func TestActionsGetNeverEmitsNull(t *testing.T) { + t.Parallel() + + nullBody := func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte(`null`)) + } + + tests := []struct { + name string + method string + endpoint string + }{ + {"get_workflow_run", "get_workflow_run", GetReposActionsRunsByOwnerByRepoByRunID}, + {"get_workflow_job", "get_workflow_job", actionsgetEndpointJobByID}, + {"get_workflow_run_usage", "get_workflow_run_usage", GetReposActionsRunsTimingByOwnerByRepoByRunID}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + deps := BaseDeps{Client: mustNewGHClient(t, MockHTTPClientWithHandlers( + map[string]http.HandlerFunc{tt.endpoint: nullBody}))} + + request := createMCPRequest(map[string]any{ + "method": tt.method, "owner": "octocat", "repo": "repo", "resource_id": float64(1), + }) + serverTool := ActionsGet(translations.NullTranslationHelper) + result, err := serverTool.Handler(deps)(ContextWithDeps(context.Background(), deps), &request) + require.NoError(t, err) + + text := getTextResult(t, result) + assert.NotEqual(t, "null", strings.TrimSpace(text.Text), + "a nil pointer must not be marshalled straight to the client") + assert.True(t, result.IsError, + "an empty body is an API anomaly and should surface as an error, not a null payload") + }) + } +} diff --git a/pkg/github/output_conformance_actions_list_test.go b/pkg/github/output_conformance_actions_list_test.go new file mode 100644 index 0000000000..d84ce95d62 --- /dev/null +++ b/pkg/github/output_conformance_actions_list_test.go @@ -0,0 +1,461 @@ +package github + +import ( + "context" + "encoding/json" + "net/http" + "testing" + "time" + + gogithub "github.com/google/go-github/v89/github" + "github.com/google/jsonschema-go/jsonschema" + "github.com/stretchr/testify/require" + + "github.com/github/github-mcp-server/pkg/translations" +) + +// The end-to-end guarantee for actions_list: what the real handler emits must +// validate against the outputSchema the tool advertises. The structured-content +// mirror publishes the exact bytes of the text block as structuredContent, so +// validating the text block here is equivalent to validating what a client +// receives. +// +// Every value of the tool's `method` enum gets a subtest, and each one covers +// the awkward payloads as well as the happy path: an empty collection (which +// go-github's omitempty tags reduce to {"total_count":0} with the item array +// absent) and a maximally sparse envelope/item (one field present, every other +// optional field gone) — those are what actually exercise the schema's required +// sets rather than a convenient, fully-populated fixture. +func TestActionsListOutputValidatesAgainstDeclaredSchema(t *testing.T) { + t.Parallel() + + serverTool := ActionsList(translations.NullTranslationHelper) + resolved := resolveToolSchema(t, actionsListOutputSchema) + + ts := gogithub.Timestamp{Time: time.Date(2026, 7, 24, 12, 0, 0, 0, time.UTC)} + + user := &gogithub.User{ + Login: gogithub.Ptr("octocat"), + ID: gogithub.Ptr(int64(1)), + NodeID: gogithub.Ptr("U_kgDOAAAAAQ"), + Type: gogithub.Ptr("User"), + AvatarURL: gogithub.Ptr("https://avatars.githubusercontent.com/u/1"), + HTMLURL: gogithub.Ptr("https://github.com/octocat"), + } + repo := &gogithub.Repository{ + ID: gogithub.Ptr(int64(10)), + NodeID: gogithub.Ptr("R_kgDOAAAACg"), + Name: gogithub.Ptr("repo"), + FullName: gogithub.Ptr("octocat/repo"), + Private: gogithub.Ptr(false), + HTMLURL: gogithub.Ptr("https://github.com/octocat/repo"), + URL: gogithub.Ptr("https://api.github.com/repos/octocat/repo"), + Owner: user, + } + + // Every documented field populated, including the nested $defs + // (repository/user/headCommit/commitAuthor/referencedWorkflow/pullRequest). + richRun := &gogithub.WorkflowRun{ + ID: gogithub.Ptr(int64(123)), + Name: gogithub.Ptr("CI"), + NodeID: gogithub.Ptr("WFR_kwDOAAAAew"), + HeadBranch: gogithub.Ptr("main"), + HeadSHA: gogithub.Ptr("6dcb09b5b57875f334f61aebed695e2e4193db5e"), + Path: gogithub.Ptr(".github/workflows/ci.yml"), + RunNumber: gogithub.Ptr(7), + RunAttempt: gogithub.Ptr(1), + Event: gogithub.Ptr("push"), + DisplayTitle: gogithub.Ptr("Fix the flaky test"), + Status: gogithub.Ptr("completed"), + Conclusion: gogithub.Ptr("success"), + WorkflowID: gogithub.Ptr(int64(1)), + CheckSuiteID: gogithub.Ptr(int64(42)), + CheckSuiteNodeID: gogithub.Ptr("CS_kwDOAAAAKg"), + URL: gogithub.Ptr("https://api.github.com/repos/octocat/repo/actions/runs/123"), + HTMLURL: gogithub.Ptr("https://github.com/octocat/repo/actions/runs/123"), + PullRequests: []*gogithub.PullRequest{{ + ID: gogithub.Ptr(int64(9)), + Number: gogithub.Ptr(3), + URL: gogithub.Ptr("https://api.github.com/repos/octocat/repo/pulls/3"), + }}, + CreatedAt: &ts, + UpdatedAt: &ts, + RunStartedAt: &ts, + JobsURL: gogithub.Ptr("https://api.github.com/repos/octocat/repo/actions/runs/123/jobs"), + LogsURL: gogithub.Ptr("https://api.github.com/repos/octocat/repo/actions/runs/123/logs"), + CheckSuiteURL: gogithub.Ptr("https://api.github.com/repos/octocat/repo/check-suites/42"), + ArtifactsURL: gogithub.Ptr("https://api.github.com/repos/octocat/repo/actions/runs/123/artifacts"), + CancelURL: gogithub.Ptr("https://api.github.com/repos/octocat/repo/actions/runs/123/cancel"), + RerunURL: gogithub.Ptr("https://api.github.com/repos/octocat/repo/actions/runs/123/rerun"), + PreviousAttemptURL: gogithub.Ptr("https://api.github.com/repos/octocat/repo/actions/runs/122"), + HeadCommit: &gogithub.HeadCommit{ + ID: gogithub.Ptr("6dcb09b5b57875f334f61aebed695e2e4193db5e"), + SHA: gogithub.Ptr("6dcb09b5b57875f334f61aebed695e2e4193db5e"), + TreeID: gogithub.Ptr("827efc6d56897b048c772eb4087f854f46256132"), + Message: gogithub.Ptr("Fix the flaky test"), + Timestamp: &ts, + URL: gogithub.Ptr("https://github.com/octocat/repo/commit/6dcb09b"), + Distinct: gogithub.Ptr(true), + Author: &gogithub.CommitAuthor{Name: gogithub.Ptr("Mona"), Email: gogithub.Ptr("mona@github.com"), Date: &ts}, + Committer: &gogithub.CommitAuthor{Name: gogithub.Ptr("Mona"), Email: gogithub.Ptr("mona@github.com"), Date: &ts}, + Added: []string{"a.go"}, + Removed: []string{"b.go"}, + Modified: []string{"c.go"}, + }, + WorkflowURL: gogithub.Ptr("https://api.github.com/repos/octocat/repo/actions/workflows/1"), + Repository: repo, + HeadRepository: repo, + Actor: user, + TriggeringActor: user, + ReferencedWorkflows: []*gogithub.ReferencedWorkflow{{ + Path: gogithub.Ptr("octocat/reusable/.github/workflows/build.yml@main"), + SHA: gogithub.Ptr("6dcb09b5b57875f334f61aebed695e2e4193db5e"), + Ref: gogithub.Ptr("refs/heads/main"), + }}, + } + + richJob := &gogithub.WorkflowJob{ + ID: gogithub.Ptr(int64(555)), + RunID: gogithub.Ptr(int64(456)), + RunURL: gogithub.Ptr("https://api.github.com/repos/octocat/repo/actions/runs/456"), + NodeID: gogithub.Ptr("J_kwDOAAAAIw"), + HeadBranch: gogithub.Ptr("main"), + HeadSHA: gogithub.Ptr("6dcb09b5b57875f334f61aebed695e2e4193db5e"), + URL: gogithub.Ptr("https://api.github.com/repos/octocat/repo/actions/jobs/555"), + HTMLURL: gogithub.Ptr("https://github.com/octocat/repo/actions/runs/456/job/555"), + Status: gogithub.Ptr("completed"), + Conclusion: gogithub.Ptr("success"), + CreatedAt: &ts, + StartedAt: &ts, + CompletedAt: &ts, + Name: gogithub.Ptr("build"), + Steps: []*gogithub.TaskStep{{ + Name: gogithub.Ptr("Set up job"), + Status: gogithub.Ptr("completed"), + Conclusion: gogithub.Ptr("success"), + Number: gogithub.Ptr(int64(1)), + StartedAt: &ts, + CompletedAt: &ts, + }}, + CheckRunURL: gogithub.Ptr("https://api.github.com/repos/octocat/repo/check-runs/555"), + Labels: []string{"ubuntu-latest"}, + RunnerID: gogithub.Ptr(int64(1)), + RunnerName: gogithub.Ptr("hosted-runner"), + RunnerGroupID: gogithub.Ptr(int64(2)), + RunnerGroupName: gogithub.Ptr("GitHub Actions"), + RunAttempt: gogithub.Ptr(int64(1)), + WorkflowName: gogithub.Ptr("CI"), + } + + richArtifact := &gogithub.Artifact{ + ID: gogithub.Ptr(int64(777)), + NodeID: gogithub.Ptr("A_kwDOAAAAAw"), + Name: gogithub.Ptr("build-output"), + SizeInBytes: gogithub.Ptr(int64(1024)), + URL: gogithub.Ptr("https://api.github.com/repos/octocat/repo/actions/artifacts/777"), + ArchiveDownloadURL: gogithub.Ptr("https://api.github.com/repos/octocat/repo/actions/artifacts/777/zip"), + Expired: gogithub.Ptr(false), + CreatedAt: &ts, + UpdatedAt: &ts, + ExpiresAt: &ts, + Digest: gogithub.Ptr("sha256:deadbeef"), + WorkflowRun: &gogithub.ArtifactWorkflowRun{ + ID: gogithub.Ptr(int64(456)), + RepositoryID: gogithub.Ptr(int64(10)), + HeadRepositoryID: gogithub.Ptr(int64(10)), + HeadBranch: gogithub.Ptr("main"), + HeadSHA: gogithub.Ptr("6dcb09b5b57875f334f61aebed695e2e4193db5e"), + }, + } + + // Local types, so this file cannot collide with helpers other tests in the + // package define. + type actionsListCase struct { + name string + args map[string]any + handlers map[string]http.HandlerFunc + } + type actionsListMethodGroup struct { + method string + cases []actionsListCase + } + + listWorkflowsArgs := func() map[string]any { + return map[string]any{"method": actionsMethodListWorkflows, "owner": "octocat", "repo": "repo"} + } + listRunsArgs := func(resourceID string) map[string]any { + a := map[string]any{"method": actionsMethodListWorkflowRuns, "owner": "octocat", "repo": "repo"} + if resourceID != "" { + a["resource_id"] = resourceID + } + return a + } + listJobsArgs := func() map[string]any { + return map[string]any{"method": actionsMethodListWorkflowJobs, "owner": "octocat", "repo": "repo", "resource_id": "456"} + } + listArtifactsArgs := func() map[string]any { + return map[string]any{"method": actionsMethodListWorkflowArtifacts, "owner": "octocat", "repo": "repo", "resource_id": "456"} + } + + groups := []actionsListMethodGroup{ + { + method: actionsMethodListWorkflows, + cases: []actionsListCase{ + { + name: "populated", + args: listWorkflowsArgs(), + handlers: map[string]http.HandlerFunc{ + GetReposActionsWorkflowsByOwnerByRepo: mockResponse(t, http.StatusOK, &gogithub.Workflows{ + TotalCount: gogithub.Ptr(1), + Workflows: []*gogithub.Workflow{{ + ID: gogithub.Ptr(int64(1)), + NodeID: gogithub.Ptr("W_kwDOAAAAAQ"), + Name: gogithub.Ptr("CI"), + Path: gogithub.Ptr(".github/workflows/ci.yml"), + State: gogithub.Ptr("active"), + CreatedAt: &ts, + UpdatedAt: &ts, + URL: gogithub.Ptr("https://api.github.com/repos/octocat/repo/actions/workflows/1"), + HTMLURL: gogithub.Ptr("https://github.com/octocat/repo/actions/workflows/ci.yml"), + BadgeURL: gogithub.Ptr("https://github.com/octocat/repo/workflows/CI/badge.svg"), + }}, + }), + }, + }, + { + // Empty collection. omitempty drops the zero-length slice + // entirely, so the wire payload is {"total_count":0} — the + // envelope's `workflows` key is absent, not []. + name: "empty collection", + args: listWorkflowsArgs(), + handlers: map[string]http.HandlerFunc{ + GetReposActionsWorkflowsByOwnerByRepo: mockResponse(t, http.StatusOK, &gogithub.Workflows{ + TotalCount: gogithub.Ptr(0), + Workflows: []*gogithub.Workflow{}, + }), + }, + }, + { + // Maximally sparse: the envelope carries only the item + // array, and the item carries a single field. Exercises the + // anyOf-over-singleton-required sets rather than a fixture + // that happens to satisfy all of them at once. + name: "sparse, all optional fields absent", + args: listWorkflowsArgs(), + handlers: map[string]http.HandlerFunc{ + GetReposActionsWorkflowsByOwnerByRepo: mockResponse(t, http.StatusOK, &gogithub.Workflows{ + Workflows: []*gogithub.Workflow{{ID: gogithub.Ptr(int64(1))}}, + }), + }, + }, + }, + }, + { + method: actionsMethodListWorkflowRuns, + cases: []actionsListCase{ + { + name: "populated, by workflow file name", + args: listRunsArgs("ci.yml"), + handlers: map[string]http.HandlerFunc{ + GetReposActionsWorkflowsRunsByOwnerByRepoByWorkflowID: mockResponse(t, http.StatusOK, &gogithub.WorkflowRuns{ + TotalCount: gogithub.Ptr(1), + WorkflowRuns: []*gogithub.WorkflowRun{richRun}, + }), + }, + }, + { + // No resource_id routes to a different endpoint + // (ListRepositoryWorkflowRuns) but the same envelope. + name: "populated, whole repository", + args: listRunsArgs(""), + handlers: map[string]http.HandlerFunc{ + GetReposActionsRunsByOwnerByRepo: mockResponse(t, http.StatusOK, &gogithub.WorkflowRuns{ + TotalCount: gogithub.Ptr(1), + WorkflowRuns: []*gogithub.WorkflowRun{richRun}, + }), + }, + }, + { + name: "empty collection", + args: listRunsArgs("ci.yml"), + handlers: map[string]http.HandlerFunc{ + GetReposActionsWorkflowsRunsByOwnerByRepoByWorkflowID: mockResponse(t, http.StatusOK, &gogithub.WorkflowRuns{ + TotalCount: gogithub.Ptr(0), + WorkflowRuns: []*gogithub.WorkflowRun{}, + }), + }, + }, + { + name: "sparse, all optional fields absent", + args: listRunsArgs("ci.yml"), + handlers: map[string]http.HandlerFunc{ + GetReposActionsWorkflowsRunsByOwnerByRepoByWorkflowID: mockResponse(t, http.StatusOK, &gogithub.WorkflowRuns{ + WorkflowRuns: []*gogithub.WorkflowRun{{ID: gogithub.Ptr(int64(123))}}, + }), + }, + }, + { + // An in-progress run has no conclusion; the schema must not + // have quietly made it required. + name: "run without a conclusion", + args: listRunsArgs("ci.yml"), + handlers: map[string]http.HandlerFunc{ + GetReposActionsWorkflowsRunsByOwnerByRepoByWorkflowID: mockResponse(t, http.StatusOK, &gogithub.WorkflowRuns{ + TotalCount: gogithub.Ptr(1), + WorkflowRuns: []*gogithub.WorkflowRun{{ + ID: gogithub.Ptr(int64(456)), + Status: gogithub.Ptr("in_progress"), + }}, + }), + }, + }, + }, + }, + { + method: actionsMethodListWorkflowJobs, + cases: []actionsListCase{ + { + // Double-nested: the handler wraps the github.Jobs envelope + // under an outer `jobs` key. + name: "populated", + args: listJobsArgs(), + handlers: map[string]http.HandlerFunc{ + GetReposActionsRunsJobsByOwnerByRepoByRunID: mockResponse(t, http.StatusOK, &gogithub.Jobs{ + TotalCount: gogithub.Ptr(1), + Jobs: []*gogithub.WorkflowJob{richJob}, + }), + }, + }, + { + name: "empty collection", + args: listJobsArgs(), + handlers: map[string]http.HandlerFunc{ + GetReposActionsRunsJobsByOwnerByRepoByRunID: mockResponse(t, http.StatusOK, &gogithub.Jobs{ + TotalCount: gogithub.Ptr(0), + Jobs: []*gogithub.WorkflowJob{}, + }), + }, + }, + { + // Every field of the inner envelope absent: {"jobs":{}}. + // The schema keeps this reachable via maxProperties:0. + name: "empty inner envelope", + args: listJobsArgs(), + handlers: map[string]http.HandlerFunc{ + GetReposActionsRunsJobsByOwnerByRepoByRunID: mockResponse(t, http.StatusOK, &gogithub.Jobs{}), + }, + }, + { + // go-github decodes into a **Jobs, so a `null` body leaves + // the pointer nil and the handler emits {"jobs":null}. + name: "null envelope from an empty body", + args: listJobsArgs(), + handlers: map[string]http.HandlerFunc{ + GetReposActionsRunsJobsByOwnerByRepoByRunID: func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte(`null`)) + }, + }, + }, + { + name: "sparse, all optional fields absent", + args: listJobsArgs(), + handlers: map[string]http.HandlerFunc{ + GetReposActionsRunsJobsByOwnerByRepoByRunID: mockResponse(t, http.StatusOK, &gogithub.Jobs{ + Jobs: []*gogithub.WorkflowJob{{ID: gogithub.Ptr(int64(555))}}, + }), + }, + }, + }, + }, + { + method: actionsMethodListWorkflowArtifacts, + cases: []actionsListCase{ + { + name: "populated", + args: listArtifactsArgs(), + handlers: map[string]http.HandlerFunc{ + GetReposActionsRunsArtifactsByOwnerByRepoByRunID: mockResponse(t, http.StatusOK, &gogithub.ArtifactList{ + TotalCount: gogithub.Ptr(int64(1)), + Artifacts: []*gogithub.Artifact{richArtifact}, + }), + }, + }, + { + name: "empty collection", + args: listArtifactsArgs(), + handlers: map[string]http.HandlerFunc{ + GetReposActionsRunsArtifactsByOwnerByRepoByRunID: mockResponse(t, http.StatusOK, &gogithub.ArtifactList{ + TotalCount: gogithub.Ptr(int64(0)), + Artifacts: []*gogithub.Artifact{}, + }), + }, + }, + { + // digest is null for artifacts from upload-artifact v3 and + // older, which is the same wire shape as absent here. + name: "sparse, all optional fields absent", + args: listArtifactsArgs(), + handlers: map[string]http.HandlerFunc{ + GetReposActionsRunsArtifactsByOwnerByRepoByRunID: mockResponse(t, http.StatusOK, &gogithub.ArtifactList{ + Artifacts: []*gogithub.Artifact{{ID: gogithub.Ptr(int64(777))}}, + }), + }, + }, + }, + }, + } + + // Guards against a method being added to the enum without a subtest here. + require.Len(t, groups, 4, "every value of the actions_list method enum needs a subtest") + + for _, group := range groups { + t.Run(group.method, func(t *testing.T) { + for _, tc := range group.cases { + t.Run(tc.name, func(t *testing.T) { + deps := BaseDeps{Client: mustNewGHClient(t, MockHTTPClientWithHandlers(tc.handlers))} + handler := serverTool.Handler(deps) + + request := createMCPRequest(tc.args) + result, err := handler(ContextWithDeps(context.Background(), deps), &request) + require.NoError(t, err) + require.False(t, result.IsError, + "handler must succeed, otherwise this test would validate an error string instead of real output") + + text := getTextResult(t, result) + var payload any + require.NoError(t, json.Unmarshal([]byte(text.Text), &payload), + "handler output must be JSON for the mirror to publish it as structuredContent: %s", text.Text) + + require.NoError(t, resolved.Validate(payload), + "real handler output for method=%s must conform to the advertised outputSchema: %s", + group.method, text.Text) + }) + } + }) + } +} + +// The enum the tool advertises is the contract this file claims to cover; if a +// method is added, the conformance table above must grow with it. +func TestActionsListMethodEnumIsFullyCovered(t *testing.T) { + t.Parallel() + + inputSchema := ActionsList(translations.NullTranslationHelper).Tool.InputSchema.(*jsonschema.Schema) + methodEnum := inputSchema.Properties["method"].Enum + + covered := map[string]bool{ + actionsMethodListWorkflows: true, + actionsMethodListWorkflowRuns: true, + actionsMethodListWorkflowJobs: true, + actionsMethodListWorkflowArtifacts: true, + } + + require.Len(t, methodEnum, len(covered)) + for _, m := range methodEnum { + name, ok := m.(string) + require.True(t, ok) + require.True(t, covered[name], + "method %q is in the actions_list enum but has no output-conformance subtest", name) + } +} diff --git a/pkg/github/output_conformance_actions_run_trigger_test.go b/pkg/github/output_conformance_actions_run_trigger_test.go new file mode 100644 index 0000000000..d03e0ac388 --- /dev/null +++ b/pkg/github/output_conformance_actions_run_trigger_test.go @@ -0,0 +1,221 @@ +package github + +import ( + "context" + "encoding/json" + "maps" + "net/http" + "testing" + + "github.com/google/jsonschema-go/jsonschema" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/github/github-mcp-server/pkg/translations" +) + +// The end-to-end guarantee for actions_run_trigger: what the real handler +// actually emits must validate against the schema the tool advertises. The +// structured-content mirror publishes the exact bytes of the text block as +// structuredContent, so validating the text block here is equivalent to +// validating what a client receives. +// +// Every value of the tool's `method` enum is exercised; the subtest table is +// cross-checked against the enum at the end so a newly added method cannot slip +// through uncovered. +func TestActionsRunTriggerOutputValidatesAgainstDeclaredSchema(t *testing.T) { + t.Parallel() + + serverTool := ActionsRunTrigger(translations.NullTranslationHelper) + resolved := resolveToolSchema(t, actionsRunTriggerOutputSchema) + + // 204 No Content — what POST .../dispatches really returns. + noContent := func(w http.ResponseWriter, _ *http.Request) { w.WriteHeader(http.StatusNoContent) } + // 201 Created — what the rerun endpoints really return. + created := func(w http.ResponseWriter, _ *http.Request) { w.WriteHeader(http.StatusCreated) } + // 202 Accepted — what POST .../cancel really returns. go-github surfaces + // this as an *AcceptedError, which the handler deliberately swallows, so + // this case also pins that the "error that isn't an error" path still + // produces schema-conformant output. + accepted := func(w http.ResponseWriter, _ *http.Request) { w.WriteHeader(http.StatusAccepted) } + + tests := []struct { + name string + // method is the enum value under test; used for coverage bookkeeping. + method string + args map[string]any + handlers map[string]http.HandlerFunc + wantMessage string + // wantFields are keys the payload must carry, asserted so a handler + // that silently stopped emitting a required field cannot pass merely + // because some other anyOf branch happens to accept the remainder. + wantFields []string + // wantStatusCode pins the HTTP status the handler echoes back. Note + // that the sibling `status` string cannot be pinned here: the mock + // transport builds an http.Response without a Status line, so + // resp.Status is "" under test where production would say + // "204 No Content". The schema only constrains it to a string, so + // both validate. + wantStatusCode int + }{ + { + name: "run_workflow", + method: actionsMethodRunWorkflow, + args: map[string]any{ + "workflow_id": "12345", + "ref": "main", + "inputs": map[string]any{"FIELD1": "value1"}, + }, + handlers: map[string]http.HandlerFunc{ + PostReposActionsWorkflowsDispatchesByOwnerByRepoByWorkflowID: noContent, + }, + wantMessage: "Workflow run has been queued", + wantFields: []string{"message", "workflow_type", "workflow_id", "ref", "inputs", "status", "status_code"}, + wantStatusCode: http.StatusNoContent, + }, + { + // The other addressing mode: a workflow file name rather than a + // numeric ID, which flips workflow_type to "workflow_file". The + // schema pins that enum, so both values need exercising. + name: "run_workflow by file name", + method: actionsMethodRunWorkflow, + args: map[string]any{ + "workflow_id": "ci.yaml", + "ref": "refs/heads/main", + "inputs": map[string]any{}, + }, + handlers: map[string]http.HandlerFunc{ + PostReposActionsWorkflowsDispatchesByOwnerByRepoByWorkflowID: noContent, + }, + wantMessage: "Workflow run has been queued", + wantFields: []string{"message", "workflow_type", "workflow_id", "ref", "inputs", "status", "status_code"}, + wantStatusCode: http.StatusNoContent, + }, + { + // The sparse case: every optional argument absent. `inputs` is a + // nil map here, which marshals to a literal JSON null while still + // being a REQUIRED property of the run_workflow branch — exactly + // the combination that exercises the branch's required set rather + // than a convenient fixture. + name: "run_workflow with all optionals absent", + method: actionsMethodRunWorkflow, + args: map[string]any{ + "workflow_id": "12345", + "ref": "main", + }, + handlers: map[string]http.HandlerFunc{ + PostReposActionsWorkflowsDispatchesByOwnerByRepoByWorkflowID: noContent, + }, + wantMessage: "Workflow run has been queued", + wantFields: []string{"message", "workflow_type", "workflow_id", "ref", "inputs", "status", "status_code"}, + wantStatusCode: http.StatusNoContent, + }, + { + name: "rerun_workflow_run", + method: actionsMethodRerunWorkflowRun, + args: map[string]any{"run_id": float64(12345)}, + handlers: map[string]http.HandlerFunc{PostReposActionsRunsRerunByOwnerByRepoByRunID: created}, + // All four acknowledgement methods share a branch; only the text + // differs, so pinning the text is what proves the right code path + // ran rather than a neighbouring one. + wantMessage: "Workflow run has been queued for re-run", + wantFields: []string{"message", "run_id", "status", "status_code"}, + wantStatusCode: http.StatusCreated, + }, + { + name: "rerun_failed_jobs", + method: actionsMethodRerunFailedJobs, + args: map[string]any{"run_id": float64(12345)}, + handlers: map[string]http.HandlerFunc{PostReposActionsRunsRerunFailedJobsByOwnerByRepoByRunID: created}, + wantMessage: "Failed jobs have been queued for re-run", + wantFields: []string{"message", "run_id", "status", "status_code"}, + wantStatusCode: http.StatusCreated, + }, + { + name: "cancel_workflow_run", + method: actionsMethodCancelWorkflowRun, + args: map[string]any{"run_id": float64(12345)}, + handlers: map[string]http.HandlerFunc{PostReposActionsRunsCancelByOwnerByRepoByRunID: accepted}, + wantMessage: "Workflow run has been cancelled", + wantFields: []string{"message", "run_id", "status", "status_code"}, + wantStatusCode: http.StatusAccepted, + }, + { + name: "delete_workflow_run_logs", + method: actionsMethodDeleteWorkflowRunLogs, + args: map[string]any{"run_id": float64(12345)}, + handlers: map[string]http.HandlerFunc{DeleteReposActionsRunsLogsByOwnerByRepoByRunID: noContent}, + wantMessage: "Workflow run logs have been deleted", + wantFields: []string{"message", "run_id", "status", "status_code"}, + wantStatusCode: http.StatusNoContent, + }, + } + + covered := map[string]bool{} + + for _, tt := range tests { + covered[tt.method] = true + + t.Run(tt.name, func(t *testing.T) { + deps := BaseDeps{Client: mustNewGHClient(t, MockHTTPClientWithHandlers(tt.handlers))} + handler := serverTool.Handler(deps) + + args := map[string]any{ + "method": tt.method, + "owner": "octocat", + "repo": "repo", + } + maps.Copy(args, tt.args) + + request := createMCPRequest(args) + result, err := handler(ContextWithDeps(context.Background(), deps), &request) + require.NoError(t, err) + require.False(t, result.IsError, "handler should succeed; a failing handler must not masquerade as conformance") + + text := getTextResult(t, result) + + // Unmarshal before validating: the mirror can only publish + // structuredContent if the text block is JSON in the first place. + var payload any + require.NoError(t, json.Unmarshal([]byte(text.Text), &payload), + "handler output must be JSON for the mirror to publish it as structuredContent") + + obj, ok := payload.(map[string]any) + require.True(t, ok, "actions_run_trigger declares an object root; got %T", payload) + for _, field := range tt.wantFields { + assert.Contains(t, obj, field, + "method=%s must emit %q, which the schema branch lists as required", tt.method, field) + } + assert.Equal(t, tt.wantMessage, obj["message"], + "payload must come from the handler for method=%s, not a neighbouring code path", tt.method) + assert.Equal(t, float64(tt.wantStatusCode), obj["status_code"], + "status_code must echo the upstream HTTP status for method=%s", tt.method) + + require.NoError(t, resolved.Validate(payload), + "real handler output for method=%s must conform to the advertised outputSchema", tt.method) + }) + } + + // Coverage is only meaningful if it tracks the enum. A method added to the + // tool without a case here would otherwise ship unvalidated. + t.Run("every method enum value is covered", func(t *testing.T) { + inputSchema, ok := serverTool.Tool.InputSchema.(*jsonschema.Schema) + require.True(t, ok, "expected a *jsonschema.Schema input schema") + methodSchema, ok := inputSchema.Properties["method"] + require.True(t, ok, "actions_run_trigger must declare a method property") + require.NotEmpty(t, methodSchema.Enum, "method must be a closed enum") + + for _, v := range methodSchema.Enum { + name, ok := v.(string) + require.True(t, ok, "method enum values must be strings, got %T", v) + assert.True(t, covered[name], + "method %q is advertised in the enum but its real output is never validated against the schema", name) + } + + // And nothing here tests a method the tool no longer advertises. + for name := range covered { + assert.Contains(t, methodSchema.Enum, any(name), + "this test exercises method %q, which the tool no longer advertises", name) + } + }) +} diff --git a/pkg/github/output_conformance_discussion_comment_write_test.go b/pkg/github/output_conformance_discussion_comment_write_test.go new file mode 100644 index 0000000000..00b440f414 --- /dev/null +++ b/pkg/github/output_conformance_discussion_comment_write_test.go @@ -0,0 +1,465 @@ +package github + +import ( + "context" + "encoding/json" + "fmt" + "net/http" + "testing" + + "github.com/shurcooL/githubv4" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/github/github-mcp-server/internal/githubv4mock" + "github.com/github/github-mcp-server/pkg/translations" + "github.com/google/jsonschema-go/jsonschema" +) + +// The end-to-end guarantee for discussion_comment_write: what the real handler +// emits must validate against the outputSchema the tool advertises. The +// structured-content mirror publishes the exact bytes of the text block as +// structuredContent, so validating the text block here is equivalent to +// validating what a client receives. +// +// Every value of the tool's `method` enum is exercised, and the enum itself is +// read back off the tool at the end so a newly added method cannot slip through +// without a conformance case. +func TestDiscussionCommentWriteOutputValidatesAgainstDeclaredSchema(t *testing.T) { + t.Parallel() + + serverTool := DiscussionCommentWrite(translations.NullTranslationHelper) + resolved := resolveToolSchema(t, discussionCommentWriteOutputSchema) + + // Resolving the discussion node ID from its number: shared by add and reply. + discussionQueryMatcher := discussionCommentWriteDiscussionQueryMatcher( + 1, + githubv4mock.DataResponse(map[string]any{ + "repository": map[string]any{ + "discussion": map[string]any{"id": "D_kwDOTest123"}, + }, + }), + ) + + // The reply path validates the target comment node before mutating. + replyValidationMatcher := discussionCommentWriteReplyValidationQueryMatcher( + "DC_kwDOComment456", + githubv4mock.DataResponse(map[string]any{ + "node": map[string]any{ + "id": "DC_kwDOComment456", + "discussion": map[string]any{"id": "D_kwDOTest123"}, + }, + }), + ) + + tests := []struct { + name string + method string + requestArgs map[string]any + mockedClient *http.Client + }{ + { + name: "add", + method: "add", + requestArgs: map[string]any{ + "method": "add", + "owner": "owner", + "repo": "repo", + "discussionNumber": int32(1), + "body": "This is a test comment", + }, + mockedClient: githubv4mock.NewMockedHTTPClient( + discussionQueryMatcher, + discussioncommentwriteAddMutationMatcher( + githubv4.AddDiscussionCommentInput{ + DiscussionID: githubv4.ID("D_kwDOTest123"), + Body: githubv4.String("This is a test comment"), + }, + githubv4mock.DataResponse(map[string]any{ + "addDiscussionComment": map[string]any{ + "comment": map[string]any{ + "id": "DC_kwDOComment456", + "url": "https://github.com/owner/repo/discussions/1#discussioncomment-456", + }, + }, + }), + ), + ), + }, + { + // The sparse case: the mutation payload comes back with the comment + // object entirely absent, so every field the handler reads is unset. + // Both keys are required by the schema branch, so this is what + // proves the handler always emits them rather than omitting on zero. + // + // It also pins a wart: githubv4.ID is `any`, and the handler + // stringifies it with fmt.Sprintf("%v", ...), so an unset ID emits + // the literal string "" rather than "". That conforms (the + // schema says string, and it is one), so this test cannot fail on + // it — but it is the shape a client would actually receive. + name: "add with mutation payload fields absent", + method: "add", + requestArgs: map[string]any{ + "method": "add", + "owner": "owner", + "repo": "repo", + "discussionNumber": int32(1), + "body": "This is a test comment", + }, + mockedClient: githubv4mock.NewMockedHTTPClient( + discussionQueryMatcher, + discussioncommentwriteAddMutationMatcher( + githubv4.AddDiscussionCommentInput{ + DiscussionID: githubv4.ID("D_kwDOTest123"), + Body: githubv4.String("This is a test comment"), + }, + githubv4mock.DataResponse(map[string]any{ + "addDiscussionComment": map[string]any{"comment": nil}, + }), + ), + ), + }, + { + name: "reply", + method: "reply", + requestArgs: map[string]any{ + "method": "reply", + "owner": "owner", + "repo": "repo", + "discussionNumber": int32(1), + "body": "This is a reply", + "commentNodeID": "DC_kwDOComment456", + }, + mockedClient: githubv4mock.NewMockedHTTPClient( + replyValidationMatcher, + discussionQueryMatcher, + discussioncommentwriteAddMutationMatcher( + githubv4.AddDiscussionCommentInput{ + DiscussionID: githubv4.ID("D_kwDOTest123"), + Body: githubv4.String("This is a reply"), + ReplyToID: githubv4ptr("DC_kwDOComment456"), + }, + githubv4mock.DataResponse(map[string]any{ + "addDiscussionComment": map[string]any{ + "comment": map[string]any{ + "id": "DC_kwDOReply789", + "url": "https://github.com/owner/repo/discussions/1#discussioncomment-789", + }, + }, + }), + ), + ), + }, + { + name: "reply with mutation payload fields absent", + method: "reply", + requestArgs: map[string]any{ + "method": "reply", + "owner": "owner", + "repo": "repo", + "discussionNumber": int32(1), + "body": "This is a reply", + "commentNodeID": "DC_kwDOComment456", + }, + mockedClient: githubv4mock.NewMockedHTTPClient( + replyValidationMatcher, + discussionQueryMatcher, + discussioncommentwriteAddMutationMatcher( + githubv4.AddDiscussionCommentInput{ + DiscussionID: githubv4.ID("D_kwDOTest123"), + Body: githubv4.String("This is a reply"), + ReplyToID: githubv4ptr("DC_kwDOComment456"), + }, + githubv4mock.DataResponse(map[string]any{ + "addDiscussionComment": map[string]any{"comment": nil}, + }), + ), + ), + }, + { + name: "update", + method: "update", + requestArgs: map[string]any{ + "method": "update", + "commentNodeID": "DC_kwDOComment456", + "body": "Updated comment body", + }, + mockedClient: githubv4mock.NewMockedHTTPClient( + discussioncommentwriteUpdateMutationMatcher( + githubv4.UpdateDiscussionCommentInput{ + CommentID: githubv4.ID("DC_kwDOComment456"), + Body: githubv4.String("Updated comment body"), + }, + githubv4mock.DataResponse(map[string]any{ + "updateDiscussionComment": map[string]any{ + "comment": map[string]any{ + "id": "DC_kwDOComment456", + "url": "https://github.com/owner/repo/discussions/1#discussioncomment-456", + }, + }, + }), + ), + ), + }, + { + // Empty strings rather than an absent object: the other way a + // payload can be maximally uninformative while still well-formed. + name: "update with empty id and url", + method: "update", + requestArgs: map[string]any{ + "method": "update", + "commentNodeID": "DC_kwDOComment456", + "body": "Updated comment body", + }, + mockedClient: githubv4mock.NewMockedHTTPClient( + discussioncommentwriteUpdateMutationMatcher( + githubv4.UpdateDiscussionCommentInput{ + CommentID: githubv4.ID("DC_kwDOComment456"), + Body: githubv4.String("Updated comment body"), + }, + githubv4mock.DataResponse(map[string]any{ + "updateDiscussionComment": map[string]any{ + "comment": map[string]any{"id": "", "url": ""}, + }, + }), + ), + ), + }, + { + name: "delete", + method: "delete", + requestArgs: map[string]any{ + "method": "delete", + "commentNodeID": "DC_kwDOComment456", + }, + mockedClient: githubv4mock.NewMockedHTTPClient( + discussioncommentwriteDeleteMutationMatcher( + githubv4.DeleteDiscussionCommentInput{ID: githubv4.ID("DC_kwDOComment456")}, + githubv4mock.DataResponse(map[string]any{ + "deleteDiscussionComment": map[string]any{ + "comment": map[string]any{ + "id": "DC_kwDOComment456", + "url": "https://github.com/owner/repo/discussions/1#discussioncomment-456", + }, + }, + }), + ), + ), + }, + { + name: "delete with mutation payload fields absent", + method: "delete", + requestArgs: map[string]any{ + "method": "delete", + "commentNodeID": "DC_kwDOComment456", + }, + mockedClient: githubv4mock.NewMockedHTTPClient( + discussioncommentwriteDeleteMutationMatcher( + githubv4.DeleteDiscussionCommentInput{ID: githubv4.ID("DC_kwDOComment456")}, + githubv4mock.DataResponse(map[string]any{ + "deleteDiscussionComment": map[string]any{"comment": nil}, + }), + ), + ), + }, + { + name: "mark_answer", + method: "mark_answer", + requestArgs: map[string]any{ + "method": "mark_answer", + "commentNodeID": "DC_kwDOComment456", + }, + mockedClient: githubv4mock.NewMockedHTTPClient( + discussioncommentwriteMarkAnswerMutationMatcher( + githubv4.MarkDiscussionCommentAsAnswerInput{ID: githubv4.ID("DC_kwDOComment456")}, + githubv4mock.DataResponse(map[string]any{ + "markDiscussionCommentAsAnswer": map[string]any{ + "discussion": map[string]any{ + "id": "D_kwDOTest123", + "url": "https://github.com/owner/repo/discussions/1", + }, + }, + }), + ), + ), + }, + { + // Exercises the second anyOf branch's required set + // (discussionID + discussionURL) with nothing to fill it from. + name: "mark_answer with mutation payload fields absent", + method: "mark_answer", + requestArgs: map[string]any{ + "method": "mark_answer", + "commentNodeID": "DC_kwDOComment456", + }, + mockedClient: githubv4mock.NewMockedHTTPClient( + discussioncommentwriteMarkAnswerMutationMatcher( + githubv4.MarkDiscussionCommentAsAnswerInput{ID: githubv4.ID("DC_kwDOComment456")}, + githubv4mock.DataResponse(map[string]any{ + "markDiscussionCommentAsAnswer": map[string]any{"discussion": nil}, + }), + ), + ), + }, + { + name: "unmark_answer", + method: "unmark_answer", + requestArgs: map[string]any{ + "method": "unmark_answer", + "commentNodeID": "DC_kwDOComment456", + }, + mockedClient: githubv4mock.NewMockedHTTPClient( + discussioncommentwriteUnmarkAnswerMutationMatcher( + githubv4.UnmarkDiscussionCommentAsAnswerInput{ID: githubv4.ID("DC_kwDOComment456")}, + githubv4mock.DataResponse(map[string]any{ + "unmarkDiscussionCommentAsAnswer": map[string]any{ + "discussion": map[string]any{ + "id": "D_kwDOTest123", + "url": "https://github.com/owner/repo/discussions/1", + }, + }, + }), + ), + ), + }, + { + name: "unmark_answer with empty discussion id and url", + method: "unmark_answer", + requestArgs: map[string]any{ + "method": "unmark_answer", + "commentNodeID": "DC_kwDOComment456", + }, + mockedClient: githubv4mock.NewMockedHTTPClient( + discussioncommentwriteUnmarkAnswerMutationMatcher( + githubv4.UnmarkDiscussionCommentAsAnswerInput{ID: githubv4.ID("DC_kwDOComment456")}, + githubv4mock.DataResponse(map[string]any{ + "unmarkDiscussionCommentAsAnswer": map[string]any{ + "discussion": map[string]any{"id": "", "url": ""}, + }, + }), + ), + ), + }, + } + + covered := map[string]bool{} + for _, tt := range tests { + covered[tt.method] = true + + t.Run(tt.name, func(t *testing.T) { + deps := BaseDeps{GQLClient: githubv4.NewClient(tt.mockedClient)} + handler := serverTool.Handler(deps) + + request := createMCPRequest(tt.requestArgs) + result, err := handler(ContextWithDeps(context.Background(), deps), &request) + require.NoError(t, err) + require.False(t, result.IsError, "handler should succeed; a tool error would validate vacuously") + + text := getTextResult(t, result) + var payload any + require.NoError(t, json.Unmarshal([]byte(text.Text), &payload), + "handler output must be JSON for the mirror to publish it as structuredContent") + + require.NoError(t, resolved.Validate(payload), + "real handler output for method=%s must conform to the advertised outputSchema: %s", tt.method, text.Text) + }) + } + + // No method of this tool returns a collection, so there is no empty-list + // case to exercise; the awkward cases here are the absent/empty payload + // fields above. Guard the enum instead: a method added to the tool without + // a conformance case is exactly the drift this file exists to catch. + t.Run("every method in the enum is covered", func(t *testing.T) { + schema, ok := serverTool.Tool.InputSchema.(*jsonschema.Schema) + require.True(t, ok, "InputSchema should be *jsonschema.Schema") + methodSchema, ok := schema.Properties["method"] + require.True(t, ok, "tool should declare a method property") + require.NotEmpty(t, methodSchema.Enum, "method should be an enum") + + for _, v := range methodSchema.Enum { + method := fmt.Sprintf("%v", v) + assert.True(t, covered[method], + "method %q is advertised by the tool but has no output-conformance case", method) + } + }) +} + +func discussioncommentwriteAddMutationMatcher(input githubv4.AddDiscussionCommentInput, response githubv4mock.GQLResponse) githubv4mock.Matcher { + return githubv4mock.NewMutationMatcher( + struct { + AddDiscussionComment struct { + Comment struct { + ID githubv4.ID + URL githubv4.String `graphql:"url"` + } + } `graphql:"addDiscussionComment(input: $input)"` + }{}, + input, + nil, + response, + ) +} + +func discussioncommentwriteUpdateMutationMatcher(input githubv4.UpdateDiscussionCommentInput, response githubv4mock.GQLResponse) githubv4mock.Matcher { + return githubv4mock.NewMutationMatcher( + struct { + UpdateDiscussionComment struct { + Comment struct { + ID githubv4.ID + URL githubv4.String `graphql:"url"` + } + } `graphql:"updateDiscussionComment(input: $input)"` + }{}, + input, + nil, + response, + ) +} + +func discussioncommentwriteDeleteMutationMatcher(input githubv4.DeleteDiscussionCommentInput, response githubv4mock.GQLResponse) githubv4mock.Matcher { + return githubv4mock.NewMutationMatcher( + struct { + DeleteDiscussionComment struct { + Comment struct { + ID githubv4.ID + URL githubv4.String `graphql:"url"` + } + } `graphql:"deleteDiscussionComment(input: $input)"` + }{}, + input, + nil, + response, + ) +} + +func discussioncommentwriteMarkAnswerMutationMatcher(input githubv4.MarkDiscussionCommentAsAnswerInput, response githubv4mock.GQLResponse) githubv4mock.Matcher { + return githubv4mock.NewMutationMatcher( + struct { + MarkDiscussionCommentAsAnswer struct { + Discussion struct { + ID githubv4.ID + URL githubv4.String `graphql:"url"` + } + } `graphql:"markDiscussionCommentAsAnswer(input: $input)"` + }{}, + input, + nil, + response, + ) +} + +func discussioncommentwriteUnmarkAnswerMutationMatcher(input githubv4.UnmarkDiscussionCommentAsAnswerInput, response githubv4mock.GQLResponse) githubv4mock.Matcher { + return githubv4mock.NewMutationMatcher( + struct { + UnmarkDiscussionCommentAsAnswer struct { + Discussion struct { + ID githubv4.ID + URL githubv4.String `graphql:"url"` + } + } `graphql:"unmarkDiscussionCommentAsAnswer(input: $input)"` + }{}, + input, + nil, + response, + ) +} diff --git a/pkg/github/output_conformance_issue_dependency_read_test.go b/pkg/github/output_conformance_issue_dependency_read_test.go new file mode 100644 index 0000000000..f7853cff12 --- /dev/null +++ b/pkg/github/output_conformance_issue_dependency_read_test.go @@ -0,0 +1,285 @@ +package github + +import ( + "context" + "encoding/json" + "net/http" + "strings" + "testing" + + gogithub "github.com/google/go-github/v89/github" + "github.com/google/jsonschema-go/jsonschema" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/github/github-mcp-server/pkg/translations" +) + +// The end-to-end guarantee for issue_dependency_read: what the real handler +// emits must validate against the outputSchema the tool advertises. The +// structured-content mirror publishes the exact bytes of the single text block +// as structuredContent, so validating the text block here is equivalent to +// validating what a client receives. +// +// Every value of the tool's `method` enum is covered, and the enum itself is +// pinned below so adding a method without adding a case fails the test rather +// than silently shipping an unvalidated branch. +func TestIssueDependencyReadOutputValidatesAgainstDeclaredSchema(t *testing.T) { + t.Parallel() + + serverTool := IssueDependencyRead(translations.NullTranslationHelper) + resolved := resolveToolSchema(t, issueDependencyReadOutputSchema) + + // A fully populated issue: every field the projection reads is present. + fullIssue := &gogithub.Issue{ + Number: gogithub.Ptr(7), + Title: gogithub.Ptr("Blocker"), + State: gogithub.Ptr("open"), + HTMLURL: gogithub.Ptr("https://github.com/owner/repo/issues/7"), + RepositoryURL: gogithub.Ptr("https://api.github.com/repos/owner/repo"), + } + // A second issue in a different repo, closed, to exercise the upper-casing + // of state and a cross-repo "owner/repo" derivation in the same payload. + crossRepoIssue := &gogithub.Issue{ + Number: gogithub.Ptr(9), + Title: gogithub.Ptr("Blocked elsewhere"), + State: gogithub.Ptr("closed"), + HTMLURL: gogithub.Ptr("https://github.com/other/thing/issues/9"), + RepositoryURL: gogithub.Ptr("https://api.github.com/repos/other/thing"), + } + + // Advertises a next page so pageInfo.hasNextPage/nextPage are non-zero + // rather than always taking the same branch. + withNextPage := func(body any) http.HandlerFunc { + return func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Link", `; rel="next"`) + w.WriteHeader(http.StatusOK) + _, _ = w.Write(MustMarshal(body)) + } + } + + tests := []struct { + name string + method string + handlers map[string]http.HandlerFunc + // Shape expectations, so a case cannot pass by validating a payload + // that never contained what the case is named for. + wantIssues int + wantNextPage bool + wantNoRepository bool + }{ + { + name: "get_blocked_by", + method: "get_blocked_by", + handlers: map[string]http.HandlerFunc{ + string(endpointBlockedBy): mockResponse(t, http.StatusOK, []*gogithub.Issue{fullIssue, crossRepoIssue}), + }, + wantIssues: 2, + }, + { + // Empty collection: the list method returning zero items. `issues` + // is still required, so [] must be emitted (never null, never absent). + name: "get_blocked_by with no blockers", + method: "get_blocked_by", + handlers: map[string]http.HandlerFunc{ + string(endpointBlockedBy): mockResponse(t, http.StatusOK, []*gogithub.Issue{}), + }, + wantIssues: 0, + }, + { + // A fully sparse issue: every optional field absent. `repository` + // is omitempty and drops out entirely, so this exercises the + // minimalIssueRef required set rather than a convenient fixture. + name: "get_blocked_by with all optionals absent", + method: "get_blocked_by", + handlers: map[string]http.HandlerFunc{ + string(endpointBlockedBy): mockResponse(t, http.StatusOK, []*gogithub.Issue{{}}), + }, + wantIssues: 1, + wantNoRepository: true, + }, + { + name: "get_blocked_by with next page", + method: "get_blocked_by", + handlers: map[string]http.HandlerFunc{ + string(endpointBlockedBy): withNextPage([]*gogithub.Issue{fullIssue}), + }, + wantIssues: 1, + wantNextPage: true, + }, + { + name: "get_blocking", + method: "get_blocking", + handlers: map[string]http.HandlerFunc{ + string(endpointBlocking): mockResponse(t, http.StatusOK, []*gogithub.Issue{fullIssue, crossRepoIssue}), + }, + wantIssues: 2, + }, + { + name: "get_blocking with none", + method: "get_blocking", + handlers: map[string]http.HandlerFunc{ + string(endpointBlocking): mockResponse(t, http.StatusOK, []*gogithub.Issue{}), + }, + wantIssues: 0, + }, + { + name: "get_blocking with all optionals absent", + method: "get_blocking", + handlers: map[string]http.HandlerFunc{ + string(endpointBlocking): mockResponse(t, http.StatusOK, []*gogithub.Issue{{}}), + }, + wantIssues: 1, + wantNoRepository: true, + }, + { + name: "get_blocking with next page", + method: "get_blocking", + handlers: map[string]http.HandlerFunc{ + string(endpointBlocking): withNextPage([]*gogithub.Issue{crossRepoIssue}), + }, + wantIssues: 1, + wantNextPage: true, + }, + } + + // Pin the enum: a new method must gain a case above, not slip through. + covered := map[string]bool{} + for _, tt := range tests { + covered[tt.method] = true + } + inputSchema, ok := serverTool.Tool.InputSchema.(*jsonschema.Schema) + require.True(t, ok, "input schema should be a *jsonschema.Schema") + for _, m := range inputSchema.Properties["method"].Enum { + assert.True(t, covered[m.(string)], + "method %q is advertised in the enum but has no output-conformance case", m) + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + deps := BaseDeps{Client: mustNewGHClient(t, MockHTTPClientWithHandlers(tt.handlers))} + handler := serverTool.Handler(deps) + + request := createMCPRequest(map[string]any{ + "method": tt.method, + "owner": "owner", + "repo": "repo", + "issue_number": float64(123), + }) + result, err := handler(ContextWithDeps(context.Background(), deps), &request) + require.NoError(t, err) + require.False(t, result.IsError, "handler should succeed") + + text := getTextResult(t, result) + + // A nil slice marshalled as `null` would violate the schema's + // required `issues` array; the handler normalises to []. + assert.NotEqual(t, "null", strings.TrimSpace(text.Text)) + + var payload any + require.NoError(t, json.Unmarshal([]byte(text.Text), &payload), + "handler output must be JSON for the mirror to publish it as structuredContent") + + require.NoError(t, resolved.Validate(payload), + "real handler output for method=%s must conform to the advertised outputSchema", tt.method) + + // Prove the payload really carried what this case is named for; + // otherwise an always-empty response would validate trivially. + var shape struct { + Issues []map[string]any `json:"issues"` + PageInfo struct { + HasNextPage bool `json:"hasNextPage"` + NextPage int `json:"nextPage"` + } `json:"pageInfo"` + } + require.NoError(t, json.Unmarshal([]byte(text.Text), &shape)) + require.Len(t, shape.Issues, tt.wantIssues) + assert.Equal(t, tt.wantNextPage, shape.PageInfo.HasNextPage) + if tt.wantNextPage { + assert.NotZero(t, shape.PageInfo.NextPage) + } else { + assert.Zero(t, shape.PageInfo.NextPage) + } + if tt.wantNoRepository { + for _, issue := range shape.Issues { + assert.NotContains(t, issue, "repository", + "the sparse case must actually omit the optional field it is testing") + } + } + }) + } + + // The cases above are only evidence if the validator can fail. Pin the + // shapes this schema must reject, so a future widening that makes every + // payload pass shows up here rather than as a silent loss of coverage. + t.Run("schema rejects drifted output", func(t *testing.T) { + valid := map[string]any{ + "number": float64(7), "title": "Blocker", "state": "OPEN", + "url": "https://github.com/owner/repo/issues/7", + } + pageInfo := map[string]any{"hasNextPage": false, "nextPage": float64(0)} + + assert.Error(t, resolved.Validate(map[string]any{"issues": []any{valid}}), + "pageInfo is required") + assert.Error(t, resolved.Validate(map[string]any{"pageInfo": pageInfo}), + "issues is required") + assert.Error(t, resolved.Validate(map[string]any{"issues": nil, "pageInfo": pageInfo}), + "issues must be an array, never null") + assert.Error(t, resolved.Validate(map[string]any{ + "issues": []any{map[string]any{"number": float64(7), "title": "Blocker", "state": "OPEN"}}, + "pageInfo": pageInfo, + }), "an issue ref missing url must be rejected") + assert.Error(t, resolved.Validate(map[string]any{ + "issues": []any{valid}, + "pageInfo": map[string]any{"hasNextPage": false}, + }), "pageInfo missing nextPage must be rejected") + }) +} + +// A 200 whose body is JSON `null` leaves go-github's slice nil. `issues` is +// required and typed as an array, so a nil slice reaching the marshaller +// unnormalised would emit `"issues":null` and break every client reading +// structuredContent. +func TestIssueDependencyReadNeverEmitsNullIssues(t *testing.T) { + t.Parallel() + + serverTool := IssueDependencyRead(translations.NullTranslationHelper) + resolved := resolveToolSchema(t, issueDependencyReadOutputSchema) + + nullBody := func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte(`null`)) + } + + for _, tc := range []struct { + method string + endpoint EndpointPattern + }{ + {"get_blocked_by", endpointBlockedBy}, + {"get_blocking", endpointBlocking}, + } { + t.Run(tc.method, func(t *testing.T) { + deps := BaseDeps{Client: mustNewGHClient(t, MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + string(tc.endpoint): nullBody, + }))} + + request := createMCPRequest(map[string]any{ + "method": tc.method, + "owner": "owner", + "repo": "repo", + "issue_number": float64(123), + }) + result, err := serverTool.Handler(deps)(ContextWithDeps(context.Background(), deps), &request) + require.NoError(t, err) + require.False(t, result.IsError) + + text := getTextResult(t, result) + assert.NotContains(t, text.Text, `"issues":null`, + "a nil slice must be normalised to [] before marshalling, or structuredContent violates the schema") + + var payload any + require.NoError(t, json.Unmarshal([]byte(text.Text), &payload)) + require.NoError(t, resolved.Validate(payload)) + }) + } +} diff --git a/pkg/github/output_conformance_issue_read_graphql_test.go b/pkg/github/output_conformance_issue_read_graphql_test.go new file mode 100644 index 0000000000..f7f3afed28 --- /dev/null +++ b/pkg/github/output_conformance_issue_read_graphql_test.go @@ -0,0 +1,277 @@ +package github + +import ( + "context" + "encoding/json" + "net/http" + "testing" + "time" + + "github.com/shurcooL/githubv4" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/github/github-mcp-server/internal/githubv4mock" + "github.com/github/github-mcp-server/pkg/translations" +) + +// The end-to-end guarantee for the two GraphQL-backed issue_read methods. +// TestIssueReadOutputValidatesAgainstDeclaredSchema covers the REST methods +// (get, get_comments, get_sub_issues); get_parent and get_labels never touch +// the REST client, so they need the githubv4mock harness instead and are +// covered here. +// +// Same contract as the REST test: run the REAL handler, take the single text +// block, and validate it against the advertised outputSchema. The +// structured-content mirror publishes those exact bytes as structuredContent, +// so validating the text block is equivalent to validating what a client sees. +func TestIssueReadGraphQLOutputValidatesAgainstDeclaredSchema(t *testing.T) { + t.Parallel() + + serverTool := IssueRead(translations.NullTranslationHelper) + resolved := resolveToolSchema(t, issueReadOutputSchema) + + // Negative controls. The union is wide — five branches, two of them bare + // arrays — so "the handler's output validated" only means something if + // near-miss payloads for these two branches are actually rejected. If any + // of these starts passing, every subtest below has gone vacuous. + for _, nearMiss := range []any{ + // labels typed as array: a nil slice marshalled to null must fail. + map[string]any{"labels": nil, "totalCount": 0}, + // a label missing id/color/description must fail its required set. + map[string]any{"labels": []any{map[string]any{"name": "bug"}}, "totalCount": 1}, + // totalCount is required alongside labels. + map[string]any{"labels": []any{}}, + // a parent ref missing title/state/url must fail issueRef. + map[string]any{"parent": map[string]any{"number": 42}}, + } { + require.Error(t, resolved.Validate(nearMiss), + "schema must reject %v, or the conformance subtests below prove nothing", nearMiss) + } + + // Query shapes must match the handler's anonymous structs exactly, or + // githubv4mock refuses to serve a response and the handler errors out. + parentQuery := struct { + Repository struct { + Issue struct { + Parent *struct { + Number githubv4.Int + Title githubv4.String + State githubv4.String + URL githubv4.String + Author struct { + Login githubv4.String + } + Repository struct { + NameWithOwner githubv4.String + } + } + } `graphql:"issue(number: $issueNumber)"` + } `graphql:"repository(owner: $owner, name: $repo)"` + }{} + + labelsQuery := struct { + Repository struct { + Issue struct { + Labels struct { + Nodes []struct { + ID githubv4.ID + Name githubv4.String + Color githubv4.String + Description githubv4.String + } + TotalCount githubv4.Int + } `graphql:"labels(first: 100)"` + } `graphql:"issue(number: $issueNumber)"` + } `graphql:"repository(owner: $owner, name: $repo)"` + }{} + + vars := map[string]any{ + "owner": githubv4.String("octocat"), + "repo": githubv4.String("repo"), + "issueNumber": githubv4.Int(1), + } + + tests := []struct { + name string + method string + gqlHTTP *http.Client + // wantContains anchors each subtest to the payload it meant to + // produce, so a handler that silently returned some other shape + // cannot coast through on a permissive union branch. + wantContains string + }{ + { + name: "get_parent", + method: "get_parent", + gqlHTTP: githubv4mock.NewMockedHTTPClient(githubv4mock.NewQueryMatcher(parentQuery, vars, + githubv4mock.DataResponse(map[string]any{ + "repository": map[string]any{ + "issue": map[string]any{ + "parent": map[string]any{ + "number": githubv4.Int(42), + "title": githubv4.String("Parent issue"), + "state": githubv4.String("OPEN"), + "url": githubv4.String("https://github.com/octocat/repo/issues/42"), + "author": map[string]any{ + "login": githubv4.String("octocat"), + }, + "repository": map[string]any{ + "nameWithOwner": githubv4.String("octocat/repo"), + }, + }, + }, + }, + }))), + wantContains: `"number":42`, + }, + { + // The wrapper's only key is null. `parent` is still required, so + // this exercises the branch's required set rather than its + // properties. + name: "get_parent with no parent", + method: "get_parent", + gqlHTTP: githubv4mock.NewMockedHTTPClient(githubv4mock.NewQueryMatcher(parentQuery, vars, + githubv4mock.DataResponse(map[string]any{ + "repository": map[string]any{ + "issue": map[string]any{ + "parent": nil, + }, + }, + }))), + wantContains: `"parent":null`, + }, + { + // A parent whose every optional GraphQL field came back empty. + // The handler still has to emit all five issueRef keys; if it + // ever started omitting empties, number/title/state/url would + // go missing and this subtest would fail. + name: "get_parent with all optionals absent", + method: "get_parent", + gqlHTTP: githubv4mock.NewMockedHTTPClient(githubv4mock.NewQueryMatcher(parentQuery, vars, + githubv4mock.DataResponse(map[string]any{ + "repository": map[string]any{ + "issue": map[string]any{ + "parent": map[string]any{}, + }, + }, + }))), + wantContains: `"repository":""`, + }, + { + name: "get_labels", + method: "get_labels", + gqlHTTP: githubv4mock.NewMockedHTTPClient(githubv4mock.NewQueryMatcher(labelsQuery, vars, + githubv4mock.DataResponse(map[string]any{ + "repository": map[string]any{ + "issue": map[string]any{ + "labels": map[string]any{ + "nodes": []any{ + map[string]any{ + "id": githubv4.ID("LA_label1"), + "name": githubv4.String("bug"), + "color": githubv4.String("d73a4a"), + "description": githubv4.String("Something isn't working"), + }, + }, + "totalCount": githubv4.Int(1), + }, + }, + }, + }))), + wantContains: `"name":"bug"`, + }, + { + // Empty collection: `labels` and `totalCount` are both required, + // so [] and 0 must still be emitted. + name: "get_labels with no labels", + method: "get_labels", + gqlHTTP: githubv4mock.NewMockedHTTPClient(githubv4mock.NewQueryMatcher(labelsQuery, vars, + githubv4mock.DataResponse(map[string]any{ + "repository": map[string]any{ + "issue": map[string]any{ + "labels": map[string]any{ + "nodes": []any{}, + "totalCount": githubv4.Int(0), + }, + }, + }, + }))), + wantContains: `"labels":[]`, + }, + { + // Same trap that get_sub_issues fell into: a null node list + // leaves the Go slice nil, which marshals to `null` unless the + // handler normalises it. The schema types `labels` as an array, + // so `null` would be a violation. + name: "get_labels with null node list", + method: "get_labels", + gqlHTTP: githubv4mock.NewMockedHTTPClient(githubv4mock.NewQueryMatcher(labelsQuery, vars, + githubv4mock.DataResponse(map[string]any{ + "repository": map[string]any{ + "issue": map[string]any{ + "labels": map[string]any{ + "nodes": nil, + "totalCount": githubv4.Int(0), + }, + }, + }, + }))), + wantContains: `"labels":[]`, + }, + { + // A label with every optional field absent. The handler builds + // the map by hand and always sets all four keys, which is what + // the branch's required list depends on. + name: "get_labels with all optionals absent", + method: "get_labels", + gqlHTTP: githubv4mock.NewMockedHTTPClient(githubv4mock.NewQueryMatcher(labelsQuery, vars, + githubv4mock.DataResponse(map[string]any{ + "repository": map[string]any{ + "issue": map[string]any{ + "labels": map[string]any{ + "nodes": []any{map[string]any{}}, + "totalCount": githubv4.Int(1), + }, + }, + }, + }))), + wantContains: `"description":""`, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + deps := BaseDeps{ + Client: mustNewGHClient(t, nil), + GQLClient: githubv4.NewClient(tt.gqlHTTP), + // get_parent consults the lockdown cache and flags before + // deciding whether to return the parent at all. + RepoAccessCache: stubRepoAccessCache(nil, 15*time.Minute), + Flags: stubFeatureFlags(map[string]bool{"lockdown-mode": false}), + } + handler := serverTool.Handler(deps) + + request := createMCPRequest(map[string]any{ + "method": tt.method, + "owner": "octocat", + "repo": "repo", + "issue_number": float64(1), + }) + result, err := handler(ContextWithDeps(context.Background(), deps), &request) + require.NoError(t, err) + require.False(t, result.IsError, "handler should succeed") + + text := getTextResult(t, result) + assert.Contains(t, text.Text, tt.wantContains, + "subtest must exercise the payload it describes") + + var payload any + require.NoError(t, json.Unmarshal([]byte(text.Text), &payload), + "handler output must be JSON for the mirror to publish it as structuredContent") + + require.NoError(t, resolved.Validate(payload), + "real handler output for method=%s must conform to the advertised outputSchema", tt.method) + }) + } +} diff --git a/pkg/github/output_conformance_pull_request_read_test.go b/pkg/github/output_conformance_pull_request_read_test.go new file mode 100644 index 0000000000..1382997cd6 --- /dev/null +++ b/pkg/github/output_conformance_pull_request_read_test.go @@ -0,0 +1,460 @@ +package github + +import ( + "context" + "encoding/json" + "net/http" + "testing" + "time" + + gogithub "github.com/google/go-github/v89/github" + "github.com/shurcooL/githubv4" + "github.com/stretchr/testify/require" + + "github.com/github/github-mcp-server/internal/githubv4mock" + "github.com/github/github-mcp-server/pkg/translations" +) + +// The end-to-end guarantee for pull_request_read, mirroring +// TestIssueReadOutputValidatesAgainstDeclaredSchema: what the real handler +// emits must validate against the schema the tool advertises. The +// structured-content mirror publishes the exact bytes of the single text block +// as structuredContent, so validating the text block here is equivalent to +// validating what a client receives. +// +// Every value of the tool's `method` enum is covered. The awkward cases are +// deliberate: list methods returning zero items (an empty array satisfies +// several array branches at once, which is why the union must be anyOf), and +// sparse objects with every optional field absent (which is what actually +// exercises each branch's required set rather than a convenient fixture). +// +// method=get_diff is the one exception, and it is asserted rather than skipped: +// it returns a raw unified diff, which is not JSON, so the mirror publishes no +// structuredContent and the schema declares no string branch. The subtest pins +// that invariant — if the diff ever became JSON, the tool would start emitting +// structuredContent that no branch describes. +func TestPullRequestReadOutputValidatesAgainstDeclaredSchema(t *testing.T) { + t.Parallel() + + serverTool := PullRequestRead(translations.NullTranslationHelper) + resolved := resolveToolSchema(t, pullRequestReadOutputSchema) + + // A populated PR. Head.SHA matters beyond method=get: get_status and + // get_check_runs resolve the head SHA from this response first. + mockPR := &gogithub.PullRequest{ + Number: gogithub.Ptr(42), + Title: gogithub.Ptr("Test PR"), + Body: gogithub.Ptr("This is a test PR"), + State: gogithub.Ptr("open"), + Draft: gogithub.Ptr(false), + Merged: gogithub.Ptr(false), + HTMLURL: gogithub.Ptr("https://github.com/owner/repo/pull/42"), + User: &gogithub.User{Login: gogithub.Ptr("octocat")}, + Head: &gogithub.PullRequestBranch{ + Ref: gogithub.Ptr("feature-branch"), + SHA: gogithub.Ptr("abcd1234"), + Repo: &gogithub.Repository{FullName: gogithub.Ptr("owner/repo")}, + }, + Base: &gogithub.PullRequestBranch{ + Ref: gogithub.Ptr("main"), + SHA: gogithub.Ptr("ef567890"), + }, + Labels: []*gogithub.Label{{Name: gogithub.Ptr("bug")}}, + Assignees: []*gogithub.User{{Login: gogithub.Ptr("octocat")}}, + RequestedReviewers: []*gogithub.User{{Login: gogithub.Ptr("reviewer")}}, + Additions: gogithub.Ptr(10), + Deletions: gogithub.Ptr(5), + ChangedFiles: gogithub.Ptr(2), + Comments: gogithub.Ptr(1), + Commits: gogithub.Ptr(3), + CreatedAt: &gogithub.Timestamp{Time: time.Date(2026, 4, 1, 12, 0, 0, 0, time.UTC)}, + UpdatedAt: &gogithub.Timestamp{Time: time.Date(2026, 4, 2, 12, 0, 0, 0, time.UTC)}, + Milestone: &gogithub.Milestone{Title: gogithub.Ptr("v1")}, + } + + // The minimum a PR fetch can carry for the methods that need a head SHA. + mockPRHeadOnly := &gogithub.PullRequest{ + Number: gogithub.Ptr(42), + Head: &gogithub.PullRequestBranch{SHA: gogithub.Ptr("abcd1234")}, + } + + stubbedDiff := `diff --git a/README.md b/README.md +index 5d6e7b2..8a4f5c3 100644 +--- a/README.md ++++ b/README.md +@@ -1,4 +1,6 @@ + # Hello-World + ++## New Section` + + reviewThreadsMatcher := func(nodes []map[string]any, pageInfo map[string]any, totalCount int) *http.Client { + return githubv4mock.NewMockedHTTPClient( + githubv4mock.NewQueryMatcher( + reviewThreadsQuery{}, + map[string]any{ + "owner": githubv4.String("owner"), + "repo": githubv4.String("repo"), + "prNum": githubv4.Int(42), + "first": githubv4.Int(30), + "commentsPerThread": githubv4.Int(100), + "after": (*githubv4.String)(nil), + }, + githubv4mock.DataResponse(map[string]any{ + "repository": map[string]any{ + "pullRequest": map[string]any{ + "reviewThreads": map[string]any{ + "nodes": nodes, + "pageInfo": pageInfo, + "totalCount": totalCount, + }, + }, + }, + }), + ), + ) + } + + tests := []struct { + name string + method string + handlers map[string]http.HandlerFunc + gqlHTTPClient *http.Client + // nonJSONText marks the one method whose result is deliberately not + // JSON, so nothing is mirrored into structuredContent and the schema + // declares no branch for it. + nonJSONText bool + }{ + { + name: "get", + method: "get", + handlers: map[string]http.HandlerFunc{ + GetReposPullsByOwnerByRepoByPullNumber: mockResponse(t, http.StatusOK, mockPR), + }, + }, + { + // A fully sparse pull request: every optional field absent. Exercises + // the branch's required set rather than a convenient fixture. + name: "get with all optionals absent", + method: "get", + handlers: map[string]http.HandlerFunc{ + GetReposPullsByOwnerByRepoByPullNumber: mockResponse(t, http.StatusOK, &gogithub.PullRequest{}), + }, + }, + { + // Raw unified diff: not JSON, so no structuredContent is published + // and the schema deliberately carries no string branch. + name: "get_diff", + method: "get_diff", + handlers: map[string]http.HandlerFunc{ + GetReposPullsByOwnerByRepoByPullNumber: mockResponse(t, http.StatusOK, stubbedDiff), + }, + nonJSONText: true, + }, + { + name: "get_status", + method: "get_status", + handlers: map[string]http.HandlerFunc{ + GetReposPullsByOwnerByRepoByPullNumber: mockResponse(t, http.StatusOK, mockPR), + GetReposCommitsStatusByOwnerByRepoByRef: mockResponse(t, http.StatusOK, &gogithub.CombinedStatus{ + State: gogithub.Ptr("success"), + SHA: gogithub.Ptr("abcd1234"), + TotalCount: gogithub.Ptr(1), + Statuses: []*gogithub.RepoStatus{{ + State: gogithub.Ptr("success"), + Context: gogithub.Ptr("ci/build"), + Description: gogithub.Ptr("Build succeeded"), + TargetURL: gogithub.Ptr("https://ci.example.com/1"), + }}, + }), + }, + }, + { + // *github.CombinedStatus is all pointers+omitempty, so an all-nil + // value really marshals to {}. The union has to keep that valid. + name: "get_status with an all-nil combined status", + method: "get_status", + handlers: map[string]http.HandlerFunc{ + GetReposPullsByOwnerByRepoByPullNumber: mockResponse(t, http.StatusOK, mockPRHeadOnly), + GetReposCommitsStatusByOwnerByRepoByRef: mockResponse(t, http.StatusOK, &gogithub.CombinedStatus{}), + }, + }, + { + // Same story one level down: an all-nil *github.RepoStatus element + // marshals to {} inside the statuses array. + name: "get_status with an all-nil status element", + method: "get_status", + handlers: map[string]http.HandlerFunc{ + GetReposPullsByOwnerByRepoByPullNumber: mockResponse(t, http.StatusOK, mockPRHeadOnly), + GetReposCommitsStatusByOwnerByRepoByRef: mockResponse(t, http.StatusOK, &gogithub.CombinedStatus{ + Statuses: []*gogithub.RepoStatus{{}}, + }), + }, + }, + { + name: "get_files", + method: "get_files", + handlers: map[string]http.HandlerFunc{ + GetReposPullsFilesByOwnerByRepoByPullNumber: mockResponse(t, http.StatusOK, []*gogithub.CommitFile{{ + Filename: gogithub.Ptr("file1.go"), + Status: gogithub.Ptr("modified"), + Additions: gogithub.Ptr(10), + Deletions: gogithub.Ptr(5), + Changes: gogithub.Ptr(15), + Patch: gogithub.Ptr("@@ -1,5 +1,10 @@"), + }}), + }, + }, + { + // The ambiguous case that makes oneOf unusable: [] satisfies every + // array branch at once. + name: "get_files with no files", + method: "get_files", + handlers: map[string]http.HandlerFunc{ + GetReposPullsFilesByOwnerByRepoByPullNumber: mockResponse(t, http.StatusOK, []*gogithub.CommitFile{}), + }, + }, + { + name: "get_files with all optionals absent", + method: "get_files", + handlers: map[string]http.HandlerFunc{ + GetReposPullsFilesByOwnerByRepoByPullNumber: mockResponse(t, http.StatusOK, []*gogithub.CommitFile{{}}), + }, + }, + { + name: "get_commits", + method: "get_commits", + handlers: map[string]http.HandlerFunc{ + GetReposPullsCommitsByOwnerByRepoByPullNumber: mockResponse(t, http.StatusOK, []*gogithub.RepositoryCommit{{ + SHA: gogithub.Ptr("abc123"), + HTMLURL: gogithub.Ptr("https://github.com/owner/repo/commit/abc123"), + Commit: &gogithub.Commit{ + Message: gogithub.Ptr("feat: add a thing"), + Author: &gogithub.CommitAuthor{ + Name: gogithub.Ptr("Test User"), + Email: gogithub.Ptr("test@example.com"), + Date: &gogithub.Timestamp{Time: time.Date(2026, 4, 1, 12, 0, 0, 0, time.UTC)}, + }, + }, + }}), + }, + }, + { + name: "get_commits with no commits", + method: "get_commits", + handlers: map[string]http.HandlerFunc{ + GetReposPullsCommitsByOwnerByRepoByPullNumber: mockResponse(t, http.StatusOK, []*gogithub.RepositoryCommit{}), + }, + }, + { + name: "get_commits with all optionals absent", + method: "get_commits", + handlers: map[string]http.HandlerFunc{ + GetReposPullsCommitsByOwnerByRepoByPullNumber: mockResponse(t, http.StatusOK, []*gogithub.RepositoryCommit{{}}), + }, + }, + { + name: "get_review_comments", + method: "get_review_comments", + gqlHTTPClient: reviewThreadsMatcher( + []map[string]any{{ + "id": "RT_kwDOA0xdyM4AX1Yz", + "isResolved": false, + "isOutdated": false, + "isCollapsed": false, + "comments": map[string]any{ + "totalCount": 1, + "nodes": []map[string]any{{ + "id": "PRRC_kwDOA0xdyM4AX1Y0", + "body": "This looks good", + "path": "file1.go", + "line": 5, + "author": map[string]any{"login": "reviewer1"}, + "createdAt": "2024-01-01T12:00:00Z", + "updatedAt": "2024-01-01T12:00:00Z", + "url": "https://github.com/owner/repo/pull/42#discussion_r101", + }}, + }, + }}, + map[string]any{ + "hasNextPage": false, + "hasPreviousPage": false, + "startCursor": "cursor1", + "endCursor": "cursor2", + }, + 1, + ), + }, + { + name: "get_review_comments with no threads", + method: "get_review_comments", + gqlHTTPClient: reviewThreadsMatcher( + []map[string]any{}, + map[string]any{"hasNextPage": false, "hasPreviousPage": false}, + 0, + ), + }, + { + // Sparse throughout: a thread with no comments, a comment with no + // line/body/author, and page info with both cursors absent + // (startCursor/endCursor are omitempty on MinimalPageInfo). + name: "get_review_comments with all optionals absent", + method: "get_review_comments", + gqlHTTPClient: reviewThreadsMatcher( + []map[string]any{ + { + "id": "RT_empty", + "comments": map[string]any{"totalCount": 0, "nodes": []map[string]any{}}, + }, + { + "id": "RT_sparse_comment", + "comments": map[string]any{ + "totalCount": 1, + "nodes": []map[string]any{{ + "path": "file1.go", + "url": "https://github.com/owner/repo/pull/42#discussion_r102", + }}, + }, + }, + }, + map[string]any{"hasNextPage": false, "hasPreviousPage": false}, + 2, + ), + }, + { + name: "get_reviews", + method: "get_reviews", + handlers: map[string]http.HandlerFunc{ + GetReposPullsReviewsByOwnerByRepoByPullNumber: mockResponse(t, http.StatusOK, []*gogithub.PullRequestReview{{ + ID: gogithub.Ptr(int64(1)), + State: gogithub.Ptr("APPROVED"), + Body: gogithub.Ptr("LGTM"), + HTMLURL: gogithub.Ptr("https://github.com/owner/repo/pull/42#pullrequestreview-1"), + User: &gogithub.User{Login: gogithub.Ptr("reviewer")}, + CommitID: gogithub.Ptr("abcd1234"), + SubmittedAt: &gogithub.Timestamp{Time: time.Date(2026, 4, 1, 12, 0, 0, 0, time.UTC)}, + AuthorAssociation: gogithub.Ptr("COLLABORATOR"), + }}), + }, + }, + { + name: "get_reviews with no reviews", + method: "get_reviews", + handlers: map[string]http.HandlerFunc{ + GetReposPullsReviewsByOwnerByRepoByPullNumber: mockResponse(t, http.StatusOK, []*gogithub.PullRequestReview{}), + }, + }, + { + name: "get_reviews with all optionals absent", + method: "get_reviews", + handlers: map[string]http.HandlerFunc{ + GetReposPullsReviewsByOwnerByRepoByPullNumber: mockResponse(t, http.StatusOK, []*gogithub.PullRequestReview{{}}), + }, + }, + { + name: "get_comments", + method: "get_comments", + handlers: map[string]http.HandlerFunc{ + GetReposIssuesCommentsByOwnerByRepoByIssueNumber: mockResponse(t, http.StatusOK, []*gogithub.IssueComment{{ + ID: gogithub.Ptr(int64(1)), + Body: gogithub.Ptr("hello"), + HTMLURL: gogithub.Ptr("https://github.com/owner/repo/pull/42#issuecomment-1"), + User: &gogithub.User{Login: gogithub.Ptr("octocat")}, + AuthorAssociation: gogithub.Ptr("MEMBER"), + Reactions: &gogithub.Reactions{TotalCount: gogithub.Ptr(1), PlusOne: gogithub.Ptr(1)}, + CreatedAt: &gogithub.Timestamp{Time: time.Date(2026, 4, 1, 12, 0, 0, 0, time.UTC)}, + }}), + }, + }, + { + name: "get_comments with no comments", + method: "get_comments", + handlers: map[string]http.HandlerFunc{ + GetReposIssuesCommentsByOwnerByRepoByIssueNumber: mockResponse(t, http.StatusOK, []*gogithub.IssueComment{}), + }, + }, + { + name: "get_comments with all optionals absent", + method: "get_comments", + handlers: map[string]http.HandlerFunc{ + GetReposIssuesCommentsByOwnerByRepoByIssueNumber: mockResponse(t, http.StatusOK, []*gogithub.IssueComment{{}}), + }, + }, + { + name: "get_check_runs", + method: "get_check_runs", + handlers: map[string]http.HandlerFunc{ + GetReposPullsByOwnerByRepoByPullNumber: mockResponse(t, http.StatusOK, mockPR), + GetReposCommitsCheckRunsByOwnerByRepoByRef: mockResponse(t, http.StatusOK, &gogithub.ListCheckRunsResults{ + Total: gogithub.Ptr(1), + CheckRuns: []*gogithub.CheckRun{{ + ID: gogithub.Ptr(int64(1)), + Name: gogithub.Ptr("build"), + Status: gogithub.Ptr("completed"), + Conclusion: gogithub.Ptr("success"), + HTMLURL: gogithub.Ptr("https://github.com/owner/repo/runs/1"), + DetailsURL: gogithub.Ptr("https://ci.example.com/1"), + StartedAt: &gogithub.Timestamp{Time: time.Date(2026, 4, 1, 12, 0, 0, 0, time.UTC)}, + CompletedAt: &gogithub.Timestamp{Time: time.Date(2026, 4, 1, 12, 5, 0, 0, time.UTC)}, + }}, + }), + }, + }, + { + name: "get_check_runs with no check runs", + method: "get_check_runs", + handlers: map[string]http.HandlerFunc{ + GetReposPullsByOwnerByRepoByPullNumber: mockResponse(t, http.StatusOK, mockPRHeadOnly), + GetReposCommitsCheckRunsByOwnerByRepoByRef: mockResponse(t, http.StatusOK, &gogithub.ListCheckRunsResults{ + Total: gogithub.Ptr(0), + CheckRuns: []*gogithub.CheckRun{}, + }), + }, + }, + { + name: "get_check_runs with all optionals absent", + method: "get_check_runs", + handlers: map[string]http.HandlerFunc{ + GetReposPullsByOwnerByRepoByPullNumber: mockResponse(t, http.StatusOK, mockPRHeadOnly), + GetReposCommitsCheckRunsByOwnerByRepoByRef: mockResponse(t, http.StatusOK, &gogithub.ListCheckRunsResults{ + CheckRuns: []*gogithub.CheckRun{{}}, + }), + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + deps := BaseDeps{Client: mustNewGHClient(t, MockHTTPClientWithHandlers(tt.handlers))} + if tt.gqlHTTPClient != nil { + deps.GQLClient = githubv4.NewClient(tt.gqlHTTPClient) + } + handler := serverTool.Handler(deps) + + request := createMCPRequest(map[string]any{ + "method": tt.method, + "owner": "owner", + "repo": "repo", + "pullNumber": float64(42), + }) + result, err := handler(ContextWithDeps(context.Background(), deps), &request) + require.NoError(t, err) + require.False(t, result.IsError, "handler should succeed") + + text := getTextResult(t, result) + + if tt.nonJSONText { + require.False(t, json.Valid([]byte(text.Text)), + "method=%s emits a raw body the mirror must not publish as structuredContent; "+ + "if it is now JSON, the schema needs a branch describing it", tt.method) + return + } + + var payload any + require.NoError(t, json.Unmarshal([]byte(text.Text), &payload), + "handler output must be JSON for the mirror to publish it as structuredContent") + + require.NoError(t, resolved.Validate(payload), + "real handler output for method=%s must conform to the advertised outputSchema", tt.method) + }) + } +} From 736cbf57e71ff96c897e79fbd0ca855e8ed134e5 Mon Sep 17 00:00:00 2001 From: olaservo Date: Sat, 25 Jul 2026 18:33:00 -0700 Subject: [PATCH 6/6] docs: add live-API size measurements for structured_content_only The synthetic 272-byte fixture undersells the effect, because the saving comes from JSON-string escaping overhead, which scales with payload size. Measured against the live GitHub API on github/github-mcp-server, comparing the full serialized tools/call result: pull_request_read get_files 99,990 -> 48,999 bytes (-51%) actions_list list_workflows 30,407 -> 15,839 bytes (-48%) actions_get get_workflow_run 30,229 -> 15,801 bytes (-48%) pull_request_read get_check_runs 10,551 -> 6,240 bytes (-41%) Co-Authored-By: Claude Opus 5 --- docs/feature-flags.md | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/docs/feature-flags.md b/docs/feature-flags.md index 052425737e..b0d49d0da4 100644 --- a/docs/feature-flags.md +++ b/docs/feature-flags.md @@ -63,7 +63,16 @@ A client that negotiated 2026-07-28 is not such a client. This flag drops the re | `output_schemas` | 405 bytes (+49%) | | `output_schemas,structured_content_only` | 254 bytes (−7%) | -(Measured by `TestStructuredContentOnlyRoughlyHalvesTheResult` on a small payload.) The result is *smaller* than having no output schema at all, because a text block embeds JSON as an escaped string — every `"` becomes `\"` — while `structuredContent` carries it raw. The saving grows with payload size. +(Measured by `TestStructuredContentOnlyRoughlyHalvesTheResult` on a small payload.) The result is *smaller* than having no output schema at all, because a text block embeds JSON as an escaped string — every `"` becomes `\"` — while `structuredContent` carries it raw. + +The saving grows with payload size. Measured against the live GitHub API on `github/github-mcp-server`, comparing the full serialized `tools/call` result: + +| Call | `output_schemas` | `+ structured_content_only` | +|------|------------------|-----------------------------| +| `pull_request_read` `get_files` | 99,990 B | 48,999 B (−51%) | +| `actions_list` `list_workflows` | 30,407 B | 15,839 B (−48%) | +| `actions_get` `get_workflow_run` | 30,229 B | 15,801 B (−48%) | +| `pull_request_read` `get_check_runs` | 10,551 B | 6,240 B (−41%) | `content` remains present as an empty array, since the schema still requires the field.