Re-land the os.Root coverage follow-ups dropped from #2220

cmd/entire/cli/agent/hook_config_file.go

@@ -127,8 +127,13 @@ func (f *HookConfigFile) Exists() bool {
 // as a symlink is refused with osroot.ErrSymlinkedPath rather than followed.
 func (f *HookConfigFile) Write(data []byte, perm os.FileMode) error {
 	if dir := path.Dir(f.name); dir != "." {
+		// 0750 for every agent, including the directories Entire creates below
+		// an agent's own (.pi/extensions/entire). The agent process runs as
+		// whoever ran `entire enable`, so it can traverse its own config
+		// directory; a setup where those differ (provisioned as root, agent as
+		// another uid) needs 0755 for all nine and is not one agent's to choose.
 		if err := osroot.MkdirAllNoSymlink(f.root, dir, 0o750); err != nil {
-			return fmt.Errorf("create %s: %w", path.Dir(f.path), err)
+			return fmt.Errorf("create %s: %w", filepath.Dir(f.path), err)
 		}
 	}
 	if info, err := osroot.LstatNoSymlinks(f.root, f.name); err == nil && info.Mode()&os.ModeSymlink != 0 {
@@ -164,19 +169,40 @@ func (f *HookConfigFile) Remove() error {
 // correct uninstall and taking the parent would delete the user's own config
 // with it.
 //
-// Refuses to act when the file sits directly at the worktree root, which would
-// make the directory to delete the repository.
+// Enforced rather than described, because "every other agent must not call
+// this" was a comment and nothing else, and the call it guards is a recursive
+// delete. The precondition is stated positively: the directory has to be one
+// ENTIRE named, which is the only kind it created to hold a generated file.
+//
+// A blocklist of the agents' own directories was the obvious alternative and is
+// not sound. AllProtectedDirs() holds `.opencode` and `.github/hooks` but not
+// `.opencode/plugins`, `.pi/extensions` or `.github`, so deriving the target
+// with path.Dir and checking it against that list still permits
+// RemoveAll(".opencode/plugins") — the user's other OpenCode plugins — for any
+// future agent whose config sits one level deeper than its root. Every agent
+// root and every shared intermediate fails the name test instead, and pi's
+// `.pi/extensions/entire` passes it because Entire is what created it.
 func (f *HookConfigFile) RemoveDir() error {
 	dir := path.Dir(f.name)
 	if dir == "." {
 		return fmt.Errorf("remove %s: refusing to remove the worktree root", filepath.Dir(f.path))
 	}
+	if path.Base(dir) != entireOwnedDirName {
+		return fmt.Errorf("remove %s: refusing to remove %q, which Entire did not create; "+
+			"RemoveDir is only for a directory named %q that holds one generated file",
+			filepath.Dir(f.path), path.Base(dir), entireOwnedDirName)
+	}
 	if err := osroot.RemoveAllNoSymlinks(f.root, dir); err != nil {
 		return fmt.Errorf("remove %s: %w", filepath.Dir(f.path), err)
 	}
 	return nil
 }
 
+// entireOwnedDirName is the directory name Entire uses when it has to create a
+// directory of its own inside a tree an agent owns (`.pi/extensions/entire`).
+// RemoveDir keys its refusal on it.
+const entireOwnedDirName = "entire"
+
 // Root exposes the underlying root and the file's name inside it, for the
 // callers that need a descriptor rather than the bytes. Both are Codex, which
 // bounds .codex/hooks.json on its stat size before reading any of it: Read is
@@ -188,8 +214,11 @@ func (f *HookConfigFile) RemoveDir() error {
 // Root.Open, which is what Read does for them.
 func (f *HookConfigFile) Root() (*os.Root, string) { return f.root, f.name }
 
-// GeneratedState is GeneratedHookFileState for a file read through this root.
-// See that function for what marker and render mean.
+// GeneratedState reports whether a generated hook file is absent, current or
+// outdated, for the agents whose whole integration is one file Entire writes
+// (Pi, OpenCode). marker is the Entire-managed sentinel that identifies the file
+// as ours rather than the user's; render is what the current template would
+// write, compared against the file's contents to tell current from outdated.
 func (f *HookConfigFile) GeneratedState(marker, render string) HookConfigState {
 	data, err := f.Read()
 	if err != nil {

cmd/entire/cli/agent/hook_config_file_test.go

@@ -3,23 +3,14 @@ package agent_test
 import (
 	"os"
 	"path/filepath"
-	"runtime"
 	"testing"
 
 	"github.com/entireio/cli/cmd/entire/cli/agent"
 	"github.com/entireio/cli/cmd/entire/cli/osroot"
+	"github.com/entireio/cli/cmd/entire/cli/testutil"
 	"github.com/stretchr/testify/require"
 )
 
-// skipWithoutSymlinks skips a test that needs to create one. On Windows that
-// takes elevation or developer mode, neither of which CI has.
-func skipWithoutSymlinks(t *testing.T) {
-	t.Helper()
-	if runtime.GOOS == "windows" {
-		t.Skip("symlink creation needs elevation on Windows")
-	}
-}
-
 func TestHookConfig_ReadWriteRemoveRoundTrip(t *testing.T) {
 	t.Parallel()
 
@@ -49,7 +40,7 @@ func TestHookConfig_ReadWriteRemoveRoundTrip(t *testing.T) {
 // through it — to wherever it pointed, outside the repository included.
 func TestHookConfig_WriteRefusesASymlinkedConfigDirectory(t *testing.T) {
 	t.Parallel()
-	skipWithoutSymlinks(t)
+	testutil.SkipWithoutSymlinks(t)
 
 	worktree := t.TempDir()
 	outside := t.TempDir()
@@ -67,7 +58,7 @@ func TestHookConfig_WriteRefusesASymlinkedConfigDirectory(t *testing.T) {
 
 func TestHookConfig_ReadRejectsARelativeSymlinkInsideTheWorktree(t *testing.T) {
 	t.Parallel()
-	skipWithoutSymlinks(t)
+	testutil.SkipWithoutSymlinks(t)
 
 	worktree := t.TempDir()
 	require.NoError(t, os.MkdirAll(filepath.Join(worktree, ".claude"), 0o750))
@@ -87,7 +78,7 @@ func TestHookConfig_ReadRejectsARelativeSymlinkInsideTheWorktree(t *testing.T) {
 // installed".
 func TestHookConfig_ReadRefusesAnAbsoluteSymlinkAtTheLeaf(t *testing.T) {
 	t.Parallel()
-	skipWithoutSymlinks(t)
+	testutil.SkipWithoutSymlinks(t)
 
 	worktree := t.TempDir()
 	outside := filepath.Join(t.TempDir(), "settings.json")
@@ -179,3 +170,64 @@ func TestHookConfigFile_RefusesASymlinkedFileAtTheLeaf(t *testing.T) {
 	require.NoError(t, err)
 	require.JSONEq(t, `{"from":"dotfiles"}`, string(data))
 }
+
+// TestHookConfigFile_RemoveDirRefusesAnAgentsOwnDirectory covers the guard the
+// doc comment used to describe and nothing enforced. Only pi calls RemoveDir
+// today; the hazard is the next agent integration copying the call, where
+// `path.Dir(".claude/settings.json")` is `.claude` and the uninstall would take
+// the user's hand-written settings, subagents and skills with it.
+func TestHookConfigFile_RemoveDirRefusesAnAgentsOwnDirectory(t *testing.T) {
+	t.Parallel()
+
+	for _, relPath := range []string{
+		".claude/settings.json",
+		".cursor/hooks.json",
+		".codex/hooks.json",
+	} {
+		t.Run(relPath, func(t *testing.T) {
+			t.Parallel()
+
+			worktree := t.TempDir()
+			cfg, err := agent.OpenHookConfig(worktree, relPath)
+			require.NoError(t, err)
+			require.NoError(t, cfg.Write([]byte("{}"), 0o600))
+
+			// Something of the user's, beside Entire's file in the same directory.
+			userFile := filepath.Join(filepath.Dir(cfg.Path()), "mine.json")
+			require.NoError(t, os.WriteFile(userFile, []byte("{}"), 0o600))
+
+			require.Error(t, cfg.RemoveDir(), "RemoveDir must refuse an agent's own directory")
+			require.FileExists(t, userFile, "the user's own config must survive")
+		})
+	}
+}
+
+// The case RemoveDir does exist for: a directory Entire created to hold one
+// generated file, which pi discovers by directory rather than by filename.
+func TestHookConfigFile_RemoveDirAcceptsADirectoryEntireCreated(t *testing.T) {
+	t.Parallel()
+
+	worktree := t.TempDir()
+	cfg, err := agent.OpenHookConfig(worktree, ".pi/extensions/entire/index.ts")
+	require.NoError(t, err)
+	require.NoError(t, cfg.Write([]byte("export {}"), 0o644))
+
+	require.NoError(t, cfg.RemoveDir())
+	require.NoDirExists(t, filepath.Join(worktree, ".pi", "extensions", "entire"))
+	require.DirExists(t, filepath.Join(worktree, ".pi", "extensions"),
+		"only the directory holding the generated file goes")
+}
+
+// RemoveDir on a file sitting directly at the worktree root would make the
+// directory to delete the repository.
+func TestHookConfigFile_RemoveDirRefusesTheWorktreeRoot(t *testing.T) {
+	t.Parallel()
+
+	worktree := t.TempDir()
+	cfg, err := agent.OpenHookConfig(worktree, "hooks.json")
+	require.NoError(t, err)
+	require.NoError(t, cfg.Write([]byte("{}"), 0o600))
+
+	require.Error(t, cfg.RemoveDir())
+	require.DirExists(t, worktree)
+}

cmd/entire/cli/agent_hook_config_guard_test.go

@@ -43,36 +43,66 @@ func TestAllHookConfigRelPaths_CoversEveryWorktreeConfigAgent(t *testing.T) {
 		t.Skipf("not in a git checkout: %v", err)
 	}
 
+	dir := strings.TrimSpace(string(repoRoot))
+	callers := agentPackagesMatching(t, dir, "agent.OpenHookConfig(")
+	locators := agentPackagesMatching(t, dir, ") HookConfigRelPath() string {")
+
+	// Sets, not counts. len(declared) == len(callers) passed whenever an added
+	// omission was offset by a removal in the same change — the failure this
+	// test exists to catch, since the agent still works and only doctor's
+	// diagnosis goes quiet — and failed on an agent whose call happens to sit in
+	// a sub-package, which is no defect at all. Both sides are package
+	// directories, so they are directly comparable.
+	require.Equal(t, locators, callers,
+		"the agent packages calling agent.OpenHookConfig and those implementing\n"+
+			"agent.HookConfigRelPath must be the same set. An agent that opens its\n"+
+			"hook config without declaring where it lives leaves the directories\n"+
+			"Entire creates between its own directory and that file unchecked by\n"+
+			"doctor's symlink diagnosis.")
+
+	// The registry is the thing doctor actually reads, so a locator that exists
+	// in source but never reaches AllHookConfigRelPaths (an agent left out of
+	// the registry, or one returning "") is its own failure.
+	//
+	// A count, deliberately, directly under the argument against counts above —
+	// and defeatable the same way, by dropping one agent from the registry while
+	// adding another locator. A set comparison would need to map a package
+	// directory to the rel path it declares, and nothing does: `geminicli`
+	// declares `.gemini/settings.json` and `copilotcli` declares
+	// `.github/hooks/entire.json`, so neither the package name nor the path's
+	// first component derives the other. The set comparison above is the guard
+	// that matters; this one only catches a locator the registry never sees.
+	require.Len(t, agent.AllHookConfigRelPaths(), len(locators),
+		"%d agent packages implement HookConfigRelPath but the registry reports %d paths (%s)",
+		len(locators), len(agent.AllHookConfigRelPaths()), strings.Join(agent.AllHookConfigRelPaths(), ", "))
+}
+
+// agentPackagesMatching returns the sorted, deduplicated agent package
+// directories whose non-test sources contain needle, asserting that the pattern
+// still matches something — a re-worded signature would otherwise turn this
+// guard into a comparison of two empty sets.
+func agentPackagesMatching(t *testing.T, repoRoot, needle string) []string {
+	t.Helper()
 	grep := exec.Command("git", "grep", "-l", "--fixed-strings", "--", //nolint:noctx // guard test, no cancellation needed
-		"agent.OpenHookConfig(", "--", ":(glob)cmd/entire/cli/agent/**/*.go")
-	grep.Dir = strings.TrimSpace(string(repoRoot))
+		needle, "--", ":(glob)cmd/entire/cli/agent/**/*.go")
+	grep.Dir = repoRoot
+	// Set for the same reason every git subprocess naming its target with
+	// cmd.Dir does: git exports GIT_DIR/GIT_WORK_TREE to hooks, and those take
+	// precedence over cmd.Dir.
 	grep.Env = gitrepo.EnvWithoutRepoOverrides()
-	out, grepErr := grep.Output()
-	require.NoError(t, grepErr, "no agent calls agent.OpenHookConfig, which cannot be right")
+	out, err := grep.Output()
+	require.NoError(t, err, "no agent source matches %q, which cannot be right", needle)
 
-	callers := make(map[string]struct{})
+	var pkgs []string
 	for line := range strings.SplitSeq(strings.TrimSpace(string(out)), "\n") {
 		if line == "" || strings.HasSuffix(line, "_test.go") {
 			continue
 		}
-		callers[path.Dir(line)] = struct{}{}
-	}
-	require.NotEmpty(t, callers, "the detection pattern has gone stale and must be re-pointed")
-
-	declared := agent.AllHookConfigRelPaths()
-	require.Len(t, declared, len(callers),
-		"%d agent packages call agent.OpenHookConfig (%s) but %d declare a path (%s).\n"+
-			"Every agent whose hook config is a worktree file must implement "+
-			"agent.HookConfigLocator, or the directories Entire creates between its "+
-			"own directory and that file go unchecked by doctor's symlink diagnosis.",
-		len(callers), strings.Join(sortedPackageDirs(callers), ", "), len(declared), strings.Join(declared, ", "))
-}
-
-func sortedPackageDirs(m map[string]struct{}) []string {
-	out := make([]string, 0, len(m))
-	for k := range m {
-		out = append(out, k)
+		if dir := path.Dir(line); !slices.Contains(pkgs, dir) {
+			pkgs = append(pkgs, dir)
+		}
 	}
-	slices.Sort(out)
-	return out
+	slices.Sort(pkgs)
+	require.NotEmpty(t, pkgs, "the detection pattern %q has gone stale and must be re-pointed", needle)
+	return pkgs
 }