[change_lister] Validate inputs and handle rev-parse error (#478)

### Issue
`change_lister` doesn't verify `remote` and `branch` inputs

### Fix
- Add new constructor helper function to handle input validation
- Handle the error from `git rev-parse --is-shallow-repository` instead
of silently discarding it, so environment misconfigurations surface
clearly.

---------

Signed-off-by: Sai Miduthuri <sai.miduthuri@anyscale.com>
Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>

Author: 3 people

raycicmd/change_lister.go

@@ -3,52 +3,74 @@ package raycicmd
 import (
 	"fmt"
 	"os/exec"
+	"regexp"
 	"strings"
 )
 
+var (
+	gitRefRe     = regexp.MustCompile(`^[a-zA-Z0-9][a-zA-Z0-9._/-]*$`)
+	commitHashRe = regexp.MustCompile(`^[0-9a-fA-F]{4,40}$`)
+)
+
 // ChangeLister lists changed files between two directory states.
 type ChangeLister interface {
 	ListChangedFiles() ([]string, error)
 }
 
-// GitChangeLister lists files changed by finding the merge-base (common ancestor)
+// gitChangeLister lists files changed by finding the merge-base (common ancestor)
 // between the base branch and the commit, then diffing against that. This
 // correctly shows only changes in a style similar to GitHub PR diffs, excluding
 // changes that happened on the base branch after the PR branch was created.
-type GitChangeLister struct {
+type gitChangeLister struct {
 	// WorkDir is the directory to run git commands in. If empty, uses current
 	// directory.
-	WorkDir string
+	workDir string
 
 	// Remote is the name of the git remote. If empty, defaults to "origin".
-	Remote string
+	remote string
 
 	// BaseBranch is the base branch to diff against (e.g., "main" or "master").
-	BaseBranch string
+	baseBranch string
 
 	// Commit is the commit to diff from the base branch.
-	Commit string
+	commit string
 }
 
-func (g *GitChangeLister) remote() string {
-	if g.Remote != "" {
-		return g.Remote
+func newGitChangeLister(
+	workDir, remote, baseBranch, commit string,
+) (*gitChangeLister, error) {
+	if remote == "" {
+		remote = "origin"
+	}
+	if !gitRefRe.MatchString(remote) {
+		return nil, fmt.Errorf("invalid remote name: %q", remote)
+	}
+	if !gitRefRe.MatchString(baseBranch) {
+		return nil, fmt.Errorf("invalid base branch name: %q", baseBranch)
 	}
-	return "origin"
+	if !commitHashRe.MatchString(commit) {
+		return nil, fmt.Errorf("invalid commit hash: %q", commit)
+	}
+	return &gitChangeLister{
+		workDir:    workDir,
+		remote:     remote,
+		baseBranch: baseBranch,
+		commit:     commit,
+	}, nil
 }
 
-func (g *GitChangeLister) run(args ...string) ([]byte, error) {
+func (g *gitChangeLister) run(args ...string) ([]byte, error) {
 	cmd := exec.Command("git", args...)
-	if g.WorkDir != "" {
-		cmd.Dir = g.WorkDir
+	if g.workDir != "" {
+		cmd.Dir = g.workDir
 	}
 	return cmd.Output()
 }
 
-func (g *GitChangeLister) runNoOutput(args ...string) error {
+func (g *gitChangeLister) runNoOutput(args ...string) error {
 	cmd := exec.Command("git", args...)
-	if g.WorkDir != "" {
-		cmd.Dir = g.WorkDir
+	if g.workDir != "" {
+		cmd.Dir = g.workDir
 	}
 	return cmd.Run()
 }
@@ -58,14 +80,17 @@ func (g *GitChangeLister) runNoOutput(args ...string) error {
 // It uses the merge-base of (remote/BaseBranch, Commit) so it only includes
 // changes introduced on the feature branch, matching GitHub PR diffs and
 // excluding new commits on the base branch.
-func (g *GitChangeLister) ListChangedFiles() ([]string, error) {
-	remote := g.remote()
+func (g *gitChangeLister) ListChangedFiles() ([]string, error) {
+	remote := g.remote
 
 	// Fetch the base branch with a refspec so the remote-tracking ref exists
 	// for merge-base. Plain `git fetch origin <branch>` only updates FETCH_HEAD.
-	remoteBranchRef := fmt.Sprintf("refs/remotes/%s/%s", remote, g.BaseBranch)
-	refspec := fmt.Sprintf("+%s:%s", g.BaseBranch, remoteBranchRef)
-	isShallowOut, _ := g.run("rev-parse", "--is-shallow-repository")
+	remoteBranchRef := fmt.Sprintf("refs/remotes/%s/%s", remote, g.baseBranch)
+	refspec := fmt.Sprintf("+%s:%s", g.baseBranch, remoteBranchRef)
+	isShallowOut, err := g.run("rev-parse", "--is-shallow-repository")
+	if err != nil {
+		return nil, fmt.Errorf("git rev-parse --is-shallow-repository: %w", err)
+	}
 	if strings.TrimSpace(string(isShallowOut)) == "true" {
 		if err := g.runNoOutput("fetch", "--unshallow", "-q", remote, refspec); err != nil {
 			return nil, fmt.Errorf("git fetch --unshallow %s %s: %w", remote, refspec, err)
@@ -89,15 +114,15 @@ func (g *GitChangeLister) ListChangedFiles() ([]string, error) {
 	//   merge-base(remote/base, B) = A
 	//   diff(A, B) = [feature.go]           <-- correct
 	//   diff(C, B) = [feature.go, other.go] <-- wrong
-	remoteBranch := remote + "/" + g.BaseBranch
-	mergeBaseOut, err := g.run("merge-base", remoteBranch, g.Commit)
+	remoteBranch := remote + "/" + g.baseBranch
+	mergeBaseOut, err := g.run("merge-base", remoteBranch, g.commit)
 	if err != nil {
-		return nil, fmt.Errorf("git merge-base %s %s: %w", remoteBranch, g.Commit, err)
+		return nil, fmt.Errorf("git merge-base %s %s: %w", remoteBranch, g.commit, err)
 	}
 	mergeBase := strings.TrimSpace(string(mergeBaseOut))
 
 	// Diff only the PR changes: merge-base..Commit.
-	diffRange := mergeBase + ".." + g.Commit
+	diffRange := mergeBase + ".." + g.commit
 	diffOut, err := g.run("diff", "--name-only", diffRange, "--")
 	if err != nil {
 		return nil, fmt.Errorf("git diff %s: %w", diffRange, err)

raycicmd/change_lister_test.go

@@ -98,6 +98,17 @@ func (h *gitTestHelper) initialCommit() {
 	h.git("commit", "-m", "initial commit")
 }
 
+func mustNewGitChangeLister(
+	t *testing.T, workDir, remote, baseBranch, commit string,
+) *gitChangeLister {
+	t.Helper()
+	lister, err := newGitChangeLister(workDir, remote, baseBranch, commit)
+	if err != nil {
+		t.Fatalf("newGitChangeLister: %v", err)
+	}
+	return lister
+}
+
 // TestListChangedFiles verifies basic changed file detection.
 //
 // Git history (time flows left to right):
@@ -121,7 +132,7 @@ func TestListChangedFiles(t *testing.T) {
 	wantFiles := []string{"src/main.go", "src/util.go", "docs/readme.txt"}
 	commit := h.commitFiles("add files", wantFiles...)
 
-	lister := &GitChangeLister{WorkDir: h.WorkDir, BaseBranch: "master", Commit: commit}
+	lister := mustNewGitChangeLister(t, h.WorkDir, "", "master", commit)
 	files, err := lister.ListChangedFiles()
 	if err != nil {
 		t.Fatalf("ListChangedFiles: %v", err)
@@ -175,7 +186,7 @@ func TestListChangedFiles_MergeBase(t *testing.T) {
 
 	// The diff should only show feature.go, not other.go,
 	// because we diff against the merge-base (common ancestor).
-	lister := &GitChangeLister{WorkDir: h.WorkDir, BaseBranch: "master", Commit: featureCommit}
+	lister := mustNewGitChangeLister(t, h.WorkDir, "", "master", featureCommit)
 	files, err := lister.ListChangedFiles()
 	if err != nil {
 		t.Fatalf("ListChangedFiles: %v", err)
@@ -203,7 +214,7 @@ func TestListChangedFiles_CustomRemote(t *testing.T) {
 	commit := h.commitFiles("add feature", "feature.go")
 
 	// Use a lister with custom remote.
-	lister := &GitChangeLister{WorkDir: h.WorkDir, Remote: "upstream", BaseBranch: "master", Commit: commit}
+	lister := mustNewGitChangeLister(t, h.WorkDir, "upstream", "master", commit)
 	files, err := lister.ListChangedFiles()
 	if err != nil {
 		t.Fatalf("ListChangedFiles: %v", err)
@@ -234,7 +245,7 @@ func TestListChangedFiles_MultipleCommits(t *testing.T) {
 	h.commitFiles("first commit", "first.go")
 	commit := h.commitFiles("second commit", "second.go")
 
-	lister := &GitChangeLister{WorkDir: h.WorkDir, BaseBranch: "master", Commit: commit}
+	lister := mustNewGitChangeLister(t, h.WorkDir, "", "master", commit)
 	files, err := lister.ListChangedFiles()
 	if err != nil {
 		t.Fatalf("ListChangedFiles: %v", err)
@@ -275,7 +286,7 @@ func TestListChangedFiles_ModifyExistingFile(t *testing.T) {
 	h.git("commit", "-m", "modify readme")
 	commit := h.head()
 
-	lister := &GitChangeLister{WorkDir: h.WorkDir, BaseBranch: "master", Commit: commit}
+	lister := mustNewGitChangeLister(t, h.WorkDir, "", "master", commit)
 	files, err := lister.ListChangedFiles()
 	if err != nil {
 		t.Fatalf("ListChangedFiles: %v", err)
@@ -312,7 +323,7 @@ func TestListChangedFiles_DeleteFile(t *testing.T) {
 	h.git("commit", "-m", "delete file")
 	commit := h.head()
 
-	lister := &GitChangeLister{WorkDir: h.WorkDir, BaseBranch: "master", Commit: commit}
+	lister := mustNewGitChangeLister(t, h.WorkDir, "", "master", commit)
 	files, err := lister.ListChangedFiles()
 	if err != nil {
 		t.Fatalf("ListChangedFiles: %v", err)
@@ -347,7 +358,7 @@ func TestListChangedFiles_RenameFile(t *testing.T) {
 	h.git("commit", "-m", "rename file")
 	commit := h.head()
 
-	lister := &GitChangeLister{WorkDir: h.WorkDir, BaseBranch: "master", Commit: commit}
+	lister := mustNewGitChangeLister(t, h.WorkDir, "", "master", commit)
 	files, err := lister.ListChangedFiles()
 	if err != nil {
 		t.Fatalf("ListChangedFiles: %v", err)
@@ -382,7 +393,7 @@ func TestListChangedFiles_NoChanges(t *testing.T) {
 	// No changes, just branch off
 	commit := h.head()
 
-	lister := &GitChangeLister{WorkDir: h.WorkDir, BaseBranch: "master", Commit: commit}
+	lister := mustNewGitChangeLister(t, h.WorkDir, "", "master", commit)
 	files, err := lister.ListChangedFiles()
 	if err != nil {
 		t.Fatalf("ListChangedFiles: %v", err)
@@ -425,7 +436,7 @@ func TestListChangedFiles_SameFileModifiedOnBothBranches(t *testing.T) {
 	h.git("commit", "-m", "modify shared.go on master")
 	h.git("push", "origin", "master")
 
-	lister := &GitChangeLister{WorkDir: h.WorkDir, BaseBranch: "master", Commit: featureCommit}
+	lister := mustNewGitChangeLister(t, h.WorkDir, "", "master", featureCommit)
 	files, err := lister.ListChangedFiles()
 	if err != nil {
 		t.Fatalf("ListChangedFiles: %v", err)
@@ -467,7 +478,7 @@ func TestListChangedFiles_ShallowClone(t *testing.T) {
 	runGitCommand(t, shallowDir, "fetch", "--depth=1", "origin", "feature-branch")
 	runGitCommand(t, shallowDir, "checkout", "-f", featureCommit)
 
-	lister := &GitChangeLister{WorkDir: shallowDir, BaseBranch: "master", Commit: featureCommit}
+	lister := mustNewGitChangeLister(t, shallowDir, "", "master", featureCommit)
 	files, err := lister.ListChangedFiles()
 	if err != nil {
 		t.Fatalf("ListChangedFiles() = %v, want nil", err)
@@ -520,7 +531,7 @@ func TestListChangedFiles_ShallowCloneNonDefaultBase(t *testing.T) {
 	runGitCommand(t, shallowDir, "fetch", "--depth=1", "origin", "stacked-branch")
 	runGitCommand(t, shallowDir, "checkout", "-f", stackedCommit)
 
-	lister := &GitChangeLister{WorkDir: shallowDir, BaseBranch: "base-branch", Commit: stackedCommit}
+	lister := mustNewGitChangeLister(t, shallowDir, "", "base-branch", stackedCommit)
 	files, err := lister.ListChangedFiles()
 	if err != nil {
 		t.Fatalf("ListChangedFiles() got %v, want nil", err)
@@ -550,7 +561,7 @@ func TestListChangedFiles_DeepDirectoryStructure(t *testing.T) {
 	wantFiles := []string{"a/b/c/deep.go", "x/y/z/another.go"}
 	commit := h.commitFiles("add nested files", wantFiles...)
 
-	lister := &GitChangeLister{WorkDir: h.WorkDir, BaseBranch: "master", Commit: commit}
+	lister := mustNewGitChangeLister(t, h.WorkDir, "", "master", commit)
 	files, err := lister.ListChangedFiles()
 	if err != nil {
 		t.Fatalf("ListChangedFiles: %v", err)
@@ -565,3 +576,108 @@ func TestListChangedFiles_DeepDirectoryStructure(t *testing.T) {
 		}
 	}
 }
+
+func TestNewGitChangeLister(t *testing.T) {
+	tests := []struct {
+		name       string
+		remote     string
+		baseBranch string
+		commit     string
+		wantErr    bool
+		wantRemote string
+	}{
+		{
+			name:       "valid inputs",
+			remote:     "origin",
+			baseBranch: "main",
+			commit:     "abc123def456",
+			wantRemote: "origin",
+		},
+		{
+			name:       "empty remote defaults to origin",
+			remote:     "",
+			baseBranch: "main",
+			commit:     "abc123def456",
+			wantRemote: "origin",
+		},
+		{
+			name:       "remote with dash prefix",
+			remote:     "--upload-pack=evil",
+			baseBranch: "main",
+			commit:     "abc123def456",
+			wantErr:    true,
+		},
+		{
+			name:       "base branch with dash prefix",
+			remote:     "origin",
+			baseBranch: "--upload-pack=evil",
+			commit:     "abc123def456",
+			wantErr:    true,
+		},
+		{
+			name:       "commit with non-hex characters",
+			remote:     "origin",
+			baseBranch: "main",
+			commit:     "not-a-hex-hash",
+			wantErr:    true,
+		},
+		{
+			name:       "commit too short",
+			remote:     "origin",
+			baseBranch: "main",
+			commit:     "abc",
+			wantErr:    true,
+		},
+		{
+			name:       "full 40-char commit hash",
+			remote:     "origin",
+			baseBranch: "main",
+			commit:     "abc123def456abc123def456abc123def456abcd",
+			wantRemote: "origin",
+		},
+		{
+			name:       "branch with slashes",
+			remote:     "origin",
+			baseBranch: "feature/my-branch",
+			commit:     "abc123def456",
+			wantRemote: "origin",
+		},
+		{
+			name:       "empty base branch",
+			remote:     "origin",
+			baseBranch: "",
+			commit:     "abc123def456",
+			wantErr:    true,
+		},
+		{
+			name:       "empty commit",
+			remote:     "origin",
+			baseBranch: "main",
+			commit:     "",
+			wantErr:    true,
+		},
+	}
+
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			lister, err := newGitChangeLister(
+				"/tmp", tc.remote, tc.baseBranch, tc.commit,
+			)
+			if tc.wantErr {
+				if err == nil {
+					t.Errorf("newGitChangeLister() = nil, want error")
+				}
+				return
+			}
+			if err != nil {
+				t.Fatalf("newGitChangeLister() = %v, want nil", err)
+			}
+			if lister.remote != tc.wantRemote {
+				t.Errorf(
+					"remote = %q, want %q",
+					lister.remote, tc.wantRemote,
+				)
+			}
+		})
+	}
+}

raycicmd/main.go

@@ -197,10 +197,19 @@ func Main(args []string, envs Envs) error {
 		return fmt.Errorf("make build info: %w", err)
 	}
 
-	lister := &GitChangeLister{
-		WorkDir:    flags.RepoDir,
-		BaseBranch: getEnv(envs, "BUILDKITE_PULL_REQUEST_BASE_BRANCH"),
-		Commit:     getEnv(envs, "BUILDKITE_COMMIT"),
+	// baseBranch and commit are empty for non-PR Buildkite builds (e.g.,
+	// branch pushes, scheduled builds). The lister is only used for PR
+	// builds, where RunTagAnalysis diffs changed files against the base.
+	var lister ChangeLister
+	baseBranch := getEnv(envs, "BUILDKITE_PULL_REQUEST_BASE_BRANCH")
+	commit := getEnv(envs, "BUILDKITE_COMMIT")
+	if baseBranch != "" && commit != "" {
+		lister, err = newGitChangeLister(
+			flags.RepoDir, "origin", baseBranch, commit,
+		)
+		if err != nil {
+			return fmt.Errorf("create change lister: %w", err)
+		}
 	}
 	pipeline, err := makePipeline(&pipelineContext{
 		repoDir:      flags.RepoDir,