Skip to content

fix(scripts): resolve repo root two levels up in Install-PSModules fallback - #2698

Open
Max Zeier (zeier) wants to merge 5 commits into
microsoft:mainfrom
zeier:fix/install-psmodules-config-path
Open

fix(scripts): resolve repo root two levels up in Install-PSModules fallback#2698
Max Zeier (zeier) wants to merge 5 commits into
microsoft:mainfrom
zeier:fix/install-psmodules-config-path

Conversation

@zeier

Copy link
Copy Markdown
Contributor

Pull Request

Description

Resolve-ConfigPath in scripts/security/Install-PSModules.ps1 strips only one path segment from $PSScriptRoot when git rev-parse --show-toplevel returns nothing. Because the script sits two levels below the repository root (<repo>/scripts/security), the fallback resolves to <repo>/scripts and the subsequent Join-Path produces a duplicated segment:

<repo>/scripts/scripts/security/ps-module-versions.json

The script then aborts with Config file not found.

This PR strips two segments instead, matching the sibling script scripts/security/Test-PSModulePins.ps1, which already resolves the same manifest correctly:

# scripts/security/Test-PSModulePins.ps1 (unchanged, already correct)
$repoRoot = Split-Path (Split-Path $PSScriptRoot -Parent) -Parent

The two scripts previously disagreed on how to locate the same file. They now agree.

The fallback is only reached when git rev-parse --show-toplevel yields no output — for example a linked git worktree whose main .git directory is not reachable from the current mount, git missing from PATH, a non-clone checkout, or git refusing to operate due to detected dubious ownership. In environments where git works normally the fallback is dead code, which is why this went unnoticed.

Related Issue(s)

Closes #2697

Type of Change

Select all that apply:

Code & Documentation:

  • Bug fix (non-breaking change fixing an issue)
  • New feature (non-breaking change adding functionality)
  • Breaking change (fix or feature causing existing functionality to change)
  • Documentation update

Other:

  • Script/automation (.ps1, .sh, .py)

Testing

Added two regression tests in scripts/tests/security/Install-PSModules.Tests.ps1 under a new context that mocks git to return nothing, forcing the fallback branch:

  • Does not duplicate the scripts path segment — asserts the result does not match scripts[\\/]scripts
  • Resolves to the existing manifest at the repository root — asserts the resolved path actually exists

Both tests were verified to fail before the fix and pass after it.

The pre-existing coverage could not catch this defect for two reasons, both addressed by the new context:

  1. The existing assertion is Should -BeLike '*ps-module-versions.json', which the broken path <repo>/scripts/scripts/security/ps-module-versions.json also satisfies.
  2. The suite runs inside a normal clone where git rev-parse succeeds, so the fallback branch was never executed.

Results:

  • npm run test:ps -- -TestPath "scripts/tests/security/Install-PSModules.Tests.ps1" — 26/26 passed (24 pre-existing plus 2 new)
  • npm run test:ps -- -TestPath "scripts/tests/security/" — 604/604 passed, 16 not run
  • Invoke-ScriptAnalyzer with scripts/linting/PSScriptAnalyzer.psd1 on both changed files — no findings
  • npm run validate:local — PSScriptAnalyzer reported 0 errors and 0 warnings across 276 files

Checklist

Required Checks

  • Documentation is updated (if applicable) — not applicable, no behavioral or interface change
  • Files follow existing naming conventions
  • Changes are backwards compatible (if applicable)
  • Tests added for new functionality (if applicable)

Required Local Checks

  • Local validation aggregate: npm run validate:local — see note below
  • Documentation validation (if docs changed): npm run validate:docs — not applicable, no docs changed
  • Spell checking: npm run spell-check — run as part of validate:local
  • Link validation: npm run lint:md-links — see note below

Note on validate:local: the aggregate exits non-zero in my environment solely because markdown-link-check reports 3 broken links out of 4134 checked, all of which are the same external URL https://eur-lex.europa.eu/eli/reg/2024/1689 returning HTTP 202, in .github/instructions/rai-planning/rai-license-posture.instructions.md, .github/skills/rai/rai-standards/references/eu-ai-act.md, and .github/skills/rai/rai-standards/SKILL.md. This PR changes no markdown, so these are pre-existing and unrelated. All other validators in the aggregate passed.

Security Considerations

  • This PR does not contain any sensitive or NDA information
  • Any new dependencies have been reviewed for security issues — no dependencies added
  • Security-related scripts follow the principle of least privilege

Additional Notes

The change is one line in Resolve-ConfigPath plus a test context. No public interface, parameter, or documented behavior changes; the -ConfigPath parameter and PS_MODULE_CONFIG_PATH environment variable continue to take precedence over the fallback exactly as before.

Install-PSModules.ps1 is invoked by .devcontainer/scripts/on-create.sh, .github/actions/setup-ps-modules/action.yml, and .github/workflows/copilot-setup-steps.yml. CI is unaffected in practice because git rev-parse succeeds there; the user-visible impact is in dev container creation, where the failure is fatal to onCreateCommand and causes container creation to abort.

…llback

- Strip two path segments from PSScriptRoot, matching Test-PSModulePins.ps1
- Prevents duplicated scripts/scripts segment when git rev-parse is unavailable
- Add regression tests that force the fallback and assert the resolved path

Closes microsoft#2697
@zeier
Max Zeier (zeier) requested a review from a team as a code owner August 13, 2026 14:46
@codecov-commenter

Codecov Comments Bot (codecov-commenter) commented Aug 13, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 81.36%. Comparing base (5b2119e) to head (979ad45).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2698      +/-   ##
==========================================
- Coverage   83.17%   81.36%   -1.81%     
==========================================
  Files         180      150      -30     
  Lines       32201    21745   -10456     
  Branches       25        0      -25     
==========================================
- Hits        26782    17693    -9089     
+ Misses       5416     4052    -1364     
+ Partials        3        0       -3     
Flag Coverage Δ
docusaurus ?
pester 83.30% <100.00%> (-0.91%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
scripts/security/Install-PSModules.ps1 96.42% <100.00%> (ø)

... and 50 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thank you for this one!

@WilliamBerryiii

Copy link
Copy Markdown
Member

Max Schoening (@max) Zeier (zeier) - you may want to enable the permissions for us to edit your forks so that we can keep the branch up to date and ensure it stays in a state that can be merged.

@zeier

Copy link
Copy Markdown
Contributor Author

Katrien De Graeve (@katriendg), thank you again for the review and approval. I don't have merge permissions, could you please merge the PR for me?

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.

fix: Install-PSModules resolves wrong repo root when git rev-parse is unavailable

4 participants