air: reject code_source include_paths entries that match no files

.nextchanges/air/7643.md

@@ -0,0 +1 @@
+* `air run` now rejects a `code_source.include_paths` path that selects no files instead of uploading a snapshot without it; glob patterns and pathspec magic are left to git. ([#7643](https://github.com/databricks/cli/pull/7643))

cmd/air/snapshot_package.go

@@ -8,6 +8,7 @@ import (
 	"io"
 	"os"
 	"os/exec"
+	"path"
 	"path/filepath"
 	"strings"
 
@@ -168,13 +169,23 @@ func snapshotFiles(ctx context.Context, repoPath string, includePaths []string,
 		if name == ".git" || strings.HasPrefix(name, ".git/") || strings.HasPrefix(base, "._") {
 			continue
 		}
+		// Pathspec magic such as ":(top)x" or ":/x" can select files above root_path.
+		if !filepath.IsLocal(filepath.FromSlash(name)) {
+			return nil, fmt.Errorf("code_source include_paths select %q, which is outside root_path %s", name, filepath.ToSlash(repoPath))
+		}
 		info, err := os.Lstat(filepath.Join(repoPath, filepath.FromSlash(name)))
 		if errors.Is(err, os.ErrNotExist) {
 			continue
 		}
 		if err != nil {
 			return nil, fmt.Errorf("failed to inspect snapshot path %q: %w", name, err)
 		}
+		// git lists a nested repository or a submodule as a directory, which tarpack
+		// does not pack; keep the listing to what lands in the archive so the empty
+		// and include_paths checks below see the same thing.
+		if !info.Mode().IsRegular() && info.Mode()&os.ModeSymlink == 0 {
+			continue
+		}
 		linkTarget := ""
 		if info.Mode()&os.ModeSymlink != 0 {
 			linkTarget, err = os.Readlink(filepath.Join(repoPath, filepath.FromSlash(name)))
@@ -193,9 +204,45 @@ func snapshotFiles(ctx context.Context, repoPath string, includePaths []string,
 	if len(files) == 0 {
 		return nil, emptySnapshotError(ctx, repoPath, includePaths, isGitRepo)
 	}
+	if unmatched := unmatchedIncludePaths(includePaths, files); len(unmatched) > 0 {
+		return nil, fmt.Errorf("code_source include_paths select no files under %s: %s. Check that these paths exist under root_path and hold files git does not ignore%s; empty directories, nested git repositories and submodules add no files", filepath.ToSlash(repoPath), strings.Join(unmatched, ", "), ignoreRuleHint(isGitRepo))
+	}
 	return files, nil
 }
 
+// unmatchedIncludePaths returns the plain-path include_paths entries that select none
+// of files. git ls-files drops a pathspec that matches nothing, so a mistyped entry
+// would silently be missing from the snapshot. Entries git reads as patterns are left
+// to git, since a path-prefix comparison cannot evaluate them.
+func unmatchedIncludePaths(includePaths []string, files []snapshotFile) []string {
+	var unmatched []string
+	for _, p := range includePaths {
+		if isPathspecPattern(p) {
+			continue
+		}
+		prefix := path.Clean(filepath.ToSlash(p))
+		matched := prefix == "."
+		for _, file := range files {
+			rel := filepath.ToSlash(file.rel)
+			if rel == prefix || strings.HasPrefix(rel, prefix+"/") {
+				matched = true
+				break
+			}
+		}
+		if !matched {
+			unmatched = append(unmatched, p)
+		}
+	}
+	return unmatched
+}
+
+// isPathspecPattern reports whether git matches p as a pattern rather than as a
+// literal path: pathspec magic (":!x", ":(exclude)x", ":/x", ...) or a glob. git
+// treats a backslash as an escape; on Windows ToSlash turns it into a separator first.
+func isPathspecPattern(p string) bool {
+	return strings.HasPrefix(p, ":") || strings.ContainsAny(filepath.ToSlash(p), `*?[\`)
+}
+
 // emptySnapshotError explains why no file was selected for the snapshot. An empty
 // tarball is accepted by the upload but fails on the node, after compute was
 // provisioned, so it is rejected here with the most likely cause.
@@ -211,7 +258,16 @@ func emptySnapshotError(ctx context.Context, repoPath string, includePaths []str
 		}
 	}
 	if len(includePaths) > 0 {
-		return fmt.Errorf("%s: no file under include_paths (%s) is selected. Check that the paths exist under root_path and are not ignored by git (`git check-ignore -v <path>`)", prefix, strings.Join(includePaths, ", "))
+		return fmt.Errorf("%s: no file under include_paths (%s) is selected. Check that the paths exist under root_path and hold files git does not ignore%s; empty directories, nested git repositories and submodules add no files", prefix, strings.Join(includePaths, ", "), ignoreRuleHint(isGitRepo))
+	}
+	return fmt.Errorf("%s: the directory is empty, holds only nested git repositories or submodules, or all of its files are excluded by .gitignore rules%s. Check code_source.root_path", prefix, ignoreRuleHint(isGitRepo))
+}
+
+// ignoreRuleHint points at git's ignore-rule lookup, which only works inside a git
+// repository.
+func ignoreRuleHint(isGitRepo bool) string {
+	if !isGitRepo {
+		return ""
 	}
-	return fmt.Errorf("%s: the directory is empty or all of its files are excluded by .gitignore rules (`git check-ignore -v <path>` shows the rule for a file). Check code_source.root_path", prefix)
+	return " (`git check-ignore -v <file>` shows the rule for a file)"
 }

cmd/air/snapshot_package_test.go

@@ -6,6 +6,7 @@ import (
 	"context"
 	"os"
 	"path/filepath"
+	"runtime"
 	"slices"
 	"strings"
 	"testing"
@@ -324,3 +325,136 @@ func TestSnapshotFiles_RejectsIncludePathsMatchingNothing(t *testing.T) {
 	assert.Contains(t, err.Error(), "contains no files")
 	assert.Contains(t, err.Error(), "include_paths")
 }
+
+func TestSnapshotFiles_RejectsIncludePathMatchingNothingAlongsideOthers(t *testing.T) {
+	repo := t.TempDir()
+	writeRepoFile(t, repo, "ray_bootstrap.sh", "#!/bin/bash\n")
+	writeRepoFile(t, repo, "my-job/main.py", "print(1)\n")
+
+	_, err := snapshotFiles(t.Context(), repo, []string{"ray_bootstrap.sh", "my_job"}, false)
+	require.Error(t, err)
+	assert.Contains(t, err.Error(), "include_paths select no files")
+	assert.Contains(t, err.Error(), ": my_job.")
+	assert.NotContains(t, err.Error(), "ray_bootstrap.sh")
+}
+
+func TestSnapshotFiles_RejectsIncludePathWhoseFilesAreIgnored(t *testing.T) {
+	repo := newTestRepo(t)
+	writeRepoFile(t, repo, ".gitignore", "build/\n")
+	writeRepoFile(t, repo, "main.py", "print(1)\n")
+	writeRepoFile(t, repo, "build/out.bin", "x")
+
+	_, err := snapshotFiles(t.Context(), repo, []string{"main.py", "build"}, true)
+	require.Error(t, err)
+	assert.Contains(t, err.Error(), ": build.")
+	assert.Contains(t, err.Error(), "check-ignore -v")
+}
+
+func TestSnapshotFiles_AcceptsIncludePathForms(t *testing.T) {
+	repo := t.TempDir()
+	writeRepoFile(t, repo, "src/model.py", "print()")
+	writeRepoFile(t, repo, "README.md", "readme")
+	writeRepoFile(t, repo, "notes.txt", "notes")
+
+	files, err := snapshotFiles(t.Context(), repo, []string{"./src/", "README.md", "*.txt"}, false)
+	require.NoError(t, err)
+	var rels []string
+	for _, file := range files {
+		rels = append(rels, filepath.ToSlash(file.rel))
+	}
+	assert.ElementsMatch(t, []string{"src/model.py", "README.md", "notes.txt"}, rels)
+}
+
+func TestSnapshotFiles_IncludePathMatching(t *testing.T) {
+	tests := []struct {
+		name         string
+		includePaths []string
+		want         []string // selected files; nil when an error is expected
+		wantErr      string
+	}{
+		{name: "root", includePaths: []string{"."}, want: []string{"README.md", "b.txt", "data_v2/a.csv", "notes.txt", "src/model.py", "src/tests/test_model.py"}},
+		{name: "root with slash", includePaths: []string{"./"}, want: []string{"README.md", "b.txt", "data_v2/a.csv", "notes.txt", "src/model.py", "src/tests/test_model.py"}},
+		{name: "root and a file", includePaths: []string{".", "README.md"}, want: []string{"README.md", "b.txt", "data_v2/a.csv", "notes.txt", "src/model.py", "src/tests/test_model.py"}},
+		{name: "question mark glob", includePaths: []string{"note?.txt"}, want: []string{"notes.txt"}},
+		{name: "bracket glob", includePaths: []string{"[ab].txt"}, want: []string{"b.txt"}},
+		{name: "exclude magic", includePaths: []string{"src", ":!src/tests"}, want: []string{"src/model.py"}},
+		{name: "long exclude magic", includePaths: []string{"src", ":(exclude)src/tests"}, want: []string{"src/model.py"}},
+		{name: "prefix of another directory", includePaths: []string{"data_v2", "data"}, wantErr: ": data."},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			repo := t.TempDir()
+			for _, f := range []string{"README.md", "b.txt", "data_v2/a.csv", "notes.txt", "src/model.py", "src/tests/test_model.py"} {
+				writeRepoFile(t, repo, f, "x")
+			}
+
+			files, err := snapshotFiles(t.Context(), repo, tt.includePaths, false)
+			if tt.wantErr != "" {
+				require.Error(t, err)
+				assert.Contains(t, err.Error(), "include_paths select no files")
+				assert.Contains(t, err.Error(), tt.wantErr)
+				return
+			}
+			require.NoError(t, err)
+			var rels []string
+			for _, file := range files {
+				rels = append(rels, filepath.ToSlash(file.rel))
+			}
+			assert.ElementsMatch(t, tt.want, rels)
+		})
+	}
+}
+
+// git lists a nested repository as a single directory entry, which the tarball
+// writer skips, so it must not count as a match or as a file.
+func TestSnapshotFiles_NestedRepoSelectsNoFiles(t *testing.T) {
+	repo := t.TempDir()
+	writeRepoFile(t, repo, "train.py", "print(1)\n")
+	nested := filepath.Join(repo, "my_lib")
+	writeRepoFile(t, nested, "lib.py", "x = 1\n")
+	runGit(t, nested, "init", "-q")
+
+	_, err := snapshotFiles(t.Context(), repo, []string{"train.py", "my_lib"}, false)
+	require.Error(t, err)
+	assert.Contains(t, err.Error(), ": my_lib.")
+
+	_, err = snapshotFiles(t.Context(), repo, []string{"my_lib"}, false)
+	require.Error(t, err)
+	assert.Contains(t, err.Error(), "contains no files")
+}
+
+func TestSnapshotFiles_RejectsPathspecOutsideRoot(t *testing.T) {
+	repo := newTestRepo(t)
+	writeRepoFile(t, repo, "secret/key.txt", "s")
+	writeRepoFile(t, repo, "proj/main.py", "print(1)\n")
+	commitAll(t, repo, "init")
+
+	_, err := snapshotFiles(t.Context(), filepath.Join(repo, "proj"), []string{"main.py", ":(top)secret"}, true)
+	require.Error(t, err)
+	assert.Contains(t, err.Error(), "outside root_path")
+}
+
+// git reads a backslash in a pathspec as an escape, so the entry is a pattern and is
+// left to git. On Windows a backslash is a path separator instead.
+func TestSnapshotFiles_AcceptsBackslashEscapedIncludePath(t *testing.T) {
+	if runtime.GOOS == "windows" {
+		t.Skip("a backslash is a path separator on Windows")
+	}
+	repo := t.TempDir()
+	writeRepoFile(t, repo, "my file.txt", "x")
+	writeRepoFile(t, repo, "other.txt", "x")
+
+	files, err := snapshotFiles(t.Context(), repo, []string{`my\ file.txt`}, false)
+	require.NoError(t, err)
+	require.Len(t, files, 1)
+	assert.Equal(t, "my file.txt", filepath.ToSlash(files[0].rel))
+}
+
+func TestSnapshotFiles_OmitsIgnoreRuleHintOutsideGit(t *testing.T) {
+	repo := t.TempDir()
+	writeRepoFile(t, repo, "main.py", "print(1)\n")
+
+	_, err := snapshotFiles(t.Context(), repo, []string{"main.py", "missing"}, false)
+	require.Error(t, err)
+	assert.NotContains(t, err.Error(), "check-ignore")
+}