Skip to content

Remove dead code from webcrypto, the node:http binding, the class codegen, build scripts, and misc crates - #39249

Merged
Jarred-Sumner merged 2 commits into
mainfrom
claude/farm/1e897411/dead-code-webcrypto-nodehttp-codegen
Aug 16, 2026
Merged

Remove dead code from webcrypto, the node:http binding, the class codegen, build scripts, and misc crates#39249
Jarred-Sumner merged 2 commits into
mainfrom
claude/farm/1e897411/dead-code-webcrypto-nodehttp-codegen

Conversation

@robobun

@robobun robobun commented Aug 16, 2026

Copy link
Copy Markdown
Collaborator

Removes 2772 lines that nothing references (136 lines of signature, import and formatting adjustments added, plus a 214-line source lint pinning the removed symbols) across the WebCrypto bindings, the node:http internal binding and the C++/Rust behind it, the class code generator, the build scripts, and four Rust crates. No behavior change.

Problem

WebCrypto (src/jsc/bindings/webcrypto/, about 560 lines)

  • SubtleCrypto.cpp has had isRSAESPKCSWebCryptoDeprecated() returning true unconditionally since 2023, so every RSAES-PKCS1-v1_5 operation is rejected with NotSupportedError before the algorithm class is reached. Its encrypt/decrypt/generateKey/importKey/exportKey overrides, the whole of CryptoAlgorithmRSAES_PKCS1_v1_5OpenSSL.cpp, and JSRsaKeyGenParams.{cpp,h} (whose only caller was the unreachable generateKey branch) were dead. isSafeCurvesEnabled() is the same shape: always true, one never-taken branch.
  • CryptoAlgorithmEcdsaParams::encoding (DER signatures) and the padding fields of RsaPssParams/RsaOaepParams were only ever set by the old src/bun.js/bindings/KeyObject.cpp, deleted in April 2025. Since then the DER branches in CryptoAlgorithmECDSAOpenSSL.cpp, the padding branches in the PSS/OAEP OpenSSL files, and platformEncryptWithHash/platformDecryptWithHash ran on constant inputs. The same file left behind the ignoreExtAndKeyOps parameter of convertDictionaryToJS(JsonWebKey) (no caller passes true) and include blocks copied verbatim into node_crypto_binding.cpp and AsymmetricKeyValue.cpp (neither file uses any of them).
  • CryptoAlgorithmMlDsaParams::isolatedCopy() has no crossThreadCopy instantiation, CryptoAlgorithm::VoidCallback has no user, SubtleCrypto::addAuthenticatedEncryptionWarningIfNecessary is an empty body with two calls, and CryptoKey.cpp/CryptoAlgorithmX25519.cpp carried includes nothing in the file uses.

node:http binding (src/js/internal/http.ts, NodeHTTP.cpp, and the Rust it reached, about 880 lines)

  • internal/http.ts destructures the object returned by createNodeHTTPInternalBinding and re-exports the members, but no module imports getHeader, setHeader, Headers, assignHeaders, setRequestTimeout, headersTuple, webRequestOrResponseHasBodyValue or getCompleteWebRequestOrResponseBodyValueAsArrayBuffer (the last users left with the http client rewrite); assignEventCallback, Request, Response and Blob were registered but not even destructured. A few symbols (kDeprecatedReplySymbol, controllerSymbol, runSymbol, deferredSymbol, firstWriteSymbol, kEmptyObject) and isAbortError/get|setIsNextIncomingMessageHTTPS lost their last reader in earlier sweeps.
  • That binding object is the only way into NodeHTTP.cpp's jsHTTPAssignHeaders, jsHTTPAssignEventCallback, jsHTTPSetTimeout, jsHTTPGetHeader and jsHTTPSetHeader, which in turn were the only callers of assignHeadersFromFetchHeaders, assignHeadersFromUWebSockets (the ...ForCall variant used by the request path stays) and the RequestHeaderKind helpers: about 600 lines of C++.
  • Those host functions were the only callers of the Rust exports jsFunctionRequestOrResponseHasBodyValue, jsFunctionGetCompleteRequestOrResponseBodyValueAsArrayBuffer (Response.rs), Request__getUWSRequest, Request__setInternalEventCallback, Request__setTimeout (Request.rs) and NodeHTTPResponse__setTimeout (NodeHTTPResponse.rs). Request__setInternalEventCallback was the only writer of Request::internal_event_callback, and the JS side stopped calling it in 2024, so InternalJSEventCallback, the RequestContext::on_timeout handler, set_timeout_handler, the HAS_TIMEOUT_HANDLER flag and its three clear sites, and AnyRequestContext::enable_timeout_events have not been reachable since then; Body::Value::is_definitely_empty was only used by the removed host function.

