fix(mcp-apps): reconcile the show/defer contract — render results, remove show_ui#2774
Merged
Merged
Conversation
The create_pull_request / issue_write / update_pull_request Views decided form-vs-success from in-app submit state only, ignoring the tool-result the host delivers on render. Per the MCP Apps 2026-01-26 spec the host renders a View whenever the tool carries _meta.ui.resourceUri — independent of whether the server deferred or executed. So when the server executed up-front (e.g. show_ui=false, or parameters the form can't represent) the View still showed its "Create pull request" input form over an already-created PR, which reads as a bug (it even shows a PR number). Drive the Views off the result instead: a new shared completedToolResult() helper returns parsed data only for a genuine completed success, and returns null for the awaiting_user_submission deferral sentinel, errors, or no result. Each write View now shows its success card when that completed result is present, so the form is only ever shown while the action is genuinely deferred. Reconciles the show/defer state machine at the View (decision layer that the host result feeds). See github/copilot-mcp-core#1864. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the MCP Apps write Views (create PR, edit PR, issue write) so they render a success state based on the host-provided tool-result when the server executed up-front (instead of deferring to the form), using a new shared helper completedToolResult().
Changes:
- Added
ui/src/lib/toolResult.tswithcompletedToolResult()to parse completed successfulCallToolResultpayloads and ignore deferral/error results. - Updated PR create/edit and issue write Views to prefer
toolResult-derived success data when present, preventing forms from rendering over already-completed operations.
Show a summary per file
| File | Description |
|---|---|
| ui/src/lib/toolResult.ts | New helper to detect/parse completed tool results vs. deferral/error results. |
| ui/src/apps/pr-write/App.tsx | Uses toolResult to show success when the PR was created up-front. |
| ui/src/apps/pr-edit/App.tsx | Uses toolResult to show success when the PR was updated up-front. |
| ui/src/apps/issue-write/App.tsx | Uses toolResult to show success when the issue was created/updated up-front. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 3
- Review effort level: Low
Comment on lines
+279
to
+287
| // When the server updated the PR up-front instead of deferring to this form, | ||
| // the host still renders this View and delivers the updated PR via | ||
| // tool-result. Render that completed result as success so we never show an | ||
| // edit form for changes already applied. The deferral sentinel | ||
| // (awaiting_user_submission) returns null here, keeping the form for the | ||
| // normal deferred flow. See github/copilot-mcp-core#1864. | ||
| const resultPR = useMemo(() => completedToolResult<PRResult>(toolResult), [toolResult]); | ||
| const shownPR = successPR ?? resultPR; | ||
|
|
Collaborator
Author
There was a problem hiding this comment.
Good catch — fixed in 09a7a78. Rather than key the parsed result per app, I scoped it centrally in useMcpApp: toolResult is now reset to null whenever a new tool-input notification arrives. The MCP Apps 2026-01-26 spec guarantees tool-input is delivered before that invocation's tool-result, so a new invocation drops any prior result before its own arrives — closing the stale-success window for all three Views without per-app invocation keys.
Address review feedback: the write Views derive their success card from `toolResult`, but it wasn't cleared when a new invocation arrived (only the in-app `successPR`/`successIssue` was reset on `toolInput` change). A completed result from a previous invocation could briefly render a stale success card over the next, still-deferred form. Clear `toolResult` whenever a new `tool-input` notification arrives. The spec guarantees `tool-input` precedes that invocation's `tool-result`, so this scopes the result to the current invocation centrally in the hook — fixing all three Views without per-app invocation keys. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
show_ui promised "skip the form and execute directly", but it can't deliver: the host renders an MCP App for any tool that carries _meta.ui.resourceUri, and the 2026-01-26 MCP Apps spec has no per-call/per-result way to opt out of rendering. show_ui only flipped the server's defer decision, so show_ui=false created the PR/issue up-front yet the host still rendered the app — exactly the contradiction this work set out to fix. And show_ui is only ever exposed to clients that support UI, i.e. precisely the clients that always render the app. Remove it entirely: - Drop the show_ui schema property, the form-param allowlist entry, and the showUI term from the defer predicate in create_pull_request and issue_write. The gate is now FF && clientSupportsUI && !_ui_submitted && !hasNonFormParams. - Delete the now-unused UI-only schema-property strip machinery in pkg/inventory (uiOnlySchemaProperties, stripUIOnlySchemaProperties, stripSchemaProperties) and the exported ConditionalSchemaPropertyDescriptions, which existed solely to surface show_ui to UI-capable clients. _meta.ui stripping is untouched. - Drop the conditional-property annotation from the docs generator. - Update toolsnaps, generated docs, and tests. With the up-front-execution Views now rendering the result (success card), the remaining contract is simple: when MCP Apps are enabled the form is the path, and the form is only shown while the action is genuinely deferred. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
SamMorrowDrums
changed the title
… 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>
This was referenced Jun 27, 2026
syf2211
added a commit
to syf2211/github-mcp-server
that referenced
this pull request
Jun 27, 2026
…move show_ui (github#2774) * fix(ui): render success view when an MCP App tool executed up-front The create_pull_request / issue_write / update_pull_request Views decided form-vs-success from in-app submit state only, ignoring the tool-result the host delivers on render. Per the MCP Apps 2026-01-26 spec the host renders a View whenever the tool carries _meta.ui.resourceUri — independent of whether the server deferred or executed. So when the server executed up-front (e.g. show_ui=false, or parameters the form can't represent) the View still showed its "Create pull request" input form over an already-created PR, which reads as a bug (it even shows a PR number). Drive the Views off the result instead: a new shared completedToolResult() helper returns parsed data only for a genuine completed success, and returns null for the awaiting_user_submission deferral sentinel, errors, or no result. Each write View now shows its success card when that completed result is present, so the form is only ever shown while the action is genuinely deferred. Reconciles the show/defer state machine at the View (decision layer that the host result feeds). See github/copilot-mcp-core#1864. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix(ui): scope tool-result to the current invocation Address review feedback: the write Views derive their success card from `toolResult`, but it wasn't cleared when a new invocation arrived (only the in-app `successPR`/`successIssue` was reset on `toolInput` change). A completed result from a previous invocation could briefly render a stale success card over the next, still-deferred form. Clear `toolResult` whenever a new `tool-input` notification arrives. The spec guarantees `tool-input` precedes that invocation's `tool-result`, so this scopes the result to the current invocation centrally in the hook — fixing all three Views without per-app invocation keys. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * refactor(mcp-apps): remove show_ui — it can't suppress app rendering show_ui promised "skip the form and execute directly", but it can't deliver: the host renders an MCP App for any tool that carries _meta.ui.resourceUri, and the 2026-01-26 MCP Apps spec has no per-call/per-result way to opt out of rendering. show_ui only flipped the server's defer decision, so show_ui=false created the PR/issue up-front yet the host still rendered the app — exactly the contradiction this work set out to fix. And show_ui is only ever exposed to clients that support UI, i.e. precisely the clients that always render the app. Remove it entirely: - Drop the show_ui schema property, the form-param allowlist entry, and the showUI term from the defer predicate in create_pull_request and issue_write. The gate is now FF && clientSupportsUI && !_ui_submitted && !hasNonFormParams. - Delete the now-unused UI-only schema-property strip machinery in pkg/inventory (uiOnlySchemaProperties, stripUIOnlySchemaProperties, stripSchemaProperties) and the exported ConditionalSchemaPropertyDescriptions, which existed solely to surface show_ui to UI-capable clients. _meta.ui stripping is untouched. - Drop the conditional-property annotation from the docs generator. - Update toolsnaps, generated docs, and tests. With the up-front-execution Views now rendering the result (success card), the remaining contract is simple: when MCP Apps are enabled the form is the path, and the form is only shown while the action is genuinely deferred. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * 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> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
When the GitHub MCP server creates a PR/issue in an MCP Apps–capable host, the host renders the "Create pull request" form even when the action has already happened (it shows a PR number) — a deferred-action UI that looks like a bug. Reported in #copilot-mcp-core Slack.
Root cause is an unenforced contract across three layers: the host renders a View off the tool's static
_meta.ui.resourceUri(per the 2026-01-26 MCP Apps spec), the server decides defer-vs-execute off runtime args, and the View chose form-vs-success off in-app state only. They could disagree. Full state-machine analysis: github/copilot-mcp-core#1864.Changes
1. Views render from
tool-result(the fix for the reported bug).The write Views (
pr-write,issue-write,pr-edit) now derive success from the host-deliveredtool-resultvia a newcompletedToolResult()helper:awaiting_user_submissionsentinel / no result → input formSo if the server executed up-front, the View shows a success card instead of a contradictory form.
useMcpAppalso clearstoolResulton each newtool-input, scoping the result to the current invocation.2. Remove
show_uientirely.show_uipromised "skip the form and execute directly", but it can't: the host renders an App for any tool carrying_meta.ui, and the spec has no per-call render opt-out. It only flipped the server's defer decision, soshow_ui=falsecreated the PR up-front while the host still rendered the form — the exact contradiction above. And it was only ever exposed to UI-capable clients (the ones that always render). Removed from the schema, the form-param allowlists, and the defer predicate. The now-orphaned UI-only schema-strip machinery inpkg/inventory(incl. the exportedConditionalSchemaPropertyDescriptions) is deleted rather than left as a no-op;_meta.uistripping is untouched.3. Centralize the show/defer decision (single source of truth).
The defer predicate was triplicated across the three handlers, each with its own near-identical
*HasNonFormParamsfunction. Extracted to one shared gate inui_capability.go:shouldDeferToForm(ctx, deps, req, args, formParams)+ a generichasNonFormParams. Each handler is nowif shouldDeferToForm(...) { return awaiting }. Per-tool allowlists and messages stay per-tool. Pure refactor; adding the next MCP App tool now reuses one gate.Contract after this PR
Scope
ui/src/lib/toolResult.ts(new),ui/src/hooks/useMcpApp.ts,ui/src/apps/{pr-write,issue-write,pr-edit}/App.tsxpkg/github/{pullrequests,issues,ui_capability}.go(+ tests, toolsnaps),pkg/inventory/{builder,registry}.go(+ tests),cmd/github-mcp-server/generate_docs.go, generated docsValidation
tsc✅ ·script/build-ui✅ ·script/lint✅ (0 issues) ·script/test✅ (race) ·script/generate-docs✅ · toolsnaps regenerated.State-machine gaps (tracked in #1864)
show_ui) → removed. G2/G3 → fixed by result-driven rendering. G4 (app calls spawning new apps) → confirmed not real. G5 (centralize the defer predicate) → done.