From 01eaa0f64b2baab83aa698f9a8730bb766a2da87 Mon Sep 17 00:00:00 2001 From: Sam Morrow Date: Thu, 20 Aug 2026 10:38:18 +0200 Subject: [PATCH] fix(issues): flatten issue comment input schema Keep cross-field validation in the handler so the canonical tool schema remains compatible with provider JSON Schema subsets. Add an inventory-wide regression guard against top-level schema combinators. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../__toolsnaps__/add_issue_comment.snap | 24 ---- pkg/github/issues.go | 10 -- pkg/github/issues_test.go | 105 +++++++++++++----- pkg/github/tools_validation_test.go | 24 ++++ 4 files changed, 100 insertions(+), 63 deletions(-) diff --git a/pkg/github/__toolsnaps__/add_issue_comment.snap b/pkg/github/__toolsnaps__/add_issue_comment.snap index 708535ff58..a68494d448 100644 --- a/pkg/github/__toolsnaps__/add_issue_comment.snap +++ b/pkg/github/__toolsnaps__/add_issue_comment.snap @@ -6,30 +6,6 @@ }, "description": "Add a comment and/or reaction to a specific issue or issue comment in a GitHub repository. Use this tool with pull requests as well (in this case pass pull request number as issue_number), but only if user is not asking specifically to add or react to review comments. At least one of body or reaction is required.", "inputSchema": { - "anyOf": [ - { - "required": [ - "body" - ] - }, - { - "required": [ - "reaction" - ] - } - ], - "dependentSchemas": { - "comment_id": { - "not": { - "required": [ - "body" - ] - }, - "required": [ - "reaction" - ] - } - }, "properties": { "body": { "description": "Comment content. Required unless reaction is provided.", diff --git a/pkg/github/issues.go b/pkg/github/issues.go index 5037c199a3..d6cc55e1ed 100644 --- a/pkg/github/issues.go +++ b/pkg/github/issues.go @@ -1399,16 +1399,6 @@ func AddIssueComment(t translations.TranslationHelperFunc) inventory.ServerTool }, }, Required: []string{"owner", "repo", "issue_number"}, - AnyOf: []*jsonschema.Schema{ - {Required: []string{"body"}}, - {Required: []string{"reaction"}}, - }, - DependentSchemas: map[string]*jsonschema.Schema{ - "comment_id": { - Required: []string{"reaction"}, - Not: &jsonschema.Schema{Required: []string{"body"}}, - }, - }, }, }, []scopes.Scope{scopes.Repo}, diff --git a/pkg/github/issues_test.go b/pkg/github/issues_test.go index 4ac8bc42d4..7e9fb2ee8e 100644 --- a/pkg/github/issues_test.go +++ b/pkg/github/issues_test.go @@ -5410,6 +5410,10 @@ func TestAddIssueCommentSchema(t *testing.T) { assert.Contains(t, schema.Properties, "body") assert.Contains(t, schema.Properties, "reaction") assert.ElementsMatch(t, schema.Required, []string{"owner", "repo", "issue_number"}) + assert.Empty(t, schema.AnyOf) + assert.Empty(t, schema.OneOf) + assert.Empty(t, schema.AllOf) + assert.Empty(t, schema.DependentSchemas) resolved, err := schema.Resolve(nil) require.NoError(t, err) @@ -5425,62 +5429,47 @@ func TestAddIssueCommentSchema(t *testing.T) { isValid bool }{ { - name: "body-only comment", - args: map[string]any{"body": "This is a comment"}, + name: "cross-field requirements are handler validated", + args: map[string]any{}, isValid: true, }, { - name: "issue or pull request reaction", - args: map[string]any{"reaction": "heart"}, + name: "comment_id relationships are handler validated", + args: map[string]any{"comment_id": 999, "body": "This is a comment"}, isValid: true, }, { - name: "comment and issue or pull request reaction", - args: map[string]any{"body": "This is a comment", "reaction": "heart"}, + name: "body minLength accepts non-empty body", + args: map[string]any{"body": "This is a comment"}, isValid: true, }, { - name: "existing comment reaction", - args: map[string]any{"comment_id": 999, "reaction": "heart"}, + name: "reaction enum accepts supported reaction", + args: map[string]any{"reaction": "heart"}, isValid: true, }, { - name: "missing body and reaction", - args: map[string]any{}, + name: "missing required owner", + args: map[string]any{"owner": nil}, isValid: false, }, { - name: "empty body", + name: "body minLength rejects empty body", args: map[string]any{"body": ""}, isValid: false, }, { - name: "comment_id without reaction", - args: map[string]any{"comment_id": 999}, - isValid: false, - }, - { - name: "comment_id with body", - args: map[string]any{"comment_id": 999, "body": "This is a comment"}, - isValid: false, - }, - { - name: "comment_id with body and reaction", - args: map[string]any{"comment_id": 999, "body": "This is a comment", "reaction": "heart"}, - isValid: false, - }, - { - name: "zero comment_id", + name: "comment_id minimum rejects zero", args: map[string]any{"comment_id": 0, "reaction": "heart"}, isValid: false, }, { - name: "fractional comment_id", + name: "comment_id integer rejects fraction", args: map[string]any{"comment_id": 1.5, "reaction": "heart"}, isValid: false, }, { - name: "invalid reaction", + name: "reaction enum rejects unsupported reaction", args: map[string]any{"reaction": "party"}, isValid: false, }, @@ -5627,6 +5616,28 @@ func TestAddIssueCommentHandler(t *testing.T) { expectToolError: true, expectedToolErrMsg: "at least one of body or reaction is required", }, + { + name: "empty body", + requestArgs: map[string]any{ + "owner": "owner", + "repo": "repo", + "issue_number": float64(42), + "body": "", + }, + expectToolError: true, + expectedToolErrMsg: "body cannot be empty when provided", + }, + { + name: "empty reaction", + requestArgs: map[string]any{ + "owner": "owner", + "repo": "repo", + "issue_number": float64(42), + "reaction": "", + }, + expectToolError: true, + expectedToolErrMsg: "reaction cannot be empty when provided", + }, { name: "missing issue_number for reaction", requestArgs: map[string]any{ @@ -5658,6 +5669,18 @@ func TestAddIssueCommentHandler(t *testing.T) { expectToolError: true, expectedToolErrMsg: "comment_id can only be provided when reaction is provided", }, + { + name: "comment_id with body but without reaction", + requestArgs: map[string]any{ + "owner": "owner", + "repo": "repo", + "issue_number": float64(42), + "comment_id": float64(999), + "body": "This is a comment", + }, + expectToolError: true, + expectedToolErrMsg: "comment_id cannot be combined with body", + }, { name: "zero comment_id", requestArgs: map[string]any{ @@ -5682,6 +5705,30 @@ func TestAddIssueCommentHandler(t *testing.T) { expectToolError: true, expectedToolErrMsg: "comment_id must be greater than 0", }, + { + name: "fractional comment_id", + requestArgs: map[string]any{ + "owner": "owner", + "repo": "repo", + "issue_number": float64(42), + "comment_id": float64(1.5), + "reaction": "heart", + }, + expectToolError: true, + expectedToolErrMsg: "parameter comment_id is not a valid number", + }, + { + name: "non-numeric comment_id", + requestArgs: map[string]any{ + "owner": "owner", + "repo": "repo", + "issue_number": float64(42), + "comment_id": "not-a-number", + "reaction": "heart", + }, + expectToolError: true, + expectedToolErrMsg: "parameter comment_id is not a valid number", + }, { name: "comment_id with body", requestArgs: map[string]any{ diff --git a/pkg/github/tools_validation_test.go b/pkg/github/tools_validation_test.go index 1db85b2fc1..dac04b5d54 100644 --- a/pkg/github/tools_validation_test.go +++ b/pkg/github/tools_validation_test.go @@ -1,6 +1,7 @@ package github import ( + "encoding/json" "go/ast" "go/parser" "go/token" @@ -47,6 +48,29 @@ func TestAllToolsHaveRequiredMetadata(t *testing.T) { } } +// TestAllToolInputSchemasAvoidTopLevelCombinators keeps the complete OSS tool +// inventory portable across provider JSON Schema subsets. Some providers reject +// an entire tools/list payload when any input schema has a top-level combinator, +// so cross-field constraints belong in handlers or below ordinary properties. +func TestAllToolInputSchemasAvoidTopLevelCombinators(t *testing.T) { + tools := AllTools(stubTranslation) + require.NotEmpty(t, tools, "AllTools should return at least one tool") + + for _, serverTool := range tools { + tool := serverTool.Tool + t.Run(tool.Name, func(t *testing.T) { + data, err := json.Marshal(tool.InputSchema) + require.NoError(t, err, "Tool %q InputSchema must marshal", tool.Name) + + var schema map[string]json.RawMessage + require.NoError(t, json.Unmarshal(data, &schema), "Tool %q InputSchema must be a JSON object", tool.Name) + assert.NotContains(t, schema, "anyOf", "Tool %q InputSchema must not use top-level anyOf", tool.Name) + assert.NotContains(t, schema, "oneOf", "Tool %q InputSchema must not use top-level oneOf", tool.Name) + assert.NotContains(t, schema, "allOf", "Tool %q InputSchema must not use top-level allOf", tool.Name) + }) + } +} + // TestAllResourcesHaveRequiredMetadata validates that all resources have mandatory metadata func TestAllResourcesHaveRequiredMetadata(t *testing.T) { // Resources are now stateless - no client functions needed