Skip to content

fix(codex): use repository hook root in linked worktrees - #2052

Open
MuskanPaliwal wants to merge 28 commits into
entireio:mainfrom
MuskanPaliwal:investigate-codex-linked-worktree-hooks
Open

fix(codex): use repository hook root in linked worktrees#2052
MuskanPaliwal wants to merge 28 commits into
entireio:mainfrom
MuskanPaliwal:investigate-codex-linked-worktree-hooks

Conversation

@MuskanPaliwal

@MuskanPaliwal MuskanPaliwal commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

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/list response 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

  • WorktreeHooksPath identifies <current-checkout>/.codex/hooks.json. It is the only mutation target used by install, uninstall, presence, freshness, deselection, and clean flows.
  • DiscoveredHooksPath identifies the project hooks file Codex is expected to load. It is used only for read-only configuration and trust diagnostics.
  • ResolveHookDiscovery consumes the shared gitrepo.ResolveGitLayout classification rather than maintaining a Codex-specific .git, commondir, or worktree-marker parser.
  • Conventional linked worktrees discover the primary checkout's hook file. Normal checkouts, ordinary submodules, and separate-Git-directory checkouts discover their own hook file.
  • .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.

HookDiscovery carries no write target, migration path, cleanup path, or lock path. Its RepositoryWide bit 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, and entire clean mutate only the current checkout's .codex/hooks.json. They do not create another checkout's .codex directory, 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 no entire-codex-hooks.lock in 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:

  • the logical target must be exactly <current-checkout>/.codex/hooks.json;
  • redirected .codex directories, final-file symlinks, non-regular files, unrelated destinations, and Git-metadata redirection are rejected;
  • existing user hooks, unknown hook events, and unrelated top-level fields are preserved;
  • uninstall removes only Entire-managed entries;
  • an existing file keeps its permission bits, while a new file uses 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 .codex directory, requires a stable regular file, caps hooks.json at 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 doctor reports the current-worktree path and Codex-discovered path, invalid or unresolved discovery, a missing project layer, managed hook drift, and trust gaps.
  • entire status keeps current-checkout installation/freshness semantics separate from effective Codex discovery and exposes the mismatch under the existing codex_hooks JSON object without changing the warning wording established in this PR.
  • SessionStart appends a short, bounded, read-only discovery or trust warning when applicable.

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.json and apply that commit to the primary checkout (typically by cherry-pick), or run entire enable from 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_LinkedWorktreeUsesPrimaryCheckout is a no-model integration test pinned to @openai/codex@0.149.0. It:

  1. creates a normal repository and conventional linked worktree with distinct hook markers;
  2. starts codex app-server in the linked worktree with an isolated CODEX_HOME;
  3. sends initialize, initialized, and hooks/list JSONL messages over stdio;
  4. requires the primary marker and primary sourcePath;
  5. rejects the linked-only marker; and
  6. requires empty warnings and errors.

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 check verification is intentionally deferred to implementation-order item 9.

@MuskanPaliwal
MuskanPaliwal marked this pull request as ready for review August 19, 2026 10:40
@MuskanPaliwal
MuskanPaliwal requested review from a team as code owners August 19, 2026 10:40
@peyton-alt

Copy link
Copy Markdown
Contributor

Hey @MuskanPaliwal, thanks again for your contribution, reviewed, this is really solid. A few things before merge:

  1. Project-layer premise — ProjectLayerExists assumes a linked checkout needs its own local .codex dir for Codex to discover the shared hooks. That claim gates AreHooksInstalled and a doctor state, and it isn't in the issue. Can you point at the codex-rs source for it, or share a hooks/list run from a linked checkout without a local .codex dir?
  2. Trust-key paths — trust matching prefixes on the symlink-resolved hooks.json path. If Codex writes its trust keys with the un-resolved path (macOS /var vs /private/var, symlinked checkouts), we'd report false trust gaps. Can you confirm which form Codex writes and pin it with a test?
  3. Lock wait is unbounded — acquireHooksLock retries until the context cancels, so a wedged holder hangs entire enable silently. Please bound the wait and return a clear error instead.
  4. Merge conflicts

Happy to merge once these are addressed. Thanks again!

@MuskanPaliwal

MuskanPaliwal commented Aug 20, 2026

Copy link
Copy Markdown
Contributor Author

Hey @peyton-alt , thanks for the review. I pushed the follow-up changes in 7020927d6.

