The host's own spelling of an argument: launcher argv, -W/-X option values, and os.stat's descriptor and dir_fd forms - #1066
Merged
Merged
Conversation
`pyre script.py $'bad\xff'` aborted the process. `std::env::args()` unwraps the UTF-8 conversion of every element, so the panic landed before parsing began and before anything could report it; CPython answers `sys.argv[1] == 'bad\udcff'` and exits 0. `targetpypystandalone.py:76-80` builds the list with `space.newfilename`, which is `fsdecode(newbytes(s))`, so an argument the filesystem encoding cannot spell arrives as the surrogate escape that re-encodes to the original byte. The arguments now stay in the host's own spelling until that decode: a Rust `String` cannot hold the escape, so narrowing anywhere earlier loses it. lexopt already hands out `OsString` — `RawArgs` yields it and `Arg::Value` carries it — so `drain_args` was doing the narrowing itself, and dropping `.string()?` there is what carries `sys.argv[1:]` through. The four run modes keep a `String` argv[0]: `-c` and stdin choose theirs from a literal, and a non-UTF-8 `-m` argument or script path is a separate case — those narrow at the parse, before `drain_args` is reached, and the payload flows on into the import machinery and the source reader rather than into a list. `gateway::fsdecode_os_str` is the decode, split the way the tree's other inbound OS-string boundaries are split — on `windows`, not on `unix`. Where the argument is bytes it takes the filesystem decode; where it is UTF-16 the host already has the code units and `Wtf8Buf::from_wide` carries them across, since routing those through the byte decode would turn an unpaired surrogate into three escapes and stop it round-tripping. `pyre-wasm-runner` had the same `std::env::args()` abort on its own positional script path. Its flags are ASCII by construction, so a value that does not convert is never one and takes the positional arm. `extra_tests/parity_tests/argv_undecodable_argument.py` passes the argument to a child, so it needs no such name on disk and runs wherever `execve` does; it self-skips on win32. Verified against CPython 3.14.5 first, then both backends. Not covered, measured and tracked separately. `-W` and `-X` option *values*: CPython reports `sys.warnoptions == ['ignore\udcff']` and exits 0 where this exits 2; those are inspected as text, folded with PYTHONWARNINGS by splitting on a comma, and carried across the wasm launch-env transport, so they move on their own. `-m` and the script path: CPython takes the argument and reports what it could not do with it — `No module named ba\udcffd` at exit 1, and `can't open file '…bad\udcff.py'` at exit 2 — where this rejects the argument itself. The script-path half cannot be exercised on APFS, which refuses such a name outright. Assisted-by: Claude
`FsEncodedPath` carries `Path.as_fd` (`interp_posix.py:140-152`), set only by
the entry points that pass `allow_fd`, and `fsencode_path_or_fd_w` names the
caller in the type error so the allowed-type list can widen with it: `stat`
answers "string, bytes, os.PathLike or integer" where `lstat` answers
"string, bytes or os.PathLike". The descriptor arm is probed with `__index__`
and sits before the PathLike arm, so an object carrying both is read as a
descriptor; `-1` is turned away as `unwrap_fd` does (:269-271).
`stat_entry` now has the three arms of `do_stat` (:633-649): a descriptor goes
to `fstat` (extracted from the `os.fstat` closure as `fstat_fd`), a
dir_fd-relative name to `fstatat` via `stat_at`, and a bare name to
`stat`/`lstat`. `dir_fd` is unwrapped with `_unwrap_dirfd`'s spelling
("integer or None"), and the two ValueErrors a descriptor triggers precede the
platform's dir_fd availability.
The `stat_result` assembly moves into `stat_result_from_fields`, taking the
fields as a `StatFields` so `std::fs::Metadata` and the raw `libc::stat` that
`fstatat` fills both reach it.
`_have_functions` gains HAVE_FSTATAT under the cfg where `stat_at` is
implemented. os.py:120-121 reads it as `stat` and `lstat` honouring dir_fd,
which `lstat` was already being advertised for through HAVE_LSTAT while
raising NotImplementedError.
`stat_impl` becomes `stat_path`, taking the already-unwrapped path: the
TypeError rewrite in its prologue reported `stat:` for both entry points and
is no longer needed now that the message is correct at its source.
Assisted-by: Claude
WalkthroughThe interpreter now preserves non-UTF-8 command-line arguments and supports file descriptors and ChangesFilesystem parity
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant OS as Operating system
participant Pyrex as pyrex launcher
participant Importing as importing argument state
participant PythonSys as Python sys.argv
OS->>Pyrex: provide OsString arguments
Pyrex->>Importing: pass host-native arguments
Importing->>PythonSys: decode filesystem values
PythonSys-->>Pyrex: expose sys.argv and sys.orig_argv
sequenceDiagram
participant PythonOS as Python os.stat
participant StatEntry as interp_posix stat_entry
participant Gateway as gateway path_or_fd conversion
participant Libc as Unix fstatat
PythonOS->>StatEntry: pass path or descriptor and dir_fd
StatEntry->>Gateway: convert path or descriptor
Gateway-->>StatEntry: return encoded path or as_fd
StatEntry->>Libc: call fstatat with symlink flags
Libc-->>StatEntry: return stat fields
StatEntry-->>PythonOS: return stat_result
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c6aeb72c08
ℹ️ 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".
| importing::init_sys_path(&script_dir, &script_dir.to_string_lossy()); | ||
| // sys.argv[0] is the script path; remaining values go to argv[1:]. | ||
| let mut argv = vec![path.clone()]; | ||
| let mut argv = vec![std::ffi::OsString::from(&path)]; |
There was a problem hiding this comment.
When the script filename itself contains non-UTF-8 bytes, this new OsString::from(&path) cannot preserve the host spelling because path has already been forced through script.string()? during argument parsing. That means a launch like pyre ./bad_\xff.py still exits with a non-unicode argument error before opening the script, instead of opening it and exposing sys.argv[0] with the same surrogateescape handling this commit adds for later arguments; keep the script path as an OsString/PathBuf until both opening and set_sys_argv.
AGENTS.md reference: AGENTS.md:L231-L233
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/pyre-interpreter/src/module/posix/interp_posix.rs`:
- Around line 2517-2554: Update the path conversion call in the stat/lstat
entry-point flow to pass an explicit allow_fd value tied to the entry point,
rather than reusing default_follow. Ensure stat continues accepting file
descriptors while lstat rejects them, independently of the follow_symlinks
default, and preserve the existing error behavior for each entry point.
- Around line 3036-3057: Update the fd validation guard in fstat_fd to reject
every negative descriptor, not just -1, before any unsafe File::from_raw_fd
call. Preserve the existing EBADF PyError construction and ensure valid
nonnegative descriptors continue through the sandbox and non-sandbox paths
unchanged.
- Around line 819-821: Update the `_have_functions` capability construction near
the `HAVE_FSTATAT` handling so `HAS_UNLINKAT` is not advertised solely because
`HAVE_FSTATAT` is present. Ensure the capability reflects implemented `unlinkat`
and `rmdirat` support, or split the `rmdir` backing capability so
`shutil.rmtree()` cannot select `_rmtree_safe_fd` while those operations remain
unimplemented.
🪄 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: 29c62c65-4d27-44c2-a16a-e8c2c7da6346
📒 Files selected for processing (9)
pyre/extra_tests/parity_tests/argv_undecodable_argument.pypyre/extra_tests/parity_tests/os_stat_file_descriptor.pypyre/pyre-interpreter/src/gateway.rspyre/pyre-interpreter/src/importing.rspyre/pyre-interpreter/src/module/posix/interp_posix.rspyre/pyre-interpreter/src/module/sys/vm.rspyre/pyre-jit/tests/gc_stress.rspyre/pyre-wasm-runner/src/main.rspyre/pyrex/src/lib.rs
Comment on lines
+819
to
+821
| // actually implemented may be listed: of the `*at` family only | ||
| // HAVE_FSTATAT is listed, because stat/lstat are the only calls that | ||
| // resolve a dir_fd-relative name, and HAVE_FDOPENDIR is omitted because |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the os.py capability wiring and every stdlib consumer of supports_dir_fd.
fd -t f 'os.py' -p 'lib-python' | xargs rg -n 'HAVE_FSTATAT|supports_dir_fd|_add\(' -C3
rg -n 'supports_dir_fd' --glob '*.py' -C4Repository: youknowone/pyre
Length of output: 6486
🏁 Script executed:
#!/bin/bash
set -eu
echo "== interp_posix fstatat context =="
sed -n '780,860p' pyre/pyre-interpreter/src/module/posix/interp_posix.rs | cat -n
echo
echo "== os fwalk implementation =="
sed -n '430,490p' lib-python/3/os.py | cat -n
echo
echo "== all supports_dir_fd uses =="
rg -n 'supports_dir_fd|fwalk\(|walk\(' --glob '*.py' lib-python/3 -C3
echo
echo "== imports/usages within lib-python/3 and module/posix =="
rg -n 'fwalk|supports_dir_fd|dir_fd|HAVE_FSTATAT|HAVE_FDOPENDIR' lib-python/3 pyre/pyre-interpreter/src/module/posix -C2Repository: youknowone/pyre
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -eu
echo "== shutil rmtree implementation guard =="
sed -n '760,830p' lib-python/3/shutil.py | cat -n
echo
echo "== os._have_functions generation =="
sed -n '84,108p' pyre/pyre-interpreter/src/module/posix/interp_posix.rs | cat -n
echo
echo "== exact HAVE_FSTATAT/stat/lstat mappings in lib-python/3/os.py =="
rg -n 'HAVE_FSTATAT|HAVE_LSTAT|supports_dir_fd|supports_follow_symlinks' lib-python/3/os.py -C2Repository: youknowone/pyre
Length of output: 6333
Avoid enabling _rmtree_safe_fd from HAVE_FSTATAT alone.
os._have_functions adds HAS_UNLINKAT to both unlink and rmdir, so advertising it can enable shutil.rmtree()’s fd-relative backend. If unlinkat/rmdirat remain unimplemented, keep HAS_UNLINKAT out of _have_functions or split the rmdir backing capability.
🤖 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 819 -
821, Update the `_have_functions` capability construction near the
`HAVE_FSTATAT` handling so `HAS_UNLINKAT` is not advertised solely because
`HAVE_FSTATAT` is present. Ensure the capability reflects implemented `unlinkat`
and `rmdirat` support, or split the `rmdir` backing capability so
`shutil.rmtree()` cannot select `_rmtree_safe_fd` while those operations remain
unimplemented.
Comment on lines
+2517
to
2554
| // `stat`/`lstat` type `dir_fd` as `DirFD(rposix.HAVE_FSTATAT)` | ||
| // (`interp_posix.py:612,660`), whose `unwrap` is `_unwrap_dirfd` | ||
| // (:274-278). | ||
| let dir_fd = match crate::builtins::kwarg_get(kwargs, "dir_fd") | ||
| .filter(|&v| !unsafe { pyre_object::is_none(v) }) | ||
| { | ||
| return Err(crate::PyError::not_implemented(format!( | ||
| "{name}: dir_fd unavailable on this platform" | ||
| ))); | ||
| } | ||
| Some(v) => Some(unwrap_fd(v, "integer or None")?), | ||
| None => None, | ||
| }; | ||
| let follow_symlinks = match crate::builtins::kwarg_get(kwargs, "follow_symlinks") { | ||
| Some(v) => crate::baseobjspace::is_true(v)?, | ||
| None => default_follow, | ||
| }; | ||
| stat_impl(&[path], follow_symlinks) | ||
| // interp_posix.py:611,659 — `stat` takes `path_or_fd(allow_fd=True)` | ||
| // and `lstat` takes `allow_fd=False`, which is also what makes their | ||
| // type errors name different allowed types. | ||
| let path = crate::gateway::fsencode_path_or_fd_w(path, name, default_follow)?; | ||
| // interp_posix.py:634-644 `do_stat` tests the descriptor first: with one | ||
| // in hand neither other argument has anything to apply to, and both | ||
| // rejections precede the platform's dir_fd availability. | ||
| if path.as_fd != -1 { | ||
| if dir_fd.is_some() { | ||
| return Err(crate::PyError::value_error(format!( | ||
| "{name}: can't specify dir_fd without matching path" | ||
| ))); | ||
| } | ||
| if !follow_symlinks { | ||
| return Err(crate::PyError::value_error(format!( | ||
| "{name}: cannot use fd and follow_symlinks together" | ||
| ))); | ||
| } | ||
| return fstat_fd(path.as_fd); | ||
| } | ||
| match dir_fd { | ||
| Some(dir_fd) => stat_at(name, &path, dir_fd, follow_symlinks), | ||
| None => stat_path(&path, follow_symlinks), | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Bind allow_fd to the entry point, not to default_follow.
Line 2533 passes default_follow as the allow_fd argument. The two flags mean different things: default_follow is the default value of follow_symlinks, and allow_fd states whether the boundary accepts a descriptor. The values agree today only because stat has both properties and lstat has neither. A future entry point, or a change to either default, silently changes the accepted argument types.
♻️ Proposed clarification
- let path = crate::gateway::fsencode_path_or_fd_w(path, name, default_follow)?;
+ // `stat` takes `path_or_fd(allow_fd=True)` and `lstat` takes
+ // `allow_fd=False`; the flag is the entry point, not the
+ // `follow_symlinks` default it happens to coincide with here.
+ let allow_fd = default_follow;
+ let path = crate::gateway::fsencode_path_or_fd_w(path, name, allow_fd)?;🤖 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 2517 -
2554, Update the path conversion call in the stat/lstat entry-point flow to pass
an explicit allow_fd value tied to the entry point, rather than reusing
default_follow. Ensure stat continues accepting file descriptors while lstat
rejects them, independently of the follow_symlinks default, and preserve the
existing error behavior for each entry point.
Comment on lines
+3036
to
+3057
| fn fstat_fd(fd: i32) -> Result<pyre_object::PyObjectRef, crate::PyError> { | ||
| // `rposix_stat.py:fstat` passes the descriptor to libc, where | ||
| // `-1` reports EBADF. Rust's `OwnedFd::from_raw_fd(-1)` | ||
| // asserts before `File::metadata` can produce that error. | ||
| if fd == -1 { | ||
| return Err(crate::PyError::os_error_with_errno( | ||
| libc::EBADF, | ||
| std::io::Error::from_raw_os_error(libc::EBADF).to_string(), | ||
| )); | ||
| } | ||
| #[cfg(feature = "sandbox")] | ||
| { | ||
| let buf = crate::host_seam::ops::fstat(fd) | ||
| .map_err(|e| crate::host_seam::seam_os_err(e, ""))?; | ||
| Ok(make_stat_result_from_statbuf(&buf)) | ||
| } | ||
| #[cfg(all(unix, not(feature = "sandbox")))] | ||
| { | ||
| use std::os::unix::io::FromRawFd; | ||
| let f = unsafe { std::fs::File::from_raw_fd(fd) }; | ||
| let meta = f.metadata(); | ||
| let _ = std::mem::ManuallyDrop::new(f); // don't close |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Reject every negative descriptor, not only -1.
The guard at Line 3040 covers -1 only. os.stat(-2) and os.fstat(-2) reach Line 3055 and pass a negative value to std::fs::File::from_raw_fd. That function requires an open descriptor owned by the caller, so a negative value violates its safety contract. OwnedFd::from_raw_fd asserts on -1 alone, so -2 is not caught. The syscall then returns EBADF, which is the correct observable result, so widen the guard to produce that result before the unsafe call.
🛡️ Proposed fix
- if fd == -1 {
+ // Any negative value names no open descriptor; libc reports EBADF for
+ // all of them, and `File::from_raw_fd` may not receive one.
+ if fd < 0 {
return Err(crate::PyError::os_error_with_errno(
libc::EBADF,
std::io::Error::from_raw_os_error(libc::EBADF).to_string(),
));
}📝 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.
Suggested change
| fn fstat_fd(fd: i32) -> Result<pyre_object::PyObjectRef, crate::PyError> { | |
| // `rposix_stat.py:fstat` passes the descriptor to libc, where | |
| // `-1` reports EBADF. Rust's `OwnedFd::from_raw_fd(-1)` | |
| // asserts before `File::metadata` can produce that error. | |
| if fd == -1 { | |
| return Err(crate::PyError::os_error_with_errno( | |
| libc::EBADF, | |
| std::io::Error::from_raw_os_error(libc::EBADF).to_string(), | |
| )); | |
| } | |
| #[cfg(feature = "sandbox")] | |
| { | |
| let buf = crate::host_seam::ops::fstat(fd) | |
| .map_err(|e| crate::host_seam::seam_os_err(e, ""))?; | |
| Ok(make_stat_result_from_statbuf(&buf)) | |
| } | |
| #[cfg(all(unix, not(feature = "sandbox")))] | |
| { | |
| use std::os::unix::io::FromRawFd; | |
| let f = unsafe { std::fs::File::from_raw_fd(fd) }; | |
| let meta = f.metadata(); | |
| let _ = std::mem::ManuallyDrop::new(f); // don't close | |
| fn fstat_fd(fd: i32) -> Result<pyre_object::PyObjectRef, crate::PyError> { | |
| // `rposix_stat.py:fstat` passes the descriptor to libc, where | |
| // `-1` reports EBADF. Rust's `OwnedFd::from_raw_fd(-1)` | |
| // asserts before `File::metadata` can produce that error. | |
| // Any negative value names no open descriptor; libc reports EBADF for | |
| // all of them, and `File::from_raw_fd` may not receive one. | |
| if fd < 0 { | |
| return Err(crate::PyError::os_error_with_errno( | |
| libc::EBADF, | |
| std::io::Error::from_raw_os_error(libc::EBADF).to_string(), | |
| )); | |
| } | |
| #[cfg(feature = "sandbox")] | |
| { | |
| let buf = crate::host_seam::ops::fstat(fd) | |
| .map_err(|e| crate::host_seam::seam_os_err(e, ""))?; | |
| Ok(make_stat_result_from_statbuf(&buf)) | |
| } | |
| #[cfg(all(unix, not(feature = "sandbox")))] | |
| { | |
| use std::os::unix::io::FromRawFd; | |
| let f = unsafe { std::fs::File::from_raw_fd(fd) }; | |
| let meta = f.metadata(); | |
| let _ = std::mem::ManuallyDrop::new(f); // don't close |
🤖 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 3036 -
3057, Update the fd validation guard in fstat_fd to reject every negative
descriptor, not just -1, before any unsafe File::from_raw_fd call. Preserve the
existing EBADF PyError construction and ensure valid nonnegative descriptors
continue through the sandbox and non-sandbox paths unchanged.
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit 218a5c4). Files in the reviewed diff1. Regressions to PyPy parity introduced by this patchNone. 2. Other mismatches introduced by this patch
3. Pre-existing mismatches (already present before this patch)
4. Structural adaptations
|
`gateway.py:705` applies the unwrap specs in the order the signature declares them, so `stat(path, *, dir_fd, follow_symlinks)` (`interp_posix.py:610-614`) resolves `path` first. `stat_entry` unwrapped `dir_fd` before converting `path`, which is observable both in which error answers when more than one argument is bad and in the order the arguments' user code runs — `__fspath__` for `path`, `__index__` for `dir_fd`, `__bool__` for `follow_symlinks`. Measured on 3.14: `os.stat(1.5, dir_fd=1.5)` reports the path type error, and `os.stat(PathLike, dir_fd=1.5)` calls `__fspath__` before rejecting `dir_fd`, with `follow_symlinks.__bool__` never read in either case. The parity test observes the call order, not only the message, so the arrangement cannot regress silently. The descriptor-plus-dir_fd message keeps the 3.14 wording and now cites the `interp_posix.py:639` spelling it differs from. Assisted-by: Claude
Owner
Author
|
Codex parity review dispositions, and one finding refuted. §2 — descriptor-plus- §2 — argument unwrap order · fixed (0ed154d). Real defect, thank you. Measured on 3.14: §2 — §3 — script path still narrowed to §4 — both adaptations confirmed as intended; each already cites its upstream decision point in-code. Unrelated to this diff, found while verifying it: — commented by Claude |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0ed154d933
ℹ️ 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".
| // `__index__` and sits BEFORE the PathLike case, so an object | ||
| // carrying both is taken as a descriptor and its `__fspath__` is | ||
| // never called. | ||
| let fd = crate::baseobjspace::c_int_w(obj)?; |
There was a problem hiding this comment.
When an os.stat path object defines an unusable __index__ but a valid __int__, this branch classifies it as fd-capable because the __index__ slot exists, then c_int_w goes through space_int, which tries __int__ before __index__; the call can therefore succeed as a descriptor even though the fd path converter is supposed to be based on the actual __index__ conversion. Use the __index__ result for this fd arm, with the same 32-bit range check, rather than converting with c_int_w.
Useful? React with 👍 / 👎.
`DirFD(available=False)` is `_DirFD_Unavailable` (`interp_posix.py:285-292`), whose `unwrap` turns a non-default `dir_fd` away before the call body runs. The availability test sat in `stat_at` instead, so on a target without `fstatat` `stat(fd, dir_fd=...)` reached `do_stat`'s descriptor conflict and answered ValueError where the argument itself is what is unsupported. `HAVE_FSTATAT` names the condition `_have_functions` already advertises on, and the message moves into `dir_fd_unavailable` so the unwrap and the (now unreachable, but still compiled) `stat_at` arm cannot drift apart. The descriptor probe's behaviour on an `__index__` that raises is recorded at the probe: `:202-207` swallows it with `except OperationError: pass` and falls through to `__fspath__`, while 3.14 propagates it. Measured — an object with a raising `__index__` and a working `__fspath__` reports that exception rather than being statted — and the parity test now pins both that and the `lstat` side, which takes no descriptor and so never probes `__index__` at all. Assisted-by: Claude
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pyre/pyre-interpreter/src/gateway.rs (1)
1665-1687: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winNegative descriptors below
-1reachFile::from_raw_fd. Both guards test the-1sentinel only, and treat that test as the complete descriptor validation.OwnedFd::from_raw_fdasserts on-1alone, so-2passes and breaks the safety contract ofFromRawFd, which requires an open descriptor owned by the caller.
pyre/pyre-interpreter/src/gateway.rs#L1665-L1687: rejectfd < 0in the descriptor branch ofpath_or_fd_w, and keep the existinginvalid file descriptor: -1message for the sentinel case.pyre/pyre-interpreter/src/module/posix/interp_posix.rs#L3064-L3073: change thefstat_fdguard fromfd == -1tofd < 0, soos.fstat(-2)reportsEBADFbefore the unsafe call.🤖 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/gateway.rs` around lines 1665 - 1687, Reject every negative descriptor before any unsafe ownership or file operation: in path_or_fd_w, change the descriptor validation to cover fd < 0 while preserving the existing invalid file descriptor: -1 message for the sentinel case; also update fstat_fd to guard fd < 0 so negative values such as -2 return EBADF before the unsafe call. Apply the changes at pyre/pyre-interpreter/src/gateway.rs lines 1665-1687 and pyre/pyre-interpreter/src/module/posix/interp_posix.rs lines 3064-3073.
🤖 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_stat_file_descriptor.py`:
- Around line 116-134: Add parity coverage for the negative descriptor value -2
alongside the existing descriptor cases, recording CPython 3.14 behavior for
both os.stat(-2) and os.fstat(-2). Ensure the test asserts the expected
exception/result for each call and specifically exercises the path_or_fd_w and
fstat_fd handling.
---
Outside diff comments:
In `@pyre/pyre-interpreter/src/gateway.rs`:
- Around line 1665-1687: Reject every negative descriptor before any unsafe
ownership or file operation: in path_or_fd_w, change the descriptor validation
to cover fd < 0 while preserving the existing invalid file descriptor: -1
message for the sentinel case; also update fstat_fd to guard fd < 0 so negative
values such as -2 return EBADF before the unsafe call. Apply the changes at
pyre/pyre-interpreter/src/gateway.rs lines 1665-1687 and
pyre/pyre-interpreter/src/module/posix/interp_posix.rs lines 3064-3073.
🪄 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: ca93ce1a-e495-4d27-a44b-2218bbb264d6
📒 Files selected for processing (3)
pyre/extra_tests/parity_tests/os_stat_file_descriptor.pypyre/pyre-interpreter/src/gateway.rspyre/pyre-interpreter/src/module/posix/interp_posix.rs
Comment on lines
+116
to
+134
| # The descriptor probe is `__index__`, and an object carrying both it and | ||
| # `__fspath__` is taken as a descriptor — so an `__index__` that raises | ||
| # reports its own exception instead of falling through to the path. | ||
| class BadIndex: | ||
| def __index__(self): | ||
| raise RuntimeError("boom") | ||
|
|
||
| def __fspath__(self): | ||
| return path | ||
|
|
||
| try: | ||
| os.stat(BadIndex()) | ||
| except RuntimeError as exc: | ||
| assert str(exc) == "boom", str(exc) | ||
| else: | ||
| raise AssertionError("stat fell through a raising __index__ to __fspath__") | ||
|
|
||
| # lstat takes no descriptor, so it never probes __index__ at all. | ||
| assert os.lstat(BadIndex()).st_size == 10 |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Add a negative-descriptor case to this suite.
The suite covers -1 through the descriptor path, but no case covers a descriptor below -1. The implementation guards -1 only, in both path_or_fd_w and fstat_fd, so os.stat(-2) and os.fstat(-2) currently reach File::from_raw_fd with a negative value. A parity case that records the CPython 3.14 result for os.stat(-2) and os.fstat(-2) would pin the intended behavior alongside the implementation fix.
I can generate that test case if you want it.
🧰 Tools
🪛 Ruff (0.16.1)
[warning] 120-120: Missing return type annotation for special method __index__
Add return type annotation: int
(ANN204)
[warning] 123-123: Missing return type annotation for special method __fspath__
(ANN204)
[warning] 129-129: Found assertion on exception exc in except block, use pytest.raises() instead
(PT017)
[warning] 129-129: Found assertion on exception exc in except block, use pytest.raises() instead
(PT017)
[warning] 131-131: 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_stat_file_descriptor.py` around lines 116 -
134, Add parity coverage for the negative descriptor value -2 alongside the
existing descriptor cases, recording CPython 3.14 behavior for both os.stat(-2)
and os.fstat(-2). Ensure the test asserts the expected exception/result for each
call and specifically exercises the path_or_fd_w and fstat_fd handling.
There was a problem hiding this comment.
💡 Codex Review
https://github.com/youknowone/pyre/blob/72e4cf0a0fc6182ac71035864ebdd568a38f57b3/pyre-interpreter/src/module/posix/interp_posix.rs#L2534
When dir_fd=-1 is supplied with an absolute path, this eager unwrap_fd rejects it before stat_at can apply the documented absolute-path behavior where the descriptor is ignored; on Unix/CPython, os.stat('/abs/path', dir_fd=-1) and os.lstat('/abs/path', dir_fd=-1) succeed just like any other ignored dir_fd. The same early rejection also prevents the descriptor-path branch from reporting the intended fd/dir_fd conflict. Convert the keyword without treating -1 as invalid here, and only reject it in the relative-path arm that actually uses the directory descriptor.
ℹ️ 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".
`app_main.py:785-786` splits an `-X` value on the first `=` and stores both
halves in `sys._xoptions` verbatim; `:892-906` appends the `-W` values and the
PYTHONWARNINGS pieces to `sys.warnoptions` verbatim. None of them is an
identifier, so none is required to have a UTF-8 spelling — `-W $'ignore\xff'`
reaches `sys.warnoptions` as `'ignore\udcff'` on 3.14, where pyre exited 2 out
of `lexopt`'s `.string()?`.
`LaunchFlags.warnoptions` / `xoptions` and the `importing` statics behind them
become `OsString`, and `sys` decodes them with `gateway::fsdecode_os_str` — the
boundary the launcher's argv already goes through. `-X` still matches the
options pyre acts on, through `to_str()`: every one of them is ASCII, so a
value with no UTF-8 form cannot be one.
`_xoptions` splits at the first `=` over the encoded bytes, which `OsStr`
documents as sound at an ASCII byte, and puts both halves through WTF-8:
`-X $'k\xff=v'` is `{'k\udcff': 'v'}`, so the key carries an escape as readily
as the value.
PYTHONWARNINGS moves from `read` to `read_raw`. `read` is
`String::from_utf8(..).ok()`, so one undecodable byte was discarding the whole
variable rather than the one comma-separated entry that carried it.
`fsdecode_os_str_wtf8` is `fsdecode_os_str`'s buffer, pairing with it the way
`fsdecode_filename_wtf8` pairs with `fsdecode_filename_bytes`, for the dict key
that has to be hashed rather than handed out.
The script path is a separate boundary and still narrows to `String`; its
`__file__` cannot round-trip until `co_filename` is WTF-8.
Assisted-by: Claude
youknowone
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 218a5c4e79
ℹ️ 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".
| // pyre acts on are matched, and every one of those is ASCII, | ||
| // so a value with no UTF-8 form simply cannot be one of them. | ||
| let option: std::ffi::OsString = parser.value()?; | ||
| match option.to_str() { |
There was a problem hiding this comment.
When a Unix -X value is not valid UTF-8 but its raw argv bytes are utf8=..., to_str() returns None, so this match falls through and records the option instead of calling fatal_utf8_config_error. For example, pyre -X $'utf8=\xff' -c pass should fail during preconfig like CPython/PyPy because argv decoding uses surrogateescape and the option still starts with utf8=, but this silently ignores the invalid utf8 setting. Check the raw/filesystem-decoded spelling for the utf8= prefix before deciding the option is unrelated.
AGENTS.md reference: AGENTS.md:L231-L232
Useful? React with 👍 / 👎.
| } | ||
| #[cfg(not(unix))] | ||
| { | ||
| std::ffi::OsString::from(String::from_utf8_lossy(value).into_owned()) |
There was a problem hiding this comment.
In the wasm-host path, the runner forwards launch environment values as raw bytes, so a Unix host can send a value like PYTHONWARNINGS=ignore\xff; however the guest is cfg(not(unix)), so this branch converts those bytes with from_utf8_lossy. That changes the undecodable byte into U+FFFD before sys.warnoptions is built, instead of the surrogateescape value that native pyre/CPython expose and round-trip, so warning filters and introspection diverge under the wasm runner. Keep the raw bytes through to the filesystem/surrogateescape decode instead of materializing a lossy OsString.
AGENTS.md reference: AGENTS.md:L231-L232
Useful? React with 👍 / 👎.
| "{name}: cannot use fd and follow_symlinks together" | ||
| ))); | ||
| } | ||
| return fstat_fd(path.as_fd); |
There was a problem hiding this comment.
Now that this branch makes os.stat accept integer descriptors, calls such as os.stat(99999999) flow into fstat_fd, which constructs std::fs::File::from_raw_fd(fd) before knowing whether the descriptor is valid or owned. CPython/PyPy report EBADF for that ordinary error case, but creating a File from an arbitrary Python integer violates the unsafe preconditions and also duplicates ownership for valid descriptors; use libc::fstat or another non-owning fd path so bad descriptors remain ordinary OSErrors.
AGENTS.md reference: AGENTS.md:L231-L232
Useful? React with 👍 / 👎.
youknowone
added a commit
that referenced
this pull request
Aug 6, 2026
`fstat_fd` served every non-Unix, non-sandbox build an unconditional EBADF, so `os.fstat(fd)` could not work on Windows — and #1066 routed `os.stat(fd)` through the same helper, extending the failure to a second entry point. There is no capability bit to withhold instead: `os.py:148` adds `stat` to `supports_fd` with no HAVE_* behind it ("fstat always works"), and `do_stat` (interp_posix.py:636) sends a descriptor to `rposix_stat.fstat` on every platform. The CRT descriptor is read through the handle it wraps — `nt::handle_from_fd`, already this file's idiom at :554 — and the `Metadata` that yields is the one `os.stat(path)` already reports there. `is_invalid_handle` is the same INVALID_HANDLE_VALUE test `_Py_fstat_noraise` makes before reporting EBADF. The parity script stops skipping win32 outright and asserts the one claim that holds on every platform: a descriptor reaches stat and fstat alike. Reported by the Codex parity review on #1066 (section 2). Assisted-by: Claude
youknowone
added a commit
that referenced
this pull request
Aug 6, 2026
`os.getcwd()` built its str with `to_string_lossy`, so a directory whose name carries a byte with no UTF-8 reading came back with U+FFFD in place of it and no longer named the directory it came from. interp_posix.py:906 is `space.fsdecode(getcwdb(space))`, whose surrogateescape round-trips. `gateway::fsdecode_os_str` and `fsdecode_filename_bytes` are the same decoders sys.argv goes through, so the two boundaries agree. Not exercised end to end here: APFS refuses to create a directory whose name is not valid UTF-8 (OSError errno 92), so the case needs a Linux host. Reported by the Codex parity review on #1066 (section 3). Assisted-by: Claude
youknowone
added a commit
that referenced
this pull request
Aug 6, 2026
`fstat_fd` served every non-Unix, non-sandbox build an unconditional EBADF, so `os.fstat(fd)` could not work on Windows — and #1066 routed `os.stat(fd)` through the same helper, extending the failure to a second entry point. There is no capability bit to withhold instead: `os.py:148` adds `stat` to `supports_fd` with no HAVE_* behind it ("fstat always works"), and `do_stat` (interp_posix.py:636) sends a descriptor to `rposix_stat.fstat` on every platform. The CRT descriptor is read through the handle it wraps — `nt::handle_from_fd`, already this file's idiom at :554 — and the `Metadata` that yields is the one `os.stat(path)` already reports there. `is_invalid_handle` is the same INVALID_HANDLE_VALUE test `_Py_fstat_noraise` makes before reporting EBADF. The parity script stops skipping win32 outright and asserts the one claim that holds on every platform: a descriptor reaches stat and fstat alike. Reported by the Codex parity review on #1066 (section 2). Assisted-by: Claude
youknowone
added a commit
that referenced
this pull request
Aug 6, 2026
`os.getcwd()` built its str with `to_string_lossy`, so a directory whose name carries a byte with no UTF-8 reading came back with U+FFFD in place of it and no longer named the directory it came from. interp_posix.py:906 is `space.fsdecode(getcwdb(space))`, whose surrogateescape round-trips. `gateway::fsdecode_os_str` and `fsdecode_filename_bytes` are the same decoders sys.argv goes through, so the two boundaries agree. Not exercised end to end here: APFS refuses to create a directory whose name is not valid UTF-8 (OSError errno 92), so the case needs a Linux host. Reported by the Codex parity review on #1066 (section 3). Assisted-by: Claude
youknowone
added a commit
that referenced
this pull request
Aug 6, 2026
`os.getcwd()` built its str with `to_string_lossy`, so a directory whose name carries a byte with no UTF-8 reading came back with U+FFFD in place of it and no longer named the directory it came from. interp_posix.py:906 is `space.fsdecode(getcwdb(space))`, whose surrogateescape round-trips. `gateway::fsdecode_os_str` and `fsdecode_filename_bytes` are the same decoders sys.argv goes through, so the two boundaries agree. Not exercised end to end here: APFS refuses to create a directory whose name is not valid UTF-8 (OSError errno 92), so the case needs a Linux host. Reported by the Codex parity review on #1066 (section 3). Assisted-by: Claude
youknowone
added a commit
that referenced
this pull request
Aug 6, 2026
`os.getcwd()` built its str with `to_string_lossy`, so a directory whose name carries a byte with no UTF-8 reading came back with U+FFFD in place of it and no longer named the directory it came from. interp_posix.py:906 is `space.fsdecode(getcwdb(space))`, whose surrogateescape round-trips. `gateway::fsdecode_os_str` and `fsdecode_filename_bytes` are the same decoders sys.argv goes through, so the two boundaries agree. Not exercised end to end here: APFS refuses to create a directory whose name is not valid UTF-8 (OSError errno 92), so the case needs a Linux host. Reported by the Codex parity review on #1066 (section 3). Assisted-by: Claude
youknowone
added a commit
that referenced
this pull request
Aug 7, 2026
`os.getcwd()` built its str with `to_string_lossy`, so a directory whose name carries a byte with no UTF-8 reading came back with U+FFFD in place of it and no longer named the directory it came from. interp_posix.py:906 is `space.fsdecode(getcwdb(space))`, whose surrogateescape round-trips. `gateway::fsdecode_os_str` and `fsdecode_filename_bytes` are the same decoders sys.argv goes through, so the two boundaries agree. Not exercised end to end here: APFS refuses to create a directory whose name is not valid UTF-8 (OSError errno 92), so the case needs a Linux host. Reported by the Codex parity review on #1066 (section 3). Assisted-by: Claude
youknowone
added a commit
that referenced
this pull request
Aug 7, 2026
… behind (#1078) * posix: take an open file descriptor in the supports_fd entry points that rejected one os.chdir, os.chmod, os.chown, os.pathconf, os.statvfs and os.utime each appeared in os.supports_fd and answered a descriptor with TypeError: expected str, bytes or os.PathLike object, not int They now unwrap through `gateway::fsencode_path_or_fd_w` and branch on `Path.as_fd` where `interp_posix.py` does: chdir to fchdir (:910-918), chmod to fchmod (:1228-1243), chown to fchown (:2475-2500), pathconf to fpathconf (:2420-2433), statvfs to fstatvfs (:704-719) and utime to futimens (:1860-1904). lchown keeps `allow_fd=0` and so keeps the narrower allowed-type list in its own type error. chown and utime reject follow_symlinks=False beside a descriptor, and utime rejects dir_fd, with the ValueErrors 3.14 words; `interp_posix.py` spells the first of them "cannnot" and words utime's dir_fd conflict "can't specify both dir_fd and fd", where 3.14 says "can't specify dir_fd without matching path". The parity suite reads CPython as the oracle. _have_functions is now built from the same constants the entry points branch on, so the advertisement cannot drift from the behaviour: the family drops out under sandbox and on the hosts carrying no host_env::posix, rather than each being spelled twice. HAVE_FUTIMES leaves the list because nothing here calls futimes, and os.py:150-151 reads either bit as the same utime capability. Assisted-by: Claude * posix: implement os.truncate os.truncate was one of the noop placeholders registered so os.py's _exists probes find a name, so it returned None and left the file untouched — on a path and on a descriptor alike — while HAVE_FTRUNCATE advertised it in os.supports_fd. os.ftruncate did the work all along: with open(p, "wb") as f: f.write(b"0123456789") os.truncate(p, 4); os.stat(p).st_size # 10 os.ftruncate(fd, 1); os.stat(fd).st_size # 1 interp_posix.py:414-431 takes a descriptor as it stands and opens a name write-only, truncates whichever it ended up with, and closes only the one it opened. The EINTR retry loop and the length conversion move out of the os.ftruncate closure so both callers share them; the path form reports the name it opened, which the descriptor form has none of. Assisted-by: Claude * posix: decode getcwd with the filesystem handler, not lossily `os.getcwd()` built its str with `to_string_lossy`, so a directory whose name carries a byte with no UTF-8 reading came back with U+FFFD in place of it and no longer named the directory it came from. interp_posix.py:906 is `space.fsdecode(getcwdb(space))`, whose surrogateescape round-trips. `gateway::fsdecode_os_str` and `fsdecode_filename_bytes` are the same decoders sys.argv goes through, so the two boundaries agree. Not exercised end to end here: APFS refuses to create a directory whose name is not valid UTF-8 (OSError errno 92), so the case needs a Linux host. Reported by the Codex parity review on #1066 (section 3). Assisted-by: Claude * extra_tests: check every os.supports_fd member against a descriptor The set is what callers read to choose an fd-relative implementation over a path-based one, so a member that rejects an integer — or accepts one and does nothing — sends the caller down a route that cannot work. The script asserts the eight-name floor before the per-name blocks, so a capability that is dropped rather than fixed fails here instead of quietly shrinking the coverage of the guarded blocks below it. Each name is then called with a descriptor and the result observed rather than the return value trusted: chdir moves the process, chmod and utime are read back through stat, truncate shrinks the file. It also pins the two type errors path_or_fd emits — the caller-named list widens with the descriptor form, so lchown answers "string, bytes or os.PathLike" where chown answers "string, bytes, os.PathLike or integer" — the two ValueErrors a descriptor beside dir_fd or follow_symlinks raises, and that utime's times/ns conflict outranks both. listdir, scandir and execve are skipped: the first two take a nullable path and word their type error differently, and execve takes an argv. Assisted-by: Claude * posix: resolve chown's dir_fd, and advertise HAVE_FCHOWNAT / HAVE_UTIMENSAT chown already reached `fchownat`, but with `AT_FDCWD` hard-coded and the dir_fd keyword rejected with NotImplementedError one step earlier; the descriptor the caller names is now what the name resolves against. `_DirFD_Unavailable.unwrap` (interp_posix.py:285-292) converts before it reports the platform, so the value is unwrapped first and the availability bit consulted second. A descriptor path plus dir_fd is the ValueError interp_posix.py:2481-2483 raises, checked ahead of the follow_symlinks one it already had. Neither macro was in `_have_functions`, so os.py put chown and utime in neither supports_dir_fd (os.py:119,133) nor supports_follow_symlinks (os.py:180,191) while both entry points implemented the modifiers — utime through one `utimensat`, chown through one `fchownat`. HAVE_LCHOWN and HAVE_LUTIMES stay unlisted for the reason HAVE_FUTIMES already carried: os.py reads them as the same capability and neither `lchown` nor `lutimes` is called here. Two lib-python tests move from failing to passing against the same corpus otherwise unchanged (dynasm, 599 tests across the two modules, no other row moves): test_os.UtimeTests.test_utime_invalid_arguments — asserted NotImplementedError for `follow_symlinks=False` because utime was absent from supports_follow_symlinks, and pyre did not raise test_shutil.TestMisc.test_chown — shutil.chown(dir_fd=...) hit the NotImplementedError Assisted-by: Claude * extra_tests: exercise dir_fd and follow_symlinks on every name that claims them The companion to os_supports_fd.py for the other two capability sets. It asserts the chown/stat/utime floor in both supports_dir_fd and supports_follow_symlinks, then calls each with the modifier and reads the result back rather than trusting the return value. Every dir_fd call is repeated against a name that does not exist under the descriptor, which is what separates "resolved the name against dir_fd" from "ignored it and reached the same file through the cwd" — a fixture whose relative name also resolves from the process cwd cannot tell the two apart. follow_symlinks=False is read back through lstat with the target asserted unmoved, the two modifiers are then used together, and the three ValueErrors a descriptor path produces are pinned by message. The last block is what the second set buys a caller: shutil.copystat with follow_symlinks=False substitutes `_nop` (shutil.py:435-439) for any name the set does not carry, so before this the call silently copied nothing. Assisted-by: Claude * majit: fold a virtual's never-stored field read to the zero constant optimize_getfield_gc answered GETFIELD_GC on a virtual only when the trace had already stored that field; an unset field fell through to PassOn and the load was emitted. virtualize.py:184-193 substitutes optimizer.new_const(fielddescr) when opinfo.getfield returns None. Port that fallback. typeptr, w_class and the GETFIELD_RAW_* opcodes stay out of it: the first two are header fields the same function already resolves from class identity, and upstream defines this handler for GETFIELD_GC_{I,R,F} only. pytraceback.rs:462 reads an exception's traceback slot before writing it, so every raise emitted that load and the arg-forcing pass materialized the exception behind it. type_immutable_reject's compiled loop body goes from 62 ops with 8 allocations to 19 ops with none. wasm's exception_value_op_caught baseline drops to guard_failures=1, the value dynasm and cranelift already record. Assisted-by: Claude * posix: derive HAVE_LSTAT from HAVE_FSTATAT and serve the three claims MS_WINDOWS makes `("HAVE_LSTAT", true)` was the one hardcoded entry in a table whose every other bit is the condition its entry point branches on. os.py reads that bit twice: :118 `_add("HAVE_LSTAT", "lstat")` builds supports_dir_fd, and :189 `_add("HAVE_LSTAT", "stat")` builds supports_follow_symlinks. On Windows HAVE_FSTATAT is false, so `os.lstat(name, dir_fd=fd)` raises while the set still listed it: os.supports_dir_fd # {<built-in function lstat>} on windows HAVE_LSTAT now follows HAVE_FSTATAT, which is the call both spellings resolve a dir_fd with. That drops os.stat from supports_follow_symlinks wherever the bit goes false, and os.py offers MS_WINDOWS (:192) as the other vehicle for that claim — which also claims chmod takes a descriptor (:143) and honours follow_symlinks (:184). So os.chmod on Windows grows the two forms the bit advertises. The descriptor form is `host_nt::fchmod` on the handle the CRT descriptor wraps, and dispatches with no follow_symlinks test, as interp_posix.py:1233-1241 does; follow_symlinks=False is `host_nt::win32_lchmod`, the name's own attributes rather than the file the link resolves to. dir_fd stays refused, and refuses the way `_DirFD_Unavailable.unwrap` (interp_posix.py:285-292) does — converting the value first, so a wrongly typed one is a TypeError. MS_WINDOWS is appended after the HAVE_* rows, the position interp_posix.py:2854-2855 gives it. os_supports_fd.py's win32 branch was asserting os.stat alone and exiting; it now exercises every name the platform advertises — the round-trip through a descriptor for chmod and truncate, follow_symlinks read back through the attribute bit, the dir_fd rejection, and the widened allowed-type message — and fails on any advertised name it has no probe for. Assisted-by: Claude * posix: listdir and scandir accept a directory descriptor fdlistdir reads the names a descriptor holds: fdopendir on an F_DUPFD_CLOEXEC duplicate, a readdir loop that clears errno per call, then rewinddir before closedir so the caller's descriptor keeps its offset. listdir returns those names as str. scandir builds its entries from the same call when it is given a descriptor. Each entry's path is the bare name and carries the descriptor, and stat, inode, is_dir, is_file and is_symlink resolve that name with fstatat against it; a failed stat names the entry. The three is_* predicates now share one dir_entry_kind that reads S_IFMT. gateway gains fsencode_path_or_fd_nullable_w, the nullable half of _unwrap_path: None resolves to "." in the unwrapper rather than at each boundary, and the allowed-type list becomes the four-way matrix, which the DeprecationWarning now words the same way as the TypeError. _have_functions lists HAVE_FDOPENDIR, in the position the upstream name list gives it. The constant is HOST_POSIX, the condition fdlistdir compiles under. os_supports_fd.py covers both descriptor forms, the None forms, and the nullable allowed-type message. Assisted-by: Claude * posix: a buffer is not a path path_or_fd_w takes bytes and no longer any readable buffer. _unwrap_path's buffer arm (interp_posix.py:188-198) accepts one with a DeprecationWarning; 3.14 completed that deprecation, so a bytearray now gets the same TypeError every other rejected type gets. posix.fspath carried a second copy of that arm, which is what let os.fsencode and os.fsdecode hand a bytearray back unconverted. It also never checked what __fspath__ answered with; both are now the gateway's rules. The type in these messages is named by _PyType_Name — its own name without the module that qualifies it elsewhere, so array.array is reported as array. os_path_argument_types.py covers the rejection at every path boundary, the two shapes of the message, and that bytes, a bytes subclass and os.PathLike still work. Assisted-by: Claude * posix: chmod takes dir_fd and follow_symlinks, and lchmod is a real call The unix chmod was registered with a fixed arity of 2, so it took no keyword at all — spelling either modifier's own default was a TypeError. It now binds its arguments the way chown_entry does and dispatches like _chmod_path (interp_posix.py:1254-1258): fchmodat where a name has to be resolved against a directory descriptor or the final symlink must not be followed, plain chmod otherwise. A descriptor still answers before either modifier is consulted (interp_posix.py:1233-1242). chown turns both away in that case and chmod does not, so the two entry points differ here on purpose. os.lchmod stops being one of the no-op stubs at the top of the module and becomes chmod's follow_symlinks=False arm, registered only on the hosts that carry a working lchmod. _have_functions gains HAVE_FCHMODAT, which os.py:118 reads as chmod honouring dir_fd, and HAVE_LCHMOD, which os.py:183 reads as chmod honouring follow_symlinks. The second is the narrower bit for the reason os.py:159-177 gives: fchmodat can be present and still not honour AT_SYMLINK_NOFOLLOW. os_chmod_modifiers.py exercises each claim rather than asserting it — the dir_fd arm resolves a name that does not exist in the working directory, and the follow_symlinks arm reads the link's mode and the target's separately. Assisted-by: Claude * posix: a stat that fails for a reason other than ENOENT is the caller's to see DirEntry's is_dir/is_file/is_symlink answered False for every stat failure. check_mode (interp_scandir.py:319-330) answers "not this type" for ENOENT alone — a vanished entry is better reported as not being of the asked-for kind than as an error — and propagates the rest, named by the entry. Both arms of dir_entry_kind now do that. chmod reads ENOTSUP and EOPNOTSUPP on the follow_symlinks=False path as the modifier being unavailable rather than as an OS error (interp_posix.py:1247-1251): a host can accept AT_SYMLINK_NOFOLLOW and not implement it, which is the same fact that makes HAVE_LCHMOD a narrower bit than HAVE_FCHMODAT. listdir and scandir given a descriptor report it as the failure's filename, since it is what named the directory. Found by the Codex parity review of this branch. Assisted-by: Claude * posix: open, mkdir, mkfifo, rmdir and unlink take dir_fd None of the five resolved a name against a directory descriptor, and none accepted the keyword at all: `open`, `mkdir` and `mkfifo` read their arguments positionally, so a call carrying any keyword reached them as a trailing dict and failed converting it to an integer; `rmdir`, `unlink` and `remove` were registered with a declared arity, which turns every keyword away. Each now binds its positional-or-keyword prefix through `bind_path_args` and reads `*, dir_fd=None` through `dir_fd_kwarg`, which spells `DirFD(available)` (`interp_posix.py:274-292`): `None` and an absent argument are the same default, and the value is converted before the platform is reported. The name form dispatches to `openat`, `mkdirat`, `mkfifoat` and `unlinkat` — the last with `AT_REMOVEDIR` for `rmdir` (`rposix.py:2717-2720`). `_have_functions` gains HAVE_MKDIRAT, HAVE_MKFIFOAT, HAVE_OPENAT and HAVE_UNLINKAT, which `os.py:124-132` reads into `supports_dir_fd`. HAVE_MKNODAT is not among them: `mknod` is still a placeholder that creates nothing. `chmod` and `chown` carried the same `dir_fd` block written out twice; both now call `dir_fd_kwarg`. The five path boundaries also name themselves when the argument is not a path — `unlink: path should be string, bytes or os.PathLike, not int`. extra_tests/parity_tests/os_dir_fd_modifiers.py exercises the modifier resolving against a descriptor rather than the working directory, the advertisement matching the behaviour, and the argument-list messages. Assisted-by: Claude * posix: chflags, lchflags and mknod are real calls, or absent The three sat in the block of noop stubs that expects a real implementation further down and had none, so each took any argument, made no syscall and reported success. Their callers probe for presence and believe the answer: `shutil.copystat` (`shutil.py:467`) reaches chflags through `lookup("chflags")`, `tempfile._resetperms` through a `try: _os.chflags`, and `tarfile.makedev` through `hasattr(os, "mknod")` — so the flags were never copied and no fallback ran. The stub list is also not `#[cfg]`-gated, so `hasattr(os, 'chflags')` was true on Linux, where the interface does not exist. `mknod(path, mode=0o600, device=0, *, dir_fd=None)` is now `mknod`/`mknodat`, registered beside `mkfifo` in the POSIX `host_env` block. `chflags(path, flags, follow_symlinks=True)` and `lchflags(path, flags)` are one call whose `follow_symlinks=False` arm is the second name, registered only on the BSD-flavoured hosts that carry the pair. `<sys/stat.h>` declares `lchflags` on the Apple targets, where `libc` carries only `chflags` and `fchflags`, so it is named in an `extern` block. The three names are dropped from the noop list, so a host without the call no longer answers `hasattr`. `os.py:112-114` guards `_add` with `fn in _globals`, and all three consumers above probe before calling. `_have_functions` gains HAVE_LCHFLAGS (`os.py:182` → chflags in supports_follow_symlinks) and HAVE_MKNODAT (`os.py:126` → mknod in supports_dir_fd). `bind_path_args` takes the keyword-only names rather than assuming `dir_fd`, because a signature without a keyword-only tail counts a surplus argument differently: every argument counts against the one limit and it is always "at most", which is why `os.lchflags(p, 0, follow_symlinks=False)` is a count error and not an unknown keyword. HAVE_LCHMOD was spelled without the `host_env` half of the condition its registration carries. Both it and the new HAVE_LCHFLAGS now read `HOST_POSIX && BSD_FLAVOURED`, which is the invariant the table's own comment states: each bit is the condition the entry point itself is compiled under. `mknod` reports the failing name in `filename`, following `interp_posix.py:1360-1372` `wrap_oserror2(space, e, w_path)`; CPython's `os_mknod_impl` uses the pathless `posix_error()` there, as it does for `mkfifo`, which pyre already named. Assisted-by: Claude * posix: major, minor and makedev compute a device number The three sat in the noop-stub block and answered `None`. `tarfile` reads a node's pair out of `st_rdev` to write a header (`tarfile.py:2275-2276`) and puts one back together to recreate the node (`:2735`), so the header field it wrote was not a number. They are `libc::major`/`minor`/`makedev` (`interp_posix.py:2551-2563`), which is the host's own encoding rather than arithmetic that can be spelled portably — macOS splits a `dev_t` 8/24 and Linux 12/20 with the minor bits in two pieces. The device argument is narrowed to `dev_t` rather than to a C int, because that type is wider than an int where the pair is, and a value that does not fit reports the overflow instead of wrapping. `libc` is a shim under `feature = "sandbox"` and carries no `dev_t`, so the names are absent there rather than answering with another host's arithmetic. The remaining 40 lying names in that stub list are measured and filed separately. Assisted-by: Claude * posix: stop binding spawnv, so os.py can define the spawn family `os.py:881` writes the spawn family in Python over fork+exec+waitpid, guarded by `if _exists("fork") and not _exists("spawnv") and _exists("execv")`. A `spawnv` bound here is therefore not a placeholder waiting to be overwritten further down — it is what stops that definition from ever running, and the noop stub won. `fork`, `execv`, `_exit`, `waitpid`, `WIFSTOPPED` and `waitstatus_to_exitcode` are all real, so dropping the four names is the whole implementation. The same block defines P_WAIT and P_NOWAIT, and the constant list above bound both to 0 — so the two modes were equal and `os.spawnv(os.P_NOWAIT, …)` waited instead of returning a pid. POSIX has no spawn call and no such constants; `<process.h>` does, so they are now bound on Windows only, with the values `_spawnv` reads. The sandbox build drops the four names too: the spawn family is app-level code in os.py rather than an external, and binding a name there would take P_WAIT and P_NOWAIT with it. `fork` is a raising stub in that build, so the definition os.py provides refuses at the fork. extra_tests/parity_tests/os_spawn_family.py spawns a child both ways and checks that P_WAIT hands back the exit code where P_NOWAIT hands back a pid — which is the assertion the two modes being equal fails. Assisted-by: Claude * posix: EX_*, ST_*, SCHED_* and RTLD_* carry the header's values The four families sat in a list that bound every name to 0, under a comment saying zero stubs were fine for os.py init. os.py names none of them, so the zero was serving nothing; the readers are the callers. `os._exit` takes an EX_* straight to the exit status, `statvfs(...).f_flag` is masked with the ST_* bits, and the RTLD_* set is handed back to `dlopen`, where a zero asks for `RTLD_LOCAL | RTLD_LAZY` whatever the caller named. Within each family every member also compared equal to every other. EX_* are `<sysexits.h>` literals: `libc` binds none of them, and the header is a verbatim descendant of the 4.3BSD one wherever it is carried, so the values are the same on every host that has it. The rest come from `libc`, except the Apple scheduling policies — `<pthread/pthread_impl.h>` declares those and the crate does not mirror them. They are bound on the POSIX builds, which is where the headers are; SCHED_BATCH and SCHED_IDLE narrow to Linux and RTLD_DEEPBIND to glibc, as `rposix.py:296-300` and `rdynload.py:50-82` read them. Before this they were bound unconditionally, so the Windows and wasm builds carried names whose header their host does not have. `host_seam::sys` names each one, because the sandbox build reaches `libc` through that facade rather than directly. WNOHANG, WCONTINUED, WUNTRACED and the PRIO_* trio stay in the zero list: the POSIX blocks further down overwrite those with the real values, and the comment above them now says so. extra_tests/parity_tests/os_constants.py checks each family for the shape it has to have — the members distinct, the flag members single bits — which is what binding a whole family to one value destroys. Assisted-by: Claude * posix: stop binding the C entry points and the names os.py writes itself Seventeen names in the noop-stub list answered `None` and were never given a body. They fall into four kinds, and none of the four is a name the module should carry. `fstatat`, `faccessat`, `futimens`, `futimes` and `fdopendir` are the C entry points the calls above them are served with — `openat` and its family are how `dir_fd` and a descriptor path are honoured, not calls of their own — and `setenv` is the C spelling of `putenv`. `moduledef.py` publishes none of them. A name bound for one is a capability a caller probes for and believes. `pipe2`, `dup3` and the four scheduling-policy calls are Linux's own additions, and were bound on every host. Nothing serves them on any host here, so they are dropped rather than kept as a stub that reports success; the Linux bodies — `sched_getparam` and `sched_setparam` need a `sched_param` type that does not exist here yet — are filed as their own task. `WEXITED`, `WNOWAIT` and `WSTOPPED` are `waitid`'s option flags, which are numbers rather than calls. They are bound with the other wait options, from libc. The fourth kind is the one `spawnv` was: names os.py writes in Python and lists in its own `__all__`. `popen` (os.py:1020-1067) and `get_exec_path` (os.py:649) were both in the stub list; `getenv` (os.py:818-825) had a real body here, and the SEEK_SET/SEEK_CUR/SEEK_END trio came off the constants table that os.py fixes at 0/1/2 itself (os.py:203-206). os.py's definitions win — unlike the spawn family, none of these four is guarded on the name being free — so what the bindings changed was `os.__all__`, where each arrived twice: once through `_get_exports_list` and once through os.py's own list. `os.__all__` held six duplicates against CPython's none; it now holds none. `popen` leaves the sandbox build's raising-stub list for the same reason the spawn family did: os.py builds it over `subprocess`, whose fork the stubs beside it already refuse. extra_tests/parity_tests/os_module_surface.py checks `os.__all__` for a name listed twice, which is what catches this whole class at once, and checks that each C entry point is absent and each option flag is a number. Assisted-by: Claude * posix: getpgrp, getpgid and ctermid call the host The three answered `None` from the stub list, which is a number a caller cannot tell from a group id and a name it cannot tell from a terminal. `getpgrp` (`interp_posix.py:2167-2172`) cannot fail and so is not checked; `getpgid` (`:2201-2210`) can be asked about a process that is not there and reports it. `ctermid` (`:2603-2608`) is read the way `rposix.py:1724-1728` reads it — the call is handed a null pointer and answers the static buffer it keeps — and the result is a filename, so it is decoded through `fsdecode_filename_bytes` rather than assumed to be text. `<stdio.h>` declares `ctermid` on every POSIX host, and the `libc` crate carries it for a handful, so the declaration is spelled out where the crate has none — the same shape `lchflags` already uses here. The sandbox build refuses all three instead: a process group and the controlling terminal's name are host facts, and the neighbouring reads (`getpid`, `getppid`, `ttyname`, `tcgetpgrp`) are refused there for the same reason. `confstr` and `confstr_names` stay stubs. They need the `_CS_*` table, which the `libc` crate carries one entry of per host while the module publishes 17 on Darwin and about fifty on glibc; spelling the rest out is what `pathconf_names` already does here, and it is filed as its own task rather than written from values this host cannot check. extra_tests/parity_tests/os_process_group.py checks the two group calls against each other and the terminal name for being a path, which is what a `None` fails. Assisted-by: Claude * check.py, extra_tests: keyword-only performance arguments and temp-directory cleanup `run_bench`'s `wasm_float_tol` and `min_pypy_ratio` are now keyword-only. Ruff reports FBT002 for the first; no call site passed either positionally. Ten parity scripts called `tempfile.mkdtemp` and never removed the result, so every run of the suite left a directory behind — three runners over 196 scripts. `atexit.register(shutil.rmtree, ...)` covers the early `raise SystemExit` on Windows and every assertion failure without wrapping each script in a `try`/`finally`. The #1078 review named the two in that diff; the other eight are the same line. os_supports_dir_fd.py's composed dir_fd + follow_symlinks call wrote `LINK_MTIME + 1`, one nanosecond past a whole second, against a file that states two lines above that every timestamp is a whole second so a coarse filesystem still reads back what was written. It also could not tell "the call did nothing" from "the value rounded down", because the step before it had already written `LINK_MTIME`. It writes a second whole-second pair instead. Assisted-by: Claude * posix: truncate's open and close, and utime's signed timestamps Four answers from the #1078 review that were the wrong value rather than the wrong call. `pathconf`'s fifth rides with the conf* tables, which it shares a helper with. `truncate` opened the name it was given with a bare `libc::open`. The name can be a FIFO with no reader, and the call waited there holding the interpreter, so no other thread could reach the other end; it also reported an interrupted open as `InterruptedError` where the `ftruncate` beside it retried. It now goes through `call_external_function` under the same retry loop, which is what `interp_posix.py:418` reaches by opening through the module's own `open`. The close is no longer discarded: `interp_posix.py:429-431` closes in a `finally`, through a `close` that raises, so a writeback error the close is first to see is the caller's. The truncation's own failure still wins when both fail. `truncate_length_w` narrowed to `off_t` with an `as` cast. A length wider than `off_t` became a different length rather than an error, and the file was truncated to that; `off_t::try_from` reports it with the message the helper already had for a too-wide value. `utime` carried both timestamps as `std::time::Duration`, which has no second below the epoch, so every pre-epoch time was refused with "timestamp out of range". `rposix.futimens` and `rposix.utimensat` (`rposix.py:2634-2671`) keep the seconds and the nanoseconds apart and signed; this does the same, with the floor-division `_PyTime_ObjectToTimespec` applies, so `ns=(-1, -1)` is `(-1, 999999999)` and reads back as `-1`. The name form now calls `utimensat` directly rather than `rustpython_host_env::posix::set_file_times_at`, whose signature cannot carry a negative second — which is also what `interp_posix.rs:3136` already said it did. The Windows host call still counts upwards from the epoch, and turns a pre-epoch time away rather than writing a different one. `times` is accepted as a keyword. `interp_posix.py:1862` puts `__kwonly__` after `w_times`, so it is the one argument here a caller may spell either way, and it was positional-only. extra_tests/parity_tests/os_utime_pathconf_truncate.py pins these against CPython 3.14, together with pathconf's answer. Assisted-by: Claude * posix: a descriptor of -1 does not become a BorrowedFd `BorrowedFd::borrow_raw` documents one value it may not be given: `-1`, which the standard library reserves as the niche that makes `Option<BorrowedFd>` cost nothing. Ten call sites built one straight out of an `fd` argument the caller supplied, and `os.fchmod(-1, 0o644)`, `os.chown(p, -1, -1, dir_fd=-1)` and `os.sendfile(-1, ...)` are all reachable from Python. `fd_borrow` answers those with the `EBADF` the syscall would have answered with, so the observable behaviour is unchanged and the one integer that may not become a handle no longer does. The sites reading a descriptor the module itself just produced — `dup2`'s result, and `path.as_fd` past its own `!= -1` guard — are left alone; those are not caller values. Reported in the #1078 review. Assisted-by: Claude * posix: confstr reads the host's string table, and pathconf answers -1 Both were noop stubs: `confstr_names` answered `None` rather than a dict, and `confstr` answered `None` whatever it was asked. `posixmodule.c posix_constants_confstr` and `rposix.py:2248-2300` name the same candidate set, every entry `#ifdef`-guarded, so a host publishes exactly the names its own `<unistd.h>` defines. `libc` carries `_CS_PATH` and nothing else, and the two numberings disagree from that first entry on — 1 on the Apple targets, 0 in glibc's `bits/confname.h`, whose enum also restarts twice, at 1000 and at 1100. Both tables are written out for that reason: 17 names on Darwin, 27 on glibc. The ten the candidate set carries for the System V hosts are defined by neither header, so neither table has them. The tables were derived by crossing `posix_constants_confstr` with each host's header rather than written from memory. The same derivation over the macOS SDK reproduces CPython 3.14's own `os.confstr_names` on this host entry for entry, which is what says the method is right. The glibc half comes from that project's `bits/confname.h`, and every value in it is confirmed a second time by `libc`: it carries `_CS_PATH` 0, `_CS_GNU_LIBC_VERSION` 2 and `_CS_GNU_LIBPTHREAD_VERSION` 3 outright, and it puts `_CS_POSIX_V6_ILP32_OFF32_CFLAGS` at 1116 — one past the end of the XBS5 run, which is what fixes that run at 1100..1115. No Linux compile was reached; this branch has none available. The call is `rposix.confstr` (`rposix.py:2129-2143`): ask for the length, fill a buffer of exactly that size. A zero length is either a name the host has no string for, which is `None`, or one it does not know, which is the errno it set — so errno is cleared before the question is put. The length counts the terminator and the string does not, so `len - 1` bytes are decoded, the way `os_confstr_impl` does; `rffi.charp2strn(buf, n)` keeps it. The value can be a search path, so it is decoded through the filesystem handler. `confname_arg` now takes the table to resolve against, and the dict-building it shares with `pathconf_names` is one function. `pathconf` and `fpathconf` come with it, because the fix lands in the lines that refactor touched. Both answered `None` where the host has no determinate limit; `interp_posix.py:2433` hands whatever `pathconf` returned to `space.newint`, so the answer is the number `-1`. `PC_ASYNC_IO` and `PC_SYMLINK_MAX` are the names that reach it on hosts that do not implement them, and `None` is neither that value nor a type a caller can compare against a limit. Reported in the #1078 review; pinned by extra_tests/parity_tests/os_utime_pathconf_truncate.py. The sandbox build refuses `confstr`: the answer is the host's own search path among other strings, and `pathconf`, `fpathconf` and `sysconf` are refused there beside it. extra_tests/parity_tests/os_confstr.py checks what holds on any host that has the call — the names resolve, the values are distinct, CS_PATH is a real search path, and an unknown name is refused. Assisted-by: Claude * posix: lockf, waitid and the sparse-file whence values None of the three existed. All are POSIX rather than Linux-only — the Apple targets carry every one of them — so they were absent on a host that has them. `lockf` is `interp_posix.py:3006-3012`: one call under the `eintr_retry` loop, put through the call gate because `F_LOCK` waits. Its four commands are published beside it. It answers `None`, which is what `os_lockf_impl` does; `interp_posix.py:3012` answers the `0` the call returns on success, and 3.14 — the oracle the parity suite reads — does not carry it. `waitid` is one `interp_posix.py:1722` names and does not have, so the shape is CPython 3.14's: a five-field `waitid_result` structseq, the three `P_*` id types and the six `CLD_*` codes. A zero `si_pid` is the "nothing to report" answer and is `None` rather than a result of zeroes. `WEXITED`, `WSTOPPED` and `WNOWAIT` were already published. `SEEK_HOLE` and `SEEK_DATA` are the two `whence` values beyond the three os.py fixes itself. The hosts whose headers define them are named rather than excluded, so a host left out is one short of a name rather than one carrying a wrong value. The sandbox build refuses `lockf` and `waitid`; their constants are numbers and stay. extra_tests/parity_tests/os_lockf_waitid_seek.py locks a region, walks a file with both new whence values, and reports a child with WNOWAIT before reaping it — which is the difference between waitid and waitpid. Assisted-by: Claude
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Three argument boundaries where the host's own spelling was being lost.
The launcher's argv
std::env::args()panics on an argument that is not valid Unicode, sopyre script.py 'bad<0xff>'aborted the process with exit 101 before theinterpreter started. CPython decodes the argument with surrogateescape and
hands it to the program.
The launcher now carries arguments as
OsStringend to end —drain_argscollects
parser.raw_args(), the fourargv[0]sites buildOsString, andsys.argv/sys.orig_argvare decoded at the boundary bygateway::fsdecode_os_str. On Windows that isWtf8Buf::from_wideoverencode_wide, which is lossless; elsewhere it is the existingfsdecode_filename_bytes. Only--inspect's interactive path still needs aString, and it narrows there withlexopt::Error::NonUnicodeValue.pyre-wasm-runnerhad the same panic on its ownargs()call and is closedthe same way.
Measured against CPython 3.14:
pyre script.py 'bad<0xff>'now reports['bad\udcff']and exits 0. A non-UTF-8-margument is a separate case —CPython accepts it and fails later with
No module named ba\udcffd(exit 1),where pyre still rejects the argument at exit 2; that half is not addressed
here.
-W,-Xand PYTHONWARNINGSThe same problem one layer up:
app_main.py:785-786splits an-Xvalue on thefirst
=and stores both halves insys._xoptionsverbatim, and:892-906appends the
-Wvalues and the PYTHONWARNINGS pieces tosys.warnoptionsverbatim. None of them is an identifier, so none is required to have a UTF-8
spelling —
-W $'ignore\xff'reachessys.warnoptionsas'ignore\udcff'on3.14, where pyre exited 2 out of
lexopt's.string()?.LaunchFlags.warnoptions/xoptionsand theimportingstatics behind thembecame
OsString, decoded insysby the samefsdecode_os_strthe argv abovegoes through.
-Xstill matches the options pyre acts on, throughto_str():every one of them is ASCII, so a value with no UTF-8 form cannot be one.
_xoptionssplits at the first=over the encoded bytes —OsStrdocumentsthat as sound at an ASCII byte — and puts both halves through WTF-8, because
-X $'k\xff=v'is{'k\udcff': 'v'}: the key carries an escape as readily asthe value.
PYTHONWARNINGS moved from
readtoread_raw.readisString::from_utf8(..).ok(), so one undecodable byte was discarding the wholevariable rather than the single comma-separated entry that carried it — a
silent drop this fixes on the way past.
The script path is a fourth boundary and is not fixed here: it still
narrows to
String. Its__file__/co_filenamecannot round-trip untilthose are WTF-8, and it cannot be tested on macOS at all — APFS refuses to
create a non-UTF-8 filename (
OSError: [Errno 92]). Tracked separately.os.stat's descriptor anddir_fdformsos.statadvertised itself inos.supports_fdwhile raisingTypeErroron adescriptor, and both
statandlstatrejecteddir_fdwithNotImplementedError—lstatwhile being listed inos.supports_dir_fdthrough
HAVE_LSTAT(os.py:121).FsEncodedPathnow carriesPath.as_fd(interp_posix.py:140-152), set onlyby the entry points that pass
allow_fd, andfsencode_path_or_fd_wnames thecaller in the type error so the allowed-type list can widen with it:
statanswers
"string, bytes, os.PathLike or integer"wherelstatanswers"string, bytes or os.PathLike". The descriptor arm is probed with__index__and sits before the PathLike arm, so an object carrying both is read as a
descriptor.
stat_entryis now the three arms ofdo_stat(interp_posix.py:633-649): adescriptor goes to
fstat, adir_fd-relative name tofstatat, and a barename to
stat/lstat. Thestat_resultassembly moves intostat_result_from_fieldssostd::fs::Metadataand the rawlibc::statthatfstatatfills both reach it._have_functionsgainsHAVE_FSTATATunder thecfg where that is implemented, which is what makes the
supports_dir_fdentryfor both calls true.
One message follows CPython rather than PyPy:
os.stat(fd, dir_fd=...)reports"stat: can't specify dir_fd without matching path"(3.14) whereinterp_posix.py:639says"can't specify both dir_fd and fd". The paritysuite's oracle is CPython.
Tests
pyre/extra_tests/parity_tests/argv_undecodable_argument.pypasses anundecodable argument to a child through
subprocess, so it needs no such fileon disk, and self-skips on win32.
pyre/extra_tests/parity_tests/os_stat_file_descriptor.pycovers thedescriptor form, both
ValueErrors, the two exactTypeErrorstrings, and —guarded on
os.stat in os.supports_dir_fd— thefstatatarm includingAT_SYMLINK_NOFOLLOWagreeing withlstat, an absolute name ignoring thedescriptor,
ENOENTreporting the name, andENOTDIRfor a non-directorydescriptor. It also pins the argument unwrap order by observation, not only
by message:
path, thendir_fd, thenfollow_symlinks, each of which can runuser code (
__fspath__,__index__,__bool__).pyre/extra_tests/parity_tests/option_value_undecodable.pycovers-W, thefour
-Xshapes (value half, key half, bare key, and a value carrying further=), and the comma-separated PYTHONWARNINGS case.All three scripts pass under CPython 3.14 first, and on both backends.
Known-red checks
pyre/check.pyis red on this PR, and every failure is inherited frommain,which is itself red on
pyre/check.py (ubuntu-24.04)and(windows-latest)atits current tip. There are no correctness failures on any of the three
platforms:
[pypy exec clamped to floor; ratio not a measurement]. The failing setdiffers from main's in both directions (6 here main lacks, 4 there this
lacks), which is the signature of noise rather than a regression; several are
minimum-ratio failures where pyre is faster than the floor.
pickle_ctor_args, reporting a jit-stats improvement(
guard_failures 436 -> 201). Byte-identical to main's failure on the samebench, same tally.
exception_escape_hot_callee_tb_node_once.wasm.jitstatsbaseline,which is deterministic and already recorded on another branch.
Tracked separately; none of it is reachable from this diff.
🤖 Generated with Claude Code