Repository: microsoft/terminal (language: cpp)

Issue:
We broke some legacy apps that said "black background" but really wanted "default background" 
Just go read #6810 cause that has all the details.


Discussion / hints:
Thanks for handling this for me.
🎂

Did the updated PSReadLine make it into Windows Server 2022 Preview? If not, then the support range for keeping the hack becomes 5-10 years (Whatever the LTSC rules are now) from late 2021, and we might want to consider introducing a config option for users who _do_ have a newer PSReadLine deployed.

Otherwise, the support-range would depend on the SAC expiry cycle, once a Windows SAC release gets the update.

I'm assuming the feature-updates didn't get it; my Windows PowerShell reports 'PSReadLine 2.0.0' on Windows 10 20H2, but I might have locally upgraded that to 2.0.0 GA and then forgotten about it.

https://github.com/PowerShell/PSReadLine still says

> Windows PowerShell on the latest Windows 10 has version 2.0.0-beta2 of PSReadLine.

which predates the fix landing in [2.1.0-rc1](https://github.com/PowerShell/PSReadLine/releases/tag/v2.1.0-rc1).
That was an excellent question. I did some digging -- they *backported* this change into the version of PSReadline in the OS (without changing the version number! tsk!) as of Windows builds 20212+ and 20182+. If the Server previews postdate those build numbers, they should not need the workaround and we won't have to support it  through LTSC :smile:
Windows Server 2022 Preview is build 20348, so that's great, and Windows 11 Insider Preview is build 22000, so the only continuing issue is Windows 10 21H2, which was [just announced as the third servicing-update release in a row](https://blogs.windows.com/windowsexperience/2021/07/15/introducing-the-next-feature-update-to-windows-10-21h2/) (i.e. build 19044), and which has a 5-year LTSC version scheduled, possibly the last Windows 10 LTSC version.

As a passing thought, what if the hack was modified to only be enabled on Windows builds lower than the version numbers you mentioned? That doesn't seem _too_ intrusive, and provides a nice bit of future-proofing.

I'm now mildly concerned that Windows 10 will never get a new full-system update, which means this bug persists for Windows 10 users until support expiry in October 2025. Would a PR to add a config option (per-profile perhaps, so trivial to apply only to PowerShell Core for example) to override the hack be acceptable? (This is _not_ a commitment to create such a PR, of course, I'm just thinking out loud.)

