objspace, typedef, frame, dict, imp: a user __getattribute__ on any layout, object's remaining text signatures, the collection frame.clear() forced, popitem on the strategy, and create_builtin's pre-filled import metadata

[youknowone]

Five commits on top of origin/main: two attribute-protocol correctness fixes, the collection every frame.clear() was forcing, the dict.popitem strategy routing, and the import metadata _imp.create_builtin was pre-filling.

__getattribute__ on a non-object layout

objspace: dispatch a user __getattribute__ on any receiver layout

getattr_str_impl gated the receiver-type __getattribute__ slot on is_instance, which is a W_ObjectObject layout check. An exception subclass is a W_BaseException and a list subclass a W_ListObject, so neither reached the gate — the override was simply never called, and attribute access fell through to object_getattr_miss and its own descriptor protocol. Only __getattr__ still fired, through instance_getattr_hook_or_err.

class E(Exception):
    def __getattribute__(self, name):
        E.calls += 1
        return super().__getattribute__(name)

E().args ran the override zero times.

objspace.py:664-670 gates on space.type(w_obj) with no layout test. setattr_str in the same file already resolves the type that way, which is exactly why __setattr__ worked on these classes while __getattribute__ did not.

Two things keep the widening from re-entering:

• The arm runs for space.getattr only, not for the bare object.__getattribute__ slot. object_getattribute hands its non-instance, non-type receivers back to getattr_str_impl, so dispatching there would recurse through every super().__getattribute__(name) an override makes.
• A runtime census over 403 types found that besides type and module (already excluded), exactly super, bound method, types.GenericAlias, types.UnionType and the two weakref proxy types answer Some from getattribute_if_not_from_object. Those are served by the shims earlier in the function — the bound-method one forwards to __func__ — so the dispatch is restricted to a heap-type owner and they keep the descriptor protocol they already took. The change is strictly additive.

The same commit fixes with_except_start_values, which read __traceback__ back through getattr_str while building the __exit__(type, value, traceback) arguments. pyopcode.py:1358-1362 reads W_BaseException.w_traceback directly, so it now reads the slot — otherwise a __getattribute__ override would newly observe the interpreter's own read.

object's remaining text signatures

typedef: give object's remaining 20 callables their __text_signature__

init_object_type stamped a signature on __new__ and __init__ only; the other 20 entries of object.__dict__ were built with the bare make_builtin_function / make_builtin_function_with_arity helpers and answered None. They now use the _and_text_signature twins, and __init_subclass__ / __subclasshook__ are stamped on the inner carrier before w_classmethod_new wraps it.

The strings are the ones cpython3.14 reports, not the ones gateway.py:1146 _generate_text_signature would derive from the RPython parameter names — that generator spells descr__str__(space, w_obj) as ($obj, /) and returns None outright for a varargname callable like descr___subclasshook__. extra_tests/parity_tests/run.py runs every fixture under the system CPython as well as each backend, so only the cpython spelling is assertable, and objectobject.py:187-189 hard-codes the same three strings for __repr__, __init__ and __new__ — a literal is the upstream shape here, not a pyre invention.

__doc__ is deliberately left alone. gateway.py:1328-1332 synthesizes a docstring beside the signature, but pyre's helpers only write w_text_signature and these descriptors report __doc__ as None today; changing that is a descriptor-surface change with its own fixtures asserting on it.

The collection frame.clear() forced

frame: stop clear() forcing an old-generation collection

init_frame_type's clear entry called finalize_explicitly_cleared_frame_references after descr_clear, so every frame.clear() ran a full non-moving major pass plus the finalizer queue.

That is not a frame-object curiosity. unittest's _AssertRaisesContext.__exit__ calls traceback.clear_frames(tb) on every successful assertRaises, and clear_frames calls tb_frame.clear() per traceback frame — so the whole vendored CPython suite pays it.

Per-op user CPU, same script on each:

	pyre before	pyre after	cpython3.14
traceback.clear_frames(tb)	4.8168 ms	0.0041 ms	0.0002 ms
tb.tb_frame.clear() loop	4.2657 ms	0.0016 ms	0.0001 ms

Measured with 162 modules loaded — a heap the size of a real test run — the same two operations cost 13.13 ms and 12.50 ms per op, because the forced pass scales with the old generation.

End to end, test.test_importlib used to time out past 300s. It now runs its 1346 tests in 10.8s (cpython3.14: 4.03s). One test in that module still fails, for an unrelated pre-existing reason — see Follow-up below.

