From 0cd022d5ae42785e9e4bf41164b7f595e817235e Mon Sep 17 00:00:00 2001 From: Rhine <2592562934@qq.com> Date: Fri, 18 Sep 2026 15:29:01 +0800 Subject: [PATCH 1/3] feat(sessions): add Ctrl+D shortcut to delete sessions (#551) --- extensions/sessions/index.ts | 151 +++++++++++++++------ extensions/sessions/sessions.ts | 29 ++++ tests/extensions/sessions/sessions.test.ts | 23 +++- 3 files changed, 163 insertions(+), 40 deletions(-) diff --git a/extensions/sessions/index.ts b/extensions/sessions/index.ts index f29299aa..5effe53f 100644 --- a/extensions/sessions/index.ts +++ b/extensions/sessions/index.ts @@ -1,4 +1,5 @@ // Adapted from pi-agent-extensions (MIT); see THIRD_PARTY_NOTICES.md. +import path from "node:path"; import type { ExtensionAPI, ExtensionCommandContext, @@ -41,6 +42,7 @@ import { buildSessionLabel, buildSessionPreview, buildSessionSearchEntries, + deleteSessionFile, filterSessionEntries, formatRelativeTime, getSessionPaneLayout, @@ -599,6 +601,31 @@ async function showSessionPicker( let focus: "list" | "preview" = "list"; let showAllWorkspaces = false; let isLoading = false; + let confirmingDeletePath: string | null = null; + + const handleDelete = async (sessionPath: string) => { + const result = await deleteSessionFile(sessionPath); + if (result.ok) { + sorted = sorted.filter((s) => s.path !== sessionPath); + sessionByPath.delete(sessionPath); + statsUniverseVersion++; + entries = buildSessionSearchEntries(sorted); + rebuild(); + schedulePreviewLoad(); + ctx.ui.notify( + result.method === "trash" + ? "Session moved to trash" + : "Session deleted", + "info", + ); + } else { + ctx.ui.notify( + `Failed to delete session: ${result.error ?? "unknown error"}`, + "error", + ); + } + tui.requestRender(); + }; const cancelPreviewLoad = () => { previewSeq++; @@ -818,21 +845,20 @@ async function showSessionPicker( selectList.onSelectionChange = (item) => setSelectedPath(item.value); container.addChild(selectList); } - container.addChild( - new Text( - hintLine( - theme, - [ - ["↑↓", "navigate"], - ["enter", "open"], - ["esc", "cancel"], - ], - Math.max(1, width - 2), - ), - 1, - 0, - ), - ); + const singlePaneHints = + confirmingDeletePath !== null + ? theme.fg("error", "Delete session? Enter confirm · Esc cancel") + : hintLine( + theme, + [ + ["↑↓", "navigate"], + ["enter", "open"], + ["ctrl+d", "delete"], + ["esc", "cancel"], + ], + Math.max(1, width - 2), + ); + container.addChild(new Text(singlePaneHints, 1, 0)); container.addChild( new DynamicBorder((text: string) => theme.fg("accent", text)), ); @@ -925,31 +951,30 @@ async function showSessionPicker( previewScrollOffset, renderedPreview.maxScroll, ); - // Hints live on their own line under the frame instead of being packed - // into the bottom border, where they had to compete with the border for - // the same row and lost the keys in a wall of dim text. - const hints = hintLine( - theme, + const hintItems = [ + renderedPreview.maxScroll > 0 + ? ([ + "", + `${previewScrollOffset + 1}-${Math.min(previewScrollOffset + contentHeight, renderedPreview.totalLines)}/${renderedPreview.totalLines}`, + ] as const) + : undefined, + renderedPreview.maxScroll > 0 + ? (["pgup/pgdn", "scroll"] as const) + : undefined, + ["t", toolsExpanded ? "compact" : "tools"] as const, + ["h", thinkingVisible ? "hide thinking" : "thinking"] as const, [ - renderedPreview.maxScroll > 0 - ? ([ - "", - `${previewScrollOffset + 1}-${Math.min(previewScrollOffset + contentHeight, renderedPreview.totalLines)}/${renderedPreview.totalLines}`, - ] as const) - : undefined, - renderedPreview.maxScroll > 0 - ? (["pgup/pgdn", "scroll"] as const) - : undefined, - ["t", toolsExpanded ? "compact" : "tools"], - ["h", thinkingVisible ? "hide thinking" : "thinking"], - [ - "opt+w/ctrl+t", - showAllWorkspaces ? "current workspace" : "all workspaces", - ], - ["esc", "close"], - ], - width, - ); + "opt+w/ctrl+t", + showAllWorkspaces ? "current workspace" : "all workspaces", + ] as const, + ["ctrl+d", "delete"] as const, + ["esc", "close"] as const, + ]; + + const hints = + confirmingDeletePath !== null + ? theme.fg("error", "Delete session? Enter confirm · Esc cancel") + : hintLine(theme, hintItems, width); const lines = [buildTopBorder(layout.listWidth, layout.previewWidth)]; for (let i = 0; i < contentHeight; i++) { @@ -981,6 +1006,29 @@ async function showSessionPicker( }, dispose: disposePicker, handleInput: (data) => { + if (confirmingDeletePath !== null) { + if ( + matchesKey(data, Key.enter) || + kb.matches(data, "tui.select.confirm") + ) { + const target = confirmingDeletePath; + confirmingDeletePath = null; + void handleDelete(target); + return; + } + if ( + matchesKey(data, Key.escape) || + kb.matches(data, "tui.select.cancel") + ) { + confirmingDeletePath = null; + tui.requestRender(); + return; + } + confirmingDeletePath = null; + tui.requestRender(); + return; + } + if (data === "\u0014" || data === "\u001bw") { showAllWorkspaces = !showAllWorkspaces; void loadWorkspaceSessions(showAllWorkspaces); @@ -1074,6 +1122,31 @@ async function showSessionPicker( } if (focus === "list") { + if ( + matchesKey(data, Key.ctrl("d")) || + kb.matches(data, "app.session.delete") || + data === "\u0004" + ) { + const selectedSession = sessionByPath.get(selectedPath); + if (selectedSession) { + const activeSessionPath = ctx.sessionManager.getSessionFile(); + if ( + activeSessionPath && + path.resolve(selectedSession.path) === + path.resolve(activeSessionPath) + ) { + ctx.ui.notify( + "Cannot delete the currently active session", + "error", + ); + return; + } + confirmingDeletePath = selectedSession.path; + tui.requestRender(); + return; + } + } + if (isPrintable(data)) { filter += data; rebuild(); diff --git a/extensions/sessions/sessions.ts b/extensions/sessions/sessions.ts index a6708cf8..dd676608 100644 --- a/extensions/sessions/sessions.ts +++ b/extensions/sessions/sessions.ts @@ -1,3 +1,6 @@ +import { spawnSync } from "node:child_process"; +import { existsSync } from "node:fs"; +import { unlink } from "node:fs/promises"; import { sanitizeTerminalText } from "../shared/terminal-text.ts"; export interface SessionInfoLike { @@ -422,3 +425,29 @@ export function buildPreviewError( error: message, }; } + +export async function deleteSessionFile( + sessionPath: string, +): Promise<{ ok: boolean; method: "trash" | "unlink"; error?: string }> { + if (!existsSync(sessionPath)) { + return { ok: false, method: "unlink", error: "File not found" }; + } + + const trashArgs = sessionPath.startsWith("-") + ? ["--", sessionPath] + : [sessionPath]; + try { + const trashResult = spawnSync("trash", trashArgs, { encoding: "utf-8" }); + if (trashResult.status === 0 || !existsSync(sessionPath)) { + return { ok: true, method: "trash" }; + } + } catch {} + + try { + await unlink(sessionPath); + return { ok: true, method: "unlink" }; + } catch (err) { + const error = err instanceof Error ? err.message : String(err); + return { ok: false, method: "unlink", error }; + } +} diff --git a/tests/extensions/sessions/sessions.test.ts b/tests/extensions/sessions/sessions.test.ts index 2f60f43a..02b792ff 100644 --- a/tests/extensions/sessions/sessions.test.ts +++ b/tests/extensions/sessions/sessions.test.ts @@ -1,13 +1,18 @@ import assert from "node:assert/strict"; +import { existsSync } from "node:fs"; +import { writeFile } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import path from "node:path"; import test from "node:test"; import { buildSessionDescription, buildSessionLabel, buildSessionPreview, + deleteSessionFile, filterSessionEntries, parseLimit, - selectSessionStatsWindow, type SessionInfoLike, + selectSessionStatsWindow, } from "../../../extensions/sessions/sessions.ts"; const session: SessionInfoLike = { @@ -144,3 +149,19 @@ test("bounded preview reports omitted messages and content bytes", () => { ]); assert.match(preview.subtitle, /100 messages/); }); + +test("deleteSessionFile removes an existing file", async () => { + const file = path.join(tmpdir(), `openpi-del-test-${Date.now()}.jsonl`); + await writeFile(file, "{}"); + assert.equal(existsSync(file), true); + + const result = await deleteSessionFile(file); + assert.equal(result.ok, true); + assert.equal(existsSync(file), false); +}); + +test("deleteSessionFile returns error on non-existent file", async () => { + const file = path.join(tmpdir(), `nonexistent-${Date.now()}.jsonl`); + const result = await deleteSessionFile(file); + assert.equal(result.ok, false); +}); From 01543d9b4cfcbca95a5b8709b5eea604ef2ecf80 Mon Sep 17 00:00:00 2001 From: Rhine <2592562934@qq.com> Date: Tue, 22 Sep 2026 09:04:52 +0800 Subject: [PATCH 2/3] fix(sessions): use bounded async process execution for trash (#551) --- extensions/sessions/sessions.ts | 55 ++++++++++++++++++---- tests/extensions/sessions/sessions.test.ts | 17 +++++++ 2 files changed, 62 insertions(+), 10 deletions(-) diff --git a/extensions/sessions/sessions.ts b/extensions/sessions/sessions.ts index dd676608..b28a4381 100644 --- a/extensions/sessions/sessions.ts +++ b/extensions/sessions/sessions.ts @@ -1,4 +1,4 @@ -import { spawnSync } from "node:child_process"; +import { execFile } from "node:child_process"; import { existsSync } from "node:fs"; import { unlink } from "node:fs/promises"; import { sanitizeTerminalText } from "../shared/terminal-text.ts"; @@ -426,22 +426,57 @@ export function buildPreviewError( }; } +const TRASH_TIMEOUT_MS = 2_000; + +function runTrashAsync( + sessionPath: string, + timeoutMs = TRASH_TIMEOUT_MS, + signal?: AbortSignal, +): Promise { + const trashArgs = sessionPath.startsWith("-") + ? ["--", sessionPath] + : [sessionPath]; + return new Promise((resolve) => { + try { + const child = execFile( + "trash", + trashArgs, + { + timeout: timeoutMs, + signal, + encoding: "utf8", + }, + (error) => { + if (!error || !existsSync(sessionPath)) { + resolve(true); + } else { + resolve(false); + } + }, + ); + child.on("error", () => resolve(false)); + } catch { + resolve(false); + } + }); +} + export async function deleteSessionFile( sessionPath: string, + options?: { timeoutMs?: number; signal?: AbortSignal }, ): Promise<{ ok: boolean; method: "trash" | "unlink"; error?: string }> { if (!existsSync(sessionPath)) { return { ok: false, method: "unlink", error: "File not found" }; } - const trashArgs = sessionPath.startsWith("-") - ? ["--", sessionPath] - : [sessionPath]; - try { - const trashResult = spawnSync("trash", trashArgs, { encoding: "utf-8" }); - if (trashResult.status === 0 || !existsSync(sessionPath)) { - return { ok: true, method: "trash" }; - } - } catch {} + const trashed = await runTrashAsync( + sessionPath, + options?.timeoutMs, + options?.signal, + ); + if (trashed || !existsSync(sessionPath)) { + return { ok: true, method: "trash" }; + } try { await unlink(sessionPath); diff --git a/tests/extensions/sessions/sessions.test.ts b/tests/extensions/sessions/sessions.test.ts index 02b792ff..fbe733af 100644 --- a/tests/extensions/sessions/sessions.test.ts +++ b/tests/extensions/sessions/sessions.test.ts @@ -165,3 +165,20 @@ test("deleteSessionFile returns error on non-existent file", async () => { const result = await deleteSessionFile(file); assert.equal(result.ok, false); }); + +test("deleteSessionFile is asynchronous and bounds execution timeout", async () => { + const file = path.join(tmpdir(), `openpi-del-async-${Date.now()}.jsonl`); + await writeFile(file, "{}"); + + let timerFired = false; + setTimeout(() => { + timerFired = true; + }, 1); + + const result = await deleteSessionFile(file, { timeoutMs: 100 }); + assert.equal(result.ok, true); + assert.equal(existsSync(file), false); + + await new Promise((resolve) => setTimeout(resolve, 5)); + assert.equal(timerFired, true); +}); From 3a621ae3d5399eb5fd31db8e3c460ae35a59b554 Mon Sep 17 00:00:00 2001 From: Rhine <2592562934@qq.com> Date: Wed, 23 Sep 2026 21:57:54 +0800 Subject: [PATCH 3/3] fix(sessions): confirm trash helper exit before deletion fallback --- extensions/sessions/sessions.ts | 96 +++++++++++++++------ tests/extensions/sessions/sessions.test.ts | 99 +++++++++++++++++++--- 2 files changed, 156 insertions(+), 39 deletions(-) diff --git a/extensions/sessions/sessions.ts b/extensions/sessions/sessions.ts index b28a4381..05ca7836 100644 --- a/extensions/sessions/sessions.ts +++ b/extensions/sessions/sessions.ts @@ -1,4 +1,4 @@ -import { execFile } from "node:child_process"; +import { spawn } from "node:child_process"; import { existsSync } from "node:fs"; import { unlink } from "node:fs/promises"; import { sanitizeTerminalText } from "../shared/terminal-text.ts"; @@ -427,37 +427,69 @@ export function buildPreviewError( } const TRASH_TIMEOUT_MS = 2_000; +const TRASH_KILL_GRACE_MS = 500; +type TrashOutcome = "closed" | "aborted" | "uncertain"; + +/** Do not fall back to unlink until the helper is confirmed closed. */ function runTrashAsync( sessionPath: string, timeoutMs = TRASH_TIMEOUT_MS, signal?: AbortSignal, -): Promise { +): Promise { + if (signal?.aborted) return Promise.resolve("aborted"); const trashArgs = sessionPath.startsWith("-") ? ["--", sessionPath] : [sessionPath]; return new Promise((resolve) => { + let child: ReturnType; try { - const child = execFile( - "trash", - trashArgs, - { - timeout: timeoutMs, - signal, - encoding: "utf8", - }, - (error) => { - if (!error || !existsSync(sessionPath)) { - resolve(true); - } else { - resolve(false); - } - }, - ); - child.on("error", () => resolve(false)); + child = spawn("trash", trashArgs, { stdio: "ignore" }); } catch { - resolve(false); + // Spawn failed before a helper process could be started. + resolve("closed"); + return; } + + let finished = false; + let stopped = false; + let aborted = false; + let killTimer: ReturnType | undefined; + const finish = (result: TrashOutcome) => { + if (finished) return; + finished = true; + clearTimeout(deadline); + if (killTimer) clearTimeout(killTimer); + signal?.removeEventListener("abort", onAbort); + resolve(result); + }; + const stop = (wasAborted: boolean) => { + if (stopped || finished) return; + stopped = true; + aborted = wasAborted; + // SIGKILL is required: a helper may ignore SIGTERM, including the + // SIGTERM sent by execFile's built-in timeout. + try { + child.kill("SIGKILL"); + } catch { + // A failed kill is not proof the helper has exited. + } + killTimer = setTimeout(() => { + child.unref(); + finish("uncertain"); + }, TRASH_KILL_GRACE_MS); + killTimer.unref?.(); + }; + const onAbort = () => stop(true); + const deadline = setTimeout(() => stop(false), timeoutMs); + deadline.unref?.(); + child.on("error", () => { + // A failed spawn is followed by close; wait for it rather than racing + // an error against a helper that may have already started. + }); + child.on("close", () => finish(aborted ? "aborted" : "closed")); + signal?.addEventListener("abort", onAbort, { once: true }); + if (signal?.aborted) stop(true); }); } @@ -469,14 +501,24 @@ export async function deleteSessionFile( return { ok: false, method: "unlink", error: "File not found" }; } - const trashed = await runTrashAsync( - sessionPath, - options?.timeoutMs, - options?.signal, - ); - if (trashed || !existsSync(sessionPath)) { - return { ok: true, method: "trash" }; + const timeoutMs = + options?.timeoutMs !== undefined && + Number.isFinite(options.timeoutMs) && + options.timeoutMs > 0 + ? Math.min(options.timeoutMs, TRASH_TIMEOUT_MS) + : TRASH_TIMEOUT_MS; + const outcome = await runTrashAsync(sessionPath, timeoutMs, options?.signal); + if (outcome === "uncertain") { + return { + ok: false, + method: "trash", + error: "Trash helper termination unconfirmed; fallback deletion skipped", + }; + } + if (outcome === "aborted") { + return { ok: false, method: "trash", error: "Session deletion cancelled" }; } + if (!existsSync(sessionPath)) return { ok: true, method: "trash" }; try { await unlink(sessionPath); diff --git a/tests/extensions/sessions/sessions.test.ts b/tests/extensions/sessions/sessions.test.ts index fbe733af..7adef7e6 100644 --- a/tests/extensions/sessions/sessions.test.ts +++ b/tests/extensions/sessions/sessions.test.ts @@ -1,6 +1,6 @@ import assert from "node:assert/strict"; import { existsSync } from "node:fs"; -import { writeFile } from "node:fs/promises"; +import { chmod, mkdtemp, readFile, rm, writeFile } from "node:fs/promises"; import { tmpdir } from "node:os"; import path from "node:path"; import test from "node:test"; @@ -166,19 +166,94 @@ test("deleteSessionFile returns error on non-existent file", async () => { assert.equal(result.ok, false); }); -test("deleteSessionFile is asynchronous and bounds execution timeout", async () => { - const file = path.join(tmpdir(), `openpi-del-async-${Date.now()}.jsonl`); +test("deleteSessionFile waits for a successful trash helper and reports trash", { + skip: process.platform === "win32" && "POSIX executable fixture", +}, async (t) => { + const dir = await mkdtemp(path.join(tmpdir(), "openpi-trash-success-")); + const oldPath = process.env.PATH; + t.after(async () => { + process.env.PATH = oldPath; + await rm(dir, { recursive: true, force: true }); + }); + const file = path.join(dir, "session.jsonl"); await writeFile(file, "{}"); + const helper = path.join(dir, "trash"); + await writeFile( + helper, + `#!/usr/bin/env node +const fs = require("node:fs"); +fs.renameSync(process.argv[2], process.argv[2] + ".trashed"); +`, + ); + await chmod(helper, 0o755); + process.env.PATH = `${dir}${path.delimiter}${path.dirname(process.execPath)}${path.delimiter}${oldPath ?? ""}`; + assert.deepEqual(await deleteSessionFile(file), { + ok: true, + method: "trash", + }); + assert.equal(existsSync(file), false); + assert.equal(existsSync(`${file}.trashed`), true); +}); - let timerFired = false; - setTimeout(() => { - timerFired = true; - }, 1); +test("deleteSessionFile does not delete when cancelled before starting", async (t) => { + const dir = await mkdtemp(path.join(tmpdir(), "openpi-trash-abort-")); + t.after(() => rm(dir, { recursive: true, force: true })); + const file = path.join(dir, "session.jsonl"); + await writeFile(file, "{}"); + const controller = new AbortController(); + controller.abort(); + const result = await deleteSessionFile(file, { signal: controller.signal }); + assert.equal(result.ok, false); + assert.equal(existsSync(file), true); +}); - const result = await deleteSessionFile(file, { timeoutMs: 100 }); - assert.equal(result.ok, true); - assert.equal(existsSync(file), false); +test("deleteSessionFile waits for a SIGTERM-resistant trash helper to close before unlink", { + skip: process.platform === "win32" && "POSIX executable fixture", +}, async (t) => { + const dir = await mkdtemp(path.join(tmpdir(), "openpi-trash-test-")); + const oldPath = process.env.PATH; + const oldReady = process.env.OPENPI_TEST_TRASH_READY; + const file = path.join(dir, "session.jsonl"); + const ready = path.join(dir, "ready"); + let pid: number | undefined; + t.after(async () => { + process.env.PATH = oldPath; + if (oldReady === undefined) delete process.env.OPENPI_TEST_TRASH_READY; + else process.env.OPENPI_TEST_TRASH_READY = oldReady; + if (pid !== undefined) { + try { + process.kill(pid, "SIGKILL"); + } catch {} + } + await rm(dir, { recursive: true, force: true }); + }); - await new Promise((resolve) => setTimeout(resolve, 5)); - assert.equal(timerFired, true); + await writeFile(file, "{}"); + const helper = path.join(dir, "trash"); + await writeFile( + helper, + `#!/usr/bin/env node +const fs = require("node:fs"); +process.on("SIGTERM", () => {}); +fs.writeFileSync(process.env.OPENPI_TEST_TRASH_READY, String(process.pid)); +setInterval(() => {}, 1000); +`, + ); + await chmod(helper, 0o755); + process.env.PATH = `${dir}${path.delimiter}${path.dirname(process.execPath)}${path.delimiter}${oldPath ?? ""}`; + process.env.OPENPI_TEST_TRASH_READY = ready; + + let pending = true; + let eventLoopResponsive = false; + setTimeout(() => { + if (pending) eventLoopResponsive = true; + }, 10); + const result = await deleteSessionFile(file, { timeoutMs: 1_500 }); + pending = false; + pid = Number(await readFile(ready, "utf8")); + assert.equal(eventLoopResponsive, true); + assert.deepEqual(result, { ok: true, method: "unlink" }); + assert.equal(existsSync(file), false); + // Once fallback has run, the trash helper cannot later touch the path. + assert.throws(() => process.kill(pid!, 0), { code: "ESRCH" }); });