diff --git a/src/api/app-messaging.ts b/src/api/app-messaging.ts index b784da4dd..7cde16089 100644 --- a/src/api/app-messaging.ts +++ b/src/api/app-messaging.ts @@ -302,9 +302,9 @@ export function createMessagingMethods( return found; }, - async upsertDirectory(members) { + async upsertDirectory(members, syncedAt) { const previous = await deps.directory.list(); - await deps.directory.replace(members); + if (!(await deps.directory.replace(members, syncedAt))) return; const present = members.filter((m) => m.type === "internal").map((m) => m.principalId); const presentSet = new Set(present); const removed = previous.map((m) => m.principalId).filter((id) => !presentSet.has(id)); @@ -329,11 +329,11 @@ export function createMessagingMethods( }); } }, - async upsertChannels(channels, channelMembers) { - await deps.directory.replaceChannels(channels, channelMembers); + async upsertChannels(channels, channelMembers, syncedAt) { + await deps.directory.replaceChannels(channels, channelMembers, syncedAt); }, - async upsertGroups(groupMembers) { - await deps.directory.replaceGroups(groupMembers); + async upsertGroups(groupMembers, syncedAt) { + await deps.directory.replaceGroups(groupMembers, syncedAt); }, async setDirectoryWorkspaceUrl(url) { await deps.directory.setWorkspaceUrl(url); diff --git a/src/api/app-types.ts b/src/api/app-types.ts index f2b461ec1..d9ca7106c 100644 --- a/src/api/app-types.ts +++ b/src/api/app-types.ts @@ -349,9 +349,9 @@ export interface App { ackDelivery(id: string, slackApiMs?: number): Promise; ackDeliveryByKey(idempotencyKey: string): Promise; setRunDeliveryState(runId: string, state: RunDeliveryState): Promise; - upsertDirectory(members: DirectoryMember[]): Promise; - upsertChannels(channels: DirectoryChannel[], channelMembers?: ChannelMembership[]): Promise; - upsertGroups(groupMembers: GroupMembership[]): Promise; + upsertDirectory(members: DirectoryMember[], syncedAt?: number): Promise; + upsertChannels(channels: DirectoryChannel[], channelMembers?: ChannelMembership[], syncedAt?: number): Promise; + upsertGroups(groupMembers: GroupMembership[], syncedAt?: number): 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 e2fcba5bd..d71660a38 100644 --- a/src/api/routes/directory.ts +++ b/src/api/routes/directory.ts @@ -4,6 +4,8 @@ import { sendJson } from "../http.ts"; import { audit, isObj, orgScope } from "./shared.ts"; import { type ApiCtx, type Route } from "./route.ts"; +const numOrUndef = (v: unknown): number | undefined => (typeof v === "number" && Number.isFinite(v) ? v : undefined); + async function deactivatePrincipal(ctx: ApiCtx): Promise { const { res, deps } = ctx; if (!deps.identity) return sendJson(res, 404, { error: "not_found" }); @@ -32,6 +34,9 @@ async function pushDirectory(ctx: ApiCtx): Promise { channelMembers?: unknown; groupMembers?: unknown; workspaceUrl?: unknown; + membersSyncedAt?: unknown; + channelsSyncedAt?: unknown; + groupsSyncedAt?: unknown; }; if (!Array.isArray(b.members) && !Array.isArray(b.channels) && !Array.isArray(b.groupMembers)) { return sendJson(res, 400, { @@ -58,7 +63,7 @@ async function pushDirectory(ctx: ApiCtx): Promise { type: m.type, ...(typeof m.slackId === "string" && m.slackId ? { slackId: m.slackId } : {}), })); - await app.upsertDirectory(members); + await app.upsertDirectory(members, numOrUndef(b.membersSyncedAt)); memberCount = members.length; } let channelCount: number | undefined; @@ -73,7 +78,7 @@ async function pushDirectory(ctx: ApiCtx): Promise { isObj(m) && typeof m.channelId === "string" && typeof m.principalId === "string", ) : undefined; - await app.upsertChannels(channels, channelMembers); + await app.upsertChannels(channels, channelMembers, numOrUndef(b.channelsSyncedAt)); channelCount = channels.length; } let groupMemberCount: number | undefined; @@ -82,7 +87,7 @@ 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); + await app.upsertGroups(groupMembers, numOrUndef(b.groupsSyncedAt)); groupMemberCount = groupMembers.length; } return sendJson(res, 200, { diff --git a/src/api/slack-core-client.ts b/src/api/slack-core-client.ts index b385f7aba..8d5c73282 100644 --- a/src/api/slack-core-client.ts +++ b/src/api/slack-core-client.ts @@ -48,6 +48,9 @@ interface DirectoryPush { channelMembers?: Array<{ channelId: string; principalId: string }>; groupMembers?: Array<{ groupId: string; principalId: string }>; workspaceUrl?: string; + membersSyncedAt?: number; + channelsSyncedAt?: number; + groupsSyncedAt?: number; } export interface SlackCoreClient { @@ -275,9 +278,9 @@ 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); - if (body.channels) await deps.app.upsertChannels(body.channels, body.channelMembers); - if (body.groupMembers) await deps.app.upsertGroups(body.groupMembers); + 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.groupMembers) await deps.app.upsertGroups(body.groupMembers, body.groupsSyncedAt); }, claimDeliveries(type, claimMs) { diff --git a/src/core/orchestrator.ts b/src/core/orchestrator.ts index 0e9802b2b..373b496ad 100644 --- a/src/core/orchestrator.ts +++ b/src/core/orchestrator.ts @@ -2300,79 +2300,101 @@ export function createOrchestrator(deps: OrchestratorDeps): Orchestrator { !result.silent ) { const firstTapeWriteFailed = !!result.tapeWriteFailed; - const nudgeHistory = filterHistory( - forModelContext(await deps.sessions.getEntries(session.id), { includeSecurityTainted: false }), - ); - const nudgeTape = tapeRows - ? await deps.sessions - .getTape(session.id) - .then(async (allRows) => { - const rows = filterTapeForAudience(allRows, conversation.audience, scopeId, resolution.orgScopeId); - const sameHarness = rows.every( - (row) => row.kind !== "message" || row.harness === undefined || row.harness === "pi", - ); - const eventsEntitled = tapeEventsEntitled( - rows, - conversation.audience, - scopeId, - resolution.orgScopeId, - ); - const primarySubturnComplete = - primarySubturnEndSeq !== undefined && - rows.some( - (row) => - row.kind === "annotation" && - row.entrySeq === primarySubturnEndSeq && - (row.payload as { subturnEnd?: unknown } | null)?.subturnEnd === true, + // The model already wrote a reply as plain assistant text — deliver that text + // directly instead of nudging it to re-post (a nudge here re-sends near-identical + // text, which surfaces that render assistant entries show twice). + const primaryReply = stripAckPrefix(result.reply ?? "", spineAckText).trim(); + if (primaryReply && defaultDestination && deps.deliveries) { + try { + const directKey = `post:${session.id}:${randomUUID()}`; + await reachEnqueue({ + deliveries: deps.deliveries, + destination: defaultDestination, + text: primaryReply, + idempotencyKey: directKey, + provenance: postProvenance(directKey), + }); + spine.surfaceOutboundCount += 1; + if (input.runId) deps.turnStream?.markSurfacePosted(input.runId); + } catch (e) { + console.error(`[orchestrator] direct reply delivery failed session=${session.id}:`, errMessage(e)); + } + } + if (spine.surfaceOutboundCount === 0) { + const nudgeHistory = filterHistory( + forModelContext(await deps.sessions.getEntries(session.id), { includeSecurityTainted: false }), + ); + const nudgeTape = tapeRows + ? await deps.sessions + .getTape(session.id) + .then(async (allRows) => { + const rows = filterTapeForAudience(allRows, conversation.audience, scopeId, resolution.orgScopeId); + const sameHarness = rows.every( + (row) => row.kind !== "message" || row.harness === undefined || row.harness === "pi", ); - if ( - primaryServedTape && - !firstTapeWriteFailed && - sameHarness && - eventsEntitled && - primarySubturnComplete - ) { - const fold = await rehydrateTape(foldTape(rows)); - if (fold.length && lintFold(fold).ok) return { rows, mode: "serve" as const, fold }; - } - return { rows, mode: "shadow" as const }; - }) - .catch((e) => { - swallow("tape: nudge read", e); - return undefined; - }) - : undefined; - result = await runHarnessTurn( - "[system] You were addressed directly. Reply with the `slack` tool's `post` action, or decline explicitly with stay_silent — ending the turn without either is not allowed here.", - { - ...(turnEnvironment ? { environment: turnEnvironment } : {}), - ...(nudgeTape?.mode !== "serve" && inbound.images.length ? { images: inbound.images } : {}), - }, - { history: nudgeHistory, ...(nudgeTape ? { tape: nudgeTape } : {}) }, - ); - if (firstTapeWriteFailed || result.tapeWriteFailed) result = { ...result, tapeWriteFailed: true }; - if (spine.surfaceOutboundCount === 0 && spine.staySilentReason === undefined && !result.silent) { - const fallback = stripAckPrefix(result.reply ?? "", spineAckText).trim(); - if (fallback && defaultDestination && deps.deliveries) { - try { - const fallbackKey = `post:${session.id}:${randomUUID()}`; - await reachEnqueue({ - deliveries: deps.deliveries, - destination: defaultDestination, - text: fallback, - idempotencyKey: fallbackKey, - provenance: postProvenance(fallbackKey), - }); - spine.surfaceOutboundCount += 1; - if (input.runId) deps.turnStream?.markSurfacePosted(input.runId); - } catch (e) { - console.error( - `[orchestrator] shed-reply fallback delivery failed session=${session.id}:`, - errMessage(e), - ); + const eventsEntitled = tapeEventsEntitled( + rows, + conversation.audience, + scopeId, + resolution.orgScopeId, + ); + const primarySubturnComplete = + primarySubturnEndSeq !== undefined && + rows.some( + (row) => + row.kind === "annotation" && + row.entrySeq === primarySubturnEndSeq && + (row.payload as { subturnEnd?: unknown } | null)?.subturnEnd === true, + ); + if ( + primaryServedTape && + !firstTapeWriteFailed && + sameHarness && + eventsEntitled && + primarySubturnComplete + ) { + const fold = await rehydrateTape(foldTape(rows)); + if (fold.length && lintFold(fold).ok) return { rows, mode: "serve" as const, fold }; + } + return { rows, mode: "shadow" as const }; + }) + .catch((e) => { + swallow("tape: nudge read", e); + return undefined; + }) + : undefined; + result = await runHarnessTurn( + "[system] You were addressed directly. Reply with the `slack` tool's `post` action, or decline explicitly with stay_silent — ending the turn without either is not allowed here.", + { + ...(turnEnvironment ? { environment: turnEnvironment } : {}), + ...(nudgeTape?.mode !== "serve" && inbound.images.length ? { images: inbound.images } : {}), + }, + { history: nudgeHistory, ...(nudgeTape ? { tape: nudgeTape } : {}) }, + ); + if (firstTapeWriteFailed || result.tapeWriteFailed) result = { ...result, tapeWriteFailed: true }; + if (spine.surfaceOutboundCount === 0 && spine.staySilentReason === undefined && !result.silent) { + const fallback = stripAckPrefix(result.reply ?? "", spineAckText).trim(); + if (fallback && defaultDestination && deps.deliveries) { + try { + const fallbackKey = `post:${session.id}:${randomUUID()}`; + await reachEnqueue({ + deliveries: deps.deliveries, + destination: defaultDestination, + text: fallback, + idempotencyKey: fallbackKey, + provenance: postProvenance(fallbackKey), + }); + spine.surfaceOutboundCount += 1; + if (input.runId) deps.turnStream?.markSurfacePosted(input.runId); + } catch (e) { + console.error( + `[orchestrator] shed-reply fallback delivery failed session=${session.id}:`, + errMessage(e), + ); + } + } else { + console.error(`[orchestrator] addressed turn ended silent after nudge session=${session.id}`); } - } else { - console.error(`[orchestrator] addressed turn ended silent after nudge session=${session.id}`); } } } diff --git a/src/directory/directory-store.ts b/src/directory/directory-store.ts index ff32a6112..b3f09fbfe 100644 --- a/src/directory/directory-store.ts +++ b/src/directory/directory-store.ts @@ -41,8 +41,12 @@ export type ChannelResolution = { kind: "one"; channel: DirectoryChannel } | { kind: "ambiguous"; candidates: DirectoryChannel[] } | { kind: "none" }; export interface DirectoryStore { - replace(members: DirectoryMember[]): Promise; - replaceChannels(channels: DirectoryChannel[], channelMembers?: ChannelMembership[]): Promise; + replace(members: DirectoryMember[], syncedAt?: number): Promise; + replaceChannels( + channels: DirectoryChannel[], + channelMembers?: ChannelMembership[], + syncedAt?: number, + ): Promise; list(): Promise; listChannels(): Promise; get(principalId: string): Promise; @@ -51,7 +55,7 @@ export interface DirectoryStore { channelMember(channelId: string, principalId: string): Promise; channelMembership(channelId: string, principalId: string): Promise; channelPrivacy(channelId: string): Promise; - replaceGroups(groupMembers: GroupMembership[]): Promise; + replaceGroups(groupMembers: GroupMembership[], syncedAt?: number): Promise; upsertGroup(groupId: string, principalIds: readonly string[]): Promise; resolveGroupByParticipants(participants: readonly string[]): Promise; groupMember(groupId: string, principalId: string): Promise; @@ -92,6 +96,18 @@ export function createDirectoryStore(): DirectoryStore { let groupMembers: Map> | undefined; let groupsSynced = false; let workspaceUrl: string | undefined; + const syncedAts = new Map(); + + function acceptSync(section: string, syncedAt: number | undefined): boolean { + if (syncedAt === undefined) return true; + const stored = syncedAts.get(section); + if (stored !== undefined && stored > syncedAt) { + console.warn(`[directory] refused stale ${section} swap: stamped ${stored - syncedAt}ms behind`); + return false; + } + syncedAts.set(section, syncedAt); + return true; + } return { async setWorkspaceUrl(url) { @@ -100,10 +116,13 @@ export function createDirectoryStore(): DirectoryStore { async meta() { return { workspaceUrl: workspaceUrl ?? null }; }, - async replace(next) { + async replace(next, syncedAt) { + if (!acceptSync("members", syncedAt)) return false; members = next.filter((m) => m.principalId && m.type === "internal"); + return true; }, - async replaceChannels(nextChannels, nextChannelMembers) { + async replaceChannels(nextChannels, nextChannelMembers, syncedAt) { + if (!acceptSync("channels", syncedAt)) return false; channels = nextChannels.filter((c) => c.channelId && c.name); if (nextChannelMembers !== undefined) { const byChannel = new Map>(); @@ -113,6 +132,7 @@ export function createDirectoryStore(): DirectoryStore { } channelMembers = byChannel; } + return true; }, async channelMember(channelId, principalId) { return channelMembers?.get(channelId)?.has(principalId) ?? false; @@ -128,7 +148,8 @@ export function createDirectoryStore(): DirectoryStore { const channel = channels.find((candidate) => candidate.channelId === channelId); return channel ? channel.isPrivate === true : undefined; }, - async replaceGroups(nextGroupMembers) { + async replaceGroups(nextGroupMembers, syncedAt) { + if (!acceptSync("groups", syncedAt)) return false; const byGroup = new Map>(); for (const m of nextGroupMembers) { if (!m.groupId || !m.principalId) continue; @@ -136,6 +157,7 @@ export function createDirectoryStore(): DirectoryStore { } groupMembers = byGroup; groupsSynced = true; + return true; }, async upsertGroup(groupId, principalIds) { const ids = principalIds.filter(Boolean); @@ -143,6 +165,8 @@ export function createDirectoryStore(): DirectoryStore { const byGroup = groupMembers ?? new Map>(); byGroup.set(groupId, new Set(ids)); groupMembers = byGroup; + const stored = syncedAts.get("groups"); + syncedAts.set("groups", Math.max(stored ?? 0, Date.now())); }, async resolveGroupByParticipants(participants) { const key = groupParticipantsKey(participants); diff --git a/src/directory/postgres-directory-store.ts b/src/directory/postgres-directory-store.ts index f3b808f07..9aae41882 100644 --- a/src/directory/postgres-directory-store.ts +++ b/src/directory/postgres-directory-store.ts @@ -85,6 +85,9 @@ const SCHEMA = [ ALTER TABLE directory_sync ADD COLUMN channel_members_synced BOOLEAN NOT NULL DEFAULT FALSE; END IF; END $$`, + `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`, `CREATE TABLE IF NOT EXISTS directory_meta( org_id TEXT PRIMARY KEY, workspace_url TEXT, @@ -182,30 +185,52 @@ export function createPostgresDirectoryStore(connectionString: string): Director async function swapIfChanged( hashCol: string, hash: string, + syncedAt: number | undefined, write: (client: PoolClient) => Promise, - ): Promise { - await withPgTransaction(await pool(), async (client) => { + ): Promise { + const syncedAtCol = hashCol.replace(/_hash$/, "_synced_at"); + return withPgTransaction(await pool(), async (client) => { await client.query("SELECT pg_advisory_xact_lock(hashtext('directory'), hashtext($1))", [orgId]); - const sync = await client.query(`SELECT ${hashCol} FROM directory_sync WHERE org_id = $1`, [orgId]); - if (sync.rows[0]?.[hashCol] === hash) return; + const sync = await client.query(`SELECT ${hashCol}, ${syncedAtCol} FROM directory_sync WHERE org_id = $1`, [ + orgId, + ]); + const row = sync.rows[0]; + if (syncedAt !== undefined && row?.[syncedAtCol] != null && Number(row[syncedAtCol]) > syncedAt) { + console.warn( + `[directory] refused stale ${hashCol.replace(/_hash$/, "")} swap for ${orgId}: stamped ${Number(row[syncedAtCol]) - syncedAt}ms behind`, + ); + return false; + } + if (row?.[hashCol] === hash) { + if (syncedAt !== undefined) { + await client.query( + `UPDATE directory_sync SET ${syncedAtCol} = GREATEST(COALESCE(${syncedAtCol}, 0), $2) WHERE org_id = $1`, + [orgId, syncedAt], + ); + } + return true; + } await write(client); await client.query( - `INSERT INTO directory_sync (org_id, ${hashCol}, updated_at) VALUES ($1, $2, $3) - ON CONFLICT (org_id) DO UPDATE SET ${hashCol} = EXCLUDED.${hashCol}, updated_at = EXCLUDED.updated_at`, - [orgId, hash, Date.now()], + `INSERT INTO directory_sync (org_id, ${hashCol}, ${syncedAtCol}, updated_at) VALUES ($1, $2, $3, $4) + ON CONFLICT (org_id) DO UPDATE SET ${hashCol} = EXCLUDED.${hashCol}, + ${syncedAtCol} = COALESCE(EXCLUDED.${syncedAtCol}, directory_sync.${syncedAtCol}), + updated_at = EXCLUDED.updated_at`, + [orgId, hash, syncedAt ?? null, Date.now()], ); + return true; }); } return { - async replace(members) { + async replace(members, syncedAt) { const byId = new Map(); for (const m of members) if (m.principalId && m.type === "internal") byId.set(m.principalId, m); const internal = [...byId.values()]; const hash = hashRoster(internal.map((m) => `${m.principalId}|${m.displayName}|${m.type}|${m.slackId ?? ""}`)); - await swapIfChanged("members_hash", hash, async (client) => { + return swapIfChanged("members_hash", hash, syncedAt, async (client) => { await client.query("DELETE FROM directory_members WHERE org_id = $1", [orgId]); if (internal.length) { await client.query( @@ -224,7 +249,7 @@ export function createPostgresDirectoryStore(connectionString: string): Director }); }, - async replaceChannels(channels, channelMembers) { + async replaceChannels(channels, channelMembers, syncedAt) { const byId = new Map(); for (const c of channels) if (c.channelId && c.name) byId.set(c.channelId, c); const list = [...byId.values()]; @@ -237,7 +262,7 @@ export function createPostgresDirectoryStore(connectionString: string): Director : ["members:known", ...membershipRows.map((m) => `m:${m.channelId}|${m.principalId}`)]; const hash = hashRoster([...channelsPart, ...membersPart]); - await swapIfChanged("channels_hash", hash, async (client) => { + const applied = await swapIfChanged("channels_hash", hash, syncedAt, async (client) => { await client.query("DELETE FROM directory_channels WHERE org_id = $1", [orgId]); if (list.length) { await client.query( @@ -263,9 +288,10 @@ export function createPostgresDirectoryStore(connectionString: string): Director } } }); - if (membershipRows !== undefined) { + if (applied && membershipRows !== undefined) { await q("UPDATE directory_sync SET channel_members_synced = TRUE WHERE org_id = $1", [orgId]); } + return applied; }, async setWorkspaceUrl(url) { @@ -311,7 +337,7 @@ export function createPostgresDirectoryStore(connectionString: string): Director return rows.length > 0 ? (rows[0]!.is_private as boolean) : undefined; }, - async replaceGroups(groupMembers) { + async replaceGroups(groupMembers, syncedAt) { const rows = dedupPairs( groupMembers, (m) => m.groupId, @@ -319,7 +345,7 @@ export function createPostgresDirectoryStore(connectionString: string): Director ); const hash = hashRoster(rows.map((m) => `${m.groupId}|${m.principalId}`)); - await swapIfChanged("groups_hash", hash, async (client) => { + return swapIfChanged("groups_hash", hash, syncedAt, async (client) => { await client.query("DELETE FROM directory_group_members WHERE org_id = $1", [orgId]); if (rows.length) { await client.query( @@ -346,8 +372,10 @@ export function createPostgresDirectoryStore(connectionString: string): Director orgId, ]); await client.query( - `INSERT INTO directory_sync (org_id, groups_hash, updated_at) VALUES ($1, $2, $3) - ON CONFLICT (org_id) DO UPDATE SET groups_hash = EXCLUDED.groups_hash, updated_at = EXCLUDED.updated_at`, + `INSERT INTO directory_sync (org_id, groups_hash, groups_synced_at, updated_at) VALUES ($1, $2, $3, $3) + ON CONFLICT (org_id) DO UPDATE SET groups_hash = EXCLUDED.groups_hash, + groups_synced_at = GREATEST(COALESCE(directory_sync.groups_synced_at, 0), EXCLUDED.groups_synced_at), + updated_at = EXCLUDED.updated_at`, [orgId, hashRoster(all.rows.map((r) => `${r.group_id}|${r.principal_id}`)), Date.now()], ); }); diff --git a/src/harness/mock-harness.ts b/src/harness/mock-harness.ts index d1d8f0755..d9bbc39f8 100644 --- a/src/harness/mock-harness.ts +++ b/src/harness/mock-harness.ts @@ -132,6 +132,7 @@ export function createMockHarness(): Harness { }); let reply: string; + let muteReply = false; let usedTool = false; let silent = false; const collected: Array<{ @@ -189,6 +190,10 @@ export function createMockHarness(): Harness { usedTool = true; reply = r.ok ? "(posted after nudge)" : `[not sent] ${r.message ?? "failed"}`; } + } else if (command0.startsWith("!shedmute")) { + shedSessions.add(turn.session.id); + reply = "worklog: did the thing but never posted"; + muteReply = true; } else if (command0.startsWith("!shed")) { shedSessions.add(turn.session.id); reply = "worklog: did the thing but never posted"; @@ -653,6 +658,7 @@ export function createMockHarness(): Harness { modelCalls, } : { reply, modelCalls }; + if (muteReply) base.reply = ""; return { ...base, ...(silent ? { silent: true as const } : {}), cacheUsage }; }, diff --git a/src/slack/directory.ts b/src/slack/directory.ts index b4d1379e0..a58ada0b8 100644 --- a/src/slack/directory.ts +++ b/src/slack/directory.ts @@ -80,6 +80,7 @@ export interface Directory { 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; } @@ -274,14 +275,18 @@ export function createDirectory(deps: { channelMembers: ChannelMembershipRow[]; groupMembers?: GroupMembershipRow[]; fetchedAt: number; + groupsFetchedAt?: number; } | undefined; let knownPublicChannelSet = new Set(); + const seenGroupIds = new Set(); async function fetchChannels(client: any): Promise<{ channels: ChannelRow[]; channelMembers: ChannelMembershipRow[]; groupMembers?: GroupMembershipRow[]; + fetchedAt: number; + groupsFetchedAt?: number; } | null> { let listed: { publicChannels: ChannelRow[]; privateChannels: PrivateChannelRef[] }; try { @@ -295,19 +300,25 @@ export function createDirectory(deps: { if (!fresh) { const computed = await computePrivateChannelMembership(client, listed.privateChannels); let groupMembers: GroupMembershipRow[] | undefined; + let groupsFetchedAt: number | undefined; try { - groupMembers = await computeGroupMembership(client, await listBotGroupDms(client)); + const groupIds = await listBotGroupDms(client); + for (const id of groupIds) seenGroupIds.add(id); + groupMembers = await computeGroupMembership(client, groupIds); + groupsFetchedAt = Date.now(); } catch (err) { console.error("[slack-plugin] group-DM list failed:", (err as Error).message); groupMembers = privateChannelsCache?.groupMembers; + groupsFetchedAt = privateChannelsCache?.groupsFetchedAt; } - privateChannelsCache = { ...computed, groupMembers, fetchedAt: Date.now() }; + privateChannelsCache = { ...computed, groupMembers, groupsFetchedAt, fetchedAt: Date.now() }; } const priv = privateChannelsCache ?? { channels: [], channelMembers: [], fetchedAt: 0 }; return { channels: [...listed.publicChannels, ...priv.channels], channelMembers: priv.channelMembers, - ...(priv.groupMembers ? { groupMembers: priv.groupMembers } : {}), + fetchedAt: priv.fetchedAt, + ...(priv.groupMembers ? { groupMembers: priv.groupMembers, groupsFetchedAt: priv.groupsFetchedAt } : {}), }; } @@ -328,11 +339,15 @@ export function createDirectory(deps: { try { await core.pushDirectory({ members, + membersSyncedAt: snap.fetchedAt, ...(fetched ? { channels: fetched.channels, channelMembers: fetched.channelMembers, - ...(fetched.groupMembers ? { groupMembers: fetched.groupMembers } : {}), + channelsSyncedAt: fetched.fetchedAt, + ...(fetched.groupMembers + ? { groupMembers: fetched.groupMembers, groupsSyncedAt: fetched.groupsFetchedAt } + : {}), } : {}), ...(ids.ownWorkspaceUrl ? { workspaceUrl: ids.ownWorkspaceUrl } : {}), @@ -378,6 +393,12 @@ export function createDirectory(deps: { return coalescedDirectorySync(); } + function syncForUnseenGroup(client: any, groupId: string): void { + if (seenGroupIds.has(groupId)) return; + seenGroupIds.add(groupId); + void forceDirectorySync(client).catch(swallowAs("slack: unseen group-DM directory sync", undefined)); + } + async function classifyUserCached(client: any, userId: string): Promise { const cached = userCache.get(userId); if (cached) return { ...cached, ok: true }; @@ -471,6 +492,7 @@ export function createDirectory(deps: { 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 d1cc696bd..b964e3f79 100644 --- a/src/slack/events.ts +++ b/src/slack/events.ts @@ -33,7 +33,7 @@ export function registerSlackEvents( const { handler, mirror, directory, ids, deduper } = deps; const { dispatch, handleReactionEvent, botHasStakeInThread } = handler; const { mirrorMessageEvent, pushSurfaceEvents } = mirror; - const { knownPublicChannels, forceDirectorySync } = directory; + const { knownPublicChannels, syncForUnseenGroup, forceDirectorySync } = directory; app.event("app_mention", async ({ event, body, client, context }: any) => { const e = event as any; @@ -126,6 +126,7 @@ export function registerSlackEvents( } if (m.channel_type === "channel" || m.channel_type === "group" || m.channel_type === "mpim") { + if (m.channel_type === "mpim" && m.channel) syncForUnseenGroup(client, String(m.channel)); const threadReply = isThreadReply(m); const isMention = mentionsBot(m.text ?? "", ids.botUserId); const willDispatch = threadReply && !isMention && (await botHasStakeInThread(client, m.channel, m.thread_ts)); diff --git a/test/directory-store.test.ts b/test/directory-store.test.ts index a0d292cd5..d394a7aff 100644 --- a/test/directory-store.test.ts +++ b/test/directory-store.test.ts @@ -171,6 +171,32 @@ describe("group-DM (mpim) membership (addressed by participant set, §10)", () = assert.equal((await d.resolveGroupByParticipants(["U-alice", "U-carol", "U-sam"])).kind, "none"); assert.equal((await d.resolveGroupByParticipants(["U-alice"])).kind, "one"); }); + + it("a swap stamped older than the stored snapshot is refused, so a stale instance cannot clobber a fresh sync", async () => { + const d = createDirectoryStore(); + assert.equal(await d.replaceGroups([{ groupId: "G-new", principalId: "U-alice" }], 2000), true); + assert.equal(await d.replaceGroups([], 1000), false); + assert.equal(await d.groupMember("G-new", "U-alice"), true); + assert.equal(await d.replaceGroups([], 3000), true); + assert.equal(await d.groupMember("G-new", "U-alice"), false); + }); + + it("an unstamped swap keeps today's last-write-wins behaviour", async () => { + const d = createDirectoryStore(); + await d.replaceGroups([{ groupId: "G-new", principalId: "U-alice" }], 2000); + assert.equal(await d.replaceGroups([], undefined), true); + assert.equal(await d.groupMember("G-new", "U-alice"), false); + }); + + 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); + assert.equal(await d.replace([], 1000), false); + assert.equal((await d.list()).length, 1); + assert.equal(await d.replaceChannels([{ channelId: "C-1", name: "eng" }], undefined, 2000), true); + assert.equal(await d.replaceChannels([], undefined, 1000), false); + assert.equal((await d.listChannels()).length, 1); + }); }); describe("private-channel membership (authorizes private-channel sends, §10)", () => { diff --git a/test/postgres-directory-store.test.ts b/test/postgres-directory-store.test.ts index 49cceec4a..0111dec3b 100644 --- a/test/postgres-directory-store.test.ts +++ b/test/postgres-directory-store.test.ts @@ -340,3 +340,41 @@ test("pg directory: workspace URL survives roster swaps and reads back per org", await store.setWorkspaceUrl("https://acme2.slack.com"); assert.deepEqual(await store.meta(), { workspaceUrl: "https://acme2.slack.com" }); }); + +test("pg directory: a swap stamped older than the stored snapshot is refused", { skip }, async () => { + const store = createPostgresDirectoryStore(URL!); + + assert.equal(await store.replaceGroups([{ groupId: "G-fresh", principalId: "U-alice" }], 2000), true); + assert.equal(await store.replaceGroups([], 1000), false, "a stale instance's swap must not clobber a fresh sync"); + assert.equal(await store.groupMember("G-fresh", "U-alice"), true); + assert.equal(await store.replaceGroups([], 3000), true); + assert.equal(await store.groupMember("G-fresh", "U-alice"), false); + + assert.equal(await store.replace([{ principalId: "U-fresh", displayName: "Fresh", type: "internal" }], 2000), true); + assert.equal(await store.replace([], 1000), false); + assert.notEqual(await store.get("U-fresh"), null); + + assert.equal(await store.replaceChannels([{ channelId: "C-fresh", name: "fresh" }], undefined, 2000), true); + assert.equal(await store.replaceChannels([], undefined, 1000), false); + assert.equal( + (await store.listChannels()).some((c) => c.channelId === "C-fresh"), + true, + ); + + assert.equal(await store.replaceChannels([], undefined, undefined), true, "an unstamped swap keeps last-write-wins"); + assert.equal((await store.listChannels()).length, 0); +}); + +test( + "pg directory: an identical push still advances the stamp, so ordering survives content-idempotent pushes", + { skip }, + async () => { + const store = createPostgresDirectoryStore(URL!); + const roster = [{ groupId: "G-idem", principalId: "U-alice" }]; + + assert.equal(await store.replaceGroups(roster, 12000), true); + assert.equal(await store.replaceGroups(roster, 13000), true); + assert.equal(await store.replaceGroups([], 12500), false, "a swap older than the newest snapshot seen must lose"); + assert.equal(await store.groupMember("G-idem", "U-alice"), true); + }, +); diff --git a/test/slack-index.integration.test.ts b/test/slack-index.integration.test.ts index 3bde168c5..d13b627a5 100644 --- a/test/slack-index.integration.test.ts +++ b/test/slack-index.integration.test.ts @@ -23,6 +23,8 @@ class FakeSlackClient { readonly membersByChannel = new Map(); readonly messagesByChannel = new Map(); readonly membershipFailures = new Set(); + groupListings = 0; + failGroupListing = false; private postSequence = 0; readonly auth = { @@ -110,6 +112,10 @@ class FakeSlackClient { } if (method === "conversations.list") { const types = String(args.types ?? ""); + if (types === "mpim") { + this.groupListings++; + if (this.failGroupListing) throw new Error("missing mpim:read"); + } yield { channels: [...this.channelsById.values()].filter((c) => (types === "mpim" ? c.is_mpim : !c.is_mpim)), }; @@ -647,6 +653,82 @@ test("a group-DM thread-follow runs unprompted yet attests its author's liveness } }); +test("a message from an unseen group DM resyncs the directory so it becomes addressable", async () => { + const f = await fixture(); + try { + f.client.channelsById.set("G9", { id: "G9", name: "", is_member: true, is_private: true, is_mpim: true }); + f.client.membersByChannel.set("G9", ["U1", "U2", "UBOT"]); + const listedBefore = f.client.groupListings; + await f.app.emitMessage({ channel: "G9", channel_type: "mpim", user: "U1", text: "hi", ts: "400.1" }); + await waitFor(() => f.client.groupListings > listedBefore); + await waitFor(() => (f.core.directories.at(-1)?.groupMembers ?? []).some((g: any) => g.groupId === "G9")); + assert.deepEqual( + f.core.directories + .at(-1) + .groupMembers.filter((g: any) => g.groupId === "G9") + .map((g: any) => g.principalId) + .sort(), + ["U1", "U2"], + "the new group's internal roster reaches core, bot excluded", + ); + + const listedAfter = f.client.groupListings; + await f.app.emitMessage({ channel: "G9", channel_type: "mpim", user: "U1", text: "again", ts: "400.2" }); + await new Promise((resolve) => setTimeout(resolve, 20)); + assert.equal(f.client.groupListings, listedAfter, "a group DM already seen does not resync on every message"); + } finally { + await f.stop(); + } +}); + +test("a failed group listing pushes its fallback rows under the OLD stamp, never a fresh one", async () => { + const f = await fixture(); + try { + f.client.channelsById.set("G7", { id: "G7", name: "", is_member: true, is_private: true, is_mpim: true }); + f.client.membersByChannel.set("G7", ["U1", "U2", "UBOT"]); + await f.app.emitMessage({ channel: "G7", channel_type: "mpim", user: "U1", text: "hi", ts: "402.1" }); + await waitFor(() => + f.core.directories.some((d: any) => (d.groupMembers ?? []).some((g: any) => g.groupId === "G7")), + ); + const goodStamp = f.core.directories.findLast((d: any) => d.groupsSyncedAt !== undefined).groupsSyncedAt; + assert.ok(goodStamp > 0); + + f.client.failGroupListing = true; + f.client.channelsById.set("G6", { id: "G6", name: "", is_member: true, is_private: true, is_mpim: true }); + await f.app.emitMessage({ channel: "G6", channel_type: "mpim", user: "U1", text: "hi", ts: "402.2" }); + await waitFor(() => f.core.directories.findLast((d: any) => d.channels)?.channelsSyncedAt > goodStamp); + const last = f.core.directories.findLast((d: any) => d.channels); + assert.equal( + last.groupMembers, + undefined, + "a failed group listing must omit the groups section, never ship rows under a fresh stamp", + ); + } 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 { + f.client.channelsById.set("G8", { id: "G8", name: "", is_member: true, is_private: true, is_mpim: true }); + f.client.membersByChannel.set("G8", ["U1", "U2", "UBOT"]); + f.client.failGroupListing = true; + const listedBefore = f.client.groupListings; + for (const ts of ["401.1", "401.2", "401.3"]) { + await f.app.emitMessage({ channel: "G8", channel_type: "mpim", user: "U1", text: "hi", ts }); + await new Promise((resolve) => setTimeout(resolve, 20)); + } + assert.equal( + f.client.groupListings - listedBefore, + 1, + "a failing listing must not make every message trigger another full sync", + ); + } finally { + await f.stop(); + } +}); + test("a peer bot's thread reply dispatches without attesting liveness", async () => { const f = await fixture(); try { diff --git a/test/surface-spine-routing.test.ts b/test/surface-spine-routing.test.ts index 0023c07f9..5e47b50e0 100644 --- a/test/surface-spine-routing.test.ts +++ b/test/surface-spine-routing.test.ts @@ -305,15 +305,41 @@ test("addressed + no post → exactly one nudge → the agent posts on the conti } }); -test("addressed + STILL no post after the nudge → the final worklog text is delivered as the fallback", async () => { +test("addressed + no post but a final text reply → the reply is delivered directly, no nudge", async () => { const built = freshApp(); built.runtime.start(); try { await built.app.turn(mention("!shed", "C-shed", "710.1")); + const direct = await pollFor(built.deliveries, (d) => d.text === "worklog: did the thing but never posted"); + assert.ok(direct, "the final text reply was delivered directly"); + assert.equal(direct.destination.target, "slack:C-shed:710.1", "delivered to the addressed conversation"); + const session = await built.sessions.getByThread("ch:C-shed:710.1"); + const requests = await built.sessions.listLlmRequests(session!.id); + assert.ok( + !requests.some((r) => JSON.stringify(r.request).includes("[system] You were addressed directly")), + "no nudge model call — the existing reply text is delivered as-is", + ); + await sleep(300); + const all = (await built.deliveries.pending("slack")) as any[]; + assert.equal( + all.filter((d) => d.text === "worklog: did the thing but never posted").length, + 1, + "the reply delivers once", + ); + } finally { + await built.runtime.stop(); + } +}); + +test("addressed + STILL no post after the nudge → the nudge turn's text is delivered as the fallback", async () => { + const built = freshApp(); + built.runtime.start(); + try { + await built.app.turn(mention("!shedmute", "C-shedmute", "710.3")); const fallback = await pollFor(built.deliveries, (d) => d.text === "worklog: did the thing but never posted"); assert.ok(fallback, "the shed reply was delivered as the fallback"); - assert.equal(fallback.destination.target, "slack:C-shed:710.1", "delivered to the addressed conversation"); - const session = await built.sessions.getByThread("ch:C-shed:710.1"); + assert.equal(fallback.destination.target, "slack:C-shedmute:710.3", "delivered to the addressed conversation"); + const session = await built.sessions.getByThread("ch:C-shedmute:710.3"); const nudgeRequest = (await built.sessions.listLlmRequests(session!.id)).at(-1)!.request as { messages?: Array<{ role?: string; content?: string }>; }; @@ -343,7 +369,7 @@ test("reply-or-decline nudge preserves the trigger image and environment", async const image = Buffer.from([0x89, 0x50, 0x4e, 0x47, 0x0d, 0x0a, 0x1a, 0x0a]); const { blobId } = await built.blobTransfer.put(image); await built.app.turn({ - ...mention("!shed", "C-nudge-image", "710.2"), + ...mention("!shedmute", "C-nudge-image", "710.2"), conversationHeader: "QA-IMAGE-ENVIRONMENT", attachments: [{ name: "qa.png", mimetype: "image/png", sizeBytes: image.length, blobId }], }); @@ -384,7 +410,7 @@ test("nudge tape reread failure falls back to refreshed history, never the stale return originalGetTape(sessionId); }; - await built.app.turn(mention("!shed", "C-nudge-read", "711.1")); + await built.app.turn(mention("!shedmute", "C-nudge-read", "711.1")); assert.ok(await pollFor(built.deliveries, (d) => d.text === "worklog: did the thing but never posted")); const session = await built.sessions.getByThread("ch:C-nudge-read:711.1"); const nudgeRequest = (await built.sessions.listLlmRequests(session!.id)).at(-1)!.request as { @@ -410,8 +436,8 @@ test("addressed spine turn: the first text block posts immediately as the ack wh const ack = await pollFor(built.deliveries, (d) => d.text === "On it — checking the deploy logs."); assert.ok(ack, "the first block was harvested and enqueued while the tool ran"); assert.equal(ack.destination.target, "slack:C-ack:720.1", "the ack lands in the addressed conversation"); - const posted = await pollFor(built.deliveries, (d) => d.text === "nudged reply"); - assert.ok(posted, "the ack did not satisfy the reply contract"); + const posted = await pollFor(built.deliveries, (d) => d.text === "All clear — nothing broke."); + assert.ok(posted, "the trailing reply text is delivered (the ack alone did not satisfy the reply contract)"); } finally { await built.runtime.stop(); } diff --git a/test/tape-nudge-continuation.test.ts b/test/tape-nudge-continuation.test.ts index a337485ff..4b04b2666 100644 --- a/test/tape-nudge-continuation.test.ts +++ b/test/tape-nudge-continuation.test.ts @@ -153,7 +153,13 @@ async function runScenario( entrySeq: finalEntry.seq, }); } - return { reply, modelCalls: 1, ...(failTapeMessage ? { tapeWriteFailed: true } : {}) }; + // A "needs nudge" turn ends with NO final reply text (like a real turn ending on + // tool calls) — a text-bearing ending is now delivered directly, without a nudge. + return { + reply: turn.input === "needs nudge" ? "" : reply, + modelCalls: 1, + ...(failTapeMessage ? { tapeWriteFailed: true } : {}), + }; }, async screenSecurity() { return { decision: "auto" as const };