fix: propagate SANDBOX_NAME to telegram bridge and resolve openshell path

[brianwtaylor]

Summary

• start-services.sh reads NEMOCLAW_SANDBOX but doesn't export SANDBOX_NAME to the telegram bridge subprocess — sandbox name mismatch (SANDBOX_NAME vs NEMOCLAW_SANDBOX env var inconsistency causes Telegram bridge to target wrong sandbox #198)
• telegram-bridge.js calls bare openshell which isn't on PATH when launched via nohup from systemd or cron (Telegram bridge fails with "openshell: not found" because ~/.local/bin isn't on PATH for child processes #199)
• Propagate SANDBOX_NAME env var to the bridge process
• Add resolveOpenshell() that checks command -v, then ~/.local/bin, /usr/local/bin, /usr/bin
• Validate that command -v returns an absolute path (rejects aliases/functions)

Fixes #198
Fixes #199

Test plan

Automated Tests

npm test

Manual Testing

• Run NEMOCLAW_SANDBOX=my-sandbox ./scripts/start-services.sh
• Verify the telegram bridge receives the correct sandbox name
• Remove openshell from PATH and verify the bridge finds it via fallback paths
• Check bridge logs for correct sandbox name and openshell path

Hardware Validation

Path resolution logic validated on DGX Spark:

Machine	command -v openshell	Fallback paths	Expected behavior
spark-c231	Not found	None present	resolveOpenshell() exits with clear error — correct
spark1	Not found	None present	Same — correct

openshell is installed during nemoclaw onboard (typically to ~/.local/bin/), so it is expected to be absent on a pre-onboard system. The fallback chain (~/.local/bin → /usr/local/bin → /usr/bin) covers all standard installation locations. The absolute-path check (startsWith("/")) prevents alias injection in non-interactive shells.

Summary by CodeRabbit

• New Features

  • Improved detection and validation of the required shell executable with explicit failure when not found.
  • Sandbox name handling now respects existing environment variables and only injects a sandbox context when valid.
  • Telegram bridge startup passes the resolved sandbox context so the bridge runs in the correct sandbox.

• Tests

  • Added tests for sandbox name expansion, environment override behavior, and shell-executable resolution and precedence.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: aea1b362-7b96-49ce-80af-d7b985a5de78

📥 Commits

Reviewing files that changed from the base of the PR and between 8be5329 and 689b4ee.

📒 Files selected for processing (2)

• bin/lib/resolve-openshell.js
• test/service-env.test.js

---

📝 Walkthrough

Walkthrough

start now reads registry.listSandboxes() to obtain a default sandbox and conditionally injects SANDBOX_NAME when spawning start-services.sh. start-services.sh prefers an existing SANDBOX_NAME or falls back to "default" and forwards it to service invocations. telegram-bridge resolves the openshell binary path (or exits) instead of relying on PATH. Tests and a resolve-openshell utility were added.

Changes

Cohort / File(s)	Summary
Startup / sandbox propagation bin/nemoclaw.js, scripts/start-services.sh	bin/nemoclaw.js reads registry.listSandboxes() to choose a default sandbox and conditionally prefixes SANDBOX_NAME when spawning start-services.sh. start-services.sh defers to an existing SANDBOX_NAME (or "default") and passes it into service starts (Telegram bridge).
OpenShell resolution & bridge bin/lib/resolve-openshell.js, scripts/telegram-bridge.js	New resolveOpenshell() utility returns an absolute openshell path (with test hooks); telegram-bridge.js initializes OPENSHELL via that resolver, exits if unresolved, and invokes openshell via the resolved absolute path.
Tests & manifest test/service-env.test.js, package.json	Adds tests for openshell resolution fallbacks and SANDBOX_NAME / NEMOCLAW_SANDBOX expansion/override behaviors; small package.json manifest updates.

Sequence Diagram(s)

