Skip to content

Add Control Panel Revival mod - #5219

Open
AdmXP8 wants to merge 29 commits into
ramensoftware:mainfrom
AdmXP8:main
Open

Add Control Panel Revival mod #5219
AdmXP8 wants to merge 29 commits into
ramensoftware:mainfrom
AdmXP8:main

Conversation

@AdmXP8

@AdmXP8 AdmXP8 commented Aug 26, 2026

Copy link
Copy Markdown

This mod restores legacy Control Panel applets in Windows 11 by bypassing the modern Settings app redirection. It requires ExplorerPatcher to function properly.

Changelog

If this pull request updates an existing mod, describe the changes below:

  • Changelog item 1...
  • Changelog item 2...

Mod authorship

If this pull request introduces a new mod, please complete the section below.

This mod was created by:

    • The submitter, without AI assistance
    • The submitter, with AI assistance
    • Claude
    • ChatGPT
    • Gemini
    • Another AI (please specify):
    • Other (please specify):

Please select the options that best apply. Your selection does not affect the acceptance criteria, but it helps reviewers understand the context of the code and provide relevant feedback.

This mod restores legacy Control Panel applets in Windows 11 by bypassing the modern Settings app redirection. It requires ExplorerPatcher to function properly.
@windhawk-reviewer windhawk-reviewer Bot added the waiting-for-author The author's turn: request an AI review, or respond to one that was posted. label Aug 26, 2026
@windhawk-reviewer

Copy link
Copy Markdown

Thanks for the pull request! This repository uses a two-stage review: an AI review that you run yourself, followed by a human review.

To get started, comment /ai-review. Once you're happy with the result, comment /ready-for-reviewer to hand it over to a human reviewer.

See the pull request review process for the full details.

@AdmXP8

AdmXP8 commented Aug 26, 2026

Copy link
Copy Markdown
Author

/ai-review

@windhawk-reviewer

Copy link
Copy Markdown

@AdmXP8 /ai-review can't be applied here: the AI review only handles a pull request that adds or updates a single mod file under mods/, but control-panel-revival.wh.cpp isn't a mod file.

Comment /ready-for-reviewer to hand this pull request over to a human reviewer directly. See the pull request review process for details.

@AdmXP8

AdmXP8 commented Aug 26, 2026

Copy link
Copy Markdown
Author

/ready-for-reviewer

@windhawk-reviewer windhawk-reviewer Bot added waiting-for-reviewer Ready for a human reviewer, and in the queue for one. and removed waiting-for-author The author's turn: request an AI review, or respond to one that was posted. labels Aug 26, 2026
@windhawk-reviewer windhawk-reviewer Bot added waiting-for-author The author's turn: request an AI review, or respond to one that was posted. and removed waiting-for-reviewer Ready for a human reviewer, and in the queue for one. labels Aug 26, 2026
@windhawk-reviewer

Copy link
Copy Markdown

New commits were pushed, so this pull request left the human review queue and is back to waiting-for-author.

Comment /ai-review to get an AI review of the updated code, then /ready-for-reviewer to hand it over to a human reviewer again. See the pull request review process for details.

AdmXP8 added 2 commits August 26, 2026 12:28
Implement Control Panel Revival mod to restore legacy Control Panel applets in Windows 11.
@AdmXP8

AdmXP8 commented Aug 26, 2026

Copy link
Copy Markdown
Author

/ai-review

@windhawk-reviewer windhawk-reviewer Bot added waiting-for-ai-review An AI review was requested and is being prepared. and removed waiting-for-author The author's turn: request an AI review, or respond to one that was posted. labels Aug 26, 2026
@windhawk-reviewer

Copy link
Copy Markdown

Submission review

Note: This review was done by Claude. Due to the amount of submissions, doing a fully manual review for each pull request is no longer feasible. Thank you for understanding.

Remember: The AI reviewer can be wrong - it may misread code, flag correct code as broken, or suggest changes that make things worse. Treat its findings as suggestions to verify, not instructions to follow blindly. You're responsible for the code you submit, so if a finding doesn't hold up, say so instead of changing working code to satisfy it.

Please address the following issues. The items in the collapsed sections are optional, so it's your call whether to address them.


The core mechanism here — blindly scanning shell32.dll's image for GUID strings and zeroing them, plus an unscoped process-wide CompareStringOrdinal hook — is not something that can be merged as-is. Both are irreversible/global in ways Windhawk mods must not be, and there are established mods in the catalog that solve the same problem with reversible hooks. There's also substantial overlap with several existing mods that needs to be addressed before anything else.

1. KillStringInModule permanently corrupts system DLL images and is not reversible. This is the biggest problem. Windhawk's core principle is that a mod's effects disappear when it is disabled — this mod zeroes bytes inside shell32.dll and windows.storage.dll, never saves the original bytes, and has no Wh_ModUninit. Disabling or updating the mod leaves Explorer permanently damaged until the process is restarted. On top of that:

  • The scan covers the entire image (.text, .rdata, resources, everything) at unaligned byte offsets, and the first match anywhere wins. Those GUID strings are also how the shell resolves the namespace items — zeroing shell32's copy of ::{ED834ED6-…} or ::{BD84B380-…} can break Personalization/Fonts rather than restore them.
  • VirtualProtect(mbi.BaseAddress, mbi.RegionSize, PAGE_READWRITE, …) flips the whole containing region — potentially an executable one — to PAGE_READWRITE, dropping execute permission while other Explorer threads are running. Any thread executing in that region during the window takes an access violation.
  • GetModuleInformation and VirtualQuery return values are unchecked. If GetModuleInformation fails, info stays zeroed, so base == 0 and size - patternLen underflows to SIZE_MAX — the loop then reads from address 0 and crashes.
  • Cost: 14 patterns × 2 modules = 28 full linear scans of ~30 MB of image data with a byte-at-a-time inner loop, all synchronously in Wh_ModInit, which runs before Explorer starts executing. That's a measurable startup stall.

The correct approach is to hook the code that consults the hidden/redirected list, not to patch the data it reads. Windows 7 Legacy Applet Restorer and Restore the classic Personalization and other CPLs both do this by hooking the registry read APIs and synthesizing the answers, which is fully reversible.

2. The CompareStringOrdinal hook reads past the end of caller buffers. CompareStringOrdinal takes explicit lengths; cchCount1/cchCount2 are only -1 when the string is null-terminated. For any other value the buffer is a counted, not necessarily terminated substring — and the hook feeds it straight to wcscmp/wcsicmp:

auto pCompFunc = bIgnoreCase ? wcsicmp : wcscmp;
if (0 == pCompFunc(lpString1, g_szAppletsToUnhide[i]) || ...)

This is an out-of-bounds read on every counted-string call in Explorer. Guard on the terminated form, as Taskbar Grouping does:

if (cchCount1 == -1 && cchCount2 == -1) { /* only then treat as C strings */ }

3. The CompareStringOrdinal hook is unscoped, so it changes semantics process-wide. Every single CompareStringOrdinal call in explorer.exe now returns CSTR_LESS_THAN whenever either operand is one of these 28 strings — including calls that legitimately need CSTR_EQUAL. Comparing Microsoft.Display against itself now reports "less than". That will affect Control Panel item lookup and sorting, control.exe /name Microsoft.Personalization, Control Panel search, and anything else in the shell that compares these names — not just the hide check you're targeting. It also adds up to 28 string comparisons to every call of a very hot API.

Scope it to the call you actually want to influence: set a thread-scoped flag around the specific shell32 call, and only apply the override when the flag is set. See Taskbar Grouping for the pattern (g_compareStringOrdinalHookThreadId = GetCurrentThreadId(); around the targeted original call, checked in the hook). Better still, symbol-hook the shell32 function that performs the visibility check directly.

4. Hook CompareStringOrdinal in kernelbase.dll, not the linked kernel32 symbol. Wh_SetFunctionHook((void*)CompareStringOrdinal, …) binds the symbol the mod links against. The real implementation lives in kernelbase.dll, and shell32's calls go through the api-set directly to kernelbase, so the hook may never fire for the callers you care about. Resolve it explicitly, as Taskbar Grouping does:

