fix(bench): use dist/ for npm benchmark installs

[carlos-alm]

Summary

• bench-config.ts: When installing codegraph from npm, use dist/ (compiled JS) instead of src/ (raw TS) as the import root. Node 22 throws ERR_UNSUPPORTED_NODE_MODULES_TYPE_STRIPPING when importing .ts files from node_modules, which caused all 4 benchmark jobs to fail on v3.4.0.
• benchmark.yml: Replace node src/cli.ts build . with npx codegraph build . in the embedding benchmark's "Build graph" step — uses the package's bin entry point instead of a hardcoded source path.

The prior fix (#612) resolved .js→.ts in srcImport(), but that still fails because Node refuses type stripping inside node_modules. Using dist/ sidesteps this entirely since it contains pre-compiled .js files. Falls back to src/ for older packages without dist/.

Test plan

• Re-run benchmark workflow with version: 3.4.0 via workflow_dispatch — all 4 jobs should pass and create PRs
• Run benchmarks in dev mode (local) — should still work via src/ + ts-resolve-loader

[claude]

Claude finished @carlos-alm's task —— View job

---

PR Review Complete

Reviewing PR #624: fix(bench): use dist/ for npm benchmark installs

Todo List

• Read and analyze changed files
• Assess root cause and solution approach
• Check for configuration change risks
• Evaluate code quality and security
• Assess backlog compliance criteria
• Provide final review recommendation

---

Root Cause Analysis

Problem Being Solved: Node.js 22+ throws ERR_UNSUPPORTED_NODE_MODULES_TYPE_STRIPPING when importing TypeScript files from node_modules, causing benchmark job failures on v3.4.0.

Why This Approach: The solution correctly addresses the root cause by using the published compiled JavaScript files (dist/) instead of raw TypeScript files (src/) when available. Node.js cannot perform type stripping on files inside node_modules, so using pre-compiled JS sidesteps this limitation entirely.

Risk Assessment: ✅ Low risk. This is a surgical fix with proper fallback logic. The changes are minimal and well-targeted.

Backlog Compliance

• Zero-dep: ✅ — No new runtime dependencies added
• Foundation-aligned: ✅ — Maintains existing build/benchmark functionality without violating core principles
• Problem-fit: 3 — Fixes infrastructure issue that enables benchmarking to work, which indirectly supports the core problem by ensuring performance monitoring remains functional
• Breaking: No — Purely internal change with backward compatibility fallback
• Tier: 1

Technical Analysis

Changes Review

bench-config.ts:158-161:

// v3.4.0+ publishes compiled JS in dist/ alongside raw TS in src/.
// Node cannot strip types from node_modules, so prefer dist/ when available.
const distDir = path.join(pkgDir, 'dist');
const srcDir = fs.existsSync(distDir) ? distDir : path.join(pkgDir, 'src');

✅ Excellent fallback strategy: Prefers dist/ when available, falls back to src/ for older packages without compiled JS.

✅ Clear documentation: Comments explain the Node.js type-stripping limitation and version context.

✅ Minimal surface area: Only changes the directory resolution logic without affecting the broader import resolution mechanism.

benchmark.yml:237-239:

- name: Build graph
  if: steps.existing.outputs.skip != 'true'
  run: npx codegraph build .

✅ Uses published interface: Changes from hardcoded node src/cli.ts build . to npx codegraph build ., which uses the package's bin entry point.

✅ Cleaner approach: Eliminates the need for Node.js flags and import loaders in this specific step.

Code Quality Assessment

✅ Error handling preserved: The updated error message correctly reflects both potential directories.

✅ Consistency: Both changes align with the same principle — use the published package interface rather than bypassing it.

✅ No security concerns: Changes don't introduce any command injection or other security vulnerabilities.

Configuration Change Risk Analysis

✅ Low-risk workflow change: The benchmark.yml modification simplifies the build step by using the standard package entry point.

✅ No rule relaxation: This fix doesn't silence warnings or lower test standards — it addresses a legitimate Node.js compatibility issue.

Critical Concerns

None identified. This is a well-executed fix that addresses the root cause without introducing technical debt.

Final Recommendation

• Rating: ⭐⭐⭐⭐☆ (4/5)
• Action: APPROVE
• Reasoning: This PR properly fixes the Node.js type-stripping issue with a minimal, well-documented approach. The fallback strategy ensures backward compatibility, and the workflow change aligns with best practices. The solution directly addresses the stated problem without introducing complexity or risk.

Minor improvement suggestion: Consider adding a test case to verify the fallback behavior, though the existing error handling provides adequate safety.

[greptile-apps]

Greptile Summary

This PR fixes benchmark job failures on Node 22 caused by ERR_UNSUPPORTED_NODE_MODULES_TYPE_STRIPPING — Node refuses to strip TypeScript types from files inside node_modules/. The fix has two parts: (1) bench-config.ts now resolves to dist/ (compiled JS) when the installed npm package ships it, falling back to src/ for older packages; and (2) the embedding benchmark's "Build graph" step replaces a manually-assembled node --experimental-strip-types src/cli.ts command with a clean npx codegraph build ., which works because the repo's prepare script compiles dist/cli.js during npm install.\n\nKey points:\n- The srcImport helper already handles both layouts: for dist/ it falls through to the .js file (.ts variant not found); for src/ it promotes .js specifiers to .ts as before.\n- The fallback chain (dist/ → src/) is correct for all existing versions: v3.4.0+ ships dist/, older versions ship only src/.\n- The error message was improved to reflect that neither directory was found, rather than only mentioning src/.\n- JSDoc comments in bench-config.ts still describe srcDir as pointing at the src/ directory; a minor update would keep the docs in sync with the new behaviour.

Confidence Score: 5/5

Safe to merge — targeted fix with correct fallback logic and no risk to existing functionality.

The core logic is sound: dist/ is preferred when present, src/ is the fallback, and srcImport transparently handles both layouts since all call sites use .js extensions. The npx codegraph build . replacement is valid because npm install runs prepare → tsc, creating dist/cli.js before the step executes. The only finding is minor documentation staleness (P2).

No files require special attention.

Important Files Changed

Filename	Overview
scripts/lib/bench-config.ts	Prefers dist/ over src/ when resolving an npm-installed package; correct fallback logic and error message; minor JSDoc staleness around the srcDir description.
.github/workflows/benchmark.yml	Replaces the hand-rolled node + ts-resolve-loader invocation with npx codegraph build .; safe because npm install triggers the prepare script which compiles dist/cli.js.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[resolveBenchmarkSource] --> B{npm mode?}
    B -- No --> C[Return local repo src/]
    B -- Yes --> D[npm install @optave/codegraph@version into tmpDir]
    D --> E{dist/ exists in pkgDir?}
    E -- Yes v3.4.0+ --> F[srcDir = pkgDir/dist/]
    E -- No older pkg --> G[srcDir = pkgDir/src/]
    F --> H{srcDir exists?}
    G --> H
    H -- No --> I[Cleanup & throw error]
    H -- Yes --> J[Return srcDir + cleanup fn]
    J --> K[srcImport srcDir file.js]
    K --> L{file ends with .js?}
    L -- Yes --> M{.ts variant exists in srcDir?}
    M -- Yes src/ layout --> N[Return file URL to .ts file]
    M -- No dist/ layout --> O[Return file URL to .js file]
    L -- No --> O

Loading

Comments Outside Diff (1)

1. scripts/lib/bench-config.ts, line 40-44 (link)

   Stale JSDoc still says "src/ directory"

   The JSDoc for resolveBenchmarkSource still describes srcDir as "absolute path to the codegraph src/ directory to import from", but as of this PR it can now point to dist/ when the installed package ships compiled JS. The same issue exists in the srcImport JSDoc (line 194) and in the file-level header comment (line 7, which reads "srcDir (local src/ or npm-installed package src/)").

   Suggested wording for line 194:

    * @param {string} srcDir  Absolute path to the codegraph dist/ (or src/) directory
   

   And in the file header (line 7):

    *   - srcDir (local src/, or npm-installed package dist/ with src/ fallback)
   

_{Reviews (1): Last reviewed commit: "fix(bench): use dist/ for npm benchmark ..." | Re-trigger Greptile}