Repository navigation
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
github-actions
Bot
added
the
vouch:unvouched
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| const parsedVersion = parseCodexCliVersion(`${result.stdout}\n${result.stderr}`); | ||
| if (parsedVersion && !isCodexCliVersionSupported(parsedVersion)) { | ||
| throw new Error(formatCodexCliUpgradeMessage(parsedVersion)); | ||
| } |
There was a problem hiding this comment.
🟢 Low src/codexCliProbe.ts:198
When parseCodexCliVersion returns null (e.g., malformed output or corrupted binary), the condition parsedVersion && !isCodexCliVersionSupported(parsedVersion) evaluates to falsy, so the function returns successfully without throwing. An unrecognized CLI version is silently treated as supported, risking downstream crashes or data corruption when the incompatible tool is invoked. Consider throwing an error when parsedVersion is null.
const parsedVersion = parseCodexCliVersion(`${result.stdout}\n${result.stderr}`);
- if (parsedVersion && !isCodexCliVersionSupported(parsedVersion)) {
+ if (!parsedVersion) {
+ throw new Error(`Failed to parse Codex CLI version from output.`);
+ }
+ if (!isCodexCliVersionSupported(parsedVersion)) {
throw new Error(formatCodexCliUpgradeMessage(parsedVersion));
}🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file apps/server/src/codexCliProbe.ts around lines 198-201:
When `parseCodexCliVersion` returns `null` (e.g., malformed output or corrupted binary), the condition `parsedVersion && !isCodexCliVersionSupported(parsedVersion)` evaluates to falsy, so the function returns successfully without throwing. An unrecognized CLI version is silently treated as supported, risking downstream crashes or data corruption when the incompatible tool is invoked. Consider throwing an error when `parsedVersion` is null.
Evidence trail:
apps/server/src/codexCliProbe.ts lines 198-201 (REVIEWED_COMMIT) - shows the condition `parsedVersion && !isCodexCliVersionSupported(parsedVersion)` which short-circuits when parsedVersion is null. apps/server/src/provider/codexCliVersion.ts lines 109-119 (REVIEWED_COMMIT) - shows `parseCodexCliVersion` returns `null` when regex match fails or parseSemver fails.
There was a problem hiding this comment.
Fixed in c6b8f24. The probe now throws when version parsing fails instead of treating malformed output as supported.
| child.once("close", (code) => { | ||
| finish(() => | ||
| resolve({ | ||
| stdout, | ||
| stderr, | ||
| code: typeof code === "number" ? code : 0, | ||
| }), | ||
| ); | ||
| }); |
There was a problem hiding this comment.
🟡 Medium src/codexCliProbe.ts:89
When the Codex CLI process crashes (e.g., SIGSEGV, SIGABRT), Node.js emits the 'close' event with code === null. The current handler converts this null to 0, which conventionally indicates success. This masks crashes: callers like assertSupportedCodexCliVersion see code !== 0 as false and treat a crashing CLI as healthy/supported. Consider using a non-zero exit code (commonly 1 or 128 + signal) when code is null to properly signal failure.
child.once("close", (code) => {
finish(() =>
resolve({
stdout,
stderr,
- code: typeof code === "number" ? code : 0,
+ code: typeof code === "number" ? code : 1,
}),
);
});🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file apps/server/src/codexCliProbe.ts around lines 89-97:
When the Codex CLI process crashes (e.g., `SIGSEGV`, `SIGABRT`), Node.js emits the 'close' event with `code === null`. The current handler converts this `null` to `0`, which conventionally indicates success. This masks crashes: callers like `assertSupportedCodexCliVersion` see `code !== 0` as false and treat a crashing CLI as healthy/supported. Consider using a non-zero exit code (commonly `1` or `128 + signal`) when `code` is `null` to properly signal failure.
Evidence trail:
apps/server/src/codexCliProbe.ts lines 89-96: `close` event handler with `code: typeof code === "number" ? code : 0` converts null to 0. apps/server/src/codexCliProbe.ts line 194: `if (result.code !== 0)` check in `assertSupportedCodexCliVersion` - passes when code is 0 (including crash-converted-to-0). apps/server/src/codexCliProbe.ts lines 12-17: `CodexCliCommandResult` interface defines `code: number`.
There was a problem hiding this comment.
Fixed in c6b8f24. Null close codes are now treated as probe failures instead of success.
2 participants
What Changed
codexCliProbemodule for Codex CLI availability checksWhy
UI Changes
Checklist
Note
Share Codex CLI probe logic across startup and health checks
codexCliProbe.tswith asyncrunCodexCliCommand,runCodexCliCommandWithRetry, andassertSupportedCodexCliVersionfunctions, replacing separate synchronous implementations in both startup and health check paths.ProviderHealth.tsdrops itseffect/unstable/processdependency and now uses the shared promise-based runner via a newmakeCheckCodexProviderStatusfactory.codexAppServerManager.tsnowawaits the async version probe before spawning the app server.Macroscope summarized c6b8f24.