diff --git a/CHANGELOG.md b/CHANGELOG.md index a1eef88c4..62e51c3fe 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,13 @@ parallel copies under `docs/` or `scripts/notes/`. At cut time: rename ## [Unreleased] +### 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. + ## [0.3.10] - 2026-08-30 ### Fixed diff --git a/docs/PRODUCT.md b/docs/PRODUCT.md index 7286e14f2..fb15a4528 100644 --- a/docs/PRODUCT.md +++ b/docs/PRODUCT.md @@ -80,6 +80,11 @@ Opens a picker of saved conversations for the working directory. Plain `corbits` always starts a fresh conversation; `corbits resume ` 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 +recovery line instead of dumping the file path and parse details. + ## Safety Model - **Tiered permission gate** — Read-only tools (`read_file`, `search_files`, `grep`, `list_dir`) run freely. Every consequential tool (`write_file`, `edit_file`, `run_shell`, …) is gated. The operator can Allow Once or Allow Always (scoped to a file, a directory, or a command shape); "Allow Always" choices persist per working directory so repeat actions don't interrupt flow. @@ -128,7 +133,9 @@ The exact turn thresholds are model-family-dependent (tighter for models with ob **What the user sees:** `Ctrl+C` mid-run, network error, or crash. The last state is persisted. -**Recovery:** `corbits resume` reloads `RunState` and continues. +**Recovery:** `corbits resume` reloads `RunState` and continues. Failed +sessions remain failed (still listed); a corrupt id gets a short recovery +line instead of a path dump. ## Configuration diff --git a/src/config.test.ts b/src/config.test.ts index a48173fbe..4bbda8cce 100644 --- a/src/config.test.ts +++ b/src/config.test.ts @@ -10,6 +10,7 @@ import { buildProviderCatalog, catalogEntryAsProviderSettings, CliHelpError, + CliUserError, CLI_HELP_TEXT, KEYLESS_API_KEY, loadConfig, @@ -26,7 +27,7 @@ import { type Settings, } from "./config/settings.js"; import { OPENCODE_GO_BASE_URL } from "../packages/opencode-go/src/index.js"; -import { generateSessionId, initSessionDir } from "./session/index.js"; +import { generateSessionId, initSessionDir, sessionDir } from "./session/index.js"; import { saveState } from "./session/state.js"; import { filterMcpServersForConnect } from "./trust/project-trust.js"; import { createExaMCPServerConfig } from "./mcp/exa.js"; @@ -467,6 +468,94 @@ describe("loadConfig", () => { } }); + test("resume --force reopens a failed session that recorded an error", async () => { + const cwd = await emptyCwd(); + const home = await mkdtemp(join(tmpdir(), "ic-resume-home-")); + try { + const globalPath = await writeGlobalSettings(cwd); + const sessionId = generateSessionId(); + await initSessionDir(cwd, sessionId, home); + await saveState( + cwd, + sessionId, + { + status: "failed", + turnsUsed: 4, + task: "ship resume after failure", + startedAt: Date.now() - 1_000, + finishedAt: Date.now(), + error: "Cycle commit failed\nhook dump: pre-commit rejected", + }, + home, + ); + const config = await loadConfig(["resume", sessionId, "--force", "--cwd", cwd], { + globalSettingsPath: globalPath, + home, + }); + assertConfigured(config); + expect(config.resumeMode).toBe("id"); + expect(config.sessionId).toBe(sessionId); + expect(config.skipInitialTask).toBe(true); + expect(config.task).toBe("ship resume after failure"); + expect(config.force).toBe(true); + } finally { + await rm(cwd, { recursive: true, force: true }); + await rm(home, { recursive: true, force: true }); + } + }); + + test("resume --force among failed siblings stays silent and reopens the target", async () => { + const cwd = await emptyCwd(); + const home = await mkdtemp(join(tmpdir(), "ic-resume-home-")); + try { + const globalPath = await writeGlobalSettings(cwd); + const targetId = generateSessionId(); + for (let i = 0; i < 6; i++) { + const id = i === 0 ? targetId : generateSessionId(); + await initSessionDir(cwd, id, home); + await saveState( + cwd, + id, + { + status: "failed", + turnsUsed: 2, + task: i === 0 ? "target failed session" : `sibling failed ${i}`, + startedAt: Date.now() - 1_000 - i, + finishedAt: Date.now() - i, + error: "Cycle commit failed\nhook dump: pre-commit rejected", + }, + 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; + let config: Awaited>; + try { + 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"); + } finally { + await rm(cwd, { recursive: true, force: true }); + await rm(home, { recursive: true, force: true }); + } + }); + test("--resume opens the picker", async () => { const cwd = await emptyCwd(); const home = await mkdtemp(join(tmpdir(), "ic-resume-home-")); @@ -558,6 +647,55 @@ describe("loadConfig", () => { } }); + test("resume of an unreadable session throws a short recovery line", async () => { + const cwd = await emptyCwd(); + const home = await mkdtemp(join(tmpdir(), "ic-resume-home-")); + try { + 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; + let thrown: unknown; + try { + await loadConfig(["resume", sessionId, "--force", "--cwd", cwd], { + globalSettingsPath: globalPath, + home, + }); + } catch (err) { + thrown = err; + } finally { + process.stderr.write = orig; + } + + expect(thrown).toBeInstanceOf(CliUserError); + const message = thrown instanceof Error ? thrown.message : String(thrown); + expect(message).toBe( + `Session ${sessionId} is unreadable. Use \`corbits resume\` to choose another.`, + ); + expect(message).not.toMatch(/No session/); + expect(message).not.toContain("ignoring unreadable"); + expect(message).not.toContain("invalid shape"); + expect(message).not.toContain(home); + expect(message.split("\n")).toHaveLength(1); + if (thrown instanceof CliUserError) { + expect(thrown.exitCode).toBe(1); + } + const text = chunks.join(""); + expect(text).not.toContain("ignoring unreadable"); + expect(text).not.toContain(home); + } finally { + await rm(cwd, { recursive: true, force: true }); + await rm(home, { recursive: true, force: true }); + } + }); + test("resume rejects a non-id positional instead of treating it as last", async () => { const cwd = await emptyCwd(); try { diff --git a/src/config/index.ts b/src/config/index.ts index 492cd3d83..cff1ebd77 100644 --- a/src/config/index.ts +++ b/src/config/index.ts @@ -3,6 +3,7 @@ import { resolve } from "node:path"; import type { InferenceSource } from "@intx/types/runtime"; import { generateSessionId, isSessionId, migrateLegacySessionIfNeeded } from "../session/index.js"; import { loadState } from "../session/state.js"; +import { COMMAND_NAME } from "../branding.js"; import { isDirectorId } from "../agent/directors/registry.js"; import { DIRECTOR_IDS, type DirectorId } from "../agent/directors/types.js"; @@ -525,6 +526,19 @@ export class CliHelpError extends Error { } } +/** + * Thrown for a recoverable operator mistake. Entry points must print + * `message` to stderr and exit 1 — not dump a stack. + */ +export class CliUserError extends Error { + readonly exitCode = 1 as const; + + constructor(message: string) { + super(message); + this.name = "CliUserError"; + } +} + export interface LoadConfigOptions { // Override the global settings file location (for tests / non-standard homes). globalSettingsPath?: string; @@ -838,12 +852,18 @@ export async function loadConfig( } else if (resumeMode === "id") { const id = resumeSessionId!; await migrateLegacySessionIfNeeded(cwd, id, options.home); - const state = await loadState(cwd, id, options.home); - if (state === null) { + const loaded = await loadState(cwd, id, options.home); + if (loaded.kind === "unreadable") { + throw new CliUserError( + `Session ${id} is unreadable. Use \`${COMMAND_NAME} resume\` to choose another.`, + ); + } + if (loaded.kind === "missing") { throw new Error( - `No session ${id} for this project. Sessions are stored under ~/.corbits/projects// (this checkout's git toplevel). Use \`corbits resume\` to choose one.`, + `No session ${id} for this project. Sessions are stored under ~/.corbits/projects// (this checkout's git toplevel). Use \`${COMMAND_NAME} resume\` to choose one.`, ); } + const state = loaded.state; sessionId = id; skipInitialTask = true; if (task.length === 0) resumeTask = state.task; diff --git a/src/index.ts b/src/index.ts index b34e8f10b..ce6dd5914 100644 --- a/src/index.ts +++ b/src/index.ts @@ -4,7 +4,7 @@ import { primeCrashReporting, writeCrashReport, type CrashKind } from "./crash/r import { getActiveRun, markCrashed } from "./session/active-run.js"; import { getActiveDisposeHost } from "./session/active-host.js"; import { saveCrashState } from "./session/state.js"; -import { loadConfig, CliHelpError } from "./config/index.js"; +import { loadConfig, CliHelpError, CliUserError } from "./config/index.js"; import { ensureTelemetrySettings, globalSettingsPath } from "./config/settings.js"; import { installFileLogSink } from "./logging/sink.js"; import { flushPerfToOtel } from "./perf/index.js"; @@ -286,6 +286,24 @@ export function installSignalHandlers(): void { } } +export function cliCaughtExit(err: unknown): { + stream: "stdout" | "stderr"; + text: string; + code: number; +} { + if (err instanceof CliHelpError) { + return { stream: "stdout", text: `${err.message}\n`, code: err.exitCode }; + } + if (err instanceof CliUserError) { + return { stream: "stderr", text: `${err.message}\n`, code: err.exitCode }; + } + return { + stream: "stderr", + text: `${err instanceof Error ? (err.stack ?? err.message) : String(err)}\n`, + code: 1, + }; +} + if (import.meta.main) { installCrashHandlers(); installSignalHandlers(); @@ -294,14 +312,10 @@ if (import.meta.main) { try { code = await main(process.argv.slice(2)); } catch (err: unknown) { - // Help is an intentional early exit, not a crash — stdout + 0. - if (err instanceof CliHelpError) { - process.stdout.write(`${err.message}\n`); - code = err.exitCode; - } else { - process.stderr.write(`${err instanceof Error ? (err.stack ?? err.message) : String(err)}\n`); - code = 1; - } + const exit = cliCaughtExit(err); + const dest = exit.stream === "stdout" ? process.stdout : process.stderr; + dest.write(exit.text); + code = exit.code; } process.exit(code); } diff --git a/src/session/index.ts b/src/session/index.ts index 099faa486..b07d8e316 100644 --- a/src/session/index.ts +++ b/src/session/index.ts @@ -240,20 +240,23 @@ export async function listSessions( const summaries: SessionSummary[] = []; for (const entry of entries) { await migrateLegacySessionIfNeeded(cwd, entry, home); - const state = await loadState(cwd, entry, home); - if (state !== null) { + const loaded = await loadState(cwd, entry, home); + if (loaded.kind === "ok") { summaries.push({ sessionId: entry, - task: state.task, - startedAt: state.startedAt, - status: state.status, + task: loaded.state.task, + startedAt: loaded.state.startedAt, + status: loaded.state.status, }); continue; } - // A session directory with context/ but no readable run.json never - // reached its first saveState call (see src/tui/runner.ts's early - // "running" write) and therefore isn't actually running: report it as - // crashed rather than fabricating liveness. + if (loaded.kind === "unreadable") { + continue; + } + // Missing run.json: a session directory with context/ never reached its + // first saveState call (see src/tui/runner.ts's early "running" write) + // and therefore isn't actually running: report it as crashed rather + // than fabricating liveness. try { const dirStat = await stat(sessionDir(cwd, entry, home)); await stat(sessionContextDir(cwd, entry, home)); @@ -290,7 +293,7 @@ export async function renameSession( } await migrateLegacySessionIfNeeded(cwd, sessionId, home); const existing = await loadState(cwd, sessionId, home); - if (existing === null) { + if (existing.kind !== "ok") { let startedAt = Date.now(); try { const dirStat = await stat(sessionDir(cwd, sessionId, home)); @@ -311,7 +314,7 @@ export async function renameSession( ); return; } - await saveState(cwd, sessionId, { ...existing, task: trimmed }, home); + await saveState(cwd, sessionId, { ...existing.state, task: trimmed }, home); } export { projectKeyFor, projectSessionsRoot, projectsRoot, projectRootFor } from "./project-key.js"; diff --git a/src/session/list-sessions.test.ts b/src/session/list-sessions.test.ts index 0aaa45f39..be9e19a3d 100644 --- a/src/session/list-sessions.test.ts +++ b/src/session/list-sessions.test.ts @@ -89,3 +89,128 @@ test("listSessions ignores a leftover goal.json from a pre-removal session", asy expect(row).toBeDefined(); expect(row?.task).toBe("pre-removal session"); }); + +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(); +}); + +test("listSessions stays silent when many sibling run.json files are unreadable", async () => { + const validId = generateSessionId(); + await initSessionDir(cwd, validId, home); + await writeFile( + join(sessionDir(cwd, validId, home), "run.json"), + JSON.stringify({ + status: "running", + turnsUsed: 1, + task: "keep me", + startedAt: 1_700_000_000_000, + }), + ); + 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 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 { + 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); +}); + +test("listSessions includes a failed run that recorded an error", async () => { + const sessionId = generateSessionId(); + await initSessionDir(cwd, sessionId, home); + await writeFile( + join(sessionDir(cwd, sessionId, home), "run.json"), + JSON.stringify({ + status: "failed", + turnsUsed: 2, + task: "failed work", + startedAt: 1_700_000_000_000, + finishedAt: 1_700_000_005_000, + error: "Cycle commit failed\nhook dump: pre-commit rejected", + }), + ); + const listed = await listSessions(cwd, home); + const row = listed.find((s) => s.sessionId === sessionId); + expect(row?.status).toBe("failed"); + expect(row?.task).toBe("failed work"); +}); + +test("listSessions includes a crashed run that recorded an error", async () => { + const sessionId = generateSessionId(); + await initSessionDir(cwd, sessionId, home); + await writeFile( + join(sessionDir(cwd, sessionId, home), "run.json"), + JSON.stringify({ + status: "crashed", + turnsUsed: 1, + task: "crashed work", + startedAt: 1_700_000_000_000, + finishedAt: 1_700_000_005_000, + error: "uncaughtException: boom", + }), + ); + const listed = await listSessions(cwd, home); + const row = listed.find((s) => s.sessionId === sessionId); + expect(row?.status).toBe("crashed"); + expect(row?.task).toBe("crashed work"); +}); + +test("listSessions stays silent when many sibling runs failed with an error", async () => { + const ids: string[] = []; + for (let i = 0; i < 8; i++) { + const id = generateSessionId(); + ids.push(id); + await initSessionDir(cwd, id, home); + await writeFile( + join(sessionDir(cwd, id, home), "run.json"), + JSON.stringify({ + status: "failed", + turnsUsed: 1, + task: `failed ${i}`, + startedAt: 1_700_000_000_000 + i, + finishedAt: 1_700_000_005_000 + i, + error: "Cycle commit failed\nhook dump", + }), + ); + } + + 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 { + 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"); +}); diff --git a/src/session/state.test.ts b/src/session/state.test.ts index cee7a89b2..b2b78af46 100644 --- a/src/session/state.test.ts +++ b/src/session/state.test.ts @@ -24,7 +24,7 @@ await withMockedModule( const { finalizeRunState, loadState, saveState } = await import("./state.js"); const { getActiveRun, setActiveRun } = await import("./active-run.js"); -type RunState = Awaited>; +type RunState = import("./state.js").RunState; let cwd = ""; let home = ""; @@ -42,7 +42,7 @@ afterEach(async () => { await rm(home, { recursive: true, force: true }); }); -function state(overrides: Partial>): NonNullable { +function state(overrides: Partial): RunState { return { status: "running", turnsUsed: 0, @@ -62,8 +62,7 @@ test("a straggler snapshot started before a terminal write does not overwrite it await Promise.all([straggler, terminal]); const final = await loadState(cwd, sessionId, home); - expect(final?.status).toBe("done"); - expect(final?.finishedAt).toBe(999); + expect(final).toMatchObject({ kind: "ok", state: { status: "done", finishedAt: 999 } }); }); test("a persisted terminal status agrees with the active-run handle without a second call site", async () => { @@ -73,7 +72,7 @@ test("a persisted terminal status agrees with the active-run handle without a se await finalizeRunState(cwd, sessionId, state({ status: "done", finishedAt: 1000 }), home); const persisted = await loadState(cwd, sessionId, home); - expect(persisted?.status).toBe("done"); + expect(persisted).toMatchObject({ kind: "ok", state: { status: "done" } }); // The only liveness representation left is presence in the active-run // slot -- a terminal RunState.status must leave nothing there to read. expect(getActiveRun()).toBeNull(); @@ -87,6 +86,6 @@ test("saveState calls for different sessions do not block each other", async () const a = await loadState(cwd, "session-a", home); const b = await loadState(cwd, "session-b", home); - expect(a?.task).toBe("a"); - expect(b?.task).toBe("b"); + expect(a).toMatchObject({ kind: "ok", state: { task: "a" } }); + expect(b).toMatchObject({ kind: "ok", state: { task: "b" } }); }); diff --git a/src/session/state.ts b/src/session/state.ts index 1ca0705e0..419095652 100644 --- a/src/session/state.ts +++ b/src/session/state.ts @@ -2,10 +2,13 @@ import { mkdir, writeFile, readFile, rename } from "node:fs/promises"; import { dirname, join } from "node:path"; import { type } from "arktype"; +import { getLogger } from "@intx/log"; import { sessionDir } from "./index.js"; import { clearActiveRun, getTestWriteGate, isCrashed } from "./active-run.js"; -import { COMMAND_NAME } from "../branding.js"; +import { LOG_NAMESPACE_ROOT } from "../branding.js"; + +const log = getLogger([LOG_NAMESPACE_ROOT, "session", "state"]); const ConnectedMcpServerSchema = type({ name: "string", @@ -49,14 +52,6 @@ export async function atomicWrite(path: string, content: string): Promise await rename(tmp, path); } -// A corrupt or shape-invalid state file means resume is silently starting over -// and prior progress is being discarded. Surface it rather than swallowing it. -export function warnUnreadableState(path: string, reason: string): void { - process.stderr.write( - `${COMMAND_NAME}: ignoring unreadable state at ${path} (${reason}); starting fresh\n`, - ); -} - // Concurrent saveState calls for the same session (a straggler progress // snapshot racing a terminal finalize write) have no ordering guarantee // between their underlying rename()s — the later call could still finish @@ -159,32 +154,41 @@ export async function saveCrashState( await atomicWrite(path, JSON.stringify(state, null, 2)); } -// Returns the parsed state, or the arktype error summary when the shape is -// invalid, so callers can surface a specific reason rather than "invalid shape". -function parseRunState(data: unknown): RunState | { error: string } { +type ParseRunStateResult = { ok: true; state: RunState } | { ok: false; reason: string }; + +// Tagged so a valid RunState.error string cannot be mistaken for a parse failure. +function parseRunState(data: unknown): ParseRunStateResult { const result = RunStateSchema(data); - return result instanceof type.errors ? { error: result.summary } : result; + return result instanceof type.errors + ? { ok: false, reason: result.summary } + : { ok: true, state: result }; } +export type LoadStateResult = + { kind: "ok"; state: RunState } | { kind: "missing" } | { kind: "unreadable" }; + export async function loadState( cwd: string, sessionId: string, home?: string, -): Promise { +): Promise { const path = statePath(cwd, sessionId, home); try { const raw = await readFile(path, "utf8"); const parsed = parseRunState(JSON.parse(raw)); - if ("error" in parsed) { - warnUnreadableState(path, `invalid shape: ${parsed.error}`); - return null; + if (!parsed.ok) { + log.warn("unreadable session state at {path}: {reason}", { + path, + reason: `invalid shape: ${parsed.reason}`, + }); + return { kind: "unreadable" }; } - return parsed; + return { kind: "ok", state: parsed.state }; } catch (err) { if (err instanceof SyntaxError) { - warnUnreadableState(path, "corrupt JSON"); - return null; + log.warn("unreadable session state at {path}: {reason}", { path, reason: "corrupt JSON" }); + return { kind: "unreadable" }; } if ( typeof err === "object" && @@ -192,7 +196,7 @@ export async function loadState( "code" in err && (err as { code?: unknown }).code === "ENOENT" ) { - return null; + return { kind: "missing" }; } throw err; } diff --git a/src/state.test.ts b/src/state.test.ts index 7df6e95dc..822d4632e 100644 --- a/src/state.test.ts +++ b/src/state.test.ts @@ -41,7 +41,70 @@ describe("state persistence", () => { test("saveState then loadState returns an equal RunState", async () => { await saveState(cwd, SESSION_ID, baseRunState, home); const loaded = await loadState(cwd, SESSION_ID, home); - expect(loaded).toEqual(baseRunState); + expect(loaded).toEqual({ kind: "ok", state: baseRunState }); + }); + + test("loadState returns a failed run that recorded an error string", async () => { + const state: RunState = { + ...baseRunState, + status: "failed", + finishedAt: 1_700_000_005_000, + error: "Cycle commit failed\nhook dump: pre-commit rejected", + }; + await saveState(cwd, SESSION_ID, state, home); + const loaded = await loadState(cwd, SESSION_ID, home); + expect(loaded).toEqual({ kind: "ok", state }); + }); + + test("loadState returns a crashed run that recorded an error string", async () => { + const state: RunState = { + ...baseRunState, + status: "crashed", + finishedAt: 1_700_000_005_000, + error: "uncaughtException: boom", + }; + await saveState(cwd, SESSION_ID, state, home); + const loaded = await loadState(cwd, SESSION_ID, home); + expect(loaded).toEqual({ kind: "ok", state }); + }); + + test("failed and crashed runs with error do not print diagnostics to stderr", async () => { + const failed: RunState = { + ...baseRunState, + status: "failed", + finishedAt: 1_700_000_005_000, + error: "Cycle commit failed\nhook dump: pre-commit rejected", + }; + await saveState(cwd, SESSION_ID, failed, home); + const crashedId = "test-session-crashed"; + await saveState( + cwd, + crashedId, + { + ...baseRunState, + status: "crashed", + finishedAt: 1_700_000_005_000, + error: "uncaughtException: boom", + }, + 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 { + 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"); }); test("saveState round-trips optional fields", async () => { @@ -52,37 +115,60 @@ describe("state persistence", () => { }; await saveState(cwd, SESSION_ID, state, home); const loaded = await loadState(cwd, SESSION_ID, home); - expect(loaded).toEqual(state); + expect(loaded).toEqual({ kind: "ok", state }); }); // --------------------------------------------------------------------------- - // 2. Missing file returns null (ENOENT mapped, no throw) + // 2. Missing file returns missing (ENOENT mapped, no throw) // --------------------------------------------------------------------------- - test("loadState on missing file returns null", async () => { + test("loadState on missing file returns missing", async () => { const result = await loadState(cwd, "nonexistent-session", home); - expect(result).toBeNull(); + expect(result).toEqual({ kind: "missing" }); }); // --------------------------------------------------------------------------- - // 3. Corrupt / truncated JSON returns null rather than throwing + // 3. Corrupt / truncated JSON returns unreadable rather than throwing // --------------------------------------------------------------------------- - test("loadState with truncated JSON returns null instead of throwing", async () => { + test("loadState with truncated JSON returns unreadable instead of throwing", async () => { const stateDir = dir(); const { mkdir } = await import("node:fs/promises"); await mkdir(stateDir, { recursive: true }); await writeFile(join(stateDir, "run.json"), '{ "turnsUsed": '); const result = await loadState(cwd, SESSION_ID, home); - expect(result).toBeNull(); + 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 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 { + 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"); }); // --------------------------------------------------------------------------- - // 4. Valid JSON but wrong shape returns null via the validators + // 4. Valid JSON but wrong shape returns unreadable via the validators // --------------------------------------------------------------------------- - test("loadState with turnsUsed as string returns null", async () => { + test("loadState with turnsUsed as string returns unreadable", async () => { const stateDir = dir(); const { mkdir } = await import("node:fs/promises"); await mkdir(stateDir, { recursive: true }); @@ -92,7 +178,7 @@ describe("state persistence", () => { ); const result = await loadState(cwd, SESSION_ID, home); - expect(result).toBeNull(); + expect(result).toEqual({ kind: "unreadable" }); }); // --------------------------------------------------------------------------- @@ -139,14 +225,13 @@ describe("state persistence", () => { }; await saveState(cwd, SESSION_ID, state, home); const loaded = await loadState(cwd, SESSION_ID, home); - expect(loaded).toEqual(state); + expect(loaded).toEqual({ kind: "ok", state }); }); test("loadState accepts a record with no model or mcpServers (pre-existing sessions)", async () => { await saveState(cwd, SESSION_ID, baseRunState, home); const loaded = await loadState(cwd, SESSION_ID, home); - expect(loaded?.model).toBeUndefined(); - expect(loaded?.mcpServers).toBeUndefined(); + expect(loaded).toEqual({ kind: "ok", state: baseRunState }); }); test("loadState rejects a mcpServers entry missing toolCount", async () => { @@ -165,6 +250,6 @@ describe("state persistence", () => { ); const result = await loadState(cwd, SESSION_ID, home); - expect(result).toBeNull(); + expect(result).toEqual({ kind: "unreadable" }); }); }); diff --git a/src/tui/run-snapshot-kind.test.ts b/src/tui/run-snapshot-kind.test.ts index d6abf5702..74645378c 100644 --- a/src/tui/run-snapshot-kind.test.ts +++ b/src/tui/run-snapshot-kind.test.ts @@ -59,7 +59,10 @@ describe("a snapshot write dispatched by kind", () => { await write("old", runState({ status: "done", finishedAt: 10 }), "session-rotation"); - expect((await loadState(cwd, "old", home))?.status).toBe("done"); + expect(await loadState(cwd, "old", home)).toMatchObject({ + kind: "ok", + state: { status: "done" }, + }); // The rotated-in session is repointed on the same handle, so the handle // must survive the write for the crash handler to have anything to close. expect(getActiveRun()).not.toBeNull(); @@ -70,7 +73,10 @@ describe("a snapshot write dispatched by kind", () => { await write("last", runState({ status: "done", finishedAt: 20 }), "run-end"); - expect((await loadState(cwd, "last", home))?.status).toBe("done"); + expect(await loadState(cwd, "last", home)).toMatchObject({ + kind: "ok", + state: { status: "done" }, + }); expect(getActiveRun()).toBeNull(); }); }); diff --git a/src/tui/runner.ts b/src/tui/runner.ts index b2f8178b0..a0933c609 100644 --- a/src/tui/runner.ts +++ b/src/tui/runner.ts @@ -671,7 +671,8 @@ export async function runTUI(initialConfig: Config): Promise { if (picked === null) return 0; sessionId = picked.sessionId; resumeSkipInitialTask = true; - const pickedState = await loadState(config.cwd, sessionId); + const loaded = await loadState(config.cwd, sessionId); + const pickedState = loaded.kind === "ok" ? loaded.state : null; resumeSeed = resolveResumeSeed(pickedState); if (pickedState !== null) { startedAt = pickedState.startedAt; diff --git a/src/workflows/state.ts b/src/workflows/state.ts index 25f85c14b..31ff84abc 100644 --- a/src/workflows/state.ts +++ b/src/workflows/state.ts @@ -1,10 +1,14 @@ import { readFile } from "node:fs/promises"; import { join } from "node:path"; +import { getLogger } from "@intx/log"; + import { sessionDir } from "../session/index.js"; -import { atomicWrite, warnUnreadableState } from "../session/state.js"; +import { atomicWrite } from "../session/state.js"; import type { StepStatus, WorkflowState } from "./types.js"; -import { COMMAND_NAME } from "../branding.js"; +import { COMMAND_NAME, LOG_NAMESPACE_ROOT } from "../branding.js"; + +const log = getLogger([LOG_NAMESPACE_ROOT, "workflows", "state"]); const STEP_STATUSES: StepStatus[] = ["pending", "active", "completed", "skipped"]; @@ -81,13 +85,13 @@ export async function loadWorkflowState( const raw = await readFile(path, "utf8"); const parsed = JSON.parse(raw); if (!isValidWorkflowState(parsed)) { - warnUnreadableState(path, "invalid shape"); + log.warn("unreadable workflow state at {path}: {reason}", { path, reason: "invalid shape" }); return null; } return parsed; } catch (err) { if (err instanceof SyntaxError) { - warnUnreadableState(path, "corrupt JSON"); + log.warn("unreadable workflow state at {path}: {reason}", { path, reason: "corrupt JSON" }); return null; } if ( diff --git a/tests/unit/index.test.ts b/tests/unit/index.test.ts index c9a0d8112..d600d6a0f 100644 --- a/tests/unit/index.test.ts +++ b/tests/unit/index.test.ts @@ -3,11 +3,12 @@ import { mkdtempSync, mkdirSync, rmSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; import type { Config } from "../../src/config/index.js"; +import { CliHelpError, CliUserError } from "../../src/config/index.js"; import { resetPricingMetadataRefreshForTests, schedulePricingMetadataRefresh, } from "../../src/cost/pricing-metadata.js"; -import { mainWithRunners } from "../../src/index.js"; +import { cliCaughtExit, mainWithRunners } from "../../src/index.js"; const envVars = { // Unit tests must never export telemetry or write an installationId into @@ -139,3 +140,32 @@ test("main launches exec for run alias", async () => { expect(cfg.task).toBe("do the thing"); }); }); + +test("CliUserError prints one line to stderr without a stack", () => { + const err = new CliUserError( + "Session abc is unreadable. Use `corbits resume` to choose another.", + ); + const exit = cliCaughtExit(err); + expect(exit.stream).toBe("stderr"); + expect(exit.code).toBe(1); + expect(exit.text).toBe(`${err.message}\n`); + expect(exit.text).not.toContain(" at "); + expect(exit.text.trimEnd().split("\n")).toHaveLength(1); +}); + +test("CliHelpError prints help to stdout and exits 0", () => { + const err = new CliHelpError("usage: corbits"); + const exit = cliCaughtExit(err); + expect(exit.stream).toBe("stdout"); + expect(exit.code).toBe(0); + expect(exit.text).toBe("usage: corbits\n"); +}); + +test("generic Error still dumps a stack to stderr", () => { + const err = new Error("No session abc for this project"); + const exit = cliCaughtExit(err); + expect(exit.stream).toBe("stderr"); + expect(exit.code).toBe(1); + expect(exit.text).toContain("No session abc for this project"); + expect(exit.text).toContain(" at "); +}); diff --git a/tests/unit/session/run-state-e2e.test.ts b/tests/unit/session/run-state-e2e.test.ts index 784bb627a..6d0a85122 100644 --- a/tests/unit/session/run-state-e2e.test.ts +++ b/tests/unit/session/run-state-e2e.test.ts @@ -78,7 +78,7 @@ describe("run.json turn-boundary snapshots — end to end", () => { expect(observed).toEqual([1, 2, 3, 4]); const onDisk = await loadState(cwd, sessionId, home); - expect(onDisk?.turnsUsed).toBe(4); + expect(onDisk).toMatchObject({ kind: "ok", state: { turnsUsed: 4 } }); runSink.sink({ type: "reactor.done", data: {} } as unknown as ReactorEmittedEvent); await saveState( @@ -88,8 +88,7 @@ describe("run.json turn-boundary snapshots — end to end", () => { home, ); const finalState = await loadState(cwd, sessionId, home); - expect(finalState?.status).toBe("done"); - expect(finalState?.turnsUsed).toBe(4); + expect(finalState).toMatchObject({ kind: "ok", state: { status: "done", turnsUsed: 4 } }); } finally { rmSync(cwd, { recursive: true, force: true }); rmSync(home, { recursive: true, force: true }); @@ -127,7 +126,7 @@ describe("run.json turn-boundary snapshots — end to end", () => { await Promise.all(writes); const finalState = await loadState(cwd, sessionId, home); - expect(finalState?.turnsUsed).toBe(20); + expect(finalState).toMatchObject({ kind: "ok", state: { turnsUsed: 20 } }); expect(runSink.getTurnCount()).toBe(20); } finally { rmSync(cwd, { recursive: true, force: true }); @@ -169,7 +168,7 @@ describe("run.json turn-boundary snapshots — end to end", () => { await Promise.all([runningWrite, doneWrite]); const finalState = await loadState(cwd, sessionId, home); - expect(finalState?.status).toBe("done"); + expect(finalState).toMatchObject({ kind: "ok", state: { status: "done" } }); } finally { rmSync(cwd, { recursive: true, force: true }); rmSync(home, { recursive: true, force: true });