From 8859d5a9a83d1555b2bea34201512b0e6d2d1eec Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" Date: Fri, 13 Mar 2026 18:18:41 +0000 Subject: [PATCH] =?UTF-8?q?chore:=20remove=20dead=20functions=20=E2=80=94?= =?UTF-8?q?=205=20functions=20removed?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Remove unreachable Go functions identified by the deadcode static analyzer. Functions removed: - WorkflowStep.ToYAML (pkg/workflow/step_types.go) - Compiler.formatDetectionRunsOn (pkg/workflow/safe_outputs_runtime.go) - getEnabledSafeOutputToolNamesReflection (pkg/workflow/safe_outputs_state.go) - GetEnabledSafeOutputToolNames (pkg/workflow/safe_outputs_state.go) - applySafeOutputEnvToSlice (pkg/workflow/safe_outputs_jobs.go) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- pkg/workflow/safe_output_helpers_test.go | 102 -------------------- pkg/workflow/safe_outputs_jobs.go | 25 ----- pkg/workflow/safe_outputs_runtime.go | 10 -- pkg/workflow/safe_outputs_state.go | 50 ---------- pkg/workflow/safe_outputs_test.go | 115 ----------------------- pkg/workflow/step_types.go | 14 --- pkg/workflow/step_types_test.go | 101 -------------------- pkg/workflow/threat_detection_test.go | 64 ------------- 8 files changed, 481 deletions(-) diff --git a/pkg/workflow/safe_output_helpers_test.go b/pkg/workflow/safe_output_helpers_test.go index 7861a85cd24..3d542d5f76d 100644 --- a/pkg/workflow/safe_output_helpers_test.go +++ b/pkg/workflow/safe_output_helpers_test.go @@ -295,95 +295,6 @@ func TestApplySafeOutputEnvToMap(t *testing.T) { } // TestApplySafeOutputEnvToSlice verifies the helper function for YAML string slices -func TestApplySafeOutputEnvToSlice(t *testing.T) { - tests := []struct { - name string - workflowData *WorkflowData - expected []string - }{ - { - name: "nil SafeOutputs", - workflowData: &WorkflowData{ - SafeOutputs: nil, - }, - expected: []string{}, - }, - { - name: "basic safe outputs", - workflowData: &WorkflowData{ - SafeOutputs: &SafeOutputsConfig{}, - }, - expected: []string{ - " GH_AW_SAFE_OUTPUTS: ${{ env.GH_AW_SAFE_OUTPUTS }}", - }, - }, - { - name: "safe outputs with staged flag", - workflowData: &WorkflowData{ - SafeOutputs: &SafeOutputsConfig{ - Staged: true, - }, - }, - expected: []string{ - " GH_AW_SAFE_OUTPUTS: ${{ env.GH_AW_SAFE_OUTPUTS }}", - " GH_AW_SAFE_OUTPUTS_STAGED: \"true\"", - }, - }, - { - name: "trial mode", - workflowData: &WorkflowData{ - TrialMode: true, - TrialLogicalRepo: "owner/repo", - SafeOutputs: &SafeOutputsConfig{}, - }, - expected: []string{ - " GH_AW_SAFE_OUTPUTS: ${{ env.GH_AW_SAFE_OUTPUTS }}", - " GH_AW_SAFE_OUTPUTS_STAGED: \"true\"", - " GH_AW_TARGET_REPO_SLUG: \"owner/repo\"", - }, - }, - { - name: "upload assets config", - workflowData: &WorkflowData{ - SafeOutputs: &SafeOutputsConfig{ - UploadAssets: &UploadAssetsConfig{ - BranchName: "gh-aw-assets", - MaxSizeKB: 10240, - AllowedExts: []string{".png", ".jpg", ".jpeg"}, - }, - }, - }, - expected: []string{ - " GH_AW_SAFE_OUTPUTS: ${{ env.GH_AW_SAFE_OUTPUTS }}", - " GH_AW_ASSETS_BRANCH: \"gh-aw-assets\"", - " GH_AW_ASSETS_MAX_SIZE_KB: 10240", - " GH_AW_ASSETS_ALLOWED_EXTS: \".png,.jpg,.jpeg\"", - }, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - var stepLines []string - applySafeOutputEnvToSlice(&stepLines, tt.workflowData) - - if len(stepLines) != len(tt.expected) { - t.Errorf("Expected %d lines, got %d", len(tt.expected), len(stepLines)) - } - - for i, expectedLine := range tt.expected { - if i >= len(stepLines) { - t.Errorf("Missing expected line %d: %q", i, expectedLine) - continue - } - if stepLines[i] != expectedLine { - t.Errorf("Line %d: expected %q, got %q", i, expectedLine, stepLines[i]) - } - } - }) - } -} - // TestBuildWorkflowMetadataEnvVars verifies the helper function for workflow metadata env vars func TestBuildWorkflowMetadataEnvVars(t *testing.T) { tests := []struct { @@ -663,11 +574,6 @@ func TestEnginesUseSameHelperLogic(t *testing.T) { envMap := make(map[string]string) applySafeOutputEnvToMap(envMap, workflowData) - // Test slice-based helper (used by claude) - var stepLines []string - applySafeOutputEnvToSlice(&stepLines, workflowData) - - // Verify both approaches produce the same env vars expectedKeys := []string{ "GH_AW_SAFE_OUTPUTS", "GH_AW_SAFE_OUTPUTS_STAGED", @@ -683,14 +589,6 @@ func TestEnginesUseSameHelperLogic(t *testing.T) { t.Errorf("envMap missing expected key: %s", key) } } - - // Check slice (should contain all keys) - sliceContent := strings.Join(stepLines, "\n") - for _, key := range expectedKeys { - if !strings.Contains(sliceContent, key) { - t.Errorf("stepLines missing expected key: %s", key) - } - } } // TestBuildAgentOutputDownloadSteps verifies the agent output download steps diff --git a/pkg/workflow/safe_outputs_jobs.go b/pkg/workflow/safe_outputs_jobs.go index e1c8948ee3d..9c2e29938c9 100644 --- a/pkg/workflow/safe_outputs_jobs.go +++ b/pkg/workflow/safe_outputs_jobs.go @@ -436,31 +436,6 @@ func applySafeOutputEnvToMap(env map[string]string, data *WorkflowData) { } } -// applySafeOutputEnvToSlice adds safe-output related environment variables to a YAML string slice -// This is for engines that build YAML line-by-line (like Claude) -func applySafeOutputEnvToSlice(stepLines *[]string, workflowData *WorkflowData) { - if workflowData.SafeOutputs == nil { - return - } - - *stepLines = append(*stepLines, " GH_AW_SAFE_OUTPUTS: ${{ env.GH_AW_SAFE_OUTPUTS }}") - - // Add staged flag if specified - if workflowData.TrialMode || workflowData.SafeOutputs.Staged { - *stepLines = append(*stepLines, " GH_AW_SAFE_OUTPUTS_STAGED: \"true\"") - } - if workflowData.TrialMode && workflowData.TrialLogicalRepo != "" { - *stepLines = append(*stepLines, fmt.Sprintf(" GH_AW_TARGET_REPO_SLUG: %q", workflowData.TrialLogicalRepo)) - } - - // Add branch name if upload assets is configured - if workflowData.SafeOutputs.UploadAssets != nil { - *stepLines = append(*stepLines, fmt.Sprintf(" GH_AW_ASSETS_BRANCH: %q", workflowData.SafeOutputs.UploadAssets.BranchName)) - *stepLines = append(*stepLines, fmt.Sprintf(" GH_AW_ASSETS_MAX_SIZE_KB: %d", workflowData.SafeOutputs.UploadAssets.MaxSizeKB)) - *stepLines = append(*stepLines, fmt.Sprintf(" GH_AW_ASSETS_ALLOWED_EXTS: %q", strings.Join(workflowData.SafeOutputs.UploadAssets.AllowedExts, ","))) - } -} - // buildWorkflowMetadataEnvVars builds workflow name and source environment variables // This extracts the duplicated workflow metadata setup logic from safe-output job builders func buildWorkflowMetadataEnvVars(workflowName string, workflowSource string) []string { diff --git a/pkg/workflow/safe_outputs_runtime.go b/pkg/workflow/safe_outputs_runtime.go index 0a7a9c6e81f..f7a97040ff8 100644 --- a/pkg/workflow/safe_outputs_runtime.go +++ b/pkg/workflow/safe_outputs_runtime.go @@ -20,16 +20,6 @@ func (c *Compiler) formatSafeOutputsRunsOn(safeOutputs *SafeOutputsConfig) strin return "runs-on: " + safeOutputs.RunsOn } -// formatDetectionRunsOn resolves the runner for the detection job using the following priority: -// 1. safe-outputs.detection.runs-on (detection-specific override) -// 2. agentRunsOn (the agent job's runner, passed by the caller) -func (c *Compiler) formatDetectionRunsOn(safeOutputs *SafeOutputsConfig, agentRunsOn string) string { - if safeOutputs != nil && safeOutputs.ThreatDetection != nil && safeOutputs.ThreatDetection.RunsOn != "" { - return "runs-on: " + safeOutputs.ThreatDetection.RunsOn - } - return agentRunsOn -} - // usesPatchesAndCheckouts checks if the workflow uses safe outputs that require // git patches and checkouts (create-pull-request or push-to-pull-request-branch) func usesPatchesAndCheckouts(safeOutputs *SafeOutputsConfig) bool { diff --git a/pkg/workflow/safe_outputs_state.go b/pkg/workflow/safe_outputs_state.go index 2cad868c83b..b605a5068b6 100644 --- a/pkg/workflow/safe_outputs_state.go +++ b/pkg/workflow/safe_outputs_state.go @@ -91,42 +91,6 @@ func hasAnySafeOutputEnabled(safeOutputs *SafeOutputsConfig) bool { return false } -// getEnabledSafeOutputToolNamesReflection uses reflection to get enabled tool names. -// Results are sorted for deterministic compilation output. -// -// NOTE: Reflection is used here to avoid a large switch statement that would need -// updating every time a new safe-output type is added. The cost is acceptable -// because this function is called infrequently (once per compilation). -func getEnabledSafeOutputToolNamesReflection(safeOutputs *SafeOutputsConfig) []string { - if safeOutputs == nil { - return nil - } - - safeOutputReflectionLog.Print("Getting enabled safe output tool names using reflection") - var tools []string - - // Use reflection to check all pointer fields - val := reflect.ValueOf(safeOutputs).Elem() - for fieldName, toolName := range safeOutputFieldMapping { - field := val.FieldByName(fieldName) - if field.IsValid() && !field.IsNil() { - tools = append(tools, toolName) - } - } - - // Add custom job tools - for jobName := range safeOutputs.Jobs { - tools = append(tools, jobName) - safeOutputReflectionLog.Printf("Added custom job tool: %s", jobName) - } - - // Sort tools to ensure deterministic compilation - sort.Strings(tools) - - safeOutputReflectionLog.Printf("Found %d enabled safe output tools", len(tools)) - return tools -} - // builtinSafeOutputFields contains the struct field names for the built-in safe output types // that are excluded from the "non-builtin" check. These are: noop, missing-data, missing-tool. var builtinSafeOutputFields = map[string]bool{ @@ -183,20 +147,6 @@ func HasSafeOutputsEnabled(safeOutputs *SafeOutputsConfig) bool { return enabled } -// GetEnabledSafeOutputToolNames returns a list of enabled safe output tool names. -// NOTE: Tool names should NOT be included in agent prompts. The agent should query -// the MCP server to discover available tools. This function is used for generating -// the tools.json file that the MCP server provides, and for diagnostic logging. -func GetEnabledSafeOutputToolNames(safeOutputs *SafeOutputsConfig) []string { - tools := getEnabledSafeOutputToolNamesReflection(safeOutputs) - - if safeOutputsConfigLog.Enabled() { - safeOutputsConfigLog.Printf("Enabled safe output tools: %v", tools) - } - - return tools -} - // checkAllEnabledToolsPresent verifies that every tool in enabledTools has a matching entry // in filteredTools. This is a compiler error check: if a safe-output type is registered in // Go code but its definition is missing from safe-output-tools.json, it will not appear in diff --git a/pkg/workflow/safe_outputs_test.go b/pkg/workflow/safe_outputs_test.go index d51eb4b0c0c..862a02671f3 100644 --- a/pkg/workflow/safe_outputs_test.go +++ b/pkg/workflow/safe_outputs_test.go @@ -1057,118 +1057,3 @@ func TestBuildStandardSafeOutputEnvVars(t *testing.T) { }) } } - -// ======================================== -// GetEnabledSafeOutputToolNames Tests -// ======================================== - -// TestGetEnabledSafeOutputToolNames tests that tool names are returned in sorted order -func TestGetEnabledSafeOutputToolNames(t *testing.T) { - tests := []struct { - name string - safeOutputs *SafeOutputsConfig - expected []string - }{ - { - name: "nil safe outputs returns nil", - safeOutputs: nil, - expected: nil, - }, - { - name: "empty safe outputs returns empty slice", - safeOutputs: &SafeOutputsConfig{}, - expected: nil, - }, - { - name: "single tool", - safeOutputs: &SafeOutputsConfig{ - CreateIssues: &CreateIssuesConfig{}, - }, - expected: []string{"create_issue"}, - }, - { - name: "multiple tools in alphabetical order", - safeOutputs: &SafeOutputsConfig{ - AddComments: &AddCommentsConfig{}, - CreateIssues: &CreateIssuesConfig{}, - UpdateIssues: &UpdateIssuesConfig{}, - }, - expected: []string{"add_comment", "create_issue", "update_issue"}, - }, - { - name: "custom jobs are sorted with standard tools", - safeOutputs: &SafeOutputsConfig{ - CreateIssues: &CreateIssuesConfig{}, - UpdateIssues: &UpdateIssuesConfig{}, - Jobs: map[string]*SafeJobConfig{ - "zzz_custom": {}, - "aaa_custom": {}, - "middle_custom": {}, - }, - }, - expected: []string{"aaa_custom", "create_issue", "middle_custom", "update_issue", "zzz_custom"}, - }, - { - name: "all standard tools are sorted", - safeOutputs: &SafeOutputsConfig{ - CreateIssues: &CreateIssuesConfig{}, - CreateAgentSessions: &CreateAgentSessionConfig{}, - CreateDiscussions: &CreateDiscussionsConfig{}, - CloseDiscussions: &CloseDiscussionsConfig{}, - CloseIssues: &CloseIssuesConfig{}, - ClosePullRequests: &ClosePullRequestsConfig{}, - AddComments: &AddCommentsConfig{}, - CreatePullRequests: &CreatePullRequestsConfig{}, - AddLabels: &AddLabelsConfig{}, - AddReviewer: &AddReviewerConfig{}, - AssignMilestone: &AssignMilestoneConfig{}, - UpdateIssues: &UpdateIssuesConfig{}, - UpdatePullRequests: &UpdatePullRequestsConfig{}, - SubmitPullRequestReview: &SubmitPullRequestReviewConfig{}, - NoOp: &NoOpConfig{}, - }, - // Expected order is alphabetical - expected: []string{ - "add_comment", - "add_labels", - "add_reviewer", - "assign_milestone", - "close_discussion", - "close_issue", - "close_pull_request", - "create_agent_session", - "create_discussion", - "create_issue", - "create_pull_request", - "noop", - "submit_pull_request_review", - "update_issue", - "update_pull_request", - }, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - result := GetEnabledSafeOutputToolNames(tt.safeOutputs) - - if len(result) != len(tt.expected) { - t.Errorf("Expected %d tools, got %d: %v", len(tt.expected), len(result), result) - return - } - - for i, tool := range result { - if tool != tt.expected[i] { - t.Errorf("Tool at index %d: expected %q, got %q", i, tt.expected[i], tool) - } - } - - // Verify the list is sorted - for i := 1; i < len(result); i++ { - if result[i-1] > result[i] { - t.Errorf("Tools not in sorted order: %q comes after %q", result[i-1], result[i]) - } - } - }) - } -} diff --git a/pkg/workflow/step_types.go b/pkg/workflow/step_types.go index fe23b707132..8f43e2c1863 100644 --- a/pkg/workflow/step_types.go +++ b/pkg/workflow/step_types.go @@ -6,7 +6,6 @@ import ( "maps" "github.com/github/gh-aw/pkg/logger" - "github.com/goccy/go-yaml" ) var stepTypesLog = logger.New("workflow:step_types") @@ -162,19 +161,6 @@ func (s *WorkflowStep) Clone() *WorkflowStep { return clone } -// ToYAML converts the WorkflowStep to YAML string -func (s *WorkflowStep) ToYAML() (string, error) { - stepTypesLog.Printf("Converting step to YAML: name=%s", s.Name) - stepMap := s.ToMap() - yamlBytes, err := yaml.Marshal(stepMap) - if err != nil { - stepTypesLog.Printf("Failed to marshal step to YAML: %v", err) - return "", fmt.Errorf("failed to marshal step to YAML: %w", err) - } - stepTypesLog.Printf("Successfully converted step to YAML: size=%d bytes", len(yamlBytes)) - return string(yamlBytes), nil -} - // SliceToSteps converts a slice of any (typically []map[string]any from YAML parsing) // to a typed slice of WorkflowStep pointers for type-safe manipulation func SliceToSteps(steps []any) ([]*WorkflowStep, error) { diff --git a/pkg/workflow/step_types_test.go b/pkg/workflow/step_types_test.go index 3a06d74fe6d..a3b35902247 100644 --- a/pkg/workflow/step_types_test.go +++ b/pkg/workflow/step_types_test.go @@ -280,51 +280,6 @@ func TestWorkflowStep_Clone(t *testing.T) { assert.False(t, exists, "Clone should deep copy Env map - modifying clone should not affect original") } -func TestWorkflowStep_ToYAML(t *testing.T) { - tests := []struct { - name string - step *WorkflowStep - wantErr bool - }{ - { - name: "simple step", - step: &WorkflowStep{ - Name: "Test step", - Uses: "actions/checkout@v4", - }, - wantErr: false, - }, - { - name: "step with complex fields", - step: &WorkflowStep{ - Name: "Complex step", - Uses: "some/action@v1", - With: map[string]any{ - "string-field": "value", - "int-field": 42, - "bool-field": true, - }, - Env: map[string]string{ - "VAR": "value", - }, - }, - wantErr: false, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - got, err := tt.step.ToYAML() - if tt.wantErr { - require.Error(t, err, "ToYAML should return error for %s", tt.name) - return - } - require.NoError(t, err, "ToYAML should succeed for %s", tt.name) - assert.NotEmpty(t, got, "ToYAML should return non-empty string for %s", tt.name) - }) - } -} - func TestMapToStep_RoundTrip(t *testing.T) { // Test that converting map -> step -> map produces the same result // Note: env field converts from map[string]any to map[string]string @@ -766,62 +721,6 @@ func TestClone_DeepNestedMaps(t *testing.T) { assert.Equal(t, "modified", origLevel2["level3"], "Current Clone implementation does shallow copy of nested maps") } -func TestToYAML_ErrorHandling(t *testing.T) { - tests := []struct { - name string - step *WorkflowStep - wantErr bool - description string - }{ - { - name: "step with complex nested maps", - step: &WorkflowStep{ - Name: "Complex step", - Uses: "some/action@v1", - With: map[string]any{ - "nested": map[string]any{ - "deep": map[string]any{ - "value": "test", - }, - }, - }, - }, - wantErr: false, - description: "should handle complex nested structures", - }, - { - name: "step with special characters", - step: &WorkflowStep{ - Name: "Test: Special chars!", - Run: "echo 'hello' && echo \"world\"", - }, - wantErr: false, - description: "should handle special characters in strings", - }, - { - name: "step with multiline run", - step: &WorkflowStep{ - Name: "Multiline", - Run: "line1\nline2\nline3", - }, - wantErr: false, - description: "should handle multiline strings", - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - yaml, err := tt.step.ToYAML() - if tt.wantErr { - require.Error(t, err, "ToYAML should return error for %s", tt.description) - } else { - require.NoError(t, err, "ToYAML should succeed for %s", tt.description) - assert.NotEmpty(t, yaml, "ToYAML should return non-empty YAML for %s", tt.description) - } - }) - } -} - func TestSliceToSteps_MixedValidInvalid(t *testing.T) { tests := []struct { name string diff --git a/pkg/workflow/threat_detection_test.go b/pkg/workflow/threat_detection_test.go index 14e39d45e3a..1bc30954d98 100644 --- a/pkg/workflow/threat_detection_test.go +++ b/pkg/workflow/threat_detection_test.go @@ -153,70 +153,6 @@ func TestParseThreatDetectionConfig(t *testing.T) { } } -func TestFormatDetectionRunsOn(t *testing.T) { - compiler := NewCompiler() - - tests := []struct { - name string - safeOutputs *SafeOutputsConfig - agentRunsOn string - expectedRunsOn string - }{ - { - name: "nil safe outputs uses agent runs-on", - safeOutputs: nil, - agentRunsOn: "runs-on: ubuntu-latest", - expectedRunsOn: "runs-on: ubuntu-latest", - }, - { - name: "detection runs-on takes priority over agent runs-on", - safeOutputs: &SafeOutputsConfig{ - RunsOn: "self-hosted", - ThreatDetection: &ThreatDetectionConfig{ - RunsOn: "detection-runner", - }, - }, - agentRunsOn: "runs-on: ubuntu-latest", - expectedRunsOn: "runs-on: detection-runner", - }, - { - name: "falls back to agent runs-on when detection runs-on is empty", - safeOutputs: &SafeOutputsConfig{ - RunsOn: "self-hosted", - ThreatDetection: &ThreatDetectionConfig{}, - }, - agentRunsOn: "runs-on: my-agent-runner", - expectedRunsOn: "runs-on: my-agent-runner", - }, - { - name: "falls back to agent runs-on when both detection and safe-outputs runs-on are empty", - safeOutputs: &SafeOutputsConfig{ - ThreatDetection: &ThreatDetectionConfig{}, - }, - agentRunsOn: "runs-on: ubuntu-latest", - expectedRunsOn: "runs-on: ubuntu-latest", - }, - { - name: "nil threat detection uses agent runs-on", - safeOutputs: &SafeOutputsConfig{ - RunsOn: "windows-latest", - ThreatDetection: nil, - }, - agentRunsOn: "runs-on: my-agent-runner", - expectedRunsOn: "runs-on: my-agent-runner", - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - result := compiler.formatDetectionRunsOn(tt.safeOutputs, tt.agentRunsOn) - if result != tt.expectedRunsOn { - t.Errorf("Expected runs-on %q, got %q", tt.expectedRunsOn, result) - } - }) - } -} - func TestBuildInlineDetectionSteps(t *testing.T) { compiler := NewCompiler()