From 0db0c87e015e9c36cf7acd6be9d522778e912776 Mon Sep 17 00:00:00 2001
From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com>
Date: Thu, 26 Feb 2026 02:44:24 +0000
Subject: [PATCH 1/4] Initial plan


From c643ba7005cd9e85133eff48ffe042b7dbb71137 Mon Sep 17 00:00:00 2001
From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com>
Date: Thu, 26 Feb 2026 02:50:03 +0000
Subject: [PATCH 2/4] Initial plan for semantic function clustering refactoring

Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
---
 pkg/workflow/data/action_pins.json | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/pkg/workflow/data/action_pins.json b/pkg/workflow/data/action_pins.json
index 8366ec146ee..6cc217598c4 100644
--- a/pkg/workflow/data/action_pins.json
+++ b/pkg/workflow/data/action_pins.json
@@ -110,6 +110,11 @@
       "version": "v6",
       "sha": "b7c566a772e6b6bfb58ed0dc250532a479d7789f"
     },
+    "anchore/sbom-action@v0": {
+      "repo": "anchore/sbom-action",
+      "version": "v0",
+      "sha": "17ae1740179002c89186b61233e0f892c3118b11"
+    },
     "anchore/sbom-action@v0.22.2": {
       "repo": "anchore/sbom-action",
       "version": "v0.22.2",

From b89c3812c46fad1be1b0ab70713f4e4c68ef8cea Mon Sep 17 00:00:00 2001
From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com>
Date: Thu, 26 Feb 2026 03:12:07 +0000
Subject: [PATCH 3/4] Implement semantic function clustering refactoring
 (Issues 1-8)

Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
---
 pkg/workflow/add_comment.go                   |  2 +-
 pkg/workflow/add_reviewer.go                  |  2 +-
 pkg/workflow/agent_validation.go              |  3 +-
 pkg/workflow/close_entity_helpers.go          |  2 +-
 pkg/workflow/compiler.go                      | 21 +++--
 pkg/workflow/compiler_orchestrator_tools.go   |  8 +-
 pkg/workflow/create_discussion.go             |  2 +-
 pkg/workflow/create_issue.go                  |  2 +-
 pkg/workflow/create_pull_request.go           |  2 +-
 .../dangerous_permissions_validation.go       | 21 +----
 pkg/workflow/engine_firewall_support.go       |  8 +-
 pkg/workflow/features_validation.go           | 12 ---
 pkg/workflow/firewall_validation.go           | 34 +++++++
 pkg/workflow/frontmatter_extraction_yaml.go   |  5 +-
 pkg/workflow/mcp_config_validation.go         | 10 +-
 pkg/workflow/network_firewall_validation.go   | 49 ----------
 pkg/workflow/npm_validation.go                | 12 +--
 pkg/workflow/package_extraction.go            | 65 +++++++++++++
 pkg/workflow/permissions_operations.go        | 16 ++++
 pkg/workflow/runtime_validation.go            | 92 +++----------------
 .../safe_outputs_target_validation.go         | 13 +++
 pkg/workflow/strict_mode_validation.go        | 25 +++--
 pkg/workflow/validation_helpers.go            | 44 ++++++---
 23 files changed, 224 insertions(+), 226 deletions(-)
 delete mode 100644 pkg/workflow/network_firewall_validation.go

diff --git a/pkg/workflow/add_comment.go b/pkg/workflow/add_comment.go
index 604aabb5832..15cf784cc14 100644
--- a/pkg/workflow/add_comment.go
+++ b/pkg/workflow/add_comment.go
@@ -183,7 +183,7 @@ func (c *Compiler) parseCommentsConfig(outputMap map[string]any) *AddCommentsCon
 	}
 
 	// Validate target-repo (wildcard "*" is not allowed)
-	if validateTargetRepoSlug(config.TargetRepoSlug, addCommentLog) {
+	if err := validateTargetRepoSlug(config.TargetRepoSlug, addCommentLog); err != nil {
 		return nil // Invalid configuration, return nil to cause validation error
 	}
 
diff --git a/pkg/workflow/add_reviewer.go b/pkg/workflow/add_reviewer.go
index 243d7346896..eadbf7b4233 100644
--- a/pkg/workflow/add_reviewer.go
+++ b/pkg/workflow/add_reviewer.go
@@ -45,7 +45,7 @@ func (c *Compiler) parseAddReviewerConfig(outputMap map[string]any) *AddReviewer
 	}
 
 	// Validate target-repo (wildcard "*" is not allowed for safe outputs)
-	if validateTargetRepoSlug(config.TargetRepoSlug, addReviewerLog) {
+	if err := validateTargetRepoSlug(config.TargetRepoSlug, addReviewerLog); err != nil {
 		return nil
 	}
 
diff --git a/pkg/workflow/agent_validation.go b/pkg/workflow/agent_validation.go
index 09af8fed5a4..6ad46dd5e4e 100644
--- a/pkg/workflow/agent_validation.go
+++ b/pkg/workflow/agent_validation.go
@@ -153,8 +153,7 @@ func (c *Compiler) validateWebSearchSupport(tools map[string]any, engine CodingA
 
 	// web-search is specified, check if the engine supports it
 	if !engine.SupportsWebSearch() {
-		fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Engine '%s' does not support the web-search tool. See https://github.github.com/gh-aw/guides/web-search/ for alternatives.", engine.GetID())))
-		c.IncrementWarningCount()
+		c.emitWarning(fmt.Sprintf("Engine '%s' does not support the web-search tool. See https://github.github.com/gh-aw/guides/web-search/ for alternatives.", engine.GetID()))
 	}
 }
 
diff --git a/pkg/workflow/close_entity_helpers.go b/pkg/workflow/close_entity_helpers.go
index e00d9f71445..46be93722f9 100644
--- a/pkg/workflow/close_entity_helpers.go
+++ b/pkg/workflow/close_entity_helpers.go
@@ -120,7 +120,7 @@ func (c *Compiler) parseCloseEntityConfig(outputMap map[string]any, params Close
 	}
 
 	// Validate target-repo (wildcard "*" is not allowed)
-	if validateTargetRepoSlug(config.TargetRepoSlug, logger) {
+	if err := validateTargetRepoSlug(config.TargetRepoSlug, logger); err != nil {
 		logger.Printf("Invalid target-repo slug for %s", params.ConfigKey)
 		return nil
 	}
diff --git a/pkg/workflow/compiler.go b/pkg/workflow/compiler.go
index 900b60290ba..998aa6c9c88 100644
--- a/pkg/workflow/compiler.go
+++ b/pkg/workflow/compiler.go
@@ -54,6 +54,12 @@ func formatCompilerMessage(filePath string, msgType string, message string) stri
 	})
 }
 
