Skip to content

fix(pathlib): compare profiles in is_relative_to - #179

Merged
shunjiad merged 1 commit into
NVIDIA:mainfrom
MaxFreedomPollard:bug-pathlib-cross-profile-relative-check
Sep 21, 2026
Merged

fix(pathlib): compare profiles in is_relative_to#179
shunjiad merged 1 commit into
NVIDIA:mainfrom
MaxFreedomPollard:bug-pathlib-cross-profile-relative-check

Conversation

@MaxFreedomPollard

@MaxFreedomPollard MaxFreedomPollard commented Sep 20, 2026 •
edited by coderabbitai Bot
Loading

Copy link
Copy Markdown
Contributor

Description

Path("msc://a/data/file.txt").is_relative_to(Path("msc://b/data")) currently returns True, even though the paths belong to different storage profiles and relative_to() rejects the same comparison.

Check the resolved profile before comparing path components, matching the profile boundary already used by relative_to() and equality. Same-profile comparisons, local filesystem aliases and unsupported input handling keep their existing behavior.

The regression tests configure two local profiles, so they exercise real path resolution without cloud credentials or storage simulators. Both cross-profile cases fail against the previous implementation. The docstring, user guide and release note describe the corrected behavior.

No tracked issue.

Checklist

  • Development PR
    • .release_notes/.unreleased.md
      • Notable changes to the client from this PR have been added.

Testing

  • 25 pathlib tests pass, including all 11 new cases. Three existing remote-storage cases were deselected locally.
  • Ruff lint and format checks pass for both changed Python files.
  • Pyright reports 0 errors and 0 warnings across the Python client and tests.
  • just multi-storage-client/prepare-toolchain succeeds, including the native Python/Rust wheel build.
  • just multi-storage-client-docs/build succeeds with warnings treated as errors.

Broader local checks: just multi-storage-client/analyze stops at the existing F403 wildcard import in contrib/os/__init__.py; just multi-storage-client/run-unit-tests stops at the simulator setup's GNU mkdir --parents command on macOS; the root just build stops because treefmt is unavailable. Storage simulators are not installed on this machine. All commands above used python-binary=python3 where applicable.

Summary by CodeRabbit

  • Bug Fixes

    • Corrected Path.is_relative_to() so paths from different storage profiles return False, even when their internal paths overlap.
    • Improved handling of local paths and equivalent filesystem URI paths.
    • Unsupported path types now return False without raising an error.

  • Documentation

    • Added guidance explaining Path.is_relative_to() behavior for matching, nested, and cross-profile paths.

@MaxFreedomPollard
MaxFreedomPollard requested a review from a team September 20, 2026 22:57
@copy-pr-bot

copy-pr-bot Bot commented Sep 20, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@coderabbitai

coderabbitai Bot commented Sep 20, 2026 •
edited
Loading

Copy link
Copy Markdown

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository: NVIDIA/multi-storage-client/.coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: ed2ba050-af91-4160-a64b-39bc5acf16cd

📥 Commits

Reviewing files that changed from the base of the PR and between f2fdeda and 1cbe1d8.

📒 Files selected for processing (4)
  • .release_notes/.unreleased.md
  • multi-storage-client-docs/src/user_guide/libraries.rst
  • multi-storage-client/src/multistorageclient/pathlib.py
  • multi-storage-client/tests/test_multistorageclient/unit/test_pathlib.py

Included review availability: Your plan provides up to 12 included reviews per hour; 10 remain after this review.

📝 Walkthrough

Walkthrough

Path.is_relative_to() now returns False for paths from different storage profiles. Tests cover profile matching, local path equivalence, containment, and unsupported inputs. Documentation and release notes describe the updated behavior.

Changes

Path containment behavior

Layer / File(s) Summary
Profile-aware containment check
multi-storage-client/src/multistorageclient/pathlib.py, multi-storage-client/tests/test_multistorageclient/unit/test_pathlib.py, multi-storage-client-docs/src/user_guide/libraries.rst, .release_notes/.unreleased.md		 is_relative_to now requires matching storage profiles before comparing internal paths. Tests cover containment, cross-profile paths, local and msc://__filesystem__/ equivalence, and unsupported base types. Documentation and release notes describe the behavior.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Suggested reviewers: shunjiad

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 functions across 2 files. (2 skipped: 2… Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly identifies the pathlib fix and the profile comparison added to is_relative_to.
Description check ✅ Passed The description explains the bug, implementation, compatibility behavior, tests, documentation, release note, and known environmental limitations. It follows the development PR template and marks the …
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage

Explanation

Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 functions across 2 files. (2 skipped: 2 unsupported.)

  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR

Comment @coderabbitai help to get the list of available commands.

@shunjiad

shunjiad commented Sep 21, 2026

Copy link
Copy Markdown
Contributor

/ok to test 1cbe1d8

@shunjiad
shunjiad added this pull request to the merge queue Sep 21, 2026
Merged via the queue into NVIDIA:main with commit 442bb07 Sep 21, 2026
23 of 24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@shunjiad shunjiad shunjiad approved these changes

Assignees

@MaxFreedomPollard MaxFreedomPollard

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@MaxFreedomPollard @shunjiad