Repository navigation
fix(codex): migrate skill to documented user path - #1781
Conversation
|
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. |
DeusData
added
bug
|
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. |
DeusData
added
priority/high
|
Direction confirmed — thank you for doing the homework on this. I re-read Codex's current skills documentation today: the USER scope is Two things before it merges:
Minor, take or leave: a symlinked skill directory now records an install error ( With those two this merges on green — and it closes a real duplicate-skill annoyance, not just a doc mismatch. |
Signed-off-by: Benyamin <benyaminjmf@gmail.com>
Signed-off-by: Benyamin <benyaminjmf@gmail.com>
Signed-off-by: Benyamin <benyaminjmf@gmail.com>
6c3f52c to
7c88e64
Compare
|
Implemented the requested changes and rebased this branch onto the latest Released-content ownership
Shared-root behavior
Verification
The optional final-directory symlink case is not included, keeping this update focused on the required compatibility and migration behavior. |
|
CI update: every substantive check passed except The failing guard is the exact known Failed job: https://github.com/DeusData/codebase-memory-mcp/actions/runs/34943783091/job/104301975858 This branch does not modify daemon coordination, IPC, or I attempted a failed-only rerun, but GitHub requires upstream repository write/admin permission. Could a maintainer please rerun the failed jobs for run |
|
Thank you for addressing the ownership follow-up and removing the shared-root suppression. Those reported changes move this back to maintainer review; they are not being described as an independently completed security review. We need more time to review the current head and attribute its CI results before an integration decision. No further speculative rebase is requested. |
DeusData
left a comment
DeusData
left a comment
There was a problem hiding this comment.
Thank you, @agn-7. This is careful, thorough work, and it closes a real duplicate-skill annoyance for every Codex user who also runs Zed, OpenHands or Warp. Both follow-ups landed exactly as asked. We checked the released payloads byte for byte against the skill text in every tag from v0.6.0 through v0.11.0, and they all match, including v0.11.0. That matters because main has changed the skill text again since your base, so your list is what keeps v0.11.0 installs owned after the merge.
The destination-first migration, the exact-owned removal and the independent uninstall of both roots all read correctly. The merged tree is clean against current main, and the memory-core ratchet passes. The only red on the last run is test-windows-guards, which is the known cold-storm race (#2057).
We'll update the branch from our side so CI runs against current main, and it merges on green; nothing further is needed from you. One optional thought for a follow-up: cli_codex_skill_migrates_every_released_legacy_payload loops over production's own list, so a hash-pinned test would also catch a wrong entry. Thanks again!
|
Thank you again, @agn-7 -- this is careful work, and the released-payload list is exactly what keeps every older Codex install owned. We have updated the branch against current main so CI runs on today's tree. The one red on the previous run (a Windows daemon IPC test, |
|
Thank you again for the Codex migration work. Our October 20 update records that the branch refresh and upstream Windows test fix are already included, so nothing further is needed from you. Sorry for the delay in the remaining maintainer follow-up—I’ll follow up soon. |
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
|
Thank you again, @agn-7. We merged the current main into this branch once more so the required checks run against today's tree. It merged cleanly and your change is untouched: the same three commits of yours plus our merge commit. We also re-checked your released-payload list against every tagged release that ships the consolidated skill, v0.6.0 through v0.12.1. Each one matches one of your four payloads or the current text byte for byte, so every existing Codex install stays recognised as installer-owned and migrates safely. If you push to this branch again, please pull first so our merge commit is kept. It merges once the checks are green, and nothing further is needed from you. |
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
|
Thank you again, @agn-7, for the Codex skill migration. Tonight's merges on main touched README.md: the Codex CLI row in the agent table gained a note about the new PreToolUse graph lookup for Bash searches, in the same cell where your change names the ~/.agents/skills path. We merged current main into the branch once more (signed-off merge commit 994432f) and kept both edits in that row. The rest of your changes is untouched. Nothing is needed from you. If you push to this branch again, please pull first so the merge commit is kept. |
README agent table: keep main's Claude Code row (drift note) and this branch's Codex CLI row (skill path + PreToolUse). Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
|
Thank you for your patience, @agn-7. This evening main gained a drift-note hook, which changed the Claude Code row of the agent table in README.md right next to your Codex CLI row, so the two conflicted. We merged current main into your branch (one merge commit, 848975f) and kept both: main's Claude Code row and your Codex CLI row with the ~/.agents/skills path. The rest of your change is untouched. We will merge once the fresh checks are green; please pull before pushing again. |
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
|
Thank you for your patience, @agn-7. A pull request that merged into main a few minutes ago (#2290) changes src/cli/cli.c, which this pull request also changes, so the CI run in progress here could no longer vouch for today's main. We merged current main into your branch once more (one merge commit, ea2dbaa; it merged cleanly and your own change is untouched) and started a fresh run. We will merge once the required checks are green. Please pull before pushing again. |
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
|
Thank you for your patience, @agn-7. Main has moved since the last CI run on this branch, and some of the new commits change README.md, which this pull request also changes, so that run can no longer vouch for today's main. We merged current main into your branch (one merge commit, 9cbc350; it merged cleanly and your own change is untouched) and started a fresh run. We will merge once the required checks are green. Please pull before pushing again. |
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
|
Thank you for your patience, @agn-7. The macOS legs of the last CI run here were cancelled by GitHub itself: it is retiring its macOS 14 runners on November 2 and ran scheduled brownouts beforehand, so those jobs never got a runner. Nothing in your change caused it. Main now runs CI on macOS 15 (#2849), so we merged current main into your branch (one merge commit, 2418526; it merged cleanly and your own change is untouched) to get a run on the new runners. Please pull before pushing again. |
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
|
Thank you for your patience, @agn-7. Your change was green, but pull requests merged since its run touch src/cli/cli.c and tests/test_cli.c, which this pull request also changes, so that run could no longer vouch for today's main. We merged current main into your branch (no conflicts, your own change is unchanged) to get a fresh CI run on the combined tree. Please pull before pushing again. We will merge as soon as the required checks are green. |
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
|
Merged as f29e74f - thank you, @agn-7! This moves the Codex skill to the documented ~/.agents/skills location, migrates every previously installed copy safely, and ends the duplicate skill entry that Codex users who also run Zed, OpenHands or Warp were seeing. The care you put into the destination-first migration and the complete released-payload list is what made this safe to ship. Thank you for seeing it through all the way from #977. |
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.