Skip to content

fix(codex): honor CODEX_HOME during migration - #65261

Open
ericfadden wants to merge 2 commits into
NousResearch:mainfrom
ericfadden:agent/codex-home-migration
Open

fix(codex): honor CODEX_HOME during migration#65261
ericfadden wants to merge 2 commits into
NousResearch:mainfrom
ericfadden:agent/codex-home-migration

Conversation

@ericfadden

@ericfadden ericfadden commented Jul 15, 2026

Copy link
Copy Markdown

What does this PR do?

Honor CODEX_HOME when the Codex runtime migration is called without an explicit codex_home argument.

The migration helper previously fell straight back to ~/.codex, while the slash-command path calls migrate(config) without passing a home. That could write the managed configuration block to the wrong config.toml even though the spawned Codex runtime used CODEX_HOME.

The resolution order is now:

  1. Explicit codex_home argument
  2. Nonblank CODEX_HOME environment variable
  3. ~/.codex

This is separate from #57792, which changes profile-specific app-server worker routing rather than migration target selection.

Related Issue

N/A — this is a narrow migration bug with a direct regression test.

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)
  • ✨ New feature (non-breaking change that adds functionality)
  • 🔒 Security fix
  • 📝 Documentation update
  • ✅ Tests (adding or improving test coverage)
  • ♻️ Refactor (no behavior change)
  • 🎯 New skill (bundled or hub)

Changes Made

  • Updated hermes_cli/codex_runtime_plugin_migration.py to resolve the migration target from the explicit argument, then CODEX_HOME, then the existing home-directory fallback.
  • Added regression coverage in tests/hermes_cli/test_codex_runtime_plugin_migration.py for environment selection, explicit-argument precedence, and blank-environment fallback.

How to Test

  1. On current main, set CODEX_HOME to an isolated directory and call migrate(config) without codex_home; the migration uses the fallback home instead of the environment-selected home.
  2. On this branch, repeat the scenario; the migration selects CODEX_HOME. An explicit argument still wins, and a blank environment value still falls back to ~/.codex.
  3. Run scripts/run_tests.sh tests/hermes_cli/test_codex_runtime_plugin_migration.py tests/hermes_cli/test_codex_runtime_switch.py -q — 101 passed.
  4. Run ruff check hermes_cli/codex_runtime_plugin_migration.py tests/hermes_cli/test_codex_runtime_plugin_migration.py — passed.
  5. CodeRabbit CLI review against current origin/main — 0 findings. Redacted Gitleaks scan of origin/main..HEAD — no leaks.

The full repository runner was also executed: 41,911 tests passed and 36 failed in 15 unrelated files. The failures were outside this PR's files and involved macOS/Linux platform assumptions, live local provider/catalog state, active approval-mode state, and one timing threshold.

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • I've run pytest tests/ -q and all tests pass
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features)
  • I've tested on my platform: macOS 26.5.2

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) — N/A; the change aligns behavior with the existing CODEX_HOME documentation
  • I've updated cli-config.yaml.example if I added/changed config keys — N/A
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — N/A
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide
  • I've updated tool descriptions/schemas if I changed tool behavior — N/A

Screenshots / Logs

N/A — automated regression coverage exercises the path-selection behavior.

@alt-glitch alt-glitch added type/bug Something isn't working P3 Low — cosmetic, nice to have comp/cli CLI entry point, hermes_cli/, setup wizard codex sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades labels Jul 15, 2026
@ericfadden
ericfadden marked this pull request as ready for review July 15, 2026 23:46
@ericfadden ericfadden closed this Jul 16, 2026
@ericfadden ericfadden reopened this Jul 16, 2026
@teknium1

Copy link
Copy Markdown
Contributor

Thanks for the focused regression fix. Current main still calls migrate(config) without a target in hermes_cli/codex_runtime_switch.py:214-215, while migrate() falls directly to Path.home() / ".codex" in hermes_cli/codex_runtime_plugin_migration.py:644-646. That conflicts with the documented CODEX_HOME migration target in website/docs/reference/environment-variables.md:731.

The proposed precedence aligns migration, plugin discovery, and the app-server's active Codex home. The added cases in tests/hermes_cli/test_codex_runtime_plugin_migration.py cover environment resolution, explicit override precedence, and blank-value fallback. The affected current-main hunk is unchanged since the PR base, so this should salvage mechanically.

Automated hermes-sweeper review.

@teknium1 teknium1 added the sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform label Jul 18, 2026
@GottZ

GottZ commented Aug 3, 2026

Copy link
Copy Markdown

This was generated by AI during triage.

Summary

Four PRs are associated with this issue complex: #27764 addresses the permission downgrade during re-migration, #25857 fixes the earlier top-level TOML placement bug, #65261 selects the correct migration target via CODEX_HOME, and #66170 forwards dynamic Hermes MCP context. Only #27764 changes permission preservation; the other three address distinct migration defects.

Related pull requests

Suggested consolidation

Keep #27764 open with the concrete salvage path from its contributor review because it is the recorded best fix for #27616 but its current diff can duplicate a user-owned root key and does not fully preserve the three reported permission keys. Keep #65261 separate and open for its CODEX_HOME fix; #25857 should remain closed because its implementation landed through #26260, and #66170 should remain closed rather than being reopened through this unrelated permission issue. No listed PRs are duplicates.

Cross-PR triage: Reviewed 4 pull requests and 1 issue in this complex. Each diff was read against this issue; Assessment working set: 25 kB of PR diffs, 12 kB of issue/PR text, 4 kB of discussion (5 comments), 2 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch.

@TheAngryPit

Copy link
Copy Markdown
Contributor

I refreshed this fix against current main (49c632310). The production behavior remains correct, but the PR's original test hunk no longer applies mechanically because the migration test file has moved since the branch base.

The current-main refresh keeps this PR's owner-compatible precedence:

  1. explicit codex_home
  2. nonblank CODEX_HOME
  3. ~/.codex

I also added two focused regressions that cover adjacent paths not currently exercised here or in #71452:

  • plugin discovery receives the already-resolved environment Codex home;
  • the real /codex-runtime apply → migration chain writes to CODEX_HOME/config.toml rather than only asserting a mocked argument handoff.

Current-main verification:

44 passed, 0 failed
Ruff: clean
git diff --check: clean

The implementation is still only three files: the migration helper plus its unit tests and the runtime-switch integration test. I am not opening a successor because #65261 is already the canonical high-salvageability owner surface. @ericfadden, I have the current-main test refresh ready if you want it folded into this branch.

@alt-glitch alt-glitch added the needs-decision Awaiting maintainer decision before any implementation label Aug 10, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

codex comp/cli CLI entry point, hermes_cli/, setup wizard needs-decision Awaiting maintainer decision before any implementation P3 Low — cosmetic, nice to have sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants