Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
126 changes: 126 additions & 0 deletions src/lib/actions/sandbox/policy-channel-conflict.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
});
});
64 changes: 64 additions & 0 deletions src/lib/actions/sandbox/policy-channel.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<boolean> {
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} <sandbox> 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
Expand Down Expand Up @@ -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
Expand Down
28 changes: 28 additions & 0 deletions src/lib/inventory/index.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
11 changes: 10 additions & 1 deletion src/lib/inventory/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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`
Expand Down
124 changes: 124 additions & 0 deletions src/lib/messaging/applier/conflict-detection-slack-gateway.test.ts
Original file line number Diff line number Diff line change
@@ -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.",
);
});
});
1 change: 1 addition & 0 deletions src/lib/messaging/applier/conflict-detection/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Loading
Loading