diff --git a/cmd/gh-aw/main.go b/cmd/gh-aw/main.go index d49d9058c95..ca3bc196672 100644 --- a/cmd/gh-aw/main.go +++ b/cmd/gh-aw/main.go @@ -276,10 +276,11 @@ Examples: FailFast: failFast, } if _, err := cli.CompileWorkflows(cmd.Context(), config); err != nil { - errMsg := err.Error() - // Check if error is already formatted (contains suggestions or starts with ✗) - if strings.Contains(errMsg, "Suggestions:") || strings.HasPrefix(errMsg, "✗") { - return fmt.Errorf("%s", errMsg) + // Format validation error for better user experience + // The main function will detect the ✗ prefix and print it without double formatting + if !jsonOutput { + // Return a new error with formatted message so main() can print it + return fmt.Errorf("%s", cli.FormatValidationError(err)) } return err } diff --git a/pkg/cli/validation_output.go b/pkg/cli/validation_output.go new file mode 100644 index 00000000000..e68916691ae --- /dev/null +++ b/pkg/cli/validation_output.go @@ -0,0 +1,50 @@ +package cli + +import ( + "fmt" + "os" + + "github.com/githubnext/gh-aw/pkg/console" +) + +// FormatValidationError formats validation errors for console output +// Preserves structured error content while applying console styling +// +// This function bridges the gap between pure validation logic (plain text errors) +// and CLI presentation layer (styled console output). By keeping validation errors +// as plain text at the validation layer, we maintain testability and reusability +// while providing consistent styled output in CLI contexts. +// +// The function handles both simple single-line errors and complex multi-line +// structured errors (like GitHubToolsetValidationError) by applying console +// formatting to preserve the error structure and readability. +func FormatValidationError(err error) string { + if err == nil { + return "" + } + + errMsg := err.Error() + + // Apply console formatting to the entire error message + // This preserves structured multi-line errors while adding visual styling + return console.FormatErrorMessage(errMsg) +} + +// PrintValidationError prints a validation error to stderr with console formatting +// +// This is a convenience helper that combines formatting and printing in one call. +// All validation errors should be printed using this function to ensure consistent +// styling across the CLI. +// +// Example usage: +// +// if err := ValidateWorkflow(config); err != nil { +// PrintValidationError(err) +// return err +// } +func PrintValidationError(err error) { + if err == nil { + return + } + fmt.Fprintln(os.Stderr, FormatValidationError(err)) +} diff --git a/pkg/cli/validation_output_test.go b/pkg/cli/validation_output_test.go new file mode 100644 index 00000000000..53db03ed17f --- /dev/null +++ b/pkg/cli/validation_output_test.go @@ -0,0 +1,235 @@ +//go:build !integration + +package cli + +import ( + "errors" + "fmt" + "strings" + "testing" + + "github.com/githubnext/gh-aw/pkg/workflow" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestFormatValidationError verifies that validation errors are formatted with console styling +func TestFormatValidationError(t *testing.T) { + tests := []struct { + name string + err error + expectEmpty bool + mustContain []string + mustNotChange string // Content that must be preserved + }{ + { + name: "nil error returns empty string", + err: nil, + expectEmpty: true, + }, + { + name: "simple single-line error", + err: errors.New("missing required field 'engine'"), + expectEmpty: false, + mustContain: []string{ + "missing required field 'engine'", + }, + mustNotChange: "missing required field 'engine'", + }, + { + name: "error with example", + err: errors.New("invalid engine: unknown. Valid engines are: copilot, claude, codex, custom. Example: engine: copilot"), + expectEmpty: false, + mustContain: []string{ + "invalid engine", + "Valid engines are", + "Example:", + }, + mustNotChange: "invalid engine: unknown. Valid engines are: copilot, claude, codex, custom. Example: engine: copilot", + }, + { + name: "multi-line error", + err: errors.New(`invalid configuration: + field 'engine' is required + field 'on' is missing`), + expectEmpty: false, + mustContain: []string{ + "invalid configuration", + "field 'engine' is required", + "field 'on' is missing", + }, + }, + { + name: "structured validation error (GitHubToolsetValidationError)", + err: workflow.NewGitHubToolsetValidationError(map[string][]string{ + "issues": {"list_issues", "create_issue"}, + }), + expectEmpty: false, + mustContain: []string{ + "ERROR", + "issues", + "list_issues", + "create_issue", + "Suggested fix", + }, + }, + { + name: "error with formatting characters", + err: fmt.Errorf("path must be relative, got: /absolute/path"), + mustContain: []string{ + "path must be relative", + "/absolute/path", + }, + mustNotChange: "path must be relative, got: /absolute/path", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := FormatValidationError(tt.err) + + if tt.expectEmpty { + assert.Empty(t, result, "Expected empty string for nil error") + return + } + + // Verify content is preserved + if tt.mustNotChange != "" { + assert.Contains(t, result, tt.mustNotChange, + "Formatted error must contain original error message") + } + + // Verify all required content is present + for _, expected := range tt.mustContain { + assert.Contains(t, result, expected, + "Formatted error must contain: %s", expected) + } + + // Verify formatting is applied (should not be identical to plain error) + if tt.err != nil && !tt.expectEmpty { + plainMsg := tt.err.Error() + // The formatted message should be longer (due to ANSI codes or prefix) + // or at minimum have the error symbol prefix + if result == plainMsg { + t.Errorf("Expected formatting to be applied, but result matches plain error.\nPlain: %s\nFormatted: %s", + plainMsg, result) + } + } + }) + } +} + +// TestPrintValidationError verifies that PrintValidationError outputs to stderr +// Note: This is a smoke test to ensure the function doesn't panic +func TestPrintValidationError(t *testing.T) { + tests := []struct { + name string + err error + }{ + { + name: "nil error does not panic", + err: nil, + }, + { + name: "simple error does not panic", + err: errors.New("test error"), + }, + { + name: "complex structured error does not panic", + err: workflow.NewGitHubToolsetValidationError(map[string][]string{ + "repos": {"get_repository"}, + }), + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + // This test ensures PrintValidationError doesn't panic + // Actual output testing would require capturing stderr + require.NotPanics(t, func() { + PrintValidationError(tt.err) + }, "PrintValidationError should not panic") + }) + } +} + +// TestFormatValidationErrorPreservesStructure verifies that multi-line errors maintain their structure +func TestFormatValidationErrorPreservesStructure(t *testing.T) { + // Create a structured error with multiple lines and sections + structuredErr := workflow.NewGitHubToolsetValidationError(map[string][]string{ + "issues": {"list_issues", "create_issue"}, + "actions": {"list_workflows"}, + }) + + result := FormatValidationError(structuredErr) + + // Verify structure is preserved + require.NotEmpty(t, result, "Result should not be empty") + + // Verify line breaks are maintained (multi-line error) + assert.Contains(t, result, "\n", "Multi-line structure should be preserved") + + // Verify all sections are present + sections := []string{ + "ERROR", + "actions", + "issues", + "list_workflows", + "list_issues", + "create_issue", + "Suggested fix", + "toolsets:", + } + + for _, section := range sections { + assert.Contains(t, result, section, + "Structured error should contain section: %s", section) + } + + // Verify the error message contains the original structured content + originalMsg := structuredErr.Error() + lines := strings.Split(originalMsg, "\n") + for _, line := range lines { + if strings.TrimSpace(line) != "" { + assert.Contains(t, result, strings.TrimSpace(line), + "Structured error should preserve line: %s", line) + } + } +} + +// TestFormatValidationErrorContentIntegrity verifies that formatting doesn't alter error content +func TestFormatValidationErrorContentIntegrity(t *testing.T) { + errorMessages := []string{ + "simple error", + "error with special chars: @#$%^&*()", + "error with path: /home/user/file.txt", + "error with URL: https://example.com", + "error with code snippet: engine: copilot", + "multi\nline\nerror\nwith\nbreaks", + "error with numbers: 123 456 789", + "error with quotes: 'single' and \"double\"", + } + + for _, msg := range errorMessages { + t.Run(fmt.Sprintf("content_integrity_%s", strings.ReplaceAll(msg, "\n", "_")), func(t *testing.T) { + err := errors.New(msg) + result := FormatValidationError(err) + + // Verify the original message content is present in the result + assert.Contains(t, result, msg, + "Formatted error must preserve original content") + + // Verify no content is lost or corrupted + // The formatted version should contain at least as many meaningful characters + originalLength := len(strings.TrimSpace(msg)) + // Remove common ANSI codes to get actual content length + cleanResult := strings.ReplaceAll(result, "\033[", "") + cleanResult = strings.ReplaceAll(cleanResult, "\x1b[", "") + + if len(cleanResult) < originalLength { + t.Errorf("Formatting appears to have removed content. Original: %d chars, Result: %d chars", + originalLength, len(cleanResult)) + } + }) + } +}