fix(presets): add missing binaries to communication presets#1084
fix(presets): add missing binaries to communication presets#1084cortexj wants to merge 2 commits intoNVIDIA:mainfrom
Conversation
|
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 ignored due to path filters (1)
📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (3)
📝 WalkthroughWalkthroughThe presets for Discord, Slack, and Telegram network policies were updated to expand each preset's binaries allowlist by adding Changes
Estimated Code Review Effort🎯 1 (Trivial) | ⏱️ ~3 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
nemoclaw-blueprint/policies/presets/slack.yaml (1)
44-48: Optional: centralize shared communication preset binaries to reduce drift.The same binary block now exists across Slack/Telegram/Discord. If the preset system supports it, consider a shared include/anchor to keep these lists consistent over time.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nemoclaw-blueprint/policies/presets/slack.yaml` around lines 44 - 48, The binaries list under the "binaries" key in the Slack preset is duplicated across Slack/Telegram/Discord; refactor by extracting the shared array into a single reusable item (e.g., a YAML anchor/alias or a shared preset/include) and reference that from each communication preset (Slack, Telegram, Discord) so the entries like "/usr/local/bin/node", "/usr/bin/node", "/usr/bin/curl", "/usr/bin/bash" are maintained in one place rather than repeated in the Slack/Telegram/Discord presets.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@nemoclaw-blueprint/policies/presets/slack.yaml`:
- Around line 44-48: The binaries list under the "binaries" key in the Slack
preset is duplicated across Slack/Telegram/Discord; refactor by extracting the
shared array into a single reusable item (e.g., a YAML anchor/alias or a shared
preset/include) and reference that from each communication preset (Slack,
Telegram, Discord) so the entries like "/usr/local/bin/node", "/usr/bin/node",
"/usr/bin/curl", "/usr/bin/bash" are maintained in one place rather than
repeated in the Slack/Telegram/Discord presets.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6bff0b9f-c608-4576-bf56-a1da2ede4c34
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (3)
nemoclaw-blueprint/policies/presets/discord.yamlnemoclaw-blueprint/policies/presets/slack.yamlnemoclaw-blueprint/policies/presets/telegram.yaml
Telegram, Discord, and Slack presets only allowlist /usr/local/bin/node. On Debian/Ubuntu where Node.js installs to /usr/bin/node, and for skills using curl or bash scripts, the OpenShell proxy returns HTTP 403. Add /usr/bin/node, /usr/bin/curl, and /usr/bin/bash to all three presets. Fixes NVIDIA#481
ee44717 to
3df7c2d
Compare
|
Good catch — YAML anchors only work within a single document, and these are independent preset files loaded separately. A shared binaries include mechanism would need changes to the preset loader, which is out of scope for this fix. Happy to contribute if the team wants to build that into the preset system. |
|
✨ Thanks for submitting this PR with a detailed summary, it addresses a bug with communication presets and proposes a fix to improve the integration of NemoClaw with Discord and Telegram, which could enhance the user experience. |
|
@cortexj Thanks for working on this. The underlying issue is real on current I’m not comfortable merging this branch as written for two concrete reasons:
If you want to get this re-reviewable, I’d suggest:
If you respin it that way, I’d be happy to take another look. |
…DIA#481) The telegram, discord, and slack presets only allowlist /usr/local/bin/node in the binaries field. OPA does exact-path matching, so on sandbox base images that install Node.js to /usr/bin/node (Debian/Ubuntu apt default instead of the manual / nvm path), the L7 proxy silently 403s every outbound HTTPS request from the bot — even though the endpoint policies are correctly applied and showing in 'openshell policy get'. Multiple users in NVIDIA#481 confirmed this on: - WSL2 Ubuntu (Bilguunzayaa, dknos) - DGX Spark ARM/Grace Blackwell (eddie-schick) - Pop!_OS x86_64 (cortexj) - Ubuntu Server 25.10 (b1skit) The original fix attempt NVIDIA#1084 by @cortexj was correct in spirit but also added /usr/bin/curl and /usr/bin/bash without a clear need, plus an unrelated package-lock.json rewrite. Maintainer (kjw3) asked for a narrowed re-spin in a 2026-04-01 review and the PR has been stalled since. This change implements exactly the narrow fix kjw3 directionally approved: just /usr/bin/node, no curl/bash, no lockfile churn. Adds a regression test asserting both /usr/local/bin/node and /usr/bin/node are present in all three communication presets. Closes NVIDIA#481 Signed-off-by: latenighthackathon <[email protected]>
|
Following up on the earlier review ask: I’m moving forward with a narrower superseding fix on branch Reason for superseding rather than reviving this PR as-is:
Planned scope of the superseding branch for the preset side:
I’ll link the follow-up PR once it is up. |
## Summary This draft PR advances the Discord/native messaging remediation work in NemoClaw without claiming that native Discord is fully fixed end to end yet. It also folds in the analogous Telegram doctor/config follow-up so rebuilt sandboxes match the current account-based channel schema and avoid a misleading empty group allowlist warning. It now also addresses the repo-owned portion of #1692 by removing deprecated `tls: terminate` directives from the messaging policy presets. It does five repo-owned things: 1. narrows and supersedes the communication preset fix from #1084 by adding only the minimal additional Node binary coverage 2. fixes onboarding so messaging policy suggestions follow the current messaging selection rather than stale stored credentials 3. fixes the baked Discord account schema at image build time, adds build-time Discord guild workspace config, and improves troubleshooting/E2E validation so baked-config failures are separated from native Discord gateway failures 4. aligns baked Telegram config with the same doctor-expected account layout and makes Telegram group handling explicit so default group delivery does not silently drop messages 5. removes deprecated TLS-termination syntax from the Discord, Slack, and Telegram presets so NemoClaw does not keep shipping policy warnings on current OpenShell builds ## Why this PR exists The current evidence suggests the remaining Discord break is not just a missing onboarding step: - the sandbox has the Discord policy applied - the Discord provider is attached - command registration `PUT` requests are allowed at runtime - the bot can authenticate against `discord.com` - the bot can read channel history and send messages back into Discord - current NemoClaw onboarding only baked token/account config, not the Discord guild config that OpenClaw documents for server-channel autonomy What still appears broken is the autonomous inbound side of native Discord handling. In current testing, OpenClaw can read and write Discord successfully, but incoming `@mention` messages do not appear to wake the agent in real time. After baking the documented guild config into the sandbox, the failure still reproduces. Direct diagnosis inside the sandbox shows that: - the baked `channels.discord.guilds.<serverId>` config is present and correct - Node DNS lookup for `gateway.discord.gg` succeeds - a plain Node `WebSocket('wss://gateway.discord.gg/...')` succeeds inside the same sandbox - OpenClaw's Discord gateway client still loops with `AggregateError` and close code `1006` That makes the remaining issue look narrower than generic connectivity: likely the live Discord gateway client path in the pinned OpenClaw runtime (`2026.3.11`) under the OpenShell proxy environment. This PR tightens NemoClaw’s side of the system so: - the sandbox policy and onboarding behavior are correct - the baked Discord config matches current OpenClaw schema expectations - Discord server-workspace config can now be baked at image build time instead of being missing from NemoClaw onboarding entirely - onboarding can ask whether the bot should reply only to `@mentions` or to all messages in the configured server - the Discord user allowlist is optional, so leaving it blank allows any member of the configured server to message the bot - Telegram single-account config is already baked in the `accounts.default` layout that `openclaw doctor` now expects - Telegram group chats default to `groupPolicy: open`, so an empty DM allowlist does not produce silent group-message drops - messaging policy presets no longer pin deprecated `tls: terminate` directives on current OpenShell builds - users are not sent toward dead-end config repair steps - tests explicitly probe the Discord gateway path instead of only REST ## Changes - `bin/lib/onboard.js` - add `getSuggestedPolicyPresets()` - make messaging-related policy suggestions respect explicit `enabledChannels` - persist messaging channel selection through onboarding/resume flow - `nemoclaw-blueprint/policies/presets/{discord,telegram,slack}.yaml` - add `/usr/bin/node` alongside `/usr/local/bin/node` - remove deprecated `tls: terminate` directives and rely on current OpenShell automatic TLS handling - intentionally do **not** broaden to `curl` or `bash` in these narrow presets - `docs/reference/troubleshooting.md` - document that `openclaw doctor --fix` cannot repair baked sandbox channel config - add guidance for distinguishing baked-config issues from native Discord gateway issues - add guidance for the analogous Telegram doctor migration/warning path - `Dockerfile` - bake messaging accounts under `channels.<provider>.accounts.default` instead of `accounts.main` - align the image with the schema `openclaw doctor` currently expects, so this migration is handled at build time rather than failing inside the immutable sandbox - add a build-time `NEMOCLAW_DISCORD_GUILDS_B64` path so Discord guild allowlist config can be baked into `openclaw.json` - set Telegram `groupPolicy: open` in the baked default account config - `bin/lib/onboard.js` - prompt for Discord Server ID and User ID during messaging setup - bake `channels.discord.guilds.<serverId>` config for the selected server(s) - prompt for mention-only vs all-message replies in guild channels - treat the Discord user allowlist as optional; leaving it blank allows any member of the configured server - keep `requireMention: true` as the default so onboarding remains safe for shared servers - `test/e2e/test-messaging-providers.sh` - add a classified native Discord gateway probe separate from REST reachability - accept both `accounts.default` and `accounts.main` during the transition so older images still validate - assert baked Telegram `groupPolicy: open` - `test/onboard.test.js` - add coverage for policy suggestions following current messaging selection - add coverage for the baked Discord guild workspace build arg - add coverage for both mention-only guild config and open-to-all-members guild config - `test/policies.test.js` - add coverage for `/usr/bin/node` in communication presets - add coverage that messaging presets no longer ship deprecated `tls: terminate` - `.agents/skills/nemoclaw-user-reference/references/troubleshooting.md` - regenerated from docs ## Related issues - supersedes #1084 for the preset change - addresses #1610 from the NemoClaw side - addresses #1692 from the NemoClaw side - provides clearer handling for #606 - improves investigation coverage for #585 and #1570 ## Validation Completed: - `git diff --check` - `bash -n test/e2e/test-messaging-providers.sh` - `rg -n "tls: terminate" nemoclaw-blueprint/policies/presets/{telegram,discord,slack}.yaml test/policies.test.js` - `node -c bin/lib/onboard.js` - manual host-side verification that the live sandbox has: - the Discord policy applied - the Discord provider attached - allowed Discord command-registration `PUT` traffic at runtime - successful Discord read/write behavior from the bot in a real server channel - a remaining failure mode where direct `@mention` traffic is only observed when OpenClaw is prompted from the TUI, rather than waking autonomously in real time - a correctly baked Discord guild config inside the sandbox after redeploy - successful `dns.lookup('gateway.discord.gg')` and raw Node WebSocket connectivity from inside the sandbox - continued OpenClaw Discord gateway failure with `AggregateError` and WebSocket close code `1006` Not completed in this environment: - full `vitest` runs were attempted but hung without producing output in this sandboxed session ## Current limitation This PR does **not** yet claim a full end-to-end native Discord fix. The remaining evidence points to the live Discord gateway client path inside OpenClaw rather than to NemoClaw config. The next step is to inspect or patch the pinned OpenClaw runtime behavior and determine whether that final fix can be applied from NemoClaw or requires upstream OpenClaw changes. ## Type of change - [x] Code change for a bug fix or refactor - [x] Doc updates - [x] Tests updated <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Discord onboarding prompts for Server ID, reply mode (@mentions-only or all messages) and optional user allowlist; messaging selections are persisted into the sandbox image. * **Documentation** * Clarified Telegram allowlist: enforced for DMs only; group chats remain open by default (prevents rebuilt sandboxes from dropping group messages). * Added troubleshooting for baked channel configs and Discord gateway diagnostics. * **Policy Changes** * Messaging network policy TLS handling adjusted and Node binary paths expanded for messaging providers. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Test User <[email protected]> Co-authored-by: Aaron Erickson 🦞 <[email protected]>
|
Closing this out in favor of #1653, which was merged yesterday and delivers the minimal justified /usr/bin/node coverage that addressed the underlying issue. Thanks @kjw3 for the thorough review and for taking this across the line with the narrower scope. I'll open separate issues if the curl/bash additions turn out to be needed for a reproducible runtime failure in a future use case. |
Summary
Communication presets (Telegram, Discord, Slack) only allowlist
/usr/local/bin/nodein thebinariesfield. On Debian/Ubuntu where Node.js installs to/usr/bin/node, and for skills usingcurlor bash scripts, the OpenShell proxy returns HTTP 403 on all outbound HTTPS from the sandbox.Related Issue
Fixes #481
Changes
/usr/bin/nodeto telegram, discord, and slack presets (Debian/Ubuntu default Node.js path)/usr/bin/curlto all three presets (HTTP client used by skills for API calls)/usr/bin/bashto all three presets (required for shell script execution inside sandbox)Type of Change
Testing
npx prek run --all-filespasses (or equivalentlymake check).npm testpasses.make docsbuilds without warnings. (for doc-only changes)Manual testing:
curland bash-based skills no longer receive 403 from proxy at 10.200.0.1:3128prek; TypeScript type-check failures are pre-existing and unrelated to this changeChecklist
General
Code Changes
npx prek run --all-filesauto-fixes formatting (ormake formatfor targeted runs).Summary by CodeRabbit