From 7b56403ad6fb814820006ad82f5e4478ec4f22cc Mon Sep 17 00:00:00 2001
From: Joel Robotham <jrobotham@squareup.com>
Date: Mon, 1 Jun 2026 10:09:04 +1000
Subject: [PATCH 1/7] fix(gitclone): refresh GitHub App token mid-subprocess
 via file-based credential helper
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

GitHub App installation tokens are valid for one hour. The previous
credential.helper embedded the token as a literal in a shell-function
closure at exec time, so any git subprocess that ran longer than the
TTL — most notably 'git lfs fetch' for large LFS repositories — would
keep presenting the original token to GitHub long after it had expired
and would loop on 'Bad credentials' until git-lfs's internal retry
budget gave up (observed in production: jobs running 20+ hours before
failing).

Switch to a file-based credential helper: the token is written to a
0600 temp file and exposed as 'credential.helper=!cat <file>', which
git re-reads on every credential query. A background goroutine
re-fetches the token on a 30s ticker for the lifetime of the returned
cleanup and rewrites the file atomically when the value changes, so
long-running subprocesses transparently pick up rotated tokens on the
next git-lfs retry. The TokenManager's own caching means the ticker is
a cheap map lookup for most ticks; only the pre-expiry refresh hits
the GitHub API.

GitCommand now returns a cleanup function alongside the cmd that
callers must defer to stop the goroutine and remove the credentials
file. All in-tree callers (clone, fetch, ls-remote, lfs snapshot) are
updated accordingly.

Amp-Thread-ID: https://ampcode.com/threads/T-019e805f-48b7-7594-9d46-415a44d2e1c5
Co-authored-by: Amp <amp@ampcode.com>
---
 internal/gitclone/command.go      | 178 +++++++++++++++++++---
 internal/gitclone/command_test.go | 244 ++++++++++++++++++++++++++----
 internal/gitclone/manager.go      |   9 +-
 internal/strategy/git/snapshot.go |   8 +-
 4 files changed, 382 insertions(+), 57 deletions(-)

diff --git a/internal/gitclone/command.go b/internal/gitclone/command.go
index 891b2314..786a3c25 100644
--- a/internal/gitclone/command.go
+++ b/internal/gitclone/command.go
@@ -5,48 +5,180 @@ package gitclone
 import (
 	"bufio"
 	"context"
+	"os"
 	"os/exec"
 	"strings"
+	"sync"
+	"time"
 
 	"github.com/alecthomas/errors"
+
+	"github.com/block/cachew/internal/logging"
 )
 
+// credentialFileRefreshInterval controls how often the background goroutine
+// re-fetches the GitHub App token and rewrites the on-disk credential file.
+// GitHub App installation tokens are valid for 1 hour and the TokenManager's
+// own cache refreshes ~5 minutes before expiry, so 30 s is dense enough to
+// pick up rotations within a window that git lfs will tolerate (it retries
+// failed batch requests several times with exponential backoff).
+const credentialFileRefreshInterval = 30 * time.Second
+
 // GitCommand returns a git subprocess configured with repository-scoped
 // authentication and any per-URL git config overrides disabled.
