Fix Windows Codex launcher + Node ESM compatibility #168
Conversation
WalkthroughThis PR migrates the shared skills core module from CommonJS (.js) to ES modules (.mjs), refactors the Codex CLI to use dynamic async imports, introduces a Windows batch launcher shim, and updates all consumer code and documentation to reference the new module path. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25–35 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/plans/2025-11-22-opencode-support-design.md (1)
48-56: Code example uses CommonJS syntax for an ES module file.The example shows
module.exports = {...}but the file is namedskills-core.mjs. ES modules useexportsyntax, notmodule.exports.Consider updating the example to use ESM syntax:
```javascript // lib/skills-core.mjs -module.exports = { - extractFrontmatter(filePath), // Parse name + description from YAML - findSkillsInDir(dir, maxDepth), // Recursive SKILL.md discovery - findAllSkills(dirs), // Scan multiple directories - resolveSkillPath(skillName, dirs), // Handle shadowing (personal > core) - checkForUpdates(repoDir) // Git fetch/status check -}; +export function extractFrontmatter(filePath) { ... } // Parse name + description from YAML +export function findSkillsInDir(dir, maxDepth) { ... } // Recursive SKILL.md discovery +export function findAllSkills(dirs) { ... } // Scan multiple directories +export function resolveSkillPath(skillName, dirs) { ... } // Handle shadowing (personal > core) +export function checkForUpdates(repoDir) { ... } // Git fetch/status check</blockquote></details> </blockquote></details>🧹 Nitpick comments (9)
docs/README.opencode.md (1)
156-165:.mjspath is correct; consider updating hook wording to match current implementationThe references to
lib/skills-core.mjsand its location are accurate and match the code. The only mismatch in this block is the still‑mentionedchat.messagehook, while the plugin now uses event hooks (session.created/session.compacted).You could align the docs with the implementation like this:
-**Components:** -- Two custom tools: `use_skill`, `find_skills` -- chat.message hook for initial context injection -- event handler for session.compacted re-injection -- Uses shared `lib/skills-core.mjs` module (also used by Codex) +**Components:** +- Two custom tools: `use_skill`, `find_skills` +- Event hook for initial context injection on `session.created` +- Event hook for `session.compacted` re-injection +- Uses shared `lib/skills-core.mjs` module (also used by Codex)lib/skills-core.js (1)
1-11: Makelib/skills-core.jsa minimal stub to avoid CJS parse issues and dead codeRight now this file throws with a clear message at the top but still contains the full implementation and an
export { ... }block. In a CommonJS context (no"type": "module"), theexport { ... }will trigger a SyntaxError before the top-levelthrowexecutes, so callers won't see your intended "moved to .mjs" message. The remaining implementation is also dead and can go out of sync withlib/skills-core.mjs.Consider turning this into a minimal, environment‑agnostic stub:
-/** - * NOTE: This file is intentionally NOT the source of truth anymore. - * - * The shared core module is `lib/skills-core.mjs` so it can be imported as an - * ES module without requiring a repo-level `package.json` with `"type":"module"`. - * - * If you are importing `lib/skills-core.js`, switch to `lib/skills-core.mjs`. - */ -'use strict'; - -throw new Error('superpowers: lib/skills-core.js has moved to lib/skills-core.mjs. Import that file instead.'); -… -export { - extractFrontmatter, - findSkillsInDir, - resolveSkillPath, - checkForUpdates, - stripFrontmatter -}; +/** + * NOTE: This file is intentionally NOT the source of truth anymore. + * + * The shared core module is `lib/skills-core.mjs` so it can be imported as an + * ES module without requiring a repo-level `package.json` with `"type":"module"`. + * + * If you are importing `lib/skills-core.js`, switch to `lib/skills-core.mjs`. + */ +'use strict'; + +throw new Error( + 'superpowers: lib/skills-core.js has moved to lib/skills-core.mjs. Import that file instead.' +);That way both
require('./lib/skills-core.js')andimport './lib/skills-core.js'surface the same clear error, and there's no secondary, unreachable implementation to maintain.lib/skills-core.mjs (5)
16-52: Frontmatter parser is intentionally minimal; consider key/format flexibilityThe logic is straightforward and matches the documented
name/descriptionformat, but it’s tightly constrained:
^(\w+):\s*(.*)$only matches single-word keys made ofA-Za-z0-9_. If you ever introduce keys with dashes or spaces (e.g.display-name,short description), they will be silently ignored.- Multi-line values and more complex YAML constructs are not supported.
If the intent is to keep SKILL frontmatter strictly to simple one-line
nameanddescription, this is fine; otherwise, relaxing the key pattern (e.g.^([^:]+):\s*(.*)$) or moving to a tiny YAML parser would future‑proof this.
62-97: Skill discovery recursion and depth handling look correct; think about error handling and symlinksThe recursive walk with
maxDepthis logically sound and mirrors the existing behavior:
- Starting at
depth = 0and stopping whendepth > maxDepthgives youmaxDepth + 1directory levels, which matches the JSDoc.- You correctly fallback to
entry.namewhennameis absent in frontmatter.Two potential robustness tweaks, non-blocking for now:
- If
fs.readdirSyncthrows (permissions, transient FS issues), the whole scan will fail. Wrapping the body ofrecursein a try/catch and skipping unreadable directories would make discovery more resilient.entry.isDirectory()will skip symlinked directories. If you ever want to support skills via symlinks, you’d need an additionalentry.isSymbolicLink()+fs.statSyncbranch.Given current usage this is acceptable, but worth noting if discovery ever becomes more dynamic.
148-170:checkForUpdatesbehavior is reasonable but can still stall for ~3sThe function is defensive (3s timeout and a broad catch returning
false), but keep in mind:
execSyncis still fully blocking for up to 3 seconds on each call, which will be noticeable ifcheckForUpdatesruns on every session bootstrap.git fetch origin && git status ...assumes a remote namedoriginand a networked repo; bare or offline clones will always hit the catch and quietly returnfalse.If you ever find startup latency becoming annoying, consider switching this to an asynchronous check that races against a shorter timeout and returns immediately on timeout, or gating it behind a config/env flag. For now the behavior is acceptable but worth documenting.
178-200:stripFrontmatterlogic is fine but may drop later---blocksThis implementation will:
- Correctly drop a single leading frontmatter block.
- Treat any subsequent
---markers as part of the (still “in frontmatter”) section and skip them, becauseinFrontmatteris never reset once set.If SKILL content never contains additional
---lines, this is fine. If it might, you could resetinFrontmatter = falseoncefrontmatterEndedis set, or more simply detect the first two---lines by index and slicecontentaccordingly.Also, now that you have
stripFrontmatterin the shared core, it’s a good opportunity to replace any duplicated “strip YAML header” loops in Codex / OpenCode plugin with a call to this function.
1-3: ESM imports/exports look good; considernode:specifiers as a minor polishThe move to a pure ESM module with named exports is clean and aligns with dynamic
import()from the launchers. One tiny stylistic improvement you might consider (not required):import fs from 'node:fs'; import path from 'node:path'; import { execSync } from 'node:child_process';Using
node:specifiers makes it explicit that these are built‑ins and can help some tooling. Behavior is identical in modern Node.Also applies to: 202-208
docs/plans/2025-11-22-opencode-support-implementation.md (2)
7-22: Plan doc still shows CommonJS forlib/skills-core.mjs; consider updating to ESM for accuracyThe implementation plan now calls out
lib/skills-core.mjsas the shared core, but the code snippets in Tasks 1–4 still use:
#!/usr/bin/env nodeconst fs = require('fs');module.exports = { ... }whereas the actual module is an ESM file using
import/exportand not intended to be executed directly.This is fine as a historical record of how the module used to be structured, but for future maintainers (or for anyone re-running these steps) it’s potentially misleading. I’d suggest either:
- Updating the snippets to match the current ESM implementation (imports and named exports), or
- Adding a short note near the top that the concrete code examples have since evolved and to refer to
lib/skills-core.mjsin the repo as the source of truth.That keeps this plan useful without sending people down the wrong path.
Also applies to: 23-81, 100-104, 185-242, 258-315
768-883: Markdownlint MD036: bold “Step” lines vs proper headingsThe doc uses bold lines like
**Step 1: ...**as pseudo‑headings, which triggers MD036 (“emphasis used instead of a heading”) in markdownlint. This is cosmetic, but if you want a clean lint run you could:
- Change them to actual headings, e.g.
### Step 1: ..., or- Configure markdownlint to allow this style in this file.
Given this is a planning doc, I’d treat it as a low‑priority, when‑you-touch-it-next cleanup.
Also applies to: 1088-1094
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
.codex/INSTALL.md(1 hunks).codex/superpowers-bootstrap.md(1 hunks).codex/superpowers-codex(2 hunks).codex/superpowers-codex.cmd(1 hunks).opencode/plugin/superpowers.js(1 hunks)RELEASE-NOTES.md(1 hunks)docs/README.codex.md(2 hunks)docs/README.opencode.md(1 hunks)docs/plans/2025-11-22-opencode-support-design.md(5 hunks)docs/plans/2025-11-22-opencode-support-implementation.md(13 hunks)lib/skills-core.js(1 hunks)lib/skills-core.mjs(1 hunks)tests/opencode/run-tests.sh(1 hunks)tests/opencode/test-plugin-loading.sh(1 hunks)tests/opencode/test-skills-core.sh(1 hunks)🧰 Additional context used
🪛 markdownlint-cli2 (0.18.1)
docs/plans/2025-11-22-opencode-support-implementation.md
21-21: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
88-88: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
103-103: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
170-170: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
249-249: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
322-322: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
🔇 Additional comments (14)
.codex/INSTALL.md (1)
35-43: Windows verification commands look consistent with new launcher shimThe added
.cmdandnodevariants align with the new Windows launcher behavior and give clear alternatives without changing existing Unix instructions.tests/opencode/run-tests.sh (1)
45-48: Updated test description matches module renameThe help text now refers generically to “skills-core library functions”, which stays correct after moving from
.jsto.mjs..opencode/plugin/superpowers.js (1)
13-31: ESM import path update looks correctSwitching the plugin to import
../../lib/skills-core.mjskeeps the same API surface (resolveSkillPath,findSkillsInDir, etc.) while aligning with the new ES module core.RELEASE-NOTES.md (1)
30-47: Release notes accurately describe the sharedskills-core.mjsmigrationThe added bullets correctly capture that
lib/skills-core.mjsis now the shared core for OpenCode and Codex, and that Codex was refactored to use the ES module variant.tests/opencode/test-skills-core.sh (1)
2-4: Comment update matches how the tests exercise skills-coreThe description now correctly states that it tests the “skills-core library functions” without tying it to a specific filename, which fits the current approach of inlining those helpers.
.codex/superpowers-bootstrap.md (1)
6-10: Windows and Node variants are clear and consistent with the launcherThe added bullets for the
.cmdshim and directnodeinvocation nicely document the new Windows behavior while keeping the original Unix command as the primary example.tests/opencode/test-plugin-loading.sh (1)
33-40: LGTM!The test correctly updated to verify the new ESM module path (
skills-core.mjs). The pass/fail messages are clear and consistent with the module rename.docs/README.codex.md (3)
94-97: LGTM!Clear Windows-specific guidance that addresses the core issue this PR fixes. The two options (
.cmdshim and explicitnodeinvocation) give users flexibility.
105-107: LGTM!Documentation correctly updated to reference the new ES module path (
lib/skills-core.mjs) and notes the ES module format.
139-143: LGTM!Helpful troubleshooting section for Windows users encountering execution issues with the extensionless script.
docs/plans/2025-11-22-opencode-support-design.md (1)
32-35: LGTM!Design document correctly updated to reference the new ES module path throughout the architecture, file structure, and implementation plan sections.
Also applies to: 203-203, 219-219, 226-226, 238-238
.codex/superpowers-codex (3)
6-8: LGTM!Good approach using
pathToFileURLfor cross-platform file URL generation. The module-scopedlet skillsCoreis assigned before any consuming code runs due to the asyncmain()flow.
243-276: LGTM!The async
main()pattern correctly handles the dynamic ESM import from CommonJS. The inline comment clearly explains why this approach is necessary (extensionless shebang script needing CommonJS, importing an ESM module). The command dispatch logic is clean and unchanged from before.
278-281: LGTM!Good error handling pattern—using
process.exitCode = 1instead ofprocess.exit(1)allows pending I/O to complete before the process exits. Theerror?.stack || String(error)fallback handles both Error objects and other thrown values.
| @echo off | ||
| setlocal | ||
|
|
||
| REM Windows shim for the extensionless Unix-style Node launcher (superpowers-codex). | ||
| REM This enables running: | ||
| REM C:\Users\dan\.codex\superpowers\.codex\superpowers-codex.cmd bootstrap | ||
| REM or (in many shells) just: | ||
| REM superpowers-codex bootstrap | ||
| REM when this directory is on PATH. | ||
|
|
||
| node "%~dp0superpowers-codex" %* | ||
|
|
There was a problem hiding this comment.
Exit code is not propagated to the caller.
When node exits with a non-zero code (e.g., on error), the batch script doesn't forward it, which could mislead callers or CI scripts expecting accurate exit status.
Apply this diff to preserve the exit code:
@echo off
setlocal
REM Windows shim for the extensionless Unix-style Node launcher (superpowers-codex).
REM This enables running:
REM C:\Users\dan\.codex\superpowers\.codex\superpowers-codex.cmd bootstrap
REM or (in many shells) just:
REM superpowers-codex bootstrap
REM when this directory is on PATH.
-node "%~dp0superpowers-codex" %*
+node "%~dp0superpowers-codex" %*
+exit /b %errorlevel%📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @echo off | |
| setlocal | |
| REM Windows shim for the extensionless Unix-style Node launcher (superpowers-codex). | |
| REM This enables running: | |
| REM C:\Users\dan\.codex\superpowers\.codex\superpowers-codex.cmd bootstrap | |
| REM or (in many shells) just: | |
| REM superpowers-codex bootstrap | |
| REM when this directory is on PATH. | |
| node "%~dp0superpowers-codex" %* | |
| @echo off | |
| setlocal | |
| REM Windows shim for the extensionless Unix-style Node launcher (superpowers-codex). | |
| REM This enables running: | |
| REM C:\Users\dan\.codex\superpowers\.codex\superpowers-codex.cmd bootstrap | |
| REM or (in many shells) just: | |
| REM superpowers-codex bootstrap | |
| REM when this directory is on PATH. | |
| node "%~dp0superpowers-codex" %* | |
| exit /b %errorlevel% | |
🤖 Prompt for AI Agents
.codex/superpowers-codex.cmd lines 1-12: the batch shim runs node but does not
propagate node's exit code to the caller; after invoking node
"%~dp0superpowers-codex" %* capture and return node's exit code by exiting the
script with the current ERRORLEVEL (add a final line to exit /b %ERRORLEVEL% so
callers/CI receive the same exit status).
| function resolveSkillPath(skillName, superpowersDir, personalDir) { | ||
| // Strip superpowers: prefix if present | ||
| const forceSuperpowers = skillName.startsWith('superpowers:'); | ||
| const actualSkillName = forceSuperpowers ? skillName.replace(/^superpowers:/, '') : skillName; | ||
|
|
||
| // Try personal skills first (unless explicitly superpowers:) | ||
| if (!forceSuperpowers && personalDir) { | ||
| const personalPath = path.join(personalDir, actualSkillName); | ||
| const personalSkillFile = path.join(personalPath, 'SKILL.md'); | ||
| if (fs.existsSync(personalSkillFile)) { | ||
| return { | ||
| skillFile: personalSkillFile, | ||
| sourceType: 'personal', | ||
| skillPath: actualSkillName | ||
| }; | ||
| } | ||
| } | ||
|
|
||
| // Try superpowers skills | ||
| if (superpowersDir) { | ||
| const superpowersPath = path.join(superpowersDir, actualSkillName); | ||
| const superpowersSkillFile = path.join(superpowersPath, 'SKILL.md'); | ||
| if (fs.existsSync(superpowersSkillFile)) { | ||
| return { | ||
| skillFile: superpowersSkillFile, | ||
| sourceType: 'superpowers', | ||
| skillPath: actualSkillName | ||
| }; | ||
| } | ||
| } | ||
|
|
||
| return null; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n lib/skills-core.mjs | sed -n '100,150p'Repository: obra/superpowers
Length of output: 2360
🏁 Script executed:
rg 'resolveSkillPath' -A 3 -B 3Repository: obra/superpowers
Length of output: 9183
🏁 Script executed:
python3 << 'EOF'
import os
# Simulate what path.join does with traversal patterns
test_cases = [
('../other-dir/secret', '/home/user/skills'),
('../../etc/passwd', '/home/user/skills'),
('normal-skill', '/home/user/skills'),
('.../something', '/home/user/skills'),
]
for skillName, baseDir in test_cases:
# Using os.path.join (equivalent to Node's path.join)
result = os.path.join(baseDir, skillName)
normalized = os.path.normpath(result)
print(f"skillName: {skillName!r}")
print(f" os.path.join result: {result}")
print(f" os.path.normpath result: {normalized}")
print(f" Escapes baseDir? {not normalized.startswith(baseDir)}")
print()
EOFRepository: obra/superpowers
Length of output: 146
🏁 Script executed:
rg 'resolveSkillPath\(' -B 5 -A 5 lib/ --type js --type mjs | head -100Repository: obra/superpowers
Length of output: 87
🏁 Script executed:
rg 'skill.?name|skillName' lib/ --type js --type mjs -B 2 -A 2Repository: obra/superpowers
Length of output: 87
🏁 Script executed:
rg 'superpowersDir|personalDir' lib/ --type js --type mjs -B 3 -A 3Repository: obra/superpowers
Length of output: 87
🏁 Script executed:
rg 'resolveSkillPath' lib/ -B 5 -A 5Repository: obra/superpowers
Length of output: 2421
🏁 Script executed:
git ls-files lib/ | grep -E '\.(mjs|js)$'Repository: obra/superpowers
Length of output: 98
🏁 Script executed:
rg 'resolveSkillPath' --type-list | head -20Repository: obra/superpowers
Length of output: 449
🏁 Script executed:
find lib -name "*.mjs" -o -name "*.js" | xargs grep -l "resolveSkillPath"Repository: obra/superpowers
Length of output: 98
🏁 Script executed:
grep -r "resolveSkillPath" . --include="*.js" --include="*.mjs" --include="*.ts" --include="*.jsx" --include="*.mts"Repository: obra/superpowers
Length of output: 622
🏁 Script executed:
ls -la | grep -E '\.(js|mjs)$'Repository: obra/superpowers
Length of output: 42
🏁 Script executed:
find . -name "*.mjs" -o -name "*.js" | head -20 | xargs cat | grep -A 10 -B 5 "skill.?name"Repository: obra/superpowers
Length of output: 42
🏁 Script executed:
cat -n package.json | head -50Repository: obra/superpowers
Length of output: 104
🏁 Script executed:
cat -n .opencode/plugin/superpowers.js | head -100Repository: obra/superpowers
Length of output: 4841
🏁 Script executed:
grep -B 20 -A 10 "resolveSkillPath" .opencode/plugin/superpowers.jsRepository: obra/superpowers
Length of output: 2808
🏁 Script executed:
grep -B 50 "skill_name" .opencode/plugin/superpowers.js | head -80Repository: obra/superpowers
Length of output: 3178
🏁 Script executed:
node << 'EOF'
const path = require('path');
// Test cases simulating the vulnerability
const baseDir = '/home/user/skills';
const testCases = [
'../../../etc/passwd',
'../../.ssh/id_rsa',
'normal-skill',
'../other-dir/secret',
];
console.log('Testing path.join behavior:');
testCases.forEach(skillName => {
const joined = path.join(baseDir, skillName);
const normalized = path.normalize(joined);
const startsWith = normalized.startsWith(baseDir);
console.log(`\nskillName: ${skillName}`);
console.log(` path.join result: ${joined}`);
console.log(` path.normalize: ${normalized}`);
console.log(` Stays within baseDir? ${startsWith}`);
});
EOFRepository: obra/superpowers
Length of output: 655
🏁 Script executed:
cat -n .opencode/plugin/superpowers.js | sed -n '86,120p'Repository: obra/superpowers
Length of output: 1844
Fix path traversal vulnerability in resolveSkillPath and plugin's use_skill tool
resolveSkillPath and the calling code in .opencode/plugin/superpowers.js (lines 98, 111) directly join user-supplied skillName into base directories without validation. Node's path.join() allows .. components to escape intended roots—a user passing ../../../etc/passwd can read arbitrary files.
Both locations need sanitization:
if (actualSkillName.includes('/') || actualSkillName.includes(path.sep)) {
return null;
}Or validate the resolved path stays within the base directory:
const resolved = path.resolve(baseDir, actualSkillName);
if (!resolved.startsWith(path.resolve(baseDir))) {
return null;
}The first approach (reject separators) is simpler and aligns with skill naming conventions.
🤖 Prompt for AI Agents
In lib/skills-core.mjs around lines 108-140 (and mirror the same validation in
.opencode/plugin/superpowers.js at the calls on lines ~98 and ~111), the
function currently joins user-supplied skillName into base dirs allowing path
traversal; reject or sanitize inputs before joining: validate actualSkillName
does not contain path separators or parent‑up segments (e.g., contains '/' or
path.sep or '..') and return null if it does, or alternatively resolve the
candidate path against the base dir and ensure the resolved path starts with the
base dir's resolved path; apply the same check in the plugin before calling
resolveSkillPath so malicious names are rejected early.
2 participants
Summary
Key changes
Why
Windows does not execute extensionless shebang scripts; it tends to open them. The .cmd shim ensures execution on Windows while keeping Linux behavior unchanged.
Summary by CodeRabbit
New Features
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.