From 109749dbf6ed3929e1ad5eeb04f674a9cc6c5648 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 23 Feb 2026 18:50:22 +0000 Subject: [PATCH 1/2] Initial plan From 88c2f440bf09f73f35bf8626274dec58818743b4 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 23 Feb 2026 19:13:15 +0000 Subject: [PATCH 2/2] refactor: remove thin-wrapper functions, move test helper to test file, consolidate formatDuration" Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/cli/health_metrics.go | 34 +-------- pkg/cli/health_metrics_test.go | 46 ------------ pkg/parser/content_extractor.go | 71 ------------------- pkg/parser/content_extractor_features_test.go | 22 +++--- pkg/parser/engine_includes_test.go | 2 +- pkg/parser/import_processor.go | 28 ++++---- pkg/parser/include_expander.go | 16 +++-- pkg/workflow/safe_outputs_env.go | 17 ----- pkg/workflow/safe_outputs_env_helpers_test.go | 20 ++++++ 9 files changed, 60 insertions(+), 196 deletions(-) create mode 100644 pkg/workflow/safe_outputs_env_helpers_test.go diff --git a/pkg/cli/health_metrics.go b/pkg/cli/health_metrics.go index 76959caf13..9ada928f85 100644 --- a/pkg/cli/health_metrics.go +++ b/pkg/cli/health_metrics.go @@ -6,6 +6,7 @@ import ( "time" "github.com/github/gh-aw/pkg/logger" + "github.com/github/gh-aw/pkg/timeutil" ) var healthMetricsLog = logger.New("cli:health_metrics") @@ -120,7 +121,7 @@ func CalculateWorkflowHealth(workflowName string, runs []WorkflowRun, threshold // Format display values displayRate := fmt.Sprintf("%.0f%% (%d/%d)", successRate, successCount, totalRuns) - displayDur := formatDuration(avgDuration) + displayDur := timeutil.FormatDuration(avgDuration) displayTokens := formatTokens(avgTokens) displayCost := formatCost(avgCost) @@ -196,37 +197,6 @@ func calculateSuccessRate(runs []WorkflowRun) float64 { return float64(successCount) / float64(len(runs)) * 100 } -// formatDuration formats a duration in a human-readable format -func formatDuration(d time.Duration) string { - if d == 0 { - return "0s" - } - - // Round to seconds - seconds := int(d.Seconds()) - if seconds < 60 { - return fmt.Sprintf("%ds", seconds) - } - - minutes := seconds / 60 - remainingSeconds := seconds % 60 - - if minutes < 60 { - if remainingSeconds > 0 { - return fmt.Sprintf("%dm %ds", minutes, remainingSeconds) - } - return fmt.Sprintf("%dm", minutes) - } - - hours := minutes / 60 - remainingMinutes := minutes % 60 - - if remainingMinutes > 0 { - return fmt.Sprintf("%dh %dm", hours, remainingMinutes) - } - return fmt.Sprintf("%dh", hours) -} - // CalculateHealthSummary calculates aggregated health metrics across all workflows func CalculateHealthSummary(workflowHealths []WorkflowHealth, period string, threshold float64) HealthSummary { healthMetricsLog.Printf("Calculating health summary: workflows=%d, period=%s", len(workflowHealths), period) diff --git a/pkg/cli/health_metrics_test.go b/pkg/cli/health_metrics_test.go index 33ee9b7b27..ae0fc6d89d 100644 --- a/pkg/cli/health_metrics_test.go +++ b/pkg/cli/health_metrics_test.go @@ -157,52 +157,6 @@ func TestCalculateTrend(t *testing.T) { } } -func TestFormatDuration(t *testing.T) { - tests := []struct { - name string - duration time.Duration - expected string - }{ - { - name: "zero duration", - duration: 0, - expected: "0s", - }, - { - name: "seconds only", - duration: 45 * time.Second, - expected: "45s", - }, - { - name: "minutes only", - duration: 5 * time.Minute, - expected: "5m", - }, - { - name: "minutes and seconds", - duration: 2*time.Minute + 30*time.Second, - expected: "2m 30s", - }, - { - name: "hours only", - duration: 2 * time.Hour, - expected: "2h", - }, - { - name: "hours and minutes", - duration: 1*time.Hour + 15*time.Minute, - expected: "1h 15m", - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - result := formatDuration(tt.duration) - assert.Equal(t, tt.expected, result, "Formatted duration should match") - }) - } -} - func TestGroupRunsByWorkflow(t *testing.T) { runs := []WorkflowRun{ {WorkflowName: "workflow-a", Conclusion: "success"}, diff --git a/pkg/parser/content_extractor.go b/pkg/parser/content_extractor.go index a20e743ffb..f71addd6d7 100644 --- a/pkg/parser/content_extractor.go +++ b/pkg/parser/content_extractor.go @@ -54,22 +54,6 @@ func extractToolsFromContent(content string) (string, error) { return strings.TrimSpace(string(extractedJSON)), nil } -// extractSafeOutputsFromContent extracts safe-outputs section from frontmatter as JSON string -func extractSafeOutputsFromContent(content string) (string, error) { - contentExtractorLog.Print("Extracting safe-outputs from content") - return extractFrontmatterField(content, "safe-outputs", "{}") -} - -// extractSafeInputsFromContent extracts safe-inputs section from frontmatter as JSON string -func extractSafeInputsFromContent(content string) (string, error) { - return extractFrontmatterField(content, "safe-inputs", "{}") -} - -// extractMCPServersFromContent extracts mcp-servers section from frontmatter as JSON string -func extractMCPServersFromContent(content string) (string, error) { - return extractFrontmatterField(content, "mcp-servers", "{}") -} - // extractStepsFromContent extracts steps section from frontmatter as YAML string func extractStepsFromContent(content string) (string, error) { result, err := ExtractFrontmatterFromContent(content) @@ -92,16 +76,6 @@ func extractStepsFromContent(content string) (string, error) { return strings.TrimSpace(string(stepsYAML)), nil } -// extractEngineFromContent extracts engine section from frontmatter as JSON string -func extractEngineFromContent(content string) (string, error) { - return extractFrontmatterField(content, "engine", "") -} - -// extractRuntimesFromContent extracts runtimes section from frontmatter as JSON string -func extractRuntimesFromContent(content string) (string, error) { - return extractFrontmatterField(content, "runtimes", "{}") -} - // extractServicesFromContent extracts services section from frontmatter as YAML string func extractServicesFromContent(content string) (string, error) { result, err := ExtractFrontmatterFromContent(content) @@ -124,41 +98,11 @@ func extractServicesFromContent(content string) (string, error) { return strings.TrimSpace(string(servicesYAML)), nil } -// extractNetworkFromContent extracts network section from frontmatter as JSON string -func extractNetworkFromContent(content string) (string, error) { - return extractFrontmatterField(content, "network", "{}") -} - // ExtractPermissionsFromContent extracts permissions section from frontmatter as JSON string func ExtractPermissionsFromContent(content string) (string, error) { return extractFrontmatterField(content, "permissions", "{}") } -// extractSecretMaskingFromContent extracts secret-masking section from frontmatter as JSON string -func extractSecretMaskingFromContent(content string) (string, error) { - return extractFrontmatterField(content, "secret-masking", "{}") -} - -// extractBotsFromContent extracts bots section from frontmatter as JSON string -func extractBotsFromContent(content string) (string, error) { - return extractFrontmatterField(content, "bots", "[]") -} - -// extractSkipRolesFromContent extracts skip-roles from on: section as JSON string -func extractSkipRolesFromContent(content string) (string, error) { - return extractOnSectionField(content, "skip-roles") -} - -// extractSkipBotsFromContent extracts skip-bots from on: section as JSON string -func extractSkipBotsFromContent(content string) (string, error) { - return extractOnSectionField(content, "skip-bots") -} - -// extractPluginsFromContent extracts plugins section from frontmatter as JSON string -func extractPluginsFromContent(content string) (string, error) { - return extractFrontmatterField(content, "plugins", "[]") -} - // extractPostStepsFromContent extracts post-steps section from frontmatter as YAML string func extractPostStepsFromContent(content string) (string, error) { result, err := ExtractFrontmatterFromContent(content) @@ -181,21 +125,6 @@ func extractPostStepsFromContent(content string) (string, error) { return strings.TrimSpace(string(postStepsYAML)), nil } -// extractLabelsFromContent extracts labels section from frontmatter as JSON string -func extractLabelsFromContent(content string) (string, error) { - return extractFrontmatterField(content, "labels", "[]") -} - -// extractCacheFromContent extracts cache section from frontmatter as JSON string -func extractCacheFromContent(content string) (string, error) { - return extractFrontmatterField(content, "cache", "{}") -} - -// extractFeaturesFromContent extracts features section from frontmatter as JSON string -func extractFeaturesFromContent(content string) (string, error) { - return extractFrontmatterField(content, "features", "{}") -} - // extractFrontmatterField extracts a specific field from frontmatter as JSON string func extractFrontmatterField(content, fieldName, emptyValue string) (string, error) { contentExtractorLog.Printf("Extracting field: %s", fieldName) diff --git a/pkg/parser/content_extractor_features_test.go b/pkg/parser/content_extractor_features_test.go index 06f5f8b648..5f016bb90b 100644 --- a/pkg/parser/content_extractor_features_test.go +++ b/pkg/parser/content_extractor_features_test.go @@ -19,7 +19,7 @@ features: # Test Workflow ` - result, err := extractFeaturesFromContent(content) + result, err := extractFrontmatterField(content, "features", "{}") require.NoError(t, err, "Should not error on valid features") assert.NotEmpty(t, result, "Should return non-empty result") assert.Contains(t, result, "feature1", "Should contain feature1") @@ -35,7 +35,7 @@ on: issues # Test Workflow ` - result, err := extractFeaturesFromContent(content) + result, err := extractFrontmatterField(content, "features", "{}") require.NoError(t, err, "Should not error when no features") assert.Equal(t, "{}", result, "Should return empty object when no features") } @@ -47,7 +47,7 @@ features: {} # Test Workflow ` - result, err := extractFeaturesFromContent(content) + result, err := extractFrontmatterField(content, "features", "{}") require.NoError(t, err, "Should not error on empty features") assert.Equal(t, "{}", result, "Should return empty object for empty features") } @@ -57,7 +57,7 @@ func TestExtractFeaturesFromContent_NoFrontmatter(t *testing.T) { This workflow has no frontmatter. ` - result, err := extractFeaturesFromContent(content) + result, err := extractFrontmatterField(content, "features", "{}") require.NoError(t, err, "Should not error when no frontmatter") assert.Equal(t, "{}", result, "Should return empty object when no frontmatter") } @@ -78,7 +78,7 @@ features: # Test Workflow ` - result, err := extractFeaturesFromContent(content) + result, err := extractFrontmatterField(content, "features", "{}") require.NoError(t, err, "Should not error on complex features") assert.NotEmpty(t, result, "Should return non-empty result") assert.Contains(t, result, "string-feature", "Should contain string-feature") @@ -95,7 +95,7 @@ features: [this is not valid yaml # Test Workflow ` - result, err := extractFeaturesFromContent(content) + result, err := extractFrontmatterField(content, "features", "{}") // Should return empty object on error, not propagate error require.NoError(t, err, "Should not error on malformed YAML") assert.Equal(t, "{}", result, "Should return empty object on malformed YAML") @@ -112,7 +112,7 @@ features: # Test Workflow ` - result, err := extractFeaturesFromContent(content) + result, err := extractFrontmatterField(content, "features", "{}") require.NoError(t, err, "Should not error on features with special characters") assert.NotEmpty(t, result, "Should return non-empty result") // The exact keys depend on YAML parsing, but we should get some features @@ -127,7 +127,7 @@ features: # Test Workflow ` - result, err := extractFeaturesFromContent(content) + result, err := extractFrontmatterField(content, "features", "{}") require.NoError(t, err, "Should not error") assert.NotEmpty(t, result, "Should return features") assert.Contains(t, result, "test-feature", "Should contain test-feature") @@ -146,7 +146,7 @@ permissions: # Test Workflow ` - result, err := extractFeaturesFromContent(content) + result, err := extractFrontmatterField(content, "features", "{}") require.NoError(t, err, "Should not error") assert.NotEmpty(t, result, "Should return features") assert.Contains(t, result, "test-feature", "Should contain test-feature") @@ -169,7 +169,7 @@ features: # Test Workflow ` - result, err := extractFeaturesFromContent(content) + result, err := extractFrontmatterField(content, "features", "{}") require.NoError(t, err, "Should not error") assert.NotEmpty(t, result, "Should return features") // All these should be parsed as boolean features @@ -188,7 +188,7 @@ features: # Test Workflow ` - result, err := extractFeaturesFromContent(content) + result, err := extractFrontmatterField(content, "features", "{}") require.NoError(t, err, "Should not error") assert.NotEmpty(t, result, "Should return features") assert.Contains(t, result, "int-feature", "Should contain int-feature") diff --git a/pkg/parser/engine_includes_test.go b/pkg/parser/engine_includes_test.go index d6cf852ec3..d4edae816a 100644 --- a/pkg/parser/engine_includes_test.go +++ b/pkg/parser/engine_includes_test.go @@ -280,7 +280,7 @@ Just markdown content. for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - result, err := extractEngineFromContent(tt.content) + result, err := extractFrontmatterField(tt.content, "engine", "") if err != nil { t.Fatalf("Unexpected error: %v", err) } diff --git a/pkg/parser/import_processor.go b/pkg/parser/import_processor.go index 723dfb5d51..8d94e8fb29 100644 --- a/pkg/parser/import_processor.go +++ b/pkg/parser/import_processor.go @@ -632,25 +632,25 @@ func processImportsFromFrontmatterWithManifestAndSource(frontmatter map[string]a } // Extract engines from imported file - engineContent, err := extractEngineFromContent(string(content)) + engineContent, err := extractFrontmatterField(string(content), "engine", "") if err == nil && engineContent != "" { engines = append(engines, engineContent) } // Extract mcp-servers from imported file - mcpServersContent, err := extractMCPServersFromContent(string(content)) + mcpServersContent, err := extractFrontmatterField(string(content), "mcp-servers", "{}") if err == nil && mcpServersContent != "" && mcpServersContent != "{}" { mcpServersBuilder.WriteString(mcpServersContent + "\n") } // Extract safe-outputs from imported file - safeOutputsContent, err := extractSafeOutputsFromContent(string(content)) + safeOutputsContent, err := extractFrontmatterField(string(content), "safe-outputs", "{}") if err == nil && safeOutputsContent != "" && safeOutputsContent != "{}" { safeOutputs = append(safeOutputs, safeOutputsContent) } // Extract safe-inputs from imported file - safeInputsContent, err := extractSafeInputsFromContent(string(content)) + safeInputsContent, err := extractFrontmatterField(string(content), "safe-inputs", "{}") if err == nil && safeInputsContent != "" && safeInputsContent != "{}" { safeInputs = append(safeInputs, safeInputsContent) } @@ -662,7 +662,7 @@ func processImportsFromFrontmatterWithManifestAndSource(frontmatter map[string]a } // Extract runtimes from imported file - runtimesContent, err := extractRuntimesFromContent(string(content)) + runtimesContent, err := extractFrontmatterField(string(content), "runtimes", "{}") if err == nil && runtimesContent != "" && runtimesContent != "{}" { runtimesBuilder.WriteString(runtimesContent + "\n") } @@ -674,7 +674,7 @@ func processImportsFromFrontmatterWithManifestAndSource(frontmatter map[string]a } // Extract network from imported file - networkContent, err := extractNetworkFromContent(string(content)) + networkContent, err := extractFrontmatterField(string(content), "network", "{}") if err == nil && networkContent != "" && networkContent != "{}" { networkBuilder.WriteString(networkContent + "\n") } @@ -686,13 +686,13 @@ func processImportsFromFrontmatterWithManifestAndSource(frontmatter map[string]a } // Extract secret-masking from imported file - secretMaskingContent, err := extractSecretMaskingFromContent(string(content)) + secretMaskingContent, err := extractFrontmatterField(string(content), "secret-masking", "{}") if err == nil && secretMaskingContent != "" && secretMaskingContent != "{}" { secretMaskingBuilder.WriteString(secretMaskingContent + "\n") } // Extract and merge bots from imported file (merge into set to avoid duplicates) - botsContent, err := extractBotsFromContent(string(content)) + botsContent, err := extractFrontmatterField(string(content), "bots", "[]") if err == nil && botsContent != "" && botsContent != "[]" { // Parse bots JSON array var importedBots []string @@ -707,7 +707,7 @@ func processImportsFromFrontmatterWithManifestAndSource(frontmatter map[string]a } // Extract and merge skip-roles from imported file (merge into set to avoid duplicates) - skipRolesContent, err := extractSkipRolesFromContent(string(content)) + skipRolesContent, err := extractOnSectionField(string(content), "skip-roles") if err == nil && skipRolesContent != "" && skipRolesContent != "[]" { // Parse skip-roles JSON array var importedSkipRoles []string @@ -722,7 +722,7 @@ func processImportsFromFrontmatterWithManifestAndSource(frontmatter map[string]a } // Extract and merge skip-bots from imported file (merge into set to avoid duplicates) - skipBotsContent, err := extractSkipBotsFromContent(string(content)) + skipBotsContent, err := extractOnSectionField(string(content), "skip-bots") if err == nil && skipBotsContent != "" && skipBotsContent != "[]" { // Parse skip-bots JSON array var importedSkipBots []string @@ -738,7 +738,7 @@ func processImportsFromFrontmatterWithManifestAndSource(frontmatter map[string]a // Extract and merge plugins from imported file (merge into set to avoid duplicates) // This now handles both simple string format and object format with MCP configs - pluginsContent, err := extractPluginsFromContent(string(content)) + pluginsContent, err := extractFrontmatterField(string(content), "plugins", "[]") if err == nil && pluginsContent != "" && pluginsContent != "[]" { // Parse plugins - can be array of strings or objects var pluginsRaw []any @@ -772,7 +772,7 @@ func processImportsFromFrontmatterWithManifestAndSource(frontmatter map[string]a } // Extract labels from imported file (merge into set to avoid duplicates) - labelsContent, err := extractLabelsFromContent(string(content)) + labelsContent, err := extractFrontmatterField(string(content), "labels", "[]") if err == nil && labelsContent != "" && labelsContent != "[]" { // Parse labels JSON array var importedLabels []string @@ -787,13 +787,13 @@ func processImportsFromFrontmatterWithManifestAndSource(frontmatter map[string]a } // Extract cache from imported file (append to list of caches) - cacheContent, err := extractCacheFromContent(string(content)) + cacheContent, err := extractFrontmatterField(string(content), "cache", "{}") if err == nil && cacheContent != "" && cacheContent != "{}" { caches = append(caches, cacheContent) } // Extract features from imported file (parse as map structure) - featuresContent, err := extractFeaturesFromContent(string(content)) + featuresContent, err := extractFrontmatterField(string(content), "features", "{}") if err == nil && featuresContent != "" && featuresContent != "{}" { // Parse JSON to map structure var featuresMap map[string]any diff --git a/pkg/parser/include_expander.go b/pkg/parser/include_expander.go index 4fbc4286be..b5455cac9e 100644 --- a/pkg/parser/include_expander.go +++ b/pkg/parser/include_expander.go @@ -77,13 +77,17 @@ func ExpandIncludesWithManifest(content, baseDir string, extractTools bool) (str // ExpandIncludesForEngines recursively expands @include and @import directives to extract engine configurations func ExpandIncludesForEngines(content, baseDir string) ([]string, error) { log.Printf("Expanding includes for engines: baseDir=%s", baseDir) - return expandIncludesForField(content, baseDir, extractEngineFromContent, "") + return expandIncludesForField(content, baseDir, func(c string) (string, error) { + return extractFrontmatterField(c, "engine", "") + }, "") } // ExpandIncludesForSafeOutputs recursively expands @include and @import directives to extract safe-outputs configurations func ExpandIncludesForSafeOutputs(content, baseDir string) ([]string, error) { log.Printf("Expanding includes for safe-outputs: baseDir=%s", baseDir) - return expandIncludesForField(content, baseDir, extractSafeOutputsFromContent, "{}") + return expandIncludesForField(content, baseDir, func(c string) (string, error) { + return extractFrontmatterField(c, "safe-outputs", "{}") + }, "{}") } // expandIncludesForField recursively expands includes to extract a specific frontmatter field @@ -117,12 +121,16 @@ 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, extractEngineFromContent, "") + 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, extractSafeOutputsFromContent, "{}") + return processIncludesForField(content, baseDir, func(c string) (string, error) { + return extractFrontmatterField(c, "safe-outputs", "{}") + }, "{}") } // processIncludesForField processes import directives to extract a specific frontmatter field diff --git a/pkg/workflow/safe_outputs_env.go b/pkg/workflow/safe_outputs_env.go index 54d4e52ad0..4593227f97 100644 --- a/pkg/workflow/safe_outputs_env.go +++ b/pkg/workflow/safe_outputs_env.go @@ -4,7 +4,6 @@ import ( "fmt" "strconv" "strings" - "testing" "github.com/github/gh-aw/pkg/logger" ) @@ -339,19 +338,3 @@ func buildAllowedReposEnvVar(envVarName string, allowedRepos []string) []string reposStr := strings.Join(allowedRepos, ",") return []string{fmt.Sprintf(" %s: %q\n", envVarName, reposStr)} } - -// ======================================== -// Test Helpers -// ======================================== - -// assertEnvVarsInSteps checks that all expected environment variables are present in the job steps. -// This is a helper function to reduce duplication in safe outputs env tests. -func assertEnvVarsInSteps(t *testing.T, steps []string, expectedEnvVars []string) { - t.Helper() - stepsStr := strings.Join(steps, "") - for _, expectedEnvVar := range expectedEnvVars { - if !strings.Contains(stepsStr, expectedEnvVar) { - t.Errorf("Expected env var %q not found in job YAML", expectedEnvVar) - } - } -} diff --git a/pkg/workflow/safe_outputs_env_helpers_test.go b/pkg/workflow/safe_outputs_env_helpers_test.go new file mode 100644 index 0000000000..88da20197a --- /dev/null +++ b/pkg/workflow/safe_outputs_env_helpers_test.go @@ -0,0 +1,20 @@ +//go:build integration || !integration + +package workflow + +import ( + "strings" + "testing" +) + +// assertEnvVarsInSteps checks that all expected environment variables are present in the job steps. +// This is a helper function to reduce duplication in safe outputs env tests. +func assertEnvVarsInSteps(t *testing.T, steps []string, expectedEnvVars []string) { + t.Helper() + stepsStr := strings.Join(steps, "") + for _, expectedEnvVar := range expectedEnvVars { + if !strings.Contains(stepsStr, expectedEnvVar) { + t.Errorf("Expected env var %q not found in job YAML", expectedEnvVar) + } + } +}