Skip to content

feat(cli): add gateway-token command to retrieve auth token#944

Open
kagura-agent wants to merge 3 commits intoNVIDIA:mainfrom
kagura-agent:feat/gateway-token-command
Open

feat(cli): add gateway-token command to retrieve auth token#944
kagura-agent wants to merge 3 commits intoNVIDIA:mainfrom
kagura-agent:feat/gateway-token-command

Conversation

@kagura-agent
Copy link
Copy Markdown
Contributor

@kagura-agent kagura-agent commented Mar 26, 2026

Summary

Adds nemoclaw <name> gateway-token command that prints the gateway auth token for a sandbox, replacing the multi-step workaround documented in #938.

Before (workaround)

d=$(mktemp -d) && openshell sandbox download my-assistant /sandbox/.openclaw/openclaw.json "$d/" && jq -r '.gateway.auth.token' "$d/openclaw.json" && rm -rf "$d"

After

nemoclaw my-assistant gateway-token

Changes

  • bin/nemoclaw.js: Added sandboxGatewayToken() function and gateway-token action
  • Downloads openclaw.json from sandbox via openshell sandbox download to a temp directory
  • Extracts gateway.auth.token and prints to stdout (pipe-friendly)
  • Cleans up temp files in all cases (try/finally)
  • Clear error messages when sandbox is inaccessible or token is missing
  • Updated help text and valid actions list

Usage

# Print the token
nemoclaw my-assistant gateway-token

# Use in scripts
TOKEN=$(nemoclaw my-assistant gateway-token)
openclaw acp --url wss://localhost:18789 --token "$TOKEN"

Design Notes

  • Reuses the same openshell sandbox download + temp directory + cleanup pattern used by fetchGatewayAuthTokenFromSandbox() in onboard.js
  • Output is just the raw token (no decorations) so it can be captured with $()
  • Discoverable via nemoclaw help under Sandbox Management

Closes #938

Signed-off-by: Kagura kagura-agent@users.noreply.github.com

Summary by CodeRabbit

  • New Features

    • Added gateway-token CLI action to fetch and print a sandbox auth token (token to stdout; warnings to stderr; exits on failure).
    • Added open CLI action to verify a sandbox, ensure local port forwarding, obtain an auth token, derive a Control UI URL (non-token URL logged) and open it in the default browser.
  • Other

    • CLI help now lists the new actions.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Mar 26, 2026

📝 Walkthrough

Walkthrough

Adds two sandbox-scoped CLI actions—gateway-token and open—and exports two onboard helpers (fetchGatewayAuthTokenFromSandbox, buildControlUiUrls). gateway-token prints a sandbox gateway auth token; open ensures a port-forward, retrieves the token, builds Control UI URLs, and launches the system browser.

Changes

Cohort / File(s) Summary
CLI: sandbox actions
bin/nemoclaw.js
Added gateway-token and open actions. New helpers: waitForPort(port, timeoutMs), sandboxGatewayToken(sandboxName), sandboxOpen(sandboxName). gateway-token fetches and prints token (warning to stderr). open starts/ensures background port-forward to local port 18789, waits for port readiness, fetches token, builds Control UI URLs, prints token-free URL, and spawns a detached platform opener. Help text and action dispatch updated.
Onboard helpers export
bin/lib/onboard.js
Expanded module exports to include fetchGatewayAuthTokenFromSandbox and buildControlUiUrls. fetchGatewayAuthTokenFromSandbox downloads/parses openclaw.json from a sandbox and returns gateway.auth.token or null. buildControlUiUrls constructs tokenized and non-tokenized Control UI URLs (deduped).

Sequence Diagram(s)

sequenceDiagram
  participant CLI as CLI (nemoclaw)
  participant Sandbox as Sandbox
  participant Onboard as onboard helpers
  participant PortFwd as PortForward
  participant Opener as OS Browser Opener

  CLI->>Sandbox: ensure sandbox is live
  CLI->>PortFwd: start/ensure port-forward 18789 (background)
  PortFwd-->>CLI: port-forward running
  CLI->>PortFwd: waitForPort(18789)
  PortFwd-->>CLI: port ready
  CLI->>Onboard: fetchGatewayAuthTokenFromSandbox(sandbox)
  Onboard-->>CLI: gateway auth token (or null)
  CLI->>Onboard: buildControlUiUrls(token)
  Onboard-->>CLI: list of Control UI URLs
  CLI->>Opener: spawn platform opener with selected URL
  Opener-->>CLI: spawn success / error
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

