Actionable comments posted: 0

🧹 Nitpick comments (2)

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

206-223: Verify handle lifetime and error semantics for standard vs non-standard fds.

Two observations:

1. For non-standard fds (lines 214-218), the BorrowedHandle from crt_fd::as_handle goes out of scope after extracting the raw handle value. This is safe because the raw value remains valid as long as the underlying fd is open, consistent with similar patterns elsewhere in this file.

2. For the null/invalid handle check (lines 221-223): when GetStdHandle returns NULL (no associated handle), calling errno_err may report a stale or misleading error since no Windows error was set. Consider setting a specific error or differentiating between the two failure modes for clearer diagnostics.

         if h.is_null() || h == INVALID_HANDLE_VALUE {
+            // Note: NULL means no console attached (not an error condition per se),
+            // while INVALID_HANDLE_VALUE indicates a true error
             return Err(errno_err(vm));
         }

---

257-261: Consider guarding against invalid window rectangles.

The srWindow coordinates are i16 (SHORT). If Right < Left or Bottom < Top (malformed rect), the subtraction yields a negative value that wraps to a large usize. While Windows should never return such a rect, a defensive check avoids surprising behavior in edge cases.

         let csbi = unsafe { csbi.assume_init() };
         let w = csbi.srWindow;
-        let columns = (w.Right - w.Left + 1) as usize;
-        let lines = (w.Bottom - w.Top + 1) as usize;
+        let columns = (w.Right.saturating_sub(w.Left).saturating_add(1)) as usize;
+        let lines = (w.Bottom.saturating_sub(w.Top).saturating_add(1)) as usize;
         Ok(_os::TerminalSizeData { columns, lines })

Alternatively, if matching CPython's exact behavior is preferred, the current implementation is acceptable.

📜 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 6680edb.

📒 Files selected for processing (1)