Code generators and build scripts (about 1000 lines)

  • generate-classes.ts/class-definitions.ts still emitted code for .classes.ts keys no class file sets: own (and the ZigGeneratedClasses.lut.txt -> .lut.h build step in scripts/build/codegen.ts, whose output was an empty header), callbacks, the accessor field variant, supportsObjectCreate, custom, zigOnly, defaultValue, plus the ONLY_ZIG and BUN_SILENT environment switches nothing sets (git grep over all 30 *.classes.ts files, and git log -G for the keys that ever existed). bundle-functions.ts handled $nakedConstructor, $sloppy and $intrinsic directives no file in src/js/builtins uses, and tracked write-only fields; generate-jssink.ts, replacements.ts (OutOfMemoryError rewrite, no builtin throws it) and cppbind.ts had smaller leftovers. src/runtime/bake/bake.bind.ts was 100% comments and produced an empty GeneratedBake.h nothing includes.
  • scripts/utils.mjs (379 lines: downloadTarget and its helpers, getBuildArtifacts and its helpers, getChangedFiles, isDocumentation, getPullRequestRepository, getRepositoryOwner, escapeYaml, escapeGitHubAction, parseNumber, getUser, isArm64) and scripts/runner.node.mjs (listArtifactsFromBuildKite, a local escapeGitHubAction) exported functions none of the 11 importers import. glob-sources.ts globbed two patterns that have never matched a file; flags.ts defined LIBUS_USE_BORINGSSL, which nothing in src/, packages/ or vendor/ reads; config.ts carried three resolved fields nothing reads (the PartialConfig inputs stay); depVersionsHeader.ts and five deps/*.ts emitted seven version macros with no consumer in BunProcess.cpp, the header's only includer.

Rust crates (about 140 lines)

  • CssModuleReference::Local/Global are never constructed (only Dependency is) and eql has no caller; bun_sys duplicated NT_UNC_OBJECT_PREFIX/_U8 (the live copies are in bun_paths) and two Windows send-flag constants the wrappers do not use; cares_sys::AddrInfo::name has no caller; libuv_sys kept Loop::{ref_, unref, unref_count, run, tick_with_timeout, wakeup}, uv_write_t::write_raw, uv_async_t::send, Process::get_pid, uv_stat_t::{atime, ctime}, ReturnCode::from_raw, ReturnCodeI64::init, five type aliases and the two externs only those methods used. These come from the workspace reachability analysis in tools/hawk/, run for linux-gnu, darwin and windows-msvc and then re-checked by hand; the items it reported that are used only from debug_assertions code (which the release-profile analysis does not see) or only on FreeBSD were kept.

Fix

  • Deletes the items above. The only non-deletion edits are the ones the deletions force: the three RSAES rejections in SubtleCrypto.cpp become unconditional (same error strings, pinned by test/js/web/crypto/web-crypto.test.ts), isSupportedExportKey loses an unused parameter, platformEncrypt/platformDecrypt in RSA-OAEP absorb their one-line WithHash wrappers, RequestContext::set_timeout keeps its clear_timeout() for 0, one const destructure in internal/http.ts is reflowed, and generate-classes.ts conditions that became constant are folded.
  • Generator changes were verified by re-running generate-classes.ts, generate-jssink.ts and bundle-modules.ts into a scratch directory and comparing with the output generated before the change: byte-identical except for the dropped #include "ZigGeneratedClasses.lut.h" line and the two deleted webcrypto files disappearing from the NativeFilenameCPP union. bun scripts/build.ts --configure-only confirms build.ninja no longer references GeneratedBake.h, the .lut pair, or LIBUS_USE_BORINGSSL, and that the C++ source list changed only by the three deleted files.
  • Verified: bun bd builds; bun run rust:check-all passes on all 12 target triples; cargo fmt --check, prettier and clang-format are clean. bun bd test passes on test/js/web/crypto/web-crypto.test.ts (94), test/js/node/crypto/{crypto.key-objects,crypto-rsa,sign-jwk-ieee-p1363,crypto-pqc,x509} (162), test/js/node/http/{node-http,node-http-server-timeouts,node-http-server-abort-events,node-http-res-settimeout-unref} (151; the one failure, the http proxy test, fails identically with the released binary in this container), test/js/bun/http/serve.test.ts (283; the 4 failures, requestIP v6, root-range port, Bun will not close socket if receive not understandable request in Bun.serve #6583 and /bun:info, fail identically with the released binary here), test/js/node/http2/node-http2.test.js, test/js/node/fs/fs-leak.test.js, test/js/bun/udp/dgram.test.ts, test/js/node/readline/readline.node.test.ts, test/bundler/css/css-modules.test.ts, test/js/node/process/process.test.js (dependency version assertions pass; the USER env assertion fails on both binaries here), the nine test/internal/build-*/bindgen/macos-cross-config/rust-* tests, test/js/bun/perf/linker-order.test.ts, and all of test/internal/source-lints/ (166). The new dead-symbols-webcrypto-nodehttp-codegen.test.ts fails on main and passes here.
  • Cross-checked against the 19 open dead-code PRs at the line level: no deleted line here is deleted by any of them, except boilerplate lines (auto& vm = ...) that Remove dead code from C++ bindings, src/js builtins, CSS, and Rust util crates #35437 also deletes from a different function in NodeHTTP.cpp. Hunks adjacent to open PRs, which will need a trivial rebase on whichever side lands second: the internal/http.ts destructure and NodeHTTP.cpp registration block (Remove dead code from C++ bindings, src/js builtins, CSS, and Rust util crates #35437 removes setServerIdleTimeout there), the CryptoKey.cpp include block (Remove dead code from the WebCore bindings, IDL converters, and Rust FFI wrappers #38005 removes the neighbouring line), CryptoAlgorithmRSA_PSSOpenSSL.cpp (Remove dead code from the WebCore bindings, IDL converters, and Rust FFI wrappers #38005's hunk still applies), class-definitions.ts (Remove dead code from bindgen codegen, orphaned scripts, and misc crates #37208 removes isEventEmitter), generate-classes.ts (Remove dead code from C++ bindings, bindgen glue, ast, and orphaned scripts #37149 removes four DOMJIT includes), and c_ares.rs (Remove dead code from bun_core, bun_css, bun_jsc, and the FFI crates #38703 removes a method 30 lines below).