🐰 I hopped through sandboxes wide and deep,
I fetched the key you wanted to keep,
I nudged a port and opened the door,
A click, a hop — your dashboard to explore! 🥕✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 20.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'feat(cli): add gateway-token command to retrieve auth token' accurately reflects the primary change in the PR, clearly summarizing the main feature addition.
Linked Issues check ✅ Passed The PR fully implements all coding requirements from issue #938: adds both gateway-token and open commands, exports shared fetch logic, includes token-only stdout output for scripts, applies security warnings, and integrates port-forward validation.
Out of Scope Changes check ✅ Passed All changes are directly aligned with the linked issue #938: the two new CLI commands, shared helper exports, and port-forward validation are all within scope and necessary for the feature.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
bin/nemoclaw.js (1)

339-392: Consider extracting shared token retrieval logic to reduce duplication.

This function duplicates most of the logic from getGatewayToken() in onboard.js (lines 2174-2193). The key difference is the candidate path discovery strategy:

  • onboard.js dynamically discovers subdirectories via fs.readdirSync(...).filter(e => e.isDirectory())
  • This implementation uses a hardcoded path sandbox/.openclaw/openclaw.json plus a find fallback

Consider extracting a shared helper (e.g., in lib/runner.js or a new lib/token.js) that both functions can use. This would ensure consistent behavior if openshell sandbox download changes its output structure.

That said, the find fallback provides reasonable resilience, and the implementation is correct.

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

In `@bin/nemoclaw.js` around lines 339 - 392, Extract the duplicated logic that
downloads/locates/parses openclaw.json and returns the gateway token into a
shared helper (e.g., create getSandboxGatewayToken in lib/token.js or
lib/runner.js) and have sandboxGatewayToken and getGatewayToken (from
onboard.js) call it; the helper should encapsulate: creating the temp dir,
invoking openshell sandbox download, checking candidate paths (openclaw.json and
sandbox/.openclaw/openclaw.json), falling back to a recursive find, parsing
JSON, returning cfg.gateway.auth.token (or throwing a clear error), and
performing cleanup of the temp dir so callers just handle printing/logging and
process.exit semantics.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@bin/nemoclaw.js`:
- Around line 339-392: Extract the duplicated logic that
downloads/locates/parses openclaw.json and returns the gateway token into a
shared helper (e.g., create getSandboxGatewayToken in lib/token.js or
lib/runner.js) and have sandboxGatewayToken and getGatewayToken (from
onboard.js) call it; the helper should encapsulate: creating the temp dir,
invoking openshell sandbox download, checking candidate paths (openclaw.json and
sandbox/.openclaw/openclaw.json), falling back to a recursive find, parsing
JSON, returning cfg.gateway.auth.token (or throwing a clear error), and
performing cleanup of the temp dir so callers just handle printing/logging and
process.exit semantics.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: a3dd1f23-9f80-465c-a03e-6a0f87c5fe44

📥 Commits

Reviewing files that changed from the base of the PR and between 1e1243d and e7818b7.

📒 Files selected for processing (1)
  • bin/nemoclaw.js

@cv
Copy link
Copy Markdown
Contributor

cv commented Mar 26, 2026

@kagura-agent thanks! can you resolve the conflicts with main, please?

@brandonpelfrey
Copy link
Copy Markdown
Collaborator

@kagura-agent We need to minimize showing the token unless required. Two requests (I will add to the issue):

  1. Let's enable nemoclaw <name> open to directly open a browser with the token applied. This minimizes exposure of the token in logs/history/screencast/etc.
  2. we can still have the nemoclaw <name> gateway-token, but it should utilize fetchGatewayAuthTokenFromSandbox which already exists in the code base and have a warning like we already show in onboarding ("Treat this token like a password — do not log or share it")

Add 'nemoclaw <name> gateway-token' command that prints the gateway auth
token for a sandbox, replacing the multi-step workaround with openshell
sandbox download + jq.

The command:
- Downloads openclaw.json from the sandbox via openshell
- Extracts gateway.auth.token from the config
- Prints the token to stdout (pipe-friendly)
- Cleans up temp files in all cases
- Provides clear error messages when sandbox is inaccessible or token
  is not configured

Usage:
  nemoclaw my-assistant gateway-token

The token can be used directly in scripts:
  TOKEN=$(nemoclaw my-assistant gateway-token)
  openclaw acp --url wss://localhost:18789 --token $TOKEN

Closes NVIDIA#938

Signed-off-by: Kagura <kagura-agent@users.noreply.github.com>
@kagura-agent kagura-agent force-pushed the feat/gateway-token-command branch from e7818b7 to f2ad690 Compare March 27, 2026 05:34
kagura-agent added a commit to kagura-agent/NemoClaw that referenced this pull request Mar 27, 2026
…h (PR NVIDIA#944 feedback)

- Export fetchGatewayAuthTokenFromSandbox and buildControlUiUrls from onboard.js
- Refactor sandboxGatewayToken() to use shared fetchGatewayAuthTokenFromSandbox()
- Add security warning to gateway-token output (stderr before token on stdout)
- Add new 'open' command that opens browser with tokenized URL
  - Ensures port forward is active before opening
  - Uses platform-appropriate opener (xdg-open/open/start)
  - Minimizes token exposure compared to manual copy-paste
- Update help text to document both commands

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@kagura-agent
Copy link
Copy Markdown
Contributor Author

kagura-agent commented Mar 27, 2026

@brandonpelfrey @cv Thank you for the thoughtful review.

@cv — Rebased on main, conflicts resolved.

@brandonpelfrey — Your point about minimizing token exposure is well taken. The token showing up in shell history, screencasts, or logs is a real risk that I hadn't given enough weight to in the original implementation. I've restructured the PR around that principle:

nemoclaw <name> open (new)
The primary way to access the UI — opens the browser directly with the token embedded in the URL fragment (never hits the server, stays client-side). This way users don't need to see or copy the token at all. It also checks that the port-forward is active before opening to avoid confusing "connection refused" errors.

nemoclaw <name> gateway-token (refactored)
For scripting use cases where the token is genuinely needed. Now:

  • Uses the shared fetchGatewayAuthTokenFromSandbox() from onboard.js instead of duplicating the retrieval logic (addresses the CodeRabbit feedback too)
  • Prints a security warning to stderr before outputting the token to stdout, so scripts can still pipe the token cleanly while interactive users see the caution

I also exported fetchGatewayAuthTokenFromSandbox and buildControlUiUrls from onboard.js so any future commands can reuse them.

Let me know if anything needs adjustment.

…h (PR NVIDIA#944 feedback)

- Export fetchGatewayAuthTokenFromSandbox and buildControlUiUrls from onboard.js
- Refactor sandboxGatewayToken() to use shared fetchGatewayAuthTokenFromSandbox()
- Add security warning to gateway-token output (stderr before token on stdout)
- Add new 'open' command that opens browser with tokenized URL
  - Ensures port forward is active before opening
  - Uses platform-appropriate opener (xdg-open/open/start)
  - Minimizes token exposure compared to manual copy-paste
- Update help text to document both commands

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@kagura-agent kagura-agent force-pushed the feat/gateway-token-command branch from f2ad690 to b926994 Compare March 27, 2026 05:39
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 3

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@bin/nemoclaw.js`:
- Around line 667-683: The code calls runOpenshell(["forward", "start",
"--background", "18789", sandboxName], { ignoreError: true }) but does not
verify that the forwarded port is accepting connections before opening the UI;
add a short retry loop after that call to probe 127.0.0.1:18789 (e.g., TCP
connect or HTTP HEAD/GET) with a small delay and a capped number of attempts
(e.g., 5–10 attempts, 200–500ms backoff), and only proceed to call
buildControlUiUrls/fetchGatewayAuthTokenFromSandbox and spawn the browser once
the probe succeeds; if the probe never succeeds within the timeout, log a clear
error and exit (similar to the fetchGatewayAuthTokenFromSandbox error handling)
so opening the Control UI is deterministic.
- Around line 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).
- Around line 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.
🪄 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

