Fixes: @students lookup, channel display, and group-editing authorization

src/slack/commands.ts

@@ -24,6 +24,7 @@ import { screeningStatus } from "../domain/rules/screening.js";
 import { log } from "../logger.js";
 import {
   isSettingKey,
+  parseHandles,
   SETTING_KEYS,
   SETTINGS,
   setting,
@@ -583,7 +584,7 @@ async function configText(
 ): Promise<string> {
   const [verb, key, ...valueWords] = rest;
 
-  if (!verb) return configListing();
+  if (!verb) return configListing(client);
 
   if (verb !== "set") {
     return (
@@ -615,11 +616,14 @@ async function configText(
     actorName: caller.name,
   });
 
+  const now = await describeValue(client, key, cleaned.value);
+  const was = before.value
+    ? await describeValue(client, key, before.value)
+    : null;
+
   const lines = [
-    `*${SETTINGS[key].label}* is now \`${cleaned.value}\`` +
-      (before.value
-        ? ` (was \`${before.value}\`, from ${before.source})`
-        : "") +
+    `*${SETTINGS[key].label}* is now ${now}` +
+      (was ? ` (was ${was}, from ${before.source})` : "") +
       ".",
   ];
 
@@ -636,17 +640,49 @@ async function configText(
   return lines.join("\n");
 }
 
-function configListing(): string {
-  const rows = SETTING_KEYS.map((key) => {
-    const { value, source } = setting(key);
-    const where =
-      source === "slack"
-        ? "set here"
-        : source === "env"
-          ? `from ${SETTINGS[key].env}`
-          : "*not set*";
-    return `• \`${key}\` — ${value ?? "—"}  _(${where})_`;
-  });
+/**
+ * Renders a stored value the way a person wrote it.
+ *
+ * Channels are stored by id, deliberately — an id survives the channel being
+ * renamed, and a stored `#name` would quietly stop resolving the day somebody
+ * tidied it up. But `C0BPAV78LKZ` tells a reader nothing, so the id is what is
+ * kept and the name is what is shown. Falls back to the raw value if Slack
+ * cannot be asked: a settings listing that throws is worse than one that is
+ * briefly ugly.
+ */
+async function describeValue(
+  client: WebClient,
+  key: SettingKey,
+  value: string
+): Promise<string> {
+  if (SETTINGS[key].kind === "channel") {
+    try {
+      const info = await client.conversations.info({ channel: value });
+      return info.channel?.name ? `#${info.channel.name}` : `\`${value}\``;
+    } catch {
+      return `\`${value}\``;
+    }
+  }
+  const handles = parseHandles(value);
+  return handles.length
+    ? handles.map((h) => `@${h}`).join(", ")
+    : `\`${value}\``;
+}
+
+async function configListing(client: WebClient): Promise<string> {
+  const rows = await Promise.all(
+    SETTING_KEYS.map(async (key) => {
+      const { value, source } = setting(key);
+      const where =
+        source === "slack"
+          ? "set here"
+          : source === "env"
+            ? `from ${SETTINGS[key].env}`
+            : "*not set*";
+      const shown = value ? await describeValue(client, key, value) : "—";
+      return `• \`${key}\` — ${shown}  _(${where})_`;
+    })
+  );
 
   const unset = SETTING_KEYS.filter((k) => setting(k).source === "unset");
 

src/slack/userGroups.ts

@@ -8,16 +8,37 @@ export type ResolvedGroup = {
   members: Set<string>;
 };
 
-/** Slack ids for user groups are `S` followed by uppercase alphanumerics. */
-const GROUP_ID = /^S[A-Z0-9]{4,}$/;
-
-export function isGroupId(ref: string): boolean {
-  return GROUP_ID.test(ref.toUpperCase());
+/**
+ * Whether a group is the one being asked for, by handle or by id.
+ *
+ * Matches on *either*, deliberately, rather than deciding up front which kind
+ * of reference it was handed. A previous version guessed with a pattern for
+ * Slack ids — `S` followed by alphanumerics — and `students` matches it:
+ * uppercased it is `STUDENTS`, which is an `S` and seven more characters. So
+ * the most important handle in this entire project was read as an opaque id,
+ * matched against nothing, and reported as a group that does not exist. The
+ * role sync said the same thing, which meant no student was being rostered at
+ * all.
+ *
+ * Handles and ids cannot realistically collide — a handle would have to be
+ * spelled exactly like some other group's id — so there is nothing to gain by
+ * telling them apart, and this cannot be wrong in the way guessing was.
+ */
+export function matchesGroup(
+  group: { id?: string; handle?: string },
+  ref: string
+): boolean {
+  const raw = ref.trim().replace(/^@/, "");
+  if (!raw) return false;
+  return (
+    (group.handle ?? "").toLowerCase() === raw.toLowerCase() ||
+    (group.id ?? "") === raw.toUpperCase()
+  );
 }
 
 /**
  * Resolves a user group by its @handle or its id. Handles are what people
- * actually type and see, so they are what the config names; ids are opaque —
+ * actually type and see, so they are what the settings name; ids are opaque —
  * but an id is what Slack sends when a slash command has link escaping on and
  * somebody types `@students`, which arrives as `<!subteam^S123|students>`.
  *
@@ -28,15 +49,16 @@ export async function resolveGroup(
   client: WebClient,
   ref: string
 ): Promise<ResolvedGroup | null> {
-  const raw = ref.replace(/^@/, "");
-  const wanted = raw.toLowerCase();
-  const byId = isGroupId(raw) ? raw.toUpperCase() : null;
+  const wanted = ref.trim().replace(/^@/, "");
   const list = await client.usergroups.list({ include_disabled: false });
-  const group = (list.usergroups ?? []).find((g) =>
-    byId ? g.id === byId : (g.handle ?? "").toLowerCase() === wanted
-  );
+  const group = (list.usergroups ?? []).find((g) => matchesGroup(g, wanted));
   if (!group?.id) {
-    log.warn("user group not found", { ref });
+    // Naming what does exist turns "it says my group is missing" into a
+    // one-glance answer, which is how this bug should have been found.
+    log.warn("user group not found", {
+      ref,
+      available: (list.usergroups ?? []).map((g) => g.handle).filter(Boolean),
+    });
     return null;
   }
 
@@ -106,3 +128,12 @@ export async function setGroupMembership(
     users: userIds.join(","),
   });
 }
+
+/** Every user group handle in the workspace, for "did you mean" messages. */
+export async function listGroupHandles(client: WebClient): Promise<string[]> {
+  const list = await client.usergroups.list({ include_disabled: false });
+  return (list.usergroups ?? [])
+    .map((g) => g.handle)
+    .filter((h): h is string => Boolean(h))
+    .sort();
+}

test/userGroups.test.ts

@@ -0,0 +1,66 @@
+import assert from "node:assert/strict";
+import { describe, it } from "node:test";
+import { matchesGroup } from "../src/slack/userGroups.js";
+
+const students = { id: "S0614TY5A", handle: "students" };
+const mentors = { id: "S07QQ2M1B", handle: "mentors" };
+
+describe("finding a user group by handle or id", () => {
+  it("matches a plain handle", () => {
+    assert.equal(matchesGroup(students, "students"), true);
+  });
+
+  it("matches a handle written with @", () => {
+    assert.equal(matchesGroup(students, "@students"), true);
+  });
+
+  it("matches regardless of case", () => {
+    assert.equal(matchesGroup(students, "@Students"), true);
+  });
+
+  it("matches the id Slack sends in an escaped mention", () => {
+    assert.equal(matchesGroup(students, "S0614TY5A"), true);
+  });
+
+  it("does not match a different group", () => {
+    assert.equal(matchesGroup(mentors, "students"), false);
+    assert.equal(matchesGroup(students, "S07QQ2M1B"), false);
+  });
+
+  it("ignores surrounding whitespace", () => {
+    assert.equal(matchesGroup(students, "  @students  "), true);
+  });
+
+  it("does not match nothing", () => {
+    assert.equal(matchesGroup(students, ""), false);
+    assert.equal(matchesGroup(students, "  "), false);
+    assert.equal(matchesGroup(students, "@"), false);
+  });
+
+  it("copes with a group Slack returned without a handle", () => {
+    assert.equal(matchesGroup({ id: "S1234ABCD" }, "students"), false);
+    assert.equal(matchesGroup({ handle: "students" }, "S1234ABCD"), false);
+  });
+
+  /**
+   * The regression, and it was as bad as it looks. Resolution used to decide up
+   * front whether a reference was a handle or an id, using a pattern for Slack
+   * ids: `S` followed by alphanumerics. `students` uppercased is `STUDENTS` —
+   * an `S` and seven more characters — so the single most important handle in
+   * this project was read as an opaque id, matched against no group, and
+   * reported as missing.
+   *
+   * That broke `/hawkmod config`, `/hawkmod group`, and the role sync, which
+   * meant no student was rostered and so none was monitored. Matching on either
+   * handle or id removes the guess that made it possible.
+   */
+  it("matches handles that look like a Slack id", () => {
+    for (const handle of ["students", "staff", "seniors", "scouting"]) {
+      assert.equal(
+        matchesGroup({ id: "S0614TY5A", handle }, handle),
+        true,
+        `@${handle} must resolve by handle`
+      );
+    }
+  });
+});