Update Open in Admin Terminal to v1.17.1 - #4993
Conversation
|
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 See the pull request review process for the full details. |
|
/ai-review |
Submission reviewNote: 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 script-execution feature launches its interpreters by bare name from a user-controlled directory (elevated), and the menu-hook gate that was removed to fix the navigation pane makes every popup menu in Explorer do COM work. Details below. 1. Script interpreters are launched by bare name, with the script's own folder as the working directory — an elevated hijack vector
The clearest failure case is the std::wstring scriptCommand =
L"cscript.exe //nologo " + QuoteCommandLineArgument(scriptPath);
if (s.keepOpenAfterScript) {
spec.executable = L"cmd.exe";
spec.parameters = L"/k " + scriptCommand;
The mod already resolves every terminal preset to a full path ( static std::wstring SystemPath(PCWSTR exe) {
WCHAR dir[MAX_PATH];
UINT len = GetSystemDirectoryW(dir, ARRAYSIZE(dir));
if (!len || len >= ARRAYSIZE(dir)) {
return exe;
}
std::wstring path(dir, len);
path += L'\\';
return path + exe;
}then 2. The window-class gate was removed from - if (menu && hwnd &&
- (IsShellViewWindow(hwnd) || IsNavigationPaneWindow(hwnd))) {
+ if (menu && hwnd) {With A cheap pre-filter restores the old behavior without breaking the fix. HWND root = GetAncestor(hwnd, GA_ROOT);
WCHAR rootClass[64] = {};
if (root) {
GetClassNameW(root, rootClass, ARRAYSIZE(rootClass));
}
bool eligibleWindow = IsShellViewWindow(hwnd) ||
_wcsicmp(rootClass, L"CabinetWClass") == 0 ||
_wcsicmp(rootClass, L"ExploreWClass") == 0;
if (menu && hwnd && eligibleWindow) { ... }For reference, mods/explorer-context-menu-custom-items.wh.cpp#L266 bails out of the same hook on the first line when the window isn't a shell view. It would also be worth resolving 3. The navigation-pane target comes from the live cursor position, not from where the menu was actually invoked
Passing 4.
if (_wcsicmp(ext, L".ps1") != 0 && _wcsicmp(ext, L".bat") != 0 &&
_wcsicmp(ext, L".cmd") != 0 && _wcsicmp(ext, L".vbs") != 0 &&
_wcsicmp(ext, L".js") != 0) {
return false;
}and only then checks the user's list. But the setting says "Semicolon-separated extensions treated as scripts", so a user who adds Optional improvements
Minor polish — none of this affects users, so it's your call.
Functionality notes
Non-critical observations and ideas about the feature behavior itself.
Next steps:
See the review process for details. |
|
Addressed the review findings in 3221a1f:
Also fixed the related stale menu-command state, reused the caller's settings snapshot, made VBS/JS labels consistently identify Windows Script Host, and changed execution-policy bypass to default off. Validation:
|
|
/ai-review |
Submission reviewNote: 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. Three things worth fixing before this goes in: 1.
if (arg.find_first_of(L" \t\r\n\"") == std::wstring::npos) {
return arg;
}That's correct for // BuildScriptInterpreterSpec, .bat/.cmd
spec.parameters = std::wstring(s.keepOpenAfterScript ? L"/k " : L"/c ") +
QuoteCommandLineArgument(scriptPath);
// BuildScriptInterpreterSpec, .vbs/.js with keepOpenAfterScript
spec.parameters = L"/k " + QuoteCommandLineArgument(cscriptPath) +
L" //nologo " + QuoteCommandLineArgument(scriptPath);
// BuildLaunchSpec, cmd terminal (pre-existing)
spec.parameters = L"/k cd /d " + QuoteCommandLineArgument(target);
Windows paths can't contain static std::wstring QuoteAlways(const std::wstring& arg) {
return L'"' + arg + L'"';
}( 2. Navigation pane:
Keep the hit test as the primary path (it is the right call for right-click, since the tree's context target is the item under the cursor), but restore the v1.16 if (!ok) {
IShellItemArray* selectedItems = nullptr;
if (SUCCEEDED(navigationPane->GetSelectedItems(&selectedItems)) && selectedItems) {
DWORD count = 0;
IShellItem* item = nullptr;
if (SUCCEEDED(selectedItems->GetCount(&count)) && count == 1 &&
SUCCEEDED(selectedItems->GetItemAt(0, &item)) && item) {
std::wstring path;
if (GetFilesystemPathFromShellItem(item, path) && IsDirectoryPath(path)) {
targetOut.path = std::move(path);
targetOut.kind = IsDriveRootPath(targetOut.path) ? TargetKind::DriveItem
: TargetKind::FolderItem;
ok = true;
}
item->Release();
}
selectedItems->Release();
}
}Also worth guarding the 3. The new unconditional ClearCurrentMenuState();
return result;
}
Two clean ways out — please pick one rather than leaving it ambiguous:
Optional improvements
Minor polish — none of this affects users, so it's your call.
Functionality notes
Non-critical observations and ideas about the feature behavior itself.
Next steps:
See the review process for details. |
|
Addressed the three mandatory findings in 1149629:
Validation: 12 regression tests passed; syntax compilation passed for x86, x64, and ARM64; Windhawk compliance passed; and the canonical/catalog files match byte-for-byte at v1.17.1. Optional refactors remain deferred. |
|
/ai-review |
|
This pull request has already had 2 AI reviews in the last 24 hours, which is the limit, so no review was posted this time. Comment |
|
/ai-review |
Submission reviewNote: 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. Findings 2 and 3 from the previous round look correctly addressed. The 1.
What that means for the three call sites: // BuildLaunchSpec, cmd terminal — OK, first char after /k is 'c', no stripping
spec.parameters = L"/k cd /d " + QuoteCmdPath(target);
// BuildScriptInterpreterSpec, .bat/.cmd — quotes stripped, path re-parsed by cmd
spec.parameters = std::wstring(s.keepOpenAfterScript ? L"/k " : L"/c ") +
QuoteCmdPath(scriptPath);
// BuildScriptInterpreterSpec, .vbs/.js with keepOpenAfterScript — four quotes,
// outer pair stripped, the rest is mangled
spec.parameters = L"/k " + QuoteCmdPath(cscriptPath) +
L" //nologo " + QuoteCmdPath(scriptPath);
The fix for all of them is the standard idiom — build the command with each argument quoted, then wrap the whole command in one more pair of quotes, so the pair static std::wstring BuildCmdCommandLine(bool keepOpen,
const std::wstring& command) {
// cmd removes the first and last quote of the /k|/c line, so wrap the
// whole command in an extra pair and let the inner quotes survive.
return std::wstring(keepOpen ? L"/k \"" : L"/c \"") + command + L'"';
}// .bat / .cmd
spec.parameters = BuildCmdCommandLine(s.keepOpenAfterScript,
QuoteCmdPath(scriptPath));
// .vbs / .js, keep open
spec.parameters = BuildCmdCommandLine(true,
QuoteCmdPath(cscriptPath) + L" //nologo " +
QuoteCmdPath(scriptPath));
Worth adding to your regression set: a Optional improvements
Minor polish — none of this affects users, so it's your call. Most of these are carried over from the previous rounds and were deliberately deferred; re-listing them only so nothing is lost.
Functionality notes
Non-critical observations and ideas about the feature behavior itself.
Next steps:
See the review process for details. |
|
Addressed the remaining mandatory
Validation: 13 tests passed; syntax compilation passed for x86, x64, and ARM64; Windhawk compliance passed; and the canonical/catalog files match byte-for-byte at v1.17.1. Findings 2 and 3 remain unchanged. Optional refactors remain deferred. |
|
/ai-review |
Submission reviewNote: 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 1. The
args = {L"new-tab", L"-d", interpreter.workingDirectory, interpreter.executable};
spec.parameters = JoinCommandLineArguments(args);
if (!interpreter.parameters.empty()) {
spec.parameters += L" " + interpreter.parameters; // raw /s /k ""…"" string
}But
The direct (non-hosted) paths you already validated are unaffected — this is only the nesting case. The simplest fix that keeps the verified behavior is to not nest when a static LaunchSpec BuildScriptLaunchSpec(const Settings& s,
const std::wstring& scriptPath) {
LaunchSpec interpreter = BuildScriptInterpreterSpec(s, scriptPath);
// A cmd /s /k "…" command line is meant for direct invocation; a host
// terminal re-parses and re-quotes it, which mangles the inner quotes.
if (!IsScriptHostChoice(s.terminalEffectiveChoice) ||
UsesCmdWrapper(s, scriptPath)) {
return interpreter;
}
...If you'd rather keep the host terminal for args = {L"new-tab", L"-d", dir, cmdExe,
s.keepOpenAfterScript ? L"/k" : L"/c", scriptPath};
spec.parameters = JoinCommandLineArguments(args);which reaches 2. Navigation-pane detection now hangs entirely on the tracking coordinates, with no fallback for the mouse path
if (IsKeyboardContextMenuPoint(invocationPoint)) {
HWND focusedWindow = GetFocus();
return focusedWindow && IsNavigationPaneWindow(focusedWindow);
}
HWND invocationWindow = WindowFromPoint(invocationPoint);
return invocationWindow && IsNavigationPaneWindow(invocationWindow);and That's fine for a plain right-click, but the classic menu isn't always tracked at the click point. Your own README notes that on Windows 11 the entry shows up under static bool IsNavigationPaneContextWindow(HWND hwnd, const POINT& pt) {
if (!IsKeyboardContextMenuPoint(pt)) {
HWND w = WindowFromPoint(pt);
if (w && IsNavigationPaneWindow(w)) {
return true;
}
}
// The tracking point isn't always the click point.
HWND focus = GetFocus();
return focus && IsNavigationPaneWindow(focus) && !IsShellViewWindow(hwnd);
}and correspondingly letting the Optional improvements
Minor polish — none of this affects users, so it's your call. Most are carried over from earlier rounds and were deliberately deferred; re-listing so nothing is lost.
Functionality notes
Non-critical observations and ideas about the feature behavior itself.
Next steps:
See the review process for details. |
|
Addressed the two required findings in 6b442f7:
Validation:
The collapsed optional refactors remain deferred as out-of-scope polish. |
|
/ai-review |
Submission reviewNote: 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 The script entry names (and shows the icon of) the selected terminal, but cmd-wrapped scripts no longer launch there The new early return in bool usesCmdWrapper = _wcsicmp(ext, L".bat") == 0 ||
_wcsicmp(ext, L".cmd") == 0 ||
(s.keepOpenAfterScript &&
(_wcsicmp(ext, L".vbs") == 0 || _wcsicmp(ext, L".js") == 0));
if (!IsScriptHostChoice(s.terminalEffectiveChoice) || usesCmdWrapper) {
return interpreter; // launches cmd.exe directly
}but the label and the icon still branch on // GetScriptTerminalDisplayName
if (IsScriptHostChoice(settings.terminalEffectiveChoice)) {
return GetTerminalDisplayName(settings); // "Windows Terminal"
}
// GetScriptIconSettings
if (IsScriptHostChoice(settings.terminalEffectiveChoice)) {
return settings; // wt's icon
}So with Windows Terminal (or WezTerm / Alacritty / ConEmu) selected and Hoisting the predicate into a helper and using it in all three places fixes label, icon and launch together: static bool UsesCmdWrapper(const Settings& s, PCWSTR ext) {
return _wcsicmp(ext, L".bat") == 0 || _wcsicmp(ext, L".cmd") == 0 ||
(s.keepOpenAfterScript &&
(_wcsicmp(ext, L".vbs") == 0 || _wcsicmp(ext, L".js") == 0));
}
static bool UsesSelectedTerminalHost(const Settings& s,
const std::wstring& scriptPath) {
return IsScriptHostChoice(s.terminalEffectiveChoice) &&
!UsesCmdWrapper(s, PathFindExtensionW(scriptPath.c_str()));
}Then Optional improvements
Minor polish — none of this affects users, so it's your call. Most are carried over from earlier rounds and were deliberately deferred; re-listed compactly so nothing is lost.
Functionality notes
Non-critical observations and ideas about the feature behavior itself.
Next steps:
See the review process for details. |
|
Addressed the current AI-review finding in
Verification:
Deferred intentionally: the optional focused-navigation fallback tightening needs manual Explorer testing first, because restricting it by the tree rectangle could regress the Windows 11 “Show more options” path. Other optional cleanup suggestions are outside this review correction. Remaining manual verification: Explorer menu behavior for supported script types and navigation-pane/menu surfaces. |
|
/ai-review |
Submission reviewNote: 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 On the desktop, every right-click that isn't a supported folder still gets the entry — retargeted at the Desktop folder The last block of if (hasSelectedCount && selectedCount > 1) {
ok = false; // multi-selection -> deliberately no entry
} else if (...) {
...
} else if (selectedPaths.size() == 1 && IsFilePath(selectedPaths[0])) {
if (settings.showOnScriptFiles && IsScriptExtension(...)) { ... }
// non-script file -> ok stays false
}
...
if (!ok && isDesktopShellView) { // <- catches all of the above
std::wstring folderPath;
if (GetDesktopFolderPath(folderPath) && IsDirectoryPath(folderPath)) {
targetOut.kind = TargetKind::FolderBackground;
targetOut.path = folderPath;
ok = true;
}
}
It also contradicts the README ("The mod intentionally targets filesystem folders and drive roots only"), and it interacts with the new feature: with The fallback is only meant to cover "the shell view / current folder couldn't be resolved for a desktop background click", so gate it on there being no selection: bool ok = false;
bool sawSelection = false;
IFolderView* folderView = nullptr;
if (SUCCEEDED(shellView->QueryInterface(IID_IFolderView, ...)) && folderView) {
...
if (hasSelectedCount && selectedCount > 1) {
sawSelection = true;
} else if (hasSelectedCount && selectedCount == 0) {
...
} else {
std::vector<std::wstring> selectedPaths;
UINT shellSelectedCount = GetSelectedPaths(shellView, selectedPaths, 2);
sawSelection = shellSelectedCount > 0 || !selectedPaths.empty();
...
}
folderView->Release();
}
// Only fall back to the desktop folder when nothing is selected — otherwise a
// right-click on a file, a virtual item or a multi-selection would silently be
// retargeted at the Desktop folder.
if (!ok && !sawSelection && isDesktopShellView) {Optional improvements
Minor polish — none of this affects users, so it's your call. Most are carried over from earlier rounds and were deliberately deferred; re-listed compactly so nothing is lost.
Functionality notes
Non-critical observations and ideas about the feature behavior itself.
Next steps:
See the review process for details. |
Changelog
If this pull request updates an existing mod, describe the changes below:
.ps1,.bat,.cmd,.vbs, and.jsfiles.Mod authorship
If this pull request introduces a new mod, please complete the section below.
This mod was created by:
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.