HMODULE kernelBaseModule = GetModuleHandleW(L"kernelbase.dll");
auto pCompareStringOrdinal = (decltype(&CompareStringOrdinal))GetProcAddress(
    kernelBaseModule, "CompareStringOrdinal");
WindhawkUtils::SetFunctionHook(pCompareStringOrdinal, CompareStringOrdinal_hook,
                               &CompareStringOrdinal_orig);

5. The hook returns an invalid value on failure. CompareStringOrdinal returns 0 on failure (with the error in GetLastError); ERROR_INVALID_PARAMETER is 87, which callers will interpret as garbage rather than as a failure:

if (!lpString1 || !lpString2) return ERROR_INVALID_PARAMETER;   // wrong

Just forward to the original in that case, or SetLastError(ERROR_INVALID_PARAMETER); return 0;.

6. The _MapLegacyName hook fails unconditionally and writes to an out buffer without checking its size.

bool COpenControlPanel__MapLegacyName_hook(void *pThis, LPCWSTR pszLegacyName,
                                           LPWSTR pszNewName, UINT uUnused, bool *nameChanged) {
    if (nameChanged) *nameChanged = false;
    if (pszNewName) *pszNewName = L'\0';
    return false;
}

pszLegacyName is never examined, so every legacy→canonical name mapping in shell32 fails — not only for the applets in your list. That path is used by control.exe /name …, Control Panel search and legacy .cpl invocations, so this is a much wider blast radius than intended. The third parameter (named uUnused) is almost certainly the output buffer's size in characters; writing L'\0' when it is 0 is a buffer overflow. Filter on pszLegacyName against your list and call COpenControlPanel__MapLegacyName_orig for everything else, and guard the write on the size parameter.

7. No Wh_ModUninit, and the LoadLibraryW reference is leaked. hWinStorage = LoadLibraryW(L"windows.storage.dll") is never released, so the module refcount is bumped once per mod load and never comes back down across enable/disable/update cycles. In explorer.exe the DLL is already loaded, so GetModuleHandleW is enough; if you do need to force-load it, use LoadLibraryExW(L"windows.storage.dll", nullptr, LOAD_LIBRARY_SEARCH_SYSTEM32) (windows.storage.dll is not a KnownDLL, so the bare-name form uses the default search order) and FreeLibrary it in Wh_ModUninit.

8. Dead placeholder loop.

for (size_t i = 50; i < ARRAYSIZE(g_szAppletsToUnhide); i++) {
    // Loop placeholder check handled safely below
}

ARRAYSIZE(g_szAppletsToUnhide) is 14, so 50 < 14 is never true and the body is empty anyway. This looks like leftover AI-generated scaffolding — please remove it, and give the rest of the file a pass for anything similar.

9. Substantial overlap with existing mods. The applet list here is largely already covered by mods in the catalog:

