nt is_dir,is_file,listmount,listvolume #6373

youknowone · 2025-12-09T09:46:40Z

Summary by CodeRabbit

Release Notes

New Features
- Volume enumeration functionality now available for Windows systems
- Mount path enumeration functionality now available for Windows systems
Bug Fixes
- Environment variable length validation on Windows to prevent overflow
- Improved directory detection with enhanced fallback behavior on Windows systems

_{✏️ Tip: You can customize this high-level summary in your review settings.}

coderabbitai · 2025-12-09T09:46:46Z

Walkthrough

These changes introduce Windows-specific functionality: a new maximum environment variable constant, two volume enumeration functions for the nt module (listvolumes and listmounts), environment variable length validation in the os module, and refactored DirEntry metadata handling with Windows fallback behavior.

Changes

Cohort / File(s)	Summary
Windows Constants `crates/common/src/windows.rs`	Added public constant `_MAX_ENV` with value `32767` representing the Windows CRT maximum environment variable size.
nt Module Volume Functions `crates/vm/src/stdlib/nt.rs`	Introduced `listvolumes()` to enumerate volume names using Windows API (`FindFirstVolumeW`/`FindNextVolumeW`), and `listmounts(volume)` to resolve mount paths for a volume using `GetVolumePathNamesForVolumeNameW` with dynamic buffer sizing. Both return Python lists and handle Windows I/O errors.
os Module Enhancements `crates/vm/src/stdlib/os.rs`	Added Windows-specific environment variable length validation in `putenv()` and `unsetenv()` against `_MAX_ENV`. Refactored `DirEntry.is_dir()` and `is_file()` to directly use metadata queries with Windows-specific `NotFound` fallback using cached `file_type` data.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~22 minutes

crates/vm/src/stdlib/nt.rs: Windows API integration and buffer handling logic in listmounts() requires careful verification of error paths and null-separated string parsing.
crates/vm/src/stdlib/os.rs: Platform-specific fallback behavior in DirEntry methods needs validation that Windows NotFound errors are handled correctly and cached data is reliable; env validation boundary conditions should be verified.
Cross-file consistency: Verify that _MAX_ENV constant usage in os.rs aligns with Windows CRT specifications.

Possibly related PRs

Windows execv, spawnv, wait #6350: Modifies the Windows nt module and environment-related handling, overlapping with volume/environment changes in this PR.
fix scandir/lstat for windows #6357: Refactors DirEntry metadata handling and adds cached lstat data, directly overlapping with the DirEntry.is_dir() and is_file() refactoring in this PR.

Poem

🐰 Hopping through Windows, volumes we find,
Mount paths and metadata, cleverly aligned,
Environment checks, no overflow today,
DirEntry now falters the NotFound way! ✨

Pre-merge checks and finishing touches

❌ Failed checks (1 inconclusive)

Check name	Status	Explanation	Resolution
Title check	❓ Inconclusive	The title references specific function names (is_dir, is_file, listmount, listvolume) that are implemented in the changeset, but uses unclear abbreviations and lacks context about what was actually changed.	Rewrite the title to be more descriptive, such as 'Add Windows volume/mount listing and improve directory metadata handling' or 'Implement nt.listvolumes/listmounts and fix is_dir/is_file on Windows'.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

Actionable comments posted: 1

📜 Review details

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between cc534d2 and 2dab1f6.

⛔ Files ignored due to path filters (1)

Lib/test/test_os.py is excluded by !Lib/**

📒 Files selected for processing (3)

crates/common/src/windows.rs (1 hunks)
crates/vm/src/stdlib/nt.rs (1 hunks)
crates/vm/src/stdlib/os.rs (5 hunks)

🧰 Additional context used

📓 Path-based instructions (1)

**/*.rs

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

