fix(slack): refresh identity caches on user_change/team_join events

src/slack/directory.ts

@@ -65,7 +65,16 @@ export interface Directory {
   getUserSnapshot(client: any): Promise<{ byId: Map<string, CachedUser>; fetchedAt: number } | undefined>;
   forceDirectorySync(client: any, invalidateChannelId?: string, invalidatePrincipalId?: string): Promise<void>;
   classifyUserCached(client: any, userId: string): Promise<CachedUser & { ok: boolean }>;
-  classifyActor(client: any, userId: string): Promise<ActorAssertion>;
+  lastKnownClassification(userId: string): ActorAssertion | undefined;
+  /** Apply a `user_change`/`team_join` event payload to the caches (#626):
+   *  re-classify from the fresh profile and overwrite BOTH the LRU entry
+   *  and the snapshot's row, so a profile/email change propagates
+   *  immediately instead of waiting out the TTL. */
+  updateUserFromEvent(user: SlackUser): void;
+  classifyActor(
+    client: any,
+    userId: string,
+  ): Promise<ActorAssertion & { ok: boolean }>;
   getChannelInfo(client: any, channel: string): Promise<ChannelMeta | undefined>;
   channelMembership(
     client: any,
@@ -588,12 +597,44 @@ export function createDirectory(deps: {
       if (ids.ownTeamId) userCache.set(userId, classified);
       return { ...classified, ok: true };
     } catch {
-      return { actor: { externalId: userId, isExternalGuest: true }, ok: false };
+      // Lookup failed — say so instead of asserting the user is an external
+      // guest (#626): the id-based actor carries lookupFailed so consumers
+      // can distinguish "couldn't check" from every checked state.
+      return { actor: { externalId: userId, lookupFailed: true }, ok: false };
+    }
+  }
+
+  function updateUserFromEvent(user: SlackUser): void {
+    // The event carries the FULL fresh profile — classify from it directly
+    // (no users.info round-trip) and overwrite every copy the caches hold.
+    // The LRU entry is the transient-lookup cache; the snapshot row feeds
+    // roster/membership resolution. Both must move together or a name/email
+    // change shows up in one surface and not the other until the TTLs lapse
+    // (#626 defect 3).
+    const actor = classifyUser(user, ids.ownTeamId, ids.identityMode);
+    const timezone = slackUserTimezone(user);
+    const classified = { actor, ...(timezone ? { timezone } : {}) };
+    if (user.id) {
+      userCache.set(user.id, classified);
+      if (userSnapshot) userSnapshot.byId.set(user.id, classified);
     }
   }
 
-  async function classifyActor(client: any, userId: string): Promise<ActorAssertion> {
-    return (await classifyUserCached(client, userId)).actor;
+  function lastKnownClassification(userId: string): ActorAssertion | undefined {
+    // The LRU entry is the last SUCCESSFUL classification (failures are
+    // never cached), i.e. exactly the fallback a transient users.info
+    // failure should fall back to (#626). Time-bounded by the cache TTL.
+    return userCache.get(userId)?.actor;
+  }
+
+  async function classifyActor(client: any, userId: string): Promise<ActorAssertion & { ok: boolean }> {
+    // ok preserved, not discarded: a failed users.info currently reads as a
+    // confident "external guest" to every caller — transiently refusing a
+    // member whose record is merely unreachable. Callers decide what a failed
+    // lookup means (skip, retry, fall back to last-known); the classification
+    // itself must not launder the failure into an identity claim (#626).
+    const { actor, ok } = await classifyUserCached(client, userId);
+    return { ...actor, ok };
   }
 
   async function getChannelInfo(client: any, channel: string): Promise<ChannelMeta | undefined> {
@@ -658,6 +699,8 @@ export function createDirectory(deps: {
     forceDirectorySync,
     classifyUserCached,
     classifyActor,
+    lastKnownClassification,
+    updateUserFromEvent,
     getChannelInfo,
     channelMembership,
     allInternalRosters,

src/slack/events.ts

@@ -1,3 +1,4 @@
+import type { SlackUser } from "./identity.ts";
 import {
   type SlackFile,
   channelPrivacyChange,
@@ -200,6 +201,26 @@ export function registerSlackEvents(
     }
   });
 
+  // Profile/membership changes must propagate to the identity caches
+  // immediately: classification updates previously waited out the 5-min
+  // snapshot/lookup TTLs, so a just-set email kept an own-team member
+  // refused (the #626 incident's ~39-min window) and a profile change
+  // showed stale names until the TTL lapsed. The event payload carries the
+  // full fresh profile, so the handler re-classifies from it directly.
+  app.event("user_change", async ({ body }: any) => {
+    const user = (body as { event?: { user?: unknown } })?.event?.user as
+      | undefined
+      | (SlackUser & { id?: string });
+    if (user?.id) directory.updateUserFromEvent(user);
+  });
+
+  app.event("team_join", async ({ body }: any) => {
+    const user = (body as { event?: { user?: unknown } })?.event?.user as
+      | undefined
+      | (SlackUser & { id?: string });
+    if (user?.id) directory.updateUserFromEvent(user);
+  });
+
   app.event("member_joined_channel", async ({ event, body, client }: any) => {
     const e = event as { user?: string; channel?: string; event_ts?: string };
     if (

src/slack/identity.ts

@@ -5,6 +5,21 @@ export interface ActorAssertion {
   isExternalGuest?: boolean;
   isBot?: boolean;
   displayName?: string;
+  /**
+   * Own-team member whose principal could not be resolved in email identity
+   * mode (email not currently visible in the directory). Refused like a
+   * guest — fail closed, never a fallback Slack-ID principal — but as its
+   * OWN state, so logs and the user-facing refusal say "unresolved member"
+   * instead of masquerading as "external guest" (#626).
+   */
+  principalUnresolved?: true;
+  /**
+   * The users.info lookup itself failed — the classification below is a
+   * placeholder, not an identity claim. Distinct from every checked state
+   * (guest, internal, principal-unresolved) so callers can retry or fall
+   * back to last-known rather than acting on "external" (#626).
+   */
+  lookupFailed?: true;
 }
 
 export interface SlackUser {
@@ -53,14 +68,22 @@ export function classifyUser(
   );
   const displayName = user.profile?.display_name || user.real_name || user.name || user.id || "";
   let externalId = String(user.id ?? "");
+  // Email mode keys members on their email — including guests, whose
+  // principal still resolves for refusal copy and audit. An email-LESS
+  // member with no Slack-side external evidence is own-team but
+  // principal-unresolved: kept distinct from an external guest (which the
+  // native flags above decide) so refusal copy and logs name what actually
+  // happened instead of masquerading as external (#626).
+  let principalUnresolved = false;
   if (identity === "email" && !user.is_bot) {
     const email = (user.profile?.email ?? "").trim().toLowerCase();
     if (email.includes("@")) externalId = email;
-    else isGuest = true;
+    else if (!isGuest) principalUnresolved = true;
   }
   return {
     externalId,
     isExternalGuest: isGuest,
+    ...(principalUnresolved ? { principalUnresolved: true as const } : {}),
     ...(user.is_bot ? { isBot: true } : {}),
     ...(displayName ? { displayName } : {}),
   };

src/slack/manifest.json

@@ -55,6 +55,8 @@
         "message.mpim",
         "member_joined_channel",
         "member_left_channel",
+        "user_change",
+        "team_join",
         "channel_created",
         "channel_rename",
         "channel_unarchive",

src/slack/turn-handler.ts

@@ -199,6 +199,27 @@ export function createTurnHandler(deps: {
         ...(inc.prefetched.timezone ? { timezone: inc.prefetched.timezone } : {}),
       };
     else classified = await classifyUserCached(client, inc.userId);
+    // A failed users.info must not read as a confident external-guest
+    // refusal: log it so the transient failure is visible, and fall back to
+    // the LAST-KNOWN classification when one exists (the incident in #626
+    // showed a single failed lookup refusing an owner outright). With no
+    // prior state, fail closed but as "unverifiable", the third-state
+    // refusal path — never a laundered "external".
+    if ("ok" in classified && !classified.ok) {
+      const cachedAgain = await directory.lastKnownClassification?.(inc.userId);
+      if (cachedAgain) {
+        console.warn(
+          `[slack-plugin] users.info failed for ${inc.userId}; using last-known classification`,
+        );
+        classified = { actor: cachedAgain };
+      } else {
+        console.warn(
+          `[slack-plugin] users.info failed for ${inc.userId} with no prior classification; ` +
+            `refusing as unverifiable ch=${inc.channel} ts=${inc.ts}`,
+        );
+        classified = { actor: { ...classified.actor, principalUnresolved: true as const } };
+      }
+    }
     const actor = classified.actor;
     const timezone = classified.timezone;
     const text = stripMention(inc.rawText, ids.botUserId);
@@ -296,7 +317,33 @@ export function createTurnHandler(deps: {
             ...(ids.botHandle ? { botHandle: ids.botHandle } : {}),
           };
 
+    // A principal-unresolved own-team member (email mode, email not yet
+    // visible) is refused as its OWN state — fail closed like a guest, but
+    // named for what it is, both to the sender and in the log, so the
+    // incident signature is decidable (#626: the refusal used to be
+    // indistinguishable from external-guest in logs, and its success path
+    // logged nothing at all).
+    const unresolvedMember = audience.find((a) => a.principalUnresolved);
+    if (unresolvedMember) {
+      console.warn(
+        `[slack-plugin] refusing turn: own-team member principal unresolved ` +
+          `(email identity mode; email not visible in the directory) ` +
+          `user=${unresolvedMember.externalId || "?"} ch=${inc.channel} ts=${inc.ts}`,
+      );
+      if (!inc.unprompted) {
+        await ephemeralOrSay(
+          "I can't respond here yet — I couldn't verify your team membership " +
+            "(your email isn't visible to me). This usually resolves within a " +
+            "few minutes of the directory refreshing; try again shortly.",
+        );
+      }
+      return;
+    }
+
     if (audience.some((a) => a.isExternalGuest) && !(await externalParticipantsEnabled())) {
+      console.warn(
+        `[slack-plugin] refusing turn: external guest ch=${inc.channel} ts=${inc.ts}`,
+      );
       if (!inc.unprompted) {
         await ephemeralOrSay(
           "I can't respond here — this conversation isn't fully internal. Try a DM or a fully-internal channel.",

test/slack-identity.test.ts

@@ -63,7 +63,26 @@ test("classifyUser email mode keys members on their normalized work email", () =
 test("classifyUser email mode fails closed to guest when a member has no visible email", () => {
   const a = classifyUser({ id: "U1", team_id: TEAM }, TEAM, "email");
   assert.equal(a.externalId, "U1");
-  assert.equal(a.isExternalGuest, true);
+  // Own-team member, principal unresolved: its OWN state, not external
+  // guest (#626). The refusal is equally closed, but logs and copy say
+  // what actually happened.
+  assert.equal(a.isExternalGuest, false);
+  assert.equal(a.principalUnresolved, true);
+});
+
+test("classifyUser email mode: native external evidence still wins over unresolved", () => {
+  // A restricted member with no visible email is an EXTERNAL GUEST, not an
+  // unresolved own-team member — the third state never masks the flags
+  // (#626).
+  const g = classifyUser({ id: "U2", team_id: TEAM, is_restricted: true }, TEAM, "email");
+  assert.equal(g.isExternalGuest, true);
+  assert.equal(g.principalUnresolved, undefined);
+});
+
+test("classifyUser slack-id mode never marks principalUnresolved", () => {
+  const a = classifyUser({ id: "U1", team_id: TEAM }, TEAM, "slack-id");
+  assert.equal(a.isExternalGuest, false);
+  assert.equal(a.principalUnresolved, undefined);
 });
 
 test("classifyUser email mode keeps bots on their Slack id and non-guest", () => {
@@ -83,6 +102,82 @@ test("classifyUser email mode still flags restricted/other-workspace members as
   assert.equal(g.isExternalGuest, true);
 });
 
+test("a failed users.info asserts lookupFailed, not external guest (#626)", async () => {
+  // The directory's classifyUserCached currently launders a transient
+  // users.info failure into a confident "external guest" — the incident
+  // refused an owner outright on one failed lookup. The failure state must
+  // be distinguishable from every checked state.
+  const { createDirectory } = await import("../src/slack/directory.ts");
+  const failingClient = {
+    users: {
+      info: async () => { throw new Error("transient"); },
+      list: async () => ({ members: [] }),
+    },
+    conversations: { list: async () => ({ channels: [] }) },
+    auth: { test: async () => ({ ok: true, user_id: "UBOT", team_id: "T1" }) },
+  };
+  const dir = createDirectory({
+    core: failingClient as never,
+    ids: { botUserId: "UBOT", ownTeamId: "T1", identityMode: "slack-id" } as never,
+  });
+  const result = await dir.classifyUserCached(failingClient, "U1");
+  assert.equal(result.ok, false);
+  assert.equal(result.actor.lookupFailed, true, "the failure is named, not guest-folded");
+  assert.equal(result.actor.isExternalGuest, undefined);
+});
+
+test("updateUserFromEvent propagates a fresh profile into both caches (#626)", async () => {
+  const { createDirectory } = await import("../src/slack/directory.ts");
+  // Email mode: the member starts email-less (principal-unresolved — the
+  // #626 incident shape) and a user_change carries the just-set email.
+  const client = {
+    users: {
+      info: async () => ({ user: { id: "U1", team_id: "T1" } }),
+      list: async () => ({ members: [] }),
+    },
+    conversations: { list: async () => ({ channels: [] }) },
+    auth: { test: async () => ({ ok: true, user_id: "UBOT", team_id: "T1", bot_id: "BBOT" }) },
+  };
+  const dir = createDirectory({
+    core: client as never,
+    ids: { botUserId: "UBOT", ownTeamId: "T1", bot_id: "BBOT", identityMode: "email" } as never,
+  });
+
+  const before = await dir.classifyUserCached(client, "U1");
+  assert.equal(before.actor.principalUnresolved, true, "fixture starts unresolved");
+
+  dir.updateUserFromEvent({ id: "U1", team_id: "T1", profile: { email: "U1@acme.com" } });
+
+  const after = await dir.classifyUserCached(client, "U1");
+  assert.equal(after.actor.principalUnresolved, undefined, "the fresh profile resolves immediately");
+  assert.equal(after.actor.externalId, "u1@acme.com", "keyed on the fresh email");
+  // A users.info failure now falls back to the UPDATED classification,
+  // not the stale unresolved one.
+  const lastKnown = dir.lastKnownClassification("U1");
+  assert.equal(lastKnown?.externalId, "u1@acme.com");
+});
+
+test("classifyActor preserves ok instead of discarding it (#626)", async () => {
+  // Shape check without a live client: the interface change is the contract
+  // (ok rides on the assertion); the directory unit above covers behavior.
+  const { createDirectory } = await import("../src/slack/directory.ts");
+  const okClient = {
+    users: {
+      info: async () => ({ user: { id: "U1", team_id: "T1" } }),
+      list: async () => ({ members: [] }),
+    },
+    conversations: { list: async () => ({ channels: [] }) },
+    auth: { test: async () => ({ ok: true, user_id: "UBOT", team_id: "T1" }) },
+  };
+  const dir = createDirectory({
+    core: okClient as never,
+    ids: { botUserId: "UBOT", ownTeamId: "T1", identityMode: "slack-id" } as never,
+  });
+  const actor = await dir.classifyActor(okClient, "U1");
+  assert.equal(actor.ok, true, "ok rides on the returned assertion");
+  assert.equal(actor.isExternalGuest, false);
+});
+
 test("slackUserTimezone extracts only valid Slack user timezones", () => {
   assert.equal(slackUserTimezone({ id: "U1", tz: "America/Los_Angeles" }), "America/Los_Angeles");
   assert.equal(slackUserTimezone({ id: "U1", profile: { tz: "Europe/London" } }), "Europe/London");