Expand on os.Root coverage - #2220
Conversation
|
@cursor review |
There was a problem hiding this comment.
Pull request overview
This PR continues the follow-up work from #2180 by expanding os.Root anchor coverage across the codebase, migrating remaining worktree / git-dir filesystem operations onto anchored helpers, and making symlink-refusal conditions visible via entire doctor—notably for agent hook/config directories (including auto-detected Pi).
Changes:
- Migrate Pi’s extension install/uninstall/read paths to
agent.HookConfigFile(anchored worktree I/O), addHookConfigLocator, and add safe directory removal (RemoveDir,RemoveAllNoSymlinks). - Anchor remaining filesystem call sites (Vercel config reads, persistent-ref lock files in git common dir, capped worktree file reads) and strengthen Windows path containment (
worktreedir.Name). - Extend
doctorto diagnose symlinked agent config/scaffold paths (plus tests and guard coverage).
Reviewed changes
Copilot reviewed 29 out of 29 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| cmd/entire/cli/worktreedir/worktreedir.go | Strengthen Name containment on Windows by pairing IsAbs with VolumeName. |
| cmd/entire/cli/worktreedir/worktreedir_test.go | Add unit tests covering relative/absolute containment and drive-relative behavior. |
| cmd/entire/cli/vercelconfig/vercelconfig.go | Replace path-based read with LoadIn(*os.Root, name) plus a 1MiB read cap. |
| cmd/entire/cli/vercelconfig/vercelconfig_test.go | Add tests for missing/malformed/oversized configs and symlink behavior. |
| cmd/entire/cli/setup.go | Route Vercel presence checks and config reads through the worktree root. |
| cmd/entire/cli/runner_gather.go | Bound worktree file reads at the read layer (not just truncation of output). |
| cmd/entire/cli/osroot/osroot.go | Add RemoveAllNoSymlinks to refuse symlinked parent components during recursive deletes. |
| cmd/entire/cli/osroot/osroot_test.go | Add coverage for RemoveAllNoSymlinks behavior (real tree, missing, symlink parent/leaf). |
| cmd/entire/cli/doctor.go | Add checkAgentDirSymlinks and supporting helpers to report symlinked agent paths. |
| cmd/entire/cli/doctor_test.go | Add tests for new doctor diagnostics and candidate-path coverage/behavior. |
| cmd/entire/cli/checkpoint/persistent_ref_update.go | Move persistent-ref lock creation/acquire under gitdir root + AcquireContextIn. |
| cmd/entire/cli/checkpoint/persistent_ref_update_test.go | Update tests to use rooted lock acquisition. |
| cmd/entire/cli/agent/registry.go | Add AllHookConfigRelPaths to enumerate locator paths from the agent registry. |
| cmd/entire/cli/agent/pi/hooks.go | Migrate Pi to anchored hook-config operations; add HookConfigRelPath; remove unrooted drift checker usage. |
| cmd/entire/cli/agent/pi/hooks_test.go | Add tests pinning symlink refusal and directory-uninstall behavior for Pi. |
| cmd/entire/cli/agent/opencode/hooks.go | Implement HookConfigLocator and centralize rel-path declaration. |
| cmd/entire/cli/agent/hook_config_file.go | Add HookConfigLocator and HookConfigFile.RemoveDir for Pi’s directory-scoped uninstall. |
| cmd/entire/cli/agent/hook_command.go | Remove path-based GeneratedHookFileState; keep content-based drift helper. |
| cmd/entire/cli/agent/geminicli/hooks.go | Implement HookConfigLocator and centralize rel-path declaration. |
| cmd/entire/cli/agent/factoryaidroid/hooks.go | Implement HookConfigLocator and centralize rel-path declaration. |
| cmd/entire/cli/agent/cursor/hooks.go | Implement HookConfigLocator and centralize rel-path declaration. |
| cmd/entire/cli/agent/copilotcli/lifecycle.go | Add compile-time assertion for HookConfigLocator. |
| cmd/entire/cli/agent/copilotcli/hooks.go | Implement HookConfigLocator and centralize rel-path declaration. |
| cmd/entire/cli/agent/codex/lifecycle.go | Add compile-time assertion for HookConfigLocator. |
| cmd/entire/cli/agent/codex/hooks.go | Implement HookConfigLocator and centralize rel-path declaration. |
| cmd/entire/cli/agent/codex/hook_root.go | Use the declared HookConfigRelPath for worktree config resolution. |
| cmd/entire/cli/agent/claudecode/hooks.go | Implement HookConfigLocator and centralize rel-path declaration. |
| cmd/entire/cli/agent_hook_config_guard_test.go | Add a source-level guard test ensuring every OpenHookConfig user declares a locator path. |
| CLAUDE.md | Update architecture docs to reflect expanded anchored coverage and diagnostics. |
Suppressed comments (1)
cmd/entire/cli/agent_hook_config_guard_test.go:38
- For the same reason as the rev-parse call above,
git grepshould also run with repo-override env vars scrubbed; otherwise it can search a different repository than grep.Dir points at when GIT_DIR/GIT_WORK_TREE/GIT_INDEX_FILE are set.
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))
out, grepErr := grep.Output()
require.NoError(t, grepErr, "no agent calls agent.OpenHookConfig, which cannot be right")
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Pi was the one registered agent whose hook-config surface never moved onto agent.HookConfigFile. It joined its path onto the repo root and called os.ReadFile / os.MkdirAll / os.WriteFile / os.RemoveAll on the result, so a repository shipping `.pi -> /elsewhere` got arbitrary file creation outside the worktree from InstallHooks and recursive deletion of an arbitrary directory from UninstallHooks, while AreHooksInstalled read the far end as its own answer. It needed no cooperation from the user: DetectPresence stats `.pi`, which follows the link, so pi reads as present, and detectOrSelectAgent returns detected agents directly with no TTY or under --yes. `entire enable` in CI therefore installed through the link unprompted. .opencode/plugins/entire.ts is the structurally identical case — embedded TS, worktree-resident, marker-checked — and already went through OpenHookConfig, so the four operations are routed the same way. Uninstall could not be a straight swap: HookConfigFile.Remove deletes the file where pi deletes the directory pi discovers extensions by, so removing only the file leaves a half-uninstalled extension behind. That is what HookConfigFile.RemoveDir and the osroot.RemoveAllNoSymlinks underneath it are for — os.Root.RemoveAll refuses an escaping component and unlinks a symlinked leaf, but says nothing about the parents, which is the half that carried the deletion out of the worktree. agent.GeneratedHookFileState, the path-based and unconfined variant, loses its last caller and goes; its reasoning moves to generatedStateFromContent, which HookConfigFile.GeneratedState already shared. TestHooks_RefuseSymlinkedExtensionDir pins all three operations. Against the previous code it reports the install succeeding, AreHooksInstalled claiming the planted file, and UninstallHooks deleting outside the worktree. Assisted-by: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Paulo Gomes <paulo@entire.io> Entire-Checkpoint: 01M1EV5RJ51NAQJV9ZR5XXD02C
checkEntireDirSymlinks covers .entire and nothing else, so a symlinked agent directory was invisible after the fact. `entire enable` fails loudly on one (HookConfigFile and writeManagedScaffold both create through MkdirAllNoSymlink), but once past that HookConfigFile.Exists() reports a symlinked parent as absent by design — so `entire status` shows the hooks missing without saying why, and listOrphanAgentTemps skips the directory on the stated grounds that doctor is what reports it. Nothing did. The candidate paths come from agent.HookConfigLocator, added here so each agent declares the config path it already passes to OpenHookConfig, and from the scaffold templates. Deliberately NOT from ProtectedDirs, which is the wrong set in both directions: it names what the AGENT owns, so it misses the levels Entire creates underneath (.pi/extensions, .pi/extensions/entire, .opencode/plugins — a symlink at any of them produced no output at all while this was keyed on ProtectedDirs), and it includes .vogon and external plugins' directories, which Entire never writes to. Every top-level agent directory Entire does write to survives as a component of the path underneath it. Candidates are full paths, not directories, because the leaf is refused too: HookConfigFile reads through ReadFileNoFollow and writes by pinned-parent rename, so a symlinked .claude/settings.json is as broken as a symlinked .claude. That contradicted CLAUDE.md, which still described the config file as a supported symlink; corrected here. Unlike .entire, these trees are largely the user's, so only the components Entire itself writes are examined — a .claude/skills/my-own -> shared link is a real setup and is not reported. scanForSymlinkedComponent walks shortest prefix first so the outermost link is the one named, and separates "unreadable" from "clean": a stat error that is not ENOENT gets the NOT READABLE arm rather than being answered with silence on a path Entire is about to write to. TestAllHookConfigRelPaths_CoversEveryWorktreeConfigAgent greps for agent.OpenHookConfig callers and fails when a package calls it without declaring a path, so the next nested agent cannot lose coverage quietly. Assisted-by: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Paulo Gomes <paulo@entire.io> Entire-Checkpoint: 01M1EV6XFNJE1G4JS87B31773Z
The Vercel detection statted vercel.json / .vercel / vercel.ts and read vercel.json by joining names onto the repo root, which is the shape the anchors exist to replace: these are working-tree files, so they arrive by clone and the joined path resolves wherever a checked-in symlink points. The read goes through worktreedir with osroot.ReadFile rather than the no-follow variant, deliberately. vercel.json is the user's file, not Entire's, and pointing it at a monorepo's shared config is a real setup — so an in-repo link is still followed and only one leaving the worktree is refused, which is the property this anchor is for. The read is also capped. It was an unbounded os.ReadFile of a file that arrives with the checkout, and everything downstream of the parse collapses to a single bool. vercelconfig had no tests; LoadIn gets them, including both symlink directions. Assisted-by: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Paulo Gomes <paulo@entire.io> Entire-Checkpoint: 01M1EV7R1MGG6Y530841S71A1P
57b275a made this fix in SessionStore.Name and listed the three other places pairing IsAbs with VolumeName. worktreedir.Name has the identical relative-branch gate and was missed. A Windows drive-relative path such as "C:foo" contains no separator and IsAbs reports false, so it was taken as a name inside the worktree, while filepath.Join drops the base directory when the appended element carries a volume. Not a reachable escape: Go's Windows os.Root runs every name through filepathlite.IsLocal, which rejects it one layer down. Verified by reading root_windows.go, not by running on Windows. 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. The package had no tests. The drive-relative case asserts each platform's correct answer — an error where VolumeName is meaningful, an ordinary filename on Unix — rather than skipping, so the guard is not silently untested in CI. Assisted-by: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Paulo Gomes <paulo@entire.io> Entire-Checkpoint: 01M1EV8RHPFG5VAQS2KD9WD4VD
Every other .git-resident lock goes through flock.AcquireIn; this one built its path with filepath.Join, created the directory with os.MkdirAll, and handed the result to flock.AcquireContext, whose os.OpenFile(path, O_RDWR|O_CREATE) follows a symlink at the lock path. 368f9de hardened openLockFileIn on the reasoning that git will not check a path out into .git, which is true and still true — so this is defence in depth rather than a reachable escape. It is also the reason openLockFileIn exists, and leaving the one caller outside it is how the next one gets written that way. persistentRefLock now mirrors shadowBranchLock in the same package: gitdir root, MkdirAllNoSymlink, AcquireContextIn. Assisted-by: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Paulo Gomes <paulo@entire.io> Entire-Checkpoint: 01M1EV95171634DB0HEG3VW0JX
readCapped read the whole file with ReadFileNoFollow and then sliced it to maxLen, so the cap described the result and not the work. These are working-tree files named by convention (go.mod, README.md, CONTRIBUTING.md): they arrive by clone, their size is not ours to trust, and the caller has already said how much of one it will use. LimitReader at maxLen+1 keeps the over-cap marker exact, so output is unchanged. Assisted-by: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Paulo Gomes <paulo@entire.io> Entire-Checkpoint: 01M1EV9S1ECH40C0DSJMT1JK2X
36daaea to
98bdf18
Compare
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 98bdf18. Configure here.
Nine findings from the trail's review of the re-land. The substantive one first.
**A FIFO at the leaf was reported clean.** The type check added in this branch
was gated on `prefix != name`, so it covered intermediate components only, and
the doc sentence claiming "the leaf is refused too" became false for types while
staying true for symlinks. Both positions are now checked, with the expectation
depending on where the component sits: a directory above, a regular file at the
leaf (`componentHasExpectedShape`).
That matters more than the parent case, because a FIFO there does not fail the
read, it blocks it. Verified with this branch's binary, in a repo where Entire is
not enabled:
mkdir -p .claude && mkfifo .claude/settings.json && entire doctor
# prints "Metadata branches: OK", then blocks in openat indefinitely
os.(*Root).Open root.go:103
osroot.OpenNoFollow osroot/osroot.go:80 <- parent.Open, no O_NONBLOCK
agent.(*HookConfigFile).Read agent/hook_config_file.go:112
claudecode.loadClaudeSettings claudecode/hooks.go:433
The hang is PRE-EXISTING — `OpenNoFollow` is untouched by this branch — and every
agent shares the shape, so refusing to open a non-regular leaf belongs there
rather than here and is left for a separate change. This commit only makes the
condition nameable, which is all a scan can do.
The item line now names what it found, via `paths.DescribeMode` exported for the
purpose: the two scans describe the same conditions in the same words instead of
growing a second vocabulary for them.
**The rest.** A 22-line doc block documenting the agent→directory mapping was
left heading `const claudeDirName` when that constant was inserted above it,
leaving `searchSkillTemplate` undocumented; it moves onto
`searchSkillTemplatePath`, which owns the mapping now. `worktreeFileName`'s bool
was the same fact as its name on every return path, so it is gone and both
callers test the name — the argument its own call-site comment already made.
`skipWithoutSymlinks` was a seventh copy of the skip in the change that removed
six; inlined at its three call sites. Pi's eight new comment lines explained
#2220's directory-mode change, in the wrong file, splitting a sentence in half:
the two-liner is restored and the 0750 rationale now sits on the
MkdirAllNoSymlink that sets it, stated once for all nine agents.
The guard test's second assertion is a count sitting directly under the argument
against counts. It stays a count — mapping a package directory to the rel path it
declares is not derivable, since `geminicli` declares `.gemini/settings.json` and
`copilotcli` declares `.github/hooks/entire.json` — and now says so, so the next
reader does not have to work out whether it was an oversight.
Plus a duplicated comment line, two wrong counts ("four shapes" over a six-row
table), `claudeDirName` half-applied across five sites in doctor_test.go, and
three comments narrating this branch's own revisions rather than the reasons
behind them.
Verified: lint 0 issues, test:ci exit 0, and `GOOS=windows|linux|darwin go vet
./...` all clean.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Entire-Checkpoint: 01M1XM81MKG3QHJVXTQQZ89B8W
Re-land the os.Root coverage follow-ups dropped from #2220
https://entire.io/gh/entireio/cli/trails/1202
Follow-up to #2180. A second pass over the migrated tree found five places still
doing filesystem work outside an anchor, and the diagnostic that would surface the
condition they now refuse. This brings them onto the same footing as the rest of
the tree.
Pi's extension directory
agent/pi/hooks.gowas the last registered agent whose hook-config surface hadnot moved. It joined
.pi/extensions/entire/index.tsonto the repo root andhanded the result to
os.ReadFile/os.MkdirAll/os.WriteFile/os.RemoveAll, each with a//nolint:gosecabout where the path was builtrather than where it resolves — the justification
agent.HookConfigFile's doccomment already calls out as the one that says nothing useful. A working tree
arrives by clone, so
.piis a component the repository supplies, and every oneof those four calls resolved through it.
Routed through
agent.OpenHookConfig, which the other eight agents already use:the base is the worktree root,
.pi/extensions/entire/index.tsis a name insideit, and directories are created with
MkdirAllNoSymlinkso a symlinked componis refused by name instead of followed.
.opencode/plugins/entire.tsis thestructurally identical case — embedded TS file, worktree-resident, marker-chec
— and was already on this path; pi is now the same shape rather than the exception
beside it. Worth noting for the next integration: pi is auto-detected
(
DetectPresencestats.pi), so it reaches these operations without the usernaming it, which is the argument for routing a new agent through the anchor by
default rather than trusting review to notice.
Two supporting pieces:
osroot.RemoveAllNoSymlinks.os.Root.RemoveAllrefuses a descent thatwould leave the root and unlinks a symlinked leaf rather than following it, but
it says nothing about the components above the leaf — which is where
.pisits. This pins the parent first, then removes a single named component. Same
relationship
OpenChildhas to a bareRoot.OpenRoot.HookConfigFile.RemoveDir. Pi discovers extensions by directory, soremoving only the file leaves an empty
.pi/extensions/entirefor pi to findwith nothing to load. Deliberately not general: every other agent writes into a
directory the agent itself owns, where
Removeof the file is the correctuninstall and taking the parent would delete the user's own config with it.
Refuses the case where the directory to remove would be the worktree root.
agent.GeneratedHookFileState— the path-based drift check that predates therooted
HookConfigFile.GeneratedState— had pi as its only caller and isdeleted, so there is no unrooted variant left to pick up.
Four sites beside it
vercel.json(setup.go,vercelconfig). A working-tree file read witos.Stat/os.ReadFileon a joined path. Now read through the worktree anas
LoadIn(root, name), with a 1 MiB cap: the file arrives with the checkout, soits size is not ours to choose, and everything downstream of the parse collapses
to a single bool. In-repo symlinks are still followed on purpose — pointing
vercel.jsonat a monorepo's shared config is a real setup, and this file iuser's rather than Entire's — while one leaving the worktree is refused, which is
the property the anchor exists to give.
worktreedir.Name. Given theVolumeNamepairingSessionStore.Namegot in57b275a, which
ValidateSessionIDand gitrepo's alternates checks alreadyos.Rootrejects a drive-relative name one layer down (filepathlite.IsLocal),so this changes no behaviour today; it is closed because
Nameexists to an"is this inside the worktree?" independently of its caller, and
readWorktreeFileSafelyfeeds it paths that arrive from the API. It was theremaining
IsAbsin a containment role without the pairing.checkpoint/persistent_ref_update.go). The las.git-resident lock still opening by path rather than through the common dir'sroot. Now mirrors
shadowBranchLock:gitdir.OpenAt+MkdirAllNoSymlink+AcquireContextIn. Git will not check a path out into.git, so this isconsistency and defence in depth — but
openLockFileInexists for exactly tand one caller sitting outside it is how the next one gets written that way
readCapped(runner_gather.go). Bounds the read, not just the result.These are working-tree files named by convention (
README.md,CLAUDE.md), sotheir size is not ours to assume, and the caller has already said how much of one
it will use. Reading a gigabyte to keep its first 4 KB is a cost with no ret
doctornow reports symlinked agent directoriesThe condition these refusals produce was invisible after the fact.
enablefaloudly on it, but once a repo is enabled
HookConfigFile.Exists()deliberatelyreports a symlinked parent as absent — so
entire statusshowed hooks missiwithout saying why, and
entire cleanskipped the directory on the stated groundsthat doctor reports a symlinked agent directory. Nothing did.
checkAgentDirSymlinksruns besidecheckEntireDirSymlinksand names theoutermost link, once per finding. Candidates come from the registry and the
scaffold templates rather than a list in the function. Unlike
.entire, thesetrees are largely the user's, so it examines only the components Entire itself
creates and writes through: `.claude/skills/my-own -> ../../shared/skills/my-o
is a real setup, and reporting it would train people to skip this section.
Follow-up
agentSymlinkCheckPathsderives fromAllProtectedDirs()plus the skilltemplates, not from the hook-config relPaths, so the two agents whose config nests
below their top-level directory have their Entire-created intermediate directories
unchecked — a symlink at
.pi/extensionsor.opencode/pluginsproduces nodoctor output. The refusals cover both; this is reporting only.
Note
Medium Risk
Touches hook install/uninstall and recursive delete paths where symlink mistakes could write or destroy files outside the repo; changes are defensive with new doctor reporting and tests, but they affect every agent’s enable flow.
Overview
This PR finishes os.Root / worktree anchoring for agent hook configuration and closes gaps where symlinked repo paths could still be followed or deleted outside the worktree.
Pi is migrated off raw
os.ReadFile/MkdirAll/WriteFile/RemoveAllontoagent.HookConfigFilelike the other agents. Uninstall uses newHookConfigFile.RemoveDir(backed byosroot.RemoveAllNoSymlinks) so the whole.pi/extensions/entiretree is removed without following a symlinked.pi. Path-basedGeneratedHookFileStateis removed; drift checks go throughHookConfigFile.GeneratedState.agent.HookConfigLocator(HookConfigRelPath) centralizes each agent’s config path forAllHookConfigRelPaths(), refactors allOpenHookConfigcall sites, and powers a build guard ensuring every agent that opens a hook config declares its path.entire doctorgainscheckAgentDirSymlinks, listing symlinked or unreadable components on paths Entire creates (hook config paths from the locator plus skill scaffold parents), so “hooks missing” onstatusgets an explicit cause.Additional hardening: persistent ref flock via
gitdir+flock.AcquireContextIn;vercelconfig.LoadInand setup’s Vercel probe through the worktree with a read cap;readCappedbounded reads;worktreedir.Nametreats Windows drive-relative paths like other containment checks. Docs inCLAUDE.mdare expanded for symlinked hook files/dirs and Pi auto-detection.Reviewed by Cursor Bugbot for commit 98bdf18. Configure here.