fix(codex): migrate skill to documented user path - #1781
Conversation
Signed-off-by: Benyamin <benyaminjmf@gmail.com>
|
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. What that means for this PR, concretely:
Things that will genuinely speed it up whenever review does happen:
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>
|
Thank you for the detailed ownership and migration work. I checked current 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. |
Summary
$HOME/.agents/skillsroot while keeping config, hooks, and profiles under$CODEX_HOME$CODEX_HOME/skillsonly after the canonical copy is written and verified--forceexplicitly authorizes replacementRationale
This follows the opportunity recorded when #977 was closed. Current Codex builds discover both
$HOME/.agents/skillsand$CODEX_HOME/skills, but the public documentation names$HOME/.agents/skillsas 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;
--forceis required to replace and migrate it.Safety
Verification
make -f Makefile.cbm cbmscripts/smoke-test.sh build/c/codebase-memory-mcp --agent-config-only286 passed, 1 failedbecause the host-installed Hermes CLI triggers the pre-existingcli_uninstall_quiesces_active_cohort_before_removing_binary_and_indexenvironment-sensitive failuremake -f Makefile.cbm lint-format CLANG_FORMAT=/Library/Developer/CommandLineTools/usr/bin/clang-format32/32when run outside the filesystem/network sandbox required for local IPCscripts/lint.shcould not runclang-tidyorcppchecklocally because those executables are not installed; the no-skips and clang-format checks pass.Related Work
#1739 also introduces
cleanup_actions_plannedfor a separate CodexAGENTS.mdcleanup. This PR uses the same receipt field for legacy skill cleanup and does not changeAGENTS.mdbehavior. 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.