Skip to content

gc: four extra root walkers instead of fifteen; the 66 unlisted gates, with a brake - #1102

Merged
youknowone merged 5 commits into
mainfrom
ec-wiring
Aug 7, 2026
Merged

gc: four extra root walkers instead of fifteen; the 66 unlisted gates, with a brake#1102
youknowone merged 5 commits into
mainfrom
ec-wiring

Conversation

@youknowone

@youknowone youknowone commented Aug 7, 2026

Copy link
Copy Markdown
Owner

Five commits closing findings from pyre/rework.md, plus one baseline correction I owed from #1095.

f62a2a8 — extra root walkers grouped by storage kind, 15 → 4

EXTRA_ROOT_WALKERS is a fixed array and the tree had filled 15 of its 16 slots
one register_extra_root_walker call at a time, so the next caller took the last
slot and the one after that hit the panic!. Grouping the callbacks by what they
actually hold makes the count a property of the design rather than of how many
walkers happened to be written:

wrapper what it holds
walk_interpreter_global_roots interpreter globals (prebuilts, threads, faulthandler)
walk_parked_exception_roots exceptions parked outside a frame
walk_immortal_store_roots immortal caches and side tables
gc_table_extra_root_walker the gcreftracer table

Behaviour-identical by construction — every callback is still called, in the same
order, from the same phase. MAX_EXTRA_ROOT_WALKERS goes 16 → 8 in the same
commit: the cap now sits above four named kinds instead of below sixteen
anonymous ones, and the comment says that raising it is the wrong response to
running out of room.

Also deletes a dead walk_bh_last_exception in pyre-interpreter.

c788f75SnapshotFrame::pc / py_pc comments

Both comments described a jitcode↔Python coordinate split that no longer exists.
Both writers stamp a JitCode offset (build_state_field_snapshot reads
MIFrame::pc; capture_snapshot_for_last_guard_multi_frame takes it from
build_framestack_snapshot), and py_pc is a forward-carried Python PC because
the resume decoder that reads it back for f_lasti holds no jitcode metadata.
Comments only.

b6d4c34 — the 66 unlisted gates, and a brake

Hand-audited, gate-triage.md had drifted to 63% empty: 66 of 105 live
PYRE_* gates had no entry, because nothing failed when a new gate skipped the
list. New §6 lists all 66, split by polarity (4 default-ON, 11 VALUE, 51
default-OFF).

The mechanical polarity rule needed a hand correction: is_none() read as a
value
means the gate is default-ON, but if …is_none() { return; } is an
early-return guard and means default-OFF. That flipped three
(PYRE_DESCR_SPELLING_GATE, PYRE_GC_DIAG, PYRE_MC_DIAG), and the correction
is recorded in the section so the next reader does not repeat it.

pyre/pyrex/tests/gate_triage_complete.rs is the brake, so the list cannot drift
again. Two details it gets right on purpose, both of which produced wrong answers
first:

  • search roots come from the workspace members list, not a tree walk, so
    untracked scratch copies of source files cannot register as live gates
    (the walk version surfaced 72 phantom gates from a scratchpad directory);
  • documented names are matched as tokens, not substrings — contains("PYRE_A")
    is satisfied by a documented PYRE_ANCHOR_STRICT, which would let a new gate
    whose name prefixes an existing one pass unnoticed.

287ac7frework.md, 441 → 227 lines

Deletes the closed findings (F1, F2), the two-date audit-refresh table, the
WS1–WS4 workstream section and the completed items; re-measures the ones still
open. F5's counts are re-derived, and the previous count was wrong in a way worth
naming: the documented-gate command counted (file, name) pairs rather than
gates, because xargs rg prefixes filenames by default — 105 distinct names live
across 127 pairs. F4's own claim that "the 16th caller dies on panic!" was
off by one (with 15 registered, the 16th succeeds and the 17th panics).

6659e16 — wasm baseline: fbw_blackhole_adopted_single_frame 3 → 0

This corrects a baseline I recorded wrong in #1095, and the reason is worth
stating because it is a rule I had only half-applied.

