fix symlink file handling and add tests for tarball create (#543)

* fix symlink file handling and add tests for tarball uploads

* add nanpa

Author: real-danm

.nanpa/agent-hosting-uploads.kdl

@@ -0,0 +1 @@
+minor type="fixed" "uploading issues with symlinks"

pkg/agentfs/tar.go

@@ -154,7 +154,7 @@ func UploadTarball(directory string, presignedUrl string, excludeFiles []string)
 			}
 		}
 
-		// Handle symlinks and get the real FileInfo if it's a symlink
+		// Follow symlinks and include the actual file contents
 		if info.Mode()&os.ModeSymlink != 0 {
 			realPath, err := filepath.EvalSymlinks(path)
 			if err != nil {
@@ -164,6 +164,28 @@ func UploadTarball(directory string, presignedUrl string, excludeFiles []string)
 			if err != nil {
 				return fmt.Errorf("failed to stat %s: %w", realPath, err)
 			}
+			// Open the real file instead of the symlink
+			file, err := os.Open(realPath)
+			if err != nil {
+				return fmt.Errorf("failed to open file %s: %w", realPath, err)
+			}
+			defer file.Close()
+
+			header, err := tar.FileInfoHeader(info, "")
+			if err != nil {
+				return fmt.Errorf("failed to create tar header for file %s: %w", path, err)
+			}
+			header.Name = relPath
+			if err := tarWriter.WriteHeader(header); err != nil {
+				return fmt.Errorf("failed to write tar header for file %s: %w", path, err)
+			}
+
+			// Copy file contents directly without progress bar
+			_, err = io.Copy(tarWriter, file)
+			if err != nil {
+				return fmt.Errorf("failed to copy file content for %s: %w", path, err)
+			}
+			return nil
 		}
 
 		// Handle directories

pkg/agentfs/tar_test.go

@@ -0,0 +1,221 @@
+package agentfs
+
+import (
+	"archive/tar"
+	"bytes"
+	"compress/gzip"
+	"io"
+	"net/http"
+	"net/http/httptest"
+	"os"
+	"path/filepath"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
+func TestUploadTarball(t *testing.T) {
+	tmpDir, err := os.MkdirTemp("", "tarball-test-*")
+	require.NoError(t, err)
+	defer os.RemoveAll(tmpDir)
+
+	subDir := filepath.Join(tmpDir, "subdir")
+	err = os.MkdirAll(subDir, 0755)
+	require.NoError(t, err)
+
+	normalFiles := []string{
+		filepath.Join(subDir, "normal1.txt"),
+		filepath.Join(subDir, "normal2.txt"),
+		filepath.Join(tmpDir, "root.txt"),
+	}
+	for _, path := range normalFiles {
+		err = os.WriteFile(path, []byte("normal content"), 0644)
+		require.NoError(t, err)
+	}
+
+	largeFile := filepath.Join(tmpDir, "large.bin")
+	f, err := os.Create(largeFile)
+	require.NoError(t, err)
+
+	fileSize := int64(1024 * 1024 * 1024) // 1GB
+	err = f.Truncate(fileSize)
+	require.NoError(t, err)
+	f.Close()
+
+	mockServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		w.WriteHeader(http.StatusOK)
+	}))
+	defer mockServer.Close()
+
+	err = UploadTarball(tmpDir, mockServer.URL, []string{})
+	require.NoError(t, err)
+}
+
+func TestUploadTarballFilePermissions(t *testing.T) {
+	tmpDir, err := os.MkdirTemp("", "tarball-test-*")
+	require.NoError(t, err)
+	defer os.RemoveAll(tmpDir)
+
+	subDir := filepath.Join(tmpDir, "subdir")
+	err = os.MkdirAll(subDir, 0755)
+	require.NoError(t, err)
+
+	normalFiles := []string{
+		filepath.Join(subDir, "normal1.txt"),
+		filepath.Join(subDir, "normal2.txt"),
+	}
+	for _, path := range normalFiles {
+		err = os.WriteFile(path, []byte("normal content"), 0644)
+		require.NoError(t, err)
+	}
+
+	restrictedFile := filepath.Join(subDir, "restricted.txt")
+	err = os.WriteFile(restrictedFile, []byte("restricted content"), 0000)
+	require.NoError(t, err)
+
+	mockServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		w.WriteHeader(http.StatusOK)
+	}))
+	defer mockServer.Close()
+
+	err = UploadTarball(tmpDir, mockServer.URL, []string{})
+	require.Error(t, err)
+	require.Contains(t, err.Error(), "permission denied")
+
+	err = os.Remove(restrictedFile)
+	require.NoError(t, err)
+
+	err = UploadTarball(tmpDir, mockServer.URL, []string{})
+	require.NoError(t, err)
+}
+
+type tarContents struct {
+	Name       string
+	Size       int64
+	IsDir      bool
+	IsSymlink  bool
+	LinkTarget string
+}
+
+func readTarContents(t *testing.T, tarData []byte) []tarContents {
+	t.Helper()
+	var contents []tarContents
+
+	gzipReader, err := gzip.NewReader(bytes.NewReader(tarData))
+	require.NoError(t, err)
+	defer gzipReader.Close()
+
+	tarReader := tar.NewReader(gzipReader)
+	for {
+		header, err := tarReader.Next()
+		if err == io.EOF {
+			break
+		}
+		require.NoError(t, err)
+
+		content := tarContents{
+			Name:       header.Name,
+			Size:       header.Size,
+			IsDir:      header.Typeflag == tar.TypeDir,
+			IsSymlink:  header.Typeflag == tar.TypeSymlink,
+			LinkTarget: header.Linkname,
+		}
+		contents = append(contents, content)
+	}
+	return contents
+}
+
+func TestUploadTarballDotfiles(t *testing.T) {
+	tmpDir, err := os.MkdirTemp("", "tarball-test-*")
+	require.NoError(t, err)
+	defer os.RemoveAll(tmpDir)
+
+	subDir := filepath.Join(tmpDir, "src")
+	err = os.MkdirAll(subDir, 0755)
+	require.NoError(t, err)
+
+	files := []struct {
+		path    string
+		content string
+	}{
+		{filepath.Join(tmpDir, "regular.txt"), "regular file"},
+		{filepath.Join(subDir, "code.go"), "package main"},
+	}
+	for _, f := range files {
+		err = os.WriteFile(f.path, []byte(f.content), 0644)
+		require.NoError(t, err)
+	}
+
+	dotfiles := []struct {
+		path    string
+		content string
+	}{
+		{filepath.Join(tmpDir, ".gitignore"), "*.env\nnode_modules/"},
+		{filepath.Join(tmpDir, ".env"), "SECRET=123"},
+		{filepath.Join(tmpDir, ".config"), "config data"},
+		{filepath.Join(subDir, ".DS_Store"), "mac file"},
+	}
+	for _, f := range dotfiles {
+		err = os.WriteFile(f.path, []byte(f.content), 0644)
+		require.NoError(t, err)
+	}
+
+	// symlinks
+	err = os.Symlink(files[0].path, filepath.Join(tmpDir, "link_to_regular.txt"))
+	require.NoError(t, err)
+	err = os.Symlink(dotfiles[2].path, filepath.Join(tmpDir, ".link_to_config"))
+	require.NoError(t, err)
+
+	nodeModules := filepath.Join(tmpDir, "node_modules")
+	err = os.MkdirAll(nodeModules, 0755)
+	require.NoError(t, err)
+	err = os.WriteFile(filepath.Join(nodeModules, "package.json"), []byte("{}"), 0644)
+	require.NoError(t, err)
+
+	var tarBuffer bytes.Buffer
+	mockServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		_, err := io.Copy(&tarBuffer, r.Body)
+		require.NoError(t, err)
+		w.WriteHeader(http.StatusOK)
+	}))
+	defer mockServer.Close()
+
+	err = UploadTarball(tmpDir, mockServer.URL, []string{})
+	require.NoError(t, err)
+
+	contents := readTarContents(t, tarBuffer.Bytes())
+
+	expectedFiles := map[string]struct{}{
+		"regular.txt":         {},
+		"src/code.go":         {},
+		".config":             {},
+		"link_to_regular.txt": {},
+		".link_to_config":     {},
+	}
+
+	excludedFiles := map[string]struct{}{
+		".env":          {},
+		".gitignore":    {},
+		"node_modules/": {},
+		".DS_Store":     {},
+	}
+
+	for _, content := range contents {
+		if _, ok := expectedFiles[content.Name]; ok {
+			delete(expectedFiles, content.Name)
+
+			switch content.Name {
+			case "link_to_regular.txt":
+				require.Equal(t, int64(len("regular file")), content.Size)
+				require.False(t, content.IsSymlink)
+			case ".link_to_config":
+				require.Equal(t, int64(len("config data")), content.Size)
+				require.False(t, content.IsSymlink)
+			}
+		}
+		_, shouldBeExcluded := excludedFiles[content.Name]
+		require.False(t, shouldBeExcluded, "excluded file was found in tar: %s", content.Name)
+	}
+
+	require.Empty(t, expectedFiles, "some expected files were not found in tar: %v", expectedFiles)
+}