diff --git a/cli/proof/677-reresolve-proof.png b/cli/proof/677-reresolve-proof.png new file mode 100644 index 0000000..2c6f219 Binary files /dev/null and b/cli/proof/677-reresolve-proof.png differ diff --git a/cli/src/slack-reresolve.test.ts b/cli/src/slack-reresolve.test.ts new file mode 100644 index 0000000..5396bcc --- /dev/null +++ b/cli/src/slack-reresolve.test.ts @@ -0,0 +1,95 @@ +import { test, describe } from "node:test"; +import { strict as assert } from "node:assert"; +import { matchPendingChannels, PendingAgent } from "./slack/reresolve.js"; +import type { SlackChannelInfo } from "./slack/client.js"; + +const chan = (name: string, id: string, isMember: boolean, isPrivate = false): SlackChannelInfo => ({ + id, + name, + isMember, + isPrivate, +}); +const p = (slug: string, channel: string): PendingAgent => ({ slug, channel }); + +describe("matchPendingChannels (the #677 self-heal core)", () => { + test("public channel that exists but the bot is NOT a member stays pending", () => { + // The #677 trap: conversations.list returns a public channel the moment it + // exists, before the invite. Mirroring it then would post to a channel the + // bot can't write to. Must NOT resolve until is_member flips true. + const { resolved, stillPending } = matchPendingChannels( + [p("kanban-keeper", "#agent-kanban-keeper")], + [chan("agent-kanban-keeper", "C1", false)], + ); + assert.equal(resolved.length, 0); + assert.deepEqual( + stillPending.map((a) => a.slug), + ["kanban-keeper"], + ); + }); + + test("resolves — with the channel id — once the bot becomes a member", () => { + const { resolved, stillPending } = matchPendingChannels( + [p("kanban-keeper", "#agent-kanban-keeper")], + [chan("agent-kanban-keeper", "C1", true)], + ); + assert.deepEqual(resolved, [{ slug: "kanban-keeper", channelId: "C1" }]); + assert.equal(stillPending.length, 0); + }); + + test("a channel absent from the snapshot stays pending", () => { + const { resolved, stillPending } = matchPendingChannels([p("x", "not-created-yet")], [chan("other", "C9", true)]); + assert.equal(resolved.length, 0); + assert.equal(stillPending.length, 1); + }); + + test("private channel (only listed once a member) resolves", () => { + // Slack only returns a private channel in conversations.list when the bot is + // a member, so its presence with isMember=true is the normal resolve path. + const { resolved } = matchPendingChannels([p("sec", "#private-sec")], [chan("private-sec", "CP", true, true)]); + assert.deepEqual(resolved, [{ slug: "sec", channelId: "CP" }]); + }); + + test("a raw channel id in config is taken as-is (operator-wired; membership is theirs)", () => { + const { resolved, stillPending } = matchPendingChannels([p("x", "C0123ABC")], []); + assert.deepEqual(resolved, [{ slug: "x", channelId: "C0123ABC" }]); + assert.equal(stillPending.length, 0); + }); + + test("'#name' and bare 'name' resolve identically", () => { + const withHash = matchPendingChannels([p("a", "#foo")], [chan("foo", "C1", true)]); + const bare = matchPendingChannels([p("a", "foo")], [chan("foo", "C1", true)]); + assert.deepEqual(withHash.resolved, bare.resolved); + }); + + test("mixed roster in ONE snapshot: members resolve, non-members and absent ones pend", () => { + const { resolved, stillPending } = matchPendingChannels( + [p("a", "aa"), p("b", "bb"), p("c", "cc")], + [chan("aa", "CA", true), chan("bb", "CB", false)], // cc absent entirely + ); + assert.deepEqual(resolved, [{ slug: "a", channelId: "CA" }]); + assert.deepEqual( + stillPending.map((a) => a.slug).sort(), + ["b", "c"], + ); + }); + + test("promote-once: resolved agents are removed from stillPending, so the next pass can't re-resolve (no double-tail)", () => { + let pending: PendingAgent[] = [p("a", "aa"), p("b", "bb")]; + // pass 1: only aa's channel is ready + let r = matchPendingChannels(pending, [chan("aa", "CA", true)]); + pending = r.stillPending; + assert.deepEqual(r.resolved, [{ slug: "a", channelId: "CA" }]); + assert.deepEqual(pending.map((x) => x.slug), ["b"]); + // pass 2: aa is STILL in the (now larger) snapshot, but it left `pending`, + // so it is not resolved a second time — only the newly-ready bb is. + r = matchPendingChannels(pending, [chan("aa", "CA", true), chan("bb", "CB", true)]); + assert.deepEqual(r.resolved, [{ slug: "b", channelId: "CB" }]); + assert.equal(r.stillPending.length, 0); + }); + + test("empty pending set resolves to nothing (loop can stop)", () => { + const { resolved, stillPending } = matchPendingChannels([], [chan("aa", "CA", true)]); + assert.equal(resolved.length, 0); + assert.equal(stillPending.length, 0); + }); +}); diff --git a/cli/src/slack/bridge.ts b/cli/src/slack/bridge.ts index ff14cd9..3351f7f 100644 --- a/cli/src/slack/bridge.ts +++ b/cli/src/slack/bridge.ts @@ -1,7 +1,8 @@ import { existsSync, statSync, openSync, readSync, closeSync } from "node:fs"; import { join } from "node:path"; import { SocketModeClient } from "@slack/socket-mode"; -import { SlackClient } from "./client.js"; +import { SlackClient, SlackChannelInfo } from "./client.js"; +import { matchPendingChannels, PendingAgent } from "./reresolve.js"; import { routeSlackMessage, prefixAuthor, ChannelMapping } from "./inbound.js"; import { formatTranscriptLines, formatCodexRolloutLines, TERMINAL_STOP_REASONS } from "./format.js"; import { loadAgentsConfig } from "../agents/config.js"; @@ -217,12 +218,14 @@ export async function runSlackBridge(opts: BridgeOptions): Promise { list.splice(i, 1); // consume so a genuine resend later is not swallowed return true; }; - for (const a of agents) { - const channelId = await client.resolveChannelId(a.slackChannel!); - if (!channelId) { - console.error(`Slack channel not found for ${a.slug}: ${a.slackChannel}`); - continue; - } + // Build a tail + channel→agent mapping for one agent whose channel is known + // and joinable. Used for agents resolved at startup AND, later, for agents + // whose channel only becomes usable after the bridge started (the pending + // re-resolution loop below). Appends live to `tails`/`mapping`, which the poll + // loop and inbound router read by reference, so a late add needs no restart. + const agentBySlug = new Map(agents.map((a) => [a.slug, a] as const)); + const buildTail = (a: (typeof agents)[number], channelId: string): void => { + if (tails.some((t) => t.slug === a.slug)) return; // promote-once: never double-tail mapping[channelId] = a.slug; const runtime = (a.runtime ?? "claude") as Runtime; const sessionId = agentIdentity(a.slug, runtime).sessionId; @@ -230,6 +233,21 @@ export async function runSlackBridge(opts: BridgeOptions): Promise { const path = runtime === "codex" ? findCodexRollout(cwd) : findSessionJsonl(sessionId); // Start at EOF so we mirror only new activity, not the whole backlog. tails.push({ slug: a.slug, runtime, sessionId, cwd, channelId, path, offset: path ? statSync(path).size : 0 }); + }; + + // Resolve every agent against ONE conversations.list snapshot, gating on bot + // MEMBERSHIP: a public channel is listed the moment it exists — before the bot + // is invited — so mirroring on mere presence would post into a channel the bot + // can't write to and gets no events from (a silent agent — exactly #677). + // Agents that don't resolve (channel absent, or bot not a member yet) go to + // `pending` and are retried by the loop below, so a channel created/invited + // AFTER this restart is picked up with no manual restart. + let pending: PendingAgent[] = agents.map((a) => ({ slug: a.slug, channel: a.slackChannel! })); + { + const snapshot = await client.listChannels(); + const { resolved, stillPending } = matchPendingChannels(pending, snapshot); + pending = stillPending; + for (const r of resolved) buildTail(agentBySlug.get(r.slug)!, r.channelId); } // Per-agent active "working…" pill. Set on each tool/thinking post, cleared @@ -497,10 +515,14 @@ export async function runSlackBridge(opts: BridgeOptions): Promise { // avoid send-keys-ing into a session that does not have this UI. const pickerByAgent = new Map(); const PICKER_POLL_MS = 1000; - const claudeAgents = tails.filter((t) => t.runtime === "claude"); - if (claudeAgents.length) { + // Schedule unconditionally and iterate `tails` live each tick so a Claude + // agent whose channel resolves AFTER startup (pending loop below) still gets + // pickers — a once-built `tails.filter(...)` snapshot would skip it, and zero + // Claude agents at boot would mean the interval was never scheduled at all. + { setInterval(async () => { - for (const t of claudeAgents) { + for (const t of tails) { + if (t.runtime !== "claude") continue; const pane = captureTmuxPane(t.slug); if (!pane) continue; const picker = parsePicker(pane); @@ -708,8 +730,58 @@ export async function runSlackBridge(opts: BridgeOptions): Promise { } } }); + // ── Pending-channel re-resolution: the #677 self-heal ─────────────────── + // Retry pending agents against a fresh snapshot on a SELF-RESCHEDULING timer + // (not setInterval — overlapping ticks under Slack rate-limiting could both + // see an agent pending and double-append its tail). The chain re-arms only + // while agents remain pending, so it costs nothing once all are mirrored, and + // pulls ONE conversations.list snapshot per pass shared across all pending + // agents. A channel created (or the bot invited) after this restart is picked + // up within one interval — no manual `systemctl restart` (issue #677). + const RESOLVE_RETRY_MS = 60_000; + const resolvePendingOnce = async (): Promise => { + if (!pending.length) return; + let snapshot: SlackChannelInfo[]; + try { + snapshot = await client.listChannels(); + } catch (e) { + console.error("pending-resolve listChannels failed (will retry):", e); + return; + } + const { resolved, stillPending } = matchPendingChannels(pending, snapshot); + pending = stillPending; + for (const r of resolved) { + const a = agentBySlug.get(r.slug); + if (!a) continue; + buildTail(a, r.channelId); + console.error(`now mirroring ${r.slug}: channel ${a.slackChannel} became reachable (no restart).`); + } + }; + const schedulePendingResolve = (): void => { + if (!pending.length) return; + setTimeout(() => { + // Always re-arm (while agents remain pending) even if a pass throws — an + // unexpected error in one pass must not silently kill the self-heal chain. + resolvePendingOnce() + .catch((e) => console.error("pending-resolve pass failed (will retry):", e)) + .finally(() => schedulePendingResolve()); + }, RESOLVE_RETRY_MS); + }; + if (pending.length) { + // One log line for the whole pending set (not one per agent per tick) so a + // permanently-misconfigured channel doesn't spam the journal every pass. + console.error( + `${pending.length} agent(s) awaiting a reachable Slack channel (created + bot invited): ${pending + .map((p) => p.slug) + .join(", ")}. Retrying every ${RESOLVE_RETRY_MS / 1000}s.`, + ); + schedulePendingResolve(); + } + await socket.start(); - console.error(`Slack bridge connected. Mirroring ${tails.length} agent(s).`); + console.error( + `Slack bridge connected. Mirroring ${tails.length} agent(s)${pending.length ? `; ${pending.length} pending channel(s).` : "."}`, + ); // Drop attachments older than the retention window so the inbox does not // grow without bound. We sweep on startup and then every hour. Override the diff --git a/cli/src/slack/client.ts b/cli/src/slack/client.ts index 366a587..8f8f5f3 100644 --- a/cli/src/slack/client.ts +++ b/cli/src/slack/client.ts @@ -1,5 +1,15 @@ import { WebClient } from "@slack/web-api"; +/// One channel from a `conversations.list` snapshot. `isMember` reflects whether +/// THIS bot token is in the channel — the bridge gates mirroring on it because a +/// public channel is listed before the bot is invited (see matchPendingChannels). +export interface SlackChannelInfo { + id: string; + name: string; + isMember: boolean; + isPrivate: boolean; +} + /// Thin wrapper over the Slack Web API for what the bridge needs: identify the /// bot, post messages, and resolve channel names to ids. export class SlackClient { @@ -96,4 +106,29 @@ export class SlackClient { } while (cursor); return undefined; } + + /// Page through every channel the bot can see in one pass and return a flat + /// snapshot (id + name + membership). Public channels are returned regardless + /// of membership (with isMember=false until the bot is invited); private + /// channels only where the bot is already a member. The bridge calls this once + /// per re-resolution pass and matches ALL pending agents against the single + /// snapshot, rather than a paginated lookup per agent. + async listChannels(): Promise { + const out: SlackChannelInfo[] = []; + let cursor: string | undefined; + do { + const r = await this.web.conversations.list({ + types: "public_channel,private_channel", + limit: 1000, + cursor, + }); + for (const c of (r.channels ?? []) as any[]) { + if (c?.id && typeof c.name === "string") { + out.push({ id: c.id, name: c.name, isMember: !!c.is_member, isPrivate: !!c.is_private }); + } + } + cursor = r.response_metadata?.next_cursor || undefined; + } while (cursor); + return out; + } } diff --git a/cli/src/slack/reresolve.ts b/cli/src/slack/reresolve.ts new file mode 100644 index 0000000..f901807 --- /dev/null +++ b/cli/src/slack/reresolve.ts @@ -0,0 +1,54 @@ +import type { SlackChannelInfo } from "./client.js"; + +/// A roster agent that wants Slack mirroring but whose channel is not yet +/// usable — the channel does not exist yet, or the bot has not been invited. +/// Held in the bridge's `pending` set and retried against fresh channel +/// snapshots until it becomes mirrorable, so a channel created (or a bot +/// invited) AFTER the bridge last started is picked up without a restart. +export interface PendingAgent { + slug: string; + /// The agent's configured slackChannel: "#name", "name", or a raw id. + channel: string; +} + +export interface ResolvedAgent { + slug: string; + channelId: string; +} + +/// Match pending agents against a single channel snapshot (one +/// `conversations.list` page-through per pass, shared across all pending +/// agents — never a lookup per agent, which would throttle the token bucket +/// the post/pill path also uses). +/// +/// An agent resolves ONLY when its channel is found AND the bot is a member. +/// `conversations.list` returns a PUBLIC channel as soon as it exists — before +/// the bot is invited — so promoting on mere presence would mirror a channel +/// the bot cannot post to (`not_in_channel`) and receives no message events +/// from: a silent half-mirrored agent, the exact bug this guards against. A +/// PRIVATE channel is only listed once the bot is a member, so the gate is +/// implied there. A raw channel id in the config is taken as-is (the operator +/// wired it explicitly; membership is their responsibility), mirroring +/// `resolveChannelId`. +/// +/// Resolved agents are removed from the returned `stillPending` set, so the +/// caller's next pass never re-resolves them (promote-once). +export function matchPendingChannels( + pending: PendingAgent[], + channels: SlackChannelInfo[], +): { resolved: ResolvedAgent[]; stillPending: PendingAgent[] } { + const byName = new Map(); + for (const c of channels) byName.set(c.name, c); + const resolved: ResolvedAgent[] = []; + const stillPending: PendingAgent[] = []; + for (const a of pending) { + if (/^[CG][A-Z0-9]+$/.test(a.channel)) { + resolved.push({ slug: a.slug, channelId: a.channel }); + continue; + } + const hit = byName.get(a.channel.replace(/^#/, "")); + if (hit && hit.isMember) resolved.push({ slug: a.slug, channelId: hit.id }); + else stillPending.push(a); + } + return { resolved, stillPending }; +}