diff --git a/src/main/agent-runtime/runtime-bridge.ts b/src/main/agent-runtime/runtime-bridge.ts index 67a43fd4..88087b30 100644 --- a/src/main/agent-runtime/runtime-bridge.ts +++ b/src/main/agent-runtime/runtime-bridge.ts @@ -562,16 +562,16 @@ export class RuntimeBridge { // as plain user turns in its context. // // Replay safety lives entirely on the `agent_message_projections` - // ledger. We deliberately do NOT touch `agent_messages.read_at` - // here even though that column exists: "we put the message into - // the agent's transcript" and "the agent has acknowledged the - // message" are two different facts, and welding them onto one - // column made every projected message immediately invisible to - // the agent's `?unreadOnly=true` API call. The agent then read - // its IC prompt ("respond to ALL unread messages"), called the - // API, got an empty list, and posted a status comment instead of - // replying. The explicit POST /messages/{id}/read is now the - // sole signal that flips `read_at`. + // ledger. "We put the message into the agent's transcript" and + // "the agent has acknowledged the message" are two different + // facts, tracked in two different tables: projections are append- + // only on dispatch, while reads are inserted into + // `agent_message_reads` only when the agent POSTs + // /messages/{id}/read. Conflating the two (as an earlier build + // that wrote to `agent_messages.read_at` at projection time did) + // made every projected message immediately invisible to + // `?unreadOnly=true`, so agents posted status comments instead of + // replies. The explicit read endpoint is the sole signal. const store = this.getOrCreateStore(payload.taskSessionId); const inboxRecords = this.deps.db.listAgentInboxUnread(payload.companyId, payload.agentId); if (inboxRecords.length > 0) { diff --git a/src/main/agent-runtime/runtime-message-injector.ts b/src/main/agent-runtime/runtime-message-injector.ts index 5f633b38..1dc56c1b 100644 --- a/src/main/agent-runtime/runtime-message-injector.ts +++ b/src/main/agent-runtime/runtime-message-injector.ts @@ -134,10 +134,10 @@ export class RuntimeMessageInjector { * the recipient is woken so the next `prepareRun` picks the message * up through the regular inbox projection. * - * `read_at` is intentionally NOT touched here: "the worker queued - * this message onto the agent" is not "the agent replied". The - * agent flips `read_at` itself by POSTing /messages/{id}/read once - * it has actually processed the row. + * The read receipt is intentionally NOT written here: "the worker + * queued this message onto the agent" is not "the agent replied". + * The agent inserts into `agent_message_reads` itself by POSTing + * /messages/{id}/read once it has actually processed the row. */ handleSteerAck(correlationId: string, accepted: boolean): void { this.resolvePending(correlationId, accepted, "worker_ack"); diff --git a/src/main/database/messages.ts b/src/main/database/messages.ts index 9c0abb0a..fb624c08 100644 --- a/src/main/database/messages.ts +++ b/src/main/database/messages.ts @@ -107,10 +107,15 @@ export function buildMessageMethods( getAgentMessage(messageId: string, companyId: string): AgentMessageRecord | null { const row = getDb() .prepare( - "select id, company_id as companyId, from_agent_id as fromAgentId, to_agent_id as toAgentId, sender_kind as senderKind, channel, channel_target_id as channelTargetId, subject, body, priority, read_at as readAt, parent_message_id as parentMessageId, attachments_json as attachmentsJson, created_at as createdAt from agent_messages where id = ? and company_id = ?", + "select id, company_id as companyId, from_agent_id as fromAgentId, to_agent_id as toAgentId, sender_kind as senderKind, channel, channel_target_id as channelTargetId, subject, body, priority, parent_message_id as parentMessageId, attachments_json as attachmentsJson, created_at as createdAt from agent_messages where id = ? and company_id = ?", ) - .get(messageId, companyId) as AgentMessageRecord | undefined; - return row ?? null; + .get(messageId, companyId) as Omit | undefined; + if (!row) return null; + // `readAt` is a per-viewer computed field resolved from + // agent_message_reads at the callers that need it (inbox + // projection, renderer snapshot). This single-row lookup has no + // viewer context, so the field is surfaced as `null`. + return { ...row, readAt: null }; }, listAgentMessages( @@ -125,8 +130,12 @@ export function buildMessageMethods( }, ): AgentMessageRecord[] { const limit = opts?.limit ?? 200; + // `readAt` is resolved from `agent_message_reads` further down, + // per viewer (BOARD_READER_ID for the operator-wide path, the + // specific agent id for the per-agent path). The column lives + // outside the row itself since v49. const selectCols = - "select id, company_id as companyId, from_agent_id as fromAgentId, to_agent_id as toAgentId, sender_kind as senderKind, channel, channel_target_id as channelTargetId, subject, body, priority, read_at as readAt, parent_message_id as parentMessageId, attachments_json as attachmentsJson, created_at as createdAt from agent_messages"; + "select id, company_id as companyId, from_agent_id as fromAgentId, to_agent_id as toAgentId, sender_kind as senderKind, channel, channel_target_id as channelTargetId, subject, body, priority, parent_message_id as parentMessageId, attachments_json as attachmentsJson, created_at as createdAt from agent_messages"; let sql = `${selectCols} where company_id = ?`; const params: SQLInputValue[] = [companyId]; if (opts?.messageId) { @@ -148,7 +157,7 @@ export function buildMessageMethods( sql += " limit ?"; params.push(limit); } - const rows = readAll(sql, ...params).reverse(); + const rows = readAll>(sql, ...params).reverse(); const boardReadReceiptMap = new Map( readAll<{ messageId: string; readAt: string }>( `select reads.message_id as messageId, reads.read_at as readAt @@ -219,7 +228,7 @@ export function buildMessageMethods( sql += " limit ?"; params.push(limit); } - const rows = readAll(sql, ...params).reverse(); + const rows = readAll>(sql, ...params).reverse(); const readReceiptMap = new Map( readAll<{ messageId: string; readAt: string }>( @@ -260,7 +269,7 @@ export function buildMessageMethods( const pattern = `%${escaped}%`; let sql = `SELECT id, company_id AS companyId, from_agent_id AS fromAgentId, to_agent_id AS toAgentId, sender_kind AS senderKind, channel, channel_target_id AS channelTargetId, - subject, body, priority, read_at AS readAt, parent_message_id AS parentMessageId, + subject, body, priority, parent_message_id AS parentMessageId, attachments_json AS attachmentsJson, created_at AS createdAt FROM agent_messages WHERE company_id = ? AND (subject LIKE ? ESCAPE '\\' OR body LIKE ? ESCAPE '\\')`; const params: SQLInputValue[] = [companyId, pattern, pattern]; @@ -274,7 +283,11 @@ export function buildMessageMethods( } sql += " ORDER BY created_at DESC LIMIT ?"; params.push(opts?.limit ?? 50); - return readAll(sql, ...params); + // Operator-facing search has no viewer context — `readAt` is + // surfaced as `null`. Callers that need per-viewer read state + // pull it from `agent_message_reads` separately (inbox projection, + // snapshot). + return readAll>(sql, ...params).map((row) => ({ ...row, readAt: null })); }, markMessageReadForReader( @@ -301,12 +314,11 @@ export function buildMessageMethods( * department when agent belongs, project when assigned/lead). * * "Unread" is defined uniformly across every channel: no row in - * `agent_message_reads` for (message_id, agent_id). The legacy - * `agent_messages.read_at` column is never written anywhere in the - * codebase and must not be used as a read signal — it only appears in - * SELECT lists for wire-shape compatibility and is overwritten from the - * read-receipts table by every caller that surfaces `readAt` to agents - * or the renderer. + * `agent_message_reads` for (message_id, agent_id). The row's + * `readAt` field is always `null` here because callers want the + * unread set — per-viewer read state lives exclusively in the + * `agent_message_reads` join table since v49 dropped the redundant + * `agent_messages.read_at` column. * * Messages where `from_agent_id = agentId` are excluded (agents don't * inject their own sent messages back into their transcript). @@ -362,9 +374,9 @@ export function buildMessageMethods( } const selectCols = - "select id, company_id as companyId, from_agent_id as fromAgentId, to_agent_id as toAgentId, sender_kind as senderKind, channel, channel_target_id as channelTargetId, subject, body, priority, read_at as readAt, parent_message_id as parentMessageId, attachments_json as attachmentsJson, created_at as createdAt from agent_messages"; + "select id, company_id as companyId, from_agent_id as fromAgentId, to_agent_id as toAgentId, sender_kind as senderKind, channel, channel_target_id as channelTargetId, subject, body, priority, parent_message_id as parentMessageId, attachments_json as attachmentsJson, created_at as createdAt from agent_messages"; const sql = `${selectCols} where company_id = ? and from_agent_id != ? and (${visibilityClauses.join(" or ")}) order by created_at asc`; - const rows = readAll(sql, companyId, agentId, ...visibilityParams); + const rows = readAll>(sql, companyId, agentId, ...visibilityParams).map((row) => ({ ...row, readAt: null })); const readIds = new Set( readAll<{ messageId: string }>( diff --git a/src/main/database/migrations/v49-v49.ts b/src/main/database/migrations/v49-v49.ts new file mode 100644 index 00000000..2e35ba1c --- /dev/null +++ b/src/main/database/migrations/v49-v49.ts @@ -0,0 +1,40 @@ +import log from "electron-log/main.js"; +import type { Migration, MigrationDeps } from "./types"; +import { columnExists, tableExists } from "./types"; + +/** + * Drop the unused `agent_messages.read_at` column. + * + * The column was added in v11 when reads were tracked per-message on a + * single shared receipt. v12 moved read tracking to the + * `agent_message_reads` join table, which is now the sole source of + * truth: every consumer that surfaces `readAt` (renderer snapshot, + * per-agent inbox, board receipts) resolves it from that table. + * `agent_messages.read_at` has not been written by any production path + * for multiple releases; leaving it around just forced every SELECT to + * carry a NULL column through wire-shape layers and invited the next + * maintainer to accidentally read stale data from it. + */ +export function createMigrationsV49V49(deps: MigrationDeps): Migration[] { + const { db } = deps; + return [ + { + version: 49, + name: "drop_agent_messages_read_at", + fn: () => { + if (!tableExists(db, "agent_messages")) { + log.info("[migration v49] agent_messages table missing; skipping"); + return; + } + if (!columnExists(db, "agent_messages", "read_at")) { + log.info("[migration v49] agent_messages.read_at already absent; skipping"); + return; + } + db.exec("ALTER TABLE agent_messages DROP COLUMN read_at"); + log.info( + "[migration v49] dropped unused agent_messages.read_at column — reads are tracked per-reader in agent_message_reads", + ); + }, + }, + ]; +} diff --git a/src/main/database/schema.ts b/src/main/database/schema.ts index a41f7b68..128ff193 100644 --- a/src/main/database/schema.ts +++ b/src/main/database/schema.ts @@ -14,6 +14,7 @@ import { createMigrationsV45V45 } from "./migrations/v45-v45"; import { createMigrationsV46V46 } from "./migrations/v46-v46"; import { createMigrationsV47V47 } from "./migrations/v47-v47"; import { createMigrationsV48V48 } from "./migrations/v48-v48"; +import { createMigrationsV49V49 } from "./migrations/v49-v49"; import { columnExists, nowIso, @@ -208,6 +209,7 @@ export function runMigrations( ...createMigrationsV46V46(deps), ...createMigrationsV47V47(deps), ...createMigrationsV48V48(deps), + ...createMigrationsV49V49(deps), ]; // Pre-migration snapshot. If any migration is pending we take a full diff --git a/src/main/database/snapshot.ts b/src/main/database/snapshot.ts index 44393504..cb94b6bc 100644 --- a/src/main/database/snapshot.ts +++ b/src/main/database/snapshot.ts @@ -41,8 +41,9 @@ export function buildSnapshotMethods( // view has exactly one human viewer (the Board operator), so the // renderer snapshot fills it from that reader's agent_message_reads // rows. Agent-side queries (listAgentMessages) resolve it against - // the current agent's receipts. The raw agent_messages.read_at - // column is never read on any path and stays NULL by design. + // the current agent's receipts. The old `agent_messages.read_at` + // column was dropped in migration v49 — the join table is the + // sole source of truth. const boardReadReceiptMap = new Map( readAll<{ messageId: string; readAt: string }>( `select reads.message_id as messageId, reads.read_at as readAt @@ -183,10 +184,9 @@ export function buildSnapshotMethods( createdAt: r.created_at, updatedAt: r.updated_at, })), agentMessages: readAll( - // The raw `read_at` column is intentionally omitted from the - // SELECT list. It is never written — readAt semantics live in - // the agent_message_reads receipts table, resolved against the - // current viewer above. + // readAt semantics live in the agent_message_reads receipts + // table, resolved against the current viewer above. The old + // `agent_messages.read_at` column was dropped in v49. `select id, company_id, from_agent_id, to_agent_id, sender_kind, channel, channel_target_id, subject, body, priority, parent_message_id, attachments_json, created_at from agent_messages${companyWhere} order by created_at desc limit 1000`, ...companyArgs, ).map((r) => ({