Background

  • createNodeHTTPInternalBinding is the C++ function behind $cpp("NodeHTTP.cpp", ...) in internal/http.ts; it builds a plain object whose properties are JS functions wrapping C++ host functions. A host function registered there is reachable only if some builtin reads the property, which is why the JS import graph decides what is dead in the C++ file.
  • Rust functions marked export_name/uws_callback(export = ...) exist for C++ callers; cppbind/js2native codegen emits the glue from the sources, so once the last C++ caller is gone they are unreachable, and the workspace lints (dead_code/unused_imports are deny) then flag whatever only they called, which is how the Request.rs/RequestContext.rs cascade was found and bounded.
  • Request::internal_event_callback was the hook node:http used to be told about per-request timeouts and aborts; RequestContext::set_timeout_handler registered the uWS timeout callback only when that hook was set. With no setter, the flag was never set and the handler never registered, so removing them changes nothing at runtime; server.timeout() still arms the socket timeout and the abort path is untouched.
  • generate-classes.ts reads every src/**/*.classes.ts and emits ZigGeneratedClasses.{h,cpp} plus generated_classes.rs; a feature of the class definition format that no class uses is dead code in the generator, and its removal is checkable by diffing the generated files.
  • tools/hawk/README.md describes the workspace-wide reachability analysis used for the Rust items: rustc's per-crate dead_code lint treats every pub item of a library crate as live, so cross-crate dead pub items need this separate pass.
