fix(git): disable LFS filter driver during clone when git-lfs binary is missing - #813
fix(git): disable LFS filter driver during clone when git-lfs binary is missing#813skevetter wants to merge 3 commits into
Conversation
…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.
✅ Deploy Preview for images-devsy-sh canceled.
|
✅ Deploy Preview for devsydev canceled.
|
|
Warning Review limit reached
Next review available in: 9 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughChangesGit LFS clone handling
Git credential cleanup
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant cloneWith
participant LFSHelpers
participant Git
participant git-lfs
cloneWith->>LFSHelpers: select clone args and environment
cloneWith->>Git: execute clone
alt git-lfs available
Git->>git-lfs: apply LFS filters with smudge skipped
else git-lfs unavailable
Git->>Git: use disabled LFS filter commands
end
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Split the integration test into focused helpers to reduce cyclomatic complexity, wrap long lines, and annotate the test-owned file read.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@e2e/tests/up/git_lfs_test.go`:
- Around line 52-56: Update the PATH setup in the test helper to keep cat
available alongside git: resolve the system cat binary, symlink it into binDir,
and fail the test if that symlink cannot be created. Preserve the existing
restricted PATH and git symlink behavior so the test still exercises the
missing-git-lfs fallback.
In `@pkg/agent/workspace.go`:
- Around line 301-304: Update the os.Stat guard in the workspace cleanup flow to
ignore only errors matching os.ErrNotExist; for other failures, log the error
before returning. Preserve the existing early return when gitConfigPath is
absent and keep credential-helper cleanup unchanged when stat succeeds.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 7716a6f0-171e-4cb7-b94f-6cb3a38c3a43
📒 Files selected for processing (5)
e2e/tests/up/git_lfs_test.gopkg/agent/workspace.gopkg/git/lfs.gopkg/git/lfs_test.gopkg/git/repo.go
- 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.
1 participant
Summary
GIT_LFS_SKIP_SMUDGEis only consulted by thegit-lfsbinary itself, so setting it did nothing when that binary wasn't installed: git still tried to exec the missing filter driver during checkout and failed outright (Clone succeeded, but checkout failed). Clone now overridesfilter.lfs.process/smudge/cleanto no-ops whengit-lfsisn't onPATH, so checkout falls back to pointer stubs instead of failing.removeGitCredentialHelperno longer errors when a failed clone already removed the workspace dir (the.git/configit tries to clean up no longer exists).e2e/tests/up/git_lfs_test.go, a real-git integration test (no mocks/docker) that reproduces the exact production failure withgit-lfshidden fromPATHand verifies the fix.Summary by CodeRabbit
Bug Fixes
Tests