fix(api-manifest): add 15 missing dispatch-table entries; move drift check onto cargo-test - #10817
proggeramlug wants to merge 3 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (6)
Included review availability: Your plan provides up to 8 included reviews per hour; 2 remain after this review. 📝 WalkthroughWalkthroughThe change adds 15 missing API manifest entries for Node.js properties. It moves dispatch-manifest coverage validation into a ChangesManifest dispatch coverage
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix Merge Risk: ⚪ Minimal · up to The changelog accurately documents the manifest coverage and test-gate relocation. No concrete merge-blocking issue remains. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings
🧪 Generate unit tests (beta)
🛠️ Fix failing CI checks 💡
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 |
|
Verifying the The choice is correct, and here is the evidence rather than the assertion. The drift test's matcher is: matches!(e.kind, ApiKind::Method { has_receiver, .. } if has_receiver == sig.has_receiver)It matches only More importantly the classification is accurate rather than merely test-satisfying: these rows exist in The question I would still put to a reviewer, because this PR cannot settle it: the drift test's stated purpose is that inconsistency here makes the unimplemented-API check (#463) error on real implementations. If #463 or That is a pre-existing property of how the dispatch table and manifest model accessors — these fifteen rows inherit it, they do not introduce it, and the alternative (registering them as |
|
Landed via merge train 242 (#10830) as v0.5.1621 — Eight PRs travelled together because their file sets are disjoint — 30 files, +1,514/−101, zero overlap. Validated as one tree: ten cheap gates, Two of the eight needed a fix before they could land, both made in the train rather than bounced back. #10816 bound #10817 added 15 dispatch entries without regenerating the docs, so the API-docs-drift check failed. Regenerated from a built binary: 2855 → 2870, exactly your 15, with For future PRs in this area: One more thing, aimed at whoever cuts the next PR here: |
What
crates/perry-codegen/tests/manifest_consistency.rs'severy_dispatch_entry_has_manifest_counterpartwas red on
origin/main:API_MANIFESTwas missing 15 entries that exist inNATIVE_MODULE_TABLE.This is the
pr-gate-blocking failure surfacing ine2e-scopedfor every PR in the repo.The 15 rows, credited to the two commits that actually added the dispatch-table rows without a
manifest counterpart:
From
b36554a2d7(#10467/#10468/#10469, node:http client response surface,class_filter = IncomingMessage):http::rawHeaders,http::__get_rawHeadershttp::httpVersionMajor,http::httpVersionMinorhttp::completeFrom
64ca0ebfe7(#10441/#10442/#10444/#10465, net.Socket surface cluster,class_filter = Some("Socket")orNone):net::prependListener,net::prependOnceListener(class_filter: Some("Socket"))net::pipe,net::unpipe(class_filter: Some("Socket"))net::writable,net::readable,net::writableEnded,net::readableEnded,net::_writableState,net::_readableState(class_filter: None, matching the dispatchtable's
native_table/net_events.rsrows exactly)All 15 are receiver-based property/getter reads (
has_receiver: true), not module-levelfunctions — they're registered via
method(module, name, true, class_filter), which is thisfile's established convention for representing a zero-arg
NativeMethodCallproperty read (seethe pre-existing
net::pending/net::bytesRead/http::statusCode/stream::writablerows andthe
#2549comment inentries/part_1.rsexplaining why).ApiKind::Propertyis reserved for adifferent mechanism entirely — module-level constants/accessors dispatched via custom
Expr::*variants outside
NATIVE_MODULE_TABLE(seeentries.rs's module doc, category 2) — so using ithere would both misdescribe these APIs and fail to satisfy the drift check, which only matches
ApiKind::Method.Added to
crates/perry-api-manifest/src/entries/part_1.rs(net) andpart_4.rs(http), next totheir respective existing
IncomingMessage/Socketblocks. Both files stay well under the2000-line cap (1318 and 1131 lines respectively after the addition).
Why nothing caught this drift on the PR that introduced it — and the fix for that
e2e-scopedonly runs an integration suite undercrates/*/tests/*.rsper-PR when the diffnames that file (CLAUDE.md, "Integration suites ... run per-PR only when the diff names them").
Neither
b36554a2d7nor64ca0ebfe7touchedmanifest_consistency.rs, so the one test builtspecifically to catch this drift never ran on either PR. It surfaced later, on an unrelated PR
that happened to touch the test file, and looked like that PR's fault.
Put precisely:
every_dispatch_entry_has_manifest_counterpart's trigger condition and itssubject are disjoint by construction. It's a consistency check between two tables
(
NATIVE_MODULE_TABLEandAPI_MANIFEST), so it's tripped by an edit to either table — but itonly runs, per-PR, when the diff touches its own file, which is the one file a drifting PR
(one that only adds dispatch-table rows) has no reason to touch. This is a fifth way a gate can
be unable to fail, distinct from the four CLAUDE.md already tracks under "Four ways a gate can
be unable to fail" (
continue-on-error, not in required contexts, cancelled by concurrency, orthe gate runs but its subject never does). Here the gate itself is fine — it runs, it can go red,
it's required — and the change class it exists to guard structurally never triggers it.
Fix for that (this PR, not just the report):
every_dispatch_entry_has_manifest_counterpartmoved to a
#[cfg(test)]unit test,perry_codegen::manifest_consistencyincrates/perry-codegen/src/manifest_consistency.rs(declared inlib.rs). This puts it on thecargo-test-visible per-PR gate unconditionally, regardless of which files a diff touches(CLAUDE.md: "Prefer putting acceptance coverage in
cargo-test-visible unit tests (#5960)").Feasibility:
perry-codegenalready depends onperry-api-manifestas an ordinary (non-dev)dependency (see
crates/perry-codegen/Cargo.toml), so reachingAPI_MANIFESTfrom inside thiscrate's own
lib.rstest module adds no new dependency edge.The integration test's copy of this check was removed, not kept as a duplicate: its trigger
condition is a strict subset of the new unit test's (unit test: every PR; integration test: only
PRs that touch its own file), so a passing integration-test copy could never catch anything the
unit test doesn't already catch first — it would just be two copies of identical matching logic
to keep in sync. The integration file's other checks are untouched and stay where they are:
manifest_param_counts_match_dispatch_table(#512),every_native_module_has_at_least_one_manifest_entry(#513),
cjs_style_node_builtins_have_default_entries, andevery_well_known_binding_has_manifest_entry(#513). Note
manifest_param_counts_match_dispatch_tablehas the same disjoint-trigger shape asthe check that moved — left as-is, out of scope for this pass; flagging it here so it isn't
mistaken for an oversight.
Verified the new unit test is non-vacuous: with the manifest fix temporarily reverted (dispatch
table unchanged),
cargo test -p perry-codegen --lib manifest_consistencyfails with the same15-entry list; with the fix restored, it passes.
Validation
origin/main@b9ba951ff, via a separate worktree):cargo test -p perry-codegen --test manifest_consistency—every_dispatch_entry_has_manifest_counterpartFAILS listing exactly these 15 entries; the other 4 tests in that file pass.
cargo test -p perry-codegen --test manifest_consistency— 4/4 pass (themoved test is gone from this file, by design).
cargo test -p perry-codegen --lib manifest_consistency— the new unit testpasses (1 passed).
cargo test -p perry-codegen(full lib + all integration suites + doc-tests) —all green, 1653 lib tests + every integration suite, 0 failures.
cargo test -p perry-api-manifest— all green (36 unit + 4 integration).passes with it.
cargo check --workspace --all-targets --exclude perry-ui-gtk4underRUSTFLAGS=-D warningson the default
devprofile (matching CI;perry-ui-gtk4excluded — the build host lacks itssystem libs) — clean, 0 warnings, 0 errors, exit 0.
scripts/run_lint_gates.shwithSKIP_COMPILE_GATES=1— 78 of 79 script gates passed(compile tier skipped, 2 CI-only gates skipped for lack of GitHub Actions context). The one
failure,
[Public benchmark evidence freshness] python3 benchmarks/ci_public_baseline_check.py,is the pre-existing red on every PR in this repo (ci: two reds on main fail every PR — gap-suite shard 5 parity regression (test_gap_10430) and a stale public benchmark baseline #10707) — not touched by this change.
git diff --statconfirmed clean (no destructive regen) before and after the lint run.Not run
campaign work; also out of scope for a manifest-only change).
--releasebuild/tests — this change has no runtime code path, only static manifest dataand a moved test;
perry-dev/dev-profile checks are sufficient here.condition is the PR being open, not CI going green.
pr-gate's "Public benchmark evidencefreshness" step is red on every PR in this repo already (ci: two reds on main fail every PR — gap-suite shard 5 parity regression (test_gap_10430) and a stale public benchmark baseline #10707) and is not mine to fix.
Summary by CodeRabbit
Bug Fixes
net.Socketstreams, including listener methods and readable/writable state fields.http.IncomingMessageproperties such as raw headers, HTTP version values, and completion status.Tests