From 9ff2ddb04a52073093f9123a4ed25ee5ecdd7097 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 19 Dec 2025 06:58:45 +0000 Subject: [PATCH 1/5] Initial plan From 5c498bb5fc12fcfd18072fb59fcb6c87f7151a0c Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 19 Dec 2025 07:09:04 +0000 Subject: [PATCH 2/5] refactor(workflow): split tools_types.go into types and parser files Separates type definitions from parsing logic for better organization. - tools_types.go: keeps type definitions and helper methods - tools_parser.go: contains all parse* functions and NewTools Issue: #1A from semantic function clustering analysis Co-authored-by: mnkiefer <8320933+mnkiefer@users.noreply.github.com> --- pkg/workflow/tools_parser.go | 461 +++++++++++++++++++++++++++++++++++ pkg/workflow/tools_types.go | 452 ---------------------------------- 2 files changed, 461 insertions(+), 452 deletions(-) create mode 100644 pkg/workflow/tools_parser.go diff --git a/pkg/workflow/tools_parser.go b/pkg/workflow/tools_parser.go new file mode 100644 index 0000000000..b4bc5e1f67 --- /dev/null +++ b/pkg/workflow/tools_parser.go @@ -0,0 +1,461 @@ +package workflow + +import ( + "fmt" + + "github.com/githubnext/gh-aw/pkg/logger" +) + +var toolsParserLog = logger.New("workflow:tools_parser") + +// NewTools creates a new Tools instance from a map +func NewTools(toolsMap map[string]any) *Tools { + toolsParserLog.Printf("Creating tools configuration from map with %d entries", len(toolsMap)) + if toolsMap == nil { + return &Tools{ + Custom: make(map[string]any), + raw: make(map[string]any), + } + } + + tools := &Tools{ + Custom: make(map[string]any), + raw: make(map[string]any), + } + + // Copy raw map + for k, v := range toolsMap { + tools.raw[k] = v + } + + // Extract and parse known tools + if val, exists := toolsMap["github"]; exists { + tools.GitHub = parseGitHubTool(val) + } + if val, exists := toolsMap["bash"]; exists { + tools.Bash = parseBashTool(val) + } + if val, exists := toolsMap["web-fetch"]; exists { + tools.WebFetch = parseWebFetchTool(val) + } + if val, exists := toolsMap["web-search"]; exists { + tools.WebSearch = parseWebSearchTool(val) + } + if val, exists := toolsMap["edit"]; exists { + tools.Edit = parseEditTool(val) + } + if val, exists := toolsMap["playwright"]; exists { + tools.Playwright = parsePlaywrightTool(val) + } + if val, exists := toolsMap["serena"]; exists { + tools.Serena = parseSerenaTool(val) + } + if val, exists := toolsMap["agentic-workflows"]; exists { + tools.AgenticWorkflows = parseAgenticWorkflowsTool(val) + } + if val, exists := toolsMap["cache-memory"]; exists { + tools.CacheMemory = parseCacheMemoryTool(val) + } + if val, exists := toolsMap["repo-memory"]; exists { + tools.RepoMemory = parseRepoMemoryTool(val) + } + if val, exists := toolsMap["safety-prompt"]; exists { + tools.SafetyPrompt = parseSafetyPromptTool(val) + } + if val, exists := toolsMap["timeout"]; exists { + tools.Timeout = parseTimeoutTool(val) + } + if val, exists := toolsMap["startup-timeout"]; exists { + tools.StartupTimeout = parseStartupTimeoutTool(val) + } + + // Extract custom MCP tools (anything not in the known list) + knownTools := map[string]bool{ + "github": true, + "bash": true, + "web-fetch": true, + "web-search": true, + "edit": true, + "playwright": true, + "serena": true, + "agentic-workflows": true, + "cache-memory": true, + "repo-memory": true, + "safety-prompt": true, + "timeout": true, + "startup-timeout": true, + } + + customCount := 0 + for name, config := range toolsMap { + if !knownTools[name] { + tools.Custom[name] = config + customCount++ + } + } + + toolsParserLog.Printf("Parsed tools: github=%v, bash=%v, playwright=%v, serena=%v, custom=%d", tools.GitHub != nil, tools.Bash != nil, tools.Playwright != nil, tools.Serena != nil, customCount) + return tools +} + +// parseGitHubTool converts raw github tool configuration to GitHubToolConfig +func parseGitHubTool(val any) *GitHubToolConfig { + if val == nil { + toolsParserLog.Print("GitHub tool enabled with default configuration") + return &GitHubToolConfig{ + ReadOnly: true, // default to read-only for security + } + } + + // Handle string type (simple enable) + if _, ok := val.(string); ok { + toolsParserLog.Print("GitHub tool enabled with string configuration") + return &GitHubToolConfig{ + ReadOnly: true, // default to read-only for security + } + } + + // Handle map type (detailed configuration) + if configMap, ok := val.(map[string]any); ok { + toolsParserLog.Print("Parsing GitHub tool detailed configuration") + config := &GitHubToolConfig{ + ReadOnly: true, // default to read-only for security + } + + if allowed, ok := configMap["allowed"].([]any); ok { + config.Allowed = make([]string, 0, len(allowed)) + for _, item := range allowed { + if str, ok := item.(string); ok { + config.Allowed = append(config.Allowed, str) + } + } + } + + if mode, ok := configMap["mode"].(string); ok { + config.Mode = mode + } + + if version, ok := configMap["version"].(string); ok { + config.Version = version + } + + if args, ok := configMap["args"].([]any); ok { + config.Args = make([]string, 0, len(args)) + for _, item := range args { + if str, ok := item.(string); ok { + config.Args = append(config.Args, str) + } + } + } + + 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 + } + + // 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 { + config.Toolset = append(config.Toolset, str) + } + } + } + + if lockdown, ok := configMap["lockdown"].(bool); ok { + config.Lockdown = lockdown + } + + return config + } + + return &GitHubToolConfig{ + ReadOnly: true, // default to read-only for security + } +} + +// parseBashTool converts raw bash tool configuration to BashToolConfig +func parseBashTool(val any) *BashToolConfig { + if val == nil { + // nil means all commands allowed + return &BashToolConfig{} + } + + // Handle array of allowed commands + if cmdArray, ok := val.([]any); ok { + config := &BashToolConfig{ + AllowedCommands: make([]string, 0, len(cmdArray)), + } + for _, item := range cmdArray { + if str, ok := item.(string); ok { + config.AllowedCommands = append(config.AllowedCommands, str) + } + } + return config + } + + return &BashToolConfig{} +} + +// parsePlaywrightTool converts raw playwright tool configuration to PlaywrightToolConfig +func parsePlaywrightTool(val any) *PlaywrightToolConfig { + if val == nil { + return &PlaywrightToolConfig{} + } + + if configMap, ok := val.(map[string]any); ok { + config := &PlaywrightToolConfig{} + + if version, ok := configMap["version"].(string); ok { + config.Version = version + } + + // Handle allowed_domains - can be string or array + if allowedDomains, ok := configMap["allowed_domains"]; ok { + if str, ok := allowedDomains.(string); ok { + config.AllowedDomains = []string{str} + } else if arr, ok := allowedDomains.([]any); ok { + config.AllowedDomains = make([]string, 0, len(arr)) + for _, item := range arr { + if str, ok := item.(string); ok { + config.AllowedDomains = append(config.AllowedDomains, str) + } + } + } + } + + if args, ok := configMap["args"].([]any); ok { + config.Args = make([]string, 0, len(args)) + for _, item := range args { + if str, ok := item.(string); ok { + config.Args = append(config.Args, str) + } + } + } + + return config + } + + return &PlaywrightToolConfig{} +} + +// parseSerenaTool converts raw serena tool configuration to SerenaToolConfig +func parseSerenaTool(val any) *SerenaToolConfig { + if val == nil { + return &SerenaToolConfig{} + } + + // Handle array format (short syntax): ["go", "typescript"] + if langArray, ok := val.([]any); ok { + config := &SerenaToolConfig{ + ShortSyntax: make([]string, 0, len(langArray)), + } + for _, item := range langArray { + if str, ok := item.(string); ok { + config.ShortSyntax = append(config.ShortSyntax, str) + } + } + return config + } + + // Handle object format with detailed configuration + if configMap, ok := val.(map[string]any); ok { + config := &SerenaToolConfig{} + + if version, ok := configMap["version"].(string); ok { + config.Version = version + } + + if args, ok := configMap["args"].([]any); ok { + config.Args = make([]string, 0, len(args)) + for _, item := range args { + if str, ok := item.(string); ok { + config.Args = append(config.Args, str) + } + } + } + + // Parse languages configuration + if languagesVal, ok := configMap["languages"].(map[string]any); ok { + config.Languages = make(map[string]*SerenaLangConfig) + for langName, langVal := range languagesVal { + if langVal == nil { + // nil means enable with defaults + config.Languages[langName] = &SerenaLangConfig{} + continue + } + if langMap, ok := langVal.(map[string]any); ok { + langConfig := &SerenaLangConfig{} + if version, ok := langMap["version"].(string); ok { + langConfig.Version = version + } else if versionNum, ok := langMap["version"].(float64); ok { + // Convert numeric version to string + langConfig.Version = fmt.Sprintf("%.0f", versionNum) + } + // Parse Go-specific fields + if langName == "go" { + if goModFile, ok := langMap["go-mod-file"].(string); ok { + langConfig.GoModFile = goModFile + } + if goplsVersion, ok := langMap["gopls-version"].(string); ok { + langConfig.GoplsVersion = goplsVersion + } + } + config.Languages[langName] = langConfig + } + } + } + + return config + } + + return &SerenaToolConfig{} +} + +// parseWebFetchTool converts raw web-fetch tool configuration +func parseWebFetchTool(val any) *WebFetchToolConfig { + // web-fetch is either nil or an empty object + return &WebFetchToolConfig{} +} + +// parseWebSearchTool converts raw web-search tool configuration +func parseWebSearchTool(val any) *WebSearchToolConfig { + // web-search is either nil or an empty object + return &WebSearchToolConfig{} +} + +// parseEditTool converts raw edit tool configuration +func parseEditTool(val any) *EditToolConfig { + // edit is either nil or an empty object + return &EditToolConfig{} +} + +// parseAgenticWorkflowsTool converts raw agentic-workflows tool configuration +func parseAgenticWorkflowsTool(val any) *AgenticWorkflowsToolConfig { + config := &AgenticWorkflowsToolConfig{} + + if boolVal, ok := val.(bool); ok { + config.Enabled = boolVal + } else if val == nil { + config.Enabled = true // nil means enabled + } + + return config +} + +// parseCacheMemoryTool converts raw cache-memory tool configuration +func parseCacheMemoryTool(val any) *CacheMemoryToolConfig { + // cache-memory can be boolean, object, or array - store raw value + return &CacheMemoryToolConfig{Raw: val} +} + +// parseRepoMemoryTool converts raw repo-memory tool configuration +func parseRepoMemoryTool(val any) *RepoMemoryToolConfig { + // repo-memory can be boolean, object, or array - store raw value + return &RepoMemoryToolConfig{Raw: val} +} + +// parseMCPGatewayTool converts raw mcp-gateway tool configuration +func parseMCPGatewayTool(val any) *MCPGatewayConfig { + if val == nil { + return nil + } + + configMap, ok := val.(map[string]any) + if !ok { + return nil + } + + config := &MCPGatewayConfig{ + Port: DefaultMCPGatewayPort, + } + + if container, ok := configMap["container"].(string); ok { + config.Container = container + } + if version, ok := configMap["version"].(string); ok { + config.Version = version + } else if versionNum, ok := configMap["version"].(float64); ok { + config.Version = fmt.Sprintf("%.0f", versionNum) + } + if args, ok := configMap["args"].([]any); ok { + config.Args = make([]string, 0, len(args)) + for _, arg := range args { + if str, ok := arg.(string); ok { + config.Args = append(config.Args, str) + } + } + } + if entrypointArgs, ok := configMap["entrypointArgs"].([]any); ok { + config.EntrypointArgs = make([]string, 0, len(entrypointArgs)) + for _, arg := range entrypointArgs { + if str, ok := arg.(string); ok { + config.EntrypointArgs = append(config.EntrypointArgs, str) + } + } + } + if env, ok := configMap["env"].(map[string]any); ok { + config.Env = make(map[string]string) + for k, v := range env { + if str, ok := v.(string); ok { + config.Env[k] = str + } + } + } + if port, ok := configMap["port"].(int); ok { + config.Port = port + } else if portFloat, ok := configMap["port"].(float64); ok { + config.Port = int(portFloat) + } + if apiKey, ok := configMap["api-key"].(string); ok { + config.APIKey = apiKey + } + + return config +} + +// parseSafetyPromptTool converts raw safety-prompt tool configuration +func parseSafetyPromptTool(val any) *bool { + if boolVal, ok := val.(bool); ok { + return &boolVal + } + // Default to true if not specified or invalid type + defaultVal := true + return &defaultVal +} + +// parseTimeoutTool converts raw timeout tool configuration +func parseTimeoutTool(val any) *int { + if intVal, ok := val.(int); ok { + return &intVal + } + if floatVal, ok := val.(float64); ok { + intVal := int(floatVal) + return &intVal + } + return nil +} + +// parseStartupTimeoutTool converts raw startup-timeout tool configuration +func parseStartupTimeoutTool(val any) *int { + if intVal, ok := val.(int); ok { + return &intVal + } + if floatVal, ok := val.(float64); ok { + intVal := int(floatVal) + return &intVal + } + return nil +} diff --git a/pkg/workflow/tools_types.go b/pkg/workflow/tools_types.go index 8538cc794f..127d0d97d3 100644 --- a/pkg/workflow/tools_types.go +++ b/pkg/workflow/tools_types.go @@ -1,8 +1,6 @@ package workflow import ( - "fmt" - "github.com/githubnext/gh-aw/pkg/logger" ) @@ -250,457 +248,7 @@ type MCPGatewayConfig struct { APIKey string `yaml:"api-key,omitempty"` // API key for gateway authentication } -// NewTools creates a new Tools instance from a map -func NewTools(toolsMap map[string]any) *Tools { - toolsTypesLog.Printf("Creating tools configuration from map with %d entries", len(toolsMap)) - if toolsMap == nil { - return &Tools{ - Custom: make(map[string]any), - raw: make(map[string]any), - } - } - - tools := &Tools{ - Custom: make(map[string]any), - raw: make(map[string]any), - } - - // Copy raw map - for k, v := range toolsMap { - tools.raw[k] = v - } - - // Extract and parse known tools - if val, exists := toolsMap["github"]; exists { - tools.GitHub = parseGitHubTool(val) - } - if val, exists := toolsMap["bash"]; exists { - tools.Bash = parseBashTool(val) - } - if val, exists := toolsMap["web-fetch"]; exists { - tools.WebFetch = parseWebFetchTool(val) - } - if val, exists := toolsMap["web-search"]; exists { - tools.WebSearch = parseWebSearchTool(val) - } - if val, exists := toolsMap["edit"]; exists { - tools.Edit = parseEditTool(val) - } - if val, exists := toolsMap["playwright"]; exists { - tools.Playwright = parsePlaywrightTool(val) - } - if val, exists := toolsMap["serena"]; exists { - tools.Serena = parseSerenaTool(val) - } - if val, exists := toolsMap["agentic-workflows"]; exists { - tools.AgenticWorkflows = parseAgenticWorkflowsTool(val) - } - if val, exists := toolsMap["cache-memory"]; exists { - tools.CacheMemory = parseCacheMemoryTool(val) - } - if val, exists := toolsMap["repo-memory"]; exists { - tools.RepoMemory = parseRepoMemoryTool(val) - } - if val, exists := toolsMap["safety-prompt"]; exists { - tools.SafetyPrompt = parseSafetyPromptTool(val) - } - if val, exists := toolsMap["timeout"]; exists { - tools.Timeout = parseTimeoutTool(val) - } - if val, exists := toolsMap["startup-timeout"]; exists { - tools.StartupTimeout = parseStartupTimeoutTool(val) - } - - // Extract custom MCP tools (anything not in the known list) - knownTools := map[string]bool{ - "github": true, - "bash": true, - "web-fetch": true, - "web-search": true, - "edit": true, - "playwright": true, - "serena": true, - "agentic-workflows": true, - "cache-memory": true, - "repo-memory": true, - "safety-prompt": true, - "timeout": true, - "startup-timeout": true, - } - - customCount := 0 - for name, config := range toolsMap { - if !knownTools[name] { - tools.Custom[name] = config - customCount++ - } - } - - toolsTypesLog.Printf("Parsed tools: github=%v, bash=%v, playwright=%v, serena=%v, custom=%d", tools.GitHub != nil, tools.Bash != nil, tools.Playwright != nil, tools.Serena != nil, customCount) - return tools -} - -// parseGitHubTool converts raw github tool configuration to GitHubToolConfig -func parseGitHubTool(val any) *GitHubToolConfig { - if val == nil { - toolsTypesLog.Print("GitHub tool enabled with default configuration") - 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{ - 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{ - ReadOnly: true, // default to read-only for security - } - - if allowed, ok := configMap["allowed"].([]any); ok { - config.Allowed = make([]string, 0, len(allowed)) - for _, item := range allowed { - if str, ok := item.(string); ok { - config.Allowed = append(config.Allowed, str) - } - } - } - - if mode, ok := configMap["mode"].(string); ok { - config.Mode = mode - } - - if version, ok := configMap["version"].(string); ok { - config.Version = version - } - - if args, ok := configMap["args"].([]any); ok { - config.Args = make([]string, 0, len(args)) - for _, item := range args { - if str, ok := item.(string); ok { - config.Args = append(config.Args, str) - } - } - } - - 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 - } - - // 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 { - config.Toolset = append(config.Toolset, str) - } - } - } - - if lockdown, ok := configMap["lockdown"].(bool); ok { - config.Lockdown = lockdown - } - - return config - } - - return &GitHubToolConfig{ - ReadOnly: true, // default to read-only for security - } -} - -// parseBashTool converts raw bash tool configuration to BashToolConfig -func parseBashTool(val any) *BashToolConfig { - if val == nil { - // nil means all commands allowed - return &BashToolConfig{} - } - - // Handle array of allowed commands - if cmdArray, ok := val.([]any); ok { - config := &BashToolConfig{ - AllowedCommands: make([]string, 0, len(cmdArray)), - } - for _, item := range cmdArray { - if str, ok := item.(string); ok { - config.AllowedCommands = append(config.AllowedCommands, str) - } - } - return config - } - - return &BashToolConfig{} -} - -// parsePlaywrightTool converts raw playwright tool configuration to PlaywrightToolConfig -func parsePlaywrightTool(val any) *PlaywrightToolConfig { - if val == nil { - return &PlaywrightToolConfig{} - } - - if configMap, ok := val.(map[string]any); ok { - config := &PlaywrightToolConfig{} - - if version, ok := configMap["version"].(string); ok { - config.Version = version - } - // Handle allowed_domains - can be string or array - if allowedDomains, ok := configMap["allowed_domains"]; ok { - if str, ok := allowedDomains.(string); ok { - config.AllowedDomains = []string{str} - } else if arr, ok := allowedDomains.([]any); ok { - config.AllowedDomains = make([]string, 0, len(arr)) - for _, item := range arr { - if str, ok := item.(string); ok { - config.AllowedDomains = append(config.AllowedDomains, str) - } - } - } - } - - if args, ok := configMap["args"].([]any); ok { - config.Args = make([]string, 0, len(args)) - for _, item := range args { - if str, ok := item.(string); ok { - config.Args = append(config.Args, str) - } - } - } - - return config - } - - return &PlaywrightToolConfig{} -} - -// parseSerenaTool converts raw serena tool configuration to SerenaToolConfig -func parseSerenaTool(val any) *SerenaToolConfig { - if val == nil { - return &SerenaToolConfig{} - } - - // Handle array format (short syntax): ["go", "typescript"] - if langArray, ok := val.([]any); ok { - config := &SerenaToolConfig{ - ShortSyntax: make([]string, 0, len(langArray)), - } - for _, item := range langArray { - if str, ok := item.(string); ok { - config.ShortSyntax = append(config.ShortSyntax, str) - } - } - return config - } - - // Handle object format with detailed configuration - if configMap, ok := val.(map[string]any); ok { - config := &SerenaToolConfig{} - - if version, ok := configMap["version"].(string); ok { - config.Version = version - } - - if args, ok := configMap["args"].([]any); ok { - config.Args = make([]string, 0, len(args)) - for _, item := range args { - if str, ok := item.(string); ok { - config.Args = append(config.Args, str) - } - } - } - - // Parse languages configuration - if languagesVal, ok := configMap["languages"].(map[string]any); ok { - config.Languages = make(map[string]*SerenaLangConfig) - for langName, langVal := range languagesVal { - if langVal == nil { - // nil means enable with defaults - config.Languages[langName] = &SerenaLangConfig{} - continue - } - if langMap, ok := langVal.(map[string]any); ok { - langConfig := &SerenaLangConfig{} - if version, ok := langMap["version"].(string); ok { - langConfig.Version = version - } else if versionNum, ok := langMap["version"].(float64); ok { - // Convert numeric version to string - langConfig.Version = fmt.Sprintf("%.0f", versionNum) - } - // Parse Go-specific fields - if langName == "go" { - if goModFile, ok := langMap["go-mod-file"].(string); ok { - langConfig.GoModFile = goModFile - } - if goplsVersion, ok := langMap["gopls-version"].(string); ok { - langConfig.GoplsVersion = goplsVersion - } - } - config.Languages[langName] = langConfig - } - } - } - - return config - } - - return &SerenaToolConfig{} -} - -// parseWebFetchTool converts raw web-fetch tool configuration -func parseWebFetchTool(val any) *WebFetchToolConfig { - // web-fetch is either nil or an empty object - return &WebFetchToolConfig{} -} - -// parseWebSearchTool converts raw web-search tool configuration -func parseWebSearchTool(val any) *WebSearchToolConfig { - // web-search is either nil or an empty object - return &WebSearchToolConfig{} -} - -// parseEditTool converts raw edit tool configuration -func parseEditTool(val any) *EditToolConfig { - // edit is either nil or an empty object - return &EditToolConfig{} -} - -// parseAgenticWorkflowsTool converts raw agentic-workflows tool configuration -func parseAgenticWorkflowsTool(val any) *AgenticWorkflowsToolConfig { - config := &AgenticWorkflowsToolConfig{} - - if boolVal, ok := val.(bool); ok { - config.Enabled = boolVal - } else if val == nil { - config.Enabled = true // nil means enabled - } - - return config -} - -// parseCacheMemoryTool converts raw cache-memory tool configuration -func parseCacheMemoryTool(val any) *CacheMemoryToolConfig { - // cache-memory can be boolean, object, or array - store raw value - return &CacheMemoryToolConfig{Raw: val} -} - -// parseRepoMemoryTool converts raw repo-memory tool configuration -func parseRepoMemoryTool(val any) *RepoMemoryToolConfig { - // repo-memory can be boolean, object, or array - store raw value - return &RepoMemoryToolConfig{Raw: val} -} - -// parseMCPGatewayTool converts raw mcp-gateway tool configuration -func parseMCPGatewayTool(val any) *MCPGatewayConfig { - if val == nil { - return nil - } - - configMap, ok := val.(map[string]any) - if !ok { - return nil - } - - config := &MCPGatewayConfig{ - Port: DefaultMCPGatewayPort, - } - - if container, ok := configMap["container"].(string); ok { - config.Container = container - } - if version, ok := configMap["version"].(string); ok { - config.Version = version - } else if versionNum, ok := configMap["version"].(float64); ok { - config.Version = fmt.Sprintf("%.0f", versionNum) - } - if args, ok := configMap["args"].([]any); ok { - config.Args = make([]string, 0, len(args)) - for _, arg := range args { - if str, ok := arg.(string); ok { - config.Args = append(config.Args, str) - } - } - } - if entrypointArgs, ok := configMap["entrypointArgs"].([]any); ok { - config.EntrypointArgs = make([]string, 0, len(entrypointArgs)) - for _, arg := range entrypointArgs { - if str, ok := arg.(string); ok { - config.EntrypointArgs = append(config.EntrypointArgs, str) - } - } - } - if env, ok := configMap["env"].(map[string]any); ok { - config.Env = make(map[string]string) - for k, v := range env { - if str, ok := v.(string); ok { - config.Env[k] = str - } - } - } - if port, ok := configMap["port"].(int); ok { - config.Port = port - } else if portFloat, ok := configMap["port"].(float64); ok { - config.Port = int(portFloat) - } - if apiKey, ok := configMap["api-key"].(string); ok { - config.APIKey = apiKey - } - - return config -} - -// parseSafetyPromptTool converts raw safety-prompt tool configuration -func parseSafetyPromptTool(val any) *bool { - if boolVal, ok := val.(bool); ok { - return &boolVal - } - // Default to true if not specified or invalid type - defaultVal := true - return &defaultVal -} - -// parseTimeoutTool converts raw timeout tool configuration -func parseTimeoutTool(val any) *int { - if intVal, ok := val.(int); ok { - return &intVal - } - if floatVal, ok := val.(float64); ok { - intVal := int(floatVal) - return &intVal - } - return nil -} - -// parseStartupTimeoutTool converts raw startup-timeout tool configuration -func parseStartupTimeoutTool(val any) *int { - if intVal, ok := val.(int); ok { - return &intVal - } - if floatVal, ok := val.(float64); ok { - intVal := int(floatVal) - return &intVal - } - return nil -} // HasTool checks if a tool is present in the configuration func (t *Tools) HasTool(name string) bool { From eeb02c2b4f4c8a58cec298cba22d7ff58951b89a Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 19 Dec 2025 07:13:15 +0000 Subject: [PATCH 3/5] fix: remove unused logger variable in tools_types.go The toolsTypesLog variable is no longer needed after moving parsing functions to tools_parser.go Co-authored-by: mnkiefer <8320933+mnkiefer@users.noreply.github.com> --- pkg/cli/fix_command_test.go | 84 ++++++++++++++++++------------------- pkg/workflow/tools_types.go | 8 ---- 2 files changed, 42 insertions(+), 50 deletions(-) diff --git a/pkg/cli/fix_command_test.go b/pkg/cli/fix_command_test.go index 7e75b112b0..2a34cd4be6 100644 --- a/pkg/cli/fix_command_test.go +++ b/pkg/cli/fix_command_test.go @@ -542,12 +542,12 @@ This is a test workflow with slash command. } func TestFixCommand_SafeInputsModeRemoval(t *testing.T) { -// Create a temporary directory for test files -tmpDir := t.TempDir() -workflowFile := filepath.Join(tmpDir, "test-workflow.md") + // Create a temporary directory for test files + tmpDir := t.TempDir() + workflowFile := filepath.Join(tmpDir, "test-workflow.md") -// Create a workflow with deprecated safe-inputs.mode field -content := `--- + // Create a workflow with deprecated safe-inputs.mode field + content := `--- on: workflow_dispatch engine: copilot safe-inputs: @@ -563,51 +563,51 @@ safe-inputs: This is a test workflow with safe-inputs mode field. ` -if err := os.WriteFile(workflowFile, []byte(content), 0644); err != nil { -t.Fatalf("Failed to create test file: %v", err) -} + if err := os.WriteFile(workflowFile, []byte(content), 0644); err != nil { + t.Fatalf("Failed to create test file: %v", err) + } -// Get the safe-inputs mode removal codemod -modeCodemod := getCodemodByID("safe-inputs-mode-removal") -if modeCodemod == nil { -t.Fatal("safe-inputs-mode-removal codemod not found") -} + // Get the safe-inputs mode removal codemod + modeCodemod := getCodemodByID("safe-inputs-mode-removal") + if modeCodemod == nil { + t.Fatal("safe-inputs-mode-removal codemod not found") + } -// Process the file -fixed, err := processWorkflowFile(workflowFile, []Codemod{*modeCodemod}, true, false) -if err != nil { -t.Fatalf("Failed to process workflow file: %v", err) -} + // Process the file + fixed, err := processWorkflowFile(workflowFile, []Codemod{*modeCodemod}, true, false) + if err != nil { + t.Fatalf("Failed to process workflow file: %v", err) + } -if !fixed { -t.Error("Expected file to be fixed, but no changes were made") -} + if !fixed { + t.Error("Expected file to be fixed, but no changes were made") + } -// Read the updated content -updatedContent, err := os.ReadFile(workflowFile) -if err != nil { -t.Fatalf("Failed to read updated file: %v", err) -} + // Read the updated content + updatedContent, err := os.ReadFile(workflowFile) + if err != nil { + t.Fatalf("Failed to read updated file: %v", err) + } -updatedStr := string(updatedContent) + updatedStr := string(updatedContent) -t.Logf("Updated content:\n%s", updatedStr) + t.Logf("Updated content:\n%s", updatedStr) -// Verify the change - mode field should be removed -if strings.Contains(updatedStr, "mode:") { -t.Errorf("Expected mode field to be removed, but it still exists:\n%s", updatedStr) -} + // Verify the change - mode field should be removed + if strings.Contains(updatedStr, "mode:") { + t.Errorf("Expected mode field to be removed, but it still exists:\n%s", updatedStr) + } -// Verify safe-inputs block and test-tool are preserved -if !strings.Contains(updatedStr, "safe-inputs:") { -t.Error("Expected safe-inputs block to be preserved") -} + // Verify safe-inputs block and test-tool are preserved + if !strings.Contains(updatedStr, "safe-inputs:") { + t.Error("Expected safe-inputs block to be preserved") + } -if !strings.Contains(updatedStr, "test-tool:") { -t.Error("Expected test-tool to be preserved") -} + if !strings.Contains(updatedStr, "test-tool:") { + t.Error("Expected test-tool to be preserved") + } -if !strings.Contains(updatedStr, "description: Test tool") { -t.Error("Expected test-tool description to be preserved") -} + if !strings.Contains(updatedStr, "description: Test tool") { + t.Error("Expected test-tool description to be preserved") + } } diff --git a/pkg/workflow/tools_types.go b/pkg/workflow/tools_types.go index 127d0d97d3..5138ef3d66 100644 --- a/pkg/workflow/tools_types.go +++ b/pkg/workflow/tools_types.go @@ -1,11 +1,5 @@ package workflow -import ( - "github.com/githubnext/gh-aw/pkg/logger" -) - -var toolsTypesLog = logger.New("workflow:tools_types") - // ToolsConfig represents the unified configuration for all tools in a workflow. // This type provides a structured alternative to the pervasive map[string]any pattern. // It includes strongly-typed fields for built-in tools and a flexible Custom map for @@ -248,8 +242,6 @@ type MCPGatewayConfig struct { APIKey string `yaml:"api-key,omitempty"` // API key for gateway authentication } - - // HasTool checks if a tool is present in the configuration func (t *Tools) HasTool(name string) bool { if t == nil { From a12c3ac07bc7eafbb01121520f7761b958397661 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 19 Dec 2025 07:16:46 +0000 Subject: [PATCH 4/5] docs: add refactoring analysis documenting decisions Documents analysis of semantic function clustering recommendations, explaining which changes were beneficial and which were not needed. Co-authored-by: mnkiefer <8320933+mnkiefer@users.noreply.github.com> --- REFACTORING_ANALYSIS.md | 168 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 168 insertions(+) create mode 100644 REFACTORING_ANALYSIS.md diff --git a/REFACTORING_ANALYSIS.md b/REFACTORING_ANALYSIS.md new file mode 100644 index 0000000000..80df1c48e4 --- /dev/null +++ b/REFACTORING_ANALYSIS.md @@ -0,0 +1,168 @@ +# Semantic Function Clustering Refactoring - Analysis Results + +## Summary + +This document provides analysis results from the semantic function clustering refactoring initiative. The goal was to improve code organization by identifying and addressing function clustering opportunities. + +## Changes Implemented + +### ✅ Task 2: Split `tools_types.go` (Issue #1A) + +**Status**: Completed + +**Problem**: The file `tools_types.go` contained both type definitions (67 lines) and parsing functions (450+ lines), making the filename misleading. + +**Solution**: +- Created `tools_parser.go` with all `parse*` functions and `NewTools()` +- Kept `tools_types.go` with only type definitions and helper methods (`ToMap`, `HasTool`, `GetToolNames`) + +**Result**: +- Clear separation between data structures and parsing logic +- File names now accurately reflect their contents +- All tests pass, linter is clean + +## Changes NOT Recommended + +### ❌ Task 1: Consolidate Entity Helpers (Issue #1B) + +**Status**: Not needed - Analysis found issue description incorrect + +**Analysis**: +- `close_entity_helpers.go` and `update_entity_helpers.go` are NOT duplicate code +- They use **intentional generic patterns** with registry-based architecture +- `parseCloseEntityConfig()` and `parseUpdateEntityConfig()` are designed as reusable generic functions +- The registry pattern (`closeEntityRegistry`, `closeEntityDefinition`) is a well-established design pattern + +**Conclusion**: These files demonstrate good software engineering with: +- Generic helper patterns +- Registry-based configuration +- Type-safe abstractions +- Clear separation of concerns + +**No changes needed.** + +### ❌ Task 3: Split `js.go` (Issue #2A) + +**Status**: Not recommended - Would reduce cohesion + +**Analysis**: +- File has 914 lines with 79 `go:embed` directives +- Primary purpose: **Embedded JavaScript file management** +- 41 functions breakdown: + - ~22 getter functions (simple wrappers for embedded scripts) + - ~10 comment removal/parsing utilities + - ~3 YAML formatting utilities + - Helper functions + +**Why NOT to split**: +1. **Semantic cohesion**: All code relates to JavaScript file handling +2. **Embedded files**: The `go:embed` directives bind the file to its purpose +3. **Dependencies**: Splitting would create circular dependencies or awkward imports +4. **Discoverability**: One file = one place to find all JavaScript-related code +5. **Current organization works**: Script registry pattern already provides good organization + +**Conclusion**: The file is appropriately organized around its core responsibility: managing embedded JavaScript files. Comment removal and YAML formatting are supporting utilities for this core purpose. + +**No changes needed.** + +### ❌ Task 4: Reorganize `scripts.go` (Issue #2B) + +**Status**: Not recommended - Already well-organized + +**Analysis**: +- File has 397 lines with similar structure to `js.go` +- Uses **Script Registry Pattern** (see `script_registry.go`) +- Primary purpose: Register embedded JavaScript scripts for lazy loading +- Structure: + - `go:embed` directives for script sources + - `init()` function to register scripts + - Getter functions (simple wrappers) + +**Why NOT to reorganize**: +1. **Already uses registry pattern**: The `DefaultScriptRegistry` provides organized access +2. **Flat is better**: All scripts in one file makes them easy to find +3. **Lazy loading**: Registry handles on-demand bundling automatically +4. **Consistent with js.go**: Both files use same organizational pattern + +**Conclusion**: The current organization with the script registry pattern is a good architectural choice. Splitting into subdirectories would: +- Break the registry pattern +- Make imports more complex +- Reduce discoverability +- Add no real value + +**No changes needed.** + +## Architectural Patterns Observed + +### 1. Script Registry Pattern +Files like `js.go` and `scripts.go` use a centralized registry (`DefaultScriptRegistry`) for managing embedded scripts. This pattern provides: +- Lazy bundling of scripts +- Centralized script management +- Consistent access patterns +- Easy testing and mocking + +### 2. Generic Helper Pattern +Files like `close_entity_helpers.go` use generic functions with registry-based configuration: +- `parseCloseEntityConfig()` handles multiple entity types +- Registry pattern (`closeEntityRegistry`) defines entity-specific behavior +- Type-safe abstractions prevent code duplication + +### 3. Embedded File Pattern +The `go:embed` directive pattern used throughout: +- Embeds JavaScript files at compile time +- Provides type-safe access to file contents +- Groups related files logically + +## Recommendations + +### For Future Development + +1. **Keep using existing patterns**: The script registry and generic helper patterns are working well + +2. **File size guidelines**: + - Files < 1000 lines are generally acceptable if semantically cohesive + - Focus on semantic cohesion over line count + - Consider splitting when files have multiple unrelated responsibilities + +3. **Naming conventions**: + - File names should reflect primary purpose + - `tools_types.go` vs `tools_parser.go` is a good example + - Avoid generic names like `utils.go` or `helpers.go` + +4. **Registry patterns**: + - Continue using for managing collections of similar items + - Provides better scalability than individual files + - Easier to maintain and test + +### What WAS Actually Valuable + +The **only change that improved code organization** was: +- ✅ Splitting `tools_types.go` into types and parser files + +This change: +- Made file names accurately reflect contents +- Separated data structures from parsing logic +- Maintained all functionality without breaking changes + +## Lessons Learned + +1. **Semantic analysis tools can be overly aggressive**: Not all "function clustering" issues are real problems +2. **Context matters**: File size alone doesn't indicate poor organization +3. **Patterns matter more than metrics**: Well-designed patterns (registry, generic helpers) are more valuable than arbitrary function grouping +4. **Cohesion trumps size**: A 900-line file with strong semantic cohesion is better than splitting into multiple files with weak cohesion + +## Conclusion + +The codebase is **already well-organized** with strong architectural patterns: +- Script registry pattern for embedded JavaScript +- Generic helper patterns with registries +- Clear separation of concerns +- Type-safe abstractions + +**Only 1 out of 4 recommended changes** was actually beneficial. The other files were already well-organized and splitting them would have reduced code quality. + +--- + +**Date**: 2025-12-19 +**Author**: GitHub Copilot +**Issue**: githubnext/gh-aw - Semantic Function Clustering Analysis From 41421dc3907df58ec34ac642f243ce21644acddd Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 19 Dec 2025 12:53:55 +0000 Subject: [PATCH 5/5] docs: remove REFACTORING_ANALYSIS.md as requested Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- REFACTORING_ANALYSIS.md | 168 ---------------------------------------- 1 file changed, 168 deletions(-) delete mode 100644 REFACTORING_ANALYSIS.md diff --git a/REFACTORING_ANALYSIS.md b/REFACTORING_ANALYSIS.md deleted file mode 100644 index 80df1c48e4..0000000000 --- a/REFACTORING_ANALYSIS.md +++ /dev/null @@ -1,168 +0,0 @@ -# Semantic Function Clustering Refactoring - Analysis Results - -## Summary - -This document provides analysis results from the semantic function clustering refactoring initiative. The goal was to improve code organization by identifying and addressing function clustering opportunities. - -## Changes Implemented - -### ✅ Task 2: Split `tools_types.go` (Issue #1A) - -**Status**: Completed - -**Problem**: The file `tools_types.go` contained both type definitions (67 lines) and parsing functions (450+ lines), making the filename misleading. - -**Solution**: -- Created `tools_parser.go` with all `parse*` functions and `NewTools()` -- Kept `tools_types.go` with only type definitions and helper methods (`ToMap`, `HasTool`, `GetToolNames`) - -**Result**: -- Clear separation between data structures and parsing logic -- File names now accurately reflect their contents -- All tests pass, linter is clean - -## Changes NOT Recommended - -### ❌ Task 1: Consolidate Entity Helpers (Issue #1B) - -**Status**: Not needed - Analysis found issue description incorrect - -**Analysis**: -- `close_entity_helpers.go` and `update_entity_helpers.go` are NOT duplicate code -- They use **intentional generic patterns** with registry-based architecture -- `parseCloseEntityConfig()` and `parseUpdateEntityConfig()` are designed as reusable generic functions -- The registry pattern (`closeEntityRegistry`, `closeEntityDefinition`) is a well-established design pattern - -**Conclusion**: These files demonstrate good software engineering with: -- Generic helper patterns -- Registry-based configuration -- Type-safe abstractions -- Clear separation of concerns - -**No changes needed.** - -### ❌ Task 3: Split `js.go` (Issue #2A) - -**Status**: Not recommended - Would reduce cohesion - -**Analysis**: -- File has 914 lines with 79 `go:embed` directives -- Primary purpose: **Embedded JavaScript file management** -- 41 functions breakdown: - - ~22 getter functions (simple wrappers for embedded scripts) - - ~10 comment removal/parsing utilities - - ~3 YAML formatting utilities - - Helper functions - -**Why NOT to split**: -1. **Semantic cohesion**: All code relates to JavaScript file handling -2. **Embedded files**: The `go:embed` directives bind the file to its purpose -3. **Dependencies**: Splitting would create circular dependencies or awkward imports -4. **Discoverability**: One file = one place to find all JavaScript-related code -5. **Current organization works**: Script registry pattern already provides good organization - -**Conclusion**: The file is appropriately organized around its core responsibility: managing embedded JavaScript files. Comment removal and YAML formatting are supporting utilities for this core purpose. - -**No changes needed.** - -### ❌ Task 4: Reorganize `scripts.go` (Issue #2B) - -**Status**: Not recommended - Already well-organized - -**Analysis**: -- File has 397 lines with similar structure to `js.go` -- Uses **Script Registry Pattern** (see `script_registry.go`) -- Primary purpose: Register embedded JavaScript scripts for lazy loading -- Structure: - - `go:embed` directives for script sources - - `init()` function to register scripts - - Getter functions (simple wrappers) - -**Why NOT to reorganize**: -1. **Already uses registry pattern**: The `DefaultScriptRegistry` provides organized access -2. **Flat is better**: All scripts in one file makes them easy to find -3. **Lazy loading**: Registry handles on-demand bundling automatically -4. **Consistent with js.go**: Both files use same organizational pattern - -**Conclusion**: The current organization with the script registry pattern is a good architectural choice. Splitting into subdirectories would: -- Break the registry pattern -- Make imports more complex -- Reduce discoverability -- Add no real value - -**No changes needed.** - -## Architectural Patterns Observed - -### 1. Script Registry Pattern -Files like `js.go` and `scripts.go` use a centralized registry (`DefaultScriptRegistry`) for managing embedded scripts. This pattern provides: -- Lazy bundling of scripts -- Centralized script management -- Consistent access patterns -- Easy testing and mocking - -### 2. Generic Helper Pattern -Files like `close_entity_helpers.go` use generic functions with registry-based configuration: -- `parseCloseEntityConfig()` handles multiple entity types -- Registry pattern (`closeEntityRegistry`) defines entity-specific behavior -- Type-safe abstractions prevent code duplication - -### 3. Embedded File Pattern -The `go:embed` directive pattern used throughout: -- Embeds JavaScript files at compile time -- Provides type-safe access to file contents -- Groups related files logically - -## Recommendations - -### For Future Development - -1. **Keep using existing patterns**: The script registry and generic helper patterns are working well - -2. **File size guidelines**: - - Files < 1000 lines are generally acceptable if semantically cohesive - - Focus on semantic cohesion over line count - - Consider splitting when files have multiple unrelated responsibilities - -3. **Naming conventions**: - - File names should reflect primary purpose - - `tools_types.go` vs `tools_parser.go` is a good example - - Avoid generic names like `utils.go` or `helpers.go` - -4. **Registry patterns**: - - Continue using for managing collections of similar items - - Provides better scalability than individual files - - Easier to maintain and test - -### What WAS Actually Valuable - -The **only change that improved code organization** was: -- ✅ Splitting `tools_types.go` into types and parser files - -This change: -- Made file names accurately reflect contents -- Separated data structures from parsing logic -- Maintained all functionality without breaking changes - -## Lessons Learned - -1. **Semantic analysis tools can be overly aggressive**: Not all "function clustering" issues are real problems -2. **Context matters**: File size alone doesn't indicate poor organization -3. **Patterns matter more than metrics**: Well-designed patterns (registry, generic helpers) are more valuable than arbitrary function grouping -4. **Cohesion trumps size**: A 900-line file with strong semantic cohesion is better than splitting into multiple files with weak cohesion - -## Conclusion - -The codebase is **already well-organized** with strong architectural patterns: -- Script registry pattern for embedded JavaScript -- Generic helper patterns with registries -- Clear separation of concerns -- Type-safe abstractions - -**Only 1 out of 4 recommended changes** was actually beneficial. The other files were already well-organized and splitting them would have reduced code quality. - ---- - -**Date**: 2025-12-19 -**Author**: GitHub Copilot -**Issue**: githubnext/gh-aw - Semantic Function Clustering Analysis