From 5c65372969dac4a589dd2e78aafeb3edb029f732 Mon Sep 17 00:00:00 2001 From: Darwin Wu Date: Wed, 5 Aug 2026 06:30:19 -0700 Subject: [PATCH] perf(users): cache exact-ID lookups Exact-ID user get calls Slack every time, even when the profile was just fetched. Reuse the existing 24-hour cache, isolate it by workspace credentials, and add refresh and bypass controls. Name lookup is unchanged; no directory scan is added. --- README.md | 4 +- skills/agent-slack/references/output.md | 2 +- src/cli/user-command.ts | 26 +++- src/slack/client.ts | 11 ++ src/slack/user-cache.ts | 133 +++++++++++++++---- test/client.test.ts | 25 ++++ test/search-command.test.ts | 1 + test/user-cache.test.ts | 168 +++++++++++++++++++++++- test/user-command-cache.test.ts | 98 ++++++++++++++ 9 files changed, 433 insertions(+), 35 deletions(-) create mode 100644 test/user-command-cache.test.ts diff --git a/README.md b/README.md index e081803..6bf4e98 100644 --- a/README.md +++ b/README.md @@ -468,8 +468,10 @@ Treat Slack user IDs beginning with `U` or `W` equivalently. # List users (email requires appropriate Slack scopes; fields are pruned if missing) agent-slack user list --workspace "https://workspace.slack.com" --limit 200 | jq . -# Get one user by id or handle +# Get one user by id or handle (exact IDs use the 24-hour profile cache) agent-slack user get U12345678 --workspace "https://workspace.slack.com" | jq . +agent-slack user get U12345678 --refresh --workspace "https://workspace.slack.com" | jq . +agent-slack user get U12345678 --no-cache --workspace "https://workspace.slack.com" | jq . agent-slack user get "@alice" --workspace "https://workspace.slack.com" | jq . # Open a DM or group DM with one to eight other users (the caller is implicit) diff --git a/skills/agent-slack/references/output.md b/skills/agent-slack/references/output.md index 576b35a..3794169 100644 --- a/skills/agent-slack/references/output.md +++ b/skills/agent-slack/references/output.md @@ -11,7 +11,7 @@ Immediate non-attachment sends return `ts` and usually a `permalink`. Attachment `canvas create` returns `canvas: { id, title?, channel_id? }`. `canvas get` returns `canvas: { id, title?, markdown }`. -Message payloads keep canonical user IDs. Pass `--resolve-users` to add display metadata under `referenced_users`, or `--refresh-users` to refresh the 24-hour per-workspace cache before resolving. +Message payloads keep canonical user IDs. Pass `--resolve-users` to add display metadata under `referenced_users`, or `--refresh-users` to refresh the 24-hour credential-scoped cache before resolving. Exact-ID `user get` reuses that cache; pass `--refresh` to replace one entry or `--no-cache` to bypass persistence. Never use cached profile fields to choose a mention or write target. Use `--max-body-chars`, `--max-content-chars`, `--limit`, or a command's counts-only mode to keep results within the task's needs. diff --git a/src/cli/user-command.ts b/src/cli/user-command.ts index a47f71b..d0e7514 100644 --- a/src/cli/user-command.ts +++ b/src/cli/user-command.ts @@ -1,6 +1,8 @@ import type { Command } from "commander"; import type { CliContext } from "./context.ts"; import { pruneEmpty } from "../lib/compact-json.ts"; +import { getCachedUserById } from "../slack/user-cache.ts"; +import { isUserId } from "../slack/user-id.ts"; import { getDmChannelForUsers, getUser, listUsers } from "../slack/users.ts"; export function registerUserCommand(input: { program: Command; ctx: CliContext }): void { @@ -49,15 +51,33 @@ export function registerUserCommand(input: { program: Command; ctx: CliContext } "--workspace ", "Workspace selector (full URL or unique substring; required if you have multiple workspaces)", ) + .option("--refresh", "Refresh an exact-ID profile instead of using the cache") + .option("--no-cache", "Fetch without reading or writing the profile cache") .action(async (...args) => { - const [user, options] = args as [string, { workspace?: string }]; + const [user, options] = args as [ + string, + { workspace?: string; refresh?: boolean; cache?: boolean }, + ]; try { const workspaceUrl = input.ctx.effectiveWorkspaceUrl(options.workspace); const payload = await input.ctx.withAutoRefresh({ workspaceUrl, work: async () => { - const { client } = await input.ctx.getClientForWorkspace(workspaceUrl); - return await getUser(client, user); + const { client, workspace_url } = await input.ctx.getClientForWorkspace(workspaceUrl); + const userId = user.trim(); + if (!isUserId(userId) || options.cache === false) { + return await getUser(client, user); + } + const profile = await getCachedUserById({ + client, + workspaceUrl: workspace_url ?? "", + userId, + forceRefresh: Boolean(options.refresh), + }); + if (!profile) { + throw new Error("users.info returned no user"); + } + return profile; }, }); console.log(JSON.stringify(pruneEmpty(payload), null, 2)); diff --git a/src/slack/client.ts b/src/slack/client.ts index fad8cbd..6c12e85 100644 --- a/src/slack/client.ts +++ b/src/slack/client.ts @@ -1,3 +1,4 @@ +import { createHash } from "node:crypto"; import { WebClient } from "@slack/web-api"; import { getUserAgent } from "../lib/version.ts"; @@ -73,10 +74,16 @@ export class SlackApiClient { private auth: SlackAuth; private web?: WebClient; private workspaceUrl?: string; + private readonly cacheScope: string; constructor(auth: SlackAuth, options?: { workspaceUrl?: string }) { this.auth = auth; this.workspaceUrl = options?.workspaceUrl; + const credentials = + auth.auth_type === "standard" ? [auth.token] : [auth.xoxc_token, auth.xoxd_cookie]; + this.cacheScope = createHash("sha256") + .update([auth.auth_type, ...credentials].join("\0")) + .digest("hex"); if (auth.auth_type === "standard") { this.web = new WebClient(auth.token, { timeout: getSlackApiTimeoutMs(), @@ -86,6 +93,10 @@ export class SlackApiClient { } } + cacheScopeKey(): string { + return this.cacheScope; + } + /** * Call a Slack API method using multipart/form-data encoding. * Some internal Slack APIs (e.g. saved.update) require multipart encoding diff --git a/src/slack/user-cache.ts b/src/slack/user-cache.ts index d0d00a8..3aa8086 100644 --- a/src/slack/user-cache.ts +++ b/src/slack/user-cache.ts @@ -1,14 +1,15 @@ -import { createHash } from "node:crypto"; -import { join } from "node:path"; +import { createHash, randomUUID } from "node:crypto"; +import { mkdir, rename, rm, writeFile } from "node:fs/promises"; +import { dirname, join } from "node:path"; import { getAppDir } from "../lib/app-dir.ts"; -import { readJsonFile, writeJsonFile } from "../lib/fs.ts"; -import { asArray, getString, isRecord } from "../lib/object-type-guards.ts"; +import { readJsonFile } from "../lib/fs.ts"; +import { asArray, isRecord } from "../lib/object-type-guards.ts"; import type { SlackApiClient } from "./client.ts"; import type { SlackMessageSummary } from "./messages.ts"; import { toCompactUser, type CompactSlackUser } from "./users.ts"; import { isUserId } from "./user-id.ts"; -const CACHE_VERSION = 1; +const CACHE_VERSION = 2; const USER_TTL_MS = 24 * 60 * 60 * 1000; const USER_MENTION_PATTERN = /<@([^>|]+)(?:\|[^>]*)?>/g; @@ -19,15 +20,27 @@ type UserCacheEntry = { type UserCacheFile = { version: number; + scope: string; entries: Record; }; -export async function resolveUsersById(input: { +type ResolveUsersInput = { client: SlackApiClient; workspaceUrl: string; userIds: string[]; forceRefresh?: boolean; -}): Promise> { +}; + +export async function resolveUsersById( + input: ResolveUsersInput, +): Promise> { + return resolveUsersByIdInternal(input, false); +} + +async function resolveUsersByIdInternal( + input: ResolveUsersInput, + throwOnError: boolean, +): Promise> { const uniqueIds = dedupeUserIds(input.userIds); if (uniqueIds.length === 0) { return new Map(); @@ -37,13 +50,15 @@ export async function resolveUsersById(input: { const now = Date.now(); const workspaceKey = hashWorkspaceUrl(input.workspaceUrl); const isUnknownWorkspace = workspaceKey === "unknown"; + const cacheScope = input.client.cacheScopeKey(); const cachePath = isUnknownWorkspace ? "" : join(getAppDir(), `users-cache-${workspaceKey}.json`); const diskCache = cachePath - ? await loadCache(cachePath) - : { version: CACHE_VERSION, entries: {} }; + ? await loadCacheBestEffort(cachePath, { now, scope: cacheScope }) + : emptyCache(cacheScope); const out = new Map(); const missing: string[] = []; + let cacheChanged = false; for (const userId of uniqueIds) { const cached = diskCache.entries[userId]; @@ -54,15 +69,20 @@ export async function resolveUsersById(input: { missing.push(userId); } - let cacheChanged = false; - if (missing.length > 0) { const fetched: { userId: string; user: CompactSlackUser | undefined }[] = []; const concurrency = 5; for (let i = 0; i < missing.length; i += concurrency) { const chunk = missing.slice(i, i + concurrency); const results = await Promise.all( - chunk.map(async (userId) => ({ userId, user: await fetchUserById(input.client, userId) })), + chunk.map(async (userId) => ({ + userId, + user: await fetchUserById({ + client: input.client, + userId, + throwOnError, + }), + })), ); fetched.push(...results); } @@ -86,7 +106,6 @@ export async function resolveUsersById(input: { if (Object.keys(diskCache.entries).length !== Object.keys(prunedCache.entries).length) { cacheChanged = true; } - if (cacheChanged) { await writeCache(cachePath, prunedCache); } @@ -95,6 +114,25 @@ export async function resolveUsersById(input: { return out; } +export async function getCachedUserById(input: { + client: SlackApiClient; + userId: string; + workspaceUrl: string; + forceRefresh?: boolean; +}): Promise { + const userId = input.userId.trim(); + const users = await resolveUsersByIdInternal( + { + client: input.client, + workspaceUrl: input.workspaceUrl, + userIds: [userId], + forceRefresh: input.forceRefresh, + }, + true, + ); + return users.get(userId); +} + export function collectReferencedUserIds( messages: SlackMessageSummary[], options?: { includeReactions?: boolean }, @@ -154,10 +192,32 @@ function hashWorkspaceUrl(workspaceUrl: string): string { return createHash("sha256").update(source).digest("hex").slice(0, 16); } -async function loadCache(path: string): Promise { +function emptyCache(scope: string): UserCacheFile { + return { version: CACHE_VERSION, scope, entries: {} }; +} + +async function loadCacheBestEffort( + path: string, + options: { now: number; scope: string }, +): Promise { + try { + return await loadCache(path, options); + } catch { + return emptyCache(options.scope); + } +} + +async function loadCache( + path: string, + options: { now: number; scope: string }, +): Promise { const file = await readJsonFile(path); - if (!file || file.version !== CACHE_VERSION || !isRecord(file.entries)) { - return { version: CACHE_VERSION, entries: {} }; + if (!file) { + return emptyCache(options.scope); + } + if (file.version !== CACHE_VERSION || file.scope !== options.scope || !isRecord(file.entries)) { + await rm(path, { force: true }); + return emptyCache(options.scope); } const entries: Record = {}; @@ -166,8 +226,16 @@ async function loadCache(path: string): Promise { continue; } const fetchedAt = typeof rawEntry.fetched_at === "number" ? rawEntry.fetched_at : undefined; - const user = isRecord(rawEntry.user) ? toCompactUser(rawEntry.user) : null; - if (!fetchedAt || !user) { + const user = isRecord(rawEntry.user) + ? toCompactUser({ ...rawEntry.user, profile: rawEntry.user }) + : null; + if ( + !fetchedAt || + !Number.isFinite(fetchedAt) || + fetchedAt > options.now || + !user || + user.id !== userId + ) { continue; } entries[userId] = { fetched_at: fetchedAt, user }; @@ -175,15 +243,21 @@ async function loadCache(path: string): Promise { return { version: CACHE_VERSION, + scope: options.scope, entries, }; } async function writeCache(path: string, file: UserCacheFile): Promise { + const tempPath = `${path}.${process.pid}.${randomUUID()}.tmp`; try { - await writeJsonFile(path, file); + await mkdir(dirname(path), { recursive: true }); + await writeFile(tempPath, `${JSON.stringify(file, null, 2)}\n`, { mode: 0o600, flag: "wx" }); + await rename(tempPath, path); } catch { // Cache writes are best effort. + } finally { + await rm(tempPath, { force: true }).catch(() => {}); } } @@ -195,21 +269,26 @@ function pruneExpiredEntries(file: UserCacheFile, now: number): UserCacheFile { } next[userId] = entry; } - return { version: CACHE_VERSION, entries: next }; + return { version: CACHE_VERSION, scope: file.scope, entries: next }; } -async function fetchUserById( - client: SlackApiClient, - userId: string, -): Promise { +async function fetchUserById(input: { + client: SlackApiClient; + userId: string; + throwOnError: boolean; +}): Promise { try { - const resp = await client.api("users.info", { user: userId }); + const resp = await input.client.api("users.info", { user: input.userId }); const user = isRecord(resp.user) ? resp.user : null; if (!user) { return undefined; } - return toCompactUser(user); - } catch { + const compact = toCompactUser(user); + return compact.id === input.userId ? compact : undefined; + } catch (error) { + if (input.throwOnError) { + throw error; + } return undefined; } } diff --git a/test/client.test.ts b/test/client.test.ts index c7e395d..1e9cf0a 100644 --- a/test/client.test.ts +++ b/test/client.test.ts @@ -10,6 +10,31 @@ afterEach(() => { delete process.env.AGENT_SLACK_RATE_LIMIT_MAX_WAIT_MS; }); +describe("SlackApiClient cache scope", () => { + test("changes when either browser credential changes", () => { + const first = new SlackApiClient({ + auth_type: "browser", + xoxc_token: "xoxc-first", + xoxd_cookie: "xoxd-first", + }); + const same = new SlackApiClient({ + auth_type: "browser", + xoxc_token: "xoxc-first", + xoxd_cookie: "xoxd-first", + }); + const rotatedCookie = new SlackApiClient({ + auth_type: "browser", + xoxc_token: "xoxc-first", + xoxd_cookie: "xoxd-second", + }); + + expect(first.cacheScopeKey()).toBe(same.cacheScopeKey()); + expect(first.cacheScopeKey()).not.toBe(rotatedCookie.cacheScopeKey()); + expect(first.cacheScopeKey()).not.toContain("xoxc-first"); + expect(first.cacheScopeKey()).not.toContain("xoxd-first"); + }); +}); + describe("SlackApiClient browser multipart transport", () => { test("retries HTTP 429 responses using Retry-After", async () => { // Fail-fast defaults to 0ms; opt in to waiting so the retry path runs. diff --git a/test/search-command.test.ts b/test/search-command.test.ts index f918ce9..13ee4fc 100644 --- a/test/search-command.test.ts +++ b/test/search-command.test.ts @@ -32,6 +32,7 @@ const mockUsersById: Record< function createClient(calls: ApiCall[]) { return { + cacheScopeKey: () => "test-principal", api: async (method: string, params: Record) => { calls.push({ method, params }); diff --git a/test/user-cache.test.ts b/test/user-cache.test.ts index b0f8fbe..564f7c3 100644 --- a/test/user-cache.test.ts +++ b/test/user-cache.test.ts @@ -1,5 +1,14 @@ -import { describe, expect, test } from "bun:test"; -import { collectReferencedUserIds, toReferencedUsers } from "../src/slack/user-cache.ts"; +import { afterEach, beforeEach, describe, expect, test } from "bun:test"; +import { mkdtemp, readFile, readdir, rm, writeFile } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { + collectReferencedUserIds, + getCachedUserById, + resolveUsersById, + toReferencedUsers, +} from "../src/slack/user-cache.ts"; +import type { SlackApiClient } from "../src/slack/client.ts"; import type { SlackMessageSummary } from "../src/slack/messages.ts"; import type { CompactSlackUser } from "../src/slack/users.ts"; @@ -58,7 +67,160 @@ describe("user-cache helpers", () => { expect(toReferencedUsers(["U11111111", "U11111111", "U99999999"], usersById)).toEqual({ U11111111: { id: "U11111111", name: "alice", display_name: "Alice" }, }); - expect(toReferencedUsers(["U99999999"], usersById)).toBeUndefined(); }); }); + +const USER_ID = "U11111111"; +const WORKSPACE = "https://workspace.slack.com"; + +function mockClient( + api: (method: string, params: Record) => Promise>, + scope = "principal-a", +): SlackApiClient { + return { api, cacheScopeKey: () => scope } as unknown as SlackApiClient; +} + +function lookup( + client: SlackApiClient, + forceRefresh = false, +): Promise { + return getCachedUserById({ + client, + workspaceUrl: WORKSPACE, + userId: USER_ID, + forceRefresh, + }); +} + +function userResponse(name: string): Record { + return { + user: { + id: USER_ID, + name, + real_name: "Alice Example", + tz: "America/Los_Angeles", + profile: { + display_name: "Alice", + email: "alice@example.com", + title: "Engineer", + status_text: "Heads down", + status_emoji: ":computer:", + status_expiration: 123, + }, + }, + }; +} + +describe("user profile cache", () => { + const originalXdg = process.env.XDG_RUNTIME_DIR; + let runtimeDir = ""; + + beforeEach(async () => { + runtimeDir = await mkdtemp(join(tmpdir(), "agent-slack-user-cache-test-")); + process.env.XDG_RUNTIME_DIR = runtimeDir; + }); + + afterEach(async () => { + if (originalXdg === undefined) { + delete process.env.XDG_RUNTIME_DIR; + } else { + process.env.XDG_RUNTIME_DIR = originalXdg; + } + await rm(runtimeDir, { recursive: true, force: true }); + }); + + test("reuses the complete profile and refreshes it on request", async () => { + let calls = 0; + const client = mockClient(async () => userResponse(`alice-${++calls}`)); + + const first = await lookup(client); + expect(await lookup(client)).toEqual(first); + expect(first).toMatchObject({ + name: "alice-1", + email: "alice@example.com", + status_text: "Heads down", + }); + expect(calls).toBe(1); + + const refreshed = await lookup(client, true); + expect(refreshed).toMatchObject({ name: "alice-2" }); + expect(await lookup(client)).toEqual(refreshed); + expect(calls).toBe(2); + }); + + test("isolates cache entries by workspace and credentials", async () => { + const calls: string[] = []; + const clientFor = (label: string, scope = "principal-a") => + mockClient(async () => { + calls.push(label); + return userResponse(label); + }, scope); + + await lookup(clientFor("workspace-a")); + await lookup(clientFor("cache-hit")); + await getCachedUserById({ + client: clientFor("workspace-b"), + workspaceUrl: "https://other.slack.com", + userId: USER_ID, + }); + await lookup(clientFor("principal-b", "principal-b")); + await lookup(clientFor("principal-a-again")); + + expect(calls).toEqual(["workspace-a", "workspace-b", "principal-b", "principal-a-again"]); + }); + + test("rejects mismatched cached and live user IDs", async () => { + await lookup(mockClient(async () => userResponse("old"))); + const cacheDir = join(runtimeDir, "agent-slack"); + const [cacheName] = (await readdir(cacheDir)).filter((name) => name.startsWith("users-cache-")); + const cachePath = join(cacheDir, cacheName!); + const cache = JSON.parse(await readFile(cachePath, "utf8")) as { + entries: Record; + }; + cache.entries[USER_ID]!.user.id = "U22222222"; + await writeFile(cachePath, JSON.stringify(cache)); + + expect(await lookup(mockClient(async () => userResponse("fresh")))).toMatchObject({ + name: "fresh", + }); + expect( + await lookup( + mockClient(async () => ({ user: { id: "U22222222", name: "wrong" } })), + true, + ), + ).toBeUndefined(); + + let calls = 0; + const preserved = await lookup( + mockClient(async () => { + calls += 1; + return userResponse("unexpected"); + }), + ); + expect(preserved).toMatchObject({ name: "fresh" }); + expect(calls).toBe(0); + }); + + test("treats cache I/O as best effort", async () => { + const blockedPath = join(runtimeDir, "not-a-directory"); + await writeFile(blockedPath, "blocked"); + process.env.XDG_RUNTIME_DIR = blockedPath; + + expect(await lookup(mockClient(async () => userResponse("alice")))).toMatchObject({ + name: "alice", + }); + }); + + test("preserves singular and batch API error behavior", async () => { + const slackError = new Error("invalid_auth"); + const client = mockClient(async () => { + throw slackError; + }); + + await expect(lookup(client)).rejects.toBe(slackError); + expect(await resolveUsersById({ client, workspaceUrl: WORKSPACE, userIds: [USER_ID] })).toEqual( + new Map(), + ); + }); +}); diff --git a/test/user-command-cache.test.ts b/test/user-command-cache.test.ts new file mode 100644 index 0000000..056d79e --- /dev/null +++ b/test/user-command-cache.test.ts @@ -0,0 +1,98 @@ +import { afterEach, beforeEach, describe, expect, test } from "bun:test"; +import { mkdtemp, rm } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { Command } from "commander"; +import { registerUserCommand } from "../src/cli/user-command.ts"; +import type { CliContext } from "../src/cli/context.ts"; + +describe("user get profile cache", () => { + const originalLog = console.log; + const originalXdg = process.env.XDG_RUNTIME_DIR; + let runtimeDir = ""; + + beforeEach(async () => { + runtimeDir = await mkdtemp(join(tmpdir(), "agent-slack-user-command-cache-test-")); + process.env.XDG_RUNTIME_DIR = runtimeDir; + console.log = () => {}; + }); + + afterEach(async () => { + console.log = originalLog; + if (originalXdg === undefined) { + delete process.env.XDG_RUNTIME_DIR; + } else { + process.env.XDG_RUNTIME_DIR = originalXdg; + } + await rm(runtimeDir, { recursive: true, force: true }); + }); + + test("reuses exact-ID profiles and refreshes on request", async () => { + let userInfoCalls = 0; + const output: string[] = []; + const client = { + cacheScopeKey: () => "test-principal", + api: async (method: string) => { + expect(method).toBe("users.info"); + userInfoCalls += 1; + return { + user: { + id: "U11111111", + name: `alice-${userInfoCalls}`, + real_name: "Alice Example", + tz: "America/Los_Angeles", + profile: { + display_name: "Alice", + email: "alice@example.com", + title: "Engineer", + status_text: "Heads down", + status_emoji: ":computer:", + status_expiration: 123, + }, + }, + }; + }, + }; + const ctx = { + effectiveWorkspaceUrl: () => "https://workspace.slack.com", + withAutoRefresh: async (input: { work: () => Promise }) => input.work(), + getClientForWorkspace: async () => ({ + client, + workspace_url: "https://workspace.slack.com", + }), + errorMessage: (error: unknown) => String(error), + } as unknown as CliContext; + const program = new Command(); + registerUserCommand({ program, ctx }); + console.log = (value?: unknown) => output.push(String(value)); + + await program.parseAsync(["user", "get", "U11111111"], { from: "user" }); + await program.parseAsync(["user", "get", "U11111111"], { from: "user" }); + expect(userInfoCalls).toBe(1); + expect(output[1]).toBe(output[0]); + expect(JSON.parse(output[0]!)).toEqual({ + id: "U11111111", + name: "alice-1", + real_name: "Alice Example", + display_name: "Alice", + email: "alice@example.com", + title: "Engineer", + tz: "America/Los_Angeles", + status_text: "Heads down", + status_emoji: ":computer:", + status_expiration: 123, + }); + + await program.parseAsync(["user", "get", "U11111111", "--refresh"], { from: "user" }); + expect(userInfoCalls).toBe(2); + + await program.parseAsync(["user", "get", "U11111111", "--no-cache"], { from: "user" }); + expect(userInfoCalls).toBe(3); + + await program.parseAsync(["user", "get", "U11111111"], { from: "user" }); + expect(userInfoCalls).toBe(3); + expect(JSON.parse(output[2]!).name).toBe("alice-2"); + expect(JSON.parse(output[3]!).name).toBe("alice-3"); + expect(output[4]).toBe(output[2]); + }); +});