-
Notifications
You must be signed in to change notification settings - Fork 8
feat: implement stack settings management system #74
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
48d6f3f
to
2c402f5
Compare
- Add interactive stack-settings.js script for project configuration - Support package manager detection and switching (npm, yarn, pnpm, bun) - Add language detection and switching integration with set-language.js - Implement automatic project-wide command replacement when changing package managers - Add commander.js and inquirer dependencies for CLI interface - Include new npm scripts: stack:settings, stack:status, package-manager commands - Provide detection-only mode with --detect-only flag for status checking The script automatically detects current stack configuration from lock files, package.json, and existing language settings, then allows interactive changes with safe cleanup and reinstallation when switching package managers. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
2c402f5
to
f4b72a8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for implementing the stack settings feature. While this addition brings value to the project, there are critical security and safety issues that need to be addressed before merging:
Blockers:
- Command injection vulnerability (security risk)
- Destructive operations without confirmation
- Unexpected git operations
Please focus on these security and safety issues first.
} | ||
} | ||
|
||
// TODO replace: when old or new bun, replace Node.js texts |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's an unfinished TODO comment that should be addressed or removed before merging:
Please either:
- Implement the missing functionality
- Remove if no longer needed
cwd: projectRoot, | ||
encoding: 'utf8', | ||
}) | ||
execSync(`git add ${lockFileMap[newManager]}`, { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
String concatenation in execSync creates a command injection vulnerability. This is particularly dangerous in a boilerplate project, as unsafe patterns will be copied into production code.
execSync(`git add ${lockFileMap[newManager]}`, { | |
const result = spawnSync('git', ['add', lockFileMap[newManager]], { | |
cwd: projectRoot, | |
stdio: 'inherit', | |
encoding: 'utf8' | |
}); | |
if (result.error) { | |
throw new Error(`Failed to add lock file: ${result.error.message}`); | |
} |
This prevents any possibility of shell injection by passing arguments as an array.
* @param {string} oldManager - Previous package manager | ||
* @param {string} newManager - New package manager to use | ||
*/ | ||
async function handlePackageManagerChange(oldManager, newManager) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before performing destructive operations (like deleting node_modules
or removing lock files), the script should always ask for user confirmation. This should be done through an interactive prompt, with an optional --force
flag for CI environments.
Here’s a sample snippet for the confirmation part:
// Show clear summary and get confirmation
const message = `
This operation will:
${changes.deleteNodeModules ? ' ✗ Delete node_modules directory\n' : ''}${changes.deleteLockFile ? ` ✗ Remove ${oldLockFile}\n` : ''} ✗ Update package manager commands in ~${changes.filesCount} files
✓ Run ${newManager} install
Continue?`;
const confirmed = await confirm({ message, default: false });
if (!confirmed) {
console.log('Operation cancelled.');
process.exit(0);
}
(This is a sample implementation. You can simplify or adapt it as needed.)
cwd: projectRoot, | ||
encoding: 'utf8', | ||
}) | ||
execSync(`git add ${lockFileMap[newManager]}`, { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The script automatically runs git add
without user consent, which can disrupt git workflows and cause unexpected behavior in CI/CD pipelines.
- Solution 1 (Recommended): Remove git operations entirely and let users manage git themselves
- Solution 2: Make it opt-in via a CLI flag (e.g.
--git-add
)
The script automatically detects current stack configuration from lock files, package.json, and existing language settings, then allows interactive changes with safe cleanup and reinstallation when switching package managers.
🤖 Generated with Claude Code