+// emitWarning prints a warning message to stderr and increments the warning count.
+func (c *Compiler) emitWarning(msg string) {
+	fmt.Fprintln(os.Stderr, console.FormatWarningMessage(msg))
+	c.IncrementWarningCount()
+}
+
 // CompileWorkflow compiles a workflow markdown file into a GitHub Actions YAML file.
 // It reads the file from disk, parses frontmatter and markdown sections, and generates
 // the corresponding workflow YAML. Returns the compiled workflow data or an error.
@@ -202,8 +208,7 @@ func (c *Compiler) validateWorkflowData(workflowData *WorkflowData, markdownPath
 
 	// Emit warning for sandbox.agent: false (disables agent sandbox firewall)
 	if isAgentSandboxDisabled(workflowData) {
-		fmt.Fprintln(os.Stderr, console.FormatWarningMessage("⚠️  WARNING: Agent sandbox disabled (sandbox.agent: false). This removes firewall protection. The AI agent will have direct network access without firewall filtering. The MCP gateway remains enabled. Only use this for testing or in controlled environments where you trust the AI agent completely."))
-		c.IncrementWarningCount()
+		c.emitWarning("⚠️  WARNING: Agent sandbox disabled (sandbox.agent: false). This removes firewall protection. The AI agent will have direct network access without firewall filtering. The MCP gateway remains enabled. Only use this for testing or in controlled environments where you trust the AI agent completely.")
 	}
 
 	// Validate: threat detection requires sandbox.agent to be enabled (detection runs inside AWF)
@@ -213,20 +218,17 @@ func (c *Compiler) validateWorkflowData(workflowData *WorkflowData, markdownPath
 
 	// Emit experimental warning for safe-inputs feature
 	if IsSafeInputsEnabled(workflowData.SafeInputs, workflowData) {
-		fmt.Fprintln(os.Stderr, console.FormatWarningMessage("Using experimental feature: safe-inputs"))
-		c.IncrementWarningCount()
+		c.emitWarning("Using experimental feature: safe-inputs")
 	}
 
 	// Emit experimental warning for plugins feature
 	if workflowData.PluginInfo != nil && len(workflowData.PluginInfo.Plugins) > 0 {
-		fmt.Fprintln(os.Stderr, console.FormatWarningMessage("Using experimental feature: plugins"))
-		c.IncrementWarningCount()
+		c.emitWarning("Using experimental feature: plugins")
 	}
 
 	// Emit experimental warning for rate-limit feature
 	if workflowData.RateLimit != nil {
-		fmt.Fprintln(os.Stderr, console.FormatWarningMessage("Using experimental feature: rate-limit"))
-		c.IncrementWarningCount()
+		c.emitWarning("Using experimental feature: rate-limit")
 	}
 
 	// Validate workflow_run triggers have branch restrictions
@@ -419,8 +421,7 @@ func (c *Compiler) generateAndValidateYAML(workflowData *WorkflowData, markdownP
 			return "", formatCompilerError(markdownPath, "error", fmt.Sprintf("repository feature validation failed: %v", err), err)
 		}
 	} else if c.verbose {
-		fmt.Fprintln(os.Stderr, console.FormatWarningMessage("Schema validation available but skipped (use SetSkipValidation(false) to enable)"))
-		c.IncrementWarningCount()
+		c.emitWarning("Schema validation available but skipped (use SetSkipValidation(false) to enable)")
 	}
 
 	return yamlContent, nil
diff --git a/pkg/workflow/compiler_orchestrator_tools.go b/pkg/workflow/compiler_orchestrator_tools.go
index 9924d57edbf..017c5d3d5d9 100644
--- a/pkg/workflow/compiler_orchestrator_tools.go
+++ b/pkg/workflow/compiler_orchestrator_tools.go
@@ -2,11 +2,9 @@ package workflow
 
 import (
 	"fmt"
-	"os"
 	"sort"
 	"strings"
 
-	"github.com/github/gh-aw/pkg/console"
 	"github.com/github/gh-aw/pkg/logger"
 	"github.com/github/gh-aw/pkg/parser"
 	"github.com/goccy/go-yaml"
@@ -217,11 +215,9 @@ func (c *Compiler) processToolsAndMarkdown(result *parser.FrontmatterResult, cle
 
 	if !agenticEngine.SupportsToolsAllowlist() {
 		// For engines that don't support tool allowlists (like custom engine), ignore tools section and provide warnings
-		fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Using experimental %s support (engine: %s)", agenticEngine.GetDisplayName(), agenticEngine.GetID())))
-		c.IncrementWarningCount()
+		c.emitWarning(fmt.Sprintf("Using experimental %s support (engine: %s)", agenticEngine.GetDisplayName(), agenticEngine.GetID()))
 		if _, hasTools := result.Frontmatter["tools"]; hasTools {
-			fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("'tools' section ignored when using engine: %s (%s doesn't support MCP tool allow-listing)", agenticEngine.GetID(), agenticEngine.GetDisplayName())))
-			c.IncrementWarningCount()
+			c.emitWarning(fmt.Sprintf("'tools' section ignored when using engine: %s (%s doesn't support MCP tool allow-listing)", agenticEngine.GetID(), agenticEngine.GetDisplayName()))
 		}
 		tools = map[string]any{}
 		// For now, we'll add a basic github tool (always uses docker MCP)
diff --git a/pkg/workflow/create_discussion.go b/pkg/workflow/create_discussion.go
index c88ff54944a..4281487842f 100644
--- a/pkg/workflow/create_discussion.go
+++ b/pkg/workflow/create_discussion.go
@@ -86,7 +86,7 @@ func (c *Compiler) parseDiscussionsConfig(outputMap map[string]any) *CreateDiscu
 	}
 
 	// Validate target-repo (wildcard "*" is not allowed)
-	if validateTargetRepoSlug(config.TargetRepoSlug, discussionLog) {
+	if err := validateTargetRepoSlug(config.TargetRepoSlug, discussionLog); err != nil {
 		return nil // Invalid configuration, return nil to cause validation error
 	}
 
diff --git a/pkg/workflow/create_issue.go b/pkg/workflow/create_issue.go
index aaccca04ef2..bef5bd749b7 100644
--- a/pkg/workflow/create_issue.go
+++ b/pkg/workflow/create_issue.go
@@ -79,7 +79,7 @@ func (c *Compiler) parseIssuesConfig(outputMap map[string]any) *CreateIssuesConf
 	}
 
 	// Validate target-repo (wildcard "*" is not allowed)
-	if validateTargetRepoSlug(config.TargetRepoSlug, createIssueLog) {
+	if err := validateTargetRepoSlug(config.TargetRepoSlug, createIssueLog); err != nil {
 		return nil // Invalid configuration, return nil to cause validation error
 	}
 
diff --git a/pkg/workflow/create_pull_request.go b/pkg/workflow/create_pull_request.go
index d0e6e474e60..4f2e6a4e82f 100644
--- a/pkg/workflow/create_pull_request.go
+++ b/pkg/workflow/create_pull_request.go
@@ -307,7 +307,7 @@ func (c *Compiler) parsePullRequestsConfig(outputMap map[string]any) *CreatePull
 	}
 
 	// Validate target-repo (wildcard "*" is not allowed)
-	if validateTargetRepoSlug(config.TargetRepoSlug, createPRLog) {
+	if err := validateTargetRepoSlug(config.TargetRepoSlug, createPRLog); err != nil {
 		return nil // Invalid configuration, return nil to cause validation error
 	}
 
diff --git a/pkg/workflow/dangerous_permissions_validation.go b/pkg/workflow/dangerous_permissions_validation.go
index c38a01f17e1..1932b23e9a4 100644
--- a/pkg/workflow/dangerous_permissions_validation.go
+++ b/pkg/workflow/dangerous_permissions_validation.go
@@ -62,27 +62,16 @@ func findWritePermissions(permissions *Permissions) []PermissionScope {
 		return nil
 	}
 
-	var writePerms []PermissionScope
-
-	// Check all permission scopes
+	// Collect scopes to check, excluding id-token and metadata
+	var scopes []PermissionScope
 	for _, scope := range GetAllPermissionScopes() {
-		// Skip id-token as it's safe and used for OIDC authentication
-		if scope == PermissionIdToken {
-			continue
-		}
-
-		// Skip metadata as it's a built-in read-only permission
-		if scope == PermissionMetadata {
+		if scope == PermissionIdToken || scope == PermissionMetadata {
 			continue
 		}
-
-		level, exists := permissions.Get(scope)
-		if exists && level == PermissionWrite {
-			writePerms = append(writePerms, scope)
-		}
+		scopes = append(scopes, scope)
 	}
 
-	return writePerms
+	return findWriteScopesForPolicy(permissions, scopes)
 }
 
 // formatDangerousPermissionsError formats an error message for write permissions violations
diff --git a/pkg/workflow/engine_firewall_support.go b/pkg/workflow/engine_firewall_support.go
index 71df4cc9625..1164745b5ae 100644
--- a/pkg/workflow/engine_firewall_support.go
+++ b/pkg/workflow/engine_firewall_support.go
@@ -3,9 +3,7 @@ package workflow
 import (
 	"errors"
 	"fmt"
-	"os"
 
-	"github.com/github/gh-aw/pkg/console"
 	"github.com/github/gh-aw/pkg/logger"
 )
 
@@ -79,8 +77,7 @@ func (c *Compiler) checkNetworkSupport(engine CodingAgentEngine, networkPermissi
 	}
 
 	// In non-strict mode, emit a warning
-	fmt.Fprintln(os.Stderr, console.FormatWarningMessage(message))
-	c.IncrementWarningCount()
+	c.emitWarning(message)
 
 	return nil
 }
@@ -107,8 +104,7 @@ func (c *Compiler) checkFirewallDisable(engine CodingAgentEngine, networkPermiss
 			}
 
 			// In non-strict mode, emit a warning
-			fmt.Fprintln(os.Stderr, console.FormatWarningMessage(message))
-			c.IncrementWarningCount()
+			c.emitWarning(message)
 		}
 
 		// Also check if engine doesn't support firewall in strict mode when there are no restrictions
