fix(codex): use repository hook root in linked worktrees - #2052
fix(codex): use repository hook root in linked worktrees#2052MuskanPaliwal wants to merge 26 commits into
Conversation
|
Hey @MuskanPaliwal, thanks again for your contribution, reviewed, this is really solid. A few things before merge:
Happy to merge once these are addressed. Thanks again! |
|
Hey @peyton-alt , thanks for the review. I pushed the follow-up changes in 7020927d6. Project-layer behavior: I tested Screenshots: 
Code changes: Trust matching now parses the complete I also fixed three related cases found while rechecking this path: migration no longer risks deleting an aliased shared hook file, unknown and user-defined JSON fields are preserved, and uninstall can find removable shared hooks when the linked checkout lacks its local project layer. Current |
| location.LockPath = filepath.Join(commonDir, "entire-codex-hooks.lock") | ||
| location.LegacyHooksPath = filepath.Join(worktreeRoot, ".codex", HooksFileName) | ||
|
|
||
| authoritativeRoot, err := canonicalPath(filepath.Dir(commonDir)) |
There was a problem hiding this comment.
This doesn't check that the parent of the common dir is actually a checkout, and isInsideGitMetadata below won't catch it (a bare repo named repo.git matches neither .git/.bare, and the walk starts above it).
git clone --bare <src> ~/repo.git
git -C ~/repo.git worktree add ~/work/wt
# from ~/work/wt: commondir=../.. -> commonDir=~/repo.git -> Dir() -> $HOME
# HooksPath = ~/.codex/hooks.json, nil errorThat's CODEX_HOME, so agent add codex merges one repo's hooks into every Codex session machine-wide, and agent remove codex can delete the file. Same cause puts hooks in ~/src/.codex for a bare repo in ~/src, or beside the git dir with --separate-git-dir.
Could we refuse when the derived root is CODEX_HOME/$HOME, with a sentinel like the submodule one? git worktree list --porcelain also marks the bare main entry if you want a positive signal. (A "must contain .git" check won't work — the .bare layout root has none.)
There was a problem hiding this comment.
Fixed. We now derive the shared Codex hooks path only when Git’s common directory is named .git or .bare, since its parent is the project root in those layouts. That assumption is false for standard bare repositories and separate Git directories, so Entire skips Codex hook installation there instead of risking a write outside the repository. We also reject paths that would collide with $HOME or CODEX_HOME, with regression tests for each case.
There was a problem hiding this comment.
The basename guard closes the repo.git case, but a separate Git directory can legitimately be named .git:
git init --separate-git-dir /tmp/storage/.git /tmp/primary
git -C /tmp/primary worktree add /tmp/linked
Enabling from /tmp/linked writes /tmp/storage/.codex/hooks.json, outside both checkouts. Could we positively validate the worktree registration and gitdir backlink instead of treating the basename as proof of ownership?
There was a problem hiding this comment.
The ownership problem is fixed. Your exact layout still resolves to <storage>, because Codex treats it as the trusted project root and reads hooks there. I chose to match that behavior and show the destination before writing. If Entire’s policy is that hooks may only be written inside a visible checkout, this layout should instead return an explicit unsupported-location error. A local fallback would not work.
| } | ||
|
|
||
| // ResolveHookLocation resolves Codex's repository-authoritative hooks file. | ||
| func ResolveHookLocation(ctx context.Context) (HookLocation, error) { |
There was a problem hiding this comment.
Worth documenting that on ErrLinkedSubmoduleHooksUnsupported this returns a partially populated HookLocation — UninstallHooks depends on LockPath surviving there, while every other error path returns the zero value. Someone "normalizing" that return to HookLocation{} compiles, passes most tests, and turns the submodule uninstall into flock.New(""). A data-carrying sentinel (the shape V1DivergedError uses in strategy/) would make it compiler-visible instead.
There was a problem hiding this comment.
Changed this to an UnsupportedHookLocationError. ResolveHookLocation now returns a zero HookLocation on failure, while the typed error carries the safe location information needed to clean up a legacy file.
| // .codex/hooks.json. | ||
| func (c *CodexAgent) InstallHooks(ctx context.Context, force bool) (int, error) { | ||
| repoRoot, err := paths.WorktreeRoot(ctx) | ||
| location, err := ResolveHookLocation(ctx) |
There was a problem hiding this comment.
This propagates the submodule sentinel through applyAgentChanges into errors.Join, so entire enable exits non-zero from a linked submodule where it succeeded on main — other agents install fine but the command still fails. Since it's a permanent limitation rather than a transient error (and UninstallHooks/CheckHookConfig both special-case it with errors.Is), can we warn and skip Codex here instead?
There was a problem hiding this comment.
Setup now treats this as a skipped Codex installation instead of failing entire enable. It prints the reason, continues configuring the other agents, and does not claim that Codex was added. I added a linked-submodule setup test for this behavior.
There was a problem hiding this comment.
One correction to my earlier reply: after matching Codex 0.149.0’s resolver, linked submodules no longer need to be skipped. Their shared-root ownership check fails, so both Codex and Entire use the submodule worktree’s local .codex/hooks.json.
Skipping remains for unsafe final destinations such as $HOME and CODEX_HOME. If a skip leaves no selected agent with hook coverage, enable fails instead of reporting success.
| if force { | ||
| groups = removeEntireHooks(groups) | ||
| } | ||
| if legacy != nil && legacy.exists { |
There was a problem hiding this comment.
The second-worktree leg of the migration looks untested: A migrates, B still has a legacy file, so install from B writes nothing to the authority (count 0) but must still clean B's legacy file here, CheckHookConfig must return Outdated on current-authority-plus-legacy, and doctor must print LEGACY COPY FOUND. Every existing migration test does the authority write and the legacy clean in one call, and LEGACY COPY FOUND is the only doctor state with no coverage. One test — install in primary, seed a legacy Entire file in the linked worktree, install from the linked worktree — pins all three.
There was a problem hiding this comment.
Added a test that installs the shared hooks from the primary checkout, creates a legacy copy in a linked worktree, and checks both status and doctor output. Running installation from the linked worktree removes the legacy copy without duplicating the shared hooks.
There was a problem hiding this comment.
The valid second-worktree migration is covered now, but malformed ignored legacy state still blocks the authoritative installation.
An empty linked/.codex/hooks.json makes enable exit before creating the primary hooks file. Could the authoritative destination be installed first, with any legacy cleanup failure reported separately?
There was a problem hiding this comment.
Changed the installation order. Entire now writes the authoritative hooks file before cleaning the legacy copy.
If the legacy file is malformed, the working authoritative installation remains in place. Entire leaves the malformed file untouched and reports that installation succeeded but cleanup failed. The test uses this exact case.
| } | ||
| inspection := inspectHookConfigAt(ctx, location.HooksPath) | ||
| switch inspection.State { | ||
| case HookFileInvalid: |
There was a problem hiding this comment.
Invalid → HooksOutdated, combined with OutdatedHookAgents now iterating agent.List(), flags Codex in repos that never enabled it: a hand-written or newer-schema hooks.json we can't parse puts codex in hooks_outdated, so checkHookDrift prints "Run entire enable --force" while checkCodexHookTrust prints "Fix or restore the file before running entire enable". Contradictory, and the first can't work — installManagedHooks hits the same parse error. Could we leave invalid files to checkCodexHookTrust, or gate this on whether Entire wrote the file?
There was a problem hiding this comment.
Fixed. Invalid hook files now report HooksAbsent, so they no longer appear under hooks_outdated or suggest running enable --force. Doctor still reports the file as INVALID.
| @@ -0,0 +1,40 @@ | |||
| package codex | |||
There was a problem hiding this comment.
This covers the helper, and TestInstallHooks_RepositoryLockDoesNotPolluteWorktree proves the lock lands in the common dir — but nothing pins that InstallHooks holds the lock across its read-modify-write. Moving readHooksDocument above acquireHooksLock would pass the whole suite while reintroducing the lost update this PR exists to prevent (A's install erasing what B just wrote). A test racing a few goroutines through install/uninstall on one authoritative file, asserting valid JSON and a surviving user hook, would pin it — flock conflicts across FDs in-process, so it works as a unit test.
There was a problem hiding this comment.
Added a concurrency test that races installs and uninstalls against user-hook updates using the same lock. The test checks that the resulting file is valid JSON and that none of the user hooks are lost.
| func OutdatedHookAgents(ctx context.Context) []types.AgentName { | ||
| var outdated []types.AgentName | ||
| for _, name := range GetAgentsWithHooksInstalled(ctx) { | ||
| for _, name := range agent.List() { |
There was a problem hiding this comment.
The doc above still opens with "returns installed agents", which this line is precisely no longer true of — and TestCheckCodexHookTrust_LinkedWorktreeReportsLegacyFile pins the opposite (codex absent from GetAgentsWithHooksInstalled, present here). Worth dropping "installed" from that first sentence. The widening itself is safe today: I checked claudecode/opencode/pi and all three return HooksAbsent rather than HooksOutdated when nothing is installed, so this only moves Codex.
There was a problem hiding this comment.
Removed “installed” from the opening sentence so the comment matches the current behavior.
| repoRoot, err := paths.WorktreeRoot(cmd.Context()) | ||
| location, err := codex.ResolveHookLocation(cmd.Context()) | ||
| if err != nil { | ||
| if errors.Is(err, codex.ErrLinkedSubmoduleHooksUnsupported) && codex.HasWorktreeLocalEntireHooks(cmd.Context()) { |
There was a problem hiding this comment.
Only the sentinel gets a diagnostic — every other resolver failure (.git with no gitdir:, unreadable/empty commondir, the commondir-mismatch and looksLikeGitDir rejections, EACCES in canonicalPath) hits the bare return below with no output. Meanwhile AreHooksInstalled → false and CheckHookConfig → HooksAbsent, so Codex vanishes from entire status, the review/investigate pickers and doctor, and agent remove codex refuses with "not installed" — while Codex may still be firing hooks, since it resolves its own root. Worth an UNRESOLVED — <err> branch when there's evidence Codex is in play (a .codex dir at the worktree root, or HasWorktreeLocalEntireHooks).
There was a problem hiding this comment.
Doctor now reports Codex hooks: UNRESOLVED with the resolver error when a local .codex project layer shows that Codex is in use. Unsupported derived roots still get their own diagnostic.
| return " from the repository-wide configuration; this affects all linked worktrees" | ||
| } | ||
|
|
||
| func writeSharedHooksNote(ctx context.Context, w io.Writer, ag agent.Agent) { |
There was a problem hiding this comment.
.codex/hooks.json is tracked and not gitignored in this repo, so installing from a linked worktree leaves an unexplained modified: in the primary checkout — a file the user never touched, in a worktree they're not looking at (and removal can leave it deleted there). This note is the only thing that could connect that diff to Entire, and it doesn't say where the file is. Can we print the resolved HooksPath here and in sharedHooksRemovalSuffix?
There was a problem hiding this comment.
Installation and removal messages now include the resolved shared hooks.json path and state that the change applies to all linked worktrees.
There was a problem hiding this comment.
The helper output is fixed, but the primary interactive enable path does not call it, and deselection/disable reports the shared scope only after removal.
Could we show the resolved path and cross-worktree effect before confirmation or mutation?
There was a problem hiding this comment.
The interactive picker now shows the resolved shared hooks.json path. Enable, deselection, direct removal, and disable also print the path and cross-worktree effect before changing anything.
The tests check that the notice appears before the installation or removal result.
| @@ -0,0 +1,36 @@ | |||
| package testutil | |||
There was a problem hiding this comment.
All five of these delegate verbatim to cmd/entire/cli/testutil, and there's no import cycle forcing the indirection — go list -deps ./cmd/entire/cli/testutil doesn't reach agent/codex. Can the codex tests import cli/testutil directly and drop this package?
There was a problem hiding this comment.
Removed the pass-through Git helper file. Codex tests now import the canonical CLI test helpers directly, with a narrow architecture-test allowance for that package.
|
@MuskanPaliwal thanks for the edits! I left a batch of inline comments. Only the hook_root.go (the derived hook root can land on CODEX_HOME or outside the repo entirely)one blocks, the rest are small. |
on it. |
|
hey @peyton-alt , I have addressed all the review comments. Thanks for the review. Kindly re-review it. Thanks! |
| return false | ||
| } | ||
| canonicalProjectDir, err := canonicalPath(filepath.Join(hookRoot, ".codex")) | ||
| return err == nil && canonicalProjectDir == canonicalCodexHome |
There was a problem hiding this comment.
This prevents an exact CODEX_HOME collision, but it does not require the resolved .codex directory to remain inside the authoritative checkout.
I reproduced enable from a linked worktree writing through the primary checkout .codex symlink into an unrelated directory.
Could we validate destination containment while preserving the intentional same-file alias where a linked worktree .codex resolves to the authoritative .codex?
There was a problem hiding this comment.
Codex hook paths now go through one destination resolver. It resolves the checkout, .codex directory, and final hooks.json target, then rejects any target outside the checkout.
The intentional same-directory alias still works. When the linked and authoritative .codex paths resolve to the same directory, legacy cleanup is skipped.
| return fmt.Errorf("failed to marshal hooks.json: %w", err) | ||
| } | ||
| if err := os.WriteFile(hooksPath, output, 0o600); err != nil { | ||
| if err := jsonutil.WriteFileAtomic(path, output, 0o600); err != nil { |
There was a problem hiding this comment.
Rename-based atomic replacement breaks a symlinked hooks.json.
With .codex/hooks.json -> ../managed/hooks.json, entire enable --force replaces the symlink with a regular file and detaches the managed target.
Could we preserve atomicity and locking while following only repository-contained, validated symlink targets? The global-writer stack WriteFileAtomicFollowingSymlinks helper may help after containment validation is added.
There was a problem hiding this comment.
Contained hooks.json symlinks are now preserved. After containment validation, atomic writes target the resolved file instead of replacing the symlink entry.
Uninstall follows the same rule. If removing Entire’s hooks leaves no other properties, it writes {} to the target rather than deleting the symlink. Outside-checkout targets remain rejected.
| errs = append(errs, setupErrs...) | ||
|
|
||
| var uninstalledAgents []agent.Agent | ||
| for _, ag := range removedAgents { |
There was a problem hiding this comment.
This removal loop can only see agents in installedNames, which runManageAgents builds with GetAgentsWithHooksInstalled.
Codex returns false for valid partial/outdated state and when the current linked checkout lacks its local project layer, even though shared hooks may still be active in sibling worktrees. Codex then disappears from the picker and cannot be deselected here.
Could we include valid removable/outdated Codex state in preselection and removal while continuing to exclude invalid arbitrary hook files?
There was a problem hiding this comment.
Agent management now includes Codex when its hooks are installed or when its valid Entire-owned configuration is outdated. That covers partial hooks and shared hooks missing the current worktree’s project layer.
Invalid JSON and user-only files still count as absent, so the picker will not offer to remove configuration Entire does not own.
There was a problem hiding this comment.
Thanks — the agent manager path is fixed. One related fresh-setup path still calls uninstallDeselectedAgentHooks, which seeds from GetAgentsWithHooksInstalled. In a new linked checkout with shared Codex hooks but no local .codex layer, Codex is Outdated rather than Installed, so deselecting it during interactive enable still leaves the repository-wide hooks active. Could this function use removableAgentHookNames too, with a regression test for the fresh setup path?
There was a problem hiding this comment.
Good catch — fixed in 76f67ec. uninstallDeselectedAgentHooks now seeds from removableAgentHookNames (same as the agent-manager path) so Outdated-but-not-Installed repository-wide hooks are still torn down when deselected. Added TestUninstallDeselectedAgentHooks_CodexOutdatedLinkedWorktreeWithoutProjectLayer, which reproduces the linked-worktree-without-a-local-.codex-layer scenario and fails without the fix.
| count, err := hookAgent.InstallHooks(ctx, forceHooks) | ||
| if err != nil { | ||
| return 0, fmt.Errorf("failed to install %s hooks: %w", ag.Name(), err) | ||
| var skipped agent.HookInstallationSkipError |
There was a problem hiding this comment.
Skipping is correct when another selected agent remains functional, but this also succeeds when Codex is the only selected agent.
I reproduced enable --agent codex printing Ready, exiting 0, and leaving status --json with enabled: true and agents: [].
Could we fail when skipping Codex leaves zero hook coverage? This preserves the earlier multi-agent skip behavior.
There was a problem hiding this comment.
Enable now fails if every selected agent is skipped. It does so before writing .entire state or marking the repository enabled.
Multi-agent setup still continues when another selected agent installs working hooks. Tests cover both explicit enable --agent codex and interactive enable.
|
@MuskanPaliwal Thanks again! I re-reviewed this and left a few more inline comments. |
|
hey @peyton-alt, the pr is ready for review. thanks! |
|
Hey! @MuskanPaliwal I reviewed again and left one more in-line comment. Everything else looks good! |
uninstallDeselectedAgentHooks seeded from GetAgentsWithHooksInstalled, which only reports Installed hooks. A linked worktree with Codex's repository-wide hooks but no local .codex layer reports them as Outdated instead, so deselecting Codex on that fresh-setup path left the root-authoritative hooks.json in place for every worktree. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MG9uEFWJ1612CjBPPvAXSF
|
Hey @peyton-alt, kindly take a look. Thanks! |
|
Hi @peyton-alt let me know if anything else is pending here :)) |
2 participants
Fixes #2020.
Codex treats the root repository as the authoritative source for hooks and hook trust across linked worktrees. Entire instead wrote
.codex/hooks.jsonrelative to the active checkout, producing a valid-looking configuration that Codex ignored. This change makes Entire install, migrate, remove, and diagnose the same hook file Codex actually discovers while preserving normal-checkout behavior.Approach
The central ownership seam is
codex.ResolveHookLocation. It returns aHookLocationcontaining the authoritative hooks path, an optional legacy worktree-local path, the shared lock path, and whether changes affect other linked worktrees. Installation, removal, presence detection, freshness checks, missing-hook reporting, trust diagnostics, status, and doctor now use this resolver instead of deriving the path independently.Normal checkouts continue using their local
.codex/hooks.json. Conventional linked worktrees resolve to the primary checkout, while bare layouts resolve to the directory containing the bare Git common directory. Relativegitdirandcommondirvalues are supported, and the expected Git layout is validated before the resolver returns a writable path.Submodules are handled explicitly. Ordinary submodules remain project-local because their Git pointer does not have the linked-worktree shape. For a linked submodule, Codex's resolution would lead inside
.git/modules; Entire detects that case and returnsErrLinkedSubmoduleHooksUnsupportedinstead of writing configuration into Git internals.HookLocation.ProjectLayerExistscaptures another part of Codex's discovery behavior: each linked checkout still needs a local.codexdirectory for Codex to construct its project layer, even thoughhooks.jsonis loaded from the authoritative root. Installation creates that project layer, while presence and doctor reporting identify when it is missing.Updates to the shared file are serialized using
entire-codex-hooks.lockin the Git common directory. This coordinates changes from different worktrees without creating a tracked.codex/hooks.json.lockfile. The existingmanagedHookstable remains the canonical definition of Entire's Codex events, commands, timeouts, trust labels, and core-presence behavior.Migration and Reporting
Installation updates the authoritative file before cleaning the ignored legacy file. It preserves user hooks and unrelated top-level fields at the destination, then removes only Entire-managed entries from the worktree-local document. A legacy file containing user configuration remains in place, while an Entire-only file is removed. Repeating the operation produces no further changes.
InspectHookConfigprovides one parsed view of the authoritative file for presence, freshness, missing-hook reporting, and doctor. It distinguishes absent, user-only, Entire-managed, and invalid configurations. This prevents a user-owned hooks file from being reported as stale Entire configuration and prevents malformed JSON from being reported as installed merely because the file exists.AreHooksInstalledchecks the authoritative path and only requires the core event set, so adding a new event does not make older installations disappear from agent presence reporting.CheckHookConfigseparately handles partial installations, missing project layers, legacy copies, malformed files, and drift from the current managed set.OutdatedHookAgentsnow consults agents implementingagent.HookFreshnessdirectly. This allows status and doctor to report an ignored legacy installation as outdated even thoughAreHooksInstalledcorrectly excludes it from the installed-agent list. Removal uses the same distinction, allowingentire agent remove codexto clean a misplaced or partial installation instead of returning "not installed."The new
agent.RepositorySharedHookscapability lets setup and removal flows explain when a Codex mutation affects the repository's other linked worktrees. Entire does not copy or synthesize Codex trust hashes; doctor reports hook discovery and installation separately from the availability of local approval records.Related Work
PR #1958 adds Codex
SubagentStartandSubagentStopsupport but remains separate from this change. Those events can join the canonicalmanagedHookstable without changing the resolver, locking, migration, or diagnostic design introduced here. Because the PRs modify some of the same Codex integration files, conflict resolution may be required if #1958 lands first.This is also distinct from #1197, which concerned starting Codex from a nested directory in a monorepo, and from commit
4b9b9636e, which managed a Codexconfig.tomlfeature flag for repositories below Codex's reservedagentsdirectory. Neither addressed linked-worktree hook-root resolution.Testing
gitdirandcommondirforms..codex/hooks.jsoninto Git metadata.mise run checkpassed with zero lint issues, race-enabled unit and integration tests, 56 Vogon canaries, and 4 Roger-Roger canaries.hooks/listsmoke test discovered all five hooks from the authoritative root with no warnings or errors and reported them as untrusted.