Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

crates/vm/src/stdlib/codecs.rs (4)

237-316: Non-strict error modes lack proper implementation.

The function only implements "strict" error handling (lines 308-312). Non-strict modes ("ignore", "replace", "xmlcharrefreplace", "backslashreplace") silently receive Windows default behavior rather than Python codec semantics, and invalid error mode names are not validated.

As per past review comments, proper error mode implementations should:

• Validate the errors parameter against allowed modes
• Implement "ignore" to skip unmappable characters
• Implement "replace" with appropriate substitution
• Implement "backslashreplace" and "xmlcharrefreplace" with proper escape sequences

---

439-518: Extract shared implementation to eliminate duplication.

This function duplicates mbcs_encode (lines 237-316), differing only in the code page constant (CP_OEMCP vs CP_ACP) and error messages. Additionally, it has the same incomplete error handling for non-strict modes.

As per past review comments and coding guidelines, extract a shared helper function:

fn windows_encode(
    s: &str,
    char_len: usize,
    errors: &str,
    code_page: u32,
    codec_name: &str,
    vm: &VirtualMachine,
) -> PyResult<(Vec<u8>, usize)> {
    // Shared implementation with code_page parameter
}

Then call from both mbcs_encode with CP_ACP and oem_encode with CP_OEMCP.

---

538-624: Extract shared implementation and fix unused errors parameter.

This function duplicates mbcs_decode (lines 336-422), differing only in the code page constant. Additionally, it has the same unused _errors parameter (line 545).

As per past review comments and coding guidelines, extract a shared helper:

fn windows_decode(
    data: &[u8],
    code_page: u32,
    errors: &str,
    codec_name: &str,
    vm: &VirtualMachine,
) -> PyResult<(String, usize)> {
    // Shared implementation
}

Call from both functions with appropriate code page constants. Also remove the underscore prefix from the errors parameter and use it to control decode behavior.

---

336-422: Unused errors parameter prevents proper error handling control.

The _errors parameter (line 343) is extracted but never used to control decode behavior. The function unconditionally tries MB_ERR_INVALID_CHARS first (line 355) and falls back to flags=0 (line 368), regardless of the user's requested error mode. This means users cannot select strict-only behavior or other specific error handling strategies.

Consider removing the underscore prefix and using the errors value to control whether to attempt strict validation or directly use replacement behavior:

-    let _errors = args.errors.as_ref().map(|s| s.as_str()).unwrap_or("strict");
+    let errors = args.errors.as_ref().map(|s| s.as_str()).unwrap_or("strict");

Then use errors to determine flags:

• If errors == "strict", use only MB_ERR_INVALID_CHARS and fail on invalid sequences
• Otherwise, use flags=0 for replacement behavior

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

324-334: Consider implementing the final parameter.

The r#final parameter is currently unused (line 333). While this matches the function signature for codec compatibility, consider whether incremental decoding support should be implemented for completeness.

---

430-437: LGTM with note on duplication.

The OemEncodeArgs struct is identical to MbcsEncodeArgs. Consider whether these could be consolidated if the encode functions are refactored to share implementation.

---

526-536: LGTM with duplication note.

The OemDecodeArgs struct duplicates MbcsDecodeArgs. If decode functions are refactored to share implementation, these structs could potentially be consolidated.

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro