diff --git a/specs/api/agent-inbox-v1.md b/specs/api/agent-inbox-v1.md index 3efdb9e..3366dd7 100644 --- a/specs/api/agent-inbox-v1.md +++ b/specs/api/agent-inbox-v1.md @@ -69,7 +69,12 @@ interface ThreadView { from: string // the message's From address; the support address for notes bodyText: string | null bodyHtml: string | null // ⚠ UNTRUSTED, UNSANITIZED — see §5 - deliveryStatus: 'pending' | 'sent' | 'failed' | null // outbound only; null otherwise + deliveryStatus: 'pending' | 'sent' | 'failed' | null + // outbound only; null otherwise. HT-70: the invariant widens + // — an outbound thread's deliveryStatus is ALSO null while it + // is an unapproved or discarded draft (draftStatus below is + // 'awaiting_review' or 'discarded'); a draft becomes eligible + // for pending/sent/failed only once approved. customerViewedAt: string | null // v1.1: outbound only, and only when open tracking is // enabled (§4g) — first time the customer viewed the reply; @@ -80,6 +85,14 @@ interface ThreadView { // attachment read-path deps (config-gated, absent by default // — same posture as open tracking, §4g) createdAt: string // ISO-8601 + authorKind: 'customer' | 'agent' | 'assistant' + // HT-70 (specs/plugins/substrate-v1.md §2, §7): who authored + // this thread — 'customer' for inbound mail, 'agent' for + // human-authored outbound/notes, 'assistant' for an + // AI-authored draft (specs/plugins/substrate-v1.md §3, §6) + draftStatus: 'awaiting_review' | 'approved' | 'discarded' | null + // HT-70: a draft's lifecycle state; null for every non-draft + // thread (specs/plugins/substrate-v1.md §2, §6) } interface AttachmentView { @@ -108,7 +121,13 @@ an identifier anywhere in this API. **`preview`** is derived at read time, not stored: the most recent thread with a non-null `bodyText` (any direction — notes included; this is an Agent-only surface), whitespace collapsed to single spaces, trimmed, first 120 characters; `''` when no -thread has text. +thread has text. **HT-70:** `preview` and `threadCount` both IGNORE an unresolved or +discarded draft (`draftStatus IN ('awaiting_review', 'discarded')`) — a draft is not +conversation content until an Agent approves it, so it contributes to neither the +count nor the latest-body derivation. An `'approved'` draft (i.e. sent mail) counts and +can become the preview like any other outbound thread. Conversation detail (§3b) still +returns the draft ROW itself in `threads` regardless of its status — only the +summary-level `preview`/`threadCount` derivations exclude it. Ids are **UUID strings**, verbatim as the store generates them — the uuid is canonical and `number` is a human-facing convenience, not a surrogate key. There is no `customer` @@ -127,10 +146,17 @@ added when a real need appears, not preemptively. wrong token is `401 unauthorized` with a generic message — the response never reveals which of those it was. (The open-tracking pixel, §4g, is the one deliberate exception to Bearer auth — it is fetched by customer mail clients and carries its own rules.) - **This is still the API's only auth model (HT-51, §5).** The Agent Inbox web app now - requires an operator to sign in before it will render any page, but that is a web-layer - door in front of this same Bearer token, not a second API auth mechanism — see §5 for - the full justification. + **This is still the API's only auth model — with one addition (HT-70).** The Agent + Inbox web app now requires an operator to sign in before it will render any page, but + that is a web-layer door in front of this same Bearer token, not a second API auth + mechanism — see §5 for the full justification. HT-70 (specs/plugins/substrate-v1.md + §3) DOES add a genuine second credential class, checked ALONGSIDE the service Bearer + token, never replacing it: a per-Assistant token (`ht_asst__`), + verified before routing under the same constant-time discipline (parse the embedded + id → single-row lookup → constant-time digest compare). An Assistant's capability set + is fixed and narrow (read conversations, create drafts, create notes — spec §3) and + enforced at one gate, distinct from every Agent-facing endpoint this document + describes. - **Never cache:** every response carries `Cache-Control: no-store`. This is authenticated support data; no edge or CDN copy, ever. - **Error envelope:** @@ -179,6 +205,14 @@ Returns a `ConversationDetail` — the conversation plus its `threads`, oldest-f not_found` if `{id}` is not a conversation (or is a `deleted` one — a deleted conversation is indistinguishable from a nonexistent one to this API, on purpose). +**HT-70:** `threads` includes draft rows (`draftStatus` non-null) for Agent/service +callers, at every lifecycle stage — the timeline shows an `awaiting_review`/`discarded` +draft alongside real mail, distinguishable by `authorKind: 'assistant'` and +`draftStatus`. An Assistant caller reads the same endpoint and sees its own drafts +through it too (no separate read surface). Only the summary-level `preview`/ +`threadCount` derivations exclude an unresolved/discarded draft (§2) — the full +`threads` array is never filtered by draft status. + ## 4. Write paths ### 4a. `POST /api/v1/conversations/{id}/replies` — the Agent replies @@ -424,6 +458,15 @@ above. ## 7. Changelog +- **v1.1 (HT-70).** Wire-contract amendments from specs/plugins/substrate-v1.md §7 + (drafts kept in `threads` rather than a separate table): `ThreadView` gains + `authorKind` and `draftStatus` (§2); the `deliveryStatus` invariant widens (outbound + stays `null` while a draft is unapproved or discarded, §2); `preview`/`threadCount` + ignore an unresolved or discarded draft (§2); conversation detail (§3b) still returns + every draft row regardless of status; and §3's auth-model statement is amended — a + second, per-Assistant credential class now authenticates alongside the service Bearer + token, for the fixed, narrow Assistant capability set specs/plugins/substrate-v1.md §3 + defines. - **v1.1 (2026-07-17, HT-51).** Documented the Agent Inbox web app's new operator login (§3, §5) — a session cookie the UI now requires before rendering any page. No API behavior changed: this is a web-layer addition in front of the unchanged diff --git a/src/api/agents.test.ts b/src/api/agents.test.ts index 35bba15..58252df 100644 --- a/src/api/agents.test.ts +++ b/src/api/agents.test.ts @@ -18,6 +18,7 @@ import { migrate } from '../db/migrate.js' import type { Keyring } from '../mail/reply-token.js' import type { EmailSender, OutboundEmail } from '../providers/index.js' import { type AgentRecord, type AgentStore, createAgentStore } from '../store/agents.js' +import { createAssistantStore } from '../store/assistants.js' import { createConversationStore } from '../store/conversations.js' import { createMailboxStore, type MailboxStore } from '../store/mailboxes.js' import { ENCRYPTION_KEY_BYTES } from '../store/token-crypto.js' @@ -96,6 +97,7 @@ describe('Agents & Authentication API', () => { store: createWebhookEndpointStore(db, WEBHOOKS_ENC_KEY), queue: { async enqueue() {} }, }, + assistants: { store: createAssistantStore(db) }, }) return { db, agentStore, mailboxStore, api, sent } } diff --git a/src/api/assistant-auth.test.ts b/src/api/assistant-auth.test.ts new file mode 100644 index 0000000..4b4d8f7 --- /dev/null +++ b/src/api/assistant-auth.test.ts @@ -0,0 +1,79 @@ +import { randomUUID } from 'node:crypto' +import { afterEach, describe, expect, it } from 'vitest' +import { mintAssistantToken } from '../auth/assistant-token.js' +import { createPgliteDb, type Db } from '../db/client.js' +import { migrate } from '../db/migrate.js' +import { createAssistantStore } from '../store/assistants.js' +import { authenticateAssistantRequest } from './assistant-auth.js' + +function req(authorization?: string): Request { + const headers: Record = {} + if (authorization !== undefined) headers.authorization = authorization + return new Request('https://x.example.test/api/v1/conversations', { headers }) +} + +describe('authenticateAssistantRequest', () => { + let db: Db | undefined + + afterEach(async () => { + await db?.close() + db = undefined + }) + + async function freshStoreWithAssistant(status: 'active' | 'disabled' = 'active') { + db = await createPgliteDb() + await migrate(db) + const store = createAssistantStore(db) + const id = randomUUID() + const minted = mintAssistantToken(id) + const assistant = await store.create({ + id, + name: 'Draft Bot', + module: 'draft-reply', + tokenHash: minted.tokenHash, + }) + if (status === 'disabled') { + await store.patch(id, { status: 'disabled' }) + } + return { store, assistant, token: minted.token } + } + + it('resolves the Assistant for a valid token', async () => { + const { store, assistant, token } = await freshStoreWithAssistant() + const resolved = await authenticateAssistantRequest(req(`Bearer ${token}`), store) + expect(resolved?.id).toBe(assistant.id) + }) + + it('returns null for a missing Authorization header', async () => { + const { store } = await freshStoreWithAssistant() + expect(await authenticateAssistantRequest(req(), store)).toBeNull() + }) + + it('returns null for a non-Bearer scheme', async () => { + const { store, token } = await freshStoreWithAssistant() + expect(await authenticateAssistantRequest(req(`Basic ${token}`), store)).toBeNull() + }) + + it('returns null for a token with the wrong secret (same assistantId)', async () => { + const { store, assistant } = await freshStoreWithAssistant() + const forged = `ht_asst_${assistant.id}_wrong-secret-value` + expect(await authenticateAssistantRequest(req(`Bearer ${forged}`), store)).toBeNull() + }) + + it('returns null for an unknown assistantId', async () => { + const { store } = await freshStoreWithAssistant() + const unknownId = '22222222-2222-4222-8222-222222222222' + const forged = `ht_asst_${unknownId}_some-secret` + expect(await authenticateAssistantRequest(req(`Bearer ${forged}`), store)).toBeNull() + }) + + it('returns null for a disabled Assistant, even with the correct secret', async () => { + const { store, token } = await freshStoreWithAssistant('disabled') + expect(await authenticateAssistantRequest(req(`Bearer ${token}`), store)).toBeNull() + }) + + it('returns null for a malformed token (not our shape)', async () => { + const { store } = await freshStoreWithAssistant() + expect(await authenticateAssistantRequest(req('Bearer not-our-token-shape'), store)).toBeNull() + }) +}) diff --git a/src/api/assistant-auth.ts b/src/api/assistant-auth.ts new file mode 100644 index 0000000..302a675 --- /dev/null +++ b/src/api/assistant-auth.ts @@ -0,0 +1,55 @@ +/** + * Assistant bearer-token request authentication (HT-70; specs/plugins/ + * substrate-v1.md §3, amending agent-inbox-v1.md §3/§7) — the SECOND + * credential class alongside the service Bearer token (`src/api/auth.ts`), + * checked ALONGSIDE it, never replacing it: `src/api/index.ts`'s pipeline + * tries the service token first, and only on a miss tries this. + * + * Verification sequence, exactly as spec §3 states it: parse the embedded + * assistantId out of the presented token → single-row lookup (no hash + * scan) → constant-time digest compare — before routing, so an Assistant's + * identity is resolved (or rejected) the same place/time the service token + * is. + */ + +import { + constantTimeHashEquals, + hashAssistantSecret, + parseAssistantToken, +} from '../auth/assistant-token.js' +import type { AssistantRecord, AssistantStore } from '../store/assistants.js' + +const BEARER_PREFIX = 'Bearer ' + +/** + * Resolve `request`'s Assistant, or `null` for anything that isn't a valid, + * active Assistant's token: a missing/malformed `Authorization` header, a + * value not shaped like `ht_asst__`, an unknown assistantId, a + * `disabled` Assistant, or a secret whose digest doesn't match the stored + * hash. Every rejection reason collapses to the same `null` — the caller + * (`src/api/index.ts`) maps it to the SAME generic `401` the service-token + * miss gets, never a more specific message that would distinguish "unknown + * id" from "wrong secret" from "disabled." Never throws. + */ +export async function authenticateAssistantRequest( + request: Request, + store: AssistantStore, +): Promise { + const header = request.headers.get('authorization') + if (header === null || !header.startsWith(BEARER_PREFIX)) return null + const token = header.slice(BEARER_PREFIX.length) + + const parsed = parseAssistantToken(token) + if (parsed === null) return null + + // One-snapshot read (CodeRabbit #80): status and token_hash come from the + // SAME row read, so a disable or rotation can never be interleaved between + // separate status/hash queries and validate stale credentials. + const auth = await store.getForAuth(parsed.assistantId) + if (auth === null || auth.record.status !== 'active') return null + + const providedHash = hashAssistantSecret(parsed.secret) + if (!constantTimeHashEquals(providedHash, auth.tokenHash)) return null + + return auth.record +} diff --git a/src/api/assistants.test.ts b/src/api/assistants.test.ts new file mode 100644 index 0000000..9d99d89 --- /dev/null +++ b/src/api/assistants.test.ts @@ -0,0 +1,327 @@ +/** + * End-to-end tests for the Assistants admin API (HT-70; + * specs/plugins/substrate-v1.md §3) — driven through the real + * `createInboxApi` pipeline (`src/api/index.ts`), matching this codebase's + * convention of testing API handlers via the full HTTP pipeline + * (`src/api/agents.test.ts`, `src/api/index.test.ts`) rather than calling + * handler functions directly. + */ + +import { afterEach, describe, expect, it } from 'vitest' +import { hashAssistantSecret, parseAssistantToken } from '../auth/assistant-token.js' +import { createPasswordAuthProvider } from '../auth/password-provider.js' +import { createPgliteDb, type Db } from '../db/client.js' +import { migrate } from '../db/migrate.js' +import type { Keyring } from '../mail/reply-token.js' +import type { EmailSender, OutboundEmail } from '../providers/index.js' +import { type AgentRecord, type AgentStore, createAgentStore } from '../store/agents.js' +import { type AssistantStore, createAssistantStore } from '../store/assistants.js' +import { createConversationStore } from '../store/conversations.js' +import { createMailboxStore } from '../store/mailboxes.js' +import { createInboxApi } from './index.js' +import type { WebhooksApiDeps } from './webhooks.js' + +const TOKEN = 'test-token-for-the-assistants-suite' +const MAIL_DOMAIN = 'mail.example.test' +const SUPPORT_ADDRESS = 'support@example.test' +const KEYRING: Keyring = { current: { keyId: 'k1', secret: 'a'.repeat(32) } } +const AGENT_HEADER = 'X-Helpthread-Agent-Id' + +function createFakeSender(): { sender: EmailSender; sent: OutboundEmail[] } { + const sent: OutboundEmail[] = [] + return { + sender: { + maxSendMs: 30_000, + async send(email) { + sent.push(email) + return {} + }, + }, + sent, + } +} + +describe('Assistants admin API (HT-70)', () => { + let db: Db | undefined + + afterEach(async () => { + await db?.close() + db = undefined + }) + + async function freshApi(): Promise<{ + db: Db + agentStore: AgentStore + assistantStore: AssistantStore + api: (request: Request) => Promise + }> { + db = await createPgliteDb() + await migrate(db) + const agentStore = createAgentStore(db) + const assistantStore = createAssistantStore(db) + const { sender } = createFakeSender() + const api = createInboxApi({ + store: createConversationStore(db), + apiToken: TOKEN, + sender, + keyring: KEYRING, + mailDomain: MAIL_DOMAIN, + supportAddress: SUPPORT_ADDRESS, + agents: { + store: agentStore, + providers: [createPasswordAuthProvider({ agentStore })], + mailboxStore: createMailboxStore(db), + }, + assistants: { store: assistantStore }, + webhooks: { + store: {} as unknown as WebhooksApiDeps['store'], + queue: { async enqueue() {} }, + } satisfies WebhooksApiDeps, + }) + return { db, agentStore, assistantStore, api } + } + + function req( + method: string, + path: string, + opts: { agentId?: string; body?: unknown } = {}, + ): Request { + const headers: Record = { Authorization: `Bearer ${TOKEN}` } + if (opts.agentId !== undefined) headers[AGENT_HEADER] = opts.agentId + const init: RequestInit = { method, headers } + if (opts.body !== undefined) { + headers['Content-Type'] = 'application/json' + init.body = JSON.stringify(opts.body) + } + return new Request(`https://x.example.test${path}`, init) + } + + async function createActiveAgent( + agentStore: AgentStore, + overrides: { email?: string; role?: 'admin' | 'agent' } = {}, + ): Promise { + const result = await agentStore.createAgent({ + name: 'Test Agent', + email: overrides.email ?? 'agent@example.test', + role: overrides.role ?? 'admin', + status: 'active', + passwordHash: 'scrypt$N=16384,r=8,p=1$c2FsdA$aGFzaA', + }) + if (!result.ok) throw new Error('expected ok') + return result.agent + } + + describe('POST /api/v1/assistants', () => { + it('admin creates an Assistant and gets the token exactly once, shaped ht_asst__', async () => { + const { api, agentStore } = await freshApi() + const admin = await createActiveAgent(agentStore) + + const res = await api( + req('POST', '/api/v1/assistants', { + agentId: admin.id, + body: { name: 'Draft Bot', module: 'draft-reply' }, + }), + ) + expect(res.status).toBe(201) + const body = (await res.json()) as { + assistant: { id: string; name: string; module: string; status: string } + token: string + } + expect(body.assistant.name).toBe('Draft Bot') + expect(body.assistant.module).toBe('draft-reply') + expect(body.assistant.status).toBe('active') + expect(body.assistant).not.toHaveProperty('tokenHash') + + const parsed = parseAssistantToken(body.token) + expect(parsed?.assistantId).toBe(body.assistant.id) + }) + + it('403s for a non-admin Agent', async () => { + const { api, agentStore } = await freshApi() + const agent = await createActiveAgent(agentStore, { role: 'agent', email: 'a@example.test' }) + const res = await api( + req('POST', '/api/v1/assistants', { + agentId: agent.id, + body: { name: 'Draft Bot', module: 'draft-reply' }, + }), + ) + expect(res.status).toBe(403) + }) + + it('401s with no acting-Agent header', async () => { + const { api } = await freshApi() + const res = await api( + req('POST', '/api/v1/assistants', { body: { name: 'Draft Bot', module: 'draft-reply' } }), + ) + expect(res.status).toBe(401) + }) + + it('400s on a missing/invalid name or module', async () => { + const { api, agentStore } = await freshApi() + const admin = await createActiveAgent(agentStore) + for (const body of [{}, { name: '', module: 'm' }, { name: 'Bot', module: '' }]) { + const res = await api(req('POST', '/api/v1/assistants', { agentId: admin.id, body })) + expect(res.status).toBe(400) + } + }) + }) + + describe('GET /api/v1/assistants', () => { + it('admin lists every Assistant, never leaking a tokenHash', async () => { + const { api, agentStore, assistantStore } = await freshApi() + const admin = await createActiveAgent(agentStore) + await assistantStore.create({ name: 'Bot A', module: 'm', tokenHash: 'h1' }) + await assistantStore.create({ name: 'Bot B', module: 'm', tokenHash: 'h2' }) + + const res = await api(req('GET', '/api/v1/assistants', { agentId: admin.id })) + expect(res.status).toBe(200) + const body = (await res.json()) as { assistants: Array> } + expect(body.assistants).toHaveLength(2) + for (const a of body.assistants) { + expect(a).not.toHaveProperty('tokenHash') + } + }) + + it('403s for a non-admin Agent (unlike GET /agents, which any active Agent may read)', async () => { + const { api, agentStore } = await freshApi() + const agent = await createActiveAgent(agentStore, { role: 'agent', email: 'a@example.test' }) + const res = await api(req('GET', '/api/v1/assistants', { agentId: agent.id })) + expect(res.status).toBe(403) + }) + }) + + describe('PATCH /api/v1/assistants/{id}', () => { + it('admin updates name and status', async () => { + const { api, agentStore, assistantStore } = await freshApi() + const admin = await createActiveAgent(agentStore) + const assistant = await assistantStore.create({ name: 'Bot', module: 'm', tokenHash: 'h' }) + + const res = await api( + req('PATCH', `/api/v1/assistants/${assistant.id}`, { + agentId: admin.id, + body: { name: 'Renamed Bot', status: 'disabled' }, + }), + ) + expect(res.status).toBe(200) + const body = (await res.json()) as { assistant: { name: string; status: string } } + expect(body.assistant.name).toBe('Renamed Bot') + expect(body.assistant.status).toBe('disabled') + }) + + it('404s for an unknown id', async () => { + const { api, agentStore } = await freshApi() + const admin = await createActiveAgent(agentStore) + const res = await api( + req('PATCH', '/api/v1/assistants/00000000-0000-4000-8000-000000000000', { + agentId: admin.id, + body: { name: 'X' }, + }), + ) + expect(res.status).toBe(404) + }) + + it('400s on an unknown field', async () => { + const { api, agentStore, assistantStore } = await freshApi() + const admin = await createActiveAgent(agentStore) + const assistant = await assistantStore.create({ name: 'Bot', module: 'm', tokenHash: 'h' }) + const res = await api( + req('PATCH', `/api/v1/assistants/${assistant.id}`, { + agentId: admin.id, + body: { module: 'not-settable' }, + }), + ) + expect(res.status).toBe(400) + }) + }) + + describe('POST /api/v1/assistants/{id}/rotate-token', () => { + it('mints a fresh token for the SAME assistant id; the old token stops verifying', async () => { + const { api, agentStore, assistantStore, db: freshDb } = await freshApi() + const admin = await createActiveAgent(agentStore) + const created = await api( + req('POST', '/api/v1/assistants', { + agentId: admin.id, + body: { name: 'Bot', module: 'm' }, + }), + ) + const { assistant, token: oldToken } = (await created.json()) as { + assistant: { id: string } + token: string + } + + const res = await api( + req('POST', `/api/v1/assistants/${assistant.id}/rotate-token`, { agentId: admin.id }), + ) + expect(res.status).toBe(200) + const body = (await res.json()) as { assistant: { id: string }; token: string } + expect(body.assistant.id).toBe(assistant.id) + expect(body.token).not.toBe(oldToken) + + const oldParsed = parseAssistantToken(oldToken) + const storedHash = (await assistantStore.getForAuth(assistant.id))?.tokenHash ?? null + expect(hashAssistantSecret(oldParsed?.secret ?? '')).not.toBe(storedHash) + + const newParsed = parseAssistantToken(body.token) + expect(hashAssistantSecret(newParsed?.secret ?? '')).toBe(storedHash) + }) + + it('404s for an unknown id', async () => { + const { api, agentStore } = await freshApi() + const admin = await createActiveAgent(agentStore) + const res = await api( + req('POST', '/api/v1/assistants/00000000-0000-4000-8000-000000000000/rotate-token', { + agentId: admin.id, + }), + ) + expect(res.status).toBe(404) + }) + }) + describe('assistant auth failure containment (CodeRabbit #80)', () => { + it('a store failure during assistant auth returns the controlled 500 envelope, not an uncontrolled throw', async () => { + const { api, agentStore, assistantStore, db: freshDb } = await freshApi() + const admin = await createActiveAgent(agentStore) + const created = await api( + req('POST', '/api/v1/assistants', { + agentId: admin.id, + body: { name: 'Draft Bot', module: 'draft-reply' }, + }), + ) + const { token } = (await created.json()) as { token: string } + + const failingStore = { + ...assistantStore, + async getForAuth(): Promise { + throw new Error('database exploded') + }, + } + const failingApi = createInboxApi({ + store: createConversationStore(freshDb), + apiToken: TOKEN, + sender: createFakeSender().sender, + keyring: KEYRING, + mailDomain: MAIL_DOMAIN, + supportAddress: SUPPORT_ADDRESS, + agents: { + store: agentStore, + providers: [createPasswordAuthProvider({ agentStore })], + mailboxStore: createMailboxStore(freshDb), + }, + assistants: { store: failingStore }, + webhooks: { + store: {} as unknown as WebhooksApiDeps['store'], + queue: { async enqueue() {} }, + } satisfies WebhooksApiDeps, + }) + + const res = await failingApi( + new Request('https://x.example.test/api/v1/conversations', { + headers: { authorization: `Bearer ${token}` }, + }), + ) + expect(res.status).toBe(500) + expect(res.headers.get('cache-control')).toBe('no-store') + const body = (await res.json()) as { error: { code: string } } + expect(body.error.code).toBe('server_error') + }) + }) +}) diff --git a/src/api/assistants.ts b/src/api/assistants.ts new file mode 100644 index 0000000..95efc42 --- /dev/null +++ b/src/api/assistants.ts @@ -0,0 +1,216 @@ +/** + * The Assistants admin API (HT-70; specs/plugins/substrate-v1.md §3): + * `POST /api/v1/assistants` (returns the token ONCE), `GET /api/v1/assistants`, + * `PATCH /api/v1/assistants/{id}` (name, status), + * `POST /api/v1/assistants/{id}/rotate-token`. + * + * Same conventions as `src/api/agents.ts`: each handler is a pure function + * of an already-authenticated, already-routed `Request` plus its + * dependencies; `src/api/index.ts` resolves the acting Agent + * (`resolveActingAgent`) and passes the result in — `null` means "no acting + * Agent" and every handler here maps that to a generic `401`. Every + * mutation is ADMIN-ONLY (no self-service carve-out — an Assistant is not + * a human who can act on its own profile). `AssistantRecord` never carries + * `tokenHash` (see `src/store/assistants.ts`'s module doc); `toAssistantJson` + * is the one place this module decides what crosses the wire, and the + * plaintext token is returned ONLY from the two mint-time endpoints + * (create, rotate-token), never persisted, never logged, never returned + * again after that single response. + */ + +import { randomUUID } from 'node:crypto' +import { mintAssistantToken } from '../auth/assistant-token.js' +import type { AgentRecord } from '../store/agents.js' +import type { AssistantRecord, AssistantStatus, AssistantStore } from '../store/assistants.js' +import { apiError, json } from './responses.js' +import { isUuid } from './uuid.js' + +/** Dependencies every handler in this module needs. */ +export interface AssistantsApiDeps { + store: AssistantStore +} + +// --- validation -------------------------------------------------------------- + +const MIN_NAME_LENGTH = 1 +const MAX_NAME_LENGTH = 200 +const MIN_MODULE_LENGTH = 1 +const MAX_MODULE_LENGTH = 100 + +/** Trim; 1-200 chars. `null` on any violation. Same rule `src/api/agents.ts`'s `validateName` uses for an Agent's name. */ +function validateName(raw: unknown): string | null { + if (typeof raw !== 'string') return null + const trimmed = raw.trim() + return trimmed.length >= MIN_NAME_LENGTH && trimmed.length <= MAX_NAME_LENGTH ? trimmed : null +} + +/** Trim; 1-100 chars — the module slug is free text in v1 (no registry exists yet to validate it against; spec §1's additive-forward rule). `null` on any violation. */ +function validateModule(raw: unknown): string | null { + if (typeof raw !== 'string') return null + const trimmed = raw.trim() + return trimmed.length >= MIN_MODULE_LENGTH && trimmed.length <= MAX_MODULE_LENGTH ? trimmed : null +} + +/** Read and JSON-parse `request`'s body without ever throwing — mirrors `src/api/agents.ts`'s helper of the same name (kept local per this codebase's per-file convention). */ +async function parseJsonBody( + request: Request, +): Promise<{ ok: true; value: unknown } | { ok: false }> { + try { + return { ok: true, value: await request.json() } + } catch { + return { ok: false } + } +} + +/** `typeof value === 'object' && value !== null`, narrowed to a plain record. */ +function asRecord(value: unknown): Record | null { + return typeof value === 'object' && value !== null ? (value as Record) : null +} + +// --- wire shape ---------------------------------------------------------- + +interface AssistantJson { + id: string + name: string + module: string + status: AssistantStatus + createdByAgentId: string | null + createdAt: string + updatedAt: string +} + +function toAssistantJson(assistant: AssistantRecord): AssistantJson { + return { + id: assistant.id, + name: assistant.name, + module: assistant.module, + status: assistant.status, + createdByAgentId: assistant.createdByAgentId, + createdAt: assistant.createdAt.toISOString(), + updatedAt: assistant.updatedAt.toISOString(), + } +} + +const UNAUTHORIZED = () => apiError(401, 'unauthorized', 'Missing or invalid Agent identity.') +const NOT_FOUND = () => apiError(404, 'not_found', 'No Assistant with that id.') +const ADMIN_REQUIRED = () => apiError(403, 'forbidden', 'Admin role required.') + +// --- POST /api/v1/assistants -------------------------------------------- + +/** + * `POST /api/v1/assistants` (spec §3) — admin only. Mints a fresh token via + * the id/token knot (`src/auth/assistant-token.ts`'s module doc: generate + * the id first, mint the token against it, then insert with that id + * explicit) and returns the full token in the response body — the ONLY + * time it is ever visible again. + */ +export async function handleCreateAssistant( + actingAgent: AgentRecord | null, + request: Request, + deps: AssistantsApiDeps, +): Promise { + if (actingAgent === null) return UNAUTHORIZED() + if (actingAgent.role !== 'admin') return ADMIN_REQUIRED() + + const parsed = await parseJsonBody(request) + if (!parsed.ok) return apiError(400, 'validation_failed', 'Request body must be valid JSON.') + const body = asRecord(parsed.value) + if (body === null) + return apiError(400, 'validation_failed', 'Request body must be a JSON object.') + + const name = validateName(body.name) + const moduleSlug = validateModule(body.module) + if (name === null || moduleSlug === null) { + return apiError(400, 'validation_failed', 'name and module are required and must be valid.') + } + + const assistantId = randomUUID() + const minted = mintAssistantToken(assistantId) + const assistant = await deps.store.create({ + id: assistantId, + name, + module: moduleSlug, + tokenHash: minted.tokenHash, + createdByAgentId: actingAgent.id, + }) + + return json(201, { assistant: toAssistantJson(assistant), token: minted.token }) +} + +// --- GET /api/v1/assistants ----------------------------------------------- + +/** `GET /api/v1/assistants` (spec §3) — admin only, unlike `GET /api/v1/agents` (which any active Agent may read): an Assistant's token-issuance surface is admin bookkeeping, not something every Agent's UI needs. */ +export async function handleListAssistants( + actingAgent: AgentRecord | null, + deps: AssistantsApiDeps, +): Promise { + if (actingAgent === null) return UNAUTHORIZED() + if (actingAgent.role !== 'admin') return ADMIN_REQUIRED() + + const assistants = await deps.store.list() + return json(200, { assistants: assistants.map(toAssistantJson) }) +} + +// --- PATCH /api/v1/assistants/{id} ---------------------------------------- + +/** `PATCH /api/v1/assistants/{id}` (spec §3) — admin only. Body: `name` and/or `status` (`'active'|'disabled'`); any other field is `400`. */ +export async function handlePatchAssistant( + id: string, + actingAgent: AgentRecord | null, + request: Request, + deps: AssistantsApiDeps, +): Promise { + if (actingAgent === null) return UNAUTHORIZED() + if (actingAgent.role !== 'admin') return ADMIN_REQUIRED() + if (!isUuid(id)) return NOT_FOUND() + + const parsed = await parseJsonBody(request) + if (!parsed.ok) return apiError(400, 'validation_failed', 'Request body must be valid JSON.') + const body = asRecord(parsed.value) + if (body === null) + return apiError(400, 'validation_failed', 'Request body must be a JSON object.') + + for (const key of Object.keys(body)) { + if (key !== 'name' && key !== 'status') { + return apiError(400, 'validation_failed', `Unknown field '${key}'.`) + } + } + + const patch: { name?: string; status?: AssistantStatus } = {} + if ('name' in body) { + const name = validateName(body.name) + if (name === null) return apiError(400, 'validation_failed', 'name must be 1-200 characters.') + patch.name = name + } + if ('status' in body) { + if (body.status !== 'active' && body.status !== 'disabled') { + return apiError(400, 'validation_failed', "status must be 'active' or 'disabled'.") + } + patch.status = body.status + } + + const updated = await deps.store.patch(id, patch) + if (updated === null) return NOT_FOUND() + return json(200, { assistant: toAssistantJson(updated) }) +} + +// --- POST /api/v1/assistants/{id}/rotate-token ----------------------------- + +/** `POST /api/v1/assistants/{id}/rotate-token` (spec §3) — admin only. Mints a fresh secret for the SAME assistant id (the id, and therefore every past `author_assistant_id` FK, never changes) and returns the new token ONCE; the old token stops verifying immediately (its hash is overwritten, not retained). */ +export async function handleRotateAssistantToken( + id: string, + actingAgent: AgentRecord | null, + deps: AssistantsApiDeps, +): Promise { + if (actingAgent === null) return UNAUTHORIZED() + if (actingAgent.role !== 'admin') return ADMIN_REQUIRED() + if (!isUuid(id)) return NOT_FOUND() + + const existing = await deps.store.get(id) + if (existing === null) return NOT_FOUND() + + const minted = mintAssistantToken(id) + await deps.store.updateTokenHash(id, minted.tokenHash) + + return json(200, { assistant: toAssistantJson(existing), token: minted.token }) +} diff --git a/src/api/conversations.ts b/src/api/conversations.ts index 20291de..9b89f75 100644 --- a/src/api/conversations.ts +++ b/src/api/conversations.ts @@ -15,6 +15,7 @@ * remember it. */ +import { deriveReplyHeaders } from '../mail/reply-headers.js' import type { Keyring } from '../mail/reply-token.js' import { type SelfEchoGuardDeps, sendReply } from '../mail/send.js' import type { BlobStore, EmailSender } from '../providers/index.js' @@ -65,8 +66,8 @@ interface AttachmentViewJson { url: string } -/** The wire shape of one `ThreadView` (specs/api/agent-inbox-v1.md §2) — `StoredThread` with `Date` fields as ISO strings and `fromAddress` renamed to `from`. */ -interface ThreadViewJson { +/** The wire shape of one `ThreadView` (specs/api/agent-inbox-v1.md §2, amended HT-70 §7) — `StoredThread` with `Date` fields as ISO strings and `fromAddress` renamed to `from`. */ +export interface ThreadViewJson { id: string direction: 'inbound' | 'outbound' | 'note' from: string @@ -78,6 +79,10 @@ interface ThreadViewJson { /** HT-46: `[]` unless this thread has stored attachment references AND the deployment wired `attachments` deps (see {@link handleGetConversation}) — absent-by-default, like `openTracking`. */ attachments: AttachmentViewJson[] createdAt: string + /** HT-70 (agent-inbox-v1.md §7): who authored this thread — `'customer'` (inbound mail), `'agent'` (human), or `'assistant'` (an AI-authored draft). */ + authorKind: 'customer' | 'agent' | 'assistant' + /** HT-70 (agent-inbox-v1.md §7): a draft's lifecycle state, or `null` for every non-draft thread. */ + draftStatus: 'awaiting_review' | 'approved' | 'discarded' | null } /** The wire shape of one `ConversationSummary` (specs/api/agent-inbox-v1.md §2) — `Date` fields as ISO strings. */ @@ -243,7 +248,14 @@ export async function handleGetConversation( subject: conversation.subject, customerEmail: conversation.customerEmail, status: conversation.status, - threadCount: conversation.threads.length, + // HT-70 (spec §7): an unresolved or discarded draft is not conversation + // content until sent — excluded from the count here exactly as + // ConversationStore.listConversations' own subqueries exclude it from + // the list view's threadCount/preview. The FULL thread list below + // (`threads:`) still includes every draft row — Agent/service callers + // see them in the timeline (spec §7's last bullet); only the count and + // the derived preview ignore them. + threadCount: countResolvedThreads(conversation.threads), preview: previewFromThreads(conversation.threads), tags: conversation.tags, assigneeAgentId: conversation.assigneeAgentId, @@ -305,22 +317,36 @@ async function toAttachmentViewJson( } } +/** HT-70 (spec §7): true for a draft that is not yet conversation content — unresolved or discarded. An `'approved'` or non-draft (`null`) thread is real content. */ +function isUnresolvedOrDiscardedDraft(thread: StoredThread): boolean { + return thread.draftStatus === 'awaiting_review' || thread.draftStatus === 'discarded' +} + /** * Derive a detail response's `preview` from the threads it already carries — * the SAME rule the store applies for list summaries (`derivePreview`, spec * §2): the most recent thread with a non-null `bodyText`. Threads arrive - * oldest-first, so this walks from the end. + * oldest-first, so this walks from the end. HT-70 (spec §7): skips any + * unresolved or discarded draft — the same exclusion + * `ConversationStore.listConversations`' `LATEST_BODY_TEXT_SUBQUERY` + * applies at the store layer for the list view. */ function previewFromThreads(threads: StoredThread[]): string { for (let i = threads.length - 1; i >= 0; i--) { - const bodyText = threads[i].bodyText - if (bodyText !== null) { - return derivePreview(bodyText) + const thread = threads[i] + if (isUnresolvedOrDiscardedDraft(thread)) continue + if (thread.bodyText !== null) { + return derivePreview(thread.bodyText) } } return '' } +/** HT-70 (spec §7): the detail response's `threadCount`, excluding unresolved/discarded drafts — mirrors `ConversationStore.listConversations`' `THREAD_COUNT_SUBQUERY` exclusion at the store layer. */ +function countResolvedThreads(threads: StoredThread[]): number { + return threads.filter((thread) => !isUnresolvedOrDiscardedDraft(thread)).length +} + /** * Handle `POST /api/v1/conversations/{id}/replies` — the Agent replies to a * conversation (spec §4a). The client supplies only `{ text, html? }`; every @@ -373,6 +399,8 @@ export async function handleReply( supportAddress: string openTracking?: { publicBaseUrl: string } selfEchoGuard?: SelfEchoGuardDeps + /** HT-70 (spec §3's author-identity forward-carry): the acting Agent's id from `X-Helpthread-Agent-Id`, or `null` when absent/unknown — `src/api/index.ts` resolves this before dispatch. Never an error when absent (spec §9 decision 4). */ + authorAgentId?: string | null }, ): Promise { if (!isUuid(id)) { @@ -391,6 +419,24 @@ export async function handleReply( `Idempotency-Key must be at most ${MAX_IDEMPOTENCY_KEY_LENGTH} characters.`, ) } + // HT-70 review fix (Opus): a reply's idempotency key is stored RAW — unlike + // a draft's, which the engine itself prefixes (`ConversationStore.appendDraft` + // stores it as `` `draft:${key}` ``, src/store/conversations.ts). Without + // this check, a caller-supplied reply key literally spelled e.g. `draft:abc` + // would land in the SAME `(conversation_id, idempotency_key)` row a draft's + // engine-owned `draft:abc` key would use — the two sub-namespaces are + // disjoint only because BOTH halves hold: the engine never lets a draft key + // escape its `draft:` prefix, AND a reply key is refused if it tries to + // enter that prefix itself. Retro-prefixing reply keys instead was rejected + // (a stored-raw key in production would lose idempotency continuity for + // every reply already in flight). + if (idempotencyKey.startsWith('draft:')) { + return apiError( + 400, + 'validation_failed', + "Idempotency-Key must not start with the reserved prefix 'draft:'.", + ) + } const parsedBody = await parseJsonBody(request) if (!parsedBody.ok) { @@ -435,6 +481,7 @@ export async function handleReply( inReplyTo, references, idempotencyKey, + authorAgentId: deps.authorAgentId ?? null, }, { store: deps.store, @@ -555,16 +602,35 @@ export async function handleDeleteConversation( return noContent() } +/** + * Who is authoring a note (HT-70; specs/plugins/substrate-v1.md §3, §6) — + * an Agent (identity from the acting-agent header, possibly unknown) or an + * authenticated Assistant (identity from its token — spec §6 makes this + * endpoint "now legal for assistants"). `src/api/index.ts` builds this from + * whichever credential authenticated the request before dispatching here. + */ +export type NoteAuthor = + | { kind: 'agent'; agentId: string | null } + | { kind: 'assistant'; assistantId: string } + /** * Handle `POST /api/v1/conversations/{id}/notes` — append an internal note - * (spec §4c, v1.1). Body: `{ text: string }`, 1–5000 chars, plain text only - * in v1. A note is Agent-only context: it is NEVER emailed — this handler - * never touches `sendReply`, mints no token, creates no outbox row (the - * boundary spec §4c calls a bug if crossed; the tests assert the sender is - * never invoked). It bumps `updatedAt` (a note is activity) but never - * changes `status` — noting a closed conversation does not reopen it + * (spec §4c, v1.1; HT-70 spec §6 opens this endpoint to Assistants too). + * Body: `{ text: string }`, 1–5000 chars, plain text only in v1. A note is + * Agent/Assistant-only context: it is NEVER emailed — this handler never + * touches `sendReply`, mints no token, creates no outbox row (the boundary + * spec §4c calls a bug if crossed; the tests assert the sender is never + * invoked). It bumps `updatedAt` (a note is activity) but never changes + * `status` — noting a closed conversation does not reopen it * (`appendThread`'s note-aware policy). * + * HT-70 (spec §3): every caller's identity is now recorded — + * `deps.author.kind === 'assistant'` writes `author_kind: 'assistant'` + + * `author_assistant_id`; an Agent/service caller writes `author_agent_id` + * (possibly `null`, when no acting-agent header was presented — never an + * error, spec §9 decision 4). This is the ONE handler HT-70 makes start + * recording author identity for every caller — pre-HT-70 it recorded none. + * * Outcomes: `201` with the created `ThreadView` (`direction: 'note'`, * `from` = the support address, `deliveryStatus: null`); * `400 validation_failed` on a bad body; `404 not_found` for a missing or @@ -573,7 +639,7 @@ export async function handleDeleteConversation( export async function handlePostNote( id: string, request: Request, - deps: { store: ConversationStore; supportAddress: string }, + deps: { store: ConversationStore; supportAddress: string; author: NoteAuthor }, ): Promise { if (!isUuid(id)) { return apiError(404, 'not_found', 'No conversation with that id.') @@ -598,6 +664,9 @@ export async function handlePostNote( messageId: null, fromAddress: deps.supportAddress, bodyText: note.text, + ...(deps.author.kind === 'assistant' + ? { authorKind: 'assistant' as const, authorAssistantId: deps.author.assistantId } + : { authorAgentId: deps.author.agentId }), }) if (!result.ok) { // not-found and deleted are one generic 404 (spec §5's no-existence-leak). @@ -829,47 +898,6 @@ function parseAssigneeBody(raw: unknown): string | null | undefined { return typeof assigneeAgentId === 'string' ? assigneeAgentId : undefined } -/** - * Derive a reply's mail headers from the conversation being replied to - * (spec §4a): - * - * - `subject`: the conversation's subject, `Re: `-prefixed unless it already - * starts with `re:` (case-insensitive) — never double-prefixed. - * - `inReplyTo`: the `messageId` of the most-recent INBOUND thread that has - * one. Threads are stored oldest-first, so this walks from the end - * looking for the first (i.e. most recent) inbound thread with a - * non-null `messageId`. `undefined` if there is none (e.g. every inbound - * message arrived without a `Message-ID`). - * - `references`: every thread's `messageId`, in chronological order, that - * is non-null. `undefined` (the key omitted entirely, per spec §4a) when - * NO thread has one — never an empty array in that case. - */ -function deriveReplyHeaders(conversation: { subject: string; threads: StoredThread[] }): { - subject: string - inReplyTo: string | undefined - references: string[] | undefined -} { - const subject = /^re:/i.test(conversation.subject) - ? conversation.subject - : `Re: ${conversation.subject}` - - let inReplyTo: string | undefined - for (let i = conversation.threads.length - 1; i >= 0; i--) { - const thread = conversation.threads[i] - if (thread.direction === 'inbound' && thread.messageId !== null) { - inReplyTo = thread.messageId - break - } - } - - const referencesList = conversation.threads - .map((t) => t.messageId) - .filter((messageId): messageId is string => messageId !== null) - const references = referencesList.length > 0 ? referencesList : undefined - - return { subject, inReplyTo, references } -} - function toConversationSummaryJson(row: { id: string number: number @@ -904,8 +932,13 @@ function toConversationSummaryJson(row: { * thread this API just created (a reply or a note) cannot yet have any * (HT-46: attachments are inbound-only, and only `handleGetConversation`'s * deps carry the `ThreadAttachmentStore`/`BlobStore` needed to look them up). + * + * Exported (HT-70): `src/api/drafts.ts`'s handlers build the SAME + * `ThreadView` shape for a draft row (a draft IS a thread, per the + * substrate spec's "keeping it in `threads`" decision) — one mapper, not a + * second copy that could drift on the `authorKind`/`draftStatus` fields. */ -function toThreadViewJson( +export function toThreadViewJson( thread: StoredThread, attachments: AttachmentViewJson[] = [], ): ThreadViewJson { @@ -920,5 +953,7 @@ function toThreadViewJson( thread.customerViewedAt === null ? null : thread.customerViewedAt.toISOString(), attachments, createdAt: thread.createdAt.toISOString(), + authorKind: thread.authorKind, + draftStatus: thread.draftStatus, } } diff --git a/src/api/cursor.ts b/src/api/cursor.ts index 3c952b5..2bbe522 100644 --- a/src/api/cursor.ts +++ b/src/api/cursor.ts @@ -18,7 +18,7 @@ * decode into `400 validation_failed`, not a crash (spec §3, §3a). */ -import type { ConversationListCursor } from '../store/conversations.js' +import type { ConversationListCursor, ListAwaitingDraftsCursor } from '../store/conversations.js' import { isUuid } from './uuid.js' /** The JSON shape actually encoded — short keys since it travels in a URL. `u` = updatedAt (ISO string), `i` = id. */ @@ -69,3 +69,48 @@ export function decodeCursor(value: string): ConversationListCursor | null { return { updatedAt, id: payload.i } } + +/** + * The `GET /api/v1/drafts` (HT-70) sibling of {@link encodeCursor}/ + * {@link decodeCursor} — same opaque base64url(JSON) shape and the same `u`/ + * `i` short keys, scoped to {@link ListAwaitingDraftsCursor}'s + * `(createdAt, id)` instead of a conversation's `(updatedAt, id)`. Kept as + * separate functions rather than a generic pair: the two cursor types are + * unrelated wire contracts that happen to share a shape today, and this + * codebase's convention (see `src/store/conversations.ts`'s draft-aware + * queries) is to accept that duplication rather than couple two independent + * endpoints through a shared abstraction. + */ +export function encodeDraftCursor(cursor: ListAwaitingDraftsCursor): string { + const payload: CursorPayload = { u: cursor.createdAt.toISOString(), i: cursor.id } + return Buffer.from(JSON.stringify(payload), 'utf8').toString('base64url') +} + +/** Decode a cursor string produced by {@link encodeDraftCursor}. Same totality contract as {@link decodeCursor} — never throws, `null` on anything malformed. */ +export function decodeDraftCursor(value: string): ListAwaitingDraftsCursor | null { + let json: unknown + try { + const decoded = Buffer.from(value, 'base64url').toString('utf8') + json = JSON.parse(decoded) + } catch { + return null + } + + if (typeof json !== 'object' || json === null) { + return null + } + const payload = json as Partial + if (typeof payload.u !== 'string' || typeof payload.i !== 'string') { + return null + } + if (!isUuid(payload.i)) { + return null + } + + const createdAt = new Date(payload.u) + if (Number.isNaN(createdAt.getTime())) { + return null + } + + return { createdAt, id: payload.i } +} diff --git a/src/api/drafts.test.ts b/src/api/drafts.test.ts new file mode 100644 index 0000000..00d0462 --- /dev/null +++ b/src/api/drafts.test.ts @@ -0,0 +1,921 @@ +/** + * End-to-end tests for the drafts API and the Assistant capability gate + * (HT-70; specs/plugins/substrate-v1.md §3, §6) — driven through the real + * `createInboxApi` pipeline (`src/api/index.ts`), matching this codebase's + * convention of testing API handlers via the full HTTP pipeline rather + * than calling handler functions directly. + */ + +import { randomUUID } from 'node:crypto' +import { afterEach, describe, expect, it } from 'vitest' +import { mintAssistantToken } from '../auth/assistant-token.js' +import { createPasswordAuthProvider } from '../auth/password-provider.js' +import { createPgliteDb, type Db } from '../db/client.js' +import { migrate } from '../db/migrate.js' +import type { Keyring } from '../mail/reply-token.js' +import type { EmailSender, OutboundEmail } from '../providers/index.js' +import { type AgentRecord, type AgentStore, createAgentStore } from '../store/agents.js' +import { + type AssistantRecord, + type AssistantStore, + createAssistantStore, +} from '../store/assistants.js' +import { type ConversationStore, createConversationStore } from '../store/conversations.js' +import { createMailboxStore } from '../store/mailboxes.js' +import { createInboxApi } from './index.js' +import type { WebhooksApiDeps } from './webhooks.js' + +const TOKEN = 'test-token-for-the-drafts-suite' +const MAIL_DOMAIN = 'mail.example.test' +const SUPPORT_ADDRESS = 'support@example.test' +const KEYRING: Keyring = { current: { keyId: 'k1', secret: 'a'.repeat(32) } } +const AGENT_HEADER = 'X-Helpthread-Agent-Id' + +function createFakeSender(): { sender: EmailSender; sent: OutboundEmail[] } { + const sent: OutboundEmail[] = [] + return { + sender: { + maxSendMs: 30_000, + async send(email) { + sent.push(email) + return {} + }, + }, + sent, + } +} + +function createThrowingSender(): EmailSender { + return { + maxSendMs: 30_000, + async send() { + throw new Error('provider rejected the message') + }, + } +} + +describe('Drafts API + Assistant capability gate (HT-70)', () => { + let db: Db | undefined + + afterEach(async () => { + await db?.close() + db = undefined + }) + + async function freshApi(overrides: { sender?: EmailSender } = {}): Promise<{ + db: Db + store: ConversationStore + agentStore: AgentStore + assistantStore: AssistantStore + api: (request: Request) => Promise + sent: OutboundEmail[] + }> { + db = await createPgliteDb() + await migrate(db) + const store = createConversationStore(db) + const agentStore = createAgentStore(db) + const assistantStore = createAssistantStore(db) + const { sender: defaultSender, sent } = createFakeSender() + const api = createInboxApi({ + store, + apiToken: TOKEN, + sender: overrides.sender ?? defaultSender, + keyring: KEYRING, + mailDomain: MAIL_DOMAIN, + supportAddress: SUPPORT_ADDRESS, + agents: { + store: agentStore, + providers: [createPasswordAuthProvider({ agentStore })], + mailboxStore: createMailboxStore(db), + }, + assistants: { store: assistantStore }, + webhooks: { + store: {} as unknown as WebhooksApiDeps['store'], + queue: { async enqueue() {} }, + } satisfies WebhooksApiDeps, + }) + return { db, store, agentStore, assistantStore, api, sent } + } + + /** A Bearer-authenticated request (service token), optionally with an acting-Agent header and/or JSON body. */ + function req( + method: string, + path: string, + opts: { agentId?: string; body?: unknown; idempotencyKey?: string } = {}, + ): Request { + const headers: Record = { Authorization: `Bearer ${TOKEN}` } + if (opts.agentId !== undefined) headers[AGENT_HEADER] = opts.agentId + if (opts.idempotencyKey !== undefined) headers['Idempotency-Key'] = opts.idempotencyKey + const init: RequestInit = { method, headers } + if (opts.body !== undefined) { + headers['Content-Type'] = 'application/json' + init.body = JSON.stringify(opts.body) + } + return new Request(`https://x.example.test${path}`, init) + } + + /** An Assistant-token-authenticated request. */ + function assistantReq( + method: string, + path: string, + token: string, + opts: { body?: unknown; idempotencyKey?: string } = {}, + ): Request { + const headers: Record = { Authorization: `Bearer ${token}` } + if (opts.idempotencyKey !== undefined) headers['Idempotency-Key'] = opts.idempotencyKey + const init: RequestInit = { method, headers } + if (opts.body !== undefined) { + headers['Content-Type'] = 'application/json' + init.body = JSON.stringify(opts.body) + } + return new Request(`https://x.example.test${path}`, init) + } + + async function createActiveAgent( + agentStore: AgentStore, + overrides: { email?: string; role?: 'admin' | 'agent' } = {}, + ): Promise { + const result = await agentStore.createAgent({ + name: 'Test Agent', + email: overrides.email ?? 'agent@example.test', + role: overrides.role ?? 'agent', + status: 'active', + passwordHash: 'scrypt$N=16384,r=8,p=1$c2FsdA$aGFzaA', + }) + if (!result.ok) throw new Error('expected ok') + return result.agent + } + + async function createActiveAssistant( + assistantStore: AssistantStore, + status: 'active' | 'disabled' = 'active', + ): Promise<{ assistant: AssistantRecord; token: string }> { + const id = randomUUID() + const minted = mintAssistantToken(id) + const assistant = await assistantStore.create({ + id, + name: 'Draft Bot', + module: 'draft-reply', + tokenHash: minted.tokenHash, + }) + if (status === 'disabled') { + await assistantStore.patch(id, { status: 'disabled' }) + } + return { assistant, token: minted.token } + } + + async function seedConversation(store: ConversationStore, overrides: { subject?: string } = {}) { + return store.createConversation({ + subject: overrides.subject ?? 'Help with my order', + customerEmail: 'customer@example.test', + firstMessage: { + direction: 'inbound', + messageId: '', + fromAddress: 'customer@example.test', + bodyText: 'Where is my order?', + }, + }) + } + + // --- Assistant token auth: the second credential class ------------------- + + describe('Assistant token authentication', () => { + it('an Assistant token authenticates alongside the service Bearer token', async () => { + const { api, assistantStore } = await freshApi() + const { token } = await createActiveAssistant(assistantStore) + const res = await api(assistantReq('GET', '/api/v1/conversations', token)) + expect(res.status).toBe(200) + }) + + it('a disabled Assistant token is 401', async () => { + const { api, assistantStore } = await freshApi() + const { token } = await createActiveAssistant(assistantStore, 'disabled') + const res = await api(assistantReq('GET', '/api/v1/conversations', token)) + expect(res.status).toBe(401) + }) + + it('a malformed/unknown token is 401, same as a missing one', async () => { + const { api } = await freshApi() + const res1 = await api(assistantReq('GET', '/api/v1/conversations', 'ht_asst_not-real')) + expect(res1.status).toBe(401) + const res2 = await api(new Request('https://x.example.test/api/v1/conversations')) + expect(res2.status).toBe(401) + }) + }) + + // --- Capability gate: an Assistant may GET conversations, POST notes/drafts, nothing else --- + + describe('Assistant capability gate', () => { + it('an Assistant may GET the conversations list and a conversation detail', async () => { + const { api, store, assistantStore } = await freshApi() + const { token } = await createActiveAssistant(assistantStore) + const { conversationId } = await seedConversation(store) + + expect((await api(assistantReq('GET', '/api/v1/conversations', token))).status).toBe(200) + expect( + (await api(assistantReq('GET', `/api/v1/conversations/${conversationId}`, token))).status, + ).toBe(200) + }) + + it('an Assistant may POST a note', async () => { + const { api, store, assistantStore } = await freshApi() + const { token } = await createActiveAssistant(assistantStore) + const { conversationId } = await seedConversation(store) + + const res = await api( + assistantReq('POST', `/api/v1/conversations/${conversationId}/notes`, token, { + body: { text: 'Internal note from the assistant.' }, + }), + ) + expect(res.status).toBe(201) + const body = (await res.json()) as { authorKind: string } + expect(body.authorKind).toBe('assistant') + }) + + it('an Assistant is 403 on every other conversations route', async () => { + const { api, store, assistantStore } = await freshApi() + const { token } = await createActiveAssistant(assistantStore) + const { conversationId } = await seedConversation(store) + + const forbidden = [ + assistantReq('PATCH', `/api/v1/conversations/${conversationId}`, token, { + body: { status: 'closed' }, + }), + assistantReq('DELETE', `/api/v1/conversations/${conversationId}`, token), + assistantReq('POST', `/api/v1/conversations/${conversationId}/replies`, token, { + body: { text: 'hi' }, + }), + assistantReq('PUT', `/api/v1/conversations/${conversationId}/tags`, token, { + body: { tags: [] }, + }), + assistantReq('PUT', `/api/v1/conversations/${conversationId}/assignee`, token, { + body: { assigneeAgentId: null }, + }), + ] + for (const request of forbidden) { + const res = await api(request) + expect(res.status).toBe(403) + } + }) + + it('an Assistant is 403 on admin surfaces (agents, assistants, mailboxes) and the drafts review queue', async () => { + const { api, assistantStore } = await freshApi() + const { token } = await createActiveAssistant(assistantStore) + + const forbidden = [ + assistantReq('GET', '/api/v1/agents', token), + assistantReq('GET', '/api/v1/assistants', token), + assistantReq('GET', '/api/v1/mailboxes', token), + assistantReq('GET', '/api/v1/drafts?status=awaiting_review', token), + ] + for (const request of forbidden) { + const res = await api(request) + expect(res.status).toBe(403) + } + }) + + it('a soft-deleted conversation is 404 (indistinguishable from nonexistent) on every Assistant path', async () => { + const { api, store, assistantStore } = await freshApi() + const { token } = await createActiveAssistant(assistantStore) + const { conversationId } = await seedConversation(store) + await store.deleteConversation(conversationId) + + expect( + (await api(assistantReq('GET', `/api/v1/conversations/${conversationId}`, token))).status, + ).toBe(404) + expect( + ( + await api( + assistantReq('POST', `/api/v1/conversations/${conversationId}/notes`, token, { + body: { text: 'hi' }, + }), + ) + ).status, + ).toBe(404) + expect( + ( + await api( + assistantReq('POST', `/api/v1/conversations/${conversationId}/drafts`, token, { + body: { bodyText: 'hi' }, + idempotencyKey: 'k1', + }), + ) + ).status, + ).toBe(404) + }) + }) + + // --- POST /api/v1/conversations/{id}/drafts --------------------------------- + + describe('POST /api/v1/conversations/{id}/drafts', () => { + it('an Assistant creates a draft: awaiting_review, no message id, no delivery status', async () => { + const { api, store, assistantStore } = await freshApi() + const { token } = await createActiveAssistant(assistantStore) + const { conversationId } = await seedConversation(store) + + const res = await api( + assistantReq('POST', `/api/v1/conversations/${conversationId}/drafts`, token, { + body: { bodyText: 'Suggested reply.' }, + idempotencyKey: 'draft-key-1', + }), + ) + expect(res.status).toBe(201) + const body = (await res.json()) as { + draftStatus: string + deliveryStatus: string | null + authorKind: string + } + expect(body.draftStatus).toBe('awaiting_review') + expect(body.deliveryStatus).toBeNull() + expect(body.authorKind).toBe('assistant') + }) + + it('requires Idempotency-Key (400 when absent)', async () => { + const { api, store, assistantStore } = await freshApi() + const { token } = await createActiveAssistant(assistantStore) + const { conversationId } = await seedConversation(store) + + const res = await api( + assistantReq('POST', `/api/v1/conversations/${conversationId}/drafts`, token, { + body: { bodyText: 'Suggested reply.' }, + }), + ) + expect(res.status).toBe(400) + }) + + it('a replayed Idempotency-Key returns the original draft, not a second one', async () => { + const { api, store, assistantStore } = await freshApi() + const { token } = await createActiveAssistant(assistantStore) + const { conversationId } = await seedConversation(store) + + const first = await api( + assistantReq('POST', `/api/v1/conversations/${conversationId}/drafts`, token, { + body: { bodyText: 'Suggested reply.' }, + idempotencyKey: 'replay-key', + }), + ) + const second = await api( + assistantReq('POST', `/api/v1/conversations/${conversationId}/drafts`, token, { + body: { bodyText: 'A completely different body — ignored on replay.' }, + idempotencyKey: 'replay-key', + }), + ) + const firstBody = (await first.json()) as { id: string } + const secondBody = (await second.json()) as { id: string; bodyText: string } + expect(secondBody.id).toBe(firstBody.id) + expect(secondBody.bodyText).toBe('Suggested reply.') + + const conversation = await store.getConversation(conversationId, { includeDeleted: false }) + expect(conversation?.threads.filter((t) => t.draftStatus !== null)).toHaveLength(1) + }) + + it('a draft idempotency key never collides with a reply idempotency key on the same conversation (draft: prefix)', async () => { + const { api, store, agentStore, assistantStore } = await freshApi() + const agent = await createActiveAgent(agentStore) + const { token } = await createActiveAssistant(assistantStore) + const { conversationId } = await seedConversation(store) + + await api( + assistantReq('POST', `/api/v1/conversations/${conversationId}/drafts`, token, { + body: { bodyText: 'Draft body.' }, + idempotencyKey: 'shared-key', + }), + ) + const replyRes = await api( + req('POST', `/api/v1/conversations/${conversationId}/replies`, { + agentId: agent.id, + body: { text: 'Real reply body.' }, + idempotencyKey: 'shared-key', + }), + ) + expect(replyRes.status).toBe(201) + + const conversation = await store.getConversation(conversationId, { includeDeleted: false }) + const idempotencyKeys = conversation?.threads.map((t) => t.idempotencyKey) + expect(idempotencyKeys).toEqual(expect.arrayContaining(['draft:shared-key', 'shared-key'])) + }) + + it('a service-Bearer caller (no Assistant identity) cannot create a draft', async () => { + const { api, store, agentStore } = await freshApi() + const agent = await createActiveAgent(agentStore) + const { conversationId } = await seedConversation(store) + + const res = await api( + req('POST', `/api/v1/conversations/${conversationId}/drafts`, { + agentId: agent.id, + body: { bodyText: 'hi' }, + idempotencyKey: 'k1', + }), + ) + expect(res.status).toBe(403) + }) + }) + + // --- GET /api/v1/drafts?status=awaiting_review ------------------------- + + describe('GET /api/v1/drafts', () => { + it('lists awaiting_review drafts newest first for a service/Agent caller', async () => { + const { api, store, assistantStore } = await freshApi() + const { token } = await createActiveAssistant(assistantStore) + const { conversationId: c1 } = await seedConversation(store) + const { conversationId: c2 } = await seedConversation(store, { subject: 'Second' }) + + await api( + assistantReq('POST', `/api/v1/conversations/${c1}/drafts`, token, { + body: { bodyText: 'first' }, + idempotencyKey: 'd1', + }), + ) + await api( + assistantReq('POST', `/api/v1/conversations/${c2}/drafts`, token, { + body: { bodyText: 'second' }, + idempotencyKey: 'd2', + }), + ) + + const res = await api(req('GET', '/api/v1/drafts?status=awaiting_review')) + expect(res.status).toBe(200) + const body = (await res.json()) as { drafts: Array<{ draftStatus: string }> } + expect(body.drafts).toHaveLength(2) + expect(body.drafts.every((d) => d.draftStatus === 'awaiting_review')).toBe(true) + }) + + it('400s without status=awaiting_review', async () => { + const { api } = await freshApi() + expect((await api(req('GET', '/api/v1/drafts'))).status).toBe(400) + expect((await api(req('GET', '/api/v1/drafts?status=approved'))).status).toBe(400) + }) + }) + + // --- POST /api/v1/drafts/{threadId}/approve --------------------------------- + + describe('POST /api/v1/drafts/{threadId}/approve', () => { + async function seedDraft( + api: (request: Request) => Promise, + store: ConversationStore, + assistantToken: string, + bodyText = 'Suggested reply.', + ): Promise<{ conversationId: string; threadId: string }> { + const { conversationId } = await seedConversation(store) + const res = await api( + assistantReq('POST', `/api/v1/conversations/${conversationId}/drafts`, assistantToken, { + body: { bodyText }, + idempotencyKey: `draft-${conversationId}`, + }), + ) + const body = (await res.json()) as { id: string } + return { conversationId, threadId: body.id } + } + + it('an Agent approves an unedited draft: it gets delivered and the row is updated', async () => { + const { api, store, agentStore, assistantStore, sent } = await freshApi() + const agent = await createActiveAgent(agentStore) + const { token } = await createActiveAssistant(assistantStore) + const { threadId } = await seedDraft(api, store, token) + + const res = await api( + req('POST', `/api/v1/drafts/${threadId}/approve`, { agentId: agent.id }), + ) + expect(res.status).toBe(200) + const body = (await res.json()) as { + draftStatus: string + deliveryStatus: string | null + bodyText: string + } + expect(body.draftStatus).toBe('approved') + expect(body.deliveryStatus).toBe('sent') + expect(body.bodyText).toBe('Suggested reply.') + expect(sent).toHaveLength(1) + expect(sent[0].to).toEqual(['customer@example.test']) + }) + + it('approve with edits overrides the body and is delivered with the edited content', async () => { + const { api, store, agentStore, assistantStore, sent } = await freshApi() + const agent = await createActiveAgent(agentStore) + const { token } = await createActiveAssistant(assistantStore) + const { threadId } = await seedDraft(api, store, token, 'Original body.') + + const res = await api( + req('POST', `/api/v1/drafts/${threadId}/approve`, { + agentId: agent.id, + body: { bodyText: 'Edited before sending.' }, + }), + ) + expect(res.status).toBe(200) + const body = (await res.json()) as { bodyText: string } + expect(body.bodyText).toBe('Edited before sending.') + expect(sent[0].text).toBe('Edited before sending.') + }) + + describe('approve-with-edits body validation (HT-70 review fix, Codex — same bound as draft-create/reply)', () => { + it('an empty-string bodyText override is 400 validation_failed; nothing is delivered', async () => { + const { api, store, agentStore, assistantStore, sent } = await freshApi() + const agent = await createActiveAgent(agentStore) + const { token } = await createActiveAssistant(assistantStore) + const { threadId } = await seedDraft(api, store, token) + + const res = await api( + req('POST', `/api/v1/drafts/${threadId}/approve`, { + agentId: agent.id, + body: { bodyText: '' }, + }), + ) + expect(res.status).toBe(400) + expect(await res.json()).toEqual({ + error: { code: 'validation_failed', message: expect.any(String) }, + }) + expect(sent).toHaveLength(0) + }) + + it('a bodyText override over 5000 characters is 400 validation_failed; nothing is delivered', async () => { + const { api, store, agentStore, assistantStore, sent } = await freshApi() + const agent = await createActiveAgent(agentStore) + const { token } = await createActiveAssistant(assistantStore) + const { threadId } = await seedDraft(api, store, token) + + const res = await api( + req('POST', `/api/v1/drafts/${threadId}/approve`, { + agentId: agent.id, + body: { bodyText: 'a'.repeat(5001) }, + }), + ) + expect(res.status).toBe(400) + expect(sent).toHaveLength(0) + }) + + it('a bodyText override of exactly 5000 characters (the boundary) is accepted and delivered', async () => { + const { api, store, agentStore, assistantStore, sent } = await freshApi() + const agent = await createActiveAgent(agentStore) + const { token } = await createActiveAssistant(assistantStore) + const { threadId } = await seedDraft(api, store, token) + const boundaryText = 'a'.repeat(5000) + + const res = await api( + req('POST', `/api/v1/drafts/${threadId}/approve`, { + agentId: agent.id, + body: { bodyText: boundaryText }, + }), + ) + expect(res.status).toBe(200) + const body = (await res.json()) as { bodyText: string } + expect(body.bodyText).toBe(boundaryText) + expect(sent[0].text).toBe(boundaryText) + }) + }) + + it('401s with no acting-Agent header', async () => { + const { api, store, assistantStore } = await freshApi() + const { token } = await createActiveAssistant(assistantStore) + const { threadId } = await seedDraft(api, store, token) + + const res = await api(req('POST', `/api/v1/drafts/${threadId}/approve`)) + expect(res.status).toBe(401) + }) + + it('404s for an unknown threadId or an already-resolved draft', async () => { + const { api, agentStore, store, assistantStore } = await freshApi() + const agent = await createActiveAgent(agentStore) + const { token } = await createActiveAssistant(assistantStore) + const { threadId } = await seedDraft(api, store, token) + + expect( + ( + await api( + req('POST', '/api/v1/drafts/00000000-0000-4000-8000-000000000000/approve', { + agentId: agent.id, + }), + ) + ).status, + ).toBe(404) + + await api(req('POST', `/api/v1/drafts/${threadId}/approve`, { agentId: agent.id })) + const second = await api( + req('POST', `/api/v1/drafts/${threadId}/approve`, { agentId: agent.id }), + ) + expect(second.status).toBe(404) + }) + + it('404s for a draft on a soft-deleted conversation', async () => { + const { api, agentStore, store, assistantStore } = await freshApi() + const agent = await createActiveAgent(agentStore) + const { token } = await createActiveAssistant(assistantStore) + const { conversationId, threadId } = await seedDraft(api, store, token) + await store.deleteConversation(conversationId) + + const res = await api( + req('POST', `/api/v1/drafts/${threadId}/approve`, { agentId: agent.id }), + ) + expect(res.status).toBe(404) + }) + + it('409s for a draft on a spam conversation', async () => { + const { api, agentStore, store, assistantStore } = await freshApi() + const agent = await createActiveAgent(agentStore) + const { token } = await createActiveAssistant(assistantStore) + const { conversationId, threadId } = await seedDraft(api, store, token) + await api( + req('PATCH', `/api/v1/conversations/${conversationId}`, { + agentId: agent.id, + body: { status: 'spam' }, + }), + ) + + const res = await api( + req('POST', `/api/v1/drafts/${threadId}/approve`, { agentId: agent.id }), + ) + expect(res.status).toBe(409) + }) + + it('approving a draft on a CLOSED conversation reopens it to active, end to end (Opus review fix)', async () => { + const { api, agentStore, store, assistantStore } = await freshApi() + const agent = await createActiveAgent(agentStore) + const { token } = await createActiveAssistant(assistantStore) + const { conversationId, threadId } = await seedDraft(api, store, token) + await api( + req('PATCH', `/api/v1/conversations/${conversationId}`, { + agentId: agent.id, + body: { status: 'closed' }, + }), + ) + + const res = await api( + req('POST', `/api/v1/drafts/${threadId}/approve`, { agentId: agent.id }), + ) + expect(res.status).toBe(200) + + const conversation = await store.getConversation(conversationId, { includeDeleted: false }) + expect(conversation?.status).toBe('active') + }) + + describe('TOCTOU: the preflight is a stale read, resolveDraft is the authoritative gate (HT-70 review fix, Codex)', () => { + /** + * Build a SECOND `createInboxApi` instance sharing the same + * `db`/`agentStore`/`assistantStore`/`sent`, but whose + * `getConversationByThreadId` ALWAYS returns `staleSnapshot` — + * simulating a preflight read taken before a concurrent delete/ + * spam-mark commits. Every OTHER store method (crucially + * `resolveDraft`, which does its own fresh, locked read) passes + * through to the real store, so this exercises exactly the race: a + * stale preflight racing a fresh, authoritative write. + */ + function racyApi( + db: Db, + realStore: ConversationStore, + staleSnapshot: Awaited>, + agentStore: AgentStore, + assistantStore: AssistantStore, + sender: EmailSender, + ): (request: Request) => Promise { + const racedStore: ConversationStore = { + ...realStore, + async getConversationByThreadId() { + return staleSnapshot + }, + } + return createInboxApi({ + store: racedStore, + apiToken: TOKEN, + sender, + keyring: KEYRING, + mailDomain: MAIL_DOMAIN, + supportAddress: SUPPORT_ADDRESS, + agents: { + store: agentStore, + providers: [createPasswordAuthProvider({ agentStore })], + mailboxStore: createMailboxStore(db), + }, + assistants: { store: assistantStore }, + webhooks: { + store: {} as unknown as WebhooksApiDeps['store'], + queue: { async enqueue() {} }, + } satisfies WebhooksApiDeps, + }) + } + + it('conversation DELETED between the stale preflight and the write: refused (404-shape), no approval, no delivery_status change', async () => { + const { api, store, agentStore, assistantStore, sent } = await freshApi() + const agent = await createActiveAgent(agentStore) + const { token } = await createActiveAssistant(assistantStore) + const { conversationId, threadId } = await seedDraft(api, store, token) + + // The snapshot the "preflight" would have seen — taken BEFORE the + // concurrent delete below. + const staleSnapshot = await store.getConversationByThreadId(threadId, { + includeDeleted: false, + }) + expect(staleSnapshot?.status).not.toBe('deleted') + + // The "concurrent" delete — committed AFTER the stale snapshot, but + // BEFORE the approve write below runs. + await store.deleteConversation(conversationId) + + const raced = racyApi(db as Db, store, staleSnapshot, agentStore, assistantStore, { + maxSendMs: 30_000, + async send(email) { + sent.push(email) + return {} + }, + }) + const res = await raced( + req('POST', `/api/v1/drafts/${threadId}/approve`, { agentId: agent.id }), + ) + expect(res.status).toBe(404) + expect(sent).toHaveLength(0) + + const conversation = await store.getConversation(conversationId, { + includeDeleted: true, + }) + const thread = conversation?.threads.find((t) => t.id === threadId) + expect(thread?.draftStatus).toBe('awaiting_review') + expect(thread?.deliveryStatus).toBeNull() + expect(thread?.messageId).toBeNull() + }) + + it('conversation marked SPAM between the stale preflight and the write: refused (409-shape), no approval, no delivery_status change', async () => { + const { api, store, agentStore, assistantStore, sent } = await freshApi() + const agent = await createActiveAgent(agentStore) + const { token } = await createActiveAssistant(assistantStore) + const { conversationId, threadId } = await seedDraft(api, store, token) + + const staleSnapshot = await store.getConversationByThreadId(threadId, { + includeDeleted: false, + }) + expect(staleSnapshot?.status).not.toBe('spam') + + await api( + req('PATCH', `/api/v1/conversations/${conversationId}`, { + agentId: agent.id, + body: { status: 'spam' }, + }), + ) + + const raced = racyApi(db as Db, store, staleSnapshot, agentStore, assistantStore, { + maxSendMs: 30_000, + async send(email) { + sent.push(email) + return {} + }, + }) + const res = await raced( + req('POST', `/api/v1/drafts/${threadId}/approve`, { agentId: agent.id }), + ) + expect(res.status).toBe(409) + expect(sent).toHaveLength(0) + + const conversation = await store.getConversation(conversationId, { + includeDeleted: false, + }) + const thread = conversation?.threads.find((t) => t.id === threadId) + expect(thread?.draftStatus).toBe('awaiting_review') + expect(thread?.deliveryStatus).toBeNull() + }) + + it('conversation CLOSED between the stale preflight and the write: still reopens to active (the locked read-then-write folds the reopen in)', async () => { + const { api, store, agentStore, assistantStore, sent } = await freshApi() + const agent = await createActiveAgent(agentStore) + const { token } = await createActiveAssistant(assistantStore) + const { conversationId, threadId } = await seedDraft(api, store, token) + + const staleSnapshot = await store.getConversationByThreadId(threadId, { + includeDeleted: false, + }) + expect(staleSnapshot?.status).not.toBe('closed') + + await api( + req('PATCH', `/api/v1/conversations/${conversationId}`, { + agentId: agent.id, + body: { status: 'closed' }, + }), + ) + + const raced = racyApi(db as Db, store, staleSnapshot, agentStore, assistantStore, { + maxSendMs: 30_000, + async send(email) { + sent.push(email) + return {} + }, + }) + const res = await raced( + req('POST', `/api/v1/drafts/${threadId}/approve`, { agentId: agent.id }), + ) + expect(res.status).toBe(200) + expect(sent).toHaveLength(1) + + const conversation = await store.getConversation(conversationId, { + includeDeleted: false, + }) + expect(conversation?.status).toBe('active') + }) + }) + + it('502s when the provider rejects the send; the draft stays approved but delivery fails', async () => { + const { api, agentStore, store, assistantStore } = await freshApi({ + sender: createThrowingSender(), + }) + const agent = await createActiveAgent(agentStore) + const { token } = await createActiveAssistant(assistantStore) + const { threadId } = await seedDraft(api, store, token) + + const res = await api( + req('POST', `/api/v1/drafts/${threadId}/approve`, { agentId: agent.id }), + ) + expect(res.status).toBe(502) + + const conversation = await store.getConversation( + (await store.getConversationByThreadId(threadId))?.id ?? '', + { includeDeleted: false }, + ) + const thread = conversation?.threads.find((t) => t.id === threadId) + expect(thread?.draftStatus).toBe('approved') + expect(thread?.deliveryStatus).toBe('failed') + }) + + it('an Assistant may never approve (not in the capability gate; also has no Agent identity)', async () => { + const { api, store, assistantStore } = await freshApi() + const { token } = await createActiveAssistant(assistantStore) + const { threadId } = await seedDraft(api, store, token) + + const res = await api(assistantReq('POST', `/api/v1/drafts/${threadId}/approve`, token)) + expect(res.status).toBe(403) + }) + }) + + // --- POST /api/v1/drafts/{threadId}/discard --------------------------------- + + describe('POST /api/v1/drafts/{threadId}/discard', () => { + it('an Agent discards a draft: no delivery, no reply sent', async () => { + const { api, store, agentStore, assistantStore, sent } = await freshApi() + const agent = await createActiveAgent(agentStore) + const { token } = await createActiveAssistant(assistantStore) + const { conversationId } = await seedConversation(store) + const created = await api( + assistantReq('POST', `/api/v1/conversations/${conversationId}/drafts`, token, { + body: { bodyText: 'Will be discarded.' }, + idempotencyKey: 'discard-key', + }), + ) + const { id: threadId } = (await created.json()) as { id: string } + + const res = await api( + req('POST', `/api/v1/drafts/${threadId}/discard`, { agentId: agent.id }), + ) + expect(res.status).toBe(200) + const body = (await res.json()) as { draftStatus: string; deliveryStatus: string | null } + expect(body.draftStatus).toBe('discarded') + expect(body.deliveryStatus).toBeNull() + expect(sent).toHaveLength(0) + }) + + it('401s with no acting-Agent header', async () => { + const { api, store, assistantStore } = await freshApi() + const { token } = await createActiveAssistant(assistantStore) + const { conversationId } = await seedConversation(store) + const created = await api( + assistantReq('POST', `/api/v1/conversations/${conversationId}/drafts`, token, { + body: { bodyText: 'hi' }, + idempotencyKey: 'k1', + }), + ) + const { id: threadId } = (await created.json()) as { id: string } + + const res = await api(req('POST', `/api/v1/drafts/${threadId}/discard`)) + expect(res.status).toBe(401) + }) + }) + + // --- Conversation list/detail: unresolved drafts excluded from preview/threadCount --- + + describe('threadCount/preview exclude unresolved drafts on the list AND detail views', () => { + it('GET /conversations and GET /conversations/{id} both ignore an awaiting_review draft', async () => { + const { api, store, assistantStore } = await freshApi() + const { token } = await createActiveAssistant(assistantStore) + const { conversationId } = await seedConversation(store) + + await api( + assistantReq('POST', `/api/v1/conversations/${conversationId}/drafts`, token, { + body: { bodyText: 'Nobody has approved this yet.' }, + idempotencyKey: 'k1', + }), + ) + + const listRes = await api(req('GET', '/api/v1/conversations')) + const listBody = (await listRes.json()) as { + conversations: Array<{ id: string; threadCount: number; preview: string }> + } + const summary = listBody.conversations.find((c) => c.id === conversationId) + expect(summary?.threadCount).toBe(1) + expect(summary?.preview).not.toContain('Nobody has approved this yet.') + + const detailRes = await api(req('GET', `/api/v1/conversations/${conversationId}`)) + const detailBody = (await detailRes.json()) as { + threadCount: number + preview: string + threads: unknown[] + } + expect(detailBody.threadCount).toBe(1) + expect(detailBody.preview).not.toContain('Nobody has approved this yet.') + // The full timeline STILL includes the draft row (spec §7's last bullet). + expect(detailBody.threads).toHaveLength(2) + }) + }) +}) diff --git a/src/api/drafts.ts b/src/api/drafts.ts new file mode 100644 index 0000000..f6855b2 --- /dev/null +++ b/src/api/drafts.ts @@ -0,0 +1,444 @@ +/** + * The drafts API (HT-70; specs/plugins/substrate-v1.md §6): + * `POST /api/v1/conversations/{id}/drafts` (assistant-auth), + * `GET /api/v1/drafts?status=awaiting_review`, + * `POST /api/v1/drafts/{threadId}/approve`, + * `POST /api/v1/drafts/{threadId}/discard` (Agent/service-auth). + * + * Same conventions as `src/api/conversations.ts`: each handler is a pure + * function of an already-authenticated, already-routed `Request` plus its + * dependencies. Which credential class is required per route is enforced + * ONE level up, in `src/api/index.ts`'s capability gate (spec §3's "capability + * enforcement lives at one point") — this module does not re-check whether + * the caller is an Assistant or a service/Agent caller itself, except where + * the SAME route is reachable by more than one credential and the two need + * different handling (`handleCreateDraft` requires the resolved + * `AssistantRecord` its caller already authenticated). + */ + +import { approveDraft } from '../mail/approve-draft.js' +import type { Keyring } from '../mail/reply-token.js' +import type { SelfEchoGuardDeps } from '../mail/send.js' +import type { EmailSender } from '../providers/index.js' +import type { AgentRecord } from '../store/agents.js' +import type { AssistantRecord } from '../store/assistants.js' +import type { ConversationStore, ListAwaitingDraftsCursor } from '../store/conversations.js' +import { toThreadViewJson } from './conversations.js' +import { decodeDraftCursor, encodeDraftCursor } from './cursor.js' +import { apiError, json } from './responses.js' +import { isUuid } from './uuid.js' + +/** Dependencies every handler in this module may need. */ +export interface DraftsHandlerDeps { + store: ConversationStore + sender: EmailSender + keyring: Keyring + mailDomain: string + supportAddress: string + openTracking?: { publicBaseUrl: string } + selfEchoGuard?: SelfEchoGuardDeps +} + +/** Mirrors `src/api/conversations.ts`'s `MAX_IDEMPOTENCY_KEY_LENGTH` — kept local per this codebase's per-file convention (that constant is not exported). */ +const MAX_IDEMPOTENCY_KEY_LENGTH = 255 +/** Same body-length bounds `src/api/conversations.ts` uses for a reply/note's `text` (spec §4a/§4c) — a draft's `bodyText` is the same kind of value. */ +const MIN_BODY_LENGTH = 1 +const MAX_BODY_LENGTH = 5000 + +const DEFAULT_LIMIT = 25 +const MAX_LIMIT = 50 +const MIN_LIMIT = 1 + +const UNAUTHORIZED = () => apiError(401, 'unauthorized', 'Missing or invalid Agent identity.') +const NOT_FOUND = () => apiError(404, 'not_found', 'No draft with that id.') + +/** Read and JSON-parse `request`'s body without ever throwing — mirrors `src/api/conversations.ts`'s helper of the same name. */ +async function parseJsonBody( + request: Request, +): Promise<{ ok: true; value: unknown } | { ok: false }> { + try { + return { ok: true, value: await request.json() } + } catch { + return { ok: false } + } +} + +// --- POST /api/v1/conversations/{id}/drafts (assistant-auth) --------------- + +/** Validated shape of `POST .../drafts`'s request body (spec §6). */ +interface DraftRequestBody { + bodyText: string + bodyHtml?: string +} + +function parseDraftBody(raw: unknown): DraftRequestBody | null { + if (typeof raw !== 'object' || raw === null) return null + const { bodyText, bodyHtml } = raw as Record + if ( + typeof bodyText !== 'string' || + bodyText.length < MIN_BODY_LENGTH || + bodyText.length > MAX_BODY_LENGTH + ) { + return null + } + if (bodyHtml !== undefined && typeof bodyHtml !== 'string') return null + return bodyHtml === undefined ? { bodyText } : { bodyText, bodyHtml } +} + +/** + * `POST /api/v1/conversations/{id}/drafts` (spec §6) — assistant-auth only; + * `assistant` is the ALREADY-AUTHENTICATED caller (`src/api/index.ts` + * resolves this via `authenticateAssistantRequest` before dispatch, and + * refuses any other credential at the capability gate). `Idempotency-Key` + * is REQUIRED, stored prefixed `` `draft:${key}` `` by + * `ConversationStore.appendDraft` — sharing the `(conversation_id, + * idempotency_key)` namespace with replies. That prefix alone is NOT what + * keeps the two sub-namespaces disjoint (a reply key is stored raw, so a + * caller-supplied reply key literally spelled `draft:abc` would otherwise + * collide with an engine-owned draft key of the same name) — the actual + * guarantee is the PAIR: this engine-owned `draft:` prefix, plus + * `handleReply` (`src/api/conversations.ts`) rejecting any caller-supplied + * reply `Idempotency-Key` that itself starts with `draft:`. `201` with the + * created `ThreadView` on success; + * `404 not_found` for a missing or soft-deleted conversation + * (indistinguishable, per §4d); `400 validation_failed` on a missing/ + * over-length `Idempotency-Key` or an invalid body. + */ +export async function handleCreateDraft( + id: string, + assistant: AssistantRecord, + request: Request, + deps: Pick, +): Promise { + if (!isUuid(id)) { + return apiError(404, 'not_found', 'No conversation with that id.') + } + + const rawIdempotencyKey = request.headers.get('Idempotency-Key') + const idempotencyKey = rawIdempotencyKey?.trim() ?? '' + if (idempotencyKey === '') { + return apiError(400, 'validation_failed', 'Idempotency-Key header is required.') + } + if (idempotencyKey.length > MAX_IDEMPOTENCY_KEY_LENGTH) { + return apiError( + 400, + 'validation_failed', + `Idempotency-Key must be at most ${MAX_IDEMPOTENCY_KEY_LENGTH} characters.`, + ) + } + + const parsedBody = await parseJsonBody(request) + if (!parsedBody.ok) { + return apiError(400, 'validation_failed', 'Request body must be valid JSON.') + } + const draft = parseDraftBody(parsedBody.value) + if (draft === null) { + return apiError( + 400, + 'validation_failed', + `bodyText is required and must be ${MIN_BODY_LENGTH}-${MAX_BODY_LENGTH} characters; bodyHtml, if present, must be a string.`, + ) + } + + const result = await deps.store.appendDraft(id, { + assistantId: assistant.id, + bodyText: draft.bodyText, + ...(draft.bodyHtml !== undefined ? { bodyHtml: draft.bodyHtml } : {}), + // The eventual outbound "From" — the deployment's support address, the + // SAME one every Agent reply uses (src/api/conversations.ts's + // handleReply). An assistant has no mailbox identity of its own to + // offer here, and approval never derives/overwrites this column, so it + // must be right at draft-creation time. + fromAddress: deps.supportAddress, + idempotencyKey, + }) + if (!result.ok) { + // not-found and deleted are one generic 404 (spec §4d's no-existence-leak). + return NOT_FOUND() + } + + return json(201, toThreadViewJson(result.thread)) +} + +// --- GET /api/v1/drafts?status=awaiting_review (Agent/service-auth) -------- + +/** + * `GET /api/v1/drafts?status=awaiting_review` (spec §6) — the cross- + * conversation review queue, newest first, keyset-paginated. `status` is + * REQUIRED and its only legal value is `'awaiting_review'` (there is no + * other queue this endpoint serves — resolved drafts surface through + * conversation detail, not here). + */ +export async function handleListDrafts( + request: Request, + deps: Pick, +): Promise { + const url = new URL(request.url) + + const status = url.searchParams.get('status') + if (status !== 'awaiting_review') { + return apiError(400, 'validation_failed', "status must be 'awaiting_review'.") + } + + const limitParam = url.searchParams.get('limit') + let limit = DEFAULT_LIMIT + if (limitParam !== null) { + const parsed = Number(limitParam) + if (!Number.isFinite(parsed)) { + return apiError(400, 'validation_failed', 'limit must be a number.') + } + limit = Math.trunc(parsed) + } + limit = Math.min(MAX_LIMIT, Math.max(MIN_LIMIT, limit)) + + const cursorParam = url.searchParams.get('cursor') + let cursor: ListAwaitingDraftsCursor | undefined + if (cursorParam !== null) { + const decoded = decodeDraftCursor(cursorParam) + if (decoded === null) { + return apiError(400, 'validation_failed', 'cursor is invalid or expired.') + } + cursor = decoded + } + + // Over-fetch-by-one for pagination detection — same trick + // handleListConversations uses (src/api/conversations.ts). + const rows = await deps.store.listAwaitingDrafts({ limit: limit + 1, cursor }) + const hasNextPage = rows.length > limit + const page = rows.slice(0, limit) + + return json(200, { + drafts: page.map((thread) => toThreadViewJson(thread)), + nextCursor: + hasNextPage && page.length > 0 + ? encodeDraftCursor({ + createdAt: page[page.length - 1].createdAt, + id: page[page.length - 1].id, + }) + : null, + }) +} + +// --- POST /api/v1/drafts/{threadId}/approve --------------------------------- + +/** + * Validated shape of the OPTIONAL "approve with edits" body (spec §6) — + * parsed leniently: an absent or empty body means "no edit", never a `400`. + * + * HT-70 review fix (Codex): a PRESENT `bodyText` gets the SAME length bound + * (`[MIN_BODY_LENGTH, MAX_BODY_LENGTH]`) `parseDraftBody` (draft-create, + * above) and `parseReplyBody` (`src/api/conversations.ts`) already enforce + * on their own required `bodyText`/`text` — without this, an Agent's edit + * could push a payload past the reply path's own limit into the send path, + * where nothing else re-checks it. `bodyHtml`, if present, is type-checked + * only (no length bound) — matching those SAME two sibling paths exactly: + * neither enforces a length bound on `bodyHtml`/`html` either. + */ +function parseApproveEditBody( + raw: unknown, +): { ok: true; edit: { bodyText?: string; bodyHtml?: string } | undefined } | { ok: false } { + if (typeof raw !== 'object' || raw === null) return { ok: false } + const { bodyText, bodyHtml } = raw as Record + if (bodyText !== undefined) { + if ( + typeof bodyText !== 'string' || + bodyText.length < MIN_BODY_LENGTH || + bodyText.length > MAX_BODY_LENGTH + ) { + return { ok: false } + } + } + if (bodyHtml !== undefined && typeof bodyHtml !== 'string') return { ok: false } + if (bodyText === undefined && bodyHtml === undefined) return { ok: true, edit: undefined } + return { + ok: true, + edit: { + ...(bodyText !== undefined ? { bodyText } : {}), + ...(bodyHtml !== undefined ? { bodyHtml } : {}), + }, + } +} + +/** + * Read `request`'s body as an OPTIONAL "approve with edits" override. An + * entirely empty body (the common case — approving unedited) is legal and + * means "no edit", NOT a JSON-parse error — unlike every other body-bearing + * endpoint in this API, whose body is required. `{}` is likewise "no edit" + * (present keys are what signal an override, not body presence alone — + * see `parseApproveEditBody`). + */ +async function readApproveEditBody( + request: Request, +): Promise< + { ok: true; edit: { bodyText?: string; bodyHtml?: string } | undefined } | { ok: false } +> { + const text = await request.text() + if (text.trim() === '') return { ok: true, edit: undefined } + let parsed: unknown + try { + parsed = JSON.parse(text) + } catch { + return { ok: false } + } + return parseApproveEditBody(parsed) +} + +/** + * `POST /api/v1/drafts/{threadId}/approve` (spec §6) — Agent/service-auth; + * `actingAgent` MUST be present (`401` if not — the row's + * `approved_by_agent_id` audit column requires a real Agent identity, spec: + * "a draft never leaves the system without an approving Agent identity on + * the row"). Optional body `{ bodyText?, bodyHtml? }` is "approve with + * edits". Refused `404` (indistinguishable-from-nonexistent, §4d) for a + * missing/soft-deleted conversation or a `threadId` that doesn't name an + * `awaiting_review` draft; refused `409 conflict` on a `spam` conversation. + * On send failure, `502 send_failed`; on a delivery-lease race, + * `409 retry_in_progress` — same shapes `handleReply` uses for the + * equivalent outcomes. + * + * **HT-70 review fix (Codex): the checks below are a FAST PATH, not the + * authoritative gate.** This preflight read (`getConversationByThreadId`) + * can go stale the instant a concurrent delete or spam-mark lands between + * it and the write — the AUTHORITATIVE check is `approveDraft` → + * `ConversationStore.resolveDraft`'s own locked, in-transaction re-read + * (`src/store/conversations.ts`), whose `conversation-deleted`/ + * `conversation-spam` outcomes are handled in the `!result.ok` branch below + * with the SAME response shapes this preflight already uses — so a race + * that slips past this stale read still resolves correctly, just one layer + * deeper. + */ +export async function handleApproveDraft( + threadId: string, + actingAgent: AgentRecord | null, + request: Request, + deps: DraftsHandlerDeps, +): Promise { + if (actingAgent === null) return UNAUTHORIZED() + if (!isUuid(threadId)) return NOT_FOUND() + + const conversation = await deps.store.getConversationByThreadId(threadId, { + includeDeleted: false, + }) + if (conversation === null) return NOT_FOUND() + + const draftThread = conversation.threads.find((t) => t.id === threadId) + if (draftThread === undefined || draftThread.draftStatus !== 'awaiting_review') { + return NOT_FOUND() + } + if (conversation.status === 'spam') { + return apiError(409, 'conflict', 'Cannot approve a draft on a spam conversation.') + } + + const parsedEdit = await readApproveEditBody(request) + if (!parsedEdit.ok) { + return apiError( + 400, + 'validation_failed', + `Request body, if present, must be a JSON object with optional bodyText (${MIN_BODY_LENGTH}-${MAX_BODY_LENGTH} characters) and bodyHtml (string) fields.`, + ) + } + + const result = await approveDraft( + { + conversation, + draftThreadId: threadId, + resolvedByAgentId: actingAgent.id, + edit: parsedEdit.edit, + }, + { + store: deps.store, + sender: deps.sender, + keyring: deps.keyring, + mailDomain: deps.mailDomain, + ...(deps.openTracking !== undefined ? { openTracking: deps.openTracking } : {}), + ...(deps.selfEchoGuard !== undefined ? { selfEchoGuard: deps.selfEchoGuard } : {}), + }, + ) + + if (!result.ok) { + if (result.reason === 'send-failed') { + return apiError(502, 'send_failed', 'The draft could not be delivered.') + } + if (result.reason === 'retry-in-progress') { + return apiError( + 409, + 'retry_in_progress', + 'A delivery attempt for this draft is already in progress.', + ) + } + if (result.reason === 'conversation-deleted') { + // The AUTHORITATIVE catch (HT-70 review fix, Codex): the preflight + // above said "not deleted", but resolveDraft's locked re-check found + // otherwise at write time — same 404-shape as the preflight's own + // (indistinguishable-from-nonexistent, §4d). + return NOT_FOUND() + } + if (result.reason === 'conversation-spam') { + // Same authoritative catch, for a concurrent spam-mark. + return apiError(409, 'conflict', 'Cannot approve a draft on a spam conversation.') + } + // 'not-a-draft' — a race between the snapshot above and the write. + return NOT_FOUND() + } + + const updated = await deps.store.getConversation(conversation.id, { includeDeleted: false }) + const thread = updated?.threads.find((t) => t.id === result.threadId) + if (updated == null || thread === undefined) { + // Should be unreachable — approveDraft just reported a successful + // resolution of exactly this thread id. Mirrors handleReply's own + // defensive fallback for the same shape of invariant. + return apiError(500, 'server_error', 'Internal server error.') + } + + return json(200, toThreadViewJson(thread)) +} + +// --- POST /api/v1/drafts/{threadId}/discard --------------------------------- + +/** + * `POST /api/v1/drafts/{threadId}/discard` (spec §6) — Agent/service-auth; + * `actingAgent` MUST be present (`401` if not, same requirement as approve + * — `resolveDraft`'s `resolvedByAgentId` is the resolution audit field + * generally, not "approval" specifically). `200` with the updated + * `ThreadView` (`draftStatus: 'discarded'`) on success; `404 not_found` + * (indistinguishable-from-nonexistent) for a missing/soft-deleted + * conversation or a `threadId` that doesn't name an `awaiting_review` + * draft. No spam restriction (unlike approve) — discarding a draft on a + * spam conversation is harmless. + */ +export async function handleDiscardDraft( + threadId: string, + actingAgent: AgentRecord | null, + deps: Pick, +): Promise { + if (actingAgent === null) return UNAUTHORIZED() + if (!isUuid(threadId)) return NOT_FOUND() + + const conversation = await deps.store.getConversationByThreadId(threadId, { + includeDeleted: false, + }) + if (conversation === null) return NOT_FOUND() + + const draftThread = conversation.threads.find((t) => t.id === threadId) + if (draftThread === undefined || draftThread.draftStatus !== 'awaiting_review') { + return NOT_FOUND() + } + + const resolved = await deps.store.resolveDraft({ + action: 'discard', + threadId, + resolvedByAgentId: actingAgent.id, + }) + // `resolveDraft`'s return type is shared with the approve branch + // (`StoredThread | null | 'conversation-deleted' | 'conversation-spam'`), + // but the discard branch itself never produces either sentinel string — + // discard has no conversation-status restriction (this handler's own doc + // comment). The `typeof ... === 'string'` arm is therefore unreachable at + // runtime; it exists only to satisfy the shared return type. + if (resolved === null || typeof resolved === 'string') { + // A race between the snapshot above and the write. + return NOT_FOUND() + } + + return json(200, toThreadViewJson(resolved)) +} diff --git a/src/api/index.test.ts b/src/api/index.test.ts index a1178cb..fdb28f2 100644 --- a/src/api/index.test.ts +++ b/src/api/index.test.ts @@ -16,6 +16,7 @@ import type { QueueProvider, } from '../providers/index.js' import { type AgentRecord, type AgentStore, createAgentStore } from '../store/agents.js' +import { type AssistantStore, createAssistantStore } from '../store/assistants.js' import { createThreadAttachmentStore, insertThreadAttachmentsInTx } from '../store/attachments.js' import { type ConversationStore, @@ -28,6 +29,7 @@ import { createMailboxStore, type MailboxStore } from '../store/mailboxes.js' import { ENCRYPTION_KEY_BYTES } from '../store/token-crypto.js' import { createWebhookEndpointStore } from '../store/webhook-endpoints.js' import type { AgentsApiDeps } from './agents.js' +import type { AssistantsApiDeps } from './assistants.js' import type { GmailReconcileJob } from './gmail-webhook.js' import { createInboxApi, type InboxApiDeps } from './index.js' import type { WebhooksApiDeps } from './webhooks.js' @@ -73,6 +75,11 @@ function testWebhooksDeps(db: Db): WebhooksApiDeps { } } +/** Build the REQUIRED `assistants` deps (HT-70) for a `createInboxApi` call wired to `db` — a real PGlite-backed `AssistantStore`, matching how `src/composition/root.ts` wires it. */ +function testAssistantsDeps(db: Db): AssistantsApiDeps { + return { store: createAssistantStore(db) } +} + /** A fake `EmailSender` that records every `OutboundEmail` it's asked to send, never fails. */ function createFakeSender(): { sender: EmailSender; sent: OutboundEmail[] } { const sent: OutboundEmail[] = [] @@ -253,6 +260,7 @@ describe('createInboxApi', () => { db: Db store: ConversationStore agentStore: AgentStore + assistantStore: AssistantStore api: (request: Request) => Promise /** Emails recorded by the default fake sender (empty if `overrides.sender` was supplied instead). */ sent: OutboundEmail[] @@ -261,6 +269,7 @@ describe('createInboxApi', () => { await migrate(db) const store = createConversationStore(db) const agentsDeps = testAgentsDeps(db) + const assistantsDeps = testAssistantsDeps(db) const { sender: defaultSender, sent } = createFakeSender() const api = createInboxApi({ store, @@ -271,6 +280,7 @@ describe('createInboxApi', () => { supportAddress: SUPPORT_ADDRESS, agents: agentsDeps, webhooks: testWebhooksDeps(db), + assistants: assistantsDeps, ...(overrides.openTracking !== undefined ? { openTracking: overrides.openTracking } : {}), ...(overrides.gmailPush !== undefined ? { gmailPush: overrides.gmailPush } : {}), ...(overrides.gmailConnect !== undefined ? { gmailConnect: overrides.gmailConnect } : {}), @@ -283,7 +293,14 @@ describe('createInboxApi', () => { } : {}), }) - return { db, store, agentStore: agentsDeps.store, api, sent } + return { + db, + store, + agentStore: agentsDeps.store, + assistantStore: assistantsDeps.store, + api, + sent, + } } // --- auth ------------------------------------------------------------------ @@ -721,6 +738,7 @@ describe('createInboxApi', () => { supportAddress: SUPPORT_ADDRESS, agents: testAgentsDeps(db), webhooks: testWebhooksDeps(db), + assistants: testAssistantsDeps(db), }) const res = await api( @@ -829,6 +847,7 @@ describe('createInboxApi', () => { supportAddress: SUPPORT_ADDRESS, agents: testAgentsDeps(db), webhooks: testWebhooksDeps(db), + assistants: testAssistantsDeps(db), }) const res = await api( @@ -930,6 +949,24 @@ describe('createInboxApi', () => { expect(sent).toHaveLength(0) }) + it('an Idempotency-Key starting with the reserved draft: prefix is 400 validation_failed (HT-70 review fix — a raw reply key could otherwise collide with an engine-owned draft key of the same name)', async () => { + const { store, api, sent } = await freshApi() + const { conversationId } = await store.createConversation(newConversation()) + + const res = await api( + replyPost( + `/api/v1/conversations/${conversationId}/replies`, + { text: 'Hi' }, + { idempotencyKey: 'draft:abc' }, + ), + ) + expect(res.status).toBe(400) + expect(await res.json()).toEqual({ + error: { code: 'validation_failed', message: expect.any(String) }, + }) + expect(sent).toHaveLength(0) + }) + it('leading/trailing whitespace in Idempotency-Key is trimmed before comparison — a whitespace-padded key and its trimmed twin replay the SAME send (one send only)', async () => { const { store, api, sent } = await freshApi() const { conversationId } = await store.createConversation(newConversation()) @@ -1035,6 +1072,7 @@ describe('createInboxApi', () => { supportAddress: SUPPORT_ADDRESS, agents: testAgentsDeps(db), webhooks: testWebhooksDeps(db), + assistants: testAssistantsDeps(db), }) const res = await api( @@ -1948,6 +1986,7 @@ describe('createInboxApi', () => { supportAddress: SUPPORT_ADDRESS, agents: testAgentsDeps(db), webhooks: testWebhooksDeps(db), + assistants: testAssistantsDeps(db), gmailPush: { verifySignature: async () => true, subscription: SUBSCRIPTION, @@ -1980,6 +2019,7 @@ describe('createInboxApi', () => { supportAddress: SUPPORT_ADDRESS, agents: testAgentsDeps(db), webhooks: testWebhooksDeps(db), + assistants: testAssistantsDeps(db), gmailPush: { verifySignature: async () => true, subscription: SUBSCRIPTION, @@ -2116,6 +2156,7 @@ describe('createInboxApi', () => { supportAddress: SUPPORT_ADDRESS, agents: testAgentsDeps(db), webhooks: testWebhooksDeps(db), + assistants: testAssistantsDeps(db), ...(gmailConnect !== undefined ? { gmailConnect } : {}), }) } @@ -2350,6 +2391,7 @@ describe('createInboxApi', () => { supportAddress: SUPPORT_ADDRESS, agents: testAgentsDeps(db), webhooks: testWebhooksDeps(db), + assistants: testAssistantsDeps(db), ...(gmailDisconnect !== undefined ? { gmailDisconnect } : {}), }) } @@ -2457,6 +2499,10 @@ describe('createInboxApi — hardening (Codex review)', () => { store: {} as unknown as WebhooksApiDeps['store'], queue: {} as unknown as WebhooksApiDeps['queue'], } satisfies WebhooksApiDeps, + // Same "never invoked, dummy is fine" posture as the AgentStore above — + // none of these tests exercise an /assistants/* route or the assistant- + // token auth path. + assistants: { store: {} as unknown as AssistantStore } satisfies AssistantsApiDeps, } it('throws at construction on an empty apiToken (fail closed — an empty token would authenticate every request)', () => { diff --git a/src/api/index.ts b/src/api/index.ts index 8fcb022..74564bb 100644 --- a/src/api/index.ts +++ b/src/api/index.ts @@ -33,6 +33,7 @@ import { TRANSPARENT_GIF, verifyViewToken } from '../mail/open-tracking.js' import type { Keyring } from '../mail/reply-token.js' import type { SelfEchoGuardDeps } from '../mail/send.js' import type { BlobStore, EmailSender } from '../providers/index.js' +import type { AssistantRecord } from '../store/assistants.js' import type { ThreadAttachmentStore } from '../store/attachments.js' import type { ConversationStore } from '../store/conversations.js' import { resolveActingAgent } from './acting-agent.js' @@ -55,6 +56,14 @@ import { handleSetAgentPassword, handleSetup, } from './agents.js' +import { authenticateAssistantRequest } from './assistant-auth.js' +import { + type AssistantsApiDeps, + handleCreateAssistant, + handleListAssistants, + handlePatchAssistant, + handleRotateAssistantToken, +} from './assistants.js' import { authenticateRequest } from './auth.js' import { handleDeleteConversation, @@ -66,6 +75,13 @@ import { handlePutTags, handleReply, } from './conversations.js' +import { + type DraftsHandlerDeps, + handleApproveDraft, + handleCreateDraft, + handleDiscardDraft, + handleListDrafts, +} from './drafts.js' import { type GmailConnectDeps, handleGmailConnect, @@ -80,6 +96,7 @@ import { matchGmailPushWebhook, matchOpenTrackingPixel, matchRoute, + type RouteMatch, } from './router.js' import { handleCreateWebhook, @@ -99,6 +116,24 @@ import { */ const MIN_API_TOKEN_LENGTH = 16 +/** + * The Assistant capability gate (HT-70; specs/plugins/substrate-v1.md §3): + * "an assistant may read conversations/threads, create drafts, and create + * notes. It may not send, approve, change status/tags/assignee, touch + * admin surfaces, or read soft-deleted conversations." Enforced at ONE + * point (spec §1's additive-forward rule: "a future scopes system swaps in + * behind the same gate") — a `RouteMatch['kind']` not in this set is + * refused for an Assistant caller, checked once right after routing, + * before any handler runs. Never consulted for a service-Bearer caller + * (unrestricted, as before this feature). + */ +const ASSISTANT_ALLOWED_ROUTE_KINDS: ReadonlySet = new Set([ + 'conversations-list', + 'conversation-item', + 'conversation-note', + 'conversation-draft-create', +]) + /** * Dependencies `createInboxApi` closes over: the HT-17 read paths need only * `store` + `apiToken`; the HT-18 write paths (specs/api/agent-inbox-v1.md @@ -128,6 +163,16 @@ export interface InboxApiDeps { * than duplicating them here). */ agents: AgentsApiDeps + /** + * Assistants (HT-70; specs/plugins/substrate-v1.md §1, §3) — REQUIRED, + * same posture as `agents` above: the module substrate is core AGPL + * surface, free forever, not an absent-by-default feature. Backs the + * Assistants admin API (`src/api/assistants.ts`) AND the second, + * per-Assistant-token credential class the main pipeline below checks + * alongside the service Bearer token (`authenticateAssistantRequest`, + * `src/api/assistant-auth.ts`). + */ + assistants: AssistantsApiDeps /** * Open tracking (spec §4g, v1.1 — HT-32): ABSENT BY DEFAULT — a deliberate * privacy stance, not an unset knob. When present, outbound replies get a @@ -316,8 +361,32 @@ export function createInboxApi(deps: InboxApiDeps): (request: Request) => Promis : apiError(404, 'not_found', 'No such route.') } - if (!authenticateRequest(request, deps.apiToken)) { - return apiError(401, 'unauthorized', 'Missing or invalid credentials.') + // HT-70 (spec §3): a SECOND credential class, checked ALONGSIDE the + // service Bearer token, never replacing it — the service token is tried + // FIRST (unchanged order/behavior for every existing caller), and only + // on a miss is the Authorization header re-parsed as an Assistant + // token. Either success authenticates the request; both misses are the + // SAME generic 401 a caller cannot use to distinguish "no such + // Assistant" from "wrong service token" from "malformed header". + let caller: { kind: 'service' } | { kind: 'assistant'; assistant: AssistantRecord } + if (authenticateRequest(request, deps.apiToken)) { + caller = { kind: 'service' } + } else { + // This await runs BEFORE the response-shaping try below, so a store + // failure here must be contained locally or it escapes as an + // uncontrolled 500 (CodeRabbit #80) — same controlled shape as the + // catch-all, never the host runtime's. + let assistant: Awaited> + try { + assistant = await authenticateAssistantRequest(request, deps.assistants.store) + } catch (err) { + console.error('[inbox-api] assistant auth store failure', err) + return apiError(500, 'server_error', 'Internal server error.') + } + if (assistant === null) { + return apiError(401, 'unauthorized', 'Missing or invalid credentials.') + } + caller = { kind: 'assistant', assistant } } // Everything past auth runs inside a catch-all so no store/serialization @@ -329,6 +398,24 @@ export function createInboxApi(deps: InboxApiDeps): (request: Request) => Promis const url = new URL(request.url) const route = matchRoute(request.method, url.pathname) + // HT-70's ONE capability-enforcement point (spec §3, §1's + // additive-forward rule) — checked AFTER routing (so `not-found`/ + // `method-not-allowed` behave identically for every caller, exactly + // as they did before Assistants existed) but BEFORE any handler runs. + // Never consulted for a service-Bearer caller. + if ( + caller.kind === 'assistant' && + route.kind !== 'not-found' && + route.kind !== 'method-not-allowed' && + !ASSISTANT_ALLOWED_ROUTE_KINDS.has(route.kind) + ) { + return apiError( + 403, + 'forbidden', + 'This Assistant is not permitted to access this endpoint.', + ) + } + switch (route.kind) { case 'not-found': return apiError(404, 'not_found', 'No such route.') @@ -375,9 +462,21 @@ export function createInboxApi(deps: InboxApiDeps): (request: Request) => Promis return await handleDeleteConversation(route.id, { store: deps.store }) case 'conversation-note': + // HT-70 (spec §6): now legal for an Assistant too — the SAME + // route serves both credential classes, distinguished by which + // one actually authenticated this request (the capability gate + // above already refused any OTHER route for an assistant caller, + // so `caller.kind` alone decides the author here). return await handlePostNote(route.id, request, { store: deps.store, supportAddress: deps.supportAddress, + author: + caller.kind === 'assistant' + ? { kind: 'assistant', assistantId: caller.assistant.id } + : { + kind: 'agent', + agentId: (await resolveActingAgent(request, deps.agents.store))?.id ?? null, + }, }) case 'conversation-tags': @@ -396,12 +495,17 @@ export function createInboxApi(deps: InboxApiDeps): (request: Request) => Promis ) case 'conversation-reply': + // Never reachable by an assistant caller (the capability gate + // above already refused it), so this is always a service caller — + // resolveActingAgent's result (possibly null) is HT-70's + // author-identity forward-carry (spec §3), threaded to sendReply. return await handleReply(route.id, request, { store: deps.store, sender: deps.sender, keyring: deps.keyring, mailDomain: deps.mailDomain, supportAddress: deps.supportAddress, + authorAgentId: (await resolveActingAgent(request, deps.agents.store))?.id ?? null, ...(deps.openTracking !== undefined ? { openTracking: deps.openTracking } : {}), ...(deps.selfEchoGuard !== undefined ? { selfEchoGuard: deps.selfEchoGuard } : {}), }) @@ -547,6 +651,84 @@ export function createInboxApi(deps: InboxApiDeps): (request: Request) => Promis await resolveActingAgent(request, deps.agents.store), deps.webhooks, ) + + // --- Assistants (HT-70; specs/modules/substrate-v1.md §3) ---------- + + case 'assistants-list': + return await handleListAssistants( + await resolveActingAgent(request, deps.agents.store), + deps.assistants, + ) + + case 'assistants-create': + return await handleCreateAssistant( + await resolveActingAgent(request, deps.agents.store), + request, + deps.assistants, + ) + + case 'assistant-patch': + return await handlePatchAssistant( + route.id, + await resolveActingAgent(request, deps.agents.store), + request, + deps.assistants, + ) + + case 'assistant-rotate-token': + return await handleRotateAssistantToken( + route.id, + await resolveActingAgent(request, deps.agents.store), + deps.assistants, + ) + + // --- Drafts (HT-70; specs/modules/substrate-v1.md §6) --------------- + + case 'conversation-draft-create': { + const draftsDeps: DraftsHandlerDeps = { + store: deps.store, + sender: deps.sender, + keyring: deps.keyring, + mailDomain: deps.mailDomain, + supportAddress: deps.supportAddress, + ...(deps.openTracking !== undefined ? { openTracking: deps.openTracking } : {}), + ...(deps.selfEchoGuard !== undefined ? { selfEchoGuard: deps.selfEchoGuard } : {}), + } + // Assistant-auth ONLY (spec §6) — a service-Bearer caller reaching + // this route (the capability gate allows it through, since a + // service token is unrestricted) has no Assistant identity to + // attribute the draft to. + if (caller.kind !== 'assistant') { + return apiError(403, 'forbidden', 'Only an Assistant may create a draft.') + } + return await handleCreateDraft(route.id, caller.assistant, request, draftsDeps) + } + + case 'drafts-list': + return await handleListDrafts(request, { store: deps.store }) + + case 'draft-approve': + return await handleApproveDraft( + route.id, + await resolveActingAgent(request, deps.agents.store), + request, + { + store: deps.store, + sender: deps.sender, + keyring: deps.keyring, + mailDomain: deps.mailDomain, + supportAddress: deps.supportAddress, + ...(deps.openTracking !== undefined ? { openTracking: deps.openTracking } : {}), + ...(deps.selfEchoGuard !== undefined ? { selfEchoGuard: deps.selfEchoGuard } : {}), + }, + ) + + case 'draft-discard': + return await handleDiscardDraft( + route.id, + await resolveActingAgent(request, deps.agents.store), + { store: deps.store }, + ) } } catch (err) { console.error('[inbox-api] unhandled error handling request', err) diff --git a/src/api/router.test.ts b/src/api/router.test.ts index 3f6383f..b3d3a41 100644 --- a/src/api/router.test.ts +++ b/src/api/router.test.ts @@ -212,6 +212,82 @@ describe('matchRoute', () => { id: 'abc-123/mailboxes', }) }) + + // --- Assistants (HT-70) ----------------------------------------------- + + it('matches GET/POST /api/v1/assistants', () => { + expect(matchRoute('GET', '/api/v1/assistants')).toEqual({ kind: 'assistants-list' }) + expect(matchRoute('POST', '/api/v1/assistants')).toEqual({ kind: 'assistants-create' }) + expect(matchRoute('DELETE', '/api/v1/assistants')).toEqual({ + kind: 'method-not-allowed', + allow: ['GET', 'POST'], + }) + }) + + it('matches PATCH /api/v1/assistants/{id}, extracting the id', () => { + expect(matchRoute('PATCH', '/api/v1/assistants/abc-123')).toEqual({ + kind: 'assistant-patch', + id: 'abc-123', + }) + expect(matchRoute('GET', '/api/v1/assistants/abc-123')).toEqual({ + kind: 'method-not-allowed', + allow: ['PATCH'], + }) + }) + + it('matches POST /api/v1/assistants/{id}/rotate-token, extracting the id, never falling into assistant-patch', () => { + expect(matchRoute('POST', '/api/v1/assistants/abc-123/rotate-token')).toEqual({ + kind: 'assistant-rotate-token', + id: 'abc-123', + }) + expect(matchRoute('GET', '/api/v1/assistants/abc-123/rotate-token')).toEqual({ + kind: 'method-not-allowed', + allow: ['POST'], + }) + }) + + // --- Drafts (HT-70) ----------------------------------------------------- + + it('matches POST /api/v1/conversations/{id}/drafts, extracting the id', () => { + expect(matchRoute('POST', '/api/v1/conversations/abc-123/drafts')).toEqual({ + kind: 'conversation-draft-create', + id: 'abc-123', + }) + expect(matchRoute('GET', '/api/v1/conversations/abc-123/drafts')).toEqual({ + kind: 'method-not-allowed', + allow: ['POST'], + }) + }) + + it('conversation item route never matches a /drafts suffix (anchored, same as /replies and /notes)', () => { + expect(matchRoute('GET', '/api/v1/conversations/abc-123/drafts')).not.toEqual({ + kind: 'conversation-item', + id: 'abc-123/drafts', + }) + }) + + it('matches GET /api/v1/drafts', () => { + expect(matchRoute('GET', '/api/v1/drafts')).toEqual({ kind: 'drafts-list' }) + expect(matchRoute('POST', '/api/v1/drafts')).toEqual({ + kind: 'method-not-allowed', + allow: ['GET'], + }) + }) + + it('matches POST /api/v1/drafts/{threadId}/approve and .../discard, extracting the id', () => { + expect(matchRoute('POST', '/api/v1/drafts/thread-123/approve')).toEqual({ + kind: 'draft-approve', + id: 'thread-123', + }) + expect(matchRoute('POST', '/api/v1/drafts/thread-123/discard')).toEqual({ + kind: 'draft-discard', + id: 'thread-123', + }) + expect(matchRoute('GET', '/api/v1/drafts/thread-123/approve')).toEqual({ + kind: 'method-not-allowed', + allow: ['POST'], + }) + }) }) describe('matchGmailPushWebhook', () => { diff --git a/src/api/router.ts b/src/api/router.ts index 93debff..8ac6911 100644 --- a/src/api/router.ts +++ b/src/api/router.ts @@ -188,6 +188,56 @@ const WEBHOOK_TEST: RouteDef = { methods: ['POST'], } +// --- Assistants (HT-70; specs/modules/substrate-v1.md §3) ------------------- +// +// Admin-only, acting-Agent header REQUIRED (same conventions as /agents) — +// never reachable by an Assistant's own token (the capability gate in +// `src/api/index.ts` refuses every route not in its allowed set). + +/** `/api/v1/assistants` — list (GET) and create (POST), both admin only — spec §3. */ +const ASSISTANTS_LIST: RouteDef = { + pattern: /^\/api\/v1\/assistants$/, + methods: ['GET', 'POST'], +} + +/** `/api/v1/assistants/{id}/rotate-token` — mint a fresh secret for the SAME assistant id (admin only) — spec §3, POST only. Checked before `ASSISTANT_ITEM` per this file's specific-before-generic convention (`AGENT_PASSWORD`/`AGENT_INVITE` before `AGENT_ITEM`), though the two patterns cannot actually collide (`[^/]+` excludes `/`). */ +const ASSISTANT_ROTATE_TOKEN: RouteDef = { + pattern: /^\/api\/v1\/assistants\/(?[^/]+)\/rotate-token$/, + methods: ['POST'], +} + +/** `/api/v1/assistants/{id}` — patch name/status (admin only) — spec §3, PATCH only (no GET-by-id or DELETE in v1). */ +const ASSISTANT_ITEM: RouteDef = { + pattern: /^\/api\/v1\/assistants\/(?[^/]+)$/, + methods: ['PATCH'], +} + +// --- Drafts (HT-70; specs/plugins/substrate-v1.md §6) ------------------------ + +/** `/api/v1/conversations/{id}/drafts` — an Assistant posts a draft (spec §6), POST only. Anchored like `CONVERSATION_REPLIES`/`CONVERSATION_NOTES` — `CONVERSATION_ITEM`'s `[^/]+$` pattern can never match this `/drafts` suffix. */ +const CONVERSATION_DRAFTS: RouteDef = { + pattern: /^\/api\/v1\/conversations\/(?[^/]+)\/drafts$/, + methods: ['POST'], +} + +/** `/api/v1/drafts` — the cross-conversation `?status=awaiting_review` review queue (spec §6), GET only. */ +const DRAFTS_LIST: RouteDef = { + pattern: /^\/api\/v1\/drafts$/, + methods: ['GET'], +} + +/** `/api/v1/drafts/{threadId}/approve` — approve, optionally with edits (spec §6), POST only. */ +const DRAFT_APPROVE: RouteDef = { + pattern: /^\/api\/v1\/drafts\/(?[^/]+)\/approve$/, + methods: ['POST'], +} + +/** `/api/v1/drafts/{threadId}/discard` — discard (spec §6), POST only. */ +const DRAFT_DISCARD: RouteDef = { + pattern: /^\/api\/v1\/drafts\/(?[^/]+)\/discard$/, + methods: ['POST'], +} + /** Every route this API recognizes, checked in order. */ const ROUTES: readonly RouteDef[] = [ CONVERSATIONS_LIST, @@ -196,6 +246,7 @@ const ROUTES: readonly RouteDef[] = [ CONVERSATION_NOTES, CONVERSATION_TAGS, CONVERSATION_ASSIGNEE, + CONVERSATION_DRAFTS, GMAIL_CONNECT, GMAIL_DISCONNECT, AUTH_PROVIDERS, @@ -212,6 +263,12 @@ const ROUTES: readonly RouteDef[] = [ WEBHOOKS_LIST, WEBHOOK_TEST, WEBHOOK_ITEM, + ASSISTANTS_LIST, + ASSISTANT_ROTATE_TOKEN, + ASSISTANT_ITEM, + DRAFTS_LIST, + DRAFT_APPROVE, + DRAFT_DISCARD, ] /** The outcome of matching a `(method, pathname)` pair against {@link ROUTES}. */ @@ -246,6 +303,14 @@ export type RouteMatch = | { kind: 'webhook-patch'; id: string } | { kind: 'webhook-delete'; id: string } | { kind: 'webhook-test'; id: string } + | { kind: 'assistants-list' } + | { kind: 'assistants-create' } + | { kind: 'assistant-patch'; id: string } + | { kind: 'assistant-rotate-token'; id: string } + | { kind: 'conversation-draft-create'; id: string } + | { kind: 'drafts-list' } + | { kind: 'draft-approve'; id: string } + | { kind: 'draft-discard'; id: string } | { kind: 'method-not-allowed'; allow: string[] } | { kind: 'not-found' } @@ -374,6 +439,12 @@ export function matchRoute(method: string, pathname: string): RouteMatch { if (route === WEBHOOKS_LIST) { return method === 'GET' ? { kind: 'webhooks-list' } : { kind: 'webhooks-create' } } + if (route === ASSISTANTS_LIST) { + return method === 'GET' ? { kind: 'assistants-list' } : { kind: 'assistants-create' } + } + if (route === DRAFTS_LIST) { + return { kind: 'drafts-list' } + } // Every remaining route guarantees a present, non-empty `id` group (per // its `[^/]+` pattern) whenever it matched. @@ -408,6 +479,21 @@ export function matchRoute(method: string, pathname: string): RouteMatch { if (method === 'DELETE') return { kind: 'webhook-delete', id } return { kind: 'webhook-patch', id } } + if (route === CONVERSATION_DRAFTS) { + return { kind: 'conversation-draft-create', id } + } + if (route === DRAFT_APPROVE) { + return { kind: 'draft-approve', id } + } + if (route === DRAFT_DISCARD) { + return { kind: 'draft-discard', id } + } + if (route === ASSISTANT_ROTATE_TOKEN) { + return { kind: 'assistant-rotate-token', id } + } + if (route === ASSISTANT_ITEM) { + return { kind: 'assistant-patch', id } + } if (route === AGENT_ITEM) { if (method === 'GET') return { kind: 'agent-item', id } if (method === 'DELETE') return { kind: 'agent-delete', id } diff --git a/src/api/webhooks.test.ts b/src/api/webhooks.test.ts index 2805fe0..71a2397 100644 --- a/src/api/webhooks.test.ts +++ b/src/api/webhooks.test.ts @@ -16,6 +16,7 @@ import { migrate } from '../db/migrate.js' import type { Keyring } from '../mail/reply-token.js' import type { EmailSender, EnqueueOptions, QueueProvider } from '../providers/index.js' import { type AgentRecord, type AgentStore, createAgentStore } from '../store/agents.js' +import type { AssistantStore } from '../store/assistants.js' import { createConversationStore } from '../store/conversations.js' import { createMailboxStore } from '../store/mailboxes.js' import { ENCRYPTION_KEY_BYTES } from '../store/token-crypto.js' @@ -24,6 +25,7 @@ import { type WebhookEndpointStore, } from '../store/webhook-endpoints.js' import { WEBHOOK_DELIVERY_TOPIC } from '../webhooks/delivery.js' +import type { AssistantsApiDeps } from './assistants.js' import { createInboxApi } from './index.js' const TOKEN = 'test-token-for-the-webhooks-admin-suite' @@ -91,6 +93,7 @@ describe('Webhooks admin API', () => { mailboxStore, }, webhooks: { store: webhookStore, queue }, + assistants: { store: {} as unknown as AssistantStore } satisfies AssistantsApiDeps, }) return { db, agentStore, webhookStore, api, enqueued } } diff --git a/src/auth/assistant-token.test.ts b/src/auth/assistant-token.test.ts new file mode 100644 index 0000000..f2548a4 --- /dev/null +++ b/src/auth/assistant-token.test.ts @@ -0,0 +1,74 @@ +import { describe, expect, it } from 'vitest' +import { + constantTimeHashEquals, + hashAssistantSecret, + mintAssistantToken, + parseAssistantToken, +} from './assistant-token.js' + +const ASSISTANT_ID = '11111111-1111-4111-8111-111111111111' + +describe('mintAssistantToken', () => { + it('mints a token shaped ht_asst__ that round-trips through parseAssistantToken', () => { + const minted = mintAssistantToken(ASSISTANT_ID) + expect(minted.token.startsWith(`ht_asst_${ASSISTANT_ID}_`)).toBe(true) + + const parsed = parseAssistantToken(minted.token) + expect(parsed).not.toBeNull() + expect(parsed?.assistantId).toBe(ASSISTANT_ID) + expect(hashAssistantSecret(parsed?.secret ?? '')).toBe(minted.tokenHash) + }) + + it('mints a different secret (and hash) every call', () => { + const a = mintAssistantToken(ASSISTANT_ID) + const b = mintAssistantToken(ASSISTANT_ID) + expect(a.token).not.toBe(b.token) + expect(a.tokenHash).not.toBe(b.tokenHash) + }) + + it('throws on a non-uuid assistantId', () => { + expect(() => mintAssistantToken('not-a-uuid')).toThrow() + expect(() => mintAssistantToken('')).toThrow() + }) +}) + +describe('parseAssistantToken', () => { + it('is total: never throws, returns null for anything not shaped like our token', () => { + const badInputs = [ + '', + 'ht_asst_', + `ht_asst_${ASSISTANT_ID}`, // missing the trailing _ + `ht_asst_${ASSISTANT_ID}_`, // empty secret + 'ht_asst_not-a-uuid_secret', + 'Bearer sometoken', + `ht_asst_${'1'.repeat(36)}${'X'.repeat(10)}`, // no separator underscore at all + ] + for (const input of badInputs) { + expect(parseAssistantToken(input)).toBeNull() + } + }) + + it('recovers the assistantId by fixed-length slice even when the secret itself contains underscores', () => { + const secretWithUnderscores = 'a_b_c_d_e_f' + const token = `ht_asst_${ASSISTANT_ID}_${secretWithUnderscores}` + const parsed = parseAssistantToken(token) + expect(parsed).toEqual({ assistantId: ASSISTANT_ID, secret: secretWithUnderscores }) + }) + + it('rejects a token whose id segment is not uuid-shaped even if the overall length matches', () => { + const bogusId = 'z'.repeat(36) + const token = `ht_asst_${bogusId}_somesecret` + expect(parseAssistantToken(token)).toBeNull() + }) +}) + +describe('constantTimeHashEquals', () => { + it('true for identical digests, false for a mismatch or a length difference', () => { + const h1 = hashAssistantSecret('secret-a') + const h2 = hashAssistantSecret('secret-a') + const h3 = hashAssistantSecret('secret-b') + expect(constantTimeHashEquals(h1, h2)).toBe(true) + expect(constantTimeHashEquals(h1, h3)).toBe(false) + expect(constantTimeHashEquals(h1, h1.slice(0, -2))).toBe(false) + }) +}) diff --git a/src/auth/assistant-token.ts b/src/auth/assistant-token.ts new file mode 100644 index 0000000..fb455ba --- /dev/null +++ b/src/auth/assistant-token.ts @@ -0,0 +1,128 @@ +/** + * Assistant bearer tokens (HT-70; specs/plugins/substrate-v1.md §3) — + * `ht_asst__`, minted once at creation or rotation and + * shown to the caller exactly that one time. Only a SHA-256 digest of the + * secret part is ever persisted (`AssistantStore.create`/`updateTokenHash`, + * `src/store/assistants.ts`) — this module never hands the plaintext token + * to storage. Verifying a PRESENTED token at request time (parse + row + * lookup + constant-time digest compare) is a separate concern — + * `src/api/assistant-auth.ts`. + * + * ## Why SHA-256, not scrypt + * + * `src/auth/password-hash.ts` uses scrypt for Agent passwords because + * CodeQL's `js/insufficient-password-hash` (and the real threat model) + * rejects a fast hash for a LOW-ENTROPY, human-chosen secret — an offline + * attacker with the hash can brute-force a weak password quickly. An + * Assistant's secret is the opposite case: server-generated, 256 bits of + * CSPRNG entropy, never typed by a human, never reused. A fast digest is + * the right tool here — the spec's own pinned design (§3): "constant-time + * comparison of SHA-256 digests of the secret part." Slowing this down + * with scrypt would only add needless CPU to every authenticated request an + * Assistant makes, for no security benefit a high-entropy secret doesn't + * already have. + * + * ## The id/token knot + * + * Same shape as `src/mail/send.ts`'s `threadId`/`messageId` pair: the token + * embeds the assistant's id, so the id must exist BEFORE the token can be + * minted, but the assistant ROW doesn't exist until it's inserted. The + * caller (`src/api/assistants.ts`) breaks the knot exactly as `sendReply` + * does — generate the id first (`crypto.randomUUID()`), mint the token + * against it, then insert the row with that id explicit + * (`AssistantStore.create`'s optional `id`, `src/store/assistants.ts`). + */ + +import { createHash, randomBytes, timingSafeEqual } from 'node:crypto' + +/** Fixed literal prefix marking a token as one of this module's Assistant tokens. */ +const TOKEN_PREFIX = 'ht_asst_' + +/** Random secret size, in bytes — 256 bits, the module doc's "why SHA-256" rationale rests on this being high-entropy. */ +const SECRET_BYTES = 32 + +/** The uuid-shape check for `assistantId` — mirrors `src/api/uuid.ts`, duplicated locally so this module has no dependency on `src/api/**` (an auth seam should not depend on the HTTP layer; same convention as `src/auth/invite-token.ts`). */ +const UUID_PATTERN = /^[0-9a-fA-F]{8}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{12}$/ + +/** The canonical uuid string length (`8-4-4-4-12` plus four hyphens) — used to recover the id by FIXED-LENGTH slice, not by splitting on `_` (see {@link parseAssistantToken}'s doc comment). */ +const UUID_LENGTH = 36 + +/** The result of {@link mintAssistantToken}. */ +export interface MintedAssistantToken { + /** The full token, shown to the caller ONCE — never stored, never logged. */ + token: string + /** SHA-256 digest (hex) of the secret part — what actually gets persisted (`AssistantStore.create`/`updateTokenHash`). */ + tokenHash: string +} + +/** + * Mint a fresh token for `assistantId` (creation or rotation). STRICT: + * throws if `assistantId` is not uuid-shaped — a caller bug (see the module + * doc's "id/token knot"), not something to silently tolerate, mirroring + * `mintReplyMessageId`/`mintInviteToken`'s "emitting an unverifiable token + * is our bug, fail loud" posture. + */ +export function mintAssistantToken(assistantId: string): MintedAssistantToken { + if (typeof assistantId !== 'string' || !UUID_PATTERN.test(assistantId)) { + throw new Error( + `mintAssistantToken: assistantId must be a uuid (got ${JSON.stringify(assistantId)})`, + ) + } + const secret = randomBytes(SECRET_BYTES).toString('base64url') + return { + token: `${TOKEN_PREFIX}${assistantId}_${secret}`, + tokenHash: hashAssistantSecret(secret), + } +} + +/** SHA-256 digest (hex) of a token's secret part — the one-way function whose output is what {@link AssistantStore} persists and {@link constantTimeHashEquals} compares. */ +export function hashAssistantSecret(secret: string): string { + return createHash('sha256').update(secret).digest('hex') +} + +/** A token's parsed segments — see {@link parseAssistantToken}. */ +export interface ParsedAssistantToken { + assistantId: string + secret: string +} + +/** + * Structurally parse `token` into its assistantId/secret parts. TOTAL — + * never throws; `null` for anything not shaped like + * `ht_asst__` (a hostile or malformed `Authorization` header + * is untrusted input reaching this on every request, so this mirrors + * `parseToken`'s totality bar in `src/mail/reply-token.ts`). + * + * The assistantId is recovered by a FIXED-LENGTH slice (the canonical + * uuid's 36 characters), not by splitting on `_` — a base64url secret can + * itself contain `_`, so a naive split would misparse. A well-formed token + * therefore always has exactly one interpretation: prefix, 36 uuid chars, + * a literal `_`, then the secret (whatever is left, non-empty). + */ +export function parseAssistantToken(token: string): ParsedAssistantToken | null { + if (typeof token !== 'string' || !token.startsWith(TOKEN_PREFIX)) return null + const rest = token.slice(TOKEN_PREFIX.length) + if (rest.length <= UUID_LENGTH + 1) return null + + const assistantId = rest.slice(0, UUID_LENGTH) + if (!UUID_PATTERN.test(assistantId)) return null + if (rest[UUID_LENGTH] !== '_') return null + + const secret = rest.slice(UUID_LENGTH + 1) + if (secret.length === 0) return null + + return { assistantId, secret } +} + +/** + * Constant-time compare of two SHA-256 hex digests. Length-guarded before + * `timingSafeEqual` (which throws on a length mismatch) — same pattern + * `src/api/auth.ts`'s `constantTimeEquals` uses for the service Bearer + * token. + */ +export function constantTimeHashEquals(a: string, b: string): boolean { + const bufA = Buffer.from(a) + const bufB = Buffer.from(b) + if (bufA.length !== bufB.length) return false + return timingSafeEqual(bufA, bufB) +} diff --git a/src/composition/root.ts b/src/composition/root.ts index 957040e..356cd3b 100644 --- a/src/composition/root.ts +++ b/src/composition/root.ts @@ -73,6 +73,7 @@ import { createSupabaseStorageBlobStore } from '../providers/adapters/supabase-s import type { BlobStore } from '../providers/blob.js' import type { QueueMessage, QueueMessageHandler } from '../providers/queue.js' import { createAgentStore } from '../store/agents.js' +import { createAssistantStore } from '../store/assistants.js' import { createConversationStore, createEventOutboxStore, @@ -155,6 +156,7 @@ export async function buildApp( const inboundDeliveryStore = createInboundDeliveryStore(db) const attachmentStore = createThreadAttachmentStore(db) const agentStore = createAgentStore(db) + const assistantStore = createAssistantStore(db) // --- Module substrate (HT-69; specs/modules/substrate-v1.md §4/§5): the // event outbox and webhook endpoint stores. `webhookEndpointStore` reuses @@ -276,6 +278,9 @@ export async function buildApp( // the substrate is core, free forever). `queue` is the SAME // `PostgresQueue` instance every other enqueue in this root shares. webhooks: { store: webhookEndpointStore, queue }, + // Assistants + drafts (HT-70) — CORE, required (same posture as + // `agents` above). + assistants: { store: assistantStore }, // HT-49 review fix: Gmail delivers a sent reply's own copy back into the // SAME mailbox it was sent from, where reconcile would otherwise re-ingest // it as a phantom inbound message (src/mail/send.ts's "The reply token's diff --git a/src/mail/approve-draft.test.ts b/src/mail/approve-draft.test.ts new file mode 100644 index 0000000..ffd1d8d --- /dev/null +++ b/src/mail/approve-draft.test.ts @@ -0,0 +1,346 @@ +import { afterEach, describe, expect, it } from 'vitest' +import { createPgliteDb, type Db } from '../db/client.js' +import { migrate } from '../db/migrate.js' +import type { EmailSender, OutboundEmail } from '../providers/index.js' +import { + type ConversationStore, + createConversationStore, + type StoredConversation, + type StoredThread, +} from '../store/conversations.js' +import { approveDraft } from './approve-draft.js' +import type { Keyring, SigningKey } from './reply-token.js' + +const KEY_A: SigningKey = { keyId: 'k1', secret: 'secret-A-high-entropy-0123456789abcdef' } +const keyring: Keyring = { current: KEY_A } +const mailDomain = 'mail.example.test' +const supportAddress = 'support@example.test' + +function fakeSender(): EmailSender & { sent: OutboundEmail[] } { + const sent: OutboundEmail[] = [] + return { + sent, + maxSendMs: 30_000, + async send(email) { + sent.push(email) + return { providerMessageId: 'provider-1' } + }, + } +} + +function failingSender(): EmailSender { + return { + maxSendMs: 30_000, + async send() { + throw new Error('boom: provider unreachable') + }, + } +} + +async function createTestAssistant(db: Db): Promise { + const [row] = await db.query<{ id: string }>( + `INSERT INTO assistants (name, module, token_hash) VALUES ('Draft Bot', 'draft-reply', 'hash') RETURNING id`, + ) + return row.id +} + +async function createTestAgent(db: Db): Promise { + const [row] = await db.query<{ id: string }>( + `INSERT INTO agents (email, name, role, status) VALUES ('agent@example.test', 'Agent', 'agent', 'active') RETURNING id`, + ) + return row.id +} + +describe('approveDraft', () => { + let db: Db | undefined + + afterEach(async () => { + await db?.close() + db = undefined + }) + + async function freshStore(): Promise<{ db: Db; store: ConversationStore }> { + db = await createPgliteDb() + await migrate(db) + return { db, store: createConversationStore(db) } + } + + async function seedConversationWithDraft( + store: ConversationStore, + assistantId: string, + bodyText = 'Suggested reply.', + bodyHtml?: string, + ): Promise<{ + conversation: StoredConversation & { threads: StoredThread[] } + draftThreadId: string + }> { + const { conversationId } = await store.createConversation({ + subject: 'Help with my order', + customerEmail: 'customer@example.test', + firstMessage: { + direction: 'inbound', + messageId: '', + fromAddress: 'customer@example.test', + bodyText: 'Where is my order?', + }, + }) + const draft = await store.appendDraft(conversationId, { + assistantId, + bodyText, + ...(bodyHtml !== undefined ? { bodyHtml } : {}), + fromAddress: supportAddress, + idempotencyKey: `draft-${conversationId}`, + }) + if (!draft.ok) throw new Error('unreachable') + const conversation = await store.getConversation(conversationId, { includeDeleted: false }) + if (conversation === null) throw new Error('unreachable') + return { conversation, draftThreadId: draft.threadId } + } + + it('happy path: mints a token for the draft thread id, derives the envelope, resolves, and delivers', async () => { + const { store, db: testDb } = await freshStore() + const assistantId = await createTestAssistant(testDb) + const agentId = await createTestAgent(testDb) + const { conversation, draftThreadId } = await seedConversationWithDraft(store, assistantId) + const sender = fakeSender() + + const result = await approveDraft( + { conversation, draftThreadId, resolvedByAgentId: agentId }, + { store, sender, keyring, mailDomain }, + ) + + expect(result.ok).toBe(true) + if (!result.ok) throw new Error('unreachable') + expect(result.threadId).toBe(draftThreadId) + expect(result.delivery).toBe('sent') + // Minted for the DRAFT's existing thread id, not a fresh one. + expect(result.messageId).toContain(`.${conversation.id}.${draftThreadId}.`) + + expect(sender.sent).toHaveLength(1) + expect(sender.sent[0]).toMatchObject({ + to: [conversation.customerEmail], + subject: 'Re: Help with my order', + from: supportAddress, + inReplyTo: '', + text: 'Suggested reply.', + }) + expect(sender.sent[0].references).toEqual([ + '', + result.messageId, + ]) + + const stored = await store.getConversation(conversation.id, { includeDeleted: false }) + const resolvedThread = stored?.threads.find((t) => t.id === draftThreadId) + expect(resolvedThread).toMatchObject({ + draftStatus: 'approved', + deliveryStatus: 'sent', + approvedByAgentId: agentId, + draftEdited: false, + }) + }) + + it('approve with edits: the sent body reflects the override and draft_edited is recorded true', async () => { + const { store, db: testDb } = await freshStore() + const assistantId = await createTestAssistant(testDb) + const agentId = await createTestAgent(testDb) + const { conversation, draftThreadId } = await seedConversationWithDraft( + store, + assistantId, + 'Original body.', + ) + const sender = fakeSender() + + const result = await approveDraft( + { + conversation, + draftThreadId, + resolvedByAgentId: agentId, + edit: { bodyText: 'Edited by the Agent before sending.' }, + }, + { store, sender, keyring, mailDomain }, + ) + + expect(result.ok).toBe(true) + expect(sender.sent[0].text).toBe('Edited by the Agent before sending.') + + const stored = await store.getConversation(conversation.id, { includeDeleted: false }) + const resolvedThread = stored?.threads.find((t) => t.id === draftThreadId) + expect(resolvedThread).toMatchObject({ + draftEdited: true, + bodyText: 'Edited by the Agent before sending.', + }) + }) + + it('HT-32 pixel injection persists the pixel-injected bodyHtml on the row, even with NO Agent edit (draft_edited stays false)', async () => { + const { store, db: testDb } = await freshStore() + const assistantId = await createTestAssistant(testDb) + const agentId = await createTestAgent(testDb) + const { conversation, draftThreadId } = await seedConversationWithDraft( + store, + assistantId, + 'Text body.', + '

Html body.

', + ) + const sender = fakeSender() + const openTracking = { publicBaseUrl: 'https://desk.example.test' } + + const result = await approveDraft( + { conversation, draftThreadId, resolvedByAgentId: agentId }, + { store, sender, keyring, mailDomain, openTracking }, + ) + + expect(result.ok).toBe(true) + expect(sender.sent[0].html).toContain('Html body.

') + + const stored = await store.getConversation(conversation.id, { includeDeleted: false }) + const resolvedThread = stored?.threads.find((t) => t.id === draftThreadId) + // The PERSISTED row already carries the pixel — what a delivery-worker + // retry would rebuild from — even though no Agent edit was submitted. + expect(resolvedThread?.bodyHtml).toContain(' { + const { store, db: testDb } = await freshStore() + const assistantId = await createTestAssistant(testDb) + const agentId = await createTestAgent(testDb) + const { conversation, draftThreadId } = await seedConversationWithDraft( + store, + assistantId, + 'Text body.', + '

Html body.

', + ) + const sender = fakeSender() + + await approveDraft( + { conversation, draftThreadId, resolvedByAgentId: agentId }, + { store, sender, keyring, mailDomain }, + ) + + const stored = await store.getConversation(conversation.id, { includeDeleted: false }) + const resolvedThread = stored?.threads.find((t) => t.id === draftThreadId) + expect(resolvedThread?.bodyHtml).toBe('

Html body.

') + }) + + it('not-a-draft: an unknown draftThreadId (not present on the conversation) is refused before any mint/write', async () => { + const { store, db: testDb } = await freshStore() + const assistantId = await createTestAssistant(testDb) + const agentId = await createTestAgent(testDb) + const { conversation } = await seedConversationWithDraft(store, assistantId) + const sender = fakeSender() + + const result = await approveDraft( + { + conversation, + draftThreadId: '00000000-0000-4000-8000-000000000000', + resolvedByAgentId: agentId, + }, + { store, sender, keyring, mailDomain }, + ) + + expect(result).toEqual({ ok: false, reason: 'not-a-draft' }) + expect(sender.sent).toHaveLength(0) + }) + + it('not-a-draft: a draft already resolved (race between the snapshot and this call) is refused', async () => { + const { store, db: testDb } = await freshStore() + const assistantId = await createTestAssistant(testDb) + const agentId = await createTestAgent(testDb) + const { conversation, draftThreadId } = await seedConversationWithDraft(store, assistantId) + + // Resolve it out from under the snapshot the test is about to pass in. + await store.resolveDraft({ + action: 'discard', + threadId: draftThreadId, + resolvedByAgentId: agentId, + }) + + const sender = fakeSender() + const result = await approveDraft( + { conversation, draftThreadId, resolvedByAgentId: agentId }, + { store, sender, keyring, mailDomain }, + ) + + expect(result).toEqual({ ok: false, reason: 'not-a-draft' }) + expect(sender.sent).toHaveLength(0) + }) + + it('conversation-deleted: a STALE conversation snapshot (pre-delete) is still refused — resolveDraft re-checks fresh, under lock (HT-70 TOCTOU fix, Codex)', async () => { + const { store, db: testDb } = await freshStore() + const assistantId = await createTestAssistant(testDb) + const agentId = await createTestAgent(testDb) + const { conversation, draftThreadId } = await seedConversationWithDraft(store, assistantId) + + // The STALE snapshot — captured BEFORE the "concurrent" delete below, + // exactly what a caller's own preflight read would have seen. + const staleConversation = conversation + + // The "concurrent" delete: committed AFTER the snapshot was taken. + await store.deleteConversation(conversation.id) + + const sender = fakeSender() + const result = await approveDraft( + { conversation: staleConversation, draftThreadId, resolvedByAgentId: agentId }, + { store, sender, keyring, mailDomain }, + ) + + expect(result).toEqual({ ok: false, reason: 'conversation-deleted' }) + expect(sender.sent).toHaveLength(0) + + const stored = await store.getConversation(conversation.id, { includeDeleted: true }) + const draftThread = stored?.threads.find((t) => t.id === draftThreadId) + expect(draftThread?.draftStatus).toBe('awaiting_review') + expect(draftThread?.deliveryStatus).toBeNull() + expect(draftThread?.messageId).toBeNull() + }) + + it('conversation-spam: a STALE conversation snapshot (pre-spam-mark) is still refused — resolveDraft re-checks fresh, under lock (HT-70 TOCTOU fix, Codex)', async () => { + const { store, db: testDb } = await freshStore() + const assistantId = await createTestAssistant(testDb) + const agentId = await createTestAgent(testDb) + const { conversation, draftThreadId } = await seedConversationWithDraft(store, assistantId) + + const staleConversation = conversation + await testDb.query("UPDATE conversations SET status = 'spam' WHERE id = $1", [conversation.id]) + + const sender = fakeSender() + const result = await approveDraft( + { conversation: staleConversation, draftThreadId, resolvedByAgentId: agentId }, + { store, sender, keyring, mailDomain }, + ) + + expect(result).toEqual({ ok: false, reason: 'conversation-spam' }) + expect(sender.sent).toHaveLength(0) + + const stored = await store.getConversation(conversation.id, { includeDeleted: false }) + const draftThread = stored?.threads.find((t) => t.id === draftThreadId) + expect(draftThread?.draftStatus).toBe('awaiting_review') + expect(draftThread?.deliveryStatus).toBeNull() + }) + + it('send-failed: the provider rejects the message — the row is left approved/failed, not resent, and the failure is reported', async () => { + const { store, db: testDb } = await freshStore() + const assistantId = await createTestAssistant(testDb) + const agentId = await createTestAgent(testDb) + const { conversation, draftThreadId } = await seedConversationWithDraft(store, assistantId) + + const result = await approveDraft( + { conversation, draftThreadId, resolvedByAgentId: agentId }, + { store, sender: failingSender(), keyring, mailDomain }, + ) + + expect(result.ok).toBe(false) + if (result.ok) throw new Error('unreachable') + expect(result.reason).toBe('send-failed') + if (result.reason !== 'send-failed') throw new Error('unreachable') + expect(result.persistedStatus).toBe('failed') + + const stored = await store.getConversation(conversation.id, { includeDeleted: false }) + const resolvedThread = stored?.threads.find((t) => t.id === draftThreadId) + // Already resolved to 'approved' — approval is a state transition, + // distinct from whatever the SEND attempt does afterward. + expect(resolvedThread?.draftStatus).toBe('approved') + expect(resolvedThread?.deliveryStatus).toBe('failed') + }) +}) diff --git a/src/mail/approve-draft.ts b/src/mail/approve-draft.ts new file mode 100644 index 0000000..9643726 --- /dev/null +++ b/src/mail/approve-draft.ts @@ -0,0 +1,253 @@ +/** + * Draft-approval orchestration (HT-70; specs/plugins/substrate-v1.md §6 + * "What approval actually does") — the write path behind + * `POST /api/v1/drafts/{threadId}/approve` (`src/api/drafts.ts`). + * + * `sendReply` (`./send.ts`) CANNOT be reused here — it mints and INSERTs a + * *new* thread row and has no entry point for resolving an EXISTING one + * (spec §6's own framing). This module instead performs, in one + * transaction via `ConversationStore.resolveDraft`, the same derivations + * `sendReply` performs pre-insert, then hands off to the EXISTING, + * UNCHANGED delivery machinery (`attemptDeliveryOfClaimedThread` — the + * exact function `sendReply`'s own keyed-retry path and the delivery + * worker both call), so a draft approval and an ordinary keyed reply share + * identical send/lease/retry/self-echo semantics from that point on. + * + * Steps (spec §6, verbatim order): + * + * 1. Mint the reply token + `Message-ID` for the draft's EXISTING thread id + * (`mintReplyMessageId`, `specs/mail/threading.md` §2a — same mint, same + * key rotation `sendReply` uses). + * 2. Derive the envelope exactly per agent-inbox-v1 §4a — recipient/subject + * from the conversation, `In-Reply-To`/`References` from the latest + * inbound thread, with the minted id as the FINAL `References` entry + * (the HT-49 rule) — via `deriveReplyHeaders` (`./reply-headers.ts`), + * the SAME function `handleReply` uses, so the two paths can never + * drift. + * 3. HT-32 pixel injection iff configured — byte-identical mail when + * absent, exactly like `sendReply`. See the "persisted body" note below + * for why this can change what gets written even on an UNEDITED + * approval. + * 4. `ConversationStore.resolveDraft` — one atomic write: message id + + * envelope snapshot, `draft_status → 'approved'`, + * `delivery_status → 'pending'`, approving-Agent audit fields. + * 5. Hand off to the EXISTING delivery path: claim the row + * (`ConversationStore.claimThreadForDelivery`) then + * `attemptDeliveryOfClaimedThread` — unchanged, not modified by this + * ticket. + * + * ## The persisted body vs. the `draft_edited` audit flag + * + * These are deliberately DECOUPLED (see `ResolveDraftInput`'s doc comment, + * `src/store/conversations.ts`). The persisted `body_html` this function + * asks `resolveDraft` to write is the pixel-injected version whenever + * HT-32 is configured — matching `sendReply`'s own "injection happens + * BEFORE persist, so every retry (which rebuilds from the stored row) + * carries the same pixel" invariant, since `attemptDeliveryOfClaimedThread` + * always sends whatever is currently persisted, never a value this + * function could pass around it. But `draft_edited` (spec §2: "did the + * approving AGENT change the body before sending") reflects ONLY whether + * the caller's `input.edit` was actually submitted — pixel injection with + * no Agent edit at all still records `edited: false`, an honest audit + * trail rather than a false positive. + */ + +import type { EmailSender } from '../providers/index.js' +import type { + ConversationStore, + SendEnvelope, + StoredConversation, + StoredThread, +} from '../store/conversations.js' +import { injectTrackingPixel, mintViewToken, pixelUrlFor } from './open-tracking.js' +import { deriveReplyHeaders } from './reply-headers.js' +import { type Keyring, mintReplyMessageId } from './reply-token.js' +import { + assertLeaseExceedsSenderBound, + attemptDeliveryOfClaimedThread, + DEFAULT_LEASE_MS, + type SelfEchoGuardDeps, +} from './send.js' + +/** Dependencies {@link approveDraft} needs — the same shape `SendReplyDeps` uses, minus `mailDomain`'s sibling fields this function doesn't need (no `from` derivation — the draft's own `from_address`, set at draft-creation time to the deployment's support address, is already correct and untouched by approval). */ +export interface ApproveDraftDeps { + store: ConversationStore + sender: EmailSender + keyring: Keyring + mailDomain: string + /** HT-32 open tracking — ABSENT BY DEFAULT, same posture as `SendReplyDeps.openTracking`. */ + openTracking?: { publicBaseUrl: string } + /** See `SendReplyDeps.selfEchoGuard`. ABSENT BY DEFAULT. */ + selfEchoGuard?: SelfEchoGuardDeps +} + +/** Input to {@link approveDraft}. */ +export interface ApproveDraftInput { + /** + * The draft's conversation, WITH every current thread. The API layer + * (`src/api/drafts.ts`) already loaded this via + * `ConversationStore.getConversationByThreadId` to check soft-delete/spam + * (spec §6) before calling here — re-fetching it would be redundant I/O + * for a value this function needs anyway (envelope derivation, and + * finding the draft's own row). + */ + conversation: StoredConversation & { threads: StoredThread[] } + draftThreadId: string + resolvedByAgentId: string + /** + * The approving Agent's optional body override ("approve with edits", + * spec §6) — RAW, exactly as the API layer parsed it from the request. + * `undefined` means no override was submitted; this is what drives the + * `draft_edited` audit flag (see the module doc), independent of + * whether HT-32 pixel injection still changes the persisted `bodyHtml`. + */ + edit?: { bodyText?: string; bodyHtml?: string } +} + +/** + * The outcome of {@link approveDraft} — mirrors `SendReplyResult`'s shape + * (`./send.ts`) for the outcomes this function shares with it (a claimed + * row's delivery attempt can fail or race exactly the same way a keyed + * reply retry's can), plus: + * - `not-a-draft` — no matching `awaiting_review` row (unknown id, already + * resolved, or a genuine race between the API layer's snapshot and this + * call landing between the lock and the thread UPDATE). + * - `conversation-deleted` / `conversation-spam` (HT-70 review fix, Codex) + * — `ConversationStore.resolveDraft`'s locked, authoritative re-check + * found the conversation deleted or marked spam AT WRITE TIME, regardless + * of what `input.conversation` (the API layer's own, possibly-stale + * snapshot) said. The draft row is left completely untouched. + */ +export type ApproveDraftResult = + | { ok: true; threadId: string; messageId: string; delivery: 'sent' } + | { ok: false; reason: 'not-a-draft' } + | { ok: false; reason: 'conversation-deleted' } + | { ok: false; reason: 'conversation-spam' } + | { ok: false; reason: 'retry-in-progress' } + | { + ok: false + reason: 'send-failed' + threadId: string + messageId: string + persistedStatus: 'failed' | 'pending' + } + +export async function approveDraft( + input: ApproveDraftInput, + deps: ApproveDraftDeps, +): Promise { + const { store, sender, keyring, mailDomain } = deps + + const draftThread = input.conversation.threads.find((t) => t.id === input.draftThreadId) + if (draftThread === undefined || draftThread.draftStatus !== 'awaiting_review') { + return { ok: false, reason: 'not-a-draft' } + } + + // Step 1: mint for the draft's EXISTING thread id — never a fresh one. + const messageId = mintReplyMessageId( + { conversationId: input.conversation.id, threadId: input.draftThreadId, mailDomain }, + keyring, + ) + + // Step 2: same derivation handleReply uses (src/api/conversations.ts). + // The draft's own row contributes nothing to this scan (its messageId is + // still null pre-approval), so no special-casing is needed to exclude it. + const { + subject, + inReplyTo, + references: ancestorReferences, + } = deriveReplyHeaders(input.conversation) + // HT-49: append this reply's own minted messageId as the FINAL References + // entry — identical rule to send.ts's sendReply. + const references = [...(ancestorReferences ?? []), messageId] + + // Step 3: HT-32 pixel injection, iff configured — only the HTML body is + // ever touched; a text-only draft gets no fabricated HTML part, matching + // sendReply's own behavior. + const overrideHtml = input.edit?.bodyHtml + const bodyHtmlBeforePixel = overrideHtml ?? draftThread.bodyHtml ?? undefined + const finalBodyHtml = + deps.openTracking !== undefined && bodyHtmlBeforePixel !== undefined + ? injectTrackingPixel( + bodyHtmlBeforePixel, + pixelUrlFor(deps.openTracking.publicBaseUrl, mintViewToken(input.draftThreadId, keyring)), + ) + : bodyHtmlBeforePixel + + // Only ask resolveDraft to WRITE a new body_html when it actually differs + // from what is already stored (an Agent override, or pixel injection) — + // an unedited, un-pixel'd approval leaves body_html completely untouched + // (COALESCE keeps the existing value either way; this is a value + // decision to avoid a no-op write, not a correctness requirement). + const bodyHtmlChanged = finalBodyHtml !== (draftThread.bodyHtml ?? undefined) + + const sendEnvelope: SendEnvelope = { + to: [input.conversation.customerEmail], + subject, + references, + } + + const editForResolve: { bodyText?: string; bodyHtml?: string } = {} + if (input.edit?.bodyText !== undefined) { + editForResolve.bodyText = input.edit.bodyText + } + if (bodyHtmlChanged && finalBodyHtml !== undefined) { + editForResolve.bodyHtml = finalBodyHtml + } + + // Step 4: one atomic write (message id + envelope + draft_status + + // delivery_status + audit fields) — also fires draft.resolved (spec §4) + // in the SAME transaction, inside the store. + const resolved = await store.resolveDraft({ + action: 'approve', + threadId: input.draftThreadId, + resolvedByAgentId: input.resolvedByAgentId, + messageId, + sendEnvelope, + inReplyTo: inReplyTo ?? null, + edit: editForResolve, + edited: input.edit !== undefined, + }) + if (resolved === 'conversation-deleted') { + return { ok: false, reason: 'conversation-deleted' } + } + if (resolved === 'conversation-spam') { + return { ok: false, reason: 'conversation-spam' } + } + if (resolved === null) { + // A race: resolved by someone else (or was never a draft) between the + // API layer's snapshot and this write. Nothing was persisted that needs + // undoing — resolveDraft's UPDATE simply matched zero rows. + return { ok: false, reason: 'not-a-draft' } + } + + // Step 5: hand off to the EXISTING, UNCHANGED delivery path — the same + // claim + attemptDeliveryOfClaimedThread pair sendReply's own keyed-retry + // branch uses. + assertLeaseExceedsSenderBound(sender, DEFAULT_LEASE_MS) + const claimed = await store.claimThreadForDelivery(resolved.id, DEFAULT_LEASE_MS) + if (claimed === null) { + // Same two-reasons disambiguation as sendReply's own claim-failure + // handling (./send.ts) — re-read to tell "someone else is already + // sending it" from "it already sent." + const current = await store.getConversation(resolved.conversationId, { + includeDeleted: false, + }) + const currentThread = current?.threads.find((t) => t.id === resolved.id) + if (currentThread?.deliveryStatus === 'sent') { + return { + ok: true, + threadId: currentThread.id, + messageId: currentThread.messageId as string, + delivery: 'sent', + } + } + return { ok: false, reason: 'retry-in-progress' } + } + + return attemptDeliveryOfClaimedThread(claimed, { + store, + sender, + selfEchoGuard: deps.selfEchoGuard, + }) +} diff --git a/src/mail/draft-equivalence.test.ts b/src/mail/draft-equivalence.test.ts new file mode 100644 index 0000000..fad273c --- /dev/null +++ b/src/mail/draft-equivalence.test.ts @@ -0,0 +1,265 @@ +/** + * THE acceptance-bar test for HT-70 (specs/plugins/substrate-v1.md §6's + * closing paragraph, CHARTER.md invariant #5's mail-semantics-equivalence + * rule): the wire-level RFC 5322 output of (assistant draft → unedited + * approve → delivery) must equal `sendReply`'s output for the same + * conversation state and body, modulo Message-ID token randomness — same + * headers, same References chain, same body handling, same pixel behavior + * in both configs. + * + * Modeled on `src/providers/adapters/gmail/mime.test.ts` — the existing + * wire-level contract test for `sendReply` itself — reusing the exact same + * `buildRawMessage` (`src/providers/adapters/gmail/mime.ts`) to turn each + * path's captured `OutboundEmail` into actual RFC 5322 bytes, then diffing + * those bytes directly rather than comparing structured fields (a + * byte-for-byte comparison is the strongest form of "these are the same + * mail" a test can assert). + * + * The two paths necessarily mint DIFFERENT Message-IDs (different + * `threadId`s — `sendReply` mints a fresh one per call; `approveDraft` + * mints for the draft's own, already-existing thread id): the messageId + * itself is deliberately excluded from the comparison ("modulo Message-ID + * token randomness", spec §6), by substituting a shared placeholder for + * each path's own token everywhere it appears (including as the final + * References entry — the HT-49 rule applies identically to both paths). + */ + +import { afterEach, describe, expect, it } from 'vitest' +import { createPgliteDb, type Db } from '../db/client.js' +import { migrate } from '../db/migrate.js' +import { buildRawMessage } from '../providers/adapters/gmail/mime.js' +import type { EmailSender, OutboundEmail } from '../providers/index.js' +import { type ConversationStore, createConversationStore } from '../store/conversations.js' +import { approveDraft } from './approve-draft.js' +import { deriveReplyHeaders } from './reply-headers.js' +import type { Keyring, SigningKey } from './reply-token.js' +import { sendReply } from './send.js' + +const KEY_A: SigningKey = { keyId: 'k1', secret: 'secret-A-high-entropy-0123456789abcdef' } +const keyring: Keyring = { current: KEY_A } +const mailDomain = 'mail.example.test' +const supportAddress = 'support@example.test' +const BODY_TEXT = 'Thanks for reaching out — here is the update on your order.' +const BODY_HTML = '

Thanks for reaching out — here is the update on your order.

' + +function fakeSender(): EmailSender & { sent: OutboundEmail[] } { + const sent: OutboundEmail[] = [] + return { + sent, + maxSendMs: 30_000, + async send(email) { + sent.push(email) + return { providerMessageId: 'provider-1' } + }, + } +} + +async function createTestAssistant(db: Db): Promise { + const [row] = await db.query<{ id: string }>( + `INSERT INTO assistants (name, module, token_hash) VALUES ('Draft Bot', 'draft-reply', 'hash') RETURNING id`, + ) + return row.id +} + +async function createTestAgent(db: Db): Promise { + const [row] = await db.query<{ id: string }>( + `INSERT INTO agents (email, name, role, status) VALUES ('agent@example.test', 'Agent', 'agent', 'active') RETURNING id`, + ) + return row.id +} + +/** Seed a conversation with exactly one inbound message — the ancestor every reply in this suite answers. */ +async function seedConversation(store: ConversationStore, customerEmail: string) { + return store.createConversation({ + subject: 'Help with my order', + customerEmail, + firstMessage: { + direction: 'inbound', + messageId: '', + fromAddress: customerEmail, + bodyText: 'Please help, my order is late.', + }, + }) +} + +/** + * Normalize away the sources of PER-CALL randomness an `OutboundEmail` + * carries that are not part of the equivalence claim — done at THIS layer + * (before `buildRawMessage`'s base64 body encoding), not on the raw wire + * text after the fact: a placeholder substituted into already-base64-encoded + * bytes would only line up with the original if it happened to share the + * source string's exact byte length AND land on a 3-byte encoding boundary, + * neither of which is guaranteed — normalizing the plaintext first and + * THEN encoding sidesteps that entirely. + * + * 1. The minted Message-ID token — `sendReply` and `approveDraft` + * necessarily mint against different `threadId`s (different + * conversations), so their tokens, and every occurrence of the LATTER + * as the final `References` entry (HT-49), can never match by + * construction. "Modulo Message-ID token randomness" (spec §6) is the + * whole point of this substitution. + * 2. The HT-32 pixel's view token, embedded in the HTML body + * (`v...` — `src/mail/open-tracking.ts`), which is + * likewise threadId-derived and therefore path-specific. + */ +function normalizeOutboundEmail(email: OutboundEmail): OutboundEmail { + return { + ...email, + messageId: '', + references: email.references?.map((ref) => (ref === email.messageId ? '' : ref)), + html: email.html?.replace( + /v\.[A-Za-z0-9_-]+\.[A-Za-z0-9_-]+\.[A-Za-z0-9_-]+\.gif/g, + '.gif', + ), + } +} + +/** + * Strip `mimetext`'s MIME multipart boundary from an already-built raw + * message — a fresh random string EVERY `buildRawMessage` call, even for + * two calls on the byte-identical `OutboundEmail`; not an engine-controlled + * value and not part of the mail-semantics equivalence this test asserts. + * Applied AFTER `buildRawMessage` (unlike {@link normalizeOutboundEmail}'s + * pre-encoding substitutions) because the boundary is generated BY that + * call, not present in its input. + */ +function normalizeBoundary(raw: string): string { + const boundary = /boundary=(\S+)/.exec(raw)?.[1] + return boundary === undefined ? raw : raw.split(boundary).join('') +} + +/** The full normalize-then-build pipeline both equivalence assertions below use. */ +function normalizedRawMessage(email: OutboundEmail): string { + return normalizeBoundary(buildRawMessage(normalizeOutboundEmail(email))) +} + +/** Run both paths (sendReply vs. assistant-draft-then-approve) against fresh, otherwise-identical conversations, and return each path's captured OutboundEmail. */ +async function runBothFlows(openTracking?: { + publicBaseUrl: string +}): Promise<{ sentA: OutboundEmail; sentB: OutboundEmail; db: Db }> { + const db = await createPgliteDb() + await migrate(db) + const store = createConversationStore(db) + const assistantId = await createTestAssistant(db) + const agentId = await createTestAgent(db) + + // --- Path A: sendReply, the ordinary Agent-reply path --- + const senderA = fakeSender() + const { conversationId: convA } = await seedConversation(store, 'customer@example.test') + const conversationA = await store.getConversation(convA, { includeDeleted: false }) + if (conversationA === null) throw new Error('unreachable: just-created conversation missing') + const headersA = deriveReplyHeaders(conversationA) + const resultA = await sendReply( + { + conversationId: conversationA.id, + from: supportAddress, + to: [conversationA.customerEmail], + subject: headersA.subject, + text: BODY_TEXT, + html: BODY_HTML, + inReplyTo: headersA.inReplyTo, + references: headersA.references, + }, + { + store, + sender: senderA, + keyring, + mailDomain, + ...(openTracking !== undefined ? { openTracking } : {}), + }, + ) + if (!resultA.ok) throw new Error(`sendReply failed: ${JSON.stringify(resultA)}`) + + // --- Path B: assistant posts a draft with the SAME body -> unedited approve -> delivery --- + const senderB = fakeSender() + const { conversationId: convB } = await seedConversation(store, 'customer@example.test') + const draftResult = await store.appendDraft(convB, { + assistantId, + bodyText: BODY_TEXT, + bodyHtml: BODY_HTML, + fromAddress: supportAddress, + idempotencyKey: 'draft-equivalence-1', + }) + if (!draftResult.ok) throw new Error(`appendDraft failed: ${JSON.stringify(draftResult)}`) + const conversationB = await store.getConversation(convB, { includeDeleted: false }) + if (conversationB === null) throw new Error('unreachable: just-created conversation missing') + + const resultB = await approveDraft( + { + conversation: conversationB, + draftThreadId: draftResult.threadId, + resolvedByAgentId: agentId, + // No `edit` — an UNEDITED approve, per the spec's acceptance bar. + }, + { + store, + sender: senderB, + keyring, + mailDomain, + ...(openTracking !== undefined ? { openTracking } : {}), + }, + ) + if (!resultB.ok) throw new Error(`approveDraft failed: ${JSON.stringify(resultB)}`) + + return { sentA: senderA.sent[0], sentB: senderB.sent[0], db } +} + +describe('draft-approval vs. sendReply: wire-level equivalence (HT-70, spec §6)', () => { + let dbToClose: Db | undefined + + afterEach(async () => { + await dbToClose?.close() + dbToClose = undefined + }) + + it('produces byte-identical RFC 5322 output modulo the Message-ID token, with HT-32 pixel injection OFF', async () => { + const { sentA, sentB, db } = await runBothFlows() + dbToClose = db + + const rawA = normalizedRawMessage(sentA) + const rawB = normalizedRawMessage(sentB) + + expect(rawA).toBe(rawB) + // Sanity: the equivalence isn't vacuous — both are real, non-empty + // replies, and the pixel is genuinely absent from each pre-encoding body. + expect(rawA).toContain(`Subject:`) + expect(sentA.html).not.toContain(' { + const openTracking = { publicBaseUrl: 'https://desk.example.test' } + const { sentA, sentB, db } = await runBothFlows(openTracking) + dbToClose = db + + const rawA = normalizedRawMessage(sentA) + const rawB = normalizedRawMessage(sentB) + + expect(rawA).toBe(rawB) + // Sanity: pixel injection actually fired on BOTH paths (not vacuously + // equal empties) — checked on the pre-encoding OutboundEmail.html, since + // buildRawMessage base64-encodes the body (an ` { + const { sentA, sentB, db } = await runBothFlows() + dbToClose = db + + expect(sentA.references).toEqual(['', sentA.messageId]) + expect(sentB.references).toEqual(['', sentB.messageId]) + expect(sentA.inReplyTo).toBe('') + expect(sentB.inReplyTo).toBe('') + }) + + it('both paths derive the identical subject, from, and to', async () => { + const { sentA, sentB, db } = await runBothFlows() + dbToClose = db + + expect(sentB.subject).toBe(sentA.subject) + expect(sentB.from).toBe(sentA.from) + expect(sentB.to).toEqual(sentA.to) + }) +}) diff --git a/src/mail/reply-headers.test.ts b/src/mail/reply-headers.test.ts new file mode 100644 index 0000000..c7e2b5f --- /dev/null +++ b/src/mail/reply-headers.test.ts @@ -0,0 +1,82 @@ +import { describe, expect, it } from 'vitest' +import type { StoredThread } from '../store/conversations.js' +import { deriveReplyHeaders } from './reply-headers.js' + +/** Minimal StoredThread builder — only the fields deriveReplyHeaders reads vary per test. */ +function thread(overrides: Partial): StoredThread { + return { + id: 'thread-id', + conversationId: 'conversation-id', + direction: 'inbound', + messageId: null, + inReplyTo: null, + fromAddress: 'customer@example.test', + bodyText: null, + bodyHtml: null, + deliveryStatus: null, + idempotencyKey: null, + sendEnvelope: null, + claimedUntil: null, + customerViewedAt: null, + createdAt: new Date('2024-01-01T00:00:00.000Z'), + authorKind: 'customer', + authorAgentId: null, + authorAssistantId: null, + draftStatus: null, + approvedByAgentId: null, + draftResolvedAt: null, + draftEdited: false, + ...overrides, + } +} + +describe('deriveReplyHeaders', () => { + it('prefixes the subject with Re: when not already prefixed', () => { + const { subject } = deriveReplyHeaders({ subject: 'Help with my order', threads: [] }) + expect(subject).toBe('Re: Help with my order') + }) + + it('does not double-prefix a subject already starting with re: (case-insensitive)', () => { + expect(deriveReplyHeaders({ subject: 'RE: Help', threads: [] }).subject).toBe('RE: Help') + expect(deriveReplyHeaders({ subject: 're: Help', threads: [] }).subject).toBe('re: Help') + }) + + it('inReplyTo is the most recent INBOUND thread with a messageId; references is every non-null messageId in order', () => { + const threads = [ + thread({ direction: 'inbound', messageId: '' }), + thread({ direction: 'outbound', messageId: '' }), + thread({ direction: 'inbound', messageId: '' }), + ] + const { inReplyTo, references } = deriveReplyHeaders({ subject: 'Help', threads }) + expect(inReplyTo).toBe('') + expect(references).toEqual([ + '', + '', + '', + ]) + }) + + it('inReplyTo is undefined when no inbound thread has a messageId', () => { + const threads = [thread({ direction: 'inbound', messageId: null })] + expect(deriveReplyHeaders({ subject: 'Help', threads }).inReplyTo).toBeUndefined() + }) + + it('references is undefined (never []) when no thread has a messageId', () => { + const threads = [thread({ direction: 'inbound', messageId: null })] + expect(deriveReplyHeaders({ subject: 'Help', threads }).references).toBeUndefined() + }) + + it('a draft thread (messageId null pre-approval) contributes nothing to references without special-casing', () => { + const threads = [ + thread({ direction: 'inbound', messageId: '' }), + thread({ + direction: 'outbound', + messageId: null, + draftStatus: 'awaiting_review', + authorKind: 'assistant', + }), + ] + const { references } = deriveReplyHeaders({ subject: 'Help', threads }) + expect(references).toEqual(['']) + }) +}) diff --git a/src/mail/reply-headers.ts b/src/mail/reply-headers.ts new file mode 100644 index 0000000..1f571da --- /dev/null +++ b/src/mail/reply-headers.ts @@ -0,0 +1,53 @@ +/** + * Derive a reply's mail headers (`subject`, `In-Reply-To`, `References`) + * from the conversation being replied to (specs/api/agent-inbox-v1.md + * §4a). Extracted from `src/api/conversations.ts` (HT-70) so the Agent- + * authored reply path (`handleReply`) and the draft-approval orchestration + * (`src/mail/approve-draft.ts`, spec §6 step 2 — "derive the envelope + * exactly per agent-inbox-v1 §4a") share the EXACT same derivation rather + * than two independently-drifting copies. Pure, no I/O — the caller + * supplies the conversation's already-loaded threads. + */ + +import type { StoredThread } from '../store/conversations.js' + +/** + * - `subject`: the conversation's subject, `Re: `-prefixed unless it + * already starts with `re:` (case-insensitive) — never double-prefixed. + * - `inReplyTo`: the `messageId` of the most-recent INBOUND thread that has + * one. Threads are stored oldest-first, so this walks from the end + * looking for the first (i.e. most recent) inbound thread with a + * non-null `messageId`. `undefined` if there is none (e.g. every inbound + * message arrived without a `Message-ID`). + * - `references`: every thread's `messageId`, in chronological order, that + * is non-null. `undefined` (the key omitted entirely, per spec §4a) when + * NO thread has one — never an empty array in that case. A draft thread + * awaiting approval always has `messageId: null` (minted only at + * approval), so it contributes nothing here without needing to be + * filtered out specially. + */ +export function deriveReplyHeaders(conversation: { subject: string; threads: StoredThread[] }): { + subject: string + inReplyTo: string | undefined + references: string[] | undefined +} { + const subject = /^re:/i.test(conversation.subject) + ? conversation.subject + : `Re: ${conversation.subject}` + + let inReplyTo: string | undefined + for (let i = conversation.threads.length - 1; i >= 0; i--) { + const thread = conversation.threads[i] + if (thread.direction === 'inbound' && thread.messageId !== null) { + inReplyTo = thread.messageId + break + } + } + + const referencesList = conversation.threads + .map((t) => t.messageId) + .filter((messageId): messageId is string => messageId !== null) + const references = referencesList.length > 0 ? referencesList : undefined + + return { subject, inReplyTo, references } +} diff --git a/src/mail/send.ts b/src/mail/send.ts index a951897..01bf69a 100644 --- a/src/mail/send.ts +++ b/src/mail/send.ts @@ -307,6 +307,17 @@ export interface SendReplyInput { * Omitted entirely means no dedup protection — a fresh send every call. */ idempotencyKey?: string + /** + * The acting Agent's id, when known (HT-70; specs/plugins/substrate-v1.md + * §3's author-identity forward-carry) — becomes `threads.author_agent_id` + * on the inserted row via `ConversationStore.appendThread`'s existing + * `NewThread.authorAgentId`. Omitted/`undefined` (every pre-HT-70 caller) + * behaves BYTE-IDENTICALLY to before this field existed: `appendThread` + * already defaults a missing `authorAgentId` to `null`, so passing + * `undefined` through unconditionally below is a no-op change to the + * persisted row. + */ + authorAgentId?: string | null } /** @@ -427,6 +438,7 @@ export async function sendReply( deliveryStatus: 'pending', idempotencyKey: input.idempotencyKey, sendEnvelope, + authorAgentId: input.authorAgentId ?? null, }) if (!appended.ok) { diff --git a/src/store/assistants.test.ts b/src/store/assistants.test.ts index 369737c..d349c70 100644 --- a/src/store/assistants.test.ts +++ b/src/store/assistants.test.ts @@ -58,6 +58,19 @@ describe('AssistantStore', () => { expect(assistant.createdByAgentId).toBeNull() }) + it('create with an explicit id (HT-70, the token/id knot) stores that id verbatim', async () => { + const { store } = await freshStore() + const explicitId = '11111111-1111-4111-8111-111111111111' + const assistant = await store.create({ + id: explicitId, + name: 'Pre-minted Bot', + module: 'draft-reply', + tokenHash: 'hash', + }) + expect(assistant.id).toBe(explicitId) + expect(await store.get(explicitId)).toEqual(assistant) + }) + it('get returns null for an unknown id', async () => { const { store } = await freshStore() expect(await store.get(RANDOM_UUID)).toBeNull() @@ -99,19 +112,19 @@ describe('AssistantStore', () => { expect(await store.patch(RANDOM_UUID, { name: 'x' })).toBeNull() }) - it('updateTokenHash replaces the hash (verified via getTokenHash) and throws for an unknown id', async () => { + it('updateTokenHash replaces the hash (verified via getForAuth) and throws for an unknown id', async () => { const { store } = await freshStore() const created = await store.create({ name: 'Bot', module: 'm', tokenHash: 'hash-v1' }) - expect(await store.getTokenHash(created.id)).toBe('hash-v1') + expect((await store.getForAuth(created.id))?.tokenHash).toBe('hash-v1') await store.updateTokenHash(created.id, 'hash-v2') - expect(await store.getTokenHash(created.id)).toBe('hash-v2') + expect((await store.getForAuth(created.id))?.tokenHash).toBe('hash-v2') await expect(store.updateTokenHash(RANDOM_UUID, 'hash-v3')).rejects.toThrow() }) - it('getTokenHash returns null for an unknown id', async () => { + it('getForAuth returns null for an unknown id', async () => { const { store } = await freshStore() - expect(await store.getTokenHash(RANDOM_UUID)).toBeNull() + expect(await store.getForAuth(RANDOM_UUID)).toBeNull() }) }) diff --git a/src/store/assistants.ts b/src/store/assistants.ts index b616fc3..27182af 100644 --- a/src/store/assistants.ts +++ b/src/store/assistants.ts @@ -45,6 +45,15 @@ export interface AssistantRecord { /** Input to {@link AssistantStore.create}. */ export interface NewAssistant { + /** + * Caller-supplied id (HT-70) — mirrors `NewThread.id` in + * `src/store/conversations.ts`'s "id/token knot" pattern: the token format + * (`ht_asst__`, spec §3) embeds the assistant's id, so + * the id must be known BEFORE the row exists in order to mint it + * (`src/auth/assistant-token.ts`'s `mintAssistantToken`). Omitted lets + * `gen_random_uuid()` assign one, same as before this field existed. + */ + id?: string name: string module: string /** The SHA-256 digest of the token's secret part (spec §3) — already hashed by the caller. This store never sees the plaintext token. */ @@ -76,14 +85,16 @@ export interface AssistantStore { updateTokenHash(id: string, tokenHash: string): Promise /** - * The raw `token_hash` for Assistant `id` — what wave 3's token verifier - * compares a presented token's secret part against (constant-time, - * outside this store). `null` if `id` doesn't exist. Never returned from - * {@link AssistantRecord} itself (module doc) — this is the one method - * that reaches the hash, by design, for the one caller that legitimately - * needs it. + * One-snapshot read for authentication: the full {@link AssistantRecord} + * PLUS the raw `token_hash`, from a single SELECT, so status and hash can + * never be observed from two different points in time (a disable or + * rotation between separate reads could otherwise validate a stale + * credential). The hash is never returned from {@link AssistantRecord} + * itself (module doc) — this is the one method that reaches it, by + * design, for the one caller that legitimately needs it. `null` if `id` + * doesn't exist. */ - getTokenHash(id: string): Promise + getForAuth(id: string): Promise<{ record: AssistantRecord; tokenHash: string } | null> } /** Raw `assistants` row shape, before mapping to {@link AssistantRecord}. */ @@ -119,12 +130,20 @@ function toAssistantRecord(row: AssistantRow): AssistantRecord { export function createAssistantStore(db: Db): AssistantStore { return { async create(input) { - const [row] = await db.query( - `INSERT INTO assistants (name, module, token_hash, created_by_agent_id) - VALUES ($1, $2, $3, $4) - RETURNING ${ASSISTANT_COLUMNS}`, - [input.name, input.module, input.tokenHash, input.createdByAgentId ?? null], - ) + const [row] = + input.id !== undefined + ? await db.query( + `INSERT INTO assistants (id, name, module, token_hash, created_by_agent_id) + VALUES ($1, $2, $3, $4, $5) + RETURNING ${ASSISTANT_COLUMNS}`, + [input.id, input.name, input.module, input.tokenHash, input.createdByAgentId ?? null], + ) + : await db.query( + `INSERT INTO assistants (name, module, token_hash, created_by_agent_id) + VALUES ($1, $2, $3, $4) + RETURNING ${ASSISTANT_COLUMNS}`, + [input.name, input.module, input.tokenHash, input.createdByAgentId ?? null], + ) return toAssistantRecord(row) }, @@ -186,13 +205,15 @@ export function createAssistantStore(db: Db): AssistantStore { } }, - async getTokenHash(id) { - const rows = await db.query<{ token_hash: string }>( - `SELECT token_hash FROM assistants WHERE id = $1`, + async getForAuth(id) { + const rows = await db.query( + `SELECT ${ASSISTANT_COLUMNS}, token_hash FROM assistants WHERE id = $1`, [id], ) const row = rows[0] - return row === undefined ? null : row.token_hash + return row === undefined + ? null + : { record: toAssistantRecord(row), tokenHash: row.token_hash } }, } } diff --git a/src/store/conversations.test.ts b/src/store/conversations.test.ts index 2356165..6bd27d5 100644 --- a/src/store/conversations.test.ts +++ b/src/store/conversations.test.ts @@ -1448,8 +1448,11 @@ describe('createConversationStore', () => { resolvedByAgentId: agentId, messageId: '', sendEnvelope: testEnvelope, + inReplyTo: null, + edited: false, }) + if (resolved === null || typeof resolved === 'string') throw new Error('unreachable') expect(resolved).toMatchObject({ id: draft.threadId, direction: 'outbound', @@ -1461,7 +1464,208 @@ describe('createConversationStore', () => { draftEdited: false, bodyText: 'Original draft body.', }) - expect(resolved?.draftResolvedAt).toBeInstanceOf(Date) + expect(resolved.draftResolvedAt).toBeInstanceOf(Date) + }) + + it('approve on a CLOSED conversation reopens it to active (HT-70 review fix — the normal reply-reopen rule applies at approval time)', async () => { + const { store, db } = await freshStore() + const assistantId = await createTestAssistant(db) + const agentId = await createTestAgent(db) + const { conversationId } = await store.createConversation(newConversation()) + const draft = await store.appendDraft(conversationId, { + assistantId, + bodyText: 'Reply to a closed conversation.', + idempotencyKey: 'approve-reopen-closed', + }) + if (!draft.ok) throw new Error('unreachable') + await setStatus(db, conversationId, 'closed') + + const resolved = await store.resolveDraft({ + action: 'approve', + threadId: draft.threadId, + resolvedByAgentId: agentId, + messageId: '', + sendEnvelope: testEnvelope, + inReplyTo: null, + edited: false, + }) + if (resolved === null || typeof resolved === 'string') throw new Error('unreachable') + expect(resolved.draftStatus).toBe('approved') + + const conversation = await store.getConversation(conversationId) + expect(conversation?.status).toBe('active') + }) + + it('approve on an ACTIVE conversation leaves its status alone', async () => { + const { store, db } = await freshStore() + const assistantId = await createTestAssistant(db) + const agentId = await createTestAgent(db) + const { conversationId } = await store.createConversation(newConversation()) + const draft = await store.appendDraft(conversationId, { + assistantId, + bodyText: 'Reply to an active conversation.', + idempotencyKey: 'approve-active-1', + }) + if (!draft.ok) throw new Error('unreachable') + expect((await store.getConversation(conversationId))?.status).toBe('active') + + await store.resolveDraft({ + action: 'approve', + threadId: draft.threadId, + resolvedByAgentId: agentId, + messageId: '', + sendEnvelope: testEnvelope, + inReplyTo: null, + edited: false, + }) + + expect((await store.getConversation(conversationId))?.status).toBe('active') + }) + + it('approve on a PENDING conversation leaves it pending — never auto-set, matching the normal-reply rule', async () => { + const { store, db } = await freshStore() + const assistantId = await createTestAssistant(db) + const agentId = await createTestAgent(db) + const { conversationId } = await store.createConversation(newConversation()) + const draft = await store.appendDraft(conversationId, { + assistantId, + bodyText: 'Reply to a pending conversation.', + idempotencyKey: 'approve-pending-1', + }) + if (!draft.ok) throw new Error('unreachable') + await setStatus(db, conversationId, 'pending') + + await store.resolveDraft({ + action: 'approve', + threadId: draft.threadId, + resolvedByAgentId: agentId, + messageId: '', + sendEnvelope: testEnvelope, + inReplyTo: null, + edited: false, + }) + + expect((await store.getConversation(conversationId))?.status).toBe('pending') + }) + + it('approve on a DELETED conversation is refused (conversation-deleted), leaving the draft row completely untouched (HT-70 review fix, Codex — the TOCTOU close)', async () => { + const { store, db } = await freshStore() + const assistantId = await createTestAssistant(db) + const agentId = await createTestAgent(db) + const { conversationId } = await store.createConversation(newConversation()) + const draft = await store.appendDraft(conversationId, { + assistantId, + bodyText: 'Reply to a deleted conversation.', + idempotencyKey: 'approve-deleted-1', + }) + if (!draft.ok) throw new Error('unreachable') + await store.deleteConversation(conversationId) + + const resolved = await store.resolveDraft({ + action: 'approve', + threadId: draft.threadId, + resolvedByAgentId: agentId, + messageId: '', + sendEnvelope: testEnvelope, + inReplyTo: null, + edited: false, + }) + expect(resolved).toBe('conversation-deleted') + + const conversation = await store.getConversation(conversationId, { includeDeleted: true }) + const thread = conversation?.threads.find((t) => t.id === draft.threadId) + expect(thread).toMatchObject({ + draftStatus: 'awaiting_review', + deliveryStatus: null, + messageId: null, + sendEnvelope: null, + approvedByAgentId: null, + }) + }) + + it('approve on a SPAM conversation is refused (conversation-spam), leaving the draft row completely untouched (HT-70 review fix, Codex — the TOCTOU close)', async () => { + const { store, db } = await freshStore() + const assistantId = await createTestAssistant(db) + const agentId = await createTestAgent(db) + const { conversationId } = await store.createConversation(newConversation()) + const draft = await store.appendDraft(conversationId, { + assistantId, + bodyText: 'Reply to a spam conversation.', + idempotencyKey: 'approve-spam-1', + }) + if (!draft.ok) throw new Error('unreachable') + await setStatus(db, conversationId, 'spam') + + const resolved = await store.resolveDraft({ + action: 'approve', + threadId: draft.threadId, + resolvedByAgentId: agentId, + messageId: '', + sendEnvelope: testEnvelope, + inReplyTo: null, + edited: false, + }) + expect(resolved).toBe('conversation-spam') + + const conversation = await store.getConversation(conversationId, { includeDeleted: false }) + const thread = conversation?.threads.find((t) => t.id === draft.threadId) + expect(thread).toMatchObject({ + draftStatus: 'awaiting_review', + deliveryStatus: null, + messageId: null, + sendEnvelope: null, + approvedByAgentId: null, + }) + // The conversation itself is untouched too — still spam, not reopened. + expect(conversation?.status).toBe('spam') + }) + + it('discard is unaffected by conversation status — no deleted/spam refusal (only approve has that restriction)', async () => { + const { store, db } = await freshStore() + const assistantId = await createTestAssistant(db) + const agentId = await createTestAgent(db) + const { conversationId } = await store.createConversation(newConversation()) + const draft = await store.appendDraft(conversationId, { + assistantId, + bodyText: 'Discard on a spam conversation is harmless.', + idempotencyKey: 'discard-spam-1', + }) + if (!draft.ok) throw new Error('unreachable') + await setStatus(db, conversationId, 'spam') + + const resolved = await store.resolveDraft({ + action: 'discard', + threadId: draft.threadId, + resolvedByAgentId: agentId, + }) + if (resolved === null || typeof resolved === 'string') throw new Error('unreachable') + expect(resolved.draftStatus).toBe('discarded') + }) + + it('approve (HT-70) also persists in_reply_to, derived by the caller at approval time — StoredThread.inReplyTo is what attemptDeliveryOfClaimedThread reads, never sendEnvelope', async () => { + const { store, db } = await freshStore() + const assistantId = await createTestAssistant(db) + const agentId = await createTestAgent(db) + const { conversationId } = await store.createConversation(newConversation()) + const draft = await store.appendDraft(conversationId, { + assistantId, + bodyText: 'A reply to the inbound message.', + idempotencyKey: 'approve-inreplyto-1', + }) + if (!draft.ok) throw new Error('unreachable') + + const resolved = await store.resolveDraft({ + action: 'approve', + threadId: draft.threadId, + resolvedByAgentId: agentId, + messageId: '', + sendEnvelope: testEnvelope, + inReplyTo: '', + edited: false, + }) + + if (resolved === null || typeof resolved === 'string') throw new Error('unreachable') + expect(resolved.inReplyTo).toBe('') }) it('approve with edits: replaces the body and records draft_edited = true', async () => { @@ -1482,7 +1686,9 @@ describe('createConversationStore', () => { resolvedByAgentId: agentId, messageId: '', sendEnvelope: testEnvelope, + inReplyTo: null, edit: { bodyText: 'Edited by the Agent before sending.' }, + edited: true, }) expect(resolved).toMatchObject({ @@ -1509,12 +1715,13 @@ describe('createConversationStore', () => { resolvedByAgentId: agentId, }) + if (resolved === null || typeof resolved === 'string') throw new Error('unreachable') expect(resolved).toMatchObject({ draftStatus: 'discarded', deliveryStatus: null, approvedByAgentId: agentId, }) - expect(resolved?.draftResolvedAt).toBeInstanceOf(Date) + expect(resolved.draftResolvedAt).toBeInstanceOf(Date) }) it('returns null for an unknown threadId, a non-draft thread, or a draft already resolved', async () => { @@ -1581,8 +1788,11 @@ describe('createConversationStore', () => { resolvedByAgentId: agentId, messageId: '', sendEnvelope: testEnvelope, + inReplyTo: null, + edited: false, }) - expect(approved?.draftStatus).toBe('approved') + if (approved === null || typeof approved === 'string') throw new Error('unreachable') + expect(approved.draftStatus).toBe('approved') // approve-then-discard: the row is no longer awaiting_review, so // discard is a no-op — it must NOT flip an already-approved, @@ -1603,6 +1813,8 @@ describe('createConversationStore', () => { resolvedByAgentId: agentId, messageId: '', sendEnvelope: { ...testEnvelope, subject: 'A different subject' }, + inReplyTo: null, + edited: false, }) expect(approveAgain).toBeNull() @@ -1640,6 +1852,8 @@ describe('createConversationStore', () => { resolvedByAgentId: agentId, messageId: '', sendEnvelope: testEnvelope, + inReplyTo: null, + edited: false, }) // Age it so it's past the staleness threshold for a 'pending' row. await db.query('UPDATE threads SET created_at = $1 WHERE id = $2', [ @@ -1654,6 +1868,243 @@ describe('createConversationStore', () => { expect(claimed?.id).toBe(draft.threadId) }) }) + + describe('draft.created / draft.resolved events (HT-70, spec §4)', () => { + /** Read back every `event_outbox` row for `conversationId`, oldest first. */ + async function outboxEventsFor( + db: Db, + conversationId: string, + ): Promise }>> { + const rows = await db.query<{ type: string; data: unknown }>( + `SELECT type, data FROM event_outbox WHERE conversation_id = $1 ORDER BY occurred_at, event_id`, + [conversationId], + ) + return rows.map((r) => ({ type: r.type, data: r.data as Record })) + } + + it('appendDraft fires draft.created exactly once, never again on an idempotency-key replay', async () => { + const { store, db } = await freshStore() + const assistantId = await createTestAssistant(db) + const { conversationId } = await store.createConversation(newConversation()) + + const first = await store.appendDraft(conversationId, { + assistantId, + bodyText: 'A draft.', + idempotencyKey: 'event-key-1', + }) + if (!first.ok) throw new Error('unreachable') + + // Replay with the SAME key: `created: false`, no second event. + const replay = await store.appendDraft(conversationId, { + assistantId, + bodyText: 'A draft.', + idempotencyKey: 'event-key-1', + }) + expect(replay).toMatchObject({ ok: true, created: false }) + + const events = await outboxEventsFor(db, conversationId) + expect(events).toEqual([ + { type: 'draft.created', data: { threadId: first.threadId, assistantId } }, + ]) + }) + + it('appendDraft fires NO event when the conversation is missing or soft-deleted', async () => { + const { store, db } = await freshStore() + const assistantId = await createTestAssistant(db) + const { conversationId } = await store.createConversation(newConversation()) + await store.deleteConversation(conversationId) + + const result = await store.appendDraft(conversationId, { + assistantId, + bodyText: 'Stranded draft.', + idempotencyKey: 'event-key-deleted', + }) + expect(result).toMatchObject({ ok: false, reason: 'deleted' }) + expect(await outboxEventsFor(db, conversationId)).toEqual([]) + }) + + it('resolveDraft(discard) fires draft.resolved with resolution=discarded, edited=false', async () => { + const { store, db } = await freshStore() + const assistantId = await createTestAssistant(db) + const agentId = await createTestAgent(db) + const { conversationId } = await store.createConversation(newConversation()) + const draft = await store.appendDraft(conversationId, { + assistantId, + bodyText: 'To discard.', + idempotencyKey: 'event-discard-1', + }) + if (!draft.ok) throw new Error('unreachable') + + await store.resolveDraft({ + action: 'discard', + threadId: draft.threadId, + resolvedByAgentId: agentId, + }) + + const events = await outboxEventsFor(db, conversationId) + expect(events).toContainEqual({ + type: 'draft.resolved', + data: { threadId: draft.threadId, resolution: 'discarded', edited: false }, + }) + }) + + it('resolveDraft(approve) fires draft.resolved with resolution=approved and the real edited flag', async () => { + const { store, db } = await freshStore() + const assistantId = await createTestAssistant(db) + const agentId = await createTestAgent(db) + const { conversationId } = await store.createConversation(newConversation()) + const draft = await store.appendDraft(conversationId, { + assistantId, + bodyText: 'To approve, edited.', + idempotencyKey: 'event-approve-1', + }) + if (!draft.ok) throw new Error('unreachable') + + await store.resolveDraft({ + action: 'approve', + threadId: draft.threadId, + resolvedByAgentId: agentId, + messageId: '', + sendEnvelope: testEnvelope, + inReplyTo: null, + edit: { bodyText: 'Edited body.' }, + edited: true, + }) + + const events = await outboxEventsFor(db, conversationId) + expect(events).toContainEqual({ + type: 'draft.resolved', + data: { threadId: draft.threadId, resolution: 'approved', edited: true }, + }) + }) + + it('a resolveDraft call that matches no row (already resolved, unknown id) fires no event', async () => { + const { store, db } = await freshStore() + const agentId = await createTestAgent(db) + const { conversationId } = await store.createConversation(newConversation()) + + const result = await store.resolveDraft({ + action: 'discard', + threadId: RANDOM_UUID, + resolvedByAgentId: agentId, + }) + expect(result).toBeNull() + expect(await outboxEventsFor(db, conversationId)).toEqual([]) + }) + }) + + describe('getConversationByThreadId (HT-70)', () => { + it('finds the conversation by ANY thread id within it, including the original inbound thread', async () => { + const { store, db } = await freshStore() + const assistantId = await createTestAssistant(db) + const { conversationId, threadId: inboundId } = await store.createConversation( + newConversation(), + ) + const draft = await store.appendDraft(conversationId, { + assistantId, + bodyText: 'A draft.', + idempotencyKey: 'lookup-1', + }) + if (!draft.ok) throw new Error('unreachable') + + const byInbound = await store.getConversationByThreadId(inboundId) + expect(byInbound?.id).toBe(conversationId) + + const byDraft = await store.getConversationByThreadId(draft.threadId) + expect(byDraft?.id).toBe(conversationId) + expect(byDraft?.threads.map((t) => t.id)).toEqual( + expect.arrayContaining([inboundId, draft.threadId]), + ) + }) + + it('returns null for an unknown thread id', async () => { + const { store } = await freshStore() + expect(await store.getConversationByThreadId(RANDOM_UUID)).toBeNull() + }) + + it('with includeDeleted: false, a thread on a soft-deleted conversation is indistinguishable from unknown', async () => { + const { store } = await freshStore() + const { conversationId, threadId } = await store.createConversation(newConversation()) + await store.deleteConversation(conversationId) + + expect(await store.getConversationByThreadId(threadId)).not.toBeNull() + expect( + await store.getConversationByThreadId(threadId, { includeDeleted: false }), + ).toBeNull() + }) + }) + + describe('threadCount/preview exclude unresolved and discarded drafts (HT-70, spec §7)', () => { + it('listConversations: an awaiting_review draft is invisible to threadCount and preview', async () => { + const { store, db } = await freshStore() + const assistantId = await createTestAssistant(db) + const { conversationId } = await store.createConversation(newConversation()) + + const before = (await store.listConversations({ limit: 10 })).find( + (c) => c.id === conversationId, + ) + expect(before?.threadCount).toBe(1) + + await store.appendDraft(conversationId, { + assistantId, + bodyText: 'A draft nobody has approved yet.', + idempotencyKey: 'preview-1', + }) + + const after = (await store.listConversations({ limit: 10 })).find( + (c) => c.id === conversationId, + ) + // Still 1 — the draft does not count, and does not become the preview. + expect(after?.threadCount).toBe(1) + expect(after?.preview).not.toContain('A draft nobody has approved yet.') + }) + + it('listConversations: a discarded draft stays invisible; an APPROVED draft counts and can become the preview', async () => { + const { store, db } = await freshStore() + const assistantId = await createTestAssistant(db) + const agentId = await createTestAgent(db) + const { conversationId } = await store.createConversation(newConversation()) + + const discarded = await store.appendDraft(conversationId, { + assistantId, + bodyText: 'Will be discarded.', + idempotencyKey: 'preview-discard', + }) + if (!discarded.ok) throw new Error('unreachable') + await store.resolveDraft({ + action: 'discard', + threadId: discarded.threadId, + resolvedByAgentId: agentId, + }) + + const afterDiscard = (await store.listConversations({ limit: 10 })).find( + (c) => c.id === conversationId, + ) + expect(afterDiscard?.threadCount).toBe(1) + + const approved = await store.appendDraft(conversationId, { + assistantId, + bodyText: 'Will be approved and should become the preview.', + idempotencyKey: 'preview-approve', + }) + if (!approved.ok) throw new Error('unreachable') + await store.resolveDraft({ + action: 'approve', + threadId: approved.threadId, + resolvedByAgentId: agentId, + messageId: '', + sendEnvelope: testEnvelope, + inReplyTo: null, + edited: false, + }) + + const afterApprove = (await store.listConversations({ limit: 10 })).find( + (c) => c.id === conversationId, + ) + expect(afterApprove?.threadCount).toBe(2) + expect(afterApprove?.preview).toContain('Will be approved and should become the preview.') + }) + }) }) describe('event emission (HT-69, spec §4)', () => { diff --git a/src/store/conversations.ts b/src/store/conversations.ts index 3d72b3e..47dc424 100644 --- a/src/store/conversations.ts +++ b/src/store/conversations.ts @@ -386,10 +386,18 @@ export interface NewDraft { /** * Caller-supplied dedup key, UNPREFIXED — {@link * createConversationStore}'s `appendDraft` stores it as `` `draft:${key}` `` - * (spec §6: "the engine stores it prefixed... so the shared - * `(conversation_id, idempotency_key)` uniqueness namespace can never - * replay a reply as a draft or vice versa"). Required — spec §6 states - * `Idempotency-Key` is required on this endpoint. + * (spec §6), sharing the `(conversation_id, idempotency_key)` uniqueness + * namespace with replies. The prefix alone does not make the two + * sub-namespaces disjoint — a reply's key is stored RAW, so a + * caller-supplied reply key literally spelled `draft:abc` could otherwise + * collide with an engine-minted draft key of the same name. The actual + * guarantee is this prefix PLUS `handleReply` + * (`src/api/conversations.ts`) rejecting any caller-supplied reply + * `Idempotency-Key` that itself starts with `draft:` — see that + * rejection's own comment for why reply keys are refused rather than + * retro-prefixed (a stored-raw key in production would lose idempotency + * continuity). Required — spec §6 states `Idempotency-Key` is required on + * this endpoint. */ idempotencyKey: string } @@ -407,18 +415,28 @@ export interface ListAwaitingDraftsCursor { } /** - * Input to {@link ConversationStore.resolveDraft} (HT-68; spec §6): either - * branch of `POST /api/v1/drafts/{threadId}/approve` or `.../discard`. - * `resolvedByAgentId` is written to `threads.approved_by_agent_id` on both - * branches (spec §2: that column is the resolution audit field generally, - * not "approval" specifically). + * Input to {@link ConversationStore.resolveDraft} (HT-68/HT-70; spec §6): + * either branch of `POST /api/v1/drafts/{threadId}/approve` or + * `.../discard`. `resolvedByAgentId` is written to + * `threads.approved_by_agent_id` on both branches (spec §2: that column is + * the resolution audit field generally, not "approval" specifically). * * The `approve` branch takes `messageId`/`sendEnvelope` as OPAQUE inputs — * this store does NOT mint a reply token or derive an envelope (spec §6 * steps 1-3 are the caller's job; `sendReply` cannot be reused for an - * existing row, see spec §6's "what approval actually does"). `edit`, - * when present, is spec §6's "approve with edits": the given fields replace - * the draft's stored body and `draft_edited` is recorded `true`. + * existing row, see spec §6's "what approval actually does"). + * + * `edit` and `edited` are DELIBERATELY separate (HT-70 — the wave-1 shape + * fused them into one `edit !== undefined` check, revised during the + * approval-orchestration build): `edit`, when present, replaces the + * draft's stored `body_text`/`body_html` (omitted fields left unchanged via + * `COALESCE`) — but the caller (`src/mail/approve-draft.ts`) also uses it + * to persist an HT-32 pixel-injected `bodyHtml` even when the approving + * Agent submitted no edit at all. `edited` is therefore the ONLY signal for + * spec §2's `draft_edited` audit column ("did the approving Agent change + * the body before sending") — the caller computes it from whether an Agent + * override was actually submitted, never from whether `edit` happens to be + * present. */ export type ResolveDraftInput = | { @@ -427,7 +445,22 @@ export type ResolveDraftInput = resolvedByAgentId: string messageId: string sendEnvelope: SendEnvelope + /** + * The `In-Reply-To` header for the eventual outbound mail (HT-70) — + * derived by the caller (`src/mail/approve-draft.ts`, the same + * `deriveReplyHeaders` derivation `handleReply` uses) at APPROVAL + * time, exactly like `sendEnvelope`. A draft's own `in_reply_to` + * column is never set at draft-creation time (spec §6 scopes envelope + * derivation to approval, not draft creation) — this is what makes + * {@link StoredThread.inReplyTo} correct on the approved row, which + * `attemptDeliveryOfClaimedThread` (`src/mail/send.ts`) reads + * directly (never from `sendEnvelope`) when rebuilding the + * `OutboundEmail` to send. + */ + inReplyTo: string | null edit?: { bodyText?: string; bodyHtml?: string } + /** Did the APPROVING AGENT explicitly change the body before sending — spec §2's `draft_edited` audit column. See this type's doc comment for why it is decoupled from `edit`'s presence. */ + edited: boolean } | { action: 'discard' @@ -686,7 +719,9 @@ export interface ConversationStore { * 'awaiting_review'`, `delivery_status NULL`. Reuses {@link * appendThreadInTx}'s not-found/deleted policy and idempotency-key * get-or-insert (the caller's key is stored `` `draft:${key}` `` — see - * {@link NewDraft.idempotencyKey}), but causes NO reopen and NO + * {@link NewDraft.idempotencyKey} for the full disjointness guarantee, + * which requires the reply-side rejection too, not this prefix alone), + * but causes NO reopen and NO * `updated_at` bump on the conversation, even if it is closed or spam * (see the module doc's "actor model + draft lifecycle" section) — * approval, not draft creation, is what later follows the normal @@ -715,11 +750,46 @@ export interface ConversationStore { * a non-outbound thread, or a draft that was already resolved (or was * never a draft) — the same "no such row in the state this method * requires" shape {@link setConversationStatus} uses for a missing/deleted - * conversation. This method does not check conversation status (spam, - * soft-deleted) — spec §6 assigns those refusals to the API layer, which - * has the conversation already loaded. + * conversation. + * + * **HT-70 review fix (Codex): the `action: 'approve'` branch IS the + * authoritative conversation-status check, not the API layer.** The API's + * own preflight read (`src/api/drafts.ts`'s `handleApproveDraft`, via + * `getConversationByThreadId`) is a stale snapshot the instant a + * concurrent delete or spam-mark lands between that read and this write — + * a TOCTOU that would otherwise let mail be armed from a conversation an + * Agent already pulled out of the deliverable set. So the approve branch + * itself locks the PARENT conversation row (`FOR UPDATE OF`) inside the + * same transaction as the thread UPDATE, and returns the sentinel string + * `'conversation-deleted'` or `'conversation-spam'` — leaving the draft + * row completely untouched — when the LOCKED, re-read status is `deleted` + * or `spam` at write time, regardless of what the caller's own snapshot + * said. A `'closed'` conversation reopens to `active` in this same locked + * read-then-write (spec §6's reply-reopen invariant); every other status + * proceeds unchanged. The `action: 'discard'` branch never returns either + * sentinel — discard has no conversation-status restriction. + */ + resolveDraft( + input: ResolveDraftInput, + ): Promise + + /** + * Read one conversation (with all of its threads) by the id of ANY thread + * within it (HT-70). The draft-approval path (`POST /api/v1/drafts/{threadId}/approve` + * and `.../discard`) is handed only a `threadId` in the URL, never the + * conversation id, so it needs this lookup to derive the reply envelope + * (spec §6 step 2) and check the conversation's status (soft-deleted/spam) + * before resolving. Same `includeDeleted` contract as {@link getConversation} + * (default `true`; pass `false` on a public read path so a soft-deleted + * conversation's thread is indistinguishable from a nonexistent one). + * Returns `null` when `threadId` names no thread at all, or (with + * `includeDeleted: false`) names a thread whose conversation is + * soft-deleted. */ - resolveDraft(input: ResolveDraftInput): Promise + getConversationByThreadId( + threadId: string, + options?: { includeDeleted?: boolean }, + ): Promise<(StoredConversation & { threads: StoredThread[] }) | null> } /** @@ -733,8 +803,16 @@ export interface ConversationStore { * plain `number` like every other count in this codebase, e.g. the * `count(*)::int` precedent in `conversations.test.ts`). */ +// HT-70 (spec §7): an unresolved or discarded draft is "not conversation +// content until sent" — both subqueries below exclude +// `draft_status IN ('awaiting_review','discarded')` rows. `IS DISTINCT FROM` +// (not `NOT IN`) on each value handles the three-valued-logic NULL trap +// correctly for every non-draft row (`draft_status IS NULL`) — the same +// guard this file's other draft-aware queries already use (e.g. +// `claimThreadForDelivery`), rather than a `NOT IN` that would silently +// exclude every non-draft row too (`NULL NOT IN (...)` is NULL, not TRUE). const THREAD_COUNT_SUBQUERY = - '(SELECT count(*) FROM threads t WHERE t.conversation_id = c.id)::int AS thread_count' + "(SELECT count(*) FROM threads t WHERE t.conversation_id = c.id AND t.draft_status IS DISTINCT FROM 'awaiting_review' AND t.draft_status IS DISTINCT FROM 'discarded')::int AS thread_count" /** * Correlated subquery for a conversation's most recent thread body that has @@ -743,9 +821,12 @@ const THREAD_COUNT_SUBQUERY = * direction). The whitespace collapse and truncation happen in JS * ({@link derivePreview}), not SQL — string munging is clearer and cheaper * to test there; SQL's only job is picking the right row. + * + * HT-70 (spec §7): also excludes `draft_status IN ('awaiting_review', + * 'discarded')` rows — see {@link THREAD_COUNT_SUBQUERY}'s comment above. */ const LATEST_BODY_TEXT_SUBQUERY = - '(SELECT t.body_text FROM threads t WHERE t.conversation_id = c.id AND t.body_text IS NOT NULL ORDER BY t.created_at DESC, t.id DESC LIMIT 1) AS latest_body_text' + "(SELECT t.body_text FROM threads t WHERE t.conversation_id = c.id AND t.body_text IS NOT NULL AND t.draft_status IS DISTINCT FROM 'awaiting_review' AND t.draft_status IS DISTINCT FROM 'discarded' ORDER BY t.created_at DESC, t.id DESC LIMIT 1) AS latest_body_text" /** Maximum length of a derived `preview`, per spec §2 (v1.1, HT-27). */ const PREVIEW_MAX_LENGTH = 120 @@ -1363,8 +1444,8 @@ export function createConversationStore(db: Db): ConversationStore { }, async appendDraft(conversationId, draft) { - return db.transaction((tx) => - appendThreadInTx(tx, conversationId, { + return db.transaction(async (tx) => { + const result = await appendThreadInTx(tx, conversationId, { direction: 'outbound', messageId: null, fromAddress: draft.fromAddress ?? '', @@ -1374,8 +1455,23 @@ export function createConversationStore(db: Db): ConversationStore { authorAssistantId: draft.assistantId, draftStatus: 'awaiting_review', idempotencyKey: `draft:${draft.idempotencyKey}`, - }), - ) + }) + // HT-70 (spec §4): fire draft.created ONLY for a genuinely NEW row + // (result.created) — an idempotency-key replay must never re-fire an + // event for the same logical draft, and a refused append + // (not-found/deleted) has nothing to announce (spec: "no event... for + // a soft-deleted conversation, including stranded drafts"). Written + // in the SAME transaction as the insert (spec §4's transactional + // outbox rule). + if (result.ok && result.created) { + await appendOutboxEventInTx(tx, { + type: 'draft.created', + conversationId, + data: { threadId: result.threadId, assistantId: draft.assistantId }, + }) + } + return result + }) }, async listAwaitingDrafts(options) { @@ -1403,50 +1499,148 @@ export function createConversationStore(db: Db): ConversationStore { }, async resolveDraft(input) { - if (input.action === 'discard') { - const rows = await db.query( + return db.transaction(async (tx) => { + if (input.action === 'discard') { + const rows = await tx.query( + `UPDATE threads + SET draft_status = 'discarded', approved_by_agent_id = $2, draft_resolved_at = now() + WHERE id = $1 AND direction = 'outbound' AND draft_status = 'awaiting_review' + RETURNING ${THREAD_COLUMNS}`, + [input.threadId, input.resolvedByAgentId], + ) + const row = rows[0] + if (row === undefined) return null + // HT-70 (spec §4): draft.resolved, in the SAME transaction as the + // write. No event for a row this UPDATE didn't touch (see above). + await appendOutboxEventInTx(tx, { + type: 'draft.resolved', + conversationId: row.conversation_id, + data: { threadId: row.id, resolution: 'discarded', edited: false }, + }) + return toStoredThread(row) + } + + // approve (spec §6 step 4 + HT-70 review fix, Codex — the TOCTOU + // close): lock the PARENT conversation row FIRST, inside this same + // transaction, before touching the thread at all. The API's own + // preflight read (src/api/drafts.ts's handleApproveDraft) is a + // snapshot that can go stale the instant a concurrent delete or + // spam-mark commits between that read and this write — an ordinary + // Postgres UPDATE on the conversations row (deleteConversation, + // setConversationStatus) already holds an exclusive row lock for its + // transaction's duration, so `FOR UPDATE OF c` here waits for any + // such in-flight transaction to finish, then re-reads the + // COMMITTED status — never the caller's stale one. + const parentRows = await tx.query<{ conversation_id: string; status: string }>( + `SELECT c.id AS conversation_id, c.status + FROM conversations c + JOIN threads t ON t.conversation_id = c.id + WHERE t.id = $1 + FOR UPDATE OF c`, + [input.threadId], + ) + const parent = parentRows[0] + if (parent === undefined) { + // No such thread at all — same "nothing to resolve" outcome as a + // draft the thread UPDATE below would also find zero rows for. + return null + } + // 'deleted'/'spam' refuse OUTRIGHT — the draft row is left + // completely untouched (never approved, never armed for delivery). + // This is the authoritative check; the API's own preflight is + // advisory only (see this method's interface doc comment). + if (parent.status === 'deleted') { + return 'conversation-deleted' + } + if (parent.status === 'spam') { + return 'conversation-spam' + } + // 'closed' reopens to active in this SAME locked read-then-write + // (spec §6's reply-reopen invariant, folded in here from the prior + // review round) — 'pending' deliberately stays pending either way + // (never auto-set — see the module doc), and every other status + // (active) proceeds unchanged. + if (parent.status === 'closed') { + await tx.query( + "UPDATE conversations SET status = 'active', updated_at = now() WHERE id = $1", + [parent.conversation_id], + ) + } + + // Writes the caller-derived envelope snapshot + message id, flips + // draft_status → 'approved' and delivery_status → 'pending' in the + // SAME statement (so the row is NEVER observably in a state where + // draft_status is 'approved' but delivery_status is still NULL, or + // vice versa), and records the approve-with-edits audit fields. + // `input.edited` (HT-70) is the ONLY signal for draft_edited — see + // ResolveDraftInput's doc comment for why it is no longer inferred + // from `edit`'s presence. + const rows = await tx.query( `UPDATE threads - SET draft_status = 'discarded', approved_by_agent_id = $2, draft_resolved_at = now() + SET message_id = $2, + send_envelope = $3::jsonb, + in_reply_to = $8, + draft_status = 'approved', + delivery_status = 'pending', + approved_by_agent_id = $4, + draft_resolved_at = now(), + draft_edited = $5, + body_text = COALESCE($6, body_text), + body_html = COALESCE($7, body_html) WHERE id = $1 AND direction = 'outbound' AND draft_status = 'awaiting_review' RETURNING ${THREAD_COLUMNS}`, - [input.threadId, input.resolvedByAgentId], + [ + input.threadId, + input.messageId, + JSON.stringify(input.sendEnvelope), + input.resolvedByAgentId, + input.edited, + input.edit?.bodyText ?? null, + input.edit?.bodyHtml ?? null, + input.inReplyTo, + ], ) const row = rows[0] - return row === undefined ? null : toStoredThread(row) + if (row === undefined) return null + await appendOutboxEventInTx(tx, { + type: 'draft.resolved', + conversationId: row.conversation_id, + data: { threadId: row.id, resolution: 'approved', edited: input.edited }, + }) + return toStoredThread(row) + }) + }, + + /** + * HT-70: see the interface doc comment. A plain join-then-select, not a + * transaction — this is a read. + */ + async getConversationByThreadId(threadId, options) { + const includeDeleted = options?.includeDeleted ?? true + const conversationRows = await db.query( + includeDeleted + ? `SELECT c.id, c.number, c.subject, c.customer_email, c.status, c.tags, c.assignee_agent_id, c.created_at, c.updated_at + FROM conversations c JOIN threads t ON t.conversation_id = c.id + WHERE t.id = $1` + : `SELECT c.id, c.number, c.subject, c.customer_email, c.status, c.tags, c.assignee_agent_id, c.created_at, c.updated_at + FROM conversations c JOIN threads t ON t.conversation_id = c.id + WHERE t.id = $1 AND c.status <> 'deleted'`, + [threadId], + ) + const conversationRow = conversationRows[0] + if (conversationRow === undefined) { + return null } - // approve (spec §6 step 4): writes the caller-derived envelope - // snapshot + message id, flips draft_status → 'approved' and - // delivery_status → 'pending' in the SAME statement (so the row is - // NEVER observably in a state where draft_status is 'approved' but - // delivery_status is still NULL, or vice versa), and records the - // approve-with-edits audit fields. - const edited = input.edit !== undefined - const rows = await db.query( - `UPDATE threads - SET message_id = $2, - send_envelope = $3::jsonb, - draft_status = 'approved', - delivery_status = 'pending', - approved_by_agent_id = $4, - draft_resolved_at = now(), - draft_edited = $5, - body_text = COALESCE($6, body_text), - body_html = COALESCE($7, body_html) - WHERE id = $1 AND direction = 'outbound' AND draft_status = 'awaiting_review' - RETURNING ${THREAD_COLUMNS}`, - [ - input.threadId, - input.messageId, - JSON.stringify(input.sendEnvelope), - input.resolvedByAgentId, - edited, - input.edit?.bodyText ?? null, - input.edit?.bodyHtml ?? null, - ], + const threadRows = await db.query( + `SELECT ${THREAD_COLUMNS} FROM threads WHERE conversation_id = $1 ORDER BY created_at, id`, + [conversationRow.id], ) - const row = rows[0] - return row === undefined ? null : toStoredThread(row) + + return { + ...toStoredConversation(conversationRow), + threads: threadRows.map(toStoredThread), + } }, async deleteConversation(conversationId) {