diff --git a/src/client/components/common/session-actions-menu/index.tsx b/src/client/components/common/session-actions-menu/index.tsx new file mode 100644 index 00000000..9fb27506 --- /dev/null +++ b/src/client/components/common/session-actions-menu/index.tsx @@ -0,0 +1,201 @@ +/** + * @license + * SPDX-License-Identifier: Apache-2.0 + */ + +/** + * One overflow menu for a session row, shared by the desktop sidebar and the + * mobile one. + * + * The row used to carry its actions as side-by-side hover icons (rename, + * archive, delete). That does not scale: three icons cost roughly a third of + * the title's width in the 268px sidebar, and every action added from here + * takes another slice. One trigger whose contents grow is the shape that + * survives the next action. + * + * Two ways in, one menu out: + * - the `⋯` button, which is the discoverable affordance; + * - a right-click anywhere on the row, which is the accelerator. Both are + * wired by the row component through `useRowMenu`. + * + * The menu positions itself from its OWN measured size. The item list is + * conditional (a pending `new-…` chat has no Rename or Delete), so a constant + * height would put the last row off-screen at the viewport edge — the same + * class of bug the file explorer's context menu works around with hand-tuned + * numbers. + */ + +import { useCallback, useEffect, useLayoutEffect, useRef, useState } from 'preact/hooks'; +import type { TargetedMouseEvent } from 'preact'; +import { createPortal } from 'preact/compat'; +import { Archive, ArchiveRestore, Pencil, Trash2 } from 'lucide-preact'; + +/** Viewport coordinates the menu hangs off, plus which edge it aligns to. */ +export interface MenuAnchor { + top: number; + right: number; + bottom: number; + left: number; + /** + * `end` hangs the menu's right edge off the anchor's — what a row's overflow + * button expects, since it sits at the row's end. `start` puts the menu's + * left edge at the anchor's, so a context-menu click opens away from the + * pointer rather than under it. + */ + align: 'start' | 'end'; +} + +export interface RowMenuHandle { + /** Non-null while the menu is open. */ + anchor: MenuAnchor | null; + /** Open below the clicked element (the `⋯` button). */ + openBelow: (event: TargetedMouseEvent) => void; + /** Open at the pointer (a right-click on the row). */ + openAtCursor: (event: TargetedMouseEvent) => void; + close: () => void; +} + +/** Anchor state for one row's overflow menu. */ +export function useRowMenu(): RowMenuHandle { + const [anchor, setAnchor] = useState(null); + + const openBelow = useCallback((event: TargetedMouseEvent) => { + event.preventDefault(); + // The row underneath selects the session on click; opening its menu is not + // a selection. + event.stopPropagation(); + const rect = event.currentTarget.getBoundingClientRect(); + setAnchor({ top: rect.top, right: rect.right, bottom: rect.bottom, left: rect.left, align: 'end' }); + }, []); + + const openAtCursor = useCallback((event: TargetedMouseEvent) => { + event.preventDefault(); + event.stopPropagation(); + const { clientX, clientY } = event; + setAnchor({ top: clientY, right: clientX, bottom: clientY, left: clientX, align: 'start' }); + }, []); + + const close = useCallback(() => setAnchor(null), []); + + return { anchor, openBelow, openAtCursor, close }; +} + +export interface SessionActionsMenuProps { + anchor: MenuAnchor; + isArchived?: boolean; + /** Omitted when the row cannot be renamed (a pending `new-…` chat). */ + onRename?: () => void; + onArchive?: () => void; + onDelete?: () => void; + onClose: () => void; +} + +export function SessionActionsMenu({ + anchor, + isArchived = false, + onRename, + onArchive, + onDelete, + onClose, +}: SessionActionsMenuProps) { + const menuRef = useRef(null); + const [pos, setPos] = useState<{ top: number; left: number } | null>(null); + + useLayoutEffect(() => { + const el = menuRef.current; + if (!el) return; + const { width, height } = el.getBoundingClientRect(); + const margin = 8; + const below = anchor.bottom + 4; + // Flip above the trigger rather than overflowing the bottom edge. + const top = below + height + margin <= window.innerHeight + ? below + : Math.max(margin, anchor.top - height - 4); + const desiredLeft = anchor.align === 'end' ? anchor.right - width : anchor.left; + setPos({ + top, + left: Math.max(margin, Math.min(desiredLeft, window.innerWidth - width - margin)), + }); + }, [anchor]); + + useEffect(() => { + const onKeyDown = (e: globalThis.KeyboardEvent) => { + if (e.key !== 'Escape') return; + e.preventDefault(); + onClose(); + }; + window.addEventListener('keydown', onKeyDown); + return () => window.removeEventListener('keydown', onKeyDown); + }, [onClose]); + + // Every item closes the menu first: Rename opens an inline editor in the row, + // and leaving the menu over it would cover the input. + const run = (action?: () => void) => () => { + onClose(); + action?.(); + }; + + return createPortal( +
{ + e.preventDefault(); + onClose(); + }} + > + , + document.body, + ); +} diff --git a/src/client/components/common/session-delete-modal/index.tsx b/src/client/components/common/session-delete-modal/index.tsx new file mode 100644 index 00000000..5278384d --- /dev/null +++ b/src/client/components/common/session-delete-modal/index.tsx @@ -0,0 +1,128 @@ +/** + * @license + * SPDX-License-Identifier: Apache-2.0 + */ + +import { useEffect } from 'preact/hooks'; +import { AlertTriangle, Loader2, Trash2 } from 'lucide-preact'; +import { Modal } from '@/client/components/common/Modal'; +import type { SessionItemData } from '@/shared/types'; + +export interface SessionDeleteModalProps { + /** The session awaiting confirmation, or null when the dialog is closed. */ + session: SessionItemData | null; + /** True while the DELETE request is in flight. */ + isDeleting: boolean; + /** Why the server refused, shown in place of the warning strip. */ + error: string | null; + onClose: () => void; + onConfirm: () => void; +} + +/** + * Confirmation for deleting a session. + * + * The dialog names what actually disappears — the transcript on disk, the + * subagent transcripts beside it, the side questions asked in it, and the + * chamber's own rows for it — because none of it is recoverable from the UI. + * That is the whole difference between this button and the archive button next + * to it, and a bare "Delete?" would leave the user guessing which one they + * pressed. + * + * A refusal is rendered INSIDE the dialog rather than as a toast: the reasons + * the server gives (the session is mid-run, or the chat has not been created + * yet) are actionable, and the user is standing in the place where they act on + * them. + */ +export function SessionDeleteModal({ + session, + isDeleting, + error, + onClose, + onConfirm, +}: SessionDeleteModalProps) { + useEffect(() => { + // No dismissing mid-delete: the dialog resolves itself on success, and an + // early close would let a second delete be queued against a stale row. + const handler = (e: globalThis.KeyboardEvent) => { + if (e.key !== 'Escape' || isDeleting) return; + e.preventDefault(); + onClose(); + }; + window.addEventListener('keydown', handler); + return () => window.removeEventListener('keydown', handler); + }, [onClose, isDeleting]); + + if (!session) return null; + + return ( + {} : onClose} + zClass="z-[70]" + maxWidthClass="max-w-[460px]" + header={ +
+ +
+

Delete session?

+

{session.title}

+
+
+ } + footer={ +
+ + +
+ } + > +
+
    +
  • + Its transcript is removed from{' '} + ~/.omp/agent/sessions — the conversation is + gone from the sidebar and from oh-my-pi. +
    • +
  • The subagent transcripts and any side questions asked in it are removed with it.
    • +
  • Its queued messages, archive state, and saved panel layout go too.
    • +
+ + {error ? ( +
+ + {error} +
+ ) : ( +
+ + This cannot be undone from the UI. Archive it instead to keep the transcript. +
+ )} +
+ + ); +} diff --git a/src/client/components/layout/session-sidebar/CategoryItem.tsx b/src/client/components/layout/session-sidebar/CategoryItem.tsx index ee58f625..6bf81d18 100644 --- a/src/client/components/layout/session-sidebar/CategoryItem.tsx +++ b/src/client/components/layout/session-sidebar/CategoryItem.tsx @@ -5,6 +5,8 @@ import { useFetcher } from '@/client/lib/router/fetcher'; import { useOnClickOutside } from '@/client/hooks/ui/on-click-outside'; import { useShowMore } from '@/client/hooks/ui/show-more'; import { useWorkspaceFolderActions } from '@/client/hooks/workspace/workspace-folder-actions'; +import { useSessionDelete } from '@/client/hooks/workspace/session-delete'; +import { SessionDeleteModal } from '@/client/components/common/session-delete-modal'; import { SessionItem } from '@/client/components/layout/session-sidebar/SessionItem'; import { SubagentList } from '@/client/components/common/subagent-list'; import { WorkspaceOptionsMenu } from '@/client/components/common/workspace-options-menu'; @@ -58,6 +60,7 @@ export function Category({ handleArchive, handleRename, } = useWorkspaceFolderActions(folder, refresh); + const sessionDelete = useSessionDelete(); // Desktop's expand toggle keeps its own fetcher: unlike pin/delete it // dispatches on the *response* (not immediately) and carries no folderId. @@ -203,6 +206,13 @@ export function Category({ awaitingInput={Boolean(session.awaitingInput)} onClick={() => onSelectSession(session.id)} onArchive={() => handleArchive(session)} + onDelete={ + // A pending `new-…` chat has no transcript anywhere yet — + // there is nothing to delete, and the server refuses it. + String(session.id).startsWith('new-') + ? undefined + : () => sessionDelete.requestDelete(session) + } onRename={String(session.id).startsWith('new-') ? undefined : (name) => void handleRename(session, name)} expandable={hasSubagents} hasSubagents={hasSubagents} @@ -236,6 +246,14 @@ export function Category({ )}
)} + + void sessionDelete.confirmDelete()} + /> ); } diff --git a/src/client/components/layout/session-sidebar/SessionItem.tsx b/src/client/components/layout/session-sidebar/SessionItem.tsx index 1344aa3d..b8c21f73 100644 --- a/src/client/components/layout/session-sidebar/SessionItem.tsx +++ b/src/client/components/layout/session-sidebar/SessionItem.tsx @@ -1,6 +1,7 @@ import type { TargetedMouseEvent } from 'preact'; -import { Archive, ArchiveRestore, Check, ChevronDown, ChevronRight, CircleQuestionMark, Loader2, Pencil } from 'lucide-preact'; +import { Check, ChevronDown, ChevronRight, CircleQuestionMark, Loader2, MoreHorizontal } from 'lucide-preact'; import { useInlineRename } from '@/client/hooks/ui/inline-rename'; +import { SessionActionsMenu, useRowMenu } from '@/client/components/common/session-actions-menu'; export interface SessionItemProps { title: string; @@ -11,6 +12,9 @@ export interface SessionItemProps { awaitingInput?: boolean; onClick?: () => void; onArchive?: () => void; + /** Omitted for a session that cannot be deleted yet (a pending `new-…` chat, + * which has no transcript anywhere). */ + onDelete?: () => void; onRename?: (name: string) => void; expandable?: boolean; isExpanded?: boolean; @@ -26,6 +30,7 @@ export function SessionItem({ awaitingInput = false, onClick, onArchive, + onDelete, onRename, expandable = false, isExpanded = false, @@ -51,116 +56,121 @@ export function SessionItem({ handleKeyDown, handleBlur, } = useInlineRename(title, onRename); + // One trigger for every row action; also reachable by right-clicking the row. + const menu = useRowMenu(); + const hasActions = Boolean(onRename || onArchive || onDelete); return ( -
- {/* One 16px slot owns both icons — never two side by side. A live run - signal (blocked question, run spinner) holds the slot; otherwise the - chevron is pinned, collapsed or expanded. While the signal holds it, - hovering hands the slot to the chevron so the roster stays reachable - mid-run. - Both layers are pointer-inert for the opposite state: the toggle - while hidden, the status glyph ALWAYS (it is painted after the - toggle, so without that it wins the hit test over the revealed - chevron and every click on a badged row selects the session instead - of expanding it). */} - - {showChevron && onToggleExpand && ( - - )} - {showStatus && ( - - {/* Waiting on an answer outranks the run spinner: the spinner says - work is happening, the question mark says it is YOUR turn. */} - {awaitingInput ? ( - - ) : status === 'stream' ? ( - - ) : ( - - )} - - )} - - - {/* Gap between icon and text */} - - - {/* Session Title - aligned straight with Folder Name */} - {isEditing ? ( - setDraft(e.currentTarget.value)} - onClick={(e) => e.stopPropagation()} - onKeyDown={handleKeyDown} - onBlur={handleBlur} - className="flex-1 min-w-0 bg-paper border border-ink/25 rounded px-1 py-0.5 text-xs text-ink outline-none focus:border-ink/50" - /> - ) : ( - - {title.charAt(0).toUpperCase() + title.slice(1)} - - )} - - {/* Quick Actions: Rename / Archive on Hover */} - {!isEditing && (onRename || onArchive) && ( -
- {onRename && ( + <> +
+ {/* One 16px slot owns both icons — never two side by side. A live run + signal (blocked question, run spinner) holds the slot; otherwise the + chevron is pinned, collapsed or expanded. While the signal holds it, + hovering hands the slot to the chevron so the roster stays reachable + mid-run. + Both layers are pointer-inert for the opposite state: the toggle + while hidden, the status glyph ALWAYS (it is painted after the + toggle, so without that it wins the hit test over the revealed + chevron and every click on a badged row selects the session instead + of expanding it). */} + + {showChevron && onToggleExpand && ( )} - {onArchive && ( - + {/* Waiting on an answer outranks the run spinner: the spinner says + work is happening, the question mark says it is YOUR turn. */} + {awaitingInput ? ( + + ) : status === 'stream' ? ( + + ) : ( + + )} + )} -
+ + + {/* Gap between icon and text */} + + + {/* Session Title - aligned straight with Folder Name */} + {isEditing ? ( + setDraft(e.currentTarget.value)} + onClick={(e) => e.stopPropagation()} + onKeyDown={handleKeyDown} + onBlur={handleBlur} + className="flex-1 min-w-0 bg-paper border border-ink/25 rounded px-1 py-0.5 text-xs text-ink outline-none focus:border-ink/50" + /> + ) : ( + + {title.charAt(0).toUpperCase() + title.slice(1)} + + )} + + {/* One trigger, whatever the row can do. The three side-by-side hover + icons this replaced cost a third of the title's width in a 268px + sidebar, and each further action took another slice — the menu grows + instead. Revealed on hover/focus; a right-click anywhere on the row + opens the same menu. */} + {!isEditing && hasActions && ( + + )} + +
+ + {menu.anchor && ( + )} -
+ ); } diff --git a/src/client/components/mobile/mobile-session-sidebar/Item.tsx b/src/client/components/mobile/mobile-session-sidebar/Item.tsx index b9775670..818c75f8 100644 --- a/src/client/components/mobile/mobile-session-sidebar/Item.tsx +++ b/src/client/components/mobile/mobile-session-sidebar/Item.tsx @@ -7,6 +7,8 @@ import { WorkspaceOptionsMenu } from '@/client/components/common/workspace-optio import { useShowMore } from '@/client/hooks/ui/show-more'; import { useExpandedSessions } from '@/client/hooks/workspace/expanded-sessions'; import { useWorkspaceFolderActions } from '@/client/hooks/workspace/workspace-folder-actions'; +import { useSessionDelete } from '@/client/hooks/workspace/session-delete'; +import { SessionDeleteModal } from '@/client/components/common/session-delete-modal'; import { getProjectIcon } from '@/shared/lib/workspace/project-icon'; import { relativeTimeAgo } from '@/shared/lib/workspace/relative-time'; import { useOnClickOutside } from '@/client/hooks/ui/on-click-outside'; @@ -53,6 +55,7 @@ export function MobileSessionCategory({ handleArchive, handleRename, } = useWorkspaceFolderActions(folder, refresh); + const sessionDelete = useSessionDelete(); useOnClickOutside(menuRef, () => { setShowMenu(false); @@ -195,6 +198,13 @@ export function MobileSessionCategory({ onToggleExpand={() => toggleSession(sessionKey)} onSelect={() => onSelectSession(session.id)} onArchive={() => handleArchive(session)} + onDelete={ + // A pending `new-…` chat has no transcript anywhere yet — + // there is nothing to delete, and the server refuses it. + sessionKey.startsWith('new-') + ? undefined + : () => sessionDelete.requestDelete(session) + } onRename={sessionKey.startsWith('new-') ? undefined : (name) => void handleRename(session, name)} /> {isRosterOpen && ( @@ -216,6 +226,14 @@ export function MobileSessionCategory({ )} )} + + void sessionDelete.confirmDelete()} + /> ); } diff --git a/src/client/components/mobile/mobile-session-sidebar/SessionRow.tsx b/src/client/components/mobile/mobile-session-sidebar/SessionRow.tsx index 55632335..9dd28374 100644 --- a/src/client/components/mobile/mobile-session-sidebar/SessionRow.tsx +++ b/src/client/components/mobile/mobile-session-sidebar/SessionRow.tsx @@ -1,6 +1,7 @@ -import { Archive, ArchiveRestore, Check, ChevronDown, ChevronRight, CircleQuestionMark, Loader2, Pencil } from 'lucide-preact'; +import { Check, ChevronDown, ChevronRight, CircleQuestionMark, Loader2, MoreHorizontal } from 'lucide-preact'; import type { SessionItemData } from '@/shared/types'; import { useInlineRename } from '@/client/hooks/ui/inline-rename'; +import { SessionActionsMenu, useRowMenu } from '@/client/components/common/session-actions-menu'; export interface MobileSessionRowProps { session: SessionItemData; @@ -16,6 +17,9 @@ export interface MobileSessionRowProps { onToggleExpand?: () => void; onSelect: () => void; onArchive: () => void; + /** Omitted for a session that cannot be deleted yet (a pending `new-…` chat, + * which has no transcript anywhere). */ + onDelete?: () => void; onRename?: (name: string) => void; } @@ -30,6 +34,7 @@ export function MobileSessionRow({ onToggleExpand, onSelect, onArchive, + onDelete, onRename, }: MobileSessionRowProps) { const { @@ -41,6 +46,9 @@ export function MobileSessionRow({ handleKeyDown, handleBlur, } = useInlineRename(session.title, onRename); + // One trigger for every row action; also reachable by long-press contextmenu. + const menu = useRowMenu(); + const hasActions = Boolean(onRename || onArchive || onDelete); // The roster toggle is the row's LAST element, so its glyph lands in the same // column as the folder header's expand chevron — one vertical line of @@ -71,76 +79,86 @@ export function MobileSessionRow({ } return ( -
- - - {onRename && ( - )} - {/* A row WITHOUT a roster toggle still has to keep the toggle's column - empty, or its archive glyph would sit where the toggle is and the - trailing icons would step in and out row to row. This `mr-1` is what - reserves that column: it moves the 14px glyph's centre from x=363 to - x=359, the exact x the toggle uses on a row that has one. */} - + {/* One trigger for rename / archive / delete, always visible: a touch + surface has no hover, and the always-on action buttons this replaced + consumed a third of a phone's row width. + `mr-1` only when the row has NO roster toggle — it reserves that + column so the trailing glyph's centre stays 19px from the row's right + edge whether the last element is this trigger or the chevron. Without + it the glyphs step in and out from row to row. */} + {hasActions && ( + + )} - {/* Last element, so its glyph sits in the folder header's chevron column. - `p-1.5 mr-1.5` centres a 14px glyph on that column while keeping a - touch-sized tap target. */} - {showChevron && ( - + {/* Last element, so its glyph sits in the folder header's chevron column. + `p-1.5 mr-1.5` centres a 14px glyph on that column while keeping a + touch-sized tap target. */} + {showChevron && ( + + )} +
+ + {menu.anchor && ( + )} - + ); } diff --git a/src/client/hooks/workspace/session-delete.ts b/src/client/hooks/workspace/session-delete.ts new file mode 100644 index 00000000..01a87d32 --- /dev/null +++ b/src/client/hooks/workspace/session-delete.ts @@ -0,0 +1,94 @@ +/** + * @license + * SPDX-License-Identifier: Apache-2.0 + */ + +/** + * Session deletion, shared by the desktop session row and the mobile one. + * + * Both sidebars render the same confirmation dialog and issue the same request, + * so the state machine (which row is pending, in-flight, why it was refused) + * lives here rather than in two components that would drift on the URL, the + * verb, or the event they dispatch afterwards. + * + * Two follow-ups are load-bearing and both happen only on success: + * + * - `omp:workspace-updated` refreshes the sidebar list. The deleted row is + * gone from disk, but the client still holds the previous snapshot. + * - when the deleted session is the one on screen, `?sessionId=` is dropped. + * Leaving the URL pointing at a removed session renders the chat's "not + * found" state over a row the user just deleted, and every panel keyed to + * that id keeps fetching for it. + */ + +import { useCallback, useState } from 'preact/hooks'; +import { useSearchParams } from '@/client/lib/router/search-params'; +import { forgetSession } from '@/shared/lib/workspace/session-state/store'; +import type { SessionItemData } from '@/shared/types'; + +export interface SessionDeleteActions { + /** The session awaiting confirmation, or null when the dialog is closed. */ + pending: SessionItemData | null; + /** True while the DELETE request is in flight. */ + isDeleting: boolean; + /** The server's refusal reason, shown inside the dialog. */ + error: string | null; + /** Open the confirmation for this session. */ + requestDelete: (session: SessionItemData) => void; + /** Close it without deleting. */ + cancelDelete: () => void; + /** Issue the delete. Resolves once the request settled. */ + confirmDelete: () => Promise; +} + +export function useSessionDelete(): SessionDeleteActions { + const [pending, setPending] = useState(null); + const [isDeleting, setIsDeleting] = useState(false); + const [error, setError] = useState(null); + const [searchParams, setSearchParams] = useSearchParams(); + + const requestDelete = useCallback((session: SessionItemData) => { + setError(null); + setPending(session); + }, []); + + const cancelDelete = useCallback(() => { + setError(null); + setPending(null); + }, []); + + const confirmDelete = useCallback(async () => { + if (!pending || isDeleting) return; + const sessionId = String(pending.id); + setIsDeleting(true); + setError(null); + try { + const res = await fetch(`/api/sessions/${encodeURIComponent(sessionId)}`, { method: 'DELETE' }); + const body = (await res.json().catch(() => null)) as { error?: string } | null; + if (!res.ok) { + // The refusal reasons the server gives (mid-run, pending chat) are + // actionable, so they stay on screen instead of closing the dialog. + setError(body?.error || `Could not delete the session (HTTP ${res.status}).`); + return; + } + forgetSession(sessionId); + if (searchParams.get('sessionId') === sessionId) { + setSearchParams((prev) => { + const next = new URLSearchParams(prev); + next.delete('sessionId'); + // A subagent transcript belongs to the session being removed. + next.delete('subagent'); + return next; + }, { replace: true }); + } + setPending(null); + window.dispatchEvent(new CustomEvent('omp:workspace-updated')); + } catch (err) { + setError(err instanceof Error ? err.message : 'Could not reach the server.'); + } finally { + setIsDeleting(false); + } + }, [pending, isDeleting, searchParams, setSearchParams]); + + return { pending, isDeleting, error, requestDelete, cancelDelete, confirmDelete }; +} diff --git a/src/server/lib/btw/purge.server.ts b/src/server/lib/btw/purge.server.ts new file mode 100644 index 00000000..a12c7211 --- /dev/null +++ b/src/server/lib/btw/purge.server.ts @@ -0,0 +1,56 @@ +/** + * @license + * SPDX-License-Identifier: Apache-2.0 + */ + +/** + * Deleting a session's side questions. + * + * A side question is keyed to the parent session but stored outside it — rows + * in `btw_topics`/`btw_turns`, a live omp child per topic, and a private + * transcript directory under `/btw///`. Deleting + * the chat session therefore leaves all three behind unless they are purged + * with it: orphan turns still readable in the database, and on disk a full + * copy of the very transcript the user just deleted. + * + * Kept out of `service.server.ts` (which owns the interactive topic + * operations) so that file's growth stays attributable. + */ + +import path from 'path'; +import { listBtwTopics, deleteBtwTopic } from '@/server/lib/btw/store.server'; +import { forgetBtwRuntime, getBtwRuntime } from '@/server/lib/btw/registry.server'; +import { getBtwRoot, removeBtwWorkspace, resolveBtwWorkspacePaths } from '@/server/lib/btw/session-copy.server'; +import { isSinglePathSegment } from '@/server/lib/fs/path-segment'; + +/** + * Drop every topic of `sessionId`, its child, its rows, and its workspace. + * + * `sessionId` MUST name one child of the btw root. The last step below removes + * `join(getBtwRoot(), sessionId)` unconditionally, and `join(root, '.')` is + * `root` itself — so an id of `.` (or any id with a separator) turns this + * per-session purge into "delete every session's side questions on disk". The + * route validates its id; this check is the independent second gate, because + * this function is reachable on its own and its failure mode is other + * sessions' data. + */ +export async function purgeBtwForSession(sessionId: string): Promise { + if (!isSinglePathSegment(sessionId)) return 0; + + const topics = await listBtwTopics(sessionId); + for (const topic of topics) { + const runtime = getBtwRuntime(topic.id); + if (runtime) { + // Disposes the child; a running turn is settled as cancelled rather than + // left to write into a transcript this purge is about to remove. + await runtime.dispose(); + forgetBtwRuntime(topic.id); + } + await deleteBtwTopic(topic.id); + await removeBtwWorkspace((await resolveBtwWorkspacePaths(sessionId, topic.id)).dir); + } + // The per-session directory as a whole: a topic whose row is already gone can + // still have a workspace (a crash between the two, a manual row delete). + await removeBtwWorkspace(path.join(await getBtwRoot(), sessionId)); + return topics.length; +} diff --git a/src/server/lib/fs/path-segment.test.ts b/src/server/lib/fs/path-segment.test.ts new file mode 100644 index 00000000..e14da41e --- /dev/null +++ b/src/server/lib/fs/path-segment.test.ts @@ -0,0 +1,100 @@ +/** + * @license + * SPDX-License-Identifier: Apache-2.0 + */ + +/** + * The session-delete blast radius. + * + * `DELETE /api/sessions/:sessionId` removes a directory under the sessions root + * and another under the btw root, both by joining the id onto the root. The + * case that shipped broken was `.`: `join(root, '.')` is `root` itself, so the + * btw purge's unconditional workspace removal deleted EVERY session's side + * questions and the route still answered 404 — an invisible, root-wide wipe + * from a request that looked like a miss. + * + * These tests pin the two properties that made it possible: the guard rejects + * every value that is not one segment, and the btw purge refuses a non-segment + * even when called directly (the route is not the only entry point). + */ + +import { afterAll, beforeAll, describe, expect, test } from 'bun:test'; +import fs from 'fs'; +import path from 'path'; +import { isSinglePathSegment } from '@/server/lib/fs/path-segment'; +import { purgeBtwForSession } from '@/server/lib/btw/purge.server'; + +describe('isSinglePathSegment', () => { + test('accepts the id shapes the app actually produces', () => { + // omp session ids are UUIDs; mock ids are integers; pending chats are `new-…`. + expect(isSinglePathSegment('11111111-2222-3333-4444-555555555555')).toBe(true); + expect(isSinglePathSegment('3')).toBe(true); + expect(isSinglePathSegment('new-1758800000000')).toBe(true); + // A dot INSIDE a name is fine — only a name that resolves to a directory is not. + expect(isSinglePathSegment('session.v2')).toBe(true); + }); + + test('refuses the values that resolve somewhere other than one child', () => { + // The shipped bug: join(root, '.') === root. + expect(isSinglePathSegment('.')).toBe(false); + expect(isSinglePathSegment('..')).toBe(false); + expect(isSinglePathSegment('a/b')).toBe(false); + expect(isSinglePathSegment('..\\a')).toBe(false); + expect(isSinglePathSegment('')).toBe(false); + // A NUL byte truncates at the syscall boundary. + expect(isSinglePathSegment('a\0b')).toBe(false); + }); + + test('the guard agrees with the join it is guarding', () => { + // The property that matters: whenever the guard says yes, joining onto a + // root stays directly inside it. Asserted against `path.join` itself rather + // than a restatement of the rule. + const root = '/tmp/root'; + for (const id of ['11111111-2222-3333-4444-555555555555', '3', 'new-1', 'session.v2']) { + expect(isSinglePathSegment(id)).toBe(true); + expect(path.join(root, id).startsWith(`${root}${path.sep}`)).toBe(true); + expect(path.dirname(path.join(root, id))).toBe(root); + } + for (const id of ['.', '..', 'a/b', '../x']) { + expect(isSinglePathSegment(id)).toBe(false); + } + }); +}); + +describe('purgeBtwForSession refuses a non-segment id', () => { + const root = '/tmp/omc-path-segment-test'; + const btwRoot = path.join(root, 'btw'); + const seeded = [ + path.join(btwRoot, 'sessA', 'topicA', 'session.jsonl'), + path.join(btwRoot, 'sessB', 'topicB', 'session.jsonl'), + ]; + + beforeAll(async () => { + await fs.promises.rm(root, { recursive: true, force: true }); + // Two unrelated sessions' side questions, exactly as the bug destroyed them. + // The btw root is derived from the database path, so pointing that at this + // tree is what puts them where the purge would look. + for (const file of seeded) { + await fs.promises.mkdir(path.dirname(file), { recursive: true }); + await Bun.write(file, '{"type":"session"}\n'); + } + Bun.env.OMPCHAMBER_DB_PATH = path.join(root, 'db.sqlite'); + }); + + afterAll(async () => { + delete Bun.env.OMPCHAMBER_DB_PATH; + await fs.promises.rm(root, { recursive: true, force: true }); + }); + + test('`.` does not remove the btw root', async () => { + // The guard returns before `listBtwTopics`, so no database is touched. + expect(await purgeBtwForSession('.')).toBe(0); + // Both sessions' transcripts must survive — this is the regression. + for (const file of seeded) expect(fs.existsSync(file)).toBe(true); + }); + + test('a separator does not reach outside the root', async () => { + expect(await purgeBtwForSession('../btw')).toBe(0); + for (const file of seeded) expect(fs.existsSync(file)).toBe(true); + }); +}); diff --git a/src/server/lib/fs/path-segment.ts b/src/server/lib/fs/path-segment.ts new file mode 100644 index 00000000..ca4ba61e --- /dev/null +++ b/src/server/lib/fs/path-segment.ts @@ -0,0 +1,45 @@ +/** + * @license + * SPDX-License-Identifier: Apache-2.0 + */ + +/** + * One rule, two callers: does this id name exactly one child of a root? + * + * The purge paths join a session id onto a root to find the directory it owns — + * `join(sessionsRoot, …)`, `join(btwRoot, sessionId)` — and then remove it + * recursively. `path.join` NORMALIZES, so a value that is not a plain segment + * silently retargets the removal: + * + * join(root, '.') === root → deletes every child of the root + * join(root, '..') === dirname(root) → deletes outside the root + * join(root, 'a/b') → reaches into a sibling subtree + * + * Measured: `DELETE /api/sessions/.` (curl needs `--path-as-is`; it strips the + * trailing dot itself) passed a `/^[A-Za-z0-9._-]+$/` guard, normalized to the + * btw root, and removed two unrelated sessions' side-question transcripts. + * + * The check is deliberately structural rather than an enumeration of dangerous + * spellings: an id is safe exactly when it is one non-dot segment. Real ids + * satisfy that by construction — omp session ids are UUIDs and mock ids are + * integers — so nothing legitimate is refused. + * + * `normalize` is compared, not just scanned for separators, because it is the + * same operation the callers' `join` performs: asserting the two agree is what + * makes this a proof about the path that will actually be built, rather than a + * guess about which inputs look suspicious. + */ + +import { normalize } from 'node:path'; + +/** True when `id` names one child of a root, and nothing else. */ +export function isSinglePathSegment(id: string): boolean { + if (id.length === 0) return false; + if (id === '.' || id === '..') return false; + if (id.includes('/') || id.includes('\\')) return false; + // A NUL byte would truncate the path at the syscall boundary. + if (id.includes('\0')) return false; + // The load-bearing assertion: normalization must be a no-op, so `join(root, id)` + // cannot resolve anywhere but directly under `root`. + return normalize(id) === id; +} diff --git a/src/server/lib/omp/session/delete.server.ts b/src/server/lib/omp/session/delete.server.ts new file mode 100644 index 00000000..33ed263a --- /dev/null +++ b/src/server/lib/omp/session/delete.server.ts @@ -0,0 +1,136 @@ +/** + * @license + * SPDX-License-Identifier: Apache-2.0 + */ + +/** + * Deleting a session: the transcript on disk, its subagent artifacts, and every + * chamber-side row keyed to the session id. + * + * The transcript is authoritative — omp's own session listing is derived from + * the JSONL alone — so a delete that removed only the chamber's rows would see + * the session come straight back on the next sidebar scan. What "delete" means + * here, in one place: + * + * - the session JSONL, plus any `.jsonl.bak-*` copy an earlier rewind + * left beside it (a full copy of the same transcript: leaving it behind + * would mean the conversation was not actually deleted); + * - its sibling artifacts directory, which holds the subagent transcripts; + * - the `archived_sessions`, `session_ui_state`, `session_stream_state`, + * `queued_messages` and `chat_sessions` rows for that id; + * - in mock mode the demo `sessions` row and its `files` tree. + * + * The caller owns the process: a live omp child MUST be destroyed before this + * runs, because omp flushes its session state on shutdown and would otherwise + * recreate the file this just removed. Side-question rows and workspaces are + * the btw module's business (`purgeBtwForSession`). + */ + +import fs from 'fs'; +import path from 'path'; +import { getDb } from '@/server/db.server'; +import type { DbClient } from '@/server/lib/db/client'; +import { findSessionFileById } from '@/server/lib/omp/session/locator'; +import { clearSessionFileCaches } from '@/server/lib/omp/session/files'; +import { invalidateOmpSidebarData } from '@/server/lib/omp/session/reader'; +import { siblingDirForSession } from '@/server/lib/omp/subagent/history/paths'; +import { isSinglePathSegment } from '@/server/lib/fs/path-segment'; + +export interface SessionPurgeResult { + /** Something keyed to this id existed — a transcript, a mock session row, or + * a chamber row. False means the id resolved nowhere at all. */ + found: boolean; + fileRemoved: boolean; + artifactsRemoved: boolean; +} + +/** + * Chamber rows keyed by a session id. Every statement is a no-op when the id + * matches nothing, which is what makes one purge serve both modes: real + * sessions (omp UUIDs, present on disk only) and mock ones (numeric ids in + * `sessions`). + * + * `files` carries a FOREIGN KEY to `sessions` and `foreign_keys` is ON, so its + * children go before the session row or the delete is refused. + */ +const SESSION_ROW_DELETES: readonly (readonly [string, string])[] = [ + ['archived_sessions', 'session_id'], + ['session_ui_state', 'session_id'], + ['session_stream_state', 'session_id'], + ['queued_messages', 'session_id'], + ['chat_sessions', 'session_id'], + ['files', 'session_id'], +]; + +async function purgeChamberRows(db: DbClient, sessionId: string): Promise { + let changes = 0; + for (const [table, column] of SESSION_ROW_DELETES) { + const result = await db.run(`DELETE FROM ${table} WHERE ${column} = ?`, [sessionId]); + changes += result.changes ?? 0; + } + const mockRow = await db.run('DELETE FROM sessions WHERE id = ?', [sessionId]); + return changes + (mockRow.changes ?? 0); +} + +/** Remove the transcript and the `.bak-*` copies beside it. */ +async function removeTranscript(filePath: string): Promise { + const existed = await Bun.file(filePath).exists(); + await fs.promises.rm(filePath, { force: true }); + const dir = path.dirname(filePath); + const prefix = `${path.basename(filePath)}.bak-`; + try { + for (const entry of await fs.promises.readdir(dir)) { + if (entry.startsWith(prefix)) await fs.promises.rm(path.join(dir, entry), { force: true }); + } + } catch { + // A backup that cannot be listed is not worth failing a delete the + // transcript half of which already succeeded. + } + return existed; +} + +/** + * Purge everything the chamber and the agent directory hold for `sessionId`. + * `sessionId` must be a single safe path segment — it names the artifacts + * directory — which the route validates before calling. + */ +export async function purgeSessionData(sessionId: string): Promise { + // Second gate, independent of the route's: an id that is not one segment + // would make `siblingDirForSession` and the btw purge resolve outside the + // session it names. Refusing here means a future caller that forgets the + // route's guard still cannot widen the blast radius. + if (!isSinglePathSegment(sessionId)) { + return { found: false, fileRemoved: false, artifactsRemoved: false }; + } + + const filePath = await findSessionFileById(sessionId); + let fileRemoved = false; + let artifactsRemoved = false; + if (filePath) { + fileRemoved = await removeTranscript(filePath); + const artifactsDir = siblingDirForSession(filePath); + // `stat`, not `Bun.file().exists()`: the latter answers for regular files + // only and reports a directory as absent, which would claim every session + // had no subagent transcripts. + artifactsRemoved = await fs.promises + .stat(artifactsDir) + .then((stats) => stats.isDirectory(), () => false); + await fs.promises.rm(artifactsDir, { recursive: true, force: true }); + } + + const db = await getDb(); + const rowChanges = await purgeChamberRows(db, sessionId); + + // Both caches outlive the mutation by their own keys: the file list/scan pair + // is mtime-keyed (a delete bumps the parent directory, but the memo can still + // answer first) and the sidebar dataset is TTL'd, so without this the deleted + // row is served back to the refresh that follows the delete. + clearSessionFileCaches(); + invalidateOmpSidebarData(); + + return { + found: Boolean(filePath) || rowChanges > 0, + fileRemoved, + artifactsRemoved, + }; +} diff --git a/src/server/lib/omp/session/reader.ts b/src/server/lib/omp/session/reader.ts index 80a01960..de652635 100644 --- a/src/server/lib/omp/session/reader.ts +++ b/src/server/lib/omp/session/reader.ts @@ -60,6 +60,19 @@ export async function loadOmpSidebarData(): Promise { return slot.inFlight; } +/** + * Drop the cached dataset so the next load re-scans disk. For a mutation the + * TTL cannot see: deleting a session removes a row rather than changing one, so + * nothing in the cached snapshot is stale — it is simply wrong, and the refresh + * that follows the delete would otherwise serve it back for up to the TTL. + */ +export function invalidateOmpSidebarData(): void { + const slot = globalThis.__ompChamberSidebarDataCache; + if (!slot) return; + slot.data = undefined as unknown as OmpSidebarData; + slot.expiresAt = 0; +} + /** Resolve each unique cwd to its project root; bounded concurrency so 100+ * unique cwds don't spawn 100 parallel git processes. */ async function resolveRootsByCwd(cwds: string[]): Promise> { diff --git a/src/server/routes/sessions/delete.test.ts b/src/server/routes/sessions/delete.test.ts new file mode 100644 index 00000000..079a7584 --- /dev/null +++ b/src/server/routes/sessions/delete.test.ts @@ -0,0 +1,84 @@ +/** + * @license + * SPDX-License-Identifier: Apache-2.0 + */ + +/** + * The delete route's refusal branches. + * + * Each one guards a different failure, and two of them are about blast radius + * rather than tidiness: the id names a directory under the sessions root and + * another under the btw root, so a value that is not one segment deletes far + * more than the session it names (`.` normalized to the btw root itself and + * wiped every session's side questions — see `fs/path-segment`). + * + * The route is called directly, as the other route tests do: these are pure + * request-shape checks that must not reach the filesystem or spawn an omp + * child, and asserting that is half the point. + */ + +import { afterAll, beforeAll, describe, expect, test } from 'bun:test'; +import fs from 'fs'; +import path from 'path'; +import { deleteSession } from '@/server/routes/sessions/delete'; + +/** + * The 404 branch resolves the id by scanning the sessions directory, so the + * agent dir is pointed at an empty temp tree. Without this the test walks the + * developer's real `~/.omp/agent/sessions` (read-only, but seconds of I/O and a + * result that depends on what happens to be installed). + */ +const AGENT_DIR = '/tmp/omc-delete-route-test/agent'; + +beforeAll(() => { + fs.rmSync(AGENT_DIR, { recursive: true, force: true }); + fs.mkdirSync(path.join(AGENT_DIR, 'sessions'), { recursive: true }); + Bun.env.PI_CODING_AGENT_DIR = AGENT_DIR; +}); + +afterAll(() => { + delete Bun.env.PI_CODING_AGENT_DIR; + fs.rmSync('/tmp/omc-delete-route-test', { recursive: true, force: true }); +}); + +function del(sessionId: string): Promise { + return deleteSession({ + request: new Request(`http://localhost/api/sessions/${sessionId}`, { method: 'DELETE' }), + params: { sessionId }, + } as never) as Promise; +} + +describe('deleteSession refusals', () => { + test('a pending chat is refused — nothing is stored for it yet', async () => { + const res = await del('new-1758800000000'); + expect(res.status).toBe(400); + expect(await res.json()).toMatchObject({ code: 'session_pending' }); + }); + + test('`.` is refused instead of wiping the btw root', async () => { + const res = await del('.'); + expect(res.status).toBe(400); + expect(await res.json()).toMatchObject({ error: 'Invalid session id' }); + }); + + test('a traversal is refused', async () => { + for (const id of ['..', '../x', 'a/b', '..\\a']) { + expect((await del(id)).status).toBe(400); + } + }); + + test('an unknown but well-formed id is a 404, not a 400', async () => { + // Distinct from the branches above: the request is valid, the session is + // not there. Collapsing the two would report a typo as a malformed request. + const res = await del('00000000-0000-0000-0000-000000000000'); + expect(res.status).toBe(404); + }); + + test('a wrong verb on the path is a 405', async () => { + const res = (await deleteSession({ + request: new Request('http://localhost/api/sessions/x', { method: 'GET' }), + params: { sessionId: 'x' }, + } as never)) as Response; + expect(res.status).toBe(405); + }); +}); diff --git a/src/server/routes/sessions/delete.ts b/src/server/routes/sessions/delete.ts new file mode 100644 index 00000000..fe56dcad --- /dev/null +++ b/src/server/routes/sessions/delete.ts @@ -0,0 +1,83 @@ +/** + * @license + * SPDX-License-Identifier: Apache-2.0 + */ + +/** + * DELETE /api/sessions/:sessionId — remove a session and everything keyed to + * it. The mirror of `archiveSession`: archive is reversible and keeps the + * transcript, this one is the destructive path the user opts into from the + * sidebar's trash button. + * + * Order matters and is enforced here, not by the callers: + * 1. refuse a session whose omp child is mid-turn (a run, a live subagent, or + * a dialog the agent is blocked on) — the delete would kill work the user + * can still see, and omp would flush the file back on shutdown anyway; + * 2. destroy the child and AWAIT its exit — omp writes session state on + * shutdown and would otherwise recreate the transcript just removed; + * 3. purge side questions, then the transcript, artifacts and chamber rows. + * + * A pending `new-…` session is refused: it is a client-side placeholder with + * nothing stored anywhere yet, so deleting it is the same as navigating away. + */ + +import { json } from '@/server/lib/remix-compat'; +import type { ActionFunctionArgs } from '@/server/lib/remix-compat'; +import { methodNotAllowed } from '@/server/lib/route-adapter'; +import { getRpcSession } from '@/server/lib/omp/rpc/session-registry'; +import { purgeSessionData } from '@/server/lib/omp/session/delete.server'; +import { purgeBtwForSession } from '@/server/lib/btw/purge.server'; +import { isSinglePathSegment } from '@/server/lib/fs/path-segment'; +import { isMockMode } from '@/server/mock.server'; + +export async function deleteSession({ request, params }: ActionFunctionArgs) { + if (request.method !== 'DELETE') { + return methodNotAllowed({ request, params }); + } + + const sessionId = params.sessionId; + if (!sessionId) return json({ error: 'Missing session id' }, { status: 400 }); + if (sessionId.startsWith('new-')) { + return json({ error: 'This chat has not been created yet.', code: 'session_pending' }, { status: 400 }); + } + // The id names a directory under the sessions root and another under the btw + // root, so it must be exactly one path segment — see `isSinglePathSegment` + // for the `.` case that made this load-bearing. Both purges re-check it + // independently; refusing here is what turns the attempt into a 400. + if (!isSinglePathSegment(sessionId)) { + return json({ error: 'Invalid session id' }, { status: 400 }); + } + + // A live child is the only other writer of the transcript. Busy means a turn, + // a live subagent, or a dialog omp is parked on — all things the user can + // still act on, so the delete is refused with a reason rather than silently + // taking them down. + const live = getRpcSession(sessionId); + if (live?.isAlive()) { + if (live.isBusy()) { + return json( + { error: 'This session is working. Stop the run before deleting it.', code: 'session_busy' }, + { status: 409 }, + ); + } + await live.destroyAndWait(); + } + + // Side questions live outside the session file and are removed first: their + // children must be gone before the parent transcript they were snapshotted + // from disappears. + const btwTopicsRemoved = isMockMode() ? 0 : await purgeBtwForSession(sessionId); + const purged = await purgeSessionData(sessionId); + + if (!purged.found) { + return json({ error: 'Session not found' }, { status: 404 }); + } + + return json({ + success: true, + sessionId, + fileRemoved: purged.fileRemoved, + artifactsRemoved: purged.artifactsRemoved, + btwTopicsRemoved, + }); +} diff --git a/src/server/routes/sessions/index.ts b/src/server/routes/sessions/index.ts index 7bce8eef..3a870b33 100644 --- a/src/server/routes/sessions/index.ts +++ b/src/server/routes/sessions/index.ts @@ -10,10 +10,15 @@ import { renameSession, } from '@/server/routes/sessions/session'; import { listSubagents, readSubagentTranscript } from '@/server/routes/sessions/subagents'; +import { deleteSession } from '@/server/routes/sessions/delete'; export const sessionsBindings: HandlerBinding[] = [ ...bindingsFor(sessionsList, '/api/sessions/list'), ...bindingsFor(sessionsFolder, '/api/sessions/:sessionId'), + // DELETE-only on the same path the folder loader answers GET on: mounting it + // through `actionBindings` would register a 405 GET fallback that Elysia + // applies after the loader, replacing the workspace listing with it. + { method: 'DELETE', path: '/api/sessions/:sessionId', handler: deleteSession }, ...actionBindings(archiveSession, '/api/sessions/:sessionId/archive'), // Per-item queue routes. `actionBindings` mounts ALL four mutating verbs on // one path, and Elysia lets a later registration override an earlier one — diff --git a/src/shared/lib/workspace/session-state/store.ts b/src/shared/lib/workspace/session-state/store.ts index 4cf4860f..6ef1f283 100644 --- a/src/shared/lib/workspace/session-state/store.ts +++ b/src/shared/lib/workspace/session-state/store.ts @@ -247,6 +247,26 @@ export function getLastOpenedAt(sessionId: string): number | undefined { return lastTouched.get(sessionId); } +/** + * Drop a session's cached UI state after it was deleted. + * + * Deliberately does NOT persist: the session no longer exists, and the persist + * endpoint upserts, so flushing here would write the row straight back. The + * pending debounce timer is cancelled for the same reason — it would fire after + * the delete and resurrect the state. + */ +export function forgetSession(sessionId: string): void { + const timer = persistTimers.get(sessionId); + if (timer) { + clearTimeout(timer); + persistTimers.delete(sessionId); + } + cache.delete(sessionId); + readySessions.delete(sessionId); + dirtySessions.delete(sessionId); + lastTouched.delete(sessionId); +} + /** * Move a pending `new-…` session's state onto the real session id adopted * by a spawn, then discard the transient slot.