From c47cb54402b35f29d034c689bf489607d58bae00 Mon Sep 17 00:00:00 2001 From: Burak Yigit Kaya Date: Wed, 22 Apr 2026 13:57:33 +0000 Subject: [PATCH 1/6] refactor(fs): add safeReadFile helper for FIFO-safe config reads MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR #806 added an `isRegularFile()` guard in `src/lib/dsn/fs-utils.ts` and applied it to four DSN-related read sites to avoid hangs on FIFO-backed `.env` files (common 1Password setup). This extends the same protection to the remaining single-file reads under user-controlled paths: - `src/lib/sentryclirc.ts::tryApplyFile` — `.sentryclirc` walk-up - `src/lib/init/tools/read-files.ts` — LLM-driven project reads - `src/lib/init/tools/apply-patchset.ts` — patchset target reads - `src/lib/init/workflow-inputs.ts::preReadCommonFiles` — common config file pre-read ## Design New `src/lib/safe-read.ts` exposes `safeReadFile(path, operation)` as the higher-level wrapper. It chains `isRegularFile` (FIFO guard) with `Bun.file().text()` and returns `string | null`, routing any unexpected errors through the existing `handleFileError` → Sentry plumbing. Callers no longer need their own try/catch for the common case. `src/lib/dsn/fs-utils.ts` stays put — its four internal callers keep their imports, and `safe-read.ts` re-exports the two companion helpers so non-DSN code doesn't have to reach into the `dsn/` module. The two sites with a size-gated read (`read-files.ts` — partial read for large files, `workflow-inputs.ts` — skip >256 KB) keep their existing `fs.promises.stat` call but add an explicit `stat.isFile()` check before the read. This catches the FIFO case without a second stat. (Subagent audit surfaced a subtle bug here: the existing `stat.size` check didn't protect against FIFOs because `stat` returns successfully with `size=0` for a FIFO, then the read would still block.) ## Regression tests `test/lib/safe-read.test.ts` — 9 tests parallel to `dsn/fifo-safety.test.ts`: - `safeReadFile` on regular file, missing file, FIFO, symlink→FIFO, directory → returns expected value or `null` without hanging. - `sentryclirc` loader on a FIFO at `.sentryclirc` — returns empty config, no hang. - `readFiles` tool on a FIFO-backed path — returns `null` entry. - `applyPatchset` tool on a FIFO target — throws a targeted error. - `precomputeDirListing` on a FIFO-backed common-config file — no hang. Verified the tests catch real regressions by temporarily removing the `isRegularFile` guard and observing the test timeout after 5000 ms (Bun's default), confirming the hang is genuinely the condition we're exercising. ## Test plan - [x] `bunx tsc --noEmit` — clean - [x] `bun run lint` — clean (1 pre-existing markdown.ts warning) - [x] `bun test test/lib/safe-read.test.ts` — **9 pass** - [x] `bun test --timeout 15000 test/lib test/commands test/types` — **5685 pass, 0 fail** - [x] `bun test test/isolated` — 138 pass - [x] Negative test: removing the FIFO guard in `safeReadFile` causes `returns null for FIFOs instead of blocking` to time out (5s), confirming the guard is genuinely protective. --- src/lib/init/tools/apply-patchset.ts | 13 +- src/lib/init/tools/read-files.ts | 6 + src/lib/init/workflow-inputs.ts | 7 + src/lib/safe-read.ts | 52 +++++++ src/lib/sentryclirc.ts | 22 +-- test/lib/safe-read.test.ts | 214 +++++++++++++++++++++++++++ 6 files changed, 293 insertions(+), 21 deletions(-) create mode 100644 src/lib/safe-read.ts create mode 100644 test/lib/safe-read.test.ts diff --git a/src/lib/init/tools/apply-patchset.ts b/src/lib/init/tools/apply-patchset.ts index 346ad4e61..4e25bb995 100644 --- a/src/lib/init/tools/apply-patchset.ts +++ b/src/lib/init/tools/apply-patchset.ts @@ -1,5 +1,6 @@ import fs from "node:fs"; import path from "node:path"; +import { safeReadFile } from "../../safe-read.js"; import { replace } from "../replacers.js"; import type { ApplyPatchsetPatch, @@ -149,7 +150,17 @@ async function applyEdits( filePath: string, edits: Array<{ oldString: string; newString: string }> ): Promise { - let content = await fs.promises.readFile(absPath, "utf-8"); + const initialContent = await safeReadFile(absPath, "apply-patchset.read"); + if (initialContent === null) { + // Unreachable on the happy path — `applyPatchset` already + // `access()`-checked the file. Reachable when the file is a + // non-regular type (FIFO, socket, symlink → FIFO) that would + // otherwise hang `readFile` indefinitely. + throw new Error( + `Cannot read "${filePath}": not a regular file or read failed` + ); + } + let content = initialContent; for (let i = 0; i < edits.length; i += 1) { const edit = edits[i]; diff --git a/src/lib/init/tools/read-files.ts b/src/lib/init/tools/read-files.ts index b0ce88db6..a331f0cf5 100644 --- a/src/lib/init/tools/read-files.ts +++ b/src/lib/init/tools/read-files.ts @@ -36,6 +36,12 @@ async function readSingleFile( try { const absPath = safePath(cwd, filePath); const stat = await fs.promises.stat(absPath); + // Guard against FIFOs / sockets / devices — both `readFile` and + // `open("r")` block indefinitely on a FIFO waiting for a writer. + // `stat` follows symlinks, so symlink → FIFO is caught too. + if (!stat.isFile()) { + return null; + } if (stat.size <= maxBytes) { return await fs.promises.readFile(absPath, "utf-8"); } diff --git a/src/lib/init/workflow-inputs.ts b/src/lib/init/workflow-inputs.ts index dc78e6d90..4af83515a 100644 --- a/src/lib/init/workflow-inputs.ts +++ b/src/lib/init/workflow-inputs.ts @@ -121,6 +121,13 @@ export async function preReadCommonFiles( try { const absPath = path.join(directory, filePath); const stat = await fs.promises.stat(absPath); + // Guard against FIFOs / sockets / devices — `fs.readFile` on a + // FIFO blocks indefinitely waiting for a writer. `stat` follows + // symlinks, so a symlink → FIFO is also caught here. + if (!stat.isFile()) { + cache[filePath] = null; + continue; + } if (stat.size > MAX_FILE_BYTES) { continue; } diff --git a/src/lib/safe-read.ts b/src/lib/safe-read.ts new file mode 100644 index 000000000..f3a3ebe2f --- /dev/null +++ b/src/lib/safe-read.ts @@ -0,0 +1,52 @@ +/** + * FIFO-safe file-read helper for user-controlled paths. + * + * Named pipes (FIFOs), commonly created by 1Password's `.env` symlink + * integration, cause `Bun.file(path).text()` to block indefinitely + * waiting for a writer. Any read of a path under the user's project + * tree or home directory needs a `stat`-based regular-file check + * first. + * + * Prefer this helper over calling `isRegularFile` + `Bun.file().text()` + * by hand: a single call, consistent error handling, no way to forget + * the guard. + */ + +import { handleFileError, isRegularFile } from "./dsn/fs-utils.js"; + +/** + * Read a file's text content, returning `null` when the path is: + * - missing / inaccessible (ENOENT / EACCES / EPERM / EISDIR / ENOTDIR) + * - not a regular file (FIFO, socket, device, symlink to any of these) + * - any other expected I/O-level failure routed through + * {@link handleFileError} + * + * Unexpected errors are captured to Sentry via `handleFileError` and + * also return `null`, so callers don't need their own try/catch. + * + * @param filePath - Absolute path to read. + * @param operation - Logical operation name, reported to Sentry when a + * stat or read fails unexpectedly. Keep it short and specific (e.g., + * `"sentryclirc.read"`, `"read-files.tool"`). + */ +export async function safeReadFile( + filePath: string, + operation: string +): Promise { + if (!(await isRegularFile(filePath, `${operation}.stat`))) { + return null; + } + try { + return await Bun.file(filePath).text(); + } catch (error) { + handleFileError(error, { operation, path: filePath }); + return null; + } +} + +// Re-exports so call sites don't have to reach into `./dsn/fs-utils.js` +// for these companion helpers. `isRegularFile` is the low-level primitive +// that sites with non-standard read shapes (size-gating, partial reads) +// still need direct access to. +// biome-ignore lint/performance/noBarrelFile: intentional two-symbol re-export for a tiny, stable helper module +export { handleFileError, isRegularFile } from "./dsn/fs-utils.js"; diff --git a/src/lib/sentryclirc.ts b/src/lib/sentryclirc.ts index d54ca7a43..b26f7b0f1 100644 --- a/src/lib/sentryclirc.ts +++ b/src/lib/sentryclirc.ts @@ -22,6 +22,7 @@ import { getConfigDir } from "./db/index.js"; import { getEnv } from "./env.js"; import { parseIni } from "./ini.js"; import { logger } from "./logger.js"; +import { safeReadFile } from "./safe-read.js"; import { walkUpFrom } from "./walk-up.js"; const log = logger.withTag("sentryclirc"); @@ -57,25 +58,6 @@ export type SentryCliRcConfig = { */ const cache = new Map>(); -/** - * Read a file's text content, returning null for expected I/O errors. - * ENOENT (missing) and EACCES (permission denied) return null. - * All other errors propagate. - */ -async function tryReadFile(filePath: string): Promise { - try { - return await Bun.file(filePath).text(); - } catch (error: unknown) { - if (error instanceof Error && "code" in error) { - const { code } = error as NodeJS.ErrnoException; - if (code === "ENOENT" || code === "EACCES") { - return null; - } - } - throw error; - } -} - /** * Fields we extract from an INI config, keyed by section.field. * @@ -134,7 +116,7 @@ async function tryApplyFile( filePath: string, isGlobal: boolean ): Promise { - const content = await tryReadFile(filePath); + const content = await safeReadFile(filePath, "sentryclirc.read"); if (content !== null) { log.debug( `Found ${isGlobal ? "global" : "local"} ${CONFIG_FILENAME} at ${filePath}` diff --git a/test/lib/safe-read.test.ts b/test/lib/safe-read.test.ts new file mode 100644 index 000000000..b986db3b3 --- /dev/null +++ b/test/lib/safe-read.test.ts @@ -0,0 +1,214 @@ +/** + * FIFO-safety tests for the `safeReadFile` helper and the migrated + * call sites. Parallels `test/lib/dsn/fifo-safety.test.ts` (added by + * PR #806) — extends coverage to the non-DSN read paths addressed by + * Group D unification. + */ + +import { afterEach, beforeEach, describe, expect, test } from "bun:test"; +import { execSync } from "node:child_process"; +import { mkdirSync, rmSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { applyPatchset } from "../../src/lib/init/tools/apply-patchset.js"; +import { readFiles } from "../../src/lib/init/tools/read-files.js"; +import { precomputeDirListing } from "../../src/lib/init/workflow-inputs.js"; +import { safeReadFile } from "../../src/lib/safe-read.js"; +import { loadSentryCliRc } from "../../src/lib/sentryclirc.js"; + +/** Create a FIFO (named pipe) at the given path using mkfifo(1). */ +function createFifo(path: string): void { + execSync(`mkfifo ${JSON.stringify(path)}`); +} + +describe("safeReadFile", () => { + let dir: string; + + beforeEach(() => { + dir = join( + tmpdir(), + `safe-read-${Date.now()}-${Math.random().toString(36).slice(2)}` + ); + mkdirSync(dir, { recursive: true }); + }); + + afterEach(() => { + rmSync(dir, { recursive: true, force: true }); + }); + + test("reads regular file content", async () => { + const path = join(dir, "config.ini"); + writeFileSync(path, "[defaults]\norg=acme\n"); + expect(await safeReadFile(path, "test")).toBe("[defaults]\norg=acme\n"); + }); + + test("returns null for missing files", async () => { + expect(await safeReadFile(join(dir, "missing"), "test")).toBeNull(); + }); + + test("returns null for FIFOs instead of blocking", async () => { + const fifo = join(dir, "pipe.ini"); + createFifo(fifo); + // If the guard weren't in place this would hang indefinitely. + // Bun's test timeout would eventually fail the test, but we + // should get a clean `null` return almost immediately. + expect(await safeReadFile(fifo, "test")).toBeNull(); + }); + + test("returns null for symlinks to FIFOs (1Password pattern)", async () => { + const fifo = join(dir, "pipe"); + const link = join(dir, "linked.env"); + createFifo(fifo); + execSync(`ln -s ${JSON.stringify(fifo)} ${JSON.stringify(link)}`); + expect(await safeReadFile(link, "test")).toBeNull(); + }); + + test("returns null for directories", async () => { + const subdir = join(dir, "sub"); + mkdirSync(subdir); + expect(await safeReadFile(subdir, "test")).toBeNull(); + }); +}); + +describe("sentryclirc FIFO safety", () => { + let dir: string; + let originalHome: string | undefined; + let originalSentryConfigDir: string | undefined; + + beforeEach(() => { + dir = join( + tmpdir(), + `sentryclirc-fifo-${Date.now()}-${Math.random().toString(36).slice(2)}` + ); + mkdirSync(dir, { recursive: true }); + // Point both global fallback locations into `dir` so the loader + // can't reach the real user's `$HOME/.sentryclirc`. + originalHome = process.env.HOME; + originalSentryConfigDir = process.env.SENTRY_CONFIG_DIR; + process.env.HOME = dir; + process.env.SENTRY_CONFIG_DIR = dir; + }); + + afterEach(() => { + process.env.HOME = originalHome; + process.env.SENTRY_CONFIG_DIR = originalSentryConfigDir; + rmSync(dir, { recursive: true, force: true }); + }); + + test("skips a `.sentryclirc` FIFO in the project tree without hanging", async () => { + // Simulate a 1Password-managed `.sentryclirc` (unusual but + // possible). The walk-up should emit a null and move on — not + // hang on the FIFO read. + const cwd = join(dir, "project"); + mkdirSync(cwd); + createFifo(join(cwd, ".sentryclirc")); + + const config = await loadSentryCliRc(cwd); + // No config values resolved, but the function returned cleanly. + expect(config.org).toBeUndefined(); + expect(config.project).toBeUndefined(); + }); +}); + +describe("init read-files FIFO safety", () => { + let dir: string; + + beforeEach(() => { + dir = join( + tmpdir(), + `readfiles-fifo-${Date.now()}-${Math.random().toString(36).slice(2)}` + ); + mkdirSync(dir, { recursive: true }); + }); + + afterEach(() => { + rmSync(dir, { recursive: true, force: true }); + }); + + test("returns null entry for a FIFO path instead of hanging", async () => { + writeFileSync(join(dir, "real.ts"), "export {};\n"); + createFifo(join(dir, ".env")); + + const result = await readFiles({ + type: "tool", + operation: "read-files", + cwd: dir, + params: { paths: ["real.ts", ".env"] }, + }); + + expect(result.ok).toBe(true); + const files = (result.data as { files: Record }) + .files; + expect(files["real.ts"]).toBe("export {};\n"); + expect(files[".env"]).toBeNull(); + }); +}); + +describe("init apply-patchset FIFO safety", () => { + let dir: string; + + beforeEach(() => { + dir = join( + tmpdir(), + `applypatch-fifo-${Date.now()}-${Math.random().toString(36).slice(2)}` + ); + mkdirSync(dir, { recursive: true }); + }); + + afterEach(() => { + rmSync(dir, { recursive: true, force: true }); + }); + + test("throws targeted error for a FIFO target instead of hanging", async () => { + createFifo(join(dir, "config.ts")); + + await expect( + applyPatchset( + { + type: "tool", + operation: "apply-patchset", + cwd: dir, + params: { + patches: [ + { + action: "modify", + path: "config.ts", + edits: [{ oldString: "foo", newString: "bar" }], + }, + ], + }, + }, + { dryRun: false, authToken: undefined } + ) + ).rejects.toThrow(/not a regular file|read failed/); + }); +}); + +describe("workflow-inputs precomputeDirListing FIFO safety", () => { + let dir: string; + + beforeEach(() => { + dir = join( + tmpdir(), + `preread-fifo-${Date.now()}-${Math.random().toString(36).slice(2)}` + ); + mkdirSync(dir, { recursive: true }); + }); + + afterEach(() => { + rmSync(dir, { recursive: true, force: true }); + }); + + test("precomputeDirListing is unaffected by FIFOs in the tree", async () => { + // `precomputeDirListing` is the function that feeds + // `preReadCommonFiles` — calling it with a FIFO in place of + // a common-config file must not hang. + writeFileSync(join(dir, "package.json"), '{"name":"x"}'); + createFifo(join(dir, "pnpm-lock.yaml")); + + // No assertion about content — we just need the call to + // complete. A hang would blow the Bun test timeout. + const listing = await precomputeDirListing(dir); + expect(Array.isArray(listing)).toBe(true); + }); +}); From cf4a776f211117ae9c06df899a7dbc59edd96ad8 Mon Sep 17 00:00:00 2001 From: Burak Yigit Kaya Date: Wed, 22 Apr 2026 14:18:25 +0000 Subject: [PATCH 2/6] fix(safe-read): test preReadCommonFiles directly + correct stale comment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Self-review subagent caught two issues in the initial PR: 1. The `workflow-inputs` test was calling `precomputeDirListing` (a listing operation that never reads file contents) instead of `preReadCommonFiles` (the function that actually has the new `stat.isFile()` guard). Removing the guard didn't fail any test — the coverage hole meant a regression wouldn't have been caught. Replaced with a direct `preReadCommonFiles` call passing a synthetic `DirEntry[]`. Negative-verified: removing the guard now times out at 5000ms as expected. 2. The "Unreachable on happy path" comment in `apply-patchset.ts` incorrectly claimed `fs.promises.access()` rejects FIFOs. Verified: `access()` follows symlinks and succeeds on FIFOs, so the null-branch is the PRIMARY guard, not a redundant belt-and-suspenders check. Rewrote the comment to reflect reality. Also added symlink → FIFO coverage for `readFiles` and `applyPatchset` (the actual 1Password pattern) — previously only `safeReadFile` itself exercised it. ## Test counts - `test/lib/safe-read.test.ts`: **11 pass** (was 9; +2 symlink→FIFO) - Full suite: 5687 pass, 0 fail --- src/lib/init/tools/apply-patchset.ts | 11 +++-- test/lib/safe-read.test.ts | 74 +++++++++++++++++++++++----- 2 files changed, 70 insertions(+), 15 deletions(-) diff --git a/src/lib/init/tools/apply-patchset.ts b/src/lib/init/tools/apply-patchset.ts index 4e25bb995..ff37d3f1f 100644 --- a/src/lib/init/tools/apply-patchset.ts +++ b/src/lib/init/tools/apply-patchset.ts @@ -152,10 +152,13 @@ async function applyEdits( ): Promise { const initialContent = await safeReadFile(absPath, "apply-patchset.read"); if (initialContent === null) { - // Unreachable on the happy path — `applyPatchset` already - // `access()`-checked the file. Reachable when the file is a - // non-regular type (FIFO, socket, symlink → FIFO) that would - // otherwise hang `readFile` indefinitely. + // `applyPatchset`'s earlier `access()` call only verifies + // existence — it follows symlinks and succeeds on FIFOs/sockets, + // so this branch is the primary guard against non-regular files + // (FIFO, socket, symlink → FIFO) that would otherwise hang + // `readFile` indefinitely, plus any other expected I/O failure + // (permission, transient read error) routed through + // `safeReadFile`. throw new Error( `Cannot read "${filePath}": not a regular file or read failed` ); diff --git a/test/lib/safe-read.test.ts b/test/lib/safe-read.test.ts index b986db3b3..76236e8ca 100644 --- a/test/lib/safe-read.test.ts +++ b/test/lib/safe-read.test.ts @@ -12,7 +12,8 @@ import { tmpdir } from "node:os"; import { join } from "node:path"; import { applyPatchset } from "../../src/lib/init/tools/apply-patchset.js"; import { readFiles } from "../../src/lib/init/tools/read-files.js"; -import { precomputeDirListing } from "../../src/lib/init/workflow-inputs.js"; +import type { DirEntry } from "../../src/lib/init/types.js"; +import { preReadCommonFiles } from "../../src/lib/init/workflow-inputs.js"; import { safeReadFile } from "../../src/lib/safe-read.js"; import { loadSentryCliRc } from "../../src/lib/sentryclirc.js"; @@ -142,6 +143,28 @@ describe("init read-files FIFO safety", () => { expect(files["real.ts"]).toBe("export {};\n"); expect(files[".env"]).toBeNull(); }); + + test("returns null entry for a symlink to a FIFO (1Password pattern)", async () => { + // 1Password's `.env` integration uses a symlink → FIFO to stream + // secrets. `stat` follows the symlink so `isFile()` is false on + // the FIFO target, correctly rejected by the guard. + const fifo = join(dir, ".env-pipe"); + const link = join(dir, ".env"); + createFifo(fifo); + execSync(`ln -s ${JSON.stringify(fifo)} ${JSON.stringify(link)}`); + + const result = await readFiles({ + type: "tool", + operation: "read-files", + cwd: dir, + params: { paths: [".env"] }, + }); + + expect(result.ok).toBe(true); + const files = (result.data as { files: Record }) + .files; + expect(files[".env"]).toBeNull(); + }); }); describe("init apply-patchset FIFO safety", () => { @@ -182,9 +205,36 @@ describe("init apply-patchset FIFO safety", () => { ) ).rejects.toThrow(/not a regular file|read failed/); }); + + test("throws targeted error for a symlink to a FIFO", async () => { + const fifo = join(dir, "config.pipe"); + const link = join(dir, "config.ts"); + createFifo(fifo); + execSync(`ln -s ${JSON.stringify(fifo)} ${JSON.stringify(link)}`); + + await expect( + applyPatchset( + { + type: "tool", + operation: "apply-patchset", + cwd: dir, + params: { + patches: [ + { + action: "modify", + path: "config.ts", + edits: [{ oldString: "foo", newString: "bar" }], + }, + ], + }, + }, + { dryRun: false, authToken: undefined } + ) + ).rejects.toThrow(/not a regular file|read failed/); + }); }); -describe("workflow-inputs precomputeDirListing FIFO safety", () => { +describe("workflow-inputs preReadCommonFiles FIFO safety", () => { let dir: string; beforeEach(() => { @@ -199,16 +249,18 @@ describe("workflow-inputs precomputeDirListing FIFO safety", () => { rmSync(dir, { recursive: true, force: true }); }); - test("precomputeDirListing is unaffected by FIFOs in the tree", async () => { - // `precomputeDirListing` is the function that feeds - // `preReadCommonFiles` — calling it with a FIFO in place of - // a common-config file must not hang. + test("returns null entry for a FIFO-backed common-config file", async () => { writeFileSync(join(dir, "package.json"), '{"name":"x"}'); - createFifo(join(dir, "pnpm-lock.yaml")); + // `tsconfig.json` is in `COMMON_CONFIG_FILES` but exists here as + // a FIFO. Without the `stat.isFile()` guard the read would hang. + createFifo(join(dir, "tsconfig.json")); - // No assertion about content — we just need the call to - // complete. A hang would blow the Bun test timeout. - const listing = await precomputeDirListing(dir); - expect(Array.isArray(listing)).toBe(true); + const listing: DirEntry[] = [ + { name: "package.json", path: "package.json", type: "file" }, + { name: "tsconfig.json", path: "tsconfig.json", type: "file" }, + ]; + const cache = await preReadCommonFiles(dir, listing); + expect(cache["package.json"]).toBe('{"name":"x"}'); + expect(cache["tsconfig.json"]).toBeNull(); }); }); From 6eed61356eeb65a354a3ee01788f2937d0977534 Mon Sep 17 00:00:00 2001 From: Burak Yigit Kaya Date: Wed, 22 Apr 2026 14:37:34 +0000 Subject: [PATCH 3/6] fix(sentryclirc): preserve narrow EPERM-etc. re-throw behavior MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Seer flagged (HIGH): the migration from `tryReadFile` to `safeReadFile` silently swallowed EPERM/EISDIR/EIO errors on `.sentryclirc` reads, turning "your config file has broken permissions" into confusing downstream auth failures instead of a clear error. The DSN scanner's broader `isIgnorableFileError` set (ENOENT, EACCES, EPERM, EISDIR, ENOTDIR) is correct for opportunistic best-effort reads — we're GUESSING a DSN might be there, so swallowing all expected errors is fine. But `.sentryclirc` is a committed config load: if a user has `chmod 000` on it or the disk is throwing EIO, they need to see that, not get mysterious "no auth token" messages downstream. Fix: reverted `sentryclirc.ts` to its original narrow-error `tryReadFile` policy (null on ENOENT/EACCES, re-throw everything else) and composed it with an `isRegularFile` FIFO guard up front. Renamed to `tryReadSentryCliRc` to reflect the narrower policy. The new docstring spells out the trade-off for future maintainers. `safeReadFile` itself stays as-is — its broad error-swallowing is correct for its remaining caller (`apply-patchset.ts`, where the null branch throws a generic error anyway). ## Also in this commit (Cursor finding, Low) The `sentryclirc` FIFO test didn't call `clearSentryCliRcCache()` in its `beforeEach`/`afterEach`, leaving the module-level `globalPaths` singleton pointing at a now-deleted temp dir for any tests that run later. Added the standard clear-before-and-after pattern from `sentryclirc.property.test.ts`. ## Negative verification Confirmed EACCES path works: `chmod 000` on a `.sentryclirc` returns empty config cleanly. EPERM isn't easy to trigger portably but the code path is clear — only ENOENT/EACCES → null, all else throws. ## Test plan - [x] `bunx tsc --noEmit` — clean - [x] `bun run lint` — clean - [x] `bun test test/lib/safe-read.test.ts test/lib/sentryclirc.test.ts` — 36 pass, 0 fail - [x] Full suite: 5687 pass, 0 fail --- src/lib/sentryclirc.ts | 34 ++++++++++++++++++++++++++++++++-- test/lib/safe-read.test.ts | 11 ++++++++++- 2 files changed, 42 insertions(+), 3 deletions(-) diff --git a/src/lib/sentryclirc.ts b/src/lib/sentryclirc.ts index b26f7b0f1..39c462b88 100644 --- a/src/lib/sentryclirc.ts +++ b/src/lib/sentryclirc.ts @@ -22,7 +22,7 @@ import { getConfigDir } from "./db/index.js"; import { getEnv } from "./env.js"; import { parseIni } from "./ini.js"; import { logger } from "./logger.js"; -import { safeReadFile } from "./safe-read.js"; +import { isRegularFile } from "./safe-read.js"; import { walkUpFrom } from "./walk-up.js"; const log = logger.withTag("sentryclirc"); @@ -107,6 +107,36 @@ function isComplete(result: SentryCliRcConfig): boolean { ); } +/** + * Read a `.sentryclirc` file's text content. Returns `null` when the + * file is absent (ENOENT), unreadable for standard reasons (EACCES), + * or is a non-regular file (FIFO / socket / symlink → FIFO — the + * 1Password pattern). + * + * Unlike {@link safeReadFile}, this deliberately re-throws other I/O + * errors (EPERM, EISDIR, EIO, etc.). A `.sentryclirc` is a committed + * config file — failing to read it with an unusual error is a + * genuine problem the user should see, not silently swallow (which + * would surface downstream as confusing "no auth token" or "no + * project" errors). + */ +async function tryReadSentryCliRc(filePath: string): Promise { + if (!(await isRegularFile(filePath, "sentryclirc.stat"))) { + return null; + } + try { + return await Bun.file(filePath).text(); + } catch (error: unknown) { + if (error instanceof Error && "code" in error) { + const { code } = error as NodeJS.ErrnoException; + if (code === "ENOENT" || code === "EACCES") { + return null; + } + } + throw error; + } +} + /** * Try to read and apply a `.sentryclirc` file to the result. * No-op if the file doesn't exist or can't be read. @@ -116,7 +146,7 @@ async function tryApplyFile( filePath: string, isGlobal: boolean ): Promise { - const content = await safeReadFile(filePath, "sentryclirc.read"); + const content = await tryReadSentryCliRc(filePath); if (content !== null) { log.debug( `Found ${isGlobal ? "global" : "local"} ${CONFIG_FILENAME} at ${filePath}` diff --git a/test/lib/safe-read.test.ts b/test/lib/safe-read.test.ts index 76236e8ca..78f5cdcaa 100644 --- a/test/lib/safe-read.test.ts +++ b/test/lib/safe-read.test.ts @@ -15,7 +15,10 @@ import { readFiles } from "../../src/lib/init/tools/read-files.js"; import type { DirEntry } from "../../src/lib/init/types.js"; import { preReadCommonFiles } from "../../src/lib/init/workflow-inputs.js"; import { safeReadFile } from "../../src/lib/safe-read.js"; -import { loadSentryCliRc } from "../../src/lib/sentryclirc.js"; +import { + clearSentryCliRcCache, + loadSentryCliRc, +} from "../../src/lib/sentryclirc.js"; /** Create a FIFO (named pipe) at the given path using mkfifo(1). */ function createFifo(path: string): void { @@ -77,6 +80,9 @@ describe("sentryclirc FIFO safety", () => { let originalSentryConfigDir: string | undefined; beforeEach(() => { + // Clear both the load cache AND the cached global-paths singleton + // so this describe block sees the overridden env vars below. + clearSentryCliRcCache(); dir = join( tmpdir(), `sentryclirc-fifo-${Date.now()}-${Math.random().toString(36).slice(2)}` @@ -94,6 +100,9 @@ describe("sentryclirc FIFO safety", () => { process.env.HOME = originalHome; process.env.SENTRY_CONFIG_DIR = originalSentryConfigDir; rmSync(dir, { recursive: true, force: true }); + // Reset again so later test files don't inherit stale globalPaths + // pointing at the now-deleted temp dir. + clearSentryCliRcCache(); }); test("skips a `.sentryclirc` FIFO in the project tree without hanging", async () => { From 017a265ad0b4b610ae73dbe99eb0fe6afb6883a5 Mon Sep 17 00:00:00 2001 From: Burak Yigit Kaya Date: Wed, 22 Apr 2026 14:54:12 +0000 Subject: [PATCH 4/6] fix(sentryclirc): stat directly so EPERM/EIO/EISDIR surface properly MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both Seer and Cursor caught a bug in the previous commit's attempted fix: `isRegularFile` from `safe-read.ts` internally calls `handleFileError`, which treats EPERM/EISDIR/ENOTDIR as ignorable errors. So the stat-phase swallowed the very errors the narrow `try/catch` was supposed to preserve — the commit's own docstring was lying. Fix: sentryclirc now calls `fs.promises.stat` directly and wraps it with the same narrow-error policy as the subsequent `Bun.file().text()`. Only ENOENT and EACCES return `null`; everything else (EPERM, EISDIR, EIO, ENOTDIR, etc.) re-throws. The duplicated `try/catch` shape was hitting Biome's cognitive complexity ceiling at 17; extracted the common check into `isNarrowAbsenceError` which keeps each error arm a single `if`-statement. ## Test plan - [x] `bunx tsc --noEmit` — clean - [x] `bun run lint` — clean - [x] `bun test test/lib/safe-read.test.ts test/lib/sentryclirc.test.ts` — 36 pass, 0 fail - [x] Full suite: 5687 pass, 0 fail - [x] Code inspection: `stat`'s `catch` only returns null for ENOENT/EACCES; all other codes throw. Same policy on the subsequent `text()` read. `isRegularFile` is no longer used here, so its broader `handleFileError`-based swallowing can no longer intercept. --- src/lib/sentryclirc.ts | 67 ++++++++++++++++++++++++++++++------------ 1 file changed, 49 insertions(+), 18 deletions(-) diff --git a/src/lib/sentryclirc.ts b/src/lib/sentryclirc.ts index 39c462b88..ad9858864 100644 --- a/src/lib/sentryclirc.ts +++ b/src/lib/sentryclirc.ts @@ -16,13 +16,13 @@ * `resolve-target.ts`. */ +import { stat } from "node:fs/promises"; import { homedir } from "node:os"; import { join } from "node:path"; import { getConfigDir } from "./db/index.js"; import { getEnv } from "./env.js"; import { parseIni } from "./ini.js"; import { logger } from "./logger.js"; -import { isRegularFile } from "./safe-read.js"; import { walkUpFrom } from "./walk-up.js"; const log = logger.withTag("sentryclirc"); @@ -108,30 +108,61 @@ function isComplete(result: SentryCliRcConfig): boolean { } /** - * Read a `.sentryclirc` file's text content. Returns `null` when the - * file is absent (ENOENT), unreadable for standard reasons (EACCES), - * or is a non-regular file (FIFO / socket / symlink → FIFO — the - * 1Password pattern). + * Read a `.sentryclirc` file's text content. Returns `null` when + * the file: * - * Unlike {@link safeReadFile}, this deliberately re-throws other I/O - * errors (EPERM, EISDIR, EIO, etc.). A `.sentryclirc` is a committed - * config file — failing to read it with an unusual error is a - * genuine problem the user should see, not silently swallow (which - * would surface downstream as confusing "no auth token" or "no - * project" errors). + * - is absent (ENOENT) + * - is unreadable for a common reason (EACCES) + * - is a non-regular file (FIFO / socket / symlink → FIFO — the + * 1Password pattern, which would otherwise block `text()` + * indefinitely) + * + * Re-throws every other I/O error (EPERM, EISDIR, EIO, + * ENOTDIR, etc.). A `.sentryclirc` is a committed config file — a + * user with `chmod 000` at the directory level (EPERM), a typo + * pointing at a directory (EISDIR), or a disk throwing EIO needs + * to see that clearly, not have it silently suppressed and surface + * downstream as a confusing "no auth token" error. + * + * Note: cannot use {@link safeReadFile} / `isRegularFile` from + * `safe-read.ts` — their `handleFileError` treats EPERM and + * EISDIR as ignorable, swallowing them during the stat phase. + * That broader policy is correct for opportunistic DSN scans; not + * for this committed config load. + */ +/** + * True for the two I/O errors we treat as "file effectively absent" + * — a missing path and a permission-denied read. Any other error + * code signals something worth surfacing to the user. */ +function isNarrowAbsenceError(error: unknown): boolean { + if (error instanceof Error && "code" in error) { + const { code } = error as NodeJS.ErrnoException; + return code === "ENOENT" || code === "EACCES"; + } + return false; +} + async function tryReadSentryCliRc(filePath: string): Promise { - if (!(await isRegularFile(filePath, "sentryclirc.stat"))) { + let statResult: Awaited>; + try { + statResult = await stat(filePath); + } catch (error) { + if (isNarrowAbsenceError(error)) { + return null; + } + throw error; + } + // Non-regular file (FIFO, socket, device, symlink → any of these) + // — `text()` would block. Return null so the walk-up moves on. + if (!statResult.isFile()) { return null; } try { return await Bun.file(filePath).text(); - } catch (error: unknown) { - if (error instanceof Error && "code" in error) { - const { code } = error as NodeJS.ErrnoException; - if (code === "ENOENT" || code === "EACCES") { - return null; - } + } catch (error) { + if (isNarrowAbsenceError(error)) { + return null; } throw error; } From 313ef62ddde8dacea212b2605bd8285595392409 Mon Sep 17 00:00:00 2001 From: Burak Yigit Kaya Date: Wed, 22 Apr 2026 15:10:25 +0000 Subject: [PATCH 5/6] chore(safe-read): drop unused re-exports Cursor flagged (Low): since the earlier commit switched sentryclirc back to calling `fs.promises.stat` directly, `safe-read.ts`'s re-exports of `handleFileError` / `isRegularFile` have zero consumers. The re-export line also required a `biome-ignore noBarrelFile` suppression. Removed both. `safe-read.ts` now has exactly one export (`safeReadFile`), no suppression. --- src/lib/safe-read.ts | 7 ------- 1 file changed, 7 deletions(-) diff --git a/src/lib/safe-read.ts b/src/lib/safe-read.ts index f3a3ebe2f..d8a62cb57 100644 --- a/src/lib/safe-read.ts +++ b/src/lib/safe-read.ts @@ -43,10 +43,3 @@ export async function safeReadFile( return null; } } - -// Re-exports so call sites don't have to reach into `./dsn/fs-utils.js` -// for these companion helpers. `isRegularFile` is the low-level primitive -// that sites with non-standard read shapes (size-gating, partial reads) -// still need direct access to. -// biome-ignore lint/performance/noBarrelFile: intentional two-symbol re-export for a tiny, stable helper module -export { handleFileError, isRegularFile } from "./dsn/fs-utils.js"; From 7c3a523ce99f9d22c5fff6da033adf1fac1ec225 Mon Sep 17 00:00:00 2001 From: Burak Yigit Kaya Date: Wed, 22 Apr 2026 15:24:27 +0000 Subject: [PATCH 6/6] chore(sentryclirc): swap helper order so JSDoc is attached correctly Cursor flagged (Low): the large JSDoc describing the read function's narrow error policy had been orphaned between `isNarrowAbsenceError` and `tryReadSentryCliRc` because I added the helper above the function without moving the doc with it. TypeScript/IDEs would associate the doc with nothing useful. Swapped: helper (with its own doc) first, then the main function's doc directly above the function it describes. --- src/lib/sentryclirc.ts | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/src/lib/sentryclirc.ts b/src/lib/sentryclirc.ts index ad9858864..2d9f37046 100644 --- a/src/lib/sentryclirc.ts +++ b/src/lib/sentryclirc.ts @@ -107,6 +107,19 @@ function isComplete(result: SentryCliRcConfig): boolean { ); } +/** + * True for the two I/O errors we treat as "file effectively absent" + * — a missing path and a permission-denied read. Any other error + * code signals something worth surfacing to the user. + */ +function isNarrowAbsenceError(error: unknown): boolean { + if (error instanceof Error && "code" in error) { + const { code } = error as NodeJS.ErrnoException; + return code === "ENOENT" || code === "EACCES"; + } + return false; +} + /** * Read a `.sentryclirc` file's text content. Returns `null` when * the file: @@ -130,19 +143,6 @@ function isComplete(result: SentryCliRcConfig): boolean { * That broader policy is correct for opportunistic DSN scans; not * for this committed config load. */ -/** - * True for the two I/O errors we treat as "file effectively absent" - * — a missing path and a permission-denied read. Any other error - * code signals something worth surfacing to the user. - */ -function isNarrowAbsenceError(error: unknown): boolean { - if (error instanceof Error && "code" in error) { - const { code } = error as NodeJS.ErrnoException; - return code === "ENOENT" || code === "EACCES"; - } - return false; -} - async function tryReadSentryCliRc(filePath: string): Promise { let statResult: Awaited>; try {