From 084177da16d3aed15e7ab345a2cec515a12ef389 Mon Sep 17 00:00:00 2001 From: Mohamed Boudra Date: Sat, 24 Jan 2026 23:34:09 +0700 Subject: [PATCH] Persist agent deletion and support subdir worktree cleanup --- .../app/e2e/delete-agent-persists.spec.ts | 67 +++++++++++ packages/app/src/components/agent-list.tsx | 3 + .../src/server/agent/agent-storage.test.ts | 64 +++++++++++ .../server/src/server/agent/agent-storage.ts | 105 +++++++++++++++--- packages/server/src/server/session.ts | 63 ++++++----- packages/server/src/utils/worktree.test.ts | 19 ++++ packages/server/src/utils/worktree.ts | 67 +++++++++-- 7 files changed, 338 insertions(+), 50 deletions(-) create mode 100644 packages/app/e2e/delete-agent-persists.spec.ts diff --git a/packages/app/e2e/delete-agent-persists.spec.ts b/packages/app/e2e/delete-agent-persists.spec.ts new file mode 100644 index 000000000..48ce3d1e2 --- /dev/null +++ b/packages/app/e2e/delete-agent-persists.spec.ts @@ -0,0 +1,67 @@ +import path from 'node:path'; + +import type { Locator, Page } from '@playwright/test'; + +import { test, expect } from './fixtures'; +import { ensureHostSelected, gotoHome, setWorkingDirectory } from './helpers/app'; +import { createTempGitRepo } from './helpers/workspace'; + +async function longPress(page: Page, locator: Locator, durationMs = 1100) { + const box = await locator.boundingBox(); + if (!box) { + throw new Error('Expected long-press target to have a bounding box.'); + } + await page.mouse.move(box.x + box.width / 2, box.y + box.height / 2); + await page.mouse.down(); + await page.waitForTimeout(durationMs); + await page.mouse.up(); +} + +test('deleting an agent via long-press persists after reload', async ({ page }) => { + const repo = await createTempGitRepo(); + const nonce = Math.random().toString(36).slice(2, 10); + const prompt = `delete-agent-persists-${nonce}`; + + try { + await gotoHome(page); + await setWorkingDirectory(page, repo.path); + await ensureHostSelected(page); + + // Create agent (via message input) so it shows up in the sidebar list. + const input = page.getByRole('textbox', { name: 'Message agent...' }); + await expect(input).toBeEditable(); + await input.fill(prompt); + await input.press('Enter'); + await page.waitForURL(/\/agent\//, { waitUntil: 'commit' }); + + const match = page.url().match(/\/agent\/([^/]+)\/([^/?#]+)/); + if (!match) { + throw new Error(`Expected /agent/:serverId/:agentId URL, got ${page.url()}`); + } + const serverId = decodeURIComponent(match[1]); + const agentId = decodeURIComponent(match[2]); + + // Return home and delete via long-press in the agent list. + await gotoHome(page); + const rowTestId = `agent-row-${serverId}-${agentId}`; + const agentRow = page.getByTestId(rowTestId).first(); + await expect(agentRow).toBeVisible({ timeout: 30000 }); + + await longPress(page, agentRow, 1200); + + const deleteButton = page.getByTestId('agent-action-delete').first(); + await expect(deleteButton).toBeVisible({ timeout: 10000 }); + await deleteButton.click({ force: true }); + await expect(page.getByTestId('agent-action-cancel')).toHaveCount(0, { timeout: 10000 }); + + // Ensure deletion finished before reload (avoids races). + await expect(page.getByTestId(rowTestId)).toHaveCount(0, { timeout: 30000 }); + + // A full reload should not bring the agent back. + await page.reload(); + await expect(page.getByRole('textbox', { name: 'Message agent...' })).toBeVisible(); + await expect(page.getByTestId(rowTestId)).toHaveCount(0, { timeout: 30000 }); + } finally { + await repo.cleanup(); + } +}); diff --git a/packages/app/src/components/agent-list.tsx b/packages/app/src/components/agent-list.tsx index 1ecbeaab3..bc06592e3 100644 --- a/packages/app/src/components/agent-list.tsx +++ b/packages/app/src/components/agent-list.tsx @@ -174,6 +174,7 @@ export function AgentList({ ]} onPress={() => handleAgentPress(agent.serverId, agent.id)} onLongPress={() => handleAgentLongPress(agent)} + testID={`agent-row-${agent.serverId}-${agent.id}`} > {({ hovered }) => ( @@ -278,6 +279,7 @@ export function AgentList({ disabled={!deleteAgent || isActionDaemonUnavailable} style={[styles.sheetButton, styles.sheetDeleteButton]} onPress={handleDeleteAgent} + testID="agent-action-delete" > Cancel diff --git a/packages/server/src/server/agent/agent-storage.test.ts b/packages/server/src/server/agent/agent-storage.test.ts index 2f0e92c7b..aea52e118 100644 --- a/packages/server/src/server/agent/agent-storage.test.ts +++ b/packages/server/src/server/agent/agent-storage.test.ts @@ -2,6 +2,7 @@ import { describe, expect, test, beforeEach, afterEach } from "vitest"; import os from "node:os"; import path from "node:path"; import { mkdtempSync, rmSync } from "node:fs"; +import { promises as fs } from "node:fs"; import { createTestLogger } from "../../test-utils/test-logger.js"; import { AgentStorage } from "./agent-storage.js"; @@ -280,4 +281,67 @@ describe("AgentStorage", () => { expect(records).toHaveLength(1); expect(records[0]?.internal).toBe(true); }); + + test("remove deletes all duplicate record files across project directories", async () => { + const agentId = "agent-duplicate"; + + // Create a valid record file in two different project directories to simulate + // storage migrations/duplication. Only one copy will be referenced in-memory, + // but deletion should remove *all* copies on disk. + const recordA = await (async () => { + await storage.applySnapshot( + createManagedAgent({ + id: agentId, + cwd: "/tmp/project-a", + provider: "codex", + }) + ); + const record = await storage.get(agentId); + expect(record).not.toBeNull(); + return record!; + })(); + + const projectDirB = path.join(storagePath, "tmp-project-b"); + await fs.mkdir(projectDirB, { recursive: true }); + const duplicatePathB = path.join(projectDirB, `${agentId}.json`); + await fs.writeFile( + duplicatePathB, + JSON.stringify({ ...recordA, cwd: "/tmp/project-b" }, null, 2), + "utf8" + ); + + // Force a reload so the registry has to discover from disk (and may choose either copy). + const reloaded = new AgentStorage(storagePath, logger); + const before = await reloaded.list(); + expect(before.map((r) => r.id)).toContain(agentId); + + await reloaded.remove(agentId); + + const hasAnyRecordFile = async () => { + try { + const projects = await fs.readdir(storagePath, { withFileTypes: true }); + for (const project of projects) { + if (!project.isDirectory()) { + continue; + } + const candidate = path.join(storagePath, project.name, `${agentId}.json`); + try { + await fs.access(candidate); + return true; + } catch { + // not here + } + } + } catch { + // ignore + } + return false; + }; + + expect(await hasAnyRecordFile()).toBe(false); + + const afterReload = new AgentStorage(storagePath, logger); + const after = await afterReload.list(); + expect(after.some((r) => r.id === agentId)).toBe(false); + }); }); diff --git a/packages/server/src/server/agent/agent-storage.ts b/packages/server/src/server/agent/agent-storage.ts index 5eb846c26..5fda7b895 100644 --- a/packages/server/src/server/agent/agent-storage.ts +++ b/packages/server/src/server/agent/agent-storage.ts @@ -67,6 +67,8 @@ export type StoredAgentRecord = z.infer; export class AgentStorage { private cache: Map = new Map(); private pathById: Map = new Map(); + private pendingWrites: Map> = new Map(); + private deleting: Set = new Set(); private loaded = false; private baseDir: string; private loadPromise: Promise | null = null; @@ -106,33 +108,102 @@ export class AgentStorage { async upsert(record: StoredAgentRecord): Promise { await this.load(); - const nextPath = this.buildRecordPath(record); - const previousPath = this.pathById.get(record.id); - - await fs.mkdir(path.dirname(nextPath), { recursive: true }); - await writeFileAtomically(nextPath, JSON.stringify(record, null, 2)); - - if (previousPath && previousPath !== nextPath) { - try { - await fs.unlink(previousPath); - } catch { - // ignore cleanup errors + const agentId = record.id; + const prev = this.pendingWrites.get(agentId) ?? Promise.resolve(); + const next = prev.then(async () => { + if (this.deleting.has(agentId)) { + return; } - } - this.cache.set(record.id, record); - this.pathById.set(record.id, nextPath); + const nextPath = this.buildRecordPath(record); + const previousPath = this.pathById.get(agentId); + + await fs.mkdir(path.dirname(nextPath), { recursive: true }); + await writeFileAtomically(nextPath, JSON.stringify(record, null, 2)); + + if (previousPath && previousPath !== nextPath) { + try { + await fs.unlink(previousPath); + } catch { + // ignore cleanup errors + } + } + + this.cache.set(agentId, record); + this.pathById.set(agentId, nextPath); + }); + + this.pendingWrites.set( + agentId, + next.finally(() => { + if (this.pendingWrites.get(agentId) === next) { + this.pendingWrites.delete(agentId); + } + }) + ); + + await next; + } + + beginDelete(agentId: string): void { + this.deleting.add(agentId); } async remove(agentId: string): Promise { await this.load(); + this.beginDelete(agentId); + await (this.pendingWrites.get(agentId) ?? Promise.resolve()); + const candidates = new Set(); + const existingPath = this.pathById.get(agentId) ?? (await this.findAgentPathById(agentId)); if (existingPath) { + candidates.add(existingPath); + } + + // Remove any stray duplicate record files across project directories. + // This can happen across storage layout migrations or when a record path changes. + try { + const projects = await fs.readdir(this.baseDir, { withFileTypes: true }); + for (const project of projects) { + if (!project.isDirectory()) { + continue; + } + const projectDir = path.join(this.baseDir, project.name); + candidates.add(path.join(projectDir, `${agentId}.json`)); + + // Support one more nesting layer (e.g. provider/version subfolders). + let entries: Array = []; + try { + entries = await fs.readdir(projectDir, { withFileTypes: true }); + } catch { + continue; + } + for (const entry of entries) { + if (!entry.isDirectory()) { + continue; + } + candidates.add(path.join(projectDir, entry.name, `${agentId}.json`)); + } + } + } catch { + // ignore scan errors + } + + // Support legacy flat layouts: baseDir/.json + candidates.add(path.join(this.baseDir, `${agentId}.json`)); + + for (const filePath of candidates) { try { - await fs.unlink(existingPath); - } catch { - // ignore removal errors + await fs.unlink(filePath); + } catch (error) { + const code = (error as NodeJS.ErrnoException).code; + if (code && code !== "ENOENT") { + this.logger.warn( + { err: error, agentId, filePath }, + "Failed to remove agent record file" + ); + } } } diff --git a/packages/server/src/server/session.ts b/packages/server/src/server/session.ts index 40ca3971e..0083289ea 100644 --- a/packages/server/src/server/session.ts +++ b/packages/server/src/server/session.ts @@ -78,12 +78,13 @@ import { WorktreeSetupError, type WorktreeConfig, type WorktreeSetupCommandResult, - slugify, - validateBranchSlug, - listPaseoWorktrees, - deletePaseoWorktree, - isPaseoOwnedWorktreeCwd, -} from "../utils/worktree.js"; + slugify, + validateBranchSlug, + listPaseoWorktrees, + deletePaseoWorktree, + isPaseoOwnedWorktreeCwd, + resolvePaseoWorktreeRootForCwd, + } from "../utils/worktree.js"; import { getCheckoutDiff, getCheckoutStatus, @@ -1148,6 +1149,9 @@ export class Session { `Deleting agent ${agentId} from registry` ); + // Prevent the persistence hook from re-creating the record while we close/delete. + this.agentStorage.beginDelete(agentId); + try { await this.agentManager.closeAgent(agentId); } catch (error: any) { @@ -3298,12 +3302,12 @@ export class Session { } } - private async handlePaseoWorktreeArchiveRequest( - msg: Extract - ): Promise { - const { requestId } = msg; - let targetPath = msg.worktreePath; - let repoRoot = msg.repoRoot ?? null; + private async handlePaseoWorktreeArchiveRequest( + msg: Extract + ): Promise { + const { requestId } = msg; + let targetPath = msg.worktreePath; + let repoRoot = msg.repoRoot ?? null; try { if (!targetPath) { @@ -3335,16 +3339,23 @@ export class Session { return; } - repoRoot = ownership.repoRoot ?? repoRoot ?? null; - if (!repoRoot) { - throw new Error("Unable to resolve repo root for worktree"); - } + repoRoot = ownership.repoRoot ?? repoRoot ?? null; + if (!repoRoot) { + throw new Error("Unable to resolve repo root for worktree"); + } - const removedAgents = new Set(); - const agents = this.agentManager.listAgents(); - for (const agent of agents) { - if (this.isPathWithinRoot(targetPath, agent.cwd)) { - removedAgents.add(agent.id); + const resolvedWorktree = await resolvePaseoWorktreeRootForCwd(targetPath, { + paseoHome: this.paseoHome, + }); + if (resolvedWorktree) { + targetPath = resolvedWorktree.worktreePath; + } + + const removedAgents = new Set(); + const agents = this.agentManager.listAgents(); + for (const agent of agents) { + if (this.isPathWithinRoot(targetPath, agent.cwd)) { + removedAgents.add(agent.id); try { await this.agentManager.closeAgent(agent.id); } catch { @@ -3370,11 +3381,11 @@ export class Session { } } - await deletePaseoWorktree({ - cwd: repoRoot, - worktreePath: targetPath, - paseoHome: this.paseoHome, - }); + await deletePaseoWorktree({ + cwd: repoRoot, + worktreePath: targetPath, + paseoHome: this.paseoHome, + }); for (const agentId of removedAgents) { this.emit({ diff --git a/packages/server/src/utils/worktree.test.ts b/packages/server/src/utils/worktree.test.ts index 62c5fc578..71e1477b0 100644 --- a/packages/server/src/utils/worktree.test.ts +++ b/packages/server/src/utils/worktree.test.ts @@ -305,6 +305,25 @@ describe("paseo worktree manager", () => { expect(remaining.map((worktree) => worktree.path)).toEqual([second.worktreePath]); }); + it("deletes a paseo worktree even when given a subdirectory path", async () => { + const created = await createWorktree({ + branchName: "main", + cwd: repoDir, + baseBranch: "main", + worktreeSlug: "alpha", + paseoHome, + }); + + const nestedDir = join(created.worktreePath, "nested", "dir"); + execSync(`mkdir -p ${nestedDir}`); + + await deletePaseoWorktree({ cwd: repoDir, worktreePath: nestedDir, paseoHome }); + expect(existsSync(created.worktreePath)).toBe(false); + + const remaining = await listPaseoWorktrees({ cwd: repoDir, paseoHome }); + expect(remaining.some((worktree) => worktree.path === created.worktreePath)).toBe(false); + }); + it("ensures .paseo is ignored in .gitignore", async () => { await ensurePaseoIgnored(repoDir); await ensurePaseoIgnored(repoDir); diff --git a/packages/server/src/utils/worktree.ts b/packages/server/src/utils/worktree.ts index dc0d1fea3..33c306bda 100644 --- a/packages/server/src/utils/worktree.ts +++ b/packages/server/src/utils/worktree.ts @@ -514,6 +514,57 @@ export async function listPaseoWorktrees({ .filter((entry) => entry.path.startsWith(rootPrefix)); } +export async function resolvePaseoWorktreeRootForCwd( + cwd: string, + options?: { paseoHome?: string } +): Promise<{ repoRoot: string; worktreeRoot: string; worktreePath: string } | null> { + let repoInfo: RepoInfo; + try { + repoInfo = await detectRepoInfo(cwd); + } catch { + return null; + } + + const worktreesRoot = await getPaseoWorktreesRoot(repoInfo.path, options?.paseoHome); + const resolvedRoot = normalizePathForOwnership(worktreesRoot) + sep; + + let worktreeRoot: string | null = null; + try { + const { stdout } = await execAsync( + "git rev-parse --path-format=absolute --show-toplevel", + { cwd, env: READ_ONLY_GIT_ENV } + ); + const trimmed = stdout.trim(); + worktreeRoot = trimmed.length > 0 ? trimmed : null; + } catch { + worktreeRoot = null; + } + + if (!worktreeRoot) { + return null; + } + + const resolvedWorktreeRoot = normalizePathForOwnership(worktreeRoot); + if (!resolvedWorktreeRoot.startsWith(resolvedRoot)) { + return null; + } + + const knownWorktrees = await listPaseoWorktrees({ + cwd: repoInfo.path, + paseoHome: options?.paseoHome, + }); + const match = knownWorktrees.find((entry) => entry.path === resolvedWorktreeRoot); + if (!match) { + return null; + } + + return { + repoRoot: repoInfo.path, + worktreeRoot: worktreesRoot, + worktreePath: match.path, + }; +} + export async function deletePaseoWorktree({ cwd, worktreePath, @@ -531,21 +582,23 @@ export async function deletePaseoWorktree({ const repoInfo = await detectRepoInfo(cwd); const worktreesRoot = await getPaseoWorktreesRoot(repoInfo.path, paseoHome); - const targetPath = worktreePath ?? join(worktreesRoot, worktreeSlug!); const resolvedRoot = normalizePathForOwnership(worktreesRoot) + sep; - const resolvedTarget = normalizePathForOwnership(targetPath); + const requestedPath = worktreePath ?? join(worktreesRoot, worktreeSlug!); + const resolvedRequested = normalizePathForOwnership(requestedPath); + const resolvedWorktree = + (await resolvePaseoWorktreeRootForCwd(requestedPath, { paseoHome }))?.worktreePath ?? + resolvedRequested; - if (!resolvedTarget.startsWith(resolvedRoot)) { + if (!resolvedWorktree.startsWith(resolvedRoot)) { throw new Error("Refusing to delete non-Paseo worktree"); } - const canonicalTargetPath = normalizePathForOwnership(targetPath); - await execAsync(`git worktree remove "${canonicalTargetPath}" --force`, { + await execAsync(`git worktree remove "${resolvedWorktree}" --force`, { cwd: repoInfo.path, }); - if (existsSync(canonicalTargetPath)) { - rmSync(canonicalTargetPath, { recursive: true, force: true }); + if (existsSync(resolvedWorktree)) { + rmSync(resolvedWorktree, { recursive: true, force: true }); } }