From 515d27e27b3dd6fcf7bcd6459bcef0ce391631aa Mon Sep 17 00:00:00 2001 From: Djordje Lukic Date: Wed, 27 May 2026 08:09:40 +0000 Subject: [PATCH] fix(snapshot): scope git operations from worktree root When the agent ran from a subfolder of the git worktree, Repo.add() mixed worktree-relative paths from 'git diff-files --name-only' with cwd-relative paths from 'git ls-files --others', then passed them as pathspecs to 'git add' running from the subfolder. Worktree-relative entries were re-resolved against the subfolder cwd and failed with: warning: could not open directory 'assistant/assistant/' fatal: pathspec 'assistant/gogo.yaml' did not match any files Run all snapshot git invocations with cwd set to the worktree root and scope them with a worktree-relative pathspec via a new Repo.scope() helper. This makes git's input and output path conventions consistent regardless of where the agent is launched from. Also canonicalize r.directory with filepath.EvalSymlinks at Open() so it uses the same symlink resolution as 'git rev-parse --show-toplevel'. Without this, a symlinked entry path (e.g. /tmp -> /private/tmp on macOS) would make filepath.Rel produce a '..'-prefixed path and scope() silently expand to the entire worktree. Fixes #2903 --- pkg/snapshot/snapshot.go | 52 ++++++++++++++---- pkg/snapshot/snapshot_test.go | 100 ++++++++++++++++++++++++++++++++++ 2 files changed, 140 insertions(+), 12 deletions(-) diff --git a/pkg/snapshot/snapshot.go b/pkg/snapshot/snapshot.go index ad452c37b..4c006f2ac 100644 --- a/pkg/snapshot/snapshot.go +++ b/pkg/snapshot/snapshot.go @@ -73,6 +73,15 @@ func (m *Manager) Open(ctx context.Context, dir string) (*Repo, error) { if err != nil { return nil, err } + // Canonicalize symlinks so r.directory uses the same convention as the + // worktree path returned by `git rev-parse --show-toplevel` (which always + // resolves symlinks). Without this, on hosts where the working directory + // is reached through a symlink (e.g. /tmp -> /private/tmp on macOS), + // filepath.Rel(worktree, directory) would produce a ".."-prefixed path + // and scope() would silently expand to the entire worktree. + if resolved, err := filepath.EvalSymlinks(abs); err == nil { + abs = resolved + } worktree, err := gitWorktree(ctx, abs) if err != nil { return nil, err @@ -127,7 +136,7 @@ func (r *Repo) Track(ctx context.Context) (string, error) { if err := r.add(ctx); err != nil { return "", err } - result := r.git(ctx, r.args("write-tree"), gitOpts{cwd: r.directory}) + result := r.git(ctx, r.args("write-tree"), gitOpts{cwd: r.worktree}) if result.err != nil { return "", result.err } @@ -149,7 +158,7 @@ func (r *Repo) Patch(ctx context.Context, hash string) (Patch, error) { if err := r.add(ctx); err != nil { return Patch{Hash: hash}, err } - result := r.git(ctx, append(quoteArgs(), r.args("diff", "--cached", "--no-ext-diff", "--name-only", hash, "--", ".")...), gitOpts{cwd: r.directory}) + result := r.git(ctx, append(quoteArgs(), r.args(append([]string{"diff", "--cached", "--no-ext-diff", "--name-only", hash, "--"}, r.scope()...)...)...), gitOpts{cwd: r.worktree}) if result.err != nil { return Patch{Hash: hash}, result.err } @@ -179,7 +188,7 @@ func (r *Repo) Diff(ctx context.Context, hash string) (string, error) { if err := r.add(ctx); err != nil { return "", err } - result := r.git(ctx, append(quoteArgs(), r.args("diff", "--cached", "--no-ext-diff", hash, "--", ".")...), gitOpts{cwd: r.directory}) + result := r.git(ctx, append(quoteArgs(), r.args(append([]string{"diff", "--cached", "--no-ext-diff", hash, "--"}, r.scope()...)...)...), gitOpts{cwd: r.worktree}) if result.err != nil { return "", result.err } @@ -344,7 +353,7 @@ func (r *Repo) Cleanup(ctx context.Context) error { if _, err := os.Stat(r.gitdir); errors.Is(err, os.ErrNotExist) { return nil } - result := r.git(ctx, r.args("gc", "--prune="+pruneAfter), gitOpts{cwd: r.directory}) + result := r.git(ctx, r.args("gc", "--prune="+pruneAfter), gitOpts{cwd: r.worktree}) if result.err != nil { return result.err } @@ -361,7 +370,7 @@ func (r *Repo) DiffFull(ctx context.Context, from, to string) ([]FileDiff, error if err := r.ensure(ctx); err != nil { return nil, err } - statuses := r.git(ctx, append(quoteArgs(), r.args("diff", "--no-ext-diff", "--name-status", "--no-renames", from, to, "--", ".")...), gitOpts{cwd: r.directory}) + statuses := r.git(ctx, append(quoteArgs(), r.args(append([]string{"diff", "--no-ext-diff", "--name-status", "--no-renames", from, to, "--"}, r.scope()...)...)...), gitOpts{cwd: r.worktree}) if statuses.err != nil { return nil, statuses.err } @@ -384,7 +393,7 @@ func (r *Repo) DiffFull(ctx context.Context, from, to string) ([]FileDiff, error } } - numstat := r.git(ctx, append(quoteArgs(), r.args("diff", "--no-ext-diff", "--no-renames", "--numstat", from, to, "--", ".")...), gitOpts{cwd: r.directory}) + numstat := r.git(ctx, append(quoteArgs(), r.args(append([]string{"diff", "--no-ext-diff", "--no-renames", "--numstat", from, to, "--"}, r.scope()...)...)...), gitOpts{cwd: r.worktree}) if numstat.err != nil { return nil, numstat.err } @@ -424,7 +433,7 @@ func (r *Repo) DiffFull(ctx context.Context, from, to string) ([]FileDiff, error } patch := "" if !row.binary { - p := r.git(ctx, append(quoteArgs(), r.args("diff", "--no-ext-diff", "--no-renames", from, to, "--", row.file)...), gitOpts{cwd: r.directory}) + p := r.git(ctx, append(quoteArgs(), r.args("diff", "--no-ext-diff", "--no-renames", from, to, "--", row.file)...), gitOpts{cwd: r.worktree}) if p.err != nil { return nil, p.err } @@ -482,8 +491,12 @@ func (r *Repo) add(ctx context.Context) error { if err := r.syncExcludes(ctx, nil); err != nil { return err } - diff := r.git(ctx, append(quoteArgs(), r.args("diff-files", "--name-only", "-z", "--", ".")...), gitOpts{cwd: r.directory}) - other := r.git(ctx, append(quoteArgs(), r.args("ls-files", "--others", "--exclude-standard", "-z", "--", ".")...), gitOpts{cwd: r.directory}) + // Both git plumbing commands emit worktree-root-relative paths here: + // diff-files always does so by design, and ls-files is forced to do so + // via --full-name. Keeping the conventions aligned is what lets us merge + // the two lists and feed them back to git as pathspecs unchanged. + diff := r.git(ctx, append(quoteArgs(), r.args(append([]string{"diff-files", "--name-only", "-z", "--"}, r.scope()...)...)...), gitOpts{cwd: r.worktree}) + other := r.git(ctx, append(quoteArgs(), r.args(append([]string{"ls-files", "--others", "--exclude-standard", "-z", "--full-name", "--"}, r.scope()...)...)...), gitOpts{cwd: r.worktree}) if diff.err != nil { return diff.err } @@ -549,7 +562,7 @@ func (r *Repo) ignore(ctx context.Context, files []string) map[string]bool { if len(files) == 0 { return out } - result := command(ctx, []string{"-C", r.worktree, "-c", "core.quotepath=false", "check-ignore", "--no-index", "--stdin", "-z"}, gitOpts{cwd: r.directory, stdin: []byte(strings.Join(files, "\x00") + "\x00")}) + result := command(ctx, []string{"-C", r.worktree, "-c", "core.quotepath=false", "check-ignore", "--no-index", "--stdin", "-z"}, gitOpts{cwd: r.worktree, stdin: []byte(strings.Join(files, "\x00") + "\x00")}) if result.err != nil || (result.code != 0 && result.code != 1) { return out } @@ -563,7 +576,7 @@ func (r *Repo) drop(ctx context.Context, files []string) error { if len(files) == 0 { return nil } - result := r.git(ctx, append(cfgArgs(), r.args("rm", "--cached", "-f", "--ignore-unmatch", "--pathspec-from-file=-", "--pathspec-file-nul")...), gitOpts{cwd: r.directory, stdin: nulList(files)}) + result := r.git(ctx, append(cfgArgs(), r.args("rm", "--cached", "-f", "--ignore-unmatch", "--pathspec-from-file=-", "--pathspec-file-nul")...), gitOpts{cwd: r.worktree, stdin: nulList(files)}) if result.err != nil { return result.err } @@ -577,7 +590,7 @@ func (r *Repo) stage(ctx context.Context, files []string) error { if len(files) == 0 { return nil } - result := r.git(ctx, append(cfgArgs(), r.args("add", "--all", "--sparse", "--pathspec-from-file=-", "--pathspec-file-nul")...), gitOpts{cwd: r.directory, stdin: nulList(files)}) + result := r.git(ctx, append(cfgArgs(), r.args("add", "--all", "--sparse", "--pathspec-from-file=-", "--pathspec-file-nul")...), gitOpts{cwd: r.worktree, stdin: nulList(files)}) if result.err != nil { return result.err } @@ -623,6 +636,21 @@ func (r *Repo) args(cmd ...string) []string { return out } +// scope returns the pathspec(s) that restrict operations to the agent's +// working directory, expressed relative to the worktree root. Returning +// worktree-relative paths keeps git's output and input conventions aligned +// regardless of whether r.directory equals r.worktree or is a subdirectory. +func (r *Repo) scope() []string { + rel, err := filepath.Rel(r.worktree, r.directory) + if err != nil || rel == "" || rel == "." { + return []string{"."} + } + if strings.HasPrefix(rel, "..") { + return []string{"."} + } + return []string{filepath.ToSlash(rel)} +} + func (r *Repo) git(ctx context.Context, args []string, opts gitOpts) gitResult { return command(ctx, args, opts) } diff --git a/pkg/snapshot/snapshot_test.go b/pkg/snapshot/snapshot_test.go index 645e73f5e..2db3cea06 100644 --- a/pkg/snapshot/snapshot_test.go +++ b/pkg/snapshot/snapshot_test.go @@ -105,6 +105,106 @@ func TestOpenOutsideGitRepo(t *testing.T) { assert.ErrorIs(t, err, ErrNotGitRepository) } +// Regression test for https://github.com/docker/docker-agent/issues/2903: +// when the agent runs from a subfolder of the git worktree, snapshotting +// must succeed without git emitting "could not open directory" warnings or +// "pathspec did not match any files" errors caused by mixing cwd-relative +// and worktree-relative paths. +func TestTrackPatchFromSubfolder(t *testing.T) { + root := bootstrapRepo(t) + sub := filepath.Join(root, "assistant") + require.NoError(t, os.MkdirAll(sub, 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(sub, "gogo.yaml"), []byte("hello"), 0o644)) + runGit(t, root, "add", ".") + runGit(t, root, "commit", "-m", "add subfolder") + + repo := openRepo(t, sub) + assert.Equal(t, root, repo.Worktree()) + + before, err := repo.Track(t.Context()) + require.NoError(t, err) + require.NotEmpty(t, before) + + require.NoError(t, os.WriteFile(filepath.Join(sub, "gogo.yaml"), []byte("hello\nworld"), 0o644)) + require.NoError(t, os.WriteFile(filepath.Join(sub, "new.yaml"), []byte("new"), 0o644)) + + patch, err := repo.Patch(t.Context(), before) + require.NoError(t, err) + assert.ElementsMatch(t, []string{ + filepath.ToSlash(filepath.Join(repo.Worktree(), "assistant", "gogo.yaml")), + filepath.ToSlash(filepath.Join(repo.Worktree(), "assistant", "new.yaml")), + }, patch.Files) + + after, err := repo.Track(t.Context()) + require.NoError(t, err) + diffs, err := repo.DiffFull(t.Context(), before, after) + require.NoError(t, err) + assert.ElementsMatch(t, []string{"assistant/gogo.yaml", "assistant/new.yaml"}, diffFiles(diffs)) + + require.NoError(t, repo.Revert(t.Context(), []Patch{patch})) + assertFile(t, filepath.Join(sub, "gogo.yaml"), "hello") + assert.NoFileExists(t, filepath.Join(sub, "new.yaml")) +} + +// When the agent runs from a subfolder, changes outside that subfolder +// must not appear in patches scoped to the agent's directory. +func TestSubfolderScopeIgnoresSiblingChanges(t *testing.T) { + root := bootstrapRepo(t) + sub := filepath.Join(root, "assistant") + require.NoError(t, os.MkdirAll(sub, 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(sub, "gogo.yaml"), []byte("hello"), 0o644)) + runGit(t, root, "add", ".") + runGit(t, root, "commit", "-m", "add subfolder") + + repo := openRepo(t, sub) + before, err := repo.Track(t.Context()) + require.NoError(t, err) + + require.NoError(t, os.WriteFile(filepath.Join(sub, "gogo.yaml"), []byte("changed"), 0o644)) + require.NoError(t, os.WriteFile(filepath.Join(root, "a.txt"), []byte("sibling change"), 0o644)) + + patch, err := repo.Patch(t.Context(), before) + require.NoError(t, err) + assert.ElementsMatch(t, []string{ + filepath.ToSlash(filepath.Join(repo.Worktree(), "assistant", "gogo.yaml")), + }, patch.Files) +} + +// Regression test: when the agent's directory is reached through a symlink +// (e.g. /tmp -> /private/tmp on macOS), git rev-parse --show-toplevel +// returns the canonicalized path while filepath.Abs preserves the symlink. +// Without canonicalizing the directory at Open() time, filepath.Rel would +// produce a "..-prefixed path and scope() would silently expand to the +// entire worktree — defeating the subfolder-scoping fix. +func TestOpenCanonicalizesSymlinkedDirectory(t *testing.T) { + root := bootstrapRepo(t) + sub := filepath.Join(root, "assistant") + require.NoError(t, os.MkdirAll(sub, 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(sub, "gogo.yaml"), []byte("hello"), 0o644)) + runGit(t, root, "add", ".") + runGit(t, root, "commit", "-m", "add subfolder") + + link := filepath.Join(t.TempDir(), "link") + if err := os.Symlink(sub, link); err != nil { + t.Skipf("cannot create symlink: %v", err) + } + + repo := openRepo(t, link) + assert.Equal(t, sub, repo.Directory(), "Open must canonicalize the directory so it matches the worktree-relative scope") + + before, err := repo.Track(t.Context()) + require.NoError(t, err) + + require.NoError(t, os.WriteFile(filepath.Join(root, "a.txt"), []byte("sibling change"), 0o644)) + require.NoError(t, os.WriteFile(filepath.Join(sub, "new.yaml"), []byte("new"), 0o644)) + + patch, err := repo.Patch(t.Context(), before) + require.NoError(t, err) + assert.ElementsMatch(t, []string{ + filepath.ToSlash(filepath.Join(repo.Worktree(), "assistant", "new.yaml")), + }, patch.Files) +} + func bootstrapRepo(t *testing.T) string { t.Helper() if _, err := exec.LookPath("git"); err != nil {