I recorded the 3 from a treatment arm that finished in 4 seconds. A wasm
relink for this bench measures 519 seconds — the 4-second arm never rebuilt
and its number was vacuous. I had demanded rebuild proof of the control arm
only.

Re-measured properly, with the root-walker commit reverse-applied in place and a
cargo check gate before the expensive run (control arm elapsed 519s, i.e. it
genuinely relinked): the counter reads 0 both with and without this branch,
so it is not this branch's doing. Three separate observations read 0, and the
re-recorded baseline then verified 15/15. loops_compiled=4 bridges_compiled=3
were unchanged throughout, so the trace shape is identical either way.

Verification

At the pre-rebase tree: check.py --backend dynasm 403/403 ALL PASSED · cargo test 2486 passed / 0 failed · cargo test -p pyrex --test gate_triage_complete
2 passed · check.py --backend wasm 398 passed · cargo fmt --all -- --check
clean.

--backend cranelift came back 401 passed with 2 failures that were timeouts,
not wrong answers
, measured at 1-min load 27–56 with two sibling worktrees also
running suites. Isolated and re-run 5× each against the same binary:
fib_recursive worst 2.59s against a 5s limit, synth/range_ctor_in_loop worst
7.93s against a 20s limit. Both clear unloaded.

Re-verification on the rebased base is running now; the base moved by #1084 and
#1085, and #1084 edits pyre-jit/src/eval.rs — the one file this branch also
touches. They merged without conflict and in different regions (#1084 adds a
custom trace and a vable_token slot; this branch rewrites
install_gc_root_walkers), and the registration count on the rebased tree is
still exactly 4, so #1084 added no walker that the grouping would have missed.

Deliberately not in this PR

Two changes that alter what gets compiled, so they need their own verification
round rather than riding along on a docs-and-wiring branch:

  • making the _other catch-all in codewriter.rs name its opcode — F4's only
    remaining silent hole, and the one thing that would make the abort census
    self-reporting;
  • renaming the pc_map local in pyjitpl.rs to match SnapshotFramePcs.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes

    • Improved runtime handling of garbage-collection roots and exception state.
    • Corrected JIT snapshot position tracking to support more accurate resume behavior and traceback information.
    • Updated JIT statistics for reused-object exception scenarios.
  • Documentation

    • Added a measured inventory of runtime configuration gates.
    • Updated project status documentation with current findings, progress metrics, and remaining work.
  • Tests

    • Added automated coverage to verify that environment configuration gates are documented.

`register_extra_root_walker` held fifteen callbacks against a cap of sixteen,
one per population that had been found to need rooting. Seven walked the same
shape -- an exception parked in a raw cell the precise collector cannot reach --
and four walked immortal process-global stores.

Group them into one registration per kind of root storage, the granularity
`framework.py root_walker.walk_roots` registers at:

  walk_interpreter_global_roots   global prebuilt, thread, faulthandler
  walk_parked_exception_roots     jit_exc, bh_last, guard, immortal singleton
                                  children, last_ca, jit_pending, active_sym
  walk_immortal_store_roots       rbigint cache, sre patterns, w_globals,
                                  mapdict method cache

Each group calls the same functions in the same order with the same visitor, so
the walk itself is unchanged. `MAX_EXTRA_ROOT_WALKERS` drops 16 -> 8, and its
comment now says a new source belongs inside an existing kind rather than in a
slot of its own.

`register_pyframe_root_walker` becomes `register_interpreter_global_root_walker`:
frame roots moved to the per-mutator `PyFrameRootArea` and it registers none.

Deletes `walk_bh_last_exception`, which had no caller -- `walk_pyframe_roots_area`
already forwards `BH_LAST_EXC_VALUE` through `walk_raw_exception_cell_area`.

Assisted-by: Claude
`pc`'s comment said pyre's tracer populates the slot with the Python bytecode PC,
and that the runtime translates `py_pc` through `pc_map` at resume time "until
pyre's walker-as-tracer epic lands". All three are false now:
`build_state_field_snapshot` and `capture_snapshot_for_last_guard_multi_frame`
both stamp a JitCode offset, and `metadata.pc_map` / `resume_jitcode_pc_for` have
no hits left.