Found dead but deliberately left alone
  • bun_shim_impl::read_without_launch and FromBunShellContext (src/install/windows-shim/bun_shim_impl.rs) have no caller, but the LauncherMode::ReadWithoutLaunch mode is threaded through the launcher, so removing it cleanly means de-generifying launcher(); better as its own change.
  • The write-only CssModule::references map itself (this PR removes only the never-constructed variants); the #[cfg(target_arch = "wasm32")] branches scattered through the parser/output code (no wasm32 target is built, but earlier commits describe it as "not built yet", so that is a product call); windows_errno::posix::{mode_t, E} and bun_sys::File::write (unused, but intentional API parity with the POSIX side); WindowsLoop::{wait, unref} and the Windows us_socket_t::write_fd stub (8 and 7 lines in files three open PRs are editing).
  • AnyResponse::on_timeout / uws_res_on_timeout in bun_uws_sys lose their last caller with this PR (set_timeout_handler was it); left for the next pass since those files are in open PRs.
  • DOMJIT support in generate-classes.ts is disabled on purpose (define() strips it), not dead; the split CI build modes in scripts/build/profiles.ts were explicitly kept by ci: build C++, Rust and link in one build-bun step instead of build-cpp + build-bun #37733 four days ago; the *.idl files in webcrypto are kept as documentation as in the previous sweeps; the AES-GCM > UINT64_MAX checks are upstream-identical.
  • Every item carrying #[allow(dead_code)] in the tree was re-verified and is live on some platform or under debug_assertions (the inventory in dead-code-escape-limits.json is accurate), so nothing was taken from there.
  • CryptoAlgorithmAKPShared.h does not include CryptoAlgorithmParameters.h, so CryptoAlgorithmMLDSA.cpp only compiles inside its unified bundle; pre-existing and unrelated to this change.

@robobun
robobun requested a review from alii as a code owner August 16, 2026 00:11
@coderabbitai

coderabbitai Bot commented Aug 16, 2026

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

You’ve reached a temporary PR review limit under our Fair Usage Limits Policy.

Your recent review volume is higher than typical usage, so adaptive limits are currently applied.

Next review available in: 35 minutes

Limit details: You’ve used all 1 included review currently available under your plan. You completed 69 included PR reviews in the past 7 days; at that activity level, included reviews refill at 1 review per hour.

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

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: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: f38a84c6-b76b-417f-a2fa-f6153a817ed7

📥 Commits

Reviewing files that changed from the base of the PR and between 22494bc and c6e13ee.

