fix: declare onnxruntime-common runtime dependency#2074
Merged
magyargergo merged 8 commits intoJun 8, 2026
Merged
Conversation
|
@xianzuyang9-blip is attempting to deploy a commit to the NexusCore Team on Vercel. A member of the Team first needs to authorize it. |
Contributor
✨ PR AutofixFound fixable formatting / unused-import issues across 16 changed lines. Comment |
magyargergo
requested changes
Jun 8, 2026
Contributor
CI Report✅ All checks passed Pipeline Status
Test Results
✅ All 10693 tests passed 16 test(s) skipped — expand for details
Code CoverageTests
📋 View full run · Generated by CI |
magyargergo
approved these changes
Jun 8, 2026
magyargergo
added a commit
to magyargergo/GitNexus
that referenced
this pull request
Jun 10, 2026
…dlx (abhigyanpatwari#307) `@huggingface/transformers` does a bare `import 'onnxruntime-common'` from its shipped `dist/transformers.node.mjs`, but never declares onnxruntime-common in its own `dependencies`. npm's flat node_modules (and pnpm with hoisting) place it on transformers' resolution path by accident; pnpm's isolated store only links a package's declared deps into its scope, so under pnpm-strict / `pnpm dlx` / `pnpx` the import dies with ERR_MODULE_NOT_FOUND before `analyze --embeddings` can run. Declaring onnxruntime-common in gitnexus' own deps (abhigyanpatwari#2074) does not fix this under pnpm: Node resolves the bare specifier from transformers' module scope, not ours, and overrides/resolutions can only re-version an existing edge, never add the missing one (verified against a real `hoist=false` install — the declaration only changes which version wins the hoist, never whether the import resolves). Fix: install a synchronous, in-thread ESM resolution hook (`module.registerHooks`) right before the lazy transformers import that redirects `onnxruntime-common` to the copy gitnexus depends on — but only when the default resolver fails. On npm / hoisted layouts the default resolver succeeds first and the hook never fires, so working setups are unchanged. The hook only intercepts the exact `onnxruntime-common` specifier on failure, so it can never mask an unrelated resolution error; onnxruntime-node's native binding still loads normally from transformers' own scope. `registerHooks` (sync, in-thread, single inline closure) is preferred over the older `module.register` (async, off-thread, now deprecated — DEP0205, removed in Node 26): the redirect is a one-line conditional that needs no worker thread, no separate hook module, and no `data` marshalling. It is available on Node >= 22.15; on older runtimes the helper is a graceful no-op (the gitnexus engines floor is >= 22.0.0, and the import still resolves on hoisted layouts there). Chosen over bundling transformers (the build is tsc-only, and transformers carries native onnxruntime-node + WASM onnxruntime-web assets that bundle poorly). Installation is idempotent, best-effort, and lazy — only on the local-embedding path, so it never affects analysis, the parse workers, or HTTP embedding mode. Validated end-to-end: the compiled resolver fixes a real pnpm `hoist=false` transformers install (ERR_MODULE_NOT_FOUND -> resolved). The separate `@ladybugdb/core` native-binary path under pure `pnpm dlx` is unchanged (abhigyanpatwari#1967 handles that gracefully). Refs abhigyanpatwari#307, abhigyanpatwari#2069 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
magyargergo
added a commit
that referenced
this pull request
Jun 10, 2026
…dlx (#307) (#2139) * fix(embeddings): resolve onnxruntime-common under pnpm-strict / pnpm dlx (#307) `@huggingface/transformers` does a bare `import 'onnxruntime-common'` from its shipped `dist/transformers.node.mjs`, but never declares onnxruntime-common in its own `dependencies`. npm's flat node_modules (and pnpm with hoisting) place it on transformers' resolution path by accident; pnpm's isolated store only links a package's declared deps into its scope, so under pnpm-strict / `pnpm dlx` / `pnpx` the import dies with ERR_MODULE_NOT_FOUND before `analyze --embeddings` can run. Declaring onnxruntime-common in gitnexus' own deps (#2074) does not fix this under pnpm: Node resolves the bare specifier from transformers' module scope, not ours, and overrides/resolutions can only re-version an existing edge, never add the missing one (verified against a real `hoist=false` install — the declaration only changes which version wins the hoist, never whether the import resolves). Fix: install a synchronous, in-thread ESM resolution hook (`module.registerHooks`) right before the lazy transformers import that redirects `onnxruntime-common` to the copy gitnexus depends on — but only when the default resolver fails. On npm / hoisted layouts the default resolver succeeds first and the hook never fires, so working setups are unchanged. The hook only intercepts the exact `onnxruntime-common` specifier on failure, so it can never mask an unrelated resolution error; onnxruntime-node's native binding still loads normally from transformers' own scope. `registerHooks` (sync, in-thread, single inline closure) is preferred over the older `module.register` (async, off-thread, now deprecated — DEP0205, removed in Node 26): the redirect is a one-line conditional that needs no worker thread, no separate hook module, and no `data` marshalling. It is available on Node >= 22.15; on older runtimes the helper is a graceful no-op (the gitnexus engines floor is >= 22.0.0, and the import still resolves on hoisted layouts there). Chosen over bundling transformers (the build is tsc-only, and transformers carries native onnxruntime-node + WASM onnxruntime-web assets that bundle poorly). Installation is idempotent, best-effort, and lazy — only on the local-embedding path, so it never affects analysis, the parse workers, or HTTP embedding mode. Validated end-to-end: the compiled resolver fixes a real pnpm `hoist=false` transformers install (ERR_MODULE_NOT_FOUND -> resolved). The separate `@ladybugdb/core` native-binary path under pure `pnpm dlx` is unchanged (#1967 handles that gracefully). Refs #307, #2069 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(embeddings): version-match the onnxruntime-common redirect target (#307) Prefer the onnxruntime-common that onnxruntime-node (the native binding transformers actually loads) depends on, so the redirected copy is version- matched to that binding even under `pnpm dlx` — where gitnexus' npm-style `overrides` block does not apply, because it is honoured only from a root manifest and gitnexus is a transitive dependency there. The walk resolves transformers' main entry (not its `exports`-blocked package.json) -> onnxruntime-node -> its onnxruntime-common, and falls back to gitnexus' own direct dependency when the chain can't be walked. Also corrects the doc comment that claimed the gitnexus copy was already "version-aligned". Addresses a PR #2139 tri-review finding (P2). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(embeddings): narrow the onnxruntime-common resolve fallback to absence errors (#307) The resolve closure's `catch` swallowed every error from `nextResolve` and redirected, which would silently paper over a genuinely present-but-broken onnxruntime-common install. Only substitute gitnexus' copy when the specifier is actually absent (ERR_MODULE_NOT_FOUND, or ERR_PACKAGE_PATH_NOT_EXPORTED for an exports-broken copy); rethrow anything else. Adds a test that an unrelated error code rethrows. Addresses a PR #2139 tri-review finding (P3). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(embeddings): cover the onnxruntime-common resolver best-effort swallow path (#307) The outer try/catch in ensureOnnxRuntimeCommonResolvable() was untested. A throwing registerHooks spy drives it; the call must not throw (initEmbedder does not guard the return, so a throw would break `analyze --embeddings`). The vitest quirk that surfaced an earlier attempt applies to throwing mock factories, not a throwing spy implementation, so this is testable cleanly. Addresses a PR #2139 tri-review finding (P2). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(embeddings): tighten the onnxruntime-common redirect-URL assertion (#307) `/^file:\/\/.*onnxruntime-common/` matched the substring anywhere, so a lookalike path (e.g. `/x/onnxruntime-common-fake/`) would pass. Require an actual `/node_modules/onnxruntime-common/...js` segment so the assertion proves the redirect resolves to the real package, not just a string match. Addresses a PR #2139 tri-review finding (P3). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(embeddings): drop the no-op __resetOnnxRuntimeCommonResolverForTests seam (#307) The test helper reloads the resolver via vi.resetModules() + a fresh import(), which already re-initialises the module-level one-shot `attempted` flag to false. The __reset export it then called was therefore a no-op. Remove the test-only export and its call; isolation now rests solely on vi.resetModules(). Addresses a PR #2139 tri-review finding (P3). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(embeddings): correct the onnxruntime-common resolver isolation comment (#307) The doc comment claimed the hook "never affects other tools' resolution". Once installed, `module.registerHooks` is process-global and its resolve closure runs for every subsequent resolution — it passes them all through untouched and only substitutes the exact `onnxruntime-common` specifier on genuine absence, at a cost of one string comparison. Also note `registerHooks` is @experimental and requires Node >= 22.15 (graceful no-op below that). Comment-only. Addresses a PR #2139 tri-review finding (P3). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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
2 participants
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
Why
@huggingface/transformers@4.2.0 imports onnxruntime-common from its Node entrypoint, but does not declare it directly. npm hoisting can hide that, while pnpm strict/global-store installs can fail with ERR_MODULE_NOT_FOUND before gitnexus analyze can run. Declaring the package directly in GitNexus makes the runtime module resolvable for consumers today.
Verification
pm install --package-lock-only --ignore-scripts in gitnexus/
pm install --ignore-scripts in gitnexus/
px vitest run test/unit/embedding-package-metadata.test.ts => 1 passed
Notes: broader local
pm run build in this Windows source-archive environment is blocked by existing optional/native workspace setup issues ( ree-sitter-dart/ ree-sitter-swift declarations and LadybugDB native loading), unrelated to this package metadata change.
Fixes #2069