Actionable comments posted: 5

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@crates/capi/src/pylifecycle.rs`:
- Around line 31-35: The current Py_InitializeEx function panics when
INITIALIZED.is_completed() is true, but per the CPython C API it must be a no-op
on repeated calls; modify Py_InitializeEx (and any Py_Initialize wrapper) to
return immediately when INITIALIZED.is_completed() is true instead of panicking
so repeated FFI calls do not abort the process, preserving existing
initialization semantics and side-effects only on the first successful
initialization.
- Around line 63-65: Py_FinalizeEx currently returns success but doesn't update
initialization state because the code uses a non-resettable Once, so
Py_IsInitialized() stays true; replace the Once-based mechanism with a
resettable atomic state (e.g., an AtomicBool named INITIALIZED) and update all
lifecycle functions: set INITIALIZED.store(true, Ordering::SeqCst) in
Py_InitializeEx (and any init paths), set INITIALIZED.store(false,
Ordering::SeqCst) in Py_FinalizeEx before returning, and have Py_IsInitialized()
read INITIALIZED.load(Ordering::SeqCst); ensure thread-safe Ordering and update
or remove the Once symbol usage so no code still relies on the old Once
primitive.

In `@crates/capi/src/pystate.rs`:
- Around line 36-39: The exported PyEval_SaveThread currently returns null which
breaks the C API contract and will corrupt callers using
Py_BEGIN_ALLOW_THREADS/PyEval_RestoreThread; either implement proper
SaveThread/RestoreThread semantics or stop exporting it. To fix: in
PyEval_SaveThread implement logic that captures the current thread's
PyThreadState (returning a non-null pointer token representing the saved state)
and ensure PyEval_RestoreThread accepts that token to restore thread state,
using the existing PyThreadState type and thread-local VM utilities in
pystate.rs; if you cannot implement correct semantics now, remove the
#[no_mangle]/pub extern "C" export for PyEval_SaveThread (and any paired
PyEval_RestoreThread export) so the unstable API is not exposed. Ensure the
chosen change keeps the stable ABI consistent with callers expecting a valid
non-null PyThreadState pointer.
- Around line 26-34: PyGILState_Ensure currently always returns 0 and
PyGILState_Release is a no-op; implement proper per-thread recursion/state
tracking so each PyGILState_Ensure returns a handle encoding the prior state (or
push the prior state on a per-thread stack in the same thread-local VM storage)
and PyGILState_Release(_state) pops/reads that handle to restore the exact
previous state (including detaching the VM if the matching Ensure attached it);
update the thread-local VM storage to track recursion depth/stack and use those
symbols (PyGILState_Ensure, PyGILState_Release, the existing thread-local VM
storage) when implementing the push/encode-on-Ensure and restore/pop-on-Release
behavior.

In `@crates/capi/src/refcount.rs`:
- Around line 5-17: The exported refcount functions currently accept raw
pointers but are declared callable from safe Rust; change both exports to be
unsafe extern "C" functions and restore the normal no_mangle attribute (replace
#[unsafe(no_mangle)] with #[no_mangle]) so the non-null/valid-pointer contract
is encoded in the signature; update _Py_DecRef and _Py_IncRef signatures to pub
unsafe extern "C" fn _Py_DecRef(op: *mut PyObject) and pub unsafe extern "C" fn
_Py_IncRef(op: *mut PyObject), remove the clippy allow on
not_unsafe_ptr_arg_deref, and adjust the bodies to use NonNull/new or unwrap
patterns consistent with other FFI functions so dereferencing/reconstructing
ownership is only allowed in unsafe code paths.

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: c1193732-a759-4d95-962e-bf28e2d74df2

Reviewing files that changed from the base of the PR and between d5921d1 and 0a01cb4.

• Cargo.lock is excluded by !**/*.lock

• crates/capi/.cargo/config.toml
• crates/capi/Cargo.toml
• crates/capi/pyo3-rustpython.config
• crates/capi/src/lib.rs
• crates/capi/src/pylifecycle.rs
• crates/capi/src/pystate.rs
• crates/capi/src/refcount.rs

Actionable comments posted: 1

crates/capi/src/refcount.rs (1)

5-17: ⚠️ Potential issue | 🟠 Major

Encode the raw-pointer contract in the function signatures.

These exports still dereference/reconstruct ownership from op while being callable from safe Rust. Make both functions pub unsafe extern "C" fn, add # Safety docs, and remove the clippy::not_unsafe_ptr_arg_deref suppressions; keep the C ABI symbol unchanged.

Proposed fix

 #[unsafe(no_mangle)]
-#[allow(clippy::not_unsafe_ptr_arg_deref)]
-pub extern "C" fn _Py_DecRef(op: *mut PyObject) {
+/// # Safety
+///
+/// `op` must be a valid, non-null pointer to a live `PyObject` with an owned reference.
+pub unsafe extern "C" fn _Py_DecRef(op: *mut PyObject) {
     // By dropping PyObjectRef, we will decrement the reference count.
     unsafe { PyObjectRef::from_raw(NonNull::new_unchecked(op)) };
 }
 
 #[unsafe(no_mangle)]
-#[allow(clippy::not_unsafe_ptr_arg_deref)]
-pub extern "C" fn _Py_IncRef(op: *mut PyObject) {
+/// # Safety
+///
+/// `op` must be a valid, non-null pointer to a live `PyObject`.
+pub unsafe extern "C" fn _Py_IncRef(op: *mut PyObject) {
     // Don't drop the owned value, as we just want to increment the refcount.
     core::mem::forget(unsafe { (*op).to_owned() });
 }

Run this read-only check to verify the signatures and lint suppression are removed:

#!/bin/bash
# Description: Verify C-ABI refcount exports encode unsafe raw-pointer contracts.

rg -n -C3 '(_Py_DecRef|_Py_IncRef|clippy::not_unsafe_ptr_arg_deref|pub\s+extern\s+"C"\s+fn\s+_Py_)' crates/capi/src/refcount.rs

As per coding guidelines, **/*.rs should follow Rust best practices for error handling and memory management.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/capi/src/refcount.rs` around lines 5 - 17, Change both exports to
encode the raw-pointer contract by making the signatures pub unsafe extern "C"
fn _Py_DecRef(op: *mut PyObject) and pub unsafe extern "C" fn _Py_IncRef(op:
*mut PyObject), remove the #[allow(clippy::not_unsafe_ptr_arg_deref)]
attributes, and keep the no-mangle C ABI symbol. Add concise # Safety doc
comments to each function stating the caller must pass a non-null, properly
aligned, valid PyObject pointer and that ownership/refcount semantics apply to
operations using PyObjectRef::from_raw and (*op).to_owned(). Ensure the body
logic (using PyObjectRef::from_raw in _Py_DecRef and
core::mem::forget((*op).to_owned()) in _Py_IncRef) remains the same but is now
behind an unsafe function signature.

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@crates/capi/Cargo.toml`:
- Line 16: Update the dependency declaration for rustpython-stdlib to be
optional and feature-gated: change the rustpython-stdlib entry so it includes
optional = true and preserves features = ["threading"] (i.e., make
rustpython-stdlib optional), and verify the existing stdlib feature still lists
"rustpython-stdlib" so enabling the stdlib feature pulls in the optional
dependency; adjust the dependency line accordingly and keep the stdlib feature
target unchanged.

---

Duplicate comments:
In `@crates/capi/src/refcount.rs`:
- Around line 5-17: Change both exports to encode the raw-pointer contract by
making the signatures pub unsafe extern "C" fn _Py_DecRef(op: *mut PyObject) and
pub unsafe extern "C" fn _Py_IncRef(op: *mut PyObject), remove the
#[allow(clippy::not_unsafe_ptr_arg_deref)] attributes, and keep the no-mangle C
ABI symbol. Add concise # Safety doc comments to each function stating the
caller must pass a non-null, properly aligned, valid PyObject pointer and that
ownership/refcount semantics apply to operations using PyObjectRef::from_raw and
(*op).to_owned(). Ensure the body logic (using PyObjectRef::from_raw in
_Py_DecRef and core::mem::forget((*op).to_owned()) in _Py_IncRef) remains the
same but is now behind an unsafe function signature.

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: 5fa614e4-9121-4b67-abb1-14a0faa61559

Reviewing files that changed from the base of the PR and between 0a01cb4 and 2b4b20f.

• Cargo.lock is excluded by !**/*.lock

• .cspell.dict/cpython.txt
• crates/capi/.cargo/config.toml
• crates/capi/Cargo.toml
• crates/capi/pyo3-rustpython.config
• crates/capi/src/lib.rs
• crates/capi/src/pylifecycle.rs
• crates/capi/src/pystate.rs
• crates/capi/src/refcount.rs

• .cspell.dict/cpython.txt
• crates/capi/pyo3-rustpython.config
• crates/capi/.cargo/config.toml
• crates/capi/src/lib.rs

• crates/capi/src/pystate.rs
• crates/capi/src/pylifecycle.rs

good idea to split to small patches 👍

Actionable comments posted: 2

crates/capi/src/pylifecycle.rs (1)

7-7: ⚠️ Potential issue | 🟠 Major

Track logical finalization separately from the OnceLock.

Py_FinalizeEx() reports success but leaves MAIN_INTERP populated, so Py_IsInitialized() remains true forever after the first Py_InitializeEx(). If full teardown is deferred, at least keep a resettable lifecycle flag so callers can observe finalize → uninitialized → initialize correctly.

Minimal state-tracking direction

 use crate::pystate::attach_vm_to_thread;
 use core::ffi::c_int;
 use rustpython_vm::Interpreter;
 use rustpython_vm::vm::thread::ThreadedVirtualMachine;
-use std::sync::{Mutex, OnceLock};
+use std::sync::{
+    atomic::{AtomicBool, Ordering},
+    Mutex, OnceLock,
+};

 static MAIN_INTERP: OnceLock<Mutex<Interpreter>> = OnceLock::new();
+static INITIALIZED: AtomicBool = AtomicBool::new(false);
@@
 pub extern "C" fn Py_IsInitialized() -> c_int {
-    MAIN_INTERP.get().is_some() as c_int
+    INITIALIZED.load(Ordering::SeqCst) as c_int
 }
@@
 pub extern "C" fn Py_InitializeEx(_initsigs: c_int) {
     MAIN_INTERP.get_or_init(|| Interpreter::with_init(Default::default(), |_vm| {}).into());
+    INITIALIZED.store(true, Ordering::SeqCst);

     attach_vm_to_thread();
 }
@@
 pub extern "C" fn Py_FinalizeEx() -> c_int {
+    INITIALIZED.store(false, Ordering::SeqCst);
     0
 }

Also applies to: 20-21, 42-43

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/capi/src/pylifecycle.rs` at line 7, MAIN_INTERP (a
OnceLock<Mutex<Interpreter>>) is never cleared so Py_IsInitialized() stays true
after Py_FinalizeEx(); introduce a separate resettable lifecycle flag (e.g., an
AtomicBool like PY_INITIALIZED) and update it to true in Py_InitializeEx and
false in Py_FinalizeEx, then have Py_IsInitialized read that flag instead of the
OnceLock; keep MAIN_INTERP as-is (do not rely on dropping it) so callers see a
correct initialized/finalized lifecycle without changing the OnceLock usage.

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@crates/vm/src/stdlib/_imp.rs`:
- Around line 79-96: Update the call sites that invoke the fork helper functions
so their cfg gating matches the exports: wrap the calls to
acquire_imp_lock_for_fork(), after_fork_child_imp_lock_release(),
reinit_imp_lock_after_fork(), and release_imp_lock_after_fork_parent() with
cfg(all(unix, feature = "threading", feature = "host_env")) so they are only
compiled when those functions are exported; locate the call sites where those
four symbols are used and replace the existing cfg checks (e.g., feature =
"threading" or all(unix, feature = "threading")) with the new all(unix, feature
= "threading", feature = "host_env") gate.

In `@crates/vm/src/stdlib/_io.rs`:
- Around line 5-6: The call-site for reinit_std_streams_after_fork in posix.rs
is only guarded by #[cfg(feature = "threading")] but the symbol is exported in
_io.rs under #[cfg(all(unix, feature = "threading", feature = "host_env"))];
change the cfg on the posix.rs invocation to #[cfg(all(unix, feature =
"threading", feature = "host_env"))] so the call is only compiled when the
function is available (search for the reinit_std_streams_after_fork call in
posix.rs to update its cfg).

---

Outside diff comments:
In `@crates/vm/src/stdlib/_thread.rs`:
- Around line 2-9: The build fails because some fork-reinit functions are
defined with #[cfg(all(unix, feature = "threading", feature = "host_env"))] but
are called under only #[cfg(feature = "threading")]; update the cfgs so callers
and definitions match: relax the definition gates for after_fork_child,
reset_weakref_locks_after_fork, the string_pool.reinit_after_fork method, and
codec_registry.reinit_after_fork to use #[cfg(all(unix, feature = "threading"))]
(i.e., remove the feature = "host_env" requirement) so the symbols exist when
threading is enabled, or alternatively add feature = "host_env" to the callers'
cfgs in posix.rs—pick one consistent approach and apply it to all listed
symbols.

---

Duplicate comments:
In `@crates/capi/src/pylifecycle.rs`:
- Line 7: MAIN_INTERP (a OnceLock<Mutex<Interpreter>>) is never cleared so
Py_IsInitialized() stays true after Py_FinalizeEx(); introduce a separate
resettable lifecycle flag (e.g., an AtomicBool like PY_INITIALIZED) and update
it to true in Py_InitializeEx and false in Py_FinalizeEx, then have
Py_IsInitialized read that flag instead of the OnceLock; keep MAIN_INTERP as-is
(do not rely on dropping it) so callers see a correct initialized/finalized
lifecycle without changing the OnceLock usage.

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: c5ea4489-fdf4-4945-bd15-c2b5f021848c

Reviewing files that changed from the base of the PR and between 2b4b20f and ce47be6.

• Cargo.lock is excluded by !**/*.lock

• .cspell.dict/cpython.txt
• crates/capi/.cargo/config.toml
• crates/capi/Cargo.toml
• crates/capi/pyo3-rustpython.config
• crates/capi/src/lib.rs
• crates/capi/src/pylifecycle.rs
• crates/capi/src/pystate.rs
• crates/capi/src/refcount.rs
• crates/vm/src/codecs.rs
• crates/vm/src/intern.rs
• crates/vm/src/object/core.rs
• crates/vm/src/stdlib/_imp.rs
• crates/vm/src/stdlib/_io.rs
• crates/vm/src/stdlib/_thread.rs
• crates/vm/src/vm/thread.rs

• crates/vm/src/vm/thread.rs
• crates/capi/.cargo/config.toml
• crates/capi/pyo3-rustpython.config
• crates/capi/src/lib.rs
• crates/capi/src/refcount.rs
• crates/capi/src/pystate.rs

• .cspell.dict/cpython.txt
• crates/capi/Cargo.toml

Actionable comments posted: 2

crates/capi/src/pylifecycle.rs (1)

43-44: ⚠️ Potential issue | 🟠 Major

Finalize should clear lifecycle state.

Py_FinalizeEx() still returns success without clearing MAIN_INTERP, so Py_IsInitialized() cannot become false after finalization and reinitialization cannot work correctly. This matches the previously flagged lifecycle-state issue and should be fixed together with the state tracking change. The Python lifecycle docs specify that Py_IsInitialized() returns false after Py_FinalizeEx(): https://docs.python.org/3/c-api/interp-lifecycle.html#c.Py_FinalizeEx

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/capi/src/pylifecycle.rs` around lines 43 - 44, Py_FinalizeEx currently
returns success without clearing the global interpreter state; update
Py_FinalizeEx() to clear the lifecycle state by resetting MAIN_INTERP (the
global tracking the main interpreter) to a null/empty state so that
Py_IsInitialized() returns false after finalization, and ensure any associated
cleanup (e.g., freeing interpreter resources or flags referenced by MAIN_INTERP)
runs before returning; locate the function Py_FinalizeEx() and the MAIN_INTERP
symbol to implement the state reset and cleanup so reinitialization works
correctly.

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@crates/capi/src/pylifecycle.rs`:
- Around line 29-34: Py_InitializeEx currently holds the MAIN_INTERP mutex
across attach_vm_to_thread(), which can re-lock MAIN_INTERP via
request_vm_from_interpreter() and deadlock; instead, construct the new
Interpreter (using Interpreter::with_init(...)), store it into MAIN_INTERP and
drop the MAIN_INTERP lock before calling attach_vm_to_thread(); specifically, in
Py_InitializeEx use get_main_interpreter()/MAIN_INTERP only to check/set the
interpreter and ensure the mutex guard is released prior to invoking
attach_vm_to_thread()/request_vm_from_interpreter().
- Around line 19-21: The Py_IsInitialized implementation currently returns the
inverse of the Python C-API contract; update the function Py_IsInitialized to
return get_main_interpreter().is_some() as c_int (instead of is_none()) so it
returns non-zero when an interpreter exists; ensure the change is made in the
Py_IsInitialized function that calls get_main_interpreter().

---

Duplicate comments:
In `@crates/capi/src/pylifecycle.rs`:
- Around line 43-44: Py_FinalizeEx currently returns success without clearing
the global interpreter state; update Py_FinalizeEx() to clear the lifecycle
state by resetting MAIN_INTERP (the global tracking the main interpreter) to a
null/empty state so that Py_IsInitialized() returns false after finalization,
and ensure any associated cleanup (e.g., freeing interpreter resources or flags
referenced by MAIN_INTERP) runs before returning; locate the function
Py_FinalizeEx() and the MAIN_INTERP symbol to implement the state reset and
cleanup so reinitialization works correctly.

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: b3bd9f59-e5ed-4610-9f85-69c32dfcab49

Reviewing files that changed from the base of the PR and between ce47be6 and 9d0000c.

• Cargo.lock is excluded by !**/*.lock

• Cargo.toml
• build.rs
• crates/capi/Cargo.toml
• crates/capi/src/lib.rs
• crates/capi/src/pylifecycle.rs
• src/lib.rs

• crates/capi/Cargo.toml

• crates/capi/src/lib.rs

Actionable comments posted: 1

crates/capi/src/lib.rs (1)

1-1: Narrow the missing_safety_doc suppression or document unsafe C API exports.

The crate-wide #![allow(clippy::missing_safety_doc)] at line 1 suppresses safety documentation requirements for all current and future FFI exports in this module. Multiple public extern "C" functions exist without documented safety contracts. Either add # Safety sections to these functions or narrow the allow to specific stubs that intentionally omit documentation. This aligns with Rust best practices for memory safety and error handling at FFI boundaries.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/capi/src/lib.rs` at line 1, The crate-wide attribute
#![allow(clippy::missing_safety_doc)] at the top suppresses safety docs for all
FFI exports; remove or narrow it and either add /// # Safety sections to each
public extern "C" function (any pub extern "C" fn in this module) describing
caller requirements and UB conditions, or replace the crate-level allow with
targeted #[allow(clippy::missing_safety_doc)] on specific stub items that
intentionally omit safety docs; update each FFI export (the pub extern "C"
functions) with concrete safety contracts or restrict the allow to those exact
symbols so future exports require documentation.

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@crates/capi/src/lib.rs`:
- Line 14: Fix the doc-comment typo in crates/capi/src/lib.rs: change the word
"of" to "if" in the comment for the main interpreter (the comment starting "Get
main interpreter of this process.") so it reads "Get main interpreter of this
process. Will be None if it has not been initialized yet." and ensure the
corrected sentence is the docstring associated with the relevant function or
item that exposes the main interpreter.

---

Nitpick comments:
In `@crates/capi/src/lib.rs`:
- Line 1: The crate-wide attribute #![allow(clippy::missing_safety_doc)] at the
top suppresses safety docs for all FFI exports; remove or narrow it and either
add /// # Safety sections to each public extern "C" function (any pub extern "C"
fn in this module) describing caller requirements and UB conditions, or replace
the crate-level allow with targeted #[allow(clippy::missing_safety_doc)] on
specific stub items that intentionally omit safety docs; update each FFI export
(the pub extern "C" functions) with concrete safety contracts or restrict the
allow to those exact symbols so future exports require documentation.

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: 0f942005-1823-4294-a497-442218c274bb

Reviewing files that changed from the base of the PR and between 9d0000c and 9346fd0.

• Cargo.lock is excluded by !**/*.lock

• Cargo.toml
• build.rs
• crates/capi/Cargo.toml
• crates/capi/src/lib.rs
• crates/capi/src/pylifecycle.rs
• src/lib.rs

• crates/capi/Cargo.toml

• src/lib.rs
• build.rs
• crates/capi/src/pylifecycle.rs

I think that the current state is good for now, this PR already brings lots of value and if we see this being fragile we can think of a solution, atm it seems like we're trying to find a solution for a problem we haven't encountered yet (talking about #7648 (comment))

LGTM!
Just need to satisfy cargo-sheer

This is generated review. Let's clear them in follow-up patches.

• Blocker: crates/capi/src/pylifecycle.rs:44 is a no-op. It is fine to leave
  APIs unimplemented, but exporting a CPython lifecycle symbol while leaving
  the interpreter initialized after Py_FinalizeEx() is incorrect behavior, not
  just an incomplete feature.
• Blocker: crates/capi/src/pystate.rs:40 always returns null. That is unsafe
  for real C API users because callers expect a restorable thread state.
  Either implement SaveThread/RestoreThread together or do not expose this
  symbol yet.
• Major: crates/vm/src/vm/thread.rs:175 does not clean up the C API external
  thread’s frame/thread slot. The normal _thread path calls crates/vm/src/
  stdlib/_thread.rs:578, but the C API release path only drops the owner and
  pops VM_STACK. This can leave stale state visible to _current_frames and
  stop-the-world tracking.
• Major: crates/vm/src/vm/thread.rs:180 drops GILSTATE_VM before popping
  crates/vm/src/vm/thread.rs:183. The intent is understandable, but it exposes
  a VM that is already being dropped as the current VM. Cleanup should happen
  while the VM is still valid, then the stack entry and owner should be
  removed in a well-defined order.
• Major: The PR needs to explicitly frame this as an opaque ABI3-style subset,
  not general CPython ABI compatibility. RustPython’s crates/vm/src/object/
  core.rs:1228 is not CPython’s object layout, so extension compatibility must
  be limited to code that treats PyObject* opaquely through exported APIs.