From 8fe70d63a0652531e630bb9e681110040ffc65d8 Mon Sep 17 00:00:00 2001 From: "vellum-apollo-bot[bot]" <242025090+vellum-apollo-bot[bot]@users.noreply.github.com> Date: Thu, 7 May 2026 16:30:03 +0000 Subject: [PATCH] fix(daemon): rehydrate conversation when surface-action arrives after eviction MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Surfaces persist as ui_surface content blocks in the messages table, but surfaceState/pendingSurfaceActions live only in memory. After a daemon restart or LRU eviction, a stale tab can still render a surface whose owning conversation is no longer loaded — clicking an action on it gets a 404 from /v1/surface-actions because findConversation* miss. Add resolveSurfaceConversation: on miss, scan the messages table for the ui_surface block, then call getOrCreateConversation to rehydrate. Apply to both the action and undo handlers. Includes regression tests covering live hits, DB-fallback rehydrate (with and without caller-supplied conversationId), and the no-surface-found 404 path. --- .../__tests__/surface-action-routes.test.ts | 384 ++++++++++++++++++ .../runtime/routes/surface-action-routes.ts | 50 ++- 2 files changed, 427 insertions(+), 7 deletions(-) create mode 100644 assistant/src/runtime/routes/__tests__/surface-action-routes.test.ts diff --git a/assistant/src/runtime/routes/__tests__/surface-action-routes.test.ts b/assistant/src/runtime/routes/__tests__/surface-action-routes.test.ts new file mode 100644 index 00000000000..16aa3b584d4 --- /dev/null +++ b/assistant/src/runtime/routes/__tests__/surface-action-routes.test.ts @@ -0,0 +1,384 @@ +/** + * Tests for surface-action route handlers. + * + * Focus is the rehydrate-on-miss path: after daemon restart or LRU eviction, + * the in-memory conversation map is empty even though the surface still + * persists as a `ui_surface` content block in the messages table. The handler + * must fall back to a DB scan, then call `getOrCreateConversation` so the + * action dispatches against a live conversation with restored `surfaceState`. + * + * Strategy: mock `conversation-store` and `raw-query` at module boundaries, + * import ROUTES afterwards, find each handler by operationId, and exercise. + */ + +import { beforeEach, describe, expect, mock, test } from "bun:test"; + +// --------------------------------------------------------------------------- +// Mock state +// --------------------------------------------------------------------------- + +interface StubConversation { + id: string; + handleSurfaceActionResult?: + | { accepted: true; conversationId?: string } + | { accepted: false; error: string }; + handleSurfaceActionThrows?: Error; + handleSurfaceUndoCalled?: boolean; + handleSurfaceUndoThrows?: Error; + surfaceActionCalls: Array<{ + surfaceId: string; + actionId: string; + data: unknown; + }>; +} + +let memoryById: StubConversation | null = null; +let memoryBySurface: StubConversation | null = null; +let rehydrated: StubConversation | null = null; +let rawGetReturn: { conversation_id: string } | null = null; + +const findConvCalls: string[] = []; +const findBySurfaceCalls: string[] = []; +const getOrCreateCalls: string[] = []; +const rawGetCalls: Array<{ sql: string; params: unknown[] }> = []; + +mock.module("../../../daemon/conversation-store.js", () => ({ + findConversation: (id: string) => { + findConvCalls.push(id); + return memoryById ?? undefined; + }, + findConversationBySurfaceId: (surfaceId: string) => { + findBySurfaceCalls.push(surfaceId); + return memoryBySurface ?? undefined; + }, + getOrCreateConversation: async (id: string) => { + getOrCreateCalls.push(id); + if (!rehydrated) { + throw new Error( + `getOrCreateConversation(${id}) called but no rehydrated stub configured`, + ); + } + return rehydrated; + }, +})); + +mock.module("../../../memory/raw-query.js", () => ({ + rawGet: (sql: string, ...params: unknown[]) => { + rawGetCalls.push({ sql, params }); + return rawGetReturn; + }, +})); + +// Defer route import until after mocks are installed. +const { ROUTES } = await import("../surface-action-routes.js"); +const { NotFoundError } = await import("../errors.js"); +import type { RouteDefinition } from "../types.js"; + +function findHandler(operationId: string): RouteDefinition["handler"] { + const route = ROUTES.find((r) => r.operationId === operationId); + if (!route) throw new Error(`Route ${operationId} not found`); + return route.handler; +} + +function makeStub(id: string): StubConversation { + const stub: StubConversation = { + id, + handleSurfaceActionResult: { accepted: true }, + surfaceActionCalls: [], + }; + // Methods reference `stub` so handler invocations land on the same object. + Object.assign(stub, { + handleSurfaceAction: async ( + surfaceId: string, + actionId: string, + data: unknown, + ) => { + stub.surfaceActionCalls.push({ surfaceId, actionId, data }); + if (stub.handleSurfaceActionThrows) throw stub.handleSurfaceActionThrows; + return stub.handleSurfaceActionResult; + }, + handleSurfaceUndo: (_surfaceId: string) => { + stub.handleSurfaceUndoCalled = true; + if (stub.handleSurfaceUndoThrows) throw stub.handleSurfaceUndoThrows; + }, + }); + return stub; +} + +// --------------------------------------------------------------------------- +// Setup +// --------------------------------------------------------------------------- + +beforeEach(() => { + memoryById = null; + memoryBySurface = null; + rehydrated = null; + rawGetReturn = null; + findConvCalls.length = 0; + findBySurfaceCalls.length = 0; + getOrCreateCalls.length = 0; + rawGetCalls.length = 0; +}); + +// --------------------------------------------------------------------------- +// triggerSurfaceAction +// --------------------------------------------------------------------------- + +describe("triggerSurfaceAction handler", () => { + test("dispatches against live in-memory conversation when found by surfaceId", async () => { + const live = makeStub("conv-live"); + memoryBySurface = live; + + const handler = findHandler("triggerSurfaceAction"); + const result = await handler({ + body: { surfaceId: "surf-1", actionId: "act-1" }, + }); + + expect(result).toEqual({ ok: true }); + expect(findBySurfaceCalls).toEqual(["surf-1"]); + expect(findConvCalls).toEqual([]); + expect(rawGetCalls).toEqual([]); + expect(getOrCreateCalls).toEqual([]); + expect(live.surfaceActionCalls).toEqual([ + { surfaceId: "surf-1", actionId: "act-1", data: undefined }, + ]); + }); + + test("uses caller-supplied conversationId for in-memory hit", async () => { + const live = makeStub("conv-explicit"); + memoryById = live; + + const handler = findHandler("triggerSurfaceAction"); + await handler({ + body: { + conversationId: "conv-explicit", + surfaceId: "surf-2", + actionId: "act-2", + data: { foo: "bar" }, + }, + }); + + expect(findConvCalls).toEqual(["conv-explicit"]); + expect(findBySurfaceCalls).toEqual([]); + expect(rawGetCalls).toEqual([]); + expect(getOrCreateCalls).toEqual([]); + expect(live.surfaceActionCalls).toEqual([ + { surfaceId: "surf-2", actionId: "act-2", data: { foo: "bar" } }, + ]); + }); + + test("rehydrates via DB fallback when in-memory lookup misses", async () => { + rawGetReturn = { conversation_id: "conv-from-db" }; + rehydrated = makeStub("conv-from-db"); + + const handler = findHandler("triggerSurfaceAction"); + const result = await handler({ + body: { surfaceId: "surf-evicted", actionId: "act-3" }, + }); + + expect(result).toEqual({ ok: true }); + expect(findBySurfaceCalls).toEqual(["surf-evicted"]); + expect(findConvCalls).toEqual([]); + expect(rawGetCalls).toHaveLength(1); + // SQL must filter the messages table by ui_surface payload pattern. + expect(rawGetCalls[0]!.sql).toContain("FROM messages"); + expect(rawGetCalls[0]!.sql).toContain("LIKE"); + expect(rawGetCalls[0]!.params).toEqual([ + `%"surfaceId":"surf-evicted"%`, + ]); + expect(getOrCreateCalls).toEqual(["conv-from-db"]); + expect(rehydrated.surfaceActionCalls).toEqual([ + { surfaceId: "surf-evicted", actionId: "act-3", data: undefined }, + ]); + }); + + test("rehydrates with caller-supplied conversationId after validating DB row", async () => { + rawGetReturn = { conversation_id: "conv-caller" }; + rehydrated = makeStub("conv-caller"); + + const handler = findHandler("triggerSurfaceAction"); + await handler({ + body: { + conversationId: "conv-caller", + surfaceId: "surf-caller", + actionId: "act-4", + }, + }); + + expect(findConvCalls).toEqual(["conv-caller"]); + expect(findBySurfaceCalls).toEqual([]); + // DB scan validates the surface actually exists before rehydrating. + expect(rawGetCalls).toHaveLength(1); + expect(getOrCreateCalls).toEqual(["conv-caller"]); + expect(rehydrated.surfaceActionCalls).toEqual([ + { surfaceId: "surf-caller", actionId: "act-4", data: undefined }, + ]); + }); + + test("returns 404 when surface is not in memory and not in DB", async () => { + const handler = findHandler("triggerSurfaceAction"); + + let caught: unknown; + try { + await handler({ + body: { surfaceId: "surf-ghost", actionId: "act-5" }, + }); + } catch (err) { + caught = err; + } + + expect(caught).toBeInstanceOf(NotFoundError); + expect(rawGetCalls).toHaveLength(1); + // No phantom conversation created. + expect(getOrCreateCalls).toEqual([]); + }); + + test("returns 404 when caller-supplied conversationId has no matching surface in DB", async () => { + // rawGetReturn stays null → DB has no row for this surface, so we + // refuse to rehydrate even though the caller named a conversation. + const handler = findHandler("triggerSurfaceAction"); + + let caught: unknown; + try { + await handler({ + body: { + conversationId: "conv-deleted", + surfaceId: "surf-x", + actionId: "act-x", + }, + }); + } catch (err) { + caught = err; + } + + expect(caught).toBeInstanceOf(NotFoundError); + // Crucially, we did NOT call getOrCreateConversation — the previous + // version would have created a phantom empty conversation here. + expect(getOrCreateCalls).toEqual([]); + }); + + test("returns 404 when caller-supplied conversationId mismatches the DB row", async () => { + // Surface exists in conv-real, but caller asserts conv-other. + rawGetReturn = { conversation_id: "conv-real" }; + const handler = findHandler("triggerSurfaceAction"); + + let caught: unknown; + try { + await handler({ + body: { + conversationId: "conv-other", + surfaceId: "surf-shared", + actionId: "act-x", + }, + }); + } catch (err) { + caught = err; + } + + expect(caught).toBeInstanceOf(NotFoundError); + expect(getOrCreateCalls).toEqual([]); + }); + + test("escapes LIKE wildcards in surfaceId", async () => { + // A request with `surfaceId: "%"` must not match unrelated rows. We + // assert the SQL uses ESCAPE and the bound parameter has the wildcard + // backslash-escaped. + const handler = findHandler("triggerSurfaceAction"); + + try { + await handler({ + body: { surfaceId: "%", actionId: "act-evil" }, + }); + } catch { + /* expected NotFoundError, we only care about the SQL */ + } + + expect(rawGetCalls).toHaveLength(1); + expect(rawGetCalls[0]!.sql).toContain("ESCAPE"); + expect(rawGetCalls[0]!.params).toEqual([`%"surfaceId":"\\%"%`]); + }); + + test("propagates accepted=false rejection as BadRequestError", async () => { + const live = makeStub("conv-reject"); + live.handleSurfaceActionResult = { + accepted: false, + error: "surface already completed", + }; + memoryBySurface = live; + + const handler = findHandler("triggerSurfaceAction"); + + let caught: unknown; + try { + await handler({ + body: { surfaceId: "surf-done", actionId: "act-y" }, + }); + } catch (err) { + caught = err; + } + + // BadRequestError is a RouteError — verify the error message bubbled. + expect(caught).toBeDefined(); + expect((caught as Error).message).toContain("surface already completed"); + }); +}); + +// --------------------------------------------------------------------------- +// undoSurfaceAction +// --------------------------------------------------------------------------- + +describe("undoSurfaceAction handler", () => { + test("dispatches against live in-memory conversation", async () => { + const live = makeStub("conv-undo-live"); + memoryById = live; + + const handler = findHandler("undoSurfaceAction"); + const result = await handler({ + body: { conversationId: "conv-undo-live" }, + pathParams: { id: "surf-undo-1" }, + }); + + expect(result).toEqual({ ok: true }); + expect(findConvCalls).toEqual(["conv-undo-live"]); + expect(rawGetCalls).toEqual([]); + expect(getOrCreateCalls).toEqual([]); + expect(live.handleSurfaceUndoCalled).toBe(true); + }); + + test("rehydrates via DB fallback when conversation evicted", async () => { + rawGetReturn = { conversation_id: "conv-undo-from-db" }; + rehydrated = makeStub("conv-undo-from-db"); + + const handler = findHandler("undoSurfaceAction"); + const result = await handler({ + body: {}, + pathParams: { id: "surf-undo-evicted" }, + }); + + expect(result).toEqual({ ok: true }); + expect(findBySurfaceCalls).toEqual(["surf-undo-evicted"]); + expect(rawGetCalls).toHaveLength(1); + expect(rawGetCalls[0]!.params).toEqual([ + `%"surfaceId":"surf-undo-evicted"%`, + ]); + expect(getOrCreateCalls).toEqual(["conv-undo-from-db"]); + expect(rehydrated.handleSurfaceUndoCalled).toBe(true); + }); + + test("returns 404 when surface cannot be located", async () => { + const handler = findHandler("undoSurfaceAction"); + + let caught: unknown; + try { + await handler({ + body: {}, + pathParams: { id: "surf-undo-ghost" }, + }); + } catch (err) { + caught = err; + } + + expect(caught).toBeInstanceOf(NotFoundError); + expect(getOrCreateCalls).toEqual([]); + }); +}); diff --git a/assistant/src/runtime/routes/surface-action-routes.ts b/assistant/src/runtime/routes/surface-action-routes.ts index 95a0e84fcea..158358b585b 100644 --- a/assistant/src/runtime/routes/surface-action-routes.ts +++ b/assistant/src/runtime/routes/surface-action-routes.ts @@ -11,10 +11,13 @@ import { z } from "zod"; import type { ChannelId } from "../../channels/types.js"; import { isHttpAuthDisabled } from "../../config/env.js"; +import type { Conversation } from "../../daemon/conversation.js"; import { findConversation, findConversationBySurfaceId, + getOrCreateConversation, } from "../../daemon/conversation-store.js"; +import { rawGet } from "../../memory/raw-query.js"; import { getLogger } from "../../util/logger.js"; import { DAEMON_INTERNAL_ASSISTANT_ID } from "../assistant-scope.js"; import { healGuardianBindingDrift } from "../guardian-vellum-migration.js"; @@ -87,6 +90,37 @@ function applyTrustContext( } } +/** + * Resolve the conversation owning a surface, rehydrating from the DB when + * the in-memory lookup misses (daemon restart or LRU eviction). The DB scan + * is also the source of truth that the surface exists — without it, + * `getOrCreateConversation` would silently create a phantom conversation + * for any caller-supplied id. + */ +async function resolveSurfaceConversation( + conversationId: string | null | undefined, + surfaceId: string, +): Promise { + const found = conversationId + ? findConversation(conversationId) + : findConversationBySurfaceId(surfaceId); + if (found) return found; + + // Escape LIKE wildcards so a `surfaceId` like "%" or "_" can't match + // unrelated rows. + const escaped = surfaceId.replace(/[\\%_]/g, "\\$&"); + const row = rawGet<{ conversation_id: string }>( + `SELECT conversation_id FROM messages + WHERE content LIKE ? ESCAPE '\\' + ORDER BY created_at DESC + LIMIT 1`, + `%"surfaceId":"${escaped}"%`, + ); + if (!row) return undefined; + if (conversationId && conversationId !== row.conversation_id) return undefined; + return await getOrCreateConversation(row.conversation_id); +} + // --------------------------------------------------------------------------- // Handlers // --------------------------------------------------------------------------- @@ -113,9 +147,10 @@ async function handleSurfaceAction({ throw new BadRequestError("conversationId must be a string"); } - const conversation = conversationId - ? findConversation(conversationId) - : findConversationBySurfaceId(surfaceId); + const conversation = await resolveSurfaceConversation( + conversationId, + surfaceId, + ); if (!conversation) { throw new NotFoundError("No active conversation found"); @@ -175,7 +210,7 @@ async function handleSurfaceAction({ } } -function handleSurfaceUndo({ body, pathParams }: RouteHandlerArgs) { +async function handleSurfaceUndo({ body, pathParams }: RouteHandlerArgs) { const surfaceId = pathParams?.id; if (!surfaceId) { throw new BadRequestError("surfaceId path parameter is required"); @@ -186,9 +221,10 @@ function handleSurfaceUndo({ body, pathParams }: RouteHandlerArgs) { throw new BadRequestError("conversationId must be a string"); } - const conversation = conversationId - ? findConversation(conversationId) - : findConversationBySurfaceId(surfaceId); + const conversation = await resolveSurfaceConversation( + conversationId, + surfaceId, + ); if (!conversation) { throw new NotFoundError("No active conversation found");