From e1546ed45d905030301a2765914f0be8d05e0286 Mon Sep 17 00:00:00 2001 From: Geronimo Date: Sat, 10 Oct 2026 13:53:01 +0530 Subject: [PATCH] fix: installer slice from PR #3120 review (Haelyra) Honor ECC_DRY_RUN=1 in install and repair arg parsing (previously only --dry-run flag worked, so ecc --dry-run install/repair silently performed real writes). Make writeInstallState atomic via writeFileAtomic (O_EXCL staging, fsync, cleanup on failure, atomic rename preserving previous state). Add writeInstallState failure/concurrency/symlink regression tests and disposable-home CLI dry-run regression tests for both global and subcommand forms. --- scripts/lib/install-state.js | 4 +- scripts/lib/install/request.js | 6 ++ scripts/repair.js | 6 ++ tests/lib/install-state.test.js | 125 ++++++++++++++++++++++++++ tests/scripts/install-dry-run.test.js | 120 +++++++++++++++++++++++++ 5 files changed, 259 insertions(+), 2 deletions(-) create mode 100644 tests/scripts/install-dry-run.test.js diff --git a/scripts/lib/install-state.js b/scripts/lib/install-state.js index 3b5b7fc23..ffab9b118 100644 --- a/scripts/lib/install-state.js +++ b/scripts/lib/install-state.js @@ -341,8 +341,8 @@ function readInstallState(filePath) { function writeInstallState(filePath, state) { assertValidInstallState(state, filePath); - fs.mkdirSync(path.dirname(filePath), { recursive: true }); - fs.writeFileSync(filePath, `${JSON.stringify(state, null, 2)}\n`); + const { writeFileAtomic } = require('./atomic-write'); + writeFileAtomic(filePath, `${JSON.stringify(state, null, 2)}\n`); return state; } diff --git a/scripts/lib/install/request.js b/scripts/lib/install/request.js index f99f5aba1..28fab4acb 100644 --- a/scripts/lib/install/request.js +++ b/scripts/lib/install/request.js @@ -88,6 +88,12 @@ function parseInstallArgs(argv) { } } + const dryRunEnv = process.env.ECC_DRY_RUN; + if (dryRunEnv !== undefined && dryRunEnv !== '0' && dryRunEnv !== '1') { + throw new Error('ECC_DRY_RUN must be "1" or "0" when set'); + } + parsed.dryRun = parsed.dryRun || dryRunEnv === '1'; + return parsed; } diff --git a/scripts/repair.js b/scripts/repair.js index 3494f1ade..d5818e536 100644 --- a/scripts/repair.js +++ b/scripts/repair.js @@ -40,6 +40,12 @@ function parseArgs(argv) { } } + const dryRunEnv = process.env.ECC_DRY_RUN; + if (dryRunEnv !== undefined && dryRunEnv !== '0' && dryRunEnv !== '1') { + throw new Error('ECC_DRY_RUN must be "1" or "0" when set'); + } + parsed.dryRun = parsed.dryRun || dryRunEnv === '1'; + return parsed; } diff --git a/tests/lib/install-state.test.js b/tests/lib/install-state.test.js index 653a6bb10..848d81be7 100644 --- a/tests/lib/install-state.test.js +++ b/tests/lib/install-state.test.js @@ -328,6 +328,131 @@ function runTests() { } })) passed++; else failed++; + function makeState(targetRoot, statePath, marker) { + return createInstallState({ + adapter: { id: 'claude-home' }, + targetRoot, + installStatePath: statePath, + request: { + profile: 'core', + modules: [], + legacyLanguages: [], + legacyMode: false, + }, + resolution: { + selectedModules: ['rules-core'], + skippedModules: [], + }, + operations: [], + source: { + repoVersion: CURRENT_PACKAGE_VERSION, + repoCommit: 'abc123', + manifestVersion: 1, + }, + lastValidatedAt: marker, + }); + } + + function listStagingFiles(dirPath) { + return fs.readdirSync(dirPath).filter(name => name.includes('.tmp')); + } + + if (test('writeInstallState preserves the previous valid state when rename fails', () => { + const testDir = createTestDir(); + const statePath = path.join(testDir, 'ecc-install-state.json'); + + try { + writeInstallState(statePath, makeState(testDir, statePath, '2026-01-01T00:00:00Z')); + const originalRenameSync = fs.renameSync; + fs.renameSync = () => { throw new Error('injected rename failure'); }; + try { + assert.throws( + () => writeInstallState(statePath, makeState(testDir, statePath, '2026-02-01T00:00:00Z')), + /injected rename failure/ + ); + } finally { + fs.renameSync = originalRenameSync; + } + assert.strictEqual(readInstallState(statePath).lastValidatedAt, '2026-01-01T00:00:00Z'); + assert.deepStrictEqual(listStagingFiles(testDir), []); + } finally { + cleanupTestDir(testDir); + } + })) passed++; else failed++; + + if (test('writeInstallState cleans up staging files when the write fails', () => { + const testDir = createTestDir(); + const statePath = path.join(testDir, 'ecc-install-state.json'); + + try { + const originalWriteFileSync = fs.writeFileSync; + fs.writeFileSync = () => { throw new Error('injected write failure'); }; + try { + assert.throws( + () => writeInstallState(statePath, makeState(testDir, statePath, '2026-01-01T00:00:00Z')), + /injected write failure/ + ); + } finally { + fs.writeFileSync = originalWriteFileSync; + } + assert.ok(!fs.existsSync(statePath)); + assert.deepStrictEqual(listStagingFiles(testDir), []); + } finally { + cleanupTestDir(testDir); + } + })) passed++; else failed++; + + if (test('writeInstallState replaces symlinks instead of following them', () => { + if (process.platform === 'win32') return; + const testDir = createTestDir(); + const statePath = path.join(testDir, 'ecc-install-state.json'); + const targetPath = path.join(testDir, 'link-target.json'); + fs.writeFileSync(targetPath, '{"untouched":true}\n'); + + try { + fs.symlinkSync(targetPath, statePath); + writeInstallState(statePath, makeState(testDir, statePath, '2026-01-01T00:00:00Z')); + assert.ok(fs.lstatSync(statePath).isFile()); + assert.strictEqual(readInstallState(statePath).lastValidatedAt, '2026-01-01T00:00:00Z'); + assert.strictEqual(fs.readFileSync(targetPath, 'utf8'), '{"untouched":true}\n'); + } finally { + cleanupTestDir(testDir); + } + })) passed++; else failed++; + + if (test('concurrent writeInstallState calls always leave a complete valid state', () => { + const testDir = createTestDir(); + const statePath = path.join(testDir, 'ecc-install-state.json'); + const markers = ['2026-03-01T00:00:00Z', '2026-03-02T00:00:00Z', '2026-03-03T00:00:00Z', '2026-03-04T00:00:00Z']; + + try { + writeInstallState(statePath, makeState(testDir, statePath, markers[0])); + const { spawnSync } = require('child_process'); + const script = 'const {readInstallState, writeInstallState} = require(process.env.REPO_ROOT + "/scripts/lib/install-state");' + + 'const state = readInstallState(process.env.STATE_PATH);' + + 'state.lastValidatedAt = process.env.MARKER;' + + 'writeInstallState(process.env.STATE_PATH, state);'; + for (const marker of markers.slice(1)) { + const child = spawnSync(process.execPath, ['-e', script], { + encoding: 'utf8', + timeout: 30000, + env: { + ...process.env, + REPO_ROOT: path.join(__dirname, '..', '..'), + STATE_PATH: statePath, + MARKER: marker, + }, + }); + assert.strictEqual(child.status, 0, child.stderr); + } + const final = readInstallState(statePath); + assert.ok(markers.includes(final.lastValidatedAt)); + assert.deepStrictEqual(listStagingFiles(testDir), []); + } finally { + cleanupTestDir(testDir); + } + })) passed++; else failed++; + console.log(`\nResults: Passed: ${passed}, Failed: ${failed}`); process.exit(failed > 0 ? 1 : 0); } diff --git a/tests/scripts/install-dry-run.test.js b/tests/scripts/install-dry-run.test.js new file mode 100644 index 000000000..25b13d4bc --- /dev/null +++ b/tests/scripts/install-dry-run.test.js @@ -0,0 +1,120 @@ +'use strict'; + +const assert = require('assert'); +const fs = require('fs'); +const os = require('os'); +const path = require('path'); +const { spawnSync } = require('child_process'); + +const repoRoot = path.join(__dirname, '..', '..'); +const eccCli = path.join(repoRoot, 'scripts', 'ecc.js'); + +let passed = 0; +let failed = 0; + +function test(name, fn) { + const root = fs.mkdtempSync(path.join(os.tmpdir(), 'ecc-install-dry-run-')); + try { + const projectRoot = path.join(root, 'project'); + const homeDir = path.join(root, 'home'); + fs.mkdirSync(projectRoot, { recursive: true }); + fs.mkdirSync(homeDir, { recursive: true }); + fn({ projectRoot, homeDir }); + passed++; + console.log(` PASS ${name}`); + } catch (error) { + failed++; + console.error(` FAIL ${name}: ${error.stack}`); + } finally { + fs.rmSync(root, { recursive: true, force: true }); + } +} + +function makeEnv(homeDir) { + const env = { + ...process.env, + HOME: homeDir, + USERPROFILE: homeDir, + CODEX_HOME: path.join(homeDir, '.codex'), + XDG_CONFIG_HOME: path.join(homeDir, '.config'), + }; + delete env.ECC_DRY_RUN; + return env; +} + +function runEcc(args, context) { + return spawnSync(process.execPath, [eccCli, ...args], { + cwd: context.projectRoot, + encoding: 'utf8', + timeout: 120000, + env: makeEnv(context.homeDir), + }); +} + +function walkFiles(dirPath) { + const found = []; + const visit = current => { + for (const entry of fs.readdirSync(current, { withFileTypes: true })) { + const full = path.join(current, entry.name); + if (entry.isDirectory()) visit(full); + else if (entry.isFile()) found.push(full); + } + }; + if (fs.existsSync(dirPath)) visit(dirPath); + return found.sort(); +} + +function findStateFiles(homeDir) { + return walkFiles(homeDir).filter(file => file.endsWith('ecc-install-state.json')); +} + +function testInstallForms() { + for (const args of [ + ['--dry-run', 'install', '--target', 'cursor', '--modules', 'rules-core'], + ['install', '--target', 'cursor', '--modules', 'rules-core', '--dry-run'], + ]) { + test(`dry-run install writes nothing (${args.slice(0, 2).join(' ')})`, context => { + const result = runEcc(args, context); + assert.strictEqual(result.status, 0, result.stderr); + assert.deepStrictEqual(walkFiles(context.homeDir), []); + assert.deepStrictEqual(findStateFiles(context.homeDir), []); + }); + } +} + +function testRepairForms() { + for (const args of [ + ['--dry-run', 'repair', '--target', 'cursor'], + ['repair', '--target', 'cursor', '--dry-run'], + ]) { + test(`dry-run repair preserves drift and state (${args.slice(0, 2).join(' ')})`, context => { + const installed = runEcc(['install', '--target', 'cursor', '--modules', 'rules-core'], context); + assert.strictEqual(installed.status, 0, installed.stderr); + const roots = [context.homeDir, context.projectRoot]; + const stateFiles = roots.flatMap(findStateFiles); + assert.ok(stateFiles.length > 0, 'expected a real install to write state first'); + const managedFile = roots.flatMap(walkFiles).find( + file => !file.endsWith('ecc-install-state.json') && !file.endsWith('.db') + ); + assert.ok(managedFile, 'expected a real install to write managed files first'); + fs.appendFileSync(managedFile, '\n# local drift marker\n'); + const drifted = fs.readFileSync(managedFile, 'utf8'); + const stateBefore = stateFiles.map(file => fs.readFileSync(file)); + + const result = runEcc(args, context); + assert.strictEqual(result.status, 0, result.stderr); + assert.strictEqual(fs.readFileSync(managedFile, 'utf8'), drifted); + assert.deepStrictEqual( + stateFiles.map(file => fs.readFileSync(file)), + stateBefore + ); + }); + } +} + +console.log('\nInstall dry-run CLI regression tests:\n'); +testInstallForms(); +testRepairForms(); + +console.log(`\nResults: Passed: ${passed}, Failed: ${failed}`); +process.exitCode = failed ? 1 : 0;