From 4eb05a3600dcdc068ea68415ea4c1c121c57e425 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Wed, 29 Jul 2026 20:44:14 -0500 Subject: [PATCH 1/3] fix(git): disable LFS filter driver during clone when git-lfs binary is missing GIT_LFS_SKIP_SMUDGE is only consulted by the git-lfs binary itself, so setting it did nothing when the binary wasn't installed: git still tried to exec the missing filter driver during checkout and failed outright. Override filter.lfs.process/smudge/clean instead so checkout falls back to pointer stubs. Also stop removeGitCredentialHelper from erroring when a failed clone already removed the workspace dir. --- e2e/tests/up/git_lfs_test.go | 85 ++++++++++++++++++++++++++++++++++++ pkg/agent/workspace.go | 11 +++-- pkg/git/lfs.go | 26 ++++++++--- pkg/git/lfs_test.go | 20 ++++++--- pkg/git/repo.go | 9 ++-- 5 files changed, 130 insertions(+), 21 deletions(-) create mode 100644 e2e/tests/up/git_lfs_test.go diff --git a/e2e/tests/up/git_lfs_test.go b/e2e/tests/up/git_lfs_test.go new file mode 100644 index 000000000..d3acfbe86 --- /dev/null +++ b/e2e/tests/up/git_lfs_test.go @@ -0,0 +1,85 @@ +package up + +import ( + "context" + "os" + "os/exec" + "path/filepath" + "testing" + + "github.com/devsy-org/devsy/pkg/command" + "github.com/devsy-org/devsy/pkg/git" +) + +func TestCloneLFSRepoSucceedsWithoutGitLFSBinary(t *testing.T) { + const gitBin = "git" + + if !command.Exists(gitBin) { + t.Skip("git not installed") + } + + realGit, err := exec.LookPath(gitBin) + if err != nil { + t.Skip("git not found on PATH") + } + + binDir := t.TempDir() + if err := os.Symlink(realGit, filepath.Join(binDir, gitBin)); err != nil { + t.Fatal(err) + } + t.Setenv("PATH", binDir) + + home := t.TempDir() + t.Setenv("HOME", home) + t.Setenv("GIT_CONFIG_NOSYSTEM", "1") + runGit(t, home, "config", "--global", "user.email", "test@example.com") + runGit(t, home, "config", "--global", "user.name", "Test") + runGit(t, home, "config", "--global", "filter.lfs.process", "git-lfs filter-process") + runGit(t, home, "config", "--global", "filter.lfs.smudge", "git-lfs smudge -- %f") + runGit(t, home, "config", "--global", "filter.lfs.clean", "git-lfs clean -- %f") + + sourceDir := t.TempDir() + runGit(t, sourceDir, "init", "--quiet") + runGit(t, sourceDir, "config", "filter.lfs.clean", "cat") + runGit(t, sourceDir, "config", "filter.lfs.smudge", "cat") + runGit(t, sourceDir, "config", "filter.lfs.process", "") + + pointer := "version https://git-lfs.github.com/spec/v1\n" + + "oid sha256:0000000000000000000000000000000000000000000000000000000000000\n" + + "size 4\n" + if err := os.WriteFile( + filepath.Join(sourceDir, ".gitattributes"), + []byte("*.bin filter=lfs diff=lfs merge=lfs -text\n"), + 0o600, + ); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(sourceDir, "data.bin"), []byte(pointer), 0o600); err != nil { + t.Fatal(err) + } + runGit(t, sourceDir, "add", ".") + runGit(t, sourceDir, "commit", "--quiet", "-m", "add lfs-tracked file") + + targetDir := filepath.Join(t.TempDir(), "clone") + if err := git.At(targetDir).Clone(context.Background(), sourceDir); err != nil { + t.Fatalf("clone: %v", err) + } + + got, err := os.ReadFile(filepath.Join(targetDir, "data.bin")) + if err != nil { + t.Fatal(err) + } + if string(got) != pointer { + t.Errorf("data.bin content = %q, want pointer stub %q", got, pointer) + } +} + +func runGit(t *testing.T, dir string, args ...string) { + t.Helper() + cmd := exec.Command("git", args...) // #nosec G204 -- fixed binary name, test-only + cmd.Dir = dir + out, err := cmd.CombinedOutput() + if err != nil { + t.Fatalf("git %v: %v\n%s", args, err, out) + } +} diff --git a/pkg/agent/workspace.go b/pkg/agent/workspace.go index b3e91ecb5..ac81a5146 100644 --- a/pkg/agent/workspace.go +++ b/pkg/agent/workspace.go @@ -297,10 +297,13 @@ func removeGitCredentialHelper(ctx context.Context, helper, workspaceDir string) if helper == "" { return } - if err := gitcredentials.RemoveHelperFromPath( - ctx, - gitcredentials.GetLocalGitConfigPath(workspaceDir), - ); err != nil { + gitConfigPath := gitcredentials.GetLocalGitConfigPath(workspaceDir) + if _, err := os.Stat(gitConfigPath); err != nil { + // Nothing to clean up, e.g. the clone failed and the workspace dir was + // already removed. + return + } + if err := gitcredentials.RemoveHelperFromPath(ctx, gitConfigPath); err != nil { log.Errorf("remove git credential helper: %v", err) } } diff --git a/pkg/git/lfs.go b/pkg/git/lfs.go index 8c9a8f64b..f8375846d 100644 --- a/pkg/git/lfs.go +++ b/pkg/git/lfs.go @@ -13,15 +13,29 @@ import ( "github.com/devsy-org/devsy/pkg/log" ) -// lfsFilterMarker is the .gitattributes token that marks a path as LFS-tracked. var lfsFilterMarker = []byte("filter=lfs") -// smudgeSkippedForClone reports whether the clone should set GIT_LFS_SKIP_SMUDGE. -func smudgeSkippedForClone(mode LFSMode) bool { - if mode == LFSFull && !command.Exists(binGitLFS) { - log.Info("git-lfs not found, skipping LFS smudge; LFS files will be pointer stubs") +// GIT_LFS_SKIP_SMUDGE is read by the git-lfs binary itself, so it's no help +// when that binary is missing; disable the filter driver instead. +var lfsDisableFilterArgs = []string{ + flagConfig, "filter.lfs.process=", + flagConfig, "filter.lfs.smudge=cat", + flagConfig, "filter.lfs.clean=cat", +} + +func cloneArgsForLFS() []string { + if !command.Exists(binGitLFS) { + log.Info("git-lfs not found, disabling LFS filters for clone; LFS files will be pointer stubs") + return lfsDisableFilterArgs + } + return nil +} + +func cloneEnvForLFS() []string { + if command.Exists(binGitLFS) { + return []string{"GIT_LFS_SKIP_SMUDGE=1"} } - return true + return nil } // SetupLFS configures Git LFS in the repository. diff --git a/pkg/git/lfs_test.go b/pkg/git/lfs_test.go index 14f4bfde8..c2b8116c9 100644 --- a/pkg/git/lfs_test.go +++ b/pkg/git/lfs_test.go @@ -32,14 +32,20 @@ func TestLFSModeResolution(t *testing.T) { } } -func TestSmudgeAlwaysSkippedForClone(t *testing.T) { - // The smudge filter is skipped at clone time for every mode; hydration is - // done explicitly afterward (or not at all). - for _, mode := range []LFSMode{LFSFull, LFSSetupOnly, LFSSkip} { - if !smudgeSkippedForClone(mode) { - t.Errorf("smudgeSkippedForClone(%v) = false, want true", mode) - } +func TestCloneEnvForLFS(t *testing.T) { + want := []string(nil) + if command.Exists(binGitLFS) { + want = []string{"GIT_LFS_SKIP_SMUDGE=1"} + } + assert.DeepEqual(t, want, cloneEnvForLFS()) +} + +func TestCloneArgsForLFS(t *testing.T) { + var want []string + if !command.Exists(binGitLFS) { + want = lfsDisableFilterArgs } + assert.DeepEqual(t, want, cloneArgsForLFS()) } // lfsSubcommands returns the `lfs ...` invocations a fakeRunner recorded. diff --git a/pkg/git/repo.go b/pkg/git/repo.go index d55ae65fa..656a90bb4 100644 --- a/pkg/git/repo.go +++ b/pkg/git/repo.go @@ -265,13 +265,14 @@ func (r *Repo) cloneWith(ctx context.Context, repository string, c cloneConfig) pw := newProgressWriter(w) env := append([]string{}, r.env...) - if smudgeSkippedForClone(c.lfsMode) { - env = append(env, "GIT_LFS_SKIP_SMUDGE=1") - } + env = append(env, cloneEnvForLFS()...) + + args := c.args(repository, r.path) + args = append(args, cloneArgsForLFS()...) _, err := r.runner.Run(ctx, RunOptions{ Env: env, - Args: c.args(repository, r.path), + Args: args, Stdout: pw, Stderr: pw, }) From 1e0ce245f7365dd61e5c994b2c9519443ba94c4d Mon Sep 17 00:00:00 2001 From: Samuel K Date: Wed, 29 Jul 2026 21:02:56 -0500 Subject: [PATCH 2/3] fix(git): satisfy golangci-lint (cyclop, golines, gosec) on the LFS fix Split the integration test into focused helpers to reduce cyclomatic complexity, wrap long lines, and annotate the test-owned file read. --- e2e/tests/up/git_lfs_test.go | 98 +++++++++++++++++++++++------------- pkg/git/lfs.go | 4 +- 2 files changed, 66 insertions(+), 36 deletions(-) diff --git a/e2e/tests/up/git_lfs_test.go b/e2e/tests/up/git_lfs_test.go index d3acfbe86..a40aa15fd 100644 --- a/e2e/tests/up/git_lfs_test.go +++ b/e2e/tests/up/git_lfs_test.go @@ -11,23 +11,56 @@ import ( "github.com/devsy-org/devsy/pkg/git" ) +const lfsPointer = "version https://git-lfs.github.com/spec/v1\n" + + "oid sha256:0000000000000000000000000000000000000000000000000000000000000\n" + + "size 4\n" + func TestCloneLFSRepoSucceedsWithoutGitLFSBinary(t *testing.T) { - const gitBin = "git" + hideGitLFSFromPath(t) + simulateStaleGlobalLFSConfig(t) - if !command.Exists(gitBin) { - t.Skip("git not installed") + sourceDir := newLFSFixtureRepo(t) + + targetDir := filepath.Join(t.TempDir(), "clone") + if err := git.At(targetDir).Clone(context.Background(), sourceDir); err != nil { + t.Fatalf("clone: %v", err) + } + + dataPath := filepath.Join(targetDir, "data.bin") + got, err := os.ReadFile(dataPath) // #nosec G304 -- path is under a test-owned t.TempDir() + if err != nil { + t.Fatal(err) + } + if string(got) != lfsPointer { + t.Errorf("data.bin content = %q, want pointer stub %q", got, lfsPointer) } +} + +// hideGitLFSFromPath restricts PATH to a directory containing only "git", +// simulating a host without the git-lfs binary installed. +func hideGitLFSFromPath(t *testing.T) { + t.Helper() - realGit, err := exec.LookPath(gitBin) + if !command.Exists("git") { + t.Skip("git not installed") + } + realGit, err := exec.LookPath("git") if err != nil { t.Skip("git not found on PATH") } binDir := t.TempDir() - if err := os.Symlink(realGit, filepath.Join(binDir, gitBin)); err != nil { + if err := os.Symlink(realGit, filepath.Join(binDir, "git")); err != nil { t.Fatal(err) } t.Setenv("PATH", binDir) +} + +// simulateStaleGlobalLFSConfig sets a fresh, isolated global gitconfig that +// registers the LFS filter driver, as if `git lfs install --global` had been +// run in the past even though the git-lfs binary is no longer present. +func simulateStaleGlobalLFSConfig(t *testing.T) { + t.Helper() home := t.TempDir() t.Setenv("HOME", home) @@ -37,41 +70,36 @@ func TestCloneLFSRepoSucceedsWithoutGitLFSBinary(t *testing.T) { runGit(t, home, "config", "--global", "filter.lfs.process", "git-lfs filter-process") runGit(t, home, "config", "--global", "filter.lfs.smudge", "git-lfs smudge -- %f") runGit(t, home, "config", "--global", "filter.lfs.clean", "git-lfs clean -- %f") +} - sourceDir := t.TempDir() - runGit(t, sourceDir, "init", "--quiet") - runGit(t, sourceDir, "config", "filter.lfs.clean", "cat") - runGit(t, sourceDir, "config", "filter.lfs.smudge", "cat") - runGit(t, sourceDir, "config", "filter.lfs.process", "") - - pointer := "version https://git-lfs.github.com/spec/v1\n" + - "oid sha256:0000000000000000000000000000000000000000000000000000000000000\n" + - "size 4\n" - if err := os.WriteFile( - filepath.Join(sourceDir, ".gitattributes"), - []byte("*.bin filter=lfs diff=lfs merge=lfs -text\n"), - 0o600, - ); err != nil { - t.Fatal(err) - } - if err := os.WriteFile(filepath.Join(sourceDir, "data.bin"), []byte(pointer), 0o600); err != nil { - t.Fatal(err) - } - runGit(t, sourceDir, "add", ".") - runGit(t, sourceDir, "commit", "--quiet", "-m", "add lfs-tracked file") +// newLFSFixtureRepo creates a repo declaring an LFS-tracked file, using local +// filter overrides so fixture setup doesn't need the (absent) git-lfs binary. +func newLFSFixtureRepo(t *testing.T) string { + t.Helper() - targetDir := filepath.Join(t.TempDir(), "clone") - if err := git.At(targetDir).Clone(context.Background(), sourceDir); err != nil { - t.Fatalf("clone: %v", err) - } + dir := t.TempDir() + runGit(t, dir, "init", "--quiet") + runGit(t, dir, "config", "filter.lfs.clean", "cat") + runGit(t, dir, "config", "filter.lfs.smudge", "cat") + runGit(t, dir, "config", "filter.lfs.process", "") - got, err := os.ReadFile(filepath.Join(targetDir, "data.bin")) - if err != nil { + writeFile( + t, + filepath.Join(dir, ".gitattributes"), + "*.bin filter=lfs diff=lfs merge=lfs -text\n", + ) + writeFile(t, filepath.Join(dir, "data.bin"), lfsPointer) + + runGit(t, dir, "add", ".") + runGit(t, dir, "commit", "--quiet", "-m", "add lfs-tracked file") + return dir +} + +func writeFile(t *testing.T, path, content string) { + t.Helper() + if err := os.WriteFile(path, []byte(content), 0o600); err != nil { t.Fatal(err) } - if string(got) != pointer { - t.Errorf("data.bin content = %q, want pointer stub %q", got, pointer) - } } func runGit(t *testing.T, dir string, args ...string) { diff --git a/pkg/git/lfs.go b/pkg/git/lfs.go index f8375846d..0904e7912 100644 --- a/pkg/git/lfs.go +++ b/pkg/git/lfs.go @@ -25,7 +25,9 @@ var lfsDisableFilterArgs = []string{ func cloneArgsForLFS() []string { if !command.Exists(binGitLFS) { - log.Info("git-lfs not found, disabling LFS filters for clone; LFS files will be pointer stubs") + log.Info( + "git-lfs not found, disabling LFS filters for clone; LFS files will be pointer stubs", + ) return lfsDisableFilterArgs } return nil From da68e0be75aebf3b1856abf86bff1999f1f8a612 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Wed, 29 Jul 2026 23:16:11 -0500 Subject: [PATCH 3/3] fix(git): address CodeRabbit review on the LFS fix - Only ignore os.ErrNotExist when skipping credential-helper cleanup; log other stat failures instead of swallowing them. - Keep "cat" reachable in the e2e test's restricted PATH so it exercises the clone's filter.lfs.smudge=cat override directly, rather than passing via git's fallback for an unresolvable filter. --- e2e/tests/up/git_lfs_test.go | 20 ++++++++++++-------- pkg/agent/workspace.go | 5 +++-- 2 files changed, 15 insertions(+), 10 deletions(-) diff --git a/e2e/tests/up/git_lfs_test.go b/e2e/tests/up/git_lfs_test.go index a40aa15fd..eabe42e18 100644 --- a/e2e/tests/up/git_lfs_test.go +++ b/e2e/tests/up/git_lfs_test.go @@ -36,22 +36,26 @@ func TestCloneLFSRepoSucceedsWithoutGitLFSBinary(t *testing.T) { } } -// hideGitLFSFromPath restricts PATH to a directory containing only "git", -// simulating a host without the git-lfs binary installed. +// hideGitLFSFromPath restricts PATH to a directory containing only "git" and +// "cat", simulating a host without the git-lfs binary installed. "cat" stays +// available so the clone's filter.lfs.smudge=cat override actually runs, +// rather than relying on git's fallback for an unresolvable filter command. func hideGitLFSFromPath(t *testing.T) { t.Helper() if !command.Exists("git") { t.Skip("git not installed") } - realGit, err := exec.LookPath("git") - if err != nil { - t.Skip("git not found on PATH") - } binDir := t.TempDir() - if err := os.Symlink(realGit, filepath.Join(binDir, "git")); err != nil { - t.Fatal(err) + for _, name := range []string{"git", "cat"} { + path, err := exec.LookPath(name) + if err != nil { + t.Skipf("%s not found on PATH", name) + } + if err := os.Symlink(path, filepath.Join(binDir, name)); err != nil { + t.Fatal(err) + } } t.Setenv("PATH", binDir) } diff --git a/pkg/agent/workspace.go b/pkg/agent/workspace.go index ac81a5146..c535868ee 100644 --- a/pkg/agent/workspace.go +++ b/pkg/agent/workspace.go @@ -299,8 +299,9 @@ func removeGitCredentialHelper(ctx context.Context, helper, workspaceDir string) } gitConfigPath := gitcredentials.GetLocalGitConfigPath(workspaceDir) if _, err := os.Stat(gitConfigPath); err != nil { - // Nothing to clean up, e.g. the clone failed and the workspace dir was - // already removed. + if !errors.Is(err, os.ErrNotExist) { + log.Errorf("stat git config: %v", err) + } return } if err := gitcredentials.RemoveHelperFromPath(ctx, gitConfigPath); err != nil {