Run ID: 7f2ea934-df08-46a3-96f8-7266985b0dca

📥 Commits

Reviewing files that changed from the base of the PR and between f2ad690 and b926994.

📒 Files selected for processing (2)
  • bin/lib/onboard.js
  • bin/nemoclaw.js
🚧 Files skipped from review as they are similar to previous changes (1)
  • bin/lib/onboard.js

bin/nemoclaw.js Outdated
Comment on lines +654 to +664
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).

Comment on lines +685 to +696
// 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);
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.

…t, fix Windows start

- gateway-token: add ensureLiveSandboxOrExit() before fetching token (consistent with connect/open)
- open: wait for port 18789 to accept connections before launching browser (TCP probe with 10s timeout)
- open: fix Windows compatibility — 'start' is a cmd built-in, needs shell: true and empty title arg
@kagura-agent
Copy link
Copy Markdown
Contributor Author

Addressed all three review points from CodeRabbit:

  1. Reconciliation before token fetchsandboxGatewayToken now calls ensureLiveSandboxOrExit() before fetching the token, consistent with connect and open.

  2. Wait for port forward — Added waitForPort(18789, 10000) in the open command — a TCP connection probe that retries every 500ms with a 10s timeout. Browser only opens once the port is actually accepting connections.

  3. Windows start compatibilitystart is a cmd built-in, not an executable. Fixed with shell: true and an empty title argument (start "" <url>) to prevent the URL from being interpreted as a window title.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

♻️ Duplicate comments (2)
bin/nemoclaw.js (2)

