fix pos-cli module update

CHANGELOG.md

@@ -2,6 +2,14 @@
 
 ## Unreleased
 
+### Fixes
+
+* `pos-cli modules install/update` no longer leaves a module directory whose manifest claims a version its files don't match — the state behind "the version was bumped but the code is stale and a file is missing". Archives are now unpacked into a throwaway staging directory under `tmp/` and swapped into `modules/<name>` only once complete, so an interrupted install leaves either the old module or no module, and both are correctly re-downloaded next run. The swap moves the old directory aside rather than unpacking over it, so files a new version no longer ships are actually removed. Previously the module directory was deleted and the archive unpacked over it in place: an interruption left a partial tree, and because staleness is detected by reading the version out of the module's own `pos-module.json`, a partial tree that had already written that file looked up-to-date to every later `install` and `update` — the stale code then shipped on the next deploy.
+* `pos-cli modules install/update` now rejects an archive that does not contain the expected `<module-name>/` directory. Such an archive used to delete `modules/<module-name>`, unpack its differently-named root beside it, and still report success.
+* `pos-cli modules install/update` waits for every module download to settle before reporting a failure. A failing module used to abort the command while its siblings were still being replaced on disk, so a Ctrl-C at the error prompt could interrupt an install that appeared to be over. All download failures are now reported together instead of only the first.
+* `pos-module.lock.json` is written only after every module has downloaded successfully. Writing it up front recorded versions that were never installed, and the next run — seeing the lock and the modules that *did* download agree — had no way to tell the install was incomplete.
+* File downloads (module archives, data exports, `pos-cli pull`) now fail on a non-2xx response instead of saving the error body as the downloaded file, and follow redirects. An expired presigned URL answers `403` with an XML body, which used to be written out as a `.zip` and only surfaced later as a corrupt-archive error. Signed query strings are stripped from error messages.
+
 ## 6.3.0 (2026-07-27)
 
 ### New Features

CLAUDE.md

@@ -283,6 +283,10 @@ project/
 
 Run all commands from project root (one level above `app/` or `modules/`).
 
+**Module staging** (`lib/modules/staging.js`). `pos-cli modules install/update` never downloads or extracts into an installed module directory. Each archive is fetched into its own throwaway staging directory under `tmp/pos-cli-module-staging/` and unpacked there; only after it is verified to contain the expected `<name>/` root is it published with two renames — the old `modules/<name>` moves into the staging directory, then the staged tree is renamed onto `modules/<name>`. Moving the old tree aside wholesale (rather than unpacking over it) is what makes files deleted between two versions actually disappear; the second rename being atomic is what guarantees `modules/<name>` is never observed half-written. A half-written module whose `pos-module.json` already reports the target version is indistinguishable from an up-to-date one (see `modulesNotOnDisk`), so it would never be repaired.
+
+Staging lives under the project's `tmp/` (already pos-cli's scratch area — deploy writes `tmp/release.zip` there) for two reasons: publishing ends in a `rename()` onto `modules/<name>`, which fails with `EXDEV` across filesystems, so `os.tmpdir()` is not safe to use; and nothing enumerates project-root `tmp/`, since every glob over modules runs with `cwd` set to `modules/` and sync only watches `dir.toWatch()`. A staging directory therefore cannot be deployed, packed, or synced by construction, with no per-enumerator exclusions to maintain.
+
 #### API Architecture
 Main endpoints (`${INSTANCE_URL}/api/app_builder/`):
 - `/marketplace_releases` (POST) - Deploy archive

bin/pos-cli-data-export.js

