From 910d0d153b178d11f6915ddb8c0f2681e07adf20 Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Fri, 11 Sep 2026 09:21:02 -0600 Subject: [PATCH 1/3] Honor install script policy in every install path Apply npm's ignore-scripts configuration to custom component install commands when scripts are disallowed, and let install_node_modules pass its explicit policy through the shared argument builder without changing its historical default. Co-Authored-By: GPT-5 Codex --- DESIGN.md | 28 ++ components/Application.ts | 43 ++- .../components/applicationInstall.test.js | 266 ++++++++++++++++++ unitTests/utility/npmUtilities.test.js | 188 +++++++++++++ utility/npmUtilities.ts | 15 + 5 files changed, 536 insertions(+), 4 deletions(-) create mode 100644 unitTests/components/applicationInstall.test.js create mode 100644 unitTests/utility/npmUtilities.test.js diff --git a/DESIGN.md b/DESIGN.md index c889c2e236..0654ba27d1 100644 --- a/DESIGN.md +++ b/DESIGN.md @@ -296,6 +296,34 @@ A package-manager timeout must not release this lock while npm descendants are s Boot's `harper-application-lock.json` records an application configuration only after preparation fulfills. Recording at queue time would make a failed install look complete and suppress its retry on the next boot. +<<<<<<< HEAD +======= +Every npm install Harper invokes directly — automatic component installation and the deprecated +`install_node_modules` operation alike — composes its arguments in `packageManagerInstallArguments()`, +which is production-only and adds `--omit=dev --no-audit --no-fund`. `--no-audit` is load-bearing, not +hygiene: npm 10 puts even a `file:` link into its audit bulk request, and the registry's answer to that +is unbounded from Harper's side. The operation accepts the established `install_allow_scripts` +spelling (and `allowInstallScripts` for compatibility), defaulting to its historical `true`; false +reaches the shared builder and adds `--ignore-scripts`. +`installApplication()` skips the package-manager child entirely when the root manifest declares no +production dependencies, non-empty workspaces, or enabled install lifecycle. An explicitly selected +non-npm manager still runs so it can discover workspace configuration outside `package.json`, and it +retains its own install defaults. A configured `install_command` remains the explicit escape hatch for +build-time tooling, but not for lifecycle-script policy: unless `install_allow_scripts` is true, its +spawn gets `npm_config_ignore_scripts=true`, which covers npm nested anywhere in the command without +adding an argument that could break non-npm tooling. The setting uses npm's configuration namespace; +other package managers that consume `npm_config_*` options can honor it too. When the policy is +omitted, Harper warns that package lifecycle scripts—including `npm run` pre/post hooks—are suppressed +and names both the operations-API and root-config opt-ins. +`readInstalledPackageMetadata()` must use the same automatic-work predicate so a +dev-only npm manifest does not force a restart on every redeploy for lacking a lockfile while an +explicit non-npm workspace install still does. Absolute local archives are classified before +package-protocol detection: a Windows drive letter's colon is path syntax, not an npm protocol. File +type detection remains asynchronous in extraction. Bare absolute Windows directory inputs retain +npm's copy/pack behavior rather than becoming live links; explicit `file:` and relative directory +inputs retain their existing symlink behavior. + +>>>>>>> 7f0d5a08c (Honor install script policy in every install path) ## Peer-side deploy_component payload read: retryable blob stalls and `Readable.from()` cancellation `readPayloadBlobWithRetry` (`components/deploymentRecorder.ts`) wraps the peer's read of a replicated `hdb_deployment` row's `payload_blob` so a transient 503 `BlobReadError` (`BLOB_UNAVAILABLE_STATUS`, `resources/blob.ts`) — content bytes not arriving within `blobReadTimeout`, e.g. a parked blob send on the origin — retries instead of failing the whole deploy. Two non-obvious constraints shaped the design: diff --git a/components/Application.ts b/components/Application.ts index a6f269a0a2..766948d645 100644 --- a/components/Application.ts +++ b/components/Application.ts @@ -888,8 +888,15 @@ export async function installApplication(application: Application) { // If node_modules doesn't exist, we need to install dependencies } + const allowInstallScripts = !!application.install?.allowInstallScripts; + // If custom install command is specified, run it if (application.install?.command) { + if (application.install.allowInstallScripts === undefined) { + application.logger.warn( + `Application ${application.name} uses install_command without install_allow_scripts; package lifecycle scripts are disabled by default for npm and tools that honor npm_config_ignore_scripts, including npm run pre/post hooks. Set install_allow_scripts (or install.allowInstallScripts in root config) to true to opt in` + ); + } const [command, ...args] = application.install.command.split(' '); const customOnLine = application.onInstallLine ? (stream: 'stdout' | 'stderr', line: string) => application.onInstallLine!(command, stream, line) @@ -901,7 +908,9 @@ export async function installApplication(application: Application) { application.dirPath, application.install?.timeout, customOnLine, - application.npmUserconfigPath + application.npmUserconfigPath, + undefined, + !allowInstallScripts ); // if it succeeds, return if (code === 0) { @@ -921,6 +930,23 @@ export async function installApplication(application: Application) { ); } +<<<<<<< HEAD +======= + const { packageManager } = packageJSON.devEngines || {}; + if (dependencyFieldHasWork(packageJSON, 'devDependencies')) { + application.logger.warn( + `Application ${application.name} declares devDependencies; automatic npm installation omits them, while explicitly selected non-npm package managers retain their own install defaults. Use install_command when deployment requires custom behavior` + ); + } + if ( + !packageHasAutomaticInstallWork(packageJSON) && + !(allowInstallScripts && packageHasAllowedInstallLifecycleWork(packageJSON)) + ) { + application.logger.info(`Application ${application.name} has no production package work; skipping install`); + return; + } + +>>>>>>> 7f0d5a08c (Honor install script policy in every install path) // Next, try package.json devEngines field const { packageManager } = packageJSON.devEngines || {}; @@ -1695,7 +1721,8 @@ export async function nonInteractiveSpawn( timeoutMs: number = DEFAULT_COMMAND_TIMEOUT_MS, onLine?: (stream: 'stdout' | 'stderr', line: string) => void, npmUserconfigPath?: string, - gitCredentialEnv?: Record + gitCredentialEnv?: Record, + ignoreNpmScripts = false ): Promise<{ stdout: string; stderr: string; code: number }> { const gitSSH = await materializeGitSSH(); try { @@ -1708,7 +1735,8 @@ export async function nonInteractiveSpawn( onLine, npmUserconfigPath, gitSSH?.command, - gitCredentialEnv + gitCredentialEnv, + ignoreNpmScripts ); } finally { await gitSSH?.cleanup(); @@ -1724,7 +1752,8 @@ function spawnWithEnv( onLine: ((stream: 'stdout' | 'stderr', line: string) => void) | undefined, npmUserconfigPath: string | undefined, gitSSHCommand: string | undefined, - gitCredentialEnv: Record | undefined + gitCredentialEnv: Record | undefined, + ignoreNpmScripts: boolean ): Promise<{ stdout: string; stderr: string; code: number }> { return new Promise((resolve, reject) => { logger @@ -1760,6 +1789,12 @@ function spawnWithEnv( } env.npm_config_userconfig = npmUserconfigPath; } + if (ignoreNpmScripts) { + for (const key of Object.keys(env)) { + if (key.toLowerCase() === 'npm_config_ignore_scripts') delete env[key]; + } + env.npm_config_ignore_scripts = 'true'; + } if (process.platform === 'win32' && command === 'npm') { command = 'npm.cmd'; diff --git a/unitTests/components/applicationInstall.test.js b/unitTests/components/applicationInstall.test.js new file mode 100644 index 0000000000..06e509da57 --- /dev/null +++ b/unitTests/components/applicationInstall.test.js @@ -0,0 +1,266 @@ +'use strict'; + +const assert = require('node:assert'); +const { access, mkdir, mkdtemp, readFile, rm, writeFile } = require('node:fs/promises'); +const { tmpdir } = require('node:os'); +const { join } = require('node:path'); + +const testUtils = require('../testUtils.js'); +testUtils.preTestPrep(); + +const { + Application, + installApplication, + packageHasAutomaticInstallWork, + packageHasProductionInstallWork, +} = require('#src/components/Application'); + +async function createApplication(root, name, packageJSON, install) { + const directory = join(root, name); + await mkdir(directory, { recursive: true }); + await writeFile(join(directory, 'package.json'), JSON.stringify({ name, version: '1.0.0', ...packageJSON })); + const application = new Application({ name, install }); + application.dirPath = directory; + return application; +} + +async function createLifecycleDependency(root, name, markerPath) { + const dependencyName = `${name}-dependency`; + const directory = join(root, dependencyName); + await mkdir(directory); + await writeFile( + join(directory, 'install.cjs'), + `require('node:fs').writeFileSync(${JSON.stringify(markerPath)}, 'ran');\n` + ); + await writeFile( + join(directory, 'package.json'), + JSON.stringify({ name: dependencyName, version: '1.0.0', scripts: { install: 'node install.cjs' } }) + ); + return { dependencyName, directory }; +} + +async function configureInstallCapture(application, root, name) { + const capturePath = join(root, `${name}-args.json`); + const captureScript = join(root, `${name}-capture.cjs`); + await writeFile( + captureScript, + `require('node:fs').writeFileSync(${JSON.stringify(capturePath)}, JSON.stringify(process.argv.slice(2)));\n` + ); + application.packageManagerPrefix = `"${process.execPath}" "${captureScript}"`; + return capturePath; +} + +describe('automatic application installation', () => { + beforeEach(async function () { + this.root = await mkdtemp(join(tmpdir(), 'application-install-')); + }); + + afterEach(async function () { + await rm(this.root, { recursive: true, force: true }); + }); + + it('identifies production dependency and workspace work conservatively', () => { + for (const manifest of [ + { dependencies: { runtime: '1' } }, + { optionalDependencies: { optional: '1' } }, + { peerDependencies: { peer: '1' } }, + { workspaces: ['packages/*'] }, + { workspaces: { packages: ['packages/*'] } }, + { dependencies: 'invalid' }, + { workspaces: 'invalid' }, + ]) { + assert.equal(packageHasProductionInstallWork(manifest), true, JSON.stringify(manifest)); + } + for (const manifest of [ + {}, + { devDependencies: { build: '1' } }, + { workspaces: [] }, + { workspaces: { packages: [] } }, + ]) { + assert.equal(packageHasProductionInstallWork(manifest), false, JSON.stringify(manifest)); + } + assert.equal( + packageHasAutomaticInstallWork({ devEngines: { packageManager: { name: 'pnpm' } } }), + true, + 'an explicit non-npm manager may discover production work outside the root manifest' + ); + }); + + it('skips automatic installation when only development dependencies are declared', async function () { + const application = await createApplication(this.root, 'development-only', { + devDependencies: { build: '1.0.0' }, + }); + const capturePath = await configureInstallCapture(application, this.root, 'development-only'); + + await installApplication(application); + + await assert.rejects(access(capturePath), (error) => error.code === 'ENOENT'); + assert.equal(application.installationIsOpaque, false); + }); + + it('uses production-only npm flags for the default and declared npm paths', async function () { + const defaultApplication = await createApplication(this.root, 'default-npm', { + dependencies: { runtime: '1.0.0' }, + }); + const defaultCapture = await configureInstallCapture(defaultApplication, this.root, 'default-npm'); + await installApplication(defaultApplication); + assert.deepEqual(JSON.parse(await readFile(defaultCapture, 'utf8')), [ + 'npm', + 'install', + '--force', + '--omit=dev', + '--no-audit', + '--no-fund', + '--ignore-scripts', + ]); + + const declaredApplication = await createApplication(this.root, 'declared-npm', { + dependencies: { runtime: '1.0.0' }, + devEngines: { packageManager: { name: 'npm' } }, + }); + const declaredCapture = await configureInstallCapture(declaredApplication, this.root, 'declared-npm'); + await installApplication(declaredApplication); + assert.deepEqual(JSON.parse(await readFile(declaredCapture, 'utf8')), [ + 'npm', + 'install', + '--omit=dev', + '--no-audit', + '--no-fund', + '--ignore-scripts', + ]); + }); + + it('preserves an explicit non-npm workspace install when the root manifest has no production work', async function () { + const application = await createApplication(this.root, 'declared-pnpm', { + devEngines: { packageManager: { name: 'pnpm' } }, + }); + await writeFile(join(application.dirPath, 'pnpm-workspace.yaml'), "packages:\n - 'packages/*'\n"); + const capturePath = await configureInstallCapture(application, this.root, 'declared-pnpm'); + + await installApplication(application); + + assert.deepEqual(JSON.parse(await readFile(capturePath, 'utf8')), ['pnpm', 'install', '--ignore-scripts']); + }); + + it('runs an allowed install lifecycle while still omitting development dependencies', async function () { + const application = await createApplication( + this.root, + 'allowed-lifecycle', + { + devDependencies: { build: '1.0.0' }, + scripts: { prepare: 'node build.js' }, + }, + { allowInstallScripts: true } + ); + const capturePath = await configureInstallCapture(application, this.root, 'allowed-lifecycle'); + + await installApplication(application); + + assert.deepEqual(JSON.parse(await readFile(capturePath, 'utf8')), [ + 'npm', + 'install', + '--force', + '--omit=dev', + '--no-audit', + '--no-fund', + ]); + assert.equal(application.installationIsOpaque, true); + }); + + it('keeps install_command as the development-dependency escape hatch', async function () { + const application = await createApplication( + this.root, + 'custom-command', + { devDependencies: { build: '1.0.0' } }, + { command: 'node custom-install.cjs' } + ); + const automaticCapture = await configureInstallCapture(application, this.root, 'custom-command'); + const customMarker = join(application.dirPath, 'custom-command-ran'); + await writeFile( + join(application.dirPath, 'custom-install.cjs'), + `require('node:fs').writeFileSync(${JSON.stringify(customMarker)}, JSON.stringify(process.argv.slice(2)));\n` + ); + + await installApplication(application); + + assert.deepEqual(JSON.parse(await readFile(customMarker, 'utf8')), []); + await assert.rejects(access(automaticCapture), (error) => error.code === 'ENOENT'); + assert.equal(application.installationIsOpaque, true); + }); + + it('applies the lifecycle-script policy to custom install commands', async function () { + this.timeout(60_000); + const inheritedPolicies = Object.entries(process.env).filter( + ([key]) => key.toLowerCase() === 'npm_config_ignore_scripts' + ); + for (const [key] of inheritedPolicies) delete process.env[key]; + process.env.npm_config_ignore_scripts = 'false'; + try { + for (const allowInstallScripts of [undefined, false, true]) { + const name = + allowInstallScripts === undefined + ? 'custom-scripts-default' + : allowInstallScripts + ? 'custom-scripts-allowed' + : 'custom-scripts-blocked'; + const markerPath = join(this.root, `${name}-marker`); + const dependency = await createLifecycleDependency(this.root, name, markerPath); + const install = { command: 'npm install --no-audit --no-fund && node -e "process.exit(0)"' }; + if (allowInstallScripts !== undefined) install.allowInstallScripts = allowInstallScripts; + const application = await createApplication( + this.root, + name, + { dependencies: { [dependency.dependencyName]: `file:${dependency.directory}` } }, + install + ); + const warnings = []; + application.logger.warn = (message) => warnings.push(message); + + await installApplication(application); + + await access(join(application.dirPath, 'node_modules', dependency.dependencyName, 'package.json')); + assert.equal( + await access(markerPath).then( + () => true, + () => false + ), + allowInstallScripts === true + ); + assert.equal(warnings.length, allowInstallScripts === undefined ? 1 : 0); + if (warnings.length) assert.match(warnings[0], /install\.allowInstallScripts/); + } + } finally { + for (const key of Object.keys(process.env)) { + if (key.toLowerCase() === 'npm_config_ignore_scripts') delete process.env[key]; + } + Object.assign(process.env, Object.fromEntries(inheritedPolicies)); + } + }); + + it('materializes runtime dependencies while omitting development dependencies', async function () { + this.timeout(60_000); + const runtimeDirectory = join(this.root, 'runtime-package'); + const developmentDirectory = join(this.root, 'development-package'); + await Promise.all([mkdir(runtimeDirectory), mkdir(developmentDirectory)]); + await Promise.all([ + writeFile(join(runtimeDirectory, 'package.json'), JSON.stringify({ name: 'runtime-package', version: '1.0.0' })), + writeFile( + join(developmentDirectory, 'package.json'), + JSON.stringify({ name: 'development-package', version: '1.0.0' }) + ), + ]); + const application = await createApplication(this.root, 'real-npm', { + dependencies: { 'runtime-package': `file:${runtimeDirectory}` }, + devDependencies: { 'development-package': `file:${developmentDirectory}` }, + }); + application.packageManagerPrefix = ''; + + await installApplication(application); + + await access(join(application.dirPath, 'node_modules', 'runtime-package', 'package.json')); + await assert.rejects( + access(join(application.dirPath, 'node_modules', 'development-package')), + (error) => error.code === 'ENOENT' + ); + }); +}); diff --git a/unitTests/utility/npmUtilities.test.js b/unitTests/utility/npmUtilities.test.js new file mode 100644 index 0000000000..fc1dcf3475 --- /dev/null +++ b/unitTests/utility/npmUtilities.test.js @@ -0,0 +1,188 @@ +'use strict'; + +const assert = require('node:assert'); +const fs = require('node:fs'); +const os = require('node:os'); +const path = require('node:path'); + +const testUtils = require('../testUtils.js'); +testUtils.preTestPrep(); + +const env = require('#src/utility/environment/environmentManager'); +const { CONFIG_PARAMS } = require('#src/utility/hdbTerms'); +const { installModules } = require('#src/utility/npmUtilities'); + +describe('install_node_modules', function () { + this.timeout(60_000); // .mocharc.json sets `timeout: 0`, and these cases spawn real npm + + let componentsRoot; + let lifecycleMarker; + + before(() => { + componentsRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'harper-install-modules-')); + lifecycleMarker = path.join(componentsRoot, 'lifecycle-marker'); + env.setProperty(CONFIG_PARAMS.COMPONENTSROOT, componentsRoot); + + fs.mkdirSync(path.join(componentsRoot, 'dependency')); + fs.writeFileSync( + path.join(componentsRoot, 'dependency', 'install.cjs'), + `require('node:fs').writeFileSync(${JSON.stringify(lifecycleMarker)}, 'ran');\n` + ); + fs.writeFileSync( + path.join(componentsRoot, 'dependency', 'package.json'), + JSON.stringify({ + name: 'local-dependency', + version: '1.0.0', + scripts: { install: 'node install.cjs' }, + }) + ); + fs.mkdirSync(path.join(componentsRoot, 'application')); + fs.writeFileSync( + path.join(componentsRoot, 'application', 'package.json'), + JSON.stringify({ + name: 'application', + version: '1.0.0', + dependencies: { 'local-dependency': 'file:../dependency' }, + }) + ); + }); + + after(() => { + fs.rmSync(componentsRoot, { recursive: true, force: true }); + }); + + afterEach(() => { + fs.rmSync(path.join(componentsRoot, 'application', 'node_modules'), { recursive: true, force: true }); + fs.rmSync(lifecycleMarker, { force: true }); + }); + + // npm still reports the dependency it would add under --dry-run, so asserting on that output + // distinguishes a real dry run from npm never running at all + function assertDryRun(response) { + assert.match(JSON.stringify(response.application.npm_output), /add local-dependency/); + assert.equal(installedDependencyExists(), false); + } + + function assertInstalled(response) { + assert.equal(response.application.npm_output.added, 1, JSON.stringify(response.application)); + assert.equal(installedDependencyExists(), true); + } + + function installedDependencyExists() { + return fs.existsSync(path.join(componentsRoot, 'application', 'node_modules', 'local-dependency')); + } + + async function withNpmLifecycleScriptsEnabled(callback) { + const inheritedPolicies = Object.entries(process.env).filter( + ([key]) => key.toLowerCase() === 'npm_config_ignore_scripts' + ); + for (const [key] of inheritedPolicies) delete process.env[key]; + process.env.npm_config_ignore_scripts = 'false'; + try { + return await callback(); + } finally { + for (const key of Object.keys(process.env)) { + if (key.toLowerCase() === 'npm_config_ignore_scripts') delete process.env[key]; + } + Object.assign(process.env, Object.fromEntries(inheritedPolicies)); + } + } + + it('honors the documented dry_run field', async () => { + const response = await installModules({ projects: ['application'], dry_run: true }); + + assertDryRun(response); + }); + + it('honors a dry_run field that arrives as a string', async () => { + const response = await installModules({ projects: ['application'], dry_run: 'true' }); + + assertDryRun(response); + }); + + it('honors the undocumented camelCase dryRun spelling', async () => { + const response = await installModules({ projects: ['application'], dryRun: true }); + + assertDryRun(response); + }); + + it('installs when dry_run is false', async () => { + const response = await installModules({ projects: ['application'], dry_run: 'false' }); + + assertInstalled(response); + }); + + it('installs when dry_run is omitted', async () => { + const response = await withNpmLifecycleScriptsEnabled(() => installModules({ projects: ['application'] })); + + assertInstalled(response); + assert.equal(fs.existsSync(lifecycleMarker), true); + }); + + it('applies the lifecycle-script policy while preserving the allowed positive control', async () => { + await withNpmLifecycleScriptsEnabled(async () => { + const blockedResponse = await installModules({ projects: ['application'], allowInstallScripts: false }); + + assertInstalled(blockedResponse); + assert.equal(fs.existsSync(lifecycleMarker), false); + fs.rmSync(path.join(componentsRoot, 'application', 'node_modules'), { recursive: true, force: true }); + + const allowedResponse = await installModules({ projects: ['application'], allowInstallScripts: true }); + + assertInstalled(allowedResponse); + assert.equal(fs.existsSync(lifecycleMarker), true); + }); + }); + + it('honors install_allow_scripts and rejects both policy spellings together', async () => { + const response = await withNpmLifecycleScriptsEnabled(() => + installModules({ projects: ['application'], install_allow_scripts: false }) + ); + + assertInstalled(response); + assert.equal(fs.existsSync(lifecycleMarker), false); + await assert.rejects( + installModules({ projects: ['application'], install_allow_scripts: false, allowInstallScripts: true }), + { statusCode: 400, message: /install_allow_scripts/ } + ); + }); + + it('rejects a request carrying both dry_run spellings', async () => { + await assert.rejects(installModules({ projects: ['application'], dry_run: true, dryRun: false }), { + statusCode: 400, + message: /dryRun/, + }); + assert.equal(installedDependencyExists(), false); + }); + + it('rejects a request without projects', async () => { + await assert.rejects(installModules({ dry_run: true }), { statusCode: 400, message: /'projects'/ }); + }); + + it('runs npm with the registry audit disabled', async function () { + if (process.platform === 'win32') return this.skip(); // the shim below is a POSIX shell script + const shimDir = fs.mkdtempSync(path.join(os.tmpdir(), 'harper-npm-shim-')); + const originalPath = process.env.PATH; + let argv; + try { + const argvPath = path.join(shimDir, 'argv.txt'); + // the destination travels as an environment value, not as text in the script: a `$` or a + // backtick in TMPDIR would otherwise be expanded by the shell that runs this + fs.writeFileSync( + path.join(shimDir, 'npm'), + '#!/bin/sh\nprintf \'%s\\n\' "$@" > "$HARPER_TEST_NPM_ARGV_PATH"\necho \'{"added":0}\'\n', + { mode: 0o755 } + ); + process.env.PATH = `${shimDir}${path.delimiter}${originalPath}`; + process.env.HARPER_TEST_NPM_ARGV_PATH = argvPath; + await installModules({ projects: ['application'] }); + argv = fs.readFileSync(argvPath, 'utf8').split('\n').filter(Boolean); + } finally { + process.env.PATH = originalPath; + delete process.env.HARPER_TEST_NPM_ARGV_PATH; + fs.rmSync(shimDir, { recursive: true, force: true }); + } + + assert.deepStrictEqual(argv, ['install', '--force', '--omit=dev', '--no-audit', '--no-fund', '--json']); + }); +}); diff --git a/utility/npmUtilities.ts b/utility/npmUtilities.ts index 7e29427aaa..4bbc39209f 100644 --- a/utility/npmUtilities.ts +++ b/utility/npmUtilities.ts @@ -31,13 +31,21 @@ export async function installModules(req: any) { throw handleHDBError(validation, validation.message, HTTP_STATUS_CODES.BAD_REQUEST); } +<<<<<<< HEAD let { projects, dryRun } = req; +======= + const { projects, dry_run: dryRun, allowInstallScripts } = validatedRequest; +>>>>>>> 7f0d5a08c (Honor install script policy in every install path) const componentsRootDirPath = getConfigPath(CONFIG_PARAMS.COMPONENTSROOT); const responseObject: any = {}; +<<<<<<< HEAD const args = ['install', '--force', '--omit=dev', '--json']; +======= + const args = [...packageManagerInstallArguments('npm', allowInstallScripts, true), '--json']; +>>>>>>> 7f0d5a08c (Honor install script policy in every install path) if (dryRun) args.push('--dry-run'); for (const project of projects) { @@ -99,7 +107,14 @@ function modulesValidator(req: any) { const funcSchema = Joi.object({ projects: Joi.array().min(1).items(Joi.string()).required(), dry_run: Joi.boolean().default(false), +<<<<<<< HEAD }); +======= + allowInstallScripts: Joi.boolean().default(true), + }) + .rename('dryRun', 'dry_run', { ignoreUndefined: true }) + .rename('install_allow_scripts', 'allowInstallScripts', { ignoreUndefined: true }); +>>>>>>> 7f0d5a08c (Honor install script policy in every install path) return validator.validateBySchema(req, funcSchema); } From d4ae5b99ced985419686a0a4e8812f53fdf82e49 Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Thu, 1 Oct 2026 18:02:00 -0600 Subject: [PATCH 2/3] Resolve the v5.2 install-script policy cherry-pick conflicts Keep v5.2's automatic install paths and legacy npm arguments while applying #2572's custom-command and deprecated-operation lifecycle script policy. Consume converted policy values locally, adapt the copied tests to release semantics, and keep the design record in the release branch's root document. Co-Authored-By: GPT-5 Codex Dispatch-Task: cherry-resolve-kriszyp_harper_2572-910d0d15 --- DESIGN.md | 42 +++----- components/Application.ts | 17 --- .../components/applicationInstall.test.js | 100 ++---------------- unitTests/utility/npmUtilities.test.js | 72 ++++--------- utility/npmUtilities.ts | 24 +---- 5 files changed, 47 insertions(+), 208 deletions(-) diff --git a/DESIGN.md b/DESIGN.md index 0654ba27d1..088cbcd567 100644 --- a/DESIGN.md +++ b/DESIGN.md @@ -296,34 +296,20 @@ A package-manager timeout must not release this lock while npm descendants are s Boot's `harper-application-lock.json` records an application configuration only after preparation fulfills. Recording at queue time would make a failed install look complete and suppress its retry on the next boot. -<<<<<<< HEAD -======= -Every npm install Harper invokes directly — automatic component installation and the deprecated -`install_node_modules` operation alike — composes its arguments in `packageManagerInstallArguments()`, -which is production-only and adds `--omit=dev --no-audit --no-fund`. `--no-audit` is load-bearing, not -hygiene: npm 10 puts even a `file:` link into its audit bulk request, and the registry's answer to that -is unbounded from Harper's side. The operation accepts the established `install_allow_scripts` -spelling (and `allowInstallScripts` for compatibility), defaulting to its historical `true`; false -reaches the shared builder and adds `--ignore-scripts`. -`installApplication()` skips the package-manager child entirely when the root manifest declares no -production dependencies, non-empty workspaces, or enabled install lifecycle. An explicitly selected -non-npm manager still runs so it can discover workspace configuration outside `package.json`, and it -retains its own install defaults. A configured `install_command` remains the explicit escape hatch for -build-time tooling, but not for lifecycle-script policy: unless `install_allow_scripts` is true, its -spawn gets `npm_config_ignore_scripts=true`, which covers npm nested anywhere in the command without -adding an argument that could break non-npm tooling. The setting uses npm's configuration namespace; -other package managers that consume `npm_config_*` options can honor it too. When the policy is -omitted, Harper warns that package lifecycle scripts—including `npm run` pre/post hooks—are suppressed -and names both the operations-API and root-config opt-ins. -`readInstalledPackageMetadata()` must use the same automatic-work predicate so a -dev-only npm manifest does not force a restart on every redeploy for lacking a lockfile while an -explicit non-npm workspace install still does. Absolute local archives are classified before -package-protocol detection: a Windows drive letter's colon is path syntax, not an npm protocol. File -type detection remains asynchronous in extraction. Bare absolute Windows directory inputs retain -npm's copy/pack behavior rather than becoming live links; explicit `file:` and relative directory -inputs retain their existing symlink behavior. - ->>>>>>> 7f0d5a08c (Honor install script policy in every install path) +Custom `install_command` spawns receive `npm_config_ignore_scripts=true` unless +`install_allow_scripts` (or `install.allowInstallScripts` in root config) is true. The child-only +environment setting reaches npm nested in shell commands without changing the command's arguments; +other package managers must honor npm's configuration namespace for it to apply. An explicit opt-in +does not clear an inherited host restriction. Omitted policy emits a warning naming the opt-ins, +because existing custom commands can rely on lifecycle scripts, including `npm run` pre/post hooks. +This is best-effort enforcement for arbitrary commands: an explicit override in the command or a +package manager that ignores npm configuration can bypass it. + +The deprecated `install_node_modules` operation retains its scripts-enabled default and existing npm +arguments. `installModules()` consumes Joi's converted `allowInstallScripts` value (also accepted as +`install_allow_scripts`) and adds `--ignore-scripts` when false. Both spellings together are rejected. +Its legacy `dryRun` behavior and the automatic component-install paths remain unchanged. + ## Peer-side deploy_component payload read: retryable blob stalls and `Readable.from()` cancellation `readPayloadBlobWithRetry` (`components/deploymentRecorder.ts`) wraps the peer's read of a replicated `hdb_deployment` row's `payload_blob` so a transient 503 `BlobReadError` (`BLOB_UNAVAILABLE_STATUS`, `resources/blob.ts`) — content bytes not arriving within `blobReadTimeout`, e.g. a parked blob send on the origin — retries instead of failing the whole deploy. Two non-obvious constraints shaped the design: diff --git a/components/Application.ts b/components/Application.ts index 766948d645..1285daa35a 100644 --- a/components/Application.ts +++ b/components/Application.ts @@ -930,23 +930,6 @@ export async function installApplication(application: Application) { ); } -<<<<<<< HEAD -======= - const { packageManager } = packageJSON.devEngines || {}; - if (dependencyFieldHasWork(packageJSON, 'devDependencies')) { - application.logger.warn( - `Application ${application.name} declares devDependencies; automatic npm installation omits them, while explicitly selected non-npm package managers retain their own install defaults. Use install_command when deployment requires custom behavior` - ); - } - if ( - !packageHasAutomaticInstallWork(packageJSON) && - !(allowInstallScripts && packageHasAllowedInstallLifecycleWork(packageJSON)) - ) { - application.logger.info(`Application ${application.name} has no production package work; skipping install`); - return; - } - ->>>>>>> 7f0d5a08c (Honor install script policy in every install path) // Next, try package.json devEngines field const { packageManager } = packageJSON.devEngines || {}; diff --git a/unitTests/components/applicationInstall.test.js b/unitTests/components/applicationInstall.test.js index 06e509da57..6f0a06397a 100644 --- a/unitTests/components/applicationInstall.test.js +++ b/unitTests/components/applicationInstall.test.js @@ -8,12 +8,7 @@ const { join } = require('node:path'); const testUtils = require('../testUtils.js'); testUtils.preTestPrep(); -const { - Application, - installApplication, - packageHasAutomaticInstallWork, - packageHasProductionInstallWork, -} = require('#src/components/Application'); +const { Application, installApplication } = require('#src/components/Application'); async function createApplication(root, name, packageJSON, install) { const directory = join(root, name); @@ -59,46 +54,7 @@ describe('automatic application installation', () => { await rm(this.root, { recursive: true, force: true }); }); - it('identifies production dependency and workspace work conservatively', () => { - for (const manifest of [ - { dependencies: { runtime: '1' } }, - { optionalDependencies: { optional: '1' } }, - { peerDependencies: { peer: '1' } }, - { workspaces: ['packages/*'] }, - { workspaces: { packages: ['packages/*'] } }, - { dependencies: 'invalid' }, - { workspaces: 'invalid' }, - ]) { - assert.equal(packageHasProductionInstallWork(manifest), true, JSON.stringify(manifest)); - } - for (const manifest of [ - {}, - { devDependencies: { build: '1' } }, - { workspaces: [] }, - { workspaces: { packages: [] } }, - ]) { - assert.equal(packageHasProductionInstallWork(manifest), false, JSON.stringify(manifest)); - } - assert.equal( - packageHasAutomaticInstallWork({ devEngines: { packageManager: { name: 'pnpm' } } }), - true, - 'an explicit non-npm manager may discover production work outside the root manifest' - ); - }); - - it('skips automatic installation when only development dependencies are declared', async function () { - const application = await createApplication(this.root, 'development-only', { - devDependencies: { build: '1.0.0' }, - }); - const capturePath = await configureInstallCapture(application, this.root, 'development-only'); - - await installApplication(application); - - await assert.rejects(access(capturePath), (error) => error.code === 'ENOENT'); - assert.equal(application.installationIsOpaque, false); - }); - - it('uses production-only npm flags for the default and declared npm paths', async function () { + it('suppresses lifecycle scripts for the default and declared npm paths', async function () { const defaultApplication = await createApplication(this.root, 'default-npm', { dependencies: { runtime: '1.0.0' }, }); @@ -108,9 +64,6 @@ describe('automatic application installation', () => { 'npm', 'install', '--force', - '--omit=dev', - '--no-audit', - '--no-fund', '--ignore-scripts', ]); @@ -120,14 +73,7 @@ describe('automatic application installation', () => { }); const declaredCapture = await configureInstallCapture(declaredApplication, this.root, 'declared-npm'); await installApplication(declaredApplication); - assert.deepEqual(JSON.parse(await readFile(declaredCapture, 'utf8')), [ - 'npm', - 'install', - '--omit=dev', - '--no-audit', - '--no-fund', - '--ignore-scripts', - ]); + assert.deepEqual(JSON.parse(await readFile(declaredCapture, 'utf8')), ['npm', 'install', '--ignore-scripts']); }); it('preserves an explicit non-npm workspace install when the root manifest has no production work', async function () { @@ -142,7 +88,7 @@ describe('automatic application installation', () => { assert.deepEqual(JSON.parse(await readFile(capturePath, 'utf8')), ['pnpm', 'install', '--ignore-scripts']); }); - it('runs an allowed install lifecycle while still omitting development dependencies', async function () { + it('allows an opted-in install lifecycle on the default npm path', async function () { const application = await createApplication( this.root, 'allowed-lifecycle', @@ -156,18 +102,11 @@ describe('automatic application installation', () => { await installApplication(application); - assert.deepEqual(JSON.parse(await readFile(capturePath, 'utf8')), [ - 'npm', - 'install', - '--force', - '--omit=dev', - '--no-audit', - '--no-fund', - ]); + assert.deepEqual(JSON.parse(await readFile(capturePath, 'utf8')), ['npm', 'install', '--force']); assert.equal(application.installationIsOpaque, true); }); - it('keeps install_command as the development-dependency escape hatch', async function () { + it('preserves the custom install command arguments', async function () { const application = await createApplication( this.root, 'custom-command', @@ -236,31 +175,4 @@ describe('automatic application installation', () => { Object.assign(process.env, Object.fromEntries(inheritedPolicies)); } }); - - it('materializes runtime dependencies while omitting development dependencies', async function () { - this.timeout(60_000); - const runtimeDirectory = join(this.root, 'runtime-package'); - const developmentDirectory = join(this.root, 'development-package'); - await Promise.all([mkdir(runtimeDirectory), mkdir(developmentDirectory)]); - await Promise.all([ - writeFile(join(runtimeDirectory, 'package.json'), JSON.stringify({ name: 'runtime-package', version: '1.0.0' })), - writeFile( - join(developmentDirectory, 'package.json'), - JSON.stringify({ name: 'development-package', version: '1.0.0' }) - ), - ]); - const application = await createApplication(this.root, 'real-npm', { - dependencies: { 'runtime-package': `file:${runtimeDirectory}` }, - devDependencies: { 'development-package': `file:${developmentDirectory}` }, - }); - application.packageManagerPrefix = ''; - - await installApplication(application); - - await access(join(application.dirPath, 'node_modules', 'runtime-package', 'package.json')); - await assert.rejects( - access(join(application.dirPath, 'node_modules', 'development-package')), - (error) => error.code === 'ENOENT' - ); - }); }); diff --git a/unitTests/utility/npmUtilities.test.js b/unitTests/utility/npmUtilities.test.js index fc1dcf3475..f69ebdaf14 100644 --- a/unitTests/utility/npmUtilities.test.js +++ b/unitTests/utility/npmUtilities.test.js @@ -17,10 +17,15 @@ describe('install_node_modules', function () { let componentsRoot; let lifecycleMarker; + let inheritedAuditPolicy; + let inheritedComponentsRoot; before(() => { + inheritedAuditPolicy = process.env.npm_config_audit; + process.env.npm_config_audit = 'false'; componentsRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'harper-install-modules-')); lifecycleMarker = path.join(componentsRoot, 'lifecycle-marker'); + inheritedComponentsRoot = env.get(CONFIG_PARAMS.COMPONENTSROOT); env.setProperty(CONFIG_PARAMS.COMPONENTSROOT, componentsRoot); fs.mkdirSync(path.join(componentsRoot, 'dependency')); @@ -48,6 +53,9 @@ describe('install_node_modules', function () { }); after(() => { + if (inheritedAuditPolicy === undefined) delete process.env.npm_config_audit; + else process.env.npm_config_audit = inheritedAuditPolicy; + env.setProperty(CONFIG_PARAMS.COMPONENTSROOT, inheritedComponentsRoot); fs.rmSync(componentsRoot, { recursive: true, force: true }); }); @@ -88,31 +96,13 @@ describe('install_node_modules', function () { } } - it('honors the documented dry_run field', async () => { - const response = await installModules({ projects: ['application'], dry_run: true }); - - assertDryRun(response); - }); - - it('honors a dry_run field that arrives as a string', async () => { - const response = await installModules({ projects: ['application'], dry_run: 'true' }); - - assertDryRun(response); - }); - - it('honors the undocumented camelCase dryRun spelling', async () => { + it('honors the legacy camelCase dryRun field', async () => { const response = await installModules({ projects: ['application'], dryRun: true }); assertDryRun(response); }); - it('installs when dry_run is false', async () => { - const response = await installModules({ projects: ['application'], dry_run: 'false' }); - - assertInstalled(response); - }); - - it('installs when dry_run is omitted', async () => { + it('allows lifecycle scripts when the policy is omitted', async () => { const response = await withNpmLifecycleScriptsEnabled(() => installModules({ projects: ['application'] })); assertInstalled(response); @@ -147,10 +137,19 @@ describe('install_node_modules', function () { ); }); - it('rejects a request carrying both dry_run spellings', async () => { - await assert.rejects(installModules({ projects: ['application'], dry_run: true, dryRun: false }), { + it('converts a string false policy while accepting operation metadata', async () => { + const response = await withNpmLifecycleScriptsEnabled(() => + installModules({ operation: 'install_node_modules', projects: ['application'], install_allow_scripts: 'false' }) + ); + + assertInstalled(response); + assert.equal(fs.existsSync(lifecycleMarker), false); + }); + + it('rejects an invalid lifecycle-script policy before installation', async () => { + await assert.rejects(installModules({ projects: ['application'], install_allow_scripts: 'invalid' }), { statusCode: 400, - message: /dryRun/, + message: /allowInstallScripts/, }); assert.equal(installedDependencyExists(), false); }); @@ -158,31 +157,4 @@ describe('install_node_modules', function () { it('rejects a request without projects', async () => { await assert.rejects(installModules({ dry_run: true }), { statusCode: 400, message: /'projects'/ }); }); - - it('runs npm with the registry audit disabled', async function () { - if (process.platform === 'win32') return this.skip(); // the shim below is a POSIX shell script - const shimDir = fs.mkdtempSync(path.join(os.tmpdir(), 'harper-npm-shim-')); - const originalPath = process.env.PATH; - let argv; - try { - const argvPath = path.join(shimDir, 'argv.txt'); - // the destination travels as an environment value, not as text in the script: a `$` or a - // backtick in TMPDIR would otherwise be expanded by the shell that runs this - fs.writeFileSync( - path.join(shimDir, 'npm'), - '#!/bin/sh\nprintf \'%s\\n\' "$@" > "$HARPER_TEST_NPM_ARGV_PATH"\necho \'{"added":0}\'\n', - { mode: 0o755 } - ); - process.env.PATH = `${shimDir}${path.delimiter}${originalPath}`; - process.env.HARPER_TEST_NPM_ARGV_PATH = argvPath; - await installModules({ projects: ['application'] }); - argv = fs.readFileSync(argvPath, 'utf8').split('\n').filter(Boolean); - } finally { - process.env.PATH = originalPath; - delete process.env.HARPER_TEST_NPM_ARGV_PATH; - fs.rmSync(shimDir, { recursive: true, force: true }); - } - - assert.deepStrictEqual(argv, ['install', '--force', '--omit=dev', '--no-audit', '--no-fund', '--json']); - }); }); diff --git a/utility/npmUtilities.ts b/utility/npmUtilities.ts index 4bbc39209f..93ff39f5b4 100644 --- a/utility/npmUtilities.ts +++ b/utility/npmUtilities.ts @@ -7,7 +7,6 @@ import { handleHDBError, hdbErrors } from './errors/hdbError.ts'; const { HTTP_STATUS_CODES } = hdbErrors; -import * as validator from '../validation/validationWrapper.ts'; import harperLogger from './logging/harper_logger.ts'; import { CONFIG_PARAMS } from './hdbTerms.ts'; @@ -26,26 +25,19 @@ export async function installModules(req: any) { 'install_node_modules is deprecated. Dependencies are automatically installed on' + ' deploy, and install_node_modules can lead to inconsistent behavior'; harperLogger.warn(deprecationWarning, req.projects); - const validation = modulesValidator(req); + const { error: validation, value: validatedRequest } = modulesValidator(req); if (validation) { throw handleHDBError(validation, validation.message, HTTP_STATUS_CODES.BAD_REQUEST); } -<<<<<<< HEAD - let { projects, dryRun } = req; -======= - const { projects, dry_run: dryRun, allowInstallScripts } = validatedRequest; ->>>>>>> 7f0d5a08c (Honor install script policy in every install path) + const { projects, dryRun, allowInstallScripts } = validatedRequest; const componentsRootDirPath = getConfigPath(CONFIG_PARAMS.COMPONENTSROOT); const responseObject: any = {}; -<<<<<<< HEAD const args = ['install', '--force', '--omit=dev', '--json']; -======= - const args = [...packageManagerInstallArguments('npm', allowInstallScripts, true), '--json']; ->>>>>>> 7f0d5a08c (Honor install script policy in every install path) + if (!allowInstallScripts) args.push('--ignore-scripts'); if (dryRun) args.push('--dry-run'); for (const project of projects) { @@ -107,14 +99,8 @@ function modulesValidator(req: any) { const funcSchema = Joi.object({ projects: Joi.array().min(1).items(Joi.string()).required(), dry_run: Joi.boolean().default(false), -<<<<<<< HEAD - }); -======= allowInstallScripts: Joi.boolean().default(true), - }) - .rename('dryRun', 'dry_run', { ignoreUndefined: true }) - .rename('install_allow_scripts', 'allowInstallScripts', { ignoreUndefined: true }); ->>>>>>> 7f0d5a08c (Honor install script policy in every install path) + }).rename('install_allow_scripts', 'allowInstallScripts', { ignoreUndefined: true }); - return validator.validateBySchema(req, funcSchema); + return funcSchema.validate(req, { allowUnknown: true, abortEarly: false, errors: { wrap: { label: "'" } } }); } From 49f266b592a94fddebf78ce11a57f9c1c767ece3 Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Thu, 1 Oct 2026 18:11:19 -0600 Subject: [PATCH 3/3] Describe the release install tests and policy without main-only context Remove unused workspace and production-install fixtures from the copied argument tests, and state the current operation contract in the design notes. Co-Authored-By: GPT-5 Codex Dispatch-Task: cherry-resolve-kriszyp_harper_2572-910d0d15 --- DESIGN.md | 8 ++++---- unitTests/components/applicationInstall.test.js | 17 ++++------------- 2 files changed, 8 insertions(+), 17 deletions(-) diff --git a/DESIGN.md b/DESIGN.md index 088cbcd567..cd21cb0f0f 100644 --- a/DESIGN.md +++ b/DESIGN.md @@ -305,10 +305,10 @@ because existing custom commands can rely on lifecycle scripts, including `npm r This is best-effort enforcement for arbitrary commands: an explicit override in the command or a package manager that ignores npm configuration can bypass it. -The deprecated `install_node_modules` operation retains its scripts-enabled default and existing npm -arguments. `installModules()` consumes Joi's converted `allowInstallScripts` value (also accepted as -`install_allow_scripts`) and adds `--ignore-scripts` when false. Both spellings together are rejected. -Its legacy `dryRun` behavior and the automatic component-install paths remain unchanged. +The deprecated `install_node_modules` operation defaults to scripts enabled for compatibility. +`installModules()` consumes Joi's converted `allowInstallScripts` value (also accepted as +`install_allow_scripts`), so a string `'false'` suppresses lifecycle scripts. Both spellings together +are rejected. Only camelCase `dryRun` controls dry-run invocation. ## Peer-side deploy_component payload read: retryable blob stalls and `Readable.from()` cancellation diff --git a/unitTests/components/applicationInstall.test.js b/unitTests/components/applicationInstall.test.js index 6f0a06397a..802e63d707 100644 --- a/unitTests/components/applicationInstall.test.js +++ b/unitTests/components/applicationInstall.test.js @@ -76,11 +76,10 @@ describe('automatic application installation', () => { assert.deepEqual(JSON.parse(await readFile(declaredCapture, 'utf8')), ['npm', 'install', '--ignore-scripts']); }); - it('preserves an explicit non-npm workspace install when the root manifest has no production work', async function () { + it('suppresses lifecycle scripts for a declared non-npm package manager', async function () { const application = await createApplication(this.root, 'declared-pnpm', { devEngines: { packageManager: { name: 'pnpm' } }, }); - await writeFile(join(application.dirPath, 'pnpm-workspace.yaml'), "packages:\n - 'packages/*'\n"); const capturePath = await configureInstallCapture(application, this.root, 'declared-pnpm'); await installApplication(application); @@ -88,16 +87,8 @@ describe('automatic application installation', () => { assert.deepEqual(JSON.parse(await readFile(capturePath, 'utf8')), ['pnpm', 'install', '--ignore-scripts']); }); - it('allows an opted-in install lifecycle on the default npm path', async function () { - const application = await createApplication( - this.root, - 'allowed-lifecycle', - { - devDependencies: { build: '1.0.0' }, - scripts: { prepare: 'node build.js' }, - }, - { allowInstallScripts: true } - ); + it('omits script suppression when the default npm path opts in', async function () { + const application = await createApplication(this.root, 'allowed-lifecycle', {}, { allowInstallScripts: true }); const capturePath = await configureInstallCapture(application, this.root, 'allowed-lifecycle'); await installApplication(application); @@ -110,7 +101,7 @@ describe('automatic application installation', () => { const application = await createApplication( this.root, 'custom-command', - { devDependencies: { build: '1.0.0' } }, + {}, { command: 'node custom-install.cjs' } ); const automaticCapture = await configureInstallCapture(application, this.root, 'custom-command');