From 7b23a81713be1449e799b3a38415979f0bb9d830 Mon Sep 17 00:00:00 2001 From: TJ Baker <1617679+zaridan@users.noreply.github.com> Date: Sun, 12 Jul 2026 09:53:00 -0700 Subject: [PATCH] =?UTF-8?q?feat(store,api):=20internal=20notes=20=E2=80=94?= =?UTF-8?q?=20thread=20direction=20'note'=20+=20POST=20/conversations/{id}?= =?UTF-8?q?/notes=20(HT-28)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A note is Agent-only context on a conversation: never emailed, no token minted, no outbox row, invisible to the delivery worker. Migration 007 swaps two constraints together (they must ship as a pair): the direction CHECK widens to admit 'note', and the delivery-status-by-direction CHECK requires notes to carry NULL like inbound — the old constraint would have rejected every note row. Store: appendThread's reopen policy is note-aware — a note on a closed/spam conversation bumps updated_at (it IS activity) but never reopens (it is not the customer coming back). The mail boundary is test-asserted: an aged note is invisible to listDeliverableThreads even at staleAfterMs 0, and the API test proves the sender is never invoked (charter invariant #5 adjacency, spec §4c). API: POST /conversations/{id}/notes — {text} 1-5000, plain text in v1; 201 with the note ThreadView (direction 'note', from = support address, deliveryStatus null). 377/377 tests. Per specs/api/agent-inbox-v1.md §4c (v1.1, HT-25). Co-Authored-By: Claude Fable 5 --- src/api/conversations.ts | 74 +++++++++++++++++++++++++- src/api/index.test.ts | 92 +++++++++++++++++++++++++++++++++ src/api/index.ts | 7 +++ src/api/router.ts | 15 +++++- src/db/migrate.test.ts | 57 +++++++++++++++++++- src/db/migrate.ts | 37 +++++++++++++ src/store/conversations.test.ts | 51 ++++++++++++++++++ src/store/conversations.ts | 11 ++-- 8 files changed, 336 insertions(+), 8 deletions(-) diff --git a/src/api/conversations.ts b/src/api/conversations.ts index 133df15..58c7e78 100644 --- a/src/api/conversations.ts +++ b/src/api/conversations.ts @@ -53,7 +53,7 @@ const MAX_IDEMPOTENCY_KEY_LENGTH = 255 /** 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 { id: string - direction: 'inbound' | 'outbound' + direction: 'inbound' | 'outbound' | 'note' from: string bodyText: string | null bodyHtml: string | null @@ -458,6 +458,58 @@ export async function handleDeleteConversation( return noContent() } +/** + * 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 + * (`appendThread`'s note-aware policy). + * + * 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 + * deleted conversation. + */ +export async function handlePostNote( + id: string, + request: Request, + deps: { store: ConversationStore; supportAddress: string }, +): Promise { + if (!isUuid(id)) { + return apiError(404, 'not_found', 'No conversation with that id.') + } + + const parsedBody = await parseJsonBody(request) + if (!parsedBody.ok) { + return apiError(400, 'validation_failed', 'Request body must be valid JSON.') + } + + const note = parseNoteBody(parsedBody.value) + if (note === null) { + return apiError( + 400, + 'validation_failed', + `text is required and must be ${MIN_REPLY_TEXT_LENGTH}-${MAX_REPLY_TEXT_LENGTH} characters.`, + ) + } + + const result = await deps.store.appendThread(id, { + direction: 'note', + messageId: null, + fromAddress: deps.supportAddress, + bodyText: note.text, + }) + if (!result.ok) { + // not-found and deleted are one generic 404 (spec §5's no-existence-leak). + return apiError(404, 'not_found', 'No conversation with that id.') + } + + return json(201, toThreadViewJson(result.thread)) +} + /** Maximum length of one tag, after trimming (spec §4e, v1.1). */ const MAX_TAG_LENGTH = 40 @@ -594,6 +646,26 @@ function parsePatchStatusBody(raw: unknown): ConversationStatus | null { : null } +/** + * Validate a POST-notes body against spec §4c: `text` must be a string of + * `[MIN_REPLY_TEXT_LENGTH, MAX_REPLY_TEXT_LENGTH]` chars; notes are plain + * text in v1, so there is no `html` (unknown properties are ignored, the + * same posture as the reply body). Returns `null` on any violation — never + * throws. + */ +function parseNoteBody(raw: unknown): { text: string } | null { + if (typeof raw !== 'object' || raw === null) return null + const { text } = raw as Record + if ( + typeof text !== 'string' || + text.length < MIN_REPLY_TEXT_LENGTH || + text.length > MAX_REPLY_TEXT_LENGTH + ) { + return null + } + return { text } +} + /** * Validate and NORMALIZE a PUT-tags body against spec §4e: `tags` must be an * array of strings; each entry is trimmed then lowercased and must be diff --git a/src/api/index.test.ts b/src/api/index.test.ts index ad3bc4c..a67c4bc 100644 --- a/src/api/index.test.ts +++ b/src/api/index.test.ts @@ -1166,6 +1166,98 @@ describe('createInboxApi', () => { }) }) + // --- notes (HT-28, spec §4c v1.1) ------------------------------------------------ + + describe('notes', () => { + it('201 with the note ThreadView: direction note, from = support address, deliveryStatus null — and the sender is NEVER invoked', async () => { + const { store, api, sent } = await freshApi() + const { conversationId } = await store.createConversation(newConversation()) + + const res = await api( + post(`/api/v1/conversations/${conversationId}/notes`, { text: 'Internal context.' }), + ) + expect(res.status).toBe(201) + const body = (await res.json()) as { + direction: string + from: string + bodyText: string | null + bodyHtml: string | null + deliveryStatus: string | null + } + expect(body).toMatchObject({ + direction: 'note', + from: SUPPORT_ADDRESS, + bodyText: 'Internal context.', + bodyHtml: null, + deliveryStatus: null, + }) + // The mail boundary (spec §4c): a note never touches the send path. + expect(sent).toEqual([]) + }) + + it('a note on a closed conversation bumps updatedAt but never reopens it', async () => { + const { db, store, api } = await freshApi() + const { conversationId } = await store.createConversation(newConversation()) + await setStatus(db, conversationId, 'closed') + await setUpdatedAt(db, conversationId, new Date('2020-01-01T00:00:00.000Z')) + + const res = await api( + post(`/api/v1/conversations/${conversationId}/notes`, { text: 'Still closed.' }), + ) + expect(res.status).toBe(201) + + const updated = await store.getConversation(conversationId) + expect(updated?.status).toBe('closed') + expect(updated?.updatedAt.getTime()).toBeGreaterThan( + new Date('2020-01-01T00:00:00.000Z').getTime(), + ) + }) + + it('400s on a missing/empty/over-limit text and a non-JSON body', async () => { + const { store, api } = await freshApi() + const { conversationId } = await store.createConversation(newConversation()) + + for (const bad of [{}, { text: '' }, { text: 'x'.repeat(5001) }, { text: 42 }]) { + const res = await api(post(`/api/v1/conversations/${conversationId}/notes`, bad)) + expect(res.status).toBe(400) + } + const rawRes = await api( + postRaw(`/api/v1/conversations/${conversationId}/notes`, 'not json{'), + ) + expect(rawRes.status).toBe(400) + }) + + it('404s for missing, deleted, and non-UUID ids', async () => { + const { db, store, api } = await freshApi() + const { conversationId } = await store.createConversation(newConversation()) + await setStatus(db, conversationId, 'deleted') + + expect( + (await api(post(`/api/v1/conversations/${RANDOM_UUID}/notes`, { text: 'x' }))).status, + ).toBe(404) + expect( + (await api(post(`/api/v1/conversations/${conversationId}/notes`, { text: 'x' }))).status, + ).toBe(404) + expect( + (await api(post('/api/v1/conversations/not-a-uuid/notes', { text: 'x' }))).status, + ).toBe(404) + }) + + it('GET on the notes route is 405 with Allow: POST; 401 without a token', async () => { + const { store, api } = await freshApi() + const { conversationId } = await store.createConversation(newConversation()) + + const wrongMethod = await api(get(`/api/v1/conversations/${conversationId}/notes`)) + expect(wrongMethod.status).toBe(405) + expect(wrongMethod.headers.get('Allow')).toBe('POST') + + const noAuth = await api( + post(`/api/v1/conversations/${conversationId}/notes`, { text: 'x' }, undefined), + ) + expect(noAuth.status).toBe(401) + }) + }) + // --- tags & assignee (HT-29/HT-31, spec §4e/§4f v1.1) --------------------------- describe('tags & assignee', () => { diff --git a/src/api/index.ts b/src/api/index.ts index 81f668f..fe58269 100644 --- a/src/api/index.ts +++ b/src/api/index.ts @@ -38,6 +38,7 @@ import { handleGetConversation, handleListConversations, handlePatchConversation, + handlePostNote, handlePutAssignee, handlePutTags, handleReply, @@ -150,6 +151,12 @@ export function createInboxApi(deps: InboxApiDeps): (request: Request) => Promis case 'conversation-delete': return await handleDeleteConversation(route.id, { store: deps.store }) + case 'conversation-note': + return await handlePostNote(route.id, request, { + store: deps.store, + supportAddress: deps.supportAddress, + }) + case 'conversation-tags': return await handlePutTags(route.id, request, { store: deps.store }) diff --git a/src/api/router.ts b/src/api/router.ts index 46a0788..00345a4 100644 --- a/src/api/router.ts +++ b/src/api/router.ts @@ -1,9 +1,9 @@ /** - * A minimal method+pathname matcher for the Agent Inbox API's five routes + * A minimal method+pathname matcher for the Agent Inbox API's six routes * (specs/api/agent-inbox-v1.md §3a, §3b, §4). * * Deliberately NOT a general-purpose router library: the whole surface is - * five static-ish paths under `/api/v1`, four with a single `{id}` path + * six static-ish paths under `/api/v1`, five with a single `{id}` path * param. Spec §3 requires distinguishing "path doesn't match anything" (404) * from "path matches, method doesn't" (405 + `Allow` header) — that's the * one piece of behavior worth a shared helper, so `index.ts` doesn't have to @@ -36,6 +36,12 @@ const CONVERSATION_REPLIES: RouteDef = { methods: ['POST'], } +/** `/api/v1/conversations/{id}/notes` — internal note (spec §4c, v1.1), POST only. */ +const CONVERSATION_NOTES: RouteDef = { + pattern: /^\/api\/v1\/conversations\/(?[^/]+)\/notes$/, + methods: ['POST'], +} + /** `/api/v1/conversations/{id}/tags` — replace the tag set (spec §4e, v1.1), PUT only. */ const CONVERSATION_TAGS: RouteDef = { pattern: /^\/api\/v1\/conversations\/(?[^/]+)\/tags$/, @@ -53,6 +59,7 @@ const ROUTES: readonly RouteDef[] = [ CONVERSATIONS_LIST, CONVERSATION_ITEM, CONVERSATION_REPLIES, + CONVERSATION_NOTES, CONVERSATION_TAGS, CONVERSATION_ASSIGNEE, ] @@ -64,6 +71,7 @@ export type RouteMatch = | { kind: 'conversation-patch'; id: string } | { kind: 'conversation-delete'; id: string } | { kind: 'conversation-reply'; id: string } + | { kind: 'conversation-note'; id: string } | { kind: 'conversation-tags'; id: string } | { kind: 'conversation-assignee'; id: string } | { kind: 'method-not-allowed'; allow: string[] } @@ -104,6 +112,9 @@ export function matchRoute(method: string, pathname: string): RouteMatch { if (route === CONVERSATION_REPLIES) { return { kind: 'conversation-reply', id } } + if (route === CONVERSATION_NOTES) { + return { kind: 'conversation-note', id } + } if (route === CONVERSATION_TAGS) { return { kind: 'conversation-tags', id } } diff --git a/src/db/migrate.test.ts b/src/db/migrate.test.ts index 011a29d..bf13b47 100644 --- a/src/db/migrate.test.ts +++ b/src/db/migrate.test.ts @@ -46,6 +46,7 @@ describe('migrate', () => { { id: 4, name: 'four_state_conversation_status' }, { id: 5, name: 'conversation_number' }, { id: 6, name: 'tags_and_assignee' }, + { id: 7, name: 'note_thread_direction' }, ]) }) @@ -55,7 +56,15 @@ describe('migrate', () => { await migrate(db) // must not throw (e.g. "relation already exists") const rows = await db.query<{ id: number }>('SELECT id FROM _migrations ORDER BY id') - expect(rows).toEqual([{ id: 1 }, { id: 2 }, { id: 3 }, { id: 4 }, { id: 5 }, { id: 6 }]) + expect(rows).toEqual([ + { id: 1 }, + { id: 2 }, + { id: 3 }, + { id: 4 }, + { id: 5 }, + { id: 6 }, + { id: 7 }, + ]) }) it('migration 002 ties delivery_status to direction: inbound must be NULL, outbound must be pending/sent/failed', async () => { @@ -441,4 +450,50 @@ describe('migrate', () => { ]), ).rejects.toThrow() }) + + it("migration 007 admits 'note' threads with NULL delivery status only; existing direction rules stay intact", async () => { + const database = await createPgliteDb() + db = database + await migrate(database) + + const [conversation] = await database.query<{ id: string }>( + 'INSERT INTO conversations (customer_email) VALUES ($1) RETURNING id', + ['customer@example.test'], + ) + + // A note with NULL delivery status is legal… + const [note] = await database.query<{ delivery_status: string | null }>( + `INSERT INTO threads (conversation_id, direction, from_address, body_text) + VALUES ($1, 'note', $2, 'internal context') RETURNING delivery_status`, + [conversation.id, 'support@example.test'], + ) + expect(note.delivery_status).toBeNull() + + // …a note with ANY delivery status is not (delivery is not a concept + // for a message that is never sent)… + await expect( + database.query( + `INSERT INTO threads (conversation_id, direction, from_address, delivery_status) + VALUES ($1, 'note', $2, 'sent')`, + [conversation.id, 'support@example.test'], + ), + ).rejects.toThrow() + + // …and the pre-007 rules survived the constraint swap: outbound still + // must carry a status, and an unknown direction is still rejected. + await expect( + database.query( + `INSERT INTO threads (conversation_id, direction, from_address, delivery_status) + VALUES ($1, 'outbound', $2, NULL)`, + [conversation.id, 'support@example.test'], + ), + ).rejects.toThrow() + await expect( + database.query( + `INSERT INTO threads (conversation_id, direction, from_address) + VALUES ($1, 'bogus', $2)`, + [conversation.id, 'support@example.test'], + ), + ).rejects.toThrow() + }) }) diff --git a/src/db/migrate.ts b/src/db/migrate.ts index 5b1567b..2dfe1d1 100644 --- a/src/db/migrate.ts +++ b/src/db/migrate.ts @@ -266,6 +266,38 @@ ALTER TABLE conversations ADD COLUMN assignee text; ALTER TABLE conversations ADD CONSTRAINT conversations_assignee_check CHECK (assignee IS NULL OR assignee = 'me'); ` +/** + * Migration 007 — the `note` thread direction (HT-28; + * specs/api/agent-inbox-v1.md §4c, v1.1). + * + * An internal note is Agent-only context on a conversation: it rides the + * `threads` table like mail but is NEVER emailed — no reply token, no outbox + * row, invisible to the delivery worker (whose queries all scope to + * `direction = 'outbound'`). + * + * Two constraint swaps, both drop-then-re-add (constraints cannot be + * altered in place), neither needing a backfill — every existing row + * satisfies the widened versions as-is: + * + * - `threads_direction_check` (migration 001's inline column CHECK, under + * Postgres's default `__check` naming) widens to admit + * `'note'`. + * - `threads_delivery_status_by_direction` (migration 002): a note must + * have a NULL `delivery_status`, exactly like inbound — delivery is not a + * concept for a message that is never sent. Without this swap the OLD + * constraint would reject every note row (a note satisfies neither of its + * two arms), so the two swaps ship together or not at all. + */ +const MIGRATION_007_NOTE_DIRECTION = ` +ALTER TABLE threads DROP CONSTRAINT threads_direction_check; +ALTER TABLE threads ADD CONSTRAINT threads_direction_check CHECK (direction IN ('inbound','outbound','note')); +ALTER TABLE threads DROP CONSTRAINT threads_delivery_status_by_direction; +ALTER TABLE threads ADD CONSTRAINT threads_delivery_status_by_direction CHECK ( + (direction IN ('inbound','note') AND delivery_status IS NULL) + OR (direction = 'outbound' AND delivery_status IS NOT NULL AND delivery_status IN ('pending','sent','failed')) +); +` + /** * Every migration, in the order they must apply. `id` is the sole ordering * key (ascending) — array position is not relied upon, so re-sorting this @@ -298,6 +330,11 @@ const MIGRATIONS: Migration[] = [ name: 'tags_and_assignee', sql: MIGRATION_006_TAGS_AND_ASSIGNEE, }, + { + id: 7, + name: 'note_thread_direction', + sql: MIGRATION_007_NOTE_DIRECTION, + }, ] /** diff --git a/src/store/conversations.test.ts b/src/store/conversations.test.ts index a360ec9..5b9f1ed 100644 --- a/src/store/conversations.test.ts +++ b/src/store/conversations.test.ts @@ -1054,4 +1054,55 @@ describe('createConversationStore', () => { expect(raw?.assignee).toBeNull() }) }) + + describe('note threads (HT-28, spec §4c v1.1)', () => { + it('a note appends with null delivery status, bumps updated_at, but NEVER reopens a closed conversation', async () => { + const { db, store } = await freshStore() + const { conversationId } = await store.createConversation(newConversation()) + await setStatus(db, conversationId, 'closed') + await setUpdatedAt(db, conversationId, new Date('2020-01-01T00:00:00.000Z')) + + const result = await store.appendThread(conversationId, { + direction: 'note', + messageId: null, + fromAddress: 'support@example.test', + bodyText: 'Finance context: PO required on every invoice.', + }) + expect(result).toMatchObject({ ok: true, created: true }) + + const conversation = await store.getConversation(conversationId) + // Still closed — a note is not the customer coming back (spec §4c)… + expect(conversation?.status).toBe('closed') + // …but it IS activity: the conversation resurfaces in the inbox. + expect(conversation?.updatedAt.getTime()).toBeGreaterThan( + new Date('2020-01-01T00:00:00.000Z').getTime(), + ) + const note = conversation?.threads.at(-1) + expect(note).toMatchObject({ + direction: 'note', + deliveryStatus: null, + messageId: null, + bodyText: 'Finance context: PO required on every invoice.', + }) + }) + + it('the delivery worker can never see a note — listDeliverableThreads is outbound-scoped', async () => { + const { db, store } = await freshStore() + const { conversationId } = await store.createConversation(newConversation()) + const appended = await store.appendThread(conversationId, { + direction: 'note', + messageId: null, + fromAddress: 'support@example.test', + bodyText: 'never send me', + }) + if (!appended.ok) throw new Error('unreachable') + // Age the note far past any staleness threshold — it must STILL be + // invisible to the retry sweep (charter invariant #5 adjacency: a note + // reaching the send path would be a bug, per spec §4c). + await setCreatedAt(db, appended.threadId, new Date('2020-01-01T00:00:00.000Z')) + + const eligible = await store.listDeliverableThreads({ staleAfterMs: 0, batchSize: 50 }) + expect(eligible).toEqual([]) + }) + }) }) diff --git a/src/store/conversations.ts b/src/store/conversations.ts index dde0139..efd92f0 100644 --- a/src/store/conversations.ts +++ b/src/store/conversations.ts @@ -115,7 +115,8 @@ export interface NewThread { * has no such circularity). */ id?: string - direction: 'inbound' | 'outbound' + /** `'note'` (v1.1, HT-28) is Agent-only context — never emailed, no delivery concept; see spec §4c. */ + direction: 'inbound' | 'outbound' | 'note' /** * The RFC `Message-ID` of this message, verbatim. For an inbound * message this is whatever the sending client wrote (or `null` if @@ -166,7 +167,7 @@ export interface NewConversation { export interface StoredThread { id: string conversationId: string - direction: 'inbound' | 'outbound' + direction: 'inbound' | 'outbound' | 'note' messageId: string | null inReplyTo: string | null fromAddress: string @@ -685,9 +686,11 @@ export function createConversationStore(db: Db): ConversationStore { // the conversation not at all — no reopen, no updated_at bump. Only // a genuinely new row counts as new activity on the conversation. // Reopen policy (spec §4a, v1.1): closed OR spam → active; pending - // deliberately stays pending (see the module doc). + // deliberately stays pending (see the module doc). A NOTE never + // reopens anything (spec §4c — noting a closed conversation is not + // the customer coming back), but it IS activity: updated_at bumps. if (created) { - if (row.status === 'closed' || row.status === 'spam') { + if ((row.status === 'closed' || row.status === 'spam') && thread.direction !== 'note') { await tx.query( "UPDATE conversations SET status = 'active', updated_at = now() WHERE id = $1", [conversationId],