diff --git a/pkg/cli/audit_cross_run_render.go b/pkg/cli/audit_cross_run_render.go index a53266795ff..00b3866edb7 100644 --- a/pkg/cli/audit_cross_run_render.go +++ b/pkg/cli/audit_cross_run_render.go @@ -81,9 +81,9 @@ func renderCrossRunReportMarkdown(report *CrossRunAuditReport) { fmt.Printf("| Avg | Min | Max |\n") fmt.Printf("|-----|-----|-----|\n") fmt.Printf("| %s | %s | %s |\n", - formatDurationNs(mt.AvgDurationNs), - formatDurationNs(mt.MinDurationNs), - formatDurationNs(mt.MaxDurationNs)) + timeutil.FormatDurationNs(mt.AvgDurationNs), + timeutil.FormatDurationNs(mt.MinDurationNs), + timeutil.FormatDurationNs(mt.MaxDurationNs)) fmt.Println() } } @@ -193,7 +193,7 @@ func renderCrossRunReportMarkdown(report *CrossRunAuditReport) { } durStr := "—" if run.Duration > 0 { - durStr = formatDurationNs(int64(run.Duration)) + durStr = timeutil.FormatDurationNs(int64(run.Duration)) } fmt.Printf("| %d | %s | %s | %s | %s | %s | %s | %s | %d | %d |\n", run.RunID, run.WorkflowName, run.Conclusion, durStr, @@ -249,9 +249,9 @@ func renderCrossRunReportPretty(report *CrossRunAuditReport) { } if mt.AvgDurationNs > 0 { fmt.Fprintf(os.Stderr, " Duration: avg=%s min=%s max=%s\n", - formatDurationNs(mt.AvgDurationNs), - formatDurationNs(mt.MinDurationNs), - formatDurationNs(mt.MaxDurationNs)) + timeutil.FormatDurationNs(mt.AvgDurationNs), + timeutil.FormatDurationNs(mt.MinDurationNs), + timeutil.FormatDurationNs(mt.MaxDurationNs)) } fmt.Fprintln(os.Stderr) } @@ -339,7 +339,7 @@ func renderCrossRunReportPretty(report *CrossRunAuditReport) { } durStr := "" if run.Duration > 0 { - durStr = " dur=" + formatDurationNs(int64(run.Duration)) + durStr = " dur=" + timeutil.FormatDurationNs(int64(run.Duration)) } if !run.HasData { fmt.Fprintf(os.Stderr, " Run #%-12d %-30s %-10s (no firewall data)%s%s%s mcp_errors=%d errors=%d\n", @@ -387,11 +387,6 @@ func formatRunIDs(ids []int64) string { return strings.Join(parts, ", ") } -// formatDurationNs formats a nanosecond duration as a human-readable string. -func formatDurationNs(ns int64) string { - return timeutil.FormatDurationNs(ns) -} - // safePercent returns percentage of part/total, returning 0 when total is 0. func safePercent(part, total int) float64 { if total == 0 { diff --git a/pkg/cli/audit_report_render.go b/pkg/cli/audit_report_render.go index afb8ccd0b90..10d57118a82 100644 --- a/pkg/cli/audit_report_render.go +++ b/pkg/cli/audit_report_render.go @@ -14,6 +14,7 @@ import ( "github.com/github/gh-aw/pkg/console" "github.com/github/gh-aw/pkg/sliceutil" "github.com/github/gh-aw/pkg/stringutil" + "github.com/github/gh-aw/pkg/timeutil" ) // renderJSON outputs the audit data as JSON @@ -1062,7 +1063,7 @@ func renderTokenUsage(summary *TokenUsageSummary) { console.FormatNumber(summary.TotalOutputTokens), console.FormatNumber(cacheTokens)) fmt.Fprintf(os.Stderr, " Requests: %d (avg %s)\n", - summary.TotalRequests, FormatDurationMs(summary.AvgDurationMs())) + summary.TotalRequests, timeutil.FormatDurationMs(summary.AvgDurationMs())) if summary.CacheEfficiency > 0 { fmt.Fprintf(os.Stderr, " Cache hit: %.1f%%\n", summary.CacheEfficiency*100) } diff --git a/pkg/cli/logs_github_api.go b/pkg/cli/logs_github_api.go index 4c9067efdda..bf2ae15b2a0 100644 --- a/pkg/cli/logs_github_api.go +++ b/pkg/cli/logs_github_api.go @@ -15,12 +15,12 @@ import ( "fmt" "os" "os/exec" + "slices" "strconv" "strings" "github.com/github/gh-aw/pkg/console" "github.com/github/gh-aw/pkg/logger" - "github.com/github/gh-aw/pkg/sliceutil" "github.com/github/gh-aw/pkg/workflow" ) @@ -283,7 +283,7 @@ func listWorkflowRunsWithPagination(opts ListWorkflowRunsOptions) ([]WorkflowRun } for _, run := range runs { - if sliceutil.Contains(agenticWorkflowNames, run.WorkflowName) { + if slices.Contains(agenticWorkflowNames, run.WorkflowName) { agenticRuns = append(agenticRuns, run) } } diff --git a/pkg/cli/mcp_permissions.go b/pkg/cli/mcp_permissions.go index ebbcd0a49ee..a246a1e2151 100644 --- a/pkg/cli/mcp_permissions.go +++ b/pkg/cli/mcp_permissions.go @@ -4,10 +4,10 @@ import ( "context" "errors" "fmt" + "slices" "strings" "time" - "github.com/github/gh-aw/pkg/sliceutil" "github.com/github/gh-aw/pkg/workflow" "github.com/modelcontextprotocol/go-sdk/jsonrpc" ) @@ -89,7 +89,7 @@ func validateMCPWorkflowName(workflowName string) error { // Check if it's a valid GitHub Actions workflow name agenticWorkflowNames, nameErr := getAgenticWorkflowNames(false) - if nameErr == nil && sliceutil.Contains(agenticWorkflowNames, workflowName) { + if nameErr == nil && slices.Contains(agenticWorkflowNames, workflowName) { mcpLog.Printf("Workflow name is valid GitHub Actions workflow name: %s", workflowName) return nil } diff --git a/pkg/cli/mcp_schema_test.go b/pkg/cli/mcp_schema_test.go index 8abb8427c4c..e7bcd091fb0 100644 --- a/pkg/cli/mcp_schema_test.go +++ b/pkg/cli/mcp_schema_test.go @@ -4,10 +4,10 @@ package cli import ( "encoding/json" + "slices" "testing" "time" - "github.com/github/gh-aw/pkg/sliceutil" "github.com/google/jsonschema-go/jsonschema" ) @@ -140,7 +140,7 @@ func TestGenerateSchema(t *testing.T) { // Check that items is an array type // In v0.4.0+, nullable slices use Types []string with ["null", "array"] // instead of Type string with "array" - isArray := itemsProp.Type == "array" || sliceutil.Contains(itemsProp.Types, "array") + isArray := itemsProp.Type == "array" || slices.Contains(itemsProp.Types, "array") if !isArray { t.Errorf("Expected items to be an array type, got Type='%s', Types=%v", itemsProp.Type, itemsProp.Types) } diff --git a/pkg/cli/mcp_tool_schemas_test.go b/pkg/cli/mcp_tool_schemas_test.go index 7136f7ef7ee..e15618b1672 100644 --- a/pkg/cli/mcp_tool_schemas_test.go +++ b/pkg/cli/mcp_tool_schemas_test.go @@ -4,9 +4,8 @@ package cli import ( "encoding/json" + "slices" "testing" - - "github.com/github/gh-aw/pkg/sliceutil" ) // TestMCPToolOutputSchemas verifies that output schemas are correctly generated for MCP tools @@ -102,7 +101,7 @@ func TestMCPToolOutputSchemas(t *testing.T) { // This will be an array schema // In v0.4.0+, nullable arrays use Types []string with ["null", "array"] // instead of Type string with "array" - isArray := schema.Type == "array" || sliceutil.Contains(schema.Types, "array") + isArray := schema.Type == "array" || slices.Contains(schema.Types, "array") if !isArray { t.Errorf("Expected schema to be an array type, got Type='%s', Types=%v", schema.Type, schema.Types) } diff --git a/pkg/cli/token_usage.go b/pkg/cli/token_usage.go index f02ff9e7c77..7777f091b84 100644 --- a/pkg/cli/token_usage.go +++ b/pkg/cli/token_usage.go @@ -264,12 +264,6 @@ func (s *TokenUsageSummary) AvgDurationMs() int { return s.TotalDurationMs / s.TotalRequests } -// FormatDurationMs formats milliseconds as a human-readable string. -// Deprecated: Use timeutil.FormatDurationMs instead. -func FormatDurationMs(ms int) string { - return timeutil.FormatDurationMs(ms) -} - // ModelRows returns the by-model data as sorted rows for console rendering func (s *TokenUsageSummary) ModelRows() []ModelTokenUsageRow { rows := make([]ModelTokenUsageRow, 0, len(s.ByModel)) @@ -287,7 +281,7 @@ func (s *TokenUsageSummary) ModelRows() []ModelTokenUsageRow { CacheWriteTokens: usage.CacheWriteTokens, EffectiveTokens: usage.EffectiveTokens, Requests: usage.Requests, - AvgDuration: FormatDurationMs(avgDur), + AvgDuration: timeutil.FormatDurationMs(avgDur), }) } // Sort by total tokens descending diff --git a/pkg/cli/token_usage_test.go b/pkg/cli/token_usage_test.go index fe218ec926f..b7a4b4bab21 100644 --- a/pkg/cli/token_usage_test.go +++ b/pkg/cli/token_usage_test.go @@ -8,6 +8,7 @@ import ( "testing" "github.com/github/gh-aw/pkg/testutil" + "github.com/github/gh-aw/pkg/timeutil" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -230,7 +231,7 @@ func TestFormatDurationMs(t *testing.T) { for _, tt := range tests { t.Run(tt.expected, func(t *testing.T) { - assert.Equal(t, tt.expected, FormatDurationMs(tt.ms), "FormatDurationMs(%d)", tt.ms) + assert.Equal(t, tt.expected, timeutil.FormatDurationMs(tt.ms), "FormatDurationMs(%d)", tt.ms) }) } } diff --git a/pkg/mathutil/mathutil.go b/pkg/mathutil/mathutil.go deleted file mode 100644 index b474ab1d643..00000000000 --- a/pkg/mathutil/mathutil.go +++ /dev/null @@ -1,18 +0,0 @@ -// Package mathutil provides basic mathematical utility functions. -package mathutil - -// Min returns the smaller of two integers. -func Min(a, b int) int { - if a < b { - return a - } - return b -} - -// Max returns the larger of two integers. -func Max(a, b int) int { - if a > b { - return a - } - return b -} diff --git a/pkg/mathutil/mathutil_test.go b/pkg/mathutil/mathutil_test.go deleted file mode 100644 index 856f1118e6a..00000000000 --- a/pkg/mathutil/mathutil_test.go +++ /dev/null @@ -1,333 +0,0 @@ -//go:build !integration - -package mathutil - -import "testing" - -func TestMin(t *testing.T) { - tests := []struct { - name string - a int - b int - expected int - }{ - { - name: "a less than b", - a: 5, - b: 10, - expected: 5, - }, - { - name: "b less than a", - a: 10, - b: 5, - expected: 5, - }, - { - name: "equal values", - a: 7, - b: 7, - expected: 7, - }, - { - name: "negative values", - a: -5, - b: -10, - expected: -10, - }, - { - name: "mixed signs", - a: -5, - b: 10, - expected: -5, - }, - { - name: "zero values", - a: 0, - b: 0, - expected: 0, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - result := Min(tt.a, tt.b) - if result != tt.expected { - t.Errorf("Min(%d, %d) = %d; want %d", tt.a, tt.b, result, tt.expected) - } - }) - } -} - -func TestMax(t *testing.T) { - tests := []struct { - name string - a int - b int - expected int - }{ - { - name: "a greater than b", - a: 10, - b: 5, - expected: 10, - }, - { - name: "b greater than a", - a: 5, - b: 10, - expected: 10, - }, - { - name: "equal values", - a: 7, - b: 7, - expected: 7, - }, - { - name: "negative values", - a: -5, - b: -10, - expected: -5, - }, - { - name: "mixed signs", - a: -5, - b: 10, - expected: 10, - }, - { - name: "zero values", - a: 0, - b: 0, - expected: 0, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - result := Max(tt.a, tt.b) - if result != tt.expected { - t.Errorf("Max(%d, %d) = %d; want %d", tt.a, tt.b, result, tt.expected) - } - }) - } -} - -func BenchmarkMin(b *testing.B) { - for b.Loop() { - Min(42, 100) - } -} - -func BenchmarkMax(b *testing.B) { - for b.Loop() { - Max(42, 100) - } -} - -// Additional edge case tests - -func TestMin_LargeNumbers(t *testing.T) { - tests := []struct { - name string - a int - b int - expected int - }{ - { - name: "very large positive", - a: 2147483647, // MaxInt32 - b: 2147483646, - expected: 2147483646, - }, - { - name: "very large negative", - a: -2147483648, // MinInt32 - b: -2147483647, - expected: -2147483648, - }, - { - name: "max positive vs zero", - a: 2147483647, - b: 0, - expected: 0, - }, - { - name: "min negative vs zero", - a: -2147483648, - b: 0, - expected: -2147483648, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - result := Min(tt.a, tt.b) - if result != tt.expected { - t.Errorf("Min(%d, %d) = %d; want %d", tt.a, tt.b, result, tt.expected) - } - }) - } -} - -func TestMax_LargeNumbers(t *testing.T) { - tests := []struct { - name string - a int - b int - expected int - }{ - { - name: "very large positive", - a: 2147483647, // MaxInt32 - b: 2147483646, - expected: 2147483647, - }, - { - name: "very large negative", - a: -2147483648, // MinInt32 - b: -2147483647, - expected: -2147483647, - }, - { - name: "max positive vs zero", - a: 2147483647, - b: 0, - expected: 2147483647, - }, - { - name: "min negative vs zero", - a: -2147483648, - b: 0, - expected: 0, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - result := Max(tt.a, tt.b) - if result != tt.expected { - t.Errorf("Max(%d, %d) = %d; want %d", tt.a, tt.b, result, tt.expected) - } - }) - } -} - -func TestMin_Commutative(t *testing.T) { - // Test that Min(a, b) == Min(b, a) - testCases := []struct { - a, b int - }{ - {5, 10}, - {-5, 10}, - {-10, -5}, - {0, 5}, - {100, 50}, - } - - for _, tc := range testCases { - result1 := Min(tc.a, tc.b) - result2 := Min(tc.b, tc.a) - - if result1 != result2 { - t.Errorf("Min is not commutative: Min(%d, %d) = %d, but Min(%d, %d) = %d", - tc.a, tc.b, result1, tc.b, tc.a, result2) - } - } -} - -func TestMax_Commutative(t *testing.T) { - // Test that Max(a, b) == Max(b, a) - testCases := []struct { - a, b int - }{ - {5, 10}, - {-5, 10}, - {-10, -5}, - {0, 5}, - {100, 50}, - } - - for _, tc := range testCases { - result1 := Max(tc.a, tc.b) - result2 := Max(tc.b, tc.a) - - if result1 != result2 { - t.Errorf("Max is not commutative: Max(%d, %d) = %d, but Max(%d, %d) = %d", - tc.a, tc.b, result1, tc.b, tc.a, result2) - } - } -} - -func TestMinMax_Consistency(t *testing.T) { - // Test that min(a, b) <= max(a, b) for all a, b - testCases := []struct { - a, b int - }{ - {5, 10}, - {10, 5}, - {-5, 10}, - {-10, -5}, - {0, 0}, - {100, -100}, - } - - for _, tc := range testCases { - minVal := Min(tc.a, tc.b) - maxVal := Max(tc.a, tc.b) - - if minVal > maxVal { - t.Errorf("Min(%d, %d) = %d should be <= Max(%d, %d) = %d", - tc.a, tc.b, minVal, tc.a, tc.b, maxVal) - } - } -} - -func TestMin_Identity(t *testing.T) { - // Test that Min(x, x) == x - values := []int{-100, -1, 0, 1, 100, 2147483647, -2147483648} - - for _, val := range values { - result := Min(val, val) - if result != val { - t.Errorf("Min(%d, %d) = %d; want %d", val, val, result, val) - } - } -} - -func TestMax_Identity(t *testing.T) { - // Test that Max(x, x) == x - values := []int{-100, -1, 0, 1, 100, 2147483647, -2147483648} - - for _, val := range values { - result := Max(val, val) - if result != val { - t.Errorf("Max(%d, %d) = %d; want %d", val, val, result, val) - } - } -} - -func BenchmarkMin_Negative(b *testing.B) { - for b.Loop() { - Min(-42, -100) - } -} - -func BenchmarkMax_Negative(b *testing.B) { - for b.Loop() { - Max(-42, -100) - } -} - -func BenchmarkMin_Large(b *testing.B) { - for b.Loop() { - Min(2147483647, 2147483646) - } -} - -func BenchmarkMax_Large(b *testing.B) { - for b.Loop() { - Max(2147483647, 2147483646) - } -} diff --git a/pkg/sliceutil/sliceutil.go b/pkg/sliceutil/sliceutil.go index 3f5c2eecbad..8d506f1c87a 100644 --- a/pkg/sliceutil/sliceutil.go +++ b/pkg/sliceutil/sliceutil.go @@ -5,11 +5,6 @@ import ( "slices" ) -// Contains checks if a string slice contains a specific string. -func Contains(slice []string, item string) bool { - return slices.Contains(slice, item) -} - // 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 f8432054153..b023d3ca90b 100644 --- a/pkg/sliceutil/sliceutil_test.go +++ b/pkg/sliceutil/sliceutil_test.go @@ -9,99 +9,8 @@ import ( "github.com/stretchr/testify/assert" ) -func TestContains(t *testing.T) { - tests := []struct { - name string - slice []string - item string - expected bool - }{ - { - name: "item exists in slice", - slice: []string{"apple", "banana", "cherry"}, - item: "banana", - expected: true, - }, - { - name: "item does not exist in slice", - slice: []string{"apple", "banana", "cherry"}, - item: "grape", - expected: false, - }, - { - name: "empty slice", - slice: []string{}, - item: "apple", - expected: false, - }, - { - name: "nil slice", - slice: nil, - item: "apple", - expected: false, - }, - { - name: "empty string item exists", - slice: []string{"", "apple", "banana"}, - item: "", - expected: true, - }, - { - name: "empty string item does not exist", - slice: []string{"apple", "banana"}, - item: "", - expected: false, - }, - { - name: "item in slice with duplicates", - slice: []string{"apple", "banana", "apple", "cherry", "apple"}, - item: "apple", - expected: true, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - result := Contains(tt.slice, tt.item) - assert.Equal(t, tt.expected, result, - "Contains should return correct value for slice %v and item %q", tt.slice, tt.item) - }) - } -} - -func BenchmarkContains(b *testing.B) { - slice := []string{"apple", "banana", "cherry", "date", "elderberry"} - for b.Loop() { - Contains(slice, "cherry") - } -} - // Additional edge case tests for better coverage -func TestContains_LargeSlice(t *testing.T) { - // Test with a large slice - largeSlice := make([]string, 1000) - for i := range 1000 { - largeSlice[i] = string(rune('a' + i%26)) - } - - // Item at beginning - assert.True(t, Contains(largeSlice, "a"), "should find 'a' at beginning of large slice") - - // Item at end - assert.True(t, Contains(largeSlice, string(rune('a'+999%26))), "should find item at end of large slice") - - // Item not in slice - assert.False(t, Contains(largeSlice, "not-present"), "should not find non-existent item in large slice") -} - -func TestContains_SingleElement(t *testing.T) { - slice := []string{"single"} - - assert.True(t, Contains(slice, "single"), "should find item in single-element slice") - assert.False(t, Contains(slice, "other"), "should not find different item in single-element slice") -} - func TestFilter(t *testing.T) { tests := []struct { name string diff --git a/pkg/stringutil/sanitize.go b/pkg/stringutil/sanitize.go index 8d4f1fd042d..72f42a79701 100644 --- a/pkg/stringutil/sanitize.go +++ b/pkg/stringutil/sanitize.go @@ -80,6 +80,29 @@ func SanitizeErrorMessage(message string) string { return sanitized } +// sanitizeIdentifierName converts a name to a valid identifier by replacing +// disallowed characters with underscores. The extraAllowed function determines +// which additional runes (beyond a-z, A-Z, 0-9, _) are permitted. +// If the resulting name starts with a digit, an underscore is prepended. +func sanitizeIdentifierName(name string, extraAllowed func(rune) bool) string { + result := strings.Map(func(r rune) rune { + if (r >= 'a' && r <= 'z') || (r >= 'A' && r <= 'Z') || (r >= '0' && r <= '9') || r == '_' { + return r + } + if extraAllowed != nil && extraAllowed(r) { + return r + } + return '_' + }, name) + + // Ensure it doesn't start with a number + if len(result) > 0 && result[0] >= '0' && result[0] <= '9' { + result = "_" + result + } + + return result +} + // SanitizeParameterName converts a parameter name to a safe JavaScript identifier // by replacing non-alphanumeric characters with underscores. // @@ -98,20 +121,7 @@ func SanitizeErrorMessage(message string) string { // SanitizeParameterName("valid_name") // returns "valid_name" // SanitizeParameterName("$special") // returns "$special" func SanitizeParameterName(name string) string { - // Replace dashes and other non-alphanumeric chars with underscores - result := strings.Map(func(r rune) rune { - if (r >= 'a' && r <= 'z') || (r >= 'A' && r <= 'Z') || (r >= '0' && r <= '9') || r == '_' || r == '$' { - return r - } - return '_' - }, name) - - // Ensure it doesn't start with a number - if len(result) > 0 && result[0] >= '0' && result[0] <= '9' { - result = "_" + result - } - - return result + return sanitizeIdentifierName(name, func(r rune) bool { return r == '$' }) } // SanitizePythonVariableName converts a parameter name to a valid Python identifier @@ -132,20 +142,7 @@ func SanitizeParameterName(name string) string { // SanitizePythonVariableName("123param") // returns "_123param" // SanitizePythonVariableName("valid_name") // returns "valid_name" func SanitizePythonVariableName(name string) string { - // Replace dashes and other non-alphanumeric chars with underscores - result := strings.Map(func(r rune) rune { - if (r >= 'a' && r <= 'z') || (r >= 'A' && r <= 'Z') || (r >= '0' && r <= '9') || r == '_' { - return r - } - return '_' - }, name) - - // Ensure it doesn't start with a number - if len(result) > 0 && result[0] >= '0' && result[0] <= '9' { - result = "_" + result - } - - return result + return sanitizeIdentifierName(name, nil) } // SanitizeToolID removes common MCP prefixes and suffixes from tool IDs. diff --git a/pkg/workflow/compiler_activation_job.go b/pkg/workflow/compiler_activation_job.go index f8fbcedf37d..c64eb953aeb 100644 --- a/pkg/workflow/compiler_activation_job.go +++ b/pkg/workflow/compiler_activation_job.go @@ -5,11 +5,11 @@ import ( "errors" "fmt" "maps" + "slices" "strings" "github.com/github/gh-aw/pkg/constants" "github.com/github/gh-aw/pkg/logger" - "github.com/github/gh-aw/pkg/sliceutil" "github.com/github/gh-aw/pkg/stringutil" ) @@ -108,10 +108,10 @@ func (c *Compiler) buildActivationJob(data *WorkflowData, preActivationJobCreate appPerms.Set(PermissionDiscussions, PermissionWrite) } if shouldRemoveLabel { - if sliceutil.Contains(filteredLabelEvents, "issues") || sliceutil.Contains(filteredLabelEvents, "pull_request") { + if slices.Contains(filteredLabelEvents, "issues") || slices.Contains(filteredLabelEvents, "pull_request") { appPerms.Set(PermissionIssues, PermissionWrite) } - if sliceutil.Contains(filteredLabelEvents, "discussion") { + if slices.Contains(filteredLabelEvents, "discussion") { appPerms.Set(PermissionDiscussions, PermissionWrite) } } @@ -435,7 +435,7 @@ func (c *Compiler) buildActivationJob(data *WorkflowData, preActivationJobCreate // run yet, causing actionlint errors and incorrect prompt substitutions. promptReferencedJobs := c.getCustomJobsReferencedInPromptWithNoActivationDep(data) for _, jobName := range promptReferencedJobs { - if !sliceutil.Contains(customJobsBeforeActivation, jobName) { + if !slices.Contains(customJobsBeforeActivation, jobName) { customJobsBeforeActivation = append(customJobsBeforeActivation, jobName) compilerActivationJobLog.Printf("Added '%s' to activation dependencies: referenced in markdown body and has no explicit needs", jobName) } @@ -551,10 +551,10 @@ func (c *Compiler) buildActivationJob(data *WorkflowData, preActivationJobCreate // for the label removal step (it uses the app token instead), so we skip them. // When remove_label is false, no label removal occurs so these permissions are not needed. if shouldRemoveLabel && data.ActivationGitHubApp == nil { - if sliceutil.Contains(filteredLabelEvents, "issues") || sliceutil.Contains(filteredLabelEvents, "pull_request") { + if slices.Contains(filteredLabelEvents, "issues") || slices.Contains(filteredLabelEvents, "pull_request") { permsMap[PermissionIssues] = PermissionWrite } - if sliceutil.Contains(filteredLabelEvents, "discussion") { + if slices.Contains(filteredLabelEvents, "discussion") { permsMap[PermissionDiscussions] = PermissionWrite } } diff --git a/pkg/workflow/compiler_yaml_helpers.go b/pkg/workflow/compiler_workflow_helpers.go similarity index 74% rename from pkg/workflow/compiler_yaml_helpers.go rename to pkg/workflow/compiler_workflow_helpers.go index 541cd841d04..4073f3dc1b1 100644 --- a/pkg/workflow/compiler_yaml_helpers.go +++ b/pkg/workflow/compiler_workflow_helpers.go @@ -7,7 +7,7 @@ import ( "github.com/github/gh-aw/pkg/logger" ) -var compilerYamlHelpersLog = logger.New("workflow:compiler_yaml_helpers") +var compilerWorkflowHelpersLog = logger.New("workflow:compiler_workflow_helpers") // ContainsCheckout returns true if the given custom steps contain an actions/checkout step func ContainsCheckout(customSteps string) bool { @@ -25,7 +25,7 @@ func ContainsCheckout(customSteps string) bool { lowerSteps := strings.ToLower(customSteps) for _, pattern := range checkoutPatterns { if strings.Contains(lowerSteps, strings.ToLower(pattern)) { - compilerYamlHelpersLog.Print("Detected actions/checkout in custom steps") + compilerWorkflowHelpersLog.Print("Detected actions/checkout in custom steps") return true } } @@ -39,7 +39,3 @@ func ContainsCheckout(customSteps string) bool { func GetWorkflowIDFromPath(markdownPath string) string { return strings.TrimSuffix(filepath.Base(markdownPath), ".md") } - -// generateGitHubScriptWithRequire is implemented in compiler_github_actions_steps.go - -// generateInlineGitHubScriptStep is implemented in compiler_github_actions_steps.go diff --git a/pkg/workflow/strict_mode_steps_validation.go b/pkg/workflow/strict_mode_steps_validation.go index e6ab8173073..815758e59ca 100644 --- a/pkg/workflow/strict_mode_steps_validation.go +++ b/pkg/workflow/strict_mode_steps_validation.go @@ -16,6 +16,7 @@ import ( "strings" "github.com/github/gh-aw/pkg/console" + "github.com/github/gh-aw/pkg/sliceutil" ) // validateStepsSecrets checks both the "steps" and "post-steps" frontmatter sections @@ -65,7 +66,7 @@ func (c *Compiler) validateStepsSectionSecrets(frontmatter map[string]any, secti strictModeValidationLog.Printf("Found %d secret expression(s) in %s section: %v", len(secretRefs), sectionName, secretRefs) // Deduplicate for cleaner messages. - secretRefs = deduplicateStringSlice(secretRefs) + secretRefs = sliceutil.Deduplicate(secretRefs) if c.strictMode { return fmt.Errorf( @@ -108,20 +109,6 @@ func extractSecretsFromStepValue(value any) []string { return refs } -// deduplicateStringSlice returns a slice with duplicate entries removed, -// preserving the order of first occurrence. -func deduplicateStringSlice(in []string) []string { - seen := make(map[string]bool, len(in)) - out := make([]string, 0, len(in)) - for _, s := range in { - if !seen[s] { - seen[s] = true - out = append(out, s) - } - } - return out -} - // filterBuiltinTokens removes secret expressions that reference GitHub's built-in // GITHUB_TOKEN from the list. GITHUB_TOKEN is automatically provided by the runner // environment and is not a user-defined secret; it therefore does not represent an diff --git a/pkg/workflow/strict_mode_steps_validation_test.go b/pkg/workflow/strict_mode_steps_validation_test.go index 873ad8fcfc6..ba13d4b6adc 100644 --- a/pkg/workflow/strict_mode_steps_validation_test.go +++ b/pkg/workflow/strict_mode_steps_validation_test.go @@ -5,6 +5,7 @@ package workflow import ( "testing" + "github.com/github/gh-aw/pkg/sliceutil" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -292,7 +293,7 @@ func TestDeduplicateStringSlice(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - result := deduplicateStringSlice(tt.input) + result := sliceutil.Deduplicate(tt.input) assert.Equal(t, tt.expected, result, "unexpected deduplication result") }) }