From 08bd741fa43633286c1a1f2f47b30b1e5f07e702 Mon Sep 17 00:00:00 2001
From: Andy Lindeman <andy@lindeman.io>
Date: Mon, 4 May 2026 17:17:12 -0400
Subject: [PATCH] fix(helm): follow directory symlinks when hashing charts

sumFiles previously skipped directory symlinks, leaving the chart hash blind to content changes inside the symlink target. The new walker enqueues each directory symlink as a fresh filepath.Walk job with cycle detection on resolved real paths and a hard error on broken symlinks. Hash keys remain the as-seen path so existing exact-hash regression tests stay valid.

Fixes #35

Assisted-by: Opus 4.7 via Claude Code
---
 pkg/helm/helm.go                              | 144 +++++++++---------
 pkg/helm/helm_test.go                         | 111 ++++++++------
 .../testdata/broken_symlink_chart/dangling    |   1 +
 pkg/helm/testdata/cycle_chart/loop            |   1 +
 pkg/helm/testdata/sym_chart/Chart.yaml        |   4 +
 pkg/helm/testdata/sym_chart/templates/a.yaml  |   6 +
 pkg/helm/testdata/sym_chart/vendored          |   1 +
 .../testdata/sym_chart_expanded/Chart.yaml    |   4 +
 .../sym_chart_expanded/templates/a.yaml       |   6 +
 .../sym_chart_expanded/vendored/b.yaml        |   6 +
 pkg/helm/testdata/sym_chart_vendored/b.yaml   |   6 +
 11 files changed, 173 insertions(+), 117 deletions(-)
 create mode 120000 pkg/helm/testdata/broken_symlink_chart/dangling
 create mode 120000 pkg/helm/testdata/cycle_chart/loop
 create mode 100644 pkg/helm/testdata/sym_chart/Chart.yaml
 create mode 100644 pkg/helm/testdata/sym_chart/templates/a.yaml
 create mode 120000 pkg/helm/testdata/sym_chart/vendored
 create mode 100644 pkg/helm/testdata/sym_chart_expanded/Chart.yaml
 create mode 100644 pkg/helm/testdata/sym_chart_expanded/templates/a.yaml
 create mode 100644 pkg/helm/testdata/sym_chart_expanded/vendored/b.yaml
 create mode 100644 pkg/helm/testdata/sym_chart_vendored/b.yaml

diff --git a/pkg/helm/helm.go b/pkg/helm/helm.go
index 1bfe1e2..b994869 100644
--- a/pkg/helm/helm.go
+++ b/pkg/helm/helm.go
@@ -262,95 +262,93 @@ type result struct {
 	err  error
 }
 
