Actionable comments posted: 1

crates/stdlib/src/ssl/compat.rs (2)

368-381: ⚡ Quick win

Using .expect() on fallible operations in error handling path.

Lines 369-370 use .expect("unlikely to happen") on create_pem_ssl_error. While attribute-setting failures are indeed rare (typically only on OOM), panicking in an error-handling path is undesirable. Consider using .unwrap_or_else() to fall back to a simpler error without attributes, matching the pattern used elsewhere in this file (e.g., line 386).

♻️ Proposed fix to avoid panic in error path

-            Self::PemLib(msg) => Self::create_pem_ssl_error(vm, format!("PEM lib: {msg}"))
-                .expect("unlikely to happen"),
+            Self::PemLib(msg) => Self::create_pem_ssl_error(vm, format!("PEM lib: {msg}"))
+                .unwrap_or_else(|_| Self::create_plain_ssl_error(vm, format!("PEM lib: {msg}"))),

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/stdlib/src/ssl/compat.rs` around lines 368 - 381, The PemLib arm
panics by calling .expect(...) on the fallible create_pem_ssl_error; change it
to handle failure like other arms by using unwrap_or_else to fall back to a
simpler error (e.g., call Self::create_plain_ssl_error with a "PEM lib: {msg}"
message) so create_pem_ssl_error(vm, format!("PEM lib:
{msg}")).unwrap_or_else(|_| Self::create_plain_ssl_error(vm, format!("PEM lib:
{msg}"))). Ensure you update the Self::PemLib match arm to use unwrap_or_else
and reference create_pem_ssl_error and create_plain_ssl_error accordingly.

---

225-252: 💤 Low value

Audit rustls::PeerIncompatible coverage vs rustls 0.23 variants (crates/stdlib/src/ssl/compat.rs, from_rustls match ~225-252)

The mapping only covers some rustls::PeerIncompatible variants in rustls 0.23; the _ => arm currently handles other existing variants (e.g., ExtendedMasterSecretExtensionRequired, IncorrectCertificateTypeExtension, NullCompressionRequired, ServerSentHelloRetryRequestWithUnknownExtension, UnsolicitedCertificateTypeExtension, ServerRejectedEncryptedClientHello(...)) by falling back to Self::Ssl(format!("peer is incompatible: {peer_err:?}")) rather than an OpenSSL reason code. Add explicit arms for the remaining variants or document that this generic fallback is intentional (since the enum is non-exhaustive).

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/stdlib/src/ssl/compat.rs` around lines 225 - 252, The match over
rustls::PeerIncompatible in from_rustls currently falls through to a generic
Ssl(...) fallback for many 0.23 variants; update the match to explicitly handle
the remaining PeerIncompatible variants (e.g.,
ExtendedMasterSecretExtensionRequired, IncorrectCertificateTypeExtension,
NullCompressionRequired, ServerSentHelloRetryRequestWithUnknownExtension,
UnsolicitedCertificateTypeExtension, ServerRejectedEncryptedClientHello, etc.)
and map each to the appropriate OpenSSL reason constant using
Self::OpenSslReason(SSL_R_...) where sensible, or if you intentionally want a
generic fallback keep the fallback but add a comment documenting that the enum
is non_exhaustive and the generic Self::Ssl(format!(...)) is deliberate; touch
the match in from_rustls so all current PeerIncompatible variants are either
explicitly mapped to an SSL_R_* constant (using the same style as existing arms)
or a documented fallback is left.

crates/vm/src/vm/mod.rs (1)

2378-2401: ⚡ Quick win

Cover the first-load hook path too.

This test only exercises the sys.modules cache-hit case because sys is already imported before Line 2383. The downstream ssl hook in crates/stdlib/src/rustls.rs:164-177 is registered during module execution and depends on the initial _find_and_load path, so a regression there would currently slip through.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/vm/src/vm/mod.rs` around lines 2378 - 2401, The test only covers the
cache-hit path; update module_loaded_hook_can_patch_imported_module to also
exercise the initial load path by registering the hook (mark_module_loaded)
before ensuring the target module is not present in sys.modules and then
importing it so the interpreter takes the _find_and_load path; specifically, in
the vm.enter closure call vm.register_module_loaded_hook("sys",
mark_module_loaded) then remove the "sys" entry from sys.modules (via the VM sys
module/object APIs) or choose a fresh module name that is guaranteed not to be
pre-imported, then call vm.import(...) to trigger the initial-load hook and
assert the "__rustpython_module_loaded_hook_ran__" attribute as the existing
test does (referencing module_loaded_hook_can_patch_imported_module,
mark_module_loaded, vm.register_module_loaded_hook, and vm.import).

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@crates/vm/src/convert/rust_py_serde.rs`:
- Around line 13-17: The code currently uses unimplemented!() for unit and
unit-struct handling inside RustPySerDe which causes a panic; instead implement
proper Serde handling that returns a Result (so errors are converted to Python
exceptions via with_serde_conf()) and treat unit/unit-struct as Python None: in
the serializer paths for unit/unit_struct return Ok(vm.ctx.none().into()) (or
the equivalent PyObject/Value representing None) and in the deserializer/visitor
paths accept Python None and return Ok(()) (or the appropriate unit value)
rather than panicking; update the functions/match arms that reference
unit/unit_struct in RustPySerDe so they return Result-based values and use the
existing with_serde_conf/error helpers to map failures to Python exceptions.

