Skip to content

osroot: refuse a non-regular leaf before opening it (fixes a FIFO hang) - #2308

Merged
Soph merged 1 commit into
mainfrom
soph/osroot-refuse-nonregular-leaf
Sep 7, 2026
Merged

osroot: refuse a non-regular leaf before opening it (fixes a FIFO hang)#2308
Soph merged 1 commit into
mainfrom
soph/osroot-refuse-nonregular-leaf

Conversation

@Soph

@Soph Soph commented Sep 7, 2026

Copy link
Copy Markdown
Collaborator

https://entire.io/gh/entireio/cli/trails/1256

A named pipe at a path Entire reads hangs the process instead of failing it. In a repo where Entire is not even enabled:

mkdir -p .claude && mkfifo .claude/settings.json
entire doctor
# prints "✓ Metadata branches: OK", then blocks in openat. SIGINT only.

open(2) on a FIFO with no writer blocks until one arrives, and none of the NoFollow helpers passes O_NONBLOCK:

os.(*Root).Open                 root.go:103
osroot.OpenNoFollow             osroot/osroot.go:80   <- parent.Open, no O_NONBLOCK
agent.(*HookConfigFile).Read    agent/hook_config_file.go:112
claudecode.loadClaudeSettings   claudecode/hooks.go:433
claudecode.CheckHookConfig      claudecode/hooks.go:499

The fix

OpenNoFollow already Lstats the leaf to reject a symlink, so the type check goes in the same place and costs nothing. Being before the open is the whole point — a post-open check cannot help, because the open is what blocks.

Directories are refused too: io.ReadAll of one already failed a step later with a platform-dependent errno, and a caller asking for a file wants the refusal rather than an EISDIR from the middle of its read.

Verified end-to-end — same repro, rebuilt binary:

WARN claude-code: failed to read settings file
     err=".claude/settings.json: path is not a regular file"

Scope, measured rather than assumed

  • entire doctor hangs. entire status and entire agent list do not — they reach the config through Exists (an Lstat), not Read. I originally described this as "every command that reads an agent config"; that was wrong, and testing each command is what corrected it.
  • The checkpoint path was never exposed. git status --porcelain does not list a FIFO at all, so the code that reads working files from status output cannot reach one. I assumed the opposite at first and checked.
  • Only a FIFO hangs. A socket fails with "operation not supported on socket"; a regular file and a char device open fine.
  • It cannot arrive by clone. Git has no tree mode for a FIFO, so this needs local action — a liveness footgun, not the traversal class the anchor work defends against.

Every read funnels through this helper, so one check covers all nine agents plus permissions.go, the plugin manifest, doctor's log readers and the settings loader. No caller wants a non-regular file: the one that reads from a directory listing (review/manifest.go:737) already skips entry.IsDir(), and the full suite passes unchanged.

Two deliberate choices

ErrNotRegularFile is not classified as os.ErrNotExist. Several callers reach these helpers to decide "is there a config here?", and answering "absent" for an occupied path would have Entire write a fresh file over whatever is there. A missing file still classifies as before, pinned by a test.

fs.ModeIrregular is masked out rather than matched, the same tolerance the .entire entry scan applies: Windows maps uncategorised reparse tags onto that bit, which lands OneDrive Files On-Demand placeholders there, and a placeholder is a readable file. Refusing it would hard-fail every repository inside a synced folder.

The error names the path and the condition but not the type, on purpose — paths.describeMode already renders one for the .entire scan, and a second copy of that vocabulary in the layer underneath is how the two drift apart.

Testing note

The FIFO test races the open against a timer instead of asserting on its error, because a regression here does not fail — it hangs, and a plain assertion would take the package's timeout with it. It is Unix-only by build constraint rather than a runtime skip, since syscall.Mkfifo does not exist on Windows and a runtime guard still has to compile.

Relationship to #2290

Follow-up to #2290, which made this condition reportable — doctor's agent-path scan now checks the leaf's type instead of passing a FIFO as clean — but a scan cannot stop the hang.

Independent, but they do share one file. Both touch cmd/entire/cli/osroot/osroot.go: #2290 unwraps an error in RemoveAllNoSymlinks (line ~401), this PR adds the sentinel and the check in OpenNoFollow (lines ~76-140). Different regions, and I verified git auto-merges them in either order — merging both onto main in a scratch worktree produced no conflicts, and the combined tree builds with the whole unit suite passing. Either can land first.

Verification

mise run lint 0 issues, mise run test:ci exit 0, and GOOS=windows|linux|darwin go vet ./... all clean.

🤖 Generated with Claude Code


Note

Medium Risk
Centralizes file-open behavior for all agent config reads; behavior change for non-regular paths is intentional but broad, with careful error semantics to avoid overwriting occupied paths.

Overview
OpenNoFollow now runs a pre-open Lstat type check via requireRegularFile, returning ErrNotRegularFile for directories, FIFOs, sockets, and devices instead of calling open (which can block indefinitely on a FIFO with no writer). ReadFileNoFollow inherits this behavior.

