fix: preserve absolute root in _strip_protocol so walk("/") works - #69
Conversation
SSHFileSystem inherited the default root_marker of "", so
_strip_protocol("/") collapsed the root to an empty string. walk("/")
then listed the home directory and produced relative paths that were
not considered to exist. Set root_marker = "/" to keep the root, and
add walk tests covering a nested tree and the root case.
There was a problem hiding this comment.
Pull request overview
This PR fixes SSHFileSystem.walk("/") returning incorrect paths by ensuring protocol-stripping preserves the root path. It aligns SSHFileSystem’s root handling with expected absolute-path semantics so directory traversal from / yields paths that exists() recognizes.
Changes:
- Set
SSHFileSystem.root_marker = "/"to prevent_strip_protocol("/")collapsing/into an empty string. - Add
walk()test coverage for a nested directory tree under a test-specific remote path. - Add a regression test ensuring
walk("/")yieldsroot == "/"and absoluteinfo["name"]entries that exist.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| tests/test_sshfs.py | Adds coverage for walk() on both nested paths and / regression case to ensure returned entries are absolute and valid. |
| sshfs/spec.py | Sets root_marker to preserve / during protocol stripping, fixing root-walk behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@shcheklein I'd like it to be merged and released. What is left? |
shcheklein
left a comment
shcheklein
left a comment
There was a problem hiding this comment.
This PR has (unintentionally) higher blast radius. Probably we need some redesign, at least don't set root_marker. Preserve / only when the caller explicitly supplied an absolute root in strip_protocol or something like this. + we need more tests to ensure that the current behavior is not affected.
mixilchenko
changed the title
|
@shcheklein updated the pr name and description |
The mock server exposes the host's real filesystem, so assertions about the contents of "/" depend on machine state: mock-ssh-server stats every entry of a listing, so a single dangling symlink at the host root fails the whole readdir, and fsspec's walk(on_error="omit") swallows the error, silently turning the name/exists assertions into a no-op. Assert only the yielded root, which distinguishes the fix (root "/") from the regression (root "") on every machine. Also correct the failure-mode note: most servers (e.g. OpenSSH) reject an empty path with ENOENT outside REALPATH; servers implementing the SFTP draft's empty-path rule resolve it to the default directory instead. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
There is one small test tweak I'd like to include before merging: 20e9924 (branch Could you either pull it into your branch: or tick "Allow edits by maintainers" on this PR so I can push it directly? Thanks! |
|
@shcheklein did both, pull and allowed maintainers edits. Thanks! |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #69 +/- ##
==========================================
- Coverage 94.30% 93.60% -0.70%
==========================================
Files 13 12 -1
Lines 738 798 +60
==========================================
+ Hits 696 747 +51
- Misses 42 51 +9 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
test_walk_root listed the host's real root through the mock server: mock-ssh-server stats every entry of a listing, so a single dangling symlink at "/" fails the whole readdir. On older fsspec releases (e.g. 2025.10.0, which Python 3.9 resolves) walk() yields nothing when the listing fails, so next() raised StopIteration -- the macos-latest/3.9 CI failure. Assert root resolution via stat instead, which depends neither on the contents of "/" nor on the fsspec version. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Thanks for the merge. Could you please make the release so I could bump the dependencies in my project? |
|
@mixilchenko I've published it, it should be available soon. There some other changes. Please test it and let me know if you hit any issues. |
Problem
SSHFileSystem._strip_protocol("/")collapsed the explicit root down to""(the inherited emptyroot_marker). Most SFTP servers, OpenSSH included, reject an empty path with ENOENT for everything exceptREALPATH, sowalk("/")silently yielded nothing andexists("/")/isdir("/")returnedFalse. Servers implementing the SFTP draft's "empty path = default directory" rule resolved it to the home directory instead.Fix
_strip_protocolnow restores/only when the caller explicitly supplied the absolute root:/,ssh://host/) →/"",ssh://host,foo/bar) → unchanged, still resolving against the home directorySetting
root_marker = "/"instead would have remapped""and baressh://hostfrom home to root and shifted_parent()for relative paths, so the narrower fix was chosen.Behavior change
"/"now refers to the actual root in every operation that goes through fsspec's path stripping:exists,isdir,info,walk,find,glob,du,get,put,rmon"/"act on the root tree instead of failing or resolving to$HOME. Methods that never stripped paths (ls,mkdir,mv, ...) are unaffected.Tests
walkcoverage over a nested tree, a stat-based root-resolution test (deterministic on any host and any supported fsspec release), and_strip_protocolunit tests pinning root/empty/relative/URL behavior.