From e8a2bbb7c07ad456622ebc362e1f8a81bd3f56f9 Mon Sep 17 00:00:00 2001 From: Brian Taylor Date: Tue, 17 Mar 2026 08:58:34 -0700 Subject: [PATCH 1/6] fix: propagate sandbox name to bridge and resolve openshell path Signed-off-by: Brian Taylor --- scripts/start-services.sh | 2 +- scripts/telegram-bridge.js | 24 +++++++++++++++++++++++- 2 files changed, 24 insertions(+), 2 deletions(-) diff --git a/scripts/start-services.sh b/scripts/start-services.sh index 7c3bd30ecc8..9f30bf2a4e8 100755 --- a/scripts/start-services.sh +++ b/scripts/start-services.sh @@ -140,7 +140,7 @@ do_start() { # Telegram bridge (only if token provided) if [ -n "${TELEGRAM_BOT_TOKEN:-}" ]; then - start_service telegram-bridge \ + SANDBOX_NAME="$SANDBOX_NAME" start_service telegram-bridge \ node "$REPO_DIR/scripts/telegram-bridge.js" fi diff --git a/scripts/telegram-bridge.js b/scripts/telegram-bridge.js index 5d1af0be5ed..342d38e7755 100755 --- a/scripts/telegram-bridge.js +++ b/scripts/telegram-bridge.js @@ -19,6 +19,28 @@ const https = require("https"); const { execSync, spawn } = require("child_process"); +function resolveOpenshell() { + try { + return execSync("command -v openshell", { encoding: "utf-8" }).trim(); + } catch {} + const home = process.env.HOME || "/tmp"; + const candidates = [ + `${home}/.local/bin/openshell`, + "/usr/local/bin/openshell", + "/usr/bin/openshell", + ]; + for (const p of candidates) { + try { + require("fs").accessSync(p, require("fs").constants.X_OK); + return p; + } catch {} + } + console.error("openshell not found on PATH or in common locations"); + process.exit(1); +} + +const OPENSHELL = resolveOpenshell(); + const TOKEN = process.env.TELEGRAM_BOT_TOKEN; const API_KEY = process.env.NVIDIA_API_KEY; const SANDBOX = process.env.SANDBOX_NAME || "nemoclaw"; @@ -85,7 +107,7 @@ async function sendTyping(chatId) { function runAgentInSandbox(message, sessionId) { return new Promise((resolve) => { - const sshConfig = execSync(`openshell sandbox ssh-config ${SANDBOX}`, { encoding: "utf-8" }); + const sshConfig = execSync(`"${OPENSHELL}" sandbox ssh-config "${SANDBOX}"`, { encoding: "utf-8" }); // Write temp ssh config const confPath = `/tmp/nemoclaw-tg-ssh-${sessionId}.conf`; From e037bfa4a4f94a7f71d9e53c2e3250a25a0b27ae Mon Sep 17 00:00:00 2001 From: Brian Taylor Date: Tue, 17 Mar 2026 09:18:09 -0700 Subject: [PATCH 2/6] fix: validate resolveOpenshell returns absolute path Reject non-absolute paths from command -v (e.g. aliases or functions) and fall through to the explicit candidate list. Signed-off-by: Brian Taylor --- scripts/telegram-bridge.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/scripts/telegram-bridge.js b/scripts/telegram-bridge.js index 342d38e7755..ea83260d1d3 100755 --- a/scripts/telegram-bridge.js +++ b/scripts/telegram-bridge.js @@ -21,7 +21,8 @@ const { execSync, spawn } = require("child_process"); function resolveOpenshell() { try { - return execSync("command -v openshell", { encoding: "utf-8" }).trim(); + const found = execSync("command -v openshell", { encoding: "utf-8" }).trim(); + if (found.startsWith("/")) return found; } catch {} const home = process.env.HOME || "/tmp"; const candidates = [ From e1e7e4662b3acb2f9718326cdb9de334c3299080 Mon Sep 17 00:00:00 2001 From: Brian Taylor Date: Tue, 17 Mar 2026 11:48:02 -0700 Subject: [PATCH 3/6] fix: pass default sandbox name from registry to start-services.sh start-services.sh unconditionally set SANDBOX_NAME to "default", ignoring any exported value. nemoclaw start also never passed the sandbox name from the registry. Together these caused the telegram bridge to target the wrong sandbox. - Preserve existing SANDBOX_NAME in start-services.sh before defaulting - Pass the registry's default sandbox from nemoclaw start Signed-off-by: Brian Taylor --- bin/nemoclaw.js | 4 +++- scripts/start-services.sh | 2 +- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/bin/nemoclaw.js b/bin/nemoclaw.js index 07bb3d5b5a2..bce0d6fd91a 100755 --- a/bin/nemoclaw.js +++ b/bin/nemoclaw.js @@ -134,7 +134,9 @@ async function deploy(instanceName) { async function start() { await ensureApiKey(); - run(`bash "${SCRIPTS}/start-services.sh"`); + const { defaultSandbox } = registry.listSandboxes(); + const sandboxEnv = defaultSandbox ? `SANDBOX_NAME="${defaultSandbox}"` : ""; + run(`${sandboxEnv} bash "${SCRIPTS}/start-services.sh"`); } function stop() { diff --git a/scripts/start-services.sh b/scripts/start-services.sh index 9f30bf2a4e8..cbce0f18359 100755 --- a/scripts/start-services.sh +++ b/scripts/start-services.sh @@ -19,7 +19,7 @@ REPO_DIR="$(cd "$SCRIPT_DIR/.." && pwd)" DASHBOARD_PORT="${DASHBOARD_PORT:-18789}" # ── Parse flags ────────────────────────────────────────────────── -SANDBOX_NAME="${NEMOCLAW_SANDBOX:-default}" +SANDBOX_NAME="${NEMOCLAW_SANDBOX:-${SANDBOX_NAME:-default}}" ACTION="start" while [ $# -gt 0 ]; do From 78b31bedce551a5e12efee0d49b18cefd52af14e Mon Sep 17 00:00:00 2001 From: Brian Taylor Date: Tue, 17 Mar 2026 13:24:09 -0700 Subject: [PATCH 4/6] test: add resolveOpenshell and SANDBOX_NAME defaulting tests Signed-off-by: Brian Taylor --- test/service-env.test.js | 138 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 138 insertions(+) create mode 100644 test/service-env.test.js diff --git a/test/service-env.test.js b/test/service-env.test.js new file mode 100644 index 00000000000..b496653ea99 --- /dev/null +++ b/test/service-env.test.js @@ -0,0 +1,138 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +const { describe, it } = require("node:test"); +const assert = require("node:assert/strict"); +const { execSync } = require("child_process"); +const fs = require("fs"); +const path = require("path"); + +describe("service environment", () => { + describe("resolveOpenshell logic", () => { + // Extract and test the resolution algorithm without requiring openshell installed + + function resolveOpenshellTestable(opts = {}) { + const commandVResult = opts.commandVResult; // string or null (throws) + const existingPaths = opts.existingPaths || []; + const home = opts.home || "/fakehome"; + + // Step 1: command -v result + if (commandVResult && commandVResult.startsWith("/")) { + return commandVResult; + } + + // Step 2: fallback candidates + const candidates = [ + `${home}/.local/bin/openshell`, + "/usr/local/bin/openshell", + "/usr/bin/openshell", + ]; + for (const p of candidates) { + if (existingPaths.includes(p)) return p; + } + + return null; // not found + } + + it("returns command -v result when absolute path", () => { + assert.equal( + resolveOpenshellTestable({ commandVResult: "/usr/bin/openshell" }), + "/usr/bin/openshell" + ); + }); + + it("rejects non-absolute command -v result (alias)", () => { + assert.equal( + resolveOpenshellTestable({ commandVResult: "openshell" }), + null + ); + }); + + it("rejects alias definition from command -v", () => { + assert.equal( + resolveOpenshellTestable({ commandVResult: "alias openshell='echo pwned'" }), + null + ); + }); + + it("falls back to ~/.local/bin when command -v fails", () => { + assert.equal( + resolveOpenshellTestable({ + commandVResult: null, + existingPaths: ["/fakehome/.local/bin/openshell"], + home: "/fakehome", + }), + "/fakehome/.local/bin/openshell" + ); + }); + + it("falls back to /usr/local/bin", () => { + assert.equal( + resolveOpenshellTestable({ + commandVResult: null, + existingPaths: ["/usr/local/bin/openshell"], + }), + "/usr/local/bin/openshell" + ); + }); + + it("falls back to /usr/bin", () => { + assert.equal( + resolveOpenshellTestable({ + commandVResult: null, + existingPaths: ["/usr/bin/openshell"], + }), + "/usr/bin/openshell" + ); + }); + + it("prefers ~/.local/bin over /usr/local/bin", () => { + assert.equal( + resolveOpenshellTestable({ + commandVResult: null, + existingPaths: ["/fakehome/.local/bin/openshell", "/usr/local/bin/openshell"], + home: "/fakehome", + }), + "/fakehome/.local/bin/openshell" + ); + }); + + it("returns null when openshell not found anywhere", () => { + assert.equal( + resolveOpenshellTestable({ + commandVResult: null, + existingPaths: [], + }), + null + ); + }); + }); + + describe("SANDBOX_NAME defaulting", () => { + it("start-services.sh preserves existing SANDBOX_NAME", () => { + // Verify the bash variable expansion logic: + // SANDBOX_NAME="${NEMOCLAW_SANDBOX:-${SANDBOX_NAME:-default}}" + const result = execSync( + 'SANDBOX_NAME=my-box bash -c \'SANDBOX_NAME="${NEMOCLAW_SANDBOX:-${SANDBOX_NAME:-default}}" && echo $SANDBOX_NAME\'', + { encoding: "utf-8" } + ).trim(); + assert.equal(result, "my-box"); + }); + + it("start-services.sh uses NEMOCLAW_SANDBOX over SANDBOX_NAME", () => { + const result = execSync( + 'NEMOCLAW_SANDBOX=from-env SANDBOX_NAME=old bash -c \'SANDBOX_NAME="${NEMOCLAW_SANDBOX:-${SANDBOX_NAME:-default}}" && echo $SANDBOX_NAME\'', + { encoding: "utf-8" } + ).trim(); + assert.equal(result, "from-env"); + }); + + it("start-services.sh falls back to default when both unset", () => { + const result = execSync( + 'bash -c \'SANDBOX_NAME="${NEMOCLAW_SANDBOX:-${SANDBOX_NAME:-default}}" && echo $SANDBOX_NAME\'', + { encoding: "utf-8" } + ).trim(); + assert.equal(result, "default"); + }); + }); +}); From 8be53296f26cb87acd71a8526dbadb51636d51a3 Mon Sep 17 00:00:00 2001 From: Brian Taylor Date: Tue, 17 Mar 2026 18:41:36 -0700 Subject: [PATCH 5/6] fix: address CodeRabbit review comments - Remove unused fs/path imports from test file - Extract resolveOpenshell to shared module with DI for direct testing - Validate sandbox name before shell interpolation --- bin/lib/resolve-openshell.js | 49 +++++++++++++++++++++++++++++++ bin/nemoclaw.js | 3 +- scripts/telegram-bridge.js | 22 ++------------ test/service-env.test.js | 56 +++++++++--------------------------- 4 files changed, 68 insertions(+), 62 deletions(-) create mode 100644 bin/lib/resolve-openshell.js diff --git a/bin/lib/resolve-openshell.js b/bin/lib/resolve-openshell.js new file mode 100644 index 00000000000..3260b44e439 --- /dev/null +++ b/bin/lib/resolve-openshell.js @@ -0,0 +1,49 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +const { execSync } = require("child_process"); +const fs = require("fs"); + +/** + * Resolve the openshell binary path. + * + * Checks `command -v` first (must return an absolute path to prevent alias + * injection), then falls back to common installation directories. + * + * @param {object} [opts] DI overrides for testing + * @param {string|null} [opts.commandVResult] Mock result (undefined = run real command) + * @param {function} [opts.checkExecutable] (path) => boolean + * @param {string} [opts.home] HOME override + * @returns {string|null} Absolute path to openshell, or null if not found + */ +function resolveOpenshell(opts = {}) { + const home = opts.home || process.env.HOME || "/tmp"; + + // Step 1: command -v + if (opts.commandVResult === undefined) { + try { + const found = execSync("command -v openshell", { encoding: "utf-8" }).trim(); + if (found.startsWith("/")) return found; + } catch {} + } else if (opts.commandVResult && opts.commandVResult.startsWith("/")) { + return opts.commandVResult; + } + + // Step 2: fallback candidates + const checkExecutable = opts.checkExecutable || ((p) => { + try { fs.accessSync(p, fs.constants.X_OK); return true; } catch { return false; } + }); + + const candidates = [ + `${home}/.local/bin/openshell`, + "/usr/local/bin/openshell", + "/usr/bin/openshell", + ]; + for (const p of candidates) { + if (checkExecutable(p)) return p; + } + + return null; +} + +module.exports = { resolveOpenshell }; diff --git a/bin/nemoclaw.js b/bin/nemoclaw.js index bce0d6fd91a..686a5dc31e0 100755 --- a/bin/nemoclaw.js +++ b/bin/nemoclaw.js @@ -135,7 +135,8 @@ async function deploy(instanceName) { async function start() { await ensureApiKey(); const { defaultSandbox } = registry.listSandboxes(); - const sandboxEnv = defaultSandbox ? `SANDBOX_NAME="${defaultSandbox}"` : ""; + const safeName = defaultSandbox && /^[a-zA-Z0-9._-]+$/.test(defaultSandbox) ? defaultSandbox : null; + const sandboxEnv = safeName ? `SANDBOX_NAME="${safeName}"` : ""; run(`${sandboxEnv} bash "${SCRIPTS}/start-services.sh"`); } diff --git a/scripts/telegram-bridge.js b/scripts/telegram-bridge.js index ea83260d1d3..80a29069d8a 100755 --- a/scripts/telegram-bridge.js +++ b/scripts/telegram-bridge.js @@ -18,30 +18,14 @@ const https = require("https"); const { execSync, spawn } = require("child_process"); +const { resolveOpenshell } = require("../bin/lib/resolve-openshell"); -function resolveOpenshell() { - try { - const found = execSync("command -v openshell", { encoding: "utf-8" }).trim(); - if (found.startsWith("/")) return found; - } catch {} - const home = process.env.HOME || "/tmp"; - const candidates = [ - `${home}/.local/bin/openshell`, - "/usr/local/bin/openshell", - "/usr/bin/openshell", - ]; - for (const p of candidates) { - try { - require("fs").accessSync(p, require("fs").constants.X_OK); - return p; - } catch {} - } +const OPENSHELL = resolveOpenshell(); +if (!OPENSHELL) { console.error("openshell not found on PATH or in common locations"); process.exit(1); } -const OPENSHELL = resolveOpenshell(); - const TOKEN = process.env.TELEGRAM_BOT_TOKEN; const API_KEY = process.env.NVIDIA_API_KEY; const SANDBOX = process.env.SANDBOX_NAME || "nemoclaw"; diff --git a/test/service-env.test.js b/test/service-env.test.js index b496653ea99..a8616fddc49 100644 --- a/test/service-env.test.js +++ b/test/service-env.test.js @@ -4,62 +4,36 @@ const { describe, it } = require("node:test"); const assert = require("node:assert/strict"); const { execSync } = require("child_process"); -const fs = require("fs"); -const path = require("path"); +const { resolveOpenshell } = require("../bin/lib/resolve-openshell"); describe("service environment", () => { describe("resolveOpenshell logic", () => { - // Extract and test the resolution algorithm without requiring openshell installed - - function resolveOpenshellTestable(opts = {}) { - const commandVResult = opts.commandVResult; // string or null (throws) - const existingPaths = opts.existingPaths || []; - const home = opts.home || "/fakehome"; - - // Step 1: command -v result - if (commandVResult && commandVResult.startsWith("/")) { - return commandVResult; - } - - // Step 2: fallback candidates - const candidates = [ - `${home}/.local/bin/openshell`, - "/usr/local/bin/openshell", - "/usr/bin/openshell", - ]; - for (const p of candidates) { - if (existingPaths.includes(p)) return p; - } - - return null; // not found - } - it("returns command -v result when absolute path", () => { assert.equal( - resolveOpenshellTestable({ commandVResult: "/usr/bin/openshell" }), + resolveOpenshell({ commandVResult: "/usr/bin/openshell" }), "/usr/bin/openshell" ); }); it("rejects non-absolute command -v result (alias)", () => { assert.equal( - resolveOpenshellTestable({ commandVResult: "openshell" }), + resolveOpenshell({ commandVResult: "openshell", checkExecutable: () => false }), null ); }); it("rejects alias definition from command -v", () => { assert.equal( - resolveOpenshellTestable({ commandVResult: "alias openshell='echo pwned'" }), + resolveOpenshell({ commandVResult: "alias openshell='echo pwned'", checkExecutable: () => false }), null ); }); it("falls back to ~/.local/bin when command -v fails", () => { assert.equal( - resolveOpenshellTestable({ + resolveOpenshell({ commandVResult: null, - existingPaths: ["/fakehome/.local/bin/openshell"], + checkExecutable: (p) => p === "/fakehome/.local/bin/openshell", home: "/fakehome", }), "/fakehome/.local/bin/openshell" @@ -68,9 +42,9 @@ describe("service environment", () => { it("falls back to /usr/local/bin", () => { assert.equal( - resolveOpenshellTestable({ + resolveOpenshell({ commandVResult: null, - existingPaths: ["/usr/local/bin/openshell"], + checkExecutable: (p) => p === "/usr/local/bin/openshell", }), "/usr/local/bin/openshell" ); @@ -78,9 +52,9 @@ describe("service environment", () => { it("falls back to /usr/bin", () => { assert.equal( - resolveOpenshellTestable({ + resolveOpenshell({ commandVResult: null, - existingPaths: ["/usr/bin/openshell"], + checkExecutable: (p) => p === "/usr/bin/openshell", }), "/usr/bin/openshell" ); @@ -88,9 +62,9 @@ describe("service environment", () => { it("prefers ~/.local/bin over /usr/local/bin", () => { assert.equal( - resolveOpenshellTestable({ + resolveOpenshell({ commandVResult: null, - existingPaths: ["/fakehome/.local/bin/openshell", "/usr/local/bin/openshell"], + checkExecutable: (p) => p === "/fakehome/.local/bin/openshell" || p === "/usr/local/bin/openshell", home: "/fakehome", }), "/fakehome/.local/bin/openshell" @@ -99,9 +73,9 @@ describe("service environment", () => { it("returns null when openshell not found anywhere", () => { assert.equal( - resolveOpenshellTestable({ + resolveOpenshell({ commandVResult: null, - existingPaths: [], + checkExecutable: () => false, }), null ); @@ -110,8 +84,6 @@ describe("service environment", () => { describe("SANDBOX_NAME defaulting", () => { it("start-services.sh preserves existing SANDBOX_NAME", () => { - // Verify the bash variable expansion logic: - // SANDBOX_NAME="${NEMOCLAW_SANDBOX:-${SANDBOX_NAME:-default}}" const result = execSync( 'SANDBOX_NAME=my-box bash -c \'SANDBOX_NAME="${NEMOCLAW_SANDBOX:-${SANDBOX_NAME:-default}}" && echo $SANDBOX_NAME\'', { encoding: "utf-8" } From 689b4eeeda377040363c21f7a61ed78f6e1288f1 Mon Sep 17 00:00:00 2001 From: Brian Taylor Date: Tue, 17 Mar 2026 20:59:23 -0700 Subject: [PATCH 6/6] fix: harden HOME fallback and make SANDBOX_NAME tests hermetic Signed-off-by: Brian Taylor --- bin/lib/resolve-openshell.js | 4 ++-- test/service-env.test.js | 21 +++++++++++++++------ 2 files changed, 17 insertions(+), 8 deletions(-) diff --git a/bin/lib/resolve-openshell.js b/bin/lib/resolve-openshell.js index 3260b44e439..6e89ee1f134 100644 --- a/bin/lib/resolve-openshell.js +++ b/bin/lib/resolve-openshell.js @@ -17,7 +17,7 @@ const fs = require("fs"); * @returns {string|null} Absolute path to openshell, or null if not found */ function resolveOpenshell(opts = {}) { - const home = opts.home || process.env.HOME || "/tmp"; + const home = opts.home ?? process.env.HOME; // Step 1: command -v if (opts.commandVResult === undefined) { @@ -35,7 +35,7 @@ function resolveOpenshell(opts = {}) { }); const candidates = [ - `${home}/.local/bin/openshell`, + ...(home && home.startsWith("/") ? [`${home}/.local/bin/openshell`] : []), "/usr/local/bin/openshell", "/usr/bin/openshell", ]; diff --git a/test/service-env.test.js b/test/service-env.test.js index a8616fddc49..cd822085dc6 100644 --- a/test/service-env.test.js +++ b/test/service-env.test.js @@ -85,24 +85,33 @@ describe("service environment", () => { describe("SANDBOX_NAME defaulting", () => { it("start-services.sh preserves existing SANDBOX_NAME", () => { const result = execSync( - 'SANDBOX_NAME=my-box bash -c \'SANDBOX_NAME="${NEMOCLAW_SANDBOX:-${SANDBOX_NAME:-default}}" && echo $SANDBOX_NAME\'', - { encoding: "utf-8" } + 'bash -c \'SANDBOX_NAME="${NEMOCLAW_SANDBOX:-${SANDBOX_NAME:-default}}"; export SANDBOX_NAME; bash -c "echo \\$SANDBOX_NAME"\'', + { + encoding: "utf-8", + env: { ...process.env, NEMOCLAW_SANDBOX: "", SANDBOX_NAME: "my-box" }, + } ).trim(); assert.equal(result, "my-box"); }); it("start-services.sh uses NEMOCLAW_SANDBOX over SANDBOX_NAME", () => { const result = execSync( - 'NEMOCLAW_SANDBOX=from-env SANDBOX_NAME=old bash -c \'SANDBOX_NAME="${NEMOCLAW_SANDBOX:-${SANDBOX_NAME:-default}}" && echo $SANDBOX_NAME\'', - { encoding: "utf-8" } + 'bash -c \'SANDBOX_NAME="${NEMOCLAW_SANDBOX:-${SANDBOX_NAME:-default}}"; export SANDBOX_NAME; bash -c "echo \\$SANDBOX_NAME"\'', + { + encoding: "utf-8", + env: { ...process.env, NEMOCLAW_SANDBOX: "from-env", SANDBOX_NAME: "old" }, + } ).trim(); assert.equal(result, "from-env"); }); it("start-services.sh falls back to default when both unset", () => { const result = execSync( - 'bash -c \'SANDBOX_NAME="${NEMOCLAW_SANDBOX:-${SANDBOX_NAME:-default}}" && echo $SANDBOX_NAME\'', - { encoding: "utf-8" } + 'bash -c \'SANDBOX_NAME="${NEMOCLAW_SANDBOX:-${SANDBOX_NAME:-default}}"; export SANDBOX_NAME; bash -c "echo \\$SANDBOX_NAME"\'', + { + encoding: "utf-8", + env: { ...process.env, NEMOCLAW_SANDBOX: "", SANDBOX_NAME: "" }, + } ).trim(); assert.equal(result, "default"); });