diff --git a/cmd/entire/cli/agent/hook_config_file.go b/cmd/entire/cli/agent/hook_config_file.go index c3f515fa27..afff49f05d 100644 --- a/cmd/entire/cli/agent/hook_config_file.go +++ b/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 { diff --git a/cmd/entire/cli/agent/hook_config_file_test.go b/cmd/entire/cli/agent/hook_config_file_test.go index 553f67747d..362c0d4d44 100644 --- a/cmd/entire/cli/agent/hook_config_file_test.go +++ b/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) +} diff --git a/cmd/entire/cli/agent_hook_config_guard_test.go b/cmd/entire/cli/agent_hook_config_guard_test.go index 5bf202a91f..97d388465e 100644 --- a/cmd/entire/cli/agent_hook_config_guard_test.go +++ b/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 } diff --git a/cmd/entire/cli/doctor.go b/cmd/entire/cli/doctor.go index 5fb4cf0876..49b7ed9440 100644 --- a/cmd/entire/cli/doctor.go +++ b/cmd/entire/cli/doctor.go @@ -5,6 +5,7 @@ import ( "errors" "fmt" "io" + "io/fs" "log/slog" "os" "path" @@ -729,19 +730,29 @@ func checkEntireDirSymlinks(cmd *cobra.Command) { } fmt.Fprintf(w, "%s contents: SYMLINKS PRESENT\n", paths.EntireDir) - for i, name := range links { - if i == symlinkReportLimit { - fmt.Fprintf(w, " ... and %d more\n", len(links)-symlinkReportLimit) - break - } - fmt.Fprintf(w, " %s -> %s\n", path.Join(paths.EntireDir, name), readlinkOrUnknownIn(root, name)) - } + printCappedList(w, links, func(name string) string { + return path.Join(paths.EntireDir, name) + " -> " + readlinkOrUnknownIn(root, name) + }) fmt.Fprintln(w, " Entire will not create or write through a symlinked directory here, so") fmt.Fprintln(w, " anything that belongs under one of these paths is not being captured.") fmt.Fprintln(w, " Fix: replace each path above with a real directory. If it is tracked in git,") fmt.Fprintln(w, " `git rm --cached` it first, and add it to .gitignore so it does not come back.") } +// printCappedList prints one indented line per name via render, replacing the +// tail past symlinkReportLimit with a count. Four call sites had this loop +// inline, differing only in the item line, so the off-by-one truncation +// contract was written out four times. +func printCappedList(w io.Writer, names []string, render func(string) string) { + for i, name := range names { + if i == symlinkReportLimit { + fmt.Fprintf(w, " ... and %d more\n", len(names)-symlinkReportLimit) + return + } + fmt.Fprintf(w, " %s\n", render(name)) + } +} + // checkAgentDirSymlinks reports a symlink at any directory component Entire // creates or writes through for an agent: the agents' own config directories // (.claude, .codex, .cursor, .gemini, .factory, .opencode, .pi, .github/hooks) @@ -774,10 +785,19 @@ func checkAgentDirSymlinks(cmd *cobra.Command) { } root, err := worktreedir.OpenAt(worktreeRoot) if err != nil { + // Reported, not swallowed, for the same reason a single unreadable + // component is: a worktree root that will not open is a state where + // every hook install also fails, so printing nothing here would hand + // back a clean bill of health on a repo where nothing can be installed. + fmt.Fprintln(w, "Agent config directories: NOT CHECKED") + fmt.Fprintf(w, " %v\n", err) + fmt.Fprintln(w, " Entire could not open the worktree root, so it cannot say whether the") + fmt.Fprintln(w, " paths it installs hooks and skills under are real directories.") + fmt.Fprintln(w, " Fix: check the ownership and permissions of the repository root.") return } - var links, unreadable []string + var links, unreadable, wrongType []string reported := make(map[string]struct{}) for _, candidate := range agentSymlinkCheckPaths() { name, outcome := scanForSymlinkedComponent(root, candidate) @@ -790,22 +810,23 @@ func checkAgentDirSymlinks(cmd *cobra.Command) { continue } reported[name] = struct{}{} - if outcome == componentScanUnreadable { + switch outcome { + case componentScanUnreadable: unreadable = append(unreadable, name) - continue + case componentScanWrongType: + wrongType = append(wrongType, name) + case componentScanLinked: + links = append(links, name) + case componentScanClean: + // Filtered out above; listed so a new outcome fails the build here. } - links = append(links, name) } if len(links) > 0 { fmt.Fprintln(w, "Agent config directories: SYMLINKS PRESENT") - for i, name := range links { - if i == symlinkReportLimit { - fmt.Fprintf(w, " ... and %d more\n", len(links)-symlinkReportLimit) - break - } - fmt.Fprintf(w, " %s -> %s\n", name, readlinkOrUnknownIn(root, name)) - } + printCappedList(w, links, func(name string) string { + return name + " -> " + readlinkOrUnknownIn(root, name) + }) fmt.Fprintln(w, " Entire will not create or write through a symlinked path here, so the") fmt.Fprintln(w, " hooks and skills that belong under these paths are not installed, and") fmt.Fprintln(w, " `entire status` reports them as absent rather than as blocked.") @@ -814,19 +835,32 @@ func checkAgentDirSymlinks(cmd *cobra.Command) { fmt.Fprintln(w, " does not come back.") } + // A regular file where a directory belongs gets its own heading, because the + // fix is to replace the path and no amount of chmod reaches it. Same split + // the .entire scan makes between ErrEntireDirNotDirectory and + // ErrEntireDirUnreadable, for the same reason. + if len(wrongType) > 0 { + fmt.Fprintln(w, "Agent config directories: BROKEN") + printCappedList(w, wrongType, func(name string) string { + what := "of an unknown type" + if info, err := osroot.LstatNoSymlinks(root, name); err == nil { + what = paths.DescribeMode(info.Mode()) + } + return fmt.Sprintf("%s is %s", name, what) + }) + fmt.Fprintln(w, " Entire cannot create the hooks and skills that belong under these paths,") + fmt.Fprintln(w, " so `entire status` reports them as absent rather than as blocked.") + fmt.Fprintln(w, " Fix: replace each path above with a real directory. If it is tracked in") + fmt.Fprintln(w, " git, `git rm --cached` it first.") + } + // Separate from the links, and reported rather than swallowed: "we could not // find out" is not "there is nothing here". Entire's own write will fail on // the same path, so a silent scan would leave the user with hooks that never // install and a doctor that says nothing. if len(unreadable) > 0 { fmt.Fprintln(w, "Agent config directories: NOT READABLE") - for i, name := range unreadable { - if i == symlinkReportLimit { - fmt.Fprintf(w, " ... and %d more\n", len(unreadable)-symlinkReportLimit) - break - } - fmt.Fprintf(w, " %s\n", name) - } + printCappedList(w, unreadable, func(name string) string { return name }) fmt.Fprintln(w, " Entire could not tell whether these paths are real directories, so it") fmt.Fprintln(w, " cannot say whether hooks and skills can be installed under them.") fmt.Fprintln(w, " Fix: check the ownership and permissions of each path above.") @@ -846,12 +880,19 @@ const ( // componentScanUnreadable: the named component could not be statted, so // nothing is known about it. componentScanUnreadable + // componentScanWrongType: the named component exists as a regular file where + // a directory has to be. Separate from componentScanUnreadable because the + // remedies are different things — replace the path versus fix its ownership + // — and separate from componentScanLinked because the path to name and the + // thing to put back are both different. + componentScanWrongType ) // agentSymlinkCheckPaths returns the worktree-relative paths Entire creates or // writes through on behalf of an agent, sorted and deduplicated. Each is a full -// path rather than a directory, because firstSymlinkedComponent examines every -// component of what it is given and the leaf is refused too: HookConfigFile +// path rather than a directory, because scanForSymlinkedComponent examines every +// component of what it is given and the leaf is refused too — for a symlink and +// for a wrong file type alike: HookConfigFile // reads and writes through ReadFileNoFollow / a pinned-parent rename, and // writeManagedScaffold does the same, so a symlinked .claude/settings.json is // as broken as a symlinked .claude. @@ -866,10 +907,18 @@ const ( // the `.pi/extensions` and `.pi/extensions/entire` that Entire creates below it // are not, and a symlink at either produced no output at all until the config // paths were added here. And it is too broad — `.vogon` and an external -// plugin's directories are in it while Entire writes nothing into them, so -// reporting a link there would contradict the rule this list follows. Every +// plugin's directories are in it while Entire writes nothing into them. Every // top-level agent directory Entire does write to is already covered, as a // component of the config or scaffold path underneath it. +// +// Not gated on the agent being configured, which is a deliberate call rather +// than an oversight. Two of these trees are shared and user-owned — `.github` +// (Copilot CLI's hook config) and `.agents` (Codex's documented skills path) — +// so a monorepo that symlinks either is told about it even though it may never +// enable those agents. That is noise, and it is the lesser fault: gating on +// installation would have to ask whether hooks are installed, and that question +// is answered by reading through the very config a symlink hides, so the check +// would fall silent in exactly the case it exists for. func agentSymlinkCheckPaths() []string { seen := make(map[string]struct{}) var out []string @@ -889,12 +938,15 @@ func agentSymlinkCheckPaths() []string { add(relPath) } for _, name := range agent.List() { - if relPath, _, ok := searchSkillTemplate(name); ok { - add(relPath) - } - if relPath, _, ok := agentHelpSkillTemplate(name); ok { - add(relPath) - } + add(searchSkillTemplatePath(name)) + add(agentHelpSkillTemplatePath(name)) + // The pre-skill subagent Entire scaffolded and now deletes. Uninstall + // goes through osroot.LstatNoSymlinks, which refuses a symlinked parent, + // so .claude/agents/ has to be here or a link there is refused with + // nothing said about it. .codex/agents and .gemini/agents were already + // covered, but only as a side effect of the agent-help template living + // under them. + add(legacySearchSubagentPath(name)) } slices.Sort(out) @@ -929,10 +981,50 @@ func scanForSymlinkedComponent(root *os.Root, name string) (string, componentSca if info.Mode()&os.ModeSymlink != 0 { return prefix, componentScanLinked } + // Every component's shape is checked, with the expectation depending on + // where it sits: a component with more path still to go has to be a + // directory, and the leaf has to be a regular file. Both are allowlists + // rather than tests for one rejected type — the .entire scan's doc spends + // a paragraph on why an allowlist a rejected type can enter by setting an + // extra bit is not an allowlist — and an earlier revision that only + // looked for a regular file at a non-leaf missed a FIFO, socket or device + // node entirely. + // + // The leaf matters as much as its parents, and for a worse reason: a FIFO + // at `.claude/settings.json` does not fail the read, it BLOCKS it. Every + // agent's config read goes through osroot.OpenNoFollow, whose open(2) has + // no O_NONBLOCK, so `entire doctor` hangs in openat until interrupted. + // Reporting it is all this scan can do; refusing to open one is + // OpenNoFollow's job. + // + // Identified from the mode rather than from the ENOTDIR the next Lstat + // would return, which would mean being right about which errno each + // platform picks. fs.ModeIrregular is tolerated the way the .entire scan + // tolerates it: Windows maps directory junctions and cloud placeholders + // onto that bit, a junction arriving as bare ModeIrregular (a + // name-surrogate reparse tag withholds ModeDir) and a placeholder + // directory as ModeDir|ModeIrregular. + if !componentHasExpectedShape(info.Mode(), prefix == name) { + return prefix, componentScanWrongType + } } return "", componentScanClean } +// componentHasExpectedShape reports whether mode is what has to be at this +// position: a regular file at the leaf, something a path can descend through +// above it. fs.ModeIrregular is masked out of both tests rather than matched +// against — see scanForSymlinkedComponent for why Windows makes that necessary, +// and note it is why a bare ModeIrregular satisfies the leaf test as well as +// the directory one. +func componentHasExpectedShape(mode fs.FileMode, isLeaf bool) bool { + t := mode.Type() &^ fs.ModeIrregular + if isLeaf { + return t == 0 + } + return t == fs.ModeDir +} + // readlinkOrUnknown renders a symlink's target for a diagnostic, never failing: // an unreadable link is still worth naming. func readlinkOrUnknown(name string) string { diff --git a/cmd/entire/cli/doctor_fifo_unix_test.go b/cmd/entire/cli/doctor_fifo_unix_test.go new file mode 100644 index 0000000000..db870d0dc1 --- /dev/null +++ b/cmd/entire/cli/doctor_fifo_unix_test.go @@ -0,0 +1,71 @@ +//go:build unix + +package cli + +import ( + "os" + "path/filepath" + "syscall" + "testing" + + "github.com/entireio/cli/cmd/entire/cli/worktreedir" +) + +// TestScanForSymlinkedComponent_NonTraversableComponent pins the allowlist. An +// earlier revision tested only for a regular file, so a FIFO, socket or device +// node where a directory belongs came back clean and doctor printed nothing — +// while os.Root and every hook install fail on it. +// +// Unix-only by build constraint rather than by a runtime t.Skip: syscall.Mkfifo +// does not exist on Windows at all, and a runtime guard still has to compile. +func TestScanForSymlinkedComponent_NonTraversableComponent(t *testing.T) { + t.Parallel() + + dir := t.TempDir() + if err := syscall.Mkfifo(filepath.Join(dir, claudeDirName), 0o600); err != nil { + t.Skipf("mkfifo unsupported: %v", err) + } + root, err := worktreedir.OpenAt(dir) + if err != nil { + t.Fatal(err) + } + + name, outcome := scanForSymlinkedComponent(root, claudeDirName+"/settings.json") + if outcome != componentScanWrongType { + t.Errorf("outcome = %v, want componentScanWrongType for a FIFO", outcome) + } + if name != claudeDirName { + t.Errorf("name = %q, want %s", name, claudeDirName) + } +} + +// TestScanForSymlinkedComponent_FifoAtTheLeaf is the half that matters most, and +// the one an earlier revision missed by gating the type check on `prefix != +// name`. A FIFO here does not fail the config read, it blocks it: every agent +// reads through osroot.OpenNoFollow, whose open(2) has no O_NONBLOCK, so +// `entire doctor` hangs in openat until interrupted. Reported so the condition +// is at least nameable. +func TestScanForSymlinkedComponent_FifoAtTheLeaf(t *testing.T) { + t.Parallel() + + dir := t.TempDir() + if err := os.MkdirAll(filepath.Join(dir, claudeDirName), 0o750); err != nil { + t.Fatal(err) + } + leaf := filepath.Join(dir, claudeDirName, "settings.json") + if err := syscall.Mkfifo(leaf, 0o600); err != nil { + t.Skipf("mkfifo unsupported: %v", err) + } + root, err := worktreedir.OpenAt(dir) + if err != nil { + t.Fatal(err) + } + + name, outcome := scanForSymlinkedComponent(root, claudeDirName+"/settings.json") + if outcome != componentScanWrongType { + t.Errorf("outcome = %v, want componentScanWrongType for a FIFO leaf", outcome) + } + if name != claudeDirName+"/settings.json" { + t.Errorf("name = %q, want the leaf itself", name) + } +} diff --git a/cmd/entire/cli/doctor_test.go b/cmd/entire/cli/doctor_test.go index 357d9edb6a..f75790a4c7 100644 --- a/cmd/entire/cli/doctor_test.go +++ b/cmd/entire/cli/doctor_test.go @@ -3,6 +3,7 @@ package cli import ( "bytes" "context" + "io/fs" "log/slog" "os" "path/filepath" @@ -22,6 +23,7 @@ import ( "github.com/entireio/cli/cmd/entire/cli/session" "github.com/entireio/cli/cmd/entire/cli/strategy" "github.com/entireio/cli/cmd/entire/cli/testutil" + "github.com/entireio/cli/cmd/entire/cli/worktreedir" "github.com/go-git/go-git/v6" "github.com/go-git/go-git/v6/plumbing" @@ -1278,7 +1280,7 @@ func TestCheckHookDrift_ClaudeCodeWarnsWhenOutdated(t *testing.T) { dir := setupGitRepoForPhaseTest(t) t.Chdir(dir) - claudeDir := filepath.Join(dir, ".claude") + claudeDir := filepath.Join(dir, claudeDirName) require.NoError(t, os.MkdirAll(claudeDir, 0o750)) stale := `{ "hooks": { @@ -1490,7 +1492,7 @@ func TestCheckAgentDirSymlinks_ReportsSymlinkedAgentDir(t *testing.T) { t.Cleanup(osroot.ResetShared) elsewhere := t.TempDir() - if err := os.Symlink(elsewhere, filepath.Join(dir, ".claude")); err != nil { + if err := os.Symlink(elsewhere, filepath.Join(dir, claudeDirName)); err != nil { t.Skipf("symlink not supported: %v", err) } @@ -1512,8 +1514,8 @@ func TestCheckAgentDirSymlinks_ReportsSymlinkedScaffoldParent(t *testing.T) { t.Cleanup(paths.ClearWorktreeRootCache) t.Cleanup(osroot.ResetShared) - require.NoError(t, os.MkdirAll(filepath.Join(dir, ".claude"), 0o750)) - if err := os.Symlink(t.TempDir(), filepath.Join(dir, ".claude", "skills")); err != nil { + require.NoError(t, os.MkdirAll(filepath.Join(dir, claudeDirName), 0o750)) + if err := os.Symlink(t.TempDir(), filepath.Join(dir, claudeDirName, "skills")); err != nil { t.Skipf("symlink not supported: %v", err) } @@ -1566,8 +1568,8 @@ func TestCheckAgentDirSymlinks_ReportsSymlinkedConfigFile(t *testing.T) { t.Cleanup(paths.ClearWorktreeRootCache) t.Cleanup(osroot.ResetShared) - require.NoError(t, os.MkdirAll(filepath.Join(dir, ".claude"), 0o750)) - if err := os.Symlink(filepath.Join(t.TempDir(), "settings.json"), filepath.Join(dir, ".claude", "settings.json")); err != nil { + require.NoError(t, os.MkdirAll(filepath.Join(dir, claudeDirName), 0o750)) + if err := os.Symlink(filepath.Join(t.TempDir(), "settings.json"), filepath.Join(dir, claudeDirName, "settings.json")); err != nil { t.Skipf("symlink not supported: %v", err) } @@ -1599,7 +1601,7 @@ func TestCheckAgentDirSymlinks_NamesTheOutermostLinkOnce(t *testing.T) { t.Cleanup(paths.ClearWorktreeRootCache) t.Cleanup(osroot.ResetShared) - if err := os.Symlink(t.TempDir(), filepath.Join(dir, ".claude")); err != nil { + if err := os.Symlink(t.TempDir(), filepath.Join(dir, claudeDirName)); err != nil { t.Skipf("symlink not supported: %v", err) } @@ -1623,7 +1625,7 @@ func TestCheckAgentDirSymlinks_ReportsAnUnreadableComponent(t *testing.T) { t.Cleanup(paths.ClearWorktreeRootCache) t.Cleanup(osroot.ResetShared) - claude := filepath.Join(dir, ".claude") + claude := filepath.Join(dir, claudeDirName) require.NoError(t, os.MkdirAll(filepath.Join(claude, "skills"), 0o750)) require.NoError(t, os.Chmod(claude, 0o000)) t.Cleanup(func() { _ = os.Chmod(claude, 0o750) }) //nolint:errcheck // best-effort restore so t.TempDir can clean up @@ -1668,9 +1670,9 @@ func TestCheckAgentDirSymlinks_SilentWhenClean(t *testing.T) { }) t.Run("real directories and a user's own link inside one", func(t *testing.T) { - require.NoError(t, os.MkdirAll(filepath.Join(dir, ".claude", "skills"), 0o750)) - require.NoError(t, os.WriteFile(filepath.Join(dir, ".claude", "settings.json"), []byte("{}"), 0o600)) - if err := os.Symlink(t.TempDir(), filepath.Join(dir, ".claude", "skills", "my-own")); err != nil { + require.NoError(t, os.MkdirAll(filepath.Join(dir, claudeDirName, "skills"), 0o750)) + require.NoError(t, os.WriteFile(filepath.Join(dir, claudeDirName, "settings.json"), []byte("{}"), 0o600)) + if err := os.Symlink(t.TempDir(), filepath.Join(dir, claudeDirName, "skills", "my-own")); err != nil { t.Skipf("symlink not supported: %v", err) } @@ -1680,3 +1682,132 @@ func TestCheckAgentDirSymlinks_SilentWhenClean(t *testing.T) { "a shared skill symlinked into place is a real setup and none of Entire's business") }) } + +// TestScanForSymlinkedComponent_RegularFileWhereDirectoryBelongs pins the split +// between BROKEN and NOT READABLE. A regular file at `.claude` used to arrive +// here as componentScanUnreadable, so doctor answered "check the ownership and +// permissions" for a condition only replacing the path fixes — the else-branch +// pattern the .entire scan separates two error values to avoid. +func TestScanForSymlinkedComponent_RegularFileWhereDirectoryBelongs(t *testing.T) { + t.Parallel() + + dir := t.TempDir() + if err := os.WriteFile(filepath.Join(dir, claudeDirName), []byte("not a dir"), 0o644); err != nil { + t.Fatal(err) + } + // No osroot.ResetShared here, unlike the t.Chdir tests below: the registry + // is process-global and closing it mid-run breaks any test running in + // parallel — it took out readCapped's, which opens a root of its own. A + // registry entry for a unique temp dir needs no cleanup. + root, err := worktreedir.OpenAt(dir) + if err != nil { + t.Fatal(err) + } + + name, outcome := scanForSymlinkedComponent(root, claudeDirName+"/settings.json") + if outcome != componentScanWrongType { + t.Errorf("outcome = %v, want componentScanWrongType", outcome) + } + if name != claudeDirName { + t.Errorf("name = %q, want %s — the component to replace, not the leaf", name, claudeDirName) + } +} + +// TestCheckAgentDirSymlinks_ReportsWrongTypedComponent checks the remedy the +// user actually reads, not just the classification. +func TestCheckAgentDirSymlinks_ReportsWrongTypedComponent(t *testing.T) { + dir := setupGitRepoForPhaseTest(t) + t.Chdir(dir) + paths.ClearWorktreeRootCache() + t.Cleanup(paths.ClearWorktreeRootCache) + t.Cleanup(osroot.ResetShared) + + if err := os.WriteFile(filepath.Join(dir, claudeDirName), []byte("not a dir"), 0o644); err != nil { + t.Fatal(err) + } + + cmd, stdout := newTestCmd(t) + checkAgentDirSymlinks(cmd) + + got := stdout.String() + if !strings.Contains(got, "BROKEN") { + t.Errorf("output should report BROKEN, got:\n%s", got) + } + if !strings.Contains(got, "replace each path above with a real directory") { + t.Errorf("output should name the replace remedy, got:\n%s", got) + } + if strings.Contains(got, "ownership and permissions") { + t.Errorf("output must not offer the permissions remedy for a wrong-typed path, got:\n%s", got) + } +} + +// TestAgentSymlinkCheckPaths_CoversLegacySubagentDir keeps .claude/agents/ in +// the scan. removeLegacySearchSubagent deletes through it with +// osroot.LstatNoSymlinks, which refuses a symlinked parent, so a link there is +// refused at enable and has to be diagnosable. .codex/agents and .gemini/agents +// were only ever covered as a side effect of the agent-help template living +// under them. +func TestAgentSymlinkCheckPaths_CoversLegacySubagentDir(t *testing.T) { + t.Parallel() + + candidates := agentSymlinkCheckPaths() + var found bool + for _, c := range candidates { + if strings.HasPrefix(c, ".claude/agents/") { + found = true + break + } + } + if !found { + t.Errorf("no candidate under .claude/agents/; got %v", candidates) + } +} + +// A real directory is traversable and reports clean, so the allowlist has not +// become a blanket rejection. +func TestScanForSymlinkedComponent_DirectoryIsClean(t *testing.T) { + t.Parallel() + + dir := t.TempDir() + if err := os.MkdirAll(filepath.Join(dir, claudeDirName), 0o755); err != nil { + t.Fatal(err) + } + root, err := worktreedir.OpenAt(dir) + if err != nil { + t.Fatal(err) + } + + if name, outcome := scanForSymlinkedComponent(root, claudeDirName+"/settings.json"); outcome != componentScanClean { + t.Errorf("outcome = %v (%q), want componentScanClean", outcome, name) + } +} + +// TestComponentHasExpectedShape pins each mode combination at both positions, +// including the Windows shapes that must not be rejected: a bare +// fs.ModeIrregular is how Go reports a directory junction, and +// ModeDir|ModeIrregular a cloud placeholder directory. +func TestComponentHasExpectedShape(t *testing.T) { + t.Parallel() + + for _, tc := range []struct { + mode fs.FileMode + wantLeaf, wantInner bool + }{ + {mode: fs.ModeDir, wantLeaf: false, wantInner: true}, + {mode: fs.ModeIrregular, wantLeaf: true, wantInner: false}, + {mode: fs.ModeDir | fs.ModeIrregular, wantLeaf: false, wantInner: true}, + {mode: 0, wantLeaf: true, wantInner: false}, + {mode: fs.ModeNamedPipe, wantLeaf: false, wantInner: false}, + {mode: fs.ModeSocket, wantLeaf: false, wantInner: false}, + {mode: fs.ModeDevice, wantLeaf: false, wantInner: false}, + {mode: fs.ModeDevice | fs.ModeCharDevice, wantLeaf: false, wantInner: false}, + {mode: fs.ModeSymlink, wantLeaf: false, wantInner: false}, + } { + if got := componentHasExpectedShape(tc.mode, true); got != tc.wantLeaf { + t.Errorf("componentHasExpectedShape(%v, leaf) = %v, want %v", tc.mode, got, tc.wantLeaf) + } + if got := componentHasExpectedShape(tc.mode, false); got != tc.wantInner { + t.Errorf("componentHasExpectedShape(%v, inner) = %v, want %v", tc.mode, got, tc.wantInner) + } + } +} diff --git a/cmd/entire/cli/osroot/osroot.go b/cmd/entire/cli/osroot/osroot.go index 681154b9a7..62b2e669bd 100644 --- a/cmd/entire/cli/osroot/osroot.go +++ b/cmd/entire/cli/osroot/osroot.go @@ -401,10 +401,9 @@ func RemoveAllNoSymlinks(root *os.Root, name string) error { return err } defer closeParent() - if err := parent.RemoveAll(leaf); err != nil { - return fmt.Errorf("remove %s: %w", name, err) - } - return nil + // Unwrapped, like RemoveNoSymlinks directly above: the one caller already + // names the directory, and wrapping here put it in the message twice. + return parent.RemoveAll(leaf) //nolint:wrapcheck // see comment } // WalkDirNoSymlinks walks dir within root, refusing a symlink anywhere it goes: diff --git a/cmd/entire/cli/paths/entiredir.go b/cmd/entire/cli/paths/entiredir.go index 620f07eb88..4691734267 100644 --- a/cmd/entire/cli/paths/entiredir.go +++ b/cmd/entire/cli/paths/entiredir.go @@ -78,7 +78,7 @@ func ValidateEntireDirAt(worktreeRoot string) error { case err != nil: return fmt.Errorf("%s %w: %w", path, ErrEntireDirUnreadable, err) case !info.Mode().IsDir(): - return fmt.Errorf("%s is %s, %w", path, describeMode(info.Mode()), ErrEntireDirNotDirectory) + return fmt.Errorf("%s is %s, %w", path, DescribeMode(info.Mode()), ErrEntireDirNotDirectory) } return validateEntireDirEntries(path) @@ -202,7 +202,7 @@ func unsupportedEntryError(path string, mode fs.FileMode) error { if mode&fs.ModeSymlink != 0 { return SymlinkedEntryError(path) } - return fmt.Errorf("%s is %s, %w", path, describeMode(mode), ErrEntireDirUnsupportedEntry) + return fmt.Errorf("%s is %s, %w", path, DescribeMode(mode), ErrEntireDirUnsupportedEntry) } // SymlinkedEntryError reports that path is a symbolic link, naming the target @@ -266,7 +266,11 @@ func RequireEntireDir(ctx context.Context) error { // describeMode names what was found. The sentinel supplies the rest of the // sentence, so these read as the first half of "X is a symbolic link, not a // directory" or "X is a named pipe, not a regular file or directory". -func describeMode(mode fs.FileMode) string { +// DescribeMode names a file type for a diagnostic ("a named pipe"). Exported so +// doctor's agent-path scan reports a wrong-typed component in the same words +// this package's .entire scan uses, rather than growing a second vocabulary for +// the same conditions. +func DescribeMode(mode fs.FileMode) string { switch { case mode&fs.ModeSymlink != 0: return "a symbolic link" diff --git a/cmd/entire/cli/setup.go b/cmd/entire/cli/setup.go index 7435e68a96..f354f9693e 100644 --- a/cmd/entire/cli/setup.go +++ b/cmd/entire/cli/setup.go @@ -2354,36 +2354,97 @@ func promptTelemetryConsent(settings *EntireSettings, telemetryFlag bool) error return nil } +// worktreeFileName reports the name to read a working-tree file by, following a +// symlink whose target stays inside the worktree and refusing one that leaves +// it. An empty name means the file is not there — a separate bool would be the +// same fact twice, which is what the callers below test. +// +// The two-step exists because os.Root refuses an ABSOLUTE symlink target +// unconditionally — including one resolving inside the root — with an error that +// is not os.ErrNotExist. A repo pointing vercel.json at a monorepo's shared +// config with an absolute link would therefore be reported as unreadable and +// skipped, dropping the feature for a setup that worked before the anchor went +// in. Resolving and re-checking containment is what actually delivers "follow a +// link that stays inside, refuse one that leaves"; the retried read still goes +// through the root at a worktree-relative name. +// +// Only for files that are the USER's. Entire's own trees refuse a link either +// way and must keep using the root directly. +func worktreeFileName(worktreeRoot string, root *os.Root, name string) (string, error) { + _, err := root.Stat(name) + if err == nil { + return name, nil + } + if os.IsNotExist(err) { + return "", nil + } + + resolved, resolveErr := worktreedir.NameFollowingLinks(worktreeRoot, name) + switch { + case errors.Is(resolveErr, os.ErrNotExist): + // A dangling link reads as absent, which is what os.Stat gave before. + return "", nil + case resolveErr != nil: + // The root's refusal stays the wrapped cause: that is the condition the + // user has to act on ("path escapes from parent"), while resolveErr only + // says the fallback did not apply. It is still worth carrying, since a + // resolve that failed for its own reason — a permission denied part-way + // down the link chain — is otherwise invisible. + return "", fmt.Errorf("check %s: %w (resolving the link: %w)", name, err, resolveErr) + } + // EvalSymlinks stats every component, so a successful resolve already proved + // the target is there. This re-stat only closes the window between the two, + // and a failure in it is a race rather than a state worth reading as absent. + if _, err := root.Stat(resolved); err != nil { + return "", fmt.Errorf("check %s: %w", resolved, err) + } + return resolved, nil +} + +// loadVercelConfigIfPresent reads the config only when there is one to read. +// The project can be detected from `.vercel` or `vercel.ts` with no vercel.json +// beside them, and an empty name is that case rather than a path to try. +func loadVercelConfigIfPresent(root *os.Root, name string) (map[string]any, bool, error) { + if name == "" { + return nil, false, nil + } + return vercelconfig.LoadIn(root, name) //nolint:wrapcheck // the caller only tests for nil +} + func maybePromptVercelDeploymentDisable(ctx context.Context, w io.Writer, targetFile string, promptFn func() (bool, error)) (bool, error) { repoRoot, rootErr := paths.WorktreeRoot(ctx) if rootErr == nil { // Through the worktree's root: these are working-tree files, so they // arrive by clone, and a joined path handed to os.Stat/os.ReadFile // resolves wherever a checked-in symlink points. In-repo links are - // still followed, which is what a monorepo's shared vercel.json needs. + // still followed, which is what a monorepo's shared vercel.json needs — + // see worktreeFileName for why the root alone does not give that. worktree, err := worktreedir.OpenAt(repoRoot) if err != nil { fmt.Fprintf(w, "Note: Skipping Vercel deployment update: could not open the worktree: %v\n", err) return false, nil } - hasVercelJSON := false - if _, err := worktree.Stat(vercelconfig.FileName); err == nil { - hasVercelJSON = true - } else if !os.IsNotExist(err) { + // vercelJSONName is empty exactly when vercel.json is absent, so it is + // both the name to read by and the presence flag; a second bool would be + // the same fact twice. + vercelJSONName, err := worktreeFileName(repoRoot, worktree, vercelconfig.FileName) + if err != nil { fmt.Fprintf(w, "Note: Skipping Vercel deployment update: could not check %s: %v\n", vercelconfig.FileName, err) return false, nil } - hasVercelProject := hasVercelJSON + hasVercelProject := vercelJSONName != "" if !hasVercelProject { for _, name := range []string{".vercel", "vercel.ts"} { - if _, err := worktree.Stat(name); err == nil { + found, statErr := worktreeFileName(repoRoot, worktree, name) + if statErr != nil { + fmt.Fprintf(w, "Note: Skipping Vercel deployment update: could not check %s: %v\n", name, statErr) + return false, nil + } + if found != "" { hasVercelProject = true break - } else if !os.IsNotExist(err) { - fmt.Fprintf(w, "Note: Skipping Vercel deployment update: could not check %s: %v\n", name, err) - return false, nil } } } @@ -2406,7 +2467,7 @@ func maybePromptVercelDeploymentDisable(ctx context.Context, w io.Writer, target return false, nil } - if config, alreadyDisabled, loadErr := vercelconfig.LoadIn(worktree, vercelconfig.FileName); loadErr == nil && + if config, alreadyDisabled, loadErr := loadVercelConfigIfPresent(worktree, vercelJSONName); loadErr == nil && config != nil && alreadyDisabled { targetSettings.Vercel = true if err := saveSettingsToTarget(ctx, targetSettings, targetFile); err != nil { diff --git a/cmd/entire/cli/setup_agent_help_skill.go b/cmd/entire/cli/setup_agent_help_skill.go index e4b15cfb98..d6c0bbd49e 100644 --- a/cmd/entire/cli/setup_agent_help_skill.go +++ b/cmd/entire/cli/setup_agent_help_skill.go @@ -86,17 +86,39 @@ func reportAgentHelpSkillScaffold(w io.Writer, ag agent.Agent, result managedSca } } +// agentHelpSkillTemplatePath is where the agent-help skill goes for an agent, or +// "" for one that gets none. Split out from agentHelpSkillTemplate for the same +// reason as searchSkillTemplatePath: doctor's symlink scan wants the path, not +// the file. +func agentHelpSkillTemplatePath(agentName types.AgentName) string { + switch agentName { + case agent.AgentNameClaudeCode: + return filepath.Join(claudeDirName, "skills", "entire", "SKILL.md") + case agent.AgentNameCodex: + return filepath.Join(".codex", "agents", "entire.toml") + case agent.AgentNameGemini: + return filepath.Join(".gemini", "agents", "entire.md") + default: + return "" + } +} + func agentHelpSkillTemplate(agentName types.AgentName) (string, []byte, bool) { + // One switch, so a fourth agent cannot get a path with no body or a body + // with no path — which is the failure splitting the path out would otherwise + // introduce. + var content string switch agentName { case agent.AgentNameClaudeCode: - return filepath.Join(".claude", "skills", "entire", "SKILL.md"), []byte(strings.TrimSpace(claudeAgentHelpSkillTemplate) + "\n"), true + content = claudeAgentHelpSkillTemplate case agent.AgentNameCodex: - return filepath.Join(".codex", "agents", "entire.toml"), []byte(strings.TrimSpace(codexAgentHelpSkillTemplate) + "\n"), true + content = codexAgentHelpSkillTemplate case agent.AgentNameGemini: - return filepath.Join(".gemini", "agents", "entire.md"), []byte(strings.TrimSpace(geminiAgentHelpSkillTemplate) + "\n"), true + content = geminiAgentHelpSkillTemplate default: return "", nil, false } + return agentHelpSkillTemplatePath(agentName), []byte(strings.TrimSpace(content) + "\n"), true } // agentHelpSkillBody is the shared, format-agnostic instruction body for the diff --git a/cmd/entire/cli/setup_search_skill.go b/cmd/entire/cli/setup_search_skill.go index 9160023a6a..9a60ef66c7 100644 --- a/cmd/entire/cli/setup_search_skill.go +++ b/cmd/entire/cli/setup_search_skill.go @@ -90,7 +90,7 @@ func isManagedSearchSkill(data []byte) bool { func legacySearchSubagentPath(agentName types.AgentName) string { switch agentName { case agent.AgentNameClaudeCode: - return filepath.Join(".claude", "agents", strategy.EntireSearchSubagentName+".md") + return filepath.Join(claudeDirName, "agents", strategy.EntireSearchSubagentName+".md") case agent.AgentNameCodex: return filepath.Join(".codex", "agents", strategy.EntireSearchSubagentName+".toml") case agent.AgentNameGemini: @@ -168,9 +168,17 @@ func reportSearchSkillScaffold(w io.Writer, ag agent.Agent, result managedScaffo } } -// searchSkillTemplate maps each agent to its documented project-level Agent -// Skills directory. Every agent shares one SKILL.md body; the skill directory -// is named after strategy.EntireSearchSubagentName — the value the +// claudeDirName is Claude Code's project directory. Named because the scaffold +// path, the legacy subagent path and doctor's tests all reach for it. +const claudeDirName = ".claude" + +// searchSkillTemplatePath maps each agent to its documented project-level +// Agent Skills directory, or "" for one that gets no skill. Split out from +// searchSkillTemplate so a caller that wants only the location — doctor's +// symlink scan asks for one per agent — does not trim and copy a multi-KB +// template body to get it. +// +// The skill directory is named after strategy.EntireSearchSubagentName — the value the // commit-condensed telemetry probe matches legacy subagent dispatches against // and the skill identity telemetry recognizes. // TestSearchSkillTemplates_NameMatchesTelemetryProbe pins that, so renaming @@ -191,11 +199,11 @@ func reportSearchSkillScaffold(w io.Writer, ag agent.Agent, result managedScaffo // protected agent root do not — and that asymmetry is accepted rather than // papered over: adding .agents (a shared, user-authored skills directory) to // ProtectedDirs would hide the user's own skills from checkpoints repo-wide. -func searchSkillTemplate(agentName types.AgentName) (string, []byte, bool) { +func searchSkillTemplatePath(agentName types.AgentName) string { var root string switch agentName { case agent.AgentNameClaudeCode: - root = ".claude" + root = claudeDirName case agent.AgentNameCodex: root = ".agents" case agent.AgentNameCopilotCLI: @@ -211,9 +219,18 @@ func searchSkillTemplate(agentName types.AgentName) (string, []byte, bool) { case agent.AgentNamePi: root = ".pi" default: + return "" + } + return filepath.Join(root, "skills", strategy.EntireSearchSubagentName, "SKILL.md") +} + +// searchSkillTemplate is searchSkillTemplatePath plus the SKILL.md body every +// agent shares. +func searchSkillTemplate(agentName types.AgentName) (string, []byte, bool) { + relPath := searchSkillTemplatePath(agentName) + if relPath == "" { return "", nil, false } - relPath := filepath.Join(root, "skills", strategy.EntireSearchSubagentName, "SKILL.md") return relPath, []byte(strings.TrimSpace(searchSkillTemplateContent) + "\n"), true } diff --git a/cmd/entire/cli/setup_test.go b/cmd/entire/cli/setup_test.go index 7c3a1f85d8..4c51715678 100644 --- a/cmd/entire/cli/setup_test.go +++ b/cmd/entire/cli/setup_test.go @@ -5205,3 +5205,112 @@ func TestConfigureCmd_SummarizeProvider_ExternalLocalOnlyRepo_GrantSurvives(t *t reason, stdout.String()) } } + +// TestWorktreeFileName covers the shapes vercel.json can arrive in. The +// absolute-in-repo row is the regression the helper exists for: os.Root reports +// `vercel.json -> /abs/path/inside/repo/shared/vercel.json` as "path escapes +// from parent", which is not os.ErrNotExist, so detection printed a note and +// skipped — silently dropping the feature for a monorepo setup that worked +// before the anchor went in. +// +// worktreedir.TestNameFollowingLinks asserts the link cases one layer down; +// this table is the caller's view, plus the rows that never reach the resolve. +func TestWorktreeFileName(t *testing.T) { + t.Parallel() + + const name = "vercel.json" + for _, tc := range []struct { + desc string + link func(t *testing.T, dir string) // nil: a real file, no link + wantName string // "" means absent + wantErr bool + }{ + { + desc: "a real file is read by its own name", + wantName: name, + }, + { + desc: "an absolute link inside the worktree resolves to its target", + link: func(t *testing.T, dir string) { + linkTo(t, dir, filepath.Join(dir, "shared", name)) + }, + wantName: "shared/vercel.json", + }, + { + // os.Root follows a RELATIVE link that stays inside it, so the fast + // path succeeds and the original name is what to read by. Only an + // absolute target reaches the resolve, which is the whole asymmetry + // this helper exists for. + desc: "a relative link inside the worktree needs no resolving", + link: func(t *testing.T, dir string) { + linkTo(t, dir, filepath.Join("shared", name)) + }, + wantName: name, + }, + { + desc: "a link out of the worktree is refused, not followed", + link: func(t *testing.T, dir string) { + outside := filepath.Join(t.TempDir(), name) + if err := os.WriteFile(outside, []byte("{}"), 0o644); err != nil { + t.Fatal(err) + } + linkTo(t, dir, outside) + }, + wantErr: true, + }, + { + desc: "a dangling link reads as absent, as os.Stat gave before", + link: func(t *testing.T, dir string) { + linkTo(t, dir, filepath.Join(dir, "missing.json")) + }, + }, + { + desc: "an absent file reads as absent", + link: func(*testing.T, string) {}, // no file at all + }, + } { + t.Run(tc.desc, func(t *testing.T) { + t.Parallel() + + dir := t.TempDir() + shared := filepath.Join(dir, "shared") + if err := os.MkdirAll(shared, 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(shared, name), []byte("{}"), 0o644); err != nil { + t.Fatal(err) + } + if tc.link == nil { + if err := os.WriteFile(filepath.Join(dir, name), []byte("{}"), 0o644); err != nil { + t.Fatal(err) + } + } else { + tc.link(t, dir) + } + + root, err := os.OpenRoot(dir) + if err != nil { + t.Fatal(err) + } + defer root.Close() + + gotName, gotErr := worktreeFileName(dir, root, name) + if (gotErr != nil) != tc.wantErr { + t.Fatalf("worktreeFileName() error = %v, wantErr %v", gotErr, tc.wantErr) + } + if gotName != tc.wantName { + t.Errorf("worktreeFileName() = %q, want %q", gotName, tc.wantName) + } + }) + } +} + +// linkTo symlinks the vercel.json under test inside dir to target, skipping +// where symlinks need privileges. +func linkTo(t *testing.T, dir, target string) { + t.Helper() + testutil.SkipWithoutSymlinks(t) + if err := os.Symlink(target, filepath.Join(dir, "vercel.json")); err != nil { + t.Fatal(err) + } +} diff --git a/cmd/entire/cli/testutil/testutil.go b/cmd/entire/cli/testutil/testutil.go index c87f08ee55..9eb5234aa0 100644 --- a/cmd/entire/cli/testutil/testutil.go +++ b/cmd/entire/cli/testutil/testutil.go @@ -5,6 +5,7 @@ package testutil import ( "os" "path/filepath" + "runtime" "testing" "time" @@ -59,6 +60,19 @@ func InitRepo(t *testing.T, repoDir string) { } } +// SkipWithoutSymlinks skips a test that needs to create a symlink. On Windows +// that takes elevation or developer mode, neither of which CI has. +// +// Here rather than per-package because six copies in three different wordings +// had accumulated across cli, agent, worktreedir and vercelconfig tests, so +// there was nowhere to make the change when a runner does gain the privilege. +func SkipWithoutSymlinks(t *testing.T) { + t.Helper() + if runtime.GOOS == "windows" { + t.Skip("symlink creation needs elevation on Windows") + } +} + // WriteFile creates a file with the given content in the repo directory. // It creates parent directories as needed. func WriteFile(t *testing.T, repoDir, path, content string) { diff --git a/cmd/entire/cli/vercelconfig/vercelconfig.go b/cmd/entire/cli/vercelconfig/vercelconfig.go index 0bf3113f8e..c437a1404d 100644 --- a/cmd/entire/cli/vercelconfig/vercelconfig.go +++ b/cmd/entire/cli/vercelconfig/vercelconfig.go @@ -79,11 +79,15 @@ const maxConfigBytes = 1 << 20 // LoadIn reads the Vercel config file named inside root, if present. // // Through the worktree's root rather than a path joined onto the repo root: -// vercel.json is a working-tree file, so it arrives by clone. A symlink that -// stays inside the repository is still followed — pointing vercel.json at a -// monorepo's shared config is a real setup, and this file is the user's, not -// Entire's — while one leaving the worktree is refused, which is the property -// the worktreedir anchor exists to give. +// vercel.json is a working-tree file, so it arrives by clone. A link leaving the +// worktree is refused, which is the property the worktreedir anchor exists to +// give. +// +// Following an in-repo link is the CALLER's job, and it has to be: os.Root +// refuses an absolute symlink target even when it resolves inside the root, so +// name is expected to be one the caller already resolved (see setup.go's +// worktreeFileName). Passing the raw file name still works for the ordinary +// case of a real file or a relative link. func LoadIn(root *os.Root, name string) (map[string]any, bool, error) { f, err := root.Open(name) if err != nil { diff --git a/cmd/entire/cli/vercelconfig/vercelconfig_test.go b/cmd/entire/cli/vercelconfig/vercelconfig_test.go index f841a4fc8c..eb00458def 100644 --- a/cmd/entire/cli/vercelconfig/vercelconfig_test.go +++ b/cmd/entire/cli/vercelconfig/vercelconfig_test.go @@ -1,10 +1,13 @@ package vercelconfig import ( + "errors" "os" "path/filepath" "strings" "testing" + + "github.com/entireio/cli/cmd/entire/cli/testutil" ) func openRoot(t *testing.T, dir string) *os.Root { @@ -108,3 +111,53 @@ func TestLoadIn(t *testing.T) { } }) } + +// TestLoadIn_AbsoluteInRepoSymlink documents what a root alone cannot do, which +// is why the caller resolves the name first. os.Root refuses an absolute +// symlink target unconditionally — even one landing inside the root — so +// pointing vercel.json at a monorepo's shared config with an absolute link +// reaches LoadIn as an error that is NOT os.ErrNotExist. +func TestLoadIn_AbsoluteInRepoSymlink(t *testing.T) { + t.Parallel() + + testutil.SkipWithoutSymlinks(t) + + dir := t.TempDir() + shared := filepath.Join(dir, "shared") + if err := os.MkdirAll(shared, 0o755); err != nil { + t.Fatal(err) + } + target := filepath.Join(shared, FileName) + if err := os.WriteFile(target, []byte(`{"git":{"deploymentEnabled":{"entire/**":false}}}`), 0o644); err != nil { + t.Fatal(err) + } + // Absolute, and resolving inside the worktree. + if err := os.Symlink(target, filepath.Join(dir, FileName)); err != nil { + t.Fatal(err) + } + + root, err := os.OpenRoot(dir) + if err != nil { + t.Fatal(err) + } + defer root.Close() + + // The raw name is refused, and not as "absent" — which is what made this + // silently drop the feature rather than fall through. + _, _, err = LoadIn(root, FileName) + if err == nil { + t.Fatalf("LoadIn(%q) succeeded; os.Root is expected to refuse an absolute link", FileName) + } + if errors.Is(err, os.ErrNotExist) { + t.Errorf("LoadIn error = %v, want something other than os.ErrNotExist", err) + } + + // The resolved name is what the caller passes, and it reads. + config, disabled, err := LoadIn(root, "shared/"+FileName) + if err != nil { + t.Fatalf("LoadIn(resolved) error = %v", err) + } + if config == nil || !disabled { + t.Errorf("LoadIn(resolved) = %v, disabled=%v; want the shared config read", config, disabled) + } +} diff --git a/cmd/entire/cli/worktreedir/worktreedir.go b/cmd/entire/cli/worktreedir/worktreedir.go index 3966487f0f..bf4940ec18 100644 --- a/cmd/entire/cli/worktreedir/worktreedir.go +++ b/cmd/entire/cli/worktreedir/worktreedir.go @@ -62,22 +62,23 @@ func OpenAt(worktreeRoot string) (*os.Root, error) { // absolute ones from them; this is the single conversion back, so callers stop // joining a root path onto a git path and reading the result. func Name(worktreeRoot, p string) (string, error) { - // VolumeName alongside IsAbs, the pairing SessionStore.Name, - // validation.ValidateSessionID and gitrepo's alternates checks already use. - // A Windows drive-relative path like "C:foo" contains no separator and IsAbs - // reports false, so it would otherwise be taken as a name inside the - // worktree — while filepath.Join drops the base directory when the appended - // element carries a volume. Sending it down the absolute branch makes - // filepath.Rel reject it across volumes, which is the answer this - // containment check exists to give. No-op on Unix, where VolumeName is - // always empty. + // filepath.IsLocal, the single primitive os.Root itself uses one layer down, + // rather than an IsAbs/VolumeName pair. Windows has two forms that are + // neither absolute nor volume-prefixed yet do not name anything inside the + // worktree: the drive-relative "C:foo", which IsAbs reports false for while + // filepath.Join drops the base directory when the appended element carries a + // volume; and the rooted-relative "\foo", where volumeNameLen is 0 because a + // single leading backslash is neither a volume nor a UNC prefix. Sending + // both down the absolute branch makes filepath.Rel reject them, which is the + // answer this containment check exists to give. IsLocal also rejects "..", + // which the traversal check below covers anyway, and the Windows reserved + // device names, which nothing here wants either. // - // Go's os.Root rejects such a name one layer down (every name goes through - // filepathlite.IsLocal), so this is not a reachable escape today. It is - // closed anyway because Name exists to answer "is this inside the worktree?" - // independently of the caller, and readWorktreeFileSafely feeds it paths - // that arrive from the API. - if !filepath.IsAbs(p) && filepath.VolumeName(p) == "" { + // os.Root rejects such a name one layer down, so neither is a reachable + // escape today. They are closed anyway because Name exists to answer "is + // this inside the worktree?" independently of the caller, and + // readWorktreeFileSafely feeds it paths that arrive from the API. + if filepath.IsLocal(p) { cleaned := filepath.ToSlash(filepath.Clean(filepath.FromSlash(p))) if cleaned == "." || paths.IsRelativeTraversal(cleaned) { return "", fmt.Errorf("%q does not name a file in the worktree", p) @@ -94,3 +95,45 @@ func Name(worktreeRoot, p string) (string, error) { } return filepath.ToSlash(rel), nil } + +// NameFollowingLinks is Name for a path that may itself be, or sit below, a +// symlink: it resolves the link first and then answers for the target. +// +// os.Root refuses an ABSOLUTE symlink target unconditionally, including one +// resolving inside the root, so a root alone cannot express "follow a link that +// stays in the worktree, refuse one that leaves". That distinction is what a +// user-owned working-tree file needs — pointing vercel.json at a monorepo's +// shared config is a real setup, and it is written absolute as often as +// relative — while Entire's own trees (.entire, an agent's hook config) refuse +// a link either way and must not use this. +// +// The name returned is worktree-relative and symlink-free, so the caller's read +// still goes through the root: a link repointed between the resolve and the read +// changes which in-worktree file is read and cannot escape the worktree. +// +// A dangling link reports os.ErrNotExist, matching what os.Stat gives for one. +func NameFollowingLinks(worktreeRoot, p string) (string, error) { + base, err := filepath.Abs(worktreeRoot) + if err != nil { + return "", fmt.Errorf("resolve %s: %w", worktreeRoot, err) + } + target := p + if !filepath.IsAbs(target) { + target = filepath.Join(base, filepath.FromSlash(target)) + } + resolved, err := filepath.EvalSymlinks(target) + if err != nil { + return "", fmt.Errorf("resolve %s: %w", p, err) + } + // The BASE has to be resolved too, against the same rules. Otherwise every + // repository living below a symlinked component is judged from a path that + // no longer matches its own resolved children: on macOS /var is a link to + // /private/var, so a worktree under /var/folders/... would have each of its + // own files reported as outside itself. The relative answer is unaffected by + // which spelling the caller's root was opened under. + resolvedBase, err := filepath.EvalSymlinks(base) + if err != nil { + return "", fmt.Errorf("resolve %s: %w", worktreeRoot, err) + } + return Name(resolvedBase, resolved) +} diff --git a/cmd/entire/cli/worktreedir/worktreedir_test.go b/cmd/entire/cli/worktreedir/worktreedir_test.go index d0f7f17625..661bfbe10e 100644 --- a/cmd/entire/cli/worktreedir/worktreedir_test.go +++ b/cmd/entire/cli/worktreedir/worktreedir_test.go @@ -1,8 +1,12 @@ package worktreedir import ( + "errors" + "os" "path/filepath" "testing" + + "github.com/entireio/cli/cmd/entire/cli/testutil" ) func TestName(t *testing.T) { @@ -46,24 +50,28 @@ func TestName(t *testing.T) { }) } - // "C:foo" is drive-relative on Windows: separator-free, and IsAbs reports - // false, so pairing IsAbs with VolumeName is what keeps it off the name - // branch — filepath.Join would otherwise drop the base directory. On Unix - // the same string is an ordinary relative filename and must be accepted, - // which is why this asserts each platform's answer rather than skipping. - t.Run("drive-relative path", func(t *testing.T) { - t.Parallel() - got, err := Name(root, "C:foo") - if filepath.VolumeName("C:foo") != "" { - if err == nil { - t.Errorf("Name(\"C:foo\") = %q, want error on a volume-aware platform", got) + // Windows has two forms that are neither absolute nor volume-prefixed yet + // name nothing inside the worktree: "C:foo" (drive-relative, separator-free) + // and "\\foo" (rooted-relative, where volumeNameLen is 0 because a single + // leading backslash is neither a volume nor a UNC prefix). filepath.IsLocal + // is what keeps both off the name branch. On Unix each is an ordinary + // relative filename and must be accepted, which is why this asserts each + // platform's answer rather than skipping. + for _, p := range []string{"C:foo", `\foo\bar`} { + t.Run("non-local path "+p, func(t *testing.T) { + t.Parallel() + got, err := Name(root, p) + if !filepath.IsLocal(p) { + if err == nil { + t.Errorf("Name(%q) = %q, want error on a volume-aware platform", p, got) + } + return } - return - } - if err != nil || got != "C:foo" { - t.Errorf("Name(\"C:foo\") = %q, %v; want it kept as a plain filename", got, err) - } - }) + if err != nil { + t.Errorf("Name(%q) = %q, %v; want it kept as a plain relative name", p, got, err) + } + }) + } t.Run("rejects an absolute path outside the worktree", func(t *testing.T) { t.Parallel() @@ -73,3 +81,83 @@ func TestName(t *testing.T) { } }) } + +// TestNameFollowingLinks covers the property Name alone cannot give: os.Root +// refuses an absolute symlink target even when it resolves inside the root, so +// a user-owned working-tree file pointed at a monorepo's shared config with an +// absolute link needs the link resolved before containment is judged. +func TestNameFollowingLinks(t *testing.T) { + t.Parallel() + + root := t.TempDir() + shared := filepath.Join(root, "shared") + if err := os.MkdirAll(shared, 0o755); err != nil { + t.Fatal(err) + } + target := filepath.Join(shared, "vercel.json") + if err := os.WriteFile(target, []byte("{}"), 0o644); err != nil { + t.Fatal(err) + } + + t.Run("absolute link inside the worktree resolves to its target", func(t *testing.T) { + t.Parallel() + link := filepath.Join(root, "abs.json") + testutil.SkipWithoutSymlinks(t) + if err := os.Symlink(target, link); err != nil { + t.Fatal(err) + } + got, err := NameFollowingLinks(root, "abs.json") + if err != nil { + t.Fatalf("NameFollowingLinks() error = %v", err) + } + if got != "shared/vercel.json" { + t.Errorf("NameFollowingLinks() = %q, want shared/vercel.json", got) + } + }) + + t.Run("relative link inside the worktree resolves too", func(t *testing.T) { + t.Parallel() + link := filepath.Join(root, "rel.json") + testutil.SkipWithoutSymlinks(t) + if err := os.Symlink(filepath.Join("shared", "vercel.json"), link); err != nil { + t.Fatal(err) + } + got, err := NameFollowingLinks(root, "rel.json") + if err != nil { + t.Fatalf("NameFollowingLinks() error = %v", err) + } + if got != "shared/vercel.json" { + t.Errorf("NameFollowingLinks() = %q, want shared/vercel.json", got) + } + }) + + t.Run("link leaving the worktree is refused", func(t *testing.T) { + t.Parallel() + outside := t.TempDir() + away := filepath.Join(outside, "vercel.json") + if err := os.WriteFile(away, []byte("{}"), 0o644); err != nil { + t.Fatal(err) + } + link := filepath.Join(root, "away.json") + testutil.SkipWithoutSymlinks(t) + if err := os.Symlink(away, link); err != nil { + t.Fatal(err) + } + if got, err := NameFollowingLinks(root, "away.json"); err == nil { + t.Errorf("NameFollowingLinks() = %q, want error for a link out of the worktree", got) + } + }) + + t.Run("dangling link reports not-exist", func(t *testing.T) { + t.Parallel() + link := filepath.Join(root, "dangling.json") + testutil.SkipWithoutSymlinks(t) + if err := os.Symlink(filepath.Join(root, "missing.json"), link); err != nil { + t.Fatal(err) + } + _, err := NameFollowingLinks(root, "dangling.json") + if !errors.Is(err, os.ErrNotExist) { + t.Errorf("NameFollowingLinks() error = %v, want os.ErrNotExist", err) + } + }) +}