fix(onboard): preserve disabled channels through rebuild so channels start can recover (#3395)

laitingsheng · jyaunches · prekshivyas · web-flow · commit cf3fd66aafe4 · 2026-05-12T21:01:28.000-07:00
## Summary `nemoclaw <sandbox> channels start <name>` reported success but left the bot offline whenever it ran after a `channels stop <name>` + rebuild. The stop path correctly disabled the bridge, but the subsequent rebuild silently dropped the channel from the registry's configured set, leaving a later `channels start` with nothing to re-enable. ## Related Issue Fixes #3381 ## Changes - `src/lib/onboard.ts`: persist the operator's configured channel set on the new registry entry (input `enabledChannels`), not the post-disabled-filter `activeMessagingChannels`. `disabledChannels` remains the runtime modifier; the baked image and the gateway provider attachments still respect it. - `test/onboard.test.ts`: regression test that drives `createSandbox` with `disabledChannels: ["telegram"]` already in the registry and asserts the new `registerSandbox` payload keeps `messagingChannels: ["telegram"]` while the Dockerfile build arg and `--provider` flags continue to exclude the disabled channel. ## Type of Change - [x] Code change (feature, bug fix, or refactor) - [ ] Code change with doc updates - [ ] Doc only (prose changes, no code sample modifications) - [ ] Doc only (includes code sample changes) ## Verification - [x] `npx prek run --all-files` passes - [x] `npm test` passes - [x] Tests added or updated for new or changed behavior - [x] No secrets, API keys, or credentials committed - [ ] Docs updated for user-facing behavior changes - [ ] `make docs` builds without warnings (doc changes only) - [ ] Doc pages follow the [style guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md) (doc changes only) - [ ] New doc pages include SPDX header and frontmatter (new pages only) --- Signed-off-by: Tinson Lai <tinsonl@nvidia.com>  ## Summary by CodeRabbit * **Bug Fixes** * Messaging channels you configure are now preserved when recreating a sandbox, allowing them to be re-enabled in subsequent rebuilds. * Disabled messaging channels no longer incorrectly prevent other sandboxes from using the same credential token. [![Review Change Stack](https://storage.googleapis.com/coderabbit_public_assets/review-stack-in-coderabbit-ui.svg)](https://app.coderabbit.ai/change-stack/NVIDIA/NemoClaw/pull/3395)  --------- Signed-off-by: Tinson Lai <tinsonl@nvidia.com> Co-authored-by: J. Yaunches <jyaunches@nvidia.com> Co-authored-by: Prekshi Vyas <34834085+prekshivyas@users.noreply.github.com>
diff --git a/src/lib/messaging-conflict.test.ts b/src/lib/messaging-conflict.test.ts
@@ -93,6 +93,24 @@ describe("findChannelConflicts", () => {
     const registry = makeRegistry([{ name: "alice", messagingChannels: ["telegram"] }]);
     expect(findChannelConflicts("bob", [], registry)).toEqual([]);
   });
+
+  it("ignores a stopped (disabled) channel — its credential is not in use (#3381)", () => {
+    const registry = makeRegistry([
+      {
+        name: "alice",
+        messagingChannels: ["telegram"],
+        disabledChannels: ["telegram"],
+        providerCredentialHashes: { TELEGRAM_BOT_TOKEN: "hash-a" },
+      },
+    ]);
+    expect(
+      findChannelConflicts(
+        "bob",
+        [{ channel: "telegram", credentialHashes: { TELEGRAM_BOT_TOKEN: "hash-a" } }],
+        registry,
+      ),
+    ).toEqual([]);
+  });
 });
 
 describe("findAllOverlaps", () => {
@@ -161,6 +179,23 @@ describe("findAllOverlaps", () => {
     ]);
     expect(findAllOverlaps(registry)).toEqual([]);
   });
