Merge PR #8915: fix(session): only advertise --resume when the session is on disk (@re2zero)

2026-08-19 01:38:35 +02:00
parent 74366ee996 6fad3772fb
commit fc24a491fb
3 changed files with 62 additions and 2 deletions
@@ -4250,10 +4250,15 @@ export class InteractiveMode implements InteractiveModeContext {
 		popTerminalTitle();
 		this.stop();

-		// Print resumption hint if this is a persisted session
+		// Print resumption hint only if the session was actually materialized to
+		// durable storage. Persistence is lazy — a session that exits before its
+		// first assistant message (or dies early to an auth error, a mid-flight
+		// Ctrl+C, or a launch-then-quit) never wrote its JSONL, so the path is
+		// allocated but the file does not exist and `--resume <id>` would fail
+		// (issue #8860).
 		const sessionId = this.sessionManager.getSessionId();
 		const sessionFile = this.sessionManager.getSessionFile();
-		if (sessionId && sessionFile) {
+		if (sessionId && sessionFile && this.sessionManager.isSessionOnDisk()) {
 			process.stderr.write(`\n${chalk.dim(`Resume this session with ${APP_NAME} --resume ${sessionId}`)}\n`);
 		}

@@ -1950,6 +1950,21 @@ export class SessionManager {
 		return this.#sessionFile;
 	}

+	/**
+	 * Whether the current session has actually been materialized to durable
+	 * storage (the JSONL exists on disk / in the active storage backend).
+	 *
+	 * Session persistence is lazy: the file is only written once the history
+	 * contains an assistant message (or an explicit {@link ensureOnDisk}
+	 * caller forces it). Until then {@link getSessionFile} returns an allocated
+	 * path that leads nowhere, so a `--resume <id>` hint built from it would
+	 * always fail. Consumers that advertise a resume command must gate on this
+	 * (issue #8860).
+	 */
+	isSessionOnDisk(): boolean {
+		return !!this.#sessionFile && this.#storage.existsSync(this.#sessionFile);
+	}
+
 	getArtifactsDir(): string | null {
 		if (this.#adoptedArtifactManager) return this.#adoptedArtifactManager.dir;
 		return artifactsDirectoryFor(this.#sessionFile);
@@ -0,0 +1,40 @@
+/**
+ * Issue #8860 — the exit banner advertises `--resume <id>` for sessions that
+ * were never written to disk. Persistence is lazy: `getSessionFile()` returns
+ * an allocated path from the start, but the JSONL is only materialized once the
+ * history crosses the persistence gate (first assistant message / explicit
+ * `ensureOnDisk()`). Consumers advertising a resume command must gate on
+ * `isSessionOnDisk()` instead of just the allocated path.
+ */
+import { describe, expect, it } from "bun:test";
+import { join } from "node:path";
+import { SessionManager } from "@oh-my-pi/pi-coding-agent/session/session-manager";
+import { MemorySessionStorage } from "@oh-my-pi/pi-coding-agent/session/session-storage";
+
+function freshSession(): SessionManager {
+	const cwd = join("/tmp", `omp-on-disk-test-${Date.now()}-${Math.random().toString(36).slice(2)}`);
+	return SessionManager.create(cwd, join(cwd, "sessions"), new MemorySessionStorage());
+}
+
+describe("SessionManager.isSessionOnDisk (issue #8860)", () => {
+	it("returns false for a fresh lazy session whose JSONL was never materialized", () => {
+		const session = freshSession();
+		expect(session.getSessionId()).not.toBe("");
+		expect(session.getSessionFile()).toBeTruthy();
+		// The path is allocated up front, but no file exists yet.
+		expect(session.isSessionOnDisk()).toBe(false);
+	});
+
+	it("returns true once ensureOnDisk() materializes the session file", async () => {
+		const session = freshSession();
+		expect(session.isSessionOnDisk()).toBe(false);
+		await session.ensureOnDisk();
+		expect(session.isSessionOnDisk()).toBe(true);
+		expect(session.getSessionFile()).toBeTruthy();
+	});
+
+	it("stays false for an in-memory (non-persisting) session", () => {
+		const session = SessionManager.inMemory();
+		expect(session.isSessionOnDisk()).toBe(false);
+	});
+});