diff --git a/pkg/workflow/features_validation.go b/pkg/workflow/features_validation.go
index 9f074dbe133..becfe877ae2 100644
--- a/pkg/workflow/features_validation.go
+++ b/pkg/workflow/features_validation.go
@@ -11,7 +11,6 @@
 //
 //   - validateFeatures() - Validates all feature flags in WorkflowData
 //   - validateActionTag() - Validates action-tag is a full SHA
-//   - isValidFullSHA() - Checks if a string is a valid 40-character SHA
 //
 // # When to Add Validation Here
 //
@@ -24,15 +23,12 @@ package workflow
 
 import (
 	"fmt"
-	"regexp"
 
 	"github.com/github/gh-aw/pkg/logger"
 )
 
 var featuresValidationLog = logger.New("workflow:features_validation")
 
-var shaRegex = regexp.MustCompile("^[0-9a-f]{40}$")
-
 // validateFeatures validates all feature flags in the workflow data
 func validateFeatures(data *WorkflowData) error {
 	if data == nil || data.Features == nil {
@@ -91,11 +87,3 @@ func validateActionTag(value any) error {
 
 	return nil
 }
-
-// isValidFullSHA checks if a string is a valid 40-character hexadecimal SHA
-func isValidFullSHA(s string) bool {
-	if len(s) != 40 {
-		return false
-	}
-	return shaRegex.MatchString(s)
-}
diff --git a/pkg/workflow/firewall_validation.go b/pkg/workflow/firewall_validation.go
index d41e78c8416..f47bff61023 100644
--- a/pkg/workflow/firewall_validation.go
+++ b/pkg/workflow/firewall_validation.go
@@ -3,6 +3,7 @@
 // This file contains domain-specific validation functions for firewall configuration:
 //   - validateFirewallConfig() - Validates the overall firewall configuration
 //   - ValidateLogLevel() - Validates firewall log-level values
+//   - validateNetworkFirewallConfig() - Validates firewall configuration dependencies
 //
 // These validation functions are organized in a dedicated file following the validation
 // architecture pattern where domain-specific validation belongs in domain validation files.
@@ -14,6 +15,7 @@ import (
 	"fmt"
 	"slices"
 
+	"github.com/github/gh-aw/pkg/constants"
 	"github.com/github/gh-aw/pkg/logger"
 )
 
@@ -59,3 +61,35 @@ func ValidateLogLevel(level string) error {
 	firewallValidationLog.Printf("Invalid log-level: %s", level)
 	return fmt.Errorf("invalid log-level '%s', must be one of: %v", level, valid)
 }
+
+// validateNetworkFirewallConfig validates network firewall configuration dependencies
+// Returns an error if the configuration is invalid
+func validateNetworkFirewallConfig(networkPermissions *NetworkPermissions) error {
+	if networkPermissions == nil {
+		return nil
+	}
+
+	firewallConfig := networkPermissions.Firewall
+	if firewallConfig == nil {
+		return nil
+	}
+
+	firewallValidationLog.Print("Validating network firewall configuration")
+
+	// Validate allow-urls requires ssl-bump
+	if len(firewallConfig.AllowURLs) > 0 && !firewallConfig.SSLBump {
+		firewallValidationLog.Printf("Validation error: allow-urls specified without ssl-bump: %d URLs", len(firewallConfig.AllowURLs))
+		return NewValidationError(
+			"network.firewall.allow-urls",
+			"requires ssl-bump: true",
+			"allow-urls requires ssl-bump: true to function. SSL Bump enables HTTPS content inspection, which is necessary for URL path filtering",
+			"Enable SSL Bump in your firewall configuration:\n\nnetwork:\n  firewall:\n    ssl-bump: true\n    allow-urls:\n      - \"https://github.com/githubnext/*\"\n\nSee: "+string(constants.DocsNetworkURL),
+		)
+	}
+
+	if len(firewallConfig.AllowURLs) > 0 {
+		firewallValidationLog.Printf("Validated allow-urls: %d URLs with ssl-bump enabled", len(firewallConfig.AllowURLs))
+	}
+
+	return nil
+}
diff --git a/pkg/workflow/frontmatter_extraction_yaml.go b/pkg/workflow/frontmatter_extraction_yaml.go
index 96ecf00b6dc..0c7459aedfc 100644
--- a/pkg/workflow/frontmatter_extraction_yaml.go
+++ b/pkg/workflow/frontmatter_extraction_yaml.go
@@ -2,11 +2,9 @@ package workflow
 
 import (
 	"fmt"
-	"os"
 	"strconv"
 	"strings"
 
-	"github.com/github/gh-aw/pkg/console"
 	"github.com/github/gh-aw/pkg/logger"
 	"github.com/github/gh-aw/pkg/parser"
 	"github.com/goccy/go-yaml"
@@ -638,8 +636,7 @@ func (c *Compiler) extractCommandConfig(frontmatter map[string]any) (commandName
 			if hasCommand {
 				// Show deprecation warning if using old field name
 				if isDeprecated {
-					fmt.Fprintln(os.Stderr, console.FormatWarningMessage("The 'command:' trigger field is deprecated. Please use 'slash_command:' instead."))
-					c.IncrementWarningCount()
+					c.emitWarning("The 'command:' trigger field is deprecated. Please use 'slash_command:' instead.")
 				}
 
 				// Check if command is a string (shorthand format)
diff --git a/pkg/workflow/mcp_config_validation.go b/pkg/workflow/mcp_config_validation.go
index 04f375bcaca..bfc145491e8 100644
--- a/pkg/workflow/mcp_config_validation.go
+++ b/pkg/workflow/mcp_config_validation.go
@@ -312,14 +312,8 @@ func validateMCPMountsSyntax(toolName string, mountsRaw any) error {
 		return fmt.Errorf("tool '%s' mcp configuration 'mounts' must be an array of strings.\n\nExample:\ntools:\n  %s:\n    container: \"my-registry/my-tool\"\n    mounts:\n      - \"/host/path:/container/path:ro\"\n\nSee: %s", toolName, toolName, constants.DocsToolsURL)
 	}
 
-	for i, mount := range mounts {
-		source, dest, mode, err := validateMountStringFormat(mount)
-		if err != nil {
-			if source == "" && dest == "" && mode == "" {
-				return fmt.Errorf("tool '%s' mcp configuration mounts[%d] must follow 'source:destination:mode' format, got: %q.\n\nExample:\ntools:\n  %s:\n    container: \"my-registry/my-tool\"\n    mounts:\n      - \"/host/path:/container/path:ro\"\n\nSee: %s", toolName, i, mount, toolName, constants.DocsToolsURL)
-			}
-			return fmt.Errorf("tool '%s' mcp configuration mounts[%d] mode must be 'ro' or 'rw', got: %q.\n\nExample:\ntools:\n  %s:\n    container: \"my-registry/my-tool\"\n    mounts:\n      - \"/host/path:/container/path:ro\"  # read-only\n      - \"/host/path:/container/path:rw\"  # read-write\n\nSee: %s", toolName, i, mode, toolName, constants.DocsToolsURL)
-		}
+	if errs := validateMountStrings(mounts, string(constants.DocsToolsURL)); len(errs) > 0 {
+		return fmt.Errorf("tool '%s' mcp configuration %s\n\nExample:\ntools:\n  %s:\n    container: \"my-registry/my-tool\"\n    mounts:\n      - \"/host/path:/container/path:ro\"\n      - \"/host/path:/container/path:rw\"", toolName, errs[0], toolName)
 	}
 
 	return nil
diff --git a/pkg/workflow/network_firewall_validation.go b/pkg/workflow/network_firewall_validation.go
deleted file mode 100644
index 0ba6453fb98..00000000000
--- a/pkg/workflow/network_firewall_validation.go
+++ /dev/null
@@ -1,49 +0,0 @@
-// This file provides network firewall validation functions for agentic workflow compilation.
-//
-// This file contains domain-specific validation functions for network firewall configuration:
-//   - validateNetworkFirewallConfig() - Validates firewall configuration dependencies
-//
-// These validation functions are organized in a dedicated file following the validation
-// architecture pattern where domain-specific validation belongs in domain validation files.
-// See validation.go for the complete validation architecture documentation.
-
-package workflow
-
-import (
-	"github.com/github/gh-aw/pkg/constants"
-	"github.com/github/gh-aw/pkg/logger"
-)
-
-var networkFirewallValidationLog = logger.New("workflow:network_firewall_validation")
-
-// validateNetworkFirewallConfig validates network firewall configuration dependencies
-// Returns an error if the configuration is invalid
-func validateNetworkFirewallConfig(networkPermissions *NetworkPermissions) error {
-	if networkPermissions == nil {
-		return nil
-	}
-
-	firewallConfig := networkPermissions.Firewall
-	if firewallConfig == nil {
-		return nil
-	}
-
-	networkFirewallValidationLog.Print("Validating network firewall configuration")
-
-	// Validate allow-urls requires ssl-bump
-	if len(firewallConfig.AllowURLs) > 0 && !firewallConfig.SSLBump {
-		networkFirewallValidationLog.Printf("Validation error: allow-urls specified without ssl-bump: %d URLs", len(firewallConfig.AllowURLs))
-		return NewValidationError(
-			"network.firewall.allow-urls",
-			"requires ssl-bump: true",
-			"allow-urls requires ssl-bump: true to function. SSL Bump enables HTTPS content inspection, which is necessary for URL path filtering",
-			"Enable SSL Bump in your firewall configuration:\n\nnetwork:\n  firewall:\n    ssl-bump: true\n    allow-urls:\n      - \"https://github.com/githubnext/*\"\n\nSee: "+string(constants.DocsNetworkURL),
-		)
-	}
-
-	if len(firewallConfig.AllowURLs) > 0 {
-		networkFirewallValidationLog.Printf("Validated allow-urls: %d URLs with ssl-bump enabled", len(firewallConfig.AllowURLs))
-	}
-
-	return nil
-}
diff --git a/pkg/workflow/npm_validation.go b/pkg/workflow/npm_validation.go
index 59ba7e31b00..91c79b2715a 100644
--- a/pkg/workflow/npm_validation.go
+++ b/pkg/workflow/npm_validation.go
@@ -68,7 +68,7 @@ func (c *Compiler) validateNpxPackages(workflowData *WorkflowData) error {
 		)
 	}
 
-	var errors []string
+	collector := NewErrorCollector(false)
 	for _, pkg := range packages {
 		npmValidationLog.Printf("Validating npm package: %s", pkg)
 
@@ -78,7 +78,7 @@ func (c *Compiler) validateNpxPackages(workflowData *WorkflowData) error {
 
 		if err != nil {
 			npmValidationLog.Printf("Package validation failed for %s: %v", pkg, err)
-			errors = append(errors, fmt.Sprintf("npx package '%s' not found on npm registry: %s", pkg, strings.TrimSpace(string(output))))
+			_ = collector.Add(fmt.Errorf("npx package '%s' not found on npm registry: %s", pkg, strings.TrimSpace(string(output))))
 		} else {
 			npmValidationLog.Printf("Package validated successfully: %s", pkg)
 			if c.verbose {
@@ -87,13 +87,13 @@ func (c *Compiler) validateNpxPackages(workflowData *WorkflowData) error {
 		}
 	}
 
-	if len(errors) > 0 {
-		npmValidationLog.Printf("npx package validation failed with %d errors", len(errors))
+	if collector.HasErrors() {
+		npmValidationLog.Printf("npx package validation failed with %d errors", collector.Count())
 		return NewValidationError(
 			"npx.packages",
-			fmt.Sprintf("%d packages not found", len(errors)),
+			fmt.Sprintf("%d packages not found", collector.Count()),
 			"npx packages not found on npm registry",
-			fmt.Sprintf("Fix package names or verify they exist on npm:\n\n%s\n\nCheck package availability:\n$ npm view <package-name>\n\nSearch for similar packages:\n$ npm search <keyword>", strings.Join(errors, "\n")),
+			fmt.Sprintf("Fix package names or verify they exist on npm:\n\n%s\n\nCheck package availability:\n$ npm view <package-name>\n\nSearch for similar packages:\n$ npm search <keyword>", collector.Error().Error()),
 		)
 	}
 
diff --git a/pkg/workflow/package_extraction.go b/pkg/workflow/package_extraction.go
index ced416490ec..ad5098c035e 100644
--- a/pkg/workflow/package_extraction.go
+++ b/pkg/workflow/package_extraction.go
@@ -340,3 +340,68 @@ func (pe *PackageExtractor) findPackageName(words []string, startIndex int) stri
 	}
 	return ""
 }
+
+// collectPackagesFromWorkflow is a generic helper that collects packages from workflow data
+// using the provided extractor function. It deduplicates packages and optionally extracts
+// from MCP tool configurations when toolCommand is provided.
+func collectPackagesFromWorkflow(
+	workflowData *WorkflowData,
+	extractor func(string) []string,
+	toolCommand string,
+) []string {
+	pkgLog.Printf("Collecting packages from workflow: toolCommand=%s", toolCommand)
+	var packages []string
+	seen := make(map[string]bool)
+
+	// Extract from custom steps
+	if workflowData.CustomSteps != "" {
+		pkgs := extractor(workflowData.CustomSteps)
+		for _, pkg := range pkgs {
+			if !seen[pkg] {
+				packages = append(packages, pkg)
+				seen[pkg] = true
+			}
+		}
+	}
+
+	// Extract from MCP server configurations (if toolCommand is provided)
+	if toolCommand != "" && workflowData.Tools != nil {
+		for _, toolConfig := range workflowData.Tools {
+			// Handle structured MCP config with command and args fields
+			if config, ok := toolConfig.(map[string]any); ok {
+				if command, hasCommand := config["command"]; hasCommand {
+					if cmdStr, ok := command.(string); ok && cmdStr == toolCommand {
+						// Extract package from args, skipping flags
+						if args, hasArgs := config["args"]; hasArgs {
+							if argsSlice, ok := args.([]any); ok {
+								for _, arg := range argsSlice {
+									if pkgStr, ok := arg.(string); ok {
+										// Skip flags (arguments starting with - or --)
+										if !strings.HasPrefix(pkgStr, "-") && !seen[pkgStr] {
+											packages = append(packages, pkgStr)
+											seen[pkgStr] = true
+											break // Only take the first non-flag argument
+										}
+									}
+								}
+							}
+						}
+					}
+				}
+			} else if cmdStr, ok := toolConfig.(string); ok {
+				// Handle string-format MCP tool (e.g., "npx -y package")
+				// Use the extractor function to parse the command string
+				pkgs := extractor(cmdStr)
+				for _, pkg := range pkgs {
+					if !seen[pkg] {
+						packages = append(packages, pkg)
+						seen[pkg] = true
+					}
+				}
+			}
+		}
+	}
+
+	pkgLog.Printf("Collected %d unique packages", len(packages))
+	return packages
+}
diff --git a/pkg/workflow/permissions_operations.go b/pkg/workflow/permissions_operations.go
index dfc54df035b..c42017d80d8 100644
--- a/pkg/workflow/permissions_operations.go
+++ b/pkg/workflow/permissions_operations.go
@@ -276,3 +276,19 @@ func (p *Permissions) RenderToYAML() string {
 
 	return strings.Join(lines, "\n")
 }
+
+// findWriteScopesForPolicy returns the subset of the given scopes that have write-level access.
+// This is a shared helper for write-permission policy checks across validation functions.
+func findWriteScopesForPolicy(permissions *Permissions, scopes []PermissionScope) []PermissionScope {
+	if permissions == nil {
+		return nil
+	}
+	var writeScopes []PermissionScope
+	for _, scope := range scopes {
+		level, exists := permissions.Get(scope)
+		if exists && level == PermissionWrite {
+			writeScopes = append(writeScopes, scope)
+		}
+	}
+	return writeScopes
+}
diff --git a/pkg/workflow/runtime_validation.go b/pkg/workflow/runtime_validation.go
index 6106a52c6b3..38d2df21b77 100644
--- a/pkg/workflow/runtime_validation.go
+++ b/pkg/workflow/runtime_validation.go
@@ -13,7 +13,6 @@
 //   - validateExpressionSizes() - Validates expression size limits (21KB max)
 //   - validateContainerImages() - Validates Docker images exist
 //   - validateRuntimePackages() - Validates npm, pip, uv packages
-//   - collectPackagesFromWorkflow() - Generic package collection helper
 //
 // # Validation Patterns
 //
@@ -95,7 +94,7 @@ func (c *Compiler) validateContainerImages(workflowData *WorkflowData) error {
 	}
 
 	runtimeValidationLog.Printf("Validating container images for %d tools", len(workflowData.Tools))
-	var errors []string
+	collector := NewErrorCollector(false)
 	for toolName, toolConfig := range workflowData.Tools {
 		if config, ok := toolConfig.(map[string]any); ok {
 			// Get the MCP configuration to extract container info
@@ -122,7 +121,7 @@ func (c *Compiler) validateContainerImages(workflowData *WorkflowData) error {
 
 				// Validate the container image exists using docker
 				if err := validateDockerImage(containerImage, c.verbose); err != nil {
-					errors = append(errors, fmt.Sprintf("tool '%s': %v", toolName, err))
+					_ = collector.Add(fmt.Errorf("tool '%s': %w", toolName, err))
 				} else if c.verbose {
 					fmt.Fprintln(os.Stderr, console.FormatInfoMessage("✓ Container image validated: "+containerImage))
 				}
@@ -130,12 +129,12 @@ func (c *Compiler) validateContainerImages(workflowData *WorkflowData) error {
 		}
 	}
 
-	if len(errors) > 0 {
+	if collector.HasErrors() {
 		return NewValidationError(
 			"container.images",
-			fmt.Sprintf("%d images failed validation", len(errors)),
+			fmt.Sprintf("%d images failed validation", collector.Count()),
 			"container image validation failed",
-			fmt.Sprintf("Fix the following container image issues:\n\n%s\n\nEnsure:\n1. Container images exist and are accessible\n2. Registry URLs are correct\n3. Image tags are specified\n4. You have pull permissions for private images", strings.Join(errors, "\n")),
+			fmt.Sprintf("Fix the following container image issues:\n\n%s\n\nEnsure:\n1. Container images exist and are accessible\n2. Registry URLs are correct\n3. Image tags are specified\n4. You have pull permissions for private images", collector.Error().Error()),
 		)
 	}
 
@@ -149,7 +148,7 @@ func (c *Compiler) validateRuntimePackages(workflowData *WorkflowData) error {
 	requirements := DetectRuntimeRequirements(workflowData)
 	runtimeValidationLog.Printf("Validating runtime packages: found %d runtime requirements", len(requirements))
 
-	var errors []string
+	collector := NewErrorCollector(false)
 	for _, req := range requirements {
 		switch req.Runtime.ID {
 		case "node":
@@ -157,100 +156,35 @@ func (c *Compiler) validateRuntimePackages(workflowData *WorkflowData) error {
 			runtimeValidationLog.Print("Validating npx packages")
 			if err := c.validateNpxPackages(workflowData); err != nil {
 				runtimeValidationLog.Printf("Npx package validation failed: %v", err)
-				errors = append(errors, err.Error())
+				_ = collector.Add(err)
 			}
 		case "python":
 			// Validate pip packages used in the workflow
 			runtimeValidationLog.Print("Validating pip packages")
 			if err := c.validatePipPackages(workflowData); err != nil {
 				runtimeValidationLog.Printf("Pip package validation failed: %v", err)
-				errors = append(errors, err.Error())
+				_ = collector.Add(err)
 			}
 		case "uv":
 			// Validate uv packages used in the workflow
 			runtimeValidationLog.Print("Validating uv packages")
 			if err := c.validateUvPackages(workflowData); err != nil {
 				runtimeValidationLog.Printf("Uv package validation failed: %v", err)
-				errors = append(errors, err.Error())
+				_ = collector.Add(err)
 			}
 		}
 	}
 
-	if len(errors) > 0 {
-		runtimeValidationLog.Printf("Runtime package validation completed with %d errors", len(errors))
+	if collector.HasErrors() {
+		runtimeValidationLog.Printf("Runtime package validation completed with %d errors", collector.Count())
 		return NewValidationError(
 			"runtime.packages",
-			fmt.Sprintf("%d package validation errors", len(errors)),
+			fmt.Sprintf("%d package validation errors", collector.Count()),
 			"runtime package validation failed",
-			fmt.Sprintf("Fix the following package issues:\n\n%s\n\nEnsure:\n1. Package names are spelled correctly\n2. Packages exist in their respective registries (npm, PyPI)\n3. Package managers (npm, pip, uv) are installed\n4. Network access is available for registry checks", strings.Join(errors, "\n")),
+			fmt.Sprintf("Fix the following package issues:\n\n%s\n\nEnsure:\n1. Package names are spelled correctly\n2. Packages exist in their respective registries (npm, PyPI)\n3. Package managers (npm, pip, uv) are installed\n4. Network access is available for registry checks", collector.Error().Error()),
 		)
 	}
 
 	runtimeValidationLog.Print("Runtime package validation passed")
 	return nil
 }
-
-// collectPackagesFromWorkflow is a generic helper that collects packages from workflow data
-// using the provided extractor function. It deduplicates packages and optionally extracts
-// from MCP tool configurations when toolCommand is provided.
-func collectPackagesFromWorkflow(
-	workflowData *WorkflowData,
-	extractor func(string) []string,
-	toolCommand string,
-) []string {
-	runtimeValidationLog.Printf("Collecting packages from workflow: toolCommand=%s", toolCommand)
-	var packages []string
-	seen := make(map[string]bool)
-
-	// Extract from custom steps
-	if workflowData.CustomSteps != "" {
-		pkgs := extractor(workflowData.CustomSteps)
-		for _, pkg := range pkgs {
-			if !seen[pkg] {
-				packages = append(packages, pkg)
-				seen[pkg] = true
-			}
-		}
-	}
-
-	// Extract from MCP server configurations (if toolCommand is provided)
-	if toolCommand != "" && workflowData.Tools != nil {
-		for _, toolConfig := range workflowData.Tools {
-			// Handle structured MCP config with command and args fields
-			if config, ok := toolConfig.(map[string]any); ok {
-				if command, hasCommand := config["command"]; hasCommand {
-					if cmdStr, ok := command.(string); ok && cmdStr == toolCommand {
-						// Extract package from args, skipping flags
-						if args, hasArgs := config["args"]; hasArgs {
-							if argsSlice, ok := args.([]any); ok {
-								for _, arg := range argsSlice {
-									if pkgStr, ok := arg.(string); ok {
-										// Skip flags (arguments starting with - or --)
-										if !strings.HasPrefix(pkgStr, "-") && !seen[pkgStr] {
-											packages = append(packages, pkgStr)
-											seen[pkgStr] = true
-											break // Only take the first non-flag argument
-										}
-									}
-								}
-							}
-						}
-					}
-				}
-			} else if cmdStr, ok := toolConfig.(string); ok {
-				// Handle string-format MCP tool (e.g., "npx -y package")
-				// Use the extractor function to parse the command string
-				pkgs := extractor(cmdStr)
-				for _, pkg := range pkgs {
-					if !seen[pkg] {
-						packages = append(packages, pkg)
-						seen[pkg] = true
-					}
-				}
-			}
-		}
-	}
-
-	runtimeValidationLog.Printf("Collected %d unique packages", len(packages))
-	return packages
-}
diff --git a/pkg/workflow/safe_outputs_target_validation.go b/pkg/workflow/safe_outputs_target_validation.go
index 686245b339c..abbc67850c2 100644
--- a/pkg/workflow/safe_outputs_target_validation.go
+++ b/pkg/workflow/safe_outputs_target_validation.go
@@ -1,6 +1,7 @@
 package workflow
 
 import (
+	"errors"
 	"fmt"
 	"strings"
 
@@ -159,3 +160,15 @@ func isGitHubExpression(s string) bool {
 	// and there must be something between them
 	return openIndex >= 0 && closeIndex > openIndex+3
 }
+
+// validateTargetRepoSlug validates that a target-repo slug is not a wildcard.
+// Returns an error if the value equals "*", which is not allowed for safe outputs.
+func validateTargetRepoSlug(targetRepoSlug string, log *logger.Logger) error {
+	if targetRepoSlug == "*" {
+		if log != nil {
+			log.Print("Invalid target-repo: wildcard '*' is not allowed")
+		}
+		return errors.New("target-repo wildcard '*' is not allowed for safe outputs")
+	}
+	return nil
+}
diff --git a/pkg/workflow/strict_mode_validation.go b/pkg/workflow/strict_mode_validation.go
index 373cad48415..bd73b964ce5 100644
--- a/pkg/workflow/strict_mode_validation.go
+++ b/pkg/workflow/strict_mode_validation.go
@@ -63,15 +63,22 @@ func (c *Compiler) validateStrictPermissions(frontmatter map[string]any) error {
 	}
 
 	// Parse permissions using the PermissionsParser
-	perms := NewPermissionsParserFromValue(permissionsValue)
-
-	// Check for write permissions on sensitive scopes
-	writePermissions := []string{"contents", "issues", "pull-requests"}
-	for _, scope := range writePermissions {
-		if perms.IsAllowed(scope, "write") {
-			strictModeValidationLog.Printf("Write permission validation failed: scope=%s", scope)
-			return fmt.Errorf("strict mode: write permission '%s: write' is not allowed for security reasons. Use 'safe-outputs.create-issue', 'safe-outputs.create-pull-request', 'safe-outputs.add-comment', or 'safe-outputs.update-issue' to perform write operations safely. See: https://github.github.com/gh-aw/reference/safe-outputs/", scope)
-		}
+	permissions := NewPermissionsParserFromValue(permissionsValue).ToPermissions()
+	if permissions == nil {
+		strictModeValidationLog.Printf("No permissions to validate, validation passed")
+		return nil
+	}
+
+	// Check for write permissions on sensitive scopes using the shared policy helper
+	sensitiveScopes := []PermissionScope{
+		PermissionContents,
+		PermissionIssues,
+		PermissionPullRequests,
+	}
+	writeScopes := findWriteScopesForPolicy(permissions, sensitiveScopes)
+	for _, scope := range writeScopes {
+		strictModeValidationLog.Printf("Write permission validation failed: scope=%s", scope)
+		return fmt.Errorf("strict mode: write permission '%s: write' is not allowed for security reasons. Use 'safe-outputs.create-issue', 'safe-outputs.create-pull-request', 'safe-outputs.add-comment', or 'safe-outputs.update-issue' to perform write operations safely. See: https://github.github.com/gh-aw/reference/safe-outputs/", scope)
 	}
 
 	strictModeValidationLog.Printf("Permissions validation passed")
diff --git a/pkg/workflow/validation_helpers.go b/pkg/workflow/validation_helpers.go
index 86a84bb3e31..72e1735b2ad 100644
--- a/pkg/workflow/validation_helpers.go
+++ b/pkg/workflow/validation_helpers.go
@@ -14,6 +14,7 @@
 //   - ValidatePositiveInt() - Validates that a value is a positive integer
 //   - ValidateNonNegativeInt() - Validates that a value is a non-negative integer
 //   - validateMountStringFormat() - Parses and validates a "source:dest:mode" mount string
+//   - isValidFullSHA() - Checks if a string is a valid 40-character hexadecimal SHA
 //
 // # Design Rationale
 //
@@ -31,6 +32,7 @@ package workflow
 import (
 	"errors"
 	"fmt"
+	"regexp"
 	"slices"
 	"strconv"
 	"strings"
@@ -40,6 +42,16 @@ import (
 
 var validationHelpersLog = logger.New("workflow:validation_helpers")
 
+var shaRegex = regexp.MustCompile("^[0-9a-f]{40}$")
+
+// isValidFullSHA checks if a string is a valid 40-character hexadecimal SHA
+func isValidFullSHA(s string) bool {
+	if len(s) != 40 {
+		return false
+	}
+	return shaRegex.MatchString(s)
+}
+
 // validateIntRange validates that a value is within the specified inclusive range [min, max].
 // It returns an error if the value is outside the range, with a descriptive message
 // including the field name and the actual value.
@@ -166,6 +178,25 @@ func validateMountStringFormat(mount string) (source, dest, mode string, err err
 	return parts[0], parts[1], parts[2], nil
 }
 
+// validateMountStrings validates a list of mount strings against the expected format.
+// It returns a slice of human-readable error strings describing any invalid mounts.
+// The docsURL is included in error messages to help users find documentation.
+// Both sandbox and MCP mount validation share this core loop.
+func validateMountStrings(mounts []string, docsURL string) []string {
+	var errs []string
+	for i, mount := range mounts {
+		source, dest, mode, err := validateMountStringFormat(mount)
+		if err != nil {
+			if source == "" && dest == "" && mode == "" {
+				errs = append(errs, fmt.Sprintf("mounts[%d] %q must follow 'source:destination:mode' format. See: %s", i, mount, docsURL))
+			} else {
+				errs = append(errs, fmt.Sprintf("mounts[%d] mode must be 'ro' or 'rw', got %q. See: %s", i, mode, docsURL))
+			}
+		}
+	}
+	return errs
+}
+
 // formatList formats a list of strings as a comma-separated list with natural language conjunction
 func formatList(items []string) string {
 	if len(items) == 0 {
@@ -179,16 +210,3 @@ func formatList(items []string) string {
 	}
 	return fmt.Sprintf("%s, and %s", formatList(items[:len(items)-1]), items[len(items)-1])
 }
-
-// validateTargetRepoSlug validates that a target-repo slug is not a wildcard.
-// Returns true if the value is invalid (i.e., equals "*").
-// This helper is used when the target-repo has already been parsed into a struct field.
-func validateTargetRepoSlug(targetRepoSlug string, log *logger.Logger) bool {
-	if targetRepoSlug == "*" {
-		if log != nil {
-			log.Print("Invalid target-repo: wildcard '*' is not allowed")
-		}
-		return true // Return true to indicate validation error
-	}
-	return false
-}

From d7c80c76774debade11fc37e3ee36b30e5670b68 Mon Sep 17 00:00:00 2001
From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com>
Date: Thu, 26 Feb 2026 03:16:00 +0000
Subject: [PATCH 4/4] Address code review: remove unnecessary nil check in
 validateStrictPermissions

Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
---
 pkg/workflow/strict_mode_validation.go | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/pkg/workflow/strict_mode_validation.go b/pkg/workflow/strict_mode_validation.go
index bd73b964ce5..627e22b1989 100644
--- a/pkg/workflow/strict_mode_validation.go
+++ b/pkg/workflow/strict_mode_validation.go
@@ -64,10 +64,6 @@ func (c *Compiler) validateStrictPermissions(frontmatter map[string]any) error {
 
 	// Parse permissions using the PermissionsParser
 	permissions := NewPermissionsParserFromValue(permissionsValue).ToPermissions()
-	if permissions == nil {
-		strictModeValidationLog.Printf("No permissions to validate, validation passed")
-		return nil
-	}
 
 	// Check for write permissions on sensitive scopes using the shared policy helper
 	sensitiveScopes := []PermissionScope{