From 9e3324603c5723d2da2280b63120c390352d9f77 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Sat, 11 Apr 2026 12:05:26 +0000 Subject: [PATCH] =?UTF-8?q?chore:=20remove=20dead=20functions=20=E2=80=94?= =?UTF-8?q?=204=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: - ReadFileFromHEAD (pkg/gitutil/gitutil.go) - normalizeContainerPins + containerPinRE (pkg/workflow/compiler.go) - ResolveAgentFilePath (pkg/workflow/engine_helpers.go) - BuildInvalidAgentPathStep (pkg/workflow/engine_helpers.go) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- pkg/gitutil/gitutil.go | 21 ---- pkg/gitutil/gitutil_test.go | 30 ------ pkg/workflow/compiler.go | 11 --- pkg/workflow/engine_helpers.go | 52 ---------- pkg/workflow/engine_helpers_test.go | 148 ---------------------------- pkg/workflow/wasm_golden_test.go | 6 +- 6 files changed, 5 insertions(+), 263 deletions(-) diff --git a/pkg/gitutil/gitutil.go b/pkg/gitutil/gitutil.go index d1400b83cb6..a6d38845bf5 100644 --- a/pkg/gitutil/gitutil.go +++ b/pkg/gitutil/gitutil.go @@ -81,27 +81,6 @@ func FindGitRoot() (string, error) { return gitRoot, nil } -// ReadFileFromHEAD returns the content of filePath as recorded in the most recent -// git commit (HEAD). This is used in safe update mode so that the manifest baseline -// comes from the committed version of the lock file, not from the working-tree copy, -// preventing a local agent from tampering with the file to bypass enforcement. -// -// filePath may be absolute or relative; it is resolved to an absolute path before -// computing its path relative to the git root. -// -// Returns an error when: -// - the current directory is not inside a git repository -// - the file does not exist in HEAD (e.g. it has never been committed) -// - the git command fails for another reason -func ReadFileFromHEAD(filePath string) (string, error) { - gitRoot, err := FindGitRoot() - if err != nil { - return "", fmt.Errorf("cannot read %q from git HEAD: %w", filePath, err) - } - - return ReadFileFromHEADWithRoot(filePath, gitRoot) -} - // ReadFileFromHEADWithRoot is like ReadFileFromHEAD but accepts a pre-computed git // repository root, avoiding the subprocess overhead of calling FindGitRoot(). // Use this when the caller already knows the git root (e.g. from a cached value). diff --git a/pkg/gitutil/gitutil_test.go b/pkg/gitutil/gitutil_test.go index b1573682ca9..e0006053005 100644 --- a/pkg/gitutil/gitutil_test.go +++ b/pkg/gitutil/gitutil_test.go @@ -252,24 +252,6 @@ func TestFindGitRoot(t *testing.T) { }) } -func TestReadFileFromHEAD(t *testing.T) { - t.Run("reads a committed file successfully", func(t *testing.T) { - // go.mod is always committed at the repo root, so it should be readable from HEAD. - gitRoot, err := FindGitRoot() - require.NoError(t, err, "must be inside a git repository") - - content, err := ReadFileFromHEAD(filepath.Join(gitRoot, "go.mod")) - require.NoError(t, err, "go.mod should be readable from HEAD") - assert.NotEmpty(t, content, "go.mod content should not be empty") - assert.Contains(t, content, "module ", "go.mod should contain a module declaration") - }) - - t.Run("returns error for file not in HEAD", func(t *testing.T) { - _, err := ReadFileFromHEAD("/nonexistent/path/that/is/not/in/git/repo/file.yml") - assert.Error(t, err, "should fail for a file not tracked by git") - }) -} - func TestReadFileFromHEADWithRoot(t *testing.T) { t.Run("reads a committed file with pre-computed root", func(t *testing.T) { gitRoot, err := FindGitRoot() @@ -291,18 +273,6 @@ func TestReadFileFromHEADWithRoot(t *testing.T) { assert.Contains(t, err.Error(), "outside the git repository root", "error should mention path is outside repo") }) - t.Run("returns same result as ReadFileFromHEAD", func(t *testing.T) { - gitRoot, err := FindGitRoot() - require.NoError(t, err, "must be inside a git repository") - - goModPath := filepath.Join(gitRoot, "go.mod") - content1, err1 := ReadFileFromHEAD(goModPath) - content2, err2 := ReadFileFromHEADWithRoot(goModPath, gitRoot) - - assert.Equal(t, err1, err2, "both functions should return the same error") - assert.Equal(t, content1, content2, "both functions should return the same content") - }) - t.Run("returns error for empty gitRoot", func(t *testing.T) { _, err := ReadFileFromHEADWithRoot("some/file.yml", "") require.Error(t, err, "should fail when gitRoot is empty") diff --git a/pkg/workflow/compiler.go b/pkg/workflow/compiler.go index bad7631137a..fc6f3414b33 100644 --- a/pkg/workflow/compiler.go +++ b/pkg/workflow/compiler.go @@ -35,17 +35,6 @@ func normalizeHeredocDelimiters(content string) string { return heredocDelimiterRE.ReplaceAllString(content, "GH_AW_${1}_NORM_EOF") } -// containerPinRE matches Docker image digest pins of the form @sha256:<64 hex chars>. -// Used to normalize output that may or may not include container pins depending on -// whether the action cache is available (native compilation has it, wasm does not). -var containerPinRE = regexp.MustCompile(`@sha256:[0-9a-f]{64}`) - -// normalizeContainerPins strips @sha256:… digest suffixes from Docker image references -// so that compiled output compares equal regardless of whether the action cache was loaded. -func normalizeContainerPins(content string) string { - return containerPinRE.ReplaceAllString(content, "") -} - const ( // MaxLockFileSize is the maximum allowed size for generated lock workflow files (500KB) MaxLockFileSize = 512000 // 500KB in bytes diff --git a/pkg/workflow/engine_helpers.go b/pkg/workflow/engine_helpers.go index ea60fe8ccfe..65a6e426e7b 100644 --- a/pkg/workflow/engine_helpers.go +++ b/pkg/workflow/engine_helpers.go @@ -114,35 +114,6 @@ func GetBaseInstallationSteps(config EngineInstallConfig, workflowData *Workflow return steps } -// ResolveAgentFilePath validates and returns the properly quoted agent file path with GITHUB_WORKSPACE prefix. -// This helper extracts the common pattern shared by Copilot, Codex, and Claude engines. -// -// The agent file path is relative to the repository root, so we prefix it with ${GITHUB_WORKSPACE} -// and wrap the entire expression in double quotes to handle paths with spaces while allowing -// shell variable expansion. -// -// Parameters: -// - agentFile: The relative path to the agent file (e.g., ".github/agents/test-agent.md") -// -// Returns: -// - string: The double-quoted path with GITHUB_WORKSPACE prefix (e.g., "${GITHUB_WORKSPACE}/.github/agents/test-agent.md") -// - error: Non-nil if agentFile contains characters that could cause shell injection -// -// The path is validated against an allowlist of safe characters before returning. Only -// alphanumeric characters, dots, underscores, hyphens, forward slashes, and spaces are -// permitted. Shell metacharacters (", $, `, ;, |, \, etc.) are rejected to prevent injection. -// -// Example: -// -// agentPath, err := ResolveAgentFilePath(".github/agents/my-agent.md") -// // Returns: "${GITHUB_WORKSPACE}/.github/agents/my-agent.md", nil -func ResolveAgentFilePath(agentFile string) (string, error) { - if !agentFilePathRegex.MatchString(agentFile) { - return "", fmt.Errorf("agent file path contains invalid characters: %q (only alphanumeric characters, dots, underscores, hyphens, forward slashes, and spaces are allowed)", agentFile) - } - return fmt.Sprintf("\"${GITHUB_WORKSPACE}/%s\"", agentFile), nil -} - // BuildStandardNpmEngineInstallSteps creates standard npm installation steps for engines // This helper extracts the common pattern shared by Copilot, Codex, and Claude engines. // @@ -477,26 +448,3 @@ func EngineHasValidateSecretStep(engine CodingAgentEngine, data *WorkflowData) b step := engine.GetSecretValidationStep(data) return len(step) > 0 } - -// BuildInvalidAgentPathStep returns a single-step slice that immediately exits non-zero -// with a human-readable error message. Use this when ResolveAgentFilePath rejects an -// agent file path at step-generation time so that the compiled workflow fails clearly -// rather than silently skipping the engine execution step. -// -// Parameters: -// - stepName: The step name to display (e.g. "Execute Codex CLI") -// - agentFile: The invalid agent file path provided by the user -// - err: The validation error returned by ResolveAgentFilePath -// -// Returns: -// - []GitHubActionStep: A slice containing a single failing step -func BuildInvalidAgentPathStep(stepName, agentFile string, err error) []GitHubActionStep { - errMsg := fmt.Sprintf("Invalid agent file path %q: %v", agentFile, err) - engineHelpersLog.Printf("Building invalid agent path step: %s", errMsg) - var stepLines []string - stepLines = append(stepLines, " - name: "+stepName) - stepLines = append(stepLines, " run: |") - stepLines = append(stepLines, fmt.Sprintf(" echo 'Error: %s' >&2", strings.ReplaceAll(errMsg, "'", "'\\''"))) - stepLines = append(stepLines, " exit 1") - return []GitHubActionStep{GitHubActionStep(stepLines)} -} diff --git a/pkg/workflow/engine_helpers_test.go b/pkg/workflow/engine_helpers_test.go index 25c5446835c..e72f73ebae7 100644 --- a/pkg/workflow/engine_helpers_test.go +++ b/pkg/workflow/engine_helpers_test.go @@ -146,154 +146,6 @@ func TestBuildStandardNpmEngineInstallSteps_AllEngines(t *testing.T) { } } -// TestResolveAgentFilePath tests the shared agent file path resolution helper -func TestResolveAgentFilePath(t *testing.T) { - tests := []struct { - name string - input string - expected string - expectError bool - }{ - { - name: "basic agent file path", - input: ".github/agents/test-agent.md", - expected: "\"${GITHUB_WORKSPACE}/.github/agents/test-agent.md\"", - }, - { - name: "path with spaces", - input: ".github/agents/my agent file.md", - expected: "\"${GITHUB_WORKSPACE}/.github/agents/my agent file.md\"", - }, - { - name: "deeply nested path", - input: ".github/copilot/instructions/deep/nested/agent.md", - expected: "\"${GITHUB_WORKSPACE}/.github/copilot/instructions/deep/nested/agent.md\"", - }, - { - name: "simple filename", - input: "agent.md", - expected: "\"${GITHUB_WORKSPACE}/agent.md\"", - }, - { - name: "path with special characters", - input: ".github/agents/test-agent_v2.0.md", - expected: "\"${GITHUB_WORKSPACE}/.github/agents/test-agent_v2.0.md\"", - }, - { - name: "path with double quote is rejected", - input: `.github/agents/a";id;"b.md`, - expectError: true, - }, - { - name: "path with dollar sign is rejected", - input: ".github/agents/$injection.md", - expectError: true, - }, - { - name: "path with backtick is rejected", - input: ".github/agents/`id`.md", - expectError: true, - }, - { - name: "path with semicolon is rejected", - input: ".github/agents/a;b.md", - expectError: true, - }, - { - name: "path with pipe is rejected", - input: ".github/agents/a|b.md", - expectError: true, - }, - { - name: "path with newline is rejected", - input: ".github/agents/a\nb.md", - expectError: true, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - result, err := ResolveAgentFilePath(tt.input) - if tt.expectError { - if err == nil { - t.Errorf("ResolveAgentFilePath(%q) expected error but got none (result: %q)", tt.input, result) - } - return - } - if err != nil { - t.Errorf("ResolveAgentFilePath(%q) unexpected error: %v", tt.input, err) - return - } - if result != tt.expected { - t.Errorf("ResolveAgentFilePath(%q) = %q, want %q", tt.input, result, tt.expected) - } - }) - } -} - -// TestResolveAgentFilePathFormat tests that the output format is consistent -func TestResolveAgentFilePathFormat(t *testing.T) { - input := ".github/agents/test.md" - result, err := ResolveAgentFilePath(input) - if err != nil { - t.Fatalf("ResolveAgentFilePath(%q) unexpected error: %v", input, err) - } - - // Verify it starts with opening quote, GITHUB_WORKSPACE variable, and forward slash - expectedPrefix := "\"${GITHUB_WORKSPACE}/" - if !strings.HasPrefix(result, expectedPrefix) { - t.Errorf("Expected path to start with %q, got: %s", expectedPrefix, result) - } - - // Verify it ends with the input path and a closing quote - expectedSuffix := input + "\"" - if !strings.HasSuffix(result, expectedSuffix) { - t.Errorf("Expected path to end with %q, got: %q", expectedSuffix, result) - } - - // Verify the complete expected format - expected := "\"${GITHUB_WORKSPACE}/" + input + "\"" - if result != expected { - t.Errorf("Expected %q, got: %q", expected, result) - } -} - -// TestShellVariableExpansionInAgentPath tests that agent paths allow shell variable expansion -func TestShellVariableExpansionInAgentPath(t *testing.T) { - agentFile := ".github/agents/test-agent.md" - result, err := ResolveAgentFilePath(agentFile) - if err != nil { - t.Fatalf("ResolveAgentFilePath(%q) unexpected error: %v", agentFile, err) - } - - // The result should be fully wrapped in double quotes (not single quotes) - // Format: "${GITHUB_WORKSPACE}/.github/agents/test-agent.md" - expected := "\"${GITHUB_WORKSPACE}/.github/agents/test-agent.md\"" - - if result != expected { - t.Errorf("ResolveAgentFilePath(%q) = %q, want %q", agentFile, result, expected) - } - - // Verify it's properly quoted for shell variable expansion - // Should start with double quote (not single quote) - if !strings.HasPrefix(result, "\"") { - t.Errorf("Agent path should start with double quote for variable expansion, got: %s", result) - } - - // Should end with double quote (not single quote) - if !strings.HasSuffix(result, "\"") { - t.Errorf("Agent path should end with double quote for variable expansion, got: %s", result) - } - - // Should NOT contain single quotes around the double-quoted section - // Old broken format was: '"${GITHUB_WORKSPACE}"/.github/agents/test.md' - if strings.Contains(result, "'\"") || strings.Contains(result, "\"'") { - t.Errorf("Agent path should not mix single and double quotes, got: %s", result) - } -} - -// TestShellEscapeArgWithFullyQuotedAgentPath tests that fully quoted agent paths ARE now escaped -// (the pre-quoted bypass has been removed to prevent shell injection attacks). func TestShellEscapeArgWithFullyQuotedAgentPath(t *testing.T) { // After the bypass removal, a double-quoted string is treated as any other argument // containing special characters and gets properly single-quoted. diff --git a/pkg/workflow/wasm_golden_test.go b/pkg/workflow/wasm_golden_test.go index e8facd29e6f..55543b7f092 100644 --- a/pkg/workflow/wasm_golden_test.go +++ b/pkg/workflow/wasm_golden_test.go @@ -6,6 +6,7 @@ import ( "fmt" "os" "path/filepath" + "regexp" "strings" "testing" @@ -13,11 +14,14 @@ import ( "github.com/stretchr/testify/require" ) +// containerPinRE matches Docker image digest pins of the form @sha256:<64 hex chars>. +var testContainerPinRE = regexp.MustCompile(`@sha256:[0-9a-f]{64}`) + // normalizeOutput applies all stable-comparison normalizations to compiled workflow output // before golden comparison: heredoc delimiter normalization and container pin normalization. // Mirrors normalize() in scripts/test-wasm-golden.mjs. func normalizeOutput(content string) string { - return normalizeContainerPins(normalizeHeredocDelimiters(content)) + return testContainerPinRE.ReplaceAllString(normalizeHeredocDelimiters(content), "") } // TestWasmGolden_CompileFixtures compiles each workflow fixture using the string API