Skip to content

[dead-code] chore: remove dead functions — 4 functions removed #25798

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
pelikhan merged 1 commit into from
Apr 11, 2026
Merged

[dead-code] chore: remove dead functions — 4 functions removed #25798

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 0 additions & 21 deletions pkg/gitutil/gitutil.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Copy Markdown
Contributor Author

@github-actions github-actions Bot Apr 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Good cleanup — ReadFileFromHEAD was a thin wrapper around ReadFileFromHEADWithRoot. Removing the wrapper reduces the public API surface and makes callers use the more explicit version directly. This is a clean dead-code removal.

// 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).
Expand Down
30 changes: 0 additions & 30 deletions pkg/gitutil/gitutil_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand All @@ -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")
Expand Down
11 changes: 0 additions & 11 deletions pkg/workflow/compiler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,17 +35,6 @@ func normalizeHeredocDelimiters(content string) string {
return heredocDelimiterRE.ReplaceAllString(content, "GH_AW_${1}_NORM_EOF")
}
Copy link
Copy Markdown
Contributor Author

@github-actions github-actions Bot Apr 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Removing containerPinRE and normalizeContainerPins here makes sense if they're no longer used. Worth confirming no test helpers or other callers reference these functions before finalizing the PR.


// 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
Expand Down
52 changes: 0 additions & 52 deletions pkg/workflow/engine_helpers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
//
Expand Down Expand Up @@ -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)}
}
148 changes: 0 additions & 148 deletions pkg/workflow/engine_helpers_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
6 changes: 5 additions & 1 deletion pkg/workflow/wasm_golden_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,18 +6,22 @@ import (
"fmt"
"os"
"path/filepath"
"regexp"
"strings"
"testing"

"github.com/charmbracelet/x/exp/golden"
"github.com/stretchr/testify/require"
)

// containerPinRE matches Docker image digest pins of the form @sha256:<64 hex chars>.
Copy link

Copilot AI Apr 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment refers to containerPinRE, but the actual regex variable here is testContainerPinRE. This can be confusing when grepping or when reading the test; consider updating the comment to match the identifier (or rename the variable/comment consistently).

Suggested change
// containerPinRE matches Docker image digest pins of the form @sha256:<64 hex chars>.
// testContainerPinRE matches Docker image digest pins of the form @sha256:<64 hex chars>.

Copilot uses AI. Check for mistakes.
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
Expand Down
Loading