Skip to content
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions bin/lib/onboard.js
Original file line number Diff line number Diff line change
Expand Up @@ -2353,6 +2353,8 @@ async function onboard(opts = {}) {

module.exports = {
buildSandboxConfigSyncScript,
buildControlUiUrls,
fetchGatewayAuthTokenFromSandbox,
getFutureShellPathHint,
createSandbox,
getSandboxInferenceConfig,
Expand Down
70 changes: 62 additions & 8 deletions bin/nemoclaw.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,11 @@ const YW = _useColor ? "\x1b[1;33m" : "";

const { ROOT, SCRIPTS, run, runCapture: _runCapture, runInteractive, shellQuote, validateName } = require("./lib/runner");
const { resolveOpenshell } = require("./lib/resolve-openshell");
const { startGatewayForRecovery } = require("./lib/onboard");
const {
buildControlUiUrls,
fetchGatewayAuthTokenFromSandbox,
startGatewayForRecovery,
} = require("./lib/onboard");
const {
ensureApiKey,
ensureGithubToken,
Expand Down Expand Up @@ -647,6 +651,52 @@ function sandboxLogs(sandboxName, follow) {
runOpenshell(args);
}

function sandboxGatewayToken(sandboxName) {
const token = fetchGatewayAuthTokenFromSandbox(sandboxName);
if (!token) {
console.error(` ${_RD}Error${R}: Could not retrieve gateway auth token from sandbox '${sandboxName}'.`);
console.error(` Make sure the sandbox is running and accessible.`);
process.exit(1);
}
// Print warning to stderr before token to stdout
console.error(` ${YW}⚠️ Treat this token like a password. Do not share it or include it in logs/screencasts.${R}`);
console.log(token);
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Run the same live-sandbox reconciliation before printing a token.

Line 655 goes straight to fetchGatewayAuthTokenFromSandbox(), so gateway-token skips the stale-registry cleanup and gateway recovery that connect and open already use. Because that helper returns null for both transport failures and missing token, this command can fail after a gateway restart even when the sandbox is still recoverable.

Suggested fix
-function sandboxGatewayToken(sandboxName) {
+async function sandboxGatewayToken(sandboxName) {
+  await ensureLiveSandboxOrExit(sandboxName);
   const token = fetchGatewayAuthTokenFromSandbox(sandboxName);
   if (!token) {
     console.error(`  ${_RD}Error${R}: Could not retrieve gateway auth token from sandbox '${sandboxName}'.`);
     console.error(`  Make sure the sandbox is running and accessible.`);
     process.exit(1);
@@
-      case "gateway-token": sandboxGatewayToken(cmd); break;
+      case "gateway-token": await sandboxGatewayToken(cmd); break;

Also applies to: 860-860

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@bin/nemoclaw.js` around lines 654 - 664, The sandboxGatewayToken path skips
the live-sandbox reconciliation that connect/open perform and thus can falsely
fail when the gateway is recoverable; modify sandboxGatewayToken to invoke the
same live-sandbox reconciliation routine used by connect/open (e.g., call the
reconcile/recovery function that performs stale-registry cleanup and gateway
recovery) before calling fetchGatewayAuthTokenFromSandbox(sandboxName), retry
fetching the token after a successful reconciliation, and ensure you distinguish
transport/recovery failures from a legitimately missing token in the error
handling and messages (use the same recovery error handling logic as
connect/open).


async function sandboxOpen(sandboxName) {
await ensureLiveSandboxOrExit(sandboxName);
// Ensure port forward is alive before opening
runOpenshell(["forward", "start", "--background", "18789", sandboxName], { ignoreError: true });
Comment on lines +694 to +695
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Stop any existing Control UI forward before reusing port 18789.

forward start --background 18789 ... is best-effort, and waitForPort() only proves that something is listening on that port. If a stale OpenShell forward or another local service already owns 127.0.0.1:18789, this command can open the wrong page and expose the #token=... fragment to that page’s JavaScript. bin/lib/onboard.js:1578-1582 already does a forward stop first for this exact case.

🛠️ Suggested fix
 async function sandboxOpen(sandboxName) {
   await ensureLiveSandboxOrExit(sandboxName);
   // Start port forward and wait for it to be ready
+  runOpenshell(["forward", "stop", "18789"], { ignoreError: true });
   runOpenshell(["forward", "start", "--background", "18789", sandboxName], { ignoreError: true });
   await waitForPort(18789, 10000);
📝 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
// Start port forward and wait for it to be ready
runOpenshell(["forward", "start", "--background", "18789", sandboxName], { ignoreError: true });
async function sandboxOpen(sandboxName) {
await ensureLiveSandboxOrExit(sandboxName);
// Start port forward and wait for it to be ready
runOpenshell(["forward", "stop", "18789"], { ignoreError: true });
runOpenshell(["forward", "start", "--background", "18789", sandboxName], { ignoreError: true });
await waitForPort(18789, 10000);
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@bin/nemoclaw.js` around lines 694 - 695, Before starting the OpenShell
forward on port 18789, stop any existing forward to avoid reusing a stale
listener; call runOpenshell(["forward","stop","18789", sandboxName], {
ignoreError: true }) (or equivalent) immediately before the existing
runOpenshell(["forward","start","--background","18789", sandboxName], ...) so
that runOpenshell("forward stop") is executed (ignoring errors) then
runOpenshell("forward start") to ensure the new forward is the one that binds to
127.0.0.1:18789 and prevents leaking the `#token` fragment to an unrelated
listener.


const token = fetchGatewayAuthTokenFromSandbox(sandboxName);
if (!token) {
console.error(` ${_RD}Error${R}: Could not retrieve gateway auth token from sandbox '${sandboxName}'.`);
console.error(` Make sure the sandbox is running and accessible.`);
process.exit(1);
}

const urls = buildControlUiUrls(token);
const url = urls[0]; // Use the first URL (localhost)

// Log without token to avoid exposure in terminal history/screencasts
const safeUrl = url.replace(/#.*$/, "");
console.log(` Opening Control UI at ${safeUrl} ...`);

// Open URL in default browser (platform-appropriate)
const opener = process.platform === "darwin" ? "open" : process.platform === "win32" ? "start" : "xdg-open";
try {
const child = require("child_process").spawn(opener, [url], {
stdio: "ignore",
detached: true,
});
child.unref();
} catch (_e) {
console.error(` ${_RD}Error${R}: Could not open browser. Please open this URL manually:`);
console.error(` ${url}`);
process.exit(1);
Comment on lines +712 to +726
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🌐 Web query:

In Node.js child_process, does spawn("start", [url])work on Windows, or isstartonly acmd.exebuiltin that requirescmd /c start "" orshell: true? Also, are command-launch failures such as ENOENT reported via the ChildProcess error event rather than thrown synchronously?

💡 Result:

No, spawn("start", [url]) does not work on Windows because start is a cmd.exe builtin command, not a standalone executable. There is no start.exe in the PATH, so spawn (which does not use a shell by default) fails with ENOENT. To use start, you must either spawn cmd.exe explicitly with spawn("cmd.exe", ["/c", "start", "", url]) (the empty quoted string "" is needed as a window title placeholder before the URL if it has spaces), use spawn("start", [url], {shell: true}), or use exec which always spawns a shell. Command-launch failures like ENOENT are reported asynchronously via the ChildProcess error event, not thrown synchronously. The spawn function returns a ChildProcess instance immediately (even on failure to launch), and errors such as inability to find the executable are emitted on the 'error' event. Unhandled errors will crash the process with an uncaught exception, but they are not synchronous throws.

Citations:


🏁 Script executed:

# First, let's find and read the bin/nemoclaw.js file
fd nemoclaw.js --type f

Repository: NVIDIA/NemoClaw

Length of output: 74


🏁 Script executed:

# Read lines 685-696 and some surrounding context to understand the function
sed -n '670,710p' bin/nemoclaw.js | cat -n

Repository: NVIDIA/NemoClaw

Length of output: 1777


🏁 Script executed:

# Search for where this function is defined and what the 'url' parameter contains
rg -B 20 'const opener = process.platform' bin/nemoclaw.js

Repository: NVIDIA/NemoClaw

Length of output: 1023


Use shell: true and cmd.exe on Windows to open URLs, add error handling for spawn failures, and print the safe URL in the fallback error message.

The code has three related issues:

  1. Windows incompatibility: spawn("start", [url]) fails with ENOENT because start is a cmd.exe builtin, not a standalone executable. This requires either spawn("cmd.exe", ["/c", "start", "", url]) or {shell: true} to work.

  2. Error handling doesn't work: spawn() errors are emitted asynchronously on the child process error event, not thrown synchronously. The try/catch won't catch spawn failures, so Windows errors silently crash the process (unless an error listener is attached). Add .on('error', ...) to handle launch failures.

  3. Token exposure in fallback: Line 695 prints the full url including the #token=... fragment in the error message. This contradicts the deliberate secret-minimization pattern on lines 677–678 (safeUrl = url.replace(/#.*$/, "")). Print safeUrl instead.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@bin/nemoclaw.js` around lines 685 - 696, The browser-launch block should
handle Windows builtins, async spawn errors, and avoid exposing tokens: when
determining the opener (see variable opener) use a Windows-specific invocation
(either spawn "cmd.exe" with ["/c","start","", url] or call spawn with { shell:
true } on win32), attach an 'error' event handler to the child process returned
by require("child_process").spawn(...) to catch asynchronous launch failures and
report them via console.error, and in the fallback error message print the
precomputed safeUrl (the variable safeUrl that strips the fragment) instead of
the full url to avoid leaking tokens; update the child.unref() usage to still
run after attaching the handler.

}
}

async function sandboxPolicyAdd(sandboxName) {
const allPresets = policies.listPresets();
const applied = policies.getAppliedPresets(sandboxName);
Expand Down Expand Up @@ -724,6 +774,8 @@ function help() {
nemoclaw <name> connect Shell into a running sandbox
nemoclaw <name> status Sandbox health + NIM status
nemoclaw <name> logs ${D}[--follow]${R} Stream sandbox logs
nemoclaw <name> open Open Control UI in browser with auth token
nemoclaw <name> gateway-token Print the gateway auth token ${D}(for scripts)${R}
nemoclaw <name> destroy Stop NIM + delete sandbox ${D}(--yes to skip prompt)${R}

${G}Policy Presets:${R}
Expand Down Expand Up @@ -800,15 +852,17 @@ const [cmd, ...args] = process.argv.slice(2);
const actionArgs = args.slice(1);

switch (action) {
case "connect": await sandboxConnect(cmd); break;
case "status": await sandboxStatus(cmd); break;
case "logs": sandboxLogs(cmd, actionArgs.includes("--follow")); break;
case "policy-add": await sandboxPolicyAdd(cmd); break;
case "policy-list": sandboxPolicyList(cmd); break;
case "destroy": await sandboxDestroy(cmd, actionArgs); break;
case "connect": await sandboxConnect(cmd); break;
case "status": await sandboxStatus(cmd); break;
case "logs": sandboxLogs(cmd, actionArgs.includes("--follow")); break;
case "policy-add": await sandboxPolicyAdd(cmd); break;
case "policy-list": sandboxPolicyList(cmd); break;
case "gateway-token": sandboxGatewayToken(cmd); break;
case "open": await sandboxOpen(cmd); break;
case "destroy": await sandboxDestroy(cmd, actionArgs); break;
default:
console.error(` Unknown action: ${action}`);
console.error(` Valid actions: connect, status, logs, policy-add, policy-list, destroy`);
console.error(` Valid actions: connect, status, logs, policy-add, policy-list, gateway-token, open, destroy`);
process.exit(1);
}
return;
Expand Down