From 19ef47f4938a83f25a1ff385b0553b548549011f Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 25 Apr 2026 06:08:48 +0000 Subject: [PATCH 1/4] Initial plan From c2f55baa8efd6ae15a914c8e8eea5b1e41d537c8 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 25 Apr 2026 06:30:51 +0000 Subject: [PATCH 2/4] perf: fix CompileSimpleWorkflow regression with 3 targeted optimizations - Eliminate redundant computeAllowedClaudeToolsString call in GetExecutionSteps (was called twice: once for --allowed-tools arg, once for comment generation) - Optimize generateAllowedToolsComment to avoid fmt.Fprintf allocations and string concatenations; pre-size builder to eliminate buffer growth reallocations - Guard featuresLog.Printf calls with Enabled() checks in features.go to avoid variadic interface boxing overhead when debug logging is disabled Reduces allocs/op from 6217 to ~5893 (-5.2%) in BenchmarkCompileSimpleWorkflow Agent-Logs-Url: https://github.com/github/gh-aw/sessions/5ea3e761-b229-49fb-87bf-7075c089bd2c Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com> --- pkg/workflow/claude_engine.go | 4 +++- pkg/workflow/claude_tools.go | 11 +++++++++-- pkg/workflow/features.go | 28 +++++++++++++++++++++------- 3 files changed, 33 insertions(+), 10 deletions(-) diff --git a/pkg/workflow/claude_engine.go b/pkg/workflow/claude_engine.go index 36e874c2034..7c3f4c7c613 100644 --- a/pkg/workflow/claude_engine.go +++ b/pkg/workflow/claude_engine.go @@ -423,7 +423,9 @@ func (e *ClaudeEngine) GetExecutionSteps(workflowData *WorkflowData, logFile str stepLines = append(stepLines, " id: agentic_execution") // Add allowed tools comment before the run section - allowedToolsComment := e.generateAllowedToolsComment(e.computeAllowedClaudeToolsString(toolsWithMountedCLIs, workflowData.SafeOutputs, workflowData.CacheMemoryConfig, workflowData.MCPScripts), " ") + // Reuse the already-computed allowedTools string (computed earlier for --allowed-tools flag) + // to avoid redundant allocations from calling computeAllowedClaudeToolsString twice. + allowedToolsComment := e.generateAllowedToolsComment(allowedTools, " ") if allowedToolsComment != "" { // Split the comment into lines and add each line commentLines := strings.Split(strings.TrimSuffix(allowedToolsComment, "\n"), "\n") diff --git a/pkg/workflow/claude_tools.go b/pkg/workflow/claude_tools.go index c41b5edbff7..759f348cb78 100644 --- a/pkg/workflow/claude_tools.go +++ b/pkg/workflow/claude_tools.go @@ -463,10 +463,17 @@ func (e *ClaudeEngine) generateAllowedToolsComment(allowedToolsStr string, inden return "" } + // Pre-size the builder to avoid reallocations: header line + one line per tool. + // Each tool line is: indent + "# - " + toolName + "\n" (~indent+6 bytes + tool length). var comment strings.Builder - comment.WriteString(indent + "# Allowed tools (sorted):\n") + comment.Grow(len(indent)*len(tools) + len("# Allowed tools (sorted):\n") + len(tools)*(len("# - \n")+16)) + comment.WriteString(indent) + comment.WriteString("# Allowed tools (sorted):\n") for _, tool := range tools { - fmt.Fprintf(&comment, "%s# - %s\n", indent, tool) + comment.WriteString(indent) + comment.WriteString("# - ") + comment.WriteString(tool) + comment.WriteByte('\n') } return comment.String() diff --git a/pkg/workflow/features.go b/pkg/workflow/features.go index f65300191a0..6dc49be3975 100644 --- a/pkg/workflow/features.go +++ b/pkg/workflow/features.go @@ -17,7 +17,9 @@ var featuresLog = logger.New("workflow:features") // If workflowData is nil or has no features, it falls back to checking the environment variable only. func isFeatureEnabled(flag constants.FeatureFlag, workflowData *WorkflowData) bool { flagLower := strings.ToLower(strings.TrimSpace(string(flag))) - featuresLog.Printf("Checking if feature is enabled: %s", flagLower) + if featuresLog.Enabled() { + featuresLog.Printf("Checking if feature is enabled: %s", flagLower) + } // First, check if the feature is explicitly set in frontmatter. // Frontmatter values always take precedence. @@ -27,11 +29,15 @@ func isFeatureEnabled(flag constants.FeatureFlag, workflowData *WorkflowData) bo // Fall back to checking the environment variable if isFeatureInEnvironment(flagLower) { - featuresLog.Printf("Feature found in GH_AW_FEATURES: %s=true", flagLower) + if featuresLog.Enabled() { + featuresLog.Printf("Feature found in GH_AW_FEATURES: %s=true", flagLower) + } return true } - featuresLog.Printf("Feature not found: %s=false", flagLower) + if featuresLog.Enabled() { + featuresLog.Printf("Feature not found: %s=false", flagLower) + } return false } @@ -42,7 +48,9 @@ func getFeatureValueFromFrontmatter(flagLower string, workflowData *WorkflowData if value, exists := workflowData.Features[flagLower]; exists { if enabled, found := parseFeatureValue(value); found { - featuresLog.Printf("Feature found in frontmatter: %s=%v", flagLower, enabled) + if featuresLog.Enabled() { + featuresLog.Printf("Feature found in frontmatter: %s=%v", flagLower, enabled) + } return enabled, true } } @@ -50,7 +58,9 @@ func getFeatureValueFromFrontmatter(flagLower string, workflowData *WorkflowData for key, value := range workflowData.Features { if strings.ToLower(key) == flagLower { if enabled, found := parseFeatureValue(value); found { - featuresLog.Printf("Feature found in frontmatter (case-insensitive): %s=%v", flagLower, enabled) + if featuresLog.Enabled() { + featuresLog.Printf("Feature found in frontmatter (case-insensitive): %s=%v", flagLower, enabled) + } return enabled, true } } @@ -72,11 +82,15 @@ func parseFeatureValue(value any) (bool, bool) { func isFeatureInEnvironment(flagLower string) bool { features := os.Getenv("GH_AW_FEATURES") if features == "" { - featuresLog.Printf("Feature not found, GH_AW_FEATURES empty: %s=false", flagLower) + if featuresLog.Enabled() { + featuresLog.Printf("Feature not found, GH_AW_FEATURES empty: %s=false", flagLower) + } return false } - featuresLog.Printf("Checking GH_AW_FEATURES environment variable: %s", features) + if featuresLog.Enabled() { + featuresLog.Printf("Checking GH_AW_FEATURES environment variable: %s", features) + } for feature := range strings.SplitSeq(features, ",") { if strings.ToLower(strings.TrimSpace(feature)) == flagLower { return true From a2103cb558d13ca076ac3fa8d845ef520f5d9915 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 25 Apr 2026 06:32:31 +0000 Subject: [PATCH 3/4] perf: document avgToolNameLen constant in generateAllowedToolsComment Address code review: replace magic number 16 with named constant avgToolNameLen with a comment explaining the rationale (typical Claude tool name lengths). Agent-Logs-Url: https://github.com/github/gh-aw/sessions/5ea3e761-b229-49fb-87bf-7075c089bd2c Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com> --- pkg/workflow/claude_tools.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/pkg/workflow/claude_tools.go b/pkg/workflow/claude_tools.go index 759f348cb78..3700a0ca58c 100644 --- a/pkg/workflow/claude_tools.go +++ b/pkg/workflow/claude_tools.go @@ -465,8 +465,11 @@ func (e *ClaudeEngine) generateAllowedToolsComment(allowedToolsStr string, inden // Pre-size the builder to avoid reallocations: header line + one line per tool. // Each tool line is: indent + "# - " + toolName + "\n" (~indent+6 bytes + tool length). + // avgToolNameLen is a conservative estimate for typical Claude tool names + // (e.g., "Bash(echo)" = 10 chars, "mcp__github__issue_read" = 23 chars). + const avgToolNameLen = 16 var comment strings.Builder - comment.Grow(len(indent)*len(tools) + len("# Allowed tools (sorted):\n") + len(tools)*(len("# - \n")+16)) + comment.Grow(len(indent)*len(tools) + len("# Allowed tools (sorted):\n") + len(tools)*(len("# - \n")+avgToolNameLen)) comment.WriteString(indent) comment.WriteString("# Allowed tools (sorted):\n") for _, tool := range tools { From ab095c5f4b505c3cc5ffabf01703e5a737be0941 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 25 Apr 2026 10:43:12 +0000 Subject: [PATCH 4/4] perf: address review feedback on Grow sizing and logEnabled caching - claude_tools.go: use exact output size in Grow() based on len(allowedToolsStr) instead of avgToolNameLen estimate; also include header indent in calculation - features.go: capture featuresLog.Enabled() once as logEnabled and thread it through getFeatureValueFromFrontmatter and isFeatureInEnvironment to avoid repeated calls and reduce repetition Agent-Logs-Url: https://github.com/github/gh-aw/sessions/0df3a808-8546-4b1f-b9a4-bd245c660aa4 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/workflow/claude_tools.go | 19 +++++++++++++------ pkg/workflow/features.go | 23 ++++++++++++----------- 2 files changed, 25 insertions(+), 17 deletions(-) diff --git a/pkg/workflow/claude_tools.go b/pkg/workflow/claude_tools.go index 3700a0ca58c..4d0450ca78d 100644 --- a/pkg/workflow/claude_tools.go +++ b/pkg/workflow/claude_tools.go @@ -463,13 +463,20 @@ func (e *ClaudeEngine) generateAllowedToolsComment(allowedToolsStr string, inden return "" } - // Pre-size the builder to avoid reallocations: header line + one line per tool. - // Each tool line is: indent + "# - " + toolName + "\n" (~indent+6 bytes + tool length). - // avgToolNameLen is a conservative estimate for typical Claude tool names - // (e.g., "Bash(echo)" = 10 chars, "mcp__github__issue_read" = 23 chars). - const avgToolNameLen = 16 + // Pre-size the builder using the exact output size: + // - header line: indent + "# Allowed tools (sorted):\n" + // - per tool: indent + "# - " + toolName + "\n" + // allowedToolsStr is comma-separated, so subtracting (len(tools)-1) gives the + // total bytes contributed by tool names alone. + toolNameBytes := len(allowedToolsStr) - (len(tools) - 1) var comment strings.Builder - comment.Grow(len(indent)*len(tools) + len("# Allowed tools (sorted):\n") + len(tools)*(len("# - \n")+avgToolNameLen)) + comment.Grow( + len(indent) + + len("# Allowed tools (sorted):\n") + + len(tools)*len(indent) + + len(tools)*len("# - \n") + + toolNameBytes, + ) comment.WriteString(indent) comment.WriteString("# Allowed tools (sorted):\n") for _, tool := range tools { diff --git a/pkg/workflow/features.go b/pkg/workflow/features.go index 6dc49be3975..bcee019ed62 100644 --- a/pkg/workflow/features.go +++ b/pkg/workflow/features.go @@ -17,38 +17,39 @@ var featuresLog = logger.New("workflow:features") // If workflowData is nil or has no features, it falls back to checking the environment variable only. func isFeatureEnabled(flag constants.FeatureFlag, workflowData *WorkflowData) bool { flagLower := strings.ToLower(strings.TrimSpace(string(flag))) - if featuresLog.Enabled() { + logEnabled := featuresLog.Enabled() + if logEnabled { featuresLog.Printf("Checking if feature is enabled: %s", flagLower) } // First, check if the feature is explicitly set in frontmatter. // Frontmatter values always take precedence. - if enabled, found := getFeatureValueFromFrontmatter(flagLower, workflowData); found { + if enabled, found := getFeatureValueFromFrontmatter(flagLower, workflowData, logEnabled); found { return enabled } // Fall back to checking the environment variable - if isFeatureInEnvironment(flagLower) { - if featuresLog.Enabled() { + if isFeatureInEnvironment(flagLower, logEnabled) { + if logEnabled { featuresLog.Printf("Feature found in GH_AW_FEATURES: %s=true", flagLower) } return true } - if featuresLog.Enabled() { + if logEnabled { featuresLog.Printf("Feature not found: %s=false", flagLower) } return false } -func getFeatureValueFromFrontmatter(flagLower string, workflowData *WorkflowData) (bool, bool) { +func getFeatureValueFromFrontmatter(flagLower string, workflowData *WorkflowData, logEnabled bool) (bool, bool) { if workflowData == nil || workflowData.Features == nil { return false, false } if value, exists := workflowData.Features[flagLower]; exists { if enabled, found := parseFeatureValue(value); found { - if featuresLog.Enabled() { + if logEnabled { featuresLog.Printf("Feature found in frontmatter: %s=%v", flagLower, enabled) } return enabled, true @@ -58,7 +59,7 @@ func getFeatureValueFromFrontmatter(flagLower string, workflowData *WorkflowData for key, value := range workflowData.Features { if strings.ToLower(key) == flagLower { if enabled, found := parseFeatureValue(value); found { - if featuresLog.Enabled() { + if logEnabled { featuresLog.Printf("Feature found in frontmatter (case-insensitive): %s=%v", flagLower, enabled) } return enabled, true @@ -79,16 +80,16 @@ func parseFeatureValue(value any) (bool, bool) { return false, false } -func isFeatureInEnvironment(flagLower string) bool { +func isFeatureInEnvironment(flagLower string, logEnabled bool) bool { features := os.Getenv("GH_AW_FEATURES") if features == "" { - if featuresLog.Enabled() { + if logEnabled { featuresLog.Printf("Feature not found, GH_AW_FEATURES empty: %s=false", flagLower) } return false } - if featuresLog.Enabled() { + if logEnabled { featuresLog.Printf("Checking GH_AW_FEATURES environment variable: %s", features) } for feature := range strings.SplitSeq(features, ",") {