+
+  it("ignores stopped (disabled) channels so nemoclaw status does not report phantom overlaps (#3381)", () => {
+    const registry = makeRegistry([
+      {
+        name: "alice",
+        messagingChannels: ["telegram"],
+        disabledChannels: ["telegram"],
+        providerCredentialHashes: { TELEGRAM_BOT_TOKEN: "hash-a" },
+      },
+      {
+        name: "bob",
+        messagingChannels: ["telegram"],
+        providerCredentialHashes: { TELEGRAM_BOT_TOKEN: "hash-a" },
+      },
+    ]);
+    expect(findAllOverlaps(registry)).toEqual([]);
+  });
 });
 
 describe("backfillMessagingChannels", () => {
diff --git a/src/lib/messaging-conflict.ts b/src/lib/messaging-conflict.ts
@@ -70,7 +70,13 @@ function getTokenKeys(channel: string): string[] {
 }
 
 function hasStoredChannel(entry: SandboxEntry, channel: string): boolean {
-  return Array.isArray(entry.messagingChannels) && entry.messagingChannels.includes(channel);
+  if (!Array.isArray(entry.messagingChannels) || !entry.messagingChannels.includes(channel)) {
+    return false;
+  }
+  // A `channels stop` sandbox keeps the channel in messagingChannels so a later
+  // `channels start` can recover, but its bridge is paused — the credential is
+  // not in use, so it must not block another sandbox from claiming the token.
+  return !(Array.isArray(entry.disabledChannels) && entry.disabledChannels.includes(channel));
 }
 
 function conflictReasonForRequest(
@@ -198,7 +204,11 @@ export function findAllOverlaps(registry: ConflictRegistry): Array<{
   const byChannel = new Map<string, SandboxEntry[]>();
   for (const entry of sandboxes) {
     if (!Array.isArray(entry.messagingChannels)) continue;
+    const disabled = new Set(
+      Array.isArray(entry.disabledChannels) ? entry.disabledChannels : [],
+    );
     for (const channel of entry.messagingChannels) {
+      if (disabled.has(channel)) continue;
       const list = byChannel.get(channel) || [];
       list.push(entry);
       byChannel.set(channel, list);
diff --git a/src/lib/onboard.ts b/src/lib/onboard.ts
@@ -6324,7 +6324,13 @@ async function createSandbox(
     providerCredentialHashes:
       Object.keys(providerCredentialHashes).length > 0 ? providerCredentialHashes : undefined,
     policies: initialSandboxPolicy.appliedPresets,
-    messagingChannels: activeMessagingChannels,
+    // Persist the operator's configured channel set, not the post-disabled-filter
+    // active set. After `channels stop X` + rebuild, activeMessagingChannels drops
+    // X, but X is still configured — losing it here means a later `channels start
+    // X` has nothing to re-enable (the next rebuild sees an empty channel set and
+    // never reattaches the gateway bridge). See #3381.
+    messagingChannels:
+      enabledChannels != null ? [...new Set(enabledChannels)] : activeMessagingChannels,
     messagingChannelConfig: messagingChannelConfig || undefined,
     disabledChannels: disabledChannels.length > 0 ? [...disabledChannels] : undefined,
     dashboardPort: actualDashboardPort,
diff --git a/test/onboard.test.ts b/test/onboard.test.ts
@@ -6271,6 +6271,165 @@ const { createSandbox } = require(${onboardPath});
     },
   );
 
+  it(
+    "preserves disabled channels in the registry after a recreate so `channels start` can re-enable them (#3381)",
+    { timeout: 60_000 },
+    async () => {
+      const repoRoot = path.join(import.meta.dirname, "..");
+      const tmpDir = fs.mkdtempSync(
+        path.join(os.tmpdir(), "nemoclaw-onboard-disabled-channels-preserve-"),
+      );
+      const fakeBin = path.join(tmpDir, "bin");
+      const scriptPath = path.join(tmpDir, "disabled-channels-preserve.js");
+      const onboardPath = JSON.stringify(path.join(repoRoot, "dist", "lib", "onboard.js"));
+      const runnerPath = JSON.stringify(path.join(repoRoot, "dist", "lib", "runner.js"));
+      const registryPath = JSON.stringify(path.join(repoRoot, "dist", "lib", "state", "registry.js"));
+      const preflightPath = JSON.stringify(path.join(repoRoot, "dist", "lib", "onboard", "preflight.js"));
+      const credentialsPath = JSON.stringify(path.join(repoRoot, "dist", "lib", "credentials", "store.js"));
+
+      fs.mkdirSync(fakeBin, { recursive: true });
+      fs.writeFileSync(path.join(fakeBin, "openshell"), "#!/usr/bin/env bash\nexit 0\n", {
+        mode: 0o755,
+      });
+
+      const script = String.raw`
+const runner = require(${runnerPath});
+const _n = (c) => (Array.isArray(c) ? c.join(" ") : String(c)).replace(/'/g, "");
+const registry = require(${registryPath});
+const preflight = require(${preflightPath});
+const credentials = require(${credentialsPath});
+const childProcess = require("node:child_process");
+const { EventEmitter } = require("node:events");
+const fs = require("node:fs");
+
+const commands = [];
+const registerCalls = [];
+registry.registerSandbox({
+  name: "my-assistant",
+  messagingChannels: ["telegram"],
+  disabledChannels: ["telegram"],
+  providerCredentialHashes: { TELEGRAM_BOT_TOKEN: "hash-telegram" },
+});
+runner.run = (command, opts = {}) => {
+  const normalized = _n(command);
+  commands.push({ command: normalized, env: opts.env || null });
+  if (normalized.includes("provider get my-assistant-telegram-bridge")) return { status: 0 };
+  if (normalized.includes("provider get")) return { status: 1 };
+  return { status: 0 };
+};
+runner.runCapture = (command) => {
+  if (_n(command).includes("sandbox get my-assistant")) return "";
+  if (_n(command).includes("sandbox list")) return "my-assistant Ready";
+  {
+    const sandboxExecCurl = require(${onboardScriptMocksPath}).mockSandboxExecCurl(command);
+    if (sandboxExecCurl !== null) return sandboxExecCurl;
+  }
+  if (_n(command).includes("forward list")) return "my-assistant 127.0.0.1 18789 12345 running";
+  return "";
+};
+registry.registerSandbox = (entry) => {
+  registerCalls.push(entry);
+  return true;
+};
+registry.updateSandbox = () => true;
+registry.setDefault = () => true;
+registry.removeSandbox = () => true;
+preflight.checkPortAvailable = async () => ({ ok: true });
+credentials.prompt = async () => "";
+
+childProcess.spawn = (...args) => {
+  const child = new EventEmitter();
+  child.stdout = new EventEmitter();
+  child.stderr = new EventEmitter();
+  const command = _n(args[1][1]);
+  const entry = { command, env: args[2]?.env || null };
+  const dockerfileMatch = command.match(/--from ([^ ]+Dockerfile)/);
+  if (dockerfileMatch) {
+    try {
+      entry.dockerfileContent = fs.readFileSync(dockerfileMatch[1], "utf-8");
+    } catch (error) {
+      entry.dockerfileReadError = String(error);
+    }
+  }
+  commands.push(entry);
+  process.nextTick(() => {
+    child.stdout.emit("data", Buffer.from("Created sandbox: my-assistant\n"));
+    child.emit("close", 0);
+  });
+  return child;
+};
+
+const { createSandbox } = require(${onboardPath});
+
+(async () => {
+  process.env.OPENSHELL_GATEWAY = "nemoclaw";
+  delete process.env.TELEGRAM_BOT_TOKEN;
+  const sandboxName = await createSandbox(
+    null, "gpt-5.4", "nvidia-prod", null, "my-assistant", null, ["telegram"],
+  );
+  console.log(JSON.stringify({ sandboxName, commands, registerCalls }));
+})().catch((error) => {
+  console.error(error);
+  process.exit(1);
+});
+`;
+      fs.writeFileSync(scriptPath, script);
+
+      const result = spawnSync(process.execPath, [scriptPath], {
+        cwd: repoRoot,
+        encoding: "utf-8",
+        env: {
+          ...process.env,
+          HOME: tmpDir,
+          PATH: `${fakeBin}:${process.env.PATH || ""}`,
+          NEMOCLAW_NON_INTERACTIVE: "1",
+          TELEGRAM_BOT_TOKEN: "",
+        },
+      });
+
+      assert.equal(result.status, 0, result.stderr);
+      const payloadLine = result.stdout
+        .trim()
+        .split("\n")
+        .slice()
+        .reverse()
+        .find((line) => line.startsWith("{") && line.endsWith("}"));
+      assert.ok(payloadLine, `expected JSON payload in stdout:\n${result.stdout}`);
+      const payload = JSON.parse(payloadLine);
+
+      const createCommand = payload.commands.find((entry: CommandEntry) =>
+        entry.command.includes("sandbox create"),
+      );
+      assert.ok(createCommand, "expected sandbox create command");
+      assert.equal(createCommand.dockerfileReadError, undefined);
+
+      const channelsLine = createCommand.dockerfileContent
+        ?.split("\n")
+        .find((line: string) => line.startsWith("ARG NEMOCLAW_MESSAGING_CHANNELS_B64="));
+      assert.ok(channelsLine, "expected messaging build arg in Dockerfile");
+      const bakedChannels = JSON.parse(
+        Buffer.from(channelsLine.split("=")[1], "base64").toString(),
+      );
+      assert.deepEqual(bakedChannels, [], "disabled channel must not be baked into the image");
+      assert.doesNotMatch(
+        createCommand.command,
+        /--provider my-assistant-telegram-bridge/,
+        "disabled channel's bridge must not be attached to the new sandbox",
+      );
+
+      assert.deepEqual(
+        payload.registerCalls[0]?.messagingChannels,
+        ["telegram"],
+        "registry.messagingChannels must keep the disabled-but-configured channel so `channels start` can recover it",
+      );
+      assert.deepEqual(
+        payload.registerCalls[0]?.disabledChannels,
+        ["telegram"],
+        "registry.disabledChannels must round-trip through the rebuild",
+      );
+    },
+  );
+
   it("aborts onboard when a messaging provider upsert fails", { timeout: 60_000 }, async () => {
     const repoRoot = path.join(import.meta.dirname, "..");
     const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), "nemoclaw-onboard-provider-fail-"));
