Merge pull request #734 from corbitsdev/cl-7287-do-not-overwrite-unreadable-run-json

Do not overwrite unreadable run.json on rename

Author: TheGreatAxios

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.

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

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<ReturnType<typeof loadConfig>>;
-      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 });

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));

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<ReturnType<typeof listSessions>> = [];
-  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<ReturnType<typeof listSessions>> = [];
-  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);
 });

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);
+});