The two generator.py:243 close paths in baseobjspace.rs keep the hook, and the doc comment is narrowed to say so. gh-142766 is about generator.close() releasing a suspended frame's locals before it returns; extra_tests/parity_tests/generator_python314.py asserts that, and it still passes. The frame method had been given the same call without a caller that needs it:

• test_frame.py::ClearTest.test_clear_locals — the obvious test for "frame.clear() released the reference" — calls support.gc_collect() itself after clear_traceback_frames(...) and only then asserts the weakref is dead.
• test.test_frame is recorded SKIP ("implementation detail") in pyre/cpython_tests/baseline.json, so the gate does not run it either way.
• jit_inline_traceback_frame_clear.py asserts only that a completed traceback's frames can be cleared, not when their contents die.

Deferring here is the behaviour a refcount-free runtime already has for every other reference drop.

frame_clear_finalization.py pins both sides: frame.clear() plus an explicit gc.collect() kills the weakref; close() on a suspended generator and on one closed inside try/finally finalizes before it returns; clear() on an executing frame still raises RuntimeError; and an assertRaises-shaped loop stays under 1 ms of user CPU per iteration.

dict.popitem on the strategy

dict: route popitem through the strategy instead of items().last()

dict_method_popitem called w_dict_items, which materialises a Vec of every pair, took .last(), then deleted that key. Draining a dict of N entries was O(N²), and an Int-strategy dict boxed every key through w_int_new on each of the N calls. dictmultiobject.py:257 descr_popitem delegates to W_DictMultiObject.popitem, which dispatches to the strategy.

DictStrategy::popitem had no caller outside its own unit tests, so this is the first time the family runs from Python — every impl needed an audit, not just the one being called:

strategy	LIFO	before	now
Object / Unicode / Bytes / Int	IndexMap::pop	no override — fell through to the trait default, i.e. the items() materialisation being removed	direct overrides (dictmultiobject.py:1119-1121 AbstractTypedStrategy.popitem)
Module	IndexMap::pop	bumped no keys_version on either storage path	bumps both
Mapdict	newest node_search(..., DICT) node	instance_node_getdictvalue(w_obj, key)?	reads the found node with plain_direct_read
Kwargs	array tail	already correct, already bumps	unchanged
Empty / EmptyKwargs	—	returns None	unchanged

Two of those were latent defects the routing would have exposed:

• The mapdict ? turns a value-lookup miss into the caller's "dictionary is empty" — it would skip the delete and raise KeyError on a non-empty __dict__. node_search has already returned the node, so the value is present by construction. Dropping instance_node_getdictvalue also drops its maybe_migrate_to_boxed side effect, which belongs at the getattr boundary, not on an entry about to be deleted.
• Nothing observed the module dict's missing keys_version bump while popitem was unreachable. A global read cached against a stale version would.

Bytes and Int allocate the returned key, so the popped value is pinned across that allocation, and dict_method_popitem pins both across w_tuple_new.

The w_dict_len(dict) == 0 guard stays ahead of the dispatch. It is not redundant: without it an empty mapdict-backed __dict__ would reach instance_lock and ensure_mapdict_initialized, and an empty module dict w_module_dict_object_storage_mut_opt, before raising.

w_dict_popitem is dont_look_inside (rlib/jit.py:139) — the concrete strategies pop from IndexMap or mapdict storage, neither of which the tracer models as inline heap mutation.

dict_popitem_strategy.py asserts LIFO across the int, str, object, kwargs, module-dict and instance-__dict__ strategies plus a dict subclass, covers a surrogate-named instance attribute, and pins the empty KeyError's type and message on the empty, drained and re-inserted shapes.

The import metadata _imp.create_builtin pre-filled

imp: stop create_builtin pre-filling __loader__ and __spec__

Removing the frame.clear() collection let test.test_importlib finish for the first time, and it finished with exactly one failure:

FAIL: test.test_importlib.builtin.test_loader.Source_LoaderTests.test_module
AssertionError: <class '_frozen_importlib.BuiltinImporter'> != <class '_frozen_importlib.BuiltinImporter'>

Two classes that print identically but are different objects. test_importlib/util.py:63 import_importlib builds a Source variant with blocked=('_frozen_importlib', '_frozen_importlib_external'), so the test holds a second, source-imported BuiltinImporter and asserts the loaded module's __loader__ is that one.

_imp.create_builtin routed through create_builtin_module → startup_builtin_module → set_builtin_module_spec, which fetches a spec from sys.modules["importlib._bootstrap"].BuiltinImporter and runs _init_module_attrs on it. The module handed back therefore already carried a __loader__, and _bootstrap.py:746 guards module_from_spec's own _init_module_attrs with if override or getattr(module, '__loader__', None) is None — so the caller's spec.loader never landed. __spec__ is assigned unconditionally, which is why module.__spec__.loader was right while module.__loader__ was wrong. The two disagreeing is the signature of this bug.

