Prune foreign-platform binaries, duplicate packages, and npm-only files from the packaged CLI - #228
Open
Miista wants to merge 10 commits into
Open
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughThe PR deduplicates runtime dependency bundles by package name, version, and content. It adds compiled CLI dependency exclusions. It expands packaged Node module sanitization for native binaries, duplicate dependencies, distribution files, SDK dependencies, and platform-specific executables. ChangesPackaging runtime and release contents
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant RuntimeBundler
participant PackageMetadata
participant BundleFilesystem
RuntimeBundler->>PackageMetadata: Read dependency name and version
RuntimeBundler->>BundleFilesystem: Compare dependency trees
RuntimeBundler->>BundleFilesystem: Copy or symlink the dependency
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
Contributor
Greptile SummaryThe PR reduces packaged CLI size by deduplicating vendored dependencies and pruning foreign native binaries, duplicate package trees, Windows-only helpers, and npm-only CommonJS output.
Confidence Score: 4/5The Windows release path needs a symlink-free fallback before merging because ordinary build hosts can reject the new directory-symlink operation. Runtime dependency deduplication now relies on directory symlink creation even on Windows, where insufficient symlink privileges cause vendoring and artifact production to abort. Files Needing Attention: packages/cli-common/src/workspaces/index.ts
|
| Filename | Overview |
|---|---|
| packages/cli-common/src/workspaces/index.ts | Adds general runtime-dependency deduplication, but its unconditional directory symlink can fail on Windows hosts lacking symlink privileges. |
| scripts/pipeline/release/lib/binary-release.mjs | Adds narrowly targeted release-tree pruning for foreign native assets, known duplicate packages, and npm-only CommonJS output. |
| packages/cli-common/src/workspaces/index.test.ts | Covers the diamond-dependency deduplication shape but does not provide a Windows-safe fallback for the operation under test. |
| scripts/pipeline/release/lib/binary-release.onnxruntime-node-prune.test.mjs | Verifies nested and flat native prebuild pruning across all supported CLI targets. |
| scripts/pipeline/release/lib/binary-release.bare-fs-and-ps-list-prune.test.mjs | Verifies bare-* prebuild and platform-specific ps-list helper pruning. |
| scripts/pipeline/release/lib/binary-release.duplicate-vendored-package-prune.test.mjs | Verifies removal of the two known nested package duplicates while preserving top-level copies. |
| scripts/pipeline/release/lib/binary-release.package-dist-cjs-prune.test.mjs | Verifies recursive removal of CommonJS files from the CLI package-dist payload. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
A[Resolve runtime dependency] --> B{Seen same name and version?}
B -- No --> C[Copy package tree]
C --> D[Record destination]
B -- Yes --> E[Create relative directory symlink]
E --> F[Package target payload]
F --> G[Sanitize native and duplicate files]
G --> H[Create release archive]
Reviews (1): Last reviewed commit: "Prune more packaged-CLI bloat: bare-fs/u..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| // survives the rename because the relationship between the two paths doesn't change. | ||
| rmDirSafeSync(depDestDir); | ||
| mkdirSync(dirname(depDestDir), { recursive: true }); | ||
| symlinkSync(relative(dirname(depDestDir), existingDedupePath), depDestDir, 'dir'); |
Contributor
There was a problem hiding this comment.
When a Windows CLI build vendors a repeated name@version dependency on a host without directory-symlink privileges, this unconditional symlinkSync(..., 'dir') throws EPERM, aborting runtime vendoring and preventing the Windows artifact from being built. Use a Windows-safe fallback that does not require Developer Mode or elevation.
Knowledge Base Used: packages/cli-common
Contributor
Author
There was a problem hiding this comment.
Fixed in 46c0fef — the symlinkSync call is now wrapped in try/catch, falling back to a real copyDirSafeSync on EPERM/EACCES/ENOSYS (the errors a Windows host without Developer Mode or elevated privileges would raise). Verified via the real local-build installer-smoke lifecycle on darwin; CI's Windows job will exercise the actual fallback path.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@packages/cli-common/src/workspaces/index.ts`:
- Around line 571-601: Update the deduplication logic around
vendorRuntimeDependencyTree in
packages/cli-common/src/workspaces/index.ts:571-601 so name@version alone never
triggers symlinking and recursion skipping; copy and recurse unless the complete
vendored dependency closures are proven equivalent. Update the related
expectations or coverage in
packages/cli-common/src/workspaces/index.test.ts:216-234 and :254-269 to
preserve nested dependency resolution and verify that distinct closures are not
deduplicated.
In `@scripts/pipeline/release/lib/binary-release.mjs`:
- Around line 693-696: Restrict the `package-dist` pruning branch in
`prunePackagedTreeDirectory` to the staged root entry at
`stageDir/package-dist`, rather than every nested directory named
`package-dist`; preserve traversal for vendored nested trees so their `.cjs`
files remain intact. Update `binary-release.package-dist-cjs-prune.test.mjs`
with a `main: './package-dist/index.cjs'` fixture and assert that
`package-dist/index.cjs` is not deleted.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 23e9516d-a293-427a-bc7a-f945fa48f61c
📒 Files selected for processing (7)
packages/cli-common/src/workspaces/index.test.tspackages/cli-common/src/workspaces/index.tsscripts/pipeline/release/lib/binary-release.bare-fs-and-ps-list-prune.test.mjsscripts/pipeline/release/lib/binary-release.duplicate-vendored-package-prune.test.mjsscripts/pipeline/release/lib/binary-release.mjsscripts/pipeline/release/lib/binary-release.onnxruntime-node-prune.test.mjsscripts/pipeline/release/lib/binary-release.package-dist-cjs-prune.test.mjs
Miista
force-pushed
the
fix/prune-onnxruntime-node-foreign-platform-binaries
branch
from
August 4, 2026 20:22
e72bd7b to
528ae71
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@packages/cli-common/src/componentArtifacts/buildCliBinaryArtifactPayload.ts`:
- Around line 62-88: Add an artifact-level regression test for
buildCliBinaryArtifactPayload that constructs a fixture compiled payload,
asserts a package from CLI_BINARY_PAYLOAD_VENDORING_EXCLUDED_PACKAGES is absent,
and asserts sharp remains vendored. Place the test alongside the
buildCliBinaryArtifactPayload implementation and exercise the compiled-payload
path rather than only the generic excludePackageNames helper.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 8ca69fe8-4627-4e79-a65b-e7f7481a726e
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (4)
apps/cli/package.jsonpackages/cli-common/src/componentArtifacts/buildCliBinaryArtifactPayload.tspackages/cli-common/src/workspaces/index.test.tspackages/cli-common/src/workspaces/index.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/cli-common/src/workspaces/index.ts
Comment on lines
+62
to
+88
| const CLI_BINARY_PAYLOAD_VENDORING_EXCLUDED_PACKAGES = new Set<string>([ | ||
| '@agentclientprotocol/sdk', | ||
| '@anthropic-ai/claude-agent-sdk', | ||
| '@modelcontextprotocol/sdk', | ||
| '@stablelib/hex', | ||
| 'archiver', | ||
| 'axios', | ||
| 'chalk', | ||
| 'cross-spawn', | ||
| 'diff', | ||
| 'expo-server-sdk', | ||
| 'fastify', | ||
| 'fastify-type-provider-zod', | ||
| 'http-proxy', | ||
| 'https-proxy-agent', | ||
| 'ink', | ||
| 'open', | ||
| 'openapi-types', | ||
| 'ps-list', | ||
| 'qrcode-terminal', | ||
| 'react', | ||
| 'react-devtools-core', | ||
| 'socket.io-client', | ||
| 'tar', | ||
| 'tmp', | ||
| 'zod', | ||
| ]); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inventory existing payload-build coverage.
fd -t f . packages/cli-common/src \
| rg '\.(test|spec)\.[cm]?[jt]sx?$' \
| xargs -r rg -n -C 3 \
'buildCliBinaryArtifactPayload|copyCliNodeRuntimePayload|CLI_BINARY_PAYLOAD_VENDORING_EXCLUDED_PACKAGES|vendorBundledPackageRuntimeDependencies|sharp'
# Confirm tests cover both exclusion and retained native runtime dependencies.
rg -n -C 3 \
'excludePackageNames|CLI_BINARY_PAYLOAD_VENDORING_EXCLUDED_PACKAGES|sharp' \
packages/cli-common/src \
-g '*.{test,spec}.{ts,tsx,js,jsx}'Repository: happier-dev/happier
Length of output: 15977
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Candidate files:"
fd -t f 'buildCliBinaryArtifactPayload.*test\.ts|buildCliBinaryArtifactPayload\.ts' packages/cli-common/src/componentArtifacts
echo
echo "buildCliBinaryArtifactPayload.ts outline:"
ast-grep outline packages/cli-common/src/componentArtifacts/buildCliBinaryArtifactPayload.ts --view compact || true
echo
echo "buildCliBinaryArtifactPayload.ts relevant sections:"
sed -n '1,220p' packages/cli-common/src/componentArtifacts/buildCliBinaryArtifactPayload.ts | cat -n
echo
echo "Workspace-sync test relevant sections:"
sed -n '1,280p' packages/cli-common/src/componentArtifacts/buildCliBinaryArtifactPayload.workspaceSync.test.ts | cat -n
echo
echo "Windows rename fallback test relevant sections:"
sed -n '1,170p' packages/cli-common/src/componentArtifacts/buildCliBinaryArtifactPayload.windowsRenameFallback.test.ts | cat -n
echo
echo "All repo references to excluded constant/list:"
rg -n -C 2 'CLI_BINARY_PAYLOAD_VENDORING_EXCLUDED_PACKAGES|chalk|axios|sha' packages/cli-common/src/componentArtifacts packages/cli-common/src/workspaces -g '*.{ts,tsx,js,jsx,test.ts,spec.ts}'Repository: happier-dev/happier
Length of output: 45696
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "buildCliBinaryArtifactPayload.ts rest:"
sed -n '220,460p' packages/cli-common/src/componentArtifacts/buildCliBinaryArtifactPayload.ts | cat -n
echo
echo "workspaces index relevant sections:"
sed -n '1,440p' packages/cli-common/src/workspaces/index.ts | cat -nRepository: happier-dev/happier
Length of output: 21874
Add an artifact-level regression test for the compiled-payload exclusions.
packages/cli-common/src/workspaces/index.test.ts covers the generic excludePackageNames helper and sharp retention, but it does not cover buildCliBinaryArtifactPayload’s compiled payload path. Add an existing-fault-future-failing test in packages/cli-common/src/componentArtifacts/buildCliBinaryArtifactPayload that builds a fixture payload, proves one listed excluded package is absent, and proves sharp remains vendored.
🤖 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 `@packages/cli-common/src/componentArtifacts/buildCliBinaryArtifactPayload.ts`
around lines 62 - 88, Add an artifact-level regression test for
buildCliBinaryArtifactPayload that constructs a fixture compiled payload,
asserts a package from CLI_BINARY_PAYLOAD_VENDORING_EXCLUDED_PACKAGES is absent,
and asserts sharp remains vendored. Place the test alongside the
buildCliBinaryArtifactPayload implementation and exercise the compiled-payload
path rather than only the generic excludePackageNames helper.
Source: Coding guidelines
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/cli-common/src/workspaces/index.test.ts (2)
138-141: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep the new tests on the typed export path.
Lines 138-141 cast the module namespace to
Record<string, unknown>. Lines 178-183 cast the export to a function. Keep the dynamic import if isolation is required, but destructure the typed export directly:const { bundleWorkspacePackageWithRuntimeDependencies } = await import('./index');This preserves strict TypeScript checking and detects export or parameter changes at compile time.
Also applies to: 178-183, 199-202, 232-236
🤖 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 `@packages/cli-common/src/workspaces/index.test.ts` around lines 138 - 141, Update the affected tests to destructure bundleWorkspacePackageWithRuntimeDependencies directly from the dynamic import of './index'. Remove the Record<string, unknown> module cast and any function casts at all referenced test blocks, preserving the typed export and compile-time parameter checking.Source: Coding guidelines
290-382: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winProtect the package-name-and-version deduplication key.
The fixture covers two copies of
shared-dep@1.2.3. Add a secondshared-depversion and assert that both versions remain separate and resolve to their own contents. A name-only deduplication bug would otherwise pass this test.🤖 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 `@packages/cli-common/src/workspaces/index.test.ts` around lines 290 - 382, Extend the test around bundleWorkspacePackageWithRuntimeDependencies to include a second shared-dep copy with a different version and distinct content, then assert both name@version instances remain separate and resolve to their own files. Ensure the existing same-version nested copy still symlinks to the first vendored copy, while the different-version copy is vendored independently rather than deduplicated by name alone.
🤖 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.
Nitpick comments:
In `@packages/cli-common/src/workspaces/index.test.ts`:
- Around line 138-141: Update the affected tests to destructure
bundleWorkspacePackageWithRuntimeDependencies directly from the dynamic import
of './index'. Remove the Record<string, unknown> module cast and any function
casts at all referenced test blocks, preserving the typed export and
compile-time parameter checking.
- Around line 290-382: Extend the test around
bundleWorkspacePackageWithRuntimeDependencies to include a second shared-dep
copy with a different version and distinct content, then assert both
name@version instances remain separate and resolve to their own files. Ensure
the existing same-version nested copy still symlinks to the first vendored copy,
while the different-version copy is vendored independently rather than
deduplicated by name alone.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: ed01fac0-cc8c-4d24-97b9-d082a5104c57
📒 Files selected for processing (3)
packages/cli-common/src/componentArtifacts/buildCliBinaryArtifactPayload.tspackages/cli-common/src/workspaces/index.test.tspackages/cli-common/src/workspaces/index.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/cli-common/src/componentArtifacts/buildCliBinaryArtifactPayload.ts
- packages/cli-common/src/workspaces/index.ts
Contributor
Author
|
The failing checks on this PR (`Installer Smoke (Windows)`, `Installer Smoke (macOS)`, `Typecheck`, `CLI Tests`, `UI Tests`, `Core E2E`, `CLI + Server E2E`) are pre-existing failures on `dev` itself, unrelated to this change. Verified by comparing against the latest `dev`-branch CI runs:
Locally, the actual change in this PR has been verified via the real `installers-smoke` local-build lifecycle (`install → --version → --help → check → reinstall → check → uninstall`) passing cleanly on every commit, plus 293/293 passing in `scripts/pipeline/**/*.test.mjs` and the `cli-common` workspace's vitest suite. Happy to help investigate/fix the upstream CI issues separately if useful, but didn't want to fold that into this PR's scope. |
onnxruntime-node (a transitive dependency of @huggingface/transformers, used for local embeddings) bundles prebuilt native binaries for every platform/arch under bin/napi-v<N>/<platform>/<arch>/ inside a single package, rather than splitting them into per-target optionalDependencies. Its package.json declares os: [win32, darwin, linux] with no cpu constraint, so the existing whole-package pruning in prunePackagedTreeDirectory never touches it — every single-platform CLI release tarball ships all 6 platform/arch binary sets (~177MB of the ~208MB onnxruntime-node bin/ tree is unused on any given install). Extend the packaging-time tree sanitizer to recognize known bundle-all-platforms directory layouts and prune non-matching platform/arch subdirectories in place, in addition to the existing whole-package os/cpu constraint check.
…ages, package-dist .cjs Extends the platform-binary pruning added earlier on this branch with four more independently-verified size reductions in the packaged CLI payload: - bare-fs/bare-url/bare-os (nested under archiver -> tar-stream) bundle prebuilt natives for every platform/arch with no os/cpu package.json gating, same anti-pattern as node-pty. Added to the existing flat-layout prune pattern list. - ps-list ships Windows-only fastlist-*.exe helpers unconditionally. prunePackagedTreeDirectory previously only pruned directories; added a file-level branch to strip these on non-Windows targets. - tar (inside onnxruntime-node's own node_modules) and @modelcontextprotocol/sdk (inside @anthropic-ai/claude-agent-sdk's own node_modules) are exact, byte-identical duplicates of copies already vendored at the payload's top-level node_modules. The independent per-package vendoring entry points in workspaces/index.ts don't share a visited set with each other, so a dependency already vendored at the top level gets vendored again inside a nested package's tree. Deleted the nested duplicates outright at packaging time; ordinary upward-walking module resolution finds the top-level copy. - package-dist/*.cjs (139 files) is the dual-format npm-publish build output copied verbatim into the Homebrew/binary payload, but only the .mjs half is ever loaded by the compiled binary's entrypoints -- the .cjs half exists solely for the separately-published npm package's require() consumers, a different distribution channel. Pruned for the binary-release path only; the npm-publish path is untouched. Also hardens workspaces/index.ts's vendorRuntimeDependencyTree with a name@version dedup map so future builds don't reintroduce the @modelcontextprotocol/sdk duplicate in the first place (symlinks to the first-vendored copy instead of copying again). Measured on the real installed darwin-arm64 v0.2.10-dev.53 payload: 991MB -> 676MB installed, 222.5MB -> 129.7MB compressed tarball.
…st type files Second round of packaged-CLI size reduction, on top of the fixes already in this PR: - @modelcontextprotocol/sdk vendors express, express-rate-limit, cors, and jose for its OAuth-authorization-server and Express-adapter code paths, and a standalone hono package alongside the (actually used) @hono/node-server. Traced every require() reachable from happier's real SDK entry points (server/index.js, server/mcp.js, server/streamableHttp.js, client/*.js) and cross-checked against zero occurrences of these package names in the compiled binary's strings output -- none of these are ever loaded. Does NOT touch first-party SDK source under server/auth/ (server/auth/errors.js is reachable via client/auth.js and must survive). - Extended DUPLICATE_VENDORED_PACKAGE_DIR_PATTERNS with more confirmed byte-identical nested duplicates: zod (4 copies), archiver-utils (1), qs (1, removes most of a get-intrinsic duplication chain as a byproduct), get-intrinsic (1 remaining pattern), readable-stream (4, all in archiver's own dependency chain). Each has a verified surviving ancestor reachable by upward node_modules resolution once the nested copy is removed. An ajv duplication pattern was considered and explicitly rejected: those copies have no surviving ancestor anywhere in the payload and must be kept. - package-dist's dual-format pruning (added earlier in this PR for .cjs) now also strips .d.mts/.d.cts type-declaration files -- confirmed dead weight for the same reason as .cjs: this is a compiled Bun binary, not a package resolved via npm's exports/types condition machinery. Measured on the already-fixed 676MB darwin-arm64 baseline: 676MB -> 600MB installed, 130.1MB -> 120.7MB compressed tarball.
@huggingface/transformers ships browser, CJS, minified, and WASM-backend
build variants in dist/ (44MB), but this payload -- a Bun/Node-only CLI --
only ever resolves dist/transformers.node.mjs via the package's own
package.json exports.node.import condition. Confirmed via
createLocalTransformersEmbeddingsProvider.ts's dynamic
`await import('@huggingface/transformers')` and its own error-handling code,
which explicitly checks for this exact filename. The bundled
ort-wasm-simd-threaded.jsep.{mjs,wasm} pair is onnxruntime-web's browser-only
WASM backend; the node build imports the native onnxruntime-node binding
instead, confirmed by the bundler's own "onnxruntime-web (ignored)" comment
in the compiled node output.
Adds a new keep-list-based pruning mechanism (DIST_ROOT_KEEP_FILE_ALLOWLIST)
alongside the existing pattern-list mechanisms, for the "package ships many
dist/ build targets, only one is ever resolved on this runtime" case.
~41MB installed size reduction (600MB -> 559MB on the already-fixed
darwin-arm64 baseline), not platform/arch-sensitive since the exports-map
resolution doesn't depend on OS/arch.
…dencies
Verified with git-history tracing, cross-workspace checks, and
peer-dependency analysis (not just grep) that these declared dependencies
have zero reachable runtime or build-time usage anywhere in the repo:
- @fastify/swagger: dead in both apps/cli and its sibling apps/server;
never wired up in either.
- @stablelib/base64: added alongside apps/cli's real base64 helper (which
uses plain Buffer.toString('base64')) and never used; its real historical
use was mobile-only and has since been fully retired there too.
- ai (Vercel AI SDK): added in the same commit as ACP/Gemini backend work
but never referenced by any file in that commit or since; confirmed
absent from the compiled dist bundle.
- http-proxy-middleware: a different, unused package confusable with the
actually-used http-proxy (which apps/cli's proxy code calls directly).
Also moved tmp to devDependencies: only imported from *.test.ts files,
never from apps/cli/src at runtime, and not referenced by any postinstall-
reachable script (unlike tar, which was considered and correctly rejected
for the same move -- unpack-tools.cjs requires it and runs via postinstall
on real end-user npm installs).
Two candidates from the initial survey were investigated and correctly
rejected: openapi-types (a mandatory, non-optional peer dependency of
fastify-type-provider-zod and @fastify/swagger, both real direct deps) and
react-devtools-core (ink's peerDependency, conditionally dynamic-imported
by ink's reconciler when DEV=true is set in the environment -- a real,
if rarely-exercised, code path outside apps/cli's own control).
Verified via the full local-build installer smoke lifecycle (install ->
version -> help -> check -> reinstall -> check -> uninstall) against a
freshly-compiled binary with these changes applied -- all steps passed.
~14.8MB removed across the 4 dropped packages.
…nary Bun's --compile tree-shakes apps/cli's own TypeScript source into the compiled binary (confirmed via `strings` on the real shipped binary: reachable files carry provenance comments, unreachable ones -- like the express/server-auth code already pruned in an earlier commit -- don't). But copyCliNodeRuntimePayload unconditionally vendored EVERY declared apps/cli dependency as loose files on disk, regardless of whether Bun had already compiled the reachable code directly into the executable. Most non-native dependencies were therefore present twice: compiled into the 76MB binary, and duplicated as a full loose node_modules tree alongside it. Verified per-package before excluding anything, not as a blanket policy: - Confirmed apps/cli/bin/*.mjs (the npm-publish entrypoint) is never copied into the compiled-binary payload at all -- it's a separate distribution channel, irrelevant to this payload's node_modules footprint. - Read every apps/cli/scripts/*.cjs sidecar script (copied into the payload via CLI_RUNTIME_SIDECAR_ENTRIES, run as child processes outside the main binary): only node_pty_relay.cjs requires a node_modules package by name, and it's node-pty/@homebridge/node-pty-prebuilt-multiarch -- already handled as Bun externals. - Audited apps/cli/src for genuinely dynamic (non-literal) require()/ import() calls that Bun's static analyzer can't resolve: found exactly one, in createLocalTransformersEmbeddingsProvider.ts, already accounted for by the existing @huggingface/transformers external. - Cross-checked every excluded package's compiled-in status via `strings` provenance-comment counts on the real binary. - Live-tested the riskiest case empirically: drove `happier auth login` through a real pty with ink deleted from the on-disk node_modules copy -- the AuthSelector terminal UI still rendered correctly (arrow-key highlighting, ANSI codes intact), direct proof rather than inference. - Kept `sharp` vendored: unlike everything else, it does a runtime- constructed require() of a platform-specific native .node binding, structurally identical to why node-pty is already external -- Bun's static analyzer categorically cannot resolve that path, so its on-disk copy is architecturally necessary regardless of tree-shaking. The exclusion is opt-in and scoped to only the compiled CLI binary payload path (copyCliNodeRuntimePayload): every other vendorBundledPackageRuntimeDependencies call site (npm-published tarball builds, apps/stack, packages/relay-server) receives no excludePackageNames argument and continues vendoring every dependency in full, unchanged. Verified via the full local-build installer smoke lifecycle (install -> version -> help -> check -> reinstall -> check -> uninstall) against a freshly-compiled binary -- all steps passed -- and via a real release build (node scripts/pipeline/release/build-cli-binaries.mjs) measured end to end: installed: 990.7MB -> 430MB (-57%) tarball: 222.5MB -> 99.2MB (-55%) node_modules alone dropped from 434MB to 186MB, of which 155MB is @huggingface/transformers (the ONNX runtime + local-embeddings model code, genuinely native/necessary) -- nearly everything else non-native collapsed to a few MB of workspace bundles and PTY native bindings.
…ndencies The compiled-binary payload also vendors apps/cli's internal @happier-dev/* workspace packages via a separate function, bundleWorkspacePackageWithRuntimeDependencies, which never received the excludePackageNames treatment added for apps/cli's own dependencies in the prior commit. Each workspace bundle's own runtime dependencies were therefore still vendored in full even where the bundle's first-party code (and by the same logic, its own dependencies) is already compiled into the binary. Verified per-package, not as a blanket policy -- and specifically re-verified using distinctive exported symbol names rather than package-name strings alone, since a package-name-only search is a proven false-negative trap (@happier-dev/protocol itself showed zero package-name hits in an earlier check, despite its real exports appearing 20 times): - @happier-dev/protocol's own nested @noble/hashes, base64-js, tweetnacl, and zod-to-json-schema: confirmed compiled in via distinctive export/error-string matches, confirmed empirically by deleting them from a real payload and running --version/--help/doctor/status/daemon status/auth request --json (which genuinely exercises a live tweetnacl crypto operation) with no MODULE_NOT_FOUND. - The other 4 bundles (cli-common, agents, release-runtime, connection-supervisor, transfers) declare no external runtime dependencies of their own, so nothing to exclude for them specifically. Found and deliberately did NOT touch a real exception: @happier-dev/cli-common ships root-level (non-dist/) files -- expandHomeDirPath.cjs and others -- that apps/cli/scripts/claude_launcher_runtime.cjs, a real production sidecar spawned as a separate process outside the compiled binary, requires directly by path. Confirmed load-bearing by reproducing the exact MODULE_NOT_FOUND that results from deleting them. These must keep being vendored regardless of what Bun compiles into the main binary process. Also deliberately left apps/cli's own top-level tweetnacl dependency unexcluded in this commit (a known follow-up, already confirmed safe by the same evidence) to keep this diff and the prior commit's exclusion list independently reviewable. Verified via the full local-build installer smoke lifecycle and a real release build, including a direct invocation of the load-bearing sidecar script against the built payload to confirm the kept exception still works: installed: 430MB -> 428MB tarball: 99.2MB -> 98.9MB
…coping Three fixes from automated review feedback on the open PR: - Greptile: the name@version dedup symlink (added in an earlier commit) used symlinkSync unconditionally. Directory symlinks require elevated privileges or Developer Mode on Windows and can throw EPERM/EACCES on an ordinary build host, which would abort vendoring and artifact production for the Windows release target. Wrapped in try/catch with a real-copy fallback on EPERM/EACCES/ENOSYS. - CodeRabbit: the same dedup logic trusted name@version alone as proof two resolved package directories are identical, with no check that their actual contents match. Added areDirectoryTreesEquivalent, a lightweight (file path + size, not full content hash) structural comparison, and gated the symlink on it -- a mismatch falls through to a normal copy instead of symlinking to the wrong content. Every dedup entry actually added to DUPLICATE_VENDORED_PACKAGE_DIR_PATTERNS in a prior commit was already manually verified byte-identical via diff -rq, so this is a safety net for future/automatic cases, not a fix for an observed bug. - CodeRabbit: prunePackageDistDualFormatDir's package-dist matching in prunePackagedTreeDirectory checked entry.name === 'package-dist' at any depth in the tree walk, not just the actual staged payload root. No vendored package in the current tree happens to ship a directory with that name, so this was latent rather than an observed bug, but a future dependency could collide with it and have its own .cjs files incorrectly stripped. Threaded a stageRootDir param through the recursive walk and scoped the check to only fire when directoryPath === stageRootDir. Added regression tests for all three: a mismatched-content dedup case that must NOT symlink, and a nested same-named package-dist directory that must survive pruning untouched. Verified via the full local-build installer smoke lifecycle against a freshly-compiled binary -- all steps passed.
The existing dedup tests covered "same name@version, same content" (dedupes) and "same name@version, different content" (falls through to a copy), but not "same package name, different version" -- a name-only dedup bug would have passed both existing tests undetected. Added a diamond-dependency fixture with shared-dep@1.2.3 and shared-dep@2.0.0 at different nesting depths, asserting both are vendored as independent real directories with no symlink either direction. Declined the review's other nitpick (destructure the typed export instead of casting through Record<string, unknown> in the two new test blocks) -- that cast pattern is this file's existing convention from before this PR (present since the file's earliest tests), and changing it selectively for only the newly-added blocks would leave the file inconsistent; a full-file style pass is a separate, unrelated change.
Miista
force-pushed
the
fix/prune-onnxruntime-node-foreign-platform-binaries
branch
from
August 5, 2026 14:38
46c0fef to
ae3a2ae
Compare
✅ Action performedReviews resumed. |
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
packages/cli-common/src/workspaces/index.ts (1)
557-566: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winMake the equivalence walk tolerant of broken symlinks.
statSync(entryPath)follows the link. If a source package tree contains a dangling symlink,statSyncthrowsENOENTand the whole vendoring run fails. The copy path usescopyDirSafeSync(..., { dereference: true }), so the walk is the only place that can abort on this input.A safe fallback is to treat an unstattable entry as a mismatch marker instead of throwing. That keeps the dedupe decision conservative: the pair falls through to a normal copy.
Note also that a symlink pointing at a directory is recorded as a single entry with the target directory's stat size, so its subtree is not compared. Both sides are treated the same way, so this only weakens the check; it does not invert it.
♻️ Proposed hardening for unstattable entries
if (!entry.isFile() && !entry.isSymbolicLink()) continue; - const size = statSync(entryPath).size; - result.set(relative(rootDir, entryPath), size); + // A dangling symlink must not abort vendoring. Record a sentinel so the pair is treated as + // non-equivalent and falls through to a normal copy. + let size: number; + try { + size = statSync(entryPath).size; + } catch { + size = Number.NaN; + } + result.set(relative(rootDir, entryPath), size);
Number.NaNnever equals itself, sofilesB.get(relPath) !== sizetreats such an entry as a mismatch on either side.🤖 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 `@packages/cli-common/src/workspaces/index.ts` around lines 557 - 566, Update the equivalence walk around the readdirSync/statSync loop to tolerate entries that cannot be statted, including dangling symlinks. Catch statSync failures and record Number.NaN as the entry size so comparisons conservatively produce a mismatch, while preserving the existing handling for regular files and symlinks.packages/cli-common/src/workspaces/index.test.ts (2)
396-441: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider extracting the shared diamond fixture.
The three dedupe tests build near-identical scaffolding. Only three values vary: the nested package version, the nested file content, and the version
consumer-depdeclares. A local helper would remove roughly 150 lines of duplication and make each test state its variable clearly.function writeDiamondFixture(rootDir: string, nested: Readonly<{ declaredVersion: string; version: string; content: string; }>): Readonly<{ srcPackageDir: string }> { /* ... */ }This is optional. The current tests are readable as written.
Also applies to: 481-527
🤖 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 `@packages/cli-common/src/workspaces/index.test.ts` around lines 396 - 441, Optionally extract the repeated diamond dependency fixture setup from the three dedupe tests into a local writeDiamondFixture helper. Have it accept nested.declaredVersion, nested.version, and nested.content, return the srcPackageDir, and update each test to provide only those varying values while preserving the existing fixture behavior.
138-141: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winImport the vendoring functions with their real types instead of re-declaring signatures inline.
All seven new tests reach the exported functions through
(workspaceModule as Record<string, unknown>)and then cast to a locally written call signature. This discards the real exported types. IfexcludePackageNameswere renamed or retyped inindex.ts, these tests would still compile and would silently pass a property the production code ignores. Line 1 already uses a typed static import for other symbols from the same module, so the typed path is the canonical one here.Adding both symbols to the existing static import removes the
Record<string, unknown>lookup, thetoBeTypeOf('function')guard, and the inline parameter type in each test.
packages/cli-common/src/workspaces/index.test.ts#L138-L141: remove the lookup and thetoBeTypeOfguard; call the statically importedbundleWorkspacePackageWithRuntimeDependenciesdirectly at lines 178-183.packages/cli-common/src/workspaces/index.test.ts#L199-L202: same change; drop the inline cast at lines 232-236.packages/cli-common/src/workspaces/index.test.ts#L293-L296: same change; drop the inline cast at lines 350-354.packages/cli-common/src/workspaces/index.test.ts#L387-L390: same change; drop the inline cast at lines 445-449.packages/cli-common/src/workspaces/index.test.ts#L473-L476: same change; drop the inline cast at lines 531-535.packages/cli-common/src/workspaces/index.test.ts#L560-L563: use the statically importedvendorBundledPackageRuntimeDependencies; drop the inline cast at lines 596-601.packages/cli-common/src/workspaces/index.test.ts#L616-L619: same change; drop the inline cast at lines 645-649.If a test intentionally needs a fresh module instance, keep the dynamic import but type it, for example
const { bundleWorkspacePackageWithRuntimeDependencies } = await import('./index');. That preserves module-cache reset while keeping the real signatures.As per coding guidelines: "Prefer satisfies, explicit interfaces, typed fixtures, and canonical schemas over casting."
🤖 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 `@packages/cli-common/src/workspaces/index.test.ts` around lines 138 - 141, Replace the dynamically imported Record<string, unknown> lookups and inline casts in packages/cli-common/src/workspaces/index.test.ts at lines 138-141, 199-202, 293-296, 387-390, 473-476, 560-563, and 616-619 with typed static imports of bundleWorkspacePackageWithRuntimeDependencies and vendorBundledPackageRuntimeDependencies, removing the toBeTypeOf guards and calling the real exported functions directly; if a fresh module instance is required, destructure the functions from a typed dynamic import instead.Source: Coding guidelines
🤖 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 `@packages/cli-common/src/workspaces/index.test.ts`:
- Around line 376-378: Update the symlink assertion in the vendored dependency
test to compare resolvedLinkTarget directly with vendoredSharedDepDir, verifying
the exact dedupe target rather than only its basename. Remove the basename
import if it is no longer used elsewhere in the test.
In `@scripts/pipeline/release/lib/binary-release.package-dist-cjs-prune.test.mjs`:
- Around line 80-83: Extend the nested vendored package setup in the scope
regression test to create both .d.mts and .d.cts declaration files alongside the
existing index files, then assert that all nested files remain after
sanitization. Update the assertions around the nested package case to cover the
complete file set without testing implementation details.
---
Nitpick comments:
In `@packages/cli-common/src/workspaces/index.test.ts`:
- Around line 396-441: Optionally extract the repeated diamond dependency
fixture setup from the three dedupe tests into a local writeDiamondFixture
helper. Have it accept nested.declaredVersion, nested.version, and
nested.content, return the srcPackageDir, and update each test to provide only
those varying values while preserving the existing fixture behavior.
- Around line 138-141: Replace the dynamically imported Record<string, unknown>
lookups and inline casts in packages/cli-common/src/workspaces/index.test.ts at
lines 138-141, 199-202, 293-296, 387-390, 473-476, 560-563, and 616-619 with
typed static imports of bundleWorkspacePackageWithRuntimeDependencies and
vendorBundledPackageRuntimeDependencies, removing the toBeTypeOf guards and
calling the real exported functions directly; if a fresh module instance is
required, destructure the functions from a typed dynamic import instead.
In `@packages/cli-common/src/workspaces/index.ts`:
- Around line 557-566: Update the equivalence walk around the
readdirSync/statSync loop to tolerate entries that cannot be statted, including
dangling symlinks. Catch statSync failures and record Number.NaN as the entry
size so comparisons conservatively produce a mismatch, while preserving the
existing handling for regular files and symlinks.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 8cebc764-b986-438b-ac7e-50f77fa4afb7
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (12)
apps/cli/package.jsonpackages/cli-common/src/componentArtifacts/buildCliBinaryArtifactPayload.tspackages/cli-common/src/workspaces/index.test.tspackages/cli-common/src/workspaces/index.tsscripts/pipeline/release/lib/binary-release.bare-fs-and-ps-list-prune.test.mjsscripts/pipeline/release/lib/binary-release.duplicate-vendored-package-prune.test.mjsscripts/pipeline/release/lib/binary-release.exhaustive-duplicate-resweep-prune.test.mjsscripts/pipeline/release/lib/binary-release.huggingface-transformers-dist-prune.test.mjsscripts/pipeline/release/lib/binary-release.mcp-sdk-unused-deps-prune.test.mjsscripts/pipeline/release/lib/binary-release.mjsscripts/pipeline/release/lib/binary-release.onnxruntime-node-prune.test.mjsscripts/pipeline/release/lib/binary-release.package-dist-cjs-prune.test.mjs
🚧 Files skipped from review as they are similar to previous changes (8)
- apps/cli/package.json
- scripts/pipeline/release/lib/binary-release.exhaustive-duplicate-resweep-prune.test.mjs
- scripts/pipeline/release/lib/binary-release.onnxruntime-node-prune.test.mjs
- scripts/pipeline/release/lib/binary-release.duplicate-vendored-package-prune.test.mjs
- packages/cli-common/src/componentArtifacts/buildCliBinaryArtifactPayload.ts
- scripts/pipeline/release/lib/binary-release.mjs
- scripts/pipeline/release/lib/binary-release.bare-fs-and-ps-list-prune.test.mjs
- scripts/pipeline/release/lib/binary-release.mcp-sdk-unused-deps-prune.test.mjs
…rget, test coverage Three more review findings, verified real before fixing: - The dedup content-equivalence check (areDirectoryTreesEquivalent, added in the prior review-feedback commit) called statSync directly while walking a resolved package tree. A dangling symlink -- a realistic node_modules artifact (broken .bin shim, an optional dependency that failed to install) -- throws ENOENT and would abort the entire vendoring run. Wrapped in try/catch, recording NaN as a sentinel size so the entry always compares as a mismatch (NaN !== NaN) and the pair conservatively falls through to a normal copy instead of crashing. Added a dedicated test that reproduces the crash on the old code (confirmed by temporarily reverting the fix and re-running it) and passes once fixed. - The diamond-dependency dedup test asserted the symlink's resolved target only by basename, with a comment explaining the link is captured inside a not-yet-renamed atomic staging tree. That's true of the implementation but irrelevant to what the test observes: by the time bundleWorkspacePackageWithRuntimeDependencies returns, destPackageDir is already the final real path, so the test can and should assert exact path equality. Tightened the assertion and removed the now-unused `basename` import. - Extended the nested-package-dist scoping regression test to also create .d.mts/.d.cts files alongside .cjs, asserting the complete file set survives at a non-root nesting depth, not just the .cjs case. Declined two lower-priority nitpicks in the same review (extracting a shared diamond-fixture test helper, and a broader typed-import refactor across all seven vendoring tests) as explicitly optional/low-value per the review itself and not worth the added indirection or file-wide convention change. Verified via cli-common's vitest suite (15/15), the full packaging pipeline suite (293/293), a clean tsc --noEmit, and the real local-build installer smoke lifecycle -- all steps passed.
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.
Summary
The packaged CLI (Homebrew/GitHub-release tarball) shipped roughly twice
what a single-platform install needs. On the darwin-arm64 dev build this
drops the installed footprint from 991MB → 428MB and the compressed
tarball from 222.5MB → 98.9MB — no functional changes, only removing
bytes that were never reachable on the packaging target, genuinely
duplicated on disk, or already compiled directly into the binary. Verified
at every stage with a full
install → --version → --help → check → reinstall → check → uninstalllifecycle against a real locally-compiledbinary, matching CI's
Installer Smokejobs exactly, plus real end-to-endrelease builds (
node scripts/pipeline/release/build-cli-binaries.mjs).The big one: stop double-shipping what Bun already compiled in
apps/clicompiles to a single Bun executable viabun --compile, whichtree-shakes reachable source directly into the binary (confirmed via
strings: reachable files carry provenance comments or their distinctiveexported symbols appear literally in the binary; unreachable ones don't).
Only 3 packages containing native code Bun can't compile in
(
@huggingface/transformers,node-pty,@homebridge/node-pty-prebuilt-multiarch)are declared
--external. But the payload-assembly code unconditionallyvendored every declared dependency — of
apps/cliitself, andseparately of its internal
@happier-dev/*workspace packages — as loosefiles regardless. Most non-native dependencies were present twice:
compiled into the 76MB binary, and duplicated as a full loose
node_modulestree alongside it.
Verified per-package before excluding anything, not as a blanket policy:
apps/cli/bin/*.mjs(npm-publish entrypoint) is never copied into thecompiled-binary payload — a separate distribution channel entirely.
apps/cli/scripts/*.cjssidecar script (the onlyapps/cli/scripts/**files actually shipped, run as child processesoutside the main binary): only
node_pty_relay.cjsrequires anode_modulespackage by name (the two already-external PTY packages),and
claude_launcher_runtime.cjsrequires@happier-dev/cli-common'sroot-level (non-
dist/) files directly by path.require()/import()callsBun's static analyzer can't resolve: found exactly one, already handled
by the
@huggingface/transformersexternal.strings,searching for distinctive exported symbol names, not just the
package-name string — a package-name-only search is a proven
false-negative trap:
@happier-dev/protocolshowed zero package-namehits on an initial check despite its real exports appearing 20 times.
happier auth loginthrough a real pty with
inkdeleted from disk — the AuthSelectorterminal UI still rendered correctly (arrow-key highlighting, ANSI codes
intact). Separately ran
auth request --json(which mints a realtweetnacl-box keypair) with both
apps/cli's and@happier-dev/protocol'snested
tweetnaclcopies deleted — succeeded with no error.sharpvendored — unlike everything else, it does aruntime-constructed
require()of a platform-specific native.nodebinding, the same reason
node-ptyis external. Bun's static analyzercategorically can't resolve that path.
@happier-dev/cli-common's root-level (non-dist/) filesvendored — confirmed load-bearing by reproducing the exact
MODULE_NOT_FOUNDthat results from deleting them;claude_launcher_runtime.cjsrequires them directly by path, executing outside the compiled binary.
Both exclusions are opt-in and scoped to only the compiled CLI binary
payload path — every other vendoring call site (npm-published tarball
builds,
apps/stack,packages/relay-server, the pre-compileBun-tree-shaking source snapshot) receives no exclusion set and continues
vendoring every dependency in full, unchanged.
node_modulesalone dropped from 434MB to under 190MB, of which ~155MB is@huggingface/transformers(the ONNX runtime + local-embeddings modelcode — genuinely native, necessary) — nearly everything else non-native
collapsed to a few MB of workspace-bundle first-party output and PTY
native bindings.
Removed 4 confirmed-unused apps/cli dependencies
Verified with git-history tracing (not just grep) that these have zero
reachable usage anywhere in the repo:
@fastify/swagger— dead in bothapps/cliand siblingapps/server.@stablelib/base64— added alongside the real base64 helper (plainBuffer.toString('base64')) and never used; its real historical use wasmobile-only, since fully retired there too.
ai(Vercel AI SDK) — added in the same commit as ACP/Gemini backendwork but never referenced by any file in that commit or since.
http-proxy-middleware— a different, unused package confusable withthe actually-used
http-proxy(called directly by the proxy code).Also moved
tmptodevDependencies(test-only usage, no postinstallreachability).
Two candidates from the initial survey were investigated and correctly
rejected:
openapi-types(a mandatory, non-optional peer dependency offastify-type-provider-zod) andreact-devtools-core(ink'speerDependency, conditionally dynamic-imported when
DEV=trueis set inthe environment).
tarwas also considered for adevDependenciesmoveand correctly rejected: it's required by
unpack-tools.cjs, which runsvia
postinstallon real end-user npm installs.Foreign-platform native binaries pruned
Extends
prunePackagedTreeDirectory/sanitizePackagedNodeModulesTree—verified across all 5 CLI targets (linux/darwin × x64/arm64, windows x64):
onnxruntime-node(~208MB → ~31MB kept per target),node-pty/@homebridge/node-pty-prebuilt-multiarch(up to ~58MB),bare-fs/bare-url/bare-os(~3.7MB),ps-list's Windows-only.exehelpers.Unused MCP SDK HTTP-transport dependencies
@modelcontextprotocol/sdkvendorsexpress,express-rate-limit,cors,jose, and a standalonehonofor OAuth-authorization-server / Express-adapter code paths never reached by happier's real SDK entry points —
~12.5MB removed, verified against the compiled binary's
stringsoutput.Does not touch first-party SDK source under
server/auth/(
server/auth/errors.jsis reachable viaclient/auth.jsand covered bya dedicated regression test).
Duplicate packages and npm-only build output
Confirmed byte-identical via
diff -rq:tar,@modelcontextprotocol/sdk,zod(4 copies),archiver-utils,qs,get-intrinsic,readable-stream(4 copies) — ~85MB combined, each with a verified surviving ancestor
reachable via ordinary module resolution. An
ajvduplication pattern wasconsidered and explicitly rejected: no surviving ancestor exists. Also
hardened
vendorRuntimeDependencyTreewith aname@versiondedup map.@huggingface/transformers's owndist/ships browser/CJS/minified/WASMbuild variants; only
dist/transformers.node.mjsis ever resolved via itsexports.node.importcondition — ~41MB pruned.package-dist/(npm-publishdual-format build output) has its
.cjsand.d.mts/.d.ctsfiles pruned.Review feedback addressed
name@versiondedup usedsymlinkSyncunconditionally, which can throwEPERM/EACCESon aWindows host without Developer Mode or elevated privileges, aborting
vendoring entirely. Wrapped in try/catch with a real-copy fallback.
name@versionalonedoesn't prove two resolved package directories are identical. Added
areDirectoryTreesEquivalent(relative file paths + sizes) and gated thesymlink on it — a mismatch now falls through to a real copy instead of
symlinking to the wrong content. Every dedup entry actually shipped in
this PR was already manually verified byte-identical via
diff -rq;this is a safety net for future/automatic cases.
package-distpruning scope (CodeRabbit): the prune matched anydirectory named
package-distat any depth in the tree walk, not justthe actual staged payload root. Threaded a
stageRootDirparam throughthe recursive walk so the check only fires at the true root.
regression test with two different versions of the same package name at
different nesting depths, asserting neither is deduped against the
other — the existing tests only covered "same version, same content" and
"same version, different content."
through
Record<string, unknown>in two test blocks) — that cast is thisfile's pre-existing convention from before this PR; changing it
selectively would leave the file inconsistent.
content-equivalence check's
statSynccall throws on a dangling symlink(a realistic
node_modulesartifact), which would abort vendoringentirely. Wrapped in try/catch with a
NaNsentinel size so such anentry always compares as a mismatch. Added a test that reproduces the
crash on the pre-fix code (confirmed by temporarily reverting the fix)
and passes once fixed.
dedup test asserted the symlink's resolved target only by basename. By
the time the vendoring function returns, the destination is already the
final real path, so tightened the assertion to exact path equality and
removed the now-unused
basenameimport.package-dist-scoping test to cover.d.mts/.d.ctsalongside
.cjs, not just the.cjscase.helper; a broader typed-import refactor across all vendoring tests) —
both explicitly marked optional/low-value in the review itself.
Not included (flagged, not fixed here)
difftbinary (108MB, unstripped): 99.9MB of it is compiled-in constantdata (
__TEXT.__const) thatstripcannot touch — measured strippingthe real binary, only ~1.4MB recoverable.
@types/ps-listmisplaced underdependencies— a dependency-placementissue, out of scope here.
before and after this PR):
@huggingface/transformers'sdist/transformers.node.mjsfails at runtime withCannot find package 'onnxruntime-common'on both Node and Bun against the realcurrently-published dev build — local deep-memory-search is likely
broken independent of this PR.
sharp's native binary also appears tofail loading on darwin-arm64 in the currently-published dev build,
affecting the pet-image-validation feature.
CI status
The
Installer Smoke (Windows/macOS),Typecheck,CLI Tests,UI Tests,Core E2E, andCLI + Server E2Echecks currently fail on this PR, but anexact diff against the latest
dev-branch CI run shows the identicalfailure set already present on
devitself (a Windows minisignarchitecture-selection bug, a
dev-branch-wideERR_MODULE_NOT_FOUNDduring the CLI's own staged-build probe, stale test-wiring metadata for
unrelated files, and V8 heap-OOM crashes in large vitest workers) — none
rooted in this diff.
Installer Smoke (Linux),Binary Smoke,Release Contracts,Shared Package Unit Tests, andServer DB Contract (Postgres)all pass.Test plan
HAPPIER_FEATURE_POLICY_ENV= node --test --test-concurrency=1 scripts/pipeline/**/*.test.mjs— 293/293 passyarn workspace @happier-dev/cli-common vitest run src/workspaces/index.test.ts— 15/15 passyarn workspace @happier-dev/cli-common tsc --noEmit— cleanInstaller Smokejobs exactly, all stepsok: trueclaude_launcher_runtime.cjs) against the real built payload, confirming it still resolves correctlyNote
Prune foreign-platform binaries, duplicate packages, and unused files from the packaged CLI binary
buildCliBinaryArtifactPayload.tsto skip vendoring specified direct and workspace-bundled dependencies into the compiled CLI binary payload.vendorRuntimeDependencyTreeinworkspaces/index.ts: identicalname@versionpackages with equivalent directory content are symlinked instead of copied.sanitizePackagedNodeModulesTreeinbinary-release.mjswith several new pruning passes: removes non-target native prebuilds (onnxruntime-node, node-pty, bare-fs/url/os), strips Windows-onlyps-listexecutables on non-Windows targets, prunes@huggingface/transformers/distto node-only outputs, removes known nested duplicate vendored packages, and deletes unused@modelcontextprotocol/sdkvendored dependencies (express, cors, jose, hono, etc.)..cjs,.d.mts, and.d.ctsfiles from the staged root package-dist, retaining only.mjsfiles.Macroscope summarized ce2bcb8.
Summary by CodeRabbit