Add GitHub tool/toolset validator for allowed tools configuration#2770
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
|
||
| // Validate GitHub tools against enabled toolsets | ||
| log.Printf("Validating GitHub tools against enabled toolsets") | ||
| if githubTool, hasGitHub := workflowData.Tools["github"]; hasGitHub { |
There was a problem hiding this comment.
@copilot use hasGithub or parsed tools instead
There was a problem hiding this comment.
Updated to use workflowData.ParsedTools.GitHub for type safety. Also fixed parsing to support both toolset and toolsets field names. (67cee68)
|
|
||
| // GitHubToolToToolsetMap maps individual GitHub MCP tools to their respective toolsets | ||
| // This mapping is based on the documentation in .github/instructions/github-mcp-server.instructions.md | ||
| var GitHubToolToToolsetMap = map[string]string{ |
There was a problem hiding this comment.
Moved to embedded JSON file at pkg/workflow/data/github_tool_to_toolset.json. The mapping is now loaded via go:embed directive in the init function. (67cee68)
- Use workflowData.ParsedTools.GitHub instead of raw Tools map - Support both 'toolset' and 'toolsets' field names in parseGitHubTool - Move GitHub tool-to-toolset mapping to embedded JSON file - Addresses review feedback from @pelikhan Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
Agentic Changeset Generator triggered by this pull request. |
There was a problem hiding this comment.
Pull Request Overview
This PR adds compile-time validation to ensure that when users specify individual GitHub tools via the allowed field, their corresponding toolsets are enabled in the workflow configuration. Previously, misconfigured workflows would compile successfully but fail at runtime.
Key Changes:
- Implemented tool-to-toolset validation with clear error messaging
- Moved tool-to-toolset mapping to embedded JSON for maintainability
- Added support for both
toolsetandtoolsetsfield names in configurations
Reviewed Changes
Copilot reviewed 9 out of 11 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/compiler.go |
Integrated validation step during workflow compilation using ParsedTools |
pkg/workflow/tools_types.go |
Added parsing support for plural toolsets field name |
pkg/workflow/github_tool_to_toolset.go |
Core validation logic with embedded JSON loader |
pkg/workflow/data/github_tool_to_toolset.json |
Tool-to-toolset mapping data (50+ GitHub tools) |
pkg/workflow/github_toolset_validation_error.go |
Custom error type with formatted output and fix suggestions |
pkg/workflow/github_tool_to_toolset_test.go |
Unit tests covering validation scenarios and mapping consistency |
pkg/workflow/github_toolset_validation_integration_test.go |
Integration tests for Copilot, Claude, and remote mode |
pkg/workflow/permissions_validator.go |
Removed unused containsToolset helper function |
.github/workflows/technical-doc-writer.lock.yml |
Updated actions/setup-node to newer version |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // 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 { |
There was a problem hiding this comment.
The code for parsing toolsets (lines 207-212) and toolset (lines 214-218, not shown but identical) is duplicated. Consider extracting this logic into a helper function to reduce duplication and improve maintainability.
|
|
||
| "get_repository": "repos", |
There was a problem hiding this comment.
[nitpick] The JSON file contains inconsistent blank lines (e.g., line 5) between groupings. While this doesn't affect functionality, removing these blank lines would make the file more consistent and easier to maintain, as JSON doesn't require blank lines for readability.
GitHub Tool/Toolset Validator Implementation ✅
This PR adds validation to ensure that when users specify individual GitHub tools using the
allowedfield, the corresponding toolsets are enabled in the configuration.Recent Updates (addressing review feedback)
Changed to use ParsedTools - Now uses
workflowData.ParsedTools.GitHubinstead of accessing rawToolsmap directly for better type safety.Moved mapping to embedded JSON - The tool-to-toolset mapping is now loaded from an embedded JSON file (
data/github_tool_to_toolset.json) instead of being a large map literal in code.Fixed toolsets field parsing - Now supports both
toolset(singular) andtoolsets(plural) field names to handle common user configurations.Problem Solved
When users specify tools like
list_workflowsin theallowedfield but forget to enable theactionstoolset, the workflow would compile but fail at runtime. This validator catches the issue at compile time with clear error messages.Solution Overview
Example Error Message
Test Coverage
Files Added
pkg/workflow/github_tool_to_toolset.go- Validation logic with embedded JSON loaderpkg/workflow/data/github_tool_to_toolset.json- Tool to toolset mapping datapkg/workflow/github_tool_to_toolset_test.go- Unit testspkg/workflow/github_toolset_validation_error.go- Error typepkg/workflow/github_toolset_validation_integration_test.go- Integration testsFiles Modified
pkg/workflow/compiler.go- Added validation using ParsedToolspkg/workflow/tools_types.go- Support both toolset/toolsets field namesQuality Checks
gofmtOriginal prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.