@@ -64,7 +64,15 @@ program
     };
 
     const handleZipFileExport = (exportTask, filename) => {
-      downloadFile(exportTask.zip_file_url, filename).then(exportFinished);
+      // Not chained into the caller's promise, so it needs its own handler: downloadFile
+      // rejects on a non-2xx response (e.g. an expired presigned URL).
+      downloadFile(exportTask.zip_file_url, filename)
+        .then(exportFinished)
+        .catch((e) => {
+          spinner.fail('Export failed');
+          logger.Error(e.message);
+          report('[ERR] Data: Export - Failed');
+        });
     };
 
     const handleJsonFileExport = (exportTask, filename) => {

lib/downloadFile.js

@@ -1,20 +1,51 @@
 import fs from 'fs';
-import https from 'https';
-import http from 'http';
+import { Readable } from 'stream';
+import { pipeline } from 'stream/promises';
 
-const downloadFile = (url, fileName) => {
-  return new Promise((resolve, reject) => {
-    let file = fs.createWriteStream(fileName).on('close', () => resolve());
-    const request = url.startsWith('https') ? https : http;
-    request.get(url, response => {
-      response.pipe(file);
-      file
-        .on('finish', () => {
-          file.close(resolve);
-        })
-        .on('error', reject);
+/**
+ * Download URLs are frequently presigned S3 links whose query string carries the
+ * signature. Strip it before putting a URL in an error message.
+ */
+const safeUrl = (url) => {
+  try {
+    const { origin, pathname } = new URL(url);
+    return `${origin}${pathname}`;
+  } catch {
+    return url;
+  }
+};
+
+/**
+ * Downloads `url` to `fileName`.
+ *
+ * Rejects on any non-2xx response instead of writing the error body to disk: an
+ * expired presigned URL answers 403 with an XML body, and silently saving that as
+ * a .zip turned an auth failure into a corrupt-archive error further down.
+ *
+ * Rejections carry `statusCode` so callers can distinguish 404 from other failures.
+ */
+const downloadFile = async (url, fileName) => {
+  let response;
+
+  try {
+    response = await fetch(url); // follows redirects, and caps the chain itself
+  } catch (error) {
+    // Wrapped rather than rethrown: fetch's own message can echo the whole signed URL,
+    // and `cause` keeps the network error code reachable (see ServerError).
+    throw new Error(`Download failed for ${safeUrl(url)}: ${error.cause?.message ?? error.message}`, {
+      cause: error
     });
-  });
+  }
+
+  if (!response.ok) {
+    const error = new Error(`Download failed with HTTP ${response.status}: ${safeUrl(url)}`);
+    error.statusCode = response.status;
+    throw error;
+  }
+
+  // pipeline (unlike pipe) forwards source errors, so a connection dropped mid-download
+  // rejects instead of leaving this promise pending forever.
+  await pipeline(Readable.fromWeb(response.body), fs.createWriteStream(fileName));
 };
 
 export default downloadFile;

lib/modules/downloadModule.js

@@ -1,73 +1,153 @@
-import { randomUUID } from 'crypto';
 import logger from '../logger.js';
 import downloadFile from '../downloadFile.js';
 import { unzip } from '../unzip.js';
 import Portal from '../portal.js';
 import fs from 'fs';
 import path from 'path';
-import os from 'os';
-import { getModulesDir, getModulePath, POS_MODULE_FILE, TEMPLATE_VALUES_FILE } from './paths.js';
+import { getModulePath, POS_MODULE_FILE, TEMPLATE_VALUES_FILE } from './paths.js';
+import { createStagingDir, clearStagingBase } from './staging.js';
 import { safeReadFile } from './postInstall.js';
 
+/** Name the downloaded archive gets inside the module's own staging directory. */
+const ARCHIVE_FILE = 'archive.zip';
+
+/**
+ * Checks that a freshly extracted staging directory contains the expected
+ * `<moduleName>/` root, and fails loudly when it does not.
+ *
+ * Without this check a mismatched archive silently wipes `modules/<moduleName>`
+ * (it is replaced by whatever directory the archive did contain) while the command
+ * still reports success.
+ */
+const verifyStagedModule = (moduleName, version, stagingDir) => {
+  const extracted = fs.readdirSync(stagingDir).filter(entry => entry !== ARCHIVE_FILE);
+
+  if (!extracted.includes(moduleName)) {
+    const detail = extracted.length > 0 ? `archive contains: ${extracted.join(', ')}` : 'the archive is empty';
+    throw new Error(`archive does not contain a "${moduleName}/" directory (${detail})`);
+  }
+
+  // A module whose own manifest disagrees with the version it was published under
+  // is installed anyway (the registry version is authoritative), but it is worth
+  // surfacing: readInstalledVersion will keep flagging it as stale on every run.
+  const stagedVersion = readVersionFromDir(path.join(stagingDir, moduleName));
+  if (stagedVersion && stagedVersion !== version) {
+    logger.Warn(
+      `${moduleName}: published as ${version} but its manifest declares ${stagedVersion}. ` +
+      `The module will be re-downloaded on every install until the two agree.`
+    );
+  }
+};
+
 /**
- * Downloads and extracts a single module archive.
+ * Publishes a verified, fully extracted module to `modules/<moduleName>` with two renames:
+ * the old tree moves aside into the staging directory, then the staged tree takes its place.
  *
- * @param {string}   moduleName    Module name (e.g. "core").
- * @param {string}   version       Exact version to download.
- * @param {string}   [registryUrl] Registry URL for the download request.
- * @param {Function} [fetchVersions] Optional: replaces Portal.moduleVersionsSearch for testing.
- *                                  Signature: (moduleWithVersion, registryUrl) => Promise<{ public_archive: string }>
+ * The old tree has to leave wholesale rather than be written over, because a file deleted
+ * between the two versions would otherwise survive the update. And only the second rename
+ * publishes anything, so an interrupted swap leaves either the old module or none at all —
+ * never a half-written tree whose manifest already claims the new version, which
+ * modulesNotOnDisk would then treat as up-to-date forever.
+ *
+ * The displaced copy stays inside `stagingDir` so the caller's cleanup removes it along
+ * with everything else, rather than needing its own cleanup path.
  */
-const downloadModule = async (moduleName, version, registryUrl, fetchVersions = null) => {
-  const fetcher = fetchVersions ?? Portal.moduleVersionsSearch.bind(Portal);
+const swapIntoPlace = async (moduleName, stagingDir) => {
+  const stagedPath = path.join(stagingDir, moduleName);
+  const modulePath = getModulePath(moduleName);
+  const backupPath = path.join(stagingDir, `old-${moduleName}`);
+
+  await fs.promises.mkdir(path.dirname(modulePath), { recursive: true });
+  const hadPrevious = fs.existsSync(modulePath);
+  if (hadPrevious) await fs.promises.rename(modulePath, backupPath);
+
+  try {
+    await fs.promises.rename(stagedPath, modulePath);
+  } catch (error) {
+    // Best effort: a failed rollback must not mask why the swap failed. Worst case the
+    // module is left absent, which the next run re-downloads.
+    if (hadPrevious) await fs.promises.rename(backupPath, modulePath).catch(() => {});
+    throw error;
+  }
+};
+
+/**
+ * Downloads a single module archive and installs it atomically.
+ *
+ * @param {string} moduleName    Module name (e.g. "core").
+ * @param {string} version       Exact version to download.
+ * @param {string} [registryUrl] Registry URL for the download request.
+ */
+const downloadModule = async (moduleName, version, registryUrl) => {
   const moduleWithVersion = `${moduleName}@${version}`;
-  // randomUUID() avoids temp-file collisions under concurrent installs of the same module.
-  const tmpFile = path.join(os.tmpdir(), `pos-module-${moduleName}-${randomUUID()}.zip`);
+  let stagingDir;
+
   try {
     logger.Info(`Downloading ${moduleWithVersion}...`);
-    const moduleVersion = await fetcher(moduleWithVersion, registryUrl);
-    await downloadFile(moduleVersion['public_archive'], tmpFile);
-    // Remove old dir only after download succeeds — keeps the module directory
-    // intact if the network/registry call fails mid-stream.
-    await fs.promises.rm(getModulePath(moduleName), { recursive: true, force: true });
-    await unzip(tmpFile, getModulesDir());
+    const moduleVersion = await Portal.moduleVersionsSearch(moduleWithVersion, registryUrl);
+    // Archive and extraction both live in the staging directory, so `modules/<name>` is
+    // only touched once a complete, verified copy is ready to swap in — and one cleanup
+    // covers both.
+    stagingDir = await createStagingDir(moduleName);
+    const archive = path.join(stagingDir, ARCHIVE_FILE);
+    await downloadFile(moduleVersion['public_archive'], archive);
+    await unzip(archive, stagingDir);
+    verifyStagedModule(moduleName, version, stagingDir);
+    await swapIntoPlace(moduleName, stagingDir);
   } catch (error) {
     throw new Error(`${moduleWithVersion}: ${error.statusCode === 404 ? '404 not found' : error.message}`);
   } finally {
-    await fs.promises.rm(tmpFile, { force: true });
+    if (stagingDir) await fs.promises.rm(stagingDir, { recursive: true, force: true });
   }
 };
 
 /**
+ * Downloads every module in `modules` concurrently.
+ *
+ * Uses allSettled rather than Promise.all so a single failure never leaves sibling
+ * downloads running unsupervised — reporting failure while other modules are still being
+ * replaced on disk means a Ctrl-C at that prompt can leave one half-installed. All
+ * failures are collected and reported together.
+ *
  * @param {Object}   modules        { name: version } map of modules to download.
  * @param {Function} getRegistryUrl (name) => registryUrl — called per module so each
  *                                  can be fetched from its own registry.
- * @param {Function} [fetchVersions] Optional: injected fetcher forwarded to downloadModule.
- *                                  Useful for testing without a global Portal mock.
  */
-const downloadAllModules = async (modules, getRegistryUrl, fetchVersions = null) => {
-  await Promise.all(
+const downloadAllModules = async (modules, getRegistryUrl) => {
+  const settled = await Promise.allSettled(
     Object.entries(modules).map(([moduleName, version]) =>
-      downloadModule(moduleName, version, getRegistryUrl(moduleName), fetchVersions)
+      downloadModule(moduleName, version, getRegistryUrl(moduleName))
     )
   );
+
+  // Best effort: also clears staging leftovers from a previously killed run. Safe here
+  // because every download of this run has settled.
+  await clearStagingBase();
+
+  const failures = settled.filter(r => r.status === 'rejected');
+  if (failures.length > 0) {
+    const messages = failures.map(f => f.reason?.message ?? String(f.reason));
+    throw new Error(
+      failures.length === 1
+        ? messages[0]
+        : `Failed to download ${failures.length} modules:\n  ${messages.join('\n  ')}`
+    );
+  }
 };
 
 const readJsonVersion = (filePath) => safeReadFile(filePath, (raw) => JSON.parse(raw).version ?? null);
 
 /**
- * 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).
+ * Reads the `version` recorded in a module directory's own manifest: pos-module.json,
+ * falling back to 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.
  */
-const readInstalledVersion = (name) => {
-  const dir = getModulePath(name);
-  return readJsonVersion(path.join(dir, POS_MODULE_FILE)) ?? readJsonVersion(path.join(dir, TEMPLATE_VALUES_FILE));
-};
+const readVersionFromDir = (dir) =>
+  readJsonVersion(path.join(dir, POS_MODULE_FILE)) ?? readJsonVersion(path.join(dir, TEMPLATE_VALUES_FILE));
+
+/** The same, for an installed module: `modules/<name>`. null means "not installed". */
+const readInstalledVersion = (name) => readVersionFromDir(getModulePath(name));
 
 /**
  * Returns the subset of modules whose installed disk version does not match
@@ -76,10 +156,14 @@ const readInstalledVersion = (name) => {
  * 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.
+ * modules whose directory exists but whose contents are stale — e.g. deleted
+ * manually then recreated empty, or simply never updated after the lock file
+ * itself was bumped (by a teammate, a merge, etc.) without the module directory
+ * being refreshed locally.
+ *
+ * Note this can only detect staleness a module's own manifest admits to. Keeping
+ * installs atomic (see swapIntoPlace) is what guarantees a directory's contents
+ * actually match the version its manifest reports.
  */
 const modulesNotOnDisk = (modules) =>
   Object.fromEntries(
@@ -98,4 +182,10 @@ const modulesToDownload = (modulesLocked, previousLock) => ({
   ...modulesNotOnDisk(modulesLocked),
 });
 
-export { downloadModule, downloadAllModules, modulesToDownload, modulesNotOnDisk, readInstalledVersion };
+export {
+  downloadModule,
+  downloadAllModules,
+  modulesToDownload,
+  modulesNotOnDisk,
+  readInstalledVersion,
+};

lib/modules/orchestrator.js

@@ -88,13 +88,9 @@ const resolveAndDownload = async (spinner, prodModules, devModules = {}, registr
     (!includeDev || isLockUnchanged(resolvedDev, prevDev)) &&
     isLockUnchanged(mergedRegistries, previousLock.registries);
 
-  if (lockUnchanged) {
-    spinner.succeed('Module dependencies up-to-date');
-  } else {
-    writePosModulesLock(resolvedProd, lockDevToWrite, mergedRegistries);
-    spinner.succeed(`Modules lock file updated: ${POS_MODULE_LOCK_FILE}`);
-  }
+  spinner.succeed(lockUnchanged ? 'Module dependencies up-to-date' : 'Module dependencies resolved');
 
+  // Computed before downloading: modulesToDownload inspects what is currently on disk.
   const toDownload = {
     ...modulesToDownload(resolvedProd, prevProd),
     ...(includeDev ? modulesToDownload(resolvedDev, prevDev) : {}),
@@ -106,6 +102,14 @@ const resolveAndDownload = async (spinner, prodModules, devModules = {}, registr
   await downloadAllModules(toDownload, getRegistryUrl);
   spinner.succeed(`Modules downloaded successfully${skipNote}`);
 
+  // Written only after every download succeeded. Writing it earlier recorded versions
+  // that were never installed, and the next run — seeing lock and disk agree on the
+  // modules that did download — had no way to tell the install was incomplete.
+  if (!lockUnchanged) {
+    writePosModulesLock(resolvedProd, lockDevToWrite, mergedRegistries);
+    spinner.succeed(`Modules lock file updated: ${POS_MODULE_LOCK_FILE}`);
+  }
+
   const allPrevious = includeDev ? { ...prevProd, ...prevDev } : prevProd;
   printDiff(allPrevious, allResolved);