Validate required params in discussion read handlers#2741
Open
rodboev wants to merge 2 commits into
Open
Conversation
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.
Summary
Fixes
get_discussionandget_discussion_comments, which accepted calls with missing required parameters and silently issued GraphQL queries with zero or empty values, by replacingmapstructure.WeakDecodewith theRequiredParam/RequiredIntvalidation pattern already used by every write handler in the same file.Why
Both read handlers declare a local struct and call
mapstructure.WeakDecode(args, ¶ms)(discussions.go:317 and discussions.go:425). WeakDecode populates struct fields from the args map using loose type coercion and leaves fields at their zero value when a key is absent. A call withoutownersilently sets it to"", which then reaches the GraphQL layer and produces a confusing API-level error rather than a clear MCP tool error naming the missing field.PR #2718 replaced WeakDecode with
RequiredParam/RequiredIntin every write handler in the same file. The read handlers were not included in that pass.Fixes #2740 (filed alongside this PR)
What changed
pkg/github/discussions.go: replacedmapstructure.WeakDecodeinget_discussionandget_discussion_commentshandlers withRequiredParam[string]/RequiredIntcalls; removed local params structs.pkg/github/discussions_test.go: added missing-param error test cases for both handlers; removedTest_GetDiscussionWithStringNumberandTest_GetDiscussionCommentsWithStringNumber(WeakDecode string coercion no longer applies).Tool parameters and schemas are unchanged.
MCP impact
Handler-only change — tool name, description, and InputSchema for
get_discussionandget_discussion_commentsare unchanged. Toolsnaps and generated docs are unaffected.Prompts tested (tool changes only)
N/A (no tool schema or behavior changes for well-formed calls).
Security / limits
Both handlers read from a user-supplied args map and pass validated values to GraphQL queries. No new scopes, no data exposure changes.
Tool renaming
Lint & tests
./script/lintDirect lint commands passed locally:
gofmt -s -w pkg/github/discussions.go pkg/github/discussions_test.goproduced no diff;golangci-lint runreported 0 issues../script/test./script/testwas not run locally.go test -race ./...passed locally (all packages ok).Docs
No tool schema changed; script/generate-docs output is unaffected.