The Windows filesystem and process surface: PEP 529 paths, OSError.winerror, the noop os calls, and subprocess - #1081
Conversation
|
Warning Review limit reached
Next review available in: 57 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
WalkthroughWindows support now preserves Win32 errors, handles WTF-8 paths, adds filesystem and process APIs, closes duplicated subprocess pipe handles, and expands CPython 3.14 parity coverage. The parity runner now rejects unavailable or mismatched interpreters. ChangesWindows OSError and filesystem model
Windows process support
Parity validation
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 47d40b41be
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| fn environment_block(w_env: PyObjectRef) -> Result<Vec<u16>, crate::PyError> { | ||
| if !unsafe { pyre_object::is_dict(w_env) } { | ||
| return Err(crate::PyError::type_error( | ||
| "environment must be a mapping object", | ||
| )); | ||
| } | ||
| let mut entries = Vec::new(); | ||
| for (w_key, w_value) in unsafe { pyre_object::w_dict_items(w_env) } { |
There was a problem hiding this comment.
On Windows, subprocess.Popen(..., env=os.environ) passes the _Environ mapping directly to _winapi.CreateProcess (subprocess.py:1573-1579). Since _Environ implements MutableMapping but is not a dict, this exact-dict check rejects a standard, valid env argument before launching the child; environment construction needs to iterate the mapping protocol rather than w_dict_items exclusively.
Useful? React with 👍 / 👎.
| "link", | ||
| crate::make_builtin_function("link", |args| { | ||
| if args.len() < 2 { | ||
| return Err(crate::PyError::type_error("link() requires 2 arguments")); | ||
| } | ||
| let src = crate::gateway::fsencode_path_w(args[0])?; |
There was a problem hiding this comment.
When os.link is called with src_dir_fd or dst_dir_fd, the builtin kwargs marker is appended to args, but this closure reads only the first two entries and silently invokes cwd-relative libc::link. A caller can therefore link a same-named file from the process cwd instead of the directory descriptor it supplied; the upstream implementation uses linkat when these arguments are present, or requires NotImplementedError when unavailable. The Windows override at lines 6043-6071 drops the same keywords.
AGENTS.md reference: AGENTS.md:L231-L233
Useful? React with 👍 / 👎.
| let path = crate::gateway::fsencode_path_w(args[0])?; | ||
| let length = crate::baseobjspace::int_w(args[1])? as libc::off_t; |
There was a problem hiding this comment.
Both new truncate implementations pass the first argument through the path-only fsencode_path_w, so os.truncate(fd, length) raises TypeError instead of truncating the open descriptor. The upstream truncate explicitly detects an integer argument and routes it through ftruncate (pypy/module/posix/interp_posix.py:414-427); the Windows implementation repeats the regression at lines 5939-5945.
AGENTS.md reference: AGENTS.md:L231-L233
Useful? React with 👍 / 👎.
| fn winerror_derived_errno(w_winerror: PyObjectRef) -> Option<i64> { | ||
| if !unsafe { pyre_object::is_int(w_winerror) } { | ||
| return None; | ||
| } | ||
| Some(winerror_to_errno(unsafe { | ||
| pyre_object::w_int_get_value(w_winerror) |
There was a problem hiding this comment.
On Windows this treats every integer fourth argument as an active Win32 code, including 0, and leaves truthy non-integers stored later in w_winerror. The ported _parse_init_args instead enters conversion only for a truthy argument, so zero leaves the original errno unchanged, while a truthy value that space.int_w rejects clears w_winerror (pypy/module/exceptions/interp_exceptions.py:565-578). Consequently constructors such as OSError(2, "m", "f", 0) and one with "notint" produce different errno/attribute behavior from upstream.
AGENTS.md reference: AGENTS.md:L231-L233
Useful? React with 👍 / 👎.
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit ede7a44). Files in the reviewed diff1. Regressions to PyPy parity introduced by this patch
2. Other mismatches introduced by this patch
3. Pre-existing mismatches (already present before this patch)
4. Structural adaptationsNone. |
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib-python/3/subprocess.py (1)
1361-1388: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftThis diverges vendored stdlib from CPython to compensate for deferred finalization.
The comment at Lines 1361-1365 states the reason: the
Handlewrapper's__del__runs too late, so the parent keeps the write end of a pipe open and the read never reaches end-of-file. That is a property of this interpreter's object finalization, not ofsubprocess.CPython upstream relies on prompt refcount-driven
Handle.__del__here. Patching_get_handlesand_make_inheritablein the vendored copy hides the finalization gap and creates a permanent divergence that a future stdlib resync will drop without warning.Confirm whether the interpreter can finalize the
Handleat the point the last reference dies. If it can, fix that and revert this file. If it cannot, record the limitation in the divergence notes so the patch survives a resync.Based on coding guidelines: "For root-cause bugs, fix the actual interpreter or JIT issue instead of implementing workarounds such as builtin fallback modules."
Also applies to: 1445-1455
🤖 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 `@lib-python/3/subprocess.py` around lines 1361 - 1388, Verify whether the interpreter promptly finalizes the last-referenced Handle through __del__. If prompt finalization works, remove the subprocess changes around _get_handles and _make_inheritable and restore the vendored CPython implementation; otherwise document the limitation in the project’s divergence notes rather than leaving this workaround undocumented.Source: Coding guidelines
🤖 Prompt for all review comments with 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.
Inline comments:
In `@pyre/extra_tests/parity_tests/oserror_winerror_syscall.py`:
- Around line 113-124: Before the destructive loop over os.close, os.ftruncate,
and os.write, explicitly verify that BAD_FD is closed by attempting an operation
that must fail for an invalid descriptor and asserting the expected OSError.
Keep the existing failure(call, BAD_FD, *args) checks unchanged after this
guard.
In `@pyre/extra_tests/parity_tests/subprocess_launch.py`:
- Around line 163-169: Quote the PY executable in all three os.popen command
strings so paths containing spaces execute correctly on Windows. Update the
commands in the pipe context and both close-status assertions, preserving their
existing arguments and expected behavior.
In `@pyre/pyre-interpreter/src/builtins.rs`:
- Around line 13524-13526: Update the tell implementation’s lseek call to use
crt_call!(libc::lseek(...)) and report failures through
fd_errno_err(crt_errno()), matching the existing seekable, seek, and append-mode
positioning paths; remove the direct libc call and
std::io::Error::last_os_error() conversion.
- Around line 7506-7517: Update the OSError stamping logic to include the
Windows-only winerror member in the w_member_set_cls list under cfg(windows).
Use the existing WINERROR_MEMBER definition and ensure its descriptor is stamped
with the OSError owning class, while leaving non-Windows behavior unchanged.
In `@pyre/pyre-interpreter/src/module/_winapi/mod.rs`:
- Around line 189-207: Update handle_list to validate w_attrs with
pyre_object::is_dict before calling the unsafe w_dict_getitem_str, matching the
guard used by environment_block. For non-dict lpAttributeList values, return the
existing None result instead of performing the unsafe access.
In `@pyre/pyre-interpreter/src/module/posix/interp_posix.rs`:
- Around line 2356-2373: Update the mode construction in the attributes-to-mode
logic to derive the execute bits from the FILE_ATTRIBUTE_DIRECTORY flag in attrs
rather than ft.is_dir(). Preserve the existing format-bit selection, including
symlink handling, so directory symlinks retain 0o111 permission bits.
- Around line 5928-5951: Update the truncate implementation around
crt_fd::ftruncate and crt_fd::close so their combined failure is converted with
the input path, matching os_truncate_impl’s path_error behavior. Preserve the
existing wopen error handling and ensure any ftruncate or close OSError includes
path.w_path() as its filename.
- Around line 4487-4515: The POSIX-only registrations must not compile on
Windows. In pyre/pyre-interpreter/src/module/posix/interp_posix.rs lines
4487-4515, gate the link registration with both the non-sandbox condition and an
appropriate Unix cfg; apply the same Unix gating to the get_inheritable
registration at lines 4720-4743, preserving its existing behavior on Unix.
- Around line 6141-6206: The startfile builtin currently interprets keyword
arguments as positional values. Update the startfile handler to use
split_builtin_kwargs, extracting operation, arguments, cwd, and show_cmd from
the keyword dictionary while retaining path as the required positional argument
and preserving existing validation/default behavior.
- Around line 4254-4282: Update the truncate implementation’s libc::truncate
invocation to run through crate::module::thread::call_external_function and
retry when the syscall returns EINTR, allowing pending signal handlers to run
between attempts; preserve the existing filename error handling and successful
None result. Use the nearby ftruncate implementation as the retry and wrapper
pattern.
- Around line 4113-4124: Update the descriptor duplication logic around the
existing dup2 call to use libc::dup3(fd, fd2, libc::O_CLOEXEC) when inheritable
is false, preserving the current dup2 path for inheritable descriptors. On
platforms without supported dup3/O_CLOEXEC, retain the existing
set_inheritable(false) fallback, while preserving errno handling and the
existing BorrowedFd-based cleanup path.
---
Outside diff comments:
In `@lib-python/3/subprocess.py`:
- Around line 1361-1388: Verify whether the interpreter promptly finalizes the
last-referenced Handle through __del__. If prompt finalization works, remove the
subprocess changes around _get_handles and _make_inheritable and restore the
vendored CPython implementation; otherwise document the limitation in the
project’s divergence notes rather than leaving this workaround undocumented.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 45354259-9cf5-43b4-bc3e-0185bf913e01
📒 Files selected for processing (33)
lib-python/3/subprocess.pypyre/extra_tests/parity_tests/compile_filename_boundary.pypyre/extra_tests/parity_tests/dict_subscript_fold.pypyre/extra_tests/parity_tests/dir_entry_uninstantiable_python314.pypyre/extra_tests/parity_tests/exception_instance_dict_attr.pypyre/extra_tests/parity_tests/import_unencodable_path_entry.pypyre/extra_tests/parity_tests/object_init_text_signature.pypyre/extra_tests/parity_tests/os_call_effects.pypyre/extra_tests/parity_tests/os_non_ascii_path.pypyre/extra_tests/parity_tests/os_stat_file_descriptor.pypyre/extra_tests/parity_tests/oserror_winerror.pypyre/extra_tests/parity_tests/oserror_winerror_syscall.pypyre/extra_tests/parity_tests/repr_surrogate_wtf8.pypyre/extra_tests/parity_tests/subprocess_launch.pypyre/extra_tests/parity_tests/symtable_filename_surrogateescape.pypyre/extra_tests/parity_tests/type_members_python314.pypyre/pyre-interpreter/Cargo.tomlpyre/pyre-interpreter/src/baseobjspace.rspyre/pyre-interpreter/src/builtins.rspyre/pyre-interpreter/src/display.rspyre/pyre-interpreter/src/error.rspyre/pyre-interpreter/src/gateway.rspyre/pyre-interpreter/src/module/_symtable/mod.rspyre/pyre-interpreter/src/module/_winapi/mod.rspyre/pyre-interpreter/src/module/posix/interp_posix.rspyre/pyre-interpreter/src/module/sys/vm.rspyre/pyre-interpreter/src/module/thread/mod.rspyre/pyre-interpreter/src/typedef.rspyre/pyre-jit-trace/src/descr.rspyre/pyre-jit-trace/src/jitcode_dispatch/specialize.rspyre/pyre-object/src/interp_exceptions.rspyre/pyre-object/src/typedef.rspyre/pyre-object/src/typeobject.rs
| // os.truncate(path, length) -> None. `os_truncate_impl` names the | ||
| // file rather than a descriptor of it where the platform has the call. | ||
| #[cfg(all(unix, not(feature = "sandbox")))] | ||
| crate::module_ns_store( | ||
| ns, | ||
| "truncate", | ||
| crate::make_builtin_function_with_arity( | ||
| "truncate", | ||
| |args| { | ||
| if args.len() < 2 { | ||
| return Err(crate::PyError::type_error("truncate() requires 2 arguments")); | ||
| } | ||
| let path = crate::gateway::fsencode_path_w(args[0])?; | ||
| let length = crate::baseobjspace::int_w(args[1])? as libc::off_t; | ||
| let c_path = std::ffi::CString::new(path.as_bytes.as_slice()) | ||
| .map_err(|_| crate::PyError::value_error("embedded null in path"))?; | ||
| let ret = unsafe { libc::truncate(c_path.as_ptr(), length) }; | ||
| if ret < 0 { | ||
| return Err(io_err_with_filename( | ||
| std::io::Error::last_os_error(), | ||
| path.w_path(), | ||
| )); | ||
| } | ||
| Ok(pyre_object::w_none()) | ||
| }, | ||
| 2, | ||
| ), | ||
| ); | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Add the EINTR retry and the external-call wrapper to truncate.
libc::truncate can fail with EINTR. Line 4239 already retries EINTR for ftruncate, and PEP 475 requires os.truncate to retry after running the pending signal handlers rather than raising InterruptedError.
The call also bypasses crate::module::thread::call_external_function, so the blocking syscall runs without the external-call guard that every other blocking call in this module takes.
🐛 Proposed fix
let c_path = std::ffi::CString::new(path.as_bytes.as_slice())
.map_err(|_| crate::PyError::value_error("embedded null in path"))?;
- let ret = unsafe { libc::truncate(c_path.as_ptr(), length) };
- if ret < 0 {
- return Err(io_err_with_filename(
- std::io::Error::last_os_error(),
- path.w_path(),
- ));
- }
+ loop {
+ let (ret, errno) =
+ crate::module::thread::call_external_function(|| unsafe {
+ libc::truncate(c_path.as_ptr(), length)
+ });
+ if ret == 0 {
+ break;
+ }
+ if errno != libc::EINTR {
+ return Err(errno_err_with_filename(errno, path.w_path()));
+ }
+ crate::builtins::eintr_retry_with(
+ std::io::Error::from_raw_os_error(errno),
+ |e| errno_err(e.raw_os_error().unwrap_or(0), ""),
+ )?;
+ }
Ok(pyre_object::w_none())📝 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.
| // os.truncate(path, length) -> None. `os_truncate_impl` names the | |
| // file rather than a descriptor of it where the platform has the call. | |
| #[cfg(all(unix, not(feature = "sandbox")))] | |
| crate::module_ns_store( | |
| ns, | |
| "truncate", | |
| crate::make_builtin_function_with_arity( | |
| "truncate", | |
| |args| { | |
| if args.len() < 2 { | |
| return Err(crate::PyError::type_error("truncate() requires 2 arguments")); | |
| } | |
| let path = crate::gateway::fsencode_path_w(args[0])?; | |
| let length = crate::baseobjspace::int_w(args[1])? as libc::off_t; | |
| let c_path = std::ffi::CString::new(path.as_bytes.as_slice()) | |
| .map_err(|_| crate::PyError::value_error("embedded null in path"))?; | |
| let ret = unsafe { libc::truncate(c_path.as_ptr(), length) }; | |
| if ret < 0 { | |
| return Err(io_err_with_filename( | |
| std::io::Error::last_os_error(), | |
| path.w_path(), | |
| )); | |
| } | |
| Ok(pyre_object::w_none()) | |
| }, | |
| 2, | |
| ), | |
| ); | |
| // os.truncate(path, length) -> None. `os_truncate_impl` names the | |
| // file rather than a descriptor of it where the platform has the call. | |
| #[cfg(all(unix, not(feature = "sandbox")))] | |
| crate::module_ns_store( | |
| ns, | |
| "truncate", | |
| crate::make_builtin_function_with_arity( | |
| "truncate", | |
| |args| { | |
| if args.len() < 2 { | |
| return Err(crate::PyError::type_error("truncate() requires 2 arguments")); | |
| } | |
| let path = crate::gateway::fsencode_path_w(args[0])?; | |
| let length = crate::baseobjspace::int_w(args[1])? as libc::off_t; | |
| let c_path = std::ffi::CString::new(path.as_bytes.as_slice()) | |
| .map_err(|_| crate::PyError::value_error("embedded null in path"))?; | |
| loop { | |
| let (ret, errno) = | |
| crate::module::thread::call_external_function(|| unsafe { | |
| libc::truncate(c_path.as_ptr(), length) | |
| }); | |
| if ret == 0 { | |
| break; | |
| } | |
| if errno != libc::EINTR { | |
| return Err(errno_err_with_filename(errno, path.w_path())); | |
| } | |
| crate::builtins::eintr_retry_with( | |
| std::io::Error::from_raw_os_error(errno), | |
| |e| errno_err(e.raw_os_error().unwrap_or(0), ""), | |
| )?; | |
| } | |
| Ok(pyre_object::w_none()) | |
| }, | |
| 2, | |
| ), | |
| ); |
🤖 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 `@pyre/pyre-interpreter/src/module/posix/interp_posix.rs` around lines 4254 -
4282, Update the truncate implementation’s libc::truncate invocation to run
through crate::module::thread::call_external_function and retry when the syscall
returns EINTR, allowing pending signal handlers to run between attempts;
preserve the existing filename error handling and successful None result. Use
the nearby ftruncate implementation as the retry and wrapper pattern.
7451ac1 to
a8535d1
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a8535d1d3a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let inheritable = match args.get(2) { | ||
| Some(&w) => crate::baseobjspace::is_true(w)?, | ||
| None => true, |
There was a problem hiding this comment.
When callers use the documented os.dup2(fd, fd2, inheritable=False) keyword form, make_builtin_function leaves the keyword-marker dictionary in args[2]; this code treats that nonempty dictionary as true and creates an inheritable descriptor. The same parsing appears in the Windows override, so code explicitly preventing descriptor inheritance can instead leak the descriptor into child processes; bind or split the keyword before reading this argument.
AGENTS.md reference: AGENTS.md:L231-L232
Useful? React with 👍 / 👎.
| crate::make_builtin_function("access", |args| { | ||
| if args.len() < 2 { | ||
| return Err(crate::PyError::type_error("access() requires 2 arguments")); | ||
| } | ||
| let path = crate::gateway::fsencode_path_w(args[0])?; |
There was a problem hiding this comment.
On Windows, calls supplying dir_fd, effective_ids, or follow_symlinks leave a trailing keyword-marker dictionary that this closure never inspects, so it silently performs the default cwd-relative, real-ID, symlink-following check. In particular, os.access(relative_path, mode, dir_fd=fd) can report permissions for a different cwd file rather than raising NotImplementedError for the unsupported descriptor, as the upstream DirFD/HAVE_FACCESSAT path requires.
AGENTS.md reference: AGENTS.md:L231-L232
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
pyre/pyre-interpreter/src/module/_winapi/mod.rs (1)
223-224: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winReject an invalid
lpAttributeList.Line 223 treats every non-dict value as no attribute list. If
inherit_handlesis true,CreateProcesscan then inherit all inheritable handles instead of failing before launch.Return
TypeErrorfor an unsupported non-Nonevalue. PreserveOk(None)only forNoneor null. CPython rejects a non-mapping attribute list before process creation. (raw.githubusercontent.com)This affects the Windows process-launch parity objective.
🤖 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 `@pyre/pyre-interpreter/src/module/_winapi/mod.rs` around lines 223 - 224, Update the lpAttributeList validation in the surrounding process-launch code to distinguish None/null from unsupported non-dict values: return Ok(None) only for None or null, and return TypeError for any other non-mapping value before CreateProcess runs. Preserve the existing dictionary handling and ensure invalid values cannot proceed when inherit_handles is enabled.
🤖 Prompt for all review comments with 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.
Inline comments:
In `@pyre/extra_tests/parity_tests/os_call_effects.py`:
- Around line 120-133: Update the l4 follow_symlinks coverage to use a
symbolic-link source instead of the regular file a_file. Create the source
symlink, call os.link with follow_symlinks=True, and assert that l4 has the
expected link or regular-file type so implementations cannot ignore the option;
preserve the existing platform-specific handling around os.link and cleanup.
In `@pyre/extra_tests/parity_tests/subprocess_launch.py`:
- Around line 96-105: Update the environment setup and cleanup around the
subprocess invocation in the parity test to capture any pre-existing PYRE_PROBE
value before assigning "inherited", then restore that value in the finally
block; only remove the variable when it was originally unset.
In `@pyre/pyre-interpreter/src/module/posix/interp_posix.rs`:
- Around line 4322-4341: Update the EINTR retry path in the truncate
implementation to run pending signal handlers before each retry, using the
established retry helper pattern from read/readinto, and execute the blocking
syscall through crate::module::thread::call_external_function. Preserve retry
behavior for EINTR and existing errno propagation for other failures.
---
Duplicate comments:
In `@pyre/pyre-interpreter/src/module/_winapi/mod.rs`:
- Around line 223-224: Update the lpAttributeList validation in the surrounding
process-launch code to distinguish None/null from unsupported non-dict values:
return Ok(None) only for None or null, and return TypeError for any other
non-mapping value before CreateProcess runs. Preserve the existing dictionary
handling and ensure invalid values cannot proceed when inherit_handles is
enabled.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: d5c97f7a-1761-41ac-8572-990dbc010ebb
📒 Files selected for processing (7)
pyre/extra_tests/parity_tests/os_call_effects.pypyre/extra_tests/parity_tests/oserror_winerror.pypyre/extra_tests/parity_tests/oserror_winerror_syscall.pypyre/extra_tests/parity_tests/subprocess_launch.pypyre/pyre-interpreter/src/builtins.rspyre/pyre-interpreter/src/module/_winapi/mod.rspyre/pyre-interpreter/src/module/posix/interp_posix.rs
| # Whether the source symlink is followed or linked is the platform's to say; | ||
| # what it cannot do is take the argument and ignore it. | ||
| l4 = os.path.join(base, "l4") | ||
| if os.link in os.supports_follow_symlinks: | ||
| os.link(a_file, l4, follow_symlinks=True) | ||
| os.remove(l4) | ||
| else: | ||
| try: | ||
| os.link(a_file, l4, follow_symlinks=True) | ||
| except (NotImplementedError, OSError): | ||
| pass | ||
| else: | ||
| raise AssertionError("link ignored follow_symlinks") | ||
| assert not os.path.exists(l4) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use a symbolic-link source for follow_symlinks.
Line 124 passes a_file, which is a regular file. An implementation that ignores follow_symlinks passes this test.
Create a source symbolic link and assert whether l4 is a link or a regular file after follow_symlinks=True. This makes the option observable. This is part of the link parity coverage.
🧰 Tools
🪛 Ruff (0.16.1)
[warning] 132-132: Avoid specifying long messages outside the exception class
(TRY003)
🤖 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 `@pyre/extra_tests/parity_tests/os_call_effects.py` around lines 120 - 133,
Update the l4 follow_symlinks coverage to use a symbolic-link source instead of
the regular file a_file. Create the source symlink, call os.link with
follow_symlinks=True, and assert that l4 has the expected link or regular-file
type so implementations cannot ignore the option; preserve the existing
platform-specific handling around os.link and cleanup.
| os.environ["PYRE_PROBE"] = "inherited" | ||
| try: | ||
| done = subprocess.run( | ||
| child("import os; print(os.environ['PYRE_PROBE'])"), | ||
| capture_output=True, | ||
| text=True, | ||
| env=os.environ, | ||
| ) | ||
| finally: | ||
| del os.environ["PYRE_PROBE"] |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Restore an existing PYRE_PROBE value.
This test deletes PYRE_PROBE even when the runner defined it before the test. Preserve and restore the old value.
Proposed fix
+old_probe = os.environ.get("PYRE_PROBE")
os.environ["PYRE_PROBE"] = "inherited"
try:
done = subprocess.run(
@@
finally:
- del os.environ["PYRE_PROBE"]
+ if old_probe is None:
+ del os.environ["PYRE_PROBE"]
+ else:
+ os.environ["PYRE_PROBE"] = old_probe📝 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.
| os.environ["PYRE_PROBE"] = "inherited" | |
| try: | |
| done = subprocess.run( | |
| child("import os; print(os.environ['PYRE_PROBE'])"), | |
| capture_output=True, | |
| text=True, | |
| env=os.environ, | |
| ) | |
| finally: | |
| del os.environ["PYRE_PROBE"] | |
| old_probe = os.environ.get("PYRE_PROBE") | |
| os.environ["PYRE_PROBE"] = "inherited" | |
| try: | |
| done = subprocess.run( | |
| child("import os; print(os.environ['PYRE_PROBE'])"), | |
| capture_output=True, | |
| text=True, | |
| env=os.environ, | |
| ) | |
| finally: | |
| if old_probe is None: | |
| del os.environ["PYRE_PROBE"] | |
| else: | |
| os.environ["PYRE_PROBE"] = old_probe |
🧰 Tools
🪛 ast-grep (0.45.0)
[error] 97-102: Use of unsanitized data to create processes
Context: subprocess.run(
child("import os; print(os.environ['PYRE_PROBE'])"),
capture_output=True,
text=True,
env=os.environ,
)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(os-system-unsanitized-data)
[error] 97-102: Command coming from incoming request
Context: subprocess.run(
child("import os; print(os.environ['PYRE_PROBE'])"),
capture_output=True,
text=True,
env=os.environ,
)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
🪛 Ruff (0.16.1)
[error] 98-98: subprocess call: check for execution of untrusted input
(S603)
[warning] 98-98: subprocess.run without explicit check argument
Add explicit check=False
(PLW1510)
🤖 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 `@pyre/extra_tests/parity_tests/subprocess_launch.py` around lines 96 - 105,
Update the environment setup and cleanup around the subprocess invocation in the
parity test to capture any pre-existing PYRE_PROBE value before assigning
"inherited", then restore that value in the finally block; only remove the
variable when it was originally unset.
`sys.getfilesystemencodeerrors` reports `surrogatepass` on Windows and `surrogateescape` elsewhere. `gateway::fsencode` / `fsdecode`, the `compile()` and `_symtable.symtable` filename converters, the `os` path converter and `fspath_buf` all take that handler: a path byte with no UTF-8 spelling is reported at the call that supplied it, and a `str` reaches the host as its own UTF-16 code units instead of a lossy re-spelling. `W_BaseException` gains a `w_winerror` slot, registered as an `OSError` member descriptor only where the platform has Windows error codes. A 2..=5 argument call derives `errno` from an integer fourth argument through the `rwin32` error map, picks the errno subclass from the derived value, rebinds `args[0]` to it, and `str()` reports `[WinError N]` where there is a filename or a strerror to render it with. The Windows `OSError` ABI basicsize is 120. The exception-new walker declines a four-or-more argument `OSError` there, since it neither guards nor writes that argument, and `w_exception_slot_descr` locates its field by offset rather than by a hand-counted index into the descr group. A `SyntaxError` filename basenames on the host's path separators. `compile_filename_boundary`, `repr_surrogate_wtf8`, `import_unencodable_path_entry`, `symtable_filename_surrogateescape` and `type_members_python314` branch on the platform's encoding and `OSError` layout; `oserror_winerror.py` is new. The wider exception layout moves one wasm jit-stats counter, re-recorded here.
`os.DirEntry` and the scandir iterator report the module `os` is implemented by, which Windows spells `nt`; the script asserted `posix` everywhere and therefore failed under CPython there. pyre registers that module as `posix` on every platform, so the script now fails for the pyre runners on Windows instead. Registering it as `nt` is separate work.
A filesystem call that fails on Windows reports a GetLastError code. `PyError::os_error_win32_syscall2` keeps it in the `winerror` slot, takes `strerror` from `FormatMessageW` with the trailing newline and full stop removed, and derives `errno` — and with it the OSError subclass — from the Win32-to-errno map, so `str(e)` opens with `[WinError 3]`. interp_posix's path calls (stat, lstat, listdir, scandir, mkdir, rmdir, remove, unlink, rename, replace, utime, readlink, DirEntry.stat/inode) and the win_nt and _winapi helpers build their OSError through it. The descriptor calls (open, read, write, close, lseek) go through the C runtime, which reports an errno, and keep the errno spelling.
The C runtime's invalid parameter handler aborts the process when a call is handed a descriptor it cannot resolve, so `os.close`/`read`/`write`/ `lseek` and their kin silence it for the duration of the call (`_Py_BEGIN_SUPPRESS_IPH`) and read the verdict from `errno`, which is where the runtime leaves it — `GetLastError` describes whichever Win32 call the runtime made last. `os.open`/`mkdir`/`rmdir`/`unlink` took the narrow entry points, which re-encode the path through the ANSI code page and answer `ERROR_NO_UNICODE_TRANSLATION` for a name that has no spelling there; they take `_wopen` / `CreateDirectoryW` / `RemoveDirectoryW` / `DeleteFileW` now. `os.fstat`, `os.dup`, `os.dup2`, `os.fsync` and `os.ftruncate` were the noop placeholders on Windows. `fstat` reads the descriptor's handle and reports `ERROR_INVALID_HANDLE` for a descriptor without one, the way `_Py_fstat_noraise` does. `stat_result.st_mode` follows `attributes_to_mode`: a directory carries the execute bits, and only the read-only attribute drops the write ones.
Windows answered truncate, chdir, access, chmod, fchmod, link, umask, pipe, getppid, getlogin, cpu_count, device_encoding, get_inheritable and set_inheritable with None and no effect, and had no startfile at all. POSIX had no truncate, link or get_inheritable either; dup handed back an inheritable copy, dup2 ignored its inheritable argument, and utime named the file in the error `os_utime_impl` raises without one.
…size that can fail Windows answered `os.system` and `os.waitpid` with None, had no `times`, `listdrives`, `listvolumes` or `listmounts`, and named the `times_result` structseq after a module the host does not have, which is the name pickle imports to resolve it. `os.get_terminal_size` ignored its fd argument and answered 80x24 for a descriptor that names no terminal, where the call reports the failure and `shutil.get_terminal_size` is what falls back.
GetStdHandle, GetCurrentProcess, GetFileType, GetLastError, TerminateProcess, CreatePipe, DuplicateHandle and CreateProcess, backed by rustpython_host_env::winapi, plus the DUPLICATE_*, FILE_TYPE_*, PROCESS_* and INVALID_HANDLE_VALUE constants the module body reads. An empty lpAttributeList["handle_list"] is passed as no attribute list at all: subprocess.STARTUPINFO starts with one, and a list carrying no handle makes CreateProcess answer ERROR_BAD_LENGTH. subprocess._get_handles closed the pipe end it duplicated by dropping the last reference to its Handle wrapper, which waits on the collector here, so the parent kept the write end of the pipe it was reading to end-of-file; _make_inheritable now takes a close flag and closes it. Parity script subprocess_launch.py covers the launch surface on both platforms.
`_DirFD_Unavailable` (interp_posix.py:285-292) raises "dir_fd unavailable on this platform", without the call's name; the message carried a "stat: " prefix that neither it nor `dir_fd_unavailable` produces. os_stat_file_descriptor.py asserted that a descriptor and a dir_fd together report the descriptor conflict. Where the platform has no fstatat the argument is turned away while it is unwrapped, which is a step earlier, so the script failed under CPython on Windows as well; it now asserts the answer each platform gives.
TimeoutExpired carries what was left of the timeout where the call waited on something before it, so subprocess_launch.py read 0.4999965 for the 0.5 it passed.
`libc::fcntl` is not a name the sandbox seam exports — under `--features sandbox` `libc` is `crate::host_seam::sys`, which re-exports types and constants but no syscall — so the call did not compile there. `rustpython_host_env::fcntl::get_inheritable` is the same read.
`seekable`, `seek` and the append-mode positioning already go through `crt_call!` and report `crt_errno()`; `tell` called `libc::lseek` directly, so on Windows the invalid parameter handler stayed armed for the duration of the call and the error came from `GetLastError` rather than the runtime's own errno.
`CreateProcess` took a dict and refused everything else, so `Popen(env=os.environ)` — a `MutableMapping` — raised. `getenvironment` reads `keys()` for the names and subscripts the mapping for each value; what no subscript can be taken from is `TypeError: environment must be dictionary or None`. The mapping and its keys are published on the shadow stack because `__getitem__` allocates. `lpAttributeList` is checked for a dict before it is subscripted through the unsafe dict accessor. `subprocess_launch.py` covers `env=os.environ`, a mapping with no dict behind it, and a non-mapping.
`os.truncate`'s path is `path_t(allow_fd=…)`, so an integer names an open descriptor; both arms take one now, retry `EINTR` through the call gate, and name the file where there is one. `HAVE_FTRUNCATE` is advertised on Windows too, which is what puts `truncate` in `os.supports_fd`. `os.link` took `src_dir_fd`, `dst_dir_fd` and `follow_symlinks` and ignored all three. POSIX resolves the descriptors through `linkat` — and advertises `HAVE_LINKAT`, which puts `link` in `supports_dir_fd` and `supports_follow_symlinks` — reaching for it only when a descriptor is given or the source symlink is not to be followed. Windows refuses the descriptors, refuses an explicit `follow_symlinks=True` that `CreateHardLinkW` cannot honour, and refuses a third positional argument rather than dropping it. `os.symlink` was a noop placeholder on Windows: it returned `None` and created nothing. It is `CreateSymbolicLinkW`, asking for the unprivileged-create path and retrying without the flag where the system rejects it, with `SYMBOLIC_LINK_FLAG_DIRECTORY` for `target_is_directory` and a refusal for `dir_fd`. `os.startfile` read its four optional arguments positionally only, so `os.startfile(p, cwd=d)` opened the file in the wrong directory; they are positional-or-keyword, and an unknown keyword is named. `dup2` asks `dup3` for the non-inheritable duplicate where it exists, so no window opens in which an exec could carry the new descriptor. `attributes_to_mode` read the execute bits off the resolved file type, so a symlink to a directory reported `0o120666`; they come from `FILE_ATTRIBUTE_DIRECTORY`, which the link's own attributes carry.
`__objclass__` is what `repr` and `inspect` read the owning class from; `winerror`'s and `errno`'s both answer `OSError` already, and now say so.
`os_utime_impl` reports the failure with `path_error(path)`, so `errno`, `strerror` and the name all arrive: 3.14 raises `FileNotFoundError(2, ..., '/tmp/x/missing')` where 3.12 raised the errno alone. The POSIX arm dropped the name, which was measured against a 3.12 on hand rather than the 3.14 this tree targets. `os_call_effects.py` compares the working directory against the names built on `getcwd`'s own answer rather than against `realpath`, which expands the 8.3 spelling a `TEMP` like `C:\Users\RUNNER~1\...` is named by and `getcwd` does not.
The parity runner compared against whatever `python3` names, which on a host whose system CPython is 3.12 is 58 failures that are not parity failures — and among them the one that was, `os.utime`'s filename, which 3.14 attaches and 3.12 does not. A reference of the wrong version measures nothing, so both runners now resolve `python3.14` before the bare names, verify the version they got, and stop with the versions they found rather than reporting against the wrong one. What they resolve to is the path the interpreter reports for itself, so a spawn does not depend on the PATH it inherits: `check.py`'s timed runs hand the child a curated environment, which is why a bare `python3.14` died there with `WinError 2` on a host where that name is an extensionless shim.
a8535d1 to
0135625
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 013562554b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
| let command = crate::gateway::fsencode_path_w(args[0])?; | ||
| let wide = wide_path(&command.as_bytes)?; | ||
| let status = crate::builtins::crt_call!(_wsystem(wide.as_ptr())); |
There was a problem hiding this comment.
On Windows, a long-running os.system() blocks here while the mutator remains in the collector's RUNNING census, so a concurrent thread that requests stop-the-world GC can wait forever for this blocked thread. The same regression exists in the new host_nt::cwait call and _winapi.WaitForSingleObject; route these waits through before_external_block/call_external_function, preserving errno or LastError before the guard drops.
AGENTS.md reference: AGENTS.md:L231-L233
Useful? React with 👍 / 👎.
| // process to wait on. | ||
| fn WaitForSingleObject(handle: i64, milliseconds: i64) -> i64 { | ||
| unsafe { | ||
| windows_sys::Win32::System::Threading::WaitForSingleObject( |
There was a problem hiding this comment.
When the handle is invalid or closed, Win32 returns WAIT_FAILED (0xFFFFFFFF), but this wrapper exposes that value as a successful result and leaves callers such as subprocess.Popen._wait to continue into GetExitCodeProcess. The ported upstream wrapper explicitly checks this sentinel and raises the saved Windows error (rpython/rlib/rwin32.py:436-443), so this function should do the same.
AGENTS.md reference: AGENTS.md:L231-L233
Useful? React with 👍 / 👎.
| let target_is_directory = match args | ||
| .get(2) | ||
| .copied() | ||
| .or_else(|| crate::builtins::kwarg_get(kwargs, "target_is_directory")) | ||
| { |
There was a problem hiding this comment.
For a call such as os.symlink(src, dst, False, target_is_directory=True), this positional-first lookup silently ignores the keyword and performs the filesystem operation using False; Python signature binding must instead raise TypeError before creating anything because the same argument was supplied twice. Use the existing duplicate-aware positional-or-keyword binder rather than or_else.
AGENTS.md reference: AGENTS.md:L231-L233
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (1)
pyre/pyre-interpreter/src/builtins.rs (1)
7553-7563: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winStamp the
winerrormember with its owning class.
WINERROR_MEMBERinstalls thewinerrordescriptor into theOSErrornamespace on Windows. The post-creation stamping loop at Line 7930 still lists only["errno", "strerror", "filename", "filename2"], sow_member_set_clsis never called forwinerror. Every other member family stamps each installed member. Without the owning class, the descriptor's class check has no class to compare against.Add the member to the stamping loop under
cfg(windows).🐛 Proposed fix for the stamping loop (lines 7929-7935)
// pyre/pyre-interpreter/src/builtins.rs if name == "OSError" { #[cfg(windows)] const OS_ERROR_MEMBER_NAMES: &[&str] = &["errno", "strerror", "filename", "filename2", "winerror"]; #[cfg(not(windows))] const OS_ERROR_MEMBER_NAMES: &[&str] = &["errno", "strerror", "filename", "filename2"]; for member_name in OS_ERROR_MEMBER_NAMES { if let Some(member) = crate::type_dict_lookup(cls, member_name) { unsafe { pyre_object::w_member_set_cls(member, cls) }; } } }🤖 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 `@pyre/pyre-interpreter/src/builtins.rs` around lines 7553 - 7563, Update the OSError post-creation stamping logic for the name check around the member-set loop to include "winerror" in the member names only under cfg(windows). Keep the existing non-Windows list unchanged, and ensure the loop passes the Windows-installed descriptor through w_member_set_cls like the other OSError members.
🤖 Prompt for all review comments with 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.
Inline comments:
In `@pyre/check.py`:
- Around line 41-79: Require CPython specifically when resolving the oracle
interpreter: in pyre/check.py lines 41-79, extend _probe_interpreter to report
sys.implementation.name and make _resolve_python3 accept candidates only when
the implementation is "cpython" and the version is CPYTHON_TARGET; apply the
same implementation check in pyre/extra_tests/parity_tests/run.py lines 126-179
before _cpython() returns an executable path.
In `@pyre/extra_tests/parity_tests/dir_entry_uninstantiable_python314.py`:
- Around line 21-64: Update the scandir-related builtin type registrations
passed to make_builtin_type so DirEntry and ScandirIterator use "nt.*" on
Windows and "posix.*" otherwise, matching the cfg!(windows) branching used for
times_result_seq_type. Preserve the existing type behavior while making their
module names platform-aware for the assertions in this test.
In `@pyre/extra_tests/parity_tests/os_non_ascii_path.py`:
- Line 36: Wrap the os.scandir(base) call in the assertion in a with block so
the scandir iterator is explicitly closed after sorting. Preserve the existing
name ordering and assertion behavior while updating the surrounding test flow to
consume the context-managed iterator.
In `@pyre/pyre-interpreter/src/builtins.rs`:
- Around line 14273-14274: Align the conditional compilation gates for
fd_errno_err with every call site in fileio_method_truncate and write_to_file.
Ensure the truncate and nearby write error-handling paths only compile when the
helper is available, or broaden fd_errno_err’s definition gate to exactly match
those callers, including the host_env requirement under unix and non-sandbox
configurations.
In `@pyre/pyre-interpreter/src/error.rs`:
- Around line 966-980: Update win32_strerror to call
rustpython_host_env::overlapped::format_message(winerror as u32) instead of
rustpython_host_env::windows::format_error_message, preserving the existing
trimming and fallback behavior while using the API that returns String directly.
In `@pyre/pyre-interpreter/src/module/posix/interp_posix.rs`:
- Around line 4550-4563: Gate the `link` registration and its `link_positional`
implementation at both interp_posix.rs sites (2744-2753 and 4550-4563) with the
same `unix` and `host_env` feature conditions required by
`rustpython_host_env::posix::linkat`; keep the registration and helper gates
synchronized so unsupported Windows or non-host-Unix builds do not reference the
unavailable helper.
---
Duplicate comments:
In `@pyre/pyre-interpreter/src/builtins.rs`:
- Around line 7553-7563: Update the OSError post-creation stamping logic for the
name check around the member-set loop to include "winerror" in the member names
only under cfg(windows). Keep the existing non-Windows list unchanged, and
ensure the loop passes the Windows-installed descriptor through w_member_set_cls
like the other OSError members.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 62e33a52-545f-4931-9883-2ab33191c740
📒 Files selected for processing (32)
lib-python/3/subprocess.pypyre/check.pypyre/extra_tests/parity_tests/compile_filename_boundary.pypyre/extra_tests/parity_tests/dir_entry_uninstantiable_python314.pypyre/extra_tests/parity_tests/import_unencodable_path_entry.pypyre/extra_tests/parity_tests/os_call_effects.pypyre/extra_tests/parity_tests/os_non_ascii_path.pypyre/extra_tests/parity_tests/os_stat_file_descriptor.pypyre/extra_tests/parity_tests/oserror_winerror.pypyre/extra_tests/parity_tests/oserror_winerror_syscall.pypyre/extra_tests/parity_tests/repr_surrogate_wtf8.pypyre/extra_tests/parity_tests/run.pypyre/extra_tests/parity_tests/subprocess_launch.pypyre/extra_tests/parity_tests/symtable_filename_surrogateescape.pypyre/extra_tests/parity_tests/type_members_python314.pypyre/pyre-interpreter/Cargo.tomlpyre/pyre-interpreter/src/baseobjspace.rspyre/pyre-interpreter/src/builtins.rspyre/pyre-interpreter/src/display.rspyre/pyre-interpreter/src/error.rspyre/pyre-interpreter/src/gateway.rspyre/pyre-interpreter/src/module/_symtable/mod.rspyre/pyre-interpreter/src/module/_winapi/mod.rspyre/pyre-interpreter/src/module/posix/interp_posix.rspyre/pyre-interpreter/src/module/sys/vm.rspyre/pyre-interpreter/src/module/thread/mod.rspyre/pyre-interpreter/src/typedef.rspyre/pyre-jit-trace/src/descr.rspyre/pyre-jit-trace/src/jitcode_dispatch/specialize.rspyre/pyre-object/src/interp_exceptions.rspyre/pyre-object/src/typedef.rspyre/pyre-object/src/typeobject.rs
| probe = "import sys; print(sys.version_info[0], sys.version_info[1]); print(sys.executable)" | ||
| try: | ||
| proc = subprocess.run( | ||
| [command, "-c", probe], capture_output=True, text=True, timeout=30, | ||
| ) | ||
| except (OSError, subprocess.SubprocessError): | ||
| return None | ||
| if proc.returncode != 0: | ||
| return None | ||
| lines = (proc.stdout or "").splitlines() | ||
| if len(lines) < 2: | ||
| return None | ||
| try: | ||
| major, minor = lines[0].split() | ||
| except ValueError: | ||
| return None | ||
| return (int(major), int(minor)), lines[1].strip() or command | ||
|
|
||
|
|
||
| def _resolve_python3(): | ||
| """The oracle interpreter, as the absolute path it reports for itself. | ||
|
|
||
| A bare name would be resolved against the PATH of whichever environment | ||
| spawns it, and the timed runs hand the child a curated one. | ||
| """ | ||
| named = os.environ.get("PYRE_CHECK_PYTHON3") | ||
| candidates = [named] if named else ["python3.14", "python3", "python"] | ||
| rejected = [] | ||
| for candidate in candidates: | ||
| if named is None and shutil.which(candidate) is None: | ||
| continue | ||
| probed = _probe_interpreter(candidate) | ||
| if probed is None: | ||
| rejected.append(f" {candidate}: did not run") | ||
| continue | ||
| version, executable = probed | ||
| if version == CPYTHON_TARGET: | ||
| return executable | ||
| rejected.append(" %s: %d.%d" % (candidate, *version)) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Require the CPython implementation before accepting the oracle.
Both resolvers accept any interpreter whose sys.version_info is (3, 14). A PyPy, GraalPy, or other implementation with that version can pass and become the parity baseline. This invalidates the comparison that these scripts report as CPython parity.
pyre/check.py#L41-L79: Includesys.implementation.namein the probe output. Reject candidates unless it equals"cpython"and the version equals(3, 14).pyre/extra_tests/parity_tests/run.py#L126-L179: Apply the same implementation check before_cpython()returns the executable path.
As per PR objectives, this layer must require a matching CPython 3.14 interpreter.
🧰 Tools
🪛 ast-grep (0.45.0)
[error] 42-44: Command coming from incoming request
Context: subprocess.run(
[command, "-c", probe], capture_output=True, text=True, timeout=30,
)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
🪛 Ruff (0.16.1)
[error] 43-43: subprocess call: check for execution of untrusted input
(S603)
[warning] 43-43: subprocess.run without explicit check argument
Add explicit check=False
(PLW1510)
[warning] 60-60: Missing return type annotation for private function _resolve_python3
(ANN202)
[warning] 79-79: Use format specifiers instead of percent format
(UP031)
📍 Affects 2 files
pyre/check.py#L41-L79(this comment)pyre/extra_tests/parity_tests/run.py#L126-L179
🤖 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 `@pyre/check.py` around lines 41 - 79, Require CPython specifically when
resolving the oracle interpreter: in pyre/check.py lines 41-79, extend
_probe_interpreter to report sys.implementation.name and make _resolve_python3
accept candidates only when the implementation is "cpython" and the version is
CPYTHON_TARGET; apply the same implementation check in
pyre/extra_tests/parity_tests/run.py lines 126-179 before _cpython() returns an
executable path.
| import sys | ||
|
|
||
| HERE = os.path.dirname(os.path.abspath(__file__)) | ||
| MODULE = "nt" if sys.platform == "win32" else "posix" | ||
|
|
||
| assert os.DirEntry.__module__ == "posix", os.DirEntry.__module__ | ||
| assert os.DirEntry.__module__ == MODULE, os.DirEntry.__module__ | ||
| assert os.DirEntry.__name__ == "DirEntry", os.DirEntry.__name__ | ||
| assert os.DirEntry.__qualname__ == "DirEntry", os.DirEntry.__qualname__ | ||
| assert repr(os.DirEntry) == "<class 'posix.DirEntry'>", repr(os.DirEntry) | ||
| assert repr(os.DirEntry) == "<class '%s.DirEntry'>" % MODULE, repr(os.DirEntry) | ||
|
|
||
| try: | ||
| os.DirEntry() | ||
| except TypeError as exc: | ||
| assert str(exc) == "cannot create 'posix.DirEntry' instances", str(exc) | ||
| assert str(exc) == "cannot create '%s.DirEntry' instances" % MODULE, str(exc) | ||
| else: | ||
| raise AssertionError("os.DirEntry() must not construct an entry") | ||
|
|
||
| try: | ||
| class _Sub(os.DirEntry): | ||
| pass | ||
| except TypeError as exc: | ||
| assert str(exc) == "type 'posix.DirEntry' is not an acceptable base type", str(exc) | ||
| expected = "type '%s.DirEntry' is not an acceptable base type" % MODULE | ||
| assert str(exc) == expected, str(exc) | ||
| else: | ||
| raise AssertionError("os.DirEntry must not be an acceptable base type") | ||
|
|
||
| it = os.scandir(HERE) | ||
| scandir_iterator = type(it) | ||
| assert scandir_iterator.__module__ == "posix", scandir_iterator.__module__ | ||
| assert scandir_iterator.__module__ == MODULE, scandir_iterator.__module__ | ||
| assert scandir_iterator.__name__ == "ScandirIterator", scandir_iterator.__name__ | ||
|
|
||
| try: | ||
| scandir_iterator() | ||
| except TypeError as exc: | ||
| assert str(exc) == "cannot create 'posix.ScandirIterator' instances", str(exc) | ||
| assert str(exc) == "cannot create '%s.ScandirIterator' instances" % MODULE, str(exc) | ||
| else: | ||
| raise AssertionError("ScandirIterator() must not construct an iterator") | ||
|
|
||
| try: | ||
| class _SubIter(scandir_iterator): | ||
| pass | ||
| except TypeError as exc: | ||
| assert str(exc) == "type 'posix.ScandirIterator' is not an acceptable base type", str(exc) | ||
| expected = "type '%s.ScandirIterator' is not an acceptable base type" % MODULE | ||
| assert str(exc) == expected, str(exc) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
f=pyre/pyre-interpreter/src/module/posix/interp_posix.rs
rg -n 'make_builtin_type\("(posix|nt)\.' "$f"
# Check whether any layer rewrites a builtin type's module name for Windows.
rg -n -C4 'cfg!\(windows\)|target_os = "windows"' pyre/pyre-interpreter/src/typedef.rs | head -50Repository: youknowone/pyre
Length of output: 761
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant typedef constructors and nearby Windows/platform-aware examples.
f=pyre/pyre-interpreter/src/module/posix/interp_posix.rs
sed -n '2920,3085p' "$f" | cat -n
# Check the test behavior expectations and module detection.
g=pyre/extra_tests/parity_tests/dir_entry_uninstantiable_python314.py
sed -n '1,90p' "$g" | cat -n
# Find all references to os.DirEntry / ScandirIterator in extra_tests and module definitions.
rg -n "DirEntry|ScandirIterator|make_builtin_type\\(\"posix\\.|make_builtin_type\\(\"nt\\." pyre/extra_tests pyre/pyre-interpreter/src | head -200Repository: youknowone/pyre
Length of output: 19799
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find any layer that chooses nt vs posix module prefixes for os types.
rg -n -C6 'os_module|os_scandir|scandir|nt|make_builtin_type\("nt|make_builtin_type\("posix|target_os = "windows|cfg!\(windows\)' pyre/pyre-interpreter/src/module | head -300
# Find os module files and their scandir-related type definitions.
fd -e rs -x sh -c 'echo "--- $1"; rg -n -C5 "scandir|DirEntry|ScandirIterator|os_module|module_ns_store|make_builtin_type" "$1"' sh pyre/pyre-interpreter/src/moduleRepository: youknowone/pyre
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read focused os module files and the typedef name handling implementation.
fd -e rs '^(os|nt|typedef)\.rs$' pyre/pyre-interpreter/src/module pyre
for f in $(fd -e rs '^(os|nt)\.rs$' pyre/pyre-interpreter/src/module); do
echo "--- $f"
wc -l "$f"
rg -n -C5 'DirEntry|ScandirIterator|scandir|os_module|nt|module_ns_store|register_module|target_os = "windows|cfg!\(windows\)' "$f" || true
done
echo "--- typedef.rs relevant lines"
rg -n -C6 'fn make_builtin_type|__module__|w_type_set_name|type_get_name|PySys_SetConfig|module_path' pyre/pyre-interpreter/src/typedef.rsRepository: youknowone/pyre
Length of output: 50373
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# List exact os/nt-related files.
git ls-files pyre/pyre-interpreter/src/module | grep -E '(^|/)(os|nt)\.rs$|/module/(os|nt)/' || true
# Search focused file list for relevant registry code.
while IFS= read -r f; do
echo "--- $f"
wc -l "$f"
rg -n -C6 'DirEntry|ScandirIterator|scandir|os_module|nt|register_module|module_ns_store|make_builtin_type|target_os = "windows|cfg!\(windows"' "$f" || true
done < <(git ls-files pyre/pyre-interpreter/src/module | grep -E '(^|/)(os|nt)\.rs$|/module/(os|nt)/')
# Read typedef name handling around the module derivation.
f=pyre/pyre-interpreter/src/typedef.rs
sed -n '2268,2293p' "$f" | cat -n
sed -n '10888,10931p' "$f" | cat -nRepository: youknowone/pyre
Length of output: 206
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "--- module files"
git ls-files pyre/pyre-interpreter/src/module | grep -E '(^|/)(os|nt)\.rs$|module/(os|nt)/' || true
echo "--- focused search for scandir/os types"
for f in pyre/pyre-interpreter/src/module/os.rs pyre/pyre-interpreter/src/module/nt.rs $(git ls-files pyre/pyre-interpreter/src/module | grep -E '(^|/)(os|nt)\.rs$'); do
if [ -f "$f" ]; then
echo "--- $f"
wc -l "$f"
rg -n -C6 'DirEntry|ScandirIterator|scandir|os_module|nt|register_module|module_ns_store|make_builtin_type|target_os = "windows|cfg!\(windows"' "$f" || true
fi
done
echo "--- os crate references"
rg -n -C4 'os_module|os_scandir|scandir_fn|DirEntry|ScandirIterator|pyre::module::os|module::os' pyre/pyre-interpreter/src pyre | head -250Repository: youknowone/pyre
Length of output: 26249
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "--- build config files mentioning posix/windows/os"
git ls-files | grep -E '(^|/)(Cargo.toml|Cargo.lock|pyproject.toml|oxc_build|build\.rs|Makefile|\.gclient|platform|windows)\.rs$' | head -200 || true
rg -n -C4 'target_arch.*wasm|windows|nt|os\scmodule|os_module|positive|posix|Platform|sys.platform|python314|platforms=' Cargo.toml configure.py pyproject.toml . | head -300 || true
echo "--- search for times_result_seq_type platform handling (mentioned in original comment)"
rg -n -C8 'times_result_seq_type|make_time|ScandirIterator|DirEntry|cfg!\(windows\)|target_os = "windows"' pyre/pyre-interpreter/src pyre/extra_tests/parity_tests | head -300
echo "--- read-only verifier: model name/module derivation and platform tests"
python3 - <<'PY'
from pathlib import Path
import re
f = Path("pyre/pyre-interpreter/src/module/posix/interp_posix.rs")
t = Path("pyre/pyre-interpreter/src/typedef.rs")
test = Path("pyre/extra_tests/parity_tests/dir_entry_uninstantiable_python314.py")
for sym in ["DirEntry", "ScandirIterator"]:
src = f.read_text()
for n, line in enumerate(src.splitlines(), 1):
if f'make_builtin_type("{sym}"' in line:
print(f"{f}:{n}: {line.strip()}")
print("typedef module derivation:")
text = t.read_text()
for n, line in enumerate(text.splitlines(), 1):
if "rsplit_once" in line and "{" in line:
start = n - 8
end = n + 10
print(f"typedef.rs:{start}-{end}")
for i in range(max(1,start), end+1):
print(f" {i}: {text.splitlines()[i-1]}")
print("test module expectation:")
lines = test.read_text()
for n, line in enumerate(lines.splitlines(), 1):
if "MODULE = " in line or "=='%s." in line:
print(f"{test}:{n}: {line.strip()}")
print("scan for any Windows module-name rewrite around these symbols:")
root = Path("pyre/pyre-interpreter/src")
matches=[]
for p in root.rglob("*.rs"):
s=p.read_text(errors="ignore")
if "DirEntry" in s or "ScandirIterator" in s or "times_result_seq_type" in s:
for n,line in enumerate(s.splitlines(),1):
if any(sg in line for sg in ["DirEntry", "ScandirIterator", "times_result_seq_type", 'make_builtin_type("nt', 'make_builtin_type("posix']):
matches.append((str(p),n,line.strip()))
for p,n,line in matches:
print(f"{p}:{n}: {line}")
PYRepository: youknowone/pyre
Length of output: 50373
Make the scandir-related typedef names platform-aware.
make_builtin_type("posix.DirEntry") and make_builtin_type("posix.ScandirIterator") register those types as posix.* even when Windows expects nt.*. Use the same cfg!(windows) branch used for times_result_seq_type, e.g. "nt.DirEntry"/"nt.ScandirIterator", so the test's MODULE = "nt" if sys.platform == "win32" else "posix" assertions pass on Windows.
[functional_correct]
🧰 Tools
🪛 Ruff (0.16.1)
[warning] 29-29: Use format specifiers instead of percent format
Replace with format specifiers
(UP031)
[warning] 34-34: Found assertion on exception exc in except block, use pytest.raises() instead
(PT017)
[warning] 34-34: Found assertion on exception exc in except block, use pytest.raises() instead
(PT017)
[warning] 34-34: Use format specifiers instead of percent format
Replace with format specifiers
(UP031)
[warning] 36-36: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 42-42: Use format specifiers instead of percent format
Replace with format specifiers
(UP031)
[warning] 43-43: Found assertion on exception exc in except block, use pytest.raises() instead
(PT017)
[warning] 43-43: Found assertion on exception exc in except block, use pytest.raises() instead
(PT017)
[warning] 45-45: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 55-55: Found assertion on exception exc in except block, use pytest.raises() instead
(PT017)
[warning] 55-55: Found assertion on exception exc in except block, use pytest.raises() instead
(PT017)
[warning] 55-55: Use format specifiers instead of percent format
Replace with format specifiers
(UP031)
[warning] 57-57: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 63-63: Use format specifiers instead of percent format
Replace with format specifiers
(UP031)
[warning] 64-64: Found assertion on exception exc in except block, use pytest.raises() instead
(PT017)
[warning] 64-64: Found assertion on exception exc in except block, use pytest.raises() instead
(PT017)
🤖 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 `@pyre/extra_tests/parity_tests/dir_entry_uninstantiable_python314.py` around
lines 21 - 64, Update the scandir-related builtin type registrations passed to
make_builtin_type so DirEntry and ScandirIterator use "nt.*" on Windows and
"posix.*" otherwise, matching the cfg!(windows) branching used for
times_result_seq_type. Preserve the existing type behavior while making their
module names platform-aware for the assertions in this test.
| # The listing spells the names back exactly, both as `str` and as `bytes`. | ||
| names = sorted(os.listdir(base)) | ||
| assert names == sorted([NAME + "_dir", NAME + ".txt"]), names | ||
| assert sorted(os.scandir(base), key=lambda e: e.name)[0].name == names[0] |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Close the scandir iterator.
sorted(os.scandir(base), ...) drops the iterator without closing it. On CPython the finalizer emits a ResourceWarning. If the parity runner compares stderr, that warning can differ between interpreters.
Use a with block.
♻️ Proposed change
-assert sorted(os.scandir(base), key=lambda e: e.name)[0].name == names[0]
+with os.scandir(base) as it:
+ assert sorted(it, key=lambda e: e.name)[0].name == names[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.
| assert sorted(os.scandir(base), key=lambda e: e.name)[0].name == names[0] | |
| with os.scandir(base) as it: | |
| assert sorted(it, key=lambda e: e.name)[0].name == names[0] |
🤖 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 `@pyre/extra_tests/parity_tests/os_non_ascii_path.py` at line 36, Wrap the
os.scandir(base) call in the assertion in a with block so the scandir iterator
is explicitly closed after sorting. Preserve the existing name ordering and
assertion behavior while updating the surrounding test flow to consume the
context-managed iterator.
| if crt_call!(libc::ftruncate(fd, size as libc::off_t)) < 0 { | ||
| return Err(fd_errno_err(crt_errno())); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Show the feature declarations for the interpreter crate.
fd -t f 'Cargo.toml' pyre --exec sh -c 'echo "== {}"; sed -n "/^\[features\]/,/^\[/p" {}'
# Show every reference to fd_errno_err with its surrounding cfg context.
rg -n -B12 'fd_errno_err' pyre/pyre-interpreter/src/builtins.rsRepository: youknowone/pyre
Length of output: 9850
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== builtins.rs around fd_errno_err definition and fileio truncation call"
sed -n '13920,13975p; 14250,14280p; 14490,14502p' pyre/pyre-interpreter/src/builtins.rs
echo
echo "== Cargo.toml features for pyre-interpreter"
sed -n '/^\[features\]/,/^\[/p' pyre/pyre-interpreter/Cargo.toml
echo
echo "== cfg-gate analysis for fd_errno_err usages"
python3 - <<'PY'
from pathlib import Path
import re
p = Path('pyre/pyre-interpreter/src/builtins.rs')
text = p.read_text()
def cfg_context(line_no):
lines = text.splitlines()
i = line_no - 1
# walk backward through cfg directives
directives = []
indent_stack = [] # not using for cfg, just show context
for j in range(i, max(0, i-35), -1):
m = re.search(r'^\s*#\s*\[[\w\s:#(),=\[\]|"-]+\]', lines[j])
if m:
directives.append((j+1, lines[j]))
return [(dirn, line.strip()) for dirn, line in directives[:10]]
for target in [13607, 13654, 13966, 14274]:
print(f"usage at line {target}")
for dn, dl in cfg_context(target):
print(f" {dn}: {dl}")
print()
# check truncated cfgs
for line_no in [13607, 13654, 13966, 14274]:
line = text.splitlines()[line_no-1].strip()
print(f"{line_no}: contains host_env: {'host_env' in line}, contains unix/not sandbox: {'unix' in line and 'sandbox' in line}")
PYRepository: youknowone/pyre
Length of output: 6141
Align the cfg gate on fd_errno_err with its fileio_method_truncate call site.
fd_errno_err is only compiled with host_env, but fileio_method_truncate compiles fd_errno_err(crt_errno()) under #[cfg(all(unix, not(feature = "sandbox")))]. With --no-default-features on a unix target, the truncate call is included without the helper unless sandbox is also enabled. Add feature = "host_env" to the truncate block, or expand the helper’s definition gate to cover the same condition.
Also applied to the nearby write_to_file error handling.
🤖 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 `@pyre/pyre-interpreter/src/builtins.rs` around lines 14273 - 14274, Align the
conditional compilation gates for fd_errno_err with every call site in
fileio_method_truncate and write_to_file. Ensure the truncate and nearby write
error-handling paths only compile when the helper is available, or broaden
fd_errno_err’s definition gate to exactly match those callers, including the
host_env requirement under unix and non-sandbox configurations.
| /// The message a Win32 error code names, with the trailing newline and | ||
| /// full stop `FormatMessageW` appends removed. Reporting the bare code is | ||
| /// the answer for a code the system cannot describe. | ||
| #[cfg(windows)] | ||
| fn win32_strerror(winerror: i32) -> String { | ||
| #[cfg(feature = "host_env")] | ||
| if let Some(message) = | ||
| rustpython_host_env::windows::format_error_message(Some(winerror as u32)) | ||
| { | ||
| return message | ||
| .trim_end_matches(|c: char| c <= ' ' || c == '.') | ||
| .to_string(); | ||
| } | ||
| format!("Windows Error 0x{winerror:x}") | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate the definition and signature of format_error_message.
rg -n -C10 'fn format_error_message' --iglob '*.rs' .
rg -n 'host_env' pyre/pyre-interpreter/Cargo.tomlRepository: youknowone/pyre
Length of output: 761
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Workspace Cargo manifests:"
fd -a 'Cargo.toml$' . | sed 's#^\./##'
echo
echo "Workspace package declarations for rustpython-host_env:"
rg -n -C3 'rustpython-host_env' pyre pyre/*/Cargo.toml Cargo.toml pyre/*/Cargo.lock pyre/*/Cargo.lock 2>/dev/null || true
echo
echo "Relevant Cargo.lock entries:"
if [ -f Cargo.lock ]; then
rg -n -C8 'name = "rustpython-host_env"|rustpython-host_env' Cargo.lock
fi
echo
echo "Search all tracked .rs files for format_error_message:"
rg -n -C8 'format_error_message' --iglob '*.rs' .Repository: youknowone/pyre
Length of output: 10106
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Git remotes:"
git remote -v || true
echo
echo "Fetching RustPython rev metadata only:"
curl -fsSL "https://github.com/@api/repos/RustPython/RustPython/git/refs/tags/v0.61.0" || true
curl -fsSL "https://github.com/RustPython/RustPython/raw/1613741f861b4f289da59ffc78a20ead4b4444fc/Cargo.toml" | sed -n '1,100p'
echo
echo "Fetching host_env package metadata from GitHub raw:"
curl -fsSL "https://github.com/RustPython/RustPython/raw/1613741f861b4f289da59ffc78a20ead4b4444fc/crates/rustpython-host_env/src/lib.rs" | sed -n '1,220p'
echo
echo "Fetching windows module from rustpython-host_env if present:"
for f in \
crates/rustpython-host_env/src/windows.rs \
crates/rustpython-host_env/src/windows/mod.rs \
crates/rustpython-host_env/src/win.rs \
crates/rustpython-host_env/src/win/mod.rs
do
if curl -fsSLI --head-only "https://github.com/RustPython/RustPython/raw/1613741f861b4f289da59ffc78a20ead4b4444fc/${f#/}" >/dev/null 2>&1; then
echo "--- ${f} ---"
curl -fsSL "https://github.com/RustPython/RustPython/raw/1613741f861b4f289da59ffc78ba4444fc/${f#/}" 2>/dev/null || curl -fsSL "https://github.com/RustPython/RustPython/raw/1613741f861b4f289da59ffc78a20ead4b4444fc/${f#/}" | sed -n '1,220p'
fi
done
echo
echo "Fetch format_error_message occurrences in that revision:"
curl -fsSL "https://github.com/@api/repos/RustPython/RustPython/git/trees/1613741f861b4f289da59ffc78a20ead4b4444fc?recursive=1" | jq -r '.tree[]? | select(.path | test("rustpython-host_env|crates/rustpython-host_env|format_error_message")) | .path' || true
curl -fsSL "https://github.com/RustPython/RustPython/raw/1613741f861b4f289da59ffc78a20ead4b4444fc/.gitmodules" 2>/dev/null || true
# Exact search across raw path is not single-API, but ripgrep a downloaded tree archive would need fetching it. Use targeted API path search?
# For now inspect likely source files containing windows module from raw.
curl -fsSL "https://github.com/RustPython/RustPython/raw/1613741f861b4f289da59ffc78a20ead4b4444fc/crates/rustpython-host_env/src/lib.rs" \
| rg -n -C8 'windows|format_error_message|GetLastError|FormatMessage' || trueRepository: youknowone/pyre
Length of output: 3568
🌐 Web query:
RustPython rev 1613741f861b4f289da59ffc78a20ead4b4444fc rustpython-host_env format_error_message
💡 Result:
The search query refers to components within the RustPython project, specifically the rustpython-host_env crate and a function or concept named format_error_message. Regarding rustpython-host_env: This is a dedicated crate within the RustPython project designed to abstract host-level system calls (such as filesystem access, networking, and process management) away from the core Python VM and standard library [1][2]. This architectural separation serves two main purposes: it enhances sandboxing by isolating system interactions, and it allows core components (like the compiler or parser) to remain no_std compatible by avoiding direct reliance on the host OS [1][2]. The crate contains modules for various system-level operations, including os, time, socket, mmap, and others [3][4][5][2]. Regarding format_error_message: There is no prominent, standard-library-level function named format_error_message inside the rustpython-host_env crate. The term "format_error_message" appears in several unrelated Rust and Python contexts, which may be the source of confusion: 1. It is a known function inside the clap_builder crate (a popular Rust CLI argument parser), which is often used in Rust projects, including RustPython's tooling, for formatting command-line interface errors [6][7]. 2. The term is also commonly used as a generic helper function name in various Python projects (e.g., in redis-py or other utility libraries) to standardize the formatting of exception messages [8][9][10]. The specific Git revision hash 1613741f861b4f289da59ffc78a20ead4b4444fc likely refers to a historical commit or snapshot within the RustPython repository. Given that rustpython-host_env is a structural crate and format_error_message is not a core part of its API, the query might be misidentifying a generic utility function from a dependency or a different part of the codebase.
Citations:
- 1: https://github.com/RustPython/RustPython/blob/main/host_env_proposal.md
- 2: https://docsearch.algolia.com/mcp/docs/repo/rustpython/rustpython
- 3: https://github.com/RustPython/RustPython/blob/885cf5c2/crates/host_env/src/os.rs
- 4: https://github.com/RustPython/RustPython/blob/885cf5c2/crates/host_env/src/lib.rs
- 5: https://github.com/RustPython/RustPython/blob/885cf5c2/crates/host_env/src/time.rs
- 6: https://codebrowser.dev/rust/crates/clap_builder/src/error/format.rs.html
- 7: https://mnemos.dev/doc/src/clap_builder/error/format.rs
- 8: https://github.com/unslothai/unsloth/blob/329f99d3/studio/backend/utils/utils.py
- 9: https://github.com/python-jsonschema/check-jsonschema/blob/ba8f7ac0/src/check_jsonschema/format_errors.py
- 10: https://github.com/redis/redis-py/blob/2b2f2cf5/redis/connection.py
🌐 Web query:
github RustPython 1613741f861b4f289da59ffc78a20ead4b4444fc crates rustpython-host_env
💡 Result:
The rustpython-host_env crate in the RustPython project is a specialized crate designed to abstract and isolate host operating system access [1][2]. Its primary purpose is to improve sandbox support by centralizing OS-level operations, which were previously managed via scattered feature flags (e.g., #[cfg(feature = "host_env")]) throughout the codebase [2][3]. Key aspects of the crate include: - Purpose: It aims to prevent host OS code from leaking into sandboxed builds by ensuring that only modules explicitly depending on rustpython-host_env can perform host-related operations [2]. - Design: It is designed to have zero dependency on the Python runtime, keeping it independent of the main RustPython virtual machine (VM) and common components [2]. - Functionality: It extracts OS abstractions previously found in the VM and standard library, such as those related to files, sockets, memory mapping, and system-specific APIs (posix, nt, etc.) [2]. - Integration: It is integrated into the workspace and is governed by a feature flag named host_env, which controls access to these host-specific abstractions [4][5][2]. Regarding the specific commit identifier 1613741f861b4f289da59ffc78a20ead4b4444fc, it is not present in the primary RustPython repository history visible in recent logs. It is likely a commit from a fork, a local branch, or a non-indexed history. Development on the rustpython-host_env crate was actively tracked in issues such as #7575 and associated proposals in the main repository as of April 2026 [2][3].
Citations:
- 1: https://github.com/RustPython/RustPython/blob/885cf5c2/crates/host_env/Cargo.toml
- 2: https://github.com/RustPython/RustPython/blob/main/host_env_proposal.md
- 3: rustpython-host_env for better sandbox support RustPython/RustPython#7575
- 4: https://github.com/RustPython/RustPython/blob/main/Cargo.toml
- 5: https://github.com/RustPython/RustPython/blob/885cf5c2/crates/vm/Cargo.toml
🏁 Script executed:
#!/bin/bash
set -euo pipefail
tmp="$(mktemp -d)"
cd "$tmp"
echo "Cloning RustPython shallowly at rev if reachable:"
git init
git remote add origin https://github.com/RustPython/RustPython.git
git fetch --depth=1 origin 1613741f861b4f289da59ffc78a20ead4b4444fc || true
if git rev-parse --verify FETCH_HEAD >/dev/null 2>&1; then
git checkout FETCH_HEAD --quiet
echo "Checked out fetched rev"
else
echo "Fetched rev unavailable"
fi
echo
echo "Locate format_error_message in RustPython checkout if present:"
rg -n -C8 'format_error_message|windows_sys::Win32::System::SystemServices::FORMAT_MESSAGE_FROM_SYSTEM|FormatMessageW|GetLastError' crates . 2>/dev/null | head -n 240 || true
echo
echo "List likely host-env source files:"
find crates -path '*rustpython*host*' -o -path '*host_env*' | sed -n '1,80p' || true
echo
echo "Search workspace packages for host_env rev at checkout:"
rg -n -C2 'rustpython-host_env|host_env_proposal|name = "host_env"|\[package\].*host_env' Cargo.toml crates Cargo.lock 2>/dev/null | head -n 220 || trueRepository: youknowone/pyre
Length of output: 19899
🏁 Script executed:
#!/bin/bash
set -euo pipefail
tmp="$(mktemp -d)"
cd "$tmp"
git init
git remote add origin https://github.com/RustPython/RustPython.git
git fetch --depth=1 origin 1613741f861b4f289da59ffc78a20ead4b4444fc || true
git checkout FETCH_HEAD --quiet
echo "Exact format_error_message occurrences:"
rg -n -C12 'pub fn format_error_message|fn format_error_message|FormatMessageW|GetLastError|winerror' crates/host_env/src crates/host_env/Cargo.toml crates/vm/src crates/stdlib/src -g '*.rs' 2>/dev/null || true
echo
echo "Relevant windows.rs excerpt:"
sed -n '1,260p' crates/host_env/src/windows.rs 2>/dev/null | nl -ba
echo
echo "Relevant windows.rs excerpt around FormatMessage:"
rg -n -C18 'FormatMessageW|format_error_message|winerror|Option<u32>|u32|String' crates/host_env/src/windows.rs 2>/dev/null || trueRepository: youknowone/pyre
Length of output: 50374
Fix the rustpython-host_env error-formatting path.
rustpython-host_env::windows::format_error_message(Some(winerror as u32)) is not the matching API: the RustPython side exposes rustpython_host_env::overlapped::format_message(error_code: u32) and it already returns String. Call that API instead so the dependency remains in sync.
🤖 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 `@pyre/pyre-interpreter/src/error.rs` around lines 966 - 980, Update
win32_strerror to call rustpython_host_env::overlapped::format_message(winerror
as u32) instead of rustpython_host_env::windows::format_error_message,
preserving the existing trimming and fallback behavior while using the API that
returns String directly.
| // os.link(src, dst) -> None — a second name for the file `src` names, | ||
| // both of which the failure reports. | ||
| #[cfg(not(feature = "sandbox"))] | ||
| crate::module_ns_store( | ||
| ns, | ||
| "link", | ||
| crate::make_builtin_function("link", |args| { | ||
| let (args, kwargs) = crate::builtins::split_builtin_kwargs(args); | ||
| crate::builtins::kwarg_reject_unknown( | ||
| kwargs, | ||
| &["src_dir_fd", "dst_dir_fd", "follow_symlinks"], | ||
| "link", | ||
| )?; | ||
| link_positional(args)?; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
f=pyre/pyre-interpreter/src/module/posix/interp_posix.rs
rg -n 'cfg\(|link_positional|"link"|"get_inheritable"' "$f" | awk -F: '$1>2700 && $1<6700'Repository: youknowone/pyre
Length of output: 5784
🏁 Script executed:
#!/bin/bash
set -euo pipefail
f=pyre/pyre-interpreter/src/module/posix/interp_posix.rs
sed -n '2680,2770p' "$f"
sed -n '4480,4580p' "$f"
sed -n '4800,4855p' "$f"
sed -n '5030,5055p' "$f"
sed -n '5908,6215p' "$f"
sed -n '6608,6650p' "$f"Repository: youknowone/pyre
Length of output: 27784
Gate the link registration behind the features its body uses. The #[cfg(not(feature = "sandbox"))] link registration in interp_posix.rs still calls rustpython_host_env::posix::linkat, but the Rust linkat helper is only compiled with #[cfg(all(unix, feature = "host_env"))]; an older nightly with RUST_LOG=trace can use this module, but Windows and non-host-Unix sandboxed builds fail to link with undefined reference to rustpython_host_env::posix::linkat. Keep link and its Unix link_positional gate synchronized with the helper that provides the host_posix::linkat call path.
📍 Affects 1 file
pyre/pyre-interpreter/src/module/posix/interp_posix.rs#L4550-L4563(this comment)pyre/pyre-interpreter/src/module/posix/interp_posix.rs#L2744-L2753
🤖 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 `@pyre/pyre-interpreter/src/module/posix/interp_posix.rs` around lines 4550 -
4563, Gate the `link` registration and its `link_positional` implementation at
both interp_posix.rs sites (2744-2753 and 4550-4563) with the same `unix` and
`host_env` feature conditions required by `rustpython_host_env::posix::linkat`;
keep the registration and helper gates synchronized so unsupported Windows or
non-host-Unix builds do not reference the unavailable helper.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ede7a44cfb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if args.len() < 2 { | ||
| return Err(crate::PyError::type_error("chmod() requires 2 arguments")); | ||
| } | ||
| let path = crate::gateway::fsencode_path_w(args[0])?; |
There was a problem hiding this comment.
On Windows, _have_functions advertises HAVE_FCHMOD, so os.py places os.chmod in os.supports_fd, but this implementation always passes its first argument through the path-only converter. Consequently os.chmod(open_fd, mode) raises TypeError instead of changing the open file; the upstream chmod dispatch explicitly detects an integer path and invokes fchmod.
AGENTS.md reference: AGENTS.md:L231-L233
Useful? React with 👍 / 👎.
| crate::builtins::kwarg_reject_unknown( | ||
| kwargs, | ||
| &["operation", "arguments", "cwd", "show_cmd"], | ||
| "startfile", |
There was a problem hiding this comment.
On Windows, filepath is a positional-or-keyword parameter of os.startfile, but the keyword allowlist omits it and the implementation subsequently requires a positional argument. A valid call such as os.startfile(filepath=name) therefore raises an unexpected-keyword TypeError; this function needs full duplicate-aware signature binding rather than handling only the optional keywords manually.
AGENTS.md reference: AGENTS.md:L231-L233
Useful? React with 👍 / 👎.
`stat_entry` consults `HAVE_FSTATAT` while unwrapping `dir_fd`, above the descriptor branch, so the descriptor+`dir_fd` conflict is unreachable where `fstatat` does not exist. The comment claimed both fd-conflict rejections come first. #1081 corrected the same claim in `extra_tests/parity_tests/os_stat_file_descriptor.py` and cites `_DirFD_Unavailable` (`interp_posix.py:285-292`) for it; this is the statement of it that sits next to the code. Assisted-by: Claude
`stat_entry` consults `HAVE_FSTATAT` while unwrapping `dir_fd`, above the descriptor branch, so the descriptor+`dir_fd` conflict is unreachable where `fstatat` does not exist. The comment claimed both fd-conflict rejections come first. #1081 corrected the same claim in `extra_tests/parity_tests/os_stat_file_descriptor.py` and cites `_DirFD_Unavailable` (`interp_posix.py:285-292`) for it; this is the statement of it that sits next to the code. Assisted-by: Claude
… on a non-measurement (#1095) * jit: stamp the qmut abort's own subwalk coordinate, and re-seed a live-NULL operand slot A walk that executed residual side effects and then fails to commit its end state falls back to the legacy replay from the traced region's entry, which runs those residuals a second time. Two shapes reached that fallback; both show up under `PYRE_FBW_CENSUS=1` as `committed=false effects>0`. `WalkSession::abort_in_subwalk` is sticky — `claim_abort_coordinate` only ever sets it — so an inline sub-walk abort the walk recovered from left it true for every later abort in the same trace attempt, and `flush_qmut_abort_state`'s gate then read a root-frame abort as a callee coordinate. The `ForceQuasiImmutable` raise in `dispatch_residual_call_iRd_kind` now stamps it from `fbw_mode.inline_subwalk` at the raise point, as the two kept-stack branch-guard raises already do. `reseed_vstack_from_shadow` rejected a NULL const-ptr shadow slot outright, because a NULL there can also mean a slot the portal never wrote. It now accepts one carrying the `virtualizable_live_null_slots` marker, which records that the last executed store into that slot wrote a NULL. PUSH_NULL's `self_or_null` sentinel is such a slot and stays live across the whole callable/args/kwargs build ahead of a CALL; the reorder region re-seeds the mirror in the middle of that build, and the rejected slot made `capture_vstack_mirror_image` refuse the image, leaving an escape inside the call with no blackhole resume. `capture_vstack_mirror_image`'s decline line gains the Python pc and the mirror boxes. The LoadName cell-fold gate comment is rewritten to the measured state: with the gate lifted the `bench/synth` corpus is output-correct, and what fails is `exception_reraise_tb_depth_jitstress` at 13.0x against its 4x pypy gate plus four benches' jit-stats. Measured with the gate lifted, in-place arms: `iter57/real_exception` 100003 -> 100000, `exception_reentry_guard_finally_residual` `leaked 4 reentry 2` -> `leaked 0 reentry 0`. Assisted-by: Claude * jit: record why reseed_vstack_from_callee_shadow keeps its NULL const-ptr rejection The callee-shadow reseed is the structural twin of `reseed_vstack_from_shadow` and rejects a NULL const-ptr the same way, but its source is a sparse `HashMap`, where a present key is already the write-witness the dense virtualizable array needed a per-slot side table to supply. So the clause discards a proven write whose value happens to be PUSH_NULL's `self_or_null`. Measured before writing this: dropping the clause leaves `check.py --backend dynasm` at 386/386 with no jit-stats movement and no baseline change, so the corpus does not distinguish the two behaviours. Behaviour unchanged; the comment records the asymmetry and the measurement. Assisted-by: Claude * rework.md: refresh the audit against the current tree The findings were measured on `pc-map` on 2026-07-05. Re-measured on `ec-wiring` at base 58fcd37, thirteen of the fifteen issues the document tracks are closed and the priority order has inverted. F1: gh#366/367/368/369 closed; `metadata.pc_map` and `resume_jitcode_pc_for` have zero hits and `resume::SnapshotFrame.pc` is the JitCode byte offset. The surviving `pc_map` matches are the compile-time exit-recovery `Vec<usize>` in jit/codewriter.rs and jit/flatten.rs, a different thing. Residue recorded: recorder.rs's SnapshotFrame doc still describes the deleted translation, py_pc is stored rather than derived, and build_state_field_snapshot stamps the JitCode offset into py_pc (unproven, needs a repro). F2: verified done — `is_full_body_walk`, `PYRE_FULL_BODY_WALK` and `OpcodeHandler for MIFrame` have zero hits each. F3: regressed to 15 registrations against MAX_EXTRA_ROOT_WALKERS = 16; the 16th caller hits `panic!("capacity exceeded")` at startup. F4: gh#346 and gh#373 closed, coverage still landing (#1065); abort_permanent unchanged in scale, but the exit criterion is the census, not a match count. F5: gate-triage.md now exists but the population grew from 119 matches to 245 distinct PYRE_* identifiers. Sequencing amended to WS3 > WS2 > WS1-residue > WS4. Assisted-by: Claude * rework.md: correct the F5 gate count to a reproducible measurement The refresh recorded 245 distinct `PYRE_*` identifiers against the audit's original 119. That figure does not reproduce: tracked `*.rs` holds 131 distinct identifiers, all tracked files 174, and 548 raw matches. The quantity comparable to the original "distinct `PYRE_*` env gates" is the set of names actually read from the environment, which is 126. The command is now stated in the document so the number can be re-derived, along with the three other counts it is easy to confuse it with. Assisted-by: Claude * check.py: do not fail a ratio gate whose baseline is clamped to the floor `_exec_time` clamps a startup-subtracted time to `EXEC_TIME_FLOOR_S` so ratios cannot divide by ~0. When the pypy baseline lands there, the ratio is `pyre_exec / EXEC_TIME_FLOOR_S` and the ceiling it is compared against is an absolute wall-clock budget of `ceiling * EXEC_TIME_FLOOR_S` seconds, fitted on whichever host wrote the header. The comparison table already marks those ratios `~` and prints "ratio is not a measurement"; the gate failed the run on them anyway. `failed_bound` now returns None whenever the baseline is clamped, instead of requiring the backend to be at the floor as well. Only the ceiling changes behaviour: the floor arms at `exec_baseline >= FLOOR_GATE_MIN_BASELINE_S`, which a clamped baseline is always under. The gate can therefore only pass more than before, never fail more. The `[... clamped to floor; ratio not a measurement]` suffix in `_gate_fail_detail` is unreachable once a clamped baseline returns no bound, and is removed; the `~` legend states the consequence instead. Three consecutive `main` runs failed this way on three different fixtures across two runners: global_cell_shortpreamble_hot 24.1x > 19x and class_reassign_hot 49.2x > 47x on ubuntu-24.04, reentrant_key_eq_mutation 10.3x > 5x on macos-latest (runs 31079972573, 31080288895). Discriminator, cranelift, `class_reassign_hot` with its ceiling temporarily set to 1: the previous check.py reports SLOWER "exec 0.13s > pypy 0.01s ratio 27.0x > gate 1x [pypy exec clamped to floor; ratio not a measurement]", this one reports PASS. With the same ceiling of 1 on seqiter_tuple_error_parity, whose pypy exec is a measurement, this check.py still reports SLOWER at 18.3x — the ceiling is untouched wherever the baseline is real. The three fixtures above pass with their own ceilings restored. Assisted-by: Claude * posix: correct which stat rejection precedes the platform's dir_fd check `stat_entry` consults `HAVE_FSTATAT` while unwrapping `dir_fd`, above the descriptor branch, so the descriptor+`dir_fd` conflict is unreachable where `fstatat` does not exist. The comment claimed both fd-conflict rejections come first. #1081 corrected the same claim in `extra_tests/parity_tests/os_stat_file_descriptor.py` and cites `_DirFD_Unavailable` (`interp_posix.py:285-292`) for it; this is the statement of it that sits next to the code. Assisted-by: Claude * bench: re-record the wasm jit-stats for exception_reused_object_tb_not_doubled `fbw_blackhole_adopted_single_frame` reads 3 where the baseline had no entry for it. `loops_compiled=4` and `bridges_compiled=3` are unchanged, so the trace shape is the same and what moved is that the walk now adopts the blackhole resume image instead of falling back to the replay from the traced region's entry. Attributed by measuring both arms with the same command, `check.py --backend wasm --synthetic-only --synthetic-pattern exception_reused_object_tb_not_doubled`: with `ff503b5d746` reverse-applied in place the bench reports ALL PASSED against the existing baseline, and with it restored it reports the 0 -> 3 change. The control arm took 2m32s against the treatment arm's 4s, which is the wasm module being relinked rather than reused. The counter arrived with #1064 and this bench's baselines were last recorded at `da5e6fb38c7` (#1059), so absence from the baseline did not by itself say which of the two it was. No CI job runs `--backend wasm`, so the wasm baselines are not gated there either. The other four keys the re-record adds -- fbw_blackhole_adopted_multi_frame, fbw_store_journal_rollback_failed, field_pos_attached_misplaced, field_pos_spec_misplaced -- are counters that did not exist at #1059 and are pinned at 0 here for the first time. The dynasm and cranelift baselines are not re-recorded: both backends still report ALL PASSED for this bench. Assisted-by: Claude * bench: restore bridges_compiled and guard_failures on three synth baselines `9d2fff92649` (#1063) re-recorded 993 jit-stats baselines. All but four gained only the two new `field_pos_*_misplaced=0` keys; three changed a value: binary_int_overflow_local_resume bridges 5 -> 6 guards 647 -> 686 exc_bridge_entry_guard_not_removed bridges 4 -> 5 guards 809 -> 1009 list_append_write_barrier_gc bridges 5 -> 6 guards 1345 -> 1562 Five runs report the pre-#1063 values and none reports the recorded ones: dynasm, cranelift and wasm here, and `main`'s own CI on ubuntu-24.04 and macos-latest at 9d2fff9 -- run 31139317566, jobs 92747505633 and 92748753166, on a tree carrying no commit from this branch. The three benches fail identically on all three backends in each of them. Only those two keys are restored; #1063's two added keys stay. The fourth bench it revalued, getattribute_override_no_bind, is left as recorded: it passes here and in that CI run, so its new values do reproduce. Assisted-by: Claude
… jit: pop-fold guard order and recorded-raise roots; typedef: __init_subclass__ keywords (#1204) * jit: read the recorded-raise roots back out of their shadow slots `walker_emit_recorded_builtin_raise` pinned `exc` and each argument and then kept using the locals it had handed to `pin_root`. `RootScope::pin_root` normalizes the address it publishes once a second mutator has existed (`gc_roots.rs`), so past that point the slot and the caller's copy can name different objects, and these values are baked into the trace as `ConstPtr`s that outlive the walk. `args_storage` carried no root at all: it was read off `exc` before the normalization could apply and then indexed once per argument. Take every value back from `shadow_stack_get` after pinning it, and pin `args_storage`, which is the shape the concrete-shadow build for zip/tuple in this file already uses. Assisted-by: Claude * typedef: refuse __init_subclass__ keywords the way parse_obj does The default `__init_subclass__` reported a leftover class-definition keyword as `ArgErr::UnknownKwds`, so `class C(Base, flag=1)` raised "got an unexpected keyword argument 'flag'". `parse_obj` does not report the keyword when the signature has neither `**kwargs` nor a keyword-only argument (`argument.py:377-380`); it collapses every such refusal to "takes no keyword arguments". This signature is `cls` alone, so that branch always applies. CPython 3.14 raises the same sentence. The qualname ahead of the `()` is left as it was: pyre spells it `object.__init_subclass__` and CPython spells it with the subclass, and that choice is not what this changes. The snippet asserted only that the message mentioned `__init_subclass__`, which held for both shapes; it now pins the sentence and that the keyword is not named. Assisted-by: Claude * parity-tests: pin isinstance over a class that overrides __class__ `isinstance` reads `obj.__class__` on an MRO miss, so a `@property __class__` decides the answer and runs on every call: `isinstance(Masked(), int)` is True here, not the False a miss looks like. Nothing in the CPython suite runs that shape hot enough to trace. Counts are printed rather than asserted, so a fold that elided the call, cached its result, or answered False shows up as a diff against CPython. Scope is recorded in the header and was measured, not assumed: this does not reach `observed_replay_safe_isinstance`, whose only consumer is the nested residual abort inside an inline sub-walk. At this call depth the residual is a plain `call_may_force` and that gate no-ops; substituting the weaker predicate it used to carry leaves every line byte-identical. The gate's witness is `bench/synth/foriter_isinstance_class_property_replay.py`. Assisted-by: Claude * parity-tests: end isinstance_class_property with the harness's OK line run.py:172 accepts a case only when the last non-empty stdout line is "OK"; without it all three runners, cpython included, report the file as a failure. Assisted-by: Claude * intobject: correct the claim that gc_interp is off on the native backends `w_int_gc_alloc`'s doc justified keeping its `dont_look_inside` boundary on the grounds that it "costs nothing where the arm is unreachable: `gc_interp::enabled()` is false on the native backends". `enabled_from_env` (`gc_interp.rs`) answers true for every `PYRE_GC_INTERP` value except exactly `0`, so it is on by default on every target, and this arm — not the `malloc_typed` one `fuse_boxing_alloc` rewrites — is the arm `w_int_new` takes. Assisted-by: Claude * jit: separate the pop fold's lock-guard-free label from its trace-guard order `w_list_pop_end`'s doc defines "the descended body must hold no guard" as the absence of a `w_list_lock` acquire/release pair, which is what declines the fold's sub-walk. `try_walker_orthodox_list_pop` and `list_pop_end_jitcode` repeated the word without that qualifier, where it reads as a claim about trace guards. The sub-walk gets no callee frame, so a guard recorded inside it resumes at the caller's CALL boundary and re-executes the whole `pop()`. Every guard the Integer arm can record lands ahead of its `ll_list_int_set_len`: the sole op after that store is the `w_int_new` call, which `dispatch_inline_call_dir_kind` short-circuits into `walker_box_int` (`NewWithVtable` + `SetfieldGc`, no guard recorded) and returns before `run_sub_jitcode_walk`. Nothing asserts the ordering. Assisted-by: Claude * jitcode_dispatch: check the pop fold's guard/store order instead of assuming it `orthodox_list_pop_commit` descends `w_list_pop_end_inner` with no callee frame, so a guard recorded inside resumes at the caller's CALL boundary and re-executes the whole `pop()`. That is sound only while every guard lands before the body's first committed store. Today it does — the Integer arm's `ll_list_int_set_len` is followed only by the `w_int_new` call, which `dispatch_inline_call_dir_kind` short-circuits into `NewWithVtable` + `SetfieldGc` and records no guard — but nothing read the order. `subwalk_guard_follows_store` scans the ops recorded since a `TracePosition` and reports whether a guard follows a `setfield` / `setarrayitem` / `setinteriorfield`. A `start` past the end of the ops vector reports true: the window is gone, so an empty read is not an answer. The commit captures the position before `run_sub_jitcode_walk` and declines with `OrthodoxSubWalkTraceUnsupported` on a positive read, which cuts the tentative IR back to the generic residual. The decline takes the same `w_list_len == len_before` re-read the apply below does: where the arm's store keeps a runtime binding the sub-walk executed it for real, and cutting back to a residual that pops again is the double-apply the append side already had to fix. Unit test covers guard-then-store, store-then-guard, a store recorded before the captured position, `SetarrayitemGc`, and a position past the end. `pyre/bench/synth/list_pop_append.py` still reads 2.2x against its `max-pypy-ratio=22`; it read 73.5x before the fold existed. Assisted-by: Claude * docs: record the spec-vs-implementation ruling the parity review keeps re-deriving Six review findings across PRs #1001, #1079, #1081, #1085 and #1113 are one policy question, not six bugs: pyre follows CPython for what a Python program observes while the review measures every line against PyPy. Nothing in the repo stated the split, so each cycle re-filed them under sections 1/2. The ruling is that pyre's implementation is a port of PyPy and pyre's spec is CPython 3.14. Six of the seven adjudicated cases carry no version delta at all (`sched_setscheduler` has returned None since 3.3, `PyUnicode_FSConverter` has accepted bytes since 3.3, PEP 529 surrogatepass is 3.6, `DirEntry` has cached its `stat_result` since PEP 471), so "3.14" names which CPython to read rather than a lag PyPy is expected to close. - AGENTS.md gains the normative section and the six tests in short form. - The `/parity` skill gains a fourth deviation class, SPEC-DEVIATION, exempt from Principle 6's auto-fix (reverting one re-introduces a known bug), plus the full procedure with its evidence rules and worked examples. - `.github/codex-review-prompt.md` replaces the "Python 3.11 vs 3.14" exception with the four conditions a section-4 entry must carry. Structure — names, module paths, control-flow order, data structures, storage owner, JIT hints — is outside the ruling and follows PyPy unconditionally. A finding where PyPy's shape serves a mechanism pyre also has stops at PyPy: the `DirEntry.stat()` object cache is one, since `interp_posix.py:537-542` states the per-call rebuild is what keeps the allocation virtual. Assisted-by: Claude * _sre: drive an ASCII str subject as bytes and read the stored length `Subject::len()` called `code_points().count()` and `char_to_byte` called `code_point_indices().nth(pos)`, so every match walked the subject before the engine started; `Request::new` and `create_cursor` then walked it again. Add `Subject::AsciiStr` for a `str` whose code points are one byte each, selected by `w_str_is_ascii` where `make_ctx` selects `is_ascii()` (interp_sre.py:246). It drives the WTF-8 payload as bytes, so a character position is already a byte offset -- `UnicodeAsciiMatchContext` (interp_sre.py:52). `StrDrive` is `count` and cursor arithmetic only and every unicode decision keys on the compiled pattern's opcode, which is the property that lets upstream spell that context as a bare `StrMatchContext` subclass. `Subject::Str` now carries the object (`ctx.w_unicode_obj`, interp_sre.py:250) and reads the stored `_len()` and `_index_to_byte` rather than re-deriving them. Its positions remain code point indices that the `Wtf8` driver still resolves by walking; the note on the variant records what converting the reported spans would take. `slice_subject`, `empty_subject` and `finish_output` branch on `is_unicode()`, and `subject_span_bytes` extracts the position mapping and slices once; `char_len` and `char_slice` are gone. On an ASCII subject with n=1.6M, `pat.match(s, pos)` measured 130.7us at pos=0 and 762us at pos=n-10; both are now 0.35us, flat in n and in pos. A differential run over match/search/fullmatch spans, pos/endpos sweeps, findall/finditer/split/sub/subn/expand, bytes/bytearray/memoryview, type mismatches, a str subclass and scanner positions is byte-identical to CPython 3.14.6 on all 608 lines, as it was before the change. check.py --backend dynasm: 425/425. Assisted-by: Claude * BINARY_SLICE: convert str bounds through the index storage `binary_slice_values`'s `str` branch collected the byte offset of every code point in the subject into a `Vec<usize>` to resolve two bounds, so `s[a:b]` cost the whole string. A one-character slice of a 200k subject measured 752us, against 0.42us for the same slice written as a prebuilt slice object, which reaches `w_str_slice_codepoints` and walks only the sliced elements. Read the stored `_length` and convert the two bounds with `_index_to_byte` (unicodeobject.py:1251), which is what the slice-object path already does. The clamping and the `.max(s)` on the stop bound are unchanged, and a bound equal to the count still resolves to the end of the buffer. `binary_slice_values` is shared with the JIT residual (`bh_binary_slice_fn`, call_jit.rs:5765), so both consumers get it. The compiler folds constant bounds to `LOAD_CONST slice` + `BINARY_OP []` and emits `BINARY_SLICE` only for computed ones, so this is the path `json/decoder.py` takes with its per-token `s[end:end + 1]`. Decoding a flat 208 KB ASCII payload with the pure-Python scanner: 25.85s -> 0.089s, with the size sweep going from x3.95/x4.13/x9.39 per doubling to x2.07/x2.62/x1.77. `s[p:p+1]` on a 200k subject: 752us -> 0.54us. A differential run over 12 subjects (ASCII, 2/3/4-byte, lone surrogates, empty, and lengths on the 63/64/65/128 index-storage block boundaries) against 19x19 bound pairs in both spellings, plus list/tuple/bytes slicing and slice assignment, is byte-identical to CPython 3.14.6 on all 4430 lines. check.py --backend dynasm: 425/425. Assisted-by: Claude * _sre: resolve non-ASCII str positions through the stored index storage Subject::Str drove the engine over &Wtf8, whose StrDrive::count counts every code point and whose create_cursor(n) steps over the first n of them. Both run once per match, so a scan that restarts at successive positions walked the subject again on every call. Add Utf8Drive, which carries the W_UnicodeObject next to the payload and answers count with w_str_len and create_cursor with w_str_index_to_byte, minting the cursor at the head of an O(1) suffix reslice. Positions stay code point indices and stepping delegates to the &Wtf8 impl, so the engine's position arithmetic is unchanged. Assisted-by: Claude
`wrap_value_expr` lowered a `String` return to `w_str_new` and a `Wtf8Buf` return to `w_str_from_wtf8`. Both allocate the value through `malloc_raw` (`pyre-object/src/lltype.rs`), a bare `Box::into_raw` that registers no owner. `w_str_new`'s own doc scopes that immortal default to the structural strings -- dict keys, code constants, names -- and directs "dynamic, short-lived strings that live only in GC-traced slots" to `w_str_new_managed`. A builtin's return value is the latter, so it now takes the `*_managed` twins. `impl PywrapKind for String` takes them too: the `Vec<T>` return arm lowers to `w_list_new(v.into_iter().map(PywrapKind::into_py))`, so a `Vec<String>` return reaches PywrapKind rather than the `"String"` arm. `impl PywrapKind for &str` keeps `w_str_new` -- a borrowed `&str` in those macros is a literal at the call site -- as do the `__doc__` and property-name lowerings, whose strings are held by off-GC structures. This makes the macro agree with `gateway.rs fsdecode_filename_bytes`, which has used `w_str_from_wtf8_managed` for the same category of value since #1081. NO RSS IMPROVEMENT IS CLAIMED OR MEASURED. The reproducer this change was written for -- 500,000 `os.getcwd()` calls -- does not reach it: `getcwd` is a raw closure, not a `#[pyre_function]`, and its result already came from `fsdecode_filename_bytes`, i.e. already from the managed allocator. That workload still grows peak RSS by 82.2 MB, so the managed constructor does not bound it. The live user of the arm this change touches is `_opcode.get_opname`. Assisted-by: Claude
…nstructor" This reverts 549e988. Three reasons, none of which were known when it landed: 1. Its motivation was refuted by its own reproducer. `os.getcwd` is not a `#[pyre_function]` -- it is a raw closure under `make_builtin_function_with_arity` returning `gateway::fsdecode_filename_bytes`, which has used `w_str_from_wtf8_managed` since #1081. The 500,000-call workload was already on the managed allocator, so it never measured the immortal path the change was aimed at, and it still grows peak RSS by 82.2 MB after the change. 2. No benefit is measured. Nothing demonstrates the conversion improves anything. 3. It changes trace shape and that was never evaluated. `w_str_new` carries `#[majit_macros::dont_look_inside]`; `w_str_new_managed` does not. Routing the macro's lowering to the un-annotated twin means the JIT traces the construction instead of residualising it, at every converted site. The doc-conformance argument for the change still stands and is worth revisiting, but it needs the annotation question answered and a gate that reaches the code. Assisted-by: Claude
`wrap_value_expr` lowered a `String` return to `w_str_new` and a `Wtf8Buf` return to `w_str_from_wtf8`. Both allocate the value through `malloc_raw` (`pyre-object/src/lltype.rs`), a bare `Box::into_raw` that registers no owner. `w_str_new`'s own doc scopes that immortal default to the structural strings -- dict keys, code constants, names -- and directs "dynamic, short-lived strings that live only in GC-traced slots" to `w_str_new_managed`. A builtin's return value is the latter, so it now takes the `*_managed` twins. `impl PywrapKind for String` takes them too: the `Vec<T>` return arm lowers to `w_list_new(v.into_iter().map(PywrapKind::into_py))`, so a `Vec<String>` return reaches PywrapKind rather than the `"String"` arm. `impl PywrapKind for &str` keeps `w_str_new` -- a borrowed `&str` in those macros is a literal at the call site -- as do the `__doc__` and property-name lowerings, whose strings are held by off-GC structures. This makes the macro agree with `gateway.rs fsdecode_filename_bytes`, which has used `w_str_from_wtf8_managed` for the same category of value since #1081. NO RSS IMPROVEMENT IS CLAIMED OR MEASURED. The reproducer this change was written for -- 500,000 `os.getcwd()` calls -- does not reach it: `getcwd` is a raw closure, not a `#[pyre_function]`, and its result already came from `fsdecode_filename_bytes`, i.e. already from the managed allocator. That workload still grows peak RSS by 82.2 MB, so the managed constructor does not bound it. The live user of the arm this change touches is `_opcode.get_opname`. Assisted-by: Claude
…nstructor" This reverts 549e988. Three reasons, none of which were known when it landed: 1. Its motivation was refuted by its own reproducer. `os.getcwd` is not a `#[pyre_function]` -- it is a raw closure under `make_builtin_function_with_arity` returning `gateway::fsdecode_filename_bytes`, which has used `w_str_from_wtf8_managed` since #1081. The 500,000-call workload was already on the managed allocator, so it never measured the immortal path the change was aimed at, and it still grows peak RSS by 82.2 MB after the change. 2. No benefit is measured. Nothing demonstrates the conversion improves anything. 3. It changes trace shape and that was never evaluated. `w_str_new` carries `#[majit_macros::dont_look_inside]`; `w_str_new_managed` does not. Routing the macro's lowering to the un-annotated twin means the JIT traces the construction instead of residualising it, at every converted site. The doc-conformance argument for the change still stands and is worth revisiting, but it needs the annotation question answered and a gate that reaches the code. Assisted-by: Claude
…AD_METHOD binding; dict view indexing (#1394) * jit: stamp last_instr on the frame a blackhole traceback node names `record_caught_blackhole_traceback` resolved the raise coordinate and stored it on the PyTraceback node, but left `PyFrame.last_instr` alone. A frame the walker seeded for an inlined callee takes no per-opcode `last_instr` store (it is not the virtualizable) and `publish_last_instr_at_live_marker` fires only for instructions the blackhole replays, so a level the exception merely passes through reached the node still holding the `-1` the frame constructor wrote, or the `pc - 1` resume coordinate a walk-end flush left. Measured on a two-exception-class loop with an inlined intermediate frame: `tb_lasti` was 14 on every shape while `tb_frame.f_lasti` read -2 on 18843 of 60000 iterations and 20 on 205 more; PYRE_JIT=off, python3 and pypy all read 14. `PYRE_M73_LASTINSTR_AUDIT=1` prints the same frames arriving at this site as `inv=7 fwd=0` and `inv=7 fwd=11`. The store is placed after the audit call so the knob keeps reporting the incoming state. It is idempotent for a frame that already carries the coordinate, and the coordinate's own fallback reads this field, so an unmappable coordinate writes back what is already there. Adds pyre/bench/synth/traceback_inlined_callee_lasti_regression.py, which asserts `f_lasti == tb_lasti` on every frame the exception has already left and fails on the unfixed binary. Assisted-by: Claude * object: index the storage directly for module and identity dict views `DictStrategy::nth_item` answers the dict-view iterator's per-step fetch. Its default materialises the whole `items()` and takes `.nth(index)`, which the doc calls fine for the tiny empty strategies. `ModuleDictStrategy` and `IdentityDictStrategy` had no override, so one walk of an n-entry dict ran that n times. For a module dict the materialisation wraps every name with `w_str_new`, which allocates through `malloc_raw` and is never reclaimed. A 350-name module dict walked ten times grew peak RSS by 156.8 MB against none on CPython, and the walk is on the startup path -- `dir(module)` inside `importlib._bootstrap`. Removing it takes interpreter startup from 63.9 to 53.6 MB peak RSS and 48.0 to 37.7 MB physical footprint, measured against a control built from the same HEAD with the same freshly extracted LLBC. For an identity dict the vectors are ordinary Rust heap, so the cost is time. Walking a freshly built 4000-entry dict took 140x a str-keyed walk of the same size, against 1.1x on CPython; at 2000 entries the key-type sweep read str 1.0x, int 1.3x, bytes 2.0x, `__hash__`-defining instances 1.0x, tuple 1.0x and identity 55.2x. With the override identity reads 0.7x. Adds `w_module_dict_nth_item` / `w_module_dict_nth_value`, which wrap only the requested name and skip the wrap entirely for a `values()` view, plus `IndexMap::get_index` overrides on both strategies. Both fixtures were shown to fail on a control binary before the change. Assisted-by: Claude * jit: record why the eager descriptor rehydration stays eager `rehydrate_build_descr_raw_sets` decodes 11,967 bincode records and reads as the obvious place to make the first JIT stop rebuilding the descriptor universe. The doc comment now records what a reader needs before taking that on. It is a first-JIT cost, not a startup one: the function is reachable only through `ensure_finish_setup`, and every caller of that sits on a JIT path. Its size is not measured, and two obvious instruments cannot measure it. Allocation is mmap-backed, so `malloc_history` and `heap` do not see the object heap; and subtracting two peak RSS readings does not isolate it either, because the JIT changes what the workload allocates -- a 200,000-iteration integer loop grows peak RSS by 5.8 MB with the JIT on and 23.8 MB with `PYRE_JIT=off`. What settles the question is soundness: `descr_from_set_member` is lookup-only by construction and `prepare_frozen_effect_info` degrades a whole EffectInfo to `RandomEffects` when any member is unresolvable, so a container published later cannot repair it. The dense-index halves of the lazy plan already ship. Assisted-by: Claude * jit: share frozen descriptor parent layouts * majit: record what reaches the heap no-effect exempt list Expand the doc comment on `OptHeap::has_no_heap_cache_effect`: - the list is consulted at two sites here against upstream's one, because `emitting_operation` is a whole-optimizer callback that fires after this pass has already dispatched; - the four entries with their own dispatch arm never reach the `handle_side_effects` test but still reach the `emitting_operation` one; - per-entry, why each of the other eleven cannot arrive today -- `DebugMergePoint` and `setinteriorfield_raw` have no producer, `jit_debug` is in the jitcode alphabet but has no caller, and six are minted inside the optimizer and routed in by `emit_for_force`; - `LeavePortalFrame` is not symmetric with `EnterPortalFrame`: its `popframe` producer is gated only on `jitdriver_sd`, which is stamped in production, so it is not recorded as dead; - nothing registers a `stroruni.*` oopspec by either the attribute route or `mark_oopspec`, so the string handlers that would exercise the list are unreachable. Assisted-by: Claude * bench: scope identity_dict_view_iteration_regression out of wasm The fixture gates on a time ratio and the wasm guest ships no `time` module, so it can only restate that gap there. `module_dict_view_iteration_regression` still walks the same dict-view path on wasm without a clock. Assisted-by: Claude * jit: narrow the caught-blackhole last_instr stamp to the -1 sentinel `record_caught_blackhole_traceback` stamped `last_instr` unconditionally. That field is read back under two conventions -- the executing coordinate this hook resolves, and the `pc - 1` a walk-end flush leaves for the frame to resume from -- so a level the exception merely passes through had its resume coordinate replaced. On `exception_reused_object_tb_not_doubled` one frame moved from 12 to 31 and the replay failed with `stack underflow during interpreter opcode`. Write only over the `-1` the frame constructor left, which is the case the stamp was added for; a frame that never ran an instruction has no coordinate to destroy. Rewrite the comment to say which convention is which. Assisted-by: Claude * interp: bind the class only for an exact classmethod at LOAD_METHOD `compute_load_method_bound` tested the descriptor with `is_classmethod`, a pointer-equal `ob_type` check that admits subclasses. A `classmethod` subclass overriding `__get__` is resolved through that override -- the attribute lookup has declined its unwrap fast path for one since #1377 -- so the attribute is the override's result, and the binder still prepended the class to it: class CM(classmethod): def __get__(self, obj, cls=None): return lambda x: ('override', x) class C: m = CM(lambda cls, x: ('plain', x)) C().m(1) # TypeError: takes 1 positional argument but 2 were given All three receiver arms did it -- instance, type, and builtin-storage payload -- and identically with the JIT off. `getattr(C(), 'm')(1)` was already correct, which is what isolates the binder from the lookup. A subclass now falls through to `method_descriptor_bound`, whose `d != attr` test already answers no-binding, so no new arm is needed. The staticmethod arm beside it needs no split: it answers PY_NULL either way. `wrapper_subclass_load_method_self.py` gates it. Its callable takes exactly one parameter, so a prepended class raises instead of being absorbed the way `wrapper_subclass_get_override`'s `*args` absorbs it; it fails on five of its seven loops before this change and its two `getattr` loops stay green across it. Assisted-by: Claude * jit: trim frozen descriptor rehydration * interp: delete the unused super_lookup_binding A repo-wide search finds only its definition; nothing calls it. It walked the MRO past the super type and then re-derived the binding by testing the descriptor's type: staticmethod to PY_NULL, classmethod to the class, a hardcoded `__new__` to PY_NULL, otherwise the instance. `W_Super.getattribute` (pypy/module/__builtin__/descriptor.py:63-83) has no such step. It walks the MRO through `lookup_starting_at` (pypy/objspace/std/typeobject.py:458-468) and invokes the descriptor's own `__get__`, leaving the binding to the descriptor protocol. `super_getattribute_wtf8`, the path that runs, already does that. Assisted-by: Claude * macros: lower a builtin's String return to the collectable constructor `wrap_value_expr` lowered a `String` return to `w_str_new` and a `Wtf8Buf` return to `w_str_from_wtf8`. Both allocate the value through `malloc_raw` (`pyre-object/src/lltype.rs`), a bare `Box::into_raw` that registers no owner. `w_str_new`'s own doc scopes that immortal default to the structural strings -- dict keys, code constants, names -- and directs "dynamic, short-lived strings that live only in GC-traced slots" to `w_str_new_managed`. A builtin's return value is the latter, so it now takes the `*_managed` twins. `impl PywrapKind for String` takes them too: the `Vec<T>` return arm lowers to `w_list_new(v.into_iter().map(PywrapKind::into_py))`, so a `Vec<String>` return reaches PywrapKind rather than the `"String"` arm. `impl PywrapKind for &str` keeps `w_str_new` -- a borrowed `&str` in those macros is a literal at the call site -- as do the `__doc__` and property-name lowerings, whose strings are held by off-GC structures. This makes the macro agree with `gateway.rs fsdecode_filename_bytes`, which has used `w_str_from_wtf8_managed` for the same category of value since #1081. NO RSS IMPROVEMENT IS CLAIMED OR MEASURED. The reproducer this change was written for -- 500,000 `os.getcwd()` calls -- does not reach it: `getcwd` is a raw closure, not a `#[pyre_function]`, and its result already came from `fsdecode_filename_bytes`, i.e. already from the managed allocator. That workload still grows peak RSS by 82.2 MB, so the managed constructor does not bound it. The live user of the arm this change touches is `_opcode.get_opname`. Assisted-by: Claude * Revert "macros: lower a builtin's String return to the collectable constructor" This reverts 549e988. Three reasons, none of which were known when it landed: 1. Its motivation was refuted by its own reproducer. `os.getcwd` is not a `#[pyre_function]` -- it is a raw closure under `make_builtin_function_with_arity` returning `gateway::fsdecode_filename_bytes`, which has used `w_str_from_wtf8_managed` since #1081. The 500,000-call workload was already on the managed allocator, so it never measured the immortal path the change was aimed at, and it still grows peak RSS by 82.2 MB after the change. 2. No benefit is measured. Nothing demonstrates the conversion improves anything. 3. It changes trace shape and that was never evaluated. `w_str_new` carries `#[majit_macros::dont_look_inside]`; `w_str_new_managed` does not. Routing the macro's lowering to the un-annotated twin means the JIT traces the construction instead of residualising it, at every converted site. The doc-conformance argument for the change still stands and is worth revisiting, but it needs the annotation question answered and a gate that reaches the code. Assisted-by: Claude * jit: shrink descriptor rehydration state * majit: give the jitcode test's BhFieldSpec builder the is_class_word field `BhFieldSpec` gained `is_class_word: Option<bool>`, and `test_bh_field` was not updated, so `cargo test` failed to build `majit-translate`'s lib test with E0063 on all three CI legs while `cargo build` and `pyre/check.py` passed. `None` is the value a spec built with no layout in reach already records (`bh_field_spec_from_parts`), and `same_descr_layout` does not read this field. Assisted-by: Claude * jit: skip the caught-blackhole last_instr stamp on the recording walk's own live frame `record_caught_blackhole_traceback` writes the raising coordinate into `PyFrame.last_instr`, which also serves as the resume coordinate (`next_instr` = `last_instr + 1`). The `last_instr < 0` test added in a34f19bab65 does not separate the two: `-1` is the coordinate that resumes at pc 0, and a walk that declines its end state hands its own live frame back holding exactly that. On `test.test_userstring` the stamp moved such a frame onto its CALL, so it re-entered one opcode later and popped an empty operand stack — `TypeError: stack underflow during interpreter opcode`. Add `pyre_jit_trace::trace::active_walk_live_frame()`, reading `live_vable_frame_addr()` off the `ACTIVE_SYM_EXC` the tracer already publishes, and skip the store when it names this frame. The levels the hook exists for — the inline-callee frames a walk seeds — take no per-opcode store and are never resumed, and read a different address. New parity fixture `jit_traceback_frame_clear_chain.py` walks a whole traceback chain calling `frame.clear()` from inside a loop-bearing callee; it fails with the same stack underflow before this change. Verified: `test.test_userstring` PASS 1 FAIL 0 via `cpython_tests/run.py`; the new fixture, `bench/synth/traceback_inlined_callee_lasti_regression.py` and `bench/synth/exception_reused_object_tb_not_doubled.py` all pass. Assisted-by: Claude
The Windows half of the filesystem and process surface: how a path is spelled,
how a failed call is reported, and — for a long list of calls that returned
Noneand did nothing — whether it ran at all.PEP 529 filesystem encoding (
cf12af0556)sys.getfilesystemencodeerrors()issurrogatepasson win32, so a name whoseUTF-16 holds a lone surrogate survives the round trip instead of being replaced.
fsencodeshort-circuits to the string's own WTF-8,fsencode_path_wrefusespath bytes that do not decode, and
fspath_buf/os_str_from_bytesbuild theOsStringthroughencode_wide.compile()and_symtable.symtabletake thechecked decode for their filename arguments.
OSError.winerror(cf12af0556,0621c3e94a)W_BaseExceptiongrows thew_winerrorslot, with the member descriptor, theWindows
__basicsize__of 120, and therwin32.py:285-306Win32-to-errno map.OSError(22, 'm', 'f', 5)is aPermissionErrorwitherrno == 13, andstr(e)opens with[WinError 5]— but only where there is a filename or astrerror to report it with, which is the pointer check
OSError_strmakes.Then the calls that actually fail:
PyError::os_error_win32_syscall2takes aGetLastErrorcode, keeps it inwinerror, takesstrerrorfromFormatMessageW(trailing newline and full stop stripped) and deriveserrno— and with it the subclass — from the map.
os.stat,lstat,listdir,scandir,mkdir,rmdir,remove,unlink,rename,replace,utime,readlink,DirEntry.stat/inode, thewin_nthelpers and_winapibuildtheir OSError through it. The descriptor calls do not: they go through the C
runtime, which reports an
errno, and so does CPython.The descriptor calls, the wide path calls, and
fstat(7dde41a2d5)os.close(999)aborted the process: the C runtime's invalid parameterhandler is a process abort, and the descriptor calls never silenced it. They now
do, for the duration of the call (
_Py_BEGIN_SUPPRESS_IPH), and read theverdict from
errnorather thanGetLastError— the latter describes whicheverWin32 call the runtime made last, not this one.
os.open/mkdir/rmdir/unlinktook the narrow entry points, whichre-encode the path through the ANSI code page.
os.mkdir(d)for a name outsideit failed with
[WinError 1113] No mapping for the Unicode character exists in the target multi-byte code page. They take_wopen/CreateDirectoryW/RemoveDirectoryW/DeleteFileWnow.os.fstat,os.dup,os.dup2,os.fsyncandos.ftruncatewere noopplaceholders on Windows — they returned
Noneand did nothing.fstatreadsthe descriptor's handle and reports
ERROR_INVALID_HANDLEfor a descriptorwithout one, the way
_Py_fstat_noraisedoes; the rest are the runtime's calls.stat_result.st_modefollowsattributes_to_mode: a directory carries theexecute bits, and only the read-only attribute drops the write ones (a directory
read
0o40755where CPython reads0o40777).The rest of the noop placeholders (
4cb73ba370)The same defect, swept:
truncate,chdir,access,chmod,fchmod,link,umask,pipe,getppid,getlogin,cpu_count,device_encoding,get_inheritableandset_inheritableall returnedNoneand did nothing on Windows, and
os.startfilewas absent. Each now calls theplatform, and each reports failure the way CPython's own arm does —
chdir,chmodandlinkthe Win32 way,fchmodthrough the handle'sERROR_INVALID_HANDLE,truncateand the descriptor calls with an errno.POSIX was not clean either: it had no
truncate,linkorget_inheritable,os.duphanded back an inheritable copy (PEP 446 says otherwise, and_Py_dupusesF_DUPFD_CLOEXEC),os.dup2ignored itsinheritableargument, and
os.utimeput the file name into the one path erroros_utime_impldeliberately raises without one.system,waitpid,times, the drive listings (51ad4d728b)os.systemandos.waitpidreturnedNoneon Windows — a command that neverran and a child that was never waited for, both reported as success. They call
_wsystemand_cwaitnow.os.times,os.listdrives,os.listvolumesandos.listmountsdid not exist;times_resultwas namedposix.times_resultona host whose module is
nt, which is the namepickleimports to resolve thetype (
posix_structseq_exports_python314.pycovers it, and only startedexercising it once
os.timesexisted).os.get_terminal_sizeignored itsfdargument and answered80x24for adescriptor that names no terminal. It reports the failure now, on both
platforms —
shutil.get_terminal_sizeis the documented place the fallbacklives, and it reaches it by catching exactly this.
subprocesson Windows (157711a0d6)import subprocessworked and a launch did not:subprocesspicks its Windowsimplementation on the presence of
msvcrt, then reaches for a_winapithathad the constants and the wait calls but none of the launch.
GetStdHandle,GetCurrentProcess,GetFileType,GetLastError,TerminateProcess,CreatePipe,DuplicateHandleandCreateProcessare here now, backed byrustpython_host_env::winapi.Two things that only show up once a real launch runs:
STARTUPINFO.lpAttributeListstarts out as{"handle_list": []}, and anattribute list carrying no handle makes
CreateProcessanswer[WinError 24] ERROR_BAD_LENGTH. An empty list is passed as no attribute listat all, which is what
getattributelistdoes with it.Popen(stdout=PIPE).stdout.read()hung._get_handlescloses the pipe endit just duplicated by dropping the last reference to its
Handlewrapper andletting
__del__run; here that waits on the collector, so the parent kept thewrite end of the pipe it was reading to end-of-file, and the read never
returned.
_make_inheritabletakes acloseflag and closes it — PyPy's ownpatch for the same reason.
dir_fdnames the argument, not the call (afa0a0a233)_DirFD_Unavailable(interp_posix.py:285-292) raisesdir_fd unavailable on this platform, and so doesdir_fd_unavailable—neither puts the call's name in front of it, and the message here did.
That surfaced through
os_stat_file_descriptor.py, which asserts that adescriptor and a
dir_fdtogether report the descriptor conflict. Where theplatform has no
fstatatthe argument is turned away while it is unwrapped,which is a step earlier than
do_stat, so the script failed under CPython onWindows too. It now asserts the answer each platform actually gives.
Verification
Every observable above was measured against CPython 3.14 on Windows first. A
17-probe error matrix (
stat/listdir/mkdir/rmdir/remove/rename/utime/readlink, each against a missing leaf, a missing parent, a wrong filetype and an existing target), a 10-probe bad-descriptor matrix and a non-ASCII
path round trip are all byte-identical to CPython, localized messages
included.
Four parity scripts carry the contract:
oserror_winerror_syscall.py(codesand the
[WinError N] <strerror>: 'path'shape, never the localized text, andthe errno spelling everywhere else),
os_non_ascii_path.py,os_call_effects.py— which asserts the state each call leaves behind, since acall that returns
Noneand does nothing passes every test that only checks itdid not raise — and
subprocess_launch.py, which walks a launch end to end:exit codes, each redirected stream,
stderr=STDOUT,communicate,env,cwd,shell, a non-ASCII argument and environment value,terminate/kill,timeout, a missing program, eight launches in a row, andos.popen.The POSIX arms were run, not just compiled: every one of these passes under a
Linux build of the same tree, which is what caught
os.dup's inheritable copyand
os.utime's filename. The full parity suite on Linux is 196/196dynasm=OK(the 58cpython=FAILthere are that box's CPython 3.12 againsta suite written for 3.14, and include this tree's own
os_stat_file_descriptor.py).check.pyon Windows: dynasm 388/388, cranelift 388/388, wasm 384/384,with no baseline re-recorded — the eight bytes
W_BaseExceptiongrows move nocounter this tree gates on. The parity suite on Windows is 191/191 across
CPython, dynasm and cranelift, which it has not been before.
Reaching that took one fix that is not Windows' at all, kept as its own commit:
dict_subscript_fold.py,exception_instance_dict_attr.pyandobject_init_text_signature.pyarrived without theprint("OK")run.pyreads, so all three failed the suite on every backend and every platform while
every assertion in them held.
One note for whoever reviews the jit-stats: a tree whose
build/llbcpredatesthe commits that last re-recorded a baseline will report that fixture as
changed, because those commits change the traced image. Re-extracting is what
makes them agree; it is not a property of this branch.
Not in scope
_sockethas no Windows implementation at all — every socket entry point is#[cfg(unix)], so there is no WSA error to map. That is a module port, not anerror-reporting fix, and it is the largest gap left.
os.get_blocking/set_blockingraiseNotImplementedErrorwhere CPythonanswers with the handle's own error — loud, so not in this sweep. Windows
stdout writes bare
\nwhere CPython's text-mode fd 1 writes\r\n, andencodes it as UTF-8 where CPython uses the ANSI code page.
Summary by CodeRabbit
New Features
OSError.winerrorvalues and improved error formatting.Bug Fixes
Tests