validate tools params

[juruen]

Context

This PR changes the way request parameters are used to make sure that they are always validated before they are used. We were previously relying on the parameters honoring the expected types, but now we are explicitly checking and returning an error if the parameter types are not as expected.

This way, even if there's a schema mismatch between the the schema itself and the implementation, the server won't crash trying to cast invalid types with something like request.Params.Arguments["owner"].(string).

For the implementation itself, I've added a few convenience functions that are used across the different tools.

Some tests have been modified to reflect the new behavior for error returning.

Apologies for the largish PR, but as we are doing the same: validating and fetching the tools parameters, it felt like we could do it in a single PR. As always, I'm happy to split it out.

Thanks! ❤️

[Copilot]

Pull Request Overview

This PR introduces explicit validation for tool request parameters to ensure type safety and prevent runtime errors by returning clear error messages when parameters are missing or of the wrong type. Key changes include:

• Enhancements to parameter validation functions (e.g. requiredStringParam, requiredNumberParam, optionalStringParam, etc.) with explicit error handling.
• Updates to test cases and tool implementations across various files (server, issues, repositories, pullrequests, search, and code_scanning) to use the new validation functions.
• Refinements in error messaging for parameter type mismatches.

Reviewed Changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
pkg/github/server.go	Updated parameter validation functions with explicit checks and error messaging.
pkg/github/issues_test.go	Modified tests to reflect new parameter validation behaviors and error messages.
pkg/github/repositories.go	Switched to using the new parameter validation helpers for improved clarity.
pkg/github/issues.go	Updated the use of parameter validation functions for issue-related requests.
pkg/github/pullrequests.go	Migrated to validated parameter extraction in pull-request functions.
pkg/github/search.go	Adopted the new helpers, ensuring consistent parsing and error handling in search.
pkg/github/code_scanning.go	Replaced direct type assertions with validation functions for consistency.

Tip: Copilot code review supports C#, Go, Java, JavaScript, Markdown, Python, Ruby and TypeScript, with more languages coming soon. Learn more

[SamMorrowDrums]

Could we use generics to make this shorter?

import (
	"fmt"
)

func main() {
	var v interface{}
	v = "Hello, 世界"
	validateParam[string](v)
	validateParam[int](v)
}

func validateParam[T any](v interface{}) (T, error) {
	out, ok := v.(T)
	if !ok {
		var x T
		// for example
		fmt.Println(fmt.Errorf("parameter %s is not of type %T", "param name", out))
		return x, fmt.Errorf("parameter %s is not of type %T", "param name", out)
	}
	return out, nil
}

The above prints:

parameter param name is not of type int

[SamMorrowDrums]

Here is a real example:

func main() {
	var params ParamsMap
	params = make(ParamsMap)
	params["a"] = "Hello, 世界"
	params["b"] = ""
	params["c"] = "sdfds"
	params["d"] = 999
	v, e := requiredParam[string](params, "a")
	fmt.Printf("%s\n", v)
	_, e = requiredParam[string](params, "b")
	fmt.Printf("%e\n", e)
	_, e = requiredParam[int](params, "c")
	fmt.Printf("%e\n", e)
	i, _ := requiredParam[int](params, "d")
	fmt.Printf("Lucky number: %d\n", i)
}

func requiredParam[T comparable](params ParamsMap, p string) (T, error) {
	var empty T

	// Check if the parameter is present in the request
	if _, ok := params[p]; !ok {
		return empty, fmt.Errorf("missing required parameter: %s", p)
	}

	// Check if the parameter is of the expected type
	v, ok := params[p].(T)
	if !ok {
		return v, fmt.Errorf("parameter %s is not of type string", p)
	}

	// Check if the parameter is not the zero value

	if reflect.TypeOf(v).String() == "string" && v == empty {
		return v, fmt.Errorf("missing required parameter: %s", p)
	}

	return v, nil
}

With output:

Hello, 世界
&{%!e(string=missing required parameter: b)}
&{%!e(string=parameter c is not of type string)}
Lucky number: 999

That said, I am a little unsure about rejecting an empty string, there are times where it could be valid input in fact. For example the Code Scanning API does allow for a category of "". So we might not need that restriction.

[juruen]

Could we use generics to make this shorter?

That was my initial attempt :) Unfortunately, we get floats for numbers, and we require ints in the GH API, so using generics didn't work well there. Or it only worked well for strings and booleans. You could still make it work, but it became a bit more cumbersome, and at that point I decided not to go with generics.

[juruen]

I gave @SamMorrowDrums suggestion a go in 3c6467f

we use generics for types that are comparable, in our case, this is mostly used by strings and booleans, and separate functions for ints to take care of the float to int cast.

[SamMorrowDrums]

This is awesome, and I think I like the generic version, although I apologise for the extra work. I think your motivations for not doing it made sense too, and it's a matter of taste for which either could be preferred.

Definitely excited to get into some black-box testing to ensure that kind of change does indeed not break any existing functionality (although the test coverage is great).

Legend for doing this!