-func (r *Repository) GitCommand(ctx context.Context, args ...string) (*exec.Cmd, error) {
-	repoURL := r.upstreamURL
-	var token string
-	if r.credentialProvider != nil && strings.Contains(repoURL, "github.com") {
-		var err error
-		token, err = r.credentialProvider.GetTokenForURL(ctx, repoURL)
-		// If error getting token, fall back to original URL (system credentials)
-		if err != nil {
-			token = ""
+//
+// When a GitHub App credential provider is configured and the upstream is a
+// github.com URL, the token is written to a 0600 temporary file and exposed
+// to git via a credential.helper that reads the file on every invocation. A
+// background goroutine then refreshes the token in-place for the lifetime of
+// the returned cleanup, so long-running subprocesses (notably `git lfs fetch`
+// against large repos like ios-register / cash-ios) survive the 1-hour token
+// TTL instead of failing with "Bad credentials" once the embedded token
+// expires mid-fetch.
+//
+// Callers MUST invoke the returned cleanup (typically via defer) once the
+// command has finished, to stop the refresh goroutine and remove the token
+// file from disk. cleanup is always non-nil and is safe to call multiple
+// times, including when GitCommand returns an error.
+func (r *Repository) GitCommand(ctx context.Context, args ...string) (*exec.Cmd, func(), error) {
+	cleanup := func() {}
+
+	configArgs, err := getInsteadOfDisableArgsForURL(ctx, r.upstreamURL)
+	if err != nil {
+		return nil, cleanup, errors.Wrap(err, "get insteadOf disable args")
+	}
+
+	var allArgs []string
+	allArgs = append(allArgs, configArgs...)
+
+	if r.credentialProvider != nil && strings.Contains(r.upstreamURL, "github.com") {
+		token, err := r.credentialProvider.GetTokenForURL(ctx, r.upstreamURL)
+		if err == nil && token != "" {
+			credFile, fileCleanup, err := r.startTokenCredentialFile(ctx, token)
+			if err != nil {
+				return nil, cleanup, errors.Wrap(err, "start token credential file")
+			}
+			cleanup = fileCleanup
+			// `!<cmd>` tells git to run <cmd> via the shell on every credential
+			// query. `cat` re-reads the file each time, so a rewrite by the
+			// refresh goroutine is picked up automatically by git-lfs retries
+			// after a token rotation.
+			allArgs = append(allArgs, "-c", "credential.helper=!cat "+shellSingleQuote(credFile))
 		}
 	}
 
-	configArgs, err := getInsteadOfDisableArgsForURL(ctx, repoURL)
+	allArgs = append(allArgs, args...)
+
+	return exec.CommandContext(ctx, "git", allArgs...), cleanup, nil
+}
+
+// startTokenCredentialFile creates a 0600 temp file containing a git
+// credential helper response for the given initial token, and spawns a
+// background goroutine that re-fetches the token and rewrites the file as
+// long as the returned cleanup has not been called and ctx has not been
+// cancelled. The credential file path is returned so callers can wire it
+// into git's credential.helper config.
+func (r *Repository) startTokenCredentialFile(ctx context.Context, initialToken string) (string, func(), error) {
+	f, err := os.CreateTemp("", "cachew-git-cred-*")
 	if err != nil {
-		return nil, errors.Wrap(err, "get insteadOf disable args")
+		return "", func() {}, errors.Wrap(err, "create credential file")
+	}
+	// path is generated by os.CreateTemp under os.TempDir(); the file
+	// descriptor is owned by this process and never user-controlled.
+	path := f.Name()
+	if err := f.Close(); err != nil {
+		_ = os.Remove(path) //nolint:gosec // path is from os.CreateTemp
+		return "", func() {}, errors.Wrap(err, "close credential file")
+	}
+	if err := os.Chmod(path, 0o600); err != nil { //nolint:gosec // path is from os.CreateTemp
+		_ = os.Remove(path) //nolint:gosec // path is from os.CreateTemp
+		return "", func() {}, errors.Wrap(err, "chmod credential file")
+	}
+	if err := writeCredentialFile(path, initialToken); err != nil {
+		_ = os.Remove(path) //nolint:gosec // path is from os.CreateTemp
+		return "", func() {}, err
 	}
 
-	var allArgs []string
-	if len(configArgs) > 0 {
-		allArgs = append(allArgs, configArgs...)
+	refreshCtx, cancel := context.WithCancel(ctx)
+	var once sync.Once
+	cleanup := func() {
+		once.Do(func() {
+			cancel()
+			_ = os.Remove(path) //nolint:gosec // path is from os.CreateTemp
+		})
 	}
 
-	// Add credential helper configuration if we have a token
-	// This ensures git uses the GitHub App token for authentication
-	// for all operations (clone, fetch, remote update, etc.)
-	if token != "" {
-		escapedToken := strings.ReplaceAll(token, "'", "'\\''")
-		credHelper := "!f() { test \"$1\" = get && echo username=x-access-token && printf 'password=%s\\n' '" + escapedToken + "'; }; f"
-		allArgs = append(allArgs, "-c", "credential.helper="+credHelper)
+	go r.refreshCredentialFile(refreshCtx, path, initialToken)
+
+	return path, cleanup, nil
+}
+
+// refreshCredentialFile is the body of the token-refresh goroutine started by
+// startTokenCredentialFile. It calls refreshCredentialFileOnce on a ticker
+// and rewrites the credential file whenever the token changes. The
+// TokenManager caches tokens internally so most ticks are a cheap map
+// lookup; only the pre-expiry refresh actually hits the GitHub API.
+func (r *Repository) refreshCredentialFile(ctx context.Context, path, current string) {
+	logger := logging.FromContext(ctx).With("upstream", r.upstreamURL, "cred_file", path)
+	ticker := time.NewTicker(credentialFileRefreshInterval)
+	defer ticker.Stop()
+	for {
+		select {
+		case <-ctx.Done():
+			return
+		case <-ticker.C:
+			next, changed, err := r.refreshCredentialFileOnce(ctx, path, current)
+			switch {
+			case err != nil:
+				logger.WarnContext(ctx, "Failed to refresh git credential file", "error", err)
+			case changed:
+				logger.DebugContext(ctx, "Git credential file refreshed with rotated token")
+				current = next
+			}
+		}
 	}
+}
 
-	allArgs = append(allArgs, args...)
+// refreshCredentialFileOnce performs a single tick of the refresh loop: it
+// fetches the current token from the credential provider and, when the value
+// differs from current, atomically rewrites path. It returns the latest token
+// it observed, whether path was actually rewritten, and any error.
+func (r *Repository) refreshCredentialFileOnce(ctx context.Context, path, current string) (string, bool, error) {
+	token, err := r.credentialProvider.GetTokenForURL(ctx, r.upstreamURL)
+	if err != nil {
+		return current, false, errors.Wrap(err, "fetch token")
+	}
+	if token == "" || token == current {
+		return current, false, nil
+	}
+	if err := writeCredentialFile(path, token); err != nil {
+		return current, false, err
+	}
+	return token, true, nil
+}
+
+// writeCredentialFile atomically writes the git credential helper response
+// for the given token to path. The on-disk format mirrors what git expects
+// to read from a `get` query on stdout, so `credential.helper=!cat <path>`
+// satisfies the protocol without any shell-level templating of the token.
+func writeCredentialFile(path, token string) error {
+	body := "username=x-access-token\npassword=" + token + "\n"
+	tmp := path + ".new"
+	if err := os.WriteFile(tmp, []byte(body), 0o600); err != nil { //nolint:gosec // path derives from os.CreateTemp
+		return errors.Wrap(err, "write temp credential file")
+	}
+	if err := os.Rename(tmp, path); err != nil { //nolint:gosec // path derives from os.CreateTemp
+		_ = os.Remove(tmp) //nolint:gosec // path derives from os.CreateTemp
+		return errors.Wrap(err, "rename credential file")
+	}
+	return nil
+}
 
-	return exec.CommandContext(ctx, "git", allArgs...), nil
+// shellSingleQuote returns s safely wrapped in POSIX single quotes for
+// inclusion in a `sh -c` command line. Embedded single quotes are escaped
+// by closing the quoted segment, inserting an escaped quote, and reopening.
+func shellSingleQuote(s string) string {
+	return "'" + strings.ReplaceAll(s, "'", `'\''`) + "'"
 }
 
 func getInsteadOfDisableArgsForURL(ctx context.Context, targetURL string) ([]string, error) {
diff --git a/internal/gitclone/command_test.go b/internal/gitclone/command_test.go
index 6a4f21b6..685c2f03 100644
--- a/internal/gitclone/command_test.go
+++ b/internal/gitclone/command_test.go
@@ -2,12 +2,26 @@ package gitclone //nolint:testpackage // Internal functions need to be tested
 
 import (
 	"context"
+	"log/slog"
+	"os"
 	"strings"
+	"sync"
+	"sync/atomic"
 	"testing"
 
 	"github.com/alecthomas/assert/v2"
+
+	"github.com/block/cachew/internal/logging"
 )
 
+// testContext returns a context wired with a default slog logger so code paths
+// that call logging.FromContext (notably the credential refresh goroutine) do
+// not panic in tests.
+func testContext(t *testing.T) context.Context {
+	t.Helper()
+	return logging.ContextWithLogger(context.Background(), slog.Default())
+}
+
 func TestGetInsteadOfDisableArgsForURL(t *testing.T) {
 	ctx := context.Background()
 
@@ -51,14 +65,13 @@ func TestGitCommand(t *testing.T) {
 		credentialProvider: nil,
 	}
 
-	cmd, err := repo.GitCommand(ctx, "version")
+	cmd, cleanup, err := repo.GitCommand(ctx, "version")
 	assert.NoError(t, err)
+	t.Cleanup(cleanup)
 
 	assert.NotZero(t, cmd)
 	assert.True(t, len(cmd.Args) >= 2)
-	// First arg should be git binary path
 	assert.Equal(t, "git", cmd.Args[0])
-	// Last arg should be "version"
 	assert.Equal(t, "version", cmd.Args[len(cmd.Args)-1])
 }
 
@@ -70,8 +83,9 @@ func TestGitCommandWithEmptyURL(t *testing.T) {
 		credentialProvider: nil,
 	}
 
-	cmd, err := repo.GitCommand(ctx, "version")
+	cmd, cleanup, err := repo.GitCommand(ctx, "version")
 	assert.NoError(t, err)
+	t.Cleanup(cleanup)
 
 	assert.NotZero(t, cmd)
 	assert.Equal(t, "git", cmd.Args[0])
@@ -79,34 +93,42 @@ func TestGitCommandWithEmptyURL(t *testing.T) {
 }
 
 type mockCredentialProvider struct {
+	mu    sync.Mutex
 	token string
+	calls atomic.Int64
 	err   error
 }
 
 func (m *mockCredentialProvider) GetTokenForURL(_ context.Context, _ string) (string, error) {
+	m.calls.Add(1)
+	m.mu.Lock()
+	defer m.mu.Unlock()
 	return m.token, m.err
 }
 
+func (m *mockCredentialProvider) setToken(token string) {
+	m.mu.Lock()
+	defer m.mu.Unlock()
+	m.token = token
+}
+
 func TestGitCommandWithCredentialProvider(t *testing.T) {
-	ctx := context.Background()
+	ctx := testContext(t)
 
 	tests := []struct {
-		name          string
-		token         string
-		expectHelper  bool
-		expectedToken string
+		name         string
+		token        string
+		expectHelper bool
 	}{
 		{
-			name:          "WithValidToken",
-			token:         "ghp_test123456",
-			expectHelper:  true,
-			expectedToken: "ghp_test123456",
+			name:         "WithValidToken",
+			token:        "ghp_test123456",
+			expectHelper: true,
 		},
 		{
-			name:          "WithTokenContainingSingleQuote",
-			token:         "token'with'quotes",
-			expectHelper:  true,
-			expectedToken: "token'with'quotes",
+			name:         "WithTokenContainingSingleQuote",
+			token:        "token'with'quotes",
+			expectHelper: true,
 		},
 		{
 			name:         "WithEmptyToken",
@@ -124,23 +146,185 @@ func TestGitCommandWithCredentialProvider(t *testing.T) {
 				},
 			}
 
-			cmd, err := repo.GitCommand(ctx, "version")
+			cmd, cleanup, err := repo.GitCommand(ctx, "version")
 			assert.NoError(t, err)
 			assert.NotZero(t, cmd)
+			t.Cleanup(cleanup)
 
-			if tt.expectHelper {
-				found := false
-				for i, arg := range cmd.Args {
-					if arg == "-c" && i+1 < len(cmd.Args) {
-						if strings.Contains(cmd.Args[i+1], "credential.helper=") {
-							found = true
-							assert.True(t, strings.Contains(cmd.Args[i+1], "username=x-access-token"))
-							break
-						}
-					}
-				}
-				assert.True(t, found, "expected credential.helper to be configured")
+			helperArg := findCredentialHelperArg(cmd.Args)
+			if !tt.expectHelper {
+				assert.Equal(t, "", helperArg, "did not expect credential.helper")
+				return
 			}
+			assert.NotEqual(t, "", helperArg, "expected credential.helper to be configured")
+
+			// The helper must NOT contain the literal token (it must point at a
+			// file instead) — that is the whole point of the refresh fix.
+			assert.False(t, strings.Contains(helperArg, tt.token),
+				"credential.helper must not embed the token literal: %q", helperArg)
+
+			path := credentialFilePathFromHelper(t, helperArg)
+			contents, err := os.ReadFile(path)
+			assert.NoError(t, err)
+			assert.Equal(t,
+				"username=x-access-token\npassword="+tt.token+"\n",
+				string(contents),
+				"credential file should contain a complete git credential helper response")
+
+			info, err := os.Stat(path)
+			assert.NoError(t, err)
+			assert.Equal(t, os.FileMode(0o600), info.Mode().Perm(),
+				"credential file must be 0600 to avoid leaking tokens to other users")
 		})
 	}
 }
+
+// TestGitCommand_CleanupRemovesCredentialFile verifies that calling the
+// returned cleanup function removes the on-disk credential file so we do not
+// leak rotated tokens between commands.
+func TestGitCommand_CleanupRemovesCredentialFile(t *testing.T) {
+	repo := &Repository{
+		upstreamURL: "https://github.com/user/repo",
+		credentialProvider: &mockCredentialProvider{
+			token: "ghs_initial",
+		},
+	}
+
+	cmd, cleanup, err := repo.GitCommand(testContext(t), "version")
+	assert.NoError(t, err)
+	assert.NotZero(t, cmd)
+
+	helperArg := findCredentialHelperArg(cmd.Args)
+	path := credentialFilePathFromHelper(t, helperArg)
+	_, err = os.Stat(path)
+	assert.NoError(t, err, "credential file should exist before cleanup")
+
+	cleanup()
+	_, err = os.Stat(path)
+	assert.True(t, os.IsNotExist(err), "credential file should be removed by cleanup, got err=%v", err)
+
+	// Idempotency: calling cleanup twice must not panic or error.
+	cleanup()
+}
+
+// TestGitCommand_RefreshGoroutineUpdatesFile verifies that the background
+// refresh goroutine rewrites the credential file when the upstream token
+// rotates. Without this behavior a long-running `git lfs fetch` (which the
+// snapshot job spawns) would keep using a stale 1-hour token after the
+// TokenManager rotates it and fail with "Bad credentials" — the exact
+// production incident this change is fixing.
+func TestGitCommand_RefreshGoroutineUpdatesFile(t *testing.T) {
+	provider := &mockCredentialProvider{token: "ghs_initial"}
+	repo := &Repository{
+		upstreamURL:        "https://github.com/user/repo",
+		credentialProvider: provider,
+	}
+
+	// Tighten the refresh interval for the test by using
+	// startTokenCredentialFile directly instead of GitCommand, so we do not
+	// have to wait 30 seconds for the goroutine to tick.
+	ctx, cancel := context.WithCancel(testContext(t))
+	defer cancel()
+	path, cleanup, err := repo.startTokenCredentialFile(ctx, "ghs_initial")
+	assert.NoError(t, err)
+	t.Cleanup(cleanup)
+
+	provider.setToken("ghs_rotated")
+
+	// Drive the refresh loop manually for deterministic test timing rather
+	// than waiting for the production 30s ticker.
+	next, changed, err := repo.refreshCredentialFileOnce(ctx, path, "ghs_initial")
+	assert.NoError(t, err)
+	assert.True(t, changed, "refresh should detect the rotated token and rewrite the file")
+	assert.Equal(t, "ghs_rotated", next)
+
+	contents, err := os.ReadFile(path)
+	assert.NoError(t, err)
+	assert.Equal(t,
+		"username=x-access-token\npassword=ghs_rotated\n",
+		string(contents),
+		"credential file should reflect the rotated token")
+
+	// A subsequent tick with the same token must be a no-op so the goroutine
+	// does not churn the file on every cycle.
+	next2, changed2, err := repo.refreshCredentialFileOnce(ctx, path, "ghs_rotated")
+	assert.NoError(t, err)
+	assert.False(t, changed2)
+	assert.Equal(t, "ghs_rotated", next2)
+}
+
+// TestWriteCredentialFile_Atomic verifies the rename-based atomic-write
+// behavior: a reader concurrent with the rewrite should always see a complete
+// credential response, never a half-written file. This protects the git
+// credential helper from observing a partial token while the refresh
+// goroutine is updating the file.
+func TestWriteCredentialFile_Atomic(t *testing.T) {
+	f, err := os.CreateTemp(t.TempDir(), "cred-*")
+	assert.NoError(t, err)
+	path := f.Name()
+	_ = f.Close()
+
+	assert.NoError(t, writeCredentialFile(path, "ghs_one"))
+
+	stop := make(chan struct{})
+	go func() {
+		for {
+			select {
+			case <-stop:
+				return
+			default:
+				b, err := os.ReadFile(path)
+				if err != nil {
+					continue
+				}
+				s := string(b)
+				assert.True(t,
+					strings.HasPrefix(s, "username=x-access-token\npassword=") && strings.HasSuffix(s, "\n"),
+					"reader observed partial write: %q", s)
+			}
+		}
+	}()
+
+	for range 200 {
+		assert.NoError(t, writeCredentialFile(path, "ghs_rotated"))
+	}
+	close(stop)
+}
+
+func TestShellSingleQuote(t *testing.T) {
+	tests := []struct {
+		in, out string
+	}{
+		{"/tmp/foo", `'/tmp/foo'`},
+		{"/tmp/with space", `'/tmp/with space'`},
+		{"weird'name", `'weird'\''name'`},
+	}
+	for _, tt := range tests {
+		assert.Equal(t, tt.out, shellSingleQuote(tt.in))
+	}
+}
+
+// findCredentialHelperArg returns the value portion of the
+// `credential.helper=...` entry in a git command's argv, or empty string if
+// none is present.
+func findCredentialHelperArg(args []string) string {
+	for i, a := range args {
+		if a == "-c" && i+1 < len(args) && strings.HasPrefix(args[i+1], "credential.helper=") {
+			return strings.TrimPrefix(args[i+1], "credential.helper=")
+		}
+	}
+	return ""
+}
+
+// credentialFilePathFromHelper extracts the filesystem path from a
+// `!cat '...'` credential helper expression and fails the test if it cannot.
+func credentialFilePathFromHelper(t *testing.T, helper string) string {
+	t.Helper()
+	const prefix = "!cat '"
+	const suffix = "'"
+	assert.True(t, strings.HasPrefix(helper, prefix), "unexpected helper format: %q", helper)
+	assert.True(t, strings.HasSuffix(helper, suffix), "unexpected helper format: %q", helper)
+	path := strings.TrimSuffix(strings.TrimPrefix(helper, prefix), suffix)
+	path = strings.ReplaceAll(path, `'\''`, `'`)
+	return path
+}
diff --git a/internal/gitclone/manager.go b/internal/gitclone/manager.go
index 90a2bb32..5b7c9aa5 100644
--- a/internal/gitclone/manager.go
+++ b/internal/gitclone/manager.go
@@ -524,10 +524,11 @@ func (r *Repository) executeClone(ctx context.Context) error {
 		r.upstreamURL, cloneDest,
 	}
 
-	cmd, err := r.GitCommand(cloneCtx, args...)
+	cmd, cleanup, err := r.GitCommand(cloneCtx, args...)
 	if err != nil {
 		return errors.Wrap(err, "create git command")
 	}
+	defer cleanup()
 	cmd.SysProcAttr = &syscall.SysProcAttr{Setpgid: true}
 	cmd.Cancel = func() error {
 		return syscall.Kill(-cmd.Process.Pid, syscall.SIGKILL)
@@ -602,10 +603,11 @@ func (r *Repository) fetchInternal(ctx context.Context, timeout time.Duration, e
 	}
 	args = append(args, "fetch", "--prune", "--prune-tags")
 
-	cmd, err := r.GitCommand(fetchCtx, args...)
+	cmd, cleanup, err := r.GitCommand(fetchCtx, args...)
 	if err != nil {
 		return errors.Wrap(err, "create git command")
 	}
+	defer cleanup()
 	// Start the process in its own process group so we can kill the entire
 	// tree (git spawns child processes like git-remote-https that inherit
 	// stdout/stderr pipes and prevent CombinedOutput from returning).
@@ -786,10 +788,11 @@ func (r *Repository) GetLocalRefs(ctx context.Context) (map[string]string, error
 
 func (r *Repository) GetUpstreamRefs(ctx context.Context) (map[string]string, error) {
 	// #nosec G204 - r.upstreamURL is controlled by us
-	cmd, err := r.GitCommand(ctx, "ls-remote", r.upstreamURL)
+	cmd, cleanup, err := r.GitCommand(ctx, "ls-remote", r.upstreamURL)
 	if err != nil {
 		return nil, errors.Wrap(err, "create git command")
 	}
+	defer cleanup()
 	output, err := cmd.CombinedOutput()
 	if err != nil {
 		return nil, errors.Wrap(err, "git ls-remote")
diff --git a/internal/strategy/git/snapshot.go b/internal/strategy/git/snapshot.go
index 214a67fe..66b5ccb5 100644
--- a/internal/strategy/git/snapshot.go
+++ b/internal/strategy/git/snapshot.go
@@ -915,12 +915,18 @@ func (s *Strategy) generateAndUploadLFSSnapshot(ctx context.Context, repo *gitcl
 		}
 
 		// Fetch only the LFS objects referenced by HEAD (the default branch).
+		// LFS fetches for large repos (e.g. ios-register at ~34 GiB) routinely
+		// run longer than a GitHub App installation token's 1-hour TTL, which
+		// is why the cleanup-deferred refresh in GitCommand exists: without
+		// it, the embedded token expires mid-fetch and git-lfs retries with
+		// the stale credential for hours before giving up.
 		fetchStart := time.Now()
-		fetchCmd, err := repo.GitCommand(ctx, "-C", workDir, "lfs", "fetch", "origin", "HEAD")
+		fetchCmd, fetchCleanup, err := repo.GitCommand(ctx, "-C", workDir, "lfs", "fetch", "origin", "HEAD")
 		if err != nil {
 			s.metrics.recordLFSPhase(ctx, upstream, "fetch", "error", time.Since(fetchStart))
 			return errors.Wrap(err, "create git lfs fetch command")
 		}
+		defer fetchCleanup()
 		if output, err := fetchCmd.CombinedOutput(); err != nil {
 			s.metrics.recordLFSPhase(ctx, upstream, "fetch", "error", time.Since(fetchStart))
 			return errors.Wrapf(err, "git lfs fetch: %s", string(output))

From df1479d4fe88574e1f085354f254c0867c3252c8 Mon Sep 17 00:00:00 2001
From: Joel Robotham <jrobotham@squareup.com>
Date: Mon, 1 Jun 2026 10:13:48 +1000
Subject: [PATCH 2/7] review: tighten comments per AGENTS.md guidance

Drop doc comments that restate the code, retain only the why-not-what
explanations (helper file rotation contract, token-TTL rationale,
atomic-write invariant, etc.). Remove internal repository name from
the snapshot.go comment.

Amp-Thread-ID: https://ampcode.com/threads/T-019e805f-48b7-7594-9d46-415a44d2e1c5
Co-authored-by: Amp <amp@ampcode.com>
---
 internal/gitclone/command.go      | 58 ++++++++-----------------------
 internal/gitclone/command_test.go | 52 +++++++--------------------
 internal/strategy/git/snapshot.go |  8 ++---
 3 files changed, 30 insertions(+), 88 deletions(-)

diff --git a/internal/gitclone/command.go b/internal/gitclone/command.go
index 786a3c25..6c6e51a8 100644
--- a/internal/gitclone/command.go
+++ b/internal/gitclone/command.go
@@ -16,29 +16,17 @@ import (
 	"github.com/block/cachew/internal/logging"
 )
 
-// credentialFileRefreshInterval controls how often the background goroutine
-// re-fetches the GitHub App token and rewrites the on-disk credential file.
-// GitHub App installation tokens are valid for 1 hour and the TokenManager's
-// own cache refreshes ~5 minutes before expiry, so 30 s is dense enough to
-// pick up rotations within a window that git lfs will tolerate (it retries
-// failed batch requests several times with exponential backoff).
+// credentialFileRefreshInterval is short enough that any rotation by
+// TokenManager (which refreshes ~5 min before the 1 h token expiry) is
+// reflected on disk before git-lfs exhausts its retry budget on a stale
+// token.
 const credentialFileRefreshInterval = 30 * time.Second
 
 // GitCommand returns a git subprocess configured with repository-scoped
 // authentication and any per-URL git config overrides disabled.
 //
-// When a GitHub App credential provider is configured and the upstream is a
-// github.com URL, the token is written to a 0600 temporary file and exposed
-// to git via a credential.helper that reads the file on every invocation. A
-// background goroutine then refreshes the token in-place for the lifetime of
-// the returned cleanup, so long-running subprocesses (notably `git lfs fetch`
-// against large repos like ios-register / cash-ios) survive the 1-hour token
-// TTL instead of failing with "Bad credentials" once the embedded token
-// expires mid-fetch.
-//
 // Callers MUST invoke the returned cleanup (typically via defer) once the
-// command has finished, to stop the refresh goroutine and remove the token
-// file from disk. cleanup is always non-nil and is safe to call multiple
+// command has finished. cleanup is always non-nil and safe to call multiple
 // times, including when GitCommand returns an error.
 func (r *Repository) GitCommand(ctx context.Context, args ...string) (*exec.Cmd, func(), error) {
 	cleanup := func() {}
@@ -59,10 +47,10 @@ func (r *Repository) GitCommand(ctx context.Context, args ...string) (*exec.Cmd,
 				return nil, cleanup, errors.Wrap(err, "start token credential file")
 			}
 			cleanup = fileCleanup
-			// `!<cmd>` tells git to run <cmd> via the shell on every credential
-			// query. `cat` re-reads the file each time, so a rewrite by the
-			// refresh goroutine is picked up automatically by git-lfs retries
-			// after a token rotation.
+			// `!cmd` runs cmd via the shell on every credential query, so a
+			// rewrite of credFile by the refresh goroutine is picked up on
+			// the next git-lfs retry — which is the whole point: long-running
+			// subprocesses can't otherwise observe a token rotation.
 			allArgs = append(allArgs, "-c", "credential.helper=!cat "+shellSingleQuote(credFile))
 		}
 	}
@@ -73,18 +61,14 @@ func (r *Repository) GitCommand(ctx context.Context, args ...string) (*exec.Cmd,
 }
 
 // startTokenCredentialFile creates a 0600 temp file containing a git
-// credential helper response for the given initial token, and spawns a
-// background goroutine that re-fetches the token and rewrites the file as
-// long as the returned cleanup has not been called and ctx has not been
-// cancelled. The credential file path is returned so callers can wire it
-// into git's credential.helper config.
+// credential helper response for the given initial token and spawns a
+// goroutine that rewrites it whenever the token rotates, until cleanup is
+// called or ctx is cancelled.
 func (r *Repository) startTokenCredentialFile(ctx context.Context, initialToken string) (string, func(), error) {
 	f, err := os.CreateTemp("", "cachew-git-cred-*")
 	if err != nil {
 		return "", func() {}, errors.Wrap(err, "create credential file")
 	}
-	// path is generated by os.CreateTemp under os.TempDir(); the file
-	// descriptor is owned by this process and never user-controlled.
 	path := f.Name()
 	if err := f.Close(); err != nil {
 		_ = os.Remove(path) //nolint:gosec // path is from os.CreateTemp
@@ -113,11 +97,6 @@ func (r *Repository) startTokenCredentialFile(ctx context.Context, initialToken
 	return path, cleanup, nil
 }
 
-// refreshCredentialFile is the body of the token-refresh goroutine started by
-// startTokenCredentialFile. It calls refreshCredentialFileOnce on a ticker
-// and rewrites the credential file whenever the token changes. The
-// TokenManager caches tokens internally so most ticks are a cheap map
-// lookup; only the pre-expiry refresh actually hits the GitHub API.
 func (r *Repository) refreshCredentialFile(ctx context.Context, path, current string) {
 	logger := logging.FromContext(ctx).With("upstream", r.upstreamURL, "cred_file", path)
 	ticker := time.NewTicker(credentialFileRefreshInterval)
@@ -139,10 +118,6 @@ func (r *Repository) refreshCredentialFile(ctx context.Context, path, current st
 	}
 }
 
-// refreshCredentialFileOnce performs a single tick of the refresh loop: it
-// fetches the current token from the credential provider and, when the value
-// differs from current, atomically rewrites path. It returns the latest token
-// it observed, whether path was actually rewritten, and any error.
 func (r *Repository) refreshCredentialFileOnce(ctx context.Context, path, current string) (string, bool, error) {
 	token, err := r.credentialProvider.GetTokenForURL(ctx, r.upstreamURL)
 	if err != nil {
@@ -158,9 +133,9 @@ func (r *Repository) refreshCredentialFileOnce(ctx context.Context, path, curren
 }
 
 // writeCredentialFile atomically writes the git credential helper response
-// for the given token to path. The on-disk format mirrors what git expects
-// to read from a `get` query on stdout, so `credential.helper=!cat <path>`
-// satisfies the protocol without any shell-level templating of the token.
+// for token to path. The on-disk format matches the helper protocol output
+// so that `credential.helper=!cat <path>` satisfies a `get` query without
+// any shell-level templating of the token value.
 func writeCredentialFile(path, token string) error {
 	body := "username=x-access-token\npassword=" + token + "\n"
 	tmp := path + ".new"
@@ -174,9 +149,6 @@ func writeCredentialFile(path, token string) error {
 	return nil
 }
 
-// shellSingleQuote returns s safely wrapped in POSIX single quotes for
-// inclusion in a `sh -c` command line. Embedded single quotes are escaped
-// by closing the quoted segment, inserting an escaped quote, and reopening.
 func shellSingleQuote(s string) string {
 	return "'" + strings.ReplaceAll(s, "'", `'\''`) + "'"
 }
diff --git a/internal/gitclone/command_test.go b/internal/gitclone/command_test.go
index 685c2f03..3e60b73c 100644
--- a/internal/gitclone/command_test.go
+++ b/internal/gitclone/command_test.go
@@ -14,9 +14,8 @@ import (
 	"github.com/block/cachew/internal/logging"
 )
 
-// testContext returns a context wired with a default slog logger so code paths
-// that call logging.FromContext (notably the credential refresh goroutine) do
-// not panic in tests.
+// testContext attaches a default slog logger so the credential refresh
+// goroutine does not panic in logging.FromContext.
 func testContext(t *testing.T) context.Context {
 	t.Helper()
 	return logging.ContextWithLogger(context.Background(), slog.Default())
@@ -158,8 +157,8 @@ func TestGitCommandWithCredentialProvider(t *testing.T) {
 			}
 			assert.NotEqual(t, "", helperArg, "expected credential.helper to be configured")
 
-			// The helper must NOT contain the literal token (it must point at a
-			// file instead) — that is the whole point of the refresh fix.
+			// The token must live in the file, not in the helper string,
+			// so a refresh can rotate it without restarting the subprocess.
 			assert.False(t, strings.Contains(helperArg, tt.token),
 				"credential.helper must not embed the token literal: %q", helperArg)
 
@@ -168,8 +167,7 @@ func TestGitCommandWithCredentialProvider(t *testing.T) {
 			assert.NoError(t, err)
 			assert.Equal(t,
 				"username=x-access-token\npassword="+tt.token+"\n",
-				string(contents),
-				"credential file should contain a complete git credential helper response")
+				string(contents))
 
 			info, err := os.Stat(path)
 			assert.NoError(t, err)
@@ -179,9 +177,6 @@ func TestGitCommandWithCredentialProvider(t *testing.T) {
 	}
 }
 
-// TestGitCommand_CleanupRemovesCredentialFile verifies that calling the
-// returned cleanup function removes the on-disk credential file so we do not
-// leak rotated tokens between commands.
 func TestGitCommand_CleanupRemovesCredentialFile(t *testing.T) {
 	repo := &Repository{
 		upstreamURL: "https://github.com/user/repo",
@@ -203,16 +198,9 @@ func TestGitCommand_CleanupRemovesCredentialFile(t *testing.T) {
 	_, err = os.Stat(path)
 	assert.True(t, os.IsNotExist(err), "credential file should be removed by cleanup, got err=%v", err)
 
-	// Idempotency: calling cleanup twice must not panic or error.
-	cleanup()
+	cleanup() // cleanup must be idempotent
 }
 
-// TestGitCommand_RefreshGoroutineUpdatesFile verifies that the background
-// refresh goroutine rewrites the credential file when the upstream token
-// rotates. Without this behavior a long-running `git lfs fetch` (which the
-// snapshot job spawns) would keep using a stale 1-hour token after the
-// TokenManager rotates it and fail with "Bad credentials" — the exact
-// production incident this change is fixing.
 func TestGitCommand_RefreshGoroutineUpdatesFile(t *testing.T) {
 	provider := &mockCredentialProvider{token: "ghs_initial"}
 	repo := &Repository{
@@ -220,9 +208,6 @@ func TestGitCommand_RefreshGoroutineUpdatesFile(t *testing.T) {
 		credentialProvider: provider,
 	}
 
-	// Tighten the refresh interval for the test by using
-	// startTokenCredentialFile directly instead of GitCommand, so we do not
-	// have to wait 30 seconds for the goroutine to tick.
 	ctx, cancel := context.WithCancel(testContext(t))
 	defer cancel()
 	path, cleanup, err := repo.startTokenCredentialFile(ctx, "ghs_initial")
@@ -231,33 +216,25 @@ func TestGitCommand_RefreshGoroutineUpdatesFile(t *testing.T) {
 
 	provider.setToken("ghs_rotated")
 
-	// Drive the refresh loop manually for deterministic test timing rather
-	// than waiting for the production 30s ticker.
+	// Drive one tick synchronously rather than waiting for the 30 s ticker.
 	next, changed, err := repo.refreshCredentialFileOnce(ctx, path, "ghs_initial")
 	assert.NoError(t, err)
-	assert.True(t, changed, "refresh should detect the rotated token and rewrite the file")
+	assert.True(t, changed)
 	assert.Equal(t, "ghs_rotated", next)
 
 	contents, err := os.ReadFile(path)
 	assert.NoError(t, err)
-	assert.Equal(t,
-		"username=x-access-token\npassword=ghs_rotated\n",
-		string(contents),
-		"credential file should reflect the rotated token")
+	assert.Equal(t, "username=x-access-token\npassword=ghs_rotated\n", string(contents))
 
-	// A subsequent tick with the same token must be a no-op so the goroutine
-	// does not churn the file on every cycle.
+	// A tick with the same token must not churn the file.
 	next2, changed2, err := repo.refreshCredentialFileOnce(ctx, path, "ghs_rotated")
 	assert.NoError(t, err)
 	assert.False(t, changed2)
 	assert.Equal(t, "ghs_rotated", next2)
 }
 
-// TestWriteCredentialFile_Atomic verifies the rename-based atomic-write
-// behavior: a reader concurrent with the rewrite should always see a complete
-// credential response, never a half-written file. This protects the git
-// credential helper from observing a partial token while the refresh
-// goroutine is updating the file.
+// TestWriteCredentialFile_Atomic guards against the git credential helper
+// observing a half-written file while the refresh goroutine is rotating it.
 func TestWriteCredentialFile_Atomic(t *testing.T) {
 	f, err := os.CreateTemp(t.TempDir(), "cred-*")
 	assert.NoError(t, err)
@@ -304,9 +281,6 @@ func TestShellSingleQuote(t *testing.T) {
 	}
 }
 
-// findCredentialHelperArg returns the value portion of the
-// `credential.helper=...` entry in a git command's argv, or empty string if
-// none is present.
 func findCredentialHelperArg(args []string) string {
 	for i, a := range args {
 		if a == "-c" && i+1 < len(args) && strings.HasPrefix(args[i+1], "credential.helper=") {
@@ -316,8 +290,6 @@ func findCredentialHelperArg(args []string) string {
 	return ""
 }
 
-// credentialFilePathFromHelper extracts the filesystem path from a
-// `!cat '...'` credential helper expression and fails the test if it cannot.
 func credentialFilePathFromHelper(t *testing.T, helper string) string {
 	t.Helper()
 	const prefix = "!cat '"
diff --git a/internal/strategy/git/snapshot.go b/internal/strategy/git/snapshot.go
index 66b5ccb5..078aee92 100644
--- a/internal/strategy/git/snapshot.go
+++ b/internal/strategy/git/snapshot.go
@@ -915,12 +915,10 @@ func (s *Strategy) generateAndUploadLFSSnapshot(ctx context.Context, repo *gitcl
 		}
 
 		// Fetch only the LFS objects referenced by HEAD (the default branch).
-		// LFS fetches for large repos (e.g. ios-register at ~34 GiB) routinely
-		// run longer than a GitHub App installation token's 1-hour TTL, which
-		// is why the cleanup-deferred refresh in GitCommand exists: without
-		// it, the embedded token expires mid-fetch and git-lfs retries with
-		// the stale credential for hours before giving up.
 		fetchStart := time.Now()
+		// fetchCleanup keeps the credential file refresh goroutine alive for
+		// the full fetch — these can exceed the GitHub App token's 1 h TTL on
+		// large LFS repos.
 		fetchCmd, fetchCleanup, err := repo.GitCommand(ctx, "-C", workDir, "lfs", "fetch", "origin", "HEAD")
 		if err != nil {
 			s.metrics.recordLFSPhase(ctx, upstream, "fetch", "error", time.Since(fetchStart))

From cc097e7a3ced6a4494fbdde1d0afb520c7109bf9 Mon Sep 17 00:00:00 2001
From: Joel Robotham <jrobotham@squareup.com>
Date: Mon, 1 Jun 2026 10:17:03 +1000
Subject: [PATCH 3/7] fix(gitclone): make credential helper ignore the action
 argument

Git appends the operation (`get`/`store`/`erase`) as a positional
argument to `!`-prefixed credential helpers. The previous
`!cat <credfile>` form therefore became `cat <credfile> get`, which
would also cat a file named `get` from the worktree (or `store`/
`erase`). Lines in that file are parsed by git as credential entries
and override the real token.

Wrap the helper in a shell function that only outputs on `get` and
absorbs the action argument. Add a regression test that drops hostile
`get`/`store`/`erase` files into a worktree and asserts
`git credential fill` returns the real token.

Reported by Codex on PR #321.

Amp-Thread-ID: https://ampcode.com/threads/T-019e805f-48b7-7594-9d46-415a44d2e1c5
Co-authored-by: Amp <amp@ampcode.com>
---
 internal/gitclone/command.go      |  9 +++++-
 internal/gitclone/command_test.go | 50 +++++++++++++++++++++++++++++--
 2 files changed, 56 insertions(+), 3 deletions(-)

diff --git a/internal/gitclone/command.go b/internal/gitclone/command.go
index 6c6e51a8..4613990d 100644
--- a/internal/gitclone/command.go
+++ b/internal/gitclone/command.go
@@ -51,7 +51,14 @@ func (r *Repository) GitCommand(ctx context.Context, args ...string) (*exec.Cmd,
 			// rewrite of credFile by the refresh goroutine is picked up on
 			// the next git-lfs retry — which is the whole point: long-running
 			// subprocesses can't otherwise observe a token rotation.
-			allArgs = append(allArgs, "-c", "credential.helper=!cat "+shellSingleQuote(credFile))
+			//
+			// Git appends the operation (`get`/`store`/`erase`) as a positional
+			// argument to the helper command. The function form here both gates
+			// on `get` and absorbs the argument, so a bare `cat <credfile>`
+			// can't be tricked into also reading a file named `get`/`store`/
+			// `erase` from the worktree.
+			allArgs = append(allArgs, "-c",
+				"credential.helper=!f() { test \"$1\" = get && cat "+shellSingleQuote(credFile)+"; }; f")
 		}
 	}
 
diff --git a/internal/gitclone/command_test.go b/internal/gitclone/command_test.go
index 3e60b73c..1e9852c5 100644
--- a/internal/gitclone/command_test.go
+++ b/internal/gitclone/command_test.go
@@ -4,6 +4,8 @@ import (
 	"context"
 	"log/slog"
 	"os"
+	"os/exec"
+	"path/filepath"
 	"strings"
 	"sync"
 	"sync/atomic"
@@ -233,6 +235,50 @@ func TestGitCommand_RefreshGoroutineUpdatesFile(t *testing.T) {
 	assert.Equal(t, "ghs_rotated", next2)
 }
 
+// TestGitCommand_HelperIgnoresHostileGetFile guards a real exploit: git
+// appends the credential operation (get/store/erase) as a positional
+// argument to `!`-prefixed helpers, so a bare `cat <credfile>` would also
+// `cat get` from the worktree and let a file named `get` override our token.
+// The helper must absorb that argument; this test exercises the full
+// invocation path through `git credential fill` to prove it does.
+func TestGitCommand_HelperIgnoresHostileGetFile(t *testing.T) {
+	if _, err := exec.LookPath("git"); err != nil {
+		t.Skip("git not on PATH")
+	}
+
+	repo := &Repository{
+		upstreamURL: "https://github.com/user/repo",
+		credentialProvider: &mockCredentialProvider{
+			token: "REAL_TOKEN",
+		},
+	}
+	cmd, cleanup, err := repo.GitCommand(testContext(t), "version")
+	assert.NoError(t, err)
+	t.Cleanup(cleanup)
+
+	helperArg := findCredentialHelperArg(cmd.Args)
+	assert.NotEqual(t, "", helperArg)
+
+	workDir := t.TempDir()
+	for _, op := range []string{"get", "store", "erase"} {
+		assert.NoError(t, os.WriteFile(filepath.Join(workDir, op),
+			[]byte("password=EVIL_TOKEN_VIA_"+op+"\n"), 0o600))
+	}
+
+	gitCmd := exec.Command("git", "-C", workDir,
+		"-c", "credential.helper=", // clear any inherited helpers
+		"-c", "credential.helper="+helperArg,
+		"credential", "fill",
+	)
+	gitCmd.Stdin = strings.NewReader("url=https://github.com/x/y\n\n")
+	out, err := gitCmd.Output()
+	assert.NoError(t, err)
+	assert.True(t, strings.Contains(string(out), "password=REAL_TOKEN\n"),
+		"expected REAL_TOKEN in helper output, got: %s", out)
+	assert.False(t, strings.Contains(string(out), "EVIL_TOKEN"),
+		"helper output must not include any worktree file content: %s", out)
+}
+
 // TestWriteCredentialFile_Atomic guards against the git credential helper
 // observing a half-written file while the refresh goroutine is rotating it.
 func TestWriteCredentialFile_Atomic(t *testing.T) {
@@ -292,8 +338,8 @@ func findCredentialHelperArg(args []string) string {
 
 func credentialFilePathFromHelper(t *testing.T, helper string) string {
 	t.Helper()
-	const prefix = "!cat '"
-	const suffix = "'"
+	const prefix = `!f() { test "$1" = get && cat '`
+	const suffix = `'; }; f`
 	assert.True(t, strings.HasPrefix(helper, prefix), "unexpected helper format: %q", helper)
 	assert.True(t, strings.HasSuffix(helper, suffix), "unexpected helper format: %q", helper)
 	path := strings.TrimSuffix(strings.TrimPrefix(helper, prefix), suffix)

From dda656c6b644c223f2b2d25522b88cd072fc28df Mon Sep 17 00:00:00 2001
From: Joel Robotham <jrobotham@squareup.com>
Date: Mon, 1 Jun 2026 10:25:14 +1000
Subject: [PATCH 4/7] security: avoid symlink-attackable tmp file; wait for
 refresh goroutine on cleanup

Two issues reported by Codex on PR #321.

(P1) writeCredentialFile previously wrote the rotated token to
`path + ".new"` and renamed it over the destination. Because path
lives in /tmp on a shared host, a hostile local user could pre-create
`<path>.new` as a symlink, and os.WriteFile would follow it, leaking
the next-rotated GitHub App token to attacker-readable storage. Switch
to os.CreateTemp (O_EXCL + random suffix) for the intermediate file so
the write target cannot be pre-positioned by anyone else.

(P2) cleanup previously cancelled the refresh context and immediately
removed the credential file. A refresh tick that began before the
cancel could finish its rename AFTER the cleanup-side os.Remove,
leaving a fresh token sitting in /tmp once the caller believed it had
been wiped. Track the refresh goroutine in a sync.WaitGroup so cleanup
blocks until it has fully exited before removing the file.

Adds two regression tests:

- TestWriteCredentialFile_IgnoresHostileSiblingSymlink plants the
  `<path>.new` symlink the old code would have followed and asserts
  the rotated write lands at path, not at the symlink target.
- TestCleanup_WaitsForInFlightRefresh shrinks the refresh interval to
  1ms, drives the production goroutine into a blocking provider, and
  asserts cleanup does not return until the goroutine has unwound.

credentialFileRefreshInterval becomes a var (with a nolint comment)
purely so the second test can shrink it to ms granularity.

Amp-Thread-ID: https://ampcode.com/threads/T-019e805f-48b7-7594-9d46-415a44d2e1c5
Co-authored-by: Amp <amp@ampcode.com>
---
 internal/gitclone/command.go      | 42 +++++++++++---
 internal/gitclone/command_test.go | 92 +++++++++++++++++++++++++++++++
 2 files changed, 125 insertions(+), 9 deletions(-)

diff --git a/internal/gitclone/command.go b/internal/gitclone/command.go
index 4613990d..6ebca4de 100644
--- a/internal/gitclone/command.go
+++ b/internal/gitclone/command.go
@@ -7,6 +7,7 @@ import (
 	"context"
 	"os"
 	"os/exec"
+	"path/filepath"
 	"strings"
 	"sync"
 	"time"
@@ -19,8 +20,9 @@ import (
 // credentialFileRefreshInterval is short enough that any rotation by
 // TokenManager (which refreshes ~5 min before the 1 h token expiry) is
 // reflected on disk before git-lfs exhausts its retry budget on a stale
-// token.
-const credentialFileRefreshInterval = 30 * time.Second
+// token. It is a var (rather than a const) only so tests can shrink it to
+// drive the refresh goroutine deterministically.
+var credentialFileRefreshInterval = 30 * time.Second //nolint:gochecknoglobals // test seam
 
 // GitCommand returns a git subprocess configured with repository-scoped
 // authentication and any per-URL git config overrides disabled.
@@ -91,16 +93,22 @@ func (r *Repository) startTokenCredentialFile(ctx context.Context, initialToken
 	}
 
 	refreshCtx, cancel := context.WithCancel(ctx)
+	var wg sync.WaitGroup
+	wg.Go(func() { r.refreshCredentialFile(refreshCtx, path, initialToken) })
+
+	// cleanup waits for any in-flight refresh tick to finish before removing
+	// the file. Otherwise a tick that began before cancel could rename a new
+	// token into place AFTER cleanup deleted the old one, leaving a stray
+	// token file in /tmp.
 	var once sync.Once
 	cleanup := func() {
 		once.Do(func() {
 			cancel()
+			wg.Wait()
 			_ = os.Remove(path) //nolint:gosec // path is from os.CreateTemp
 		})
 	}
 
-	go r.refreshCredentialFile(refreshCtx, path, initialToken)
-
 	return path, cleanup, nil
 }
 
@@ -143,14 +151,30 @@ func (r *Repository) refreshCredentialFileOnce(ctx context.Context, path, curren
 // for token to path. The on-disk format matches the helper protocol output
 // so that `credential.helper=!cat <path>` satisfies a `get` query without
 // any shell-level templating of the token value.
+//
+// The intermediate file is created via os.CreateTemp (O_EXCL + random
+// suffix), not as a deterministic `path + ".new"` sibling — otherwise a
+// hostile local user on a shared host could pre-create that sibling as a
+// symlink and have os.WriteFile follow it, leaking the rotated token to
+// attacker-readable storage.
 func writeCredentialFile(path, token string) error {
-	body := "username=x-access-token\npassword=" + token + "\n"
-	tmp := path + ".new"
-	if err := os.WriteFile(tmp, []byte(body), 0o600); err != nil { //nolint:gosec // path derives from os.CreateTemp
+	body := []byte("username=x-access-token\npassword=" + token + "\n")
+	tmp, err := os.CreateTemp(filepath.Dir(path), filepath.Base(path)+".*")
+	if err != nil {
+		return errors.Wrap(err, "create temp credential file")
+	}
+	tmpPath := tmp.Name()
+	if _, err := tmp.Write(body); err != nil {
+		_ = tmp.Close()
+		_ = os.Remove(tmpPath) //nolint:gosec // tmpPath is from os.CreateTemp
 		return errors.Wrap(err, "write temp credential file")
 	}
-	if err := os.Rename(tmp, path); err != nil { //nolint:gosec // path derives from os.CreateTemp
-		_ = os.Remove(tmp) //nolint:gosec // path derives from os.CreateTemp
+	if err := tmp.Close(); err != nil {
+		_ = os.Remove(tmpPath) //nolint:gosec // tmpPath is from os.CreateTemp
+		return errors.Wrap(err, "close temp credential file")
+	}
+	if err := os.Rename(tmpPath, path); err != nil { //nolint:gosec // both paths are from os.CreateTemp
+		_ = os.Remove(tmpPath) //nolint:gosec // tmpPath is from os.CreateTemp
 		return errors.Wrap(err, "rename credential file")
 	}
 	return nil
diff --git a/internal/gitclone/command_test.go b/internal/gitclone/command_test.go
index 1e9852c5..b2089690 100644
--- a/internal/gitclone/command_test.go
+++ b/internal/gitclone/command_test.go
@@ -10,6 +10,7 @@ import (
 	"sync"
 	"sync/atomic"
 	"testing"
+	"time"
 
 	"github.com/alecthomas/assert/v2"
 
@@ -279,6 +280,97 @@ func TestGitCommand_HelperIgnoresHostileGetFile(t *testing.T) {
 		"helper output must not include any worktree file content: %s", out)
 }
 
+// TestWriteCredentialFile_IgnoresHostileSiblingSymlink ensures that a
+// pre-existing `<path>.new` symlink planted by a hostile local user can't
+// redirect the rotated token write to attacker-readable storage.
+func TestWriteCredentialFile_IgnoresHostileSiblingSymlink(t *testing.T) {
+	dir := t.TempDir()
+	path := filepath.Join(dir, "cred")
+	assert.NoError(t, writeCredentialFile(path, "ghs_initial"))
+
+	// Plant the symlink the old `path + ".new"` code would have followed.
+	sentinel := filepath.Join(dir, "attacker-readable")
+	assert.NoError(t, os.WriteFile(sentinel, []byte("unchanged"), 0o600))
+	assert.NoError(t, os.Symlink(sentinel, path+".new"))
+
+	assert.NoError(t, writeCredentialFile(path, "ghs_rotated"))
+
+	credBytes, err := os.ReadFile(path)
+	assert.NoError(t, err)
+	assert.Equal(t, "username=x-access-token\npassword=ghs_rotated\n", string(credBytes))
+
+	sentinelBytes, err := os.ReadFile(sentinel)
+	assert.NoError(t, err)
+	assert.Equal(t, "unchanged", string(sentinelBytes),
+		"writeCredentialFile must not follow a hostile <path>.new symlink")
+}
+
+// TestCleanup_WaitsForInFlightRefresh ensures cleanup blocks until the
+// refresh goroutine exits so it can't race a rename and leave a stray file
+// behind after the caller thinks the credentials have been wiped.
+func TestCleanup_WaitsForInFlightRefresh(t *testing.T) {
+	prev := credentialFileRefreshInterval
+	credentialFileRefreshInterval = time.Millisecond
+	t.Cleanup(func() { credentialFileRefreshInterval = prev })
+
+	release := make(chan struct{})
+	var releaseOnce sync.Once
+	releaseProvider := func() { releaseOnce.Do(func() { close(release) }) }
+	provider := &blockingCredentialProvider{
+		token:   "ghs_rotated",
+		release: release,
+		entered: make(chan struct{}),
+	}
+	repo := &Repository{
+		upstreamURL:        "https://github.com/user/repo",
+		credentialProvider: provider,
+	}
+
+	path, cleanup, err := repo.startTokenCredentialFile(testContext(t), "ghs_initial")
+	assert.NoError(t, err)
+	t.Cleanup(func() { releaseProvider(); cleanup() })
+
+	// Wait for the production refresh goroutine to enter the blocking
+	// provider, so we know cleanup will hit an in-flight tick.
+	<-provider.entered
+
+	cleanupReturned := make(chan struct{})
+	go func() {
+		cleanup()
+		close(cleanupReturned)
+	}()
+
+	// cleanup must NOT have returned yet — the goroutine is blocked inside
+	// GetTokenForURL and wg.Wait must hold cleanup until it unwinds.
+	select {
+	case <-cleanupReturned:
+		t.Fatal("cleanup returned while a refresh tick was still in flight")
+	case <-time.After(50 * time.Millisecond):
+	}
+
+	releaseProvider()
+	<-cleanupReturned
+
+	_, err = os.Stat(path)
+	assert.True(t, os.IsNotExist(err), "credential file should be removed after cleanup, got err=%v", err)
+}
+
+// blockingCredentialProvider blocks GetTokenForURL until release is closed.
+// entered is closed on the first call so tests can wait for the goroutine
+// to be inside the provider before exercising cancellation.
+type blockingCredentialProvider struct {
+	token       string
+	release     chan struct{}
+	entered     chan struct{}
+	enteredOnce sync.Once
+}
+
+func (p *blockingCredentialProvider) GetTokenForURL(_ context.Context, _ string) (string, error) {
+	p.enteredOnce.Do(func() { close(p.entered) })
+	<-p.release
+	return p.token, nil
+}
+
 // TestWriteCredentialFile_Atomic guards against the git credential helper
 // observing a half-written file while the refresh goroutine is rotating it.
 func TestWriteCredentialFile_Atomic(t *testing.T) {

From d5b50e39b0f8a8b6fd538b14fd9de7e81f20496e Mon Sep 17 00:00:00 2001
From: Joel Robotham <jrobotham@squareup.com>
Date: Mon, 1 Jun 2026 10:37:46 +1000
Subject: [PATCH 5/7] snapshot/lfs: tear down credential file as soon as lfs
 fetch returns

The deferred cleanup kept the credential helper file and its background
refresh goroutine alive through snapshot.CreatePaths archive upload,
which only touches the local cache. Scope the credential lifetime to
the subprocess that actually needs it.

Amp-Thread-ID: https://ampcode.com/threads/T-019e805f-48b7-7594-9d46-415a44d2e1c5
Co-authored-by: Amp <amp@ampcode.com>
---
 internal/strategy/git/snapshot.go | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/internal/strategy/git/snapshot.go b/internal/strategy/git/snapshot.go
index 078aee92..f0a57dd8 100644
--- a/internal/strategy/git/snapshot.go
+++ b/internal/strategy/git/snapshot.go
@@ -918,16 +918,19 @@ func (s *Strategy) generateAndUploadLFSSnapshot(ctx context.Context, repo *gitcl
 		fetchStart := time.Now()
 		// fetchCleanup keeps the credential file refresh goroutine alive for
 		// the full fetch — these can exceed the GitHub App token's 1 h TTL on
-		// large LFS repos.
+		// large LFS repos. We tear it down as soon as the subprocess returns
+		// so the token file doesn't outlive the command it's scoped to (the
+		// subsequent archive upload runs against the local cache, not GitHub).
 		fetchCmd, fetchCleanup, err := repo.GitCommand(ctx, "-C", workDir, "lfs", "fetch", "origin", "HEAD")
 		if err != nil {
 			s.metrics.recordLFSPhase(ctx, upstream, "fetch", "error", time.Since(fetchStart))
 			return errors.Wrap(err, "create git lfs fetch command")
 		}
-		defer fetchCleanup()
-		if output, err := fetchCmd.CombinedOutput(); err != nil {
+		fetchOutput, fetchErr := fetchCmd.CombinedOutput()
+		fetchCleanup()
+		if fetchErr != nil {
 			s.metrics.recordLFSPhase(ctx, upstream, "fetch", "error", time.Since(fetchStart))
-			return errors.Wrapf(err, "git lfs fetch: %s", string(output))
+			return errors.Wrapf(fetchErr, "git lfs fetch: %s", string(fetchOutput))
 		}
 		s.metrics.recordLFSPhase(ctx, upstream, "fetch", "success", time.Since(fetchStart))
 

From 188bbc22e8ffc1948119833a467c5493f2d22aef Mon Sep 17 00:00:00 2001
From: Joel Robotham <jrobotham@squareup.com>
Date: Mon, 1 Jun 2026 10:41:54 +1000
Subject: [PATCH 6/7] review: tighten writeCredentialFile doc comment

Amp-Thread-ID: https://ampcode.com/threads/T-019e805f-48b7-7594-9d46-415a44d2e1c5
Co-authored-by: Amp <amp@ampcode.com>
---
 internal/gitclone/command.go | 14 ++++----------
 1 file changed, 4 insertions(+), 10 deletions(-)

diff --git a/internal/gitclone/command.go b/internal/gitclone/command.go
index 6ebca4de..fa20f223 100644
--- a/internal/gitclone/command.go
+++ b/internal/gitclone/command.go
@@ -147,16 +147,10 @@ func (r *Repository) refreshCredentialFileOnce(ctx context.Context, path, curren
 	return token, true, nil
 }
 
-// writeCredentialFile atomically writes the git credential helper response
-// for token to path. The on-disk format matches the helper protocol output
-// so that `credential.helper=!cat <path>` satisfies a `get` query without
-// any shell-level templating of the token value.
-//
-// The intermediate file is created via os.CreateTemp (O_EXCL + random
-// suffix), not as a deterministic `path + ".new"` sibling — otherwise a
-// hostile local user on a shared host could pre-create that sibling as a
-// symlink and have os.WriteFile follow it, leaking the rotated token to
-// attacker-readable storage.
+// writeCredentialFile atomically rotates the git credential helper file at
+// path to contain token. The intermediate file uses os.CreateTemp rather
+// than a deterministic <path>.new sibling so a hostile local user can't
+// pre-plant a symlink there and redirect the rotated token write.
 func writeCredentialFile(path, token string) error {
 	body := []byte("username=x-access-token\npassword=" + token + "\n")
 	tmp, err := os.CreateTemp(filepath.Dir(path), filepath.Base(path)+".*")

From f7ebcbf492f25bbb502f3860cb6483d387fef602 Mon Sep 17 00:00:00 2001
From: Joel Robotham <jrobotham@squareup.com>
Date: Mon, 1 Jun 2026 10:49:09 +1000
Subject: [PATCH 7/7] review: dramatically trim comments; fix goroutine leak in
 atomic test

Amp-Thread-ID: https://ampcode.com/threads/T-019e805f-48b7-7594-9d46-415a44d2e1c5
Co-authored-by: Amp <amp@ampcode.com>
---
 internal/gitclone/command.go      | 47 +++++++++-------------------
 internal/gitclone/command_test.go | 52 +++++++++++--------------------
 internal/strategy/git/snapshot.go |  7 ++---
 3 files changed, 35 insertions(+), 71 deletions(-)

diff --git a/internal/gitclone/command.go b/internal/gitclone/command.go
index fa20f223..0958fdb9 100644
--- a/internal/gitclone/command.go
+++ b/internal/gitclone/command.go
@@ -17,19 +17,13 @@ import (
 	"github.com/block/cachew/internal/logging"
 )
 
-// credentialFileRefreshInterval is short enough that any rotation by
-// TokenManager (which refreshes ~5 min before the 1 h token expiry) is
-// reflected on disk before git-lfs exhausts its retry budget on a stale
-// token. It is a var (rather than a const) only so tests can shrink it to
-// drive the refresh goroutine deterministically.
+// credentialFileRefreshInterval beats the GitHub App token's 1 h TTL with
+// margin. Var (not const) so tests can shrink it.
 var credentialFileRefreshInterval = 30 * time.Second //nolint:gochecknoglobals // test seam
 
 // GitCommand returns a git subprocess configured with repository-scoped
-// authentication and any per-URL git config overrides disabled.
-//
-// Callers MUST invoke the returned cleanup (typically via defer) once the
-// command has finished. cleanup is always non-nil and safe to call multiple
-// times, including when GitCommand returns an error.
+// authentication. Callers MUST invoke cleanup (typically via defer) once
+// the command has finished. cleanup is always non-nil and idempotent.
 func (r *Repository) GitCommand(ctx context.Context, args ...string) (*exec.Cmd, func(), error) {
 	cleanup := func() {}
 
@@ -49,16 +43,10 @@ func (r *Repository) GitCommand(ctx context.Context, args ...string) (*exec.Cmd,
 				return nil, cleanup, errors.Wrap(err, "start token credential file")
 			}
 			cleanup = fileCleanup
-			// `!cmd` runs cmd via the shell on every credential query, so a
-			// rewrite of credFile by the refresh goroutine is picked up on
-			// the next git-lfs retry — which is the whole point: long-running
-			// subprocesses can't otherwise observe a token rotation.
-			//
-			// Git appends the operation (`get`/`store`/`erase`) as a positional
-			// argument to the helper command. The function form here both gates
-			// on `get` and absorbs the argument, so a bare `cat <credfile>`
-			// can't be tricked into also reading a file named `get`/`store`/
-			// `erase` from the worktree.
+			// Shell-form (`!cmd`) re-reads credFile on every credential query
+			// so refreshes take effect mid-subprocess. The f() wrapper gates
+			// on the op arg git appends (`get`/`store`/`erase`), so a worktree
+			// file named `get` can't be cat'd as a token.
 			allArgs = append(allArgs, "-c",
 				"credential.helper=!f() { test \"$1\" = get && cat "+shellSingleQuote(credFile)+"; }; f")
 		}
@@ -69,10 +57,8 @@ func (r *Repository) GitCommand(ctx context.Context, args ...string) (*exec.Cmd,
 	return exec.CommandContext(ctx, "git", allArgs...), cleanup, nil
 }
 
-// startTokenCredentialFile creates a 0600 temp file containing a git
-// credential helper response for the given initial token and spawns a
-// goroutine that rewrites it whenever the token rotates, until cleanup is
-// called or ctx is cancelled.
+// startTokenCredentialFile creates a 0600 credential file and spawns a
+// goroutine that rewrites it on token rotation until cleanup or ctx ends.
 func (r *Repository) startTokenCredentialFile(ctx context.Context, initialToken string) (string, func(), error) {
 	f, err := os.CreateTemp("", "cachew-git-cred-*")
 	if err != nil {
@@ -96,10 +82,8 @@ func (r *Repository) startTokenCredentialFile(ctx context.Context, initialToken
 	var wg sync.WaitGroup
 	wg.Go(func() { r.refreshCredentialFile(refreshCtx, path, initialToken) })
 
-	// cleanup waits for any in-flight refresh tick to finish before removing
-	// the file. Otherwise a tick that began before cancel could rename a new
-	// token into place AFTER cleanup deleted the old one, leaving a stray
-	// token file in /tmp.
+	// Wait for any in-flight refresh tick before removing the file so it
+	// can't rename a new token into place after we've deleted the old one.
 	var once sync.Once
 	cleanup := func() {
 		once.Do(func() {
@@ -147,10 +131,9 @@ func (r *Repository) refreshCredentialFileOnce(ctx context.Context, path, curren
 	return token, true, nil
 }
 
-// writeCredentialFile atomically rotates the git credential helper file at
-// path to contain token. The intermediate file uses os.CreateTemp rather
-// than a deterministic <path>.new sibling so a hostile local user can't
-// pre-plant a symlink there and redirect the rotated token write.
+// writeCredentialFile atomically rotates the credential file at path to
+// contain token. Uses os.CreateTemp (not a <path>.new sibling) so a planted
+// symlink can't redirect the write.
 func writeCredentialFile(path, token string) error {
 	body := []byte("username=x-access-token\npassword=" + token + "\n")
 	tmp, err := os.CreateTemp(filepath.Dir(path), filepath.Base(path)+".*")
diff --git a/internal/gitclone/command_test.go b/internal/gitclone/command_test.go
index b2089690..936ab642 100644
--- a/internal/gitclone/command_test.go
+++ b/internal/gitclone/command_test.go
@@ -17,8 +17,7 @@ import (
 	"github.com/block/cachew/internal/logging"
 )
 
-// testContext attaches a default slog logger so the credential refresh
-// goroutine does not panic in logging.FromContext.
+// testContext attaches a slog logger so refreshCredentialFile doesn't panic.
 func testContext(t *testing.T) context.Context {
 	t.Helper()
 	return logging.ContextWithLogger(context.Background(), slog.Default())
@@ -160,8 +159,8 @@ func TestGitCommandWithCredentialProvider(t *testing.T) {
 			}
 			assert.NotEqual(t, "", helperArg, "expected credential.helper to be configured")
 
-			// The token must live in the file, not in the helper string,
-			// so a refresh can rotate it without restarting the subprocess.
+			// Token must live in the file, not the helper string, so refresh
+			// can rotate it without restarting the subprocess.
 			assert.False(t, strings.Contains(helperArg, tt.token),
 				"credential.helper must not embed the token literal: %q", helperArg)
 
@@ -174,8 +173,7 @@ func TestGitCommandWithCredentialProvider(t *testing.T) {
 
 			info, err := os.Stat(path)
 			assert.NoError(t, err)
-			assert.Equal(t, os.FileMode(0o600), info.Mode().Perm(),
-				"credential file must be 0600 to avoid leaking tokens to other users")
+			assert.Equal(t, os.FileMode(0o600), info.Mode().Perm())
 		})
 	}
 }
@@ -219,7 +217,6 @@ func TestGitCommand_RefreshGoroutineUpdatesFile(t *testing.T) {
 
 	provider.setToken("ghs_rotated")
 
-	// Drive one tick synchronously rather than waiting for the 30 s ticker.
 	next, changed, err := repo.refreshCredentialFileOnce(ctx, path, "ghs_initial")
 	assert.NoError(t, err)
 	assert.True(t, changed)
@@ -229,19 +226,16 @@ func TestGitCommand_RefreshGoroutineUpdatesFile(t *testing.T) {
 	assert.NoError(t, err)
 	assert.Equal(t, "username=x-access-token\npassword=ghs_rotated\n", string(contents))
 
-	// A tick with the same token must not churn the file.
+	// Same token must not churn the file.
 	next2, changed2, err := repo.refreshCredentialFileOnce(ctx, path, "ghs_rotated")
 	assert.NoError(t, err)
 	assert.False(t, changed2)
 	assert.Equal(t, "ghs_rotated", next2)
 }
 
-// TestGitCommand_HelperIgnoresHostileGetFile guards a real exploit: git
-// appends the credential operation (get/store/erase) as a positional
-// argument to `!`-prefixed helpers, so a bare `cat <credfile>` would also
-// `cat get` from the worktree and let a file named `get` override our token.
-// The helper must absorb that argument; this test exercises the full
-// invocation path through `git credential fill` to prove it does.
+// Git appends the credential op (get/store/erase) as a positional arg to
+// `!`-helpers, so a bare `cat <credfile>` would also cat a worktree file
+// named `get`. The helper must absorb that argument.
 func TestGitCommand_HelperIgnoresHostileGetFile(t *testing.T) {
 	if _, err := exec.LookPath("git"); err != nil {
 		t.Skip("git not on PATH")
@@ -267,7 +261,7 @@ func TestGitCommand_HelperIgnoresHostileGetFile(t *testing.T) {
 	}
 
 	gitCmd := exec.Command("git", "-C", workDir,
-		"-c", "credential.helper=", // clear any inherited helpers
+		"-c", "credential.helper=", // empty value resets inherited helpers
 		"-c", "credential.helper="+helperArg,
 		"credential", "fill",
 	)
@@ -280,9 +274,6 @@ func TestGitCommand_HelperIgnoresHostileGetFile(t *testing.T) {
 		"helper output must not include any worktree file content: %s", out)
 }
 
-// TestWriteCredentialFile_IgnoresHostileSiblingSymlink ensures that a
-// pre-existing `<path>.new` symlink planted by a hostile local user can't
-// redirect the rotated token write to attacker-readable storage.
 func TestWriteCredentialFile_IgnoresHostileSiblingSymlink(t *testing.T) {
 	dir := t.TempDir()
 	path := filepath.Join(dir, "cred")
@@ -301,13 +292,9 @@ func TestWriteCredentialFile_IgnoresHostileSiblingSymlink(t *testing.T) {
 
 	sentinelBytes, err := os.ReadFile(sentinel)
 	assert.NoError(t, err)
-	assert.Equal(t, "unchanged", string(sentinelBytes),
-		"writeCredentialFile must not follow a hostile <path>.new symlink")
+	assert.Equal(t, "unchanged", string(sentinelBytes))
 }
 
-// TestCleanup_WaitsForInFlightRefresh ensures cleanup blocks until the
-// refresh goroutine exits so it can't race a rename and leave a stray file
-// behind after the caller thinks the credentials have been wiped.
 func TestCleanup_WaitsForInFlightRefresh(t *testing.T) {
 	prev := credentialFileRefreshInterval
 	credentialFileRefreshInterval = time.Millisecond
@@ -330,8 +317,6 @@ func TestCleanup_WaitsForInFlightRefresh(t *testing.T) {
 	assert.NoError(t, err)
 	t.Cleanup(func() { releaseProvider(); cleanup() })
 
-	// Wait for the production refresh goroutine to enter the blocking
-	// provider, so we know cleanup will hit an in-flight tick.
 	<-provider.entered
 
 	cleanupReturned := make(chan struct{})
@@ -340,8 +325,6 @@ func TestCleanup_WaitsForInFlightRefresh(t *testing.T) {
 		close(cleanupReturned)
 	}()
 
-	// cleanup must NOT have returned yet — the goroutine is blocked inside
-	// GetTokenForURL and wg.Wait must hold cleanup until it unwinds.
 	select {
 	case <-cleanupReturned:
 		t.Fatal("cleanup returned while a refresh tick was still in flight")
@@ -355,9 +338,8 @@ func TestCleanup_WaitsForInFlightRefresh(t *testing.T) {
 	assert.True(t, os.IsNotExist(err), "credential file should be removed after cleanup, got err=%v", err)
 }
 
-// blockingCredentialProvider blocks GetTokenForURL until release is closed.
-// entered is closed on the first call so tests can wait for the goroutine
-// to be inside the provider before exercising cancellation.
+// blockingCredentialProvider blocks GetTokenForURL until release is closed,
+// and closes entered on the first call.
 type blockingCredentialProvider struct {
 	token       string
 	release     chan struct{}
@@ -371,8 +353,8 @@ func (p *blockingCredentialProvider) GetTokenForURL(_ context.Context, _ string)
 	return p.token, nil
 }
 
-// TestWriteCredentialFile_Atomic guards against the git credential helper
-// observing a half-written file while the refresh goroutine is rotating it.
+// Concurrent rotation must never expose a partial credential file to the
+// helper running in parallel.
 func TestWriteCredentialFile_Atomic(t *testing.T) {
 	f, err := os.CreateTemp(t.TempDir(), "cred-*")
 	assert.NoError(t, err)
@@ -382,7 +364,8 @@ func TestWriteCredentialFile_Atomic(t *testing.T) {
 	assert.NoError(t, writeCredentialFile(path, "ghs_one"))
 
 	stop := make(chan struct{})
-	go func() {
+	var wg sync.WaitGroup
+	wg.Go(func() {
 		for {
 			select {
 			case <-stop:
@@ -398,12 +381,13 @@ func TestWriteCredentialFile_Atomic(t *testing.T) {
 					"reader observed partial write: %q", s)
 			}
 		}
-	}()
+	})
 
 	for range 200 {
 		assert.NoError(t, writeCredentialFile(path, "ghs_rotated"))
 	}
 	close(stop)
+	wg.Wait()
 }
 
 func TestShellSingleQuote(t *testing.T) {
diff --git a/internal/strategy/git/snapshot.go b/internal/strategy/git/snapshot.go
index f0a57dd8..914b63a6 100644
--- a/internal/strategy/git/snapshot.go
+++ b/internal/strategy/git/snapshot.go
@@ -915,12 +915,9 @@ func (s *Strategy) generateAndUploadLFSSnapshot(ctx context.Context, repo *gitcl
 		}
 
 		// Fetch only the LFS objects referenced by HEAD (the default branch).
+		// Cleanup runs as soon as the subprocess returns so the credential
+		// file doesn't outlive the only command that needs it.
 		fetchStart := time.Now()
-		// fetchCleanup keeps the credential file refresh goroutine alive for
-		// the full fetch — these can exceed the GitHub App token's 1 h TTL on
-		// large LFS repos. We tear it down as soon as the subprocess returns
-		// so the token file doesn't outlive the command it's scoped to (the
-		// subsequent archive upload runs against the local cache, not GitHub).
 		fetchCmd, fetchCleanup, err := repo.GitCommand(ctx, "-C", workDir, "lfs", "fetch", "origin", "HEAD")
 		if err != nil {
 			s.metrics.recordLFSPhase(ctx, upstream, "fetch", "error", time.Since(fetchStart))