• crates/vm/src/stdlib/nt.rs (1 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/vm/src/stdlib/nt.rs

🧬 Code graph analysis (1) crates/vm/src/stdlib/nt.rs (3)

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

• GetStdHandle (82-96)
• GetLastError (172-174)
• std (444-444)

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

• as_handle (341-345)

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

• errno_err (39-41)
• std (1111-1111)

🔇 Additional comments (1)

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

235-256: CONOUT$ fallback logic is well-structured.

The fallback path correctly:

• Opens CONOUT$ with appropriate access flags
• Retries the screen buffer info query
• Ensures CloseHandle is called before checking the result

This approach matches CPython's behavior for handling redirected stdout scenarios.

Actionable comments posted: 0

🧹 Nitpick comments (1)

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

206-215: Consider removing the redundant INVALID_HANDLE_VALUE check.

The crt_fd::as_handle() function returns io::Result<BorrowedHandle<'_>>, which means it already validates the handle internally. The explicit check for INVALID_HANDLE_VALUE on lines 213-215 is redundant because as_handle() would have returned an error if the handle were invalid.

Apply this diff to remove the redundant check:

 let fd = fd.unwrap_or(1); // default to stdout

 // Use _get_osfhandle for all fds
 let borrowed = unsafe { crt_fd::Borrowed::borrow_raw(fd) };
 let handle = crt_fd::as_handle(borrowed).map_err(|e| e.to_pyexception(vm))?;
 let h = handle.as_raw_handle() as Foundation::HANDLE;

-if h == INVALID_HANDLE_VALUE {
-    return Err(errno_err(vm));
-}
-
 let mut csbi = MaybeUninit::uninit();

📜 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 6680edb and 4992e7e.

📒 Files selected for processing (1)

• crates/vm/src/stdlib/nt.rs (1 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/vm/src/stdlib/nt.rs

🧬 Code graph analysis (1) crates/vm/src/stdlib/nt.rs (3)

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

• as_handle (341-345)

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

• errno_err (39-41)
• std (1131-1131)

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

• GetLastError (172-174)
• std (444-444)

⏰ 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: Check the WASM package and demo
• GitHub Check: Check Rust code with clippy
• GitHub Check: Run snippets and cpython tests (windows-latest)
• GitHub Check: Run snippets and cpython tests (ubuntu-latest)
• GitHub Check: Run snippets and cpython tests (macos-latest)
• GitHub Check: Ensure compilation on various targets
• GitHub Check: Run rust tests (ubuntu-latest)
• GitHub Check: Run rust tests (macos-latest)
• GitHub Check: Run rust tests (windows-latest)

🔇 Additional comments (3)

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

217-226: LGTM! Proper error discrimination.

The error handling correctly distinguishes between ERROR_ACCESS_DENIED (which requires a fallback mechanism) and other errors (which should be propagated immediately). This matches the expected Windows console behavior where a handle might not have read permissions.

---

227-248: LGTM! Proper fallback implementation.

The fallback correctly opens CONOUT$ (the Windows console device) when ERROR_ACCESS_DENIED is encountered, retrieves the console buffer info, and properly closes the handle afterward. The resource management is correct, and the error propagation is appropriate.

---

249-253: LGTM! Correct terminal size computation.

The use of assume_init() is safe here since we only reach this point after a successful GetConsoleScreenBufferInfo call. The window dimension calculations correctly handle inclusive bounds: (Right - Left + 1) for columns and (Bottom - Top + 1) for lines.

Actionable comments posted: 0

🧹 Nitpick comments (1)

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

238-243: Consider saving the error before CloseHandle to preserve the correct error code.

If GetConsoleScreenBufferInfo fails and then CloseHandle also fails (rare but possible), the error code from GetLastError() captured by errno_err would be from CloseHandle rather than from the actual console operation.

         let ret =
             unsafe { Console::GetConsoleScreenBufferInfo(console_handle, csbi.as_mut_ptr()) };
+        let err = if ret == 0 {
+            Some(errno_err(vm))
+        } else {
+            None
+        };
         unsafe { Foundation::CloseHandle(console_handle) };
-        if ret == 0 {
-            return Err(errno_err(vm));
+        if let Some(e) = err {
+            return Err(e);
         }

📜 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 4992e7e and aafa930.

⛔ Files ignored due to path filters (1)

• Lib/test/test_subprocess.py is excluded by !Lib/**

📒 Files selected for processing (1)

• crates/vm/src/stdlib/nt.rs (1 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/vm/src/stdlib/nt.rs

🧬 Code graph analysis (1) crates/vm/src/stdlib/nt.rs (3)

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

• as_handle (341-345)

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

• GetLastError (172-174)
• std (444-444)

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

• errno_err (39-41)
• std (1131-1131)

⏰ 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). (11)

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

🔇 Additional comments (2)

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

206-211: LGTM!

The unified fd handling approach is clean. Defaulting to stdout (fd 1), using crt_fd::as_handle to get the OS handle, and properly propagating errors via to_pyexception aligns with the existing patterns in this file.

---

245-249: LGTM!

The terminal size computation correctly uses srWindow bounds, and the assume_init is safely guarded by the control flow ensuring GetConsoleScreenBufferInfo succeeded. This matches CPython's approach for computing visible console dimensions.

Actionable comments posted: 0

🧹 Nitpick comments (2)

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

223-240: Consider using RAII for console handle resource management.

The console_handle is manually opened and closed, which could leak the handle if a panic occurs between lines 234 and 240. While panics are unlikely in this code path, following Rust best practices would involve wrapping the handle in an RAII type with a Drop implementation.

Additionally, GetConsoleScreenBufferInfo only requires GENERIC_READ access, so GENERIC_WRITE on line 227 may be unnecessary.

Consider wrapping the handle:

struct ConsoleHandle(Foundation::HANDLE);

impl Drop for ConsoleHandle {
    fn drop(&mut self) {
        unsafe { Foundation::CloseHandle(self.0) };
    }
}

Then use:

-            let console_handle = unsafe {
+            let console_handle = ConsoleHandle(unsafe {
                 FileSystem::CreateFileW(
                     conout.as_ptr(),
-                    Foundation::GENERIC_READ | Foundation::GENERIC_WRITE,
+                    Foundation::GENERIC_READ,
                     FileSystem::FILE_SHARE_READ | FileSystem::FILE_SHARE_WRITE,
                     std::ptr::null(),
                     FileSystem::OPEN_EXISTING,
                     0,
                     std::ptr::null_mut(),
                 )
-            };
+            });
             if console_handle == INVALID_HANDLE_VALUE {
                 return Err(errno_err(vm));
             }
             let ret =
-                unsafe { Console::GetConsoleScreenBufferInfo(console_handle, csbi.as_mut_ptr()) };
-            unsafe { Foundation::CloseHandle(console_handle) };
+                unsafe { Console::GetConsoleScreenBufferInfo(console_handle.0, csbi.as_mut_ptr()) };

Based on coding guidelines, Rust best practices for resource management should be followed.

---

245-249: Optional: Add defensive bounds checking for window coordinates.

The calculation assumes w.Right >= w.Left and w.Bottom >= w.Top. While the Windows API should always return valid bounds, adding defensive checks would make the code more robust against invalid data.

         let csbi = unsafe { csbi.assume_init() };
         let w = csbi.srWindow;
+        if w.Right < w.Left || w.Bottom < w.Top {
+            return Err(vm.new_os_error("Invalid console screen buffer dimensions".to_string()));
+        }
         let columns = (w.Right - w.Left + 1) as usize;
         let lines = (w.Bottom - w.Top + 1) as usize;

📜 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 aafa930 and 9acfc46.

📒 Files selected for processing (1)

• crates/vm/src/stdlib/nt.rs (1 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/vm/src/stdlib/nt.rs

🧬 Code graph analysis (1) crates/vm/src/stdlib/nt.rs (5)

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

• handle (188-190)

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

• as_handle (341-345)

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

• GetLastError (172-174)
• std (444-444)
• CloseHandle (77-79)

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

• errno_err (39-41)
• std (1131-1131)

crates/vm/src/windows.rs (3)

• std (410-410)
• std (463-463)
• std (474-474)

🔇 Additional comments (1)

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

206-212: LGTM! Handle derivation approach is sound.

The unified approach to derive a Windows handle from the file descriptor is well-implemented. Defaulting to stdout (fd=1) is reasonable, and the error handling properly converts I/O errors to Python exceptions.