Add codex-global target in skillpack-install.mjs#12
Conversation
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
jonathanbossenger
left a comment
There was a problem hiding this comment.
@jmakinen81 thank you for the PR.
Review summary
Infrastructure-only PR adding codex-global as a new install target in skillpack-install.mjs. The change follows the existing *-global pattern established by claude-global and cursor-global — same source dir mapping, ~/.codex/skills/ destination, isGlobal flag, and --dest exemption. Mechanically solid and consistent with the codebase conventions.
✅ Looks good
- Follows the established pattern exactly. Every place in the code that special-cases
claude-globalorcursor-globalhas been extended with the matchingcodex-globalentry —VALID_TARGETS,getSourceDir,getDestDir,isGlobalcheck, andneedsDestguard. No spot was missed. - Usage text and examples updated. The help output includes the new target in the options list, the targets section, and a new example block — consistent with how
cursor-globalis documented in the same help text. - Minimal, focused diff. One file, one concern, no unrelated changes. Easy to review and low risk.
🟡 Should-fix
- Missing
--globalshorthand parity.--globalis currently hardcoded toclaude-globalonly. The existingcursor-globaltarget also lacks a shorthand, so this isn't a regression — but the PR is a good opportunity to address the growing inconsistency. At minimum, the help text for--global(line:" --global Shorthand for --targets=claude-global...") should note it's Claude-specific, or the PR could propose a--codex-globalflag (or make--globalaccept a value like--global=codex). Worth a conversation with maintainers on the preferred direction. docs/packaging.mdnot updated. The packaging doc lists the dist output directories and install examples but doesn't mention any global targets at all. Since this PR adds a new global target, it should at least addcodex-globalto the install examples — and ideally bring the doc up to date with the existingclaude-globalandcursor-globaltargets too.README.mdnot updated. The README has explicit "Install globally for Claude Code" and "Install globally for Cursor" sections but no equivalent for Codex. Users discovering the feature via the README won't knowcodex-globalexists. A matching section like the Cursor one (3 lines) would complete the picture.
💭 Nits
- Growing ternary chain in
getSourceDir. ThesourceTargetassignment is now a 4-branch ternary. Not a problem today, but one more target addition will make it hard to read. A lookup object ({ "claude-global": "claude", "codex-global": "codex", "cursor-global": "cursor" }) with a fallback would be cleaner — though this is arguably a pre-existing issue, not something this PR introduced. - Potential merge conflicts. PRs #10, #20, and #31 are also open and likely touch
VALID_TARGETSand the same mapping functions. Whoever merges second will need to resolve conflicts. Not actionable for this PR, just worth being aware of.
|
@jmakinen81 as we've not heard back from you around the requested changes, I'm closing this PR for now. Please feel free to reopen it with your feedback or changes if you'd like to move it forward. |
I am testing codex app and installing skills there globally would be nice. Added new target codex-global.