The maintainer strongly prefers extending an existing mod (adding an option, or a PR to the original author's repo) over merging a near-duplicate. Please state explicitly in the PR description what this mod does that those don't, and if the delta is thin, consider contributing the difference to one of them instead.

10. The ExplorerPatcher requirement is unexplained and unenforced. The README says "Installing ExplorerPatcher is necessary for this mod to work", but nothing in the code references or depends on ExplorerPatcher. Please explain what specifically breaks without it and why — and ideally make the mod work standalone. A mod that only functions alongside a third-party patcher, with no stated reason, is hard to accept into the catalog.

11. All hook results are unchecked and there is no logging. Wh_SetFunctionHook and WindhawkUtils::HookSymbols return values are discarded, and the mod contains no Wh_Log calls at all, so every failure mode (symbol not resolved, shell32 handle null, KillStringInModule finding no match) is completely silent. Add Wh_Log calls on the failure paths and return FALSE from Wh_ModInit where appropriate.

12. The symbol-hook array declares the wrong module. The comment above it says // explorer.exe, control.exe, but the array is resolved against shell32.dll:

// explorer.exe, control.exe
const WindhawkUtils::SYMBOL_HOOK explorerControlHooks[] = { ... };
...
WindhawkUtils::HookSymbols(hShell32, explorerControlHooks, ARRAYSIZE(explorerControlHooks));

The declaration must name the module the hooks are actually applied to. Rename the array to shell32DllHooks (the naming convention encodes the module) and drop the comment.

13. The README needs a screenshot and more detail. This mod has a clearly visible effect, so a screenshot of the restored applets in Control Panel would help a lot. The README is also currently three lines — it should list which applets are restored, which Windows 11 builds it was tested on, and the ExplorerPatcher rationale from item 10.

Optional improvements

Minor polish — none of this affects users, so it's your call.

  • Unused dependencies: -lcomctl32 and -lshlwapi in @compilerOptions, plus #include <shlwapi.h>, aren't used by anything in the file. Only -lpsapi is needed (for GetModuleInformation).
  • Prefer the type-safe WindhawkUtils::SetFunctionHook() over raw Wh_SetFunctionHook with void* casts — it catches signature mismatches at compile time:
    WindhawkUtils::SetFunctionHook(pCompareStringOrdinal, CompareStringOrdinal_hook,
                                   &CompareStringOrdinal_orig);
  • wcsicmp is a deprecated POSIX-name alias; _wcsicmp is the portable spelling.
  • The string tables could be static constexpr — they're never modified.
  • for (UINT i = 0; i < ARRAYSIZE(...)) compares a UINT against a size_t; use size_t for consistency.
  • Consider adding a settings block with per-applet toggles instead of a hard-coded list, the way restore-classic-cpls does — users rarely want all 14 at once.
  • @description starts with a lowercase letter ("control panel unhider…"); the other mods capitalize it.

Functionality notes

Non-critical observations and ideas about the feature behavior itself.

  • The two tables don't line up. g_szCanonicalNames has entries with no GUID counterpart (Microsoft.LocationAndOtherSensors, Microsoft.LocationSettings, Microsoft.RegionalAndLanguageOptions, Microsoft.Printers, Microsoft.WindowsUpdate) and the GUID list has entries with no canonical counterpart (Family Safety, Text to Speech, Programs and Features, BitLocker, HomeGroup). That asymmetry suggests the two lists were assembled independently rather than derived from the applets you actually verified. Worth pruning to the set you've tested.
  • Some of these applets no longer exist in Windows 11 at all, so unhiding a name won't bring them back. HomeGroup ({67CA7650-…}) was removed in Windows 10 1803 and the classic Display page ({C555438B-…}) in 1703 — the catalog mods that restore them (Win11 HomeGroup Restorer, Windows 7 Display Control Panel Restorer) have to recreate the namespace extension, not just unhide it. Worth confirming which entries in your list actually do something.
  • KillStringInModule returns after the first successful match. If the same GUID appears in more than one table inside the module, only one copy is affected — which may explain inconsistent results between builds.
  • Please confirm the mod behaves the same when enabled mid-session as it does after a reboot / Explorer restart. Patching a string that shell32 has already read and cached won't have any effect until the process restarts, so mid-session behavior can differ from cold-start behavior.


Next steps:

  • /ai-review - after pushing fixes, to get a review of the updated code. You can repeat this as many times as you need, but each review is thorough and usually there's no need for more than 2-3 iterations.
  • /ready-for-reviewer - once you're satisfied with the state of the pull request, to hand it over to a human reviewer. If some findings above are left unaddressed, add a short note explaining why.

See the review process for details.

@windhawk-reviewer windhawk-reviewer Bot added waiting-for-author The author's turn: request an AI review, or respond to one that was posted. and removed waiting-for-ai-review An AI review was requested and is being prepared. labels Aug 26, 2026
AdmXP8 added 2 commits August 26, 2026 13:48
Refactor Control Panel Revival mod to improve hooks and error handling.
@AdmXP8

AdmXP8 commented Aug 26, 2026

Copy link
Copy Markdown
Author

/ai-review

@windhawk-reviewer windhawk-reviewer Bot added waiting-for-ai-review An AI review was requested and is being prepared. and removed waiting-for-author The author's turn: request an AI review, or respond to one that was posted. labels Aug 26, 2026
@windhawk-reviewer

Copy link
Copy Markdown

Submission review

Note: This review was done by Claude. Due to the amount of submissions, doing a fully manual review for each pull request is no longer feasible. Thank you for understanding.

Remember: The AI reviewer can be wrong - it may misread code, flag correct code as broken, or suggest changes that make things worse. Treat its findings as suggestions to verify, not instructions to follow blindly. You're responsible for the code you submit, so if a finding doesn't hold up, say so instead of changing working code to satisfy it.

Please address the following issues. The items in the collapsed sections are optional, so it's your call whether to address them.


The two mechanisms this mod is built on — a process-wide CompareStringOrdinal hook and blind byte-patching of shell32.dll / windows.storage.dll — are both very blunt instruments with a much wider blast radius than the stated goal, and there's substantial overlap with mods already in the catalog. Details below.

1. Hooking CompareStringOrdinal process-wide and lying about the result is unsafe.

CompareStringOrdinal_hook (lines 151-176) replaces kernelbase's general-purpose ordinal string comparison for the entire process — every module in explorer.exe, including every shell extension and every other Windhawk mod. Whenever either operand matches one of the 28 hard-coded strings, it returns CSTR_LESS_THAN regardless of what the other operand is. Two problems follow:

  • Comparing one of those CLSIDs with itself now reports "less than" instead of CSTR_EQUAL. Any unrelated shell code that legitimately compares those strings silently takes the wrong branch — and several of them are the applets' own shell-folder CLSIDs (::{BD84B380-8CA2-1069-AB1D-08000948F534} is the Fonts folder, ::{7B81BE6A-CE2B-4676-A29E-EB907A5126C5} is Programs and Features), so this can break the very applets the mod is trying to restore.
  • The comparator stops being antisymmetric: Compare(A, B) and Compare(B, A) both return CSTR_LESS_THAN when A is a listed string. Anything that uses CompareStringOrdinal as a sort predicate or in a binary search (the shell does, a lot) now has an inconsistent ordering, which yields wrong results at best.

Also worth noting: this runs up to 28 _wcsicmp/wcscmp calls on every null-terminated comparison in a very hot API.

The fix is to hook the specific function that makes the redirection decision, not a generic comparison primitive. The COpenControlPanel::_MapLegacyName hook you already have is exactly the right shape — please find the equivalent targeted hook for the CLSID path instead of intercepting CompareStringOrdinal.

2. KillStringInModuleReversible patches system modules blindly.

The scan (lines 96-129) walks the whole mapped image byte-by-byte and zeroes the first byte-sequence match it finds anywhere in it. There is no section filter, no WCHAR alignment requirement, and no check that the match is a standalone NUL-terminated string. So a match can land at an odd offset, or inside a longer string (a shell:::{...} path, a registry key path, a concatenated table entry), truncating unrelated data. And even when the hit is the intended string, zeroing it kills every use of that string in the module, not just the redirection check — same over-reach as item 1.

On top of that, when the mod is enabled mid-session (Explorer already running), the patch is written underneath live threads that may be reading those bytes, and Wh_ModUninit restores them the same way.

This is the "workaround instead of root cause" pattern the project pushes back on. Please replace the memory patching with a targeted hook on whatever function consults the redirect list — patching data in a system DLL is not something that can be made safe by adding more guards around it.

3. Wh_ModInit failure paths leave the process permanently patched.

The memory patches are applied at lines 201-206, before any hook is registered. Every return FALSE after that (lines 212, 218, 223, 229) leaves shell32.dll / windows.storage.dll with zeroed strings and the windows.storage.dll reference still held — and Wh_ModUninit is not called when Wh_ModInit returns FALSE, so nothing ever reverts it for the life of the process. Windhawk reloads the mod after every settings change, so the leaked module reference also accumulates.

This is not hypothetical: the _MapLegacyName hook is declared required (false in the last SYMBOL_HOOK field, line 184), so on any build where that private symbol isn't present in shell32.dll, init fails after patching. For comparison, settings-to-control-panel marks the same symbol optional.

Minimum fix: register all hooks first and apply the patches last, and revert every applied patch (plus FreeLibrary(g_hWinStorage)) before each return FALSE.

4. Substantial overlap with existing mods.

  • settings-to-control-panel already hooks private: bool __cdecl COpenControlPanel::_MapLegacyName(unsigned short const *,unsigned short *,unsigned int,bool *) in shell32.dll in explorer.exe, with a suppression body that is essentially identical to yours (*nameChanged = false; *pszNewName = L'\0'; return false;). Two mods installing the same hook on the same symbol in the same process is also a direct conflict.
  • win7-legacy-applet-restorer and restore-classic-cpls already restore classic Control Panel applets (Personalization, BitLocker, HomeGroup, Printers, …), and there are per-applet restorers for Windows Update, Display and HomeGroup.

The project's preference is to extend an existing mod (add an option, or open an issue/PR on the original author's repo) rather than merge a parallel mod. Please state explicitly which cases this covers that the mods above don't. If the _MapLegacyName hook was derived from settings-to-control-panel, that should be credited in the README and the licenses kept compatible.

5. "Requires ExplorerPatcher" is a hard external dependency.

The README (line 22) states that installing ExplorerPatcher is necessary for the mod to work, but nothing in the code references or depends on ExplorerPatcher. A Windhawk mod should be self-contained and work on a stock system; requiring a third-party shell-replacement tool that hooks the same shell surfaces is both a design problem and a conflict risk. Please either explain precisely what EP provides here (and make the mod degrade gracefully without it), or drop the requirement from the README.

6. No screenshot in the README.

The mod has a clearly visible effect (Control Panel applets opening instead of Settings). Please add a screenshot or short GIF — only i.imgur.com and raw.githubusercontent.com are allowed image hosts.

Optional improvements

Minor polish — none of this affects users today, so it's your call.

  • Latent irreversible patch when the table fills up. In KillStringInModuleReversible, the ZeroMemory at line 121 runs unconditionally, but the backup at lines 113-118 is skipped once g_appliedPatchCount >= ARRAYSIZE(g_appliedPatches). With 14 strings × 2 modules the cap can't be hit today, but if the lists grow the mod would start writing patches it can never undo. Either bail out when there's no room, or move the ZeroMemory inside the recorded branch.
  • Off-by-one / underflow in the scan loop. for (size_t i = 0; i < size - patternLen; i++) (line 96) misses the last valid offset (<= is correct), and size - patternLen underflows to a huge value if patternLen > size, producing an unbounded read. The patternLen > sizeof(PatchRecord::originalBytes) check above it doesn't cover that case.
  • Unused dependencies. -lcomctl32 and -lshlwapi in @compilerOptions (line 10) and #include <shlwapi.h> (line 28) aren't used anywhere. -lpsapi is the only one that's needed (GetModuleInformation).
  • Dead checks. g_szAppletsToUnhide[i] && / g_szCanonicalNames[i] && (lines 137, 160, 165) test string literals in a static array, so they're always true. Likewise if (!CompareStringOrdinal_orig) return 0; (lines 171-173) can't trigger — Wh_ModInit already returns FALSE if the hook wasn't installed.
  • @architecture. The symbol string uses the x64 __cdecl form, so the hook can only resolve on 64-bit. Since Windows 11 is 64-bit only, declaring @architecture x86-64 (as the sibling Control Panel mods do) makes the target explicit.
  • _wcsicmp is locale-sensitive, unlike CompareStringOrdinal(..., TRUE) which is ordinal. Equivalent for the ASCII strings here, but if you keep any case-insensitive comparison, CompareStringOrdinal_orig(..., TRUE) or _wcsicmp_l with the invariant locale is the exact match.
  • Late module load. Wh_ModInit bails with FALSE if shell32.dll isn't loaded yet (lines 191-195). That's fine for explorer.exe, but if it ever isn't a static import of a target the mod silently never applies. The usual pattern is to hook LoadLibraryExW in kernelbase.dll and apply the module's hooks when it loads — see disable-thumbnail-minimum-size for the LOAD_LIBRARY_SEARCH_SYSTEM32 half, which you already do correctly for windows.storage.dll.

