Repository navigation
fix: return non-nil empty completion results - #3665
Conversation
Return an empty values array for prompt and non-resource completions to prevent nil result finalization panics. Add regression coverage for serialization, resource routing, and errors. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟢 Approval recommended
No active review issues are identified, and the change addresses nil completion results with corresponding coverage for result handling and routing.
No open issues 🎉
What changed in this PR
This PR prevents nil completion results from causing SDK finalization panics by returning non-nil empty results that serialize as values: [] for prompt completions and direct non-resource repository completion calls.
Changes:
- Return initialized empty
CompleteResultvalues for both completion paths. - Preserve existing repository-resource routing and error behavior.
- Add coverage for empty results, JSON serialization, routing, client errors, and unsupported references or URIs.
| File | Description |
|---|---|
pkg/​github/​server.go |
Returns a non-nil empty result for prompt completions. |
pkg/​github/​server_test.go |
Tests completion routing, errors, non-nil results, and JSON serialization. |
pkg/​github/​repository_resource_completions.go |
Returns a non-nil empty result for direct non-resource calls. |
pkg/​github/​repository_resource_completions_test.go |
Expands coverage for empty results and serialization. |
Review effort: Lite (auto)
notlakshitha
left a comment
There was a problem hiding this comment.
nice, this covers the prompt/non-resource paths. one more spot that can still send "values": null though: the success path at repository_resource_completions.go:66-72 passes the resolver output straight through, and every resolver starts from var values []string (e.g. :79, :95 for the owner filter, :126, :153, :173...). so when nothing matches, e.g. typing an owner prefix that matches none of your orgs, the resolver hands back a nil slice and the response marshals as:
{"completion":{"values":null}}
(CompletionResultDetails.Values has no omitempty in go-sdk, so nil -> null.) I checked this on the PR head with a throwaway test that swaps the owner resolver for one returning an empty filtered slice.
maybe just normalize before building the result:
if values == nil {
values = []string{}
}plus a "no matches" case in TestRepositoryResourceCompletionHandler asserting {"completion":{"values":[],"total":0}}, so the empty-result case is covered too and not just the early returns.
Ensure successful repository resource completion responses serialize empty values as an array when a resolver has no matches. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
Thanks @notlakshitha! I added that fix and a test for that. |
Summary
Return non-nil empty completion results for prompt completions and direct non-resource calls to the repository completion handler. Both paths serialize completion values as
[], notnull.Why
A successful completion handler returning a typed nil
*mcp.CompleteResultcan panic during SDK result finalization after request recovery middleware has returned. This fixes the upstream source of that production crash; the remote defense-in-depth wrapper is tracked in github/github-mcp-server-remote#1108.No upstream issue is associated with this fix. The remote dependency bump is release-dependent and is not included here; no SDK changes are included.
What changed
CompleteResultwith an initialized values slice forref/promptinCompletionsHandler.RepositoryResourceCompletionHandler.values: []for both paths, and cover unchangedrepo://routing, client errors, unsupported references, and unsupported resource URIs.MCP impact
No tool definitions or schemas change. Empty completion responses now return a valid result instead of a nil pointer; repository resource resolution and error handling are unchanged.
Prompts tested (tool changes only)
Security / limits
This prevents a completion-result nil-pointer crash without changing authentication, permissions, data exposure, or limits.
Tool renaming
deprecated_tool_aliases.goNo tool names change.
Note: if you're renaming tools, you must add the tool aliases. For more information on how to do so, please refer to the official docs.
Lint & tests
./script/lint./script/testValidation results:
go test ./pkg/github -run 'Test(CompletionsHandler|RepositoryResourceCompletionHandler)' -count=1— passed.script/lint— passed, 0 issues. The initial automatic linter download stalled; reran successfully using the already-installed matching golangci-lint v2.14.0 binary in the ignoredbin/directory.script/test— passed (go test -race ./...).go build ./cmd/github-mcp-server— passed.git diff --check— passed.Docs
Internal completion bug fix only; no tool definitions, schemas, toolsets, or generated documentation change.