diff --git a/.changeset/patch-github-tool-validator.md b/.changeset/patch-github-tool-validator.md new file mode 100644 index 00000000000..5d523621ba3 --- /dev/null +++ b/.changeset/patch-github-tool-validator.md @@ -0,0 +1,5 @@ +--- +"gh-aw": patch +--- + +Add GitHub tool/toolset validator for allowed tools configuration diff --git a/.github/workflows/technical-doc-writer.lock.yml b/.github/workflows/technical-doc-writer.lock.yml index d57b6453f0e..6f6dcedaf16 100644 --- a/.github/workflows/technical-doc-writer.lock.yml +++ b/.github/workflows/technical-doc-writer.lock.yml @@ -493,7 +493,7 @@ jobs: with: persist-credentials: false - name: Setup Node.js - uses: actions/setup-node@49933ea5288caeca8642d1e84afbd3f7d6820020 + uses: actions/setup-node@a0853c24544627f65ddf259abe73b1d18a591444 with: cache: npm cache-dependency-path: docs/package-lock.json diff --git a/pkg/workflow/compiler.go b/pkg/workflow/compiler.go index b3c683180eb..89dad8fc81b 100644 --- a/pkg/workflow/compiler.go +++ b/pkg/workflow/compiler.go @@ -281,14 +281,14 @@ func (c *Compiler) CompileWorkflowData(workflowData *WorkflowData, markdownPath // Parse permissions from the workflow data // WorkflowData.Permissions contains the raw YAML string (including "permissions:" prefix) permissions := NewPermissionsParser(workflowData.Permissions).ToPermissions() - + // Validate permissions validationResult := ValidatePermissions(permissions, githubTool) - + if validationResult.HasValidationIssues { // Format the validation message message := FormatValidationMessage(validationResult, c.strictMode) - + if len(validationResult.MissingPermissions) > 0 { // Missing permissions are always errors formattedErr := console.FormatError(console.CompilerError{ @@ -302,7 +302,7 @@ func (c *Compiler) CompileWorkflowData(workflowData *WorkflowData, markdownPath }) return errors.New(formattedErr) } - + if len(validationResult.ExcessPermissions) > 0 { // Excess permissions are warnings by default, errors in strict mode if c.strictMode { @@ -333,6 +333,28 @@ func (c *Compiler) CompileWorkflowData(workflowData *WorkflowData, markdownPath } } + // Validate GitHub tools against enabled toolsets + log.Printf("Validating GitHub tools against enabled toolsets") + if workflowData.ParsedTools != nil && workflowData.ParsedTools.GitHub != nil { + // Extract allowed tools and enabled toolsets from ParsedTools + allowedTools := workflowData.ParsedTools.GitHub.Allowed + enabledToolsets := ParseGitHubToolsets(strings.Join(workflowData.ParsedTools.GitHub.Toolset, ",")) + + // Validate that all allowed tools have their toolsets enabled + if err := ValidateGitHubToolsAgainstToolsets(allowedTools, enabledToolsets); err != nil { + formattedErr := console.FormatError(console.CompilerError{ + Position: console.ErrorPosition{ + File: markdownPath, + Line: 1, + Column: 1, + }, + Type: "error", + Message: err.Error(), + }) + return errors.New(formattedErr) + } + } + // Note: Markdown content size is now handled by splitting into multiple steps in generatePrompt log.Printf("Workflow: %s, Tools: %d", workflowData.Name, len(workflowData.Tools)) diff --git a/pkg/workflow/data/github_tool_to_toolset.json b/pkg/workflow/data/github_tool_to_toolset.json new file mode 100644 index 00000000000..a12d60cae56 --- /dev/null +++ b/pkg/workflow/data/github_tool_to_toolset.json @@ -0,0 +1,69 @@ +{ + "get_me": "context", + "get_teams": "context", + "get_team_members": "context", + + "get_repository": "repos", + "get_file_contents": "repos", + "search_code": "repos", + "list_commits": "repos", + "get_commit": "repos", + "get_latest_release": "repos", + "list_releases": "repos", + "get_release_by_tag": "repos", + "get_tag": "repos", + "list_tags": "repos", + "list_branches": "repos", + + "issue_read": "issues", + "list_issues": "issues", + "create_issue": "issues", + "update_issue": "issues", + "search_issues": "issues", + "add_reaction": "issues", + "create_issue_comment": "issues", + + "pull_request_read": "pull_requests", + "list_pull_requests": "pull_requests", + "get_pull_request": "pull_requests", + "create_pull_request": "pull_requests", + "search_pull_requests": "pull_requests", + + "list_workflows": "actions", + "list_workflow_runs": "actions", + "get_workflow_run": "actions", + "download_workflow_run_artifact": "actions", + "get_workflow_run_usage": "actions", + "list_workflow_jobs": "actions", + "get_job_logs": "actions", + "list_workflow_run_artifacts": "actions", + + "list_code_scanning_alerts": "code_security", + "get_code_scanning_alert": "code_security", + "create_code_scanning_alert": "code_security", + + "list_discussions": "discussions", + "create_discussion": "discussions", + + "create_gist": "gists", + "list_gists": "gists", + + "get_label": "labels", + "list_labels": "labels", + "create_label": "labels", + + "list_notifications": "notifications", + "mark_notifications_read": "notifications", + + "get_organization": "orgs", + "list_organizations": "orgs", + + "list_secret_scanning_alerts": "secret_protection", + "get_secret_scanning_alert": "secret_protection", + + "get_user": "users", + "list_users": "users", + + "search_repositories": "search", + "search_users": "search" +} diff --git a/pkg/workflow/github_tool_to_toolset.go b/pkg/workflow/github_tool_to_toolset.go new file mode 100644 index 00000000000..6664749b05f --- /dev/null +++ b/pkg/workflow/github_tool_to_toolset.go @@ -0,0 +1,58 @@ +package workflow + +import ( + _ "embed" + "encoding/json" +) + +//go:embed data/github_tool_to_toolset.json +var githubToolToToolsetJSON []byte + +// GitHubToolToToolsetMap maps individual GitHub MCP tools to their respective toolsets +// This mapping is loaded from an embedded JSON file based on the documentation +// in .github/instructions/github-mcp-server.instructions.md +var GitHubToolToToolsetMap map[string]string + +func init() { + // Load the mapping from embedded JSON + if err := json.Unmarshal(githubToolToToolsetJSON, &GitHubToolToToolsetMap); err != nil { + panic("failed to load GitHub tool to toolset mapping: " + err.Error()) + } +} + +// ValidateGitHubToolsAgainstToolsets validates that all allowed GitHub tools have their +// corresponding toolsets enabled in the configuration +func ValidateGitHubToolsAgainstToolsets(allowedTools []string, enabledToolsets []string) error { + if len(allowedTools) == 0 { + // No specific tools restricted, validation not needed + return nil + } + + // Create a set of enabled toolsets for fast lookup + enabledSet := make(map[string]bool) + for _, toolset := range enabledToolsets { + enabledSet[toolset] = true + } + + // Track missing toolsets and which tools need them + missingToolsets := make(map[string][]string) // toolset -> list of tools that need it + + for _, tool := range allowedTools { + requiredToolset, exists := GitHubToolToToolsetMap[tool] + if !exists { + // Tool not in our mapping - this could be a new tool or a typo + // We'll skip validation for unknown tools to avoid false positives + continue + } + + if !enabledSet[requiredToolset] { + missingToolsets[requiredToolset] = append(missingToolsets[requiredToolset], tool) + } + } + + if len(missingToolsets) > 0 { + return NewGitHubToolsetValidationError(missingToolsets) + } + + return nil +} diff --git a/pkg/workflow/github_tool_to_toolset_test.go b/pkg/workflow/github_tool_to_toolset_test.go new file mode 100644 index 00000000000..100b8006e6d --- /dev/null +++ b/pkg/workflow/github_tool_to_toolset_test.go @@ -0,0 +1,307 @@ +package workflow + +import ( + "strings" + "testing" +) + +func TestValidateGitHubToolsAgainstToolsets(t *testing.T) { + tests := []struct { + name string + allowedTools []string + enabledToolsets []string + expectError bool + errorContains []string + }{ + { + name: "No allowed tools - validation passes", + allowedTools: []string{}, + enabledToolsets: []string{"repos"}, + expectError: false, + }, + { + name: "Nil allowed tools - validation passes", + allowedTools: nil, + enabledToolsets: []string{"repos"}, + expectError: false, + }, + { + name: "All tools have corresponding toolsets enabled", + allowedTools: []string{"get_repository", "list_commits", "get_file_contents"}, + enabledToolsets: []string{"repos"}, + expectError: false, + }, + { + name: "Mix of toolsets all enabled", + allowedTools: []string{"get_repository", "list_issues", "pull_request_read"}, + enabledToolsets: []string{"repos", "issues", "pull_requests"}, + expectError: false, + }, + { + name: "Default toolset includes required toolset", + allowedTools: []string{"get_repository", "list_issues"}, + enabledToolsets: []string{"default"}, // Default expands to include repos and issues + expectError: false, + }, + { + name: "All toolset enables everything", + allowedTools: []string{"get_repository", "list_issues", "list_workflows", "create_gist"}, + enabledToolsets: []string{"all"}, + expectError: false, + }, + { + name: "Missing single toolset", + allowedTools: []string{"get_repository", "list_issues"}, + enabledToolsets: []string{"repos"}, // issues toolset missing + expectError: true, + errorContains: []string{"issues", "list_issues"}, + }, + { + name: "Missing multiple toolsets", + allowedTools: []string{"get_repository", "list_issues", "list_workflows"}, + enabledToolsets: []string{"repos"}, // issues and actions missing + expectError: true, + errorContains: []string{"issues", "actions", "list_issues", "list_workflows"}, + }, + { + name: "Missing toolset for pull request tools", + allowedTools: []string{"search_pull_requests", "pull_request_read", "list_pull_requests"}, + enabledToolsets: []string{"repos", "issues"}, // pull_requests missing + expectError: true, + errorContains: []string{"pull_requests", "search_pull_requests"}, + }, + { + name: "Unknown tool is ignored", + allowedTools: []string{"get_repository", "unknown_tool_xyz"}, + enabledToolsets: []string{"repos"}, + expectError: false, + }, + { + name: "Mix of known and unknown tools", + allowedTools: []string{"get_repository", "unknown_tool", "list_issues"}, + enabledToolsets: []string{"repos"}, // issues missing + expectError: true, + errorContains: []string{"issues", "list_issues"}, + }, + { + name: "Actions toolset tools", + allowedTools: []string{"list_workflows", "get_workflow_run", "download_workflow_run_artifact"}, + enabledToolsets: []string{"actions"}, + expectError: false, + }, + { + name: "Actions toolset missing", + allowedTools: []string{"list_workflows", "get_workflow_run"}, + enabledToolsets: []string{"repos"}, + expectError: true, + errorContains: []string{"actions", "list_workflows", "get_workflow_run"}, + }, + { + name: "Discussions and gists toolsets", + allowedTools: []string{"create_discussion", "create_gist"}, + enabledToolsets: []string{"discussions", "gists"}, + expectError: false, + }, + { + name: "Security-related toolsets", + allowedTools: []string{"list_code_scanning_alerts", "list_secret_scanning_alerts"}, + enabledToolsets: []string{"code_security", "secret_protection"}, + expectError: false, + }, + { + name: "Users and context toolsets", + allowedTools: []string{"get_user", "get_me", "get_teams"}, + enabledToolsets: []string{"users", "context"}, + expectError: false, + }, + { + name: "Search toolset", + allowedTools: []string{"search_repositories", "search_users"}, + enabledToolsets: []string{"search"}, + expectError: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + // Expand special toolsets (default, all) for testing + expandedToolsets := expandToolsetsForTesting(tt.enabledToolsets) + + err := ValidateGitHubToolsAgainstToolsets(tt.allowedTools, expandedToolsets) + + if tt.expectError { + if err == nil { + t.Errorf("Expected error but got none") + return + } + + errMsg := err.Error() + for _, expectedSubstr := range tt.errorContains { + if !strings.Contains(errMsg, expectedSubstr) { + t.Errorf("Expected error to contain %q, but it didn't.\nError: %s", expectedSubstr, errMsg) + } + } + } else { + if err != nil { + t.Errorf("Expected no error but got: %v", err) + } + } + }) + } +} + +func TestGitHubToolsetValidationError_Error(t *testing.T) { + tests := []struct { + name string + missingToolsets map[string][]string + expectedParts []string + }{ + { + name: "Single missing toolset with single tool", + missingToolsets: map[string][]string{ + "actions": {"list_workflows"}, + }, + expectedParts: []string{ + "ERROR", + "actions", + "list_workflows", + "Suggested fix", + }, + }, + { + name: "Single missing toolset with multiple tools", + missingToolsets: map[string][]string{ + "pull_requests": {"search_pull_requests", "pull_request_read", "list_pull_requests"}, + }, + expectedParts: []string{ + "ERROR", + "pull_requests", + "search_pull_requests", + "pull_request_read", + "list_pull_requests", + }, + }, + { + name: "Multiple missing toolsets", + missingToolsets: map[string][]string{ + "issues": {"list_issues", "create_issue"}, + "actions": {"list_workflows"}, + }, + expectedParts: []string{ + "ERROR", + "actions", + "issues", + "list_workflows", + "list_issues", + "create_issue", + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := NewGitHubToolsetValidationError(tt.missingToolsets) + errMsg := err.Error() + + for _, expectedPart := range tt.expectedParts { + if !strings.Contains(errMsg, expectedPart) { + t.Errorf("Expected error message to contain %q, but it didn't.\nError: %s", expectedPart, errMsg) + } + } + + // Verify it's a multi-line error with helpful formatting + if !strings.Contains(errMsg, "\n") { + t.Error("Expected multi-line error message") + } + }) + } +} + +func TestGitHubToolToToolsetMap_Completeness(t *testing.T) { + // Verify that the map contains entries for all expected tool categories + expectedToolsets := []string{ + "context", "repos", "issues", "pull_requests", "actions", + "code_security", "discussions", "gists", "labels", + "notifications", "orgs", "users", "search", "secret_protection", + } + + foundToolsets := make(map[string]bool) + for _, toolset := range GitHubToolToToolsetMap { + foundToolsets[toolset] = true + } + + for _, expectedToolset := range expectedToolsets { + if !foundToolsets[expectedToolset] { + t.Errorf("Expected to find tools for toolset %q in GitHubToolToToolsetMap", expectedToolset) + } + } +} + +func TestGitHubToolToToolsetMap_ConsistencyWithDocumentation(t *testing.T) { + // Sample of tools that should be in the map based on documentation + expectedMappings := map[string]string{ + "get_me": "context", + "get_repository": "repos", + "get_file_contents": "repos", + "list_issues": "issues", + "create_issue": "issues", + "pull_request_read": "pull_requests", + "search_pull_requests": "pull_requests", + "list_workflows": "actions", + "get_workflow_run": "actions", + "list_code_scanning_alerts": "code_security", + "create_discussion": "discussions", + "create_gist": "gists", + "get_label": "labels", + "list_notifications": "notifications", + "get_organization": "orgs", + "get_user": "users", + "search_repositories": "search", + "list_secret_scanning_alerts": "secret_protection", + } + + for tool, expectedToolset := range expectedMappings { + actualToolset, exists := GitHubToolToToolsetMap[tool] + if !exists { + t.Errorf("Expected tool %q to be in GitHubToolToToolsetMap", tool) + continue + } + if actualToolset != expectedToolset { + t.Errorf("Tool %q: expected toolset %q, got %q", tool, expectedToolset, actualToolset) + } + } +} + +// expandToolsetsForTesting expands "default" and "all" toolsets for testing purposes +func expandToolsetsForTesting(toolsets []string) []string { + var expanded []string + seenToolsets := make(map[string]bool) + + for _, toolset := range toolsets { + if toolset == "default" { + // Add default toolsets + for _, dt := range DefaultGitHubToolsets { + if !seenToolsets[dt] { + expanded = append(expanded, dt) + seenToolsets[dt] = true + } + } + } else if toolset == "all" { + // Add all toolsets from the permissions map + for t := range toolsetPermissionsMap { + if !seenToolsets[t] { + expanded = append(expanded, t) + seenToolsets[t] = true + } + } + } else { + // Add individual toolset + if !seenToolsets[toolset] { + expanded = append(expanded, toolset) + seenToolsets[toolset] = true + } + } + } + + return expanded +} diff --git a/pkg/workflow/github_toolset_validation_error.go b/pkg/workflow/github_toolset_validation_error.go new file mode 100644 index 00000000000..cd059ead730 --- /dev/null +++ b/pkg/workflow/github_toolset_validation_error.go @@ -0,0 +1,64 @@ +package workflow + +import ( + "fmt" + "sort" + "strings" +) + +// GitHubToolsetValidationError represents an error when GitHub tools are specified +// but their required toolsets are not enabled +type GitHubToolsetValidationError struct { + // MissingToolsets maps toolset name to the list of tools that require it + MissingToolsets map[string][]string +} + +// NewGitHubToolsetValidationError creates a new validation error +func NewGitHubToolsetValidationError(missingToolsets map[string][]string) *GitHubToolsetValidationError { + return &GitHubToolsetValidationError{ + MissingToolsets: missingToolsets, + } +} + +// Error implements the error interface +func (e *GitHubToolsetValidationError) Error() string { + var lines []string + lines = append(lines, "ERROR: GitHub tools specified in 'allowed' field require toolsets that are not enabled:") + lines = append(lines, "") + + // Sort toolsets for consistent output + var toolsets []string + for toolset := range e.MissingToolsets { + toolsets = append(toolsets, toolset) + } + sort.Strings(toolsets) + + // List each missing toolset and the tools that need it + for _, toolset := range toolsets { + tools := e.MissingToolsets[toolset] + sort.Strings(tools) + lines = append(lines, fmt.Sprintf(" Toolset '%s' is required by:", toolset)) + for _, tool := range tools { + lines = append(lines, fmt.Sprintf(" - %s", tool)) + } + lines = append(lines, "") + } + + // Provide fix suggestion + lines = append(lines, "Suggested fix: Add the missing toolsets to your GitHub tool configuration:") + lines = append(lines, "") + lines = append(lines, "tools:") + lines = append(lines, " github:") + lines = append(lines, " toolsets:") + + // Build the toolsets list + var allToolsets []string + allToolsets = append(allToolsets, "default") // Start with default + allToolsets = append(allToolsets, toolsets...) + + for _, toolset := range allToolsets { + lines = append(lines, fmt.Sprintf(" - %s", toolset)) + } + + return strings.Join(lines, "\n") +} diff --git a/pkg/workflow/github_toolset_validation_integration_test.go b/pkg/workflow/github_toolset_validation_integration_test.go new file mode 100644 index 00000000000..d79dd032389 --- /dev/null +++ b/pkg/workflow/github_toolset_validation_integration_test.go @@ -0,0 +1,369 @@ +package workflow + +import ( + "os" + "path/filepath" + "strings" + "testing" +) + +func TestGitHubToolsetValidationIntegration(t *testing.T) { + tests := []struct { + name string + frontmatter string + expectError bool + errorContains []string + }{ + { + name: "Valid configuration - tools match toolsets", + frontmatter: `--- +on: issues +engine: copilot +tools: + github: + toolsets: + - repos + - pull_requests + allowed: + - get_repository + - search_pull_requests +--- + +# Test workflow +Test content. +`, + expectError: false, + }, + { + name: "Valid configuration - default toolsets include required toolsets", + frontmatter: `--- +on: issues +engine: copilot +tools: + github: + toolsets: + - default + allowed: + - get_repository + - list_issues +--- + +# Test workflow +Test content. +`, + expectError: false, + }, + { + name: "Valid configuration - all toolset enables everything", + frontmatter: `--- +on: issues +engine: copilot +tools: + github: + toolsets: + - all + allowed: + - get_repository + - list_workflows + - create_gist +--- + +# Test workflow +Test content. +`, + expectError: false, + }, + { + name: "Invalid configuration - missing toolset for allowed tool", + frontmatter: `--- +on: issues +engine: copilot +tools: + github: + toolsets: + - repos + allowed: + - get_repository + - list_issues +--- + +# Test workflow +Test content. +`, + expectError: true, + errorContains: []string{"issues", "list_issues", "Toolset 'issues' is required by"}, + }, + { + name: "Invalid configuration - multiple missing toolsets", + frontmatter: `--- +on: issues +engine: copilot +tools: + github: + toolsets: + - repos + allowed: + - get_repository + - list_issues + - list_workflows + - search_pull_requests +--- + +# Test workflow +Test content. +`, + expectError: true, + errorContains: []string{ + "issues", + "actions", + "pull_requests", + "list_issues", + "list_workflows", + "search_pull_requests", + }, + }, + { + name: "Valid configuration - no allowed field means no validation", + frontmatter: `--- +on: issues +engine: copilot +tools: + github: + toolsets: + - repos +--- + +# Test workflow +Test content. +`, + expectError: false, + }, + { + name: "Valid configuration - empty allowed array means no validation", + frontmatter: `--- +on: issues +engine: copilot +tools: + github: + toolsets: + - repos + allowed: [] +--- + +# Test workflow +Test content. +`, + expectError: false, + }, + { + name: "Invalid configuration - actions toolset missing", + frontmatter: `--- +on: workflow_dispatch +engine: copilot +tools: + github: + toolsets: + - repos + - issues + allowed: + - list_workflows + - get_workflow_run + - download_workflow_run_artifact +--- + +# Test workflow +Test content. +`, + expectError: true, + errorContains: []string{ + "actions", + "list_workflows", + "get_workflow_run", + "download_workflow_run_artifact", + }, + }, + { + name: "Valid configuration - default plus additional toolset", + frontmatter: `--- +on: issues +engine: copilot +tools: + github: + toolsets: + - default + - actions + allowed: + - get_repository + - list_issues + - list_workflows +--- + +# Test workflow +Test content. +`, + expectError: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + // Create a temporary directory for the test + tmpDir, err := os.MkdirTemp("", "github-toolset-validation-test-*") + if err != nil { + t.Fatalf("Failed to create temp dir: %v", err) + } + defer os.RemoveAll(tmpDir) + + // Create the .github/workflows directory + workflowsDir := filepath.Join(tmpDir, ".github", "workflows") + if err := os.MkdirAll(workflowsDir, 0755); err != nil { + t.Fatalf("Failed to create workflows dir: %v", err) + } + + // Write the test workflow + workflowPath := filepath.Join(workflowsDir, "test-workflow.md") + if err := os.WriteFile(workflowPath, []byte(tt.frontmatter), 0644); err != nil { + t.Fatalf("Failed to write workflow file: %v", err) + } + + // Create a compiler instance + compiler := NewCompiler(false, "", "test") + + // Try to compile the workflow + err = compiler.CompileWorkflow(workflowPath) + + if tt.expectError { + if err == nil { + t.Errorf("Expected compilation error but got none") + return + } + + errMsg := err.Error() + for _, expectedSubstr := range tt.errorContains { + if !strings.Contains(errMsg, expectedSubstr) { + t.Errorf("Expected error to contain %q, but it didn't.\nError: %s", expectedSubstr, errMsg) + } + } + } else { + if err != nil { + t.Errorf("Expected no error but got: %v", err) + } + } + }) + } +} + +func TestGitHubToolsetValidationWithRemoteMode(t *testing.T) { + frontmatter := `--- +on: issues +engine: copilot +tools: + github: + mode: remote + toolsets: + - repos + allowed: + - get_repository + - list_issues +--- + +# Test workflow +Test content. +` + + // Create a temporary directory for the test + tmpDir, err := os.MkdirTemp("", "github-toolset-validation-remote-test-*") + if err != nil { + t.Fatalf("Failed to create temp dir: %v", err) + } + defer os.RemoveAll(tmpDir) + + // Create the .github/workflows directory + workflowsDir := filepath.Join(tmpDir, ".github", "workflows") + if err := os.MkdirAll(workflowsDir, 0755); err != nil { + t.Fatalf("Failed to create workflows dir: %v", err) + } + + // Write the test workflow + workflowPath := filepath.Join(workflowsDir, "test-workflow.md") + if err := os.WriteFile(workflowPath, []byte(frontmatter), 0644); err != nil { + t.Fatalf("Failed to write workflow file: %v", err) + } + + // Create a compiler instance + compiler := NewCompiler(false, "", "test") + + // Try to compile the workflow + err = compiler.CompileWorkflow(workflowPath) + + // Should fail because 'issues' toolset is missing + if err == nil { + t.Error("Expected compilation error but got none") + return + } + + errMsg := err.Error() + expectedSubstrings := []string{"issues", "list_issues"} + for _, expectedSubstr := range expectedSubstrings { + if !strings.Contains(errMsg, expectedSubstr) { + t.Errorf("Expected error to contain %q, but it didn't.\nError: %s", expectedSubstr, errMsg) + } + } +} + +func TestGitHubToolsetValidationWithClaudeEngine(t *testing.T) { + frontmatter := `--- +on: issues +engine: claude +tools: + github: + toolsets: + - repos + allowed: + - get_repository + - create_discussion +--- + +# Test workflow +Test content. +` + + // Create a temporary directory for the test + tmpDir, err := os.MkdirTemp("", "github-toolset-validation-claude-test-*") + if err != nil { + t.Fatalf("Failed to create temp dir: %v", err) + } + defer os.RemoveAll(tmpDir) + + // Create the .github/workflows directory + workflowsDir := filepath.Join(tmpDir, ".github", "workflows") + if err := os.MkdirAll(workflowsDir, 0755); err != nil { + t.Fatalf("Failed to create workflows dir: %v", err) + } + + // Write the test workflow + workflowPath := filepath.Join(workflowsDir, "test-workflow.md") + if err := os.WriteFile(workflowPath, []byte(frontmatter), 0644); err != nil { + t.Fatalf("Failed to write workflow file: %v", err) + } + + // Create a compiler instance + compiler := NewCompiler(false, "", "test") + + // Try to compile the workflow + err = compiler.CompileWorkflow(workflowPath) + + // Should fail because 'discussions' toolset is missing + if err == nil { + t.Error("Expected compilation error but got none") + return + } + + errMsg := err.Error() + expectedSubstrings := []string{"discussions", "create_discussion"} + for _, expectedSubstr := range expectedSubstrings { + if !strings.Contains(errMsg, expectedSubstr) { + t.Errorf("Expected error to contain %q, but it didn't.\nError: %s", expectedSubstr, errMsg) + } + } +} diff --git a/pkg/workflow/github_toolsets_test.go b/pkg/workflow/github_toolsets_test.go index 89288273447..5ce6459c4dc 100644 --- a/pkg/workflow/github_toolsets_test.go +++ b/pkg/workflow/github_toolsets_test.go @@ -7,11 +7,11 @@ import ( func TestDefaultGitHubToolsets(t *testing.T) { // Verify the default toolsets match the documented defaults expected := []string{"context", "repos", "issues", "pull_requests", "users"} - + if len(DefaultGitHubToolsets) != len(expected) { t.Errorf("Expected %d default toolsets, got %d", len(expected), len(DefaultGitHubToolsets)) } - + for i, toolset := range expected { if i >= len(DefaultGitHubToolsets) || DefaultGitHubToolsets[i] != toolset { t.Errorf("Expected default toolset[%d] to be %s, got %s", i, toolset, DefaultGitHubToolsets[i]) diff --git a/pkg/workflow/permissions_validator.go b/pkg/workflow/permissions_validator.go index d01eee6b169..7d09b8b952b 100644 --- a/pkg/workflow/permissions_validator.go +++ b/pkg/workflow/permissions_validator.go @@ -166,16 +166,6 @@ func containsAllToolset(toolsetsStr string) bool { return false } -// containsToolset checks if a slice contains a specific toolset -func containsToolset(toolsets []string, target string) bool { - for _, t := range toolsets { - if t == target { - return true - } - } - return false -} - // collectRequiredPermissions collects all required permissions for the given toolsets func collectRequiredPermissions(toolsets []string, readOnly bool) map[PermissionScope]PermissionLevel { required := make(map[PermissionScope]PermissionLevel) diff --git a/pkg/workflow/permissions_validator_test.go b/pkg/workflow/permissions_validator_test.go index a19e81d6595..0ad7e4f1f4d 100644 --- a/pkg/workflow/permissions_validator_test.go +++ b/pkg/workflow/permissions_validator_test.go @@ -121,19 +121,19 @@ func TestCollectRequiredPermissions(t *testing.T) { func TestValidatePermissions_MissingPermissions(t *testing.T) { tests := []struct { - name string - permissions *Permissions - githubTool any - expectMissing map[PermissionScope]PermissionLevel - expectMissingCount int - expectExcessCount int - expectHasIssues bool + name string + permissions *Permissions + githubTool any + expectMissing map[PermissionScope]PermissionLevel + expectMissingCount int + expectExcessCount int + expectHasIssues bool }{ { - name: "No GitHub tool configured", - permissions: NewPermissions(), - githubTool: nil, - expectMissing: map[PermissionScope]PermissionLevel{}, + name: "No GitHub tool configured", + permissions: NewPermissions(), + githubTool: nil, + expectMissing: map[PermissionScope]PermissionLevel{}, expectMissingCount: 0, expectExcessCount: 0, expectHasIssues: false, @@ -375,11 +375,11 @@ func TestValidatePermissions_ExcessPermissions(t *testing.T) { func TestFormatValidationMessage(t *testing.T) { tests := []struct { - name string - result *PermissionsValidationResult - strict bool - expectContains []string - expectNotContains []string + name string + result *PermissionsValidationResult + strict bool + expectContains []string + expectNotContains []string }{ { name: "No validation issues", @@ -551,7 +551,7 @@ func TestValidatePermissions_ComplexScenarios(t *testing.T) { }, }, { - name: "All: read with discussions toolset", + name: "All: read with discussions toolset", permissions: NewPermissionsAllRead(), githubTool: map[string]any{ "toolsets": []string{"discussions"}, diff --git a/pkg/workflow/tools_types.go b/pkg/workflow/tools_types.go index 0c1f26a2255..7d8c3101849 100644 --- a/pkg/workflow/tools_types.go +++ b/pkg/workflow/tools_types.go @@ -202,7 +202,15 @@ func parseGitHubTool(val any) *GitHubToolConfig { config.GitHubToken = token } - if toolset, ok := configMap["toolset"].([]any); ok { + // Check for both "toolset" and "toolsets" (plural is more common in user configs) + if toolset, ok := configMap["toolsets"].([]any); ok { + config.Toolset = make([]string, 0, len(toolset)) + for _, item := range toolset { + if str, ok := item.(string); ok { + config.Toolset = append(config.Toolset, str) + } + } + } else if toolset, ok := configMap["toolset"].([]any); ok { config.Toolset = make([]string, 0, len(toolset)) for _, item := range toolset { if str, ok := item.(string); ok {