-type nonRegularFile struct {
-	fileName string
-	isDir    bool
-}
-
-func resolvesTo(filePath string) (nonRegularFile, error) {
-	fileData := nonRegularFile{}
-	info, err := os.Lstat(filePath)
-	if err != nil {
-		return fileData, fmt.Errorf("failed to lstat file: %w", err)
-	}
-
-	if info.IsDir() {
-		fileData.fileName = filePath
-		fileData.isDir = true
-		return fileData, nil
-	}
-
-	if info.Mode()&fs.ModeSymlink != 0 {
-		fileName, err := os.Readlink(filePath)
-		if err != nil {
-			return fileData, fmt.Errorf("failed to follow symlink: %w", err)
-		}
-		fileName = strings.ReplaceAll(filePath, info.Name(), fileName)
-		fileData.fileName = fileName
-		fileInfo, err := os.Lstat(fileName)
-		if err != nil {
-			return fileData, fmt.Errorf("failed to lstat file: %w", err)
-		}
-		if fileInfo.IsDir() {
-			fileData.isDir = true
-			return fileData, nil
-		}
-	}
-	return fileData, nil
-}
-
 // sumFiles starts goroutines to walk the directory tree at root and digest each
 // regular file.  These goroutines send the results of the digests on the result
 // channel and send the result of the walk on the error channel.  If done is
 // closed, sumFiles abandons its work.
 func sumFiles(done <-chan struct{}, root string) (<-chan result, <-chan error) {
-	// For each regular file, start a goroutine that sums the file and sends
-	// the result on c.  Send the result of the walk on errc.
 	c := make(chan result)
 	errc := make(chan error, 1)
-	go func() { // HL
+	go func() {
 		var wg sync.WaitGroup
-		err := filepath.Walk(root, func(path string, info os.FileInfo, err error) error {
-			if err != nil {
-				return fmt.Errorf("error walking the file path %s: %w", root, err)
-			}
-			if !info.Mode().IsRegular() {
-				resolvedInfo, err := resolvesTo(root)
-				if err != nil {
-					return err
-				}
-				if resolvedInfo.isDir {
-					// TODO: figure out how to handle dirs and
-					// symlinked dirs
-					return nil
-				}
-				path = resolvedInfo.fileName
-			}
+
+		hashFile := func(keyPath, readPath string) {
 			wg.Add(1)
-			go func() { // HL
-				data, err := os.ReadFile(path)
+			go func() {
+				data, err := os.ReadFile(readPath)
 				select {
-				case c <- result{path, sha256.Sum256(data), err}: // HL
-				case <-done: // HL
+				case c <- result{keyPath, sha256.Sum256(data), err}:
+				case <-done:
 				}
 				wg.Done()
 			}()
-			// Abort the walk if done is closed.
-			select {
-			case <-done: // HL
-				return errors.New("walk canceled")
-			default:
+		}
+
+		// filepath.Walk does not descend into directory symlinks, so when
+		// one is encountered its resolved target is enqueued as its own
+		// walk job. The "logical" path (as seen through the symlink) is
+		// used for hash keys; the "physical" path (after resolution) is
+		// used for cycle detection.
+		type job struct{ logical, physical string }
+		visited := map[string]struct{}{filepath.Clean(root): {}}
+		queue := []job{{logical: root, physical: root}}
+
+		var walkErr error
+		for len(queue) > 0 && walkErr == nil {
+			j := queue[0]
+			queue = queue[1:]
+
+			walkErr = filepath.Walk(j.physical, func(path string, info os.FileInfo, err error) error {
+				if err != nil {
+					return err
+				}
+				select {
+				case <-done:
+					return errors.New("walk canceled")
+				default:
+				}
+
+				rel, err := filepath.Rel(j.physical, path)
+				if err != nil {
+					return err
+				}
+				logical := filepath.Join(j.logical, rel)
+
+				// For symlinks, follow once with Stat so symlink-to-file
+				// is treated like a regular file. Broken symlinks error
+				// here, propagating out and failing the hash.
+				target := info
+				if info.Mode()&fs.ModeSymlink != 0 {
+					target, err = os.Stat(path)
+					if err != nil {
+						return err
+					}
+				}
+
+				switch {
+				case info.Mode()&fs.ModeSymlink != 0 && target.IsDir():
+					resolved, err := filepath.EvalSymlinks(path)
+					if err != nil {
+						return err
+					}
+					if _, seen := visited[resolved]; seen {
+						log.Printf("sumFiles: skipping symlink cycle at %s -> %s", logical, resolved)
+						return nil
+					}
+					visited[resolved] = struct{}{}
+					queue = append(queue, job{logical: logical, physical: resolved})
+				case target.Mode().IsRegular():
+					hashFile(logical, path)
+				}
 				return nil
-			}
-		})
-		// Walk has returned, so all calls to wg.Add are done.  Start a
-		// goroutine to close c once all the sends are done.
-		go func() { // HL
+			})
+		}
+
+		go func() {
 			wg.Wait()
-			close(c) // HL
+			close(c)
 		}()
-		// No select needed here, since errc is buffered.
-		errc <- err // HL
+		errc <- walkErr
 	}()
 	return c, errc
 }
diff --git a/pkg/helm/helm_test.go b/pkg/helm/helm_test.go
index 7818cac..a8db360 100644
--- a/pkg/helm/helm_test.go
+++ b/pkg/helm/helm_test.go
@@ -5,10 +5,22 @@ import (
 	"errors"
 	"log"
 	"os"
+	"path/filepath"
+	"sort"
 	"strings"
 	"testing"
+	"time"
 )
 
+func mapKeys(m map[string][32]byte) []string {
+	out := make([]string, 0, len(m))
+	for k := range m {
+		out = append(out, k)
+	}
+	sort.Strings(out)
+	return out
+}
+
 func TestHelm(t *testing.T) {
 	// Set up tests to use current package's testdata as the working directory
 	oldWD, _ := os.Getwd()
@@ -250,54 +262,65 @@ kind: Application
 		}
 	})
 
-	t.Run("ResolvesTo", func(t *testing.T) {
-		scenarios := []struct {
-			name        string
-			expected    string
-			file        string
-			isDirectory bool
-		}{
-			{
-				name:        "symlinked file resolves to its target",
-				expected:    "crdData_override_testfile.yaml",
-				file:        "crdData_override_testfile_sym_link.yaml",
-				isDirectory: false,
-			},
-			{
-				name:        "regular directory returns own name",
-				expected:    "nonSymDir",
-				file:        "nonSymDir",
-				isDirectory: true,
-			},
-			{
-				name:        "symlinked directory returns its target",
-				expected:    "nonSymDir",
-				file:        "SymDir",
-				isDirectory: true,
-			},
-			/*
-				{
-					name:        "fail to find",
-					expected:    "nonSymDir",
-					file:        "phantom",
-					isDirectory: true,
-				},
-			*/
+	t.Run("SumFilesFollowsDirSymlink", func(t *testing.T) {
+		symMap, err := sha256Dir("sym_chart")
+		if err != nil {
+			t.Fatalf("sha256Dir(sym_chart): %v", err)
+		}
+		expandedMap, err := sha256Dir("sym_chart_expanded")
+		if err != nil {
+			t.Fatalf("sha256Dir(sym_chart_expanded): %v", err)
 		}
 
-		for _, tt := range scenarios {
-			t.Run(tt.name, func(t *testing.T) {
-				dataGot, err := resolvesTo(tt.file)
+		// Compare per-file content sums under matching relative paths.
+		// generalHashFunction folds the absolute key into its hash, so we
+		// can't just compare hex hashes - we have to strip the root prefix.
+		stripPrefix := func(m map[string][32]byte, prefix string) map[string][32]byte {
+			out := make(map[string][32]byte, len(m))
+			for k, v := range m {
+				rel, err := filepath.Rel(prefix, k)
 				if err != nil {
-					t.Errorf("failed to resolve file err: %v", err)
-				}
-				if dataGot.fileName != tt.expected {
-					t.Errorf("resolved files do not match. got: %s wanted: %s", dataGot.fileName, tt.expected)
-				}
-				if dataGot.isDir != tt.isDirectory {
-					t.Errorf("failed checking directory status. got: %t wanted: %t", dataGot.isDir, tt.isDirectory)
+					t.Fatalf("filepath.Rel(%q, %q): %v", prefix, k, err)
 				}
-			})
+				out[rel] = v
+			}
+			return out
+		}
+		got := stripPrefix(symMap, "sym_chart")
+		want := stripPrefix(expandedMap, "sym_chart_expanded")
+
+		gotKeys := mapKeys(got)
+		wantKeys := mapKeys(want)
+		if len(got) != len(want) {
+			t.Fatalf("file count mismatch: sym_chart=%v expanded=%v", gotKeys, wantKeys)
+		}
+		for k, v := range want {
+			if got[k] != v {
+				t.Errorf("hash mismatch at %s: sym_chart=%x expanded=%x", k, got[k], v)
+			}
+		}
+	})
+
+	t.Run("SumFilesHandlesSymlinkCycle", func(t *testing.T) {
+		done := make(chan error, 1)
+		go func() {
+			_, err := sha256Dir("cycle_chart")
+			done <- err
+		}()
+		select {
+		case err := <-done:
+			if err != nil {
+				t.Errorf("expected nil error for cycle_chart, got: %v", err)
+			}
+		case <-time.After(5 * time.Second):
+			t.Fatal("sha256Dir on cycle_chart did not terminate within 5s - cycle detection broken")
+		}
+	})
+
+	t.Run("SumFilesBrokenSymlinkErrors", func(t *testing.T) {
+		_, err := sha256Dir("broken_symlink_chart")
+		if err == nil {
+			t.Error("expected error for broken symlink, got nil")
 		}
 	})
 
diff --git a/pkg/helm/testdata/broken_symlink_chart/dangling b/pkg/helm/testdata/broken_symlink_chart/dangling
new file mode 120000
index 0000000..6e1fa95
--- /dev/null
+++ b/pkg/helm/testdata/broken_symlink_chart/dangling
@@ -0,0 +1 @@
+/nonexistent/path
\ No newline at end of file
diff --git a/pkg/helm/testdata/cycle_chart/loop b/pkg/helm/testdata/cycle_chart/loop
new file mode 120000
index 0000000..945c9b4
--- /dev/null
+++ b/pkg/helm/testdata/cycle_chart/loop
@@ -0,0 +1 @@
+.
\ No newline at end of file
diff --git a/pkg/helm/testdata/sym_chart/Chart.yaml b/pkg/helm/testdata/sym_chart/Chart.yaml
new file mode 100644
index 0000000..e2516fa
--- /dev/null
+++ b/pkg/helm/testdata/sym_chart/Chart.yaml
@@ -0,0 +1,4 @@
+apiVersion: v2
+name: sym-chart
+description: Fixture chart for sumFiles directory-symlink test
+version: 0.1.0
diff --git a/pkg/helm/testdata/sym_chart/templates/a.yaml b/pkg/helm/testdata/sym_chart/templates/a.yaml
new file mode 100644
index 0000000..5eccb96
--- /dev/null
+++ b/pkg/helm/testdata/sym_chart/templates/a.yaml
@@ -0,0 +1,6 @@
+apiVersion: v1
+kind: ConfigMap
+metadata:
+  name: a
+data:
+  key: a
diff --git a/pkg/helm/testdata/sym_chart/vendored b/pkg/helm/testdata/sym_chart/vendored
new file mode 120000
index 0000000..2b3f3da
--- /dev/null
+++ b/pkg/helm/testdata/sym_chart/vendored
@@ -0,0 +1 @@
+../sym_chart_vendored
\ No newline at end of file
diff --git a/pkg/helm/testdata/sym_chart_expanded/Chart.yaml b/pkg/helm/testdata/sym_chart_expanded/Chart.yaml
new file mode 100644
index 0000000..e2516fa
--- /dev/null
+++ b/pkg/helm/testdata/sym_chart_expanded/Chart.yaml
@@ -0,0 +1,4 @@
+apiVersion: v2
+name: sym-chart
+description: Fixture chart for sumFiles directory-symlink test
+version: 0.1.0
diff --git a/pkg/helm/testdata/sym_chart_expanded/templates/a.yaml b/pkg/helm/testdata/sym_chart_expanded/templates/a.yaml
new file mode 100644
index 0000000..5eccb96
--- /dev/null
+++ b/pkg/helm/testdata/sym_chart_expanded/templates/a.yaml
@@ -0,0 +1,6 @@
+apiVersion: v1
+kind: ConfigMap
+metadata:
+  name: a
+data:
+  key: a
diff --git a/pkg/helm/testdata/sym_chart_expanded/vendored/b.yaml b/pkg/helm/testdata/sym_chart_expanded/vendored/b.yaml
new file mode 100644
index 0000000..3f78e63
--- /dev/null
+++ b/pkg/helm/testdata/sym_chart_expanded/vendored/b.yaml
@@ -0,0 +1,6 @@
+apiVersion: v1
+kind: ConfigMap
+metadata:
+  name: b
+data:
+  key: b
diff --git a/pkg/helm/testdata/sym_chart_vendored/b.yaml b/pkg/helm/testdata/sym_chart_vendored/b.yaml
new file mode 100644
index 0000000..3f78e63
--- /dev/null
+++ b/pkg/helm/testdata/sym_chart_vendored/b.yaml
@@ -0,0 +1,6 @@
+apiVersion: v1
+kind: ConfigMap
+metadata:
+  name: b
+data:
+  key: b