Tighten issue type removal changes

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot-Session: ea8faa5c-7f26-4e2d-bf9c-6f0b5f173e8c

Author: zwick

cmd/github-mcp-server/generate_docs.go

@@ -301,23 +301,16 @@ func schemaTypeString(schema *jsonschema.Schema) (string, error) {
 	if schema == nil {
 		return "", fmt.Errorf("schema is nil")
 	}
-	if schema.Type == "array" {
+
+	switch {
+	case schema.Type == "array":
 		if schema.Items != nil {
-			itemType, err := schemaTypeString(schema.Items)
-			if err != nil {
-				return "", fmt.Errorf("array items: %w", err)
-			}
-			if strings.Contains(itemType, " | ") {
-				itemType = "(" + itemType + ")"
-			}
-			return itemType + "[]", nil
+			return schema.Items.Type + "[]", nil
 		}
 		return "array", nil
-	}
-	if schema.Type != "" {
+	case schema.Type != "":
 		return schema.Type, nil
-	}
-	if len(schema.Types) > 0 {
+	case len(schema.Types) > 0:
 		return strings.Join(schema.Types, " | "), nil
 	}
 
@@ -328,11 +321,12 @@ func schemaTypeString(schema *jsonschema.Schema) (string, error) {
 	if len(union) == 0 {
 		return "any", nil
 	}
+
 	types := make([]string, 0, len(union))
-	for i, member := range union {
+	for _, member := range union {
 		memberType, err := schemaTypeString(member)
 		if err != nil {
-			return "", fmt.Errorf("union member %d: %w", i, err)
+			return "", err
 		}
 		if !slices.Contains(types, memberType) {
 			types = append(types, memberType)

cmd/github-mcp-server/main_test.go

@@ -47,40 +47,15 @@ func TestSchemaTypeString(t *testing.T) {
 	}{
 		{name: "type", schema: &jsonschema.Schema{Type: "string"}, want: "string"},
 		{name: "types", schema: &jsonschema.Schema{Types: []string{"string", "number"}}, want: "string | number"},
-		{name: "nil", schema: nil, wantError: true},
+		{name: "nil", wantError: true},
 		{name: "unconstrained", schema: &jsonschema.Schema{}, want: "any"},
-		{
-			name: "anyOf",
-			schema: &jsonschema.Schema{AnyOf: []*jsonschema.Schema{
-				{Type: "string"},
-				{Type: "null"},
-			}},
-			want: "string | null",
-		},
-		{
-			name: "oneOf",
-			schema: &jsonschema.Schema{OneOf: []*jsonschema.Schema{
-				{Type: "number"},
-				{Type: "string"},
-			}},
-			want: "number | string",
-		},
+		{name: "anyOf", schema: &jsonschema.Schema{AnyOf: []*jsonschema.Schema{{Type: "string"}, {Type: "null"}}}, want: "string | null"},
+		{name: "oneOf", schema: &jsonschema.Schema{OneOf: []*jsonschema.Schema{{Type: "number"}, {Type: "string"}}}, want: "number | string"},
 		{
 			name:   "array",
 			schema: &jsonschema.Schema{Type: "array", Items: &jsonschema.Schema{Type: "string"}},
 			want:   "string[]",
 		},
-		{
-			name: "array of union",
-			schema: &jsonschema.Schema{
-				Type: "array",
-				Items: &jsonschema.Schema{AnyOf: []*jsonschema.Schema{
-					{Type: "string"},
-					{Type: "object"},
-				}},
-			},
-			want: "(string | object)[]",
-		},
 		{name: "untyped array", schema: &jsonschema.Schema{Type: "array"}, want: "array"},
 	}
 

docs/feature-flags.md

@@ -150,7 +150,7 @@ runtime behavior (such as output formatting) won't appear here.
 
 - **update_issue_assignees** - Update Issue Assignees
   - **Required OAuth Scopes**: `repo`
-  - `assignees`: GitHub usernames to assign to this issue. ((string | object)[], required)
+  - `assignees`: GitHub usernames to assign to this issue. ([], required)
   - `issue_number`: The issue number to update (number, required)
   - `owner`: Repository owner (username or organization) (string, required)
   - `repo`: Repository name (string, required)
@@ -165,7 +165,7 @@ runtime behavior (such as output formatting) won't appear here.
 - **update_issue_labels** - Update Issue Labels
   - **Required OAuth Scopes**: `repo`
   - `issue_number`: The issue number to update (number, required)
-  - `labels`: Labels to apply to this issue. ((string | object)[], required)
+  - `labels`: Labels to apply to this issue. ([], required)
   - `owner`: Repository owner (username or organization) (string, required)
   - `repo`: Repository name (string, required)
 

pkg/github/granular_tools_test.go

@@ -15,7 +15,6 @@ import (
 	"github.com/github/github-mcp-server/pkg/inventory"
 	"github.com/github/github-mcp-server/pkg/translations"
 	gogithub "github.com/google/go-github/v89/github"
-	"github.com/google/jsonschema-go/jsonschema"
 	"github.com/shurcooL/githubv4"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
@@ -756,13 +755,6 @@ func TestGranularUpdateIssueMilestone(t *testing.T) {
 }
 
 func TestGranularUpdateIssueType(t *testing.T) {
-	toolSchema := GranularUpdateIssueType(translations.NullTranslationHelper).Tool.InputSchema.(*jsonschema.Schema)
-	issueTypeSchema := toolSchema.Properties["issue_type"]
-	require.Len(t, issueTypeSchema.AnyOf, 2)
-	assert.Equal(t, "string", issueTypeSchema.AnyOf[0].Type)
-	assert.Equal(t, 1, *issueTypeSchema.AnyOf[0].MinLength)
-	assert.Equal(t, "null", issueTypeSchema.AnyOf[1].Type)
-
 	tests := []struct {
 		name        string
 		requestArgs map[string]any
@@ -828,48 +820,18 @@ func TestGranularUpdateIssueType(t *testing.T) {
 	}
 }
 
-func TestGranularUpdateIssueTypeRejectsEmptyIssueType(t *testing.T) {
-	deps := BaseDeps{Client: mustNewGHClient(t, MockHTTPClientWithHandlers(nil))}
-	serverTool := GranularUpdateIssueType(translations.NullTranslationHelper)
-	handler := serverTool.Handler(deps)
-	request := createMCPRequest(map[string]any{
-		"owner":        "owner",
-		"repo":         "repo",
-		"issue_number": float64(1),
-		"issue_type":   "",
-	})
-
-	result, err := handler(ContextWithDeps(context.Background(), deps), &request)
-	require.NoError(t, err)
-	errorContent := getErrorResult(t, result)
-	assert.Contains(t, errorContent.Text, "parameter issue_type must not be empty")
-}
-
-func TestGranularUpdateIssueTypeRejectsMissingIssueType(t *testing.T) {
-	deps := BaseDeps{Client: mustNewGHClient(t, MockHTTPClientWithHandlers(nil))}
-	serverTool := GranularUpdateIssueType(translations.NullTranslationHelper)
-	handler := serverTool.Handler(deps)
-
-	request := createMCPRequest(map[string]any{
-		"owner":        "owner",
-		"repo":         "repo",
-		"issue_number": float64(1),
-	})
-	result, err := handler(ContextWithDeps(context.Background(), deps), &request)
-	require.NoError(t, err)
-
-	errorContent := getErrorResult(t, result)
-	assert.Contains(t, errorContent.Text, "missing required parameter: issue_type")
-}
-
-func TestGranularUpdateIssueTypeRejectsMetadataWhenRemovingType(t *testing.T) {
+func TestGranularUpdateIssueTypeRejectsInvalidInput(t *testing.T) {
 	tests := []struct {
-		name string
-		args map[string]any
+		name      string
+		args      map[string]any
+		omitType  bool
+		wantError string
 	}{
-		{name: "rationale", args: map[string]any{"rationale": "live validation"}},
-		{name: "confidence", args: map[string]any{"confidence": "HIGH"}},
-		{name: "suggestion", args: map[string]any{"is_suggestion": true}},
+		{name: "missing type", omitType: true, wantError: "missing required parameter: issue_type"},
+		{name: "empty type", args: map[string]any{"issue_type": ""}, wantError: "parameter issue_type must not be empty"},
+		{name: "null with rationale", args: map[string]any{"rationale": "live validation"}, wantError: "suggestion metadata is not supported"},
+		{name: "null with confidence", args: map[string]any{"confidence": "HIGH"}, wantError: "suggestion metadata is not supported"},
+		{name: "null suggestion", args: map[string]any{"is_suggestion": true}, wantError: "suggestion metadata is not supported"},
 	}
 
 	for _, tc := range tests {
@@ -883,13 +845,16 @@ func TestGranularUpdateIssueTypeRejectsMetadataWhenRemovingType(t *testing.T) {
 				"issue_number": float64(1),
 				"issue_type":   nil,
 			}
+			if tc.omitType {
+				delete(args, "issue_type")
+			}
 			maps.Copy(args, tc.args)
 			request := createMCPRequest(args)
 
 			result, err := handler(ContextWithDeps(context.Background(), deps), &request)
 			require.NoError(t, err)
 			errorContent := getErrorResult(t, result)
-			assert.Contains(t, errorContent.Text, "suggestion metadata is not supported when removing an issue type; omit rationale, confidence, and is_suggestion")
+			assert.Contains(t, errorContent.Text, tc.wantError)
 		})
 	}
 }

pkg/github/issues.go

@@ -2228,12 +2228,9 @@ Options are:
 			// Hand off to the interactive MCP App form unless this call must
 			// execute now (see shouldDeferToForm).
 			deferToForm := shouldDeferToForm(ctx, deps, req, args, issueWriteFormParams)
-			if method == "update" {
-				if issueType, ok := args["type"]; ok && issueType == nil {
-					// The form replaces a null type with the current type, so execute
-					// directly to preserve the clear and any co-submitted values.
-					deferToForm = false
-				}
+			if issueType, ok := args["type"]; method == "update" && ok && issueType == nil {
+				// The form restores the current type for null, so execute all inputs directly.
+				deferToForm = false
 			}
 			if deferToForm {
 				issueNumber := 0

pkg/github/issues_test.go

@@ -1435,11 +1435,6 @@ func Test_CreateIssue(t *testing.T) {
 	assert.Contains(t, tool.InputSchema.(*jsonschema.Schema).Properties, "labels")
 	assert.Contains(t, tool.InputSchema.(*jsonschema.Schema).Properties, "milestone")
 	assert.Contains(t, tool.InputSchema.(*jsonschema.Schema).Properties, "type")
-	issueTypeSchema := tool.InputSchema.(*jsonschema.Schema).Properties["type"]
-	require.Len(t, issueTypeSchema.AnyOf, 2)
-	assert.Equal(t, "string", issueTypeSchema.AnyOf[0].Type)
-	assert.Equal(t, 1, *issueTypeSchema.AnyOf[0].MinLength)
-	assert.Equal(t, "null", issueTypeSchema.AnyOf[1].Type)
 	assert.Contains(t, tool.InputSchema.(*jsonschema.Schema).Properties, "issue_fields")
 	assert.ElementsMatch(t, tool.InputSchema.(*jsonschema.Schema).Required, []string{"method", "owner", "repo"})
 
@@ -1518,18 +1513,6 @@ func Test_CreateIssue(t *testing.T) {
 				State:   github.Ptr("open"),
 			},
 		},
-		{
-			name:         "empty issue type is rejected when creating",
-			mockedClient: MockHTTPClientWithHandlers(nil),
-			requestArgs: map[string]any{
-				"method": "create",
-				"owner":  "owner",
-				"repo":   "repo",
-				"title":  "Issue without a type",
-				"type":   "",
-			},
-			expectedErrMsg: "parameter type must not be empty",
-		},
 		{
 			name: "successful issue creation with issue fields reconciled by names",
 			mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{
@@ -2891,83 +2874,7 @@ func TestIssueWriteUpdatesIssueType(t *testing.T) {
 	}
 }
 
-func TestIssueWriteRejectsEmptyIssueType(t *testing.T) {
-	tests := []struct {
-		name string
-		args map[string]any
-	}{
-		{
-			name: "create",
-			args: map[string]any{
-				"method": "create",
-				"title":  "New issue",
-			},
-		},
-		{
-			name: "update",
-			args: map[string]any{
-				"method":       "update",
-				"issue_number": float64(123),
-			},
-		},
-	}
-
-	for _, tc := range tests {
-		t.Run(tc.name, func(t *testing.T) {
-			deps := BaseDeps{Client: mustNewGHClient(t, MockHTTPClientWithHandlers(nil))}
-			serverTool := IssueWrite(translations.NullTranslationHelper)
-			handler := serverTool.Handler(deps)
-			args := map[string]any{
-				"owner": "owner",
-				"repo":  "repo",
-				"type":  "",
-			}
-			maps.Copy(args, tc.args)
-			request := createMCPRequest(args)
-
-			result, err := handler(ContextWithDeps(context.Background(), deps), &request)
-			require.NoError(t, err)
-			errorContent := getErrorResult(t, result)
-			assert.Contains(t, errorContent.Text, "parameter type must not be empty")
-		})
-	}
-}
-
-func TestIssueWriteClearTypeBypassesMCPAppForm(t *testing.T) {
-	var gotRequestBody []byte
-	var readErr error
-	client := mustNewGHClient(t, MockHTTPClientWithHandlers(map[string]http.HandlerFunc{
-		PatchReposIssuesByOwnerByRepoByIssueNumber: func(w http.ResponseWriter, r *http.Request) {
-			gotRequestBody, readErr = io.ReadAll(r.Body)
-			w.WriteHeader(http.StatusOK)
-			_, _ = w.Write([]byte(`{"number":123,"html_url":"https://github.com/owner/repo/issues/123"}`))
-		},
-	}))
-	deps := BaseDeps{
-		Client:         client,
-		GQLClient:      githubv4.NewClient(githubv4mock.NewMockedHTTPClient()),
-		featureChecker: featureCheckerFor(MCPAppsFeatureFlag),
-	}
-	serverTool := IssueWrite(translations.NullTranslationHelper)
-	handler := serverTool.Handler(deps)
-	request := createMCPRequestWithSession(t, ClientNameVSCodeInsiders, true, map[string]any{
-		"method":       "update",
-		"owner":        "owner",
-		"repo":         "repo",
-		"issue_number": float64(123),
-		"type":         nil,
-	})
-
-	result, err := handler(ContextWithDeps(context.Background(), deps), &request)
-	require.NoError(t, err)
-	require.False(t, result.IsError)
-	require.NoError(t, readErr)
-	require.JSONEq(t, `{"type":null}`, string(gotRequestBody))
-	textContent := getTextResult(t, result)
-	require.Contains(t, textContent.Text, "https://github.com/owner/repo/issues/123")
-}
-
-func TestIssueWriteClearTypeBypassesMCPAppFormWithStateChange(t *testing.T) {
+func TestIssueWriteNullTypeBypassesMCPAppFormWithStateChange(t *testing.T) {
 	var gotRequestBody []byte
 	var readErr error
 	client := mustNewGHClient(t, MockHTTPClientWithHandlers(map[string]http.HandlerFunc{
@@ -3049,8 +2956,6 @@ func TestIssueWriteClearTypeBypassesMCPAppFormWithStateChange(t *testing.T) {
 	require.False(t, result.IsError)
 	require.NoError(t, readErr)
 	require.JSONEq(t, `{"type":null}`, string(gotRequestBody))
-	textContent := getTextResult(t, result)
-	require.Contains(t, textContent.Text, "https://github.com/owner/repo/issues/123")
 }
 
 func Test_UpdateIssue(t *testing.T) {

pkg/github/params.go

@@ -34,8 +34,7 @@ func OptionalParamOK[T any, A map[string]any](args A, p string) (value T, ok boo
 	return
 }
 
-// OptionalNullableStringParam returns a non-empty string or nil while preserving
-// whether the parameter was omitted.
+// OptionalNullableStringParam preserves omitted, null, and non-empty string values.
 func OptionalNullableStringParam(args map[string]any, p string) (*string, bool, error) {
 	value, ok := args[p]
 	if !ok {