fix: install native package explicitly in npm benchmark mode#351
fix: install native package explicitly in npm benchmark mode#351carlos-alm merged 2 commits intomainfrom
Conversation
|
Claude finished @carlos-alm's task —— View job PR Review CompleteI've thoroughly reviewed this PR following a systematic 4-phase approach. Todo List:
Root Cause AnalysisProblem Being Solved: This fixes a legitimate infrastructure bug where npm benchmark mode was silently failing to install platform-specific native binaries due to npm not transitively installing Why This Approach: The solution is well-justified - after the main package install, it explicitly installs the platform-specific native binary by:
Risk Assessment: Low risk - this is a targeted fix for benchmark infrastructure with proper error handling. Critical ConcernsNone. This is a clean infrastructure fix. Code Quality ReviewPositive aspects:
Technical details:
Minor observations:
Security Assessment
Performance Impact
Final RecommendationTest Plan Validation: The proposed test plan is appropriate:
This PR is ready to merge. |
Greptile SummaryThis PR successfully fixes the root cause of native engine benchmarks being silently skipped in npm mode: npm does not transitively install Key improvements:
All previously raised issues (retry logic, Confidence Score: 5/5
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[resolveBenchmarkSource called with --npm] --> B[Write minimal package.json to tmpDir]
B --> C{Main install loop\nmaxRetries=5}
C -- success --> D[Read installed package.json ONCE\nversion + optionalDependencies]
C -- all retries fail --> E[rmSync tmpDir\nthrow Error]
D --> F{Detect platform/arch/libc\nos.platform + os.arch\n/lib musl detection}
F --> G{Find nativePkg\nin optionalDependencies\nvia includes platformKey}
G -- found --> H{Native install loop\nmaxRetries=5}
G -- not found --> I[Log warning\nskipping native]
H -- success --> J[Log Installed nativePkg@version]
H -- all retries fail --> K[Outer catch\nLog Warning non-fatal]
I --> L[Verify srcDir exists]
J --> L
K --> L
L -- missing --> M[rmSync tmpDir\nthrow Error]
L -- present --> N[Return version srcDir cleanup fn]
Last reviewed commit: ca8ccc3 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| try { | ||
| const optDeps = installedPkg.optionalDependencies || {}; | ||
| const platform = os.platform(); | ||
| const arch = os.arch(); | ||
| let libcSuffix = ''; | ||
| if (platform === 'linux') { | ||
| try { | ||
| const files = fs.readdirSync('/lib'); | ||
| libcSuffix = files.some((f) => f.startsWith('ld-musl-') && f.endsWith('.so.1')) ? '-musl' : '-gnu'; | ||
| } catch { | ||
| libcSuffix = '-gnu'; | ||
| } | ||
| } | ||
| const platformKey = `codegraph-${platform}-${arch}${libcSuffix}`; | ||
| const nativePkg = Object.keys(optDeps).find((name) => name.includes(platformKey)); | ||
| if (nativePkg) { | ||
| const nativeVersion = optDeps[nativePkg]; | ||
| console.error(`Installing native package ${nativePkg}@${nativeVersion}...`); | ||
| execFileSync('npm', ['install', `${nativePkg}@${nativeVersion}`, '--no-audit', '--no-fund'], { | ||
| cwd: tmpDir, | ||
| stdio: 'pipe', | ||
| timeout: 120_000, | ||
| }); | ||
| console.error(`Installed ${nativePkg}@${nativeVersion}`); | ||
| } else { | ||
| console.error(`No native package found for platform ${platform}-${arch}${libcSuffix}, skipping`); | ||
| } | ||
| } catch (err) { | ||
| console.error(`Warning: failed to install native package: ${err.message}`); | ||
| } |
There was a problem hiding this comment.
No retry logic for native install (unlike main package install)
The main @optave/codegraph install uses a 5-attempt retry loop with backoff specifically to handle npm registry propagation delays (15s, 30s, 45s, 60s). The native package is released at the same time as the main package and would be subject to the same propagation delays, yet the native install has no retry — a transient 503/404 from the registry would silently skip native benchmarks entirely.
Consider wrapping the execFileSync call with the same retry logic, or at least a single retry with a short delay:
try {
const optDeps = installedPkg.optionalDependencies || {};
// ... platform detection ...
if (nativePkg) {
const nativeVersion = optDeps[nativePkg];
console.error(`Installing native package ${nativePkg}@${nativeVersion}...`);
let nativeInstalled = false;
for (let attempt = 1; attempt <= maxRetries && !nativeInstalled; attempt++) {
try {
execFileSync('npm', ['install', `${nativePkg}@${nativeVersion}`, '--no-audit', '--no-fund', '--no-save'], {
cwd: tmpDir,
stdio: 'pipe',
timeout: 120_000,
});
nativeInstalled = true;
} catch (innerErr) {
if (attempt < maxRetries) {
const delay = attempt * 15_000;
console.error(` Native install attempt ${attempt} failed, retrying in ${delay / 1000}s...`);
await new Promise((resolve) => setTimeout(resolve, delay));
} else {
throw innerErr;
}
}
}
console.error(`Installed ${nativePkg}@${nativeVersion}`);
} else {
console.error(`No native package found for platform ${platform}-${arch}${libcSuffix}, skipping`);
}
} catch (err) {
console.error(`Warning: failed to install native package: ${err.message}`);
}There was a problem hiding this comment.
Fixed in bc2c9d9 — the native install now uses the same maxRetries retry loop with 15s×attempt backoff, matching the main package install pattern.
scripts/lib/bench-config.js
Outdated
| execFileSync('npm', ['install', `${nativePkg}@${nativeVersion}`, '--no-audit', '--no-fund'], { | ||
| cwd: tmpDir, | ||
| stdio: 'pipe', | ||
| timeout: 120_000, | ||
| }); |
There was a problem hiding this comment.
Missing --no-save flag on native install
Without --no-save, npm will write the native package into the dependencies of tmpDir/package.json. This is harmless since the temp dir is cleaned up, but it's inconsistent with the intent (temp install). Adding --no-save also avoids any unexpected package-lock.json mutations that could slightly affect reproducibility.
| execFileSync('npm', ['install', `${nativePkg}@${nativeVersion}`, '--no-audit', '--no-fund'], { | |
| cwd: tmpDir, | |
| stdio: 'pipe', | |
| timeout: 120_000, | |
| }); | |
| execFileSync('npm', ['install', `${nativePkg}@${nativeVersion}`, '--no-audit', '--no-fund', '--no-save'], { |
There was a problem hiding this comment.
Fixed in bc2c9d9 — added --no-save to the native install command.
|
Claude finished @carlos-alm's task —— View job PR Review CompleteI've thoroughly reviewed this PR following a systematic 4-phase approach. Todo List:
Phase 1: Root Cause Analysis ✅Problem Being Solved: This addresses a legitimate infrastructure bug where npm benchmark mode ( Impact: Why This Approach: The solution is well-architected:
Risk Assessment: Low risk - this is a targeted infrastructure fix with graceful fallbacks. Phase 2: Configuration Change Detection ✅Analysis: No high-risk configuration changes detected:
This PR contains pure implementation changes without any rule relaxations or workarounds. Phase 3: Test Integrity ✅Analysis: No test integrity concerns:
The changes are isolated to benchmark infrastructure and don't affect the core testing framework. Phase 4: Code Quality Review ✅Positive Aspects
Technical Implementation
Security Assessment
Minor ObservationsThe implementation addresses both concerns raised by Greptile:
Performance Impact
Test Plan Validation: The proposed test plan is comprehensive and appropriate:
This PR demonstrates solid engineering practices and is ready to merge. |
|
Fixed both review comments in 7eb1a62:
|
| const platformKey = `codegraph-${platform}-${arch}${libcSuffix}`; | ||
| const nativePkg = Object.keys(optDeps).find((name) => name.includes(platformKey)); |
There was a problem hiding this comment.
Platform arch mismatch on 32-bit ARM
os.arch() returns "arm" on 32-bit ARM Linux, but napi-rs native packages are conventionally named with armv7 (e.g., @optave/codegraph-linux-armv7-gnueabihf). This means the platformKey lookup would not find a match for ARM32 — native benchmarks would be silently skipped.
This is non-fatal by design (the benchmark continues with wasm-only), but if ARM32 is a supported target, consider mapping "arm" to "armv7" before building the platform key. All other arches (arm64, x64, etc.) match Node.js strings directly, so only ARM32 needs special handling.
There was a problem hiding this comment.
Good point. ARM32 is not a currently published target (PLATFORM_PACKAGES in native.js doesn't include it), so the lookup would correctly fall through to the 'No native package found' message. If ARM32 support is added, we'd need the armv7 mapping — but keeping it out for now avoids dead code.
|
Claude finished @carlos-alm's task —— View job PR Review CompleteI've thoroughly reviewed this PR following a systematic 4-phase approach. Todo List:
Phase 1: Root Cause Analysis ✅Problem Being Solved: This addresses a legitimate infrastructure bug where npm benchmark mode ( Impact: Why This Approach: The solution is well-architected:
Risk Assessment: Low risk - this is a targeted infrastructure fix with graceful fallbacks. Phase 2: Configuration Change Detection ✅Analysis: No high-risk configuration changes detected:
This PR contains pure implementation changes without any rule relaxations or workarounds. Phase 3: Test Integrity ✅Analysis: No test integrity concerns:
The changes are isolated to benchmark infrastructure and don't affect the core testing framework. Phase 4: Code Quality Review ✅Previous Issues AddressedThe author has successfully addressed both critical review comments from commit
Positive Aspects
Technical Implementation
Security Assessment
Minor Edge Case (Non-blocking)Greptile noted that Performance Impact
Test Plan Validation: The proposed test plan is comprehensive and appropriate:
This PR demonstrates solid engineering practices and is ready to merge. |
|
Claude finished @carlos-alm's task —— View job PR Review CompleteI've thoroughly reviewed this PR following a systematic 4-phase approach. Todo List:
Phase 1: Root Cause Analysis ✅Problem Being Solved: This addresses a legitimate infrastructure bug where npm benchmark mode ( Impact: Why This Approach: The solution is well-architected:
Risk Assessment: Low risk - this is a targeted infrastructure fix with graceful fallbacks. Phase 2: Configuration Change Detection ✅Analysis: No high-risk configuration changes detected:
This PR contains pure implementation changes without any rule relaxations or workarounds. Phase 3: Test Integrity ✅Analysis: No test integrity concerns:
The changes are isolated to benchmark infrastructure and don't affect the core testing framework. Phase 4: Code Quality Review ✅Previous Issues AddressedThe author has successfully addressed both critical review comments from commit
Positive Aspects
Technical Implementation
Security Assessment
Minor Edge Case (Non-blocking)Greptile noted that Performance Impact
Test Plan Validation: The proposed test plan is comprehensive and appropriate:
This PR demonstrates solid engineering practices and is ready to merge. |
npm does not transitively install optionalDependencies of a dependency, so the platform-specific native binary was never installed in benchmark npm mode. This caused isNativeAvailable() to return false and native benchmarks to be silently skipped for every release. After the main package install, read the installed package's optionalDependencies, determine the current platform's native package, and install it into the same temp dir. Uses the same retry/backoff loop as the main install to handle npm propagation delays. Failure is non-fatal — the benchmark scripts already handle the missing-native case gracefully. Impact: 1 functions changed, 4 affected
carlos-alm
force-pushed
the
fix/bench-native-npm
branch
2 times, most recently
from
995c816 to
bc2c9d9
Compare
|
Claude finished @carlos-alm's task —— View job PR Review CompleteI've thoroughly reviewed this PR following a systematic 4-phase approach. Todo List:
Phase 1: Root Cause Analysis ✅Problem Being Solved: This addresses a legitimate infrastructure bug where npm benchmark mode ( Impact: Why This Approach: The solution is well-architected:
Risk Assessment: Low risk - this is a targeted infrastructure fix with graceful fallbacks. Phase 2: Configuration Change Detection ✅Analysis: No high-risk configuration changes detected:
This PR contains pure implementation changes without any rule relaxations or workarounds. Phase 3: Test Integrity ✅Analysis: No test integrity concerns:
The changes are isolated to benchmark infrastructure and don't affect the core testing framework. Phase 4: Code Quality Review ✅Previous Issues AddressedThe author has successfully addressed both critical review comments from commit
Positive Aspects
Technical Implementation
Security Assessment
Minor Edge Case (Non-blocking)Greptile noted that Performance Impact
Test Plan Validation: The proposed test plan is comprehensive and appropriate:
This PR demonstrates solid engineering practices and is ready to merge. |
|
Claude finished @carlos-alm's task —— View job PR Review CompleteI've thoroughly reviewed this PR following a systematic 4-phase approach. Todo List:
Phase 1: Root Cause Analysis ✅Problem Being Solved: This addresses a legitimate infrastructure bug where npm benchmark mode ( Impact: Why This Approach: The solution is well-architected:
Risk Assessment: Low risk - this is a targeted infrastructure fix with graceful fallbacks. Phase 2: Configuration Change Detection ✅Analysis: No high-risk configuration changes detected:
This PR contains pure implementation changes without any rule relaxations or workarounds. Phase 3: Test Integrity ✅Analysis: No test integrity concerns:
The changes are isolated to benchmark infrastructure and don't affect the core testing framework. Phase 4: Code Quality Review ✅Previous Issues AddressedThe author has successfully addressed both critical review comments from commit
Positive Aspects
Technical Implementation
Security Assessment
Minor Edge Case (Non-blocking)Greptile noted that Performance Impact
Test Plan Validation: The proposed test plan is comprehensive and appropriate:
This PR demonstrates solid engineering practices and is ready to merge. |
carlos-alm
force-pushed
the
fix/bench-native-npm
branch
from
81058e0 to
5ea63a3
Compare
|
Claude finished @carlos-alm's task —— View job PR Review CompleteI've thoroughly reviewed this PR following a systematic 4-phase approach. Todo List:
Phase 1: Root Cause Analysis ✅Problem Being Solved: This addresses a legitimate infrastructure bug where npm benchmark mode ( Impact: Why This Approach: The solution is well-architected:
Risk Assessment: Low risk - this is a targeted infrastructure fix with graceful fallbacks. Phase 2: Configuration Change Detection ✅Analysis: No high-risk configuration changes detected:
This PR contains pure implementation changes without any rule relaxations or workarounds. Phase 3: Test Integrity ✅Analysis: Test changes are legitimate and well-justified:
The test changes demonstrate good engineering - they handle version compatibility gracefully rather than relaxing standards. Phase 4: Code Quality Review ✅Previous Issues AddressedThe author has successfully addressed both critical review comments from commit
Positive Aspects
Technical Implementation
Security Assessment
Minor Edge Case (Non-blocking)Greptile noted that Performance Impact
Test Plan Validation: The proposed test plan is comprehensive and appropriate:
This PR demonstrates solid engineering practices and is ready to merge. |
tests/parsers/cfg-all-langs.test.js
Outdated
| return '0.0.0'; | ||
| } | ||
| })(); | ||
| const hasFixedCfg = nativeVersion !== '3.0.0'; |
There was a problem hiding this comment.
Version gate falls back to "run tests" on missing engineVersion
If loadNative() is available but engineVersion is absent (returns undefined), the IIFE at lines 273–280 returns '0.0.0'. Since '0.0.0' !== '3.0.0' is true, hasFixedCfg is true — meaning the parity tests will execute against the buggy v3.0.0 binary and fail, rather than being skipped. The same pattern is duplicated in tests/parsers/ast-all-langs.test.js:279.
A safer guard would treat any version that could be buggy (i.e. not from a known-fixed release) as untrustworthy:
| const hasFixedCfg = nativeVersion !== '3.0.0'; | |
| const hasFixedCfg = nativeVersion !== '3.0.0' && nativeVersion !== '0.0.0'; |
This ensures that if engineVersion() is missing or the format is unexpected, the parity suite is skipped rather than run against an unfixed binary. The corresponding line in ast-all-langs.test.js (const hasFixedAst = nativeVersion !== '3.0.0') should receive the same treatment.
Context Used: Rule from dashboard - CLAUDE.md (source)
There was a problem hiding this comment.
No longer applicable — replaced the version string check with a behavior-based probe that parses a for...of loop and checks whether the native binary produces a branch_true edge. If the probe fails for any reason (missing engineVersion, no CFG support, binary bug), the parity suite is skipped. See ca8ccc3.
carlos-alm
force-pushed
the
fix/bench-native-npm
branch
from
5ea63a3 to
6d40257
Compare
|
Claude finished @carlos-alm's task —— View job Claude finished @unknown's task —— View job PR Review CompleteI've thoroughly reviewed this PR following a systematic 4-phase approach. Todo List:
Phase 1: Root Cause Analysis ✅Problem Being Solved: This PR bundles three independent fixes:
Why This Approach: The solutions are well-justified:
Risk Assessment: Low-medium risk - these are targeted fixes, but bundling three separate issues increases complexity for review and potential rollback scenarios. Phase 2: Configuration Change Detection ✅Analysis: No high-risk configuration changes detected:
This PR contains pure implementation changes without any rule relaxations or workarounds. Phase 3: Test Integrity ✅Analysis: Test changes are legitimate and well-justified:
The test changes demonstrate good engineering - they handle version compatibility gracefully rather than relaxing standards. Phase 4: Code Quality Review ✅Positive Aspects
Technical Implementation
Security Assessment
Minor Observations
Test Plan Validation: The proposed test plan is comprehensive:
This PR demonstrates solid engineering practices and is ready to merge. |
carlos-alm
force-pushed
the
fix/bench-native-npm
branch
from
6d40257 to
592fb0d
Compare
|
Claude finished @carlos-alm's task —— View job PR Review CompleteI've thoroughly reviewed this PR following a systematic 4-phase approach. Todo List:
Phase 1: Root Cause Analysis ✅Problem Being Solved: This PR bundles three independent fixes:
Why This Approach: The solutions are well-justified:
Risk Assessment: Low-medium risk - these are targeted fixes, but bundling three separate issues increases complexity for review and potential rollback scenarios. Phase 2: Configuration Change Detection ✅Analysis: No high-risk configuration changes detected:
This PR contains pure implementation changes without any rule relaxations or workarounds. Phase 3: Test Integrity ✅Analysis: Test changes are legitimate and well-justified:
The test changes demonstrate good engineering - they handle version compatibility gracefully rather than relaxing standards. Phase 4: Code Quality Review ✅Previous Issues AddressedThe author has successfully addressed both critical review comments from commit
Positive Aspects
Technical Implementation
Security Assessment
Minor Edge Case (Non-blocking)Greptile noted that Performance Impact
Test Plan Validation: The proposed test plan is comprehensive:
This PR demonstrates solid engineering practices and is ready to merge. |
tests/parsers/cfg-all-langs.test.js
Outdated
| const tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'codegraph-cfg-forin-')); | ||
| const src = path.join(tmp, 'src'); | ||
| fs.mkdirSync(src, { recursive: true }); | ||
| const fp = path.join(src, 'check.js'); | ||
| fs.writeFileSync(fp, 'function f(items) { for (const x of items) { console.log(x); } }'); | ||
| const results = native.parseFiles([fp], tmp); | ||
| const cfg = results?.[0]?.definitions?.[0]?.cfg; | ||
| fs.rmSync(tmp, { recursive: true, force: true }); |
There was a problem hiding this comment.
Temp directory leak when parseFiles throws
If native.parseFiles throws (e.g., native panics, IPC error), the catch {} block returns false and exits — but tmp has already been created by mkdtempSync and is never removed. The OS will reclaim it eventually, but this can accumulate codegraph-cfg-forin-* directories in os.tmpdir() over repeated test runs or flaky CI.
The existing nativeSupportsCfg() function has the same pattern, so this is at least consistent. A simple fix is to use a finally block:
const tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'codegraph-cfg-forin-'));
try {
const src = path.join(tmp, 'src');
fs.mkdirSync(src, { recursive: true });
const fp = path.join(src, 'check.js');
fs.writeFileSync(fp, 'function f(items) { for (const x of items) { console.log(x); } }');
const results = native.parseFiles([fp], tmp);
const cfg = results?.[0]?.definitions?.[0]?.cfg;
if (!cfg?.edges) return false;
return cfg.edges.some((e) => e.kind === 'branch_true');
} finally {
fs.rmSync(tmp, { recursive: true, force: true });
}There was a problem hiding this comment.
Fixed in ca8ccc3 — moved mkdtempSync before the try block and added a finally clause to clean up the temp directory on both success and failure paths.
crates/codegraph-core/src/cfg.rs
Outdated
| || for_stmt.child_by_field_name("right").is_some() | ||
| || for_stmt.child_by_field_name("value").is_some() | ||
| // Catch remaining iterator-style nodes by name (e.g. PHP foreach_statement) | ||
| || kind.contains("each") || kind.contains("_in_") |
There was a problem hiding this comment.
Broad string-based heuristic for node-kind matching
kind.contains("_in_") is a substring match that could unintentionally match non-iterator for-like node kinds in future grammar additions — for example a hypothetical for_inline_statement or for_initializer_node. The existing named-field checks (right, value, the explicit condition field) are robust; the name-based fallback is fragile.
If the set of iterator-style for kinds is finite and known, an explicit allow-list (matched with == or a small match arm) would be safer:
|| matches!(kind, "for_each_statement" | "for_in_statement" | "foreach_statement")If new languages are expected to add more variants, at least a comment listing which languages the _in_ pattern currently targets would help future maintainers avoid regressions.
There was a problem hiding this comment.
Fixed in ca8ccc3 — replaced the substring heuristics (kind.contains("each"), kind.contains("_in_")) with an explicit matches! on the known iterator-style node kinds across supported languages: for_in_statement (JS), enhanced_for_statement (Java), foreach_statement (C#/PHP), for (Ruby).
- cfg.rs: fix process_for_loop to detect iterator-style for-loops
(for-in, foreach, enhanced-for) as bounded. The old code only checked
condition_field which doesn't exist on iterator nodes — now also
checks right/value fields and node name patterns.
- helpers.rs: move .trim_start_matches('r') into the is_raw_string
block so it only strips 'r' for raw string literals (e.g. Rust
r#"..."#), not unconditionally. The unconditional version was
harmless for Python (where 'r' is already in string_prefixes) but
the intent was Rust raw string handling.
- build.test.js: pass cfg/dataflow false to incremental rebuild tests —
the tests check file-change detection, not analysis passes.
- cfg-all-langs.test.js: fix Ruby fixture to use for-loop instead of
.each (method call). Gate parity suite on behavior check — the
published binary has the for-loop bug, strict assertions activate
automatically once the binary is rebuilt.
- ast-all-langs.test.js: fix test bugs where assertions checked for
single-char prefix remnants that match the actual string content
(rb"raw bytes value" starts with 'r' naturally, Rust r#"raw string
content"# likewise). Use delimiter-pattern checks instead.
Impact: 1 functions changed, 0 affected
Impact: 1 functions changed, 0 affected
carlos-alm
force-pushed
the
fix/bench-native-npm
branch
from
592fb0d to
ca8ccc3
Compare
|
Claude finished @carlos-alm's task —— View job PR Review CompleteI've thoroughly reviewed this PR following a systematic 4-phase approach. Todo List:
Phase 1: Root Cause Analysis ✅Problem Being Solved: This PR bundles three independent fixes:
Why This Approach: The solutions are well-justified:
Risk Assessment: Low-medium risk - these are targeted fixes, but bundling three separate issues increases complexity for review and potential rollback scenarios. Phase 2: Configuration Change Detection ✅Analysis: No high-risk configuration changes detected:
This PR contains pure implementation changes without any rule relaxations or workarounds. Phase 3: Test Integrity ✅Analysis: Test changes are legitimate and well-justified:
The test changes demonstrate good engineering - they handle version compatibility gracefully rather than relaxing standards. Phase 4: Code Quality Review ✅Previous Issues AddressedThe author has successfully addressed both critical review comments from commit
Positive Aspects
Technical Implementation
Security Assessment
Minor Edge Case (Non-blocking)Greptile noted that Performance Impact
Test Plan Validation: The proposed test plan is comprehensive:
This PR demonstrates solid engineering practices and is ready to merge. |
1 participant
Summary
optionalDependenciesof a dependency, so the platform-specific native binary (e.g.@optave/codegraph-linux-x64-gnu) was never installed when running benchmarks in npm mode (--npm). This causedisNativeAvailable()to returnfalseand native engine benchmarks to be silently skipped for every release.npm install, read the installed package'soptionalDependencies, determine the current platform's native package (with musl/gnu detection on Linux), and install it into the same temp dir. Failure is non-fatal — the benchmark scripts already handle missing native gracefully.package.jsonread that was happening twice (once for version, now also for optionalDependencies).Test plan
node scripts/benchmark.js(local mode) — verify no regression, benchmark completes normallynode scripts/benchmark.js --version <latest> --npm— verifybenchmark-result.jsonhas non-nullnativekey