Repository navigation
perf(pm): reduce utoo-pm binary size - #3183
Conversation
elrrrrrrr
changed the title
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request replaces the gix library with the system git executable for cloning and checking out git dependencies, significantly reducing the dependency tree by removing crates like gix, hickory-resolver, and moka. It also removes the macOS-specific aws-lc-rs TLS configuration, standardizing on reqwest's default rustls provider across all native platforms. Feedback on these changes highlights a critical argument injection vulnerability in the git clone execution that can be mitigated by using -- to separate options from positional arguments, as well as a future-proofing suggestion to support 64-character SHA-256 commit hashes in the is_full_sha helper.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| clone_args.extend(["--branch", ref_name]); | ||
| } | ||
| } | ||
| clone_args.extend([effective_url.as_str(), checkout_str]); |
There was a problem hiding this comment.
Security Vulnerability: Argument Injection in git clone
If effective_url starts with a dash (-), it can be interpreted as a command-line option by git clone (e.g., --config or -u), potentially leading to arbitrary command execution.
To prevent this, use -- to separate options from positional arguments (the repository URL and destination directory).
| clone_args.extend([effective_url.as_str(), checkout_str]); | |
| clone_args.push("--"); | |
| clone_args.extend([effective_url.as_str(), checkout_str]); |
| fn is_full_sha(commit_ish: Option<&str>) -> bool { | ||
| commit_ish.is_some_and(|c| c.len() == 40 && c.chars().all(|ch| ch.is_ascii_hexdigit())) | ||
| } |
There was a problem hiding this comment.
Future-proofing: Support SHA-256 Commit Hashes
Git is transitioning to SHA-256, where full commit hashes are 64 hex characters instead of 40. Updating is_full_sha to accept both 40 and 64 character hex strings ensures compatibility with SHA-256 repositories.
| fn is_full_sha(commit_ish: Option<&str>) -> bool { | |
| commit_ish.is_some_and(|c| c.len() == 40 && c.chars().all(|ch| ch.is_ascii_hexdigit())) | |
| } | |
| fn is_full_sha(commit_ish: Option<&str>) -> bool { | |
| commit_ish.is_some_and(|c| { | |
| (c.len() == 40 || c.len() == 64) && c.chars().all(|ch| ch.is_ascii_hexdigit()) | |
| }) | |
| } |
4173edc to
000232f
Compare
|
@codex review |
![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: 000232f2be
ℹ️ 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".
| clone_args.extend(["--branch", ref_name]); | ||
| } | ||
| } | ||
| clone_args.extend(["--", effective_url.as_str(), checkout_str]); |
There was a problem hiding this comment.
When GITHUB_TOKEN/GH_TOKEN is set for an HTTPS git dependency, effective_url contains x-access-token:<token>@..., and passing that value directly as the git clone command-line argument exposes the secret through process listings such as /proc/*/cmdline or ps while the clone is running. Use a credential helper, askpass, or an http.extraHeader/stdin-based mechanism so private-repo installs do not leak tokens to other local processes.
Useful? React with 👍 / 👎.
Empirical backing for dropping the macOS
|
| registry | ring (median / min) | aws-lc-rs (median / min) | aws-lc vs ring |
|---|---|---|---|
| registry.npmmirror.com | 1.47s / 1.32s | 1.87s / 1.83s | +27% slower |
| registry.antgroup-inc.cn | 2.59s / 2.26s | 2.84s / 2.19s | +10% median / tied at min (noisy) |
cold resolve is network-bound, so aws-lc's crypto edge is invisible, while its per-client load_native_certs + install_default adds fixed overhead. So unifying to ring is a size win and a small perf win on China registries — no tradeoff on the registries that matter here. (The original ~40% likely came from npmjs.org / a different setup.)
Superseding my #3182 in favor of this PR — it's a superset (also trims tokio/full → concrete features + stream, plus SHA-256 / -- clone hardening). Closing #3182.
…e::chunk` (#3188) #3142 added live install progress using `response.bytes_stream()`, which requires reqwest's `stream` feature. #3183 (binary slimming) had removed that feature on a base that predated the call site, so once both landed on `next` the combination failed to compile (`E0599: no method bytes_stream`). #3187 fixed the breakage by re-adding the `stream` feature. But download progress never needed `stream`. `Response::chunk()` reads the body frame-by-frame with no feature gate, so it feeds the same per-chunk `DOWNLOADED_BYTES` counter the spinner renders. The body is still fully buffered before return (the extractor is not streaming), so this only buys the progress signal, not lower peak memory — identical behavior, one fewer feature. Revert #3187's `stream` feature (drops the wasm-only `wasm-streams` dep from the lock) and rewrite the download loop to use `chunk()`, dropping the `futures::StreamExt` import. Verified: cargo check -p utoo-pm, cargo clippy --all-targets -D warnings, cargo fmt all clean; Cargo.lock diff is the exact inverse of #3187. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Summary
utoo-ruboristnative git resolution by delegating clone/checkout to the systemgitexecutable, while keeping the existing cache layout, token injection, dedup cache, manifest finalization, and atomic cache commit protocol.utoo-pm/utoo-ruboristrelease features: drop reqwesthttp2,hickory-dns, andstream; remove the macOS aws-lc TLS override; keep socks proxy support; replacetokio/fullwith the features the PM code actually uses; align PM on workspacedirs.utoopm-binary-sizeGitHub Actions job that assertsh2,hickory-resolver,gix, andaws-lc-rsstay out of the Linux release dependency tree and reports the release binary size.--before clone positional args and accepting both 40-char SHA-1 and 64-char SHA-256 full hashes.Local Size Result
Measured in this worktree with
cargo build -p utoo-pm --release --bin utooon macOS:origin/next:10,419,376bytes6,319,232bytes-4,100,144bytes (-39.35%)The local Linux-target release-tree assertion also passes:
cargo tree -p utoo-pm --target x86_64-unknown-linux-gnu -e normal,build --prefix none # no h2 / hickory-resolver / gix / aws-lc-rs entriesReqwest / Tokio Alternatives Notes
reqwestis still the lowest-risk fit for the async registry clients because the code relies on async requests, shared client builders, proxy handling, socks proxy support, gzip/json, rustls, and custom DNS resolver policy. The important win here is feature diet rather than replacing it. Reqwest docs also call out socks as an explicit feature for socks proxy envs, so this PR keeps it: https://docs.rs/reqwest/ureqis a plausible future experiment for isolated blocking download paths, but it is blocking by design and would needspawn_blockingplus new pooling/proxy/retry integration for the resolver: https://docs.rs/ureqminreqis even smaller/minimal, but too bare for the current registry client behavior without rebuilding much of the HTTP policy stack: https://docs.rs/minreqtokioshould stay; PM uses process, fs, net, sync, time, select, and the multi-thread runtime. The size win is replacingfullwith a concrete feature list. Tokio's feature list showsfullincludes extras such asio-std,parking_lot, andsignalthat PM release code does not need: https://docs.rs/crate/tokio/latest/featuresValidation
cargo fmttombi format Cargo.toml crates/pm/Cargo.toml crates/ruborist/Cargo.tomlcargo clippy --all-targets -- -D warnings --no-depscargo test -p utoo-ruborist --features native-git -- --nocapture(201 passedbefore the follow-up hardening)cargo test -p utoo-ruborist --features native-git full_sha_detection_accepts_sha1_and_sha256 -- --nocapturecargo test -p utoo-ruborist --features native-git ensure_repo_cached_clones_local_repo -- --nocapturecargo test -p utoo-pm -- --nocapture(272 passed,3 ignored)cargo build -p utoo-pm --release --bin utoo