Skip to content

fix(codex): stop a Codex pane launch from deleting another profile's real-home hooks (STA-5679) - #20345

Closed
brennanb2025 wants to merge 1 commit into
mainfrom
brennanb2025/codex-realhome-install-only
Closed

brennanb2025 wants to merge 1 commit into
mainfrom
brennanb2025/codex-realhome-install-only

Conversation

@brennanb2025

@brennanb2025 brennanb2025 commented Sep 12, 2026 •

Copy link
Copy Markdown
Contributor
Files Added Deleted Net
Test 2 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​184 0 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​184
Prod 2 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​24 $\color{#cf222e}{\Huge{\mathbf{−}}}$​15 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​9

ELI5

Two Orca windows can be signed into different profiles, but they share one ~/.codex folder. If you turned agent status hooks off in one of them, simply launching or resuming a Codex pane there deleted the Codex hook — and its trust records — that the other profile had installed. This makes both of those code paths install-only: with hooks off they now do nothing instead of deleting.

What Changed

Two per-pane Codex launch paths stopped forwarding this profile's agentStatusHooksEnabled preference into the real-home hook writer, and now gate on it instead:

  • src/main/startup/codex-launch-preparation.ts — ensureRealHomeHooksIfSelected gains the preference in its existing early-return guard and passes a hard hooksEnabled: true.
  • src/main/startup/codex-session-resume-launch.ts — the preference is resolved once and returns early before the isSystemHome test, so neither the real-home lane nor the managed-home lane runs.

Adds the first test files for both modules. No production behavior changes when hooks are on.

Why

This is a pre-existing bug, split out of #19974. That PR does not cause it; it makes it easy to reach, because its first-run checkbox fires only on fresh profiles — exactly the second-profile scenario where the damage shows up.

The mechanism. ~/.codex/hooks.json and ~/.codex/config.toml are user-global, one copy shared by every Orca profile on the machine. sweepRealHomeCodexHook identifies Orca-managed entries by script filename (createManagedCommandMatcher(getCodexManagedScriptFileName())), not by which profile installed them. So a sweep driven by this profile's off switch removes the hook entry, and removeCodexManagedHookTrustEntries removes the [hooks.state] trust record, that another running profile depends on. That is STA-5679.

ensureRealHomeCodexHookState is hooksEnabled ? install : sweep (src/main/codex/codex-real-home-hook-install.ts:101), so forwarding a false preference is requesting the destructive branch. On the resume path the non-system-home else branch was just as bad in a less obvious way: refreshRuntimeUserHooksForLaunchPrep ends in cleanupLegacyManagedHookRepresentations() → cleanupLegacySystemManagedHooks(), which mutates the real ~/.codex/config.toml — and the sweep-suppression gate in main-process-account-services.ts deliberately re-arms exactly when hooks are off.

Why mirroring startup is the right fix. Startup was already fixed this way in 4ff07da: it gates on the preference and then passes a hard hooksEnabled: true, with the reasoning recorded on resolveStartupManagedHookAction — "Honoring the off switch only requires skipping the install; removal stays on the explicit Settings toggle, which is the user-initiated path that should own it." These two paths are the per-pane twins of that same decision, and were simply missed.

Nothing legitimate is lost.

  • Routing is unchanged. isRealHomeCodexHookLaneUsable() is currentLane !== 'unavailable', so the lane reads as usable whether it sits at 'pending' (never attempted) or 'removed' (swept). PTY env, rate limits, and commit-message routing all see the same answer.
  • Opt-out still converges. The Settings toggle calls applyAgentStatusHooksEnabled(false) → removeManagedAgentHooks() → codexHookService.remove() → removeCodexHooksExclusively → cleanupLegacyManagedHookRepresentations() → cleanupLegacySystemManagedHooks(), which sweeps the real home. That is user-initiated, which is where STA-5679 says removal belongs.

The stale exemption. The commit that exempted Codex from the original STA-5679 fix justified it in its own message: "Codex is unaffected either way because its hooks live in an Orca-owned runtime home" — 4ff07da, 2026-08-27. That had stopped being true six weeks earlier: c3f37e5 (2026-07-14) moved the system-default lane onto the user's real ~/.codex. The exemption was written against a world that no longer existed.

Adjacent site deliberately left alone

src/main/startup/codex-launch-preparation.ts:73-80 still passes the raw agentStatusHooksEnabled preference into codexHookService.prepareRuntimeHomeForLaunch(runtimeHomePath, hookTarget, hooksEnabled). Its hooks-off branch reaches refreshRuntimeUserHooks → cleanupLegacyManagedHookRepresentations() → cleanupLegacySystemManagedHooks(), which mutates the real ~/.codex/config.toml — and systemCodexHomeHookSweepSuppressed requires isAgentStatusHooksEnabled, so hooks-off is exactly the state that un-suppresses that sweep. It is unreachable on the system-default lane, which returns null at line 64 before ever getting there, so it only fires for managed-home and WSL launches.

It is excluded from this PR on purpose. The two sites fixed here are unambiguous: they sweep the real ~/.codex on behalf of a profile that may own nothing in it, and the intent (don't reinstall) and the effect (delete) plainly diverge. The third is a different trade. The legacy cleanup exists to converge old system-home representations after the move to managed homes — downgrade and rollback convergence, not current-preference enforcement. Whether that convergence should also be Settings-toggle-only is a separate question from the one this PR answers, and changing it would alter behaviour for managed-home and WSL users who are not the population this fix is about. That deserves its own decision rather than riding along on this one. (An earlier draft of this section claimed dropping it would strand stale legacy entries — that was speculation and is withdrawn; the argument here is scope, not consequence.)

A reviewer who thinks the legacy sweep should also be Settings-toggle-only should say so — it will be a follow-up, not a change to this PR.

Linked Issue

STA-5679 (Linear). No GitHub issue exists for this one; split out of #19974.

Visual Proof

N/A — there is no UI change. This PR only removes calls into a hook/config writer on a disabled-hooks code path; nothing rendered changes in either state.

Testing

codex-launch-preparation.ts and codex-session-resume-launch.ts had no test files. Both now have one, each with a hooks-OFF assertion and a hooks-ON anti-vacuity anchor. isAgentStatusHooksEnabled is deliberately not mocked in either file, so the gate is read through the real predicate rather than a stub.

Every hooks-OFF assertion was ablated at the final commit — production change reverted, test confirmed failing, restored, confirmed green:

  • revert codex-launch-preparation.ts → never touches the user-global ~/.codex when this profile has status hooks off fails with expected "vi.fn()" to not be called at all, but actually been called 1 times / { "hooksEnabled": false, "userDataPath": "/user-data" }
  • revert codex-session-resume-launch.ts → both its hooks-OFF tests fail, the second showing the legacy refresh called with "/user-data/codex-home"
  • both production files at main → 3 failed | 4 passed (7)
  • both restored → 7 passed (7)

Gates, all green: pnpm tc, pnpm test src/main/startup src/main/codex/codex-real-home-hook-install.test.ts src/main/agent-hooks (1334 passed | 17 skipped), oxlint, check:code-quality:changed, check:react-doctor:changed, check:max-lines-ratchet, pnpm format, and all four verify:localization-* scripts.

  • I manually tested these changes locally
  • Automated tests added/updated, or explained why not below

Platforms: logic is platform-independent and exercised on macOS. The WSL branch in codex-launch-preparation.ts is untouched, and the real-home lane is native-only. No SSH surface: this is local ~/.codex state on the execution host.

AI Disclosure

Review

Agent skill upstream boundary

  • Not applicable, or this change follows docs/reference/agent-skill-sharing-upstream-boundary.md and copies or mechanically translates no upstream skill-installer source, tests, fixtures, registry entries, path tables, comments, or documentation.

Notes

  • Security: strictly reduces writes to user-global files; no new file or process surface.
  • Cross-platform: no platform-dependent logic added; the WSL guard is preserved unchanged.
  • Remote SSH: none — the real-home lane is local-only, and nothing about remote execution-host reporting changes.
  • Mobile: no surface.
  • Backwards compatibility: a profile with hooks off that previously swept on launch now leaves the entry in place; the Settings toggle still removes it. No wire, schema, or persisted-state change.
  • Performance: removes a file read/write and a trust-config mutation from every hooks-off pane launch.

Checklist

  • This PR is small and focused
  • I explained what changed and why (including ELI5)
  • Before/after screenshots or videos attached for UI changes, or N/A with reason
  • Self-reviewed for correctness, security, and performance
  • Cross-platform, SSH/remote, and path/shortcut impact considered (or N/A)
  • pnpm lint, pnpm typecheck, pnpm test, and pnpm build pass (or CI will cover; local preferred)

…real-home hooks (STA-5679)

`~/.codex/hooks.json` and `~/.codex/config.toml` are user-global, and
`sweepRealHomeCodexHook` matches Orca-managed entries by script filename, not by
which Orca profile installed them. So any profile with `agentStatusHooksEnabled`
off deleted the hook entry and the trust records another profile had installed,
just by launching or resuming a Codex pane.

Startup already honors the off switch by skipping the install (see
`main-process-ready-runtime.ts`, which gates on the preference and then passes a
hard `hooksEnabled: true`). Two per-pane twins were left passing the preference
through instead:

- `codex-launch-preparation.ts` routed a hooks-off launch into the real-home
  sweep.
- `codex-session-resume-launch.ts` did the same on the system-home lane, and its
  managed-home lane ran `refreshRuntimeUserHooksForLaunchPrep`, whose legacy
  cleanup mutates the real `~/.codex/config.toml` as well.

Both now mirror startup: gate on `isAgentStatusHooksEnabled` and pass a hard
`hooksEnabled: true`; with hooks off they do nothing at all.

Nothing legitimate is lost. Routing is unchanged — `isRealHomeCodexHookLaneUsable`
reads the lane as usable whether it is 'pending' or 'removed' — and opt-out
convergence already belongs to the Settings toggle, which reaches the real home
through `applyAgentStatusHooksEnabled(false)` -> `removeManagedAgentHooks()` ->
`cleanupLegacySystemManagedHooks()`. That path is user-initiated, which is where
STA-5679 says removal belongs.

The commit that exempted Codex from the original STA-5679 fix justified it with
"its hooks live in an Orca-owned runtime home" (4ff07da, 2026-08-27). That had
stopped being true six weeks earlier, when the system-default lane moved onto the
real `~/.codex` (c3f37e5, 2026-07-14).

Adds the first test files for both modules; each hooks-off assertion was ablated
against its production change.
@coderabbitai

coderabbitai Bot commented Sep 12, 2026 •

Copy link
Copy Markdown
Contributor

Review Change StackReview Change Stack

📝 Walkthrough

Walkthrough

The change adds status-hook gates to runtime-home and session-resume launch preparation. Disabled hooks now skip hook repair and installation. Enabled hooks use hooksEnabled: true for real-home installation and installForLaunchPrep for managed resume homes. New tests cover disabled, enabled, and unset preference states.

Priority: ➖ Normal

Merge Risk: 🟡 Moderate · up to fd9e0

Profiles with status hooks disabled can still modify managed Codex hook and trust configuration during launch preparation. Guard this path before merging.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 5 functions across 4 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly identifies the Codex launch bug and the protection against deleting another profile's real-home hooks. It is specific and related to the main change.
Description check ✅ Passed The description is complete and relevant. It explains the problem, implementation, scope, linked issue, testing, platform impact, and checklist status. The template's Fixes # placeholder is not used…
  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

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.

Caution

Some comments are outside the diff and can’t be posted inline due to GitHub limitations.

⚠️ Outside diff range comments (1)
src/main/startup/codex-launch-preparation.ts (1)

83-87: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Skip managed-home hook preparation when status hooks are disabled

When prepareForCodexLaunchAsync returns a managed home and hooksEnabled is false, prepareRuntimeHomeForLaunch refreshes that home. The refresh promotes approvals and rewrites managed hook state. Guard this call when status hooks are disabled, and add managed-home coverage.


ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Advanced

Run ID: 78146657-f935-4503-8ab7-21e807b11301

📥 Commits

Reviewing files that changed from the base of the PR and between ef6eeab and fd9e0d5.

📒 Files selected for processing (4)
  • src/main/startup/codex-launch-preparation.test.ts
  • src/main/startup/codex-launch-preparation.ts
  • src/main/startup/codex-session-resume-launch.test.ts
  • src/main/startup/codex-session-resume-launch.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 7 remain after this review.

@pullfrog pullfrog Bot left a comment

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.

Important

The resume early-return suppresses more than the real-home sweep: for a managed (non-system) resume home it also drops the user-hook mirroring that runs when Orca status hooks are off. One scope note and two nits below.

Reviewed changes

  • codex-launch-preparation.ts real-home gate: ensureRealHomeHooksIfSelected now returns early when this profile has status hooks off and passes a hard hooksEnabled: true, so a hooks-off launch neither sweeps nor reinstalls the user-global ~/.codex. No behavior change when hooks are on.
  • codex-session-resume-launch.ts resume gate: hooks-off now returns resumeHome before both the real-home installer and the legacy runtime-user refresh, leaving the shared ~/.codex untouched on resume.
  • Tests: first test files for both modules — hooks-off non-invocation assertions plus hooks-on / undefined anti-vacuity anchors, reading the real isAgentStatusHooksEnabled predicate, ablation-verified per the PR body.

ℹ️ Adjacent managed-home/WSL launch path still mutates the real home

The scope call in the PR body is correct: codex-launch-preparation.ts:80-87 still forwards this profile's raw hooksEnabled into prepareRuntimeHomeForLaunch, and for a native managed-home launch with hooks off that reaches refreshRuntimeUserHooksForLaunchPrep → refreshCodexRuntimeUserHooksExclusively → cleanupLegacyManagedHookRepresentations() → cleanupLegacySystemManagedHooks(), which writes ~/.codex/hooks.json and ~/.codex/config.toml. Since setSystemCodexHomeHookSweepSuppressed requires isAgentStatusHooksEnabled (main-process-account-services.ts:44-49), hooks-off is exactly the state that runs that sweep. Not a blocker for this PR, but since this is still a live instance of the same STA-5679 class for the managed-home/WSL population, it would be worth linking a follow-up so it isn't considered fully closed.

Technical details
# Third launch-prep site still sweeps the real home with hooks off

## Affected sites
- src/main/startup/codex-launch-preparation.ts:80-87 — raw hooksEnabled passed to prepareRuntimeHomeForLaunch.
- src/main/codex/codex-hook-service-implementation.ts:161,169-172 — hooks-off branch calls the runtime-user refresh.
- src/main/codex/codex-hook-local-maintenance.ts:87 — unconditional cleanupLegacyManagedHookRepresentations().
- src/main/codex/codex-hook-legacy-cleanup.ts:58-151 — sweeps ~/.codex/hooks.json and ~/.codex/config.toml.

## Required outcome
- Track as a follow-up, or state in a code comment that this path is intentionally permanent.

ℹ️ Nitpicks

  • codex-launch-preparation.ts:38: after a hooks-ON grant failure sets the real-home lane to unavailable, toggling hooks off now leaves it unavailable, so prepareForCodexLaunchAsync can fall back to the managed home where the old sweep reset the lane to removed and stayed on the real home. Narrow and arguably benign, but the PR body's "routing is unchanged" claim does not hold for that sequence.
  • codex-session-resume-launch.ts:91-94: the comment says "both branches below", but the runtime-user refresh branch no longer exists below the guard — reword to describe what would have run.

Pullfrog  | Fix all ➔ | Fix 👍s ➔ | View workflow run | Using openrouter/deepseek/deepseek-v4.1-flash (free via Pullfrog for OSS) | 𝕏

Comment on lines +90 to +96
if (!isAgentStatusHooksEnabled(store.getSettings())) {
// Why nothing at all: both branches below reach the user-global ~/.codex — the sweep
// directly, the runtime-user refresh through its legacy system-home cleanup — and match
// Orca entries by script filename, so THIS profile's off switch would delete the hook and
// trust records another profile installed (STA-5679). Removal stays on the Settings toggle.
return resumeHome
}

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 early return also drops refreshRuntimeUserHooksForLaunchPrep(resumeHome) for a managed resume home, and that call is not only the legacy sweep. refreshCodexRuntimeUserHooksExclusively mirrors the user's own ~/.codex hooks and trust into the runtime CODEX_HOME — its own comment says this is the path used when Orca status hooks are disabled, so the runtime home "should keep user hooks" (codex-hook-local-maintenance.ts:69-70). For a per-account managed resume home this call is the only place that mirror runs (reconcileSharedRuntimeAuth is false, so reconcileSharedRuntimeResumeHome returns without calling prepareCodexRuntimeHomeForLaunch). Is dropping the mirror intended, or should the managed branch keep mirroring while only the real-home sweep is skipped?

Technical details
# Managed resume homes lose user-hook mirroring when hooks are off

## Affected sites
- src/main/startup/codex-session-resume-launch.ts:90-96 — early return skips refreshRuntimeUserHooksForLaunchPrep for every resumeHome.
- src/main/codex/codex-hook-local-maintenance.ts:49-87 — refreshCodexRuntimeUserHooksExclusively mirrors system user hooks/trust into the runtime home, then runs cleanupLegacyManagedHookRepresentations().
- src/main/ipc/pty/host-env/codex-resume.ts:106-118 — reconcileSharedRuntimeResumeHome only calls prepareCodexRuntimeHomeForLaunch (which mirrors) when reconcileSharedRuntimeAuth is true, i.e. only for the shared managed home.

## Required outcome
- Confirm whether a per-account managed resume home must still receive the user-hook mirror when agentStatusHooksEnabled is false.
- If yes, keep the mirror and suppress only the real-home legacy sweep for the managed branch.

## Open questions for the human
- Is abandoning the managed-home mirror intended, or an oversight in the "nothing legitimate is lost" rationale?

@brennanb2025

Copy link
Copy Markdown
Contributor Author

Opening or resuming Codex now leaves the shared hook alone when this profile has hooks off. #23552 and #25743 replaced these launch paths, so this PR can be closed.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant