feat(codex): add native runtime support - #375
Open
cmartins88 wants to merge 1 commit into
Open
Conversation
Install nForma workflows as Codex skills, convert custom agents and hooks, manage MCP configuration safely, and cover install/reinstall/uninstall behavior.
WalkthroughChangesCodex runtime support
Estimated code review effort: 4 (Complex) | ~60 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Installer
participant CodexSkills
participant Providers
participant CodexConfig
participant CodexHooks
Installer->>CodexSkills: Install converted skills
Installer->>Providers: Persist normalized providers
Installer->>CodexConfig: Write managed MCP configuration
Installer->>CodexHooks: Write and normalize hooks.json
CodexHooks-->>Installer: Complete Codex setup
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 7
🧹 Nitpick comments (6)
test/install-virgin.test.cjs (3)
77-118: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the shared env block from
runInstall/runUninstall.The two helpers carry byte-identical env literals. They must stay in sync (an install-side
NF_INSTALL_SKIP_*that isn't mirrored on uninstall silently changes what the uninstall path exercises), so a single builder removes the drift class.♻️ Proposed extraction
+function testEnv(homeDir) { + return { + ...process.env, + // Prevent any env var overrides from affecting test + CLAUDE_CONFIG_DIR: undefined, + GEMINI_CONFIG_DIR: undefined, + CODEX_HOME: undefined, + CODEX_CONFIG_DIR: undefined, + OPENCODE_CONFIG_DIR: undefined, + OPENCODE_CONFIG: undefined, + XDG_CONFIG_HOME: undefined, + // Skip heavy network installs (River ML, `@huggingface/transformers`) to avoid CI timeouts + NF_INSTALL_SKIP_OPTIONAL: '1', + NF_INSTALL_SKIP_FORMAL: '1', + ...(homeDir ? { HOME: homeDir, USERPROFILE: homeDir } : {}), + }; +}Then both helpers use
env: testEnv(homeDir).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/install-virgin.test.cjs` around lines 77 - 118, Extract the duplicated environment object from runInstall and runUninstall into a shared testEnv(homeDir) helper. Preserve all existing variable overrides, optional-install flags, and conditional HOME/USERPROFILE behavior, then replace both inline env literals with env: testEnv(homeDir) so the helpers remain synchronized.
311-314: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGuard
readIfExistsresults before matching.
readIfExistsreturnsnullon a missing file, soassert.match(null, ...)andJSON.parse(null)surface as aTypeError(or a confusingCannot read properties of null) rather than a message naming the file that wasn't written — exactly the diagnostic you want when a Codex install regresses. Line 301 already does this fornewProject; apply the same guard toplanner,hooks.json,configLoader,promptHook, andconfig.toml.A small helper keeps it terse:
♻️ Proposed helper
+function mustRead(filePath) { + const content = readIfExists(filePath); + assert.ok(content, `expected file to exist: ${filePath}`); + return content; +}Then e.g.
const planner = mustRead(path.join(agentsDir, 'nf-planner.toml'));andJSON.parse(mustRead(path.join(tmpDir, 'hooks.json'))).Also applies to: 318-318, 332-336, 363-364
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/install-virgin.test.cjs` around lines 311 - 314, Guard every readIfExists result in the install assertions by introducing and using a mustRead helper that reports the missing file path before returning its contents. Apply it to planner, hooks.json, configLoader, promptHook, and config.toml, including the JSON.parse input, while preserving the existing assertions and parsing behavior.
375-396: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSuite is order-dependent; make that explicit.
re-install is idempotentanduninstall removes native Codex integrationmutate the sharedtmpDir/homeDirstate seeded inbefore, and the uninstall test tears it down. Any test appended after line 396 will fail against an uninstalled tree. A short comment above the idempotency test noting "these two must run last, in this order" prevents that trap.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/install-virgin.test.cjs` around lines 375 - 396, Add a concise comment immediately above the “re-install is idempotent across native files and managed TOML” test documenting that this test and the following uninstall test must run last and in their current order, since they mutate and then tear down the shared seeded tmpDir/homeDir state.bin/codex-install.cjs (1)
217-254: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRebuild
aliasesoutside the function.The
aliasesmap is a constant but is re-created on every call. Hoist it to module scope.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bin/codex-install.cjs` around lines 217 - 254, Move the constant aliases map out of normalizeDetectedProvider and define it once at module scope. Keep the existing codex and copilot alias entries and continue referencing the hoisted map when assigning extraTools.bin/codex-install.test.cjs (1)
97-113: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the destructive helpers.
The suite covers pure conversion functions, but the filesystem-mutating ones —
removeCodexSkills/removeOwnedSkillDirectories(must not delete non-nf:skill directories) andremoveCodexMcp(must not delete aconfig.tomlthat still has user content) — are untested despite being the riskiest paths. This test already establishes the tmpdir +t.afterpattern to extend.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bin/codex-install.test.cjs` around lines 97 - 113, Extend the test suite using the existing tmpdir and t.after cleanup pattern to cover removeCodexSkills/removeOwnedSkillDirectories and removeCodexMcp. Verify skill cleanup removes only directories owned by the nf: marker and preserves unrelated skill directories, and verify MCP cleanup preserves config.toml when it contains user content while still covering the removable case.bin/install.js (1)
2610-2610: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffCodex skills are absent from the manifest.
For Codex the primary artifacts are the
SKILL.mdfiles undercodexSkillsDir(outsideconfigDir), so drift/modification detection covers agents but not the workflows themselves. Consider hashing the Codex skill files too, keyed by a stable relative prefix.Also applies to: 2626-2633
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bin/install.js` at line 2610, Update writeManifest to include Codex SKILL.md files from codexSkillsDir in the manifest’s drift/modification hashes, using a stable relative-path prefix so entries remain consistent independently of configDir. Preserve the existing agent manifest entries and apply this only to the Codex runtime path.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@bin/codex-install.cjs`:
- Around line 326-332: Update replaceManagedMcpBlock to construct its
managedPattern with the global flag so every managed MCP block is removed before
inserting the replacement block. Apply the same global-regex behavior to the
pattern used by removeCodexMcp, preserving existing matching and cleanup
behavior.
- Around line 334-343: Update configureCodexMcp to ignore a read failure only
when fs.readFileSync throws an ENOENT error, preserving the missing-config
first-install behavior. For any other error, propagate it and do not call
fs.writeFileSync, preventing existing config.toml contents from being
overwritten.
- Around line 256-298: Update ensureCodexProviders so entries for which
normalizeDetectedProvider returns null are retained in the providers array using
their original objects instead of being filtered out. Preserve these
unnormalizable entries when rewriting providersPath, while continuing to use
normalized entries for deduplication, selection, and the active return value.
In `@bin/install.js`:
- Around line 2492-2497: Restrict Codex validation to nForma-owned skills in the
`isCodex` command-count logic and the transformation scan around the Codex
validation block. Filter entries using the `nf-*` directories produced by
`installCodexSkills` or frontmatter names beginning with `nf:`, so unrelated
skills neither contribute to `EXPECTED_COMMANDS` nor trigger transformation
failures.
- Around line 3204-3224: Fix the uv availability flow around _spawnRiver and the
uvAvailable branch: do not invoke curl in a way that only downloads the
installer without executing it, and avoid relying on the Unix-only which command
on Windows. Either deliberately execute the installer through an appropriate
shell while preserving the existing failure handling, or remove the installation
attempt and report that uv is required; ensure the resulting PATH check and
River ML skip behavior remain accurate across platforms.
- Around line 2717-2721: Update the codexSkillsDir calculation alongside
targetDir so global Codex skill installs and removals derive from the resolved
Codex config directory, honoring --config-dir, CODEX_HOME, and CODEX_CONFIG_DIR;
retain the project-local .agents/skills path for non-global installs and use
~/.codex/.agents/skills as the minimum fallback.
In `@test/install-virgin.test.cjs`:
- Around line 340-354: Add a 120-second timeout option to the execFileSync
invocation running hooks/nf-prompt.js, matching the timeout used by the other
execFileSync calls in this test file. Keep the existing input, encoding, and
environment configuration unchanged.
---
Nitpick comments:
In `@bin/codex-install.cjs`:
- Around line 217-254: Move the constant aliases map out of
normalizeDetectedProvider and define it once at module scope. Keep the existing
codex and copilot alias entries and continue referencing the hoisted map when
assigning extraTools.
In `@bin/codex-install.test.cjs`:
- Around line 97-113: Extend the test suite using the existing tmpdir and
t.after cleanup pattern to cover removeCodexSkills/removeOwnedSkillDirectories
and removeCodexMcp. Verify skill cleanup removes only directories owned by the
nf: marker and preserves unrelated skill directories, and verify MCP cleanup
preserves config.toml when it contains user content while still covering the
removable case.
In `@bin/install.js`:
- Line 2610: Update writeManifest to include Codex SKILL.md files from
codexSkillsDir in the manifest’s drift/modification hashes, using a stable
relative-path prefix so entries remain consistent independently of configDir.
Preserve the existing agent manifest entries and apply this only to the Codex
runtime path.
In `@test/install-virgin.test.cjs`:
- Around line 77-118: Extract the duplicated environment object from runInstall
and runUninstall into a shared testEnv(homeDir) helper. Preserve all existing
variable overrides, optional-install flags, and conditional HOME/USERPROFILE
behavior, then replace both inline env literals with env: testEnv(homeDir) so
the helpers remain synchronized.
- Around line 311-314: Guard every readIfExists result in the install assertions
by introducing and using a mustRead helper that reports the missing file path
before returning its contents. Apply it to planner, hooks.json, configLoader,
promptHook, and config.toml, including the JSON.parse input, while preserving
the existing assertions and parsing behavior.
- Around line 375-396: Add a concise comment immediately above the “re-install
is idempotent across native files and managed TOML” test documenting that this
test and the following uninstall test must run last and in their current order,
since they mutate and then tear down the shared seeded tmpDir/homeDir state.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: eb9b31f3-ca7c-41a9-adfe-42a29fac6a53
⛔ Files ignored due to path filters (1)
hooks/dist/config-loader.jsis excluded by!**/dist/**
📒 Files selected for processing (8)
.gitignoreCHANGELOG.mdREADME.mdbin/codex-install.cjsbin/codex-install.test.cjsbin/install.jspackage.jsontest/install-virgin.test.cjs
Comment on lines
+256
to
+298
| function ensureCodexProviders(providersPath, detectedProviders, selectedSlots = null) { | ||
| let data = { providers: [] }; | ||
| try { | ||
| if (fs.existsSync(providersPath)) { | ||
| data = JSON.parse(fs.readFileSync(providersPath, 'utf8')); | ||
| } | ||
| } catch (_) { | ||
| data = { providers: [] }; | ||
| } | ||
|
|
||
| const existing = (Array.isArray(data.providers) ? data.providers : []) | ||
| .map(normalizeDetectedProvider) | ||
| .filter(Boolean); | ||
| const byName = new Map(existing.map(provider => [provider.name, provider])); | ||
|
|
||
| for (const rawProvider of detectedProviders || []) { | ||
| const provider = normalizeDetectedProvider(rawProvider); | ||
| if (!provider) continue; | ||
| if (selectedSlots | ||
| && !selectedSlots.includes(provider.name) | ||
| && !selectedSlots.includes(rawProvider.name) | ||
| && !selectedSlots.includes(provider.mainTool)) { | ||
| continue; | ||
| } | ||
| if (!byName.has(provider.name)) byName.set(provider.name, provider); | ||
| } | ||
|
|
||
| const isSelected = provider => { | ||
| if (!selectedSlots) return true; | ||
| const unprefixedName = provider.name.replace(/^nforma-/, ''); | ||
| const familyName = unprefixedName.replace(/-\d+$/, ''); | ||
| return selectedSlots.includes(provider.name) | ||
| || selectedSlots.includes(unprefixedName) | ||
| || selectedSlots.includes(provider.mainTool) | ||
| || selectedSlots.includes(familyName); | ||
| }; | ||
| const active = [...byName.values()].filter(provider => | ||
| provider && provider.name && provider.active !== false && isSelected(provider) | ||
| ); | ||
| fs.mkdirSync(path.dirname(providersPath), { recursive: true }); | ||
| fs.writeFileSync(providersPath, JSON.stringify({ ...data, providers: [...byName.values()] }, null, 2) + '\n', 'utf8'); | ||
| return active; | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Unnormalizable provider entries are silently dropped from providers.json.
normalizeDetectedProvider returns null when a family can't be derived or no args_template resolves, and those entries are filtered out before the file is rewritten at Line 296. Since this path targets the installed nf-bin/providers.json — the very file mergeProvidersJson preserves user-added slots in (bin/install.js Lines 2990-2994) — a Codex install permanently deletes hand-edited or preset-created slots that don't normalize.
Preserve the originals you can't normalize instead of discarding them.
🛡️ Proposed fix to preserve unnormalizable entries
- const existing = (Array.isArray(data.providers) ? data.providers : [])
- .map(normalizeDetectedProvider)
- .filter(Boolean);
- const byName = new Map(existing.map(provider => [provider.name, provider]));
+ const rawExisting = Array.isArray(data.providers) ? data.providers : [];
+ const preserved = [];
+ const byName = new Map();
+ for (const raw of rawExisting) {
+ const normalized = normalizeDetectedProvider(raw);
+ if (normalized) byName.set(normalized.name, normalized);
+ else preserved.push(raw);
+ }- fs.writeFileSync(providersPath, JSON.stringify({ ...data, providers: [...byName.values()] }, null, 2) + '\n', 'utf8');
+ fs.writeFileSync(
+ providersPath,
+ JSON.stringify({ ...data, providers: [...byName.values(), ...preserved] }, null, 2) + '\n',
+ 'utf8'
+ );📝 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.
Suggested change
| function ensureCodexProviders(providersPath, detectedProviders, selectedSlots = null) { | |
| let data = { providers: [] }; | |
| try { | |
| if (fs.existsSync(providersPath)) { | |
| data = JSON.parse(fs.readFileSync(providersPath, 'utf8')); | |
| } | |
| } catch (_) { | |
| data = { providers: [] }; | |
| } | |
| const existing = (Array.isArray(data.providers) ? data.providers : []) | |
| .map(normalizeDetectedProvider) | |
| .filter(Boolean); | |
| const byName = new Map(existing.map(provider => [provider.name, provider])); | |
| for (const rawProvider of detectedProviders || []) { | |
| const provider = normalizeDetectedProvider(rawProvider); | |
| if (!provider) continue; | |
| if (selectedSlots | |
| && !selectedSlots.includes(provider.name) | |
| && !selectedSlots.includes(rawProvider.name) | |
| && !selectedSlots.includes(provider.mainTool)) { | |
| continue; | |
| } | |
| if (!byName.has(provider.name)) byName.set(provider.name, provider); | |
| } | |
| const isSelected = provider => { | |
| if (!selectedSlots) return true; | |
| const unprefixedName = provider.name.replace(/^nforma-/, ''); | |
| const familyName = unprefixedName.replace(/-\d+$/, ''); | |
| return selectedSlots.includes(provider.name) | |
| || selectedSlots.includes(unprefixedName) | |
| || selectedSlots.includes(provider.mainTool) | |
| || selectedSlots.includes(familyName); | |
| }; | |
| const active = [...byName.values()].filter(provider => | |
| provider && provider.name && provider.active !== false && isSelected(provider) | |
| ); | |
| fs.mkdirSync(path.dirname(providersPath), { recursive: true }); | |
| fs.writeFileSync(providersPath, JSON.stringify({ ...data, providers: [...byName.values()] }, null, 2) + '\n', 'utf8'); | |
| return active; | |
| } | |
| function ensureCodexProviders(providersPath, detectedProviders, selectedSlots = null) { | |
| let data = { providers: [] }; | |
| try { | |
| if (fs.existsSync(providersPath)) { | |
| data = JSON.parse(fs.readFileSync(providersPath, 'utf8')); | |
| } | |
| } catch (_) { | |
| data = { providers: [] }; | |
| } | |
| const rawExisting = Array.isArray(data.providers) ? data.providers : []; | |
| const preserved = []; | |
| const byName = new Map(); | |
| for (const raw of rawExisting) { | |
| const normalized = normalizeDetectedProvider(raw); | |
| if (normalized) byName.set(normalized.name, normalized); | |
| else preserved.push(raw); | |
| } | |
| for (const rawProvider of detectedProviders || []) { | |
| const provider = normalizeDetectedProvider(rawProvider); | |
| if (!provider) continue; | |
| if (selectedSlots | |
| && !selectedSlots.includes(provider.name) | |
| && !selectedSlots.includes(rawProvider.name) | |
| && !selectedSlots.includes(provider.mainTool)) { | |
| continue; | |
| } | |
| if (!byName.has(provider.name)) byName.set(provider.name, provider); | |
| } | |
| const isSelected = provider => { | |
| if (!selectedSlots) return true; | |
| const unprefixedName = provider.name.replace(/^nforma-/, ''); | |
| const familyName = unprefixedName.replace(/-\d+$/, ''); | |
| return selectedSlots.includes(provider.name) | |
| || selectedSlots.includes(unprefixedName) | |
| || selectedSlots.includes(provider.mainTool) | |
| || selectedSlots.includes(familyName); | |
| }; | |
| const active = [...byName.values()].filter(provider => | |
| provider && provider.name && provider.active !== false && isSelected(provider) | |
| ); | |
| fs.mkdirSync(path.dirname(providersPath), { recursive: true }); | |
| fs.writeFileSync( | |
| providersPath, | |
| JSON.stringify({ ...data, providers: [...byName.values(), ...preserved] }, null, 2) + '\n', | |
| 'utf8' | |
| ); | |
| return active; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@bin/codex-install.cjs` around lines 256 - 298, Update ensureCodexProviders so
entries for which normalizeDetectedProvider returns null are retained in the
providers array using their original objects instead of being filtered out.
Preserve these unnormalizable entries when rewriting providersPath, while
continuing to use normalized entries for deduplication, selection, and the
active return value.
Comment on lines
+326
to
+332
| function replaceManagedMcpBlock(content, block) { | ||
| const escapedBegin = MCP_BLOCK_BEGIN.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); | ||
| const escapedEnd = MCP_BLOCK_END.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); | ||
| const managedPattern = new RegExp(`(?:^|\\n)${escapedBegin}[\\s\\S]*?${escapedEnd}(?:\\n|$)`); | ||
| const withoutManaged = content.replace(managedPattern, '\n').replace(/\s+$/, ''); | ||
| return [withoutManaged, block].filter(Boolean).join(withoutManaged ? '\n\n' : '') + '\n'; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use a global regex so stray duplicate managed blocks are all replaced.
Without g, only the first block is removed; any second block (hand-copied config, interrupted earlier write) survives and keeps stale mcp_servers entries registered. removeCodexMcp has the same blind spot.
🔧 Proposed fix
- const managedPattern = new RegExp(`(?:^|\\n)${escapedBegin}[\\s\\S]*?${escapedEnd}(?:\\n|$)`);
+ const managedPattern = new RegExp(`(?:^|\\n)${escapedBegin}[\\s\\S]*?${escapedEnd}(?:\\n|$)`, 'g');📝 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.
Suggested change
| function replaceManagedMcpBlock(content, block) { | |
| const escapedBegin = MCP_BLOCK_BEGIN.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); | |
| const escapedEnd = MCP_BLOCK_END.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); | |
| const managedPattern = new RegExp(`(?:^|\\n)${escapedBegin}[\\s\\S]*?${escapedEnd}(?:\\n|$)`); | |
| const withoutManaged = content.replace(managedPattern, '\n').replace(/\s+$/, ''); | |
| return [withoutManaged, block].filter(Boolean).join(withoutManaged ? '\n\n' : '') + '\n'; | |
| } | |
| function replaceManagedMcpBlock(content, block) { | |
| const escapedBegin = MCP_BLOCK_BEGIN.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); | |
| const escapedEnd = MCP_BLOCK_END.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); | |
| const managedPattern = new RegExp(`(?:^|\\n)${escapedBegin}[\\s\\S]*?${escapedEnd}(?:\\n|$)`, 'g'); | |
| const withoutManaged = content.replace(managedPattern, '\n').replace(/\s+$/, ''); | |
| return [withoutManaged, block].filter(Boolean).join(withoutManaged ? '\n\n' : '') + '\n'; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@bin/codex-install.cjs` around lines 326 - 332, Update replaceManagedMcpBlock
to construct its managedPattern with the global flag so every managed MCP block
is removed before inserting the replacement block. Apply the same global-regex
behavior to the pattern used by removeCodexMcp, preserving existing matching and
cleanup behavior.
Comment on lines
+334
to
+343
| function configureCodexMcp(configPath, providers, targetDir, providersPath) { | ||
| let existing = ''; | ||
| try { | ||
| existing = fs.readFileSync(configPath, 'utf8'); | ||
| } catch (_) { | ||
| // A missing config is a normal first-install case. | ||
| } | ||
| const block = renderCodexMcpBlock(providers, targetDir, providersPath); | ||
| fs.mkdirSync(path.dirname(configPath), { recursive: true }); | ||
| fs.writeFileSync(configPath, replaceManagedMcpBlock(existing, block), 'utf8'); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Only treat ENOENT as "no existing config" — other read errors clobber config.toml.
The catch swallows every failure (EACCES, EISDIR, EIO). On a transient/permission read error, existing stays '' and Line 343 rewrites the user's config.toml with just the managed block, destroying unrelated Codex configuration.
🛡️ Proposed fix
let existing = '';
try {
existing = fs.readFileSync(configPath, 'utf8');
- } catch (_) {
- // A missing config is a normal first-install case.
+ } catch (e) {
+ // A missing config is a normal first-install case; anything else is fatal
+ // because we would otherwise overwrite a config we could not read.
+ if (e.code !== 'ENOENT') throw e;
}📝 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.
Suggested change
| function configureCodexMcp(configPath, providers, targetDir, providersPath) { | |
| let existing = ''; | |
| try { | |
| existing = fs.readFileSync(configPath, 'utf8'); | |
| } catch (_) { | |
| // A missing config is a normal first-install case. | |
| } | |
| const block = renderCodexMcpBlock(providers, targetDir, providersPath); | |
| fs.mkdirSync(path.dirname(configPath), { recursive: true }); | |
| fs.writeFileSync(configPath, replaceManagedMcpBlock(existing, block), 'utf8'); | |
| function configureCodexMcp(configPath, providers, targetDir, providersPath) { | |
| let existing = ''; | |
| try { | |
| existing = fs.readFileSync(configPath, 'utf8'); | |
| } catch (e) { | |
| // A missing config is a normal first-install case; anything else is fatal | |
| // because we would otherwise overwrite a config we could not read. | |
| if (e.code !== 'ENOENT') throw e; | |
| } | |
| const block = renderCodexMcpBlock(providers, targetDir, providersPath); | |
| fs.mkdirSync(path.dirname(configPath), { recursive: true }); | |
| fs.writeFileSync(configPath, replaceManagedMcpBlock(existing, block), 'utf8'); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@bin/codex-install.cjs` around lines 334 - 343, Update configureCodexMcp to
ignore a read failure only when fs.readFileSync throws an ENOENT error,
preserving the missing-config first-install behavior. For any other error,
propagate it and do not call fs.writeFileSync, preventing existing config.toml
contents from being overwritten.
Comment on lines
2492
to
+2497
| // 1. Verify Commands (Expected: 60) | ||
| const EXPECTED_COMMANDS = 60; | ||
| let commandCount = 0; | ||
| if (isOpencode) { | ||
| if (isCodex) { | ||
| commandCount = countFiles(codexSkillsDir || path.join(targetDir, 'skills'), f => f === 'SKILL.md'); | ||
| } else if (isOpencode) { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Codex validation scans the whole shared skills directory, including third-party skills.
codexSkillsDir is ~/.agents/skills — a shared discovery location, not an nForma-owned tree. Two consequences:
- Line 2496: unrelated user skills count toward
EXPECTED_COMMANDS, so a partial nForma install can still pass. - Lines 2542-2546: a third-party
SKILL.mdthat merely mentions/nf:pushes a "Codex transformation failed" error, andinstall()then exits with status 1 (Line 3317) on something nForma doesn't own.
Restrict both to directories nForma wrote (e.g. the nf-* directory names produced by installCodexSkills, or frontmatter name starting with nf:).
Also applies to: 2540-2548
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@bin/install.js` around lines 2492 - 2497, Restrict Codex validation to
nForma-owned skills in the `isCodex` command-count logic and the transformation
scan around the Codex validation block. Filter entries using the `nf-*`
directories produced by `installCodexSkills` or frontmatter names beginning with
`nf:`, so unrelated skills neither contribute to `EXPECTED_COMMANDS` nor trigger
transformation failures.
Comment on lines
+2717
to
+2721
| const codexSkillsDir = isCodex | ||
| ? (isGlobal | ||
| ? path.join(os.homedir(), '.agents', 'skills') | ||
| : path.join(process.cwd(), '.agents', 'skills')) | ||
| : null; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# How do the isolated install helpers/tests scope Codex HOME/config?
rg -n --iglob '*install-virgin*' -C3 'CODEX_HOME|\.agents|codexSkillsDir'
rg -n -C3 'CODEX_HOME' --glob '!node_modules' .Repository: nForma-AI/nForma
Length of output: 204
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== repo files matching install/test/virgin/config names =="
git ls-files | rg '(^|/)(install\.js|.*install.*test.*|.*test.*install.*)$|config|codex' || true
echo
echo "== bin/install.js outline around relevant symbols =="
if [ -f bin/install.js ]; then
wc -l bin/install.js
rg -n -C 4 'codexSkillsDir|explicitConfigDir|getGlobalDir|targetDir|CODEX_HOME|\.agents' bin/install.js
fi
echo
echo "== likely test/helper files =="
for f in $(git ls-files | rg 'install|virgin|test' | head -80); do
echo "--- $f"
wc -l "$f"
rg -n -C 4 'codexSkillsDir|CODEX_HOME|\.agents|explicitConfigDir|targetDir|getGlobalDir' "$f" || true
doneRepository: nForma-AI/nForma
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files mentioning CODEX_HOME / .agents / codexSkillsDir outside planning/archives =="
rg -n -C 3 'CODEX_HOME|\.agents|codexSkillsDir|--config-dir|CONFIG_DIR' --glob '!node_modules' --glob '!test/**' --glob '!tests/**' --glob '!docs/**' .
echo
echo "== bin/install.js install/uninstall relevant blocks =="
sed -n '1008,1020p;1760,1835p;2449,2460p;2704,2725p;2869,2882p;4640,4665p' bin/install.js | csplit -f /tmp/cp_ -z -n 3 >/tmp/cp_cuts || true
for f in /tmp/cp_*; do
echo "--- $f"
sed -n '1,220p' "$f"
done
echo
echo "== Codex tests mentioning skills/config/home =="
rg -n -C 4 'codex|skills|CODEX|agents|config-dir|~|home' bin/codex-install.test.cjs test tests test/golden tests/bin install-virgin.test clobber || trueRepository: nForma-AI/nForma
Length of output: 50373
🏁 Script executed:
#!/bin/bash
set -euo pipefail
tmp="$(mktemp -d)"
node - <<'JS' > "$tmp/resolve.txt"
const path = require('node:path');
function homeLikeRuntimeConfigDir(runtime, explicitDir, env) {
if (runtime === 'opencode') {
if (explicitDir) return path.join('/sandbox', explicitDir.replace(/^~\//, ''));
if (env.OPENCODE_CONFIG_DIR) return path.join('/sandbox', env.OPENCODE_CONFIG_DIR.replace(/^~\//, ''));
if (env.OPENCODE_CONFIG) return path.join('/sandbox', env.OPENCODE_CONFIG.replace(/^~\//, ''));
return path.join('/sandbox', '.opencode');
}
if (runtime === 'codex') {
if (explicitDir) return path.join('/sandbox', explicitDir.replace(/^~\//, ''));
if (env.CODEX_HOME) return path.join('/sandbox', env.CODEX_HOME.replace(/^~\//, ''));
if (env.CODEX_CONFIG_DIR) return path.join('/sandbox', env.CODEX_CONFIG_DIR.replace(/^~\//, ''));
return path.join('/sandbox', '.codex');
}
// placeholder
return path.join('/sandbox', '.' + runtime);
}
for (const label of [
'no-override',
'explicit-config-dir',
'codex-home',
'codex-config-dir'
]) {
let env;
let explicitDir = null;
if (label === 'explicit-config-dir') {
explicitDir = '/tmp/nf-codex-sandbox';
env = {};
} else if (label === 'codex-home') {
env = { CODEX_HOME: '/tmp/nf-codex-home-sandbox' };
} else if (label === 'codex-config-dir') {
env = { CODEX_CONFIG_DIR: '/tmp/nf-codex-configdir-sandbox' };
} else {
env = {};
}
const targetDir = homeLikeRuntimeConfigDir('codex', explicitDir, env);
const codexSkillsDir = path.join(process.env.HOME || '/home/user', '.agents', 'skills');
console.log(JSON.stringify({ label, explicitDir, env, targetDir, codexSkillsDir }));
}
JS
cat "$tmp/resolve.txt"
echo
echo "== path behavior summary =="
node - <<'JS'
const path = require('node:path');
console.log(path.join('/tmp/sandbox/config', '.agents', 'skills'));
console.log(process.env.HOME);
JSRepository: nForma-AI/nForma
Length of output: 938
Derive Codex skill directory from the resolved Codex config location.
codexSkillsDir ignores --config-dir, CODEX_HOME, and CODEX_CONFIG_DIR, unlike targetDir, so global Codex installs/uninstalls write and clean up ~/.agents/skills even when the config is scoped to a temp path. Base it on Codex’s config resolution, or at minimum use ~/.codex/.agents/skills.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@bin/install.js` around lines 2717 - 2721, Update the codexSkillsDir
calculation alongside targetDir so global Codex skill installs and removals
derive from the resolved Codex config directory, honoring --config-dir,
CODEX_HOME, and CODEX_CONFIG_DIR; retain the project-local .agents/skills path
for non-global installs and use ~/.codex/.agents/skills as the minimum fallback.
Comment on lines
3204
to
3224
| const uvCheck = _spawnRiver('which', ['uv'], { timeout: 3000 }); | ||
| if (uvCheck.status !== 0) { | ||
| log(` ${cyan}↓${reset} Installing uv...`); | ||
| const uvInstall = _spawnRiver('curl', ['-sSL', 'https://astral.sh/uv/install.sh'], { timeout: 30000 }); | ||
| if (uvInstall.status !== 0) { | ||
| log(` ${yellow}⚠${reset} uv install failed — skipping River ML`); | ||
| return; | ||
| } | ||
| // Reload PATH so uv is found immediately after install | ||
| const newPath = [...new Set([path.join(os.homedir(), '.local', 'bin'), ...process.env.PATH.split(':')])].join(':'); | ||
| const uvPathCheck = _spawnRiver('which', ['uv'], { timeout: 3000, env: { ...process.env, PATH: newPath } }); | ||
| if (uvPathCheck.status !== 0) { | ||
| log(` ${yellow}⚠${reset} uv not in PATH after install — skipping River ML`); | ||
| return; | ||
| uvAvailable = false; | ||
| } else { | ||
| // Reload PATH so uv is found immediately after install. | ||
| const currentPath = process.env.PATH || ''; | ||
| const newPath = [...new Set([ | ||
| path.join(os.homedir(), '.local', 'bin'), | ||
| ...currentPath.split(path.delimiter).filter(Boolean), | ||
| ])].join(path.delimiter); | ||
| const uvPathCheck = _spawnRiver('which', ['uv'], { timeout: 3000, env: { ...process.env, PATH: newPath } }); | ||
| if (uvPathCheck.status !== 0) { | ||
| log(` ${yellow}⚠${reset} uv not in PATH after install — skipping River ML`); | ||
| uvAvailable = false; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
The "Installing uv" step never installs uv.
Line 3207 downloads install.sh to stdout and discards it — nothing is executed — yet the log says Installing uv.... The subsequent PATH check then fails and River ML is skipped, so the branch only burns a 30s-timeout network call and prints a misleading message. Also, which (Lines 3204, 3218) doesn't exist on Windows, so this whole path always reports uv as missing there.
Either pipe the installer to a shell deliberately (with the security implications spelled out), or drop the install attempt and just report that uv is required.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@bin/install.js` around lines 3204 - 3224, Fix the uv availability flow around
_spawnRiver and the uvAvailable branch: do not invoke curl in a way that only
downloads the installer without executing it, and avoid relying on the Unix-only
which command on Windows. Either deliberately execute the installer through an
appropriate shell while preserving the existing failure handling, or remove the
installation attempt and report that uv is required; ensure the resulting PATH
check and River ML skip behavior remain accurate across platforms.
Comment on lines
+340
to
+354
| const output = execFileSync(process.execPath, [path.join(tmpDir, 'hooks', 'nf-prompt.js')], { | ||
| input: JSON.stringify({ | ||
| hook_event_name: 'UserPromptSubmit', | ||
| prompt: '$nf:plan-phase 1', | ||
| cwd: tmpRoot, | ||
| session_id: 'codex-hook-test', | ||
| }), | ||
| encoding: 'utf8', | ||
| env: { | ||
| ...process.env, | ||
| HOME: homeDir, | ||
| USERPROFILE: homeDir, | ||
| NF_SKIP_PREFLIGHT: '1', | ||
| }, | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Add a timeout to this spawn.
Every other execFileSync in this file caps at 120s; this one has none, so a hook that blocks on stdin or a stalled preflight hangs the CI job until the overall budget expires instead of failing fast.
🔧 Proposed fix
encoding: 'utf8',
+ timeout: 120000,
env: {📝 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.
Suggested change
| const output = execFileSync(process.execPath, [path.join(tmpDir, 'hooks', 'nf-prompt.js')], { | |
| input: JSON.stringify({ | |
| hook_event_name: 'UserPromptSubmit', | |
| prompt: '$nf:plan-phase 1', | |
| cwd: tmpRoot, | |
| session_id: 'codex-hook-test', | |
| }), | |
| encoding: 'utf8', | |
| env: { | |
| ...process.env, | |
| HOME: homeDir, | |
| USERPROFILE: homeDir, | |
| NF_SKIP_PREFLIGHT: '1', | |
| }, | |
| }); | |
| const output = execFileSync(process.execPath, [path.join(tmpDir, 'hooks', 'nf-prompt.js')], { | |
| input: JSON.stringify({ | |
| hook_event_name: 'UserPromptSubmit', | |
| prompt: '$nf:plan-phase 1', | |
| cwd: tmpRoot, | |
| session_id: 'codex-hook-test', | |
| }), | |
| encoding: 'utf8', | |
| timeout: 120000, | |
| env: { | |
| ...process.env, | |
| HOME: homeDir, | |
| USERPROFILE: homeDir, | |
| NF_SKIP_PREFLIGHT: '1', | |
| }, | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/install-virgin.test.cjs` around lines 340 - 354, Add a 120-second
timeout option to the execFileSync invocation running hooks/nf-prompt.js,
matching the timeout used by the other execFileSync calls in this test file.
Keep the existing input, encoding, and environment configuration unchanged.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Adds native OpenAI Codex as an nForma install runtime, including
$nf:*skills, TOML custom agents, Codex hooks, managed MCP configuration, migration/uninstall handling, documentation, and regression coverage.Why
Codex users should be able to install and run the full nForma workflow natively instead of relying on Claude-style slash-command layouts that Codex does not discover.
Testing
Windows verification:
node bin/install.js --codex --global— installed v0.44.1 successfullynode --test bin/codex-install.test.cjs— 6/6 passtest/install-virgin.test.cjs— install, hooks, quorum context, MCP config, reinstall, and uninstall passnode scripts/lint-isolation.js— passnode scripts/verify-hooks-sync.cjs— passnode bin/lint-changelog-sections.cjs— passgit diff --check— passLocal Windows harness limitations: the broader cross-runtime install test still hits the existing OpenCode
--config-dirassertion and Windows cannot resolve barenpmthroughspawnSync(npm.cmdis required). The Codex suite itself passes, and the live Codex install was verified.Checklist
code-review-and-quality)Breaking Changes
None
Summary by CodeRabbit
New Features
$nf:*skills, native agents, hooks, and MCP server configuration.$nf:helpcommands and skill discovery through/skills.Bug Fixes
Documentation