Functionality notes

Non-critical observations and ideas about the feature behavior itself.

  • Verify that _MapLegacyName actually sees the names you match against. g_szCanonicalNames only contains Microsoft.* forms. settings-to-control-panel matches both the short legacy names and the Microsoft.* forms (system and microsoft.system, personalization and microsoft.personalization), which suggests the short form is what actually arrives in at least some paths. Worth logging pszLegacyName for a session and confirming the hook fires at all — if it doesn't, the whole feature is silently carried by the memory patching.
  • Only the first match per module is patched. KillStringInModuleReversible returns after the first hit (line 125). CLSID strings are typically referenced from more than one place in a module, so the remaining copies stay intact — which may explain why the CompareStringOrdinal hook was needed as a second layer.
  • No settings block. The applet and canonical-name lists are hard-coded. Exposing them (or at least per-applet toggles) would let users enable only what they need and disable an entry that misbehaves, without waiting for a mod update. That matters more than usual here, given how wide-reaching each entry is.


Next steps:

  • /ai-review - after pushing fixes, to get a review of the updated code. You can repeat this as many times as you need, but each review is thorough and usually there's no need for more than 2-3 iterations.
  • /ready-for-reviewer - once you're satisfied with the state of the pull request, to hand it over to a human reviewer. If some findings above are left unaddressed, add a short note explaining why.

See the review process for details.

@windhawk-reviewer windhawk-reviewer Bot added waiting-for-author The author's turn: request an AI review, or respond to one that was posted. and removed waiting-for-ai-review An AI review was requested and is being prepared. labels Aug 26, 2026
@windhawk-reviewer windhawk-reviewer Bot added waiting-for-ai-review An AI review was requested and is being prepared. and removed waiting-for-author The author's turn: request an AI review, or respond to one that was posted. labels Aug 29, 2026
@windhawk-reviewer

Copy link
Copy Markdown

Submission review

Note: This review was done by Claude. Due to the amount of submissions, doing a fully manual review for each pull request is no longer feasible. Thank you for understanding.

Remember: The AI reviewer can be wrong - it may misread code, flag correct code as broken, or suggest changes that make things worse. Treat its findings as suggestions to verify, not instructions to follow blindly. You're responsible for the code you submit, so if a finding doesn't hold up, say so instead of changing working code to satisfy it.

Please address the following issues. The items in the collapsed sections are optional, so it's your call whether to address them.


The _MapLegacyName part of this mod is sound and follows an established pattern. The CompareStringOrdinal part is the problem — it's an unscoped process-wide override, and the mod's own README says its actual call site is unknown. That needs to be resolved before this can ship.

1. The unscoped CompareStringOrdinal hook has a process-wide blast radius and must be scoped.

CompareStringOrdinal is one of the most frequently called string primitives in explorer.exe — the shell, third-party shell extensions, and other mods all go through it. The hook lies to every caller in the process whenever the real result was CSTR_EQUAL and either operand matches the list. Three concrete consequences:

  • It breaks antisymmetry. CompareStringOrdinal_hook returns CSTR_LESS_THAN when either string matches, so cmp(A, B) and cmp(B, A) both report "less than" for a targeted string. Any sort, binary search, or ordered container in the process that uses CompareStringOrdinal as its comparator now has an inconsistent comparator — that's silent wrong results at best, and out-of-bounds reads in a sort implementation at worst.
  • The targeted strings are not obscure. Microsoft.System, ::{BB06C0E4-D293-4f75-8A90-CB05B6477EEE}, ::{BD84B380-8CA2-1069-AB1D-08000948F534} etc. appear all over the shell — PIDL parsing-name comparisons, namespace registration lookups, jump lists, search. Forcing "not equal" for them anywhere in Explorer can break Control Panel behavior unrelated to the redirect this mod targets.
  • CustomApplets lets users extend the blast radius arbitrarily. IsPlausibleAppletId (line 157) only checks non-empty and ≤ 128 characters. A user who types Documents or Desktop into the setting makes every equality comparison of that exact string in explorer.exe report "not equal", process-wide, with no way to tell what broke.

The README's stated reason for leaving it unscoped is that return-address scoping failed. That's the right diagnosis of the wrong technique — __builtin_return_address(0) / a stack walk lands in a trampoline or in an inlined helper, so caller-module scoping is unreliable. The idiomatic Windhawk approach is thread-and-window scoping: arm the override on the current thread immediately before you call into the shell code whose behavior you want to change, and disarm it right after. See taskbar-grouping.wh.cpp#L1316-L1345 for the hook and #L1105-L1115 for the arm/disarm:

std::atomic<DWORD> g_compareStringOrdinalHookThreadId;
...
g_compareStringOrdinalHookThreadId = GetCurrentThreadId();
g_compareStringOrdinalAnySuffixEqual = true;
// ... call the shell function whose internal comparison you need to influence ...
g_compareStringOrdinalHookThreadId = 0;
g_compareStringOrdinalAnySuffixEqual = false;

You don't need to know the exact inner call site for this to work — you only need an outer shell32 function you already hook (COpenControlPanel::_MapLegacyName, or whichever IOpenControlPanel::Open/GetPath-level entry point precedes the redirect decision) to arm and disarm around. That reduces the override from "the whole process, forever" to "one thread, for the duration of one call". If no such outer function exists for the path you need, that's a strong signal the redirect is being decided somewhere else, and the right fix is to find and hook that function rather than to override a global comparison primitive.

2. Debug diagnostics are left in the shipped code, and one of them is not free.

The stack-walk block in CompareStringOrdinal_hook (lines 377-399) is marked TEMPORARY DIAGNOSTIC but runs unconditionally on every override — RtlCaptureStackBackTrace, then up to six GetModuleHandleExW + GetModuleFileNameW pairs. Unlike Wh_Log, those are real calls that execute whether or not logging is enabled. They also clobber the thread's last error before the hook returns, which a hook of a kernelbase API shouldn't do. Please remove the block; if you want the data, gate the whole thing behind a build-time #if 0 or drop it once you've collected it. The TEMPORARY DIAGNOSTIC block at lines 259-272 in COpenControlPanel__MapLegacyName_hook should go too — see the next item.

3. The mod ships with its targeting admittedly incomplete.

The comment at lines 259-269 says the bare legacy-name spellings for the six target applets aren't known, that those calls "silently fall through", and instructs the user to read the log and add the missing strings to CustomApplets themselves. That's a debugging session, not a shippable configuration — a mod shouldn't require users to discover its own input strings.

The answer is already in the repo: settings-to-control-panel handles exactly this and its whitelist shows the actual forms Control Panel passes — lowercased bare keywords and canonical names (settings-to-control-panel.wh.cpp#L1768-L1791):

static const std::unordered_set<std::wstring> kNames = {
    L"system",           L"microsoft.system",
    L"sound",            L"microsoft.sound",
    L"backupandrestore", L"microsoft.backupandrestore",
    ...
};

Add the bare forms (troubleshooting, installedupdates, defaultprograms, devicesandprinters, fonts, system) to the built-in list and remove the diagnostic. It's also worth verifying whether the ::{GUID} spellings ever reach _MapLegacyName at all — if they don't, they're dead entries in g_szAppletsToUnhide for that hook (they'd still be live for the CompareStringOrdinal path).

4. Substantial overlap with settings-to-control-panel.

