Skip to content

[release/v7.4.16] Fix checks for local user config file paths#27453

Closed
adityapatwardhan wants to merge 1 commit into
PowerShell:release/v7.4.16from
adityapatwardhan:backport/release/v7.4.16/26269-c7ee0d294
Closed

[release/v7.4.16] Fix checks for local user config file paths#27453
adityapatwardhan wants to merge 1 commit into
PowerShell:release/v7.4.16from
adityapatwardhan:backport/release/v7.4.16/26269-c7ee0d294

Conversation

@adityapatwardhan
Copy link
Copy Markdown
Member

@adityapatwardhan adityapatwardhan commented May 14, 2026

Backport of #26269 to release/v7.4.16

Triggered by @adityapatwardhan on behalf of @SeeminglyScience

Original CL Label: CL-Engine

/cc @PowerShell/powershell-maintainers

Impact

REQUIRED: Choose either Tooling Impact or Customer Impact (or both). At least one checkbox must be selected.

Tooling Impact

  • Required tooling change
  • Optional tooling change (include reasoning)

Customer Impact

  • Customer reported
  • Found internally

Fixes an issue where configuration file loading could fail when the Documents directory location was not yet created. The fix ensures PowerShell can locate user config files even when their parent directories don't exist yet.

Regression

REQUIRED: Check exactly one box.

  • Yes
  • No

This is not a regression.

Testing

Original PR testing included verification that SYSTEM's default config path is properly resolved regardless of whether the Documents directory exists. Uses DoNotVerify option with Environment.GetFolderPath to ensure configured location is returned.

Risk

REQUIRED: Check exactly one box.

  • High
  • Medium
  • Low

This is a targeted fix for config path resolution that improves robustness by ensuring paths are returned regardless of directory existence. The changes are localized to config file path validation logic with no impact to core functionality.

Copilot AI review requested due to automatic review settings May 14, 2026 20:17
@adityapatwardhan adityapatwardhan added the CL-Engine Indicates that a PR should be marked as an engine change in the Change Log label May 14, 2026
@adityapatwardhan adityapatwardhan force-pushed the backport/release/v7.4.16/26269-c7ee0d294 branch from 27652f2 to 18f3098 Compare May 14, 2026 20:21
@adityapatwardhan adityapatwardhan mentioned this pull request May 14, 2026
Open
9 tasks
Copilot AI reviewed May 14, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR backports path-resolution hardening for user-local PowerShell configuration/cache/profile locations so missing or unverified special-folder directories do not prevent startup/config loading.

Changes:

  • Uses DoNotVerify when deriving Windows special-folder paths and adds safe cache/config path derivation helpers.
  • Adds guards for empty config/cache/profile/transcript paths.
  • Updates cache consumers, telemetry UUID storage, update notifications, and profile optimization to tolerate unavailable local paths.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
src/System.Management.Automation/CoreCLR/CorePsPlatform.cs Safely derives Windows cache/config paths and special-folder lookups.
src/System.Management.Automation/engine/PSConfiguration.cs Skips current-user config file reads when no user config path is available.
src/System.Management.Automation/engine/Modules/AnalysisCache.cs Avoids serializing module analysis cache when no cache path is available.
src/System.Management.Automation/engine/hostifaces/MshHostUserInterface.cs Handles empty transcript base paths.
src/System.Management.Automation/engine/hostifaces/HostUtilities.cs Handles empty profile base paths for current-user and all-user profiles.
src/System.Management.Automation/engine/CommandDiscovery.cs Uses unverified user-profile folder lookup for ~ path expansion.
src/Microsoft.PowerShell.ConsoleHost/host/msh/UpdatesNotification.cs Disables update cache usage when no cache path can be derived.
src/Microsoft.PowerShell.ConsoleHost/host/msh/ConsoleHost.cs Skips profile optimization cache setup when cache directory is unavailable.
src/System.Management.Automation/utils/Telemetry.cs Moves telemetry UUID path derivation to the shared cache helper and updates telemetry module metadata.

Comment thread src/System.Management.Automation/utils/Telemetry.cs
Comment on lines +155 to +156
private static readonly HashSet<string> s_knownSubsystemNames;