📒 Files selected for processing (65)
  • scripts/build/codegen.ts
  • scripts/build/config.ts
  • scripts/build/depVersionsHeader.ts
  • scripts/build/deps/libjpeg-turbo.ts
  • scripts/build/deps/libspng.ts
  • scripts/build/deps/libwebp.ts
  • scripts/build/deps/lsqpack.ts
  • scripts/build/deps/lsquic.ts
  • scripts/build/flags.ts
  • scripts/build/unified.ts
  • scripts/glob-sources.ts
  • scripts/runner.node.mjs
  • scripts/utils.mjs
  • src/cares_sys/c_ares.rs
  • src/codegen/bundle-functions.ts
  • src/codegen/bundle-modules.ts
  • src/codegen/class-definitions.ts
  • src/codegen/cppbind.ts
  • src/codegen/generate-classes.ts
  • src/codegen/generate-host-exports.ts
  • src/codegen/generate-jssink.ts
  • src/codegen/replacements.ts
  • src/css/css_modules.rs
  • src/js/builtins.d.ts
  • src/js/internal-for-testing.ts
  • src/js/internal/http.ts
  • src/js/internal/readline/interface.js
  • src/js/internal/repl/node-primordials.js
  • src/js/node/wasi.ts
  • src/js/private.d.ts
  • src/jsc/bindings/AsymmetricKeyValue.cpp
  • src/jsc/bindings/NodeHTTP.cpp
  • src/jsc/bindings/NodeHTTP.h
  • src/jsc/bindings/node/crypto/node_crypto_binding.cpp
  • src/jsc/bindings/webcrypto/CryptoAlgorithm.h
  • src/jsc/bindings/webcrypto/CryptoAlgorithmECDSAOpenSSL.cpp
  • src/jsc/bindings/webcrypto/CryptoAlgorithmEcdsaParams.h
  • src/jsc/bindings/webcrypto/CryptoAlgorithmMlDsaParams.h
  • src/jsc/bindings/webcrypto/CryptoAlgorithmRSAES_PKCS1_v1_5.cpp
  • src/jsc/bindings/webcrypto/CryptoAlgorithmRSAES_PKCS1_v1_5.h
  • src/jsc/bindings/webcrypto/CryptoAlgorithmRSAES_PKCS1_v1_5OpenSSL.cpp
  • src/jsc/bindings/webcrypto/CryptoAlgorithmRSA_OAEP.h
  • src/jsc/bindings/webcrypto/CryptoAlgorithmRSA_OAEPOpenSSL.cpp
  • src/jsc/bindings/webcrypto/CryptoAlgorithmRSA_PSSOpenSSL.cpp
  • src/jsc/bindings/webcrypto/CryptoAlgorithmRsaOaepParams.h
  • src/jsc/bindings/webcrypto/CryptoAlgorithmRsaPssParams.h
  • src/jsc/bindings/webcrypto/CryptoAlgorithmX25519.cpp
  • src/jsc/bindings/webcrypto/CryptoKey.cpp
  • src/jsc/bindings/webcrypto/JSJsonWebKey.cpp
  • src/jsc/bindings/webcrypto/JSJsonWebKey.h
  • src/jsc/bindings/webcrypto/JSRsaKeyGenParams.cpp
  • src/jsc/bindings/webcrypto/JSRsaKeyGenParams.h
  • src/jsc/bindings/webcrypto/SubtleCrypto.cpp
  • src/jsc/bindings/webcrypto/SubtleCrypto.h
  • src/libuv_sys/libuv.rs
  • src/runtime/bake/bake.bind.ts
  • src/runtime/server/AnyRequestContext.rs
  • src/runtime/server/NodeHTTPResponse.rs
  • src/runtime/server/RequestContext.rs
  • src/runtime/webcore/Body.rs
  • src/runtime/webcore/Request.rs
  • src/runtime/webcore/Response.rs
  • src/sys/lib.rs
  • src/sys/windows/mod.rs
  • test/js/node/fs/fs-leak.test.js

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

@robobun

robobun commented Aug 16, 2026

Copy link
Copy Markdown
Collaborator Author
Updated 6:10 PM PT - Aug 15th, 2026

@Jarred-Sumner, your commit c6e13ee is building: #98927

@robobun

robobun commented Aug 16, 2026

Copy link
Copy Markdown
Collaborator Author

Merged as a42889a (the pinning lint test was dropped before merge; the deletions landed as described above).

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I reviewed this PR and didn't find any bugs. Because it's a ~2800-line removal across 66 files that touches the live RequestContext::on_abort/set_timeout paths, drops a field from Request, and rewires the class code generator, a human look would still be worthwhile.

