list_label silently turns perPage=0 into 100 although the schema advertises minimum: 1 #3647

Describe the bug

list_label advertises perPage with minimum: 1 and maximum: 100, but a request that sends perPage: 0 is accepted and silently replaced by the server default of 100. The value is never validated against the range the tool publishes, so an out-of-domain argument produces a successful response with a page size the caller did not ask for.

Affected version

2.x, commit e3809fe91c7546633241006bf0b91f23b2595e28 (Add cursor pagination to list_label (#3505)), tested on windows/amd64 with Go 1.27.3.

Steps to reproduce the behavior

1. Add the test below to pkg/github.
2. Run go test ./pkg/github -run TestListLabelPerPageZero -v.
3. The test passes — that is the bug: 0 is out of the advertised domain, and it is neither rejected nor reported.

package github

import "testing"

func TestListLabelPerPageZero(t *testing.T) {
	// This is the call list_label makes at labels.go:180.
	got, err := OptionalIntParamWithDefault(
		map[string]any{"perPage": float64(0)},
		"perPage",
		listLabelsDefaultPerPage,
	)
	if err != nil {
		t.Fatalf("perPage=0 was rejected with %v", err)
	}
	if got != listLabelsDefaultPerPage {
		t.Fatalf("perPage=0 resolved to %d", got)
	}
	t.Logf("perPage=0 was silently replaced by the default %d", got)
}

Expected vs actual behavior

Actual behavior: perPage=0 and "argument omitted" are indistinguishable to the handler, so an explicit 0 returns 100 labels instead of an error. The response carries no indication that the requested value was replaced. Related: perPage=101 also passes the parsing stage and is rejected later by CursorPaginationParams.ToGraphQLParams with a different error (perPage value 101 exceeds maximum of 100), so the two ends of the range travel through unrelated code paths.

Expected behavior: a perPage outside 1..100 should be rejected with an argument error at the point the parameter is read — the same way 101 already is — rather than being silently coerced.

Logs

args=map[perPage:0]   -> perPage=100 err=<nil>
args=map[perPage:1]   -> perPage=1   err=<nil>
args=map[perPage:100] -> perPage=100 err=<nil>
args=map[perPage:101] -> perPage=101 err=<nil>   (rejected later by ToGraphQLParams)
args=map[]            -> perPage=100 err=<nil>

Root cause

// pkg/github/params.go:236
func OptionalIntParamWithDefault(args map[string]any, p string, d int) (int, error) {
	v, err := OptionalIntParam(args, p)
	if err != nil {
		return 0, err
	}
	if v == 0 {          // zero value, not "key absent"
		return d, nil
	}
	return v, nil
}

The helper conflates "the caller sent 0" with "the caller sent nothing". Its boolean sibling two functions below, OptionalBoolParamWithDefault, checks presence instead. CursorPaginationParams.ToGraphQLParams already validates both ends:

if p.PerPage > 100 {
	return nil, fmt.Errorf("perPage value %d exceeds maximum of 100", p.PerPage)
}
if p.PerPage < 0 {
	return nil, fmt.Errorf("perPage value %d cannot be negative", p.PerPage)
}

0 satisfies neither branch and is consumed by the defaulting helper before the validator sees it.

The same PR's table test makes the gap explicit — pkg/github/labels_test.go:209-219 contains:

{
	name: "rejects a page size above 100",
	requestArgs: map[string]any{"owner": "owner", "repo": "repo", "perPage": float64(101)},
	expectToolError:    true,
	expectedToolErrMsg: "perPage value 101 exceeds maximum of 100",
},

so the upper bound was deliberately enforced and tested, while the lower bound has no case and never reaches ToGraphQLParams.

Suggested fix

Make the default depend on key presence and validate the range:

perPage := listLabelsDefaultPerPage
if _, present := args["perPage"]; present {
	v, err := OptionalIntParam(args, "perPage")
	if err != nil {
		return utils.NewToolResultError(err.Error()), nil, nil
	}
	if v < 1 || v > 100 {
		return utils.NewToolResultError(
			fmt.Sprintf("perPage must be between 1 and 100, got %d", v)), nil, nil
	}
	perPage = v
}

or, more generally, change OptionalIntParamWithDefault to check presence the way OptionalBoolParamWithDefault does and let ToGraphQLParams do the range check. The second option changes behaviour for every tool that uses the helper, so it probably wants its own issue; the first keeps the change scoped to list_label.