fix(telegram): read model from sandbox registry instead of hardcoding#379
Open
futhgar wants to merge 5 commits intoNVIDIA:mainfrom
Open
fix(telegram): read model from sandbox registry instead of hardcoding#379futhgar wants to merge 5 commits intoNVIDIA:mainfrom
futhgar wants to merge 5 commits intoNVIDIA:mainfrom
Conversation
Contributor
📝 WalkthroughWalkthroughThe telegram bridge now queries Changes
Sequence Diagram(s)sequenceDiagram
participant User as Telegram User
participant Bridge as telegram-bridge.js
participant Registry as bin/lib/registry
participant Console as Startup Console
User->>Bridge: sends /start
Bridge->>Registry: getSandbox(SANDBOX)
Registry-->>Bridge: { provider, model }
Bridge->>Console: build banner using provider/model
Bridge-->>User: send welcome message with provider/model
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/telegram-bridge-banner.test.js`:
- Around line 11-31: Add a behavioral test that mocks the registry lookup to
return a non-default model (e.g., "ollama/llama3"), executes the telegram-bridge
script, and asserts the rendered "Model:" banner line contains that model;
specifically, stub the module imported by telegram-bridge (the one matched by
require(...registry...)) so its getSandbox returns "ollama/llama3", then
re-require or invoke the banner generation and reuse the existing bannerLines
extraction to assert one of those lines includes "ollama/llama3" rather than the
default model.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0e402916-45e0-4e4f-b8b3-2329b905dcc7
📒 Files selected for processing (2)
scripts/telegram-bridge.jstest/telegram-bridge-banner.test.js
wscurran
added
bug
Contributor
|
This is a good narrow improvement, but I don’t think it fully closes #24 yet. The banner model line now reads from the sandbox registry, which fixes part of the hardcoded Concretely:
I’d be happy to re-review if you extend this so the Telegram bridge consistently reflects the |
Author
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/telegram-bridge-banner.test.js (1)
37-39: Make temp-dir cleanup unconditional.If assertions fail before Line 97/181, temporary directories are left behind. Use
try/finally(ornode:testteardown hooks) so cleanup always runs.Minimal pattern
- const tmp = fs.mkdtempSync(...); - ... - fs.rmSync(tmp, { recursive: true, force: true }); + const tmp = fs.mkdtempSync(...); + try { + ... + } finally { + fs.rmSync(tmp, { recursive: true, force: true }); + }Also applies to: 96-98, 131-133, 181-181
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/telegram-bridge-banner.test.js` around lines 37 - 39, Wrap the temporary-directory setup and usage (the tmp created by fs.mkdtempSync and registryDir) in a try/finally so cleanup always runs even if assertions fail: move fs.mkdtempSync(path.join(os.tmpdir(), "...")) and fs.mkdirSync(registryDir, { recursive: true }) into the test scope, run the test logic in the try block, and call fs.rmSync(tmp, { recursive: true, force: true }) (or appropriate rmdir/cleanup) in the finally; apply the same pattern to the other temp-dir occurrences around the blocks creating tmp/registryDir so all temporary dirs are removed unconditionally.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/telegram-bridge-banner.test.js`:
- Around line 63-94: The test currently reconstructs the banner via a node "-e"
inline snippet (uses spawnSync and registry.getSandbox) instead of running the
real bridge output, so update the test to invoke the actual bridge code: replace
the spawnSync invocation that passes "-e" and inline code with a spawnSync call
that executes the real script (e.g., "node scripts/telegram-bridge.js") or
require and call the bridge's exported function (if available) so the assertions
exercise the true output; make the same change for the second occurrence that
mirrors this pattern.
---
Nitpick comments:
In `@test/telegram-bridge-banner.test.js`:
- Around line 37-39: Wrap the temporary-directory setup and usage (the tmp
created by fs.mkdtempSync and registryDir) in a try/finally so cleanup always
runs even if assertions fail: move fs.mkdtempSync(path.join(os.tmpdir(), "..."))
and fs.mkdirSync(registryDir, { recursive: true }) into the test scope, run the
test logic in the try block, and call fs.rmSync(tmp, { recursive: true, force:
true }) (or appropriate rmdir/cleanup) in the finally; apply the same pattern to
the other temp-dir occurrences around the blocks creating tmp/registryDir so all
temporary dirs are removed unconditionally.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: fd147081-87c6-431f-8744-863446d246be
📒 Files selected for processing (1)
test/telegram-bridge-banner.test.js
2 tasks
futhgar
force-pushed
the
fix/telegram-bridge-hardcoded-model
branch
from
ea99692 to
65c5cd2
Compare
mafueee
pushed a commit
to mafueee/NemoClaw
that referenced
this pull request
Mar 28, 2026
Author
|
@coderabbitai resume |
Contributor
✅ Actions performedReviews resumed. |
The Telegram bridge banner always showed "nvidia/nemotron-3-super-120b-a12b" regardless of the actual configured inference provider. Users running Ollama or other providers saw a misleading model identity. Read the model from the host-side sandbox registry (~/.nemoclaw/sandboxes.json) which is populated by nemoclaw onboard with the actual provider and model selection. Fall back to the default model only if the registry entry is missing. Fixes NVIDIA#24 Signed-off-by: Josue Gomez <josue@guatulab.com> Signed-off-by: futhgar <jmaldonado.rosa@gmail.com>
Address CodeRabbit review: add a test that sets up a registry with a non-default model (ollama/llama3), runs the registry lookup with HOME pointed at the temp dir, and verifies the banner output contains the registry model instead of the hardcoded default. Signed-off-by: Josue Gomez <josue@guatulab.com> Signed-off-by: futhgar <jmaldonado.rosa@gmail.com>
…banner Address kjw3 review: the /start Telegram response still hardcoded "Nemotron 3 Super 120B". Now reads both model and provider from the sandbox registry for both the /start message and the startup banner. Consistent identity across all user-facing output. Signed-off-by: Josue Gomez <josue@guatulab.com> Signed-off-by: futhgar <jmaldonado.rosa@gmail.com>
Add structural test verifying the /start handler has no hardcoded model string, and a behavioral test confirming it reflects configured provider and model from the sandbox registry. Signed-off-by: Josue Gomez <josue@guatulab.com> Signed-off-by: futhgar <jmaldonado.rosa@gmail.com>
Address CodeRabbit review: - Wrap all temp dir tests in try/finally for guaranteed cleanup - Extract createTestRegistry() helper to reduce duplication - Add comment explaining why tests use registry.getSandbox() directly rather than the full bridge script (requires Telegram token + openshell) Signed-off-by: Josue Gomez <josue@guatulab.com> Signed-off-by: futhgar <jmaldonado.rosa@gmail.com>
futhgar
force-pushed
the
fix/telegram-bridge-hardcoded-model
branch
from
65c5cd2 to
bdd8fc5
Compare
Author
|
@kjw3 thanks for the review — your feedback was addressed in the follow-up commits (pushed shortly after your comment). The current state of the PR:
Rebased onto main. Happy to adjust if anything else is needed. |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/telegram-bridge-banner.test.js (1)
4-9: Use ES module imports instead of CommonJSrequire().The coding guidelines require ES modules for test files in
test/. Convert these to ESM syntax.♻️ Proposed fix
-const { describe, it } = require("node:test"); -const assert = require("node:assert/strict"); -const fs = require("node:fs"); -const os = require("node:os"); -const path = require("node:path"); -const { spawnSync } = require("node:child_process"); +import { describe, it } from "node:test"; +import assert from "node:assert/strict"; +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; +import { fileURLToPath } from "node:url"; +import { spawnSync } from "node:child_process"; + +const __dirname = path.dirname(fileURLToPath(import.meta.url));As per coding guidelines:
test/**/*.test.js: Use ES modules (import/export) for test files in thetest/directory.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/telegram-bridge-banner.test.js` around lines 4 - 9, Replace the CommonJS require() calls with ESM imports: change "const { describe, it } = require('node:test')" to "import { describe, it } from 'node:test'", "const assert = require('node:assert/strict')" to "import assert from 'node:assert/strict'", and similarly convert "fs", "os", "path" and "{ spawnSync }" (from 'node:child_process') to ESM import statements; update any destructuring usage to match the imported bindings (describe, it, assert, fs, os, path, spawnSync) and ensure the test file is treated as an ES module by your project config.
🤖 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-banner.test.js`:
- Around line 4-9: Replace the CommonJS require() calls with ESM imports: change
"const { describe, it } = require('node:test')" to "import { describe, it } from
'node:test'", "const assert = require('node:assert/strict')" to "import assert
from 'node:assert/strict'", and similarly convert "fs", "os", "path" and "{
spawnSync }" (from 'node:child_process') to ESM import statements; update any
destructuring usage to match the imported bindings (describe, it, assert, fs,
os, path, spawnSync) and ensure the test file is treated as an ES module by your
project config.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4b86181a-ac81-4f00-a6be-723973b993ba
📒 Files selected for processing (2)
scripts/telegram-bridge.jstest/telegram-bridge-banner.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/telegram-bridge.js
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The Telegram bridge banner always displays
nvidia/nemotron-3-super-120b-a12bregardless of the actual configured inference provider. Users running Ollama, vLLM, or any non-default provider see a misleading model identity in the bridge startup output.Fix
Read the model from the host-side sandbox registry (
~/.nemoclaw/sandboxes.json) whichnemoclaw onboardpopulates with the actual provider and model selection. Fall back to the default model only if the registry entry is missing.Changes
scripts/telegram-bridge.jssandboxInfo.modelin bannertest/telegram-bridge-banner.test.jsTest plan
npm test— 14/14 related tests pass (registry + banner + CLI)Fixes #24
Summary by CodeRabbit
Bug Fixes
Tests
Signed-off-by: Josue Balandrano Coronel josuebc@pm.me