fix(codex): use repository hook root in linked worktrees - #2052
fix(codex): use repository hook root in linked worktrees#2052MuskanPaliwal wants to merge 28 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 :)) |
|
Hey @MuskanPaliwal thanks for doing this. I had a look at this today, and I would like to ask if we can reduce the scope of this PR and maybe split it. What I like to keep is resolver + inspection + warnings but pull all the write parts out for now. I think the main issue here is surfacing that something is wrong but trying to fix it for the user might be a bit too much. Like I if the user adds this to a worktree but then commits he can just bring that commit to his root worktree and things work. Writing across worktrees - with the potential of him missing the change in the root tree completely - is to much work from my point of view for something we just need to surface more properly. On top of the split I have these findings / asks: 1. Fourth implementation of worktree traversal. 2. Cross-checkout write needs a gate. Measured: install from a linked worktree leaves 3. Containment stops at the checkout. 4. Nothing in CI catches this regressing. No Minor: |
Hey @Soph, yes that makes sense. let me split the pr and get back to you :)) |
3 participants
Fixes #2020.
Codex 0.149.0 can load project hooks from a different checkout than the one in which it is running. In a conventional linked worktree, its app-server
hooks/listresponse reports the primary checkout's.codex/hooks.json; a divergent file present only in the linked worktree is not loaded.This PR makes that effective location visible without giving Entire ownership of another checkout. Discovery and mutation are deliberately separate: Entire may inspect the file Codex actually loads, but installs, removes, and cleans hooks only in the exact checkout where the command was invoked.
Discovery and ownership
WorktreeHooksPathidentifies<current-checkout>/.codex/hooks.json. It is the only mutation target used by install, uninstall, presence, freshness, deselection, and clean flows.DiscoveredHooksPathidentifies the project hooks file Codex is expected to load. It is used only for read-only configuration and trust diagnostics.ResolveHookDiscoveryconsumes the sharedgitrepo.ResolveGitLayoutclassification rather than maintaining a Codex-specific.git,commondir, or worktree-marker parser..bare/worktrees, linked submodules, contradictory metadata, and other behavior not pinned against Codex are reported as unresolved. Entire does not claim an unsupported layout or guess a writable fallback.HookDiscoverycarries no write target, migration path, cleanup path, or lock path. ItsRepositoryWidebit describes the reach of the configuration Codex discovers; it does not authorize repository-wide mutation.Checkout-local mutation
entire enable,entire agent remove codex, agent deselection, andentire cleanmutate only the current checkout's.codex/hooks.json. They do not create another checkout's.codexdirectory, migrate a worktree-local file into the primary checkout, remove a discovered file owned elsewhere, or perform repository-wide cleanup.Concurrent updates are serialized only with
<current-checkout>/.codex/hooks.json.lock. There is noentire-codex-hooks.lockin the Git common directory and no synchronization across worktrees. An old common-directory lock, if present from a previous build, is neither used nor cleaned from another checkout.Current-checkout writes are hardened before the final atomic replacement:
<current-checkout>/.codex/hooks.json;.codexdirectories, final-file symlinks, non-regular files, unrelated destinations, and Git-metadata redirection are rejected;0600.Bounded read-only inspection
The local and discovered files are inspected independently as absent, user-only, Entire-managed, invalid, or unresolved. Inspection is contained to the exact canonical
.codexdirectory, requires a stable regular file, capshooks.jsonat 1 MiB, reads through a limit with one overflow byte, and checks file identity before and after the read. It never creates a directory, rewrites configuration, removes entries, or acquires a mutation lock.The structured inspection also reports whether the core Entire events are present, which current events are missing, whether managed commands and timeouts match this CLI, whether the current checkout has the project layer Codex requires, whether local trust records were readable, and which declared hooks lack an approval record. Entire does not compute, copy, or synthesize Codex trust hashes.
Diagnostics and remedy
entire doctorreports the current-worktree path and Codex-discovered path, invalid or unresolved discovery, a missing project layer, managed hook drift, and trust gaps.entire statuskeeps current-checkout installation/freshness semantics separate from effective Codex discovery and exposes the mismatch under the existingcodex_hooksJSON object without changing the warning wording established in this PR.When hooks exist only in a linked worktree, Entire does not repair the primary checkout automatically. The user-owned remedy is to commit
.codex/hooks.jsonand apply that commit to the primary checkout (typically by cherry-pick), or runentire enablefrom the primary checkout. Because linked worktrees share Git objects, the commit is immediately available to cherry-pick; no push or fetch is required.Pinned Codex integration contract
TestCodexAppServerHooksList_LinkedWorktreeUsesPrimaryCheckoutis a no-model integration test pinned to@openai/codex@0.149.0. It:codex app-serverin the linked worktree with an isolatedCODEX_HOME;initialize,initialized, andhooks/listJSONL messages over stdio;sourcePath;warningsanderrors.The test makes no model request and needs no OpenAI credentials. Local integration runs skip when the exact Codex binary is unavailable. CI installs version 0.149.0 and sets
ENTIRE_TEST_REQUIRE_CODEX_APP_SERVER=1, so a missing binary or version drift fails rather than silently skipping.Verification status
The implementation includes focused unit coverage for Git-layout classification, path-role separation, current-checkout-only install/remove/clean behavior, exact-worktree locking, destination containment, symlink refusal, bounded reads, mode preservation, doctor/status/SessionStart diagnostics, trust inspection, and JSON output, plus the pinned app-server integration contract above.
Full formatting, lint, unit, integration, canary, duplication, and
mise run checkverification is intentionally deferred to implementation-order item 9.