sequenceDiagram
    participant User as User
    participant NM as nemoclaw.js
    participant Start as start-services.sh
    participant Bridge as telegram-bridge.js
    participant Open as openshell

    User->>NM: npm start
    NM->>NM: registry.listSandboxes()
    NM->>Start: spawn start-services.sh (prefix SANDBOX_NAME if valid)
    Start->>Start: SANDBOX_NAME = ${SANDBOX_NAME:-default}
    Start->>Bridge: start telegram-bridge with SANDBOX_NAME env
    Bridge->>Bridge: OPENSHELL = resolveOpenshell()
    alt OPENSHELL found
        Bridge->>Open: exec OPENSHELL sandbox ssh-config ...
        Open-->>Bridge: ssh-config output
    else not found
        Bridge-->>Bridge: exit with error
    end

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

🐰 I hopped through PATHs both near and far,
I found the shell beneath the star.
Sandbox names now tag along,
Start to bridge — the links are strong.
🥕

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 50.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 accurately captures both main changes: propagating SANDBOX_NAME to the telegram bridge and resolving the openshell path, matching the core issues being fixed.
Linked Issues check	✅ Passed	The pull request addresses all coding requirements from both linked issues: propagates SANDBOX_NAME to telegram bridge subprocess [#198], implements resolveOpenshell with absolute path validation and fallback paths [#199].
Out of Scope Changes check	✅ Passed	All changes are directly scoped to fixing #198 and #199: SANDBOX_NAME propagation, openshell resolution, related test coverage, and manifest updates—no unrelated modifications detected.

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

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

📝 Coding Plan

• Generate coding plan for human review comments

---

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

[coderabbitai]

🧹 Nitpick comments (3)

test/service-env.test.js (2)

7-8: Remove unused imports.

fs and path are imported but not used in this test file.

🧹 Proposed fix

 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");

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/service-env.test.js` around lines 7 - 8, Remove the unused imports by
deleting the require statements for fs and path (the top-level const fs =
require("fs"); and const path = require("path");) from the test file so there
are no unused variables; run the tests/linter to confirm no references remain
and commit the cleanup.

---

14-35: Good test double, but consider testing the actual production function directly.

The resolveOpenshellTestable function mirrors the production logic in telegram-bridge.js, but if the production code changes, this test double might drift out of sync. Consider either:

1. Exporting resolveOpenshell from telegram-bridge.js for direct testing, or
2. Adding an integration test that exercises the real function in a controlled environment.

That said, the current approach is pragmatic given that importing telegram-bridge.js causes immediate side effects (module-level resolveOpenshell() call and potential process.exit).

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/service-env.test.js` around lines 14 - 35, The test defines a local
duplicate resolveOpenshellTestable that can drift from the real resolveOpenshell
in telegram-bridge.js; instead either export the real resolveOpenshell from
telegram-bridge.js and update tests to call that exported function, or add an
integration test that spawns/loading telegram-bridge.js in a controlled
environment (mocking environment variables, PATH and filesystem and
capturing/neutralizing the module-level resolveOpenshell() call and any
process.exit) so the production logic is exercised directly; reference the
function name resolveOpenshell (and the test double resolveOpenshellTestable)
when making the change.

bin/nemoclaw.js (1)

137-139: Consider sanitizing or validating the sandbox name before shell interpolation.

If defaultSandbox contains shell metacharacters (e.g., spaces, quotes, $, backticks), the constructed command could break or be exploited. While sandbox names are typically controlled, defensive quoting or validation would be safer.

♻️ Suggested improvement

 async function start() {
   await ensureApiKey();
   const { defaultSandbox } = registry.listSandboxes();
-  const sandboxEnv = defaultSandbox ? `SANDBOX_NAME="${defaultSandbox}"` : "";
+  // Validate sandbox name contains only safe characters
+  const safeName = defaultSandbox && /^[a-zA-Z0-9_-]+$/.test(defaultSandbox) ? defaultSandbox : null;
+  const sandboxEnv = safeName ? `SANDBOX_NAME="${safeName}"` : "";
   run(`${sandboxEnv} bash "${SCRIPTS}/start-services.sh"`);
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@bin/nemoclaw.js` around lines 137 - 139, The command construction uses
defaultSandbox from registry.listSandboxes() and interpolates it into sandboxEnv
passed to run(), which can break or be exploited if it contains shell
metacharacters; before building sandboxEnv and calling run(`${sandboxEnv} bash
"${SCRIPTS}/start-services.sh"`), validate or sanitize defaultSandbox (e.g.,
allow only a safe pattern like [A-Za-z0-9._-], reject or normalize others) or
perform proper shell-escaping/quoting of the value so it cannot inject
additional commands; update the code around defaultSandbox, sandboxEnv and the
run() invocation to use the validated/escaped value.

🤖 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 137-139: The command construction uses defaultSandbox from
registry.listSandboxes() and interpolates it into sandboxEnv passed to run(),
which can break or be exploited if it contains shell metacharacters; before
building sandboxEnv and calling run(`${sandboxEnv} bash
"${SCRIPTS}/start-services.sh"`), validate or sanitize defaultSandbox (e.g.,
allow only a safe pattern like [A-Za-z0-9._-], reject or normalize others) or
perform proper shell-escaping/quoting of the value so it cannot inject
additional commands; update the code around defaultSandbox, sandboxEnv and the
run() invocation to use the validated/escaped value.

In `@test/service-env.test.js`:
- Around line 7-8: Remove the unused imports by deleting the require statements
for fs and path (the top-level const fs = require("fs"); and const path =
require("path");) from the test file so there are no unused variables; run the
tests/linter to confirm no references remain and commit the cleanup.
- Around line 14-35: The test defines a local duplicate resolveOpenshellTestable
that can drift from the real resolveOpenshell in telegram-bridge.js; instead
either export the real resolveOpenshell from telegram-bridge.js and update tests
to call that exported function, or add an integration test that spawns/loading
telegram-bridge.js in a controlled environment (mocking environment variables,
PATH and filesystem and capturing/neutralizing the module-level
resolveOpenshell() call and any process.exit) so the production logic is
exercised directly; reference the function name resolveOpenshell (and the test
double resolveOpenshellTestable) when making the change.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 92beba4d-c565-4a01-b983-a0f8b2d4b9cd

📥 Commits

Reviewing files that changed from the base of the PR and between 2a9afbc and 78b31be.

📒 Files selected for processing (4)

• bin/nemoclaw.js
• scripts/start-services.sh
• scripts/telegram-bridge.js
• test/service-env.test.js

[coderabbitai]

Actionable comments posted: 2

🤖 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/lib/resolve-openshell.js`:
- Around line 20-41: The code sets home = opts.home || process.env.HOME ||
"/tmp", which can make the candidates list use /tmp and allow path-hijack;
change the logic in resolve-openshell to avoid falling back to "/tmp": prefer
opts.home, then require('os').homedir() / process.env.HOME, and if no real home
is available omit or skip the user-local candidate
`${home}/.local/bin/openshell` when building the candidates array; update
references to the home variable and the candidates construction (and any
tests/mock that set opts.home) so the user-local path is only added when home is
a valid, non-world-writable user home.

In `@test/service-env.test.js`:
- Around line 85-108: The tests call execSync without providing an explicit env,
so they can inherit NEMOCLAW_SANDBOX or SANDBOX_NAME from the parent and are
flaky and miss verifying export to child processes; update each failing case
that invokes execSync (the three uses shown) to pass a hermetic env object
(e.g., Object.assign({}, process.env, { NEMOCLAW_SANDBOX: ..., SANDBOX_NAME: ...
}) or {} with only the vars you need) via the execSync options.env parameter,
and extend each shell command to spawn a nested subprocess to assert the
variable is exported (for example run the expansion then run a nested bash -c
'echo $SANDBOX_NAME' and assert that nested output matches expected). Ensure you
update the three execSync calls referenced and assert the nested-child output,
not just the parent-shell expansion.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: b77fde36-0603-4676-8cb7-35ef83f377f6

📥 Commits

Reviewing files that changed from the base of the PR and between 78b31be and 8be5329.

📒 Files selected for processing (4)

• bin/lib/resolve-openshell.js
• bin/nemoclaw.js
• scripts/telegram-bridge.js
• test/service-env.test.js

🚧 Files skipped from review as they are similar to previous changes (2)

• scripts/telegram-bridge.js
• bin/nemoclaw.js

[ericksoa]

LGTM — thorough fix for the env var propagation issue, plus the resolveOpenshell() hardening is a nice security bonus. Great test coverage. Thanks @brianwtaylor!