From 3a26563be25189ee1f6523c45e4528621aaf69e4 Mon Sep 17 00:00:00 2001 From: indifferentketchup Date: Sat, 30 May 2026 22:01:25 +0000 Subject: [PATCH] feat(coder): guard session delete against worktree work loss MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Deleting a BooChat session CASCADE-wipes its session_worktrees row, which would silently orphan uncommitted/unpushed/unmerged work in the worktree. Add a pre-DELETE gate: the server reads session_worktrees from the shared DB first (no row = chat-only session = delete immediately, zero round-trip), and for worktree-backed sessions calls a new BooCoder endpoint that runs git on the host (only the host systemd service can see /tmp/booworktrees). checkWorktreeWorkAtRisk reports dirty/unpushed/unmerged via the audited hostExec+shellEscape path; default branch is detected from refs/remotes/origin/HEAD (not the worktree's own branch), never hardcoded. Any at-risk worktree returns 409 with per-worktree RiskReport[]; force=true bypasses the check entirely. Fail-closed: coder unreachable/errored also blocks (force still escapes). The sidebar renders a block dialog distinguishing work-at-risk (Commit/Stash/Force) from couldn't-verify (Cancel/Force only); stash uses -u and re-blocks on remaining commits with an explanatory message. Commit never auto-commits — it routes the user to the session. Co-Authored-By: Claude Opus 4.8 (1M context) --- apps/coder/src/index.ts | 2 + apps/coder/src/routes/worktree-safety.ts | 45 ++++++ apps/coder/src/services/worktrees.ts | 153 +++++++++++++++++++ apps/server/src/routes/sessions.ts | 47 +++++- apps/server/src/types/api.ts | 14 ++ apps/web/src/api/client.ts | 13 +- apps/web/src/api/types.ts | 13 ++ apps/web/src/components/ProjectSidebar.tsx | 170 ++++++++++++++++++++- 8 files changed, 448 insertions(+), 9 deletions(-) create mode 100644 apps/coder/src/routes/worktree-safety.ts diff --git a/apps/coder/src/index.ts b/apps/coder/src/index.ts index 8436502..bbd4dd7 100644 --- a/apps/coder/src/index.ts +++ b/apps/coder/src/index.ts @@ -30,6 +30,7 @@ import { registerInboxRoutes } from './routes/inbox.js'; import { registerStatsRoutes } from './routes/stats.js'; import { registerArenaRoutes } from './routes/arena.js'; import { registerProviderRoutes } from './routes/providers.js'; +import { registerWorktreeSafetyRoutes } from './routes/worktree-safety.js'; import { registerWebSocket } from './routes/ws.js'; // Phase 4: dispatcher + agent probe import { createDispatcher } from './services/dispatcher.js'; @@ -195,6 +196,7 @@ async function main() { registerStatsRoutes(app, sql); registerArenaRoutes(app, sql); registerProviderRoutes(app, sql, config); + registerWorktreeSafetyRoutes(app, sql); registerWebSocket(app, sql, broker); // Serve static frontend (built web app). In production, the dist/ is diff --git a/apps/coder/src/routes/worktree-safety.ts b/apps/coder/src/routes/worktree-safety.ts new file mode 100644 index 0000000..830ead6 --- /dev/null +++ b/apps/coder/src/routes/worktree-safety.ts @@ -0,0 +1,45 @@ +/** + * Session-delete work-loss guard (coder side). + * + * Session delete itself lives in apps/server (Docker), which CANNOT see the + * host worktree dirs (/tmp/booworktrees) or run git on them. Only BooCoder + * (host systemd) can. So the server's DELETE route calls these endpoints + * pre-delete to learn whether a session's worktree holds work at risk, and to + * stash it. The server owns the gate; coder owns the git truth. + */ +import type { FastifyInstance } from 'fastify'; +import type { Sql } from '../db.js'; +import { checkWorktreeWorkAtRisk, stashWorktree } from '../services/worktrees.js'; + +export function registerWorktreeSafetyRoutes(app: FastifyInstance, sql: Sql): void { + // GET risk for a session's worktree(s). One row per session today (PK on + // session_id); the loop already handles the Phase-1.5 multi-worktree case. + app.get<{ Params: { sessionId: string } }>( + '/api/sessions/:sessionId/worktree-risk', + async (req) => { + const rows = await sql<{ worktree_path: string }[]>` + SELECT worktree_path FROM session_worktrees WHERE session_id = ${req.params.sessionId} + `; + const reports = []; + for (const row of rows) { + reports.push(await checkWorktreeWorkAtRisk(row.worktree_path)); + } + return { reports }; + }, + ); + + // Stash a session's worktree(s) — clears the dirty risk; recoverable. + app.post<{ Params: { sessionId: string } }>( + '/api/sessions/:sessionId/worktree-stash', + async (req) => { + const rows = await sql<{ worktree_path: string }[]>` + SELECT worktree_path FROM session_worktrees WHERE session_id = ${req.params.sessionId} + `; + const results = []; + for (const row of rows) { + results.push({ worktreePath: row.worktree_path, ...(await stashWorktree(row.worktree_path)) }); + } + return { results }; + }, + ); +} diff --git a/apps/coder/src/services/worktrees.ts b/apps/coder/src/services/worktrees.ts index f7635a2..820635a 100644 --- a/apps/coder/src/services/worktrees.ts +++ b/apps/coder/src/services/worktrees.ts @@ -182,6 +182,159 @@ export async function ensureSessionWorktree( }; } +// ─── Session-delete work-loss guard ───────────────────────────────────────── + +/** + * Risk report for a single worktree, returned by checkWorktreeWorkAtRisk. + * `atRisk` is the gate the server reads before allowing a session delete. + * A git error never silently passes — it forces `atRisk` true and surfaces + * the message in `error` (fail-closed). + */ +export interface RiskReport { + worktreePath: string; + branch: string; + dirty: boolean; // uncommitted working-tree changes (incl. untracked) + unpushed: number; // commits ahead of upstream, or -1 if no upstream is set + unmerged: number; // commits on this branch not in the project default branch + atRisk: boolean; // dirty || unpushed !== 0 || unmerged > 0 || git error + error?: string; // populated on a git failure; presence forces atRisk +} + +/** + * Resolve the project's default branch as a git-usable ref (e.g. "origin/main"). + * + * `refs/remotes/origin/HEAD` lives in the repo's COMMON git dir and is shared + * across every linked worktree, so reading it from the session worktree returns + * the REMOTE's default branch — never this worktree's own `session-` branch + * (that would be `symbolic-ref HEAD`, a different ref). Falls back to probing + * common defaults by verified existence when origin/HEAD isn't set (e.g. a repo + * that never ran `git remote set-head`). Returns null if none resolve, in which + * case the unmerged check is skipped (dirty + unpushed still protect the work). + */ +async function detectDefaultBranchRef( + worktreePath: string, + opts?: { signal?: AbortSignal }, +): Promise { + const head = await hostExec( + `git -C ${shellEscape(worktreePath)} symbolic-ref --short refs/remotes/origin/HEAD`, + { signal: opts?.signal, timeoutMs: 10_000 }, + ); + if (head.exitCode === 0) { + const ref = head.stdout.trim(); // e.g. "origin/main" + if (ref) { + const verify = await hostExec( + `git -C ${shellEscape(worktreePath)} rev-parse --verify --quiet ${shellEscape(ref + '^{commit}')}`, + { signal: opts?.signal, timeoutMs: 10_000 }, + ); + if (verify.exitCode === 0 && verify.stdout.trim()) return ref; + } + } + // origin/HEAD unset or unresolvable — probe common defaults. Prefer the + // remote-tracking ref (always resolvable in a fresh worktree) over the local + // head, which may not exist if the default branch lives only in the main tree. + for (const cand of ['origin/main', 'origin/master', 'main', 'master']) { + const verify = await hostExec( + `git -C ${shellEscape(worktreePath)} rev-parse --verify --quiet ${shellEscape(cand + '^{commit}')}`, + { signal: opts?.signal, timeoutMs: 10_000 }, + ); + if (verify.exitCode === 0 && verify.stdout.trim()) return cand; + } + return null; +} + +/** + * Inspect a worktree for work that would be lost if its session were deleted. + * Three checks, all via the audited hostExec + shellEscape path (every + * interpolated value — paths, refs — is single-quote-escaped; no bare + * interpolation). Any unexpected git failure is treated as at-risk, never a + * silent pass. + */ +export async function checkWorktreeWorkAtRisk( + worktreePath: string, + opts?: { signal?: AbortSignal }, +): Promise { + // Branch name — also doubles as the "is this still a git worktree?" probe. + const br = await hostExec( + `git -C ${shellEscape(worktreePath)} rev-parse --abbrev-ref HEAD`, + { signal: opts?.signal, timeoutMs: 10_000 }, + ); + if (br.exitCode !== 0) { + return { + worktreePath, + branch: '', + dirty: false, + unpushed: 0, + unmerged: 0, + atRisk: true, + error: `git rev-parse failed: ${br.stderr.trim() || 'not a git worktree'}`, + }; + } + const branch = br.stdout.trim(); + + // (a) Uncommitted (dirty working tree, including untracked files). + const st = await hostExec( + `git -C ${shellEscape(worktreePath)} status --porcelain`, + { signal: opts?.signal, timeoutMs: 15_000 }, + ); + if (st.exitCode !== 0) { + return { + worktreePath, + branch, + dirty: false, + unpushed: 0, + unmerged: 0, + atRisk: true, + error: `git status failed: ${st.stderr.trim()}`, + }; + } + const dirty = st.stdout.trim().length > 0; + + // (b) Unpushed commits. No upstream configured => work exists only locally; + // treat as unpushed-by-definition (-1) rather than an error. + const up = await hostExec( + `git -C ${shellEscape(worktreePath)} rev-list --count ${shellEscape('@{u}..HEAD')}`, + { signal: opts?.signal, timeoutMs: 15_000 }, + ); + const unpushed = up.exitCode === 0 ? (parseInt(up.stdout.trim() || '0', 10) || 0) : -1; + + // (c) Unmerged commits — on this branch but not in the project default branch. + const defaultRef = await detectDefaultBranchRef(worktreePath, opts); + let unmerged = 0; + if (defaultRef) { + const rl = await hostExec( + `git -C ${shellEscape(worktreePath)} rev-list --count ${shellEscape(defaultRef + '..HEAD')}`, + { signal: opts?.signal, timeoutMs: 15_000 }, + ); + if (rl.exitCode === 0) unmerged = parseInt(rl.stdout.trim() || '0', 10) || 0; + } + + const atRisk = dirty || unpushed !== 0 || unmerged > 0; + return { worktreePath, branch, dirty, unpushed, unmerged, atRisk }; +} + +/** + * Stash a worktree's uncommitted changes (including untracked, via -u) so the + * working tree is clean. Stash entries live in the repo's common git dir, so + * they survive worktree-dir removal — this is the recoverable, safe-by-default + * escape. Note it only clears the *dirty* risk; unpushed/unmerged commits + * remain on the branch, so a re-attempted delete may still block on those. + */ +export async function stashWorktree( + worktreePath: string, + opts?: { signal?: AbortSignal }, +): Promise<{ stashed: boolean; error?: string }> { + const r = await hostExec( + `git -C ${shellEscape(worktreePath)} stash push -u -m ${shellEscape('boocode: pre-delete stash')}`, + { signal: opts?.signal, timeoutMs: 30_000 }, + ); + if (r.exitCode !== 0) { + return { stashed: false, error: r.stderr.trim() || r.stdout.trim() }; + } + // "No local changes to save" => exit 0, nothing stashed — not an error. + const stashed = !/no local changes to save/i.test(r.stdout); + return { stashed }; +} + /** Minimal shell escape for paths (single-quote wrapping). */ function shellEscape(s: string): string { // Replace single quotes with escaped version, wrap in single quotes diff --git a/apps/server/src/routes/sessions.ts b/apps/server/src/routes/sessions.ts index 371d630..288cc28 100644 --- a/apps/server/src/routes/sessions.ts +++ b/apps/server/src/routes/sessions.ts @@ -3,7 +3,7 @@ import { z } from 'zod'; import type { Sql } from '../db.js'; import type { Config } from '../config.js'; import type { Broker } from '../services/broker.js'; -import type { Session } from '../types/api.js'; +import type { Session, WorktreeRiskReport } from '../types/api.js'; import { getSetting } from './settings.js'; const CreateBody = z.object({ @@ -426,10 +426,53 @@ export function registerSessionRoutes( } ); - app.delete<{ Params: { id: string } }>( + app.delete<{ Params: { id: string }; Querystring: { force?: string } }>( '/api/sessions/:id', async (req, reply) => { const id = req.params.id; + const force = req.query.force === 'true' || req.query.force === '1'; + + // Session-delete work-loss guard. CASCADE on session_worktrees means the + // DELETE below auto-wipes the worktree row, so the safety check MUST run + // BEFORE it (paths read while the row still exists, pre-CASCADE). + // + // Optimization: read session_worktrees from our own (shared) DB first. + // No row => chat-only session => nothing on disk => delete immediately, + // zero round-trip. Only worktree-backed sessions pay the host git check. + if (!force) { + const worktrees = await sql<{ worktree_path: string }[]>` + SELECT worktree_path FROM session_worktrees WHERE session_id = ${id} + `; + if (worktrees.length > 0) { + // Worktree dirs live on the host; only BooCoder can run git on them. + const origin = process.env.BOOCODER_URL ?? 'http://boocoder:3000'; + let reports: WorktreeRiskReport[]; + try { + const res = await fetch(`${origin}/api/sessions/${id}/worktree-risk`); + if (!res.ok) { + // Fail-closed: can't verify => don't risk silent loss. Force escapes. + reply.code(409); + return { + error: 'could not verify worktree safety (BooCoder check failed). Use force to delete anyway.', + reports: [] as WorktreeRiskReport[], + }; + } + reports = ((await res.json()) as { reports?: WorktreeRiskReport[] }).reports ?? []; + } catch { + // Fail-closed: BooCoder unreachable. Force bypasses this path entirely. + reply.code(409); + return { + error: 'BooCoder unreachable; cannot verify worktree safety. Use force to delete anyway.', + reports: [] as WorktreeRiskReport[], + }; + } + if (reports.some((r) => r.atRisk)) { + reply.code(409); + return { error: 'This session has work at risk in its worktree.', reports }; + } + } + } + const deleted = await sql<{ project_id: string }[]>` DELETE FROM sessions WHERE id = ${id} RETURNING project_id `; diff --git a/apps/server/src/types/api.ts b/apps/server/src/types/api.ts index b41596c..257dc2b 100644 --- a/apps/server/src/types/api.ts +++ b/apps/server/src/types/api.ts @@ -25,6 +25,20 @@ export interface AvailableProject { export type SessionStatus = 'open' | 'archived'; +// Session-delete work-loss guard. Returned (as `reports`) in the 409 body when +// a delete is blocked because the session's worktree holds work at risk. The +// shape is produced by BooCoder's checkWorktreeWorkAtRisk and passed through +// verbatim; mirrored byte-for-byte in apps/web/src/api/types.ts for the dialog. +export interface WorktreeRiskReport { + worktreePath: string; + branch: string; + dirty: boolean; + unpushed: number; // commits ahead of upstream, or -1 if no upstream + unmerged: number; // commits not in the project default branch + atRisk: boolean; + error?: string; +} + export interface Session { id: string; project_id: string; diff --git a/apps/web/src/api/client.ts b/apps/web/src/api/client.ts index 4c1d989..758c5d4 100644 --- a/apps/web/src/api/client.ts +++ b/apps/web/src/api/client.ts @@ -151,8 +151,17 @@ export const api = { method: 'PATCH', body: JSON.stringify(body), }), - remove: (id: string) => - request(`/api/sessions/${id}`, { method: 'DELETE' }), + // force=true bypasses the server-side worktree work-loss guard. A blocked + // delete throws ApiError(409) whose body carries { error, reports }. + remove: (id: string, force = false) => + request(`/api/sessions/${id}${force ? '?force=true' : ''}`, { method: 'DELETE' }), + // Stash the session's worktree (uncommitted changes) on the host, via the + // BooCoder proxy. Recoverable escape from the work-at-risk dialog. + worktreeStash: (id: string) => + request<{ results: { worktreePath: string; stashed: boolean; error?: string }[] }>( + `/api/coder/sessions/${id}/worktree-stash`, + { method: 'POST' }, + ), archive: (id: string) => request(`/api/sessions/${id}/archive`, { method: 'POST' }), unarchive: (id: string) => diff --git a/apps/web/src/api/types.ts b/apps/web/src/api/types.ts index 04e6f6c..45c3b3f 100644 --- a/apps/web/src/api/types.ts +++ b/apps/web/src/api/types.ts @@ -34,6 +34,19 @@ export interface AvailableProject { export type SessionStatus = 'open' | 'archived'; +// Session-delete work-loss guard. Mirror of WorktreeRiskReport in +// apps/server/src/types/api.ts — edit both copies together. Arrives as the +// `reports` field of the 409 body when a delete is blocked. +export interface WorktreeRiskReport { + worktreePath: string; + branch: string; + dirty: boolean; + unpushed: number; // commits ahead of upstream, or -1 if no upstream + unmerged: number; // commits not in the project default branch + atRisk: boolean; + error?: string; +} + export interface Session { id: string; project_id: string; diff --git a/apps/web/src/components/ProjectSidebar.tsx b/apps/web/src/components/ProjectSidebar.tsx index 1161f90..5b47c91 100644 --- a/apps/web/src/components/ProjectSidebar.tsx +++ b/apps/web/src/components/ProjectSidebar.tsx @@ -19,12 +19,12 @@ import { DialogDescription, } from '@/components/ui/dialog'; import { AddProjectModal } from './AddProjectModal'; -import { api } from '@/api/client'; +import { api, ApiError } from '@/api/client'; import { useSidebar } from '@/hooks/useSidebar'; import { useSidebarDrawer } from '@/hooks/useSidebarDrawer'; import { useViewport } from '@/hooks/useViewport'; import { usePullToRefresh } from '@/hooks/usePullToRefresh'; -import type { SidebarProject } from '@/api/types'; +import type { SidebarProject, WorktreeRiskReport } from '@/api/types'; import { giteaUrlFor } from '@/lib/projectUrls'; import { isCoderSessionName } from '@/lib/coder-session'; import { cn } from '@/lib/utils'; @@ -110,6 +110,16 @@ export function ProjectSidebar() { const [renamingProject, setRenamingProject] = useState(null); const [renameProjectValue, setRenameProjectValue] = useState(''); const [archiveProjectConfirm, setArchiveProjectConfirm] = useState<{ id: string; name: string } | null>(null); + // Work-at-risk dialog: shown when a delete is blocked (409) because the + // session's worktree holds uncommitted/unpushed/unmerged work. + const [riskState, setRiskState] = useState<{ + sessionId: string; + projectId: string; + name: string; + message: string; + reports: WorktreeRiskReport[]; + } | null>(null); + const [riskBusy, setRiskBusy] = useState(false); const navigate = useNavigate(); const location = useLocation(); const lastToastedError = useRef(null); @@ -174,16 +184,81 @@ export function ProjectSidebar() { } } - async function handleDeleteSession(sessionId: string, projectId: string) { + async function handleDeleteSession( + sessionId: string, + projectId: string, + name: string, + force = false, + ) { try { - await api.sessions.remove(sessionId); + await api.sessions.remove(sessionId, force); // Server publishes session_deleted via WS; useUserEvents delivers it. + setRiskState(null); if (activeSession === sessionId) navigate(`/project/${projectId}`); } catch (err) { + // 409 => the server's work-loss guard blocked the delete. Open the + // work-at-risk dialog with the per-worktree reports instead of toasting. + if ( + err instanceof ApiError && + err.status === 409 && + err.body && typeof err.body === 'object' && 'reports' in err.body + ) { + const body = err.body as { error?: string; reports?: WorktreeRiskReport[] }; + setRiskState({ + sessionId, + projectId, + name, + message: body.error ?? 'This session has work at risk.', + reports: body.reports ?? [], + }); + return; + } toast.error(err instanceof Error ? err.message : 'failed to delete session'); } } + // Stash the worktree's uncommitted changes (recoverable), then re-attempt the + // delete. If unpushed/unmerged commits remain, the retry 409s again and the + // dialog re-renders with the narrowed risk. + async function handleStashAndRetry() { + if (!riskState || riskBusy) return; + setRiskBusy(true); + try { + const { results } = await api.sessions.worktreeStash(riskState.sessionId); + const failed = results.find((r) => r.error); + if (failed) { + toast.error(`stash failed: ${failed.error}`); + return; + } + await handleDeleteSession(riskState.sessionId, riskState.projectId, riskState.name, false); + } catch (err) { + toast.error(err instanceof Error ? err.message : 'stash failed'); + } finally { + setRiskBusy(false); + } + } + + // Explicit, destructive override — deletes despite work at risk. + async function handleForceDelete() { + if (!riskState || riskBusy) return; + setRiskBusy(true); + try { + await handleDeleteSession(riskState.sessionId, riskState.projectId, riskState.name, true); + } finally { + setRiskBusy(false); + } + } + + // Route the user to commit it themselves — never auto-commit. Opens the + // session workspace where they can use a terminal or agent pane. + function handleGoCommit() { + if (!riskState) return; + const sessionId = riskState.sessionId; + setRiskState(null); + navigate(`/session/${sessionId}`); + toast.info('Open a terminal or agent in this session, commit and push your work, then delete again.'); + } + async function handleRenameSession(sessionId: string) { const trimmed = renameValue.trim(); setRenamingSession(null); @@ -216,6 +291,20 @@ export function ProjectSidebar() { ) : 'w-60 shrink-0 border-r bg-sidebar text-sidebar-foreground flex flex-col h-screen'; + // Work-at-risk dialog framing. The server returns 409 in two distinct + // situations: (1) work genuinely at risk (reports has ≥1 atRisk entry), or + // (2) it couldn't verify (BooCoder down/errored → reports is empty). These + // are different user stories — "your work is in danger" vs "the checker is + // offline" — so the dialog must not show one generic message for both. + const atRiskReports = riskState?.reports.filter((r) => r.atRisk) ?? []; + const verifyFailed = riskState !== null && atRiskReports.length === 0; + const anyDirty = atRiskReports.some((r) => r.dirty); + // Commit-based risk (unpushed/unmerged) that stash can NOT clear. When this is + // all that remains (e.g. after a stash cleared the dirty changes), the dialog + // explains why it re-blocked and hides the Stash button so it doesn't look + // like stash "didn't work". + const anyCommits = atRiskReports.some((r) => r.unpushed !== 0 || r.unmerged > 0); + return ( ); }