---

Nitpick comments:
In `@crates/stdlib/src/ssl/compat.rs`:
- Around line 368-381: The PemLib arm panics by calling .expect(...) on the
fallible create_pem_ssl_error; change it to handle failure like other arms by
using unwrap_or_else to fall back to a simpler error (e.g., call
Self::create_plain_ssl_error with a "PEM lib: {msg}" message) so
create_pem_ssl_error(vm, format!("PEM lib: {msg}")).unwrap_or_else(|_|
Self::create_plain_ssl_error(vm, format!("PEM lib: {msg}"))). Ensure you update
the Self::PemLib match arm to use unwrap_or_else and reference
create_pem_ssl_error and create_plain_ssl_error accordingly.
- Around line 225-252: The match over rustls::PeerIncompatible in from_rustls
currently falls through to a generic Ssl(...) fallback for many 0.23 variants;
update the match to explicitly handle the remaining PeerIncompatible variants
(e.g., ExtendedMasterSecretExtensionRequired, IncorrectCertificateTypeExtension,
NullCompressionRequired, ServerSentHelloRetryRequestWithUnknownExtension,
UnsolicitedCertificateTypeExtension, ServerRejectedEncryptedClientHello, etc.)
and map each to the appropriate OpenSSL reason constant using
Self::OpenSslReason(SSL_R_...) where sensible, or if you intentionally want a
generic fallback keep the fallback but add a comment documenting that the enum
is non_exhaustive and the generic Self::Ssl(format!(...)) is deliberate; touch
the match in from_rustls so all current PeerIncompatible variants are either
explicitly mapped to an SSL_R_* constant (using the same style as existing arms)
or a documented fallback is left.

In `@crates/vm/src/vm/mod.rs`:
- Around line 2378-2401: The test only covers the cache-hit path; update
module_loaded_hook_can_patch_imported_module to also exercise the initial load
path by registering the hook (mark_module_loaded) before ensuring the target
module is not present in sys.modules and then importing it so the interpreter
takes the _find_and_load path; specifically, in the vm.enter closure call
vm.register_module_loaded_hook("sys", mark_module_loaded) then remove the "sys"
entry from sys.modules (via the VM sys module/object APIs) or choose a fresh
module name that is guaranteed not to be pre-imported, then call vm.import(...)
to trigger the initial-load hook and assert the
"__rustpython_module_loaded_hook_ran__" attribute as the existing test does
(referencing module_loaded_hook_can_patch_imported_module, mark_module_loaded,
vm.register_module_loaded_hook, and vm.import).

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Run ID: 0ca7babc-3895-4256-8a71-a9ba266cfb79

Reviewing files that changed from the base of the PR and between 885cf5c and a3ef4a9.

• Cargo.lock is excluded by !**/*.lock
• Lib/test/test_asyncio/test_events.py is excluded by !Lib/**
• Lib/test/test_asyncio/test_sendfile.py is excluded by !Lib/**
• Lib/test/test_asyncio/test_ssl.py is excluded by !Lib/**
• Lib/test/test_ssl.py is excluded by !Lib/**

• Cargo.toml
• crates/stdlib/Cargo.toml
• crates/stdlib/src/lib.rs
• crates/stdlib/src/rustls-data/obj_mac.num
• crates/stdlib/src/rustls-data/objects.txt
• crates/stdlib/src/rustls.rs
• crates/stdlib/src/socket.rs
• crates/stdlib/src/ssl.rs
• crates/stdlib/src/ssl/cert.rs
• crates/stdlib/src/ssl/compat.rs
• crates/stdlib/src/ssl/error.rs
• crates/stdlib/src/ssl/oid.rs
• crates/stdlib/src/ssl/providers.rs
• crates/vm/Cargo.toml
• crates/vm/src/convert/mod.rs
• crates/vm/src/convert/rust_py_serde.rs
• crates/vm/src/import.rs
• crates/vm/src/vm/interpreter.rs
• crates/vm/src/vm/mod.rs
• examples/custom_tls_providers.rs
• extra_tests/snippets/stdlib_ssl_bio_unencrypted_trailer.py
• src/interpreter.rs

• crates/stdlib/src/ssl/error.rs
• crates/stdlib/src/ssl/oid.rs
• crates/stdlib/src/ssl/cert.rs

TYSM for working on this!

This will become a lot easier to maintain:)

I have few nitpicks