**/*.rs: Follow the default rustfmt code style by running cargo fmt to format Rust code
Always run clippy to lint Rust code (cargo clippy) before completing tasks and fix any warnings or lints introduced by changes
Follow Rust best practices for error handling and memory management
Use the macro system (pyclass, pymodule, pyfunction, etc.) when implementing Python functionality in Rust

Files:

crates/common/src/windows.rs
crates/vm/src/stdlib/nt.rs
crates/vm/src/stdlib/os.rs

🧠 Learnings (1)

📚 Learning: 2025-06-28T16:31:03.991Z

Learnt from: arihant2math
Repo: RustPython/RustPython PR: 5790
File: build.rs:2-2
Timestamp: 2025-06-28T16:31:03.991Z
Learning: In Cargo build scripts (build.rs), the environment variable CARGO_CFG_TARGET_OS is guaranteed to exist and is automatically set by Cargo during the build process, making unwrap() safe to use when accessing this variable.

Applied to files:

crates/vm/src/stdlib/os.rs

🧬 Code graph analysis (1)

crates/vm/src/stdlib/nt.rs (1)

crates/vm/src/stdlib/os.rs (1)

errno_err (39-41)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)

GitHub Check: Run snippets and cpython tests on wasm-wasi
GitHub Check: Run snippets and cpython tests (macos-latest)
GitHub Check: Run snippets and cpython tests (ubuntu-latest)
GitHub Check: Run snippets and cpython tests (windows-latest)
GitHub Check: Run rust tests (windows-latest)
GitHub Check: Check the WASM package and demo
GitHub Check: Run rust tests (macos-latest)
GitHub Check: Run rust tests (ubuntu-latest)
GitHub Check: Ensure compilation on various targets
GitHub Check: Check Rust code with clippy

🔇 Additional comments (6)

crates/vm/src/stdlib/nt.rs (2)

685-717: LGTM! Volume enumeration implementation looks correct.

The handle lifecycle is properly managed: FindVolumeClose is called on any FindNextVolumeW failure, covering both normal termination (ERROR_NO_MORE_FILES) and error conditions. The buffer size of MAX_PATH + 1 is appropriate for volume GUIDs.

Minor observation: from_utf16_lossy silently replaces invalid sequences. This is acceptable for volume names which should always be valid UTF-16.

719-764: LGTM! Mount path enumeration handles dynamic buffer sizing correctly.

The retry loop for ERROR_MORE_DATA with buffer resizing follows the standard Windows pattern. The null-separated string parsing with double-null termination detection (lines 757-758) is correct.

crates/common/src/windows.rs (1)

6-8: LGTM!

The constant value 32767 correctly matches the Windows CRT _MAX_ENV definition, and the doc comment clearly identifies the source.

crates/vm/src/stdlib/os.rs (3)

431-443: LGTM! Windows environment variable length validation.

The helper function correctly validates against _MAX_ENV and provides a clear error message.

529-546: LGTM! Windows fallback for is_dir handles race conditions.

The fallback to cached file_type when the file is removed between directory enumeration and the is_dir call is a sensible approach for Windows. This matches CPython's behavior of using directory entry metadata when the file no longer exists.

548-565: LGTM! Consistent pattern with is_dir.

The is_file refactoring mirrors the is_dir implementation with the same Windows-specific fallback to cached file_type on NotFound.

coderabbitai · 2025-12-09T10:27:45Z

crates/vm/src/stdlib/os.rs

+        // For unsetenv, size is key + '=' (no value, just clearing)
+        #[cfg(windows)]
+        check_env_var_len(key.len() + 1, vm)?;


⚠️ Potential issue | 🟡 Minor

Potential off-by-one: missing null terminator in size calculation.

The comment states "size is key + '='" but Windows _MAX_ENV includes the null terminator. For unsetenv, Windows internally sets "KEY=\0", so the size should be key.len() + 2 (key + '=' + '\0'), not key.len() + 1.

// For unsetenv, size is key + '=' (no value, just clearing) #[cfg(windows)] - check_env_var_len(key.len() + 1, vm)?; + check_env_var_len(key.len() + 2, vm)?; // key + '=' + '\0'

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

// For unsetenv, size is key + '=' (no value, just clearing)

#[cfg(windows)]

check_env_var_len(key.len() + 1, vm)?;

// For unsetenv, size is key + '=' (no value, just clearing)

#[cfg(windows)]

check_env_var_len(key.len() + 2, vm)?; // key + '=' + '\0'

🤖 Prompt for AI Agents

In crates/vm/src/stdlib/os.rs around lines 483 to 485, the size computed for unsetenv on Windows omits the null terminator; change the Windows branch to pass key.len() + 2 (key + '=' + '\0') into check_env_var_len instead of key.len() + 1 so the length accounts for the terminating NUL when validating against _MAX_ENV.

youknowone added 3 commits December 9, 2025 19:24

is_dir/is_file for windows

d82577a

listmount/listvolume

2e038cf

check_env_var_len

2dab1f6

youknowone force-pushed the more-windows branch from ac19e43 to 2dab1f6 Compare December 9, 2025 10:24

youknowone changed the title ~~nt is_dir,is_file,listmount,listvolume,terminalsize~~ nt is_dir,is_file,listmount,listvolume Dec 9, 2025

youknowone marked this pull request as ready for review December 9, 2025 10:24

coderabbitai bot reviewed Dec 9, 2025

View reviewed changes

youknowone merged commit a99164f into RustPython:main Dec 9, 2025
13 checks passed

youknowone deleted the more-windows branch December 9, 2025 11:08

This was referenced Dec 9, 2025

nt.terminalsize #6374

Merged

new_last_{os,errno}_error #6381

Merged

test_os, test_io on windows #6379

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

nt is_dir,is_file,listmount,listvolume #6373

nt is_dir,is_file,listmount,listvolume #6373

Uh oh!

youknowone commented Dec 9, 2025 •

edited by coderabbitai bot

Loading

Uh oh!

coderabbitai bot commented Dec 9, 2025 •

edited

Loading

Uh oh!

coderabbitai bot left a comment

Uh oh!

coderabbitai bot Dec 9, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

nt is_dir,is_file,listmount,listvolume #6373

nt is_dir,is_file,listmount,listvolume #6373

Uh oh!

Conversation

youknowone commented Dec 9, 2025 • edited by coderabbitai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Release Notes

Uh oh!

coderabbitai bot commented Dec 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Possibly related PRs

Poem

Pre-merge checks and finishing touches

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot Dec 9, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

youknowone commented Dec 9, 2025 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Dec 9, 2025 •

edited

Loading