On an uncached errno:

	pyre before	pyre after	cpython3.14
_imp.create_builtin(spec).__loader__	frozen BuiltinImporter	None	None
_imp.create_builtin(spec).__spec__	already a ModuleSpec	None	None

set_builtin_module_spec is right for the native load_part path, which never reaches app-level _init_module_attrs. startup_builtin_module_impl now takes that decision as a parameter — the native wrapper passes true and is unchanged, _imp.create_builtin passes false.

test.test_importlib goes from FAILED (failures=1, skipped=65) to OK (skipped=65) over 1346 tests, and its baseline entry moves off the stale IMPORTERROR, so those tests are gated from now on.

Two divergences are deliberately left, both stated in the commit:

• set_sys_module stays on this path. cpython3.14 does not insert on a direct _imp.create_builtin call — a second del sys.modules[name] raises KeyError there and succeeds here — but removing it segfaults, so the rooting it provides is still load-bearing.
• The source-imported BuiltinImporter.__module__ still reads _frozen_importlib where cpython3.14 reads importlib._bootstrap. It survives this fix, so it is a separate defect rather than another symptom.

A main regression that main has already fixed

Earlier CI runs on this PR are red on test.test_datetime:

  File ".../test/datetimetester.py", line 6525, in _find_ti
    idx = bisect.bisect_right(lt, timestamp)
TypeError: bisect_right() missing 1 required positional argument: 'x'

That is not this branch's, and it is already gone. Measured with the same five commits:

