Merge pull request #6 from jsilvanus/claude/fix-copilot-errors-R7shg

Apply copilot/implement-review-recommendations and fix cli-format test regression

Author: jsilvanus

.github/workflows/ci.yml

@@ -11,23 +11,19 @@ jobs:
     runs-on: ubuntu-latest
     strategy:
       matrix:
-        node-version: [18.x]
+        node-version: [18.x, 20.x, 22.x]
     steps:
       - uses: actions/checkout@v4
       - uses: actions/setup-node@v4
         with:
           node-version: ${{ matrix.node-version }}
-          cache: 'pnpm'
-      - name: Enable pnpm
-        run: |
-          corepack enable
-          corepack prepare pnpm@latest --activate
+          cache: 'npm'
       - name: Install dependencies
-        run: pnpm install
+        run: npm ci
       - name: Run tests with coverage
-        run: pnpm run coverage
+        run: npm run coverage
       - name: Upload coverage report (artifact)
         uses: actions/upload-artifact@v4
         with:
-          name: coverage-report
+          name: coverage-report-${{ matrix.node-version }}
           path: coverage

eslint.config.cjs

@@ -12,6 +12,10 @@ module.exports = [
     rules: {
       'no-unused-vars': ['warn', { argsIgnorePattern: '^_' }],
       'no-console': 'off',
+      'semi': ['warn', 'always'],
+      'eqeqeq': ['error', 'always'],
+      'no-var': 'error',
+      'prefer-const': 'warn',
     },
   },
   {

package.json

@@ -4,13 +4,18 @@
   "description": "A node.js embedding tool with optional GPU acceleration",
   "main": "src/index.js",
   "types": "src/index.d.ts",
+  "exports": {
+    ".": {
+      "import": "./src/index.js",
+      "types": "./src/index.d.ts"
+    }
+  },
   "bin": {
     "embedeer": "src/cli.js"
   },
   "files": [
     "src",
-    "README.md",
-    "bench"
+    "README.md"
   ],
   "scripts": {
     "test": "node --test test/*.test.js",

src/cli.js

@@ -40,7 +40,6 @@
  *   -h, --help                   Show this help
  */
 
-import { Embedder } from './embedder.js';
 import { getCacheDir, DEFAULT_CACHE_DIR } from './model-cache.js';
 import { readFileSync, writeFileSync, appendFileSync } from 'fs';
 import readline from 'readline';
@@ -321,6 +320,7 @@ async function runInteractive(cacheDir) {
   }
 
   // Load the model before opening the reader so we are ready to embed immediately.
+  const { Embedder } = await import('./embedder.js');
   console.error(`Loading model: ${options.model}…`);
   const embedder = await Embedder.create(options.model, {
     batchSize: options.batchSize,
@@ -463,6 +463,7 @@ async function main() {
     const stdinRaw = await readStdin();
     if (!stdinRaw) {
       // No data — download/cache the model and exit.
+      const { Embedder } = await import('./embedder.js');
       console.error(`Pulling model: ${options.model}`);
       console.error(`Cache directory: ${resolvedCacheDir}`);
       await Embedder.loadModel(options.model, {
@@ -505,6 +506,7 @@ async function main() {
 
 async function runEmbedding(texts, cacheDir) {
   const t0 = options.timer ? performance.now() : 0;
+  const { Embedder } = await import('./embedder.js');
 
   const embedder = await Embedder.create(options.model, {
     batchSize: options.batchSize,

src/embedder.js

@@ -19,7 +19,7 @@ import { getCacheDir, buildPipelineOptions } from './model-cache.js';
 import os from 'os';
 import fs from 'fs';
 import { join } from 'path';
-import { execSync } from 'child_process';
+import { execFileSync } from 'child_process';
 
 export class Embedder {
   /**
@@ -56,7 +56,7 @@ export class Embedder {
 
     this.batchSize = options.batchSize ?? (device === 'gpu' ? 64 : 32);
     // Default provider selection for GPU if not provided
-    const provider = options.provider ?? (device === 'gpu' ? (process.platform === 'win32' ? 'dml' : 'cuda') : options.provider);
+    const provider = options.provider ?? (device === 'gpu' ? (process.platform === 'win32' ? 'dml' : 'cuda') : undefined);
 
     this._pool = new WorkerPool(modelName, {
       poolSize: concurrency,
@@ -132,11 +132,11 @@ export class Embedder {
    */
   static async applyPerfProfile(profilePath, device) {
     const data = JSON.parse(fs.readFileSync(profilePath, 'utf8'));
-    const results = (data.results || []).filter((r) => r.success);
+    let results = (data.results || []).filter((r) => r.success);
     if (device) {
       const byDevice = results.filter((r) => r.device === device);
       if (byDevice.length) {
-        results.splice(0, results.length, ...byDevice);
+        results = byDevice;
       }
     }
     if (results.length === 0) throw new Error('No successful results in profile');
@@ -232,9 +232,8 @@ export class Embedder {
       // Run the bench/grid-search.js script and wait for it to finish.
       const script = join(process.cwd(), 'bench', 'grid-search.js');
       const out = profileOut ?? join(process.cwd(), 'bench', `grid-results-${device}-${Date.now()}.json`);
-      const cmd = `node "${script}" --device ${device} --sample-size ${sampleSize} --out "${out}"`;
       try {
-        execSync(cmd, { stdio: 'inherit' });
+        execFileSync(process.execPath, [script, '--device', device, '--sample-size', String(sampleSize), '--out', out], { stdio: 'inherit' });
       } catch (err) {
         throw new Error(`Grid search failed: ${err.message}`);
       }

src/model-management.js

@@ -11,6 +11,23 @@ async function exists(path) {
   }
 }
 
+/**
+ * Check whether a directory entry name matches a given model name.
+ * Matches exactly or with a '-' or '/' separator suffix to avoid
+ * overly-broad substring matches (e.g. 'bert' must not match 'roberta').
+ *
+ * @param {string} entryName  Directory name in the cache
+ * @param {string} modelName  Model name to match against
+ * @returns {boolean}
+ */
+function matchesModelName(entryName, modelName) {
+  return (
+    entryName === modelName ||
+    entryName.startsWith(modelName + '-') ||
+    entryName.startsWith(modelName + '/')
+  )
+}
+
 /**
  * Check whether a model appears to be present in the embedeer cache.
  * This is a best-effort check that looks for an entry matching the
@@ -25,11 +42,12 @@ export async function isModelDownloaded(modelName, opts = {}) {
   const target = join(dir, modelName)
   if (await exists(target)) return true
 
-  // Fallback: look for any directory whose name contains the modelName
+  // Fallback: look for any directory whose name matches the model name
+  // (exact or with a separator suffix) to avoid false positives.
   try {
     const entries = await fs.promises.readdir(dir, { withFileTypes: true })
     for (const e of entries) {
-      if (e.isDirectory() && e.name.includes(modelName)) return true
+      if (e.isDirectory() && matchesModelName(e.name, modelName)) return true
     }
   } catch (err) {
     // ignore and return false
@@ -177,10 +195,12 @@ export async function deleteModel(modelName, opts = {}) {
     return true;
   }
 
-  // Fallback: remove any directory whose name contains the modelName string
+  // Fallback: remove any directory whose name matches the model name
+  // (exact or with a separator suffix) to avoid accidentally deleting
+  // unrelated models.
   try {
     const entries = await fs.promises.readdir(dir, { withFileTypes: true });
-    const matches = entries.filter((e) => e.isDirectory() && e.name.includes(modelName));
+    const matches = entries.filter((e) => e.isDirectory() && matchesModelName(e.name, modelName));
     if (matches.length === 0) return false;
     for (const m of matches) {
       await fs.promises.rm(join(dir, m.name), { recursive: true, force: true });

src/provider-loader.js

@@ -15,7 +15,10 @@
  */
 
 import { execSync } from 'child_process';
-import { existsSync } from 'fs';
+import fs from 'fs';
+
+/** Cached output of `ldconfig -p` to avoid repeated subprocess calls. */
+let _ldconfigCache = null;
 
 // ── CUDA (linux/x64) ─────────────────────────────────────────────────────────
 
@@ -64,15 +67,17 @@ function cudaSearchDirs() {
  */
 function findLib(libName) {
   for (const dir of cudaSearchDirs()) {
-    if (existsSync(`${dir}/${libName}`)) return `${dir}/${libName}`;
+    if (fs.existsSync(`${dir}/${libName}`)) return `${dir}/${libName}`;
   }
   try {
-    const output = execSync('ldconfig -p', {
-      stdio: ['ignore', 'pipe', 'ignore'],
-      encoding: 'utf8',
-      timeout: 3000,
-    });
-    for (const line of output.split('\n')) {
+    if (_ldconfigCache === null) {
+      _ldconfigCache = execSync('ldconfig -p', {
+        stdio: ['ignore', 'pipe', 'ignore'],
+        encoding: 'utf8',
+        timeout: 3000,
+      });
+    }
+    for (const line of _ldconfigCache.split('\n')) {
       if (line.includes(libName) && line.includes('=>')) {
         const match = line.match(/=>\s*(.+)/);
         if (match) return match[1].trim();
@@ -91,8 +96,8 @@ function findLib(libName) {
  * @returns {Promise<void>}
  * @throws {Error} If NVIDIA GPU is not detected or required CUDA libraries are missing.
  */
-async function activateCuda() {
-  if (!existsSync('/dev/nvidiactl')) {
+function activateCuda() {
+  if (!fs.existsSync('/dev/nvidiactl')) {
     throw new Error(
       'No NVIDIA GPU detected (/dev/nvidiactl not found).\n' +
       'Ensure NVIDIA drivers are installed. Verify with: nvidia-smi',
@@ -124,7 +129,7 @@ async function activateCuda() {
  * @returns {Promise<void>}
  * @throws {Error} If not running on Windows.
  */
-async function activateDml() {
+function activateDml() {
   if (process.platform !== 'win32') {
     throw new Error(
       `DirectML is only available on Windows (current platform: ${process.platform}).`,

src/worker-pool.js

@@ -261,6 +261,18 @@ export class WorkerPool {
   _removeWorker(worker) {
     this.workers = this.workers.filter((w) => w !== worker);
     this.idleWorkers = this.idleWorkers.filter((w) => w !== worker);
+    // If all workers have crashed, reject any pending tasks rather than hanging.
+    if (this.workers.length === 0 && this._initialized && this.pendingTasks.length > 0) {
+      console.error('WorkerPool: all workers have crashed — rejecting all pending tasks');
+      const pending = this.pendingTasks.splice(0);
+      for (const task of pending) {
+        const cb = this.taskCallbacks.get(task.id);
+        if (cb) {
+          this.taskCallbacks.delete(task.id);
+          cb.reject(new Error('WorkerPool: all workers have crashed'));
+        }
+      }
+    }
   }
 }
 

test/embedder-grid.test.js

@@ -27,7 +27,8 @@ describe('Embedder.generateAndSaveProfile (grid mode)', () => {
 import child_process from 'child_process'
 import os from 'os'
 import fs from 'fs'
-// Avoid running the real benchmark script
+// Avoid running the real benchmark script (execFileSync after copilot security fix)
+child_process.execFileSync = () => {}
 child_process.execSync = () => {}
 // Use tmp homedir so we don't write to the user's real home
 os.homedir = () => ${JSON.stringify(tmp)}

test/provider-loader-cuda.test.js

@@ -35,11 +35,13 @@ describe('provider-loader (CUDA activation via runner)', () => {
       const providerLoaderFileUrl = pathToFileURL(providerLoaderPath).href
 
       const code = `
-import fs from 'fs'
+import { createRequire } from 'module'
 import process from 'process'
-const origExists = fs.existsSync
+const req = createRequire(import.meta.url)
+const fsModule = req('fs')
+const origExists = fsModule.existsSync
 // pretend /dev/nvidiactl exists on this test runner
-fs.existsSync = (p) => (p === '/dev/nvidiactl') || origExists(p)
+fsModule.existsSync = (p) => (p === '/dev/nvidiactl') || origExists(p)
 // point LD_LIBRARY_PATH to our temp dir so findLib sees the dummy libs
 process.env.LD_LIBRARY_PATH = ${JSON.stringify(tmp)}
 const mod = await import(${JSON.stringify(providerLoaderFileUrl)})