Both mods hook COpenControlPanel::_MapLegacyName in shell32.dll inside explorer.exe, for the same purpose, with the same suppression logic (*nameChanged = false, empty pszNewName, return false). The README argues the difference is which applets are covered — but that difference is a handful of entries in that mod's kNames set. Windhawk's maintainer strongly prefers extending an existing mod over merging a near-duplicate; the natural path here is a PR to settings-to-control-panel adding the missing names to its LegacyNameMappingFix whitelist (and, if the CompareStringOrdinal mechanism turns out to be genuinely necessary and can be properly scoped, adding that as an option there too). If you believe a separate mod is justified, please make the case explicitly — and note that with both mods installed, both hooks sit in the same chain on the same function.

5. The CustomApplets $description contradicts the code.

The setting text says canonical names "must be at least 8 characters and contain a dot, same shape as Microsoft's own names". IsPlausibleAppletId (line 157) no longer enforces any of that — the FIX: comment above it says the rule was deliberately removed. Users following the description will avoid entries that are actually accepted. The README also says "12 built-in" (line 39) while the setting says "6 built-in" — both counts are defensible (6 GUIDs + 6 canonical names), but they should agree. Please make the description describe the current behavior.

Optional improvements

Minor polish — none of this affects users, so it's your call.

  • -lcomctl32 is unused. Nothing in the mod uses comctl32; drop it from @compilerOptions.
  • g_isInitialized is written but never read (lines 96, 554, 576). Dead state — remove it.
  • The try/catch wrappers are unnecessary. None of the wrapped blocks allocate or can throw — CompareStringOrdinal_hook's body is wcslen/_wcsnicmp/iteration over an already-built vector, and the same is true of _MapLegacyName_hook and LoadLibraryExW_hook. The comments justify them as protection against std::bad_alloc escaping into non-C++ frames, but there is no allocation on those paths. Removing them makes the hooks noticeably easier to read.
  • Use the type-safe SetFunctionHook overload. WindhawkUtils::SetFunctionHook is a template that deduces and checks the prototype; casting all three arguments to void*/void** (lines 523-526, 541-544) defeats that. Pass them directly:
    WindhawkUtils::SetFunctionHook(pCompareStringOrdinal, CompareStringOrdinal_hook,
                                   &CompareStringOrdinal_orig);
  • volatile LONG + Interlocked*std::atomic<bool>. g_shell32HookApplied is a single flag; std::atomic<bool> with compare_exchange_strong is clearer and is what other mods use.
  • SafeCompareString mixes two return-value domains. It returns CSTR_LESS_THAN (= 1) for the error cases but wcsncmp-style values otherwise, where 1 means "greater". Callers only test == 0 so it works today, but it's a footgun — have it return bool (matched / didn't match) instead.
  • Hoist the wcslen. COpenControlPanel__MapLegacyName_hook calls wcslen(pszLegacyName) once per list entry (up to 12 + N times per call). Compute it once. Similarly, MatchesTargetList computes the length via GetEffectiveLength and then SafeCompareString recomputes it for every length-matching candidate.
  • MatchesTargetList's bIgnoreCase parameter is unused — drop it rather than commenting it out.
  • The LoadLibraryExW hook is likely never needed and stays installed for the mod's lifetime. explorer.exe imports shell32.dll statically, so the late-load path effectively can't fire there. add-virtual-folders-to-nav-top only installs the loader hook when the target module isn't already present (#L2471-L2485) — doing the same avoids running a hook on every LoadLibraryExW call in Explorer for no benefit.
  • Comments document the development history rather than the code. // FIX: this used to also require >= 8 characters... (lines 146-154), // FIX: was using exact (case-sensitive) comparison here (lines 227-233), // NOTE: caller-module scoping was attempted here ... Reverted intentionally (lines 358-361). A reader of the merged mod has no context for what "used to" be. Same for the README's "A note on CompareStringOrdinal scoping" paragraph and "We're open to a more surgical fix if a maintainer can point at the actual call site" — that's review correspondence, not user documentation. Describe what the code does now.

Functionality notes

Non-critical observations and ideas about the feature behavior itself.

  • Version gate. IsSupportedWindowsVersion requires build ≥ 22631, so the mod is a no-op on Windows 10 and on Windows 11 21H2/22H2. That matches @description, and returning FALSE from Wh_ModInit is the right thing to do here — just flagging that an earlier commit in this PR advertised Windows 10 22H2+ support, so make sure the current gate is the intended one.
  • @include control.exe. On current builds control.exe largely delegates to explorer.exe for the Control Panel folder. If you've confirmed the hooks actually do something in that process, fine; if not, dropping it narrows the injection scope for free.
  • Even a correctly scoped CompareStringOrdinal override is a comparison on a hot path. If item 1 leads you to a specific shell32 function instead, that's strictly better — hooking the decision point is both cheaper and far easier to reason about than influencing a string primitive it happens to call.


Next steps:

  • /ai-review - after pushing fixes, to get a review of the updated code. You can repeat this as many times as you need, but each review is thorough and usually there's no need for more than 2-3 iterations.
  • /ready-for-reviewer - once you're satisfied with the state of the pull request, to hand it over to a human reviewer. If some findings above are left unaddressed, add a short note explaining why.

See the review process for details.

@windhawk-reviewer windhawk-reviewer Bot added waiting-for-author The author's turn: request an AI review, or respond to one that was posted. and removed waiting-for-ai-review An AI review was requested and is being prepared. labels Aug 29, 2026
Refactor comparison logic and improve applet handling.
@AdmXP8

AdmXP8 commented Aug 29, 2026

Copy link
Copy Markdown
Author

/ai-review

@windhawk-reviewer windhawk-reviewer Bot added waiting-for-ai-review An AI review was requested and is being prepared. and removed waiting-for-author The author's turn: request an AI review, or respond to one that was posted. labels Aug 29, 2026
@windhawk-reviewer

Copy link
Copy Markdown

Submission review

Note: This review was done by Claude. Due to the amount of submissions, doing a fully manual review for each pull request is no longer feasible. Thank you for understanding.

Remember: The AI reviewer can be wrong - it may misread code, flag correct code as broken, or suggest changes that make things worse. Treat its findings as suggestions to verify, not instructions to follow blindly. You're responsible for the code you submit, so if a finding doesn't hold up, say so instead of changing working code to satisfy it.

Please address the following issues. The items in the collapsed sections are optional, so it's your call whether to address them.


The _MapLegacyName half of the mod is fine and follows an established pattern. The CompareStringOrdinal half is the blocker: as written it corrupts string comparisons process-wide in Explorer, and that needs to be scoped (or removed) before this can ship.

1. The process-wide CompareStringOrdinal hook is not safe to ship as-is.

CompareStringOrdinal is one of the hottest string primitives in explorer.exe — shell32, windows.storage, ExplorerFrame and friends use it for file/folder names, extensions, canonical names, property keys and shell-namespace identity checks. The hook rewrites CSTR_EQUALCSTR_LESS_THAN for every comparison in the process whose operand exactly matches one of the target strings, and the target list includes very ordinary words:

constexpr std::wstring_view g_szBareLegacyNames[] = {
    L"troubleshooting", L"installedupdates", L"defaultprograms",
    L"devicesandprinters", L"fonts", L"system",
};

Concrete consequences:

  • C:\Windows\Fonts and C:\Windows\System are real shell folders. Any CompareStringOrdinal(L"Fonts", -1, L"Fonts", -1, TRUE) anywhere in Explorer now answers "not equal", regardless of who asked or why. The same holds for the ::{...} CLSID paths, which Explorer compares when resolving/deduplicating namespace items.
  • Worse than a wrong answer: it makes the comparator inconsistent. cmp(a, b) and cmp(b, a) both return CSTR_LESS_THAN for equal strings, which violates antisymmetry. Sorts and — more importantly — the binary searches the shell runs over sorted canonical-name tables are built on this primitive. A wrong result mid-search doesn't just fail to find the targeted entry, it sends the search down the wrong half and can return the wrong entry (or miss a different applet entirely). Inconsistent comparators are also a classic source of out-of-bounds reads in sort implementations.
  • The mod's own CustomApplets setting hands users a direct lever on this: the $description explicitly invites "bare legacy keywords (e.g. system)", so a user typing music, home or settings silently breaks that string's equality everywhere in Explorer.

