Skip to content

Commit e0db637

Browse files
fix(pull-requests): preserve legacy validation order
Replay parameter checks in handler order and scope cursor validation to review-thread reads. Keep commit message schema descriptions distinct from operation messages. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
1 parent d351532 commit e0db637

3 files changed

Lines changed: 118 additions & 20 deletions

File tree

‎pkg/github/__toolsnaps__/pull_request_read.snap‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -730,7 +730,6 @@
730730
"type": "string"
731731
},
732732
"message": {
733-
"description": "Operation result message.",
734733
"type": "string"
735734
},
736735
"sha": {

‎pkg/github/consolidated_pull_request_types.go‎

Lines changed: 103 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -339,7 +339,9 @@ func describePullRequestOutputSchema(schema *jsonschema.Schema) {
339339
case "total_count", "totalCount":
340340
field.Description = "Total number of results."
341341
case "message":
342-
field.Description = "Operation result message."
342+
if schema.Properties["author"] == nil {
343+
field.Description = "Operation result message."
344+
}
343345
case "merged":
344346
field.Description = "Whether the pull request was merged."
345347
case "diff":
@@ -412,17 +414,17 @@ type pullRequestArgumentSpec struct {
412414
var pullRequestArgumentSpecs = map[string]pullRequestArgumentSpec{
413415
"read": {
414416
required: []string{"method", "owner", "repo"}, requiredInts: []string{"pullNumber"},
415-
ints: []string{"page", "perPage"}, strings: []string{"after"},
417+
ints: []string{"page", "perPage"},
416418
methods: []string{"get", "get_diff", "get_status", "get_files", "get_commits", "get_review_comments", "get_reviews", "get_comments", "get_check_runs"},
417419
},
418420
"create": {
419421
required: []string{"owner", "repo"}, deferred: []string{"title", "head", "base"},
420-
strings: []string{"title", "head", "base", "body"}, bools: []string{"draft", "maintainer_can_modify", "_ui_submitted"},
422+
strings: []string{"title", "head", "base", "body"}, bools: []string{"draft", "maintainer_can_modify"},
421423
stringArrays: []string{"reviewers"},
422424
},
423425
"update": {
424426
required: []string{"owner", "repo"}, requiredInts: []string{"pullNumber"},
425-
strings: []string{"title", "body", "state", "base"}, bools: []string{"draft", "maintainer_can_modify", "_ui_submitted"},
427+
strings: []string{"title", "body", "state", "base"}, bools: []string{"draft", "maintainer_can_modify"},
426428
stringArrays: []string{"reviewers"},
427429
},
428430
"merge": {
@@ -483,35 +485,83 @@ func normalizePullRequestArguments(kind string) func(json.RawMessage) (json.RawM
483485
return nil, fmt.Errorf("commentId must be greater than 0")
484486
}
485487
args["commentId"] = commentID
486-
// pullNumber is only read, and validated, when a reply body is sent.
487-
if value, exists := args["pullNumber"]; exists {
488-
pullNumber, err := toInt(value)
489-
switch {
490-
case err == nil:
491-
args["pullNumber"] = pullNumber
492-
case args["body"] != nil:
493-
return nil, fmt.Errorf("parameter pullNumber is not a valid number: %w", err)
494-
default:
495-
delete(args, "pullNumber")
496-
}
497-
}
498488
}
499489
for _, field := range spec.deferred {
500490
if _, exists := args[field]; !exists {
501491
args[field] = ""
502492
}
503493
}
504-
for _, field := range spec.strings {
494+
495+
stringFields := spec.strings
496+
boolFields := spec.bools
497+
stringArrayFields := spec.stringArrays
498+
if kind == "create" {
499+
stringFields = []string{"title", "head", "base"}
500+
boolFields = []string{"draft", "maintainer_can_modify"}
501+
stringArrayFields = []string{"reviewers"}
502+
}
503+
if kind == "update" {
504+
boolFields = []string{"maintainer_can_modify"}
505+
}
506+
if kind == "reply" {
507+
stringFields = []string{"body", "reaction"}
508+
}
509+
if kind == "update" {
510+
// The update handler checks draft before title/body/state/base.
511+
if _, err := OptionalParam[bool](args, "draft"); err != nil {
512+
return nil, err
513+
}
514+
}
515+
if kind == "create" || kind == "update" {
516+
// This flag is inspected from the raw call only to control form
517+
// deferral; legacy handlers never type-validated it.
518+
if _, err := OptionalParam[bool](args, "_ui_submitted"); err != nil {
519+
delete(args, "_ui_submitted")
520+
}
521+
}
522+
for _, field := range stringFields {
505523
if _, err := OptionalParam[string](args, field); err != nil {
506524
return nil, err
507525
}
508526
}
509-
for _, field := range spec.bools {
527+
if kind == "create" {
528+
missingRequired := ""
529+
for _, field := range []string{"title", "head", "base"} {
530+
if args[field] == "" && missingRequired == "" {
531+
missingRequired = field
532+
}
533+
}
534+
if missingRequired != "" {
535+
// The handler may defer to the form before checking these later
536+
// fields. Drop malformed values so strict typed decoding cannot
537+
// change the earlier missing-field result.
538+
if _, err := OptionalParam[string](args, "body"); err != nil {
539+
delete(args, "body")
540+
}
541+
for _, field := range []string{"draft", "maintainer_can_modify", "_ui_submitted"} {
542+
if _, err := OptionalParam[bool](args, field); err != nil {
543+
delete(args, field)
544+
}
545+
}
546+
if _, exists := args["reviewers"]; exists {
547+
if _, err := OptionalStringArrayParam(args, "reviewers"); err != nil {
548+
delete(args, "reviewers")
549+
}
550+
}
551+
boolFields = nil
552+
stringArrayFields = nil
553+
} else {
554+
if _, err := OptionalParam[string](args, "body"); err != nil {
555+
return nil, err
556+
}
557+
}
558+
}
559+
for _, field := range boolFields {
510560
if _, err := OptionalParam[bool](args, field); err != nil {
511561
return nil, err
512562
}
513563
}
514-
for _, field := range spec.stringArrays {
564+
for _, field := range stringArrayFields {
515565
if _, exists := args[field]; !exists {
516566
continue
517567
}
@@ -534,6 +584,40 @@ func normalizePullRequestArguments(kind string) func(json.RawMessage) (json.RawM
534584
if spec.methods != nil && !slices.Contains(spec.methods, args["method"].(string)) {
535585
return nil, fmt.Errorf("unknown method: %s", args["method"])
536586
}
587+
if kind == "read" {
588+
if args["method"] == "get_review_comments" {
589+
if _, err := OptionalParam[string](args, "after"); err != nil {
590+
return nil, err
591+
}
592+
} else {
593+
delete(args, "after")
594+
}
595+
}
596+
if kind == "reply" {
597+
_, hasBody := args["body"]
598+
_, hasReaction := args["reaction"]
599+
if !hasBody && !hasReaction {
600+
return nil, fmt.Errorf("at least one of body or reaction is required")
601+
}
602+
if hasBody && args["body"] == "" {
603+
return nil, fmt.Errorf("body cannot be empty when provided")
604+
}
605+
if hasReaction && args["reaction"] == "" {
606+
return nil, fmt.Errorf("reaction cannot be empty when provided")
607+
}
608+
// pullNumber is only read, and validated, when a reply body is sent.
609+
if value, exists := args["pullNumber"]; exists {
610+
pullNumber, err := toInt(value)
611+
switch {
612+
case err == nil && hasBody:
613+
args["pullNumber"] = pullNumber
614+
case err != nil && hasBody:
615+
return nil, fmt.Errorf("parameter pullNumber is not a valid number: %w", err)
616+
default:
617+
delete(args, "pullNumber")
618+
}
619+
}
620+
}
537621
for _, field := range spec.ignoredStrings {
538622
if _, isString := args[field].(string); !isString {
539623
delete(args, field)

‎pkg/github/typed_consolidated_pull_request_outputs_test.go‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -278,6 +278,7 @@ func TestTypedConsolidatedPullRequestOutputs(t *testing.T) {
278278
cases := []typedPullRequestCase{
279279
{"pull_request_read", map[string]any{"method": "get", "pullNumber": "1"}, minimalPR},
280280
{"pull_request_read", map[string]any{"method": "get", "pullNumber": 3}, `null`},
281+
{"pull_request_read", map[string]any{"method": "get", "pullNumber": 1, "after": 1}, minimalPR},
281282
{"pull_request_read", map[string]any{"method": "get_diff", "pullNumber": 1.0}, typedPRDiff},
282283
{"pull_request_read", map[string]any{"method": "get_status", "pullNumber": 1}, `{"state":"success","sha":"abc","total_count":1,"statuses":[{"state":"success","context":"ci","target_url":"https://ci"}]}`},
283284
{"pull_request_read", map[string]any{"method": "get_files", "pullNumber": 1, "page": "1", "perPage": 5}, `[{"filename":"file","status":"modified","additions":1,"changes":1,"patch":"+added"}]`},
@@ -408,7 +409,9 @@ func TestTypedConsolidatedPullRequestErrors(t *testing.T) {
408409
{"pull_request_read", map[string]any{"method": "unknown", "pullNumber": 1}, "unknown method: unknown"},
409410
{"pull_request_read", map[string]any{"method": "get", "pullNumber": 1.5}, "parameter pullNumber is not a valid number: non-integer numeric value: 1.5"},
410411
{"create_pull_request", map[string]any{"title": "Subject", "head": "feature", "base": "main", "draft": "yes"}, "parameter draft is not of type bool, is string"},
412+
{"create_pull_request", map[string]any{"head": "feature", "base": "main", "draft": "yes"}, "missing required parameter: title"},
411413
{"update_pull_request", map[string]any{"pullNumber": 0, "title": "Subject"}, "missing required parameter: pullNumber"},
414+
{"update_pull_request", map[string]any{"pullNumber": 1, "draft": "yes", "title": 1}, "parameter draft is not of type bool, is string"},
412415
{"pull_request_review_write", map[string]any{"method": "nope", "pullNumber": 1}, "unknown method: nope"},
413416
{"add_comment_to_pending_review", map[string]any{"pullNumber": 1, "body": "nit", "subjectType": "LINE"}, "missing required parameter: path"},
414417
{"add_reply_to_pull_request_comment", map[string]any{"commentId": 0, "body": "reply"}, "missing required parameter: commentId"},
@@ -424,6 +427,8 @@ func TestTypedConsolidatedPullRequestErrors(t *testing.T) {
424427
{"add_reply_to_pull_request_comment", map[string]any{"commentId": 42, "body": "reply"}, "missing required parameter: pullNumber"},
425428
{"add_reply_to_pull_request_comment", map[string]any{"commentId": "1e30", "body": "reply"}, "parameter commentId is not a valid number: numeric value 1e+30 is too large to fit in int64"},
426429
{"add_reply_to_pull_request_comment", map[string]any{"commentId": 42, "pullNumber": "x", "body": "reply"}, "parameter pullNumber is not a valid number: invalid numeric value: x"},
430+
{"add_reply_to_pull_request_comment", map[string]any{"commentId": 42, "pullNumber": "x", "body": ""}, "body cannot be empty when provided"},
431+
{"add_reply_to_pull_request_comment", map[string]any{"commentId": 42, "pullNumber": "x", "body": "reply", "reaction": 1}, "parameter reaction is not of type string, is float64"},
427432
}
428433
for _, protocol := range typedPullRequestProtocols {
429434
t.Run("protocol="+protocol, func(t *testing.T) {
@@ -592,6 +597,16 @@ func TestConsolidatedPullRequestOutputSchemas(t *testing.T) {
592597
}
593598
}
594599

600+
func TestPullRequestOutputDescriptionsAreShapeAware(t *testing.T) {
601+
commitSchema := pullRequestOutputSchema[MinimalPullRequestCommit]()
602+
require.Contains(t, commitSchema.Properties, "message")
603+
assert.Empty(t, commitSchema.Properties["message"].Description)
604+
605+
branchUpdateSchema := pullRequestBranchUpdateOutputSchema()
606+
require.Contains(t, branchUpdateSchema.Properties, "message")
607+
assert.Equal(t, "Operation result message.", branchUpdateSchema.Properties["message"].Description)
608+
}
609+
595610
func TestConsolidatedPullRequestOutputTypesAreCurated(t *testing.T) {
596611
visited := make(map[reflect.Type]bool)
597612
var check func(reflect.Type)

0 commit comments

Comments
 (0)