Project-layer behavior: I tested hooks/list from a conventional linked worktree using the latest stable Codex, 0.148.0. With no local .codex directory, Codex returned hooks: []. After creating an empty local .codex directory, it found the hook from the primary checkout’s .codex/hooks.json and reported it as trustStatus: "untrusted". Everything else was kept unchanged. This confirms that the local directory enables project-layer discovery, while the hook configuration remains root-authoritative and approval remains separate. I have screenshots of the version, filesystem states, and raw responses.

Screenshots:

Screenshot 2026-08-20 at 12 21 07 PM Screenshot 2026-08-20 at 12 20 58 PM

Code changes: Trust matching now parses the complete <path>:<event>:<group>:<handler> key, validates the indexes, and compares canonicalized paths. The regression test covers a logical nested-symlink path against the physical authoritative path. Lock acquisition now times out after five seconds with a clear error, while caller cancellation still returns context.Canceled.

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 main is merged, and the seven-hook set from #1958 remains intact, including SubagentStart and SubagentStop. The merged head passes mise run check, including race-enabled unit and integration tests, all 56 Vogon canaries, and all 4 Roger-Roger canaries.

Comment thread cmd/entire/cli/agent/codex/hook_root.go Outdated
location.LockPath = filepath.Join(commonDir, "entire-codex-hooks.lock")
location.LegacyHooksPath = filepath.Join(worktreeRoot, ".codex", HooksFileName)

authoritativeRoot, err := canonicalPath(filepath.Dir(commonDir))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 error

