From fe3d82e280ee6faa5234417b5eab0a28b4027ec0 Mon Sep 17 00:00:00 2001 From: wakqasahmed Date: Mon, 7 Sep 2026 10:30:24 +0200 Subject: [PATCH] fix(install): derive dependency versions from package.json, harden fixture isolation (#2822) Addresses Greptile's review on #2994: - missing-dependency.js no longer hardcodes a second copy of the four runtime dependency versions; it reads them from package.json's dependencies field instead, so the two can't silently drift apart. describeMissingDependencyError() still recognizes a tracked dependency even if package.json can't be read for some reason, just without a version-pinned install command in that case. - The regression test now asserts no ancestor directory of its temp fixture has a node_modules, so a stray one wouldn't let Node resolve ajv/sql.js from there and mask what the test is actually meant to exercise. Also copies package.json into the fixture, matching a real plugin-marketplace git clone and what the version-lookup above now needs. --- scripts/lib/missing-dependency.js | 53 ++++++++++++++----- ...lugin-install-without-node-modules.test.js | 26 ++++++++- 2 files changed, 65 insertions(+), 14 deletions(-) diff --git a/scripts/lib/missing-dependency.js b/scripts/lib/missing-dependency.js index 7292cd9d5..d75334714 100644 --- a/scripts/lib/missing-dependency.js +++ b/scripts/lib/missing-dependency.js @@ -1,15 +1,38 @@ 'use strict'; -// Production dependencies declared in package.json's "dependencies" field. -// `npm install` never runs when ECC is installed via the Claude Code plugin -// marketplace (a plain git clone), so these can be missing at runtime even -// though the code that needs them is fine. -const RUNTIME_DEPENDENCY_VERSIONS = { - ajv: '8.20.0', - 'sql.js': '1.14.2', - 'js-yaml': '4.3.1', - '@iarna/toml': '2.2.5', -}; +const fs = require('fs'); +const path = require('path'); + +// Runtime dependencies that can be missing when ECC is installed via the +// Claude Code plugin marketplace (a plain git clone, so `npm install` never +// runs), even though the code that needs them is fine. Versions are read +// straight from package.json's "dependencies" field instead of a second +// hardcoded copy, so this can't silently drift out of sync with what's +// actually declared there. +const TRACKED_DEPENDENCIES = ['ajv', 'sql.js', 'js-yaml', '@iarna/toml']; + +function loadRuntimeDependencyVersions() { + try { + const packageJsonPath = path.join(__dirname, '..', '..', 'package.json'); + const declared = JSON.parse(fs.readFileSync(packageJsonPath, 'utf8')).dependencies || {}; + + const versions = {}; + for (const name of TRACKED_DEPENDENCIES) { + const declaredVersion = declared[name]; + if (declaredVersion) { + versions[name] = declaredVersion.replace(/^[\^~]/, ''); + } + } + return versions; + } catch { + // package.json isn't reachable from here for some reason. Fall back to + // an empty map rather than crash — describeMissingDependencyError() + // just won't be able to suggest a pinned version in that case. + return {}; + } +} + +const RUNTIME_DEPENDENCY_VERSIONS = loadRuntimeDependencyVersions(); function describeMissingDependencyError(error) { if (!error || error.code !== 'MODULE_NOT_FOUND') { @@ -18,17 +41,21 @@ function describeMissingDependencyError(error) { const match = /Cannot find module '([^']+)'/.exec(error.message || ''); const moduleName = match && match[1]; - const pinnedVersion = moduleName && RUNTIME_DEPENDENCY_VERSIONS[moduleName]; - if (!pinnedVersion) { + if (!moduleName || !TRACKED_DEPENDENCIES.includes(moduleName)) { return null; } + const pinnedVersion = RUNTIME_DEPENDENCY_VERSIONS[moduleName]; + const installCommand = pinnedVersion + ? `npm install --no-save ${moduleName}@${pinnedVersion}` + : `npm install --no-save ${moduleName}`; + return ( `Missing dependency '${moduleName}'. ECC's production dependencies aren't installed ` + '(this happens when ECC was installed via the Claude Code plugin marketplace, which ' + 'clones the repo but never runs npm install). Run "npm install" from the ECC repo ' + - `root, or install just this package with "npm install --no-save ${moduleName}@${pinnedVersion}".` + `root, or install just this package with "${installCommand}".` ); } diff --git a/tests/scripts/plugin-install-without-node-modules.test.js b/tests/scripts/plugin-install-without-node-modules.test.js index b89ab74e2..6f7d5ce26 100644 --- a/tests/scripts/plugin-install-without-node-modules.test.js +++ b/tests/scripts/plugin-install-without-node-modules.test.js @@ -30,11 +30,31 @@ function test(name, fn) { } function copyRuntimeFiles(destDir) { - for (const entry of ['scripts', 'schemas', 'manifests']) { + for (const entry of ['scripts', 'schemas', 'manifests', 'package.json']) { fs.cpSync(path.join(REPO_ROOT, entry), path.join(destDir, entry), { recursive: true }); } } +// Node's module resolution walks up the directory tree looking for +// node_modules, so if any ancestor of pluginDir happened to have one, a +// require('ajv') from inside pluginDir could resolve there instead of +// hitting the MODULE_NOT_FOUND path this test exists to exercise. Confirm +// the fixture is actually isolated before trusting any of the results below. +function assertNoNodeModulesInAncestry(dir) { + let current = dir; + while (true) { + if (fs.existsSync(path.join(current, 'node_modules'))) { + throw new Error( + `Fixture is not isolated: ${path.join(current, 'node_modules')} exists, so this test ` + + 'would resolve dependencies from there instead of exercising the missing-dependency path.' + ); + } + const parent = path.dirname(current); + if (parent === current) break; + current = parent; + } +} + function run(scriptRelativePath, args, cwd) { try { const stdout = execFileSync('node', [path.join(cwd, scriptRelativePath), ...args], { @@ -63,6 +83,10 @@ function runTests() { const pluginDir = fs.mkdtempSync(path.join(os.tmpdir(), 'ecc-plugin-install-')); try { + if (test('fixture has no node_modules anywhere in its ancestor chain', () => { + assertNoNodeModulesInAncestry(pluginDir); + })) passed++; else failed++; + copyRuntimeFiles(pluginDir); if (test('install-plan.js --list-profiles runs without ajv installed', () => {