From 62477dbdf2890716dcf078505f17db22e29d44d2 Mon Sep 17 00:00:00 2001 From: Don Syme Date: Sat, 28 Feb 2026 12:04:10 +0000 Subject: [PATCH 1/5] docs: update DEADCODE.md with console_wasm.go pitfall note --- DEADCODE.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/DEADCODE.md b/DEADCODE.md index 519afa9f526..07bd662d83f 100644 --- a/DEADCODE.md +++ b/DEADCODE.md @@ -36,6 +36,8 @@ make fmt - `compiler.ParseWorkflowString` - `compiler.CompileToYAML` +**`pkg/console/console_wasm.go`** — this file provides WASM-specific stub implementations of many `pkg/console` functions (gated with `//go:build js || wasm`). Before deleting any function from `pkg/console/`, `grep` for it in `console_wasm.go`. If the function is called there, either inline the logic in `console_wasm.go` or delete the call. Batch 10 mistake: deleted `renderTreeSimple` from `render.go` but `console_wasm.go`'s `RenderTree` still called it, breaking the WASM build. Fix: replaced the `RenderTree` body in `console_wasm.go` with an inlined closure that no longer calls the deleted helper. + **`compiler_test_helpers.go`** — shows 3 dead functions but serves as shared test infrastructure for ≥15 test files. Do not delete. **Constant/embed rescue** — Some otherwise-dead files contain live constants or `//go:embed` directives. Extract them before deleting the file. @@ -163,7 +165,9 @@ For each batch: - [ ] Delete the function - [ ] Delete test functions that exclusively call the deleted function (not shared helpers) - [ ] Check for now-unused imports in edited files +- [ ] If editing `pkg/console/`, check `pkg/console/console_wasm.go` for calls to the deleted functions - [ ] `go build ./...` +- [ ] `GOARCH=wasm GOOS=js go build ./pkg/console/...` (if `pkg/console/` was touched) - [ ] `go vet ./...` - [ ] `go vet -tags=integration ./...` - [ ] `make fmt` From fed370034394e72ee58a17839edcf41062874c73 Mon Sep 17 00:00:00 2001 From: Don Syme Date: Sat, 28 Feb 2026 12:28:45 +0000 Subject: [PATCH 2/5] dead12: remove dead utility functions from 7 packages Deleted unreachable functions from utility packages: - pkg/sliceutil: ContainsAny, ContainsIgnoreCase (+ all tests) - pkg/logger: NewSlogLogger, Discard (+ tests for NewSlogLogger) - pkg/fileutil: CalculateDirectorySize (+ test) - pkg/repoutil: ParseGitHubURL (+ all tests) - pkg/stringutil/identifiers: IsAgenticWorkflow, IsLockFile (+ tests) - pkg/stringutil/pat_validation: IsFineGrainedPAT, IsClassicPAT, IsOAuthToken (+ tests) - pkg/stringutil/stringutil: StripANSIEscapeCodes (+ tests) Also replaces sliceutil.ContainsAny calls in docker_images_test.go with inline strings.Contains expressions. 986 lines removed across 15 files. --- pkg/cli/audit_report_helpers_test.go | 71 -------- pkg/cli/docker_images_test.go | 13 +- pkg/fileutil/fileutil.go | 19 --- pkg/logger/slog_adapter.go | 14 -- pkg/logger/slog_adapter_test.go | 85 ---------- pkg/repoutil/repoutil.go | 29 ---- pkg/repoutil/repoutil_test.go | 163 ------------------ pkg/sliceutil/sliceutil.go | 16 -- pkg/sliceutil/sliceutil_test.go | 232 -------------------------- pkg/stringutil/identifiers.go | 26 --- pkg/stringutil/identifiers_test.go | 107 ------------ pkg/stringutil/pat_validation.go | 15 -- pkg/stringutil/pat_validation_test.go | 21 --- pkg/stringutil/stringutil.go | 24 --- pkg/stringutil/stringutil_test.go | 157 ----------------- 15 files changed, 6 insertions(+), 986 deletions(-) diff --git a/pkg/cli/audit_report_helpers_test.go b/pkg/cli/audit_report_helpers_test.go index d837c9771b8..b0e222e97e1 100644 --- a/pkg/cli/audit_report_helpers_test.go +++ b/pkg/cli/audit_report_helpers_test.go @@ -10,81 +10,10 @@ import ( "testing" "time" - "github.com/github/gh-aw/pkg/fileutil" "github.com/github/gh-aw/pkg/stringutil" "github.com/github/gh-aw/pkg/testutil" ) -func TestCalculateDirectorySize(t *testing.T) { - tests := []struct { - name string - setup func(t *testing.T) string - expected int64 - }{ - { - name: "empty directory", - setup: func(t *testing.T) string { - dir := testutil.TempDir(t, "test-*") - return dir - }, - expected: 0, - }, - { - name: "single file", - setup: func(t *testing.T) string { - dir := testutil.TempDir(t, "test-*") - err := os.WriteFile(filepath.Join(dir, "test.txt"), []byte("hello"), 0644) - if err != nil { - t.Fatal(err) - } - return dir - }, - expected: 5, - }, - { - name: "multiple files in nested directories", - setup: func(t *testing.T) string { - dir := testutil.TempDir(t, "test-*") - // File 1: 10 bytes - err := os.WriteFile(filepath.Join(dir, "file1.txt"), []byte("0123456789"), 0644) - if err != nil { - t.Fatal(err) - } - // Create subdirectory - subdir := filepath.Join(dir, "subdir") - err = os.Mkdir(subdir, 0755) - if err != nil { - t.Fatal(err) - } - // File 2: 5 bytes - err = os.WriteFile(filepath.Join(subdir, "file2.txt"), []byte("hello"), 0644) - if err != nil { - t.Fatal(err) - } - return dir - }, - expected: 15, - }, - { - name: "nonexistent directory", - setup: func(t *testing.T) string { - return "/nonexistent/path/that/does/not/exist" - }, - expected: 0, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - dir := tt.setup(t) - got := fileutil.CalculateDirectorySize(dir) - if got != tt.expected { - t.Errorf("fileutil.CalculateDirectorySize() = %d, want %d", got, tt.expected) - } - }) - } -} - func TestParseDurationString(t *testing.T) { tests := []struct { name string diff --git a/pkg/cli/docker_images_test.go b/pkg/cli/docker_images_test.go index 1513aa6b23a..b0e7470ba7f 100644 --- a/pkg/cli/docker_images_test.go +++ b/pkg/cli/docker_images_test.go @@ -4,10 +4,9 @@ package cli import ( "context" + "strings" "testing" "time" - - "github.com/github/gh-aw/pkg/sliceutil" ) func TestCheckAndPrepareDockerImages_NoToolsRequested(t *testing.T) { @@ -39,7 +38,7 @@ func TestCheckAndPrepareDockerImages_ImageAlreadyDownloading(t *testing.T) { // Error message should mention downloading and retry if err != nil { errMsg := err.Error() - if !sliceutil.ContainsAny(errMsg, "downloading", "retry") { + if !strings.Contains(errMsg, "downloading") && !strings.Contains(errMsg, "retry") { t.Errorf("Expected error to mention downloading and retry, got: %s", errMsg) } } @@ -110,7 +109,7 @@ func TestDockerImageConstants(t *testing.T) { } for name, image := range expectedImages { - if !sliceutil.ContainsAny(image, "/", ":") { + if !strings.Contains(image, "/") && !strings.Contains(image, ":") { t.Errorf("%s image %s does not look like a Docker image reference", name, image) } } @@ -138,7 +137,7 @@ func TestCheckAndPrepareDockerImages_MultipleImages(t *testing.T) { // Error should mention downloading images if err != nil { errMsg := err.Error() - if !sliceutil.ContainsAny(errMsg, "downloading", "retry") { + if !strings.Contains(errMsg, "downloading") && !strings.Contains(errMsg, "retry") { t.Errorf("Expected error to mention downloading and retry, got: %s", errMsg) } } @@ -172,7 +171,7 @@ func TestCheckAndPrepareDockerImages_RetryMessageFormat(t *testing.T) { } for _, expected := range expectations { - if !sliceutil.ContainsAny(errMsg, expected) { + if !strings.Contains(errMsg, expected) { t.Errorf("Expected error message to contain '%s', got: %s", expected, errMsg) } } @@ -199,7 +198,7 @@ func TestCheckAndPrepareDockerImages_StartedDownloadingMessage(t *testing.T) { errMsg := err.Error() // Should contain zizmor since it's downloading - if !sliceutil.ContainsAny(errMsg, "zizmor") { + if !strings.Contains(errMsg, "zizmor") { t.Errorf("Expected error message to mention zizmor, got: %s", errMsg) } diff --git a/pkg/fileutil/fileutil.go b/pkg/fileutil/fileutil.go index 8b0eba2ce77..505a796972c 100644 --- a/pkg/fileutil/fileutil.go +++ b/pkg/fileutil/fileutil.go @@ -101,22 +101,3 @@ func CopyFile(src, dst string) error { log.Printf("File copied successfully: src=%s, dst=%s", src, dst) return out.Sync() } - -// CalculateDirectorySize recursively calculates the total size of files in a directory. -func CalculateDirectorySize(dirPath string) int64 { - log.Printf("Calculating directory size: %s", dirPath) - 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 - }) - - log.Printf("Directory size: path=%s, size=%d bytes", dirPath, totalSize) - return totalSize -} diff --git a/pkg/logger/slog_adapter.go b/pkg/logger/slog_adapter.go index dc240e2c32c..0727cdae108 100644 --- a/pkg/logger/slog_adapter.go +++ b/pkg/logger/slog_adapter.go @@ -3,7 +3,6 @@ package logger import ( "context" "fmt" - "io" "log/slog" "strings" ) @@ -91,21 +90,8 @@ func formatSlogValue(v any) string { return slog.AnyValue(v).String() } -// NewSlogLogger creates a new slog.Logger that uses gh-aw's logger package -// This allows integration with libraries that expect slog.Logger -func NewSlogLogger(namespace string) *slog.Logger { - logger := New(namespace) - handler := NewSlogHandler(logger) - return slog.New(handler) -} - // NewSlogLoggerWithHandler creates a new slog.Logger using an existing Logger instance func NewSlogLoggerWithHandler(logger *Logger) *slog.Logger { handler := NewSlogHandler(logger) return slog.New(handler) } - -// Discard returns a slog.Logger that discards all output -func Discard() *slog.Logger { - return slog.New(slog.NewTextHandler(io.Discard, nil)) -} diff --git a/pkg/logger/slog_adapter_test.go b/pkg/logger/slog_adapter_test.go index f4cf61e5a46..59e35aae6f5 100644 --- a/pkg/logger/slog_adapter_test.go +++ b/pkg/logger/slog_adapter_test.go @@ -10,91 +10,6 @@ import ( "testing" ) -func TestSlogAdapter(t *testing.T) { - // Only run if DEBUG is enabled - if os.Getenv("DEBUG") == "" { - t.Skip("Skipping test: DEBUG environment variable not set") - } - - // Capture stderr output - oldStderr := os.Stderr - r, w, _ := os.Pipe() - os.Stderr = w - - // Create slog logger using our adapter - slogLogger := NewSlogLogger("test:slog") - - // Test different log levels - slogLogger.Info("info message", "key", "value") - slogLogger.Debug("debug message", "count", 42) - slogLogger.Warn("warning message") - slogLogger.Error("error message", "error", "something went wrong") - - // Close write end and read output - w.Close() - var buf bytes.Buffer - io.Copy(&buf, r) - output := buf.String() - - // Restore stderr - os.Stderr = oldStderr - - // Verify output contains expected messages - if !strings.Contains(output, "[INFO] info message") { - t.Errorf("Expected info message in output, got: %s", output) - } - if !strings.Contains(output, "[DEBUG] debug message") { - t.Errorf("Expected debug message in output, got: %s", output) - } - if !strings.Contains(output, "[WARN] warning message") { - t.Errorf("Expected warn message in output, got: %s", output) - } - if !strings.Contains(output, "[ERROR] error message") { - t.Errorf("Expected error message in output, got: %s", output) - } - - // Verify attributes are included - if !strings.Contains(output, "key=value") { - t.Errorf("Expected 'key=value' in output, got: %s", output) - } - if !strings.Contains(output, "count=42") { - t.Errorf("Expected 'count=42' in output, got: %s", output) - } -} - -func TestSlogAdapterDisabled(t *testing.T) { - // Only run if DEBUG is not set - if os.Getenv("DEBUG") != "" { - t.Skip("Skipping test: DEBUG environment variable is set") - } - - // Capture stderr output - oldStderr := os.Stderr - r, w, _ := os.Pipe() - os.Stderr = w - - // Create slog logger using our adapter - slogLogger := NewSlogLogger("test:slog") - - // Test logging (should be disabled) - slogLogger.Info("info message", "key", "value") - slogLogger.Debug("debug message") - - // Close write end and read output - w.Close() - var buf bytes.Buffer - io.Copy(&buf, r) - output := buf.String() - - // Restore stderr - os.Stderr = oldStderr - - // Verify no output - if output != "" { - t.Errorf("Expected no output when logger is disabled, got: %s", output) - } -} - func TestNewSlogLoggerWithHandler(t *testing.T) { // Only run if DEBUG is enabled if os.Getenv("DEBUG") == "" { diff --git a/pkg/repoutil/repoutil.go b/pkg/repoutil/repoutil.go index 8d793e9dfc9..304743f6268 100644 --- a/pkg/repoutil/repoutil.go +++ b/pkg/repoutil/repoutil.go @@ -23,35 +23,6 @@ func SplitRepoSlug(slug string) (owner, repo string, err error) { return parts[0], parts[1], nil } -// ParseGitHubURL extracts the owner and repo from a GitHub URL. -// Handles both SSH (git@github.com:owner/repo.git) and HTTPS (https://github.com/owner/repo.git) formats. -func ParseGitHubURL(url string) (owner, repo string, err error) { - log.Printf("Parsing GitHub URL: %s", url) - var repoPath string - - // SSH format: git@github.com:owner/repo.git - if after, ok := strings.CutPrefix(url, "git@github.com:"); ok { - repoPath = after - log.Printf("Detected SSH format, extracted path: %s", repoPath) - } else if strings.Contains(url, "github.com/") { - // HTTPS format: https://github.com/owner/repo.git - parts := strings.Split(url, "github.com/") - if len(parts) >= 2 { - repoPath = parts[1] - log.Printf("Detected HTTPS format, extracted path: %s", repoPath) - } - } else { - log.Printf("URL does not match known GitHub formats: %s", url) - return "", "", fmt.Errorf("URL does not appear to be a GitHub repository: %s", url) - } - - // Remove .git suffix if present - repoPath = strings.TrimSuffix(repoPath, ".git") - - // Split into owner/repo - return SplitRepoSlug(repoPath) -} - // SanitizeForFilename converts a repository slug (owner/repo) to a filename-safe string. // Replaces "/" with "-". Returns "clone-mode" if the slug is empty. func SanitizeForFilename(slug string) string { diff --git a/pkg/repoutil/repoutil_test.go b/pkg/repoutil/repoutil_test.go index 03619848c2e..97b949d1f17 100644 --- a/pkg/repoutil/repoutil_test.go +++ b/pkg/repoutil/repoutil_test.go @@ -70,81 +70,6 @@ func TestSplitRepoSlug(t *testing.T) { } } -func TestParseGitHubURL(t *testing.T) { - tests := []struct { - name string - url string - expectedOwner string - expectedRepo string - expectError bool - }{ - { - name: "SSH format with .git", - url: "git@github.com:github/gh-aw.git", - expectedOwner: "github", - expectedRepo: "gh-aw", - expectError: false, - }, - { - name: "SSH format without .git", - url: "git@github.com:octocat/hello-world", - expectedOwner: "octocat", - expectedRepo: "hello-world", - expectError: false, - }, - { - name: "HTTPS format with .git", - url: "https://github.com/github/gh-aw.git", - expectedOwner: "github", - expectedRepo: "gh-aw", - expectError: false, - }, - { - name: "HTTPS format without .git", - url: "https://github.com/octocat/hello-world", - expectedOwner: "octocat", - expectedRepo: "hello-world", - expectError: false, - }, - { - name: "non-GitHub URL", - url: "https://gitlab.com/user/repo.git", - expectError: true, - }, - { - name: "invalid URL", - url: "not-a-url", - expectError: true, - }, - { - name: "empty URL", - url: "", - expectError: true, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - owner, repo, err := ParseGitHubURL(tt.url) - if tt.expectError { - if err == nil { - t.Errorf("ParseGitHubURL(%q) expected error, got nil", tt.url) - } - } else { - if err != nil { - t.Errorf("ParseGitHubURL(%q) unexpected error: %v", tt.url, err) - } - if owner != tt.expectedOwner { - t.Errorf("ParseGitHubURL(%q) owner = %q; want %q", tt.url, owner, tt.expectedOwner) - } - if repo != tt.expectedRepo { - t.Errorf("ParseGitHubURL(%q) repo = %q; want %q", tt.url, repo, tt.expectedRepo) - } - } - }) - } -} - func TestSanitizeForFilename(t *testing.T) { tests := []struct { name string @@ -190,13 +115,6 @@ func BenchmarkSplitRepoSlug(b *testing.B) { } } -func BenchmarkParseGitHubURL(b *testing.B) { - url := "https://github.com/github/gh-aw.git" - for b.Loop() { - _, _, _ = ParseGitHubURL(url) - } -} - func BenchmarkSanitizeForFilename(b *testing.B) { slug := "github/gh-aw" for b.Loop() { @@ -312,73 +230,6 @@ func TestSplitRepoSlug_SpecialCharacters(t *testing.T) { } } -func TestParseGitHubURL_Variants(t *testing.T) { - tests := []struct { - name string - url string - expectedOwner string - expectedRepo string - expectError bool - }{ - { - name: "SSH with port (invalid format)", - url: "git@github.com:22:owner/repo.git", - expectedOwner: "", - expectedRepo: "", - expectError: false, // Will parse but give unexpected results - }, - { - name: "HTTPS with www", - url: "https://www.github.com/owner/repo.git", - expectedOwner: "owner", - expectedRepo: "repo", - expectError: false, - }, - { - name: "HTTP instead of HTTPS", - url: "http://github.com/owner/repo.git", - expectedOwner: "owner", - expectedRepo: "repo", - expectError: false, - }, - { - name: "URL with trailing slash (will fail)", - url: "https://github.com/owner/repo/", - expectedOwner: "", - expectedRepo: "", - expectError: true, // Will fail due to extra slash - }, - { - name: "SSH without git extension", - url: "git@github.com:owner/repo", - expectedOwner: "owner", - expectedRepo: "repo", - expectError: false, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - owner, repo, err := ParseGitHubURL(tt.url) - if tt.expectError { - if err == nil { - t.Errorf("Expected error for URL %q", tt.url) - } - } else { - if err != nil && tt.expectedOwner != "" { - t.Errorf("Unexpected error for URL %q: %v", tt.url, err) - } - if err == nil && tt.expectedOwner != "" { - if owner != tt.expectedOwner || repo != tt.expectedRepo { - t.Errorf("ParseGitHubURL(%q) = (%q, %q); want (%q, %q)", - tt.url, owner, repo, tt.expectedOwner, tt.expectedRepo) - } - } - } - }) - } -} - func TestSanitizeForFilename_SpecialCases(t *testing.T) { tests := []struct { name string @@ -458,17 +309,3 @@ func BenchmarkSplitRepoSlug_Invalid(b *testing.B) { _, _, _ = SplitRepoSlug(slug) } } - -func BenchmarkParseGitHubURL_SSH(b *testing.B) { - url := "git@github.com:github/gh-aw.git" - for b.Loop() { - _, _, _ = ParseGitHubURL(url) - } -} - -func BenchmarkParseGitHubURL_HTTPS(b *testing.B) { - url := "https://github.com/github/gh-aw.git" - for b.Loop() { - _, _, _ = ParseGitHubURL(url) - } -} diff --git a/pkg/sliceutil/sliceutil.go b/pkg/sliceutil/sliceutil.go index 265e265a076..ea6166e23db 100644 --- a/pkg/sliceutil/sliceutil.go +++ b/pkg/sliceutil/sliceutil.go @@ -3,7 +3,6 @@ package sliceutil import ( "slices" - "strings" ) // Contains checks if a string slice contains a specific string. @@ -11,21 +10,6 @@ func Contains(slice []string, item string) bool { return slices.Contains(slice, item) } -// ContainsAny checks if a string contains any of the given substrings. -func ContainsAny(s string, substrings ...string) bool { - for _, sub := range substrings { - if strings.Contains(s, sub) { - return true - } - } - return false -} - -// ContainsIgnoreCase checks if a string contains a substring, ignoring case. -func ContainsIgnoreCase(s, substr string) bool { - return strings.Contains(strings.ToLower(s), strings.ToLower(substr)) -} - // Filter returns a new slice containing only elements that match the predicate. // This is a pure function that does not modify the input slice. func Filter[T any](slice []T, predicate func(T) bool) []T { diff --git a/pkg/sliceutil/sliceutil_test.go b/pkg/sliceutil/sliceutil_test.go index ce63b5e2c75..1b34c00cb1e 100644 --- a/pkg/sliceutil/sliceutil_test.go +++ b/pkg/sliceutil/sliceutil_test.go @@ -62,138 +62,6 @@ func TestContains(t *testing.T) { } } -func TestContainsAny(t *testing.T) { - tests := []struct { - name string - s string - substrings []string - expected bool - }{ - { - name: "contains first substring", - s: "hello world", - substrings: []string{"hello", "goodbye"}, - expected: true, - }, - { - name: "contains second substring", - s: "hello world", - substrings: []string{"goodbye", "world"}, - expected: true, - }, - { - name: "contains no substrings", - s: "hello world", - substrings: []string{"goodbye", "farewell"}, - expected: false, - }, - { - name: "empty substrings", - s: "hello world", - substrings: []string{}, - expected: false, - }, - { - name: "empty string", - s: "", - substrings: []string{"hello"}, - expected: false, - }, - { - name: "contains empty substring", - s: "hello world", - substrings: []string{""}, - expected: true, - }, - { - name: "multiple matches", - s: "Docker images are being downloaded", - substrings: []string{"downloading", "retry"}, - expected: false, - }, - { - name: "match found", - s: "downloading images", - substrings: []string{"downloading", "retry"}, - expected: true, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - result := ContainsAny(tt.s, tt.substrings...) - assert.Equal(t, tt.expected, result, - "ContainsAny should return correct value for string %q and substrings %v", tt.s, tt.substrings) - }) - } -} - -func TestContainsIgnoreCase(t *testing.T) { - tests := []struct { - name string - s string - substr string - expected bool - }{ - { - name: "exact match", - s: "Hello World", - substr: "Hello", - expected: true, - }, - { - name: "case insensitive match", - s: "Hello World", - substr: "hello", - expected: true, - }, - { - name: "case insensitive match uppercase", - s: "hello world", - substr: "WORLD", - expected: true, - }, - { - name: "no match", - s: "Hello World", - substr: "goodbye", - expected: false, - }, - { - name: "empty substring", - s: "Hello World", - substr: "", - expected: true, - }, - { - name: "empty string", - s: "", - substr: "hello", - expected: false, - }, - { - name: "both empty", - s: "", - substr: "", - expected: true, - }, - { - name: "mixed case substring in mixed case string", - s: "GitHub Actions Workflow", - substr: "actions", - expected: true, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - result := ContainsIgnoreCase(tt.s, tt.substr) - assert.Equal(t, tt.expected, result, - "ContainsIgnoreCase should return correct value for string %q and substring %q", tt.s, tt.substr) - }) - } -} - func BenchmarkContains(b *testing.B) { slice := []string{"apple", "banana", "cherry", "date", "elderberry"} for b.Loop() { @@ -201,22 +69,6 @@ func BenchmarkContains(b *testing.B) { } } -func BenchmarkContainsAny(b *testing.B) { - s := "hello world from the testing framework" - substrings := []string{"goodbye", "world", "farewell"} - for b.Loop() { - ContainsAny(s, substrings...) - } -} - -func BenchmarkContainsIgnoreCase(b *testing.B) { - s := "Hello World From The Testing Framework" - substr := "world" - for b.Loop() { - ContainsIgnoreCase(s, substr) - } -} - // Additional edge case tests for better coverage func TestContains_LargeSlice(t *testing.T) { @@ -243,77 +95,6 @@ func TestContains_SingleElement(t *testing.T) { assert.False(t, Contains(slice, "other"), "should not find different item in single-element slice") } -func TestContainsAny_MultipleMatches(t *testing.T) { - s := "The quick brown fox jumps over the lazy dog" - - // Multiple substrings that match - assert.True(t, ContainsAny(s, "quick", "lazy"), "should find at least one matching substring") - - // First one matches - assert.True(t, ContainsAny(s, "quick", "missing", "absent"), "should find first matching substring") - - // Last one matches - assert.True(t, ContainsAny(s, "missing", "absent", "dog"), "should find last matching substring") -} - -func TestContainsAny_NilSubstrings(t *testing.T) { - s := "test string" - - // Nil substrings should return false - assert.False(t, ContainsAny(s, nil...), "should return false for nil substrings") -} - -func TestContainsIgnoreCase_Unicode(t *testing.T) { - tests := []struct { - name string - s string - substr string - expected bool - }{ - { - name: "unicode characters", - s: "Café España", - substr: "café", - expected: true, - }, - { - name: "unicode uppercase", - s: "café españa", - substr: "CAFÉ", - expected: true, - }, - { - name: "emoji in string", - s: "Hello 👋 World", - substr: "👋", - expected: true, - }, - { - name: "special characters", - s: "test@example.com", - substr: "EXAMPLE", - expected: true, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - result := ContainsIgnoreCase(tt.s, tt.substr) - assert.Equal(t, tt.expected, result, - "ContainsIgnoreCase should return correct value for string %q and substring %q", tt.s, tt.substr) - }) - } -} - -func TestContainsIgnoreCase_PartialMatch(t *testing.T) { - s := "GitHub Actions Workflow" - - // Should find partial matches - assert.True(t, ContainsIgnoreCase(s, "hub"), "should find partial match 'hub' in 'GitHub'") - assert.True(t, ContainsIgnoreCase(s, "WORK"), "should find partial match 'WORK' in 'Workflow'") - assert.True(t, ContainsIgnoreCase(s, "actions workflow"), "should find multi-word partial match") -} - func TestContains_Duplicates(t *testing.T) { // Slice with duplicate values slice := []string{"apple", "banana", "apple", "cherry", "apple"} @@ -329,16 +110,3 @@ func TestContains_Duplicates(t *testing.T) { } assert.Equal(t, 3, count, "should count all occurrences of duplicate item") } - -func TestContainsAny_OrderMatters(t *testing.T) { - s := "test string with multiple words" - - // Test that function returns on first match (short-circuit behavior) - // Both should find a match, order shouldn't affect result - result1 := ContainsAny(s, "string", "words") - result2 := ContainsAny(s, "words", "string") - - assert.Equal(t, result1, result2, "should return same result regardless of substring order") - assert.True(t, result1, "should find matches in first ordering") - assert.True(t, result2, "should find matches in second ordering") -} diff --git a/pkg/stringutil/identifiers.go b/pkg/stringutil/identifiers.go index 51d730036ff..8ae8607533a 100644 --- a/pkg/stringutil/identifiers.go +++ b/pkg/stringutil/identifiers.go @@ -99,29 +99,3 @@ func LockFileToMarkdown(lockPath string) string { cleaned := filepath.Clean(lockPath) return strings.TrimSuffix(cleaned, ".lock.yml") + ".md" } - -// IsAgenticWorkflow returns true if the file path is an agentic workflow file. -// Agentic workflows end with .md. -// -// Examples: -// -// IsAgenticWorkflow("test.md") // returns true -// IsAgenticWorkflow("weekly-research.md") // returns true -// IsAgenticWorkflow(".github/workflows/workflow.md") // returns true -// IsAgenticWorkflow("test.lock.yml") // returns false -func IsAgenticWorkflow(path string) bool { - // Must end with .md - return strings.HasSuffix(path, ".md") -} - -// IsLockFile returns true if the file path is a compiled lock file. -// Lock files end with .lock.yml and are compiled from agentic workflows. -// -// Examples: -// -// IsLockFile("test.lock.yml") // returns true -// IsLockFile(".github/workflows/workflow.lock.yml") // returns true -// IsLockFile("test.md") // returns false -func IsLockFile(path string) bool { - return strings.HasSuffix(path, ".lock.yml") -} diff --git a/pkg/stringutil/identifiers_test.go b/pkg/stringutil/identifiers_test.go index e282901e55c..9ed78399864 100644 --- a/pkg/stringutil/identifiers_test.go +++ b/pkg/stringutil/identifiers_test.go @@ -3,7 +3,6 @@ package stringutil import ( - "strings" "testing" ) @@ -283,109 +282,3 @@ func TestRoundTripConversions(t *testing.T) { } }) } - -func TestIsAgenticWorkflow(t *testing.T) { - tests := []struct { - name string - path string - expected bool - }{ - { - name: "regular workflow", - path: "test.md", - expected: true, - }, - { - name: "workflow with path", - path: ".github/workflows/weekly-research.md", - expected: true, - }, - { - name: "workflow with dots in name", - path: "my.workflow.test.md", - expected: true, - }, - { - name: "lock file", - path: "test.lock.yml", - expected: false, - }, - { - name: "no extension", - path: "test", - expected: false, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - result := IsAgenticWorkflow(tt.path) - if result != tt.expected { - t.Errorf("IsAgenticWorkflow(%q) = %v, expected %v", tt.path, result, tt.expected) - } - }) - } -} - -func TestIsLockFile(t *testing.T) { - tests := []struct { - name string - path string - expected bool - }{ - { - name: "regular lock file", - path: "test.lock.yml", - expected: true, - }, - { - name: "lock file with path", - path: ".github/workflows/test.lock.yml", - expected: true, - }, - { - name: "workflow file", - path: "test.md", - expected: false, - }, - { - name: "yaml file", - path: "test.yml", - expected: false, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - result := IsLockFile(tt.path) - if result != tt.expected { - t.Errorf("IsLockFile(%q) = %v, expected %v", tt.path, result, tt.expected) - } - }) - } -} - -func TestFileTypeHelpers_Exclusivity(t *testing.T) { - // Test that file types are mutually exclusive (except lock files) - testPaths := []string{ - "test.md", - "test.lock.yml", - } - - for _, path := range testPaths { - t.Run(path, func(t *testing.T) { - isWorkflow := IsAgenticWorkflow(path) - isLock := IsLockFile(path) - - // All .md files should be workflows - if strings.HasSuffix(path, ".md") && !isWorkflow { - t.Errorf("Path %q should be a workflow but isn't", path) - } - - // All .lock.yml files should be lock files - if strings.HasSuffix(path, ".lock.yml") && !isLock { - t.Errorf("Path %q should be a lock file but isn't", path) - } - }) - } -} diff --git a/pkg/stringutil/pat_validation.go b/pkg/stringutil/pat_validation.go index a1c79bcabd2..53c5e0e957d 100644 --- a/pkg/stringutil/pat_validation.go +++ b/pkg/stringutil/pat_validation.go @@ -66,21 +66,6 @@ func ClassifyPAT(token string) PATType { return patType } -// IsFineGrainedPAT returns true if the token is a fine-grained personal access token -func IsFineGrainedPAT(token string) bool { - return strings.HasPrefix(token, "github_pat_") -} - -// IsClassicPAT returns true if the token is a classic personal access token -func IsClassicPAT(token string) bool { - return strings.HasPrefix(token, "ghp_") -} - -// IsOAuthToken returns true if the token is an OAuth token (not a PAT) -func IsOAuthToken(token string) bool { - return strings.HasPrefix(token, "gho_") -} - // ValidateCopilotPAT validates that a token is a valid fine-grained PAT for Copilot. // Returns an error if the token is not a fine-grained PAT with a descriptive error message. // diff --git a/pkg/stringutil/pat_validation_test.go b/pkg/stringutil/pat_validation_test.go index 2adaa03bea2..4e49a7d2e8a 100644 --- a/pkg/stringutil/pat_validation_test.go +++ b/pkg/stringutil/pat_validation_test.go @@ -74,27 +74,6 @@ func TestPATType_IsValid(t *testing.T) { assert.False(t, PATTypeUnknown.IsValid(), "unknown should not be valid") } -func TestIsFineGrainedPAT(t *testing.T) { - assert.True(t, IsFineGrainedPAT("github_pat_abc123"), "should identify fine-grained PAT") - assert.False(t, IsFineGrainedPAT("ghp_abc123"), "should not identify classic PAT as fine-grained") - assert.False(t, IsFineGrainedPAT("gho_abc123"), "should not identify OAuth token as fine-grained") - assert.False(t, IsFineGrainedPAT("random"), "should not identify unknown token as fine-grained") -} - -func TestIsClassicPAT(t *testing.T) { - assert.True(t, IsClassicPAT("ghp_abc123"), "should identify classic PAT") - assert.False(t, IsClassicPAT("github_pat_abc123"), "should not identify fine-grained PAT as classic") - assert.False(t, IsClassicPAT("gho_abc123"), "should not identify OAuth token as classic") - assert.False(t, IsClassicPAT("random"), "should not identify unknown token as classic") -} - -func TestIsOAuthToken(t *testing.T) { - assert.True(t, IsOAuthToken("gho_abc123"), "should identify OAuth token") - assert.False(t, IsOAuthToken("github_pat_abc123"), "should not identify fine-grained PAT as OAuth") - assert.False(t, IsOAuthToken("ghp_abc123"), "should not identify classic PAT as OAuth") - assert.False(t, IsOAuthToken("random"), "should not identify unknown token as OAuth") -} - func TestValidateCopilotPAT(t *testing.T) { tests := []struct { name string diff --git a/pkg/stringutil/stringutil.go b/pkg/stringutil/stringutil.go index 325c49e1d0e..4dcfeb52280 100644 --- a/pkg/stringutil/stringutil.go +++ b/pkg/stringutil/stringutil.go @@ -81,27 +81,3 @@ func IsPositiveInteger(s string) bool { num, err := strconv.ParseInt(s, 10, 64) return err == nil && num > 0 } - -// 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). -// -// Common ANSI escape sequences that are removed: -// - Color codes: \x1b[31m (red), \x1b[0m (reset) -// - Text formatting: \x1b[1m (bold), \x1b[4m (underline) -// - Cursor control: \x1b[2J (clear screen) -// -// Example: -// -// input := "Hello \x1b[31mWorld\x1b[0m" // "Hello [red]World[reset]" -// output := StripANSIEscapeCodes(input) // "Hello World" -// -// This function is particularly important for: -// - 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 StripANSI(s) -} diff --git a/pkg/stringutil/stringutil_test.go b/pkg/stringutil/stringutil_test.go index 2247f3c1266..ee577db98ac 100644 --- a/pkg/stringutil/stringutil_test.go +++ b/pkg/stringutil/stringutil_test.go @@ -414,163 +414,6 @@ func TestParseVersionValue(t *testing.T) { } } -func TestStripANSIEscapeCodes(t *testing.T) { - tests := []struct { - name string - input string - expected string - }{ - { - name: "no ANSI codes", - input: "Hello World", - expected: "Hello World", - }, - { - name: "simple color reset", - input: "Hello World[m", - expected: "Hello World[m", // [m without ESC is not an ANSI code - }, - { - name: "ANSI color reset", - input: "Hello World\x1b[m", - expected: "Hello World", - }, - { - name: "ANSI color code with reset", - input: "Hello \x1b[31mWorld\x1b[0m", - expected: "Hello World", - }, - { - name: "ANSI bold text", - input: "\x1b[1mBold text\x1b[0m", - expected: "Bold text", - }, - { - name: "multiple ANSI codes", - input: "\x1b[1m\x1b[31mRed Bold\x1b[0m", - expected: "Red Bold", - }, - { - name: "ANSI with parameters", - input: "Text \x1b[1;32mgreen bold\x1b[0m more text", - expected: "Text green bold more text", - }, - { - name: "ANSI clear screen", - input: "\x1b[2JCleared", - expected: "Cleared", - }, - { - name: "empty string", - input: "", - expected: "", - }, - { - name: "only ANSI codes", - input: "\x1b[0m\x1b[31m\x1b[1m", - expected: "", - }, - { - name: "real-world example from issue", - input: "2. **REQUIRED**: Run 'make recompile' to update workflows (MUST be run after any constant changes)\x1b[m", - expected: "2. **REQUIRED**: Run 'make recompile' to update workflows (MUST be run after any constant changes)", - }, - { - name: "another real-world example", - input: "- **SAVE TO CACHE**: Store help outputs (main and all subcommands) and version check results in cache-memory\x1b[m", - expected: "- **SAVE TO CACHE**: Store help outputs (main and all subcommands) and version check results in cache-memory", - }, - { - name: "ANSI underline", - input: "\x1b[4mUnderlined\x1b[0m text", - expected: "Underlined text", - }, - { - name: "ANSI 256 color", - input: "\x1b[38;5;214mOrange\x1b[0m", - expected: "Orange", - }, - { - name: "mixed content with newlines", - input: "Line 1\x1b[31m\nLine 2\x1b[0m\nLine 3", - expected: "Line 1\nLine 2\nLine 3", - }, - { - name: "ANSI cursor movement", - input: "\x1b[2AMove up\x1b[3BMove down", - expected: "Move upMove down", - }, - { - name: "ANSI erase in line", - input: "Start\x1b[KEnd", - expected: "StartEnd", - }, - { - name: "consecutive ANSI codes", - input: "\x1b[1m\x1b[31m\x1b[4mRed Bold Underline\x1b[0m\x1b[0m\x1b[0m", - expected: "Red Bold Underline", - }, - { - name: "ANSI with large parameter", - input: "\x1b[38;5;255mWhite\x1b[0m", - expected: "White", - }, - { - name: "ANSI RGB color (24-bit)", - input: "\x1b[38;2;255;128;0mOrange RGB\x1b[0m", - expected: "Orange RGB", - }, - { - name: "ANSI codes in the middle of words", - input: "hel\x1b[31mlo\x1b[0m wor\x1b[32mld\x1b[0m", - expected: "hello world", - }, - { - name: "ANSI save/restore cursor", - input: "Text\x1b[s more text\x1b[u end", - expected: "Text more text end", - }, - { - name: "ANSI cursor position", - input: "\x1b[H\x1b[2JClear and home", - expected: "Clear and home", - }, - { - name: "long string with multiple ANSI codes", - input: "\x1b[1mThis\x1b[0m \x1b[31mis\x1b[0m \x1b[32ma\x1b[0m \x1b[33mvery\x1b[0m \x1b[34mlong\x1b[0m \x1b[35mstring\x1b[0m \x1b[36mwith\x1b[0m \x1b[37mmany\x1b[0m \x1b[1mANSI\x1b[0m \x1b[4mcodes\x1b[0m", - expected: "This is a very long string with many ANSI codes", - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - result := StripANSIEscapeCodes(tt.input) - if result != tt.expected { - t.Errorf("StripANSIEscapeCodes(%q) = %q, expected %q", tt.input, result, tt.expected) - } - - // Verify no ANSI escape sequences remain - if result != "" && strings.Contains(result, "\x1b[") { - t.Errorf("Result still contains ANSI escape sequences: %q", result) - } - }) - } -} - -func BenchmarkStripANSIEscapeCodes_Clean(b *testing.B) { - s := "This is a clean string without any ANSI codes" - for b.Loop() { - StripANSIEscapeCodes(s) - } -} - -func BenchmarkStripANSIEscapeCodes_WithCodes(b *testing.B) { - s := "This \x1b[31mhas\x1b[0m some \x1b[1mANSI\x1b[0m codes" - for b.Loop() { - StripANSIEscapeCodes(s) - } -} - func TestIsPositiveInteger(t *testing.T) { tests := []struct { name string From 13914cf905bf33b64dec45f4d843e2f0a941b6f7 Mon Sep 17 00:00:00 2001 From: Don Syme Date: Sat, 28 Feb 2026 12:45:27 +0000 Subject: [PATCH 3/5] dead13: remove dead functions from parser and workflow packages - Delete EnsureLocalhostDomains from pkg/parser/mcp.go and its test - Delete AddIfTrue from pkg/workflow/compiler_safe_outputs_config.go - Delete HasUserCheckouts from pkg/workflow/checkout_manager.go - Delete 14 dead methods from pkg/workflow/artifact_manager.go (SetCurrentJob, GetCurrentJob, RecordUpload, RecordDownload, computeNormalizedPaths, findCommonParent, ComputeDownloadPath, FindUploadedArtifact, ValidateDownload, matchesPattern, GetUploadsForJob, GetDownloadsForJob, ValidateAllDownloads, GetAllArtifacts) - Delete artifact_manager test files (all tests tested deleted methods): artifact_manager_test.go, artifact_manager_integration_test.go, artifact_manager_workflows_integration_test.go 2216 lines removed across 9 files --- pkg/parser/mcp.go | 49 - pkg/parser/mcp_test.go | 57 -- pkg/workflow/artifact_manager.go | 383 -------- .../artifact_manager_integration_test.go | 280 ------ pkg/workflow/artifact_manager_test.go | 866 ------------------ ...fact_manager_workflows_integration_test.go | 566 ------------ pkg/workflow/checkout_manager.go | 5 - pkg/workflow/checkout_manager_test.go | 4 +- pkg/workflow/compiler_safe_outputs_config.go | 8 - 9 files changed, 2 insertions(+), 2216 deletions(-) delete mode 100644 pkg/workflow/artifact_manager_integration_test.go delete mode 100644 pkg/workflow/artifact_manager_test.go delete mode 100644 pkg/workflow/artifact_manager_workflows_integration_test.go diff --git a/pkg/parser/mcp.go b/pkg/parser/mcp.go index d3375207df8..c4d038e887a 100644 --- a/pkg/parser/mcp.go +++ b/pkg/parser/mcp.go @@ -30,55 +30,6 @@ func IsMCPType(typeStr string) bool { } } -// EnsureLocalhostDomains ensures that localhost and 127.0.0.1 are always included -// in the allowed domains list for Playwright, even when custom domains are specified -// Includes port variations to allow all ports on localhost and 127.0.0.1 -func EnsureLocalhostDomains(domains []string) []string { - hasLocalhost := false - hasLocalhostPorts := false - hasLoopback := false - hasLoopbackPorts := false - - for _, domain := range domains { - switch domain { - case "localhost": - hasLocalhost = true - case "localhost:*": - hasLocalhostPorts = true - case "127.0.0.1": - hasLoopback = true - case "127.0.0.1:*": - hasLoopbackPorts = true - } - } - - // CWE-190: Allocation Size Overflow Prevention - // Instead of pre-calculating capacity (len(domains)+4), which could overflow - // if domains is extremely large, we let Go's append handle capacity growth - // automatically. This is safe and efficient for domain arrays which are - // typically small in practice. - var result []string - - // Always add localhost domains first (with and without port specifications) - if !hasLocalhost { - result = append(result, "localhost") - } - if !hasLocalhostPorts { - result = append(result, "localhost:*") - } - if !hasLoopback { - result = append(result, "127.0.0.1") - } - if !hasLoopbackPorts { - result = append(result, "127.0.0.1:*") - } - - // Add the rest of the domains - result = append(result, domains...) - - return result -} - // MCPServerConfig represents a parsed MCP server configuration. // It embeds BaseMCPServerConfig for common fields and adds parser-specific fields. type MCPServerConfig struct { diff --git a/pkg/parser/mcp_test.go b/pkg/parser/mcp_test.go index 40c73f533fc..e0bd82427fa 100644 --- a/pkg/parser/mcp_test.go +++ b/pkg/parser/mcp_test.go @@ -13,63 +13,6 @@ import ( ) // TestEnsureLocalhostDomains tests the helper function that ensures localhost domains are always included -func TestEnsureLocalhostDomains(t *testing.T) { - tests := []struct { - name string - input []string - expected []string - }{ - { - name: "Empty input should add all localhost domains with ports", - input: []string{}, - expected: []string{"localhost", "localhost:*", "127.0.0.1", "127.0.0.1:*"}, - }, - { - name: "Custom domains without localhost should add localhost domains with ports", - input: []string{"github.com", "*.github.com"}, - expected: []string{"localhost", "localhost:*", "127.0.0.1", "127.0.0.1:*", "github.com", "*.github.com"}, - }, - { - name: "Input with localhost but no 127.0.0.1 should add missing domains", - input: []string{"localhost", "example.com"}, - expected: []string{"localhost:*", "127.0.0.1", "127.0.0.1:*", "localhost", "example.com"}, - }, - { - name: "Input with 127.0.0.1 but no localhost should add missing domains", - input: []string{"127.0.0.1", "example.com"}, - expected: []string{"localhost", "localhost:*", "127.0.0.1:*", "127.0.0.1", "example.com"}, - }, - { - name: "Input with both localhost domains should add port variants", - input: []string{"localhost", "127.0.0.1", "example.com"}, - expected: []string{"localhost:*", "127.0.0.1:*", "localhost", "127.0.0.1", "example.com"}, - }, - { - name: "Input with both in different order should add port variants", - input: []string{"example.com", "127.0.0.1", "localhost"}, - expected: []string{"localhost:*", "127.0.0.1:*", "example.com", "127.0.0.1", "localhost"}, - }, - { - name: "Input with all localhost variants should remain unchanged", - input: []string{"localhost", "localhost:*", "127.0.0.1", "127.0.0.1:*", "example.com"}, - expected: []string{"localhost", "localhost:*", "127.0.0.1", "127.0.0.1:*", "example.com"}, - }, - { - name: "Input with some localhost variants should add missing ones", - input: []string{"localhost:*", "127.0.0.1", "example.com"}, - expected: []string{"localhost", "127.0.0.1:*", "localhost:*", "127.0.0.1", "example.com"}, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - result := EnsureLocalhostDomains(tt.input) - if !reflect.DeepEqual(result, tt.expected) { - t.Errorf("EnsureLocalhostDomains(%v) = %v, want %v", tt.input, result, tt.expected) - } - }) - } -} func TestExtractMCPConfigurations(t *testing.T) { tests := []struct { diff --git a/pkg/workflow/artifact_manager.go b/pkg/workflow/artifact_manager.go index f3a9036816c..966c9fb97a1 100644 --- a/pkg/workflow/artifact_manager.go +++ b/pkg/workflow/artifact_manager.go @@ -1,11 +1,6 @@ package workflow import ( - "errors" - "fmt" - "path/filepath" - "strings" - "github.com/github/gh-aw/pkg/logger" ) @@ -98,388 +93,10 @@ func NewArtifactManager() *ArtifactManager { } } -// SetCurrentJob sets the job currently being processed -func (am *ArtifactManager) SetCurrentJob(jobName string) { - artifactManagerLog.Printf("Setting current job: %s", jobName) - am.currentJob = jobName -} - -// GetCurrentJob returns the current job name -func (am *ArtifactManager) GetCurrentJob() string { - return am.currentJob -} - -// RecordUpload records an artifact upload operation -func (am *ArtifactManager) RecordUpload(upload *ArtifactUpload) error { - if upload.Name == "" { - return errors.New("artifact upload must have a name") - } - if len(upload.Paths) == 0 { - return errors.New("artifact upload must have at least one path") - } - - // Set the job name if not already set - if upload.JobName == "" { - upload.JobName = am.currentJob - } - - // Compute normalized paths with common parent removed - upload.NormalizedPaths = computeNormalizedPaths(upload.Paths) - - artifactManagerLog.Printf("Recording upload: artifact=%s, job=%s, paths=%v, normalized=%v", - upload.Name, upload.JobName, upload.Paths, upload.NormalizedPaths) - - am.uploads[upload.JobName] = append(am.uploads[upload.JobName], upload) - return nil -} - -// RecordDownload records an artifact download operation -func (am *ArtifactManager) RecordDownload(download *ArtifactDownload) error { - if download.Name == "" && download.Pattern == "" { - return errors.New("artifact download must have either name or pattern") - } - if download.Path == "" { - return errors.New("artifact download must have a path") - } - - // Set the job name if not already set - if download.JobName == "" { - download.JobName = am.currentJob - } - - artifactManagerLog.Printf("Recording download: name=%s, pattern=%s, job=%s, path=%s", - download.Name, download.Pattern, download.JobName, download.Path) - - am.downloads[download.JobName] = append(am.downloads[download.JobName], download) - return nil -} - -// computeNormalizedPaths computes normalized paths with common parent directory removed. -// This simulates GitHub Actions behavior where files uploaded with paths like: -// // /tmp/gh-aw/aw-prompts/prompt.txt // /tmp/gh-aw/aw.patch -// -// are stored in the artifact as: -// // aw-prompts/prompt.txt // aw.patch -// -// (with common parent /tmp/gh-aw/ removed) -func computeNormalizedPaths(paths []string) map[string]string { - if len(paths) == 0 { - return nil - } - - // If only one path, normalize it relative to its parent - if len(paths) == 1 { - path := filepath.Clean(paths[0]) - // Get the base name (file/dir name without parent) - base := filepath.Base(path) - result := make(map[string]string) - result[path] = base - artifactManagerLog.Printf("Single path normalization: %s -> %s", path, base) - return result - } - - // Find common parent directory for multiple paths - commonParent := findCommonParent(paths) - artifactManagerLog.Printf("Common parent for %d paths: %s", len(paths), commonParent) - - // Create mapping of original path to normalized path - normalized := make(map[string]string) - for _, path := range paths { - cleanPath := filepath.Clean(path) - var relativePath string - - if commonParent != "" && commonParent != "." { - // Remove common parent - rel, err := filepath.Rel(commonParent, cleanPath) - if err != nil { - // If we can't compute relative path, use the base name - relativePath = filepath.Base(cleanPath) - } else { - relativePath = rel - } - } else { - // No common parent, use base name - relativePath = filepath.Base(cleanPath) - } - - normalized[cleanPath] = relativePath - artifactManagerLog.Printf("Path normalization: %s -> %s (parent: %s)", cleanPath, relativePath, commonParent) - } - - return normalized -} - -// findCommonParent finds the common parent directory of multiple paths -func findCommonParent(paths []string) string { - if len(paths) == 0 { - return "" - } - if len(paths) == 1 { - return filepath.Dir(filepath.Clean(paths[0])) - } - - // Clean all paths and split into components - splitPaths := make([][]string, len(paths)) - for i, p := range paths { - cleanPath := filepath.Clean(p) - // Split the full path (not just directory) - // Handle absolute paths starting with / - if strings.HasPrefix(cleanPath, string(filepath.Separator)) { - cleanPath = cleanPath[1:] // Remove leading separator for splitting - } - splitPaths[i] = strings.Split(cleanPath, string(filepath.Separator)) - } - - // Find the minimum length among all paths - minLen := len(splitPaths[0]) - for _, sp := range splitPaths[1:] { - if len(sp) < minLen { - minLen = len(sp) - } - } - - // Find common prefix by comparing each component - var commonParts []string - for i := range minLen - 1 { // minLen-1 to exclude the filename - part := splitPaths[0][i] - allMatch := true - for _, sp := range splitPaths[1:] { - if sp[i] != part { - allMatch = false - break - } - } - if allMatch { - commonParts = append(commonParts, part) - } else { - break - } - } - - if len(commonParts) == 0 { - return "" - } - - // Reconstruct the path with leading separator if original paths were absolute - result := filepath.Join(commonParts...) - if strings.HasPrefix(paths[0], string(filepath.Separator)) { - result = string(filepath.Separator) + result - } - - return result -} - -// ComputeDownloadPath computes the actual file path after download -// based on GitHub Actions v4 behavior. -// -// Rules: -// - Download by name: files extracted directly to path/ (e.g., path/file.txt) -// - Download by pattern without merge: files in path/artifact-name/ (e.g., path/artifact-1/file.txt) -// - Download by pattern with merge: files extracted directly to path/ (e.g., path/file.txt) -// - Common parent directories are stripped during upload (simulated via NormalizedPaths) -func (am *ArtifactManager) ComputeDownloadPath(download *ArtifactDownload, upload *ArtifactUpload, originalPath string) string { - // Get the normalized path (with common parent removed) from the upload - // This simulates how GitHub Actions strips common parent directories - cleanOriginal := filepath.Clean(originalPath) - normalizedPath := cleanOriginal - - // If upload has normalized paths, use them - if upload.NormalizedPaths != nil { - if normalized, ok := upload.NormalizedPaths[cleanOriginal]; ok { - normalizedPath = normalized - artifactManagerLog.Printf("Using normalized path from upload: %s -> %s", cleanOriginal, normalizedPath) - } - } else { - // Fallback: remove leading ./ - normalizedPath = strings.TrimPrefix(originalPath, "./") - } - - // If downloading by name (not pattern), files go directly to download path - if download.Name != "" && download.Pattern == "" { - result := filepath.Join(download.Path, normalizedPath) - artifactManagerLog.Printf("Download by name: %s -> %s", originalPath, result) - return result - } - - // If downloading by pattern with merge-multiple, files go directly to download path - if download.Pattern != "" && download.MergeMultiple { - result := filepath.Join(download.Path, normalizedPath) - artifactManagerLog.Printf("Download by pattern (merge): %s -> %s", originalPath, result) - return result - } - - // If downloading by pattern without merge, files go to path/artifact-name/ - if download.Pattern != "" && !download.MergeMultiple { - result := filepath.Join(download.Path, upload.Name, normalizedPath) - artifactManagerLog.Printf("Download by pattern (no merge): %s -> %s", originalPath, result) - return result - } - - // Default: direct to download path - result := filepath.Join(download.Path, normalizedPath) - artifactManagerLog.Printf("Download default: %s -> %s", originalPath, result) - return result -} - -// FindUploadedArtifact finds an uploaded artifact by name from jobs this job depends on -func (am *ArtifactManager) FindUploadedArtifact(artifactName string, dependsOn []string) *ArtifactUpload { - // Search in all dependent jobs - for _, jobName := range dependsOn { - uploads := am.uploads[jobName] - for _, upload := range uploads { - if upload.Name == artifactName { - artifactManagerLog.Printf("Found artifact %s uploaded by job %s", artifactName, jobName) - return upload - } - } - } - - // If not found in dependencies, search all jobs (for backwards compatibility) - // This handles cases where dependencies aren't explicitly tracked - for jobName, uploads := range am.uploads { - for _, upload := range uploads { - if upload.Name == artifactName { - artifactManagerLog.Printf("Found artifact %s uploaded by job %s (global search)", artifactName, jobName) - return upload - } - } - } - - artifactManagerLog.Printf("Artifact %s not found in any job", artifactName) - return nil -} - -// ValidateDownload validates that a download operation can find its artifact -func (am *ArtifactManager) ValidateDownload(download *ArtifactDownload) error { - if download.Name != "" { - // Download by name - must find exact artifact - upload := am.FindUploadedArtifact(download.Name, download.DependsOn) - if upload == nil { - return fmt.Errorf("artifact '%s' downloaded by job '%s' not found in any dependent job", - download.Name, download.JobName) - } - artifactManagerLog.Printf("Validated download: artifact=%s exists in job=%s", - download.Name, upload.JobName) - } - - if download.Pattern != "" { - // Download by pattern - must find at least one matching artifact - found := false - for _, jobName := range download.DependsOn { - uploads := am.uploads[jobName] - for _, upload := range uploads { - // Simple pattern matching for now (could be enhanced with glob) - if matchesPattern(upload.Name, download.Pattern) { - found = true - break - } - } - if found { - break - } - } - if !found { - // Try global search - for _, uploads := range am.uploads { - for _, upload := range uploads { - if matchesPattern(upload.Name, download.Pattern) { - found = true - break - } - } - if found { - break - } - } - } - if !found { - return fmt.Errorf("no artifacts matching pattern '%s' found for job '%s'", - download.Pattern, download.JobName) - } - artifactManagerLog.Printf("Validated download: pattern=%s matches at least one artifact", - download.Pattern) - } - - return nil -} - -// matchesPattern performs simple wildcard pattern matching -// Supports * as wildcard (e.g., "agent-*" matches "agent-artifacts") -func matchesPattern(name, pattern string) bool { - // If pattern has no wildcard, do exact match - if !strings.Contains(pattern, "*") { - return name == pattern - } - - // Handle leading wildcard: "*suffix" - if after, ok := strings.CutPrefix(pattern, "*"); ok { - suffix := after - return strings.HasSuffix(name, suffix) - } - - // Handle trailing wildcard: "prefix*" - if before, ok := strings.CutSuffix(pattern, "*"); ok { - prefix := before - return strings.HasPrefix(name, prefix) - } - - // Handle middle wildcard: "prefix*suffix" - parts := strings.Split(pattern, "*") - if len(parts) == 2 { - prefix, suffix := parts[0], parts[1] - // Check that name starts with prefix, ends with suffix, and has something in between - if strings.HasPrefix(name, prefix) && strings.HasSuffix(name, suffix) { - // Ensure there's content between prefix and suffix - // The middle part should be at least as long as the non-overlapping parts - minLength := len(prefix) + len(suffix) - return len(name) >= minLength - } - return false - } - - // For more complex patterns, just do exact match - return name == pattern -} - -// GetUploadsForJob returns all uploads for a specific job -func (am *ArtifactManager) GetUploadsForJob(jobName string) []*ArtifactUpload { - return am.uploads[jobName] -} - -// GetDownloadsForJob returns all downloads for a specific job -func (am *ArtifactManager) GetDownloadsForJob(jobName string) []*ArtifactDownload { - return am.downloads[jobName] -} - -// ValidateAllDownloads validates all download operations -func (am *ArtifactManager) ValidateAllDownloads() []error { - var errors []error - - for jobName, downloads := range am.downloads { - for _, download := range downloads { - if err := am.ValidateDownload(download); err != nil { - errors = append(errors, fmt.Errorf("job %s: %w", jobName, err)) - } - } - } - - if len(errors) > 0 { - artifactManagerLog.Printf("Validation found %d error(s)", len(errors)) - } else { - artifactManagerLog.Print("All downloads validated successfully") - } - - return errors -} - -// GetAllArtifacts returns all uploaded artifacts -func (am *ArtifactManager) GetAllArtifacts() map[string][]*ArtifactUpload { - return am.uploads -} // Reset clears all tracked uploads and downloads func (am *ArtifactManager) Reset() { diff --git a/pkg/workflow/artifact_manager_integration_test.go b/pkg/workflow/artifact_manager_integration_test.go deleted file mode 100644 index c17df1b6ebb..00000000000 --- a/pkg/workflow/artifact_manager_integration_test.go +++ /dev/null @@ -1,280 +0,0 @@ -//go:build integration - -package workflow - -import ( - "os" - "path/filepath" - "testing" - - "github.com/github/gh-aw/pkg/stringutil" - - "github.com/github/gh-aw/pkg/testutil" - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" -) - -// TestArtifactManagerIntegrationWithCompiler tests that the artifact manager -// is properly integrated into the compiler and resets between compilations -func TestArtifactManagerIntegrationWithCompiler(t *testing.T) { - tmpDir := testutil.TempDir(t, "artifact-manager-integration-*") - - // Create a simple workflow - workflowContent := `--- -on: workflow_dispatch -permissions: - contents: read -engine: copilot ---- - -# Test Artifact Manager Integration - -This test verifies that the artifact manager is integrated into the compiler. -` - - workflowFile := filepath.Join(tmpDir, "test-workflow.md") - err := os.WriteFile(workflowFile, []byte(workflowContent), 0644) - require.NoError(t, err) - - // Create compiler - compiler := NewCompiler() - - // Verify artifact manager is initialized - artifactManager := compiler.GetArtifactManager() - require.NotNil(t, artifactManager, "Artifact manager should be initialized") - - // First compilation - err = compiler.CompileWorkflow(workflowFile) - require.NoError(t, err) - - // Artifact manager should be reset (empty) after first compilation - assert.Empty(t, artifactManager.GetAllArtifacts(), "Artifact manager should be reset between compilations") - - // Manually add some test data to artifact manager - artifactManager.SetCurrentJob("test-job") - err = artifactManager.RecordUpload(&ArtifactUpload{ - Name: "test-artifact", - Paths: []string{"/tmp/test.txt"}, - JobName: "test-job", - }) - require.NoError(t, err) - - // Second compilation should reset the artifact manager - err = compiler.CompileWorkflow(workflowFile) - require.NoError(t, err) - - // Verify artifact manager was reset - assert.Empty(t, artifactManager.GetAllArtifacts(), "Artifact manager should be reset after second compilation") -} - -// TestArtifactManagerAccessDuringCompilation demonstrates how the artifact -// manager can be accessed and used during workflow compilation -func TestArtifactManagerAccessDuringCompilation(t *testing.T) { - tmpDir := testutil.TempDir(t, "artifact-manager-access-*") - - workflowContent := `--- -on: workflow_dispatch -permissions: - contents: read -safe-outputs: - create-issue: - title-prefix: "[bot] " -engine: copilot ---- - -# Test Artifact Manager Access - -This workflow has safe outputs configured. -` - - workflowFile := filepath.Join(tmpDir, "test-workflow.md") - err := os.WriteFile(workflowFile, []byte(workflowContent), 0644) - require.NoError(t, err) - - compiler := NewCompiler() - - // Compile the workflow - err = compiler.CompileWorkflow(workflowFile) - require.NoError(t, err) - - // Access the artifact manager after compilation - artifactManager := compiler.GetArtifactManager() - require.NotNil(t, artifactManager) - - // The manager should be available but empty (since we didn't track anything yet) - // In future integration, the compiler would populate this during job generation - assert.NotNil(t, artifactManager, "Artifact manager should be accessible after compilation") -} - -// TestArtifactManagerWithMultipleWorkflows tests that the artifact manager -// properly resets between multiple workflow compilations -func TestArtifactManagerWithMultipleWorkflows(t *testing.T) { - tmpDir := testutil.TempDir(t, "artifact-manager-multi-*") - - // Create multiple workflow files - workflows := []struct { - name string - content string - }{ - { - name: "workflow1.md", - content: `--- -on: push -permissions: - contents: read -engine: copilot ---- - -# Workflow 1 -Test workflow 1. -`, - }, - { - name: "workflow2.md", - content: `--- -on: pull_request -permissions: - contents: read -engine: copilot ---- - -# Workflow 2 -Test workflow 2. -`, - }, - { - name: "workflow3.md", - content: `--- -on: workflow_dispatch -permissions: - contents: read -engine: copilot ---- - -# Workflow 3 -Test workflow 3. -`, - }, - } - - compiler := NewCompiler() - artifactManager := compiler.GetArtifactManager() - - for i, wf := range workflows { - workflowFile := filepath.Join(tmpDir, wf.name) - err := os.WriteFile(workflowFile, []byte(wf.content), 0644) - require.NoError(t, err) - - // Add some test data before compilation - artifactManager.SetCurrentJob("test-job") - err = artifactManager.RecordUpload(&ArtifactUpload{ - Name: "artifact-" + wf.name, - Paths: []string{"/tmp/file.txt"}, - JobName: "test-job", - }) - require.NoError(t, err) - - // Compile workflow - err = compiler.CompileWorkflow(workflowFile) - require.NoError(t, err, "Workflow %d should compile successfully", i+1) - - // Verify artifact manager was reset - assert.Empty(t, artifactManager.GetAllArtifacts(), - "Artifact manager should be reset after compiling workflow %d", i+1) - - // Verify lock file was created - lockFile := stringutil.MarkdownToLockFile(workflowFile) - _, err = os.Stat(lockFile) - assert.NoError(t, err, "Lock file should exist for workflow %d", i+1) - } -} - -// TestArtifactManagerLazyInitialization tests that the artifact manager -// is lazily initialized if not present -func TestArtifactManagerLazyInitialization(t *testing.T) { - tmpDir := testutil.TempDir(t, "artifact-manager-lazy-*") - - workflowContent := `--- -on: workflow_dispatch -permissions: - contents: read -engine: copilot ---- - -# Test Lazy Init - -Test lazy initialization. -` - - workflowFile := filepath.Join(tmpDir, "test-workflow.md") - err := os.WriteFile(workflowFile, []byte(workflowContent), 0644) - require.NoError(t, err) - - // Create compiler without initializing artifact manager - compiler := &Compiler{ - verbose: false, - version: "test", - skipValidation: true, - actionMode: ActionModeDev, - jobManager: NewJobManager(), - engineRegistry: GetGlobalEngineRegistry(), - stepOrderTracker: NewStepOrderTracker(), - // artifactManager intentionally not initialized - } - - // GetArtifactManager should lazy-initialize - artifactManager := compiler.GetArtifactManager() - assert.NotNil(t, artifactManager, "GetArtifactManager should lazy-initialize") - - // Second call should return same instance - artifactManager2 := compiler.GetArtifactManager() - assert.Same(t, artifactManager, artifactManager2, "Should return same instance") -} - -// TestArtifactManagerValidationExample demonstrates how validation could work -// This is a conceptual test showing how the artifact manager could validate -// artifact dependencies in a workflow -func TestArtifactManagerValidationExample(t *testing.T) { - // Create a compiler with artifact manager - compiler := NewCompiler() - artifactManager := compiler.GetArtifactManager() - - // Simulate job 1 uploading an artifact - artifactManager.SetCurrentJob("build") - err := artifactManager.RecordUpload(&ArtifactUpload{ - Name: "build-artifact", - Paths: []string{"/dist/app"}, - JobName: "build", - }) - require.NoError(t, err) - - // Simulate job 2 downloading the artifact - artifactManager.SetCurrentJob("test") - err = artifactManager.RecordDownload(&ArtifactDownload{ - Name: "build-artifact", - Path: "/tmp/build", - JobName: "test", - DependsOn: []string{"build"}, - }) - require.NoError(t, err) - - // Validate all downloads - errors := artifactManager.ValidateAllDownloads() - assert.Empty(t, errors, "All downloads should be valid") - - // Simulate a job trying to download a non-existent artifact - artifactManager.SetCurrentJob("deploy") - err = artifactManager.RecordDownload(&ArtifactDownload{ - Name: "nonexistent-artifact", - Path: "/tmp/deploy", - JobName: "deploy", - DependsOn: []string{"build"}, - }) - require.NoError(t, err) - - // Validation should catch the missing artifact - errors = artifactManager.ValidateAllDownloads() - assert.Len(t, errors, 1, "Should detect missing artifact") - assert.Contains(t, errors[0].Error(), "nonexistent-artifact") - assert.Contains(t, errors[0].Error(), "not found") -} diff --git a/pkg/workflow/artifact_manager_test.go b/pkg/workflow/artifact_manager_test.go deleted file mode 100644 index 9cff97b0d7d..00000000000 --- a/pkg/workflow/artifact_manager_test.go +++ /dev/null @@ -1,866 +0,0 @@ -//go:build !integration - -package workflow - -import ( - "testing" - - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" -) - -func TestNewArtifactManager(t *testing.T) { - am := NewArtifactManager() - assert.NotNil(t, am) - assert.NotNil(t, am.uploads) - assert.NotNil(t, am.downloads) - assert.Empty(t, am.currentJob) -} - -func TestSetCurrentJob(t *testing.T) { - am := NewArtifactManager() - am.SetCurrentJob("test-job") - assert.Equal(t, "test-job", am.GetCurrentJob()) -} - -func TestRecordUpload(t *testing.T) { - tests := []struct { - name string - upload *ArtifactUpload - wantError bool - errorMsg string - }{ - { - name: "valid upload", - upload: &ArtifactUpload{ - Name: "test-artifact", - Paths: []string{"/tmp/test.txt"}, - JobName: "test-job", - }, - wantError: false, - }, - { - name: "upload without name", - upload: &ArtifactUpload{ - Paths: []string{"/tmp/test.txt"}, - JobName: "test-job", - }, - wantError: true, - errorMsg: "artifact upload must have a name", - }, - { - name: "upload without paths", - upload: &ArtifactUpload{ - Name: "test-artifact", - Paths: []string{}, - JobName: "test-job", - }, - wantError: true, - errorMsg: "artifact upload must have at least one path", - }, - { - name: "upload with multiple paths", - upload: &ArtifactUpload{ - Name: "multi-path-artifact", - Paths: []string{"/tmp/file1.txt", "/tmp/file2.txt"}, - JobName: "test-job", - }, - wantError: false, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - am := NewArtifactManager() - err := am.RecordUpload(tt.upload) - - if tt.wantError { - require.Error(t, err) - assert.Contains(t, err.Error(), tt.errorMsg) - } else { - require.NoError(t, err) - uploads := am.GetUploadsForJob(tt.upload.JobName) - assert.Len(t, uploads, 1) - assert.Equal(t, tt.upload.Name, uploads[0].Name) - } - }) - } -} - -func TestRecordUploadUsesCurrentJob(t *testing.T) { - am := NewArtifactManager() - am.SetCurrentJob("current-job") - - upload := &ArtifactUpload{ - Name: "test-artifact", - Paths: []string{"/tmp/test.txt"}, - // JobName not set - should use current job - } - - err := am.RecordUpload(upload) - require.NoError(t, err) - assert.Equal(t, "current-job", upload.JobName) - - uploads := am.GetUploadsForJob("current-job") - assert.Len(t, uploads, 1) -} - -func TestRecordDownload(t *testing.T) { - tests := []struct { - name string - download *ArtifactDownload - wantError bool - errorMsg string - }{ - { - name: "valid download by name", - download: &ArtifactDownload{ - Name: "test-artifact", - Path: "/tmp/download", - JobName: "test-job", - }, - wantError: false, - }, - { - name: "valid download by pattern", - download: &ArtifactDownload{ - Pattern: "agent-*", - Path: "/tmp/download", - JobName: "test-job", - }, - wantError: false, - }, - { - name: "download without name or pattern", - download: &ArtifactDownload{ - Path: "/tmp/download", - JobName: "test-job", - }, - wantError: true, - errorMsg: "artifact download must have either name or pattern", - }, - { - name: "download without path", - download: &ArtifactDownload{ - Name: "test-artifact", - JobName: "test-job", - }, - wantError: true, - errorMsg: "artifact download must have a path", - }, - { - name: "download with merge-multiple", - download: &ArtifactDownload{ - Pattern: "build-*", - Path: "/tmp/builds", - MergeMultiple: true, - JobName: "test-job", - }, - wantError: false, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - am := NewArtifactManager() - err := am.RecordDownload(tt.download) - - if tt.wantError { - require.Error(t, err) - assert.Contains(t, err.Error(), tt.errorMsg) - } else { - require.NoError(t, err) - downloads := am.GetDownloadsForJob(tt.download.JobName) - assert.Len(t, downloads, 1) - } - }) - } -} - -func TestRecordDownloadUsesCurrentJob(t *testing.T) { - am := NewArtifactManager() - am.SetCurrentJob("current-job") - - download := &ArtifactDownload{ - Name: "test-artifact", - Path: "/tmp/download", - // JobName not set - should use current job - } - - err := am.RecordDownload(download) - require.NoError(t, err) - assert.Equal(t, "current-job", download.JobName) - - downloads := am.GetDownloadsForJob("current-job") - assert.Len(t, downloads, 1) -} - -func TestComputeDownloadPath(t *testing.T) { - tests := []struct { - name string - download *ArtifactDownload - upload *ArtifactUpload - originalPath string - expectedPath string - }{ - { - name: "download by name - direct path", - download: &ArtifactDownload{ - Name: "agent-artifacts", - Path: "/tmp/download", - }, - upload: &ArtifactUpload{ - Name: "agent-artifacts", - }, - originalPath: "file.txt", - expectedPath: "/tmp/download/file.txt", - }, - { - name: "download by name - nested file", - download: &ArtifactDownload{ - Name: "agent-artifacts", - Path: "/tmp/download", - }, - upload: &ArtifactUpload{ - Name: "agent-artifacts", - }, - originalPath: "subdir/file.txt", - expectedPath: "/tmp/download/subdir/file.txt", - }, - { - name: "download by pattern with merge - direct path", - download: &ArtifactDownload{ - Pattern: "build-*", - Path: "/tmp/builds", - MergeMultiple: true, - }, - upload: &ArtifactUpload{ - Name: "build-linux", - }, - originalPath: "app.exe", - expectedPath: "/tmp/builds/app.exe", - }, - { - name: "download by pattern without merge - artifact subdirectory", - download: &ArtifactDownload{ - Pattern: "build-*", - Path: "/tmp/builds", - MergeMultiple: false, - }, - upload: &ArtifactUpload{ - Name: "build-linux", - }, - originalPath: "app.exe", - expectedPath: "/tmp/builds/build-linux/app.exe", - }, - { - name: "download by pattern without merge - nested file", - download: &ArtifactDownload{ - Pattern: "agent-*", - Path: "/tmp/agents", - MergeMultiple: false, - }, - upload: &ArtifactUpload{ - Name: "agent-output", - }, - originalPath: "logs/output.json", - expectedPath: "/tmp/agents/agent-output/logs/output.json", - }, - { - name: "download with leading ./ in original path", - download: &ArtifactDownload{ - Name: "test-artifact", - Path: "/tmp/test", - }, - upload: &ArtifactUpload{ - Name: "test-artifact", - }, - originalPath: "./data/file.txt", - expectedPath: "/tmp/test/data/file.txt", - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - am := NewArtifactManager() - result := am.ComputeDownloadPath(tt.download, tt.upload, tt.originalPath) - assert.Equal(t, tt.expectedPath, result) - }) - } -} - -func TestFindUploadedArtifact(t *testing.T) { - am := NewArtifactManager() - - // Setup: create uploads in different jobs - am.SetCurrentJob("job1") - err := am.RecordUpload(&ArtifactUpload{ - Name: "artifact-1", - Paths: []string{"/tmp/file1.txt"}, - JobName: "job1", - }) - require.NoError(t, err) - - am.SetCurrentJob("job2") - err = am.RecordUpload(&ArtifactUpload{ - Name: "artifact-2", - Paths: []string{"/tmp/file2.txt"}, - JobName: "job2", - }) - require.NoError(t, err) - - tests := []struct { - name string - artifactName string - dependsOn []string - expectFound bool - expectedJob string - }{ - { - name: "find artifact in dependencies", - artifactName: "artifact-1", - dependsOn: []string{"job1"}, - expectFound: true, - expectedJob: "job1", - }, - { - name: "find artifact with multiple dependencies", - artifactName: "artifact-2", - dependsOn: []string{"job1", "job2"}, - expectFound: true, - expectedJob: "job2", - }, - { - name: "artifact not in dependencies but exists", - artifactName: "artifact-1", - dependsOn: []string{"job2"}, - expectFound: true, - expectedJob: "job1", - }, - { - name: "artifact does not exist", - artifactName: "nonexistent", - dependsOn: []string{"job1", "job2"}, - expectFound: false, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - result := am.FindUploadedArtifact(tt.artifactName, tt.dependsOn) - - if tt.expectFound { - assert.NotNil(t, result, "Expected to find artifact") - assert.Equal(t, tt.artifactName, result.Name) - assert.Equal(t, tt.expectedJob, result.JobName) - } else { - assert.Nil(t, result, "Expected not to find artifact") - } - }) - } -} - -func TestValidateDownload(t *testing.T) { - am := NewArtifactManager() - - // Setup: create uploads - am.SetCurrentJob("upload-job") - err := am.RecordUpload(&ArtifactUpload{ - Name: "test-artifact", - Paths: []string{"/tmp/file.txt"}, - JobName: "upload-job", - }) - require.NoError(t, err) - - err = am.RecordUpload(&ArtifactUpload{ - Name: "build-linux", - Paths: []string{"/tmp/linux.exe"}, - JobName: "upload-job", - }) - require.NoError(t, err) - - err = am.RecordUpload(&ArtifactUpload{ - Name: "build-windows", - Paths: []string{"/tmp/windows.exe"}, - JobName: "upload-job", - }) - require.NoError(t, err) - - tests := []struct { - name string - download *ArtifactDownload - wantError bool - errorMsg string - }{ - { - name: "valid download by name", - download: &ArtifactDownload{ - Name: "test-artifact", - Path: "/tmp/download", - JobName: "download-job", - DependsOn: []string{"upload-job"}, - }, - wantError: false, - }, - { - name: "invalid download - artifact not found", - download: &ArtifactDownload{ - Name: "nonexistent-artifact", - Path: "/tmp/download", - JobName: "download-job", - DependsOn: []string{"upload-job"}, - }, - wantError: true, - errorMsg: "not found in any dependent job", - }, - { - name: "valid download by pattern", - download: &ArtifactDownload{ - Pattern: "build-*", - Path: "/tmp/builds", - JobName: "download-job", - DependsOn: []string{"upload-job"}, - }, - wantError: false, - }, - { - name: "invalid download - pattern matches nothing", - download: &ArtifactDownload{ - Pattern: "logs-*", - Path: "/tmp/tests", - JobName: "download-job", - DependsOn: []string{"upload-job"}, - }, - wantError: true, - errorMsg: "no artifacts matching pattern", - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - err := am.ValidateDownload(tt.download) - - if tt.wantError { - require.Error(t, err) - assert.Contains(t, err.Error(), tt.errorMsg) - } else { - require.NoError(t, err) - } - }) - } -} - -func TestValidateAllDownloads(t *testing.T) { - am := NewArtifactManager() - - // Setup: create uploads - am.SetCurrentJob("upload-job") - err := am.RecordUpload(&ArtifactUpload{ - Name: "artifact-1", - Paths: []string{"/tmp/file1.txt"}, - JobName: "upload-job", - }) - require.NoError(t, err) - - // Setup: create downloads (some valid, some invalid) - am.SetCurrentJob("download-job") - err = am.RecordDownload(&ArtifactDownload{ - Name: "artifact-1", - Path: "/tmp/download1", - JobName: "download-job", - DependsOn: []string{"upload-job"}, - }) - require.NoError(t, err) - - err = am.RecordDownload(&ArtifactDownload{ - Name: "nonexistent", - Path: "/tmp/download2", - JobName: "download-job", - DependsOn: []string{"upload-job"}, - }) - require.NoError(t, err) - - // Validate all downloads - errors := am.ValidateAllDownloads() - - // Should have 1 error (nonexistent artifact) - assert.Len(t, errors, 1) - assert.Contains(t, errors[0].Error(), "nonexistent") - assert.Contains(t, errors[0].Error(), "not found") -} - -func TestMatchesPattern(t *testing.T) { - tests := []struct { - name string - pattern string - matches []string - noMatch []string - }{ - { - name: "exact match", - pattern: "artifact", - matches: []string{"artifact"}, - noMatch: []string{"artifact-1", "test-artifact", "other"}, - }, - { - name: "leading wildcard", - pattern: "*-artifact", - matches: []string{"test-artifact", "my-artifact"}, - noMatch: []string{"artifact", "artifact-test"}, - }, - { - name: "trailing wildcard", - pattern: "build-*", - matches: []string{"build-linux", "build-windows", "build-"}, - noMatch: []string{"build", "test-build-linux"}, - }, - { - name: "middle wildcard", - pattern: "build-*-x64", - matches: []string{"build-linux-x64", "build-windows-x64"}, - noMatch: []string{"build-x64", "build-linux-arm64"}, - }, - { - name: "wildcard matches all", - pattern: "*", - matches: []string{"anything", "test", "build-linux"}, - noMatch: []string{}, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - for _, name := range tt.matches { - assert.True(t, matchesPattern(name, tt.pattern), - "Expected %s to match pattern %s", name, tt.pattern) - } - for _, name := range tt.noMatch { - assert.False(t, matchesPattern(name, tt.pattern), - "Expected %s NOT to match pattern %s", name, tt.pattern) - } - }) - } -} - -func TestReset(t *testing.T) { - am := NewArtifactManager() - - // Add some data - am.SetCurrentJob("test-job") - err := am.RecordUpload(&ArtifactUpload{ - Name: "test-artifact", - Paths: []string{"/tmp/file.txt"}, - JobName: "test-job", - }) - require.NoError(t, err) - - err = am.RecordDownload(&ArtifactDownload{ - Name: "test-artifact", - Path: "/tmp/download", - JobName: "test-job", - }) - require.NoError(t, err) - - // Verify data exists - assert.Len(t, am.uploads, 1) - assert.Len(t, am.downloads, 1) - assert.Equal(t, "test-job", am.currentJob) - - // Reset - am.Reset() - - // Verify everything is cleared - assert.Empty(t, am.uploads) - assert.Empty(t, am.downloads) - assert.Empty(t, am.currentJob) -} - -func TestComplexWorkflowScenario(t *testing.T) { - am := NewArtifactManager() - - // Job 1: Upload agent artifacts - am.SetCurrentJob("agent") - err := am.RecordUpload(&ArtifactUpload{ - Name: "agent-artifacts", - Paths: []string{"/tmp/gh-aw/aw-prompts/prompt.txt", "/tmp/gh-aw/patch/aw.patch"}, - JobName: "agent", - }) - require.NoError(t, err) - - // Job 2: Download agent artifacts for safe outputs - am.SetCurrentJob("safe_outputs") - err = am.RecordDownload(&ArtifactDownload{ - Name: "agent-artifacts", - Path: "/tmp/gh-aw/", - JobName: "safe_outputs", - DependsOn: []string{"agent"}, - }) - require.NoError(t, err) - - // Validate downloads - errors := am.ValidateAllDownloads() - assert.Empty(t, errors, "Expected no validation errors") - - // Test path computation - download := am.GetDownloadsForJob("safe_outputs")[0] - upload := am.FindUploadedArtifact("agent-artifacts", []string{"agent"}) - require.NotNil(t, upload) - - // Files should be extracted directly to download path (v4 behavior) - promptPath := am.ComputeDownloadPath(download, upload, "aw-prompts/prompt.txt") - assert.Equal(t, "/tmp/gh-aw/aw-prompts/prompt.txt", promptPath) - - patchPath := am.ComputeDownloadPath(download, upload, "patch/aw.patch") - assert.Equal(t, "/tmp/gh-aw/patch/aw.patch", patchPath) -} - -func TestMultipleArtifactsPatternDownload(t *testing.T) { - am := NewArtifactManager() - - // Job 1: Upload multiple build artifacts - am.SetCurrentJob("build") - for _, platform := range []string{"linux", "windows", "macos"} { - err := am.RecordUpload(&ArtifactUpload{ - Name: "build-" + platform, - Paths: []string{"/build/" + platform + "/app"}, - JobName: "build", - }) - require.NoError(t, err) - } - - // Job 2: Download all build artifacts with pattern (no merge) - am.SetCurrentJob("test") - err := am.RecordDownload(&ArtifactDownload{ - Pattern: "build-*", - Path: "/tmp/artifacts", - MergeMultiple: false, - JobName: "test", - DependsOn: []string{"build"}, - }) - require.NoError(t, err) - - // Validate - errors := am.ValidateAllDownloads() - assert.Empty(t, errors) - - // Test path computation for each artifact - download := am.GetDownloadsForJob("test")[0] - - linuxUpload := am.FindUploadedArtifact("build-linux", []string{"build"}) - require.NotNil(t, linuxUpload) - linuxPath := am.ComputeDownloadPath(download, linuxUpload, "linux/app") - assert.Equal(t, "/tmp/artifacts/build-linux/linux/app", linuxPath) - - windowsUpload := am.FindUploadedArtifact("build-windows", []string{"build"}) - require.NotNil(t, windowsUpload) - windowsPath := am.ComputeDownloadPath(download, windowsUpload, "windows/app") - assert.Equal(t, "/tmp/artifacts/build-windows/windows/app", windowsPath) -} - -func TestPatternDownloadWithMerge(t *testing.T) { - am := NewArtifactManager() - - // Upload multiple artifacts - am.SetCurrentJob("job1") - err := am.RecordUpload(&ArtifactUpload{ - Name: "logs-part1", - Paths: []string{"/logs/part1.txt"}, - JobName: "job1", - }) - require.NoError(t, err) - - err = am.RecordUpload(&ArtifactUpload{ - Name: "logs-part2", - Paths: []string{"/logs/part2.txt"}, - JobName: "job1", - }) - require.NoError(t, err) - - // Download with merge - am.SetCurrentJob("job2") - err = am.RecordDownload(&ArtifactDownload{ - Pattern: "logs-*", - Path: "/tmp/all-logs", - MergeMultiple: true, - JobName: "job2", - DependsOn: []string{"job1"}, - }) - require.NoError(t, err) - - // Validate - errors := am.ValidateAllDownloads() - assert.Empty(t, errors) - - // With merge, files go directly to path (no artifact subdirectories) - download := am.GetDownloadsForJob("job2")[0] - - part1Upload := am.FindUploadedArtifact("logs-part1", []string{"job1"}) - require.NotNil(t, part1Upload) - part1Path := am.ComputeDownloadPath(download, part1Upload, "part1.txt") - assert.Equal(t, "/tmp/all-logs/part1.txt", part1Path) - - part2Upload := am.FindUploadedArtifact("logs-part2", []string{"job1"}) - require.NotNil(t, part2Upload) - part2Path := am.ComputeDownloadPath(download, part2Upload, "part2.txt") - assert.Equal(t, "/tmp/all-logs/part2.txt", part2Path) -} - -// TestCommonParentStripping tests that common parent directories are stripped -// when multiple files are uploaded, simulating GitHub Actions behavior -func TestCommonParentStripping(t *testing.T) { - am := NewArtifactManager() - am.SetCurrentJob("upload-job") - - // Upload files with common parent /tmp/gh-aw/ - err := am.RecordUpload(&ArtifactUpload{ - Name: "test-artifact", - Paths: []string{ - "/tmp/gh-aw/aw-prompts/prompt.txt", - "/tmp/gh-aw/aw.patch", - }, - JobName: "upload-job", - }) - require.NoError(t, err) - - uploads := am.GetUploadsForJob("upload-job") - require.Len(t, uploads, 1) - upload := uploads[0] - - // Verify normalized paths have common parent stripped - assert.NotNil(t, upload.NormalizedPaths) - assert.Equal(t, "aw-prompts/prompt.txt", upload.NormalizedPaths["/tmp/gh-aw/aw-prompts/prompt.txt"]) - assert.Equal(t, "aw.patch", upload.NormalizedPaths["/tmp/gh-aw/aw.patch"]) - - // Verify download paths use normalized paths - am.SetCurrentJob("download-job") - download := &ArtifactDownload{ - Name: "test-artifact", - Path: "/workspace", - JobName: "download-job", - DependsOn: []string{"upload-job"}, - } - - // Download should use the normalized paths (with common parent stripped) - promptPath := am.ComputeDownloadPath(download, upload, "/tmp/gh-aw/aw-prompts/prompt.txt") - assert.Equal(t, "/workspace/aw-prompts/prompt.txt", promptPath) - - patchPath := am.ComputeDownloadPath(download, upload, "/tmp/gh-aw/aw.patch") - assert.Equal(t, "/workspace/aw.patch", patchPath) -} - -// TestCommonParentStrippingNestedPaths tests common parent stripping with nested paths -func TestCommonParentStrippingNestedPaths(t *testing.T) { - am := NewArtifactManager() - am.SetCurrentJob("build") - - // Upload files with deeper nesting - err := am.RecordUpload(&ArtifactUpload{ - Name: "build-outputs", - Paths: []string{ - "/home/runner/work/project/dist/app.js", - "/home/runner/work/project/dist/styles.css", - "/home/runner/work/project/dist/assets/logo.png", - }, - JobName: "build", - }) - require.NoError(t, err) - - upload := am.GetUploadsForJob("build")[0] - - // Common parent should be /home/runner/work/project/dist - assert.NotNil(t, upload.NormalizedPaths) - assert.Equal(t, "app.js", upload.NormalizedPaths["/home/runner/work/project/dist/app.js"]) - assert.Equal(t, "styles.css", upload.NormalizedPaths["/home/runner/work/project/dist/styles.css"]) - assert.Equal(t, "assets/logo.png", upload.NormalizedPaths["/home/runner/work/project/dist/assets/logo.png"]) -} - -// TestCommonParentStrippingSingleFile tests that single file uploads work correctly -func TestCommonParentStrippingSingleFile(t *testing.T) { - am := NewArtifactManager() - am.SetCurrentJob("job1") - - // Upload single file - err := am.RecordUpload(&ArtifactUpload{ - Name: "single-file", - Paths: []string{"/tmp/gh-aw/report.pdf"}, - JobName: "job1", - }) - require.NoError(t, err) - - upload := am.GetUploadsForJob("job1")[0] - - // Single file should be normalized to just its base name - assert.NotNil(t, upload.NormalizedPaths) - assert.Equal(t, "report.pdf", upload.NormalizedPaths["/tmp/gh-aw/report.pdf"]) - - // Download should use the normalized path - download := &ArtifactDownload{ - Name: "single-file", - Path: "/downloads", - JobName: "job2", - DependsOn: []string{"job1"}, - } - - path := am.ComputeDownloadPath(download, upload, "/tmp/gh-aw/report.pdf") - assert.Equal(t, "/downloads/report.pdf", path) -} - -// TestCommonParentStrippingNoCommonParent tests files with no common parent -func TestCommonParentStrippingNoCommonParent(t *testing.T) { - am := NewArtifactManager() - am.SetCurrentJob("job1") - - // Upload files from completely different paths - err := am.RecordUpload(&ArtifactUpload{ - Name: "mixed-files", - Paths: []string{ - "/tmp/file1.txt", - "/var/file2.txt", - }, - JobName: "job1", - }) - require.NoError(t, err) - - upload := am.GetUploadsForJob("job1")[0] - - // No common parent (beyond root), should use base names - assert.NotNil(t, upload.NormalizedPaths) - assert.Equal(t, "file1.txt", upload.NormalizedPaths["/tmp/file1.txt"]) - assert.Equal(t, "file2.txt", upload.NormalizedPaths["/var/file2.txt"]) -} - -// TestCommonParentWithPatternDownload tests common parent stripping with pattern downloads -func TestCommonParentWithPatternDownload(t *testing.T) { - am := NewArtifactManager() - - // Job 1: Upload with common parent - am.SetCurrentJob("build") - err := am.RecordUpload(&ArtifactUpload{ - Name: "build-linux", - Paths: []string{ - "/build/output/linux/app", - "/build/output/linux/lib.so", - }, - JobName: "build", - }) - require.NoError(t, err) - - // Job 2: Download with pattern - am.SetCurrentJob("deploy") - download := &ArtifactDownload{ - Pattern: "build-*", - Path: "/deploy", - MergeMultiple: false, - JobName: "deploy", - DependsOn: []string{"build"}, - } - - upload := am.GetUploadsForJob("build")[0] - - // With pattern download (no merge), files go to path/artifact-name/normalized-path - appPath := am.ComputeDownloadPath(download, upload, "/build/output/linux/app") - assert.Equal(t, "/deploy/build-linux/app", appPath) - - libPath := am.ComputeDownloadPath(download, upload, "/build/output/linux/lib.so") - assert.Equal(t, "/deploy/build-linux/lib.so", libPath) -} diff --git a/pkg/workflow/artifact_manager_workflows_integration_test.go b/pkg/workflow/artifact_manager_workflows_integration_test.go deleted file mode 100644 index 1d2964cd9b5..00000000000 --- a/pkg/workflow/artifact_manager_workflows_integration_test.go +++ /dev/null @@ -1,566 +0,0 @@ -//go:build integration - -package workflow - -import ( - "fmt" - "os" - "path/filepath" - "sort" - "strings" - "testing" - - "github.com/github/gh-aw/pkg/stringutil" - - "github.com/goccy/go-yaml" - "github.com/stretchr/testify/require" -) - -// JobArtifacts holds upload and download information for a job -type JobArtifacts struct { - Uploads []*ArtifactUpload - Downloads []*ArtifactDownload -} - -// TestGenerateArtifactsReference compiles all agentic workflows and generates -// a reference document mapping artifacts to file paths per job. -// This document is meant to be used by agents to generate file paths in JavaScript and Go. -func TestGenerateArtifactsReference(t *testing.T) { - // Find all workflow markdown files - workflowsDir := filepath.Join("..", "..", ".github", "workflows") - entries, err := os.ReadDir(workflowsDir) - require.NoError(t, err, "Failed to read workflows directory") - - // Collect workflow files (exclude campaign files and lock files) - var workflowFiles []string - for _, entry := range entries { - if entry.IsDir() { - continue - } - name := entry.Name() - if strings.HasSuffix(name, ".md") && - !strings.HasSuffix(name, ".lock.yml") && - !strings.Contains(name, ".campaign.") { - workflowFiles = append(workflowFiles, filepath.Join(workflowsDir, name)) - } - } - - t.Logf("Found %d workflow files to process", len(workflowFiles)) - - // Map to store artifacts per workflow - workflowArtifacts := make(map[string]map[string]*JobArtifacts) // workflow -> job -> artifacts - - // Compile each workflow and extract artifact information - // Use dry-run mode (noEmit) so we don't write lock.yml files - compiler := NewCompiler() - compiler.SetNoEmit(true) // Enable dry-run mode - validate without generating lock files - successCount := 0 - - for _, workflowPath := range workflowFiles { - workflowName := filepath.Base(workflowPath) - - // Parse the workflow - workflowData, err := compiler.ParseWorkflowFile(workflowPath) - if err != nil { - t.Logf("Warning: Failed to parse %s: %v", workflowName, err) - continue - } - - // Try to compile the workflow - err = compiler.CompileWorkflowData(workflowData, workflowPath) - if err != nil { - // Some workflows may fail compilation for various reasons (missing permissions, etc.) - // We'll skip these for the artifact analysis - t.Logf("Warning: Failed to compile %s: %v", workflowName, err) - continue - } - - // Read the compiled lock file to extract artifact information - lockPath := stringutil.MarkdownToLockFile(workflowPath) - lockContent, err := os.ReadFile(lockPath) - if err != nil { - t.Logf("Warning: Failed to read lock file for %s: %v", workflowName, err) - continue - } - - // Parse the lock file to extract artifact steps - jobs := extractArtifactsFromYAML(string(lockContent), workflowName, t) - - if len(jobs) > 0 { - workflowArtifacts[workflowName] = jobs - successCount++ - } - } - - t.Logf("Successfully analyzed %d workflows with artifacts", successCount) - - // Build a global summary of artifacts by job name - artifactsByJob := buildArtifactsSummary(workflowArtifacts) - - // Generate the markdown reference document - markdown := generateArtifactsMarkdown(workflowArtifacts, artifactsByJob) - - // Write to scratchpad/artifacts.md - specsDir := filepath.Join("..", "..", "specs") - err = os.MkdirAll(specsDir, 0755) - require.NoError(t, err, "Failed to create specs directory") - - artifactsPath := filepath.Join(specsDir, "artifacts.md") - err = os.WriteFile(artifactsPath, []byte(markdown), 0644) - require.NoError(t, err, "Failed to write artifacts.md") - - t.Logf("Generated artifacts reference at %s", artifactsPath) -} - -// extractArtifactsFromYAML parses compiled YAML and extracts artifact upload/download information -func extractArtifactsFromYAML(yamlContent string, workflowName string, t *testing.T) map[string]*JobArtifacts { - // Parse YAML - var workflow map[string]interface{} - err := yaml.Unmarshal([]byte(yamlContent), &workflow) - if err != nil { - t.Logf("Warning: Failed to parse YAML for %s: %v", workflowName, err) - return nil - } - - // Get jobs - jobsRaw, ok := workflow["jobs"].(map[string]interface{}) - if !ok { - return nil - } - - result := make(map[string]*JobArtifacts) - - for jobName, jobDataRaw := range jobsRaw { - jobData, ok := jobDataRaw.(map[string]interface{}) - if !ok { - continue - } - - steps, ok := jobData["steps"].([]interface{}) - if !ok { - continue - } - - jobArtifacts := &JobArtifacts{} - hasArtifacts := false - - for _, stepRaw := range steps { - step, ok := stepRaw.(map[string]interface{}) - if !ok { - continue - } - - uses, ok := step["uses"].(string) - if !ok { - continue - } - - // Check for upload-artifact - if strings.Contains(uses, "actions/upload-artifact@") { - upload := &ArtifactUpload{ - JobName: jobName, - } - - // Extract 'with' parameters - withParams, ok := step["with"].(map[string]interface{}) - if ok { - if name, ok := withParams["name"].(string); ok { - upload.Name = name - } - // Handle path which could be a string or multiline string - if pathStr, ok := withParams["path"].(string); ok { - // Split by newlines and trim whitespace - lines := strings.Split(pathStr, "\n") - for _, line := range lines { - line = strings.TrimSpace(line) - if line != "" { - upload.Paths = append(upload.Paths, line) - } - } - } - } - - if upload.Name != "" { - jobArtifacts.Uploads = append(jobArtifacts.Uploads, upload) - hasArtifacts = true - } - } - - // Check for download-artifact - if strings.Contains(uses, "actions/download-artifact@") { - download := &ArtifactDownload{ - JobName: jobName, - } - - // Extract 'with' parameters - withParams, ok := step["with"].(map[string]interface{}) - if ok { - if name, ok := withParams["name"].(string); ok { - download.Name = name - } - if pattern, ok := withParams["pattern"].(string); ok { - download.Pattern = pattern - } - if pathStr, ok := withParams["path"].(string); ok { - download.Path = pathStr - } - if merge, ok := withParams["merge-multiple"].(bool); ok { - download.MergeMultiple = merge - } - } - - // Try to infer dependencies from job needs - if needs, ok := jobData["needs"].([]interface{}); ok { - for _, need := range needs { - if needStr, ok := need.(string); ok { - download.DependsOn = append(download.DependsOn, needStr) - } - } - } else if needStr, ok := jobData["needs"].(string); ok { - download.DependsOn = []string{needStr} - } - - if download.Name != "" || download.Pattern != "" { - jobArtifacts.Downloads = append(jobArtifacts.Downloads, download) - hasArtifacts = true - } - } - } - - if hasArtifacts { - result[jobName] = jobArtifacts - } - } - - return result -} - -// ArtifactSummary holds merged artifact information for a job across all workflows -type ArtifactSummary struct { - JobName string - Uploads map[string]*ArtifactUploadInfo // artifact name -> upload info - Downloads map[string]*ArtifactDownloadInfo // artifact name/pattern -> download info -} - -// ArtifactUploadInfo holds merged upload information -type ArtifactUploadInfo struct { - ArtifactName string - Paths map[string]bool // unique paths across all workflows - Workflows []string // workflows that upload this artifact -} - -// ArtifactDownloadInfo holds merged download information -type ArtifactDownloadInfo struct { - Identifier string // artifact name or pattern - DownloadPaths map[string]bool // unique download paths - Workflows []string // workflows that download this - MergeMultiple bool -} - -// buildArtifactsSummary creates a summary of artifacts by job name, merging duplicates -func buildArtifactsSummary(workflowArtifacts map[string]map[string]*JobArtifacts) map[string]*ArtifactSummary { - summary := make(map[string]*ArtifactSummary) - - for workflowName, jobs := range workflowArtifacts { - for jobName, artifacts := range jobs { - // Get or create job summary - if summary[jobName] == nil { - summary[jobName] = &ArtifactSummary{ - JobName: jobName, - Uploads: make(map[string]*ArtifactUploadInfo), - Downloads: make(map[string]*ArtifactDownloadInfo), - } - } - jobSummary := summary[jobName] - - // Merge uploads - for _, upload := range artifacts.Uploads { - if upload.Name == "" { - continue - } - - if jobSummary.Uploads[upload.Name] == nil { - jobSummary.Uploads[upload.Name] = &ArtifactUploadInfo{ - ArtifactName: upload.Name, - Paths: make(map[string]bool), - Workflows: []string{}, - } - } - uploadInfo := jobSummary.Uploads[upload.Name] - - // Add paths - for _, path := range upload.Paths { - uploadInfo.Paths[path] = true - } - - // Add workflow if not already present - if !artifactContainsWorkflow(uploadInfo.Workflows, workflowName) { - uploadInfo.Workflows = append(uploadInfo.Workflows, workflowName) - } - } - - // Merge downloads - for _, download := range artifacts.Downloads { - identifier := download.Name - if identifier == "" { - identifier = download.Pattern - } - if identifier == "" { - continue - } - - if jobSummary.Downloads[identifier] == nil { - jobSummary.Downloads[identifier] = &ArtifactDownloadInfo{ - Identifier: identifier, - DownloadPaths: make(map[string]bool), - Workflows: []string{}, - MergeMultiple: download.MergeMultiple, - } - } - downloadInfo := jobSummary.Downloads[identifier] - - // Add download path - if download.Path != "" { - downloadInfo.DownloadPaths[download.Path] = true - } - - // Add workflow if not already present - if !artifactContainsWorkflow(downloadInfo.Workflows, workflowName) { - downloadInfo.Workflows = append(downloadInfo.Workflows, workflowName) - } - } - } - } - - return summary -} - -// artifactContainsWorkflow checks if a string slice contains a value -func artifactContainsWorkflow(slice []string, value string) bool { - for _, item := range slice { - if item == value { - return true - } - } - return false -} - -// generateArtifactsMarkdown generates a markdown document with artifact information -func generateArtifactsMarkdown(workflowArtifacts map[string]map[string]*JobArtifacts, artifactsByJob map[string]*ArtifactSummary) string { - var sb strings.Builder - - sb.WriteString("\n\n") - sb.WriteString("# Artifact File Locations Reference\n\n") - sb.WriteString("This document provides a reference for artifact file locations across all agentic workflows.\n") - sb.WriteString("It is generated automatically and meant to be used by agents when generating file paths in JavaScript and Go code.\n\n") - sb.WriteString("## Overview\n\n") - sb.WriteString("When artifacts are uploaded, GitHub Actions strips the common parent directory from file paths.\n") - sb.WriteString("When artifacts are downloaded, files are extracted based on the download mode:\n\n") - sb.WriteString("- **Download by name**: Files extracted directly to `path/` (e.g., `path/file.txt`)\n") - sb.WriteString("- **Download by pattern (no merge)**: Files in `path/artifact-name/` (e.g., `path/artifact-1/file.txt`)\n") - sb.WriteString("- **Download by pattern (merge)**: Files extracted directly to `path/` (e.g., `path/file.txt`)\n\n") - - // Add summary section - sb.WriteString("## Summary by Job\n\n") - sb.WriteString("This section provides an overview of artifacts organized by job name, with duplicates merged across workflows.\n\n") - - // Sort job names for consistent output - jobNames := make([]string, 0, len(artifactsByJob)) - for jobName := range artifactsByJob { - jobNames = append(jobNames, jobName) - } - sort.Strings(jobNames) - - for _, jobName := range jobNames { - summary := artifactsByJob[jobName] - - fmt.Fprintf(&sb, "### Job: `%s`\n\n", jobName) - - // Uploads summary - if len(summary.Uploads) > 0 { - sb.WriteString("**Artifacts Uploaded:**\n\n") - - // Sort artifact names - uploadNames := make([]string, 0, len(summary.Uploads)) - for name := range summary.Uploads { - uploadNames = append(uploadNames, name) - } - sort.Strings(uploadNames) - - for _, name := range uploadNames { - info := summary.Uploads[name] - fmt.Fprintf(&sb, "- `%s`\n", info.ArtifactName) - - // Sort and list paths - paths := make([]string, 0, len(info.Paths)) - for path := range info.Paths { - paths = append(paths, path) - } - sort.Strings(paths) - - sb.WriteString(" - **Paths**: ") - for i, path := range paths { - if i > 0 { - sb.WriteString(", ") - } - fmt.Fprintf(&sb, "`%s`", path) - } - sb.WriteString("\n") - - // Sort and list workflows - sort.Strings(info.Workflows) - fmt.Fprintf(&sb, " - **Used in**: %d workflow(s) - %s\n", len(info.Workflows), strings.Join(info.Workflows, ", ")) - } - sb.WriteString("\n") - } - - // Downloads summary - if len(summary.Downloads) > 0 { - sb.WriteString("**Artifacts Downloaded:**\n\n") - - // Sort identifiers - downloadIds := make([]string, 0, len(summary.Downloads)) - for id := range summary.Downloads { - downloadIds = append(downloadIds, id) - } - sort.Strings(downloadIds) - - for _, id := range downloadIds { - info := summary.Downloads[id] - fmt.Fprintf(&sb, "- `%s`\n", info.Identifier) - - // Sort and list download paths - paths := make([]string, 0, len(info.DownloadPaths)) - for path := range info.DownloadPaths { - paths = append(paths, path) - } - sort.Strings(paths) - - sb.WriteString(" - **Download paths**: ") - for i, path := range paths { - if i > 0 { - sb.WriteString(", ") - } - fmt.Fprintf(&sb, "`%s`", path) - } - sb.WriteString("\n") - - // Sort and list workflows - sort.Strings(info.Workflows) - fmt.Fprintf(&sb, " - **Used in**: %d workflow(s) - %s\n", len(info.Workflows), strings.Join(info.Workflows, ", ")) - } - sb.WriteString("\n") - } - } - - sb.WriteString("## Workflows\n\n") - - // Sort workflow names for consistent output - workflowNames := make([]string, 0, len(workflowArtifacts)) - for name := range workflowArtifacts { - workflowNames = append(workflowNames, name) - } - sort.Strings(workflowNames) - - for _, workflowName := range workflowNames { - jobs := workflowArtifacts[workflowName] - - fmt.Fprintf(&sb, "### %s\n\n", workflowName) - - // Sort job names - jobNames := make([]string, 0, len(jobs)) - for jobName := range jobs { - jobNames = append(jobNames, jobName) - } - sort.Strings(jobNames) - - for _, jobName := range jobNames { - artifacts := jobs[jobName] - - fmt.Fprintf(&sb, "#### Job: `%s`\n\n", jobName) - - // Uploads - if len(artifacts.Uploads) > 0 { - sb.WriteString("**Uploads:**\n\n") - for _, upload := range artifacts.Uploads { - fmt.Fprintf(&sb, "- **Artifact**: `%s`\n", upload.Name) - sb.WriteString(" - **Upload paths**:\n") - for _, path := range upload.Paths { - fmt.Fprintf(&sb, " - `%s`\n", path) - } - - if len(upload.NormalizedPaths) > 0 { - sb.WriteString(" - **Paths in artifact** (after common parent stripping):\n") - - // Sort normalized paths for consistent output - var normalizedKeys []string - for key := range upload.NormalizedPaths { - normalizedKeys = append(normalizedKeys, key) - } - sort.Strings(normalizedKeys) - - for _, key := range normalizedKeys { - normalizedPath := upload.NormalizedPaths[key] - fmt.Fprintf(&sb, " - `%s` → `%s`\n", key, normalizedPath) - } - } - sb.WriteString("\n") - } - } - - // Downloads - if len(artifacts.Downloads) > 0 { - sb.WriteString("**Downloads:**\n\n") - for _, download := range artifacts.Downloads { - if download.Name != "" { - fmt.Fprintf(&sb, "- **Artifact**: `%s` (by name)\n", download.Name) - } else if download.Pattern != "" { - fmt.Fprintf(&sb, "- **Pattern**: `%s`", download.Pattern) - if download.MergeMultiple { - sb.WriteString(" (merge-multiple: true)\n") - } else { - sb.WriteString(" (merge-multiple: false)\n") - } - } - fmt.Fprintf(&sb, " - **Download path**: `%s`\n", download.Path) - if len(download.DependsOn) > 0 { - fmt.Fprintf(&sb, " - **Depends on jobs**: %v\n", download.DependsOn) - } - sb.WriteString("\n") - } - } - } - } - - sb.WriteString("## Usage Examples\n\n") - sb.WriteString("### JavaScript (actions/github-script)\n\n") - sb.WriteString("```javascript\n") - sb.WriteString("// Reading a file from a downloaded artifact\n") - sb.WriteString("const fs = require('fs');\n") - sb.WriteString("const path = require('path');\n\n") - sb.WriteString("// If artifact 'build-output' was downloaded to '/tmp/artifacts'\n") - sb.WriteString("// and contains 'dist/app.js' (after common parent stripping)\n") - sb.WriteString("const filePath = path.join('/tmp/artifacts', 'dist', 'app.js');\n") - sb.WriteString("const content = fs.readFileSync(filePath, 'utf8');\n") - sb.WriteString("```\n\n") - sb.WriteString("### Go\n\n") - sb.WriteString("```go\n") - sb.WriteString("// Reading a file from a downloaded artifact\n") - sb.WriteString("import (\n") - sb.WriteString(" \"os\"\n") - sb.WriteString(" \"path/filepath\"\n") - sb.WriteString(")\n\n") - sb.WriteString("// If artifact 'build-output' was downloaded to '/tmp/artifacts'\n") - sb.WriteString("// and contains 'dist/app.js' (after common parent stripping)\n") - sb.WriteString("filePath := filepath.Join(\"/tmp/artifacts\", \"dist\", \"app.js\")\n") - sb.WriteString("content, err := os.ReadFile(filePath)\n") - sb.WriteString("```\n\n") - sb.WriteString("## Notes\n\n") - sb.WriteString("- This document is auto-generated from workflow analysis\n") - sb.WriteString("- Actual file paths may vary based on the workflow execution context\n") - sb.WriteString("- Always verify file existence before reading in production code\n") - sb.WriteString("- Common parent directories are automatically stripped during upload\n") - sb.WriteString("- Use `ComputeDownloadPath()` from the artifact manager for accurate path computation\n") - - return sb.String() -} diff --git a/pkg/workflow/checkout_manager.go b/pkg/workflow/checkout_manager.go index 58e459bdb73..2ee90e97972 100644 --- a/pkg/workflow/checkout_manager.go +++ b/pkg/workflow/checkout_manager.go @@ -154,11 +154,6 @@ func (cm *CheckoutManager) add(cfg *CheckoutConfig) { } } -// HasUserCheckouts returns true if any user-supplied checkouts were registered. -func (cm *CheckoutManager) HasUserCheckouts() bool { - return len(cm.ordered) > 0 -} - // GetDefaultCheckoutOverride returns the resolved checkout for the default workspace // (empty path, empty repository). Returns nil if the user did not configure one. func (cm *CheckoutManager) GetDefaultCheckoutOverride() *resolvedCheckout { diff --git a/pkg/workflow/checkout_manager_test.go b/pkg/workflow/checkout_manager_test.go index 11af372dca6..a142f052f89 100644 --- a/pkg/workflow/checkout_manager_test.go +++ b/pkg/workflow/checkout_manager_test.go @@ -14,7 +14,7 @@ import ( func TestNewCheckoutManager(t *testing.T) { t.Run("empty configs produces empty manager", func(t *testing.T) { cm := NewCheckoutManager(nil) - assert.False(t, cm.HasUserCheckouts(), "empty manager should report no user checkouts") + // HasUserCheckouts removed (dead code) assert.Nil(t, cm.GetDefaultCheckoutOverride(), "empty manager should have no default override") }) @@ -23,7 +23,7 @@ func TestNewCheckoutManager(t *testing.T) { cm := NewCheckoutManager([]*CheckoutConfig{ {FetchDepth: &depth}, }) - assert.True(t, cm.HasUserCheckouts(), "should have user checkouts") + // HasUserCheckouts removed (dead code) override := cm.GetDefaultCheckoutOverride() require.NotNil(t, override, "should have default override") require.NotNil(t, override.fetchDepth, "fetch depth should be set") diff --git a/pkg/workflow/compiler_safe_outputs_config.go b/pkg/workflow/compiler_safe_outputs_config.go index d73eec00bc2..476ed54955f 100644 --- a/pkg/workflow/compiler_safe_outputs_config.go +++ b/pkg/workflow/compiler_safe_outputs_config.go @@ -75,14 +75,6 @@ func (b *handlerConfigBuilder) AddIfNotEmpty(key string, value string) *handlerC return b } -// AddIfTrue adds a boolean field only if the value is true -func (b *handlerConfigBuilder) AddIfTrue(key string, value bool) *handlerConfigBuilder { - if value { - b.config[key] = true - } - return b -} - // AddStringSlice adds a string slice field only if the slice is not empty func (b *handlerConfigBuilder) AddStringSlice(key string, value []string) *handlerConfigBuilder { if len(value) > 0 { From 5b55ade19581f1144ef2307e849120da33f7ff23 Mon Sep 17 00:00:00 2001 From: Don Syme Date: Sat, 28 Feb 2026 12:59:12 +0000 Subject: [PATCH 4/5] dead14: remove dead functions from parser package Delete dead parser functions and their exclusive tests: pkg/parser/include_processor.go: - Delete ProcessIncludes pkg/parser/include_expander.go: - Delete ExpandIncludes, ProcessIncludesForEngines, ProcessIncludesForSafeOutputs pkg/parser/schema_validation.go: - Delete ValidateMainWorkflowFrontmatterWithSchema - Delete ValidateIncludedFileFrontmatterWithSchema - Delete ValidateMCPConfigWithSchema pkg/parser/yaml_error.go: - Delete ExtractYAMLError, extractFromGoccyFormat, extractFromStringParsing Test cleanup (deleted whole files): - schema_oneof_test.go - schema_passthrough_validation_test.go - schema_additional_properties_test.go - frontmatter_syntax_errors_test.go - schema_validation_test.go Test cleanup (removed specific functions): - frontmatter_includes_test.go: 5 functions for ProcessIncludes/ExpandIncludes - schema_test.go: 3 functions for dead schema validators - schema_utilities_test.go: 2 functions for dead validators - yaml_error_test.go: 3 functions for ExtractYAMLError - import_syntax_test.go: TestProcessIncludesWithNewSyntax - frontmatter_benchmark_test.go: 2 benchmark functions 5129 lines removed across 15 files --- pkg/parser/frontmatter_benchmark_test.go | 88 - pkg/parser/frontmatter_includes_test.go | 571 ------ pkg/parser/frontmatter_syntax_errors_test.go | 691 ------- pkg/parser/import_syntax_test.go | 83 - pkg/parser/include_expander.go | 14 - pkg/parser/include_processor.go | 5 - .../schema_additional_properties_test.go | 283 --- pkg/parser/schema_oneof_test.go | 334 --- .../schema_passthrough_validation_test.go | 572 ------ pkg/parser/schema_test.go | 1804 ----------------- pkg/parser/schema_utilities_test.go | 138 -- pkg/parser/schema_validation.go | 54 - pkg/parser/schema_validation_test.go | 74 - pkg/parser/yaml_error.go | 146 -- pkg/parser/yaml_error_test.go | 272 --- 15 files changed, 5129 deletions(-) delete mode 100644 pkg/parser/frontmatter_syntax_errors_test.go delete mode 100644 pkg/parser/schema_additional_properties_test.go delete mode 100644 pkg/parser/schema_oneof_test.go delete mode 100644 pkg/parser/schema_passthrough_validation_test.go delete mode 100644 pkg/parser/schema_validation_test.go diff --git a/pkg/parser/frontmatter_benchmark_test.go b/pkg/parser/frontmatter_benchmark_test.go index 42bdbbb9e69..021b7cfe9c3 100644 --- a/pkg/parser/frontmatter_benchmark_test.go +++ b/pkg/parser/frontmatter_benchmark_test.go @@ -170,93 +170,5 @@ Workflow demonstrating array handling in frontmatter. } // BenchmarkValidateSchema benchmarks schema validation -func BenchmarkValidateSchema(b *testing.B) { - frontmatter := map[string]any{ - "on": "push", - "permissions": map[string]any{ - "contents": "read", - "issues": "write", - "pull-requests": "read", - }, - "engine": "claude", - "tools": map[string]any{ - "github": map[string]any{ - "allowed": []any{"issue_read", "add_issue_comment"}, - }, - "bash": []any{"echo", "ls"}, - }, - "timeout-minutes": 10, - } - - for b.Loop() { - _ = ValidateMainWorkflowFrontmatterWithSchema(frontmatter) - } -} // BenchmarkValidateSchema_Complex benchmarks schema validation with complex data -func BenchmarkValidateSchema_Complex(b *testing.B) { - frontmatter := map[string]any{ - "on": map[string]any{ - "pull_request": map[string]any{ - "types": []any{"opened", "synchronize", "reopened"}, - "forks": []any{"org/*", "user/repo"}, - }, - }, - "permissions": map[string]any{ - "contents": "read", - "issues": "write", - "pull-requests": "write", - "actions": "read", - }, - "engine": map[string]any{ - "id": "copilot", - "max-turns": 5, - "max-concurrency": 3, - "model": "gpt-5", - }, - "mcp-servers": map[string]any{ - "github": map[string]any{ - "mode": "remote", - "toolsets": []any{"default", "actions", "discussions"}, - "read-only": false, - }, - "playwright": map[string]any{ - "container": "mcr.microsoft.com/playwright:v1.41.0", - "allowed-domains": []any{"github.com", "*.github.io"}, - }, - }, - "network": map[string]any{ - "allowed": []any{"defaults", "python", "node"}, - "firewall": map[string]any{ - "version": "v1.0.0", - "log-level": "debug", - }, - }, - "tools": map[string]any{ - "edit": true, - "web-fetch": true, - "web-search": true, - "bash": []any{"git status", "git diff", "npm test"}, - }, - "safe-outputs": map[string]any{ - "create-pull-requests": map[string]any{ - "title-prefix": "[ai] ", - "labels": []any{"automation", "ai-generated"}, - "draft": true, - }, - "add-comments": map[string]any{ - "max": 3, - "target": "*", - }, - }, - "timeout-minutes": 30, - "concurrency": map[string]any{ - "group": "workflow-123", - "cancel-in-progress": true, - }, - } - - for b.Loop() { - _ = ValidateMainWorkflowFrontmatterWithSchema(frontmatter) - } -} diff --git a/pkg/parser/frontmatter_includes_test.go b/pkg/parser/frontmatter_includes_test.go index e6503fc90f5..4ec353b3e08 100644 --- a/pkg/parser/frontmatter_includes_test.go +++ b/pkg/parser/frontmatter_includes_test.go @@ -7,580 +7,9 @@ import ( "path/filepath" "strings" "testing" - - "github.com/github/gh-aw/pkg/constants" ) -func TestProcessIncludes(t *testing.T) { - // Create temporary test files - tempDir, err := os.MkdirTemp("", "test_includes") - if err != nil { - t.Fatalf("Failed to create temp dir: %v", err) - } - defer os.RemoveAll(tempDir) - - // Create test file with markdown content - testFile := filepath.Join(tempDir, "test.md") - testContent := `--- -tools: - bash: - allowed: ["ls", "cat"] ---- - -# Test Content -This is a test file content. -` - if err := os.WriteFile(testFile, []byte(testContent), 0644); err != nil { - t.Fatalf("Failed to write test file: %v", err) - } - - // Create test file with extra newlines for trimming test - testFileWithNewlines := filepath.Join(tempDir, "test-newlines.md") - testContentWithNewlines := ` - -# Content with Extra Newlines -Some content here. - - -` - if err := os.WriteFile(testFileWithNewlines, []byte(testContentWithNewlines), 0644); err != nil { - t.Fatalf("Failed to write test file with newlines: %v", err) - } - - tests := []struct { - name string - content string - baseDir string - extractTools bool - expected string - wantErr bool - }{ - { - name: "no includes", - content: "# Title\nRegular content", - baseDir: tempDir, - extractTools: false, - expected: "# Title\nRegular content\n", - }, - { - name: "simple include", - content: "@include test.md\n# After include", - baseDir: tempDir, - extractTools: false, - expected: "# Test Content\nThis is a test file content.\n# After include\n", - }, - { - name: "extract tools", - content: "@include test.md", - baseDir: tempDir, - extractTools: true, - expected: `{"bash":{"allowed":["ls","cat"]}}` + "\n", - }, - { - name: "file not found", - content: "@include nonexistent.md", - baseDir: tempDir, - extractTools: false, - wantErr: true, // Now expects error instead of embedding comment - }, - { - name: "include file with extra newlines", - content: "@include test-newlines.md\n# After include", - baseDir: tempDir, - extractTools: false, - expected: "# Content with Extra Newlines\nSome content here.\n# After include\n", - }, - { - name: "simple import (alias for include)", - content: "@import test.md\n# After import", - baseDir: tempDir, - extractTools: false, - expected: "# Test Content\nThis is a test file content.\n# After import\n", - }, - { - name: "extract tools with import", - content: "@import test.md", - baseDir: tempDir, - extractTools: true, - expected: `{"bash":{"allowed":["ls","cat"]}}` + "\n", - }, - { - name: "import file not found", - content: "@import nonexistent.md", - baseDir: tempDir, - extractTools: false, - wantErr: true, - }, - { - name: "optional import missing file", - content: "@import? missing.md\n", - baseDir: tempDir, - extractTools: false, - expected: "", - }, - } - - // Create test file with invalid frontmatter for testing validation - invalidFile := filepath.Join(tempDir, "invalid.md") - invalidContent := `--- -title: Invalid File -on: push -tools: - bash: - allowed: ["ls"] ---- - -# Invalid Content -This file has invalid frontmatter for an included file. -` - if err := os.WriteFile(invalidFile, []byte(invalidContent), 0644); err != nil { - t.Fatalf("Failed to write invalid test file: %v", err) - } - - // Add test case for invalid frontmatter in included file (should now pass with warnings for non-workflow files) - tests = append(tests, struct { - name string - content string - baseDir string - extractTools bool - expected string - wantErr bool - }{ - name: "invalid frontmatter in included file", - content: "@include invalid.md", - baseDir: tempDir, - extractTools: false, - expected: "# Invalid Content\nThis file has invalid frontmatter for an included file.\n", - wantErr: false, - }) - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - result, err := ProcessIncludes(tt.content, tt.baseDir, tt.extractTools) - - if tt.wantErr && err == nil { - t.Errorf("ProcessIncludes() expected error, got nil") - return - } - - if !tt.wantErr && err != nil { - t.Errorf("ProcessIncludes() error = %v", err) - return - } - - // Special handling for the invalid frontmatter test case - it should now pass with warnings - if tt.name == "invalid frontmatter in included file" { - // Check that the content was successfully included - if !strings.Contains(result, "# Invalid Content") { - t.Errorf("ProcessIncludes() = %q, expected to contain '# Invalid Content'", result) - } - return - } - - if result != tt.expected { - t.Errorf("ProcessIncludes() = %q, want %q", result, tt.expected) - } - }) - } -} - -func TestProcessIncludesConditionalValidation(t *testing.T) { - // Create temporary directory structure - tempDir, err := os.MkdirTemp("", "test_conditional_validation") - if err != nil { - t.Fatalf("Failed to create temp dir: %v", err) - } - defer os.RemoveAll(tempDir) - - // Create .github/workflows directory structure - workflowsDir := filepath.Join(tempDir, constants.GetWorkflowDir()) - if err := os.MkdirAll(workflowsDir, 0755); err != nil { - t.Fatalf("Failed to create workflows dir: %v", err) - } - - // Create docs directory for non-workflow files - docsDir := filepath.Join(tempDir, "docs") - if err := os.MkdirAll(docsDir, 0755); err != nil { - t.Fatalf("Failed to create docs dir: %v", err) - } - - // Test file 1: Valid workflow file (should pass strict validation) - validWorkflowFile := filepath.Join(workflowsDir, "valid.md") - validWorkflowContent := `--- -tools: - github: - allowed: [issue_read] ---- - -# Valid Workflow -This is a valid workflow file.` - if err := os.WriteFile(validWorkflowFile, []byte(validWorkflowContent), 0644); err != nil { - t.Fatalf("Failed to write valid workflow file: %v", err) - } - - // Test file 2: Invalid workflow file (should fail strict validation) - invalidWorkflowFile := filepath.Join(workflowsDir, "invalid.md") - invalidWorkflowContent := `--- -title: Invalid Field -on: push -tools: - github: - allowed: [issue_read] ---- - -# Invalid Workflow -This has invalid frontmatter fields.` - if err := os.WriteFile(invalidWorkflowFile, []byte(invalidWorkflowContent), 0644); err != nil { - t.Fatalf("Failed to write invalid workflow file: %v", err) - } - - // Test file 2.5: Invalid non-workflow file (should pass with warnings) - invalidNonWorkflowFile := filepath.Join(docsDir, "invalid-external.md") - invalidNonWorkflowContent := `--- -title: Invalid Field -on: push -tools: - github: - allowed: [issue_read] ---- - -# Invalid External File -This has invalid frontmatter fields but it's outside workflows dir.` - if err := os.WriteFile(invalidNonWorkflowFile, []byte(invalidNonWorkflowContent), 0644); err != nil { - t.Fatalf("Failed to write invalid non-workflow file: %v", err) - } - - // Test file 3: Agent instructions file (should pass with warnings) - agentFile := filepath.Join(docsDir, "agent-instructions.md") - agentContent := `--- -description: Agent instructions -applyTo: "**/*.py" -temperature: 0.7 -tools: - github: - allowed: [issue_read] ---- - -# Agent Instructions -These are instructions for AI agents.` - if err := os.WriteFile(agentFile, []byte(agentContent), 0644); err != nil { - t.Fatalf("Failed to write agent file: %v", err) - } - - // Test file 4: Plain markdown file (no frontmatter) - plainFile := filepath.Join(docsDir, "plain.md") - plainContent := `# Plain Markdown -This is just plain markdown content with no frontmatter.` - if err := os.WriteFile(plainFile, []byte(plainContent), 0644); err != nil { - t.Fatalf("Failed to write plain file: %v", err) - } - - tests := []struct { - name string - content string - baseDir string - extractTools bool - wantErr bool - checkContent string - }{ - { - name: "valid workflow file inclusion", - content: "@include .github/workflows/valid.md", - baseDir: tempDir, - extractTools: false, - wantErr: false, - checkContent: "# Valid Workflow", - }, - { - name: "invalid workflow file inclusion should fail", - content: "@include .github/workflows/invalid.md", - baseDir: tempDir, - extractTools: false, - wantErr: true, // Now expects error instead of embedding comment - }, - { - name: "invalid non-workflow file inclusion should succeed with warnings", - content: "@include docs/invalid-external.md", - baseDir: tempDir, - extractTools: false, - wantErr: false, - checkContent: "# Invalid External File", - }, - { - name: "agent instructions file inclusion should succeed", - content: "@include docs/agent-instructions.md", - baseDir: tempDir, - extractTools: false, - wantErr: false, - checkContent: "# Agent Instructions", - }, - { - name: "plain markdown file inclusion should succeed", - content: "@include docs/plain.md", - baseDir: tempDir, - extractTools: false, - wantErr: false, - checkContent: "# Plain Markdown", - }, - { - name: "extract tools from valid workflow file", - content: "@include .github/workflows/valid.md", - baseDir: tempDir, - extractTools: true, - wantErr: false, - checkContent: `{"github":{"allowed":["issue_read"]}}`, - }, - { - name: "extract tools from agent file", - content: "@include docs/agent-instructions.md", - baseDir: tempDir, - extractTools: true, - wantErr: false, - checkContent: `{"github":{"allowed":["issue_read"]}}`, - }, - { - name: "extract tools from plain file (no tools)", - content: "@include docs/plain.md", - baseDir: tempDir, - extractTools: true, - wantErr: false, - checkContent: `{}`, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - result, err := ProcessIncludes(tt.content, tt.baseDir, tt.extractTools) - - if tt.wantErr && err == nil { - t.Errorf("ProcessIncludes() expected error, got nil") - return - } - - if !tt.wantErr && err != nil { - t.Errorf("ProcessIncludes() error = %v", err) - return - } - - if !tt.wantErr && tt.checkContent != "" { - if !strings.Contains(result, tt.checkContent) { - t.Errorf("ProcessIncludes() result = %q, expected to contain %q", result, tt.checkContent) - } - } - }) - } -} - -func TestExpandIncludes(t *testing.T) { - // Create temporary test files - tempDir, err := os.MkdirTemp("", "test_expand") - if err != nil { - t.Fatalf("Failed to create temp dir: %v", err) - } - defer os.RemoveAll(tempDir) - - // Create go.mod to make it project root for component resolution - goModFile := filepath.Join(tempDir, "go.mod") - if err := os.WriteFile(goModFile, []byte("module test"), 0644); err != nil { - t.Fatalf("Failed to write go.mod: %v", err) - } - - // Create test file - testFile := filepath.Join(tempDir, "test.md") - testContent := `--- -tools: - bash: - allowed: ["ls"] ---- - -# Test Content -This is test content. -` - if err := os.WriteFile(testFile, []byte(testContent), 0644); err != nil { - t.Fatalf("Failed to write test file: %v", err) - } - - tests := []struct { - name string - content string - baseDir string - extractTools bool - wantContains string - wantErr bool - }{ - { - name: "expand markdown content", - content: "# Start\n@include test.md\n# End", - baseDir: tempDir, - extractTools: false, - wantContains: "# Test Content\nThis is test content.", - }, - { - name: "expand tools", - content: "@include test.md", - baseDir: tempDir, - extractTools: true, - wantContains: `"bash"`, - }, - { - name: "expand markdown content with import", - content: "# Start\n@import test.md\n# End", - baseDir: tempDir, - extractTools: false, - wantContains: "# Test Content\nThis is test content.", - }, - { - name: "expand tools with import", - content: "@import test.md", - baseDir: tempDir, - extractTools: true, - wantContains: `"bash"`, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - result, err := ExpandIncludes(tt.content, tt.baseDir, tt.extractTools) - - if tt.wantErr { - if err == nil { - t.Errorf("ExpandIncludes() expected error, got nil") - } - return - } - - if err != nil { - t.Errorf("ExpandIncludes() error = %v", err) - return - } - - if !strings.Contains(result, tt.wantContains) { - t.Errorf("ExpandIncludes() = %q, want to contain %q", result, tt.wantContains) - } - }) - } -} - // Test ExtractWorkflowNameFromMarkdown function -func TestProcessIncludesOptional(t *testing.T) { - // Create temporary directory structure - tempDir, err := os.MkdirTemp("", "test_optional_includes") - if err != nil { - t.Fatalf("Failed to create temp dir: %v", err) - } - defer os.RemoveAll(tempDir) - - // Create an existing include file - existingFile := filepath.Join(tempDir, "existing.md") - existingContent := "# Existing Include\nThis file exists." - if err := os.WriteFile(existingFile, []byte(existingContent), 0644); err != nil { - t.Fatalf("Failed to write existing file: %v", err) - } - - tests := []struct { - name string - content string - extractTools bool - expectedOutput string - expectError bool - }{ - { - name: "regular include existing file", - content: "@include existing.md\n", - extractTools: false, - expectedOutput: existingContent, - }, - { - name: "regular include missing file", - content: "@include missing.md\n", - extractTools: false, - expectError: true, // Now expects error instead of embedding comment - }, - { - name: "optional include existing file", - content: "@include? existing.md\n", - extractTools: false, - expectedOutput: existingContent, - }, - { - name: "optional include missing file", - content: "@include? missing.md\n", - extractTools: false, - expectedOutput: "", // No content added, friendly message goes to stdout - }, - { - name: "optional include missing file extract tools", - content: "@include? missing.md\n", - extractTools: true, - expectedOutput: "", - }, - { - name: "regular include missing file extract tools", - content: "@include missing.md\n", - extractTools: true, - expectError: true, // Now expects error instead of returning {} - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - result, err := ProcessIncludes(tt.content, tempDir, tt.extractTools) - - if tt.expectError { - if err == nil { - t.Errorf("ProcessIncludes expected error, got nil") - } - return - } - - if err != nil { - t.Errorf("ProcessIncludes unexpected error: %v", err) - return - } - - if !strings.Contains(result, tt.expectedOutput) { - t.Errorf("ProcessIncludes output = %q, expected to contain %q", result, tt.expectedOutput) - } - }) - } -} - -func TestProcessIncludesWithCycleDetection(t *testing.T) { - // Create a temporary directory for test files - tempDir, err := os.MkdirTemp("", "test_cycle_detection") - if err != nil { - t.Fatalf("Failed to create temp dir: %v", err) - } - defer os.RemoveAll(tempDir) - - // Create file A that includes file B - fileA := filepath.Join(tempDir, "fileA.md") - if err := os.WriteFile(fileA, []byte("# File A\n@include fileB.md\n"), 0644); err != nil { - t.Fatalf("Failed to write fileA: %v", err) - } - - // Create file B that includes file A (creating a cycle) - fileB := filepath.Join(tempDir, "fileB.md") - if err := os.WriteFile(fileB, []byte("# File B\n@include fileA.md\n"), 0644); err != nil { - t.Fatalf("Failed to write fileB: %v", err) - } - - // Process includes from file A - should not hang due to cycle detection - content := "# Main\n@include fileA.md\n" - result, err := ProcessIncludes(content, tempDir, false) - - if err != nil { - t.Errorf("ProcessIncludes with cycle should not error: %v", err) - } - - // Result should contain content from fileA and fileB, but cycle should be prevented - if !strings.Contains(result, "File A") { - t.Errorf("ProcessIncludes result should contain File A content") - } - if !strings.Contains(result, "File B") { - t.Errorf("ProcessIncludes result should contain File B content") - } -} func TestProcessIncludedFileWithNameAndDescription(t *testing.T) { tempDir := t.TempDir() diff --git a/pkg/parser/frontmatter_syntax_errors_test.go b/pkg/parser/frontmatter_syntax_errors_test.go deleted file mode 100644 index 17d4bf2539b..00000000000 --- a/pkg/parser/frontmatter_syntax_errors_test.go +++ /dev/null @@ -1,691 +0,0 @@ -//go:build !integration - -package parser - -import ( - "os" - "path/filepath" - "strings" - "testing" - - "github.com/github/gh-aw/pkg/console" -) - -// TestFrontmatterSyntaxErrors provides extensive test suite for frontmatter syntax errors -func TestFrontmatterSyntaxErrors(t *testing.T) { - tests := []struct { - name string - frontmatterContent string - markdownContent string - expectError bool - expectedMinLine int // Minimum expected line number - expectedMinColumn int // Minimum expected column number - expectedErrorContains string // Substring that should be in error message - description string // Human-readable description of the error scenario - }{ - { - name: "missing_colon_in_mapping", - frontmatterContent: `--- -name: Test Workflow -on push -permissions: read-all ----`, - markdownContent: `# Test Workflow -This is a test workflow.`, - expectError: true, - expectedMinLine: 3, - expectedMinColumn: 1, - expectedErrorContains: "non-map value", - description: "Missing colon in YAML mapping", - }, - { - name: "invalid_indentation", - frontmatterContent: `--- -name: Test Workflow -on: - push: - branches: - - main -permissions: read-all ----`, - markdownContent: `# Test Workflow -This workflow has invalid indentation.`, - expectError: true, - expectedMinLine: 4, - expectedMinColumn: 1, - expectedErrorContains: "non-map value", - description: "Invalid indentation in nested YAML structure", - }, - { - name: "duplicate_keys", - frontmatterContent: `--- -name: Test Workflow -on: push -name: Duplicate Name -permissions: read-all ----`, - markdownContent: `# Test Workflow -This workflow has duplicate keys.`, - expectError: true, - expectedMinLine: 4, - expectedMinColumn: 1, - expectedErrorContains: "duplicate", - description: "Duplicate keys in YAML frontmatter", - }, - { - name: "unclosed_bracket_in_array", - frontmatterContent: `--- -name: Test Workflow -on: - push: - branches: [main, dev -permissions: read-all ----`, - markdownContent: `# Test Workflow -This workflow has unclosed brackets.`, - expectError: true, - expectedMinLine: 5, - expectedMinColumn: 1, - expectedErrorContains: "must be specified", - description: "Unclosed bracket in YAML array", - }, - { - name: "unclosed_brace_in_object", - frontmatterContent: `--- -name: Test Workflow -on: - push: {branches: [main], types: [opened -permissions: read-all ----`, - markdownContent: `# Test Workflow -This workflow has unclosed braces.`, - expectError: true, - expectedMinLine: 4, - expectedMinColumn: 1, - expectedErrorContains: "must be specified", - description: "Unclosed brace in YAML object", - }, - { - name: "invalid_yaml_character", - frontmatterContent: `--- -name: Test Workflow -on: @invalid_character -permissions: read-all ----`, - markdownContent: `# Test Workflow -This workflow has invalid YAML characters.`, - expectError: true, - expectedMinLine: 3, - expectedMinColumn: 1, - expectedErrorContains: "reserved character", - description: "Invalid character that cannot start YAML token", - }, - { - name: "malformed_string_quotes", - frontmatterContent: `--- -name: "Test Workflow -on: push -permissions: read-all ----`, - markdownContent: `# Test Workflow -This workflow has malformed string quotes.`, - expectError: true, - expectedMinLine: 2, - expectedMinColumn: 1, - expectedErrorContains: "could not find end character", - description: "Malformed string quotes in YAML", - }, - { - name: "invalid_boolean_value", - frontmatterContent: `--- -name: Test Workflow -on: push -enabled: yes_please -permissions: read-all ----`, - markdownContent: `# Test Workflow -This workflow has invalid boolean value.`, - expectError: false, // This may not cause a parse error, just invalid data - expectedMinLine: 0, - expectedMinColumn: 0, - expectedErrorContains: "", - description: "Invalid boolean value in YAML (may parse as string)", - }, - { - name: "missing_value_after_colon", - frontmatterContent: `--- -name: Test Workflow -on: -permissions: read-all ----`, - markdownContent: `# Test Workflow -This workflow has missing value after colon.`, - expectError: false, // This actually parses as null value - expectedMinLine: 0, - expectedMinColumn: 0, - expectedErrorContains: "", - description: "Missing value after colon in YAML mapping (parses as null)", - }, - { - name: "invalid_list_structure", - frontmatterContent: `--- -name: Test Workflow -on: - push: - branches: - main - - dev -permissions: read-all ----`, - markdownContent: `# Test Workflow -This workflow has invalid list structure.`, - expectError: false, // This may actually parse successfully - expectedMinLine: 0, - expectedMinColumn: 0, - expectedErrorContains: "", - description: "Invalid list structure mixing plain and dash syntax (may be accepted)", - }, - { - name: "unexpected_end_of_stream", - frontmatterContent: `--- -name: Test Workflow -on: - push: - branches: [ ----`, - markdownContent: `# Test Workflow -This workflow has unexpected end of stream.`, - expectError: true, - expectedMinLine: 5, - expectedMinColumn: 14, - expectedErrorContains: "not found", - description: "Unexpected end of stream in YAML", - }, - { - name: "invalid_escape_sequence", - frontmatterContent: `--- -name: Test Workflow -description: "Invalid escape: \z" -on: push -permissions: read-all ----`, - markdownContent: `# Test Workflow -This workflow has invalid escape sequence.`, - expectError: true, - expectedMinLine: 3, - expectedMinColumn: 26, - expectedErrorContains: "escape", - description: "Invalid escape sequence in YAML string", - }, - { - name: "mixed_tab_and_space_indentation", - frontmatterContent: `--- -name: Test Workflow -on: - push: - branches: - - main -permissions: read-all ----`, - markdownContent: `# Test Workflow -This workflow has mixed tab and space indentation.`, - expectError: true, // goccy actually does catch this error - expectedMinLine: 5, - expectedMinColumn: 1, - expectedErrorContains: "cannot start any token", - description: "Mixed tab and space indentation in YAML", - }, - { - name: "anchor_without_alias", - frontmatterContent: `--- -name: Test Workflow -defaults: &default_settings - timeout: 30 -on: push -job1: *missing_anchor -permissions: read-all ----`, - markdownContent: `# Test Workflow -This workflow has anchor without alias.`, - expectError: true, - expectedMinLine: 6, - expectedMinColumn: 7, - expectedErrorContains: "alias", - description: "Reference to undefined YAML anchor", - }, - { - name: "complex_nested_structure_error", - frontmatterContent: `--- -name: Test Workflow -on: - push: - branches: - - main - paths: - - "src/**" - pull_request: - types: [opened, synchronize - branches: [main] -jobs: - test: - runs-on: ubuntu-latest - steps: - - uses: actions/checkout@v3 -permissions: read-all ----`, - markdownContent: `# Test Workflow -This workflow has complex nested structure error.`, - expectError: true, - expectedMinLine: 10, - expectedMinColumn: 1, - expectedErrorContains: "must be specified", - description: "Complex nested structure with missing closing bracket", - }, - { - name: "invalid_multiline_string", - frontmatterContent: `--- -name: Test Workflow -description: | - This is a multiline - description that has -invalid_key: value -on: push -permissions: read-all ----`, - markdownContent: `# Test Workflow -This workflow has invalid multiline string.`, - expectError: false, // This may actually parse successfully with literal block - expectedMinLine: 0, - expectedMinColumn: 0, - expectedErrorContains: "", - description: "Invalid multiline string structure in YAML (may be accepted)", - }, - { - name: "schema_validation_error_unknown_field", - frontmatterContent: `--- -name: Test Workflow -on: push -unknown_field: value -invalid_permissions: write -permissions: read-all ----`, - markdownContent: `# Test Workflow -This workflow may have schema validation errors.`, - expectError: false, // This might not be a YAML syntax error but a schema error - expectedMinLine: 0, - expectedMinColumn: 0, - expectedErrorContains: "", - description: "Schema validation error with unknown fields (may not cause parse error)", - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - // Create temporary file for testing - tempDir, err := os.MkdirTemp("", "frontmatter_syntax_test_*") - if err != nil { - t.Fatalf("Failed to create temp directory: %v", err) - } - defer os.RemoveAll(tempDir) - - // Write test file with frontmatter and markdown content - testFile := filepath.Join(tempDir, "test.md") - fullContent := tt.frontmatterContent + "\n\n" + tt.markdownContent - if err := os.WriteFile(testFile, []byte(fullContent), 0644); err != nil { - t.Fatalf("Failed to write test file: %v", err) - } - - // Attempt to parse frontmatter - result, err := ExtractFrontmatterFromContent(fullContent) - - if tt.expectError { - if err == nil { - t.Errorf("Expected error for %s, but parsing succeeded", tt.description) - return - } - - // Extract error location information - line, column, message := ExtractYAMLError(err, 2) // Frontmatter starts at line 2 - - // Verify error location is reasonable - if line > 0 && line < tt.expectedMinLine { - t.Errorf("Expected line >= %d, got %d for %s", tt.expectedMinLine, line, tt.description) - } - - if column > 0 && tt.expectedMinColumn > 0 && column < tt.expectedMinColumn { - t.Errorf("Expected column >= %d, got %d for %s", tt.expectedMinColumn, column, tt.description) - } - - // Verify error message contains expected content - if tt.expectedErrorContains != "" && !strings.Contains(strings.ToLower(message), strings.ToLower(tt.expectedErrorContains)) { - t.Errorf("Expected error message to contain '%s', got '%s' for %s", tt.expectedErrorContains, message, tt.description) - } - - // Log detailed error information for debugging - t.Logf("✓ %s: Line %d, Column %d, Error: %s", tt.description, line, column, message) - - // Verify that console error formatting works - compilerError := console.CompilerError{ - Position: console.ErrorPosition{ - File: "test.md", - Line: line, - Column: column, - }, - Type: "error", - Message: "frontmatter parsing failed: " + message, - } - - formattedError := console.FormatError(compilerError) - if formattedError == "" { - t.Errorf("Console error formatting failed for %s", tt.description) - } - - } else { - if err != nil { - t.Errorf("Unexpected error for %s: %v", tt.description, err) - return - } - - if result == nil { - t.Errorf("Expected successful parsing result for %s", tt.description) - return - } - - t.Logf("✓ %s: Successfully parsed (no syntax error as expected)", tt.description) - } - }) - } -} - -// TestFrontmatterParsingWithRealGoccyErrors tests frontmatter parsing with actual goccy/go-yaml errors -func TestFrontmatterParsingWithRealGoccyErrors(t *testing.T) { - tests := []struct { - name string - yamlContent string - expectPreciseLocation bool - description string - }{ - { - name: "real_mapping_error", - yamlContent: `name: Test -on: push -invalid syntax here -permissions: read`, - expectPreciseLocation: true, - description: "Real mapping syntax error that goccy should catch with precise location", - }, - { - name: "real_indentation_error", - yamlContent: `name: Test -on: - push: - branches: - invalid_indent: here -permissions: read`, - expectPreciseLocation: false, // This may actually parse successfully - description: "Real indentation error that may not cause parse error", - }, - { - name: "real_array_error", - yamlContent: `name: Test -on: - push: - branches: [main, dev, feature/test -permissions: read`, - expectPreciseLocation: true, - description: "Real array syntax error that goccy should catch with precise location", - }, - { - name: "real_string_error", - yamlContent: `name: "Unterminated string -on: push -permissions: read`, - expectPreciseLocation: true, - description: "Real string syntax error that goccy should catch with precise location", - }, - { - name: "real_complex_structure_error", - yamlContent: `name: Test -on: - workflow_dispatch: - inputs: - version: - description: 'Version to deploy' - required: true - default: 'latest' - type: string - environment: - description: 'Environment' - required: true - default: 'staging' - type: choice - options: [staging, production -jobs: - deploy: - runs-on: ubuntu-latest`, - expectPreciseLocation: true, - description: "Real complex structure error that goccy should catch with precise location", - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - // Create full frontmatter content - fullContent := "---\n" + tt.yamlContent + "\n---\n\n# Test\nContent here." - - // Attempt to parse frontmatter - _, err := ExtractFrontmatterFromContent(fullContent) - - if err == nil { - if tt.expectPreciseLocation { - t.Errorf("Expected parsing to fail for %s", tt.description) - return - } else { - t.Logf("✓ %s: Parsed successfully (may not be an error)", tt.description) - return - } - } - - // Extract error location using our goccy parser - line, column, message := ExtractYAMLError(err, 2) // Frontmatter starts at line 2 - - t.Logf("Goccy Error for %s:", tt.description) - t.Logf(" Original Error: %s", err.Error()) - t.Logf(" Parsed Location: Line %d, Column %d", line, column) - t.Logf(" Parsed Message: %s", message) - - if tt.expectPreciseLocation { - // Verify we got a reasonable line and column - if line < 2 { // Should be at least at frontmatter start - t.Errorf("Expected line >= 2, got %d for %s", line, tt.description) - } - - if column <= 0 { - t.Errorf("Expected column > 0, got %d for %s", column, tt.description) - } - - if message == "" { - t.Errorf("Expected non-empty message for %s", tt.description) - } - - // Verify that we're getting goccy's native format, not fallback parsing - if strings.Contains(err.Error(), "[") && strings.Contains(err.Error(), "]") { - t.Logf("✓ Using goccy native [line:column] format for %s", tt.description) - } else { - t.Logf("ℹ Using fallback string parsing for %s", tt.description) - } - } - }) - } -} - -// TestFrontmatterErrorContextExtraction tests that we extract good context for error reporting -func TestFrontmatterErrorContextExtraction(t *testing.T) { - content := `--- -name: Test Workflow -on: - push: - branches: [main, dev - pull_request: - types: [opened] -permissions: read-all -jobs: - test: - runs-on: ubuntu-latest ---- - -# Test Workflow - -This is a test workflow with a syntax error in the frontmatter. -The error is on line 5 where there's an unclosed bracket.` - - result, err := ExtractFrontmatterFromContent(content) - - if err == nil { - t.Fatal("Expected parsing to fail due to syntax error") - } - - // Extract error information - line, column, message := ExtractYAMLError(err, 2) - - if line <= 2 { - t.Errorf("Expected error line > 2, got %d", line) - } - - if column <= 0 { - t.Errorf("Expected error column > 0, got %d", column) - } - - // Verify we have frontmatter lines for context - if result != nil && len(result.FrontmatterLines) > 0 { - t.Logf("✓ Frontmatter context available with %d lines", len(result.FrontmatterLines)) - } else { - t.Log("ℹ No frontmatter context available (expected for parse errors)") - } - - // Create console error format - compilerError := console.CompilerError{ - Position: console.ErrorPosition{ - File: "test.md", - Line: line, - Column: column, - }, - Type: "error", - Message: "frontmatter parsing failed: " + message, - } - - // Test that error formatting works - formattedError := console.FormatError(compilerError) - if formattedError == "" { - t.Error("Error formatting failed") - } else { - t.Logf("✓ Formatted error:\n%s", formattedError) - } - - t.Logf("Error details: Line %d, Column %d, Message: %s", line, column, message) -} - -// TestFrontmatterSyntaxErrorBoundaryConditions tests edge cases and boundary conditions -func TestFrontmatterSyntaxErrorBoundaryConditions(t *testing.T) { - tests := []struct { - name string - content string - expectError bool - description string - }{ - { - name: "minimal_invalid_frontmatter", - content: `--- -: ---- - -# Content`, - expectError: true, - description: "Minimal invalid frontmatter with just a colon", - }, - { - name: "empty_frontmatter_with_error", - content: `--- ---- - -# Content`, - expectError: false, - description: "Empty frontmatter should not cause parse error", - }, - { - name: "very_long_line_with_error", - content: `--- -name: Test -very_long_line_with_error: ` + strings.Repeat("a", 1000) + ` invalid: syntax -permissions: read-all ---- - -# Content`, - expectError: true, - description: "Very long line with syntax error", - }, - { - name: "unicode_content_with_error", - content: `--- -name: "测试工作流 🚀" -description: "这是一个测试" -invalid_syntax_here -on: push -permissions: read-all ---- - -# 测试内容 - -这里是 markdown 内容。`, - expectError: true, - description: "Unicode content with syntax error", - }, - { - name: "deeply_nested_error", - content: `--- -name: Test -jobs: - test: - strategy: - matrix: - os: [ubuntu, windows] - node: [14, 16, 18] - include: - - os: ubuntu - node: 20 - special: true - exclude: - - os: windows - node: 14 - invalid syntax here -permissions: read-all ---- - -# Content`, - expectError: true, - description: "Deeply nested structure with syntax error", - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - _, err := ExtractFrontmatterFromContent(tt.content) - - if tt.expectError { - if err == nil { - t.Errorf("Expected error for %s", tt.description) - return - } - - line, column, message := ExtractYAMLError(err, 2) - t.Logf("✓ %s: Line %d, Column %d, Error: %s", tt.description, line, column, message) - } else { - if err != nil { - t.Errorf("Unexpected error for %s: %v", tt.description, err) - } else { - t.Logf("✓ %s: Parsed successfully as expected", tt.description) - } - } - }) - } -} diff --git a/pkg/parser/import_syntax_test.go b/pkg/parser/import_syntax_test.go index b662d9b2563..ae8c60d2422 100644 --- a/pkg/parser/import_syntax_test.go +++ b/pkg/parser/import_syntax_test.go @@ -3,11 +3,8 @@ package parser import ( - "os" "strings" "testing" - - "github.com/github/gh-aw/pkg/testutil" ) func TestParseImportDirective(t *testing.T) { @@ -172,83 +169,3 @@ func TestParseImportDirective(t *testing.T) { }) } } - -func TestProcessIncludesWithNewSyntax(t *testing.T) { - // Create temporary test files - tempDir := testutil.TempDir(t, "test-*") - - // Create test file with markdown content - testFile := tempDir + "/test.md" - testContent := `--- -tools: - bash: - allowed: ["ls", "cat"] ---- - -# Test Content -This is a test file content. -` - if err := os.WriteFile(testFile, []byte(testContent), 0644); err != nil { - t.Fatalf("Failed to write test file: %v", err) - } - - tests := []struct { - name string - content string - expected string - wantErr bool - }{ - { - name: "new syntax - basic import with colon", - content: "{{#import: test.md}}\n# After import", - expected: "# Test Content\nThis is a test file content.\n# After import\n", - wantErr: false, - }, - { - name: "new syntax - basic import without colon", - content: "{{#import test.md}}\n# After import", - expected: "# Test Content\nThis is a test file content.\n# After import\n", - wantErr: false, - }, - { - name: "new syntax - optional import with colon (file exists)", - content: "{{#import?: test.md}}\n# After import", - expected: "# Test Content\nThis is a test file content.\n# After import\n", - wantErr: false, - }, - { - name: "new syntax - optional import without colon (file exists)", - content: "{{#import? test.md}}\n# After import", - expected: "# Test Content\nThis is a test file content.\n# After import\n", - wantErr: false, - }, - { - name: "new syntax - optional import (file missing)", - content: "{{#import?: nonexistent.md}}\n# After import", - expected: "# After import\n", - wantErr: false, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - result, err := ProcessIncludes(tt.content, tempDir, false) - - if tt.wantErr { - if err == nil { - t.Errorf("ProcessIncludes() expected error, got nil") - } - return - } - - if err != nil { - t.Errorf("ProcessIncludes() unexpected error = %v", err) - return - } - - if result != tt.expected { - t.Errorf("ProcessIncludes() result = %q, want %q", result, tt.expected) - } - }) - } -} diff --git a/pkg/parser/include_expander.go b/pkg/parser/include_expander.go index b5455cac9e0..a49514d36b0 100644 --- a/pkg/parser/include_expander.go +++ b/pkg/parser/include_expander.go @@ -10,10 +10,6 @@ import ( // ExpandIncludes recursively expands @include and @import directives until no more remain // This matches the bash expand_includes function behavior -func ExpandIncludes(content, baseDir string, extractTools bool) (string, error) { - expandedContent, _, err := ExpandIncludesWithManifest(content, baseDir, extractTools) - return expandedContent, err -} // ExpandIncludesWithManifest recursively expands @include and @import directives and returns list of included files func ExpandIncludesWithManifest(content, baseDir string, extractTools bool) (string, []string, error) { @@ -120,18 +116,8 @@ func expandIncludesForField(content, baseDir string, extractFunc func(string) (s } // ProcessIncludesForEngines processes import directives to extract engine configurations -func ProcessIncludesForEngines(content, baseDir string) ([]string, string, error) { - return processIncludesForField(content, baseDir, func(c string) (string, error) { - return extractFrontmatterField(c, "engine", "") - }, "") -} // ProcessIncludesForSafeOutputs processes import directives to extract safe-outputs configurations -func ProcessIncludesForSafeOutputs(content, baseDir string) ([]string, string, error) { - return processIncludesForField(content, baseDir, func(c string) (string, error) { - return extractFrontmatterField(c, "safe-outputs", "{}") - }, "{}") -} // processIncludesForField processes import directives to extract a specific frontmatter field func processIncludesForField(content, baseDir string, extractFunc func(string) (string, error), emptyValue string) ([]string, string, error) { diff --git a/pkg/parser/include_processor.go b/pkg/parser/include_processor.go index 012453d7288..ee72de5fb6c 100644 --- a/pkg/parser/include_processor.go +++ b/pkg/parser/include_processor.go @@ -17,11 +17,6 @@ var includeLog = logger.New("parser:include_processor") // ProcessIncludes processes @include, @import (deprecated), and {{#import: directives in markdown content // This matches the bash process_includes function behavior -func ProcessIncludes(content, baseDir string, extractTools bool) (string, error) { - includeLog.Printf("Processing includes: baseDir=%s, extractTools=%t, content_size=%d", baseDir, extractTools, len(content)) - visited := make(map[string]bool) - return processIncludesWithVisited(content, baseDir, extractTools, visited) -} // processIncludesWithVisited processes import directives with cycle detection func processIncludesWithVisited(content, baseDir string, extractTools bool, visited map[string]bool) (string, error) { diff --git a/pkg/parser/schema_additional_properties_test.go b/pkg/parser/schema_additional_properties_test.go deleted file mode 100644 index c6c1f0dd874..00000000000 --- a/pkg/parser/schema_additional_properties_test.go +++ /dev/null @@ -1,283 +0,0 @@ -//go:build !integration - -package parser - -import ( - "strings" - "testing" -) - -// TestAdditionalPropertiesFalse_CommonTypos tests that common typos in frontmatter -// are properly rejected by the schema validation due to additionalProperties: false -func TestAdditionalPropertiesFalse_CommonTypos(t *testing.T) { - tests := []struct { - name string - frontmatter map[string]any - typoField string // The typo field name that should be rejected - }{ - { - name: "typo: permisions instead of permissions", //nolint:misspell - frontmatter: map[string]any{ - "on": "push", - "permisions": "write-all", //nolint:misspell // typo: should be "permissions" - }, - typoField: "permisions", //nolint:misspell - }, - { - name: "typo: engnie instead of engine", - frontmatter: map[string]any{ - "on": "push", - "engnie": "claude", // typo: should be "engine" - }, - typoField: "engnie", - }, - { - name: "typo: toolz instead of tools", - frontmatter: map[string]any{ - "on": "push", - "toolz": map[string]any{ // typo: should be "tools" - "github": nil, - }, - }, - typoField: "toolz", - }, - { - name: "typo: timeout_minute instead of timeout_minutes", - frontmatter: map[string]any{ - "on": "push", - "timeout_minute": 10, // typo: should be "timeout_minutes" - }, - typoField: "timeout_minute", - }, - { - name: "typo: runs_on instead of runs-on", - frontmatter: map[string]any{ - "on": "push", - "runs_on": "ubuntu-latest", // typo: should be "runs-on" with dash - }, - typoField: "runs_on", - }, - { - name: "typo: safe_outputs instead of safe-outputs", - frontmatter: map[string]any{ - "on": "push", - "safe_outputs": map[string]any{ // typo: should be "safe-outputs" with dash - "create-issue": nil, - }, - }, - typoField: "safe_outputs", - }, - { - name: "typo: mcp_servers instead of mcp-servers", - frontmatter: map[string]any{ - "on": "push", - "mcp_servers": map[string]any{ // typo: should be "mcp-servers" with dash - "test": map[string]any{ - "command": "test", - }, - }, - }, - typoField: "mcp_servers", - }, - { - name: "multiple typos: permisions, engnie, toolz", //nolint:misspell - frontmatter: map[string]any{ - "on": "push", - "permisions": "write-all", //nolint:misspell // typo - "engnie": "claude", // typo - "toolz": map[string]any{ // typo - "github": nil, - }, - }, - typoField: "permisions", //nolint:misspell // error should mention at least one typo - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - err := ValidateMainWorkflowFrontmatterWithSchema(tt.frontmatter) - - if err == nil { - t.Fatalf("Expected validation error for typo field '%s', but validation passed", tt.typoField) - } - - errorMsg := err.Error() - - // The error should mention unknown/additional properties - if !strings.Contains(strings.ToLower(errorMsg), "unknown") && - !strings.Contains(strings.ToLower(errorMsg), "additional") && - !strings.Contains(strings.ToLower(errorMsg), "not allowed") { - t.Errorf("Error message should mention unknown/additional properties, got: %s", errorMsg) - } - - // The error should mention the typo field - if !strings.Contains(errorMsg, tt.typoField) { - t.Errorf("Error message should mention the typo field '%s', got: %s", tt.typoField, errorMsg) - } - }) - } -} - -// TestAdditionalPropertiesFalse_IncludedFileSchema tests that the included file schema -// also rejects unknown properties -func TestAdditionalPropertiesFalse_IncludedFileSchema(t *testing.T) { - tests := []struct { - name string - frontmatter map[string]any - typoField string - }{ - { - name: "typo in included file: toolz instead of tools", - frontmatter: map[string]any{ - "toolz": map[string]any{ // typo: should be "tools" - "github": nil, - }, - }, - typoField: "toolz", - }, - { - name: "typo in included file: mcp_servers instead of mcp-servers", - frontmatter: map[string]any{ - "mcp_servers": map[string]any{ // typo: should be "mcp-servers" - "test": map[string]any{ - "command": "test", - }, - }, - }, - typoField: "mcp_servers", - }, - { - name: "typo in included file: safe_outputs instead of safe-outputs", - frontmatter: map[string]any{ - "safe_outputs": map[string]any{ // typo: should be "safe-outputs" - "jobs": map[string]any{ - "test": map[string]any{ - "inputs": map[string]any{ - "test": map[string]any{ - "type": "string", - }, - }, - }, - }, - }, - }, - typoField: "safe_outputs", - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - err := ValidateIncludedFileFrontmatterWithSchema(tt.frontmatter) - - if err == nil { - t.Fatalf("Expected validation error for typo field '%s', but validation passed", tt.typoField) - } - - errorMsg := err.Error() - - // The error should mention the typo field - if !strings.Contains(errorMsg, tt.typoField) { - t.Errorf("Error message should mention the typo field '%s', got: %s", tt.typoField, errorMsg) - } - }) - } -} - -// TestAdditionalPropertiesFalse_MCPConfigSchema tests that the MCP config schema -// also rejects unknown properties -func TestAdditionalPropertiesFalse_MCPConfigSchema(t *testing.T) { - tests := []struct { - name string - frontmatter map[string]any - typoField string - }{ - { - name: "typo in MCP config: comand instead of command", - frontmatter: map[string]any{ - "comand": "npx", // typo: should be "command" - }, - typoField: "comand", - }, - { - name: "typo in MCP config: typ instead of type", - frontmatter: map[string]any{ - "typ": "stdio", // typo: should be "type" - "command": "test", - }, - typoField: "typ", - }, - { - name: "typo in MCP config: environement instead of env", - frontmatter: map[string]any{ - "command": "test", - "environement": map[string]any{ // typo: should be "env" - "TEST": "value", - }, - }, - typoField: "environement", - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - err := ValidateMCPConfigWithSchema(tt.frontmatter, "test-tool") - - if err == nil { - t.Fatalf("Expected validation error for typo field '%s', but validation passed", tt.typoField) - } - - errorMsg := err.Error() - - // The error should mention the typo field - if !strings.Contains(errorMsg, tt.typoField) { - t.Errorf("Error message should mention the typo field '%s', got: %s", tt.typoField, errorMsg) - } - }) - } -} - -// TestValidProperties_NotRejected ensures that valid properties are still accepted -func TestValidProperties_NotRejected(t *testing.T) { - tests := []struct { - name string - frontmatter map[string]any - }{ - { - name: "valid main workflow with all common fields", - frontmatter: map[string]any{ - "on": "push", - "permissions": "read-all", - "engine": "claude", - "tools": map[string]any{ - "github": nil, - }, - "timeout-minutes": 10, - "runs-on": "ubuntu-latest", - "safe-outputs": map[string]any{ - "create-issue": nil, - }, - "mcp-servers": map[string]any{ - "test": map[string]any{ - "command": "test", - }, - }, - }, - }, - { - name: "valid minimal workflow", - frontmatter: map[string]any{ - "on": "push", - }, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - err := ValidateMainWorkflowFrontmatterWithSchema(tt.frontmatter) - - if err != nil { - t.Fatalf("Expected no validation error for valid frontmatter, got: %v", err) - } - }) - } -} diff --git a/pkg/parser/schema_oneof_test.go b/pkg/parser/schema_oneof_test.go deleted file mode 100644 index 0a6ffb1b15d..00000000000 --- a/pkg/parser/schema_oneof_test.go +++ /dev/null @@ -1,334 +0,0 @@ -//go:build !integration - -package parser - -import ( - "strings" - "testing" -) - -// TestValidateOneOfConstraints tests the oneOf constraints added to the schema -// to prevent mutually exclusive fields from being specified together. -func TestValidateOneOfConstraints(t *testing.T) { - tests := []struct { - name string - frontmatter map[string]any - wantErr bool - errContains string - }{ - // branches vs branches-ignore in push event - { - name: "invalid: both branches and branches-ignore in push", - frontmatter: map[string]any{ - "on": map[string]any{ - "push": map[string]any{ - "branches": []string{"main"}, - "branches-ignore": []string{"dev"}, - }, - }, - }, - wantErr: true, - errContains: "oneOf", - }, - { - name: "valid: only branches in push", - frontmatter: map[string]any{ - "on": map[string]any{ - "push": map[string]any{ - "branches": []string{"main"}, - }, - }, - }, - wantErr: false, - }, - { - name: "valid: only branches-ignore in push", - frontmatter: map[string]any{ - "on": map[string]any{ - "push": map[string]any{ - "branches-ignore": []string{"dev"}, - }, - }, - }, - wantErr: false, - }, - { - name: "valid: neither branches nor branches-ignore in push", - frontmatter: map[string]any{ - "on": map[string]any{ - "push": map[string]any{ - "tags": []string{"v*"}, - }, - }, - }, - wantErr: false, - }, - - // paths vs paths-ignore in push event - { - name: "invalid: both paths and paths-ignore in push", - frontmatter: map[string]any{ - "on": map[string]any{ - "push": map[string]any{ - "paths": []string{"src/**"}, - "paths-ignore": []string{"docs/**"}, - }, - }, - }, - wantErr: true, - errContains: "oneOf", - }, - { - name: "valid: only paths in push", - frontmatter: map[string]any{ - "on": map[string]any{ - "push": map[string]any{ - "paths": []string{"src/**"}, - }, - }, - }, - wantErr: false, - }, - { - name: "valid: only paths-ignore in push", - frontmatter: map[string]any{ - "on": map[string]any{ - "push": map[string]any{ - "paths-ignore": []string{"docs/**"}, - }, - }, - }, - wantErr: false, - }, - - // branches vs branches-ignore in pull_request event - { - name: "invalid: both branches and branches-ignore in pull_request", - frontmatter: map[string]any{ - "on": map[string]any{ - "pull_request": map[string]any{ - "branches": []string{"main"}, - "branches-ignore": []string{"dev"}, - }, - }, - }, - wantErr: true, - errContains: "oneOf", - }, - { - name: "valid: only branches in pull_request", - frontmatter: map[string]any{ - "on": map[string]any{ - "pull_request": map[string]any{ - "branches": []string{"main"}, - }, - }, - }, - wantErr: false, - }, - { - name: "valid: only branches-ignore in pull_request", - frontmatter: map[string]any{ - "on": map[string]any{ - "pull_request": map[string]any{ - "branches-ignore": []string{"dev"}, - }, - }, - }, - wantErr: false, - }, - - // paths vs paths-ignore in pull_request event - { - name: "invalid: both paths and paths-ignore in pull_request", - frontmatter: map[string]any{ - "on": map[string]any{ - "pull_request": map[string]any{ - "paths": []string{"src/**"}, - "paths-ignore": []string{"docs/**"}, - }, - }, - }, - wantErr: true, - errContains: "oneOf", - }, - { - name: "valid: only paths in pull_request", - frontmatter: map[string]any{ - "on": map[string]any{ - "pull_request": map[string]any{ - "paths": []string{"src/**"}, - }, - }, - }, - wantErr: false, - }, - - // branches vs branches-ignore in pull_request_target event - { - name: "invalid: both branches and branches-ignore in pull_request_target", - frontmatter: map[string]any{ - "on": map[string]any{ - "pull_request_target": map[string]any{ - "branches": []string{"main"}, - "branches-ignore": []string{"dev"}, - }, - }, - }, - wantErr: true, - errContains: "oneOf", - }, - { - name: "valid: only branches in pull_request_target", - frontmatter: map[string]any{ - "on": map[string]any{ - "pull_request_target": map[string]any{ - "branches": []string{"main"}, - }, - }, - }, - wantErr: false, - }, - - // paths vs paths-ignore in pull_request_target event - { - name: "invalid: both paths and paths-ignore in pull_request_target", - frontmatter: map[string]any{ - "on": map[string]any{ - "pull_request_target": map[string]any{ - "paths": []string{"src/**"}, - "paths-ignore": []string{"docs/**"}, - }, - }, - }, - wantErr: true, - errContains: "oneOf", - }, - - // branches vs branches-ignore in workflow_run event - { - name: "invalid: both branches and branches-ignore in workflow_run", - frontmatter: map[string]any{ - "on": map[string]any{ - "workflow_run": map[string]any{ - "workflows": []string{"CI"}, - "branches": []string{"main"}, - "branches-ignore": []string{"dev"}, - }, - }, - }, - wantErr: true, - errContains: "oneOf", - }, - { - name: "valid: only branches in workflow_run", - frontmatter: map[string]any{ - "on": map[string]any{ - "workflow_run": map[string]any{ - "workflows": []string{"CI"}, - "branches": []string{"main"}, - }, - }, - }, - wantErr: false, - }, - - // slash_command vs label events - { - name: "invalid: slash_command with label event", - frontmatter: map[string]any{ - "on": map[string]any{ - "slash_command": "mybot", - "label": map[string]any{ - "types": []string{"created"}, - }, - }, - }, - wantErr: true, - errContains: "not", - }, - { - name: "valid: slash_command without label event", - frontmatter: map[string]any{ - "on": map[string]any{ - "slash_command": "mybot", - }, - }, - wantErr: false, - }, - { - name: "valid: label event without slash_command", - frontmatter: map[string]any{ - "on": map[string]any{ - "label": map[string]any{ - "types": []string{"created"}, - }, - }, - }, - wantErr: false, - }, - - // command vs label events (deprecated command field) - { - name: "invalid: command with label event", - frontmatter: map[string]any{ - "on": map[string]any{ - "command": "mybot", - "label": map[string]any{ - "types": []string{"created"}, - }, - }, - }, - wantErr: true, - errContains: "not", - }, - - // Valid combinations of branches and paths - { - name: "valid: branches and paths in push", - frontmatter: map[string]any{ - "on": map[string]any{ - "push": map[string]any{ - "branches": []string{"main"}, - "paths": []string{"src/**"}, - }, - }, - }, - wantErr: false, - }, - { - name: "valid: branches-ignore and paths-ignore in push", - frontmatter: map[string]any{ - "on": map[string]any{ - "push": map[string]any{ - "branches-ignore": []string{"dev"}, - "paths-ignore": []string{"docs/**"}, - }, - }, - }, - wantErr: false, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - err := ValidateMainWorkflowFrontmatterWithSchema(tt.frontmatter) - - if tt.wantErr && err == nil { - t.Errorf("ValidateMainWorkflowFrontmatterWithSchema() expected error, got nil") - return - } - - if !tt.wantErr && err != nil { - t.Errorf("ValidateMainWorkflowFrontmatterWithSchema() error = %v", err) - return - } - - if err != nil && tt.errContains != "" { - if !strings.Contains(err.Error(), tt.errContains) { - t.Errorf("ValidateMainWorkflowFrontmatterWithSchema() error = %v, expected to contain %q", err.Error(), tt.errContains) - } - } - }) - } -} diff --git a/pkg/parser/schema_passthrough_validation_test.go b/pkg/parser/schema_passthrough_validation_test.go deleted file mode 100644 index 8c2c0fb889a..00000000000 --- a/pkg/parser/schema_passthrough_validation_test.go +++ /dev/null @@ -1,572 +0,0 @@ -//go:build !integration - -package parser - -import ( - "strings" - "testing" -) - -// TestPassThroughFieldValidation tests that pass-through YAML fields -// (concurrency, container, environment, env, secrets, runs-on, services) are -// properly validated by the schema during frontmatter parsing. -// -// These fields are "pass-through" in that they are extracted from -// frontmatter and passed directly to GitHub Actions without modification, -// but they still need basic structure validation to catch obvious errors -// at compile time rather than at GitHub Actions runtime. -func TestPassThroughFieldValidation(t *testing.T) { - tests := []struct { - name string - frontmatter map[string]any - wantErr bool - errContains string - }{ - // Concurrency field tests - { - name: "valid concurrency - simple string", - frontmatter: map[string]any{ - "on": "push", - "concurrency": "my-group", - }, - wantErr: false, - }, - { - name: "valid concurrency - object with group", - frontmatter: map[string]any{ - "on": "push", - "concurrency": map[string]any{ - "group": "my-group", - }, - }, - wantErr: false, - }, - { - name: "valid concurrency - object with group and cancel-in-progress", - frontmatter: map[string]any{ - "on": "push", - "concurrency": map[string]any{ - "group": "my-group", - "cancel-in-progress": true, - }, - }, - wantErr: false, - }, - { - name: "invalid concurrency - array", - frontmatter: map[string]any{ - "on": "push", - "concurrency": []string{"invalid"}, - }, - wantErr: true, - errContains: "oneOf", - }, - { - name: "invalid concurrency - object missing required group", - frontmatter: map[string]any{ - "on": "push", - "concurrency": map[string]any{ - "cancel-in-progress": true, - }, - }, - wantErr: true, - errContains: "missing property 'group'", - }, - { - name: "invalid concurrency - object with invalid field", - frontmatter: map[string]any{ - "on": "push", - "concurrency": map[string]any{ - "group": "my-group", - "invalid": "field", - }, - }, - wantErr: true, - errContains: "additional properties 'invalid' not allowed", - }, - - // Container field tests - { - name: "valid container - simple string", - frontmatter: map[string]any{ - "on": "push", - "container": "ubuntu:latest", - }, - wantErr: false, - }, - { - name: "valid container - object with image", - frontmatter: map[string]any{ - "on": "push", - "container": map[string]any{ - "image": "ubuntu:latest", - }, - }, - wantErr: false, - }, - { - name: "valid container - object with image and credentials", - frontmatter: map[string]any{ - "on": "push", - "container": map[string]any{ - "image": "ubuntu:latest", - "credentials": map[string]any{ - "username": "user", - "password": "${{ secrets.PASSWORD }}", - }, - }, - }, - wantErr: false, - }, - { - name: "invalid container - array", - frontmatter: map[string]any{ - "on": "push", - "container": []string{"invalid"}, - }, - wantErr: true, - errContains: "got array", - }, - { - name: "invalid container - object missing required image", - frontmatter: map[string]any{ - "on": "push", - "container": map[string]any{ - "env": map[string]string{"TEST": "value"}, - }, - }, - wantErr: true, - errContains: "missing property 'image'", - }, - { - name: "invalid container - object with invalid field", - frontmatter: map[string]any{ - "on": "push", - "container": map[string]any{ - "image": "ubuntu:latest", - "invalid": "field", - }, - }, - wantErr: true, - errContains: "additional properties 'invalid' not allowed", - }, - - // Environment field tests - { - name: "valid environment - simple string", - frontmatter: map[string]any{ - "on": "push", - "environment": "production", - }, - wantErr: false, - }, - { - name: "valid environment - object with name", - frontmatter: map[string]any{ - "on": "push", - "environment": map[string]any{ - "name": "production", - }, - }, - wantErr: false, - }, - { - name: "valid environment - object with name and url", - frontmatter: map[string]any{ - "on": "push", - "environment": map[string]any{ - "name": "production", - "url": "https://prod.example.com", - }, - }, - wantErr: false, - }, - { - name: "invalid environment - array", - frontmatter: map[string]any{ - "on": "push", - "environment": []string{"invalid"}, - }, - wantErr: true, - errContains: "oneOf", - }, - { - name: "invalid environment - object missing required name", - frontmatter: map[string]any{ - "on": "push", - "environment": map[string]any{ - "url": "https://prod.example.com", - }, - }, - wantErr: true, - errContains: "missing property 'name'", - }, - { - name: "invalid environment - object with invalid field", - frontmatter: map[string]any{ - "on": "push", - "environment": map[string]any{ - "name": "production", - "invalid": "field", - }, - }, - wantErr: true, - errContains: "additional properties 'invalid' not allowed", - }, - - // Env field tests - { - name: "valid env - object with string values", - frontmatter: map[string]any{ - "on": "push", - "env": map[string]any{ - "NODE_ENV": "production", - "API_KEY": "${{ secrets.API_KEY }}", - }, - }, - wantErr: false, - }, - { - name: "valid env - string (pass-through)", - frontmatter: map[string]any{ - "on": "push", - "env": "some-string", - }, - wantErr: false, - }, - { - name: "invalid env - array", - frontmatter: map[string]any{ - "on": "push", - "env": []string{"invalid"}, - }, - wantErr: true, - errContains: "oneOf", - }, - - // Secrets field tests - { - name: "valid secrets - simple string values", - frontmatter: map[string]any{ - "on": "push", - "secrets": map[string]any{ - "API_TOKEN": "${{ secrets.API_TOKEN }}", - "DATABASE_URL": "${{ secrets.DB_URL }}", - }, - }, - wantErr: false, - }, - { - name: "valid secrets - object with value and description", - frontmatter: map[string]any{ - "on": "push", - "secrets": map[string]any{ - "API_TOKEN": map[string]any{ - "value": "${{ secrets.API_TOKEN }}", - "description": "API token for external service", - }, - }, - }, - wantErr: false, - }, - { - name: "valid secrets - mixed simple and object values", - frontmatter: map[string]any{ - "on": "push", - "secrets": map[string]any{ - "API_TOKEN": "${{ secrets.API_TOKEN }}", - "DB_URL": map[string]any{ - "value": "${{ secrets.DB_URL }}", - "description": "Database connection string", - }, - }, - }, - wantErr: false, - }, - { - name: "invalid secrets - object missing required value field", - frontmatter: map[string]any{ - "on": "push", - "secrets": map[string]any{ - "API_TOKEN": map[string]any{ - "description": "Missing value field", - }, - }, - }, - wantErr: true, - errContains: "missing property 'value'", - }, - { - name: "invalid secrets - object with additional properties", - frontmatter: map[string]any{ - "on": "push", - "secrets": map[string]any{ - "API_TOKEN": map[string]any{ - "value": "${{ secrets.API_TOKEN }}", - "invalid": "field", - }, - }, - }, - wantErr: true, - errContains: "additional properties 'invalid' not allowed", - }, - { - name: "invalid secrets - array", - frontmatter: map[string]any{ - "on": "push", - "secrets": []string{"invalid"}, - }, - wantErr: true, - errContains: "got array, want object", - }, - { - name: "invalid secrets - non-string, non-object value", - frontmatter: map[string]any{ - "on": "push", - "secrets": map[string]any{ - "API_TOKEN": 123, - }, - }, - wantErr: true, - errContains: "oneOf", - }, - - // Runs-on field tests - { - name: "valid runs-on - simple string", - frontmatter: map[string]any{ - "on": "push", - "runs-on": "ubuntu-latest", - }, - wantErr: false, - }, - { - name: "valid runs-on - array of strings", - frontmatter: map[string]any{ - "on": "push", - "runs-on": []string{"ubuntu-latest", "self-hosted"}, - }, - wantErr: false, - }, - { - name: "valid runs-on - object with labels", - frontmatter: map[string]any{ - "on": "push", - "runs-on": map[string]any{ - "labels": []string{"ubuntu-latest"}, - }, - }, - wantErr: false, - }, - { - name: "valid runs-on - object with group and labels", - frontmatter: map[string]any{ - "on": "push", - "runs-on": map[string]any{ - "group": "larger-runners", - "labels": []string{"ubuntu-latest-8-cores"}, - }, - }, - wantErr: false, - }, - { - name: "invalid runs-on - number", - frontmatter: map[string]any{ - "on": "push", - "runs-on": 123, - }, - wantErr: true, - errContains: "oneOf", - }, - { - name: "invalid runs-on - object with invalid field", - frontmatter: map[string]any{ - "on": "push", - "runs-on": map[string]any{ - "labels": []string{"ubuntu-latest"}, - "invalid": "field", - }, - }, - wantErr: true, - errContains: "additional properties 'invalid' not allowed", - }, - - // Services field tests - { - name: "valid services - object with service names", - frontmatter: map[string]any{ - "on": "push", - "services": map[string]any{ - "postgres": "postgres:14", - }, - }, - wantErr: false, - }, - { - name: "valid services - object with service configuration", - frontmatter: map[string]any{ - "on": "push", - "services": map[string]any{ - "postgres": map[string]any{ - "image": "postgres:14", - "env": map[string]any{ - "POSTGRES_PASSWORD": "${{ secrets.DB_PASSWORD }}", - }, - }, - }, - }, - wantErr: false, - }, - { - name: "invalid services - string", - frontmatter: map[string]any{ - "on": "push", - "services": "invalid", - }, - wantErr: true, - errContains: "got string, want object", - }, - { - name: "invalid services - array", - frontmatter: map[string]any{ - "on": "push", - "services": []string{"invalid"}, - }, - wantErr: true, - errContains: "got array, want object", - }, - { - name: "invalid services - service object missing required image", - frontmatter: map[string]any{ - "on": "push", - "services": map[string]any{ - "postgres": map[string]any{ - "env": map[string]any{ - "POSTGRES_PASSWORD": "secret", - }, - }, - }, - }, - wantErr: true, - errContains: "missing property 'image'", - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - err := ValidateMainWorkflowFrontmatterWithSchema(tt.frontmatter) - - if tt.wantErr && err == nil { - t.Errorf("Expected error but got none") - return - } - - if !tt.wantErr && err != nil { - t.Errorf("Unexpected error: %v", err) - return - } - - if tt.wantErr && err != nil && tt.errContains != "" { - if !strings.Contains(err.Error(), tt.errContains) { - t.Errorf("Error = %v, expected to contain %q", err, tt.errContains) - } - } - }) - } -} - -// TestPassThroughFieldEdgeCases tests additional edge cases for pass-through fields -func TestPassThroughFieldEdgeCases(t *testing.T) { - tests := []struct { - name string - frontmatter map[string]any - wantErr bool - errContains string - }{ - { - name: "concurrency with expression in group", - frontmatter: map[string]any{ - "on": "push", - "concurrency": map[string]any{ - "group": "workflow-${{ github.ref }}", - }, - }, - wantErr: false, - }, - { - name: "runs-on with empty labels array is valid", - frontmatter: map[string]any{ - "on": "push", - "runs-on": map[string]any{ - "labels": []string{}, - }, - }, - wantErr: false, - }, - { - name: "container with all optional fields", - frontmatter: map[string]any{ - "on": "push", - "container": map[string]any{ - "image": "ubuntu:latest", - "env": map[string]any{ - "TEST": "value", - }, - "ports": []any{8080, "9090"}, - "volumes": []string{"/tmp:/tmp"}, - "options": "--cpus 1", - }, - }, - wantErr: false, - }, - { - name: "environment with expression in url", - frontmatter: map[string]any{ - "on": "push", - "environment": map[string]any{ - "name": "staging", - "url": "${{ steps.deploy.outputs.url }}", - }, - }, - wantErr: false, - }, - { - name: "services with credentials", - frontmatter: map[string]any{ - "on": "push", - "services": map[string]any{ - "redis": map[string]any{ - "image": "redis:alpine", - "credentials": map[string]any{ - "username": "user", - "password": "${{ secrets.PASSWORD }}", - }, - }, - }, - }, - wantErr: false, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - err := ValidateMainWorkflowFrontmatterWithSchema(tt.frontmatter) - - if tt.wantErr && err == nil { - t.Errorf("Expected error but got none") - return - } - - if !tt.wantErr && err != nil { - t.Errorf("Unexpected error: %v", err) - return - } - - if tt.wantErr && err != nil && tt.errContains != "" { - if !strings.Contains(err.Error(), tt.errContains) { - t.Errorf("Error = %v, expected to contain %q", err, tt.errContains) - } - } - }) - } -} diff --git a/pkg/parser/schema_test.go b/pkg/parser/schema_test.go index d4d35f47921..a9aaa1f6695 100644 --- a/pkg/parser/schema_test.go +++ b/pkg/parser/schema_test.go @@ -8,1734 +8,6 @@ import ( "testing" ) -func TestValidateMainWorkflowFrontmatterWithSchema(t *testing.T) { - tests := []struct { - name string - frontmatter map[string]any - wantErr bool - errContains string - }{ - { - name: "valid frontmatter with all allowed keys", - frontmatter: map[string]any{ - "on": map[string]any{ - "push": map[string]any{ - "branches": []string{"main"}, - }, - "stop-after": "2024-12-31", - }, - "permissions": "read-all", - "run-name": "Test Run", - "runs-on": "ubuntu-latest", - "timeout-minutes": 30, - "concurrency": "test", - "env": map[string]string{"TEST": "value"}, - "if": "true", - "steps": []string{"step1"}, - "engine": "claude", - "tools": map[string]any{"github": "test"}, - "command": "test-workflow", - }, - wantErr: false, - }, - { - name: "valid frontmatter with subset of keys", - frontmatter: map[string]any{ - "on": "push", - "engine": "claude", - }, - wantErr: false, - }, - { - name: "empty frontmatter - missing required 'on' field", - frontmatter: map[string]any{}, - wantErr: true, - errContains: "missing property 'on'", - }, - { - name: "valid engine string format - claude", - frontmatter: map[string]any{ - "on": "push", - "engine": "claude", - }, - wantErr: false, - }, - { - name: "valid engine string format - codex", - frontmatter: map[string]any{ - "on": "push", - "engine": "codex", - }, - wantErr: false, - }, - { - name: "valid engine object format - minimal", - frontmatter: map[string]any{ - "on": "push", - "engine": map[string]any{ - "id": "claude", - }, - }, - wantErr: false, - }, - { - name: "valid engine object format - with version", - frontmatter: map[string]any{ - "on": "push", - "engine": map[string]any{ - "id": "claude", - "version": "beta", - }, - }, - wantErr: false, - }, - { - name: "valid engine object format - with model", - frontmatter: map[string]any{ - "on": "push", - "engine": map[string]any{ - "id": "codex", - "model": "gpt-4o", - }, - }, - wantErr: false, - }, - { - name: "valid engine object format - complete", - frontmatter: map[string]any{ - "on": "push", - "engine": map[string]any{ - "id": "claude", - "version": "beta", - "model": "claude-3-5-sonnet-20241022", - }, - }, - wantErr: false, - }, - { - name: "invalid engine string format", - frontmatter: map[string]any{ - "on": "push", - "engine": "invalid-engine", - }, - wantErr: true, - errContains: "value must be one of 'claude', 'codex'", - }, - { - name: "invalid engine object format - invalid id", - frontmatter: map[string]any{ - "on": "push", - "engine": map[string]any{ - "id": "invalid-engine", - }, - }, - wantErr: true, - errContains: "value must be one of 'claude', 'codex'", - }, - { - name: "invalid engine object format - missing id", - frontmatter: map[string]any{ - "on": "push", - "engine": map[string]any{ - "version": "beta", - "model": "gpt-4o", - }, - }, - wantErr: true, - errContains: "missing property 'id'", - }, - { - name: "invalid engine object format - additional properties", - frontmatter: map[string]any{ - "on": "push", - "engine": map[string]any{ - "id": "claude", - "invalid": "property", - }, - }, - wantErr: true, - errContains: "additional properties", - }, - { - name: "invalid frontmatter with unexpected key", - frontmatter: map[string]any{ - "on": "push", - "invalid_key": "value", - }, - wantErr: true, - errContains: "additional properties 'invalid_key' not allowed", - }, - { - name: "invalid frontmatter with multiple unexpected keys", - frontmatter: map[string]any{ - "on": "push", - "invalid_key": "value", - "another_invalid": "value2", - }, - wantErr: true, - errContains: "additional properties", - }, - { - name: "valid frontmatter with complex on object", - frontmatter: map[string]any{ - "on": map[string]any{ - "schedule": []map[string]any{ - {"cron": "0 9 * * *"}, - }, - "workflow_dispatch": map[string]any{}, - }, - "engine": "claude", - }, - wantErr: false, - }, - { - name: "valid frontmatter with command trigger", - frontmatter: map[string]any{ - "on": map[string]any{ - "command": map[string]any{ - "name": "test-command", - }, - }, - "permissions": map[string]any{ - "issues": "write", - "contents": "read", - }, - }, - wantErr: false, - }, - { - name: "valid frontmatter with discussion trigger", - frontmatter: map[string]any{ - "on": map[string]any{ - "discussion": map[string]any{ - "types": []string{"created", "edited", "answered"}, - }, - }, - "permissions": "read-all", - }, - wantErr: false, - }, - { - name: "valid frontmatter with discussion_comment trigger", - frontmatter: map[string]any{ - "on": map[string]any{ - "discussion_comment": map[string]any{ - "types": []string{"created", "edited"}, - }, - }, - "permissions": "read-all", - }, - wantErr: false, - }, - { - name: "valid frontmatter with simple discussion trigger", - frontmatter: map[string]any{ - "on": "discussion", - "engine": "claude", - }, - wantErr: false, - }, - { - name: "valid frontmatter with branch_protection_rule trigger", - frontmatter: map[string]any{ - "on": map[string]any{ - "branch_protection_rule": map[string]any{ - "types": []string{"created", "deleted"}, - }, - }, - "permissions": "read-all", - }, - wantErr: false, - }, - { - name: "valid frontmatter with check_run trigger", - frontmatter: map[string]any{ - "on": map[string]any{ - "check_run": map[string]any{ - "types": []string{"completed", "rerequested"}, - }, - }, - "permissions": "read-all", - }, - wantErr: false, - }, - { - name: "valid frontmatter with check_suite trigger", - frontmatter: map[string]any{ - "on": map[string]any{ - "check_suite": map[string]any{ - "types": []string{"completed"}, - }, - }, - "permissions": "read-all", - }, - wantErr: false, - }, - { - name: "valid frontmatter with simple create trigger", - frontmatter: map[string]any{ - "on": "create", - "engine": "claude", - }, - wantErr: false, - }, - { - name: "valid frontmatter with simple delete trigger", - frontmatter: map[string]any{ - "on": "delete", - "engine": "claude", - }, - wantErr: false, - }, - { - name: "valid frontmatter with simple fork trigger", - frontmatter: map[string]any{ - "on": "fork", - "engine": "claude", - }, - wantErr: false, - }, - { - name: "valid frontmatter with simple gollum trigger", - frontmatter: map[string]any{ - "on": "gollum", - "engine": "claude", - }, - wantErr: false, - }, - { - name: "valid frontmatter with label trigger", - frontmatter: map[string]any{ - "on": map[string]any{ - "label": map[string]any{ - "types": []string{"created", "deleted"}, - }, - }, - "permissions": "read-all", - }, - wantErr: false, - }, - { - name: "valid frontmatter with merge_group trigger", - frontmatter: map[string]any{ - "on": map[string]any{ - "merge_group": map[string]any{ - "types": []string{"checks_requested"}, - }, - }, - "permissions": "read-all", - }, - wantErr: false, - }, - { - name: "valid frontmatter with milestone trigger", - frontmatter: map[string]any{ - "on": map[string]any{ - "milestone": map[string]any{ - "types": []string{"opened", "closed"}, - }, - }, - "permissions": "read-all", - }, - wantErr: false, - }, - { - name: "valid frontmatter with simple page_build trigger", - frontmatter: map[string]any{ - "on": "page_build", - "engine": "claude", - }, - wantErr: false, - }, - { - name: "valid frontmatter with simple public trigger", - frontmatter: map[string]any{ - "on": "public", - "engine": "claude", - }, - wantErr: false, - }, - { - name: "valid frontmatter with pull_request_target trigger", - frontmatter: map[string]any{ - "on": map[string]any{ - "pull_request_target": map[string]any{ - "types": []string{"opened", "synchronize"}, - "branches": []string{"main"}, - }, - }, - "permissions": "read-all", - }, - wantErr: false, - }, - { - name: "valid frontmatter with pull_request_review trigger", - frontmatter: map[string]any{ - "on": map[string]any{ - "pull_request_review": map[string]any{ - "types": []string{"submitted", "edited"}, - }, - }, - "permissions": "read-all", - }, - wantErr: false, - }, - { - name: "valid frontmatter with registry_package trigger", - frontmatter: map[string]any{ - "on": map[string]any{ - "registry_package": map[string]any{ - "types": []string{"published", "updated"}, - }, - }, - "permissions": "read-all", - }, - wantErr: false, - }, - { - name: "valid frontmatter with repository_dispatch trigger", - frontmatter: map[string]any{ - "on": map[string]any{ - "repository_dispatch": map[string]any{ - "types": []string{"custom-event", "deploy"}, - }, - }, - "permissions": "read-all", - }, - wantErr: false, - }, - { - name: "valid frontmatter with simple status trigger", - frontmatter: map[string]any{ - "on": "status", - "engine": "claude", - }, - wantErr: false, - }, - { - name: "valid frontmatter with watch trigger", - frontmatter: map[string]any{ - "on": map[string]any{ - "watch": map[string]any{ - "types": []string{"started"}, - }, - }, - "permissions": "read-all", - }, - wantErr: false, - }, - { - name: "valid frontmatter with simple workflow_call trigger", - frontmatter: map[string]any{ - "on": "workflow_call", - "engine": "claude", - }, - wantErr: false, - }, - { - name: "valid frontmatter with updated issues trigger types", - frontmatter: map[string]any{ - "on": map[string]any{ - "issues": map[string]any{ - "types": []string{"opened", "typed", "untyped"}, - }, - }, - "permissions": "read-all", - }, - wantErr: false, - }, - { - name: "valid frontmatter with issues trigger lock-for-agent field", - frontmatter: map[string]any{ - "on": map[string]any{ - "issues": map[string]any{ - "types": []string{"opened"}, - "lock-for-agent": true, - }, - }, - "permissions": "read-all", - }, - wantErr: false, - }, - { - name: "valid frontmatter with issues trigger lock-for-agent false", - frontmatter: map[string]any{ - "on": map[string]any{ - "issues": map[string]any{ - "types": []string{"opened"}, - "lock-for-agent": false, - }, - }, - "permissions": "read-all", - }, - wantErr: false, - }, - { - name: "valid frontmatter with issue_comment trigger lock-for-agent field", - frontmatter: map[string]any{ - "on": map[string]any{ - "issue_comment": map[string]any{ - "types": []string{"created"}, - "lock-for-agent": true, - }, - }, - "permissions": "read-all", - }, - wantErr: false, - }, - { - name: "valid frontmatter with issue_comment trigger lock-for-agent false", - frontmatter: map[string]any{ - "on": map[string]any{ - "issue_comment": map[string]any{ - "types": []string{"created"}, - "lock-for-agent": false, - }, - }, - "permissions": "read-all", - }, - wantErr: false, - }, - { - name: "valid frontmatter with updated pull_request trigger types", - frontmatter: map[string]any{ - "on": map[string]any{ - "pull_request": map[string]any{ - "types": []string{"opened", "milestoned", "demilestoned", "ready_for_review", "auto_merge_enabled"}, - }, - }, - "permissions": "read-all", - }, - wantErr: false, - }, - { - name: "valid frontmatter with detailed permissions", - frontmatter: map[string]any{ - "on": "push", - "permissions": map[string]any{ - "contents": "read", - "issues": "write", - "pull-requests": "read", - "models": "read", - }, - }, - wantErr: false, - }, - { - name: "valid frontmatter with single cache configuration", - frontmatter: map[string]any{ - "on": "push", - "cache": map[string]any{ - "key": "node-modules-${{ hashFiles('package-lock.json') }}", - "path": "node_modules", - "restore-keys": []string{"node-modules-"}, - }, - }, - wantErr: false, - }, - { - name: "valid frontmatter with multiple cache configurations", - frontmatter: map[string]any{ - "on": "push", - "cache": []any{ - map[string]any{ - "key": "cache1", - "path": "path1", - }, - map[string]any{ - "key": "cache2", - "path": []string{"path2", "path3"}, - "restore-keys": "restore-key", - "fail-on-cache-miss": true, - }, - }, - }, - wantErr: false, - }, - { - name: "invalid cache configuration missing required key", - frontmatter: map[string]any{ - "cache": map[string]any{ - "path": "node_modules", - }, - }, - wantErr: true, - errContains: "missing property 'key'", - }, - // Test cases for additional properties validation - { - name: "invalid permissions with additional property", - frontmatter: map[string]any{ - "on": "push", - "permissions": map[string]any{ - "contents": "read", - "invalid_perm": "write", - }, - }, - wantErr: true, - errContains: "additional properties 'invalid_perm' not allowed", - }, - { - name: "invalid on trigger with additional properties", - frontmatter: map[string]any{ - "on": map[string]any{ - "push": map[string]any{ - "branches": []string{"main"}, - "invalid_prop": "value", - }, - }, - }, - wantErr: true, - errContains: "additional properties 'invalid_prop' not allowed", - }, - { - name: "invalid schedule with additional properties", - frontmatter: map[string]any{ - "on": map[string]any{ - "schedule": []map[string]any{ - { - "cron": "0 9 * * *", - "invalid_prop": "value", - }, - }, - }, - }, - wantErr: true, - errContains: "additional properties 'invalid_prop' not allowed", - }, - { - name: "invalid empty schedule array", - frontmatter: map[string]any{ - "on": map[string]any{ - "schedule": []map[string]any{}, - }, - }, - wantErr: true, - errContains: "minItems: got 0, want 1", - }, - { - name: "invalid empty toolsets array", - frontmatter: map[string]any{ - "on": "push", - "tools": map[string]any{ - "github": map[string]any{ - "toolsets": []string{}, - }, - }, - }, - wantErr: true, - errContains: "minItems", - }, - { - name: "invalid empty issue names array", - frontmatter: map[string]any{ - "on": map[string]any{ - "issues": map[string]any{ - "types": []string{"labeled"}, - "names": []string{}, - }, - }, - }, - wantErr: true, - errContains: "minItems", - }, - { - name: "invalid empty pull_request names array", - frontmatter: map[string]any{ - "on": map[string]any{ - "pull_request": map[string]any{ - "types": []string{"labeled"}, - "names": []string{}, - }, - }, - }, - wantErr: true, - errContains: "minItems", - }, - { - name: "valid schedule with multiple cron entries", - frontmatter: map[string]any{ - "on": map[string]any{ - "schedule": []map[string]any{ - {"cron": "0 9 * * *"}, - {"cron": "0 17 * * *"}, - }, - }, - "engine": "claude", - }, - wantErr: false, - }, - { - name: "invalid workflow_dispatch with additional properties", - frontmatter: map[string]any{ - "on": map[string]any{ - "workflow_dispatch": map[string]any{ - "inputs": map[string]any{ - "test_input": map[string]any{ - "description": "Test input", - "type": "string", - }, - }, - "invalid_prop": "value", - }, - }, - }, - wantErr: true, - errContains: "additional properties 'invalid_prop' not allowed", - }, - { - name: "invalid concurrency with additional properties", - frontmatter: map[string]any{ - "concurrency": map[string]any{ - "group": "test-group", - "cancel-in-progress": true, - "invalid_prop": "value", - }, - }, - wantErr: true, - errContains: "additional properties 'invalid_prop' not allowed", - }, - { - name: "invalid runs-on object with additional properties", - frontmatter: map[string]any{ - "runs-on": map[string]any{ - "group": "test-group", - "labels": []string{"ubuntu-latest"}, - "invalid_prop": "value", - }, - }, - wantErr: true, - errContains: "additional properties 'invalid_prop' not allowed", - }, - { - name: "invalid github tools with additional properties", - frontmatter: map[string]any{ - "tools": map[string]any{ - "github": map[string]any{ - "allowed": []string{"create_issue"}, - "invalid_prop": "value", - }, - }, - }, - wantErr: true, - errContains: "additional properties 'invalid_prop' not allowed", - }, - { - name: "invalid claude top-level field (deprecated)", - frontmatter: map[string]any{ - "claude": map[string]any{ - "model": "claude-3", - }, - }, - wantErr: true, - errContains: "additional properties 'claude' not allowed", - }, - { - name: "invalid safe-outputs configuration with additional properties", - frontmatter: map[string]any{ - "safe-outputs": map[string]any{ - "create-issue": map[string]any{ - "title-prefix": "[ai] ", - "invalid_prop": "value", - }, - }, - }, - wantErr: true, - errContains: "additional properties 'invalid_prop' not allowed", - }, - { - name: "valid permissions with repository-projects property", - frontmatter: map[string]any{ - "on": "push", - "permissions": map[string]any{ - "contents": "read", - "attestations": "write", - "id-token": "write", - "packages": "read", - "pages": "write", - "repository-projects": "none", - }, - }, - wantErr: false, - }, - { - name: "valid permissions with organization-projects property", - frontmatter: map[string]any{ - "on": "push", - "permissions": map[string]any{ - "contents": "read", - "organization-projects": "write", - }, - }, - wantErr: false, - }, - { - name: "valid claude engine with network permissions", - frontmatter: map[string]any{ - "on": "push", - "engine": map[string]any{ - "id": "claude", - }, - }, - wantErr: false, - }, - { - name: "valid codex engine without permissions", - frontmatter: map[string]any{ - "on": "push", - "engine": map[string]any{ - "id": "codex", - "model": "gpt-4o", - }, - }, - wantErr: false, - }, - { - name: "valid codex string engine (no permissions possible)", - frontmatter: map[string]any{ - "on": "push", - "engine": "codex", - }, - wantErr: false, - }, - { - name: "valid network defaults", - frontmatter: map[string]any{ - "on": "push", - "network": "defaults", - }, - wantErr: false, - }, - { - name: "valid network empty object", - frontmatter: map[string]any{ - "on": "push", - "network": map[string]any{}, - }, - wantErr: false, - }, - { - name: "valid network with allowed domains", - frontmatter: map[string]any{ - "on": "push", - "network": map[string]any{ - "allowed": []string{"example.com", "*.trusted.com"}, - }, - }, - wantErr: false, - }, - { - name: "invalid network string (not defaults)", - frontmatter: map[string]any{ - "on": "push", - "network": "invalid", - }, - wantErr: true, - errContains: "oneOf", - }, - { - name: "invalid network object with unknown property", - frontmatter: map[string]any{ - "on": "push", - "network": map[string]any{ - "invalid": []string{"example.com"}, - }, - }, - wantErr: true, - errContains: "additional properties 'invalid' not allowed", - }, - { - name: "missing required on field", - frontmatter: map[string]any{ - "engine": "claude", - "permissions": map[string]any{ - "contents": "read", - }, - }, - wantErr: true, - errContains: "missing property 'on'", - }, - { - name: "missing required on field with other valid fields", - frontmatter: map[string]any{ - "engine": "copilot", - "timeout-minutes": 30, - "permissions": map[string]any{ - "issues": "write", - }, - }, - wantErr: true, - errContains: "missing property 'on'", - }, - { - name: "invalid: command trigger with issues event", - frontmatter: map[string]any{ - "on": map[string]any{ - "command": map[string]any{ - "name": "test-bot", - }, - "issues": map[string]any{ - "types": []string{"opened"}, - }, - }, - }, - wantErr: true, - errContains: "command trigger cannot be used with 'issues' event", - }, - { - name: "invalid: command trigger with issue_comment event", - frontmatter: map[string]any{ - "on": map[string]any{ - "command": "test-bot", - "issue_comment": map[string]any{ - "types": []string{"created"}, - }, - }, - }, - wantErr: true, - errContains: "command trigger cannot be used with 'issue_comment' event", - }, - { - name: "invalid: command trigger with pull_request event", - frontmatter: map[string]any{ - "on": map[string]any{ - "command": map[string]any{ - "name": "test-bot", - }, - "pull_request": map[string]any{ - "types": []string{"opened"}, - }, - }, - }, - wantErr: true, - errContains: "command trigger cannot be used with 'pull_request' event", - }, - { - name: "invalid: command trigger with pull_request_review_comment event", - frontmatter: map[string]any{ - "on": map[string]any{ - "command": "test-bot", - "pull_request_review_comment": map[string]any{ - "types": []string{"created"}, - }, - }, - }, - wantErr: true, - errContains: "command trigger cannot be used with 'pull_request_review_comment' event", - }, - { - name: "invalid: command trigger with multiple conflicting events", - frontmatter: map[string]any{ - "on": map[string]any{ - "command": map[string]any{ - "name": "test-bot", - }, - "issues": map[string]any{ - "types": []string{"opened"}, - }, - "pull_request": map[string]any{ - "types": []string{"opened"}, - }, - }, - }, - wantErr: true, - errContains: "command trigger cannot be used with these events", - }, - { - name: "valid: command trigger with non-conflicting events", - frontmatter: map[string]any{ - "on": map[string]any{ - "command": map[string]any{ - "name": "test-bot", - }, - "workflow_dispatch": nil, - "schedule": []map[string]any{ - {"cron": "0 0 * * *"}, - }, - }, - }, - wantErr: false, - }, - { - name: "valid: command trigger alone", - frontmatter: map[string]any{ - "on": map[string]any{ - "command": "test-bot", - }, - }, - wantErr: false, - }, - { - name: "valid: command trigger as null (default workflow name)", - frontmatter: map[string]any{ - "on": map[string]any{ - "command": nil, - }, - }, - wantErr: false, - }, - { - name: "valid: issues event without command", - frontmatter: map[string]any{ - "on": map[string]any{ - "issues": map[string]any{ - "types": []string{"opened"}, - }, - }, - }, - wantErr: false, - }, - { - name: "invalid: empty string for name field", - frontmatter: map[string]any{ - "on": "push", - "name": "", - }, - wantErr: true, - errContains: "minLength", - }, - { - name: "invalid: empty string for on field (string format)", - frontmatter: map[string]any{ - "on": "", - }, - wantErr: true, - errContains: "minLength", - }, - { - name: "invalid: empty string for command trigger (string format)", - frontmatter: map[string]any{ - "on": map[string]any{ - "command": "", - }, - }, - wantErr: true, - errContains: "minLength", - }, - { - name: "invalid: empty string for command.name field", - frontmatter: map[string]any{ - "on": map[string]any{ - "command": map[string]any{ - "name": "", - }, - }, - }, - wantErr: true, - errContains: "minLength", - }, - { - name: "invalid: command name starting with slash (string format)", - frontmatter: map[string]any{ - "on": map[string]any{ - "command": "/mybot", - }, - }, - wantErr: true, - errContains: "pattern", - }, - { - name: "invalid: command.name starting with slash (object format)", - frontmatter: map[string]any{ - "on": map[string]any{ - "command": map[string]any{ - "name": "/mybot", - }, - }, - }, - wantErr: true, - errContains: "pattern", - }, - { - name: "valid: command name without slash (string format)", - frontmatter: map[string]any{ - "on": map[string]any{ - "command": "mybot", - }, - }, - wantErr: false, - }, - { - name: "valid: command.name without slash (object format)", - frontmatter: map[string]any{ - "on": map[string]any{ - "command": map[string]any{ - "name": "mybot", - }, - }, - }, - wantErr: false, - }, - { - name: "invalid: empty events array for command trigger", - frontmatter: map[string]any{ - "on": map[string]any{ - "command": map[string]any{ - "name": "test-bot", - "events": []any{}, - }, - }, - }, - wantErr: true, - errContains: "minItems", - }, - { - name: "valid: workflow_dispatch with 25 inputs (max allowed)", - frontmatter: map[string]any{ - "on": map[string]any{ - "workflow_dispatch": map[string]any{ - "inputs": map[string]any{ - "input1": map[string]any{"description": "Input 1", "type": "string"}, - "input2": map[string]any{"description": "Input 2", "type": "string"}, - "input3": map[string]any{"description": "Input 3", "type": "string"}, - "input4": map[string]any{"description": "Input 4", "type": "string"}, - "input5": map[string]any{"description": "Input 5", "type": "string"}, - "input6": map[string]any{"description": "Input 6", "type": "string"}, - "input7": map[string]any{"description": "Input 7", "type": "string"}, - "input8": map[string]any{"description": "Input 8", "type": "string"}, - "input9": map[string]any{"description": "Input 9", "type": "string"}, - "input10": map[string]any{"description": "Input 10", "type": "string"}, - "input11": map[string]any{"description": "Input 11", "type": "string"}, - "input12": map[string]any{"description": "Input 12", "type": "string"}, - "input13": map[string]any{"description": "Input 13", "type": "string"}, - "input14": map[string]any{"description": "Input 14", "type": "string"}, - "input15": map[string]any{"description": "Input 15", "type": "string"}, - "input16": map[string]any{"description": "Input 16", "type": "string"}, - "input17": map[string]any{"description": "Input 17", "type": "string"}, - "input18": map[string]any{"description": "Input 18", "type": "string"}, - "input19": map[string]any{"description": "Input 19", "type": "string"}, - "input20": map[string]any{"description": "Input 20", "type": "string"}, - "input21": map[string]any{"description": "Input 21", "type": "string"}, - "input22": map[string]any{"description": "Input 22", "type": "string"}, - "input23": map[string]any{"description": "Input 23", "type": "string"}, - "input24": map[string]any{"description": "Input 24", "type": "string"}, - "input25": map[string]any{"description": "Input 25", "type": "string"}, - }, - }, - }, - }, - wantErr: false, - }, - { - name: "invalid: workflow_dispatch with 26 inputs (exceeds max)", - frontmatter: map[string]any{ - "on": map[string]any{ - "workflow_dispatch": map[string]any{ - "inputs": map[string]any{ - "input1": map[string]any{"description": "Input 1", "type": "string"}, - "input2": map[string]any{"description": "Input 2", "type": "string"}, - "input3": map[string]any{"description": "Input 3", "type": "string"}, - "input4": map[string]any{"description": "Input 4", "type": "string"}, - "input5": map[string]any{"description": "Input 5", "type": "string"}, - "input6": map[string]any{"description": "Input 6", "type": "string"}, - "input7": map[string]any{"description": "Input 7", "type": "string"}, - "input8": map[string]any{"description": "Input 8", "type": "string"}, - "input9": map[string]any{"description": "Input 9", "type": "string"}, - "input10": map[string]any{"description": "Input 10", "type": "string"}, - "input11": map[string]any{"description": "Input 11", "type": "string"}, - "input12": map[string]any{"description": "Input 12", "type": "string"}, - "input13": map[string]any{"description": "Input 13", "type": "string"}, - "input14": map[string]any{"description": "Input 14", "type": "string"}, - "input15": map[string]any{"description": "Input 15", "type": "string"}, - "input16": map[string]any{"description": "Input 16", "type": "string"}, - "input17": map[string]any{"description": "Input 17", "type": "string"}, - "input18": map[string]any{"description": "Input 18", "type": "string"}, - "input19": map[string]any{"description": "Input 19", "type": "string"}, - "input20": map[string]any{"description": "Input 20", "type": "string"}, - "input21": map[string]any{"description": "Input 21", "type": "string"}, - "input22": map[string]any{"description": "Input 22", "type": "string"}, - "input23": map[string]any{"description": "Input 23", "type": "string"}, - "input24": map[string]any{"description": "Input 24", "type": "string"}, - "input25": map[string]any{"description": "Input 25", "type": "string"}, - "input26": map[string]any{"description": "Input 26", "type": "string"}, - }, - }, - }, - }, - wantErr: true, - errContains: "maxProperties", - }, - { - name: "valid: workflow_dispatch with all valid input types", - frontmatter: map[string]any{ - "on": map[string]any{ - "workflow_dispatch": map[string]any{ - "inputs": map[string]any{ - "string_input": map[string]any{ - "description": "String input", - "type": "string", - "default": "default value", - }, - "choice_input": map[string]any{ - "description": "Choice input", - "type": "choice", - "options": []string{"option1", "option2", "option3"}, - "default": "option1", - }, - "boolean_input": map[string]any{ - "description": "Boolean input", - "type": "boolean", - "default": true, - }, - "number_input": map[string]any{ - "description": "Number input", - "type": "number", - "default": 42, - }, - "environment_input": map[string]any{ - "description": "Environment input", - "type": "environment", - "default": "production", - }, - }, - }, - }, - }, - wantErr: false, - }, - { - name: "invalid: workflow_dispatch with invalid input type 'text'", - frontmatter: map[string]any{ - "on": map[string]any{ - "workflow_dispatch": map[string]any{ - "inputs": map[string]any{ - "test_input": map[string]any{ - "description": "Test input", - "type": "text", - }, - }, - }, - }, - }, - wantErr: true, - errContains: "value must be one of 'string', 'choice', 'boolean', 'number', 'environment'", - }, - { - name: "invalid: workflow_dispatch with invalid input type 'int'", - frontmatter: map[string]any{ - "on": map[string]any{ - "workflow_dispatch": map[string]any{ - "inputs": map[string]any{ - "test_input": map[string]any{ - "description": "Test input", - "type": "int", - }, - }, - }, - }, - }, - wantErr: true, - errContains: "value must be one of 'string', 'choice', 'boolean', 'number', 'environment'", - }, - { - name: "invalid: workflow_dispatch with invalid input type 'bool'", - frontmatter: map[string]any{ - "on": map[string]any{ - "workflow_dispatch": map[string]any{ - "inputs": map[string]any{ - "test_input": map[string]any{ - "description": "Test input", - "type": "bool", - }, - }, - }, - }, - }, - wantErr: true, - errContains: "value must be one of 'string', 'choice', 'boolean', 'number', 'environment'", - }, - { - name: "invalid: workflow_dispatch with invalid input type 'select'", - frontmatter: map[string]any{ - "on": map[string]any{ - "workflow_dispatch": map[string]any{ - "inputs": map[string]any{ - "test_input": map[string]any{ - "description": "Test input", - "type": "select", - }, - }, - }, - }, - }, - wantErr: true, - errContains: "value must be one of 'string', 'choice', 'boolean', 'number', 'environment'", - }, - { - name: "invalid: workflow_dispatch with invalid input type 'dropdown'", - frontmatter: map[string]any{ - "on": map[string]any{ - "workflow_dispatch": map[string]any{ - "inputs": map[string]any{ - "test_input": map[string]any{ - "description": "Test input", - "type": "dropdown", - }, - }, - }, - }, - }, - wantErr: true, - errContains: "value must be one of 'string', 'choice', 'boolean', 'number', 'environment'", - }, - { - name: "invalid: workflow_dispatch with invalid input type 'checkbox'", - frontmatter: map[string]any{ - "on": map[string]any{ - "workflow_dispatch": map[string]any{ - "inputs": map[string]any{ - "test_input": map[string]any{ - "description": "Test input", - "type": "checkbox", - }, - }, - }, - }, - }, - wantErr: true, - errContains: "value must be one of 'string', 'choice', 'boolean', 'number', 'environment'", - }, - { - name: "valid metadata with various key-value pairs", - frontmatter: map[string]any{ - "on": "workflow_dispatch", - "metadata": map[string]any{ - "author": "John Doe", - "version": "1.0.0", - "category": "automation", - "description": "A workflow that automates something", - }, - }, - wantErr: false, - }, - { - name: "valid metadata with max length key (64 chars)", - frontmatter: map[string]any{ - "on": "workflow_dispatch", - "metadata": map[string]any{ - "a123456789b123456789c123456789d123456789e123456789f123456789abcd": "value", - }, - }, - wantErr: false, - }, - { - name: "valid metadata with max length value (1024 chars)", - frontmatter: map[string]any{ - "on": "workflow_dispatch", - "metadata": map[string]any{ - "long-value": strings.Repeat("a", 1024), - }, - }, - wantErr: false, - }, - { - name: "invalid metadata with key too long (65 chars)", - frontmatter: map[string]any{ - "on": "workflow_dispatch", - "metadata": map[string]any{ - "a123456789b123456789c123456789d123456789e123456789f123456789abcde": "value", - }, - }, - wantErr: true, - errContains: "additional properties", - }, - { - name: "invalid metadata with value too long (1025 chars)", - frontmatter: map[string]any{ - "on": "workflow_dispatch", - "metadata": map[string]any{ - "test": strings.Repeat("a", 1025), - }, - }, - wantErr: true, - errContains: "maxLength", - }, - { - name: "invalid metadata with non-string value", - frontmatter: map[string]any{ - "on": "workflow_dispatch", - "metadata": map[string]any{ - "count": 123, - }, - }, - wantErr: true, - errContains: "want string", - }, - { - name: "invalid metadata with empty key", - frontmatter: map[string]any{ - "on": "workflow_dispatch", - "metadata": map[string]any{ - "": "value", - }, - }, - wantErr: true, - errContains: "additional properties", - }, - { - name: "invalid name too long (257 chars)", - frontmatter: map[string]any{ - "on": "workflow_dispatch", - "name": strings.Repeat("a", 257), - }, - wantErr: true, - errContains: "maxLength", - }, - { - name: "valid name at max length (256 chars)", - frontmatter: map[string]any{ - "on": "workflow_dispatch", - "name": strings.Repeat("a", 256), - }, - wantErr: false, - }, - { - name: "invalid description too long (10001 chars)", - frontmatter: map[string]any{ - "on": "workflow_dispatch", - "description": strings.Repeat("a", 10001), - }, - wantErr: true, - errContains: "maxLength", - }, - { - name: "valid description at max length (10000 chars)", - frontmatter: map[string]any{ - "on": "workflow_dispatch", - "description": strings.Repeat("a", 10000), - }, - wantErr: false, - }, - { - name: "invalid tracker-id too long (129 chars)", - frontmatter: map[string]any{ - "on": "workflow_dispatch", - "tracker-id": strings.Repeat("a", 129), - }, - wantErr: true, - errContains: "maxLength", - }, - { - name: "valid tracker-id at max length (128 chars)", - frontmatter: map[string]any{ - "on": "workflow_dispatch", - "tracker-id": strings.Repeat("a", 128), - }, - wantErr: false, - }, - // id-token permission validation - id-token only supports "write" and "none", not "read" - // See: https://docs.github.com/en/actions/using-jobs/assigning-permissions-to-jobs#defining-access-for-the-github_token-scopes - { - name: "invalid: id-token: read is not allowed (only write and none)", - frontmatter: map[string]any{ - "on": "workflow_dispatch", - "permissions": map[string]any{ - "id-token": "read", - }, - }, - wantErr: true, - errContains: "id-token", - }, - { - name: "valid: id-token: write is allowed", - frontmatter: map[string]any{ - "on": "workflow_dispatch", - "permissions": map[string]any{ - "id-token": "write", - }, - }, - wantErr: false, - }, - { - name: "valid: id-token: none is allowed", - frontmatter: map[string]any{ - "on": "workflow_dispatch", - "permissions": map[string]any{ - "id-token": "none", - }, - }, - wantErr: false, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - err := ValidateMainWorkflowFrontmatterWithSchema(tt.frontmatter) - - if tt.wantErr && err == nil { - t.Errorf("ValidateMainWorkflowFrontmatterWithSchema() expected error, got nil") - return - } - - if !tt.wantErr && err != nil { - t.Errorf("ValidateMainWorkflowFrontmatterWithSchema() error = %v", err) - return - } - - if tt.wantErr && err != nil && tt.errContains != "" { - if !strings.Contains(err.Error(), tt.errContains) { - t.Errorf("ValidateMainWorkflowFrontmatterWithSchema() error = %v, expected to contain %v", err, tt.errContains) - } - } - }) - } -} - -func TestValidateIncludedFileFrontmatterWithSchema(t *testing.T) { - tests := []struct { - name string - frontmatter map[string]any - wantErr bool - errContains string - }{ - { - name: "valid frontmatter with tools only", - frontmatter: map[string]any{ - "tools": map[string]any{"github": "test"}, - }, - wantErr: false, - }, - { - name: "empty frontmatter", - frontmatter: map[string]any{}, - wantErr: false, - }, - { - name: "invalid frontmatter with on trigger", - frontmatter: map[string]any{ - "on": "push", - "tools": map[string]any{"github": "test"}, - }, - wantErr: true, - errContains: "cannot be used in shared workflows", - }, - { - name: "invalid frontmatter with multiple unexpected keys", - frontmatter: map[string]any{ - "on": "push", - "permissions": "read-all", - "tools": map[string]any{"github": "test"}, - }, - wantErr: true, - errContains: "cannot be used in shared workflows", - }, - { - name: "invalid frontmatter with only unexpected keys", - frontmatter: map[string]any{ - "on": "push", - "permissions": "read-all", - }, - wantErr: true, - errContains: "cannot be used in shared workflows", - }, - { - name: "valid frontmatter with complex tools object", - frontmatter: map[string]any{ - "tools": map[string]any{ - "github": map[string]any{ - "allowed": []string{"list_issues", "issue_read"}, - }, - "bash": []string{"echo", "ls"}, - }, - }, - wantErr: false, - }, - { - name: "valid frontmatter with bash as boolean true", - frontmatter: map[string]any{ - "tools": map[string]any{ - "bash": true, - }, - }, - wantErr: false, - }, - { - name: "valid frontmatter with bash as boolean false", - frontmatter: map[string]any{ - "tools": map[string]any{ - "bash": false, - }, - }, - wantErr: false, - }, - { - name: "valid frontmatter with bash as null", - frontmatter: map[string]any{ - "tools": map[string]any{ - "bash": nil, - }, - }, - wantErr: false, - }, - { - name: "valid frontmatter with custom MCP tool", - frontmatter: map[string]any{ - "tools": map[string]any{ - "myTool": map[string]any{ - "mcp": map[string]any{ - "type": "http", - "url": "https://api.contoso.com", - "headers": map[string]any{"Authorization": "Bearer token"}, - }, - "allowed": []string{"api_call1", "api_call2"}, - }, - }, - }, - wantErr: false, - }, - { - name: "valid frontmatter with HTTP MCP tool with underscored headers", - frontmatter: map[string]any{ - "tools": map[string]any{ - "datadog": map[string]any{ - "type": "http", - "url": "https://mcp.datadoghq.com/api/unstable/mcp-server/mcp", - "headers": map[string]any{ - "DD_API_KEY": "test-key", - "DD_APPLICATION_KEY": "test-app", - "DD_SITE": "datadoghq.com", - }, - "allowed": []string{"get-monitors", "get-monitor"}, - }, - }, - }, - wantErr: false, - }, - { - name: "valid frontmatter with cache-memory as boolean true", - frontmatter: map[string]any{ - "tools": map[string]any{ - "cache-memory": true, - }, - }, - wantErr: false, - }, - { - name: "valid frontmatter with cache-memory as boolean false", - frontmatter: map[string]any{ - "tools": map[string]any{ - "cache-memory": false, - }, - }, - wantErr: false, - }, - { - name: "valid frontmatter with cache-memory as nil", - frontmatter: map[string]any{ - "tools": map[string]any{ - "cache-memory": nil, - }, - }, - wantErr: false, - }, - { - name: "valid frontmatter with cache-memory as object with key", - frontmatter: map[string]any{ - "tools": map[string]any{ - "cache-memory": map[string]any{ - "key": "custom-memory-${{ github.workflow }}", - }, - }, - }, - wantErr: false, - }, - { - name: "valid frontmatter with cache-memory with all valid options", - frontmatter: map[string]any{ - "tools": map[string]any{ - "cache-memory": map[string]any{ - "key": "custom-key", - "retention-days": 30, - "description": "Test cache description", - }, - }, - }, - wantErr: false, - }, - { - name: "invalid cache-memory with invalid retention-days (too low)", - frontmatter: map[string]any{ - "tools": map[string]any{ - "cache-memory": map[string]any{ - "retention-days": 0, - }, - }, - }, - wantErr: true, - errContains: "got 0, want 1", - }, - { - name: "invalid cache-memory with invalid retention-days (too high)", - frontmatter: map[string]any{ - "tools": map[string]any{ - "cache-memory": map[string]any{ - "retention-days": 91, - }, - }, - }, - wantErr: true, - errContains: "got 91, want 90", - }, - { - name: "invalid cache-memory with unsupported docker-image field", - frontmatter: map[string]any{ - "tools": map[string]any{ - "cache-memory": map[string]any{ - "docker-image": "custom/memory:latest", - }, - }, - }, - wantErr: true, - errContains: "additional properties 'docker-image' not allowed", - }, - { - name: "invalid cache-memory with additional property", - frontmatter: map[string]any{ - "tools": map[string]any{ - "cache-memory": map[string]any{ - "key": "custom-key", - "invalid_option": "value", - }, - }, - }, - wantErr: true, - errContains: "additional properties 'invalid_option' not allowed", - }, - { - name: "invalid: included file cannot have inputs at root level", - frontmatter: map[string]any{ - "inputs": map[string]any{ - "input1": map[string]any{"description": "Input 1", "type": "string"}, - }, - }, - wantErr: true, - errContains: "additional properties 'inputs' not allowed", - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - err := ValidateIncludedFileFrontmatterWithSchema(tt.frontmatter) - - if tt.wantErr && err == nil { - t.Errorf("ValidateIncludedFileFrontmatterWithSchema() expected error, got nil") - return - } - - if !tt.wantErr && err != nil { - t.Errorf("ValidateIncludedFileFrontmatterWithSchema() error = %v", err) - return - } - - if tt.wantErr && err != nil && tt.errContains != "" { - if !strings.Contains(err.Error(), tt.errContains) { - t.Errorf("ValidateIncludedFileFrontmatterWithSchema() error = %v, expected to contain %v", err, tt.errContains) - } - } - }) - } -} - func TestValidateWithSchema(t *testing.T) { tests := []struct { name string @@ -1864,82 +136,6 @@ timeout_minu tes: 10 } } -func TestValidateMCPConfigWithSchema(t *testing.T) { - tests := []struct { - name string - mcpConfig map[string]any - toolName string - wantErr bool - errContains string - }{ - { - name: "valid stdio MCP config with command", - mcpConfig: map[string]any{ - "type": "stdio", - "command": "npx", - "args": []string{"-y", "@modelcontextprotocol/server-memory"}, - }, - toolName: "memory", - wantErr: false, - }, - { - name: "valid http MCP config with url", - mcpConfig: map[string]any{ - "type": "http", - "url": "https://api.example.com/mcp", - }, - toolName: "api-server", - wantErr: false, - }, - { - name: "invalid: empty string for command field", - mcpConfig: map[string]any{ - "type": "stdio", - "command": "", - }, - toolName: "test-tool", - wantErr: true, - errContains: "minLength", - }, - { - name: "invalid: empty string for url field", - mcpConfig: map[string]any{ - "type": "http", - "url": "", - }, - toolName: "test-tool", - wantErr: true, - errContains: "minLength", - }, - { - name: "valid stdio MCP config with container", - mcpConfig: map[string]any{ - "type": "stdio", - "container": "ghcr.io/modelcontextprotocol/server-memory", - }, - toolName: "memory", - wantErr: false, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - err := ValidateMCPConfigWithSchema(tt.mcpConfig, tt.toolName) - - if (err != nil) != tt.wantErr { - t.Errorf("ValidateMCPConfigWithSchema() error = %v, wantErr %v", err, tt.wantErr) - return - } - - if err != nil && tt.errContains != "" { - if !strings.Contains(err.Error(), tt.errContains) { - t.Errorf("Error message should contain %q, got: %v", tt.errContains, err) - } - } - }) - } -} - // TestGetSafeOutputTypeKeys tests extracting safe output type keys from the embedded schema func TestGetSafeOutputTypeKeys(t *testing.T) { keys, err := GetSafeOutputTypeKeys() diff --git a/pkg/parser/schema_utilities_test.go b/pkg/parser/schema_utilities_test.go index 48bb52f7fda..1fc5d3003a0 100644 --- a/pkg/parser/schema_utilities_test.go +++ b/pkg/parser/schema_utilities_test.go @@ -3,7 +3,6 @@ package parser import ( - "strings" "testing" "github.com/github/gh-aw/pkg/constants" @@ -132,140 +131,3 @@ func TestFilterIgnoredFields(t *testing.T) { }) } } - -func TestValidateMainWorkflowWithIgnoredFields(t *testing.T) { - tests := []struct { - name string - frontmatter map[string]any - wantErr bool - errContains string - }{ - { - name: "valid frontmatter with description field - now properly validated", - frontmatter: map[string]any{ - "on": "push", - "description": "This is a test workflow description", - "engine": "claude", - }, - wantErr: false, - }, - { - name: "invalid frontmatter with applyTo field - not allowed in main workflow", - frontmatter: map[string]any{ - "on": "push", - "applyTo": "some-target", - "engine": "claude", - }, - wantErr: true, - errContains: "applyTo", - }, - { - name: "valid frontmatter with description - now properly validated", - frontmatter: map[string]any{ - "on": "push", - "description": "Test workflow", - "engine": "claude", - }, - wantErr: false, - }, - { - name: "valid frontmatter with user-invokable field - should be ignored", - frontmatter: map[string]any{ - "on": "push", - "user-invokable": true, - "engine": "claude", - }, - wantErr: false, - }, - { - name: "invalid frontmatter with ignored fields - other validation should still work", - frontmatter: map[string]any{ - "on": "push", - "description": "Test workflow", - "applyTo": "some-target", - "invalid_field": "should-fail", - }, - wantErr: true, - errContains: "invalid_field", - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - err := ValidateMainWorkflowFrontmatterWithSchema(tt.frontmatter) - - if (err != nil) != tt.wantErr { - t.Errorf("ValidateMainWorkflowFrontmatterWithSchema() error = %v, wantErr %v", err, tt.wantErr) - return - } - - if err != nil && tt.errContains != "" { - if !strings.Contains(err.Error(), tt.errContains) { - t.Errorf("Error message should contain %q, got: %v", tt.errContains, err) - } - } - }) - } -} - -func TestValidateIncludedFileWithIgnoredFields(t *testing.T) { - tests := []struct { - name string - frontmatter map[string]any - wantErr bool - errContains string - }{ - { - name: "valid included file with applyTo - should fail (not in schema yet)", - frontmatter: map[string]any{ - "applyTo": "some-target", - "engine": "claude", - }, - wantErr: true, - errContains: "applyTo", - }, - { - name: "valid included file with description - should pass", - frontmatter: map[string]any{ - "description": "This is a test description", - "engine": "claude", - }, - wantErr: false, - }, - { - name: "invalid included file with 'on' field - should fail", - frontmatter: map[string]any{ - "on": "push", - "engine": "claude", - }, - wantErr: true, - errContains: "on", - }, - { - name: "invalid included file with invalid field - should fail", - frontmatter: map[string]any{ - "invalid_field": "should-fail", - "engine": "claude", - }, - wantErr: true, - errContains: "invalid_field", - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - err := ValidateIncludedFileFrontmatterWithSchema(tt.frontmatter) - - if (err != nil) != tt.wantErr { - t.Errorf("ValidateIncludedFileFrontmatterWithSchema() error = %v, wantErr %v", err, tt.wantErr) - return - } - - if err != nil && tt.errContains != "" { - if !strings.Contains(err.Error(), tt.errContains) { - t.Errorf("Error message should contain %q, got: %v", tt.errContains, err) - } - } - }) - } -} diff --git a/pkg/parser/schema_validation.go b/pkg/parser/schema_validation.go index 41432f5e7a8..3e4ead0fb06 100644 --- a/pkg/parser/schema_validation.go +++ b/pkg/parser/schema_validation.go @@ -52,28 +52,6 @@ func validateSharedWorkflowFields(frontmatter map[string]any) error { // - Invalid additional properties (e.g., unknown fields) // // See pkg/parser/schema_passthrough_validation_test.go for comprehensive test coverage. -func ValidateMainWorkflowFrontmatterWithSchema(frontmatter map[string]any) error { - schemaValidationLog.Print("Validating main workflow frontmatter with schema") - - // Filter out ignored fields before validation - filtered := filterIgnoredFields(frontmatter) - - // First run custom validation for command trigger conflicts (provides better error messages) - if err := validateCommandTriggerConflicts(filtered); err != nil { - schemaValidationLog.Printf("Command trigger validation failed: %v", err) - return err - } - - // Then run the standard schema validation - // This validates all fields including pass-through fields (concurrency, container, etc.) - if err := validateWithSchema(filtered, mainWorkflowSchema, "main workflow file"); err != nil { - schemaValidationLog.Printf("Schema validation failed for main workflow: %v", err) - return err - } - - // Finally run other custom validation rules - return validateEngineSpecificRules(filtered) -} // ValidateMainWorkflowFrontmatterWithSchemaAndLocation validates main workflow frontmatter with file location info func ValidateMainWorkflowFrontmatterWithSchemaAndLocation(frontmatter map[string]any, filePath string) error { @@ -95,34 +73,6 @@ func ValidateMainWorkflowFrontmatterWithSchemaAndLocation(frontmatter map[string } // ValidateIncludedFileFrontmatterWithSchema validates included file frontmatter using JSON schema -func ValidateIncludedFileFrontmatterWithSchema(frontmatter map[string]any) error { - schemaValidationLog.Print("Validating included file frontmatter with schema") - - // Filter out ignored fields before validation - filtered := filterIgnoredFields(frontmatter) - - // First check for forbidden fields in shared workflows - if err := validateSharedWorkflowFields(filtered); err != nil { - schemaValidationLog.Printf("Shared workflow field validation failed: %v", err) - return err - } - - // To validate shared workflows against the main schema, we temporarily add an 'on' field - // This allows us to use the full schema validation while still enforcing the forbidden field check above - tempFrontmatter := make(map[string]any) - maps.Copy(tempFrontmatter, filtered) - // Add a temporary 'on' field to satisfy the schema's required field - tempFrontmatter["on"] = "push" - - // Validate with the main schema (which will catch unknown fields) - if err := validateWithSchema(tempFrontmatter, mainWorkflowSchema, "included file"); err != nil { - schemaValidationLog.Printf("Schema validation failed for included file: %v", err) - return err - } - - // Run custom validation for engine-specific rules - return validateEngineSpecificRules(filtered) -} // ValidateIncludedFileFrontmatterWithSchemaAndLocation validates included file frontmatter with file location info func ValidateIncludedFileFrontmatterWithSchemaAndLocation(frontmatter map[string]any, filePath string) error { @@ -150,7 +100,3 @@ func ValidateIncludedFileFrontmatterWithSchemaAndLocation(frontmatter map[string } // ValidateMCPConfigWithSchema validates MCP configuration using JSON schema -func ValidateMCPConfigWithSchema(mcpConfig map[string]any, toolName string) error { - schemaValidationLog.Printf("Validating MCP configuration for tool: %s", toolName) - return validateWithSchema(mcpConfig, mcpConfigSchema, fmt.Sprintf("MCP configuration for tool '%s'", toolName)) -} diff --git a/pkg/parser/schema_validation_test.go b/pkg/parser/schema_validation_test.go deleted file mode 100644 index 0618a07c4f8..00000000000 --- a/pkg/parser/schema_validation_test.go +++ /dev/null @@ -1,74 +0,0 @@ -//go:build !integration - -package parser - -import ( - "strings" - "testing" - - "github.com/github/gh-aw/pkg/constants" -) - -// TestForbiddenFieldsInSharedWorkflows verifies each forbidden field is properly rejected -func TestForbiddenFieldsInSharedWorkflows(t *testing.T) { - // Use the SharedWorkflowForbiddenFields constant from constants package - forbiddenFields := constants.SharedWorkflowForbiddenFields - - for _, field := range forbiddenFields { - t.Run("reject_"+field, func(t *testing.T) { - frontmatter := map[string]any{ - field: "test-value", - "tools": map[string]any{"bash": true}, - } - - err := ValidateIncludedFileFrontmatterWithSchema(frontmatter) - if err == nil { - t.Errorf("Expected error for forbidden field '%s', got nil", field) - } - - if err != nil && !strings.Contains(err.Error(), "cannot be used in shared workflows") { - t.Errorf("Error message should mention shared workflows, got: %v", err) - } - }) - } -} - -// TestAllowedFieldsInSharedWorkflows verifies allowed fields work correctly -func TestAllowedFieldsInSharedWorkflows(t *testing.T) { - allowedFields := map[string]any{ - "tools": map[string]any{"bash": true}, - "engine": "copilot", - "network": map[string]any{"allowed": []string{"defaults"}}, - "mcp-servers": map[string]any{}, - "permissions": "read-all", - "runtimes": map[string]any{"node": map[string]any{"version": "20"}}, - "safe-outputs": map[string]any{}, - "safe-inputs": map[string]any{}, - "services": map[string]any{}, - "steps": []any{}, - "secret-masking": true, - "jobs": map[string]any{"test": map[string]any{"runs-on": "ubuntu-latest", "steps": []any{map[string]any{"run": "echo test"}}}}, - "description": "test", - "metadata": map[string]any{}, - "inputs": map[string]any{}, - "bots": []string{"copilot"}, - "post-steps": []any{map[string]any{"run": "echo cleanup"}}, - "labels": []string{"automation", "testing"}, - "imports": []string{"./shared.md"}, - "cache": map[string]any{"key": "test-key", "path": "node_modules"}, - "source": "githubnext/agentics/workflows/ci-doctor.md@v1.0.0", - } - - for field, value := range allowedFields { - t.Run("allow_"+field, func(t *testing.T) { - frontmatter := map[string]any{ - field: value, - } - - err := ValidateIncludedFileFrontmatterWithSchema(frontmatter) - if err != nil && strings.Contains(err.Error(), "cannot be used in shared workflows") { - t.Errorf("Field '%s' should be allowed in shared workflows, got error: %v", field, err) - } - }) - } -} diff --git a/pkg/parser/yaml_error.go b/pkg/parser/yaml_error.go index c5d39819989..b46ee4a8383 100644 --- a/pkg/parser/yaml_error.go +++ b/pkg/parser/yaml_error.go @@ -102,153 +102,7 @@ func adjustLineNumbersInFormattedError(formatted string, offset int) string { // // NOTE: This function is kept for backward compatibility. New code should use FormatYAMLError() // which leverages yaml.FormatError() for better error messages with source context. -func ExtractYAMLError(err error, frontmatterLineOffset int) (line int, column int, message string) { - yamlErrorLog.Printf("Extracting YAML error information: offset=%d", frontmatterLineOffset) - errStr := err.Error() - - // First try to extract from goccy/go-yaml's [line:column] format - line, column, message = extractFromGoccyFormat(errStr, frontmatterLineOffset) - if line > 0 || column > 0 { - yamlErrorLog.Printf("Extracted error location from goccy format: line=%d, column=%d", line, column) - return line, column, message - } - - // Fallback to standard YAML error string parsing for other libraries - yamlErrorLog.Print("Falling back to string parsing for error location") - return extractFromStringParsing(errStr, frontmatterLineOffset) -} // extractFromGoccyFormat extracts line/column from goccy/go-yaml's [line:column] message format -func extractFromGoccyFormat(errStr string, frontmatterLineOffset int) (line int, column int, message string) { - // Look for goccy format like "[5:10] mapping value is not allowed in this context" - if strings.Contains(errStr, "[") && strings.Contains(errStr, "]") { - start := strings.Index(errStr, "[") - end := strings.Index(errStr, "]") - if start >= 0 && end > start { - locationPart := errStr[start+1 : end] - messagePart := strings.TrimSpace(errStr[end+1:]) - - // Parse line:column format - if strings.Contains(locationPart, ":") { - parts := strings.Split(locationPart, ":") - if len(parts) == 2 { - lineStr := strings.TrimSpace(parts[0]) - columnStr := strings.TrimSpace(parts[1]) - - // Parse line and column numbers - if _, parseErr := fmt.Sscanf(lineStr, "%d", &line); parseErr == nil { - if _, parseErr := fmt.Sscanf(columnStr, "%d", &column); parseErr == nil { - // Adjust line number to account for frontmatter position in file - if line > 0 { - line += frontmatterLineOffset - 1 // -1 because line numbers in YAML errors are 1-based relative to YAML content - } - - // Only return valid positions - avoid returning 1,1 when location is unknown - if line <= frontmatterLineOffset && column <= 1 { - return 0, 0, messagePart - } - - return line, column, messagePart - } - } - } - } - } - } - - return 0, 0, "" -} // extractFromStringParsing provides fallback string parsing for other YAML libraries -func extractFromStringParsing(errStr string, frontmatterLineOffset int) (line int, column int, message string) { - // Parse "yaml: line X: column Y: message" format (enhanced parsers that provide column info) - if strings.Contains(errStr, "yaml: line ") && strings.Contains(errStr, "column ") { - parts := strings.SplitN(errStr, "yaml: line ", 2) - if len(parts) > 1 { - lineInfo := parts[1] - - // Look for column information - colonIndex := strings.Index(lineInfo, ":") - if colonIndex > 0 { - lineStr := lineInfo[:colonIndex] - - // Parse line number - if _, parseErr := fmt.Sscanf(lineStr, "%d", &line); parseErr == nil { - // Look for column part - remaining := lineInfo[colonIndex+1:] - if strings.Contains(remaining, "column ") { - columnParts := strings.SplitN(remaining, "column ", 2) - if len(columnParts) > 1 { - columnInfo := columnParts[1] - colonIndex2 := strings.Index(columnInfo, ":") - if colonIndex2 > 0 { - columnStr := columnInfo[:colonIndex2] - message = strings.TrimSpace(columnInfo[colonIndex2+1:]) - - // Parse column number - if _, parseErr := fmt.Sscanf(columnStr, "%d", &column); parseErr == nil { - // Adjust line number to account for frontmatter position in file - line += frontmatterLineOffset - 1 // -1 because line numbers in YAML errors are 1-based relative to YAML content - return - } - } - } - } - } - } - } - } - - // Parse "yaml: line X: message" format (standard format without column info) - if strings.Contains(errStr, "yaml: line ") { - parts := strings.SplitN(errStr, "yaml: line ", 2) - if len(parts) > 1 { - lineInfo := parts[1] - colonIndex := strings.Index(lineInfo, ":") - if colonIndex > 0 { - lineStr := lineInfo[:colonIndex] - message = strings.TrimSpace(lineInfo[colonIndex+1:]) - - // Parse line number - if _, parseErr := fmt.Sscanf(lineStr, "%d", &line); parseErr == nil { - // Adjust line number to account for frontmatter position in file - line += frontmatterLineOffset - 1 // -1 because line numbers in YAML errors are 1-based relative to YAML content - // Don't default to column 1 when not provided - return 0 instead - column = 0 - return - } - } - } - } - - // Parse "yaml: unmarshal errors: line X: message" format (multiline errors) - if strings.Contains(errStr, "yaml: unmarshal errors:") && strings.Contains(errStr, "line ") { - lines := strings.SplitSeq(errStr, "\n") - for errorLine := range lines { - errorLine = strings.TrimSpace(errorLine) - if strings.Contains(errorLine, "line ") && strings.Contains(errorLine, ":") { - // Extract the first line number found in the error - parts := strings.SplitN(errorLine, "line ", 2) - if len(parts) > 1 { - colonIndex := strings.Index(parts[1], ":") - if colonIndex > 0 { - lineStr := parts[1][:colonIndex] - restOfMessage := strings.TrimSpace(parts[1][colonIndex+1:]) - - // Parse line number - if _, parseErr := fmt.Sscanf(lineStr, "%d", &line); parseErr == nil { - // Adjust line number to account for frontmatter position in file - line += frontmatterLineOffset - 1 // -1 because line numbers in YAML errors are 1-based relative to YAML content - column = 0 // Don't default to column 1 - message = restOfMessage - return - } - } - } - } - } - } - - // Fallback: return original error message with no location - return 0, 0, errStr -} diff --git a/pkg/parser/yaml_error_test.go b/pkg/parser/yaml_error_test.go index 59d69266bcb..1847307bb02 100644 --- a/pkg/parser/yaml_error_test.go +++ b/pkg/parser/yaml_error_test.go @@ -3,7 +3,6 @@ package parser import ( - "errors" "fmt" "strings" "testing" @@ -11,277 +10,6 @@ import ( "github.com/goccy/go-yaml" ) -func TestExtractYAMLError(t *testing.T) { - tests := []struct { - name string - err error - frontmatterLineOffset int - expectedLine int - expectedColumn int - expectedMessage string - }{ - { - name: "yaml line error", - err: errors.New("yaml: line 7: mapping values are not allowed in this context"), - frontmatterLineOffset: 1, - expectedLine: 7, // 7 + 1 - 1 = 7 - expectedColumn: 0, // No column info provided in string format - expectedMessage: "mapping values are not allowed in this context", - }, - { - name: "yaml line error with frontmatter offset", - err: errors.New("yaml: line 3: found character that cannot start any token"), - frontmatterLineOffset: 5, - expectedLine: 7, // 3 + 5 - 1 = 7 - expectedColumn: 0, // No column info provided in string format - expectedMessage: "found character that cannot start any token", - }, - { - name: "non-yaml error", - err: errors.New("some other error"), - frontmatterLineOffset: 1, - expectedLine: 0, - expectedColumn: 0, - expectedMessage: "some other error", - }, - { - name: "yaml error with different message format", - err: errors.New("yaml: line 15: found unexpected end of stream"), - frontmatterLineOffset: 2, - expectedLine: 16, // 15 + 2 - 1 = 16 - expectedColumn: 0, // No column info provided in string format - expectedMessage: "found unexpected end of stream", - }, - { - name: "yaml error with indentation issue", - err: errors.New("yaml: line 4: bad indentation of a mapping entry"), - frontmatterLineOffset: 1, - expectedLine: 4, // 4 + 1 - 1 = 4 - expectedColumn: 0, // No column info provided in string format - expectedMessage: "bad indentation of a mapping entry", - }, - { - name: "yaml error with duplicate key", - err: errors.New("yaml: line 6: found duplicate key"), - frontmatterLineOffset: 3, - expectedLine: 8, // 6 + 3 - 1 = 8 - expectedColumn: 0, // No column info provided in string format - expectedMessage: "found duplicate key", - }, - { - name: "yaml error with complex format", - err: errors.New("yaml: line 12: did not find expected ',' or ']'"), - frontmatterLineOffset: 0, - expectedLine: 11, // 12 + 0 - 1 = 11 - expectedColumn: 0, // No column info provided in string format - expectedMessage: "did not find expected ',' or ']'", - }, - { - name: "yaml unmarshal error multiline", - err: errors.New("yaml: unmarshal errors:\n line 4: mapping key \"permissions\" already defined at line 2"), - frontmatterLineOffset: 1, - expectedLine: 4, // 4 + 1 - 1 = 4 - expectedColumn: 0, // No column info provided in string format - expectedMessage: "mapping key \"permissions\" already defined at line 2", - }, - { - name: "yaml error with flow mapping", - err: errors.New("yaml: line 8: did not find expected ',' or '}'"), - frontmatterLineOffset: 1, - expectedLine: 8, // 8 + 1 - 1 = 8 - expectedColumn: 0, // No column info provided in string format - expectedMessage: "did not find expected ',' or '}'", - }, - { - name: "yaml error with invalid character", - err: errors.New("yaml: line 5: found character that cannot start any token"), - frontmatterLineOffset: 0, - expectedLine: 4, // 5 + 0 - 1 = 4 - expectedColumn: 0, // No column info provided in string format - expectedMessage: "found character that cannot start any token", - }, - { - name: "yaml error with unmarshal type issue", - err: errors.New("yaml: line 3: cannot unmarshal !!str `yes_please` into bool"), - frontmatterLineOffset: 2, - expectedLine: 4, // 3 + 2 - 1 = 4 - expectedColumn: 0, // No column info provided in string format - expectedMessage: "cannot unmarshal !!str `yes_please` into bool", - }, - { - name: "yaml complex unmarshal error with nested line info", - err: errors.New("yaml: unmarshal errors:\n line 7: found unexpected end of stream\n line 9: mapping values are not allowed in this context"), - frontmatterLineOffset: 1, - expectedLine: 7, // First line 7 + 1 - 1 = 7 - expectedColumn: 0, // No column info provided in string format - expectedMessage: "found unexpected end of stream", - }, - { - name: "yaml error with column information greater than 1", - err: errors.New("yaml: line 5: column 12: invalid character at position"), - frontmatterLineOffset: 1, - expectedLine: 5, // 5 + 1 - 1 = 5 - expectedColumn: 12, - expectedMessage: "invalid character at position", - }, - { - name: "yaml error with high column number", - err: errors.New("yaml: line 3: column 45: unexpected token found"), - frontmatterLineOffset: 2, - expectedLine: 4, // 3 + 2 - 1 = 4 - expectedColumn: 45, - expectedMessage: "unexpected token found", - }, - { - name: "yaml error with column 1 explicitly specified", - err: errors.New("yaml: line 8: column 1: mapping values not allowed in this context"), - frontmatterLineOffset: 0, - expectedLine: 7, // 8 + 0 - 1 = 7 - expectedColumn: 1, - expectedMessage: "mapping values not allowed in this context", - }, - { - name: "yaml error with medium column position", - err: errors.New("yaml: line 2: column 23: found character that cannot start any token"), - frontmatterLineOffset: 3, - expectedLine: 4, // 2 + 3 - 1 = 4 - expectedColumn: 23, - expectedMessage: "found character that cannot start any token", - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - line, column, message := ExtractYAMLError(tt.err, tt.frontmatterLineOffset) - - if line != tt.expectedLine { - t.Errorf("Expected line %d, got %d", tt.expectedLine, line) - } - if column != tt.expectedColumn { - t.Errorf("Expected column %d, got %d", tt.expectedColumn, column) - } - if message != tt.expectedMessage { - t.Errorf("Expected message '%s', got '%s'", tt.expectedMessage, message) - } - }) - } -} - -// TestExtractYAMLErrorWithGoccyErrors tests extraction from actual goccy/go-yaml errors -func TestExtractYAMLErrorWithGoccyErrors(t *testing.T) { - tests := []struct { - name string - yamlContent string - frontmatterLineOffset int - expectedMinLine int // Use min line since exact line may vary - expectedMinColumn int // Use min column since exact column may vary - expectValidLocation bool - }{ - { - name: "goccy invalid syntax", - yamlContent: "invalid: yaml: content", - frontmatterLineOffset: 1, - expectedMinLine: 1, // Should be >= frontmatterLineOffset - expectedMinColumn: 5, // Should have a valid column - expectValidLocation: true, - }, - { - name: "goccy indentation error", - yamlContent: "name: test\n invalid_indentation: here", - frontmatterLineOffset: 2, - expectedMinLine: 2, // Should be >= frontmatterLineOffset - expectedMinColumn: 1, // Should have a valid column - expectValidLocation: true, - }, - { - name: "goccy duplicate key", - yamlContent: "name: test\nname: duplicate", - frontmatterLineOffset: 0, - expectedMinLine: 0, // Should be >= frontmatterLineOffset (could be 0 for some cases) - expectedMinColumn: 1, // Should have a valid column - expectValidLocation: true, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - // Generate an actual goccy/go-yaml error - var result map[string]any - err := yaml.Unmarshal([]byte(tt.yamlContent), &result) - - if err == nil { - t.Errorf("Expected YAML parsing to fail for content: %q", tt.yamlContent) - return - } - - line, column, message := ExtractYAMLError(err, tt.frontmatterLineOffset) - - if tt.expectValidLocation { - if line < tt.expectedMinLine { - t.Errorf("Expected line >= %d, got %d", tt.expectedMinLine, line) - } - if column < tt.expectedMinColumn { - t.Errorf("Expected column >= %d, got %d", tt.expectedMinColumn, column) - } - if message == "" { - t.Errorf("Expected non-empty message") - } - } else { - if line != 0 || column != 0 { - t.Errorf("Expected no location (0,0) when location unknown, got (%d,%d)", line, column) - } - } - - t.Logf("YAML: %q -> Line: %d, Column: %d, Message: %s", tt.yamlContent, line, column, message) - }) - } -} - -// TestExtractYAMLErrorUnknownLocation tests that 0,0 is returned when location is unknown -func TestExtractYAMLErrorUnknownLocation(t *testing.T) { - tests := []struct { - name string - err error - frontmatterLineOffset int - expectedLine int - expectedColumn int - expectedMessage string - }{ - { - name: "non-yaml error without location", - err: errors.New("generic error without location info"), - frontmatterLineOffset: 1, - expectedLine: 0, - expectedColumn: 0, - expectedMessage: "generic error without location info", - }, - { - name: "malformed yaml error string", - err: errors.New("yaml: some error without line info"), - frontmatterLineOffset: 1, - expectedLine: 0, - expectedColumn: 0, - expectedMessage: "yaml: some error without line info", - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - line, column, message := ExtractYAMLError(tt.err, tt.frontmatterLineOffset) - - if line != tt.expectedLine { - t.Errorf("Expected line %d, got %d", tt.expectedLine, line) - } - if column != tt.expectedColumn { - t.Errorf("Expected column %d, got %d", tt.expectedColumn, column) - } - if message != tt.expectedMessage { - t.Errorf("Expected message '%s', got '%s'", tt.expectedMessage, message) - } - }) - } -} - // TestFormatYAMLError tests the new FormatYAMLError function that uses yaml.FormatError() func TestFormatYAMLError(t *testing.T) { tests := []struct { From 30ec6bfc213708278f43f10f7175e78c1fee3945 Mon Sep 17 00:00:00 2001 From: Don Syme Date: Sat, 28 Feb 2026 14:14:45 +0000 Subject: [PATCH 5/5] dead15: remove dead get*Script() stubs from js.go --- pkg/workflow/add_comment.go | 2 +- .../compiler_safe_outputs_specialized.go | 2 +- pkg/workflow/create_code_scanning_alert.go | 2 +- pkg/workflow/create_discussion.go | 2 +- pkg/workflow/create_pr_review_comment.go | 2 +- pkg/workflow/js.go | 16 ++++------------ 6 files changed, 9 insertions(+), 17 deletions(-) diff --git a/pkg/workflow/add_comment.go b/pkg/workflow/add_comment.go index 604aabb5832..cc8e59194fb 100644 --- a/pkg/workflow/add_comment.go +++ b/pkg/workflow/add_comment.go @@ -134,7 +134,7 @@ func (c *Compiler) buildCreateOutputAddCommentJob(data *WorkflowData, mainJobNam StepID: "add_comment", MainJobName: mainJobName, CustomEnvVars: customEnvVars, - Script: getAddCommentScript(), + Script: "", Permissions: permissions, Outputs: outputs, Condition: jobCondition, diff --git a/pkg/workflow/compiler_safe_outputs_specialized.go b/pkg/workflow/compiler_safe_outputs_specialized.go index a69b984aff5..bdf352ae979 100644 --- a/pkg/workflow/compiler_safe_outputs_specialized.go +++ b/pkg/workflow/compiler_safe_outputs_specialized.go @@ -172,7 +172,7 @@ func (c *Compiler) buildCreateProjectStepConfig(data *WorkflowData, mainJobName StepName: "Create Project", StepID: "create_project", ScriptName: "create_project", - Script: getCreateProjectScript(), + Script: "", CustomEnvVars: customEnvVars, Condition: condition, Token: effectiveToken, diff --git a/pkg/workflow/create_code_scanning_alert.go b/pkg/workflow/create_code_scanning_alert.go index 12d42e65c84..920fa56e089 100644 --- a/pkg/workflow/create_code_scanning_alert.go +++ b/pkg/workflow/create_code_scanning_alert.go @@ -79,7 +79,7 @@ func (c *Compiler) buildCreateOutputCodeScanningAlertJob(data *WorkflowData, mai StepID: "create_code_scanning_alert", MainJobName: mainJobName, CustomEnvVars: customEnvVars, - Script: getCreateCodeScanningAlertScript(), + Script: "", Permissions: NewPermissionsContentsReadSecurityEventsWriteActionsRead(), Outputs: outputs, Condition: jobCondition, diff --git a/pkg/workflow/create_discussion.go b/pkg/workflow/create_discussion.go index c88ff54944a..eae879f7bcb 100644 --- a/pkg/workflow/create_discussion.go +++ b/pkg/workflow/create_discussion.go @@ -195,7 +195,7 @@ func (c *Compiler) buildCreateOutputDiscussionJob(data *WorkflowData, mainJobNam StepID: "create_discussion", MainJobName: mainJobName, CustomEnvVars: customEnvVars, - Script: getCreateDiscussionScript(), + Script: "", Permissions: NewPermissionsContentsReadIssuesWriteDiscussionsWrite(), Outputs: outputs, Needs: needs, diff --git a/pkg/workflow/create_pr_review_comment.go b/pkg/workflow/create_pr_review_comment.go index c3c2693bffe..739e27d9908 100644 --- a/pkg/workflow/create_pr_review_comment.go +++ b/pkg/workflow/create_pr_review_comment.go @@ -74,7 +74,7 @@ func (c *Compiler) buildCreateOutputPullRequestReviewCommentJob(data *WorkflowDa StepID: "create_pr_review_comment", MainJobName: mainJobName, CustomEnvVars: customEnvVars, - Script: getCreatePRReviewCommentScript(), + Script: "", Permissions: NewPermissionsContentsReadPRWrite(), Outputs: outputs, Condition: jobCondition, diff --git a/pkg/workflow/js.go b/pkg/workflow/js.go index 1eaa09a6512..3a748175c01 100644 --- a/pkg/workflow/js.go +++ b/pkg/workflow/js.go @@ -67,18 +67,10 @@ func init() { // All getter functions return empty strings since embedded scripts were removed -func getAddCommentScript() string { return "" } -func getAssignToAgentScript() string { return "" } -func getCreateCodeScanningAlertScript() string { return "" } -func getCreateDiscussionScript() string { return "" } -func getCreateIssueScript() string { return "" } - -//nolint:unused // Only used in integration tests -func getCreatePRReviewCommentScript() string { return "" } -func getNoOpScript() string { return "" } -func getNotifyCommentErrorScript() string { return "" } -func getCreateProjectScript() string { return "" } -func getUploadAssetsScript() string { return "" } +func getAssignToAgentScript() string { return "" } +func getNoOpScript() string { return "" } +func getNotifyCommentErrorScript() string { return "" } +func getUploadAssetsScript() string { return "" } // Public Get* functions return empty strings since embedded scripts were removed