From db1320c45f3e3b844beb752ddee1391f60323b8a Mon Sep 17 00:00:00 2001 From: Takashi Matsuyama Date: Fri, 16 Oct 2026 07:45:40 +0900 Subject: [PATCH 1/2] basou: Keep the path out of decision capture's and review record's error for a --file they cannot read "Input file not found: " printed the path as given, which can be absolute, and any other read failure (no read permission, say) passed on Node's own message, path included. Both broke the CLI's pathless error contract. The read is now one function, readInputFileText in lib/input-file.ts, which note already used in effect: a fixed message naming the flag (does not exist / is a directory / could not be read), with the cause kept for --verbose. decision capture, review record and note share it. --- CHANGELOG.md | 7 ++++ packages/cli/src/commands/decision.test.ts | 30 +++++++++++---- packages/cli/src/commands/decision.ts | 12 +----- packages/cli/src/commands/note.ts | 18 +-------- packages/cli/src/commands/review.test.ts | 36 +++++++++++++++++- packages/cli/src/commands/review.ts | 12 +----- packages/cli/src/lib/input-file.test.ts | 43 ++++++++++++++++++++++ packages/cli/src/lib/input-file.ts | 23 ++++++++++++ 8 files changed, 137 insertions(+), 44 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2aa7141..794f1d8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -57,6 +57,13 @@ All notable changes to **basou** are recorded here. The project follows promises this (see Changed). A signal Node has no name for (`SIGEMT` on macOS, the real-time signals on Linux) still reaches basou as an exit code of `0`, so basou still exits `0` for it. +- **`basou decision capture` and `basou review record` no longer print the + path of a `--file` they cannot read.** `Input file not found: ` + carried the path as given, which can be absolute, and any other failure to + read it (a file without read permission, say) passed on Node's own message, + path included. They now say `--file names a file that does not exist`, + `--file names a directory, not a file` or `Could not read the file --file + names`, as `basou note` does, and `--verbose` adds the cause's code. ## 0.68.4 — 2026-10-15 diff --git a/packages/cli/src/commands/decision.test.ts b/packages/cli/src/commands/decision.test.ts index c1fd352..b604178 100644 --- a/packages/cli/src/commands/decision.test.ts +++ b/packages/cli/src/commands/decision.test.ts @@ -1,5 +1,6 @@ import { execFile } from "node:child_process"; import { + chmod, mkdir, mkdtemp, readdir, @@ -1333,15 +1334,30 @@ describe("doRunDecisionCapture (input validation, agent-facing errors)", () => { expect(process.exitCode).toBe(1); }); - it("cap-err-9: --file that does not exist is rejected without hanging", async () => { + it("cap-err-9: a --file that cannot be read is refused with a fixed message that carries no path", async () => { const repo = await setupInitedRepo(); + await mkdir(join(repo, "a-directory")); + await writeFile(join(repo, "locked.json"), "[]"); + await chmod(join(repo, "locked.json"), 0o000); + const cases: Array<[string, string]> = [ + [join(repo, "missing.json"), "--file names a file that does not exist"], + [join(repo, "a-directory"), "--file names a directory, not a file"], + ]; + // Root reads a 0o000 file anyway, so the permission case only means + // something for an ordinary user. + if (process.getuid?.() !== 0) { + cases.push([join(repo, "locked.json"), "Could not read the file --file names"]); + } const err = captureStderr(); - await runDecisionCapture( - { file: join(repo, "missing.json") }, - { cwd: repo, nowProvider: () => FIXED_NOW }, - ); - expect(joinCalls(err)).toContain("Input file not found:"); - expect(process.exitCode).toBe(1); + for (const [file, message] of cases) { + err.mockClear(); + process.exitCode = 0; + await runDecisionCapture({ file }, { cwd: repo, nowProvider: () => FIXED_NOW }); + expect(joinCalls(err), file).toBe(message); + expect(process.exitCode, file).toBe(1); + } + const dirs = await readdir(basouPaths(repo).sessions); + expect(dirs.filter((d) => d.startsWith("ses_"))).toEqual([]); }); it("cap-err-9b: --file - is refused as reserved for stdin, before the root lookup and whatever is on disk", async () => { diff --git a/packages/cli/src/commands/decision.ts b/packages/cli/src/commands/decision.ts index bb98dc7..0723049 100644 --- a/packages/cli/src/commands/decision.ts +++ b/packages/cli/src/commands/decision.ts @@ -1,4 +1,3 @@ -import { readFile } from "node:fs/promises"; import { homedir } from "node:os"; import { join, resolve } from "node:path"; import { @@ -32,7 +31,7 @@ import { renderCliError, shortSessionId, } from "../lib/error-render.js"; -import { refuseDashInputFile } from "../lib/input-file.js"; +import { readInputFileText, refuseDashInputFile } from "../lib/input-file.js"; import { resolveBasouRootForCommand } from "../lib/repo-root.js"; import { registerDecisionGapsCommand } from "./decision-gaps.js"; @@ -737,14 +736,7 @@ async function readCaptureInput( ctx: DecisionCaptureContext, ): Promise { if (options.file !== undefined) { - try { - return await readFile(options.file, "utf8"); - } catch (error: unknown) { - if (findErrorCode(error, "ENOENT")) { - throw new Error(`Input file not found: ${options.file}`); - } - throw error; - } + return await readInputFileText("--file", options.file); } if (ctx.readInput !== undefined) { return await ctx.readInput(); diff --git a/packages/cli/src/commands/note.ts b/packages/cli/src/commands/note.ts index e7ce38a..4533519 100644 --- a/packages/cli/src/commands/note.ts +++ b/packages/cli/src/commands/note.ts @@ -1,4 +1,3 @@ -import { readFile } from "node:fs/promises"; import { homedir } from "node:os"; import { resolve } from "node:path"; import { @@ -24,7 +23,7 @@ import { renderCliError, shortSessionId, } from "../lib/error-render.js"; -import { refuseDashInputFile } from "../lib/input-file.js"; +import { readInputFileText, refuseDashInputFile } from "../lib/input-file.js"; import { resolveBasouRootForCommand } from "../lib/repo-root.js"; // Single words that are almost certainly a mistyped subcommand rather than a @@ -317,20 +316,7 @@ async function readNoteBody( return { body: bodyArgument, source: "argument" }; } if (options.file !== undefined) { - let raw: string; - try { - raw = await readFile(resolve(cwd, options.file), "utf8"); - } catch (error: unknown) { - // Fixed messages, like `basou task new --from-file`: the error surface - // never carries a path. The cause (shown with --verbose) has the detail. - if (findErrorCode(error, "ENOENT")) { - throw new Error("--file names a file that does not exist", { cause: error }); - } - if (findErrorCode(error, "EISDIR")) { - throw new Error("--file names a directory, not a file", { cause: error }); - } - throw new Error("Could not read the file --file names", { cause: error }); - } + const raw = await readInputFileText("--file", resolve(cwd, options.file)); return { body: dropTrailingNewlines(raw), source: "file" }; } if (ctx.readInput !== undefined) { diff --git a/packages/cli/src/commands/review.test.ts b/packages/cli/src/commands/review.test.ts index 2611b3b..6d9e627 100644 --- a/packages/cli/src/commands/review.test.ts +++ b/packages/cli/src/commands/review.test.ts @@ -1,5 +1,15 @@ import { execFile } from "node:child_process"; -import { mkdtemp, readdir, readFile, realpath, rm, symlink, writeFile } from "node:fs/promises"; +import { + chmod, + mkdir, + mkdtemp, + readdir, + readFile, + realpath, + rm, + symlink, + writeFile, +} from "node:fs/promises"; import { devNull, tmpdir } from "node:os"; import { join } from "node:path"; import { promisify } from "node:util"; @@ -387,6 +397,30 @@ describe("doRunReviewRecord (validation errors surface to the agent)", () => { expect(readInput).not.toHaveBeenCalled(); }); + it("rev-err-6: a --file that cannot be read is refused with a fixed message that carries no path", async () => { + const repo = await setupInitedRepo(); + await mkdir(join(repo, "a-directory")); + await writeFile(join(repo, "locked.json"), "[]"); + await chmod(join(repo, "locked.json"), 0o000); + const cases: Array<[string, string]> = [ + [join(repo, "missing.json"), "--file names a file that does not exist"], + [join(repo, "a-directory"), "--file names a directory, not a file"], + ]; + // Root reads a 0o000 file anyway, so the permission case only means + // something for an ordinary user. + if (process.getuid?.() !== 0) { + cases.push([join(repo, "locked.json"), "Could not read the file --file names"]); + } + for (const [file, message] of cases) { + await expect( + doRunReviewRecord({ file }, { cwd: repo, nowProvider: () => FIXED_NOW }), + file, + ).rejects.toHaveProperty("message", message); + } + const dirs = (await readdir(basouPaths(repo).sessions)).filter((d) => d.startsWith("ses_")); + expect(dirs).toHaveLength(0); + }); + it("rev-err-5: a --file path that names a file called - is read", async () => { const repo = await setupInitedRepo(); captureStdout(); diff --git a/packages/cli/src/commands/review.ts b/packages/cli/src/commands/review.ts index 28b64d6..1b0c2d7 100644 --- a/packages/cli/src/commands/review.ts +++ b/packages/cli/src/commands/review.ts @@ -1,4 +1,3 @@ -import { readFile } from "node:fs/promises"; import { homedir } from "node:os"; import { resolve } from "node:path"; import { @@ -24,7 +23,7 @@ import { renderCliError, shortSessionId, } from "../lib/error-render.js"; -import { refuseDashInputFile } from "../lib/input-file.js"; +import { readInputFileText, refuseDashInputFile } from "../lib/input-file.js"; import { resolveBasouRootForCommand } from "../lib/repo-root.js"; /** @@ -260,14 +259,7 @@ async function readReviewInput( ctx: ReviewRecordContext, ): Promise { if (options.file !== undefined) { - try { - return await readFile(options.file, "utf8"); - } catch (error: unknown) { - if (findErrorCode(error, "ENOENT")) { - throw new Error(`Input file not found: ${options.file}`); - } - throw error; - } + return await readInputFileText("--file", options.file); } if (ctx.readInput !== undefined) { return await ctx.readInput(); diff --git a/packages/cli/src/lib/input-file.test.ts b/packages/cli/src/lib/input-file.test.ts index 26330a7..7463dcd 100644 --- a/packages/cli/src/lib/input-file.test.ts +++ b/packages/cli/src/lib/input-file.test.ts @@ -1,7 +1,11 @@ +import { chmod, mkdir, mkdtemp, rm, writeFile } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; import { describe, expect, it } from "vitest"; import { type InputFileAlternative, type InputFileFlag, + readInputFileText, refuseDashInputFile, } from "./input-file.js"; @@ -47,3 +51,42 @@ describe("refuseDashInputFile", () => { } }); }); + +describe("readInputFileText", () => { + it("reads the file, and refuses one it cannot read with a message that names the flag, not the path", async () => { + const dir = await mkdtemp(join(tmpdir(), "basou-input-file-")); + try { + await writeFile(join(dir, "in.json"), "[1]\n"); + await mkdir(join(dir, "a-directory")); + await writeFile(join(dir, "locked.json"), "[]"); + await chmod(join(dir, "locked.json"), 0o000); + expect(await readInputFileText("--file", join(dir, "in.json"))).toBe("[1]\n"); + const cases: Array<[InputFileFlag, string, string, string]> = [ + ["--file", "missing.json", "--file names a file that does not exist", "ENOENT"], + ["--from", "a-directory", "--from names a directory, not a file", "EISDIR"], + ]; + // Root reads a 0o000 file anyway. + if (process.getuid?.() !== 0) { + cases.push([ + "--from-file", + "locked.json", + "Could not read the file --from-file names", + "EACCES", + ]); + } + for (const [flag, name, message, code] of cases) { + const error = await readInputFileText(flag, join(dir, name)).then( + () => undefined, + (e: unknown) => e, + ); + expect(error, name).toBeInstanceOf(Error); + expect((error as Error).message, name).toBe(message); + expect((error as Error).message, name).not.toContain(dir); + expect(((error as Error).cause as NodeJS.ErrnoException).code, name).toBe(code); + } + } finally { + await chmod(join(dir, "locked.json"), 0o600).catch(() => undefined); + await rm(dir, { recursive: true, force: true }); + } + }); +}); diff --git a/packages/cli/src/lib/input-file.ts b/packages/cli/src/lib/input-file.ts index 0f61703..d4762a2 100644 --- a/packages/cli/src/lib/input-file.ts +++ b/packages/cli/src/lib/input-file.ts @@ -1,3 +1,6 @@ +import { readFile } from "node:fs/promises"; +import { findErrorCode } from "@basou/core"; + /** * The flags that name a file to read a command's input from: `--file` * (`basou decision capture`, `basou review record`, `basou note`), @@ -52,3 +55,23 @@ export function refuseDashInputFile( `${toFile}; a file named - is read with ${flag} ./-.`, ); } + +/** + * Read the text of the file an input-file flag names. A failure is a fixed + * message that names the flag and never the path, per the CLI's pathless error + * contract (a path the caller gave can be absolute); the cause, shown with + * `--verbose` as its code, has the detail. + */ +export async function readInputFileText(flag: InputFileFlag, path: string): Promise { + try { + return await readFile(path, "utf8"); + } catch (error: unknown) { + if (findErrorCode(error, "ENOENT")) { + throw new Error(`${flag} names a file that does not exist`, { cause: error }); + } + if (findErrorCode(error, "EISDIR")) { + throw new Error(`${flag} names a directory, not a file`, { cause: error }); + } + throw new Error(`Could not read the file ${flag} names`, { cause: error }); + } +} From 09ed93d4f681a9ef0fd43e165baf41032f8f9c02 Mon Sep 17 00:00:00 2001 From: Takashi Matsuyama Date: Fri, 16 Oct 2026 07:57:34 +0900 Subject: [PATCH 2/2] basou: Pin the --verbose cause line and the relative --file base for decision capture and review record The cause readInputFileText attaches must be an Error, as --verbose prints a code only from one; a test now checks that, and that decision capture with -v prints "Caused by: ENOENT" after the message. Tests also pin that a relative --file is read from the working directory, not the workspace root, for decision capture and review record. The CHANGELOG now says most other read failures named the path: a directory's EISDIR message never did. --- CHANGELOG.md | 6 ++-- packages/cli/src/commands/decision.test.ts | 41 ++++++++++++++++++++++ packages/cli/src/commands/review.test.ts | 18 ++++++++++ packages/cli/src/lib/input-file.test.ts | 3 ++ 4 files changed, 65 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 794f1d8..deae5d9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -59,9 +59,9 @@ All notable changes to **basou** are recorded here. The project follows of `0`, so basou still exits `0` for it. - **`basou decision capture` and `basou review record` no longer print the path of a `--file` they cannot read.** `Input file not found: ` - carried the path as given, which can be absolute, and any other failure to - read it (a file without read permission, say) passed on Node's own message, - path included. They now say `--file names a file that does not exist`, + carried the path as given, which can be absolute, and most other failures + to read it (a file without read permission, say) passed on Node's own + message, which names the path. They now say `--file names a file that does not exist`, `--file names a directory, not a file` or `Could not read the file --file names`, as `basou note` does, and `--verbose` adds the cause's code. diff --git a/packages/cli/src/commands/decision.test.ts b/packages/cli/src/commands/decision.test.ts index b604178..2e152f2 100644 --- a/packages/cli/src/commands/decision.test.ts +++ b/packages/cli/src/commands/decision.test.ts @@ -1360,6 +1360,47 @@ describe("doRunDecisionCapture (input validation, agent-facing errors)", () => { expect(dirs.filter((d) => d.startsWith("ses_"))).toEqual([]); }); + it("cap-err-9c: --verbose adds the code of the cause, still with no path", async () => { + const repo = await setupInitedRepo(); + const err = captureStderr(); + await runDecisionCapture( + { file: join(repo, "missing.json"), verbose: true }, + { cwd: repo, nowProvider: () => FIXED_NOW }, + ); + expect(joinCalls(err)).toBe("--file names a file that does not exist\nCaused by: ENOENT"); + expect(process.exitCode).toBe(1); + }); + + it("cap-err-9d: a relative --file is read from the working directory, not the workspace root", async () => { + const repo = await setupInitedRepo(); + const sub = join(repo, "sub"); + await mkdir(sub); + await writeFile( + join(sub, "rel.json"), + JSON.stringify([{ title: "from sub", kind: "decision" }]), + ); + await writeFile( + join(repo, "rel.json"), + JSON.stringify([{ title: "from root", kind: "decision" }]), + ); + captureStdout(); + const previous = process.cwd(); + process.chdir(sub); + try { + await doRunDecisionCapture({ file: "rel.json" }, { cwd: sub, nowProvider: () => FIXED_NOW }); + } finally { + process.chdir(previous); + } + const sid = await findAdHocSessionId(repo); + const events = (await readFile(join(basouPaths(repo).sessions, sid, "events.jsonl"), "utf8")) + .split("\n") + .filter((l) => l.length > 0) + .map((l) => JSON.parse(l) as Record); + expect(events.filter((e) => e.type === "decision_recorded").map((e) => e.title)).toEqual([ + "from sub", + ]); + }); + it("cap-err-9b: --file - is refused as reserved for stdin, before the root lookup and whatever is on disk", async () => { const input = JSON.stringify([{ title: "x", kind: "decision" }]); const readInput = vi.fn(async () => input); diff --git a/packages/cli/src/commands/review.test.ts b/packages/cli/src/commands/review.test.ts index 6d9e627..b8df71b 100644 --- a/packages/cli/src/commands/review.test.ts +++ b/packages/cli/src/commands/review.test.ts @@ -421,6 +421,24 @@ describe("doRunReviewRecord (validation errors surface to the agent)", () => { expect(dirs).toHaveLength(0); }); + it("rev-err-7: a relative --file is read from the working directory, not the workspace root", async () => { + const repo = await setupInitedRepo(); + const sub = join(repo, "sub"); + await mkdir(sub); + await writeFile(join(sub, "rev.json"), '{ "reviewer": "from-sub", "target": "wt" }'); + await writeFile(join(repo, "rev.json"), '{ "reviewer": "from-root", "target": "wt" }'); + captureStdout(); + const previous = process.cwd(); + process.chdir(sub); + try { + await doRunReviewRecord({ file: "rev.json" }, { cwd: sub, nowProvider: () => FIXED_NOW }); + } finally { + process.chdir(previous); + } + const reviews = (await readAdHocEvents(repo)).filter((e) => e.type === "review_recorded"); + expect(reviews.map((e) => e.reviewer)).toEqual(["from-sub"]); + }); + it("rev-err-5: a --file path that names a file called - is read", async () => { const repo = await setupInitedRepo(); captureStdout(); diff --git a/packages/cli/src/lib/input-file.test.ts b/packages/cli/src/lib/input-file.test.ts index 7463dcd..ea0fb3a 100644 --- a/packages/cli/src/lib/input-file.test.ts +++ b/packages/cli/src/lib/input-file.test.ts @@ -82,6 +82,9 @@ describe("readInputFileText", () => { expect(error, name).toBeInstanceOf(Error); expect((error as Error).message, name).toBe(message); expect((error as Error).message, name).not.toContain(dir); + // An Error, so that --verbose can print its code (renderCliError reads + // the code only off an Error). + expect((error as Error).cause, name).toBeInstanceOf(Error); expect(((error as Error).cause as NodeJS.ErrnoException).code, name).toBe(code); } } finally {