From 5ac1eb47eb2b9d47c59c4e4386c8169161ec0227 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 12 Jan 2026 17:03:29 +0000 Subject: [PATCH 1/2] Initial plan From cd528d3968ba75c1457df6dd33f6a543d61c3e53 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 12 Jan 2026 17:08:43 +0000 Subject: [PATCH 2/2] Add scope validation feature to inventory Builder Co-authored-by: SamMorrowDrums <4811358+SamMorrowDrums@users.noreply.github.com> --- pkg/inventory/builder.go | 26 +++++ pkg/inventory/builder_test.go | 209 ++++++++++++++++++++++++++++++++++ 2 files changed, 235 insertions(+) create mode 100644 pkg/inventory/builder_test.go diff --git a/pkg/inventory/builder.go b/pkg/inventory/builder.go index 0400c2a24..3d5f83298 100644 --- a/pkg/inventory/builder.go +++ b/pkg/inventory/builder.go @@ -2,6 +2,7 @@ package inventory import ( "context" + "fmt" "sort" "strings" ) @@ -25,6 +26,7 @@ type ToolFilter func(ctx context.Context, tool *ServerTool) (bool, error) // WithToolsets([]string{"repos", "issues"}). // WithFeatureChecker(checker). // WithFilter(myFilter). +// WithRequireScopes(true). // Build() type Builder struct { tools []ServerTool @@ -39,6 +41,7 @@ type Builder struct { additionalTools []string // raw input, processed at Build() featureChecker FeatureFlagChecker filters []ToolFilter // filters to apply to all tools + requireScopes bool // when true, validates all tools have scope definitions } // NewBuilder creates a new Builder. @@ -127,11 +130,34 @@ func (b *Builder) WithFilter(filter ToolFilter) *Builder { return b } +// WithRequireScopes enables validation that all tools have scope definitions. +// When enabled, Build() will panic if any tool has both RequiredScopes and +// AcceptedScopes set to nil. Empty slices ([]string{}) are allowed, as they +// explicitly indicate the tool requires no scopes. +// Returns self for chaining. +func (b *Builder) WithRequireScopes(require bool) *Builder { + b.requireScopes = require + return b +} + // Build creates the final Inventory with all configuration applied. // This processes toolset filtering, tool name resolution, and sets up // the inventory for use. The returned Inventory is ready for use with // AvailableTools(), RegisterAll(), etc. +// +// If WithRequireScopes(true) was called, this method will panic if any +// tool has both RequiredScopes and AcceptedScopes set to nil. func (b *Builder) Build() *Inventory { + // Validate scopes if required + if b.requireScopes { + for i := range b.tools { + tool := &b.tools[i] + if tool.RequiredScopes == nil && tool.AcceptedScopes == nil { + panic(fmt.Sprintf("tool %q missing scope definitions (both RequiredScopes and AcceptedScopes are nil)", tool.Tool.Name)) + } + } + } + r := &Inventory{ tools: b.tools, resourceTemplates: b.resourceTemplates, diff --git a/pkg/inventory/builder_test.go b/pkg/inventory/builder_test.go new file mode 100644 index 000000000..2ed8ca85a --- /dev/null +++ b/pkg/inventory/builder_test.go @@ -0,0 +1,209 @@ +package inventory + +import ( + "testing" +) + +// mockToolWithScopes creates a mock tool with specified scope definitions +func mockToolWithScopes(name string, toolsetID string, requiredScopes, acceptedScopes []string) ServerTool { + tool := mockTool(name, toolsetID, false) + tool.RequiredScopes = requiredScopes + tool.AcceptedScopes = acceptedScopes + return tool +} + +func TestWithRequireScopes_AllToolsHaveScopes(t *testing.T) { + tools := []ServerTool{ + mockToolWithScopes("tool1", "toolset1", []string{"repo"}, []string{"repo", "admin:org"}), + mockToolWithScopes("tool2", "toolset1", []string{"repo", "user"}, []string{"repo", "user", "admin:org"}), + mockToolWithScopes("tool3", "toolset2", []string{}, []string{}), // empty slices are valid + } + + // Should not panic when all tools have scope definitions + defer func() { + if r := recover(); r != nil { + t.Fatalf("Build() panicked unexpectedly: %v", r) + } + }() + + _ = NewBuilder(). + SetTools(tools). + WithRequireScopes(true). + Build() +} + +func TestWithRequireScopes_ToolMissingBothScopes(t *testing.T) { + tools := []ServerTool{ + mockToolWithScopes("tool1", "toolset1", []string{"repo"}, []string{"repo"}), + mockTool("tool2", "toolset1", false), // This tool has nil RequiredScopes and AcceptedScopes + } + + // Should panic when a tool is missing both scope definitions + defer func() { + r := recover() + if r == nil { + t.Fatal("Build() should have panicked for tool missing scope definitions") + } + errMsg, ok := r.(string) + if !ok { + t.Fatalf("Expected panic message to be a string, got %T", r) + } + expectedMsg := `tool "tool2" missing scope definitions (both RequiredScopes and AcceptedScopes are nil)` + if errMsg != expectedMsg { + t.Errorf("Expected panic message %q, got %q", expectedMsg, errMsg) + } + }() + + _ = NewBuilder(). + SetTools(tools). + WithRequireScopes(true). + Build() +} + +func TestWithRequireScopes_OnlyRequiredScopesSet(t *testing.T) { + tool := mockTool("tool1", "toolset1", false) + tool.RequiredScopes = []string{"repo"} + tool.AcceptedScopes = nil + + tools := []ServerTool{tool} + + // Should not panic when only RequiredScopes is set (AcceptedScopes can be nil if RequiredScopes is set) + defer func() { + if r := recover(); r != nil { + t.Fatalf("Build() panicked unexpectedly: %v", r) + } + }() + + _ = NewBuilder(). + SetTools(tools). + WithRequireScopes(true). + Build() +} + +func TestWithRequireScopes_OnlyAcceptedScopesSet(t *testing.T) { + tool := mockTool("tool1", "toolset1", false) + tool.RequiredScopes = nil + tool.AcceptedScopes = []string{"repo"} + + tools := []ServerTool{tool} + + // Should not panic when only AcceptedScopes is set (RequiredScopes can be nil if AcceptedScopes is set) + defer func() { + if r := recover(); r != nil { + t.Fatalf("Build() panicked unexpectedly: %v", r) + } + }() + + _ = NewBuilder(). + SetTools(tools). + WithRequireScopes(true). + Build() +} + +func TestWithRequireScopes_EmptySlicesAllowed(t *testing.T) { + tools := []ServerTool{ + mockToolWithScopes("tool1", "toolset1", []string{}, []string{}), + mockToolWithScopes("tool2", "toolset2", []string{}, []string{}), + } + + // Should not panic when tools have empty slices (explicit "no scopes needed") + defer func() { + if r := recover(); r != nil { + t.Fatalf("Build() panicked unexpectedly: %v", r) + } + }() + + _ = NewBuilder(). + SetTools(tools). + WithRequireScopes(true). + Build() +} + +func TestWithRequireScopes_False(t *testing.T) { + tools := []ServerTool{ + mockTool("tool1", "toolset1", false), // Missing scope definitions + mockTool("tool2", "toolset1", false), // Missing scope definitions + } + + // Should not panic when WithRequireScopes(false) or not set + defer func() { + if r := recover(); r != nil { + t.Fatalf("Build() panicked unexpectedly: %v", r) + } + }() + + _ = NewBuilder(). + SetTools(tools). + WithRequireScopes(false). + Build() +} + +func TestWithRequireScopes_NotSet(t *testing.T) { + tools := []ServerTool{ + mockTool("tool1", "toolset1", false), // Missing scope definitions + mockTool("tool2", "toolset1", false), // Missing scope definitions + } + + // Should not panic when WithRequireScopes is not called (default is false) + defer func() { + if r := recover(); r != nil { + t.Fatalf("Build() panicked unexpectedly: %v", r) + } + }() + + _ = NewBuilder(). + SetTools(tools). + Build() +} + +func TestWithRequireScopes_MixedTools(t *testing.T) { + tools := []ServerTool{ + mockToolWithScopes("tool1", "toolset1", []string{"repo"}, []string{"repo"}), + mockToolWithScopes("tool2", "toolset1", []string{}, []string{}), + mockTool("tool3", "toolset2", false), // Missing scope definitions + } + + // Should panic on the first tool with missing scope definitions + defer func() { + r := recover() + if r == nil { + t.Fatal("Build() should have panicked for tool missing scope definitions") + } + errMsg, ok := r.(string) + if !ok { + t.Fatalf("Expected panic message to be a string, got %T", r) + } + expectedMsg := `tool "tool3" missing scope definitions (both RequiredScopes and AcceptedScopes are nil)` + if errMsg != expectedMsg { + t.Errorf("Expected panic message %q, got %q", expectedMsg, errMsg) + } + }() + + _ = NewBuilder(). + SetTools(tools). + WithRequireScopes(true). + Build() +} + +func TestWithRequireScopes_Chaining(t *testing.T) { + tools := []ServerTool{ + mockToolWithScopes("tool1", "toolset1", []string{"repo"}, []string{"repo"}), + } + + // Test that WithRequireScopes returns the builder for chaining + builder := NewBuilder() + result := builder.WithRequireScopes(true) + + if result != builder { + t.Error("WithRequireScopes should return the same builder instance for chaining") + } + + // Verify the build works + defer func() { + if r := recover(); r != nil { + t.Fatalf("Build() panicked unexpectedly: %v", r) + } + }() + + _ = result.SetTools(tools).Build() +}