feat(shim): enhance support for managing local executable shims #6504
Conversation
- `scoop shim add/rm` now remembers manually added shims in the config file. - `scoop shim list` adds a `--added` option to show only manually added shims; output uses `Format-Table`. - `scoop shim info` and `scoop shim list` display an `isAdded` field to indicate if a shim was manually added. - `scoop shim alter` now supports paths to non-scoop-installed executables.
WalkthroughAdds Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant CLI as scoop-shim.ps1
participant FS as Filesystem
participant Cfg as AddedShims Config
rect rgb(235,245,255)
note over CLI: Add subcommand
User->>CLI: shim add <name> <path> [--global] [args]
CLI->>FS: Create shim at local/global path
FS-->>CLI: Shim created
CLI->>Cfg: Add-ShimToConfig(name, global, path, args)
Cfg-->>CLI: Persisted
CLI-->>User: Added shim (local/global)
end
sequenceDiagram
autonumber
actor User
participant CLI as scoop-shim.ps1
participant FS as Filesystem
participant Cfg as AddedShims Config
rect rgb(245,255,235)
note over CLI: List subcommand
User->>CLI: shim list [--added] [--global]
alt --added
CLI->>Cfg: Get-AddedShimsConfig()
Cfg-->>CLI: Entries
CLI->>CLI: Compute shim paths by global flag
else filesystem scan
CLI->>FS: Enumerate shims (local/global)
FS-->>CLI: Shim paths
CLI->>Cfg: Test-ShimAdded per shim
Cfg-->>CLI: IsAdded flags
end
CLI-->>User: Table of shims (+IsAdded)
end
sequenceDiagram
autonumber
actor User
participant CLI as scoop-shim.ps1
participant FS as Filesystem
participant Cfg as AddedShims Config
rect rgb(255,245,235)
note over CLI: Remove subcommand
User->>CLI: shim rm <name> [--global]
CLI->>FS: Delete shim at local/global path
FS-->>CLI: Deleted/NotFound
CLI->>Cfg: Remove-ShimFromConfig(name, global)
Cfg-->>CLI: Removed/No-op
CLI-->>User: Removal result
end
sequenceDiagram
autonumber
actor Caller
participant Core as lib/core.ps1
Caller->>Core: get_app_name(path)
alt matches appsdir pattern
Core-->>Caller: appName (matched)
else matches appsdir true pattern
Core-->>Caller: appName (matched)
else fallback
Core->>Core: Regex extract filename, toLower()
Core-->>Caller: appName (derived or empty)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (7)
libexec/scoop-shim.ps1 (7)
46-53: Consider error handling for config operations.
Get-AddedShimsConfigcallsget_configwhich may fail or return unexpected data. Consider adding error handling to ensure the function always returns a valid object.function Get-AddedShimsConfig { - $added = get_config 'added' - if ($null -eq $added) { - return [PSCustomObject]@{} + try { + $added = get_config 'added' + if ($null -eq $added -or $added -isnot [PSCustomObject]) { + return [PSCustomObject]@{} + } + return $added + } catch { + warn "Failed to read added shims config: $_" + return [PSCustomObject]@{} } - return $added }
56-75: Validate parameters and handle edge cases.The
Add-ShimToConfigfunction should validate inputs and handle potential failures:
- Empty or null
$shimNamecould cause issues$commandArgsmight be$nullwhen joining- No error handling if
set_configfailsfunction Add-ShimToConfig($shimName, $global, $commandPath, $commandArgs) { + if ([string]::IsNullOrWhiteSpace($shimName)) { + throw "shimName cannot be empty" + } + $added = Get-AddedShimsConfig # Use new structure: shimName as key, global as a boolean field $shimInfo = [PSCustomObject]@{ CommandPath = $commandPath - CommandArgs = $commandArgs -join ' ' + CommandArgs = if ($commandArgs) { $commandArgs -join ' ' } else { '' } AddedDate = (Get-Date).ToString('yyyy-MM-dd HH:mm:ss') global = $global } # If shimName does not exist, add it if (-not $added.PSObject.Properties[$shimName]) { $added | Add-Member -MemberType NoteProperty -Name $shimName -Value $shimInfo } else { $added.$shimName = $shimInfo } - set_config 'added' $added | Out-Null + try { + set_config 'added' $added | Out-Null + } catch { + error "Failed to save shim configuration: $_" + throw + } }
154-158: Fix inconsistent indentation.Line 158 has incorrect indentation with the closing brace.
error "<command_path> must be specified for subcommand 'add'" my_usage exit 1 - } + }
164-172: Fix inconsistent indentation.Line 171 has incorrect indentation with the closing brace.
$exCommand = Get-Command $shortPath -ErrorAction SilentlyContinue if ($exCommand -and $exCommand.CommandType -eq 'Application') { $commandPath = $exCommand.Path - } + } }
206-222: Handle edge case: empty pattern array.If
$otheris empty,$patternsbecomes an empty array after filtering out'*'. The code should handle this case explicitly to avoid unexpected behavior.- # Handle pattern matching - $patterns = @($other) -ne '*' + # Handle pattern matching: if no patterns provided, match all + $patterns = @($other) | Where-Object { $_ -ne '*' } + $hasPatterns = $patterns.Count -gt 0 # Validate regex patterns $patterns | ForEach-Object { try { $pattern = $_ [void][Regex]::New($pattern) } catch { Write-Host "ERROR: Invalid pattern: " -ForegroundColor Red -NoNewline Write-Host $pattern -ForegroundColor Magenta exit 1 } } - $pattern = $patterns -join '|' + $pattern = if ($hasPatterns) { $patterns -join '|' } else { $null }
225-243: Consider performance impact of config-based listing.When
--addedis used, the code iterates through config entries and callsGet-ShimPathfor each one. If a shim was removed from the filesystem but not from config, it will be silently skipped. Consider notifying the user about orphaned config entries.# Iterate over each shim in config $added.PSObject.Properties | ForEach-Object { $shimName = $_.Name $shimConfig = $_.Value # Apply regex filter if (!$pattern -or ($shimName -match $pattern)) { # Determine shim path based on global flag $shimPath = Get-ShimPath $shimName $shimConfig.global if ($shimPath) { $shimInfos += Get-ShimInfo $shimPath + } else { + warn "Shim '$shimName' is in config but not found in filesystem" } } }
330-330: Inconsistent exit code.Line 330 uses
exitwithout an exit code, while other error paths useexit 3. This creates inconsistent behavior.- exit + exit 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.gitignore(1 hunks)lib/core.ps1(1 hunks)libexec/scoop-shim.ps1(10 hunks)
🔇 Additional comments (4)
.gitignore (1)
10-10: LGTM!Adding
.vscode/*to.gitignoreis a standard practice to avoid committing IDE-specific configuration files.libexec/scoop-shim.ps1 (2)
175-180: LGTM with minor suggestion.The add subcommand correctly persists shims to config after creating them. The messaging clearly indicates whether it's a global or local shim.
One small improvement: Consider adding error handling around
Add-ShimToConfigin case config persistence fails after the shim is already created.
191-193: LGTM!The rm subcommand correctly removes shims from both the filesystem and config. The global flag is properly passed through.
lib/core.ps1 (1)
838-845: Critical: Fix syntax errors and remove debug statements.Multiple critical issues detected:
Syntax Error (Lines 841, 844): PowerShell doesn't support the
+operator for string concatenation withWrite-Host. The current codeWrite-Host $path + '1' + $appNamewill output three separate values, not a concatenated string.Debug Code in Production: The
Write-Hoststatements appear to be diagnostic/debug output that should not be in production code. These will pollute the console output for all users.Regex Pattern Concern: The pattern
[/\\]([^/\\]+?)(?:\.[^/\\"]+?)uses a non-capturing group(?:...)but doesn't capture the extension, and the comment suggests it should handle filenames with extensions. The pattern may not work as intended for all executable paths.Apply this diff to fix the syntax errors and remove debug output:
} elseif ($path -match '[/\\]([^/\\]+?)(?:\.[^/\\"]+?)') { - # Fallback: extract the executable file name (with extension if present): [/\\]([^/\\]+?)(?:\.?[^./\\]+?)`" $appName = $Matches[1].ToLower() - Write-Host $path + '1' + $appName } else { $appName = '' - Write-Host $path + '2' + $appName }If diagnostic output is needed for debugging, use the
debugfunction instead:} elseif ($path -match '[/\\]([^/\\]+?)(?:\.[^/\\"]+?)') { $appName = $Matches[1].ToLower() + debug "Fallback extraction: path=$path, appName=$appName" } else { $appName = '' + debug "No app name extracted: path=$path" }Likely an incorrect or invalid review comment.
libexec/scoop-shim.ps1
Outdated
| . "$PSScriptRoot\..\apps\scoop\current\lib\getopt.ps1" | ||
| . "$PSScriptRoot\..\apps\scoop\current\lib\core.ps1" # for config related ops | ||
| . "$PSScriptRoot\..\apps\scoop\current\lib\install.ps1" # for rm_shim | ||
| . "$PSScriptRoot\..\apps\scoop\current\lib\system.ps1" # 'Add-Path' (indirectly) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Verify hardcoded relative paths.
The import paths use hardcoded relative paths ($PSScriptRoot\..\apps\scoop\current\...) that assume a specific directory structure. This will break if:
- The script is moved to a different location
- Scoop is installed in a non-standard location
- The script is invoked from a different context
Run the following script to check if there are standard path resolution patterns used elsewhere in the codebase:
🏁 Script executed:
#!/bin/bash
# Search for how other libexec scripts resolve paths to lib files
rg -n -C3 '^\. .*(lib|supporting)/' libexec/ | head -50Length of output: 0
🏁 Script executed:
#!/bin/bash
# List all scripts under libexec to identify candidates
find libexec -maxdepth 1 -type f -print
# Search for PSScriptRoot usage to find alternative resolution patterns
rg -n '\$PSScriptRoot' -C2 libexec/Length of output: 16025
🏁 Script executed:
#!/bin/bash
# Verify if apps/scoop/current/lib exists in the repo
find . -maxdepth 5 -type d -path "./apps/scoop/current/lib"
# If found, list its contents
if [ -d "apps/scoop/current/lib" ]; then
ls -1 apps/scoop/current/lib
else
echo "apps/scoop/current/lib not found"
fiLength of output: 32
Derive the scoop install path dynamically instead of hardcoded relative imports in scoop-shim.ps1. Lines 40–43 import from $PSScriptRoot\..\apps\scoop\current\lib\*.ps1, which assumes a fixed install layout (and diverges from other libexec scripts that load from ..\lib). Refactor to compute the scoop root at runtime (e.g. via an environment variable or config) or clearly document the required directory structure. [lines 40–43]
| function Test-ShimAdded($shimName, $gqlobal) { | ||
| $added = Get-AddedShimsConfig | ||
|
|
||
| # Check if shim exists and matches global flag | ||
| return ($added.PSObject.Properties[$shimName] -and $added.$shimName.global -eq $global) | ||
| } |
There was a problem hiding this comment.
Critical: Fix parameter name typo causing undefined variable reference.
Line 89 declares the parameter as $gqlobal (typo), but line 93 references $global, which is undefined in this function scope. This will cause the function to always return $false or produce incorrect results.
Apply this diff to fix the typo:
-function Test-ShimAdded($shimName, $gqlobal) {
+function Test-ShimAdded($shimName, $global) {
$added = Get-AddedShimsConfig
# Check if shim exists and matches global flag
return ($added.PSObject.Properties[$shimName] -and $added.$shimName.global -eq $global)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function Test-ShimAdded($shimName, $gqlobal) { | |
| $added = Get-AddedShimsConfig | |
| # Check if shim exists and matches global flag | |
| return ($added.PSObject.Properties[$shimName] -and $added.$shimName.global -eq $global) | |
| } | |
| function Test-ShimAdded($shimName, $global) { | |
| $added = Get-AddedShimsConfig | |
| # Check if shim exists and matches global flag | |
| return ($added.PSObject.Properties[$shimName] -and $added.$shimName.global -eq $global) | |
| } |
🤖 Prompt for AI Agents
In libexec/scoop-shim.ps1 around lines 89 to 94, the function Test-ShimAdded
declares the parameter as $gqlobal but later checks $global, causing an
undefined variable reference; rename the parameter to $global (or update all
references to match the intended name) so the parameter and its usage are
consistent, then run a quick syntax check to ensure no other references to the
misspelled name remain.
| if ($SubCommand -ne 'list' -and $other.Length -eq 0) { | ||
| error "<shim_name> must be specified for subcommand '$SubCommand'" | ||
| my_usage | ||
| my_usages |
There was a problem hiding this comment.
🧩 Analysis chain
Verify function name change.
Line 115 calls my_usages (plural), but the help text at the top shows my_usage (singular). Ensure this function exists in the imported modules.
🏁 Script executed:
#!/bin/bash
# Search for my_usage and my_usages function definitions
rg -n "function (my_usage|my_usages)" lib/ supporting/ libexec/Length of output: 71
Rename function call to match defined helper
In libexec/scoop-shim.ps1 at line 115, the code calls my_usages, but only my_usage (singular) is defined (lib/help.ps1:14). Update the invocation:
- my_usages
+ my_usage📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| my_usages | |
| my_usage |
🤖 Prompt for AI Agents
In libexec/scoop-shim.ps1 around line 115, the script calls my_usages but the
defined helper is my_usage (singular); replace the incorrect call with my_usage
so the correct function is invoked and avoid undefined function errors.
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (2)
libexec/scoop-shim.ps1 (2)
89-94: Critical: typo in parameter name causes undefined variable usage.
$gqlobalvs$globalbreaks the check; also avoid dotted access for the same reason as above.-function Test-ShimAdded($shimName, $gqlobal) { +function Test-ShimAdded($shimName, $global) { $added = Get-AddedShimsConfig # Check if shim exists and matches global flag - return ($added.PSObject.Properties[$shimName] -and $added.$shimName.global -eq $global) + $prop = $added.PSObject.Properties[$shimName] + return ($prop -and $prop.Value.global -eq $global) }
115-115: Call the correct helper: my_usage (singular), not my_usages.- my_usages + my_usage
🧹 Nitpick comments (3)
libexec/scoop-shim.ps1 (3)
179-181: Persisting args: avoid lossy join when calling shim and saving config.If
$commandArgsis an array, passing it as-is preserves tokenization; the config change above stores the array.- shim $commandPath $global $shimName $commandArgs + if ($null -ne $commandArgs) { + shim $commandPath $global $shimName @($commandArgs) + } else { + shim $commandPath $global $shimName + }
248-257: Guard local scan with Test-Path to avoid exceptions on fresh installs.
$localShimDirmay not exist yet.- if (!$global) { - $shims += Get-ChildItem -Path $localShimDir -Recurse -Include '*.shim', '*.ps1' | + if (!$global -and (Test-Path -LiteralPath $localShimDir)) { + $shims += Get-ChildItem -Path $localShimDir -Recurse -Include '*.shim','*.ps1' | Where-Object { !$pattern -or ($_.BaseName -match $pattern) } | Select-Object -ExpandProperty FullName }
262-262: Consider not forcing Format-Table for scriptability.Formatting at the cmdlet breaks piping. Prefer emitting objects and letting type data shape the view; add
--tablelater if needed.- $shimInfos | Format-Table -Property * -AutoSize + $shimInfos
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.gitignore(1 hunks)libexec/scoop-shim.ps1(10 hunks)
✅ Files skipped from review due to trivial changes (1)
- .gitignore
🔇 Additional comments (3)
libexec/scoop-shim.ps1 (3)
138-140: IsAdded depends on Test-ShimAdded fix; re-evaluate after correcting property access.Once Test-ShimAdded is fixed, this is fine. Consider guarding with
$null -ne $info.Nameif any shim has unexpected names.
192-194: Nice: config removal mirrored in rm flow.This keeps filesystem and config consistent. Reuse the property-bag-safe Remove-ShimFromConfig after you patch it.
41-41: Import comment is fine; verify core helpers availability for config ops.Assuming
get_config/set_configare from core.ps1, this import is correct. Please confirm no circular import.
| function Add-ShimToConfig($shimName, $global, $commandPath, $commandArgs) { | ||
| $added = Get-AddedShimsConfig | ||
|
|
||
| # Use new structure: shimName as key, global as a boolean field | ||
| $shimInfo = [PSCustomObject]@{ | ||
| CommandPath = $commandPath | ||
| CommandArgs = $commandArgs -join ' ' | ||
| AddedDate = (Get-Date).ToString('yyyy-MM-dd HH:mm:ss') | ||
| global = $global | ||
| } | ||
|
|
||
| # If shimName does not exist, add it | ||
| if (-not $added.PSObject.Properties[$shimName]) { | ||
| $added | Add-Member -MemberType NoteProperty -Name $shimName -Value $shimInfo | ||
| } else { | ||
| $added.$shimName = $shimInfo | ||
| } | ||
|
|
||
| set_config 'added' $added | Out-Null | ||
| } |
There was a problem hiding this comment.
Avoid dotted property access for arbitrary shim names; use PSObject property bag.
Shim names may contain dots/hyphens. $added.$shimName will break. Use PSObject.Properties[$name] for both add and update. Also store args as an array to preserve tokens.
function Add-ShimToConfig($shimName, $global, $commandPath, $commandArgs) {
$added = Get-AddedShimsConfig
# Use new structure: shimName as key, global as a boolean field
$shimInfo = [PSCustomObject]@{
CommandPath = $commandPath
- CommandArgs = $commandArgs -join ' '
+ CommandArgs = $commandArgs
AddedDate = (Get-Date).ToString('yyyy-MM-dd HH:mm:ss')
global = $global
}
- # If shimName does not exist, add it
- if (-not $added.PSObject.Properties[$shimName]) {
- $added | Add-Member -MemberType NoteProperty -Name $shimName -Value $shimInfo
- } else {
- $added.$shimName = $shimInfo
- }
+ $prop = $added.PSObject.Properties[$shimName]
+ if ($prop) {
+ $prop.Value = $shimInfo
+ } else {
+ $added | Add-Member -MemberType NoteProperty -Name $shimName -Value $shimInfo
+ }
set_config 'added' $added | Out-Null
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function Add-ShimToConfig($shimName, $global, $commandPath, $commandArgs) { | |
| $added = Get-AddedShimsConfig | |
| # Use new structure: shimName as key, global as a boolean field | |
| $shimInfo = [PSCustomObject]@{ | |
| CommandPath = $commandPath | |
| CommandArgs = $commandArgs -join ' ' | |
| AddedDate = (Get-Date).ToString('yyyy-MM-dd HH:mm:ss') | |
| global = $global | |
| } | |
| # If shimName does not exist, add it | |
| if (-not $added.PSObject.Properties[$shimName]) { | |
| $added | Add-Member -MemberType NoteProperty -Name $shimName -Value $shimInfo | |
| } else { | |
| $added.$shimName = $shimInfo | |
| } | |
| set_config 'added' $added | Out-Null | |
| } | |
| function Add-ShimToConfig($shimName, $global, $commandPath, $commandArgs) { | |
| $added = Get-AddedShimsConfig | |
| # Use new structure: shimName as key, global as a boolean field | |
| $shimInfo = [PSCustomObject]@{ | |
| CommandPath = $commandPath | |
| CommandArgs = $commandArgs | |
| AddedDate = (Get-Date).ToString('yyyy-MM-dd HH:mm:ss') | |
| global = $global | |
| } | |
| $prop = $added.PSObject.Properties[$shimName] | |
| if ($prop) { | |
| $prop.Value = $shimInfo | |
| } else { | |
| $added | Add-Member -MemberType NoteProperty -Name $shimName -Value $shimInfo | |
| } | |
| set_config 'added' $added | Out-Null | |
| } |
| # Remove shim from config | ||
| function Remove-ShimFromConfig($shimName, $global) { | ||
| $added = Get-AddedShimsConfig | ||
|
|
||
| # Check if shim exists and matches global flag | ||
| if ($added.PSObject.Properties[$shimName] -and $added.$shimName.global -eq $global) { | ||
| $added.PSObject.Properties.Remove($shimName) | ||
| set_config 'added' $added | Out-Null | ||
| } | ||
| } |
There was a problem hiding this comment.
Property removal must not rely on dotted access; compare flag via property value.
Direct $added.$shimName.global fails for keys with special chars. Use the property bag value.
function Remove-ShimFromConfig($shimName, $global) {
$added = Get-AddedShimsConfig
# Check if shim exists and matches global flag
- if ($added.PSObject.Properties[$shimName] -and $added.$shimName.global -eq $global) {
- $added.PSObject.Properties.Remove($shimName)
+ $prop = $added.PSObject.Properties[$shimName]
+ if ($prop -and $prop.Value.global -eq $global) {
+ [void]$added.PSObject.Properties.Remove($shimName)
set_config 'added' $added | Out-Null
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Remove shim from config | |
| function Remove-ShimFromConfig($shimName, $global) { | |
| $added = Get-AddedShimsConfig | |
| # Check if shim exists and matches global flag | |
| if ($added.PSObject.Properties[$shimName] -and $added.$shimName.global -eq $global) { | |
| $added.PSObject.Properties.Remove($shimName) | |
| set_config 'added' $added | Out-Null | |
| } | |
| } | |
| # Remove shim from config | |
| function Remove-ShimFromConfig($shimName, $global) { | |
| $added = Get-AddedShimsConfig | |
| # Check if shim exists and matches global flag | |
| $prop = $added.PSObject.Properties[$shimName] | |
| if ($prop -and $prop.Value.global -eq $global) { | |
| [void]$added.PSObject.Properties.Remove($shimName) | |
| set_config 'added' $added | Out-Null | |
| } | |
| } |
🤖 Prompt for AI Agents
In libexec/scoop-shim.ps1 around lines 77 to 86, the code uses dotted access
($added.$shimName.global) which breaks for property names with special
characters; instead retrieve the property object and read its Value field.
Change the condition to get the property: $prop =
$added.PSObject.Properties[$shimName]; then check if ($prop -and
$prop.Value.global -eq $global) before removing the property and calling
set_config 'added' $added | Out-Null.
| if (-not (Get-FormatData ScoopShims)) { | ||
| Update-FormatData "$PSScriptRoot\..\supporting\formats\ScoopTypes.Format.ps1xml" | ||
| Update-FormatData "$PSScriptRoot\..\apps\scoop\current\supporting\formats\ScoopTypes.Format.ps1xml" | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Derive format data path robustly; avoid hardcoded apps\scoop\current.
Fallback to repo-relative supporting path first; only try the apps path if present.
-if (-not (Get-FormatData ScoopShims)) {
- Update-FormatData "$PSScriptRoot\..\apps\scoop\current\supporting\formats\ScoopTypes.Format.ps1xml"
-}
+if (-not (Get-FormatData ScoopShims)) {
+ $candidates = @(
+ "$PSScriptRoot\..\supporting\formats\ScoopTypes.Format.ps1xml",
+ "$PSScriptRoot\..\apps\scoop\current\supporting\formats\ScoopTypes.Format.ps1xml"
+ )
+ foreach ($p in $candidates) {
+ if (Test-Path -LiteralPath $p) { Update-FormatData $p; break }
+ }
+}🤖 Prompt for AI Agents
In libexec/scoop-shim.ps1 around lines 119 to 121, the format data path is
hardcoded to ..\apps\scoop\current\supporting\formats\ScoopTypes.Format.ps1xml;
change it to first try a repo-relative supporting path (e.g. resolve
"$PSScriptRoot\..\supporting\formats\ScoopTypes.Format.ps1xml" or similar
relative-to-repo location) and if that file doesn't exist, fall back to the apps
path only if Test-Path confirms it exists; construct both candidate paths with
Join-Path/Resolve-Path based on $PSScriptRoot, check existence with Test-Path,
and call Update-FormatData with the first existing path (or skip/emit a warning
if neither exists).
| if ($showOnlyAdded) { | ||
| # When --added option is used, read from config directly | ||
| $added = Get-AddedShimsConfig | ||
|
|
||
| # Iterate over each shim in config | ||
| $added.PSObject.Properties | ForEach-Object { | ||
| $shimName = $_.Name | ||
| $shimConfig = $_.Value | ||
|
|
||
| # Apply regex filter | ||
| if (!$pattern -or ($shimName -match $pattern)) { | ||
| # Determine shim path based on global flag | ||
| $shimPath = Get-ShimPath $shimName $shimConfig.global | ||
|
|
||
| if ($shimPath) { | ||
| $shimInfos += Get-ShimInfo $shimPath | ||
| } | ||
| } | ||
| } | ||
| } else { |
There was a problem hiding this comment.
Honor -g with --added; current code lists both scopes.
Mirror the non---added behavior: without -g, show both; with -g, show only global.
- # Apply regex filter
- if (!$pattern -or ($shimName -match $pattern)) {
- # Determine shim path based on global flag
- $shimPath = Get-ShimPath $shimName $shimConfig.global
- if ($shimPath) {
- $shimInfos += Get-ShimInfo $shimPath
- }
- }
+ # Apply regex filter and optional -g scope filter
+ if ((-not $pattern -or ($shimName -match $pattern)) -and ((-not $global) -or $shimConfig.global)) {
+ $shimPath = Get-ShimPath $shimName $shimConfig.global
+ if ($shimPath) { $shimInfos += Get-ShimInfo $shimPath }
+ }🤖 Prompt for AI Agents
In libexec/scoop-shim.ps1 around lines 225 to 244, the --added branch currently
ignores the -g/$global switch and always uses the shim's configured scope;
adjust the loop to honor -g by filtering added entries: if $global is set only
include shims whose $shimConfig.global is true, otherwise include all added
shims (no scope filter). Implement this by checking the $global switch inside
the ForEach-Object and skipping entries that are not global when $global is
present, then continue to call Get-ShimPath/Get-ShimInfo as before.
| exit 2 | ||
| } | ||
| exit 3 | ||
| exit |
There was a problem hiding this comment.
Return a consistent non-zero exit code on error path.
Use exit 3 (as above) instead of bare exit.
- exit
+ exit 3📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| exit | |
| exit 3 |
🤖 Prompt for AI Agents
In libexec/scoop-shim.ps1 around line 330, the error path uses a bare "exit"
which returns a zero/ambiguous status; change it to "exit 3" to return a
consistent non-zero exit code (matching other error paths). Replace the bare
exit statement with exit 3 so callers can reliably detect failure.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
libexec/scoop-shim.ps1 (2)
174-185: Missing error handling for config persistence.If
Add-ShimToConfigor its internalset_configcall fails (line 180), the shim will be created but not tracked in config, leading to inconsistent state. Consider wrapping in try/catch or checking the result.Consider adding error handling:
if ($commandPath -and (Test-Path $commandPath)) { Write-Host "Adding $(if ($global) { 'global' } else { 'local' }) shim " -NoNewline Write-Host $shimName -ForegroundColor Cyan -NoNewline Write-Host '...' shim $commandPath $global $shimName $commandArgs - # Save shim to config - Add-ShimToConfig $shimName $global $commandPath $commandArgs + try { + Add-ShimToConfig $shimName $global $commandPath $commandArgs + } catch { + warn "Failed to save shim to config: $_" + } } else {
187-204: Missing error handling for config cleanup.If
Remove-ShimFromConfigfails (line 193), the shim will be deleted but remain tracked in config, creating inconsistent state.Consider adding error handling:
$other | ForEach-Object { if (Get-ShimPath $_ $global) { rm_shim $_ (shimdir $global) - # Remove shim from config - Remove-ShimFromConfig $_ $global + try { + Remove-ShimFromConfig $_ $global + } catch { + warn "Failed to remove shim from config: $_" + } } else {
♻️ Duplicate comments (7)
libexec/scoop-shim.ps1 (7)
119-121: Format data path should handle missing files gracefully.While the path now uses
supporting\formatsinstead of the hardcodedapps\scoop\currentpath, there's no fallback or error handling if the file doesn't exist.Consider adding existence check:
if (-not (Get-FormatData ScoopShims)) { - Update-FormatData "$PSScriptRoot\..\supporting\formats\ScoopTypes.Format.ps1xml" + $formatPath = "$PSScriptRoot\..\supporting\formats\ScoopTypes.Format.ps1xml" + if (Test-Path -LiteralPath $formatPath) { + Update-FormatData $formatPath + } }
89-94: CRITICAL: Parameter typo causes undefined variable reference.Line 89 declares
$gqlobal(typo) but line 93 references$global, which is undefined in this scope. This function will always return$falseor produce incorrect results.Apply this diff:
-function Test-ShimAdded($shimName, $gqlobal) { +function Test-ShimAdded($shimName, $global) { $added = Get-AddedShimsConfig # Check if shim exists and matches global flag return ($added.PSObject.Properties[$shimName] -and $added.$shimName.global -eq $global) }
56-75: Property access will fail for shim names with special characters.Line 71 uses
$added.$shimNamewhich breaks when$shimNamecontains dots, hyphens, or other special characters. Additionally, line 62 joins args as a string, losing token boundaries.Apply this diff:
function Add-ShimToConfig($shimName, $global, $commandPath, $commandArgs) { $added = Get-AddedShimsConfig # Use new structure: shimName as key, global as a boolean field $shimInfo = [PSCustomObject]@{ CommandPath = $commandPath - CommandArgs = $commandArgs -join ' ' + CommandArgs = $commandArgs AddedDate = (Get-Date).ToString('yyyy-MM-dd HH:mm:ss') global = $global } - # If shimName does not exist, add it - if (-not $added.PSObject.Properties[$shimName]) { - $added | Add-Member -MemberType NoteProperty -Name $shimName -Value $shimInfo - } else { - $added.$shimName = $shimInfo - } + $prop = $added.PSObject.Properties[$shimName] + if ($prop) { + $prop.Value = $shimInfo + } else { + $added | Add-Member -MemberType NoteProperty -Name $shimName -Value $shimInfo + } set_config 'added' $added | Out-Null }
77-86: Property access will fail for shim names with special characters.Line 82 uses
$added.$shimName.globalwhich breaks when$shimNamecontains dots, hyphens, or other special characters.Apply this diff:
function Remove-ShimFromConfig($shimName, $global) { $added = Get-AddedShimsConfig # Check if shim exists and matches global flag - if ($added.PSObject.Properties[$shimName] -and $added.$shimName.global -eq $global) { - $added.PSObject.Properties.Remove($shimName) + $prop = $added.PSObject.Properties[$shimName] + if ($prop -and $prop.Value.global -eq $global) { + [void]$added.PSObject.Properties.Remove($shimName) set_config 'added' $added | Out-Null } }
115-115: Function name error -my_usagesdoes not exist.Line 115 calls
my_usages, but onlymy_usage(singular) is defined in lib/help.ps1. This will cause a runtime error.Apply this diff:
- my_usages + my_usage
225-244: The--addedbranch ignores the-gflag.When
--addedis used, the code lists all manually added shims regardless of the-g/--globalflag. Users would expect-g --addedto show only global added shims.Apply this diff to honor the
-gflag:# Apply regex filter - if (!$pattern -or ($shimName -match $pattern)) { + if ((!$pattern -or ($shimName -match $pattern)) -and ((-not $global) -or $shimConfig.global)) { # Determine shim path based on global flag $shimPath = Get-ShimPath $shimName $shimConfig.global - if ($shimPath) { $shimInfos += Get-ShimInfo $shimPath } }
330-330: Use consistent non-zero exit code.Line 330 uses bare
exitwhich returns zero/ambiguous status. Other error paths in this file useexit 3.Apply this diff:
- exit + exit 3
🧹 Nitpick comments (2)
libexec/scoop-shim.ps1 (2)
154-158: Inconsistent indentation on closing brace.Line 158's closing brace is indented excessively compared to the rest of the code block.
Apply this diff:
if ($other.Length -lt 2 -or $other[1] -eq '') { error "<command_path> must be specified for subcommand 'add'" my_usage exit 1 - } + }
164-172: Inconsistent indentation on closing brace.Line 171's closing brace is indented excessively compared to the surrounding code block structure.
Apply this diff:
if ($exCommand -and $exCommand.CommandType -eq 'Application') { $commandPath = $exCommand.Path - } + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
libexec/scoop-shim.ps1(10 hunks)
🔇 Additional comments (1)
libexec/scoop-shim.ps1 (1)
207-220: Good defensive pattern validation.The early regex validation (lines 211-220) prevents runtime errors during pattern matching and provides clear error messages to users.
1 participant
scoop shim add/rmnow remembers manually added shims in the config file.scoop shim listadds a--addedoption to show only manually added shims; output usesFormat-Table.scoop shim infoandscoop shim listdisplay anisAddedfield to indicate if a shim was manually added.scoop shim alternow supports paths to non-scoop-installed executables.Description
Motivation and Context
Relates to #6503
How Has This Been Tested?
Checklist:
developbranch.Summary by CodeRabbit
New Features
Bug Fixes
Chores