Skip to content

fix install shim target resolution#6684

Open
marcelcodes wants to merge 1 commit into
ScoopInstaller:masterfrom
marcelcodes:codex/fix-install-shim-target-resolution
Open

fix install shim target resolution#6684
marcelcodes wants to merge 1 commit into
ScoopInstaller:masterfrom
marcelcodes:codex/fix-install-shim-target-resolution

Conversation

@marcelcodes

@marcelcodes marcelcodes commented Jul 6, 2026

Copy link
Copy Markdown

Summary

This fixes shim creation during install when the newly linked current directory does not expose the target binary at the moment create_shims resolves it.

Root cause

install_app links the version directory to current and then calls create_shims with only the current path. If Test-Path "$dir\$target" does not see the binary, the fallback calls Get-Command $target, which emits a command-not-found error and can leave the app in a failed install state even though the downloaded binary exists in the version directory.

Changes

  • Pass the original version directory into create_shims.
  • Resolve shim targets by checking current, then the original version directory, then existing absolute/relative/PATH targets.
  • Use Get-Command -ErrorAction Ignore for the PATH fallback so missing app-local binaries do not leak a raw command-not-found error.
  • Add regression coverage for original-directory fallback and silent missing-target handling.

Validation

  • Ran the targeted Pester file with Pester 5.7.1: Tests Passed: 10, Failed: 0.
  • Reinstalled scoop-search successfully with scoop uninstall scoop-search; scoop install scoop-search --no-update-scoop.
  • Verified Get-Command scoop-search resolves to the shim and scoop-search git returns results.
  • Ran git diff --check -- lib/install.ps1 test/Scoop-Install.Tests.ps1.

Open in Stage

@stage-review

stage-review Bot commented Jul 6, 2026

Copy link
Copy Markdown

Ready to review this PR? Stage has broken it down into 3 individual chapters for you:

Title
1 Introduce robust shim target resolution logic
2 Pass original version directory to shim creator
3 Verify shim resolution and error handling
Open in Stage

Chapters generated by Stage for commit 68f8bd0 on Jul 6, 2026 10:11pm UTC.

@marcelcodes
marcelcodes marked this pull request as ready for review July 6, 2026 22:31
@coderabbitai

coderabbitai Bot commented Jul 6, 2026

Copy link
Copy Markdown

Review Change Stack

Summary by CodeRabbit

  • Bug Fixes
    • Improved app launch shim resolution so commands can still work when the active linked folder does not expose the binary directly.
    • Added fallback handling that checks the original installation location before using a general command lookup, reducing missing-command issues.
  • Tests
    • Added coverage for shim resolution behavior, including fallback to the original directory and error-free handling when a target cannot be found.

Walkthrough

The PR adds a resolve_shim_target helper function in lib/install.ps1 that centralizes shim target resolution, searching rooted/relative candidates in current and original directories before falling back to Get-Command. create_shims and install_app are updated to pass and use original_dir. Tests are added.

Changes

Shim Target Resolution

Layer / File(s) Summary
Resolver implementation and wiring
lib/install.ps1
Adds resolve_shim_target($dir, $target, $original_dir = $null) which searches candidate paths and falls back to Get-Command; updates create_shims signature to accept original_dir and use the resolver instead of inline Test-Path/Get-Command logic; install_app now passes $original_dir to create_shims.
Resolver test suite
test/Scoop-Install.Tests.ps1
Adds a Windows Describe 'resolve_shim_target' block with setup/teardown, verifying fallback to the original version directory and safe handling (no $Error population) when a target is missing.

Sequence Diagram(s)

sequenceDiagram
  participant install_app
  participant create_shims
  participant resolve_shim_target
  participant GetCommand as Get-Command

  install_app->>create_shims: call with original_dir
  create_shims->>resolve_shim_target: resolve_shim_target(dir, target, original_dir)
  resolve_shim_target->>resolve_shim_target: check rooted/relative candidates in dir and original_dir
  alt candidate found
    resolve_shim_target-->>create_shims: return matched leaf path
  else no candidate found
    resolve_shim_target->>GetCommand: Get-Command target -CommandType Application
    GetCommand-->>resolve_shim_target: Source path
    resolve_shim_target-->>create_shims: return Source
  end
Loading

Estimated code review effort: 2 (out of 5)

Related issues: None found.

Related PRs: None found.

Suggested labels: enhancement

Suggested reviewers: None identified.

🐰

A shim once lost, now finds its way,
Through original dirs it hops all day,
Get-Command waits if all else fails,
Tests confirm the fallback trails,
Hop, resolve, and land just right! 🥕
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the install shim target resolution fix.
Description check ✅ Passed The description matches the change and explains the bug, fix, and validation.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
lib/install.ps1 (1)

180-205: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Optional: consider testing/covering priority ordering (current wins over original).

The resolver correctly checks $dir before $original_dir, but there's no test asserting this precedence when a target exists in both locations. Not blocking, but would strengthen regression coverage for the exact bug this PR fixes.


ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 0d1e380f-ec64-4065-8d02-ad7b3bc04e84

📥 Commits

Reviewing files that changed from the base of the PR and between b588a06 and 68f8bd0.

📒 Files selected for processing (2)
  • lib/install.ps1
  • test/Scoop-Install.Tests.ps1

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.

1 participant