`py_pc` gains the reason it is carried rather than derived: the resume decoder
that reads it back for `f_lasti` and traceback reconstruction holds no jitcode
metadata, so no jitcode->Python inverse is available at that point.

Assisted-by: Claude
… 67th

Measured against the tree, 66 of the 105 distinct `PYRE_*` names read from the
environment had no entry in `gate-triage.md`. The earlier counts of 119 and 126
"distinct names" were (file, name) pairs: the documented command keeps rg's
filename prefix, so `sort -u` counted read sites rather than gates.

Section 6 lists all 66, split by the polarity their read expression implies: 4
default-ON, 11 VALUE knobs, 51 default-OFF diagnostics. The polarity rule needed
a correction to produce that split -- an `is_none()` whose value is the enable
flag means ON, but an `if ...is_none() { return; }` early-return guard means OFF,
which is how three diagnostics read as ON under the unqualified rule.

`pyre/pyrex/tests/gate_triage_complete.rs` fails when a `PYRE_*` env read has no
entry. It searches the workspace members' `.rs` files rather than the whole
repository, so untracked scratch copies of source do not count, and it matches
`PYRE_*` tokens rather than substrings, so a new gate whose name is a prefix of a
listed one cannot pass.

Assisted-by: Claude
F1 (resume coordinates invented their own system) and F2 (three trace-time
executors) are deleted: `is_full_body_walk`, `PYRE_FULL_BODY_WALK`,
`OpcodeHandler for MIFrame`, `metadata.pc_map` and `resume_jitcode_pc_for` all
have zero hits. Also deleted: the two-date audit-refresh table, the workstream
section that duplicated each finding's own task list, and the items already done.

Two numbers were wrong.

F5 reported 126 "distinct `PYRE_*` names read from the environment"; the command
it documents counts (file, name) pairs. The distinct count is 105, and 66 of them
had no entry in `gate-triage.md`.

F4 rested on "~214 abort_permanent matches, unchanged in scale". Counting
emission sites rather than mentions gives 23 `emit_abort_permanent!` calls, all in
`codewriter.rs`: 9 generator/async trace boundaries, 5 conditional shapes inside
otherwise-lowered opcodes, 8 unported opcodes, and one `_other` catch-all. The
catch-all is the whole unlisted surface, so the finding needs a one-site change
rather than the census it was recorded as blocked on.

F3 records the four remaining root sources and the question that decides the next
deletion: whether a thread-local exception cell needs the mid-major
`rescan_major_nonstack_roots_and_drain` pass the per-mutator areas skip.

Sequencing returns to the charter's F4 > F3 > F5.

441 -> 227 lines.

Assisted-by: Claude
…produced

`a8673ce33a4` recorded `fbw_blackhole_adopted_single_frame=3` for
`synth/exception_reused_object_tb_not_doubled` on wasm. It does not reproduce.
Three observations on this tree read 0 -- a full `check.py --backend wasm` run,
and both arms of an attribution experiment -- with `loops_compiled=4` and
`bridges_compiled=3` unchanged throughout, so the trace shape is the same and
only the adoption count differs. check.py re-ran the fixture under
`JITSTATS_STABILITY_RUNS` and it reproduced 0, so this is not run-to-run noise.

The 3 came from an arm that cannot be shown to have rebuilt. That experiment
read the control arm's 2m32s as proof it relinked the wasm module, but applied
no such test to the treatment arm, which returned in 4s. Measured here, a wasm
relink for this bench takes 519s; a 4s arm reused whatever module was already on
disk.

Re-recorded with `check.py --snapshot --backend wasm --synthetic-pattern
exception_reused_object_tb_not_doubled`; the bench then passes 15/15 on wasm.
dynasm and cranelift never carried the key at a nonzero value and are untouched.

Assisted-by: Claude
@coderabbitai

coderabbitai Bot commented Aug 7, 2026

Copy link
Copy Markdown

Review Change Stack

Walkthrough

The PR consolidates GC root-walker registration, clarifies JIT snapshot coordinates, catalogs environment gates with an enforcement test, updates a JIT statistics baseline, and rewrites the rework status record.

Changes

GC root-walker consolidation

