From fe5f6cf6ce60e5241d83fa4c0573377d36c3cfd3 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 29 May 2026 23:03:06 +0000 Subject: [PATCH 1/7] Initial plan From 8372404992c63c0dccbd31a280028171bfd66457 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 29 May 2026 23:08:24 +0000 Subject: [PATCH 2/7] chore: start lint complexity refactor plan Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com> --- .github/workflows/avenger.lock.yml | 2 +- .github/workflows/step-name-alignment.lock.yml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/avenger.lock.yml b/.github/workflows/avenger.lock.yml index d290ebc74ce..d6f76460cb2 100644 --- a/.github/workflows/avenger.lock.yml +++ b/.github/workflows/avenger.lock.yml @@ -1,5 +1,5 @@ # gh-aw-metadata: {"schema_version":"v4","frontmatter_hash":"c9ca8d9435dba91e4b9c57a962cf751815e09437a33fd04f4b53374d95d0dcea","body_hash":"4d51ef429f578f6a81277bb5b7fb6972093ac204c21c50b06168c848812076b4","strict":true,"agent_id":"claude"} -# gh-aw-manifest: {"version":1,"secrets":["ANTHROPIC_API_KEY","GH_AW_CI_TRIGGER_TOKEN","GH_AW_GITHUB_MCP_SERVER_TOKEN","GH_AW_GITHUB_TOKEN","GH_AW_OTEL_GRAFANA_AUTHORIZATION","GH_AW_OTEL_GRAFANA_ENDPOINT","GH_AW_OTEL_SENTRY_AUTHORIZATION","GH_AW_OTEL_SENTRY_ENDPOINT","GITHUB_TOKEN"],"actions":[{"repo":"actions/checkout","sha":"de0fac2e4500dabe0009e67214ff5f5447ce83dd","version":"v6.0.2"},{"repo":"actions/download-artifact","sha":"3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c","version":"v8.0.1"},{"repo":"actions/github-script","sha":"3a2844b7e9c422d3c10d287c895573f7108da1b3","version":"v9.0.0"},{"repo":"actions/setup-go","sha":"4a3601121dd01d1626a1e23e37211e3254c1c06c","version":"v6.4.0"},{"repo":"actions/setup-node","sha":"48b55a011bda9f5d6aeb4c2d9c7362e8dae4041e","version":"v6.4.0"},{"repo":"actions/upload-artifact","sha":"043fb46d1a93c77aae656e7c1c64a875d1fc6a0a","version":"v7.0.1"}],"containers":[{"image":"ghcr.io/github/gh-aw-firewall/agent:0.25.56"},{"image":"ghcr.io/github/gh-aw-firewall/api-proxy:0.25.56"},{"image":"ghcr.io/github/gh-aw-firewall/cli-proxy:0.25.56"},{"image":"ghcr.io/github/gh-aw-firewall/squid:0.25.56"},{"image":"ghcr.io/github/gh-aw-mcpg:v0.3.20"},{"image":"ghcr.io/github/github-mcp-server:v1.1.0"},{"image":"node:lts-alpine","digest":"sha256:2bdb65ed1dab192432bc31c95f94155ca5ad7fc1392fb7eb7526ab682fa5bf14","pinned_image":"node:lts-alpine@sha256:2bdb65ed1dab192432bc31c95f94155ca5ad7fc1392fb7eb7526ab682fa5bf14"}]} +# gh-aw-manifest: {"version":1,"secrets":["ANTHROPIC_API_KEY","GH_AW_CI_TRIGGER_TOKEN","GH_AW_GITHUB_MCP_SERVER_TOKEN","GH_AW_GITHUB_TOKEN","GH_AW_OTEL_GRAFANA_AUTHORIZATION","GH_AW_OTEL_GRAFANA_ENDPOINT","GH_AW_OTEL_SENTRY_AUTHORIZATION","GH_AW_OTEL_SENTRY_ENDPOINT","GITHUB_TOKEN"],"actions":[{"repo":"actions/checkout","sha":"de0fac2e4500dabe0009e67214ff5f5447ce83dd","version":"v6.0.2"},{"repo":"actions/download-artifact","sha":"3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c","version":"v8.0.1"},{"repo":"actions/github-script","sha":"3a2844b7e9c422d3c10d287c895573f7108da1b3","version":"v9.0.0"},{"repo":"actions/setup-go","sha":"4a3601121dd01d1626a1e23e37211e3254c1c06c","version":"v6.4.0"},{"repo":"actions/setup-node","sha":"48b55a011bda9f5d6aeb4c2d9c7362e8dae4041e","version":"v6.4.0"},{"repo":"actions/upload-artifact","sha":"043fb46d1a93c77aae656e7c1c64a875d1fc6a0a","version":"v7.0.1"}],"containers":[{"image":"ghcr.io/github/gh-aw-firewall/agent:0.25.57"},{"image":"ghcr.io/github/gh-aw-firewall/api-proxy:0.25.57"},{"image":"ghcr.io/github/gh-aw-firewall/cli-proxy:0.25.57"},{"image":"ghcr.io/github/gh-aw-firewall/squid:0.25.57"},{"image":"ghcr.io/github/gh-aw-mcpg:v0.3.21"},{"image":"ghcr.io/github/github-mcp-server:v1.1.0"},{"image":"node:lts-alpine","digest":"sha256:2bdb65ed1dab192432bc31c95f94155ca5ad7fc1392fb7eb7526ab682fa5bf14","pinned_image":"node:lts-alpine@sha256:2bdb65ed1dab192432bc31c95f94155ca5ad7fc1392fb7eb7526ab682fa5bf14"}]} # ___ _ _ # / _ \ | | (_) # | |_| | __ _ ___ _ __ | |_ _ ___ diff --git a/.github/workflows/step-name-alignment.lock.yml b/.github/workflows/step-name-alignment.lock.yml index 28bed4c4b32..f1aabc22775 100644 --- a/.github/workflows/step-name-alignment.lock.yml +++ b/.github/workflows/step-name-alignment.lock.yml @@ -1,5 +1,5 @@ # gh-aw-metadata: {"schema_version":"v4","frontmatter_hash":"83b5a45cfd615457e238f8481a8a2cfc8db504f8e0d7a11b0e1b7bd5a2e8bc93","body_hash":"704e7551cc4111b0f1a9e5cab14da2678286ca5fdd97ab522c81672ed3487633","strict":true,"agent_id":"claude"} -# gh-aw-manifest: {"version":1,"secrets":["ANTHROPIC_API_KEY","GH_AW_GITHUB_MCP_SERVER_TOKEN","GH_AW_GITHUB_TOKEN","GH_AW_OTEL_GRAFANA_AUTHORIZATION","GH_AW_OTEL_GRAFANA_ENDPOINT","GH_AW_OTEL_SENTRY_AUTHORIZATION","GH_AW_OTEL_SENTRY_ENDPOINT","GITHUB_TOKEN"],"actions":[{"repo":"actions/cache/restore","sha":"27d5ce7f107fe9357f9df03efb73ab90386fccae","version":"v5.0.5"},{"repo":"actions/cache/save","sha":"27d5ce7f107fe9357f9df03efb73ab90386fccae","version":"v5.0.5"},{"repo":"actions/checkout","sha":"de0fac2e4500dabe0009e67214ff5f5447ce83dd","version":"v6.0.2"},{"repo":"actions/download-artifact","sha":"3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c","version":"v8.0.1"},{"repo":"actions/github-script","sha":"3a2844b7e9c422d3c10d287c895573f7108da1b3","version":"v9.0.0"},{"repo":"actions/setup-node","sha":"48b55a011bda9f5d6aeb4c2d9c7362e8dae4041e","version":"v6.4.0"},{"repo":"actions/upload-artifact","sha":"043fb46d1a93c77aae656e7c1c64a875d1fc6a0a","version":"v7.0.1"}],"containers":[{"image":"ghcr.io/github/gh-aw-firewall/agent:0.25.56"},{"image":"ghcr.io/github/gh-aw-firewall/api-proxy:0.25.56"},{"image":"ghcr.io/github/gh-aw-firewall/cli-proxy:0.25.56"},{"image":"ghcr.io/github/gh-aw-firewall/squid:0.25.56"},{"image":"ghcr.io/github/gh-aw-mcpg:v0.3.20"},{"image":"ghcr.io/github/github-mcp-server:v1.1.0"},{"image":"node:lts-alpine","digest":"sha256:2bdb65ed1dab192432bc31c95f94155ca5ad7fc1392fb7eb7526ab682fa5bf14","pinned_image":"node:lts-alpine@sha256:2bdb65ed1dab192432bc31c95f94155ca5ad7fc1392fb7eb7526ab682fa5bf14"}]} +# gh-aw-manifest: {"version":1,"secrets":["ANTHROPIC_API_KEY","GH_AW_GITHUB_MCP_SERVER_TOKEN","GH_AW_GITHUB_TOKEN","GH_AW_OTEL_GRAFANA_AUTHORIZATION","GH_AW_OTEL_GRAFANA_ENDPOINT","GH_AW_OTEL_SENTRY_AUTHORIZATION","GH_AW_OTEL_SENTRY_ENDPOINT","GITHUB_TOKEN"],"actions":[{"repo":"actions/cache/restore","sha":"27d5ce7f107fe9357f9df03efb73ab90386fccae","version":"v5.0.5"},{"repo":"actions/cache/save","sha":"27d5ce7f107fe9357f9df03efb73ab90386fccae","version":"v5.0.5"},{"repo":"actions/checkout","sha":"de0fac2e4500dabe0009e67214ff5f5447ce83dd","version":"v6.0.2"},{"repo":"actions/download-artifact","sha":"3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c","version":"v8.0.1"},{"repo":"actions/github-script","sha":"3a2844b7e9c422d3c10d287c895573f7108da1b3","version":"v9.0.0"},{"repo":"actions/setup-node","sha":"48b55a011bda9f5d6aeb4c2d9c7362e8dae4041e","version":"v6.4.0"},{"repo":"actions/upload-artifact","sha":"043fb46d1a93c77aae656e7c1c64a875d1fc6a0a","version":"v7.0.1"}],"containers":[{"image":"ghcr.io/github/gh-aw-firewall/agent:0.25.57"},{"image":"ghcr.io/github/gh-aw-firewall/api-proxy:0.25.57"},{"image":"ghcr.io/github/gh-aw-firewall/cli-proxy:0.25.57"},{"image":"ghcr.io/github/gh-aw-firewall/squid:0.25.57"},{"image":"ghcr.io/github/gh-aw-mcpg:v0.3.21"},{"image":"ghcr.io/github/github-mcp-server:v1.1.0"},{"image":"node:lts-alpine","digest":"sha256:2bdb65ed1dab192432bc31c95f94155ca5ad7fc1392fb7eb7526ab682fa5bf14","pinned_image":"node:lts-alpine@sha256:2bdb65ed1dab192432bc31c95f94155ca5ad7fc1392fb7eb7526ab682fa5bf14"}]} # ___ _ _ # / _ \ | | (_) # | |_| | __ _ ___ _ __ | |_ _ ___ From 6b1512170e2cf1d57e08ceba7be50679b5f3ee48 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 29 May 2026 23:14:31 +0000 Subject: [PATCH 3/7] refactor: split claude allowed-tools computation helpers Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com> --- pkg/workflow/claude_tools.go | 563 ++++++++++------------ pkg/workflow/claude_tools_helpers_test.go | 48 ++ 2 files changed, 303 insertions(+), 308 deletions(-) create mode 100644 pkg/workflow/claude_tools_helpers_test.go diff --git a/pkg/workflow/claude_tools.go b/pkg/workflow/claude_tools.go index de97716b69d..246fb24b5c6 100644 --- a/pkg/workflow/claude_tools.go +++ b/pkg/workflow/claude_tools.go @@ -145,364 +145,311 @@ func isExplicitlyDisabledTool(tool any) bool { func (e *ClaudeEngine) computeAllowedClaudeToolsString(tools map[string]any, safeOutputs *SafeOutputsConfig, cacheMemoryConfig *CacheMemoryConfig, mcpScripts *MCPScriptsConfig, sandboxConfig *SandboxConfig) string { claudeToolsLog.Print("Computing allowed Claude tools string") - // Initialize tools map if nil + tools = e.prepareClaudeToolsForAllowedList(tools) + allowedTools := collectClaudeAllowedTools(tools) + allowedTools = appendTopLevelClaudeTools(allowedTools, tools, cacheMemoryConfig) + allowedTools = appendSandboxWritableTools(allowedTools, sandboxConfig) + allowedTools = appendSafeOutputsTools(allowedTools, safeOutputs) + allowedTools = appendMCPScriptsTools(allowedTools, mcpScripts) + allowedTools = dedupeAllowedTools(allowedTools) + + // Sort the allowed tools alphabetically for consistent output + sort.Strings(allowedTools) + + claudeToolsLog.Printf("Generated allowed tools string with %d tools", len(allowedTools)) + + return strings.Join(allowedTools, ",") +} + +func (e *ClaudeEngine) prepareClaudeToolsForAllowedList(tools map[string]any) map[string]any { if tools == nil { tools = make(map[string]any) } - - // Enforce that only neutral tools are provided - fail if claude section is present if _, hasClaudeSection := tools["claude"]; hasClaudeSection { claudeToolsLog.Print("ERROR: Claude section found in input tools, should only contain neutral tools") panic("BUG: computeAllowedClaudeToolsString should only receive neutral tools, not claude section tools") } - - // Convert neutral tools to Claude-specific tools claudeToolsLog.Print("Converting neutral tools to Claude-specific format") tools = e.expandNeutralToolsToClaudeTools(tools) + defaultClaudeTools := []string{"Task", "Glob", "Grep", "ExitPlanMode", "TodoWrite", "LS", "Read", "NotebookRead"} + ensureDefaultClaudeAllowedTools(tools, defaultClaudeTools) + claudeToolsLog.Printf("Added %d default Claude tools to allowed list", len(defaultClaudeTools)) + return tools +} - defaultClaudeTools := []string{ - "Task", - "Glob", - "Grep", - "ExitPlanMode", - "TodoWrite", - "LS", - "Read", - "NotebookRead", - } - - // Ensure claude section exists with the new format - var claudeSection map[string]any - if existing, hasClaudeSection := tools["claude"]; hasClaudeSection { - if claudeMap, ok := existing.(map[string]any); ok { - claudeSection = claudeMap - } else { - claudeSection = make(map[string]any) +func ensureDefaultClaudeAllowedTools(tools map[string]any, defaultClaudeTools []string) { + claudeSection := getOrCreateToolMap(tools, "claude") + claudeAllowed := getOrCreateToolMap(claudeSection, "allowed") + for _, defaultTool := range defaultClaudeTools { + if _, exists := claudeAllowed[defaultTool]; !exists { + claudeAllowed[defaultTool] = nil } - } else { - claudeSection = make(map[string]any) } - - // Get existing allowed tools from the new format (map structure) - var claudeExistingAllowed map[string]any - if allowed, hasAllowed := claudeSection["allowed"]; hasAllowed { - if allowedMap, ok := allowed.(map[string]any); ok { - claudeExistingAllowed = allowedMap - } else { - claudeExistingAllowed = make(map[string]any) + if _, hasBash := claudeAllowed["Bash"]; hasBash { + if _, exists := claudeAllowed["KillBash"]; !exists { + claudeAllowed["KillBash"] = nil + } + if _, exists := claudeAllowed["BashOutput"]; !exists { + claudeAllowed["BashOutput"] = nil } - } else { - claudeExistingAllowed = make(map[string]any) } + claudeSection["allowed"] = claudeAllowed + tools["claude"] = claudeSection +} - // Add default tools that aren't already present - for _, defaultTool := range defaultClaudeTools { - if _, exists := claudeExistingAllowed[defaultTool]; !exists { - claudeExistingAllowed[defaultTool] = nil // Add tool with null value +func getOrCreateToolMap(container map[string]any, key string) map[string]any { + if existing, ok := container[key]; ok { + if existingMap, ok := existing.(map[string]any); ok { + return existingMap } } + return make(map[string]any) +} - // Check if Bash tools are present and add implicit KillBash and BashOutput - if _, hasBash := claudeExistingAllowed["Bash"]; hasBash { - // Implicitly add KillBash and BashOutput when any Bash tools are allowed - if _, exists := claudeExistingAllowed["KillBash"]; !exists { - claudeExistingAllowed["KillBash"] = nil +func collectClaudeAllowedTools(tools map[string]any) []string { + claudeConfig, ok := typeutil.LookupMap(tools, "claude") + if !ok { + return nil + } + allowedMap, ok := typeutil.LookupMap(claudeConfig, "allowed") + if !ok { + return nil + } + allowedTools := make([]string, 0, len(allowedMap)) + for toolName, toolValue := range allowedMap { + if toolName == "Bash" { + allowedTools = appendClaudeBashTools(allowedTools, toolValue) + continue } - if _, exists := claudeExistingAllowed["BashOutput"]; !exists { - claudeExistingAllowed["BashOutput"] = nil + if isClaudeToolName(toolName) { + allowedTools = append(allowedTools, toolName) } } + return allowedTools +} - // Update the claude section with the new format - claudeSection["allowed"] = claudeExistingAllowed - tools["claude"] = claudeSection - - claudeToolsLog.Printf("Added %d default Claude tools to allowed list", len(defaultClaudeTools)) - - var allowedTools []string +func appendClaudeBashTools(allowedTools []string, toolValue any) []string { + bashCommands, ok := toolValue.([]any) + if !ok { + return append(allowedTools, "Bash") + } + if hasBashWildcard(bashCommands) { + return append(allowedTools, "Bash") + } + for _, cmd := range bashCommands { + if cmdStr, ok := cmd.(string); ok { + normalized, _ := normalizeBashCommand(cmdStr) + allowedTools = append(allowedTools, fmt.Sprintf("Bash(%s)", normalized)) + } + } + return allowedTools +} - // Process claude-specific tools from the claude section (new format only) - claudeConfig, ok := typeutil.LookupMap(tools, "claude") - if ok { - allowedMap, ok := typeutil.LookupMap(claudeConfig, "allowed") - if ok { - // In the new format, allowed is a map where keys are tool names - for toolName, toolValue := range allowedMap { - if toolName == "Bash" { - // Handle Bash tool with specific commands - if bashCommands, ok := toolValue.([]any); ok { - // Check for :* wildcard first - if present, ignore all other bash commands - for _, cmd := range bashCommands { - if cmdStr, ok := cmd.(string); ok && cmdStr == ":*" { - // :* means allow all bash and ignore other commands - allowedTools = append(allowedTools, "Bash") - goto nextClaudeTool - } - } - // Process the allowed bash commands (no :* found) - for _, cmd := range bashCommands { - if cmdStr, ok := cmd.(string); ok && cmdStr == "*" { - // Wildcard means allow all bash - allowedTools = append(allowedTools, "Bash") - goto nextClaudeTool - } - } - // Add individual bash commands with Bash() prefix - for _, cmd := range bashCommands { - if cmdStr, ok := cmd.(string); ok { - // Normalize trailing " *" wildcard (e.g. "jq *" → "jq") so that - // all engines emit the canonical prefix form (Bash(jq)) regardless - // of whether the command was written with or without the wildcard. - normalized, _ := normalizeBashCommand(cmdStr) - allowedTools = append(allowedTools, fmt.Sprintf("Bash(%s)", normalized)) - } - } - } else { - // Bash with no specific commands or null value - allow all bash - allowedTools = append(allowedTools, "Bash") - } - } else if strings.HasPrefix(toolName, strings.ToUpper(toolName[:1])) { - // Tool name starts with uppercase letter - regular Claude tool - allowedTools = append(allowedTools, toolName) - } - nextClaudeTool: - } +func hasBashWildcard(commands []any) bool { + for _, cmd := range commands { + if cmdStr, ok := cmd.(string); ok && (cmdStr == ":*" || cmdStr == "*") { + return true } } + return false +} + +func isClaudeToolName(toolName string) bool { + return len(toolName) > 0 && strings.HasPrefix(toolName, strings.ToUpper(toolName[:1])) +} - // Process top-level tools (MCP tools and claude) +func appendTopLevelClaudeTools(allowedTools []string, tools map[string]any, cacheMemoryConfig *CacheMemoryConfig) []string { for toolName, toolValue := range tools { if toolName == "claude" { - // Skip the claude section as we've already processed it continue - } else { - // Handle cache-memory as a special case - it provides file system access but no MCP tool - if toolName == "cache-memory" { - // Add path-specific Read/Write/Edit/MultiEdit tools for each cache directory. - // Pattern: {cacheDir}/* grants access to all files within the directory. - if cacheMemoryConfig != nil { - for _, cache := range cacheMemoryConfig.Caches { - cacheDirPattern := cacheMemoryDirFor(cache.ID) + "/*" - - // Add path-specific tools for cache directory access - if !slices.Contains(allowedTools, fmt.Sprintf("Read(%s)", cacheDirPattern)) { - allowedTools = append(allowedTools, fmt.Sprintf("Read(%s)", cacheDirPattern)) - } - if !slices.Contains(allowedTools, fmt.Sprintf("Write(%s)", cacheDirPattern)) { - allowedTools = append(allowedTools, fmt.Sprintf("Write(%s)", cacheDirPattern)) - } - if !slices.Contains(allowedTools, fmt.Sprintf("Edit(%s)", cacheDirPattern)) { - allowedTools = append(allowedTools, fmt.Sprintf("Edit(%s)", cacheDirPattern)) - } - if !slices.Contains(allowedTools, fmt.Sprintf("MultiEdit(%s)", cacheDirPattern)) { - allowedTools = append(allowedTools, fmt.Sprintf("MultiEdit(%s)", cacheDirPattern)) - } - - // If unrestricted bash is not already granted, inject the minimal bash - // permissions needed to manipulate cache files (create subdirs, read, write, - // move). This avoids requiring every workflow author to add these manually. - if !slices.Contains(allowedTools, "Bash") { - cacheDir := cacheMemoryDirFor(cache.ID) - cacheDirSlash := cacheDir + "/" - bashCacheTools := []string{ - fmt.Sprintf("Bash(mkdir -p %s)", cacheDirSlash), - fmt.Sprintf("Bash(cat %s)", cacheDirSlash), - fmt.Sprintf("Bash(cat > %s)", cacheDirSlash), - fmt.Sprintf("Bash(mv %s)", cacheDirSlash), - } - for _, bashTool := range bashCacheTools { - if !slices.Contains(allowedTools, bashTool) { - allowedTools = append(allowedTools, bashTool) - } - } - // Add BashOutput and KillBash since bash commands are now allowed - if !slices.Contains(allowedTools, "BashOutput") { - allowedTools = append(allowedTools, "BashOutput") - } - if !slices.Contains(allowedTools, "KillBash") { - allowedTools = append(allowedTools, "KillBash") - } - } - } - } - continue - } + } + switch toolName { + case "cache-memory": + allowedTools = appendCacheMemoryTools(allowedTools, cacheMemoryConfig) + case "agentic-workflows": + allowedTools = append(allowedTools, "mcp__"+string(constants.AgenticWorkflowsMCPServerID)) + default: + allowedTools = appendMCPToolPermissions(allowedTools, toolName, toolValue) + } + } + return allowedTools +} - // agentic-workflows is a known system MCP server whose tool value is a bool (enabled flag), - // not a map. Handle it before the generic map check so it does not fall through silently. - if toolName == "agentic-workflows" { - allowedTools = append(allowedTools, "mcp__"+string(constants.AgenticWorkflowsMCPServerID)) - continue - } +func appendCacheMemoryTools(allowedTools []string, cacheMemoryConfig *CacheMemoryConfig) []string { + if cacheMemoryConfig == nil { + return allowedTools + } + for _, cache := range cacheMemoryConfig.Caches { + cacheDir := cacheMemoryDirFor(cache.ID) + cacheDirPattern := cacheDir + "/*" + allowedTools = appendIfMissing(allowedTools, fmt.Sprintf("Read(%s)", cacheDirPattern)) + allowedTools = appendIfMissing(allowedTools, fmt.Sprintf("Write(%s)", cacheDirPattern)) + allowedTools = appendIfMissing(allowedTools, fmt.Sprintf("Edit(%s)", cacheDirPattern)) + allowedTools = appendIfMissing(allowedTools, fmt.Sprintf("MultiEdit(%s)", cacheDirPattern)) + allowedTools = appendCacheMemoryBashTools(allowedTools, cacheDir) + } + return allowedTools +} - // Check if this is an MCP tool (has MCP-compatible type) or standard MCP tool (github) - if mcpConfig, ok := toolValue.(map[string]any); ok { - // Check if it's explicitly marked as MCP type - isCustomMCP := false - if hasMcp, _ := hasMCPConfig(mcpConfig); hasMcp { - isCustomMCP = true - } - - // Handle standard MCP tools (github, playwright) or tools with MCP-compatible type - if toolName == "github" { - // Parse GitHub tool configuration for type safety - githubConfig := parseGitHubTool(toolValue) - if githubConfig != nil && len(githubConfig.Allowed) > 0 { - // Check for wildcard access first - hasWildcard := false - for _, tool := range githubConfig.Allowed { - if string(tool) == "*" { - hasWildcard = true - break - } - } - - if hasWildcard { - // For wildcard access, just add the server name with mcp__ prefix - allowedTools = append(allowedTools, "mcp__"+toolName) - } else { - // For specific tools, add each one individually - for _, tool := range githubConfig.Allowed { - allowedTools = append(allowedTools, fmt.Sprintf("mcp__%s__%s", toolName, string(tool))) - } - } - } else { - // For GitHub tools without explicit allowed list, use appropriate default GitHub tools based on mode - githubMode := getGitHubType(mcpConfig) - var defaultTools []string - if githubMode == "remote" { - defaultTools = constants.DefaultGitHubToolsRemote - } else { - defaultTools = constants.DefaultGitHubToolsLocal - } - for _, defaultTool := range defaultTools { - allowedTools = append(allowedTools, "mcp__github__"+defaultTool) - } - } - } else if toolName == "playwright" || isCustomMCP { - // Handle playwright and custom MCP tools with generic parsing - if allowed, hasAllowed := mcpConfig["allowed"]; hasAllowed { - if allowedSlice, ok := allowed.([]any); ok { - // Check for wildcard access first - hasWildcard := false - for _, item := range allowedSlice { - if str, ok := item.(string); ok && str == "*" { - hasWildcard = true - break - } - } - - if hasWildcard { - // For wildcard access, just add the server name with mcp__ prefix - allowedTools = append(allowedTools, "mcp__"+toolName) - } else { - // For specific tools, add each one individually - for _, item := range allowedSlice { - if str, ok := item.(string); ok { - allowedTools = append(allowedTools, fmt.Sprintf("mcp__%s__%s", toolName, str)) - } - } - } - } - } else { - // No explicit allowed list: default to granting access to all tools from this - // MCP server. The user explicitly added the server in their workflow, so they - // intend to use its tools. Server-side restrictions still apply. - allowedTools = append(allowedTools, "mcp__"+toolName) - } - } +func appendCacheMemoryBashTools(allowedTools []string, cacheDir string) []string { + if slices.Contains(allowedTools, "Bash") { + return allowedTools + } + cacheDirSlash := cacheDir + "/" + bashCacheTools := []string{ + fmt.Sprintf("Bash(mkdir -p %s)", cacheDirSlash), + fmt.Sprintf("Bash(cat %s)", cacheDirSlash), + fmt.Sprintf("Bash(cat > %s)", cacheDirSlash), + fmt.Sprintf("Bash(mv %s)", cacheDirSlash), + } + for _, bashTool := range bashCacheTools { + allowedTools = appendIfMissing(allowedTools, bashTool) + } + allowedTools = appendIfMissing(allowedTools, "BashOutput") + allowedTools = appendIfMissing(allowedTools, "KillBash") + return allowedTools +} + +func appendMCPToolPermissions(allowedTools []string, toolName string, toolValue any) []string { + mcpConfig, ok := toolValue.(map[string]any) + if !ok { + return allowedTools + } + isCustomMCP := false + if hasMcp, _ := hasMCPConfig(mcpConfig); hasMcp { + isCustomMCP = true + } + if toolName == "github" { + return appendGitHubMCPTools(allowedTools, toolName, toolValue, mcpConfig) + } + if toolName == "playwright" || isCustomMCP { + return appendGenericMCPTools(allowedTools, toolName, mcpConfig) + } + return allowedTools +} + +func appendGitHubMCPTools(allowedTools []string, toolName string, toolValue any, mcpConfig map[string]any) []string { + githubConfig := parseGitHubTool(toolValue) + if githubConfig != nil && len(githubConfig.Allowed) > 0 { + for _, tool := range githubConfig.Allowed { + if string(tool) == "*" { + return append(allowedTools, "mcp__"+toolName) } } + for _, tool := range githubConfig.Allowed { + allowedTools = append(allowedTools, fmt.Sprintf("mcp__%s__%s", toolName, string(tool))) + } + return allowedTools } + githubMode := getGitHubType(mcpConfig) + defaultTools := constants.DefaultGitHubToolsLocal + if githubMode == "remote" { + defaultTools = constants.DefaultGitHubToolsRemote + } + for _, defaultTool := range defaultTools { + allowedTools = append(allowedTools, "mcp__github__"+defaultTool) + } + return allowedTools +} - // Grant path-scoped file tool access for sandbox writable paths. - // Claude workflows should always be able to use /tmp even when not explicitly - // listed in sandbox.agent.config.filesystem.allowWrite. - if sandboxConfig != nil { - writablePaths := []string{defaultClaudeTmpWritePath} - if sandboxConfig.Agent != nil && sandboxConfig.Agent.Config != nil && sandboxConfig.Agent.Config.Filesystem != nil { - writablePaths = append(writablePaths, sandboxConfig.Agent.Config.Filesystem.AllowWrite...) +func appendGenericMCPTools(allowedTools []string, toolName string, mcpConfig map[string]any) []string { + allowed, hasAllowed := mcpConfig["allowed"] + if !hasAllowed { + return append(allowedTools, "mcp__"+toolName) + } + allowedSlice, ok := allowed.([]any) + if !ok { + return allowedTools + } + for _, item := range allowedSlice { + if str, ok := item.(string); ok && str == "*" { + return append(allowedTools, "mcp__"+toolName) } - seenPatterns := make(map[string]struct{}, len(writablePaths)) - for _, writablePath := range writablePaths { - path := strings.TrimSpace(writablePath) - if path == "" { - continue - } - // Claude path-scoped tool permissions must be absolute. - if !strings.HasPrefix(path, "/") { - continue - } - pattern := path - // Treat plain directory paths as "directory contents" grants to mirror cache-memory behavior. - if !strings.ContainsAny(pattern, "*?[]{}") { - pattern = strings.TrimRight(pattern, "/") + "/*" - } - if _, seen := seenPatterns[pattern]; seen { - continue - } - seenPatterns[pattern] = struct{}{} - for _, toolPattern := range []string{ - fmt.Sprintf("Read(%s)", pattern), - fmt.Sprintf("Write(%s)", pattern), - fmt.Sprintf("Edit(%s)", pattern), - fmt.Sprintf("MultiEdit(%s)", pattern), - } { - if !slices.Contains(allowedTools, toolPattern) { - allowedTools = append(allowedTools, toolPattern) - } - } + } + for _, item := range allowedSlice { + if str, ok := item.(string); ok { + allowedTools = append(allowedTools, fmt.Sprintf("mcp__%s__%s", toolName, str)) } } + return allowedTools +} - // Handle SafeOutputs: grant access to all safe-outputs MCP tools and Write permission. - // The safeoutputs MCP server is started whenever safe-outputs is configured; with - // --permission-mode acceptEdits its tools must be explicitly listed in --allowed-tools - // (unlike bypassPermissions which silently ignores the allowlist). - if safeOutputs != nil { - allowedTools = append(allowedTools, "mcp__"+string(constants.SafeOutputsMCPServerID)) +func appendSandboxWritableTools(allowedTools []string, sandboxConfig *SandboxConfig) []string { + if sandboxConfig == nil { + return allowedTools + } + writablePaths := []string{defaultClaudeTmpWritePath} + if sandboxConfig.Agent != nil && sandboxConfig.Agent.Config != nil && sandboxConfig.Agent.Config.Filesystem != nil { + writablePaths = append(writablePaths, sandboxConfig.Agent.Config.Filesystem.AllowWrite...) + } + seenPatterns := make(map[string]struct{}, len(writablePaths)) + for _, writablePath := range writablePaths { + pattern, ok := normalizeSandboxWritablePattern(writablePath) + if !ok { + continue + } + if _, seen := seenPatterns[pattern]; seen { + continue + } + seenPatterns[pattern] = struct{}{} + allowedTools = appendIfMissing(allowedTools, fmt.Sprintf("Read(%s)", pattern)) + allowedTools = appendIfMissing(allowedTools, fmt.Sprintf("Write(%s)", pattern)) + allowedTools = appendIfMissing(allowedTools, fmt.Sprintf("Edit(%s)", pattern)) + allowedTools = appendIfMissing(allowedTools, fmt.Sprintf("MultiEdit(%s)", pattern)) + } + return allowedTools +} - // Check if a general "Write" permission is already granted - hasGeneralWrite := slices.Contains(allowedTools, "Write") +func normalizeSandboxWritablePattern(writablePath string) (string, bool) { + path := strings.TrimSpace(writablePath) + if path == "" || !strings.HasPrefix(path, "/") { + return "", false + } + if strings.ContainsAny(path, "*?[]{}") { + return path, true + } + return strings.TrimRight(path, "/") + "/*", true +} - // If no general Write permission and SafeOutputs is configured, - // add specific write permission for GH_AW_SAFE_OUTPUTS - if !hasGeneralWrite { - allowedTools = append(allowedTools, "Write") - // Ideally we would only give permission to the exact file, but that doesn't seem - // to be working with Claude. See https://github.com/github/gh-aw/issues/244#issuecomment-3240319103 - //allowedTools = append(allowedTools, "Write(${{ env.GH_AW_SAFE_OUTPUTS }})") - } +func appendSafeOutputsTools(allowedTools []string, safeOutputs *SafeOutputsConfig) []string { + if safeOutputs == nil { + return allowedTools } + allowedTools = append(allowedTools, "mcp__"+string(constants.SafeOutputsMCPServerID)) + if !slices.Contains(allowedTools, "Write") { + allowedTools = append(allowedTools, "Write") + } + return allowedTools +} - // Handle MCPScripts: grant access to all mcpscripts MCP tools. - // The mcpscripts server is started when mcp-scripts tools are configured; with - // --permission-mode acceptEdits its tools must be explicitly listed in --allowed-tools. +func appendMCPScriptsTools(allowedTools []string, mcpScripts *MCPScriptsConfig) []string { if HasMCPScripts(mcpScripts) { allowedTools = append(allowedTools, "mcp__"+string(constants.MCPScriptsMCPServerID)) } + return allowedTools +} - // Deduplicate tools before sorting/joining to avoid duplicate Bash(...) entries - // when equivalent wildcard/non-wildcard bash commands normalize to the same form. - if len(allowedTools) > 1 { - seen := make(map[string]struct{}, len(allowedTools)) - deduped := make([]string, 0, len(allowedTools)) - for _, tool := range allowedTools { - if _, ok := seen[tool]; ok { - continue - } - seen[tool] = struct{}{} - deduped = append(deduped, tool) +func dedupeAllowedTools(allowedTools []string) []string { + if len(allowedTools) <= 1 { + return allowedTools + } + seen := make(map[string]struct{}, len(allowedTools)) + deduped := make([]string, 0, len(allowedTools)) + for _, tool := range allowedTools { + if _, ok := seen[tool]; ok { + continue } - allowedTools = deduped + seen[tool] = struct{}{} + deduped = append(deduped, tool) } + return deduped +} - // Sort the allowed tools alphabetically for consistent output - sort.Strings(allowedTools) - - claudeToolsLog.Printf("Generated allowed tools string with %d tools", len(allowedTools)) - - return strings.Join(allowedTools, ",") +func appendIfMissing(items []string, item string) []string { + if slices.Contains(items, item) { + return items + } + return append(items, item) } // generateAllowedToolsComment generates a multi-line comment showing each allowed tool diff --git a/pkg/workflow/claude_tools_helpers_test.go b/pkg/workflow/claude_tools_helpers_test.go new file mode 100644 index 00000000000..a5ace74b3ac --- /dev/null +++ b/pkg/workflow/claude_tools_helpers_test.go @@ -0,0 +1,48 @@ +package workflow + +import "testing" + +func TestHasBashWildcard(t *testing.T) { + tests := []struct { + name string + commands []any + want bool + }{ + {name: "no wildcard", commands: []any{"jq", "sed"}, want: false}, + {name: "star wildcard", commands: []any{"*"}, want: true}, + {name: "colon star wildcard", commands: []any{":*"}, want: true}, + {name: "non-string values", commands: []any{1, true}, want: false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := hasBashWildcard(tt.commands); got != tt.want { + t.Fatalf("hasBashWildcard() = %v, want %v", got, tt.want) + } + }) + } +} + +func TestNormalizeSandboxWritablePattern(t *testing.T) { + tests := []struct { + name string + input string + want string + wantOkay bool + }{ + {name: "absolute directory path", input: "/tmp/cache", want: "/tmp/cache/*", wantOkay: true}, + {name: "absolute glob path", input: "/tmp/cache/*", want: "/tmp/cache/*", wantOkay: true}, + {name: "trim whitespace", input: " /tmp/cache ", want: "/tmp/cache/*", wantOkay: true}, + {name: "relative path rejected", input: "tmp/cache", want: "", wantOkay: false}, + {name: "empty path rejected", input: " ", want: "", wantOkay: false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, gotOkay := normalizeSandboxWritablePattern(tt.input) + if got != tt.want || gotOkay != tt.wantOkay { + t.Fatalf("normalizeSandboxWritablePattern() = (%q, %v), want (%q, %v)", got, gotOkay, tt.want, tt.wantOkay) + } + }) + } +} From d0113e8177990f09782fc349f422e7f99f89bfbf Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 29 May 2026 23:18:48 +0000 Subject: [PATCH 4/7] test: align claude helper tests with unit conventions Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com> --- pkg/workflow/claude_tools_helpers_test.go | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/pkg/workflow/claude_tools_helpers_test.go b/pkg/workflow/claude_tools_helpers_test.go index a5ace74b3ac..d8988a42387 100644 --- a/pkg/workflow/claude_tools_helpers_test.go +++ b/pkg/workflow/claude_tools_helpers_test.go @@ -1,6 +1,12 @@ +//go:build !integration + package workflow -import "testing" +import ( + "testing" + + "github.com/stretchr/testify/assert" +) func TestHasBashWildcard(t *testing.T) { tests := []struct { @@ -16,9 +22,8 @@ func TestHasBashWildcard(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - if got := hasBashWildcard(tt.commands); got != tt.want { - t.Fatalf("hasBashWildcard() = %v, want %v", got, tt.want) - } + got := hasBashWildcard(tt.commands) + assert.Equal(t, tt.want, got, "hasBashWildcard() should match expected value") }) } } @@ -40,9 +45,8 @@ func TestNormalizeSandboxWritablePattern(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { got, gotOkay := normalizeSandboxWritablePattern(tt.input) - if got != tt.want || gotOkay != tt.wantOkay { - t.Fatalf("normalizeSandboxWritablePattern() = (%q, %v), want (%q, %v)", got, gotOkay, tt.want, tt.wantOkay) - } + assert.Equal(t, tt.want, got, "pattern should match expected") + assert.Equal(t, tt.wantOkay, gotOkay, "ok flag should match expected") }) } } From 4db7eb4c8c2e26c034f17e5f983625320b23c97a Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Fri, 29 May 2026 23:37:08 +0000 Subject: [PATCH 5/7] docs(adr): add draft ADR-35812 for Claude allowed-tools decomposition Co-Authored-By: Claude Opus 4.8 (1M context) --- ...decompose-claude-allowed-tools-assembly.md | 74 +++++++++++++++++++ 1 file changed, 74 insertions(+) create mode 100644 docs/adr/35812-decompose-claude-allowed-tools-assembly.md diff --git a/docs/adr/35812-decompose-claude-allowed-tools-assembly.md b/docs/adr/35812-decompose-claude-allowed-tools-assembly.md new file mode 100644 index 00000000000..5107f90e71a --- /dev/null +++ b/docs/adr/35812-decompose-claude-allowed-tools-assembly.md @@ -0,0 +1,74 @@ +# ADR-35812: Decompose Claude Allowed-Tools Assembly into Focused Helpers + +**Date**: 2026-05-29 +**Status**: Draft +**Deciders**: Unknown + +--- + +## Part 1 — Narrative (Human-Friendly) + +### Context + +`ClaudeEngine.computeAllowedClaudeToolsString` in `pkg/workflow/claude_tools.go` had grown into a single ~360-line function that assembled the `--allowed-tools` string for the Claude engine. It interleaved many unrelated responsibilities in one scope: neutral→Claude tool conversion and defaulting, Claude `allowed` extraction (including Bash wildcard handling via `goto` labels), top-level MCP/cache-memory permission expansion, sandbox writable-path normalization, safe-outputs and mcp-scripts grants, and final de-duplication. The deep nesting, `goto nextClaudeTool` control flow, and repeated `slices.Contains` guards made the function a flagged high-complexity hotspot in the ongoing "lint-monster" effort to reduce large-function complexity across the workflow/CLI codepaths, and the embedded wildcard/path-normalization logic was not independently unit-testable. + +### Decision + +We will decompose `computeAllowedClaudeToolsString` into a pipeline of focused, single-responsibility helper functions — `prepareClaudeToolsForAllowedList`, `collectClaudeAllowedTools`, `appendTopLevelClaudeTools`, `appendSandboxWritableTools`, `appendSafeOutputsTools`, `appendMCPScriptsTools`, and `dedupeAllowedTools` — each transforming the running `allowedTools` slice, with the orchestrator reduced to a linear sequence of calls followed by the existing sort/join. This is a strictly **behavior-preserving** refactor: the emitted `--allowed-tools` string remains sorted and de-duplicated exactly as before. The primary driver is reducing cyclomatic complexity for maintainability; a secondary benefit is extracting pure logic (`hasBashWildcard`, `normalizeSandboxWritablePattern`, `getOrCreateToolMap`, `appendIfMissing`) into units covered by new helper-level unit tests. + +### Alternatives Considered + +#### Alternative 1: Leave the function as-is or suppress the complexity lint + +Keep the monolithic function and silence the complexity warning with a suppression directive. Rejected because the function is on a hot, security-sensitive path (it decides which tools an agent is permitted to invoke), and its `goto`-driven control flow and untestable inner logic make it error-prone to modify. Suppressing the lint preserves the maintenance hazard the lint exists to surface. + +#### Alternative 2: Introduce a builder/struct that accumulates permissions + +Model the assembly as a stateful `allowedToolsBuilder` type with mutating methods (`.addDefaults()`, `.addMCP()`, etc.) instead of free functions threading a `[]string`. Rejected for this PR because it is a larger change that introduces new state and lifecycle, increasing the surface area beyond a behavior-preserving extraction; the free-function pipeline keeps each step pure and independently testable while matching the existing procedural style of the file. A builder remains a reasonable future evolution if more permission sources are added. + +### Consequences + +#### Positive +- Each permission source (Claude allowed, cache-memory, MCP, sandbox, safe-outputs, mcp-scripts) lives in a named helper, so a contributor changing one concern reads and edits only that helper rather than navigating a 360-line function. +- Pure helpers (`hasBashWildcard`, `normalizeSandboxWritablePattern`) are now unit-tested in `claude_tools_helpers_test.go`, locking the wildcard and path-normalization semantics against regressions during subsequent complexity-reduction work. +- The `goto nextClaudeTool` control flow is eliminated in favor of early-returning helpers, and repeated `!slices.Contains(...)` guards are consolidated behind `appendIfMissing`. + +#### Negative +- The number of package-level functions in `claude_tools.go` grows substantially, and the running `allowedTools` slice is now threaded through many `append*` calls, which a reader must follow across functions to reconstruct the full output. +- Extraction introduces new exported-within-package surface (`getOrCreateToolMap`, `appendIfMissing`, etc.) that future contributors may reuse inconsistently if the intended scope is not understood. + +#### Neutral +- The output contract is unchanged by design, so no caller, snapshot, or generated lock file should differ; correctness rests on the existing higher-level tests plus the new helper tests. +- `appendIfMissing` centralizes the dedup-on-append idiom, but a final `dedupeAllowedTools` pass is still retained to catch duplicates arising from wildcard/non-wildcard bash normalization, so the two mechanisms coexist intentionally. + +--- + +## Part 2 — Normative Specification (RFC 2119) + +> The key words **MUST**, **MUST NOT**, **REQUIRED**, **SHALL**, **SHALL NOT**, **SHOULD**, **SHOULD NOT**, **RECOMMENDED**, **MAY**, and **OPTIONAL** in this section are to be interpreted as described in [RFC 2119](https://www.rfc-editor.org/rfc/rfc2119). + +### Output Contract Preservation + +1. The refactored `computeAllowedClaudeToolsString` **MUST** produce a byte-for-byte identical `--allowed-tools` string to the pre-refactor implementation for any given `(tools, safeOutputs, cacheMemoryConfig, mcpScripts, sandboxConfig)` input. +2. The final output **MUST** remain sorted alphabetically and de-duplicated. +3. The refactor **MUST NOT** add, remove, or rename any tool permission that the prior implementation would have emitted. + +### Decomposition Structure + +1. The orchestrator function **MUST** delegate each distinct permission-source concern (Claude `allowed` extraction, top-level/MCP permissions, cache-memory, sandbox writable paths, safe-outputs, mcp-scripts, de-duplication) to a dedicated helper function. +2. Helper functions that accumulate permissions **SHOULD** accept and return the running `allowedTools` slice rather than mutating shared package state. +3. Pure decision logic extracted from the orchestrator (bash wildcard detection, sandbox path normalization) **MUST** be implemented as standalone functions with no side effects. +4. The implementation **SHOULD NOT** reintroduce `goto`-based control flow for tool iteration. + +### Test Coverage + +1. Extracted pure helpers (`hasBashWildcard`, `normalizeSandboxWritablePattern`) **MUST** have direct unit tests covering wildcard, non-wildcard, and rejection (relative/empty path) cases. +2. The pre-existing higher-level tests for `computeAllowedClaudeToolsString` **MUST** continue to pass unchanged. + +### Conformance + +An implementation is considered conformant with this ADR if it satisfies all **MUST** and **MUST NOT** requirements above. Failure to meet any **MUST** or **MUST NOT** requirement constitutes non-conformance. + +--- + +*This is a DRAFT ADR generated by the [Design Decision Gate](https://github.com/github/gh-aw/actions/runs/26667328855) workflow. The PR author must review, complete, and finalize this document before the PR can merge.* From a659e1549c7758c0373c2af3d0b8b3399d8a059f Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 29 May 2026 23:57:47 +0000 Subject: [PATCH 6/7] chore: start review-comment triage Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- go.mod | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/go.mod b/go.mod index ea1d4b08374..f546a97c9f1 100644 --- a/go.mod +++ b/go.mod @@ -7,6 +7,7 @@ require ( charm.land/bubbletea/v2 v2.0.6 charm.land/huh/v2 v2.0.3 charm.land/lipgloss/v2 v2.0.3 + github.com/charmbracelet/colorprofile v0.4.3 github.com/charmbracelet/x/exp/golden v0.0.0-20260527151214-009e6338d40d github.com/cli/go-gh/v2 v2.13.0 github.com/creack/pty v1.1.24 @@ -43,7 +44,6 @@ require ( github.com/catppuccin/go v0.3.0 // indirect github.com/ccojocar/zxcvbn-go v1.0.4 // indirect github.com/cespare/xxhash/v2 v2.3.0 // indirect - github.com/charmbracelet/colorprofile v0.4.3 // indirect github.com/charmbracelet/harmonica v0.2.0 // indirect github.com/charmbracelet/ultraviolet v0.0.0-20260416161146-9c68a866306c // indirect github.com/charmbracelet/x/ansi v0.11.7 // indirect From 06dd7f221d8c283fddc5302ec9452f205d4fac92 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 30 May 2026 00:04:50 +0000 Subject: [PATCH 7/7] fix(workflow): address review feedback on Claude tool helpers Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/workflow/claude_tools.go | 11 +++++-- pkg/workflow/claude_tools_helpers_test.go | 38 +++++++++++++++++++++++ 2 files changed, 46 insertions(+), 3 deletions(-) diff --git a/pkg/workflow/claude_tools.go b/pkg/workflow/claude_tools.go index 246fb24b5c6..862ec64459d 100644 --- a/pkg/workflow/claude_tools.go +++ b/pkg/workflow/claude_tools.go @@ -193,8 +193,6 @@ func ensureDefaultClaudeAllowedTools(tools map[string]any, defaultClaudeTools [] claudeAllowed["BashOutput"] = nil } } - claudeSection["allowed"] = claudeAllowed - tools["claude"] = claudeSection } func getOrCreateToolMap(container map[string]any, key string) map[string]any { @@ -203,7 +201,9 @@ func getOrCreateToolMap(container map[string]any, key string) map[string]any { return existingMap } } - return make(map[string]any) + created := make(map[string]any) + container[key] = created + return created } func collectClaudeAllowedTools(tools map[string]any) []string { @@ -254,6 +254,8 @@ func hasBashWildcard(commands []any) bool { return false } +// isClaudeToolName uses the existing Claude naming convention heuristic: +// valid Claude tool keys are expected to start with an uppercase letter. func isClaudeToolName(toolName string) bool { return len(toolName) > 0 && strings.HasPrefix(toolName, strings.ToUpper(toolName[:1])) } @@ -417,6 +419,9 @@ func appendSafeOutputsTools(allowedTools []string, safeOutputs *SafeOutputsConfi } allowedTools = append(allowedTools, "mcp__"+string(constants.SafeOutputsMCPServerID)) if !slices.Contains(allowedTools, "Write") { + // Ideally we would grant Write only for the exact safe outputs file, but Claude + // doesn't currently honor that scoped grant reliably. + // See: https://github.com/github/gh-aw/issues/244#issuecomment-3240319103 allowedTools = append(allowedTools, "Write") } return allowedTools diff --git a/pkg/workflow/claude_tools_helpers_test.go b/pkg/workflow/claude_tools_helpers_test.go index d8988a42387..01c5826a265 100644 --- a/pkg/workflow/claude_tools_helpers_test.go +++ b/pkg/workflow/claude_tools_helpers_test.go @@ -5,6 +5,7 @@ package workflow import ( "testing" + "github.com/github/gh-aw/pkg/constants" "github.com/stretchr/testify/assert" ) @@ -50,3 +51,40 @@ func TestNormalizeSandboxWritablePattern(t *testing.T) { }) } } + +func TestGetOrCreateToolMapStoresCreatedMap(t *testing.T) { + container := map[string]any{} + + created := getOrCreateToolMap(container, "claude") + created["allowed"] = map[string]any{"Read": nil} + + stored, ok := container["claude"].(map[string]any) + assert.True(t, ok) + assert.Equal(t, created, stored) +} + +func TestAppendGitHubMCPTools(t *testing.T) { + t.Run("uses wildcard alias", func(t *testing.T) { + mcpConfig := map[string]any{"allowed": []any{"*"}} + got := appendGitHubMCPTools(nil, "github", mcpConfig, mcpConfig) + assert.Equal(t, []string{"mcp__github"}, got) + }) + + t.Run("expands specific allowed tools", func(t *testing.T) { + mcpConfig := map[string]any{"allowed": []any{"issue_read", "issue_update"}} + got := appendGitHubMCPTools(nil, "github", mcpConfig, mcpConfig) + assert.Equal(t, []string{"mcp__github__issue_read", "mcp__github__issue_update"}, got) + }) + + t.Run("falls back to remote defaults", func(t *testing.T) { + mcpConfig := map[string]any{"mode": "remote"} + got := appendGitHubMCPTools(nil, "github", mcpConfig, mcpConfig) + assert.Equal(t, len(constants.DefaultGitHubToolsRemote), len(got)) + assert.Contains(t, got, "mcp__github__issue_read") + }) +} + +func TestDedupeAllowedTools(t *testing.T) { + got := dedupeAllowedTools([]string{"Read", "Bash", "Read", "Bash"}) + assert.Equal(t, []string{"Read", "Bash"}, got) +}