osroot: refuse a non-regular leaf before opening it (fixes a FIFO hang)

cmd/entire/cli/osroot/openfifo_unix_test.go

@@ -0,0 +1,57 @@
+//go:build unix
+
+package osroot_test
+
+import (
+	"errors"
+	"os"
+	"path/filepath"
+	"syscall"
+	"testing"
+	"time"
+
+	"github.com/entireio/cli/cmd/entire/cli/osroot"
+)
+
+// TestOpenNoFollow_RefusesFifoWithoutBlocking is the case the refusal exists
+// for. open(2) on a FIFO with no writer blocks until one arrives, and none of
+// these helpers passes O_NONBLOCK, so before the Lstat gate this call hung the
+// process instead of failing it — `entire doctor` in a repo with a FIFO at
+// .claude/settings.json was unkillable short of SIGINT.
+//
+// The test is written as a race against a timer rather than a plain error
+// assertion, because a regression here does not fail: it hangs, and an
+// unguarded assertion would take the whole package's timeout with it.
+//
+// Unix-only by build constraint rather than a runtime skip: syscall.Mkfifo does
+// not exist on Windows, and a runtime guard still has to compile.
+func TestOpenNoFollow_RefusesFifoWithoutBlocking(t *testing.T) {
+	t.Parallel()
+
+	dir := t.TempDir()
+	if err := syscall.Mkfifo(filepath.Join(dir, "settings.json"), 0o600); err != nil {
+		t.Skipf("mkfifo unsupported: %v", err)
+	}
+	root, err := os.OpenRoot(dir)
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer root.Close()
+
+	done := make(chan error, 1)
+	go func() {
+		_, openErr := osroot.OpenNoFollow(root, "settings.json")
+		done <- openErr
+	}()
+
+	select {
+	case openErr := <-done:
+		if !errors.Is(openErr, osroot.ErrNotRegularFile) {
+			t.Errorf("OpenNoFollow(fifo) error = %v, want ErrNotRegularFile", openErr)
+		}
+	case <-time.After(10 * time.Second):
+		// Deliberately not t.Fatal: the goroutine is still parked in openat and
+		// will stay there, so say what happened and let the process exit.
+		t.Error("OpenNoFollow(fifo) blocked instead of returning; the pre-open type check is gone")
+	}
+}

cmd/entire/cli/osroot/osroot.go

