diff --git a/docs/api-reference.md b/docs/api-reference.md index 52f2929e..77d92d94 100644 --- a/docs/api-reference.md +++ b/docs/api-reference.md @@ -1495,7 +1495,7 @@ One thread per tab in a private Discord channel, alerts out and replies typed ba | `GET` | `/api/discord` | none | Config without the token, plus `hasToken`, `tokenMasked`, `gateway` (`off` \| `connecting` \| `connected` \| `error`), `gatewayError` and `lastResult`. | | `PUT` | `/api/discord` | `{ enabled?, token?, guildId?, parentChannelId?, allowedUserIds?, threadCreation?, onSessionDelete?, pingMode?, scope?, startupHint?, typedHint? }` (strict) | `threadCreation`: `lazy` \| `onSessionStart`. `onSessionDelete`: `keep` \| `archive` \| `delete`. `pingMode`: `off` \| `critical` \| `all`. `scope`: `attention` \| `all`. An absent `token` keeps the saved one; `""` clears it. `400` for a non-numeric id or enabling without a token, guild, channel and at least one allowed user. | | `POST` | `/api/discord/test` | none | Posts one message to the parent channel with the saved config, even while disabled. `200` with `data.ok`; `400` if no token or channel is saved. | -| `POST` | `/api/sessions/:id/discord-notify` | `{ message, urgent?, file? }` (strict, message 1 to 1500 characters) | A CLI asks for the user: posts into that tab's thread, pinging when `urgent`. `file` is an absolute path inside the tab's working folder (real path, regular file, at most 8 MB, local sessions only) uploaded with the post; `400` otherwise. `409` while the bridge is disabled, `429` past 6 posts a minute per tab. The `codeman notify "message" [--urgent] [--file path]` command wraps this route. | +| `POST` | `/api/sessions/:id/discord-notify` | `{ message, urgent?, file? }` (strict, message 1 to 1500 characters) | A CLI asks for the user: posts into that tab's thread, pinging when `urgent`. `file` is an absolute path inside the tab's working folder (real path, regular file, at most 8 MB, local sessions only) uploaded with the post; `400` otherwise. A `200` carries the delivery result: `data.ok` is `false` (with `data.error`) when Discord refused or failed the post. `409` while the bridge is disabled, `429` past 6 posts a minute per tab. The `codeman notify "message" [--urgent] [--file path]` command wraps this route. | Only replies from `allowedUserIds`, in `guildId`, inside a thread the bridge created are typed into a session, as one printable line plus Enter, at most one per second per tab. Attached files are saved under `.codeman-discord/` in the tab's folder and their paths appended to that line (see [discord-bridge.md](discord-bridge.md)). diff --git a/src/cli-notify.ts b/src/cli-notify.ts index d14f102f..44e19b72 100644 --- a/src/cli-notify.ts +++ b/src/cli-notify.ts @@ -14,6 +14,8 @@ import https from 'node:https'; import { isAbsolute, join } from 'node:path'; import { readCodemanCredentials } from './codeman-credentials.js'; +const REQUEST_TIMEOUT_MS = 30_000; + export interface NotifyOptions { message: string; urgent?: boolean; @@ -32,7 +34,7 @@ export type JsonPost = ( apiUrl: string, apiPath: string, payload: unknown -) => Promise<{ status: number; error?: string }>; +) => Promise<{ status: number; error?: string; delivery?: { ok: boolean; error?: string } }>; /** POST JSON to the Codeman API with the same credentials `codeman attach` uses. Status 0 means no answer. */ export const postJson: JsonPost = (apiUrl, apiPath, payload) => { @@ -56,21 +58,28 @@ export const postJson: JsonPost = (apiUrl, apiPath, payload) => { path: `${url.pathname}${url.search}`, rejectUnauthorized: false, headers, + timeout: REQUEST_TIMEOUT_MS, }, (res) => { let text = ''; res.on('data', (c) => (text += c)); res.on('end', () => { let error: string | undefined; + let delivery: { ok: boolean; error?: string } | undefined; try { - error = (JSON.parse(text) as { error?: string }).error; + const body = JSON.parse(text) as { error?: string; data?: { ok?: unknown; error?: string } }; + error = body.error; + // A 200 only says Codeman took the request; `data.ok` says whether Discord took the post. + if (typeof body.data?.ok === 'boolean') delivery = { ok: body.data.ok, error: body.data.error }; } catch { /* not JSON */ } - resolve({ status: res.statusCode ?? 0, error }); + resolve({ status: res.statusCode ?? 0, error, delivery }); }); } ); + // A server that takes the connection and never answers must not hang the agent: status 0 is "no answer". + req.on('timeout', () => req.destroy()); req.on('error', () => resolve({ status: 0 })); req.write(body); req.end(); @@ -91,12 +100,17 @@ export async function runNotify( const payload: { message: string; urgent?: boolean; file?: string } = { message: opts.message }; if (opts.urgent) payload.urgent = true; if (opts.file) payload.file = isAbsolute(opts.file) ? opts.file : join(cwd, opts.file); - const { status, error } = await post( + const { status, error, delivery } = await post( apiUrl, `/api/sessions/${encodeURIComponent(sessionId)}/discord-notify`, payload ); - if (status >= 200 && status < 300) return { code: 0, out: 'Posted to Discord' }; + if (status >= 200 && status < 300) { + if (delivery?.ok === false) { + return { code: 1, err: `Could not post to Discord${delivery.error ? `: ${delivery.error}` : ''}` }; + } + return { code: 0, out: 'Posted to Discord' }; + } // The bridge being off is not a failure: the caller just says it in the terminal. if (status === 409) return { code: 0, out: 'The Discord bridge is off. Say it in the terminal instead.' }; if (status === 429) diff --git a/src/discord-bridge.ts b/src/discord-bridge.ts index c6e65268..54105e9d 100644 --- a/src/discord-bridge.ts +++ b/src/discord-bridge.ts @@ -306,20 +306,32 @@ export async function writeDiscordConfig(configDir: string, cfg: DiscordConfig): await writePrivate(discordConfigPath(configDir), coerce(cfg), 0o600); } -export async function readThreadMap(configDir: string): Promise> { +/** The thread map plus the parent channel it was built under (empty for an older flat file). */ +async function readThreadFile(configDir: string): Promise<{ parent: string; threads: Record }> { try { const raw = JSON.parse(await fs.readFile(join(configDir, THREADS_FILE), 'utf-8')) as Record; - const out: Record = {}; - for (const [k, v] of Object.entries(raw)) if (typeof v === 'string' && SNOWFLAKE.test(v)) out[k] = v; - return out; + const nested = typeof raw.threads === 'object' && raw.threads !== null; + const entries = Object.entries(nested ? (raw.threads as Record) : raw); + const threads: Record = {}; + for (const [k, v] of entries) if (typeof v === 'string' && SNOWFLAKE.test(v)) threads[k] = v; + const parent = nested && typeof raw.parentChannelId === 'string' ? raw.parentChannelId : ''; + return { parent, threads }; } catch { - return {}; + return { parent: '', threads: {} }; } } -async function writeThreadMap(configDir: string, map: Record): Promise { +export async function readThreadMap(configDir: string): Promise> { + return (await readThreadFile(configDir)).threads; +} + +async function writeThreadMap( + configDir: string, + parentChannelId: string, + threads: Record +): Promise { if (!existsSync(configDir)) mkdirSync(configDir, { recursive: true }); - await writePrivate(join(configDir, THREADS_FILE), map, 0o600); + await writePrivate(join(configDir, THREADS_FILE), { parentChannelId, threads }, 0o600); } // --------------------------------------------------------------------------- @@ -657,7 +669,9 @@ export class DiscordBridge { private names = new Map(); /** Brand-new sessions that have not yet been sent the startup hint (non-Claude CLIs). */ private fresh = new Set(); - private pending = new Map>(); + private pending = new Map>(); + /** The parent channel the thread map was built under. */ + private threadsParent = ''; /** Thread-map writes run one after another: concurrent creations must not interleave on the tmp file. */ private persistChain: Promise = Promise.resolve(); private mapLoad?: Promise; @@ -693,11 +707,23 @@ export class DiscordBridge { this.cfg = await readDiscordConfig(this.deps.configDir); // The thread map is read once, by whoever gets here first; everyone else waits for that read. A second // read finishing later would replace the map and drop threads made in between. - this.mapLoad ??= readThreadMap(this.deps.configDir).then((saved) => { - this.threads = { ...saved, ...this.threads }; + this.mapLoad ??= readThreadFile(this.deps.configDir).then((saved) => { + this.threads = { ...saved.threads, ...this.threads }; + this.threadsParent ||= saved.parent; this.loaded = true; }); await this.mapLoad; + // Threads belong to the channel they were made under: a new parent channel (or guild) starts afresh, + // or every tab would keep posting into the old one. + const parent = this.cfg.parentChannelId; + if (parent && parent !== this.threadsParent) { + if (this.threadsParent !== '') { + this.threads = {}; + this.pending.clear(); + } + this.threadsParent = parent; + await this.persistThreads(); + } this.gateway?.stop(); this.gateway = null; activeStartupHint = this.cfg.enabled && this.cfg.token && this.cfg.startupHint ? DISCORD_STARTUP_HINT : undefined; @@ -742,13 +768,13 @@ export class DiscordBridge { private persistThreads(): Promise { this.persistChain = this.persistChain - .then(() => writeThreadMap(this.deps.configDir, { ...this.threads })) + .then(() => writeThreadMap(this.deps.configDir, this.threadsParent, { ...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 { + private threadFor(sessionId: string, name: string | undefined): Promise { const existing = this.threads[sessionId]; if (existing) return Promise.resolve(existing); const inflight = this.pending.get(sessionId); @@ -756,16 +782,12 @@ export class DiscordBridge { const p = (async () => { try { const t = await this.api.createThread(this.cfg.parentChannelId, threadNameFor(name, sessionId)); - if (!t?.id) { - this.last = this.fail(new Error('Discord did not return a thread')); - return null; - } + if (!t?.id) return this.fail(new Error('Discord did not return a thread')); this.threads[sessionId] = t.id; await this.persistThreads(); return t.id; } catch (err) { - this.last = this.fail(err); - return null; + return this.fail(err); } finally { this.pending.delete(sessionId); } @@ -799,7 +821,7 @@ export class DiscordBridge { let target = this.cfg.parentChannelId; if (msg.sessionId) { const thread = await this.threadFor(msg.sessionId, msg.sessionName); - if (!thread) return this.last; + if (typeof thread !== 'string') return (this.last = thread); target = thread; } const send = (to: string) => (msg.file ? this.api.postWithFile(to, post, msg.file) : this.api.post(to, post)); @@ -810,7 +832,7 @@ export class DiscordBridge { if (msg.sessionId && err instanceof DiscordHttpError && err.status === 404) { delete this.threads[msg.sessionId]; const fresh = await this.threadFor(msg.sessionId, msg.sessionName); - if (!fresh) return this.last; + if (typeof fresh !== 'string') return (this.last = fresh); await send(fresh); } else { throw err; @@ -852,7 +874,8 @@ export class DiscordBridge { // 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); if (!this.cfg.enabled || this.cfg.threadCreation !== 'onSessionStart') return; - await this.threadFor(sessionId, name); + const made = await this.threadFor(sessionId, name); + if (typeof made !== 'string') this.last = made; } /** diff --git a/src/web/routes/discord-routes.ts b/src/web/routes/discord-routes.ts index 42b63a7a..771e6830 100644 --- a/src/web/routes/discord-routes.ts +++ b/src/web/routes/discord-routes.ts @@ -96,6 +96,10 @@ async function fileProblem( if ((await probePathKind(workingDir, { pastCap: true })) !== 'directory') { return describeUnknownPath('The session folder', workingDir, { pastCap: true }); } + // The agent chose this path: probe it bounded too, before realpath can hold a worker on a dead mount. + const kind = await probePathKind(file, { pastCap: true }); + if (kind === 'unknown') return describeUnknownPath('The file', file, { pastCap: true }); + if (kind !== 'file') return 'Not a regular file'; try { const [root, real] = await Promise.all([realpath(workingDir), realpath(file)]); const rel = relative(root, real); diff --git a/test/cli-notify.test.ts b/test/cli-notify.test.ts index fa20caee..cb7407fe 100644 --- a/test/cli-notify.test.ts +++ b/test/cli-notify.test.ts @@ -10,8 +10,8 @@ import { postJson, runNotify, type JsonPost } from '../src/cli-notify.js'; const ENV = { CODEMAN_SESSION_ID: 'abc12345-full-id', CODEMAN_API_URL: 'http://127.0.0.1:3000' }; -function fake(status: number, error?: string) { - const post = vi.fn(async () => ({ status, error })); +function fake(status: number, error?: string, delivery?: { ok: boolean; error?: string }) { + const post = vi.fn(async () => ({ status, error, delivery })); return post; } @@ -47,6 +47,17 @@ describe('runNotify', () => { expect(post.mock.calls[0].slice(0, 2)).toEqual(['http://h:1', '/api/sessions/other/discord-notify']); }); + it('exits 1 when Codeman answered 200 but Discord refused the post, so a blocked agent knows nobody was told', async () => { + const r = await runNotify( + { message: 'x' }, + ENV, + '/w', + fake(200, undefined, { ok: false, error: 'The bot token was refused' }) + ); + expect(r).toEqual({ code: 1, err: 'Could not post to Discord: The bot token was refused' }); + expect((await runNotify({ message: 'x' }, ENV, '/w', fake(200, undefined, { ok: true }))).code).toBe(0); + }); + it('treats a disabled bridge (409) as success that says so', async () => { const r = await runNotify({ message: 'x' }, ENV, '/work', fake(409)); expect(r.code).toBe(0); diff --git a/test/discord-bridge.test.ts b/test/discord-bridge.test.ts index c9a577c9..43192995 100644 --- a/test/discord-bridge.test.ts +++ b/test/discord-bridge.test.ts @@ -477,6 +477,50 @@ describe('DiscordBridge', () => { }); }); + it('starts afresh when the parent channel changes: tabs get a thread in the new channel', async () => { + const b = bridge(); + await b.notify(MSG); + expect(await readThreadMap(dir)).toEqual({ s1: THREAD }); + posts.length = 0; + await writeDiscordConfig(dir, { ...CFG, parentChannelId: '777777777777777777' }); + await b.reload(); + await b.notify({ ...MSG, event: 'hook:idle_prompt' }); + expect(posts.find((p) => p.path.endsWith('/threads'))?.path).toBe('/channels/777777777777777777/threads'); + b.stop(); + }); + + it('keeps the threads across a restart under the same parent channel', async () => { + const first = bridge(); + await first.notify(MSG); + first.stop(); + posts.length = 0; + const second = bridge(); + await second.reload(); + await second.notify({ ...MSG, event: 'hook:idle_prompt' }); + expect(posts.some((p) => p.path.endsWith('/threads'))).toBe(false); + second.stop(); + }); + + it('a failed thread creation reports its own failure even if another post succeeds meanwhile', async () => { + let n = 0; + const b = bridge({}, async (url) => { + if (String(url).endsWith('/threads')) { + await new Promise((r) => setTimeout(r, 20)); + return new Response('{}', { status: 403 }); + } + n++; + return jsonRes({ id: 'msg' }); + }); + const [failed, ok] = await Promise.all([ + b.notify(MSG), + b.notify({ ...MSG, sessionId: undefined, event: 'session:error' }), + ]); + expect(n).toBeGreaterThan(0); + expect(ok?.ok).toBe(true); + expect(failed).toMatchObject({ ok: false, status: 403 }); + b.stop(); + }); + describe('sendTest', () => { it('posts with the config it is given and leaves the live config alone', async () => { const auth: string[] = []; diff --git a/test/routes/discord-routes.test.ts b/test/routes/discord-routes.test.ts index 052ed50a..282104ac 100644 --- a/test/routes/discord-routes.test.ts +++ b/test/routes/discord-routes.test.ts @@ -175,6 +175,16 @@ describe('POST /api/sessions/:id/discord-notify', () => { bridge.stop(); }); + it('answers 200 with data.ok false when Discord refuses the post, which the CLI turns into exit 1', async () => { + const { app, bridge } = await harness(); + await app.inject({ method: 'PUT', url: '/api/discord', payload: FULL }); + fetchImpl.mockImplementation(async () => new Response(JSON.stringify({ code: 0 }), { status: 401 })); + const res = await app.inject({ method: 'POST', url, payload: { message: 'help' } }); + expect(res.statusCode).toBe(200); + expect(res.json().data).toMatchObject({ ok: false, status: 401 }); + bridge.stop(); + }); + it('answers 429 RATE_LIMITED once a tab posts more than its share in a minute', async () => { const { app, bridge } = await harness(); await app.inject({ method: 'PUT', url: '/api/discord', payload: FULL }); @@ -256,6 +266,13 @@ describe('discord-notify with a file', () => { bridge.stop(); }); + it('probes the file before realpath: a path that is not a regular file is refused', async () => { + const { app, bridge } = await ready(); + mkdirSync(join(work, 'dir')); + expect((await post(app, join(work, 'dir'))).json().error).toMatch(/Not a regular file/); + bridge.stop(); + }); + it('refuses a symlink that points outside the folder', async () => { const { app, bridge } = await ready(); mkdirSync(join(work, 'sub'));