What was reviewed:

  • Verified none of the removed internal/http.ts exports are imported by any of the 12 consuming modules; the headersTuple hit in http2.ts is an unrelated local parameter name.
  • Traced assignEventCallbackRequest__setInternalEventCallbackinternal_event_callback: no remaining setter anywhere in src/, so the removed iec_trigger/iec_deinit calls in on_abort/finalize_request were provably no-ops; set_timeout(0) still calls clear_timeout().
  • Confirmed CssModuleReference::{Local,Global} have no constructors or match arms, the removed Config fields (androidNdk/androidApiLevel/freebsdVersion) are only read as locals during resolution (the PartialConfig inputs stay), and the dropped versionMacro deps have no BUN_DEP_* consumer in src/.
  • Checked dgramInternals.isFdAdopted, fsStreamInternals, ObjectSetPrototypeOf (interface.js), $sloppy/$nakedConstructor/OutOfMemoryError, and the Loader type decl — no live references.
Extended reasoning...

Overview

This PR removes ~2772 lines of dead code across five subsystems: WebCrypto bindings (RSAES-PKCS1-v1_5 gated behind an always-true deprecation flag; ECDSA DER encoding and RSA padding overrides whose only setter was deleted in April 2025), the node:http internal binding (NodeHTTP.cpp host functions and the Rust Request::internal_event_callback cascade whose only JS entry point was never destructured), the class code generator (own/callbacks/accessor/supportsObjectCreate/custom/zigOnly features no .classes.ts file uses, plus the empty .lut.txt.lut.h build step), build-script helpers in scripts/utils.mjs/runner.node.mjs, and cross-crate-dead Rust items in libuv_sys/cares_sys/bun_sys/css. A 214-line source lint pins every removed symbol.

Security risks

None identified. The WebCrypto changes fold constant conditions (isRSAESPKCSWebCryptoDeprecated() was already unconditionally true) — the resulting NotSupportedError rejections are byte-identical, and the ECDSA/RSA-OAEP/RSA-PSS refactors preserve the behavior their previous constant-default branches produced. No auth, permission, or input-validation code is weakened.

Level of scrutiny

High. While each individual removal is mechanical, the PR modifies live control flow in RequestContext::on_abort and set_timeout (removing calls that were provably no-ops because the callback field was never set), drops a struct field from Request (touching every constructor site), and edits the class code generator whose output feeds every generated JSC binding. The PR description states generated output was diffed byte-for-byte and rust:check-all passes on all 12 targets, and my spot-checks of the reachability claims held, but the breadth (66 files spanning C++/Rust/TS/build scripts) and the fact that several removals are cascades from a single reachability root (assignEventCallback was registered but never destructured → 880 lines across three languages) make this worth a maintainer's eye.

Other factors

The verification section is unusually thorough (build, all-target cargo check, generated-output diff, ~15 test suites, cross-check against 19 open dead-code PRs). The new source-lint test guards against reintroduction. No prior human or bot review has landed on this PR yet. I spot-checked ~15 of the removed symbols against the current tree and found no remaining references; the one apparent hit (isFdAdopted in dgram.ts) is a separate $newRustFunction binding, not an import of the removed internal-for-testing export.

…egen, build scripts, and misc crates

WebCrypto: RSAES-PKCS1-v1_5 has been rejected by an unconditional
"deprecated" check since 2023, so its encrypt/decrypt/generateKey/importKey/
exportKey overrides, the OpenSSL implementation file and the
JSRsaKeyGenParams converter were unreachable; the algorithm stays registered
so the error is unchanged. Also drops the ECDSA DER output mode and the RSA
PSS/OAEP padding overrides (nothing set them since the old KeyObject.cpp was
deleted), an unused MlDsaParams::isolatedCopy, and a few include blocks
copied from that file.

node:http: internal/http.ts still destructured eight members of the
NodeHTTP.cpp binding that no module imports. Removing them makes the host
functions behind them (assignHeaders, assignEventCallback, setRequestTimeout,
getHeader, setHeader), their header-copying helpers, and the Rust exports
they called (Request__getUWSRequest, Request__setInternalEventCallback,
Request__setTimeout, NodeHTTPResponse__setTimeout, the two body-value host
functions) unreachable. Request__setInternalEventCallback was the only writer
of the per-request internal event callback, so that machinery and the
RequestContext timeout handler it armed go too.

