fix pos-cli modules install default behaviour for stale packages

lib/modules/downloadModule.js

@@ -6,7 +6,8 @@ import Portal from '../portal.js';
 import fs from 'fs';
 import path from 'path';
 import os from 'os';
-import { getModulesDir, getModulePath } from './paths.js';
+import { getModulesDir, getModulePath, POS_MODULE_FILE, TEMPLATE_VALUES_FILE } from './paths.js';
+import { safeReadFile } from './postInstall.js';
 
 /**
  * Downloads and extracts a single module archive.
@@ -52,32 +53,49 @@ const downloadAllModules = async (modules, getRegistryUrl, fetchVersions = null)
   );
 };
 
+const readJsonVersion = (filePath) => safeReadFile(filePath, (raw) => JSON.parse(raw).version ?? null);
+
 /**
- * Returns the subset of modulesLocked that actually needs to be downloaded.
- *
- * A module is skipped when BOTH of the following are true:
- *  1. Its version in previousLock matches the newly resolved version (no change).
- *  2. Its directory already exists on disk (a previous download succeeded).
- *
- * The disk check catches the case where the lock file is up-to-date but the
- * module directory was deleted manually — in that case we must re-download.
+ * Reads the `version` field recorded in an installed module's own manifest:
+ * modules/<name>/pos-module.json, falling back to modules/<name>/template-values.json
+ * for modules published before the pos-module.json convention existed (many
+ * currently-published registry modules still ship this way). Returns null when
+ * neither file exists, is readable, or carries a `version` field — treated the
+ * same as "not installed" by callers. Uses the same safe-read primitive as
+ * postInstall.js's module-manifest lookups (postInstall.js's safeReadFile).
  */
-const modulesToDownload = (modulesLocked, previousLock) =>
-  Object.fromEntries(
-    Object.entries(modulesLocked).filter(([name, version]) => {
-      if (previousLock[name] !== version) return true;
-      return !fs.existsSync(getModulePath(name));
-    })
-  );
+const readInstalledVersion = (name) => {
+  const dir = getModulePath(name);
+  return readJsonVersion(path.join(dir, POS_MODULE_FILE)) ?? readJsonVersion(path.join(dir, TEMPLATE_VALUES_FILE));
+};
 
 /**
- * Returns the subset of modules whose directory is missing from disk.
- * Used by --frozen mode where the lock is already the source of truth and
- * there is no "previous lock" to compare versions against.
+ * Returns the subset of modules whose installed disk version does not match
+ * the target version — including modules missing from disk entirely. Used by
+ * --frozen mode (and smartInstall's fast path) where the lock is already the
+ * source of truth and there is no "previous lock" to compare versions against.
+ *
+ * Checking installed disk version rather than mere directory presence catches
+ * modules whose directory exists but whose contents are stale or corrupted —
+ * e.g. deleted manually then recreated empty, a failed/partial extraction, or
+ * simply never updated after the lock file itself was bumped (by a teammate,
+ * a merge, etc.) without the module directory being refreshed locally.
  */
 const modulesNotOnDisk = (modules) =>
   Object.fromEntries(
-    Object.entries(modules).filter(([name]) => !fs.existsSync(getModulePath(name)))
+    Object.entries(modules).filter(([name, version]) => readInstalledVersion(name) !== version)
   );
 
-export { downloadModule, downloadAllModules, modulesToDownload, modulesNotOnDisk };
+/**
+ * Returns the subset of modulesLocked that actually needs to be downloaded:
+ * everything modulesNotOnDisk flags, plus any module whose version in
+ * previousLock no longer matches the newly resolved version.
+ */
+const modulesToDownload = (modulesLocked, previousLock) => ({
+  ...Object.fromEntries(
+    Object.entries(modulesLocked).filter(([name, version]) => previousLock[name] !== version)
+  ),
+  ...modulesNotOnDisk(modulesLocked),
+});
+
+export { downloadModule, downloadAllModules, modulesToDownload, modulesNotOnDisk, readInstalledVersion };

lib/modules/orchestrator.js

@@ -144,8 +144,9 @@ const isLockValidForInstall = (lock, prodModules, devModules, includeDev) =>
  *  - any dep in pos-module.json is absent from the lock file (lock is stale)
  *  - any locked version does not satisfy the constraint declared in pos-module.json
  *
- * Downloads only modules that are missing from disk; already-present modules
- * at the locked version are skipped, making it safe to cache `modules/` in CI.
+ * Downloads only modules that are missing from disk or whose installed version
+ * doesn't match the lock; already-present modules at the locked version are
+ * skipped, making it safe to cache `modules/` in CI.
  *
  * @param {ora.Ora} spinner
  * @param {Object}  prodModules              - dependencies from pos-module.json

lib/modules/paths.js

@@ -6,6 +6,7 @@
 import path from 'path';
 
 export const POS_MODULE_FILE = 'pos-module.json';
+export const TEMPLATE_VALUES_FILE = 'template-values.json';
 export const POS_MODULE_LOCK_FILE = 'pos-module.lock.json';
 export const LEGACY_POS_MODULES_FILE = 'app/pos-modules.json';
 export const LEGACY_POS_MODULES_LOCK_FILE = 'app/pos-modules.lock.json';

lib/modules/postInstall.js

@@ -63,7 +63,8 @@ const normalize = (raw) => {
  * Reads and transforms a file, returning null when it is absent or unreadable.
  * A single read (no existsSync pre-check) handles the missing-file case via the
  * ENOENT catch; any other read/parse failure is logged at Debug and treated as
- * "no message" so a broken module never aborts the install.
+ * absent, so a broken or missing file never aborts the caller (post-install
+ * messages, installed-version lookups, etc.).
  */
 const safeReadFile = (filePath, transform) => {
   try {
@@ -134,4 +135,4 @@ const printPostInstallMessages = (moduleNames, { isTTY = Boolean(process.stdout.
   return printed;
 };
 
-export { readPostInstall, printPostInstallMessages, normalize, stripUnsafe };
+export { readPostInstall, printPostInstallMessages, normalize, stripUnsafe, safeReadFile };

lib/modules/version.js

@@ -6,10 +6,9 @@ import files from '../files.js';
 import logger from '../logger.js';
 import report from '../logger/report.js';
 import { moduleConfig } from '../modules.js';
-import { POS_MODULE_FILE as moduleManifestFileName } from './paths.js';
+import { POS_MODULE_FILE as moduleManifestFileName, TEMPLATE_VALUES_FILE } from './paths.js';
 
 const BUMP_TYPES = ['major', 'minor', 'patch'];
-const TEMPLATE_VALUES_FILE = 'template-values.json';
 
 const templateValuesPath = (moduleName) =>
   path.join('modules', moduleName, TEMPLATE_VALUES_FILE);

package.json

@@ -8,12 +8,12 @@
     "#test/*": "./test/*"
   },
   "scripts": {
-    "pretest": "npm install --prefix test/fixtures/yeoman --silent",
+    "pretest": "npm install --prefix test/fixtures/yeoman --silent && npm install --prefix test/fixtures/yeoman/custom --silent",
     "test": "vitest run",
     "test:watch": "vitest",
     "test:coverage": "vitest run --coverage",
     "test:coverage:watch": "vitest --coverage",
-    "pretest:unit": "npm install --prefix test/fixtures/yeoman --silent",
+    "pretest:unit": "npm install --prefix test/fixtures/yeoman --silent && npm install --prefix test/fixtures/yeoman/custom --silent",
     "test:unit": "vitest run test/unit",
     "test:integration": "vitest run test/integration",
     "test:mcp-min": "vitest run mcp-min/__tests__",