diff --git a/slack-bridge/README.md b/slack-bridge/README.md index 20ff274d..953daab9 100644 --- a/slack-bridge/README.md +++ b/slack-bridge/README.md @@ -94,6 +94,8 @@ export SLACK_APP_TOKEN="xapp-..." Settings in `settings.json` override environment variables. +Only the broker (and single-player mode) needs Slack tokens. A worker that joins a running broker with `/pinet follow` can run without them; it makes its Slack calls through the broker. Downloading Slack files is the exception and still needs a local bot token. + ## Control who can use Pinet Slack access is default-deny. Configure one of these: diff --git a/slack-bridge/broker/client.test.ts b/slack-bridge/broker/client.test.ts index cf9d9ece..b083bfd5 100644 --- a/slack-bridge/broker/client.test.ts +++ b/slack-bridge/broker/client.test.ts @@ -1372,10 +1372,10 @@ describe("BrokerClient — adapter capabilities", () => { expect(req.params.params.method).toBe("conversations.history"); expect((req.params.params.params as Record).channel).toBe("C123"); - mock.respondTo(mock.connections[0], req.id, { messages: [] }); + mock.respondTo(mock.connections[0], req.id, { ok: true, messages: [] }); const result = await proxyPromise; - expect(result).toEqual({ messages: [] }); + expect(result).toEqual({ ok: true, messages: [] }); client.disconnect(); }); @@ -1409,13 +1409,32 @@ describe("BrokerClient — adapter capabilities", () => { expect(fallbackReq.params.method).toBe("conversations.history"); expect(fallbackReq.params.params.channel).toBe("C123"); - mock.respondTo(mock.connections[0], fallbackReq.id, { messages: [] }); + mock.respondTo(mock.connections[0], fallbackReq.id, { ok: true, messages: [] }); const result = await proxyPromise; - expect(result).toEqual({ messages: [] }); + expect(result).toEqual({ ok: true, messages: [] }); client.disconnect(); }); + + it.each([null, {}, { ok: "true" }, { ok: false, error: 42 }])( + "rejects malformed Slack proxy response %j", + async (response) => { + const client = new BrokerClient(mock.connectOpts); + await client.connect(); + try { + const result = expect(client.slackProxy("reactions.add", {})).rejects.toThrow( + "Invalid Slack proxy response", + ); + await waitFor(() => mock.received.length > 0); + const request = JSON.parse(mock.received[0]); + mock.respondTo(mock.connections[0], request.id, response); + await result; + } finally { + client.disconnect(); + } + }, + ); }); describe("BrokerClient — error handling", () => { diff --git a/slack-bridge/broker/client.ts b/slack-bridge/broker/client.ts index 35a4616e..e1411f75 100644 --- a/slack-bridge/broker/client.ts +++ b/slack-bridge/broker/client.ts @@ -1,4 +1,5 @@ import * as net from "node:net"; +import type { SlackResult } from "../slack-api.js"; import { computeBackoffDelay, withTimeout } from "@pinet/transport-core/async"; import { readMeshSecret } from "./auth.js"; import { DEFAULT_SOCKET_PATH as PINET_DEFAULT_SOCKET_PATH } from "./paths.js"; @@ -717,19 +718,27 @@ export class BrokerClient { * The broker RPC boundary remains adapter-neutral; Slack-specific payloads are * interpreted inside the Slack adapter's api.call capability. */ - async slackProxy( - method: string, - params: Record, - ): Promise> { - try { - return await this.invokeAdapterCapability("slack", "api.call", { method, params }); - } catch (err) { - if (!isRpcMethodNotFoundError(err, "adapter.capability")) { - throw err; - } + async slackProxy(method: string, params: Record): Promise { + const result = await this.invokeAdapterCapability("slack", "api.call", { + method, + params, + }).catch((err) => { + if (!isRpcMethodNotFoundError(err, "adapter.capability")) throw err; + return this.request("slack.proxy", { method, params }); + }); + if ( + typeof result !== "object" || + result === null || + !("ok" in result) || + typeof result.ok !== "boolean" + ) { + throw new Error("Invalid Slack proxy response: expected a boolean ok field."); } - - return (await this.request("slack.proxy", { method, params })) as Record; + const error = "error" in result ? result.error : undefined; + if (error !== undefined && typeof error !== "string") { + throw new Error("Invalid Slack proxy response: expected a string error field."); + } + return { ...result, ok: result.ok, error }; } // ─── Events ────────────────────────────────────────── diff --git a/slack-bridge/index.ts b/slack-bridge/index.ts index efd5cb3b..41896bef 100644 --- a/slack-bridge/index.ts +++ b/slack-bridge/index.ts @@ -102,7 +102,7 @@ import { consumePinetReadConfirmationReplies } from "./pinet-confirmation-replie import { createAgentPromptGuidance } from "./agent-prompt-guidance.js"; import { createAgentEventRuntime } from "./agent-event-runtime.js"; import { createSessionUiRuntime } from "./session-ui-runtime.js"; -import { createSlackRequestRuntime } from "./slack-request-runtime.js"; +import { createSlackRequestRuntime, type SlackRequestCall } from "./slack-request-runtime.js"; import { getSlackMessageAgeMs, isStaleSlackMessage } from "./stale-slack-messages.js"; import { formatSlackAgentsDashboard, @@ -143,11 +143,17 @@ export default function (pi: ExtensionAPI) { let botToken = settings.botToken ?? process.env.SLACK_BOT_TOKEN; let appToken = settings.appToken ?? process.env.SLACK_APP_TOKEN; - - if (!botToken || !appToken) return; + // Followers need no Slack tokens; only the broker and single-player mode talk to the Slack app. + const hasSlackTokens = () => Boolean(botToken && appToken); const slackRequestRuntime = createSlackRequestRuntime(); - const { slack } = slackRequestRuntime; + // An empty token means "no local Slack app": a follower then calls Slack through the broker. + const slack: SlackRequestCall = async (method, token, body) => { + if (token) return slackRequestRuntime.slack(method, token, body); + if (brokerRole === "follower" && brokerClient) + return brokerClient.client.slackProxy(method, body ?? {}); + throw new Error("Slack bot token is not configured."); + }; // allowedUsers / allowAllWorkspaceUsers: settings.json takes priority, env vars as fallback let allowedUsers = buildAllowlist( @@ -455,13 +461,13 @@ export default function (pi: ExtensionAPI) { let isSinglePlayerConnected = () => false; const slackThreadStatuses = new SlackThreadStatusManager({ slack, - getBotToken: () => botToken!, + getBotToken: () => botToken ?? "", formatError: msg, logger: console, }); const slackRuntimeAccess = createSlackRuntimeAccess({ slack, - getBotToken: () => botToken!, + getBotToken: () => botToken ?? "", userNames, channelCache, persistState, @@ -1217,6 +1223,11 @@ export default function (pi: ExtensionAPI) { ): Promise { return runPinetLifecycle(async () => { signal?.throwIfAborted(); + if (mode !== "off" && mode !== "follower" && !hasSlackTokens()) { + throw new Error( + `Pinet ${mode} mode needs SLACK_BOT_TOKEN and SLACK_APP_TOKEN. Without them, join a running broker with /pinet follow.`, + ); + } if (currentRuntimeMode === mode) { if (mode === "off") { setExtStatus(ctx, "off"); @@ -1327,7 +1338,7 @@ export default function (pi: ExtensionAPI) { const reloadWork = runPinetLifecycle(async () => { let reloadBrokerInPlace = false; const validateRefreshedState = () => { - if (!botToken || !appToken) { + if (brokerRole !== "follower" && !hasSlackTokens()) { throw new Error("Slack tokens are not configured after reload."); } if (brokerRole === "broker") { @@ -1405,12 +1416,7 @@ export default function (pi: ExtensionAPI) { const toolRegistrationRuntime = createToolRegistrationRuntime({ buildPromptGuidelines: agentPromptGuidance.buildPromptGuidelines, slackTools: { - getBotToken: () => { - if (!botToken) { - throw new Error("Slack bot token is not configured."); - } - return botToken; - }, + getBotToken: () => botToken ?? "", getDefaultChannel: () => settings.defaultChannel, getSecurityPrompt: () => securityPrompt, inbox, @@ -1905,10 +1911,11 @@ export default function (pi: ExtensionAPI) { refreshSettings(); maybeWarnSlackUserAccess(ctx); - const startupMode = resolveSlackBridgeStartupRuntimeMode(settings, { + const configuredMode = resolveSlackBridgeStartupRuntimeMode(settings, { brokerSocketExists: fs.existsSync(resolveBrokerSocketPath()), brokerManagedFollowerLaunch: isBrokerManagedFollowerLaunch(), }); + const startupMode = configuredMode === "follower" || hasSlackTokens() ? configuredMode : "off"; if (startupMode === "off") { await transitionToRuntimeMode(ctx, startupMode); diff --git a/slack-bridge/slack-file-access.test.ts b/slack-bridge/slack-file-access.test.ts index abcf8fb8..e6cb239f 100644 --- a/slack-bridge/slack-file-access.test.ts +++ b/slack-bridge/slack-file-access.test.ts @@ -168,6 +168,16 @@ describe("fetchSlackFileToCache", () => { expect(fetchImpl).not.toHaveBeenCalled(); }); + it("refuses to download without a bot token instead of fetching a login page", async () => { + const slack = vi.fn(); + const fetchImpl = vi.fn(); + + await expect( + fetchSlackFileToCache("F123", {}, { slack, token: "", fetchImpl }), + ).rejects.toThrow("needs a local Slack bot token"); + expect(fetchImpl).not.toHaveBeenCalled(); + }); + it("rejects files above the configured safe size limit before downloading", async () => { const cacheDir = await makeTempDir(); const slack = vi.fn( diff --git a/slack-bridge/slack-file-access.ts b/slack-bridge/slack-file-access.ts index b034ca6d..adddafeb 100644 --- a/slack-bridge/slack-file-access.ts +++ b/slack-bridge/slack-file-access.ts @@ -260,6 +260,10 @@ export async function fetchSlackFileToCache( if (!trimmedFileId) { throw new Error("file_id is required."); } + // Private file URLs need the bot token itself; the broker's Slack proxy cannot fetch bytes. + if (!deps.token) { + throw new Error("Downloading Slack files needs a local Slack bot token; this worker has none."); + } const fetchImpl = deps.fetchImpl ?? fetch; const now = deps.now?.() ?? new Date(); diff --git a/slack-bridge/token-free-follower.test.ts b/slack-bridge/token-free-follower.test.ts new file mode 100644 index 00000000..1f5e6ceb --- /dev/null +++ b/slack-bridge/token-free-follower.test.ts @@ -0,0 +1,124 @@ +import * as fs from "node:fs"; +import * as os from "node:os"; +import * as path from "node:path"; +import { afterEach, expect, it, vi } from "vitest"; +import type { + CommandDefinition, + ExtensionAPI, + ExtensionContext, + ToolDefinition, +} from "@earendil-works/pi-coding-agent"; +import extension from "./index.js"; +import { BrokerDB } from "./broker/schema.js"; +import { BrokerSocketServer } from "./broker/socket-server.js"; + +afterEach(() => { + vi.unstubAllEnvs(); + vi.restoreAllMocks(); +}); + +type Handler = (event: object, ctx: ExtensionContext) => Promise; + +it("joins a real broker without Slack tokens and calls Slack through the broker", async () => { + const root = fs.mkdtempSync(path.join(os.tmpdir(), "pinet-token-free-")); + const socket = path.join(root, "broker.sock"); + fs.mkdirSync(path.join(root, ".pi", "agent"), { recursive: true }); + // runtimeMode "broker" mirrors a shared settings.json; without tokens it must not start a broker. + fs.writeFileSync( + path.join(root, ".pi", "agent", "settings.json"), + JSON.stringify({ "slack-bridge": { runtimeMode: "broker", allowAllWorkspaceUsers: true } }), + ); + vi.stubEnv("HOME", root); + vi.stubEnv("SLACK_BOT_TOKEN", undefined); + vi.stubEnv("SLACK_APP_TOKEN", undefined); + vi.stubEnv("PINET_SOCKET_PATH", socket); + vi.stubEnv("PINET_MESH_SECRET", "test-mesh-secret"); + + const db = new BrokerDB(":memory:"); + db.initialize(); + const server = new BrokerSocketServer(db, socket, { meshSecret: "test-mesh-secret" }); + const slackCalls: object[] = []; + server.setOutboundMessageAdapters([ + { + name: "slack", + send: vi.fn(), + invokeCapability: async ({ params }) => { + slackCalls.push(params); + return { result: { ok: true } }; + }, + }, + ]); + await server.start(); + + const events = new Map(); + const commands = new Map(); + const tools = new Map(); + const pi = { + registerCommand: (name: string, definition: CommandDefinition) => + commands.set(name, definition), + registerTool: (definition: ToolDefinition) => tools.set(definition.name, definition), + registerMessageRenderer: vi.fn(), + on: (name: string, fn: Handler) => events.set(name, fn), + appendEntry: vi.fn(), + sendMessage: vi.fn(), + sendUserMessage: vi.fn(), + getActiveTools: () => ["read", "slack", "pinet"], + setActiveTools: vi.fn(), + getSessionName: () => undefined, + } as ExtensionAPI; + const notify = vi.fn(); + const context = { + cwd: root, + hasUI: true, + isIdle: () => true, + ui: { + notify, + onTerminalInput: vi.fn(), + setStatus: vi.fn(), + select: vi.fn(), + custom: vi.fn(), + theme: { fg: (_color: string, text: string) => text }, + }, + sessionManager: { + getEntries: () => [], + getBranch: () => [], + getLeafId: () => "token-free-test", + getSessionFile: () => path.join(root, "session.jsonl"), + }, + } as ExtensionContext; + + try { + extension(pi); + expect(commands.has("pinet")).toBe(true); + await events.get("session_start")!({}, context); + expect(db.getAgents()).toHaveLength(0); + + await commands.get("pinet")!.handler("follow", context); + await expect.poll(() => db.getAgents().length, { timeout: 10_000 }).toBe(1); + + await tools.get("slack")!.execute!( + "call-1", + { + action: "react", + args: { emoji: "eyes", timestamp: "1712345678.000100", channel: "C0TEST" }, + }, + undefined, + undefined, + context, + ); + expect(slackCalls).toContainEqual({ + method: "reactions.add", + params: { channel: "C0TEST", timestamp: "1712345678.000100", name: "eyes" }, + }); + + // Asking a token-less follower to become broker fails before it leaves the mesh. + await commands.get("pinet")!.handler("start", context); + expect(notify).toHaveBeenCalledWith(expect.stringContaining("needs SLACK_BOT_TOKEN"), "error"); + expect(db.getAgents()[0].disconnectedAt).toBeFalsy(); + } finally { + await events.get("session_shutdown")?.({}, context); + await server.stop(); + db.close(); + fs.rmSync(root, { force: true, recursive: true }); + } +}, 20_000);