fix(native): prefer local dev binary over npm package in load order#1389
Conversation
When a locally compiled `crates/codegraph-core/*.node` binary exists it is now loaded before the published npm platform package. This ensures that Rust changes (like the prototype-method extraction from PR #1339) are picked up immediately in development without waiting for a new npm release. Load order is now: 1. NAPI_RS_NATIVE_LIBRARY_PATH env var — explicit override 2. crates/codegraph-core/<platform>.node — freshly compiled dev binary 3. @optave/codegraph-<platform> npm pkg — published production binary Closes #1361
|
Claude finished @carlos-alm's task —— View job PR Review —
|
Greptile SummaryThis PR rewrites the native addon load order in
Confidence Score: 5/5Safe to merge. All changes are well-scoped, prior review feedback has been incorporated, and new behaviour is covered by integration tests. The native loader changes are straightforward and the failure semantics are consistent: both the env-var and local-binary paths halt on load failure rather than silently falling through. The incremental builder additions mirror the existing full-build path and are exercised by the new integration test. No regressions are visible in the parser or extractor changes. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[loadNative called] --> B{_cached !== undefined?}
B -- yes --> C[return _cached]
B -- no --> D{NAPI_RS_NATIVE_LIBRARY_PATH set?}
D -- yes, success --> E[_cached = module\nreturn module]
D -- yes, fail --> F[warn log\n_cached = null\nreturn null]
D -- no --> G{localFile exists\nfor platform?}
G -- file absent --> I[try npm package]
G -- file present, success --> H[_cached = module\nreturn module]
G -- file present, fail --> J[warn log\n_cached = null\nreturn null]
I -- success --> K[_cached = module\nreturn module]
I -- fail --> L[debug log\n_cached = null\nreturn null]
I -- unsupported platform --> M[_loadError = platform error\n_cached = null\nreturn null]
Reviews (17): Last reviewed commit: "fix(native): warn and halt on local bina..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| // 1. Explicit path override — highest priority. | ||
| const envPath = process.env.NAPI_RS_NATIVE_LIBRARY_PATH; | ||
| if (envPath) { | ||
| try { | ||
| _cached = _require(envPath) as NativeAddon; | ||
| debug(`loadNative: loaded from NAPI_RS_NATIVE_LIBRARY_PATH: ${envPath}`); | ||
| return _cached; | ||
| } catch (err) { | ||
| _loadError = err as Error; | ||
| debug(`loadNative: NAPI_RS_NATIVE_LIBRARY_PATH load failed: ${toErrorMessage(err as Error)}`); | ||
| } | ||
| } |
There was a problem hiding this comment.
When NAPI_RS_NATIVE_LIBRARY_PATH is set but the load fails (bad path, ABI mismatch, corrupt file, etc.), the code catches the error, logs a debug line, and continues to the local binary and npm package. An explicit override failing silently and loading a different binary is hard to diagnose — the operator set the variable intentionally and has no indication it was ignored. This path should either return null immediately (preserving _loadError so getNative() surfaces the real reason) or at least log at warning level rather than debug. As written, _loadError is also left pointing at the env-var failure even when a subsequent fallback succeeds, creating a misleading error state if _loadError is ever inspected externally.
There was a problem hiding this comment.
Fixed — when NAPI_RS_NATIVE_LIBRARY_PATH is set but fails to load, we now: (1) log at warn level instead of debug, (2) set _cached = null and return immediately, so no other loader is tried. This ensures an operator who set the variable explicitly gets a visible warning and a clear error from getNative() rather than silently loading a different binary. (commit 183f78e)
| /** | ||
| * Map of (platform-arch[-libc]) → locally compiled binary filename. | ||
| * Used as a dev-mode fallback when the npm package is not installed, | ||
| * e.g. when working with Rust changes that haven't been published yet. | ||
| */ |
There was a problem hiding this comment.
PLATFORM_LOCAL_BINARIES says it is a "dev-mode fallback when the npm package is not installed", but the load order now puts it before the npm package — it is checked regardless of whether the npm package is installed. The comment misrepresents the new priority semantics.
| /** | |
| * Map of (platform-arch[-libc]) → locally compiled binary filename. | |
| * Used as a dev-mode fallback when the npm package is not installed, | |
| * e.g. when working with Rust changes that haven't been published yet. | |
| */ | |
| /** | |
| * Map of (platform-arch[-libc]) → locally compiled binary filename. | |
| * Checked before the npm package so that locally compiled Rust changes | |
| * are picked up immediately in development without publishing a new release. | |
| */ |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Fixed — updated the JSDoc to 'Checked before the npm package so that locally compiled Rust changes are picked up immediately in development without publishing a new release.' (commit 183f78e)
Codegraph Impact Analysis7 functions changed → 115 callers affected across 40 files
|
|
Addressed Claude's review feedback:
|
…+ static block + field def
- Add `(class name: ...)` query patterns for JS/TS class expressions so that
`return class Foo extends Bar { ... }` records the extends relationship in
ctx.classes — previously only class_declaration was captured, leaving class
expressions invisible to resolveThisDispatch.
- Add `class_static_block` → `ClassName.<static>` synthetic method definition
in both query path (extractClassMembersWalk) and walk path (walkJavaScriptNode).
Calls inside `static { super.f(); }` blocks are now attributed to a method-kind
node so the CHA parents map can resolve `super.f()` to the parent class.
- Add `field_definition`/`public_field_definition` → `ClassName.fieldName` method
definition when the field value is an arrow function or function expression.
`static f = () => { ... }` becomes a resolvable `A.f` node so
`resolveThisDispatch` can emit the `B.<static> → A.f` edge.
- Mirror all three changes in the native Rust extractor for parity.
- Add 6 parser unit tests and import Jelly micro-test fixtures for super,
super2, super3, super4, super5 as ground-truth benchmarks.
Benchmark result: super fixture 31% → 38% recall (B.<static> → A.f now resolved).
docs check acknowledged
Closes #1377
…llbacks into buildCallEdges After resolveCallTargets returns empty: 1. Retry with class-qualified name (ClassName.method) for this-receiver calls 2. Resolve via Object.defineProperty accessor receiver typeMap or same-file lookup These two blocks existed in buildFileCallEdges (build-edges.ts) but were missing from the incremental path, causing divergence between full and watch-mode builds. Also adds the missing callerName argument to resolveCallTargets. Closes #1384
PLATFORM_PACKAGES[platformKey] was inlined in loadNative() while getNativePackageVersion() used resolvePlatformPackage(). Any future change to the lookup (fallback key, default value) would only apply to one site. Call resolvePlatformPackage() consistently in both places.
|
Addressed the remaining Greptile suggestion from the summary:
|
|
Scope note: The branch unexpectedly contained two commits from other in-flight PRs (#1399, #1401) that were added by a prior session before this sweep started. Specifically:
These could not be removed without a force-push, so they were merged in as-is. The net diff against main now includes those changes alongside the intended native.ts load-order fix. Reviewers should be aware that merging this PR will also land part of the work from those two PRs. If this is undesirable, the branch owner should force-push a clean branch from |
…ame in buildCallEdges Incremental rebuilds using buildCallEdges (incremental.ts) were missing two things needed for Phase 8.3f scoped-key resolution (#1358 / #1369): 1. The typeMap was not seeded with callee::restName entries from objectRestParamBindings × paramBindings. Without this seeding the scoped key `callee::restName` (e.g. `f2::rest`) is absent from the map, so resolveByMethodOrGlobal's third fallback (`typeMap.get(callerName::receiver)`) has nothing to find and the rest-param call goes unresolved. 2. caller.callerName was not passed to resolveCallTargets, so even if the scoped key was present in the typeMap (e.g. from WASM extraction), the `${callerName}::${effectiveReceiver}` lookup in resolveByMethodOrGlobal never fired. Fix: mirror what buildObjectRestParamPostPass (native full-build post-pass) and buildCallEdgesJS (WASM full-build path) already do: - Compute restNameCallees to know how many callees share a rest name. - Seed typeMap[callee::restName] for each objectRestParamBinding × paramBinding pair. - Also seed the unscoped key when only one callee uses that rest name, so resolution still works when callerName is null (findCaller couldn't match). - Pass caller.callerName to resolveCallTargets (already present from #1389). Also syncs call-resolver.ts and build-edges.ts with the scoped-key changes from PR #1368 (merged to main separately), which this branch was missing. docs check acknowledged Closes #1369
…or static blocks (#1389) The merge conflict resolution in 8d5ba14 dropped two changes from ca3123f: 1. The 'class' case in walkJavaScriptNode's switch — class expressions like 'return class Child extends Parent { ... }' were never dispatched to handleClassDecl, so ctx.classes remained empty and no extends relationship was recorded. 2. handleStaticBlock used kind: 'function' instead of kind: 'method', so the CHA parents map could not walk up to the parent class for super.method() resolution. Also update the existing test that asserted kind: 'function'.
|
Fixed CI failures introduced during merge conflict resolution:
Also merged origin/main to resolve branch divergence (3 commits: jelly-micro fixture import, Phase 8.3f typeMap scoping, vitest bump). Schema path conflict in super/expected-edges.json resolved by taking main's '../../../' path which is correct for the fixture's directory depth. |
…n with local binary (#1389)
|
Addressed Greptile's diagnostic note on getNativePackageVersion() — added a JSDoc note clarifying that this function always reports the npm package version, so when the local dev binary or NAPI_RS_NATIVE_LIBRARY_PATH is loaded instead, the reported version may not match the running binary. (commit 612bde0) |
…eclaration (#1389) - native.ts: add debug log when step 3 (npm package) loads successfully, matching the trace already present for steps 1 and 2 - incremental.ts: split `let { targets, importedFrom }` into `const { targets: initialTargets, importedFrom }` + `let targets = initialTargets` to make explicit that only targets is ever reassigned
|
Addressed the two remaining items from Greptile's latest summary:
|
|
Fixed — aligned lockfile to vitest 4.1.8 to match main. The 4.1.7 entries for |
| // 2. Locally compiled dev binary — preferred over npm package so that Rust | ||
| // changes are visible without publishing. Only used when the file exists. | ||
| const localFile = PLATFORM_LOCAL_BINARIES[platformKey]; | ||
| if (localFile) { | ||
| try { | ||
| const localPath = fileURLToPath( | ||
| new URL(`../../crates/codegraph-core/${localFile}`, import.meta.url), | ||
| ); | ||
| _cached = _require(localPath) as NativeAddon; | ||
| debug(`loadNative: loaded local dev binary: ${localPath}`); | ||
| return _cached; | ||
| } catch (err) { | ||
| debug(`loadNative: local dev binary not available: ${toErrorMessage(err as Error)}`); | ||
| } | ||
| } |
There was a problem hiding this comment.
When the local dev binary exists on disk but fails to load (e.g., a stale .node file from a prior build after an ABI change), the catch block logs at debug level and falls through to the npm package — the exact scenario the PR is trying to prevent. A developer would see their Rust changes "tested" but the integration suite would actually run against the stale npm binary, potentially giving a false-green.
The env-var path (step 1) already distinguishes "explicit operator choice, failure is fatal" from general failures. The same distinction is worth making here: err.code === 'MODULE_NOT_FOUND' means the file simply isn't there (normal in CI/production, silent skip is correct); any other error means the file exists but failed to load — that case should log at warn level and ideally return null rather than fall through.
There was a problem hiding this comment.
Fixed — when a local binary exists on disk but fails to load, we now use existsSync to distinguish the two cases: if the file is absent, fall through normally to the npm package; if the file is present but require throws, log at warn level and return null immediately (no fallthrough). This mirrors the same pattern used for NAPI_RS_NATIVE_LIBRARY_PATH failures. Commit: f57ad9c.
…llthrough When a local dev binary exists on disk but fails to load (e.g. stale ABI after a Rust rebuild), warn at warn level and return null immediately. Only fall through to the npm package when the file is absent entirely. This preserves the priority guarantee: a present-but-broken local binary surfaces as an explicit developer error rather than silently running the published npm binary.
1 participant
Summary
loadNative()now checks for a locally compiledcrates/codegraph-core/<platform>.nodebinary before the published npm package, so Rust changes land immediately in development without a new npm releaseNAPI_RS_NATIVE_LIBRARY_PATHenv-var support as an explicit highest-priority overrideresolvePlatformKey()shared helper used by bothPLATFORM_PACKAGESand the newPLATFORM_LOCAL_BINARIESmapLoad order is now:
NAPI_RS_NATIVE_LIBRARY_PATHenv var (explicit override)crates/codegraph-core/<platform>.node(freshly compiled dev binary)@optave/codegraph-<platform>npm package (published production binary)Root cause
PR #1339 added prototype-method extraction to the Rust extractor, but the published npm binary (
@optave/codegraph-darwin-arm64v3.11.2) was built before that merge. In workspaces where the npm package is installed in a parentnode_modules/, Node's module resolution finds it before the local binary — so the stale binary was always loaded.By reversing the priority (local binary first) the integration tests pick up the current Rust code, and
Dog.barkis correctly emitted.Test plan
tests/integration/prototype-method-resolution.test.ts— 3/3 passing (was 0/3)tests/integration/) — 633/633 passingbiome check src/infrastructure/native.ts— no errorsCloses #1361