fix(messaging): warn/block second Slack Socket Mode sandbox on a shared gateway#5039
Conversation
|
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: Enterprise Run ID: 📒 Files selected for processing (12)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (8)
📝 WalkthroughWalkthroughAdds gateway-scoped Slack Socket Mode conflict detection, status reporting and overlap merging, a Slack-only add-channel precheck, and an onboarding guard that enforces or prompts on gateway-level Slack conflicts, with tests covering detection, messaging, and control-flow behaviors. ChangesSlack Socket Mode Gateway-Scoped Conflict Detection and Enforcement
Sequence Diagram(s)sequenceDiagram
participant User as User/Operator
participant Onboard as Onboard Flow
participant Guard as enforceMessagingChannelConflicts
participant Registry as Sandbox Registry
participant Detection as findSlackSocketModeGatewayConflicts / detectAllSlackSocketModeGatewayOverlaps
participant Prompt as promptContinue
User->>Onboard: Create sandbox with Slack
Onboard->>Guard: enforceMessagingChannelConflicts(deps)
Guard->>Registry: listSandboxes / backfill
Registry-->>Guard: [existing sandboxes]
Guard->>Detection: detect gateway conflicts/overlaps
Detection-->>Guard: [{sandbox: "other", gatewayName: "shared-gateway"}]
Guard->>Guard: Log Slack conflict warning via formatSlackSocketModeConflictMessage
alt Non-interactive
Guard->>Onboard: Abort (exit)
else Interactive
Guard->>Prompt: "Continue anyway?" prompt
alt User declines
Prompt-->>Guard: false
Guard->>Onboard: Abort (throw)
else User accepts
Prompt-->>Guard: true
Guard->>Onboard: Resolve (allow creation)
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
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.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lib/onboard.ts`:
- Around line 2778-2793: enforceMessagingChannelConflicts is being run against
the full configured channel set before resolveDisabledChannels/disabledEnvKeys
pruning, causing sandboxes with channels marked disabled (e.g., slack stopped)
to falsely block others; update the call site to compute the effective channel
set (run resolveDisabledChannels and/or inspect disabledEnvKeys) and pass that
post-pruning state into enforceMessagingChannelConflicts (or alternatively move
the enforceMessagingChannelConflicts invocation to after resolveDisabledChannels
returns) so Socket Mode conflict checks use the pruned channel list; update the
argument signature or add a new parameter (e.g., disabledChannels or
effectiveChannels) to enforceMessagingChannelConflicts and adjust callers
accordingly (symbols: enforceMessagingChannelConflicts, resolveDisabledChannels,
disabledEnvKeys).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 413743a7-2543-4d1a-a58f-fd86f20f1c75
📒 Files selected for processing (12)
src/lib/actions/sandbox/policy-channel-conflict.test.tssrc/lib/actions/sandbox/policy-channel.tssrc/lib/inventory/index.test.tssrc/lib/inventory/index.tssrc/lib/messaging/applier/conflict-detection-slack-gateway.test.tssrc/lib/messaging/applier/conflict-detection/index.tssrc/lib/messaging/applier/conflict-detection/slack-socket-mode.tssrc/lib/messaging/applier/conflict-detection/types.tssrc/lib/onboard.tssrc/lib/onboard/messaging-conflict-guard.test.tssrc/lib/onboard/messaging-conflict-guard.tssrc/lib/status-command-deps.ts
|
Thanks @coderabbitai — fixed in f2d86c1. The current sandbox's stopped channels ( |
|
Tip For best results, initiate chat on the files or code changes.
🐇✨ ✏️ Learnings added
|
wscurran
added
area: messaging
yimoj
force-pushed
the
fix/4953-slack-socket-mode-conflict
branch
from
f2d86c1 to
4faff18
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
@coderabbitai review — rebased onto current main (conflict resolved) and addressed the credential-vs-gateway ordering: status now reports both conflict axes independently, and |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/lib/actions/sandbox/policy-channel.ts (1)
494-494:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove the unused fallback initializer.
The initial value
(s) => sassigned toformatSlackSocketModeConflictMessageis never used because the variable is always overwritten at line 497 inside the try block. If the try block throws, the function returns early at line 509, so the fallback is unreachable. Remove the initializer and declare the variable with type annotation only.🧹 Proposed fix
- let formatSlackSocketModeConflictMessage: (s: string) => string = (s) => s; + let formatSlackSocketModeConflictMessage: (s: string) => string;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/actions/sandbox/policy-channel.ts` at line 494, The variable formatSlackSocketModeConflictMessage currently has an unused fallback initializer "(s) => s"; remove that initializer and declare it with only its type annotation (e.g., let formatSlackSocketModeConflictMessage: (s: string) => string;) so the value is assigned exclusively inside the try block where it's actually set and the unreachable fallback is eliminated.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@src/lib/actions/sandbox/policy-channel.ts`:
- Line 494: The variable formatSlackSocketModeConflictMessage currently has an
unused fallback initializer "(s) => s"; remove that initializer and declare it
with only its type annotation (e.g., let formatSlackSocketModeConflictMessage:
(s: string) => string;) so the value is assigned exclusively inside the try
block where it's actually set and the unreachable fallback is eliminated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 64ead58a-796d-493b-9d87-a8ba42b8fdd8
📒 Files selected for processing (12)
src/lib/actions/sandbox/policy-channel-conflict.test.tssrc/lib/actions/sandbox/policy-channel.tssrc/lib/inventory/index.test.tssrc/lib/inventory/index.tssrc/lib/messaging/applier/conflict-detection-slack-gateway.test.tssrc/lib/messaging/applier/conflict-detection/index.tssrc/lib/messaging/applier/conflict-detection/slack-socket-mode.tssrc/lib/messaging/applier/conflict-detection/types.tssrc/lib/onboard.tssrc/lib/onboard/messaging-conflict-guard.test.tssrc/lib/onboard/messaging-conflict-guard.tssrc/lib/status-command-deps.ts
|
Thanks @coderabbitai — addressed in 6f2b837. Dropped the unreachable |
|
Tip For best results, initiate chat on the files or code changes.
🐇✨ |
|
@coderabbitai review |
✅ Action performedReview finished.
|
…ed gateway Slack Socket Mode is gateway-scoped, not only credential-scoped: even with two distinct Slack apps (different bot/app tokens), only one sandbox per OpenShell gateway reliably receives Socket Mode events. NemoClaw still reported the second sandbox's bridge as healthy, so the operator got a silent black hole (NVIDIA#4953). The existing detection only catches a *shared* token (matching-token / unknown-token). Add a complementary gateway-scoped axis, keyed on the sandbox's OpenShell gateway registration name, that fires whenever another sandbox on the same gateway already has Slack active — regardless of whether the tokens differ: - New conflict-detection/slack-socket-mode.ts with pure detection helpers (findSlackSocketModeGatewayConflicts, detectAllSlackSocketModeGatewayOverlaps) and the operator-facing message. A missing gatewayName normalizes to the default `nemoclaw` gateway so legacy entries compare correctly (NVIDIA#4422). - Onboard: extract both conflict axes into onboard/messaging-conflict-guard.ts (keeps onboard.ts net-negative for the growth guardrail) and refuse a second Slack sandbox on the same gateway unless overridden. Stopped channels (`channels stop`) are folded into the effective plan's disabledChannels before both axes so a stopped bridge is not counted as a consumer. - `channels add slack`: run the credential check first (gateway-independent and more actionable), then a fail-soft gateway-scoped check for the distinct-token, same-gateway case; honors --force. - `status`: report credential and gateway overlaps independently so a pair that shares both a token and a gateway still surfaces the cross-gateway credential warning; mark a shared-gateway Slack pair conflicted instead of healthy. E2E: onboarded a first Slack sandbox, then a second on the same gateway — before the fix it silently proceeded to build; after the fix it aborts with the Socket Mode conflict message before creating the sandbox. Signed-off-by: Yimo Jiang <yimoj@nvidia.com>
yimoj
force-pushed
the
fix/4953-slack-socket-mode-conflict
branch
from
6f2b837 to
39b0b0a
Compare
|
@coderabbitai review — rebased onto current main (resolved conflicts from the #4945 revert / #5115), and addressed the channel-add gateway-resolution finding: the Slack conflict check now compares against the default |
|
✅ Action performedReview finished.
|
prekshivyas
left a comment
prekshivyas
left a comment
There was a problem hiding this comment.
Reviewed (code + security checklist). Adds a gateway-scoped Slack Socket Mode conflict axis (independent of the existing credential-token axis) so a second Slack sandbox on the same OpenShell gateway is warned/blocked at onboard and channels add, and flagged as conflicted in status, instead of becoming a silent black hole (#4953).
✅ Approve. Verified the load-bearing wiring against source: policy-channel.ts hardcoding BASE_GATEWAY_NAME for the add path is correct (recoverNamedGatewayRuntime always selects nemoclaw), while onboard uses the per-port GATEWAY_NAME — no false negative; gatewayName is a real persisted field with legacy→nemoclaw normalization; the credential axis runs first and short-circuits the shared-token case to the more-actionable warning. Conflict messages interpolate only sandbox names, never tokens (tests assert tokens absent). Security: all categories pass.
Non-blocking nits:
- PR body says the status Slack line is "deduped against the credential-overlap line," but the code intentionally does not dedup (concatenates both axes, with an explaining comment). Code is fine — correct the body wording.
detectAllSlackSocketModeGatewayOverlapsemits all N-choose-2 pairs, so 3+ Slack sandboxes on one gateway produce O(N²) status lines. N is realistically tiny; optionally collapse to one line per gateway.
Tests thorough: pure-helper cases (same/different/missing gateway, disabled Slack, self-exclusion, N-pair enumeration), onboard-guard orchestration, channels add paths incl. credential-first ordering and fail-soft, and the status renderer.
## Summary - Add the v0.0.63 release-note section using the published development note as source context. - Update source docs for sandbox recovery, OpenClaw config restore safety, managed vLLM selection, Slack Socket Mode conflict handling, and host diagnostics. - Refresh generated `nemoclaw-user-*` skills from the updated Fern MDX docs. - Update the release-doc refresh skill so post-release docs for version `n` look up the matching announcement discussion and use the `n+1` patch release label. - Fix CLI/docs parity by avoiding a `--from <Dockerfile>` flag mention inside the `upgrade-sandboxes` command section. ## Source summary - #5034 -> `docs/reference/troubleshooting.mdx`, `docs/about/release-notes.mdx`: Document safer stale-sandbox recovery through `rebuild --yes` before recreating from scratch. - #5091 -> `docs/reference/troubleshooting.mdx`, `docs/about/release-notes.mdx`: Document Docker-driver post-reboot recovery from OpenShell container labels. - #5101, #5174, #5177 -> `docs/manage-sandboxes/backup-restore.mdx`, `docs/about/release-notes.mdx`: Document OpenClaw `openclaw.json` preservation, merge behavior, and fail-safe restore handling. - #5102 -> `docs/reference/commands.mdx`, `docs/reference/commands-nemohermes.mdx`, `docs/manage-sandboxes/lifecycle.mdx`, `docs/about/release-notes.mdx`: Document `upgrade-sandboxes` image-fingerprint drift detection. - #4201 -> `docs/reference/troubleshooting.mdx`, `docs/about/release-notes.mdx`: Document the installer diagnostic for unexpected Docker daemon access outside the `docker` group. - #5038 -> `docs/inference/inference-options.mdx`, `docs/inference/use-local-inference.mdx`, `docs/about/release-notes.mdx`: Document the interactive managed-vLLM model picker and non-interactive override behavior. - #5040, #5041 -> `docs/reference/troubleshooting.mdx`, `docs/about/release-notes.mdx`: Document Ollama auth-proxy recovery and host DNS preflight diagnostics. - #4986, #5039 -> `docs/manage-sandboxes/messaging-channels.mdx`, `docs/about/release-notes.mdx`: Document Slack validation and duplicate Slack Socket Mode sandbox handling. - #4981, #5168 -> `docs/about/release-notes.mdx`: Capture Hermes gateway secret-guard and wrapped-argv startup hardening in the release surface. - Follow-up -> `.agents/skills/nemoclaw-contributor-update-docs/SKILL.md`: Record the post-release docs workflow, discussion-announcement lookup, and next-patch release label rule. - Follow-up -> `docs/reference/commands.mdx`, `docs/reference/commands-nemohermes.mdx`: Reword custom Dockerfile sandbox text so CLI parity does not treat `--from` as an `upgrade-sandboxes` flag. ## Verification - `python3 scripts/docs-to-skills.py docs/ .agents/skills/ --prefix nemoclaw-user --doc-platform fern-mdx` - `npm run docs` - `npm run build:cli` - `bash test/e2e/e2e-cloud-experimental/check-docs.sh --only-cli` - Skip-term scan for `docs/.docs-skip` blocked terms across generated user skills <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Enhanced local inference setup with interactive model selection prompts and environment variable overrides * Improved sandbox upgrade detection using build fingerprints and version checks * Clarified configuration restore behavior preserving user settings during rebuild/restore * Added gateway authentication as fifth security layer * Expanded Slack messaging validation with live credential checking * Enhanced troubleshooting guidance for Docker access, DNS issues, and sandbox recovery * Updated release notes for v0.0.63 featuring sandbox recovery and inference improvements <!-- end of auto-generated comment: release notes by coderabbit.ai -->
5 participants
Summary
Slack Socket Mode is gateway-scoped, not only credential-scoped: even with two distinct Slack apps (different bot/app tokens), only one sandbox per OpenShell gateway reliably receives Socket Mode events. NemoClaw still reported the second sandbox's bridge as healthy, so the operator got a silent black hole. This adds a gateway-scoped conflict axis that warns/blocks (and marks
statusconflicted) when a second Slack sandbox is added to a gateway that already has one.Related Issue
Fixes #4953
Changes
conflict-detection/slack-socket-mode.tswith pure helpers (findSlackSocketModeGatewayConflicts,detectAllSlackSocketModeGatewayOverlaps,formatSlackSocketModeConflictMessage). A missinggatewayNamenormalizes to the defaultnemoclawgateway so legacy entries compare correctly ([WSL2 x86_64][Sandbox] NEMOCLAW_GATEWAY_PORT=N onboard recreates global gateway and destroys previous sandbox — concurrent instances unsupported #4422). This axis is independent of the existingmatching-token/unknown-tokencredential detection — it fires on a shared gateway regardless of whether the tokens differ.onboard/messaging-conflict-guard.ts(keepsonboard.tsnet-negative for the codebase-growth guardrail) and refuse a second Slack sandbox on the same gateway in non-interactive mode / require override interactively.channels add slack: warn/block on a shared gateway, honoring--force; the gateway lookup is fail-soft so a malformed registry cannot crash the add or bypass the existing guarded credential check.status: mark a shared-gateway Slack pair as conflicted (slack-socket-mode-gateway) rather than silently healthy, deduped against the credential-overlap line.channels addpaths (same-gateway block, different-gateway pass, fail-soft), and thestatusrenderer.Type of Change
Verification
npm testpasses (CLI project; the only failures are pre-existing environment-driven subprocess-spawn timeouts and thee2e-fixture-contextSIGKILL/SIGTERM timing test — all unrelated to these files, which pass in isolation)codex review --uncommittedclean ("No correctness-impacting issues were found")E2E (real CLI, isolated gateway port): onboarded a first Slack sandbox (
sa-4953) on gatewaynemoclaw-8085, then onboarded a second Slack sandbox on the same gateway. Before the fix it silently proceeded to build the second sandbox (the #4953 black hole); after the fix it aborts before sandbox creation with:Signed-off-by: Yimo Jiang yimoj@nvidia.com
Summary by CodeRabbit
New Features
Behavior
Tests
Refactor