Skip to content

fix(codex): migrate skill to documented user path - #1781

Open
agn-7 wants to merge 2 commits into
DeusData:mainfrom
agn-7:feat/codex-skill-location-compat
Open

fix(codex): migrate skill to documented user path#1781
agn-7 wants to merge 2 commits into
DeusData:mainfrom
agn-7:feat/codex-skill-location-compat

Conversation

@agn-7

@agn-7 agn-7 commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

Summary

  • install the Codex user skill at the documented $HOME/.agents/skills root while keeping config, hooks, and profiles under $CODEX_HOME
  • migrate byte-identical installer-owned legacy copies from $CODEX_HOME/skills only after the canonical copy is written and verified
  • preserve foreign, modified, linked, or unsafe content unless --force explicitly authorizes replacement
  • cover canonical/legacy reconciliation in plan, dry-run, repeat install, failure, force, and uninstall paths

Rationale

This follows the opportunity recorded when #977 was closed. Current Codex builds discover both $HOME/.agents/skills and $CODEX_HOME/skills, but the public documentation names $HOME/.agents/skills as the user-scoped location. Permanently writing the same named skill to both roots creates duplicate selector entries, so this change converges provably owned installs to one canonical copy instead.

The legacy path is not treated as broken. Unowned legacy content remains in place and prevents creation of a duplicate canonical copy; --force is required to replace and migrate it.

Safety

  • destination publication is verified before owned legacy cleanup
  • cleanup uses the existing exact-content ownership APIs and preserves sibling files
  • equal or same-file roots are deduplicated
  • hook-preflight failures cannot let another shared-root client create a second Codex copy
  • uninstall checks canonical and legacy roots independently and removes only exact-owned content

Verification

  • make -f Makefile.cbm cbm
  • scripts/smoke-test.sh build/c/codebase-memory-mcp --agent-config-only
  • CLI suite: all new Codex tests and the extended preflight regression pass; local aggregate is 286 passed, 1 failed because the host-installed Hermes CLI triggers the pre-existing cli_uninstall_quiesces_active_cohort_before_removing_binary_and_index environment-sensitive failure
  • make -f Makefile.cbm lint-format CLANG_FORMAT=/Library/Developer/CommandLineTools/usr/bin/clang-format
  • security layers pass, including MCP fuzz 32/32 when run outside the filesystem/network sandbox required for local IPC

scripts/lint.sh could not run clang-tidy or cppcheck locally because those executables are not installed; the no-skips and clang-format checks pass.

Related Work

#1739 also introduces cleanup_actions_planned for a separate Codex AGENTS.md cleanup. This PR uses the same receipt field for legacy skill cleanup and does not change AGENTS.md behavior. If #1739 merges first, the resolution is to retain the shared field and both cleanup action types.

Follow-up to #977. Related merged hardening: #1373.

Signed-off-by: Benyamin <benyaminjmf@gmail.com>
@agn-7
agn-7 requested a review from DeusData as a code owner August 21, 2026 10:58
@github-actions

Copy link
Copy Markdown

Thanks for opening this — it has been seen, and it is queued.

This note is automated, but it is not a brush-off: it exists so you know where your PR stands instead of having to guess from silence.

Current review status: working through a backlog. 0.9.1-rc.1 is out, so the release freeze that held reviews is over — but it left a large queue of open pull requests behind it, and we are reading through them oldest-first. The background is in discussion #1144.

What that means for this PR, concretely:

  • It will not be closed for inactivity. No stale bot touches pull requests here.
  • It may still sit a while before a human reads it. That is on us, not on you.
  • Older PRs are read first, so a recent one is not being skipped — it is behind a queue.

Things that will genuinely speed it up whenever review does happen:

  • Keep it rebased on main — the tree is moving quickly right now, and a conflicting branch cannot be reviewed as the diff you intended.
  • Get CI green, or say which failures you believe are pre-existing.
  • Keep the change to one claim. Bundled features and refactors get split before they get merged, which costs you a round trip.
  • Every commit needs a sign-off (git commit -s) — CI enforces DCO.

If this fixes a bug, a reproduction we can run is worth more than a description of the symptom.

Thanks for contributing, and sorry in advance for the wait.

Signed-off-by: Benyamin <benyaminjmf@gmail.com>
@DeusData DeusData added bug Something isn't working editor/integration Editor compatibility and CLI integration ux/behavior Display bugs, docs, adoption UX security Security vulnerabilities, hardening priority/normal Standard review queue; useful PR with ordinary maintainer urgency. labels Aug 24, 2026
@DeusData

Copy link
Copy Markdown
Owner

Thank you for the detailed ownership and migration work. I checked current main: Codex configuration honors CODEX_HOME, and the installer currently derives the Codex skill directory as $CODEX_HOME/skills; install and uninstall both use that same legacy location. The local migration premise is therefore grounded.

I have labeled this as an editor-integration, UX, and installer-security item and routed it for detailed review. The external Codex path contract and the exact-owned cleanup behavior still need to be verified together before any merge decision, especially because this changes where durable user content is written and removed. The PR is one coherent migration despite its size, so I am not asking for an artificial split at triage. Our review queue is full, and this may take a little time. Thank you for covering plan, dry-run, repeat-install, failure, force, and uninstall paths.

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

Labels

bug Something isn't working editor/integration Editor compatibility and CLI integration priority/normal Standard review queue; useful PR with ordinary maintainer urgency. security Security vulnerabilities, hardening ux/behavior Display bugs, docs, adoption UX

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants