Skip to content

Commit 0f2ad87

Browse files
Guard issue_write/create_pull_request schemas against UI-gating desync
The form-routing logic depends on a hand-maintained classification of each schema property into form-resendable vs known-non-form. A new property added without updating the classification would silently shift UI gating behavior (e.g. a form-incompatible param wouldn't trigger the safety-net bypass). Add Test_issueWriteSchemaClassification and Test_createPullRequestSchemaClassification that enumerate each tool's InputSchema.Properties and require every property to be classified as exactly one of: - form-resendable (member of issueWriteFormParams / pullRequestWriteFormParams) - known-non-form (test-local allowlist) A future schema addition without classification fails the test with a message pointing at the exact set the contributor needs to update. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent 76b9cfb commit 0f2ad87

2 files changed

Lines changed: 77 additions & 0 deletions

File tree

pkg/github/issues_test.go

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ import (
1515
"github.com/github/github-mcp-server/internal/toolsnaps"
1616
"github.com/github/github-mcp-server/pkg/http/headers"
1717
transportpkg "github.com/github/github-mcp-server/pkg/http/transport"
18+
"github.com/github/github-mcp-server/pkg/inventory"
1819
"github.com/github/github-mcp-server/pkg/translations"
1920
"github.com/google/go-github/v87/github"
2021
"github.com/google/jsonschema-go/jsonschema"
@@ -1907,6 +1908,56 @@ func Test_issueWriteHasNonFormParams(t *testing.T) {
19071908
}
19081909
}
19091910

1911+
// Test_issueWriteSchemaClassification fails when a schema property is added
1912+
// without classifying it as either form-resendable (issueWriteFormParams) or
1913+
// known-non-form (knownNonForm below). Without this guard, an unclassified
1914+
// property would silently flip UI gating: form-incompatible fields would
1915+
// stop tripping the safety-net bypass and the form would drop their values.
1916+
func Test_issueWriteSchemaClassification(t *testing.T) {
1917+
t.Parallel()
1918+
1919+
// Schema properties the MCP App form cannot represent — their presence
1920+
// must trigger the safety-net bypass via issueWriteHasNonFormParams.
1921+
knownNonForm := map[string]struct{}{
1922+
"assignees": {},
1923+
"labels": {},
1924+
"milestone": {},
1925+
"type": {},
1926+
"state": {},
1927+
"state_reason": {},
1928+
"duplicate_of": {},
1929+
"issue_fields": {}, // only on the FF-enabled IssueWrite variant
1930+
}
1931+
1932+
cases := []struct {
1933+
name string
1934+
tool inventory.ServerTool
1935+
}{
1936+
{name: "IssueWrite", tool: IssueWrite(translations.NullTranslationHelper)},
1937+
{name: "LegacyIssueWrite", tool: LegacyIssueWrite(translations.NullTranslationHelper)},
1938+
}
1939+
1940+
for _, tc := range cases {
1941+
t.Run(tc.name, func(t *testing.T) {
1942+
t.Parallel()
1943+
schema, ok := tc.tool.Tool.InputSchema.(*jsonschema.Schema)
1944+
require.True(t, ok, "InputSchema should be *jsonschema.Schema")
1945+
1946+
for prop := range schema.Properties {
1947+
_, isForm := issueWriteFormParams[prop]
1948+
_, isNonForm := knownNonForm[prop]
1949+
1950+
assert.Falsef(t, isForm && isNonForm,
1951+
"property %q is classified as both form-resendable and non-form — pick one", prop)
1952+
assert.Truef(t, isForm || isNonForm,
1953+
"property %q in %s schema is unclassified — add it to issueWriteFormParams (pkg/github/issues.go) "+
1954+
"if the MCP App form can carry it on submit, otherwise add it to the knownNonForm allowlist in this test",
1955+
prop, tc.name)
1956+
}
1957+
})
1958+
}
1959+
}
1960+
19101961
func Test_ListIssues(t *testing.T) {
19111962
// Verify tool definition
19121963
serverTool := ListIssues(translations.NullTranslationHelper)

pkg/github/pullrequests_test.go

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2615,6 +2615,32 @@ func Test_pullRequestWriteHasNonFormParams(t *testing.T) {
26152615
}
26162616
}
26172617

2618+
// Test_createPullRequestSchemaClassification fails when a schema property is
2619+
// added without classifying it as either form-resendable
2620+
// (pullRequestWriteFormParams) or known-non-form (knownNonForm below).
2621+
// Today every property is form-resendable, so knownNonForm is empty.
2622+
func Test_createPullRequestSchemaClassification(t *testing.T) {
2623+
t.Parallel()
2624+
2625+
knownNonForm := map[string]struct{}{}
2626+
2627+
tool := CreatePullRequest(translations.NullTranslationHelper)
2628+
schema, ok := tool.Tool.InputSchema.(*jsonschema.Schema)
2629+
require.True(t, ok, "InputSchema should be *jsonschema.Schema")
2630+
2631+
for prop := range schema.Properties {
2632+
_, isForm := pullRequestWriteFormParams[prop]
2633+
_, isNonForm := knownNonForm[prop]
2634+
2635+
assert.Falsef(t, isForm && isNonForm,
2636+
"property %q is classified as both form-resendable and non-form — pick one", prop)
2637+
assert.Truef(t, isForm || isNonForm,
2638+
"property %q in create_pull_request schema is unclassified — add it to pullRequestWriteFormParams "+
2639+
"(pkg/github/pullrequests.go) if the MCP App form can carry it on submit, otherwise add it to "+
2640+
"the knownNonForm allowlist in this test", prop)
2641+
}
2642+
}
2643+
26182644
func TestCreateAndSubmitPullRequestReview(t *testing.T) {
26192645
t.Parallel()
26202646

0 commit comments

Comments
 (0)