ref(fs): add safeReadFile helper for FIFO-safe config reads#819
Merged
Conversation
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.
Contributor
|
Contributor
Codecov Results 📊✅ 138 passed | Total: 138 | Pass Rate: 100% | Execution Time: 0ms 📊 Comparison with Base Branch
✨ No test changes detected All tests are passing successfully. ✅ Patch coverage is 90.00%. Project has 1966 uncovered lines. Files with missing lines (1)
Coverage diff@@ Coverage Diff @@
## main #PR +/-##
==========================================
+ Coverage 95.18% 95.18% —%
==========================================
Files 282 283 +1
Lines 40723 40801 +78
Branches 0 0 —
==========================================
+ Hits 38762 38835 +73
- Misses 1961 1966 +5
- Partials 0 0 —Generated by Codecov Action |
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
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
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.
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.
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 313ef62. Configure here.
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

Summary
Extends the FIFO-safety pattern from #806 beyond DSN detection. All remaining single-file reads of user-controlled paths now have a
stat.isFile()guard (either via a newsafeReadFilehelper or inline), eliminating the FIFO-hang bug class across the codebase.Why
PR #806 fixed
Bun.file().text()hanging on 1Password-style symlinked FIFOs for DSN detection (4 sites insrc/lib/dsn/). But the CLI has ~4 more read sites under user-controlled paths with the same vulnerability:src/lib/sentryclirc.ts— reads.sentryclircat every directory during walk-up (hit on every CLI invocation).src/lib/init/tools/read-files.ts— LLM-driven reads of arbitrary project files during init wizard.src/lib/init/tools/apply-patchset.ts— reads modify-target files during init wizard.src/lib/init/workflow-inputs.ts::preReadCommonFiles— scans a fixed allowlist of common config filenames.All four would hang on a 1Password-symlinked
.env,.sentryclirc,package.json, etc. An audit pass also surfaced a subtle bug in the two sites that already had astat()-based size check:staton a FIFO returns successfully withsize=0, so the size-check passes and the subsequent read still hangs. Addingstat.isFile()alongside the size check fixes this without a secondstatcall.Design
Two distinct policies, corresponding to two different read contexts:
src/lib/safe-read.ts::safeReadFile— opportunistic best-effort readsChains
isRegularFile(stat + FIFO guard) withBun.file(path).text(). Returnsnullfor any expected error (ENOENT, EACCES, EPERM, EISDIR, ENOTDIR, non-regular file, etc.); unexpected errors route through the existinghandleFileError→ Sentry pipeline and also returnnull. Used byapply-patchset.ts(where null is translated to a targeted throw).sentryclirc.ts::tryReadSentryCliRc— committed config loadDirect
fs.promises.stat+ narrowtry/catchthat returnsnullonly on ENOENT/EACCES; re-throws every other error. A user with a broken.sentryclirc(EPERM at the dir level, EISDIR, EIO, etc.) sees the error clearly rather than getting confusing downstream "no auth token" failures.The two sites with size-gated reads (
read-files.ts,workflow-inputs.ts::preReadCommonFiles) keep their existingfs.promises.statcall but now checkstat.isFile()alongsidestat.size. No extra stat calls.Per-site changes
sentryclirc.ts::tryApplyFiletryReadFile(18 LOC, ENOENT/EACCES-only)tryReadSentryCliRc— addsstat.isFile()FIFO guard, keeps the narrow ENOENT/EACCES re-throw policy intactinit/tools/read-files.tsfs.promises.stat+ size gate + readstat.isFile()check before the readinit/tools/apply-patchset.tsfs.promises.readFile(absPath)safeReadFile→ throws targeted "not a regular file or read failed" on nullinit/workflow-inputs.ts::preReadCommonFilesstat+ size gate + readstat.isFile()check; setscache[filePath] = nullon non-regularRegression tests
test/lib/safe-read.test.ts— 11 tests covering every migration site:safeReadFileon regular file / missing / FIFO / symlink→FIFO / directorysentryclircloader on a FIFO-backed.sentryclirc→ empty config, no hangreadFilestool on FIFO and symlink→FIFO paths (both test cases)applyPatchsettool on FIFO and symlink→FIFO targets (both test cases)preReadCommonFileson a FIFO-backed common-config fileVerified negatively: for each migrated site, temporarily removing the FIFO guard causes the corresponding regression test to time out at Bun's 5s default, confirming the guards are genuinely the condition being exercised.
Review cycle
The PR went through multiple rounds of self-review + bot review after the initial post. Key fixes from review:
fs.promises.statwith the narrow original error policy (not viaisRegularFile, which had a broaderhandleFileErrorswallow-list).isRegularFileinternally callshandleFileError. Fixed by callingstatdirectly.safe-read.tsafter the sentryclirc-goes-direct change removed the last consumer. Dropped.precomputeDirListing(a listing op) instead ofpreReadCommonFiles(the function with the new guard). Fixed to call the right function and negative-verified.Test plan
bunx tsc --noEmit— cleanbun run lint— clean (1 pre-existing markdown.ts warning)bun test test/lib/safe-read.test.ts— 11 passbun test --timeout 15000 test/lib test/commands test/types— 5687 pass, 0 failbun test test/isolated— 138 pass🤖 Generated with Claude Code
Co-authored-by: Claude Opus 4.7 (1M context) noreply@anthropic.com