Codegen and build scripts: generate-classes.ts carried template branches for
.classes.ts keys no class file uses (own, callbacks, accessor,
supportsObjectCreate, custom, zigOnly, defaultValue) plus the ONLY_ZIG and
BUN_SILENT env switches nothing sets; generated output is byte-identical
apart from the now-empty ZigGeneratedClasses.lut.h include, whose build step
is removed as well. bundle-functions.ts loses directives no builtin uses,
bake.bind.ts was entirely commented out, and scripts/utils.mjs,
runner.node.mjs, glob-sources.ts, flags.ts, config.ts and the dependency
version header lose helpers and entries with no readers.

Rust: CssModuleReference::Local/Global were never constructed, and a handful
of libuv_sys / bun_sys / cares_sys items had no callers on any target.

A source lint pins the removed symbols.
@robobun
robobun force-pushed the claude/farm/1e897411/dead-code-webcrypto-nodehttp-codegen branch from d92318c to 7bd9912 Compare August 16, 2026 00:32

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I reviewed this PR and didn't find any bugs. Given the scope — ~2,800 lines removed across 66 files spanning WebCrypto, the node:http server request lifecycle (RequestContext/Request), the class code generator, and build infrastructure — a human look is still warranted before merging.

What was reviewed:

  • Spot-checked the JS→C++→Rust reachability chain for the removed internal/http.ts binding members, InternalJSEventCallback, and the RequestContext timeout-handler cascade — no remaining references in src/.
  • Verified ObjectSetPrototypeOf has no other importer of repl/node-primordials using it, and that the headersTuple hits in http2.ts are unrelated local parameter names.
  • Checked the RSA-OAEP/PSS/ECDSA constant-folding against the removed padding/encoding fields — the surviving branches match the pre-change defaults.
Extended reasoning...

Overview

This PR removes ~2,772 lines of dead code across 66 files: WebCrypto C++ bindings (the always-rejected RSAES-PKCS1-v1_5 implementation, never-set encoding/padding fields on ECDSA/RSA-PSS/RSA-OAEP params, unused includes), the node:http internal binding (unreferenced createNodeHTTPInternalBinding members and the C++/Rust host functions, Request::internal_event_callback, and the RequestContext timeout-handler machinery only it fed), unused .classes.ts features in generate-classes.ts, unused exports in scripts/utils.mjs/build scripts, and cross-crate-dead Rust items in libuv_sys/css/sys/cares_sys. A 214-line source-lint test pins the removed symbols.

Security risks

The PR touches WebCrypto (RSA-OAEP, RSA-PSS, ECDSA, RSAES-PKCS1-v1_5) and the HTTP server request lifecycle. The crypto edits fold branches whose selector fields were never written (padding defaulted to 0, encoding defaulted to IeeeP1363), so the surviving code paths are the ones that always ran; the RSAES removal is behind an unconditional NotSupportedError that the PR keeps. No security check is weakened that I can see, but crypto changes deserve maintainer eyes.

Level of scrutiny

High. The change is large, crosses three languages plus code generators, and its correctness depends on cross-FFI reachability arguments (JS import graph → C++ host-fn registration → Rust export_name) that the compiler cannot verify. The RequestContext::on_abort change removing the internal_event_callback trigger is a real behavior deletion — the PR argues it has been unreachable since 2024 because nothing sets the callback, and I found no writer, but this is exactly the kind of claim a maintainer familiar with the node:http server rewrite should confirm.

Other factors

The PR description is unusually thorough (byte-for-byte diff of regenerated codegen output, 12-triple rust:check-all, per-subsystem test runs, cross-check against 19 open dead-code PRs). The bug-hunting system found nothing. My spot checks of the reachability claims (internal/http.ts exports, ObjectSetPrototypeOf importers, CssModuleReference variants, fsStreamInternals in tests) all held. CI is still building. No prior human review on the thread.