Huh, and while poking around, I noticed that [PowerShell 6.x fell out of support last September](https://docs.microsoft.com/en-au/lifecycle/products/powershell), and [PowerShell 7.0 expires at the end of 2022](https://docs.microsoft.com/en-au/powershell/scripting/powershell-support-lifecycle?view=powershell-7.1), so perhaps the `pwsh.exe` part of this has a shorter lifetime than the `powershell.exe` part. Powershell 7.1 included PSReadLine 2.1.0, so new releases won't move these dates.

In the meantime, I'm weighing up whether I just locally work around this by switching from hand-installed PowerShell 7 MSI to using the .NET Global Tool version of PowerShell. I checked, and it looks right, but I'm not sure yet if it actually has any differences I need to care about.
which is the requirement for reverting the shim again?

Fix patch:
diff --git a/src/server/ConsoleShimPolicy.cpp b/src/server/ConsoleShimPolicy.cpp
index b4f0870ccf1..7f6cc56d690 100644
--- a/src/server/ConsoleShimPolicy.cpp
+++ b/src/server/ConsoleShimPolicy.cpp
@@ -9,51 +9,44 @@
 // - Constructs a new instance of the shim policy class.
 // Arguments:
 // - All arguments specify a true/false status to a policy that could be applied to a console client app.
-ConsoleShimPolicy::ConsoleShimPolicy(const bool isCmd,
-                                     const bool isPowershell) :
-    _isCmd{ isCmd },
-    _isPowershell{ isPowershell }
-{
-}
-
-// Routine Description:
-// - Opens the process token for the given handle and resolves the process name.
-//   We'll initialize the new ConsoleShimPolicy based on whether the client
-//   process is "cmd.exe" or "powershell.exe".
-// - For more info, see GH#3126
-// Arguments:
-// - hProcess - Handle to a connected process
-// Return Value:
-// - ConsoleShimPolicy object containing resolved shim policy data.
-ConsoleShimPolicy ConsoleShimPolicy::s_CreateInstance(const HANDLE hProcess)
+ConsoleShimPolicy::ConsoleShimPolicy(const HANDLE hProcess)
 {
     // If we cannot determine the exe name, then we're probably not cmd or powershell.
-    auto isCmd = false;
-    auto isPowershell = false;
-
     try
     {
         const std::filesystem::path processName = wil::GetModuleFileNameExW<std::wstring>(hProcess, nullptr);
-        auto clientName = processName.filename().wstring();
-        // For whatever reason, wil::GetModuleFileNameExW leaves trailing nulls, so get rid of them.
-        clientName.erase(std::find(clientName.begin(), clientName.end(), '\0'), clientName.end());
-
-        // Convert to lower case, just in case
-        std::transform(clientName.begin(), clientName.end(), clientName.begin(), std::towlower);
+        const auto clientName = processName.filename().native();
 
-        isCmd = clientName.compare(L"cmd.exe") == 0;
+        _isCmd = til::equals_insensitive_ascii(clientName, L"cmd.exe");
 
-        // For powershell, we need both Windows PowersShell (powershell.exe) and
-        // PowerShell Core (pwsh.exe). If PowerShell Core is ever updated to use
+        // For powershell, we need both Windows Powershell (powershell.exe) and
+        // Powershell Core (pwsh.exe). If Powershell Core is ever updated to use
         // ^[[3J for Clear-Host, then it won't ever hit the shim code path, but
         // we're keeping this for the long tail of pwsh versions that still
         // _don't_ use that sequence.
-        isPowershell = (clientName.compare(L"powershell.exe") == 0) ||
-                       (clientName.compare(L"pwsh.exe") == 0);
+        const auto isInboxPowershell = til::equals_insensitive_ascii(clientName, L"powershell.exe");
+        const auto isPwsh = til::equals_insensitive_ascii(clientName, L"pwsh.exe");
+        _isPowershell = isInboxPowershell || isPwsh;
+
+        // Inside Windows, we are guaranteed that we're building alongside a new (good) inbox Powershell.
+        // Therefore, we can default _requiresVtColorQuirk to false.
+#ifndef __INSIDE_WINDOWS
+        // Outside of Windows, we need to check the OS version: Powershell was fixed in early Iron builds.
+        static auto doesInboxPowershellVersionRequireQuirk = [] {
+            OSVERSIONINFOEXW osver{};
+            osver.dwOSVersionInfoSize = sizeof(osver);
+            osver.dwBuildNumber = 20348; // Windows Server 2022 RTM
+
+            DWORDLONG dwlConditionMask = 0;
+            VER_SET_CONDITION(dwlConditionMask, VER_BUILDNUMBER, VER_LESS);
+
+            return VerifyVersionInfoW(&osver, VER_BUILDNUMBER, dwlConditionMask) != FALSE;
+        }();
+        _requiresVtColorQuirk = isInboxPowershell && doesInboxPowershellVersionRequireQuirk;
+        // All modern versions of pwsh.exe have been fixed, and we can direct users to update.
+#endif
     }
     CATCH_LOG();
-
-    return ConsoleShimPolicy(isCmd, isPowershell);
 }
 
 // Method Description:
@@ -90,6 +83,5 @@ bool ConsoleShimPolicy::IsPowershellExe() const noexcept
 // - True as laid out above.
 bool ConsoleShimPolicy::IsVtColorQuirkRequired() const noexcept
 {
-    // Right now, the only client we're shimming is powershell.
-    return IsPowershellExe();
+    return _requiresVtColorQuirk;
 }
diff --git a/src/server/ConsoleShimPolicy.h b/src/server/ConsoleShimPolicy.h
index e3bd57ad1b8..38c42217100 100644
--- a/src/server/ConsoleShimPolicy.h
+++ b/src/server/ConsoleShimPolicy.h
@@ -21,16 +21,13 @@ Module Name:
 class ConsoleShimPolicy
 {
 public:
-    static ConsoleShimPolicy s_CreateInstance(const HANDLE hProcess);
-
+    ConsoleShimPolicy(const HANDLE hProcess);
     bool IsCmdExe() const noexcept;
     bool IsPowershellExe() const noexcept;
     bool IsVtColorQuirkRequired() const noexcept;
 
 private:
-    ConsoleShimPolicy(const bool isCmd,
-                      const bool isPowershell);
-
-    const bool _isCmd;
-    const bool _isPowershell;
+    bool _isCmd{ false };
+    bool _isPowershell{ false };
+    bool _requiresVtColorQuirk{ false };
 };
diff --git a/src/server/ProcessHandle.cpp b/src/server/ProcessHandle.cpp
index 7c2adfc6417..b9cf2f1b359 100644
--- a/src/server/ProcessHandle.cpp
+++ b/src/server/ProcessHandle.cpp
@@ -27,7 +27,7 @@ ConsoleProcessHandle::ConsoleProcessHandle(const DWORD dwProcessId,
                                                  FALSE,
                                                  dwProcessId))),
     _policy(ConsoleProcessPolicy::s_CreateInstance(_hProcess.get())),
-    _shimPolicy(ConsoleShimPolicy::s_CreateInstance(_hProcess.get())),
+    _shimPolicy(_hProcess.get()),
     _processCreationTime(0)
 {
     if (nullptr != _hProcess.get())


Test patch: