From 24aef16c97b52e276c6ce13db9cd9e2bf8307aeb Mon Sep 17 00:00:00 2001 From: issacboy123 Date: Thu, 20 Aug 2026 06:48:42 -0700 Subject: [PATCH 1/2] feat(coding-agent): delete parent-owned subagent sessions --- .../src/core/subagents/persistence.ts | 62 +++++ .../components/session-selector.ts | 89 +++++-- .../test/session-selector-path-delete.test.ts | 231 +++++++++++++++++- 3 files changed, 353 insertions(+), 29 deletions(-) diff --git a/packages/coding-agent/src/core/subagents/persistence.ts b/packages/coding-agent/src/core/subagents/persistence.ts index 7783da3ad..5c661e02b 100644 --- a/packages/coding-agent/src/core/subagents/persistence.ts +++ b/packages/coding-agent/src/core/subagents/persistence.ts @@ -62,6 +62,16 @@ export interface SelectedBranchRosterSnapshot { readonly warnings: readonly string[]; } +interface PathDeletionResult { + readonly ok: boolean; + readonly error?: string; +} + +interface ParentSessionDeletion { + readonly parentResult: T; + readonly ownedDirectoryMovedToTrash: boolean; +} + interface ChildCandidate { readonly name: string; readonly childSessionId?: string; @@ -299,12 +309,64 @@ function assertSafeDirectory(path: string, description: string): void { throw new Error(`${description} is not a safe directory: ${path}`); } +function lstatIfPresent(path: string): ReturnType | undefined { + try { + return lstatSync(path); + } catch (error) { + if ((error as NodeJS.ErrnoException).code === "ENOENT") return undefined; + throw error; + } +} + function ensureSafeDirectory(path: string, parentDirectory: string, description: string): void { assertContainedPath(parentDirectory, path); if (!existsSync(path)) mkdirSync(path); assertSafeDirectory(path, description); } +/** Internal helper for deleting a parent and only its validated, recoverably trashed child namespace. */ +export async function deleteParentSessionWithOwnedSubagents( + parentSessionFile: string, + moveOwnedDirectoryToTrash: (ownedDirectory: string) => Promise, + deleteParentSessionFile: (parentSessionFile: string) => Promise, +): Promise> { + const resolvedParentFile = resolve(parentSessionFile); + const header = loadEntriesFromFile(resolvedParentFile)[0]; + if (header?.type !== "session") throw new Error(`Session header is unavailable: ${resolvedParentFile}`); + assertValidSessionId(header.id); + + const parentDirectory = dirname(resolvedParentFile); + const namespaceDirectory = resolve(parentDirectory, ".subagents"); + const ownedDirectory = resolve(namespaceDirectory, header.id); + assertContainedPath(parentDirectory, namespaceDirectory); + assertContainedPath(parentDirectory, ownedDirectory); + + let ownedDirectoryMovedToTrash = false; + const namespaceStats = lstatIfPresent(namespaceDirectory); + if (namespaceStats) { + if (namespaceStats.isSymbolicLink() || !namespaceStats.isDirectory()) { + throw new Error(`Subagent namespace is not a safe directory: ${namespaceDirectory}`); + } + + const ownedStats = lstatIfPresent(ownedDirectory); + if (ownedStats) { + if (ownedStats.isSymbolicLink() || !ownedStats.isDirectory()) { + throw new Error(`Parent-owned subagent directory is not a safe directory: ${ownedDirectory}`); + } + await moveOwnedDirectoryToTrash(ownedDirectory); + if (lstatIfPresent(ownedDirectory)) { + throw new Error(`Parent-owned subagent directory remains after trash operation: ${ownedDirectory}`); + } + ownedDirectoryMovedToTrash = true; + } + } + + return { + parentResult: await deleteParentSessionFile(resolvedParentFile), + ownedDirectoryMovedToTrash, + }; +} + export class ParentOwnedSubagentSessionStore { readonly #parent: SessionManager; readonly #parentDirectory: string; diff --git a/packages/coding-agent/src/modes/interactive/components/session-selector.ts b/packages/coding-agent/src/modes/interactive/components/session-selector.ts index 1eec63c36..5aff0bc4d 100644 --- a/packages/coding-agent/src/modes/interactive/components/session-selector.ts +++ b/packages/coding-agent/src/modes/interactive/components/session-selector.ts @@ -17,6 +17,7 @@ import { } from "@earendil-works/diffex-tui"; import { KeybindingsManager } from "../../../core/keybindings.ts"; import type { SessionInfo, SessionListProgress } from "../../../core/session-manager.ts"; +import { deleteParentSessionWithOwnedSubagents } from "../../../core/subagents/persistence.ts"; import { canonicalizePath as _canonicalizePath } from "../../../utils/paths.ts"; import { theme } from "../theme/theme.ts"; import { DynamicBorder } from "./dynamic-border.ts"; @@ -641,31 +642,29 @@ class SessionList implements Component, Focusable { type SessionsLoader = (onProgress?: SessionListProgress) => Promise; -/** - * Delete a session file, trying the `trash` CLI first, then falling back to unlink - */ -async function deleteSessionFile( - sessionPath: string, -): Promise<{ ok: boolean; method: "trash" | "unlink"; error?: string }> { - // Try `trash` first (if installed) - const trashArgs = sessionPath.startsWith("-") ? ["--", sessionPath] : [sessionPath]; - const trashResult = spawnSync("trash", trashArgs, { encoding: "utf-8" }); +type SessionFileDeletionResult = { ok: boolean; method: "trash" | "unlink"; error?: string }; - const getTrashErrorHint = (): string | null => { - const parts: string[] = []; - if (trashResult.error) { - parts.push(trashResult.error.message); - } - const stderr = trashResult.stderr?.trim(); - if (stderr) { - parts.push(stderr.split("\n")[0] ?? stderr); - } - if (parts.length === 0) return null; - return `trash: ${parts.join(" · ").slice(0, 200)}`; +function movePathToTrash(path: string): { ok: boolean; error?: string } { + const trashArgs = path.startsWith("-") ? ["--", path] : [path]; + const trashResult = spawnSync("trash", trashArgs, { encoding: "utf-8" }); + const parts: string[] = []; + if (trashResult.error) parts.push(trashResult.error.message); + const stderr = trashResult.stderr?.trim(); + if (stderr) parts.push(stderr.split("\n")[0] ?? stderr); + + if (trashResult.status === 0 || !existsSync(path)) return { ok: true }; + return { + ok: false, + ...(parts.length === 0 ? {} : { error: `trash: ${parts.join(" · ").slice(0, 200)}` }), }; +} - // If trash reports success, or the file is gone afterwards, treat it as successful - if (trashResult.status === 0 || !existsSync(sessionPath)) { +/** + * Delete a session file, trying the `trash` CLI first, then falling back to unlink + */ +async function deleteSessionFile(sessionPath: string): Promise { + const trashResult = movePathToTrash(sessionPath); + if (trashResult.ok) { return { ok: true, method: "trash" }; } @@ -675,12 +674,48 @@ async function deleteSessionFile( return { ok: true, method: "unlink" }; } catch (err) { const unlinkError = err instanceof Error ? err.message : String(err); - const trashErrorHint = getTrashErrorHint(); - const error = trashErrorHint ? `${unlinkError} (${trashErrorHint})` : unlinkError; + const error = trashResult.error ? `${unlinkError} (${trashResult.error})` : unlinkError; return { ok: false, method: "unlink", error }; } } +type MoveOwnedDirectoryToTrash = (ownedDirectory: string) => Promise; +type DeleteParentSessionFile = (parentSessionFile: string) => Promise; + +/** Internal selector deletion seam used by focused tests. */ +export async function deleteSessionWithOwnedSubagents( + sessionPath: string, + moveOwnedDirectoryToTrash: MoveOwnedDirectoryToTrash, + deleteParentSessionFile: DeleteParentSessionFile, +): Promise { + try { + const deletion = await deleteParentSessionWithOwnedSubagents( + sessionPath, + moveOwnedDirectoryToTrash, + deleteParentSessionFile, + ); + if (!deletion.parentResult.ok && deletion.ownedDirectoryMovedToTrash) { + return { + ...deletion.parentResult, + error: `Parent-owned subagent sessions were moved to trash, but parent deletion failed: ${deletion.parentResult.error ?? "unknown error"}`, + }; + } + return deletion.parentResult; + } catch (error) { + return { + ok: false, + method: "trash", + error: `Parent-owned subagent cleanup failed; parent session was retained: ${error instanceof Error ? error.message : String(error)}`, + }; + } +} + +async function moveOwnedSubagentDirectoryToTrash(ownedDirectory: string): Promise { + const result = movePathToTrash(ownedDirectory); + if (!result.ok) throw new Error(result.error ?? "trash operation failed"); + if (existsSync(ownedDirectory)) throw new Error("trash reported success but the directory remains"); +} + /** * Component that renders a session selector */ @@ -832,7 +867,11 @@ export class SessionSelectorComponent extends Container implements Focusable { // Handle session deletion this.sessionList.onDeleteSession = async (sessionPath: string) => { - const result = await deleteSessionFile(sessionPath); + const result = await deleteSessionWithOwnedSubagents( + sessionPath, + moveOwnedSubagentDirectoryToTrash, + deleteSessionFile, + ); if (result.ok) { if (this.currentSessions) { diff --git a/packages/coding-agent/test/session-selector-path-delete.test.ts b/packages/coding-agent/test/session-selector-path-delete.test.ts index f50e9e0df..093ee8f61 100644 --- a/packages/coding-agent/test/session-selector-path-delete.test.ts +++ b/packages/coding-agent/test/session-selector-path-delete.test.ts @@ -1,11 +1,14 @@ -import { mkdirSync, mkdtempSync, rmSync, symlinkSync, writeFileSync } from "node:fs"; +import { existsSync, mkdirSync, mkdtempSync, renameSync, rmSync, symlinkSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; -import { join } from "node:path"; +import { basename, join } from "node:path"; import { setKeybindings } from "@earendil-works/diffex-tui"; import { afterEach, beforeAll, beforeEach, describe, expect, it } from "vitest"; import { KeybindingsManager } from "../src/core/keybindings.ts"; -import type { SessionInfo } from "../src/core/session-manager.ts"; -import { SessionSelectorComponent } from "../src/modes/interactive/components/session-selector.ts"; +import { type SessionInfo, SessionManager } from "../src/core/session-manager.ts"; +import { + deleteSessionWithOwnedSubagents, + SessionSelectorComponent, +} from "../src/modes/interactive/components/session-selector.ts"; import { initTheme } from "../src/modes/interactive/theme/theme.ts"; type Deferred = { @@ -83,6 +86,33 @@ function createSymlinkedSessionPaths(): { }; } +function createPersistedParentFixture(parentId = "parent-id"): { + baseDir: string; + parentFile: string; + namespaceDirectory: string; + ownedDirectory: string; +} { + const baseDir = mkdtempSync(join(tmpdir(), "diffex-parent-delete-")); + const parentFile = join(baseDir, "parent.jsonl"); + writeFileSync( + parentFile, + `${JSON.stringify({ + type: "session", + version: 3, + id: parentId, + timestamp: "2026-08-20T00:00:00.000Z", + cwd: baseDir, + })}\n`, + ); + const namespaceDirectory = join(baseDir, ".subagents"); + return { + baseDir, + parentFile, + namespaceDirectory, + ownedDirectory: join(namespaceDirectory, parentId), + }; +} + const CTRL_D = "\x04"; const CTRL_BACKSPACE = "\x1b[127;5u"; @@ -351,4 +381,197 @@ describe("session selector path/delete interactions", () => { expect(confirmationChanges).toEqual([]); expect(errorMessage).toBe("Cannot delete the currently active session"); }); + + it("trashes only the selected parent's owned directory before deleting the parent", async () => { + const fixture = createPersistedParentFixture(); + tempDirs.push(fixture.baseDir); + mkdirSync(fixture.ownedDirectory, { recursive: true }); + writeFileSync( + join(fixture.ownedDirectory, "child.jsonl"), + `${JSON.stringify({ + type: "session", + version: 3, + id: "child-id", + timestamp: "2026-08-20T00:00:00.000Z", + cwd: fixture.baseDir, + parentSession: fixture.parentFile, + })}\n`, + ); + const siblingDirectory = join(fixture.namespaceDirectory, "other-parent"); + mkdirSync(siblingDirectory); + writeFileSync(join(siblingDirectory, "sibling.jsonl"), "sibling\n"); + + const listed = await SessionManager.list(fixture.baseDir, fixture.baseDir); + expect(listed.map((session) => session.path)).toEqual([fixture.parentFile]); + + const calls: string[] = []; + const result = await deleteSessionWithOwnedSubagents( + fixture.parentFile, + async (ownedDirectory) => { + calls.push(`owned:${ownedDirectory}`); + renameSync(ownedDirectory, join(fixture.baseDir, `trashed-${basename(ownedDirectory)}`)); + }, + async (parentFile) => { + calls.push(`parent:${parentFile}`); + renameSync(parentFile, join(fixture.baseDir, "trashed-parent.jsonl")); + return { ok: true, method: "trash" }; + }, + ); + + expect(result).toEqual({ ok: true, method: "trash" }); + expect(calls).toEqual([`owned:${fixture.ownedDirectory}`, `parent:${fixture.parentFile}`]); + expect(existsSync(fixture.ownedDirectory)).toBe(false); + expect(existsSync(siblingDirectory)).toBe(true); + expect(existsSync(fixture.parentFile)).toBe(false); + }); + + it.each(["namespace", "owned"] as const)("deletes the parent when the %s directory is absent", async (missing) => { + const fixture = createPersistedParentFixture(); + tempDirs.push(fixture.baseDir); + if (missing === "owned") mkdirSync(fixture.namespaceDirectory); + let cleanupCalled = false; + + const result = await deleteSessionWithOwnedSubagents( + fixture.parentFile, + async () => { + cleanupCalled = true; + }, + async (parentFile) => { + renameSync(parentFile, join(fixture.baseDir, "trashed-parent.jsonl")); + return { ok: true, method: "trash" }; + }, + ); + + expect(result.ok).toBe(true); + expect(cleanupCalled).toBe(false); + expect(existsSync(fixture.parentFile)).toBe(false); + }); + + it("retains the parent when owned-directory cleanup fails", async () => { + const fixture = createPersistedParentFixture(); + tempDirs.push(fixture.baseDir); + mkdirSync(fixture.ownedDirectory, { recursive: true }); + let parentDeletionCalled = false; + + const result = await deleteSessionWithOwnedSubagents( + fixture.parentFile, + async () => { + throw new Error("trash unavailable"); + }, + async () => { + parentDeletionCalled = true; + return { ok: true, method: "trash" }; + }, + ); + + expect(result.ok).toBe(false); + expect(result.error).toContain("cleanup failed; parent session was retained"); + expect(result.error).toContain("trash unavailable"); + expect(parentDeletionCalled).toBe(false); + expect(existsSync(fixture.parentFile)).toBe(true); + expect(existsSync(fixture.ownedDirectory)).toBe(true); + }); + + it("retains the parent when cleanup reports success without moving the owned directory", async () => { + const fixture = createPersistedParentFixture(); + tempDirs.push(fixture.baseDir); + mkdirSync(fixture.ownedDirectory, { recursive: true }); + let parentDeletionCalled = false; + + const result = await deleteSessionWithOwnedSubagents( + fixture.parentFile, + async () => {}, + async () => { + parentDeletionCalled = true; + return { ok: true, method: "trash" }; + }, + ); + + expect(result.ok).toBe(false); + expect(result.error).toContain("remains after trash operation"); + expect(parentDeletionCalled).toBe(false); + expect(existsSync(fixture.parentFile)).toBe(true); + }); + + it("reports partial deletion when parent deletion fails after owned cleanup", async () => { + const fixture = createPersistedParentFixture(); + tempDirs.push(fixture.baseDir); + mkdirSync(fixture.ownedDirectory, { recursive: true }); + + const result = await deleteSessionWithOwnedSubagents( + fixture.parentFile, + async (ownedDirectory) => { + renameSync(ownedDirectory, join(fixture.baseDir, "trashed-owned")); + }, + async () => ({ ok: false, method: "unlink", error: "parent locked" }), + ); + + expect(result.ok).toBe(false); + expect(result.error).toBe( + "Parent-owned subagent sessions were moved to trash, but parent deletion failed: parent locked", + ); + expect(existsSync(fixture.ownedDirectory)).toBe(false); + expect(existsSync(fixture.parentFile)).toBe(true); + }); + + it.each(["namespace-file", "namespace-link", "owned-file", "owned-link"] as const)( + "rejects an unsafe %s without deleting the parent", + async (unsafeComponent) => { + const fixture = createPersistedParentFixture(); + tempDirs.push(fixture.baseDir); + const linkTarget = join(fixture.baseDir, "link-target"); + mkdirSync(linkTarget); + if (unsafeComponent === "namespace-file") { + writeFileSync(fixture.namespaceDirectory, "not a directory\n"); + } else if (unsafeComponent === "namespace-link") { + symlinkSync(linkTarget, fixture.namespaceDirectory, process.platform === "win32" ? "junction" : "dir"); + } else { + mkdirSync(fixture.namespaceDirectory); + if (unsafeComponent === "owned-file") writeFileSync(fixture.ownedDirectory, "not a directory\n"); + else symlinkSync(linkTarget, fixture.ownedDirectory, process.platform === "win32" ? "junction" : "dir"); + } + let parentDeletionCalled = false; + + const result = await deleteSessionWithOwnedSubagents( + fixture.parentFile, + async () => {}, + async () => { + parentDeletionCalled = true; + return { ok: true, method: "trash" }; + }, + ); + + expect(result.ok).toBe(false); + expect(result.error).toContain("is not a safe directory"); + expect(parentDeletionCalled).toBe(false); + expect(existsSync(fixture.parentFile)).toBe(true); + }, + ); + + it("rejects an invalid parent header id without touching another path", async () => { + const fixture = createPersistedParentFixture("../outside"); + tempDirs.push(fixture.baseDir); + const outsidePath = join(fixture.baseDir, "outside"); + mkdirSync(outsidePath); + let cleanupCalled = false; + let parentDeletionCalled = false; + + const result = await deleteSessionWithOwnedSubagents( + fixture.parentFile, + async () => { + cleanupCalled = true; + }, + async () => { + parentDeletionCalled = true; + return { ok: true, method: "trash" }; + }, + ); + + expect(result.ok).toBe(false); + expect(result.error).toContain("Session id must be non-empty"); + expect(cleanupCalled).toBe(false); + expect(parentDeletionCalled).toBe(false); + expect(existsSync(outsidePath)).toBe(true); + expect(existsSync(fixture.parentFile)).toBe(true); + }); }); From 101a69ec6ede57d17205eaa20718170ceaccf69d Mon Sep 17 00:00:00 2001 From: issacboy123 Date: Thu, 20 Aug 2026 06:54:40 -0700 Subject: [PATCH 2/2] fix(coding-agent): report partial subagent deletion --- .../components/session-selector.ts | 12 +++++++++- .../test/session-selector-path-delete.test.ts | 24 +++++++++++++++++++ 2 files changed, 35 insertions(+), 1 deletion(-) diff --git a/packages/coding-agent/src/modes/interactive/components/session-selector.ts b/packages/coding-agent/src/modes/interactive/components/session-selector.ts index 5aff0bc4d..2c63bc7b3 100644 --- a/packages/coding-agent/src/modes/interactive/components/session-selector.ts +++ b/packages/coding-agent/src/modes/interactive/components/session-selector.ts @@ -692,7 +692,17 @@ export async function deleteSessionWithOwnedSubagents( const deletion = await deleteParentSessionWithOwnedSubagents( sessionPath, moveOwnedDirectoryToTrash, - deleteParentSessionFile, + async (parentSessionFile): Promise => { + try { + return await deleteParentSessionFile(parentSessionFile); + } catch (error) { + return { + ok: false, + method: "trash", + error: error instanceof Error ? error.message : String(error), + }; + } + }, ); if (!deletion.parentResult.ok && deletion.ownedDirectoryMovedToTrash) { return { diff --git a/packages/coding-agent/test/session-selector-path-delete.test.ts b/packages/coding-agent/test/session-selector-path-delete.test.ts index 093ee8f61..10f26f528 100644 --- a/packages/coding-agent/test/session-selector-path-delete.test.ts +++ b/packages/coding-agent/test/session-selector-path-delete.test.ts @@ -514,6 +514,30 @@ describe("session selector path/delete interactions", () => { expect(existsSync(fixture.parentFile)).toBe(true); }); + it("reports partial deletion when parent deletion throws after owned cleanup", async () => { + const fixture = createPersistedParentFixture(); + tempDirs.push(fixture.baseDir); + mkdirSync(fixture.ownedDirectory, { recursive: true }); + + const result = await deleteSessionWithOwnedSubagents( + fixture.parentFile, + async (ownedDirectory) => { + renameSync(ownedDirectory, join(fixture.baseDir, "trashed-owned")); + }, + async () => { + throw new Error("parent callback failed"); + }, + ); + + expect(result).toEqual({ + ok: false, + method: "trash", + error: "Parent-owned subagent sessions were moved to trash, but parent deletion failed: parent callback failed", + }); + expect(existsSync(fixture.ownedDirectory)).toBe(false); + expect(existsSync(fixture.parentFile)).toBe(true); + }); + it.each(["namespace-file", "namespace-link", "owned-file", "owned-link"] as const)( "rejects an unsafe %s without deleting the parent", async (unsafeComponent) => {