diff --git a/src/lib/actions/sandbox/policy-channel-conflict.test.ts b/src/lib/actions/sandbox/policy-channel-conflict.test.ts index b718d8c67ee..f14bc60078f 100644 --- a/src/lib/actions/sandbox/policy-channel-conflict.test.ts +++ b/src/lib/actions/sandbox/policy-channel-conflict.test.ts @@ -643,4 +643,130 @@ describe("addSandboxChannel cross-sandbox conflict check (#4305)", () => { expect(text).not.toContain(slackApp); expect(upsertMock).toHaveBeenCalledTimes(1); }); + + it("slack: a second sandbox on the SAME gateway is blocked even with a different token (#4953)", async () => { + const slackBot = "xoxb-alpha-bot-token"; + const slackApp = "xapp-alpha-app-token"; + arrangeRegistry({ + current: { name: "alpha", messagingChannels: [] } as SandboxEntry, + // bob holds Slack on the default gateway with entirely different tokens — + // the credential axis would NOT flag this, but the gateway axis must. + others: [ + makePlanEntry("bob", "slack", [ + { + providerEnvKey: "SLACK_BOT_TOKEN", + credentialHash: hashCredential("xoxb-bob-bot") as string, + }, + { + providerEnvKey: "SLACK_APP_TOKEN", + credentialHash: hashCredential("xapp-bob-app") as string, + }, + ]), + ], + }); + getCredentialMock.mockImplementation((key: string) => + key === "SLACK_BOT_TOKEN" ? slackBot : key === "SLACK_APP_TOKEN" ? slackApp : null, + ); + promptMock.mockResolvedValue("n"); // decline the conflict prompt + + await addSandboxChannel("alpha", { channel: "slack" }); + + const text = loggedText(); + expect(text).toContain("Slack Socket Mode is already enabled for sandbox 'bob'"); + expect(text).not.toContain("same slack credential"); // gateway axis, not a token match + expect(text).not.toContain(slackBot); + expect(text).not.toContain(slackApp); + expect(conflictPromptShown()).toBe(true); + expect(upsertMock).not.toHaveBeenCalled(); // aborted before registering + }); + + it("slack: shared token on the same gateway reports the credential conflict first (#4953)", async () => { + // The credential axis runs before the gateway axis, so a shared Slack token + // surfaces the gateway-independent "same slack credential" warning (more + // actionable: it conflicts even after moving to another gateway) instead of + // only the same-gateway remediation. + const slackBot = "xoxb-shared-bot-token"; + const slackApp = "xapp-shared-app-token"; + arrangeRegistry({ + current: { name: "alpha", messagingChannels: [] } as SandboxEntry, + others: [ + makePlanEntry("bob", "slack", [ + { providerEnvKey: "SLACK_BOT_TOKEN", credentialHash: hashCredential(slackBot) as string }, + { providerEnvKey: "SLACK_APP_TOKEN", credentialHash: hashCredential(slackApp) as string }, + ]), + ], + }); + getCredentialMock.mockImplementation((key: string) => + key === "SLACK_BOT_TOKEN" ? slackBot : key === "SLACK_APP_TOKEN" ? slackApp : null, + ); + process.env.NEMOCLAW_NON_INTERACTIVE = "1"; + + await expect(addSandboxChannel("alpha", { channel: "slack" })).rejects.toThrow( + "process.exit(1)", + ); + + const text = loggedText(); + expect(text).toContain("same slack credential"); // credential axis fired first + expect(text).not.toContain(slackBot); + expect(text).not.toContain(slackApp); + expect(exitMock).toHaveBeenCalledWith(1); + expect(upsertMock).not.toHaveBeenCalled(); + }); + + it("slack: a second sandbox on a DIFFERENT gateway is not gateway-blocked (#4953)", async () => { + const bob = makePlanEntry("bob", "slack", [ + { + providerEnvKey: "SLACK_BOT_TOKEN", + credentialHash: hashCredential("xoxb-bob-bot") as string, + }, + { + providerEnvKey: "SLACK_APP_TOKEN", + credentialHash: hashCredential("xapp-bob-app") as string, + }, + ]); + (bob as { gatewayName?: string }).gatewayName = "nemoclaw-9090"; + arrangeRegistry({ + current: { name: "alpha", messagingChannels: [] } as SandboxEntry, + others: [bob], + }); + getCredentialMock.mockImplementation((key: string) => + key === "SLACK_BOT_TOKEN" + ? "xoxb-alpha-bot" + : key === "SLACK_APP_TOKEN" + ? "xapp-alpha-app" + : null, + ); + promptMock.mockResolvedValue("n"); // would abort if any conflict prompt were shown + + await addSandboxChannel("alpha", { channel: "slack" }); + + const text = loggedText(); + expect(text).not.toContain("Slack Socket Mode is already enabled"); + expect(conflictPromptShown()).toBe(false); + expect(upsertMock).toHaveBeenCalledTimes(1); + }); + + it("slack: a gateway conflict-detection failure is fail-soft, not a crash (#4953)", async () => { + arrangeRegistry({ current: { name: "alpha", messagingChannels: [] } as SandboxEntry }); + // Simulate a malformed registry read: listSandboxes throws. The Slack + // gateway lookup must swallow it (best-effort warning) rather than crash + // the add or bypass the downstream guarded credential check. + listSandboxesMock.mockImplementation(() => { + throw new Error("registry boom"); + }); + getCredentialMock.mockImplementation((key: string) => + key === "SLACK_BOT_TOKEN" + ? "xoxb-alpha-bot" + : key === "SLACK_APP_TOKEN" + ? "xapp-alpha-app" + : null, + ); + promptMock.mockResolvedValue("y"); // proceed through any "could not verify" prompt + + await addSandboxChannel("alpha", { channel: "slack" }); + + expect(loggedText()).toContain("Could not verify Slack Socket Mode gateway conflicts"); + expect(exitMock).not.toHaveBeenCalled(); + expect(upsertMock).toHaveBeenCalledTimes(1); + }); }); diff --git a/src/lib/actions/sandbox/policy-channel.ts b/src/lib/actions/sandbox/policy-channel.ts index 0c213c9e887..0d2b1369c8c 100644 --- a/src/lib/actions/sandbox/policy-channel.ts +++ b/src/lib/actions/sandbox/policy-channel.ts @@ -475,6 +475,65 @@ async function checkChannelAddConflict( return false; } +// Gateway-scoped Slack Socket Mode conflict (#4953): even with a distinct Slack +// app/token, only one sandbox per OpenShell gateway reliably receives Socket +// Mode events. Runs AFTER `checkChannelAddConflict` so the credential axis — +// which catches a *shared* token and stays accurate across gateways — is +// reported first; this axis then catches the distinct-token, same-gateway case +// instead of letting it become a silent black hole. Returns true to PROCEED, +// false to abort. Fail-soft: a detection error must not crash the add or bypass +// `--force`, so it is swallowed (the credential axis already ran its guarded +// check). Only meaningful for Slack; other channels proceed unchanged. +async function checkSlackSocketModeGatewayConflict( + sandboxName: string, + channelName: string, + force: boolean, +): Promise { + if (channelName !== "slack") return true; + let conflictMessages: string[] = []; + try { + const applier = require("../../messaging/applier") as typeof import("../../messaging/applier"); + const { BASE_GATEWAY_NAME } = + require("../../onboard/gateway-binding") as typeof import("../../onboard/gateway-binding"); + // `channels add` registers the Slack provider on the default `nemoclaw` + // gateway — applyChannelAddToGatewayAndRegistry → recoverNamedGatewayRuntime + // selects `nemoclaw` regardless of the sandbox's recorded gateway. Detect + // conflicts on the gateway the add actually mutates so the check matches the + // provider registration and cannot leave a false negative (#4953). + const gatewayName = BASE_GATEWAY_NAME; + conflictMessages = applier + .findSlackSocketModeGatewayConflicts( + sandboxName, + gatewayName, + registry.listSandboxes().sandboxes, + ) + .map(({ sandbox }) => applier.formatSlackSocketModeConflictMessage(sandbox)); + } catch (err) { + const message = err instanceof Error ? err.message : String(err); + console.log(` ${YW}⚠${R} Could not verify Slack Socket Mode gateway conflicts: ${message}`); + return true; + } + if (conflictMessages.length === 0) return true; + + for (const message of conflictMessages) { + console.log(` ${YW}⚠${R} ${message}`); + } + if (force) { + console.log(" --force: proceeding despite the Slack Socket Mode gateway conflict above."); + return true; + } + if (isNonInteractive()) { + console.error( + ` Aborting: only one sandbox per gateway can receive Slack Socket Mode events. Run \`${CLI_NAME} channels remove slack\` on the other sandbox, onboard this sandbox on a separate gateway (set NEMOCLAW_GATEWAY_PORT), or re-run with --force.`, + ); + process.exit(1); + } + const answer = (await askPrompt(" Continue anyway? [y/N]: ")).trim().toLowerCase(); + if (answer === "y" || answer === "yes") return true; + console.log(" Aborting channel add."); + return false; +} + // Push channel tokens to the OpenShell gateway and add the channel to the // sandbox registry's messagingChannels list. Done eagerly at `channels // add` time (not deferred to rebuild) because the host-side credential @@ -1063,6 +1122,11 @@ export async function addSandboxChannel( if (!(await checkChannelAddConflict(sandboxName, canonical, acquired, force))) { return; // user aborted; nothing registered or widened } + // Credential axis passed; now the gateway-scoped Slack Socket Mode axis (#4953) + // catches the distinct-token, same-gateway case the credential check cannot. + if (!(await checkSlackSocketModeGatewayConflict(sandboxName, canonical, force))) { + return; // user aborted; nothing registered or widened + } assertAddChannelPlanActive(sandboxName, manifest, plan); // QR-paired channels that own their session inside the sandbox have no diff --git a/src/lib/inventory/index.test.ts b/src/lib/inventory/index.test.ts index b9ca85e3994..1a119adca8c 100644 --- a/src/lib/inventory/index.test.ts +++ b/src/lib/inventory/index.test.ts @@ -446,6 +446,34 @@ describe("inventory commands", () => { ).toBe(true); }); + it("marks a shared-gateway Slack Socket Mode overlap as conflicted (#4953)", () => { + const lines: string[] = []; + const backfillAndFindOverlaps = vi + .fn() + .mockReturnValue([ + { channel: "slack", sandboxes: ["alice", "bob"], reason: "slack-socket-mode-gateway" }, + ]); + showStatusCommand({ + listSandboxes: () => ({ + sandboxes: [ + { name: "alice", model: "m", messagingChannels: ["slack"] }, + { name: "bob", model: "m", messagingChannels: ["slack"] }, + ], + defaultSandbox: "alice", + }), + getLiveInference: () => null, + showServiceStatus: vi.fn(), + backfillAndFindOverlaps, + log: (message = "") => lines.push(message), + }); + + expect( + lines.some((l) => + l.includes("'alice' and 'bob' both have Slack Socket Mode enabled on the same gateway"), + ), + ).toBe(true); + }); + it("surfaces Hermes gateway log when messaging is degraded", () => { const lines: string[] = []; const checkMessagingBridgeHealth = vi diff --git a/src/lib/inventory/index.ts b/src/lib/inventory/index.ts index 74c7752fa9f..43df3f6d13a 100644 --- a/src/lib/inventory/index.ts +++ b/src/lib/inventory/index.ts @@ -85,7 +85,10 @@ export interface SandboxInventoryResult { export interface MessagingOverlap { channel: string; sandboxes: [string, string]; - reason?: "matching-token" | "unknown-token"; + // "slack-socket-mode-gateway": both sandboxes have Slack Socket Mode active on + // the same OpenShell gateway, so only one receives events (#4953) — distinct + // from the credential-sharing reasons, which catch a *shared* token. + reason?: "matching-token" | "unknown-token" | "slack-socket-mode-gateway"; } export interface GatewayHealth { @@ -474,6 +477,12 @@ export function showStatusCommand(deps: ShowStatusCommandDeps): void { if (overlaps.length > 0) { log(""); for (const { channel, sandboxes: pair, reason } of overlaps) { + if (reason === "slack-socket-mode-gateway") { + log( + ` ⚠ '${pair[0]}' and '${pair[1]}' both have Slack Socket Mode enabled on the same gateway; only one sandbox can receive Slack Socket Mode events unless the gateway supports multiplexing.`, + ); + continue; + } const detail = reason === "matching-token" ? `share the same ${channel} credential` diff --git a/src/lib/messaging/applier/conflict-detection-slack-gateway.test.ts b/src/lib/messaging/applier/conflict-detection-slack-gateway.test.ts new file mode 100644 index 00000000000..7f7a1637666 --- /dev/null +++ b/src/lib/messaging/applier/conflict-detection-slack-gateway.test.ts @@ -0,0 +1,124 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +import { describe, expect, it } from "vitest"; + +import { + makePlan, + planEntry, + slackBindings, + slackChannel, + tgChannel, +} from "../../../../test/helpers/messaging-conflict-fixtures"; +import type { ConflictRegistryEntry } from "./conflict-detection"; +import { + detectAllSlackSocketModeGatewayOverlaps, + findSlackSocketModeGatewayConflicts, + formatSlackSocketModeConflictMessage, +} from "./conflict-detection"; + +function slackEntry(name: string, gatewayName?: string | null): ConflictRegistryEntry { + const entry = planEntry( + name, + makePlan(name, { + channels: [slackChannel()], + credentialBindings: slackBindings("b", "a", name), + }), + ); + return gatewayName === undefined ? entry : { ...entry, gatewayName }; +} + +describe("findSlackSocketModeGatewayConflicts", () => { + it("flags another sandbox with Slack active on the same gateway", () => { + const alice = slackEntry("alice", "nemoclaw"); + expect(findSlackSocketModeGatewayConflicts("bob", "nemoclaw", [alice])).toEqual([ + { sandbox: "alice", gatewayName: "nemoclaw" }, + ]); + }); + + it("does not flag a sandbox on a different gateway", () => { + const alice = slackEntry("alice", "nemoclaw-9090"); + expect(findSlackSocketModeGatewayConflicts("bob", "nemoclaw", [alice])).toEqual([]); + }); + + it("treats a missing gatewayName as the default nemoclaw gateway", () => { + // Legacy entry created before per-port gateway naming (#4422): no recorded + // name means it was on the default gateway. + const legacy = slackEntry("legacy", undefined); + expect(findSlackSocketModeGatewayConflicts("bob", "nemoclaw", [legacy])).toEqual([ + { sandbox: "legacy", gatewayName: "nemoclaw" }, + ]); + }); + + it("excludes the current sandbox itself", () => { + const bob = slackEntry("bob", "nemoclaw"); + expect(findSlackSocketModeGatewayConflicts("bob", "nemoclaw", [bob])).toEqual([]); + }); + + it("ignores a sandbox whose Slack channel is disabled", () => { + const alice = planEntry( + "alice", + makePlan("alice", { + disabledChannels: ["slack"], + channels: [{ ...slackChannel(), disabled: true }], + credentialBindings: slackBindings("b", "a", "alice"), + }), + ); + expect( + findSlackSocketModeGatewayConflicts("bob", "nemoclaw", [ + { ...alice, gatewayName: "nemoclaw" }, + ]), + ).toEqual([]); + }); + + it("ignores a sandbox without Slack active", () => { + const alice = planEntry("alice", makePlan("alice", { channels: [tgChannel()] })); + expect( + findSlackSocketModeGatewayConflicts("bob", "nemoclaw", [ + { ...alice, gatewayName: "nemoclaw" }, + ]), + ).toEqual([]); + }); +}); + +describe("detectAllSlackSocketModeGatewayOverlaps", () => { + it("reports one pair for two Slack sandboxes on the same gateway", () => { + expect( + detectAllSlackSocketModeGatewayOverlaps([ + slackEntry("alice", "nemoclaw"), + slackEntry("bob", "nemoclaw"), + ]), + ).toEqual([{ gatewayName: "nemoclaw", sandboxes: ["alice", "bob"] }]); + }); + + it("does not report Slack sandboxes on different gateways", () => { + expect( + detectAllSlackSocketModeGatewayOverlaps([ + slackEntry("alice", "nemoclaw"), + slackEntry("bob", "nemoclaw-9090"), + ]), + ).toEqual([]); + }); + + it("reports every pair when three Slack sandboxes share a gateway", () => { + const overlaps = detectAllSlackSocketModeGatewayOverlaps([ + slackEntry("a", "nemoclaw"), + slackEntry("b", "nemoclaw"), + slackEntry("c", "nemoclaw"), + ]); + expect(overlaps).toEqual([ + { gatewayName: "nemoclaw", sandboxes: ["a", "b"] }, + { gatewayName: "nemoclaw", sandboxes: ["a", "c"] }, + { gatewayName: "nemoclaw", sandboxes: ["b", "c"] }, + ]); + }); +}); + +describe("formatSlackSocketModeConflictMessage", () => { + it("names the other sandbox and states the one-per-gateway constraint", () => { + expect(formatSlackSocketModeConflictMessage("alice")).toBe( + "Slack Socket Mode is already enabled for sandbox 'alice' on this gateway; " + + "only one sandbox can receive Slack Socket Mode events unless the gateway supports multiplexing.", + ); + }); +}); diff --git a/src/lib/messaging/applier/conflict-detection/index.ts b/src/lib/messaging/applier/conflict-detection/index.ts index 2622b19fcf3..59ea640afc6 100644 --- a/src/lib/messaging/applier/conflict-detection/index.ts +++ b/src/lib/messaging/applier/conflict-detection/index.ts @@ -6,4 +6,5 @@ export * from "./entries"; export * from "./plan"; export * from "./probe"; export * from "./registry"; +export * from "./slack-socket-mode"; export type * from "./types"; diff --git a/src/lib/messaging/applier/conflict-detection/slack-socket-mode.ts b/src/lib/messaging/applier/conflict-detection/slack-socket-mode.ts new file mode 100644 index 00000000000..33246ff0a67 --- /dev/null +++ b/src/lib/messaging/applier/conflict-detection/slack-socket-mode.ts @@ -0,0 +1,120 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +import { BASE_GATEWAY_NAME } from "../../../onboard/gateway-binding"; +import { resolveActiveChannelsFromEntry } from "./entries"; +import type { ConflictRegistryEntry } from "./types"; + +/** + * Gateway-scoped Slack Socket Mode conflict detection (#4953). + * + * Slack Socket Mode is special among NemoClaw's messaging channels: the + * conflict is *gateway-scoped*, not only credential-scoped. Even when two + * sandboxes use two distinct Slack apps (different bot/app tokens), only one + * sandbox per OpenShell gateway reliably receives Socket Mode events — the + * effective runtime routes events to a single registered consumer, so a second + * Slack sandbox on the same gateway silently receives nothing while NemoClaw + * still reports its bridge as healthy (the silent black hole in #4953). + * + * The credential-based detection in `entries.ts` (`matching-token` / + * `unknown-token`) catches the *same-token* case, which also covers two + * sandboxes on *different* gateways sharing one Slack app. This module is the + * complementary axis: same *gateway*, regardless of whether the tokens differ. + * Both run together — neither subsumes the other. + * + * The gateway a sandbox is bound to is identified by its OpenShell gateway + * registration name (`SandboxEntry.gatewayName`), which the per-port resolver + * derives 1:1 from the gateway port (default port -> `nemoclaw`, any other port + * -> `nemoclaw-`; see `onboard/gateway-binding.ts` and #4422). The name is + * therefore the authoritative gateway key. Entries created before per-port + * gateway naming have no recorded name and were always on the default gateway, + * so a missing name normalizes to `nemoclaw`. + */ + +export const SLACK_CHANNEL_ID = "slack"; + +/** + * The OpenShell gateway registration name a registry entry is bound to. + * A missing/null name normalizes to the default `nemoclaw` gateway so legacy + * entries (and entries on the default port) compare equal to a current onboard + * targeting the default gateway. + */ +export function resolveEntryGatewayName(entry: ConflictRegistryEntry): string { + return entry.gatewayName ?? BASE_GATEWAY_NAME; +} + +/** True when the entry has Slack active (present and not disabled). */ +export function entryHasActiveSlack(entry: ConflictRegistryEntry): boolean { + return resolveActiveChannelsFromEntry(entry)?.includes(SLACK_CHANNEL_ID) ?? false; +} + +export interface SlackGatewayConflict { + /** The other sandbox already holding Slack Socket Mode on this gateway. */ + readonly sandbox: string; + /** The shared gateway registration name. */ + readonly gatewayName: string; +} + +/** + * Return every *other* sandbox bound to `currentGatewayName` that already has + * Slack active. Used by the onboard and `channels add` paths to warn/block + * before a second Slack Socket Mode bridge is added to the same gateway. + */ +export function findSlackSocketModeGatewayConflicts( + currentSandbox: string | null, + currentGatewayName: string, + entries: readonly ConflictRegistryEntry[], +): SlackGatewayConflict[] { + return entries + .filter((entry) => entry.name !== currentSandbox) + .filter((entry) => entryHasActiveSlack(entry)) + .filter((entry) => resolveEntryGatewayName(entry) === currentGatewayName) + .map((entry) => ({ sandbox: entry.name, gatewayName: currentGatewayName })); +} + +export interface SlackGatewayOverlap { + readonly gatewayName: string; + readonly sandboxes: [string, string]; +} + +/** + * Detect Slack Socket Mode gateway overlaps across all entries, returning each + * pair at most once. Used by `nemoclaw status` to mark a second Slack sandbox + * on a shared gateway as conflicted rather than silently healthy. + */ +export function detectAllSlackSocketModeGatewayOverlaps( + entries: readonly ConflictRegistryEntry[], +): SlackGatewayOverlap[] { + const byGateway = new Map(); + for (const entry of entries) { + if (!entryHasActiveSlack(entry)) continue; + const gatewayName = resolveEntryGatewayName(entry); + const list = byGateway.get(gatewayName) ?? []; + list.push(entry.name); + byGateway.set(gatewayName, list); + } + + const overlaps: SlackGatewayOverlap[] = []; + for (const [gatewayName, names] of byGateway) { + if (names.length < 2) continue; + for (let i = 0; i < names.length; i += 1) { + for (let j = i + 1; j < names.length; j += 1) { + overlaps.push({ gatewayName, sandboxes: [names[i], names[j]] }); + } + } + } + return overlaps; +} + +/** + * The canonical operator-facing message for a Slack Socket Mode gateway + * conflict. Worded to match the issue's expected behavior (#4953): one + * sandbox per gateway receives Socket Mode events unless the gateway + * multiplexes. + */ +export function formatSlackSocketModeConflictMessage(otherSandbox: string): string { + return ( + `Slack Socket Mode is already enabled for sandbox '${otherSandbox}' on this gateway; ` + + "only one sandbox can receive Slack Socket Mode events unless the gateway supports multiplexing." + ); +} diff --git a/src/lib/messaging/applier/conflict-detection/types.ts b/src/lib/messaging/applier/conflict-detection/types.ts index fc33eaaefa3..822d7ca748b 100644 --- a/src/lib/messaging/applier/conflict-detection/types.ts +++ b/src/lib/messaging/applier/conflict-detection/types.ts @@ -44,6 +44,10 @@ export interface ConflictRegistryEntry { readonly messaging?: { readonly plan: SandboxMessagingPlan } | null; readonly messagingChannels?: readonly string[] | null; readonly disabledChannels?: readonly string[] | null; + // OpenShell gateway registration name this sandbox is bound to. Used by the + // gateway-scoped Slack Socket Mode conflict detection (#4953). A missing name + // normalizes to the default `nemoclaw` gateway (see slack-socket-mode.ts). + readonly gatewayName?: string | null; } export interface ConflictRegistry { diff --git a/src/lib/onboard.ts b/src/lib/onboard.ts index 95248e8f540..6383497cf6f 100644 --- a/src/lib/onboard.ts +++ b/src/lib/onboard.ts @@ -2920,43 +2920,32 @@ async function createSandbox( // for the current sandbox creation. const envPlan = readMessagingPlanFromEnv(); const currentPlan = envPlan?.sandboxName === sandboxName ? envPlan : null; - const hasPlanCredentials = - currentPlan?.credentialBindings.some((b) => b.credentialAvailable) ?? false; - if (hasPlanCredentials) { - const { - backfillMessagingChannels, - findChannelConflictsFromPlan, - createMessagingConflictProbe, - } = require("./messaging/applier") as typeof import("./messaging/applier"); - const probe = createMessagingConflictProbe({ - checkGatewayLiveness: () => - runOpenshell(["sandbox", "list"], { ignoreError: true, suppressOutput: true }).status === 0, - providerExists: (name) => providerExistsInGateway(name), - }); - backfillMessagingChannels(registry, probe); - const conflicts = findChannelConflictsFromPlan(sandboxName, currentPlan!, registry); - if (conflicts.length > 0) { - for (const { channel, sandbox, reason } of conflicts) { - const detail = - reason === "matching-token" - ? `uses the same ${channel} credential` - : `already has ${channel} enabled, but its credential hash is unavailable`; - console.log( - ` ⚠ Sandbox '${sandbox}' ${detail}. Shared channel credentials only allow one sandbox to poll/connect — continuing may break both bridges.`, - ); - } - if (isNonInteractive()) { - console.error( - ` Aborting: resolve the messaging channel conflict above or run \`${cliName()} channels stop \` / \`${cliName()} channels remove \` on the other sandbox.`, - ); - process.exit(1); - } - if (!(await promptYesNoOrDefault(" Continue anyway?", null, false))) { - console.log(" Aborting sandbox creation."); - process.exit(1); - } - } - } + // Drop channels the operator disabled via `nemoclaw channels stop`. + // Credentials stay in the keychain; the bridge simply isn't registered with + // the gateway on the next rebuild. `channels start` removes the entry and + // the bridge comes back. Resolved before conflict detection so a *stopped* + // channel on this sandbox is not treated as an active consumer (a stopped + // Slack bridge must not block a second sandbox on the same gateway). + const disabledChannels: string[] = + require("./onboard/channel-state").resolveDisabledChannels(sandboxName); + const disabledChannelNames = new Set(disabledChannels); + const { enforceMessagingChannelConflicts } = + require("./onboard/messaging-conflict-guard") as typeof import("./onboard/messaging-conflict-guard"); + await enforceMessagingChannelConflicts({ + sandboxName, + gatewayName: GATEWAY_NAME, + currentPlan, + currentSandboxDisabledChannels: disabledChannels, + registry, + checkGatewayLiveness: () => + runOpenshell(["sandbox", "list"], { ignoreError: true, suppressOutput: true }).status === 0, + providerExists: (name) => providerExistsInGateway(name), + isNonInteractive, + promptContinue: () => promptYesNoOrDefault(" Continue anyway?", null, false), + cliName, + log: (message) => console.log(message), + error: (message) => console.error(message), + }); // When enabledChannels is provided (from the toggle picker), only include // channels the user selected. When null (backward compat), include all. @@ -2969,13 +2958,6 @@ async function createSandbox( ) : null; - // Drop channels the operator disabled via `nemoclaw channels stop`. - // Credentials stay in the keychain; the bridge simply isn't registered with - // the gateway on the next rebuild. `channels start` removes the entry and - // the bridge comes back. - const disabledChannels: string[] = - require("./onboard/channel-state").resolveDisabledChannels(sandboxName); - const disabledChannelNames = new Set(disabledChannels); const disabledEnvKeys = new Set( MESSAGING_CHANNELS.filter((c) => disabledChannelNames.has(c.name)).flatMap((c) => getChannelTokenKeys(c), diff --git a/src/lib/onboard/messaging-conflict-guard.test.ts b/src/lib/onboard/messaging-conflict-guard.test.ts new file mode 100644 index 00000000000..60e3cb0b5f6 --- /dev/null +++ b/src/lib/onboard/messaging-conflict-guard.test.ts @@ -0,0 +1,117 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +import { describe, expect, it, vi } from "vitest"; + +import { + makePlan, + planEntry, + slackBindings, + slackChannel, +} from "../../../test/helpers/messaging-conflict-fixtures"; +import type { SandboxMessagingPlan } from "../messaging/manifest"; +import { enforceMessagingChannelConflicts } from "./messaging-conflict-guard"; + +class AbortError extends Error {} + +// Distinct per-sandbox token hashes so the credential-sharing axis stays +// silent and the gateway axis is exercised in isolation: the whole point of +// #4953 is that *different* Slack apps still collide on a shared gateway. +function slackPlan(sandboxName: string): SandboxMessagingPlan { + return makePlan(sandboxName, { + channels: [slackChannel()], + credentialBindings: slackBindings(`${sandboxName}-bot`, `${sandboxName}-app`, sandboxName), + }); +} + +function makeDeps(overrides: Record = {}) { + const log = vi.fn(); + const error = vi.fn(); + const promptContinue = vi.fn(async () => false); + const exit = vi.fn((_code: number) => { + throw new AbortError("exit"); + }) as unknown as (code: number) => never; + const otherSlack = { ...planEntry("alice", slackPlan("alice")), gatewayName: "nemoclaw" }; + const deps = { + sandboxName: "bob", + gatewayName: "nemoclaw", + currentPlan: slackPlan("bob"), + registry: { + listSandboxes: () => ({ sandboxes: [otherSlack], defaultSandbox: "alice" }), + updateSandbox: vi.fn(() => true), + }, + checkGatewayLiveness: () => false, + providerExists: () => false, + isNonInteractive: () => true, + promptContinue, + cliName: () => "nemoclaw", + log, + error, + exit, + ...overrides, + }; + return { deps, log, error, promptContinue, exit }; +} + +describe("enforceMessagingChannelConflicts — Slack Socket Mode gateway axis (#4953)", () => { + it("aborts a second Slack sandbox on the same gateway in non-interactive mode", async () => { + const { deps, log, error } = makeDeps(); + await expect(enforceMessagingChannelConflicts(deps as never)).rejects.toBeInstanceOf( + AbortError, + ); + expect(log).toHaveBeenCalledWith( + expect.stringContaining("Slack Socket Mode is already enabled for sandbox 'alice'"), + ); + expect(error).toHaveBeenCalledWith( + expect.stringContaining("only one sandbox per gateway can receive Slack Socket Mode events"), + ); + }); + + it("aborts when the interactive operator declines to continue", async () => { + const promptContinue = vi.fn(async () => false); + const { deps } = makeDeps({ isNonInteractive: () => false, promptContinue }); + await expect(enforceMessagingChannelConflicts(deps as never)).rejects.toBeInstanceOf( + AbortError, + ); + expect(promptContinue).toHaveBeenCalled(); + }); + + it("proceeds when the interactive operator overrides the conflict", async () => { + const promptContinue = vi.fn(async () => true); + const { deps, log } = makeDeps({ isNonInteractive: () => false, promptContinue }); + await expect(enforceMessagingChannelConflicts(deps as never)).resolves.toBeUndefined(); + expect(log).toHaveBeenCalledWith( + expect.stringContaining("Slack Socket Mode is already enabled for sandbox 'alice'"), + ); + }); + + it("does not warn when the only other Slack sandbox is on a different gateway", async () => { + const otherSlack = { ...planEntry("alice", slackPlan("alice")), gatewayName: "nemoclaw-9090" }; + const { deps, log, error } = makeDeps({ + registry: { + listSandboxes: () => ({ sandboxes: [otherSlack], defaultSandbox: "alice" }), + updateSandbox: vi.fn(() => true), + }, + }); + await expect(enforceMessagingChannelConflicts(deps as never)).resolves.toBeUndefined(); + expect(log).not.toHaveBeenCalled(); + expect(error).not.toHaveBeenCalled(); + }); + + it("does not block when Slack is stopped on the current sandbox (#4953)", async () => { + // currentPlan still lists slack as configured/active, but the operator has + // stopped it on this sandbox, so it must not count as a Socket Mode consumer. + const { deps, log, error } = makeDeps({ currentSandboxDisabledChannels: ["slack"] }); + await expect(enforceMessagingChannelConflicts(deps as never)).resolves.toBeUndefined(); + expect(log).not.toHaveBeenCalledWith( + expect.stringContaining("Slack Socket Mode is already enabled"), + ); + expect(error).not.toHaveBeenCalled(); + }); + + it("is a no-op when the current plan does not enable Slack", async () => { + const { deps, log } = makeDeps({ currentPlan: makePlan("bob") }); + await expect(enforceMessagingChannelConflicts(deps as never)).resolves.toBeUndefined(); + expect(log).not.toHaveBeenCalled(); + }); +}); diff --git a/src/lib/onboard/messaging-conflict-guard.ts b/src/lib/onboard/messaging-conflict-guard.ts new file mode 100644 index 00000000000..6e2d319220b --- /dev/null +++ b/src/lib/onboard/messaging-conflict-guard.ts @@ -0,0 +1,165 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +/** + * Pre-commit messaging channel conflict guard for the onboard entrypoint. + * + * Extracted from `onboard.ts` to keep that oversized entrypoint from growing + * (the codebase-growth guardrail blocks net additions to `src/lib/onboard.ts`; + * logic under `src/lib/onboard/` is allowed to grow). It bundles the two + * conflict axes that must be checked before a sandbox is committed: + * + * 1. **Credential-scoped** (`findChannelConflictsFromPlan`): another sandbox + * already uses one of this sandbox's channel credentials. Shared channel + * credentials (Telegram getUpdates, Discord gateway, Slack Socket Mode) + * only allow one consumer, so two sandboxes with the same token silently + * break both bridges (#1953). + * + * 2. **Gateway-scoped Slack Socket Mode** (`findSlackSocketModeGatewayConflicts`): + * even with distinct Slack apps/tokens, only one sandbox per OpenShell + * gateway reliably receives Socket Mode events. A second Slack sandbox on + * the same gateway is a silent black hole — NemoClaw reports its bridge + * healthy while events never arrive (#4953). This axis is independent of + * the credential check, which only catches a *shared* token. + * + * Dependencies are injected so the orchestration is unit-testable without a + * live gateway or the onboard module's global state. + */ + +import type { ConflictRegistry, ConflictRegistryEntry } from "../messaging/applier"; +import { + backfillMessagingChannels, + createMessagingConflictProbe, + findChannelConflictsFromPlan, + findSlackSocketModeGatewayConflicts, + formatSlackSocketModeConflictMessage, + getActiveChannelIdsFromPlan, +} from "../messaging/applier"; +import type { SandboxMessagingPlan } from "../messaging/manifest"; + +export interface MessagingConflictGuardDeps { + readonly sandboxName: string; + /** Resolved OpenShell gateway registration name for this onboard (#4422). */ + readonly gatewayName: string; + /** Compiled messaging plan for the current run, or null when none applies. */ + readonly currentPlan: SandboxMessagingPlan | null; + /** + * Channels this sandbox has stopped (`channels stop`). The compiled env plan + * still lists them as configured, but they will not be re-registered with the + * gateway, so a stopped channel must not count as an active consumer for + * conflict detection — e.g. a stopped Slack bridge must not block another + * sandbox on the same gateway (CodeRabbit, #4953). + */ + readonly currentSandboxDisabledChannels?: readonly string[]; + /** Registry facade: list/update sandboxes (state/registry satisfies this). */ + readonly registry: ConflictRegistry & { + listSandboxes: () => { sandboxes: ConflictRegistryEntry[]; defaultSandbox?: string | null }; + }; + /** `openshell sandbox list` succeeded — gateway answered (for backfill probe). */ + readonly checkGatewayLiveness: () => boolean; + /** Whether the named OpenShell provider exists (gateway assumed alive). */ + readonly providerExists: (name: string) => boolean; + readonly isNonInteractive: () => boolean; + /** Interactive "Continue anyway?" prompt; resolves true to proceed. */ + readonly promptContinue: () => Promise; + readonly cliName: () => string; + readonly log: (message: string) => void; + readonly error: (message: string) => void; + /** Abort the onboard. Defaults to `process.exit`; injectable for tests. */ + readonly exit?: (code: number) => never; +} + +function abort(deps: MessagingConflictGuardDeps): never { + return (deps.exit ?? ((code: number) => process.exit(code)))(1); +} + +/** + * Run both conflict axes and warn/abort/prompt as appropriate. Returns when it + * is safe (or the operator chose) to proceed; calls the injected `exit` (and + * never returns) when the operator aborts or non-interactive mode blocks. + */ +export async function enforceMessagingChannelConflicts( + deps: MessagingConflictGuardDeps, +): Promise { + const { sandboxName, registry } = deps; + + // Fold channels stopped on this sandbox into the plan's disabled set so every + // conflict axis sees the *effective* (post-`channels stop`) channel list. A + // stopped bridge is not re-registered, so it is neither a credential consumer + // (axis 1) nor a Socket Mode consumer (axis 2) and must not block another + // sandbox. Both `getActiveChannelIdsFromPlan` and `planToConflictChannelRequests` + // already honor `plan.disabledChannels`, so this single fold covers both axes + // (CodeRabbit, #4953). + const currentPlan: SandboxMessagingPlan | null = deps.currentPlan + ? { + ...deps.currentPlan, + disabledChannels: [ + ...new Set([ + ...deps.currentPlan.disabledChannels, + ...(deps.currentSandboxDisabledChannels ?? []), + ]), + ], + } + : null; + + // Axis 1: credential-scoped conflict (#1953). Only runs when the plan carries + // an available credential hash to compare; backfill first so legacy entries + // expose their active channels. + const hasPlanCredentials = + currentPlan?.credentialBindings.some((b) => b.credentialAvailable) ?? false; + if (currentPlan && hasPlanCredentials) { + const probe = createMessagingConflictProbe({ + checkGatewayLiveness: deps.checkGatewayLiveness, + providerExists: deps.providerExists, + }); + backfillMessagingChannels(registry, probe); + const conflicts = findChannelConflictsFromPlan(sandboxName, currentPlan, registry); + if (conflicts.length > 0) { + for (const { channel, sandbox, reason } of conflicts) { + const detail = + reason === "matching-token" + ? `uses the same ${channel} credential` + : `already has ${channel} enabled, but its credential hash is unavailable`; + deps.log( + ` ⚠ Sandbox '${sandbox}' ${detail}. Shared channel credentials only allow one sandbox to poll/connect — continuing may break both bridges.`, + ); + } + if (deps.isNonInteractive()) { + deps.error( + ` Aborting: resolve the messaging channel conflict above or run \`${deps.cliName()} channels stop \` / \`${deps.cliName()} channels remove \` on the other sandbox.`, + ); + abort(deps); + } + if (!(await deps.promptContinue())) { + deps.log(" Aborting sandbox creation."); + abort(deps); + } + } + } + + // Axis 2: gateway-scoped Slack Socket Mode conflict (#4953). Runs whenever the + // effective plan still enables Slack, regardless of credential availability, + // because the conflict is the shared gateway, not the shared token. + if (currentPlan && getActiveChannelIdsFromPlan(currentPlan).includes("slack")) { + const slackConflicts = findSlackSocketModeGatewayConflicts( + sandboxName, + deps.gatewayName, + registry.listSandboxes().sandboxes, + ); + if (slackConflicts.length > 0) { + for (const { sandbox } of slackConflicts) { + deps.log(` ⚠ ${formatSlackSocketModeConflictMessage(sandbox)}`); + } + if (deps.isNonInteractive()) { + deps.error( + ` Aborting: only one sandbox per gateway can receive Slack Socket Mode events. Run \`${deps.cliName()} channels stop slack\` / \`${deps.cliName()} channels remove slack\` on the other sandbox, or onboard this sandbox on a separate gateway (set NEMOCLAW_GATEWAY_PORT).`, + ); + abort(deps); + } + if (!(await deps.promptContinue())) { + deps.log(" Aborting sandbox creation."); + abort(deps); + } + } + } +} diff --git a/src/lib/status-command-deps.ts b/src/lib/status-command-deps.ts index db1ed6ea42e..f1081d17712 100644 --- a/src/lib/status-command-deps.ts +++ b/src/lib/status-command-deps.ts @@ -2,16 +2,19 @@ // SPDX-License-Identifier: Apache-2.0 import { spawnSync } from "node:child_process"; - -import { getNamedGatewayLifecycleState } from "./gateway-runtime-action"; -import { getLiveGatewayInference } from "./inference/live"; -import type { GatewayHealth, MessagingBridgeHealth, ShowStatusCommandDeps } from "./inventory"; -import { backfillMessagingChannels, findAllOverlaps } from "./messaging/applier"; import type { CaptureOpenshellResult } from "./adapters/openshell/client"; import { captureOpenshellCommand, stripAnsi } from "./adapters/openshell/client"; +import { resolveOpenshell } from "./adapters/openshell/resolve"; import { OPENSHELL_PROBE_TIMEOUT_MS } from "./adapters/openshell/timeouts"; +import { getNamedGatewayLifecycleState } from "./gateway-runtime-action"; +import { getLiveGatewayInference } from "./inference/live"; +import type { GatewayHealth, MessagingBridgeHealth, ShowStatusCommandDeps } from "./inventory"; +import { + backfillMessagingChannels, + detectAllSlackSocketModeGatewayOverlaps, + findAllOverlaps, +} from "./messaging/applier"; import * as registry from "./state/registry"; -import { resolveOpenshell } from "./adapters/openshell/resolve"; import { createSystemDeps, parseSshProcesses } from "./state/sandbox-session"; import { getServiceStatuses, showStatus as showServiceStatus } from "./tunnel/services"; @@ -102,7 +105,24 @@ function backfillAndFindOverlaps(rootDir: string) { // registry write throws, so any failure yields an empty overlap list. try { backfillMessagingChannels(registry, makeConflictProbe(rootDir)); - return findAllOverlaps(registry); + // Report both conflict axes independently and without deduping. They are + // distinct, both-true facts: a shared messaging credential conflicts on any + // gateway (the gateway-independent, more actionable signal), while two Slack + // sandboxes on one gateway conflict even with distinct tokens (#4953). A + // pair that hits both genuinely has two problems, so surfacing both avoids + // masking the credential warning behind the gateway one. + const credentialOverlaps = findAllOverlaps(registry); + const slackGatewayOverlaps = detectAllSlackSocketModeGatewayOverlaps( + registry.listSandboxes().sandboxes, + ); + return [ + ...credentialOverlaps, + ...slackGatewayOverlaps.map((o) => ({ + channel: "slack", + sandboxes: o.sandboxes, + reason: "slack-socket-mode-gateway" as const, + })), + ]; } catch { return []; }