diff --git a/CHANGELOG.md b/CHANGELOG.md index fb11af23..ecd611c5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -235,6 +235,29 @@ one they are, so those match too. No configuration changes and nothing is stored differently; a deployment that was already on the light theme sees no difference at all. +### A conversation can be deleted + +Nothing removed a channel. Starting one was the only lever the product gave a person, and every +conversation with every coworker sat in the roster forever, growing on every message the way +`DEFAULT_CHANNEL_PAGE`'s own note already described: a page that was instant in a demo returns +thousands of rows for anybody who has actually been using the product a while, one that never shrinks +again. + +Deleting a channel now removes it for good. The channel row goes, and its memberships, its linked +coworkers, and its Intelligence thread mapping go with it through the same foreign-key cascades that +already existed for them — no migration needed, only a query that finally uses them. The deployment +also asks Intelligence to permanently delete the thread itself, so the message history is not just +unlisted, it is gone from the platform too. + +A thread the platform refuses to delete does not hold the channel hostage. The local removal already +committed by the time that call runs, so a rejected or unreachable upstream delete leaves the channel +gone from the roster regardless, with an audit row (`channel.deleted`) naming the thread and whether +Intelligence actually forgot it. A channel that is gone locally with an orphaned thread still on the +platform is a smaller, more honest failure than a channel sitting in the roster with its history +silently wiped out from under it, and the audit trail is where an administrator finds the one that +did not clean up completely. `DELETE /api/channels/:channelId` answers with `historyLeftBehind`, so a +screen showing the outcome does not have to guess which of the two happened. + ## 0.0.4 ### A click citing a ref this deployment cannot resolve is refused diff --git a/app/src/components/app-sidebar/channel.tsx b/app/src/components/app-sidebar/channel.tsx index 74314781..9b15b799 100644 --- a/app/src/components/app-sidebar/channel.tsx +++ b/app/src/components/app-sidebar/channel.tsx @@ -1,10 +1,34 @@ -import { Link } from "@tanstack/react-router"; -import { memo } from "react"; +import { IconDots } from "@tabler/icons-react"; +import { useMutation, useQueryClient } from "@tanstack/react-query"; +import { Link, useNavigate, useParams } from "@tanstack/react-router"; +import { memo, useState } from "react"; +import { deleteChannelMutationOptions } from "@/lib/channels/mutations"; import { ChannelAvatar } from "../channels/avatar"; +import { + AlertDialog, + AlertDialogCancel, + AlertDialogContent, + AlertDialogDescription, + AlertDialogFooter, + AlertDialogHeader, + AlertDialogTitle, +} from "../ui/alert-dialog"; +import { Button } from "../ui/button"; +import { + DropdownMenu, + DropdownMenuContent, + DropdownMenuGroup, + DropdownMenuItem, + DropdownMenuTrigger, +} from "../ui/dropdown-menu"; /** * Memoized roster row. `use-channel-events` preserves unchanged row identity, and * `content-visibility` keeps off-screen rows cheap without virtualization. + * + * State inside a row is no reason to drop the memo: `memo` compares the props it is handed and has + * nothing to say about a hook. Dropping it re-renders every row in the roster whenever the sidebar + * renders, which is the cost the identity-preserving patch in `use-channel-events` exists to avoid. */ export const Channel = memo(function Channel({ channelId, @@ -19,32 +43,132 @@ export const Channel = memo(function Channel({ lastMessage?: string; lastMessageAt?: string; }) { + const queryClient = useQueryClient(); + const navigate = useNavigate(); + // `strict: false`: this row renders in the sidebar on every screen, not only while its own + // channel is open, so there may be no `channelId` route param to read at all. + const { channelId: openChannelId } = useParams({ strict: false }); + const [deleteDialogOpen, setDeleteDialogOpen] = useState(false); + const deleteChannel = useMutation(deleteChannelMutationOptions(queryClient)); + + const handleDelete = async () => { + // Navigate away first: the row this menu lives on unmounts the moment the list invalidates, + // and a screen still pointed at a channel id that no longer resolves is worse than a screen + // that moved on a beat early. + if (openChannelId === channelId) { + await navigate({ to: "/" }); + } + try { + await deleteChannel.mutateAsync(channelId); + /* + * Closed on success rather than left to the unmount. + * + * The row does go away when the roster invalidates, taking this dialog with it, but that is a + * side effect of a cache write and not something this component controls. + */ + setDeleteDialogOpen(false); + } catch { + /* + * Left open, deliberately. A delete that failed leaves the row exactly where it was, so + * closing would return the person to a roster that still lists the conversation they just + * asked to be rid of, with nothing anywhere saying why. The message is rendered below; + * `mutateAsync` rejects rather than swallowing, which is why this catch exists at all. + */ + } + }; + return ( - -
- -
-
-
- {name} -
- {lastMessageAt} -
+
+ +
+
-
- - {lastMessage} - +
+
+ {name} +
+ {lastMessageAt} +
+
+
+ + {lastMessage} + +
+ +
+ + + + + } + /> + + + {/* Only opens the dialog below; the menu closes on click, too early to confirm anything. */} + setDeleteDialogOpen(true)} + variant="destructive" + > + Delete + + + +
- + + + + Delete this conversation? + + This deletes your conversation with{" "} + {name}, + including its message history. This cannot be undone. + + + {deleteChannel.isError ? ( +

+ {deleteChannel.error.message} +

+ ) : null} + + + Cancel + + {/* + * A plain button, not `AlertDialogAction`. + * + * That one renders the primitive's `Close`, so it shuts the dialog the instant it is + * pressed, before the request it starts has been answered. Nothing then reports a + * delete that failed: the dialog is gone, the conversation is still in the roster, and + * the person is left to work out for themselves that the thing they asked for did not + * happen. It also means "Deleting…" below could never appear. + */} + + +
+
+
); }); diff --git a/app/src/components/ui/alert-dialog.tsx b/app/src/components/ui/alert-dialog.tsx new file mode 100644 index 00000000..b30e42a2 --- /dev/null +++ b/app/src/components/ui/alert-dialog.tsx @@ -0,0 +1,150 @@ +import { AlertDialog as AlertDialogPrimitive } from "@base-ui/react/alert-dialog"; +import type * as React from "react"; + +import { Button } from "@/components/ui/button"; +import { cn } from "@/lib/utils"; + +/** + * A modal for an action a click cannot undo. Built on `AlertDialogRoot` rather than `Dialog`'s + * `DialogRoot`: it carries `role="alertdialog"` and is announced immediately, and has no corner + * close button — the only way out is one of the footer's own buttons. + */ +function AlertDialog({ ...props }: AlertDialogPrimitive.Root.Props) { + return ; +} + +function AlertDialogTrigger({ ...props }: AlertDialogPrimitive.Trigger.Props) { + return ( + + ); +} + +function AlertDialogPortal({ ...props }: AlertDialogPrimitive.Portal.Props) { + return ( + + ); +} + +function AlertDialogOverlay({ + className, + ...props +}: AlertDialogPrimitive.Backdrop.Props) { + return ( + + ); +} + +function AlertDialogContent({ + className, + ...props +}: AlertDialogPrimitive.Popup.Props) { + return ( + + + + + ); +} + +function AlertDialogHeader({ className, ...props }: React.ComponentProps<"div">) { + return ( +
+ ); +} + +function AlertDialogFooter({ className, ...props }: React.ComponentProps<"div">) { + return ( +
+ ); +} + +function AlertDialogTitle({ + className, + ...props +}: AlertDialogPrimitive.Title.Props) { + return ( + + ); +} + +function AlertDialogDescription({ + className, + ...props +}: AlertDialogPrimitive.Description.Props) { + return ( + + ); +} + +/** The button that answers "no" or "not now." Closes without running anything else. */ +function AlertDialogCancel({ + className, + ...props +}: React.ComponentProps) { + return ( + } + /> + ); +} + +/** + * The button that carries out the action, styled destructive by default since that is the only + * reason this component exists rather than the ordinary `Dialog`. Pass `onClick` to run the action; + * closing is automatic, the same as `AlertDialogCancel`. + */ +function AlertDialogAction({ + className, + variant = "destructive", + ...props +}: React.ComponentProps) { + return ( + } + /> + ); +} + +export { + AlertDialog, + AlertDialogAction, + AlertDialogCancel, + AlertDialogContent, + AlertDialogDescription, + AlertDialogFooter, + AlertDialogHeader, + AlertDialogOverlay, + AlertDialogPortal, + AlertDialogTitle, + AlertDialogTrigger, +}; diff --git a/app/src/lib/channels/mutations.ts b/app/src/lib/channels/mutations.ts index 95f7185d..1fb3f76c 100644 --- a/app/src/lib/channels/mutations.ts +++ b/app/src/lib/channels/mutations.ts @@ -22,6 +22,35 @@ export function createChannelMutationOptions(queryClient: QueryClient) { }); } +/** + * Delete a channel, and ask the platform to forget the thread behind it. + * + * Other tabs learn a channel is gone from the socket event in `use-channel-events.ts`; this tab + * issued the delete itself and never receives its own event, so it clears the roster and detail + * cache directly on success. + * + * Resolves to whether the message history outlived the channel. The local delete commits first and + * the thread deletion can fail on its own, so this is not a failure to throw on: the conversation + * is gone either way, and the caller shows the residue rather than reporting an error that did not + * happen. + */ +export function deleteChannelMutationOptions(queryClient: QueryClient) { + return mutationOptions({ + mutationFn: async (channelId: string): Promise => { + const response = await client(`/api/channels/${channelId}`, { + method: "DELETE", + fallback: "Could not delete this conversation", + }); + const body = (await response.json()) as { historyLeftBehind?: boolean }; + return body.historyLeftBehind === true; + }, + onSuccess: (_data, channelId) => { + queryClient.invalidateQueries({ queryKey: channelKeys.all }); + queryClient.removeQueries({ queryKey: channelKeys.detail(channelId) }); + }, + }); +} + /** * Report the last thing said in a channel. * diff --git a/app/src/lib/channels/use-channel-events.ts b/app/src/lib/channels/use-channel-events.ts index d8737b86..63459dd4 100644 --- a/app/src/lib/channels/use-channel-events.ts +++ b/app/src/lib/channels/use-channel-events.ts @@ -1,4 +1,5 @@ import { useQueryClient } from "@tanstack/react-query"; +import { useRouter } from "@tanstack/react-router"; import { useEffect } from "react"; import { type ChannelPage, type ChannelSummary, channelKeys } from "./queries"; @@ -14,6 +15,8 @@ type ChannelActivityEvent = { lastMessage: string | null; lastMessageAt: string | null; lastMessageAgentId: string | null; + /** The channel is gone. Absent on an ordinary activity event. */ + deleted?: true; }; const FIRST_RETRY_MS = 500; @@ -27,6 +30,7 @@ function socketUrl() { export function useChannelEvents() { const queryClient = useQueryClient(); + const router = useRouter(); useEffect(() => { let socket: WebSocket | undefined; @@ -72,6 +76,24 @@ export function useChannelEvents() { (channel) => channel.id === activity.channelId, ), ); + + // Must run before the patch below, which spreads the event onto the existing row — + // reaching that first would stamp `deleted: true` on the row instead of removing it. + // An unknown channel here is already gone from this cache, so there is nothing to patch + // or invalidate for, unlike the "unknown channel" case below for an ordinary event. + if (activity.deleted) { + if (holdingPage === -1) return data; + const page = data.pages[holdingPage] as ChannelPage; + const pages = data.pages.slice(); + pages[holdingPage] = { + ...page, + channels: page.channels.filter( + (channel) => channel.id !== activity.channelId, + ), + }; + return { ...data, pages }; + } + // An unknown channel id means the roster is stale; refetch rather than patch. if (holdingPage === -1) { void queryClient.invalidateQueries({ @@ -103,6 +125,24 @@ export function useChannelEvents() { return { ...data, pages }; }, ); + + /* + * A tab looking at the channel somebody just deleted in another tab. + * + * The tab that issued the delete moves itself before it fires the request. Every other tab + * only ever hears about it here, and dropping the row without moving leaves that tab on a + * route whose channel no longer resolves: an error, or an empty conversation, depending on + * which query answers first. + * + * Read off the router at event time rather than through `useParams`, so the effect does not + * have to be torn down and reconnected on every navigation just to keep this value fresh. + */ + if (activity.deleted) { + const { pathname } = router.state.location; + if (pathname === `/channel/${activity.channelId}`) { + void router.navigate({ to: "/" }); + } + } }; // WebSocket needs explicit reconnect handling. @@ -122,7 +162,7 @@ export function useChannelEvents() { if (socket) socket.onclose = null; socket?.close(); }; - }, [queryClient]); + }, [queryClient, router]); } /** diff --git a/server/src/app.ts b/server/src/app.ts index fcf65c87..edd47dff 100644 --- a/server/src/app.ts +++ b/server/src/app.ts @@ -686,10 +686,19 @@ export function createApp( } } + // Shared by the thread-existence check below and a deleted channel's request to forget its thread. + const intelligence = createIntelligenceClient(config.runtime.intelligence); + if (channelStore) { app.route( "/api/channels", - createChannelRoutes(channelStore, requireUser, channelEvents), + createChannelRoutes( + channelStore, + requireUser, + channelEvents, + auditStore, + (params) => intelligence.deleteThread(params), + ), ); } @@ -828,13 +837,9 @@ export function createApp( threadIdentity, requireUser, // config.ts refuses to boot without the full Intelligence contract (see copilot.ts's - // header comment), so `config.runtime.intelligence` is never missing here. Built from it - // rather than assumed, though: this is the one place besides the runtime mount itself that - // needs to reach Intelligence, and it should keep working unmodified if that guarantee ever - // loosens and a deployment can legitimately have no reader to build. - createThreadReader( - createIntelligenceClient(config.runtime.intelligence), - ), + // header comment), so `config.runtime.intelligence` is never missing here, and the shared + // `intelligence` client built above is never missing either. + createThreadReader(intelligence), ), ); } diff --git a/server/src/audit.ts b/server/src/audit.ts index 24ff40f8..d504024e 100644 --- a/server/src/audit.ts +++ b/server/src/audit.ts @@ -50,6 +50,12 @@ export const auditEventTypes = [ * routing decision is a fact about where a conversation went, not a copy of what was said. */ "channel.routed", + /** + * A channel was deleted, taking its memberships, agent links, and thread mapping with it. The + * channel row is already gone by the time this is written, so this is the only place left that + * says it ever existed. `payload.threadForgotten: false` marks a thread that outlived it. + */ + "channel.deleted", "agent.invoked", /** * An address this deployment declined to dial for a Bot, and why. diff --git a/server/src/channels/events.ts b/server/src/channels/events.ts index 3bd6cd12..b0682a41 100644 --- a/server/src/channels/events.ts +++ b/server/src/channels/events.ts @@ -22,6 +22,8 @@ export type ChannelActivityEvent = { lastMessage: string | null; lastMessageAt: string | null; lastMessageAgentId: string | null; + /** The channel is gone. Absent on an ordinary activity event. */ + deleted?: true; }; type Send = (payload: string) => void; diff --git a/server/src/channels/routes.ts b/server/src/channels/routes.ts index 9dfa8065..a912aa68 100644 --- a/server/src/channels/routes.ts +++ b/server/src/channels/routes.ts @@ -6,6 +6,7 @@ import { type AgentProfileStore, } from "../agents/profile-store"; import type { AgentActor, AgentProfile } from "../agents/profile-types"; +import { type AuditStore, recordAuditEvent } from "../audit"; import type { AppVariables } from "../auth/guards"; import type { Database } from "../db/client"; import { @@ -102,6 +103,8 @@ export type ChannelStore = { channelId: string, activity: ChannelActivity, ): Promise; + /** Deletes the channel for everyone in it. Returns the thread it owned, so the caller can forget it upstream. */ + remove(actor: AgentActor, channelId: string): Promise; }; const PRIVATE_AGENT_CHANNEL_DESCRIPTION = "Private agent channel."; @@ -438,6 +441,64 @@ export function createChannelStore( { isolationLevel: "read committed" }, ); }, + + // `create` inserts exactly one membership row, so "delete" and "leave" are the same act while a + // channel has exactly one member. Named `remove` for the multi-member split this becomes later. + remove(actor, channelId) { + return database.transaction( + async (transaction) => { + const [membership] = await transaction + .select({ channelId: channelMemberships.channelId }) + .from(channelMemberships) + .where( + and( + eq(channelMemberships.channelId, channelId), + eq(channelMemberships.userId, actor.id), + ), + ); + // Not a member, or no such channel: the same answer either way, so belonging to a channel + // is not something an outsider can probe for. Same reasoning as `recordActivity` above. + if (!membership) throw new ChannelNotFoundError(channelId); + + // Read before the delete, not after: the cascade below wipes both of these tables, and the + // notify payload needs the members it is telling, while the caller needs the thread id to + // ask Intelligence to forget it. + const members = await transaction + .select({ userId: channelMemberships.userId }) + .from(channelMemberships) + .where(eq(channelMemberships.channelId, channelId)); + const [mapping] = await transaction + .select({ threadId: intelligenceChannelMappings.threadId }) + .from(intelligenceChannelMappings) + .where( + and( + eq(intelligenceChannelMappings.channelId, channelId), + eq(intelligenceChannelMappings.userId, actor.id), + ), + ); + + // Cascades `channel_memberships`, `channel_agents`, and `intelligence_channel_mappings`: + // see the `onDelete: "cascade"` on each in db/schema/core.ts. Nothing else references a + // channel, so this one delete is the whole local removal. + await transaction.delete(channels).where(eq(channels.id, channelId)); + + const event: ChannelActivityEvent = { + channelId, + memberIds: members.map((member) => member.userId), + lastMessage: null, + lastMessageAt: null, + lastMessageAgentId: null, + deleted: true, + }; + await transaction.execute( + sql`select pg_notify(${CHANNEL_ACTIVITY_TOPIC}, ${JSON.stringify(event)})`, + ); + + return mapping?.threadId ?? null; + }, + { isolationLevel: "read committed" }, + ); + }, }; } @@ -528,9 +589,64 @@ export function createChannelRoutes( requireUser: MiddlewareHandler<{ Variables: AppVariables }>, /** Absent in tests and wherever live updates are not wanted; the routes still work without it. */ events?: ChannelEventHub, + /** Where a channel's deletion is written. Absent in tests that do not care about the trail. */ + auditStore?: AuditStore, + /** + * Ask Intelligence to permanently delete a thread. Absent leaves a channel deletable and its + * thread left behind on the platform: the local removal below does not depend on this existing. + */ + forgetThread?: (params: { + threadId: string; + userId: string; + agentId: string; + }) => Promise, ) { const routes = new Hono<{ Variables: AppVariables }>(); + /** + * Write the one audit row this file ever writes, tolerantly. + * + * Mirrors `record` in agents/routes.ts: never fatal, because the channel is already gone and the + * caller has already been told so by the time this runs. A trail that is briefly unavailable is + * not a reason to report a failure that did not happen. + */ + const recordDeleted = async ( + context: Context<{ Variables: AppVariables }>, + channelId: string, + payload: { threadId: string | null; threadForgotten: boolean }, + ): Promise => { + if (!auditStore) return; + const actor = context.var.actor; + try { + await recordAuditEvent(auditStore, { + eventType: "channel.deleted", + targetType: "channel", + targetId: channelId, + /* + * Attributed, including in single-user mode. + * + * The other audited surfaces drop this id when the actor is the local development one, on + * the grounds that `audit_events.actor_user_id` has a foreign key into `users` that it would + * violate. It has no foreign key, and `initializeDevActorUser` writes that row at start-up + * anyway, so neither half of the reason holds. It matters here more than most: single-user + * is the mode `.env.example` ships switched on, so an unattributed row is what a fork sees + * by default, and "somebody deleted this conversation" is the whole point of the row. + */ + actorUserId: actor.id, + payload, + }); + } catch (error) { + console.error( + JSON.stringify({ + type: "channel-audit-write-failed", + eventType: "channel.deleted", + channelId, + error: String(error), + }), + ); + } + }; + // Before `/:channelId`, or "events" is read as a channel id. if (events) { routes.get( @@ -622,6 +738,62 @@ export function createChannelRoutes( } }); + // Registered unconditionally, unlike `GET /:threadId` in thread-routes.ts: removing your own + // channel from your own roster can always succeed locally, whether or not Intelligence is reachable. + routes.delete("/:channelId", requireUser, async (context) => { + const channelId = context.req.param("channelId"); + try { + const threadId = await store.remove(context.var.actor, channelId); + + // The local delete already committed above, so a failed thread deletion is non-fatal: a + // channel gone locally with an orphaned thread beats one still on screen with its history wiped. + let threadForgotten = false; + if (threadId && forgetThread) { + try { + await forgetThread({ + threadId, + userId: context.var.actor.id, + // Derived here, never accepted from the caller: this is the same string + // channel-chat.tsx builds for the same channel, and trusting a client-supplied agentId + // would let a request name any thread it likes and ask the platform to delete it. + agentId: `channel:${channelId}`, + }); + threadForgotten = true; + } catch { + console.error( + JSON.stringify({ + type: "channel-thread-forget-failed", + note: "Could not delete the Intelligence thread for a removed channel.", + channelId, + threadId, + }), + ); + } + } + + await recordDeleted(context, channelId, { threadId, threadForgotten }); + + /* + * 200 with the outcome, not a bare 204. + * + * The thread deletion is the half that can fail on its own, and 204 says the whole act + * succeeded whichever way it went. The screen then tells somebody their message history is + * gone while it is still sitting on the platform, which is the one thing a person deleting a + * conversation is asking about. + * + * Reported as the question the caller has rather than the two facts it is derived from: no + * thread and a forgotten thread both mean nothing was left behind, and only a thread that + * survived is worth putting on a screen. + */ + return context.json( + { historyLeftBehind: threadId !== null && !threadForgotten }, + 200, + ); + } catch (error) { + return mapStoreError(context, error); + } + }); + return routes; } diff --git a/server/tests/channel-routes.test.ts b/server/tests/channel-routes.test.ts index af81fc33..17906cc7 100644 --- a/server/tests/channel-routes.test.ts +++ b/server/tests/channel-routes.test.ts @@ -1,4 +1,11 @@ -import { afterAll, afterEach, describe, expect, test } from "bun:test"; +import { + afterAll, + afterEach, + beforeEach, + describe, + expect, + test, +} from "bun:test"; import { randomUUID } from "node:crypto"; import { eq } from "drizzle-orm"; import type { MiddlewareHandler } from "hono"; @@ -9,6 +16,8 @@ import { } from "../src/agents/profile-store"; import type { AgentActor } from "../src/agents/profile-types"; import { createApp } from "../src/app"; +import type { AuditEventInput, AuditStore } from "../src/audit"; +import { DEV_ACTOR } from "../src/auth/dev-actor"; import type { AppVariables } from "../src/auth/guards"; import { type AgentChannel, @@ -21,7 +30,6 @@ import { import { createThreadIdentity } from "../src/channels/thread-identity"; import { loadConfig } from "../src/config"; import { createDatabase } from "../src/db/client"; -import { TEST_POOL } from "./support/database"; import { agentProfiles, agents, @@ -31,6 +39,7 @@ import { intelligenceChannelMappings, users, } from "../src/db/schema"; +import { TEST_POOL } from "./support/database"; import { testEnvironment } from "./support/environment"; const actor = { @@ -65,6 +74,10 @@ function fakeStore( calls.push(["get", receivedActor, id]); return channel({ id }); }, + async remove(receivedActor, id) { + calls.push(["remove", receivedActor, id]); + return "thread-1"; + }, }; return Object.assign(base, overrides, { calls }); @@ -292,6 +305,200 @@ describe("channel routes", () => { }); }); +describe("channel delete route", () => { + /** Rows written by the route under test, in order. */ + let audited: AuditEventInput[] = []; + + beforeEach(() => { + audited = []; + }); + + function appWithForget( + store: ChannelStore, + forgetThread?: (params: { + threadId: string; + userId: string; + agentId: string; + }) => Promise, + auditStore: AuditStore = { + insert: async (event) => void audited.push(event), + }, + ) { + const app = new Hono<{ Variables: AppVariables }>(); + app.route( + "/", + createChannelRoutes( + store, + requireUser, + undefined, + auditStore, + forgetThread, + ), + ); + return app; + } + + test("returns 200 and calls store.remove with the actor and channel id", async () => { + const store = fakeStore(); + const response = await appWithForget(store, async () => {}).request( + "http://openbot.test/channel-1", + { method: "DELETE" }, + ); + + expect(response.status).toBe(200); + expect(await json(response)).toEqual({ historyLeftBehind: false }); + expect(store.calls).toEqual([["remove", actor, "channel-1"]]); + }); + + test("maps ChannelNotFoundError to 404", async () => { + const store = fakeStore({ + remove: async () => { + throw new ChannelNotFoundError("channel-1"); + }, + }); + const response = await appWithForget(store).request( + "http://openbot.test/channel-1", + { method: "DELETE" }, + ); + + expect(response.status).toBe(404); + expect(await json(response)).toEqual({ error: "Channel not found." }); + }); + + test("calls forgetThread with the derived channel-scoped agent id", async () => { + const store = fakeStore(); + const calls: unknown[] = []; + const response = await appWithForget(store, async (params) => { + calls.push(params); + }).request("http://openbot.test/channel-1", { method: "DELETE" }); + + expect(response.status).toBe(200); + expect(calls).toEqual([ + { threadId: "thread-1", userId: actor.id, agentId: "channel:channel-1" }, + ]); + }); + + /* + * The critical ordering-decision regression test: a rejected upstream delete must not become a + * failure response. The local removal already committed by the time `forgetThread` runs, so this + * MUST fail if a later change propagates that rejection as an error status instead of swallowing + * it and still answering 204. + */ + test("still succeeds when forgetThread rejects, and says the history survived", async () => { + const store = fakeStore(); + const response = await appWithForget(store, async () => { + throw new Error("Intelligence is unreachable"); + }).request("http://openbot.test/channel-1", { method: "DELETE" }); + + expect(response.status).toBe(200); + // The half that failed is the half a person deleting a conversation is asking about, so it has + // to reach them rather than being swallowed into an indistinguishable success. + expect(await json(response)).toEqual({ historyLeftBehind: true }); + }); + + test("does not call forgetThread when remove found no thread to forget", async () => { + const store = fakeStore({ + remove: async (receivedActor, id) => { + store.calls.push(["remove", receivedActor, id]); + return null; + }, + }); + let called = false; + const response = await appWithForget(store, async () => { + called = true; + }).request("http://openbot.test/channel-1", { method: "DELETE" }); + + expect(response.status).toBe(200); + // Nothing to forget is not something left behind. + expect(await json(response)).toEqual({ historyLeftBehind: false }); + expect(called).toBe(false); + }); + + /* + * The channel row is gone by the time this runs, so this row is the only thing left that says the + * conversation ever existed or who ended it. Untested, it is also the easiest thing to drop in a + * later refactor without anything going red. + */ + test("writes an attributed audit row naming the thread it forgot", async () => { + const store = fakeStore(); + await appWithForget(store, async () => {}).request( + "http://openbot.test/channel-1", + { method: "DELETE" }, + ); + + expect(audited).toEqual([ + { + eventType: "channel.deleted", + targetType: "channel", + targetId: "channel-1", + actorUserId: actor.id, + payload: { threadId: "thread-1", threadForgotten: true }, + }, + ]); + }); + + test("records a thread that outlived the channel", async () => { + const store = fakeStore(); + await appWithForget(store, async () => { + throw new Error("Intelligence is unreachable"); + }).request("http://openbot.test/channel-1", { method: "DELETE" }); + + expect(audited).toEqual([ + { + eventType: "channel.deleted", + targetType: "channel", + targetId: "channel-1", + actorUserId: actor.id, + payload: { threadId: "thread-1", threadForgotten: false }, + }, + ]); + }); + + /* + * Single-user is the mode `.env.example` ships switched on, so this is the row a fork sees by + * default. The other audited surfaces drop the id here, believing `audit_events.actor_user_id` + * has a foreign key into `users`; it has none, and `initializeDevActorUser` writes that row at + * start-up regardless. An unattributed row would answer "was this conversation deleted" and not + * "by whom", which is the half worth keeping. + */ + test("attributes the local development actor rather than dropping it", async () => { + const store = fakeStore(); + const app = new Hono<{ Variables: AppVariables }>(); + app.route( + "/", + createChannelRoutes( + store, + async (context, next) => { + context.set("actor", DEV_ACTOR); + await next(); + }, + undefined, + { insert: async (event) => void audited.push(event) }, + async () => {}, + ), + ); + + await app.request("http://openbot.test/channel-1", { method: "DELETE" }); + + expect(audited[0]?.actorUserId).toBe(DEV_ACTOR.id); + }); + + /* + * The channel is already gone and the caller has already been told so. A trail that is briefly + * unavailable is not a reason to report a failure that did not happen. + */ + test("still answers when the audit write throws", async () => { + const store = fakeStore(); + const response = await appWithForget(store, async () => {}, { + insert: async () => { + throw new Error("audit table is unreachable"); + }, + }).request("http://openbot.test/channel-1", { method: "DELETE" }); + + expect(response.status).toBe(200); + }); +}); + describe("channel route composition", () => { test("mounts the store behind createApp authentication with the derived actor", async () => { const store = fakeStore(); @@ -739,6 +946,45 @@ describe("channel store integration", () => { expect(await channelTableSnapshot()).toEqual(before); }, ); + + test("deletes the channel row and cascades memberships, agents, and the thread mapping", async () => { + const actor = await createPersistentUser(); + const agentId = await createPersistentAgent({ + name: "Removable agent", + owner: actor, + }); + const created = await persistentStore.create(actor, [agentId]); + // Not pushed to createdChannelIds: `remove` is the thing under test, and afterEach's cleanup + // deleting an already-deleted row is a no-op either way. + + const threadId = await persistentStore.remove(actor, created.id); + + expect(threadId).toBe(created.threadId); + const persisted = await persistedChannel(created.id); + expect(persisted.channelRow).toBeUndefined(); + expect(persisted.memberships).toEqual([]); + expect(persisted.linkedAgents).toEqual([]); + expect(persisted.mappings).toEqual([]); + }); + + test("refuses a non-member's remove and leaves the channel row untouched", async () => { + const owner = await createPersistentUser(); + const outsider = await createPersistentUser(); + const agentId = await createPersistentAgent({ + name: "Guarded agent", + owner, + }); + const created = await persistentStore.create(owner, [agentId]); + createdChannelIds.push(created.id); + + await expect( + persistentStore.remove(outsider, created.id), + ).rejects.toBeInstanceOf(ChannelNotFoundError); + + expect((await persistedChannel(created.id)).channelRow).toMatchObject({ + id: created.id, + }); + }); }); /**