The new sentinel is not treated as os.ErrNotExist so callers do not treat a wrong-type path as “missing config,” and fs.ModeIrregular is ignored so Windows OneDrive placeholders stay readable.

Tests add a Unix FIFO race-with-timeout guard, plus coverage for directory rejection, normal files, and absent paths still reporting not-exist.

Reviewed by Cursor Bugbot for commit 2efcf77. Configure here.

A named pipe at a path Entire reads hung the process instead of failing it.
open(2) on a FIFO with no writer blocks until one arrives, and none of the
NoFollow helpers passes O_NONBLOCK, so in a repo with a FIFO at
.claude/settings.json:

    mkdir -p .claude && mkfifo .claude/settings.json && entire doctor
    # prints "Metadata branches: OK", then blocks in openat. SIGINT only.

    os.(*Root).Open                 root.go:103
    osroot.OpenNoFollow             osroot/osroot.go:80   <- parent.Open, no O_NONBLOCK
    agent.(*HookConfigFile).Read    agent/hook_config_file.go:112
    claudecode.loadClaudeSettings   claudecode/hooks.go:433
    claudecode.CheckHookConfig      claudecode/hooks.go:499

OpenNoFollow already Lstats the leaf to reject a symlink, so the type check goes
in the same place and costs nothing. Being BEFORE the open is the whole point: a
post-open check cannot help, because the open is what blocks.

Directories are refused too. io.ReadAll of one already failed a step later with
a platform-dependent errno, and a caller asking for a file wants the refusal
rather than an EISDIR from the middle of its read.

Scope, measured rather than assumed:

- `entire doctor` hangs. `entire status` and `entire agent list` do NOT — they
  reach the config through Exists (an Lstat), not Read. The exposure is narrower
  than "every command that reads an agent config", which is what I first wrote.
- `git status --porcelain` does not list a FIFO at all, so the checkpoint path
  that reads working files from status output was never exposed.
- Every read funnels through here, so the fix covers all nine agents plus
  permissions.go, the plugin manifest, doctor's log readers and the settings
  loader. Verified no caller wants a non-regular file: the one that reads from a
  directory listing (review/manifest.go:737) already skips entry.IsDir().

ErrNotRegularFile is deliberately not classified as os.ErrNotExist. Several
callers reach these helpers to decide "is there a config here?", and answering
"absent" for an occupied path would have Entire write a fresh file over whatever
is there. A missing file still classifies as before, which a test pins.

fs.ModeIrregular is masked out rather than matched, the same tolerance the
.entire entry scan applies: Windows maps uncategorised reparse tags onto that
bit, which lands OneDrive Files On-Demand placeholders there, and a placeholder
is a readable file.

The FIFO test races the open against a timer instead of asserting on its error,
because a regression does not fail here — it hangs, and a plain assertion would
take the package's timeout with it.

Follow-up to #2290, which made this condition reportable by checking the leaf's
type in doctor's scan but could not stop the hang. Independent of it: no shared
files, either can land first.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Entire-Checkpoint: 01M1XP8G6Y4CN4CR752AM0M9FD
@Soph
Soph requested a review from a team as a code owner September 7, 2026 10:23
Copilot AI lite review requested due to automatic review settings September 7, 2026 10:23

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The change is narrowly scoped to OpenNoFollow’s pre-open validation, aligns with the stated error semantics, and is backed by targeted tests including a non-hanging FIFO regression guard.

Pull request overview

This PR hardens osroot.OpenNoFollow by refusing non-regular leaf paths before attempting open(2), preventing indefinite blocking when the leaf is a FIFO (named pipe) with no writer. This fits the CLI’s broader os.Root-based containment and safety work by ensuring file reads fail fast and deterministically when the on-disk object type is not what callers expect.

Changes:

  • Add a pre-open “regular file required” gate to OpenNoFollow, returning a new sentinel ErrNotRegularFile for directories/FIFOs/sockets/devices (while tolerating fs.ModeIrregular as intended for Windows placeholders).
  • Introduce ErrNotRegularFile and a small helper (requireRegularFile) to centralize the check and keep error semantics consistent.
  • Add unit coverage for directory rejection, regular-file allowance, missing-file classification, and a Unix-only FIFO regression test that would otherwise hang.
File summaries
File Description
cmd/entire/cli/osroot/osroot.go Adds pre-open leaf type validation and introduces ErrNotRegularFile to prevent FIFO hangs and normalize non-regular handling.
cmd/entire/cli/osroot/osroot_test.go Adds portable tests validating directory refusal, regular-file allowance, and “missing stays os.ErrNotExist” behavior.
cmd/entire/cli/osroot/openfifo_unix_test.go Adds a Unix-only FIFO regression test guarded by a timeout race to detect hangs without stalling the suite.
Review details
  • Files reviewed: 3/3 changed files
  • Comments generated: 0
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@Soph
Soph merged commit a1cdf4b into main Sep 7, 2026
14 of 15 checks passed
@Soph
Soph deleted the soph/osroot-refuse-nonregular-leaf branch September 7, 2026 14:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants