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.

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

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 examplefmt.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

Here is a real example:

func main() {var params ParamsMapparams = make(ParamsMap)params["a"] = "Hello, 世界"params["b"] = ""params["c"] = "sdfds"params["d"] = 999v, 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 requestif _, ok := params[p]; !ok {return empty, fmt.Errorf("missing required parameter: %s", p)}// Check if the parameter is of the expected typev, 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 valueif 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.

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!