Skip to content

Commit 1bf9a65

Browse files
committed
fix(pull_requests): validate required params in add_comment_to_pending_review
Replace mapstructure.WeakDecode with RequiredParam/RequiredInt checks so missing arguments like owner return a clear validation error instead of running the GraphQL query with zero values. Fixes #2718
1 parent 9430064 commit 1bf9a65

2 files changed

Lines changed: 82 additions & 22 deletions

File tree

pkg/github/pullrequests.go

Lines changed: 58 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -2277,38 +2277,74 @@ func AddCommentToPendingReview(t translations.TranslationHelperFunc) inventory.S
22772277
},
22782278
[]scopes.Scope{scopes.Repo},
22792279
func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) {
2280-
var params struct {
2281-
Owner string
2282-
Repo string
2283-
PullNumber int32
2284-
Path string
2285-
Body string
2286-
SubjectType string
2287-
Line *int32
2288-
Side *string
2289-
StartLine *int32
2290-
StartSide *string
2280+
owner, err := RequiredParam[string](args, "owner")
2281+
if err != nil {
2282+
return utils.NewToolResultError(err.Error()), nil, nil
22912283
}
2292-
if err := mapstructure.WeakDecode(args, &params); err != nil {
2284+
repo, err := RequiredParam[string](args, "repo")
2285+
if err != nil {
2286+
return utils.NewToolResultError(err.Error()), nil, nil
2287+
}
2288+
pullNumber, err := RequiredInt(args, "pullNumber")
2289+
if err != nil {
2290+
return utils.NewToolResultError(err.Error()), nil, nil
2291+
}
2292+
path, err := RequiredParam[string](args, "path")
2293+
if err != nil {
22932294
return utils.NewToolResultError(err.Error()), nil, nil
22942295
}
2296+
body, err := RequiredParam[string](args, "body")
2297+
if err != nil {
2298+
return utils.NewToolResultError(err.Error()), nil, nil
2299+
}
2300+
subjectType, err := RequiredParam[string](args, "subjectType")
2301+
if err != nil {
2302+
return utils.NewToolResultError(err.Error()), nil, nil
2303+
}
2304+
line, err := OptionalIntParam(args, "line")
2305+
if err != nil {
2306+
return utils.NewToolResultError(err.Error()), nil, nil
2307+
}
2308+
side, _ := OptionalParam[string](args, "side")
2309+
startLine, err := OptionalIntParam(args, "startLine")
2310+
if err != nil {
2311+
return utils.NewToolResultError(err.Error()), nil, nil
2312+
}
2313+
startSide, _ := OptionalParam[string](args, "startSide")
22952314

22962315
client, err := deps.GetGQLClient(ctx)
22972316
if err != nil {
22982317
return utils.NewToolResultErrorFromErr("failed to get GitHub GQL client", err), nil, nil
22992318
}
23002319

2320+
var linePtr, startLinePtr *int32
2321+
if line != 0 {
2322+
l := int32(line) // #nosec G115
2323+
linePtr = &l
2324+
}
2325+
if startLine != 0 {
2326+
sl := int32(startLine) // #nosec G115
2327+
startLinePtr = &sl
2328+
}
2329+
var sidePtr, startSidePtr *string
2330+
if side != "" {
2331+
sidePtr = &side
2332+
}
2333+
if startSide != "" {
2334+
startSidePtr = &startSide
2335+
}
2336+
23012337
result, err := AddCommentToPendingReviewCall(ctx, client, AddCommentToPendingReviewParams{
2302-
Owner: params.Owner,
2303-
Repo: params.Repo,
2304-
PullNumber: params.PullNumber,
2305-
Path: params.Path,
2306-
Body: params.Body,
2307-
SubjectType: params.SubjectType,
2308-
Line: params.Line,
2309-
Side: params.Side,
2310-
StartLine: params.StartLine,
2311-
StartSide: params.StartSide,
2338+
Owner: owner,
2339+
Repo: repo,
2340+
PullNumber: int32(pullNumber), // #nosec G115 - PR numbers are always small positive integers
2341+
Path: path,
2342+
Body: body,
2343+
SubjectType: subjectType,
2344+
Line: linePtr,
2345+
Side: sidePtr,
2346+
StartLine: startLinePtr,
2347+
StartSide: startSidePtr,
23122348
})
23132349
return result, nil, err
23142350
})

pkg/github/pullrequests_test.go

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3516,6 +3516,30 @@ func TestAddPullRequestReviewCommentToPendingReview(t *testing.T) {
35163516
),
35173517
),
35183518
},
3519+
{
3520+
name: "missing required parameter owner",
3521+
requestArgs: map[string]any{
3522+
"repo": "gated-probe",
3523+
"pullNumber": float64(1),
3524+
"path": "f.go",
3525+
"body": "x",
3526+
"subjectType": "LINE",
3527+
},
3528+
expectToolError: true,
3529+
expectedToolErrMsg: "missing required parameter: owner",
3530+
},
3531+
{
3532+
name: "missing required parameter path",
3533+
requestArgs: map[string]any{
3534+
"owner": "owner",
3535+
"repo": "repo",
3536+
"pullNumber": float64(42),
3537+
"body": "This is a test comment",
3538+
"subjectType": "LINE",
3539+
},
3540+
expectToolError: true,
3541+
expectedToolErrMsg: "missing required parameter: path",
3542+
},
35193543
{
35203544
name: "thread ID is nil - invalid line number",
35213545
requestArgs: map[string]any{

0 commit comments

Comments
 (0)