base	result
e4f299c1159	gate PASS 46, FAIL 0, no regressions
1391a9656dd (#1091)	test_datetime FAIL 3/3
aaefe84f9a3 (adds 9d2fff92649 = #1063)	test_datetime PASS 3/3

The gate had not been green on any main sha between 678319fcf23 and 9d2fff92649, and neither 07e6c4aff3e (#1074) nor 1391a9656dd (#1091) ever got a CI run of its own. #1063 closed it — it stops try_walker_specialize_newtuple_object declining arity 2, so a BUILD_TUPLE pair is now the canonical array-backed tuple rather than a makespecialisedtuple2 pair, and it teaches try_walker_inline_resolved_user_call to seed a *args callee's vararg local.

The PR's first CI run merged against main at 1391a9656dd, before #1063 landed. The current tip carries the fix.

Verification

All of the below were run at the branch tip, each after a pyre/scripts/extract-llbc.py extraction taken after the last Rust edit.

	result
check.py --backend dynasm	ALL PASSED 391/391
check.py --backend cranelift	ALL PASSED 391/391
check.py --backend wasm	ALL PASSED 387/387
extra_tests/parity_tests/run.py	all parity tests pass
cargo test -p pyre-object -p pyre-jit-trace -p pyre-interpreter --features dynasm	passed
cargo fmt --all -- --check	passed
cpython_tests/run.py --backend dynasm --baseline … --jobs 3 --timeout 300	PASS 46, FAIL 0, no regressions at the earlier base; at the current base the only failure is the test.test_datetime main regression described above

New parity fixtures, each cpython=OK dynasm=OK cranelift=OK: exception_getattribute_override.py, object_init_text_signature.py, frame_clear_finalization.py, dict_popitem_strategy.py, builtin_module_loader_spec.py.

Two check.py dynasm runs failed a wall-clock ratio gate on this shared host and were attributed by re-running each alone on the same binary: synth/dir_full_mro passed 3/3 with the ratio reading 4.3x, 2.3x and 1.4x across identical runs (its whole budget is 0.02s), and synth/range_ctor_in_loop passed 2/2 with a direct timed run measuring 5.97s user CPU against 10.19s wall. Neither is a correctness or jitstats diff, and the two backends that passed 391/391 and 387/387 saw neither on the same tree.

The branch was rebased onto a newer origin/main twice during this work, which invalidates locally built binaries and the LLBC extraction each time; the numbers above were taken after re-extraction at their respective bases, and the CI run on this push is the authority for the current one.

Summary by CodeRabbit

• Bug Fixes

  • Improved dict.popitem() behavior, including LIFO ordering, module namespaces, subclasses, and unusual attribute names.
  • Corrected builtin module metadata and import behavior.
  • Improved exception attribute access, traceback handling, and generator/frame cleanup.
  • Uncaught KeyboardInterrupt now exits with standard interrupt status.

• Compatibility

  • Added support for additional object method text signatures and interpreter compatibility behaviors.

• Tests

  • Expanded parity coverage for imports, dictionaries, exceptions, frames, process exit statuses, and builtin APIs.

[coderabbitai]

Warning

Review limit reached

@youknowone, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 52 minutes

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 852c5ded-56a6-4b62-a7ff-5dd2a2b7dbbe

📥 Commits

Reviewing files that changed from the base of the PR and between 148cbd1 and 4af5443.

📒 Files selected for processing (12)

• pyre/cpython_tests/baseline.json
• pyre/extra_tests/parity_tests/dict_popitem_strategy.py
• pyre/extra_tests/parity_tests/exception_getattribute_override.py
• pyre/extra_tests/parity_tests/keyboard_interrupt_exit_status.py
• pyre/extra_tests/parity_tests/syntax_error_invalid_character.py
• pyre/pyre-interpreter/src/baseobjspace.rs
• pyre/pyre-interpreter/src/builtins.rs
• pyre/pyre-interpreter/src/objspace/std/mapdict.rs
• pyre/pyre-interpreter/src/type_methods.rs
• pyre/pyre-interpreter/src/typedef.rs
• pyre/pyre-object/src/dictmultiobject.rs
• pyre/pyrex/src/lib.rs

Walkthrough

The PR adds CPython parity tests and updates interpreter behavior for attribute access, dictionary popitem(), builtin imports, frame finalization, object signatures, KeyboardInterrupt exit status, and multi-interpreter extension checks.

Changes

Interpreter parity and runtime behavior

Layer / File(s)	Summary
Attribute dispatch and exception traceback access pyre/pyre-interpreter/src/baseobjspace.rs, pyre/pyre-interpreter/src/eval.rs, pyre/extra_tests/parity_tests/exception_getattribute_override.py	Attribute dispatch covers additional receiver types and avoids recursive default-slot dispatch. Exception traceback handling reads the typed traceback field directly.
Strategy-backed dictionary popitem() pyre/pyre-object/src/dictmultiobject.rs, pyre/pyre-object/src/celldict.rs, pyre/pyre-interpreter/src/objspace/std/mapdict.rs, pyre/pyre-interpreter/src/type_methods.rs, pyre/extra_tests/parity_tests/dict_popitem_strategy.py	Dictionary strategies remove the last entry directly, preserve special keys, root returned objects, and update mutation and key-version state.
Builtin module initialization and test execution pyre/pyre-interpreter/src/importing.rs, pyre/extra_tests/parity_tests/builtin_module_loader_spec.py, pyre/cpython_tests/run.py, pyre/cpython_tests/baseline.json	Builtin creation separates spec stamping from startup. The CPython runner executes test.test_runpy through the dotted-identity driver, and related baseline results are marked as passing.
Frame clearing and generator finalization pyre/pyre-interpreter/src/executioncontext.rs, pyre/pyre-interpreter/src/typedef.rs, pyre/extra_tests/parity_tests/frame_clear_finalization.py	Frame cleanup documentation and finalization paths now distinguish synchronous generator closing from deferred exhaustion cleanup.
Object method text signatures pyre/pyre-interpreter/src/typedef.rs, pyre/extra_tests/parity_tests/object_init_text_signature.py	Object builtins expose positional-only text signatures through gateway constructors without changing their method bodies or arity checks.
KeyboardInterrupt process termination pyre/pyrex/Cargo.toml, pyre/pyrex/src/lib.rs, pyre/extra_tests/parity_tests/keyboard_interrupt_exit_status.py	Uncaught KeyboardInterrupt restores the default SIGINT handler after reporting and finalization for both execution paths.
Multi-interpreter extension restriction API pyre/pyre-interpreter/src/module/imp/interp_imp.rs, pyre/extra_tests/parity_tests/imp_multi_interp_override.py	The new builtin validates one argument and rejects override use in the main interpreter.

Estimated code review effort: 4 (Complex) | ~60 minutes

Sequence Diagram(s)

sequenceDiagram
  participant PyrexExecution
  participant RuntimeFinalization
  participant SIGINTHandler
  PyrexExecution->>RuntimeFinalization: report KeyboardInterrupt and finalize
  RuntimeFinalization->>SIGINTHandler: restore default SIGINT disposition
  SIGINTHandler-->>PyrexExecution: terminate with SIGINT status

Loading

Possibly related PRs

• youknowone/pyre#1067 — Directly overlaps with MapDictStrategy::popitem and dictionary strategy removal behavior.
• youknowone/pyre#731 — Modifies builtin-module __spec__ and __loader__ initialization in the same import implementation.
• youknowone/pyre#654 — Also routes CPython tests through dotted-identity drivers.

Suggested reviewers: lifthrasiir

Poem

A rabbit tests each key and frame,
While builtins keep their proper name.
Signatures line up neat,
SIGINT keeps its beat,
And parity hops through every lane.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately identifies the PR's main runtime changes, including getattribute, frame clearing, dict.popitem(), and create_builtin metadata.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch import

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 5ec5fc46bd

ℹ️ 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".

[chatgpt-codex-connector]

Preserve upstream signature generation

When introspection runs against Pyre as a PyPy port, these new literals expose CPython 3.14 signatures instead of the signatures produced by PyPy's gateway. In pypy/objspace/std/objectobject.py, only __repr__, __init__, and __new__ receive explicit signatures; the remaining methods flow through gateway.py::_generate_text_signature, which derives names from the RPython arguments and returns None for unsupported varargs shapes such as __subclasshook__. Hard-coding all 20 CPython spellings therefore creates observable structural divergence; port the gateway-generation path and reserve literals for the three upstream literals.

AGENTS.md reference: AGENTS.md:L231-L233

Useful? React with 👍 / 👎.

[github-actions]

🤖 Codex parity review

Static analysis of this diff vs the local RPython/PyPy sources (commit 4af5443).
Updated: 2026-08-07T18:15:24.997Z

Files in the reviewed diff

Cargo.lock
pyre/cpython_tests/baseline.json
pyre/cpython_tests/run.py
pyre/extra_tests/parity_tests/builtin_module_loader_spec.py
pyre/extra_tests/parity_tests/dict_popitem_strategy.py
pyre/extra_tests/parity_tests/exception_getattribute_override.py
pyre/extra_tests/parity_tests/frame_clear_finalization.py
pyre/extra_tests/parity_tests/imp_multi_interp_override.py
pyre/extra_tests/parity_tests/keyboard_interrupt_exit_status.py
pyre/extra_tests/parity_tests/object_init_text_signature.py
pyre/extra_tests/parity_tests/syntax_error_invalid_character.py
pyre/pyre-interpreter/src/baseobjspace.rs
pyre/pyre-interpreter/src/builtins.rs
pyre/pyre-interpreter/src/eval.rs
pyre/pyre-interpreter/src/executioncontext.rs
pyre/pyre-interpreter/src/importing.rs
pyre/pyre-interpreter/src/module/imp/interp_imp.rs
pyre/pyre-interpreter/src/objspace/std/mapdict.rs
pyre/pyre-interpreter/src/type_methods.rs
pyre/pyre-interpreter/src/typedef.rs
pyre/pyre-object/src/celldict.rs
pyre/pyre-object/src/dictmultiobject.rs
pyre/pyrex/Cargo.toml
pyre/pyrex/src/lib.rs

Codex did not produce a report (exit 1). Last log lines:

"Structural adaptations".

Scope discipline: before writing the report, run
`git diff upstream/main --name-only -- . ':(exclude)*.jitstats'` and treat that
file list as the authoritative definition of "this patch" (when an authoritative
changed-file list is appended below, use that instead of re-deriving it). The
excluded `*.jitstats` files are `pyre/check.py`'s recorded jit-stats baselines —
generated golden data with no RPython/PyPy counterpart, so no parity finding can
cite one, and a bulk re-record of them is not a change to review. Findings under
sections 1 and 2 MUST cite our-side files from that list; a divergence in any
file NOT in the list is by definition not introduced by this patch — report
it under section 3 instead, or omit it. Verify every section-1/2 citation
against the list before finalizing the report.

---

Output format requirements (so the report can be parsed mechanically and
posted/triaged automatically). Use these four headings VERBATIM, in this
order, and nothing else at heading level 2:

## 1. Regressions to PyPy parity introduced by this patch
## 2. Other mismatches introduced by this patch
## 3. Pre-existing mismatches (already present before this patch)
## 4. Structural adaptations

Under each heading, list every finding as a bullet. For each finding cite the
concrete `our_file.rs:line ↔ rpython_or_pypy_file.py:line` pair and quote the
divergence concisely. If a section has no findings, still emit the heading
followed by a single line `None.` so all four sections are always present.
Do not modify any files; produce the report only.

Authoritative changed-file list for this patch (git diff upstream/main --name-only,
minus 0 generated `*.jitstats` baseline file(s)):
Cargo.lock
pyre/cpython_tests/baseline.json
pyre/cpython_tests/run.py
pyre/extra_tests/parity_tests/builtin_module_loader_spec.py
pyre/extra_tests/parity_tests/dict_popitem_strategy.py
pyre/extra_tests/parity_tests/exception_getattribute_override.py
pyre/extra_tests/parity_tests/frame_clear_finalization.py
pyre/extra_tests/parity_tests/imp_multi_interp_override.py
pyre/extra_tests/parity_tests/keyboard_interrupt_exit_status.py
pyre/extra_tests/parity_tests/object_init_text_signature.py
pyre/extra_tests/parity_tests/syntax_error_invalid_character.py
pyre/pyre-interpreter/src/baseobjspace.rs
pyre/pyre-interpreter/src/builtins.rs
pyre/pyre-interpreter/src/eval.rs
pyre/pyre-interpreter/src/executioncontext.rs
pyre/pyre-interpreter/src/importing.rs
pyre/pyre-interpreter/src/module/imp/interp_imp.rs
pyre/pyre-interpreter/src/objspace/std/mapdict.rs
pyre/pyre-interpreter/src/type_methods.rs
pyre/pyre-interpreter/src/typedef.rs
pyre/pyre-object/src/celldict.rs
pyre/pyre-object/src/dictmultiobject.rs
pyre/pyrex/Cargo.toml
pyre/pyrex/src/lib.rs
warning: Codex could not find bubblewrap on PATH. Install bubblewrap with your OS package manager. See the sandbox prerequisites: https://developers.openai.com/codex/concepts/sandboxing#prerequisites. Codex will use the bundled bubblewrap in the meantime.
ERROR: You've hit your usage limit. Visit https://chatgpt.com/codex/settings/usage to purchase more credits or try again at Aug 8th, 2026 3:32 AM.
ERROR: You've hit your usage limit. Visit https://chatgpt.com/codex/settings/usage to purchase more credits or try again at Aug 8th, 2026 3:32 AM.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e46450f2df

ℹ️ 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".

[chatgpt-codex-connector]

Root strategy values before allocating popped keys

When popitem() runs on a module globals dict or a mapdict-backed obj.__dict__, this new strategy dispatch reaches implementations that copy/remove the value and then allocate a string key before the value is pinned (celldict.rs:1213-1216 and mapdict.rs:4598-4601). If that allocation triggers a nursery collection, the returned local can still contain the pre-relocation pointer (or, for a removed module value, be reclaimed entirely); pinning k and v after w_dict_popitem returns is too late. Root and reload the value inside each allocating strategy implementation before constructing the key.

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Seed __package__ for directly created builtins

When _imp.create_builtin(spec) is called directly, module_from_spec() has not yet initialized import metadata, and load_builtin_module() only seeds __name__; this helper adds __loader__ and __spec__ but leaves __package__ absent, so accessing module.__package__ raises AttributeError. CPython's directly created builtin modules expose all three attributes as None, and the existing module initializer in typedef.rs seeds that same trio, so __package__ should be initialized here as well.

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

💡 Codex Review

https://github.com/youknowone/pyre/blob/c09d7ee4f54df9d91fe30655df218c6baeaa166a/pyre-object/src/dictmultiobject.rs#L4169
Keep popitem strategy dispatch traceable

In JIT-compiled code that repeatedly calls dict.popitem(), this annotation forces every operation through a residual call and hides the live-strategy dispatch and concrete popitem implementation from optimization. The corresponding W_DictMultiObject.popitem and strategy methods in pypy/objspace/std/dictmultiobject.py have no dont_look_inside barrier; rpython/rlib/jit.py explicitly defines that decorator as preventing tracing. The translator should learn the required IndexMap/mapdict mutations rather than introducing a permanent JIT boundary around an otherwise line-by-line port.

AGENTS.md reference: AGENTS.md:L231-L233

ℹ️ 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".

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 6386bd5e47

ℹ️ 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".

[chatgpt-codex-connector]

Use an allowlisted self-signal in sandbox builds

In Linux sandbox builds, real_main installs pyre-sandbox's seccomp filter before executing user code, and that filter allows tkill/tgkill but not kill, with every unlisted syscall trapped. Consequently, an uncaught KeyboardInterrupt in a pyre interact guest reaches this raw kill(2) and exits through the SIGSYS handler with a “blocked syscall” diagnostic rather than terminating by SIGINT as intended. Use an allowlisted self-thread signaling path, or add an appropriately constrained sandbox allowance.

Useful? React with 👍 / 👎.

[coderabbitai]

Actionable comments posted: 7

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/typedef.rs (1)

19055-19076: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Enforce the declared positional arity.

make_builtin_function_with_text_signature does not enforce arity, and both closures ignore positional arguments. As a result, invalid calls such as object.__init_subclass__(int, 1) can return instead of raising TypeError.

Require exactly one positional class argument for __init_subclass__ after splitting the internal keyword carrier. Register __subclasshook__ with the arity-aware gateway and require its object argument.

🤖 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/typedef.rs` around lines 19055 - 19076, Update the
__init_subclass__ closure to validate the positional arguments returned by
split_builtin_kwargs, requiring exactly one class argument before processing
keyword arguments. Change the __subclasshook__ registration to use the
arity-aware gateway and enforce its required object argument, while preserving
existing keyword validation and behavior.

Source: Coding guidelines

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@pyre/extra_tests/parity_tests/dict_popitem_strategy.py`:
- Around line 46-48: Add a bytes-only LIFO test case alongside the existing int,
str, and object cases, using only byte-string keys so it selects
BytesDictStrategy and exercises popitem. Assert reverse insertion order while
covering Vec<u8> key reconstruction and preserving the associated values.

In `@pyre/extra_tests/parity_tests/exception_getattribute_override.py`:
- Around line 54-56: Rename the ambiguous list variable l in the test setup to a
descriptive identifier, and update the subsequent marker assertion to use the
new name while preserving the existing assertions and behavior.

In `@pyre/extra_tests/parity_tests/frame_clear_finalization.py`:
- Around line 74-77: Update the assertions in the generator finalization test so
it requires seen == ["del"] immediately after generator.close() returns, before
del generator and gc.collect(); retain the later collection and final assertion
to verify cleanup remains finalized.
- Around line 127-132: Remove the per-operation CPU-time calculation and the
`per_op_ms < 1.0` assertion from this test. Replace the timing-based check
around `check.assertRaises(Expected, raises)` with a deterministic assertion
that verifies the expected cleanup behavior, using the existing test resources
and observable state.

In `@pyre/extra_tests/parity_tests/keyboard_interrupt_exit_status.py`:
- Around line 56-64: Extend the parity test around the `subprocess.run` `-m`
call by adding a temporary importable module whose execution raises
`KeyboardInterrupt`, while retaining the existing missing-module status-1
assertion. Run that module with `-m` and assert the platform-specific SIGINT
return status plus the expected traceback output.

In `@pyre/pyre-interpreter/src/eval.rs`:
- Around line 3335-3346: Add a parity test covering a context manager’s __exit__
arguments when E is raised inside a with statement. Define __exit__ to assert
its traceback argument is the stored traceback rather than
"overridden-traceback", complementing the existing direct exc.__traceback__
test.

In `@pyre/pyre-interpreter/src/objspace/std/mapdict.rs`:
- Around line 4600-4611: Update the code around plain_direct_read and
maybe_migrate_to_boxed to root w_obj before boxing, then root w_key before
reading and w_value before migration so all GC-managed references survive
allocation or movement. After migration, reload the rooted w_obj, w_key, and
w_value and recompute inst and map before calling self.delitem and returning the
pair.

---

Outside diff comments:
In `@pyre/pyre-interpreter/src/typedef.rs`:
- Around line 19055-19076: Update the __init_subclass__ closure to validate the
positional arguments returned by split_builtin_kwargs, requiring exactly one
class argument before processing keyword arguments. Change the __subclasshook__
registration to use the arity-aware gateway and enforce its required object
argument, while preserving existing keyword validation and behavior.

🪄 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: fb33cad0-5de0-4292-9428-cc8447ef2707

📥 Commits

Reviewing files that changed from the base of the PR and between dad2a72 and 6386bd5.

⛔ Files ignored due to path filters (1)

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

📒 Files selected for processing (21)

• pyre/cpython_tests/baseline.json
• pyre/cpython_tests/run.py
• pyre/extra_tests/parity_tests/builtin_module_loader_spec.py
• pyre/extra_tests/parity_tests/dict_popitem_strategy.py
• pyre/extra_tests/parity_tests/exception_getattribute_override.py
• pyre/extra_tests/parity_tests/frame_clear_finalization.py
• pyre/extra_tests/parity_tests/imp_multi_interp_override.py
• pyre/extra_tests/parity_tests/keyboard_interrupt_exit_status.py
• pyre/extra_tests/parity_tests/object_init_text_signature.py
• pyre/pyre-interpreter/src/baseobjspace.rs
• pyre/pyre-interpreter/src/eval.rs
• pyre/pyre-interpreter/src/executioncontext.rs
• pyre/pyre-interpreter/src/importing.rs
• pyre/pyre-interpreter/src/module/imp/interp_imp.rs
• pyre/pyre-interpreter/src/objspace/std/mapdict.rs
• pyre/pyre-interpreter/src/type_methods.rs
• pyre/pyre-interpreter/src/typedef.rs
• pyre/pyre-object/src/celldict.rs
• pyre/pyre-object/src/dictmultiobject.rs
• pyre/pyrex/Cargo.toml
• pyre/pyrex/src/lib.rs

[coderabbitai]

📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win

Rename the ambiguous list variable.

Ruff E741 rejects l as an ambiguous identifier. Rename it and update its uses.

Proposed fix

-l = L([1, 2, 3])
-assert l.marker is SENTINEL
+list_instance = L([1, 2, 3])
+assert list_instance.marker is SENTINEL

📝 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

	l = L([1, 2, 3])
	assert l.marker is SENTINEL
	assert L.calls == 1
	list_instance = L([1, 2, 3])
	assert list_instance.marker is SENTINEL
	assert L.calls == 1

🧰 Tools

🪛 Ruff (0.16.1)

[error] 54-54: Ambiguous variable name: l

(E741)

🤖 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/exception_getattribute_override.py` around
lines 54 - 56, Rename the ambiguous list variable l in the test setup to a
descriptive identifier, and update the subsequent marker assertion to use the
new name while preserving the existing assertions and behavior.

Source: Linters/SAST tools

[coderabbitai]

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Require finalization when close() returns.

The compatibility path explicitly collects after an unstarted generator frame is cleared. seen in ([], ["del"]) accepts a regression where generator.close() returns before Watched.__del__ runs.

Require seen == ["del"] before deleting generator.

🤖 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/frame_clear_finalization.py` around lines 74 -
77, Update the assertions in the generator finalization test so it requires seen
== ["del"] immediately after generator.close() returns, before del generator and
gc.collect(); retain the later collection and final assertion to verify cleanup
remains finalized.

[coderabbitai]

🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

Remove the CPU-time threshold.

This assertion depends on host speed and ru_utime resolution. It can fail on loaded CI workers. It can also pass when the measurement rounds to zero.

Keep this test deterministic. Assert cleanup behavior instead of elapsed CPU time.

🧰 Tools

🪛 Ruff (0.16.1)

[warning] 129-129: Use pytest.raises instead of unittest-style assertRaises

(PT027)

🤖 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/frame_clear_finalization.py` around lines 127 -
132, Remove the per-operation CPU-time calculation and the `per_op_ms < 1.0`
assertion from this test. Replace the timing-based check around
`check.assertRaises(Expected, raises)` with a deterministic assertion that
verifies the expected cleanup behavior, using the existing test resources and
observable state.

[coderabbitai]

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Root and reload references across allocation.

w_key crosses plain_direct_read, which boxes unboxed values. w_value then crosses maybe_migrate_to_boxed. Both operations can allocate or move GC-managed objects.

Pin w_obj before allocation. Pin w_key before plain_direct_read. Pin w_value before migration. Reload the rooted references and derive inst and map again before deletion and return. Otherwise a moving collection can leave self.delitem or the returned pair with stale pointers.

🤖 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/objspace/std/mapdict.rs` around lines 4600 - 4611,
Update the code around plain_direct_read and maybe_migrate_to_boxed to root
w_obj before boxing, then root w_key before reading and w_value before migration
so all GC-managed references survive allocation or movement. After migration,
reload the rooted w_obj, w_key, and w_value and recompute inst and map before
calling self.delitem and returning the pair.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 148cbd16d7

ℹ️ 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".

[chatgpt-codex-connector]

Apply C-int conversion before refusing the override

When the argument is an integer that fits i64 but not a C int (for example 1 << 40), gateway_int_w accepts it and this function raises the main-interpreter RuntimeError; CPython 3.14 instead rejects it with OverflowError before checking the interpreter. The same converter also accepts objects implementing only __int__, while this API requires the integer-index protocol. Use a C-int/index conversion here so invalid arguments retain the API's argument error rather than being misreported as an interpreter-state error.

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

💡 Codex Review

https://github.com/youknowone/pyre/blob/4af544321fe4c26457c12acc5a80abcb773d2577/pyre-interpreter/src/objspace/std/mapdict.rs#L4609
Publish the mapdict wrapper with its carrier

When obj.__dict__.popitem() runs while the wrapper is still in the nursery, this pin_roots call can trigger a collection after publishing only w_obj; tracing the carrier then forwards its __dict__ wrapper, but the local w_dict retains the old address and is later passed to self.delitem. The new result rooting therefore still permits a stale-pointer dereference or heap corruption under minor collection. Publish w_obj and w_dict together and reload the wrapper before deletion.

ℹ️ 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".