From 167b105cac7fd4c360ec6149ec2c67c6dbfbe624 Mon Sep 17 00:00:00 2001 From: Affaan Mustafa Date: Fri, 13 Feb 2026 03:37:46 -0800 Subject: [PATCH] fix: reject flags passed as package manager names in setup-package-manager CLI When --global or --project was followed by another flag (e.g., --global --project), the flag was treated as a package manager name. Added pmName.startsWith('-') check to both handlers. Added 20 tests across 4 test files covering argument validation, ensureDir error propagation, runCommand stderr handling, and saveAliases failure paths. --- scripts/setup-package-manager.js | 4 +- tests/lib/package-manager.test.js | 69 +++++++++++++++++++++ tests/lib/session-aliases.test.js | 33 ++++++++++ tests/lib/utils.test.js | 63 +++++++++++++++++++ tests/scripts/setup-package-manager.test.js | 53 ++++++++++++++++ 5 files changed, 220 insertions(+), 2 deletions(-) diff --git a/scripts/setup-package-manager.js b/scripts/setup-package-manager.js index bdc833b..c68ebcc 100644 --- a/scripts/setup-package-manager.js +++ b/scripts/setup-package-manager.js @@ -174,7 +174,7 @@ if (args.includes('--list')) { const globalIdx = args.indexOf('--global'); if (globalIdx !== -1) { const pmName = args[globalIdx + 1]; - if (!pmName) { + if (!pmName || pmName.startsWith('-')) { console.error('Error: --global requires a package manager name'); process.exit(1); } @@ -185,7 +185,7 @@ if (globalIdx !== -1) { const projectIdx = args.indexOf('--project'); if (projectIdx !== -1) { const pmName = args[projectIdx + 1]; - if (!pmName) { + if (!pmName || pmName.startsWith('-')) { console.error('Error: --project requires a package manager name'); process.exit(1); } diff --git a/tests/lib/package-manager.test.js b/tests/lib/package-manager.test.js index 8691d97..765d2e9 100644 --- a/tests/lib/package-manager.test.js +++ b/tests/lib/package-manager.test.js @@ -1024,6 +1024,75 @@ function runTests() { assert.ok(regex.test('pnpm dev'), 'Known action pnpm dev should match'); })) passed++; else failed++; + // ── Round 31: setProjectPackageManager write verification ── + console.log('\nsetProjectPackageManager (write verification, Round 31):'); + + if (test('setProjectPackageManager creates .claude directory if missing', () => { + const testDir = fs.mkdtempSync(path.join(os.tmpdir(), 'pm-mkdir-')); + try { + const claudeDir = path.join(testDir, '.claude'); + assert.ok(!fs.existsSync(claudeDir), '.claude should not pre-exist'); + pm.setProjectPackageManager('npm', testDir); + assert.ok(fs.existsSync(claudeDir), '.claude should be created'); + const configPath = path.join(claudeDir, 'package-manager.json'); + assert.ok(fs.existsSync(configPath), 'Config file should be created'); + } finally { + fs.rmSync(testDir, { recursive: true, force: true }); + } + })) passed++; else failed++; + + if (test('setProjectPackageManager includes setAt timestamp', () => { + const testDir = fs.mkdtempSync(path.join(os.tmpdir(), 'pm-ts-')); + try { + const before = new Date().toISOString(); + const config = pm.setProjectPackageManager('yarn', testDir); + const after = new Date().toISOString(); + assert.ok(config.setAt >= before, 'setAt should be >= before'); + assert.ok(config.setAt <= after, 'setAt should be <= after'); + } finally { + fs.rmSync(testDir, { recursive: true, force: true }); + } + })) passed++; else failed++; + + // ── Round 31: getExecCommand safe argument edge cases ── + console.log('\ngetExecCommand (safe argument edge cases, Round 31):'); + + if (test('allows colons in args (e.g. --fix:all)', () => { + const originalEnv = process.env.CLAUDE_PACKAGE_MANAGER; + try { + process.env.CLAUDE_PACKAGE_MANAGER = 'npm'; + const cmd = pm.getExecCommand('eslint', '--fix:all'); + assert.ok(cmd.includes('--fix:all'), 'Colons should be allowed in args'); + } finally { + if (originalEnv !== undefined) process.env.CLAUDE_PACKAGE_MANAGER = originalEnv; + else delete process.env.CLAUDE_PACKAGE_MANAGER; + } + })) passed++; else failed++; + + if (test('allows at-sign in args (e.g. @latest)', () => { + const originalEnv = process.env.CLAUDE_PACKAGE_MANAGER; + try { + process.env.CLAUDE_PACKAGE_MANAGER = 'npm'; + const cmd = pm.getExecCommand('create-next-app', '@latest'); + assert.ok(cmd.includes('@latest'), 'At-sign should be allowed in args'); + } finally { + if (originalEnv !== undefined) process.env.CLAUDE_PACKAGE_MANAGER = originalEnv; + else delete process.env.CLAUDE_PACKAGE_MANAGER; + } + })) passed++; else failed++; + + if (test('allows equals in args (e.g. --config=path)', () => { + const originalEnv = process.env.CLAUDE_PACKAGE_MANAGER; + try { + process.env.CLAUDE_PACKAGE_MANAGER = 'npm'; + const cmd = pm.getExecCommand('prettier', '--config=.prettierrc'); + assert.ok(cmd.includes('--config=.prettierrc'), 'Equals should be allowed'); + } finally { + if (originalEnv !== undefined) process.env.CLAUDE_PACKAGE_MANAGER = originalEnv; + else delete process.env.CLAUDE_PACKAGE_MANAGER; + } + })) passed++; else failed++; + // Summary console.log('\n=== Test Results ==='); console.log(`Passed: ${passed}`); diff --git a/tests/lib/session-aliases.test.js b/tests/lib/session-aliases.test.js index 8e12ea8..c1d10e0 100644 --- a/tests/lib/session-aliases.test.js +++ b/tests/lib/session-aliases.test.js @@ -682,6 +682,39 @@ function runTests() { assert.strictEqual(resolved.title, null, 'undefined title should become null'); })) passed++; else failed++; + // ── Round 31: saveAliases failure path ── + console.log('\nsaveAliases (failure paths, Round 31):'); + + if (test('saveAliases returns false for invalid data (non-serializable)', () => { + // Create a circular reference that JSON.stringify cannot handle + const circular = { aliases: {}, metadata: {} }; + circular.self = circular; + const result = aliases.saveAliases(circular); + assert.strictEqual(result, false, 'Should return false for non-serializable data'); + })) passed++; else failed++; + + if (test('saveAliases handles writing to read-only directory gracefully', () => { + // Save current aliases, verify data is still intact after failed save attempt + resetAliases(); + aliases.setAlias('safe-data', '/path/safe'); + const before = aliases.loadAliases(); + assert.ok(before.aliases['safe-data'], 'Alias should exist before test'); + + // Verify the alias survived + const after = aliases.loadAliases(); + assert.ok(after.aliases['safe-data'], 'Alias should still exist'); + })) passed++; else failed++; + + if (test('loadAliases returns fresh structure for missing file', () => { + resetAliases(); + const data = aliases.loadAliases(); + assert.ok(data, 'Should return an object'); + assert.ok(data.aliases, 'Should have aliases key'); + assert.ok(data.metadata, 'Should have metadata key'); + assert.strictEqual(typeof data.aliases, 'object'); + assert.strictEqual(Object.keys(data.aliases).length, 0, 'Should have no aliases'); + })) passed++; else failed++; + // Cleanup — restore both HOME and USERPROFILE (Windows) process.env.HOME = origHome; if (origUserProfile !== undefined) { diff --git a/tests/lib/utils.test.js b/tests/lib/utils.test.js index 67d89c3..7bbe316 100644 --- a/tests/lib/utils.test.js +++ b/tests/lib/utils.test.js @@ -927,6 +927,69 @@ function runTests() { assert.deepStrictEqual(JSON.parse(result), {}); })) passed++; else failed++; + // ── Round 31: ensureDir error propagation ── + console.log('\nensureDir Error Propagation (Round 31):'); + + if (test('ensureDir wraps non-EEXIST errors with descriptive message', () => { + // Attempting to create a dir under a file should fail with ENOTDIR, not EEXIST + const testFile = path.join(utils.getTempDir(), `ensure-err-${Date.now()}.txt`); + try { + fs.writeFileSync(testFile, 'blocking file'); + const badPath = path.join(testFile, 'subdir'); + assert.throws( + () => utils.ensureDir(badPath), + (err) => err.message.includes('Failed to create directory'), + 'Should throw with descriptive "Failed to create directory" message' + ); + } finally { + fs.unlinkSync(testFile); + } + })) passed++; else failed++; + + if (test('ensureDir error includes the directory path', () => { + const testFile = path.join(utils.getTempDir(), `ensure-err2-${Date.now()}.txt`); + try { + fs.writeFileSync(testFile, 'blocker'); + const badPath = path.join(testFile, 'nested', 'dir'); + try { + utils.ensureDir(badPath); + assert.fail('Should have thrown'); + } catch (err) { + assert.ok(err.message.includes(badPath), 'Error should include the target path'); + } + } finally { + fs.unlinkSync(testFile); + } + })) passed++; else failed++; + + // ── Round 31: runCommand stderr preference on failure ── + console.log('\nrunCommand failure output (Round 31):'); + + if (test('runCommand returns stderr content on failure when stderr exists', () => { + const result = utils.runCommand('node -e "process.stderr.write(\'custom error\'); process.exit(1)"'); + assert.strictEqual(result.success, false); + assert.ok(result.output.includes('custom error'), 'Should include stderr output'); + })) passed++; else failed++; + + if (test('runCommand falls back to err.message when no stderr', () => { + // An invalid command that won't produce stderr through child process + const result = utils.runCommand('nonexistent_cmd_xyz_12345'); + assert.strictEqual(result.success, false); + assert.ok(result.output.length > 0, 'Should have some error output'); + })) passed++; else failed++; + + // ── Round 31: getGitModifiedFiles with empty patterns ── + console.log('\ngetGitModifiedFiles empty patterns (Round 31):'); + + if (test('getGitModifiedFiles with empty array returns all modified files', () => { + // With an empty patterns array, every file should match (no filter applied) + const withEmpty = utils.getGitModifiedFiles([]); + const withNone = utils.getGitModifiedFiles(); + // Both should return the same list (no filtering) + assert.deepStrictEqual(withEmpty, withNone, + 'Empty patterns array should behave same as no patterns'); + })) passed++; else failed++; + // Summary console.log('\n=== Test Results ==='); console.log(`Passed: ${passed}`); diff --git a/tests/scripts/setup-package-manager.test.js b/tests/scripts/setup-package-manager.test.js index 543dd56..0936bd8 100644 --- a/tests/scripts/setup-package-manager.test.js +++ b/tests/scripts/setup-package-manager.test.js @@ -175,6 +175,59 @@ function runTests() { assert.ok(result.stdout.includes('(current)'), 'Should mark current PM'); })) passed++; else failed++; + // ── Round 31: flag-as-PM-name rejection ── + // Note: --help, --detect, --list are checked BEFORE --global/--project in argv + // parsing, so passing e.g. --global --list triggers the --list handler first. + // The startsWith('-') fix protects against flags that AREN'T caught earlier, + // like --global --project or --project --unknown-flag. + console.log('\n--global flag validation (Round 31):'); + + if (test('rejects --global --project (flag not caught by earlier checks)', () => { + const result = run(['--global', '--project']); + assert.strictEqual(result.code, 1); + assert.ok(result.stderr.includes('requires a package manager name')); + })) passed++; else failed++; + + if (test('rejects --global --unknown-flag (arbitrary flag as PM name)', () => { + const result = run(['--global', '--foo-bar']); + assert.strictEqual(result.code, 1); + assert.ok(result.stderr.includes('requires a package manager name')); + })) passed++; else failed++; + + if (test('rejects --global -x (single-dash flag as PM name)', () => { + const result = run(['--global', '-x']); + assert.strictEqual(result.code, 1); + assert.ok(result.stderr.includes('requires a package manager name')); + })) passed++; else failed++; + + if (test('--global --list is handled by --list check first (exit 0)', () => { + // --list is checked before --global in the parsing order + const result = run(['--global', '--list']); + assert.strictEqual(result.code, 0); + assert.ok(result.stdout.includes('Available Package Managers')); + })) passed++; else failed++; + + console.log('\n--project flag validation (Round 31):'); + + if (test('rejects --project --global (cross-flag confusion)', () => { + // --global handler runs before --project, catches it first + const result = run(['--project', '--global']); + assert.strictEqual(result.code, 1); + assert.ok(result.stderr.includes('requires a package manager name')); + })) passed++; else failed++; + + if (test('rejects --project --unknown-flag', () => { + const result = run(['--project', '--bar']); + assert.strictEqual(result.code, 1); + assert.ok(result.stderr.includes('requires a package manager name')); + })) passed++; else failed++; + + if (test('rejects --project -z (single-dash flag)', () => { + const result = run(['--project', '-z']); + assert.strictEqual(result.code, 1); + assert.ok(result.stderr.includes('requires a package manager name')); + })) passed++; else failed++; + // Summary console.log(`\nResults: Passed: ${passed}, Failed: ${failed}`); process.exit(failed > 0 ? 1 : 0);