diff --git a/pkg/cli/logs_report_test.go b/pkg/cli/logs_report_test.go index 367298627ac..3e57ff67985 100644 --- a/pkg/cli/logs_report_test.go +++ b/pkg/cli/logs_report_test.go @@ -16,6 +16,14 @@ import ( "github.com/github/gh-aw/pkg/sliceutil" ) +func TestNormalizeJobNamePreservesPeriods(t *testing.T) { + t.Parallel() + + if got := normalizeJobName(" Worker.V1 "); got != "worker.v1" { + t.Errorf("normalizeJobName() = %q, want %q; periods are not separators in log job-name matching", got, "worker.v1") + } +} + func TestToolUsageSummariesShareStatsBase(t *testing.T) { t.Parallel() diff --git a/pkg/stringutil/README.md b/pkg/stringutil/README.md index 2b4e512d93c..bb16c3d7eb7 100644 --- a/pkg/stringutil/README.md +++ b/pkg/stringutil/README.md @@ -12,7 +12,9 @@ The `stringutil` package is organized into focused sub-files: | Sub-file | Functions | |----------|-----------| -| `stringutil.go` | General string helpers | +| `stringutil.go` | General string helpers (`Truncate`, `FormatList`, `IsPositiveInteger`) | +| `whitespace.go` | Whitespace normalization | +| `version.go` | Version value coercion | | `ansi.go` | ANSI escape-code stripping | | `identifiers.go` | Workflow name and path normalization | | `sanitize.go` | Security-sensitive string sanitization | @@ -47,6 +49,20 @@ stringutil.Truncate("hello world", 8) // "hello..." stringutil.Truncate("hi", 8) // "hi" ``` +### `FormatList(items []string) string` + +Formats a slice of strings as a natural-language list with an Oxford comma. + +```go +stringutil.FormatList([]string{"a", "b", "c"}) // "a, b, and c" +``` + +### `IsPositiveInteger(s string) bool` + +Returns `true` if and only if `s` is a decimal integer that is strictly greater than zero, has no leading zeros, and contains no non-digit characters. Returns `false` for `""`, `"0"`, negative strings (e.g. `"-5"`), strings with leading zeros (e.g. `"007"`), and non-numeric strings. + +## Whitespace Normalization (`whitespace.go`) + ### `NormalizeWhitespace(content string) string` Normalizes trailing whitespace in multi-line content. Trims trailing spaces and tabs from every line, then ensures the content ends with exactly one newline (or is empty). This reduces spurious diffs caused by trailing-whitespace differences. @@ -55,6 +71,8 @@ Normalizes trailing whitespace in multi-line content. Trims trailing spaces and Removes shared leading indentation from non-empty lines in a multi-line string. This is useful for normalizing heredoc-like blocks while preserving relative indentation. +## Version Value Coercion (`version.go`) + ### `ParseVersionValue(version any) string` Converts a `any`-typed version value (typically from YAML parsing, which may produce `int`, `float64`, or `string`) into a string. Returns an empty string for nil. @@ -65,18 +83,6 @@ stringutil.ParseVersionValue(20) // "20" stringutil.ParseVersionValue(20.0) // "20" ``` -### `FormatList(items []string) string` - -Formats a slice of strings as a natural-language list with an Oxford comma. - -```go -stringutil.FormatList([]string{"a", "b", "c"}) // "a, b, and c" -``` - -### `IsPositiveInteger(s string) bool` - -Returns `true` if and only if `s` is a decimal integer that is strictly greater than zero, has no leading zeros, and contains no non-digit characters. Returns `false` for `""`, `"0"`, negative strings (e.g. `"-5"`), strings with leading zeros (e.g. `"007"`), and non-numeric strings. - ## ANSI Escape Code Stripping (`ansi.go`) ### `StripANSI(s string) string` @@ -109,6 +115,15 @@ stringutil.NormalizeSafeOutputIdentifier("create-issue") // "create_is stringutil.NormalizeSafeOutputIdentifier("executor-workflow.agent") // "executor_workflow_agent" ``` +### `NormalizeIdentifierToHyphens(identifier string) string` + +Converts underscores **and periods** to hyphens, normalizing user-facing `underscore_separated` and dot-separated formats to the hyphen-separated format conventionally used for GitHub Actions job names. This is the hyphen-canonical counterpart to `NormalizeSafeOutputIdentifier`. + +```go +stringutil.NormalizeIdentifierToHyphens("create_issue") // "create-issue" +stringutil.NormalizeIdentifierToHyphens("executor_workflow.agent") // "executor-workflow-agent" +``` + ### `MarkdownToLockFile(mdPath string) string` Converts a workflow markdown path (`.md`) to its compiled lock file path (`.lock.yml`). Returns the path unchanged if it already ends with `.lock.yml`. @@ -306,7 +321,7 @@ distance := stringutil.LevenshteinDistance("copiliot", "copilot") ## Design Decisions -- All debug output uses namespace-prefixed loggers (`stringutil:identifiers`, `stringutil:sanitize`, `stringutil:urls`, `stringutil:pat_validation`) and is only emitted when `DEBUG=stringutil:*`. +- All debug output uses namespace-prefixed loggers (`stringutil:identifiers`, `stringutil:sanitize`, `stringutil:urls`, `stringutil:pat_validation`, `stringutil:whitespace`, `stringutil:version`) and is only emitted when `DEBUG=stringutil:*`. - `SanitizeErrorMessage` is intentionally conservative: it excludes common GitHub Actions keywords to avoid over-redacting legitimate error messages. - `StripANSI` handles both CSI sequences (`ESC[`) and other ESC-prefixed sequences to cover the full range of ANSI escape codes found in terminal output. @@ -324,7 +339,7 @@ This appendix is generated from the current non-test Go source files in this pac | Types | 2 | | Constants | 4 | | Variables | 0 | -| Functions and methods | 28 | +| Functions and methods | 29 | | Additional symbols documented in this appendix | 0 | The sections above already mention every exported top-level symbol in the current source tree. diff --git a/pkg/stringutil/identifiers.go b/pkg/stringutil/identifiers.go index 89ba76e525f..dd5c11515ea 100644 --- a/pkg/stringutil/identifiers.go +++ b/pkg/stringutil/identifiers.go @@ -65,6 +65,31 @@ func NormalizeSafeOutputIdentifier(identifier string) string { return result } +// NormalizeIdentifierToHyphens converts underscores and periods to hyphens. +// This is the hyphen-canonical counterpart to NormalizeSafeOutputIdentifier, +// standardizing identifiers to the hyphen-separated format conventionally used +// for GitHub Actions job names. +// +// Both underscore-separated and hyphen-separated formats are valid inputs. +// Periods are also replaced since job names should not contain them. +// +// This function performs normalization only - it assumes the input is already +// a valid identifier and does NOT perform character validation, case +// conversion, or whitespace trimming. +// +// Examples: +// +// NormalizeIdentifierToHyphens("create_issue") // returns "create-issue" +// NormalizeIdentifierToHyphens("create-issue") // returns "create-issue" (unchanged) +// NormalizeIdentifierToHyphens("add_comment") // returns "add-comment" +// NormalizeIdentifierToHyphens("update_pr") // returns "update-pr" +// NormalizeIdentifierToHyphens("executor_workflow.agent") // returns "executor-workflow-agent" +func NormalizeIdentifierToHyphens(identifier string) string { + result := strings.ReplaceAll(identifier, "_", "-") + result = strings.ReplaceAll(result, ".", "-") + return result +} + // MarkdownToLockFile converts a workflow markdown file path to its compiled lock file path. // This is the standard transformation for agentic workflow files. // diff --git a/pkg/stringutil/identifiers_test.go b/pkg/stringutil/identifiers_test.go index 92a80f82ab4..43afff393d3 100644 --- a/pkg/stringutil/identifiers_test.go +++ b/pkg/stringutil/identifiers_test.go @@ -176,6 +176,101 @@ func TestNormalizeSafeOutputIdentifier(t *testing.T) { } } +func TestNormalizeIdentifierToHyphens(t *testing.T) { + t.Parallel() + tests := []struct { + name string + identifier string + expected string + }{ + { + name: "underscore-separated to hyphen", + identifier: "create_issue", + expected: "create-issue", + }, + { + name: "already hyphen-separated", + identifier: "create-issue", + expected: "create-issue", + }, + { + name: "multiple underscores", + identifier: "add_comment_to_issue", + expected: "add-comment-to-issue", + }, + { + name: "mixed dashes and underscores", + identifier: "update-pr_status", + expected: "update-pr-status", + }, + { + name: "no dashes or underscores", + identifier: "createissue", + expected: "createissue", + }, + { + name: "single underscore", + identifier: "add_comment", + expected: "add-comment", + }, + { + name: "trailing underscore", + identifier: "update_", + expected: "update-", + }, + { + name: "leading underscore", + identifier: "_create", + expected: "-create", + }, + { + name: "consecutive underscores", + identifier: "create__issue", + expected: "create--issue", + }, + { + name: "empty string", + identifier: "", + expected: "", + }, + { + name: "only underscores", + identifier: "___", + expected: "---", + }, + { + name: "period in workflow name", + identifier: "executor_workflow.agent", + expected: "executor-workflow-agent", + }, + { + name: "period only", + identifier: "my.workflow", + expected: "my-workflow", + }, + { + name: "multiple periods", + identifier: "my.workflow.agent", + expected: "my-workflow-agent", + }, + { + name: "period and underscores", + identifier: "my_workflow.agent", + expected: "my-workflow-agent", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + result := NormalizeIdentifierToHyphens(tt.identifier) + if result != tt.expected { + t.Errorf("NormalizeIdentifierToHyphens(%q) = %q, want %q", tt.identifier, result, tt.expected) + } + }) + } +} + func BenchmarkNormalizeWorkflowName(b *testing.B) { name := "weekly-research-workflow.lock.yml" for b.Loop() { @@ -190,6 +285,13 @@ func BenchmarkNormalizeSafeOutputIdentifier(b *testing.B) { } } +func BenchmarkNormalizeIdentifierToHyphens(b *testing.B) { + identifier := "create_pull_request_review_comment" + for b.Loop() { + NormalizeIdentifierToHyphens(identifier) + } +} + func TestMarkdownToLockFile(t *testing.T) { t.Parallel() tests := []struct { diff --git a/pkg/stringutil/spec_test.go b/pkg/stringutil/spec_test.go index 830d8c0445c..321244f7052 100644 --- a/pkg/stringutil/spec_test.go +++ b/pkg/stringutil/spec_test.go @@ -337,6 +337,46 @@ func TestSpec_PublicAPI_NormalizeSafeOutputIdentifier(t *testing.T) { } } +// TestSpec_PublicAPI_NormalizeIdentifierToHyphens validates the documented +// behavior of NormalizeIdentifierToHyphens as described in the package README.md. +// +// Specification: "Converts underscores and periods to hyphens, normalizing +// user-facing underscore-separated and dot-separated formats to the +// hyphen-separated format conventionally used for GitHub Actions job names." +// +// Specification examples: +// +// stringutil.NormalizeIdentifierToHyphens("create_issue") // "create-issue" +// stringutil.NormalizeIdentifierToHyphens("executor_workflow.agent") // "executor-workflow-agent" +func TestSpec_PublicAPI_NormalizeIdentifierToHyphens(t *testing.T) { + t.Parallel() + tests := []struct { + name string + input string + expected string + }{ + { + name: "converts underscores to hyphens (documented example)", + input: "create_issue", + expected: "create-issue", + }, + { + name: "converts underscores and periods to hyphens (documented example)", + input: "executor_workflow.agent", + expected: "executor-workflow-agent", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + result := NormalizeIdentifierToHyphens(tt.input) + assert.Equal(t, tt.expected, result, + "NormalizeIdentifierToHyphens(%q) should match documented output", tt.input) + }) + } +} + // TestSpec_PublicAPI_MarkdownToLockFile validates the documented behavior of // MarkdownToLockFile as described in the package README.md. // diff --git a/pkg/stringutil/stringutil.go b/pkg/stringutil/stringutil.go index dd382980d96..2f96cc64576 100644 --- a/pkg/stringutil/stringutil.go +++ b/pkg/stringutil/stringutil.go @@ -2,7 +2,6 @@ package stringutil import ( - "fmt" "strconv" "strings" @@ -29,42 +28,6 @@ func Truncate(s string, maxLen int) string { return s[:maxLen-3] + "..." } -// NormalizeWhitespace normalizes trailing whitespace and newlines to reduce spurious conflicts. -// It trims trailing whitespace from each line and ensures exactly one trailing newline. -func NormalizeWhitespace(content string) string { - // Split into lines and trim trailing whitespace from each line - lines := strings.Split(content, "\n") - for i, line := range lines { - lines[i] = strings.TrimRight(line, " \t") - } - - // Join back and ensure exactly one trailing newline if content is not empty - normalized := strings.Join(lines, "\n") - normalized = strings.TrimRight(normalized, "\n") - if normalized != "" { - normalized += "\n" - } - - return normalized -} - -// ParseVersionValue converts version values of various types to strings. -// Supports string, int, int64, uint64, and float64 types. -// Returns empty string for unsupported types. -func ParseVersionValue(version any) string { - switch v := version.(type) { - case string: - return v - case int, int64, uint64: - return fmt.Sprintf("%d", v) - case float64: - return fmt.Sprintf("%g", v) - default: - stringutilLog.Printf("ParseVersionValue: unsupported type %T, returning empty string", version) - return "" - } -} - // FormatList formats a slice of strings as a natural-language comma-separated list // with an Oxford comma and "and" before the final item. // @@ -87,58 +50,6 @@ func FormatList(items []string) string { } } -// NormalizeLeadingWhitespace removes consistent leading whitespace from all lines -// of a multi-line string. It finds the minimum indentation across all non-empty -// lines and strips that many leading whitespace characters (spaces or tabs) from -// every line. -// -// This is useful for cleaning up content generated with extra indentation, -// such as heredoc bodies. -func NormalizeLeadingWhitespace(content string) string { - lines := strings.Split(content, "\n") - if len(lines) == 0 { - return content - } - - // Find minimum leading whitespace (excluding empty lines) - minLeading := -1 - for _, line := range lines { - if strings.TrimSpace(line) == "" { - continue // Skip empty lines - } - leading := len(line) - len(strings.TrimLeft(line, " \t")) - if minLeading == -1 || leading < minLeading { - minLeading = leading - } - } - - // If no content or no leading whitespace, return as-is - if minLeading <= 0 { - return content - } - - stringutilLog.Printf("NormalizeLeadingWhitespace: stripping %d leading whitespace chars from %d lines", minLeading, len(lines)) - - // Remove the minimum leading whitespace from all lines - var result strings.Builder - for i, line := range lines { - if i > 0 { - result.WriteString("\n") - } - if strings.TrimSpace(line) == "" { - // Keep empty lines as empty - result.WriteString("") - } else if len(line) >= minLeading { - // Remove leading whitespace - result.WriteString(line[minLeading:]) - } else { - result.WriteString(line) - } - } - - return result.String() -} - // IsPositiveInteger checks if a string is a positive integer. // Returns true for strings like "1", "123", "999" but false for: // - Zero ("0") diff --git a/pkg/stringutil/stringutil_test.go b/pkg/stringutil/stringutil_test.go index dbc3f808384..2b3a0773696 100644 --- a/pkg/stringutil/stringutil_test.go +++ b/pkg/stringutil/stringutil_test.go @@ -3,7 +3,6 @@ package stringutil import ( - "strings" "testing" "github.com/stretchr/testify/assert" @@ -100,69 +99,6 @@ func TestTruncate(t *testing.T) { } } -func TestNormalizeWhitespace(t *testing.T) { - t.Parallel() - tests := []struct { - name string - content string - expected string - }{ - { - name: "no trailing whitespace", - content: "hello\nworld", - expected: "hello\nworld\n", - }, - { - name: "trailing spaces on lines", - content: "hello \nworld ", - expected: "hello\nworld\n", - }, - { - name: "trailing tabs on lines", - content: "hello\t\nworld\t", - expected: "hello\nworld\n", - }, - { - name: "multiple trailing newlines", - content: "hello\nworld\n\n\n", - expected: "hello\nworld\n", - }, - { - name: "empty string", - content: "", - expected: "", - }, - { - name: "single newline", - content: "\n", - expected: "", - }, - { - name: "mixed whitespace", - content: "hello \t\nworld \t \n\n", - expected: "hello\nworld\n", - }, - { - name: "content with no newline", - content: "hello world", - expected: "hello world\n", - }, - { - name: "content already normalized", - content: "hello\nworld\n", - expected: "hello\nworld\n", - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - t.Parallel() - result := NormalizeWhitespace(tt.content) - assert.Equal(t, tt.expected, result, "NormalizeWhitespace(%q) should normalize trailing whitespace and newlines", tt.content) - }) - } -} - func BenchmarkTruncate(b *testing.B) { s := "this is a very long string that needs to be truncated for testing purposes" for b.Loop() { @@ -170,13 +106,6 @@ func BenchmarkTruncate(b *testing.B) { } } -func BenchmarkNormalizeWhitespace(b *testing.B) { - content := "line1 \nline2\t\nline3 \t\nline4\n\n" - for b.Loop() { - NormalizeWhitespace(content) - } -} - // Additional edge case tests func TestTruncate_Unicode(t *testing.T) { @@ -216,81 +145,6 @@ func TestTruncate_Unicode(t *testing.T) { } } -func TestNormalizeWhitespace_OnlyWhitespace(t *testing.T) { - t.Parallel() - tests := []struct { - name string - content string - expected string - }{ - { - name: "only spaces", - content: " ", - expected: "", // After trimming trailing spaces and newlines, becomes empty - }, - { - name: "only tabs", - content: "\t\t\t", - expected: "", // After trimming trailing tabs and newlines, becomes empty - }, - { - name: "mixed spaces and tabs", - content: " \t \t", - expected: "", // After trimming, becomes empty - }, - { - name: "only newlines", - content: "\n\n\n", - expected: "", - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - t.Parallel() - result := NormalizeWhitespace(tt.content) - assert.Equal(t, tt.expected, result, "NormalizeWhitespace(%q) should handle whitespace-only input", tt.content) - }) - } -} - -func TestNormalizeWhitespace_ManyLines(t *testing.T) { - t.Parallel() - // Test with many lines - lines := make([]string, 100) - for i := range 100 { - lines[i] = "line with trailing spaces " - } - var content strings.Builder - for _, line := range lines { - content.WriteString(line + "\n") - } - - result := NormalizeWhitespace(content.String()) - - // Check that all trailing spaces are removed - expectedLines := make([]string, 100) - for i := range 100 { - expectedLines[i] = "line with trailing spaces" - } - var expected strings.Builder - for _, line := range expectedLines { - expected.WriteString(line + "\n") - } - - assert.Equal(t, expected.String(), result, "NormalizeWhitespace should remove trailing spaces from all lines in large inputs") -} - -func TestNormalizeWhitespace_PreservesContent(t *testing.T) { - t.Parallel() - // Ensure that non-trailing whitespace is preserved - content := "line1 middle spaces\nline2\t\tmiddle\t\ttabs\n" - result := NormalizeWhitespace(content) - - assert.Contains(t, result, "middle spaces", "NormalizeWhitespace should preserve non-trailing spaces inside lines") - assert.Contains(t, result, "middle\t\ttabs", "NormalizeWhitespace should preserve non-trailing tabs inside lines") -} - func BenchmarkTruncate_Short(b *testing.B) { s := "short" for b.Loop() { @@ -305,107 +159,6 @@ func BenchmarkTruncate_Long(b *testing.B) { } } -func BenchmarkNormalizeWhitespace_NoChange(b *testing.B) { - content := "line1\nline2\nline3\n" - for b.Loop() { - NormalizeWhitespace(content) - } -} - -func BenchmarkNormalizeWhitespace_ManyChanges(b *testing.B) { - content := "line1 \t \nline2 \t \nline3 \t \n\n\n" - for b.Loop() { - NormalizeWhitespace(content) - } -} - -func TestParseVersionValue(t *testing.T) { - t.Parallel() - tests := []struct { - name string - version any - expected string - }{ - // String versions - { - name: "string version", - version: "v1.2.3", - expected: "v1.2.3", - }, - { - name: "numeric string", - version: "123", - expected: "123", - }, - { - name: "empty string", - version: "", - expected: "", - }, - // Integer versions - { - name: "int version", - version: 42, - expected: "42", - }, - { - name: "int64 version", - version: int64(100), - expected: "100", - }, - { - name: "uint64 version", - version: uint64(999), - expected: "999", - }, - // Float versions - { - name: "float64 simple", - version: float64(1.5), - expected: "1.5", - }, - { - name: "float64 whole number", - version: float64(2.0), - expected: "2", - }, - { - name: "float64 with precision", - version: float64(1.234), - expected: "1.234", - }, - // Unsupported types - { - name: "nil", - version: nil, - expected: "", - }, - { - name: "bool", - version: true, - expected: "", - }, - { - name: "slice", - version: []string{"1", "2"}, - expected: "", - }, - { - name: "map", - version: map[string]string{"version": "1.0"}, - expected: "", - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - t.Parallel() - result := ParseVersionValue(tt.version) - assert.Equal(t, tt.expected, result, "ParseVersionValue(%v) should return normalized string representation", tt.version) - }) - } -} - func TestFormatList(t *testing.T) { t.Parallel() tests := []struct { @@ -449,59 +202,6 @@ func TestFormatList(t *testing.T) { } } -func TestNormalizeLeadingWhitespace(t *testing.T) { - t.Parallel() - tests := []struct { - name string - input string - expected string - }{ - { - name: "removes consistent leading spaces", - input: " Line 1\n Line 2\n Line 3", - expected: "Line 1\nLine 2\nLine 3", - }, - { - name: "handles no leading spaces", - input: "Line 1\nLine 2", - expected: "Line 1\nLine 2", - }, - { - name: "preserves relative indentation", - input: " Line 1\n Indented Line 2\n Line 3", - expected: "Line 1\n Indented Line 2\nLine 3", - }, - { - name: "handles empty lines", - input: " Line 1\n\n Line 3", - expected: "Line 1\n\nLine 3", - }, - { - name: "empty string", - input: "", - expected: "", - }, - { - name: "removes consistent leading tabs", - input: "\t\tLine 1\n\t\tLine 2\n\t\tLine 3", - expected: "Line 1\nLine 2\nLine 3", - }, - { - name: "removes consistent mixed tab and space indentation", - input: "\t Line 1\n\t Line 2\n\t Line 3", - expected: "Line 1\nLine 2\nLine 3", - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - t.Parallel() - result := NormalizeLeadingWhitespace(tt.input) - assert.Equal(t, tt.expected, result, "NormalizeLeadingWhitespace should normalize indentation for case %q", tt.name) - }) - } -} - func TestIsPositiveInteger(t *testing.T) { t.Parallel() tests := []struct { diff --git a/pkg/stringutil/version.go b/pkg/stringutil/version.go new file mode 100644 index 00000000000..004480e9e15 --- /dev/null +++ b/pkg/stringutil/version.go @@ -0,0 +1,26 @@ +package stringutil + +import ( + "fmt" + + "github.com/github/gh-aw/pkg/logger" +) + +var versionLog = logger.New("stringutil:version") + +// ParseVersionValue converts version values of various types to strings. +// Supports string, int, int64, uint64, and float64 types. +// Returns empty string for unsupported types. +func ParseVersionValue(version any) string { + switch v := version.(type) { + case string: + return v + case int, int64, uint64: + return fmt.Sprintf("%d", v) + case float64: + return fmt.Sprintf("%g", v) + default: + versionLog.Printf("ParseVersionValue: unsupported type %T, returning empty string", version) + return "" + } +} diff --git a/pkg/stringutil/version_test.go b/pkg/stringutil/version_test.go new file mode 100644 index 00000000000..e7d03051e79 --- /dev/null +++ b/pkg/stringutil/version_test.go @@ -0,0 +1,96 @@ +//go:build !integration + +package stringutil + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestParseVersionValue(t *testing.T) { + t.Parallel() + tests := []struct { + name string + version any + expected string + }{ + // String versions + { + name: "string version", + version: "v1.2.3", + expected: "v1.2.3", + }, + { + name: "numeric string", + version: "123", + expected: "123", + }, + { + name: "empty string", + version: "", + expected: "", + }, + // Integer versions + { + name: "int version", + version: 42, + expected: "42", + }, + { + name: "int64 version", + version: int64(100), + expected: "100", + }, + { + name: "uint64 version", + version: uint64(999), + expected: "999", + }, + // Float versions + { + name: "float64 simple", + version: float64(1.5), + expected: "1.5", + }, + { + name: "float64 whole number", + version: float64(2.0), + expected: "2", + }, + { + name: "float64 with precision", + version: float64(1.234), + expected: "1.234", + }, + // Unsupported types + { + name: "nil", + version: nil, + expected: "", + }, + { + name: "bool", + version: true, + expected: "", + }, + { + name: "slice", + version: []string{"1", "2"}, + expected: "", + }, + { + name: "map", + version: map[string]string{"version": "1.0"}, + expected: "", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + result := ParseVersionValue(tt.version) + assert.Equal(t, tt.expected, result, "ParseVersionValue(%v) should return normalized string representation", tt.version) + }) + } +} diff --git a/pkg/stringutil/whitespace.go b/pkg/stringutil/whitespace.go new file mode 100644 index 00000000000..9608e4d25cf --- /dev/null +++ b/pkg/stringutil/whitespace.go @@ -0,0 +1,80 @@ +package stringutil + +import ( + "strings" + + "github.com/github/gh-aw/pkg/logger" +) + +var whitespaceLog = logger.New("stringutil:whitespace") + +// NormalizeWhitespace normalizes trailing whitespace and newlines to reduce spurious conflicts. +// It trims trailing whitespace from each line and ensures exactly one trailing newline. +func NormalizeWhitespace(content string) string { + // Split into lines and trim trailing whitespace from each line + lines := strings.Split(content, "\n") + for i, line := range lines { + lines[i] = strings.TrimRight(line, " \t") + } + + // Join back and ensure exactly one trailing newline if content is not empty + normalized := strings.Join(lines, "\n") + normalized = strings.TrimRight(normalized, "\n") + if normalized != "" { + normalized += "\n" + } + + return normalized +} + +// NormalizeLeadingWhitespace removes consistent leading whitespace from all lines +// of a multi-line string. It finds the minimum indentation across all non-empty +// lines and strips that many leading whitespace characters (spaces or tabs) from +// every line. +// +// This is useful for cleaning up content generated with extra indentation, +// such as heredoc bodies. +func NormalizeLeadingWhitespace(content string) string { + lines := strings.Split(content, "\n") + if len(lines) == 0 { + return content + } + + // Find minimum leading whitespace (excluding empty lines) + minLeading := -1 + for _, line := range lines { + if strings.TrimSpace(line) == "" { + continue // Skip empty lines + } + leading := len(line) - len(strings.TrimLeft(line, " \t")) + if minLeading == -1 || leading < minLeading { + minLeading = leading + } + } + + // If no content or no leading whitespace, return as-is + if minLeading <= 0 { + return content + } + + whitespaceLog.Printf("NormalizeLeadingWhitespace: stripping %d leading whitespace chars from %d lines", minLeading, len(lines)) + + // Remove the minimum leading whitespace from all lines + var result strings.Builder + for i, line := range lines { + if i > 0 { + result.WriteString("\n") + } + if strings.TrimSpace(line) == "" { + // Keep empty lines as empty + result.WriteString("") + } else if len(line) >= minLeading { + // Remove leading whitespace + result.WriteString(line[minLeading:]) + } else { + result.WriteString(line) + } + } + + return result.String() +} diff --git a/pkg/stringutil/whitespace_test.go b/pkg/stringutil/whitespace_test.go new file mode 100644 index 00000000000..dc98037dcec --- /dev/null +++ b/pkg/stringutil/whitespace_test.go @@ -0,0 +1,222 @@ +//go:build !integration + +package stringutil + +import ( + "strings" + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestNormalizeWhitespace(t *testing.T) { + t.Parallel() + tests := []struct { + name string + content string + expected string + }{ + { + name: "no trailing whitespace", + content: "hello\nworld", + expected: "hello\nworld\n", + }, + { + name: "trailing spaces on lines", + content: "hello \nworld ", + expected: "hello\nworld\n", + }, + { + name: "trailing tabs on lines", + content: "hello\t\nworld\t", + expected: "hello\nworld\n", + }, + { + name: "multiple trailing newlines", + content: "hello\nworld\n\n\n", + expected: "hello\nworld\n", + }, + { + name: "empty string", + content: "", + expected: "", + }, + { + name: "single newline", + content: "\n", + expected: "", + }, + { + name: "mixed whitespace", + content: "hello \t\nworld \t \n\n", + expected: "hello\nworld\n", + }, + { + name: "content with no newline", + content: "hello world", + expected: "hello world\n", + }, + { + name: "content already normalized", + content: "hello\nworld\n", + expected: "hello\nworld\n", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + result := NormalizeWhitespace(tt.content) + assert.Equal(t, tt.expected, result, "NormalizeWhitespace(%q) should normalize trailing whitespace and newlines", tt.content) + }) + } +} + +func BenchmarkNormalizeWhitespace(b *testing.B) { + content := "line1 \nline2\t\nline3 \t\nline4\n\n" + for b.Loop() { + NormalizeWhitespace(content) + } +} + +func TestNormalizeWhitespace_OnlyWhitespace(t *testing.T) { + t.Parallel() + tests := []struct { + name string + content string + expected string + }{ + { + name: "only spaces", + content: " ", + expected: "", // After trimming trailing spaces and newlines, becomes empty + }, + { + name: "only tabs", + content: "\t\t\t", + expected: "", // After trimming trailing tabs and newlines, becomes empty + }, + { + name: "mixed spaces and tabs", + content: " \t \t", + expected: "", // After trimming, becomes empty + }, + { + name: "only newlines", + content: "\n\n\n", + expected: "", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + result := NormalizeWhitespace(tt.content) + assert.Equal(t, tt.expected, result, "NormalizeWhitespace(%q) should handle whitespace-only input", tt.content) + }) + } +} + +func TestNormalizeWhitespace_ManyLines(t *testing.T) { + t.Parallel() + // Test with many lines + lines := make([]string, 100) + for i := range 100 { + lines[i] = "line with trailing spaces " + } + var content strings.Builder + for _, line := range lines { + content.WriteString(line + "\n") + } + + result := NormalizeWhitespace(content.String()) + + // Check that all trailing spaces are removed + expectedLines := make([]string, 100) + for i := range 100 { + expectedLines[i] = "line with trailing spaces" + } + var expected strings.Builder + for _, line := range expectedLines { + expected.WriteString(line + "\n") + } + + assert.Equal(t, expected.String(), result, "NormalizeWhitespace should remove trailing spaces from all lines in large inputs") +} + +func TestNormalizeWhitespace_PreservesContent(t *testing.T) { + t.Parallel() + // Ensure that non-trailing whitespace is preserved + content := "line1 middle spaces\nline2\t\tmiddle\t\ttabs\n" + result := NormalizeWhitespace(content) + + assert.Contains(t, result, "middle spaces", "NormalizeWhitespace should preserve non-trailing spaces inside lines") + assert.Contains(t, result, "middle\t\ttabs", "NormalizeWhitespace should preserve non-trailing tabs inside lines") +} + +func BenchmarkNormalizeWhitespace_NoChange(b *testing.B) { + content := "line1\nline2\nline3\n" + for b.Loop() { + NormalizeWhitespace(content) + } +} + +func BenchmarkNormalizeWhitespace_ManyChanges(b *testing.B) { + content := "line1 \t \nline2 \t \nline3 \t \n\n\n" + for b.Loop() { + NormalizeWhitespace(content) + } +} + +func TestNormalizeLeadingWhitespace(t *testing.T) { + t.Parallel() + tests := []struct { + name string + input string + expected string + }{ + { + name: "removes consistent leading spaces", + input: " Line 1\n Line 2\n Line 3", + expected: "Line 1\nLine 2\nLine 3", + }, + { + name: "handles no leading spaces", + input: "Line 1\nLine 2", + expected: "Line 1\nLine 2", + }, + { + name: "preserves relative indentation", + input: " Line 1\n Indented Line 2\n Line 3", + expected: "Line 1\n Indented Line 2\nLine 3", + }, + { + name: "handles empty lines", + input: " Line 1\n\n Line 3", + expected: "Line 1\n\nLine 3", + }, + { + name: "empty string", + input: "", + expected: "", + }, + { + name: "removes consistent leading tabs", + input: "\t\tLine 1\n\t\tLine 2\n\t\tLine 3", + expected: "Line 1\nLine 2\nLine 3", + }, + { + name: "removes consistent mixed tab and space indentation", + input: "\t Line 1\n\t Line 2\n\t Line 3", + expected: "Line 1\nLine 2\nLine 3", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + result := NormalizeLeadingWhitespace(tt.input) + assert.Equal(t, tt.expected, result, "NormalizeLeadingWhitespace should normalize indentation for case %q", tt.name) + }) + } +} diff --git a/pkg/workflow/compiler_safe_output_jobs.go b/pkg/workflow/compiler_safe_output_jobs.go index bf71421a987..306779eb0d9 100644 --- a/pkg/workflow/compiler_safe_output_jobs.go +++ b/pkg/workflow/compiler_safe_output_jobs.go @@ -191,7 +191,7 @@ func (c *Compiler) buildCallWorkflowJobs(data *WorkflowData, markdownPath string for _, workflowName := range config.Workflows { // Build the job name: "call-{sanitized-workflow-name}" - // sanitizeJobName normalizes underscores to hyphens (NormalizeSafeOutputIdentifier + dash conversion) + // sanitizeJobName normalizes underscores and periods to hyphens. sanitizedName := sanitizeJobName(workflowName) jobName := "call-" + sanitizedName diff --git a/pkg/workflow/repo_config.go b/pkg/workflow/repo_config.go index f0ab6287b86..a8bfdd3f52a 100644 --- a/pkg/workflow/repo_config.go +++ b/pkg/workflow/repo_config.go @@ -119,6 +119,8 @@ func (m *MaintenanceConfig) IsLabelTriggerEnabled() bool { return *m.LabelTriggers } +// normalizeMaintenanceJobName normalizes a maintenance job name for +// case/whitespace-insensitive comparison, converting underscores to hyphens. func normalizeMaintenanceJobName(name string) string { normalized := strings.ToLower(strings.TrimSpace(name)) return strings.ReplaceAll(normalized, "_", "-") diff --git a/pkg/workflow/repo_config_test.go b/pkg/workflow/repo_config_test.go index 3f7def9cdd3..2cf5f198998 100644 --- a/pkg/workflow/repo_config_test.go +++ b/pkg/workflow/repo_config_test.go @@ -306,6 +306,7 @@ func TestLoadRepoConfig_DisabledJobs(t *testing.T) { require.Len(t, cfg.Maintenance.DisabledJobs, 2, "disabled_jobs should be parsed") assert.True(t, cfg.Maintenance.IsJobDisabled("close-expired-entities"), "hyphenated job name should match") assert.True(t, cfg.Maintenance.IsJobDisabled("label_apply_safe_outputs"), "underscored lookup should match hyphen/underscore equivalently") + assert.False(t, cfg.Maintenance.IsJobDisabled("close.expired.entities"), "periods should not match hyphenated job names") assert.False(t, cfg.Maintenance.IsJobDisabled("create_labels"), "unlisted jobs should remain enabled") } diff --git a/pkg/workflow/safe_outputs_call_workflow_test.go b/pkg/workflow/safe_outputs_call_workflow_test.go index 75f8a38878a..68c1480f647 100644 --- a/pkg/workflow/safe_outputs_call_workflow_test.go +++ b/pkg/workflow/safe_outputs_call_workflow_test.go @@ -23,10 +23,11 @@ func TestBuildCallWorkflowJobs_GeneratesConditionalJobs(t *testing.T) { BaseSafeOutputConfig: BaseSafeOutputConfig{ Max: strPtr("1"), }, - Workflows: []string{"spring-boot-bugfix", "frontend-dep-upgrade"}, + Workflows: []string{"spring-boot-bugfix", "frontend-dep-upgrade", "worker.v1"}, WorkflowFiles: map[string]string{ "spring-boot-bugfix": "./.github/workflows/spring-boot-bugfix.lock.yml", "frontend-dep-upgrade": "./.github/workflows/frontend-dep-upgrade.lock.yml", + "worker.v1": "./.github/workflows/worker.v1.lock.yml", }, }, }, @@ -34,9 +35,10 @@ func TestBuildCallWorkflowJobs_GeneratesConditionalJobs(t *testing.T) { jobNames, err := compiler.buildCallWorkflowJobs(workflowData, "") require.NoError(t, err, "Should not error building call-workflow jobs") - assert.Len(t, jobNames, 2, "Should generate 2 fan-out jobs") + assert.Len(t, jobNames, 3, "Should generate 3 fan-out jobs") assert.Contains(t, jobNames, "call-spring-boot-bugfix", "Should generate job for spring-boot-bugfix") assert.Contains(t, jobNames, "call-frontend-dep-upgrade", "Should generate job for frontend-dep-upgrade") + assert.Contains(t, jobNames, "call-worker-v1", "Should generate a hyphenated job ID for worker.v1") // Check that the jobs were added to the job manager job, exists := compiler.jobManager.GetJob("call-spring-boot-bugfix") @@ -50,6 +52,13 @@ func TestBuildCallWorkflowJobs_GeneratesConditionalJobs(t *testing.T) { // (including the canonical payload) are forwarded. _, hasPayload := job.With["payload"] assert.False(t, hasPayload, "Should not pass payload when worker inputs are unknown") + + dottedJob, exists := compiler.jobManager.GetJob("call-worker-v1") + require.True(t, exists, "call-worker-v1 job should exist in job manager") + assert.Equal(t, "needs.safe_outputs.outputs.call_workflow_name == 'worker.v1'", dottedJob.If, + "Should retain the dotted workflow name in the dispatch condition") + assert.Equal(t, "./.github/workflows/worker.v1.lock.yml", dottedJob.Uses, + "Should retain the dotted workflow name in the workflow path") } // TestBuildCallWorkflowJobs_NoConfig returns nil when call-workflow is not configured @@ -142,6 +151,7 @@ func TestSanitizeJobName(t *testing.T) { }{ {"spring-boot-bugfix", "spring-boot-bugfix"}, {"frontend_dep_upgrade", "frontend-dep-upgrade"}, + {"worker.v1", "worker-v1"}, {"worker123", "worker123"}, } diff --git a/pkg/workflow/strings.go b/pkg/workflow/strings.go index 6274b48c3c2..a31b74607db 100644 --- a/pkg/workflow/strings.go +++ b/pkg/workflow/strings.go @@ -34,6 +34,7 @@ // Functions: // - stringutil.NormalizeWorkflowName: Removes file extensions (.md, .lock.yml) // - stringutil.NormalizeSafeOutputIdentifier: Converts dashes to underscores +// - stringutil.NormalizeIdentifierToHyphens: Converts underscores and periods to hyphens // // Example: // @@ -231,13 +232,11 @@ func SanitizeWorkflowIDForCacheKey(workflowID string) string { } // sanitizeJobName converts a workflow name to a valid GitHub Actions job name. -// It delegates normalization to NormalizeSafeOutputIdentifier (which converts -// hyphens to underscores), then converts underscores back to hyphens for -// GitHub Actions job name conventions. +// It delegates normalization to stringutil.NormalizeIdentifierToHyphens, +// which converts underscores and periods to hyphens for GitHub Actions job +// name conventions. func sanitizeJobName(workflowName string) string { - normalized := stringutil.NormalizeSafeOutputIdentifier(workflowName) - // NormalizeSafeOutputIdentifier uses underscores; convert to hyphens for job names - return strings.ReplaceAll(normalized, "_", "-") + return stringutil.NormalizeIdentifierToHyphens(workflowName) } // sanitizeRefForPath sanitizes a git ref for use in a file path.