fix(scripts): gen_parity_gaps reads split api-manifest entries - #6504
Merged
Conversation
The manifest was split from a single entries.rs into entries/part_1.rs ..part_4.rs on 2026-07-03 (to stay under the 2000-line CI gate), but gen_parity_gaps.py's MANIFEST constant still pointed only at entries.rs, which now just declares the `mod part_N;`s and concatenates them at compile time. A dry run today parsed 0 manifest entries from the now-mostly-empty file, undercounting coverage by ~480 entries (e.g. node:os swinging from 0 gaps to 143). Glob crates/perry-api-manifest/src/entries*.rs + entries/*.rs instead of a hardcoded path, and fail loudly if the parsed entry count is implausibly low, so a future re-split can't silently break this again. docs/runtime-parity-gaps.md: land the 2026-07-16 hand-audited counts (2238/280) that were sitting uncommitted, and rewrite the banner/audit note to reflect the fixed script -- a dry run now lands at 2232/286, recovering most but not all of the 16 hand-confirmed false positives (13 remain uncounted because they're declared via the manifest's class()/internal_method() macros or non-manifest dispatch paths that the script's matcher doesn't recognize -- a separate, pre-existing gap, out of scope here).
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe parity generator now reads split manifest sources and validates coverage completeness. The runtime parity documentation updates aggregate counts, audit notes, and remaining gaps for several Node.js modules. ChangesRuntime parity audit
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/runtime-parity-gaps.md (1)
79-95: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRestore correct sort order for manually updated modules.
Because these module gap counts were updated manually, the document's structure is now out of sync with its stated sorting rule (descending by gap size, then ascending by module name). Modules that had their gaps reduced need to be moved down.
docs/runtime-parity-gaps.md#L79-L95: Reorder the summary table rows.node:urlandnode:fs(2 gaps) belong belowtimers/promises,node:buffer(5 gaps) belongs belowtrace_events, andnode:async_hooks(0 gaps) belongs in the 0-gap section directly abovediagnostics_channel.docs/runtime-parity-gaps.md#L393-L418: Move the### node:urland### node:fsbreakdown sections further down the document to correctly group them with the other 2-gap modules. Ensure the### node:async_hookssection is completely removed if it is still present, as it now has zero gaps.📝 Proposed correct order for the table rows
-| `node:url` | 47 | 2 | 49 | -| `node:fs` | 180 | 2 | 182 | | `node:readline/promises` | 0 | 7 | 7 | | `node:assert` | 21 | 6 | 27 | -| `node:buffer` | 103 | 5 | 108 | | `node:cluster` | 29 | 6 | 35 | | `node:events` | 35 | 6 | 41 | | `node:trace_events` | 0 | 6 | 6 | +| `node:buffer` | 103 | 5 | 108 | | `node:crypto` | 133 | 5 | 138 | | `node:tty` | 15 | 4 | 19 | | `node:https` | 21 | 3 | 24 | | `node:child_process` | 35 | 2 | 37 | +| `node:fs` | 180 | 2 | 182 | | `node:readline` | 27 | 2 | 29 | | `node:sqlite` | 50 | 2 | 52 | | `node:timers/promises` | 3 | 2 | 5 | +| `node:url` | 47 | 2 | 49 | | `node:worker_threads` | 63 | 1 | 64 | -| `node:async_hooks` | 29 | 0 | 29 | | `node:console` | 22 | 1 | 23 | | `node:dgram` | 27 | 1 | 28 | | `node:fs/promises` | 60 | 1 | 61 | | `node:http` | 140 | 1 | 141 | | `node:net` | 77 | 1 | 78 | | `node:stream` | 80 | 1 | 81 | | `node:zlib` | 90 | 1 | 91 | +| `node:async_hooks` | 29 | 0 | 29 | | `node:diagnostics_channel` | 30 | 0 | 30 |🤖 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 `@docs/runtime-parity-gaps.md` around lines 79 - 95, Restore the documented descending-gap, ascending-module sort order in docs/runtime-parity-gaps.md: reorder the summary rows at lines 79-95 so node:buffer follows node:trace_events, node:url and node:fs join the 2-gap group below node:timers/promises, and node:async_hooks is placed in the 0-gap section above diagnostics_channel. Move the node:url and node:fs breakdown sections at lines 393-418 to their matching 2-gap group, and remove the node:async_hooks breakdown section entirely if present.
🤖 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 `@scripts/gen_parity_gaps.py`:
- Around line 147-149: Update the manifest-reading loop in the parity-gap
generation flow to explicitly read each file as UTF-8 by passing the encoding to
path.read_text(). Keep the existing MANIFEST_RE parsing and by_module population
unchanged.
---
Outside diff comments:
In `@docs/runtime-parity-gaps.md`:
- Around line 79-95: Restore the documented descending-gap, ascending-module
sort order in docs/runtime-parity-gaps.md: reorder the summary rows at lines
79-95 so node:buffer follows node:trace_events, node:url and node:fs join the
2-gap group below node:timers/promises, and node:async_hooks is placed in the
0-gap section above diagnostics_channel. Move the node:url and node:fs breakdown
sections at lines 393-418 to their matching 2-gap group, and remove the
node:async_hooks breakdown section entirely if present.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 20c9606a-0e08-4f0b-80c9-1bcea97276dc
📒 Files selected for processing (2)
docs/runtime-parity-gaps.mdscripts/gen_parity_gaps.py
read_text() without an encoding uses the locale codec, so on a Windows runner (cp1252) reading UTF-8 Rust sources raises UnicodeDecodeError — or, on the two readers that pass errors="ignore", silently mis-decodes and drops rows from the gap report. Rust sources are always UTF-8; say so. Reported by CodeRabbit on #6504.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Root cause
scripts/gen_parity_gaps.py:35hardcoded the manifest path to a singlefile:
crates/perry-api-manifest/src/entries.rswas split intoentries/part_1.rs..part_4.rson 2026-07-03 (5212 lines total, to stayunder the 2000-line CI gate).
entries.rs(crates/perry-api-manifest/src/entries.rs:459-467)now just declares
mod part_1;..mod part_4;and concatenates them intoAPI_MANIFESTat compile time — the actualmethod(...)/property(...)rows all moved into the split files. The script never picked up the split,
so
parse_manifest()(scripts/gen_parity_gaps.py:122-126, old) parsedentries.rsalone and found ~0 manifest rows, making almost every modulelook unimplemented.
Before this fix, a dry run landed at 1753 covered / 765 gap (vs. the
true ballpark of ~2238/280) — e.g.
node:osswung from 0 gaps to 143.Fix
scripts/gen_parity_gaps.py: replaced the singleMANIFESTpath withMANIFEST_FILES = glob("entries*.rs") + glob("entries/*.rs"), andparse_manifest()now reads every matched file. Added a loud failure ifzero manifest files are found, and a second loud failure if the total
parsed entry count is implausibly low (
< 1500), so a future re-splitcan't silently zero out most of the manifest again — the script exits
with a clear message instead of quietly emitting bogus, worse-than-useless
gap counts.
docs/runtime-parity-gaps.md: landed the 2026-07-16 hand-audited counts(2238 covered / 280 gap) that a prior audit session had already worked
out but left uncommitted, and rewrote the banner + audit note to reflect
the fixed script's actual dry-run output rather than the pre-fix
catastrophic numbers.
Dry-run before / after
entries.rspath)entries.rs+entries/*.rs)Spot-check of the 5 named items
Checked with
python3 scripts/gen_parity_gaps.py --module <mod>after thefix:
new AsyncLocalStorage()(node:async_hooks) — still listed as a gapnew URLSearchParams(...)overloads (node:url) — still listed as agap;
params.entries()/.keys()/.values()on the same module arenow recovered as covered
Buffer.allocUnsafeSlow(node:buffer) — still listed as a gapstats.dev/.ino/.nlink/.rdev/.size/.blksize(node:fs) —still listed as a gap
new Worker(filename)(node:worker_threads) — still listed as a gapSo per the task's own fallback criterion, I did not run
--emit/regenerate the doc from scratch — the fixed script's manifest data still
wrongly lists 13 of the 16 hand-confirmed 2026-07-16 false positives as
missing, so a fresh
--emittoday would still be worse than the existinghand-corrected file (2232/286 vs. 2238/280) and would silently drop the
hand-corrections. I only fixed the manifest-file-discovery bug, hand-landed
the already-audited doc content (previously sitting uncommitted in another
session's working tree), and rewrote the banner/audit note to describe the
current, more accurate state.
Why those 13 rows are still invisible (separate, pre-existing bug — not fixed here)
All 13 are declared via manifest macros
MANIFEST_REnever matched, evenwith the file-glob fix:
new AsyncLocalStorage(),new URLSearchParams(),new Worker(...)aredeclared via
class("async_hooks", "AsyncLocalStorage")/class("url", "URLSearchParams")/class("worker_threads", "Worker")—class(is never matched byMANIFEST_RE(onlymethod(/property(are).
Buffer.allocUnsafeSlowis declared viainternal_method("buffer", "allocUnsafeSlow", false, None)(
crates/perry-api-manifest/src/entries/part_4.rs:111) — the\bword-boundary in
MANIFEST_REdoesn't fire between_andmethod, sointernal_method(/internal_property(/internal_class(calls areinvisible to the regex too.
stats.dev/.ino/etc. aren't manifest rows at all — they're populatedvia a packed key array (
STATS_KEYS_REGULARincrates/perry-runtime/src/fs/stats.rs), a fifth coverage shape thescript's four recognized sources don't model.
Fixing
MANIFEST_RE/adding a fifth coverage source is a separate, largerchange (would need care not to swing other modules the wrong way) and is
out of scope for this manifest-path fix. Left as a documented gap in the
updated audit note for a follow-up.
Test plan
python3 scripts/gen_parity_gaps.py(dry run, no--emit) —2232 covered / 286 gap, monotonically better than the broken
1753/765 baseline across every module (no regressions)
matched files, and simulated only-
entries.rs-matched with 0entries)
--emit;docs/runtime-parity-gaps.md's table/summarynumbers are the hand-audited 2238/280, only the banner prose changed
cargo fmt/build neededSummary by CodeRabbit
Documentation
Bug Fixes