fix(file-tools): make the device/proc read-guard fire on Windows - #69403
Sora-bluesky wants to merge 1 commit into
Conversation
|
The duplicate note above no longer applies: #69401 was closed by its author on 2026-07-24 as stale, so there is nothing left to choose between and this is the only open fix for #69373. Re-checked on Windows against main 199f558. |
monerostar
left a comment
There was a problem hiding this comment.
monerostar native Windows live-verify
Environment
- Windows 11 native (Build 26200) · Git Bash · Python 3.11.15 (hermes venv)
- Control: install /
origin/maintools/file_tools.py(no separator fold) - PR HEAD:
f85d26979— worktree%LOCALAPPDATA%\hermes\pr-worktrees\pr-69403 - CI: Python test slices + ruff pass on this PR
Root cause confirmed on this host
os.sep == '\\'
os.path.normpath('/dev/zero') == '\\dev\\zero' # never equals '/dev/zero'
So the POSIX string comparisons in _is_blocked_device_path are a silent no-op on Windows without a fold.
Live _is_blocked_device_path matrix
| Path | main (control) | this PR |
|---|---|---|
/dev/zero |
False | True |
/dev/tty |
False | True |
/proc/self/environ |
False | True |
/proc/1/maps |
False | True |
\\dev\\zero |
False | True |
/dev/../dev/zero |
False | True |
C:/dev/zero |
False | False (drive-qualified kept) |
C:\Users\...\notes.txt |
False | False |
//server/share/dev/zero |
False | False |
Exactly the intended shape: MSYS-style device/proc paths block; native drive/UNC stay free.
Unit tests
PYTHONPATH=<pr> pytest tests/tools/test_file_read_guards.py -q -o addopts=
→ 45 passed, 5 skipped in ~2.3s
Includes new test_windows_normpath_device_paths_are_blocked (ntpath + os.sep='\\' simulation).
Note: Running the main tree's full test_file_read_guards.py on this box hung past 60s mid-run (failures already showing). That is consistent with unguarded device/fd reads in the suite or tool path — another reason this guard should land. I did not leave a hung reader running.
Native open('/dev/zero') aside
Plain Win32 CPython here: open('/dev/zero') → FileNotFoundError (no MSYS). The issue's hang class is specifically agent read I/O via Git Bash/MSYS, which emulates /dev/* and /proc/*. The fold still has to fire on the path string before any backend open — which this PR does at the single choke point used by symlink/realpath callers too.
Assessment
- Correct, minimal (+ fold + tests), no competing open fix PR for #69373.
- Conservative over-match of bare
\\dev\\zerois called out and acceptable. - Prefer this over a Windows-only parallel blocklist.
Formal Approve: blocked for external collaborator — comment review only.
|
Thanks for running this on real hardware. The control-vs-PR matrix is exactly the shape I hoped for, and it makes the fold's effect easy to check at a glance, including the drive-qualified and UNC non-matches staying free. The hang you hit mid-run on main's |
|
Thanks for the focused Windows regression fix. Current main still normalizes in The proposed fold is at the shared helper used by the literal, symlink-hop, and realpath guard paths ( Automated hermes-sweeper review. |
|
suggesting changes The Windows normalization and NT-namespace checks repair the reported local-backend bypass, but the new search guard is explicitly disabled for every container-path backend. A caller can therefore pass a literal blocked path such as /dev/zero or /proc/self/environ to search_tool under Docker, Singularity, Modal, or Daytona and reach the backend search implementation. That preserves both the blocking-device and proc-data exposure class on those supported trust boundaries. The guard should always apply its pure lexical checks and restrict only host symlink/realpath dereferencing to local backends.
Security evidence:
Uncertainty: No Windows runner was available, so actual Git Bash handling of raw DOS reserved names such as CON and NUL was not established.; Pytest was not installed in the leased checkout environment, preventing execution of the three focused pytest files through the requested runner grammar.; No live Docker, Singularity, Modal, or Daytona backend was invoked; the container bypass was validated at the dispatch boundary and against the backend source.; A full current-main patch replay was not required by the bound decision and was not performed; the reviewed head was coherent and the changed functions were compared directly with current main. Signed: GPT-5.6-sol-xhigh in Codex |
|
You were right, and the container exception was worse than one skipped check. Fixed in 234c8605d. I reproduced it first: with a container task, The pure path check now runs for every backend; only the symlink and realpath hops stay local, since those dereference on the host. Container tasks classify with POSIX semantics on the resolved path, plus the raw input when it is absolute, because Measured through On your point about NT namespace rules: Two cases are still open, and I would rather name them than imply they are covered. A workspace symlink inside the container pointing at a device is not reachable from a host-side path check. Neither is |
|
suggesting changes The PR fixes the reported Windows normalization failure and adds a pre-dispatch device guard to search_tool, including a container-specific lexical normalizer. The Windows fix behaved correctly in direct positive and negative probes, and the current-main failure was reproduced from the exact bound main source. One in-scope container bypass remains: the new normalizer models /proc//root symlinks but not /proc//cwd. A search path routed through that symlink can resolve to /dev/zero inside the backend while the guard classifies the lexical spelling as safe and invokes the search backend. This preserves the hang/timeout class the new search guard is intended to eliminate.
Security evidence:
Uncertainty: The changed pytest files could not be executed because no pytest-capable Python environment is present in the leased checkout.; No live Docker backend was created for the probe; backend dispatch and Linux symlink resolution were validated separately with deterministic local probes.; The exact timeout behavior of each supported remote/container backend was not exercised, though all affected search implementations pass the path to rg, grep, or find after this guard. Signed: GPT-5.6-sol-xhigh in Codex |
234c860 to
80797ad
Compare
SummaryTwo PRs address #69373's Windows separator mismatch: both restore POSIX-form comparisons after Related pull requests
Duplicates#69401 and #69403 duplicate the core Windows separator-normalization repair for #69373; #69403 is not a full duplicate because it adds NT-namespace, search-dispatch, container-path, and stronger runner-independent regression coverage. Suggested consolidationKeep #69403 open with a salvage path: preserve its verified Windows separator fold and Complex graphflowchart LR
classDef open fill:#dbeafe,stroke:#1d4ed8,color:#1e3a8a
classDef merged fill:#dcfce7,stroke:#15803d,color:#14532d
classDef closed fill:#e5e7eb,stroke:#6b7280,color:#1f2937
classDef unverified fill:#f3f4f6,stroke:#9ca3af,color:#374151
classDef best stroke-width:3px,stroke:#b45309
classDef target stroke-width:3px,stroke:#4338ca
I69373(["issue #69373 (open)"])
subgraph Dup69401 ["PRs duplicating each other"]
P69401["PR #69401 (closed)"]
P69403["PR #69403 (open)"]
end
P69403 -->|best fix| I69373
class I69373 open
class P69401 closed
class P69403 open
class P69401 best
class P69403 best
class P69403 target
click I69373 "https://github.com/NousResearch/hermes-agent/issues/69373"
click P69401 "https://github.com/NousResearch/hermes-agent/pull/69401"
click P69403 "https://github.com/NousResearch/hermes-agent/pull/69403"
Graph: solid arrow = fixes / best fix, dashed arrow = partial or unverified (see edge label); boxed group = PRs duplicating each other; amber border = best fix; indigo border = target; gray node = closed (state tag in the node label). Cross-PR triage: Reviewed 2 pull requests and 1 issue in this complex. Each diff was read against this issue; Assessment working set: 26 kB of PR diffs, 7 kB of issue/PR text, 21 kB of discussion (14 comments), 4 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch. |
|
suggesting changes The new guard still lets a container search reach a blocked device through a Security evidence:
Not checked:
Signed: GPT-5.6-sol-xhigh in Codex |
|
Checked this against 80797ade4. The gap is real: This is a known residual class in this PR's guard -- the walker fold for cwd/exe, refusal tests included, is already spec'd as the planned next change on this branch. |
80797ad to
9ef41a2
Compare
|
@egilewski Rebased onto current main and pushed round 2 (change commit 9ef41a2b69). What changed since your last look:
Coverage: 7 new test functions plus extended parametrized tables (43 newly collected cases) across the alias forms, dot/slash bypass spellings, and the cache ordering. CI is green on the current head; the one red slice on the first run was a Telegram polling-timing test this PR doesn't touch, and it cleared on the retrigger. |
adba93a to
69b0328
Compare
|
#101714 ( |
_is_blocked_device_path() normalizes with os.path.normpath and compares
against POSIX strings ("/dev/zero", "/proc/*/environ", ...). On Windows
normpath turns "/dev/zero" into "\dev\zero", which never matches, so the
entire device/fd/proc read-guard -- including the /proc secret-leak family
from NousResearch#4427 and the realpath re-check from NousResearch#10141 -- was a silent no-op there.
Windows read I/O shells through Git Bash, whose MSYS layer emulates /dev/*
and /proc/* as real streams, so read_file("/dev/zero") genuinely hangs the
agent (the repo's own device-rejection test wedges past 120s on Windows).
Fold separators back to "/" after normpath when os.sep is "\\", so the POSIX
comparisons fire. Every caller (literal path, each symlink hop, and the
realpath re-check) routes through _is_blocked_device_path, so this single
point fixes them all. Drive-qualified and UNC paths keep their prefix
("C:\dev\zero" -> "C:/dev/zero", "\\server\share\dev\zero" ->
"//server/share/dev/zero") and still fall through unblocked. The one
deliberate over-match is a bare root-relative native path such as
"\dev\zero" on the current drive, which collapses to "/dev/zero" and is now
blocked; an exotic spelling, and blocking it is the conservative side.
Adds a regression test that drives the function's normpath through
ntpath.normpath with os.sep forced to "\\", so the Windows branch runs on
Linux CI too, not only on a real Windows box.
Fixes NousResearch#69373.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
69b0328 to
2a9523b
Compare
What
_is_blocked_device_path()runsos.path.normpathon the candidate before comparing it against the POSIX strings in_BLOCKED_DEVICE_PATHS. On Windowsnormpath("/dev/zero")returns\dev\zero, which never matches/dev/zero, so the whole device/fd/proc read-guard is a silent no-op there, including the/proc/*/environsecret-leak checks added for #4427.It bites on Windows specifically because read I/O shells through Git Bash, whose MSYS layer emulates
/dev/*and/proc/*as real (often blocking) streams, soread_file("/dev/zero")genuinely hangs the agent.Fixes #69373.
Fix
One guard at the top of
_is_blocked_device_path: whenos.sep == "\\", fold the post-normpathseparators back to/so the existing comparisons fire. Every caller routes through this function (literal path, each symlink hop, and the realpath re-check in_is_blocked_device), so the single point covers all of them, #10141's realpath guard included.Drive-qualified native paths keep their prefix (
C:\dev\zerobecomesC:/dev/zero) and still fall through unblocked. The one conservative over-match is a bare root-relative native path like\dev\zero, which collapses to/dev/zeroand is treated as blocked, an acceptable call for such an exotic input.Test
test_windows_normpath_device_paths_are_blockeddrives the function'snormpaththroughntpathwithos.sepforced to\\, so it reproduces the reported Windows flow on Linux CI without needing a real Windows box. It asserts forward-slash, mixed, redundant, and trailing-separator/devand/procinputs are blocked, and thatC:/dev/zero,C:\Users\me\notes.txt, and//server/share/dev/zerostay unblocked. Verified failing without the fix and passing with it (48 passed, 2 skipped in the guard suite).