feat(git): add LFS support with configuration flags#590
Merged
Conversation
✅ Deploy Preview for images-devsy-sh canceled.
|
✅ Deploy Preview for devsydev canceled.
|
📝 WalkthroughWalkthroughThis PR rewrites the git integration layer with a new ChangesGit package rewrite
Consumer migration and context propagation
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant CLI as Workspace CLI
participant Provider as pkg/workspace.UpdateProvider
participant Resolve as provider.ResolveProvider
participant Download as download.File
participant Resolver as gitcredentials.Resolver
participant Repo as git.Repo
CLI->>Provider: UpdateProvider(ctx, config, name, source)
Provider->>Resolve: ResolveProvider(ctx, source)
Resolve->>Download: File(ctx, url, WithCredentialResolver(Resolver))
Download->>Resolver: Resolve(ctx, protocol, host, path)
Resolver->>Repo: CredentialFill(ctx, request)
Repo-->>Resolver: username/password
Resolver-->>Download: credentials
Download-->>Resolve: provider.yaml bytes
Resolve-->>Provider: ProviderSource
Provider->>Provider: DownloadBinaries(ctx, ...)
Provider-->>CLI: updated provider config
sequenceDiagram
participant Up as Workspace Up
participant Repo as git.Repo (At)
participant Runner as git.Runner
participant LFS as Repo.SetupLFS
Up->>Repo: CloneFromInfo(ctx, gitInfo, helper, opts)
Repo->>Runner: Run(ctx, "clone" args)
Runner-->>Repo: RunResult / CommandError
alt PR ref
Repo->>Runner: Fetch + Switch
else commit
Repo->>Runner: Reset(ctx, commit, ResetHard)
end
Repo->>LFS: SetupLFS(ctx, mode)
LFS->>Runner: git lfs install/pull
LFS-->>Repo: done
Possibly related PRs
Suggested labels: 🚥 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 |
skevetter
changed the title
skevetter
force-pushed
the
refactor/git-repo-runner-lfs
branch
4 times, most recently
from
July 5, 2026 00:32
e1e3531 to
b075c8a
Compare
skevetter
marked this pull request as ready for review
July 5, 2026 01:05
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (6)
pkg/devcontainer/setup/setup.go (1)
515-538: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
setupPlatformGitCredentialshardcodescontext.Background()instead of acceptingctx.The function doesn't take a
context.Contextparameter, so it can't propagate a caller-supplied context togitcredentials.GetUser/SetUser— both of which were just refactored specifically to support context-based cancellation. This defeats the purpose of threading context through this chain, as called out in the PR objectives.♻️ Proposed fix
-func setupPlatformGitCredentials( - userName string, - platformOptions *devsy.PlatformOptions, -) error { +func setupPlatformGitCredentials( + ctx context.Context, + userName string, + platformOptions *devsy.PlatformOptions, +) error { // platform is not enabled, skip if !platformOptions.Enabled { return nil } // setup platform git user if platformOptions.UserCredentials.GitUser != "" && platformOptions.UserCredentials.GitEmail != "" { - gitUser, err := gitcredentials.GetUser(context.Background(), userName, "") + gitUser, err := gitcredentials.GetUser(ctx, userName, "") if err == nil && gitUser.Name == "" && gitUser.Email == "" { log.Info("Setup workspace git user and email") - err := gitcredentials.SetUser(context.Background(), userName, &gitcredentials.GitUser{ + err := gitcredentials.SetUser(ctx, userName, &gitcredentials.GitUser{ Name: platformOptions.UserCredentials.GitUser, Email: platformOptions.UserCredentials.GitEmail, })Please verify the caller has a real
ctxavailable to pass in.🤖 Prompt for 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. In `@pkg/devcontainer/setup/setup.go` around lines 515 - 538, setupPlatformGitCredentials is using context.Background() instead of the caller’s context, so the refactored context-aware gitcredentials calls cannot be canceled or scoped properly. Update setupPlatformGitCredentials to accept a ctx parameter and pass that ctx through to gitcredentials.GetUser and gitcredentials.SetUser, then verify the caller already has a real ctx available and forwards it into this helper.pkg/gitcredentials/gitcredentials.go (1)
91-136: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
ConfigureHelper/RemoveHelperFromPathalso hardcodecontext.Background()instead of acceptingctx.Same pattern as
GetCredentialsbelow: these functions run git config subprocesses but have no way for callers (CLI commands that already have acobraCmd.Context()) to bound or cancel them. Lower risk than the download-path credential resolution above, but worth aligning for consistency with the rest of the context-threading effort in this PR.🤖 Prompt for 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. In `@pkg/gitcredentials/gitcredentials.go` around lines 91 - 136, ConfigureHelper and RemoveHelperFromPath hardcode context.Background() for git config operations, so they cannot be canceled or bounded by caller context. Update these functions to accept a ctx parameter and thread it through cfg.GetAll, cfg.UnsetAll, and cfg.Add, then adjust RemoveHelper to pass the same ctx into RemoveHelperFromPath and update any callers such as the CLI flow to use their existing cobraCmd.Context() or equivalent.cmd/workspace/ssh.go (1)
760-788: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winThread
ctxthroughgpgSigningKey()setupGPGAgentalready has actx, butgpgSigningKey()drops it by usingcontext.Background()for both git config reads. Pass the caller context into the helper so config lookups can honor cancellation and timeouts.🤖 Prompt for 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. In `@cmd/workspace/ssh.go` around lines 760 - 788, Thread the caller context through gpgSigningKey() instead of using context.Background() for config reads. Update setupGPGAgent and gpgSigningKey() so the helper accepts ctx and passes it to both config.Get calls, preserving cancellation and timeout behavior while keeping the existing ssh/x509 and signingKey detection logic unchanged.pkg/git/lfs.go (1)
21-21: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the
binGitLFSconstant instead of the literal"git-lfs".
installer.goandlfs_test.goboth referencebinGitLFSfor this binary name; this file hardcodes the string twice, risking drift if the constant changes.♻️ Proposed fix
func smudgeSkippedForClone(mode LFSMode) bool { - if mode == LFSFull && !command.Exists("git-lfs") { + if mode == LFSFull && !command.Exists(binGitLFS) { log.Info("git-lfs not found, skipping LFS smudge; LFS files will be pointer stubs") } return true }- if !command.Exists("git-lfs") { + if !command.Exists(binGitLFS) { if err := InstallLFS(); err != nil {Also applies to: 36-36
🤖 Prompt for 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. In `@pkg/git/lfs.go` at line 21, The LFS binary check in lfs.go is hardcoding "git-lfs" instead of using the shared binGitLFS constant, which can drift from installer.go and lfs_test.go. Update the LFS availability checks in the relevant LFS setup logic to reference binGitLFS everywhere the binary name is compared or passed to command.Exists, keeping the binary name centralized and consistent.pkg/gitsshsigning/utils.go (1)
15-29: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThread the request context through the signing-key lookup chain.
ExtractGitConfigurationis only reached fromRunServices(ctx, ...), so passctxthroughbuildCredentialsCommandandaddGitSSHSigningKeyinstead of callingcontext.Background(). That keeps the git config lookup cancellable and consistent with the rest of the command path.🤖 Prompt for 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. In `@pkg/gitsshsigning/utils.go` around lines 15 - 29, Thread the request context through the git config lookup path in ExtractGitConfiguration instead of using context.Background(). Update the call chain from RunServices(ctx, ...) through buildCredentialsCommand and addGitSSHSigningKey so the ctx is passed into ExtractGitConfiguration, then use that ctx for both config.Get calls when reading GPGFormatConfigKey and UsersSigningKeyConfigKey. This keeps the signing-key lookup cancellable and consistent with the rest of the command flow.pkg/git/runner.go (1)
89-114: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winPreserve stderr in
CommandErrorwhen callers pass a writer
pkg/git/repo.goandpkg/git/installer.gosetRunOptions.Stderr, so those failures return aCommandErrorwith an emptyStderr. Tee stderr intoerrBuf(for example,io.MultiWriter(opts.Stderr, &errBuf)) so the git message still appears in the typed error.🤖 Prompt for 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. In `@pkg/git/runner.go` around lines 89 - 114, The git runner currently loses stderr in CommandError when RunOptions.Stderr is provided, because cmd.Stderr is replaced and errBuf stays empty. Update runner.go in the Run/command execution path to tee stderr to both the caller-provided writer and errBuf (for example via io.MultiWriter) so CommandError.Stderr is still populated. Keep the fix centered on cmd.Run, CommandError, and the opts.Stderr handling used by repo.go and installer.go.
🤖 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 `@pkg/git/installer_release.go`:
- Around line 34-52: The assetURL helper in installer_release.go is incorrectly
building a .tar.gz download URL for darwin and windows, which do not match the
published git-lfs release assets. Update the assetURL function to either branch
by goos and use the correct archive format and binary name for each platform, or
restrict the fallback to only the platforms actually supported by this
downloader; keep the existing unsupported OS/architecture checks in place.
In `@pkg/git/installer.go`:
- Around line 123-135: The releaseStrategy.install path currently trusts the
downloaded GitHub release asset without any integrity check, so add artifact
verification before installing the binary. Update releaseStrategy.install (and
any helper it delegates to) to validate the downloaded release using an
available checksum or signature mechanism before moving it into place, and fail
closed if verification is missing or does not pass. Use the existing
releaseStrategy and t.release.install flow as the hook point so the binary is
only installed after successful verification.
- Around line 11-20: The InstallBinary and InstallLFS entry points currently
hardcode context.Background(), which prevents callers from cancelling or timing
out installs. Update both functions to accept a ctx parameter and thread it
through to newInstaller(defaultRunner).ensure, then update the call sites in
workspace and LFS-related code to pass their existing request context instead of
relying on background context. Use the InstallBinary, InstallLFS, and ensure
symbols to locate the affected flow.
In `@pkg/gitcredentials/gitcredentials.go`:
- Around line 208-266: `gitcredentials.Resolver.Resolve` is discarding the
caller’s context, so private-download credential lookup cannot be cancelled.
Thread `ctx` through `GetCredentials` and
`credentialsViaServer`/`credentialsViaGit` instead of using
`context.Background()` and `exec.Command`, and ensure `Resolve` passes its `ctx`
through. Also update the direct `GetCredentials` callers in
`pkg/agent/tunnelserver/tunnelserver.go` and
`pkg/daemon/platform/local_server.go` to supply the request context
consistently.
---
Nitpick comments:
In `@cmd/workspace/ssh.go`:
- Around line 760-788: Thread the caller context through gpgSigningKey() instead
of using context.Background() for config reads. Update setupGPGAgent and
gpgSigningKey() so the helper accepts ctx and passes it to both config.Get
calls, preserving cancellation and timeout behavior while keeping the existing
ssh/x509 and signingKey detection logic unchanged.
In `@pkg/devcontainer/setup/setup.go`:
- Around line 515-538: setupPlatformGitCredentials is using context.Background()
instead of the caller’s context, so the refactored context-aware gitcredentials
calls cannot be canceled or scoped properly. Update setupPlatformGitCredentials
to accept a ctx parameter and pass that ctx through to gitcredentials.GetUser
and gitcredentials.SetUser, then verify the caller already has a real ctx
available and forwards it into this helper.
In `@pkg/git/lfs.go`:
- Line 21: The LFS binary check in lfs.go is hardcoding "git-lfs" instead of
using the shared binGitLFS constant, which can drift from installer.go and
lfs_test.go. Update the LFS availability checks in the relevant LFS setup logic
to reference binGitLFS everywhere the binary name is compared or passed to
command.Exists, keeping the binary name centralized and consistent.
In `@pkg/git/runner.go`:
- Around line 89-114: The git runner currently loses stderr in CommandError when
RunOptions.Stderr is provided, because cmd.Stderr is replaced and errBuf stays
empty. Update runner.go in the Run/command execution path to tee stderr to both
the caller-provided writer and errBuf (for example via io.MultiWriter) so
CommandError.Stderr is still populated. Keep the fix centered on cmd.Run,
CommandError, and the opts.Stderr handling used by repo.go and installer.go.
In `@pkg/gitcredentials/gitcredentials.go`:
- Around line 91-136: ConfigureHelper and RemoveHelperFromPath hardcode
context.Background() for git config operations, so they cannot be canceled or
bounded by caller context. Update these functions to accept a ctx parameter and
thread it through cfg.GetAll, cfg.UnsetAll, and cfg.Add, then adjust
RemoveHelper to pass the same ctx into RemoveHelperFromPath and update any
callers such as the CLI flow to use their existing cobraCmd.Context() or
equivalent.
In `@pkg/gitsshsigning/utils.go`:
- Around line 15-29: Thread the request context through the git config lookup
path in ExtractGitConfiguration instead of using context.Background(). Update
the call chain from RunServices(ctx, ...) through buildCredentialsCommand and
addGitSSHSigningKey so the ctx is passed into ExtractGitConfiguration, then use
that ctx for both config.Get calls when reading GPGFormatConfigKey and
UsersSigningKeyConfigKey. This keeps the signing-key lookup cancellable and
consistent with the rest of the command flow.
🪄 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
Run ID: 4d5e16fe-ef36-4901-bfe1-2a37176e738b
📒 Files selected for processing (53)
cmd/internal/agent_daemon.gocmd/internal/agentcontainer/credentials_server.gocmd/internal/agentcontainer/setup.gocmd/internal/agentworkspace/build.gocmd/internal/agentworkspace/install_dotfiles.gocmd/internal/agentworkspace/setup_gpg.gocmd/internal/agentworkspace/up.gocmd/internal/check_provider_update.gocmd/internal/container_tunnel.gocmd/internal/get_provider_name.gocmd/internal/git_credentials.gocmd/pro/login.gocmd/pro/update_provider.gocmd/provider/add.gocmd/provider/set_source.gocmd/workspace/build.gocmd/workspace/ssh.gocmd/workspace/up/up_client.gocmd/workspace/up/up_flags.gocmd/workspace/workspace.gopkg/agent/tunnelserver/tunnelserver.gopkg/agent/workspace.gopkg/devcontainer/helpers.gopkg/devcontainer/setup/dotfiles.gopkg/devcontainer/setup/setup.gopkg/download/download.gopkg/driver/custom/custom.gopkg/git/args.gopkg/git/clone.gopkg/git/clone_test.gopkg/git/config.gopkg/git/config_test.gopkg/git/git.gopkg/git/install.gopkg/git/installer.gopkg/git/installer_release.gopkg/git/installer_test.gopkg/git/lfs.gopkg/git/lfs_test.gopkg/git/repo.gopkg/git/repo_test.gopkg/git/runner.gopkg/git/runner_test.gopkg/gitcredentials/gitcredentials.gopkg/gitcredentials/gitcredentials_test.gopkg/gitsshsigning/utils.gopkg/gitsshsigning/utils_test.gopkg/provider/download.gopkg/provider/resolve.gopkg/provider/workspace.gopkg/workspace/provider.gopkg/workspace/provider_update.gopkg/workspace/provider_versions.go
💤 Files with no reviewable changes (1)
- pkg/git/install.go
Comment on lines
+123
to
+135
| // releaseStrategy installs a tool by downloading its GitHub release asset. | ||
| type releaseStrategy struct{} | ||
|
|
||
| func (releaseStrategy) name() string { return "github-release" } | ||
|
|
||
| func (releaseStrategy) usable() bool { return true } | ||
|
|
||
| func (releaseStrategy) install(ctx context.Context, t tool) error { | ||
| if t.release == nil { | ||
| return fmt.Errorf("%s has no release download available", t.binary) | ||
| } | ||
| return t.release.install(ctx, t.binary) | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd installer_release.go --exec cat -n {}Repository: devsy-org/devsy
Length of output: 5608
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the download helper and any related integrity-checking code paths.
fd download.go pkg -x sh -c 'echo "### {}"; cat -n "{}"' \; 2>/dev/null || true
rg -n "checksum|signature|sha256|gpg|cosign|verify|integrity" pkg -g '!**/*_test.go'Repository: devsy-org/devsy
Length of output: 42896
Add integrity validation before installing the GitHub release binary. releaseStrategy.install downloads and extracts the asset with no checksum or signature check, so a tampered release would be installed directly from the network. Add artifact verification before moving it into place.
🤖 Prompt for 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.
In `@pkg/git/installer.go` around lines 123 - 135, The releaseStrategy.install
path currently trusts the downloaded GitHub release asset without any integrity
check, so add artifact verification before installing the binary. Update
releaseStrategy.install (and any helper it delegates to) to validate the
downloaded release using an available checksum or signature mechanism before
moving it into place, and fail closed if verification is missing or does not
pass. Use the existing releaseStrategy and t.release.install flow as the hook
point so the binary is only installed after successful verification.
skevetter
force-pushed
the
refactor/git-repo-runner-lfs
branch
2 times, most recently
from
July 5, 2026 09:43
4ffd3b7 to
358d872
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
pkg/git/installer_release.go (1)
44-44: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winWindows/darwin release path is incompletely supported.
assetNameandchecksumsadvertisewindows/darwinassets, but two things break non-Linux installs:
binaryInArchiveis hardcoded tobinGitLFS(git-lfs), while the Windows zip containsgit-lfs.exe;findBinarywould then fail to locate the executable.installDirdefaults to/usr/local/bin, which is not a valid Windows install location.Either resolve the per-OS binary name (append
.exeon windows) and install dir, or restrict the fallback to the platforms this installer actually targets (linux) and drop the darwin/windows asset/checksum entries.#!/bin/bash rg -nP -C3 'binGitLFS|installDir|GOOS|runtime\.GOOS' pkg/git/Also applies to: 45-63, 108-108
🤖 Prompt for 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. In `@pkg/git/installer_release.go` at line 44, The release installer currently advertises windows and darwin assets but still assumes the Linux binary name and install path, so fix the platform handling in installer_release.go. Update the logic around binaryInArchive and installDir so the installer resolves the executable name per OS (for example, the windows archive binary name) and uses a valid destination for each target, or else remove the unsupported windows/darwin asset and checksum entries and keep the installer Linux-only. Use the existing installer fields and methods in installer_release.go to ensure findBinary and install behavior match the advertised release platforms.
🧹 Nitpick comments (1)
pkg/git/installer.go (1)
68-92: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider falling back to the next strategy when a package-manager install fails, not just when it's absent.
Currently, once a strategy's
install()returns an error,ensurereturns immediately (Line 79-81) instead of trying subsequent strategies. Onlyusable()==false(binary missing) causes a skip. So ifapt/apkis present but fails (no root, no network, broken repo lists — common in minimal containers), the GitHub release fallback is never attempted, even though it could still succeed.♻️ Suggested fix: fall through to next strategy on install failure
func (i *Installer) ensure(ctx context.Context, t tool) error { if command.Exists(t.binary) { return nil } tried := []string{} + var errs []error for _, s := range i.strategies { if !s.usable() { continue } tried = append(tried, s.name()) if err := s.install(ctx, t); err != nil { - return fmt.Errorf("install %s via %s: %w", t.binary, s.name(), err) + errs = append(errs, fmt.Errorf("via %s: %w", s.name(), err)) + continue } if command.Exists(t.binary) { log.Infof("installed %s via %s", t.binary, s.name()) return nil } } if len(tried) == 0 { return fmt.Errorf("no usable strategy to install %s", t.binary) } - return fmt.Errorf("installed %s but it is still not on PATH (tried: %v)", t.binary, tried) + return fmt.Errorf("failed to install %s (tried: %v): %w", t.binary, tried, errors.Join(errs...)) }Requires adding
"errors"to imports.If this scoping (fallback only for "no package manager") is intentional, feel free to disregard.
🤖 Prompt for 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. In `@pkg/git/installer.go` around lines 68 - 92, The installer fallback in ensure stops at the first strategy failure, so later strategies like GitHub releases are never tried. Update ensure in Installer to continue iterating after a failed s.install(ctx, t) instead of returning immediately, while still logging or collecting the error context if needed, and only return a failure after all usable strategies in i.strategies have been attempted.
🤖 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 `@cmd/internal/agentcontainer/credentials_server.go`:
- Around line 131-134: The deferred cleanup in credentials_server.go is reusing
the shutdown ctx, so RemoveHelper in the anonymous defer can be canceled before
it clears the git credential helper. Update the cleanup path around
gitcredentials.RemoveHelper and the deferred func(userName string) to run with a
context that is not canceled on shutdown, such as context.WithoutCancel(ctx) or
an equivalent uncanceled context, while still removing the helper for cmd.User.
---
Duplicate comments:
In `@pkg/git/installer_release.go`:
- Line 44: The release installer currently advertises windows and darwin assets
but still assumes the Linux binary name and install path, so fix the platform
handling in installer_release.go. Update the logic around binaryInArchive and
installDir so the installer resolves the executable name per OS (for example,
the windows archive binary name) and uses a valid destination for each target,
or else remove the unsupported windows/darwin asset and checksum entries and
keep the installer Linux-only. Use the existing installer fields and methods in
installer_release.go to ensure findBinary and install behavior match the
advertised release platforms.
---
Nitpick comments:
In `@pkg/git/installer.go`:
- Around line 68-92: The installer fallback in ensure stops at the first
strategy failure, so later strategies like GitHub releases are never tried.
Update ensure in Installer to continue iterating after a failed s.install(ctx,
t) instead of returning immediately, while still logging or collecting the error
context if needed, and only return a failure after all usable strategies in
i.strategies have been attempted.
🪄 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
Run ID: 117ef80e-cda7-401e-a40a-a86eea03b525
📒 Files selected for processing (57)
cmd/internal/agent_daemon.gocmd/internal/agentcontainer/credentials_server.gocmd/internal/agentcontainer/setup.gocmd/internal/agentworkspace/build.gocmd/internal/agentworkspace/install_dotfiles.gocmd/internal/agentworkspace/setup_gpg.gocmd/internal/agentworkspace/up.gocmd/internal/check_provider_update.gocmd/internal/container_tunnel.gocmd/internal/get_provider_name.gocmd/internal/git_credentials.gocmd/pro/login.gocmd/pro/update_provider.gocmd/provider/add.gocmd/provider/set_source.gocmd/workspace/build.gocmd/workspace/ssh.gocmd/workspace/ssh_test.gocmd/workspace/up/up_client.gocmd/workspace/up/up_flags.gocmd/workspace/workspace.gopkg/agent/tunnelserver/tunnelserver.gopkg/agent/workspace.gopkg/daemon/platform/local_server.gopkg/devcontainer/helpers.gopkg/devcontainer/setup/dotfiles.gopkg/devcontainer/setup/setup.gopkg/download/download.gopkg/driver/custom/custom.gopkg/git/args.gopkg/git/clone.gopkg/git/clone_test.gopkg/git/config.gopkg/git/config_test.gopkg/git/git.gopkg/git/install.gopkg/git/installer.gopkg/git/installer_release.gopkg/git/installer_test.gopkg/git/lfs.gopkg/git/lfs_test.gopkg/git/repo.gopkg/git/repo_test.gopkg/git/runner.gopkg/git/runner_test.gopkg/gitcredentials/gitcredentials.gopkg/gitcredentials/gitcredentials_test.gopkg/gitsshsigning/utils.gopkg/gitsshsigning/utils_test.gopkg/provider/download.gopkg/provider/resolve.gopkg/provider/workspace.gopkg/tunnel/services.gopkg/tunnel/services_test.gopkg/workspace/provider.gopkg/workspace/provider_update.gopkg/workspace/provider_versions.go
💤 Files with no reviewable changes (1)
- pkg/git/install.go
🚧 Files skipped from review as they are similar to previous changes (43)
- cmd/internal/agent_daemon.go
- cmd/workspace/workspace.go
- pkg/git/args.go
- cmd/internal/container_tunnel.go
- cmd/internal/agentworkspace/setup_gpg.go
- pkg/provider/workspace.go
- cmd/pro/update_provider.go
- cmd/internal/get_provider_name.go
- pkg/devcontainer/setup/dotfiles.go
- pkg/gitsshsigning/utils_test.go
- cmd/provider/add.go
- pkg/workspace/provider_update.go
- pkg/driver/custom/custom.go
- cmd/provider/set_source.go
- cmd/internal/check_provider_update.go
- pkg/gitcredentials/gitcredentials_test.go
- cmd/workspace/ssh.go
- pkg/agent/workspace.go
- cmd/workspace/up/up_flags.go
- pkg/git/runner.go
- cmd/internal/git_credentials.go
- pkg/git/repo_test.go
- cmd/workspace/build.go
- pkg/devcontainer/helpers.go
- cmd/internal/agentworkspace/install_dotfiles.go
- pkg/git/clone_test.go
- pkg/git/lfs_test.go
- pkg/workspace/provider_versions.go
- cmd/pro/login.go
- cmd/internal/agentcontainer/setup.go
- pkg/git/repo.go
- cmd/internal/agentworkspace/build.go
- pkg/git/config_test.go
- pkg/git/git.go
- pkg/git/lfs.go
- pkg/git/config.go
- pkg/download/download.go
- pkg/provider/resolve.go
- pkg/git/clone.go
- pkg/provider/download.go
- cmd/internal/agentworkspace/up.go
- pkg/workspace/provider.go
- pkg/git/runner_test.go
skevetter
marked this pull request as draft
July 5, 2026 10:00
skevetter
force-pushed
the
refactor/git-repo-runner-lfs
branch
from
July 5, 2026 10:00
358d872 to
33ba4e9
Compare
…eading Rework pkg/git around a Repo handle bound to path+env with an injectable Runner seam, replacing scattered exec calls. Adds: - Repo with Clone/CloneFromInfo/Fetch/Checkout/Reset/Config/CredentialFill - strategy-based tool installer with git-lfs GitHub-release fallback - typed CommandError + consistent git-binary preflight - three-way Git LFS mode (full/setup-only/skip) via --git-lfs-mode - depth-bounded LFS .gitattributes detection Moves credential-helper config and gitcredentials off ad-hoc file/shell surgery onto the git config seam (fixing a SetUser shell-injection), and inverts the pkg/download -> gitcredentials dependency so download stays a leaf. Threads context.Context through the download/provider/workspace clone chains to real command handlers.
skevetter
force-pushed
the
refactor/git-repo-runner-lfs
branch
from
July 5, 2026 10:17
33ba4e9 to
37f9cb5
Compare
Signed-off-by: Samuel K <skevetter@pm.me>
Signed-off-by: Samuel K <skevetter@pm.me>
skevetter
force-pushed
the
refactor/git-repo-runner-lfs
branch
from
July 5, 2026 12:58
acb4bf6 to
56ac939
Compare
skevetter
marked this pull request as ready for review
July 5, 2026 14:56
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/git/installer_test.go (1)
120-159: 🔒 Security & Privacy | 🔵 Trivial | 🏗️ Heavy liftAdd coverage for checksum-mismatch/fail-closed behavior.
TestGitLFSReleaseAllAssetsHaveChecksumsonly verifies that a checksum entry exists for each resolvable asset name; it doesn't exercise the actual verification path that the PR objectives call out as a key security addition ("failing closed on missing or mismatched digests"). A test that serves a tampered/mismatched asset (e.g., viahttptest.Server) and assertsinstallreturns an error would give confidence that the fail-closed guarantee is actually enforced at runtime, not just that the checksum table is populated.🤖 Prompt for 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. In `@pkg/git/installer_test.go` around lines 120 - 159, Add a test that exercises the real checksum verification path, not just checksum-table presence. Extend the git LFS installer tests around gitLFSRelease.install (or the relevant download/verify helper) to serve a tampered asset from an httptest.Server and assert the install fails with an error when the digest does not match, covering the fail-closed behavior alongside TestGitLFSReleaseAllAssetsHaveChecksums.
🤖 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.
Nitpick comments:
In `@pkg/git/installer_test.go`:
- Around line 120-159: Add a test that exercises the real checksum verification
path, not just checksum-table presence. Extend the git LFS installer tests
around gitLFSRelease.install (or the relevant download/verify helper) to serve a
tampered asset from an httptest.Server and assert the install fails with an
error when the digest does not match, covering the fail-closed behavior
alongside TestGitLFSReleaseAllAssetsHaveChecksums.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9ed346a6-cf4b-4548-a8a7-893b91958798
📒 Files selected for processing (63)
cmd/internal/agent_daemon.gocmd/internal/agentcontainer/credentials_server.gocmd/internal/agentcontainer/setup.gocmd/internal/agentworkspace/build.gocmd/internal/agentworkspace/install_dotfiles.gocmd/internal/agentworkspace/setup_gpg.gocmd/internal/agentworkspace/up.gocmd/internal/check_provider_update.gocmd/internal/container_tunnel.gocmd/internal/get_provider_name.gocmd/internal/git_credentials.gocmd/pro/login.gocmd/pro/update_provider.gocmd/provider/add.gocmd/provider/set_source.gocmd/workspace/build.gocmd/workspace/ssh.gocmd/workspace/ssh_test.gocmd/workspace/up/up_client.gocmd/workspace/up/up_flags.gocmd/workspace/workspace.gopkg/agent/tunnelserver/tunnelserver.gopkg/agent/workspace.gopkg/config/pathmanager.gopkg/config/pathmanager_darwin.gopkg/config/pathmanager_linux.gopkg/config/pathmanager_linux_test.gopkg/config/pathmanager_windows.gopkg/daemon/platform/local_server.gopkg/devcontainer/helpers.gopkg/devcontainer/setup/dotfiles.gopkg/devcontainer/setup/setup.gopkg/download/download.gopkg/driver/custom/custom.gopkg/git/clone.gopkg/git/clone_test.gopkg/git/config.gopkg/git/config_test.gopkg/git/const.gopkg/git/git.gopkg/git/git_test.gopkg/git/install.gopkg/git/installer.gopkg/git/installer_release.gopkg/git/installer_test.gopkg/git/lfs.gopkg/git/lfs_test.gopkg/git/repo.gopkg/git/repo_test.gopkg/git/runner.gopkg/git/runner_test.gopkg/gitcredentials/gitcredentials.gopkg/gitcredentials/gitcredentials_test.gopkg/gitsshsigning/utils.gopkg/gitsshsigning/utils_test.gopkg/provider/download.gopkg/provider/resolve.gopkg/provider/workspace.gopkg/tunnel/services.gopkg/tunnel/services_test.gopkg/workspace/provider.gopkg/workspace/provider_update.gopkg/workspace/provider_versions.go
💤 Files with no reviewable changes (1)
- pkg/git/install.go
✅ Files skipped from review due to trivial changes (3)
- pkg/git/const.go
- cmd/internal/agent_daemon.go
- cmd/internal/agentworkspace/build.go
🚧 Files skipped from review as they are similar to previous changes (52)
- cmd/workspace/workspace.go
- cmd/provider/add.go
- cmd/internal/agentworkspace/setup_gpg.go
- cmd/pro/update_provider.go
- cmd/internal/git_credentials.go
- pkg/driver/custom/custom.go
- pkg/devcontainer/setup/dotfiles.go
- pkg/provider/workspace.go
- cmd/internal/container_tunnel.go
- cmd/workspace/up/up_client.go
- cmd/internal/get_provider_name.go
- pkg/workspace/provider_versions.go
- pkg/agent/tunnelserver/tunnelserver.go
- cmd/internal/agentworkspace/install_dotfiles.go
- pkg/tunnel/services.go
- cmd/internal/agentcontainer/credentials_server.go
- cmd/workspace/up/up_flags.go
- pkg/gitsshsigning/utils_test.go
- pkg/tunnel/services_test.go
- pkg/git/config_test.go
- cmd/internal/check_provider_update.go
- pkg/git/runner_test.go
- pkg/agent/workspace.go
- pkg/workspace/provider_update.go
- cmd/provider/set_source.go
- pkg/git/clone_test.go
- cmd/workspace/build.go
- pkg/devcontainer/helpers.go
- pkg/workspace/provider.go
- pkg/devcontainer/setup/setup.go
- pkg/gitsshsigning/utils.go
- pkg/git/lfs_test.go
- pkg/gitcredentials/gitcredentials_test.go
- pkg/git/lfs.go
- pkg/git/clone.go
- cmd/workspace/ssh.go
- pkg/git/config.go
- cmd/pro/login.go
- pkg/provider/resolve.go
- pkg/git/runner.go
- pkg/git/repo_test.go
- pkg/daemon/platform/local_server.go
- pkg/git/git.go
- pkg/git/repo.go
- cmd/internal/agentcontainer/setup.go
- pkg/git/installer_release.go
- pkg/download/download.go
- cmd/workspace/ssh_test.go
- pkg/git/installer.go
- cmd/internal/agentworkspace/up.go
- pkg/provider/download.go
- pkg/gitcredentials/gitcredentials.go
This was referenced Jul 21, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Reworks
pkg/gitaround aRepohandle bound to path + environment with an injectableRunnerexecution seam, replacing scatteredexeccalls.Highlights
RepowithClone/CloneFromInfo/Fetch/Checkout/Reset/Config/CredentialFillCommandError(exit code + stderr) and a consistent git-binary preflightfull/setup-only/skip) exposed via--git-lfs-mode.gitattributesLFS detectionLayering / correctness
gitcredentialsoff ad-hoc file/shell surgery onto the git config seam (fixes aSetUsershell-injection)pkg/download→gitcredentialsdependency sodownloadstays a leafcontext.Contextthrough the download/provider/workspace clone chains to real command handlersCLI usage-error/help changes are intentionally split into a follow-up PR.
Summary by CodeRabbit
New Features
wsalias for the workspace command.Bug Fixes