That'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.)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Worth documenting that on ErrLinkedSubmoduleHooksUnsupported this returns a partially populated HookLocationUninstallHooks 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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread cmd/entire/cli/agent/codex/hooks.go Outdated
if force {
groups = removeEntireHooks(groups)
}
if legacy != nil && legacy.exists {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread cmd/entire/cli/config.go
func OutdatedHookAgents(ctx context.Context) []types.AgentName {
var outdated []types.AgentName
for _, name := range GetAgentsWithHooksInstalled(ctx) {
for _, name := range agent.List() {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed “installed” from the opening sentence so the comment matches the current behavior.

Comment thread cmd/entire/cli/doctor.go Outdated
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()) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 CheckHookConfigHooksAbsent, 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).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread cmd/entire/cli/setup.go
return " from the repository-wide configuration; this affects all linked worktrees"
}

func writeSharedHooksNote(ctx context.Context, w io.Writer, ag agent.Agent) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.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?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Installation and removal messages now include the resolved shared hooks.json path and state that the change applies to all linked worktrees.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread cmd/entire/cli/agent/testutil/git.go Outdated
@@ -0,0 +1,36 @@
package testutil

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@peyton-alt

peyton-alt commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

@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.

@MuskanPaliwal

Copy link
Copy Markdown
Contributor Author

@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.

@MuskanPaliwal

Copy link
Copy Markdown
Contributor Author

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread cmd/entire/cli/agent/codex/hooks.go Outdated
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 {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread cmd/entire/cli/setup.go
errs = append(errs, setupErrs...)

var uninstalledAgents []agent.Agent
for _, ag := range removedAgents {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread cmd/entire/cli/setup.go
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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@peyton-alt

Copy link
Copy Markdown
Contributor

@MuskanPaliwal Thanks again! I re-reviewed this and left a few more inline comments.

@MuskanPaliwal

Copy link
Copy Markdown
Contributor Author

hey @peyton-alt, the pr is ready for review. thanks!

@peyton-alt

Copy link
Copy Markdown
Contributor

Hey! @MuskanPaliwal I reviewed again and left one more in-line comment. Everything else looks good!

MuskanPaliwal and others added 2 commits August 22, 2026 00:23
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
@MuskanPaliwal

Copy link
Copy Markdown
Contributor Author

Hey @peyton-alt, kindly take a look. Thanks!

@MuskanPaliwal

Copy link
Copy Markdown
Contributor Author

Hi @peyton-alt let me know if anything else is pending here :))

@Soph

Soph commented Aug 24, 2026

Copy link
Copy Markdown
Collaborator

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. gitrepo.resolveDotGitPath (gitrepo/repository.go:155) + resolveCommonGitPath (:183) already parse gitdir: and commondir; paths.parseWorktreeID already handles .git/worktrees/, .bare/worktrees/ and the submodule /modules/ carve-out; session.GetGitCommonDir (session/state.go:1021) is memoized. The problem isn't line count — parseWorktreeID treats .bare/worktrees/ as a linked worktree while ownsCommonDir makes bare fall back to worktree-local. Two answers in one repo, and dupl won't flag it. Reusing gitrepo's primitives also gets memoization, which matters since ResolveHookLocation is now on the SessionStart path via HookTrustGaps.

2. Cross-checkout write needs a gate. Measured: install from a linked worktree leaves M .codex/hooks.json in the primary checkout and resets its mode 0644 → 0600 (WriteFileAtomic chmods unconditionally, hooks.go:460; the os.WriteFile it replaced didn't). Worse in the removal direction — entire clean in a scratch worktree strips capture repo-wide, and writeSharedHooksRemovalNote prints after the confirm prompt in runUninstall. Suggest: scope text into the prompt, write behind a flag.

3. Containment stops at the checkout. hook_path.go:67/:95 accept any target under checkoutRoot, and .git/ qualifies. Measured with committed symlinks: .codex -> .git wrote .git/hooks.json; .codex/hooks.json -> ../package.json rewrote package.json at 0600. Both commit and clone cleanly. Still better than main (bare os.WriteFile followed symlinks anywhere) — just short. Fix is already in the PR: run the resolved destination through isInsideGitMetadata (hook_root.go:386) and require the project dir to be exactly <checkoutRoot>/.codex.

4. Nothing in CI catches this regressing. No e2e/ or integration_test/ file touched, and no e2e test runs in a linked worktree — every "worktree" mention in e2e/tests/ is a prompt saying "Do not use worktrees." The unit tests assert the resolver against our beliefs about Codex; the only anchor to Codex's real behavior is the comment at hook_root_test.go:115. Can we get one integration test driving codex app-server's hooks/list in a linked worktree?

Minor: ErrLinkedSubmoduleHooksUnsupported in the description doesn't exist (linked submodules fall back to worktree-local, no error), and the bare-layout description contradicts TestResolveHookLocation_BareRepositoryWorktreeUsesWorktreeLocalHooks#2020 saw the opposite on 0.147-era vs 0.149 in the comment, worth pinning down. Also hooks.go:341 / trust.go:96 read the repo-controlled hooks file unbounded while hook_root.go caps git metadata at 64KiB.

@MuskanPaliwal

Copy link
Copy Markdown
Contributor Author

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. gitrepo.resolveDotGitPath (gitrepo/repository.go:155) + resolveCommonGitPath (:183) already parse gitdir: and commondir; paths.parseWorktreeID already handles .git/worktrees/, .bare/worktrees/ and the submodule /modules/ carve-out; session.GetGitCommonDir (session/state.go:1021) is memoized. The problem isn't line count — parseWorktreeID treats .bare/worktrees/ as a linked worktree while ownsCommonDir makes bare fall back to worktree-local. Two answers in one repo, and dupl won't flag it. Reusing gitrepo's primitives also gets memoization, which matters since ResolveHookLocation is now on the SessionStart path via HookTrustGaps.

2. Cross-checkout write needs a gate. Measured: install from a linked worktree leaves M .codex/hooks.json in the primary checkout and resets its mode 0644 → 0600 (WriteFileAtomic chmods unconditionally, hooks.go:460; the os.WriteFile it replaced didn't). Worse in the removal direction — entire clean in a scratch worktree strips capture repo-wide, and writeSharedHooksRemovalNote prints after the confirm prompt in runUninstall. Suggest: scope text into the prompt, write behind a flag.

3. Containment stops at the checkout. hook_path.go:67/:95 accept any target under checkoutRoot, and .git/ qualifies. Measured with committed symlinks: .codex -> .git wrote .git/hooks.json; .codex/hooks.json -> ../package.json rewrote package.json at 0600. Both commit and clone cleanly. Still better than main (bare os.WriteFile followed symlinks anywhere) — just short. Fix is already in the PR: run the resolved destination through isInsideGitMetadata (hook_root.go:386) and require the project dir to be exactly <checkoutRoot>/.codex.

4. Nothing in CI catches this regressing. No e2e/ or integration_test/ file touched, and no e2e test runs in a linked worktree — every "worktree" mention in e2e/tests/ is a prompt saying "Do not use worktrees." The unit tests assert the resolver against our beliefs about Codex; the only anchor to Codex's real behavior is the comment at hook_root_test.go:115. Can we get one integration test driving codex app-server's hooks/list in a linked worktree?

Minor: ErrLinkedSubmoduleHooksUnsupported in the description doesn't exist (linked submodules fall back to worktree-local, no error), and the bare-layout description contradicts TestResolveHookLocation_BareRepositoryWorktreeUsesWorktreeLocalHooks#2020 saw the opposite on 0.147-era vs 0.149 in the comment, worth pinning down. Also hooks.go:341 / trust.go:96 read the repo-controlled hooks file unbounded while hook_root.go caps git metadata at 64KiB.

Hey @Soph, yes that makes sense. let me split the pr and get back to you :))

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

Codex sessions are not captured in linked worktrees because hooks are installed in an ignored worktree-local location

3 participants