From 9df8c56c037688fd2cdefcd473ef1fbfff259bd4 Mon Sep 17 00:00:00 2001 From: Josh France <12610835+16francej@users.noreply.github.com> Date: Fri, 14 Aug 2026 17:53:41 -0700 Subject: [PATCH 01/10] Tighten shared runtime boundaries --- src/api/app-helpers.ts | 6 +++ src/api/app-turn.ts | 6 +-- src/credentials/keychain.ts | 7 ++-- src/credentials/paths.ts | 2 +- src/deploy/docker-deploy-provider.ts | 24 ++++++++--- src/identity/identity-service.ts | 2 +- src/slack/directory.ts | 2 +- src/slack/events.ts | 2 + src/slack/identity.ts | 11 ++--- src/slack/turn-handler.ts | 1 + test/admin-agent-capability.test.ts | 1 + test/capability-routes.test.ts | 5 ++- test/dm-relay.test.ts | 1 + test/docker-deploy-provider.test.ts | 40 ++++++++++++++++++ test/environment-routes.test.ts | 1 + test/external-slack-participants.test.ts | 13 ++++++ test/identity.test.ts | 6 +++ test/keychain.test.ts | 28 +++++++++++++ test/oauth-consent-bridge.test.ts | 4 ++ test/projects.test.ts | 32 ++++++++++++++ test/secret-drop.test.ts | 2 + test/slack-identity.test.ts | 14 +++++++ test/slack-index.integration.test.ts | 53 +++++++++++++++++++++--- test/surface-context.test.ts | 1 + 24 files changed, 238 insertions(+), 26 deletions(-) create mode 100644 test/docker-deploy-provider.test.ts diff --git a/src/api/app-helpers.ts b/src/api/app-helpers.ts index 6c2b93a7..9d24450d 100644 --- a/src/api/app-helpers.ts +++ b/src/api/app-helpers.ts @@ -404,6 +404,12 @@ export function createAppHelpers(deps: AppDeps, app: App) { claims: Pick, ): Promise { const { kind, ref } = parseScopeId(claims.scopeId); + if ( + (kind === "channel" || kind === "group") && + !(await principalCanAccessCurrentScope(claims.actorId, claims.scopeId)) + ) { + return false; + } if (kind !== "group" || deps.projects?.recognizes(ref) !== true) return true; return ( (await principalCanManageScope(claims.actorId, claims.scopeId)) && diff --git a/src/api/app-turn.ts b/src/api/app-turn.ts index 2ae9bff0..3ff04d7a 100644 --- a/src/api/app-turn.ts +++ b/src/api/app-turn.ts @@ -57,6 +57,9 @@ export function createTurnMethods( async turn(req: TurnRequest): Promise { await deps.identity.refresh(); const actor: Principal = deps.identity.resolve(req.actor); + if (!deps.identity.isInternal(actor)) { + return { status: "refused", reason: "internal-only: non-internal principals cannot interact" }; + } let projectAudience: Principal[] | undefined; let projectName: string | undefined; let projectVersion: string | undefined; @@ -66,9 +69,6 @@ export function createTurnMethods( const projectId = projectGroup ? projectIdFromGroupRef(conversationRef) : null; if (projectGroup) { - if (!deps.identity.isInternal(actor)) { - return { status: "refused", reason: "you're not a member of that context" }; - } if (!projectId) return { status: "refused", reason: "you're not a member of that context" }; const project = await deps.projects?.get(projectId); if ( diff --git a/src/credentials/keychain.ts b/src/credentials/keychain.ts index 22b88f16..e152bc6d 100644 --- a/src/credentials/keychain.ts +++ b/src/credentials/keychain.ts @@ -1279,8 +1279,9 @@ const FILE_ENV_POINTERS: Array<[RegExp, (abs: string) => string]> = [ export function renderUseScript(m: MaterializedCred): string { if (m.kind === "env") return m.env.map((e) => `export ${e.key}=${shq(e.value)}`).join("\n") + "\n"; + const files = m.files.map((f) => ({ ...f, path: homeRelativePath(f.path) })); const lines = [`__kc_dir="$(mktemp -d "\${TMPDIR:-/tmp}/keychain.XXXXXX")"`, `umask 077`]; - for (const f of m.files) { + for (const f of files) { const parent = f.path.includes("/") ? f.path.replace(/\/[^/]*$/, "") : ""; if (parent) lines.push(`mkdir -p "$__kc_dir/${parent}"`); lines.push( @@ -1289,7 +1290,7 @@ export function renderUseScript(m: MaterializedCred): string { ); } const pointed = new Set(); - for (const f of m.files) { + for (const f of files) { for (const [re, render] of FILE_ENV_POINTERS) { if (re.test(f.path) && !pointed.has(re)) { pointed.add(re); @@ -1297,7 +1298,7 @@ export function renderUseScript(m: MaterializedCred): string { } } } - if (m.files.some((f) => !FILE_ENV_POINTERS.some(([re]) => re.test(f.path)))) { + if (files.some((f) => !FILE_ENV_POINTERS.some(([re]) => re.test(f.path)))) { lines.push( `for __e in "$HOME"/.[!.]* "$HOME"/*; do [ -e "$__e" ] || continue; __b=\${__e##*/}; [ -e "$__kc_dir/$__b" ] || ln -s "$__e" "$__kc_dir/$__b"; done`, `export HOME="$__kc_dir"`, diff --git a/src/credentials/paths.ts b/src/credentials/paths.ts index 08ad20c3..3eaaa5dd 100644 --- a/src/credentials/paths.ts +++ b/src/credentials/paths.ts @@ -4,7 +4,7 @@ function stripHomePrefix(target: string): string { export function homeRelativePath(path: string): string { const rel = stripHomePrefix(path).replace(/^\.\//, ""); - if (!rel || rel.startsWith("/") || rel.split("/").includes("..")) { + if (!rel || rel.startsWith("/") || rel.split("/").includes("..") || !/^[A-Za-z0-9._@+ /-]+$/.test(rel)) { throw new Error(`file path must be home-relative: ${path}`); } return rel; diff --git a/src/deploy/docker-deploy-provider.ts b/src/deploy/docker-deploy-provider.ts index c8fe4052..c6a2058b 100644 --- a/src/deploy/docker-deploy-provider.ts +++ b/src/deploy/docker-deploy-provider.ts @@ -1,14 +1,14 @@ import type { Deployment, DeploymentVersion } from "./deploy-store.ts"; import type { DeployEndpoint, DeployProvider } from "./deploy-provider.ts"; -import { spawnDockerExec } from "../sandbox/docker-exec.ts"; +import { spawnDockerExec, type DockerExec } from "../sandbox/docker-exec.ts"; -const NETWORK = "agent-deploynet"; const APP_PORT = 8080; export interface DockerDeployProviderOptions { image?: string; docker?: string; basePort?: number; + dockerExec?: DockerExec; } export function createDockerDeployProvider(opts: DockerDeployProviderOptions = {}): DeployProvider { @@ -32,15 +32,26 @@ export function createDockerDeployProvider(opts: DockerDeployProviderOptions = { } }; - const dexec = spawnDockerExec(docker); + const dexec = opts.dockerExec ?? spawnDockerExec(docker); const name = (d: Deployment) => `agent-deploy-${d.id.slice(0, 12)}`; + const network = (d: Deployment) => `${name(d)}-net`; + const ensureNetwork = async (d: Deployment): Promise => { + const net = network(d); + if ((await dexec(["network", "inspect", net])).code !== 0) { + const r = await dexec(["network", "create", net]); + if (r.code !== 0 && !/already exists/i.test(r.stderr)) { + throw new Error(`docker network create ${net} failed: ${r.stderr.trim()}`); + } + } + return net; + }; return { profile: { managedScaleToZero: false }, async apply(d: Deployment, version: DeploymentVersion): Promise { - await dexec(["network", "create", NETWORK]); + const net = await ensureNetwork(d); await dexec(["rm", "-f", name(d)]); const hostPort = allocPort(name(d)); const envArgs = Object.entries(version.env ?? {}).flatMap(([k, v]) => ["-e", `${k}=${v}`]); @@ -50,7 +61,7 @@ export function createDockerDeployProvider(opts: DockerDeployProviderOptions = { "--name", name(d), "--network", - NETWORK, + net, "--memory", "512m", "--cpus", @@ -72,6 +83,8 @@ export function createDockerDeployProvider(opts: DockerDeployProviderOptions = { version.entrypoint, ]); if (r.code !== 0) { + await dexec(["rm", "-f", name(d)]); + await dexec(["network", "rm", net]); freePort(name(d)); throw new Error(`deploy run failed: ${r.stderr.trim()}`); } @@ -87,6 +100,7 @@ export function createDockerDeployProvider(opts: DockerDeployProviderOptions = { async destroy(d: Deployment): Promise { await dexec(["rm", "-f", name(d)]); + await dexec(["network", "rm", network(d)]); freePort(name(d)); }, }; diff --git a/src/identity/identity-service.ts b/src/identity/identity-service.ts index bcf3f479..77c214bd 100644 --- a/src/identity/identity-service.ts +++ b/src/identity/identity-service.ts @@ -104,7 +104,7 @@ export function createIdentityService(backing?: DurableMap): return refreshP; }, resolve(actor: ActorAssertion): Principal { - const p = classify(actor.externalId, actor.isExternalGuest); + const p = classify(actor.externalId, actor.isExternalGuest || actor.isBot); return { ...p, ...(actor.teamIds ? { teamIds: actor.teamIds } : {}), diff --git a/src/slack/directory.ts b/src/slack/directory.ts index a58ada0b..474e1cdc 100644 --- a/src/slack/directory.ts +++ b/src/slack/directory.ts @@ -324,7 +324,7 @@ export function createDirectory(deps: { async function pushDirectory(snap: UserSnapshot, client: any): Promise { const members = [...snap.byId.entries()] - .filter(([, u]) => !u.actor.isExternalGuest) + .filter(([, u]) => !u.actor.isExternalGuest && !u.actor.isBot) .map(([slackId, u]) => { const a = u.actor; return { diff --git a/src/slack/events.ts b/src/slack/events.ts index cca6f541..d1e03078 100644 --- a/src/slack/events.ts +++ b/src/slack/events.ts @@ -59,6 +59,7 @@ export function registerSlackEvents( files: (e.files as SlackFile[]) ?? [], threadTs: e.thread_ts, ts: e.ts, + ...(e.bot_id || e.subtype === "bot_message" ? { botAuthored: true } : {}), ackGate: context.ackGate as AckGate | undefined, }, client, @@ -124,6 +125,7 @@ export function registerSlackEvents( files: (m.files as SlackFile[]) ?? [], threadTs: m.thread_ts, ts: m.ts, + ...(m.bot_id || m.subtype === "bot_message" ? { botAuthored: true } : {}), ackGate, }, client, diff --git a/src/slack/identity.ts b/src/slack/identity.ts index 25c66a55..df702c43 100644 --- a/src/slack/identity.ts +++ b/src/slack/identity.ts @@ -128,7 +128,7 @@ export function computeChannelAudience( ): ActorAssertion[] { if (members && members.length) { const byId = new Map(); - for (const m of [actor, ...members]) if (m.externalId) byId.set(m.externalId, m); + for (const m of [actor, ...members]) if (m.externalId && !m.isBot) byId.set(m.externalId, m); const audience = [...byId.values()]; if (isExternallyShared(info) && audience.every((m) => !m.isExternalGuest)) { audience.push(externalMarker()); @@ -147,7 +147,7 @@ export function computePublishMembers( ): ActorAssertion[] | undefined { if (!complete) return undefined; if (isExternallyShared(info)) return undefined; - const all = [actor, ...members]; + const all = [actor, ...members].filter((m) => !m.isBot); if (all.some((m) => m.isExternalGuest)) return undefined; const byId = new Map(); for (const m of all) if (m.externalId) byId.set(m.externalId, m); @@ -161,9 +161,10 @@ export function allInternalChannelMembers( ): string[] | undefined { if (!complete) return undefined; if (isExternallyShared(info)) return undefined; - if (members.some((m) => m.isExternalGuest)) return undefined; + const humans = members.filter((m) => !m.isBot); + if (humans.some((m) => m.isExternalGuest)) return undefined; const ids = new Set(); - for (const m of members) if (m.externalId) ids.add(m.externalId); + for (const m of humans) if (m.externalId) ids.add(m.externalId); return [...ids]; } @@ -189,7 +190,7 @@ export async function resolveChannelMembership(opts: { for (const id of memberIds) { const { actor: member, ok } = await opts.classify(id); members.push(member); - if (member.externalId && !member.isExternalGuest) slackIdsByPrincipal.set(member.externalId, id); + if (member.externalId && !member.isExternalGuest && !member.isBot) slackIdsByPrincipal.set(member.externalId, id); if (!ok) complete = false; } const audience = computeChannelAudience(actor, members, info); diff --git a/src/slack/turn-handler.ts b/src/slack/turn-handler.ts index 167407c8..d56587f6 100644 --- a/src/slack/turn-handler.ts +++ b/src/slack/turn-handler.ts @@ -196,6 +196,7 @@ export function createTurnHandler(deps: { const timezone = classified.timezone; const text = stripMention(inc.rawText, ids.botUserId); if (!hasContent(text, inc.files)) return; + if (actor.isBot || inc.botAuthored) return; let audience: ActorAssertion[] = [actor]; let channelRef: string | undefined; diff --git a/test/admin-agent-capability.test.ts b/test/admin-agent-capability.test.ts index cac0ef92..d6a4ae8e 100644 --- a/test/admin-agent-capability.test.ts +++ b/test/admin-agent-capability.test.ts @@ -30,6 +30,7 @@ function start() { signingSecret: SECRET, }), ); + void built.directory.replaceChannels([{ channelId: "C1", name: "agent-admin", isPrivate: false }]); const keychain = createKeychain({ creds: createMemoryMap(), grants: createMemoryMap(), diff --git a/test/capability-routes.test.ts b/test/capability-routes.test.ts index f81a1df9..cd9bbd32 100644 --- a/test/capability-routes.test.ts +++ b/test/capability-routes.test.ts @@ -64,6 +64,7 @@ describe("capability-token control plane (crons + SOUL)", () => { signingSecret: SECRET, }), ); + await built.directory.replaceChannels([{ channelId: "C", name: "eng", isPrivate: false }]); server = createServer(built.app, { signingSecret: SECRET, scheduler: built.scheduler, @@ -625,7 +626,7 @@ describe("capability-token control plane (crons + SOUL)", () => { assert.equal((await get(`/v1/crons/${id}`, { "x-agent-capability": await capFor("U1") })).status, 404); }); - it("get/patch/delete/run of an OWNER cron are denied to another user (403), even in the same channel", async () => { + it("another public-channel user can read an OWNER cron but cannot patch, run, or delete it", async () => { const created = (await ( await post( "/v1/crons", @@ -634,7 +635,7 @@ describe("capability-token control plane (crons + SOUL)", () => { ) ).json()) as any; const id = created.cron.id; - assert.equal((await get(`/v1/crons/${id}`, { "x-agent-capability": await capChannel("U2") })).status, 403); + assert.equal((await get(`/v1/crons/${id}`, { "x-agent-capability": await capChannel("U2") })).status, 200); assert.equal( (await patch(`/v1/crons/${id}`, { action: "hijacked" }, { "x-agent-capability": await capChannel("U2") })).status, 403, diff --git a/test/dm-relay.test.ts b/test/dm-relay.test.ts index eb8cc472..c86942c0 100644 --- a/test/dm-relay.test.ts +++ b/test/dm-relay.test.ts @@ -60,6 +60,7 @@ describe("agent → teammate DM: the cron recipient route (§10)", () => { ]); await built.app.upsertChannels( [ + { channelId: "C", name: "current" }, { channelId: "C-eng", name: "eng" }, { channelId: "C-d1", name: "design-frontend" }, { channelId: "C-d2", name: "design-backend" }, diff --git a/test/docker-deploy-provider.test.ts b/test/docker-deploy-provider.test.ts new file mode 100644 index 00000000..459d38f2 --- /dev/null +++ b/test/docker-deploy-provider.test.ts @@ -0,0 +1,40 @@ +import assert from "node:assert/strict"; +import { test } from "node:test"; +import { createDockerDeployProvider } from "../src/deploy/docker-deploy-provider.ts"; +import { createDeployStore } from "../src/deploy/deploy-store.ts"; +import type { DockerExec } from "../src/sandbox/docker-exec.ts"; +import { scopeId } from "../src/types.ts"; + +test("Docker deployments use isolated networks and remove them on destroy", async () => { + const calls: string[][] = []; + const dockerExec: DockerExec = async (args) => { + calls.push(args); + return { code: args[1] === "inspect" ? 1 : 0, stdout: "", stderr: "" }; + }; + const store = createDeployStore(); + const first = await store.create({ + ownerScopeId: scopeId("personal", "U1"), + createdBy: "U1", + entrypoint: "node server.js", + snapshotDir: "/snap/one", + }); + const second = await store.create({ + ownerScopeId: scopeId("personal", "U2"), + createdBy: "U2", + entrypoint: "node server.js", + snapshotDir: "/snap/two", + }); + const provider = createDockerDeployProvider({ dockerExec }); + + await provider.apply(first, first.versions[0]!); + await provider.apply(second, second.versions[0]!); + await provider.destroy(first); + + const firstName = `agent-deploy-${first.id.slice(0, 12)}`; + const secondName = `agent-deploy-${second.id.slice(0, 12)}`; + assert.ok(calls.some((args) => args.join(" ") === `network create ${firstName}-net`)); + assert.ok(calls.some((args) => args.join(" ") === `network create ${secondName}-net`)); + assert.ok(calls.some((args) => args.join(" ").includes(`--name ${firstName} --network ${firstName}-net`))); + assert.ok(calls.some((args) => args.join(" ").includes(`--name ${secondName} --network ${secondName}-net`))); + assert.ok(calls.some((args) => args.join(" ") === `network rm ${firstName}-net`)); +}); diff --git a/test/environment-routes.test.ts b/test/environment-routes.test.ts index 0b434a3f..b904b79f 100644 --- a/test/environment-routes.test.ts +++ b/test/environment-routes.test.ts @@ -36,6 +36,7 @@ describe("environment verbs (list / create / attach, owner-gated)", async () => signingSecret: SECRET, }), ); + await built.directory.replaceChannels([{ channelId: "C-eng", name: "eng", isPrivate: false }]); server = createServer(built.app, { signingSecret: SECRET, scheduler: built.scheduler }); await new Promise((resolve) => server.listen(0, resolve)); base = `http://localhost:${(server.address() as AddressInfo).port}`; diff --git a/test/external-slack-participants.test.ts b/test/external-slack-participants.test.ts index c58db8b5..65c1cb0e 100644 --- a/test/external-slack-participants.test.ts +++ b/test/external-slack-participants.test.ts @@ -89,6 +89,19 @@ test("the toggle never lets an external actor interact", async () => { assert.match(res.reason ?? "", /internal-only/); }); +test("a bot assertion is refused before entering the turn pipeline", async () => { + const built = freshApp(); + const res = await built.app.turn({ + surface: "slack", + actor: { externalId: "B1", isBot: true }, + conversation: { kind: "dm", threadRef: "dm:B1:t1" }, + text: "hello", + }); + assert.equal(res.status, "refused"); + assert.match(res.reason ?? "", /internal-only/); + assert.equal((await built.runs.list()).length, 0); +}); + test("admin resource: org-only PUT, read-back, and the surface-config echo", async () => { const built = freshApp(); const server = createInsecureTestServer(built.app, { diff --git a/test/identity.test.ts b/test/identity.test.ts index e2e195c0..8d6b5b9a 100644 --- a/test/identity.test.ts +++ b/test/identity.test.ts @@ -17,6 +17,12 @@ test("classifies a flagged Slack Connect user as guest", () => { assert.equal(id.isInternal(p), false); }); +test("resolves bot assertions as non-internal", () => { + const p = id.resolve({ externalId: "B1", isBot: true }); + assert.equal(p.type, "guest"); + assert.equal(id.isInternal(p), false); +}); + test("audienceIsAllInternal is false if any member is non-internal (G1)", () => { const internal = id.classify("U1"); const guest = id.classify("U2", true); diff --git a/test/keychain.test.ts b/test/keychain.test.ts index 0df8596e..c6b5b716 100644 --- a/test/keychain.test.ts +++ b/test/keychain.test.ts @@ -502,6 +502,22 @@ test("file bundles: one item per service, materialize to a /tmp script with env (e: KeychainError) => e.status === 400, "paths must be home-relative", ); + await assert.rejects( + k.save({ ownerId: "U1", service: "unsafe", files: [{ path: "$(uname)", contentBase64: b64("x") }] }), + (e: KeychainError) => e.status === 400, + "paths must contain portable filename characters", + ); + assert.throws( + () => + renderUseScript({ + kind: "file", + credentialId: "legacy-unsafe", + ownerId: "U1", + service: "unsafe", + files: [{ path: "$(uname)", contentBase64: b64("x") }], + }), + /file path must be home-relative/, + ); const grant = await k.createGrant({ credentialId: cred.id, @@ -952,6 +968,18 @@ describe("/v1/keychain routes (capability-authed)", () => { before(async () => { built = buildApp(testConfig({ dataDir: mkdtempSync(join(tmpdir(), "kc-routes-")), signingSecret: SECRET })); + await built.directory.replaceChannels([ + { channelId: "C_SECONDS", name: "seconds", isPrivate: false }, + { channelId: "C_BAD_EXP", name: "bad-exp", isPrivate: false }, + { channelId: "C7", name: "grants", isPrivate: false }, + { channelId: "C_OVERVIEW", name: "overview", isPrivate: false }, + { channelId: "C_NAMED", name: "pilot-portal", isPrivate: false }, + { channelId: "C8", name: "file-grants", isPrivate: false }, + ]); + await built.directory.replaceGroups([ + { groupId: "G_CONN", principalId: "alex@conn" }, + { groupId: "G_CONN", principalId: "carol@conn" }, + ]); server = createServer(built.app, { signingSecret: SECRET, keychain: built.keychain, diff --git a/test/oauth-consent-bridge.test.ts b/test/oauth-consent-bridge.test.ts index 68f68869..b22c75c5 100644 --- a/test/oauth-consent-bridge.test.ts +++ b/test/oauth-consent-bridge.test.ts @@ -26,6 +26,10 @@ function start( opts: { portalUrl?: string } = { portalUrl: "http://callback.test" }, ): { base: string; built: BuiltApp; close: () => Promise } { const built = buildApp(testConfig({ dataDir: mkdtempSync(join(tmpdir(), "consent-")), signingSecret: SECRET })); + void built.directory.replaceChannels([ + { channelId: "C1", name: "consent", isPrivate: false }, + { channelId: "C9", name: "connectors", isPrivate: false }, + ]); const server = createServer(built.app, { signingSecret: SECRET, replayDedupe: built.replayDedupe, diff --git a/test/projects.test.ts b/test/projects.test.ts index b9216013..659ffd82 100644 --- a/test/projects.test.ts +++ b/test/projects.test.ts @@ -192,6 +192,38 @@ test("managed groups override Slack membership and historical sessions grant no ); }); +test("capability scope checks follow current shared rosters", async () => { + const built = buildApp(testConfig({ dataDir: mkdtempSync(join(tmpdir(), "capability-roster-")) })); + await built.app.upsertDirectory([{ principalId: "member", displayName: "Member", type: "internal" }]); + await built.directory.replaceChannels( + [ + { channelId: "C-public", name: "public" }, + { channelId: "C-private", name: "private", isPrivate: true }, + ], + [{ channelId: "C-private", principalId: "member" }], + 1, + ); + await built.directory.replaceGroups([{ groupId: "G1", principalId: "member" }], 1); + + assert.equal(await built.app.authorizesCapabilityScope({ actorId: "member", scopeId: "channel:C-private" }), true); + assert.equal(await built.app.authorizesCapabilityScope({ actorId: "member", scopeId: "group:G1" }), true); + assert.equal(await built.app.authorizesCapabilityScope({ actorId: "member", scopeId: "channel:C-public" }), true); + + await built.directory.replaceChannels( + [ + { channelId: "C-public", name: "public" }, + { channelId: "C-private", name: "private", isPrivate: true }, + ], + [], + 2, + ); + await built.directory.replaceGroups([], 2); + + assert.equal(await built.app.authorizesCapabilityScope({ actorId: "member", scopeId: "channel:C-private" }), false); + assert.equal(await built.app.authorizesCapabilityScope({ actorId: "member", scopeId: "group:G1" }), false); + assert.equal(await built.app.authorizesCapabilityScope({ actorId: "member", scopeId: "channel:C-public" }), true); +}); + async function listen(server: Server): Promise { await new Promise((resolve) => server.listen(0, resolve)); return `http://127.0.0.1:${(server.address() as AddressInfo).port}`; diff --git a/test/secret-drop.test.ts b/test/secret-drop.test.ts index 2c013751..a30726c2 100644 --- a/test/secret-drop.test.ts +++ b/test/secret-drop.test.ts @@ -227,6 +227,7 @@ describe("/v1/keychain/drops — mint, form, redeem", async () => { before(async () => { built = buildApp(testConfig({ dataDir: mkdtempSync(join(tmpdir(), "secret-drop-")), signingSecret: SECRET })); + await built.directory.replaceChannels([{ channelId: "C1", name: "drops", isPrivate: false }]); server = createServer(built.app, { signingSecret: SECRET, keychain: built.keychain, @@ -517,6 +518,7 @@ describe("/v1/keychain/drops — sibling-aware resume", () => { const built = buildApp( testConfig({ dataDir: mkdtempSync(join(tmpdir(), "secret-drop-sib-")), signingSecret: SECRET }), ); + await built.directory.replaceChannels([{ channelId: "C1", name: "drops", isPrivate: false }]); const fires: DropResolution[] = []; let fired: (() => void) | undefined; const server = createServer(built.app, { diff --git a/test/slack-identity.test.ts b/test/slack-identity.test.ts index bdc59279..922388ed 100644 --- a/test/slack-identity.test.ts +++ b/test/slack-identity.test.ts @@ -240,6 +240,20 @@ test("allInternalChannelMembers: all-internal + complete → deduped ids; WITHHE ); }); +test("bot accounts are absent from shared-scope rosters", () => { + assert.deepEqual( + allInternalChannelMembers( + [ + { externalId: "U1", isExternalGuest: false }, + { externalId: "B1", isExternalGuest: false, isBot: true }, + ], + true, + { is_private: true }, + ), + ["U1"], + ); +}); + function membershipDeps(overrides: Partial[0]> = {}) { const internal = (externalId: string): ActorAssertion => ({ externalId, isExternalGuest: false }); const byId: Record = { diff --git a/test/slack-index.integration.test.ts b/test/slack-index.integration.test.ts index 08ca29a4..2f1223b6 100644 --- a/test/slack-index.integration.test.ts +++ b/test/slack-index.integration.test.ts @@ -614,6 +614,52 @@ test("an external principal is refused in a DM before core sees the text", async } }); +test("a bot-authored mention never becomes a turn", async () => { + const f = await fixture(); + try { + f.client.usersById.set("B1", { + id: "B1", + team_id: "T1", + is_bot: true, + name: "peerbot", + profile: { display_name: "Peer Bot" }, + }); + f.client.membersByChannel.set("C1", ["U1", "U2", "B1", "UBOT"]); + await f.app.emitEvent("app_mention", { + channel: "C1", + channel_type: "channel", + user: "B1", + bot_id: "B-PEER", + text: "<@UBOT> hello", + ts: "102.2", + }); + assert.equal(f.core.turns.length, 0); + assert.equal(f.client.posts.length, 0); + } finally { + await f.stop(); + } +}); + +test("a bot-authored stop cannot abort a live run", async () => { + const f = await fixture(); + try { + f.core.activeRun = "run-active"; + await f.app.emitMessage({ + channel: "D1", + channel_type: "im", + subtype: "bot_message", + user: "B1", + bot_id: "B-PEER", + text: "stop", + ts: "102.3", + }); + assert.deepEqual(f.core.abortedRuns, []); + assert.equal(f.core.turns.length, 0); + } finally { + await f.stop(); + } +}); + test("a Slack Connect mention is refused ephemerally and never mirrored", async () => { const f = await fixture(); try { @@ -832,7 +878,7 @@ test("a group DM whose listing fails is retried at most once, never once per mes } }); -test("a peer bot's thread reply dispatches without attesting liveness", async () => { +test("a peer bot's thread reply is mirrored without dispatching a turn", async () => { const f = await fixture(); try { f.client.usersById.set("UB2", { id: "UB2", team_id: "T1", name: "copilot", is_bot: true }); @@ -850,10 +896,7 @@ test("a peer bot's thread reply dispatches without attesting liveness", async () ts: "301.3", thread_ts: "301.1", }); - assert.equal(f.core.turns.length, 1); - assert.equal(f.core.turns[0].unprompted, true); - assert.equal(f.core.turns[0].entryTs, "301.3"); - assert.equal(f.core.turns[0].liveActor, undefined, "a bot author is automation, never a live act"); + assert.equal(f.core.turns.length, 0); } finally { await f.stop(); } diff --git a/test/surface-context.test.ts b/test/surface-context.test.ts index f14fdab8..692233f6 100644 --- a/test/surface-context.test.ts +++ b/test/surface-context.test.ts @@ -81,6 +81,7 @@ describe("surface-context pulls", async () => { ]); await built.app.upsertChannels( [ + { channelId: "C9", name: "current" }, { channelId: "C-ENG", name: "eng" }, { channelId: "CPUBLIC01", name: "general" }, { channelId: "C-SECRET", name: "warroom", isPrivate: true }, From b8b7e39df7c8ed5a3320eca2c34f7e80543c8e17 Mon Sep 17 00:00:00 2001 From: Josh France <12610835+16francej@users.noreply.github.com> Date: Fri, 14 Aug 2026 19:26:51 -0700 Subject: [PATCH 02/10] Enforce current capability rosters --- src/api/app-helpers.ts | 5 +-- src/slack/directory.ts | 54 +++++++++++++++++----------- src/slack/events.ts | 11 +++--- src/slack/index.ts | 2 +- test/admin-agent-capability.test.ts | 5 ++- test/capability-routes.test.ts | 5 ++- test/dm-relay.test.ts | 5 ++- test/environment-routes.test.ts | 5 ++- test/keychain-ask.test.ts | 1 + test/keychain.test.ts | 30 +++++++++++----- test/oauth-consent-bridge.test.ts | 14 +++++--- test/projects.test.ts | 7 ++-- test/secret-drop.test.ts | 10 ++++-- test/skills-http.test.ts | 5 ++- test/slack-index.integration.test.ts | 29 +++++++++++++++ test/surface-context.test.ts | 7 +++- 16 files changed, 140 insertions(+), 55 deletions(-) diff --git a/src/api/app-helpers.ts b/src/api/app-helpers.ts index 9d24450d..c9be4ef7 100644 --- a/src/api/app-helpers.ts +++ b/src/api/app-helpers.ts @@ -404,10 +404,7 @@ export function createAppHelpers(deps: AppDeps, app: App) { claims: Pick, ): Promise { const { kind, ref } = parseScopeId(claims.scopeId); - if ( - (kind === "channel" || kind === "group") && - !(await principalCanAccessCurrentScope(claims.actorId, claims.scopeId)) - ) { + if ((kind === "channel" || kind === "group") && !(await principalCanWriteScope(claims.actorId, claims.scopeId))) { return false; } if (kind !== "group" || deps.projects?.recognizes(ref) !== true) return true; diff --git a/src/slack/directory.ts b/src/slack/directory.ts index 474e1cdc..c02b2a5e 100644 --- a/src/slack/directory.ts +++ b/src/slack/directory.ts @@ -46,13 +46,13 @@ interface GroupMembershipRow { groupId: string; principalId: string; } -interface PrivateChannelRef { +interface ChannelRef { id: string; name: string; info: ChannelMeta; } -type RosterKind = { plural: string; authz: string; item: string }; +type RosterKind = { plural: string; authz: string; item: string; limit?: number }; const MEMBERS_PAGE_LIMIT = 200; const MAX_CLASSIFY_MEMBERS = 200; @@ -79,7 +79,6 @@ export interface Directory { refs: ReadonlyArray<{ id: string; info?: ChannelMeta }>, kind: RosterKind, ): Promise>; - knownPublicChannels: { has(channel: string): boolean; add(channel: string): void; delete(channel: string): void }; syncForUnseenGroup(client: any, groupId: string): void; resolveAutoIdentityMode(client: any): Promise; maxClassifyMembers: number; @@ -144,9 +143,9 @@ export function createDirectory(deps: { async function listBotChannels( client: any, - ): Promise<{ publicChannels: ChannelRow[]; privateChannels: PrivateChannelRef[] }> { - const publicChannels: ChannelRow[] = []; - const privateChannels: PrivateChannelRef[] = []; + ): Promise<{ publicChannels: ChannelRef[]; privateChannels: ChannelRef[] }> { + const publicChannels: ChannelRef[] = []; + const privateChannels: ChannelRef[] = []; for await (const res of client.paginate("conversations.list", { types: "public_channel,private_channel", exclude_archived: true, @@ -168,7 +167,11 @@ export function createDirectory(deps: { info: { is_private: true, is_ext_shared: c.is_ext_shared, is_pending_ext_shared: c.is_pending_ext_shared }, }); } else { - publicChannels.push({ channelId: c.id, name: c.name }); + publicChannels.push({ + id: c.id, + name: c.name, + info: { is_private: false, is_ext_shared: c.is_ext_shared, is_pending_ext_shared: c.is_pending_ext_shared }, + }); } } } @@ -193,10 +196,11 @@ export function createDirectory(deps: { kind: RosterKind, ): Promise> { const rosters = new Map(); - const slice = refs.slice(0, MAX_PRIVATE_CHANNELS); + const limit = kind.limit ?? MAX_PRIVATE_CHANNELS; + const slice = refs.slice(0, limit); if (refs.length > slice.length) { console.error( - `[slack-plugin] ${refs.length} ${kind.plural} exceed the cap (${MAX_PRIVATE_CHANNELS}); ${refs.length - slice.length} omitted from ${kind.authz} authorization`, + `[slack-plugin] ${refs.length} ${kind.plural} exceed the cap (${limit}); ${refs.length - slice.length} omitted from ${kind.authz} authorization`, ); } for (const ref of slice) { @@ -220,12 +224,24 @@ export function createDirectory(deps: { return rosters; } - async function computePrivateChannelMembership( + async function computeChannelMembership( client: any, - privateChannels: PrivateChannelRef[], + publicChannels: ChannelRef[], + privateChannels: ChannelRef[], ): Promise<{ channels: ChannelRow[]; channelMembers: ChannelMembershipRow[] }> { const channels: ChannelRow[] = []; const channelMembers: ChannelMembershipRow[] = []; + const publicRosters = await allInternalRosters(client, publicChannels, { + plural: "public channels", + authz: "public-channel-capability", + item: "public channel", + limit: publicChannels.length, + }); + for (const channel of publicChannels) { + for (const pid of publicRosters.get(channel.id) ?? []) { + channelMembers.push({ channelId: channel.id, principalId: pid }); + } + } const rosters = await allInternalRosters(client, privateChannels, { plural: "private channels", authz: "private-channel-send", @@ -278,7 +294,6 @@ export function createDirectory(deps: { groupsFetchedAt?: number; } | undefined; - let knownPublicChannelSet = new Set(); const seenGroupIds = new Set(); async function fetchChannels(client: any): Promise<{ @@ -288,17 +303,16 @@ export function createDirectory(deps: { fetchedAt: number; groupsFetchedAt?: number; } | null> { - let listed: { publicChannels: ChannelRow[]; privateChannels: PrivateChannelRef[] }; + let listed: { publicChannels: ChannelRef[]; privateChannels: ChannelRef[] }; try { listed = await listBotChannels(client); } catch (err) { console.error("[slack-plugin] channel list failed:", (err as Error).message); return null; } - knownPublicChannelSet = new Set(listed.publicChannels.map((channel) => channel.channelId)); const fresh = privateChannelsCache && Date.now() - privateChannelsCache.fetchedAt < CHANNEL_MEMBERS_TTL_MS; if (!fresh) { - const computed = await computePrivateChannelMembership(client, listed.privateChannels); + const computed = await computeChannelMembership(client, listed.publicChannels, listed.privateChannels); let groupMembers: GroupMembershipRow[] | undefined; let groupsFetchedAt: number | undefined; try { @@ -315,7 +329,10 @@ export function createDirectory(deps: { } const priv = privateChannelsCache ?? { channels: [], channelMembers: [], fetchedAt: 0 }; return { - channels: [...listed.publicChannels, ...priv.channels], + channels: [ + ...listed.publicChannels.map((channel) => ({ channelId: channel.id, name: channel.name })), + ...priv.channels, + ], channelMembers: priv.channelMembers, fetchedAt: priv.fetchedAt, ...(priv.groupMembers ? { groupMembers: priv.groupMembers, groupsFetchedAt: priv.groupsFetchedAt } : {}), @@ -487,11 +504,6 @@ export function createDirectory(deps: { getChannelInfo, channelMembership, allInternalRosters, - knownPublicChannels: { - has: (channel: string) => knownPublicChannelSet.has(channel), - add: (channel: string) => void knownPublicChannelSet.add(channel), - delete: (channel: string) => void knownPublicChannelSet.delete(channel), - }, syncForUnseenGroup, resolveAutoIdentityMode, maxClassifyMembers: MAX_CLASSIFY_MEMBERS, diff --git a/src/slack/events.ts b/src/slack/events.ts index d1e03078..98f287b5 100644 --- a/src/slack/events.ts +++ b/src/slack/events.ts @@ -39,7 +39,7 @@ export function registerSlackEvents( const { handler, mirror, directory, ids, deduper } = deps; const { dispatch, handleReactionEvent, botHasStakeInThread } = handler; const { mirrorMessageEvent, pushSurfaceEvents } = mirror; - const { knownPublicChannels, syncForUnseenGroup, forceDirectorySync } = directory; + const { syncForUnseenGroup, forceDirectorySync } = directory; app.event("app_mention", async ({ event, body, client, context }: any) => { const e = event as any; @@ -68,10 +68,7 @@ export function registerSlackEvents( app.message(async ({ message, body, client, context }: any) => { const m = message as any; - const privacyChange = channelPrivacyChange(m); - if (privacyChange) { - if (privacyChange.isPrivate) knownPublicChannels.delete(privacyChange.channel); - else knownPublicChannels.add(privacyChange.channel); + if (channelPrivacyChange(m)) { await forceDirectorySync(client); return; } @@ -198,7 +195,7 @@ export function registerSlackEvents( } : {}), }); - } else if (!e.channel || !knownPublicChannels.has(e.channel)) { + } else { await forceDirectorySync(client); } }); @@ -221,7 +218,7 @@ export function registerSlackEvents( ) ) return; - if (!e.channel || !knownPublicChannels.has(e.channel)) await forceDirectorySync(client); + await forceDirectorySync(client); }); app.event("reaction_added", async ({ event, body, client }: any) => { diff --git a/src/slack/index.ts b/src/slack/index.ts index f56f42d6..eb2e2510 100644 --- a/src/slack/index.ts +++ b/src/slack/index.ts @@ -226,6 +226,7 @@ export async function startSlackPlugin( ); } } + await directory.forceDirectorySync(app.client); await app.start(); } catch (err) { stopped = true; @@ -237,7 +238,6 @@ export async function startSlackPlugin( console.log( `[slack-plugin] connected as @${auth.user} (bot ${ids.botUserId}) in team ${auth.team} (${ids.ownTeamId}); in-process core`, ); - void directory.getUserSnapshot(app.client).catch(swallowAs("slack: initial user snapshot", undefined)); ackEmoji.refreshAckEmoji(app.client); let deliveriesPollInFlight = false; diff --git a/test/admin-agent-capability.test.ts b/test/admin-agent-capability.test.ts index d6a4ae8e..a7bb5026 100644 --- a/test/admin-agent-capability.test.ts +++ b/test/admin-agent-capability.test.ts @@ -30,7 +30,10 @@ function start() { signingSecret: SECRET, }), ); - void built.directory.replaceChannels([{ channelId: "C1", name: "agent-admin", isPrivate: false }]); + void built.directory.replaceChannels( + [{ channelId: "C1", name: "agent-admin", isPrivate: false }], + [{ channelId: "C1", principalId: "admin-alice" }], + ); const keychain = createKeychain({ creds: createMemoryMap(), grants: createMemoryMap(), diff --git a/test/capability-routes.test.ts b/test/capability-routes.test.ts index cd9bbd32..0664311f 100644 --- a/test/capability-routes.test.ts +++ b/test/capability-routes.test.ts @@ -64,7 +64,10 @@ describe("capability-token control plane (crons + SOUL)", () => { signingSecret: SECRET, }), ); - await built.directory.replaceChannels([{ channelId: "C", name: "eng", isPrivate: false }]); + await built.directory.replaceChannels( + [{ channelId: "C", name: "eng", isPrivate: false }], + ["admin-alice", "U1", "U2", "U8"].map((principalId) => ({ channelId: "C", principalId })), + ); server = createServer(built.app, { signingSecret: SECRET, scheduler: built.scheduler, diff --git a/test/dm-relay.test.ts b/test/dm-relay.test.ts index c86942c0..f55b6b09 100644 --- a/test/dm-relay.test.ts +++ b/test/dm-relay.test.ts @@ -66,7 +66,10 @@ describe("agent → teammate DM: the cron recipient route (§10)", () => { { channelId: "C-d2", name: "design-backend" }, { channelId: "C-secret", name: "secret", isPrivate: true }, ], - [{ channelId: "C-secret", principalId: "U-carol" }], + [ + { channelId: "C", principalId: "U-carol" }, + { channelId: "C-secret", principalId: "U-carol" }, + ], ); await built.app.upsertGroups([ { groupId: "G-jrs", principalId: "U-carol" }, diff --git a/test/environment-routes.test.ts b/test/environment-routes.test.ts index b904b79f..df7896c6 100644 --- a/test/environment-routes.test.ts +++ b/test/environment-routes.test.ts @@ -36,7 +36,10 @@ describe("environment verbs (list / create / attach, owner-gated)", async () => signingSecret: SECRET, }), ); - await built.directory.replaceChannels([{ channelId: "C-eng", name: "eng", isPrivate: false }]); + await built.directory.replaceChannels( + [{ channelId: "C-eng", name: "eng", isPrivate: false }], + [{ channelId: "C-eng", principalId: "U-owner" }], + ); server = createServer(built.app, { signingSecret: SECRET, scheduler: built.scheduler }); await new Promise((resolve) => server.listen(0, resolve)); base = `http://localhost:${(server.address() as AddressInfo).port}`; diff --git a/test/keychain-ask.test.ts b/test/keychain-ask.test.ts index 313d922d..44a04afc 100644 --- a/test/keychain-ask.test.ts +++ b/test/keychain-ask.test.ts @@ -587,6 +587,7 @@ describe("/v1/keychain/asks — the consent ladder end to end", async () => { { channelId: "C_INFRA", principalId: "U_ALICE" }, { channelId: "C_INFRA", principalId: "U_BOB" }, { channelId: "C_NOALICE", principalId: "U_BOB" }, + { channelId: "C_PUBLIC", principalId: "U_BOB" }, ], ); server = createServer(built.app, { diff --git a/test/keychain.test.ts b/test/keychain.test.ts index c6b5b716..44eb1d9c 100644 --- a/test/keychain.test.ts +++ b/test/keychain.test.ts @@ -968,14 +968,28 @@ describe("/v1/keychain routes (capability-authed)", () => { before(async () => { built = buildApp(testConfig({ dataDir: mkdtempSync(join(tmpdir(), "kc-routes-")), signingSecret: SECRET })); - await built.directory.replaceChannels([ - { channelId: "C_SECONDS", name: "seconds", isPrivate: false }, - { channelId: "C_BAD_EXP", name: "bad-exp", isPrivate: false }, - { channelId: "C7", name: "grants", isPrivate: false }, - { channelId: "C_OVERVIEW", name: "overview", isPrivate: false }, - { channelId: "C_NAMED", name: "pilot-portal", isPrivate: false }, - { channelId: "C8", name: "file-grants", isPrivate: false }, - ]); + await built.directory.replaceChannels( + [ + { channelId: "C_SECONDS", name: "seconds", isPrivate: false }, + { channelId: "C_BAD_EXP", name: "bad-exp", isPrivate: false }, + { channelId: "C7", name: "grants", isPrivate: false }, + { channelId: "C_OVERVIEW", name: "overview", isPrivate: false }, + { channelId: "C_NAMED", name: "pilot-portal", isPrivate: false }, + { channelId: "C_MYSTERY", name: "", isPrivate: false }, + { channelId: "C8", name: "file-grants", isPrivate: false }, + ], + [ + { channelId: "C_SECONDS", principalId: "U_SECONDS" }, + { channelId: "C_BAD_EXP", principalId: "U_BAD_EXP" }, + { channelId: "C7", principalId: "OWNER" }, + { channelId: "C7", principalId: "U3" }, + { channelId: "C_OVERVIEW", principalId: "OVERVIEW_OWNER" }, + { channelId: "C_NAMED", principalId: "SCOPENAME_OWNER" }, + { channelId: "C_MYSTERY", principalId: "SCOPENAME_OWNER" }, + { channelId: "C8", principalId: "OWNER" }, + { channelId: "C8", principalId: "U3" }, + ], + ); await built.directory.replaceGroups([ { groupId: "G_CONN", principalId: "alex@conn" }, { groupId: "G_CONN", principalId: "carol@conn" }, diff --git a/test/oauth-consent-bridge.test.ts b/test/oauth-consent-bridge.test.ts index b22c75c5..5ab3fffd 100644 --- a/test/oauth-consent-bridge.test.ts +++ b/test/oauth-consent-bridge.test.ts @@ -26,10 +26,16 @@ function start( opts: { portalUrl?: string } = { portalUrl: "http://callback.test" }, ): { base: string; built: BuiltApp; close: () => Promise } { const built = buildApp(testConfig({ dataDir: mkdtempSync(join(tmpdir(), "consent-")), signingSecret: SECRET })); - void built.directory.replaceChannels([ - { channelId: "C1", name: "consent", isPrivate: false }, - { channelId: "C9", name: "connectors", isPrivate: false }, - ]); + void built.directory.replaceChannels( + [ + { channelId: "C1", name: "consent", isPrivate: false }, + { channelId: "C9", name: "connectors", isPrivate: false }, + ], + [ + { channelId: "C1", principalId: "U1" }, + { channelId: "C9", principalId: "U1" }, + ], + ); const server = createServer(built.app, { signingSecret: SECRET, replayDedupe: built.replayDedupe, diff --git a/test/projects.test.ts b/test/projects.test.ts index 659ffd82..af77ae29 100644 --- a/test/projects.test.ts +++ b/test/projects.test.ts @@ -200,7 +200,10 @@ test("capability scope checks follow current shared rosters", async () => { { channelId: "C-public", name: "public" }, { channelId: "C-private", name: "private", isPrivate: true }, ], - [{ channelId: "C-private", principalId: "member" }], + [ + { channelId: "C-public", principalId: "member" }, + { channelId: "C-private", principalId: "member" }, + ], 1, ); await built.directory.replaceGroups([{ groupId: "G1", principalId: "member" }], 1); @@ -221,7 +224,7 @@ test("capability scope checks follow current shared rosters", async () => { assert.equal(await built.app.authorizesCapabilityScope({ actorId: "member", scopeId: "channel:C-private" }), false); assert.equal(await built.app.authorizesCapabilityScope({ actorId: "member", scopeId: "group:G1" }), false); - assert.equal(await built.app.authorizesCapabilityScope({ actorId: "member", scopeId: "channel:C-public" }), true); + assert.equal(await built.app.authorizesCapabilityScope({ actorId: "member", scopeId: "channel:C-public" }), false); }); async function listen(server: Server): Promise { diff --git a/test/secret-drop.test.ts b/test/secret-drop.test.ts index a30726c2..92baad5f 100644 --- a/test/secret-drop.test.ts +++ b/test/secret-drop.test.ts @@ -227,7 +227,10 @@ describe("/v1/keychain/drops — mint, form, redeem", async () => { before(async () => { built = buildApp(testConfig({ dataDir: mkdtempSync(join(tmpdir(), "secret-drop-")), signingSecret: SECRET })); - await built.directory.replaceChannels([{ channelId: "C1", name: "drops", isPrivate: false }]); + await built.directory.replaceChannels( + [{ channelId: "C1", name: "drops", isPrivate: false }], + [{ channelId: "C1", principalId: "U_A" }], + ); server = createServer(built.app, { signingSecret: SECRET, keychain: built.keychain, @@ -518,7 +521,10 @@ describe("/v1/keychain/drops — sibling-aware resume", () => { const built = buildApp( testConfig({ dataDir: mkdtempSync(join(tmpdir(), "secret-drop-sib-")), signingSecret: SECRET }), ); - await built.directory.replaceChannels([{ channelId: "C1", name: "drops", isPrivate: false }]); + await built.directory.replaceChannels( + [{ channelId: "C1", name: "drops", isPrivate: false }], + [{ channelId: "C1", principalId: "U_A" }], + ); const fires: DropResolution[] = []; let fired: (() => void) | undefined; const server = createServer(built.app, { diff --git a/test/skills-http.test.ts b/test/skills-http.test.ts index 568dc7d6..1b81b830 100644 --- a/test/skills-http.test.ts +++ b/test/skills-http.test.ts @@ -945,7 +945,10 @@ test("an ORG- or TEAM-homed skill is never inline-managed, even by its author (p test("a PUBLIC channel (self-joinable) stays owner-only — a non-author member cannot edit it", async () => { const srv = await startSecure(); try { - await srv.directory.replaceChannels([{ channelId: "CPUB", name: "general", isPrivate: false }], []); + await srv.directory.replaceChannels( + [{ channelId: "CPUB", name: "general", isPrivate: false }], + ["owner", "rando"].map((principalId) => ({ channelId: "CPUB", principalId })), + ); const created = await fetch(`${srv.base}/v1/skills`, { method: "POST", headers: { diff --git a/test/slack-index.integration.test.ts b/test/slack-index.integration.test.ts index 2f1223b6..137c78d9 100644 --- a/test/slack-index.integration.test.ts +++ b/test/slack-index.integration.test.ts @@ -423,6 +423,35 @@ test("a DM becomes one scoped live turn and one Slack reply", async () => { } }); +test("public channel rosters stay current in the core directory", async () => { + const f = await fixture(); + try { + assert.ok(f.core.directories.some((d: any) => d.channelMembers)); + assert.deepEqual( + f.core.directories + .at(-1) + .channelMembers.filter((m: any) => m.channelId === "C1") + .map((m: any) => m.principalId) + .sort(), + ["U1", "U2"], + ); + + f.client.membersByChannel.set("C1", ["U1", "UBOT"]); + const pushes = f.core.directories.length; + await f.app.emitEvent("member_left_channel", { user: "U2", channel: "C1", event_ts: "100.2" }, "Ev-u2-left"); + await waitFor(() => f.core.directories.length > pushes); + assert.deepEqual( + f.core.directories + .at(-1) + .channelMembers.filter((m: any) => m.channelId === "C1") + .map((m: any) => m.principalId), + ["U1"], + ); + } finally { + await f.stop(); + } +}); + test("a human's DM sets the conversation header to the serving model + web surface", async () => { const f = await fixture({ webUiPublicUrl: "https://claw.example.dev" }); try { diff --git a/test/surface-context.test.ts b/test/surface-context.test.ts index 692233f6..ed7b813a 100644 --- a/test/surface-context.test.ts +++ b/test/surface-context.test.ts @@ -86,7 +86,12 @@ describe("surface-context pulls", async () => { { channelId: "CPUBLIC01", name: "general" }, { channelId: "C-SECRET", name: "warroom", isPrivate: true }, ], - [{ channelId: "C-SECRET", principalId: "U-member" }], + [ + { channelId: "C9", principalId: "U1" }, + { channelId: "C9", principalId: "U-ghost" }, + { channelId: "C9", principalId: "U-member" }, + { channelId: "C-SECRET", principalId: "U-member" }, + ], ); }); From ce5e0413ec0b7bcd6120df3e84772472943a905a Mon Sep 17 00:00:00 2001 From: Josh France <12610835+16francej@users.noreply.github.com> Date: Sat, 15 Aug 2026 11:22:47 -0700 Subject: [PATCH 03/10] Preserve valid channel capability rosters --- src/api/app-messaging.ts | 4 +- src/api/app-types.ts | 7 ++- src/api/routes/directory.ts | 6 +- src/api/slack-core-client.ts | 4 +- src/directory/directory-store.ts | 27 ++++++--- src/directory/postgres-directory-store.ts | 70 ++++++++++++++++++----- src/slack/directory.ts | 46 +++++++++------ src/slack/identity.ts | 9 ++- src/slack/lib.ts | 1 + test/directory-store.test.ts | 18 ++++++ test/postgres-directory-store.test.ts | 18 ++++++ test/slack-identity.test.ts | 18 ++++-- test/slack-index.integration.test.ts | 55 ++++++++++++++++++ 13 files changed, 231 insertions(+), 52 deletions(-) diff --git a/src/api/app-messaging.ts b/src/api/app-messaging.ts index f0e3c9b5..f975250e 100644 --- a/src/api/app-messaging.ts +++ b/src/api/app-messaging.ts @@ -339,8 +339,8 @@ export function createMessagingMethods( }); } }, - async upsertChannels(channels, channelMembers, syncedAt) { - await deps.directory.replaceChannels(channels, channelMembers, syncedAt); + async upsertChannels(channels, channelMembers, syncedAt, channelRosterIds) { + await deps.directory.replaceChannels(channels, channelMembers, syncedAt, channelRosterIds); await h.syncLinkedProjectRosters(); }, async upsertGroups(groupMembers, syncedAt) { diff --git a/src/api/app-types.ts b/src/api/app-types.ts index abe276a0..355e470a 100644 --- a/src/api/app-types.ts +++ b/src/api/app-types.ts @@ -386,7 +386,12 @@ export interface App { ackDeliveryByKey(idempotencyKey: string): Promise; setRunDeliveryState(runId: string, state: RunDeliveryState): Promise; upsertDirectory(members: DirectoryMember[], syncedAt?: number): Promise; - upsertChannels(channels: DirectoryChannel[], channelMembers?: ChannelMembership[], syncedAt?: number): Promise; + upsertChannels( + channels: DirectoryChannel[], + channelMembers?: ChannelMembership[], + syncedAt?: number, + channelRosterIds?: string[], + ): Promise; upsertGroups(groupMembers: GroupMembership[], syncedAt?: number): Promise; setDirectoryWorkspaceUrl(url: string): Promise; directoryMeta(): Promise; diff --git a/src/api/routes/directory.ts b/src/api/routes/directory.ts index d71660a3..13b67cdc 100644 --- a/src/api/routes/directory.ts +++ b/src/api/routes/directory.ts @@ -32,6 +32,7 @@ async function pushDirectory(ctx: ApiCtx): Promise { members?: unknown; channels?: unknown; channelMembers?: unknown; + channelRosterIds?: unknown; groupMembers?: unknown; workspaceUrl?: unknown; membersSyncedAt?: unknown; @@ -78,7 +79,10 @@ async function pushDirectory(ctx: ApiCtx): Promise { isObj(m) && typeof m.channelId === "string" && typeof m.principalId === "string", ) : undefined; - await app.upsertChannels(channels, channelMembers, numOrUndef(b.channelsSyncedAt)); + const channelRosterIds = Array.isArray(b.channelRosterIds) + ? b.channelRosterIds.filter((channelId): channelId is string => typeof channelId === "string") + : undefined; + await app.upsertChannels(channels, channelMembers, numOrUndef(b.channelsSyncedAt), channelRosterIds); channelCount = channels.length; } let groupMemberCount: number | undefined; diff --git a/src/api/slack-core-client.ts b/src/api/slack-core-client.ts index 7519b3f6..f8019793 100644 --- a/src/api/slack-core-client.ts +++ b/src/api/slack-core-client.ts @@ -47,6 +47,7 @@ interface DirectoryPush { members?: Array<{ principalId: string; displayName: string; type: "internal"; slackId?: string }>; channels?: Array<{ channelId: string; name: string; isPrivate?: boolean }>; channelMembers?: Array<{ channelId: string; principalId: string }>; + channelRosterIds?: string[]; groupMembers?: Array<{ groupId: string; principalId: string }>; workspaceUrl?: string; membersSyncedAt?: number; @@ -302,7 +303,8 @@ export function createSlackCoreClient(deps: SlackCoreClientDeps): SlackCoreClien async pushDirectory(body) { if (body.workspaceUrl) await deps.app.setDirectoryWorkspaceUrl(body.workspaceUrl); if (body.members) await deps.app.upsertDirectory(body.members, body.membersSyncedAt); - if (body.channels) await deps.app.upsertChannels(body.channels, body.channelMembers, body.channelsSyncedAt); + if (body.channels) + await deps.app.upsertChannels(body.channels, body.channelMembers, body.channelsSyncedAt, body.channelRosterIds); if (body.groupMembers) await deps.app.upsertGroups(body.groupMembers, body.groupsSyncedAt); }, diff --git a/src/directory/directory-store.ts b/src/directory/directory-store.ts index 007f8bd3..a3b20e0a 100644 --- a/src/directory/directory-store.ts +++ b/src/directory/directory-store.ts @@ -46,6 +46,7 @@ export interface DirectoryStore { channels: DirectoryChannel[], channelMembers?: ChannelMembership[], syncedAt?: number, + channelRosterIds?: string[], ): Promise; list(): Promise; listChannels(): Promise; @@ -94,6 +95,7 @@ export function createDirectoryStore(): DirectoryStore { let members: DirectoryMember[] = []; let channels: DirectoryChannel[] = []; let channelMembers: Map> | undefined; + let knownChannelRosters: Set | undefined; let groupMembers: Map> | undefined; let groupsSynced = false; let workspaceUrl: string | undefined; @@ -122,16 +124,26 @@ export function createDirectoryStore(): DirectoryStore { members = next.filter((m) => m.principalId && m.type === "internal"); return true; }, - async replaceChannels(nextChannels, nextChannelMembers, syncedAt) { + async replaceChannels(nextChannels, nextChannelMembers, syncedAt, nextChannelRosterIds) { if (!acceptSync("channels", syncedAt)) return false; channels = nextChannels.filter((c) => c.channelId && c.name); + const listed = new Set(channels.map((channel) => channel.channelId)); + knownChannelRosters = knownChannelRosters + ? new Set([...knownChannelRosters].filter((channelId) => listed.has(channelId))) + : undefined; if (nextChannelMembers !== undefined) { - const byChannel = new Map>(); + const rosterIds = new Set(nextChannelRosterIds ?? channels.map((channel) => channel.channelId)); + const byChannel = new Map( + [...(channelMembers ?? [])].filter(([channelId]) => listed.has(channelId) && !rosterIds.has(channelId)), + ); + for (const channelId of rosterIds) if (listed.has(channelId)) byChannel.set(channelId, new Set()); for (const m of nextChannelMembers) { - if (!m.channelId || !m.principalId) continue; + if (!m.channelId || !m.principalId || !rosterIds.has(m.channelId) || !listed.has(m.channelId)) continue; (byChannel.get(m.channelId) ?? byChannel.set(m.channelId, new Set()).get(m.channelId)!).add(m.principalId); } channelMembers = byChannel; + knownChannelRosters ??= new Set(); + for (const channelId of rosterIds) if (listed.has(channelId)) knownChannelRosters.add(channelId); } return true; }, @@ -139,14 +151,13 @@ export function createDirectoryStore(): DirectoryStore { return channelMembers?.get(channelId)?.has(principalId) ?? false; }, async channelMemberIds(channelId) { - if (!channelMembers) return undefined; - return [...(channelMembers.get(channelId) ?? [])]; + if (!knownChannelRosters?.has(channelId)) return undefined; + return [...(channelMembers?.get(channelId) ?? [])]; }, async channelMembership(channelId, principalId) { - const memberships = channelMembers; const channel = channels.find((candidate) => candidate.channelId === channelId); - if (!memberships || !channel) return undefined; - const member = memberships.get(channelId)?.has(principalId) ?? false; + if (!channel || !knownChannelRosters?.has(channelId)) return undefined; + const member = channelMembers?.get(channelId)?.has(principalId) ?? false; return member || channel.isPrivate === true ? member : undefined; }, async channelPrivacy(channelId) { diff --git a/src/directory/postgres-directory-store.ts b/src/directory/postgres-directory-store.ts index 975e12a1..91b345be 100644 --- a/src/directory/postgres-directory-store.ts +++ b/src/directory/postgres-directory-store.ts @@ -42,6 +42,7 @@ const SCHEMA = [ name TEXT NOT NULL, name_lc TEXT NOT NULL, is_private BOOLEAN NOT NULL DEFAULT FALSE, + roster_known BOOLEAN NOT NULL DEFAULT FALSE, PRIMARY KEY (org_id, channel_id) )`, `CREATE INDEX IF NOT EXISTS directory_channels_name @@ -88,6 +89,17 @@ const SCHEMA = [ `ALTER TABLE directory_sync ADD COLUMN IF NOT EXISTS members_synced_at BIGINT`, `ALTER TABLE directory_sync ADD COLUMN IF NOT EXISTS channels_synced_at BIGINT`, `ALTER TABLE directory_sync ADD COLUMN IF NOT EXISTS groups_synced_at BIGINT`, + `DO $$ + BEGIN + IF NOT EXISTS ( + SELECT 1 FROM information_schema.columns + WHERE table_name = 'directory_channels' AND column_name = 'roster_known' + ) THEN + ALTER TABLE directory_channels ADD COLUMN roster_known BOOLEAN NOT NULL DEFAULT FALSE; + UPDATE directory_channels c SET roster_known = TRUE + FROM directory_sync s WHERE c.org_id = s.org_id AND s.channel_members_synced = TRUE; + END IF; + END $$`, `CREATE TABLE IF NOT EXISTS directory_meta( org_id TEXT PRIMARY KEY, workspace_url TEXT, @@ -249,36 +261,64 @@ export function createPostgresDirectoryStore(connectionString: string): Director }); }, - async replaceChannels(channels, channelMembers, syncedAt) { + async replaceChannels(channels, channelMembers, syncedAt, channelRosterIds) { const byId = new Map(); for (const c of channels) if (c.channelId && c.name) byId.set(c.channelId, c); const list = [...byId.values()]; - - const membershipRows = channelMembers === undefined ? undefined : dedupMemberships(channelMembers); + const listedIds = new Set(list.map((channel) => channel.channelId)); + + const rosterIds = + channelMembers === undefined + ? undefined + : new Set((channelRosterIds ?? [...listedIds]).filter((channelId) => listedIds.has(channelId))); + const membershipRows = + channelMembers === undefined + ? undefined + : dedupMemberships(channelMembers).filter((member) => rosterIds!.has(member.channelId)); const channelsPart = list.map((c) => `${c.channelId}|${c.name}|${c.isPrivate ? 1 : 0}`); const membersPart = membershipRows === undefined ? [] - : ["members:known", ...membershipRows.map((m) => `m:${m.channelId}|${m.principalId}`)]; + : [ + ...[...rosterIds!].map((channelId) => `r:${channelId}`), + ...membershipRows.map((m) => `m:${m.channelId}|${m.principalId}`), + ]; const hash = hashRoster([...channelsPart, ...membersPart]); const applied = await swapIfChanged("channels_hash", hash, syncedAt, async (client) => { - await client.query("DELETE FROM directory_channels WHERE org_id = $1", [orgId]); + const channelIds = [...listedIds]; + await client.query("DELETE FROM directory_channels WHERE org_id = $1 AND NOT (channel_id = ANY($2::text[]))", [ + orgId, + channelIds, + ]); + await client.query( + "DELETE FROM directory_channel_members WHERE org_id = $1 AND NOT (channel_id = ANY($2::text[]))", + [orgId, channelIds], + ); if (list.length) { await client.query( - `INSERT INTO directory_channels (org_id, channel_id, name, name_lc, is_private) - SELECT $1, * FROM unnest($2::text[], $3::text[], $4::text[], $5::boolean[])`, + `INSERT INTO directory_channels (org_id, channel_id, name, name_lc, is_private, roster_known) + SELECT $1, * FROM unnest($2::text[], $3::text[], $4::text[], $5::boolean[], $6::boolean[]) + ON CONFLICT (org_id, channel_id) DO UPDATE SET + name = EXCLUDED.name, + name_lc = EXCLUDED.name_lc, + is_private = EXCLUDED.is_private, + roster_known = directory_channels.roster_known OR EXCLUDED.roster_known`, [ orgId, - list.map((c) => c.channelId), + channelIds, list.map((c) => c.name), list.map((c) => normDirectoryQuery(c.name)), list.map((c) => !!c.isPrivate), + list.map((c) => rosterIds?.has(c.channelId) ?? false), ], ); } if (membershipRows !== undefined) { - await client.query("DELETE FROM directory_channel_members WHERE org_id = $1", [orgId]); + await client.query( + "DELETE FROM directory_channel_members WHERE org_id = $1 AND channel_id = ANY($2::text[])", + [orgId, [...rosterIds!]], + ); if (membershipRows.length) { await client.query( `INSERT INTO directory_channel_members (org_id, channel_id, principal_id) @@ -316,8 +356,11 @@ export function createPostgresDirectoryStore(connectionString: string): Director }, async channelMemberIds(channelId) { - const synced = await q("SELECT channel_members_synced FROM directory_sync WHERE org_id = $1", [orgId]); - if (synced[0]?.channel_members_synced !== true) return undefined; + const known = await q("SELECT roster_known FROM directory_channels WHERE org_id = $1 AND channel_id = $2", [ + orgId, + channelId, + ]); + if (known[0]?.roster_known !== true) return undefined; const rows = await q( "SELECT principal_id FROM directory_channel_members WHERE org_id = $1 AND channel_id = $2 ORDER BY principal_id", [orgId, channelId], @@ -332,11 +375,12 @@ export function createPostgresDirectoryStore(connectionString: string): Director WHERE org_id = $1 AND channel_id = $2 AND principal_id = $3 ) AS member, (SELECT is_private FROM directory_channels WHERE org_id = $1 AND channel_id = $2) AS is_private, - (SELECT channel_members_synced FROM directory_sync WHERE org_id = $1) AS synced`, + (SELECT roster_known FROM directory_channels WHERE org_id = $1 AND channel_id = $2) AS roster_known`, [orgId, channelId, principalId], ); const member = rows[0]?.member === true; - return member || (rows[0]?.is_private === true && rows[0]?.synced === true) ? member : undefined; + if (rows[0]?.roster_known !== true) return undefined; + return member || rows[0]?.is_private === true ? member : undefined; }, async channelPrivacy(channelId) { diff --git a/src/slack/directory.ts b/src/slack/directory.ts index c02b2a5e..333ebb1b 100644 --- a/src/slack/directory.ts +++ b/src/slack/directory.ts @@ -5,6 +5,7 @@ import { type SlackIdentityMode, type SlackUser, allInternalChannelMembers, + internalChannelMembers, classifyUser, createRefreshCoalescer, createUserCache, @@ -52,10 +53,9 @@ interface ChannelRef { info: ChannelMeta; } -type RosterKind = { plural: string; authz: string; item: string; limit?: number }; +type RosterKind = { plural: string; authz: string; item: string; limit?: number; allowExternal?: boolean }; const MEMBERS_PAGE_LIMIT = 200; -const MAX_CLASSIFY_MEMBERS = 200; export interface Directory { getUserSnapshot(client: any): Promise<{ byId: Map; fetchedAt: number } | undefined>; @@ -81,7 +81,6 @@ export interface Directory { ): Promise>; syncForUnseenGroup(client: any, groupId: string): void; resolveAutoIdentityMode(client: any): Promise; - maxClassifyMembers: number; } export function createDirectory(deps: { @@ -178,16 +177,15 @@ export function createDirectory(deps: { return { publicChannels, privateChannels }; } - async function fetchChannelMemberIds(client: any, channel: string): Promise<{ ids: string[]; complete: boolean }> { + async function fetchChannelMemberIds(client: any, channel: string): Promise { const memberIds: string[] = []; for await (const res of client.paginate("conversations.members", { channel, limit: MEMBERS_PAGE_LIMIT, }) as AsyncIterable) { for (const id of res.members ?? []) if (id !== ids.botUserId) memberIds.push(id); - if (memberIds.length > MAX_CLASSIFY_MEMBERS) return { ids: memberIds, complete: false }; } - return { ids: memberIds, complete: true }; + return memberIds; } async function allInternalRosters( @@ -204,21 +202,23 @@ export function createDirectory(deps: { ); } for (const ref of slice) { - let fetched: { ids: string[]; complete: boolean }; + let memberIds: string[]; try { - fetched = await fetchChannelMemberIds(client, ref.id); + memberIds = await fetchChannelMemberIds(client, ref.id); } catch (err) { console.error(`[slack-plugin] members fetch failed for ${kind.item} ${ref.id}:`, (err as Error).message); continue; } const actors: ActorAssertion[] = []; - let complete = fetched.complete; - for (const id of fetched.ids) { + let complete = true; + for (const id of memberIds) { const { actor, ok } = await classifyUserCached(client, id); actors.push(actor); if (!ok) complete = false; } - const internalIds = allInternalChannelMembers(actors, complete, ref.info); + const internalIds = kind.allowExternal + ? internalChannelMembers(actors, complete) + : allInternalChannelMembers(actors, complete, ref.info); if (internalIds) rosters.set(ref.id, internalIds); } return rosters; @@ -228,17 +228,22 @@ export function createDirectory(deps: { client: any, publicChannels: ChannelRef[], privateChannels: ChannelRef[], - ): Promise<{ channels: ChannelRow[]; channelMembers: ChannelMembershipRow[] }> { + ): Promise<{ channels: ChannelRow[]; channelMembers: ChannelMembershipRow[]; channelRosterIds: string[] }> { const channels: ChannelRow[] = []; const channelMembers: ChannelMembershipRow[] = []; + const channelRosterIds: string[] = []; const publicRosters = await allInternalRosters(client, publicChannels, { plural: "public channels", authz: "public-channel-capability", item: "public channel", limit: publicChannels.length, + allowExternal: true, }); for (const channel of publicChannels) { - for (const pid of publicRosters.get(channel.id) ?? []) { + const internalIds = publicRosters.get(channel.id); + if (!internalIds) continue; + channelRosterIds.push(channel.id); + for (const pid of internalIds) { channelMembers.push({ channelId: channel.id, principalId: pid }); } } @@ -248,12 +253,13 @@ export function createDirectory(deps: { item: "private channel", }); for (const c of privateChannels) { + channels.push({ channelId: c.id, name: c.name, isPrivate: true }); const internalIds = rosters.get(c.id); if (!internalIds) continue; - channels.push({ channelId: c.id, name: c.name, isPrivate: true }); + channelRosterIds.push(c.id); for (const pid of internalIds) channelMembers.push({ channelId: c.id, principalId: pid }); } - return { channels, channelMembers }; + return { channels, channelMembers, channelRosterIds }; } async function listBotGroupDms(client: any): Promise { @@ -289,6 +295,7 @@ export function createDirectory(deps: { | { channels: ChannelRow[]; channelMembers: ChannelMembershipRow[]; + channelRosterIds: string[]; groupMembers?: GroupMembershipRow[]; fetchedAt: number; groupsFetchedAt?: number; @@ -299,6 +306,7 @@ export function createDirectory(deps: { async function fetchChannels(client: any): Promise<{ channels: ChannelRow[]; channelMembers: ChannelMembershipRow[]; + channelRosterIds: string[]; groupMembers?: GroupMembershipRow[]; fetchedAt: number; groupsFetchedAt?: number; @@ -327,13 +335,14 @@ export function createDirectory(deps: { } privateChannelsCache = { ...computed, groupMembers, groupsFetchedAt, fetchedAt: Date.now() }; } - const priv = privateChannelsCache ?? { channels: [], channelMembers: [], fetchedAt: 0 }; + const priv = privateChannelsCache ?? { channels: [], channelMembers: [], channelRosterIds: [], fetchedAt: 0 }; return { channels: [ ...listed.publicChannels.map((channel) => ({ channelId: channel.id, name: channel.name })), ...priv.channels, ], channelMembers: priv.channelMembers, + channelRosterIds: priv.channelRosterIds, fetchedAt: priv.fetchedAt, ...(priv.groupMembers ? { groupMembers: priv.groupMembers, groupsFetchedAt: priv.groupsFetchedAt } : {}), }; @@ -361,6 +370,7 @@ export function createDirectory(deps: { ? { channels: fetched.channels, channelMembers: fetched.channelMembers, + channelRosterIds: fetched.channelRosterIds, channelsSyncedAt: fetched.fetchedAt, ...(fetched.groupMembers ? { groupMembers: fetched.groupMembers, groupsSyncedAt: fetched.groupsFetchedAt } @@ -458,13 +468,12 @@ export function createDirectory(deps: { slackIdsByPrincipal?: Map; }> { try { - const { ids: memberIds } = await fetchChannelMemberIds(client, channel); + const memberIds = await fetchChannelMemberIds(client, channel); return await resolveChannelMembership({ memberIds, actor, actorSlackId, info, - maxClassifyMembers: MAX_CLASSIFY_MEMBERS, classify: (id) => classifyUserCached(client, id), }); } catch { @@ -506,6 +515,5 @@ export function createDirectory(deps: { allInternalRosters, syncForUnseenGroup, resolveAutoIdentityMode, - maxClassifyMembers: MAX_CLASSIFY_MEMBERS, }; } diff --git a/src/slack/identity.ts b/src/slack/identity.ts index df702c43..3e90fbb7 100644 --- a/src/slack/identity.ts +++ b/src/slack/identity.ts @@ -163,8 +163,13 @@ export function allInternalChannelMembers( if (isExternallyShared(info)) return undefined; const humans = members.filter((m) => !m.isBot); if (humans.some((m) => m.isExternalGuest)) return undefined; + return internalChannelMembers(humans, true); +} + +export function internalChannelMembers(members: ActorAssertion[], complete: boolean): string[] | undefined { + if (!complete) return undefined; const ids = new Set(); - for (const m of humans) if (m.externalId) ids.add(m.externalId); + for (const m of members) if (m.externalId && !m.isExternalGuest && !m.isBot) ids.add(m.externalId); return [...ids]; } @@ -173,7 +178,6 @@ export async function resolveChannelMembership(opts: { actor: ActorAssertion; actorSlackId: string; info: ChannelMeta | undefined; - maxClassifyMembers: number; classify(id: string): Promise<{ actor: ActorAssertion; ok: boolean }>; }): Promise<{ audience: ActorAssertion[]; @@ -181,7 +185,6 @@ export async function resolveChannelMembership(opts: { slackIdsByPrincipal?: Map; }> { const { memberIds, actor, info } = opts; - if (memberIds.length > opts.maxClassifyMembers) return { audience: [actor, externalMarker()] }; if (!memberIds.includes(opts.actorSlackId)) return { audience: [actor, externalMarker()] }; const members: ActorAssertion[] = []; diff --git a/src/slack/lib.ts b/src/slack/lib.ts index 3503667c..14b7b22c 100644 --- a/src/slack/lib.ts +++ b/src/slack/lib.ts @@ -27,6 +27,7 @@ export { computeChannelAudience, computePublishMembers, allInternalChannelMembers, + internalChannelMembers, resolveChannelMembership, } from "./identity.ts"; export { diff --git a/test/directory-store.test.ts b/test/directory-store.test.ts index d394a7af..fc91f699 100644 --- a/test/directory-store.test.ts +++ b/test/directory-store.test.ts @@ -236,4 +236,22 @@ describe("private-channel membership (authorizes private-channel sends, §10)", assert.equal(await d.channelMembership("C-public", "U-alice"), undefined); assert.equal(await d.channelMembership("C-sec", "U-alice"), false); }); + + it("partially replaces only the channel rosters known by the source", async () => { + const d = createDirectoryStore(); + const channels = [ + { channelId: "C-one", name: "one", isPrivate: true }, + { channelId: "C-two", name: "two", isPrivate: true }, + { channelId: "C-new", name: "new", isPrivate: true }, + ]; + await d.replaceChannels(channels.slice(0, 2), [ + { channelId: "C-one", principalId: "U-old-one" }, + { channelId: "C-two", principalId: "U-old-two" }, + ]); + await d.replaceChannels(channels, [{ channelId: "C-two", principalId: "U-new-two" }], undefined, ["C-two"]); + assert.equal(await d.channelMembership("C-one", "U-old-one"), true); + assert.equal(await d.channelMembership("C-two", "U-old-two"), false); + assert.equal(await d.channelMembership("C-two", "U-new-two"), true); + assert.equal(await d.channelMembership("C-new", "U-new"), undefined); + }); }); diff --git a/test/postgres-directory-store.test.ts b/test/postgres-directory-store.test.ts index 0111dec3..18c2cf54 100644 --- a/test/postgres-directory-store.test.ts +++ b/test/postgres-directory-store.test.ts @@ -365,6 +365,24 @@ test("pg directory: a swap stamped older than the stored snapshot is refused", { assert.equal((await store.listChannels()).length, 0); }); +test("pg directory: a partial roster swap preserves channels whose roster is unknown", { skip }, async () => { + const store = createPostgresDirectoryStore(URL!); + const channels = [ + { channelId: "C-one", name: "one", isPrivate: true }, + { channelId: "C-two", name: "two", isPrivate: true }, + { channelId: "C-new", name: "new", isPrivate: true }, + ]; + await store.replaceChannels(channels.slice(0, 2), [ + { channelId: "C-one", principalId: "U-old-one" }, + { channelId: "C-two", principalId: "U-old-two" }, + ]); + await store.replaceChannels(channels, [{ channelId: "C-two", principalId: "U-new-two" }], undefined, ["C-two"]); + assert.equal(await store.channelMembership("C-one", "U-old-one"), true); + assert.equal(await store.channelMembership("C-two", "U-old-two"), false); + assert.equal(await store.channelMembership("C-two", "U-new-two"), true); + assert.equal(await store.channelMembership("C-new", "U-new"), undefined); +}); + test( "pg directory: an identical push still advances the stamp, so ordering survives content-idempotent pushes", { skip }, diff --git a/test/slack-identity.test.ts b/test/slack-identity.test.ts index 922388ed..e351c1f0 100644 --- a/test/slack-identity.test.ts +++ b/test/slack-identity.test.ts @@ -265,7 +265,6 @@ function membershipDeps(overrides: Partial ({ actor: byId[id] ?? { externalId: id, isExternalGuest: true }, ok: Boolean(byId[id]), @@ -299,10 +298,21 @@ test("resolveChannelMembership: never matches the sender's email against raw mem ); }); -test("resolveChannelMembership fails closed past the classify ceiling and withholds publishMembers on incomplete classify", async () => { +test("resolveChannelMembership handles large channels and withholds publishMembers on incomplete classify", async () => { const big = Array.from({ length: 201 }, (_, i) => `U${i}`); - const capped = await resolveChannelMembership(membershipDeps({ memberIds: big, actorSlackId: "U1" })); - assert.ok(capped.audience.some((a) => a.isExternalGuest)); + const large = await resolveChannelMembership( + membershipDeps({ + memberIds: big, + actorSlackId: "U1", + classify: async (id: string) => ({ + actor: { externalId: id === "U1" ? "alice@acme.com" : id, isExternalGuest: false }, + ok: true, + }), + }), + ); + assert.equal(large.audience.length, 201); + assert.ok(!large.audience.some((a) => a.isExternalGuest)); + assert.equal(large.publishMembers?.length, 201); const incomplete = await resolveChannelMembership( membershipDeps({ diff --git a/test/slack-index.integration.test.ts b/test/slack-index.integration.test.ts index 137c78d9..68a8e6e5 100644 --- a/test/slack-index.integration.test.ts +++ b/test/slack-index.integration.test.ts @@ -452,6 +452,61 @@ test("public channel rosters stay current in the core directory", async () => { } }); +test("large public channels publish their complete roster and accept internal turns", async () => { + const f = await fixture(); + try { + const members = Array.from({ length: 201 }, (_, i) => `UL${i}`); + for (const id of members) f.client.usersById.set(id, internalUser(id, id)); + f.client.membersByChannel.set("C1", [...members, "UBOT"]); + const pushes = f.core.directories.length; + await f.app.emitEvent("member_joined_channel", { user: members[0], channel: "C1", event_ts: "100.3" }); + await waitFor(() => f.core.directories.length > pushes); + assert.equal( + f.core.directories.at(-1).channelMembers.filter((m: any) => m.channelId === "C1").length, + members.length, + ); + + await f.app.emitEvent("app_mention", { + channel: "C1", + channel_type: "channel", + user: members[0], + text: "<@UBOT> hello", + ts: "100.4", + }); + assert.equal(f.core.turns.length, 1); + } finally { + await f.stop(); + } +}); + +test("failed roster reads are marked unknown instead of clearing known members", async () => { + const f = await fixture(); + try { + assert.ok(f.core.directories.at(-1).channelRosterIds.includes("C1")); + f.client.membershipFailures.add("C1"); + const pushes = f.core.directories.length; + await f.app.emitEvent("member_left_channel", { user: "U2", channel: "C1", event_ts: "100.5" }); + await waitFor(() => f.core.directories.length > pushes); + assert.ok(!f.core.directories.at(-1).channelRosterIds.includes("C1")); + } finally { + await f.stop(); + } +}); + +test("Slack Connect directory rosters contain only internal humans", async () => { + const f = await fixture({ externalParticipants: true }); + try { + const pushed = f.core.directories.at(-1); + assert.ok(pushed.channelRosterIds.includes("CX")); + assert.deepEqual( + pushed.channelMembers.filter((m: any) => m.channelId === "CX").map((m: any) => m.principalId), + ["U1"], + ); + } finally { + await f.stop(); + } +}); + test("a human's DM sets the conversation header to the serving model + web surface", async () => { const f = await fixture({ webUiPublicUrl: "https://claw.example.dev" }); try { From 965ef49c9b54e0748bb694426f08634e392b696c Mon Sep 17 00:00:00 2001 From: Josh France <12610835+16francej@users.noreply.github.com> Date: Sat, 15 Aug 2026 12:29:20 -0700 Subject: [PATCH 04/10] Preserve capabilities through roster transitions --- src/api/app-helpers.ts | 11 +- src/api/app-messaging.ts | 14 +- src/api/app-types.ts | 14 +- src/api/routes/directory.ts | 37 ++++- src/api/slack-core-client.ts | 16 +- src/deploy/docker-deploy-provider.ts | 104 ++++++++++++- src/directory/directory-store.ts | 89 ++++++++++- src/directory/postgres-directory-store.ts | 169 +++++++++++++++++++- src/slack/directory.ts | 179 +++++++++++++++++++--- src/slack/events.ts | 4 +- test/capability-routes.test.ts | 14 ++ test/directory-store.test.ts | 68 ++++++++ test/docker-deploy-provider.test.ts | 83 +++++++++- test/postgres-directory-store.test.ts | 70 ++++++++- test/projects.test.ts | 24 +++ test/slack-index.integration.test.ts | 82 +++++++++- 16 files changed, 922 insertions(+), 56 deletions(-) diff --git a/src/api/app-helpers.ts b/src/api/app-helpers.ts index c9be4ef7..9c42dd48 100644 --- a/src/api/app-helpers.ts +++ b/src/api/app-helpers.ts @@ -404,7 +404,16 @@ export function createAppHelpers(deps: AppDeps, app: App) { claims: Pick, ): Promise { const { kind, ref } = parseScopeId(claims.scopeId); - if ((kind === "channel" || kind === "group") && !(await principalCanWriteScope(claims.actorId, claims.scopeId))) { + if (kind === "channel" && !deps.identity.isInternal(deps.identity.classify(claims.actorId))) return false; + const capabilityMembership = + kind === "channel" + ? await deps.directory.channelCapabilityMembership(ref, claims.actorId).catch(() => undefined) + : undefined; + if ( + (kind === "channel" && + !(capabilityMembership ?? (await principalCanAccessCurrentScope(claims.actorId, claims.scopeId)))) || + (kind === "group" && !(await principalCanWriteScope(claims.actorId, claims.scopeId))) + ) { return false; } if (kind !== "group" || deps.projects?.recognizes(ref) !== true) return true; diff --git a/src/api/app-messaging.ts b/src/api/app-messaging.ts index f975250e..6458f4e9 100644 --- a/src/api/app-messaging.ts +++ b/src/api/app-messaging.ts @@ -59,6 +59,7 @@ export function createMessagingMethods( | "setRunDeliveryState" | "upsertDirectory" | "upsertChannels" + | "upsertCapabilityChannels" | "upsertGroups" | "setDirectoryWorkspaceUrl" | "directoryMeta" @@ -343,8 +344,17 @@ export function createMessagingMethods( await deps.directory.replaceChannels(channels, channelMembers, syncedAt, channelRosterIds); await h.syncLinkedProjectRosters(); }, - async upsertGroups(groupMembers, syncedAt) { - await deps.directory.replaceGroups(groupMembers, syncedAt); + async upsertGroups(groupMembers, syncedAt, groupIds, groupRosterIds) { + await deps.directory.replaceGroups(groupMembers, syncedAt, groupIds, groupRosterIds); + }, + async upsertCapabilityChannels(channelIds, channelMembers, channelRosterIds, syncedAt, revocations) { + await deps.directory.replaceCapabilityChannels( + channelIds, + channelMembers, + channelRosterIds, + syncedAt, + revocations, + ); }, async setDirectoryWorkspaceUrl(url) { await deps.directory.setWorkspaceUrl(url); diff --git a/src/api/app-types.ts b/src/api/app-types.ts index 355e470a..c7dcfa5a 100644 --- a/src/api/app-types.ts +++ b/src/api/app-types.ts @@ -392,7 +392,19 @@ export interface App { syncedAt?: number, channelRosterIds?: string[], ): Promise; - upsertGroups(groupMembers: GroupMembership[], syncedAt?: number): Promise; + upsertGroups( + groupMembers: GroupMembership[], + syncedAt?: number, + groupIds?: string[], + groupRosterIds?: string[], + ): Promise; + upsertCapabilityChannels( + channelIds: string[], + channelMembers: ChannelMembership[], + channelRosterIds: string[], + syncedAt?: number, + revocations?: ChannelMembership[], + ): Promise; setDirectoryWorkspaceUrl(url: string): Promise; directoryMeta(): Promise; resolveRecipient(query: string): Promise; diff --git a/src/api/routes/directory.ts b/src/api/routes/directory.ts index 13b67cdc..88b7d639 100644 --- a/src/api/routes/directory.ts +++ b/src/api/routes/directory.ts @@ -33,7 +33,12 @@ async function pushDirectory(ctx: ApiCtx): Promise { channels?: unknown; channelMembers?: unknown; channelRosterIds?: unknown; + capabilityChannelMembers?: unknown; + capabilityChannelRosterIds?: unknown; + capabilityChannelRevocations?: unknown; groupMembers?: unknown; + groupIds?: unknown; + groupRosterIds?: unknown; workspaceUrl?: unknown; membersSyncedAt?: unknown; channelsSyncedAt?: unknown; @@ -83,6 +88,30 @@ async function pushDirectory(ctx: ApiCtx): Promise { ? b.channelRosterIds.filter((channelId): channelId is string => typeof channelId === "string") : undefined; await app.upsertChannels(channels, channelMembers, numOrUndef(b.channelsSyncedAt), channelRosterIds); + const capabilityChannelMembers = Array.isArray(b.capabilityChannelMembers) + ? b.capabilityChannelMembers.filter( + (m): m is { channelId: string; principalId: string } => + isObj(m) && typeof m.channelId === "string" && typeof m.principalId === "string", + ) + : undefined; + const capabilityChannelRosterIds = Array.isArray(b.capabilityChannelRosterIds) + ? b.capabilityChannelRosterIds.filter((channelId): channelId is string => typeof channelId === "string") + : undefined; + const capabilityChannelRevocations = Array.isArray(b.capabilityChannelRevocations) + ? b.capabilityChannelRevocations.filter( + (m): m is { channelId: string; principalId: string } => + isObj(m) && typeof m.channelId === "string" && typeof m.principalId === "string", + ) + : undefined; + if (capabilityChannelMembers && capabilityChannelRosterIds) { + await app.upsertCapabilityChannels( + channels.map((channel) => channel.channelId), + capabilityChannelMembers, + capabilityChannelRosterIds, + numOrUndef(b.channelsSyncedAt), + capabilityChannelRevocations, + ); + } channelCount = channels.length; } let groupMemberCount: number | undefined; @@ -91,7 +120,13 @@ async function pushDirectory(ctx: ApiCtx): Promise { (m): m is { groupId: string; principalId: string } => isObj(m) && typeof m.groupId === "string" && typeof m.principalId === "string", ); - await app.upsertGroups(groupMembers, numOrUndef(b.groupsSyncedAt)); + const groupIds = Array.isArray(b.groupIds) + ? b.groupIds.filter((groupId): groupId is string => typeof groupId === "string") + : undefined; + const groupRosterIds = Array.isArray(b.groupRosterIds) + ? b.groupRosterIds.filter((groupId): groupId is string => typeof groupId === "string") + : undefined; + await app.upsertGroups(groupMembers, numOrUndef(b.groupsSyncedAt), groupIds, groupRosterIds); groupMemberCount = groupMembers.length; } return sendJson(res, 200, { diff --git a/src/api/slack-core-client.ts b/src/api/slack-core-client.ts index f8019793..170fcc51 100644 --- a/src/api/slack-core-client.ts +++ b/src/api/slack-core-client.ts @@ -48,7 +48,12 @@ interface DirectoryPush { channels?: Array<{ channelId: string; name: string; isPrivate?: boolean }>; channelMembers?: Array<{ channelId: string; principalId: string }>; channelRosterIds?: string[]; + capabilityChannelMembers?: Array<{ channelId: string; principalId: string }>; + capabilityChannelRosterIds?: string[]; + capabilityChannelRevocations?: Array<{ channelId: string; principalId: string }>; groupMembers?: Array<{ groupId: string; principalId: string }>; + groupIds?: string[]; + groupRosterIds?: string[]; workspaceUrl?: string; membersSyncedAt?: number; channelsSyncedAt?: number; @@ -305,7 +310,16 @@ export function createSlackCoreClient(deps: SlackCoreClientDeps): SlackCoreClien if (body.members) await deps.app.upsertDirectory(body.members, body.membersSyncedAt); if (body.channels) await deps.app.upsertChannels(body.channels, body.channelMembers, body.channelsSyncedAt, body.channelRosterIds); - if (body.groupMembers) await deps.app.upsertGroups(body.groupMembers, body.groupsSyncedAt); + if (body.channels && body.capabilityChannelMembers && body.capabilityChannelRosterIds) + await deps.app.upsertCapabilityChannels( + body.channels.map((channel) => channel.channelId), + body.capabilityChannelMembers, + body.capabilityChannelRosterIds, + body.channelsSyncedAt, + body.capabilityChannelRevocations, + ); + if (body.groupMembers) + await deps.app.upsertGroups(body.groupMembers, body.groupsSyncedAt, body.groupIds, body.groupRosterIds); }, claimDeliveries(type, claimMs) { diff --git a/src/deploy/docker-deploy-provider.ts b/src/deploy/docker-deploy-provider.ts index c6a2058b..4ef1193f 100644 --- a/src/deploy/docker-deploy-provider.ts +++ b/src/deploy/docker-deploy-provider.ts @@ -3,6 +3,7 @@ import type { DeployEndpoint, DeployProvider } from "./deploy-provider.ts"; import { spawnDockerExec, type DockerExec } from "../sandbox/docker-exec.ts"; const APP_PORT = 8080; +const LEGACY_NETWORK = "agent-deploynet"; export interface DockerDeployProviderOptions { image?: string; @@ -36,8 +37,7 @@ export function createDockerDeployProvider(opts: DockerDeployProviderOptions = { const name = (d: Deployment) => `agent-deploy-${d.id.slice(0, 12)}`; const network = (d: Deployment) => `${name(d)}-net`; - const ensureNetwork = async (d: Deployment): Promise => { - const net = network(d); + const ensureNetwork = async (net: string): Promise => { if ((await dexec(["network", "inspect", net])).code !== 0) { const r = await dexec(["network", "create", net]); if (r.code !== 0 && !/already exists/i.test(r.stderr)) { @@ -47,11 +47,102 @@ export function createDockerDeployProvider(opts: DockerDeployProviderOptions = { return net; }; + const migrateContainer = async (container: string): Promise => { + const inspected = await dexec(["inspect", "--format", "{{json .NetworkSettings.Networks}}", container]); + if (inspected.code !== 0) return false; + let attached: Record; + try { + attached = JSON.parse(inspected.stdout) as Record; + } catch { + return false; + } + const target = `${container}-net`; + try { + await ensureNetwork(target); + } catch { + return false; + } + if (!(target in attached) && (await dexec(["network", "connect", target, container])).code !== 0) return false; + if (LEGACY_NETWORK in attached && (await dexec(["network", "disconnect", LEGACY_NETWORK, container])).code !== 0) + return false; + return true; + }; + + let migrationRetryable = false; + const migrateLegacyNetworks = async (): Promise => { + const listed = await dexec([ + "network", + "inspect", + "--format", + "{{range .Containers}}{{println .Name}}{{end}}", + LEGACY_NETWORK, + ]); + migrationRetryable = listed.code === 0; + if (listed.code !== 0) return /no such network|not found/i.test(listed.stderr); + let migrated = true; + for (const container of listed.stdout + .split(/\s+/) + .filter((candidate) => /^agent-deploy-[a-zA-Z0-9_-]+$/.test(candidate))) { + if (!(await migrateContainer(container))) migrated = false; + } + if (!migrated) return false; + const removed = await dexec(["network", "rm", LEGACY_NETWORK]); + if (removed.code === 0 || /no such network|not found/i.test(removed.stderr)) return true; + const remaining = await dexec([ + "network", + "inspect", + "--format", + "{{range .Containers}}{{println .Name}}{{end}}", + LEGACY_NETWORK, + ]); + return ( + remaining.code === 0 && + !remaining.stdout.split(/\s+/).some((candidate) => /^agent-deploy-[a-zA-Z0-9_-]+$/.test(candidate)) + ); + }; + + let migrationComplete = false; + let migrationInFlight: Promise | undefined; + let migrationRetryTimer: ReturnType | undefined; + let migrationRetryDelayMs = 1000; + const runMigration = (): Promise => { + if (migrationComplete) return Promise.resolve(true); + if (migrationInFlight) return migrationInFlight; + migrationInFlight = migrateLegacyNetworks() + .then((complete) => { + migrationComplete = complete; + if (complete) { + if (migrationRetryTimer) clearTimeout(migrationRetryTimer); + migrationRetryTimer = undefined; + migrationRetryDelayMs = 1000; + } else if (!migrationRetryTimer) { + const delay = migrationRetryable ? migrationRetryDelayMs : 30_000; + migrationRetryDelayMs = Math.min(delay * 2, 30_000); + migrationRetryTimer = setTimeout(() => { + migrationRetryTimer = undefined; + void runMigration(); + }, delay); + migrationRetryTimer.unref(); + } + return complete; + }) + .finally(() => { + migrationInFlight = undefined; + }); + return migrationInFlight; + }; + const ensureMigration = async (): Promise => { + if ((await runMigration()) || (await runMigration())) return; + throw new Error("legacy Docker network migration incomplete"); + }; + void runMigration(); + return { profile: { managedScaleToZero: false }, async apply(d: Deployment, version: DeploymentVersion): Promise { - const net = await ensureNetwork(d); + await ensureMigration(); + const net = await ensureNetwork(network(d)); await dexec(["rm", "-f", name(d)]); const hostPort = allocPort(name(d)); const envArgs = Object.entries(version.env ?? {}).flatMap(([k, v]) => ["-e", `${k}=${v}`]); @@ -92,6 +183,7 @@ export function createDockerDeployProvider(opts: DockerDeployProviderOptions = { }, async logs(d: Deployment, opts: { tailLines: number }): Promise { + await ensureMigration(); const lines = Math.max(1, Math.min(2000, Math.floor(opts.tailLines))); const r = await dexec(["logs", "--tail", String(lines), name(d)]); if (r.code !== 0) return null; @@ -99,9 +191,15 @@ export function createDockerDeployProvider(opts: DockerDeployProviderOptions = { }, async destroy(d: Deployment): Promise { + await ensureMigration(); await dexec(["rm", "-f", name(d)]); await dexec(["network", "rm", network(d)]); freePort(name(d)); }, + + async resolveEndpoint(d): Promise { + await ensureMigration(); + return (await migrateContainer(name(d))) ? d.endpoint : null; + }, }; } diff --git a/src/directory/directory-store.ts b/src/directory/directory-store.ts index a3b20e0a..0587f340 100644 --- a/src/directory/directory-store.ts +++ b/src/directory/directory-store.ts @@ -57,7 +57,20 @@ export interface DirectoryStore { channelMembership(channelId: string, principalId: string): Promise; channelMemberIds(channelId: string): Promise; channelPrivacy(channelId: string): Promise; - replaceGroups(groupMembers: GroupMembership[], syncedAt?: number): Promise; + replaceGroups( + groupMembers: GroupMembership[], + syncedAt?: number, + groupIds?: string[], + groupRosterIds?: string[], + ): Promise; + replaceCapabilityChannels( + channelIds: string[], + channelMembers: ChannelMembership[], + channelRosterIds: string[], + syncedAt?: number, + revocations?: ChannelMembership[], + ): Promise; + channelCapabilityMembership(channelId: string, principalId: string): Promise; upsertGroup(groupId: string, principalIds: readonly string[]): Promise; resolveGroupByParticipants(participants: readonly string[]): Promise; groupMember(groupId: string, principalId: string): Promise; @@ -96,7 +109,12 @@ export function createDirectoryStore(): DirectoryStore { let channels: DirectoryChannel[] = []; let channelMembers: Map> | undefined; let knownChannelRosters: Set | undefined; + let capabilityChannelMembers: Map> | undefined; + let knownCapabilityChannelRosters: Set | undefined; + let capabilityChannelRevocations = new Map>(); let groupMembers: Map> | undefined; + let listedGroupIds: Set | undefined; + let knownGroupRosters: Set | undefined; let groupsSynced = false; let workspaceUrl: string | undefined; const syncedAts = new Map(); @@ -164,14 +182,66 @@ export function createDirectoryStore(): DirectoryStore { const channel = channels.find((candidate) => candidate.channelId === channelId); return channel ? channel.isPrivate === true : undefined; }, - async replaceGroups(nextGroupMembers, syncedAt) { + async replaceCapabilityChannels(channelIds, nextChannelMembers, nextChannelRosterIds, syncedAt, revocations = []) { + if (!acceptSync("capabilityChannels", syncedAt)) return false; + const listed = new Set(channelIds.filter(Boolean)); + const rosterIds = new Set(nextChannelRosterIds.filter((channelId) => listed.has(channelId))); + knownCapabilityChannelRosters = knownCapabilityChannelRosters + ? new Set([...knownCapabilityChannelRosters].filter((channelId) => listed.has(channelId))) + : new Set(); + capabilityChannelRevocations = new Map( + [...capabilityChannelRevocations].filter(([channelId]) => listed.has(channelId)), + ); + const byChannel = new Map( + [...(capabilityChannelMembers ?? [])].filter( + ([channelId]) => listed.has(channelId) && !rosterIds.has(channelId), + ), + ); + for (const channelId of rosterIds) byChannel.set(channelId, new Set()); + for (const member of nextChannelMembers) { + if (!member.principalId || !rosterIds.has(member.channelId)) continue; + (byChannel.get(member.channelId) ?? byChannel.set(member.channelId, new Set()).get(member.channelId)!).add( + member.principalId, + ); + } + capabilityChannelMembers = byChannel; + for (const channelId of rosterIds) { + knownCapabilityChannelRosters.add(channelId); + capabilityChannelRevocations.delete(channelId); + } + for (const member of revocations) { + if (!listed.has(member.channelId) || !member.principalId) continue; + const revoked = capabilityChannelRevocations.get(member.channelId) ?? new Set(); + revoked.add(member.principalId); + capabilityChannelRevocations.set(member.channelId, revoked); + capabilityChannelMembers.get(member.channelId)?.delete(member.principalId); + } + return true; + }, + async channelCapabilityMembership(channelId, principalId) { + if (capabilityChannelRevocations.get(channelId)?.has(principalId)) return false; + if (!knownCapabilityChannelRosters?.has(channelId)) return undefined; + return capabilityChannelMembers?.get(channelId)?.has(principalId) ?? false; + }, + async replaceGroups(nextGroupMembers, syncedAt, nextGroupIds, nextGroupRosterIds) { if (!acceptSync("groups", syncedAt)) return false; - const byGroup = new Map>(); + const legacy = nextGroupIds === undefined || nextGroupRosterIds === undefined; + const listed = new Set((nextGroupIds ?? nextGroupMembers.map((member) => member.groupId)).filter(Boolean)); + const rosterIds = new Set((nextGroupRosterIds ?? [...listed]).filter((groupId) => listed.has(groupId))); + const byGroup = new Map( + [...(groupMembers ?? [])].filter(([groupId]) => listed.has(groupId) && !rosterIds.has(groupId)), + ); + for (const groupId of rosterIds) byGroup.set(groupId, new Set()); for (const m of nextGroupMembers) { - if (!m.groupId || !m.principalId) continue; + if (!m.groupId || !m.principalId || !rosterIds.has(m.groupId)) continue; (byGroup.get(m.groupId) ?? byGroup.set(m.groupId, new Set()).get(m.groupId)!).add(m.principalId); } groupMembers = byGroup; + listedGroupIds = listed; + knownGroupRosters = legacy + ? new Set(rosterIds) + : new Set([...(knownGroupRosters ?? [])].filter((groupId) => listed.has(groupId))); + for (const groupId of rosterIds) knownGroupRosters.add(groupId); groupsSynced = true; return true; }, @@ -181,6 +251,10 @@ export function createDirectoryStore(): DirectoryStore { const byGroup = groupMembers ?? new Map>(); byGroup.set(groupId, new Set(ids)); groupMembers = byGroup; + listedGroupIds ??= new Set(); + listedGroupIds.add(groupId); + knownGroupRosters ??= new Set(); + knownGroupRosters.add(groupId); const stored = syncedAts.get("groups"); syncedAts.set("groups", Math.max(stored ?? 0, Date.now())); }, @@ -199,9 +273,10 @@ export function createDirectoryStore(): DirectoryStore { return groupMembers?.get(groupId)?.has(principalId) ?? false; }, async groupMembership(groupId, principalId) { - const memberships = groupMembers; - if (!memberships || !groupsSynced) return undefined; - return memberships.get(groupId)?.has(principalId) ?? false; + if (!groupsSynced) return undefined; + if (!listedGroupIds?.has(groupId)) return false; + if (!knownGroupRosters?.has(groupId)) return undefined; + return groupMembers?.get(groupId)?.has(principalId) ?? false; }, async listGroupsFor(principalId) { const memberships = groupMembers; diff --git a/src/directory/postgres-directory-store.ts b/src/directory/postgres-directory-store.ts index 91b345be..6b047753 100644 --- a/src/directory/postgres-directory-store.ts +++ b/src/directory/postgres-directory-store.ts @@ -55,12 +55,38 @@ const SCHEMA = [ principal_id TEXT NOT NULL, PRIMARY KEY (org_id, channel_id, principal_id) )`, + `CREATE TABLE IF NOT EXISTS directory_capability_channels( + org_id TEXT NOT NULL, + channel_id TEXT NOT NULL, + PRIMARY KEY (org_id, channel_id) + )`, + `CREATE TABLE IF NOT EXISTS directory_capability_channel_members( + org_id TEXT NOT NULL, + channel_id TEXT NOT NULL, + principal_id TEXT NOT NULL, + PRIMARY KEY (org_id, channel_id, principal_id) + )`, + `CREATE TABLE IF NOT EXISTS directory_capability_channel_revocations( + org_id TEXT NOT NULL, + channel_id TEXT NOT NULL, + principal_id TEXT NOT NULL, + PRIMARY KEY (org_id, channel_id, principal_id) + )`, `CREATE TABLE IF NOT EXISTS directory_group_members( org_id TEXT NOT NULL, group_id TEXT NOT NULL, principal_id TEXT NOT NULL, PRIMARY KEY (org_id, group_id, principal_id) )`, + `CREATE TABLE IF NOT EXISTS directory_groups( + org_id TEXT NOT NULL, + group_id TEXT NOT NULL, + roster_known BOOLEAN NOT NULL DEFAULT FALSE, + PRIMARY KEY (org_id, group_id) + )`, + `INSERT INTO directory_groups (org_id, group_id, roster_known) + SELECT DISTINCT org_id, group_id, TRUE FROM directory_group_members + ON CONFLICT (org_id, group_id) DO NOTHING`, `CREATE INDEX IF NOT EXISTS directory_group_members_principal ON directory_group_members (org_id, principal_id, group_id)`, `CREATE TABLE IF NOT EXISTS directory_sync( @@ -89,6 +115,8 @@ const SCHEMA = [ `ALTER TABLE directory_sync ADD COLUMN IF NOT EXISTS members_synced_at BIGINT`, `ALTER TABLE directory_sync ADD COLUMN IF NOT EXISTS channels_synced_at BIGINT`, `ALTER TABLE directory_sync ADD COLUMN IF NOT EXISTS groups_synced_at BIGINT`, + `ALTER TABLE directory_sync ADD COLUMN IF NOT EXISTS capability_channels_hash TEXT`, + `ALTER TABLE directory_sync ADD COLUMN IF NOT EXISTS capability_channels_synced_at BIGINT`, `DO $$ BEGIN IF NOT EXISTS ( @@ -391,16 +419,132 @@ export function createPostgresDirectoryStore(connectionString: string): Director return rows.length > 0 ? (rows[0]!.is_private as boolean) : undefined; }, - async replaceGroups(groupMembers, syncedAt) { - const rows = dedupPairs( + async replaceCapabilityChannels(channelIds, channelMembers, channelRosterIds, syncedAt, revocations = []) { + const listedIds = [...new Set(channelIds.filter(Boolean))]; + const listed = new Set(listedIds); + const rosterIds = [...new Set(channelRosterIds.filter((channelId) => listed.has(channelId)))]; + const rostered = new Set(rosterIds); + const membershipRows = dedupMemberships(channelMembers).filter((member) => rostered.has(member.channelId)); + const revokedRows = dedupMemberships(revocations).filter((member) => listed.has(member.channelId)); + const hash = hashRoster([ + ...listedIds.map((channelId) => `c:${channelId}`), + ...rosterIds.map((channelId) => `r:${channelId}`), + ...membershipRows.map((member) => `m:${member.channelId}|${member.principalId}`), + ...revokedRows.map((member) => `x:${member.channelId}|${member.principalId}`), + ]); + return swapIfChanged("capability_channels_hash", hash, syncedAt, async (client) => { + await client.query( + "DELETE FROM directory_capability_channel_members WHERE org_id = $1 AND NOT (channel_id = ANY($2::text[]))", + [orgId, listedIds], + ); + await client.query( + "DELETE FROM directory_capability_channels WHERE org_id = $1 AND NOT (channel_id = ANY($2::text[]))", + [orgId, listedIds], + ); + await client.query( + "DELETE FROM directory_capability_channel_revocations WHERE org_id = $1 AND NOT (channel_id = ANY($2::text[]))", + [orgId, listedIds], + ); + await client.query( + "DELETE FROM directory_capability_channel_members WHERE org_id = $1 AND channel_id = ANY($2::text[])", + [orgId, rosterIds], + ); + await client.query( + "DELETE FROM directory_capability_channel_revocations WHERE org_id = $1 AND channel_id = ANY($2::text[])", + [orgId, rosterIds], + ); + if (rosterIds.length) { + await client.query( + `INSERT INTO directory_capability_channels (org_id, channel_id) + SELECT $1, * FROM unnest($2::text[]) + ON CONFLICT (org_id, channel_id) DO NOTHING`, + [orgId, rosterIds], + ); + } + if (membershipRows.length) { + await client.query( + `INSERT INTO directory_capability_channel_members (org_id, channel_id, principal_id) + SELECT $1, * FROM unnest($2::text[], $3::text[])`, + [orgId, membershipRows.map((m) => m.channelId), membershipRows.map((m) => m.principalId)], + ); + } + if (revokedRows.length) { + await client.query( + `DELETE FROM directory_capability_channel_members member + USING unnest($2::text[], $3::text[]) AS revoked(channel_id, principal_id) + WHERE member.org_id = $1 AND member.channel_id = revoked.channel_id + AND member.principal_id = revoked.principal_id`, + [orgId, revokedRows.map((m) => m.channelId), revokedRows.map((m) => m.principalId)], + ); + await client.query( + `INSERT INTO directory_capability_channel_revocations (org_id, channel_id, principal_id) + SELECT $1, * FROM unnest($2::text[], $3::text[]) + ON CONFLICT (org_id, channel_id, principal_id) DO NOTHING`, + [orgId, revokedRows.map((m) => m.channelId), revokedRows.map((m) => m.principalId)], + ); + } + }); + }, + + async channelCapabilityMembership(channelId, principalId) { + const rows = await q( + `SELECT EXISTS ( + SELECT 1 FROM directory_capability_channels + WHERE org_id = $1 AND channel_id = $2 + ) AS known, + EXISTS ( + SELECT 1 FROM directory_capability_channel_members + WHERE org_id = $1 AND channel_id = $2 AND principal_id = $3 + ) AS member, + EXISTS ( + SELECT 1 FROM directory_capability_channel_revocations + WHERE org_id = $1 AND channel_id = $2 AND principal_id = $3 + ) AS revoked`, + [orgId, channelId, principalId], + ); + if (rows[0]?.revoked === true) return false; + return rows[0]?.known === true ? rows[0]?.member === true : undefined; + }, + + async replaceGroups(groupMembers, syncedAt, groupIds, groupRosterIds) { + const allRows = dedupPairs( groupMembers, (m) => m.groupId, (m) => m.principalId, ); - const hash = hashRoster(rows.map((m) => `${m.groupId}|${m.principalId}`)); + const listedIds = [...new Set((groupIds ?? allRows.map((member) => member.groupId)).filter(Boolean))]; + const listed = new Set(listedIds); + const rosterIds = [...new Set((groupRosterIds ?? listedIds).filter((groupId) => listed.has(groupId)))]; + const rostered = new Set(rosterIds); + const rows = allRows.filter((member) => rostered.has(member.groupId)); + const hash = hashRoster([ + ...listedIds.map((groupId) => `g:${groupId}`), + ...rosterIds.map((groupId) => `r:${groupId}`), + ...rows.map((member) => `m:${member.groupId}|${member.principalId}`), + ]); return swapIfChanged("groups_hash", hash, syncedAt, async (client) => { - await client.query("DELETE FROM directory_group_members WHERE org_id = $1", [orgId]); + await client.query( + "DELETE FROM directory_group_members WHERE org_id = $1 AND NOT (group_id = ANY($2::text[]))", + [orgId, listedIds], + ); + await client.query("DELETE FROM directory_groups WHERE org_id = $1 AND NOT (group_id = ANY($2::text[]))", [ + orgId, + listedIds, + ]); + if (listedIds.length) { + await client.query( + `INSERT INTO directory_groups (org_id, group_id, roster_known) + SELECT $1, * FROM unnest($2::text[], $3::boolean[]) + ON CONFLICT (org_id, group_id) DO UPDATE SET + roster_known = directory_groups.roster_known OR EXCLUDED.roster_known`, + [orgId, listedIds, listedIds.map((groupId) => rostered.has(groupId))], + ); + } + await client.query("DELETE FROM directory_group_members WHERE org_id = $1 AND group_id = ANY($2::text[])", [ + orgId, + rosterIds, + ]); if (rows.length) { await client.query( `INSERT INTO directory_group_members (org_id, group_id, principal_id) @@ -416,6 +560,11 @@ export function createPostgresDirectoryStore(connectionString: string): Director if (!groupId || !ids.length) return; await withPgTransaction(await pool(), async (client) => { await client.query("SELECT pg_advisory_xact_lock(hashtext('directory'), hashtext($1))", [orgId]); + await client.query( + `INSERT INTO directory_groups (org_id, group_id, roster_known) VALUES ($1, $2, TRUE) + ON CONFLICT (org_id, group_id) DO UPDATE SET roster_known = TRUE`, + [orgId, groupId], + ); await client.query("DELETE FROM directory_group_members WHERE org_id = $1 AND group_id = $2", [orgId, groupId]); await client.query( `INSERT INTO directory_group_members (org_id, group_id, principal_id) @@ -463,12 +612,20 @@ export function createPostgresDirectoryStore(connectionString: string): Director SELECT 1 FROM directory_group_members WHERE org_id = $1 AND group_id = $2 AND principal_id = $3 ) AS member, + EXISTS ( + SELECT 1 FROM directory_groups WHERE org_id = $1 AND group_id = $2 + ) AS listed, + COALESCE(( + SELECT roster_known FROM directory_groups WHERE org_id = $1 AND group_id = $2 + ), FALSE) AS roster_known, EXISTS ( SELECT 1 FROM directory_sync WHERE org_id = $1 AND groups_hash IS NOT NULL - ) AS known`, + ) AS synced`, [orgId, groupId, principalId], ); - return rows[0]?.known === true ? rows[0]?.member === true : undefined; + if (rows[0]?.member === true) return true; + if (rows[0]?.listed !== true) return rows[0]?.synced === true ? false : undefined; + return rows[0]?.roster_known === true ? rows[0]?.member === true : undefined; }, async listGroupsFor(principalId) { diff --git a/src/slack/directory.ts b/src/slack/directory.ts index 333ebb1b..36e966d7 100644 --- a/src/slack/directory.ts +++ b/src/slack/directory.ts @@ -43,6 +43,7 @@ interface ChannelMembershipRow { channelId: string; principalId: string; } +type ChannelInvalidations = ReadonlyMap>; interface GroupMembershipRow { groupId: string; principalId: string; @@ -59,7 +60,7 @@ const MEMBERS_PAGE_LIMIT = 200; export interface Directory { getUserSnapshot(client: any): Promise<{ byId: Map; fetchedAt: number } | undefined>; - forceDirectorySync(client: any): Promise; + forceDirectorySync(client: any, invalidateChannelId?: string, invalidatePrincipalId?: string): Promise; classifyUserCached(client: any, userId: string): Promise; classifyActor(client: any, userId: string): Promise; getChannelInfo(client: any, channel: string): Promise; @@ -228,10 +229,21 @@ export function createDirectory(deps: { client: any, publicChannels: ChannelRef[], privateChannels: ChannelRef[], - ): Promise<{ channels: ChannelRow[]; channelMembers: ChannelMembershipRow[]; channelRosterIds: string[] }> { + invalidations: ChannelInvalidations, + ): Promise<{ + channels: ChannelRow[]; + channelMembers: ChannelMembershipRow[]; + channelRosterIds: string[]; + capabilityChannelMembers: ChannelMembershipRow[]; + capabilityChannelRosterIds: string[]; + capabilityChannelRevocations: ChannelMembershipRow[]; + }> { const channels: ChannelRow[] = []; const channelMembers: ChannelMembershipRow[] = []; const channelRosterIds: string[] = []; + const capabilityChannelMembers: ChannelMembershipRow[] = []; + const capabilityChannelRosterIds: string[] = []; + const capabilityChannelRevocations: ChannelMembershipRow[] = []; const publicRosters = await allInternalRosters(client, publicChannels, { plural: "public channels", authz: "public-channel-capability", @@ -241,25 +253,57 @@ export function createDirectory(deps: { }); for (const channel of publicChannels) { const internalIds = publicRosters.get(channel.id); - if (!internalIds) continue; + if (!internalIds) { + for (const principalId of invalidations.get(channel.id) ?? []) { + capabilityChannelRevocations.push({ channelId: channel.id, principalId }); + } + continue; + } channelRosterIds.push(channel.id); + capabilityChannelRosterIds.push(channel.id); for (const pid of internalIds) { channelMembers.push({ channelId: channel.id, principalId: pid }); + capabilityChannelMembers.push({ channelId: channel.id, principalId: pid }); } } const rosters = await allInternalRosters(client, privateChannels, { plural: "private channels", authz: "private-channel-send", item: "private channel", + limit: privateChannels.length, + }); + const capabilityRosters = await allInternalRosters(client, privateChannels, { + plural: "private channels", + authz: "private-channel-capability", + item: "private channel", + limit: privateChannels.length, + allowExternal: true, }); for (const c of privateChannels) { channels.push({ channelId: c.id, name: c.name, isPrivate: true }); const internalIds = rosters.get(c.id); - if (!internalIds) continue; - channelRosterIds.push(c.id); - for (const pid of internalIds) channelMembers.push({ channelId: c.id, principalId: pid }); + if (internalIds) { + channelRosterIds.push(c.id); + for (const pid of internalIds) channelMembers.push({ channelId: c.id, principalId: pid }); + } + const capabilityIds = capabilityRosters.get(c.id); + if (!capabilityIds) { + for (const principalId of invalidations.get(c.id) ?? []) { + capabilityChannelRevocations.push({ channelId: c.id, principalId }); + } + continue; + } + capabilityChannelRosterIds.push(c.id); + for (const pid of capabilityIds) capabilityChannelMembers.push({ channelId: c.id, principalId: pid }); } - return { channels, channelMembers, channelRosterIds }; + return { + channels, + channelMembers, + channelRosterIds, + capabilityChannelMembers, + capabilityChannelRosterIds, + capabilityChannelRevocations, + }; } async function listBotGroupDms(client: any): Promise { @@ -276,19 +320,22 @@ export function createDirectory(deps: { return groupIds; } - async function computeGroupMembership(client: any, groupIds: string[]): Promise { + async function computeGroupMembership( + client: any, + groupIds: string[], + ): Promise<{ groupMembers: GroupMembershipRow[]; groupRosterIds: string[] }> { const groupMembers: GroupMembershipRow[] = []; const rosters = await allInternalRosters( client, groupIds.map((id) => ({ id })), - { plural: "group DMs", authz: "group-DM-send", item: "group DM" }, + { plural: "group DMs", authz: "group-DM-send", item: "group DM", limit: groupIds.length }, ); for (const id of groupIds) { const internalIds = rosters.get(id); if (!internalIds) continue; for (const pid of internalIds) groupMembers.push({ groupId: id, principalId: pid }); } - return groupMembers; + return { groupMembers, groupRosterIds: [...rosters.keys()] }; } let privateChannelsCache: @@ -296,18 +343,31 @@ export function createDirectory(deps: { channels: ChannelRow[]; channelMembers: ChannelMembershipRow[]; channelRosterIds: string[]; + capabilityChannelMembers: ChannelMembershipRow[]; + capabilityChannelRosterIds: string[]; + capabilityChannelRevocations: ChannelMembershipRow[]; groupMembers?: GroupMembershipRow[]; + groupIds?: string[]; + groupRosterIds?: string[]; fetchedAt: number; groupsFetchedAt?: number; } | undefined; const seenGroupIds = new Set(); - async function fetchChannels(client: any): Promise<{ + async function fetchChannels( + client: any, + invalidations: ChannelInvalidations, + ): Promise<{ channels: ChannelRow[]; channelMembers: ChannelMembershipRow[]; channelRosterIds: string[]; + capabilityChannelMembers: ChannelMembershipRow[]; + capabilityChannelRosterIds: string[]; + capabilityChannelRevocations: ChannelMembershipRow[]; groupMembers?: GroupMembershipRow[]; + groupIds?: string[]; + groupRosterIds?: string[]; fetchedAt: number; groupsFetchedAt?: number; } | null> { @@ -319,23 +379,51 @@ export function createDirectory(deps: { return null; } const fresh = privateChannelsCache && Date.now() - privateChannelsCache.fetchedAt < CHANNEL_MEMBERS_TTL_MS; + let includeGroups = true; if (!fresh) { - const computed = await computeChannelMembership(client, listed.publicChannels, listed.privateChannels); + const computed = await computeChannelMembership( + client, + listed.publicChannels, + listed.privateChannels, + invalidations, + ); let groupMembers: GroupMembershipRow[] | undefined; + let groupIds: string[] | undefined; + let groupRosterIds: string[] | undefined; let groupsFetchedAt: number | undefined; try { - const groupIds = await listBotGroupDms(client); + groupIds = await listBotGroupDms(client); for (const id of groupIds) seenGroupIds.add(id); - groupMembers = await computeGroupMembership(client, groupIds); + const computedGroups = await computeGroupMembership(client, groupIds); + groupMembers = computedGroups.groupMembers; + groupRosterIds = computedGroups.groupRosterIds; groupsFetchedAt = Date.now(); } catch (err) { console.error("[slack-plugin] group-DM list failed:", (err as Error).message); + includeGroups = false; groupMembers = privateChannelsCache?.groupMembers; + groupIds = privateChannelsCache?.groupIds; + groupRosterIds = privateChannelsCache?.groupRosterIds; groupsFetchedAt = privateChannelsCache?.groupsFetchedAt; } - privateChannelsCache = { ...computed, groupMembers, groupsFetchedAt, fetchedAt: Date.now() }; + privateChannelsCache = { + ...computed, + groupMembers, + groupIds, + groupRosterIds, + groupsFetchedAt, + fetchedAt: Date.now(), + }; } - const priv = privateChannelsCache ?? { channels: [], channelMembers: [], channelRosterIds: [], fetchedAt: 0 }; + const priv = privateChannelsCache ?? { + channels: [], + channelMembers: [], + channelRosterIds: [], + capabilityChannelMembers: [], + capabilityChannelRosterIds: [], + capabilityChannelRevocations: [], + fetchedAt: 0, + }; return { channels: [ ...listed.publicChannels.map((channel) => ({ channelId: channel.id, name: channel.name })), @@ -343,12 +431,26 @@ export function createDirectory(deps: { ], channelMembers: priv.channelMembers, channelRosterIds: priv.channelRosterIds, + capabilityChannelMembers: priv.capabilityChannelMembers, + capabilityChannelRosterIds: priv.capabilityChannelRosterIds, + capabilityChannelRevocations: priv.capabilityChannelRevocations, fetchedAt: priv.fetchedAt, - ...(priv.groupMembers ? { groupMembers: priv.groupMembers, groupsFetchedAt: priv.groupsFetchedAt } : {}), + ...(includeGroups && priv.groupMembers + ? { + groupMembers: priv.groupMembers, + groupIds: priv.groupIds, + groupRosterIds: priv.groupRosterIds, + groupsFetchedAt: priv.groupsFetchedAt, + } + : {}), }; } - async function pushDirectory(snap: UserSnapshot, client: any): Promise { + async function pushDirectory( + snap: UserSnapshot, + client: any, + invalidations: ChannelInvalidations = new Map(), + ): Promise { const members = [...snap.byId.entries()] .filter(([, u]) => !u.actor.isExternalGuest && !u.actor.isBot) .map(([slackId, u]) => { @@ -360,8 +462,8 @@ export function createDirectory(deps: { ...(slackId && slackId !== a.externalId ? { slackId } : {}), }; }); - const fetched = await fetchChannels(client); - if (!members.length && !(fetched && fetched.channels.length)) return; + const fetched = await fetchChannels(client, invalidations); + if (!members.length && !(fetched && fetched.channels.length)) return false; try { await core.pushDirectory({ members, @@ -371,16 +473,26 @@ export function createDirectory(deps: { channels: fetched.channels, channelMembers: fetched.channelMembers, channelRosterIds: fetched.channelRosterIds, + capabilityChannelMembers: fetched.capabilityChannelMembers, + capabilityChannelRosterIds: fetched.capabilityChannelRosterIds, + capabilityChannelRevocations: fetched.capabilityChannelRevocations, channelsSyncedAt: fetched.fetchedAt, ...(fetched.groupMembers - ? { groupMembers: fetched.groupMembers, groupsSyncedAt: fetched.groupsFetchedAt } + ? { + groupMembers: fetched.groupMembers, + groupIds: fetched.groupIds, + groupRosterIds: fetched.groupRosterIds, + groupsSyncedAt: fetched.groupsFetchedAt, + } : {}), } : {}), ...(ids.ownWorkspaceUrl ? { workspaceUrl: ids.ownWorkspaceUrl } : {}), }); + return fetched !== null; } catch (err) { console.error("[slack-plugin] directory push failed:", (err as Error).message); + return false; } } @@ -409,14 +521,33 @@ export function createDirectory(deps: { } let directorySyncClient: any; + const invalidatedChannelMembers = new Map>(); const coalescedDirectorySync = createRefreshCoalescer(async () => { - privateChannelsCache = undefined; + if (privateChannelsCache) privateChannelsCache.fetchedAt = 0; const snap = userSnapshot ?? (await getUserSnapshot(directorySyncClient)); - if (snap) await pushDirectory(snap, directorySyncClient); + const pendingInvalidations = new Map( + [...invalidatedChannelMembers].map(([channelId, principalIds]) => [channelId, new Set(principalIds)]), + ); + if (snap && (await pushDirectory(snap, directorySyncClient, pendingInvalidations))) { + for (const [channelId, principalIds] of pendingInvalidations) { + const current = invalidatedChannelMembers.get(channelId); + for (const principalId of principalIds) current?.delete(principalId); + if (!current?.size) invalidatedChannelMembers.delete(channelId); + } + } }); - function forceDirectorySync(client: any): Promise { + function forceDirectorySync( + client: any, + invalidateChannelId?: string, + invalidatePrincipalId?: string, + ): Promise { directorySyncClient = client; + if (invalidateChannelId && invalidatePrincipalId) { + const principals = invalidatedChannelMembers.get(invalidateChannelId) ?? new Set(); + principals.add(invalidatePrincipalId); + invalidatedChannelMembers.set(invalidateChannelId, principals); + } return coalescedDirectorySync(); } diff --git a/src/slack/events.ts b/src/slack/events.ts index 98f287b5..1228c063 100644 --- a/src/slack/events.ts +++ b/src/slack/events.ts @@ -211,14 +211,14 @@ export function registerSlackEvents( } app.event("member_left_channel", async ({ event, body, client }: any) => { - const e = event as { channel?: string; event_ts?: string }; + const e = event as { channel?: string; user?: string; event_ts?: string }; if ( deduper.seen( dedupeKey({ event_id: (body as { event_id?: string })?.event_id, channel: e.channel, ts: e.event_ts }), ) ) return; - await forceDirectorySync(client); + await forceDirectorySync(client, e.channel, e.user); }); app.event("reaction_added", async ({ event, body, client }: any) => { diff --git a/test/capability-routes.test.ts b/test/capability-routes.test.ts index 0664311f..f9bc347c 100644 --- a/test/capability-routes.test.ts +++ b/test/capability-routes.test.ts @@ -68,6 +68,11 @@ describe("capability-token control plane (crons + SOUL)", () => { [{ channelId: "C", name: "eng", isPrivate: false }], ["admin-alice", "U1", "U2", "U8"].map((principalId) => ({ channelId: "C", principalId })), ); + await built.directory.replaceCapabilityChannels( + ["C"], + ["admin-alice", "U1", "U2", "U8"].map((principalId) => ({ channelId: "C", principalId })), + ["C"], + ); server = createServer(built.app, { signingSecret: SECRET, scheduler: built.scheduler, @@ -741,4 +746,13 @@ describe("capability-token control plane (crons + SOUL)", () => { "surfaced read-only in visible", ); }); + + it("a dedicated capability roster revokes a member even when a legacy channel roster still contains them", async () => { + await built.directory.replaceCapabilityChannels( + ["C"], + ["admin-alice", "U1", "U2"].map((principalId) => ({ channelId: "C", principalId })), + ["C"], + ); + assert.equal((await get("/v1/soul", { "x-agent-capability": await capChannel("U8") })).status, 403); + }); }); diff --git a/test/directory-store.test.ts b/test/directory-store.test.ts index fc91f699..81b7e35b 100644 --- a/test/directory-store.test.ts +++ b/test/directory-store.test.ts @@ -188,6 +188,29 @@ describe("group-DM (mpim) membership (addressed by participant set, §10)", () = assert.equal(await d.groupMember("G-new", "U-alice"), false); }); + it("partially replaces only group rosters known by the source", async () => { + const d = createDirectoryStore(); + await d.replaceGroups( + [ + { groupId: "G-one", principalId: "U-old-one" }, + { groupId: "G-two", principalId: "U-old-two" }, + ], + undefined, + ["G-one", "G-two"], + ["G-one", "G-two"], + ); + await d.replaceGroups( + [{ groupId: "G-two", principalId: "U-new-two" }], + undefined, + ["G-one", "G-two", "G-new"], + ["G-two"], + ); + assert.equal(await d.groupMembership("G-one", "U-old-one"), true); + assert.equal(await d.groupMembership("G-two", "U-old-two"), false); + assert.equal(await d.groupMembership("G-two", "U-new-two"), true); + assert.equal(await d.groupMembership("G-new", "U-new"), undefined); + }); + it("members and channels swaps are stale-guarded the same way", async () => { const d = createDirectoryStore(); assert.equal(await d.replace([{ principalId: "U-new", displayName: "New", type: "internal" }], 2000), true); @@ -254,4 +277,49 @@ describe("private-channel membership (authorizes private-channel sends, §10)", assert.equal(await d.channelMembership("C-two", "U-new-two"), true); assert.equal(await d.channelMembership("C-new", "U-new"), undefined); }); + + it("keeps capability rosters separate from legacy channel-directory swaps", async () => { + const d = createDirectoryStore(); + await d.replaceCapabilityChannels( + ["C-one", "C-two", "C-new"], + [ + { channelId: "C-one", principalId: "U-old-one" }, + { channelId: "C-one", principalId: "U-keep" }, + { channelId: "C-two", principalId: "U-old-two" }, + ], + ["C-one", "C-two"], + ); + await d.replaceCapabilityChannels( + ["C-one", "C-two", "C-new"], + [{ channelId: "C-two", principalId: "U-new-two" }], + ["C-two"], + ); + await d.replaceChannels( + [ + { channelId: "C-one", name: "one" }, + { channelId: "C-two", name: "two" }, + { channelId: "C-new", name: "new" }, + ], + [], + ); + assert.equal(await d.channelCapabilityMembership("C-one", "U-old-one"), true); + assert.equal(await d.channelCapabilityMembership("C-two", "U-old-two"), false); + assert.equal(await d.channelCapabilityMembership("C-two", "U-new-two"), true); + assert.equal(await d.channelCapabilityMembership("C-new", "U-new"), undefined); + await d.replaceCapabilityChannels(["C-one", "C-two", "C-new"], [], [], undefined, [ + { channelId: "C-one", principalId: "U-old-one" }, + ]); + assert.equal(await d.channelCapabilityMembership("C-one", "U-old-one"), false); + assert.equal(await d.channelCapabilityMembership("C-one", "U-keep"), true); + await d.replaceCapabilityChannels(["C-one", "C-two", "C-new"], [], [], undefined, [ + { channelId: "C-new", principalId: "U-new" }, + ]); + assert.equal(await d.channelCapabilityMembership("C-new", "U-new"), false); + await d.replaceCapabilityChannels( + ["C-one", "C-two", "C-new"], + [{ channelId: "C-new", principalId: "U-new" }], + ["C-new"], + ); + assert.equal(await d.channelCapabilityMembership("C-new", "U-new"), true); + }); }); diff --git a/test/docker-deploy-provider.test.ts b/test/docker-deploy-provider.test.ts index 459d38f2..c72a64f0 100644 --- a/test/docker-deploy-provider.test.ts +++ b/test/docker-deploy-provider.test.ts @@ -9,7 +9,11 @@ test("Docker deployments use isolated networks and remove them on destroy", asyn const calls: string[][] = []; const dockerExec: DockerExec = async (args) => { calls.push(args); - return { code: args[1] === "inspect" ? 1 : 0, stdout: "", stderr: "" }; + return { + code: args[1] === "inspect" ? 1 : 0, + stdout: "", + stderr: args[1] === "inspect" ? "No such network" : "", + }; }; const store = createDeployStore(); const first = await store.create({ @@ -38,3 +42,80 @@ test("Docker deployments use isolated networks and remove them on destroy", asyn assert.ok(calls.some((args) => args.join(" ").includes(`--name ${secondName} --network ${secondName}-net`))); assert.ok(calls.some((args) => args.join(" ") === `network rm ${firstName}-net`)); }); + +test("Docker provider migrates running deployments off the legacy shared network", async () => { + const calls: string[][] = []; + let containerName = ""; + let connectAttempts = 0; + let targetAttached = false; + let legacyAttached = true; + const dockerExec: DockerExec = async (args) => { + calls.push(args); + if (args.join(" ") === "network inspect --format {{range .Containers}}{{println .Name}}{{end}} agent-deploynet") { + return { code: 0, stdout: legacyAttached ? `${containerName}\n` : "", stderr: "" }; + } + if (args[0] === "network" && args[1] === "inspect") return { code: 1, stdout: "", stderr: "missing" }; + if (args[0] === "network" && args[1] === "connect" && ++connectAttempts === 1) { + return { code: 1, stdout: "", stderr: "transient" }; + } + if (args[0] === "network" && args[1] === "connect") targetAttached = true; + if (args[0] === "network" && args[1] === "disconnect") legacyAttached = false; + if (args[0] === "inspect") { + return { + code: 0, + stdout: JSON.stringify({ + ...(legacyAttached ? { "agent-deploynet": {} } : {}), + ...(targetAttached ? { [`${containerName}-net`]: {} } : {}), + }), + stderr: "", + }; + } + return { code: 0, stdout: "", stderr: "" }; + }; + const store = createDeployStore(); + const deployment = await store.create({ + ownerScopeId: scopeId("personal", "U1"), + createdBy: "U1", + entrypoint: "node server.js", + snapshotDir: "/snap/legacy", + }); + containerName = `agent-deploy-${deployment.id.slice(0, 12)}`; + await store.setEndpoint(deployment.id, { host: "127.0.0.1", port: 9200 }); + const running = (await store.get(deployment.id))!; + const provider = createDockerDeployProvider({ dockerExec }); + + assert.deepEqual(await provider.resolveEndpoint!(running, running.versions[0]!), running.endpoint); + assert.equal(connectAttempts, 2); + assert.ok(calls.some((args) => args.join(" ") === `network connect ${containerName}-net ${containerName}`)); + assert.ok(calls.some((args) => args.join(" ") === `network disconnect agent-deploynet ${containerName}`)); +}); + +test("Docker provider retries legacy migration after the daemon recovers", async (t) => { + t.mock.timers.enable({ apis: ["setTimeout"] }); + const calls: string[][] = []; + const container = "agent-deploy-legacy123"; + let legacyInspections = 0; + const dockerExec: DockerExec = async (args) => { + calls.push(args); + if (args.join(" ") === "network inspect --format {{range .Containers}}{{println .Name}}{{end}} agent-deploynet") { + legacyInspections++; + return legacyInspections === 1 + ? { code: 1, stdout: "", stderr: "daemon unavailable" } + : { code: 0, stdout: `${container}\n`, stderr: "" }; + } + if (args[0] === "inspect") { + return { code: 0, stdout: JSON.stringify({ "agent-deploynet": {} }), stderr: "" }; + } + if (args[0] === "network" && args[1] === "inspect") return { code: 1, stdout: "", stderr: "missing" }; + return { code: 0, stdout: "", stderr: "" }; + }; + + createDockerDeployProvider({ dockerExec }); + await new Promise((resolve) => setImmediate(resolve)); + assert.equal(legacyInspections, 1); + t.mock.timers.tick(30_000); + await new Promise((resolve) => setImmediate(resolve)); + await new Promise((resolve) => setImmediate(resolve)); + assert.equal(legacyInspections, 2); + assert.ok(calls.some((args) => args.join(" ") === `network disconnect agent-deploynet ${container}`)); +}); diff --git a/test/postgres-directory-store.test.ts b/test/postgres-directory-store.test.ts index 18c2cf54..de5f4356 100644 --- a/test/postgres-directory-store.test.ts +++ b/test/postgres-directory-store.test.ts @@ -10,7 +10,7 @@ before(async () => { const pg = (await import("pg")).default; const p = new pg.Pool({ connectionString: URL }); await p.query( - "DROP TABLE IF EXISTS directory_members, directory_channels, directory_channel_members, directory_group_members, directory_sync, directory_meta CASCADE", + "DROP TABLE IF EXISTS directory_members, directory_channels, directory_channel_members, directory_capability_channels, directory_capability_channel_members, directory_capability_channel_revocations, directory_groups, directory_group_members, directory_sync, directory_meta CASCADE", ); await p.end(); }); @@ -383,6 +383,74 @@ test("pg directory: a partial roster swap preserves channels whose roster is unk assert.equal(await store.channelMembership("C-new", "U-new"), undefined); }); +test("pg directory: capability rosters survive legacy channel-directory swaps", { skip }, async () => { + const store = createPostgresDirectoryStore(URL!); + await store.replaceCapabilityChannels( + ["C-one", "C-two", "C-new"], + [ + { channelId: "C-one", principalId: "U-old-one" }, + { channelId: "C-one", principalId: "U-keep" }, + { channelId: "C-two", principalId: "U-old-two" }, + ], + ["C-one", "C-two"], + ); + await store.replaceCapabilityChannels( + ["C-one", "C-two", "C-new"], + [{ channelId: "C-two", principalId: "U-new-two" }], + ["C-two"], + ); + await store.replaceChannels( + [ + { channelId: "C-one", name: "one" }, + { channelId: "C-two", name: "two" }, + { channelId: "C-new", name: "new" }, + ], + [], + ); + assert.equal(await store.channelCapabilityMembership("C-one", "U-old-one"), true); + assert.equal(await store.channelCapabilityMembership("C-two", "U-old-two"), false); + assert.equal(await store.channelCapabilityMembership("C-two", "U-new-two"), true); + assert.equal(await store.channelCapabilityMembership("C-new", "U-new"), undefined); + await store.replaceCapabilityChannels(["C-one", "C-two", "C-new"], [], [], undefined, [ + { channelId: "C-one", principalId: "U-old-one" }, + ]); + assert.equal(await store.channelCapabilityMembership("C-one", "U-old-one"), false); + assert.equal(await store.channelCapabilityMembership("C-one", "U-keep"), true); + await store.replaceCapabilityChannels(["C-one", "C-two", "C-new"], [], [], undefined, [ + { channelId: "C-new", principalId: "U-new" }, + ]); + assert.equal(await store.channelCapabilityMembership("C-new", "U-new"), false); + await store.replaceCapabilityChannels( + ["C-one", "C-two", "C-new"], + [{ channelId: "C-new", principalId: "U-new" }], + ["C-new"], + ); + assert.equal(await store.channelCapabilityMembership("C-new", "U-new"), true); +}); + +test("pg directory: a partial group swap preserves unknown rosters", { skip }, async () => { + const store = createPostgresDirectoryStore(URL!); + await store.replaceGroups( + [ + { groupId: "G-one", principalId: "U-old-one" }, + { groupId: "G-two", principalId: "U-old-two" }, + ], + undefined, + ["G-one", "G-two"], + ["G-one", "G-two"], + ); + await store.replaceGroups( + [{ groupId: "G-two", principalId: "U-new-two" }], + undefined, + ["G-one", "G-two", "G-new"], + ["G-two"], + ); + assert.equal(await store.groupMembership("G-one", "U-old-one"), true); + assert.equal(await store.groupMembership("G-two", "U-old-two"), false); + assert.equal(await store.groupMembership("G-two", "U-new-two"), true); + assert.equal(await store.groupMembership("G-new", "U-new"), undefined); +}); + test( "pg directory: an identical push still advances the stamp, so ordering survives content-idempotent pushes", { skip }, diff --git a/test/projects.test.ts b/test/projects.test.ts index af77ae29..99fd257c 100644 --- a/test/projects.test.ts +++ b/test/projects.test.ts @@ -206,6 +206,15 @@ test("capability scope checks follow current shared rosters", async () => { ], 1, ); + await built.directory.replaceCapabilityChannels( + ["C-public", "C-private"], + [ + { channelId: "C-public", principalId: "member" }, + { channelId: "C-private", principalId: "member" }, + ], + ["C-public", "C-private"], + 1, + ); await built.directory.replaceGroups([{ groupId: "G1", principalId: "member" }], 1); assert.equal(await built.app.authorizesCapabilityScope({ actorId: "member", scopeId: "channel:C-private" }), true); @@ -221,12 +230,27 @@ test("capability scope checks follow current shared rosters", async () => { 2, ); await built.directory.replaceGroups([], 2); + await built.directory.replaceCapabilityChannels(["C-public", "C-private"], [], ["C-public", "C-private"], 2); assert.equal(await built.app.authorizesCapabilityScope({ actorId: "member", scopeId: "channel:C-private" }), false); assert.equal(await built.app.authorizesCapabilityScope({ actorId: "member", scopeId: "group:G1" }), false); assert.equal(await built.app.authorizesCapabilityScope({ actorId: "member", scopeId: "channel:C-public" }), false); }); +test("channel capabilities bridge legacy public rosters but still honor deactivation", async () => { + const built = buildApp(testConfig({ dataDir: mkdtempSync(join(tmpdir(), "capability-transition-")) })); + await built.app.upsertDirectory([{ principalId: "member", displayName: "Member", type: "internal" }]); + await built.directory.replaceChannels([{ channelId: "C-public", name: "public" }], []); + assert.equal(await built.app.authorizesCapabilityScope({ actorId: "member", scopeId: "channel:C-public" }), true); + await built.directory.replaceCapabilityChannels( + ["C-public"], + [{ channelId: "C-public", principalId: "member" }], + ["C-public"], + ); + await built.identity.deactivate("member"); + assert.equal(await built.app.authorizesCapabilityScope({ actorId: "member", scopeId: "channel:C-public" }), false); +}); + async function listen(server: Server): Promise { await new Promise((resolve) => server.listen(0, resolve)); return `http://127.0.0.1:${(server.address() as AddressInfo).port}`; diff --git a/test/slack-index.integration.test.ts b/test/slack-index.integration.test.ts index 68a8e6e5..5404246b 100644 --- a/test/slack-index.integration.test.ts +++ b/test/slack-index.integration.test.ts @@ -346,8 +346,16 @@ async function fixture(options: { externalParticipants?: boolean; webUiPublicUrl is_private: false, is_ext_shared: true, }); + app.client.channelsById.set("CPX", { + id: "CPX", + name: "private-shared", + is_member: true, + is_private: true, + is_ext_shared: true, + }); app.client.membersByChannel.set("C1", ["U1", "U2", "UBOT"]); app.client.membersByChannel.set("CX", ["U1", "UX", "UBOT"]); + app.client.membersByChannel.set("CPX", ["U1", "UX", "UBOT"]); const plugin = await started; await new Promise((resolve) => setImmediate(resolve)); return { app, client: app.client, core, stop: () => plugin.stop() }; @@ -479,15 +487,30 @@ test("large public channels publish their complete roster and accept internal tu } }); -test("failed roster reads are marked unknown instead of clearing known members", async () => { +test("failed background roster reads are marked unknown instead of clearing known capabilities", async () => { const f = await fixture(); try { - assert.ok(f.core.directories.at(-1).channelRosterIds.includes("C1")); + assert.ok(f.core.directories.at(-1).capabilityChannelRosterIds.includes("C1")); f.client.membershipFailures.add("C1"); const pushes = f.core.directories.length; - await f.app.emitEvent("member_left_channel", { user: "U2", channel: "C1", event_ts: "100.5" }); + await f.app.emitEvent("channel_rename", { channel: { id: "C1" }, event_ts: "100.5" }); await waitFor(() => f.core.directories.length > pushes); - assert.ok(!f.core.directories.at(-1).channelRosterIds.includes("C1")); + assert.ok(!f.core.directories.at(-1).capabilityChannelRosterIds.includes("C1")); + } finally { + await f.stop(); + } +}); + +test("a failed refresh after a leave event revokes only the departing member", async () => { + const f = await fixture(); + try { + f.client.membershipFailures.add("C1"); + const pushes = f.core.directories.length; + await f.app.emitEvent("member_left_channel", { user: "U2", channel: "C1", event_ts: "100.6" }); + await waitFor(() => f.core.directories.length > pushes); + const pushed = f.core.directories.at(-1); + assert.ok(!pushed.capabilityChannelRosterIds.includes("C1")); + assert.deepEqual(pushed.capabilityChannelRevocations, [{ channelId: "C1", principalId: "U2" }]); } finally { await f.stop(); } @@ -497,11 +520,17 @@ test("Slack Connect directory rosters contain only internal humans", async () => const f = await fixture({ externalParticipants: true }); try { const pushed = f.core.directories.at(-1); - assert.ok(pushed.channelRosterIds.includes("CX")); + assert.ok(pushed.capabilityChannelRosterIds.includes("CX")); + assert.ok(pushed.capabilityChannelRosterIds.includes("CPX")); assert.deepEqual( - pushed.channelMembers.filter((m: any) => m.channelId === "CX").map((m: any) => m.principalId), + pushed.capabilityChannelMembers.filter((m: any) => m.channelId === "CX").map((m: any) => m.principalId), ["U1"], ); + assert.deepEqual( + pushed.capabilityChannelMembers.filter((m: any) => m.channelId === "CPX").map((m: any) => m.principalId), + ["U1"], + ); + assert.ok(!pushed.channelRosterIds.includes("CPX")); } finally { await f.stop(); } @@ -941,6 +970,47 @@ test("a failed group listing pushes its fallback rows under the OLD stamp, never } }); +test("a failed group member read marks only that roster unknown", async () => { + const f = await fixture(); + try { + f.client.channelsById.set("G5", { id: "G5", name: "", is_member: true, is_private: true, is_mpim: true }); + f.client.membersByChannel.set("G5", ["U1", "U2", "UBOT"]); + await f.app.emitMessage({ channel: "G5", channel_type: "mpim", user: "U1", text: "hi", ts: "403.1" }); + await waitFor(() => + f.core.directories.some((d: any) => (d.groupMembers ?? []).some((g: any) => g.groupId === "G5")), + ); + const good = f.core.directories.findLast((d: any) => d.groupsSyncedAt !== undefined); + f.client.membershipFailures.add("G5"); + const pushes = f.core.directories.length; + await f.app.emitEvent("channel_rename", { channel: { id: "C1" }, event_ts: "403.2" }); + await waitFor(() => f.core.directories.length > pushes); + const last = f.core.directories.at(-1); + assert.ok(last.groupsSyncedAt > good.groupsSyncedAt); + assert.ok(last.groupIds.includes("G5")); + assert.ok(!last.groupRosterIds.includes("G5")); + assert.equal(last.groupMembers.filter((member: any) => member.groupId === "G5").length, 0); + } finally { + await f.stop(); + } +}); + +test("all listed group DMs reach the directory past the legacy private-channel cap", async () => { + const f = await fixture(); + try { + for (let i = 0; i < 51; i++) { + const id = `G${i}`; + f.client.channelsById.set(id, { id, name: "", is_member: true, is_private: true, is_mpim: true }); + f.client.membersByChannel.set(id, ["U1", "U2", "UBOT"]); + } + const pushes = f.core.directories.length; + await f.app.emitEvent("channel_rename", { channel: { id: "C1" }, event_ts: "403.3" }); + await waitFor(() => f.core.directories.length > pushes); + assert.equal(new Set(f.core.directories.at(-1).groupMembers.map((member: any) => member.groupId)).size, 51); + } finally { + await f.stop(); + } +}); + test("a group DM whose listing fails is retried at most once, never once per message", async () => { const f = await fixture(); try { From 86e95ee413d7dccbc4b4565d09550a99b151497f Mon Sep 17 00:00:00 2001 From: Josh France <12610835+16francej@users.noreply.github.com> Date: Sat, 15 Aug 2026 13:25:11 -0700 Subject: [PATCH 05/10] Refine shared runtime behavior --- src/api/app-helpers.ts | 9 ++-- src/credentials/keychain.ts | 45 +++++++++++++------- src/credentials/paths.ts | 2 +- src/deploy/docker-deploy-provider.ts | 45 ++++++++++++-------- src/identity/identity-service.ts | 2 +- src/slack/directory.ts | 54 ++++++++++++------------ src/slack/identity.ts | 13 +++--- src/slack/turn-handler.ts | 1 - test/capability-routes.test.ts | 4 +- test/docker-deploy-provider.test.ts | 43 +++++++++++++++++++ test/external-slack-participants.test.ts | 7 ++- test/identity.test.ts | 6 +-- test/keychain.test.ts | 45 +++++++++++++------- test/projects.test.ts | 5 ++- test/slack-identity.test.ts | 4 +- test/slack-index.integration.test.ts | 45 ++++++++++++-------- 16 files changed, 211 insertions(+), 119 deletions(-) diff --git a/src/api/app-helpers.ts b/src/api/app-helpers.ts index 9c42dd48..eaefecbd 100644 --- a/src/api/app-helpers.ts +++ b/src/api/app-helpers.ts @@ -405,10 +405,11 @@ export function createAppHelpers(deps: AppDeps, app: App) { ): Promise { const { kind, ref } = parseScopeId(claims.scopeId); if (kind === "channel" && !deps.identity.isInternal(deps.identity.classify(claims.actorId))) return false; - const capabilityMembership = - kind === "channel" - ? await deps.directory.channelCapabilityMembership(ref, claims.actorId).catch(() => undefined) - : undefined; + const privateChannel = + kind === "channel" && (await deps.directory.channelPrivacy?.(ref).catch(() => undefined)) === true; + const capabilityMembership = privateChannel + ? await deps.directory.channelCapabilityMembership(ref, claims.actorId).catch(() => undefined) + : undefined; if ( (kind === "channel" && !(capabilityMembership ?? (await principalCanAccessCurrentScope(claims.actorId, claims.scopeId)))) || diff --git a/src/credentials/keychain.ts b/src/credentials/keychain.ts index e152bc6d..d7b088fa 100644 --- a/src/credentials/keychain.ts +++ b/src/credentials/keychain.ts @@ -1262,19 +1262,34 @@ export function createKeychain(deps: { }; } -const FILE_ENV_POINTERS: Array<[RegExp, (abs: string) => string]> = [ - [/(^|\/)\.aws\/credentials$/, (abs) => `export AWS_SHARED_CREDENTIALS_FILE="${abs}"`], - [/(^|\/)\.aws\/config$/, (abs) => `export AWS_CONFIG_FILE="${abs}"`], - [/(^|\/)\.kube\/config$/, (abs) => `export KUBECONFIG="${abs}"`], - [/(^|\/)\.config\/gh\/hosts\.yml$/, (abs) => `export GH_CONFIG_DIR="${abs.replace(/\/hosts\.yml$/, "")}"`], +const tempCredentialPath = (rel: string): string => + /^[A-Za-z0-9._@+ /-]+$/.test(rel) ? `"$__kc_dir/${rel}"` : `"$__kc_dir"${shq(`/${rel}`)}`; + +const FILE_ENV_POINTERS: Array<[RegExp, (rel: string) => string]> = [ + [/(^|\/)\.aws\/credentials$/, (rel) => `export AWS_SHARED_CREDENTIALS_FILE=${tempCredentialPath(rel)}`], + [/(^|\/)\.aws\/config$/, (rel) => `export AWS_CONFIG_FILE=${tempCredentialPath(rel)}`], + [/(^|\/)\.kube\/config$/, (rel) => `export KUBECONFIG=${tempCredentialPath(rel)}`], + [ + /(^|\/)\.config\/gh\/hosts\.yml$/, + (rel) => `export GH_CONFIG_DIR=${tempCredentialPath(rel.replace(/\/hosts\.yml$/, ""))}`, + ], [ /(^|\/)(?:\.config\/(?:glab-cli|glab)|Library\/Application Support\/glab-cli)\/config\.yml$/, - (abs) => `export GLAB_CONFIG_DIR="${abs.replace(/\/config\.yml$/, "")}"`, + (rel) => `export GLAB_CONFIG_DIR=${tempCredentialPath(rel.replace(/\/config\.yml$/, ""))}`, + ], + [ + /(^|\/)\.docker\/config\.json$/, + (rel) => `export DOCKER_CONFIG=${tempCredentialPath(rel.replace(/\/config\.json$/, ""))}`, + ], + [/(^|\/)\.npmrc$/, (rel) => `export NPM_CONFIG_USERCONFIG=${tempCredentialPath(rel)}`], + [/(^|\/)\.netrc$/, (rel) => `export NETRC=${tempCredentialPath(rel)}`], + [ + /(^|\/)\.ssh\/[^/]*(id_|key)[^/]*$/, + (rel) => + /^[A-Za-z0-9._@+ /-]+$/.test(rel) + ? `export GIT_SSH_COMMAND="ssh -i $__kc_dir/${rel} -o IdentitiesOnly=yes"` + : `export GIT_SSH_COMMAND="ssh -i $__kc_dir"${shq(`/${rel}`)}" -o IdentitiesOnly=yes"`, ], - [/(^|\/)\.docker\/config\.json$/, (abs) => `export DOCKER_CONFIG="${abs.replace(/\/config\.json$/, "")}"`], - [/(^|\/)\.npmrc$/, (abs) => `export NPM_CONFIG_USERCONFIG="${abs}"`], - [/(^|\/)\.netrc$/, (abs) => `export NETRC="${abs}"`], - [/(^|\/)\.ssh\/[^/]*(id_|key)[^/]*$/, (abs) => `export GIT_SSH_COMMAND="ssh -i ${abs} -o IdentitiesOnly=yes"`], ]; export function renderUseScript(m: MaterializedCred): string { @@ -1283,18 +1298,16 @@ export function renderUseScript(m: MaterializedCred): string { const lines = [`__kc_dir="$(mktemp -d "\${TMPDIR:-/tmp}/keychain.XXXXXX")"`, `umask 077`]; for (const f of files) { const parent = f.path.includes("/") ? f.path.replace(/\/[^/]*$/, "") : ""; - if (parent) lines.push(`mkdir -p "$__kc_dir/${parent}"`); - lines.push( - `printf '%s' ${shq(f.contentBase64)} | base64 -d > "$__kc_dir/${f.path}"`, - `chmod 600 "$__kc_dir/${f.path}"`, - ); + if (parent) lines.push(`mkdir -p ${tempCredentialPath(parent)}`); + const path = tempCredentialPath(f.path); + lines.push(`printf '%s' ${shq(f.contentBase64)} | base64 -d > ${path}`, `chmod 600 ${path}`); } const pointed = new Set(); for (const f of files) { for (const [re, render] of FILE_ENV_POINTERS) { if (re.test(f.path) && !pointed.has(re)) { pointed.add(re); - lines.push(render(`$__kc_dir/${f.path}`)); + lines.push(render(f.path)); } } } diff --git a/src/credentials/paths.ts b/src/credentials/paths.ts index 3eaaa5dd..2a39a8e0 100644 --- a/src/credentials/paths.ts +++ b/src/credentials/paths.ts @@ -4,7 +4,7 @@ function stripHomePrefix(target: string): string { export function homeRelativePath(path: string): string { const rel = stripHomePrefix(path).replace(/^\.\//, ""); - if (!rel || rel.startsWith("/") || rel.split("/").includes("..") || !/^[A-Za-z0-9._@+ /-]+$/.test(rel)) { + if (!rel || rel.startsWith("/") || rel.split("/").includes("..") || rel.includes("\0")) { throw new Error(`file path must be home-relative: ${path}`); } return rel; diff --git a/src/deploy/docker-deploy-provider.ts b/src/deploy/docker-deploy-provider.ts index 4ef1193f..a09bcd76 100644 --- a/src/deploy/docker-deploy-provider.ts +++ b/src/deploy/docker-deploy-provider.ts @@ -49,23 +49,35 @@ export function createDockerDeployProvider(opts: DockerDeployProviderOptions = { const migrateContainer = async (container: string): Promise => { const inspected = await dexec(["inspect", "--format", "{{json .NetworkSettings.Networks}}", container]); - if (inspected.code !== 0) return false; + if (inspected.code !== 0) { + if (/no such (?:object|container)|not found/i.test(inspected.stderr)) return false; + throw new Error(`docker inspect ${container} failed: ${inspected.stderr.trim()}`); + } let attached: Record; try { attached = JSON.parse(inspected.stdout) as Record; } catch { - return false; + throw new Error(`docker inspect ${container} returned invalid network state`); } const target = `${container}-net`; + await ensureNetwork(target); + if (!(target in attached)) { + const connected = await dexec(["network", "connect", target, container]); + if (connected.code !== 0) throw new Error(`docker network connect ${target} failed: ${connected.stderr.trim()}`); + } + if (LEGACY_NETWORK in attached) { + const disconnected = await dexec(["network", "disconnect", LEGACY_NETWORK, container]); + if (disconnected.code !== 0) + throw new Error(`docker network disconnect ${LEGACY_NETWORK} failed: ${disconnected.stderr.trim()}`); + } + return true; + }; + const migrateTarget = async (container: string): Promise => { try { - await ensureNetwork(target); + return await migrateContainer(container); } catch { - return false; + return migrateContainer(container); } - if (!(target in attached) && (await dexec(["network", "connect", target, container])).code !== 0) return false; - if (LEGACY_NETWORK in attached && (await dexec(["network", "disconnect", LEGACY_NETWORK, container])).code !== 0) - return false; - return true; }; let migrationRetryable = false; @@ -83,7 +95,11 @@ export function createDockerDeployProvider(opts: DockerDeployProviderOptions = { for (const container of listed.stdout .split(/\s+/) .filter((candidate) => /^agent-deploy-[a-zA-Z0-9_-]+$/.test(candidate))) { - if (!(await migrateContainer(container))) migrated = false; + try { + if (!(await migrateContainer(container))) migrated = false; + } catch { + migrated = false; + } } if (!migrated) return false; const removed = await dexec(["network", "rm", LEGACY_NETWORK]); @@ -131,17 +147,12 @@ export function createDockerDeployProvider(opts: DockerDeployProviderOptions = { }); return migrationInFlight; }; - const ensureMigration = async (): Promise => { - if ((await runMigration()) || (await runMigration())) return; - throw new Error("legacy Docker network migration incomplete"); - }; void runMigration(); return { profile: { managedScaleToZero: false }, async apply(d: Deployment, version: DeploymentVersion): Promise { - await ensureMigration(); const net = await ensureNetwork(network(d)); await dexec(["rm", "-f", name(d)]); const hostPort = allocPort(name(d)); @@ -183,7 +194,7 @@ export function createDockerDeployProvider(opts: DockerDeployProviderOptions = { }, async logs(d: Deployment, opts: { tailLines: number }): Promise { - await ensureMigration(); + if (!(await migrateTarget(name(d)))) return null; const lines = Math.max(1, Math.min(2000, Math.floor(opts.tailLines))); const r = await dexec(["logs", "--tail", String(lines), name(d)]); if (r.code !== 0) return null; @@ -191,15 +202,13 @@ export function createDockerDeployProvider(opts: DockerDeployProviderOptions = { }, async destroy(d: Deployment): Promise { - await ensureMigration(); await dexec(["rm", "-f", name(d)]); await dexec(["network", "rm", network(d)]); freePort(name(d)); }, async resolveEndpoint(d): Promise { - await ensureMigration(); - return (await migrateContainer(name(d))) ? d.endpoint : null; + return (await migrateTarget(name(d))) ? d.endpoint : null; }, }; } diff --git a/src/identity/identity-service.ts b/src/identity/identity-service.ts index 77c214bd..bcf3f479 100644 --- a/src/identity/identity-service.ts +++ b/src/identity/identity-service.ts @@ -104,7 +104,7 @@ export function createIdentityService(backing?: DurableMap): return refreshP; }, resolve(actor: ActorAssertion): Principal { - const p = classify(actor.externalId, actor.isExternalGuest || actor.isBot); + const p = classify(actor.externalId, actor.isExternalGuest); return { ...p, ...(actor.teamIds ? { teamIds: actor.teamIds } : {}), diff --git a/src/slack/directory.ts b/src/slack/directory.ts index 36e966d7..00e64b8f 100644 --- a/src/slack/directory.ts +++ b/src/slack/directory.ts @@ -194,7 +194,25 @@ export function createDirectory(deps: { refs: ReadonlyArray<{ id: string; info?: ChannelMeta }>, kind: RosterKind, ): Promise> { + const classified = await allClassifiedRosters(client, refs, kind); const rosters = new Map(); + for (const ref of refs) { + const roster = classified.get(ref.id); + if (!roster) continue; + const internalIds = kind.allowExternal + ? internalChannelMembers(roster.actors, roster.complete) + : allInternalChannelMembers(roster.actors, roster.complete, ref.info); + if (internalIds) rosters.set(ref.id, internalIds); + } + return rosters; + } + + async function allClassifiedRosters( + client: any, + refs: ReadonlyArray<{ id: string; info?: ChannelMeta }>, + kind: RosterKind, + ): Promise> { + const rosters = new Map(); const limit = kind.limit ?? MAX_PRIVATE_CHANNELS; const slice = refs.slice(0, limit); if (refs.length > slice.length) { @@ -217,10 +235,7 @@ export function createDirectory(deps: { actors.push(actor); if (!ok) complete = false; } - const internalIds = kind.allowExternal - ? internalChannelMembers(actors, complete) - : allInternalChannelMembers(actors, complete, ref.info); - if (internalIds) rosters.set(ref.id, internalIds); + rosters.set(ref.id, { actors, complete }); } return rosters; } @@ -253,40 +268,25 @@ export function createDirectory(deps: { }); for (const channel of publicChannels) { const internalIds = publicRosters.get(channel.id); - if (!internalIds) { - for (const principalId of invalidations.get(channel.id) ?? []) { - capabilityChannelRevocations.push({ channelId: channel.id, principalId }); - } - continue; - } + if (!internalIds) continue; channelRosterIds.push(channel.id); - capabilityChannelRosterIds.push(channel.id); - for (const pid of internalIds) { - channelMembers.push({ channelId: channel.id, principalId: pid }); - capabilityChannelMembers.push({ channelId: channel.id, principalId: pid }); - } + for (const pid of internalIds) channelMembers.push({ channelId: channel.id, principalId: pid }); } - const rosters = await allInternalRosters(client, privateChannels, { + const rosters = await allClassifiedRosters(client, privateChannels, { plural: "private channels", - authz: "private-channel-send", + authz: "private-channel", item: "private channel", limit: privateChannels.length, }); - const capabilityRosters = await allInternalRosters(client, privateChannels, { - plural: "private channels", - authz: "private-channel-capability", - item: "private channel", - limit: privateChannels.length, - allowExternal: true, - }); for (const c of privateChannels) { channels.push({ channelId: c.id, name: c.name, isPrivate: true }); - const internalIds = rosters.get(c.id); + const roster = rosters.get(c.id); + const internalIds = roster && allInternalChannelMembers(roster.actors, roster.complete, c.info); if (internalIds) { channelRosterIds.push(c.id); for (const pid of internalIds) channelMembers.push({ channelId: c.id, principalId: pid }); } - const capabilityIds = capabilityRosters.get(c.id); + const capabilityIds = roster && internalChannelMembers(roster.actors, roster.complete); if (!capabilityIds) { for (const principalId of invalidations.get(c.id) ?? []) { capabilityChannelRevocations.push({ channelId: c.id, principalId }); @@ -452,7 +452,7 @@ export function createDirectory(deps: { invalidations: ChannelInvalidations = new Map(), ): Promise { const members = [...snap.byId.entries()] - .filter(([, u]) => !u.actor.isExternalGuest && !u.actor.isBot) + .filter(([, u]) => !u.actor.isExternalGuest) .map(([slackId, u]) => { const a = u.actor; return { diff --git a/src/slack/identity.ts b/src/slack/identity.ts index 3e90fbb7..64149dad 100644 --- a/src/slack/identity.ts +++ b/src/slack/identity.ts @@ -128,7 +128,7 @@ export function computeChannelAudience( ): ActorAssertion[] { if (members && members.length) { const byId = new Map(); - for (const m of [actor, ...members]) if (m.externalId && !m.isBot) byId.set(m.externalId, m); + for (const m of [actor, ...members]) if (m.externalId) byId.set(m.externalId, m); const audience = [...byId.values()]; if (isExternallyShared(info) && audience.every((m) => !m.isExternalGuest)) { audience.push(externalMarker()); @@ -147,7 +147,7 @@ export function computePublishMembers( ): ActorAssertion[] | undefined { if (!complete) return undefined; if (isExternallyShared(info)) return undefined; - const all = [actor, ...members].filter((m) => !m.isBot); + const all = [actor, ...members]; if (all.some((m) => m.isExternalGuest)) return undefined; const byId = new Map(); for (const m of all) if (m.externalId) byId.set(m.externalId, m); @@ -161,15 +161,14 @@ export function allInternalChannelMembers( ): string[] | undefined { if (!complete) return undefined; if (isExternallyShared(info)) return undefined; - const humans = members.filter((m) => !m.isBot); - if (humans.some((m) => m.isExternalGuest)) return undefined; - return internalChannelMembers(humans, true); + if (members.some((m) => m.isExternalGuest)) return undefined; + return internalChannelMembers(members, true); } export function internalChannelMembers(members: ActorAssertion[], complete: boolean): string[] | undefined { if (!complete) return undefined; const ids = new Set(); - for (const m of members) if (m.externalId && !m.isExternalGuest && !m.isBot) ids.add(m.externalId); + for (const m of members) if (m.externalId && !m.isExternalGuest) ids.add(m.externalId); return [...ids]; } @@ -193,7 +192,7 @@ export async function resolveChannelMembership(opts: { for (const id of memberIds) { const { actor: member, ok } = await opts.classify(id); members.push(member); - if (member.externalId && !member.isExternalGuest && !member.isBot) slackIdsByPrincipal.set(member.externalId, id); + if (member.externalId && !member.isExternalGuest) slackIdsByPrincipal.set(member.externalId, id); if (!ok) complete = false; } const audience = computeChannelAudience(actor, members, info); diff --git a/src/slack/turn-handler.ts b/src/slack/turn-handler.ts index d56587f6..167407c8 100644 --- a/src/slack/turn-handler.ts +++ b/src/slack/turn-handler.ts @@ -196,7 +196,6 @@ export function createTurnHandler(deps: { const timezone = classified.timezone; const text = stripMention(inc.rawText, ids.botUserId); if (!hasContent(text, inc.files)) return; - if (actor.isBot || inc.botAuthored) return; let audience: ActorAssertion[] = [actor]; let channelRef: string | undefined; diff --git a/test/capability-routes.test.ts b/test/capability-routes.test.ts index f9bc347c..265a0555 100644 --- a/test/capability-routes.test.ts +++ b/test/capability-routes.test.ts @@ -747,12 +747,12 @@ describe("capability-token control plane (crons + SOUL)", () => { ); }); - it("a dedicated capability roster revokes a member even when a legacy channel roster still contains them", async () => { + it("a public channel remains available to an active internal principal outside its current roster", async () => { await built.directory.replaceCapabilityChannels( ["C"], ["admin-alice", "U1", "U2"].map((principalId) => ({ channelId: "C", principalId })), ["C"], ); - assert.equal((await get("/v1/soul", { "x-agent-capability": await capChannel("U8") })).status, 403); + assert.equal((await get("/v1/soul", { "x-agent-capability": await capChannel("U8") })).status, 200); }); }); diff --git a/test/docker-deploy-provider.test.ts b/test/docker-deploy-provider.test.ts index c72a64f0..a3c0b4d4 100644 --- a/test/docker-deploy-provider.test.ts +++ b/test/docker-deploy-provider.test.ts @@ -119,3 +119,46 @@ test("Docker provider retries legacy migration after the daemon recovers", async assert.equal(legacyInspections, 2); assert.ok(calls.some((args) => args.join(" ") === `network disconnect agent-deploynet ${container}`)); }); + +test("an unrelated legacy migration failure does not block a new deployment", async () => { + const dockerExec: DockerExec = async (args) => { + if (args.join(" ") === "network inspect --format {{range .Containers}}{{println .Name}}{{end}} agent-deploynet") { + return { code: 0, stdout: "agent-deploy-broken\n", stderr: "" }; + } + if (args[0] === "inspect") return { code: 1, stdout: "", stderr: "daemon unavailable" }; + if (args[0] === "network" && args[1] === "inspect") return { code: 1, stdout: "", stderr: "missing" }; + return { code: 0, stdout: "", stderr: "" }; + }; + const store = createDeployStore(); + const deployment = await store.create({ + ownerScopeId: scopeId("personal", "U1"), + createdBy: "U1", + entrypoint: "node server.js", + snapshotDir: "/snap/new", + }); + const provider = createDockerDeployProvider({ dockerExec }); + + await assert.doesNotReject(provider.apply(deployment, deployment.versions[0]!)); +}); + +test("a transient target inspection failure does not report the deployment missing", async () => { + const dockerExec: DockerExec = async (args) => { + if (args.join(" ") === "network inspect --format {{range .Containers}}{{println .Name}}{{end}} agent-deploynet") { + return { code: 1, stdout: "", stderr: "No such network" }; + } + if (args[0] === "inspect") return { code: 1, stdout: "", stderr: "daemon unavailable" }; + return { code: 0, stdout: "", stderr: "" }; + }; + const store = createDeployStore(); + const deployment = await store.create({ + ownerScopeId: scopeId("personal", "U1"), + createdBy: "U1", + entrypoint: "node server.js", + snapshotDir: "/snap/running", + }); + await store.setEndpoint(deployment.id, { host: "127.0.0.1", port: 9200 }); + const running = (await store.get(deployment.id))!; + const provider = createDockerDeployProvider({ dockerExec }); + + await assert.rejects(provider.resolveEndpoint!(running, running.versions[0]!), /daemon unavailable/); +}); diff --git a/test/external-slack-participants.test.ts b/test/external-slack-participants.test.ts index 65c1cb0e..f95d5a15 100644 --- a/test/external-slack-participants.test.ts +++ b/test/external-slack-participants.test.ts @@ -89,7 +89,7 @@ test("the toggle never lets an external actor interact", async () => { assert.match(res.reason ?? "", /internal-only/); }); -test("a bot assertion is refused before entering the turn pipeline", async () => { +test("a bot assertion can enter the turn pipeline", async () => { const built = freshApp(); const res = await built.app.turn({ surface: "slack", @@ -97,9 +97,8 @@ test("a bot assertion is refused before entering the turn pipeline", async () => conversation: { kind: "dm", threadRef: "dm:B1:t1" }, text: "hello", }); - assert.equal(res.status, "refused"); - assert.match(res.reason ?? "", /internal-only/); - assert.equal((await built.runs.list()).length, 0); + assert.equal(res.status, "ok"); + assert.equal((await built.runs.list()).length, 1); }); test("admin resource: org-only PUT, read-back, and the surface-config echo", async () => { diff --git a/test/identity.test.ts b/test/identity.test.ts index 8d6b5b9a..58eaa044 100644 --- a/test/identity.test.ts +++ b/test/identity.test.ts @@ -17,10 +17,10 @@ test("classifies a flagged Slack Connect user as guest", () => { assert.equal(id.isInternal(p), false); }); -test("resolves bot assertions as non-internal", () => { +test("resolves bot assertions as internal automation callers", () => { const p = id.resolve({ externalId: "B1", isBot: true }); - assert.equal(p.type, "guest"); - assert.equal(id.isInternal(p), false); + assert.equal(p.type, "internal"); + assert.equal(id.isInternal(p), true); }); test("audienceIsAllInternal is false if any member is non-internal (G1)", () => { diff --git a/test/keychain.test.ts b/test/keychain.test.ts index 44eb1d9c..4994676b 100644 --- a/test/keychain.test.ts +++ b/test/keychain.test.ts @@ -5,6 +5,7 @@ import assert from "node:assert/strict"; import { mkdtempSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; +import { execFileSync } from "node:child_process"; import type { AddressInfo } from "node:net"; import type { Server } from "node:http"; import { buildApp, type BuiltApp } from "../src/wiring.ts"; @@ -502,22 +503,36 @@ test("file bundles: one item per service, materialize to a /tmp script with env (e: KeychainError) => e.status === 400, "paths must be home-relative", ); - await assert.rejects( - k.save({ ownerId: "U1", service: "unsafe", files: [{ path: "$(uname)", contentBase64: b64("x") }] }), - (e: KeychainError) => e.status === 400, - "paths must contain portable filename characters", - ); - assert.throws( - () => - renderUseScript({ - kind: "file", - credentialId: "legacy-unsafe", - ownerId: "U1", - service: "unsafe", - files: [{ path: "$(uname)", contentBase64: b64("x") }], - }), - /file path must be home-relative/, + const unusual = await k.save({ + ownerId: "U1", + service: "unusual", + files: [{ path: "$(echo injected)", contentBase64: b64("x") }], + }); + assert.deepEqual(unusual.targets, ["$(echo injected)"]); + const unusualScript = renderUseScript({ + kind: "file", + credentialId: "legacy-unusual", + ownerId: "U1", + service: "unusual", + files: [{ path: "$(echo injected)", contentBase64: b64("x") }], + }); + const renderedPath = execFileSync("sh", ["-c", `${unusualScript}\nfind "$HOME" -maxdepth 1 -type f -print`], { + encoding: "utf8", + }); + assert.match(renderedPath, /\/\$\(echo injected\)$/m); + const pointerScript = renderUseScript({ + kind: "file", + credentialId: "legacy-pointer", + ownerId: "U1", + service: "unusual-pointer", + files: [{ path: "$(echo injected)/.aws/config", contentBase64: b64("x") }], + }); + const pointerPath = execFileSync( + "sh", + ["-c", `${pointerScript}\nprintf '%s\\n' "$AWS_CONFIG_FILE"\nfind "$__kc_dir" -type f -print`], + { encoding: "utf8" }, ); + assert.match(pointerPath, /\/\$\(echo injected\)\/\.aws\/config$/m); const grant = await k.createGrant({ credentialId: cred.id, diff --git a/test/projects.test.ts b/test/projects.test.ts index 99fd257c..40c72d7f 100644 --- a/test/projects.test.ts +++ b/test/projects.test.ts @@ -203,6 +203,7 @@ test("capability scope checks follow current shared rosters", async () => { [ { channelId: "C-public", principalId: "member" }, { channelId: "C-private", principalId: "member" }, + { channelId: "C-private", principalId: "B1" }, ], 1, ); @@ -211,6 +212,7 @@ test("capability scope checks follow current shared rosters", async () => { [ { channelId: "C-public", principalId: "member" }, { channelId: "C-private", principalId: "member" }, + { channelId: "C-private", principalId: "B1" }, ], ["C-public", "C-private"], 1, @@ -218,6 +220,7 @@ test("capability scope checks follow current shared rosters", async () => { await built.directory.replaceGroups([{ groupId: "G1", principalId: "member" }], 1); assert.equal(await built.app.authorizesCapabilityScope({ actorId: "member", scopeId: "channel:C-private" }), true); + assert.equal(await built.app.authorizesCapabilityScope({ actorId: "B1", scopeId: "channel:C-private" }), true); assert.equal(await built.app.authorizesCapabilityScope({ actorId: "member", scopeId: "group:G1" }), true); assert.equal(await built.app.authorizesCapabilityScope({ actorId: "member", scopeId: "channel:C-public" }), true); @@ -234,7 +237,7 @@ test("capability scope checks follow current shared rosters", async () => { assert.equal(await built.app.authorizesCapabilityScope({ actorId: "member", scopeId: "channel:C-private" }), false); assert.equal(await built.app.authorizesCapabilityScope({ actorId: "member", scopeId: "group:G1" }), false); - assert.equal(await built.app.authorizesCapabilityScope({ actorId: "member", scopeId: "channel:C-public" }), false); + assert.equal(await built.app.authorizesCapabilityScope({ actorId: "member", scopeId: "channel:C-public" }), true); }); test("channel capabilities bridge legacy public rosters but still honor deactivation", async () => { diff --git a/test/slack-identity.test.ts b/test/slack-identity.test.ts index e351c1f0..c260e770 100644 --- a/test/slack-identity.test.ts +++ b/test/slack-identity.test.ts @@ -240,7 +240,7 @@ test("allInternalChannelMembers: all-internal + complete → deduped ids; WITHHE ); }); -test("bot accounts are absent from shared-scope rosters", () => { +test("bot accounts can hold shared-scope membership", () => { assert.deepEqual( allInternalChannelMembers( [ @@ -250,7 +250,7 @@ test("bot accounts are absent from shared-scope rosters", () => { true, { is_private: true }, ), - ["U1"], + ["U1", "B1"], ); }); diff --git a/test/slack-index.integration.test.ts b/test/slack-index.integration.test.ts index 5404246b..e3a166f0 100644 --- a/test/slack-index.integration.test.ts +++ b/test/slack-index.integration.test.ts @@ -23,6 +23,7 @@ class FakeSlackClient { readonly membersByChannel = new Map(); readonly messagesByChannel = new Map(); readonly membershipFailures = new Set(); + readonly membershipListings = new Map(); groupListings = 0; failGroupListing = false; private postSequence = 0; @@ -135,6 +136,7 @@ class FakeSlackClient { return; } if (method === "conversations.members") { + this.membershipListings.set(args.channel, (this.membershipListings.get(args.channel) ?? 0) + 1); if (this.membershipFailures.has(args.channel)) throw new Error("missing conversations:read"); yield { members: this.membersByChannel.get(args.channel) ?? [] }; return; @@ -490,12 +492,12 @@ test("large public channels publish their complete roster and accept internal tu test("failed background roster reads are marked unknown instead of clearing known capabilities", async () => { const f = await fixture(); try { - assert.ok(f.core.directories.at(-1).capabilityChannelRosterIds.includes("C1")); - f.client.membershipFailures.add("C1"); + assert.ok(f.core.directories.at(-1).capabilityChannelRosterIds.includes("CPX")); + f.client.membershipFailures.add("CPX"); const pushes = f.core.directories.length; await f.app.emitEvent("channel_rename", { channel: { id: "C1" }, event_ts: "100.5" }); await waitFor(() => f.core.directories.length > pushes); - assert.ok(!f.core.directories.at(-1).capabilityChannelRosterIds.includes("C1")); + assert.ok(!f.core.directories.at(-1).capabilityChannelRosterIds.includes("CPX")); } finally { await f.stop(); } @@ -504,26 +506,26 @@ test("failed background roster reads are marked unknown instead of clearing know test("a failed refresh after a leave event revokes only the departing member", async () => { const f = await fixture(); try { - f.client.membershipFailures.add("C1"); + f.client.membershipFailures.add("CPX"); const pushes = f.core.directories.length; - await f.app.emitEvent("member_left_channel", { user: "U2", channel: "C1", event_ts: "100.6" }); + await f.app.emitEvent("member_left_channel", { user: "U1", channel: "CPX", event_ts: "100.6" }); await waitFor(() => f.core.directories.length > pushes); const pushed = f.core.directories.at(-1); - assert.ok(!pushed.capabilityChannelRosterIds.includes("C1")); - assert.deepEqual(pushed.capabilityChannelRevocations, [{ channelId: "C1", principalId: "U2" }]); + assert.ok(!pushed.capabilityChannelRosterIds.includes("CPX")); + assert.deepEqual(pushed.capabilityChannelRevocations, [{ channelId: "CPX", principalId: "U1" }]); } finally { await f.stop(); } }); -test("Slack Connect directory rosters contain only internal humans", async () => { +test("Slack Connect directory rosters contain only internal principals", async () => { const f = await fixture({ externalParticipants: true }); try { const pushed = f.core.directories.at(-1); - assert.ok(pushed.capabilityChannelRosterIds.includes("CX")); + assert.ok(!pushed.capabilityChannelRosterIds.includes("CX")); assert.ok(pushed.capabilityChannelRosterIds.includes("CPX")); assert.deepEqual( - pushed.capabilityChannelMembers.filter((m: any) => m.channelId === "CX").map((m: any) => m.principalId), + pushed.channelMembers.filter((m: any) => m.channelId === "CX").map((m: any) => m.principalId), ["U1"], ); assert.deepEqual( @@ -531,6 +533,11 @@ test("Slack Connect directory rosters contain only internal humans", async () => ["U1"], ); assert.ok(!pushed.channelRosterIds.includes("CPX")); + f.client.membershipListings.set("CPX", 0); + const pushes = f.core.directories.length; + await f.app.emitEvent("channel_rename", { channel: { id: "C1" }, event_ts: "100.7" }); + await waitFor(() => f.core.directories.length > pushes); + assert.equal(f.client.membershipListings.get("CPX"), 1); } finally { await f.stop(); } @@ -727,7 +734,7 @@ test("an external principal is refused in a DM before core sees the text", async } }); -test("a bot-authored mention never becomes a turn", async () => { +test("a bot-authored mention can become a turn", async () => { const f = await fixture(); try { f.client.usersById.set("B1", { @@ -746,14 +753,15 @@ test("a bot-authored mention never becomes a turn", async () => { text: "<@UBOT> hello", ts: "102.2", }); - assert.equal(f.core.turns.length, 0); - assert.equal(f.client.posts.length, 0); + assert.equal(f.core.turns.length, 1); + assert.equal(f.core.turns[0].actor.externalId, "B1"); + assert.equal(f.client.posts[0].text, "agent reply"); } finally { await f.stop(); } }); -test("a bot-authored stop cannot abort a live run", async () => { +test("a bot-authored stop can abort a live run", async () => { const f = await fixture(); try { f.core.activeRun = "run-active"; @@ -766,7 +774,7 @@ test("a bot-authored stop cannot abort a live run", async () => { text: "stop", ts: "102.3", }); - assert.deepEqual(f.core.abortedRuns, []); + assert.deepEqual(f.core.abortedRuns, ["run-active"]); assert.equal(f.core.turns.length, 0); } finally { await f.stop(); @@ -1032,7 +1040,7 @@ test("a group DM whose listing fails is retried at most once, never once per mes } }); -test("a peer bot's thread reply is mirrored without dispatching a turn", async () => { +test("a peer bot's thread reply dispatches without attesting liveness", async () => { const f = await fixture(); try { f.client.usersById.set("UB2", { id: "UB2", team_id: "T1", name: "copilot", is_bot: true }); @@ -1050,7 +1058,10 @@ test("a peer bot's thread reply is mirrored without dispatching a turn", async ( ts: "301.3", thread_ts: "301.1", }); - assert.equal(f.core.turns.length, 0); + assert.equal(f.core.turns.length, 1); + assert.equal(f.core.turns[0].unprompted, true); + assert.equal(f.core.turns[0].entryTs, "301.3"); + assert.equal(f.core.turns[0].liveActor, undefined, "a bot author is automation, never a live act"); } finally { await f.stop(); } From 62ca2327ea17244f0de9f6bdf18af935dda0a09a Mon Sep 17 00:00:00 2001 From: Josh France <12610835+16francej@users.noreply.github.com> Date: Sat, 15 Aug 2026 14:14:38 -0700 Subject: [PATCH 06/10] Refine shared runtime behavior --- src/api/app-helpers.ts | 12 +++- src/api/app-turn.ts | 1 + src/api/app-types.ts | 4 +- src/api/git-http-broker.ts | 3 + src/api/routes/context.ts | 3 + src/api/routes/secret-drop.ts | 45 +++++++++++---- src/api/server.ts | 3 + src/auth/capability-token.ts | 2 + src/core/orchestrator.ts | 25 ++++---- src/deploy/docker-deploy-provider.ts | 69 ----------------------- src/directory/directory-store.ts | 16 ++++++ src/directory/postgres-directory-store.ts | 33 ++++++++++- src/slack/events.ts | 39 +++++++++++-- src/slack/identity.ts | 2 +- src/slack/turn-handler.ts | 13 ++++- src/types.ts | 1 + test/capability-routes.test.ts | 27 +++++++++ test/directory-store.test.ts | 16 ++++++ test/docker-deploy-provider.test.ts | 22 +------- test/git-http-broker.test.ts | 25 ++++++++ test/orchestrator.test.ts | 42 ++++++++++++++ test/postgres-directory-store.test.ts | 22 ++++++++ test/secret-drop.test.ts | 25 ++++++++ test/service-credential-route.test.ts | 21 +++++++ test/slack-index.integration.test.ts | 68 +++++++++++++++++++++- test/surface-context.test.ts | 11 +++- 26 files changed, 421 insertions(+), 129 deletions(-) diff --git a/src/api/app-helpers.ts b/src/api/app-helpers.ts index eaefecbd..cb29b8de 100644 --- a/src/api/app-helpers.ts +++ b/src/api/app-helpers.ts @@ -401,7 +401,7 @@ export function createAppHelpers(deps: AppDeps, app: App) { const membershipControlsScope = createMembershipControlsScope(scopeMembershipDeps); async function authorizesCapabilityScope( - claims: Pick, + claims: Pick, ): Promise { const { kind, ref } = parseScopeId(claims.scopeId); if (kind === "channel" && !deps.identity.isInternal(deps.identity.classify(claims.actorId))) return false; @@ -410,9 +410,17 @@ export function createAppHelpers(deps: AppDeps, app: App) { const capabilityMembership = privateChannel ? await deps.directory.channelCapabilityMembership(ref, claims.actorId).catch(() => undefined) : undefined; + const attestedBot = + privateChannel && + claims.botActor === true && + claims.liveActor === true && + claims.members?.some((member) => member.id === claims.actorId && member.type === "internal") === true; if ( (kind === "channel" && - !(capabilityMembership ?? (await principalCanAccessCurrentScope(claims.actorId, claims.scopeId)))) || + !( + attestedBot || + (capabilityMembership ?? (await principalCanAccessCurrentScope(claims.actorId, claims.scopeId))) + )) || (kind === "group" && !(await principalCanWriteScope(claims.actorId, claims.scopeId))) ) { return false; diff --git a/src/api/app-turn.ts b/src/api/app-turn.ts index 3ff04d7a..9d5dc70b 100644 --- a/src/api/app-turn.ts +++ b/src/api/app-turn.ts @@ -243,6 +243,7 @@ export function createTurnMethods( ...(req.readOnly ? { readOnly: true } : {}), ...(req.skipMemory ? { skipMemory: true } : {}), ...(req.unattendedGrants?.length ? { unattendedGrants: req.unattendedGrants } : {}), + ...(req.botActor ? { botActor: true } : {}), ...(req.surfaceTools ? { surfaceTools: true } : {}), ...(req.envelopeWrapped ? { envelopeWrapped: true } : {}), ...(typeof req.displayText === "string" && req.displayText ? { displayText: req.displayText } : {}), diff --git a/src/api/app-types.ts b/src/api/app-types.ts index c7dcfa5a..5bae2bbb 100644 --- a/src/api/app-types.ts +++ b/src/api/app-types.ts @@ -318,7 +318,9 @@ export interface App { listScopeResources(principalId: string, scope: ScopeId): Promise; managesScope(principalId: string, scope: ScopeId): Promise; membershipControlsScope(scope: ScopeId): Promise; - authorizesCapabilityScope(claims: Pick): Promise; + authorizesCapabilityScope( + claims: Pick, + ): Promise; openFileForViewer(id: string, principalId: string): Promise; grant(g: Grant): Promise; revokeGrant(ownerScopeId: ScopeId, ref: string, granteeScopeId: ScopeId, revokedBy: string): Promise; diff --git a/src/api/git-http-broker.ts b/src/api/git-http-broker.ts index cb09e838..6f325cb9 100644 --- a/src/api/git-http-broker.ts +++ b/src/api/git-http-broker.ts @@ -148,6 +148,9 @@ export async function brokerGitHttp(ctx: BaseCtx): Promise { actorId: claims.actorId, scopeId: claims.scopeId, ...(claims.scopeVersion ? { scopeVersion: claims.scopeVersion } : {}), + ...(claims.botActor ? { botActor: true } : {}), + ...(claims.liveActor ? { liveActor: true } : {}), + ...(claims.members ? { members: claims.members } : {}), })) ) { return sendJson(ctx.res, 403, { error: "forbidden", message: "capability scope membership has been revoked" }); diff --git a/src/api/routes/context.ts b/src/api/routes/context.ts index d316159a..326faae8 100644 --- a/src/api/routes/context.ts +++ b/src/api/routes/context.ts @@ -168,6 +168,9 @@ function awaitFileFulfillment(ctx: ApiCtx, requestId: string): Promise { actorId: cap.actorId, scopeId: cap.scopeId, ...(cap.scopeVersion ? { scopeVersion: cap.scopeVersion } : {}), + ...(cap.botActor ? { botActor: true } : {}), + ...(cap.liveActor ? { liveActor: true } : {}), + ...(cap.members ? { members: cap.members } : {}), aud: BLOB_TRANSFER_AUD, blob: { dir: "read", id: file.blobId }, exp: Date.now() + FILE_DOWNLOAD_TTL_MS, diff --git a/src/api/routes/secret-drop.ts b/src/api/routes/secret-drop.ts index 664c3844..e6253f6c 100644 --- a/src/api/routes/secret-drop.ts +++ b/src/api/routes/secret-drop.ts @@ -1,5 +1,10 @@ import { orgId as configOrgId } from "../../config.ts"; -import { mintCapabilityToken, verifyCapabilityToken, SECRET_DROP_AUD } from "../../auth/capability-token.ts"; +import { + mintCapabilityToken, + verifyCapabilityToken, + SECRET_DROP_AUD, + type CapabilityClaims, +} from "../../auth/capability-token.ts"; import { KeychainError, type GrantMode } from "../../credentials/keychain.ts"; import { SECRET_DROP_TTL_MS, type SecretDropField, type SecretDropRecord } from "../../credentials/secret-drop.ts"; import { isSharedScope, parseScopeId } from "../../types.ts"; @@ -43,7 +48,7 @@ function dropNotYoursHtml(): string {

This link is for someone else

This credential request was created for a different teammate. If it was meant for you, sign in as yourself and open it again.

`; } -function dropScopeAuthorized(ctx: ApiCtx, rec: SecretDropRecord): Promise { +function dropScopeAuthorized(ctx: ApiCtx, rec: SecretDropRecord, claims?: CapabilityClaims): Promise { const audienceScopeId = rec.audienceScopeId; if (!audienceScopeId || !isSharedScope(audienceScopeId)) return Promise.resolve(true); return ctx.app @@ -51,19 +56,32 @@ function dropScopeAuthorized(ctx: ApiCtx, rec: SecretDropRecord): Promise false); } -async function dropLinkTokenOk(ctx: ApiCtx, dropId: string, rec: SecretDropRecord): Promise { +async function dropLinkClaims( + ctx: ApiCtx, + dropId: string, + rec: SecretDropRecord, +): Promise { if (!rec.requiresToken) return true; const capSecret = ctx.deps.capabilitySecret ?? ctx.secret; const token = ctx.url.searchParams.get("t"); - if (!capSecret || !token) return false; + if (!capSecret || !token) return null; const claims = await verifyCapabilityToken(token, capSecret); - return ( - !!claims && claims.aud === SECRET_DROP_AUD && claims.drop === dropId && samePerson(claims.actorId, rec.ownerId) - ); + if ( + !claims || + claims.aud !== SECRET_DROP_AUD || + claims.drop !== dropId || + !samePerson(claims.actorId, rec.ownerId) || + (rec.audienceScopeId && claims.scopeId !== rec.audienceScopeId) + ) + return null; + return claims; } function dropFormHtml( @@ -165,6 +183,9 @@ async function mintDrop(ctx: ApiCtx): Promise { { actorId: capability.actorId, scopeId: capability.scopeId, + ...(capability.botActor ? { botActor: true } : {}), + ...(capability.liveActor ? { liveActor: true } : {}), + ...(capability.members ? { members: capability.members } : {}), aud: SECRET_DROP_AUD, drop: dropId, exp: Date.now() + SECRET_DROP_TTL_MS, @@ -183,7 +204,7 @@ async function dropForm(ctx: ApiCtx): Promise { res.writeHead(404, { "content-type": "text/html; charset=utf-8" }); return void res.end(dropFormHtml(params.id!, null)); } - if (!(await dropLinkTokenOk(ctx, params.id!, peeked.rec))) { + if (!(await dropLinkClaims(ctx, params.id!, peeked.rec))) { res.writeHead(404, { "content-type": "text/html; charset=utf-8" }); return void res.end(dropFormHtml(params.id!, null)); } @@ -213,7 +234,8 @@ async function redeemDrop(ctx: ApiCtx): Promise { : "this drop link is invalid or was already used — ask the agent for a fresh one"; return sendJson(res, 404, { error: "not_found", message }); } - if (!(await dropLinkTokenOk(ctx, params.id!, peeked.rec))) { + const linkClaims = await dropLinkClaims(ctx, params.id!, peeked.rec); + if (!linkClaims) { return sendJson(res, 404, { error: "not_found", message: "this drop link is invalid or was already used — ask the agent for a fresh one", @@ -225,7 +247,8 @@ async function redeemDrop(ctx: ApiCtx): Promise { message: "sign in as the account owner to complete this credential drop", }); } - if (!(await dropScopeAuthorized(ctx, peeked.rec))) { + const attestation = linkClaims === true ? undefined : linkClaims; + if (!(await dropScopeAuthorized(ctx, peeked.rec, attestation))) { await deps.secretDrops.redeem(params.id!).catch(() => null); return sendJson(res, 409, { error: "scope_changed", @@ -266,7 +289,7 @@ async function redeemDrop(ctx: ApiCtx): Promise { ...(drop.host ? { host: drop.host } : {}), origin: "secret-drop", }); - const mayShare = await dropScopeAuthorized(ctx, drop); + const mayShare = await dropScopeAuthorized(ctx, drop, attestation); let grantId: string | undefined; if (mayShare && drop.grantMode && drop.audienceScopeId) { const grant = await deps.keychain.createGrant({ diff --git a/src/api/server.ts b/src/api/server.ts index 016d0a9c..4244226d 100644 --- a/src/api/server.ts +++ b/src/api/server.ts @@ -192,6 +192,9 @@ async function gate( actorId: capability.actorId, scopeId: capability.scopeId, ...(capability.scopeVersion ? { scopeVersion: capability.scopeVersion } : {}), + ...(capability.botActor ? { botActor: true } : {}), + ...(capability.liveActor ? { liveActor: true } : {}), + ...(capability.members ? { members: capability.members } : {}), })) ) { sendJson(res, 403, { error: "forbidden", message: "capability scope membership has been revoked" }); diff --git a/src/auth/capability-token.ts b/src/auth/capability-token.ts index 0fa54508..0a84313c 100644 --- a/src/auth/capability-token.ts +++ b/src/auth/capability-token.ts @@ -36,6 +36,7 @@ export interface CapabilityClaims { drop?: string; memory?: { write?: ScopeId; orgWrite?: ScopeId; read: ScopeId[] }; liveActor?: boolean; + botActor?: boolean; liveAuthor?: boolean; triggered?: boolean; grants?: string[]; @@ -84,6 +85,7 @@ export async function verifyCapabilityToken( if (claims.keychainMembers !== undefined && !Array.isArray(claims.keychainMembers)) return null; if (claims.memory !== undefined && !Array.isArray(claims.memory?.read)) return null; if (claims.liveActor !== undefined && typeof claims.liveActor !== "boolean") return null; + if (claims.botActor !== undefined && typeof claims.botActor !== "boolean") return null; if (claims.liveAuthor !== undefined && typeof claims.liveAuthor !== "boolean") return null; if (claims.blob !== undefined && claims.blob?.dir !== "read" && claims.blob?.dir !== "write") return null; if (claims.drop !== undefined && typeof claims.drop !== "string") return null; diff --git a/src/core/orchestrator.ts b/src/core/orchestrator.ts index b6c4a4c5..ebcc6e46 100644 --- a/src/core/orchestrator.ts +++ b/src/core/orchestrator.ts @@ -1066,6 +1066,14 @@ export function createOrchestrator(deps: OrchestratorDeps): Orchestrator { let actorIsOrgAdmin = false; let orgMemoryWrite: ScopeId | undefined; let controlClaims: CapabilityClaims | undefined; + const scopeAttestation = { + actorId: actor.id, + scopeId, + ...(input.scopeVersion ? { scopeVersion: input.scopeVersion } : {}), + ...(conversation.publishMembers ? { members: conversation.publishMembers } : {}), + ...(liveTurn ? { liveActor: true } : {}), + ...(input.botActor ? { botActor: true } : {}), + }; if (!strictReadOnly && deps.signingSecret && deps.apiBaseUrl) { const destination = defaultDestination; connectorEnv.AGENT_API_URL = deps.apiBaseUrl; @@ -1087,16 +1095,13 @@ export function createOrchestrator(deps: OrchestratorDeps): Orchestrator { ? { ...memoryAccess, ...(orgMemoryWrite ? { orgWrite: orgMemoryWrite } : {}) } : undefined; controlClaims = { - actorId: actor.id, - scopeId, - ...(input.scopeVersion ? { scopeVersion: input.scopeVersion } : {}), + ...scopeAttestation, aud: CONTROL_PLANE_AUD, exp: Date.now() + CAPABILITY_TTL_MS, ...(turnTimezone ? { timezone: turnTimezone } : {}), ...(destination ? { destination } : {}), ...(delivery.candidates.length > 0 ? { destinations: delivery.candidates } : {}), ...(delivery.defaultKey ? { defaultDestinationKey: delivery.defaultKey } : {}), - ...(conversation.publishMembers ? { members: conversation.publishMembers } : {}), ...(conversation.kind !== "dm" ? { keychainMembers: conversation.audience.filter((p) => p.type === "internal") } : {}), @@ -1107,7 +1112,6 @@ export function createOrchestrator(deps: OrchestratorDeps): Orchestrator { ? { privateScope: true } : {}), ...(memoryClaim ? { memory: memoryClaim } : {}), - ...(liveTurn ? { liveActor: true } : {}), ...(liveAuthorTurn ? { liveAuthor: true } : {}), ...(automatedTurn ? { triggered: true } : {}), ...(!liveTurn && input.unattendedGrants ? { grants: input.unattendedGrants } : {}), @@ -1119,9 +1123,7 @@ export function createOrchestrator(deps: OrchestratorDeps): Orchestrator { ); connectorEnv.AGENT_OAUTH_CONSENT_TOKEN = await mintCapabilityToken( { - actorId: actor.id, - scopeId, - ...(input.scopeVersion ? { scopeVersion: input.scopeVersion } : {}), + ...scopeAttestation, aud: OAUTH_CONSENT_AUD, exp: Date.now() + CAPABILITY_TTL_MS, }, @@ -1144,9 +1146,7 @@ export function createOrchestrator(deps: OrchestratorDeps): Orchestrator { if (slugs.length > 0) { connectorEnv.AGENT_CREDENTIAL_TOKEN = await mintCapabilityToken( { - actorId: actor.id, - scopeId, - ...(input.scopeVersion ? { scopeVersion: input.scopeVersion } : {}), + ...scopeAttestation, aud: CREDENTIAL_BROKER_AUD, credentials: slugs, exp: Date.now() + CAPABILITY_TTL_MS, @@ -1185,8 +1185,7 @@ export function createOrchestrator(deps: OrchestratorDeps): Orchestrator { if (!strictReadOnly && egressSecret) { egressTokenForTurn = await mintCapabilityToken( { - actorId: actor.id, - scopeId, + ...scopeAttestation, aud: EGRESS_PROXY_AUD, egress: egressClaimAllowingControlPlane( resolution.egress, diff --git a/src/deploy/docker-deploy-provider.ts b/src/deploy/docker-deploy-provider.ts index a09bcd76..51615b07 100644 --- a/src/deploy/docker-deploy-provider.ts +++ b/src/deploy/docker-deploy-provider.ts @@ -80,75 +80,6 @@ export function createDockerDeployProvider(opts: DockerDeployProviderOptions = { } }; - let migrationRetryable = false; - const migrateLegacyNetworks = async (): Promise => { - const listed = await dexec([ - "network", - "inspect", - "--format", - "{{range .Containers}}{{println .Name}}{{end}}", - LEGACY_NETWORK, - ]); - migrationRetryable = listed.code === 0; - if (listed.code !== 0) return /no such network|not found/i.test(listed.stderr); - let migrated = true; - for (const container of listed.stdout - .split(/\s+/) - .filter((candidate) => /^agent-deploy-[a-zA-Z0-9_-]+$/.test(candidate))) { - try { - if (!(await migrateContainer(container))) migrated = false; - } catch { - migrated = false; - } - } - if (!migrated) return false; - const removed = await dexec(["network", "rm", LEGACY_NETWORK]); - if (removed.code === 0 || /no such network|not found/i.test(removed.stderr)) return true; - const remaining = await dexec([ - "network", - "inspect", - "--format", - "{{range .Containers}}{{println .Name}}{{end}}", - LEGACY_NETWORK, - ]); - return ( - remaining.code === 0 && - !remaining.stdout.split(/\s+/).some((candidate) => /^agent-deploy-[a-zA-Z0-9_-]+$/.test(candidate)) - ); - }; - - let migrationComplete = false; - let migrationInFlight: Promise | undefined; - let migrationRetryTimer: ReturnType | undefined; - let migrationRetryDelayMs = 1000; - const runMigration = (): Promise => { - if (migrationComplete) return Promise.resolve(true); - if (migrationInFlight) return migrationInFlight; - migrationInFlight = migrateLegacyNetworks() - .then((complete) => { - migrationComplete = complete; - if (complete) { - if (migrationRetryTimer) clearTimeout(migrationRetryTimer); - migrationRetryTimer = undefined; - migrationRetryDelayMs = 1000; - } else if (!migrationRetryTimer) { - const delay = migrationRetryable ? migrationRetryDelayMs : 30_000; - migrationRetryDelayMs = Math.min(delay * 2, 30_000); - migrationRetryTimer = setTimeout(() => { - migrationRetryTimer = undefined; - void runMigration(); - }, delay); - migrationRetryTimer.unref(); - } - return complete; - }) - .finally(() => { - migrationInFlight = undefined; - }); - return migrationInFlight; - }; - void runMigration(); - return { profile: { managedScaleToZero: false }, diff --git a/src/directory/directory-store.ts b/src/directory/directory-store.ts index 0587f340..5d6d7e0b 100644 --- a/src/directory/directory-store.ts +++ b/src/directory/directory-store.ts @@ -144,11 +144,27 @@ export function createDirectoryStore(): DirectoryStore { }, async replaceChannels(nextChannels, nextChannelMembers, syncedAt, nextChannelRosterIds) { if (!acceptSync("channels", syncedAt)) return false; + const becamePrivate = new Set( + nextChannels + .filter( + (next) => + next.isPrivate === true && + channels.some((current) => current.channelId === next.channelId && current.isPrivate !== true), + ) + .map((channel) => channel.channelId), + ); channels = nextChannels.filter((c) => c.channelId && c.name); const listed = new Set(channels.map((channel) => channel.channelId)); knownChannelRosters = knownChannelRosters ? new Set([...knownChannelRosters].filter((channelId) => listed.has(channelId))) : undefined; + for (const channelId of becamePrivate) { + channelMembers?.delete(channelId); + knownChannelRosters?.delete(channelId); + capabilityChannelMembers?.delete(channelId); + knownCapabilityChannelRosters?.delete(channelId); + capabilityChannelRevocations.delete(channelId); + } if (nextChannelMembers !== undefined) { const rosterIds = new Set(nextChannelRosterIds ?? channels.map((channel) => channel.channelId)); const byChannel = new Map( diff --git a/src/directory/postgres-directory-store.ts b/src/directory/postgres-directory-store.ts index 6b047753..2e901519 100644 --- a/src/directory/postgres-directory-store.ts +++ b/src/directory/postgres-directory-store.ts @@ -315,6 +315,34 @@ export function createPostgresDirectoryStore(connectionString: string): Director const applied = await swapIfChanged("channels_hash", hash, syncedAt, async (client) => { const channelIds = [...listedIds]; + const privateIds = list.filter((channel) => channel.isPrivate === true).map((channel) => channel.channelId); + const becamePrivate = privateIds.length + ? ( + await client.query( + "SELECT channel_id FROM directory_channels WHERE org_id = $1 AND channel_id = ANY($2::text[]) AND is_private = FALSE", + [orgId, privateIds], + ) + ).rows.map((row) => row.channel_id as string) + : []; + if (becamePrivate.length) { + await client.query( + "DELETE FROM directory_channel_members WHERE org_id = $1 AND channel_id = ANY($2::text[])", + [orgId, becamePrivate], + ); + await client.query( + "DELETE FROM directory_capability_channel_members WHERE org_id = $1 AND channel_id = ANY($2::text[])", + [orgId, becamePrivate], + ); + await client.query( + "DELETE FROM directory_capability_channels WHERE org_id = $1 AND channel_id = ANY($2::text[])", + [orgId, becamePrivate], + ); + await client.query( + "DELETE FROM directory_capability_channel_revocations WHERE org_id = $1 AND channel_id = ANY($2::text[])", + [orgId, becamePrivate], + ); + await client.query("UPDATE directory_sync SET capability_channels_hash = NULL WHERE org_id = $1", [orgId]); + } await client.query("DELETE FROM directory_channels WHERE org_id = $1 AND NOT (channel_id = ANY($2::text[]))", [ orgId, channelIds, @@ -331,7 +359,10 @@ export function createPostgresDirectoryStore(connectionString: string): Director name = EXCLUDED.name, name_lc = EXCLUDED.name_lc, is_private = EXCLUDED.is_private, - roster_known = directory_channels.roster_known OR EXCLUDED.roster_known`, + roster_known = CASE + WHEN NOT directory_channels.is_private AND EXCLUDED.is_private THEN EXCLUDED.roster_known + ELSE directory_channels.roster_known OR EXCLUDED.roster_known + END`, [ orgId, channelIds, diff --git a/src/slack/events.ts b/src/slack/events.ts index 1228c063..38f28a5f 100644 --- a/src/slack/events.ts +++ b/src/slack/events.ts @@ -40,9 +40,34 @@ export function registerSlackEvents( const { dispatch, handleReactionEvent, botHasStakeInThread } = handler; const { mirrorMessageEvent, pushSurfaceEvents } = mirror; const { syncForUnseenGroup, forceDirectorySync } = directory; + const eventIdentity = async ( + client: any, + event: { user?: string; bot_id?: string }, + ): Promise<{ userId: string; actor?: { externalId: string; isBot: true; displayName?: string } }> => { + if (event.user) return { userId: event.user }; + if (!event.bot_id) return { userId: "" }; + try { + const bot = (await client.bots.info({ bot: event.bot_id })).bot; + if (bot?.user_id) return { userId: String(bot.user_id) }; + if (bot?.id === event.bot_id && bot.deleted !== true) { + return { + userId: event.bot_id, + actor: { + externalId: event.bot_id, + isBot: true, + ...(bot.name ? { displayName: String(bot.name) } : {}), + }, + }; + } + } catch { + return { userId: event.bot_id }; + } + return { userId: event.bot_id }; + }; app.event("app_mention", async ({ event, body, client, context }: any) => { const e = event as any; + const identity = await eventIdentity(client, e); const key = dedupeKey({ event_id: (body as any)?.event_id, client_msg_id: e.client_msg_id, @@ -54,7 +79,8 @@ export function registerSlackEvents( { kind: "channel", channel: e.channel, - userId: e.user, + userId: identity.userId, + ...(identity.actor ? { actor: identity.actor } : {}), rawText: e.text ?? "", files: (e.files as SlackFile[]) ?? [], threadTs: e.thread_ts, @@ -105,6 +131,7 @@ export function registerSlackEvents( if (!shouldProcessMessage(m, ids.botUserId, ids.ownBotId)) return; if (m.channel_type === "im") { + const identity = await eventIdentity(client, m); const key = dedupeKey({ event_id: (body as any)?.event_id, client_msg_id: m.client_msg_id, @@ -116,7 +143,8 @@ export function registerSlackEvents( { kind: "dm", channel: m.channel, - userId: m.user, + userId: identity.userId, + ...(identity.actor ? { actor: identity.actor } : {}), ...(m.bot_profile?.name || m.username ? { authorName: String(m.bot_profile?.name || m.username) } : {}), rawText: m.text ?? "", files: (m.files as SlackFile[]) ?? [], @@ -150,12 +178,14 @@ export function registerSlackEvents( channel: m.channel, ts: m.ts, }); + const identity = await eventIdentity(client, m); await dispatch( key, { kind: "channel", channel: m.channel, - userId: m.user, + userId: identity.userId, + ...(identity.actor ? { actor: identity.actor } : {}), ...(m.bot_profile?.name || m.username ? { authorName: String(m.bot_profile?.name || m.username) } : {}), rawText: m.text ?? "", files: (m.files as SlackFile[]) ?? [], @@ -218,7 +248,8 @@ export function registerSlackEvents( ) ) return; - await forceDirectorySync(client, e.channel, e.user); + const principalId = e.user ? (await directory.classifyUserCached(client, e.user)).actor.externalId : undefined; + await forceDirectorySync(client, e.channel, principalId); }); app.event("reaction_added", async ({ event, body, client }: any) => { diff --git a/src/slack/identity.ts b/src/slack/identity.ts index 64149dad..f03aaf8d 100644 --- a/src/slack/identity.ts +++ b/src/slack/identity.ts @@ -184,7 +184,7 @@ export async function resolveChannelMembership(opts: { slackIdsByPrincipal?: Map; }> { const { memberIds, actor, info } = opts; - if (!memberIds.includes(opts.actorSlackId)) return { audience: [actor, externalMarker()] }; + if (!actor.isBot && !memberIds.includes(opts.actorSlackId)) return { audience: [actor, externalMarker()] }; const members: ActorAssertion[] = []; const slackIdsByPrincipal = new Map(); diff --git a/src/slack/turn-handler.ts b/src/slack/turn-handler.ts index 167407c8..76f70992 100644 --- a/src/slack/turn-handler.ts +++ b/src/slack/turn-handler.ts @@ -67,6 +67,7 @@ interface Incoming { kind: "dm" | "channel"; channel: string; userId: string; + actor?: ActorAssertion; authorName?: string; rawText: string; files: SlackFile[]; @@ -189,9 +190,14 @@ export function createTurnHandler(deps: { inc.recvWall !== undefined && inc.eventTs !== undefined ? Math.max(0, Math.round(inc.recvWall - inc.eventTs * 1000)) : undefined; - const classified = inc.prefetched - ? { actor: inc.prefetched.actor, ...(inc.prefetched.timezone ? { timezone: inc.prefetched.timezone } : {}) } - : await classifyUserCached(client, inc.userId); + let classified: { actor: ActorAssertion; timezone?: string }; + if (inc.actor) classified = { actor: inc.actor }; + else if (inc.prefetched) + classified = { + actor: inc.prefetched.actor, + ...(inc.prefetched.timezone ? { timezone: inc.prefetched.timezone } : {}), + }; + else classified = await classifyUserCached(client, inc.userId); const actor = classified.actor; const timezone = classified.timezone; const text = stripMention(inc.rawText, ids.botUserId); @@ -433,6 +439,7 @@ export function createTurnHandler(deps: { : { entryTs: inc.ts, ...(actor.isBot || inc.botAuthored ? {} : { liveActor: true }) }), } : { liveActor: true, triggerTs: inc.ts }), + ...(actor.isBot ? { botActor: true } : {}), ...(conversationHeader ? { conversationHeader } : {}), ...(priorTurns ? { priorTurns } : {}), ...(overheard ? { overheard } : {}), diff --git a/src/types.ts b/src/types.ts index 8eba624c..edeaa6db 100644 --- a/src/types.ts +++ b/src/types.ts @@ -396,6 +396,7 @@ export interface TurnRequest { ownerKeychainUnion?: boolean; unprompted?: boolean; liveActor?: boolean; + botActor?: boolean; conversationHeader?: string; priorTurns?: ConversationTurn[]; overheard?: OverheardMessage[]; diff --git a/test/capability-routes.test.ts b/test/capability-routes.test.ts index 265a0555..82fc1b50 100644 --- a/test/capability-routes.test.ts +++ b/test/capability-routes.test.ts @@ -755,4 +755,31 @@ describe("capability-token control plane (crons + SOUL)", () => { ); assert.equal((await get("/v1/soul", { "x-agent-capability": await capChannel("U8") })).status, 200); }); + + it("a live verified bot retains private-channel tools without a Slack user principal", async () => { + await built.directory.replaceChannels( + [{ channelId: "C", name: "eng", isPrivate: true }], + ["admin-alice", "U1", "U2"].map((principalId) => ({ channelId: "C", principalId })), + ); + await built.directory.replaceCapabilityChannels( + ["C"], + ["admin-alice", "U1", "U2"].map((principalId) => ({ channelId: "C", principalId })), + ["C"], + ); + const members = [{ id: "B-LEGACY", type: "internal" as const }]; + const token = await capFor("B-LEGACY", scopeId("channel", "C"), { + botActor: true, + liveActor: true, + members, + }); + assert.equal((await get("/v1/soul", { "x-agent-capability": token })).status, 200); + assert.equal( + ( + await get("/v1/soul", { + "x-agent-capability": await capFor("B-LEGACY", scopeId("channel", "C"), { members }), + }) + ).status, + 403, + ); + }); }); diff --git a/test/directory-store.test.ts b/test/directory-store.test.ts index 81b7e35b..f4d56395 100644 --- a/test/directory-store.test.ts +++ b/test/directory-store.test.ts @@ -322,4 +322,20 @@ describe("private-channel membership (authorizes private-channel sends, §10)", ); assert.equal(await d.channelCapabilityMembership("C-new", "U-new"), true); }); + + it("invalidates preserved rosters when a public channel becomes private", async () => { + const d = createDirectoryStore(); + await d.replaceChannels([{ channelId: "C-one", name: "one" }], [{ channelId: "C-one", principalId: "U-old" }]); + await d.replaceCapabilityChannels(["C-one"], [{ channelId: "C-one", principalId: "U-old" }], ["C-one"]); + await d.replaceChannels([{ channelId: "C-one", name: "one", isPrivate: true }]); + assert.equal(await d.channelMembership("C-one", "U-old"), undefined); + assert.equal(await d.channelCapabilityMembership("C-one", "U-old"), undefined); + + await d.replaceChannels([{ channelId: "C-one", name: "one" }]); + await d.replaceChannels( + [{ channelId: "C-one", name: "one", isPrivate: true }], + [{ channelId: "C-one", principalId: "U-current" }], + ); + assert.deepEqual(await d.channelMemberIds("C-one"), ["U-current"]); + }); }); diff --git a/test/docker-deploy-provider.test.ts b/test/docker-deploy-provider.test.ts index a3c0b4d4..e760bdf1 100644 --- a/test/docker-deploy-provider.test.ts +++ b/test/docker-deploy-provider.test.ts @@ -90,34 +90,16 @@ test("Docker provider migrates running deployments off the legacy shared network assert.ok(calls.some((args) => args.join(" ") === `network disconnect agent-deploynet ${containerName}`)); }); -test("Docker provider retries legacy migration after the daemon recovers", async (t) => { - t.mock.timers.enable({ apis: ["setTimeout"] }); +test("constructing a Docker provider does not inspect or migrate unrelated deployments", async () => { const calls: string[][] = []; - const container = "agent-deploy-legacy123"; - let legacyInspections = 0; const dockerExec: DockerExec = async (args) => { calls.push(args); - if (args.join(" ") === "network inspect --format {{range .Containers}}{{println .Name}}{{end}} agent-deploynet") { - legacyInspections++; - return legacyInspections === 1 - ? { code: 1, stdout: "", stderr: "daemon unavailable" } - : { code: 0, stdout: `${container}\n`, stderr: "" }; - } - if (args[0] === "inspect") { - return { code: 0, stdout: JSON.stringify({ "agent-deploynet": {} }), stderr: "" }; - } - if (args[0] === "network" && args[1] === "inspect") return { code: 1, stdout: "", stderr: "missing" }; return { code: 0, stdout: "", stderr: "" }; }; createDockerDeployProvider({ dockerExec }); await new Promise((resolve) => setImmediate(resolve)); - assert.equal(legacyInspections, 1); - t.mock.timers.tick(30_000); - await new Promise((resolve) => setImmediate(resolve)); - await new Promise((resolve) => setImmediate(resolve)); - assert.equal(legacyInspections, 2); - assert.ok(calls.some((args) => args.join(" ") === `network disconnect agent-deploynet ${container}`)); + assert.deepEqual(calls, []); }); test("an unrelated legacy migration failure does not block a new deployment", async () => { diff --git a/test/git-http-broker.test.ts b/test/git-http-broker.test.ts index 13d7ec1e..f781ade5 100644 --- a/test/git-http-broker.test.ts +++ b/test/git-http-broker.test.ts @@ -79,6 +79,7 @@ async function ctx( test("git HTTP broker streams a smart-HTTP request through the pinned service credential", async () => { let seen: { url: string; method: string; headers: Record; body: string } | undefined; + let authorized: unknown; const deps: ServerDeps = { control: {} as ServerDeps["control"], serviceCreds: { @@ -106,10 +107,34 @@ test("git HTTP broker streams a smart-HTTP request through the pinned service cr }, }; const c = await ctx("/v1/credentials/git/gitlab/acme/repo.git/git-receive-pack", "POST", deps, "PACK"); + c.req.headers["x-agent-capability"] = await mintCapabilityToken( + { + actorId: "B-LEGACY", + scopeId: "channel:C1", + aud: CREDENTIAL_BROKER_AUD, + credentials: ["gitlab"], + botActor: true, + liveActor: true, + members: [{ id: "B-LEGACY", type: "internal" }], + exp: Date.now() + CAPABILITY_TTL_MS, + }, + SECRET, + ); + c.app.authorizesCapabilityScope = async (claims) => { + authorized = claims; + return true; + }; await brokerGitHttp(c); assert.equal(await text(c.res), "0000"); assert.equal(c.res.statusCode, 200); + assert.deepEqual(authorized, { + actorId: "B-LEGACY", + scopeId: "channel:C1", + botActor: true, + liveActor: true, + members: [{ id: "B-LEGACY", type: "internal" }], + }); assert.equal(c.res.capturedHeaders?.["content-type"], "application/x-git-receive-pack-result"); assert.deepEqual(seen, { url: "https://gitlab.example/acme/repo.git/git-receive-pack", diff --git a/test/orchestrator.test.ts b/test/orchestrator.test.ts index a4520c01..f52eb9ce 100644 --- a/test/orchestrator.test.ts +++ b/test/orchestrator.test.ts @@ -435,6 +435,48 @@ test("a per-turn egress-proxy token is minted and passed to provision, carrying assert.deepEqual(captured!.egress, { allowedHosts: [], deniedHosts: [] }); }); +test("live bot attestation reaches control, OAuth, and egress capabilities", async () => { + const config = testConfig({ + dataDir: mkdtempSync(join(tmpdir(), "ap-")), + signingSecret: "test-secret", + apiBaseUrl: "https://core.example.com", + }); + const { app, sandbox } = buildApp(config); + let captured: ProvisionOptions | undefined; + const realProvision = sandbox.provision.bind(sandbox); + sandbox.provision = (layers, opts) => { + captured = opts; + return realProvision(layers, opts); + }; + const actor = { externalId: "B-LEGACY", isBot: true }; + const res = await app.turn( + channel("!run echo bot", { + actor, + botActor: true, + liveActor: true, + conversation: { + kind: "channel", + threadRef: "ch:C1:bot", + channelRef: "C1", + isPrivate: true, + audience: [actor], + publishMembers: [actor], + }, + }), + ); + assert.equal(res.status, "ok"); + for (const token of [ + captured!.env!.AGENT_API_TOKEN, + captured!.env!.AGENT_OAUTH_CONSENT_TOKEN, + captured!.egressToken, + ]) { + const claims = await verifyCapabilityToken(token!, TEST_CAPABILITY_SECRET); + assert.equal(claims?.botActor, true); + assert.equal(claims?.liveActor, true); + assert.deepEqual(claims?.members, [{ id: "B-LEGACY", type: "internal" }]); + } +}); + test("org env-delivery credentials ride provision env under their envKey — read live, so a rotation applies next turn", async () => { const config = testConfig({ dataDir: mkdtempSync(join(tmpdir(), "ap-")), diff --git a/test/postgres-directory-store.test.ts b/test/postgres-directory-store.test.ts index de5f4356..a485bc4d 100644 --- a/test/postgres-directory-store.test.ts +++ b/test/postgres-directory-store.test.ts @@ -428,6 +428,28 @@ test("pg directory: capability rosters survive legacy channel-directory swaps", assert.equal(await store.channelCapabilityMembership("C-new", "U-new"), true); }); +test("pg directory: public-to-private transitions invalidate preserved rosters", { skip }, async () => { + const store = createPostgresDirectoryStore(URL!); + await store.replaceChannels( + [{ channelId: "C-transition", name: "transition" }], + [{ channelId: "C-transition", principalId: "U-old" }], + ); + await store.replaceCapabilityChannels( + ["C-transition"], + [{ channelId: "C-transition", principalId: "U-old" }], + ["C-transition"], + ); + await store.replaceChannels([{ channelId: "C-transition", name: "transition", isPrivate: true }]); + assert.equal(await store.channelMembership("C-transition", "U-old"), undefined); + assert.equal(await store.channelCapabilityMembership("C-transition", "U-old"), undefined); + await store.replaceCapabilityChannels( + ["C-transition"], + [{ channelId: "C-transition", principalId: "U-old" }], + ["C-transition"], + ); + assert.equal(await store.channelCapabilityMembership("C-transition", "U-old"), true); +}); + test("pg directory: a partial group swap preserves unknown rosters", { skip }, async () => { const store = createPostgresDirectoryStore(URL!); await store.replaceGroups( diff --git a/test/secret-drop.test.ts b/test/secret-drop.test.ts index 92baad5f..81729deb 100644 --- a/test/secret-drop.test.ts +++ b/test/secret-drop.test.ts @@ -514,6 +514,31 @@ describe("/v1/keychain/drops — mint, form, redeem", async () => { assert.match(html, /location\.search/, "the submit fetch carries location.search"); assert.ok(!html.includes(t), "the token itself is never embedded in the page body"); }); + + it("a live bot attestation survives mint and private-scope redemption", async () => { + await built.directory.replaceChannels( + [{ channelId: "C1", name: "drops", isPrivate: true }], + [{ channelId: "C1", principalId: "U_A" }], + ); + await built.directory.replaceCapabilityChannels(["C1"], [{ channelId: "C1", principalId: "U_A" }], ["C1"]); + const members = [{ id: "B-LEGACY", type: "internal" as const }]; + const minted = await post( + "/v1/keychain/drops", + { service: "botsvc", purpose: "bot credential" }, + await capFor("B-LEGACY", scopeId("channel", "C1"), { + botActor: true, + liveActor: true, + members, + }), + ); + assert.equal(minted.status, 200); + const { dropId, formPath } = (await minted.json()) as { dropId: string; formPath: string }; + const token = await verifyCapabilityToken(linkToken(formPath)!, SECRET); + assert.equal(token?.botActor, true); + assert.equal(token?.liveActor, true); + assert.deepEqual(token?.members, members); + assert.equal((await redeem(dropId, { secret: "bot-secret" }, "B-LEGACY", linkToken(formPath))).status, 200); + }); }); describe("/v1/keychain/drops — sibling-aware resume", () => { diff --git a/test/service-credential-route.test.ts b/test/service-credential-route.test.ts index cb6c40e4..6476d2b5 100644 --- a/test/service-credential-route.test.ts +++ b/test/service-credential-route.test.ts @@ -1010,6 +1010,27 @@ test("orchestrator stamps AGENT_CREDENTIAL_TOKEN with an org-wide credential's s const claims = await verifyCapabilityToken(token!, TEST_CAPABILITY_SECRET); assert.equal(claims?.aud, CREDENTIAL_BROKER_AUD); assert.deepEqual(claims?.credentials, ["x-firehose"]); + + const actor = { externalId: "B-LEGACY", isBot: true }; + await built.app.turn({ + surface: "slack", + actor, + botActor: true, + liveActor: true, + conversation: { + kind: "channel", + threadRef: "ch:C1:bot", + channelRef: "C1", + isPrivate: true, + audience: [actor], + publishMembers: [actor], + }, + text: "!run echo bot", + }); + const botClaims = await verifyCapabilityToken(env()!.AGENT_CREDENTIAL_TOKEN!, TEST_CAPABILITY_SECRET); + assert.equal(botClaims?.botActor, true); + assert.equal(botClaims?.liveActor, true); + assert.deepEqual(botClaims?.members, [{ id: "B-LEGACY", type: "internal" }]); }); test("orchestrator does NOT stamp a credential granted only to someone else", async () => { diff --git a/test/slack-index.integration.test.ts b/test/slack-index.integration.test.ts index e3a166f0..56a327f3 100644 --- a/test/slack-index.integration.test.ts +++ b/test/slack-index.integration.test.ts @@ -24,6 +24,7 @@ class FakeSlackClient { readonly messagesByChannel = new Map(); readonly membershipFailures = new Set(); readonly membershipListings = new Map(); + readonly botsById = new Map(); groupListings = 0; failGroupListing = false; private postSequence = 0; @@ -117,7 +118,7 @@ class FakeSlackClient { uploadV2: async () => ({ ok: true }), info: async () => ({ file: {} }), }; - readonly bots = { info: async () => ({ bot: {} }) }; + readonly bots = { info: async ({ bot }: { bot: string }) => ({ bot: this.botsById.get(bot) }) }; async *paginate(method: string, args: any): AsyncGenerator { if (method === "users.list") { @@ -324,14 +325,16 @@ async function waitFor(cond: () => boolean, timeoutMs = 2000): Promise { } } -async function fixture(options: { externalParticipants?: boolean; webUiPublicUrl?: string } = {}) { +async function fixture( + options: { externalParticipants?: boolean; webUiPublicUrl?: string; identityEmail?: "0" | "1" } = {}, +) { const core = new FakeCore(); core.externalParticipants = options.externalParticipants ?? false; const started = startSlackPlugin( { botToken: "xoxb-test", appToken: "xapp-test", - identityEmail: "0", + identityEmail: options.identityEmail ?? "0", ...(options.webUiPublicUrl ? { webUiPublicUrl: options.webUiPublicUrl } : {}), }, core, @@ -518,6 +521,21 @@ test("a failed refresh after a leave event revokes only the departing member", a } }); +test("a failed email-mode refresh revokes the departing canonical principal", async () => { + const f = await fixture({ identityEmail: "1" }); + try { + f.client.membershipFailures.add("CPX"); + const pushes = f.core.directories.length; + await f.app.emitEvent("member_left_channel", { user: "U1", channel: "CPX", event_ts: "100.7" }); + await waitFor(() => f.core.directories.length > pushes); + assert.deepEqual(f.core.directories.at(-1).capabilityChannelRevocations, [ + { channelId: "CPX", principalId: "alice@example.com" }, + ]); + } finally { + await f.stop(); + } +}); + test("Slack Connect directory rosters contain only internal principals", async () => { const f = await fixture({ externalParticipants: true }); try { @@ -761,6 +779,50 @@ test("a bot-authored mention can become a turn", async () => { } }); +test("a bot-authored mention without a user resolves its bot principal", async () => { + const f = await fixture(); + try { + f.client.usersById.set("B1", { + id: "B1", + team_id: "T1", + is_bot: true, + name: "peerbot", + profile: { display_name: "Peer Bot" }, + }); + f.client.botsById.set("B-PEER", { id: "B-PEER", user_id: "B1", name: "Peer Bot" }); + f.client.membersByChannel.set("C1", ["U1", "U2", "B1", "UBOT"]); + await f.app.emitEvent("app_mention", { + channel: "C1", + channel_type: "channel", + bot_id: "B-PEER", + text: "<@UBOT> hello", + ts: "102.25", + }); + assert.equal(f.core.turns.length, 1); + assert.equal(f.core.turns[0].actor.externalId, "B1"); + } finally { + await f.stop(); + } +}); + +test("a verified legacy bot without a user principal can become a turn", async () => { + const f = await fixture(); + try { + f.client.botsById.set("B-LEGACY", { id: "B-LEGACY", name: "Legacy Bot" }); + await f.app.emitEvent("app_mention", { + channel: "C1", + channel_type: "channel", + bot_id: "B-LEGACY", + text: "<@UBOT> hello", + ts: "102.26", + }); + assert.equal(f.core.turns.length, 1); + assert.equal(f.core.turns[0].actor.externalId, "B-LEGACY"); + } finally { + await f.stop(); + } +}); + test("a bot-authored stop can abort a live run", async () => { const f = await fixture(); try { diff --git a/test/surface-context.test.ts b/test/surface-context.test.ts index ed7b813a..e94c890c 100644 --- a/test/surface-context.test.ts +++ b/test/surface-context.test.ts @@ -251,7 +251,13 @@ describe("surface-context pulls", async () => { const asking = post( "/v1/surface-file", { channel: "#eng", ts: "1699.5", name: "wave.png" }, - { "x-agent-capability": await cap() }, + { + "x-agent-capability": await cap({ + botActor: true, + liveActor: true, + members: [{ id: "U1", type: "internal" }], + }), + }, ); const query = await fulfillNext(() => ({ file: { blobId: "blob-42", name: "wave.png", sizeBytes: 3, mimetype: "image/png", author: "Alice" }, @@ -268,6 +274,9 @@ describe("surface-context pulls", async () => { assert.ok(token, "the download token verifies against the core secret"); assert.equal(token!.aud, "blob-transfer"); assert.deepEqual(token!.blob, { dir: "read", id: "blob-42" }, "the token moves this one blob, read-only"); + assert.equal(token!.botActor, true); + assert.equal(token!.liveActor, true); + assert.deepEqual(token!.members, [{ id: "U1", type: "internal" }]); }); it("a current-conversation file pull rides the token's opaque target and passes threadTs through", async () => { From c122135a2a8526c2fbcc32225e9302607642b86a Mon Sep 17 00:00:00 2001 From: Josh France <12610835+16francej@users.noreply.github.com> Date: Sun, 16 Aug 2026 11:53:19 -0400 Subject: [PATCH 07/10] Simplify directory synchronization --- src/api/app-helpers.ts | 2 +- src/api/app-messaging.ts | 14 +- src/api/app-types.ts | 8 +- src/api/routes/directory.ts | 36 ++--- src/api/slack-core-client.ts | 18 +-- src/directory/directory-store.ts | 84 ++-------- src/directory/postgres-directory-store.ts | 187 +++++----------------- src/slack/directory.ts | 157 +++++++++--------- src/slack/events.ts | 6 +- test/capability-routes.test.ts | 15 +- test/directory-store.test.ts | 62 ++----- test/postgres-directory-store.test.ts | 70 ++------ test/projects.test.ts | 16 -- test/secret-drop.test.ts | 1 - test/slack-index.integration.test.ts | 29 ++-- 15 files changed, 210 insertions(+), 495 deletions(-) diff --git a/src/api/app-helpers.ts b/src/api/app-helpers.ts index cb29b8de..3bd8fd4d 100644 --- a/src/api/app-helpers.ts +++ b/src/api/app-helpers.ts @@ -408,7 +408,7 @@ export function createAppHelpers(deps: AppDeps, app: App) { const privateChannel = kind === "channel" && (await deps.directory.channelPrivacy?.(ref).catch(() => undefined)) === true; const capabilityMembership = privateChannel - ? await deps.directory.channelCapabilityMembership(ref, claims.actorId).catch(() => undefined) + ? await deps.directory.channelMembership(ref, claims.actorId).catch(() => undefined) : undefined; const attestedBot = privateChannel && diff --git a/src/api/app-messaging.ts b/src/api/app-messaging.ts index 6458f4e9..0743bf54 100644 --- a/src/api/app-messaging.ts +++ b/src/api/app-messaging.ts @@ -59,7 +59,6 @@ export function createMessagingMethods( | "setRunDeliveryState" | "upsertDirectory" | "upsertChannels" - | "upsertCapabilityChannels" | "upsertGroups" | "setDirectoryWorkspaceUrl" | "directoryMeta" @@ -340,22 +339,13 @@ export function createMessagingMethods( }); } }, - async upsertChannels(channels, channelMembers, syncedAt, channelRosterIds) { - await deps.directory.replaceChannels(channels, channelMembers, syncedAt, channelRosterIds); + async upsertChannels(channels, channelMembers, syncedAt, channelRosterIds, revocations) { + await deps.directory.replaceChannels(channels, channelMembers, syncedAt, channelRosterIds, revocations); await h.syncLinkedProjectRosters(); }, async upsertGroups(groupMembers, syncedAt, groupIds, groupRosterIds) { await deps.directory.replaceGroups(groupMembers, syncedAt, groupIds, groupRosterIds); }, - async upsertCapabilityChannels(channelIds, channelMembers, channelRosterIds, syncedAt, revocations) { - await deps.directory.replaceCapabilityChannels( - channelIds, - channelMembers, - channelRosterIds, - syncedAt, - revocations, - ); - }, async setDirectoryWorkspaceUrl(url) { await deps.directory.setWorkspaceUrl(url); }, diff --git a/src/api/app-types.ts b/src/api/app-types.ts index 5bae2bbb..3cd94c55 100644 --- a/src/api/app-types.ts +++ b/src/api/app-types.ts @@ -393,6 +393,7 @@ export interface App { channelMembers?: ChannelMembership[], syncedAt?: number, channelRosterIds?: string[], + revocations?: ChannelMembership[], ): Promise; upsertGroups( groupMembers: GroupMembership[], @@ -400,13 +401,6 @@ export interface App { groupIds?: string[], groupRosterIds?: string[], ): Promise; - upsertCapabilityChannels( - channelIds: string[], - channelMembers: ChannelMembership[], - channelRosterIds: string[], - syncedAt?: number, - revocations?: ChannelMembership[], - ): Promise; setDirectoryWorkspaceUrl(url: string): Promise; directoryMeta(): Promise; resolveRecipient(query: string): Promise; diff --git a/src/api/routes/directory.ts b/src/api/routes/directory.ts index 88b7d639..aadab8bf 100644 --- a/src/api/routes/directory.ts +++ b/src/api/routes/directory.ts @@ -33,9 +33,7 @@ async function pushDirectory(ctx: ApiCtx): Promise { channels?: unknown; channelMembers?: unknown; channelRosterIds?: unknown; - capabilityChannelMembers?: unknown; - capabilityChannelRosterIds?: unknown; - capabilityChannelRevocations?: unknown; + channelRevocations?: unknown; groupMembers?: unknown; groupIds?: unknown; groupRosterIds?: unknown; @@ -75,7 +73,7 @@ async function pushDirectory(ctx: ApiCtx): Promise { let channelCount: number | undefined; if (Array.isArray(b.channels)) { const channels = b.channels.filter( - (c): c is { channelId: string; name: string; isPrivate?: boolean } => + (c): c is { channelId: string; name: string; isPrivate?: boolean; isExternal?: boolean } => isObj(c) && typeof c.channelId === "string" && typeof c.name === "string", ); const channelMembers = Array.isArray(b.channelMembers) @@ -87,31 +85,19 @@ async function pushDirectory(ctx: ApiCtx): Promise { const channelRosterIds = Array.isArray(b.channelRosterIds) ? b.channelRosterIds.filter((channelId): channelId is string => typeof channelId === "string") : undefined; - await app.upsertChannels(channels, channelMembers, numOrUndef(b.channelsSyncedAt), channelRosterIds); - const capabilityChannelMembers = Array.isArray(b.capabilityChannelMembers) - ? b.capabilityChannelMembers.filter( + const channelRevocations = Array.isArray(b.channelRevocations) + ? b.channelRevocations.filter( (m): m is { channelId: string; principalId: string } => isObj(m) && typeof m.channelId === "string" && typeof m.principalId === "string", ) : undefined; - const capabilityChannelRosterIds = Array.isArray(b.capabilityChannelRosterIds) - ? b.capabilityChannelRosterIds.filter((channelId): channelId is string => typeof channelId === "string") - : undefined; - const capabilityChannelRevocations = Array.isArray(b.capabilityChannelRevocations) - ? b.capabilityChannelRevocations.filter( - (m): m is { channelId: string; principalId: string } => - isObj(m) && typeof m.channelId === "string" && typeof m.principalId === "string", - ) - : undefined; - if (capabilityChannelMembers && capabilityChannelRosterIds) { - await app.upsertCapabilityChannels( - channels.map((channel) => channel.channelId), - capabilityChannelMembers, - capabilityChannelRosterIds, - numOrUndef(b.channelsSyncedAt), - capabilityChannelRevocations, - ); - } + await app.upsertChannels( + channels, + channelMembers, + numOrUndef(b.channelsSyncedAt), + channelRosterIds, + channelRevocations, + ); channelCount = channels.length; } let groupMemberCount: number | undefined; diff --git a/src/api/slack-core-client.ts b/src/api/slack-core-client.ts index 170fcc51..7673e6c0 100644 --- a/src/api/slack-core-client.ts +++ b/src/api/slack-core-client.ts @@ -45,12 +45,10 @@ interface StoredApprovalView { interface DirectoryPush { members?: Array<{ principalId: string; displayName: string; type: "internal"; slackId?: string }>; - channels?: Array<{ channelId: string; name: string; isPrivate?: boolean }>; + channels?: Array<{ channelId: string; name: string; isPrivate?: boolean; isExternal?: boolean }>; channelMembers?: Array<{ channelId: string; principalId: string }>; channelRosterIds?: string[]; - capabilityChannelMembers?: Array<{ channelId: string; principalId: string }>; - capabilityChannelRosterIds?: string[]; - capabilityChannelRevocations?: Array<{ channelId: string; principalId: string }>; + channelRevocations?: Array<{ channelId: string; principalId: string }>; groupMembers?: Array<{ groupId: string; principalId: string }>; groupIds?: string[]; groupRosterIds?: string[]; @@ -309,14 +307,12 @@ export function createSlackCoreClient(deps: SlackCoreClientDeps): SlackCoreClien if (body.workspaceUrl) await deps.app.setDirectoryWorkspaceUrl(body.workspaceUrl); if (body.members) await deps.app.upsertDirectory(body.members, body.membersSyncedAt); if (body.channels) - await deps.app.upsertChannels(body.channels, body.channelMembers, body.channelsSyncedAt, body.channelRosterIds); - if (body.channels && body.capabilityChannelMembers && body.capabilityChannelRosterIds) - await deps.app.upsertCapabilityChannels( - body.channels.map((channel) => channel.channelId), - body.capabilityChannelMembers, - body.capabilityChannelRosterIds, + await deps.app.upsertChannels( + body.channels, + body.channelMembers, body.channelsSyncedAt, - body.capabilityChannelRevocations, + body.channelRosterIds, + body.channelRevocations, ); if (body.groupMembers) await deps.app.upsertGroups(body.groupMembers, body.groupsSyncedAt, body.groupIds, body.groupRosterIds); diff --git a/src/directory/directory-store.ts b/src/directory/directory-store.ts index 5d6d7e0b..6e128c50 100644 --- a/src/directory/directory-store.ts +++ b/src/directory/directory-store.ts @@ -12,6 +12,7 @@ export interface DirectoryChannel { channelId: string; name: string; isPrivate?: boolean; + isExternal?: boolean; } export interface ChannelMembership { @@ -47,6 +48,7 @@ export interface DirectoryStore { channelMembers?: ChannelMembership[], syncedAt?: number, channelRosterIds?: string[], + revocations?: ChannelMembership[], ): Promise; list(): Promise; listChannels(): Promise; @@ -63,14 +65,6 @@ export interface DirectoryStore { groupIds?: string[], groupRosterIds?: string[], ): Promise; - replaceCapabilityChannels( - channelIds: string[], - channelMembers: ChannelMembership[], - channelRosterIds: string[], - syncedAt?: number, - revocations?: ChannelMembership[], - ): Promise; - channelCapabilityMembership(channelId: string, principalId: string): Promise; upsertGroup(groupId: string, principalIds: readonly string[]): Promise; resolveGroupByParticipants(participants: readonly string[]): Promise; groupMember(groupId: string, principalId: string): Promise; @@ -109,9 +103,6 @@ export function createDirectoryStore(): DirectoryStore { let channels: DirectoryChannel[] = []; let channelMembers: Map> | undefined; let knownChannelRosters: Set | undefined; - let capabilityChannelMembers: Map> | undefined; - let knownCapabilityChannelRosters: Set | undefined; - let capabilityChannelRevocations = new Map>(); let groupMembers: Map> | undefined; let listedGroupIds: Set | undefined; let knownGroupRosters: Set | undefined; @@ -142,29 +133,13 @@ export function createDirectoryStore(): DirectoryStore { members = next.filter((m) => m.principalId && m.type === "internal"); return true; }, - async replaceChannels(nextChannels, nextChannelMembers, syncedAt, nextChannelRosterIds) { + async replaceChannels(nextChannels, nextChannelMembers, syncedAt, nextChannelRosterIds, revocations = []) { if (!acceptSync("channels", syncedAt)) return false; - const becamePrivate = new Set( - nextChannels - .filter( - (next) => - next.isPrivate === true && - channels.some((current) => current.channelId === next.channelId && current.isPrivate !== true), - ) - .map((channel) => channel.channelId), - ); channels = nextChannels.filter((c) => c.channelId && c.name); const listed = new Set(channels.map((channel) => channel.channelId)); knownChannelRosters = knownChannelRosters ? new Set([...knownChannelRosters].filter((channelId) => listed.has(channelId))) : undefined; - for (const channelId of becamePrivate) { - channelMembers?.delete(channelId); - knownChannelRosters?.delete(channelId); - capabilityChannelMembers?.delete(channelId); - knownCapabilityChannelRosters?.delete(channelId); - capabilityChannelRevocations.delete(channelId); - } if (nextChannelMembers !== undefined) { const rosterIds = new Set(nextChannelRosterIds ?? channels.map((channel) => channel.channelId)); const byChannel = new Map( @@ -179,9 +154,15 @@ export function createDirectoryStore(): DirectoryStore { knownChannelRosters ??= new Set(); for (const channelId of rosterIds) if (listed.has(channelId)) knownChannelRosters.add(channelId); } + for (const member of revocations) { + if (listed.has(member.channelId) && member.principalId) + channelMembers?.get(member.channelId)?.delete(member.principalId); + } return true; }, async channelMember(channelId, principalId) { + if (channels.some((channel) => channel.channelId === channelId && channel.isPrivate && channel.isExternal)) + return false; return channelMembers?.get(channelId)?.has(principalId) ?? false; }, async channelMemberIds(channelId) { @@ -198,47 +179,6 @@ export function createDirectoryStore(): DirectoryStore { const channel = channels.find((candidate) => candidate.channelId === channelId); return channel ? channel.isPrivate === true : undefined; }, - async replaceCapabilityChannels(channelIds, nextChannelMembers, nextChannelRosterIds, syncedAt, revocations = []) { - if (!acceptSync("capabilityChannels", syncedAt)) return false; - const listed = new Set(channelIds.filter(Boolean)); - const rosterIds = new Set(nextChannelRosterIds.filter((channelId) => listed.has(channelId))); - knownCapabilityChannelRosters = knownCapabilityChannelRosters - ? new Set([...knownCapabilityChannelRosters].filter((channelId) => listed.has(channelId))) - : new Set(); - capabilityChannelRevocations = new Map( - [...capabilityChannelRevocations].filter(([channelId]) => listed.has(channelId)), - ); - const byChannel = new Map( - [...(capabilityChannelMembers ?? [])].filter( - ([channelId]) => listed.has(channelId) && !rosterIds.has(channelId), - ), - ); - for (const channelId of rosterIds) byChannel.set(channelId, new Set()); - for (const member of nextChannelMembers) { - if (!member.principalId || !rosterIds.has(member.channelId)) continue; - (byChannel.get(member.channelId) ?? byChannel.set(member.channelId, new Set()).get(member.channelId)!).add( - member.principalId, - ); - } - capabilityChannelMembers = byChannel; - for (const channelId of rosterIds) { - knownCapabilityChannelRosters.add(channelId); - capabilityChannelRevocations.delete(channelId); - } - for (const member of revocations) { - if (!listed.has(member.channelId) || !member.principalId) continue; - const revoked = capabilityChannelRevocations.get(member.channelId) ?? new Set(); - revoked.add(member.principalId); - capabilityChannelRevocations.set(member.channelId, revoked); - capabilityChannelMembers.get(member.channelId)?.delete(member.principalId); - } - return true; - }, - async channelCapabilityMembership(channelId, principalId) { - if (capabilityChannelRevocations.get(channelId)?.has(principalId)) return false; - if (!knownCapabilityChannelRosters?.has(channelId)) return undefined; - return capabilityChannelMembers?.get(channelId)?.has(principalId) ?? false; - }, async replaceGroups(nextGroupMembers, syncedAt, nextGroupIds, nextGroupRosterIds) { if (!acceptSync("groups", syncedAt)) return false; const legacy = nextGroupIds === undefined || nextGroupRosterIds === undefined; @@ -300,7 +240,11 @@ export function createDirectoryStore(): DirectoryStore { return [...memberships].filter(([, members]) => members.has(principalId)).map(([groupId]) => groupId); }, async listChannelsFor(principalId) { - return channels.filter((c) => !c.isPrivate || (channelMembers?.get(c.channelId)?.has(principalId) ?? false)); + return channels.filter( + (channel) => + !channel.isPrivate || + (!channel.isExternal && (channelMembers?.get(channel.channelId)?.has(principalId) ?? false)), + ); }, async list() { return members; diff --git a/src/directory/postgres-directory-store.ts b/src/directory/postgres-directory-store.ts index 2e901519..e44b7082 100644 --- a/src/directory/postgres-directory-store.ts +++ b/src/directory/postgres-directory-store.ts @@ -42,6 +42,7 @@ const SCHEMA = [ name TEXT NOT NULL, name_lc TEXT NOT NULL, is_private BOOLEAN NOT NULL DEFAULT FALSE, + is_external BOOLEAN NOT NULL DEFAULT FALSE, roster_known BOOLEAN NOT NULL DEFAULT FALSE, PRIMARY KEY (org_id, channel_id) )`, @@ -55,23 +56,6 @@ const SCHEMA = [ principal_id TEXT NOT NULL, PRIMARY KEY (org_id, channel_id, principal_id) )`, - `CREATE TABLE IF NOT EXISTS directory_capability_channels( - org_id TEXT NOT NULL, - channel_id TEXT NOT NULL, - PRIMARY KEY (org_id, channel_id) - )`, - `CREATE TABLE IF NOT EXISTS directory_capability_channel_members( - org_id TEXT NOT NULL, - channel_id TEXT NOT NULL, - principal_id TEXT NOT NULL, - PRIMARY KEY (org_id, channel_id, principal_id) - )`, - `CREATE TABLE IF NOT EXISTS directory_capability_channel_revocations( - org_id TEXT NOT NULL, - channel_id TEXT NOT NULL, - principal_id TEXT NOT NULL, - PRIMARY KEY (org_id, channel_id, principal_id) - )`, `CREATE TABLE IF NOT EXISTS directory_group_members( org_id TEXT NOT NULL, group_id TEXT NOT NULL, @@ -115,8 +99,7 @@ const SCHEMA = [ `ALTER TABLE directory_sync ADD COLUMN IF NOT EXISTS members_synced_at BIGINT`, `ALTER TABLE directory_sync ADD COLUMN IF NOT EXISTS channels_synced_at BIGINT`, `ALTER TABLE directory_sync ADD COLUMN IF NOT EXISTS groups_synced_at BIGINT`, - `ALTER TABLE directory_sync ADD COLUMN IF NOT EXISTS capability_channels_hash TEXT`, - `ALTER TABLE directory_sync ADD COLUMN IF NOT EXISTS capability_channels_synced_at BIGINT`, + `ALTER TABLE directory_channels ADD COLUMN IF NOT EXISTS is_external BOOLEAN NOT NULL DEFAULT FALSE`, `DO $$ BEGIN IF NOT EXISTS ( @@ -145,7 +128,12 @@ function memberRow(r: Record): DirectoryMember { } const MEMBER_COLS = "principal_id, display_name, type, slack_id"; function channelRow(r: Record): DirectoryChannel { - return { channelId: r.channel_id as string, name: r.name as string, isPrivate: r.is_private as boolean }; + return { + channelId: r.channel_id as string, + name: r.name as string, + isPrivate: r.is_private as boolean, + ...(r.is_external ? { isExternal: true } : {}), + }; } function hashRoster(rows: string[]): string { @@ -289,7 +277,7 @@ export function createPostgresDirectoryStore(connectionString: string): Director }); }, - async replaceChannels(channels, channelMembers, syncedAt, channelRosterIds) { + async replaceChannels(channels, channelMembers, syncedAt, channelRosterIds, revocations = []) { const byId = new Map(); for (const c of channels) if (c.channelId && c.name) byId.set(c.channelId, c); const list = [...byId.values()]; @@ -303,46 +291,20 @@ export function createPostgresDirectoryStore(connectionString: string): Director channelMembers === undefined ? undefined : dedupMemberships(channelMembers).filter((member) => rosterIds!.has(member.channelId)); - const channelsPart = list.map((c) => `${c.channelId}|${c.name}|${c.isPrivate ? 1 : 0}`); + const revokedRows = dedupMemberships(revocations).filter((member) => listedIds.has(member.channelId)); + const channelsPart = list.map((c) => `${c.channelId}|${c.name}|${c.isPrivate ? 1 : 0}|${c.isExternal ? 1 : 0}`); const membersPart = membershipRows === undefined ? [] : [ ...[...rosterIds!].map((channelId) => `r:${channelId}`), ...membershipRows.map((m) => `m:${m.channelId}|${m.principalId}`), + ...revokedRows.map((m) => `x:${m.channelId}|${m.principalId}`), ]; const hash = hashRoster([...channelsPart, ...membersPart]); const applied = await swapIfChanged("channels_hash", hash, syncedAt, async (client) => { const channelIds = [...listedIds]; - const privateIds = list.filter((channel) => channel.isPrivate === true).map((channel) => channel.channelId); - const becamePrivate = privateIds.length - ? ( - await client.query( - "SELECT channel_id FROM directory_channels WHERE org_id = $1 AND channel_id = ANY($2::text[]) AND is_private = FALSE", - [orgId, privateIds], - ) - ).rows.map((row) => row.channel_id as string) - : []; - if (becamePrivate.length) { - await client.query( - "DELETE FROM directory_channel_members WHERE org_id = $1 AND channel_id = ANY($2::text[])", - [orgId, becamePrivate], - ); - await client.query( - "DELETE FROM directory_capability_channel_members WHERE org_id = $1 AND channel_id = ANY($2::text[])", - [orgId, becamePrivate], - ); - await client.query( - "DELETE FROM directory_capability_channels WHERE org_id = $1 AND channel_id = ANY($2::text[])", - [orgId, becamePrivate], - ); - await client.query( - "DELETE FROM directory_capability_channel_revocations WHERE org_id = $1 AND channel_id = ANY($2::text[])", - [orgId, becamePrivate], - ); - await client.query("UPDATE directory_sync SET capability_channels_hash = NULL WHERE org_id = $1", [orgId]); - } await client.query("DELETE FROM directory_channels WHERE org_id = $1 AND NOT (channel_id = ANY($2::text[]))", [ orgId, channelIds, @@ -353,22 +315,21 @@ export function createPostgresDirectoryStore(connectionString: string): Director ); if (list.length) { await client.query( - `INSERT INTO directory_channels (org_id, channel_id, name, name_lc, is_private, roster_known) - SELECT $1, * FROM unnest($2::text[], $3::text[], $4::text[], $5::boolean[], $6::boolean[]) + `INSERT INTO directory_channels (org_id, channel_id, name, name_lc, is_private, is_external, roster_known) + SELECT $1, * FROM unnest($2::text[], $3::text[], $4::text[], $5::boolean[], $6::boolean[], $7::boolean[]) ON CONFLICT (org_id, channel_id) DO UPDATE SET name = EXCLUDED.name, name_lc = EXCLUDED.name_lc, is_private = EXCLUDED.is_private, - roster_known = CASE - WHEN NOT directory_channels.is_private AND EXCLUDED.is_private THEN EXCLUDED.roster_known - ELSE directory_channels.roster_known OR EXCLUDED.roster_known - END`, + is_external = EXCLUDED.is_external, + roster_known = directory_channels.roster_known OR EXCLUDED.roster_known`, [ orgId, channelIds, list.map((c) => c.name), list.map((c) => normDirectoryQuery(c.name)), list.map((c) => !!c.isPrivate), + list.map((c) => !!c.isExternal), list.map((c) => rosterIds?.has(c.channelId) ?? false), ], ); @@ -386,6 +347,15 @@ export function createPostgresDirectoryStore(connectionString: string): Director ); } } + if (revokedRows.length) { + await client.query( + `DELETE FROM directory_channel_members member + USING unnest($2::text[], $3::text[]) AS revoked(channel_id, principal_id) + WHERE member.org_id = $1 AND member.channel_id = revoked.channel_id + AND member.principal_id = revoked.principal_id`, + [orgId, revokedRows.map((m) => m.channelId), revokedRows.map((m) => m.principalId)], + ); + } }); if (applied && membershipRows !== undefined) { await q("UPDATE directory_sync SET channel_members_synced = TRUE WHERE org_id = $1", [orgId]); @@ -408,7 +378,10 @@ export function createPostgresDirectoryStore(connectionString: string): Director async channelMember(channelId, principalId) { const rows = await q( - "SELECT 1 FROM directory_channel_members WHERE org_id = $1 AND channel_id = $2 AND principal_id = $3 LIMIT 1", + `SELECT 1 FROM directory_channel_members member + JOIN directory_channels channel USING (org_id, channel_id) + WHERE member.org_id = $1 AND member.channel_id = $2 AND member.principal_id = $3 + AND NOT (channel.is_private AND channel.is_external) LIMIT 1`, [orgId, channelId, principalId], ); return rows.length > 0; @@ -450,93 +423,6 @@ export function createPostgresDirectoryStore(connectionString: string): Director return rows.length > 0 ? (rows[0]!.is_private as boolean) : undefined; }, - async replaceCapabilityChannels(channelIds, channelMembers, channelRosterIds, syncedAt, revocations = []) { - const listedIds = [...new Set(channelIds.filter(Boolean))]; - const listed = new Set(listedIds); - const rosterIds = [...new Set(channelRosterIds.filter((channelId) => listed.has(channelId)))]; - const rostered = new Set(rosterIds); - const membershipRows = dedupMemberships(channelMembers).filter((member) => rostered.has(member.channelId)); - const revokedRows = dedupMemberships(revocations).filter((member) => listed.has(member.channelId)); - const hash = hashRoster([ - ...listedIds.map((channelId) => `c:${channelId}`), - ...rosterIds.map((channelId) => `r:${channelId}`), - ...membershipRows.map((member) => `m:${member.channelId}|${member.principalId}`), - ...revokedRows.map((member) => `x:${member.channelId}|${member.principalId}`), - ]); - return swapIfChanged("capability_channels_hash", hash, syncedAt, async (client) => { - await client.query( - "DELETE FROM directory_capability_channel_members WHERE org_id = $1 AND NOT (channel_id = ANY($2::text[]))", - [orgId, listedIds], - ); - await client.query( - "DELETE FROM directory_capability_channels WHERE org_id = $1 AND NOT (channel_id = ANY($2::text[]))", - [orgId, listedIds], - ); - await client.query( - "DELETE FROM directory_capability_channel_revocations WHERE org_id = $1 AND NOT (channel_id = ANY($2::text[]))", - [orgId, listedIds], - ); - await client.query( - "DELETE FROM directory_capability_channel_members WHERE org_id = $1 AND channel_id = ANY($2::text[])", - [orgId, rosterIds], - ); - await client.query( - "DELETE FROM directory_capability_channel_revocations WHERE org_id = $1 AND channel_id = ANY($2::text[])", - [orgId, rosterIds], - ); - if (rosterIds.length) { - await client.query( - `INSERT INTO directory_capability_channels (org_id, channel_id) - SELECT $1, * FROM unnest($2::text[]) - ON CONFLICT (org_id, channel_id) DO NOTHING`, - [orgId, rosterIds], - ); - } - if (membershipRows.length) { - await client.query( - `INSERT INTO directory_capability_channel_members (org_id, channel_id, principal_id) - SELECT $1, * FROM unnest($2::text[], $3::text[])`, - [orgId, membershipRows.map((m) => m.channelId), membershipRows.map((m) => m.principalId)], - ); - } - if (revokedRows.length) { - await client.query( - `DELETE FROM directory_capability_channel_members member - USING unnest($2::text[], $3::text[]) AS revoked(channel_id, principal_id) - WHERE member.org_id = $1 AND member.channel_id = revoked.channel_id - AND member.principal_id = revoked.principal_id`, - [orgId, revokedRows.map((m) => m.channelId), revokedRows.map((m) => m.principalId)], - ); - await client.query( - `INSERT INTO directory_capability_channel_revocations (org_id, channel_id, principal_id) - SELECT $1, * FROM unnest($2::text[], $3::text[]) - ON CONFLICT (org_id, channel_id, principal_id) DO NOTHING`, - [orgId, revokedRows.map((m) => m.channelId), revokedRows.map((m) => m.principalId)], - ); - } - }); - }, - - async channelCapabilityMembership(channelId, principalId) { - const rows = await q( - `SELECT EXISTS ( - SELECT 1 FROM directory_capability_channels - WHERE org_id = $1 AND channel_id = $2 - ) AS known, - EXISTS ( - SELECT 1 FROM directory_capability_channel_members - WHERE org_id = $1 AND channel_id = $2 AND principal_id = $3 - ) AS member, - EXISTS ( - SELECT 1 FROM directory_capability_channel_revocations - WHERE org_id = $1 AND channel_id = $2 AND principal_id = $3 - ) AS revoked`, - [orgId, channelId, principalId], - ); - if (rows[0]?.revoked === true) return false; - return rows[0]?.known === true ? rows[0]?.member === true : undefined; - }, - async replaceGroups(groupMembers, syncedAt, groupIds, groupRosterIds) { const allRows = dedupPairs( groupMembers, @@ -669,10 +555,10 @@ export function createPostgresDirectoryStore(connectionString: string): Director async listChannelsFor(principalId) { const rows = await q( - `SELECT channel_id, name, is_private FROM directory_channels c - WHERE org_id = $1 AND (is_private = FALSE OR EXISTS ( + `SELECT channel_id, name, is_private, is_external FROM directory_channels c + WHERE org_id = $1 AND (is_private = FALSE OR (is_external = FALSE AND EXISTS ( SELECT 1 FROM directory_channel_members m - WHERE m.org_id = c.org_id AND m.channel_id = c.channel_id AND m.principal_id = $2)) + WHERE m.org_id = c.org_id AND m.channel_id = c.channel_id AND m.principal_id = $2))) ORDER BY name_lc`, [orgId, principalId], ); @@ -685,7 +571,10 @@ export function createPostgresDirectoryStore(connectionString: string): Director }, async listChannels() { - const rows = await q("SELECT channel_id, name, is_private FROM directory_channels WHERE org_id = $1", [orgId]); + const rows = await q( + "SELECT channel_id, name, is_private, is_external FROM directory_channels WHERE org_id = $1", + [orgId], + ); return rows.map(channelRow); }, @@ -718,7 +607,7 @@ export function createPostgresDirectoryStore(connectionString: string): Director "directory_channels", "channel_id", "name_lc", - "channel_id, name, is_private", + "channel_id, name, is_private, is_external", channelRow, ); if (m.kind === "one") return { kind: "one", channel: m.item }; diff --git a/src/slack/directory.ts b/src/slack/directory.ts index 00e64b8f..6d772d07 100644 --- a/src/slack/directory.ts +++ b/src/slack/directory.ts @@ -10,6 +10,7 @@ import { createRefreshCoalescer, createUserCache, externalMarker, + isExternallyShared, isReservedMentionName, probeIdentityMode, resolveChannelMembership, @@ -38,6 +39,7 @@ interface ChannelRow { channelId: string; name: string; isPrivate?: boolean; + isExternal?: boolean; } interface ChannelMembershipRow { channelId: string; @@ -54,7 +56,7 @@ interface ChannelRef { info: ChannelMeta; } -type RosterKind = { plural: string; authz: string; item: string; limit?: number; allowExternal?: boolean }; +type RosterKind = { plural: string; authz: string; item: string; limit?: number }; const MEMBERS_PAGE_LIMIT = 200; @@ -95,7 +97,7 @@ export function createDirectory(deps: { const { core, ids } = deps; const USER_SNAPSHOT_TTL_MS = deps.userSnapshotTtlMs ?? 5 * 60_000; const CHANNEL_MEMBERS_TTL_MS = deps.channelMembersTtlMs ?? 30 * 60_000; - const MAX_PRIVATE_CHANNELS = deps.maxPrivateChannels ?? 50; + const MAX_PRIVATE_CHANNELS = deps.maxPrivateChannels ?? Infinity; const userCache = createUserCache(deps.userCacheTtlMs ? { ttlMs: deps.userCacheTtlMs } : {}); let userSnapshot: UserSnapshot | undefined; @@ -199,9 +201,7 @@ export function createDirectory(deps: { for (const ref of refs) { const roster = classified.get(ref.id); if (!roster) continue; - const internalIds = kind.allowExternal - ? internalChannelMembers(roster.actors, roster.complete) - : allInternalChannelMembers(roster.actors, roster.complete, ref.info); + const internalIds = allInternalChannelMembers(roster.actors, roster.complete, ref.info); if (internalIds) rosters.set(ref.id, internalIds); } return rosters; @@ -213,7 +213,7 @@ export function createDirectory(deps: { kind: RosterKind, ): Promise> { const rosters = new Map(); - const limit = kind.limit ?? MAX_PRIVATE_CHANNELS; + const limit = kind.limit ?? Infinity; const slice = refs.slice(0, limit); if (refs.length > slice.length) { console.error( @@ -249,61 +249,37 @@ export function createDirectory(deps: { channels: ChannelRow[]; channelMembers: ChannelMembershipRow[]; channelRosterIds: string[]; - capabilityChannelMembers: ChannelMembershipRow[]; - capabilityChannelRosterIds: string[]; - capabilityChannelRevocations: ChannelMembershipRow[]; + channelRevocations: ChannelMembershipRow[]; }> { - const channels: ChannelRow[] = []; + const refs = [...publicChannels, ...privateChannels]; + const channels = refs.map((channel) => ({ + channelId: channel.id, + name: channel.name, + ...(channel.info.is_private ? { isPrivate: true } : {}), + ...(isExternallyShared(channel.info) ? { isExternal: true } : {}), + })); const channelMembers: ChannelMembershipRow[] = []; const channelRosterIds: string[] = []; - const capabilityChannelMembers: ChannelMembershipRow[] = []; - const capabilityChannelRosterIds: string[] = []; - const capabilityChannelRevocations: ChannelMembershipRow[] = []; - const publicRosters = await allInternalRosters(client, publicChannels, { - plural: "public channels", - authz: "public-channel-capability", - item: "public channel", - limit: publicChannels.length, - allowExternal: true, + const channelRevocations = [...invalidations].flatMap(([channelId, principalIds]) => + [...principalIds].map((principalId) => ({ channelId, principalId })), + ); + const rosters = await allClassifiedRosters(client, refs, { + plural: "channels", + authz: "channel", + item: "channel", + limit: publicChannels.length + Math.min(privateChannels.length, MAX_PRIVATE_CHANNELS), }); - for (const channel of publicChannels) { - const internalIds = publicRosters.get(channel.id); + for (const channel of refs) { + const roster = rosters.get(channel.id); + const internalIds = roster && internalChannelMembers(roster.actors, roster.complete); if (!internalIds) continue; + const revoked = invalidations.get(channel.id); channelRosterIds.push(channel.id); - for (const pid of internalIds) channelMembers.push({ channelId: channel.id, principalId: pid }); - } - const rosters = await allClassifiedRosters(client, privateChannels, { - plural: "private channels", - authz: "private-channel", - item: "private channel", - limit: privateChannels.length, - }); - for (const c of privateChannels) { - channels.push({ channelId: c.id, name: c.name, isPrivate: true }); - const roster = rosters.get(c.id); - const internalIds = roster && allInternalChannelMembers(roster.actors, roster.complete, c.info); - if (internalIds) { - channelRosterIds.push(c.id); - for (const pid of internalIds) channelMembers.push({ channelId: c.id, principalId: pid }); + for (const principalId of internalIds) { + if (!revoked?.has(principalId)) channelMembers.push({ channelId: channel.id, principalId }); } - const capabilityIds = roster && internalChannelMembers(roster.actors, roster.complete); - if (!capabilityIds) { - for (const principalId of invalidations.get(c.id) ?? []) { - capabilityChannelRevocations.push({ channelId: c.id, principalId }); - } - continue; - } - capabilityChannelRosterIds.push(c.id); - for (const pid of capabilityIds) capabilityChannelMembers.push({ channelId: c.id, principalId: pid }); } - return { - channels, - channelMembers, - channelRosterIds, - capabilityChannelMembers, - capabilityChannelRosterIds, - capabilityChannelRevocations, - }; + return { channels, channelMembers, channelRosterIds, channelRevocations }; } async function listBotGroupDms(client: any): Promise { @@ -343,9 +319,7 @@ export function createDirectory(deps: { channels: ChannelRow[]; channelMembers: ChannelMembershipRow[]; channelRosterIds: string[]; - capabilityChannelMembers: ChannelMembershipRow[]; - capabilityChannelRosterIds: string[]; - capabilityChannelRevocations: ChannelMembershipRow[]; + channelRevocations: ChannelMembershipRow[]; groupMembers?: GroupMembershipRow[]; groupIds?: string[]; groupRosterIds?: string[]; @@ -358,13 +332,12 @@ export function createDirectory(deps: { async function fetchChannels( client: any, invalidations: ChannelInvalidations, + targetChannelIds?: ReadonlySet, ): Promise<{ channels: ChannelRow[]; channelMembers: ChannelMembershipRow[]; channelRosterIds: string[]; - capabilityChannelMembers: ChannelMembershipRow[]; - capabilityChannelRosterIds: string[]; - capabilityChannelRevocations: ChannelMembershipRow[]; + channelRevocations: ChannelMembershipRow[]; groupMembers?: GroupMembershipRow[]; groupIds?: string[]; groupRosterIds?: string[]; @@ -378,6 +351,36 @@ export function createDirectory(deps: { console.error("[slack-plugin] channel list failed:", (err as Error).message); return null; } + const listedChannels = [...listed.publicChannels, ...listed.privateChannels]; + if (targetChannelIds?.size) { + const computed = await computeChannelMembership( + client, + listed.publicChannels.filter((channel) => targetChannelIds.has(channel.id)), + listed.privateChannels.filter((channel) => targetChannelIds.has(channel.id)), + invalidations, + ); + const channels = listedChannels.map((channel) => ({ + channelId: channel.id, + name: channel.name, + ...(channel.info.is_private ? { isPrivate: true } : {}), + ...(isExternallyShared(channel.info) ? { isExternal: true } : {}), + })); + if (privateChannelsCache) { + const refreshed = new Set(computed.channelRosterIds); + privateChannelsCache.channels = channels; + privateChannelsCache.channelMembers = [ + ...privateChannelsCache.channelMembers.filter( + (member) => + !refreshed.has(member.channelId) && !invalidations.get(member.channelId)?.has(member.principalId), + ), + ...computed.channelMembers, + ]; + privateChannelsCache.channelRosterIds = [ + ...new Set([...privateChannelsCache.channelRosterIds, ...computed.channelRosterIds]), + ]; + } + return { ...computed, channels, fetchedAt: Date.now() }; + } const fresh = privateChannelsCache && Date.now() - privateChannelsCache.fetchedAt < CHANNEL_MEMBERS_TTL_MS; let includeGroups = true; if (!fresh) { @@ -419,21 +422,14 @@ export function createDirectory(deps: { channels: [], channelMembers: [], channelRosterIds: [], - capabilityChannelMembers: [], - capabilityChannelRosterIds: [], - capabilityChannelRevocations: [], + channelRevocations: [], fetchedAt: 0, }; return { - channels: [ - ...listed.publicChannels.map((channel) => ({ channelId: channel.id, name: channel.name })), - ...priv.channels, - ], + channels: priv.channels, channelMembers: priv.channelMembers, channelRosterIds: priv.channelRosterIds, - capabilityChannelMembers: priv.capabilityChannelMembers, - capabilityChannelRosterIds: priv.capabilityChannelRosterIds, - capabilityChannelRevocations: priv.capabilityChannelRevocations, + channelRevocations: priv.channelRevocations, fetchedAt: priv.fetchedAt, ...(includeGroups && priv.groupMembers ? { @@ -450,6 +446,7 @@ export function createDirectory(deps: { snap: UserSnapshot, client: any, invalidations: ChannelInvalidations = new Map(), + targetChannelIds?: ReadonlySet, ): Promise { const members = [...snap.byId.entries()] .filter(([, u]) => !u.actor.isExternalGuest) @@ -462,7 +459,7 @@ export function createDirectory(deps: { ...(slackId && slackId !== a.externalId ? { slackId } : {}), }; }); - const fetched = await fetchChannels(client, invalidations); + const fetched = await fetchChannels(client, invalidations, targetChannelIds); if (!members.length && !(fetched && fetched.channels.length)) return false; try { await core.pushDirectory({ @@ -473,9 +470,7 @@ export function createDirectory(deps: { channels: fetched.channels, channelMembers: fetched.channelMembers, channelRosterIds: fetched.channelRosterIds, - capabilityChannelMembers: fetched.capabilityChannelMembers, - capabilityChannelRosterIds: fetched.capabilityChannelRosterIds, - capabilityChannelRevocations: fetched.capabilityChannelRevocations, + channelRevocations: fetched.channelRevocations, channelsSyncedAt: fetched.fetchedAt, ...(fetched.groupMembers ? { @@ -522,18 +517,28 @@ export function createDirectory(deps: { let directorySyncClient: any; const invalidatedChannelMembers = new Map>(); + const targetedChannelIds = new Set(); + let fullDirectorySyncRequested = false; const coalescedDirectorySync = createRefreshCoalescer(async () => { - if (privateChannelsCache) privateChannelsCache.fetchedAt = 0; + const fullSync = fullDirectorySyncRequested; + fullDirectorySyncRequested = false; + const scheduledTargets = new Set(targetedChannelIds); + const targets = fullSync || !scheduledTargets.size ? undefined : scheduledTargets; + if (!targets && privateChannelsCache) privateChannelsCache.fetchedAt = 0; const snap = userSnapshot ?? (await getUserSnapshot(directorySyncClient)); const pendingInvalidations = new Map( [...invalidatedChannelMembers].map(([channelId, principalIds]) => [channelId, new Set(principalIds)]), ); - if (snap && (await pushDirectory(snap, directorySyncClient, pendingInvalidations))) { + if (snap && (await pushDirectory(snap, directorySyncClient, pendingInvalidations, targets))) { + if (privateChannelsCache) privateChannelsCache.channelRevocations = []; + for (const channelId of scheduledTargets) targetedChannelIds.delete(channelId); for (const [channelId, principalIds] of pendingInvalidations) { const current = invalidatedChannelMembers.get(channelId); for (const principalId of principalIds) current?.delete(principalId); if (!current?.size) invalidatedChannelMembers.delete(channelId); } + } else if (fullSync) { + fullDirectorySyncRequested = true; } }); @@ -543,11 +548,13 @@ export function createDirectory(deps: { invalidatePrincipalId?: string, ): Promise { directorySyncClient = client; + if (!invalidateChannelId) fullDirectorySyncRequested = true; if (invalidateChannelId && invalidatePrincipalId) { const principals = invalidatedChannelMembers.get(invalidateChannelId) ?? new Set(); principals.add(invalidatePrincipalId); invalidatedChannelMembers.set(invalidateChannelId, principals); } + if (invalidateChannelId) targetedChannelIds.add(invalidateChannelId); return coalescedDirectorySync(); } diff --git a/src/slack/events.ts b/src/slack/events.ts index 38f28a5f..7f5a4d69 100644 --- a/src/slack/events.ts +++ b/src/slack/events.ts @@ -95,7 +95,7 @@ export function registerSlackEvents( app.message(async ({ message, body, client, context }: any) => { const m = message as any; if (channelPrivacyChange(m)) { - await forceDirectorySync(client); + await forceDirectorySync(client, m.channel); return; } if (isGroupMembershipMessage(m)) { @@ -226,7 +226,7 @@ export function registerSlackEvents( : {}), }); } else { - await forceDirectorySync(client); + await forceDirectorySync(client, e.channel); } }); @@ -236,7 +236,7 @@ export function registerSlackEvents( const channel = typeof e.channel === "string" ? e.channel : e.channel?.id; if (deduper.seen(dedupeKey({ event_id: (body as { event_id?: string })?.event_id, channel, ts: e.event_ts }))) return; - await forceDirectorySync(client); + await forceDirectorySync(client, channel); }); } diff --git a/test/capability-routes.test.ts b/test/capability-routes.test.ts index 82fc1b50..fdab5b06 100644 --- a/test/capability-routes.test.ts +++ b/test/capability-routes.test.ts @@ -68,11 +68,6 @@ describe("capability-token control plane (crons + SOUL)", () => { [{ channelId: "C", name: "eng", isPrivate: false }], ["admin-alice", "U1", "U2", "U8"].map((principalId) => ({ channelId: "C", principalId })), ); - await built.directory.replaceCapabilityChannels( - ["C"], - ["admin-alice", "U1", "U2", "U8"].map((principalId) => ({ channelId: "C", principalId })), - ["C"], - ); server = createServer(built.app, { signingSecret: SECRET, scheduler: built.scheduler, @@ -748,10 +743,9 @@ describe("capability-token control plane (crons + SOUL)", () => { }); it("a public channel remains available to an active internal principal outside its current roster", async () => { - await built.directory.replaceCapabilityChannels( - ["C"], + await built.directory.replaceChannels( + [{ channelId: "C", name: "eng", isPrivate: false }], ["admin-alice", "U1", "U2"].map((principalId) => ({ channelId: "C", principalId })), - ["C"], ); assert.equal((await get("/v1/soul", { "x-agent-capability": await capChannel("U8") })).status, 200); }); @@ -761,11 +755,6 @@ describe("capability-token control plane (crons + SOUL)", () => { [{ channelId: "C", name: "eng", isPrivate: true }], ["admin-alice", "U1", "U2"].map((principalId) => ({ channelId: "C", principalId })), ); - await built.directory.replaceCapabilityChannels( - ["C"], - ["admin-alice", "U1", "U2"].map((principalId) => ({ channelId: "C", principalId })), - ["C"], - ); const members = [{ id: "B-LEGACY", type: "internal" as const }]; const token = await capFor("B-LEGACY", scopeId("channel", "C"), { botActor: true, diff --git a/test/directory-store.test.ts b/test/directory-store.test.ts index f4d56395..c1efc122 100644 --- a/test/directory-store.test.ts +++ b/test/directory-store.test.ts @@ -278,64 +278,34 @@ describe("private-channel membership (authorizes private-channel sends, §10)", assert.equal(await d.channelMembership("C-new", "U-new"), undefined); }); - it("keeps capability rosters separate from legacy channel-directory swaps", async () => { + it("applies removals without clearing a failed channel refresh", async () => { const d = createDirectoryStore(); - await d.replaceCapabilityChannels( - ["C-one", "C-two", "C-new"], + await d.replaceChannels( + [{ channelId: "C-one", name: "one", isPrivate: true }], [ - { channelId: "C-one", principalId: "U-old-one" }, + { channelId: "C-one", principalId: "U-leaving" }, { channelId: "C-one", principalId: "U-keep" }, - { channelId: "C-two", principalId: "U-old-two" }, ], - ["C-one", "C-two"], - ); - await d.replaceCapabilityChannels( - ["C-one", "C-two", "C-new"], - [{ channelId: "C-two", principalId: "U-new-two" }], - ["C-two"], ); await d.replaceChannels( - [ - { channelId: "C-one", name: "one" }, - { channelId: "C-two", name: "two" }, - { channelId: "C-new", name: "new" }, - ], + [{ channelId: "C-one", name: "one", isPrivate: true }], [], + undefined, + [], + [{ channelId: "C-one", principalId: "U-leaving" }], ); - assert.equal(await d.channelCapabilityMembership("C-one", "U-old-one"), true); - assert.equal(await d.channelCapabilityMembership("C-two", "U-old-two"), false); - assert.equal(await d.channelCapabilityMembership("C-two", "U-new-two"), true); - assert.equal(await d.channelCapabilityMembership("C-new", "U-new"), undefined); - await d.replaceCapabilityChannels(["C-one", "C-two", "C-new"], [], [], undefined, [ - { channelId: "C-one", principalId: "U-old-one" }, - ]); - assert.equal(await d.channelCapabilityMembership("C-one", "U-old-one"), false); - assert.equal(await d.channelCapabilityMembership("C-one", "U-keep"), true); - await d.replaceCapabilityChannels(["C-one", "C-two", "C-new"], [], [], undefined, [ - { channelId: "C-new", principalId: "U-new" }, - ]); - assert.equal(await d.channelCapabilityMembership("C-new", "U-new"), false); - await d.replaceCapabilityChannels( - ["C-one", "C-two", "C-new"], - [{ channelId: "C-new", principalId: "U-new" }], - ["C-new"], - ); - assert.equal(await d.channelCapabilityMembership("C-new", "U-new"), true); + assert.equal(await d.channelMembership("C-one", "U-leaving"), false); + assert.equal(await d.channelMembership("C-one", "U-keep"), true); }); - it("invalidates preserved rosters when a public channel becomes private", async () => { + it("uses one Slack Connect roster without making a private room an ordinary send target", async () => { const d = createDirectoryStore(); - await d.replaceChannels([{ channelId: "C-one", name: "one" }], [{ channelId: "C-one", principalId: "U-old" }]); - await d.replaceCapabilityChannels(["C-one"], [{ channelId: "C-one", principalId: "U-old" }], ["C-one"]); - await d.replaceChannels([{ channelId: "C-one", name: "one", isPrivate: true }]); - assert.equal(await d.channelMembership("C-one", "U-old"), undefined); - assert.equal(await d.channelCapabilityMembership("C-one", "U-old"), undefined); - - await d.replaceChannels([{ channelId: "C-one", name: "one" }]); await d.replaceChannels( - [{ channelId: "C-one", name: "one", isPrivate: true }], - [{ channelId: "C-one", principalId: "U-current" }], + [{ channelId: "C-connect", name: "connect", isPrivate: true, isExternal: true }], + [{ channelId: "C-connect", principalId: "U-member" }], ); - assert.deepEqual(await d.channelMemberIds("C-one"), ["U-current"]); + assert.equal(await d.channelMembership("C-connect", "U-member"), true); + assert.equal(await d.channelMember("C-connect", "U-member"), false); + assert.deepEqual(await d.listChannelsFor("U-member"), []); }); }); diff --git a/test/postgres-directory-store.test.ts b/test/postgres-directory-store.test.ts index a485bc4d..d0823ac8 100644 --- a/test/postgres-directory-store.test.ts +++ b/test/postgres-directory-store.test.ts @@ -10,7 +10,7 @@ before(async () => { const pg = (await import("pg")).default; const p = new pg.Pool({ connectionString: URL }); await p.query( - "DROP TABLE IF EXISTS directory_members, directory_channels, directory_channel_members, directory_capability_channels, directory_capability_channel_members, directory_capability_channel_revocations, directory_groups, directory_group_members, directory_sync, directory_meta CASCADE", + "DROP TABLE IF EXISTS directory_members, directory_channels, directory_channel_members, directory_groups, directory_group_members, directory_sync, directory_meta CASCADE", ); await p.end(); }); @@ -383,71 +383,35 @@ test("pg directory: a partial roster swap preserves channels whose roster is unk assert.equal(await store.channelMembership("C-new", "U-new"), undefined); }); -test("pg directory: capability rosters survive legacy channel-directory swaps", { skip }, async () => { +test("pg directory: removals apply without clearing a failed channel refresh", { skip }, async () => { const store = createPostgresDirectoryStore(URL!); - await store.replaceCapabilityChannels( - ["C-one", "C-two", "C-new"], + await store.replaceChannels( + [{ channelId: "C-one", name: "one", isPrivate: true }], [ - { channelId: "C-one", principalId: "U-old-one" }, + { channelId: "C-one", principalId: "U-leaving" }, { channelId: "C-one", principalId: "U-keep" }, - { channelId: "C-two", principalId: "U-old-two" }, ], - ["C-one", "C-two"], - ); - await store.replaceCapabilityChannels( - ["C-one", "C-two", "C-new"], - [{ channelId: "C-two", principalId: "U-new-two" }], - ["C-two"], ); await store.replaceChannels( - [ - { channelId: "C-one", name: "one" }, - { channelId: "C-two", name: "two" }, - { channelId: "C-new", name: "new" }, - ], + [{ channelId: "C-one", name: "one", isPrivate: true }], [], + undefined, + [], + [{ channelId: "C-one", principalId: "U-leaving" }], ); - assert.equal(await store.channelCapabilityMembership("C-one", "U-old-one"), true); - assert.equal(await store.channelCapabilityMembership("C-two", "U-old-two"), false); - assert.equal(await store.channelCapabilityMembership("C-two", "U-new-two"), true); - assert.equal(await store.channelCapabilityMembership("C-new", "U-new"), undefined); - await store.replaceCapabilityChannels(["C-one", "C-two", "C-new"], [], [], undefined, [ - { channelId: "C-one", principalId: "U-old-one" }, - ]); - assert.equal(await store.channelCapabilityMembership("C-one", "U-old-one"), false); - assert.equal(await store.channelCapabilityMembership("C-one", "U-keep"), true); - await store.replaceCapabilityChannels(["C-one", "C-two", "C-new"], [], [], undefined, [ - { channelId: "C-new", principalId: "U-new" }, - ]); - assert.equal(await store.channelCapabilityMembership("C-new", "U-new"), false); - await store.replaceCapabilityChannels( - ["C-one", "C-two", "C-new"], - [{ channelId: "C-new", principalId: "U-new" }], - ["C-new"], - ); - assert.equal(await store.channelCapabilityMembership("C-new", "U-new"), true); + assert.equal(await store.channelMembership("C-one", "U-leaving"), false); + assert.equal(await store.channelMembership("C-one", "U-keep"), true); }); -test("pg directory: public-to-private transitions invalidate preserved rosters", { skip }, async () => { +test("pg directory: a private Slack Connect roster is not an ordinary send target", { skip }, async () => { const store = createPostgresDirectoryStore(URL!); await store.replaceChannels( - [{ channelId: "C-transition", name: "transition" }], - [{ channelId: "C-transition", principalId: "U-old" }], - ); - await store.replaceCapabilityChannels( - ["C-transition"], - [{ channelId: "C-transition", principalId: "U-old" }], - ["C-transition"], - ); - await store.replaceChannels([{ channelId: "C-transition", name: "transition", isPrivate: true }]); - assert.equal(await store.channelMembership("C-transition", "U-old"), undefined); - assert.equal(await store.channelCapabilityMembership("C-transition", "U-old"), undefined); - await store.replaceCapabilityChannels( - ["C-transition"], - [{ channelId: "C-transition", principalId: "U-old" }], - ["C-transition"], + [{ channelId: "C-connect", name: "connect", isPrivate: true, isExternal: true }], + [{ channelId: "C-connect", principalId: "U-member" }], ); - assert.equal(await store.channelCapabilityMembership("C-transition", "U-old"), true); + assert.equal(await store.channelMembership("C-connect", "U-member"), true); + assert.equal(await store.channelMember("C-connect", "U-member"), false); + assert.deepEqual(await store.listChannelsFor("U-member"), []); }); test("pg directory: a partial group swap preserves unknown rosters", { skip }, async () => { diff --git a/test/projects.test.ts b/test/projects.test.ts index 40c72d7f..a9b45679 100644 --- a/test/projects.test.ts +++ b/test/projects.test.ts @@ -207,16 +207,6 @@ test("capability scope checks follow current shared rosters", async () => { ], 1, ); - await built.directory.replaceCapabilityChannels( - ["C-public", "C-private"], - [ - { channelId: "C-public", principalId: "member" }, - { channelId: "C-private", principalId: "member" }, - { channelId: "C-private", principalId: "B1" }, - ], - ["C-public", "C-private"], - 1, - ); await built.directory.replaceGroups([{ groupId: "G1", principalId: "member" }], 1); assert.equal(await built.app.authorizesCapabilityScope({ actorId: "member", scopeId: "channel:C-private" }), true); @@ -233,7 +223,6 @@ test("capability scope checks follow current shared rosters", async () => { 2, ); await built.directory.replaceGroups([], 2); - await built.directory.replaceCapabilityChannels(["C-public", "C-private"], [], ["C-public", "C-private"], 2); assert.equal(await built.app.authorizesCapabilityScope({ actorId: "member", scopeId: "channel:C-private" }), false); assert.equal(await built.app.authorizesCapabilityScope({ actorId: "member", scopeId: "group:G1" }), false); @@ -245,11 +234,6 @@ test("channel capabilities bridge legacy public rosters but still honor deactiva await built.app.upsertDirectory([{ principalId: "member", displayName: "Member", type: "internal" }]); await built.directory.replaceChannels([{ channelId: "C-public", name: "public" }], []); assert.equal(await built.app.authorizesCapabilityScope({ actorId: "member", scopeId: "channel:C-public" }), true); - await built.directory.replaceCapabilityChannels( - ["C-public"], - [{ channelId: "C-public", principalId: "member" }], - ["C-public"], - ); await built.identity.deactivate("member"); assert.equal(await built.app.authorizesCapabilityScope({ actorId: "member", scopeId: "channel:C-public" }), false); }); diff --git a/test/secret-drop.test.ts b/test/secret-drop.test.ts index 81729deb..8880b1b6 100644 --- a/test/secret-drop.test.ts +++ b/test/secret-drop.test.ts @@ -520,7 +520,6 @@ describe("/v1/keychain/drops — mint, form, redeem", async () => { [{ channelId: "C1", name: "drops", isPrivate: true }], [{ channelId: "C1", principalId: "U_A" }], ); - await built.directory.replaceCapabilityChannels(["C1"], [{ channelId: "C1", principalId: "U_A" }], ["C1"]); const members = [{ id: "B-LEGACY", type: "internal" as const }]; const minted = await post( "/v1/keychain/drops", diff --git a/test/slack-index.integration.test.ts b/test/slack-index.integration.test.ts index 56a327f3..68fd3006 100644 --- a/test/slack-index.integration.test.ts +++ b/test/slack-index.integration.test.ts @@ -495,12 +495,12 @@ test("large public channels publish their complete roster and accept internal tu test("failed background roster reads are marked unknown instead of clearing known capabilities", async () => { const f = await fixture(); try { - assert.ok(f.core.directories.at(-1).capabilityChannelRosterIds.includes("CPX")); + assert.ok(f.core.directories.at(-1).channelRosterIds.includes("CPX")); f.client.membershipFailures.add("CPX"); const pushes = f.core.directories.length; - await f.app.emitEvent("channel_rename", { channel: { id: "C1" }, event_ts: "100.5" }); + await f.app.emitEvent("channel_rename", { channel: { id: "CPX" }, event_ts: "100.5" }); await waitFor(() => f.core.directories.length > pushes); - assert.ok(!f.core.directories.at(-1).capabilityChannelRosterIds.includes("CPX")); + assert.ok(!f.core.directories.at(-1).channelRosterIds.includes("CPX")); } finally { await f.stop(); } @@ -514,8 +514,8 @@ test("a failed refresh after a leave event revokes only the departing member", a await f.app.emitEvent("member_left_channel", { user: "U1", channel: "CPX", event_ts: "100.6" }); await waitFor(() => f.core.directories.length > pushes); const pushed = f.core.directories.at(-1); - assert.ok(!pushed.capabilityChannelRosterIds.includes("CPX")); - assert.deepEqual(pushed.capabilityChannelRevocations, [{ channelId: "CPX", principalId: "U1" }]); + assert.ok(!pushed.channelRosterIds.includes("CPX")); + assert.deepEqual(pushed.channelRevocations, [{ channelId: "CPX", principalId: "U1" }]); } finally { await f.stop(); } @@ -528,7 +528,7 @@ test("a failed email-mode refresh revokes the departing canonical principal", as const pushes = f.core.directories.length; await f.app.emitEvent("member_left_channel", { user: "U1", channel: "CPX", event_ts: "100.7" }); await waitFor(() => f.core.directories.length > pushes); - assert.deepEqual(f.core.directories.at(-1).capabilityChannelRevocations, [ + assert.deepEqual(f.core.directories.at(-1).channelRevocations, [ { channelId: "CPX", principalId: "alice@example.com" }, ]); } finally { @@ -540,22 +540,25 @@ test("Slack Connect directory rosters contain only internal principals", async ( const f = await fixture({ externalParticipants: true }); try { const pushed = f.core.directories.at(-1); - assert.ok(!pushed.capabilityChannelRosterIds.includes("CX")); - assert.ok(pushed.capabilityChannelRosterIds.includes("CPX")); + assert.ok(pushed.channelRosterIds.includes("CX")); + assert.ok(pushed.channelRosterIds.includes("CPX")); + assert.equal(pushed.channels.find((channel: any) => channel.channelId === "CPX")?.isExternal, true); assert.deepEqual( pushed.channelMembers.filter((m: any) => m.channelId === "CX").map((m: any) => m.principalId), ["U1"], ); assert.deepEqual( - pushed.capabilityChannelMembers.filter((m: any) => m.channelId === "CPX").map((m: any) => m.principalId), + pushed.channelMembers.filter((m: any) => m.channelId === "CPX").map((m: any) => m.principalId), ["U1"], ); - assert.ok(!pushed.channelRosterIds.includes("CPX")); + assert.ok(pushed.channelRosterIds.includes("CPX")); f.client.membershipListings.set("CPX", 0); + f.client.membershipListings.set("C1", 0); const pushes = f.core.directories.length; - await f.app.emitEvent("channel_rename", { channel: { id: "C1" }, event_ts: "100.7" }); + await f.app.emitEvent("channel_rename", { channel: { id: "CPX" }, event_ts: "100.7" }); await waitFor(() => f.core.directories.length > pushes); assert.equal(f.client.membershipListings.get("CPX"), 1); + assert.equal(f.client.membershipListings.get("C1"), 0); } finally { await f.stop(); } @@ -1052,7 +1055,7 @@ test("a failed group member read marks only that roster unknown", async () => { const good = f.core.directories.findLast((d: any) => d.groupsSyncedAt !== undefined); f.client.membershipFailures.add("G5"); const pushes = f.core.directories.length; - await f.app.emitEvent("channel_rename", { channel: { id: "C1" }, event_ts: "403.2" }); + await f.app.emitMessage({ channel: "G5", channel_type: "mpim", subtype: "group_join", ts: "403.2" }); await waitFor(() => f.core.directories.length > pushes); const last = f.core.directories.at(-1); assert.ok(last.groupsSyncedAt > good.groupsSyncedAt); @@ -1073,7 +1076,7 @@ test("all listed group DMs reach the directory past the legacy private-channel c f.client.membersByChannel.set(id, ["U1", "U2", "UBOT"]); } const pushes = f.core.directories.length; - await f.app.emitEvent("channel_rename", { channel: { id: "C1" }, event_ts: "403.3" }); + await f.app.emitMessage({ channel: "G0", channel_type: "mpim", subtype: "group_join", ts: "403.3" }); await waitFor(() => f.core.directories.length > pushes); assert.equal(new Set(f.core.directories.at(-1).groupMembers.map((member: any) => member.groupId)).size, 51); } finally { From a8e4cc83c1ec0f2b8132666512c4863b3f8a75f3 Mon Sep 17 00:00:00 2001 From: Josh France <12610835+16francej@users.noreply.github.com> Date: Sun, 16 Aug 2026 12:04:17 -0400 Subject: [PATCH 08/10] Improve synchronization behavior --- src/slack/directory.ts | 54 ++++++++++++++++------------ src/slack/turn-handler.ts | 24 ++++++------- test/slack-index.integration.test.ts | 53 ++++++++++++++++++++++++--- 3 files changed, 93 insertions(+), 38 deletions(-) diff --git a/src/slack/directory.ts b/src/slack/directory.ts index 6d772d07..94181f77 100644 --- a/src/slack/directory.ts +++ b/src/slack/directory.ts @@ -59,6 +59,7 @@ interface ChannelRef { type RosterKind = { plural: string; authz: string; item: string; limit?: number }; const MEMBERS_PAGE_LIMIT = 200; +const ROSTER_FETCH_CONCURRENCY = 4; export interface Directory { getUserSnapshot(client: any): Promise<{ byId: Map; fetchedAt: number } | undefined>; @@ -104,6 +105,7 @@ export function createDirectory(deps: { let userSnapshotInFlight: Promise | undefined; async function refreshUserSnapshot(client: any): Promise { + const fetchedAt = Date.now(); const byId = new Map(); const mentionIndex = new Map(); const ambiguousNames = new Set(); @@ -140,7 +142,7 @@ export function createDirectory(deps: { `[slack] email identity mode: ${missingEmails} own-team member(s) have no visible email (missing users:read.email scope?) — they fail closed to guest`, ); } - return { byId, fetchedAt: Date.now() }; + return { byId, fetchedAt }; } async function listBotChannels( @@ -220,23 +222,30 @@ export function createDirectory(deps: { `[slack-plugin] ${refs.length} ${kind.plural} exceed the cap (${limit}); ${refs.length - slice.length} omitted from ${kind.authz} authorization`, ); } - for (const ref of slice) { - let memberIds: string[]; - try { - memberIds = await fetchChannelMemberIds(client, ref.id); - } catch (err) { - console.error(`[slack-plugin] members fetch failed for ${kind.item} ${ref.id}:`, (err as Error).message); - continue; - } - const actors: ActorAssertion[] = []; - let complete = true; - for (const id of memberIds) { - const { actor, ok } = await classifyUserCached(client, id); - actors.push(actor); - if (!ok) complete = false; - } - rosters.set(ref.id, { actors, complete }); - } + const queue = [...slice]; + await Promise.all( + Array.from({ length: Math.min(ROSTER_FETCH_CONCURRENCY, queue.length) }, async () => { + for (;;) { + const ref = queue.pop(); + if (!ref) return; + let memberIds: string[]; + try { + memberIds = await fetchChannelMemberIds(client, ref.id); + } catch (err) { + console.error(`[slack-plugin] members fetch failed for ${kind.item} ${ref.id}:`, (err as Error).message); + continue; + } + const actors: ActorAssertion[] = []; + let complete = true; + for (const id of memberIds) { + const { actor, ok } = await classifyUserCached(client, id); + actors.push(actor); + if (!ok) complete = false; + } + rosters.set(ref.id, { actors, complete }); + } + }), + ); return rosters; } @@ -344,6 +353,7 @@ export function createDirectory(deps: { fetchedAt: number; groupsFetchedAt?: number; } | null> { + const fetchedAt = Date.now(); let listed: { publicChannels: ChannelRef[]; privateChannels: ChannelRef[] }; try { listed = await listBotChannels(client); @@ -379,7 +389,7 @@ export function createDirectory(deps: { ...new Set([...privateChannelsCache.channelRosterIds, ...computed.channelRosterIds]), ]; } - return { ...computed, channels, fetchedAt: Date.now() }; + return { ...computed, channels, fetchedAt }; } const fresh = privateChannelsCache && Date.now() - privateChannelsCache.fetchedAt < CHANNEL_MEMBERS_TTL_MS; let includeGroups = true; @@ -395,12 +405,12 @@ export function createDirectory(deps: { let groupRosterIds: string[] | undefined; let groupsFetchedAt: number | undefined; try { + groupsFetchedAt = Date.now(); groupIds = await listBotGroupDms(client); for (const id of groupIds) seenGroupIds.add(id); const computedGroups = await computeGroupMembership(client, groupIds); groupMembers = computedGroups.groupMembers; groupRosterIds = computedGroups.groupRosterIds; - groupsFetchedAt = Date.now(); } catch (err) { console.error("[slack-plugin] group-DM list failed:", (err as Error).message); includeGroups = false; @@ -415,7 +425,7 @@ export function createDirectory(deps: { groupIds, groupRosterIds, groupsFetchedAt, - fetchedAt: Date.now(), + fetchedAt, }; } const priv = privateChannelsCache ?? { @@ -499,7 +509,7 @@ export function createDirectory(deps: { userSnapshotInFlight = refreshUserSnapshot(client) .then((s) => { userSnapshot = s; - void pushDirectory(s, client); + if (snap) void pushDirectory(s, client); return s; }) .finally(() => { diff --git a/src/slack/turn-handler.ts b/src/slack/turn-handler.ts index 76f70992..03019a45 100644 --- a/src/slack/turn-handler.ts +++ b/src/slack/turn-handler.ts @@ -255,18 +255,6 @@ export function createTurnHandler(deps: { replyThreadTs = root; } - if (!inc.unprompted) { - const intercepted = await maybeInterceptStop({ - text, - threadRef, - getInFlightRun: (ref) => - inFlightRunByThread.get(ref) ?? - fetchActiveRunForThread(ref).catch(swallowAs("slack: active-run lookup", undefined)), - signalAbort: signalRunAbort, - }).catch(swallowAs("slack: abort signal", true)); - if (intercepted) return; - } - let queuedRunId: string | undefined; let taskList: TaskListPresenter | undefined; const ack = inc.unprompted @@ -353,6 +341,18 @@ export function createTurnHandler(deps: { return; } + if (!inc.unprompted) { + const intercepted = await maybeInterceptStop({ + text, + threadRef, + getInFlightRun: (ref) => + inFlightRunByThread.get(ref) ?? + fetchActiveRunForThread(ref).catch(swallowAs("slack: active-run lookup", undefined)), + signalAbort: signalRunAbort, + }).catch(swallowAs("slack: abort signal", true)); + if (intercepted) return; + } + { const containerName = inc.kind === "dm" ? actor.displayName?.trim() || undefined : channelName; void mirrorMessageEvent( diff --git a/test/slack-index.integration.test.ts b/test/slack-index.integration.test.ts index 68fd3006..1c0e3307 100644 --- a/test/slack-index.integration.test.ts +++ b/test/slack-index.integration.test.ts @@ -25,6 +25,10 @@ class FakeSlackClient { readonly membershipFailures = new Set(); readonly membershipListings = new Map(); readonly botsById = new Map(); + membershipDelayMs = 0; + activeMembershipListings = 0; + maxActiveMembershipListings = 0; + firstMembershipListingStartedAt: number | undefined; groupListings = 0; failGroupListing = false; private postSequence = 0; @@ -137,9 +141,17 @@ class FakeSlackClient { return; } if (method === "conversations.members") { + this.firstMembershipListingStartedAt ??= Date.now(); this.membershipListings.set(args.channel, (this.membershipListings.get(args.channel) ?? 0) + 1); - if (this.membershipFailures.has(args.channel)) throw new Error("missing conversations:read"); - yield { members: this.membersByChannel.get(args.channel) ?? [] }; + this.activeMembershipListings++; + this.maxActiveMembershipListings = Math.max(this.maxActiveMembershipListings, this.activeMembershipListings); + try { + if (this.membershipDelayMs) await new Promise((resolve) => setTimeout(resolve, this.membershipDelayMs)); + if (this.membershipFailures.has(args.channel)) throw new Error("missing conversations:read"); + yield { members: this.membersByChannel.get(args.channel) ?? [] }; + } finally { + this.activeMembershipListings--; + } return; } throw new Error(`unexpected pagination method: ${method}`); @@ -326,7 +338,13 @@ async function waitFor(cond: () => boolean, timeoutMs = 2000): Promise { } async function fixture( - options: { externalParticipants?: boolean; webUiPublicUrl?: string; identityEmail?: "0" | "1" } = {}, + options: { + externalParticipants?: boolean; + webUiPublicUrl?: string; + identityEmail?: "0" | "1"; + extraChannels?: number; + membershipDelayMs?: number; + } = {}, ) { const core = new FakeCore(); core.externalParticipants = options.externalParticipants ?? false; @@ -340,6 +358,7 @@ async function fixture( core, ); const app = FakeApp.instances.at(-1)!; + app.client.membershipDelayMs = options.membershipDelayMs ?? 0; app.client.usersById.set("U1", internalUser("U1", "Alice")); app.client.usersById.set("U2", internalUser("U2", "Bob")); app.client.usersById.set("UX", { id: "UX", team_id: "T2", name: "mallory", profile: { display_name: "Mallory" } }); @@ -361,6 +380,11 @@ async function fixture( app.client.membersByChannel.set("C1", ["U1", "U2", "UBOT"]); app.client.membersByChannel.set("CX", ["U1", "UX", "UBOT"]); app.client.membersByChannel.set("CPX", ["U1", "UX", "UBOT"]); + for (let i = 0; i < (options.extraChannels ?? 0); i++) { + const id = `CE${i}`; + app.client.channelsById.set(id, { id, name: `extra-${i}`, is_member: true, is_private: false }); + app.client.membersByChannel.set(id, ["U1", "UBOT"]); + } const plugin = await started; await new Promise((resolve) => setImmediate(resolve)); return { app, client: app.client, core, stop: () => plugin.stop() }; @@ -465,6 +489,18 @@ test("public channel rosters stay current in the core directory", async () => { } }); +test("full directory refreshes bound concurrent Slack roster reads", async () => { + const f = await fixture({ extraChannels: 5, membershipDelayMs: 10 }); + try { + assert.equal(f.client.membershipListings.size, 8); + assert.ok(f.client.maxActiveMembershipListings > 1); + assert.ok(f.client.maxActiveMembershipListings <= 4); + assert.ok(f.core.directories.at(-1).channelsSyncedAt <= f.client.firstMembershipListingStartedAt!); + } finally { + await f.stop(); + } +}); + test("large public channels publish their complete roster and accept internal turns", async () => { const f = await fixture(); try { @@ -829,6 +865,13 @@ test("a verified legacy bot without a user principal can become a turn", async ( test("a bot-authored stop can abort a live run", async () => { const f = await fixture(); try { + f.client.usersById.set("B1", { + id: "B1", + team_id: "T1", + is_bot: true, + name: "peerbot", + profile: { display_name: "Peer Bot" }, + }); f.core.activeRun = "run-active"; await f.app.emitMessage({ channel: "D1", @@ -849,9 +892,11 @@ test("a bot-authored stop can abort a live run", async () => { test("a Slack Connect mention is refused ephemerally and never mirrored", async () => { const f = await fixture(); try { - const event = { channel: "CX", channel_type: "channel", user: "U1", text: "<@UBOT> share secrets", ts: "103.1" }; + f.core.activeRun = "run-active"; + const event = { channel: "CX", channel_type: "channel", user: "U1", text: "<@UBOT> stop", ts: "103.1" }; await f.app.emitEvent("app_mention", event); assert.equal(f.core.turns.length, 0); + assert.equal(f.core.abortedRuns.length, 0); assert.equal(f.core.ingests.length, 0); assert.equal(f.client.posts.length, 0); assert.equal(f.client.ephemerals.length, 1); From 9707cc9628832c93ccad9d6890997ecdd520fae5 Mon Sep 17 00:00:00 2001 From: Josh France <12610835+16francej@users.noreply.github.com> Date: Sun, 16 Aug 2026 12:05:57 -0400 Subject: [PATCH 09/10] Refine request handling --- src/slack/turn-handler.ts | 74 ++++++++++++++-------------- test/slack-index.integration.test.ts | 2 + 2 files changed, 39 insertions(+), 37 deletions(-) diff --git a/src/slack/turn-handler.ts b/src/slack/turn-handler.ts index 03019a45..fb11669d 100644 --- a/src/slack/turn-handler.ts +++ b/src/slack/turn-handler.ts @@ -257,42 +257,6 @@ export function createTurnHandler(deps: { let queuedRunId: string | undefined; let taskList: TaskListPresenter | undefined; - const ack = inc.unprompted - ? undefined - : createAckPresenter({ - postAck: async (text) => { - const rendered = toSlackMrkdwn(text); - if (await taskList?.addLead(rendered)) return; - const ts = await postReply(rendered); - if (ts) await taskList?.attach(ts, rendered); - }, - addReaction: (name) => client.reactions.add({ channel: inc.channel, timestamp: inc.ts, name }).then(() => {}), - removeReaction: (name) => - client.reactions.remove({ channel: inc.channel, timestamp: inc.ts, name }).then(() => {}), - emojiCandidates: [...DEFAULT_ACK_REACTIONS], - emojiPick: ackEmoji.requestAckEmoji(text, ackEmoji.ackPickCandidates(client), { - channel: inc.channel, - ts: inc.ts, - }), - }); - if (!inc.unprompted) { - taskList = createTaskListPresenter({ - post: (text, blocks) => postReply(text, blocks), - update: (ts, text, blocks) => - client.chat.update({ channel: inc.channel, ts, text, blocks, ...botIdentityArgs() }).then(() => { - mirrorSelfPost(inc.channel, ts, text, { sub: replyThreadTs, editedAt: Date.now() }); - }), - checkpoint: async (ts) => { - if (queuedRunId) await checkpointRunEditRef(queuedRunId, ts); - }, - remove: (ts) => client.chat.delete({ channel: inc.channel, ts }).then(() => {}), - onSurfacePosted: () => ack?.onSurfacePosted(), - onError: (error) => console.error("[slack-plugin] task-list update failed:", (error as Error).message), - }); - } - const settleAck = async (): Promise => { - await ack?.settle().catch(swallowAs("slack: ack settle", undefined)); - }; if (inc.kind === "channel") { const membership = inc.prefetched @@ -332,7 +296,6 @@ export function createTurnHandler(deps: { }; if (audience.some((a) => a.isExternalGuest) && !(await externalParticipantsEnabled())) { - await settleAck(); if (!inc.unprompted) { await ephemeralOrSay( "I can't respond here — this conversation isn't fully internal. Try a DM or a fully-internal channel.", @@ -353,6 +316,43 @@ export function createTurnHandler(deps: { if (intercepted) return; } + const ack = inc.unprompted + ? undefined + : createAckPresenter({ + postAck: async (text) => { + const rendered = toSlackMrkdwn(text); + if (await taskList?.addLead(rendered)) return; + const ts = await postReply(rendered); + if (ts) await taskList?.attach(ts, rendered); + }, + addReaction: (name) => client.reactions.add({ channel: inc.channel, timestamp: inc.ts, name }).then(() => {}), + removeReaction: (name) => + client.reactions.remove({ channel: inc.channel, timestamp: inc.ts, name }).then(() => {}), + emojiCandidates: [...DEFAULT_ACK_REACTIONS], + emojiPick: ackEmoji.requestAckEmoji(text, ackEmoji.ackPickCandidates(client), { + channel: inc.channel, + ts: inc.ts, + }), + }); + if (!inc.unprompted) { + taskList = createTaskListPresenter({ + post: (text, blocks) => postReply(text, blocks), + update: (ts, text, blocks) => + client.chat.update({ channel: inc.channel, ts, text, blocks, ...botIdentityArgs() }).then(() => { + mirrorSelfPost(inc.channel, ts, text, { sub: replyThreadTs, editedAt: Date.now() }); + }), + checkpoint: async (ts) => { + if (queuedRunId) await checkpointRunEditRef(queuedRunId, ts); + }, + remove: (ts) => client.chat.delete({ channel: inc.channel, ts }).then(() => {}), + onSurfacePosted: () => ack?.onSurfacePosted(), + onError: (error) => console.error("[slack-plugin] task-list update failed:", (error as Error).message), + }); + } + const settleAck = async (): Promise => { + await ack?.settle().catch(swallowAs("slack: ack settle", undefined)); + }; + { const containerName = inc.kind === "dm" ? actor.displayName?.trim() || undefined : channelName; void mirrorMessageEvent( diff --git a/test/slack-index.integration.test.ts b/test/slack-index.integration.test.ts index 1c0e3307..961e13ae 100644 --- a/test/slack-index.integration.test.ts +++ b/test/slack-index.integration.test.ts @@ -884,6 +884,7 @@ test("a bot-authored stop can abort a live run", async () => { }); assert.deepEqual(f.core.abortedRuns, ["run-active"]); assert.equal(f.core.turns.length, 0); + assert.equal(f.core.ackPicks.length, 0); } finally { await f.stop(); } @@ -897,6 +898,7 @@ test("a Slack Connect mention is refused ephemerally and never mirrored", async await f.app.emitEvent("app_mention", event); assert.equal(f.core.turns.length, 0); assert.equal(f.core.abortedRuns.length, 0); + assert.equal(f.core.ackPicks.length, 0); assert.equal(f.core.ingests.length, 0); assert.equal(f.client.posts.length, 0); assert.equal(f.client.ephemerals.length, 1); From ab6fc291580a6598bbbad92c1120b974336e67a2 Mon Sep 17 00:00:00 2001 From: Josh France <12610835+16francej@users.noreply.github.com> Date: Sun, 16 Aug 2026 12:11:57 -0400 Subject: [PATCH 10/10] Preserve startup synchronization --- src/slack/directory.ts | 4 ++-- src/slack/index.ts | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/slack/directory.ts b/src/slack/directory.ts index 94181f77..baa2624c 100644 --- a/src/slack/directory.ts +++ b/src/slack/directory.ts @@ -507,9 +507,9 @@ export function createDirectory(deps: { const stale = !snap || Date.now() - snap.fetchedAt >= USER_SNAPSHOT_TTL_MS; if (stale && !userSnapshotInFlight) { userSnapshotInFlight = refreshUserSnapshot(client) - .then((s) => { + .then(async (s) => { userSnapshot = s; - if (snap) void pushDirectory(s, client); + await pushDirectory(s, client); return s; }) .finally(() => { diff --git a/src/slack/index.ts b/src/slack/index.ts index eb2e2510..0b3aaa01 100644 --- a/src/slack/index.ts +++ b/src/slack/index.ts @@ -226,7 +226,7 @@ export async function startSlackPlugin( ); } } - await directory.forceDirectorySync(app.client); + await directory.getUserSnapshot(app.client); await app.start(); } catch (err) { stopped = true;