refactor(mcp-apps): centralize the show/defer decision (single source…

… of truth) The defer-to-form predicate was triplicated across create_pull_request, update_pull_request, and issue_write, each with its own near-identical *HasNonFormParams function. As more MCP App tools are added this duplication would grow and the copies could silently drift. Extract one shared gate in ui_capability.go: - shouldDeferToForm(ctx, deps, req, args, formParams) — the single show/defer decision (MCP Apps enabled, client supports UI, not a form submission, and no non-form params). - hasNonFormParams(args, formParams) — one generic helper replacing the three per-tool functions. - uiSubmitted(args) — small shared predicate. Each handler is now a one-line `if shouldDeferToForm(...) { return awaiting }`. The per-tool form-parameter allowlists and the user-facing messages stay per-tool (that is the genuine per-tool config). Pure refactor — behavior unchanged; existing tests now exercise the generic helper. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
SamMorrowDrums · Jun 26, 2026 · Jun 26, 2026 · Jun 26, 2026 · Jun 26, 2026 · Jun 26, 2026
commit 8b2363d31eba31600f3bfee6ad76d2af8a9f755e
@@ -1726,7 +1726,11 @@ const IssueWriteUIResourceURI = "ui://github-mcp-server/issue-write"
 
 // issueWriteFormParams are the parameters the issue_write MCP App form collects
 // and re-sends on submit. Any other parameter present on a call cannot be
-// represented by the form.
+// represented by the form. The form collects (and prefills) every parameter in
+// the tool's current input schema, so hasNonFormParams against this set is a
+// forward-compatibility safety net: a parameter added to the schema in the
+// future but not yet wired into the form trips the check and bypasses the form
+// so the supplied value isn't silently dropped.
 var issueWriteFormParams = map[string]struct{}{
 	"method":        {},
 	"owner":         {},
@@ -1745,24 +1749,6 @@ var issueWriteFormParams = map[string]struct{}{
 	"_ui_submitted": {},
 }
 
-// issueWriteHasNonFormParams reports whether the call carries any parameter the
-// issue_write MCP App form cannot represent (anything outside issueWriteFormParams).
-// The form collects (and prefills) every parameter in the tool's current input
-// schema, so this is a forward-compatibility safety net: a parameter added to the
-// schema in the future but not yet wired into the form trips this check and bypasses
-// the UI so the supplied value isn't silently dropped.
-func issueWriteHasNonFormParams(args map[string]any) bool {
-	for key, value := range args {
-		if value == nil {
-			continue
-		}
-		if _, ok := issueWriteFormParams[key]; !ok {
-			return true
-		}
-	}
-	return false
-}
-
 // issueWriteAwaitingFormResult builds the "awaiting form submission" stub
 // returned when issue_write hands off to the MCP App form. The body is shared
 // by IssueWrite and LegacyIssueWrite. The result is marked IsError=true so
@@ -1938,15 +1924,9 @@ Options are:
 				return utils.NewToolResultError(err.Error()), nil, nil
 			}
 
