From b8bd6e5a89cf7d019827dc72aabcc08f90ad9094 Mon Sep 17 00:00:00 2001 From: Ty Tremblay Date: Thu, 20 Aug 2026 23:03:11 -0400 Subject: [PATCH 1/2] Fix: @students never resolved, so no student was rostered MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Resolution decided up front whether a reference was a handle or a Slack id, using a pattern for 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 a group that does not exist. That broke `/hawkmod group`, and worse, it broke `syncRolesFromUserGroups`: with STUDENT_USERGROUP=students the sync could not resolve the group, so nobody was rostered as a student and nobody was monitored as one. It reported this honestly as a `workspace_config` violation saying the group did not exist, which was true of the lookup and false of the workspace. It went unnoticed because the test workspace used `hawkmod-test-students`, which does not begin with `s` followed by alphanumerics alone. `mentors` was never affected either. Only the handles that happen to look like ids — students, staff, seniors — and one of those is the one that matters. Matching now tries handle *or* id and never guesses which it was given. The two cannot realistically collide: a handle would have to be spelled exactly like some other group's id. There is nothing to gain by telling them apart, and this cannot fail the way guessing did. A failed lookup now logs the handles that do exist, which is how this should have been found in the first place. Co-Authored-By: Claude Opus 5 --- src/slack/userGroups.ts | 57 +++++++++++++++++++++++++++-------- test/userGroups.test.ts | 66 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 110 insertions(+), 13 deletions(-) create mode 100644 test/userGroups.test.ts diff --git a/src/slack/userGroups.ts b/src/slack/userGroups.ts index 0a94367..f1a6eb4 100644 --- a/src/slack/userGroups.ts +++ b/src/slack/userGroups.ts @@ -8,16 +8,37 @@ export type ResolvedGroup = { members: Set; }; -/** 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 ``. * @@ -28,15 +49,16 @@ export async function resolveGroup( client: WebClient, ref: string ): Promise { - 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 { + const list = await client.usergroups.list({ include_disabled: false }); + return (list.usergroups ?? []) + .map((g) => g.handle) + .filter((h): h is string => Boolean(h)) + .sort(); +} diff --git a/test/userGroups.test.ts b/test/userGroups.test.ts new file mode 100644 index 0000000..9532087 --- /dev/null +++ b/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` + ); + } + }); +}); From 3a1c028ab4bdd0a913ab974f84c8584b6feb3b96 Mon Sep 17 00:00:00 2001 From: Ty Tremblay Date: Thu, 20 Aug 2026 23:06:34 -0400 Subject: [PATCH 2/2] Show channel names and group handles, not raw ids MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `/hawkmod config` reported the alert channel as C0BPAV78LKZ, which tells a reader nothing about which channel it is — the whole point of the listing being to answer "what is it actually using". Channels stay stored by id. That part was right: an id survives the channel being renamed, where a stored `#name` would quietly stop resolving the day somebody tidied it up. So the id is kept and the name is shown, resolved at display time, falling back to the raw value if Slack cannot be asked — a settings listing that throws is worse than one that is briefly ugly. User groups render as @handle for the same reason, and the confirmation after a change now describes both the new value and the old one. Co-Authored-By: Claude Opus 5 --- src/slack/commands.ts | 68 +++++++++++++++++++++++++++++++++---------- 1 file changed, 52 insertions(+), 16 deletions(-) diff --git a/src/slack/commands.ts b/src/slack/commands.ts index 26a6974..b7657d3 100644 --- a/src/slack/commands.ts +++ b/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 { 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 { + 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 { + 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");