Repository navigation
Fix startup crash when formatting localized update notifications - #28236
wait4x (wait4xx) wants to merge 3 commits into
Conversation
The first line of the notification template was assumed to be the longest, but several localized templates have a longer second or third line. The computed padding then becomes negative, and PadRight throws during console host startup, terminating the process before the user gets a prompt. Clamp both padding widths to zero so a cosmetic alignment mismatch cannot crash the host. The existing layout is unchanged for locales where the first line is the longest.
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
🟢 Approval recommended
Both guards directly prevent the reported exception without changing valid padding calculations.
No open issues 🎉
What changed in this PR
Prevents the localized update-notification startup crash reported in #28131 while preserving existing valid padding behavior.
Changes:
- Clamps both padding widths to zero or greater.
- Updates the comment to explain localized line-length differences.
| File | Description |
|---|---|
| src/Microsoft.PowerShell.ConsoleHost/host/msh/UpdatesNotification.cs | Guards both padding calculations against negative widths. |
Review effort: Balanced
|
Independent reproduction on 7.6.6 stable, with the exact zh-Hans numbers:
The line-2 expression is positive for zh-Hans, but the other locale reports show it can also go negative, so clamping both expressions in this PR is the right minimal fix. The template-length assumption is the real bug; console width is not involved. |
Extract the message formatting in ShowUpdateNotification into an internal FormatUpdateNotification method so it can be tested without an update file in the user cache directory. The formatting logic is unchanged. The new xUnit tests cover: - Neutral templates keep all three lines at the same width. - Templates whose second or third line is longer than the first line. - Every shipped localized template. Without the padding clamp, 27 of the 39 localized templates throw ArgumentOutOfRangeException from PadRight.
|
wait4x (@wait4xx) Thanks for the fix! I prepared some additional test coverage for the update notification padding issue in my fork: https://github.com/boylin0/PowerShell/tree/fix/update-notification-padding BTW, besides adding the tests, the commit also moves the message formatting in Feel free to take a look! If you find these tests useful, you can Thanks!
|
|
Thanks BOYI (@boylin0)! Since your commit is based directly on the PR head, I fast-forwarded the branch to I also double-checked that the test assembly ( |
@microsoft-github-policy-service agree |
The CodeFactor check requires the file name to match the first type name and static readonly fields to be PascalCase without prefixes. - Rename test_UpdatesNotification.cs to UpdatesNotificationTests.cs to match the test class. - Rename s_templateNames/s_localizedCultures to TemplateNames/LocalizedCultures.
bd2cc22 to
160068a
Compare
PR Summary
Fixes #28131.
UpdatesNotification.ShowUpdateNotificationassumes the first line of the localized notification template is the longest and derives the trailing padding from it. Several templates shipped in PowerShell 7.6.6 have a longer URL line, so the computed width goes negative andstring.PadRightthrows duringConsoleHost.Start. The exception is unhandled, sopwshterminates before the prompt.This clamps both padding widths to zero. The change is intentionally minimal: locales whose first line is the longest keep the current layout, and the remaining mismatch becomes cosmetic instead of a startup crash.
A follow-up commit by BOYI (@boylin0) extracts the message formatting into an internal
FormatUpdateNotificationmethod — the formatting logic is unchanged — so it can be unit tested without an update file in the user cache directory, and adds xUnit tests covering the neutral templates, templates whose second or third line is longer than the first, and every shipped localized template.PR Context
POWERSHELL_UPDATECHECK=Offavoids the crash but also disables notifications;LTSdoes not help because several cultures also go negative with the LTS template.release/v7.6.Validation
Extracted the three notification templates from all 13 satellite assemblies shipped with 7.6.6 and evaluated both paddings with release tag
7.6.7:The zh-Hans stable/preview templates reproduce the reported
-17(38 - 58 + 3); with the clamp the value is 0.The added
test_UpdatesNotification.csruns the same calculation throughFormatUpdateNotificationfor the neutral and all localized templates; without the clamp 27 of the 39 localized templates throwArgumentOutOfRangeExceptionfromPadRight.A full local build was not possible in this environment (no .NET SDK installed). The fix itself only wraps two existing integer expressions in
Math.Max, and the follow-up commit is a mechanical extraction plus tests; CI will compile and run them.PR Checklist
.h,.cpp,.cs,.ps1and.psm1files have the correct copyright header