diff --git a/controlplane/src/dashboards/handler.ts b/controlplane/src/dashboards/handler.ts index 5c98d964..2e2c3af5 100644 --- a/controlplane/src/dashboards/handler.ts +++ b/controlplane/src/dashboards/handler.ts @@ -5,13 +5,14 @@ * Dashboard API Handlers */ -// REVISION: dashboards-v6-link-sync-with-rbac +// REVISION: dashboards-v8-teardown-without-snapshot import type { Env, Dashboard, DashboardItem, DashboardEdge } from '../types'; import { syncItemToLinked, syncEdgeToLinked } from '../links/handler'; import { populateFromTemplate } from '../templates/handler'; import type { EnvWithDriveCache } from '../storage/drive-cache'; import { FlyMachinesClient } from '../sandbox/fly-machines'; +import { SandboxClient } from '../sandbox/client'; import { preProvisionDashboardSandbox } from '../sessions/handler'; function generateId(): string { @@ -282,21 +283,38 @@ export async function deleteDashbоard( return Response.json({ error: 'E79304: Not found or not owner' }, { status: 404 }); } - // Stop any active sessions first so their PTYs are killed in the sandbox. - // CASCADE-deleting the session rows would otherwise orphan live processes - // (shells, and agent children like node/chromium) in the VM. Mirrors deleteItem. + // Mark active sessions stopped. Deliberately NOT via stopSession(). + // + // stopSession is the "user closed a terminal, keep the dashboard usable" path: + // when it stops the last session it captures a workspace snapshot into R2 for + // recovery, and deletes that session's PTY. Both are wrong when the dashboard + // itself is going away — the snapshot is written moments before its dashboard + // ceases to exist and nothing ever collects it, so every deletion leaked an + // R2 object. + // + // It was also called in a sequential loop, one round trip per terminal, each + // able to block for the full 15s sandbox timeout. That made deletion slowest + // in exactly the case where it matters most — a wedged sandbox, which is the + // state a user is trying to clean up. The single bounded deleteSession below + // replaces all of it: destroying the sandbox session tears down every PTY it + // owns, so the per-PTY deletes were redundant anyway. try { - const activeSessions = await env.DB.prepare(` - SELECT id FROM sessions WHERE dashboard_id = ? AND status IN ('creating', 'active') - `).bind(dashboardId).all<{ id: string }>(); - if (activeSessions.results.length > 0) { - const { stоpSessiоn } = await import('../sessions/handler'); - for (const session of activeSessions.results) { - await stоpSessiоn(env as EnvWithDriveCache, session.id, userId); - } - } + const now = new Date().toISOString(); + await env.DB.prepare(` + UPDATE sessions SET status = 'stopped', stopped_at = ? + WHERE dashboard_id = ? AND status IN ('creating', 'active') + `).bind(now, dashboardId).run(); } catch { - // Best-effort — don't block dashboard deletion if session cleanup fails. + // Best-effort — don't block dashboard deletion if the status update fails. + } + + // Drop the dashboard's R2 objects (workspace snapshot, mirror manifests). + // Nothing cascades these; without this they outlive the dashboard forever. + try { + const { purgeDashbоardStorage } = await import('../sessions/handler'); + await purgeDashbоardStorage(env as EnvWithDriveCache, dashboardId); + } catch (e) { + console.error(`[deleteDashboard] Storage purge failed for ${dashboardId}: ${e}`); } // Delete dependent records that don't have ON DELETE CASCADE @@ -332,12 +350,55 @@ export async function deleteDashbоard( .bind(dashboardId) .run(); + // Read the sandbox row once: both the session release below and the Fly + // teardown after it need it. + const sandboxRow = await env.DB.prepare(` + SELECT sandbox_session_id, sandbox_machine_id, fly_volume_id FROM dashboard_sandboxes WHERE dashboard_id = ? + `).bind(dashboardId).first<{ + sandbox_session_id: string; + sandbox_machine_id: string; + fly_volume_id: string; + }>(); + + // Release the sandbox session — UNCONDITIONALLY, not just on Fly. + // + // A sandbox session owns real resources inside the VM: its PTYs and a full + // Chromium/Xvfb/x11vnc stack, several hundred MB each. Session.Close() is + // reachable only via DELETE /sessions/:id, and this was previously called + // only from the Fly-gated block below plus a create-race dedup. On any + // deployment sharing one sandbox (local dev, desktop, self-hosted), deleting + // a dashboard therefore freed its D1 rows and nothing else, so every + // dashboard ever created leaked a browser stack until the VM exhausted its + // memory — at which point the Go server was starved badly enough that even + // GET /health timed out, and every subsequent session create failed with + // "fetch error: The operation was aborted". + // + // Destroying the Fly machine (below) implicitly reclaims everything, so this + // is redundant there and merely tidy; it is load-bearing everywhere else. + // + // Best-effort: an unreachable or already-wedged sandbox must not block + // deleting the dashboard, or the user cannot clean up the very state that is + // wedging it. + if (sandboxRow?.sandbox_session_id && env.SANDBOX_URL) { + try { + const sandbox = new SandboxClient(env.SANDBOX_URL, env.SANDBOX_INTERNAL_TOKEN); + await sandbox.deleteSession( + sandboxRow.sandbox_session_id, + sandboxRow.sandbox_machine_id || undefined + ); + console.log( + `[deleteDashboard] Released sandbox session ${sandboxRow.sandbox_session_id} for dashboard ${dashboardId}` + ); + } catch (e) { + console.error( + `[deleteDashboard] Failed to release sandbox session ${sandboxRow.sandbox_session_id}: ${e}` + ); + } + } + // Destroy Fly machine and volume for this dashboard (best-effort) if (env.FLY_API_TOKEN && env.FLY_APP_NAME) { try { - const sandboxRow = await env.DB.prepare(` - SELECT sandbox_machine_id, fly_volume_id FROM dashboard_sandboxes WHERE dashboard_id = ? - `).bind(dashboardId).first<{ sandbox_machine_id: string; fly_volume_id: string }>(); if (sandboxRow?.sandbox_machine_id) { const fly = new FlyMachinesClient(env.FLY_APP_NAME, env.FLY_API_TOKEN); try { diff --git a/controlplane/src/index.ts b/controlplane/src/index.ts index fb58f08c..f49c9f4d 100644 --- a/controlplane/src/index.ts +++ b/controlplane/src/index.ts @@ -1180,6 +1180,38 @@ async function handleRequest(request: Request, env: EnvWithBindings, ctx: Pick ({})) as { apply?: boolean; limit?: number }; + const { sweepOrphanedStorage } = await import('./storage/orphan-sweep'); + const result = await sweepOrphanedStorage(ensureDriveCache(env), { + apply: body.apply === true, + limit: typeof body.limit === 'number' ? body.limit : undefined, + }); + return Response.json(result); + } + // POST /auth/logout - clear session cookie if (segments[0] === 'auth' && segments[1] === 'logout' && segments.length === 2 && method === 'POST') { return authLogout.logout(request, env); diff --git a/controlplane/src/sessions/handler.ts b/controlplane/src/sessions/handler.ts index e354b9a1..992f3fe1 100644 --- a/controlplane/src/sessions/handler.ts +++ b/controlplane/src/sessions/handler.ts @@ -926,6 +926,108 @@ function workspaceSnapshotKey(dashboardId: string): string { return `workspace/${dashboardId}/snapshot.json`; } +/** Mirror providers with a known key namespace; see discoverMirrorProviders. */ +const MIRROR_PROVIDERS = ['github', 'box', 'onedrive'] as const; + +/** Bounds the delete loop at 1M objects rather than spinning forever. */ +const MAX_PURGE_ROUNDS = 1000; + +/** + * Delete every object under an R2 prefix. + * + * list() caps at 1000 keys, so anything with more cached files than that needs + * repeating — otherwise the tail is silently left behind, the same class of bug + * as deleting only manifests. + * + * Deliberately re-lists from the start each round instead of paging with a + * cursor: we delete the very keys we just listed, and a cursor that encodes a + * position rather than a key skips survivors once earlier keys disappear. (An + * earlier cursor version of this passed by hand and failed the 2500-object + * test.) Every round removes everything it saw, so this always terminates. + */ +async function purgeR2Prefix(bucket: R2Bucket, prefix: string): Promise { + let deleted = 0; + + for (let round = 0; round < MAX_PURGE_ROUNDS; round++) { + const listed = await bucket.list({ prefix, limit: 1000 }); + const keys = listed.objects.map((object) => object.key); + if (keys.length === 0) { + return deleted; + } + await bucket.delete(keys); + deleted += keys.length; + } + + console.error( + `[purgeDashboardStorage] Hit the ${MAX_PURGE_ROUNDS}-round cap for ${prefix}; objects may remain` + ); + return deleted; +} + +/** + * Mirror keys are namespaced by provider, so purging a dashboard needs the set + * of providers. Discovered from the bucket rather than trusted from a constant: + * a provider added later would otherwise keep leaking files with nothing to + * signal it. Falls back to (and always includes) the known list, so this still + * works if delimited listing returns nothing. + */ +async function discоverMirrorPrоviders(bucket: R2Bucket): Promise { + const providers = new Set(MIRROR_PROVIDERS); + try { + const listed = await bucket.list({ prefix: 'mirror/', delimiter: '/' }); + for (const prefix of listed.delimitedPrefixes ?? []) { + // "mirror//" -> "" + const provider = prefix.slice('mirror/'.length).replace(/\/$/, ''); + if (provider) providers.add(provider); + } + } catch { + // Delimited listing unsupported or failed — the known list still applies. + } + return [...providers]; +} + +/** + * Delete every R2 object belonging to a dashboard. + * + * These live outside D1, so nothing cascades them: deleting a dashboard used to + * leave its cached content in the bucket forever. + * + * Purges by PREFIX rather than by known key. Each family stores per-file objects + * alongside its manifest — + * workspace//snapshot.json + * drive//manifest.json + drive//files/ + * mirror///manifest.json + mirror///files/ + * — so deleting just the manifests orphaned every uploaded and mirrored file, + * indefinitely, and left the dashboard's content unreferenced but billable. + * Sweeping the prefix also means a new key shape under it is covered + * automatically, instead of quietly leaking until someone notices. + * + * Callers: the dev clear-workspace endpoint, and deleteDashboard. + */ +export async function purgeDashbоardStorage( + env: EnvWithDriveCache, + dashboardId: string +): Promise { + const bucket = env.DRIVE_CACHE; + const prefixes = [`workspace/${dashboardId}/`, `drive/${dashboardId}/`]; + for (const provider of await discоverMirrorPrоviders(bucket)) { + prefixes.push(`mirror/${provider}/${dashboardId}/`); + } + + let total = 0; + for (const prefix of prefixes) { + try { + total += await purgeR2Prefix(bucket, prefix); + } catch (e) { + // Best-effort per prefix: one failure must not strand the others. + console.error(`[purgeDashboardStorage] Failed to purge ${prefix}: ${e}`); + } + } + if (total > 0) { + console.log(`[purgeDashboardStorage] Deleted ${total} object(s) for dashboard ${dashboardId}`); + } +} + export async function clearWorkspaceDev( request: Request, env: EnvWithDriveCache, @@ -952,11 +1054,7 @@ export async function clearWorkspaceDev( return Response.json({ error: 'E79792: Not found or no access' }, { status: 404 }); } - await env.DRIVE_CACHE.delete(workspaceSnapshotKey(data.dashboardId)); - await env.DRIVE_CACHE.delete(driveManifestKey(data.dashboardId)); - for (const provider of ['github', 'box', 'onedrive']) { - await env.DRIVE_CACHE.delete(mirrorManifestKey(provider, data.dashboardId)); - } + await purgeDashbоardStorage(env, data.dashboardId); const now = new Date().toISOString(); await env.DB.prepare(` diff --git a/controlplane/src/sessions/purge-storage.test.ts b/controlplane/src/sessions/purge-storage.test.ts new file mode 100644 index 00000000..113ca5c6 --- /dev/null +++ b/controlplane/src/sessions/purge-storage.test.ts @@ -0,0 +1,135 @@ +import { describe, it, expect } from 'vitest'; +import { purgeDashbоardStorage } from './handler'; +import type { EnvWithDriveCache } from '../storage/drive-cache'; + +/** + * Minimal in-memory R2 stand-in. + * + * Implements just what purgeDashboardStorage touches: prefix listing with a + * 1000-key page cap and a cursor, delimited listing, and bulk delete. The page + * cap is the point — the real bucket truncates at 1000, so a dashboard with + * more cached files than that is exactly where a missing cursor loop silently + * strands objects. + */ +function makeBucket(keys: string[]) { + const store = new Set(keys); + const deleteCalls: number[] = []; + + const bucket = { + async list(options?: { + prefix?: string; + cursor?: string; + limit?: number; + delimiter?: string; + }) { + const prefix = options?.prefix ?? ''; + const all = [...store].filter((k) => k.startsWith(prefix)).sort(); + + if (options?.delimiter) { + const prefixes = new Set(); + for (const key of all) { + const rest = key.slice(prefix.length); + const idx = rest.indexOf(options.delimiter); + if (idx >= 0) prefixes.add(prefix + rest.slice(0, idx + 1)); + } + return { objects: [], truncated: false, delimitedPrefixes: [...prefixes] }; + } + + const limit = options?.limit ?? 1000; + const start = options?.cursor ? Number(options.cursor) : 0; + const page = all.slice(start, start + limit); + const next = start + page.length; + const truncated = next < all.length; + return { + objects: page.map((key) => ({ key })), + truncated, + ...(truncated ? { cursor: String(next) } : {}), + delimitedPrefixes: [], + }; + }, + async delete(keys: string | string[]) { + const list = Array.isArray(keys) ? keys : [keys]; + deleteCalls.push(list.length); + for (const key of list) store.delete(key); + }, + }; + + return { bucket, store, deleteCalls }; +} + +const envFor = (bucket: unknown) => ({ DRIVE_CACHE: bucket }) as unknown as EnvWithDriveCache; + +describe('purgeDashboardStorage', () => { + const DASH = 'dash-1'; + + it('deletes per-file objects, not just manifests', async () => { + const { bucket, store } = makeBucket([ + `workspace/${DASH}/snapshot.json`, + `drive/${DASH}/manifest.json`, + `drive/${DASH}/files/file-a`, + `drive/${DASH}/files/file-b`, + `mirror/github/${DASH}/manifest.json`, + `mirror/github/${DASH}/files/repo-1`, + `mirror/box/${DASH}/files/box-1`, + ]); + + await purgeDashbоardStorage(envFor(bucket), DASH); + + expect([...store]).toEqual([]); + }); + + it('leaves other dashboards untouched', async () => { + const other = [ + `workspace/dash-2/snapshot.json`, + `drive/dash-2/files/file-a`, + `mirror/github/dash-2/files/repo-1`, + ]; + const { bucket, store } = makeBucket([ + `drive/${DASH}/files/file-a`, + `mirror/github/${DASH}/files/repo-1`, + ...other, + ]); + + await purgeDashbоardStorage(envFor(bucket), DASH); + + expect([...store].sort()).toEqual([...other].sort()); + }); + + it('pages past the 1000-key list cap', async () => { + const many = Array.from({ length: 2500 }, (_, i) => `drive/${DASH}/files/file-${i}`); + const { bucket, store } = makeBucket(many); + + await purgeDashbоardStorage(envFor(bucket), DASH); + + expect([...store]).toEqual([]); + }); + + it('purges a mirror provider discovered from the bucket', async () => { + // Not in the hardcoded provider list — must still be swept. + const { bucket, store } = makeBucket([ + `mirror/dropbox/${DASH}/manifest.json`, + `mirror/dropbox/${DASH}/files/f-1`, + ]); + + await purgeDashbоardStorage(envFor(bucket), DASH); + + expect([...store]).toEqual([]); + }); + + it('continues purging other prefixes when one fails', async () => { + const { bucket, store } = makeBucket([ + `workspace/${DASH}/snapshot.json`, + `drive/${DASH}/files/file-a`, + ]); + const realList = bucket.list.bind(bucket); + bucket.list = async (options?: Parameters[0]) => { + if (options?.prefix === `workspace/${DASH}/`) throw new Error('R2 unavailable'); + return realList(options); + }; + + await purgeDashbоardStorage(envFor(bucket), DASH); + + // The failing prefix survives; the healthy one is still cleaned up. + expect([...store]).toEqual([`workspace/${DASH}/snapshot.json`]); + }); +}); diff --git a/controlplane/src/storage/orphan-sweep.test.ts b/controlplane/src/storage/orphan-sweep.test.ts new file mode 100644 index 00000000..14586a22 --- /dev/null +++ b/controlplane/src/storage/orphan-sweep.test.ts @@ -0,0 +1,155 @@ +import { describe, it, expect } from 'vitest'; +import { sweepOrphanedStorage } from './orphan-sweep'; +import type { EnvWithDriveCache } from './drive-cache'; + +/** In-memory R2 with delimited listing, paging, sizes and bulk delete. */ +function makeBucket(entries: Record) { + const store = new Map(Object.entries(entries)); + + return { + store, + bucket: { + async list(options?: { + prefix?: string; + cursor?: string; + limit?: number; + delimiter?: string; + }) { + const prefix = options?.prefix ?? ''; + const all = [...store.keys()].filter((k) => k.startsWith(prefix)).sort(); + + if (options?.delimiter) { + const prefixes = new Set(); + for (const key of all) { + const rest = key.slice(prefix.length); + const idx = rest.indexOf(options.delimiter); + if (idx >= 0) prefixes.add(prefix + rest.slice(0, idx + 1)); + } + return { objects: [], truncated: false, delimitedPrefixes: [...prefixes] }; + } + + const limit = options?.limit ?? 1000; + const start = options?.cursor ? Number(options.cursor) : 0; + const page = all.slice(start, start + limit); + const next = start + page.length; + const truncated = next < all.length; + return { + objects: page.map((key) => ({ key, size: store.get(key) ?? 0 })), + truncated, + ...(truncated ? { cursor: String(next) } : {}), + delimitedPrefixes: [], + }; + }, + async delete(keys: string | string[]) { + for (const key of Array.isArray(keys) ? keys : [keys]) store.delete(key); + }, + }, + }; +} + +/** D1 stand-in answering only the `SELECT id FROM dashboards WHERE id IN (...)`. */ +function makeDb(liveIds: string[]) { + return { + prepare(_sql: string) { + let bound: string[] = []; + const stmt = { + bind(...args: string[]) { + bound = args; + return stmt; + }, + async all() { + return { + results: bound + .filter((id) => liveIds.includes(id)) + .map((id) => ({ id }) as unknown as T), + }; + }, + }; + return stmt; + }, + }; +} + +const envFor = (bucket: unknown, liveIds: string[]) => + ({ DRIVE_CACHE: bucket, DB: makeDb(liveIds) }) as unknown as EnvWithDriveCache; + +describe('sweepOrphanedStorage', () => { + const bucketContents = { + // live dashboard + 'workspace/live-1/snapshot.json': 10, + 'drive/live-1/files/a': 100, + // deleted dashboard + 'workspace/dead-1/snapshot.json': 20, + 'drive/dead-1/manifest.json': 5, + 'drive/dead-1/files/b': 200, + 'mirror/github/dead-1/files/c': 300, + // deleted dashboard, only mirrored content + 'mirror/dropbox/dead-2/files/d': 400, + }; + + it('reports orphans without deleting on a dry run', async () => { + const { bucket, store } = makeBucket(bucketContents); + const before = store.size; + + const result = await sweepOrphanedStorage(envFor(bucket, ['live-1'])); + + expect(result.dryRun).toBe(true); + expect(result.orphans.sort()).toEqual(['dead-1', 'dead-2']); + expect(result.objects).toBe(5); + expect(result.bytes).toBe(20 + 5 + 200 + 300 + 400); + // Nothing removed. + expect(store.size).toBe(before); + }); + + it('deletes only orphaned dashboards when applied', async () => { + const { bucket, store } = makeBucket(bucketContents); + + const result = await sweepOrphanedStorage(envFor(bucket, ['live-1']), { apply: true }); + + expect(result.dryRun).toBe(false); + expect([...store.keys()].sort()).toEqual([ + 'drive/live-1/files/a', + 'workspace/live-1/snapshot.json', + ]); + }); + + it('honours limit and flags truncation', async () => { + const { bucket, store } = makeBucket(bucketContents); + + const result = await sweepOrphanedStorage(envFor(bucket, ['live-1']), { + apply: true, + limit: 1, + }); + + expect(result.orphans).toHaveLength(1); + expect(result.truncated).toBe(true); + // The unprocessed orphan's objects survive this batch. + expect([...store.keys()].some((k) => k.includes(result.orphans[0]))).toBe(false); + expect(store.size).toBeGreaterThan(2); + }); + + it('does nothing when every dashboard is live', async () => { + const { bucket, store } = makeBucket(bucketContents); + const before = store.size; + + const result = await sweepOrphanedStorage( + envFor(bucket, ['live-1', 'dead-1', 'dead-2']), + { apply: true } + ); + + expect(result.orphans).toEqual([]); + expect(result.objects).toBe(0); + expect(store.size).toBe(before); + }); + + it('pages discovery past the 1000-object list cap', async () => { + const many: Record = {}; + for (let i = 0; i < 2500; i++) many[`drive/dead-1/files/f-${i}`] = 1; + const { bucket, store } = makeBucket(many); + + const result = await sweepOrphanedStorage(envFor(bucket, []), { apply: true }); + + expect(result.objects).toBe(2500); + expect(store.size).toBe(0); + }); +}); diff --git a/controlplane/src/storage/orphan-sweep.ts b/controlplane/src/storage/orphan-sweep.ts new file mode 100644 index 00000000..e16a052e --- /dev/null +++ b/controlplane/src/storage/orphan-sweep.ts @@ -0,0 +1,197 @@ +// Copyright 2026 Rob Macrae. All rights reserved. +// SPDX-License-Identifier: LicenseRef-Proprietary + +// REVISION: orphan-sweep-v1-initial + +/** + * One-off sweep for R2 objects whose dashboard no longer exists. + * + * Until purgeDashboardStorage swept by prefix, deleting a dashboard removed its + * manifests and left every uploaded and mirrored file behind — unreferenced but + * still billable. This reclaims what accumulated before that fix. New deletions + * are handled inline, so this is a backfill, not an ongoing job. + * + * DRY RUN BY DEFAULT. Deleting requires an explicit apply flag: the input is a + * whole R2 bucket and a mistake here is unrecoverable, so the safe mode is the + * one you get by accident. + */ + +import type { Env } from '../types'; +import type { EnvWithDriveCache } from './drive-cache'; +import { purgeDashbоardStorage } from '../sessions/handler'; + +/** SQLite caps bound parameters (~999); stay well under when checking ids. */ +const ID_CHUNK = 400; + +/** Bounds discovery listing rather than spinning on a pathological bucket. */ +const MAX_LIST_ROUNDS = 1000; + +export interface OrphanSweepResult { + revision: string; + dryRun: boolean; + /** Dashboard ids found in R2. */ + candidates: number; + /** Candidates with no surviving row in `dashboards`. */ + orphans: string[]; + /** Objects counted (dry run) or deleted (apply). */ + objects: number; + /** Bytes counted (dry run) or reclaimed (apply). */ + bytes: number; + /** True when the orphan list was capped by `limit`. */ + truncated: boolean; +} + +const SWEEP_REVISION = 'orphan-sweep-v1-initial'; + +/** + * List the immediate child "directories" of a prefix. + * + * Uses delimited listing so discovery costs one page per prefix level instead of + * enumerating every object in the bucket. Pages via cursor, which is safe here + * because discovery deletes nothing (unlike purgeR2Prefix, where deleting the + * listed keys invalidates a positional cursor). + */ +async function listChildPrefixes(bucket: R2Bucket, prefix: string): Promise { + const found = new Set(); + let cursor: string | undefined; + + for (let round = 0; round < MAX_LIST_ROUNDS; round++) { + const listed = await bucket.list({ prefix, delimiter: '/', cursor, limit: 1000 }); + for (const child of listed.delimitedPrefixes ?? []) { + const name = child.slice(prefix.length).replace(/\/$/, ''); + if (name) found.add(name); + } + if (!listed.truncated) break; + cursor = listed.cursor; + if (!cursor) break; + } + + return [...found]; +} + +/** Total objects and bytes under a prefix, without deleting. */ +async function measurePrefix( + bucket: R2Bucket, + prefix: string +): Promise<{ objects: number; bytes: number }> { + let objects = 0; + let bytes = 0; + let cursor: string | undefined; + + for (let round = 0; round < MAX_LIST_ROUNDS; round++) { + const listed = await bucket.list({ prefix, cursor, limit: 1000 }); + for (const object of listed.objects) { + objects += 1; + bytes += object.size ?? 0; + } + if (!listed.truncated) break; + cursor = listed.cursor; + if (!cursor) break; + } + + return { objects, bytes }; +} + +/** Every dashboard id referenced anywhere in the bucket. */ +async function discoverDashbоardIds(bucket: R2Bucket): Promise> { + const ids = new Set(); + + for (const prefix of ['workspace/', 'drive/']) { + for (const id of await listChildPrefixes(bucket, prefix)) { + ids.add(id); + } + } + + // mirror/// — one level deeper. + for (const provider of await listChildPrefixes(bucket, 'mirror/')) { + for (const id of await listChildPrefixes(bucket, `mirror/${provider}/`)) { + ids.add(id); + } + } + + return ids; +} + +/** Subset of `ids` that still have a row in `dashboards`. */ +async function findLiveDashbоardIds(env: Env, ids: string[]): Promise> { + const live = new Set(); + + for (let i = 0; i < ids.length; i += ID_CHUNK) { + const chunk = ids.slice(i, i + ID_CHUNK); + const placeholders = chunk.map(() => '?').join(','); + const rows = await env.DB.prepare( + `SELECT id FROM dashboards WHERE id IN (${placeholders})` + ) + .bind(...chunk) + .all<{ id: string }>(); + for (const row of rows.results) { + live.add(row.id); + } + } + + return live; +} + +/** + * Find (and optionally delete) R2 objects belonging to deleted dashboards. + * + * `apply` defaults to false: without it this only reports what it would remove. + * `limit` caps how many orphaned dashboards are acted on in one call, so a + * large backfill can be done in reviewable batches. + */ +export async function sweepOrphanedStorage( + env: EnvWithDriveCache, + options: { apply?: boolean; limit?: number } = {} +): Promise { + const apply = options.apply === true; + const limit = options.limit && options.limit > 0 ? options.limit : Infinity; + const bucket = env.DRIVE_CACHE; + + const candidateIds = [...(await discoverDashbоardIds(bucket))]; + const live = await findLiveDashbоardIds(env, candidateIds); + + const allOrphans = candidateIds.filter((id) => !live.has(id)); + const orphans = allOrphans.slice(0, limit === Infinity ? undefined : limit); + + let objects = 0; + let bytes = 0; + + for (const dashboardId of orphans) { + // Measure first either way: after purging there is nothing left to count, + // and the apply path should still report what it reclaimed. + for (const prefix of [`workspace/${dashboardId}/`, `drive/${dashboardId}/`]) { + const measured = await measurePrefix(bucket, prefix); + objects += measured.objects; + bytes += measured.bytes; + } + for (const provider of await listChildPrefixes(bucket, 'mirror/')) { + const measured = await measurePrefix(bucket, `mirror/${provider}/${dashboardId}/`); + objects += measured.objects; + bytes += measured.bytes; + } + + if (apply) { + // Reuse the same purge the delete path uses, so the sweep and normal + // deletion can never disagree about what belongs to a dashboard. + await purgeDashbоardStorage(env, dashboardId); + } + } + + const result: OrphanSweepResult = { + revision: SWEEP_REVISION, + dryRun: !apply, + candidates: candidateIds.length, + orphans, + objects, + bytes, + truncated: orphans.length < allOrphans.length, + }; + + console.log( + `[orphan-sweep] ${apply ? 'DELETED' : 'DRY RUN'} — ` + + `${orphans.length}/${allOrphans.length} orphaned dashboards, ` + + `${objects} object(s), ${bytes} byte(s)` + ); + + return result; +} diff --git a/e2e/.auth/.gitignore b/e2e/.auth/.gitignore new file mode 100644 index 00000000..d6b7ef32 --- /dev/null +++ b/e2e/.auth/.gitignore @@ -0,0 +1,2 @@ +* +!.gitignore diff --git a/e2e/.env.example b/e2e/.env.example index 567f8b3b..bb71105f 100644 --- a/e2e/.env.example +++ b/e2e/.env.example @@ -1,10 +1,47 @@ # Required: URL of the Orcabot instance to test against ORCABOT_URL=http://localhost:3000 +# Optional: the control plane origin. Derived from ORCABOT_URL by default +# (localhost -> :8787, dev.orcabot.com -> api.dev.orcabot.com, etc.). +# Only set this for a non-standard split origin. +# CONTROLPLANE_URL=http://localhost:8787 + +# Optional: path to a Playwright storageState JSON file for a pre-authenticated session. +# +# This is the ONLY way to log the BROWSER in on an instance without dev auth. +# Create it by logging in by hand, once: +# npm run auth:capture +# which writes e2e/.auth/orcabot-user.json (the default path, picked up +# automatically). Set this only to use a different location. +# +# Google login is NOT automated: Playwright puts action arguments into step +# titles, and those are written into the HTML report even for passing tests, so +# a password in env would end up in published artifacts. Hence no +# GOOGLE_TEST_PASSWORD below. +E2E_STORAGE_STATE=/absolute/path/to/orcabot2/e2e/.auth/orcabot-user.json + +# Optional: personal access token (Settings -> Personal Access Tokens) used for +# direct control plane API calls. This is the way to do API setup/teardown on an +# instance without dev auth. +# It authenticates API calls ONLY — it cannot log the browser in, so it is not a +# substitute for E2E_STORAGE_STATE in UI tests. +E2E_API_TOKEN= + +# Optional: per-boot surface token. Only needed against a target that sets +# SURFACE_TOKEN (desktop builds), which gates dev auth to the trusted frontend. +E2E_SURFACE_TOKEN= + # Optional: override test user credentials E2E_USER_NAME=E2E Test User E2E_USER_EMAIL=e2e-test@orcabot.test +# Optional: real account DATA for integration assertions (which mailbox to +# search, which calendar to read). NOT login credentials — logging in is done +# via E2E_STORAGE_STATE above. Put real values in e2e/.env.test.local, not here. +GOOGLE_TEST_EMAIL= +GOOGLE_GMAIL_TEST_EMAIL= +GOOGLE_CALENDAR_TEST_ID= + # Optional: API keys for agent/integration tests (tests that need these will skip if unset) ANTHROPIC_API_KEY= GEMINI_API_KEY= diff --git a/e2e/fixtures/auth.ts b/e2e/fixtures/auth.ts index 77dc59eb..f4ed502b 100644 --- a/e2e/fixtures/auth.ts +++ b/e2e/fixtures/auth.ts @@ -1,9 +1,197 @@ -import { type Page, type BrowserContext, expect } from "@playwright/test"; +// REVISION: e2e-auth-v7-nonmutating-devauth-probe +import { type Page, expect, request as playwrightRequest } from "@playwright/test"; import { generateUserId } from "../helpers/api"; +import { getEnv } from "../helpers/env"; +// The control-plane origin is derived from ORCABOT_URL (with CONTROLPLANE_URL as +// an override) so a single knob points the whole harness at an instance. import { CONTROLPLANE_URL } from "../helpers/controlplane-url"; -const DEFAULT_NAME = process.env.E2E_USER_NAME || "E2E Test User"; -const DEFAULT_EMAIL = process.env.E2E_USER_EMAIL || "e2e-test@orcabot.test"; +const MODULE_REVISION = "e2e-auth-v7-nonmutating-devauth-probe"; +console.log( + `[e2e-auth] REVISION: ${MODULE_REVISION} loaded at ${new Date().toISOString()}` +); + +/** + * There is deliberately NO password-based Google login here. + * + * Driving Google's form meant calling fill() with the account password, and + * Playwright puts action parameters into the step title — literally + * `Fill "" locator(...)`. That title is serialized into the HTML + * report's embedded data for PASSING tests, independently of trace and video + * settings, so any report published as a CI artifact carried the credential. + * Suppressing individual sinks (trace, then video, then the report) is + * whack-a-mole; the fix is to never type the password. + * + * For an instance without dev auth, capture a browser session once: + * + * npm run auth:capture + * + * That opens a real browser, you log in by hand (SSO, 2FA, whatever Google + * asks), and the resulting storageState is saved to e2e/.auth/orcabot-user.json, + * which playwright.config.ts picks up automatically. No secret in env, none in + * artifacts. + * + * E2E_API_TOKEN is NOT an alternative for UI tests — a PAT authenticates direct + * control-plane calls only and cannot log the browser in. + */ +const DEFAULT_NAME = getEnv("E2E_USER_NAME", "E2E Test User")!; +const DEFAULT_EMAIL = getEnv("E2E_USER_EMAIL", "e2e-test@orcabot.test")!; + +/** + * Whether the splash page is currently offering the dev-mode login form. + * + * Shared by devModeLoginViaUI and the UI-login spec so the test's skip decision + * and the helper's strategy choice can never disagree. Waits briefly rather + * than checking instantly, because the splash renders its login controls only + * after auth resolves. The caller must already be on a real page — on + * about:blank this is always false. + */ +export async function devLoginFormVisible( + page: Page, + timeoutMs = 8_000 +): Promise { + return page + .getByRole("button", { name: /dev mode login/i }) + .first() + .waitFor({ state: "visible", timeout: timeoutMs }) + .then(() => true) + .catch(() => false); +} + +/** Cached across a worker — whether dev auth is usable can't change mid-run. */ +let devAuthAvailableCache: boolean | undefined; + +/** + * Probe whether the target honors dev auth. + * + * Reads GET /users/me rather than POSTing /auth/dev/session. Both answer the + * question — dev auth is either honored or it isn't — but the POST MINTS A + * SESSION, and the probe then threw the cookie away. Every probe therefore left + * a 30-day user_sessions row that nothing ever collects, once per worker per + * run. /users/me is a plain read: it authenticates via the same dev-auth + * headers and writes no session. + * + * Deliberately uses an ISOLATED request context, not `page.request`: the latter + * shares the browser context's cookie jar, so an existing session could answer + * the probe and mask whether dev auth actually works — and, with the old + * minting probe, would silently pre-authenticate the very UI login flow that + * devModeLoginViaUI exists to exercise. + * + * A 401/403 means dev auth is off (E79406) or a SURFACE_TOKEN is provisioned + * and we aren't the trusted surface (E79407) — both mean "not usable", so fall + * through to another strategy. + */ +async function devAuthAvailable(): Promise { + if (devAuthAvailableCache !== undefined) { + return devAuthAvailableCache; + } + + const probe = await playwrightRequest.newContext(); + try { + const response = await probe.get(`${CONTROLPLANE_URL}/users/me`, { + headers: devAuthHeaders(DEFAULT_EMAIL, DEFAULT_NAME), + }); + devAuthAvailableCache = response.ok(); + } catch { + // Unreachable control plane — treat as unavailable and let the caller + // fall through to another strategy (or fail with a clearer message). + devAuthAvailableCache = false; + } finally { + await probe.dispose(); + } + + return devAuthAvailableCache; +} + +async function isAlreadyAuthenticated(page: Page): Promise { + await page.goto("/dashboards", { waitUntil: "domcontentloaded" }); + + const onDashboards = await page + .waitForURL(/\/dashboards(?:$|\?)/, { timeout: 10_000 }) + .then(() => true) + .catch(() => false); + + if (!onDashboards) { + return false; + } + + const dashboardsVisible = await dashboardsHeading(page) + .waitFor({ state: "visible", timeout: 10_000 }) + .then(() => true) + .catch(() => false); + + return dashboardsVisible; +} + +/** + * The "New Dashboard" section heading — the marker that we're on the dashboard + * picker rather than the splash page. + * + * Must be an exact heading match. A bare getByText("New Dashboard") is a + * case-insensitive SUBSTRING match, so it also matches the splash page's "Sign + * in and create a new dashboard — a shared workspace" — which made + * isAlreadyAuthenticated() return true while logged out, silently skipping + * login and failing the test later with a confusing URL assertion. + */ +function dashboardsHeading(page: Page) { + return page + .getByRole("heading", { name: "New Dashboard", exact: true }) + .first(); +} + +/** + * Headers that identify us to dev auth. + * + * `X-Orcabot-Surface` is only required when the target provisions a + * SURFACE_TOKEN (desktop builds); on cloud/local-dev it is unset and ignored. + */ +function devAuthHeaders(email: string, name: string): Record { + const headers: Record = { + "X-User-ID": generateUserId(email), + "X-User-Email": email, + "X-User-Name": name, + }; + const surfaceToken = getEnv("E2E_SURFACE_TOKEN"); + if (surfaceToken) { + headers["X-Orcabot-Surface"] = surfaceToken; + } + return headers; +} + +/** + * Dismiss the "Do you have an AI API key?" onboarding card. + * + * GET /user/setup answers `needsAiSetup: !hasAiKey && !dismissed`, and the test + * user has no AI key — so on every dashboard the chat panel expands and its + * setup card sits over the canvas as a fixed, centred overlay. It swallows + * clicks aimed at terminal and block connectors, which is not a product bug but + * makes canvas tests fail with an unhelpful "element intercepts pointer events". + * + * Dismissed through the real endpoint rather than by stubbing GET /user/setup, + * so the app is in a state a real user can actually be in. The flag is stored + * per user and this is idempotent, so repeat calls are cheap. + * + * Best-effort: a failure here must not fail login. If it does fail, the tests + * that care will fail loudly on the overlay anyway. + * + * Note for anyone adding onboarding coverage: a test that WANTS this card must + * use a fresh user, since dismissal persists server-side. + */ +async function dismissAiSetupPrompt( + page: Page, + email: string, + name: string +): Promise { + await page.request + // Cookies from the browser context authenticate this on instances without + // dev auth; the dev-auth headers are ignored there because authenticate() + // checks the session cookie first. + .post(`${CONTROLPLANE_URL}/user/setup/ai-dismissed`, { + headers: devAuthHeaders(email, name), + data: {}, + }) + .catch(() => undefined); +} /** * Log in by creating a server-side session directly via the control plane API, @@ -25,21 +213,27 @@ export async function devModeLogin( // Step 1: Create server-side session via direct API call const response = await page.request.post( `${CONTROLPLANE_URL}/auth/dev/session`, - { - headers: { - "X-User-ID": userId, - "X-User-Email": email, - "X-User-Name": name, - }, - } + { headers: devAuthHeaders(email, name) } ); - if (response.status() !== 204) { + // Any 2xx is success. This used to require exactly 204; the endpoint now + // returns 200 with a JSON body, which made every dev login fail here. + if (!response.ok()) { throw new Error( `Failed to create dev session: ${response.status()} ${await response.text()}` ); } + // Dev auth is email-keyed server-side: if a user with this email already + // exists, the control plane reconciles to ITS id and ignores our generated + // one. Prefer the resolved id for the injected auth state, or the frontend's + // /users/me sync sees a mismatch and lands on an empty dashboard list. + const resolvedUserId = await response + .json() + .then((body: { id?: string }) => body?.id) + .catch(() => undefined); + const effectiveUserId = resolvedUserId || userId; + // Step 2: Extract session cookie from the response const setCookieHeader = response.headers()["set-cookie"] || ""; const sessionMatch = setCookieHeader.match(/orcabot_session=([^;]+)/); @@ -67,7 +261,7 @@ export async function devModeLogin( const authState = JSON.stringify({ state: { user: { - id: userId, + id: effectiveUserId, name: name.trim(), email: email.trim().toLowerCase(), createdAt: new Date().toISOString(), @@ -86,13 +280,56 @@ export async function devModeLogin( authState ); - // Step 5: Navigate to dashboards (full load with auth state already set) + // Step 5: Clear the AI onboarding card while we are still off the canvas. + // The session cookie is in the context by now, so this call is authenticated. + await dismissAiSetupPrompt(page, email, name); + + // Step 6: Navigate to dashboards (full load with auth state already set) await page.goto("/dashboards"); - // Step 6: Wait for the dashboards page to stabilize + // Step 7: Wait for the dashboards page to stabilize await waitForDashboardsPage(page); } + +/** + * Log in using whichever strategy the target instance supports. + * + * Order matters — cheapest and least brittle first: + * 1. Already authenticated (a saved storageState, see E2E_STORAGE_STATE) + * 2. Dev auth (local / dev instances) + * There is no third strategy: automating Google's password form is deliberately + * not supported (see the note at the top of this file). On an instance without + * dev auth, capture a session with `npm run auth:capture` so step 1 succeeds. + */ +export async function login( + page: Page, + name = DEFAULT_NAME, + email = DEFAULT_EMAIL +): Promise { + if (await isAlreadyAuthenticated(page)) { + // Also needed on this path: a saved session can still have the onboarding + // card pending, and it would cover the canvas in every later test. + await dismissAiSetupPrompt(page, email, name); + return; + } + + if (await devAuthAvailable()) { + await devModeLogin(page, name, email); + return; + } + + throw new Error( + "No usable login strategy found.\n" + + "This instance does not accept dev auth, so the browser needs a saved session.\n" + + "Run `npm run auth:capture` to log in once by hand — it writes " + + "e2e/.auth/orcabot-user.json, which the config picks up automatically " + + "(override the path with E2E_STORAGE_STATE).\n" + + "Note E2E_API_TOKEN does NOT help here: a PAT authenticates control-plane " + + "API calls only and cannot log the browser in." + ); +} + /** * Log in via the dev mode UI form on the splash page. * @@ -107,11 +344,25 @@ export async function devModeLoginViaUI( name = DEFAULT_NAME, email = DEFAULT_EMAIL ): Promise { + if (await isAlreadyAuthenticated(page)) { + return; + } + await page.goto("/"); - // Wait for auth to resolve (login buttons appear) + // Decide from what the page actually renders, not from whether dev auth is + // reachable: an instance can accept dev auth over the API while the splash + // offers only Google, and waiting for a form that will never appear just + // burns the timeout. const devLoginBtn = page.getByRole("button", { name: /dev mode login/i }); - await devLoginBtn.waitFor({ state: "visible", timeout: 15_000 }); + if (!(await devLoginFormVisible(page))) { + throw new Error( + "No UI login strategy available: this build has no dev-mode login form, " + + "and Google login is not automated (it would leak the password into " + + "Playwright step titles). Use `npm run auth:capture` for a saved session, " + + "and let the UI-login spec skip." + ); + } // If already authenticated, we may see "Go to Dashboards" instead const alreadyLoggedIn = await page @@ -161,6 +412,10 @@ export async function devModeLoginViaUI( // Wait for the page to be stable await waitForDashboardsPage(page); + + // Same normalization as the API login path, so tests behave identically + // regardless of which strategy got us here. + await dismissAiSetupPrompt(page, email, name); } /** @@ -174,9 +429,7 @@ async function waitForDashboardsPage(page: Page): Promise { // Then wait for dashboard-specific content to confirm we're stable // (the "New Dashboard" section heading is always on the dashboard picker) - await expect( - page.getByText("New Dashboard").first() - ).toBeVisible({ timeout: 10_000 }); + await expect(dashboardsHeading(page)).toBeVisible({ timeout: 10_000 }); } /** @@ -195,6 +448,8 @@ export async function logout(page: Page): Promise { page .getByRole("button", { name: /dev mode login/i }) .or(page.getByRole("button", { name: /continue with google/i })) + .or(page.getByRole("link", { name: /^sign in$/i })) + .or(page.getByRole("link", { name: /get started/i })) .or(page.getByRole("button", { name: /get started/i })) .first() ).toBeVisible({ timeout: 10_000 }); diff --git a/e2e/fixtures/base.ts b/e2e/fixtures/base.ts index 41f184cc..ba04940b 100644 --- a/e2e/fixtures/base.ts +++ b/e2e/fixtures/base.ts @@ -1,5 +1,6 @@ +// REVISION: e2e-base-v2-diagnostics-env import { test as base, expect } from "@playwright/test"; -import { devModeLogin, devModeLoginViaUI, logout } from "./auth"; +import { login, devModeLoginViaUI, logout } from "./auth"; import { createDashboard, gotoDashboard, @@ -13,6 +14,13 @@ import { } from "./terminal"; import { OrcabotAPI } from "../helpers/api"; import { CONTROLPLANE_URL } from "../helpers/controlplane-url"; +import { createDiagnostics, type E2EDiagnostics } from "../helpers/diagnostics"; +import { getEnv, requiredEnvReport } from "../helpers/env"; + +const MODULE_REVISION = "e2e-base-v2-diagnostics-env"; +console.log( + `[e2e-base] REVISION: ${MODULE_REVISION} loaded at ${new Date().toISOString()}` +); /** Bundled auth helpers available in every test */ export interface AuthFixture { @@ -45,6 +53,11 @@ export interface APIFixture { client: OrcabotAPI; } +/** Run-level diagnostics attached to every test */ +export interface DiagnosticsFixture { + collector: E2EDiagnostics; +} + /** * Extended test with auth, dashboard, terminal, and api fixtures. * Import { test, expect } from this file in all recipe specs. @@ -54,10 +67,20 @@ export const test = base.extend<{ dashboard: DashboardFixture; terminal: TerminalFixture; api: APIFixture; + diagnostics: DiagnosticsFixture; }>({ + diagnostics: [ + async ({ page }, use, testInfo) => { + const collector = await createDiagnostics(page); + await use({ collector }); + await collector.attach(testInfo); + }, + { auto: true }, + ], + auth: async ({ page }, use) => { await use({ - login: (opts) => devModeLogin(page, opts?.name, opts?.email), + login: (opts) => login(page, opts?.name, opts?.email), loginViaUI: (opts) => devModeLoginViaUI(page, opts?.name, opts?.email), logout: () => logout(page), }); @@ -78,12 +101,10 @@ export const test = base.extend<{ }); // Auto-cleanup: attempt API-based delete for all tracked dashboards - const cpUrl = CONTROLPLANE_URL; if (trackedIds.length > 0) { - const email = - process.env.E2E_USER_EMAIL || "e2e-test@orcabot.test"; - const name = process.env.E2E_USER_NAME || "E2E Test User"; - const api = new OrcabotAPI(page.request, cpUrl, email, name); + const email = getEnv("E2E_USER_EMAIL", "e2e-test@orcabot.test")!; + const name = getEnv("E2E_USER_NAME", "E2E Test User")!; + const api = new OrcabotAPI(page.request, CONTROLPLANE_URL, email, name); for (const id of trackedIds) { try { await api.deleteDashboard(id); @@ -104,11 +125,47 @@ export const test = base.extend<{ }, api: async ({ page }, use) => { - const email = process.env.E2E_USER_EMAIL || "e2e-test@orcabot.test"; - const name = process.env.E2E_USER_NAME || "E2E Test User"; + const email = getEnv("E2E_USER_EMAIL", "e2e-test@orcabot.test")!; + const name = getEnv("E2E_USER_NAME", "E2E Test User")!; const client = new OrcabotAPI(page.request, CONTROLPLANE_URL, email, name); await use({ client }); }, }); +/** + * Validate the environment at import time. + * + * Deliberately NOT a test.beforeAll(): this module is imported by every spec, + * and a hook registered here binds to whichever spec's root suite happens to be + * loading, which Playwright rejects outright ("did not expect test.beforeAll() + * to be called here"). Plain module scope runs once per worker and fails before + * any test starts, which is what we actually want. + */ +function checkEnvironment(): void { + const report = requiredEnvReport(); + + // Only the smoke tier is fatal, and it holds just the values that cannot be + // defaulted or derived. Everything else is reported so a run that skips + // optional tiers says why, instead of failing a run that would have worked. + if (!report.smoke.ready) { + throw new Error( + `Smoke-tier E2E env is incomplete. Missing: ${report.smoke.missing.join( + ", " + )}. Set them in e2e/.env.test.local.` + ); + } + + for (const tier of ["google", "gemini"] as const) { + if (!report[tier].ready) { + console.log( + `[e2e-base] ${tier} tier unavailable — missing ${report[tier].missing.join( + ", " + )}. Tests needing it will skip.` + ); + } + } +} + +checkEnvironment(); + export { expect }; diff --git a/e2e/helpers/api.ts b/e2e/helpers/api.ts index 54efb9c3..d4d0aa5b 100644 --- a/e2e/helpers/api.ts +++ b/e2e/helpers/api.ts @@ -1,4 +1,11 @@ +// REVISION: e2e-api-v2-pat-auth import type { APIRequestContext } from "@playwright/test"; +import { getEnv } from "./env"; + +const MODULE_REVISION = "e2e-api-v2-pat-auth"; +console.log( + `[e2e-api] REVISION: ${MODULE_REVISION} loaded at ${new Date().toISOString()}` +); /** * Generate a stable user ID from email — mirrors frontend/src/stores/auth-store.ts:52 @@ -13,12 +20,23 @@ export function generateUserId(email: string): string { return `dev-${Math.abs(hash).toString(36)}`; } +/** Prefix the control plane uses to recognize a personal access token. */ +const PAT_PREFIX = "orca_pat_"; + /** * Direct control plane API calls for test setup/teardown. - * Uses the same header-based auth as the frontend's dev mode. + * + * Auth is either a personal access token (E2E_API_TOKEN) or dev-mode identity + * headers. A PAT is the only one of the two that works against an instance + * without dev auth, so it's the way to do API setup/teardown on a hosted + * instance. Mint one in Settings → Personal Access Tokens. + * + * Note a PAT authenticates API calls only — it is not a browser session, so UI + * tests still need dev auth or a saved storageState. */ export class OrcabotAPI { private userId: string; + private apiToken = getEnv("E2E_API_TOKEN"); constructor( private request: APIRequestContext, @@ -27,9 +45,20 @@ export class OrcabotAPI { private userName: string ) { this.userId = generateUserId(userEmail); + if (this.apiToken && !this.apiToken.startsWith(PAT_PREFIX)) { + throw new Error( + `E2E_API_TOKEN does not look like a personal access token (expected a ${PAT_PREFIX} prefix).` + ); + } } - private headers() { + private headers(): Record { + if (this.apiToken) { + return { + "Content-Type": "application/json", + Authorization: `Bearer ${this.apiToken}`, + }; + } return { "Content-Type": "application/json", "X-User-ID": this.userId, diff --git a/e2e/helpers/diagnostics.ts b/e2e/helpers/diagnostics.ts new file mode 100644 index 00000000..6ce8764c --- /dev/null +++ b/e2e/helpers/diagnostics.ts @@ -0,0 +1,336 @@ +// REVISION: e2e-diagnostics-v4-scope-401-to-users-me +import type { ConsoleMessage, Page, Request, TestInfo } from "@playwright/test"; + +const MODULE_REVISION = "e2e-diagnostics-v4-scope-401-to-users-me"; +console.log( + `[e2e-diagnostics] REVISION: ${MODULE_REVISION} loaded at ${new Date().toISOString()}` +); + +type DiagnosticLevel = "log" | "warning" | "error"; + +interface PerfEntry { + name?: string; + entryType?: string; + startTime?: number; + duration?: number; + value?: number; + hadRecentInput?: boolean; +} + +interface PerformanceSummary { + longTaskCount: number; + longTaskMaxMs: number; + cumulativeLayoutShift: number; + resourceCount: number; + slowResources: Array<{ name: string; duration: number }>; + navigation?: Record; +} + +export interface DiagnosticsSnapshot { + revision: string; + collectedAt: string; + url: string; + console: Array<{ type: string; text: string; location?: string }>; + pageErrors: string[]; + requestFailures: Array<{ url: string; method: string; failure: string | null }>; + performance: PerformanceSummary; + heuristics: { + consoleErrors: number; + pageErrors: number; + requestFailures: number; + longTaskMaxMs: number; + cumulativeLayoutShift: number; + }; +} + +export interface E2EDiagnostics { + snapshot: () => Promise; + attach: (testInfo: TestInfo) => Promise; + assertNoSevereIssues: (options?: { ignore?: RegExp[] }) => Promise; +} + +interface ExpectedConsoleError { + /** Why this specific response is expected, not a defect. */ + why: string; + /** Matched against the console message text (carries the status code). */ + text: RegExp; + /** Matched against the message location URL, when the case is endpoint-specific. */ + url?: RegExp; +} + +/** + * The narrow set of console errors that are browser-generated noise rather than + * app defects. + * + * Chromium emits "Failed to load resource: ... " for EVERY non-2xx + * response, so a couple of responses the app deliberately provokes and handles + * would otherwise fail every test that logs in. + * + * Deliberately enumerated case by case, scoped by endpoint wherever the status + * alone is ambiguous. A blanket 4xx filter would swallow real breakage — 400 + * (bad request), 409 (conflict), 422 (validation), 429 (rate limited) are all + * genuine failures worth failing a test over, as is anything 5xx. + * + * Everything is still captured in the attached diagnostics.json regardless; this + * only affects the pass/fail decision. Uncaught exceptions (pageErrors) and + * genuine console.error calls from app code are never filtered. + */ +const EXPECTED_CONSOLE_ERRORS: ExpectedConsoleError[] = [ + { + why: + "The login helpers load authenticated routes while logged out on purpose " + + "(isAlreadyAuthenticated opens /dashboards before any login), so the auth " + + "bootstrap's GET /users/me correctly answers 401/403. Scoped to that one " + + "endpoint: the dashboard page's other queries are gated on " + + "`isAuthenticated && isAuthResolved` so they never fire while logged out, " + + "which means a 401/403 from anywhere ELSE is a real authorization " + + "regression and must still fail the test.", + text: /the server responded with a status of 40[13]\b/i, + url: /\/users\/me\b/, + }, + { + why: + "A dashboard with no cached workspace snapshot answers 404; " + + "getWorkspaceSnapshot() documents that as expected and returns null.", + text: /the server responded with a status of 404\b/i, + url: /\/dashboards\/[^/]+\/workspace-snapshot\b/, + }, +]; + +/** True when a console error is one of the documented expected responses. */ +function isExpectedConsoleError(message: { + text: string; + location?: string; +}): boolean { + return EXPECTED_CONSOLE_ERRORS.some( + (expected) => + expected.text.test(message.text) && + (!expected.url || expected.url.test(message.location ?? "")) + ); +} + +declare global { + interface Window { + __orcabotE2EPerf?: { + longTasks: PerfEntry[]; + layoutShifts: PerfEntry[]; + resources: PerfEntry[]; + }; + } +} + +function formatConsoleLocation(msg: ConsoleMessage): string | undefined { + const location = msg.location(); + if (!location.url) return undefined; + return `${location.url}:${location.lineNumber ?? 0}:${location.columnNumber ?? 0}`; +} + +function classifyConsoleType(type: string): DiagnosticLevel { + if (type === "error" || type === "assert") return "error"; + if (type === "warning") return "warning"; + return "log"; +} + +async function installPerformanceObservers(page: Page): Promise { + await page.addInitScript(() => { + window.__orcabotE2EPerf = { + longTasks: [], + layoutShifts: [], + resources: [], + }; + + const store = window.__orcabotE2EPerf; + if (!store) return; + + try { + const longTaskObserver = new PerformanceObserver((list) => { + for (const entry of list.getEntries()) { + store.longTasks.push({ + name: entry.name, + entryType: entry.entryType, + startTime: entry.startTime, + duration: entry.duration, + }); + } + }); + longTaskObserver.observe({ type: "longtask", buffered: true }); + } catch {} + + try { + const layoutShiftObserver = new PerformanceObserver((list) => { + for (const entry of list.getEntries()) { + const layoutShift = entry as PerformanceEntry & { + value?: number; + hadRecentInput?: boolean; + }; + store.layoutShifts.push({ + name: entry.name, + entryType: entry.entryType, + startTime: entry.startTime, + duration: entry.duration, + value: layoutShift.value, + hadRecentInput: layoutShift.hadRecentInput, + }); + } + }); + layoutShiftObserver.observe({ type: "layout-shift", buffered: true }); + } catch {} + + try { + const resourceObserver = new PerformanceObserver((list) => { + for (const entry of list.getEntries()) { + store.resources.push({ + name: entry.name, + entryType: entry.entryType, + startTime: entry.startTime, + duration: entry.duration, + }); + } + }); + resourceObserver.observe({ type: "resource", buffered: true }); + } catch {} + }); +} + +function summarizePerformance( + perfData: Window["__orcabotE2EPerf"] | null, + navigationTiming: Record | undefined +): PerformanceSummary { + const longTasks = perfData?.longTasks ?? []; + const layoutShifts = perfData?.layoutShifts ?? []; + const resources = perfData?.resources ?? []; + const slowResources = resources + .filter((entry) => (entry.duration ?? 0) >= 1_000 && entry.name) + .slice(0, 10) + .map((entry) => ({ + name: entry.name || "unknown", + duration: Math.round(entry.duration || 0), + })); + + return { + longTaskCount: longTasks.length, + longTaskMaxMs: Math.round( + longTasks.reduce((max, entry) => Math.max(max, entry.duration || 0), 0) + ), + cumulativeLayoutShift: Number( + layoutShifts + .filter((entry) => !entry.hadRecentInput) + .reduce((sum, entry) => sum + (entry.value || 0), 0) + .toFixed(4) + ), + resourceCount: resources.length, + slowResources, + navigation: navigationTiming, + }; +} + +export async function createDiagnostics(page: Page): Promise { + const consoleMessages: DiagnosticsSnapshot["console"] = []; + const pageErrors: string[] = []; + const requestFailures: DiagnosticsSnapshot["requestFailures"] = []; + + await installPerformanceObservers(page); + + page.on("console", (msg) => { + const level = classifyConsoleType(msg.type()); + if (level === "log") return; + consoleMessages.push({ + type: msg.type(), + text: msg.text(), + location: formatConsoleLocation(msg), + }); + }); + + page.on("pageerror", (error) => { + pageErrors.push(error.message); + }); + + page.on("requestfailed", (request: Request) => { + requestFailures.push({ + url: request.url(), + method: request.method(), + failure: request.failure()?.errorText ?? null, + }); + }); + + async function snapshot(): Promise { + const [perfData, navigationTiming] = await Promise.all([ + page + .evaluate(() => window.__orcabotE2EPerf || null) + .catch(() => null), + page + .evaluate(() => { + const entry = performance.getEntriesByType( + "navigation" + )[0] as PerformanceNavigationTiming | undefined; + if (!entry) return undefined; + return { + domContentLoaded: Math.round( + entry.domContentLoadedEventEnd - entry.startTime + ), + loadEvent: Math.round(entry.loadEventEnd - entry.startTime), + responseStart: Math.round(entry.responseStart - entry.startTime), + }; + }) + .catch(() => undefined), + ]); + + // Deliberately NOT named `performance`: that shadows the global inside the + // page.evaluate callbacks above, so `performance.getEntriesByType` would + // resolve to this summary object instead of the browser's Performance API. + const perfSummary = summarizePerformance(perfData, navigationTiming); + + return { + revision: MODULE_REVISION, + collectedAt: new Date().toISOString(), + url: page.url(), + console: [...consoleMessages], + pageErrors: [...pageErrors], + requestFailures: [...requestFailures], + performance: perfSummary, + heuristics: { + consoleErrors: consoleMessages.filter( + (message) => classifyConsoleType(message.type) === "error" + ).length, + pageErrors: pageErrors.length, + requestFailures: requestFailures.length, + longTaskMaxMs: perfSummary.longTaskMaxMs, + cumulativeLayoutShift: perfSummary.cumulativeLayoutShift, + }, + }; + } + + async function attach(testInfo: TestInfo): Promise { + const data = await snapshot(); + await testInfo.attach("diagnostics.json", { + body: JSON.stringify(data, null, 2), + contentType: "application/json", + }); + } + + async function assertNoSevereIssues( + options: { ignore?: RegExp[] } = {} + ): Promise { + const data = await snapshot(); + if (data.pageErrors.length > 0) { + throw new Error( + `Detected page errors:\n${data.pageErrors.map((msg) => `- ${msg}`).join("\n")}` + ); + } + + const extraIgnores = options.ignore ?? []; + const errors = data.console + .filter((message) => classifyConsoleType(message.type) === "error") + .filter((message) => !isExpectedConsoleError(message)) + .filter((message) => !extraIgnores.some((pattern) => pattern.test(message.text))) + .map((message) => + message.location ? `- ${message.text} (${message.location})` : `- ${message.text}` + ); + + if (errors.length > 0) { + throw new Error(`Detected console errors:\n${errors.join("\n")}`); + } + } + + return { snapshot, attach, assertNoSevereIssues }; +} diff --git a/e2e/helpers/env.ts b/e2e/helpers/env.ts new file mode 100644 index 00000000..afa33319 --- /dev/null +++ b/e2e/helpers/env.ts @@ -0,0 +1,83 @@ +// REVISION: e2e-env-v3-no-password-tier +const MODULE_REVISION = "e2e-env-v3-no-password-tier"; +console.log( + `[e2e-env] REVISION: ${MODULE_REVISION} loaded at ${new Date().toISOString()}` +); + +export type E2ETier = "smoke" | "google" | "gemini"; + +export const E2E_ENV_REQUIREMENTS: Record = { + // Only what cannot be derived or defaulted. CONTROLPLANE_URL is derived from + // ORCABOT_URL (helpers/controlplane-url.ts) and is an override, not a + // requirement; E2E_USER_EMAIL / E2E_USER_NAME have working defaults. Demanding + // those would break invocations that pass ORCABOT_URL alone and work fine. + smoke: ["ORCABOT_URL"], + // Account DATA for integration assertions — not login credentials. Browser + // login uses a captured storageState (`npm run auth:capture`); no password is + // stored or typed, because Playwright would serialize it into step titles. + google: [ + "GOOGLE_TEST_EMAIL", + "GOOGLE_GMAIL_TEST_EMAIL", + "GOOGLE_CALENDAR_TEST_ID", + ], + gemini: ["GEMINI_API_KEY"], +}; + +export function getEnv(name: string, fallback?: string): string | undefined { + const value = process.env[name]; + if (typeof value === "string" && value.trim().length > 0) { + return value.trim(); + } + return fallback; +} + +export function hasEnv(name: string): boolean { + return Boolean(getEnv(name)); +} + +export function missingEnv(names: readonly string[]): string[] { + return names.filter((name) => !hasEnv(name)); +} + +export function missingTierEnv(tier: E2ETier): string[] { + return missingEnv(E2E_ENV_REQUIREMENTS[tier]); +} + +export function tierReady(tier: E2ETier): boolean { + return missingTierEnv(tier).length === 0; +} + +export function describeMissingEnv(names: readonly string[]): string { + const missing = missingEnv(names); + return missing.length === 0 + ? "" + : `Missing env: ${missing.join(", ")}. Set them in e2e/.env.test.local.`; +} + +export function requireEnv(name: string): string { + const value = getEnv(name); + if (!value) { + throw new Error(`Missing required env ${name}. Set it in e2e/.env.test.local.`); + } + return value; +} + +export function requiredEnvReport() { + return { + smoke: { + required: [...E2E_ENV_REQUIREMENTS.smoke], + missing: missingTierEnv("smoke"), + ready: tierReady("smoke"), + }, + google: { + required: [...E2E_ENV_REQUIREMENTS.google], + missing: missingTierEnv("google"), + ready: tierReady("google"), + }, + gemini: { + required: [...E2E_ENV_REQUIREMENTS.gemini], + missing: missingTierEnv("gemini"), + ready: tierReady("gemini"), + }, + }; +} diff --git a/e2e/package-lock.json b/e2e/package-lock.json index 12d43cf2..fa666e9e 100644 --- a/e2e/package-lock.json +++ b/e2e/package-lock.json @@ -7,7 +7,9 @@ "name": "orcabot-e2e", "devDependencies": { "@playwright/test": "^1.57.0", - "dotenv": "^17.3.1" + "@types/node": "^26.1.1", + "dotenv": "^17.3.1", + "typescript": "^5.9.3" } }, "node_modules/@playwright/test": { @@ -26,6 +28,16 @@ "node": ">=18" } }, + "node_modules/@types/node": { + "version": "26.1.1", + "resolved": "https://registry.npmjs.org/@types/node/-/node-26.1.1.tgz", + "integrity": "sha512-nxAkRSVkN1Y0JC1W8ky/fTfkGsMmcrRsbx+3XoZE+rMOX71kLYTV7fLXpqud1GpbpP5TuffXFqfX7fH2GgZREw==", + "dev": true, + "license": "MIT", + "dependencies": { + "undici-types": "~8.3.0" + } + }, "node_modules/dotenv": { "version": "17.3.1", "resolved": "https://registry.npmjs.org/dotenv/-/dotenv-17.3.1.tgz", @@ -85,6 +97,27 @@ "engines": { "node": ">=18" } + }, + "node_modules/typescript": { + "version": "5.9.3", + "resolved": "https://registry.npmjs.org/typescript/-/typescript-5.9.3.tgz", + "integrity": "sha512-jl1vZzPDinLr9eUt3J/t7V6FgNEw9QjvBPdysz9KfQDD41fQrC2Y4vKQdiaUpFT4bXlb1RHhLpp8wtm6M5TgSw==", + "dev": true, + "license": "Apache-2.0", + "bin": { + "tsc": "bin/tsc", + "tsserver": "bin/tsserver" + }, + "engines": { + "node": ">=14.17" + } + }, + "node_modules/undici-types": { + "version": "8.3.0", + "resolved": "https://registry.npmjs.org/undici-types/-/undici-types-8.3.0.tgz", + "integrity": "sha512-j375ScV60dom+YkPFIfTLcOiPxkN/buHz5GobjLhixFuANaNs3C9l4GmrWqejgXWJ7BbJcFYpTEUkS1Ge8bpZQ==", + "dev": true, + "license": "MIT" } } } diff --git a/e2e/package.json b/e2e/package.json index 0eac8803..ba42aadc 100644 --- a/e2e/package.json +++ b/e2e/package.json @@ -11,10 +11,14 @@ "test:terminal": "playwright test recipes/03-terminal-basic.spec.ts recipes/04-terminal-command.spec.ts", "test:integration": "playwright test recipes/05-integration-blocks.spec.ts recipes/06-integration-wiring.spec.ts recipes/07-policy-editor.spec.ts", "report": "playwright show-report", - "install-browsers": "playwright install chromium" + "install-browsers": "playwright install chromium", + "auth:capture": "node scripts/capture-auth.mjs", + "typecheck": "tsc --noEmit" }, "devDependencies": { "@playwright/test": "^1.57.0", - "dotenv": "^17.3.1" + "@types/node": "^26.1.1", + "dotenv": "^17.3.1", + "typescript": "^5.9.3" } } diff --git a/e2e/playwright.config.ts b/e2e/playwright.config.ts index f6ab0d4f..3454fbb4 100644 --- a/e2e/playwright.config.ts +++ b/e2e/playwright.config.ts @@ -1,14 +1,63 @@ +// REVISION: e2e-config-v4-storage-state-only-ui-auth import { defineConfig, devices } from "@playwright/test"; import dotenv from "dotenv"; +import { existsSync, readFileSync } from "fs"; import { resolve } from "path"; -// Load env vars from .env.test (API keys, ORCABOT_URL, etc.) -dotenv.config({ path: resolve(__dirname, ".env.test") }); +const MODULE_REVISION = "e2e-config-v4-storage-state-only-ui-auth"; +console.log( + `[e2e-config] REVISION: ${MODULE_REVISION} loaded at ${new Date().toISOString()}` +); + +/** + * Environment precedence, highest first: + * 1. the real environment — `ORCABOT_URL=... npx playwright test`, CI vars + * 2. .env.test.local (personal overrides, gitignored) + * 3. .env (personal defaults, gitignored) + * 4. .env.test (shared defaults) + * + * Applied by hand rather than with dotenv's `override: true`, which replaces + * values already in process.env. That would silently beat an inline + * ORCABOT_URL — pointing a run at the wrong instance despite the documented + * invocation below — and could also overwrite injected credentials or `CI` + * itself, which drives `forbidOnly` and `retries`. + */ +const ENV_FILES_LOWEST_FIRST = [".env.test", ".env", ".env.test.local"]; +const PRESET_ENV_KEYS = new Set(Object.keys(process.env)); + +for (const file of ENV_FILES_LOWEST_FIRST) { + const path = resolve(__dirname, file); + if (!existsSync(path)) continue; + const parsed = dotenv.parse(readFileSync(path)); + for (const [key, value] of Object.entries(parsed)) { + // Never touch a key the real environment already set. + if (PRESET_ENV_KEYS.has(key)) continue; + // Later files outrank earlier ones. + process.env[key] = value; + } +} const ORCABOT_URL = process.env.ORCABOT_URL; + +/** + * Pre-authenticated browser session, captured by `npm run auth:capture`. + * + * This is the ONLY way to log the browser in on an instance without dev auth. + * E2E_API_TOKEN is not an alternative here: a PAT authenticates direct + * control-plane API calls (the `api` fixture) and cannot establish a browser + * session. Automating Google's password form is deliberately unsupported — see + * the note at the top of fixtures/auth.ts. + * + * Full artifact capture stays on precisely because no password is ever typed. + */ +const storageStatePath = + process.env.E2E_STORAGE_STATE || resolve(__dirname, ".auth/orcabot-user.json"); +const storageState = existsSync(storageStatePath) ? storageStatePath : undefined; + if (!ORCABOT_URL) { throw new Error( "ORCABOT_URL environment variable is required.\n" + + "Set it in e2e/.env, e2e/.env.test.local, or pass it inline.\n" + "Example: ORCABOT_URL=https://app.orcabot.com npx playwright test" ); } @@ -42,9 +91,25 @@ export default defineConfig({ use: { baseURL: ORCABOT_URL, - trace: "on-first-retry", + storageState, + // "retain-on-failure" records EVERY test and discards the passing ones, so + // any cost lands on all of them — but measured per-test on this suite, that + // cost is small and machine load dominates: + // ~17s no artifacts + // ~50s trace + video, machine under load + // ~45s trace only, machine under load + // ~14s trace only, machine idle + // The same trace-only config produced both 45s and 14s, so earlier runs were + // slow because of contention (concurrent dev servers), not tracing. Video is + // off because a trace already carries DOM snapshots, network and console, + // which is what actually gets debugged. + // + // If wall clock ever does become the bottleneck, switch to "on-first-retry" + // with retries: 1 — traces then cost nothing on a first-attempt pass, at the + // price of reporting flakes as "flaky" rather than failing them. + trace: "retain-on-failure", screenshot: "only-on-failure", - video: "on-first-retry", + video: "off", actionTimeout: 15_000, }, diff --git a/e2e/recipes/01-auth-flow.spec.ts b/e2e/recipes/01-auth-flow.spec.ts index c7657b1a..0a6e1bda 100644 --- a/e2e/recipes/01-auth-flow.spec.ts +++ b/e2e/recipes/01-auth-flow.spec.ts @@ -1,38 +1,73 @@ +// REVISION: e2e-auth-flow-v2-ui-login-skip import { test, expect } from "../fixtures/base"; +import { devLoginFormVisible } from "../fixtures/auth"; + +const MODULE_REVISION = "e2e-auth-flow-v2-ui-login-skip"; +console.log( + `[e2e-auth-flow] REVISION: ${MODULE_REVISION} loaded at ${new Date().toISOString()}` +); test.describe("Recipe: Authentication Flow", () => { test("should log in via API and reach dashboards", async ({ page, auth, + diagnostics, }) => { await auth.login(); await expect(page).toHaveURL(/\/dashboards/); + await diagnostics.collector.assertNoSevereIssues(); }); - test("should log in via dev mode UI form", async ({ - page, - auth, - }) => { + // The dev-mode login form was removed from the splash page in "Add new splash + // page" (#193) — `loginDevMode` now only runs for desktop auto-login. The only + // other UI login is Google, which is deliberately not automated (it would put + // the password into Playwright step titles, and those reach the HTML report). + // So this covers the dev-mode form where it exists, and skips where it doesn't + // rather than being deleted. + test("should log in via the UI", async ({ page, auth, diagnostics }) => { + // Must navigate first: the page starts on about:blank, where no locator can + // ever be visible and the check would skip unconditionally. + await page.goto("/"); + + test.skip( + !(await devLoginFormVisible(page)), + "This build has no dev-mode login form; Google login is not automated." + ); + await auth.loginViaUI(); await expect(page).toHaveURL(/\/dashboards/); + await diagnostics.collector.assertNoSevereIssues(); }); - test("should persist auth across page reload", async ({ page, auth }) => { + test("should persist auth across page reload", async ({ + page, + auth, + diagnostics, + }) => { await auth.login(); await page.reload(); // Should still be on dashboards after reload await expect(page).toHaveURL(/\/dashboards/); + await diagnostics.collector.assertNoSevereIssues(); }); - test("should log out and redirect to splash", async ({ page, auth }) => { + test("should log out and redirect to splash", async ({ + page, + auth, + diagnostics, + }) => { await auth.login(); await auth.logout(); - // Should be back at splash page with login options visible + // Should be back at splash page with login options visible. The splash CTAs + // are links ("Sign In", "Get Started Free"), not buttons. await expect( page .getByRole("button", { name: /dev mode login/i }) + .or(page.getByRole("link", { name: /^sign in$/i })) + .or(page.getByRole("link", { name: /get started/i })) .or(page.getByRole("button", { name: /get started/i })) .first() ).toBeVisible({ timeout: 10_000 }); + await diagnostics.collector.assertNoSevereIssues(); }); }); diff --git a/e2e/recipes/02-dashboard-crud.spec.ts b/e2e/recipes/02-dashboard-crud.spec.ts index 1575c921..4bc72d9e 100644 --- a/e2e/recipes/02-dashboard-crud.spec.ts +++ b/e2e/recipes/02-dashboard-crud.spec.ts @@ -1,5 +1,11 @@ +// REVISION: e2e-dashboard-v1-diagnostics import { test, expect } from "../fixtures/base"; +const MODULE_REVISION = "e2e-dashboard-v1-diagnostics"; +console.log( + `[e2e-dashboard] REVISION: ${MODULE_REVISION} loaded at ${new Date().toISOString()}` +); + test.describe("Recipe: Dashboard CRUD", () => { test.beforeEach(async ({ page, auth }) => { await page.goto("/"); @@ -7,17 +13,23 @@ test.describe("Recipe: Dashboard CRUD", () => { await auth.login(); }); - test("should create a blank dashboard", async ({ page, dashboard }) => { + test("should create a blank dashboard", async ({ + page, + dashboard, + diagnostics, + }) => { const id = await dashboard.create("E2E-Blank-Test"); // Should be on the dashboard page with canvas visible await expect(page).toHaveURL(new RegExp(`/dashboards/${id}`)); await expect(page.locator(".react-flow")).toBeVisible(); + await diagnostics.collector.assertNoSevereIssues(); }); test("should show created dashboard in the list", async ({ page, dashboard, + diagnostics, }) => { const name = `E2E-List-${Date.now()}`; await dashboard.create(name); @@ -28,11 +40,13 @@ test.describe("Recipe: Dashboard CRUD", () => { // Should see the dashboard we just created await expect(page.getByText(name)).toBeVisible(); + await diagnostics.collector.assertNoSevereIssues(); }); test("should navigate back to dashboard list from canvas", async ({ page, dashboard, + diagnostics, }) => { await dashboard.create("E2E-Nav-Test"); @@ -44,5 +58,6 @@ test.describe("Recipe: Dashboard CRUD", () => { .click(); await expect(page).toHaveURL(/\/dashboards$/); + await diagnostics.collector.assertNoSevereIssues(); }); }); diff --git a/e2e/scripts/capture-auth.mjs b/e2e/scripts/capture-auth.mjs new file mode 100644 index 00000000..b49df580 --- /dev/null +++ b/e2e/scripts/capture-auth.mjs @@ -0,0 +1,110 @@ +// REVISION: e2e-capture-auth-v1-manual-storage-state +/** + * Capture a logged-in browser session for the e2e suite. + * + * Replaces automating Google's password form, which cannot be done safely: + * Playwright puts action parameters into step titles (`Fill "" ...`), + * and those titles are serialized into the HTML report even for passing tests, + * independently of trace/video settings. + * + * Here you log in by hand — SSO, 2FA, hardware key, whatever the account needs — + * and only the resulting cookies/localStorage are written to disk. No password + * is ever typed by Playwright, so none can reach an artifact. + * + * npm run auth:capture + * ORCABOT_URL=https://dev.orcabot.com npm run auth:capture + * + * The output path matches what playwright.config.ts loads by default; override + * with E2E_STORAGE_STATE on both capture and run. + * + * The saved file IS credential material (live session cookies). e2e/.auth/ is + * gitignored — keep it that way, and re-run this when the session expires. + * + * Plain .mjs on purpose: the suite has no TypeScript runner, and this must work + * with nothing installed beyond the existing devDependencies. + */ +import { chromium } from "@playwright/test"; +import dotenv from "dotenv"; +import { existsSync, mkdirSync, readFileSync } from "fs"; +import { dirname, resolve } from "path"; +import { fileURLToPath } from "url"; + +const MODULE_REVISION = "e2e-capture-auth-v1-manual-storage-state"; +console.log( + `[e2e-capture-auth] REVISION: ${MODULE_REVISION} loaded at ${new Date().toISOString()}` +); + +const E2E_DIR = resolve(dirname(fileURLToPath(import.meta.url)), ".."); + +// Same precedence as playwright.config.ts: the real environment always wins. +const PRESET_ENV_KEYS = new Set(Object.keys(process.env)); +for (const file of [".env.test", ".env", ".env.test.local"]) { + const path = resolve(E2E_DIR, file); + if (!existsSync(path)) continue; + for (const [key, value] of Object.entries(dotenv.parse(readFileSync(path)))) { + if (PRESET_ENV_KEYS.has(key)) continue; + process.env[key] = value; + } +} + +const ORCABOT_URL = process.env.ORCABOT_URL; +if (!ORCABOT_URL) { + console.error( + "ORCABOT_URL is required.\n" + + "Example: ORCABOT_URL=https://dev.orcabot.com npm run auth:capture" + ); + process.exit(1); +} + +const storageStatePath = + process.env.E2E_STORAGE_STATE || resolve(E2E_DIR, ".auth/orcabot-user.json"); + +async function main() { + console.log(`\nOpening ${ORCABOT_URL} …`); + console.log("Log in in the browser window. Leave it open until you land on"); + console.log("the dashboards page — the session saves automatically.\n"); + + const browser = await chromium.launch({ headless: false }); + const context = await browser.newContext(); + const page = await context.newPage(); + + await page.goto(ORCABOT_URL, { waitUntil: "domcontentloaded" }); + + // Wait for the dashboards route, however the user gets there. Generous + // timeout: a human may need to fetch a 2FA code. + const LOGIN_TIMEOUT_MS = 5 * 60_000; + try { + await page.waitForURL(/\/dashboards/, { timeout: LOGIN_TIMEOUT_MS }); + } catch { + console.error( + "\nTimed out waiting for /dashboards — nothing was saved.\n" + + "Re-run and complete the login within 5 minutes." + ); + await browser.close(); + process.exit(1); + } + + // Let the app settle so the auth state is fully written before we snapshot. + await page + .getByRole("heading", { name: "New Dashboard", exact: true }) + .first() + .waitFor({ state: "visible", timeout: 30_000 }) + .catch(() => { + console.warn( + "Reached /dashboards but the dashboard list did not render; saving anyway." + ); + }); + + mkdirSync(dirname(storageStatePath), { recursive: true }); + await context.storageState({ path: storageStatePath }); + await browser.close(); + + console.log(`\nSaved session to ${storageStatePath}`); + console.log("Treat it as a credential — it is gitignored; do not commit or share it."); + console.log("The suite will now pick it up automatically.\n"); +} + +main().catch((error) => { + console.error(error); + process.exit(1); +}); diff --git a/frontend/src/app/(app)/dashboards/[id]/page.tsx b/frontend/src/app/(app)/dashboards/[id]/page.tsx index 2045f450..8b2c7e0f 100644 --- a/frontend/src/app/(app)/dashboards/[id]/page.tsx +++ b/frontend/src/app/(app)/dashboards/[id]/page.tsx @@ -3,8 +3,8 @@ "use client"; -// REVISION: dashboard-v54-ai-setup-prompt -console.log(`[dashboard] REVISION: dashboard-v54-ai-setup-prompt loaded at ${new Date().toISOString()}`); +// REVISION: dashboard-v55-stable-tool-ids +console.log(`[dashboard] REVISION: dashboard-v55-stable-tool-ids loaded at ${new Date().toISOString()}`); import * as React from "react"; import { useParams, useRouter } from "next/navigation"; @@ -137,6 +137,21 @@ function formatTimeAgo(timestamp: number): string { type BlockType = DashboardItem["type"]; type BlockTool = { + /** + * Stable identifier for this toolbar entry — used for `data-guidance-target` + * and as the React key. + * + * Must be unique across ALL tool arrays. Deliberately explicit rather than + * derived from `label`: two different tools can legitimately share a display + * label (Google Calendar and Outlook Calendar both read "Calendar"), and a + * label-derived target made them indistinguishable — UIGuidanceOverlay + * resolves targets with querySelector, so guidance silently pointed at + * whichever button came first in the DOM. `type` is not usable either, since + * every terminal preset shares type "terminal". + * + * Renaming a label is a copy change and must not move a guidance target. + */ + id: string; type: BlockType; icon: React.ReactNode; label: string; @@ -165,39 +180,41 @@ const toFlowEdge = (edge: DashboardEdge): Edge => ({ // Only include types that exist in the DB schema const blockTools: BlockTool[] = [ - { type: "note", icon: , label: "Note" }, - { type: "todo", icon: , label: "Todo" }, - { type: "prompt", icon: , label: "Prompt" }, - { type: "decision", icon: , label: "Decision" }, - { type: "schedule", icon: , label: "Schedule" }, - { type: "browser", icon: , label: "Browser" }, - { type: "benchmark", icon: , label: "Benchmark" }, + { id: "note", type: "note", icon: , label: "Note" }, + { id: "todo", type: "todo", icon: , label: "Todo" }, + { id: "prompt", type: "prompt", icon: , label: "Prompt" }, + { id: "decision", type: "decision", icon: , label: "Decision" }, + { id: "schedule", type: "schedule", icon: , label: "Schedule" }, + { id: "browser", type: "browser", icon: , label: "Browser" }, + { id: "benchmark", type: "benchmark", icon: , label: "Benchmark" }, // Recipe is not in DB schema yet - uncomment when added: - // { type: "recipe", icon: , label: "Recipe" }, + // { id: "recipe", type: "recipe", icon: , label: "Recipe" }, ]; // Google integrations in their own section const googleTools: BlockTool[] = [ - { type: "gmail", icon: , label: "Gmail" }, - { type: "calendar", icon: , label: "Calendar" }, - { type: "contacts", icon: , label: "Contacts" }, - { type: "sheets", icon: , label: "Sheets" }, - { type: "forms", icon: , label: "Forms" }, + { id: "gmail", type: "gmail", icon: , label: "Gmail" }, + { id: "calendar", type: "calendar", icon: , label: "Calendar" }, + { id: "contacts", type: "contacts", icon: , label: "Contacts" }, + { id: "sheets", type: "sheets", icon: , label: "Sheets" }, + { id: "forms", type: "forms", icon: , label: "Forms" }, ]; // Microsoft integrations const microsoftTools: BlockTool[] = [ - { type: "outlook", icon: , label: "Outlook" }, - { type: "outlook_calendar", icon: , label: "Calendar" }, - { type: "teams", icon: , label: "Teams" }, + { id: "outlook", type: "outlook", icon: , label: "Outlook" }, + // Shares the label "Calendar" with the Google entry above — hence the + // explicit, distinct id. + { id: "outlook-calendar", type: "outlook_calendar", icon: , label: "Calendar" }, + { id: "teams", type: "teams", icon: , label: "Teams" }, ]; // Messaging integrations in their own section const messagingToolsAll: BlockTool[] = [ - { type: "slack", icon: , label: "Slack" }, - { type: "discord", icon: , label: "Discord" }, - { type: "whatsapp", icon: , label: "WhatsApp" }, - { type: "twitter", icon: , label: "X" }, + { id: "slack", type: "slack", icon: , label: "Slack" }, + { id: "discord", type: "discord", icon: , label: "Discord" }, + { id: "whatsapp", type: "whatsapp", icon: , label: "WhatsApp" }, + { id: "twitter", type: "twitter", icon: , label: "X" }, // Telegram, Matrix, Google Chat hidden until ready ]; @@ -210,18 +227,21 @@ const messagingTools: BlockTool[] = messagingToolsAll.filter( const terminalTools: BlockTool[] = [ { + id: "claude-code", type: "terminal", label: "Claude Code", icon: , terminalPreset: { command: "claude", agentic: true }, }, { + id: "gemini-cli", type: "terminal", label: "Gemini CLI", icon: , terminalPreset: { command: "gemini", agentic: true }, }, { + id: "codex", type: "terminal", label: "Codex", icon: , @@ -230,6 +250,7 @@ const terminalTools: BlockTool[] = [ // OpenCode - temporarily hidden: its newer "opentui" TUI crashes/garbles in the // xterm.js terminal and exits back to the shell. Re-enable in a later PR once fixed. // { + // id: "opencode", // type: "terminal", // label: "OpenCode", // icon: , @@ -237,6 +258,7 @@ const terminalTools: BlockTool[] = [ // }, // Droid - temporarily hidden until stable release // { + // id: "droid", // type: "terminal", // label: "Droid", // icon: , @@ -244,12 +266,14 @@ const terminalTools: BlockTool[] = [ // }, // OpenClaw - temporarily hidden (not installed in sandbox image) // { + // id: "openclaw", // type: "terminal", // label: "OpenClaw", // icon: , // terminalPreset: { command: "[ -f ~/.openclaw/.env ] && openclaw tui || openclaw onboard", agentic: true }, // }, { + id: "terminal", type: "terminal", label: "Terminal", icon: , @@ -3807,13 +3831,13 @@ export default function DashboardPage() { {!toolbarAgentsCollapsed && terminalTools.map((tool) => ( - + @@ -3834,13 +3858,13 @@ export default function DashboardPage() { {!toolbarBlocksCollapsed && blockTools.map((tool) => ( - + @@ -3861,13 +3885,13 @@ export default function DashboardPage() { {!toolbarGoogleCollapsed && googleTools.map((tool) => ( - + @@ -3888,13 +3912,13 @@ export default function DashboardPage() { {!toolbarMicrosoftCollapsed && microsoftTools.map((tool) => ( - + @@ -3916,13 +3940,13 @@ export default function DashboardPage() { {!toolbarMessagingCollapsed && messagingTools.map((tool) => ( - + diff --git a/frontend/src/components/terminal/XtermTerminal.tsx b/frontend/src/components/terminal/XtermTerminal.tsx index 71252ed7..453f6442 100644 --- a/frontend/src/components/terminal/XtermTerminal.tsx +++ b/frontend/src/components/terminal/XtermTerminal.tsx @@ -3,6 +3,12 @@ "use client"; +// REVISION: xterm-terminal-v2-clip-overflow +const MODULE_REVISION = "xterm-terminal-v2-clip-overflow"; +console.log( + `[xterm-terminal] REVISION: ${MODULE_REVISION} loaded at ${new Date().toISOString()}` +); + import * as React from "react"; import { cn } from "@/lib/utils"; import type { TerminalHandle, TerminalProps, TerminalTheme } from "./types"; @@ -230,8 +236,22 @@ export const XtermTerminal = React.forwardRef( ); } + // overflow-hidden keeps xterm's own layers inside the block. + // + // fit() only runs once the terminal connects, so a terminal that fails to + // get a session stays at xterm's 80x24 default. In a block narrower than + // ~454px the .xterm-rows layer then renders wider than its viewport and, + // being pointer-events:auto with overflow:visible, spills onto the canvas + // and swallows clicks meant for whatever sits to its right — a neighbouring + // block's connector, for example. Clipping means one terminal failing to + // connect can't make the rest of the canvas unusable. + // + // A no-op when the terminal is sized correctly, since there is then nothing + // outside the box. Deliberately applied here and NOT to the portal wrapper + // in TerminalBlock: that wrapper also holds ConnectionMarkers, which are + // positioned outside the block edges on purpose and must not be clipped. return ( -
+
{isLoading && (
Loading terminal... diff --git a/sandbox/docker/Dockerfile b/sandbox/docker/Dockerfile index 64ad6d95..896bfd1a 100644 --- a/sandbox/docker/Dockerfile +++ b/sandbox/docker/Dockerfile @@ -39,6 +39,8 @@ RUN apt-get update && apt-get install -y --no-install-recommends \ curl \ wget \ git \ + # Minimal init, used as PID 1 so orphaned processes get reaped (see ENTRYPOINT) + tini \ # Build essentials (needed for some npm packages) build-essential \ # Python (useful for user scripts) @@ -273,5 +275,20 @@ HEALTHCHECK --interval=15s --timeout=5s --start-period=10s --retries=3 \ # NOTE: Consider using capability-based approach (CAP_SYS_PTRACE, CAP_SETUID) in future USER root +# Run under tini as PID 1 so orphaned processes are reaped. +# +# Chromium forks a tree (zygote, renderers, GPU, crashpad) that are grandchildren +# of the server. When their parent is killed at session teardown they are +# reparented to PID 1, and PID 1 was the Go server, which has no SIGCHLD reaper — +# so they accumulated as zombies, one batch per browser session. Measured: 500 +# zombies / 519 PIDs after ~35 dashboards, all with PPID 1. That leaks PID slots +# until the container cannot fork. +# +# A blanket wait4(-1) reaper inside the server is NOT the fix: internal/pty +# relies on cmd.Wait() reaping its own children to recycle pool slots, and a +# competing reaper would steal those exit statuses. tini reaps only what gets +# reparented to it, leaving the server's direct children alone. +ENTRYPOINT ["/usr/bin/tini", "--"] + # Start the Go server CMD ["/usr/local/bin/orcabot-server"] diff --git a/sandbox/internal/browser/browser.go b/sandbox/internal/browser/browser.go index 1e90690d..20f0e4f4 100644 --- a/sandbox/internal/browser/browser.go +++ b/sandbox/internal/browser/browser.go @@ -1,7 +1,7 @@ // Copyright 2026 Rob Macrae. All rights reserved. // SPDX-License-Identifier: LicenseRef-Proprietary -// REVISION: browser-v11-status-kill-wait-reap +// REVISION: browser-v12-process-group-kill package browser import ( @@ -20,9 +20,35 @@ import ( "strings" "sync" "sync/atomic" + "syscall" "time" ) +// killProcessTree kills a browser process and the process group it leads, then +// reaps it. +// +// Each browser process is started with Setpgid so it leads its own group. +// Killing only the direct process leaves Chromium's children (zygote, +// renderers, GPU, crashpad) running; they are then reparented to PID 1 and +// linger. Signalling the negative pid delivers to the whole group instead. +// +// The Wait() is what actually reaps the direct child — without it a killed +// process stays a zombie holding its PID. Grandchildren are reaped by tini +// (see the Dockerfile ENTRYPOINT), which cannot be done from here. +func killProcessTree(cmd *exec.Cmd) { + if cmd == nil || cmd.Process == nil { + return + } + pid := cmd.Process.Pid + // Negative pid = the process group led by pid. Best-effort: the group may + // already be gone, and children that called setsid() escape it (tini still + // reaps those once they exit). + if err := syscall.Kill(-pid, syscall.SIGKILL); err != nil { + _ = cmd.Process.Kill() + } + _, _ = cmd.Process.Wait() +} + type Status struct { Running bool `json:"running"` Ready bool `json:"ready"` @@ -44,8 +70,8 @@ type Controller struct { processes []*exec.Cmd } -// REVISION: browser-v10-no-sandbox-warning -const browserRevision = "browser-v10-no-sandbox-warning" +// REVISION: browser-v12-process-group-kill +const browserRevision = "browser-v12-process-group-kill" func init() { log.Printf("[browser] REVISION: %s loaded at %s", browserRevision, time.Now().Format(time.RFC3339)) @@ -206,9 +232,7 @@ func (c *Controller) Start() (Status, error) { processes := []*exec.Cmd{xvfbCmd, vncCmd, websockifyCmd, chromiumCmd} killAll := func() { for _, cmd := range processes { - if cmd.Process != nil { - _ = cmd.Process.Kill() - } + killProcessTree(cmd) } } for i, cmd := range processes { @@ -217,6 +241,10 @@ func (c *Controller) Start() (Status, error) { logWriter := newProcessLogWriter(procName) cmd.Stdout = logWriter cmd.Stderr = logWriter + // Lead a process group so teardown can signal the whole tree, not just + // this pid — Chromium's children would otherwise survive and be + // reparented to PID 1. + cmd.SysProcAttr = &syscall.SysProcAttr{Setpgid: true} if err := cmd.Start(); err != nil { log.Printf("browser failed to start %s: %v", cmd.Path, err) killAll() @@ -345,11 +373,7 @@ func (c *Controller) Stop() { } for i := len(c.processes) - 1; i >= 0; i-- { - cmd := c.processes[i] - if cmd.Process != nil { - _ = cmd.Process.Kill() - _, _ = cmd.Process.Wait() - } + killProcessTree(c.processes[i]) } c.processes = nil @@ -375,11 +399,8 @@ func (c *Controller) Status() Status { if c.running && c.wsPort == wsPort { log.Printf("[browser] websockify port %d gone, processes exited unexpectedly — clearing state", wsPort) for _, p := range c.processes { - if p.Process != nil { - _ = p.Process.Kill() - // Reap so killed children don't linger as zombies (matches Stop()). - _, _ = p.Process.Wait() - } + // Kill the group and reap, same as Stop(). + killProcessTree(p) } c.processes = nil c.running = false