_io: port typed buffered and text streams - #735
Conversation
|
Warning Review limit reached
Next review available in: 40 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. 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 (33)
WalkthroughThe change adds PyPy-compatible buffered binary and text I/O implementations, rewrites file-opening composition, updates runtime registration, improves incremental codec, array, random, POSIX, JIT, dictionary, and set behavior, and expands compatibility and regression coverage. ChangesBuffered and text I/O
Runtime and collection compatibility
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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
https://github.com/youknowone/pyre/blob/5439b84d8d8ea706b97addf6a878bdcbc4198bfd/pyre-interpreter/src/module/_io/buffered_writer.rs#L443-L444
For a raw writer that reports seekable() == False and has pending buffered bytes, this path enters the lock and flushes those bytes before any seekability check; if raw.seek then fails, BufferedWriter.seek() has still committed data that should have remained buffered, and if a custom raw.seek returns a value the seek can even succeed despite seekable() being false. BufferedReader.seek() already performs the capability check first, and the PyPy port this mirrors does the same before the flush path, so move the seekability rejection ahead of this block.
AGENTS.md reference: AGENTS.md:L194-L195
https://github.com/youknowone/pyre/blob/5439b84d8d8ea706b97addf6a878bdcbc4198bfd/pyre-interpreter/src/module/_io/buffered.rs#L309-L310
When read(n) takes this direct readinto path for requests at least one buffer long, the freshly allocated bytearray is passed to call_method_result without being pinned. call_method_result performs getattr_str before call_function_impl_result roots the argument slice, so a Python raw object whose readinto lookup/bound-method creation allocates can trigger GC while this target is still only a raw local pointer; the other raw_read/readinto paths pin the temp before making the same call, and this path needs the same protection to avoid passing a stale or collected bytearray to the raw stream.
ℹ️ 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".
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit 33c4c95). 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)None. 4. Structural adaptations
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e28884c40a
ℹ️ 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 readable(&self) -> Result<PyObjectRef, crate::PyError> { | ||
| self.check_init()?; | ||
| super::call_method_result(self.w_raw, "readable", &[]) |
There was a problem hiding this comment.
When the wrapped raw stream is bidirectional, this forwards readable() and reports True even though BufferedWriter still does not implement any read path (reads remain inherited/unsupported). Code that selects a read branch based on writer.readable() will now try to read from an object that cannot read; PyPy/CPython keep BufferedWriter non-readable regardless of the raw stream, so this should not delegate to raw.readable().
AGENTS.md reference: AGENTS.md:L195-L195
Useful? React with 👍 / 👎.
| } else { | ||
| pos | ||
| }; | ||
| let result = this.raw_seek(adjusted, whence)?; |
There was a problem hiding this comment.
When a writable raw stream reports seekable() == False but still has a seek method, this path flushes and calls raw.seek anyway; PyPy/CPython raise UnsupportedOperation before invoking the raw seek. That lets pipe-like or explicitly non-seekable streams observe side effects or even succeed from BufferedWriter.seek() despite advertising that seeking is unsupported.
AGENTS.md reference: AGENTS.md:L195-L195
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: 2
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/module/_io/mod.rs (1)
113-128: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAlign
_checkClosedarity with the inherited_pyioimplementation.
_pyio.pystill definesIOBase._checkClosed(self, msg=None)and_BufferedIOBaseinherits it unchanged while calls in_pyio.pypass custom messages, e.g."peek of closed file". The no-argextra_testonly covers the exposed base helper and will catch the arity change, but this breaks the inherited interpreter-level_pyioAPI and subclass calls with a custom message.🤖 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/_io/mod.rs` around lines 113 - 128, Update iobase_check_closed to accept self plus an optional message argument, matching _pyio IOBase._checkClosed(self, msg=None) and allowing inherited subclass calls with custom messages. Keep the closed-file error behavior, using the supplied message when present and the existing default otherwise, while preserving rejection of additional arguments.
🤖 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/_random/mod.rs`:
- Around line 148-159: The W_Random implementation must root obj before
random.seed, since seed(None) may trigger collection, and reload the object from
its shadow-stack slot after seeding before returning. In
pyre/pyre-interpreter/src/module/_random/mod.rs lines 148-159, update the
allocation/rooting flow around W_Random::allocate_stable and random.seed; in
pyre/pyre-macros/src/lib.rs lines 1844-1849, root or snapshot every ABI argument
before `#unwrap_stmts` and use those rooted values for coercion and receiver
extraction, rather than reading raw args after potentially collecting calls.
- Around line 266-270: Update the nbytes calculation in the random
byte-generation flow to validate the computed byte count before converting it to
usize, rejecting values that exceed usize::MAX with
crate::PyError::memory_error(""). Preserve the existing try_reserve_exact
handling for representable counts so oversized requests cannot truncate or
trigger uncontrolled Vec growth.
---
Outside diff comments:
In `@pyre/pyre-interpreter/src/module/_io/mod.rs`:
- Around line 113-128: Update iobase_check_closed to accept self plus an
optional message argument, matching _pyio IOBase._checkClosed(self, msg=None)
and allowing inherited subclass calls with custom messages. Keep the closed-file
error behavior, using the supplied message when present and the existing default
otherwise, while preserving rejection of additional arguments.
🪄 Autofix (Beta)
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: d6f149f9-3905-48da-b931-6d6b5523f314
📒 Files selected for processing (19)
pyre/extra_tests/snippets/builtin_set.pypyre/extra_tests/snippets/stdlib_io.pypyre/extra_tests/snippets/stdlib_io_buffered.pypyre/extra_tests/snippets/stdlib_io_buffered_random.pypyre/extra_tests/snippets/stdlib_io_buffered_rwpair.pypyre/extra_tests/snippets/stdlib_io_buffered_writer.pypyre/extra_tests/snippets/stdlib_random.pypyre/pyre-interpreter/src/baseobjspace.rspyre/pyre-interpreter/src/lib.rspyre/pyre-interpreter/src/module/_io/buffered.rspyre/pyre-interpreter/src/module/_io/buffered_random.rspyre/pyre-interpreter/src/module/_io/buffered_rwpair.rspyre/pyre-interpreter/src/module/_io/buffered_writer.rspyre/pyre-interpreter/src/module/_io/mod.rspyre/pyre-interpreter/src/module/_random/mod.rspyre/pyre-jit/src/eval.rspyre/pyre-macros/src/lib.rspyre/pyre-object/src/pyobject.rspyre/pyre-object/src/setobject.rs
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: c4d44e03fc
ℹ️ 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 temp = pyre_object::bytearrayobject::w_bytearray_new(block); | ||
| let result = super::call_method_result(self.w_raw, "readinto", &[temp])?; |
There was a problem hiding this comment.
When read(n) takes this large direct-read path, the freshly allocated bytearray is passed to the raw stream's readinto without being pinned/reloaded, unlike the other readinto paths in this file. If a Python-level raw.readinto allocates or otherwise triggers GC, temp is only an unrooted Rust local before the later w_bytearray_data(temp) access, so the reader can copy from a stale/moved object or corrupt the returned bytes. Pin self and temp around the callback as the nearby raw_read and readinto_impl paths do.
Useful? React with 👍 / 👎.
youknowone
changed the title
There was a problem hiding this comment.
💡 Codex Review
https://github.com/youknowone/pyre/blob/b592e030797a1d0e6916b15a4f1731dcf657b1a4/pyre-interpreter/src/builtins.rs#L11386-L11390
When text wrapper construction fails here after the raw FileIO and buffered layer have already been created (for example open(path, newline='bad') raises from TextIOWrapper.__init__), builtin_open returns the error without closing the current stream. PyPy wraps this construction sequence in a try/except that closes w_result on any later failure; without that cleanup, repeated invalid opens can leave real file descriptors open until a future GC/finalizer runs.
AGENTS.md reference: AGENTS.md:L194-L195
https://github.com/youknowone/pyre/blob/b592e030797a1d0e6916b15a4f1731dcf657b1a4/pyre-interpreter/src/builtins.rs#L11230
Using index_int_w_preserve_negative for open(..., buffering=...) turns a negative bigint outside the machine range into i64::MIN, and the later buffering < 0 path silently replaces it with the default buffer size. PyPy's @unwrap_spec(buffering=int)/space.c_int_w and CPython both raise OverflowError for values like buffering=-(1 << 1000), so this accepts invalid arguments and opens the file instead of failing before any I/O object is built.
AGENTS.md reference: AGENTS.md:L194-L195
https://github.com/youknowone/pyre/blob/b592e030797a1d0e6916b15a4f1731dcf657b1a4/pyre-macros/src/lib.rs#L1844
For generated instance-method wrappers, placing the argument unwraps before the receiver preamble means Python-level conversions run even when the descriptor is called with the wrong self (for example an unbound _io.BufferedReader.seek(object(), evil_index) invokes evil_index.__index__() before rejecting object() as the receiver). CPython/PyPy reject the descriptor receiver first, so this introduces observable side effects or replacement exceptions on invalid method calls; keep the receiver type check before any argument coercion while avoiding a mutable borrow across those coercions.
AGENTS.md reference: AGENTS.md:L194-L195
ℹ️ 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".
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/_io/buffered_writer.rs`:
- Around line 436-461: Update BufferedWriter::seek to check the underlying raw
stream’s seekability before entering the lock’s flush and seek flow, matching
BufferedReader::seek. Call the existing raw seekable check on the writer’s raw
object and propagate its UnsupportedOperation result before
writer_flush_unlocked or raw_seek executes.
In `@pyre/pyre-interpreter/src/module/_io/textio.rs`:
- Around line 98-115: Update TextIOWrapper’s decode and write paths to honor the
validated w_newline setting: in decode, preserve newline sequences when
w_newline is empty and otherwise retain the existing universal translation; in
write, translate \n to the configured newline when w_newline is neither empty
nor "\n", while leaving output unchanged otherwise. Use the existing w_newline
state and the decode/write methods without altering unrelated encoding behavior.
🪄 Autofix (Beta)
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: 805ff24a-8bda-4232-8ee2-1c4b52ebc2c8
📒 Files selected for processing (22)
pyre/extra_tests/snippets/builtin_set.pypyre/extra_tests/snippets/stdlib_io.pypyre/extra_tests/snippets/stdlib_io_buffered.pypyre/extra_tests/snippets/stdlib_io_buffered_random.pypyre/extra_tests/snippets/stdlib_io_buffered_rwpair.pypyre/extra_tests/snippets/stdlib_io_buffered_writer.pypyre/extra_tests/snippets/stdlib_random.pypyre/pyre-interpreter/src/baseobjspace.rspyre/pyre-interpreter/src/builtins.rspyre/pyre-interpreter/src/lib.rspyre/pyre-interpreter/src/module/_io/buffered.rspyre/pyre-interpreter/src/module/_io/buffered_random.rspyre/pyre-interpreter/src/module/_io/buffered_rwpair.rspyre/pyre-interpreter/src/module/_io/buffered_writer.rspyre/pyre-interpreter/src/module/_io/mod.rspyre/pyre-interpreter/src/module/_io/textio.rspyre/pyre-interpreter/src/module/_random/mod.rspyre/pyre-interpreter/src/module/sys/vm.rspyre/pyre-jit/src/eval.rspyre/pyre-macros/src/lib.rspyre/pyre-object/src/pyobject.rspyre/pyre-object/src/setobject.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d33061ea9e
ℹ️ 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 tell(&self) -> Result<PyObjectRef, crate::PyError> { | ||
| self.check_closed()?; | ||
| self.call_buffer("tell", &[]) |
There was a problem hiding this comment.
When a caller reads only part of a TextIOWrapper, ensure_decoded() has already drained the underlying buffer into self.decoded; this tell() then returns the raw buffer's EOF position instead of a cookie reflecting decoded_pos. For example, after TextIOWrapper(BytesIO(b"abc")).read(1), CPython/PyPy report position 1 so seek(tell()) resumes at b, but this reports 3 and resumes at EOF.
AGENTS.md reference: AGENTS.md:L195-L195
Useful? React with 👍 / 👎.
|
|
||
| fn readlines( | ||
| &mut self, | ||
| #[default(pyre_object::w_none())] _w_hint: PyObjectRef, |
There was a problem hiding this comment.
When readlines() is called with a positive hint, this parameter is ignored and the loop reads through EOF, so callers that intentionally request a bounded batch get every remaining line in the first call. CPython/PyPy stop after the accumulated line lengths exceed the hint, leaving the rest of the stream for subsequent reads.
AGENTS.md reference: AGENTS.md:L195-L195
Useful? React with 👍 / 👎.
| let encoding = Self::checked_text0(encoding, "utf-8", "encoding")?; | ||
| let errors = Self::checked_text0(errors, "strict", "errors")?; | ||
| let _newline_value = Self::unwrap_newline(newline)?; | ||
| Self::validate_text_codec(&encoding)?; |
There was a problem hiding this comment.
When TextIOWrapper(..., encoding="locale") or open(..., encoding="locale") reaches this constructor, validate_text_codec() looks up the literal codec name locale, which is not a codec alias and raises LookupError; CPython 3.14 treats it as a locale-encoding sentinel, and this file already performs that normalization in reconfigure(), so the constructor needs the same handling before validation and storage.
Useful? React with 👍 / 👎.
992aeeb to
5ec62f9
Compare
There was a problem hiding this comment.
💡 Codex Review
https://github.com/youknowone/pyre/blob/5ec62f9bd51f86bb07893a6bc047fc348ff0415b/pyre-interpreter/src/builtins.rs#L11420
When TextIOWrapper construction fails here after the raw FileIO and buffered layer have already been opened (for example open(path, newline="bad") or an invalid codec), the ? returns without closing either intermediate stream. PyPy's open keeps the latest layer in w_result and closes it on the exception path, so this can leak file descriptors until GC on invalid text-wrapper options; wrap the post-raw construction in cleanup that closes the current layer on error.
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c9ae5b4bd1
ℹ️ 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".
| result: PyObjectRef, | ||
| length: usize, | ||
| ) -> Result<usize, crate::PyError> { | ||
| let size = match crate::baseobjspace::int_w(result) { |
There was a problem hiding this comment.
When a raw stream's readinto() returns an object that defines __int__ but not __index__, int_w accepts it as a byte count; CPython/PyPy reject that result and only accept the index protocol. That lets an invalid raw implementation advance the buffered state and return data that should have raised instead, and the same conversion rule is needed for these new raw callback counts rather than int_w.
AGENTS.md reference: AGENTS.md:L195-L195
Useful? React with 👍 / 👎.
| if !unsafe { pyre_object::bytesobject::is_bytes_like(data) } { | ||
| return Err(crate::PyError::type_error("read() should return bytes")); |
There was a problem hiding this comment.
When a custom raw stream implements read() and returns a bytearray, this accepts it because is_bytes_like includes bytearray, even though CPython/PyPy require raw read() to return an actual bytes object and raise TypeError otherwise. The buffered reader therefore silently consumes data from invalid raws instead of preserving the _io protocol error.
AGENTS.md reference: AGENTS.md:L195-L195
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
https://github.com/youknowone/pyre/blob/08cbf2567b1797fbe37e8984592885cc0a602e98/pyre-interpreter/src/builtins.rs#L11383-L11386
When open() has already constructed the raw FileIO and then hits a later error such as text mode with buffering=0 (or a failing text wrapper/mode assignment), this returns without closing the current layer. PyPy's _open keeps the current result and closes it in the exception path; without that cleanup, repeated failed opens can leak file descriptors until GC/finalization, and the caller has no returned object to close. Add cleanup before propagating post-raw errors.
AGENTS.md reference: AGENTS.md:L195-L195
https://github.com/youknowone/pyre/blob/08cbf2567b1797fbe37e8984592885cc0a602e98/pyre-interpreter/src/module/array/mod.rs#L782-L785
When count * itemsize fits in usize but exceeds i64 (for example array('h').fromfile(f, 2**63-1) on 64-bit), checked_mul succeeds and w_int_new(size as i64) wraps the read size negative. That calls f.read(-2) instead of raising MemoryError before any read as PyPy's ovfcheck(self.itemsize * n) does, so a normal file can be consumed and appended before the eventual EOF path. Check the product against i64::MAX before boxing it.
AGENTS.md reference: AGENTS.md:L195-L195
ℹ️ 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".
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pyre/pyre-jit-trace/src/jitcode_dispatch/residual_call.rs (1)
185-209: 🎯 Functional Correctness | 🔴 Critical | 🏗️ Heavy liftSample the escape opcode window before the write-status gate.
escape_stackonly opens the window for residuals with!writes_live_heap, whilefbw_bump_executed_effect()is called unconditionally for non-pure residuals that write live heap. If the first residual of a same-pc opcode writes live heap, it leavesESCAPE_OPCODE_WINDOWempty and bumps the executed-effect count; a later non-writing residual of that opcode then becomes the “first” window sample and can latch despite an earlier same-opcode effect. Update this as the true first residual dispatch for the opcode, independent of the latch predicate.🤖 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-jit-trace/src/jitcode_dispatch/residual_call.rs` around lines 185 - 209, Update the residual dispatch flow around escape_stack so escape_opcode_window_clean is evaluated for the first residual of each opcode before the writes_live_heap gate. Preserve the unconditional fbw_bump_executed_effect behavior for non-pure live-heap writes, and ensure later non-writing residuals cannot latch after an earlier same-opcode effect.
♻️ Duplicate comments (1)
pyre/pyre-interpreter/src/module/_random/mod.rs (1)
153-160: 🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy liftRoot raw managed pointers across collecting calls.
pyre/pyre-interpreter/src/module/_random/mod.rs#L153-L160: pinobjimmediately after allocation, then reload it afterrandom.seed(...); seeding can allocate or invoke arbitrary__hash__.pyre/pyre-macros/src/lib.rs#L1845-L1852: root and reload all incoming ABI arguments before keyword binding or typed coercions. Either path can invoke Python and move objects before later unwraps or receiver extraction readargs.🤖 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/_random/mod.rs` around lines 153 - 160, Root the newly allocated obj immediately in the _random constructor before calling random.seed, then reload the managed pointer after seeding before returning it. In pyre/pyre-macros/src/lib.rs around the ABI argument handling, root every incoming argument and reload the rooted values before keyword binding, typed coercions, later unwraps, or receiver extraction; apply the change to both affected sites.
🤖 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/bench/synth/foriter_inplace_immutable.py`:
- Line 5: Update the __init__ method signature to include the required None
return annotation, changing it to def __init__(self) -> None:.
In `@pyre/pyre-interpreter/src/baseobjspace.rs`:
- Around line 10376-10395: The negative-overflow sentinel in
index_int_w_preserve_negative must not be used by builtin_open for buffering.
Update builtin_open to convert buffering via space_index followed by int_w,
preserving OverflowError for values outside the i64 range while leaving the
helper unchanged for consumers that validate negative values themselves.
In `@pyre/pyre-interpreter/src/module/_codecs/mod.rs`:
- Line 805: Update the buffer acquisition in utf16_32_ex_decode_impl,
utf16_32_decode_impl, and utf8_decode_impl to report a decode-appropriate
TypeError instead of reusing file_write_buffer_bytes’ “write() expects str or
bytes” message. Preserve the existing byte-buffer behavior while ensuring
invalid decoder inputs produce context-specific decode error text.
- Around line 1924-1926: Update the affected UTF-8 decode entry points,
including the wrapper around utf8_decode_impl and the corresponding entries at
the referenced ranges, so their Python keyword-bound parameter is exposed as
_final or final rather than the Rust identifier final_. Preserve the internal
final_ variable if needed by explicitly mapping the exposed keyword to it, and
apply the same naming consistently unless the _final behavior is intentionally
documented.
In `@pyre/pyre-sandbox/src/seccomp.rs`:
- Around line 159-163: Update the seccomp filter construction around the
SYS_ioctl allowlist entry to validate the ioctl command argument and permit only
the required probe/size commands, specifically TCGETS and TIOCGWINSZ (plus any
commands explicitly documented by the existing comments). Reject all other ioctl
requests, including TIOCSTI, while preserving the existing descriptor
inheritance and marshalling behavior.
---
Outside diff comments:
In `@pyre/pyre-jit-trace/src/jitcode_dispatch/residual_call.rs`:
- Around line 185-209: Update the residual dispatch flow around escape_stack so
escape_opcode_window_clean is evaluated for the first residual of each opcode
before the writes_live_heap gate. Preserve the unconditional
fbw_bump_executed_effect behavior for non-pure live-heap writes, and ensure
later non-writing residuals cannot latch after an earlier same-opcode effect.
---
Duplicate comments:
In `@pyre/pyre-interpreter/src/module/_random/mod.rs`:
- Around line 153-160: Root the newly allocated obj immediately in the _random
constructor before calling random.seed, then reload the managed pointer after
seeding before returning it. In pyre/pyre-macros/src/lib.rs around the ABI
argument handling, root every incoming argument and reload the rooted values
before keyword binding, typed coercions, later unwraps, or receiver extraction;
apply the change to both affected sites.
🪄 Autofix (Beta)
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: 18341e65-09f2-4905-a02c-2e6f966a83df
📒 Files selected for processing (33)
pyre/bench/synth/dict_update_source_mutation.pypyre/bench/synth/foriter_inplace_immutable.pypyre/extra_tests/snippets/builtin_set.pypyre/extra_tests/snippets/stdlib_array.pypyre/extra_tests/snippets/stdlib_io.pypyre/extra_tests/snippets/stdlib_io_buffered.pypyre/extra_tests/snippets/stdlib_io_buffered_random.pypyre/extra_tests/snippets/stdlib_io_buffered_rwpair.pypyre/extra_tests/snippets/stdlib_io_buffered_writer.pypyre/extra_tests/snippets/stdlib_random.pypyre/pyre-interpreter/src/baseobjspace.rspyre/pyre-interpreter/src/builtins.rspyre/pyre-interpreter/src/lib.rspyre/pyre-interpreter/src/module/_codecs/mod.rspyre/pyre-interpreter/src/module/_io/buffered.rspyre/pyre-interpreter/src/module/_io/buffered_random.rspyre/pyre-interpreter/src/module/_io/buffered_rwpair.rspyre/pyre-interpreter/src/module/_io/buffered_writer.rspyre/pyre-interpreter/src/module/_io/mod.rspyre/pyre-interpreter/src/module/_io/textio.rspyre/pyre-interpreter/src/module/_random/mod.rspyre/pyre-interpreter/src/module/array/mod.rspyre/pyre-interpreter/src/module/posix/interp_posix.rspyre/pyre-interpreter/src/module/sys/vm.rspyre/pyre-interpreter/src/type_methods.rspyre/pyre-interpreter/src/typedef.rspyre/pyre-jit-trace/src/jitcode_dispatch/residual_call.rspyre/pyre-jit/src/eval.rspyre/pyre-macros/src/lib.rspyre/pyre-object/src/interp_array.rspyre/pyre-object/src/pyobject.rspyre/pyre-object/src/setobject.rspyre/pyre-sandbox/src/seccomp.rs
| /// Index-protocol conversion for an argument whose callee performs its own | ||
| /// negative-value check. A positive bigint outside the machine range raises | ||
| /// OverflowError; a negative overflow is represented by i64::MIN so the | ||
| /// callee can issue its specified ValueError instead. CPython 3.14's | ||
| /// `_random.Random.getrandbits` has exactly this ordering (`-1 << 1000` is a | ||
| /// domain error, while `1 << 1000` is a conversion overflow). | ||
| pub fn index_int_w_preserve_negative(obj: PyObjectRef) -> Result<i64, PyError> { | ||
| let w_index = space_index(obj)?; | ||
| match int_w(w_index) { | ||
| Ok(index) => Ok(index), | ||
| Err(error) if error.kind == PyErrorKind::OverflowError => { | ||
| let big = unsafe { crate::builtins::obj_to_bigint(w_index) }; | ||
| if big.sign() == malachite_bigint::Sign::Minus { | ||
| Ok(i64::MIN) | ||
| } else { | ||
| Err(error) | ||
| } | ||
| } | ||
| Err(error) => Err(error), | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not reuse the negative-overflow sentinel for open().
builtin_open passes buffering through this helper. A value such as -(1 << 1000) becomes i64::MIN, then silently selects default buffering (buffering < 0), whereas the unrepresentable argument must raise OverflowError. Keep this sentinel conversion scoped to consumers such as getrandbits that perform their own negative-domain validation; convert open() with space_index + int_w instead.
🤖 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/baseobjspace.rs` around lines 10376 - 10395, The
negative-overflow sentinel in index_int_w_preserve_negative must not be used by
builtin_open for buffering. Update builtin_open to convert buffering via
space_index followed by int_w, preserving OverflowError for values outside the
i64 range while leaving the helper unchanged for consumers that validate
negative values themselves.
| #[default(w_bool_from(false))] final_: PyObjectRef, | ||
| ) -> Result<PyObjectRef, crate::PyError> { | ||
| decode_with_name(obj, errors, "utf-8") | ||
| utf8_decode_impl(obj, errors, final_) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether this codebase's builtin-function macro strips leading/trailing
# underscores from parameter names when deriving the Python keyword name.
rg -n 'fn utf_8_decode' -A5 pyre/pyre-interpreter/src/module/_codecs/mod.rs
rg -n 'macro_rules!|proc_macro' pyre/pyre-macros/src/lib.rs | head -30Repository: youknowone/pyre
Length of output: 2051
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== codecs exports around renamed functions =="
rg -n 'utf_8_decode|utf_16_decode|utf_32_decode|#\[builtin|#[crate_export|decode_impl' pyre/pyre-interpreter/src/module/_codecs/mod.rs
echo "== pyre-macros expand_pyre_function relevant section =="
sed -n '60,195p' pyre/pyre-macros/src/lib.rs
echo "== pyre-macros inspect_arg / arg_default / python name handling =="
sed -n '340,390p' pyre/pyre-macros/src/lib.rs
rg -n 'strip|arg|kw|keyword|default|pat|arg_default|expand_pyre_function|generate' pyre/pyre-macros/src/lib.rs | head -80
echo "== changed lines context =="
sed -n '1900,2025p' pyre/pyre-interpreter/src/module/_codecs/mod.rsRepository: youknowone/pyre
Length of output: 403
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== codecs exports around renamed functions =="
rg -n 'utf_8_decode|utf_16_decode|utf_32_decode|#\[builtin|#\[crate_export|decode_impl' pyre/pyre-interpreter/src/module/_codecs/mod.rs
echo "== pyre-macros expand_pyre_function relevant section =="
sed -n '60,195p' pyre/pyre-macros/src/lib.rs
echo "== pyre-macros inspect_arg / arg_default / python name handling =="
sed -n '340,390p' pyre/pyre-macros/src/lib.rs
rg -n 'strip|arg|kw|keyword|default|pat|arg_default|expand_pyre_function|generate' pyre/pyre-macros/src/lib.rs | head -80
echo "== changed lines context =="
sed -n '1900,2025p' pyre/pyre-interpreter/src/module/_codecs/mod.rsRepository: youknowone/pyre
Length of output: 14645
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate _match_signature implementation =="
rg -n 'bind_builtin_kwargs|match_signature|_match_signature|SignatureBuilder|kwargname' pyre -S
echo "== inspect builtins/signature area =="
file="$(rg -l 'bind_builtin_kwargs|match_signature' pyre | head -1 || true)"
if [ -n "$file" ]; then
sed -n '1,180p' "$file"
fi
echo "== inspect pyre-macros param_name/unwrap_arg area =="
rg -n 'fn param_name|fn unwrap_arg|Pat::Ident|to_string' pyre/pyre-macros/src/lib.rs -A4 -B2
echo "== current codecs decorator/attr area =="
sed -n '1886,1930p' pyre/pyre-interpreter/src/module/_codecs/mod.rsRepository: youknowone/pyre
Length of output: 26841
Keep the _final Python keyword name on Python keyword-bound paths.
param_name() / SignatureBuilder use the Rust identifier verbatim, so utf_8_decode(..., final_=…) is the exposed keyword here, not PyPy/CPython final=…. If this is intended to match upstream keyword usage, keep the signature exposed as _final/final and have the wrapper body accept it internally as final_<|final if needed. Applies to the same decode entry points at 1937-1947, 1958-1960, 1971-1973, 1984-1994, 2005-2007, 2018-2020 unless matching _final intent is documented.
🤖 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/_codecs/mod.rs` around lines 1924 - 1926,
Update the affected UTF-8 decode entry points, including the wrapper around
utf8_decode_impl and the corresponding entries at the referenced ranges, so
their Python keyword-bound parameter is exposed as _final or final rather than
the Rust identifier final_. Preserve the internal final_ variable if needed by
explicitly mapping the exposed keyword to it, and apply the same naming
consistently unless the _final behavior is intentionally documented.
| // Python 3.14's standard-stream construction probes the inherited | ||
| // descriptors with isatty(), which glibc implements as ioctl(TCGETS). | ||
| // The sandbox cannot open host paths, so this is limited to descriptors | ||
| // inherited from the controller or returned by the marshalling seam. | ||
| libc::SYS_ioctl, |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | rg 'pyre/pyre-sandbox/src/seccomp\.rs|seccomp\.rs$' || true
echo "== seccomp outline/sections =="
if [ -f pyre/pyre-sandbox/src/seccomp.rs ]; then
wc -l pyre/pyre-sandbox/src/seccomp.rs
sed -n '130,185p' pyre/pyre-sandbox/src/seccomp.rs
sed -n '230,310p' pyre/pyre-sandbox/src/seccomp.rs
fi
echo "== search ioctls/allowed_syscalls =="
rg -n "allowed_syscalls|ioctl|TCGETS|TIOCGWINSZ|SYS_ioctl|TIOCSTI|SECCOMP_DATA|args\\[1\\]" pyre/pyre-sandbox/src/seccomp.rs pyre -g '*.rs' || true
echo "== git diff stat/name =="
git diff --stat || true
git diff -- pyre/pyre-sandbox/src/seccomp.rs | sed -n '1,220p' || trueRepository: youknowone/pyre
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== seccomp top/offsets and seccomp consts =="
sed -n '1,130p' pyre/pyre-sandbox/src/seccomp.rs
echo "== argument handling / file descriptor setup =="
rg -n "fd|stdin|stdout|stderr|set_inherited|install_runtime|allow|isatty|TIOCGWINSZ|ioctl|PR_SET_SECCOMP|PR_SET_NO_NEW_PRIVS" pyre/pyre-sandbox/src seccomp.rs pyre -g '*.rs' | head -200Repository: youknowone/pyre
Length of output: 24013
🌐 Web query:
TIOCSTI ioctl sandbox escape controlling terminal input queue documentation
💡 Result:
The TIOCSTI ioctl is a legacy terminal interface operation in Linux that allows a process to insert characters into the controlling terminal's input queue, effectively faking keyboard input as if a user had typed it [1][2][3]. Because this functionality operates on the controlling terminal (often accessed via /dev/tty), it has historically served as a mechanism for sandbox escapes and privilege escalation [4][5][6]. If a sandboxed process retains access to the parent's terminal device, it can use TIOCSTI to inject arbitrary commands (e.g., "id\n", "exit\n", or malicious scripts) into the parent shell's input buffer, causing the parent process to execute them with its higher privileges once the child process terminates or loses focus [7][8][6]. Key technical and security details: Mechanism: By calling ioctl(fd, TIOCSTI, &char), a process pushes a character into the input buffer of the terminal associated with the file descriptor 'fd' [1][2][9]. If that terminal is also used by a more privileged parent process (such as a root shell running 'su' or 'sudo' without proper isolation), the parent process reads this "faked" input as if it originated from the user, leading to unintended command execution [3][6]. Mitigation and Evolution: - Isolation (Pseudo-terminals): The primary defense against TIOCSTI-based attacks is to ensure that sandboxed or less-privileged processes do not share a controlling terminal with more privileged ones. Tools like sudo (via the use_pty flag) and su now support allocating a new pseudo-terminal (pty) to isolate the child process's I/O from the user's main terminal [3][6]. - Kernel Restrictions: Due to its history of security vulnerabilities (e.g., CVE-2017-5226, CVE-2016-7545), modern Linux kernels (starting from version 6.2) have introduced mechanisms to restrict or disable TIOCSTI [5][1]. - Sysctl Control: The kernel now includes a sysctl variable, dev.tty.legacy_tiocsti, which allows administrators to disable this functionality system-wide [5]. When set to false, the use of TIOCSTI may require the CAP_SYS_ADMIN capability, effectively mitigating the risk for unprivileged applications [5][1][10]. Because the vulnerability is inherent to how the kernel manages TTY input queues, it is widely considered a kernel-level issue rather than a bug in individual applications like sudo or su [11]. Using modern, isolated terminal sessions remains the most robust defense [3][6].
Citations:
- 1: https://man7.org/linux/man-pages/man2/tiocsti.2const.html
- 2: https://kristian.ronningen.no/linux/faking-input-with-ioctl-tiocsti/
- 3: https://www.errno.fr/TTYPushback.html
- 4: GHSA-7gfv-rvfx-h87x
- 5: https://lists.openwall.net/linux-kernel/2022/10/15/79
- 6: https://ruderich.org/simon/notes/su-sudo-from-root-tty-hijacking
- 7: CVE-2017-5226 -- bubblewrap escape via TIOCSTI ioctl containers/bubblewrap#142
- 8: https://www.spinics.net/lists/selinux/msg20107.html
- 9: https://stackoverflow.com/questions/29614264/unable-to-fake-terminal-input-with-termios-tiocsti
- 10: https://manpages.ubuntu.com/manpages/questing/en/man2/TIOCSTI.2const.html
- 11: https://jdebp.uk/FGA/TIOCSTI-is-a-kernel-problem.html
Restrict ioctl with a cmd-level seccomp filter.
The allowlist only permits SYS_ioctl by syscall number, so any reachable inherited fd/pipe fd could be used for arbitrary ioctl commands, including dangerous terminal commands like TIOCSTI. Restrict the BPF filter to the probe/size ioctl commands shown in the comments, or TCGETS/TIOCGWINSZ as the intended minimal set.
🤖 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-sandbox/src/seccomp.rs` around lines 159 - 163, Update the seccomp
filter construction around the SYS_ioctl allowlist entry to validate the ioctl
command argument and permit only the required probe/size commands, specifically
TCGETS and TIOCGWINSZ (plus any commands explicitly documented by the existing
comments). Reject all other ioctl requests, including TIOCSTI, while preserving
the existing descriptor inheritance and marshalling behavior.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5bbad2680a
ℹ️ 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 __new__(_cls: PyObjectRef, _args: &[PyObjectRef]) -> PyObjectRef { | ||
| W_BufferedReader::allocate_stable(W_BufferedReader::default()) |
There was a problem hiding this comment.
When a subclass of io.BufferedReader is instantiated, type.__call__ passes that subclass as cls, but this allocator ignores it and returns an exact BufferedReader. Because pyre only runs __init__ when the object returned by __new__ is an instance of the requested class, subclass constructors such as tarfile.ExFileObject.__init__ are skipped and type(ReaderSubclass(raw)) is not the subclass; validate/stamp cls as the other typed allocators do.
AGENTS.md reference: AGENTS.md:L195-L195
Useful? React with 👍 / 👎.
| if unsafe { pyre_object::is_none(result) } { | ||
| return Err(make_write_blocking_error(0)); | ||
| } | ||
| let written = crate::baseobjspace::int_w(result)?; |
There was a problem hiding this comment.
When a custom raw stream's write() returns an object that defines __int__ but not __index__, this int_w conversion accepts it as the number of bytes written and advances buffered state; CPython/PyPy treat such raw write counts as invalid, so an invalid raw implementation can silently drop or mis-account data instead of raising. Use the index-sized conversion/validation path for raw callback counts here as well.
AGENTS.md reference: AGENTS.md:L195-L195
Useful? React with 👍 / 👎.
| if n != 0 { | ||
| array_check_resize(obj)?; |
There was a problem hiding this comment.
When an array has an exported buffer and code deletes an empty slice such as del a[1:1], n is zero so this skips the export check, but the code below still rebuilds and replaces the backing Vec. That can invalidate existing memoryviews even though no elements are removed; CPython/PyPy raise BufferError for this case, so either check exports before rebuilding or return without touching storage when n == 0.
AGENTS.md reference: AGENTS.md:L195-L195
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
https://github.com/youknowone/pyre/blob/3cb88a09bb0565b323060febdda4912b73cca291/pyre-interpreter/src/module/_io/textio.rs#L796
When _io.TextIOWrapper.__new__ is called directly with a non-subtype such as int, this writes that arbitrary type into the object header instead of rejecting it. The allocated payload still has the W_TextIOWrapper layout, so advertising it as an unrelated built-in type can route later operations through methods that assume a different layout; PyPy/CPython validate the requested subtype before preserving subclass identity. Add the same check_user_subclass guard used by the other typed allocators before assigning w_class.
AGENTS.md reference: AGENTS.md:L195-L195
https://github.com/youknowone/pyre/blob/3cb88a09bb0565b323060febdda4912b73cca291/pyre-interpreter/src/module/_io/textio.rs#L1345
When the underlying buffer is already closed, reconfigure() only checks that the wrapper is attached and then validates options and looks up a replacement codec before the later flush() notices the closed stream. For example, a closed stream with reconfigure(encoding="bad-codec") raises the codec lookup error instead of the required closed-file ValueError; PyPy/CPython reject every reconfigure attempt on a closed wrapper before inspecting the new settings.
AGENTS.md reference: AGENTS.md:L195-L195
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1c0a5a810b
ℹ️ 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 wrapper = crate::call::call_function_impl_result( | ||
| text_io_wrapper_type(), | ||
| &[ | ||
| pyre_object::gc_roots::shadow_stack_get(buffer_slot), | ||
| w_encoding, | ||
| w_errors, | ||
| w_newline, | ||
| w_bool_from(line_buffering), | ||
| ], | ||
| )?; |
There was a problem hiding this comment.
When text-mode open() reaches this constructor call and TextIOWrapper.__init__ fails after the FileIO/buffer have been created—e.g. an unknown encoding—the ? propagates without closing buffer and therefore its raw FileIO. That leaves an opened fd/live raw stream behind instead of unwinding the partially constructed stack; close the buffered object on this error path before returning the exception.
AGENTS.md reference: AGENTS.md:L195-L195
Useful? React with 👍 / 👎.
| if !unsafe { pyre_object::bytesobject::is_bytes_like(data) } { | ||
| return Err(crate::PyError::type_error("read() should return bytes")); |
There was a problem hiding this comment.
The analogous BufferedReader path now rejects non-bytes raw.read() results, but BufferedRandom.read(-1) still accepts any bytes-like object here. When a custom seekable raw stream returns bytearray from read(), CPython/PyPy raise TypeError; pyre will consume it and advance buffered state, hiding an invalid raw implementation in the random-access class. Use the same exact-bytes check as BufferedReader.
AGENTS.md reference: AGENTS.md:L195-L195
Useful? React with 👍 / 👎.
- dict_update_source_mutation: accept a runtime that silently absorbs the source mutation during update, so the fixture agrees with a non-3.14 oracle - foriter_inplace_immutable: annotate __init__ return type Assisted-by: Claude
- BufferedWriter.readable returns False regardless of the raw stream
- BufferedWriter.seek rejects a non-seekable raw before flushing/seeking
- BufferedWriter raw write count goes through the index protocol
- BufferedReader.read requires an exact bytes result from the raw stream
- TextIOWrapper constructor resolves encoding="locale" to utf-8
- open() buffering raises OverflowError outside the machine-int range
- utf_{8,16,32}_decode report a bytes-like TypeError on non-buffer input
- array slice delete of an empty range leaves exported storage intact
- _random.getrandbits rejects a byte count exceeding usize
Assisted-by: Claude
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 33c4c95ef5
ℹ️ 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 __new__(_cls: PyObjectRef, _args: &[PyObjectRef]) -> PyObjectRef { | ||
| W_BufferedWriter::allocate_stable(W_BufferedWriter::default()) |
There was a problem hiding this comment.
Fresh evidence: this BufferedWriter constructor has its own __new__ and still ignores the requested cls, so instantiating a subclass of io.BufferedWriter returns an exact BufferedWriter; pyre's type-call guard then treats the result as foreign to the subclass and skips the subclass __init__, leaving subclass state uninitialized and type(subclass(raw)) wrong. Validate/stamp cls the same way the typed allocators that preserve subclass identity do.
AGENTS.md reference: AGENTS.md:L195-L195
Useful? React with 👍 / 👎.
| if unsafe { pyre_object::is_none(result) } { | ||
| return Err(super::buffered_writer::make_write_blocking_error(0)); | ||
| } | ||
| let written = crate::baseobjspace::int_w(result)?; |
There was a problem hiding this comment.
Fresh evidence: this separate BufferedRandom::raw_write implementation still uses int_w, so a seekable raw stream whose write() returns an object with __int__ but no __index__ is accepted as a byte count. CPython/PyPy reject non-index raw callback sizes; accepting them here can advance abs_pos and flush buffered data for an invalid raw implementation instead of raising.
AGENTS.md reference: AGENTS.md:L195-L195
Useful? React with 👍 / 👎.
* _io, builtins: address PR #735 follow-up review - TextIOWrapper: run the dev-mode error-handler lookup (io_check_errors) in __init__ and reconfigure; rebuild the codec on a newline change only when the new mode is universal-newline; always reset the encoder state and b2cratio at the end of reconfigure. - builtin_open: close the outermost constructed layer when a later layer's constructor raises, so a failed open() never leaks the raw descriptor. - BufferedReader/BufferedRandom raw read: accept bytes subclasses via isinstance_bytes_w; BufferedRandom raw write counts via space_index_w. Assisted-by: Claude * _io: pin the direct-read bytearray across raw readinto callbacks BufferedReader.read()'s large direct-read path allocated a bytearray and passed it to the raw stream's readinto without rooting it; a Python raw whose readinto triggers GC could move or collect the bytearray before the subsequent w_bytearray_data access. Pin self and the bytearray on the shadow stack around the callback and reload the bytearray afterward, as the sibling raw_read path already does. Assisted-by: Claude * pyre-macros, _codecs: drop a trailing underscore from derived keyword names A single trailing underscore on a parameter identifier is the convention for dodging a Rust keyword clash (final_, type_); the Python keyword name it binds under should drop it. Apply the strip in both the keyword table (param_name) and the Signature builder, leaving dunder names intact. This makes _codecs.utf_8_decode and friends bind their final argument under 'final' rather than 'final_'. Assisted-by: Claude * _io: rebuild the text decoder when reconfigure flips the newline flags TextIOWrapper.reconfigure only rebuilt the codec when the new newline selected universal mode, so a universal-to-fixed change (e.g. newline=None then newline="\n") left the translating IncrementalNewline Decoder installed and kept converting "\r\n". Rebuild whenever the supplied newline changes readuniversal or readtranslate, so the decoder's translation always reflects the current newline. Assisted-by: Claude
1 participant
Summary
Verification
cargo fmt --all -- --checkcargo check --workspace --features dynasmcargo test --all --no-default-features --features dynasmSummary by CodeRabbit
New Features
open()mode validation and append-mode behavior.arraytypecodew.Bug Fixes