gate brake: see the host seam, and stop counting retirement rows as documentation - #1105
Conversation
`SnapshotFrame::pc` and `py_pc` are `u32` in this type, so the `-1` the comment paired them with is not a value either can hold. `u32::MAX` occurred nowhere else in the file: no writer stamps it and no reader tests it. The sentinel belongs to the `i32`-typed `resume::SnapshotFrame` and `resumedata::RebuiltFrame`, whose comments are unchanged. Assisted-by: Claude
…ge entries Two holes in the brake added earlier on this branch. `gates_read_by` recognised only `env::var`/`env::var_os`, so the host seam was invisible: `importing.rs` reads `PYRE_STDLIB` through `host_os::var` and through `host_seam::ops::getenv`, which takes a byte string. That gate stayed visible only because `pyre-wasm-runner` reads it through `env::var` as well; a wasm- or sandbox-only gate would have had no such cover. `READ_FORMS` now carries all three forms and the scan accepts an optional `b` prefix. `gates_documented_in` accepted a `PYRE_*` token anywhere in `gate-triage.md`, including the retirement tables. `PYRE_SINGLE_PASS` has no read site and is named only in §1c, so re-introducing a reader for it would have landed green on the strength of its own retirement row. Tokens now count only from sections whose `##` heading does not say retired/dead/not-gates, with an assertion on each side of the split so that a heading reword fails loudly rather than silently widening what the test accepts. Re-measured with the widened forms: 105 distinct names, unchanged, because both seam reads are `PYRE_STDLIB`; 128 (file, name) pairs, up one because the two seam reads share a file; 0 undocumented under the stricter rule. F5 is rewritten to what the re-measurement shows. Of the 8 live-named gates with no `*.rs` reader, one is the `PYRE_FBW_*` fragment in §1d's heading and four (`PYRE_CHECK_PYPY3`, `PYRE_CHECK_PYTHON3`, `PYRE_SHARED_BUILD`, `PYRE_SYNTH_PYPY`) are read from `check.py`, `check_synthetic.py`, the CI workflows and `scripts/llbc_extract.py` — live, and outside this Rust-only scan. Three remain as retire targets. The scan's scope is now stated in the test's own module doc. Also in `rework.md`: the deletion policy names the Settled section as its one exception; F4/F3 parallel-safety no longer claims different crates, since both touch `pyre-jit`; and the F4 census is described as built rather than pending. Code fences in both documents carry a language. Assisted-by: Claude
|
Warning Review limit reached
Next review available in: 4 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 20537f27bc
ℹ️ 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 lower = heading.to_ascii_lowercase(); | ||
| // "retired", not "retire": §4 is a *live* section whose heading says when | ||
| // its gates will go ("retire when the epic closes"). | ||
| lower.contains("retired") || lower.contains("dead") || lower.contains("not gates") |
There was a problem hiding this comment.
The keyword-based classification treats §1d as live because its heading says “Parity verdicts” rather than “retired,” even though its table explicitly marks PYRE_FBW_VABLE_SCALAR_CA as retired. If a reader for that retired gate is reintroduced, gates_documented_in includes the historical row and the brake passes without requiring new polarity or retirement documentation—the exact failure this change is intended to prevent. Classify §1d as history explicitly rather than relying only on these heading keywords.
Useful? React with 👍 / 👎.
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit 20537f2). Files in the reviewed diff1. Regressions to PyPy parity introduced by this patchNone. 2. Other mismatches introduced by this patchNone. 3. Pre-existing mismatches (already present before this patch)None. 4. Structural adaptations
|
1 participant
Follow-up to #1102, which merged while these were being written. Nine review
findings, all verified against the source before fixing; none turned out to be a
false positive.
e720509— theu32SnapshotFramecarries no-1sentinelThere are two
SnapshotFrametypes.resume::SnapshotFrameandresumedata::RebuiltFramearei32-typed and-1is a real value there.recorder::SnapshotFrameisu32, and #1102 — in a commit titled correct theSnapshotFramepc and py_pc comments — carried thei32twin's sentinel wordingacross to it.
u32::MAXoccurred nowhere else in that file: no writer stamps it,no reader tests it. The comment now says so and points at where the sentinel does
live. The two
i32comments are correct and are left alone.20537f2— two holes in the brake #1102 addedBoth were reported by Codex, and both are the brake failing to brake.
The scanner could not see the host seam.
gates_read_byrecognised onlyenv::var/env::var_os.pyre-interpreter/src/importing.rsreadsPYRE_STDLIBthroughhost_os::varand throughhost_seam::ops::getenv,which takes a byte string — invisible to a
std::envsearch. That gate stayedcovered only by accident, because
pyre-wasm-runneralso reads it throughenv::var; a wasm- or sandbox-only gate would have had no such cover and wouldhave bypassed the brake entirely.
READ_FORMSnow carries all three forms andthe scan accepts an optional
bprefix.Retirement rows counted as documentation.
gates_documented_inaccepted aPYRE_*token anywhere ingate-triage.md, including §1/§1b/§1c/§2/§3 — thesections that record gates which are gone.
PYRE_SINGLE_PASShas no read siteand is named only in §1c, so re-introducing a reader for it would have landed
green on the strength of its own retirement row, with neither its new polarity
nor a retirement plan written down. Tokens now count only from sections whose
##heading does not say retired/dead/not-gates.That exclusion is itself load-bearing, so it is anchored on both sides rather
than trusted:
PYRE_SINGLE_PASSmust not count as documented, andPYRE_JD1(live in §6a) must. A heading reword now fails loudly instead of silently
widening what the test accepts.
Re-measured with the widened forms: 105 distinct names — unchanged, because
both seam reads are
PYRE_STDLIB— 128 (file, name) pairs, up one because thetwo seam reads share a file, and 0 undocumented under the stricter rule.
What the re-measurement did to F5
F5 claimed 51 names left to retire. Once §6 landed and the live/history split was
made explicit, the real number is 3. Of the 8 live-named gates with no
*.rsreader:
PYRE_FBW_*fragment in §1d's heading, not a gate;PYRE_CHECK_PYPY3,PYRE_CHECK_PYTHON3,PYRE_SHARED_BUILDandPYRE_SYNTH_PYPYare read fromcheck.py,check_synthetic.py, the CI workflows andscripts/llbc_extract.py. A*.rscensus reads them as retire targets andthey are not;
PYRE_FBW_REC_UNROLL,PYRE_FBW_VABLE_SCALAR_CA,PYRE_P2_DRAIN) areread from nothing in the tree and are the actual debt.
So the brake is Rust-only, and a Python- or YAML-only gate can still enter
undocumented — which is the hole §6 was written to close. That limitation is now
stated in the test's own module doc and tracked as F5, rather than left for the
next reader to rediscover. Widening the scan is deliberately not in this PR.
Also in the documents
contradicting the section two screens below it.
pyre-jit. They share no file and no symbol, which is the real reason.Verification
cargo test -p pyrex --test gate_triage_complete2 passed / 0 failed ·cargo test -p majit-metainterpclean ·cargo fmt --all -- --checkrc=0.The brake run finished in 6s, so its binary was checked against the source rather
than trusted: compiled 22:02:29, source last edited 21:51:05.
The base moved by #999 and #1097 (43
.rsfiles) plus #1102's own squash. Thegate population is unchanged at 105 — none added, none removed — so the brake's
green is a real green and not a moved goalpost.
🤖 Generated with Claude Code