From eb0553a645f58ee503ac3f723a8bfa462458bd38 Mon Sep 17 00:00:00 2001 From: Devvyn <22340871+opticon454@users.noreply.github.com> Date: Thu, 29 Oct 2026 23:03:26 +0800 Subject: [PATCH] fix(discord): address review on #741 Builds the alert tail only for a post that goes out; separates deduped from disabled and never dedupes a deliberate notify; types the startup hint with fromUser false and skips resumed, remote and Docker tabs; clears per-tab maps and closes the bridge on stop; acts only on a real delete, not a detach; serialises thread-map writes; reloads the bridge after a discord.json restore; probes and self-ignores .codeman-discord; validates the message id; reacts when a reply is rate limited; documents what leaves the machine. --- CLAUDE.md | 4 +- docs/discord-bridge.md | 4 +- docs/security-architecture.md | 2 +- docs/wiki/Discord-Bridge.md | 2 +- src/discord-bridge.ts | 122 ++++++++++++++--- src/web/routes/config-backup-routes.ts | 9 +- src/web/routes/discord-routes.ts | 3 +- src/web/server.ts | 63 +++++---- test/discord-bridge.test.ts | 158 +++++++++++++++++++++-- test/discord-push-glue.test.ts | 56 ++++++-- test/routes/config-backup-routes.test.ts | 18 ++- test/routes/discord-routes.test.ts | 14 ++ 12 files changed, 380 insertions(+), 75 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 51b0d2fb..57b45d6a 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -131,7 +131,7 @@ Codeman is a Claude Code session manager with web interface and autonomous Ralph **Agent CLI (`codeman agent …`, `src/cli-agent.ts`)**: the command-line client for the wait primitives and the session verbs, for agents the skill does not reach (pi, shell, no `jq`; `seedAgentSessionPreamble` covers every local session whose CLI takes the skill, `capabilities.agentSkillDir`; every pane still exports `CODEMAN_MUX`/`CODEMAN_SESSION_ID`/`CODEMAN_API_URL`). It is an HTTP CLIENT: inspection and guarded stage/submit use the ownership-checked `agent-session-routes.ts` module (experimental, behind `agentCoordinationEnabled`, SYNCED, default OFF: all three routes answer `403 FORBIDDEN` naming it while off, and the CLI shows that verbatim); the other verbs reuse existing routes, with no second transport (the plan's "Socket API" non-goal stands), every call carries `X-Codeman-Parent-Session`, only `spawn`'s quick-start carries `X-Codeman-Agent-Origin` (pinned in test/cli-agent.test.ts), while `spawn` without a case (cwd or `--working-dir`) is `POST /api/v1/sessions` plus `/interactive` or `/shell` without that header, so no agent-case marker, and refuses (exit 4, before any request) an empty `--working-dir` and a bare spawn from a HOME cwd, since it always sends an explicit `workingDir` and the server's no-`workingDir` hooks skip cannot apply (test/cli-agent-project-spawn.test.ts), auth (`src/codeman-credentials.ts`, shared with `codeman attach` and the TUI) from `CODEMAN_PASSWORD` or the data dir's `.env` (the same reader `codeman attach` uses — `readCodemanEnvFile`). Verbs return exit codes (`0` ok / `1` error / `2` timeout / `3` exited / `4` refused) instead of calling `process.exit`, so `test/cli-agent.test.ts` drives every verb against a recording fake transport plus the real `httpRequest` against a local server. ⚠️ Three guards live in code, not prose, and the tests pin them: refuse without `CODEMAN_MUX=1`+`CODEMAN_API_URL` (never guess a URL); `send` transmits printable text + `\r` only (`inputRefusal`: any byte < 0x20 or DEL refuses BEFORE the transport — opencode's `Ctrl+C` is `app_exit`), ESC exists solely as `interrupt` with no `\r`; `rm` fails closed (`deleteRefusal`: empty id, self shorter than 8, prefix match in EITHER direction, re-checked on the resolved id). ⚠️ The routes accept FULL ids only — an 8-char prefix is a 404 (measured live) — while `ls` prints prefixes, so `resolveSessionId` maps a short id through `GET /api/v1/sessions` and refuses an ambiguous one. It also refuses any id under 8 characters (exit 4, on every verb, before any request), the same floor as the server's `PARENT_SESSION_ID_MIN_PREFIX`, so `rm 9` can never pick an arbitrary session. ⚠️ A 400 for `--until stop` on a hook-less mode is passed through verbatim, never papered over: the marker path is the answer there. `spawn` on a readiness timeout exits 2 and LEAVES the session for inspection (the skill's `spawn_worker` deletes it; the CLI's caller is often a human); its stdout is the id ALONE (prose on stderr) so `SID=$(…)` works. ⚠️ Two server answers that look like success and are not: `delivered:false` without `duplicate` (the bytes went into a dead pane — the field exists so a client says "restart" instead of "wait longer") and `wait.ended:true, signal:null` (the worker died during a `stop`/`idle` wait: the registry satisfies only waiters that listed `exit`, then cancels the rest) — both exit 3, checked BEFORE the happy paths in `waitExitCode`. → README "`codeman agent`", `docs/api-reference.md` -**Discord bridge (`src/discord-bridge.ts`, `src/web/routes/discord-routes.ts`, `src/cli-notify.ts`)**: opt-in two-way Discord (a bot, not the one-way webhook), one thread per tab. Split like `webhook-notify.ts`: pure helpers, a 0600 store (`discord.json` holds the bot token and is never returned; `discord-bridge.json` maps sessionId to thread id), then IO (`DiscordApi` REST with injected fetch, `DiscordGateway` inbound WebSocket with injected socket, `DiscordBridge` rules). Outbound hangs off `WebServer.sendPushNotifications` beside the webhook (so it works with zero push subscriptions) and tab events off `WebServer.broadcast` (`bridgeSessionEvent`); the alert tail is the RENDERED tmux screen (`screenTail`), never the raw stream, which a full-screen UI repaints in place. Inbound replies are typed through `session.writeViaMux`. ⚠️ Guards that live in code and tests pin: only `allowedUserIds`, in `guildId`, inside a thread the bridge created, are acted on, everything else is dropped unread; a reply is one printable line plus Enter, capped and rate limited; posts send `allowed_mentions` limited to the allowlisted ids; received files go only from Discord's own HTTPS hosts (max 5 x 8 MB, names reduced to `[A-Za-z0-9._-]`) into `/.codeman-discord/`, and a sent file's REAL path must lie inside the tab's folder (`fileProblem`, symlinks followed); a Discord failure never reaches a session. The startup hint ("how to reach the user") rides the CLI registry as the engine value `appendSystemPrompt` (Claude's `--append-system-prompt`, undefined unless the bridge and `startupHint` are on, so every other launch is byte-identical) and is typed once as a first message to brand-new external CLI tabs on their first idle (`sessionIdle`; never a shell, a restored tab or a resumed one). `codeman notify "" [--urgent] [--file ]` wraps `POST /api/sessions/:id/discord-notify`, which accepts a unique 8-character id prefix (a Docker case exports it cut). → `docs/discord-bridge.md`, `docs/wiki/Discord-Bridge.md`, `docs/api-reference.md` +**Discord bridge (`src/discord-bridge.ts`, `src/web/routes/discord-routes.ts`, `src/cli-notify.ts`)**: opt-in two-way Discord (a bot, not the one-way webhook), one thread per tab. Split like `webhook-notify.ts`: pure helpers, a 0600 store (`discord.json` holds the bot token and is never returned; `discord-bridge.json` maps sessionId to thread id), then IO (`DiscordApi` REST with injected fetch, `DiscordGateway` inbound WebSocket with injected socket, `DiscordBridge` rules). Outbound hangs off `WebServer.sendPushNotifications` beside the webhook (so it works with zero push subscriptions) and tab events off `WebServer.broadcast` (`bridgeSessionEvent`); the alert tail is the RENDERED tmux screen (`screenTail`), never the raw stream, which a full-screen UI repaints in place. Inbound replies are typed through `session.writeViaMux`. ⚠️ Guards that live in code and tests pin: only `allowedUserIds`, in `guildId`, inside a thread the bridge created, are acted on, everything else is dropped unread; a reply is one printable line plus Enter, capped and rate limited; posts send `allowed_mentions` limited to the allowlisted ids; received files go only from Discord's own HTTPS hosts (max 5 x 8 MB, names reduced to `[A-Za-z0-9._-]`) into `/.codeman-discord/`, and a sent file's REAL path must lie inside the tab's folder (`fileProblem`, symlinks followed); a Discord failure never reaches a session; the alert tail is a function the bridge calls only once it will post (no tmux exec per event while off); a delete reaches the bridge from the cleanup path with `killMux` true, never from `session:deleted`, which a detach also fires; `.codeman-discord/` is probed with the bounded probe, refused if a symlink, and ignores itself with a `.gitignore` of `*`; a config restore of `discord.json` reloads the running bridge (`afterRestore`). The startup hint ("how to reach the user") rides the CLI registry as the engine value `appendSystemPrompt` (Claude's `--append-system-prompt`, undefined unless the bridge and `startupHint` are on, so every other launch is byte-identical) and is typed once as a first message to brand-new external CLI tabs on their first idle (`sessionIdle`; never a shell, a restored tab, a resumed one, or a remote (ssh) or Docker tab, which have no `codeman` on their PATH). `codeman notify "" [--urgent] [--file ]` wraps `POST /api/sessions/:id/discord-notify`, which accepts a unique 8-character id prefix (a Docker case exports it cut). → `docs/discord-bridge.md`, `docs/wiki/Discord-Bridge.md`, `docs/api-reference.md` **MCP server (`codeman mcp`, `src/mcp-server.ts`)**: a Model Context Protocol server on stdio for any MCP client, a CLIENT of the HTTP API like `codeman agent` (reuses its `httpRequest`, `inputRefusal`, `buildSendBody`; the MCP server adds no routes). Seven tools: `list_sessions`, `start_session`, `send_prompt`, `wait_for_session`, `read_output`, `interrupt_session`, `delete_session`. ⚠️ Dual-era: answers the legacy `initialize` handshake (2024-11-05 to 2025-11-25) AND the 2026-07-28 revision (per-request `_meta` with `protocolVersion` + `clientCapabilities`, `server/discover`, `resultType: "complete"` on every modern result); a modern `tools/list`/`server/discover` MUST carry `ttlMs` + `cacheScope` (Claude Code 2.1.296 drops every tool without them: measured). ⚠️ stdout is protocol only; logs go to stderr. ⚠️ The URL comes from `--url`/`CODEMAN_API_URL`, never a guess, and unconfigured it still lists tools (registries introspect) while every call answers how to configure it. ⚠️ Prompt/interrupt/delete only sessions the PROCESS started (in-memory allowlist, so a restart fails closed) or, inside a pane, the calling session's own children, unless `--allow-any-session`; never its own session. ⚠️ `read_output` returns text ONLY, no `structuredContent` (a client that prefers it showed the model metadata instead of the output: measured). Waits default 45 s (Codex's MCP tool timeout is 60 s). Registry manifest `config/mcp-registry/server.json` (`mcpName` in package.json, version kept by `scripts/sync-plugin.mjs`). Tests: `test/mcp-server.test.ts`. @@ -191,7 +191,7 @@ Codeman is a Claude Code session manager with web interface and autonomous Ralph **MCP server sync** (opt-in, `mcpSyncEnabled`, SYNCED, default OFF; `src/mcp-sync.ts`, `GET`/`POST /api/mcp-sync`): copies each installed, enabled CLI's user-level MCP servers into the others. It is the ONE subsystem that writes another CLI's REAL user config (`~/.claude.json`, `~/.codex/config.toml`, `~/.gemini/*`, opencode's), which is why it is opt-in and admin-only in multi-user mode (both verbs 403 for a non-admin, and the Settings group is hidden for them). Where each CLI keeps the file is registry data, `capabilities.mcpConfig` (`{ path, format, relocation? }`), never a branch on the id. ⚠️ Tools that sync but are not run modes (GitHub Copilot CLI, the first) are plain data in `MCP_SYNC_ONLY_TOOLS` (`src/mcp-sync-targets.ts`), take part only when installed or already configured, and their companion `settings.json` (`disabledMcpServers`) is read, never written; a test must clear their relocation vars too. ⚠️ ADDITIVE only: a name already defined, in any shape, is never edited or removed (a different same-name definition is a reported conflict), and a server switched off in its own CLI is never copied. ⚠️ Never write a file that did not parse; re-parse the NEW text and require every added server to read back before the tmp+rename (written through a symlink, previous file kept as `.codeman-bak`, one apply at a time, else 409). ⚠️ A file that receives copied `env`/`headers` (secrets) is left `0600`, and so is the backup. ⚠️ Responses carry server NAMES only, never env values, headers or file text: a parse failure is reported by line and column (`describeMcpSyncError`), never the parser's own message (smol-toml and V8 both quote source). ⚠️ `mcpConfig.relocation` names the env var the CLI reads to move its file (`CLAUDE_CONFIG_DIR`, `CODEX_HOME`, `XDG_CONFIG_HOME`, `GEMINI_CLI_HOME`, and `COPILOT_HOME` for Copilot), resolved from the SERVER env at call time; a relative value reports the target `skipped`, never a guessed write, and a per-session `envOverrides` relocation is not followed. Tests must pass `home` (which drops the `process.env` default) or clear those vars first. → `docs/cli-registry.md` (MCP server sync), `docs/api-reference.md`, `docs/wiki/Settings-Reference.md` -**Auto-named sessions** (`autoNameSessions`, SYNCED, default OFF): a placeholder tab (`w3-myapp`) takes its first real prompt as a title in the `: ` form, so the case identity and `w<n>` counter survive. Ownership is `SessionState.nameSource` (`placeholder` | `auto` | `manual`; the `name` setter / `PUT /api/sessions/:id/name` makes it `manual`, never touched again). ⚠️ `applyAutoName()` flips to `auto` even if the string is unchanged, so only the FIRST titled prompt names the tab. ⚠️ Only user input counts: `SessionWriteOptions.fromUser` is set by the browser WS path, `POST /api/sessions/:id/input` and the experimental guarded stage (`stageGuardedInput()`, `POST /api/sessions/:id/stage`, typed on an agent's behalf like the input route) ONLY; any new user-input path must set it (and the send-key Shift+Enter path must call `trackUserInput()`). ⚠️ The pure tracker (`session-auto-name.ts`) sits on the raw keystroke stream with an explicit rule per key; add a rule for any new key class. ⚠️ `nameSource` also decides `--name`: by default only a `manual` name is pinned on the claude CLI (`Session.cliPinnedName`), since `--name` is also the `/resume` title and a pinned one stops Claude generating its own; the opt-in synced `pinCliSessionName` (default OFF, re-read at every spawn and respawn) pins every tab name, after which a Codeman rename holds only until the next turn; a rename appends a `custom-title` row to a LOCAL, non-docker transcript, and a same-name PUT is a no-op (never flips to `manual`). Tests: `test/session-auto-name.test.ts`. → [architecture-invariants#auto-named-sessions-first-prompt--tab-title](docs/architecture-invariants.md#auto-named-sessions-first-prompt--tab-title) +**Auto-named sessions** (`autoNameSessions`, SYNCED, default OFF): a placeholder tab (`w3-myapp`) takes its first real prompt as a title in the `<prefix>: <title>` form, so the case identity and `w<n>` counter survive. Ownership is `SessionState.nameSource` (`placeholder` | `auto` | `manual`; the `name` setter / `PUT /api/sessions/:id/name` makes it `manual`, never touched again). ⚠️ `applyAutoName()` flips to `auto` even if the string is unchanged, so only the FIRST titled prompt names the tab. ⚠️ Only user input counts: `SessionWriteOptions.fromUser` is set by the browser WS path, `POST /api/sessions/:id/input` the experimental guarded stage (`stageGuardedInput()`, `POST /api/sessions/:id/stage`, typed on an agent's behalf like the input route) and a Discord reply (`DiscordBridge.onGatewayMessage` via `sendInput(…, true)`; the bridge's own startup hint is typed with `fromUser: false`) ONLY; any new user-input path must set it (and the send-key Shift+Enter path must call `trackUserInput()`). ⚠️ The pure tracker (`session-auto-name.ts`) sits on the raw keystroke stream with an explicit rule per key; add a rule for any new key class. ⚠️ `nameSource` also decides `--name`: by default only a `manual` name is pinned on the claude CLI (`Session.cliPinnedName`), since `--name` is also the `/resume` title and a pinned one stops Claude generating its own; the opt-in synced `pinCliSessionName` (default OFF, re-read at every spawn and respawn) pins every tab name, after which a Codeman rename holds only until the next turn; a rename appends a `custom-title` row to a LOCAL, non-docker transcript, and a same-name PUT is a no-op (never flips to `manual`). Tests: `test/session-auto-name.test.ts`. → [architecture-invariants#auto-named-sessions-first-prompt--tab-title](docs/architecture-invariants.md#auto-named-sessions-first-prompt--tab-title) **Maintainer bot (external)**: the Telegram bot that reviews open PRs and triages discussion threads in Codeman sessions used to live at `scripts/pr-bot/`. It moved OUT of this repository on 2026-09-14, to `~/codeman-cases/prbot/` (its own private git repo, systemd unit `codeman-pr-bot`, guide + agent rules in its own `README.md` and `CLAUDE.md`). It is a CLIENT of Codeman's HTTP API like any other, so nothing here depends on it and it is not part of the server, the CLI or the npm package. ⚠️ It spawns real sessions named `prbot-<n>` / `dscbot-<n>` on the local Codeman and holds clones under `~/.codeman/pr-bot/`, so those session names and that data dir are taken; it also fetches PR heads into `refs/pr-bot/*` of this checkout and must never check out, reset or clean it. The CHANGELOG entries for 1.25.0 and earlier still describe it, which is history rather than drift. diff --git a/docs/discord-bridge.md b/docs/discord-bridge.md index ab5cb8c0..71f0b563 100644 --- a/docs/discord-bridge.md +++ b/docs/discord-bridge.md @@ -101,14 +101,14 @@ In the Discord mobile app set the server to **All Messages**. Discord's own push A CLI only posts deliberately if it knows it can. While the bridge and `startupHint` are on: - **Claude** is launched with `--append-system-prompt`, so nothing shows in the conversation. -- **Every other CLI** is sent the same text once, as a message, the first time a brand-new tab becomes idle. Tabs that are restored when Codeman starts, resumed tabs and shell tabs are never sent it. +- **Every other CLI** is sent the same text once, as a message, the first time a brand-new tab becomes idle. Tabs that are restored when Codeman starts, resumed tabs, shell tabs, and remote (ssh) and Docker tabs (which have no `codeman` command) are never sent it. The text explains `codeman notify "message" --urgent`. `codeman` is already on the PATH of every session and reads `CODEMAN_SESSION_ID` and `CODEMAN_API_URL`, so no other setup is needed. Without a bridge it prints that the bridge is off and exits cleanly. Tabs started before the bridge was enabled are not told; use the skill or `CLAUDE.md` for those. ## Files - **Received:** files you attach to a reply from an allowed user are saved as `<working folder>/.codeman-discord/<message id>-<name>` (mode 0600) and the one-line reply the CLI sees ends with `[attached: <paths>]`. At most 5 files of 8 MB each; names are reduced to letters, digits, `.`, `_` and `-`; only Discord's own HTTPS addresses are fetched. Anything skipped is named in the thread. Add `.codeman-discord/` to your `.gitignore`. Remote (ssh) tabs have no local folder, so no file is saved for them. -- **Sent:** `codeman notify "message" --file <path>` uploads a regular file of at most 8 MB. The real path (symlinks followed) must be inside the tab's working folder, so a CLI cannot post `~/.codeman/discord.json` or an ssh key. Remote tabs cannot send files. +- **Sent:** `codeman notify "message" --file <path>` uploads a regular file of at most 8 MB. The real path (symlinks followed) must be inside the tab's working folder, which stops an upload of something outside it by mistake. The agent runs as your user, so this is a guard against accidents, not a sandbox. Remote tabs cannot send files. - A downloaded file is untrusted content: only allowed users can send one, but a CLI that opens it is exposed to anything written inside it. ## Security model diff --git a/docs/security-architecture.md b/docs/security-architecture.md index 153efeb6..da6306db 100644 --- a/docs/security-architecture.md +++ b/docs/security-architecture.md @@ -582,7 +582,7 @@ Opt-in and off by default: the server POSTs the Web Push events (permission prom - **A second server-side outbound channel through the web-tab egress guard (§10b).** Delivery goes through `webviewFetch`, so link-local and cloud-metadata targets are refused at save time and again on the RESOLVED address at connect time; redirects are not followed (`redirect: 'manual'`) and each send is bounded by a 5 s timeout. Loopback and RFC1918 stay allowed on purpose (a self-hosted ntfy is the point), so **Send test** works as a blind reachability probe (status, refused or timed out, never a response body) for whoever may call it. Web tabs already give that caller full LAN reach with bodies, so nothing new is exposed. - **The URL is a bearer secret** (anyone holding a Slack or Discord webhook URL can post as it). It lives in `~/.codeman/webhook.json` (0600, tmp+rename), is kept out of `settings.json` (which every logged-in user reads through `GET /api/settings`), is never returned (`GET /api/webhook` gives scheme + host only), and never appears in a log line, a delivery result or an error message. -- **Discord bridge** (`src/discord-bridge.ts`, [setup](discord-bridge.md)): the bot token lives in `~/.codeman/discord.json` (0600, tmp+rename) and is never returned, logged or put in an error. Inbound replies are typed into a session only from `allowedUserIds`, in the configured guild, inside a thread the bridge created; anything else is dropped unread. A reply becomes one printable line plus Enter, capped at 1000 characters and one per second per tab. Posts send `allowed_mentions` limited to the allowlisted ids, so agent output cannot ping a server. **Files:** an attachment is fetched only from Discord's own HTTPS hosts (no redirects followed), at most 5 per reply and 8 MB each, saved 0600 under `<tab folder>/.codeman-discord/` with the name reduced to `[A-Za-z0-9._-]`; a file a CLI sends must be a regular file whose real path (symlinks followed) lies inside its own tab's folder, so it cannot post `discord.json` or a key, and remote sessions send and receive none. A received file is untrusted content to whatever opens it. **Startup hint:** a fixed text (no user input) is added to Claude's system prompt, or typed once to a brand-new external CLI tab, never to a shell. The bridge only dials out (REST and the gateway WebSocket), so it needs no inbound exposure. A Discord account on the allowlist can drive agents that run shell commands: it needs 2FA. +- **Discord bridge** (`src/discord-bridge.ts`, [setup](discord-bridge.md)): the bot token lives in `~/.codeman/discord.json` (0600, tmp+rename) and is never returned, logged or put in an error. Inbound replies are typed into a session only from `allowedUserIds`, in the configured guild, inside a thread the bridge created; anything else is dropped unread. A reply becomes one printable line plus Enter, capped at 1000 characters and one per second per tab. Posts send `allowed_mentions` limited to the allowlisted ids, so agent output cannot ping a server. **Files:** an attachment is fetched only from Discord's own HTTPS hosts (no redirects followed), at most 5 per reply and 8 MB each, saved 0600 under `<tab folder>/.codeman-discord/` with the name reduced to `[A-Za-z0-9._-]`; a file a CLI sends must be a regular file whose real path (symlinks followed) lies inside its own tab's folder, which stops a file outside that folder being uploaded by mistake, and remote sessions send and receive none. A received file is untrusted content to whatever opens it. **What the checks do not stop:** the agent runs as the same OS user, so the path check prevents accidental uploads, not a determined agent that pastes a file's contents into the message. **What leaves the machine:** each alert sends the rendered screen (whatever the agent printed) to Discord, and with **Which events** set to Everything that is every turn end; treat the Discord server as holding that output. **Multi-user:** the bridge is one allowlist for the whole server, so an allowlisted Discord account can type into every user's sessions; only an admin can configure it. **Startup hint:** a fixed text (no user input) is added to Claude's system prompt, or typed once to a brand-new external CLI tab, never to a shell. The bridge only dials out (REST and the gateway WebSocket), so it needs no inbound exposure. A Discord account on the allowlist can drive agents that run shell commands: it needs 2FA. - **It carries session data to a third party.** Titles and bodies include session names, tool names and error text, all agent- or user-controlled, so Discord gets `allowed_mentions: { parse: [] }` and Slack's `& < >` are escaped: agent output cannot ping a channel. In multi-user mode all three routes are admin-only and the channel is instance-wide: it receives every user's session events, the same reach an admin's own Web Push has, which means non-admins' session details leave the box at the admin's choice. --- diff --git a/docs/wiki/Discord-Bridge.md b/docs/wiki/Discord-Bridge.md index 1f99124b..8899bf3f 100644 --- a/docs/wiki/Discord-Bridge.md +++ b/docs/wiki/Discord-Bridge.md @@ -55,7 +55,7 @@ An agent only posts deliberately if it knows it can. With **Tell each CLI how to - **Claude** is launched with the instruction as part of its system prompt, so nothing shows in the chat. - **Every other CLI** is sent the same text once, as a message, the first time a brand-new tab goes idle. - Restored tabs, resumed tabs and shell tabs are never sent it. + Restored tabs, resumed tabs, shell tabs, and remote (ssh) and Docker tabs (which have no `codeman` command) are never sent it. Tabs that were already open when you turned the bridge on are not told. The alerts still reach Discord for them; only the agent-initiated `codeman notify` habit is missing. diff --git a/src/discord-bridge.ts b/src/discord-bridge.ts index f548359f..571931c7 100644 --- a/src/discord-bridge.ts +++ b/src/discord-bridge.ts @@ -22,6 +22,7 @@ import { existsSync, mkdirSync } from 'node:fs'; import fs from 'node:fs/promises'; import { join } from 'node:path'; +import { probePathKind } from './utils/bounded-path-probe.js'; import WebSocket from 'ws'; import { WEBHOOK_SCOPES, type WebhookScope, type WebhookUrgency } from './types/push.js'; @@ -85,7 +86,7 @@ export interface DiscordMessage { sessionId?: string; sessionName?: string; /** The tail of what the CLI last printed, so the reader can see what it is asking. */ - tail?: string; + tail?: string | (() => string); host?: string; /** A file to upload with the post (already read and size-checked by the caller). */ file?: DiscordFile; @@ -98,6 +99,8 @@ export interface DiscordFile { export interface DiscordResult { ok: boolean; + /** True when the same event for the same tab was posted within the last few seconds, so nothing was sent. */ + deduped?: boolean; status?: number; error?: string; at: number; @@ -227,7 +230,14 @@ export function buildPost(msg: DiscordMessage, cfg: DiscordConfig, explicitUrgen const ping = shouldPing(cfg.pingMode, msg.urgency, explicitUrgent) ? cfg.allowedUserIds : []; const mentions = ping.map((id) => `<@${id}>`).join(' '); const head = `${mentions ? `${mentions} ` : ''}**${clip(msg.title, 120)}**`; - const fence = msg.tail?.trim() ? `\n\`\`\`\n${msg.tail.trim().replace(/```/g, "'''")}\n\`\`\`` : ''; + // Built here, not by the caller, so the screen is only read for a post that is really going out. + let tailText: string | undefined; + try { + tailText = typeof msg.tail === 'function' ? msg.tail() : msg.tail; + } catch { + tailText = undefined; + } + const fence = tailText?.trim() ? `\n\`\`\`\n${tailText.trim().replace(/```/g, "'''")}\n\`\`\`` : ''; const host = msg.host ? ` (${clip(msg.host, 60)})` : ''; const content = clip(`${head}${host}${msg.body ? `\n${msg.body}` : ''}${fence}`, MAX_CONTENT); // Agent output and session names must not be able to @everyone the server; only the allowlisted ids ping. @@ -579,11 +589,48 @@ export class DiscordGateway { // The bridge // --------------------------------------------------------------------------- +/** + * Create or re-verify `<workingDir>/.codeman-discord`, or null when it is not safe to write there. + * Same safeguards as the prompt uploads folder: the workspace is probed with the bounded probe before + * the first disk touch (a dead mount must not hang the event loop), a symlink or a non-directory at + * the folder is refused (an agent could plant `.codeman-discord -> ~/.ssh`), and the folder ignores + * itself with a `.gitignore` of `*`, written once with O_EXCL so an `git add -A` never commits what was + * sent and a file already there stays the user's. + */ +export async function ensureReceivedDir(workingDir: string): Promise<string | null> { + if ((await probePathKind(workingDir, { pastCap: true })) !== 'directory') return null; + const dir = join(workingDir, RECEIVED_DIR); + try { + const st = await fs.lstat(dir); + if (st.isSymbolicLink() || !st.isDirectory()) return null; + } catch (err) { + if ((err as NodeJS.ErrnoException).code !== 'ENOENT') return null; + try { + await fs.mkdir(dir); + } catch (mkErr) { + if ((mkErr as NodeJS.ErrnoException).code !== 'EEXIST') return null; + const again = await fs.lstat(dir).catch(() => null); + if (!again || again.isSymbolicLink() || !again.isDirectory()) return null; + } + } + const ignore = join(dir, '.gitignore'); + try { + await fs.writeFile(ignore, '*\n', { flag: 'wx' }); + } catch (err) { + if ((err as NodeJS.ErrnoException).code === 'EEXIST') return dir; + // An empty ignore file would read as the user's next time, so take it back. + await fs.rm(ignore, { force: true }).catch(() => undefined); + return null; + } + return dir; +} + export interface DiscordBridgeDeps { configDir: string; fetchImpl: DiscordFetch; /** Types `text` (already ending in Enter) into the session. False when the session is gone. */ - sendInput: (sessionId: string, text: string) => Promise<boolean>; + /** `fromUser` is true only for what a person typed (a Discord reply), false for text Codeman types itself. */ + sendInput: (sessionId: string, text: string, fromUser: boolean) => Promise<boolean>; /** A session's working folder, where received files are saved. Undefined when it is gone or remote. */ sessionDir?: (sessionId: string) => string | undefined; socketFactory?: SocketFactory; @@ -604,6 +651,8 @@ export class DiscordBridge { /** Brand-new sessions that have not yet been sent the startup hint (non-Claude CLIs). */ private fresh = new Set<string>(); private pending = new Map<string, Promise<string | null>>(); + /** Thread-map writes run one after another: concurrent creations must not interleave on the tmp file. */ + private persistChain: Promise<void> = Promise.resolve(); private last: DiscordResult | null = null; private readonly api: DiscordApi; private readonly now: () => number; @@ -648,6 +697,10 @@ export class DiscordBridge { this.gateway?.stop(); this.gateway = null; activeStartupHint = undefined; + this.lastSent.clear(); + this.lastReply.clear(); + this.names.clear(); + this.fresh.clear(); } private fail(err: unknown): DiscordResult { @@ -668,6 +721,13 @@ export class DiscordBridge { return { ok: false, status, error, at: this.now() }; } + private persistThreads(): Promise<void> { + this.persistChain = this.persistChain + .then(() => writeThreadMap(this.deps.configDir, { ...this.threads })) + .catch(() => undefined); + return this.persistChain; + } + /** The thread for a session, creating it when absent. Concurrent callers share one creation. */ private threadFor(sessionId: string, name: string | undefined): Promise<string | null> { const existing = this.threads[sessionId]; @@ -679,7 +739,7 @@ export class DiscordBridge { const t = await this.api.createThread(this.cfg.parentChannelId, threadNameFor(name, sessionId)); if (!t?.id) return null; this.threads[sessionId] = t.id; - await writeThreadMap(this.deps.configDir, this.threads).catch(() => undefined); + await this.persistThreads(); return t.id; } catch (err) { this.last = this.fail(err); @@ -693,13 +753,21 @@ export class DiscordBridge { } /** Post an alert. Session-less messages go to the parent channel. Never throws. */ - async notify(msg: DiscordMessage, explicitUrgent = false): Promise<DiscordResult | null> { + async notify( + msg: DiscordMessage, + opts: { urgent?: boolean; deliberate?: boolean } = {} + ): Promise<DiscordResult | null> { + const explicitUrgent = opts.urgent === true; if (!this.loaded) await this.reload(); if (!shouldSendDiscord(this.cfg, msg.urgency)) return null; const key = `${msg.event}:${msg.sessionId ?? ''}`; const t = this.now(); const prev = this.lastSent.get(key); - if (!explicitUrgent && prev !== undefined && t - prev < DEDUPE_WINDOW_MS) return null; + // A deliberate post (an agent's own `codeman notify`) is never deduped: a second, different message is real. + // Automatic events are, and say so, which is not the same answer as "the bridge is off" (null). + if (!opts.deliberate && prev !== undefined && t - prev < DEDUPE_WINDOW_MS) { + return { ok: true, deduped: true, at: t }; + } this.lastSent.set(key, t); if (this.lastSent.size > 256) { for (const [k, v] of this.lastSent) if (t - v > DEDUPE_WINDOW_MS) this.lastSent.delete(k); @@ -759,10 +827,16 @@ export class DiscordBridge { } /** Session tab created: make its thread now when `threadCreation` is `onSessionStart`. */ - async sessionCreated(sessionId: string, name: string | undefined, createdAt?: number): Promise<void> { + async sessionCreated( + sessionId: string, + name: string | undefined, + createdAt?: number, + resumed = false + ): Promise<void> { if (!this.loaded) await this.reload(); // Only a tab made just now: the same event also fires for sessions restored at server start. - if (createdAt !== undefined && this.now() - createdAt < 60_000) this.fresh.add(sessionId); + // A resumed tab is created now too, but its conversation is not new, so it is never hinted. + if (!resumed && createdAt !== undefined && this.now() - createdAt < 60_000) this.fresh.add(sessionId); if (!this.cfg.enabled || this.cfg.threadCreation !== 'onSessionStart') return; await this.threadFor(sessionId, name); } @@ -771,11 +845,12 @@ export class DiscordBridge { * First idle of a brand-new non-Claude session means its prompt is up: type the startup hint once. * Claude gets the same text as a launch flag instead, and a shell must never receive it. */ - async sessionIdle(sessionId: string, external: boolean): Promise<void> { + async sessionIdle(sessionId: string, hintable: boolean): Promise<void> { if (!this.fresh.delete(sessionId)) return; - if (!external || !this.cfg.enabled || !this.cfg.startupHint) return; + if (!hintable || !this.cfg.enabled || !this.cfg.startupHint) return; const text = sanitiseReply(`${DISCORD_STARTUP_HINT} Reply with just: ok`); - if (text) await this.deps.sendInput(sessionId, text).catch(() => false); + // fromUser false: this is Codeman talking, so it must not auto-name the tab or count as the person's input. + if (text) await this.deps.sendInput(sessionId, text, false).catch(() => false); } async sessionRenamed(sessionId: string, name: string | undefined): Promise<void> { @@ -788,12 +863,14 @@ export class DiscordBridge { /** Session tab deleted: apply `onSessionDelete` and forget the mapping. */ async sessionDeleted(sessionId: string): Promise<void> { + // Per-tab state goes whether or not the tab ever had a thread, so these maps cannot grow for the life of the server. + this.lastReply.delete(sessionId); + this.names.delete(sessionId); + this.fresh.delete(sessionId); const thread = this.threads[sessionId]; if (!thread) return; delete this.threads[sessionId]; - this.lastReply.delete(sessionId); - this.names.delete(sessionId); - await writeThreadMap(this.deps.configDir, this.threads).catch(() => undefined); + await this.persistThreads(); if (!this.cfg.enabled) return; if (this.cfg.onSessionDelete === 'delete') await this.api.remove(thread).catch(() => undefined); else if (this.cfg.onSessionDelete === 'archive') await this.api.archive(thread).catch(() => undefined); @@ -813,7 +890,10 @@ export class DiscordBridge { if (list.length === 0) return { paths: [] }; const dir = this.deps.sessionDir?.(sessionId); if (!dir) return { paths: [], refused: 'That tab has no local folder to save files in, so no file was saved.' }; - const target = join(dir, RECEIVED_DIR); + // The message id goes into a file name, so it must be a snowflake and nothing else. + if (!SNOWFLAKE.test(m.id)) return { paths: [], refused: 'That message could not be saved.' }; + const target = await ensureReceivedDir(dir); + if (!target) return { paths: [], refused: 'The tab folder is not safe to write to, so no file was saved.' }; const paths: string[] = []; const skipped: string[] = []; for (const [i, a] of list.entries()) { @@ -832,9 +912,9 @@ export class DiscordBridge { skipped.push(name); continue; } - await fs.mkdir(target, { recursive: true }); const dest = join(target, `${m.id}-${name}`); - await fs.writeFile(dest, buf, { mode: 0o600 }); + // 'wx' (O_EXCL) also refuses a symlink planted at the name. + await fs.writeFile(dest, buf, { mode: 0o600, flag: 'wx' }); paths.push(dest); } catch { skipped.push(name); @@ -857,7 +937,11 @@ export class DiscordBridge { if (!sessionId) return; const t = this.now(); const prev = this.lastReply.get(sessionId); - if (prev !== undefined && t - prev < REPLY_MIN_INTERVAL_MS) return; + if (prev !== undefined && t - prev < REPLY_MIN_INTERVAL_MS) { + // Too soon after the last reply: say it was not typed, instead of dropping it silently. + await this.api.react(m.channel_id, m.id, '⏳').catch(() => undefined); + return; + } this.lastReply.set(sessionId, t); const saved = await this.saveAttachments(sessionId, m); const text = sanitiseReply(m.content ?? '', saved.paths); @@ -866,7 +950,7 @@ export class DiscordBridge { return; } if (saved.refused) await this.say(m.channel_id, saved.refused); - const ok = await this.deps.sendInput(sessionId, text).catch(() => false); + const ok = await this.deps.sendInput(sessionId, text, true).catch(() => false); const reply = ok ? this.api.react(m.channel_id, m.id, '✅') : this.api.post(m.channel_id, { diff --git a/src/web/routes/config-backup-routes.ts b/src/web/routes/config-backup-routes.ts index a59d852d..1eadcfbb 100644 --- a/src/web/routes/config-backup-routes.ts +++ b/src/web/routes/config-backup-routes.ts @@ -150,7 +150,12 @@ export async function runConfigBackup( }); } -export function registerConfigBackupRoutes(app: FastifyInstance): void { +export interface ConfigBackupRouteDeps { + /** Re-read discord.json into the running bridge (it caches its config and would keep the pre-restore token). */ + reloadDiscord?: () => Promise<void>; +} + +export function registerConfigBackupRoutes(app: FastifyInstance, deps: ConfigBackupRouteDeps = {}): void { const deny = (req: FastifyRequest, reply: FastifyReply): ApiResponse<never> | null => { if (isMultiUserMode() && !isAdmin(req)) { reply.code(403); @@ -236,6 +241,8 @@ export function registerConfigBackupRoutes(app: FastifyInstance): void { if (restored.includes('intents.json')) intentStore.reload(); // restoreConfigBackup writes clis.json directly (the one writer besides mutateRegistryFile). if (restored.includes('clis.json')) reloadCliRegistry(); + // The Discord bridge caches discord.json and rewrites nothing from it, so a restore must reach the running bridge. + if (restored.includes('discord.json')) void deps.reloadDiscord?.().catch(() => undefined); // A restore of users.json replaces accounts, password hashes, roles and bypass grants. if (isMultiUserMode()) { void appendAdminAudit({ diff --git a/src/web/routes/discord-routes.ts b/src/web/routes/discord-routes.ts index f730c8ce..96b62e4e 100644 --- a/src/web/routes/discord-routes.ts +++ b/src/web/routes/discord-routes.ts @@ -171,7 +171,8 @@ export function registerDiscordRoutes(app: FastifyInstance, deps: DiscordRouteDe host: deps.hostTitle(), file: upload, }, - urgent === true + // deliberate: an agent's own message is never deduped, so a second, different post is not lost. + { urgent: urgent === true, deliberate: true } ); if (result === null) { reply.code(409); diff --git a/src/web/server.ts b/src/web/server.ts index b7b092ce..42ea4967 100644 --- a/src/web/server.ts +++ b/src/web/server.ts @@ -512,11 +512,13 @@ export class WebServer extends EventEmitter { private discordBridge = new DiscordBridge({ configDir: getDataDir(), fetchImpl: webviewFetch, - sendInput: async (sessionId, text) => { + sendInput: async (sessionId, text, fromUser) => { const session = this.sessions.get(sessionId); if (!session) return false; - if (await session.writeViaMux(text, { fromUser: true }).catch(() => false)) return true; - return session.write(text, { fromUser: true }); + // fromUser is true only for a Discord reply (a person typed it); the startup hint is Codeman's own + // text and must not auto-name the tab or count as the person's input. + if (await session.writeViaMux(text, { fromUser }).catch(() => false)) return true; + return session.write(text, { fromUser }); }, // Remote (ssh) sessions keep their folder on another host, so received files have nowhere local to go. sessionDir: (sessionId) => { @@ -1342,7 +1344,7 @@ export class WebServer extends EventEmitter { registerWebviewRoutes(this.app, ctx, this.basePath); registerTabLayoutRoutes(this.app, ctx); registerMcpSyncRoutes(this.app); - registerConfigBackupRoutes(this.app); + registerConfigBackupRoutes(this.app, { reloadDiscord: () => this.discordBridge.reload() }); registerWebhookRoutes(this.app, { notifier: this.webhookNotifier, configDir: getDataDir(), @@ -1792,7 +1794,11 @@ export class WebServer extends EventEmitter { // detach: mail posted before that is still there when the session is re-attached // (while detached, the routes answer 404, so nothing new arrives). Before // cancelAll, so a reader released by it reads an empty inbox. - if (killMux) agentInbox.drop(sessionId); + if (killMux) { + agentInbox.drop(sessionId); + // A real delete only: a detach (killMux=false) leaves the session running and re-attachable. + void this.discordBridge.sessionDeleted(sessionId).catch(() => undefined); + } sessionWaits.notifySignal(sessionId, 'exit'); sessionWaits.cancelAll(sessionId); approvalInbox.resolveForSession(sessionId, 'session_ended'); @@ -2727,41 +2733,42 @@ export class WebServer extends EventEmitter { this.bridgeSessionEvent(event, data); } - private isExternalSession(sessionId: string): boolean { + /** + * Whether the startup hint may be typed into this tab: an external CLI (never Claude, which has it as a + * launch flag, and never a shell), running locally. A remote (ssh) or Docker tab has no `codeman` on its + * PATH, so the hint would tell the agent to run a command it does not have. + */ + private isHintableSession(sessionId: string): boolean { const session = this.sessions.get(sessionId); - return !!session && isExternalCliMode(session.mode); + return !!session && isExternalCliMode(session.mode) && !session.remote && !session.docker; } /** Keep the Discord thread in step with the tab: created, renamed, deleted. Never throws. */ private bridgeSessionEvent(event: string, data: unknown): void { - if ( - event !== SseEvent.SessionCreated && - event !== SseEvent.SessionUpdated && - event !== SseEvent.SessionDeleted && - event !== SseEvent.SessionIdle - ) { + // Deletion is NOT handled here: `session:deleted` also fires for a detach (killMux=false), where the + // session keeps running and can be re-attached. The real delete calls the bridge from the cleanup path. + if (event !== SseEvent.SessionCreated && event !== SseEvent.SessionUpdated && event !== SseEvent.SessionIdle) { return; } const d = (data ?? {}) as { id?: string; name?: string; createdAt?: number; - session?: { id?: string; name?: string; createdAt?: number }; + resumeSessionId?: string; + session?: { id?: string; name?: string; createdAt?: number; resumeSessionId?: string }; }; const s = d.session ?? d; if (!s.id) return; const id = s.id; const run = - event === SseEvent.SessionDeleted - ? this.discordBridge.sessionDeleted(id) - : event === SseEvent.SessionIdle - ? // Claude is told at launch; a shell must never be typed at. - this.discordBridge.sessionIdle(id, this.isExternalSession(id)) - : event === SseEvent.SessionCreated - ? this.discordBridge - .sessionCreated(id, s.name, s.createdAt) - .then(() => this.discordBridge.sessionRenamed(id, s.name)) - : this.discordBridge.sessionRenamed(id, s.name); + event === SseEvent.SessionIdle + ? // Claude is told at launch; a shell, a remote or a Docker tab must never be typed at. + this.discordBridge.sessionIdle(id, this.isHintableSession(id)) + : event === SseEvent.SessionCreated + ? this.discordBridge + .sessionCreated(id, s.name, s.createdAt, !!s.resumeSessionId) + .then(() => this.discordBridge.sessionRenamed(id, s.name)) + : this.discordBridge.sessionRenamed(id, s.name); void run.catch(() => undefined); } @@ -3009,7 +3016,8 @@ export class WebServer extends EventEmitter { urgency: template.urgency as WebhookUrgency, sessionId: sessionId || undefined, sessionName: sessionName || undefined, - tail: dcSession ? this.discordTail(dcSession) : undefined, + // A function, so the screen is only read (a synchronous tmux exec) once the bridge has decided to post. + tail: dcSession ? () => this.discordTail(dcSession) : undefined, host: this.windowTitle, }) .catch(() => undefined); @@ -3830,6 +3838,8 @@ export class WebServer extends EventEmitter { } catch (err) { console.warn(`[Server] releasing the tab layout slot failed: ${getErrorMessage(err)}`); } + // A half-built session never had a thread, but its per-tab bridge state goes with it. + void this.discordBridge.sessionDeleted(sessionId).catch(() => undefined); // Any `session:updated` the half-built session emitted before it failed left a // tab on every other open board, and the client's handler is an upsert. this.broadcast(SseEvent.SessionDeleted, { id: sessionId }); @@ -4402,6 +4412,9 @@ export class WebServer extends EventEmitter { // only by the periodic idle sweep, whose interval is disposed just below. closeAllLlamaSwapLogTails(); + // Close the Discord gateway socket and its heartbeat, and clear the bridge's per-tab maps. + this.discordBridge.stop(); + // Dispose all managed timers (intervals + resettable timeouts) this.cleanup.dispose(); diff --git a/test/discord-bridge.test.ts b/test/discord-bridge.test.ts index f7143ca7..57a4de71 100644 --- a/test/discord-bridge.test.ts +++ b/test/discord-bridge.test.ts @@ -3,13 +3,14 @@ * @fileoverview Discord bridge: pure rules, the 0600 store, the REST client, the gateway state * machine (fake socket) and the bridge's gates. No network, no real Discord. */ -import { mkdtempSync, readFileSync, rmSync, statSync } from 'node:fs'; +import { existsSync, mkdirSync, mkdtempSync, readFileSync, rmSync, statSync, symlinkSync, writeFileSync } from 'node:fs'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; import { buildPost, DISCORD_STARTUP_HINT, + ensureReceivedDir, isDiscordCdnUrl, safeAttachmentName, screenTail, @@ -261,7 +262,7 @@ describe('DiscordGateway', () => { describe('DiscordBridge', () => { let posts: Array<{ path: string; method: string; body: Record<string, unknown> | undefined }>; - let sendInput: ReturnType<typeof vi.fn<(id: string, text: string) => Promise<boolean>>>; + let sendInput: ReturnType<typeof vi.fn<(id: string, text: string, fromUser: boolean) => Promise<boolean>>>; let t: number; function bridge(overrides: Partial<DiscordConfig> = {}, fetchOverride?: DiscordFetch) { @@ -308,16 +309,65 @@ describe('DiscordBridge', () => { b.stop(); }); - it('dedupes the same event within 3 s but lets an explicit urgent post through', async () => { + it('dedupes an automatic event within 3 s and says so, which is not the "bridge is off" answer', async () => { const b = bridge(); - await b.notify(MSG); - await b.notify(MSG); + expect((await b.notify(MSG))?.ok).toBe(true); + const second = await b.notify(MSG); + expect(second).toMatchObject({ ok: true, deduped: true }); expect(posts.filter((p) => p.path.endsWith('/messages'))).toHaveLength(1); - await b.notify(MSG, true); + b.stop(); + }); + + it('never dedupes a deliberate post: two different messages from one tab within 3 s both go out', async () => { + const b = bridge(); + const a = await b.notify({ ...MSG, event: 'session:discord-notify', body: 'first' }, { deliberate: true }); + const c = await b.notify({ ...MSG, event: 'session:discord-notify', body: 'second' }, { deliberate: true }); + expect(a?.deduped).toBeUndefined(); + expect(c?.deduped).toBeUndefined(); expect(posts.filter((p) => p.path.endsWith('/messages'))).toHaveLength(2); b.stop(); }); + it('builds the alert tail only for a post that goes out (not while off, not when deduped)', async () => { + const tail = vi.fn(() => 'screen text'); + const b = bridge(); + await b.notify({ ...MSG, tail }); + expect(tail).toHaveBeenCalledTimes(1); + await b.notify({ ...MSG, tail }); // deduped + expect(tail).toHaveBeenCalledTimes(1); + expect(posts.at(-1)?.body?.content).toContain('screen text'); + await writeDiscordConfig(dir, { ...CFG, enabled: false }); + await b.reload(); + await b.notify({ ...MSG, event: 'hook:idle_prompt', tail }); // off + expect(tail).toHaveBeenCalledTimes(1); + b.stop(); + }); + + it('survives a tail that throws', async () => { + const b = bridge(); + const r = await b.notify({ + ...MSG, + tail: () => { + throw new Error('tmux gone'); + }, + }); + expect(r?.ok).toBe(true); + b.stop(); + }); + + it('writes the thread map one write at a time, so threads made at once all survive a restart', async () => { + let n = 0; + const b = bridge({}, async (url, init) => { + const path = new URL(String(url)).pathname.replace('/api/v10', ''); + if (path.endsWith('/threads')) return jsonRes({ id: String(500000000000000000n + BigInt(++n)) }); + return jsonRes({ id: 'msg' }); + }); + await Promise.all(['a1', 'a2', 'a3', 'a4'].map((id) => b.notify({ ...MSG, sessionId: id, event: `e-${id}` }))); + const map = await readThreadMap(dir); + expect(Object.keys(map).sort()).toEqual(['a1', 'a2', 'a3', 'a4']); + b.stop(); + }); + it('skips info events under scope attention and everything while disabled', async () => { const b = bridge(); expect(await b.notify({ ...MSG, urgency: 'info' })).toBeNull(); @@ -377,7 +427,7 @@ describe('DiscordBridge', () => { it('types an allowed reply into the mapped session and ticks it', async () => { const b = await ready(); await b.onGatewayMessage(reply()); - expect(sendInput).toHaveBeenCalledWith('s1', 'press y\r'); + expect(sendInput).toHaveBeenCalledWith('s1', 'press y\r', true); // a person typed it expect(posts[0].method).toBe('PUT'); expect(posts[0].path).toContain('/reactions/'); b.stop(); @@ -402,7 +452,9 @@ describe('DiscordBridge', () => { await b.onGatewayMessage(reply({ content: 'a\nb' })); await b.onGatewayMessage(reply({ content: 'again' })); expect(sendInput).toHaveBeenCalledTimes(1); - expect(sendInput).toHaveBeenCalledWith('s1', 'a b\r'); + expect(sendInput).toHaveBeenCalledWith('s1', 'a b\r', true); + // The second reply was not typed, and the person is told so instead of it vanishing. + expect(posts.some((p) => p.method === 'PUT' && p.path.includes(encodeURIComponent('⏳')))).toBe(true); b.stop(); }); @@ -416,6 +468,35 @@ describe('DiscordBridge', () => { }); }); + describe('per-tab state', () => { + const internals = (b: DiscordBridge) => + b as unknown as { names: Map<string, string>; lastReply: Map<string, number>; fresh: Set<string> }; + + it('a deleted tab that never had a thread still frees its entries', async () => { + const b = bridge(); + await b.reload(); + await b.sessionCreated('nothread', 'w', t - 1000); + await b.sessionRenamed('nothread', 'w'); + internals(b).lastReply.set('nothread', 1); + await b.sessionDeleted('nothread'); + expect(internals(b).names.has('nothread')).toBe(false); + expect(internals(b).lastReply.has('nothread')).toBe(false); + expect(internals(b).fresh.has('nothread')).toBe(false); + b.stop(); + }); + + it('stop() clears the maps and the gateway', async () => { + const b = bridge(); + await b.reload(); + await b.sessionCreated('x', 'w', t - 1000); + await b.sessionRenamed('x', 'w'); + b.stop(); + expect(internals(b).fresh.size).toBe(0); + expect(internals(b).names.size).toBe(0); + expect(b.gatewayState).toBe('off'); + }); + }); + describe('tab lifecycle', () => { it('creates the thread at once under onSessionStart, not under lazy', async () => { const lazy = bridge(); @@ -519,7 +600,7 @@ describe('files', () => { describe('DiscordBridge files and startup hint', () => { const reply = (over: Record<string, unknown> = {}) => ({ - id: 'm1', + id: '555555555555555555', channel_id: THREAD, guild_id: GUILD, content: '', @@ -530,7 +611,7 @@ describe('DiscordBridge files and startup hint', () => { ...over, }); let work: string; - let sent: Array<[string, string]>; + let sent: Array<[string, string, boolean]>; let posted: Array<{ path: string; body?: Record<string, unknown> }>; let downloads: string[]; @@ -547,7 +628,7 @@ describe('DiscordBridge files and startup hint', () => { posted.push({ path, body: init.body ? JSON.parse(String(init.body)) : undefined }); return path.endsWith('/threads') ? jsonRes({ id: THREAD }) : jsonRes({ id: 'msg' }); }, - sendInput: async (id, text) => (sent.push([id, text]), true), + sendInput: async (id, text, fromUser) => (sent.push([id, text, fromUser]), true), sessionDir: dirFor, socketFactory: () => new FakeSocket(), now: () => t, @@ -573,10 +654,10 @@ describe('DiscordBridge files and startup hint', () => { it('saves an attachment under .codeman-discord and types its path, even with no text', async () => { const b = await make(); await b.onGatewayMessage(reply()); - const dest = join(work, '.codeman-discord', 'm1-shot_one.png'); + const dest = join(work, '.codeman-discord', '555555555555555555-shot_one.png'); expect(readFileSync(dest)).toEqual(Buffer.from([7, 8, 9])); expect(statSync(dest).mode & 0o777).toBe(0o600); - expect(sent).toEqual([['s1', `[attached: ${dest}]\r`]]); + expect(sent).toEqual([['s1', `[attached: ${dest}]\r`, true]]); b.stop(); }); @@ -627,10 +708,28 @@ describe('DiscordBridge files and startup hint', () => { expect(sent[0][0]).toBe('s3'); expect(sent[0][1]).toContain('codeman notify'); expect(sent[0][1].endsWith('\r')).toBe(true); + expect(sent[0][2]).toBe(false); // Codeman's own text: not a person's input, so it cannot auto-name the tab expect(DISCORD_STARTUP_HINT).toContain('codeman notify'); b.stop(); }); + it('does not hint a resumed tab, even though it was created just now', async () => { + const b = await make(); + await b.sessionCreated('s6', 'resumed', t - 1000, true); + await b.sessionIdle('s6', true); + expect(sent).toHaveLength(0); + b.stop(); + }); + + it('does not save a file for a message id that is not a snowflake, and says so', async () => { + const b = await make(); + await b.onGatewayMessage(reply({ id: '../../x' })); + expect(downloads).toHaveLength(0); + expect(sent).toHaveLength(0); + expect(String(posted.at(-1)?.body?.content)).toMatch(/could not be saved/); + b.stop(); + }); + it('does not hint a restored (old) session, or when startupHint is off', async () => { const b = await make(); await b.sessionCreated('s4', 'old', 1); // created long ago: restored at server start @@ -684,3 +783,36 @@ describe('screenTail', () => { expect(screenTail('\n\n ───── \n')).toBe(''); }); }); + +describe('ensureReceivedDir', () => { + let work: string; + beforeEach(() => { + work = mkdtempSync(join(tmpdir(), 'discord-recv-')); + }); + afterEach(() => rmSync(work, { recursive: true, force: true })); + + it('makes the folder and ignores it with a .gitignore of *, once', async () => { + const dir = await ensureReceivedDir(work); + expect(dir).toBe(join(work, '.codeman-discord')); + expect(readFileSync(join(dir!, '.gitignore'), 'utf8')).toBe('*\n'); + }); + + it('leaves a .gitignore that is already there alone', async () => { + mkdirSync(join(work, '.codeman-discord')); + writeFileSync(join(work, '.codeman-discord', '.gitignore'), 'mine\n'); + expect(await ensureReceivedDir(work)).toBe(join(work, '.codeman-discord')); + expect(readFileSync(join(work, '.codeman-discord', '.gitignore'), 'utf8')).toBe('mine\n'); + }); + + it('refuses a symlink or a plain file at the folder, and a workspace that is not a directory', async () => { + const elsewhere = mkdtempSync(join(tmpdir(), 'discord-elsewhere-')); + symlinkSync(elsewhere, join(work, '.codeman-discord')); + expect(await ensureReceivedDir(work)).toBeNull(); + expect(existsSync(join(elsewhere, '.gitignore'))).toBe(false); + rmSync(elsewhere, { recursive: true, force: true }); + rmSync(join(work, '.codeman-discord')); + writeFileSync(join(work, '.codeman-discord'), 'x'); + expect(await ensureReceivedDir(work)).toBeNull(); + expect(await ensureReceivedDir(join(work, 'missing'))).toBeNull(); + }); +}); diff --git a/test/discord-push-glue.test.ts b/test/discord-push-glue.test.ts index 4c77b4bb..04c18ba5 100644 --- a/test/discord-push-glue.test.ts +++ b/test/discord-push-glue.test.ts @@ -24,6 +24,7 @@ type Internals = { mux: { capturePaneText?: (name: string) => string | null }; sendPushNotifications: (event: string, data: Record<string, unknown>) => Promise<void>; broadcast: (event: string, data: unknown) => void; + cleanupSession: (id: string, killMux?: boolean, reason?: string) => Promise<void>; }; function server(): Internals { @@ -35,6 +36,7 @@ function server(): Internals { sessionRenamed: vi.fn(async () => undefined), sessionDeleted: vi.fn(async () => undefined), sessionIdle: vi.fn(async () => undefined), + stop: vi.fn(), }; return s; } @@ -45,22 +47,28 @@ describe('sendPushNotifications -> Discord', () => { it('posts a permission prompt with zero push subscriptions, with the rendered screen as the tail', async () => { const s = server(); s.sessions.set('s1', { muxName: 'codeman-s1', textOutput: '', mode: 'claude' }); - s.mux = { capturePaneText: () => 'Which option?\n──────────\n 1. Cap GPUs\n\n 2. Cap CPU\n' }; + const capture = vi.fn(() => 'Which option?\n──────────\n 1. Cap GPUs\n\n 2. Cap CPU\n'); + s.mux = { capturePaneText: capture }; await s.sendPushNotifications('hook:permission_prompt', { sessionId: 's1', sessionName: 'w1-app', tool_name: 'Bash', }); expect(s.discordBridge.notify).toHaveBeenCalledTimes(1); - expect(s.discordBridge.notify.mock.calls[0][0]).toMatchObject({ + const msg = s.discordBridge.notify.mock.calls[0][0]; + expect(msg).toMatchObject({ event: 'hook:permission_prompt', title: 'Permission Required', body: '[w1-app] Tool: Bash', urgency: 'critical', sessionId: 's1', sessionName: 'w1-app', - tail: 'Which option?\n 1. Cap GPUs\n 2. Cap CPU', }); + // A function, so the screen is read (a synchronous tmux exec) only once the bridge will post. + expect(typeof msg.tail).toBe('function'); + expect(capture).not.toHaveBeenCalled(); + expect(msg.tail()).toBe('Which option?\n 1. Cap GPUs\n 2. Cap CPU'); + expect(capture).toHaveBeenCalledTimes(1); expect(sendNotification).not.toHaveBeenCalled(); }); @@ -68,7 +76,7 @@ describe('sendPushNotifications -> Discord', () => { const s = server(); s.sessions.set('s2', { muxName: null, textOutput: 'plain answer\n', mode: 'claude' }); await s.sendPushNotifications('hook:idle_prompt', { sessionId: 's2', sessionName: 'w2' }); - expect(s.discordBridge.notify.mock.calls[0][0].tail).toBe('plain answer'); + expect(s.discordBridge.notify.mock.calls[0][0].tail()).toBe('plain answer'); await s.sendPushNotifications('hook:idle_prompt', { sessionId: 'gone', sessionName: 'w3' }); expect(s.discordBridge.notify.mock.calls[1][0].tail).toBeUndefined(); }); @@ -87,27 +95,57 @@ describe('broadcast -> Discord tab lifecycle', () => { expect(s.discordBridge.sessionCreated).toHaveBeenCalledWith('s1', 'w1', 123); s.broadcast(SseEvent.SessionUpdated, { id: 's1', name: 'renamed' }); expect(s.discordBridge.sessionRenamed).toHaveBeenCalledWith('s1', 'renamed'); + }); + + it('does not touch the thread on session:deleted, which a detach also fires', () => { + const s = server(); s.broadcast(SseEvent.SessionDeleted, { id: 's1' }); - expect(s.discordBridge.sessionDeleted).toHaveBeenCalledWith('s1'); + expect(s.discordBridge.sessionDeleted).not.toHaveBeenCalled(); + }); + + it('tells the bridge about a real delete (killMux true) but not a detach (killMux false)', async () => { + const s = server(); + await s.cleanupSession('gone', false); + expect(s.discordBridge.sessionDeleted).not.toHaveBeenCalled(); + await s.cleanupSession('gone', true); + expect(s.discordBridge.sessionDeleted).toHaveBeenCalledWith('gone'); + }); + + it('marks a resumed tab as resumed, so it is never sent the typed hint', () => { + const s = server(); + s.broadcast(SseEvent.SessionCreated, { session: { id: 's9', name: 'w9', createdAt: 5, resumeSessionId: 'abc' } }); + expect(s.discordBridge.sessionCreated).toHaveBeenCalledWith('s9', 'w9', 5, true); + s.broadcast(SseEvent.SessionCreated, { session: { id: 's8', name: 'w8', createdAt: 6 } }); + expect(s.discordBridge.sessionCreated).toHaveBeenLastCalledWith('s8', 'w8', 6, false); }); it('ignores an event with no session id', () => { const s = server(); - s.broadcast(SseEvent.SessionDeleted, {}); - expect(s.discordBridge.sessionDeleted).not.toHaveBeenCalled(); + s.broadcast(SseEvent.SessionUpdated, {}); + expect(s.discordBridge.sessionRenamed).not.toHaveBeenCalled(); }); - it('tells the bridge whether an idle session is an external CLI: codex yes, claude and shell no', () => { + it('allows the typed hint only for a local external CLI: not Claude, a shell, a remote or a Docker tab', () => { const s = server(); s.sessions.set('cx', { mode: 'codex' }); s.sessions.set('cl', { mode: 'claude' }); s.sessions.set('sh', { mode: 'shell' }); - for (const id of ['cx', 'cl', 'sh', 'missing']) s.broadcast(SseEvent.SessionIdle, { id }); + s.sessions.set('rm', { mode: 'codex', remote: { label: 'box' } }); + s.sessions.set('dk', { mode: 'codex', docker: { image: 'x' } }); + for (const id of ['cx', 'cl', 'sh', 'rm', 'dk', 'missing']) s.broadcast(SseEvent.SessionIdle, { id }); expect(s.discordBridge.sessionIdle.mock.calls).toEqual([ ['cx', true], ['cl', false], ['sh', false], + ['rm', false], + ['dk', false], ['missing', false], ]); }); + + it('closes the bridge when the server stops', async () => { + const s = server(); + await (s as unknown as { stop: () => Promise<void> }).stop(); + expect(s.discordBridge.stop).toHaveBeenCalledTimes(1); + }); }); diff --git a/test/routes/config-backup-routes.test.ts b/test/routes/config-backup-routes.test.ts index 3eb25547..854b2e6e 100644 --- a/test/routes/config-backup-routes.test.ts +++ b/test/routes/config-backup-routes.test.ts @@ -4,7 +4,7 @@ * * ⚠️ test/setup.ts gives the whole FILE one temp HOME (and so one data dir). Port: N/A (inject()). */ -import { afterEach, beforeEach, describe, expect, it } from 'vitest'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; import { existsSync, mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs'; import { tmpdir } from 'node:os'; import { dirname, join } from 'node:path'; @@ -120,6 +120,22 @@ describe('/api/config-backups', () => { } }); + it('a restored discord.json reaches the running bridge (it caches its config)', async () => { + const reloadDiscord = vi.fn(async () => undefined); + const { app } = await createRouteTestHarness((a) => registerConfigBackupRoutes(a, { reloadDiscord })); + writeFileSync(join(dirname(SETTINGS_PATH), 'discord.json'), '{"enabled":false}', { mode: 0o600 }); + const id = (await app.inject({ method: 'POST', url: '/api/config-backups' })).json().data.id; + writeFileSync(join(dirname(SETTINGS_PATH), 'discord.json'), '{"enabled":true}', { mode: 0o600 }); + const res = await app.inject({ method: 'POST', url: `/api/config-backups/${id}/restore` }); + expect(res.json().data.restored).toContain('discord.json'); + expect(reloadDiscord).toHaveBeenCalledTimes(1); + // A restore that does not carry the file leaves the bridge alone. + rmSync(join(dirname(SETTINGS_PATH), 'discord.json'), { force: true }); + const id2 = (await app.inject({ method: 'POST', url: '/api/config-backups' })).json().data.id; + await app.inject({ method: 'POST', url: `/api/config-backups/${id2}/restore` }); + expect(reloadDiscord).toHaveBeenCalledTimes(1); + }); + it('a restored intents.json is not reverted by the next captured prompt', async () => { const { app } = await createRouteTestHarness(registerConfigBackupRoutes); const dir = mkdtempSync(join(tmpdir(), 'cfg-backup-intent-')); diff --git a/test/routes/discord-routes.test.ts b/test/routes/discord-routes.test.ts index 19f42846..14efe600 100644 --- a/test/routes/discord-routes.test.ts +++ b/test/routes/discord-routes.test.ts @@ -109,6 +109,7 @@ describe('/api/discord', () => { }); describe('POST /api/sessions/:id/discord-notify', () => { + const url = '/api/sessions/test-session-1/discord-notify'; it('is refused while the bridge is off, and posts into the session thread once on', async () => { const { app, bridge } = await harness(); const url = `/api/sessions/test-session-1/discord-notify`; @@ -147,6 +148,19 @@ describe('POST /api/sessions/:id/discord-notify', () => { bridge.stop(); }); + it('lets two different deliberate messages from one tab through within 3 s (neither is lost)', async () => { + const { app, bridge } = await harness(); + await app.inject({ method: 'PUT', url: '/api/discord', payload: FULL }); + const first = await app.inject({ method: 'POST', url, payload: { message: 'first thing' } }); + const second = await app.inject({ method: 'POST', url, payload: { message: 'second thing' } }); + expect(first.statusCode).toBe(200); + expect(second.statusCode).toBe(200); + expect(second.json().data.deduped).toBeUndefined(); + const sent = fetchImpl.mock.calls.map((c) => String(c[1].body)); + expect(sent.filter((b) => b.includes('first thing') || b.includes('second thing'))).toHaveLength(2); + bridge.stop(); + }); + it('rejects an empty or oversized message', async () => { const { app } = await harness(); const url = `/api/sessions/test-session-1/discord-notify`;