fix(slack): paginate the member channel lookup; structure member-rule denials

src/__tests__/slack-user/listChannelRows.test.ts

@@ -195,18 +195,26 @@ describe('renderListSummary', () => {
 // === diagnoseChannelAccess helpers ===
 
 import { lookupChannelByName, explainDenial, describeChannel } from '../../slack-user/server.js';
-import { assertAccess, SlackAccessDenied } from '../../slack-user/accessControl.js';
+import { assertAccess, assertDmMemberAccess, SlackAccessDenied } from '../../slack-user/accessControl.js';
 import type { ChannelMeta } from '../../slack-user/accessControl.js';
 
+/** `mine` may be a flat list (one page) or a list of member pages. */
 function lookupClient(mine: any[], workspacePages: any[][] = []) {
-  let page = 0;
+  const memberPages: any[][] = Array.isArray(mine[0]) ? mine as any[][] : [mine];
+  let mp = 0;
+  let wp = 0;
   return {
-    conversationsList: async () => ({ channels: mine }),
+    conversationsList: async () => {
+      const channels = memberPages[mp] ?? [];
+      const more = mp < memberPages.length - 1;
+      mp++;
+      return { channels, response_metadata: more ? { next_cursor: `m${mp}` } : {} };
+    },
     conversationsListAll: async () => {
-      const channels = workspacePages[page] ?? [];
-      const more = page < workspacePages.length - 1;
-      page++;
-      return { channels, response_metadata: more ? { next_cursor: `c${page}` } : {} };
+      const channels = workspacePages[wp] ?? [];
+      const more = wp < workspacePages.length - 1;
+      wp++;
+      return { channels, response_metadata: more ? { next_cursor: `c${wp}` } : {} };
     },
   } as any;
 }
@@ -239,6 +247,7 @@ describe('lookupChannelByName', () => {
     const res = await lookupChannelByName(client, 'missing');
     assert.equal(res.kind, 'scanBounded');
     assert.equal((res as any).pages, 10);
+    assert.equal((res as any).source, 'workspace');
   });
 
   it('reports a genuine absence when the scan reached the end', async () => {
@@ -396,3 +405,132 @@ describe('renderChannelLines', () => {
     assert.equal(line, '#bare (C2)\n  Type: public');
   });
 });
+
+describe('lookupChannelByName pagination', () => {
+  it('follows the member-list cursor to find a DM on a later page', async () => {
+    // users.conversations is the ONLY source that returns im/mpim — the
+    // workspace fallback cannot see them — so not following its cursor made a
+    // DM past page one unfindable, not merely slower.
+    const client = lookupClient([
+      [{ id: 'D1', name: 'someone-else', is_im: true }],
+      [{ id: 'D2', name: 'alice', is_im: true }],
+    ]);
+    assert.deepEqual(await lookupChannelByName(client, 'alice'), { kind: 'found', channelId: 'D2' });
+  });
+
+  it('finds a group DM on a later member page', async () => {
+    const client = lookupClient([
+      [{ id: 'G1', name: 'mpdm-x', is_mpim: true }],
+      [{ id: 'G2', name: 'mpdm-target', is_mpim: true }],
+    ]);
+    assert.deepEqual(await lookupChannelByName(client, 'mpdm-target'), { kind: 'found', channelId: 'G2' });
+  });
+
+  it('reports a bounded member scan instead of falling through to a wrong answer', async () => {
+    const pages = Array.from({ length: 12 }, (_, i) => [{ id: `D${i}`, name: `dm-${i}`, is_im: true }]);
+    const res = await lookupChannelByName(lookupClient(pages), 'missing');
+    assert.equal(res.kind, 'scanBounded');
+    assert.equal((res as any).pages, 10);
+    assert.equal((res as any).source, 'member');
+  });
+
+  it('stops paginating members as soon as it matches', async () => {
+    let calls = 0;
+    const client: any = {
+      conversationsList: async () => {
+        calls++;
+        return { channels: [{ id: 'C1', name: 'found-it' }], response_metadata: { next_cursor: 'more' } };
+      },
+      conversationsListAll: async () => ({ channels: [], response_metadata: {} }),
+    };
+    await lookupChannelByName(client, 'found-it');
+    assert.equal(calls, 1);
+  });
+
+  it('still falls back to the workspace list for a channel you are not in', async () => {
+    const client = lookupClient([[]], [[{ id: 'C9', name: 'eng-general' }]]);
+    assert.deepEqual(await lookupChannelByName(client, 'eng-general'), { kind: 'found', channelId: 'C9' });
+  });
+});
+
+describe('explainDenial for member rules', () => {
+  const rulesWithOrg: SlackAccessRules = {
+    allowedOrgs: ['T_MINE'], blacklistUsers: ['U_BAD'],
+    whitelistChannels: ['*'], blacklistChannels: [], allowPublicOnly: false,
+  };
+
+  async function dmDenial(meta: any, client: any): Promise<SlackAccessDenied> {
+    try { await assertDmMemberAccess(client, rulesWithOrg, meta, meta.id ?? 'D1'); }
+    catch (e) { return e as SlackAccessDenied; }
+    throw new Error('expected a denial');
+  }
+
+  it('names the DM counterpart\'s organisation and the fix', async () => {
+    const client: any = { usersInfo: async () => ({ user: { id: 'U1', team_id: 'T_OTHER' } }) };
+    const d = await dmDenial({ is_im: true, user: 'U1' }, client);
+    assert.equal(d.reason, 'dm-org-not-allowed');
+    assert.deepEqual(d.orgIds, ['T_OTHER']);
+    const out = explainDenial(d, rulesWithOrg, '', new Map([['T_OTHER', 'Acme Corp']])).join('\n');
+    assert.match(out, /The other participant belongs to an organisation that is not allowed: Acme Corp \(T_OTHER\)/);
+    assert.match(out, /Access Rules → Organizations/);
+  });
+
+  it('names a group DM member\'s organisation', async () => {
+    const client: any = {
+      conversationsMembers: async () => ({ members: ['U1'] }),
+      usersInfo: async () => ({ user: { id: 'U1', team_id: 'T_OTHER' } }),
+    };
+    const d = await dmDenial({ is_mpim: true, id: 'G1' }, client);
+    assert.equal(d.reason, 'group-dm-org');
+    const out = explainDenial(d, rulesWithOrg, '').join('\n');
+    assert.match(out, /A member of this group DM belongs to an organisation that is not allowed/);
+  });
+
+  it('points a blocked group-DM member at the blocked-users list', async () => {
+    const client: any = {
+      conversationsMembers: async () => ({ members: ['U_BAD'] }),
+      usersInfo: async () => ({ user: { id: 'U_BAD', team_id: 'T_MINE' } }),
+    };
+    const d = await dmDenial({ is_mpim: true, id: 'G1' }, client);
+    assert.equal(d.reason, 'group-dm-blacklist');
+    const out = explainDenial(d, rulesWithOrg, '').join('\n');
+    assert.match(out, /blocked-users list/);
+    assert.match(out, /Access Rules → Blocked users/);
+  });
+
+  it('degrades to the raw ID when the org cannot be named', async () => {
+    const client: any = { usersInfo: async () => ({ user: { id: 'U1', team_id: 'T_OTHER' } }) };
+    const d = await dmDenial({ is_im: true, user: 'U1' }, client);
+    assert.match(explainDenial(d, rulesWithOrg, '').join('\n'), /not allowed: T_OTHER/);
+  });
+});
+
+describe('lookupChannelByName workspace fallback after a bounded member scan', () => {
+  it('still finds a workspace channel when the member scan ran out of pages', async () => {
+    // The regression: returning early on the member bound meant an account with
+    // enough member conversations could never reach the workspace list, so a
+    // public channel it is not a member of became undiscoverable.
+    const memberPages = Array.from({ length: 11 }, (_, i) => [{ id: `D${i}`, name: `dm-${i}`, is_im: true }]);
+    const client = lookupClient(memberPages, [[{ id: 'C_TARGET', name: 'eng-general' }]]);
+    assert.deepEqual(await lookupChannelByName(client, 'eng-general'), {
+      kind: 'found', channelId: 'C_TARGET',
+    });
+  });
+
+  it('reports both limits when neither scan finished', async () => {
+    const memberPages = Array.from({ length: 12 }, (_, i) => [{ id: `D${i}`, name: `dm-${i}`, is_im: true }]);
+    const workspacePages = Array.from({ length: 12 }, (_, i) => [{ id: `C${i}`, name: `ch-${i}` }]);
+    const res = await lookupChannelByName(lookupClient(memberPages, workspacePages), 'missing');
+    assert.equal(res.kind, 'scanBounded');
+    assert.equal((res as any).source, 'both');
+  });
+
+  it('reports only the member limit when the workspace list was exhausted', async () => {
+    const memberPages = Array.from({ length: 12 }, (_, i) => [{ id: `D${i}`, name: `dm-${i}`, is_im: true }]);
+    const res = await lookupChannelByName(
+      lookupClient(memberPages, [[{ id: 'C1', name: 'something-else' }]]), 'missing',
+    );
+    assert.equal(res.kind, 'scanBounded');
+    assert.equal((res as any).source, 'member');
+  });
+});

src/slack-user/accessControl.ts

@@ -22,6 +22,9 @@ import type { SlackAccessRules } from '../mcpConnectionStore.js';
 export type SlackDenialReason =
   | 'org-not-allowed'
   | 'org-unverified'
+  | 'dm-org-not-allowed'
+  | 'group-dm-blacklist'
+  | 'group-dm-org'
   | 'whitelist-empty'
   | 'whitelist-miss'
   | 'blacklist-channel'
@@ -242,21 +245,30 @@ export async function assertDmMemberAccess(
   if (meta.is_im && meta.user && rules.allowedOrgs.length > 0) {
     const { user } = await client.usersInfo(meta.user);
     if (user.team_id && !rules.allowedOrgs.includes(user.team_id)) {
-      throw new UserError('Access denied: this user belongs to an organisation not in your allowed list.');
+      throw new SlackAccessDenied(
+        'Access denied: this user belongs to an organisation not in your allowed list.',
+        { reason: 'dm-org-not-allowed', orgIds: [user.team_id] },
+      );
     }
   }
 
   // Group DM: no member may be blacklisted or from a non-allowed org.
   if (meta.is_mpim && (rules.blacklistUsers.length > 0 || rules.allowedOrgs.length > 0)) {
     const { members } = await client.conversationsMembers(channelId);
     if (rules.blacklistUsers.length > 0 && members.some(uid => rules.blacklistUsers.includes(uid))) {
-      throw new UserError('Access denied: this group DM contains a blacklisted user.');
+      throw new SlackAccessDenied(
+        'Access denied: this group DM contains a blacklisted user.',
+        { reason: 'group-dm-blacklist' },
+      );
     }
     if (rules.allowedOrgs.length > 0) {
       for (const uid of members) {
         const { user } = await client.usersInfo(uid);
         if (user.team_id && !rules.allowedOrgs.includes(user.team_id)) {
-          throw new UserError('Access denied: this group DM contains a user from a non-allowed organisation.');
+          throw new SlackAccessDenied(
+            'Access denied: this group DM contains a user from a non-allowed organisation.',
+            { reason: 'group-dm-org', orgIds: [user.team_id] },
+          );
         }
       }
     }

src/slack-user/server.ts

@@ -188,6 +188,9 @@ const DENIAL_LABELS: Record<string, string> = {
   'blacklist-channel': 'blacklist',
   'blacklist-user': 'blocked user',
   'dm-rules': 'DM/group-DM rules (blocked user or organisation)',
+  'dm-org-not-allowed': 'organisation of the other participant',
+  'group-dm-blacklist': 'blocked user in the group DM',
+  'group-dm-org': 'organisation of a group DM member',
   'public-only': 'private (allowPublicOnly)',
   'org-not-allowed': 'organisation',
   'org-unverified': 'organisation unverified',
@@ -951,7 +954,8 @@ const DIAGNOSE_MAX_PAGES = 10;
 export type ChannelLookup =
   | { kind: 'found'; channelId: string }
   | { kind: 'none' }
-  | { kind: 'scanBounded'; pages: number }
+  /** Which scan ran out of pages — the caller has to name the right one. */
+  | { kind: 'scanBounded'; pages: number; source: 'member' | 'workspace' | 'both' }
   | { kind: 'ambiguous'; ids: string[] };
 
 /**
@@ -972,23 +976,50 @@ export async function lookupChannelByName(
   const matches = (channels: ListedChannel[]) =>
     channels.filter(ch => ch.name?.toLowerCase() === wanted);
 
-  const mine = await client.conversationsList(undefined, 'public_channel,private_channel,mpim,im');
-  let hits = matches(mine.channels as ListedChannel[]);
-
+  // Pass 1: users.conversations, paginated. It is member-scoped, so it is both
+  // cheap and the *only* source that returns DMs and group DMs — the workspace
+  // fallback below cannot see them at all. Following its cursor therefore is
+  // not an optimisation: without it, a DM on page two is simply unfindable.
+  // No initialiser: the do-while below always assigns before anything reads it.
+  let hits: ListedChannel[];
   let cursor: string | undefined;
-  let pages = 0;
-  while (hits.length === 0 && pages < DIAGNOSE_MAX_PAGES) {
+  let memberPages = 0;
+  do {
+    const page = await client.conversationsList(cursor, 'public_channel,private_channel,mpim,im');
+    hits = matches(page.channels as ListedChannel[]);
+    cursor = page.response_metadata?.next_cursor || undefined;
+    memberPages++;
+  } while (hits.length === 0 && cursor && memberPages < DIAGNOSE_MAX_PAGES);
+
+  // Hitting the member bound must NOT skip the workspace scan: a public channel
+  // the user is not in is only ever found there, and an account with enough
+  // member conversations would otherwise never reach it. Remember the
+  // uncertainty and carry on.
+  const memberScanBounded = hits.length === 0 && !!cursor;
+
+  // Pass 2: the workspace-wide list, for a channel the user is not in.
+  let workspacePages = 0;
+  cursor = undefined;
+  while (hits.length === 0 && workspacePages < DIAGNOSE_MAX_PAGES) {
     const page = await client.conversationsListAll(cursor, 'public_channel,private_channel');
     hits = matches(page.channels as ListedChannel[]);
     cursor = page.response_metadata?.next_cursor || undefined;
-    pages++;
+    workspacePages++;
     if (!cursor) break;
   }
 
   if (hits.length === 1) return { kind: 'found', channelId: hits[0].id };
   if (hits.length > 1) return { kind: 'ambiguous', ids: hits.map(h => h.id) };
-  // A bounded scan is a floor, not a verdict.
-  return cursor ? { kind: 'scanBounded', pages } : { kind: 'none' };
+
+  // A bounded scan is a floor, not a verdict — and the caller has to be told
+  // which list ran out, since the two mean different things to the user.
+  const workspaceScanBounded = !!cursor;
+  if (memberScanBounded && workspaceScanBounded) {
+    return { kind: 'scanBounded', pages: Math.max(memberPages, workspacePages), source: 'both' };
+  }
+  if (workspaceScanBounded) return { kind: 'scanBounded', pages: workspacePages, source: 'workspace' };
+  if (memberScanBounded) return { kind: 'scanBounded', pages: memberPages, source: 'member' };
+  return { kind: 'none' };
 }
 
 /**
@@ -1015,6 +1046,25 @@ export function explainDenial(
       );
       break;
     }
+    case 'dm-org-not-allowed':
+    case 'group-dm-org': {
+      const labels = (denial.orgIds ?? []).map(id => formatTeamLabel(id, orgNames)).join(', ');
+      const who = denial.reason === 'dm-org-not-allowed'
+        ? 'The other participant belongs to'
+        : 'A member of this group DM belongs to';
+      out.push(
+        `${who} an organisation that is not allowed: ${labels || '(unresolved)'}`,
+        `Your allowed organisations: ${rules.allowedOrgs.length ? rules.allowedOrgs.join(', ') : '(none)'}`,
+        'Fix: tick the organisation under Access Rules → Organizations in the dashboard, or remove the person from the conversation.',
+      );
+      break;
+    }
+    case 'group-dm-blacklist':
+      out.push(
+        'One of this group DM\'s members is on your blocked-users list.',
+        'Fix: remove them under Access Rules → Blocked users, or leave the conversation.',
+      );
+      break;
     case 'org-unverified':
       out.push(
         'Slack returned no organisation for this shared channel, so it cannot be checked against your allowlist.',
@@ -1092,11 +1142,17 @@ slackUserServer.addTool({
         case 'ambiguous':
           return [`${found.ids.length} channels are named "${args.name}". Re-run with one of these IDs:`,
             ...found.ids.map(id => `  ${id}`)].join('\n');
-        case 'scanBounded':
+        case 'scanBounded': {
+          const which = {
+            member: 'your own conversations',
+            workspace: 'the workspace channel list',
+            both: 'both your own conversations and the workspace channel list',
+          }[found.source];
           return [
-            `No channel named "${args.name}" found in the first ${found.pages} page(s) of the workspace channel list.`,
+            `No channel named "${args.name}" found in the first ${found.pages} page(s) of ${which}.`,
             'The scan stopped at its page limit, so the channel may still exist further in. Pass channelId to check it directly.',
           ].join('\n');
+        }
         default:
           return `No channel named "${args.name}" is visible to your Slack account. Slack itself does not return it, so this is not an access-rules problem — you are most likely not a member of it.`;
       }
@@ -1111,16 +1167,19 @@ slackUserServer.addTool({
       await assertDmMemberAccess(client, rules, meta, channelId!);
     } catch (err) {
       if (err instanceof SlackAccessDenied) denial = err;
-      else if (err instanceof UserError) return [header, '', `Denied by: ${err.message}`].join('\n');
+      // Every access rule now throws SlackAccessDenied, so this branch is only
+      // for a UserError that is not a denial at all — a missing rules record,
+      // say. Report it rather than dressing it up as a rule verdict.
+      else if (err instanceof UserError) return [header, '', `Could not evaluate: ${err.message}`].join('\n');
       else throw err;
     }
 
     if (!denial) {
       return [header, '', 'Readable: yes. Every access rule passes for this channel.'].join('\n');
     }
 
-    const orgNames = denial.reason === 'org-not-allowed'
-      ? (await resolveTeamNames(client, denial.orgIds ?? [], { tokenKey })).names
+    const orgNames = (denial.orgIds?.length ?? 0) > 0
+      ? (await resolveTeamNames(client, denial.orgIds!, { tokenKey })).names
       : undefined;
     return [header, '', ...explainDenial(denial, rules, meta.name, orgNames)].join('\n');
   },