diff --git a/pkg/workflow/action_pins_test.go b/pkg/workflow/action_pins_test.go index 0cc597224b..d4a8db8e09 100644 --- a/pkg/workflow/action_pins_test.go +++ b/pkg/workflow/action_pins_test.go @@ -1595,8 +1595,8 @@ func TestSliceToStepsWithActionPinning(t *testing.T) { wantCount: 3, }, { - name: "empty steps slice", - steps: []any{}, + name: "empty steps slice", + steps: []any{}, wantErr: false, wantCount: 0, }, diff --git a/pkg/workflow/compiler.go b/pkg/workflow/compiler.go index abcd1dbea7..0fbc9ad6c9 100644 --- a/pkg/workflow/compiler.go +++ b/pkg/workflow/compiler.go @@ -37,6 +37,39 @@ const ( //go:embed schemas/github-workflow.json var githubWorkflowSchema string +// formatCompilerError creates a formatted compiler error message +// 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 { + formattedErr := console.FormatError(console.CompilerError{ + Position: console.ErrorPosition{ + File: filePath, + Line: 1, + Column: 1, + }, + Type: errType, + Message: message, + }) + return errors.New(formattedErr) +} + +// formatCompilerMessage creates a formatted compiler message string (for warnings printed to stderr) +// filePath: the file path to include in the message (typically markdownPath or lockFile) +// msgType: the message type ("error" or "warning") +// message: the message text +func formatCompilerMessage(filePath string, msgType string, message string) string { + return console.FormatError(console.CompilerError{ + Position: console.ErrorPosition{ + File: filePath, + Line: 1, + Column: 1, + }, + Type: msgType, + Message: message, + }) +} + // CompileWorkflow converts a markdown workflow to GitHub Actions YAML func (c *Compiler) CompileWorkflow(markdownPath string) error { // Store markdownPath for use in dynamic tool generation @@ -52,16 +85,7 @@ func (c *Compiler) CompileWorkflow(markdownPath string) error { return err } // Otherwise, create a basic formatted error - formattedErr := console.FormatError(console.CompilerError{ - Position: console.ErrorPosition{ - File: markdownPath, - Line: 1, - Column: 1, - }, - Type: "error", - Message: err.Error(), - }) - return errors.New(formattedErr) + return formatCompilerError(markdownPath, "error", err.Error()) } return c.CompileWorkflowData(workflowData, markdownPath) @@ -102,16 +126,7 @@ func (c *Compiler) CompileWorkflowData(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 { - formattedErr := console.FormatError(console.CompilerError{ - Position: console.ErrorPosition{ - File: markdownPath, - Line: 1, - Column: 1, - }, - Type: "error", - Message: err.Error(), - }) - return errors.New(formattedErr) + return formatCompilerError(markdownPath, "error", err.Error()) } // Validate expressions in runtime-import files at compile time @@ -121,31 +136,13 @@ func (c *Compiler) CompileWorkflowData(workflowData *WorkflowData, markdownPath githubDir := filepath.Dir(workflowDir) // .github workspaceDir := filepath.Dir(githubDir) // repo root if err := validateRuntimeImportFiles(workflowData.MarkdownContent, workspaceDir); err != nil { - formattedErr := console.FormatError(console.CompilerError{ - Position: console.ErrorPosition{ - File: markdownPath, - Line: 1, - Column: 1, - }, - Type: "error", - Message: err.Error(), - }) - return errors.New(formattedErr) + return formatCompilerError(markdownPath, "error", err.Error()) } // Validate feature flags log.Printf("Validating feature flags") if err := validateFeatures(workflowData); err != nil { - formattedErr := console.FormatError(console.CompilerError{ - Position: console.ErrorPosition{ - File: markdownPath, - Line: 1, - Column: 1, - }, - Type: "error", - Message: err.Error(), - }) - return errors.New(formattedErr) + return formatCompilerError(markdownPath, "error", err.Error()) } // Check for action-mode feature flag override @@ -154,16 +151,7 @@ func (c *Compiler) CompileWorkflowData(workflowData *WorkflowData, markdownPath if actionModeStr, ok := actionModeVal.(string); ok && actionModeStr != "" { mode := ActionMode(actionModeStr) if !mode.IsValid() { - formattedErr := console.FormatError(console.CompilerError{ - Position: console.ErrorPosition{ - File: markdownPath, - Line: 1, - Column: 1, - }, - Type: "error", - Message: fmt.Sprintf("invalid action-mode feature flag '%s'. Must be 'dev', 'release', or 'script'", actionModeStr), - }) - return errors.New(formattedErr) + return formatCompilerError(markdownPath, "error", fmt.Sprintf("invalid action-mode feature flag '%s'. Must be 'dev', 'release', or 'script'", actionModeStr)) } log.Printf("Overriding action mode from feature flag: %s", mode) c.SetActionMode(mode) @@ -174,16 +162,7 @@ func (c *Compiler) CompileWorkflowData(workflowData *WorkflowData, markdownPath // Validate dangerous permissions log.Printf("Validating dangerous permissions") if err := validateDangerousPermissions(workflowData); err != nil { - formattedErr := console.FormatError(console.CompilerError{ - Position: console.ErrorPosition{ - File: markdownPath, - Line: 1, - Column: 1, - }, - Type: "error", - Message: err.Error(), - }) - return errors.New(formattedErr) + return formatCompilerError(markdownPath, "error", err.Error()) } // Validate agent file exists if specified in engine config @@ -195,61 +174,25 @@ func (c *Compiler) CompileWorkflowData(workflowData *WorkflowData, markdownPath // Validate sandbox configuration log.Printf("Validating sandbox configuration") if err := validateSandboxConfig(workflowData); err != nil { - formattedErr := console.FormatError(console.CompilerError{ - Position: console.ErrorPosition{ - File: markdownPath, - Line: 1, - Column: 1, - }, - Type: "error", - Message: err.Error(), - }) - return errors.New(formattedErr) + return formatCompilerError(markdownPath, "error", err.Error()) } // Validate safe-outputs target configuration log.Printf("Validating safe-outputs target fields") if err := validateSafeOutputsTarget(workflowData.SafeOutputs); err != nil { - formattedErr := console.FormatError(console.CompilerError{ - Position: console.ErrorPosition{ - File: markdownPath, - Line: 1, - Column: 1, - }, - Type: "error", - Message: err.Error(), - }) - return errors.New(formattedErr) + return formatCompilerError(markdownPath, "error", err.Error()) } // Validate safe-outputs allowed-domains configuration log.Printf("Validating safe-outputs allowed-domains") if err := validateSafeOutputsAllowedDomains(workflowData.SafeOutputs); err != nil { - formattedErr := console.FormatError(console.CompilerError{ - Position: console.ErrorPosition{ - File: markdownPath, - Line: 1, - Column: 1, - }, - Type: "error", - Message: err.Error(), - }) - return errors.New(formattedErr) + return formatCompilerError(markdownPath, "error", err.Error()) } // Validate network allowed domains configuration log.Printf("Validating network allowed domains") if err := validateNetworkAllowedDomains(workflowData.NetworkPermissions); err != nil { - formattedErr := console.FormatError(console.CompilerError{ - Position: console.ErrorPosition{ - File: markdownPath, - Line: 1, - Column: 1, - }, - Type: "error", - Message: err.Error(), - }) - return errors.New(formattedErr) + return formatCompilerError(markdownPath, "error", err.Error()) } // Emit experimental warning for sandbox-runtime feature @@ -302,28 +245,10 @@ func (c *Compiler) CompileWorkflowData(workflowData *WorkflowData, markdownPath if len(validationResult.MissingPermissions) > 0 { if c.strictMode { // In strict mode, missing permissions are errors - formattedErr := console.FormatError(console.CompilerError{ - Position: console.ErrorPosition{ - File: markdownPath, - Line: 1, - Column: 1, - }, - Type: "error", - Message: message, - }) - return errors.New(formattedErr) + return formatCompilerError(markdownPath, "error", message) } else { // In non-strict mode, missing permissions are warnings - formattedWarning := console.FormatError(console.CompilerError{ - Position: console.ErrorPosition{ - File: markdownPath, - Line: 1, - Column: 1, - }, - Type: "warning", - Message: message, - }) - fmt.Fprintln(os.Stderr, formattedWarning) + fmt.Fprintln(os.Stderr, formatCompilerMessage(markdownPath, "warning", message)) c.IncrementWarningCount() } } @@ -339,16 +264,7 @@ func (c *Compiler) CompileWorkflowData(workflowData *WorkflowData, markdownPath // Validate that all allowed tools have their toolsets enabled if err := ValidateGitHubToolsAgainstToolsets(allowedTools, enabledToolsets); err != nil { - formattedErr := console.FormatError(console.CompilerError{ - Position: console.ErrorPosition{ - File: markdownPath, - Line: 1, - Column: 1, - }, - Type: "error", - Message: err.Error(), - }) - return errors.New(formattedErr) + return formatCompilerError(markdownPath, "error", err.Error()) } // Print informational message if "projects" toolset is explicitly specified @@ -380,31 +296,13 @@ func (c *Compiler) CompileWorkflowData(workflowData *WorkflowData, markdownPath message += "permissions:\n" message += " actions: read" - formattedErr := console.FormatError(console.CompilerError{ - Position: console.ErrorPosition{ - File: markdownPath, - Line: 1, - Column: 1, - }, - Type: "error", - Message: message, - }) - return errors.New(formattedErr) + return formatCompilerError(markdownPath, "error", message) } // Validate dispatch-workflow configuration log.Print("Validating dispatch-workflow configuration") if err := c.validateDispatchWorkflow(workflowData, markdownPath); err != nil { - formattedErr := console.FormatError(console.CompilerError{ - Position: console.ErrorPosition{ - File: markdownPath, - Line: 1, - Column: 1, - }, - Type: "error", - Message: fmt.Sprintf("dispatch-workflow validation failed: %v", err), - }) - return errors.New(formattedErr) + return formatCompilerError(markdownPath, "error", fmt.Sprintf("dispatch-workflow validation failed: %v", err)) } } @@ -418,78 +316,48 @@ func (c *Compiler) CompileWorkflowData(workflowData *WorkflowData, markdownPath // Generate the YAML content yamlContent, err := c.generateYAML(workflowData, markdownPath) if err != nil { - formattedErr := console.FormatError(console.CompilerError{ - Position: console.ErrorPosition{ - File: markdownPath, - Line: 1, - Column: 1, - }, - Type: "error", - Message: fmt.Sprintf("failed to generate YAML: %v", err), - }) - return errors.New(formattedErr) + return formatCompilerError(markdownPath, "error", fmt.Sprintf("failed to generate YAML: %v", 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 { - formattedErr := console.FormatError(console.CompilerError{ - Position: console.ErrorPosition{ - File: markdownPath, - Line: 1, - Column: 1, - }, - Type: "error", - Message: fmt.Sprintf("expression size validation failed: %v", err), - }) + // Store error first so we can write invalid YAML before returning + formattedErr := formatCompilerError(markdownPath, "error", fmt.Sprintf("expression size validation failed: %v", 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 { fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Invalid workflow YAML written to: %s", console.ToRelativePath(invalidFile)))) } - return errors.New(formattedErr) + return formattedErr } // Validate for template injection vulnerabilities - detect unsafe expression usage in run: commands log.Print("Validating for template injection vulnerabilities") if err := validateNoTemplateInjection(yamlContent); err != nil { - formattedErr := console.FormatError(console.CompilerError{ - Position: console.ErrorPosition{ - File: markdownPath, - Line: 1, - Column: 1, - }, - Type: "error", - Message: err.Error(), - }) + // Store error first so we can write invalid YAML before returning + formattedErr := formatCompilerError(markdownPath, "error", err.Error()) // 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 { fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Workflow with template injection risks written to: %s", console.ToRelativePath(invalidFile)))) } - return errors.New(formattedErr) + return formattedErr } // Validate against GitHub Actions schema (unless skipped) if !c.skipValidation { log.Print("Validating workflow against GitHub Actions schema") if err := c.validateGitHubActionsSchema(yamlContent); err != nil { - formattedErr := console.FormatError(console.CompilerError{ - Position: console.ErrorPosition{ - File: markdownPath, - Line: 1, - Column: 1, - }, - Type: "error", - Message: fmt.Sprintf("workflow schema validation failed: %v", err), - }) + // Store error first so we can write invalid YAML before returning + formattedErr := formatCompilerError(markdownPath, "error", fmt.Sprintf("workflow schema validation failed: %v", 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 { fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Invalid workflow YAML written to: %s", console.ToRelativePath(invalidFile)))) } - return errors.New(formattedErr) + return formattedErr } // Validate container images used in MCP configurations @@ -497,62 +365,26 @@ func (c *Compiler) CompileWorkflowData(workflowData *WorkflowData, markdownPath if err := c.validateContainerImages(workflowData); err != nil { // Treat container image validation failures as warnings, not errors // This is because validation may fail due to auth issues locally (e.g., private registries) - formattedWarning := console.FormatError(console.CompilerError{ - Position: console.ErrorPosition{ - File: markdownPath, - Line: 1, - Column: 1, - }, - Type: "warning", - Message: fmt.Sprintf("container image validation failed: %v", err), - }) - fmt.Fprintln(os.Stderr, formattedWarning) + fmt.Fprintln(os.Stderr, formatCompilerMessage(markdownPath, "warning", fmt.Sprintf("container image validation failed: %v", err))) c.IncrementWarningCount() } // Validate runtime packages (npx, uv) log.Print("Validating runtime packages") if err := c.validateRuntimePackages(workflowData); err != nil { - formattedErr := console.FormatError(console.CompilerError{ - Position: console.ErrorPosition{ - File: markdownPath, - Line: 1, - Column: 1, - }, - Type: "error", - Message: fmt.Sprintf("runtime package validation failed: %v", err), - }) - return errors.New(formattedErr) + return formatCompilerError(markdownPath, "error", fmt.Sprintf("runtime package validation failed: %v", err)) } // Validate firewall configuration (log-level enum) log.Print("Validating firewall configuration") if err := c.validateFirewallConfig(workflowData); err != nil { - formattedErr := console.FormatError(console.CompilerError{ - Position: console.ErrorPosition{ - File: markdownPath, - Line: 1, - Column: 1, - }, - Type: "error", - Message: fmt.Sprintf("firewall configuration validation failed: %v", err), - }) - return errors.New(formattedErr) + return formatCompilerError(markdownPath, "error", fmt.Sprintf("firewall configuration validation failed: %v", err)) } // Validate repository features (discussions, issues) log.Print("Validating repository features") if err := c.validateRepositoryFeatures(workflowData); err != nil { - formattedErr := console.FormatError(console.CompilerError{ - Position: console.ErrorPosition{ - File: markdownPath, - Line: 1, - Column: 1, - }, - Type: "error", - Message: fmt.Sprintf("repository feature validation failed: %v", err), - }) - return errors.New(formattedErr) + return formatCompilerError(markdownPath, "error", fmt.Sprintf("repository feature validation failed: %v", err)) } } else if c.verbose { fmt.Fprintln(os.Stderr, console.FormatWarningMessage("Schema validation available but skipped (use SetSkipValidation(false) to enable)")) @@ -584,16 +416,7 @@ func (c *Compiler) CompileWorkflowData(workflowData *WorkflowData, markdownPath } if err := os.WriteFile(lockFile, []byte(yamlContent), 0644); err != nil { - formattedErr := console.FormatError(console.CompilerError{ - Position: console.ErrorPosition{ - File: lockFile, - Line: 1, - Column: 1, - }, - Type: "error", - Message: fmt.Sprintf("failed to write lock file: %v", err), - }) - return errors.New(formattedErr) + return formatCompilerError(lockFile, "error", fmt.Sprintf("failed to write lock file: %v", err)) } if shouldForceWrite { diff --git a/pkg/workflow/compiler_error_formatting_test.go b/pkg/workflow/compiler_error_formatting_test.go new file mode 100644 index 0000000000..f10720e5d8 --- /dev/null +++ b/pkg/workflow/compiler_error_formatting_test.go @@ -0,0 +1,155 @@ +package workflow + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestFormatCompilerError tests the formatCompilerError helper function +func TestFormatCompilerError(t *testing.T) { + tests := []struct { + name string + filePath string + errType string + message string + wantContain []string + }{ + { + name: "error type with simple message", + filePath: "/path/to/workflow.md", + errType: "error", + message: "validation failed", + wantContain: []string{ + "/path/to/workflow.md", + "1:1", + "error", + "validation failed", + }, + }, + { + name: "warning type with detailed message", + filePath: "/path/to/workflow.md", + errType: "warning", + message: "missing required permission", + wantContain: []string{ + "/path/to/workflow.md", + "1:1", + "warning", + "missing required permission", + }, + }, + { + name: "lock file path", + filePath: "/path/to/workflow.lock.yml", + errType: "error", + message: "failed to write lock file", + wantContain: []string{ + "/path/to/workflow.lock.yml", + "1:1", + "error", + "failed to write lock file", + }, + }, + { + name: "formatted message with error details", + filePath: "test.md", + errType: "error", + message: "failed to generate YAML: syntax error", + wantContain: []string{ + "test.md", + "1:1", + "error", + "failed to generate YAML: syntax error", + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := formatCompilerError(tt.filePath, tt.errType, tt.message) + 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) + } + }) + } +} + +// TestFormatCompilerError_OutputFormat verifies the output format remains consistent +func TestFormatCompilerError_OutputFormat(t *testing.T) { + err := formatCompilerError("/test/workflow.md", "error", "test message") + require.Error(t, err) + + errStr := err.Error() + + // Verify the error format contains the standard compiler error structure + assert.Contains(t, errStr, "/test/workflow.md", "Should contain file path") + assert.Contains(t, errStr, "1:1", "Should contain line:column") + assert.Contains(t, errStr, "error", "Should contain error type") + assert.Contains(t, errStr, "test message", "Should contain message") +} + +// 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") + + require.Error(t, errorErr) + require.Error(t, warningErr) + + assert.Contains(t, errorErr.Error(), "error", "Error type should be present") + assert.Contains(t, warningErr.Error(), "warning", "Warning type should be present") + + // Ensure they produce different outputs + assert.NotEqual(t, errorErr.Error(), warningErr.Error(), "Error and warning should have different outputs") +} + +// TestFormatCompilerMessage tests the formatCompilerMessage helper function +func TestFormatCompilerMessage(t *testing.T) { + tests := []struct { + name string + filePath string + msgType string + message string + wantContain []string + }{ + { + name: "warning message", + filePath: "/path/to/workflow.md", + msgType: "warning", + message: "container image validation failed", + wantContain: []string{ + "/path/to/workflow.md", + "1:1", + "warning", + "container image validation failed", + }, + }, + { + name: "error message as string", + filePath: "test.md", + msgType: "error", + message: "validation error", + wantContain: []string{ + "test.md", + "1:1", + "error", + "validation error", + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + msg := formatCompilerMessage(tt.filePath, tt.msgType, tt.message) + + for _, want := range tt.wantContain { + assert.Contains(t, msg, want, "Message should contain: %s", want) + } + }) + } +}