From 4f3ef423c07a2c4b324f6606c5218010be4d8c0a Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 19 Feb 2026 18:01:18 +0000 Subject: [PATCH 1/3] Initial plan From 38ced37809974de0ef6d82a472f59a0c540108ab Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 19 Feb 2026 18:12:24 +0000 Subject: [PATCH 2/3] Initial plan for semantic function clustering refactoring Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- .github/workflows/smoke-macos-arm64.lock.yml | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/.github/workflows/smoke-macos-arm64.lock.yml b/.github/workflows/smoke-macos-arm64.lock.yml index 959e04a4304..8b39539afea 100644 --- a/.github/workflows/smoke-macos-arm64.lock.yml +++ b/.github/workflows/smoke-macos-arm64.lock.yml @@ -2085,6 +2085,13 @@ jobs: setupGlobals(core, github, context, exec, io); const { main } = require('/opt/gh-aw/actions/safe_output_handler_manager.cjs'); await main(); + - name: Upload safe output items manifest + if: always() + uses: actions/upload-artifact@b7c566a772e6b6bfb58ed0dc250532a479d7789f # v6 + with: + name: safe-output-items + path: /tmp/safe-output-items.jsonl + if-no-files-found: warn send_slack_message: needs: From 624293bd7c879af4a861a711c48e32464677fc25 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 19 Feb 2026 18:30:37 +0000 Subject: [PATCH 3/3] Semantic function clustering refactoring: consolidate duplicates and reduce file fragmentation" Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/cli/add_integration_test.go | 2 +- pkg/cli/audit.go | 2 +- pkg/cli/audit_report_helpers_test.go | 2 +- pkg/cli/compile_integration_test.go | 2 +- pkg/cli/fileutil/fileutil.go | 71 ------ pkg/cli/fileutil/fileutil_test.go | 237 ------------------ pkg/cli/logs_download.go | 2 +- pkg/cli/logs_download_test.go | 2 +- pkg/cli/logs_metrics.go | 2 +- pkg/cli/logs_parsing_core.go | 2 +- pkg/cli/logs_parsing_firewall.go | 2 +- pkg/cli/trial_command.go | 2 +- pkg/fileutil/fileutil.go | 66 +++++ pkg/parser/ansi_strip.go | 105 +------- pkg/stringutil/ansi.go | 114 +++++++++ pkg/stringutil/stringutil.go | 10 +- pkg/workflow/compiler_activation_jobs.go | 4 +- pkg/workflow/compiler_yaml.go | 12 +- pkg/workflow/dispatch_workflow_validation.go | 7 +- ...neration.go => git_configuration_steps.go} | 0 pkg/workflow/prompt_step_helper.go | 120 --------- pkg/workflow/safe_outputs_config_helpers.go | 102 ++++++++ .../safe_outputs_config_helpers_reflection.go | 107 -------- pkg/workflow/time_delta.go | 31 +++ pkg/workflow/time_delta_constants.go | 32 --- pkg/workflow/unified_prompt_step.go | 39 +++ pkg/workflow/validation_helpers.go | 60 ----- pkg/workflow/validation_helpers_test.go | 13 +- 28 files changed, 388 insertions(+), 762 deletions(-) delete mode 100644 pkg/cli/fileutil/fileutil.go delete mode 100644 pkg/cli/fileutil/fileutil_test.go create mode 100644 pkg/stringutil/ansi.go rename pkg/workflow/{yaml_generation.go => git_configuration_steps.go} (100%) delete mode 100644 pkg/workflow/prompt_step_helper.go delete mode 100644 pkg/workflow/safe_outputs_config_helpers_reflection.go delete mode 100644 pkg/workflow/time_delta_constants.go diff --git a/pkg/cli/add_integration_test.go b/pkg/cli/add_integration_test.go index b26b3838fdd..8ed93a71a04 100644 --- a/pkg/cli/add_integration_test.go +++ b/pkg/cli/add_integration_test.go @@ -9,7 +9,7 @@ import ( "strings" "testing" - "github.com/github/gh-aw/pkg/cli/fileutil" + "github.com/github/gh-aw/pkg/fileutil" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) diff --git a/pkg/cli/audit.go b/pkg/cli/audit.go index 54b2448a4ee..7b5c9e19e8c 100644 --- a/pkg/cli/audit.go +++ b/pkg/cli/audit.go @@ -10,9 +10,9 @@ import ( "strings" "time" - "github.com/github/gh-aw/pkg/cli/fileutil" "github.com/github/gh-aw/pkg/console" "github.com/github/gh-aw/pkg/constants" + "github.com/github/gh-aw/pkg/fileutil" "github.com/github/gh-aw/pkg/logger" "github.com/github/gh-aw/pkg/parser" "github.com/github/gh-aw/pkg/timeutil" diff --git a/pkg/cli/audit_report_helpers_test.go b/pkg/cli/audit_report_helpers_test.go index f36a98ea85f..d837c9771b8 100644 --- a/pkg/cli/audit_report_helpers_test.go +++ b/pkg/cli/audit_report_helpers_test.go @@ -10,7 +10,7 @@ import ( "testing" "time" - "github.com/github/gh-aw/pkg/cli/fileutil" + "github.com/github/gh-aw/pkg/fileutil" "github.com/github/gh-aw/pkg/stringutil" "github.com/github/gh-aw/pkg/testutil" ) diff --git a/pkg/cli/compile_integration_test.go b/pkg/cli/compile_integration_test.go index 1797a376e75..968c87486e0 100644 --- a/pkg/cli/compile_integration_test.go +++ b/pkg/cli/compile_integration_test.go @@ -13,7 +13,7 @@ import ( "time" "github.com/creack/pty" - "github.com/github/gh-aw/pkg/cli/fileutil" + "github.com/github/gh-aw/pkg/fileutil" ) // Global binary path shared across all integration tests diff --git a/pkg/cli/fileutil/fileutil.go b/pkg/cli/fileutil/fileutil.go deleted file mode 100644 index 2704bd7514a..00000000000 --- a/pkg/cli/fileutil/fileutil.go +++ /dev/null @@ -1,71 +0,0 @@ -package fileutil - -import ( - "io" - "os" - "path/filepath" -) - -// FileExists checks if a file exists and is not a directory. -func FileExists(path string) bool { - info, err := os.Stat(path) - if err != nil { - return false - } - return !info.IsDir() -} - -// DirExists checks if a directory exists. -func DirExists(path string) bool { - info, err := os.Stat(path) - if err != nil { - return false - } - return info.IsDir() -} - -// IsDirEmpty checks if a directory is empty. -func IsDirEmpty(path string) bool { - files, err := os.ReadDir(path) - if err != nil { - return true // Consider it empty if we can't read it - } - return len(files) == 0 -} - -// CopyFile copies a file from src to dst using buffered IO. -func CopyFile(src, dst string) error { - in, err := os.Open(src) - if err != nil { - return err - } - defer in.Close() - - out, err := os.Create(dst) - if err != nil { - return err - } - defer func() { _ = out.Close() }() - - if _, err = io.Copy(out, in); err != nil { - return err - } - return out.Sync() -} - -// CalculateDirectorySize recursively calculates the total size of files in a directory. -func CalculateDirectorySize(dirPath string) int64 { - var totalSize int64 - - _ = filepath.Walk(dirPath, func(path string, info os.FileInfo, err error) error { - if err != nil { - return nil - } - if !info.IsDir() { - totalSize += info.Size() - } - return nil - }) - - return totalSize -} diff --git a/pkg/cli/fileutil/fileutil_test.go b/pkg/cli/fileutil/fileutil_test.go deleted file mode 100644 index 1e3bc5372b7..00000000000 --- a/pkg/cli/fileutil/fileutil_test.go +++ /dev/null @@ -1,237 +0,0 @@ -//go:build !integration - -package fileutil - -import ( - "os" - "path/filepath" - "runtime" - "testing" - - "github.com/github/gh-aw/pkg/testutil" -) - -func TestFileExists(t *testing.T) { - // Create a temporary directory - tmpDir := testutil.TempDir(t, "test-*") - - // Test with existing file - testFile := filepath.Join(tmpDir, "testfile.txt") - if err := os.WriteFile(testFile, []byte("test content"), 0644); err != nil { - t.Fatalf("Failed to create test file: %v", err) - } - - if !FileExists(testFile) { - t.Errorf("FileExists() = false, want true for existing file") - } - - // Test with non-existent file - nonExistentFile := filepath.Join(tmpDir, "nonexistent.txt") - if FileExists(nonExistentFile) { - t.Errorf("FileExists() = true, want false for non-existent file") - } - - // Test with directory (should return false) - testDir := filepath.Join(tmpDir, "testdir") - if err := os.Mkdir(testDir, 0755); err != nil { - t.Fatalf("Failed to create test directory: %v", err) - } - - if FileExists(testDir) { - t.Errorf("FileExists() = true, want false for directory") - } -} - -func TestDirExists(t *testing.T) { - // Create a temporary directory - tmpDir := testutil.TempDir(t, "test-*") - - // Test with existing directory - testDir := filepath.Join(tmpDir, "testdir") - if err := os.Mkdir(testDir, 0755); err != nil { - t.Fatalf("Failed to create test directory: %v", err) - } - - if !DirExists(testDir) { - t.Errorf("DirExists() = false, want true for existing directory") - } - - // Test with non-existent directory - nonExistentDir := filepath.Join(tmpDir, "nonexistent") - if DirExists(nonExistentDir) { - t.Errorf("DirExists() = true, want false for non-existent directory") - } - - // Test with file (should return false) - testFile := filepath.Join(tmpDir, "testfile.txt") - if err := os.WriteFile(testFile, []byte("test"), 0644); err != nil { - t.Fatalf("Failed to create test file: %v", err) - } - - if DirExists(testFile) { - t.Errorf("DirExists() = true, want false for file") - } - - // Test with inaccessible path (permission denied) - if runtime.GOOS != "windows" && os.Getuid() != 0 { - parentDir := filepath.Join(tmpDir, "noaccess") - childDir := filepath.Join(parentDir, "child") - if err := os.MkdirAll(childDir, 0755); err != nil { - t.Fatalf("Failed to create child dir: %v", err) - } - if err := os.Chmod(parentDir, 0000); err != nil { - t.Fatalf("Failed to chmod parent dir: %v", err) - } - t.Cleanup(func() { os.Chmod(parentDir, 0755) }) - - if DirExists(childDir) { - t.Errorf("DirExists() = true, want false for inaccessible path") - } - } -} - -func TestIsDirEmpty(t *testing.T) { - // Create a temporary directory - tmpDir := testutil.TempDir(t, "test-*") - - // Test with empty directory - emptyDir := filepath.Join(tmpDir, "empty") - if err := os.Mkdir(emptyDir, 0755); err != nil { - t.Fatalf("Failed to create empty directory: %v", err) - } - - if !IsDirEmpty(emptyDir) { - t.Errorf("IsDirEmpty() = false, want true for empty directory") - } - - // Test with non-empty directory - nonEmptyDir := filepath.Join(tmpDir, "nonempty") - if err := os.Mkdir(nonEmptyDir, 0755); err != nil { - t.Fatalf("Failed to create non-empty directory: %v", err) - } - - testFile := filepath.Join(nonEmptyDir, "file.txt") - if err := os.WriteFile(testFile, []byte("content"), 0644); err != nil { - t.Fatalf("Failed to create test file: %v", err) - } - - if IsDirEmpty(nonEmptyDir) { - t.Errorf("IsDirEmpty() = true, want false for non-empty directory") - } - - // Test with non-existent directory (should return true) - nonExistentDir := filepath.Join(tmpDir, "nonexistent") - if !IsDirEmpty(nonExistentDir) { - t.Errorf("IsDirEmpty() = false, want true for non-existent directory") - } -} - -func TestCopyFile(t *testing.T) { - // Create a temporary directory - tmpDir := testutil.TempDir(t, "test-*") - - // Test successful copy - srcFile := filepath.Join(tmpDir, "source.txt") - srcContent := []byte("test content for copy") - if err := os.WriteFile(srcFile, srcContent, 0644); err != nil { - t.Fatalf("Failed to create source file: %v", err) - } - - dstFile := filepath.Join(tmpDir, "destination.txt") - if err := CopyFile(srcFile, dstFile); err != nil { - t.Errorf("CopyFile() error = %v, want nil", err) - } - - // Verify destination file exists and has same content - if !FileExists(dstFile) { - t.Errorf("Destination file does not exist after copy") - } - - dstContent, err := os.ReadFile(dstFile) - if err != nil { - t.Fatalf("Failed to read destination file: %v", err) - } - - if string(dstContent) != string(srcContent) { - t.Errorf("Destination file content = %q, want %q", dstContent, srcContent) - } - - // Test copy with non-existent source - nonExistentSrc := filepath.Join(tmpDir, "nonexistent.txt") - invalidDst := filepath.Join(tmpDir, "invalid.txt") - if err := CopyFile(nonExistentSrc, invalidDst); err == nil { - t.Errorf("CopyFile() with non-existent source: error = nil, want error") - } - - // Test copy to invalid destination (subdirectory doesn't exist) - invalidDstDir := filepath.Join(tmpDir, "nonexistent", "destination.txt") - if err := CopyFile(srcFile, invalidDstDir); err == nil { - t.Errorf("CopyFile() with invalid destination: error = nil, want error") - } -} - -func TestCalculateDirectorySize(t *testing.T) { - // Create a temporary directory - tmpDir := testutil.TempDir(t, "test-*") - - // Test with empty directory - emptyDir := filepath.Join(tmpDir, "empty") - if err := os.Mkdir(emptyDir, 0755); err != nil { - t.Fatalf("Failed to create empty directory: %v", err) - } - - size := CalculateDirectorySize(emptyDir) - if size != 0 { - t.Errorf("CalculateDirectorySize(empty) = %d, want 0", size) - } - - // Test with directory containing files - testDir := filepath.Join(tmpDir, "withfiles") - if err := os.Mkdir(testDir, 0755); err != nil { - t.Fatalf("Failed to create test directory: %v", err) - } - - // Create files with known sizes - file1 := filepath.Join(testDir, "file1.txt") - file1Content := []byte("12345") // 5 bytes - if err := os.WriteFile(file1, file1Content, 0644); err != nil { - t.Fatalf("Failed to create file1: %v", err) - } - - file2 := filepath.Join(testDir, "file2.txt") - file2Content := []byte("1234567890") // 10 bytes - if err := os.WriteFile(file2, file2Content, 0644); err != nil { - t.Fatalf("Failed to create file2: %v", err) - } - - expectedSize := int64(len(file1Content) + len(file2Content)) - actualSize := CalculateDirectorySize(testDir) - if actualSize != expectedSize { - t.Errorf("CalculateDirectorySize() = %d, want %d", actualSize, expectedSize) - } - - // Test with nested directories - nestedDir := filepath.Join(testDir, "nested") - if err := os.Mkdir(nestedDir, 0755); err != nil { - t.Fatalf("Failed to create nested directory: %v", err) - } - - file3 := filepath.Join(nestedDir, "file3.txt") - file3Content := []byte("abc") // 3 bytes - if err := os.WriteFile(file3, file3Content, 0644); err != nil { - t.Fatalf("Failed to create file3: %v", err) - } - - expectedSizeWithNested := expectedSize + int64(len(file3Content)) - actualSizeWithNested := CalculateDirectorySize(testDir) - if actualSizeWithNested != expectedSizeWithNested { - t.Errorf("CalculateDirectorySize(with nested) = %d, want %d", actualSizeWithNested, expectedSizeWithNested) - } - - // Test with non-existent directory (should return 0) - nonExistentDir := filepath.Join(tmpDir, "nonexistent") - nonExistentSize := CalculateDirectorySize(nonExistentDir) - if nonExistentSize != 0 { - t.Errorf("CalculateDirectorySize(non-existent) = %d, want 0", nonExistentSize) - } -} diff --git a/pkg/cli/logs_download.go b/pkg/cli/logs_download.go index 5e19248159a..9b83905523e 100644 --- a/pkg/cli/logs_download.go +++ b/pkg/cli/logs_download.go @@ -19,8 +19,8 @@ import ( "strconv" "strings" - "github.com/github/gh-aw/pkg/cli/fileutil" "github.com/github/gh-aw/pkg/console" + "github.com/github/gh-aw/pkg/fileutil" "github.com/github/gh-aw/pkg/logger" "github.com/github/gh-aw/pkg/workflow" ) diff --git a/pkg/cli/logs_download_test.go b/pkg/cli/logs_download_test.go index 543db49cab9..24c21644850 100644 --- a/pkg/cli/logs_download_test.go +++ b/pkg/cli/logs_download_test.go @@ -11,7 +11,7 @@ import ( "strings" "testing" - "github.com/github/gh-aw/pkg/cli/fileutil" + "github.com/github/gh-aw/pkg/fileutil" "github.com/github/gh-aw/pkg/testutil" ) diff --git a/pkg/cli/logs_metrics.go b/pkg/cli/logs_metrics.go index 4550663a1df..4d7c9350783 100644 --- a/pkg/cli/logs_metrics.go +++ b/pkg/cli/logs_metrics.go @@ -19,9 +19,9 @@ import ( "path/filepath" "strings" - "github.com/github/gh-aw/pkg/cli/fileutil" "github.com/github/gh-aw/pkg/console" "github.com/github/gh-aw/pkg/constants" + "github.com/github/gh-aw/pkg/fileutil" "github.com/github/gh-aw/pkg/logger" "github.com/github/gh-aw/pkg/workflow" ) diff --git a/pkg/cli/logs_parsing_core.go b/pkg/cli/logs_parsing_core.go index e6ef1995520..341211b7fa8 100644 --- a/pkg/cli/logs_parsing_core.go +++ b/pkg/cli/logs_parsing_core.go @@ -17,9 +17,9 @@ import ( "path/filepath" "strings" - "github.com/github/gh-aw/pkg/cli/fileutil" "github.com/github/gh-aw/pkg/console" "github.com/github/gh-aw/pkg/constants" + "github.com/github/gh-aw/pkg/fileutil" "github.com/github/gh-aw/pkg/logger" "github.com/github/gh-aw/pkg/workflow" ) diff --git a/pkg/cli/logs_parsing_firewall.go b/pkg/cli/logs_parsing_firewall.go index 6bf485d49c2..67030e08d94 100644 --- a/pkg/cli/logs_parsing_firewall.go +++ b/pkg/cli/logs_parsing_firewall.go @@ -16,8 +16,8 @@ import ( "path/filepath" "strings" - "github.com/github/gh-aw/pkg/cli/fileutil" "github.com/github/gh-aw/pkg/console" + "github.com/github/gh-aw/pkg/fileutil" "github.com/github/gh-aw/pkg/logger" "github.com/github/gh-aw/pkg/workflow" ) diff --git a/pkg/cli/trial_command.go b/pkg/cli/trial_command.go index d437b7e67ed..b5dcc11eb48 100644 --- a/pkg/cli/trial_command.go +++ b/pkg/cli/trial_command.go @@ -11,9 +11,9 @@ import ( "strings" "time" - "github.com/github/gh-aw/pkg/cli/fileutil" "github.com/github/gh-aw/pkg/console" "github.com/github/gh-aw/pkg/constants" + "github.com/github/gh-aw/pkg/fileutil" "github.com/github/gh-aw/pkg/logger" "github.com/github/gh-aw/pkg/repoutil" "github.com/github/gh-aw/pkg/workflow" diff --git a/pkg/fileutil/fileutil.go b/pkg/fileutil/fileutil.go index 80e34cb47cb..54e5bd4ad01 100644 --- a/pkg/fileutil/fileutil.go +++ b/pkg/fileutil/fileutil.go @@ -3,6 +3,8 @@ package fileutil import ( "fmt" + "io" + "os" "path/filepath" ) @@ -43,3 +45,67 @@ func ValidateAbsolutePath(path string) (string, error) { return cleanPath, nil } + +// FileExists checks if a file exists and is not a directory. +func FileExists(path string) bool { + info, err := os.Stat(path) + if err != nil { + return false + } + return !info.IsDir() +} + +// DirExists checks if a directory exists. +func DirExists(path string) bool { + info, err := os.Stat(path) + if err != nil { + return false + } + return info.IsDir() +} + +// IsDirEmpty checks if a directory is empty. +func IsDirEmpty(path string) bool { + files, err := os.ReadDir(path) + if err != nil { + return true // Consider it empty if we can't read it + } + return len(files) == 0 +} + +// CopyFile copies a file from src to dst using buffered IO. +func CopyFile(src, dst string) error { + in, err := os.Open(src) + if err != nil { + return err + } + defer in.Close() + + out, err := os.Create(dst) + if err != nil { + return err + } + defer func() { _ = out.Close() }() + + if _, err = io.Copy(out, in); err != nil { + return err + } + return out.Sync() +} + +// CalculateDirectorySize recursively calculates the total size of files in a directory. +func CalculateDirectorySize(dirPath string) int64 { + var totalSize int64 + + _ = filepath.Walk(dirPath, func(path string, info os.FileInfo, err error) error { + if err != nil { + return nil + } + if !info.IsDir() { + totalSize += info.Size() + } + return nil + }) + + return totalSize +} diff --git a/pkg/parser/ansi_strip.go b/pkg/parser/ansi_strip.go index d7a1825d8ce..ad905765621 100644 --- a/pkg/parser/ansi_strip.go +++ b/pkg/parser/ansi_strip.go @@ -1,111 +1,14 @@ package parser import ( - "strings" - - "github.com/github/gh-aw/pkg/logger" + "github.com/github/gh-aw/pkg/stringutil" ) -var ansiStripLog = logger.New("parser:ansi_strip") - // StripANSI removes ANSI escape codes from a string. +// This is a thin wrapper around stringutil.StripANSI for backward compatibility. +// The comprehensive implementation lives in pkg/stringutil/ansi.go. func StripANSI(s string) string { - if ansiStripLog.Enabled() { - ansiStripLog.Printf("Stripping ANSI codes from string: length=%d", len(s)) - } - - if s == "" { - return s - } - - var result strings.Builder - result.Grow(len(s)) // Pre-allocate capacity for efficiency - - i := 0 - escapeSequences := 0 - for i < len(s) { - if s[i] == '\x1b' { - escapeSequences++ - if i+1 >= len(s) { - // ESC at end of string, skip it - i++ - continue - } - // Found ESC character, determine sequence type - switch s[i+1] { - case '[': - // CSI sequence: \x1b[...final_char - // Parameters are in range 0x30-0x3F (0-?), intermediate chars 0x20-0x2F (space-/) - // Final characters are in range 0x40-0x7E (@-~) - i += 2 // Skip ESC and [ - for i < len(s) { - if isFinalCSIChar(s[i]) { - i++ // Skip the final character - break - } else if isCSIParameterChar(s[i]) { - i++ // Skip parameter/intermediate character - } else { - // Invalid character in CSI sequence, stop processing this escape - break - } - } - case ']': - // OSC sequence: \x1b]...terminator - // Terminators: \x07 (BEL) or \x1b\\ (ST) - i += 2 // Skip ESC and ] - for i < len(s) { - if s[i] == '\x07' { - i++ // Skip BEL - break - } else if s[i] == '\x1b' && i+1 < len(s) && s[i+1] == '\\' { - i += 2 // Skip ESC and \ - break - } - i++ - } - case '(': - // G0 character set selection: \x1b(char - i += 2 // Skip ESC and ( - if i < len(s) { - i++ // Skip the character - } - case ')': - // G1 character set selection: \x1b)char - i += 2 // Skip ESC and ) - if i < len(s) { - i++ // Skip the character - } - case '=': - // Application keypad mode: \x1b= - i += 2 - case '>': - // Normal keypad mode: \x1b> - i += 2 - case 'c': - // Reset: \x1bc - i += 2 - default: - // Other escape sequences (2-character) - // Handle common ones like \x1b7, \x1b8, \x1bD, \x1bE, \x1bH, \x1bM - if i+1 < len(s) && (s[i+1] >= '0' && s[i+1] <= '~') { - i += 2 - } else { - // Invalid or incomplete escape sequence, just skip ESC - i++ - } - } - } else { - // Regular character, keep it - result.WriteByte(s[i]) - i++ - } - } - - if ansiStripLog.Enabled() { - ansiStripLog.Printf("ANSI stripping complete: input_length=%d, output_length=%d, escape_sequences=%d", len(s), result.Len(), escapeSequences) - } - - return result.String() + return stringutil.StripANSI(s) } // isFinalCSIChar checks if a character is a valid CSI final character diff --git a/pkg/stringutil/ansi.go b/pkg/stringutil/ansi.go new file mode 100644 index 00000000000..648bf00df50 --- /dev/null +++ b/pkg/stringutil/ansi.go @@ -0,0 +1,114 @@ +// Package stringutil provides utility functions for working with strings. +package stringutil + +import ( + "strings" +) + +// StripANSI removes ANSI escape codes from a string using a comprehensive byte scanner. +// It handles CSI sequences (\x1b[), OSC sequences (\x1b]), G0/G1 character set selections, +// keypad mode sequences, reset sequences, and other common 2-character escape sequences. +// +// This is more thorough than regex-based approaches and correctly handles edge cases +// such as incomplete sequences, nested sequences, and non-standard terminal sequences. +func StripANSI(s string) string { + if s == "" { + return s + } + + var result strings.Builder + result.Grow(len(s)) // Pre-allocate capacity for efficiency + + i := 0 + for i < len(s) { + if s[i] == '\x1b' { + if i+1 >= len(s) { + // ESC at end of string, skip it + i++ + continue + } + // Found ESC character, determine sequence type + switch s[i+1] { + case '[': + // CSI sequence: \x1b[...final_char + // Parameters are in range 0x30-0x3F (0-?), intermediate chars 0x20-0x2F (space-/) + // Final characters are in range 0x40-0x7E (@-~) + i += 2 // Skip ESC and [ + for i < len(s) { + if isFinalCSIChar(s[i]) { + i++ // Skip the final character + break + } else if isCSIParameterChar(s[i]) { + i++ // Skip parameter/intermediate character + } else { + // Invalid character in CSI sequence, stop processing this escape + break + } + } + case ']': + // OSC sequence: \x1b]...terminator + // Terminators: \x07 (BEL) or \x1b\\ (ST) + i += 2 // Skip ESC and ] + for i < len(s) { + if s[i] == '\x07' { + i++ // Skip BEL + break + } else if s[i] == '\x1b' && i+1 < len(s) && s[i+1] == '\\' { + i += 2 // Skip ESC and \ + break + } + i++ + } + case '(': + // G0 character set selection: \x1b(char + i += 2 // Skip ESC and ( + if i < len(s) { + i++ // Skip the character + } + case ')': + // G1 character set selection: \x1b)char + i += 2 // Skip ESC and ) + if i < len(s) { + i++ // Skip the character + } + case '=': + // Application keypad mode: \x1b= + i += 2 + case '>': + // Normal keypad mode: \x1b> + i += 2 + case 'c': + // Reset: \x1bc + i += 2 + default: + // Other escape sequences (2-character) + // Handle common ones like \x1b7, \x1b8, \x1bD, \x1bE, \x1bH, \x1bM + if i+1 < len(s) && (s[i+1] >= '0' && s[i+1] <= '~') { + i += 2 + } else { + // Invalid or incomplete escape sequence, just skip ESC + i++ + } + } + } else { + // Regular character, keep it + result.WriteByte(s[i]) + i++ + } + } + + return result.String() +} + +// isFinalCSIChar checks if a character is a valid CSI final character +// Final characters are in range 0x40-0x7E (@-~) +func isFinalCSIChar(b byte) bool { + return b >= 0x40 && b <= 0x7E +} + +// isCSIParameterChar checks if a character is a valid CSI parameter or intermediate character +// Parameter characters are in range 0x30-0x3F (0-?) +// Intermediate characters are in range 0x20-0x2F (space-/) +func isCSIParameterChar(b byte) bool { + return (b >= 0x20 && b <= 0x2F) || (b >= 0x30 && b <= 0x3F) +} diff --git a/pkg/stringutil/stringutil.go b/pkg/stringutil/stringutil.go index 203232e90f5..325c49e1d0e 100644 --- a/pkg/stringutil/stringutil.go +++ b/pkg/stringutil/stringutil.go @@ -3,7 +3,6 @@ package stringutil import ( "fmt" - "regexp" "strconv" "strings" ) @@ -83,11 +82,6 @@ func IsPositiveInteger(s string) bool { return err == nil && num > 0 } -// ansiEscapePattern matches ANSI escape sequences -// Pattern matches: ESC [ -// Examples: \x1b[0m, \x1b[31m, \x1b[1;32m -var ansiEscapePattern = regexp.MustCompile(`\x1b\[[0-9;]*[a-zA-Z]`) - // StripANSIEscapeCodes removes ANSI escape sequences from a string. // This prevents terminal color codes and other control sequences from // being accidentally included in generated files (e.g., YAML workflows). @@ -106,6 +100,8 @@ var ansiEscapePattern = regexp.MustCompile(`\x1b\[[0-9;]*[a-zA-Z]`) // - Workflow descriptions copied from terminal output // - Comments in generated YAML files // - Any text that should be plain ASCII +// +// Deprecated: Use StripANSI instead, which handles a broader range of terminal sequences. func StripANSIEscapeCodes(s string) string { - return ansiEscapePattern.ReplaceAllString(s, "") + return StripANSI(s) } diff --git a/pkg/workflow/compiler_activation_jobs.go b/pkg/workflow/compiler_activation_jobs.go index dbf41b025a8..0b8844e9b22 100644 --- a/pkg/workflow/compiler_activation_jobs.go +++ b/pkg/workflow/compiler_activation_jobs.go @@ -112,7 +112,7 @@ func (c *Compiler) buildPreActivationJob(data *WorkflowData, needsPermissionChec steps = append(steps, fmt.Sprintf(" uses: %s\n", GetActionPin("actions/github-script"))) steps = append(steps, " env:\n") // Strip ANSI escape codes from stop-time value - cleanStopTime := stringutil.StripANSIEscapeCodes(data.StopTime) + cleanStopTime := stringutil.StripANSI(data.StopTime) steps = append(steps, fmt.Sprintf(" GH_AW_STOP_TIME: %s\n", cleanStopTime)) steps = append(steps, fmt.Sprintf(" GH_AW_WORKFLOW_NAME: %q\n", workflowName)) steps = append(steps, " with:\n") @@ -701,7 +701,7 @@ func (c *Compiler) buildActivationJob(data *WorkflowData, preActivationJobCreate var environment string if data.ManualApproval != "" { // Strip ANSI escape codes from manual-approval environment name - cleanManualApproval := stringutil.StripANSIEscapeCodes(data.ManualApproval) + cleanManualApproval := stringutil.StripANSI(data.ManualApproval) environment = fmt.Sprintf("environment: %s", cleanManualApproval) } diff --git a/pkg/workflow/compiler_yaml.go b/pkg/workflow/compiler_yaml.go index 80b8212f90f..19ce4fffc35 100644 --- a/pkg/workflow/compiler_yaml.go +++ b/pkg/workflow/compiler_yaml.go @@ -64,7 +64,7 @@ func (c *Compiler) generateWorkflowHeader(yaml *strings.Builder, data *WorkflowD // Add description comment if provided if data.Description != "" { - cleanDescription := stringutil.StripANSIEscapeCodes(data.Description) + cleanDescription := stringutil.StripANSI(data.Description) // Split description into lines and prefix each with "# " descriptionLines := strings.Split(strings.TrimSpace(cleanDescription), "\n") for _, line := range descriptionLines { @@ -75,7 +75,7 @@ func (c *Compiler) generateWorkflowHeader(yaml *strings.Builder, data *WorkflowD // Add source comment if provided if data.Source != "" { yaml.WriteString("#\n") - cleanSource := stringutil.StripANSIEscapeCodes(data.Source) + cleanSource := stringutil.StripANSI(data.Source) // Normalize to Unix paths (forward slashes) for cross-platform compatibility cleanSource = filepath.ToSlash(cleanSource) fmt.Fprintf(yaml, "# Source: %s\n", cleanSource) @@ -89,7 +89,7 @@ func (c *Compiler) generateWorkflowHeader(yaml *strings.Builder, data *WorkflowD if len(data.ImportedFiles) > 0 { yaml.WriteString("# Imports:\n") for _, file := range data.ImportedFiles { - cleanFile := stringutil.StripANSIEscapeCodes(file) + cleanFile := stringutil.StripANSI(file) // Normalize to Unix paths (forward slashes) for cross-platform compatibility cleanFile = filepath.ToSlash(cleanFile) fmt.Fprintf(yaml, "# - %s\n", cleanFile) @@ -99,7 +99,7 @@ func (c *Compiler) generateWorkflowHeader(yaml *strings.Builder, data *WorkflowD if len(data.IncludedFiles) > 0 { yaml.WriteString("# Includes:\n") for _, file := range data.IncludedFiles { - cleanFile := stringutil.StripANSIEscapeCodes(file) + cleanFile := stringutil.StripANSI(file) // Normalize to Unix paths (forward slashes) for cross-platform compatibility cleanFile = filepath.ToSlash(cleanFile) fmt.Fprintf(yaml, "# - %s\n", cleanFile) @@ -124,14 +124,14 @@ func (c *Compiler) generateWorkflowHeader(yaml *strings.Builder, data *WorkflowD // Add stop-time comment if configured if data.StopTime != "" { yaml.WriteString("#\n") - cleanStopTime := stringutil.StripANSIEscapeCodes(data.StopTime) + cleanStopTime := stringutil.StripANSI(data.StopTime) fmt.Fprintf(yaml, "# Effective stop-time: %s\n", cleanStopTime) } // Add manual-approval comment if configured if data.ManualApproval != "" { yaml.WriteString("#\n") - cleanManualApproval := stringutil.StripANSIEscapeCodes(data.ManualApproval) + cleanManualApproval := stringutil.StripANSI(data.ManualApproval) fmt.Fprintf(yaml, "# Manual approval required: environment '%s'\n", cleanManualApproval) } diff --git a/pkg/workflow/dispatch_workflow_validation.go b/pkg/workflow/dispatch_workflow_validation.go index 46b9d1e70cf..387fef0db05 100644 --- a/pkg/workflow/dispatch_workflow_validation.go +++ b/pkg/workflow/dispatch_workflow_validation.go @@ -6,6 +6,7 @@ import ( "path/filepath" "strings" + "github.com/github/gh-aw/pkg/fileutil" "github.com/github/gh-aw/pkg/logger" "github.com/goccy/go-yaml" ) @@ -289,9 +290,9 @@ func findWorkflowFile(workflowName string, currentWorkflowPath string) (*findWor result.mdPath = mdPath result.lockPath = lockPath result.ymlPath = ymlPath - result.mdExists = fileExists(mdPath) - result.lockExists = fileExists(lockPath) - result.ymlExists = fileExists(ymlPath) + result.mdExists = fileutil.FileExists(mdPath) + result.lockExists = fileutil.FileExists(lockPath) + result.ymlExists = fileutil.FileExists(ymlPath) return result, nil } diff --git a/pkg/workflow/yaml_generation.go b/pkg/workflow/git_configuration_steps.go similarity index 100% rename from pkg/workflow/yaml_generation.go rename to pkg/workflow/git_configuration_steps.go diff --git a/pkg/workflow/prompt_step_helper.go b/pkg/workflow/prompt_step_helper.go deleted file mode 100644 index b4e35a7adce..00000000000 --- a/pkg/workflow/prompt_step_helper.go +++ /dev/null @@ -1,120 +0,0 @@ -// This file provides helper functions for generating prompt workflow steps. -// -// This file contains utilities for building GitHub Actions workflow steps that -// append prompt text to prompt files used by AI engines. These helpers extract -// common patterns used across multiple prompt generators (XPIA, temp folder, -// playwright, edit tool, etc.) to reduce code duplication and ensure security. -// -// # Organization Rationale -// -// These prompt step helpers are grouped here because they: -// - Provide common patterns for prompt text generation used by 5+ generators -// - Handle GitHub Actions expression extraction for security -// - Ensure consistent prompt step formatting across engines -// - Centralize template injection prevention logic -// -// This follows the helper file conventions documented in the developer instructions. -// See skills/developer/SKILL.md#helper-file-conventions for details. -// -// # Key Functions -// -// Static Prompt Generation: -// - generateStaticPromptStep() - Generate steps for static prompt text -// - generateStaticPromptStepWithExpressions() - Generate steps with secure expression handling -// -// # Usage Patterns -// -// These helpers are used when generating workflow steps that append text to -// prompt files. They follow two patterns: -// -// 1. **Static Text** (no GitHub Actions expressions): -// ```go -// generateStaticPromptStep(yaml, -// "Append XPIA security instructions to prompt", -// xpiaPromptText, -// data.SafetyPrompt) -// ``` -// -// 2. **Text with Expressions** (contains ${{ ... }}): -// ```go -// generateStaticPromptStepWithExpressions(yaml, -// "Append dynamic context to prompt", -// promptWithExpressions, -// shouldInclude) -// ``` -// -// The expression-aware helper extracts GitHub Actions expressions into -// environment variables to prevent template injection vulnerabilities. -// -// # Security Considerations -// -// Always use generateStaticPromptStepWithExpressions() when prompt text -// contains GitHub Actions expressions (${{ ... }}). This ensures: -// - Expressions are evaluated in controlled env: context -// - No inline shell script interpolation (prevents injection) -// - Safe placeholder substitution via JavaScript -// -// See scratchpad/template-injection-prevention.md for security details. -// -// # When to Use vs Alternatives -// -// Use these helpers when: -// - Generating workflow steps that append text to prompt files -// - Working with static or expression-containing prompt text -// - Need consistent prompt step formatting across engines -// -// For other prompt-related functionality, see: -// - *_engine.go files for engine-specific prompt generation -// - engine_helpers.go for shared engine utilities - -package workflow - -import ( - "strings" - - "github.com/github/gh-aw/pkg/logger" -) - -var promptStepHelperLog = logger.New("workflow:prompt_step_helper") - -// generateStaticPromptStep is a helper function that generates a workflow step -// for appending static prompt text to the prompt file. It encapsulates the common -// pattern used across multiple prompt generators (XPIA, temp folder, playwright, edit tool, etc.) -// to reduce code duplication and ensure consistency. -// -// Parameters: -// - yaml: The string builder to write the YAML to -// - description: The name of the workflow step (e.g., "Append XPIA security instructions to prompt") -// - promptText: The static text content to append to the prompt (used for backward compatibility) -// - shouldInclude: Whether to generate the step (false means skip generation entirely) -// -// Example usage: -// -// generateStaticPromptStep(yaml, -// "Append XPIA security instructions to prompt", -// xpiaPromptText, -// data.SafetyPrompt) -// -// Deprecated: This function is kept for backward compatibility with inline prompts. -// Use generateStaticPromptStepFromFile for new code. -func generateStaticPromptStep(yaml *strings.Builder, description string, promptText string, shouldInclude bool) { - promptStepHelperLog.Printf("Generating static prompt step: description=%s, shouldInclude=%t", description, shouldInclude) - // Skip generation if guard condition is false - if !shouldInclude { - return - } - - // Use the existing appendPromptStep helper with a renderer that writes the prompt text - appendPromptStep(yaml, - description, - func(y *strings.Builder, indent string) { - WritePromptTextToYAML(y, promptText, indent) - }, - "", // no condition - " ") -} - -// TODO: generateStaticPromptStepFromFile and generateStaticPromptStepFromFileWithExpressions -// could be implemented in the future to generate workflow steps for appending prompt files. -// For now, we use the unified prompt step approach in unified_prompt_step.go. -// See commit history if this needs to be restored. diff --git a/pkg/workflow/safe_outputs_config_helpers.go b/pkg/workflow/safe_outputs_config_helpers.go index 44ae135cea2..9958fe3364a 100644 --- a/pkg/workflow/safe_outputs_config_helpers.go +++ b/pkg/workflow/safe_outputs_config_helpers.go @@ -2,14 +2,116 @@ package workflow import ( "fmt" + "reflect" + "sort" "github.com/github/gh-aw/pkg/constants" + "github.com/github/gh-aw/pkg/logger" ) // ======================================== // Safe Output Configuration Helpers // ======================================== +var safeOutputReflectionLog = logger.New("workflow:safe_outputs_config_helpers_reflection") + +// safeOutputFieldMapping maps struct field names to their tool names +var safeOutputFieldMapping = map[string]string{ + "CreateIssues": "create_issue", + "CreateAgentSessions": "create_agent_session", + "CreateDiscussions": "create_discussion", + "UpdateDiscussions": "update_discussion", + "CloseDiscussions": "close_discussion", + "CloseIssues": "close_issue", + "ClosePullRequests": "close_pull_request", + "AddComments": "add_comment", + "CreatePullRequests": "create_pull_request", + "CreatePullRequestReviewComments": "create_pull_request_review_comment", + "SubmitPullRequestReview": "submit_pull_request_review", + "ReplyToPullRequestReviewComment": "reply_to_pull_request_review_comment", + "ResolvePullRequestReviewThread": "resolve_pull_request_review_thread", + "CreateCodeScanningAlerts": "create_code_scanning_alert", + "AddLabels": "add_labels", + "RemoveLabels": "remove_labels", + "AddReviewer": "add_reviewer", + "AssignMilestone": "assign_milestone", + "AssignToAgent": "assign_to_agent", + "AssignToUser": "assign_to_user", + "UpdateIssues": "update_issue", + "UpdatePullRequests": "update_pull_request", + "PushToPullRequestBranch": "push_to_pull_request_branch", + "UploadAssets": "upload_asset", + "UpdateRelease": "update_release", + "UpdateProjects": "update_project", + "CreateProjects": "create_project", + "CreateProjectStatusUpdates": "create_project_status_update", + "LinkSubIssue": "link_sub_issue", + "HideComment": "hide_comment", + "DispatchWorkflow": "dispatch_workflow", + "MissingTool": "missing_tool", + "NoOp": "noop", + "MarkPullRequestAsReadyForReview": "mark_pull_request_as_ready_for_review", +} + +// hasAnySafeOutputEnabled uses reflection to check if any safe output field is non-nil +func hasAnySafeOutputEnabled(safeOutputs *SafeOutputsConfig) bool { + if safeOutputs == nil { + return false + } + + safeOutputReflectionLog.Print("Checking if any safe outputs are enabled using reflection") + + // Check Jobs separately as it's a map + if len(safeOutputs.Jobs) > 0 { + safeOutputReflectionLog.Printf("Found %d custom jobs enabled", len(safeOutputs.Jobs)) + return true + } + + // Use reflection to check all pointer fields + val := reflect.ValueOf(safeOutputs).Elem() + for fieldName := range safeOutputFieldMapping { + field := val.FieldByName(fieldName) + if field.IsValid() && !field.IsNil() { + safeOutputReflectionLog.Printf("Found enabled safe output field: %s", fieldName) + return true + } + } + + safeOutputReflectionLog.Print("No safe outputs enabled") + return false +} + +// getEnabledSafeOutputToolNamesReflection uses reflection to get enabled tool names +func getEnabledSafeOutputToolNamesReflection(safeOutputs *SafeOutputsConfig) []string { + if safeOutputs == nil { + return nil + } + + safeOutputReflectionLog.Print("Getting enabled safe output tool names using reflection") + var tools []string + + // Use reflection to check all pointer fields + val := reflect.ValueOf(safeOutputs).Elem() + for fieldName, toolName := range safeOutputFieldMapping { + field := val.FieldByName(fieldName) + if field.IsValid() && !field.IsNil() { + tools = append(tools, toolName) + } + } + + // Add custom job tools + for jobName := range safeOutputs.Jobs { + tools = append(tools, jobName) + safeOutputReflectionLog.Printf("Added custom job tool: %s", jobName) + } + + // Sort tools to ensure deterministic compilation + sort.Strings(tools) + + safeOutputReflectionLog.Printf("Found %d enabled safe output tools", len(tools)) + return tools +} + // formatSafeOutputsRunsOn formats the runs-on value from SafeOutputsConfig for job output func (c *Compiler) formatSafeOutputsRunsOn(safeOutputs *SafeOutputsConfig) string { if safeOutputs == nil || safeOutputs.RunsOn == "" { diff --git a/pkg/workflow/safe_outputs_config_helpers_reflection.go b/pkg/workflow/safe_outputs_config_helpers_reflection.go deleted file mode 100644 index 1fd17a477aa..00000000000 --- a/pkg/workflow/safe_outputs_config_helpers_reflection.go +++ /dev/null @@ -1,107 +0,0 @@ -package workflow - -import ( - "reflect" - "sort" - - "github.com/github/gh-aw/pkg/logger" -) - -var safeOutputReflectionLog = logger.New("workflow:safe_outputs_config_helpers_reflection") - -// safeOutputFieldMapping maps struct field names to their tool names -var safeOutputFieldMapping = map[string]string{ - "CreateIssues": "create_issue", - "CreateAgentSessions": "create_agent_session", - "CreateDiscussions": "create_discussion", - "UpdateDiscussions": "update_discussion", - "CloseDiscussions": "close_discussion", - "CloseIssues": "close_issue", - "ClosePullRequests": "close_pull_request", - "AddComments": "add_comment", - "CreatePullRequests": "create_pull_request", - "CreatePullRequestReviewComments": "create_pull_request_review_comment", - "SubmitPullRequestReview": "submit_pull_request_review", - "ReplyToPullRequestReviewComment": "reply_to_pull_request_review_comment", - "ResolvePullRequestReviewThread": "resolve_pull_request_review_thread", - "CreateCodeScanningAlerts": "create_code_scanning_alert", - "AddLabels": "add_labels", - "RemoveLabels": "remove_labels", - "AddReviewer": "add_reviewer", - "AssignMilestone": "assign_milestone", - "AssignToAgent": "assign_to_agent", - "AssignToUser": "assign_to_user", - "UpdateIssues": "update_issue", - "UpdatePullRequests": "update_pull_request", - "PushToPullRequestBranch": "push_to_pull_request_branch", - "UploadAssets": "upload_asset", - "UpdateRelease": "update_release", - "UpdateProjects": "update_project", - "CreateProjects": "create_project", - "CreateProjectStatusUpdates": "create_project_status_update", - "LinkSubIssue": "link_sub_issue", - "HideComment": "hide_comment", - "DispatchWorkflow": "dispatch_workflow", - "MissingTool": "missing_tool", - "NoOp": "noop", - "MarkPullRequestAsReadyForReview": "mark_pull_request_as_ready_for_review", -} - -// hasAnySafeOutputEnabled uses reflection to check if any safe output field is non-nil -func hasAnySafeOutputEnabled(safeOutputs *SafeOutputsConfig) bool { - if safeOutputs == nil { - return false - } - - safeOutputReflectionLog.Print("Checking if any safe outputs are enabled using reflection") - - // Check Jobs separately as it's a map - if len(safeOutputs.Jobs) > 0 { - safeOutputReflectionLog.Printf("Found %d custom jobs enabled", len(safeOutputs.Jobs)) - return true - } - - // Use reflection to check all pointer fields - val := reflect.ValueOf(safeOutputs).Elem() - for fieldName := range safeOutputFieldMapping { - field := val.FieldByName(fieldName) - if field.IsValid() && !field.IsNil() { - safeOutputReflectionLog.Printf("Found enabled safe output field: %s", fieldName) - return true - } - } - - safeOutputReflectionLog.Print("No safe outputs enabled") - return false -} - -// getEnabledSafeOutputToolNamesReflection uses reflection to get enabled tool names -func getEnabledSafeOutputToolNamesReflection(safeOutputs *SafeOutputsConfig) []string { - if safeOutputs == nil { - return nil - } - - safeOutputReflectionLog.Print("Getting enabled safe output tool names using reflection") - var tools []string - - // Use reflection to check all pointer fields - val := reflect.ValueOf(safeOutputs).Elem() - for fieldName, toolName := range safeOutputFieldMapping { - field := val.FieldByName(fieldName) - if field.IsValid() && !field.IsNil() { - tools = append(tools, toolName) - } - } - - // Add custom job tools - for jobName := range safeOutputs.Jobs { - tools = append(tools, jobName) - safeOutputReflectionLog.Printf("Added custom job tool: %s", jobName) - } - - // Sort tools to ensure deterministic compilation - sort.Strings(tools) - - safeOutputReflectionLog.Printf("Found %d enabled safe output tools", len(tools)) - return tools -} diff --git a/pkg/workflow/time_delta.go b/pkg/workflow/time_delta.go index 87e8ec76d78..8bfa5e06d2f 100644 --- a/pkg/workflow/time_delta.go +++ b/pkg/workflow/time_delta.go @@ -465,3 +465,34 @@ func parseRelativeTimeSpec(spec string) int { return 0 } } + +// Time delta validation limits +// +// Policy: Maximum stop-after time is 1 year to prevent scheduling too far in the future. +// These constants define the maximum allowed values for each time unit when parsing +// time deltas in workflow schedules. The limits ensure workflows don't schedule actions +// unreasonably far into the future, which could indicate configuration errors or create +// operational challenges. +// +// All limits are equivalent to approximately 1 year: +// - 12 months = 1 year (exact) +// - 52 weeks = 364 days ≈ 1 year +// - 365 days = 1 year (non-leap year) +// - 8760 hours = 365 days * 24 hours +// - 525600 minutes = 365 days * 24 hours * 60 minutes +const ( + // MaxTimeDeltaMonths is the maximum allowed months in a time delta (1 year) + MaxTimeDeltaMonths = 12 + + // MaxTimeDeltaWeeks is the maximum allowed weeks in a time delta (approximately 1 year) + MaxTimeDeltaWeeks = 52 + + // MaxTimeDeltaDays is the maximum allowed days in a time delta (1 year, non-leap) + MaxTimeDeltaDays = 365 + + // MaxTimeDeltaHours is the maximum allowed hours in a time delta (365 days * 24 hours) + MaxTimeDeltaHours = 8760 + + // MaxTimeDeltaMinutes is the maximum allowed minutes in a time delta (365 days * 24 hours * 60 minutes) + MaxTimeDeltaMinutes = 525600 +) diff --git a/pkg/workflow/time_delta_constants.go b/pkg/workflow/time_delta_constants.go deleted file mode 100644 index 1f245086e9a..00000000000 --- a/pkg/workflow/time_delta_constants.go +++ /dev/null @@ -1,32 +0,0 @@ -package workflow - -// Time delta validation limits -// -// Policy: Maximum stop-after time is 1 year to prevent scheduling too far in the future. -// These constants define the maximum allowed values for each time unit when parsing -// time deltas in workflow schedules. The limits ensure workflows don't schedule actions -// unreasonably far into the future, which could indicate configuration errors or create -// operational challenges. -// -// All limits are equivalent to approximately 1 year: -// - 12 months = 1 year (exact) -// - 52 weeks = 364 days ≈ 1 year -// - 365 days = 1 year (non-leap year) -// - 8760 hours = 365 days * 24 hours -// - 525600 minutes = 365 days * 24 hours * 60 minutes -const ( - // MaxTimeDeltaMonths is the maximum allowed months in a time delta (1 year) - MaxTimeDeltaMonths = 12 - - // MaxTimeDeltaWeeks is the maximum allowed weeks in a time delta (approximately 1 year) - MaxTimeDeltaWeeks = 52 - - // MaxTimeDeltaDays is the maximum allowed days in a time delta (1 year, non-leap) - MaxTimeDeltaDays = 365 - - // MaxTimeDeltaHours is the maximum allowed hours in a time delta (365 days * 24 hours) - MaxTimeDeltaHours = 8760 - - // MaxTimeDeltaMinutes is the maximum allowed minutes in a time delta (365 days * 24 hours * 60 minutes) - MaxTimeDeltaMinutes = 525600 -) diff --git a/pkg/workflow/unified_prompt_step.go b/pkg/workflow/unified_prompt_step.go index 7997152e7a8..102e7ccaf08 100644 --- a/pkg/workflow/unified_prompt_step.go +++ b/pkg/workflow/unified_prompt_step.go @@ -638,3 +638,42 @@ func (c *Compiler) generateUnifiedPromptCreationStep(yaml *strings.Builder, buil // This allows the substitution to happen AFTER runtime-import processing return allExpressionMappings } + +var promptStepHelperLog = logger.New("workflow:prompt_step_helper") + +// generateStaticPromptStep is a helper function that generates a workflow step +// for appending static prompt text to the prompt file. It encapsulates the common +// pattern used across multiple prompt generators (XPIA, temp folder, playwright, edit tool, etc.) +// to reduce code duplication and ensure consistency. +// +// Parameters: +// - yaml: The string builder to write the YAML to +// - description: The name of the workflow step (e.g., "Append XPIA security instructions to prompt") +// - promptText: The static text content to append to the prompt (used for backward compatibility) +// - shouldInclude: Whether to generate the step (false means skip generation entirely) +// +// Example usage: +// +// generateStaticPromptStep(yaml, +// "Append XPIA security instructions to prompt", +// xpiaPromptText, +// data.SafetyPrompt) +// +// Deprecated: This function is kept for backward compatibility with inline prompts. +// Use generateStaticPromptStepFromFile for new code. +func generateStaticPromptStep(yaml *strings.Builder, description string, promptText string, shouldInclude bool) { + promptStepHelperLog.Printf("Generating static prompt step: description=%s, shouldInclude=%t", description, shouldInclude) + // Skip generation if guard condition is false + if !shouldInclude { + return + } + + // Use the existing appendPromptStep helper with a renderer that writes the prompt text + appendPromptStep(yaml, + description, + func(y *strings.Builder, indent string) { + WritePromptTextToYAML(y, promptText, indent) + }, + "", // no condition + " ") +} diff --git a/pkg/workflow/validation_helpers.go b/pkg/workflow/validation_helpers.go index 3c6a8e0d052..4c110c0bd2a 100644 --- a/pkg/workflow/validation_helpers.go +++ b/pkg/workflow/validation_helpers.go @@ -13,13 +13,11 @@ // - ValidateInList() - Validates that a value is in an allowed list // - ValidatePositiveInt() - Validates that a value is a positive integer // - ValidateNonNegativeInt() - Validates that a value is a non-negative integer -// - fileExists() - Checks if a file exists at the given path // - isEmptyOrNil() - Checks if a value is empty, nil, or zero (Phase 2) // - getMapFieldAsString() - Safely extracts a string field from a map[string]any (Phase 2) // - getMapFieldAsMap() - Safely extracts a nested map from a map[string]any (Phase 2) // - getMapFieldAsBool() - Safely extracts a boolean field from a map[string]any (Phase 2) // - getMapFieldAsInt() - Safely extracts an integer field from a map[string]any (Phase 2) -// - dirExists() - Checks if a directory exists at the given path (Phase 2) // // # Design Rationale // @@ -36,7 +34,6 @@ package workflow import ( "fmt" - "os" "strings" "github.com/github/gh-aw/pkg/logger" @@ -155,31 +152,6 @@ func ValidateNonNegativeInt(field string, value int) error { return nil } -// fileExists checks if a file exists at the given path. -// Returns true if the file exists and is accessible, false otherwise. -// -// This helper consolidates common file existence checking patterns. -// -// Example: -// -// if !fileExists(agentPath) { -// return NewValidationError("agent.file", agentPath, "file does not exist", "...") -// } -func fileExists(path string) bool { - if path == "" { - validationHelpersLog.Print("File existence check failed: empty path") - return false - } - - info, err := os.Stat(path) - if err != nil { - validationHelpersLog.Printf("File existence check failed: path=%s, error=%v", path, err) - return false - } - - return !info.IsDir() -} - // isEmptyOrNil evaluates whether a value represents an empty or absent state. // This consolidates various emptiness checks across the codebase into a single // reusable function. The function handles multiple value types with appropriate @@ -397,35 +369,3 @@ func getMapFieldAsInt(source map[string]any, fieldKey string, fallback int) int return convertedInt } - -// dirExists verifies that a filesystem path exists and represents a directory. -// This consolidates directory existence checking patterns used throughout validation code. -// Returns false for empty paths, non-existent paths, or paths that reference files. -// -// Example usage: -// -// if !dirExists(workflowDirectory) { -// return NewValidationError("directory", workflowDirectory, "directory not found", "create the directory") -// } -func dirExists(targetPath string) bool { - // Reject empty paths immediately - if len(targetPath) == 0 { - validationHelpersLog.Print("Directory check received empty path") - return false - } - - // Stat the path to check existence and type - pathInfo, statError := os.Stat(targetPath) - if statError != nil { - validationHelpersLog.Printf("Directory check failed for %q: %v", targetPath, statError) - return false - } - - // Verify it's actually a directory - isDirectory := pathInfo.IsDir() - if !isDirectory { - validationHelpersLog.Printf("Path %q exists but is not a directory", targetPath) - } - - return isDirectory -} diff --git a/pkg/workflow/validation_helpers_test.go b/pkg/workflow/validation_helpers_test.go index 600ceadd118..c408704cece 100644 --- a/pkg/workflow/validation_helpers_test.go +++ b/pkg/workflow/validation_helpers_test.go @@ -6,6 +6,7 @@ import ( "strings" "testing" + "github.com/github/gh-aw/pkg/fileutil" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -715,33 +716,33 @@ func TestGetMapFieldAsInt(t *testing.T) { } } -// TestDirExists tests the dirExists helper function +// TestDirExists tests the fileutil.DirExists helper function func TestDirExists(t *testing.T) { t.Run("empty path returns false", func(t *testing.T) { - result := dirExists("") + result := fileutil.DirExists("") assert.False(t, result, "empty path should return false") }) t.Run("non-existent path returns false", func(t *testing.T) { - result := dirExists("/nonexistent/path/to/directory") + result := fileutil.DirExists("/nonexistent/path/to/directory") assert.False(t, result, "non-existent path should return false") }) t.Run("file path returns false", func(t *testing.T) { // validation_helpers.go should exist and be a file, not a directory - result := dirExists("validation_helpers.go") + result := fileutil.DirExists("validation_helpers.go") assert.False(t, result, "file path should return false") }) t.Run("directory path returns true", func(t *testing.T) { // Current directory should exist - result := dirExists(".") + result := fileutil.DirExists(".") assert.True(t, result, "current directory should return true") }) t.Run("parent directory returns true", func(t *testing.T) { // Parent directory should exist - result := dirExists("..") + result := fileutil.DirExists("..") assert.True(t, result, "parent directory should return true") }) }