fix: resolve four bugs from issue #258

package.json

@@ -1,6 +1,6 @@
 {
   "name": "@openacp/slack-adapter",
-  "version": "2026.506.2",
+  "version": "2026.525.1",
   "description": "Slack messaging platform adapter plugin for OpenACP",
   "type": "module",
   "main": "dist/index.js",

src/__tests__/event-router.test.ts

@@ -181,52 +181,6 @@ describe("SlackEventRouter", () => {
     expect(onIncoming).not.toHaveBeenCalled();
   });
 
-  it("falls back to global allowedUserIds when Slack-specific list is empty", async () => {
-    const onIncoming = vi.fn();
-    const onNewSession = vi.fn();
-    const sessionLookup = vi.fn().mockReturnValue(undefined);
-    const router = new SlackEventRouter(
-      sessionLookup,
-      onIncoming,
-      "BOT1",
-      "NOTIF",
-      onNewSession,
-      makeConfig({ allowedUserIds: [] }),
-      ["U_GLOBAL_ALLOWED"],
-    );
-    const app = createMockApp();
-    router.register(app as any);
-
-    await app._trigger("message", { message: { channel: "NOTIF", user: "U_RANDOM", text: "hello" } });
-    expect(onNewSession).not.toHaveBeenCalled();
-
-    await app._trigger("message", { message: { channel: "NOTIF", user: "U_GLOBAL_ALLOWED", text: "hello" } });
-    expect(onNewSession).toHaveBeenCalledWith("hello", "U_GLOBAL_ALLOWED");
-  });
-
-  it("prefers Slack-specific list over global list when both are set", async () => {
-    const onIncoming = vi.fn();
-    const onNewSession = vi.fn();
-    const sessionLookup = vi.fn().mockReturnValue(undefined);
-    const router = new SlackEventRouter(
-      sessionLookup,
-      onIncoming,
-      "BOT1",
-      "NOTIF",
-      onNewSession,
-      makeConfig({ allowedUserIds: ["U_SLACK_ONLY"] }),
-      ["U_GLOBAL_ONLY"],
-    );
-    const app = createMockApp();
-    router.register(app as any);
-
-    await app._trigger("message", { message: { channel: "NOTIF", user: "U_GLOBAL_ONLY", text: "hello" } });
-    expect(onNewSession).not.toHaveBeenCalled();
-
-    await app._trigger("message", { message: { channel: "NOTIF", user: "U_SLACK_ONLY", text: "hello" } });
-    expect(onNewSession).toHaveBeenCalledWith("hello", "U_SLACK_ONLY");
-  });
-
   it("ignores thread replies", async () => {
     const onIncoming = vi.fn();
     const onNewSession = vi.fn();
@@ -252,7 +206,6 @@ describe("SlackEventRouter", () => {
       "NOTIF",
       onNewSession,
       makeConfig({ allowedUserIds: [] }),
-      [],
     );
     const app = createMockApp();
     router.register(app as any);

src/__tests__/thread-ready-persistence.test.ts

@@ -0,0 +1,87 @@
+import { describe, expect, it, vi } from "vitest";
+import { makeThreadReadyHandler } from "../adapter.js";
+import type { SlackSessionMeta } from "../types.js";
+
+/**
+ * Regression test for issue #258 bug 4: "No Slack channel for session".
+ *
+ * Core invokes `createSessionThread("", name)` BEFORE the real sessionId is
+ * assigned. Before this fix, the adapter did `this.sessions.set("", meta)` and
+ * `patchRecord("", { platform: { channelId } })` — both no-ops against the
+ * real session record. After restart, `tryRestoreSessionFromRecord` returned
+ * early because `platform.channelId` was missing.
+ *
+ * Fix: pre-created channels are stashed by slug in `_pendingChannelsBySlug`
+ * and re-keyed onto the real sessionId when `SESSION_THREAD_READY` fires.
+ *
+ * These tests import the actual production handler factory so they catch
+ * regressions where the handler logic diverges.
+ */
+describe("issue #258 bug 4: makeThreadReadyHandler", () => {
+  function setup() {
+    const sessions = new Map<string, SlackSessionMeta>();
+    const pendingChannelsBySlug = new Map<string, SlackSessionMeta>();
+    const patchRecord = vi.fn().mockResolvedValue(undefined);
+    const onError = vi.fn();
+    const handler = makeThreadReadyHandler({
+      sessions,
+      pendingChannelsBySlug,
+      patchRecord,
+      onError,
+    });
+    return { sessions, pendingChannelsBySlug, patchRecord, onError, handler };
+  }
+
+  it("re-keys pending channel onto real sessionId and persists channelId+threadId", () => {
+    const { sessions, pendingChannelsBySlug, patchRecord, handler } = setup();
+    const slug = "openacp-fix-bug-abcd";
+    const channelId = "C0123";
+    pendingChannelsBySlug.set(slug, { channelId, channelSlug: slug });
+
+    handler({ sessionId: "sess-real-1", channelId: "slack", threadId: slug });
+
+    expect(sessions.get("sess-real-1")).toEqual({ channelId, channelSlug: slug });
+    expect(pendingChannelsBySlug.has(slug)).toBe(false);
+    expect(patchRecord).toHaveBeenCalledWith("sess-real-1", {
+      platform: { channelId, threadId: slug },
+    });
+  });
+
+  it("ignores events from other adapters (channelId !== 'slack')", () => {
+    const { sessions, pendingChannelsBySlug, patchRecord, handler } = setup();
+    pendingChannelsBySlug.set("openacp-x", { channelId: "C1", channelSlug: "openacp-x" });
+
+    handler({ sessionId: "sess-1", channelId: "telegram", threadId: "openacp-x" });
+
+    expect(sessions.size).toBe(0);
+    expect(pendingChannelsBySlug.size).toBe(1);
+    expect(patchRecord).not.toHaveBeenCalled();
+  });
+
+  it("no-ops when slug is not pending (e.g. startup reuse path emits its own event)", () => {
+    const { sessions, patchRecord, handler } = setup();
+
+    handler({ sessionId: "sess-1", channelId: "slack", threadId: "startup-12345678" });
+
+    expect(sessions.size).toBe(0);
+    expect(patchRecord).not.toHaveBeenCalled();
+  });
+
+  it("invokes onError if patchRecord rejects", async () => {
+    const sessions = new Map<string, SlackSessionMeta>();
+    const pendingChannelsBySlug = new Map<string, SlackSessionMeta>();
+    const patchErr = new Error("disk full");
+    const patchRecord = vi.fn().mockRejectedValue(patchErr);
+    const onError = vi.fn();
+    const handler = makeThreadReadyHandler({ sessions, pendingChannelsBySlug, patchRecord, onError });
+    pendingChannelsBySlug.set("openacp-x", { channelId: "C1", channelSlug: "openacp-x" });
+
+    handler({ sessionId: "sess-1", channelId: "slack", threadId: "openacp-x" });
+    // Let the rejection propagate through the .catch chain
+    await new Promise((r) => setImmediate(r));
+
+    expect(onError).toHaveBeenCalledWith(patchErr, "sess-1");
+    // Even on persist failure, sessions Map is still updated so in-memory routing works
+    expect(sessions.get("sess-1")).toBeDefined();
+  });
+});

src/adapter.ts

@@ -37,9 +37,50 @@ import { SlackActivityTracker } from "./activity-tracker.js";
 import { toSlug } from "./slug.js";
 import { isAudioClip } from "./utils.js";
 
+/** Payload of `SESSION_THREAD_READY` emitted by core after a session+thread is created. */
+export interface SessionThreadReadyPayload {
+  sessionId: string;
+  channelId: string;
+  threadId: string;
+}
+
+/**
+ * Dependencies needed by the `SESSION_THREAD_READY` handler. Extracted so the
+ * handler is unit-testable without booting the full SlackAdapter (Bolt App is
+ * heavy and would dwarf the test).
+ */
+export interface ThreadReadyDeps {
+  sessions: Map<string, SlackSessionMeta>;
+  pendingChannelsBySlug: Map<string, SlackSessionMeta>;
+  patchRecord(sessionId: string, patch: { platform: { channelId: string; threadId: string } }): Promise<void>;
+  onError?(err: unknown, sessionId: string): void;
+}
+
+/**
+ * Build the `SESSION_THREAD_READY` handler. Re-keys a pre-created channel
+ * (stashed by slug in `pendingChannelsBySlug`) onto the real sessionId and
+ * persists `platform.channelId` so the session can be restored after restart.
+ *
+ * Exported for direct testing — see `__tests__/thread-ready-persistence.test.ts`.
+ */
+export function makeThreadReadyHandler(
+  deps: ThreadReadyDeps,
+): (payload: SessionThreadReadyPayload) => void {
+  return (payload) => {
+    if (payload.channelId !== "slack") return;
+    const meta = deps.pendingChannelsBySlug.get(payload.threadId);
+    if (!meta) return;
+    deps.pendingChannelsBySlug.delete(payload.threadId);
+    deps.sessions.set(payload.sessionId, meta);
+    deps.patchRecord(payload.sessionId, {
+      platform: { channelId: meta.channelId, threadId: payload.threadId },
+    }).catch((err) => deps.onError?.(err, payload.sessionId));
+  };
+}
+
 /** Minimal interface for the core kernel, accessed via ctx.kernel */
 interface CoreKernel {
-  configManager: { get(): { security: { allowedUserIds: string[] } } };
+  configManager: { get(): Record<string, unknown> };
   lifecycleManager?: {
     serviceRegistry?: { get(name: string): unknown };
   };
@@ -58,6 +99,10 @@ interface CoreKernel {
   fileService: FileServiceInterface;
   handleMessage(msg: { channelId: string; threadId: string; userId: string; text: string; attachments?: Attachment[] }): Promise<void>;
   handleNewSession(platform: string, userId?: string, text?: string, opts?: { createThread: boolean }): Promise<{ id: string; threadId?: string }>;
+  eventBus?: {
+    on(event: string, handler: (payload: SessionThreadReadyPayload) => void): void;
+    off(event: string, handler: (payload: SessionThreadReadyPayload) => void): void;
+  };
 }
 
 export class SlackAdapter extends MessagingAdapter {
@@ -87,6 +132,14 @@ export class SlackAdapter extends MessagingAdapter {
   private _skillCommandsTs = new Map<string, string>();
   /** Commands queued before a session channel was ready */
   private _pendingSkillCommands = new Map<string, AgentCommand[]>();
+  /**
+   * Channel meta keyed by slug for channels created BEFORE the real sessionId is
+   * known. Core calls `createSessionThread("", name)` because session.id isn't
+   * assigned yet; we stash the meta here and re-key into `sessions` once
+   * `SESSION_THREAD_READY` fires with the real sessionId.
+   */
+  private _pendingChannelsBySlug = new Map<string, SlackSessionMeta>();
+  private _threadReadyHandler?: (payload: SessionThreadReadyPayload) => void;
   private adapterDefaultOutputMode: OutputMode | undefined;
   private botUserId = "";
   private slackConfig: SlackChannelConfig;
@@ -314,11 +367,30 @@ export class SlackAdapter extends MessagingAdapter {
         }
       },
       this.slackConfig,
-      this.core.configManager.get().security.allowedUserIds,
       this.log,
     );
     this.eventRouter.register(this.app);
 
+    // Re-key pre-created channels onto the real sessionId once core emits
+    // SESSION_THREAD_READY. Without this, channels created via
+    // `createSessionThread("", name)` would be orphaned in the sessions Map
+    // and never persist channelId — causing "No Slack channel for session"
+    // after restart.
+    this._threadReadyHandler = makeThreadReadyHandler({
+      sessions: this.sessions,
+      pendingChannelsBySlug: this._pendingChannelsBySlug,
+      patchRecord: (sessionId, patch) => this.core.sessionManager.patchRecord(sessionId, patch),
+      onError: (err, sessionId) => this.log.warn({ err, sessionId }, "Failed to persist Slack channelId"),
+    });
+    if (this.core.eventBus) {
+      this.core.eventBus.on("session:threadReady", this._threadReadyHandler);
+    } else {
+      // Without eventBus, channels created via `createSessionThread("", ...)` never
+      // get re-keyed onto the real sessionId — bug 4 silently reoccurs. Surface this
+      // loudly so it's caught in dev, not in a user's session weeks later.
+      this.log.warn("core.eventBus is not available — Slack sessions created via createThread:true will not persist channelId across restarts");
+    }
+
     // Start Bolt (Socket Mode)
     await this.app.start();
     this.log.info("Slack adapter started (Socket Mode)");
@@ -437,6 +509,10 @@ export class SlackAdapter extends MessagingAdapter {
   }
 
   async stop(): Promise<void> {
+    if (this._threadReadyHandler) {
+      this.core.eventBus?.off("session:threadReady", this._threadReadyHandler);
+      this._threadReadyHandler = undefined;
+    }
     // Cleanup all activity trackers
     for (const [sessionId, tracker] of this.sessionTrackers) {
       try {
@@ -466,14 +542,23 @@ export class SlackAdapter extends MessagingAdapter {
 
   async createSessionThread(sessionId: string, name: string): Promise<string> {
     const meta = await this.channelManager.createChannel(sessionId, name);
-    this.sessions.set(sessionId, meta);
+
+    if (sessionId) {
+      // Direct caller knew the sessionId — register and persist immediately.
+      // The startup reuse path bypasses this method (createThread:false), so this
+      // branch is reserved for any future caller that already has session.id.
+      this.sessions.set(sessionId, meta);
+      await this.core.sessionManager.patchRecord(sessionId, {
+        platform: { channelId: meta.channelId, threadId: meta.channelSlug },
+      });
+    } else {
+      // Core invokes us with sessionId="" before session.id is assigned. Stash
+      // the meta by slug; SESSION_THREAD_READY will re-key it once the real
+      // sessionId is known, and patchRecord then persists channelId.
+      this._pendingChannelsBySlug.set(meta.channelSlug, meta);
+    }
+
     this.log.info({ sessionId, channelId: meta.channelId, slug: meta.channelSlug }, "Session channel created");
-    // Persist Slack-specific channelId (C01234...) so it can be recovered after restart.
-    // Core will also patchRecord with platform.threadId after this returns; it merges,
-    // so the final record will have both channelId and threadId.
-    await this.core.sessionManager.patchRecord(sessionId, {
-      platform: { channelId: meta.channelId },
-    });
     // Return the slug as the threadId so that lookups via getSessionByThread work
     return meta.channelSlug;
   }

src/event-router.ts

@@ -42,15 +42,15 @@ export class SlackEventRouter implements ISlackEventRouter {
     private notificationChannelId: string | undefined,
     private onNewSession: NewSessionCallback,
     private config: SlackChannelConfig,
-    private globalAllowedUserIds: string[] = [],
     logger?: Logger,
   ) {
     this.log = logger ?? { info() {}, warn() {}, error() {}, debug() {} };
   }
 
+  // Empty allowedUserIds means "allow all" — matches Telegram convention and what
+  // the setup wizard advertises ("press Enter to allow all").
   private isAllowedUser(userId: string): boolean {
-    const slackAllowed = this.config.allowedUserIds ?? [];
-    const allowed = slackAllowed.length > 0 ? slackAllowed : this.globalAllowedUserIds;
+    const allowed = this.config.allowedUserIds ?? [];
     if (allowed.length === 0) return true;
     return allowed.includes(userId);
   }

src/index.ts

@@ -5,7 +5,7 @@ function createSlackPlugin(): OpenACPPlugin {
   let adapter: { stop(): Promise<void> } | null = null
 
   return {
-    name: '@openacp/slack',
+    name: '@openacp/slack-adapter',
     version: '1.0.0',
     description: 'Slack adapter with channels and threads',
     essential: false,