fix: standardize messaging integrations with host-side bridge framework#617
Open
fix: standardize messaging integrations with host-side bridge framework#617
Conversation
Standardize how user-provided secrets are passed into the sandbox: - Add TELEGRAM_BOT_TOKEN to sandbox creation env vars (was only in deploy path, missing from onboard) - Standardize NVIDIA_API_KEY to use getCredential() like all other tokens instead of checking process.env directly - Add inline comment documenting the credential passing pattern - Add "Credential Handling" section to architecture doc with inventory table covering all six credentials and their flow All four user tokens (NVIDIA, Discord, Slack, Telegram) now follow the same getCredential() || process.env pattern at sandbox creation. Closes #616
📝 WalkthroughWalkthroughAdds a host-side messaging bridge framework (core, runner, per-platform adapters), blueprint bridge configs, onboarding/service changes to auto-start bridges when credentials exist, documentation of credential flow, and tests covering bridge behavior and credential handling. Changes
Sequence DiagramsequenceDiagram
participant User as Messaging Platform (Telegram/Discord/Slack)
participant Bridge as Host Bridge (scripts/bridge.js)
participant Core as Bridge Core (scripts/bridge-core.js)
participant SSH as OpenShell SSH
participant Sandbox as Sandbox Agent
User->>Bridge: Incoming message
activate Bridge
Bridge->>Bridge: Log metadata & start typing timer
Bridge->>Core: runAgentInSandbox(text, sessionId)
activate Core
Core->>Core: Write temp SSH config
Core->>SSH: ssh -T -F <config> openshell-<SANDBOX> "export ... && nemoclaw-start openclaw agent ..."
activate SSH
SSH->>Sandbox: Execute agent command
activate Sandbox
Sandbox-->>SSH: Agent stdout (response)
deactivate Sandbox
SSH-->>Core: Captured output
deactivate SSH
Core->>Core: Filter output, cleanup temp files
Core-->>Bridge: Filtered response
deactivate Core
Bridge->>Bridge: Stop typing, chunk response
Bridge->>User: Send reply chunks
deactivate Bridge
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 docstrings
🧪 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.
🧹 Nitpick comments (3)
docs/reference/architecture.md (3)
126-126: Two sentences on one line.Line 126 contains two sentences, which makes diffs harder to read. Split onto separate lines.
Suggested fix
-The Telegram bridge (`scripts/telegram-bridge.js`) runs on the host and communicates with the sandbox via OpenShell SSH. This is intentional: +The Telegram bridge (`scripts/telegram-bridge.js`) runs on the host and communicates with the sandbox via OpenShell SSH. +This is intentional:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/reference/architecture.md` at line 126, The sentence "The Telegram bridge (`scripts/telegram-bridge.js`) runs on the host and communicates with the sandbox via OpenShell SSH. This is intentional:" is currently on one line; split it into two separate lines so each sentence is its own line for clearer diffs—locate the sentence referencing scripts/telegram-bridge.js and replace the single line with two lines: one containing "The Telegram bridge (`scripts/telegram-bridge.js`) runs on the host and communicates with the sandbox via OpenShell SSH." and the next containing "This is intentional:".
110-111: Passive voice detected.Lines 110 and 111 use passive constructions. Revise to active voice.
Suggestions:
- Line 110: "Passed into the sandbox..." → "The CLI passes them into the sandbox as environment variables at creation time."
- Line 111: "Never written to..." → "The system never writes them to
openclaw.json(immutable at runtime)."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/reference/architecture.md` around lines 110 - 111, Replace the passive phrases in the two bullet points so they use active voice: change "Passed into the sandbox as environment variables at creation time." to "The CLI passes them into the sandbox as environment variables at creation time." and change "Never written to `openclaw.json` (immutable at runtime)." to "The system never writes them to `openclaw.json` (immutable at runtime)." Update the lines containing those exact phrases in docs/reference/architecture.md so the document reads actively and preserves the reference to `openclaw.json`.
130-130: Passive voice detected."The
TELEGRAM_BOT_TOKENis still passed" uses passive voice.Suggestion: "NemoClaw still passes the
TELEGRAM_BOT_TOKENinto the sandbox for future OpenClaw channel plugin support."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/reference/architecture.md` at line 130, Replace the passive-voice sentence about TELEGRAM_BOT_TOKEN with an active-voice form: change "The `TELEGRAM_BOT_TOKEN` is still passed into the sandbox for future OpenClaw channel plugin support." to an active sentence such as "NemoClaw still passes the `TELEGRAM_BOT_TOKEN` into the sandbox for future OpenClaw channel plugin support," referencing the `TELEGRAM_BOT_TOKEN`, NemoClaw, sandbox, and OpenClaw channel plugin to keep meaning unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@docs/reference/architecture.md`:
- Line 126: The sentence "The Telegram bridge (`scripts/telegram-bridge.js`)
runs on the host and communicates with the sandbox via OpenShell SSH. This is
intentional:" is currently on one line; split it into two separate lines so each
sentence is its own line for clearer diffs—locate the sentence referencing
scripts/telegram-bridge.js and replace the single line with two lines: one
containing "The Telegram bridge (`scripts/telegram-bridge.js`) runs on the host
and communicates with the sandbox via OpenShell SSH." and the next containing
"This is intentional:".
- Around line 110-111: Replace the passive phrases in the two bullet points so
they use active voice: change "Passed into the sandbox as environment variables
at creation time." to "The CLI passes them into the sandbox as environment
variables at creation time." and change "Never written to `openclaw.json`
(immutable at runtime)." to "The system never writes them to `openclaw.json`
(immutable at runtime)." Update the lines containing those exact phrases in
docs/reference/architecture.md so the document reads actively and preserves the
reference to `openclaw.json`.
- Line 130: Replace the passive-voice sentence about TELEGRAM_BOT_TOKEN with an
active-voice form: change "The `TELEGRAM_BOT_TOKEN` is still passed into the
sandbox for future OpenClaw channel plugin support." to an active sentence such
as "NemoClaw still passes the `TELEGRAM_BOT_TOKEN` into the sandbox for future
OpenClaw channel plugin support," referencing the `TELEGRAM_BOT_TOKEN`,
NemoClaw, sandbox, and OpenClaw channel plugin to keep meaning unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 74600614-babe-486b-80ad-786e5a8672aa
📒 Files selected for processing (2)
bin/lib/onboard.jsdocs/reference/architecture.md
Extract shared sandbox relay logic from telegram-bridge.js into bridge-core.js, then build a Discord bridge on the same pattern. bridge-core.js handles: - OpenShell SSH config generation and temp file management - Agent invocation via SSH relay to sandbox - Response line filtering (strips setup boilerplate) - SANDBOX_NAME validation and NVIDIA_API_KEY requirement discord-bridge.js uses discord.js for WebSocket-based gateway connection. Messages relay through bridge-core the same way Telegram does — credentials stay on the host, never enter sandbox. telegram-bridge.js refactored to import from bridge-core with no behavior changes. start-services.sh updated to manage the Discord bridge service alongside Telegram when DISCORD_BOT_TOKEN is set. Closes #618
ericksoa
changed the title
ericksoa
changed the title
ericksoa
changed the title
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
scripts/telegram-bridge.js (1)
30-30: UnusedactiveSessionsMap.The
activeSessionsMap is declared but never written to or read from in the current code. If session history management was removed during refactoring, consider removing this dead code.♻️ Proposed cleanup
-const activeSessions = new Map(); // chatId → message history🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/telegram-bridge.js` at line 30, Remove the unused Map declaration "activeSessions" (const activeSessions = new Map()) from scripts/telegram-bridge.js; if session history management is required instead of deletion, wire this Map into the send/receive/handler functions (e.g., the Telegram message handler and any session cleanup logic) so messages are stored and looked up, otherwise delete the declaration and any related dead references to keep the file clean.scripts/discord-bridge.js (1)
100-100: Add error handling forclient.login().If the token is invalid or the login fails, this will result in an unhandled promise rejection. Consider adding error handling for a cleaner exit.
♻️ Proposed fix
-client.login(TOKEN); +client.login(TOKEN).catch((err) => { + console.error("Failed to login to Discord:", err.message); + process.exit(1); +});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/discord-bridge.js` at line 100, Wrap the call to client.login(TOKEN) with proper promise error handling: catch any rejection from client.login and log the error (including the error object) and exit the process with a non-zero code. Locate the client.login(TOKEN) invocation in scripts/discord-bridge.js and add a .catch or try/catch around it so failures (invalid TOKEN, network issues) are handled by printing a clear error via your logger/console and calling process.exit(1).scripts/bridge-core.js (2)
77-80: Usefs.rmSyncwithrecursiveoption for safer cleanup.
fs.rmdirSyncwill fail if the directory somehow contains additional files. Usingfs.rmSyncwith{ recursive: true, force: true }is more robust.♻️ Proposed fix
try { - fs.unlinkSync(confPath); - fs.rmdirSync(confDir); + fs.rmSync(confDir, { recursive: true, force: true }); } catch {}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/bridge-core.js` around lines 77 - 80, Replace the fragile unlinkSync + rmdirSync cleanup with fs.rmSync using the recursive/force flags: instead of calling fs.unlinkSync(confPath) and fs.rmdirSync(confDir) inside the try block, call fs.rmSync(confPath, { force: true }) (or rmSync confDir directly) and then fs.rmSync(confDir, { recursive: true, force: true }) to ensure removal even if the directory contains files; keep the existing try/catch around this logic and reference the same symbols confPath and confDir so the change is localized to that cleanup block.
114-121: RemoveAPI_KEYfrom module exports — it's internal only.The
API_KEYis populated from the environment variable and used only withinrunAgentInSandbox()(line 61). Since no external modules (telegram-bridge.js,discord-bridge.js, etc.) import it, exporting it creates an unnecessary public API surface and risks accidental misuse.♻️ Proposed fix
module.exports = { runAgentInSandbox, SANDBOX, - API_KEY, OPENSHELL, shellQuote, validateName, };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/bridge-core.js` around lines 114 - 121, Remove API_KEY from the public exports: the module-level constant API_KEY is internal to runAgentInSandbox and should not be exposed from the module.exports object. Edit the module.exports block so it exports runAgentInSandbox, SANDBOX, OPENSHELL, shellQuote, and validateName but omit API_KEY; verify runAgentInSandbox continues to use the internal API_KEY and that no other module imports API_KEY.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/bridge-core.js`:
- Around line 65-68: The spawn call that creates proc currently passes a
non-functional timeout option; remove the timeout entry from the options passed
to spawn("ssh", ["-T", "-F", confPath, `openshell-${SANDBOX}`, cmd], ...) and
implement a manual timeout: after creating proc, start a setTimeout that calls
proc.kill() (and marks/returns a timeout error) if the process hasn't exited
within the desired period, clear that timer in proc.on('exit')/proc.on('close')
handlers, and ensure you propagate the kill/timeout as an error (or reject the
surrounding promise) and handle any errors from proc.kill(); keep existing stdio
handling and preserve the proc variable and spawn invocation as the locus of the
change.
In `@scripts/discord-bridge.js`:
- Around line 31-38: The Client is missing channel partial support, so update
the discord.js import and client options: add Partials to the require/import
(alongside Client and GatewayIntentBits) and include partials:
[Partials.Channel] in the Client constructor (the symbols to change are Client,
GatewayIntentBits, and Partials and the Client({ ... }) options).
In `@scripts/start-services.sh`:
- Around line 147-151: show_status() and do_stop() were not updated for the new
Discord bridge; update show_status() to include "discord-bridge" alongside
"telegram-bridge cloudflared" when iterating services to report their status,
and update do_stop() to also call the stop routine for "discord-bridge" (same
pattern used for "telegram-bridge") so --status shows its state and --stop
terminates it; reference the service name "discord-bridge" and the helper used
to start it (start_service) to mirror existing conditional startup behavior.
---
Nitpick comments:
In `@scripts/bridge-core.js`:
- Around line 77-80: Replace the fragile unlinkSync + rmdirSync cleanup with
fs.rmSync using the recursive/force flags: instead of calling
fs.unlinkSync(confPath) and fs.rmdirSync(confDir) inside the try block, call
fs.rmSync(confPath, { force: true }) (or rmSync confDir directly) and then
fs.rmSync(confDir, { recursive: true, force: true }) to ensure removal even if
the directory contains files; keep the existing try/catch around this logic and
reference the same symbols confPath and confDir so the change is localized to
that cleanup block.
- Around line 114-121: Remove API_KEY from the public exports: the module-level
constant API_KEY is internal to runAgentInSandbox and should not be exposed from
the module.exports object. Edit the module.exports block so it exports
runAgentInSandbox, SANDBOX, OPENSHELL, shellQuote, and validateName but omit
API_KEY; verify runAgentInSandbox continues to use the internal API_KEY and that
no other module imports API_KEY.
In `@scripts/discord-bridge.js`:
- Line 100: Wrap the call to client.login(TOKEN) with proper promise error
handling: catch any rejection from client.login and log the error (including the
error object) and exit the process with a non-zero code. Locate the
client.login(TOKEN) invocation in scripts/discord-bridge.js and add a .catch or
try/catch around it so failures (invalid TOKEN, network issues) are handled by
printing a clear error via your logger/console and calling process.exit(1).
In `@scripts/telegram-bridge.js`:
- Line 30: Remove the unused Map declaration "activeSessions" (const
activeSessions = new Map()) from scripts/telegram-bridge.js; if session history
management is required instead of deletion, wire this Map into the
send/receive/handler functions (e.g., the Telegram message handler and any
session cleanup logic) so messages are stored and looked up, otherwise delete
the declaration and any related dead references to keep the file clean.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 80cb1e2e-6f4f-4e17-8f49-e44d2d61c621
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (6)
package.jsonscripts/bridge-core.jsscripts/discord-bridge.jsscripts/start-services.shscripts/telegram-bridge.jstest/runner.test.js
✅ Files skipped from review due to trivial changes (1)
- package.json
Completes the host-side bridge pattern for all three messaging integrations. Slack bridge uses @slack/bolt in Socket Mode (requires SLACK_BOT_TOKEN + SLACK_APP_TOKEN). Replies in threads, chunks long responses, and follows the same bridge-core relay pattern as Telegram and Discord.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
scripts/slack-bridge.js (2)
72-87: Add top-level startup error handling.Wrap the startup IIFE with a terminal
.catch(...)so failedapp.start()exits cleanly with a clear error.Suggested patch
-(async () => { +(async () => { await app.start(); @@ console.log(" └─────────────────────────────────────────────────────┘"); console.log(""); -})(); +})().catch((err) => { + const errorMessage = err && err.message ? err.message : String(err); + console.error(`Slack bridge failed to start: ${errorMessage}`); + process.exit(1); +});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/slack-bridge.js` around lines 72 - 87, The top-level async IIFE that calls await app.start() lacks error handling; modify the IIFE invocation so it attaches a .catch handler that logs the error (including error.stack or message) and exits the process with a non-zero code (e.g., process.exit(1)), ensuring failures in app.start() produce a clear error and clean exit; locate the async IIFE that calls app.start() and add the terminal .catch(...) handling there.
28-30: Harden allowlist parsing against empty channel IDs.If
ALLOWED_CHANNEL_IDScontains trailing commas or blank entries,[""]can be produced and cause confusing filtering behavior. Trim + filter empties up front.Suggested patch
-const ALLOWED_CHANNELS = process.env.ALLOWED_CHANNEL_IDS - ? process.env.ALLOWED_CHANNEL_IDS.split(",").map((s) => s.trim()) - : null; +const ALLOWED_CHANNELS = process.env.ALLOWED_CHANNEL_IDS + ? process.env.ALLOWED_CHANNEL_IDS + .split(",") + .map((s) => s.trim()) + .filter(Boolean) + : null;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/slack-bridge.js` around lines 28 - 30, ALLOWED_CHANNELS parsing can produce empty strings when ALLOWED_CHANNEL_IDS has trailing commas or blanks; update the initialization that reads process.env.ALLOWED_CHANNEL_IDS so after splitting and trimming you also filter out empty entries (e.g., .split(",").map(s => s.trim()).filter(s => s.length > 0) or .filter(Boolean)) to ensure ALLOWED_CHANNELS contains only non-empty channel IDs (keep returning null when process.env.ALLOWED_CHANNEL_IDS is falsy).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/slack-bridge.js`:
- Around line 50-55: The current console logs expose raw user and model content
via the variables message.text and response in the slack bridge; update the
logging around userName/message and runAgentInSandbox(response) so you never
write full text to host logs — instead log metadata only: channelId,
message.user (or userName), message text length, response length, and duration
of runAgentInSandbox, or a safely redacted preview (e.g., first N chars replaced
with ****) if a preview is required; update the logging calls near the userName
assignment and after await runAgentInSandbox(...) to use these metadata fields
and remove direct message.text/response output.
- Around line 65-67: The catch block that calls say({ text: `Error:
${err.message}`, thread_ts: message.ts }) should defensively normalize any
thrown value before composing the reply: detect if err is an instance of Error
and use err.message (and err.stack if present), otherwise serialize the value
with a safe serializer (e.g., String(err) or util.inspect) and include that in
the text; update the catch around say in scripts/slack-bridge.js to produce a
reliable, non-empty error string for all thrown values while preserving
thread_ts.
---
Nitpick comments:
In `@scripts/slack-bridge.js`:
- Around line 72-87: The top-level async IIFE that calls await app.start() lacks
error handling; modify the IIFE invocation so it attaches a .catch handler that
logs the error (including error.stack or message) and exits the process with a
non-zero code (e.g., process.exit(1)), ensuring failures in app.start() produce
a clear error and clean exit; locate the async IIFE that calls app.start() and
add the terminal .catch(...) handling there.
- Around line 28-30: ALLOWED_CHANNELS parsing can produce empty strings when
ALLOWED_CHANNEL_IDS has trailing commas or blanks; update the initialization
that reads process.env.ALLOWED_CHANNEL_IDS so after splitting and trimming you
also filter out empty entries (e.g., .split(",").map(s => s.trim()).filter(s =>
s.length > 0) or .filter(Boolean)) to ensure ALLOWED_CHANNELS contains only
non-empty channel IDs (keep returning null when process.env.ALLOWED_CHANNEL_IDS
is falsy).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2d851174-0418-4a35-815f-6853ba478892
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (4)
package.jsonscripts/slack-bridge.jsscripts/start-services.shtest/runner.test.js
✅ Files skipped from review due to trivial changes (1)
- package.json
🚧 Files skipped from review as they are similar to previous changes (2)
- scripts/start-services.sh
- test/runner.test.js
scripts/slack-bridge.js
Outdated
Comment on lines
+50
to
+55
| const userName = message.user || "someone"; | ||
| console.log(`[${channelId}] ${userName}: ${message.text}`); | ||
|
|
||
| try { | ||
| const response = await runAgentInSandbox(message.text, `sl-${channelId}`); | ||
| console.log(`[${channelId}] agent: ${response.slice(0, 100)}...`); |
There was a problem hiding this comment.
Avoid logging raw user/model content to host logs.
Line 51 and Line 55 currently log message text/response content, which can leak sensitive data (PII/secrets) into logs. Prefer metadata-only logging (IDs, lengths, timings) or aggressively redacted previews.
Suggested patch
- console.log(`[${channelId}] ${userName}: ${message.text}`);
+ console.log(
+ `[${channelId}] ${userName}: inbound message (len=${message.text.length})`
+ );
@@
- console.log(`[${channelId}] agent: ${response.slice(0, 100)}...`);
+ console.log(`[${channelId}] agent response (len=${response.length})`);📝 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
| const userName = message.user || "someone"; | |
| console.log(`[${channelId}] ${userName}: ${message.text}`); | |
| try { | |
| const response = await runAgentInSandbox(message.text, `sl-${channelId}`); | |
| console.log(`[${channelId}] agent: ${response.slice(0, 100)}...`); | |
| const userName = message.user || "someone"; | |
| console.log( | |
| `[${channelId}] ${userName}: inbound message (len=${message.text.length})` | |
| ); | |
| try { | |
| const response = await runAgentInSandbox(message.text, `sl-${channelId}`); | |
| console.log(`[${channelId}] agent response (len=${response.length})`); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/slack-bridge.js` around lines 50 - 55, The current console logs
expose raw user and model content via the variables message.text and response in
the slack bridge; update the logging around userName/message and
runAgentInSandbox(response) so you never write full text to host logs — instead
log metadata only: channelId, message.user (or userName), message text length,
response length, and duration of runAgentInSandbox, or a safely redacted preview
(e.g., first N chars replaced with ****) if a preview is required; update the
logging calls near the userName assignment and after await
runAgentInSandbox(...) to use these metadata fields and remove direct
message.text/response output.
Contributor
Author
There was a problem hiding this comment.
Acknowledged — keeping content logging for now (matches existing Telegram bridge pattern) but will revisit as a separate logging hygiene pass.
There was a problem hiding this comment.
@ericksoa, understood — keeping it consistent with the Telegram bridge pattern for now is a reasonable call. Would you like me to open a follow-up GitHub issue to track the logging hygiene pass (covering both slack-bridge.js and telegram-bridge.js) so it doesn't fall through the cracks?
（´･_･`）
Replace three standalone bridge scripts with a generic bridge runner that reads YAML configs from nemoclaw-blueprint/bridges/ and loads thin platform adapters. Structure: - nemoclaw-blueprint/bridges/messaging/*.yaml — declarative config (token env, session prefix, chunk size, adapter name) - scripts/adapters/messaging/*.js — thin SDK wrappers that implement connect + send/receive for each platform - scripts/bridge.js — generic runner that reads YAML, loads adapter, and drives the shared message flow (typing → agent → chunk → reply) Adding a new messaging platform is now a YAML file + small adapter. The bridge type system (messaging/) is extensible to future bridge types (e.g., MCP bridges).
- Replace no-op spawn() timeout with manual setTimeout + proc.kill() - Add Partials.Channel to Discord adapter for DM support - Add discord-bridge and slack-bridge to show_status() and do_stop() - Normalize error messages in catch blocks for non-Error throws
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
scripts/adapters/messaging/slack.js (1)
13-22: Keep the Socket Mode token declarative.Lines 14-21 are the one place this bridge layer stops being config-driven: the runner already models extra credentials in config, but the adapter hardcodes
SLACK_APP_TOKEN. Reading the app token from config (or a named config field) keeps the YAML as the source of truth and avoids adapter-ownedprocess.exit(1)behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/adapters/messaging/slack.js` around lines 13 - 22, The adapter currently hardcodes APP_TOKEN from process.env.SLACK_APP_TOKEN and calls process.exit(1); instead read the app token from the config (e.g. use process.env[config.credentials.app_token_env] or a named config field) alongside BOT_TOKEN and ALLOWED so credentials are fully config-driven; remove the direct process.exit(1) call in the module (throw an Error or return a rejected promise from the initialization function) and update any initialization code that expects APP_TOKEN to surface the failure instead of exiting the process. Ensure you modify the APP_TOKEN binding and error path where APP_TOKEN is referenced and keep BOT_TOKEN, ALLOWED, config.credentials.token_env and config.credentials.allowed_env consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/adapters/messaging/telegram.js`:
- Around line 34-35: The current JSON parse block always resolves even when the
Bot API returns an error object, so API errors (parsed JSON with ok: false)
never trigger the retry fallback; change the parse/resolve logic so that after
parsing you check if parsed.ok === false and in that case reject (or throw) the
parsed error instead of resolving it, otherwise resolve the parsed result;
update the code around the Promise that contains the resolve(JSON.parse(buf)) /
catch(...) block so API error responses cause the outer catch (the handler
around lines 76-84) to run and trigger the Markdown->plain-text retry.
In `@scripts/bridge.js`:
- Around line 48-56: Array items parsed by the "- " branch are being pushed into
parent[lastKey] which may be an empty object created earlier for a block key (so
the first list item is lost); modify the "- " handling (the block that uses
lastKey, parent, and unquote) to detect when parent[lastKey] exists and is a
plain empty object (e.g., Object.prototype.toString.call(parent[lastKey]) ===
'[object Object]' and Object.keys(parent[lastKey]).length === 0) and replace it
with an array before pushing, or if parent[lastKey] is undefined create an
array; then push unquote(val) into that array so block-style YAML lists like
extra_env become arrays instead of remaining {}.
- Around line 147-156: Remove or gate the plaintext transcript logging around
the bridge: stop writing raw chat content with the console.log that prints
`[${config.name}] [${msg.channelId}] ${msg.userName}: ${msg.text}` and the agent
output log that prints `agent: ${response.slice(0, 100)}...`; instead only emit
metadata (e.g., config.name, msg.channelId, timestamp, and message length) or
wrap these content logs behind an explicit debug flag so they are not written to
host stdout by default. Locate the surrounding code that calls msg.sendTyping(),
runAgentInSandbox(msg.text, ...) and references the response variable, and
replace the direct content logs with either metadata-only logs or conditional
logging driven by a DEBUG_BRIDGE_TRAFFIC environment/config flag.
---
Nitpick comments:
In `@scripts/adapters/messaging/slack.js`:
- Around line 13-22: The adapter currently hardcodes APP_TOKEN from
process.env.SLACK_APP_TOKEN and calls process.exit(1); instead read the app
token from the config (e.g. use process.env[config.credentials.app_token_env] or
a named config field) alongside BOT_TOKEN and ALLOWED so credentials are fully
config-driven; remove the direct process.exit(1) call in the module (throw an
Error or return a rejected promise from the initialization function) and update
any initialization code that expects APP_TOKEN to surface the failure instead of
exiting the process. Ensure you modify the APP_TOKEN binding and error path
where APP_TOKEN is referenced and keep BOT_TOKEN, ALLOWED,
config.credentials.token_env and config.credentials.allowed_env consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1cf4899e-462d-4b95-b309-edbd3af96379
📒 Files selected for processing (10)
nemoclaw-blueprint/bridges/messaging/discord.yamlnemoclaw-blueprint/bridges/messaging/slack.yamlnemoclaw-blueprint/bridges/messaging/telegram.yamlscripts/adapters/messaging/discord.jsscripts/adapters/messaging/slack.jsscripts/adapters/messaging/telegram.jsscripts/bridge.jsscripts/start-services.shscripts/telegram-bridge.jstest/runner.test.js
💤 Files with no reviewable changes (1)
- scripts/telegram-bridge.js
✅ Files skipped from review due to trivial changes (3)
- nemoclaw-blueprint/bridges/messaging/slack.yaml
- nemoclaw-blueprint/bridges/messaging/discord.yaml
- nemoclaw-blueprint/bridges/messaging/telegram.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- scripts/start-services.sh
- test/runner.test.js
Comment on lines
+34
to
+35
| try { resolve(JSON.parse(buf)); } catch { resolve({ ok: false, error: buf }); } | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In the Telegram Bot API, do sendMessageparse errors return a JSON response withok: false rather than rejecting the HTTP request?
💡 Result:
Yes. The Bot API returns a JSON body that always includes an ok boolean; for unsuccessful requests it sets "ok": false and includes "description" (and usually "error_code"). [1]
For sendMessage formatting/parse failures (e.g., "Bad Request: can't parse entities ..."), Telegram responds with an HTTP 400 Bad Request and a JSON body like:
{"ok":false,"error_code":400,"description":"Bad Request: can't parse entities: ..."}rather than “rejecting” the request at the transport level with no Bot-API JSON response. [2]
Sources: [1] [2]
🏁 Script executed:
cd scripts/adapters/messaging && wc -l telegram.jsRepository: NVIDIA/NemoClaw
Length of output: 73
🏁 Script executed:
cd scripts/adapters/messaging && sed -n '30,90p' telegram.jsRepository: NVIDIA/NemoClaw
Length of output: 2019
The Telegram Markdown fallback never fires on API errors.
Lines 34-35 resolve Bot API failures as { ok: false }, so the catch in lines 76-84 only handles transport errors. A parse failure from parse_mode: "Markdown" will therefore drop the reply instead of retrying without formatting.
💡 Proposed fix
async reply(text) {
- await tgApi("sendMessage", {
+ const res = await tgApi("sendMessage", {
chat_id: channelId,
text,
reply_to_message_id: msg.message_id,
parse_mode: "Markdown",
- }).catch(() =>
- tgApi("sendMessage", { chat_id: channelId, text, reply_to_message_id: msg.message_id }),
- );
+ });
+ if (!res.ok) {
+ const fallback = await tgApi("sendMessage", {
+ chat_id: channelId,
+ text,
+ reply_to_message_id: msg.message_id,
+ });
+ if (!fallback.ok) {
+ throw new Error(fallback.description || fallback.error || "Telegram sendMessage failed");
+ }
+ }
},🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/adapters/messaging/telegram.js` around lines 34 - 35, The current
JSON parse block always resolves even when the Bot API returns an error object,
so API errors (parsed JSON with ok: false) never trigger the retry fallback;
change the parse/resolve logic so that after parsing you check if parsed.ok ===
false and in that case reject (or throw) the parsed error instead of resolving
it, otherwise resolve the parsed result; update the code around the Promise that
contains the resolve(JSON.parse(buf)) / catch(...) block so API error responses
cause the outer catch (the handler around lines 76-84) to run and trigger the
Markdown->plain-text retry.
Replace raw message text and agent response logging with metadata (channel ID, username, message length, response length) to avoid leaking PII or secrets into host logs.
The minimal YAML parser silently dropped array items under nested keys
(e.g., extra_env: [SLACK_APP_TOKEN] parsed as {}). Use js-yaml which
is already in the dependency tree via eslint.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
scripts/bridge-core.js (1)
120-127: Consider whetherAPI_KEYexport is necessary.The
API_KEYis exported but appears to only be used internally withinrunAgentInSandbox. If no external consumers require it, removing it from exports reduces the surface area where the key could be accidentally logged or exposed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/bridge-core.js` around lines 120 - 127, API_KEY is being exported from the module but is only used internally by runAgentInSandbox; remove API_KEY from module.exports to reduce accidental exposure, leaving runAgentInSandbox and other intended exports intact; verify there are no external consumers importing API_KEY (search for symbol API_KEY) and, if any exist, either update those callers to use runAgentInSandbox or provide a safer accessor function, and ensure runAgentInSandbox continues to reference the internal API_KEY variable without changing its behavior.scripts/bridge.js (1)
65-71: Multiline YAML strings (|) are silently mishandled.When
valPart === "|", the parser treats it as a nested object rather than a multiline string. If bridge configs ever use multiline values, they'll be parsed incorrectly. Consider adding a comment noting this limitation or throwing an error if|or>is encountered.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/bridge.js` around lines 65 - 71, The parser currently treats valPart === "|" as a nested object (setting parent[key] = {} and pushing to stack), misparsing YAML multiline scalars; update the logic in the block handling valPart (the branch that sets parent[key] and calls stack.push) to detect multiline indicators "|" and ">" and either (a) throw a clear error indicating multiline strings are not supported, or (b) implement multiline parsing by reading subsequent indented lines into a single string and assigning parent[key] = that string; ensure you reference valPart, parent[key], and the stack handling so the fix replaces the nested-object behavior for "|" and ">" with the chosen error or multiline-accumulation behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/bridge-core.js`:
- Around line 49-63: The execFileSync call inside runAgentInSandbox can throw
synchronously and crash the process; modify runAgentInSandbox to accept both
resolve and reject in the Promise constructor (change Promise((resolve) => ...)
to Promise((resolve, reject) => ...)) and wrap the execFileSync (and any other
synchronous operations that may throw) in a try/catch; on catch call reject(err)
so the caller can handle the failure. Reference function runAgentInSandbox and
the execFileSync(OPENSHELL, ["sandbox", "ssh-config", SANDBOX], ...) call when
making this change.
In `@scripts/bridge.js`:
- Around line 143-144: The code accesses config.messaging.session_prefix and
config.messaging.max_chunk_size (prefix and maxChunk) without checking that
config.messaging exists; add a defensive check or validation before these
accesses—e.g., ensure config.messaging is an object (or validate the config on
load) and provide sensible defaults or throw a clear error if missing so that
prefix and maxChunk are only read when config.messaging is defined; update the
block that sets prefix and maxChunk to use that guard (or optional chaining with
defaults) to prevent a TypeError.
---
Nitpick comments:
In `@scripts/bridge-core.js`:
- Around line 120-127: API_KEY is being exported from the module but is only
used internally by runAgentInSandbox; remove API_KEY from module.exports to
reduce accidental exposure, leaving runAgentInSandbox and other intended exports
intact; verify there are no external consumers importing API_KEY (search for
symbol API_KEY) and, if any exist, either update those callers to use
runAgentInSandbox or provide a safer accessor function, and ensure
runAgentInSandbox continues to reference the internal API_KEY variable without
changing its behavior.
In `@scripts/bridge.js`:
- Around line 65-71: The parser currently treats valPart === "|" as a nested
object (setting parent[key] = {} and pushing to stack), misparsing YAML
multiline scalars; update the logic in the block handling valPart (the branch
that sets parent[key] and calls stack.push) to detect multiline indicators "|"
and ">" and either (a) throw a clear error indicating multiline strings are not
supported, or (b) implement multiline parsing by reading subsequent indented
lines into a single string and assigning parent[key] = that string; ensure you
reference valPart, parent[key], and the stack handling so the fix replaces the
nested-object behavior for "|" and ">" with the chosen error or
multiline-accumulation behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cd4c3b07-a552-4a63-a06a-767f534791af
📒 Files selected for processing (4)
scripts/adapters/messaging/discord.jsscripts/bridge-core.jsscripts/bridge.jsscripts/start-services.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/start-services.sh
Bridge definitions now live in components.bridges alongside sandbox, inference, and policy — following the existing blueprint schema with credential_env field naming. Removes the separate bridges/ directory and hand-rolled YAML parser in favor of js-yaml reading blueprint.yaml.
Users and scripts that reference scripts/telegram-bridge.js directly continue to work — the wrapper injects "telegram" into argv and delegates to the generic bridge runner.
RISKY CHANGE — migration path to standardize messaging configuration. When messaging tokens (Telegram, Discord, Slack) are detected at onboard completion, automatically start the host-side bridges via start-services.sh. This seamlessly migrates users from the in-sandbox OpenClaw plugin path (#601) to the host-side bridge architecture. Discord and Slack enforce single gateway connections per token, so the host bridge naturally takes over from the in-sandbox plugin. The env var passthrough is kept for backwards compatibility during this transition.
Tests cover: - blueprint.yaml bridge config schema and credential_env naming - slack extra_credential_env array parsing (the js-yaml fix) - telegram-bridge.js backwards-compat wrapper delegation - bridge.js reads from blueprint.yaml, not separate files - bridge.js logs metadata only (no raw message content) - onboard auto-starts bridges with risk annotation - all four credential types use getCredential pattern
ericksoa
changed the title
If openshell sandbox ssh-config fails (sandbox not running), the synchronous throw inside the Promise constructor would crash the bridge. Now catches and resolves with a descriptive error message.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
scripts/adapters/messaging/telegram.js (1)
76-84:⚠️ Potential issue | 🟠 MajorRetry on
ok: false, not just transport failures.
tgApi()resolves Bot API 4xx bodies as JSON, so this.catch()never sees Markdown parse errors. Those replies are dropped instead of retried withoutparse_mode.💡 Proposed fix
async reply(text) { - await tgApi("sendMessage", { + const res = await tgApi("sendMessage", { chat_id: channelId, text, reply_to_message_id: msg.message_id, parse_mode: "Markdown", - }).catch(() => - tgApi("sendMessage", { chat_id: channelId, text, reply_to_message_id: msg.message_id }), - ); + }); + if (!res.ok) { + const fallback = await tgApi("sendMessage", { + chat_id: channelId, + text, + reply_to_message_id: msg.message_id, + }); + if (!fallback.ok) { + throw new Error(fallback.description || fallback.error || "Telegram sendMessage failed"); + } + } },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/adapters/messaging/telegram.js` around lines 76 - 84, The reply() helper currently only retries when tgApi() rejects, but Telegram returns 200 with {ok:false} for parse errors so the catch() never runs; update reply(text) to inspect tgApi("sendMessage", ...) response and if response.ok === false (or response.result is missing) retry a second tgApi("sendMessage", ...) call without parse_mode (and preserve chat_id and reply_to_message_id). Ensure this logic references the existing reply method and uses the same parameters (chat_id, reply_to_message_id, text) when retrying so Markdown parse failures are retried without parse_mode.
🤖 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/onboard.js`:
- Around line 978-990: startMessagingBridges currently detects file-backed
tokens via getCredential(...) but launches start-services.sh with only
process.env, so tokens stored in credentials.json never reach the child shell;
update startMessagingBridges to resolve each possible credential
(TELEGRAM_BOT_TOKEN, DISCORD_BOT_TOKEN, SLACK_BOT_TOKEN) by calling
getCredential(...) and merge those values into an env object passed to run(...,
{ env: mergedEnv, ignoreError: true }) so the child process and scripts like
start-services.sh / bridge.js can read the tokens directly from process.env;
keep the existing sandboxEnv handling and only add the resolved credential keys
to the env passed to run.
In `@nemoclaw-blueprint/blueprint.yaml`:
- Around line 65-80: The blueprint currently points both messaging adapters to
the same allowlist env var (ALLOWED_CHANNEL_IDS) causing
process.env[config.allowed_env] in scripts/adapters/messaging/discord.js and
scripts/adapters/messaging/slack.js to share a single cross-platform filter;
change the discord and slack service blocks to use platform-specific allowed_env
names (e.g., DISCORD_ALLOWED_CHANNEL_IDS for the discord block and
SLACK_ALLOWED_CHANNEL_IDS for the slack block) and update any documentation or
deployment envs accordingly so each adapter reads its own env var via
config.allowed_env.
In `@scripts/adapters/messaging/telegram.js`:
- Around line 20-40: The tgApi function currently never sets a client-side
socket timeout, so a hung HTTPS request can block forever; update tgApi to
accept/derive a timeout (e.g., default 30s) and after creating req call
req.setTimeout(timeout) and attach req.on("timeout", () => { req.destroy(); /*
cause request error/abort */ }); also ensure the request's "error" handler or
the promise rejection conveys a clear timeout error so callers of
tgApi("getUpdates") won't hang indefinitely; reference tgApi, req, and the
"error" handler when making the changes.
In `@scripts/bridge.js`:
- Around line 132-138: The listing currently only checks c.credential_env but
runBridge() also requires c.extra_credential_env; update the "--list" block
(where configs is iterated) so the presence token is "✓" only when process.env
has the primary credential and all extras: treat c.extra_credential_env as
either a string or iterable (array) of env names, check each via process.env,
and show "✗" if any required env is missing; reference the loadBridgeConfigs
result iteration and the c.credential_env / c.extra_credential_env fields when
implementing the check.
---
Duplicate comments:
In `@scripts/adapters/messaging/telegram.js`:
- Around line 76-84: The reply() helper currently only retries when tgApi()
rejects, but Telegram returns 200 with {ok:false} for parse errors so the
catch() never runs; update reply(text) to inspect tgApi("sendMessage", ...)
response and if response.ok === false (or response.result is missing) retry a
second tgApi("sendMessage", ...) call without parse_mode (and preserve chat_id
and reply_to_message_id). Ensure this logic references the existing reply method
and uses the same parameters (chat_id, reply_to_message_id, text) when retrying
so Markdown parse failures are retried without parse_mode.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1361678e-ee1d-494c-8ccc-fa0455152b22
📒 Files selected for processing (10)
bin/lib/onboard.jsnemoclaw-blueprint/blueprint.yamlscripts/adapters/messaging/discord.jsscripts/adapters/messaging/slack.jsscripts/adapters/messaging/telegram.jsscripts/bridge-core.jsscripts/bridge.jsscripts/start-services.shscripts/telegram-bridge.jstest/runner.test.js
🚧 Files skipped from review as they are similar to previous changes (3)
- scripts/start-services.sh
- scripts/adapters/messaging/slack.js
- scripts/bridge-core.js
- Export file-backed credentials to process.env before calling start-services.sh so bridges see tokens from credentials.json - Use per-platform allowlist env vars (ALLOWED_DISCORD_CHANNEL_IDS, ALLOWED_SLACK_CHANNEL_IDS, ALLOWED_TELEGRAM_CHAT_IDS) to prevent cross-platform filter collision - Add client-side socket timeout (60s) to Telegram HTTP adapter to prevent hung connections from blocking the poll loop forever - Show all required env vars in bridge --list (Slack shows both SLACK_BOT_TOKEN and SLACK_APP_TOKEN, marks ✗ if either missing) - Keep backwards compat: Telegram adapter falls back to legacy ALLOWED_CHAT_IDS env var
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
bin/lib/onboard.js (1)
456-463:⚠️ Potential issue | 🟠 MajorDon’t keep Discord/Slack on the immutable sandbox-env path by default.
The comment immediately above says OpenClaw auto-enables channels from these env vars, and Line 966 now auto-starts host-side bridges too. That leaves Discord/Slack active from both sides and keeps stale tokens baked into the sandbox until recreate.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/lib/onboard.js` around lines 456 - 463, Currently the onboarding script unconditionally injects DISCORD_BOT_TOKEN and SLACK_BOT_TOKEN into envArgs (variables discordToken, slackToken, envArgs, using getCredential and shellQuote), which bakes tokens into the immutable sandbox-env; change this to opt-in only: remove the unconditional pushes and instead only push these tokens when an explicit opt-in flag or environment variable is set (e.g., OPENCLAW_ENABLE_HOST_BRIDGES or a CLI option) so host-side bridges are not auto-enabled for sandbox environments; update the logic around where discordToken/slackToken are read to check that opt-in flag before calling envArgs.push.
🤖 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/onboard.js`:
- Around line 452-455: The code only adds the resolved NVIDIA API key to envArgs
but leaves process.env.NVIDIA_API_KEY untouched, so later reads still fail for
keys stored in credentials.json; modify the onboarding flow so that when
getCredential("NVIDIA_API_KEY") returns a value you also set
process.env.NVIDIA_API_KEY (or otherwise replace later direct process.env reads
with the resolved value) so the non-interactive cloud path uses the credential.
Ensure this change is applied where envArgs is built and will cover subsequent
usages that currently access process.env.NVIDIA_API_KEY (refer to getCredential,
envArgs and the later reads of process.env.NVIDIA_API_KEY).
In `@scripts/bridge.js`:
- Around line 88-90: The typing indicator should be best-effort: move the
initial await msg.sendTyping() into a try/catch so a transient failure doesn't
abort handling, and change the setInterval callback that calls msg.sendTyping()
so it does not drop promise rejections (e.g. call msg.sendTyping().catch(()=>{})
or wrap in try/catch), ensuring the typingInterval behavior remains unchanged;
reference the msg.sendTyping() call, the typingInterval variable, and the
setInterval callback when making the change.
- Around line 105-108: The catch block currently sends err.message back to the
channel (see use of err.message in the catch handling for typingInterval and
msg.reply); change it to send a neutral/generic reply (e.g., "An error occurred
while processing your request.") instead of exposing err.message, and move the
full diagnostic into a local log (e.g., console.error or your process logger) so
host/relay details remain in the bridge process; retain
clearInterval(typingInterval) and the existing msg.reply(...).catch(() => {})
behavior but replace the public message content and log the original err
locally.
---
Outside diff comments:
In `@bin/lib/onboard.js`:
- Around line 456-463: Currently the onboarding script unconditionally injects
DISCORD_BOT_TOKEN and SLACK_BOT_TOKEN into envArgs (variables discordToken,
slackToken, envArgs, using getCredential and shellQuote), which bakes tokens
into the immutable sandbox-env; change this to opt-in only: remove the
unconditional pushes and instead only push these tokens when an explicit opt-in
flag or environment variable is set (e.g., OPENCLAW_ENABLE_HOST_BRIDGES or a CLI
option) so host-side bridges are not auto-enabled for sandbox environments;
update the logic around where discordToken/slackToken are read to check that
opt-in flag before calling envArgs.push.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 61939143-c33b-4f1e-90e9-a4610da3822a
📒 Files selected for processing (4)
bin/lib/onboard.jsnemoclaw-blueprint/blueprint.yamlscripts/adapters/messaging/telegram.jsscripts/bridge.js
🚧 Files skipped from review as they are similar to previous changes (2)
- nemoclaw-blueprint/blueprint.yaml
- scripts/adapters/messaging/telegram.js
Comment on lines
+452
to
455
| const apiKey = getCredential("NVIDIA_API_KEY") || process.env.NVIDIA_API_KEY; | ||
| if (apiKey) { | ||
| envArgs.push(`NVIDIA_API_KEY=${shellQuote(apiKey)}`); | ||
| } |
There was a problem hiding this comment.
Finish the NVIDIA_API_KEY migration beyond envArgs.
These lines resolve the key only for sandbox env injection. Line 701 and Line 728 still read process.env.NVIDIA_API_KEY, so a key stored only in ~/.nemoclaw/credentials.json still breaks the non-interactive cloud path.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bin/lib/onboard.js` around lines 452 - 455, The code only adds the resolved
NVIDIA API key to envArgs but leaves process.env.NVIDIA_API_KEY untouched, so
later reads still fail for keys stored in credentials.json; modify the
onboarding flow so that when getCredential("NVIDIA_API_KEY") returns a value you
also set process.env.NVIDIA_API_KEY (or otherwise replace later direct
process.env reads with the resolved value) so the non-interactive cloud path
uses the credential. Ensure this change is applied where envArgs is built and
will cover subsequent usages that currently access process.env.NVIDIA_API_KEY
(refer to getCredential, envArgs and the later reads of
process.env.NVIDIA_API_KEY).
Comment on lines
+88
to
+90
| // Typing indicator | ||
| await msg.sendTyping(); | ||
| const typingInterval = setInterval(() => msg.sendTyping(), 4000); |
There was a problem hiding this comment.
Make typing indicators best-effort.
Line 89 is outside the try, and Line 90 drops promise rejections from the interval callback. A transient typing failure can abort message handling entirely or surface as unhandled rejections every 4 seconds.
💡 Minimal fix
- await msg.sendTyping();
- const typingInterval = setInterval(() => msg.sendTyping(), 4000);
+ await Promise.resolve(msg.sendTyping()).catch(() => {});
+ const typingInterval = setInterval(() => {
+ Promise.resolve(msg.sendTyping()).catch(() => {});
+ }, 4000);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/bridge.js` around lines 88 - 90, The typing indicator should be
best-effort: move the initial await msg.sendTyping() into a try/catch so a
transient failure doesn't abort handling, and change the setInterval callback
that calls msg.sendTyping() so it does not drop promise rejections (e.g. call
msg.sendTyping().catch(()=>{}) or wrap in try/catch), ensuring the
typingInterval behavior remains unchanged; reference the msg.sendTyping() call,
the typingInterval variable, and the setInterval callback when making the
change.
Comment on lines
+105
to
+108
| } catch (err) { | ||
| clearInterval(typingInterval); | ||
| const errorMsg = err && err.message ? err.message : String(err); | ||
| await msg.reply(`Error: ${errorMsg}`).catch(() => {}); |
There was a problem hiding this comment.
Return a generic failure message to chat.
err.message here can include host-side relay details that should stay inside the bridge process. Keep diagnostics local and send a neutral error string back to the channel.
💡 Minimal fix
- const errorMsg = err && err.message ? err.message : String(err);
- await msg.reply(`Error: ${errorMsg}`).catch(() => {});
+ console.error(`[${config.name}] [${msg.channelId}] relay failed`);
+ await msg.reply("Sorry — the bridge hit an internal error. Please try again.").catch(() => {});📝 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
| } catch (err) { | |
| clearInterval(typingInterval); | |
| const errorMsg = err && err.message ? err.message : String(err); | |
| await msg.reply(`Error: ${errorMsg}`).catch(() => {}); | |
| } catch (err) { | |
| clearInterval(typingInterval); | |
| console.error(`[${config.name}] [${msg.channelId}] relay failed`); | |
| await msg.reply("Sorry — the bridge hit an internal error. Please try again.").catch(() => {}); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/bridge.js` around lines 105 - 108, The catch block currently sends
err.message back to the channel (see use of err.message in the catch handling
for typingInterval and msg.reply); change it to send a neutral/generic reply
(e.g., "An error occurred while processing your request.") instead of exposing
err.message, and move the full diagnostic into a local log (e.g., console.error
or your process logger) so host/relay details remain in the bridge process;
retain clearInterval(typingInterval) and the existing msg.reply(...).catch(() =>
{}) behavior but replace the public message content and log the original err
locally.
cv
reviewed
Mar 22, 2026
| let buf = ""; | ||
| res.on("data", (c) => (buf += c)); | ||
| res.on("end", () => { | ||
| try { resolve(JSON.parse(buf)); } catch { resolve({ ok: false, error: buf }); } |
Contributor
There was a problem hiding this comment.
tgApi always resolves — even when Telegram returns ok: false (e.g. Markdown parse errors on sendMessage). This means the .catch() fallback in reply() never fires on API errors, only on network errors. Rejecting on ok: false fixes the Markdown retry and makes all callers handle API errors consistently.
Suggested change
| try { resolve(JSON.parse(buf)); } catch { resolve({ ok: false, error: buf }); } | |
| try { | |
| const json = JSON.parse(buf); | |
| if (json.ok) resolve(json); else reject(new Error(json.description || "Telegram API error")); | |
| } catch { reject(new Error(`Telegram API returned non-JSON: ${buf.slice(0, 200)}`)); } |
Note: getMe at startup already checks me.ok explicitly — with this change it would also reject on the promise, but the result is the same (thrown error with a descriptive message). You could simplify that caller too if you want.
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
2 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
Credential unification (#616)
TELEGRAM_BOT_TOKENinto sandbox at creation time (was only in deploy path)NVIDIA_API_KEYto usegetCredential()like all other tokensYAML-driven bridge framework (#618)
blueprint.yamlundercomponents.bridges— same schema as sandbox, inference, and policy componentsscripts/bridge.js— generic runner that reads blueprint, loads adapter, drives shared message flowscripts/bridge-core.js— sandbox relay via OpenShell SSH (extracted from telegram-bridge.js)scripts/adapters/messaging/— thin SDK wrappers for Telegram, Discord, Slackscripts/telegram-bridge.js— backwards-compat wrapper delegating to bridge.jsAuto-start bridges after onboard (RISKY — migration path)
Security fix (#573)
bridge-core.jsusesshellQuote(API_KEY)when interpolating into SSH commandsArchitecture
All messaging bridges follow the same pattern:
Closes #616
Closes #618
Closes #573
Related to #585 — the host-side Discord bridge bypasses the in-sandbox proxy issue (fetch failed on gateway connection) by running Discord API calls on the host.
Test plan
Summary by CodeRabbit
New Features
Documentation
Tests