@Jarred-Sumner
Jarred-Sumner merged commit a42889a into main Aug 16, 2026
9 of 10 checks passed
@Jarred-Sumner
Jarred-Sumner deleted the claude/farm/1e897411/dead-code-webcrypto-nodehttp-codegen branch August 16, 2026 01:10

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I reviewed this PR and the automated bug-hunt found no issues. Given its breadth — ~2,800 lines removed across 65 files spanning WebCrypto (RSA-OAEP/PSS/ECDSA constant-folding), the node:http binding and its Rust request-context cascade, the class code generator, and Windows-only libuv_sys — a human pass is still worthwhile before merging.

What was reviewed:

  • Confirmed no remaining references to the removed internal/http.ts exports, InternalJSEventCallback/HAS_TIMEOUT_HANDLER cascade, or the Request__*/NodeHTTPResponse__setTimeout Rust exports.
  • Verified the removed CryptoAlgorithmRsaOaepParams::padding/RsaPssParams::padding/EcdsaParams::encoding fields have no writers; the .padding hits in ncrypto.cpp/JSCipher.cpp are a different struct.
  • Checked fsStreamInternals/isFdAdopted have no test consumers, CssModuleReference::{Local,Global} are never constructed, and libuv_sys::Loop::wakeup/uv_async_send extern have no Rust callers (the .wakeup() hits elsewhere are on uws_sys::Loop/EventLoop; napi's uv_async_send re-export goes through the C stub, not this extern).
Extended reasoning...

Overview

This PR removes ~2,772 unreferenced lines across four clusters: WebCrypto bindings (RSAES-PKCS1-v1_5 dead overrides, constant-true feature gates, unused padding/encoding param fields and the branches they guarded), the node:http internal binding (unused $cpp exports, their C++ host functions, and the Rust Request::internal_event_callback / RequestContext::on_timeout cascade they alone reached), the class/builtin code generators (unused .classes.ts keys own/callbacks/accessor/supportsObjectCreate/custom/zigOnly and the .lut.txt build step), build scripts (utils.mjs helpers, versionMacro fields with no header consumer, unmatched glob patterns, LIBUS_USE_BORINGSSL), and misc Rust crates (libuv_sys, css_modules, cares_sys, bun_sys Windows constants). The only non-deletion edits fold now-constant conditions.

Security risks

WebCrypto is directly touched: RSA-OAEP and RSA-PSS padding branches are folded to their sole reachable value, ECDSA's DER branch is dropped, and RSAES-PKCS1-v1_5's method bodies are removed behind an unconditional NotSupportedError. The transformations are behavior-preserving if the removed fields truly had no writer — I grepped and found none outside the deleted code (the .padding hits in ncrypto.cpp are on ncrypto's own params struct, unrelated). Still, constant-folding in crypto padding paths deserves a second pair of eyes.

Level of scrutiny

High. This is not a mechanical change: it spans security-sensitive crypto, the production node:http request path (RequestContext::on_abort loses its iec_trigger call and request_weakref.deref() moves up), a code generator whose output was verified only by the author's byte-diff, and Windows-only libuv_sys items I cannot cross-check on this platform. The PR description is unusually thorough and every spot-check I ran (removed http exports, Rust FFI symbols, param-field writers, CssModuleReference constructors, libuv_sys::Loop::wakeup callers) confirmed its claims, but the sheer surface area and the "delete odd-looking code only after understanding why it was written" review rule make this a case for human review rather than auto-approval.

Other factors

The description states generator output was byte-diffed, rust:check-all passes on all 12 triples, and the relevant test suites pass; a source-lint test pins the removed symbols. The second commit deletes that source-lint test (per the PR title), so the "fails on main / passes here" claim in the description refers to the first commit. Buildkite is still running per the timeline. No prior human or claude[bot] review exists on this PR.

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.

2 participants