Fix add_issue_comment schema compatibility regression

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.",

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

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{

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