From 2c007ee3eaa4fd74849e1c40e7d3cb471ed3f8b4 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 29 May 2026 01:20:04 +0000 Subject: [PATCH 1/4] Initial plan From 6037f3eb3a3c8d9102d95b712972b16dea8b1c47 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 29 May 2026 01:33:30 +0000 Subject: [PATCH 2/4] Refactor sanitizer helpers and compiler error formatter callsites Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com> --- pkg/cli/deploy_command.go | 3 +- pkg/cli/logs_run_processor.go | 3 +- pkg/cli/update_command.go | 8 +- pkg/parser/import_cache.go | 17 +-- pkg/parser/import_cache_test.go | 5 +- pkg/stringutil/sanitize.go | 35 ++++-- pkg/stringutil/sanitize_test.go | 35 ++++++ pkg/workflow/agent_validation.go | 16 +-- pkg/workflow/compiler.go | 24 ++-- pkg/workflow/compiler_error_formatter.go | 108 +++++++----------- pkg/workflow/compiler_error_formatter_test.go | 10 +- .../compiler_error_formatting_test.go | 22 ++-- .../compiler_orchestrator_workflow.go | 20 ++-- pkg/workflow/compiler_validators.go | 70 ++++++------ pkg/workflow/markdown_security_scanner.go | 11 +- pkg/workflow/model_alias_validation.go | 25 ++-- .../permissions_compiler_validator.go | 12 +- .../pull_request_target_validation.go | 2 +- pkg/workflow/runs_on_validation.go | 16 +-- pkg/workflow/strings.go | 5 + 20 files changed, 226 insertions(+), 221 deletions(-) diff --git a/pkg/cli/deploy_command.go b/pkg/cli/deploy_command.go index f548cad921a..81a27811bad 100644 --- a/pkg/cli/deploy_command.go +++ b/pkg/cli/deploy_command.go @@ -13,6 +13,7 @@ import ( "github.com/github/gh-aw/pkg/gitutil" "github.com/github/gh-aw/pkg/logger" "github.com/github/gh-aw/pkg/parser" + "github.com/github/gh-aw/pkg/stringutil" "github.com/spf13/cobra" ) @@ -118,7 +119,7 @@ func runDeploy(ctx context.Context, targetRepo string, workflows []string, addOp return err } - checkoutDir := filepath.Join(updatesDir, sanitizeRepoPath(targetRepo)) + checkoutDir := filepath.Join(updatesDir, stringutil.SanitizeForFilenameWithSeparator(targetRepo, "__")) if err := shallowCloneTargetRepo(ctx, targetRepo, checkoutDir); err != nil { return err } diff --git a/pkg/cli/logs_run_processor.go b/pkg/cli/logs_run_processor.go index e6e364e3333..2a9c3154d47 100644 --- a/pkg/cli/logs_run_processor.go +++ b/pkg/cli/logs_run_processor.go @@ -24,6 +24,7 @@ import ( "github.com/github/gh-aw/pkg/constants" "github.com/github/gh-aw/pkg/parser" "github.com/github/gh-aw/pkg/stringutil" + "github.com/github/gh-aw/pkg/workflow" "github.com/sourcegraph/conc/pool" ) @@ -524,7 +525,7 @@ func inferWorkflowPathFromDisplayName(displayName string) string { } // Match sanitizeWorkflowName.cjs: lowercase, replace :[\/\s] with "-", // replace other non-identifier chars with "-", preserve "." and "_". - slug := stringutil.SanitizeName(displayName, &stringutil.SanitizeOptions{ + slug := workflow.SanitizeName(displayName, &workflow.SanitizeOptions{ PreserveSpecialChars: []rune{'.', '_'}, TrimHyphens: true, }) diff --git a/pkg/cli/update_command.go b/pkg/cli/update_command.go index 8eca12591f5..bec04eae43d 100644 --- a/pkg/cli/update_command.go +++ b/pkg/cli/update_command.go @@ -13,6 +13,7 @@ import ( "github.com/github/gh-aw/pkg/constants" "github.com/github/gh-aw/pkg/gitutil" "github.com/github/gh-aw/pkg/logger" + "github.com/github/gh-aw/pkg/stringutil" "github.com/spf13/cobra" ) @@ -207,7 +208,7 @@ func runUpdateForTargetRepo(ctx context.Context, targetRepo string, opts UpdateW return err } - checkoutDir := filepath.Join(updatesDir, sanitizeRepoPath(targetRepo)) + checkoutDir := filepath.Join(updatesDir, stringutil.SanitizeForFilenameWithSeparator(targetRepo, "__")) if err := shallowCloneTargetRepo(ctx, targetRepo, checkoutDir); err != nil { return err } @@ -288,8 +289,3 @@ func shallowCloneTargetRepo(ctx context.Context, repo, destination string) error } return nil } - -func sanitizeRepoPath(repo string) string { - replacer := strings.NewReplacer("/", "__", "\\", "__", ":", "__", "@", "__") - return replacer.Replace(repo) -} diff --git a/pkg/parser/import_cache.go b/pkg/parser/import_cache.go index eb066c7be28..ea1a888ae92 100644 --- a/pkg/parser/import_cache.go +++ b/pkg/parser/import_cache.go @@ -8,8 +8,8 @@ import ( "strings" "github.com/github/gh-aw/pkg/constants" - "github.com/github/gh-aw/pkg/logger" + "github.com/github/gh-aw/pkg/stringutil" ) var importCacheLog = logger.New("parser:import_cache") @@ -19,17 +19,6 @@ const ( ImportCacheDir = ".github/aw/imports" ) -// sanitizePath converts a file path to a safe filename by using filepath.Clean -// and replacing directory separators with underscores -func sanitizePath(path string) string { - // Clean the path to remove any ".." or other suspicious elements - cleaned := filepath.Clean(path) - // Replace directory separators with underscores to create a flat filename - // This prevents directory traversal while preserving path uniqueness - sanitized := strings.ReplaceAll(cleaned, string(filepath.Separator), "_") - return sanitized -} - // validatePathComponents validates that path components don't contain malicious sequences func validatePathComponents(owner, repo, path, sha string) error { components := []string{owner, repo, path, sha} @@ -72,7 +61,7 @@ func NewImportCache(repoRoot string) *ImportCache { func (c *ImportCache) Get(owner, repo, path, sha string) (string, bool) { // Use SHA-based approach: cache files are stored by commit SHA // Cache path: .github/aw/imports/owner/repo/sha/sanitized_path.md - sanitizedPath := sanitizePath(path) + sanitizedPath := stringutil.SanitizeForFilenameWithSeparator(filepath.Clean(path), "_") relativeCachePath := filepath.Join(ImportCacheDir, owner, repo, sha, sanitizedPath) fullCachePath := filepath.Join(c.baseDir, relativeCachePath) @@ -111,7 +100,7 @@ func (c *ImportCache) Set(owner, repo, path, sha string, content []byte) (string // Use SHA in path for consistent caching // This ensures that different refs pointing to the same commit reuse the same cache - sanitizedPath := sanitizePath(path) + sanitizedPath := stringutil.SanitizeForFilenameWithSeparator(filepath.Clean(path), "_") relativeCachePath := filepath.Join(ImportCacheDir, owner, repo, sha, sanitizedPath) fullCachePath := filepath.Join(c.baseDir, relativeCachePath) diff --git a/pkg/parser/import_cache_test.go b/pkg/parser/import_cache_test.go index 4015dba61f1..6508901badc 100644 --- a/pkg/parser/import_cache_test.go +++ b/pkg/parser/import_cache_test.go @@ -7,6 +7,7 @@ import ( "path/filepath" "testing" + "github.com/github/gh-aw/pkg/stringutil" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -183,8 +184,8 @@ func TestSanitizePath(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - result := sanitizePath(tt.input) - assert.Equal(t, tt.expected, result, "sanitizePath(%q) should return expected value", tt.input) + result := stringutil.SanitizeForFilenameWithSeparator(filepath.Clean(tt.input), "_") + assert.Equal(t, tt.expected, result, "sanitize path(%q) should return expected value", tt.input) }) } } diff --git a/pkg/stringutil/sanitize.go b/pkg/stringutil/sanitize.go index f4c902b09e9..19d08d0df43 100644 --- a/pkg/stringutil/sanitize.go +++ b/pkg/stringutil/sanitize.go @@ -287,26 +287,41 @@ func SanitizeToolID(toolID string) string { return cleaned } -// SanitizeForFilename converts a repository slug (owner/repo) to a filename-safe string. -// Replaces "/" with "-" and any remaining non-alphanumeric characters (except "-", "_", ".") -// with "-". Returns "clone-mode" if the slug is empty. +// SanitizeForFilenameWithSeparator converts a repository slug (owner/repo) to a filename-safe string. +// Replaces "/" with the provided separator and any remaining non-alphanumeric characters +// (except "-", "_", ".") with the same separator. Returns "clone-mode" if the slug is empty. // -// Examples: +// Example: // -// SanitizeForFilename("owner/repo") // returns "owner-repo" -// SanitizeForFilename("my.org/my_repo") // returns "my.org-my_repo" -// SanitizeForFilename("") // returns "clone-mode" -func SanitizeForFilename(slug string) string { +// SanitizeForFilenameWithSeparator("owner/repo", "-") // returns "owner-repo" +// SanitizeForFilenameWithSeparator("owner/repo", "__") // returns "owner__repo" +func SanitizeForFilenameWithSeparator(slug, separator string) string { if slug == "" { return "clone-mode" } + if separator == "" { + separator = "-" + } var sb strings.Builder - for _, r := range strings.ReplaceAll(slug, "/", "-") { + for _, r := range strings.ReplaceAll(slug, "/", separator) { if (r >= 'a' && r <= 'z') || (r >= 'A' && r <= 'Z') || (r >= '0' && r <= '9') || r == '-' || r == '_' || r == '.' { sb.WriteRune(r) } else { - sb.WriteRune('-') + sb.WriteString(separator) } } return sb.String() } + +// SanitizeForFilename converts a repository slug (owner/repo) to a filename-safe string. +// Replaces "/" with "-" and any remaining non-alphanumeric characters (except "-", "_", ".") +// with "-". Returns "clone-mode" if the slug is empty. +// +// Examples: +// +// SanitizeForFilename("owner/repo") // returns "owner-repo" +// SanitizeForFilename("my.org/my_repo") // returns "my.org-my_repo" +// SanitizeForFilename("") // returns "clone-mode" +func SanitizeForFilename(slug string) string { + return SanitizeForFilenameWithSeparator(slug, "-") +} diff --git a/pkg/stringutil/sanitize_test.go b/pkg/stringutil/sanitize_test.go index 972422c783b..a750d890a99 100644 --- a/pkg/stringutil/sanitize_test.go +++ b/pkg/stringutil/sanitize_test.go @@ -652,6 +652,41 @@ func TestSanitizeForFilename(t *testing.T) { } } +func TestSanitizeForFilenameWithSeparator(t *testing.T) { + tests := []struct { + name string + slug string + separator string + expected string + }{ + { + name: "custom separator for repo path", + slug: "owner/repo", + separator: "__", + expected: "owner__repo", + }, + { + name: "special chars use separator", + slug: "owner:repo@main", + separator: "__", + expected: "owner__repo__main", + }, + { + name: "empty separator falls back to hyphen", + slug: "owner/repo", + separator: "", + expected: "owner-repo", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := SanitizeForFilenameWithSeparator(tt.slug, tt.separator) + assertSanitizeResult(t, "SanitizeForFilenameWithSeparator", tt.slug, result, tt.expected) + }) + } +} + func BenchmarkSanitizeForFilename(b *testing.B) { slug := "github/gh-aw" for b.Loop() { diff --git a/pkg/workflow/agent_validation.go b/pkg/workflow/agent_validation.go index 7b524a1d831..f9ea432ea55 100644 --- a/pkg/workflow/agent_validation.go +++ b/pkg/workflow/agent_validation.go @@ -71,8 +71,8 @@ func (c *Compiler) validateAgentFile(workflowData *WorkflowData, markdownPath st // Only alphanumeric characters, dots, underscores, hyphens, forward slashes, // and spaces are permitted. Shell metacharacters are rejected. if !agentFilePathRegex.MatchString(agentPath) { - return formatCompilerError(markdownPath, "error", - fmt.Sprintf("agent file path '%s' contains invalid characters. Only alphanumeric characters, dots, underscores, hyphens, forward slashes, and spaces are allowed.", agentPath), nil) + return formatCompilerError(compilerErrorOpts{FilePath: markdownPath, ErrType: "error", Message: fmt.Sprintf("agent file path '%s' contains invalid characters. Only alphanumeric characters, dots, underscores, hyphens, forward slashes, and spaces are allowed.", agentPath), Cause: nil}) + } var fullAgentPath string @@ -93,12 +93,12 @@ func (c *Compiler) validateAgentFile(workflowData *WorkflowData, markdownPath st // Check if the file exists 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), nil) + return formatCompilerError(compilerErrorOpts{FilePath: markdownPath, ErrType: "error", Message: fmt.Sprintf("agent file '%s' does not exist. Ensure the file exists in the repository and is properly imported.", agentPath), Cause: nil}) + } // Other error (permissions, etc.) - return formatCompilerError(markdownPath, "error", - fmt.Sprintf("failed to access agent file '%s': %v", agentPath, err), err) + return formatCompilerError(compilerErrorOpts{FilePath: markdownPath, ErrType: "error", Message: fmt.Sprintf("failed to access agent file '%s': %v", agentPath, err), Cause: err}) + } if c.verbose { @@ -291,7 +291,7 @@ on: workflow_run: workflows: ["your-workflow"] types: [completed]` - return formatCompilerError(markdownPath, "error", message, nil) + return formatCompilerError(compilerErrorOpts{FilePath: markdownPath, ErrType: "error", Message: message, Cause: nil}) } // Check if workflow_run has branches field @@ -319,7 +319,7 @@ on: if c.strictMode { // In strict mode, this is an error - return formatCompilerError(markdownPath, "error", message, nil) + return formatCompilerError(compilerErrorOpts{FilePath: markdownPath, ErrType: "error", Message: message, Cause: nil}) } // In normal mode, this is a warning diff --git a/pkg/workflow/compiler.go b/pkg/workflow/compiler.go index 3f6b8014fb5..76b4dd37224 100644 --- a/pkg/workflow/compiler.go +++ b/pkg/workflow/compiler.go @@ -70,7 +70,7 @@ func (c *Compiler) CompileWorkflow(markdownPath string) error { return err } // Fallback for any unformatted error that slipped through. - return formatCompilerError(markdownPath, "error", err.Error(), err) + return formatCompilerError(compilerErrorOpts{FilePath: markdownPath, ErrType: "error", Message: err.Error(), Cause: err}) } return c.CompileWorkflowData(workflowData, markdownPath) @@ -123,7 +123,7 @@ func (c *Compiler) generateAndValidateYAML(workflowData *WorkflowData, markdownP // (returned to avoid a second scan of the full YAML in the caller for safe update enforcement). yamlContent, bodySecrets, bodyActions, err := c.generateYAML(workflowData, markdownPath) if err != nil { - return "", nil, nil, formatCompilerError(markdownPath, "error", fmt.Sprintf("failed to generate YAML: %v", err), err) + return "", nil, nil, formatCompilerError(compilerErrorOpts{FilePath: markdownPath, ErrType: "error", Message: fmt.Sprintf("failed to generate YAML: %v", err), Cause: err}) } // Always validate expression sizes - this is a hard limit from GitHub Actions (21KB) @@ -131,7 +131,7 @@ func (c *Compiler) generateAndValidateYAML(workflowData *WorkflowData, markdownP workflowLog.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), err) + formattedErr := formatCompilerError(compilerErrorOpts{FilePath: markdownPath, ErrType: "error", Message: fmt.Sprintf("expression size validation failed: %v", err), Cause: 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), constants.FilePermPublic); writeErr == nil { @@ -197,8 +197,8 @@ func (c *Compiler) generateAndValidateYAML(workflowData *WorkflowData, markdownP } } // Store error first so we can write invalid YAML before returning - formattedErr := formatCompilerErrorWithPosition(markdownPath, fieldLine, 1, "error", - fmt.Sprintf("invalid workflow: %v", schemaErr), schemaErr) + formattedErr := formatCompilerError(compilerErrorOpts{FilePath: markdownPath, Line: fieldLine, Column: 1, ErrType: "error", Message: fmt.Sprintf("invalid workflow: %v", schemaErr), Cause: schemaErr}) + // 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), constants.FilePermPublic); writeErr == nil { @@ -219,19 +219,19 @@ func (c *Compiler) generateAndValidateYAML(workflowData *WorkflowData, markdownP // Validate runtime packages (npx, uv) workflowLog.Print("Validating runtime packages") if err := c.validateRuntimePackages(workflowData); err != nil { - return "", nil, nil, formatCompilerError(markdownPath, "error", fmt.Sprintf("runtime package validation failed: %v", err), err) + return "", nil, nil, formatCompilerError(compilerErrorOpts{FilePath: markdownPath, ErrType: "error", Message: fmt.Sprintf("runtime package validation failed: %v", err), Cause: err}) } // Validate firewall configuration (log-level enum) workflowLog.Print("Validating firewall configuration") if err := c.validateFirewallConfig(workflowData); err != nil { - return "", nil, nil, formatCompilerError(markdownPath, "error", fmt.Sprintf("firewall configuration validation failed: %v", err), err) + return "", nil, nil, formatCompilerError(compilerErrorOpts{FilePath: markdownPath, ErrType: "error", Message: fmt.Sprintf("firewall configuration validation failed: %v", err), Cause: err}) } // Validate repository features (discussions, issues) workflowLog.Print("Validating repository features") if err := c.validateRepositoryFeatures(workflowData); err != nil { - return "", nil, nil, formatCompilerError(markdownPath, "error", fmt.Sprintf("repository feature validation failed: %v", err), err) + return "", nil, nil, formatCompilerError(compilerErrorOpts{FilePath: markdownPath, ErrType: "error", Message: fmt.Sprintf("repository feature validation failed: %v", err), Cause: err}) } } else if c.verbose { fmt.Fprintln(os.Stderr, console.FormatWarningMessage("Schema validation available but skipped (use SetSkipValidation(false) to enable)")) @@ -263,7 +263,7 @@ func (c *Compiler) writeWorkflowOutput(lockFile, yamlContent string, markdownPat // Only write if content has changed if !contentUnchanged { if err := os.WriteFile(lockFile, []byte(yamlContent), constants.FilePermPublic); err != nil { - return formatCompilerError(lockFile, "error", fmt.Sprintf("failed to write lock file: %v", err), err) + return formatCompilerError(compilerErrorOpts{FilePath: lockFile, ErrType: "error", Message: fmt.Sprintf("failed to write lock file: %v", err), Cause: err}) } workflowLog.Print("Lock file written successfully") } @@ -345,7 +345,7 @@ func (c *Compiler) validateTemplateInjection(yamlContent, lockFile, markdownPath if templateErr != nil { // Store error first so we can write invalid YAML before returning - formattedErr := formatCompilerError(markdownPath, "error", templateErr.Error(), templateErr) + formattedErr := formatCompilerError(compilerErrorOpts{FilePath: markdownPath, ErrType: "error", Message: templateErr.Error(), Cause: templateErr}) // 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), constants.FilePermPublic); writeErr == nil { @@ -493,7 +493,7 @@ func (c *Compiler) CompileWorkflowData(workflowData *WorkflowData, markdownPath if isFormattedCompilerError(err) { return err } - return formatCompilerError(markdownPath, "error", "workflow validation: "+err.Error(), err) + return formatCompilerError(compilerErrorOpts{FilePath: markdownPath, ErrType: "error", Message: "workflow validation: " + err.Error(), Cause: err}) } // Note: Markdown content size is now handled by splitting into multiple steps in generatePrompt @@ -512,7 +512,7 @@ func (c *Compiler) CompileWorkflowData(workflowData *WorkflowData, markdownPath if isFormattedCompilerError(err) { return err } - return formatCompilerError(markdownPath, "error", "YAML generation: "+err.Error(), err) + return formatCompilerError(compilerErrorOpts{FilePath: markdownPath, ErrType: "error", Message: "YAML generation: " + err.Error(), Cause: err}) } // Enforce safe update mode: emit a warning prompt (not a hard error) when unapproved diff --git a/pkg/workflow/compiler_error_formatter.go b/pkg/workflow/compiler_error_formatter.go index b443db477b1..2b592cdb048 100644 --- a/pkg/workflow/compiler_error_formatter.go +++ b/pkg/workflow/compiler_error_formatter.go @@ -21,36 +21,60 @@ type wrappedCompilerError struct { func (e *wrappedCompilerError) Error() string { return e.formatted } func (e *wrappedCompilerError) Unwrap() error { return e.cause } -// formatCompilerError creates a formatted compiler error at line 1, column 1. -// Use this when the exact source position is unknown; IDE tooling can still navigate to the file. -// Use formatCompilerErrorWithPosition when a specific line/column is available. -// When cause is a *WorkflowValidationError with Line > 0, the error's own position is used -// instead of the default 1:1, giving precise IDE-navigable locations. -// -// 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 -// cause: optional underlying error to wrap (use nil for validation errors) -func formatCompilerError(filePath string, errType string, message string, cause error) error { - line, column := 1, 1 +type compilerErrorOpts struct { + FilePath string + Line int + Column int + ErrType string + Message string + Cause error + Context []string +} + +// formatCompilerError creates a formatted compiler error from options. +// Defaults line/column to 1:1 when they are not provided. +// When Cause is a *WorkflowValidationError with Line > 0, the error's own +// position (and file, when available) takes precedence. +func formatCompilerError(opts compilerErrorOpts) error { + line := opts.Line + column := opts.Column + if line <= 0 { + line = 1 + } + if column <= 0 { + column = 1 + } + // Promote precise source location from WorkflowValidationError when available so that // the emitted "file:line:col: error:" prefix points directly at the problematic field // rather than always defaulting to line 1, column 1. var vErr *WorkflowValidationError - if errors.As(cause, &vErr) && vErr.Line > 0 { + if errors.As(opts.Cause, &vErr) && vErr.Line > 0 { line = vErr.Line if vErr.Column > 0 { column = vErr.Column } if vErr.File != "" { - filePath = vErr.File + opts.FilePath = vErr.File } } - return formatCompilerErrorWithPosition(filePath, line, column, errType, message, cause) + compilerErrorLog.Printf("Formatting compiler error: file=%s, line=%d, column=%d, type=%s, context=%d lines", opts.FilePath, line, column, opts.ErrType, len(opts.Context)) + formattedErr := console.FormatError(console.CompilerError{ + Position: console.ErrorPosition{ + File: opts.FilePath, + Line: line, + Column: column, + }, + Type: opts.ErrType, + Message: opts.Message, + Context: opts.Context, + }) + + return &wrappedCompilerError{formatted: formattedErr, cause: opts.Cause} } // isFormattedCompilerError reports whether err is already a console-formatted compiler error -// produced by formatCompilerError, formatCompilerErrorWithPosition, or parser.FormatImportError. +// produced by formatCompilerError or parser.FormatImportError. // Use this instead of fragile string-contains checks to avoid double-wrapping. func isFormattedCompilerError(err error) bool { var wce *wrappedCompilerError @@ -63,58 +87,6 @@ func isFormattedCompilerError(err error) bool { return errors.As(err, &fpe) } -// formatCompilerErrorWithPosition creates a formatted compiler error with specific line/column position. -// -// filePath: the file path to include in the error -// line: the line number where the error occurred -// column: the column number where the error occurred -// errType: the error type ("error" or "warning") -// message: the error message text -// cause: optional underlying error to wrap (use nil for validation errors) -func formatCompilerErrorWithPosition(filePath string, line int, column int, errType string, message string, cause error) error { - compilerErrorLog.Printf("Formatting compiler error: file=%s, line=%d, column=%d, type=%s, message=%s", filePath, line, column, errType, message) - formattedErr := console.FormatError(console.CompilerError{ - Position: console.ErrorPosition{ - File: filePath, - Line: line, - Column: column, - }, - Type: errType, - Message: message, - }) - - // Always return a *wrappedCompilerError so isFormattedCompilerError can detect it. - // cause may be nil for validation errors that have no underlying cause. - return &wrappedCompilerError{formatted: formattedErr, cause: cause} -} - -// formatCompilerErrorWithContext creates a formatted compiler error with specific -// line/column position and source-code context lines for Rust-style rendering. -// Use this when source context is available; otherwise use formatCompilerErrorWithPosition. -// -// filePath: the file path to include in the error -// line: the line number where the error occurred -// column: the column number where the error occurred -// errType: the error type ("error" or "warning") -// message: the error message text -// cause: optional underlying error to wrap (use nil for validation errors) -// context: source code lines around the error for Rust-like snippet rendering -func formatCompilerErrorWithContext(filePath string, line int, column int, errType string, message string, cause error, context []string) error { - compilerErrorLog.Printf("Formatting compiler error with context: file=%s, line=%d, column=%d, type=%s, context=%d lines", filePath, line, column, errType, len(context)) - formattedErr := console.FormatError(console.CompilerError{ - Position: console.ErrorPosition{ - File: filePath, - Line: line, - Column: column, - }, - Type: errType, - Message: message, - Context: context, - }) - - return &wrappedCompilerError{formatted: formattedErr, cause: cause} -} - // 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") diff --git a/pkg/workflow/compiler_error_formatter_test.go b/pkg/workflow/compiler_error_formatter_test.go index d631ee16da9..f2ec5908aa5 100644 --- a/pkg/workflow/compiler_error_formatter_test.go +++ b/pkg/workflow/compiler_error_formatter_test.go @@ -11,7 +11,7 @@ import ( ) // TestIsFormattedCompilerError verifies that the helper correctly identifies -// errors produced by formatCompilerError / formatCompilerErrorWithPosition and +// errors produced by formatCompilerError and // returns false for other error types. func TestIsFormattedCompilerError(t *testing.T) { tests := []struct { @@ -21,17 +21,17 @@ func TestIsFormattedCompilerError(t *testing.T) { }{ { name: "error from formatCompilerError with nil cause", - err: formatCompilerError("file.md", "error", "something went wrong", nil), + err: formatCompilerError(compilerErrorOpts{FilePath: "file.md", ErrType: "error", Message: "something went wrong", Cause: nil}), expected: true, }, { name: "error from formatCompilerError with non-nil cause", - err: formatCompilerError("file.md", "error", "something went wrong", errors.New("root cause")), + err: formatCompilerError(compilerErrorOpts{FilePath: "file.md", ErrType: "error", Message: "something went wrong", Cause: errors.New("root cause")}), expected: true, }, { - name: "error from formatCompilerErrorWithPosition", - err: formatCompilerErrorWithPosition("file.md", 5, 3, "error", "bad value", nil), + name: "error from formatCompilerError with explicit position", + err: formatCompilerError(compilerErrorOpts{FilePath: "file.md", Line: 5, Column: 3, ErrType: "error", Message: "bad value", Cause: nil}), expected: true, }, { diff --git a/pkg/workflow/compiler_error_formatting_test.go b/pkg/workflow/compiler_error_formatting_test.go index 96e796fa87a..634262cd247 100644 --- a/pkg/workflow/compiler_error_formatting_test.go +++ b/pkg/workflow/compiler_error_formatting_test.go @@ -90,7 +90,7 @@ 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, tt.cause) + err := formatCompilerError(compilerErrorOpts{FilePath: tt.filePath, ErrType: tt.errType, Message: tt.message, Cause: tt.cause}) require.Error(t, err, "formatCompilerError should return an error") errStr := err.Error() @@ -108,7 +108,7 @@ func TestFormatCompilerError(t *testing.T) { // TestFormatCompilerError_OutputFormat verifies the output format remains consistent func TestFormatCompilerError_OutputFormat(t *testing.T) { - err := formatCompilerError("/test/workflow.md", "error", "test message", nil) + err := formatCompilerError(compilerErrorOpts{FilePath: "/test/workflow.md", ErrType: "error", Message: "test message", Cause: nil}) require.Error(t, err) errStr := err.Error() @@ -123,8 +123,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", nil) - warningErr := formatCompilerError("test.md", "warning", "warning message", nil) + errorErr := formatCompilerError(compilerErrorOpts{FilePath: "test.md", ErrType: "error", Message: "error message", Cause: nil}) + warningErr := formatCompilerError(compilerErrorOpts{FilePath: "test.md", ErrType: "warning", Message: "warning message", Cause: nil}) require.Error(t, errorErr) require.Error(t, warningErr) @@ -186,7 +186,7 @@ func TestFormatCompilerError_ErrorWrapping(t *testing.T) { underlyingErr := errors.New("underlying validation error") // Wrap it with formatCompilerError - wrappedErr := formatCompilerError("test.md", "error", "validation failed", underlyingErr) + wrappedErr := formatCompilerError(compilerErrorOpts{FilePath: "test.md", ErrType: "error", Message: "validation failed", Cause: underlyingErr}) require.Error(t, wrappedErr) @@ -207,7 +207,7 @@ func TestFormatCompilerError_SameMessageAndCause(t *testing.T) { // This is the most common call pattern in compiler.go: // return formatCompilerError(path, "error", err.Error(), err) - wrappedErr := formatCompilerError("test.md", "error", underlying.Error(), underlying) + wrappedErr := formatCompilerError(compilerErrorOpts{FilePath: "test.md", ErrType: "error", Message: underlying.Error(), Cause: underlying}) require.Error(t, wrappedErr) @@ -223,7 +223,7 @@ func TestFormatCompilerError_SameMessageAndCause(t *testing.T) { // TestFormatCompilerError_NilCause verifies that nil cause creates a new error func TestFormatCompilerError_NilCause(t *testing.T) { - err := formatCompilerError("test.md", "error", "validation error", nil) + err := formatCompilerError(compilerErrorOpts{FilePath: "test.md", ErrType: "error", Message: "validation error", Cause: nil}) require.Error(t, err) @@ -244,7 +244,7 @@ func TestFormatCompilerError_UsesLocationFromValidationError(t *testing.T) { t.Run("uses line and column from validation error", func(t *testing.T) { vErr := &WorkflowValidationError{Field: "engine", Value: "copiliot", Reason: "not a valid engine", Suggestion: "Did you mean 'copilot'?", File: "workflow.md", Line: 15, Column: 3} - wrapped := formatCompilerError("workflow.md", "error", vErr.Error(), vErr) + wrapped := formatCompilerError(compilerErrorOpts{FilePath: "workflow.md", ErrType: "error", Message: vErr.Error(), Cause: vErr}) require.Error(t, wrapped) errStr := wrapped.Error() @@ -258,7 +258,7 @@ func TestFormatCompilerError_UsesLocationFromValidationError(t *testing.T) { t.Run("uses file from validation error when different from filePath", func(t *testing.T) { vErr := &WorkflowValidationError{Field: "concurrency", Value: "invalid", Reason: "reason", File: "actual-source.md", Line: 7, Column: 1} - wrapped := formatCompilerError("other.md", "error", vErr.Error(), vErr) + wrapped := formatCompilerError(compilerErrorOpts{FilePath: "other.md", ErrType: "error", Message: vErr.Error(), Cause: vErr}) require.Error(t, wrapped) errStr := wrapped.Error() @@ -268,7 +268,7 @@ func TestFormatCompilerError_UsesLocationFromValidationError(t *testing.T) { t.Run("falls back to 1:1 when validation error has no location", func(t *testing.T) { vErr := NewValidationError("engine", "copiliot", "not a valid engine", "") - wrapped := formatCompilerError("workflow.md", "error", vErr.Error(), vErr) + wrapped := formatCompilerError(compilerErrorOpts{FilePath: "workflow.md", ErrType: "error", Message: vErr.Error(), Cause: vErr}) require.Error(t, wrapped) errStr := wrapped.Error() @@ -277,7 +277,7 @@ func TestFormatCompilerError_UsesLocationFromValidationError(t *testing.T) { t.Run("non-validation-error cause still defaults to 1:1", func(t *testing.T) { cause := errors.New("some other error") - wrapped := formatCompilerError("workflow.md", "error", "message", cause) + wrapped := formatCompilerError(compilerErrorOpts{FilePath: "workflow.md", ErrType: "error", Message: "message", Cause: cause}) require.Error(t, wrapped) errStr := wrapped.Error() diff --git a/pkg/workflow/compiler_orchestrator_workflow.go b/pkg/workflow/compiler_orchestrator_workflow.go index 0a83ce629d9..f9d78f32051 100644 --- a/pkg/workflow/compiler_orchestrator_workflow.go +++ b/pkg/workflow/compiler_orchestrator_workflow.go @@ -42,7 +42,7 @@ func (c *Compiler) ParseWorkflowFile(markdownPath string) (*WorkflowData, error) engineSetup, err := c.setupEngineAndImports(result, cleanPath, content, markdownDir) if err != nil { // Wrap unformatted errors with file location. Errors produced by - // formatCompilerError/formatCompilerErrorWithPosition are already + // formatCompilerError already // console-formatted and must not be double-wrapped. if isFormattedCompilerError(err) { return nil, err @@ -53,9 +53,9 @@ func (c *Compiler) ParseWorkflowFile(markdownPath string) (*WorkflowData, error) if engineLine > 0 { // Read source context lines (±3 lines around the error) for Rust-style rendering contextLines := readSourceContextLines(content, engineLine) - return nil, formatCompilerErrorWithContext(cleanPath, engineLine, 1, "error", err.Error(), err, contextLines) + return nil, formatCompilerError(compilerErrorOpts{FilePath: cleanPath, Line: engineLine, Column: 1, ErrType: "error", Message: err.Error(), Cause: err, Context: contextLines}) } - return nil, formatCompilerError(cleanPath, "error", err.Error(), err) + return nil, formatCompilerError(compilerErrorOpts{FilePath: cleanPath, ErrType: "error", Message: err.Error(), Cause: err}) } // Process tools and markdown @@ -64,7 +64,7 @@ func (c *Compiler) ParseWorkflowFile(markdownPath string) (*WorkflowData, error) if isFormattedCompilerError(err) { return nil, err } - return nil, formatCompilerError(cleanPath, "error", err.Error(), err) + return nil, formatCompilerError(compilerErrorOpts{FilePath: cleanPath, ErrType: "error", Message: err.Error(), Cause: err}) } // Build initial workflow data structure @@ -131,11 +131,11 @@ func (c *Compiler) ParseWorkflowFile(markdownPath string) (*WorkflowData, error) // Validate that inlined-imports is not used with agent file imports. // Agent files require runtime access and cannot be resolved without sources. if workflowData.InlinedImports && engineSetup.importsResult.AgentFile != "" { - return nil, formatCompilerError(cleanPath, "error", - fmt.Sprintf("inlined-imports cannot be used with agent file imports: '%s'. "+ - "Agent files require runtime access and will not be resolved without sources. "+ - "Remove 'inlined-imports: true' or do not import agent files.", - engineSetup.importsResult.AgentFile), nil) + return nil, formatCompilerError(compilerErrorOpts{FilePath: cleanPath, ErrType: "error", Message: fmt.Sprintf("inlined-imports cannot be used with agent file imports: '%s'. "+ + "Agent files require runtime access and will not be resolved without sources. "+ + "Remove 'inlined-imports: true' or do not import agent files.", + engineSetup.importsResult.AgentFile), Cause: nil}) + } // Validate bash tool configuration BEFORE applying defaults @@ -177,7 +177,7 @@ func (c *Compiler) ParseWorkflowFile(markdownPath string) (*WorkflowData, error) // Extract YAML configuration sections from frontmatter if err := c.extractYAMLSections(result.Frontmatter, workflowData); err != nil { - return nil, formatCompilerError(cleanPath, "error", err.Error(), err) + return nil, formatCompilerError(compilerErrorOpts{FilePath: cleanPath, ErrType: "error", Message: err.Error(), Cause: err}) } // Merge observability endpoints and custom attributes from imports with those diff --git a/pkg/workflow/compiler_validators.go b/pkg/workflow/compiler_validators.go index 4095bec7477..555c7e879ef 100644 --- a/pkg/workflow/compiler_validators.go +++ b/pkg/workflow/compiler_validators.go @@ -19,7 +19,7 @@ func (c *Compiler) validateExpressions(workflowData *WorkflowData, markdownPath if strings.Contains(workflowData.MarkdownContent, "${{") { workflowLog.Printf("Validating expression safety") if err := validateExpressionSafety(workflowData.MarkdownContent); err != nil { - return formatCompilerError(markdownPath, "error", err.Error(), err) + return formatCompilerError(compilerErrorOpts{FilePath: markdownPath, ErrType: "error", Message: err.Error(), Cause: err}) } } @@ -39,7 +39,7 @@ func (c *Compiler) validateExpressions(workflowData *WorkflowData, markdownPath c.IncrementWarningCount() } if err != nil { - return formatCompilerError(markdownPath, "error", err.Error(), err) + return formatCompilerError(compilerErrorOpts{FilePath: markdownPath, ErrType: "error", Message: err.Error(), Cause: err}) } } @@ -98,13 +98,13 @@ func (c *Compiler) validateFeatureConfig(workflowData *WorkflowData, markdownPat // Validate feature flags workflowLog.Printf("Validating feature flags") if err := validateFeatures(workflowData); err != nil { - return formatCompilerError(markdownPath, "error", err.Error(), err) + return formatCompilerError(compilerErrorOpts{FilePath: markdownPath, ErrType: "error", Message: err.Error(), Cause: err}) } // Inline sub-agents are always enabled and can no longer be disabled. if workflowData.InlineSubAgentsDisabled { msg := "inline-sub-agents: false is not supported. Inline sub-agents are always enabled. Remove inline-sub-agents from your frontmatter." - return formatCompilerError(markdownPath, "error", msg, errors.New("inline-sub-agents cannot be set to false")) + return formatCompilerError(compilerErrorOpts{FilePath: markdownPath, ErrType: "error", Message: msg, Cause: errors.New("inline-sub-agents cannot be set to false")}) } // Check for action-mode feature flag override @@ -113,7 +113,7 @@ func (c *Compiler) validateFeatureConfig(workflowData *WorkflowData, markdownPat 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), nil) + return formatCompilerError(compilerErrorOpts{FilePath: markdownPath, ErrType: "error", Message: fmt.Sprintf("invalid action-mode feature flag '%s'. Must be 'dev', 'release', or 'script'", actionModeStr), Cause: nil}) } workflowLog.Printf("Overriding action mode from feature flag: %s", mode) c.SetActionMode(mode) @@ -140,19 +140,19 @@ func (c *Compiler) validateToolConfiguration(workflowData *WorkflowData, markdow // Validate sandbox configuration workflowLog.Printf("Validating sandbox configuration") if err := validateSandboxConfig(workflowData); err != nil { - return formatCompilerError(markdownPath, "error", err.Error(), err) + return formatCompilerError(compilerErrorOpts{FilePath: markdownPath, ErrType: "error", Message: err.Error(), Cause: err}) } // Validate safe-outputs target configuration workflowLog.Printf("Validating safe-outputs target fields") if err := validateSafeOutputsTarget(workflowData.SafeOutputs); err != nil { - return formatCompilerError(markdownPath, "error", err.Error(), err) + return formatCompilerError(compilerErrorOpts{FilePath: markdownPath, ErrType: "error", Message: err.Error(), Cause: err}) } // Validate safe-outputs max configuration workflowLog.Printf("Validating safe-outputs max fields") if err := validateSafeOutputsMax(workflowData.SafeOutputs); err != nil { - return formatCompilerError(markdownPath, "error", err.Error(), err) + return formatCompilerError(compilerErrorOpts{FilePath: markdownPath, ErrType: "error", Message: err.Error(), Cause: err}) } // Validate safe-outputs steps for dangerous shell expansion patterns. @@ -161,7 +161,7 @@ func (c *Compiler) validateToolConfiguration(workflowData *WorkflowData, markdow workflowLog.Printf("Validating safe-outputs steps for shell expansion patterns") if err := validateSafeOutputsStepsShellExpansion(workflowData.SafeOutputs); err != nil { if c.strictMode { - return formatCompilerError(markdownPath, "error", err.Error(), err) + return formatCompilerError(compilerErrorOpts{FilePath: markdownPath, ErrType: "error", Message: err.Error(), Cause: err}) } fmt.Fprintln(os.Stderr, formatCompilerMessage(markdownPath, "warning", err.Error())) c.IncrementWarningCount() @@ -170,31 +170,31 @@ func (c *Compiler) validateToolConfiguration(workflowData *WorkflowData, markdow // Validate safe-outputs allowed-domains configuration workflowLog.Printf("Validating safe-outputs allowed-domains") if err := c.validateSafeOutputsAllowedDomains(workflowData.SafeOutputs); err != nil { - return formatCompilerError(markdownPath, "error", err.Error(), err) + return formatCompilerError(compilerErrorOpts{FilePath: markdownPath, ErrType: "error", Message: err.Error(), Cause: err}) } // Validate safe-outputs merge-pull-request configuration workflowLog.Printf("Validating safe-outputs merge-pull-request") if err := validateSafeOutputsMergePullRequest(workflowData.SafeOutputs); err != nil { - return formatCompilerError(markdownPath, "error", err.Error(), err) + return formatCompilerError(compilerErrorOpts{FilePath: markdownPath, ErrType: "error", Message: err.Error(), Cause: err}) } // Validate safe-outputs needs declarations workflowLog.Printf("Validating safe-outputs needs declarations") if err := validateSafeOutputsNeeds(workflowData); err != nil { - return formatCompilerError(markdownPath, "error", err.Error(), err) + return formatCompilerError(compilerErrorOpts{FilePath: markdownPath, ErrType: "error", Message: err.Error(), Cause: err}) } // Validate on.needs declarations and on.github-app needs expressions workflowLog.Printf("Validating on.needs declarations") if err := c.validateOnNeeds(workflowData); err != nil { - return formatCompilerError(markdownPath, "error", err.Error(), err) + return formatCompilerError(compilerErrorOpts{FilePath: markdownPath, ErrType: "error", Message: err.Error(), Cause: err}) } // Validate safe-job needs: declarations against known generated job IDs workflowLog.Printf("Validating safe-job needs declarations") if err := validateSafeJobNeeds(workflowData); err != nil { - return formatCompilerError(markdownPath, "error", err.Error(), err) + return formatCompilerError(compilerErrorOpts{FilePath: markdownPath, ErrType: "error", Message: err.Error(), Cause: err}) } // Emit warnings for push-to-pull-request-branch misconfiguration @@ -204,51 +204,51 @@ func (c *Compiler) validateToolConfiguration(workflowData *WorkflowData, markdow // Reject bare "*" in allowed-labels (CTR-015) workflowLog.Printf("Validating safe-outputs allowed-labels glob scope") if err := c.validateSafeOutputsAllowedLabelsGlobScope(workflowData.SafeOutputs); err != nil { - return formatCompilerError(markdownPath, "error", err.Error(), err) + return formatCompilerError(compilerErrorOpts{FilePath: markdownPath, ErrType: "error", Message: err.Error(), Cause: err}) } // Validate network allowed domains configuration workflowLog.Printf("Validating network allowed domains") if err := c.validateNetworkAllowedDomains(workflowData.NetworkPermissions); err != nil { - return formatCompilerError(markdownPath, "error", err.Error(), err) + return formatCompilerError(compilerErrorOpts{FilePath: markdownPath, ErrType: "error", Message: err.Error(), Cause: err}) } // Validate network firewall configuration workflowLog.Printf("Validating network firewall configuration") if err := validateNetworkFirewallConfig(workflowData.NetworkPermissions); err != nil { - return formatCompilerError(markdownPath, "error", err.Error(), err) + return formatCompilerError(compilerErrorOpts{FilePath: markdownPath, ErrType: "error", Message: err.Error(), Cause: err}) } // Validate safe-outputs allow-workflows requires GitHub App workflowLog.Printf("Validating safe-outputs allow-workflows") if err := validateSafeOutputsAllowWorkflows(workflowData.SafeOutputs); err != nil { - return formatCompilerError(markdownPath, "error", err.Error(), err) + return formatCompilerError(compilerErrorOpts{FilePath: markdownPath, ErrType: "error", Message: err.Error(), Cause: err}) } // Validate labels configuration workflowLog.Printf("Validating labels") if err := validateLabels(workflowData); err != nil { - return formatCompilerError(markdownPath, "error", err.Error(), err) + return formatCompilerError(compilerErrorOpts{FilePath: markdownPath, ErrType: "error", Message: err.Error(), Cause: err}) } // Validate workflow_dispatch required inputs with slash/label command triggers. workflowLog.Printf("Validating workflow_dispatch input requirements for command triggers") if err := validateCommandWorkflowDispatchInputs(workflowData); err != nil { - return formatCompilerError(markdownPath, "error", err.Error(), err) + return formatCompilerError(compilerErrorOpts{FilePath: markdownPath, ErrType: "error", Message: err.Error(), Cause: err}) } // Validate workflow-level concurrency group expression workflowLog.Printf("Validating workflow-level concurrency configuration") if workflowData.Concurrency != "" { if err := validateConcurrencyQueueConfiguration(workflowData.Concurrency); err != nil { - return formatCompilerError(markdownPath, "error", "workflow-level concurrency validation failed: "+err.Error(), err) + return formatCompilerError(compilerErrorOpts{FilePath: markdownPath, ErrType: "error", Message: "workflow-level concurrency validation failed: " + err.Error(), Cause: err}) } // Use the cached validation result from applyDefaults to avoid re-running the // expensive ExpressionParser (regex + tokenize + parse) on every validateWorkflowData call. if workflowData.CachedConcurrencyGroupExprSet { if workflowData.CachedConcurrencyGroupExprErr != nil { - return formatCompilerError(markdownPath, "error", "workflow-level concurrency validation failed: "+workflowData.CachedConcurrencyGroupExprErr.Error(), workflowData.CachedConcurrencyGroupExprErr) + return formatCompilerError(compilerErrorOpts{FilePath: markdownPath, ErrType: "error", Message: "workflow-level concurrency validation failed: " + workflowData.CachedConcurrencyGroupExprErr.Error(), Cause: workflowData.CachedConcurrencyGroupExprErr}) } } else { // Fallback: cache not populated (e.g. WorkflowData created without applyDefaults). @@ -257,7 +257,7 @@ func (c *Compiler) validateToolConfiguration(workflowData *WorkflowData, markdow groupExpr := extractConcurrencyGroupFromYAML(workflowData.Concurrency) if groupExpr != "" { if err := validateConcurrencyGroupExpression(groupExpr); err != nil { - return formatCompilerError(markdownPath, "error", "workflow-level concurrency validation failed: "+err.Error(), err) + return formatCompilerError(compilerErrorOpts{FilePath: markdownPath, ErrType: "error", Message: "workflow-level concurrency validation failed: " + err.Error(), Cause: err}) } } } @@ -266,7 +266,7 @@ func (c *Compiler) validateToolConfiguration(workflowData *WorkflowData, markdow // Validate concurrency.job-discriminator expression if workflowData.ConcurrencyJobDiscriminator != "" { if err := validateConcurrencyGroupExpression(workflowData.ConcurrencyJobDiscriminator); err != nil { - return formatCompilerError(markdownPath, "error", "concurrency.job-discriminator validation failed: "+err.Error(), err) + return formatCompilerError(compilerErrorOpts{FilePath: markdownPath, ErrType: "error", Message: "concurrency.job-discriminator validation failed: " + err.Error(), Cause: err}) } } @@ -274,14 +274,14 @@ func (c *Compiler) validateToolConfiguration(workflowData *WorkflowData, markdow workflowLog.Printf("Validating engine-level concurrency configuration") if workflowData.EngineConfig != nil && workflowData.EngineConfig.Concurrency != "" { if err := validateConcurrencyQueueConfiguration(workflowData.EngineConfig.Concurrency); err != nil { - return formatCompilerError(markdownPath, "error", "engine.concurrency validation failed: "+err.Error(), err) + return formatCompilerError(compilerErrorOpts{FilePath: markdownPath, ErrType: "error", Message: "engine.concurrency validation failed: " + err.Error(), Cause: err}) } // Extract the group expression from the engine concurrency YAML groupExpr := extractConcurrencyGroupFromYAML(workflowData.EngineConfig.Concurrency) if groupExpr != "" { if err := validateConcurrencyGroupExpression(groupExpr); err != nil { - return formatCompilerError(markdownPath, "error", "engine.concurrency validation failed: "+err.Error(), err) + return formatCompilerError(compilerErrorOpts{FilePath: markdownPath, ErrType: "error", Message: "engine.concurrency validation failed: " + err.Error(), Cause: err}) } } } @@ -289,7 +289,7 @@ func (c *Compiler) validateToolConfiguration(workflowData *WorkflowData, markdow // Validate safe-outputs concurrency group expression if workflowData.SafeOutputs != nil && workflowData.SafeOutputs.ConcurrencyGroup != "" { if err := validateConcurrencyGroupExpression(workflowData.SafeOutputs.ConcurrencyGroup); err != nil { - return formatCompilerError(markdownPath, "error", "safe-outputs.concurrency-group validation failed: "+err.Error(), err) + return formatCompilerError(compilerErrorOpts{FilePath: markdownPath, ErrType: "error", Message: "safe-outputs.concurrency-group validation failed: " + err.Error(), Cause: err}) } } @@ -323,7 +323,7 @@ func (c *Compiler) validateToolConfiguration(workflowData *WorkflowData, markdow // Validate: threat detection requires sandbox.agent to be enabled (detection runs inside AWF) if workflowData.SafeOutputs != nil && workflowData.SafeOutputs.ThreatDetection != nil && isAgentSandboxDisabled(workflowData) { - return formatCompilerError(markdownPath, "error", "threat detection requires sandbox.agent to be enabled. Threat detection runs inside the agent sandbox (AWF) with fully blocked network. Either enable sandbox.agent or use 'threat-detection: false' to disable the threat-detection configuration in safe-outputs.", errors.New("threat detection requires sandbox.agent")) + return formatCompilerError(compilerErrorOpts{FilePath: markdownPath, ErrType: "error", Message: "threat detection requires sandbox.agent to be enabled. Threat detection runs inside the agent sandbox (AWF) with fully blocked network. Either enable sandbox.agent or use 'threat-detection: false' to disable the threat-detection configuration in safe-outputs.", Cause: errors.New("threat detection requires sandbox.agent")}) } // Emit warning when assign-to-agent is used with github-app: but no explicit github-token:. @@ -406,7 +406,7 @@ func (c *Compiler) validateToolConfiguration(workflowData *WorkflowData, markdow // Validate that all allowed tools have their toolsets enabled if err := ValidateGitHubToolsAgainstToolsets(allowedTools, enabledToolsets); err != nil { - return formatCompilerError(markdownPath, "error", err.Error(), err) + return formatCompilerError(compilerErrorOpts{FilePath: markdownPath, ErrType: "error", Message: err.Error(), Cause: err}) } // Print informational message if "projects" toolset is explicitly specified @@ -432,7 +432,7 @@ func (c *Compiler) validateToolConfiguration(workflowData *WorkflowData, markdow message += "permissions:\n" message += " actions: read" - return formatCompilerError(markdownPath, "error", message, nil) + return formatCompilerError(compilerErrorOpts{FilePath: markdownPath, ErrType: "error", Message: message, Cause: nil}) } } @@ -441,8 +441,8 @@ func (c *Compiler) validateToolConfiguration(workflowData *WorkflowData, markdow if workflowData.ParsedFrontmatter != nil { for _, r := range workflowData.ParsedFrontmatter.Resources { if strings.Contains(r, "${{") { - return formatCompilerError(markdownPath, "error", - fmt.Sprintf("resources entry %q contains GitHub Actions expression syntax (${{) which is not allowed; use static paths only", r), nil) + return formatCompilerError(compilerErrorOpts{FilePath: markdownPath, ErrType: "error", Message: fmt.Sprintf("resources entry %q contains GitHub Actions expression syntax (${{) which is not allowed; use static paths only", r), Cause: nil}) + } } } @@ -450,19 +450,19 @@ func (c *Compiler) validateToolConfiguration(workflowData *WorkflowData, markdow // Validate dispatch-workflow configuration (independent of agentic-workflows tool) workflowLog.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), err) + return formatCompilerError(compilerErrorOpts{FilePath: markdownPath, ErrType: "error", Message: fmt.Sprintf("dispatch-workflow validation failed: %v", err), Cause: err}) } // Validate dispatch_repository configuration (independent of agentic-workflows tool) workflowLog.Print("Validating dispatch_repository configuration") if err := c.validateDispatchRepository(workflowData, markdownPath); err != nil { - return formatCompilerError(markdownPath, "error", fmt.Sprintf("dispatch_repository validation failed: %v", err), err) + return formatCompilerError(compilerErrorOpts{FilePath: markdownPath, ErrType: "error", Message: fmt.Sprintf("dispatch_repository validation failed: %v", err), Cause: err}) } // Validate call-workflow configuration (independent of agentic-workflows tool) workflowLog.Print("Validating call-workflow configuration") if err := c.validateCallWorkflow(workflowData, markdownPath); err != nil { - return formatCompilerError(markdownPath, "error", fmt.Sprintf("call-workflow validation failed: %v", err), err) + return formatCompilerError(compilerErrorOpts{FilePath: markdownPath, ErrType: "error", Message: fmt.Sprintf("call-workflow validation failed: %v", err), Cause: err}) } return nil diff --git a/pkg/workflow/markdown_security_scanner.go b/pkg/workflow/markdown_security_scanner.go index d8d119eb69e..e710773c281 100644 --- a/pkg/workflow/markdown_security_scanner.go +++ b/pkg/workflow/markdown_security_scanner.go @@ -147,7 +147,7 @@ func FormatSecurityFindings(findings []SecurityFinding, filePath string) string var sb strings.Builder fmt.Fprintf(&sb, "Security scan found %d issue(s) in workflow markdown:\n\n", len(findings)) - // Format each finding using formatCompilerErrorWithPosition for consistency + // Format each finding with explicit line/column information for consistency. for _, f := range findings { line := f.Line if line <= 0 { @@ -155,14 +155,7 @@ func FormatSecurityFindings(findings []SecurityFinding, filePath string) string } // Create a formatted error for this finding - findingErr := formatCompilerErrorWithPosition( - filePath, - line, - 1, // Column 1 (we don't have column info) - "error", - fmt.Sprintf("[%s] %s", f.Category, f.Description), - nil, - ) + findingErr := formatCompilerError(compilerErrorOpts{FilePath: filePath, Line: line, Column: 1, ErrType: "error", Message: fmt.Sprintf("[%s] %s", f.Category, f.Description), Cause: nil}) // Append the formatted error to our output sb.WriteString(findingErr.Error()) diff --git a/pkg/workflow/model_alias_validation.go b/pkg/workflow/model_alias_validation.go index 7e6c3ae7638..666f87d766e 100644 --- a/pkg/workflow/model_alias_validation.go +++ b/pkg/workflow/model_alias_validation.go @@ -110,10 +110,9 @@ func (c *Compiler) validateModelAliasMap( if engineModel != "" { literalText := ExpressionPattern.ReplaceAllString(engineModel, "") if strings.Contains(literalText, "*") { - return formatCompilerError(markdownPath, "error", - fmt.Sprintf("engine.model: glob patterns are not allowed in engine.model; "+ - "got %q — glob patterns may only appear in models alias list entries (V-MAF-004)", engineModel), - nil) + return formatCompilerError(compilerErrorOpts{FilePath: markdownPath, ErrType: "error", Message: fmt.Sprintf("engine.model: glob patterns are not allowed in engine.model; "+ + "got %q — glob patterns may only appear in models alias list entries (V-MAF-004)", engineModel), Cause: nil}) + } // Syntax and parameter checks ($-character parsing, known params) are // skipped for runtime-resolved expressions — they cannot be parsed at @@ -121,7 +120,7 @@ func (c *Compiler) validateModelAliasMap( if !containsExpression(engineModel) { // V-MAF-001 + V-MAF-006: validate syntax of engine.model. if errs := validateModelIdentifierStrings([]string{engineModel}, "engine.model"); len(errs) > 0 { - return formatCompilerError(markdownPath, "error", errs[0], nil) + return formatCompilerError(compilerErrorOpts{FilePath: markdownPath, ErrType: "error", Message: errs[0], Cause: nil}) } // V-MAF-011: warn about unrecognised parameter keys in engine.model. c.warnUnrecognizedModelParams([]string{engineModel}, markdownPath) @@ -137,7 +136,7 @@ func (c *Compiler) validateModelAliasMap( // V-MAF-001 + V-MAF-002 + V-MAF-003 + V-MAF-006: validate each entry string. if errs := validateModelIdentifierStrings(entries, "models."+displayKey(key)); len(errs) > 0 { - return formatCompilerError(markdownPath, "error", errs[0], nil) + return formatCompilerError(compilerErrorOpts{FilePath: markdownPath, ErrType: "error", Message: errs[0], Cause: nil}) } // V-MAF-011: warn about unrecognised parameter keys in each entry. @@ -163,9 +162,8 @@ func validateAliasKey(key, markdownPath string) error { } for _, forbidden := range []string{"/", "?", "&"} { if strings.Contains(key, forbidden) { - return formatCompilerError(markdownPath, "error", - fmt.Sprintf("models: alias key %q must not contain %q (V-MAF-005)", key, forbidden), - nil) + return formatCompilerError(compilerErrorOpts{FilePath: markdownPath, ErrType: "error", Message: fmt.Sprintf("models: alias key %q must not contain %q (V-MAF-005)", key, forbidden), Cause: nil}) + } } return nil @@ -259,11 +257,10 @@ func detectCircularModelAliases(aliasMap map[string][]string, markdownPath strin if cycle := dfsCycleCheck(key, aliasMap, visited, path); cycle != nil { // Format cycle chain for a clear error message. chain := strings.Join(append(cycle, cycle[0]), " → ") - return formatCompilerError(markdownPath, "error", - fmt.Sprintf("circular alias reference detected: %s\n\n"+ - "Circular alias references are prohibited. Remove or rewrite the cycle in the 'models:' "+ - "frontmatter section (V-MAF-010).", chain), - nil) + return formatCompilerError(compilerErrorOpts{FilePath: markdownPath, ErrType: "error", Message: fmt.Sprintf("circular alias reference detected: %s\n\n"+ + "Circular alias references are prohibited. Remove or rewrite the cycle in the 'models:' "+ + "frontmatter section (V-MAF-010).", chain), Cause: nil}) + } } diff --git a/pkg/workflow/permissions_compiler_validator.go b/pkg/workflow/permissions_compiler_validator.go index 01070df5533..95f27b42b5c 100644 --- a/pkg/workflow/permissions_compiler_validator.go +++ b/pkg/workflow/permissions_compiler_validator.go @@ -67,25 +67,25 @@ func (c *Compiler) validatePermissions(workflowData *WorkflowData, markdownPath scopeValidationErr = ValidatePermissionScopeNames(workflowData.Permissions) } if scopeValidationErr != nil { - return nil, formatCompilerError(markdownPath, "error", scopeValidationErr.Error(), scopeValidationErr) + return nil, formatCompilerError(compilerErrorOpts{FilePath: markdownPath, ErrType: "error", Message: scopeValidationErr.Error(), Cause: scopeValidationErr}) } // Validate dangerous permissions workflowLog.Printf("Validating dangerous permissions") if err := validateDangerousPermissions(workflowData, workflowPermissions); err != nil { - return nil, formatCompilerError(markdownPath, "error", err.Error(), err) + return nil, formatCompilerError(compilerErrorOpts{FilePath: markdownPath, ErrType: "error", Message: err.Error(), Cause: err}) } // Validate GitHub App-only permissions require a GitHub App to be configured workflowLog.Printf("Validating GitHub App-only permissions") if err := validateGitHubAppOnlyPermissions(workflowData, workflowPermissions); err != nil { - return nil, formatCompilerError(markdownPath, "error", err.Error(), err) + return nil, formatCompilerError(compilerErrorOpts{FilePath: markdownPath, ErrType: "error", Message: err.Error(), Cause: err}) } // Validate tools.github.github-app.permissions does not use "write" workflowLog.Printf("Validating GitHub MCP app permissions (no write)") if err := validateGitHubMCPAppPermissionsNoWrite(workflowData); err != nil { - return nil, formatCompilerError(markdownPath, "error", err.Error(), err) + return nil, formatCompilerError(compilerErrorOpts{FilePath: markdownPath, ErrType: "error", Message: err.Error(), Cause: err}) } // Warn when github-app.permissions is set in contexts that don't support it @@ -131,7 +131,7 @@ func (c *Compiler) validatePermissions(workflowData *WorkflowData, markdownPath downgradeToWarning := c.strictMode && shouldDowngradeDefaultToolsetPermissionError(workflowData.ParsedTools.GitHub) if c.strictMode && !downgradeToWarning { // In strict mode, missing permissions are errors - return nil, formatCompilerError(markdownPath, "error", message, nil) + return nil, formatCompilerError(compilerErrorOpts{FilePath: markdownPath, ErrType: "error", Message: message, Cause: nil}) } if downgradeToWarning { @@ -149,7 +149,7 @@ func (c *Compiler) validatePermissions(workflowData *WorkflowData, markdownPath // Enforce required id-token: write permission for OIDC auth users. if err := validateOIDCPermissions(workflowData, workflowPermissions); err != nil { - return nil, formatCompilerError(markdownPath, "error", err.Error(), err) + return nil, formatCompilerError(compilerErrorOpts{FilePath: markdownPath, ErrType: "error", Message: err.Error(), Cause: err}) } // Emit warning if id-token: write permission is detected diff --git a/pkg/workflow/pull_request_target_validation.go b/pkg/workflow/pull_request_target_validation.go index 6d8436f86aa..8c284b40fbf 100644 --- a/pkg/workflow/pull_request_target_validation.go +++ b/pkg/workflow/pull_request_target_validation.go @@ -133,7 +133,7 @@ func (c *Compiler) validatePullRequestTargetTrigger(workflowData *WorkflowData, "See: https://securitylab.github.com/resources/github-actions-preventing-pwn-requests/" if effectiveStrictMode { - return formatCompilerError(markdownPath, "error", message, nil) + return formatCompilerError(compilerErrorOpts{FilePath: markdownPath, ErrType: "error", Message: message, Cause: nil}) } // Non-strict mode: emit a warning so existing workflows continue to compile. diff --git a/pkg/workflow/runs_on_validation.go b/pkg/workflow/runs_on_validation.go index c160ff8bf34..9316ccfe534 100644 --- a/pkg/workflow/runs_on_validation.go +++ b/pkg/workflow/runs_on_validation.go @@ -50,14 +50,14 @@ func validateRunsOn(frontmatter map[string]any, markdownPath string) error { for _, label := range labels { lower := strings.ToLower(label) if strings.HasPrefix(lower, "macos-") || lower == "macos" { - return formatCompilerError(markdownPath, "error", - fmt.Sprintf("runner '%s' is not supported in agentic workflows.\n\n"+ - "macOS runners are not supported because agentic workflows rely on containers "+ - "for the secure Agent Workflow Firewall sandbox, and GitHub-hosted macOS runners "+ - "do not support container jobs.\n\n"+ - "Use 'ubuntu-latest' (default) or another Linux-based runner instead.\n\n"+ - "See %s for details.", - label, macOSRunnerFAQURL), nil) + return formatCompilerError(compilerErrorOpts{FilePath: markdownPath, ErrType: "error", Message: fmt.Sprintf("runner '%s' is not supported in agentic workflows.\n\n"+ + "macOS runners are not supported because agentic workflows rely on containers "+ + "for the secure Agent Workflow Firewall sandbox, and GitHub-hosted macOS runners "+ + "do not support container jobs.\n\n"+ + "Use 'ubuntu-latest' (default) or another Linux-based runner instead.\n\n"+ + "See %s for details.", + label, macOSRunnerFAQURL), Cause: nil}) + } } diff --git a/pkg/workflow/strings.go b/pkg/workflow/strings.go index 590f9ba241f..1eef1fe5139 100644 --- a/pkg/workflow/strings.go +++ b/pkg/workflow/strings.go @@ -93,6 +93,11 @@ import ( var stringsLog = logger.New("workflow:strings") +// SanitizeOptions and SanitizeName are intentionally re-exported from stringutil +// so workflow-facing callers can consistently use workflow's sanitize helpers. +// Keep external callers on this package-level entry point rather than mixing direct +// stringutil.SanitizeName usage. +// // SanitizeOptions configures the behavior of the SanitizeName function. type SanitizeOptions = stringutil.SanitizeOptions From 7428f45a8fa6f82bfca2a00151854c91ac0a7684 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 29 May 2026 01:44:21 +0000 Subject: [PATCH 3/4] Complete sanitizer consolidation and compiler error formatter refactor Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com> --- pkg/stringutil/sanitize.go | 13 ++++++++- pkg/workflow/compiler_error_formatter.go | 36 +++++++++++++++--------- pkg/workflow/runs_on_validation.go | 5 ++-- pkg/workflow/strings.go | 9 +++--- 4 files changed, 41 insertions(+), 22 deletions(-) diff --git a/pkg/stringutil/sanitize.go b/pkg/stringutil/sanitize.go index 19d08d0df43..6cad02e346f 100644 --- a/pkg/stringutil/sanitize.go +++ b/pkg/stringutil/sanitize.go @@ -304,7 +304,7 @@ func SanitizeForFilenameWithSeparator(slug, separator string) string { } var sb strings.Builder for _, r := range strings.ReplaceAll(slug, "/", separator) { - if (r >= 'a' && r <= 'z') || (r >= 'A' && r <= 'Z') || (r >= '0' && r <= '9') || r == '-' || r == '_' || r == '.' { + if isFilenameSafeRune(r) { sb.WriteRune(r) } else { sb.WriteString(separator) @@ -313,6 +313,17 @@ func SanitizeForFilenameWithSeparator(slug, separator string) string { return sb.String() } +// isFilenameSafeRune reports whether a rune can be preserved in sanitized +// filename strings without replacement. +func isFilenameSafeRune(r rune) bool { + return (r >= 'a' && r <= 'z') || + (r >= 'A' && r <= 'Z') || + (r >= '0' && r <= '9') || + r == '-' || + r == '_' || + r == '.' +} + // SanitizeForFilename converts a repository slug (owner/repo) to a filename-safe string. // Replaces "/" with "-" and any remaining non-alphanumeric characters (except "-", "_", ".") // with "-". Returns "clone-mode" if the slug is empty. diff --git a/pkg/workflow/compiler_error_formatter.go b/pkg/workflow/compiler_error_formatter.go index 2b592cdb048..8ddd14ebc54 100644 --- a/pkg/workflow/compiler_error_formatter.go +++ b/pkg/workflow/compiler_error_formatter.go @@ -21,14 +21,28 @@ type wrappedCompilerError struct { func (e *wrappedCompilerError) Error() string { return e.formatted } func (e *wrappedCompilerError) Unwrap() error { return e.cause } +func normalizePosition(value, defaultValue int) int { + if value <= 0 { + return defaultValue + } + return value +} + type compilerErrorOpts struct { + // FilePath is the source file path shown in the formatted diagnostic. FilePath string - Line int - Column int - ErrType string - Message string - Cause error - Context []string + // Line is the 1-based source line. Defaults to 1 when unset. + Line int + // Column is the 1-based source column. Defaults to 1 when unset. + Column int + // ErrType is the diagnostic kind (for example "error" or "warning"). + ErrType string + // Message is the human-readable diagnostic message. + Message string + // Cause is the optional wrapped underlying error. + Cause error + // Context contains optional source lines for Rust-style context rendering. + Context []string } // formatCompilerError creates a formatted compiler error from options. @@ -36,14 +50,8 @@ type compilerErrorOpts struct { // When Cause is a *WorkflowValidationError with Line > 0, the error's own // position (and file, when available) takes precedence. func formatCompilerError(opts compilerErrorOpts) error { - line := opts.Line - column := opts.Column - if line <= 0 { - line = 1 - } - if column <= 0 { - column = 1 - } + line := normalizePosition(opts.Line, 1) + column := normalizePosition(opts.Column, 1) // Promote precise source location from WorkflowValidationError when available so that // the emitted "file:line:col: error:" prefix points directly at the problematic field diff --git a/pkg/workflow/runs_on_validation.go b/pkg/workflow/runs_on_validation.go index 9316ccfe534..e1c2e9e20d5 100644 --- a/pkg/workflow/runs_on_validation.go +++ b/pkg/workflow/runs_on_validation.go @@ -50,13 +50,14 @@ func validateRunsOn(frontmatter map[string]any, markdownPath string) error { for _, label := range labels { lower := strings.ToLower(label) if strings.HasPrefix(lower, "macos-") || lower == "macos" { - return formatCompilerError(compilerErrorOpts{FilePath: markdownPath, ErrType: "error", Message: fmt.Sprintf("runner '%s' is not supported in agentic workflows.\n\n"+ + message := fmt.Sprintf("runner '%s' is not supported in agentic workflows.\n\n"+ "macOS runners are not supported because agentic workflows rely on containers "+ "for the secure Agent Workflow Firewall sandbox, and GitHub-hosted macOS runners "+ "do not support container jobs.\n\n"+ "Use 'ubuntu-latest' (default) or another Linux-based runner instead.\n\n"+ "See %s for details.", - label, macOSRunnerFAQURL), Cause: nil}) + label, macOSRunnerFAQURL) + return formatCompilerError(compilerErrorOpts{FilePath: markdownPath, ErrType: "error", Message: message, Cause: nil}) } } diff --git a/pkg/workflow/strings.go b/pkg/workflow/strings.go index 1eef1fe5139..566ef928104 100644 --- a/pkg/workflow/strings.go +++ b/pkg/workflow/strings.go @@ -93,14 +93,13 @@ import ( var stringsLog = logger.New("workflow:strings") -// SanitizeOptions and SanitizeName are intentionally re-exported from stringutil -// so workflow-facing callers can consistently use workflow's sanitize helpers. -// Keep external callers on this package-level entry point rather than mixing direct -// stringutil.SanitizeName usage. -// // SanitizeOptions configures the behavior of the SanitizeName function. type SanitizeOptions = stringutil.SanitizeOptions +// SanitizeName is intentionally re-exported from stringutil so workflow-facing +// callers can consistently use workflow's sanitize helpers. +// Keep external callers on this package-level entry point rather than mixing +// direct stringutil.SanitizeName usage. // SanitizeName sanitizes a string for use as an identifier, file name, or similar context. func SanitizeName(name string, opts *SanitizeOptions) string { return stringutil.SanitizeName(name, opts) From 49c107404f01ef87d044b3bc60fd8db6b7d656ff Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Fri, 29 May 2026 04:31:49 +0000 Subject: [PATCH 4/4] docs(adr): add draft ADR-35613 for sanitizer consolidation and options-based compiler error formatter Co-Authored-By: Claude Opus 4.7 (1M context) --- ...-options-based-compiler-error-formatter.md | 93 +++++++++++++++++++ 1 file changed, 93 insertions(+) create mode 100644 docs/adr/35613-consolidate-sanitizers-and-options-based-compiler-error-formatter.md diff --git a/docs/adr/35613-consolidate-sanitizers-and-options-based-compiler-error-formatter.md b/docs/adr/35613-consolidate-sanitizers-and-options-based-compiler-error-formatter.md new file mode 100644 index 00000000000..c32339daa31 --- /dev/null +++ b/docs/adr/35613-consolidate-sanitizers-and-options-based-compiler-error-formatter.md @@ -0,0 +1,93 @@ +# ADR-35613: Consolidate Sanitization Helpers and Unify Compiler Error Formatter as an Options-Based API + +**Date**: 2026-05-29 +**Status**: Draft +**Deciders**: Unknown + +--- + +## Part 1 — Narrative (Human-Friendly) + +### Context + +The codebase had grown three independent duplicate implementations of filename-style sanitization (`sanitizePath` in `pkg/parser/import_cache.go`, `sanitizeRepoPath` in `pkg/cli/update_command.go` and `pkg/cli/deploy_command.go`, and the package-level `stringutil.SanitizeForFilename`), each with subtly different separators (`_`, `__`, `-`). In parallel, the workflow compiler exposed three overloads for emitting formatted diagnostics — `formatCompilerError`, `formatCompilerErrorWithPosition`, and `formatCompilerErrorWithContext` — and each new piece of diagnostic metadata required adding yet another overload. A third inconsistency was that some callers reached `stringutil.SanitizeName` directly while others routed through the `pkg/workflow` re-export layer (`workflow.SanitizeName` / `workflow.SanitizeOptions`), making the intended layering ambiguous. The combined effect was duplicate logic, drift risk in sanitization, and an overload family that was about to grow further. + +### Decision + +We will consolidate the sanitization surface and the compiler-error formatter surface as follows: + +1. Add `stringutil.SanitizeForFilenameWithSeparator(slug, separator string)` as the single configurable filename-sanitization helper, and re-implement `stringutil.SanitizeForFilename` as a thin `"-"`-separator wrapper over it. Remove the duplicate `sanitizePath` and `sanitizeRepoPath` helpers and route every caller through the shared helper. +2. Collapse `formatCompilerError`, `formatCompilerErrorWithPosition`, and `formatCompilerErrorWithContext` into a single function `formatCompilerError(opts compilerErrorOpts)` where `compilerErrorOpts` carries `FilePath`, `Line`, `Column`, `ErrType`, `Message`, `Cause`, and `Context`. Defaults (`Line=1`, `Column=1`) and `WorkflowValidationError` location promotion are applied inside this single function. +3. Keep the `pkg/workflow` re-export layer (`SanitizeOptions` / `SanitizeName`) as the canonical entry point for workflow-package callers, and route the previously bypassing CLI caller (`pkg/cli/logs_run_processor.go`) through `workflow.SanitizeName` to enforce that layering. + +### Alternatives Considered + +#### Alternative 1: Add a fourth overload (`formatCompilerErrorWithContextAndCause`) + +Continue the existing pattern of adding a new overload each time the diagnostic carries an additional optional field. Rejected because the overload family was already at three with overlapping responsibilities, and each new metadata field (context, cause, structured validation info) would force a combinatorial expansion of overloads and call sites. The team has already had to add `formatCompilerErrorWithContext` once; adding a fourth would entrench the pattern. + +#### Alternative 2: Functional options pattern (`formatCompilerError(opts ...Option)`) + +Use Go's idiomatic functional-options pattern with helpers like `WithLine(n)`, `WithContext(lines)`, etc. Rejected as heavier weight than needed: the diagnostic shape is small, the option set is closed, and the struct literal at the call site is at least as readable as a chain of `WithX(...)` calls while requiring no per-field helper functions. A struct also lets the `WorkflowValidationError`-promotion logic mutate one struct field in place rather than threading through an option pipeline. + +#### Alternative 3: Move `SanitizeName` fully into `stringutil` and delete the `workflow` re-exports + +Delete `workflow.SanitizeOptions` / `workflow.SanitizeName` and require all callers to import `stringutil` directly. Rejected because the re-export layer documents an intentional boundary — workflow-facing callers should depend on `pkg/workflow`, not reach across into `pkg/stringutil`. Deleting the re-export would push that import-graph concern onto every workflow consumer and make the layering harder to enforce by review. The chosen direction documents the re-export's intent in `pkg/workflow/strings.go` and migrates the one bypassing caller. + +#### Alternative 4: Per-caller bespoke sanitizers + +Leave each caller with its own small sanitizer (the status quo). Rejected because the separators were already drifting (`_` vs `__` vs `-`) and the only thing distinguishing the helpers was the separator string — a parameter, not a behavior. + +### Consequences + +#### Positive +- Single source of truth for repository-slug / path filename sanitization, parameterized only by separator. +- A single, extensible compiler-error formatter: adding a new diagnostic field is a struct-field addition, not a new function. +- The `pkg/workflow` re-export layer is now the only sanctioned path for workflow-package sanitization, making layering reviewable by import inspection. +- `WorkflowValidationError` position promotion is implemented exactly once, instead of risking drift across overloads. + +#### Negative +- Call sites are more verbose: `formatCompilerError(compilerErrorOpts{FilePath: x, ErrType: "error", Message: m, Cause: e})` is longer than the previous positional form, and many call sites had to be rewritten in this PR. +- The struct-based API loses compile-time enforcement that `Message` is supplied; a caller could construct `compilerErrorOpts{}` and emit an empty diagnostic. +- `SanitizeForFilenameWithSeparator` silently falls back to `"-"` when given an empty separator, which is convenient but hides a likely bug at the call site. + +#### Neutral +- Introduces two small internal helpers (`isFilenameSafeRune`, `normalizePosition`) that did not previously exist. +- The PR touches a large number of unrelated workflow validation files purely to update call sites; this is mechanical churn, not behavior change. +- `pkg/cli/logs_run_processor.go` now imports `pkg/workflow` in addition to `pkg/stringutil` because of the re-export routing decision. + +--- + +## Part 2 — Normative Specification (RFC 2119) + +> The key words **MUST**, **MUST NOT**, **REQUIRED**, **SHALL**, **SHALL NOT**, **SHOULD**, **SHOULD NOT**, **RECOMMENDED**, **MAY**, and **OPTIONAL** in this section are to be interpreted as described in [RFC 2119](https://www.rfc-editor.org/rfc/rfc2119). + +### Filename Sanitization + +1. New code that converts a repository slug or path to a filename-safe string **MUST** use `stringutil.SanitizeForFilenameWithSeparator(slug, separator)` or its `"-"`-separator wrapper `stringutil.SanitizeForFilename(slug)`. +2. Packages other than `pkg/stringutil` **MUST NOT** define local helpers that replicate filename sanitization (for example, helpers named `sanitizePath`, `sanitizeRepoPath`, or equivalents). +3. Callers that previously relied on a non-`"-"` separator (for example `"_"` for cache paths, `"__"` for repo checkout directories) **MUST** pass that separator explicitly to `SanitizeForFilenameWithSeparator` rather than reintroducing a local sanitizer. +4. `SanitizeForFilenameWithSeparator` **SHOULD** treat an empty `separator` argument as a defaulting hint to `"-"`; callers **SHOULD NOT** rely on this fallback and **SHOULD** pass an explicit separator. + +### Compiler Error Formatting + +1. Code in `pkg/workflow` that emits a console-formatted compiler diagnostic **MUST** use `formatCompilerError(compilerErrorOpts{...})`. +2. Code in `pkg/workflow` **MUST NOT** reintroduce overloads such as `formatCompilerErrorWithPosition` or `formatCompilerErrorWithContext`. Additional diagnostic metadata **MUST** be added as new fields on `compilerErrorOpts`. +3. Callers **MUST** populate at minimum `FilePath`, `ErrType`, and `Message` on `compilerErrorOpts`. Callers **MAY** omit `Line` and `Column`, in which case both default to `1`. +4. Callers **MUST NOT** pre-promote `WorkflowValidationError` position/file into `compilerErrorOpts.Line` / `Column` / `FilePath`; this promotion is the responsibility of `formatCompilerError` itself. +5. Callers that have source-context lines available **SHOULD** populate `compilerErrorOpts.Context` so that Rust-style snippet rendering is used. +6. Code that needs to detect whether an `error` is already a console-formatted compiler diagnostic **MUST** call `isFormattedCompilerError(err)` and **MUST NOT** rely on string-contains checks against the formatted text. + +### SanitizeName Layering + +1. Callers within `pkg/workflow` and its CLI consumers **MUST** use `workflow.SanitizeName` and `workflow.SanitizeOptions` rather than `stringutil.SanitizeName` / `stringutil.SanitizeOptions` directly. +2. Code in `pkg/stringutil` **MUST** remain the canonical implementation of `SanitizeName`; `pkg/workflow/strings.go` **MUST** continue to be a thin re-export. +3. New non-workflow packages that need name sanitization **MAY** call `stringutil.SanitizeName` directly; the re-export rule applies to workflow-facing callers only. + +### Conformance + +An implementation is considered conformant with this ADR if it satisfies all **MUST** and **MUST NOT** requirements above. Failure to meet any **MUST** or **MUST NOT** requirement constitutes non-conformance. + +--- + +*This is a DRAFT ADR generated by the [Design Decision Gate](https://github.com/github/gh-aw/actions/runs/26617861244) workflow. The PR author must review, complete, and finalize this document before the PR can merge.*