test(native): add unit tests for loadNative() load-order and env-var override

[carlos-alm]

Summary

• Adds tests/unit/native-loader.test.ts with 5 tests that exercise each load path in loadNative() in isolation
• Each test uses vi.resetModules() + vi.doMock() + dynamic import() to get a fresh module with clean singleton state (_cached / _loadError reset per test)

Tests added

Scenario	What is verified
NAPI_RS_NATIVE_LIBRARY_PATH set and valid	Returns the module; second call hits the singleton cache (require called once)
NAPI_RS_NATIVE_LIBRARY_PATH set but bad path	Returns null; no fall-through to local binary or npm package; WARN emitted
No env var, local binary present	Loads local binary; npm package require never attempted
No env var, no local binary, npm package present	Loads npm package (@optave/codegraph-darwin-arm64)
No env var, unsupported platform	Returns null without any require calls; getNative() throws EngineError

Closes #1392

[greptile-apps]

Greptile Summary

Adds tests/unit/native-loader.test.ts with five isolation tests for loadNative(), one per load-order branch (env-path success/failure, local binary, npm package, unsupported platform). Each test re-imports the module after vi.resetModules() + vi.doMock() to get a clean singleton state.

• Tests 1–2 cover the NAPI_RS_NATIVE_LIBRARY_PATH override, including cache behaviour and the no-fall-through guarantee; test 2 now also confirms the null result is cached (second call does not retry require).
• Tests 3–4 exercise the local-binary-first / npm-package-fallback ordering via a controlled existsSync mock.
• Test 5 verifies the unsupported-platform path returns null with zero require calls and that getNative() raises EngineError.

Confidence Score: 5/5

Test-only addition with no changes to production code; safe to merge.

The change is entirely additive — new test file only. The five tests correctly isolate singleton state via vi.resetModules + dynamic import, assertions match the actual load-order logic in native.ts, and the feedback from previous review rounds (redundant restore, null-result cache, node: pass-through) has been incorporated.

No files require special attention.

Important Files Changed

Filename	Overview
tests/unit/native-loader.test.ts	New test file covering all five load-order branches of loadNative(); uses vi.resetModules + vi.doMock + dynamic import pattern correctly to isolate singleton state per test.

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 --> E[_require envPath]
    E -- success --> F[_cached = module\nreturn module]
    E -- error --> G[_cached = null\nwarn to stderr\nreturn null]
    D -- No --> H{localFile defined?\nexistsSync localPath?}
    H -- Yes --> I[_require localPath]
    I -- success --> J[_cached = module\nreturn module]
    I -- error --> K[_cached = null\nwarn\nreturn null]
    H -- No --> L{Platform in PLATFORM_PACKAGES?}
    L -- Yes --> M[_require pkg]
    M -- success --> N[_cached = module\nreturn module]
    M -- error --> O[_loadError = err\n_cached = null\nreturn null]
    L -- No --> P[_loadError = unsupported platform\n_cached = null\nreturn null]

Loading

_{Reviews (8): Last reviewed commit: "Merge branch 'main' into test/native-loa..." | Re-trigger Greptile}

[greptile-apps]

Redundant spy restore: stderrSpy.mockRestore() is called on line 57, and then vi.restoreAllMocks() is called on line 58 — which already invokes mockRestore() on every active spy, including stderrSpy. The explicit call is a no-op and can be removed to keep afterEach clean.

Suggested change

	afterEach(() => {
	stderrSpy.mockRestore();
	vi.restoreAllMocks();
	vi.unstubAllEnvs();
	});
	afterEach(() => {
	vi.restoreAllMocks();
	vi.unstubAllEnvs();
	});

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!

[carlos-alm]

Fixed — removed the redundant stderrSpy.mockRestore() call. vi.restoreAllMocks() already covers all active spies including stderrSpy.

[greptile-apps]

The null-result singleton cache is exercised by test 1 (positive path) but not by this test for the negative path. A second loadNative() call here would confirm that, after a failed env-path load, the _cached = null short-circuit fires and require is not retried — mirroring the toHaveBeenCalledTimes(1) assertion in test 1.

Suggested change

	expect(result).toBeNull();
	// Only the env-path require was attempted — no fall-through to local or npm
	expect(requireFn).toHaveBeenCalledTimes(1);
	expect(requireFn).toHaveBeenCalledWith('/bad/path.node');
	// A warning mentioning the env var name was emitted to stderr
	const stderr = stderrSpy.mock.calls.map((c) => String(c[0])).join('');
	expect(stderr).toContain('NAPI_RS_NATIVE_LIBRARY_PATH');
	expect(result).toBeNull();
	// Only the env-path require was attempted — no fall-through to local or npm
	expect(requireFn).toHaveBeenCalledTimes(1);
	expect(requireFn).toHaveBeenCalledWith('/bad/path.node');
	// A warning mentioning the env var name was emitted to stderr
	const stderr = stderrSpy.mock.calls.map((c) => String(c[0])).join('');
	expect(stderr).toContain('NAPI_RS_NATIVE_LIBRARY_PATH');
	// Null result is cached — second call must not invoke require again
	expect(loadNative()).toBeNull();
	expect(requireFn).toHaveBeenCalledTimes(1);

[carlos-alm]

Fixed — added the null-result cache second-call assertion to the bad-env-path test. A second loadNative() call now confirms requireFn is still only called once.

[greptile-apps]

makeMockRequire handles absolute paths and @optave/ package names, but has no branch for bare Node built-in specifiers (e.g. node:fs). In loadNative(), detectLibc() calls _require('node:fs') for the Linux platform. Because all current tests mock the platform as darwin or freebsd, detectLibc() is never triggered and the tests pass today. However, if a future test adds a Linux scenario (e.g. linux-x64-gnu), makeMockRequire will throw Unexpected require call: node:fs, producing a misleading failure.

[carlos-alm]

Fixed — added a node: pass-through branch in makeMockRequire that delegates to the real require for Node built-ins. This prevents a misleading Unexpected require call: node:fs failure if a future test exercises the Linux code path through detectLibc().

[carlos-alm]

@greptileai

[carlos-alm]

Addressed the two additional issues from the Greptile review summary:

• Test 3 (local binary present): added second-call cache assertion — confirms requireFn is not re-invoked after the result is cached.
• Test 4 (npm fallback): added load-order assertions — toHaveBeenCalledTimes(2) plus explicit checks that both the absolute local-binary path and @optave/codegraph-darwin-arm64 were called in order.

[carlos-alm]

@greptileai

[carlos-alm]

Merge conflict resolution (main → this branch):

Two files had conflicts:

1. src/infrastructure/native.ts: Resolved by taking origin/main's behavior — existsSync check before calling require() on the local dev binary, with warn-and-halt if the file exists but fails to load.

2. src/domain/graph/builder/incremental.ts: Resolved by taking origin/main's const/let split (const { targets: initialTargets, ... } = ... / let targets = initialTargets) and removing the duplicate if (targets.length === 0 ...) blocks that appeared from the merge.

Test update: The existsSync change in native.ts required updating tests/unit/native-loader.test.ts:

• mockDeps now accepts a localBinaryExists parameter and mocks node:fs accordingly.
• Test 3 (local binary present): passes localBinaryExists: true so existsSync returns true and require is called for the local binary.
• Test 4 (npm fallback): passes localBinaryExists: false so existsSync returns false — the local binary require is skipped entirely, resulting in 1 call total (npm package only) rather than the 2 calls described in the previous comment. This is correct behavior: existsSync is the guard, so when the file is absent, require is never attempted.

All 5 native-loader tests pass.

[carlos-alm]

@greptileai