The README's argument that the blast radius is bounded because the hook "never touches a comparison unless the real result was already CSTR_EQUAL" is the part that's backwards: turning a correct "these are equal" into "not equal" is exactly the damaging direction, and equality is what callers act on.

What to do instead — in order of preference:

  • Find the actual decision site and hook that. The README says three attempts at return-address/stack-walk scoping failed. The reliable way to identify the caller isn't from inside the hook — it's from a debugger on the unhooked function: break on kernelbase!CompareStringOrdinal conditioned on the target string, take a stack trace with shell32/windows.storage symbols loaded, and read off the calling function. Once you have a name, hook that function directly, the way control-panel-in-modern-file-explorer hooks the specific UseSeparateProcess/IsControlPanel decision functions instead of a generic string API.

  • Or arm the override from a narrow, already-hooked entry point with a thread_local flag, so the generic hook is inert everywhere else. This is the standard Windhawk pattern for exactly this problem — see classic-explorer-dragdrop, which scopes a process-wide OpenThemeData hook to a single shell32 call:

    thread_local bool g_fInLegacyNameMapping = false;
    
    bool COpenControlPanel__MapLegacyName_hook(...) {
        g_fInLegacyNameMapping = true;
        bool r = COpenControlPanel__MapLegacyName_orig(...);
        g_fInLegacyNameMapping = false;
        return r;
    }
    
    int WINAPI CompareStringOrdinal_hook(...) {
        int result = CompareStringOrdinal_orig(...);
        if (!g_fInLegacyNameMapping) return result;   // inert everywhere else
        ...
    }

    If _MapLegacyName turns out not to be the enclosing frame, the debugger step above tells you which shell32 function is — hook that one and arm the flag around it.

  • Or drop the second hook. Worth testing per applet which of the six actually still redirect with only the _MapLegacyName hook installed. If it's a subset, the CompareStringOrdinal target list shrinks a lot (and the bare-word entries like system/fonts, which carry most of the risk, may not be needed at all).

Until it's scoped, please don't ship the unscoped version — "we couldn't scope it" isn't a limitation users can opt out of, since it applies to the whole process from the moment the mod loads.

2. CustomApplets entries must not feed the CompareStringOrdinal hook.

Independently of how #1 is resolved: IsPlausibleAppletId accepts any non-empty string up to 128 chars, and MatchesTargetList then applies it process-wide. Even with a properly scoped hook, letting arbitrary user input into a generic string-comparison override is a sharp edge with no upside. Restrict g_customApplets to the _MapLegacyName hook (where a non-matching entry is genuinely harmless), and keep the CompareStringOrdinal list to the built-in, vetted set.

3. The symbol hook is marked optional, so the "failed to hook" log can never fire.

const WindhawkUtils::SYMBOL_HOOK shell32DllHooks[] = {
    { { L"private: bool __cdecl COpenControlPanel::_MapLegacyName(...)" },
      (void**)&COpenControlPanel__MapLegacyName_orig,
      (void*)COpenControlPanel__MapLegacyName_hook,
      true }   // <- optional
};

Per windhawk_utils.h, optional = true means "the absence of this symbol isn't considered an error" — HookSymbols returns true and originalFunction is left unchanged. This is the only hook in the array and the mod is useless without it, so the comment above the call ("If that happens, we log it clearly instead of silently doing nothing") describes the opposite of what the code does: on a Windows build where the symbol changed, the mod silently does nothing and logs success. Set it to false.

4. Overlap with settings-to-control-panel.

