fix(pull_request_read): reject pagination parameters the selected method does not support

Author: larik15

pkg/github/pullrequests.go

@@ -117,6 +117,17 @@ Possible options:
 				return attachRepoVisibilityIFCLabel(ctx, deps, client, owner, repo, r, ifc.LabelRepoUserContent)
 			}
 
+			// Each method honours exactly one pagination style: get_review_comments
+			// takes a GraphQL cursor (`after`), the other list methods take
+			// `page`/`perPage`. A parameter the selected method cannot use is an
+			// error rather than a silent no-op, so an LLM caller that passes
+			// `after` to get_files (or `page` to get_review_comments) learns that
+			// it did not advance the page instead of receiving the first page
+			// again as if it had.
+			if err := rejectUnsupportedPagination(method, args); err != nil {
+				return utils.NewToolResultError(err.Error()), nil, nil
+			}
+
 			switch method {
 			case "get":
 				result, err := GetPullRequest(ctx, client, deps, owner, repo, pullNumber)
@@ -2502,3 +2513,32 @@ func newGQLIntPtr(i *int32) *githubv4.Int {
 	gi := githubv4.Int(*i)
 	return &gi
 }
+
+// rejectUnsupportedPagination returns an error when the caller supplies a
+// pagination parameter the selected pull_request_read method does not honour.
+// get_review_comments paginates by GraphQL cursor (`after`); the other list
+// methods paginate by `page`/`perPage`; `get`, `get_diff` and `get_status`
+// return a single object and paginate by nothing. Without this check the
+// unsupported parameter is dropped silently and the caller receives the first
+// page again, indistinguishable from a successful advance.
+func rejectUnsupportedPagination(method string, args map[string]any) error {
+	_, hasAfter := args["after"]
+	_, hasPage := args["page"]
+	_, hasPerPage := args["perPage"]
+
+	switch method {
+	case "get_review_comments":
+		if hasPage {
+			return fmt.Errorf("method %q paginates by cursor (perPage, after); \"page\" is not supported", method)
+		}
+	case "get_files", "get_commits", "get_reviews", "get_comments", "get_check_runs":
+		if hasAfter {
+			return fmt.Errorf("method %q paginates by page/perPage; \"after\" is not supported", method)
+		}
+	case "get", "get_diff", "get_status":
+		if hasAfter || hasPage || hasPerPage {
+			return fmt.Errorf("method %q returns a single result and does not accept pagination parameters", method)
+		}
+	}
+	return nil
+}

pkg/github/pullrequests_test.go

@@ -4877,3 +4877,57 @@ func TestResolveReviewThread(t *testing.T) {
 		})
 	}
 }
+
+func Test_rejectUnsupportedPagination(t *testing.T) {
+	tests := []struct {
+		name    string
+		method  string
+		args    map[string]any
+		wantErr string
+	}{
+		{
+			name:   "get_files with page is fine",
+			method: "get_files",
+			args:   map[string]any{"page": float64(2), "perPage": float64(10)},
+		},
+		{
+			name:    "get_files with after is rejected",
+			method:  "get_files",
+			args:    map[string]any{"after": "Y3Vyc29y"},
+			wantErr: `method "get_files" paginates by page/perPage; "after" is not supported`,
+		},
+		{
+			name:   "get_review_comments with after is fine",
+			method: "get_review_comments",
+			args:   map[string]any{"after": "Y3Vyc29y", "perPage": float64(10)},
+		},
+		{
+			name:    "get_review_comments with page is rejected",
+			method:  "get_review_comments",
+			args:    map[string]any{"page": float64(2)},
+			wantErr: `method "get_review_comments" paginates by cursor (perPage, after); "page" is not supported`,
+		},
+		{
+			name:    "get with any pagination is rejected",
+			method:  "get",
+			args:    map[string]any{"perPage": float64(10)},
+			wantErr: `method "get" returns a single result and does not accept pagination parameters`,
+		},
+		{
+			name:   "get without pagination is fine",
+			method: "get",
+			args:   map[string]any{},
+		},
+	}
+
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			err := rejectUnsupportedPagination(tc.method, tc.args)
+			if tc.wantErr == "" {
+				require.NoError(t, err)
+				return
+			}
+			require.EqualError(t, err, tc.wantErr)
+		})
+	}
+}