-			// When MCP Apps are enabled and the client supports UI, route the
-			// call to the interactive form unless:
-			//   - it is itself a form submission (the UI sends _ui_submitted=true), or
-			//   - it carries parameters the form cannot represent (e.g. labels,
-			//     assignees or issue_fields). Those must be applied directly so
-			//     their values aren't silently dropped.
-			uiSubmitted, _ := OptionalParam[bool](args, "_ui_submitted")
-
-			if deps.IsFeatureEnabled(ctx, MCPAppsFeatureFlag) && clientSupportsUI(ctx, req) && !uiSubmitted && !issueWriteHasNonFormParams(args) {
+			// Hand off to the interactive MCP App form unless this call must
+			// execute now (see shouldDeferToForm).
+			if shouldDeferToForm(ctx, deps, req, args, issueWriteFormParams) {
 				issueNumber := 0
 				if method == "update" {
 					n, numErr := RequiredInt(args, "issue_number")

@@ -1701,7 +1701,7 @@ func Test_issueWriteHasNonFormParams(t *testing.T) {
 	for _, tc := range tests {
 		t.Run(tc.name, func(t *testing.T) {
 			t.Parallel()
-			assert.Equal(t, tc.want, issueWriteHasNonFormParams(tc.args))
+			assert.Equal(t, tc.want, hasNonFormParams(tc.args, issueWriteFormParams))
 		})
 	}
 }
@@ -1715,7 +1715,7 @@ func Test_issueWriteSchemaClassification(t *testing.T) {
 	t.Parallel()
 
 	// Schema properties the MCP App form cannot represent — their presence
-	// must trigger the safety-net bypass via issueWriteHasNonFormParams. The
+	// must trigger the safety-net bypass via hasNonFormParams. The
 	// form currently collects every schema property, so this allowlist is
 	// empty; add a property here only if it is added to the schema without
 	// corresponding form support.

@@ -620,34 +620,6 @@ var pullRequestUpdateFormParams = map[string]struct{}{
 	"_ui_submitted":         {},
 }
 
-// pullRequestWriteHasNonFormParams reports whether the call carries any parameter
-// the create_pull_request MCP App form cannot represent (anything outside
-// pullRequestWriteFormParams). Such calls must bypass the UI form and execute
-// directly so the supplied values aren't silently dropped.
-func pullRequestWriteHasNonFormParams(args map[string]any) bool {
-	for key, value := range args {
-		if value == nil {
-			continue
-		}
-		if _, ok := pullRequestWriteFormParams[key]; !ok {
-			return true
-		}
-	}
-	return false
-}
-
-func pullRequestUpdateHasNonFormParams(args map[string]any) bool {
-	for key, value := range args {
-		if value == nil {
-			continue
-		}
-		if _, ok := pullRequestUpdateFormParams[key]; !ok {
-			return true
-		}
-	}
-	return false
-}
-
 // CreatePullRequest creates a tool to create a new pull request.
 func CreatePullRequest(t translations.TranslationHelperFunc) inventory.ServerTool {
 	return NewTool(
@@ -722,14 +694,9 @@ func CreatePullRequest(t translations.TranslationHelperFunc) inventory.ServerToo
 				return utils.NewToolResultError(err.Error()), nil, nil
 			}
 
-			// When MCP Apps are enabled and the client supports UI, route the
-			// call to the interactive form unless:
-			//   - it is itself a form submission (the UI sends _ui_submitted=true), or
-			//   - it carries parameters the form cannot represent. Those must be
-			//     applied directly so their values aren't silently dropped.
-			uiSubmitted, _ := OptionalParam[bool](args, "_ui_submitted")
-
-			if deps.IsFeatureEnabled(ctx, MCPAppsFeatureFlag) && clientSupportsUI(ctx, req) && !uiSubmitted && !pullRequestWriteHasNonFormParams(args) {
+			// Hand off to the interactive MCP App form unless this call must
+			// execute now (see shouldDeferToForm).
+			if shouldDeferToForm(ctx, deps, req, args, pullRequestWriteFormParams) {
 				return utils.NewToolResultAwaitingFormSubmission(fmt.Sprintf(
 					"An interactive form has been shown to the user for creating a new pull request in %s/%s. "+
 						"STOP — do not call any other tools, do not respond as if the pull request was created, "+
@@ -948,8 +915,9 @@ func UpdatePullRequest(t translations.TranslationHelperFunc) inventory.ServerToo
 				return utils.NewToolResultError(err.Error()), nil, nil
 			}
 
-			uiSubmitted, _ := OptionalParam[bool](args, "_ui_submitted")
-			if deps.IsFeatureEnabled(ctx, MCPAppsFeatureFlag) && clientSupportsUI(ctx, req) && !uiSubmitted && !pullRequestUpdateHasNonFormParams(args) {
+			// Hand off to the interactive MCP App form unless this call must
+			// execute now (see shouldDeferToForm).
+			if shouldDeferToForm(ctx, deps, req, args, pullRequestUpdateFormParams) {
 				return utils.NewToolResultAwaitingFormSubmission(fmt.Sprintf(
 					"An interactive form has been shown to the user for editing pull request #%d in %s/%s. "+
 						"STOP — do not call any other tools, do not respond as if the pull request was updated, "+

@@ -2801,7 +2801,7 @@ func Test_pullRequestWriteHasNonFormParams(t *testing.T) {
 	for _, tc := range tests {
 		t.Run(tc.name, func(t *testing.T) {
 			t.Parallel()
-			assert.Equal(t, tc.want, pullRequestWriteHasNonFormParams(tc.args))
+			assert.Equal(t, tc.want, hasNonFormParams(tc.args, pullRequestWriteFormParams))
 		})
 	}
 }

@@ -33,3 +33,44 @@ func clientSupportsUI(ctx context.Context, req *mcp.CallToolRequest) bool {
 	}
 	return false
 }
+
+// uiSubmitted reports whether the call is itself an MCP App form submission.
+// The form re-invokes its tool with _ui_submitted=true; such calls must execute
+// rather than re-render the form.
+func uiSubmitted(args map[string]any) bool {
+	submitted, _ := OptionalParam[bool](args, "_ui_submitted")
+	return submitted
+}
+
+// hasNonFormParams reports whether the call carries any parameter the tool's MCP
+// App form cannot represent (anything outside formParams). Such calls must
+// bypass the form and execute directly so the supplied values aren't silently
+// dropped. formParams is the set of parameters the form collects and re-sends
+// on submit.
+func hasNonFormParams(args map[string]any, formParams map[string]struct{}) bool {
+	for key, value := range args {
+		if value == nil {
+			continue
+		}
+		if _, ok := formParams[key]; !ok {
+			return true
+		}
+	}
+	return false
+}
+
+// shouldDeferToForm is the single source of truth for the show/defer decision
+// shared by the form-backed write tools (create_pull_request,
+// update_pull_request, issue_write). It reports whether a call should be handed
+// off to its MCP App form instead of executing now: defer only when MCP Apps
+// are enabled, the client can render UI, the call is not itself a form
+// submission, and every supplied parameter can be represented by the form
+// (formParams is the tool's form-parameter allowlist). When it returns false
+// the handler executes directly; the host may still render the tool's view,
+// which renders the result rather than an input form.
+func shouldDeferToForm(ctx context.Context, deps ToolDependencies, req *mcp.CallToolRequest, args map[string]any, formParams map[string]struct{}) bool {
+	return deps.IsFeatureEnabled(ctx, MCPAppsFeatureFlag) &&
+		clientSupportsUI(ctx, req) &&
+		!uiSubmitted(args) &&
+		!hasNonFormParams(args, formParams)
+}