fix(cli): surface OpenShell errors with context and docs links#345
Closed
hkc5 wants to merge 1 commit intoNVIDIA:mainfrom
Closed
fix(cli): surface OpenShell errors with context and docs links#345hkc5 wants to merge 1 commit intoNVIDIA:mainfrom
hkc5 wants to merge 1 commit intoNVIDIA:mainfrom
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (12)
📒 Files selected for processing (5)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughThis PR standardizes and improves OpenShell-related error handling across CLI commands: it detects missing OpenShell (ENOENT), wraps and reformats errors with documentation/install links, surfaces OpenShell error messages in sandbox status, and updates tests to expect the new status output. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
nemoclaw/src/commands/connect.ts (1)
27-35: Consider using a shared OpenShell error formatter across commands.These strings/URLs are now duplicated in multiple command files; centralizing them (as a shared helper) will prevent drift and keep messaging consistent.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nemoclaw/src/commands/connect.ts` around lines 27 - 35, The duplicated OpenShell error messages in connect.ts (the logger.error calls that output "OpenShell CLI not found..." and the template string using err.message and troubleshooting URL) should be moved into a single shared formatter helper (e.g., formatOpenShellError) exported from a common module (like openshell/errors or openshell/utils); update the connect command to call formatOpenShellError(err, {notFoundMessage?}) or similar and pass the returned string to logger.error, and replace other command files' inline strings with the same helper to centralize the URLs and message formatting.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@nemoclaw/src/commands/logs.ts`:
- Around line 56-66: cliLogs() currently returns early when isSandboxRunning()
fails, which masks OpenShell-missing ENOENT errors; update cliLogs() so errors
from isSandboxRunning() are either rethrown or handled the same way as the
existing catch that checks err.message.includes("ENOENT"). Concretely, in
cliLogs() wrap the isSandboxRunning() call with try/catch and when the caught
error message contains "ENOENT" call the same logger.error message used in the
diff (the OpenShell CLI not found message and install guide), otherwise
propagate or rethrow the error so the outer error handling (the else branch that
logs `OpenShell error running 'openshell sandbox connect'`) can run. Ensure you
reference isSandboxRunning(), cliLogs(), and the logger.error messages so the
behavior is consistent.
In `@nemoclaw/src/commands/status.test.ts`:
- Around line 126-136: Update the test "shows OpenShell error context and 'Not
configured' in text output" to also simulate the missing OpenShell CLI by
causing the underlying exec/spawn/which call used by cliStatus to throw an
ENOENT error (e.g., stub the method that checks/executes OpenShell) while still
using captureLogger() and defaultConfig, then call cliStatus({ json: false,
logger, pluginConfig: defaultConfig }) and add an assertion that the output
contains the OpenShell install-guide URL (the install-guide branch) in addition
to the existing troubleshooting URL check so the ENOENT/install-guide branch in
cliStatus is exercised.
In `@nemoclaw/src/commands/status.ts`:
- Around line 147-151: The current ENOENT check on the exec() error is
unreliable; update the error detection in the status handler that builds
openshellError (using variables err, msg, and openshellError) to also treat
shell-missing-command cases as "OpenShell CLI not found": check if the error
object has code === 127 or if stderr/message contains "command not found"
(case-insensitive) and, if so, set the openshellError to the install-guide
string; otherwise fall back to the existing generic openshell/troubleshooting
message.
---
Nitpick comments:
In `@nemoclaw/src/commands/connect.ts`:
- Around line 27-35: The duplicated OpenShell error messages in connect.ts (the
logger.error calls that output "OpenShell CLI not found..." and the template
string using err.message and troubleshooting URL) should be moved into a single
shared formatter helper (e.g., formatOpenShellError) exported from a common
module (like openshell/errors or openshell/utils); update the connect command to
call formatOpenShellError(err, {notFoundMessage?}) or similar and pass the
returned string to logger.error, and replace other command files' inline strings
with the same helper to centralize the URLs and message formatting.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 01e22515-d682-4342-98b6-cbe2779cd8f5
⛔ Files ignored due to path filters (12)
nemoclaw/dist/commands/connect.d.ts.mapis excluded by!**/dist/**,!**/*.mapnemoclaw/dist/commands/connect.jsis excluded by!**/dist/**nemoclaw/dist/commands/connect.js.mapis excluded by!**/dist/**,!**/*.mapnemoclaw/dist/commands/logs.d.ts.mapis excluded by!**/dist/**,!**/*.mapnemoclaw/dist/commands/logs.jsis excluded by!**/dist/**nemoclaw/dist/commands/logs.js.mapis excluded by!**/dist/**,!**/*.mapnemoclaw/dist/commands/onboard.d.ts.mapis excluded by!**/dist/**,!**/*.mapnemoclaw/dist/commands/onboard.jsis excluded by!**/dist/**nemoclaw/dist/commands/onboard.js.mapis excluded by!**/dist/**,!**/*.mapnemoclaw/dist/commands/status.d.ts.mapis excluded by!**/dist/**,!**/*.mapnemoclaw/dist/commands/status.jsis excluded by!**/dist/**nemoclaw/dist/commands/status.js.mapis excluded by!**/dist/**,!**/*.map
📒 Files selected for processing (5)
nemoclaw/src/commands/connect.tsnemoclaw/src/commands/logs.tsnemoclaw/src/commands/onboard.tsnemoclaw/src/commands/status.test.tsnemoclaw/src/commands/status.ts
Comment on lines
+56
to
+66
| if (err.message.includes("ENOENT")) { | ||
| logger.error( | ||
| "OpenShell CLI not found. Is OpenShell installed?\n" + | ||
| " Install guide: https://docs.nvidia.com/nemoclaw/openshell/install", | ||
| ); | ||
| } else { | ||
| logger.error( | ||
| `OpenShell error running 'openshell sandbox connect': ${err.message}\n` + | ||
| " Troubleshooting: https://docs.nvidia.com/nemoclaw/openshell/troubleshooting", | ||
| ); | ||
| } |
Contributor
There was a problem hiding this comment.
OpenShell-missing errors are still masked before this handler runs.
cliLogs() returns early when isSandboxRunning() fails, so users can still get “not running” instead of the new OpenShell guidance in missing-CLI cases. Please surface OpenShell errors from the preflight path too.
Suggested direction
-const sandboxRunning = await isSandboxRunning(sandboxName);
-if (!sandboxRunning) {
+const sandbox = await isSandboxRunning(sandboxName);
+if (sandbox.error) {
+ logger.error(sandbox.error);
+ return;
+}
+if (!sandbox.running) {
logger.info(`Sandbox '${sandboxName}' is not running. No live logs available.`);
return;
}-async function isSandboxRunning(sandboxName: string): Promise<boolean> {
+async function isSandboxRunning(
+ sandboxName: string,
+): Promise<{ running: boolean; error: string | null }> {
try {
const { stdout } = await execAsync(`openshell sandbox get ${sandboxName} --json`, {
timeout: 5000,
});
const parsed = JSON.parse(stdout) as { state?: string };
- return parsed.state === "running";
- } catch {
- return false;
+ return { running: parsed.state === "running", error: null };
+ } catch (err) {
+ const msg = err instanceof Error ? err.message : String(err);
+ return {
+ running: false,
+ error: `OpenShell preflight error: ${msg}\n Troubleshooting: https://docs.nvidia.com/nemoclaw/openshell/troubleshooting`,
+ };
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@nemoclaw/src/commands/logs.ts` around lines 56 - 66, cliLogs() currently
returns early when isSandboxRunning() fails, which masks OpenShell-missing
ENOENT errors; update cliLogs() so errors from isSandboxRunning() are either
rethrown or handled the same way as the existing catch that checks
err.message.includes("ENOENT"). Concretely, in cliLogs() wrap the
isSandboxRunning() call with try/catch and when the caught error message
contains "ENOENT" call the same logger.error message used in the diff (the
OpenShell CLI not found message and install guide), otherwise propagate or
rethrow the error so the outer error handling (the else branch that logs
`OpenShell error running 'openshell sandbox connect'`) can run. Ensure you
reference isSandboxRunning(), cliLogs(), and the logger.error messages so the
behavior is consistent.
Comment on lines
+126
to
136
| it("shows OpenShell error context and 'Not configured' in text output", async () => { | ||
| const { lines, logger } = captureLogger(); | ||
|
|
||
| await cliStatus({ json: false, logger, pluginConfig: defaultConfig }); | ||
|
|
||
| const output = lines.join("\n"); | ||
| expect(output).toContain("Status: not running"); | ||
| // When openshell is not installed, status shows an actionable error with a docs link | ||
| // rather than a silent "not running" — see https://github.com/NVIDIA/NemoClaw/issues/60 | ||
| expect(output).toContain("Status: unknown —"); | ||
| expect(output).toContain("https://docs.nvidia.com/nemoclaw/openshell/troubleshooting"); | ||
| expect(output).toContain("Not configured"); |
Contributor
There was a problem hiding this comment.
“No openshell” scenario should explicitly cover the install-guide branch.
This assertion currently checks the troubleshooting URL path. Please add/adjust a case that simulates missing CLI (ENOENT) and asserts the install guide URL so the intended branch is protected.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@nemoclaw/src/commands/status.test.ts` around lines 126 - 136, Update the test
"shows OpenShell error context and 'Not configured' in text output" to also
simulate the missing OpenShell CLI by causing the underlying exec/spawn/which
call used by cliStatus to throw an ENOENT error (e.g., stub the method that
checks/executes OpenShell) while still using captureLogger() and defaultConfig,
then call cliStatus({ json: false, logger, pluginConfig: defaultConfig }) and
add an assertion that the output contains the OpenShell install-guide URL (the
install-guide branch) in addition to the existing troubleshooting URL check so
the ENOENT/install-guide branch in cliStatus is exercised.
Comment on lines
+147
to
+151
| const msg = err instanceof Error ? err.message : String(err); | ||
| const openshellError = msg.includes("ENOENT") | ||
| ? "OpenShell CLI not found — see https://docs.nvidia.com/nemoclaw/openshell/install" | ||
| : `OpenShell error: ${msg} — see https://docs.nvidia.com/nemoclaw/openshell/troubleshooting`; | ||
| return { name: sandboxName, running: false, uptime: null, insideSandbox: false, openshellError }; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Node.js child_process.exec (shell command string), what error fields are typically set when the command is missing on Linux/macOS? Specifically, is ENOENT guaranteed in error.message, or is it usually an exit code/stderr "command not found" path?
💡 Result:
On Linux/macOS, child_process.exec(commandString, cb) runs your string inside a shell (typically /bin/sh -c ...), so a missing command is usually not a spawn failure in Node—it’s a shell-level error. Node’s docs note that exec() “spawns a shell” and that, on error, error.code is the exit code of the process. [1]
What you typically see with exec() when the command is missing
error.code: commonly127(the conventional/POSIX shell exit status for “command not found”). [3]stderr: usually contains something likesh: <cmd>: command not found(exact wording varies by shell).error.message: typically starts withCommand failed: <your command>and often includes the shell’s stderr text, but this is not guaranteed to includeENOENT(because the shell did run; the inner command didn’t). [1]
When you do get ENOENT
You get ENOENT when the process can’t be spawned at all (e.g., using spawn()/execFile() without a shell and the executable can’t be found, or if the configured shell path itself doesn’t exist). Node explicitly documents for spawn() that ENOENT is emitted when the command does not exist. [2]
Bottom line: For exec() with a missing command on Linux/macOS, expect exit code 127 + “command not found” on stderr, not an ENOENT in error.message. ENOENT is typical for non-shell spawning (spawn/execFile) or a missing shell, not for a missing command inside the shell. [1][2][3]
Sources: [1] Node.js child_process.exec() docs (latest) (nodejs.org), [2] Node.js child_process.spawn() docs on ENOENT (r2.nodejs.org), [3] POSIX/shell convention: exit status 127 for “command not found” (unix.stackexchange.com)
Citations:
- 1: https://nodejs.org/docs/latest/api/child_process.html?utm_source=openai
- 2: https://r2.nodejs.org/docs/v24.0.1/api/child_process.html?utm_source=openai
- 3: https://unix.stackexchange.com/questions/242111/using-reserved-codes-for-exit-status-of-shell-scripts?utm_source=openai
ENOENT detection is unreliable—exec() surfaces missing commands as shell exit code 127 + stderr.
When child_process.exec() runs with a missing command on Linux/macOS, the shell (not Node) encounters the missing executable and returns exit code 127 with stderr containing "command not found". The error object does not contain ENOENT in the message. The current check msg.includes("ENOENT") will miss this case, showing a generic error instead of the install-guide message.
Suggested fix
} catch (err) {
const msg = err instanceof Error ? err.message : String(err);
- const openshellError = msg.includes("ENOENT")
+ const stderr =
+ err && typeof err === "object" && "stderr" in err
+ ? String((err as { stderr?: unknown }).stderr ?? "")
+ : "";
+ const code =
+ err && typeof err === "object" && "code" in err
+ ? (err as { code?: unknown }).code
+ : undefined;
+ const combined = `${msg}\n${stderr}`.toLowerCase();
+ const missingOpenShell =
+ code === "ENOENT" ||
+ code === 127 ||
+ combined.includes("command not found") ||
+ combined.includes("not found");
+
+ const openshellError = missingOpenShell
? "OpenShell CLI not found — see https://docs.nvidia.com/nemoclaw/openshell/install"
: `OpenShell error: ${msg} — see https://docs.nvidia.com/nemoclaw/openshell/troubleshooting`;
return { name: sandboxName, running: false, uptime: null, insideSandbox: false, openshellError };
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@nemoclaw/src/commands/status.ts` around lines 147 - 151, The current ENOENT
check on the exec() error is unreliable; update the error detection in the
status handler that builds openshellError (using variables err, msg, and
openshellError) to also treat shell-missing-command cases as "OpenShell CLI not
found": check if the error object has code === 127 or if stderr/message contains
"command not found" (case-insensitive) and, if so, set the openshellError to the
install-guide string; otherwise fall back to the existing generic
openshell/troubleshooting message.
wscurran
added
documentation
Closes NVIDIA#60 When an `openshell` CLI call fails, errors now identify OpenShell as the source and include a docs link rather than surfacing raw process errors or silently returning a default value. Changes: - onboard.ts: add `wrapOpenShellError()` helper that wraps ENOENT as "OpenShell CLI not found" and all other failures as "OpenShell error running 'openshell <subcommand>': <detail>" with a link to the troubleshooting guide. `execOpenShell()` now calls this helper so every call site gets consistent messages. - connect.ts: enrich both ENOENT and generic error branches with the same "OpenShell error" prefix and docs link. - logs.ts: add matching ENOENT / generic error handling to the log streaming spawn error handler. - status.ts: instead of silently returning `running: false` when `openshell sandbox status` fails, capture the error into a new `openshellError` field and display it in the status output as "Status: unknown — <message>" so the user knows why the sandbox state could not be determined. - status.test.ts: update the "no openshell" scenario to assert the new actionable error message rather than the silent "not running".
hkc5
force-pushed
the
fix/openshell-error-context
branch
from
ef53663 to
04fb08e
Compare
Contributor
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
3 participants
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.
Closes #60
Problem
When an
openshellCLI call fails, NemoClaw currently either:running: false,"0.0.0") with no user-visible feedbackUsers hitting OpenShell errors have no way to tell whether the problem is NemoClaw or OpenShell, and no pointer to where to look for help.
Changes
onboard.ts— addwrapOpenShellError()helper and use it insideexecOpenShell():"OpenShell CLI not found. Is OpenShell installed?\n Install guide: <url>""OpenShell error running 'openshell <subcommand>': <detail>\n Troubleshooting: <url>"Catch blocks that previously extracted raw
stderrfrom the error object now just readerr.message, which already contains the enriched detail.connect.ts— enrich both ENOENT and generic branches of thespawnerror handler with the same prefix and docs link.logs.ts— same treatment for the log-streaming spawn error handler.status.ts— instead of silently returningrunning: falsewhenopenshell sandbox statusfails, capture the error into a newopenshellError: string | nullfield and display it as"Status: unknown — <message>"so the user knows why the sandbox state could not be determined.status.test.ts— update the "no openshell, blank state" scenario to assert the new actionable error output instead of the silent"Status: not running".Test
Summary by CodeRabbit
New Features
Bug Fixes
Tests