That mod already hooks COpenControlPanel::_MapLegacyName and suppresses mapping for a whitelist (settings-to-control-panel.wh.cpp#L1766-L1825) — including system / microsoft.system, which this mod also targets. The _MapLegacyName half of this submission is functionally the same feature with a different whitelist; the genuinely new part is the second hook. The maintainer's consistent preference is to extend an existing mod (add the applets as an option, or open an issue/PR on that mod) rather than merge a near-overlapping one — the catalog already has several mods in this exact area (settings-to-control-panel, win7-legacy-applet-restorer, windows-update-control-panel-restorer, control-panel-in-modern-file-explorer). The README does explain the delta, which helps, but it's worth checking with the author of settings-to-control-panel whether the six applets can just be added there.

5. The README is written for the reviewer, not for users.

The README is what users read inside Windhawk. Right now it contains a development/negotiation log:

  • "we've made three separate attempts to restrict this override… (1) … (2) … (3) …"
  • "Per a reviewer's feedback, this is consistent with a known limitation…"
  • "We're open to a properly-scoped fix if a maintainer can identify the actual redirect-decision call site."
  • "It does not patch or modify any module's memory - earlier versions did, but testing showed…"

None of that means anything to someone installing the mod. Keep the README to: what it does, which applets, how to configure CustomApplets, and a plain statement of any user-visible limitation. The rationale and the attempt history belong in this PR thread. The same applies to the code comments (// FIX: all comparisons are case-insensitive, the multi-paragraph // NOTE: three separate attempts at caller-module scoping…) — comments should describe what the code does now.

Optional improvements

Minor polish — none of this affects users, so it's your call.

  • -lcomctl32 is unused. Nothing in the mod uses comctl32; drop it from @compilerOptions.

  • g_isInitialized is dead. It's assigned in Wh_ModInit and cleared in Wh_ModUninit, but never read. Same for the InterlockedExchange(&g_shell32HookApplied, 0) in Wh_ModUninit — the module is unloaded immediately after, so resetting globals there does nothing.

  • The LoadLibraryExW hook is dead code for this mod's @include list. Both explorer.exe and control.exe import shell32.dll statically, so it's always loaded by the time Wh_ModInit runs and ApplyShell32Hooks(false) always wins the race. Detouring a hot loader API for a case that can't happen is pure cost — consider only installing it when GetModuleHandleW(L"shell32.dll") returns null, as win7-open-with-dialog does for the same module. Also, the comment in the hook is inaccurate: it says the handle comparison is there because "shell32 can arrive as a static dependency of some other DLL loaded through this same API" — in that case result is the other DLL's base, so result == GetModuleHandleW(L"shell32.dll") is false and the case isn't covered either way.

  • try/catch in the hooks is dead weight. COpenControlPanel__MapLegacyName_hook and CompareStringOrdinal_hook only touch fixed arrays, _wcsnicmp, and iterate an already-built vector — nothing there can throw. The wrappers add noise (and a Wh_Log call in the CompareStringOrdinal catch path would re-enter the hook). The try/catch in Wh_ModInit is reasonable since LoadCustomAppletSettings allocates.

  • Use the type-safe SetFunctionHook overload. Both targets are already correctly typed, so the (void*) casts aren't needed and lose the compile-time signature check:

    WindhawkUtils::SetFunctionHook(pCompareStringOrdinal, CompareStringOrdinal_hook,
                                   &CompareStringOrdinal_orig);
  • SafeCompareString mixes two return conventions. It returns CSTR_LESS_THAN (1) on invalid input but _wcsnicmp's 0/±1 otherwise. It happens not to produce a false match (callers test == 0), but it's confusing. Both call sites also already verified the lengths are equal, so the minLen logic is unreachable — a plain _wcsnicmp(str, entry, len) == 0 would do. Related: when cch == -1, MatchesTargetList computes wcslen once via GetEffectiveLength and then again inside SafeCompareString for every candidate; pass the length you already have.

Functionality notes

Non-critical observations and ideas about the feature behavior itself.

  • The 22631 build gate may be stricter than needed. Both hooks are name/symbol-scoped, so they're inert on builds where the redirect doesn't happen. Gating at dwBuildNumber >= 22631 means 22H2 (22621) users get nothing even though several of these applets redirect there too. Worth testing whether the gate can be lowered — or dropped, letting the symbol resolution itself decide.

  • Verify the Fonts entry is actually needed. Of the six applets, Fonts (::{BD84B380-8CA2-1069-AB1D-08000948F534}) is the one that still opens a classic shell folder on most 23H2/24H2 builds. If it doesn't need suppressing, dropping it removes one of the two riskiest bare keywords (fonts) from the comparison list.

  • Both this mod and settings-to-control-panel suppress system in _MapLegacyName. Harmless when both are enabled (both return the same "not mapped" answer), but worth stating in the README's compatibility notes since users are likely to run both.

  • The "an empty row ends the list" caveat is standard Windhawk array behavior, so the $description warning is fine — but the log line for a gap is Loaded N custom applet ID(s) with no hint that later rows were dropped. Logging when the loop stops at row i while row i+1 is non-empty would make a mis-edited list obvious.


Next steps:

  • /ai-review - after pushing fixes, to get a review of the updated code. You can repeat this as many times as you need, but each review is thorough and usually there's no need for more than 2-3 iterations.
  • /ready-for-reviewer - once you're satisfied with the state of the pull request, to hand it over to a human reviewer. If some findings above are left unaddressed, add a short note explaining why.

See the review process for details.

@windhawk-reviewer windhawk-reviewer Bot added waiting-for-author The author's turn: request an AI review, or respond to one that was posted. and removed waiting-for-ai-review An AI review was requested and is being prepared. labels Aug 29, 2026
This update changes the version from 0.9.9 to 1.0.0 and enhances the mod's functionality by improving the description of what the mod does and clarifying its limitations. It also refines the code for better readability and performance.
@AdmXP8

AdmXP8 commented Aug 29, 2026

Copy link
Copy Markdown
Author

/ai-review

@windhawk-reviewer windhawk-reviewer Bot added waiting-for-ai-review An AI review was requested and is being prepared. and removed waiting-for-author The author's turn: request an AI review, or respond to one that was posted. labels Aug 29, 2026
@windhawk-reviewer

Copy link
Copy Markdown

This pull request has already had 3 AI reviews in the last 24 hours, which is the limit, so no review was posted this time.

Comment /ai-review again after 2026-08-30 06:44 UTC (in 14 hours) to get another one.

@windhawk-reviewer windhawk-reviewer Bot added waiting-for-author The author's turn: request an AI review, or respond to one that was posted. and removed waiting-for-ai-review An AI review was requested and is being prepared. labels Aug 29, 2026
@AdmXP8

AdmXP8 commented Aug 29, 2026

Copy link
Copy Markdown
Author

I think that's enough; the remaining issues aren't significant, and replacing 'CompareStringOrdinal' would break the mod

Restored original in-Control-Panel behavior and fixed shell32 hook application logic.
@AdmXP8

AdmXP8 commented Aug 30, 2026

Copy link
Copy Markdown
Author

/ready-for-reviewer

@windhawk-reviewer

Copy link
Copy Markdown

@AdmXP8 /ready-for-reviewer can't be applied here: the most recent AI review covers 93eb3c5, but the current head of this pull request is 5307c7b. Comment /ai-review to get a review of the current code.

@AdmXP8

AdmXP8 commented Aug 30, 2026

Copy link
Copy Markdown
Author

/ai-review

@windhawk-reviewer windhawk-reviewer Bot added the waiting-for-ai-review An AI review was requested and is being prepared. label Aug 30, 2026
@windhawk-reviewer

Copy link
Copy Markdown

Submission review

Note: This review was done by Claude. Due to the amount of submissions, doing a fully manual review for each pull request is no longer feasible. Thank you for understanding.

Remember: The AI reviewer can be wrong - it may misread code, flag correct code as broken, or suggest changes that make things worse. Treat its findings as suggestions to verify, not instructions to follow blindly. You're responsible for the code you submit, so if a finding doesn't hold up, say so instead of changing working code to satisfy it.

Please address the following issues. The items in the collapsed sections are optional, so it's your call whether to address them.


A lot has improved since the last round: the memory patching is gone, _MapLegacyName is properly whitelisted, the symbol hook is required rather than optional, CompareStringOrdinal/LoadLibraryExW are resolved from kernelbase.dll, CustomApplets no longer feeds the comparison override, and the mod makes no persistent changes. Three things still block, and the first one is the same one as last time.

1. The unscoped CompareStringOrdinal override still changes string equality for the whole process — and the built-in list makes it worse than it looks.

MatchesBuiltInList matches with _wcsnicmp regardless of the caller's bIgnoreCase, against three lists that include six ordinary English words:

constexpr std::wstring_view g_szBareLegacyNames[] = {
    L"troubleshooting", L"installedupdates", L"defaultprograms",
    L"devicesandprinters", L"fonts", L"system",
};

So after this hook, any comparison in explorer.exe / control.exe of System, SYSTEM, system, Fonts, FONTS, … against itself returns CSTR_LESS_THAN instead of CSTR_EQUAL — including case-sensitive calls (bIgnoreCase == FALSE), because the list match ignores case either way. C:\Windows\System and C:\Windows\Fonts are real shell folders, and shell32 / windows.storage / ExplorerFrame compare file and folder names, extensions, canonical names and property keys with exactly this API. There is no way to predict which of those paths now take the wrong branch.

Two further points that aren't cosmetic:

  • It breaks antisymmetry: for a listed string A, cmp(A, B) and cmp(B, A) both report CSTR_LESS_THAN, and cmp(A, A) reports "less than itself". The shell binary-searches sorted canonical-name tables with this primitive; a wrong answer mid-search doesn't just miss your entry, it sends the search down the wrong half. Feeding a non-strict-weak ordering to a sort is undefined behavior in the sort itself.
  • The catalog already documents this specific hazard. win7-legacy-applet-restorer.wh.cpp#L3252-L3261 explicitly declines this technique: "a CompareStringOrdinal override perturbs comparisons the shell also uses for ordered lookups, and a name blocked with no classic page behind it makes explorer.exe fail with 0xC0000005."

I understand you don't want to ship a scoped version that doesn't work. Two concrete things you can do that don't require finding the call site first:

  • Shrink the list to the spellings that actually trigger an override. Right now all 18 strings are live in this hook, but only some of them can plausibly be what the redirect table is keyed on. Log every override (both operands + a RtlCaptureStackBackTrace), open each of the six applets once, then delete every entry that never fired. The six bare words carry nearly all of the collision risk and are the least likely form for a GUID- or canonical-name-keyed check — try dropping g_szBareLegacyNames and g_szCanonicalNames from MatchesBuiltInList first and see whether the applets still open.

  • Re-check why scoping attempt (4) failed, because the code explains it. Your _MapLegacyName hook returns early for exactly the six targeted names, without calling the original:

    if (isTargeted) {
        if (nameChanged) *nameChanged = false;
        if (pszNewName && uUnused > 0) *pszNewName = L'\0';
        return false;   // original never runs
    }

    A flag armed "for the duration of the _MapLegacyName call" is therefore never armed on the one path that matters, and it is disarmed again by the time the caller acts on the result. That's consistent with what you observed, and it doesn't mean the comparison is asynchronous — it means the arm/disarm has to bracket the caller, not the callee. Hook an outer shell32 entry point (IOpenControlPanel::Open / COpenControlPanel::Open, or whatever leads into the navigation) and set a thread_local bool around its call to the original; CompareStringOrdinal_hook returns the real result unless that flag is set. classic-explorer-dragdrop.wh.cpp#L43-L70 is a minimal example of that shape; taskbar-grouping.wh.cpp#L1105-L1115 plus #L1316-L1346 is the thread-id variant.

    If you'd rather find the caller directly: don't try to detect it from inside the hook. Break on kernelbase!CompareStringOrdinal in WinDbg with a condition on the target string, with shell32 symbols loaded, and read the frame off the stack.

2. Wh_ModInit reports success even when the one targeted hook fails to resolve.

The comment above shell32DllHooks says a resolution failure "must be a reported failure, not a silently-accepted no-op" — but ApplyShell32Hooks returns void and only logs, so Wh_ModInit returns TRUE regardless:

if (!WindhawkUtils::HookSymbols(hShell32, shell32DllHooks, ARRAYSIZE(shell32DllHooks))) {
    Wh_Log(L"Failed to resolve/hook COpenControlPanel::_MapLegacyName - ...");
}

On a build where the symbol changed, the user gets the process-wide CompareStringOrdinal override installed and none of the feature. Make it return bool and, on the Wh_ModInit path, return FALSE when it fails — Windhawk reloads the mod after each settings change, so that's a safe way to fail. (The same comment also says "this is the only hook this mod installs", which isn't true — there are three.)

3. Review correspondence is shipped inside the mod source.

Lines 494-518, after Wh_ModSettingsChanged, are a verbatim PR reply pasted into the file:

//The AI reviewer is right that an unscoped CompareStringOrdinal hook isn't ideal — I want to explain why it's shipping this way instead of just disagreeing with the finding.
//Four different scoping attempts were made, and each one broke the mod's actual functionality:
...
//I'm keeping this as-is rather than shipping a "safer-looking" scoped version that's actually broken.

That belongs in this thread, not in the mod. The same applies to the four-attempt narrative at lines 302-323 and the // FIX (regressed from an earlier round, restored here): ... blocks at lines 361-372 and 401-410 — they describe revisions of an unmerged file that no future reader can see. Keep the comments that explain what the code does and why (the late-load rationale, the "custom applets deliberately excluded from the comparison hook" note) and delete the rest.

4. Please follow up on the collaboration offer in this thread.

settings-to-control-panel hooks the same private shell32 symbol with the same suppression body, driven by its own hardcoded whitelist (settings-to-control-panel.wh.cpp#L1766-L1824), and win7-legacy-applet-restorer already carries a compatibility note naming this mod (#L75). Their author offered here to merge your applets and the CustomApplets setting into their mod and credit you, and you agreed — the maintainer's standing preference is exactly that, so actually landing it there is likely the shortest path into the catalog. If you decide to keep this as a separate mod after all, please say why in the PR description, and note in the README that both mods hook _MapLegacyName so users running both know what to expect.

Optional improvements

Minor polish — none of this affects users, so it's your call.

  • -lcomctl32 in @compilerOptions is unused — nothing in the mod uses comctl32.
  • @description says the mod works "by unhiding legacy elements safely", but the README says "This mod does not reveal hidden Control Panel applets." The description is the catalog text users read first; make the two agree.
  • LoadCustomAppletSettings uses raw Wh_GetStringSetting + Wh_FreeStringSetting. WindhawkUtils::StringSetting is the RAII form and removes the "must not skip the free on the break path" bookkeeping.
  • Use the type-safe WindhawkUtils::SetFunctionHook overload instead of casting all three arguments — the void*/void** casts throw away the compile-time signature check:
    WindhawkUtils::SetFunctionHook(pCompareStringOrdinal, CompareStringOrdinal_hook,
                                   &CompareStringOrdinal_orig);
  • volatile LONG g_shell32HookApplied + Interlocked*std::atomic<bool> with compare_exchange_strong. And InterlockedExchange(&g_shell32HookApplied, 0) in Wh_ModUninit is a no-op — the image is unloaded right after, so the global goes with it.
  • The second MatchesBuiltInList call is dead: the branch only runs when the original already returned CSTR_EQUAL, so if lpString2 matches, lpString1 matched too. Drop the ||.
  • The try/catch blocks in the three hooks don't protect anything — the bodies only touch fixed arrays, _wcsnicmp, and an already-built vector, none of which allocate or throw. The one in Wh_ModInit is reasonable (LoadCustomAppletSettings allocates). Also, CompareStringOrdinal_hook's catch handler calls the original a second time after the first call already succeeded.
  • The LoadLibraryExW hook can't fire usefully for this @include list: both explorer.exe and control.exe import shell32.dll statically, so ApplyShell32Hooks(false) always wins and the flag is set before any DLL loads. Detouring a hot loader API for a case that can't happen is pure cost — install it only when GetModuleHandleW(L"shell32.dll") returns null, as win7-open-with-dialog.wh.cpp#L8768-L8788 does.
  • Include the headers for what you use rather than relying on <windows.h> transitively: <cwchar> for wcslen / _wcsnicmp.

Functionality notes

Non-critical observations and ideas about the feature behavior itself.

  • The GUID form the setting advertises may not do anything. NormalizeAppletId turns a user's bare GUID into ::{GUID}, but g_customApplets is now checked only against _MapLegacyName's legacy-name parameter, and custom entries are deliberately excluded from the CompareStringOrdinal path. If ::{GUID} monikers never reach _MapLegacyName (worth confirming by logging pszLegacyName for one Control Panel session), the GUID support in the $description is a no-op for custom entries even though the built-in GUIDs are live via the other hook. Either way the two paths should behave consistently, and the description should match.
  • IsSupportedWindowsVersion requires dwMajorVersion == 10, so a future major-version bump silently disables the mod on a system where it would work. dwBuildNumber >= 22631 alone is more durable. The gate also excludes 22H2 (22621), where at least some of these applets redirect too — both hooks are name-scoped, so they're inert on builds that don't redirect.
  • Worth confirming the Fonts entry (::{BD84B380-8CA2-1069-AB1D-08000948F534}) is needed at all — on most 23H2/24H2 builds Fonts still opens as a classic shell folder. Dropping it also removes one of the two riskiest bare keywords from the comparison list.
  • The README's "Difference from other mods" sentence ("Other mods hook this same function but within a new window") is hard to parse. A plain statement — which applets this covers that settings-to-control-panel's whitelist doesn't — would land better with users.
  • The before/after GIFs are a genuinely useful addition for this kind of mod.


Next steps:

  • /ai-review - after pushing fixes, to get a review of the updated code. You can repeat this as many times as you need, but each review is thorough and usually there's no need for more than 2-3 iterations.
  • /ready-for-reviewer - once you're satisfied with the state of the pull request, to hand it over to a human reviewer. If some findings above are left unaddressed, add a short note explaining why.

See the review process for details.

@AdmXP8

AdmXP8 commented Aug 30, 2026

Copy link
Copy Markdown
Author

no listening to this guy makes the mod breaks

@AdmXP8

AdmXP8 commented Aug 30, 2026

Copy link
Copy Markdown
Author

/ready-for-reviewer

@m417z

m417z commented Aug 30, 2026

Copy link
Copy Markdown
Member

What's the status of the collaboration you discussed? As the review explains, having multiple similar mods is sub-optimal. A single, polished and tested mod is preferred.

@AdmXP8

AdmXP8 commented Aug 30, 2026

Copy link
Copy Markdown
Author

What's the status of the collaboration you discussed? As the review explains, having multiple similar mods is sub-optimal. A single, polished and tested mod is preferred.

Hi
I followed up with the author of settings-to-control-panel directly about this, since I wanted a real answer rather than just asserting a difference myself.

The core issue: settings-to-control-panel prevents the redirect by opening the applet in a new, separate window. This mod instead loads the applet in-place, inside the existing Control Panel window. That's not a stylistic choice - I tested it, and on 23H2+, three of the six applets this mod targets (Troubleshooting, Installed Updates, Default Programs) can no longer be launched in a new window at all, not even via shell:::. So whitelisting them on the other mod's side wouldn't actually restore them; the new-window path itself is broken for those three regardless of the redirect fix.

I asked the author directly whether their mod could adopt in-place loading instead, since if it could, I'd genuinely rather merge into one polished mod than maintain two overlapping ones. Their answer: their mod's redirect handling is intentionally broader - it targets multiple redirect scenarios, not just the Control Panel-opening case this mod focuses on - and they agreed the two are "similar, but the feature is different," and suggested I keep this mod separate on that basis.

So: same underlying symbol hooked, different mechanism, different scope, and confirmed as such by the other mod's author rather than just my own read of it. I'll add a compatibility note to both READMEs noting both mods hook _MapLegacyName, so anyone running both knows what to expect.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

waiting-for-author The author's turn: request an AI review, or respond to one that was posted.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants