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.

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();
+  });
+});

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);
+  });
 });

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-'));

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`;