Fix Codex Desktop CLI discovery#24
Conversation
📝 WalkthroughWalkthroughAdds shared Codex CLI discovery with explicit, environment, PATH, and macOS Codex Desktop fallbacks. Run execution and doctor diagnostics use the resolver, with smoke tests, documentation, help text, and release metadata updated for version 0.3.20. ChangesCodex executable discovery
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant RunLoop as run-loop.mjs
participant Resolver as resolveCodexCli
participant Codex as Codex CLI
User->>RunLoop: run --execute codex
RunLoop->>Resolver: resolve requested and environment options
Resolver-->>RunLoop: Codex binary and source
RunLoop->>Codex: execute proof with resolved binary
Codex-->>RunLoop: proof result
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 4
🧹 Nitpick comments (2)
skills/loop-it/scripts/run-loop.mjs (1)
1229-1229: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHelp text wording inconsistent across CLI entry points.
This line reads "Codex executable override; otherwise discover PATH or Codex Desktop", while the equivalent
--codex-bindescription indoctor.mjs(line 553) andbin/loop-it.mjs(line 180) both read "Override PATH and Codex Desktop executable discovery". Aligning the phrasing across all three help outputs improves consistency for users running--helpon different subcommands.✏️ Suggested alignment
- --codex-bin <path> Codex executable override; otherwise discover PATH or Codex Desktop + --codex-bin <path> Override PATH and Codex Desktop executable discovery🤖 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 `@skills/loop-it/scripts/run-loop.mjs` at line 1229, Update the --codex-bin help text in run-loop.mjs to use the same “Override PATH and Codex Desktop executable discovery” wording already used by doctor.mjs and bin/loop-it.mjs.tests/smoke-install.mjs (1)
1216-1231: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
envoverride drops the rest of the process environment.Passing
env: { HOME: desktopHome, PATH: [...] }torun()replaces the child's entire environment (spawnSync does not merge withprocess.envwhenenvis supplied), unlike the equivalent block added intests/run-proof-smoke.mjs(line ~231) which spreads...process.envfirst. This makes the child process here more fragile to missing ambient vars than its sibling test, for no apparent isolation benefit beyond HOME/PATH.🔧 Suggested fix for consistency with run-proof-smoke.mjs
], { env: { + ...process.env, HOME: desktopHome, PATH: [dirname(process.execPath), "/opt/homebrew/bin", "/usr/local/bin", "/usr/bin", "/bin"].join(delimiter), }, }).stdout);🤖 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 `@tests/smoke-install.mjs` around lines 1216 - 1231, Update the environment override passed to run() in the desktopReady smoke-test setup to preserve the parent environment by spreading process.env before applying the custom HOME and PATH values. Keep the existing HOME and PATH overrides unchanged and match the established pattern used by the sibling smoke test.
🤖 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 `@skills/loop-it/scripts/lib/codex-cli.mjs`:
- Line 1: Update executable-candidate validation in the codex CLI resolution
flow to require both execute access and a non-directory filesystem entry. Apply
this check consistently to PATH candidates and desktop candidates before
selecting them, so directory entries are skipped and resolution continues to the
next candidate.
- Around line 43-55: Update desktopCandidates so CODEX_DESKTOP_RELATIVE_PATHS
are resolved and included only when process.platform is "darwin"; keep the
absolute macOS application paths and deduplication behavior unchanged, and
ensure non-macOS platforms return no desktop candidates.
- Around line 12-18: Update the source selection in the resolver around
requested so it reflects the normalized override used to choose the binary, not
the raw options.requested truthiness. Label the result as "argument" only when
the normalized argument value is selected; otherwise preserve "environment" for
the environment-derived binary.
In `@tests/run-proof-smoke.mjs`:
- Around line 375-397: Update createDesktopCodexFallback so the generated
desktopCodex stub uses CommonJS require() instead of top-level import
statements, preserving its existing file, path, test-replacement, and
output-writing behavior.
---
Nitpick comments:
In `@skills/loop-it/scripts/run-loop.mjs`:
- Line 1229: Update the --codex-bin help text in run-loop.mjs to use the same
“Override PATH and Codex Desktop executable discovery” wording already used by
doctor.mjs and bin/loop-it.mjs.
In `@tests/smoke-install.mjs`:
- Around line 1216-1231: Update the environment override passed to run() in the
desktopReady smoke-test setup to preserve the parent environment by spreading
process.env before applying the custom HOME and PATH values. Keep the existing
HOME and PATH overrides unchanged and match the established pattern used by the
sibling smoke test.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c004288b-8278-46e1-a107-36f6a055c62c
📒 Files selected for processing (14)
.claude-plugin/plugin.json.codex-plugin/plugin.json.cursor-plugin/plugin.jsonCHANGELOG.mdREADME.mdbin/loop-it.mjsdocs/examples.mddocs/install.mdpackage.jsonskills/loop-it/scripts/doctor.mjsskills/loop-it/scripts/lib/codex-cli.mjsskills/loop-it/scripts/run-loop.mjstests/run-proof-smoke.mjstests/smoke-install.mjs
| @@ -0,0 +1,82 @@ | |||
| import { accessSync, constants } from "node:fs"; | |||
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Reject directories as executable candidates.
accessSync(path, constants.X_OK) can succeed for a traversable directory on POSIX. A directory named codex in PATH or a desktop candidate can therefore be selected, causing downstream spawnSync execution to fail instead of trying another candidate.
Proposed fix
-import { accessSync, constants } from "node:fs";
+import { accessSync, constants, statSync } from "node:fs";
function isExecutable(path) {
try {
+ if (!statSync(path).isFile()) {
+ return false;
+ }
accessSync(path, constants.X_OK);
return true;
} catch {
return false;
}
}Also applies to: 71-77
🤖 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 `@skills/loop-it/scripts/lib/codex-cli.mjs` at line 1, Update
executable-candidate validation in the codex CLI resolution flow to require both
execute access and a non-directory filesystem entry. Apply this check
consistently to PATH candidates and desktop candidates before selecting them, so
directory entries are skipped and resolution continues to the next candidate.
| const requested = nonEmpty(options.requested) || nonEmpty(env.LOOP_IT_CODEX_BIN) || "codex"; | ||
|
|
||
| if (requested !== "codex") { | ||
| return { | ||
| bin: requested, | ||
| source: options.requested ? "argument" : "environment", | ||
| }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Base the source on the normalized override.
A truthy whitespace-only options.requested causes the environment binary to be selected while the result is incorrectly labeled "argument". This makes doctor’s reported source inconsistent with the actual resolver decision.
Proposed fix
- const requested = nonEmpty(options.requested) || nonEmpty(env.LOOP_IT_CODEX_BIN) || "codex";
+ const requestedArgument = nonEmpty(options.requested);
+ const requestedEnvironment = nonEmpty(env.LOOP_IT_CODEX_BIN);
+ const requested = requestedArgument || requestedEnvironment || "codex";
...
- source: options.requested ? "argument" : "environment",
+ source: requestedArgument ? "argument" : "environment",📝 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.
| const requested = nonEmpty(options.requested) || nonEmpty(env.LOOP_IT_CODEX_BIN) || "codex"; | |
| if (requested !== "codex") { | |
| return { | |
| bin: requested, | |
| source: options.requested ? "argument" : "environment", | |
| }; | |
| const requestedArgument = nonEmpty(options.requested); | |
| const requestedEnvironment = nonEmpty(env.LOOP_IT_CODEX_BIN); | |
| const requested = requestedArgument || requestedEnvironment || "codex"; | |
| if (requested !== "codex") { | |
| return { | |
| bin: requested, | |
| source: requestedArgument ? "argument" : "environment", | |
| }; |
🤖 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 `@skills/loop-it/scripts/lib/codex-cli.mjs` around lines 12 - 18, Update the
source selection in the resolver around requested so it reflects the normalized
override used to choose the binary, not the raw options.requested truthiness.
Label the result as "argument" only when the normalized argument value is
selected; otherwise preserve "environment" for the environment-derived binary.
| function desktopCandidates(env) { | ||
| const home = nonEmpty(env.HOME) || homedir(); | ||
| const candidates = CODEX_DESKTOP_RELATIVE_PATHS.map((parts) => resolve(home, ...parts)); | ||
|
|
||
| if (process.platform === "darwin") { | ||
| candidates.push( | ||
| "/Applications/ChatGPT.app/Contents/Resources/codex", | ||
| "/Applications/Codex.app/Contents/Resources/codex" | ||
| ); | ||
| } | ||
|
|
||
| return [...new Set(candidates)]; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Only consider desktop candidates on macOS.
The home-relative candidates are built before the Darwin check, so Linux and Windows also treat ~/Applications/ChatGPT.app/.../codex and ~/Applications/Codex.app/.../codex as desktop binaries. If such a path exists, downstream execution will label it "desktop" and invoke it.
Proposed fix
function desktopCandidates(env) {
+ if (process.platform !== "darwin") {
+ return [];
+ }
+
const home = nonEmpty(env.HOME) || homedir();
const candidates = CODEX_DESKTOP_RELATIVE_PATHS.map((parts) => resolve(home, ...parts));
- if (process.platform === "darwin") {
- candidates.push(
- "/Applications/ChatGPT.app/Contents/Resources/codex",
- "/Applications/Codex.app/Contents/Resources/codex"
- );
- }
+ candidates.push(
+ "/Applications/ChatGPT.app/Contents/Resources/codex",
+ "/Applications/Codex.app/Contents/Resources/codex"
+ );
return [...new Set(candidates)];
}📝 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.
| function desktopCandidates(env) { | |
| const home = nonEmpty(env.HOME) || homedir(); | |
| const candidates = CODEX_DESKTOP_RELATIVE_PATHS.map((parts) => resolve(home, ...parts)); | |
| if (process.platform === "darwin") { | |
| candidates.push( | |
| "/Applications/ChatGPT.app/Contents/Resources/codex", | |
| "/Applications/Codex.app/Contents/Resources/codex" | |
| ); | |
| } | |
| return [...new Set(candidates)]; | |
| } | |
| function desktopCandidates(env) { | |
| if (process.platform !== "darwin") { | |
| return []; | |
| } | |
| const home = nonEmpty(env.HOME) || homedir(); | |
| const candidates = CODEX_DESKTOP_RELATIVE_PATHS.map((parts) => resolve(home, ...parts)); | |
| candidates.push( | |
| "/Applications/ChatGPT.app/Contents/Resources/codex", | |
| "/Applications/Codex.app/Contents/Resources/codex" | |
| ); | |
| return [...new Set(candidates)]; | |
| } |
🤖 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 `@skills/loop-it/scripts/lib/codex-cli.mjs` around lines 43 - 55, Update
desktopCandidates so CODEX_DESKTOP_RELATIVE_PATHS are resolved and included only
when process.platform is "darwin"; keep the absolute macOS application paths and
deduplication behavior unchanged, and ensure non-macOS platforms return no
desktop candidates.
| function createDesktopCodexFallback() { | ||
| mkdirSync(dirname(desktopCodex), { recursive: true }); | ||
| writeFileSync( | ||
| desktopCodex, | ||
| [ | ||
| "#!/usr/bin/env node", | ||
| "import { readFileSync, writeFileSync } from 'node:fs';", | ||
| "import { resolve } from 'node:path';", | ||
| "", | ||
| "const testPath = resolve(process.cwd(), 'test.mjs');", | ||
| "const before = readFileSync(testPath, 'utf8');", | ||
| "writeFileSync(testPath, before.replace('assert.equal(total, 3);', 'assert.equal(total, 2);'));", | ||
| "const outputIndex = process.argv.indexOf('--output-last-message');", | ||
| "if (outputIndex !== -1 && process.argv[outputIndex + 1]) {", | ||
| " writeFileSync(resolve(process.cwd(), process.argv[outputIndex + 1]), 'Codex Desktop fallback fixed the test.\\n');", | ||
| "}", | ||
| "", | ||
| ].join("\n"), | ||
| "utf8" | ||
| ); | ||
| chmodSync(desktopCodex, 0o755); | ||
| } | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Does Node.js enable module syntax detection (--experimental-detect-module) by default on Node 18.x?
💡 Result:
No, Node.js does not enable module syntax detection (--experimental-detect-module) by default on Node.js 18.x [1][2]. The --experimental-detect-module feature was introduced in later versions of Node.js (specifically added in v21.1.0 and v20.10.0) [3][4]. Because it was not present in the 18.x release line, it cannot be enabled or disabled by default in those versions [1][2]. Even in versions where the flag exists, it was originally disabled by default and only later enabled by default in much newer releases [5][6].
Citations:
- 1: https://nodejs.org/download/release/v18.20.8/docs/api/esm.html
- 2: https://nodejs.org/download/release/v18.18.0/docs/api/cli.html
- 3: https://nodejs.org/download/release/v22.2.0/docs/api/cli.html
- 4: https://nodejs.org/download/release/v21.7.3/docs/api/cli.html
- 5: module: unflag detect-module nodejs/node#53619
- 6: https://mirrors.aliyun.com/nodejs-release/v22.7.0/docs/api/cli.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== package.json / node engines ==\n'
git ls-files 'package.json' '**/package.json' | xargs -r -I{} sh -c "echo '--- {}'; sed -n '1,220p' '{}'" | sed -n '1,260p'
printf '\n== relevant test file slice ==\n'
sed -n '360,420p' tests/run-proof-smoke.mjs
printf '\n== nearby package markers for tests ==\n'
find . -path '*/package.json' -o -name '.node-version' -o -name '.nvmrc' -o -name '.tool-versions' | sed 's#^\./##' | sortRepository: fhajjej-ship-it/loop-it
Length of output: 5180
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check whether the generated desktop CLI path is under a package scope with type=module
python3 - <<'PY'
from pathlib import Path
p = Path('tests/run-proof-smoke.mjs').resolve()
for parent in [p.parent, *p.parents]:
pkg = parent / 'package.json'
if pkg.exists():
print(f'nearest package.json: {pkg}')
print(pkg.read_text())
break
else:
print('nearest package.json: none')
PYRepository: fhajjej-ship-it/loop-it
Length of output: 3103
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate desktopCodex and the temp project layout around the smoke test
rg -n "desktopCodex|createDesktopCodexFallback|mkdtemp|tmpdir|package.json" tests/run-proof-smoke.mjs
printf '\n== top of file ==\n'
sed -n '1,120p' tests/run-proof-smoke.mjsRepository: fhajjej-ship-it/loop-it
Length of output: 5462
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the helper that creates the sandboxed project to see whether it carries package.json/type=module.
sed -n '120,260p' tests/run-proof-smoke.mjsRepository: fhajjej-ship-it/loop-it
Length of output: 5212
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "desktopCodex|createDesktopCodexFallback|mkdtemp|tmpdir|package.json" tests/run-proof-smoke.mjs
printf '\n== top of file ==\n'
sed -n '1,120p' tests/run-proof-smoke.mjs
printf '\n== middle of file ==\n'
sed -n '120,260p' tests/run-proof-smoke.mjsRepository: fhajjej-ship-it/loop-it
Length of output: 10535
Use CommonJS for the desktop fallback stub
tests/run-proof-smoke.mjs:375-397 writes desktopCodex under a temp HOME path, outside any package.json, so the top-level import depends on Node’s module-syntax detection and will fail on Node 18. Switch this fixture to require() or make it an explicit .mjs file.
🤖 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 `@tests/run-proof-smoke.mjs` around lines 375 - 397, Update
createDesktopCodexFallback so the generated desktopCodex stub uses CommonJS
require() instead of top-level import statements, preserving its existing file,
path, test-replacement, and output-writing behavior.
1 participant
Summary
codexis absent from a normal terminalPATHloop-it runandloop-it doctor, while preserving explicit CLI and environment overrides0.3.20package/plugin metadataVerification
npm run check— passednpm run smoke:public-install— passed against the current public baselinenpm publish --dry-run --access public— passed, including the prepublish checkPATHlookup resolved/Applications/ChatGPT.app/Contents/Resources/codexNotes
PATHinherited by a normal terminal, while Loop It0.3.19invoked barecodex.@latestpackage remains0.3.19until thev0.3.20GitHub release publishes this change.Summary by CodeRabbit
New Features
codexis not onPATH.--codex-binandLOOP_IT_CODEX_BINoptions for explicit executable overrides.doctorreporting to show Codex availability and discovery sources.Documentation
Tests