Skip to content

Introduce ValidatableTool interface for type-safe permission validation #6896

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
pelikhan merged 4 commits into from
Dec 18, 2025
Merged

Introduce ValidatableTool interface for type-safe permission validation #6896

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5ee4cf0
Initial plan
Copilot Dec 18, 2025
514871c
Initial plan for ValidatableTool interface implementation
Copilot Dec 18, 2025
0e88c8e
Implement ValidatableTool interface for type-safe permission validation
Copilot Dec 18, 2025
b4e4dab
Complete ValidatableTool interface implementation - all tests pass
Copilot Dec 18, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions .github/workflows/daily-team-status.lock.yml
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 2 additions & 0 deletions docs/src/content/docs/labs.mdx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@ These are experimental agentic workflows used by the GitHub Next team to learn,
| [Glossary Maintainer](https://github.com/githubnext/gh-aw/blob/main/.github/workflows/glossary-maintainer.md) | copilot | [![Glossary Maintainer](https://github.com/githubnext/gh-aw/actions/workflows/glossary-maintainer.lock.yml/badge.svg)](https://github.com/githubnext/gh-aw/actions/workflows/glossary-maintainer.lock.yml) | `0 10 * * 1-5` | - |
| [Go Fan](https://github.com/githubnext/gh-aw/blob/main/.github/workflows/go-fan.md) | claude | [![Go Fan](https://github.com/githubnext/gh-aw/actions/workflows/go-fan.lock.yml/badge.svg)](https://github.com/githubnext/gh-aw/actions/workflows/go-fan.lock.yml) | `0 7 * * 1-5` | - |
| [Go File Size Reduction Campaign](https://github.com/githubnext/gh-aw/blob/main/.github/workflows/go-file-size-reduction.campaign.g.md) | copilot | [![Go File Size Reduction Campaign](https://github.com/githubnext/gh-aw/actions/workflows/go-file-size-reduction.campaign.g.lock.yml/badge.svg)](https://github.com/githubnext/gh-aw/actions/workflows/go-file-size-reduction.campaign.g.lock.yml) | `0 18 * * *` | - |
| [Go File Size Reduction Campaign (Project 64)](https://github.com/githubnext/gh-aw/blob/main/.github/workflows/go-file-size-reduction-project64.campaign.g.md) | copilot | [![Go File Size Reduction Campaign (Project 64)](https://github.com/githubnext/gh-aw/actions/workflows/go-file-size-reduction-project64.campaign.g.lock.yml/badge.svg)](https://github.com/githubnext/gh-aw/actions/workflows/go-file-size-reduction-project64.campaign.g.lock.yml) | `0 18 * * *` | - |
| [Go Logger Enhancement](https://github.com/githubnext/gh-aw/blob/main/.github/workflows/go-logger.md) | claude | [![Go Logger Enhancement](https://github.com/githubnext/gh-aw/actions/workflows/go-logger.lock.yml/badge.svg)](https://github.com/githubnext/gh-aw/actions/workflows/go-logger.lock.yml) | - | - |
| [Go Pattern Detector](https://github.com/githubnext/gh-aw/blob/main/.github/workflows/go-pattern-detector.md) | claude | [![Go Pattern Detector](https://github.com/githubnext/gh-aw/actions/workflows/go-pattern-detector.lock.yml/badge.svg)](https://github.com/githubnext/gh-aw/actions/workflows/go-pattern-detector.lock.yml) | `0 14 * * 1-5` | - |
| [Grumpy Code Reviewer 🔥](https://github.com/githubnext/gh-aw/blob/main/.github/workflows/grumpy-reviewer.md) | copilot | [![Grumpy Code Reviewer 🔥](https://github.com/githubnext/gh-aw/actions/workflows/grumpy-reviewer.lock.yml/badge.svg)](https://github.com/githubnext/gh-aw/actions/workflows/grumpy-reviewer.lock.yml) | - | - |
Expand All @@ -77,6 +78,7 @@ These are experimental agentic workflows used by the GitHub Next team to learn,
| [Issue Monster](https://github.com/githubnext/gh-aw/blob/main/.github/workflows/issue-monster.md) | copilot | [![Issue Monster](https://github.com/githubnext/gh-aw/actions/workflows/issue-monster.lock.yml/badge.svg)](https://github.com/githubnext/gh-aw/actions/workflows/issue-monster.lock.yml) | - | - |
| [Issue Summary to Notion](https://github.com/githubnext/gh-aw/blob/main/.github/workflows/notion-issue-summary.md) | copilot | [![Issue Summary to Notion](https://github.com/githubnext/gh-aw/actions/workflows/notion-issue-summary.lock.yml/badge.svg)](https://github.com/githubnext/gh-aw/actions/workflows/notion-issue-summary.lock.yml) | - | - |
| [Issue Triage Agent](https://github.com/githubnext/gh-aw/blob/main/.github/workflows/issue-triage-agent.md) | copilot | [![Issue Triage Agent](https://github.com/githubnext/gh-aw/actions/workflows/issue-triage-agent.lock.yml/badge.svg)](https://github.com/githubnext/gh-aw/actions/workflows/issue-triage-agent.lock.yml) | - | - |
| [jsweep - JavaScript Unbloater](https://github.com/githubnext/gh-aw/blob/main/.github/workflows/jsweep.md) | copilot | [![jsweep - JavaScript Unbloater](https://github.com/githubnext/gh-aw/actions/workflows/jsweep.lock.yml/badge.svg)](https://github.com/githubnext/gh-aw/actions/workflows/jsweep.lock.yml) | - | - |
| [Layout Specification Maintainer](https://github.com/githubnext/gh-aw/blob/main/.github/workflows/layout-spec-maintainer.md) | copilot | [![Layout Specification Maintainer](https://github.com/githubnext/gh-aw/actions/workflows/layout-spec-maintainer.lock.yml/badge.svg)](https://github.com/githubnext/gh-aw/actions/workflows/layout-spec-maintainer.lock.yml) | `0 7 * * 1-5` | - |
| [Lockfile Statistics Analysis Agent](https://github.com/githubnext/gh-aw/blob/main/.github/workflows/lockfile-stats.md) | claude | [![Lockfile Statistics Analysis Agent](https://github.com/githubnext/gh-aw/actions/workflows/lockfile-stats.lock.yml/badge.svg)](https://github.com/githubnext/gh-aw/actions/workflows/lockfile-stats.lock.yml) | - | - |
| [MCP Inspector Agent](https://github.com/githubnext/gh-aw/blob/main/.github/workflows/mcp-inspector.md) | copilot | [![MCP Inspector Agent](https://github.com/githubnext/gh-aw/actions/workflows/mcp-inspector.lock.yml/badge.svg)](https://github.com/githubnext/gh-aw/actions/workflows/mcp-inspector.lock.yml) | - | - |
Expand Down
6 changes: 3 additions & 3 deletions pkg/workflow/compiler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,13 +134,13 @@ func (c *Compiler) CompileWorkflowData(workflowData *WorkflowData, markdownPath

// Validate permissions against GitHub MCP toolsets
log.Printf("Validating permissions for GitHub MCP toolsets")
if githubTool, hasGitHub := workflowData.Tools["github"]; hasGitHub {
if workflowData.ParsedTools != nil && workflowData.ParsedTools.GitHub != nil {
// 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)
// Validate permissions using the typed GitHub tool configuration
validationResult := ValidatePermissions(permissions, workflowData.ParsedTools.GitHub)

if validationResult.HasValidationIssues {
// Format the validation message
Expand Down
53 changes: 41 additions & 12 deletions pkg/workflow/permissions_validator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,35 @@ func GetToolsetsData() GitHubToolsetsData {
return data
}

// ValidatableTool represents a tool configuration that can be validated for permissions
// This interface abstracts the tool configuration structure to enable type-safe permission validation
type ValidatableTool interface {
// GetToolsets returns the comma-separated list of toolsets configured for this tool
GetToolsets() string
// IsReadOnly returns whether the tool is configured in read-only mode
IsReadOnly() bool
}

// GetToolsets implements ValidatableTool for GitHubToolConfig
func (g *GitHubToolConfig) GetToolsets() string {
if g == nil {
// Should not happen - ValidatePermissions checks for nil before calling this
return ""
}
// Convert toolset array to comma-separated string
// If empty, expandDefaultToolset will apply defaults
toolsetsStr := strings.Join(g.Toolset, ",")
return expandDefaultToolset(toolsetsStr)
}

// IsReadOnly implements ValidatableTool for GitHubToolConfig
func (g *GitHubToolConfig) IsReadOnly() bool {
if g == nil {
return true // default to read-only for security
}
return g.ReadOnly
}

// PermissionsValidationResult contains the result of permissions validation
type PermissionsValidationResult struct {
MissingPermissions map[PermissionScope]PermissionLevel // Permissions required but not granted
Expand All @@ -95,21 +124,14 @@ type PermissionsValidationResult struct {
//
// Parameters:
// - permissions: The workflow's declared permissions
// - githubTool: The GitHub tool configuration (uses any type because the structure varies
// based on the engine and toolsets being used. This is an appropriate use of any for
// dynamic configuration data that cannot be known at compile time.)
// - githubTool: The GitHub tool configuration implementing ValidatableTool interface
//
// Returns:
// - A validation result indicating any missing permissions and which toolsets require them
//
// Type Pattern Note: githubTool uses 'any' because tool configuration structure is dynamic
// and varies based on engine (copilot, claude, codex) and mode (local, remote). This is
// parsed from YAML frontmatter where the structure is not known until runtime.
// See specs/go-type-patterns.md for guidance on when to use 'any' types.
//
// Use ValidatePermissions (this function) for general permission validation against GitHub MCP toolsets.
// Use ValidateIncludedPermissions (in imports.go) when validating permissions from included/imported workflow files.
func ValidatePermissions(permissions *Permissions, githubTool any) *PermissionsValidationResult {
func ValidatePermissions(permissions *Permissions, githubTool ValidatableTool) *PermissionsValidationResult {
permissionsValidatorLog.Print("Starting permissions validation")

result := &PermissionsValidationResult{
Expand All @@ -118,14 +140,21 @@ func ValidatePermissions(permissions *Permissions, githubTool any) *PermissionsV
}

// If GitHub tool is not configured, no validation needed
// Check both for nil interface and nil concrete type
if githubTool == nil {
permissionsValidatorLog.Print("No GitHub tool configured, skipping validation")
permissionsValidatorLog.Print("No GitHub tool configured (nil interface), skipping validation")
return result
}

// Check if concrete type is nil (interface wrapping nil pointer)
if config, ok := githubTool.(*GitHubToolConfig); ok && config == nil {
permissionsValidatorLog.Print("No GitHub tool configured (nil concrete type), skipping validation")
return result
}

// Extract toolsets from GitHub tool configuration
toolsetsStr := getGitHubToolsets(githubTool)
readOnly := getGitHubReadOnly(githubTool)
toolsetsStr := githubTool.GetToolsets()
readOnly := githubTool.IsReadOnly()
result.ReadOnlyMode = readOnly

permissionsValidatorLog.Printf("Validating toolsets: %s, read-only: %v", toolsetsStr, readOnly)
Expand Down
60 changes: 30 additions & 30 deletions pkg/workflow/permissions_validator_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,24 +123,24 @@ func TestValidatePermissions_MissingPermissions(t *testing.T) {
tests := []struct {
name string
permissions *Permissions
githubTool any
githubToolConfig *GitHubToolConfig
expectMissing map[PermissionScope]PermissionLevel
expectMissingCount int
expectHasIssues bool
}{
{
name: "No GitHub tool configured",
permissions: NewPermissions(),
githubTool: nil,
githubToolConfig: nil,
expectMissing: map[PermissionScope]PermissionLevel{},
expectMissingCount: 0,
expectHasIssues: false,
},
{
name: "Default toolsets with no permissions",
permissions: NewPermissions(),
githubTool: map[string]any{
"toolsets": []string{"default"},
githubToolConfig: &GitHubToolConfig{
Toolset: []string{"default"},
},
expectMissingCount: 3, // contents, issues, pull-requests
expectHasIssues: true,
Expand All @@ -152,9 +152,9 @@ func TestValidatePermissions_MissingPermissions(t *testing.T) {
PermissionIssues: PermissionWrite,
PermissionPullRequests: PermissionWrite,
}),
githubTool: map[string]any{
"toolsets": []string{"default"},
"read-only": false,
githubToolConfig: &GitHubToolConfig{
Toolset: []string{"default"},
ReadOnly: false,
},
expectMissingCount: 0,
expectHasIssues: false,
Expand All @@ -166,9 +166,9 @@ func TestValidatePermissions_MissingPermissions(t *testing.T) {
PermissionIssues: PermissionRead,
PermissionPullRequests: PermissionRead,
}),
githubTool: map[string]any{
"toolsets": []string{"default"},
"read-only": false, // Need write permissions
githubToolConfig: &GitHubToolConfig{
Toolset: []string{"default"},
ReadOnly: false, // Need write permissions
},
expectMissingCount: 3, // All need write
expectHasIssues: true,
Expand All @@ -180,9 +180,9 @@ func TestValidatePermissions_MissingPermissions(t *testing.T) {
PermissionIssues: PermissionRead,
PermissionPullRequests: PermissionRead,
}),
githubTool: map[string]any{
"toolsets": []string{"default"},
"read-only": true,
githubToolConfig: &GitHubToolConfig{
Toolset: []string{"default"},
ReadOnly: true,
},
expectMissingCount: 0,
expectHasIssues: false,
Expand All @@ -192,9 +192,9 @@ func TestValidatePermissions_MissingPermissions(t *testing.T) {
permissions: NewPermissionsFromMap(map[PermissionScope]PermissionLevel{
PermissionContents: PermissionWrite,
}),
githubTool: map[string]any{
"toolsets": []string{"repos", "issues"},
"read-only": false,
githubToolConfig: &GitHubToolConfig{
Toolset: []string{"repos", "issues"},
ReadOnly: false,
},
expectMissingCount: 1, // Missing issues: write
expectHasIssues: true,
Expand All @@ -204,8 +204,8 @@ func TestValidatePermissions_MissingPermissions(t *testing.T) {
permissions: NewPermissionsFromMap(map[PermissionScope]PermissionLevel{
PermissionActions: PermissionRead,
}),
githubTool: map[string]any{
"toolsets": []string{"actions"},
githubToolConfig: &GitHubToolConfig{
Toolset: []string{"actions"},
},
expectMissingCount: 0,
expectHasIssues: false,
Expand All @@ -214,7 +214,7 @@ func TestValidatePermissions_MissingPermissions(t *testing.T) {

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
result := ValidatePermissions(tt.permissions, tt.githubTool)
result := ValidatePermissions(tt.permissions, tt.githubToolConfig)

if len(result.MissingPermissions) != tt.expectMissingCount {
t.Errorf("Expected %d missing permissions, got %d: %v",
Expand Down Expand Up @@ -336,17 +336,17 @@ func TestToolsetPermissionsMapping(t *testing.T) {

func TestValidatePermissions_ComplexScenarios(t *testing.T) {
tests := []struct {
name string
permissions *Permissions
githubTool any
expectMsg []string
name string
permissions *Permissions
githubToolConfig *GitHubToolConfig
expectMsg []string
}{
{
name: "Shorthand read-all with default toolsets",
permissions: NewPermissionsReadAll(),
githubTool: map[string]any{
"toolsets": []string{"default"},
"read-only": false,
githubToolConfig: &GitHubToolConfig{
Toolset: []string{"default"},
ReadOnly: false,
},
expectMsg: []string{
"Missing required permissions for github toolsets:",
Expand All @@ -358,9 +358,9 @@ func TestValidatePermissions_ComplexScenarios(t *testing.T) {
{
name: "All: read with discussions toolset",
permissions: NewPermissionsAllRead(),
githubTool: map[string]any{
"toolsets": []string{"discussions"},
"read-only": false,
githubToolConfig: &GitHubToolConfig{
Toolset: []string{"discussions"},
ReadOnly: false,
},
expectMsg: []string{
"Missing required permissions for github toolsets:",
Expand All @@ -371,7 +371,7 @@ func TestValidatePermissions_ComplexScenarios(t *testing.T) {

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
result := ValidatePermissions(tt.permissions, tt.githubTool)
result := ValidatePermissions(tt.permissions, tt.githubToolConfig)
message := FormatValidationMessage(result, false)

for _, expected := range tt.expectMsg {
Expand Down
17 changes: 13 additions & 4 deletions pkg/workflow/tools_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -344,19 +344,25 @@ func NewTools(toolsMap map[string]any) *Tools {
func parseGitHubTool(val any) *GitHubToolConfig {
if val == nil {
toolsTypesLog.Print("GitHub tool enabled with default configuration")
return &GitHubToolConfig{}
return &GitHubToolConfig{
ReadOnly: true, // default to read-only for security
}
}

// Handle string type (simple enable)
if _, ok := val.(string); ok {
toolsTypesLog.Print("GitHub tool enabled with string configuration")
return &GitHubToolConfig{}
return &GitHubToolConfig{
ReadOnly: true, // default to read-only for security
}
}

// Handle map type (detailed configuration)
if configMap, ok := val.(map[string]any); ok {
toolsTypesLog.Print("Parsing GitHub tool detailed configuration")
config := &GitHubToolConfig{}
config := &GitHubToolConfig{
ReadOnly: true, // default to read-only for security
}

if allowed, ok := configMap["allowed"].([]any); ok {
config.Allowed = make([]string, 0, len(allowed))
Expand Down Expand Up @@ -387,6 +393,7 @@ func parseGitHubTool(val any) *GitHubToolConfig {
if readOnly, ok := configMap["read-only"].(bool); ok {
config.ReadOnly = readOnly
}
// else: defaults to true (set above)

if token, ok := configMap["github-token"].(string); ok {
config.GitHubToken = token
Expand Down Expand Up @@ -416,7 +423,9 @@ func parseGitHubTool(val any) *GitHubToolConfig {
return config
}

return &GitHubToolConfig{}
return &GitHubToolConfig{
ReadOnly: true, // default to read-only for security
}
}

// parseBashTool converts raw bash tool configuration to BashToolConfig
Expand Down
Loading