Standardize error wrapping in compiler to preserve error chains (#14435)

Copilot · web-flow · commit 867aa72a4eb3 · 2026-02-07T18:32:59.000-08:00
diff --git a/pkg/workflow/agent_validation.go b/pkg/workflow/agent_validation.go
@@ -84,11 +84,11 @@ func (c *Compiler) validateAgentFile(workflowData *WorkflowData, markdownPath st
 	if _, err := os.Stat(fullAgentPath); err != nil {
 		if os.IsNotExist(err) {
 			return formatCompilerError(markdownPath, "error",
-				fmt.Sprintf("agent file '%s' does not exist. Ensure the file exists in the repository and is properly imported.", agentPath))
+				fmt.Sprintf("agent file '%s' does not exist. Ensure the file exists in the repository and is properly imported.", agentPath), nil)
 		}
 		// Other error (permissions, etc.)
 		return formatCompilerError(markdownPath, "error",
-			fmt.Sprintf("failed to access agent file '%s': %v", agentPath, err))
+			fmt.Sprintf("failed to access agent file '%s': %v", agentPath, err), err)
 	}
 
 	if c.verbose {
@@ -228,7 +228,7 @@ func (c *Compiler) validateWorkflowRunBranches(workflowData *WorkflowData, markd
 
 	if c.strictMode {
 		// In strict mode, this is an error
-		return formatCompilerError(markdownPath, "error", message)
+		return formatCompilerError(markdownPath, "error", message, nil)
 	}
 
 	// In normal mode, this is a warning
diff --git a/pkg/workflow/compiler.go b/pkg/workflow/compiler.go
@@ -37,11 +37,12 @@ const (
 //go:embed schemas/github-workflow.json
 var githubWorkflowSchema string
 
-// formatCompilerError creates a formatted compiler error message
+// formatCompilerError creates a formatted compiler error message with optional error wrapping
 // filePath: the file path to include in the error (typically markdownPath or lockFile)
 // errType: the error type ("error" or "warning")
 // message: the error message text
-func formatCompilerError(filePath string, errType string, message string) error {
+// cause: optional underlying error to wrap (use nil for validation errors)
+func formatCompilerError(filePath string, errType string, message string, cause error) error {
 	formattedErr := console.FormatError(console.CompilerError{
 		Position: console.ErrorPosition{
 			File:   filePath,
@@ -51,6 +52,13 @@ func formatCompilerError(filePath string, errType string, message string) error
 		Type:    errType,
 		Message: message,
 	})
+
+	// Wrap the underlying error if provided (preserves error chain)
+	if cause != nil {
+		return fmt.Errorf("%s: %w", formattedErr, cause)
+	}
+
+	// Create new error for validation errors (no underlying cause)
 	return errors.New(formattedErr)
 }
 
@@ -84,8 +92,8 @@ func (c *Compiler) CompileWorkflow(markdownPath string) error {
 			// Already formatted, return as-is
 			return err
 		}
-		// Otherwise, create a basic formatted error
-		return formatCompilerError(markdownPath, "error", err.Error())
+		// Otherwise, create a basic formatted error with wrapping
+		return formatCompilerError(markdownPath, "error", err.Error(), err)
 	}
 
 	return c.CompileWorkflowData(workflowData, markdownPath)
@@ -97,7 +105,7 @@ func (c *Compiler) validateWorkflowData(workflowData *WorkflowData, markdownPath
 	// Validate expression safety - check that all GitHub Actions expressions are in the allowed list
 	log.Printf("Validating expression safety")
 	if err := validateExpressionSafety(workflowData.MarkdownContent); err != nil {
-		return formatCompilerError(markdownPath, "error", err.Error())
+		return formatCompilerError(markdownPath, "error", err.Error(), err)
 	}
 
 	// Validate expressions in runtime-import files at compile time
@@ -107,13 +115,13 @@ func (c *Compiler) validateWorkflowData(workflowData *WorkflowData, markdownPath
 	githubDir := filepath.Dir(workflowDir)    // .github
 	workspaceDir := filepath.Dir(githubDir)   // repo root
 	if err := validateRuntimeImportFiles(workflowData.MarkdownContent, workspaceDir); err != nil {
-		return formatCompilerError(markdownPath, "error", err.Error())
+		return formatCompilerError(markdownPath, "error", err.Error(), err)
 	}
 
 	// Validate feature flags
 	log.Printf("Validating feature flags")
 	if err := validateFeatures(workflowData); err != nil {
-		return formatCompilerError(markdownPath, "error", err.Error())
+		return formatCompilerError(markdownPath, "error", err.Error(), err)
 	}
 
 	// Check for action-mode feature flag override
@@ -122,7 +130,7 @@ func (c *Compiler) validateWorkflowData(workflowData *WorkflowData, markdownPath
 			if actionModeStr, ok := actionModeVal.(string); ok && actionModeStr != "" {
 				mode := ActionMode(actionModeStr)
 				if !mode.IsValid() {
-					return formatCompilerError(markdownPath, "error", fmt.Sprintf("invalid action-mode feature flag '%s'. Must be 'dev', 'release', or 'script'", actionModeStr))
+					return formatCompilerError(markdownPath, "error", fmt.Sprintf("invalid action-mode feature flag '%s'. Must be 'dev', 'release', or 'script'", actionModeStr), nil)
 				}
 				log.Printf("Overriding action mode from feature flag: %s", mode)
 				c.SetActionMode(mode)
@@ -133,7 +141,7 @@ func (c *Compiler) validateWorkflowData(workflowData *WorkflowData, markdownPath
 	// Validate dangerous permissions
 	log.Printf("Validating dangerous permissions")
 	if err := validateDangerousPermissions(workflowData); err != nil {
-		return formatCompilerError(markdownPath, "error", err.Error())
+		return formatCompilerError(markdownPath, "error", err.Error(), err)
 	}
 
 	// Validate agent file exists if specified in engine config
@@ -145,25 +153,25 @@ func (c *Compiler) validateWorkflowData(workflowData *WorkflowData, markdownPath
 	// Validate sandbox configuration
 	log.Printf("Validating sandbox configuration")
 	if err := validateSandboxConfig(workflowData); err != nil {
-		return formatCompilerError(markdownPath, "error", err.Error())
+		return formatCompilerError(markdownPath, "error", err.Error(), err)
 	}
 
 	// Validate safe-outputs target configuration
 	log.Printf("Validating safe-outputs target fields")
 	if err := validateSafeOutputsTarget(workflowData.SafeOutputs); err != nil {
-		return formatCompilerError(markdownPath, "error", err.Error())
+		return formatCompilerError(markdownPath, "error", err.Error(), err)
 	}
 
 	// Validate safe-outputs allowed-domains configuration
 	log.Printf("Validating safe-outputs allowed-domains")
 	if err := c.validateSafeOutputsAllowedDomains(workflowData.SafeOutputs); err != nil {
-		return formatCompilerError(markdownPath, "error", err.Error())
+		return formatCompilerError(markdownPath, "error", err.Error(), err)
 	}
 
 	// Validate network allowed domains configuration
 	log.Printf("Validating network allowed domains")
 	if err := c.validateNetworkAllowedDomains(workflowData.NetworkPermissions); err != nil {
-		return formatCompilerError(markdownPath, "error", err.Error())
+		return formatCompilerError(markdownPath, "error", err.Error(), err)
 	}
 
 	// Emit experimental warning for sandbox-runtime feature
@@ -207,7 +215,7 @@ func (c *Compiler) validateWorkflowData(workflowData *WorkflowData, markdownPath
 			if len(validationResult.MissingPermissions) > 0 {
 				if c.strictMode {
 					// In strict mode, missing permissions are errors
-					return formatCompilerError(markdownPath, "error", message)
+					return formatCompilerError(markdownPath, "error", message, nil)
 				} else {
 					// In non-strict mode, missing permissions are warnings
 					fmt.Fprintln(os.Stderr, formatCompilerMessage(markdownPath, "warning", message))
@@ -226,7 +234,7 @@ func (c *Compiler) validateWorkflowData(workflowData *WorkflowData, markdownPath
 
 		// Validate that all allowed tools have their toolsets enabled
 		if err := ValidateGitHubToolsAgainstToolsets(allowedTools, enabledToolsets); err != nil {
-			return formatCompilerError(markdownPath, "error", err.Error())
+			return formatCompilerError(markdownPath, "error", err.Error(), err)
 		}
 
 		// Print informational message if "projects" toolset is explicitly specified
@@ -258,14 +266,14 @@ func (c *Compiler) validateWorkflowData(workflowData *WorkflowData, markdownPath
 			message += "permissions:\n"
 			message += "  actions: read"
 
-			return formatCompilerError(markdownPath, "error", message)
+			return formatCompilerError(markdownPath, "error", message, nil)
 		}
 	}
 
 	// Validate dispatch-workflow configuration (independent of agentic-workflows tool)
 	log.Print("Validating dispatch-workflow configuration")
 	if err := c.validateDispatchWorkflow(workflowData, markdownPath); err != nil {
-		return formatCompilerError(markdownPath, "error", fmt.Sprintf("dispatch-workflow validation failed: %v", err))
+		return formatCompilerError(markdownPath, "error", fmt.Sprintf("dispatch-workflow validation failed: %v", err), err)
 	}
 
 	return nil
@@ -277,15 +285,15 @@ func (c *Compiler) generateAndValidateYAML(workflowData *WorkflowData, markdownP
 	// Generate the YAML content
 	yamlContent, err := c.generateYAML(workflowData, markdownPath)
 	if err != nil {
-		return "", formatCompilerError(markdownPath, "error", fmt.Sprintf("failed to generate YAML: %v", err))
+		return "", formatCompilerError(markdownPath, "error", fmt.Sprintf("failed to generate YAML: %v", err), err)
 	}
 
 	// Always validate expression sizes - this is a hard limit from GitHub Actions (21KB)
 	// that cannot be bypassed, so we validate it unconditionally
 	log.Print("Validating expression sizes")
 	if err := c.validateExpressionSizes(yamlContent); err != nil {
 		// Store error first so we can write invalid YAML before returning
-		formattedErr := formatCompilerError(markdownPath, "error", fmt.Sprintf("expression size validation failed: %v", err))
+		formattedErr := formatCompilerError(markdownPath, "error", fmt.Sprintf("expression size validation failed: %v", err), err)
 		// Write the invalid YAML to a .invalid.yml file for inspection
 		invalidFile := strings.TrimSuffix(lockFile, ".lock.yml") + ".invalid.yml"
 		if writeErr := os.WriteFile(invalidFile, []byte(yamlContent), 0644); writeErr == nil {
@@ -298,7 +306,7 @@ func (c *Compiler) generateAndValidateYAML(workflowData *WorkflowData, markdownP
 	log.Print("Validating for template injection vulnerabilities")
 	if err := validateNoTemplateInjection(yamlContent); err != nil {
 		// Store error first so we can write invalid YAML before returning
-		formattedErr := formatCompilerError(markdownPath, "error", err.Error())
+		formattedErr := formatCompilerError(markdownPath, "error", err.Error(), err)
 		// Write the invalid YAML to a .invalid.yml file for inspection
 		invalidFile := strings.TrimSuffix(lockFile, ".lock.yml") + ".invalid.yml"
 		if writeErr := os.WriteFile(invalidFile, []byte(yamlContent), 0644); writeErr == nil {
@@ -312,7 +320,7 @@ func (c *Compiler) generateAndValidateYAML(workflowData *WorkflowData, markdownP
 		log.Print("Validating workflow against GitHub Actions schema")
 		if err := c.validateGitHubActionsSchema(yamlContent); err != nil {
 			// Store error first so we can write invalid YAML before returning
-			formattedErr := formatCompilerError(markdownPath, "error", fmt.Sprintf("workflow schema validation failed: %v", err))
+			formattedErr := formatCompilerError(markdownPath, "error", fmt.Sprintf("workflow schema validation failed: %v", err), err)
 			// Write the invalid YAML to a .invalid.yml file for inspection
 			invalidFile := strings.TrimSuffix(lockFile, ".lock.yml") + ".invalid.yml"
 			if writeErr := os.WriteFile(invalidFile, []byte(yamlContent), 0644); writeErr == nil {
@@ -333,19 +341,19 @@ func (c *Compiler) generateAndValidateYAML(workflowData *WorkflowData, markdownP
 		// Validate runtime packages (npx, uv)
 		log.Print("Validating runtime packages")
 		if err := c.validateRuntimePackages(workflowData); err != nil {
-			return "", formatCompilerError(markdownPath, "error", fmt.Sprintf("runtime package validation failed: %v", err))
+			return "", formatCompilerError(markdownPath, "error", fmt.Sprintf("runtime package validation failed: %v", err), err)
 		}
 
 		// Validate firewall configuration (log-level enum)
 		log.Print("Validating firewall configuration")
 		if err := c.validateFirewallConfig(workflowData); err != nil {
-			return "", formatCompilerError(markdownPath, "error", fmt.Sprintf("firewall configuration validation failed: %v", err))
+			return "", formatCompilerError(markdownPath, "error", fmt.Sprintf("firewall configuration validation failed: %v", err), err)
 		}
 
 		// Validate repository features (discussions, issues)
 		log.Print("Validating repository features")
 		if err := c.validateRepositoryFeatures(workflowData); err != nil {
-			return "", formatCompilerError(markdownPath, "error", fmt.Sprintf("repository feature validation failed: %v", err))
+			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)"))
@@ -377,7 +385,7 @@ func (c *Compiler) writeWorkflowOutput(lockFile, yamlContent string, markdownPat
 		// Only write if content has changed
 		if !contentUnchanged {
 			if err := os.WriteFile(lockFile, []byte(yamlContent), 0644); err != nil {
-				return formatCompilerError(lockFile, "error", fmt.Sprintf("failed to write lock file: %v", err))
+				return formatCompilerError(lockFile, "error", fmt.Sprintf("failed to write lock file: %v", err), err)
 			}
 			log.Print("Lock file written successfully")
 		}
diff --git a/pkg/workflow/compiler_error_formatting_test.go b/pkg/workflow/compiler_error_formatting_test.go
@@ -3,6 +3,7 @@
 package workflow
 
 import (
+	"fmt"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -16,13 +17,15 @@ func TestFormatCompilerError(t *testing.T) {
 		filePath    string
 		errType     string
 		message     string
+		cause       error
 		wantContain []string
 	}{
 		{
-			name:     "error type with simple message",
+			name:     "error type with simple message, no cause",
 			filePath: "/path/to/workflow.md",
 			errType:  "error",
 			message:  "validation failed",
+			cause:    nil,
 			wantContain: []string{
 				"/path/to/workflow.md",
 				"1:1",
@@ -31,22 +34,37 @@ func TestFormatCompilerError(t *testing.T) {
 			},
 		},
 		{
-			name:     "warning type with detailed message",
+			name:     "warning type with detailed message, no cause",
 			filePath: "/path/to/workflow.md",
 			errType:  "warning",
 			message:  "missing required permission",
+			cause:    nil,
 			wantContain: []string{
 				"/path/to/workflow.md",
 				"1:1",
 				"warning",
 				"missing required permission",
 			},
 		},
+		{
+			name:     "error with underlying cause",
+			filePath: "/path/to/workflow.md",
+			errType:  "error",
+			message:  "failed to parse YAML",
+			cause:    fmt.Errorf("syntax error at line 42"),
+			wantContain: []string{
+				"/path/to/workflow.md",
+				"1:1",
+				"error",
+				"failed to parse YAML",
+			},
+		},
 		{
 			name:     "lock file path",
 			filePath: "/path/to/workflow.lock.yml",
 			errType:  "error",
 			message:  "failed to write lock file",
+			cause:    nil,
 			wantContain: []string{
 				"/path/to/workflow.lock.yml",
 				"1:1",
@@ -55,10 +73,11 @@ func TestFormatCompilerError(t *testing.T) {
 			},
 		},
 		{
-			name:     "formatted message with error details",
+			name:     "formatted message with error details and cause",
 			filePath: "test.md",
 			errType:  "error",
 			message:  "failed to generate YAML: syntax error",
+			cause:    fmt.Errorf("underlying error"),
 			wantContain: []string{
 				"test.md",
 				"1:1",
@@ -70,20 +89,25 @@ func TestFormatCompilerError(t *testing.T) {
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			err := formatCompilerError(tt.filePath, tt.errType, tt.message)
+			err := formatCompilerError(tt.filePath, tt.errType, tt.message, tt.cause)
 			require.Error(t, err, "formatCompilerError should return an error")
 
 			errStr := err.Error()
 			for _, want := range tt.wantContain {
 				assert.Contains(t, errStr, want, "Error message should contain: %s", want)
 			}
+
+			// If cause is provided, verify error wrapping
+			if tt.cause != nil {
+				assert.ErrorIs(t, err, tt.cause, "Error should wrap the cause")
+			}
 		})
 	}
 }
 
 // TestFormatCompilerError_OutputFormat verifies the output format remains consistent
 func TestFormatCompilerError_OutputFormat(t *testing.T) {
-	err := formatCompilerError("/test/workflow.md", "error", "test message")
+	err := formatCompilerError("/test/workflow.md", "error", "test message", nil)
 	require.Error(t, err)
 
 	errStr := err.Error()
@@ -97,8 +121,8 @@ func TestFormatCompilerError_OutputFormat(t *testing.T) {
 
 // TestFormatCompilerError_ErrorVsWarning tests differentiation between error and warning types
 func TestFormatCompilerError_ErrorVsWarning(t *testing.T) {
-	errorErr := formatCompilerError("test.md", "error", "error message")
-	warningErr := formatCompilerError("test.md", "warning", "warning message")
+	errorErr := formatCompilerError("test.md", "error", "error message", nil)
+	warningErr := formatCompilerError("test.md", "warning", "warning message", nil)
 
 	require.Error(t, errorErr)
 	require.Error(t, warningErr)
@@ -155,3 +179,37 @@ func TestFormatCompilerMessage(t *testing.T) {
 		})
 	}
 }
+
+// TestFormatCompilerError_ErrorWrapping verifies that error wrapping preserves error chains
+func TestFormatCompilerError_ErrorWrapping(t *testing.T) {
+	// Create an underlying error
+	underlyingErr := fmt.Errorf("underlying validation error")
+
+	// Wrap it with formatCompilerError
+	wrappedErr := formatCompilerError("test.md", "error", "validation failed", underlyingErr)
+
+	require.Error(t, wrappedErr)
+
+	// Verify error chain is preserved
+	require.ErrorIs(t, wrappedErr, underlyingErr, "Should preserve error chain with %w")
+
+	// Verify formatted message is in the error string
+	assert.Contains(t, wrappedErr.Error(), "test.md")
+	assert.Contains(t, wrappedErr.Error(), "validation failed")
+}
+
+// TestFormatCompilerError_NilCause verifies that nil cause creates a new error
+func TestFormatCompilerError_NilCause(t *testing.T) {
+	err := formatCompilerError("test.md", "error", "validation error", nil)
+
+	require.Error(t, err)
+
+	// Verify error message contains expected content
+	assert.Contains(t, err.Error(), "test.md")
+	assert.Contains(t, err.Error(), "validation error")
+
+	// Verify it's a new error (not wrapping anything)
+	// This is a validation error, so it should not wrap
+	dummyErr := fmt.Errorf("some other error")
+	assert.NotErrorIs(t, err, dummyErr, "Should not wrap any error when cause is nil")
+}
diff --git a/pkg/workflow/compiler_orchestrator_frontmatter.go b/pkg/workflow/compiler_orchestrator_frontmatter.go

Original file line number	Diff line number	Diff line change
`@@ -84,11 +84,11 @@ func (c Compiler) validateAgentFile(workflowData WorkflowData, markdownPath st`
`84`	`84`	`if _, err := os.Stat(fullAgentPath); err != nil {`
`85`	`85`	`if os.IsNotExist(err) {`
`86`	`86`	`return formatCompilerError(markdownPath, "error",`
`87`		`- fmt.Sprintf("agent file '%s' does not exist. Ensure the file exists in the repository and is properly imported.", agentPath))`
	`87`	`+ fmt.Sprintf("agent file '%s' does not exist. Ensure the file exists in the repository and is properly imported.", agentPath), nil)`
`88`	`88`	`}`
`89`	`89`	`// Other error (permissions, etc.)`
`90`	`90`	`return formatCompilerError(markdownPath, "error",`
`91`		`- fmt.Sprintf("failed to access agent file '%s': %v", agentPath, err))`
	`91`	`+ fmt.Sprintf("failed to access agent file '%s': %v", agentPath, err), err)`
`92`	`92`	`}`
`93`	`93`
`94`	`94`	`if c.verbose {`
`@@ -228,7 +228,7 @@ func (c Compiler) validateWorkflowRunBranches(workflowData WorkflowData, markd`
`228`	`228`
`229`	`229`	`if c.strictMode {`
`230`	`230`	`// In strict mode, this is an error`
`231`		`- return formatCompilerError(markdownPath, "error", message)`
	`231`	`+ return formatCompilerError(markdownPath, "error", message, nil)`
`232`	`232`	`}`
`233`	`233`
`234`	`234`	`// In normal mode, this is a warning`