daxian-dbw
daxian-dbw requested changes May 14, 2026
View reviewed changes
Copy link
Copy Markdown
Member

@daxian-dbw daxian-dbw left a comment

Choose a reason for hiding this comment

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

CIs are failing. It seems the build failed.

@microsoft-github-policy-service microsoft-github-policy-service Bot added the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label May 14, 2026
@skipp-dev
Copy link
Copy Markdown

skipp-dev commented May 15, 2026

I checked this against the original PR #26269 and I think this backport picked up an intermediate Telemetry.cs state instead of the final merged one.

In 18f3098, ApplicationInsightsTelemetry appears to have an extra closing brace immediately after the if (!CanSendTelemetry) { return; } block. If that's really the file content on release/v7.4.16, it would explain the cross-platform build failures because the static constructor body is malformed.

Separately, s_knownSubsystemNames also looks suspicious on this branch: in the backport it is declared and initialized, but I couldn't find a corresponding usage in the file. So even after fixing the brace, that field may still fail due to being unused.

The original PR #26269 had a follow-up commit 36fc0a2 (Fix cherry pick mistake), which appears to clean this up. My guess is this backport needs either:

  • a fresh cherry-pick from the final merged state of #26269, or
  • an equivalent follow-up to 36fc0a2.

So my read is that the CI failures are very likely caused by an incomplete/incorrect backport of Telemetry.cs, not by the config-path logic itself.

@skipp-dev
Copy link
Copy Markdown

skipp-dev commented May 15, 2026

Follow-up: if the goal is the minimal safe fix for release/v7.4.16, I think the concrete options are:

  1. Cherry-pick the original follow-up fix 36fc0a2 (Fix cherry pick mistake), or
  2. Manually reduce the Telemetry.cs changes to only the UUID/cache-path pieces needed for this backport.

Concretely, I'd expect the branch to need:

  • removal of the extra } after if (!CanSendTelemetry) { return; }
  • keeping s_uuidPath / Platform.TryDeriveFromCache("telemetry.uuid", out s_uuidPath)
  • keeping the GetUniqueIdentifier() call-site changes that use s_uuidPath
  • either fully backporting the s_knownSubsystemNames consumer logic, or removing s_knownSubsystemNames entirely on this branch

That should separate the actual config-path fix from the accidental Telemetry.cs drift.

@skipp-dev
Copy link
Copy Markdown

skipp-dev commented May 15, 2026 •
edited by unfurl-links Bot
Loading

I put together a minimal fix branch for the Telemetry.cs backport drift here:

What it changes:

  • removes the stray extra } after the if (!CanSendTelemetry) { return; } path
  • keeps the intended s_uuidPath / Platform.TryDeriveFromCache("telemetry.uuid", out s_uuidPath) backport
  • keeps GetUniqueIdentifier() using s_uuidPath
  • removes s_knownSubsystemNames and its initializer on this branch
  • reverts unrelated Telemetry allowlist drift that came along with the bad cherry-pick (AIShell, CompletionPredictor, Microsoft.PowerShell.ConsoleGuiTools, Microsoft.PowerShell.PSAdapter, WinGetCommandNotFound)

So this reduces Telemetry.cs back to the release/v7.4.16 shape plus only the UUID/cache-path behavior needed for this PR.

I couldn't run a full local dotnet build on this machine because there is no .NET SDK installed here, but the file diagnostics are clean and this should address the exact CS1519/CS1022 constructor breakage showing up in CI.

GitHub
PowerShell for every system! Contribute to skipp-dev/PowerShell development by creating an account on GitHub.

@adityapatwardhan
Copy link
Copy Markdown
Member Author

adityapatwardhan commented May 15, 2026

Going to do a manual backport instead.

@microsoft-github-policy-service microsoft-github-policy-service Bot removed the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label May 15, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

@daxian-dbw daxian-dbw daxian-dbw requested changes

@andyleejordan andyleejordan andyleejordan approved these changes

Assignees

No one assigned

Labels

CL-Engine Indicates that a PR should be marked as an engine change in the Change Log

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@adityapatwardhan @skipp-dev @daxian-dbw @andyleejordan @SeeminglyScience