fix(codex): use repository hook root in linked worktrees - #2052
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 |
|
@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! |
|
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 :)) |
763d1d0 to
978bc6b
Compare
|
Hey @Soph, thanks for the detailed review. I reduced the scope as requested and pushed the follow-up changes in The resolver remains read-only. Install, uninstall, and clean only modify the checkout where the command runs; when that differs from the file Codex reads, setup, removal, doctor, status, and SessionStart warn instead of writing across checkouts. I also clarified the warnings so they name the exact files involved: the current worktree's I added a doctor regression covering that Validation: focused Codex/doctor tests pass,
|
5690551 to
b810240
Compare
…e-hooks' into investigate-codex-linked-worktree-hooks
|
On top of this, I’m working on a broader consolidation of the Git traversal logic in a follow-up PR. |
|
Ran this against 1. There is no healthy state for a linked worktree. Repro:
The working configuration and the broken one differ only in which warning you get, and the remedy points at a merge that already happened.
Suggest splitting the state at Keep the warning for missing / not-Entire-managed; stale is already 2. Trim the remedy to the one thing that applies.
Both remedy blocks currently print two hedged lines unconditionally: Every linked-worktree user sees the Anyone running a Same trim in Verification: post-merge repro prints |
Soph
left a comment
There was a problem hiding this comment.
and thanks for continuing! :)
|
Hey @Soph , kindly re-review. Thanks :) |



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.