Skip to content

Conversation

@hongquan
Copy link
Contributor

Fix #58

@github-actions
Copy link

Here's the comprehensive review of the code changes:

Script Analysis

  • Improved error handling in format_error with proper empty check before processing
  • Replaced cat /etc/shells with open /etc/shells for better Nu-native file handling
  • Fixed newline handling in add-shells function to prevent malformed file appends
  • Added proper file termination handling in remove-nu-from-shells to maintain valid file format
  • Modified test assertion in test nu is not the only shell to be more flexible (from >1 to >0)
  • Better structured data flow in shell manipulation operations

Security Review

  • ❗ Potential security issue in test Nu version is correct where it runs external nu --version without sanitization
  • ❗ All /etc/shells modifications require root privileges but don't validate execution context
  • ⚠️ File operations don't include error handling for permission failures
  • ✅ Improved security in file handling by using Nushell's builtin open instead of external cat

Optimization Suggestions

  • ✅ All instances of cat replaced with builtin open for better performance
  • ✅ File operations now handle edge cases (newlines) more efficiently
  • 💡 Consider adding let-env validation in main functions to ensure proper execution context
  • 💡 Could use try/catch blocks for more robust error handling in file operations

Overall Quality: 4

The changes show good attention to detail in file handling and data processing, with proper Nu 0.90+ compatibility. The security fixes around builtin command usage are excellent, though some permission/context validation would make it even better. The newline handling improvements are particularly well-done.

@hustcer
Copy link
Contributor

hustcer commented Jun 11, 2025

Thanks, let's give it a try

@hustcer hustcer merged commit 67cdea8 into nushell:main Jun 11, 2025
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

deb package overwrites /etc/shells

2 participants