Layer / File(s) Summary
Aggregate root walkers and registration
majit/majit-gc/src/shadow_stack.rs, pyre/pyre-interpreter/src/eval.rs, pyre/pyre-jit/src/eval.rs, pyre/pyre-jit/src/call_jit.rs, pyre/bench/synth/...
The runtime registers aggregate interpreter, parked-exception, and immortal-store walkers. Obsolete blackhole exception traversal is removed. Root-walker capacity and rooting documentation are updated. The JIT statistics baseline records zero single-frame blackhole adoption.

Snapshot coordinate semantics

Layer / File(s) Summary
Snapshot frame coordinate contract
majit/majit-metainterp/src/recorder.rs
SnapshotFrame::pc is documented as a JitCode byte offset. py_pc remains the Python instruction offset for resume decoding and traceback reconstruction.

Environment-gate audit

Layer / File(s) Summary
Gate inventory and classifications
pyre/gate-triage.md
The triage document adds measured gate counts and classifications for default-ON gates, configuration knobs, and default-OFF diagnostics.
Workspace gate completeness check
pyre/pyrex/tests/gate_triage_complete.rs
A Rust test scans workspace sources for PYRE_* environment reads and verifies that each gate and file pair appears in the triage document.

Rework status record

Layer / File(s) Summary
Measured findings and status
pyre/rework.md
The document records current F4, F3, and F5 findings, remaining actions, sequencing, settled decisions, and falsification criteria.

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

Sequence Diagram(s)

sequenceDiagram
  participant install_gc_root_walkers
  participant interpreter_root_walker
  participant parked_exception_roots
  participant immortal_store_roots
  install_gc_root_walkers->>interpreter_root_walker: register interpreter-global roots
  install_gc_root_walkers->>parked_exception_roots: register parked-exception roots
  install_gc_root_walkers->>immortal_store_roots: register immortal-store roots
Loading
sequenceDiagram
  participant gate_test
  participant workspace
  participant rust_sources
  participant triage_document
  gate_test->>workspace: discover member crates
  gate_test->>rust_sources: scan PYRE_* reads
  gate_test->>triage_document: read documented gate names
  gate_test->>gate_test: report missing gate/file pairs
Loading

Possibly related PRs

Suggested reviewers: lifthrasiir

Poem

A rabbit hops through roots so tight,
JIT offsets now point just right.
Gate names bloom in rows of green,
Old blackhole counts fade unseen.
The status book records the way.

🚥 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 clearly identifies the two main changes: GC root-walker consolidation and documentation of 66 previously unlisted gates.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
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 ec-wiring

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

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

@github-actions

github-actions Bot commented Aug 7, 2026

Copy link
Copy Markdown

🤖 Codex parity review

Static analysis of this diff vs the local RPython/PyPy sources (commit 6659e16).
Updated: 2026-08-07T09:48:40.139Z

Files in the reviewed diff
majit/majit-gc/src/shadow_stack.rs
majit/majit-metainterp/src/recorder.rs
pyre/gate-triage.md
pyre/pyre-interpreter/src/eval.rs
pyre/pyre-jit/src/call_jit.rs
pyre/pyre-jit/src/eval.rs
pyre/pyrex/tests/gate_triage_complete.rs
pyre/rework.md

1. Regressions to PyPy parity introduced by this patch

None.

2. Other mismatches introduced by this patch

None.

3. Pre-existing mismatches (already present before this patch)

None.

4. Structural adaptations

  • majit/majit-gc/src/shadow_stack.rs:1275 ↔ rpython/memory/gctransform/framework.py:1758 — Pyre retains a fixed, runtime-registered eight-walker array; RPython’s translated root walker enumerates statically generated root ranges. This is a Rust/GC integration adaptation, not a semantic change in this patch.

  • majit/majit-metainterp/src/recorder.rs:90 ↔ rpython/jit/metainterp/resume.py:249 — Pyre carries py_pc alongside the upstream-equivalent (jitcode_index, pc) resume coordinate. PyPy serializes only JitCode index and JitCode PC; the extra Python-PC coordinate is required by Pyre’s CPython-compatible traceback/f_lasti reconstruction path.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 6659e16c35

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

