From 902cf5de754ed42245a383de83048f3171910ffa Mon Sep 17 00:00:00 2001 From: hatayama Date: Fri, 17 Jul 2026 03:09:13 +0900 Subject: [PATCH] fix: focus-window now verifies foreground state instead of trusting AppActivate on Windows The Windows focus scripts reported success whenever WScript.Shell AppActivate returned true, but the OS foreground lock rejects SetForegroundWindow from background processes, so `uloop focus-window` returned Success:true while Unity stayed behind other windows (measured with GetForegroundWindow on Windows 11). - Escalate activation: SW_RESTORE only when minimized, plain SetForegroundWindow, then AttachThreadInput to the foreground thread's input queue, then the transient Alt-key workaround - Verify with GetForegroundWindow that the foreground window belongs to the Unity PID after each stage; throw an actionable error (foreground lock, click Unity manually) when every stage fails - Remove the AppActivate fallback, the source of the false success - Map a FocusCommandTimeout kill (empty stderr) to an explanatory error instead of a bare "exit status 1", since activation calls can block on a stalled Editor message pump during domain reloads --- cli/common/unityprocess/command_error.go | 19 +++++ cli/common/unityprocess/command_error_test.go | 29 ++++++++ .../unityprocess/focus_unity_process.ps1 | 69 +++++++++++++++++-- .../focus_unity_process_with_restore.ps1 | 68 ++++++++++++++++-- cli/common/unityprocess/focus_windows.go | 6 +- cli/common/unityprocess/process_test.go | 53 +++++++++----- 6 files changed, 209 insertions(+), 35 deletions(-) diff --git a/cli/common/unityprocess/command_error.go b/cli/common/unityprocess/command_error.go index c259dfa36d..9ef78874c5 100644 --- a/cli/common/unityprocess/command_error.go +++ b/cli/common/unityprocess/command_error.go @@ -1,6 +1,8 @@ package unityprocess import ( + "context" + "errors" "fmt" "strings" ) @@ -15,3 +17,20 @@ func commandErrorWithStderr(err error, stderr string) error { } return fmt.Errorf("%w: %s", err, trimmedStderr) } + +// focusCommandError converts a focus script failure into an actionable error. +// Why: a timeout kills the script before it can write anything to stderr, so without this +// mapping the caller only sees a bare "exit status 1" with no hint about the stalled Editor. +func focusCommandError(contextErr error, runErr error, stderr string) error { + if runErr == nil { + return nil + } + if errors.Is(contextErr, context.DeadlineExceeded) { + return fmt.Errorf( + "focusing the Unity window timed out after %s; the Unity Editor may be busy (for example during a domain reload), retry once it is responsive: %w", + FocusCommandTimeout, + runErr, + ) + } + return commandErrorWithStderr(runErr, stderr) +} diff --git a/cli/common/unityprocess/command_error_test.go b/cli/common/unityprocess/command_error_test.go index 3f1c7c3b7c..fed786e542 100644 --- a/cli/common/unityprocess/command_error_test.go +++ b/cli/common/unityprocess/command_error_test.go @@ -1,6 +1,7 @@ package unityprocess import ( + "context" "errors" "strings" "testing" @@ -25,3 +26,31 @@ func TestCommandErrorWithStderrKeepsOriginalErrorWithoutStderr(t *testing.T) { t.Fatalf("expected original error, got %v", actual) } } + +// Verifies a timed-out focus script is reported as a busy-Editor timeout instead of a bare exit status. +func TestFocusCommandErrorReportsTimeoutWhenContextDeadlineExceeded(t *testing.T) { + err := focusCommandError(context.DeadlineExceeded, errors.New("exit status 1"), "") + + if err == nil || !strings.Contains(err.Error(), "timed out") || !strings.Contains(err.Error(), "domain reload") { + t.Fatalf("expected timeout explanation, got %v", err) + } +} + +// Verifies a focus script throw keeps the stderr text without the timeout explanation. +func TestFocusCommandErrorKeepsStderrForScriptFailures(t *testing.T) { + err := focusCommandError(nil, errors.New("exit status 1"), "Windows refused to bring the Unity window\r\n") + + if err == nil || !strings.Contains(err.Error(), "Windows refused to bring the Unity window") { + t.Fatalf("expected stderr in error, got %v", err) + } + if strings.Contains(err.Error(), "timed out") { + t.Fatalf("expected no timeout explanation, got %v", err) + } +} + +// Verifies a nil run error yields no focus error. +func TestFocusCommandErrorReturnsNilWithoutRunError(t *testing.T) { + if err := focusCommandError(context.DeadlineExceeded, nil, ""); err != nil { + t.Fatalf("expected nil, got %v", err) + } +} diff --git a/cli/common/unityprocess/focus_unity_process.ps1 b/cli/common/unityprocess/focus_unity_process.ps1 index 64d904e43f..89519c3141 100644 --- a/cli/common/unityprocess/focus_unity_process.ps1 +++ b/cli/common/unityprocess/focus_unity_process.ps1 @@ -3,18 +3,73 @@ Add-Type -TypeDefinition @" using System; using System.Runtime.InteropServices; public static class Win32Interop { + [DllImport("user32.dll")] public static extern IntPtr GetForegroundWindow(); [DllImport("user32.dll")] public static extern bool SetForegroundWindow(IntPtr hWnd); [DllImport("user32.dll")] public static extern bool ShowWindowAsync(IntPtr hWnd, int nCmdShow); + [DllImport("user32.dll")] public static extern bool IsIconic(IntPtr hWnd); + [DllImport("user32.dll")] public static extern bool BringWindowToTop(IntPtr hWnd); + [DllImport("user32.dll")] public static extern uint GetWindowThreadProcessId(IntPtr hWnd, IntPtr processIdPointer); + [DllImport("kernel32.dll")] public static extern uint GetCurrentThreadId(); + [DllImport("user32.dll")] public static extern bool AttachThreadInput(uint idAttach, uint idAttachTo, bool fAttach); + [DllImport("user32.dll")] public static extern void keybd_event(byte bVk, byte bScan, uint dwFlags, UIntPtr dwExtraInfo); + public static uint GetWindowProcessId(IntPtr hWnd) { + IntPtr buffer = Marshal.AllocHGlobal(4); + try { + GetWindowThreadProcessId(hWnd, buffer); + return (uint)Marshal.ReadInt32(buffer); + } finally { + Marshal.FreeHGlobal(buffer); + } + } } "@ +# Why: SetForegroundWindow can report the switch before the shell finishes it, so poll briefly instead of a single read. +function Test-TargetForeground { + for ($attempt = 0; $attempt -lt 10; $attempt++) { + if ([Win32Interop]::GetWindowProcessId([Win32Interop]::GetForegroundWindow()) -eq {{PID}}) { return $true } + Start-Sleep -Milliseconds 50 + } + return $false +} try { $process = Get-Process -Id {{PID}} -ErrorAction Stop } catch { throw 'Unity process was not found: {{PID}}' } $handle = $process.MainWindowHandle if ($handle -eq 0) { throw 'Unity process has no main window handle: {{PID}}' } -$shown = [Win32Interop]::ShowWindowAsync($handle, 9) -if (-not $shown) { throw 'Failed to show Unity window' } -$focused = [Win32Interop]::SetForegroundWindow($handle) -if (-not $focused) { - $shell = New-Object -ComObject WScript.Shell - $focused = $shell.AppActivate({{PID}}) +# Why: SW_RESTORE on a non-minimized window would shrink a maximized Unity window, so restore only when minimized. +if ([Win32Interop]::IsIconic($handle)) { + $shown = [Win32Interop]::ShowWindowAsync($handle, 9) + if (-not $shown) { throw 'Failed to show Unity window' } +} +[void][Win32Interop]::SetForegroundWindow($handle) +if (-not (Test-TargetForeground)) { + # Why: the Windows foreground lock rejects SetForegroundWindow from background processes; sharing the + # foreground thread's input queue via AttachThreadInput lifts that restriction. + $currentThreadId = [Win32Interop]::GetCurrentThreadId() + $foreground = [Win32Interop]::GetForegroundWindow() + $foregroundThreadId = [Win32Interop]::GetWindowThreadProcessId($foreground, [IntPtr]::Zero) + $targetThreadId = [Win32Interop]::GetWindowThreadProcessId($handle, [IntPtr]::Zero) + $attachedForeground = $false + $attachedTarget = $false + try { + if ($foregroundThreadId -ne 0 -and $foregroundThreadId -ne $currentThreadId) { + $attachedForeground = [Win32Interop]::AttachThreadInput($currentThreadId, $foregroundThreadId, $true) + } + if ($targetThreadId -ne 0 -and $targetThreadId -ne $currentThreadId) { + $attachedTarget = [Win32Interop]::AttachThreadInput($currentThreadId, $targetThreadId, $true) + } + [void][Win32Interop]::BringWindowToTop($handle) + [void][Win32Interop]::SetForegroundWindow($handle) + } finally { + if ($attachedTarget) { [void][Win32Interop]::AttachThreadInput($currentThreadId, $targetThreadId, $false) } + if ($attachedForeground) { [void][Win32Interop]::AttachThreadInput($currentThreadId, $foregroundThreadId, $false) } + } +} +if (-not (Test-TargetForeground)) { + # Why: a transient Alt keypress makes this process the last input source, a documented workaround + # that unlocks SetForegroundWindow when AttachThreadInput alone is not enough. + [Win32Interop]::keybd_event(0x12, 0, 0, [UIntPtr]::Zero) + [void][Win32Interop]::SetForegroundWindow($handle) + [Win32Interop]::keybd_event(0x12, 0, 2, [UIntPtr]::Zero) +} +if (-not (Test-TargetForeground)) { + throw 'Windows refused to bring the Unity window (PID: {{PID}}) to the foreground (foreground lock). Click the Unity window or its taskbar icon to focus it manually.' } -if (-not $focused) { throw 'Failed to focus Unity window' } diff --git a/cli/common/unityprocess/focus_unity_process_with_restore.ps1 b/cli/common/unityprocess/focus_unity_process_with_restore.ps1 index 8c1c396e6d..69175be53e 100644 --- a/cli/common/unityprocess/focus_unity_process_with_restore.ps1 +++ b/cli/common/unityprocess/focus_unity_process_with_restore.ps1 @@ -6,18 +6,72 @@ public static class Win32Interop { [DllImport("user32.dll")] public static extern IntPtr GetForegroundWindow(); [DllImport("user32.dll")] public static extern bool SetForegroundWindow(IntPtr hWnd); [DllImport("user32.dll")] public static extern bool ShowWindowAsync(IntPtr hWnd, int nCmdShow); + [DllImport("user32.dll")] public static extern bool IsIconic(IntPtr hWnd); + [DllImport("user32.dll")] public static extern bool BringWindowToTop(IntPtr hWnd); + [DllImport("user32.dll")] public static extern uint GetWindowThreadProcessId(IntPtr hWnd, IntPtr processIdPointer); + [DllImport("kernel32.dll")] public static extern uint GetCurrentThreadId(); + [DllImport("user32.dll")] public static extern bool AttachThreadInput(uint idAttach, uint idAttachTo, bool fAttach); + [DllImport("user32.dll")] public static extern void keybd_event(byte bVk, byte bScan, uint dwFlags, UIntPtr dwExtraInfo); + public static uint GetWindowProcessId(IntPtr hWnd) { + IntPtr buffer = Marshal.AllocHGlobal(4); + try { + GetWindowThreadProcessId(hWnd, buffer); + return (uint)Marshal.ReadInt32(buffer); + } finally { + Marshal.FreeHGlobal(buffer); + } + } } "@ +# Why: SetForegroundWindow can report the switch before the shell finishes it, so poll briefly instead of a single read. +function Test-TargetForeground { + for ($attempt = 0; $attempt -lt 10; $attempt++) { + if ([Win32Interop]::GetWindowProcessId([Win32Interop]::GetForegroundWindow()) -eq {{PID}}) { return $true } + Start-Sleep -Milliseconds 50 + } + return $false +} $previous = [Win32Interop]::GetForegroundWindow() try { $process = Get-Process -Id {{PID}} -ErrorAction Stop } catch { throw 'Unity process was not found: {{PID}}' } $handle = $process.MainWindowHandle if ($handle -eq 0) { throw 'Unity process has no main window handle: {{PID}}' } -$shown = [Win32Interop]::ShowWindowAsync($handle, 9) -if (-not $shown) { throw 'Failed to show Unity window' } -$focused = [Win32Interop]::SetForegroundWindow($handle) -if (-not $focused) { - $shell = New-Object -ComObject WScript.Shell - $focused = $shell.AppActivate({{PID}}) +# Why: SW_RESTORE on a non-minimized window would shrink a maximized Unity window, so restore only when minimized. +if ([Win32Interop]::IsIconic($handle)) { + $shown = [Win32Interop]::ShowWindowAsync($handle, 9) + if (-not $shown) { throw 'Failed to show Unity window' } +} +[void][Win32Interop]::SetForegroundWindow($handle) +if (-not (Test-TargetForeground)) { + # Why: the Windows foreground lock rejects SetForegroundWindow from background processes; sharing the + # foreground thread's input queue via AttachThreadInput lifts that restriction. + $currentThreadId = [Win32Interop]::GetCurrentThreadId() + $foreground = [Win32Interop]::GetForegroundWindow() + $foregroundThreadId = [Win32Interop]::GetWindowThreadProcessId($foreground, [IntPtr]::Zero) + $targetThreadId = [Win32Interop]::GetWindowThreadProcessId($handle, [IntPtr]::Zero) + $attachedForeground = $false + $attachedTarget = $false + try { + if ($foregroundThreadId -ne 0 -and $foregroundThreadId -ne $currentThreadId) { + $attachedForeground = [Win32Interop]::AttachThreadInput($currentThreadId, $foregroundThreadId, $true) + } + if ($targetThreadId -ne 0 -and $targetThreadId -ne $currentThreadId) { + $attachedTarget = [Win32Interop]::AttachThreadInput($currentThreadId, $targetThreadId, $true) + } + [void][Win32Interop]::BringWindowToTop($handle) + [void][Win32Interop]::SetForegroundWindow($handle) + } finally { + if ($attachedTarget) { [void][Win32Interop]::AttachThreadInput($currentThreadId, $targetThreadId, $false) } + if ($attachedForeground) { [void][Win32Interop]::AttachThreadInput($currentThreadId, $foregroundThreadId, $false) } + } +} +if (-not (Test-TargetForeground)) { + # Why: a transient Alt keypress makes this process the last input source, a documented workaround + # that unlocks SetForegroundWindow when AttachThreadInput alone is not enough. + [Win32Interop]::keybd_event(0x12, 0, 0, [UIntPtr]::Zero) + [void][Win32Interop]::SetForegroundWindow($handle) + [Win32Interop]::keybd_event(0x12, 0, 2, [UIntPtr]::Zero) +} +if (-not (Test-TargetForeground)) { + throw 'Windows refused to bring the Unity window (PID: {{PID}}) to the foreground (foreground lock). Click the Unity window or its taskbar icon to focus it manually.' } -if (-not $focused) { throw 'Failed to focus Unity window' } Write-Output $previous.ToInt64() diff --git a/cli/common/unityprocess/focus_windows.go b/cli/common/unityprocess/focus_windows.go index 7591fc6b03..07e72c4da8 100644 --- a/cli/common/unityprocess/focus_windows.go +++ b/cli/common/unityprocess/focus_windows.go @@ -16,7 +16,7 @@ func FocusUnityProcess(ctx context.Context, pid int) error { command := exec.CommandContext(commandContext, windowsPowerShellCommand, "-NoProfile", "-Command", script) command.Stderr = &stderr if err := command.Run(); err != nil { - return commandErrorWithStderr(err, stderr.String()) + return focusCommandError(commandContext.Err(), err, stderr.String()) } return nil } @@ -30,7 +30,7 @@ func FocusUnityProcessWithRestore(ctx context.Context, pid int) (RestoreFocusFun command.Stderr = &stderr output, err := command.Output() if err != nil { - return nil, commandErrorWithStderr(err, stderr.String()) + return nil, focusCommandError(commandContext.Err(), err, stderr.String()) } previousHandle := parseWindowsForegroundHandle(string(output)) if previousHandle == 0 { @@ -49,7 +49,7 @@ func restoreWindowsForegroundWindow(ctx context.Context, handle int64) error { command := exec.CommandContext(commandContext, windowsPowerShellCommand, "-NoProfile", "-Command", script) command.Stderr = &stderr if err := command.Run(); err != nil { - return commandErrorWithStderr(err, stderr.String()) + return focusCommandError(commandContext.Err(), err, stderr.String()) } return nil } diff --git a/cli/common/unityprocess/process_test.go b/cli/common/unityprocess/process_test.go index f29976f524..ed92051d46 100644 --- a/cli/common/unityprocess/process_test.go +++ b/cli/common/unityprocess/process_test.go @@ -63,40 +63,57 @@ func TestExtractProjectPathSupportsEqualsAndSpaces(t *testing.T) { } } -// Verifies the embedded Windows focus script throws instead of silently returning on failures. -func TestBuildFocusUnityProcessWindowsScriptThrowsOnFailures(t *testing.T) { +// Verifies the embedded Windows focus script verifies the foreground result and throws instead of trusting API return values. +func TestBuildFocusUnityProcessWindowsScriptVerifiesForegroundAndThrowsOnFailures(t *testing.T) { script := buildFocusUnityProcessWindowsScript(123) + assertWindowsFocusScriptContract(t, script) +} + +// Verifies the embedded Windows focus-with-restore script captures the previous foreground window and shares the focus contract. +func TestBuildFocusUnityProcessWindowsWithRestoreScriptCapturesForegroundWindow(t *testing.T) { + script := buildFocusUnityProcessWindowsWithRestoreScript(123) + + assertWindowsFocusScriptContract(t, script) for _, expected := range []string{ - "throw 'Unity process was not found: 123'", - "throw 'Unity process has no main window handle: 123'", - "throw 'Failed to show Unity window'", - "$focused = $shell.AppActivate(123)", - "throw 'Failed to focus Unity window'", + "$previous = [Win32Interop]::GetForegroundWindow()", + "Write-Output $previous.ToInt64()", } { if !strings.Contains(script, expected) { t.Fatalf("script missing %q: %s", expected, script) } } - if strings.Contains(script, "catch { return }") || strings.Contains(script, "{ return }") { - t.Fatalf("script should not silently return: %s", script) - } } -// Verifies the embedded Windows focus-with-restore script captures the previous foreground window. -func TestBuildFocusUnityProcessWindowsWithRestoreScriptCapturesForegroundWindow(t *testing.T) { - script := buildFocusUnityProcessWindowsWithRestoreScript(123) - +// Asserts the shared Windows focus contract: escalation techniques, foreground verification, and no trust in AppActivate. +func assertWindowsFocusScriptContract(t *testing.T, script string) { + t.Helper() for _, expected := range []string{ - "GetForegroundWindow", - "$previous = [Win32Interop]::GetForegroundWindow()", - "Write-Output $previous.ToInt64()", - "$focused = $shell.AppActivate(123)", + "throw 'Unity process was not found: 123'", + "throw 'Unity process has no main window handle: 123'", + "if ([Win32Interop]::IsIconic($handle)) {", + "throw 'Failed to show Unity window'", + "function Test-TargetForeground", + "[Win32Interop]::GetWindowProcessId([Win32Interop]::GetForegroundWindow()) -eq 123", + "AttachThreadInput($currentThreadId, $foregroundThreadId, $true)", + "AttachThreadInput($currentThreadId, $targetThreadId, $true)", + "AttachThreadInput($currentThreadId, $targetThreadId, $false)", + "AttachThreadInput($currentThreadId, $foregroundThreadId, $false)", + "BringWindowToTop", + "keybd_event(0x12, 0, 0, [UIntPtr]::Zero)", + "keybd_event(0x12, 0, 2, [UIntPtr]::Zero)", + "throw 'Windows refused to bring the Unity window (PID: 123) to the foreground (foreground lock). Click the Unity window or its taskbar icon to focus it manually.'", } { if !strings.Contains(script, expected) { t.Fatalf("script missing %q: %s", expected, script) } } + if strings.Contains(script, "AppActivate") { + t.Fatalf("script must not trust AppActivate return values: %s", script) + } + if strings.Contains(script, "catch { return }") || strings.Contains(script, "{ return }") { + t.Fatalf("script should not silently return: %s", script) + } } // Verifies the embedded Windows restore script fails when the saved foreground window cannot be restored.