@@ -76,6 +76,18 @@ func OpenNoFollow(root *os.Root, name string) (*os.File, error) {
 	if before.Mode()&os.ModeSymlink != 0 {
 		return nil, fmt.Errorf("%s: %w", name, ErrSymlinkedPath)
 	}
+	// Checked BEFORE the open, which is the whole point: open(2) on a FIFO with
+	// no writer blocks until one arrives, and none of these helpers passes
+	// O_NONBLOCK. A named pipe at a path Entire reads therefore hung the process
+	// in openat rather than failing it — `entire doctor` in a repo with a FIFO
+	// at .claude/settings.json was unkillable short of SIGINT.
+	//
+	// A directory is refused here too. io.ReadAll of one already failed a step
+	// later with a platform-dependent errno, and a caller asking for a file
+	// wants the refusal, not an EISDIR from the middle of its read.
+	if err := requireRegularFile(name, before.Mode()); err != nil {
+		return nil, err
+	}
 
 	f, err := parent.Open(leaf)
 	if err != nil {
@@ -128,6 +140,25 @@ func OpenFileNoFollow(root *os.Root, name string, flag int, perm os.FileMode) (*
 	return f, nil
 }
 
+// requireRegularFile rejects anything that is not a regular file.
+//
+// The error names the path and the condition but not the type, deliberately:
+// paths.describeMode already renders one for the .entire scan, and a second
+// copy of that vocabulary in the layer underneath it is how the two drift
+// apart. `ls -l` answers "which kind", and doctor's agent-path scan names it.
+//
+// fs.ModeIrregular is masked out rather than matched: Windows maps every reparse
+// tag it has no category for onto that bit, which lands OneDrive Files
+// On-Demand placeholders there, and a placeholder is a perfectly readable file.
+// The same tolerance the .entire entry scan applies, for the same reason —
+// refusing it would hard-fail every repository inside a synced folder.
+func requireRegularFile(name string, mode fs.FileMode) error {
+	if mode.Type()&^fs.ModeIrregular != 0 {
+		return fmt.Errorf("%s: %w", name, ErrNotRegularFile)
+	}
+	return nil
+}
+
 func validateOpenedFile(root *os.Root, name string, f *os.File) error {
 	pathInfo, err := root.Lstat(name)
 	if err != nil {
@@ -225,6 +256,15 @@ func ReadDirNoSymlinks(root *os.Root, name string) ([]os.DirEntry, error) {
 // Callers match it with errors.Is.
 var ErrSymlinkedPath = errors.New("path component is a symlink")
 
+// ErrNotRegularFile reports a directory, FIFO, socket or device where a file was
+// required. Callers match it with errors.Is.
+//
+// Deliberately not classified as os.ErrNotExist, though several callers reach
+// these helpers to decide "is there a config here?": a path occupied by the
+// wrong kind of object is a broken repository, and answering "absent" would
+// have Entire write a fresh file over whatever is there.
+var ErrNotRegularFile = errors.New("path is not a regular file")
+
 // ErrReplacedDuringOpen reports that name resolved to one file when it was
 // opened and a different one by the time the open was validated: something
 // replaced it in between. Callers match it with errors.Is.

cmd/entire/cli/osroot/osroot_test.go

@@ -1,6 +1,7 @@
 package osroot_test
 
 import (
+	"errors"
 	"io/fs"
 	"os"
 	"path/filepath"
@@ -730,3 +731,71 @@ func TestRemoveAllNoSymlinks(t *testing.T) {
 		assert.FileExists(t, filepath.Join(outside, "keep"), "its target is not")
 	})
 }
+
+// TestOpenNoFollow_RejectsDirectory pins the portable half of the non-regular
+// refusal. A directory used to open fine and fail a step later, inside the
+// caller's io.ReadAll, with a platform-dependent errno.
+func TestOpenNoFollow_RejectsDirectory(t *testing.T) {
+	t.Parallel()
+
+	dir := t.TempDir()
+	if err := os.MkdirAll(filepath.Join(dir, "sub"), 0o750); err != nil {
+		t.Fatal(err)
+	}
+	root, err := os.OpenRoot(dir)
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer root.Close()
+
+	if _, err := osroot.OpenNoFollow(root, "sub"); !errors.Is(err, osroot.ErrNotRegularFile) {
+		t.Errorf("OpenNoFollow(dir) error = %v, want ErrNotRegularFile", err)
+	}
+	if _, err := osroot.ReadFileNoFollow(root, "sub"); !errors.Is(err, osroot.ErrNotRegularFile) {
+		t.Errorf("ReadFileNoFollow(dir) error = %v, want ErrNotRegularFile", err)
+	}
+}
+
+// A regular file is unaffected — the refusal must not have become a blanket one.
+func TestOpenNoFollow_AllowsRegularFile(t *testing.T) {
+	t.Parallel()
+
+	dir := t.TempDir()
+	if err := os.WriteFile(filepath.Join(dir, "f.json"), []byte(`{"a":1}`), 0o600); err != nil {
+		t.Fatal(err)
+	}
+	root, err := os.OpenRoot(dir)
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer root.Close()
+
+	data, err := osroot.ReadFileNoFollow(root, "f.json")
+	if err != nil {
+		t.Fatalf("ReadFileNoFollow() error = %v", err)
+	}
+	if string(data) != `{"a":1}` {
+		t.Errorf("ReadFileNoFollow() = %q", data)
+	}
+}
+
+// A missing file must still classify as os.ErrNotExist, because callers use that
+// to tell "no config here" from "broken config here".
+func TestOpenNoFollow_MissingStaysNotExist(t *testing.T) {
+	t.Parallel()
+
+	dir := t.TempDir()
+	root, err := os.OpenRoot(dir)
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer root.Close()
+
+	_, err = osroot.ReadFileNoFollow(root, "absent.json")
+	if !os.IsNotExist(err) {
+		t.Errorf("ReadFileNoFollow(absent) error = %v, want os.IsNotExist", err)
+	}
+	if errors.Is(err, osroot.ErrNotRegularFile) {
+		t.Error("an absent file must not report ErrNotRegularFile")
+	}
+}