/// a listed one would slip through the brake unnoticed.
fn gates_documented_in(triage: &str) -> BTreeSet<&str> {
let mut found = BTreeSet::new();
for (at, _) in triage.match_indices("PYRE_") {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Distinguish active entries from historical mentions

When a retired gate is reintroduced, this accepts its old mention in a retired section as current documentation. For example, PYRE_WASM_CA and PYRE_SINGLE_PASS remain in the document's retirement history, so restoring either reader would leave this test green without recording its new polarity or retirement plan. Restrict the accepted set to structured live entries rather than every PYRE_* token in the file.

Useful? React with 👍 / 👎.

Comment on lines +82 to +85
for (at, _) in text.match_indices("env::var") {
let rest = &text[at + "env::var".len()..];
let rest = rest.strip_prefix("_os").unwrap_or(rest);
let Some(rest) = rest.strip_prefix('(') else {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Include the repository's environment abstractions

When a gate is read only through a platform abstraction, this scanner never sees it because it recognizes only env::var and env::var_os. The inspected tree already reads the environment through crate::host_seam::ops::getenv and host_os::var in pyre-interpreter/src/importing.rs; PYRE_STDLIB happens to be masked by additional standard-library reads elsewhere, but a new sandbox- or host-only gate would bypass the brake entirely. Include these established read paths or derive gate reads from a centralized API.

Useful? React with 👍 / 👎.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 7

🤖 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 `@majit/majit-metainterp/src/recorder.rs`:
- Line 84: Clarify the no-snapshot sentinel documentation near
SnapshotFrame::pc: distinguish the upstream or resumed framestack `pc == -1`
representation from the stored Rust-side condition `pc == u32::MAX`. Preserve
the existing snapshot writer and reader code paths and document whichever
sentinel is intentionally used consistently.

In `@pyre/gate-triage.md`:
- Around line 853-855: Specify the shell language on the fenced block containing
the git/rg command by changing its opening fence to use sh, while leaving the
command unchanged.

In `@pyre/rework.md`:
- Around line 3-7: Resolve the contradiction between the deletion policy near
the document introduction and the retained closed findings in the settled
section: either remove the two closed entries there, or revise the policy to
explicitly allow settled decisions to retain a brief rationale. Keep the chosen
rule consistent with the document’s existing purpose and terminology.
- Line 235: Update the F4 census wording in rework.md to remove the obsolete
“once built” future-state phrasing. Refer directly to the already-measured
census result, or describe the remaining _other opcode naming step.
- Around line 147-149: Update the F5 item in pyre/rework.md to mark the
gate-completeness check for undocumented PYRE_* environment reads as
implemented, reflecting pyre/pyrex/tests/gate_triage_complete.rs. Remove only
that completed work from the future-work wording and retain the remaining
classification and stale-name tasks.
- Line 128: Add the shell language identifier to the fenced code block
containing the command at lines 129–130 by changing its opening fence to a
shell-tagged fence, while leaving the command content unchanged.
- Line 192: Update the F4/F3 entry in rework.md to remove the claim that they
are parallel-safe and that they involve different crates with no shared surface.
Replace it with the actual shared dependency boundary, or omit any
parallel-safety declaration.
🪄 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: 02e83915-98c6-43f8-be21-bffece4c6f86

📥 Commits

Reviewing files that changed from the base of the PR and between 128590c and 6659e16.

📒 Files selected for processing (9)
  • majit/majit-gc/src/shadow_stack.rs
  • majit/majit-metainterp/src/recorder.rs
  • pyre/bench/synth/exception_reused_object_tb_not_doubled.wasm.jitstats
  • pyre/gate-triage.md
  • pyre/pyre-interpreter/src/eval.rs
  • pyre/pyre-jit/src/call_jit.rs
  • pyre/pyre-jit/src/eval.rs
  • pyre/pyrex/tests/gate_triage_complete.rs
  • pyre/rework.md

/// between the two.
pub pc: u32,
/// Forward-carried Python instruction PC for this JitCode position.
/// `u32::MAX` is the no-snapshot sentinel paired with `pc == -1`.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

rg -n -C 4 \
  'SnapshotFrame|build_state_field_snapshot|build_framestack_snapshot|capture_snapshot_for_last_guard_multi_frame|py_pc|u32::MAX|pc\s*==\s*-1' \
  --glob '*.rs' .

Repository: youknowone/pyre

Length of output: 50372


🏁 Script executed:

#!/bin/bash
set -euo pipefail

FILE="majit/majit-metainterp/src/recorder.rs"
if [ -f "$FILE" ]; then
  echo "== recorder.rs outline =="
  ast-grep outline "$FILE" --view compact || true
  echo "== recorder.rs relevant lines =="
  sed -n '1,180p' "$FILE" | cat -n
else
  echo "missing $FILE"
  fd -a recorder.rs .
fi

echo "== exact matches focused =="
rg -n -C 3 'SnapshotFrame|build_state_field_snapshot|build_framestack_snapshot|capture_snapshot_for_last_guard_multi_frame|u32::MAX|pc == -1|pc == u32::MAX' majit/majit-metainterp/src/recorder.rs majit/majit-metainterp/src majit/majit-metainterp || true

Repository: youknowone/pyre

Length of output: 50372


Clarify the no-snapshot py_pc sentinel.

SnapshotFrame::pc is u32, so pc == -1 is not a Rust-side value. The -1 reference matches the resumed/framestack form; note the upstream frame PC sentinel if intentional, or document the stored condition as pc == u32::MAX. Ensure snapshot writers/readers keep the same code path.

🤖 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 `@majit/majit-metainterp/src/recorder.rs` at line 84, Clarify the no-snapshot
sentinel documentation near SnapshotFrame::pc: distinguish the upstream or
resumed framestack `pc == -1` representation from the stored Rust-side condition
`pc == u32::MAX`. Preserve the existing snapshot writer and reader code paths
and document whichever sentinel is intentionally used consistently.

Comment thread pyre/gate-triage.md
Comment on lines +853 to +855
```
git ls-files '*.rs' | xargs rg --no-filename -o 'env::var[_a-z]*\("(PYRE_[A-Z0-9_]+)"' -r '$1' | sort -u
```

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Specify the shell language for this fenced block.

Add sh to the opening fence. This fixes markdownlint rule MD040.

Proposed fix
-```
+```sh
 git ls-files '*.rs' | xargs rg --no-filename -o 'env::var[_a-z]*\("(PYRE_[A-Z0-9_]+)"' -r '$1' | sort -u
📝 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
```
git ls-files '*.rs' | xargs rg --no-filename -o 'env::var[_a-z]*\("(PYRE_[A-Z0-9_]+)"' -r '$1' | sort -u
```
🧰 Tools
🪛 markdownlint-cli2 (0.23.2)

[warning] 853-853: Fenced code blocks should have a language specified

(MD040, fenced-code-language)

🤖 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/gate-triage.md` around lines 853 - 855, Specify the shell language on
the fenced block containing the git/rg command by changing its opening fence to
use sh, while leaving the command unchanged.

Source: Linters/SAST tools

Comment thread pyre/rework.md
Comment on lines +3 to +7
**Status**: living record, companion to `design.md` (the charter). Where the
charter states what pyre must be, this states where today's code violates it and
what is left to do about it. **Findings are deleted as they close** — the history
of what was once wrong belongs in git, not here. Keep this document small enough
that it is worth re-reading.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Align the closed-finding policy with the retained section.

Line 5 says findings are deleted when they close, but Lines 202-215 retain two closed findings. Either remove those entries or state that settled decisions may retain a short rationale.

Based on the document’s own policy and settled section, these rules currently conflict.

🤖 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/rework.md` around lines 3 - 7, Resolve the contradiction between the
deletion policy near the document introduction and the retained closed findings
in the settled section: either remove the two closed entries there, or revise
the policy to explicitly allow settled decisions to retain a brief rationale.
Keep the chosen rule consistent with the document’s existing purpose and
terminology.

Comment thread pyre/rework.md
| names mentioned in `gate-triage.md` | 90 | of which only 39 are still read |
| **live gates absent from `gate-triage.md`** | **66** | the debt |

```

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Add a language tag to the shell fence.

The command at Lines 129-130 is in an untyped fenced block. Add shell to satisfy Markdown lint.

Based on the markdownlint hint, this fence requires a language identifier.

Proposed fix
-```
+```shell
📝 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
```
🧰 Tools
🪛 markdownlint-cli2 (0.23.2)

[warning] 128-128: Fenced code blocks should have a language specified

(MD040, fenced-code-language)

🤖 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/rework.md` at line 128, Add the shell language identifier to the fenced
code block containing the command at lines 129–130 by changing its opening fence
to a shell-tagged fence, while leaving the command content unchanged.

Source: Linters/SAST tools

Comment thread pyre/rework.md
Comment on lines +147 to +149
2. **Add the brake**: a check that fails when a `PYRE_*` env read has no entry in
`gate-triage.md`. Without it the table re-rots the moment it is filled, which
is how it got to 63% empty.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Mark the gate-completeness check as implemented.

The PR adds pyre/pyrex/tests/gate_triage_complete.rs for undocumented PYRE_* reads, but these lines still list that check as future work. Update F5 to record the test as complete and retain only the remaining classification and stale-name work.

Based on the PR objectives and stack context, the enforcement test is part of this change.

🤖 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/rework.md` around lines 147 - 149, Update the F5 item in pyre/rework.md
to mark the gate-completeness check for undocumented PYRE_* environment reads as
implemented, reflecting pyre/pyrex/tests/gate_triage_complete.rs. Remove only
that completed work from the future-work wording and retain the remaining
classification and stale-name tasks.

Comment thread pyre/rework.md
- **F5 out of order whenever convenient** — it is cheap, it is the only item that
gets *worse* while ignored, and its brake is what keeps it closed.

F4 and F3 are parallel-safe: different crates, no shared surface.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

rg -n -C 4 \
  'emit_abort_permanent|EXTRA_ROOT_WALKERS|walk_.*roots|register.*root' \
  pyre/pyre-jit pyre/pyre-interpreter majit/majit-gc

Repository: youknowone/pyre

Length of output: 50372


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "## pyre/rework.md around F4/F3"
sed -n '170,210p' pyre/rework.md | cat -n

echo
echo "## changed files likely in F3/F4 (if repo has diff metadata)"
git diff --name-only HEAD~1..HEAD 2>/dev/null || git diff --name-only 2>/dev/null | sed -n '1,120p'

echo
echo "## search F4/F3 identifiers and codewriter/eval/call_jit mentions"
rg -n -C 2 '\bF[0-9]\b|codewriter\.rs|call_jit\.rs|pyre-jit' pyre/rework.md pyre/pyre-jit/pyproject.toml pyre/pyre-jit/src 2>/dev/null | sed -n '1,220p'

Repository: youknowone/pyre

Length of output: 18149


Remove the parallel-safe claim for F4 and F3.

F4 is in pyre/pyre-jit/src/jit/codewriter.rs; F3 touches the same crate, including pyre/pyre-jit/src/eval.rs and pyre/pyre-jit/src/call_jit.rs, where call_jit.rs registers root walkers and uses codewriter/eval interfaces. Replace “different crates, no shared surface” with the actual dependency boundary or do not declare these findings parallel-safe.

🤖 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/rework.md` at line 192, Update the F4/F3 entry in rework.md to remove
the claim that they are parallel-safe and that they involve different crates
with no shared surface. Replace it with the actual shared dependency boundary,
or omit any parallel-safety declaration.

Comment thread pyre/rework.md
- If WS3's class-(b) absorption measurably regresses minor-collection
pause (prebuilt scanning cost), the registry survives *for that class
only*, documented as the deliberate adaptation it currently isn't.
- If the F4 census, once built, shows the unlisted set is already empty, F4

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Remove the obsolete future-state wording.

The F4 census is already reported at Lines 24-30. Line 235 still says “once built,” which describes completed work as future work. Refer to the current census result or to the remaining _other opcode naming step.

Based on the current F4 section, the census has already been measured.

🤖 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/rework.md` at line 235, Update the F4 census wording in rework.md to
remove the obsolete “once built” future-state phrasing. Refer directly to the
already-measured census result, or describe the remaining _other opcode naming
step.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant