fix: resolve sandbox name from registry in Telegram bridge#96
fix: resolve sandbox name from registry in Telegram bridge#96WuKongAI-CMU wants to merge 3 commits intoNVIDIA:mainfrom
Conversation
|
Once merged in, do we have to recreate our sandbox again? |
merged it in manually, restarted with |
|
Thanks @maximgeerinck. Hopefully the PR will be merged into NVIDIA:main as I suspect nearly all users are facing the issue |
|
Yes! Replacing my telegram-bridge.ts with this: and stopping & restarting NemoClaw worked for me. Location: /lib/node_modules/nemoclaw/scripts Thanks @WuKongAI-CMU |
WuKongAI-CMU
force-pushed
the
fix/telegram-sandbox-name
branch
from
d7bc761 to
9d093dc
Compare
|
Rebased this branch onto the current I also added a regression test for the sandbox-name resolution behavior in
Community validation on this PR is still pointing the same way: multiple users reported that replacing |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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 selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughReplace hardcoded sandbox derivation in Changes
Sequence Diagram(s)sequenceDiagram
participant Script as scripts/telegram-bridge.js
participant Env as process.env
participant Registry as ../bin/lib/registry
Script->>Env: check SANDBOX_NAME
alt SANDBOX_NAME set
Env-->>Script: return SANDBOX_NAME
else SANDBOX_NAME unset
Script->>Registry: require() and call getDefault()
alt registry returns default
Registry-->>Script: return registryDefault
else registry missing or throws
Script-->>Script: use "my-assistant" fallback
end
end
Script-->>Script: set SANDBOX and continue (SSH target/banner usage)
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 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)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/telegram-bridge.js (1)
15-15:⚠️ Potential issue | 🟡 MinorDocumentation inconsistency: comment still references old default.
The comment states
(default: nemoclaw)but the new default fallback is"my-assistant". This should be updated to avoid confusion.📝 Proposed fix
-* SANDBOX_NAME — sandbox name (default: nemoclaw) +* SANDBOX_NAME — sandbox name (default: from registry, or "my-assistant")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/telegram-bridge.js` at line 15, Update the inline documentation for SANDBOX_NAME to reflect the new default value ("my-assistant") so the comment matches the actual fallback; locate the comment mentioning SANDBOX_NAME in scripts/telegram-bridge.js and change the text "(default: nemoclaw)" to "(default: my-assistant)".
🧹 Nitpick comments (1)
test/telegram-bridge-sandbox-name.test.js (1)
12-33: Source-based assertions are brittle but acceptable for regression testing.These tests will break if code formatting changes (e.g., switching to single quotes, adding whitespace). Consider this a tradeoff: the tests catch reintroduction of the bug but require maintenance if the code is reformatted.
For more robust coverage, you could add a unit test that actually invokes
resolveSandboxName()with mockedprocess.envand registry module. However, the current approach is sufficient as a regression guard.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/telegram-bridge-sandbox-name.test.js` around lines 12 - 33, The current source-string-based tests are brittle; add a more robust unit test that directly exercises the resolveSandboxName function by mocking process.env.SANDBOX_NAME and the registry module to assert three behaviors: when process.env.SANDBOX_NAME is set resolveSandboxName returns that value, when env is unset but registry.getDefault() returns a value resolveSandboxName returns the registry value, and when both are absent resolveSandboxName returns "my-assistant"; locate the function named resolveSandboxName and the symbols process.env.SANDBOX_NAME and registry.getDefault to implement these mocks and assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@scripts/telegram-bridge.js`:
- Line 15: Update the inline documentation for SANDBOX_NAME to reflect the new
default value ("my-assistant") so the comment matches the actual fallback;
locate the comment mentioning SANDBOX_NAME in scripts/telegram-bridge.js and
change the text "(default: nemoclaw)" to "(default: my-assistant)".
---
Nitpick comments:
In `@test/telegram-bridge-sandbox-name.test.js`:
- Around line 12-33: The current source-string-based tests are brittle; add a
more robust unit test that directly exercises the resolveSandboxName function by
mocking process.env.SANDBOX_NAME and the registry module to assert three
behaviors: when process.env.SANDBOX_NAME is set resolveSandboxName returns that
value, when env is unset but registry.getDefault() returns a value
resolveSandboxName returns the registry value, and when both are absent
resolveSandboxName returns "my-assistant"; locate the function named
resolveSandboxName and the symbols process.env.SANDBOX_NAME and
registry.getDefault to implement these mocks and assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f17361fd-72f7-4a03-9047-3f2fb33575df
📒 Files selected for processing (2)
scripts/telegram-bridge.jstest/telegram-bridge-sandbox-name.test.js
wscurran
added
bug
WuKongAI-CMU
force-pushed
the
fix/telegram-sandbox-name
branch
from
c5cfbf1 to
f5c4a95
Compare
|
Rebased onto latest main. Resolved the conflict with the upstream Also updated the doc comment to reflect the actual default ( Community validation still holds — multiple users confirmed this patch fixes Telegram bridge "sandbox not found" errors. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/telegram-bridge-sandbox-name.test.js (1)
11-11: Regex extraction is fragile and style-dependent.The pattern
[\s\S]*?\n\}relies on the function's closing brace being at column 0 (no indentation). If the source file is reformatted (e.g., wrapped in a module, changed indentation), this regex will silently capture an incomplete function body, causing misleading test failures.Consider exporting
resolveSandboxNamefor direct testing, or using a more robust extraction approach (e.g., matching balanced braces or an AST parser likeacorn).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/telegram-bridge-sandbox-name.test.js` at line 11, The test's regex that builds RESOLVE_SANDBOX_NAME_SOURCE from SCRIPT using /function resolveSandboxName\(\) \{[\s\S]*?\n\}/ is fragile; update the code to either export resolveSandboxName from the module and import it directly in the test (preferred) or replace the brittle extraction with a robust parser approach (e.g., use an AST parser like acorn to locate the FunctionDeclaration named "resolveSandboxName" or implement a balanced-brace extractor that starts at "function resolveSandboxName()" and reads until the matching closing brace) so the test reliably obtains the full function body regardless of formatting changes to SCRIPT.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/telegram-bridge-sandbox-name.test.js`:
- Line 11: The test's regex that builds RESOLVE_SANDBOX_NAME_SOURCE from SCRIPT
using /function resolveSandboxName\(\) \{[\s\S]*?\n\}/ is fragile; update the
code to either export resolveSandboxName from the module and import it directly
in the test (preferred) or replace the brittle extraction with a robust parser
approach (e.g., use an AST parser like acorn to locate the FunctionDeclaration
named "resolveSandboxName" or implement a balanced-brace extractor that starts
at "function resolveSandboxName()" and reads until the matching closing brace)
so the test reliably obtains the full function body regardless of formatting
changes to SCRIPT.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 616fa3d6-5a42-4cc2-a550-7b00c493d9d1
📒 Files selected for processing (2)
scripts/telegram-bridge.jstest/telegram-bridge-sandbox-name.test.js
✅ Files skipped from review due to trivial changes (1)
- scripts/telegram-bridge.js
|
Ｔｈａｎｋｓ ｆｏｒ ｙｏｕｒ ａｄｖｉｓｅ。 |
|
Addressed the remaining test fragility nit in b453c17. The sandbox-resolution regression test now uses a small balanced-brace extractor instead of the style-dependent regex, so it keeps exercising the real Verification: |
The Telegram bridge hardcoded the sandbox name default to "nemoclaw" while the onboard wizard defaults to "my-assistant", causing "sandbox not found" errors when the bridge tries to connect. Now resolves the sandbox name via: 1. SANDBOX_NAME env var (explicit override) 2. Default sandbox from the NemoClaw registry 3. Fallback to "my-assistant" (matches onboard default) Also quotes the sandbox name in the ssh-config shell command. Fixes NVIDIA#93 Signed-off-by: peteryuqin <peter.yuqin@gmail.com>
WuKongAI-CMU
force-pushed
the
fix/telegram-sandbox-name
branch
from
b453c17 to
c67c71f
Compare
|
Latest branch state is already rebased and the remaining CodeRabbit nit was addressed in b453c17. Triggering a fresh pass so the review state reflects the current branch. @coderabbitai review |
|
✅ Actions performedReview triggered.
|
|
Closing this to free a contributor slot for a fresher issue; can reopen later if needed. |
…DIA#97) * refactor(policy): consolidate duplicated YAML struct hierarchies into navigator-policy Closes NVIDIA#96 Merge the Deserialize-only input structs and Serialize-only output structs into a single set of types in navigator-policy that derive both Serialize and Deserialize. This eliminates the duplicate PolicyYaml hierarchy in navigator-cli and fixes three round-trip issues: - filesystem_policy vs filesystem field name mismatch - allowed_ips silently dropped on proto-to-YAML conversion - network policy name field silently dropped on proto-to-YAML conversion Also adds api_patterns support to the inference YAML schema and switches network_policies from HashMap to BTreeMap for deterministic output ordering. * fix(e2e): update non-CONNECT test assertion from 405 to 403 Align test_l4_non_connect_method_rejected with the proxy behavior change in c06117e which intentionally returns 403 for non-CONNECT requests. --------- Co-authored-by: John Myers <johntmyers@users.noreply.github.com>
6 participants
Summary
"nemoclaw"while onboard creates"my-assistant"by default, which leads to "sandbox not found" failuresSANDBOX_NAMEstill supported as an explicit override"my-assistant")resolveOpenshell()path handling from currentmainscripts/telegram-bridge.jsFixes #93
Root cause
The Telegram bridge assumed a default sandbox name of
nemoclaw, but the onboarding flow createsmy-assistantunless the user picks another name. On a default install, the bridge would therefore target the wrong sandbox.Test plan
node --test test/telegram-bridge-sandbox-name.test.jsnpm testSummary by CodeRabbit
New Features
Tests