diff --git a/app/src/providers/ChatRuntimeProvider.tsx b/app/src/providers/ChatRuntimeProvider.tsx index 0565d8879a..598de6168d 100644 --- a/app/src/providers/ChatRuntimeProvider.tsx +++ b/app/src/providers/ChatRuntimeProvider.tsx @@ -218,6 +218,26 @@ function chatDoneExtraMetadata(event: ChatDoneEvent): Record | return Object.keys(meta).length > 0 ? meta : undefined; } +/** + * Message id for a reply the CORE already persisted before announcing it. + * + * Core-initiated turns (`client_id === 'system'`: autonomous task sessions and + * background sub-agent result delivery via `run_system_turn_on_thread`) write + * their own closing message — `task_session::append_final`, keyed + * `agent:` — and only then emit `chat_done` / `chat_error` with that + * run id as `request_id`. Reusing the same id here makes our own + * `addInferenceResponse` append collapse onto the core's row (the conversation + * store is idempotent by message id) instead of persisting a second copy that + * rendered as a duplicate reply under the answer (#5933). Interactive turns + * keep their generated ids: nothing else has persisted them. + */ +function corePersistedMessageId(event: { + client_id?: string; + request_id?: string; +}): string | undefined { + return event.client_id === 'system' && event.request_id ? `agent:${event.request_id}` : undefined; +} + /** * Map a `chat_done` event's holistic usage onto the `recordChatTurnUsage` * payload. Prefers the structured `usage` object (tokens + cost + context window @@ -1175,6 +1195,7 @@ const ChatRuntimeProvider = ({ children }: { children: React.ReactNode }) => { addInferenceResponse({ content: event.full_response, threadId: event.thread_id, + messageId: corePersistedMessageId(event), extraMetadata: chatDoneExtraMetadata(event), }) ).unwrap(); @@ -1210,6 +1231,7 @@ const ChatRuntimeProvider = ({ children }: { children: React.ReactNode }) => { addInferenceResponse({ content: event.full_response, threadId: event.thread_id, + messageId: corePersistedMessageId(event), extraMetadata: chatDoneExtraMetadata(event), }) ).unwrap(); @@ -1333,9 +1355,23 @@ const ChatRuntimeProvider = ({ children }: { children: React.ReactNode }) => { // surfacing it tells the user *why* the turn failed instead of a blanket apology. // The hardcoded constant is only a last-resort fallback for an empty/missing message. const errorContent = event.message || USER_FACING_AGENT_ERROR_MESSAGE; - if (!(lastMsg?.sender === 'agent' && lastMsg?.content === errorContent)) { + // A core-owned failure carries a deterministic id, so dedupe on that + // rather than on the text. Two runs can fail with byte-identical + // content — the same upstream provider message, or the generic + // fallback above — and a text check would then read the previous + // run's row as this one and drop the current failure from the cache. + // Interactive turns have no pre-persisted id and keep the text check. + const errorMessageId = corePersistedMessageId(event); + const alreadyPresent = errorMessageId + ? threadMessages.some(message => message.id === errorMessageId) + : lastMsg?.sender === 'agent' && lastMsg?.content === errorContent; + if (!alreadyPresent) { void dispatch( - addInferenceResponse({ content: errorContent, threadId: event.thread_id }) + addInferenceResponse({ + content: errorContent, + threadId: event.thread_id, + messageId: errorMessageId, + }) ); } diff --git a/app/src/providers/__tests__/ChatRuntimeProvider.test.tsx b/app/src/providers/__tests__/ChatRuntimeProvider.test.tsx index 9044213625..91f9498273 100644 --- a/app/src/providers/__tests__/ChatRuntimeProvider.test.tsx +++ b/app/src/providers/__tests__/ChatRuntimeProvider.test.tsx @@ -482,6 +482,147 @@ describe('ChatRuntimeProvider — dedupe, proactive resolution, mid-turn invaria await waitFor(() => expect(mockRefetchSnapshot).toHaveBeenCalledTimes(1)); }); + it('persists a core-initiated (system) turn under the id the core already wrote (#5933)', async () => { + const listeners = renderProvider(); + + act(() => { + listeners.onDone?.({ + thread_id: 't-sys', + request_id: 'bgdeliver-1', + client_id: 'system', + full_response: 'Same two issues as before', + rounds_used: 1, + }); + }); + + // The same `agent:` id `task_session::append_final` used, so the + // core's idempotent store collapses this append onto its own row instead + // of keeping a second copy of the reply. + await waitFor(() => + expect(threadApi.appendMessage).toHaveBeenCalledWith( + 't-sys', + expect.objectContaining({ + id: 'agent:bgdeliver-1', + sender: 'agent', + content: 'Same two issues as before', + extraMetadata: expect.objectContaining({ requestId: 'bgdeliver-1' }), + }) + ) + ); + }); + + it('persists a core-initiated (system) turn failure under the same core id', async () => { + const listeners = renderProvider(); + + act(() => { + listeners.onError?.({ + thread_id: 't-sys-err', + request_id: 'bgdeliver-2', + client_id: 'system', + message: 'Run failed: boom', + error_type: 'inference', + round: null, + }); + }); + + await waitFor(() => + expect(threadApi.appendMessage).toHaveBeenCalledWith( + 't-sys-err', + expect.objectContaining({ id: 'agent:bgdeliver-2', sender: 'agent' }) + ) + ); + }); + + it('persists a second core failure with identical text under its own id (#5933)', async () => { + const listeners = renderProvider(); + + act(() => { + listeners.onError?.({ + thread_id: 't-sys-err-dup', + request_id: 'bgdeliver-a', + client_id: 'system', + message: 'Run failed: boom', + error_type: 'inference', + round: null, + }); + }); + + await waitFor(() => + expect(threadApi.appendMessage).toHaveBeenCalledWith( + 't-sys-err-dup', + expect.objectContaining({ id: 'agent:bgdeliver-a' }) + ) + ); + + // Same thread, same failure text, a different run. The core persisted + // this one as `agent:bgdeliver-b`; deduping on the last row's content + // would read the previous run's row as this one and drop the new + // failure from the cache entirely. + act(() => { + listeners.onError?.({ + thread_id: 't-sys-err-dup', + request_id: 'bgdeliver-b', + client_id: 'system', + message: 'Run failed: boom', + error_type: 'inference', + round: null, + }); + }); + + await waitFor(() => + expect(threadApi.appendMessage).toHaveBeenCalledWith( + 't-sys-err-dup', + expect.objectContaining({ id: 'agent:bgdeliver-b', sender: 'agent' }) + ) + ); + expect(threadApi.appendMessage).toHaveBeenCalledTimes(2); + }); + + it('still suppresses a repeat of the same core failure event', async () => { + const listeners = renderProvider(); + const fire = () => + act(() => { + listeners.onError?.({ + thread_id: 't-sys-err-same', + request_id: 'bgdeliver-c', + client_id: 'system', + message: 'Run failed: boom', + error_type: 'inference', + round: null, + }); + }); + + fire(); + await waitFor(() => expect(threadApi.appendMessage).toHaveBeenCalledTimes(1)); + + // The row is in the cache under `agent:bgdeliver-c` now, so the id check + // recognises the redelivery. Trading the content check for an id check + // must not turn a duplicate event into a duplicate append. + fire(); + await act(async () => { + await Promise.resolve(); + }); + expect(threadApi.appendMessage).toHaveBeenCalledTimes(1); + }); + + it('keeps a generated id for an interactive chat_done (nothing else persisted it)', async () => { + const listeners = renderProvider(); + + act(() => { + listeners.onDone?.({ + thread_id: 't-user', + request_id: 'r-user', + full_response: 'hi', + rounds_used: 1, + }); + }); + + await waitFor(() => expect(threadApi.appendMessage).toHaveBeenCalledTimes(1)); + const [, persisted] = vi.mocked(threadApi.appendMessage).mock.calls[0]; + expect(persisted.id).not.toBe('agent:r-user'); + expect(persisted.sender).toBe('agent'); + }); + it('stores a parked plan review from the plan_review_request event', () => { const listeners = renderProvider(); act(() => { diff --git a/app/src/services/api/threadApi.test.ts b/app/src/services/api/threadApi.test.ts index 67e8baeaa9..261fc40c0c 100644 --- a/app/src/services/api/threadApi.test.ts +++ b/app/src/services/api/threadApi.test.ts @@ -58,6 +58,65 @@ describe('threadApi', () => { expect(result).toEqual(message); }); + it('folds the legacy `assistant` sender onto `agent` when listing messages (#5933)', async () => { + mockCallCoreRpc.mockResolvedValueOnce({ + data: { + messages: [ + { + id: 'user:1', + content: 'hi', + type: 'text', + extraMetadata: {}, + sender: 'user', + createdAt: '2026-04-10T12:01:00Z', + }, + { + // Written by an autonomous task run before the core switched its + // closing message to the `agent` vocabulary. + id: 'assistant:legacy', + content: 'done', + type: 'text', + extraMetadata: { scope: 'autonomous_task_result' }, + sender: 'assistant', + createdAt: '2026-04-10T12:02:00Z', + }, + ], + count: 2, + }, + }); + + const { threadApi } = await import('./threadApi'); + const result = await threadApi.getThreadMessages('default-thread'); + + expect(result.count).toBe(2); + expect(result.messages.map(m => m.sender)).toEqual(['user', 'agent']); + // Everything else on the row is untouched. + expect(result.messages[1]).toMatchObject({ id: 'assistant:legacy', content: 'done' }); + }); + + it('folds the legacy `assistant` sender onto `agent` on append and update results', async () => { + const stored = { + id: 'agent:run-1', + content: 'done', + type: 'text', + extraMetadata: {}, + sender: 'assistant', + createdAt: '2026-04-10T12:02:00Z', + }; + mockCallCoreRpc.mockResolvedValueOnce({ data: stored }); + mockCallCoreRpc.mockResolvedValueOnce({ data: stored }); + + const { threadApi } = await import('./threadApi'); + const appended = await threadApi.appendMessage('default-thread', { + ...stored, + sender: 'agent', + }); + const updated = await threadApi.updateMessage('default-thread', 'agent:run-1', {}); + + expect(appended.sender).toBe('agent'); + expect(updated.sender).toBe('agent'); + }); + it('generates a thread title via threads RPC', async () => { const thread = { id: 'default-thread', diff --git a/app/src/services/api/threadApi.ts b/app/src/services/api/threadApi.ts index bf01d34bcf..a837bc3e03 100644 --- a/app/src/services/api/threadApi.ts +++ b/app/src/services/api/threadApi.ts @@ -46,6 +46,18 @@ function unwrapEnvelope(response: Envelope | T): T { const generateTitleLog = debug('threadApi.generateTitleIfNeeded'); +/** + * The core's `sender` vocabulary is `user` | `agent`, but some core writers + * stored the assistant side as `assistant` (autonomous task sessions before + * #5933, channel-session mirrors). Fold that alias onto `agent` at the transport + * boundary so every `sender === 'agent'` check in the renderers — and the + * assistant-ui role mapping — treats such a row as the assistant instead of + * painting it as a user turn. + */ +function normalizeThreadMessage(message: ThreadMessage): ThreadMessage { + return (message.sender as string) === 'assistant' ? { ...message, sender: 'agent' } : message; +} + export const threadApi = { createNewThread: async (labels?: string[]): Promise => { const response = await callCoreRpc>({ @@ -67,7 +79,8 @@ export const threadApi = { method: 'openhuman.threads_messages_list', params: { thread_id: threadId }, }); - return unwrapEnvelope(response); + const data = unwrapEnvelope(response); + return { ...data, messages: data.messages.map(normalizeThreadMessage) }; }, appendMessage: async (threadId: string, message: ThreadMessage): Promise => { @@ -75,7 +88,7 @@ export const threadApi = { method: 'openhuman.threads_message_append', params: { thread_id: threadId, message }, }); - return unwrapEnvelope(response); + return normalizeThreadMessage(unwrapEnvelope(response)); }, generateTitleIfNeeded: async (threadId: string, assistantMessage?: string): Promise => { @@ -108,7 +121,7 @@ export const threadApi = { method: 'openhuman.threads_message_update', params: { thread_id: threadId, message_id: messageId, extra_metadata: extraMetadata }, }); - return unwrapEnvelope(response); + return normalizeThreadMessage(unwrapEnvelope(response)); }, deleteThread: async (threadId: string): Promise => { diff --git a/app/src/services/chatService.ts b/app/src/services/chatService.ts index 2a493d61c7..2249ff670f 100644 --- a/app/src/services/chatService.ts +++ b/app/src/services/chatService.ts @@ -89,6 +89,14 @@ export interface TurnUsageWire { export interface ChatDoneEvent { thread_id: string; request_id?: string; + /** + * Socket.IO client that owns the turn. `"system"` marks a turn the core ran + * on its own behalf (autonomous task sessions, background sub-agent result + * delivery, cron/flow agents); such turns are broadcast to every client. + * Always on the wire (`WebChannelEvent.client_id`); declared here for the + * consumers that key off it. + */ + client_id?: string; /** Per-request monotonic ordering key stamped by the core progress bridge. */ seq?: number; full_response: string; @@ -165,6 +173,14 @@ export interface ChatInterimEvent { export interface ChatErrorEvent { thread_id: string; request_id?: string; + /** + * Socket.IO client that owns the turn. `"system"` marks a turn the core ran + * on its own behalf (autonomous task sessions, background sub-agent result + * delivery, cron/flow agents); such turns are broadcast to every client. + * Always on the wire (`WebChannelEvent.client_id`); declared here for the + * consumers that key off it. + */ + client_id?: string; message: string; error_type: | 'network' diff --git a/app/src/store/__tests__/threadSlice.test.ts b/app/src/store/__tests__/threadSlice.test.ts index 6762ab74e1..8f537ad6bd 100644 --- a/app/src/store/__tests__/threadSlice.test.ts +++ b/app/src/store/__tests__/threadSlice.test.ts @@ -444,6 +444,26 @@ describe('threadSlice addInferenceResponse thunk', () => { expect(state.activeThreadIds).toEqual({}); }); + it('replaces a cached entry that already carries the persisted id instead of duplicating it (#5933)', async () => { + // A core-initiated turn's reply is persisted by the core under + // `agent:` and announced afterwards; a thread reload can fetch that + // row before our own same-id append resolves. + const store = createStore(); + store.dispatch(setSelectedThread('t-1')); + const coreRow = makeMessage({ id: 'agent:run-1', sender: 'agent', content: 'from core' }); + mockedThreadApi.getThreadMessages.mockResolvedValueOnce({ messages: [coreRow], count: 1 }); + await store.dispatch(loadThreadMessages('t-1')); + + mockedThreadApi.appendMessage.mockResolvedValueOnce(coreRow); + await store.dispatch( + addInferenceResponse({ content: 'from core', threadId: 't-1', messageId: 'agent:run-1' }) + ); + + const state = store.getState().thread; + expect(state.messagesByThreadId['t-1']).toEqual([coreRow]); + expect(state.messages).toEqual([coreRow]); + }); + it('falls back to the selected thread when no threadId is supplied', async () => { // Under parallel inference there is no single "active" thread to fall back // to, so the legacy fallback target is now the selected thread. diff --git a/app/src/store/threadSlice.ts b/app/src/store/threadSlice.ts index 62f963f05e..e8387988a5 100644 --- a/app/src/store/threadSlice.ts +++ b/app/src/store/threadSlice.ts @@ -71,21 +71,35 @@ const initialState: ThreadState = { createThreadRequestId: null, }; +/** + * Append a persisted message to the thread cache, or replace the entry that + * already carries its id. + * + * Ids are no longer guaranteed fresh on append: a core-initiated turn's reply + * is persisted by the core under `agent:` and then persisted again by + * our `chat_done` handler under the same id (the store collapses the two — + * #5933). If a `loadThreadMessages` fetch lands between those two writes, the + * cache already holds that id when the append resolves, and a second entry + * would hand React and assistant-ui a duplicate key (assistant-ui throws on + * one). `replaceExisting` keeps its narrower contract for callers that only + * ever update a message already in the cache (reactions). + */ function appendMessageToCache( state: ThreadState, threadId: string, message: ThreadMessage, replaceExisting = false ) { - const existing = state.messagesByThreadId[threadId] ?? []; - const next = replaceExisting - ? existing.map(e => (e.id === message.id ? message : e)) - : [...existing, message]; - state.messagesByThreadId[threadId] = next; + const upsert = (list: ThreadMessage[]) => { + const present = list.some(e => e.id === message.id); + if (replaceExisting || present) { + return list.map(e => (e.id === message.id ? message : e)); + } + return [...list, message]; + }; + state.messagesByThreadId[threadId] = upsert(state.messagesByThreadId[threadId] ?? []); if (threadId === state.selectedThreadId) { - state.messages = replaceExisting - ? state.messages.map(e => (e.id === message.id ? message : e)) - : [...state.messages, message]; + state.messages = upsert(state.messages); } } diff --git a/docs/RELEASE-MANUAL-SMOKE.md b/docs/RELEASE-MANUAL-SMOKE.md index ec48eae7ed..9398a058a1 100644 --- a/docs/RELEASE-MANUAL-SMOKE.md +++ b/docs/RELEASE-MANUAL-SMOKE.md @@ -55,6 +55,7 @@ Applies to every release, all platforms. ### Cross-platform - [ ] **First launch flow completes for a brand-new user** — Fresh OS user account, no `~/.openhuman` directory. Walk through onboarding to first agent reply. Expected: no crashes, no permission deadlocks, no stale-config errors. +- [ ] **A background sub-agent result lands in Chat exactly once** — Ask for something that delegates to a background sub-agent (e.g. "how's my day looking?" with a calendar connected, or any `delegate_*` archetype), then wait for it to finish. Expected: the delivered reply appears once, as a normal agent message; no user-side (right-aligned) bubble showing raw `**markdown**`, and still one copy after switching to another thread and back (#5933). - [ ] **Auto-update download + relaunch succeeds** — Install the previous release, point the updater feed at this release, trigger an update check. Expected: download completes, relaunch installs the new binary, version string in `Settings > About` matches the release tag. - [ ] **GitHub Release notes are AI-generated from the previous release tag** — Before publishing the draft production release, inspect the GitHub Release body. Expected: notes start with a thematic H1 title, include high-level highlight sections with PR links and contributor thanks, omit a separate pull-request dump, include new-contributor thanks only when applicable, and the full compare URL is previous release tag → current release tag. - [ ] **Logging out + logging back in preserves nothing private** — Sign out, sign in as a different user. Expected: no leaked memory, threads, or skill state from the previous session (regression watch — see #900). diff --git a/docs/TEST-COVERAGE-MATRIX.md b/docs/TEST-COVERAGE-MATRIX.md index 0f5b3f666b..5bfd9efad7 100644 --- a/docs/TEST-COVERAGE-MATRIX.md +++ b/docs/TEST-COVERAGE-MATRIX.md @@ -277,6 +277,7 @@ End-to-end coverage of the agent harness via the web-chat RPC surface against an | 6.3.12 | Cross-turn sub-agent roster (durable store merge) | RU | `src/openhuman/agent/orchestration/running_subagents.rs::snapshot_and_block_scope_to_parent_and_reflect_live_status` | ✅ | `[active_subagents]` merges the in-memory registry with the per-workspace `subagent_sessions` store, so a cold-booted parent still sees earlier workers (id, status, task title) and resumes instead of re-delegating; Closed sessions excluded, other parents' sessions never leak. | | 6.3.13 | continue_subagent durable resume (no pause checkpoint) | RU | `src/openhuman/agent/orchestration/tools/tools_e2e_tests.rs::{continue_subagent_resumes_idle_durable_session_e2e,continue_subagent_without_checkpoint_or_durable_session_names_the_roster}` | ✅ | With no `ask_user_clarification` checkpoint, `continue_subagent` resolves the durable session by subagent_session_id / task id, resumes it via the reusable async path seeded with persisted history, and keeps the same durable id; missing sessions error with a pointer at the roster. | | 6.3.14 | Workflow proposal durability (async builder → chat card) | RU+VU | `src/openhuman/agent/orchestration/tools/spawn_async_subagent.rs::{extract_workflow_proposal_finds_last_proposal_tool_result,attach_workflow_proposal_persists_thread_message_and_extends_summary,attach_workflow_proposal_without_proposal_returns_summary_unchanged}`, `app/src/lib/workflows/workflowProposal.test.ts` | ✅ | A `workflow_proposal` payload in a finished async child's history is persisted as a parent-thread message (metadata scope `workflow_proposal`) and embedded in the delivery notice; the frontend rehydrates the newest unconsumed proposal into the card on thread load, and Save/Dismiss mark the source message consumed. | +| 6.3.15 | Autonomous reply persisted once (core-owned id, idempotent append) | RU+VU | `src/openhuman/agent/task_session_tests.rs::{append_final_writes_agent_outcome_keyed_by_run_id,append_final_records_failure_as_unsuccessful_agent_message,append_final_is_idempotent_per_run}`, `src/openhuman/memory/conversations/store/store_tests.rs::append_message_is_idempotent_by_message_id`, `src/openhuman/web_chat/presentation_tests.rs::single_bubble_delivery_emits_one_unsegmented_chat_done_without_reaction`, `app/src/providers/__tests__/ChatRuntimeProvider.test.tsx` (system `chat_done`/`chat_error` reuse the core id), `app/src/services/api/threadApi.test.ts` (legacy `assistant` sender folded onto `agent`), `app/src/store/__tests__/threadSlice.test.ts` (same-id cache upsert) | ✅ | #5933 — a background-delivery / autonomous-task reply is persisted by the core FIRST as `agent:` (`sender: agent`, `extraMetadata.requestId`), announced as one unsegmented `chat_done`, and the frontend's own `chat_done` append reuses that id so the id-idempotent store keeps one row; failure outcomes land as `Run failed: …` under the same id. Previously the reply was persisted twice, the second copy as `assistant`, which rendered as a user-side raw-markdown bubble. | ### 6.4 Managed Cloud File Storage diff --git a/gitbooks/developing/architecture/agent-harness.md b/gitbooks/developing/architecture/agent-harness.md index be0e613333..78a27cc951 100644 --- a/gitbooks/developing/architecture/agent-harness.md +++ b/gitbooks/developing/architecture/agent-harness.md @@ -288,7 +288,7 @@ The child run itself still uses the same runner: `wait_subagent` and `steer_subagent` accept either the durable `subagent_session_id` or the transient `task_id`; durable ids are preferred across turns. `list_subagents` shows reusable children for the current parent thread, and `close_subagent` marks a worker non-reusable and cancels it if it is still running. Inline blocking is explicit via `blocking: true`; it is no longer the default. -The synthesized archetype delegations (`delegate_*`, `build_workflow`, and the other `delegate_name` tools) follow the same contract: they route through the durable async path by default, returning an `[async_subagent_ref]` (with `subagent_session_id` + `task_id`) immediately, and the finished result is inserted into the parent chat as a new system turn via `background_completions`/`background_delivery`. They fall back to inline blocking automatically when there is no parent agent turn or no chat thread to deliver into (cron/CLI), or when `blocking: true` is passed. Cross-turn continuity comes from three pieces: the per-turn `[active_subagents]` roster merges the live in-memory registry with the durable `subagent_sessions` store (so a cold-booted orchestrator still sees earlier workers); `continue_subagent` falls back from pause checkpoints to the durable store, resuming an idle worker with its persisted history; and a `workflow_proposal` payload found in a finished child's history is persisted as a parent-thread message (`extraMetadata.scope = "workflow_proposal"`) that the frontend rehydrates into the proposal card on thread load. +The synthesized archetype delegations (`delegate_*`, `build_workflow`, and the other `delegate_name` tools) follow the same contract: they route through the durable async path by default, returning an `[async_subagent_ref]` (with `subagent_session_id` + `task_id`) immediately, and the finished result is inserted into the parent chat as a new system turn via `background_completions`/`background_delivery`. That delivery turn (`task_dispatcher::run_system_turn_on_thread`, the same runner autonomous task sessions use) persists its own closing message — `sender: "agent"`, id `agent:`, `extraMetadata.requestId = run_id` — **before** it emits `chat_done` as `client_id: "system"`; the frontend reuses that id for system turns, so its usual `chat_done` append collapses onto the same row (the conversation store is idempotent for these deterministic `agent:`-prefixed ids; every other id is UUID-fresh and keeps the constant-time append path) instead of persisting the delivered result a second time (#5933). They fall back to inline blocking automatically when there is no parent agent turn or no chat thread to deliver into (cron/CLI), or when `blocking: true` is passed. Cross-turn continuity comes from three pieces: the per-turn `[active_subagents]` roster merges the live in-memory registry with the durable `subagent_sessions` store (so a cold-booted orchestrator still sees earlier workers); `continue_subagent` falls back from pause checkpoints to the durable store, resuming an idle worker with its persisted history; and a `workflow_proposal` payload found in a finished child's history is persisted as a parent-thread message (`extraMetadata.scope = "workflow_proposal"`) that the frontend rehydrates into the proposal card on thread load. ### Spawn hierarchy and tiers diff --git a/src/openhuman/agent/task_dispatcher/executor.rs b/src/openhuman/agent/task_dispatcher/executor.rs index 368e76665e..ecbbf33ef3 100644 --- a/src/openhuman/agent/task_dispatcher/executor.rs +++ b/src/openhuman/agent/task_dispatcher/executor.rs @@ -241,27 +241,37 @@ pub(super) async fn run_autonomous( } .map_err(|e| format!("{e:#}")); - // Emit the terminal chat event so a client viewing the session stops - // "processing" and finalizes the assistant bubble — the SAME chat_done / - // chat_error the web channel emits at the end of a normal turn. The - // progress bridge only streams intermediate deltas; without this terminal - // signal the live-streamed session spins forever. Broadcast as "system" so - // any viewer of the thread receives it (frontend keys by thread_id). + // Close the run in its thread. Order matters (#5933): persist the closing + // message FIRST, announce the terminal event SECOND. A client viewing the + // thread persists whatever `chat_done` carries as well, under the same + // `agent:` id (`ChatRuntimeProvider` mirrors `append_final`'s id + // for `client_id: "system"` turns), and the conversation store is + // idempotent by message id — so the second writer collapses onto the row + // that already exists instead of leaving the duplicate reply the issue + // reported. Persisting first makes the core's row the one that exists. if let Some(thread_id) = session_thread_id.as_deref() { + // Persist the final response (or failure) as the closing agent message + // so a reopened session shows the outcome like a finished manual run — + // and so it is already there when any viewer reacts to the event below. + task_session::append_final(workspace_dir, thread_id, run_id, &result); + + // Emit the terminal chat event so a client viewing the session stops + // "processing" and finalizes the assistant bubble — the SAME chat_done / + // chat_error the web channel emits at the end of a normal turn. The + // progress bridge only streams intermediate deltas; without this terminal + // signal the live-streamed session spins forever. Broadcast as "system" so + // any viewer of the thread receives it (frontend keys by thread_id). match &result { Ok(response) => { - crate::openhuman::web_chat::presentation::deliver_response( - "system", - thread_id, - run_id, - response, - prompt, - &[], - // Background/cron turns don't surface in the chat footer; their - // token/cost spend is still captured by the global cost tracker. - None, - ) - .await; + // One bubble, never segmented: the reply was persisted as a + // single row above, and a segmented delivery would have a + // viewing client persist one row per segment beside it. + // Background/cron turns don't surface usage in the chat footer; + // their token/cost spend is still captured by the global cost + // tracker. + crate::openhuman::web_chat::presentation::deliver_response_single_bubble( + "system", thread_id, run_id, response, None, + ); } Err(err) => { crate::openhuman::web_chat::publish_web_channel_event( @@ -277,9 +287,6 @@ pub(super) async fn run_autonomous( ); } } - // Persist the final response as the closing assistant message so a - // reopened session shows the outcome like a finished manual run. - task_session::append_final(workspace_dir, thread_id, &result); } result } diff --git a/src/openhuman/agent/task_session.rs b/src/openhuman/agent/task_session.rs index 80c5e06dde..f2014e32fd 100644 --- a/src/openhuman/agent/task_session.rs +++ b/src/openhuman/agent/task_session.rs @@ -30,7 +30,7 @@ use serde_json::json; use crate::openhuman::agent::task_board::TaskBoardCard; use crate::openhuman::memory::conversations::{ - self as conversations, ConversationMessage, CreateConversationThread, + self as conversations, run_reply_message_id, ConversationMessage, CreateConversationThread, }; /// Label that lands a thread in the Conversations → Tasks tab. Mirrors the @@ -121,11 +121,23 @@ fn session_title(card: &TaskBoardCard) -> String { } /// Append the run's final response (or failure reason) to the session thread as -/// the closing `assistant` message, so a reopened session shows the outcome. +/// the closing `agent` message, so a reopened session shows the outcome. /// No-op on an empty response. Best-effort: a store failure is logged only. +/// +/// The row is keyed [`run_reply_message_id`] (`agent:`) on purpose — a +/// deterministic id, which is also what makes the store's idempotency lookup +/// apply to it. A client viewing the thread +/// persists the same reply again from the `chat_done` the run announces, and it +/// reuses this id for `client_id: "system"` turns (`ChatRuntimeProvider`), so +/// the conversation store's id idempotency collapses both writes onto one row +/// instead of the duplicate reply #5933 reported. `sender` is `agent` — the +/// vocabulary every renderer keys on; the former `assistant` painted this row +/// as a USER bubble. `requestId` carries the run id so the reply anchors to its +/// own process trail exactly like an interactive turn's answer. pub(crate) fn append_final( workspace_dir: PathBuf, thread_id: &str, + run_id: &str, outcome: &Result, ) { let (content, success) = match outcome { @@ -139,11 +151,15 @@ pub(crate) fn append_final( workspace_dir, thread_id, ConversationMessage { - id: format!("assistant:{}", uuid::Uuid::new_v4()), + id: run_reply_message_id(run_id), content, message_type: "text".to_string(), - extra_metadata: json!({ "scope": "autonomous_task_result", "success": success }), - sender: "assistant".to_string(), + extra_metadata: json!({ + "scope": "autonomous_task_result", + "success": success, + "requestId": run_id, + }), + sender: "agent".to_string(), created_at: chrono::Utc::now().to_rfc3339(), }, ) { diff --git a/src/openhuman/agent/task_session_tests.rs b/src/openhuman/agent/task_session_tests.rs index a9f67c3aea..de3ad69dcd 100644 --- a/src/openhuman/agent/task_session_tests.rs +++ b/src/openhuman/agent/task_session_tests.rs @@ -59,22 +59,61 @@ fn creates_top_level_tasks_thread_and_seeds_prompt() { } #[test] -fn append_final_writes_assistant_outcome() { +fn append_final_writes_agent_outcome_keyed_by_run_id() { let ws = temp_ws(); let id = create_session_thread(ws.clone(), &card("X"), "run-2", "prompt").expect("thread"); - append_final(ws.clone(), &id, &Ok("All done.".to_string())); + append_final(ws.clone(), &id, "run-2", &Ok("All done.".to_string())); let msgs = conversations::get_messages(ws, &id).expect("messages"); let last = msgs.last().expect("has messages"); - assert_eq!(last.sender, "assistant"); + // `agent` is the sender every renderer keys on; `assistant` used to land + // here and painted the closing reply as a USER bubble (#5933). + assert_eq!(last.sender, "agent"); assert_eq!(last.content, "All done."); + // Deterministic per run so a client that also persists the announced + // reply under `agent:` collapses onto this row. + assert_eq!(last.id, "agent:run-2"); + assert_eq!(last.extra_metadata["requestId"], "run-2"); + assert_eq!(last.extra_metadata["success"], true); + assert_eq!(last.extra_metadata["scope"], "autonomous_task_result"); +} + +#[test] +fn append_final_records_failure_as_unsuccessful_agent_message() { + let ws = temp_ws(); + let id = create_session_thread(ws.clone(), &card("X"), "run-5", "prompt").expect("thread"); + append_final(ws.clone(), &id, "run-5", &Err("boom".to_string())); + + let msgs = conversations::get_messages(ws, &id).expect("messages"); + let last = msgs.last().expect("has messages"); + assert_eq!(last.sender, "agent"); + assert_eq!(last.id, "agent:run-5"); + assert_eq!(last.content, "Run failed: boom"); + assert_eq!(last.extra_metadata["success"], false); +} + +#[test] +fn append_final_is_idempotent_per_run() { + // The core persists first, then a viewing client persists the announced + // reply under the same id — the store must keep exactly one row. + let ws = temp_ws(); + let id = create_session_thread(ws.clone(), &card("X"), "run-4", "prompt").expect("thread"); + append_final(ws.clone(), &id, "run-4", &Ok("All done.".to_string())); + append_final(ws.clone(), &id, "run-4", &Ok("All done.".to_string())); + + let msgs = conversations::get_messages(ws, &id).expect("messages"); + assert_eq!( + msgs.iter().filter(|m| m.sender == "agent").count(), + 1, + "a second append for the same run must not add a second closing message" + ); } #[test] fn append_final_skips_empty_response() { let ws = temp_ws(); let id = create_session_thread(ws.clone(), &card("X"), "run-3", "prompt").expect("thread"); - append_final(ws.clone(), &id, &Ok(" ".to_string())); + append_final(ws.clone(), &id, "run-3", &Ok(" ".to_string())); let msgs = conversations::get_messages(ws, &id).expect("messages"); assert_eq!( @@ -89,3 +128,44 @@ fn empty_title_falls_back_to_generic_label() { assert_eq!(session_title(&card(" ")), "Autonomous task"); assert_eq!(session_title(&card("Real title")), "Real title"); } + +/// The closing row that lands first is the one that survives, and the loser's +/// content is discarded — so the *order* in `run_autonomous` decides which +/// writer's text a reader sees. +/// +/// `append_message` is idempotent by id and returns the **stored** row, so the +/// second write of `agent:` is dropped whole, not merged. That is +/// correct and is what collapses the duplicate in #5933, but it means the +/// persist-before-announce ordering is load-bearing beyond tidiness: if the +/// terminal event were announced first, a viewing client would persist +/// `chat_done`'s text and the core's later `append_final` of a *failure* would +/// be silently discarded, leaving a thread that claims the run succeeded. +/// +/// Pinned here rather than by driving `run_autonomous`, which needs a live +/// agent. This is the property that would break if the two statements were +/// swapped; the existing `append_final_is_idempotent_per_run` writes the same +/// content twice and so cannot see it. +#[test] +fn the_first_closing_row_wins_and_a_later_same_id_append_is_discarded() { + let ws = temp_ws(); + let id = create_session_thread(ws.clone(), &card("X"), "run-6", "prompt").expect("thread"); + + // The core persists the real outcome first — here, a failure. + append_final(ws.clone(), &id, "run-6", &Err("boom".to_string())); + // A viewing client then persists what `chat_done` carried, same id. + append_final(ws.clone(), &id, "run-6", &Ok("All done.".to_string())); + + let agent_rows: Vec<_> = conversations::get_messages(ws, &id) + .expect("messages") + .into_iter() + .filter(|m| m.sender == "agent") + .collect(); + + assert_eq!(agent_rows.len(), 1, "still exactly one closing row"); + assert_eq!( + agent_rows[0].content, "Run failed: boom", + "the row that landed first must survive; a later same-id append must \ + not overwrite a recorded failure with a success" + ); + assert_eq!(agent_rows[0].extra_metadata["success"], false); +} diff --git a/src/openhuman/memory/conversations/mod.rs b/src/openhuman/memory/conversations/mod.rs index 550635119f..e6eb369aff 100644 --- a/src/openhuman/memory/conversations/mod.rs +++ b/src/openhuman/memory/conversations/mod.rs @@ -53,8 +53,8 @@ mod store; pub use bus::register_conversation_persistence_subscriber; pub use store::{ - append_message, delete_thread, ensure_thread, get_messages, list_threads, purge_threads, - update_message, update_thread_labels, update_thread_title, ConversationMessage, - ConversationMessagePatch, ConversationPurgeStats, ConversationStore, ConversationThread, - CreateConversationThread, CrossThreadHit, + append_message, delete_thread, ensure_thread, get_messages, is_deterministic_message_id, + list_threads, purge_threads, run_reply_message_id, update_message, update_thread_labels, + update_thread_title, ConversationMessage, ConversationMessagePatch, ConversationPurgeStats, + ConversationStore, ConversationThread, CreateConversationThread, CrossThreadHit, }; diff --git a/src/openhuman/memory/conversations/store/mod.rs b/src/openhuman/memory/conversations/store/mod.rs index 795989ebab..c775c9a8b6 100644 --- a/src/openhuman/memory/conversations/store/mod.rs +++ b/src/openhuman/memory/conversations/store/mod.rs @@ -86,6 +86,6 @@ pub use store::{ ConversationStore, }; pub use types::{ - ConversationMessage, ConversationMessagePatch, ConversationThread, CreateConversationThread, - CrossThreadHit, + is_deterministic_message_id, run_reply_message_id, ConversationMessage, + ConversationMessagePatch, ConversationThread, CreateConversationThread, CrossThreadHit, }; diff --git a/src/openhuman/memory/conversations/store/store.rs b/src/openhuman/memory/conversations/store/store.rs index 4dd82060eb..354a5a5bb8 100644 --- a/src/openhuman/memory/conversations/store/store.rs +++ b/src/openhuman/memory/conversations/store/store.rs @@ -275,6 +275,36 @@ where Ok(items) } +/// Find one message in a thread's JSONL log by id, without materializing the +/// whole transcript. +/// +/// Only the lines whose raw text carries the quoted id are deserialized, so a +/// lookup costs one parse rather than one per stored message; a line that +/// merely quotes the id inside its own content is rejected by the `id` check. +/// Mirrors [`read_jsonl`]'s tolerance of blank and corrupt lines. +pub(super) fn find_message_by_id( + path: &Path, + id: &str, +) -> Result, String> { + if !path.exists() { + return Ok(None); + } + let needle = serde_json::to_string(id).map_err(|e| format!("encode message id {id}: {e}"))?; + let file = File::open(path).map_err(|e| format!("open {}: {e}", path.display()))?; + for (line_no, line) in BufReader::new(file).lines().enumerate() { + let line = + line.map_err(|e| format!("read {} line {}: {e}", path.display(), line_no + 1))?; + if !line.contains(&needle) { + continue; + } + match serde_json::from_str::(&line) { + Ok(message) if message.id == id => return Ok(Some(message)), + _ => continue, + } + } + Ok(None) +} + /// Append one serialized value as a JSONL line, fsync'd before returning. pub(super) fn append_jsonl(path: &Path, value: &T) -> Result<(), String> where diff --git a/src/openhuman/memory/conversations/store/store_ops.rs b/src/openhuman/memory/conversations/store/store_ops.rs index 1189cb1d16..a96322dc58 100644 --- a/src/openhuman/memory/conversations/store/store_ops.rs +++ b/src/openhuman/memory/conversations/store/store_ops.rs @@ -6,13 +6,13 @@ use std::fs; use super::super::types::{ - ConversationMessage, ConversationMessagePatch, ConversationThread, CreateConversationThread, - CrossThreadHit, + is_deterministic_message_id, ConversationMessage, ConversationMessagePatch, ConversationThread, + CreateConversationThread, CrossThreadHit, }; use super::{ - append_jsonl, normalize_labels, read_jsonl, rewrite_jsonl, ConversationPurgeStats, - ConversationStore, ThreadLogEntry, CONVERSATION_INDEX_CACHE, CONVERSATION_STORE_LOCK, - THREADS_FILENAME, + append_jsonl, find_message_by_id, normalize_labels, read_jsonl, rewrite_jsonl, + ConversationPurgeStats, ConversationStore, ThreadLogEntry, CONVERSATION_INDEX_CACHE, + CONVERSATION_STORE_LOCK, THREADS_FILENAME, }; impl ConversationStore { @@ -115,6 +115,22 @@ impl ConversationStore { /// row, then a compact `MessageAppended` stat entry. Thread reads reconcile /// that stat trail against the message file, repairing a crash between the /// two appends. + /// + /// Idempotent for the ids the core mints deterministically + /// ([`is_deterministic_message_id`]): when the thread already holds a row + /// with that id, nothing is written (no message row, no stat bump, no index + /// insert) and the stored row is returned exactly as a fresh append would + /// return its input. Two writers can legitimately persist the same reply — + /// an autonomous run's `task_session::append_final` and the client that + /// also persists the `chat_done` it announced (#5933) — and a thread must + /// never carry two messages under one id (the frontend keys React and + /// assistant-ui resources by it). + /// + /// The lookup is deliberately narrow. Every other id in the store is + /// UUID-fresh by construction and cannot be re-presented, so it must not + /// pay to have that verified: a lookup on *every* append would put a scan + /// of the thread's transcript on the hot write path, under the + /// process-wide store lock, and make growing a thread quadratic. pub fn append_message( &self, thread_id: &str, @@ -125,6 +141,11 @@ impl ConversationStore { return Err(format!("thread {} not found", thread_id)); } let path = self.thread_messages_path(thread_id); + if is_deterministic_message_id(&message.id) { + if let Some(existing) = find_message_by_id(&path, &message.id)? { + return Ok(existing); + } + } if let Some(parent) = path.parent() { fs::create_dir_all(parent) .map_err(|e| format!("create conversation dir {}: {e}", parent.display()))?; diff --git a/src/openhuman/memory/conversations/store/store_tests.rs b/src/openhuman/memory/conversations/store/store_tests.rs index a47d879ffa..a9a982c07d 100644 --- a/src/openhuman/memory/conversations/store/store_tests.rs +++ b/src/openhuman/memory/conversations/store/store_tests.rs @@ -52,6 +52,135 @@ fn store_roundtrips_threads_and_messages() { assert_eq!(messages[0].content, "hello"); } +#[test] +fn append_message_is_idempotent_by_message_id() { + let (_temp, store) = make_store(); + store + .ensure_thread(CreateConversationThread { + parent_thread_id: None, + id: "t".to_string(), + title: "Conversation".to_string(), + created_at: "2026-04-10T12:00:00Z".to_string(), + labels: None, + personality_id: None, + }) + .expect("ensure thread"); + let first = ConversationMessage { + id: "agent:run-1".to_string(), + content: "first".to_string(), + message_type: "text".to_string(), + extra_metadata: json!({}), + sender: "agent".to_string(), + created_at: "2026-04-10T12:01:00Z".to_string(), + }; + store.append_message("t", first.clone()).expect("append"); + + // A second writer racing for the same id (the client persisting the + // `chat_done` an autonomous run already persisted itself — #5933). + let returned = store + .append_message( + "t", + ConversationMessage { + content: "second".to_string(), + created_at: "2026-04-10T12:02:00Z".to_string(), + ..first + }, + ) + .expect("append again"); + + // The stored row wins, and is what the second writer gets back. + assert_eq!(returned.content, "first"); + assert_eq!(returned.created_at, "2026-04-10T12:01:00Z"); + let messages = store.get_messages("t").expect("get messages"); + assert_eq!(messages.len(), 1, "one id, one row"); + assert_eq!(messages[0].content, "first"); + // The no-op append did not bump the stat trail either. + let threads = store.list_threads().expect("list threads"); + assert_eq!(threads[0].message_count, 1); + assert_eq!(threads[0].last_message_at, "2026-04-10T12:01:00Z"); +} + +#[test] +fn append_message_does_not_dedupe_client_generated_ids() { + // The idempotency lookup is scoped to the ids the core mints + // deterministically. Client-generated ids are UUID-fresh per message, so + // paying a transcript scan to verify that on every append would put a + // quadratic write path under the process-wide store lock — the store takes + // them at face value instead. + let (_temp, store) = make_store(); + store + .ensure_thread(CreateConversationThread { + parent_thread_id: None, + id: "t".to_string(), + title: "Conversation".to_string(), + created_at: "2026-04-10T12:00:00Z".to_string(), + labels: None, + personality_id: None, + }) + .expect("ensure thread"); + let message = ConversationMessage { + id: "user:5f1d0c3e-1f8b-4c1a-9c2e-2a7b6d4e8f90".to_string(), + content: "hello".to_string(), + message_type: "text".to_string(), + extra_metadata: json!({}), + sender: "user".to_string(), + created_at: "2026-04-10T12:01:00Z".to_string(), + }; + store.append_message("t", message.clone()).expect("append"); + store.append_message("t", message).expect("append again"); + + assert_eq!(store.get_messages("t").expect("get messages").len(), 2); +} + +#[test] +fn append_message_idempotency_ignores_an_id_quoted_inside_content() { + // The lookup narrows candidate lines by raw text before parsing them; a + // message that merely *quotes* another message's id must not be mistaken + // for that message and swallow the real append. + let (_temp, store) = make_store(); + store + .ensure_thread(CreateConversationThread { + parent_thread_id: None, + id: "t".to_string(), + title: "Conversation".to_string(), + created_at: "2026-04-10T12:00:00Z".to_string(), + labels: None, + personality_id: None, + }) + .expect("ensure thread"); + store + .append_message( + "t", + ConversationMessage { + id: "user:1".to_string(), + content: "agent:run-9".to_string(), + message_type: "text".to_string(), + extra_metadata: json!({}), + sender: "user".to_string(), + created_at: "2026-04-10T12:01:00Z".to_string(), + }, + ) + .expect("append quoting message"); + let stored = store + .append_message( + "t", + ConversationMessage { + id: "agent:run-9".to_string(), + content: "the real reply".to_string(), + message_type: "text".to_string(), + extra_metadata: json!({}), + sender: "agent".to_string(), + created_at: "2026-04-10T12:02:00Z".to_string(), + }, + ) + .expect("append reply"); + + assert_eq!(stored.content, "the real reply"); + let messages = store.get_messages("t").expect("get messages"); + assert_eq!(messages.len(), 2); + assert_eq!(messages[1].id, "agent:run-9"); +} + #[test] fn get_messages_for_new_empty_thread_returns_empty_list() { let (_temp, store) = make_store(); diff --git a/src/openhuman/memory/conversations/store/types.rs b/src/openhuman/memory/conversations/store/types.rs index e81ec16e68..665938f86d 100644 --- a/src/openhuman/memory/conversations/store/types.rs +++ b/src/openhuman/memory/conversations/store/types.rs @@ -115,6 +115,38 @@ pub struct CrossThreadHit { pub score: f64, } +/// Prefix of the message ids the core mints **deterministically** rather than +/// from a fresh UUID. +/// +/// Only such an id can be presented to the store twice by two different +/// writers, so it is also the marker +/// [`is_deterministic_message_id`] keys the store's idempotency lookup on. +pub const DETERMINISTIC_MESSAGE_ID_PREFIX: &str = "agent:"; + +/// The id an autonomous run's closing reply is stored under. +/// +/// Two writers legitimately persist that one reply — the core's +/// `task_session::append_final` and the client that also persists the +/// `chat_done` the run announces (`ChatRuntimeProvider` mirrors this shape for +/// `client_id: "system"` turns) — so both must derive the same id and the store +/// must collapse the second write onto the first (#5933). +pub fn run_reply_message_id(run_id: &str) -> String { + format!("{DETERMINISTIC_MESSAGE_ID_PREFIX}{run_id}") +} + +/// Whether `id` is one the core mints deterministically, i.e. one a second +/// writer can legitimately present again. +/// +/// This is what buys back the constant-time append path: every other id in the +/// store is UUID-fresh by construction (`user:`, `:`), can +/// never be re-presented, and so must not pay for a duplicate lookup. The +/// `agent:` ids the subagent/worker-thread writers mint do match — they +/// pay for a lookup they can never hit, which is one cheap scan of a +/// two-message worker transcript. +pub fn is_deterministic_message_id(id: &str) -> bool { + id.starts_with(DETERMINISTIC_MESSAGE_ID_PREFIX) +} + #[cfg(test)] #[path = "types_tests.rs"] mod tests; diff --git a/src/openhuman/memory/conversations/store/types_tests.rs b/src/openhuman/memory/conversations/store/types_tests.rs index 50fee0305a..eb14f876fe 100644 --- a/src/openhuman/memory/conversations/store/types_tests.rs +++ b/src/openhuman/memory/conversations/store/types_tests.rs @@ -63,3 +63,24 @@ fn create_thread_optional_fields_roundtrip() { ); assert!(decoded.parent_thread_id.is_none()); } + +#[test] +fn run_reply_id_is_deterministic_and_recognised_as_such() { + // The producer (`task_session::append_final`) and the predicate the store + // gates its idempotency lookup on must agree, or the two writers of an + // autonomous reply stop collapsing onto one row (#5933). + assert_eq!(run_reply_message_id("run-7"), "agent:run-7"); + assert!(is_deterministic_message_id(&run_reply_message_id("run-7"))); +} + +#[test] +fn client_generated_ids_are_not_deterministic() { + // These are UUID-fresh per message, so they can never be re-presented and + // must keep the constant-time append path. + assert!(!is_deterministic_message_id( + "user:5f1d0c3e-1f8b-4c1a-9c2e-2a7b6d4e8f90" + )); + assert!(!is_deterministic_message_id( + "assistant:5f1d0c3e-1f8b-4c1a-9c2e-2a7b6d4e8f90" + )); +} diff --git a/src/openhuman/web_chat/presentation.rs b/src/openhuman/web_chat/presentation.rs index 44e14076cb..b0921856eb 100644 --- a/src/openhuman/web_chat/presentation.rs +++ b/src/openhuman/web_chat/presentation.rs @@ -76,46 +76,15 @@ pub(crate) async fn deliver_response( if segments.len() <= 1 { // Single bubble — emit chat_done directly. - publish_web_channel_event(WebChannelEvent { - event: "chat_done".to_string(), - client_id: client_id.to_string(), - thread_id: thread_id.to_string(), - request_id: request_id.to_string(), - full_response: Some(full_response.to_string()), - message: None, - error_type: None, - error_source: None, - error_retryable: None, - error_retry_after_ms: None, - error_provider: None, - error_fallback_available: None, - tool_name: None, - skill_id: None, - args: None, - output: None, - success: None, - round: None, + publish_chat_done( + client_id, + thread_id, + request_id, + full_response, reaction_emoji, - segment_index: None, - segment_total: None, - delta: None, - delta_kind: None, - tool_call_id: None, - failure: None, - subagent: None, - task_board: None, - tool_display_label: None, - tool_display_detail: None, - citations: if citations.is_empty() { - None - } else { - Some(serde_json::json!(citations)) - }, - usage: usage_payload, - // Terminal delivery events are emitted outside the seq-stamping - // progress bridge; leave `seq` unset (older clients ignore it). - seq: None, - }); + citations, + usage_payload, + ); return; } @@ -213,6 +182,85 @@ pub(crate) async fn deliver_response( }); } +/// Deliver an agent response as exactly one `chat_done` bubble — no +/// segmentation, no reaction — for turns the core runs on its own behalf +/// (autonomous task sessions, background sub-agent result delivery). +/// +/// Those turns persist their closing message themselves, as a single row, +/// before announcing it (`task_session::append_final`). Splitting the reply +/// into `chat_segment` bubbles would have a viewing client persist one row per +/// segment beside that single row (#5933), so the conversational segmentation +/// of [`deliver_response`] is deliberately not offered here. +pub(crate) fn deliver_response_single_bubble( + client_id: &str, + thread_id: &str, + request_id: &str, + full_response: &str, + usage: Option<&LastTurnUsage>, +) { + publish_chat_done( + client_id, + thread_id, + request_id, + full_response, + None, + &[], + usage_payload(usage), + ); +} + +/// Emit the terminal `chat_done` for an unsegmented reply. +fn publish_chat_done( + client_id: &str, + thread_id: &str, + request_id: &str, + full_response: &str, + reaction_emoji: Option, + citations: &[crate::openhuman::memory::agent::memory_loader::MemoryCitation], + usage_payload: Option, +) { + publish_web_channel_event(WebChannelEvent { + event: "chat_done".to_string(), + client_id: client_id.to_string(), + thread_id: thread_id.to_string(), + request_id: request_id.to_string(), + full_response: Some(full_response.to_string()), + message: None, + error_type: None, + error_source: None, + error_retryable: None, + error_retry_after_ms: None, + error_provider: None, + error_fallback_available: None, + tool_name: None, + skill_id: None, + args: None, + output: None, + success: None, + round: None, + reaction_emoji, + segment_index: None, + segment_total: None, + delta: None, + delta_kind: None, + tool_call_id: None, + failure: None, + subagent: None, + task_board: None, + tool_display_label: None, + tool_display_detail: None, + citations: if citations.is_empty() { + None + } else { + Some(serde_json::json!(citations)) + }, + usage: usage_payload, + // Terminal delivery events are emitted outside the seq-stamping + // progress bridge; leave `seq` unset (older clients ignore it). + seq: None, + }); +} + // ── Segmentation ───────────────────────────────────────────────────────────── /// Decide whether and how to split a response into multiple chat bubbles. diff --git a/src/openhuman/web_chat/presentation_tests.rs b/src/openhuman/web_chat/presentation_tests.rs index cb04ff0ac7..270f6bd42f 100644 --- a/src/openhuman/web_chat/presentation_tests.rs +++ b/src/openhuman/web_chat/presentation_tests.rs @@ -270,3 +270,42 @@ fn segment_for_delivery_single_short_returns_one() { let r = segment_for_delivery("Quick."); assert_eq!(r.len(), 1); } + +#[test] +fn single_bubble_delivery_emits_one_unsegmented_chat_done_without_reaction() { + let mut rx = crate::openhuman::web_chat::subscribe_web_channel_events(); + // Prose `deliver_response` WOULD split into several `chat_segment` bubbles + // (long, multi-paragraph, no fences) — the shape a background delivery turn + // produces. A core-persisted single row must be announced as one bubble. + let text = "Same three meetings as before, and nothing on the calendar moved since the last check.\n\n\ + The product standup is still at noon and the design review still follows it at two.\n\n\ + Nothing needs input from you right now, so I have not rescheduled anything on your behalf."; + assert!( + segment_for_delivery(text).len() > 1, + "fixture must be one the conversational path would segment" + ); + let request_id = format!("single-bubble-{}", uuid::Uuid::new_v4()); + + deliver_response_single_bubble("system", "thread-1", &request_id, text, None); + + // Other tests publish on the same process-global bus; keep only ours. + let mut mine = Vec::new(); + loop { + match rx.try_recv() { + Ok(event) if event.request_id == request_id => mine.push(event), + Ok(_) => continue, + Err(tokio::sync::broadcast::error::TryRecvError::Lagged(_)) => continue, + Err(_) => break, + } + } + assert_eq!(mine.len(), 1, "exactly one terminal event, no chat_segment"); + let done = &mine[0]; + assert_eq!(done.event, "chat_done"); + assert_eq!(done.client_id, "system"); + assert_eq!(done.thread_id, "thread-1"); + assert_eq!(done.full_response.as_deref(), Some(text)); + assert_eq!(done.segment_total, None); + assert_eq!(done.segment_index, None); + assert_eq!(done.reaction_emoji, None); + assert!(done.usage.is_none()); +}