From 2718dd337e26569dfefc43299ae30ae3d4a07b85 Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Mon, 31 Aug 2026 08:14:25 -0700 Subject: [PATCH] Do not overwrite unreadable run.json on rename The default resume picker is still running and cancelled only; document --force for failed and done. Silence tests pin a file log sink so a console leak fails. renameSession throws instead of clobbering corrupt state, and the TUI does not persist a snapshot after that failure. --- CHANGELOG.md | 8 +-- docs/PRODUCT.md | 5 +- src/config.test.ts | 61 ++++++++------------- src/session/index.ts | 5 +- src/session/list-sessions.test.ts | 49 +++++++---------- src/session/rename-session.test.ts | 87 ++++++++++++++++++++++++++++++ src/state.test.ts | 58 ++++++++------------ src/tui/runner.ts | 10 ++-- tests/helpers/file-log-sink.ts | 32 +++++++++++ 9 files changed, 203 insertions(+), 112 deletions(-) create mode 100644 src/session/rename-session.test.ts create mode 100644 tests/helpers/file-log-sink.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 28c277952..5861c63ed 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,9 +16,11 @@ parallel copies under `docs/` or `scripts/notes/`. At cut time: rename ### Fixed - Failed sessions with an `error` string in `run.json` are valid resume - candidates, not corrupt files. A truly unreadable session id prints one - recovery line; parse diagnostics go to the structured log, not the - terminal. + candidates, not corrupt files. The default picker still shows only + running and cancelled sessions; `--force` includes failed and done. A + truly unreadable session id prints one recovery line; parse diagnostics + go to the structured log, not the terminal. Renaming a session does not + overwrite an unreadable `run.json`. - `ask_operator` no longer pre-authorizes a model-authored shell command when the operator picks any option, including Reject. Clarification choices cannot mint shell grants. diff --git a/docs/PRODUCT.md b/docs/PRODUCT.md index 9d5e6a805..57fe2815b 100644 --- a/docs/PRODUCT.md +++ b/docs/PRODUCT.md @@ -81,8 +81,9 @@ Opens a picker of saved conversations for the working directory. Plain is the direct, explicit resume path. A session that ended in `failed` (including one that recorded an `error` -string in `run.json`) is a failed session, not a corrupt one — it still -appears in the picker. Passing a corrupt session id prints one short +string in `run.json`) is a failed session, not a corrupt one. The default +picker still shows only running and cancelled sessions; pass `--force` to +include failed and done. Passing a corrupt session id prints one short recovery line instead of dumping the file path and parse details. ## Safety Model diff --git a/src/config.test.ts b/src/config.test.ts index 4bbda8cce..633911bb3 100644 --- a/src/config.test.ts +++ b/src/config.test.ts @@ -31,6 +31,7 @@ import { generateSessionId, initSessionDir, sessionDir } from "./session/index.j import { saveState } from "./session/state.js"; import { filterMcpServersForConnect } from "./trust/project-trust.js"; import { createExaMCPServerConfig } from "./mcp/exa.js"; +import { withFileLogSink } from "../tests/helpers/file-log-sink.js"; const BUILTIN_EXA_MCP = createExaMCPServerConfig(); @@ -528,28 +529,18 @@ describe("loadConfig", () => { ); } - const chunks: string[] = []; - const orig = process.stderr.write.bind(process.stderr); - process.stderr.write = ((chunk: string | Uint8Array, ...rest: unknown[]) => { - chunks.push(typeof chunk === "string" ? chunk : Buffer.from(chunk).toString()); - return orig(chunk, ...(rest as [])); - }) as typeof process.stderr.write; let config: Awaited>; - try { + const logged = await withFileLogSink(async () => { config = await loadConfig(["resume", targetId, "--force", "--cwd", cwd], { globalSettingsPath: globalPath, home, }); - } finally { - process.stderr.write = orig; - } - assertConfigured(config); - expect(config.sessionId).toBe(targetId); - expect(config.task).toBe("target failed session"); - const text = chunks.join(""); - expect(text).not.toContain("ignoring unreadable"); - expect(text).not.toContain(home); - expect(text).not.toContain("invalid shape"); + }); + assertConfigured(config!); + expect(config!.sessionId).toBe(targetId); + expect(config!.task).toBe("target failed session"); + expect(logged).not.toContain("unreadable session state"); + expect(logged).not.toContain(home); } finally { await rm(cwd, { recursive: true, force: true }); await rm(home, { recursive: true, force: true }); @@ -654,25 +645,20 @@ describe("loadConfig", () => { const globalPath = await writeGlobalSettings(cwd); const sessionId = generateSessionId(); await initSessionDir(cwd, sessionId, home); - await writeFile(join(sessionDir(cwd, sessionId, home), "run.json"), "{ not json"); - - const chunks: string[] = []; - const orig = process.stderr.write.bind(process.stderr); - process.stderr.write = ((chunk: string | Uint8Array, ...rest: unknown[]) => { - chunks.push(typeof chunk === "string" ? chunk : Buffer.from(chunk).toString()); - return orig(chunk, ...(rest as [])); - }) as typeof process.stderr.write; + const runPath = join(sessionDir(cwd, sessionId, home), "run.json"); + await writeFile(runPath, "{ not json"); + let thrown: unknown; - try { - await loadConfig(["resume", sessionId, "--force", "--cwd", cwd], { - globalSettingsPath: globalPath, - home, - }); - } catch (err) { - thrown = err; - } finally { - process.stderr.write = orig; - } + const logged = await withFileLogSink(async () => { + try { + await loadConfig(["resume", sessionId, "--force", "--cwd", cwd], { + globalSettingsPath: globalPath, + home, + }); + } catch (err) { + thrown = err; + } + }); expect(thrown).toBeInstanceOf(CliUserError); const message = thrown instanceof Error ? thrown.message : String(thrown); @@ -687,9 +673,8 @@ describe("loadConfig", () => { if (thrown instanceof CliUserError) { expect(thrown.exitCode).toBe(1); } - const text = chunks.join(""); - expect(text).not.toContain("ignoring unreadable"); - expect(text).not.toContain(home); + expect(logged).toContain(runPath); + expect(logged).toContain("corrupt JSON"); } finally { await rm(cwd, { recursive: true, force: true }); await rm(home, { recursive: true, force: true }); diff --git a/src/session/index.ts b/src/session/index.ts index b07d8e316..3c2845cfe 100644 --- a/src/session/index.ts +++ b/src/session/index.ts @@ -293,7 +293,10 @@ export async function renameSession( } await migrateLegacySessionIfNeeded(cwd, sessionId, home); const existing = await loadState(cwd, sessionId, home); - if (existing.kind !== "ok") { + if (existing.kind === "unreadable") { + throw new Error("Session state is unreadable"); + } + if (existing.kind === "missing") { let startedAt = Date.now(); try { const dirStat = await stat(sessionDir(cwd, sessionId, home)); diff --git a/src/session/list-sessions.test.ts b/src/session/list-sessions.test.ts index be9e19a3d..7d412927f 100644 --- a/src/session/list-sessions.test.ts +++ b/src/session/list-sessions.test.ts @@ -4,6 +4,7 @@ import { join } from "node:path"; import { tmpdir } from "node:os"; import { generateSessionId, initSessionDir, listSessions, sessionDir } from "./index.js"; +import { withFileLogSink } from "../../tests/helpers/file-log-sink.js"; let cwd = ""; let home = ""; @@ -94,8 +95,10 @@ test("listSessions skips a session whose run.json is unreadable", async () => { const sessionId = generateSessionId(); await initSessionDir(cwd, sessionId, home); await writeFile(join(sessionDir(cwd, sessionId, home), "run.json"), "{ not json"); - const listed = await listSessions(cwd, home); - expect(listed.find((s) => s.sessionId === sessionId)).toBeUndefined(); + await withFileLogSink(async () => { + const listed = await listSessions(cwd, home); + expect(listed.find((s) => s.sessionId === sessionId)).toBeUndefined(); + }); }); test("listSessions stays silent when many sibling run.json files are unreadable", async () => { @@ -110,29 +113,25 @@ test("listSessions stays silent when many sibling run.json files are unreadable" startedAt: 1_700_000_000_000, }), ); + const unreadablePaths: string[] = []; for (let i = 0; i < 8; i++) { const id = generateSessionId(); await initSessionDir(cwd, id, home); - await writeFile(join(sessionDir(cwd, id, home), "run.json"), '{ "turnsUsed": '); + const runPath = join(sessionDir(cwd, id, home), "run.json"); + unreadablePaths.push(runPath); + await writeFile(runPath, '{ "turnsUsed": '); } - const chunks: string[] = []; - const orig = process.stderr.write.bind(process.stderr); - process.stderr.write = ((chunk: string | Uint8Array, ...rest: unknown[]) => { - chunks.push(typeof chunk === "string" ? chunk : Buffer.from(chunk).toString()); - return orig(chunk, ...(rest as [])); - }) as typeof process.stderr.write; let listed: Awaited> = []; - try { + const logged = await withFileLogSink(async () => { listed = await listSessions(cwd, home); - } finally { - process.stderr.write = orig; - } + }); expect(listed.map((s) => s.sessionId)).toEqual([validId]); - const text = chunks.join(""); - expect(text).not.toContain("ignoring unreadable"); - expect(text).not.toContain(home); + expect(logged).toContain("corrupt JSON"); + for (const runPath of unreadablePaths) { + expect(logged).toContain(runPath); + } }); test("listSessions includes a failed run that recorded an error", async () => { @@ -194,23 +193,13 @@ test("listSessions stays silent when many sibling runs failed with an error", as ); } - const chunks: string[] = []; - const orig = process.stderr.write.bind(process.stderr); - process.stderr.write = ((chunk: string | Uint8Array, ...rest: unknown[]) => { - chunks.push(typeof chunk === "string" ? chunk : Buffer.from(chunk).toString()); - return orig(chunk, ...(rest as [])); - }) as typeof process.stderr.write; let listed: Awaited> = []; - try { + const logged = await withFileLogSink(async () => { listed = await listSessions(cwd, home); - } finally { - process.stderr.write = orig; - } + }); expect(listed.map((s) => s.sessionId).sort()).toEqual([...ids].sort()); expect(listed.every((s) => s.status === "failed")).toBe(true); - const text = chunks.join(""); - expect(text).not.toContain("ignoring unreadable"); - expect(text).not.toContain(home); - expect(text).not.toContain("invalid shape"); + expect(logged).not.toContain("unreadable session state"); + expect(logged).not.toContain(home); }); diff --git a/src/session/rename-session.test.ts b/src/session/rename-session.test.ts new file mode 100644 index 000000000..9c8354424 --- /dev/null +++ b/src/session/rename-session.test.ts @@ -0,0 +1,87 @@ +import { afterEach, beforeEach, expect, test } from "bun:test"; +import { mkdir, readFile, rm, writeFile } from "node:fs/promises"; +import { join } from "node:path"; +import { tmpdir } from "node:os"; + +import { generateSessionId, initSessionDir, renameSession, sessionDir } from "./index.js"; +import { loadState } from "./state.js"; + +let cwd = ""; +let home = ""; + +beforeEach(async () => { + const stamp = `${Date.now()}-${Math.random().toString(16).slice(2)}`; + cwd = join(tmpdir(), `corbits-rename-session-${stamp}`); + home = join(tmpdir(), `corbits-rename-home-${stamp}`); + await mkdir(cwd, { recursive: true }); + await mkdir(home, { recursive: true }); +}); + +afterEach(async () => { + await rm(cwd, { recursive: true, force: true }); + await rm(home, { recursive: true, force: true }); +}); + +test("renameSession updates task on a readable run.json and preserves other fields", async () => { + const sessionId = generateSessionId(); + await initSessionDir(cwd, sessionId, home); + await writeFile( + join(sessionDir(cwd, sessionId, home), "run.json"), + JSON.stringify({ + status: "done", + turnsUsed: 4, + task: "old name", + startedAt: 1_700_000_000_000, + finishedAt: 1_700_000_100_000, + model: "provider:model", + }), + ); + + await renameSession(cwd, sessionId, "new name", home); + + const loaded = await loadState(cwd, sessionId, home); + expect(loaded).toEqual({ + kind: "ok", + state: { + status: "done", + turnsUsed: 4, + task: "new name", + startedAt: 1_700_000_000_000, + finishedAt: 1_700_000_100_000, + model: "provider:model", + }, + }); +}); + +test("renameSession creates a running record when run.json is missing", async () => { + const sessionId = generateSessionId(); + await initSessionDir(cwd, sessionId, home); + + await renameSession(cwd, sessionId, "named session", home); + + const loaded = await loadState(cwd, sessionId, home); + expect(loaded.kind).toBe("ok"); + if (loaded.kind !== "ok") return; + expect(loaded.state.status).toBe("running"); + expect(loaded.state.turnsUsed).toBe(0); + expect(loaded.state.task).toBe("named session"); + expect(loaded.state.startedAt).toBeGreaterThan(0); +}); + +test("renameSession throws on unreadable run.json and leaves the bytes unchanged", async () => { + const sessionId = generateSessionId(); + await initSessionDir(cwd, sessionId, home); + const path = join(sessionDir(cwd, sessionId, home), "run.json"); + const corrupt = "{ not json"; + await writeFile(path, corrupt); + + let thrown: unknown; + try { + await renameSession(cwd, sessionId, "should not land", home); + } catch (err) { + thrown = err; + } + expect(thrown).toBeInstanceOf(Error); + expect(thrown instanceof Error ? thrown.message : "").toBe("Session state is unreadable"); + expect(await readFile(path, "utf8")).toBe(corrupt); +}); diff --git a/src/state.test.ts b/src/state.test.ts index 822d4632e..24e5d329e 100644 --- a/src/state.test.ts +++ b/src/state.test.ts @@ -4,6 +4,7 @@ import { tmpdir } from "node:os"; import { join } from "node:path"; import { saveState, loadState, type RunState } from "./session/state.js"; import { sessionDir } from "./session/index.js"; +import { withFileLogSink } from "../tests/helpers/file-log-sink.js"; const SESSION_ID = "test-session-001"; @@ -89,22 +90,12 @@ describe("state persistence", () => { home, ); - const chunks: string[] = []; - const orig = process.stderr.write.bind(process.stderr); - process.stderr.write = ((chunk: string | Uint8Array, ...rest: unknown[]) => { - chunks.push(typeof chunk === "string" ? chunk : Buffer.from(chunk).toString()); - return orig(chunk, ...(rest as [])); - }) as typeof process.stderr.write; - try { + const logged = await withFileLogSink(async () => { expect(await loadState(cwd, SESSION_ID, home)).toEqual({ kind: "ok", state: failed }); expect((await loadState(cwd, crashedId, home)).kind).toBe("ok"); - } finally { - process.stderr.write = orig; - } - const text = chunks.join(""); - expect(text).not.toContain("ignoring unreadable"); - expect(text).not.toContain(home); - expect(text).not.toContain("invalid shape"); + }); + expect(logged).not.toContain("unreadable session state"); + expect(logged).not.toContain(home); }); test("saveState round-trips optional fields", async () => { @@ -137,31 +128,24 @@ describe("state persistence", () => { await mkdir(stateDir, { recursive: true }); await writeFile(join(stateDir, "run.json"), '{ "turnsUsed": '); - const result = await loadState(cwd, SESSION_ID, home); - expect(result).toEqual({ kind: "unreadable" }); + await withFileLogSink(async () => { + const result = await loadState(cwd, SESSION_ID, home); + expect(result).toEqual({ kind: "unreadable" }); + }); }); test("loadState does not print unreadable-state diagnostics to stderr", async () => { const stateDir = dir(); const { mkdir } = await import("node:fs/promises"); await mkdir(stateDir, { recursive: true }); - await writeFile(join(stateDir, "run.json"), '{ "turnsUsed": '); + const runPath = join(stateDir, "run.json"); + await writeFile(runPath, '{ "turnsUsed": '); - const chunks: string[] = []; - const orig = process.stderr.write.bind(process.stderr); - process.stderr.write = ((chunk: string | Uint8Array, ...rest: unknown[]) => { - chunks.push(typeof chunk === "string" ? chunk : Buffer.from(chunk).toString()); - return orig(chunk, ...(rest as [])); - }) as typeof process.stderr.write; - try { + const logged = await withFileLogSink(async () => { expect(await loadState(cwd, SESSION_ID, home)).toEqual({ kind: "unreadable" }); - } finally { - process.stderr.write = orig; - } - const text = chunks.join(""); - expect(text).not.toContain("ignoring unreadable"); - expect(text).not.toContain(home); - expect(text).not.toContain("invalid shape"); + }); + expect(logged).toContain(runPath); + expect(logged).toContain("corrupt JSON"); }); // --------------------------------------------------------------------------- @@ -177,8 +161,10 @@ describe("state persistence", () => { JSON.stringify({ status: "running", turnsUsed: "not-a-number", task: "x", startedAt: 0 }), ); - const result = await loadState(cwd, SESSION_ID, home); - expect(result).toEqual({ kind: "unreadable" }); + await withFileLogSink(async () => { + const result = await loadState(cwd, SESSION_ID, home); + expect(result).toEqual({ kind: "unreadable" }); + }); }); // --------------------------------------------------------------------------- @@ -249,7 +235,9 @@ describe("state persistence", () => { }), ); - const result = await loadState(cwd, SESSION_ID, home); - expect(result).toEqual({ kind: "unreadable" }); + await withFileLogSink(async () => { + const result = await loadState(cwd, SESSION_ID, home); + expect(result).toEqual({ kind: "unreadable" }); + }); }); }); diff --git a/src/tui/runner.ts b/src/tui/runner.ts index e09ab6af0..ae60a9cff 100644 --- a/src/tui/runner.ts +++ b/src/tui/runner.ts @@ -2098,9 +2098,13 @@ export async function runTUI(initialConfig: Config): Promise { if (trimmed.length === 0) return "Session name cannot be empty"; runTaskTitle = trimmed; emitter.emit("session.title", truncateSessionLabel(runTaskTitle)); - void renameSession(config.cwd, sessionId, trimmed).then(() => - persistRunSnapshot("running"), - ); + void renameSession(config.cwd, sessionId, trimmed) + .then(() => persistRunSnapshot("running")) + .catch((err: unknown) => { + tuiLogger.warn("rename session failed: {error}", { + error: err instanceof Error ? err.message : String(err), + }); + }); return undefined; }, submitFeedback: (text) => { diff --git a/tests/helpers/file-log-sink.ts b/tests/helpers/file-log-sink.ts new file mode 100644 index 000000000..00add2be6 --- /dev/null +++ b/tests/helpers/file-log-sink.ts @@ -0,0 +1,32 @@ +import { expect, spyOn } from "bun:test"; +import { existsSync, mkdtempSync, readFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; + +import { installFileLogSink } from "../../src/logging/sink.js"; + +/** + * Installs a temp-file log sink, spies stdout/stderr around `fn`, asserts + * those streams were unused, and returns the sink file contents. + * + * Leaves the sink installed (same as `src/logging/sink.test.ts`) and does + * not delete the temp dir — later tests in the same process may still log. + */ +export async function withFileLogSink(fn: () => Promise): Promise { + const dir = mkdtempSync(join(tmpdir(), "corbits-silence-log-")); + const file = join(dir, "corbits.log"); + installFileLogSink(file); + + const stdoutWrite = spyOn(process.stdout, "write"); + const stderrWrite = spyOn(process.stderr, "write"); + try { + await fn(); + expect(stdoutWrite).not.toHaveBeenCalled(); + expect(stderrWrite).not.toHaveBeenCalled(); + } finally { + stdoutWrite.mockRestore(); + stderrWrite.mockRestore(); + } + + return existsSync(file) ? readFileSync(file, "utf8") : ""; +}