Merged
Conversation
Introduces Invoke-SudoCommand and Test-SudoRequired helpers to handle sudo requirements for system-wide installs and uninstalls on Linux. Updates Install-AITool and Uninstall-AITool to use these helpers, ensuring proper privilege escalation and user prompts. Improves documentation in README to clarify installation scopes and privilege requirements.
Introduces tests to verify the Scope parameter handling for both Install-AITool and Uninstall-AITool, including validation of accepted values, parameter types, and default behaviors for CurrentUser and LocalMachine scopes.
Pull Request Review: Sudo and Scope SupportStrengths
Critical IssuesInconsistent sudo handling (Install-AITool.ps1:412) Medium Priority
Testing GapsTests only verify parameter attributes and already-installed path. Need unit tests for actual sudo logic, error paths, and platform-specific behavior. StyleInconsistent string quotes - use single quotes for literals per PowerShell conventions Security
PerformanceConsider caching sudo checks or adding -SkipSudoCheck parameter Recommendation: Approve with changes Well-designed feature! Main fixes needed:
Great work on documentation and architecture! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request adds robust support for user-local and system-wide installation and uninstallation scopes on Linux, including proper sudo handling. It introduces helper functions to determine when sudo is needed and to prompt for credentials when required. The documentation and tests have been updated to reflect these changes.
Scope-aware installation and uninstallation with sudo handling:
Invoke-SudoCommand(private/Invoke-SudoCommand.ps1) to execute commands with sudo when required, including credential validation and clear error messages.Test-SudoRequired(private/Test-SudoRequired.ps1) to determine if sudo is needed based on OS and installation scope.Install-AIToolto useInvoke-SudoCommandfor system-wide (LocalMachine) pipx installs and improved Node.js installation logic to handle sudo and error reporting more gracefully. [1] [2] [3]Uninstall-AIToolto support theScopeparameter and useInvoke-SudoCommandfor system-wide uninstalls on Linux. [1] [2] [3] [4]Documentation and testing updates:
Scopeparameter for both installation and uninstallation, including validation of allowed values and default behaviors.Uninstall-AITooldocumentation.