439ec711fa
* Add fields param to six more list/search tools Extend the optional `fields` response-filtering parameter (gated behind the `fields_param` feature flag, with adoption/savings telemetry) to six more read tools, following the dual-variant pattern already used by search_code and get_file_contents: - list_issues, list_pull_requests, list_commits, list_releases - search_issues, search_pull_requests For each tool, `X` is the flag-enabled variant that advertises `fields` and filters each result item to the requested subset, while `LegacyX` exposes the original schema and never filters, acting as a kill switch when the flag is off. Exactly one variant survives inventory filtering for any flag state via mutually exclusive FeatureFlagEnable / FeatureFlagDisable annotations. Wrapped responses (list_issues, search_issues, search_pull_requests) preserve their count / pagination envelope and only filter the item list; bare-array responses keep their array shape. Filtering reuses the shared filterEachField helper. A new fieldsSchemaProperty helper builds the `fields` schema (search_code and get_file_contents now use it too), and a shared recordFieldsUsageFor helper centralizes the telemetry full-size computation. Each field enum lists only the JSON fields the specific tool actually emits: list_commits omits stats/files (requested without per-file detail) and list_issues lists only the fields its GraphQL fragment populates. Adds per-tool field-filtering, telemetry, and Legacy definition tests, extends the mutual-exclusivity gating test to all eight gated tools, and regenerates the `_ff_fields_param` toolsnaps and feature-flag docs. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * Remove duplicate tool doc comments Address review: drop the leftover one-line doc comment above each dual-variant tool constructor (list_issues, list_pull_requests, list_commits, list_releases, search_issues, search_pull_requests); the detailed variant comment remains. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
85 lines
2.9 KiB
Go
85 lines
2.9 KiB
Go
package github
|
|
|
|
import (
|
|
"context"
|
|
"testing"
|
|
|
|
"github.com/github/github-mcp-server/pkg/translations"
|
|
"github.com/google/jsonschema-go/jsonschema"
|
|
"github.com/stretchr/testify/assert"
|
|
"github.com/stretchr/testify/require"
|
|
)
|
|
|
|
// Test_FieldsParamVariants_MutuallyExclusive guards the dual-variant
|
|
// registration for the fields_param feature flag. The flag-enabled tools and
|
|
// their Legacy* counterparts share a tool name, so exactly one of each pair must
|
|
// survive inventory filtering for any flag state. If both ever leaked, a client
|
|
// could be offered two tools with the same name. This asserts that each gated
|
|
// tool is present exactly once, advertising the `fields` parameter only when
|
|
// fields_param is enabled.
|
|
func Test_FieldsParamVariants_MutuallyExclusive(t *testing.T) {
|
|
gatedTools := []string{
|
|
"search_code",
|
|
"get_file_contents",
|
|
"list_issues",
|
|
"list_releases",
|
|
"list_pull_requests",
|
|
"search_issues",
|
|
"search_pull_requests",
|
|
"list_commits",
|
|
}
|
|
|
|
for _, tc := range []struct {
|
|
name string
|
|
flagEnabled bool
|
|
expectFields bool
|
|
featureChecks func(context.Context, string) (bool, error)
|
|
}{
|
|
{
|
|
name: "flag off registers the legacy variant without fields",
|
|
flagEnabled: false,
|
|
expectFields: false,
|
|
featureChecks: featureCheckerFor(), // fields_param disabled
|
|
},
|
|
{
|
|
name: "flag on registers the fields variant with fields",
|
|
flagEnabled: true,
|
|
expectFields: true,
|
|
featureChecks: featureCheckerFor(FeatureFlagFieldsParam),
|
|
},
|
|
} {
|
|
t.Run(tc.name, func(t *testing.T) {
|
|
inv, err := NewInventory(translations.NullTranslationHelper).
|
|
WithToolsets([]string{"all"}).
|
|
WithFeatureChecker(tc.featureChecks).
|
|
Build()
|
|
require.NoError(t, err)
|
|
|
|
available := inv.AvailableTools(context.Background())
|
|
|
|
counts := make(map[string]int, len(available))
|
|
for _, tool := range available {
|
|
counts[tool.Tool.Name]++
|
|
}
|
|
|
|
// Each gated tool must be present exactly once (never both variants)
|
|
// and advertise `fields` only when the flag is enabled.
|
|
for _, name := range gatedTools {
|
|
require.Equalf(t, 1, counts[name], "expected exactly one %q for flagEnabled=%v; dual variants must be mutually exclusive", name, tc.flagEnabled)
|
|
|
|
tool := requireToolByName(t, available, name)
|
|
schema, ok := tool.Tool.InputSchema.(*jsonschema.Schema)
|
|
require.Truef(t, ok, "%q InputSchema should be *jsonschema.Schema", name)
|
|
|
|
if tc.expectFields {
|
|
assert.Containsf(t, schema.Properties, "fields", "%q should advertise fields when flag is on", name)
|
|
assert.Equalf(t, FeatureFlagFieldsParam, tool.FeatureFlagEnable, "%q should be the flag-enabled variant", name)
|
|
} else {
|
|
assert.NotContainsf(t, schema.Properties, "fields", "%q must not advertise fields when flag is off", name)
|
|
assert.Containsf(t, tool.FeatureFlagDisable, FeatureFlagFieldsParam, "%q should be the legacy (flag-disabled) variant", name)
|
|
}
|
|
}
|
|
})
|
|
}
|
|
}
|