709-710: ⚠️ Potential issue | 🟠 Major

Handle browser-launch failures on the child process, and keep the fallback URL redacted.

spawn() launch failures here are reported on the returned child’s error event, so the current try/catch will miss missing xdg-open/open/shell failures. If the fallback ever does run, it logs url instead of safeUrl, which defeats the secret-minimization goal of this command. Attach child.on("error", ...) and keep the manual-open message fragment-free.

🛠️ Suggested fix
-  try {
-    const child = require("child_process").spawn(opener, isWin ? ["", url] : [url], {
-      stdio: "ignore",
-      detached: true,
-      shell: isWin,
-    });
-    child.unref();
-  } catch (_e) {
-    console.error(`  ${_RD}Error${R}: Could not open browser. Please open this URL manually:`);
-    console.error(`  ${url}`);
-    process.exit(1);
-  }
+  const child = require("child_process").spawn(opener, isWin ? ["", url] : [url], {
+    stdio: "ignore",
+    detached: true,
+    shell: isWin,
+  });
+  child.on("error", () => {
+    console.error(`  ${_RD}Error${R}: Could not open browser. Please open this URL manually:`);
+    console.error(`  ${safeUrl}`);
+    console.error(`  If you need the auth token for manual login, run: nemoclaw ${sandboxName} gateway-token`);
+    process.exit(1);
+  });
+  child.unref();
In Node.js child_process.spawn, are launch failures like ENOENT delivered via the ChildProcess "error" event rather than thrown synchronously, and is `start` on Windows only usable through cmd/shell handling?

Also applies to: 716-725

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

In `@bin/nemoclaw.js` around lines 709 - 710, The code currently spawns a browser
child and only relies on try/catch, which misses async spawn failures; attach an
error handler to the returned ChildProcess (child.on("error", err => {
processLogger.error(...); })) to detect ENOENT etc., and in that handler print
the manual-open fallback using safeUrl (not url) so secrets remain redacted;
update the spawn logic around the variable used to store the ChildProcess (the
`child` returned from spawn) and ensure the existing fallback console message
uses `safeUrl` and is only emitted after handling both child errors and normal
exit conditions.

696-696: ⚠️ Potential issue | 🟠 Major

Catch waitForPort() timeouts and exit cleanly.

If this probe times out, the rejection currently escapes the command instead of producing a normal CLI error. Wrap the await in try/catch and print the same style of actionable message you use for the other sandbox failure paths.

🛠️ Suggested fix
-  await waitForPort(18789, 10000);
+  try {
+    await waitForPort(18789, 10000);
+  } catch {
+    console.error(`  ${_RD}Error${R}: Control UI port 18789 did not become reachable for sandbox '${sandboxName}'.`);
+    process.exit(1);
+  }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@bin/nemoclaw.js` at line 696, The await waitForPort(18789, 10000); call can
reject on timeout; wrap it in a try/catch inside the same function that runs the
sandbox startup, catch the rejection from waitForPort and call the same CLI
failure handler used for other sandbox failures (print a clear actionable error
message and exit with a non-zero code), e.g., mirror the style used elsewhere in
this module for sandbox errors so the command exits cleanly instead of letting
the rejection escape.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@bin/nemoclaw.js`:
- Around line 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.

---

Duplicate comments:
In `@bin/nemoclaw.js`:
- Around line 709-710: The code currently spawns a browser child and only relies
on try/catch, which misses async spawn failures; attach an error handler to the
returned ChildProcess (child.on("error", err => { processLogger.error(...); }))
to detect ENOENT etc., and in that handler print the manual-open fallback using
safeUrl (not url) so secrets remain redacted; update the spawn logic around the
variable used to store the ChildProcess (the `child` returned from spawn) and
ensure the existing fallback console message uses `safeUrl` and is only emitted
after handling both child errors and normal exit conditions.
- Line 696: The await waitForPort(18789, 10000); call can reject on timeout;
wrap it in a try/catch inside the same function that runs the sandbox startup,
catch the rejection from waitForPort and call the same CLI failure handler used
for other sandbox failures (print a clear actionable error message and exit with
a non-zero code), e.g., mirror the style used elsewhere in this module for
sandbox errors so the command exits cleanly instead of letting the rejection
escape.
🪄 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

Run ID: 0790694c-5097-47d8-944c-9fdd56317f9d

📥 Commits

Reviewing files that changed from the base of the PR and between b926994 and 77477f4.

📒 Files selected for processing (1)
  • bin/nemoclaw.js

Comment on lines +694 to +695
// Start port forward and wait for it to be ready
runOpenshell(["forward", "start", "--background", "18789", sandboxName], { ignoreError: true });
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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add CLI command to retrieve gateway auth token

3 participants