diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index d8a49897..266d72b3 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -4,6 +4,7 @@ ### Fixed +- Fixed `--session` and `SessionManager.open()` to reject non-empty invalid session files without overwriting them ([#6002](https://github.com/earendil-works/pi/issues/6002)). - Fixed assistant messages stopped by output length to show a visible incomplete-response error ([#4290](https://github.com/earendil-works/pi/issues/4290)). - Fixed `--no-session --session-id` so ephemeral CLI runs can use deterministic session IDs for provider cache affinity ([#6070](https://github.com/earendil-works/pi/issues/6070)). - Fixed disk BMP image files to be detected, converted to PNG, and attached through `read` and CLI `@file` inputs ([#6047](https://github.com/earendil-works/pi/issues/6047)). diff --git a/packages/coding-agent/src/core/session-manager.ts b/packages/coding-agent/src/core/session-manager.ts index 56cab30d..feb177ad 100644 --- a/packages/coding-agent/src/core/session-manager.ts +++ b/packages/coding-agent/src/core/session-manager.ts @@ -795,10 +795,13 @@ export class SessionManager { if (existsSync(this.sessionFile)) { this.fileEntries = loadEntriesFromFile(this.sessionFile); - // If file was empty or corrupted (no valid header), truncate and start fresh - // to avoid appending messages without a session header (which breaks the session) + // If file was empty, initialize it with a valid session header. If it was + // non-empty but did not parse as a pi session, fail without modifying it. if (this.fileEntries.length === 0) { const explicitPath = this.sessionFile; + if (statSync(explicitPath).size > 0) { + throw new Error(`Session file is not a valid pi session and was not modified: ${explicitPath}`); + } this.newSession(); this.sessionFile = explicitPath; this._rewriteFile(); diff --git a/packages/coding-agent/src/main.ts b/packages/coding-agent/src/main.ts index 9ee6df2a..6b345982 100644 --- a/packages/coding-agent/src/main.ts +++ b/packages/coding-agent/src/main.ts @@ -241,6 +241,16 @@ function validateSessionIdFlags(parsed: Args): void { } } +function openSessionOrExit(path: string, sessionDir?: string): SessionManager { + try { + return SessionManager.open(path, sessionDir); + } catch (error: unknown) { + const message = error instanceof Error ? error.message : String(error); + console.error(chalk.red(`Error: ${message}`)); + process.exit(1); + } +} + function forkSessionOrExit(sourcePath: string, cwd: string, sessionDir?: string, sessionId?: string): SessionManager { try { return SessionManager.forkFrom(sourcePath, cwd, sessionDir, { id: sessionId }); @@ -290,7 +300,7 @@ async function createSessionManager( switch (resolved.type) { case "path": case "local": - return SessionManager.open(resolved.path, sessionDir); + return openSessionOrExit(resolved.path, sessionDir); case "global": { console.log(chalk.yellow(`Session found in different project: ${resolved.cwd}`)); diff --git a/packages/coding-agent/test/session-file-invalid.test.ts b/packages/coding-agent/test/session-file-invalid.test.ts new file mode 100644 index 00000000..47767b5f --- /dev/null +++ b/packages/coding-agent/test/session-file-invalid.test.ts @@ -0,0 +1,67 @@ +import { spawn } from "node:child_process"; +import { mkdirSync, mkdtempSync, readFileSync, realpathSync, rmSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join, resolve } from "node:path"; +import { afterEach, describe, expect, it } from "vitest"; +import { ENV_AGENT_DIR } from "../src/config.ts"; + +const cliPath = resolve(__dirname, "../src/cli.ts"); +const tempDirs: string[] = []; + +afterEach(() => { + for (const dir of tempDirs.splice(0)) { + rmSync(dir, { recursive: true, force: true }); + } +}); + +function createTempDir(): string { + const dir = realpathSync(mkdtempSync(join(tmpdir(), "pi-session-file-invalid-"))); + tempDirs.push(dir); + return dir; +} + +async function runCli(args: string[], cwd: string, agentDir: string): Promise<{ code: number | null; stderr: string }> { + let stderr = ""; + const code = await new Promise((resolvePromise, reject) => { + const child = spawn(process.execPath, [cliPath, ...args], { + cwd, + env: { + ...process.env, + [ENV_AGENT_DIR]: agentDir, + PI_OFFLINE: "1", + TSX_TSCONFIG_PATH: resolve(__dirname, "../../../tsconfig.json"), + }, + stdio: ["ignore", "ignore", "pipe"], + }); + child.stderr.on("data", (chunk) => { + stderr += chunk.toString(); + }); + child.on("error", reject); + child.on("close", resolvePromise); + }); + + return { code, stderr }; +} + +describe("--session invalid file handling", () => { + it("prints a friendly error and preserves non-session file content", async () => { + const tempRoot = createTempDir(); + const agentDir = join(tempRoot, "agent"); + const projectDir = join(tempRoot, "project"); + const sessionFile = join(tempRoot, "not-a-session.log"); + const originalContent = '{"type":"event","data":"not a session"}\n'; + mkdirSync(agentDir, { recursive: true }); + mkdirSync(projectDir, { recursive: true }); + writeFileSync(sessionFile, originalContent); + + const result = await runCli(["--session", sessionFile, "-p", "hi"], projectDir, agentDir); + + expect(result.code).toBe(1); + expect(result.stderr).toContain( + `Error: Session file is not a valid pi session and was not modified: ${sessionFile}`, + ); + expect(result.stderr).not.toContain("SessionManager.open"); + expect(result.stderr).not.toContain("at "); + expect(readFileSync(sessionFile, "utf8")).toBe(originalContent); + }); +}); diff --git a/packages/coding-agent/test/session-manager/file-operations.test.ts b/packages/coding-agent/test/session-manager/file-operations.test.ts index 626dd52c..379c73fd 100644 --- a/packages/coding-agent/test/session-manager/file-operations.test.ts +++ b/packages/coding-agent/test/session-manager/file-operations.test.ts @@ -268,28 +268,27 @@ describe("SessionManager.setSessionFile with corrupted files", () => { expect(header.id).toBe(sm.getSessionId()); }); - it("truncates and rewrites file without valid header", () => { + it("throws and preserves non-empty file without valid header", () => { const noHeaderFile = join(tempDir, "no-header.jsonl"); - // File with messages but no session header (corrupted state) - writeFileSync( - noHeaderFile, - '{"type":"message","id":"abc","parentId":"orphaned","timestamp":"2025-01-01T00:00:00Z","message":{"role":"assistant","content":"test"}}\n', + const originalContent = + '{"type":"message","id":"abc","parentId":"orphaned","timestamp":"2025-01-01T00:00:00Z","message":{"role":"assistant","content":"test"}}\n'; + writeFileSync(noHeaderFile, originalContent); + + expect(() => SessionManager.open(noHeaderFile, tempDir)).toThrow( + `Session file is not a valid pi session and was not modified: ${noHeaderFile}`, ); + expect(readFileSync(noHeaderFile, "utf-8")).toBe(originalContent); + }); - const sm = SessionManager.open(noHeaderFile, tempDir); + it("throws and preserves non-session JSONL files", () => { + const nonSessionFile = join(tempDir, "not-a-session.log"); + const originalContent = '{"type":"event","data":"not a session"}\n'; + writeFileSync(nonSessionFile, originalContent); - // Should have created a new session with valid header - expect(sm.getSessionId()).toBeTruthy(); - expect(sm.getHeader()).toBeTruthy(); - expect(sm.getHeader()?.type).toBe("session"); - - // File should now contain only a valid header (old content truncated) - const content = readFileSync(noHeaderFile, "utf-8"); - const lines = content.trim().split("\n").filter(Boolean); - expect(lines.length).toBe(1); - const header = JSON.parse(lines[0]); - expect(header.type).toBe("session"); - expect(header.id).toBe(sm.getSessionId()); + expect(() => SessionManager.open(nonSessionFile, tempDir)).toThrow( + `Session file is not a valid pi session and was not modified: ${nonSessionFile}`, + ); + expect(readFileSync(nonSessionFile, "utf-8")).toBe(originalContent); }); it("preserves explicit session file path when recovering from corrupted file", () => { @@ -302,16 +301,14 @@ describe("SessionManager.setSessionFile with corrupted files", () => { expect(sm.getSessionFile()).toBe(explicitPath); }); - it("subsequent loads of recovered file work correctly", () => { - const corruptedFile = join(tempDir, "corrupted.jsonl"); - writeFileSync(corruptedFile, "garbage content\n"); + it("subsequent loads of initialized empty file work correctly", () => { + const emptyFile = join(tempDir, "empty.jsonl"); + writeFileSync(emptyFile, ""); - // First open recovers the file - const sm1 = SessionManager.open(corruptedFile, tempDir); + const sm1 = SessionManager.open(emptyFile, tempDir); const sessionId = sm1.getSessionId(); - // Second open should load the recovered file successfully - const sm2 = SessionManager.open(corruptedFile, tempDir); + const sm2 = SessionManager.open(emptyFile, tempDir); expect(sm2.getSessionId()).toBe(sessionId); expect(sm2.getHeader()?.type).toBe("session"); });