fix(cli): tolerate vanished entries during --force fresh-tree backup (#4475)

* fix(cli): tolerate vanished entries during --force fresh-tree backup Skip fs.ErrNotExist on children during CopyDir so a vanished mid-walk entry (Windows GetFileAttributesEx on a transient .gotmp) does not abort the --force pre-merge backup and revert implemented novels to scaffolds. Closes #4456 Co-authored-by: Trevin Chow <tmchow@users.noreply.github.com> * fix(cli): keep required copyFile targets and restore walk hooks copyFile no longer swallows a vanished source so patch records, legacy patch indexes, and research.json fail closed instead of omitting the destination. CopyDir still skips mid-walk children. SetCopyDirWalkHookForTest cleanup CAS-restores the previous hook so an earlier cleanup cannot clobber a later installation. Co-authored-by: Trevin Chow <tmchow@users.noreply.github.com> --------- Co-authored-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: Trevin Chow <tmchow@users.noreply.github.com>
2026-09-14 15:38:08 +08:00 · 2026-08-31 05:27:39 -07:00
parent c08d3bc274
commit 3a397e2d3e
3 changed files with 274 additions and 6 deletions
@@ -1,10 +1,12 @@
 package cli

 import (
+	"io/fs"
 	"os"
 	"path/filepath"
 	"testing"

+	"github.com/mvanhorn/cli-printing-press/v4/internal/pipeline"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
@@ -117,3 +119,33 @@ func TestFinalizeForceMergeReplacesRewrittenBodyOnVersionBumpWithoutBase(t *test
 	assert.Contains(t, string(novel), "NovelKeep")
 	require.NoError(t, compileGeneratedTree(freshDir))
 }
+
+func TestFinalizeForceMergeSkipsVanishedFreshTreeEntry(t *testing.T) {
+	dir := t.TempDir()
+	snapshotDir := filepath.Join(dir, "mini.preserve-1")
+	freshDir := filepath.Join(dir, "mini")
+
+	writeMiniModule(t, snapshotDir, map[string]string{
+		"internal/cli/novel.go": "package cli\n\nfunc NovelKeep() string { return \"implemented\" }\n",
+	})
+	writeMiniModule(t, freshDir, map[string]string{
+		"internal/cli/novel.go": "package cli\n\nfunc NovelKeep() string { return \"TODO scaffold\" }\n",
+	})
+	gotmp := filepath.Join(freshDir, ".gotmp")
+	require.NoError(t, os.MkdirAll(gotmp, 0o755))
+	require.NoError(t, os.WriteFile(filepath.Join(gotmp, "x"), []byte("tmp"), 0o644))
+
+	clear := pipeline.SetCopyDirWalkHookForTest(func(path string, _ fs.DirEntry) {
+		if path == gotmp {
+			require.NoError(t, os.RemoveAll(path))
+		}
+	})
+	t.Cleanup(clear)
+
+	require.NoError(t, finalizeForceMerge(snapshotDir, freshDir, nil, false, nil))
+
+	novel, err := os.ReadFile(filepath.Join(freshDir, "internal", "cli", "novel.go"))
+	require.NoError(t, err)
+	assert.Contains(t, string(novel), "implemented")
+	assert.NotContains(t, string(novel), "TODO scaffold")
+}
@@ -2,6 +2,7 @@ package pipeline

 import (
 	"encoding/json"
+	"errors"
 	"fmt"
 	"io"
 	"io/fs"
@@ -10,6 +11,7 @@ import (
 	"slices"
 	"sort"
 	"strings"
+	"sync/atomic"
 	"time"

 	"github.com/mvanhorn/cli-printing-press/v4/internal/graphql"
@@ -701,6 +703,43 @@ func pathHasComponent(path, component string) bool {
 	return slices.Contains(strings.Split(filepath.ToSlash(filepath.Clean(path)), "/"), component)
 }

+// A WalkDir child can disappear between enumeration and stat (Windows
+// GetFileAttributesEx on a vanished .gotmp during --force backup). Skip
+// that entry; keep real IO errors and a vanished source root fatal.
+func skipIfVanished(err error, isDir bool) error {
+	if err == nil || !errors.Is(err, fs.ErrNotExist) {
+		return err
+	}
+	if isDir {
+		return filepath.SkipDir
+	}
+	return nil
+}
+
+type copyDirWalkHook func(path string, d fs.DirEntry)
+
+var copyDirTestHook atomic.Pointer[copyDirWalkHook]
+
+func invokeCopyDirTestHook(path string, d fs.DirEntry) {
+	if p := copyDirTestHook.Load(); p != nil {
+		(*p)(path, d)
+	}
+}
+
+func SetCopyDirWalkHookForTest(fn func(path string, d fs.DirEntry)) func() {
+	prev := copyDirTestHook.Load()
+	var installed *copyDirWalkHook
+	if fn != nil {
+		hook := copyDirWalkHook(fn)
+		installed = &hook
+	}
+	copyDirTestHook.Store(installed)
+	return func() {
+		// Leave a later installation in place; restore only what this call set.
+		copyDirTestHook.CompareAndSwap(installed, prev)
+	}
+}
+
 func copyDirFiltered(src, dst string, skipFile func(path string, info fs.FileInfo) bool) error {
 	info, err := os.Stat(src)
 	if err != nil {
@@ -723,8 +762,12 @@ func copyDirFiltered(src, dst string, skipFile func(path string, info fs.FileInf
 	// callback sees them as symlink entries and we can validate them
 	// without descending into potentially huge or circular targets.
 	return filepath.WalkDir(src, func(path string, d fs.DirEntry, walkErr error) error {
+		invokeCopyDirTestHook(path, d)
 		if walkErr != nil {
-			return walkErr
+			if path == src {
+				return walkErr
+			}
+			return skipIfVanished(walkErr, d != nil && d.IsDir())
 		}
 		if path == src {
 			return nil
@@ -756,7 +799,7 @@ func copyDirFiltered(src, dst string, skipFile func(path string, info fs.FileInf
 		if d.Type()&os.ModeSymlink != 0 {
 			link, err := os.Readlink(path)
 			if err != nil {
-				return err
+				return skipIfVanished(err, false)
 			}
 			ok, err := symlinkTargetWithinRoot(srcRoot, path, link)
 			if err != nil {
@@ -768,7 +811,7 @@ func copyDirFiltered(src, dst string, skipFile func(path string, info fs.FileInf
 			if skipFile != nil {
 				info, err := d.Info()
 				if err != nil {
-					return err
+					return skipIfVanished(err, d.IsDir())
 				}
 				if skipFile(path, info) {
 					return nil
@@ -787,7 +830,7 @@ func copyDirFiltered(src, dst string, skipFile func(path string, info fs.FileInf
 		if d.IsDir() {
 			info, err := d.Info()
 			if err != nil {
-				return err
+				return skipIfVanished(err, true)
 			}
 			// A filtered copy may prune whole subtrees (e.g. downloaded
 			// third-party `sources/` in manuscripts). The unfiltered CopyDir
@@ -800,12 +843,15 @@ func copyDirFiltered(src, dst string, skipFile func(path string, info fs.FileInf

 		info, err := d.Info()
 		if err != nil {
-			return err
+			return skipIfVanished(err, false)
 		}
 		if skipFile != nil && skipFile(path, info) {
 			return nil
 		}
-		return copyFile(path, target, info.Mode())
+		if err := copyFile(path, target, info.Mode()); err != nil {
+			return skipIfVanished(err, false)
+		}
+		return nil
 	})
 }

@@ -2,6 +2,8 @@ package pipeline

 import (
 	"encoding/json"
+	"errors"
+	"io/fs"
 	"os"
 	"path/filepath"
 	"strings"
@@ -189,6 +191,194 @@ func TestCopyDirRejectsExternalSymlinks(t *testing.T) {
 	}
 }

+func installCopyDirTestHook(t *testing.T, fn copyDirWalkHook) {
+	t.Helper()
+	t.Cleanup(SetCopyDirWalkHookForTest(fn))
+}
+
+func TestSkipIfVanished(t *testing.T) {
+	t.Parallel()
+
+	denied := &os.PathError{Op: "stat", Path: "secret", Err: fs.ErrPermission}
+	windowsGone := &os.PathError{Op: "GetFileAttributesEx", Path: `\.gotmp`, Err: fs.ErrNotExist}
+
+	tests := []struct {
+		name  string
+		err   error
+		isDir bool
+		want  error
+	}{
+		{name: "nil"},
+		{name: "vanished file", err: fs.ErrNotExist},
+		{name: "vanished directory", err: fs.ErrNotExist, isDir: true, want: filepath.SkipDir},
+		{name: "windows GetFileAttributesEx", err: windowsGone, isDir: true, want: filepath.SkipDir},
+		{name: "permission denied", err: denied, isDir: true, want: denied},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+			got := skipIfVanished(tt.err, tt.isDir)
+			if tt.want == nil {
+				assert.NoError(t, got)
+				return
+			}
+			if errors.Is(tt.want, filepath.SkipDir) {
+				assert.ErrorIs(t, got, filepath.SkipDir)
+				return
+			}
+			assert.Equal(t, tt.want, got)
+		})
+	}
+}
+
+// backupFreshTree's --force pre-merge copy walks a live output tree. A child
+// that disappears between WalkDir enumeration and stat must not abort the copy.
+func TestCopyDirSkipsVanishedMidWalkEntry(t *testing.T) {
+	t.Run("vanished directory", func(t *testing.T) {
+		src := filepath.Join(t.TempDir(), "src")
+		dst := filepath.Join(t.TempDir(), "dst")
+		gotmp := filepath.Join(src, ".gotmp")
+		require.NoError(t, os.MkdirAll(filepath.Join(gotmp, "cache"), 0o755))
+		require.NoError(t, os.WriteFile(filepath.Join(gotmp, "cache", "x"), []byte("tmp"), 0o644))
+		require.NoError(t, os.WriteFile(filepath.Join(src, "novel.go"), []byte("package cli\nfunc Novel() {}\n"), 0o644))
+
+		installCopyDirTestHook(t, func(path string, _ fs.DirEntry) {
+			if path == gotmp {
+				require.NoError(t, os.RemoveAll(path))
+			}
+		})
+
+		require.NoError(t, CopyDir(src, dst))
+		assert.FileExists(t, filepath.Join(dst, "novel.go"))
+		got, err := os.ReadFile(filepath.Join(dst, "novel.go"))
+		require.NoError(t, err)
+		assert.Contains(t, string(got), "func Novel()")
+		assert.NoDirExists(t, filepath.Join(dst, ".gotmp"))
+	})
+
+	t.Run("vanished file", func(t *testing.T) {
+		src := filepath.Join(t.TempDir(), "src")
+		dst := filepath.Join(t.TempDir(), "dst")
+		require.NoError(t, os.MkdirAll(src, 0o755))
+		scratch := filepath.Join(src, "scratch.tmp")
+		require.NoError(t, os.WriteFile(scratch, []byte("tmp"), 0o644))
+		require.NoError(t, os.WriteFile(filepath.Join(src, "keep.txt"), []byte("keep"), 0o644))
+
+		installCopyDirTestHook(t, func(path string, _ fs.DirEntry) {
+			if path == scratch {
+				require.NoError(t, os.Remove(path))
+			}
+		})
+
+		require.NoError(t, CopyDir(src, dst))
+		assert.FileExists(t, filepath.Join(dst, "keep.txt"))
+		assert.NoFileExists(t, filepath.Join(dst, "scratch.tmp"))
+	})
+}
+
+func TestCopyDirFailsWhenSourceRootVanishes(t *testing.T) {
+	src := filepath.Join(t.TempDir(), "src")
+	dst := filepath.Join(t.TempDir(), "dst")
+	require.NoError(t, os.MkdirAll(src, 0o755))
+	require.NoError(t, os.WriteFile(filepath.Join(src, "keep.txt"), []byte("keep"), 0o644))
+
+	installCopyDirTestHook(t, func(path string, _ fs.DirEntry) {
+		if path == src {
+			require.NoError(t, os.RemoveAll(src))
+		}
+	})
+
+	err := CopyDir(src, dst)
+	require.Error(t, err)
+	assert.ErrorIs(t, err, fs.ErrNotExist)
+}
+
+func TestCopyDirStillFailsOnUnreadableDirectory(t *testing.T) {
+	if os.Getuid() == 0 {
+		t.Skip("root bypasses directory permissions")
+	}
+
+	src := filepath.Join(t.TempDir(), "src")
+	dst := filepath.Join(t.TempDir(), "dst")
+	secret := filepath.Join(src, "secret")
+	require.NoError(t, os.MkdirAll(secret, 0o755))
+	require.NoError(t, os.WriteFile(filepath.Join(secret, "x.txt"), []byte("x"), 0o644))
+	require.NoError(t, os.WriteFile(filepath.Join(src, "keep.txt"), []byte("keep"), 0o644))
+	require.NoError(t, os.Chmod(secret, 0))
+	t.Cleanup(func() { _ = os.Chmod(secret, 0o755) })
+
+	err := CopyDir(src, dst)
+	require.Error(t, err)
+	assert.False(t, errors.Is(err, fs.ErrNotExist), "permission failures must stay fatal, got %v", err)
+}
+
+func TestCopyFileReportsVanishedSource(t *testing.T) {
+	t.Parallel()
+
+	tests := []struct {
+		name string
+		dest string
+	}{
+		{name: "research.json", dest: "research.json"},
+		{name: "patch record", dest: filepath.Join(PatchesDirName, "drop-envelope.json")},
+		{name: "legacy patch index", dest: PatchesIndexFilename},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+			dst := filepath.Join(t.TempDir(), tt.dest)
+			err := copyFile(filepath.Join(t.TempDir(), filepath.Base(tt.dest)), dst, 0o644)
+			require.Error(t, err)
+			assert.ErrorIs(t, err, fs.ErrNotExist)
+			assert.NoFileExists(t, dst)
+		})
+	}
+}
+
+func TestSetCopyDirWalkHookForTestRestoresPrevious(t *testing.T) {
+	src := filepath.Join(t.TempDir(), "src")
+	require.NoError(t, os.MkdirAll(src, 0o755))
+	require.NoError(t, os.WriteFile(filepath.Join(src, "keep.txt"), []byte("keep"), 0o644))
+
+	var hits []string
+	clearOuter := SetCopyDirWalkHookForTest(func(string, fs.DirEntry) { hits = append(hits, "outer") })
+	t.Cleanup(func() { copyDirTestHook.Store(nil) })
+	clearInner := SetCopyDirWalkHookForTest(func(string, fs.DirEntry) { hits = append(hits, "inner") })
+
+	require.NoError(t, CopyDir(src, filepath.Join(t.TempDir(), "inner")))
+	assert.NotContains(t, hits, "outer")
+	assert.Contains(t, hits, "inner")
+
+	clearInner()
+	hits = nil
+	require.NoError(t, CopyDir(src, filepath.Join(t.TempDir(), "outer")))
+	assert.Contains(t, hits, "outer")
+	assert.NotContains(t, hits, "inner")
+
+	clearOuter()
+	hits = nil
+	require.NoError(t, CopyDir(src, filepath.Join(t.TempDir(), "none")))
+	assert.Empty(t, hits)
+}
+
+func TestSetCopyDirWalkHookForTestEarlierCleanupDoesNotClobberLater(t *testing.T) {
+	src := filepath.Join(t.TempDir(), "src")
+	require.NoError(t, os.MkdirAll(src, 0o755))
+	require.NoError(t, os.WriteFile(filepath.Join(src, "keep.txt"), []byte("keep"), 0o644))
+
+	var hits []string
+	clearFirst := SetCopyDirWalkHookForTest(func(string, fs.DirEntry) { hits = append(hits, "first") })
+	clearSecond := SetCopyDirWalkHookForTest(func(string, fs.DirEntry) { hits = append(hits, "second") })
+	t.Cleanup(func() { copyDirTestHook.Store(nil) })
+
+	clearFirst()
+	require.NoError(t, CopyDir(src, filepath.Join(t.TempDir(), "dst")))
+	assert.NotContains(t, hits, "first")
+	assert.Contains(t, hits, "second")
+
+	clearSecond()
+}
+
 func TestCopyPublishableManuscriptDirFiltersSymlinks(t *testing.T) {
 	src := filepath.Join(t.TempDir(), "src")
 	dst := filepath.Join(t.TempDir(), "dst")