diff --git a/src/main/browser/agent-browser-bridge.ts b/src/main/browser/agent-browser-bridge.ts index 565352d3d50..750ebd8b0a6 100644 --- a/src/main/browser/agent-browser-bridge.ts +++ b/src/main/browser/agent-browser-bridge.ts @@ -2604,7 +2604,11 @@ export class AgentBrowserBridge { child = execFile( this.agentBrowserBin, ['--session', sessionName, 'close'], - { timeout: STALE_SESSION_CLOSE_TIMEOUT_MS }, + // Why windowsHide: agent-browser is console-subsystem and Orca's main + // process owns no console, so each spawn gets a fresh visible conhost + // that takes foreground -- keystrokes typed into a terminal at that + // moment land in the black box (#14543). + { timeout: STALE_SESSION_CLOSE_TIMEOUT_MS, windowsHide: true }, (error) => finish( error @@ -2667,6 +2671,9 @@ export class AgentBrowserBridge { { timeout: execOptions?.timeoutMs ?? EXEC_TIMEOUT_MS, maxBuffer: 50 * 1024 * 1024, + // Why windowsHide: see the stale-session close above -- every + // agent-browser invocation would otherwise flash a console (#14543). + windowsHide: true, env: execOptions?.envOverrides ? { ...process.env, ...execOptions.envOverrides } : process.env diff --git a/src/main/git/runner.ts b/src/main/git/runner.ts index 0f78961815c..9f5e3346dcf 100644 --- a/src/main/git/runner.ts +++ b/src/main/git/runner.ts @@ -689,6 +689,10 @@ function execFileCapture( args, { cwd: options.cwd, + // Why: git.exe is console-subsystem and Orca's main process owns no + // console, so every spawn without this flashes a conhost that takes + // foreground. Git runs on nearly every interaction (#14543). + windowsHide: true, encoding: options.encoding, maxBuffer: options.maxBuffer ?? DEFAULT_GIT_MAX_BUFFER, env: options.env @@ -1451,7 +1455,8 @@ export function gitExecFileSync( encoding: options.encoding ?? 'utf-8', env: untranslatedGitOutputEnv(), stdio: options.stdio ?? ['pipe', 'pipe', 'pipe'], - timeout: options.timeout ?? GIT_EXEC_SYNC_TIMEOUT_MS + timeout: options.timeout ?? GIT_EXEC_SYNC_TIMEOUT_MS, + windowsHide: true }) as string } finally { // Sync exec blocks the main thread for its whole duration — the cost issue #7576 flags. @@ -1496,6 +1501,7 @@ export function gitSpawn(args: string[], options: GitSpawnOptions): ChildProcess const child = spawn(resolved.binary, resolved.args, { ...spawnOptions, env: untranslatedGitOutputEnv(spawnOptions.env ?? process.env), + windowsHide: true, cwd: resolved.cwd }) recordSubprocessSpawn(resolved.binary, resolved.args, performance.now() - spawnStartedAt) @@ -1984,6 +1990,7 @@ export function wslAwareSpawn( const spawnStartedAt = performance.now() const child = spawn(resolved.binary, resolved.args, { ...spawnOptions, + windowsHide: true, cwd: resolved.cwd }) recordSubprocessSpawn(resolved.binary, resolved.args, performance.now() - spawnStartedAt) diff --git a/src/shared/child-process/__fixtures__/windows-console-visibility-allowlist.txt b/src/shared/child-process/__fixtures__/windows-console-visibility-allowlist.txt new file mode 100644 index 00000000000..32537dc211c --- /dev/null +++ b/src/shared/child-process/__fixtures__/windows-console-visibility-allowlist.txt @@ -0,0 +1,79 @@ +# Files with a direct child-process call that does not pass `windowsHide`. +# Enforced by ../windows-console-visibility.test.ts. This list only shrinks: +# it is the migration worklist for routing these calls through run-process.ts. +cli/handlers/account.ts +cli/handlers/core.ts +cli/handlers/skills.ts +main/agent-hooks/managed-hook-owner-identity.ts +main/app-icon.ts +main/automations/external-manager.ts +main/automations/precheck-runner.ts +main/browser/browser-cookie-import.ts +main/claude-accounts/keychain.ts +main/claude-accounts/runtime-auth-service.ts +main/claude-accounts/service.ts +main/cli/cli-installer.ts +main/cli/wsl-cli-installer.ts +main/codex-accounts/runtime-home-service.ts +main/codex-accounts/service.ts +main/computer/macos-computer-use-permission-status.ts +main/computer/macos-computer-use-permissions.ts +main/computer/macos-native-provider-transport.ts +main/daemon/daemon-bash-wrapper-osc133-fixture.ts +main/daemon/daemon-process-identity-query.ts +main/daemon/daemon-process-start-time.ts +main/emulator/android/android-command-runner.ts +main/emulator/android/scrcpy-stream-session.ts +main/emulator/serve-sim-helper-processes.ts +main/emulator/serve-sim-runtime-materializer.ts +main/emulator/simctl-simulator-devices.ts +main/emulator/simulator-app-visibility.ts +main/hooks.ts +main/ipc/app.ts +main/ipc/developer-permissions.ts +main/ipc/filesystem.ts +main/ipc/macos-keyboard-layout-snapshot.ts +main/ipc/notebook.ts +main/ipc/notification-authorization-status.ts +main/ipc/repos.ts +main/ipc/worktree-apfs-clone.ts +main/local-builds/local-build-candidate.ts +main/macos-tcc-prompt-watch.ts +main/network/macos-system-resolver-health.ts +main/network/macos-tailscale-dns-diagnostic.ts +main/pi/agent-status-wsl-curl-source.ts +main/providers/macos-login-session-pty-probe.ts +main/pty-descendant-termination.ts +main/pty/posix-pty-foreground-group.ts +main/pty/posix-pty-process-groups.ts +main/pty/windows-environment-path.ts +main/rate-limits/codex-fetcher.ts +main/runtime/orca-runtime-files.ts +main/runtime/tls-certificate.ts +main/ssh/ssh-connection.ts +main/startup/appimage-cli-redirect.ts +main/startup/ensure-virtual-display.ts +main/startup/packaged-cli-entry-redirect.ts +main/startup/windows-install-dir-acl-probe.ts +main/win32-utils.ts +main/window/clipboard-ipc-handlers.ts +main/workspace-space-analysis.ts +main/wsl-availability.ts +main/wsl-unc-delete.ts +main/wsl.ts +main/zsh-startup-hook-pty-harness.ts +relay/agent-exec-handler.ts +relay/external-automations-handler.ts +relay/fs-handler-git-fallback.ts +relay/fs-handler-utils.ts +relay/fs-list-files-fallback-chain.ts +relay/git-handler.ts +relay/pty-shell-utils.ts +relay/subprocess-tree-termination.ts +relay/windows-port-scan.ts +relay/workspace-space-scan.ts +shared/fish-binary-requirement.ts +shared/process-table-snapshot.ts +shared/pty-slave-line-discipline-echo.ts +shared/ripgrep-process-availability.ts +shared/shell-process-readiness.ts diff --git a/src/shared/child-process/windows-console-visibility.test.ts b/src/shared/child-process/windows-console-visibility.test.ts new file mode 100644 index 00000000000..984b6e998bc --- /dev/null +++ b/src/shared/child-process/windows-console-visibility.test.ts @@ -0,0 +1,132 @@ +import { readFileSync, readdirSync, statSync } from 'node:fs' +import { join, relative, resolve } from 'node:path' +import { describe, expect, it } from 'vitest' + +/** + * Every direct child-process call must pass `windowsHide`. + * + * Without it a GUI Electron process that spawns a console subsystem binary gets + * a real console window: it flashes, and it steals foreground. On a git status + * poll that is once per poll (#10488). `run-process.ts` sets the flag for + * everything routed through it; this guards the calls that still spawn directly. + * + * The flag is inert off Windows, so this asks for it unconditionally rather than + * making each site reason about its platform. + * + * The allowlist only shrinks — it is also the migration worklist. Removing a + * file from it means either adding the flag or, better, routing the call + * through the chokepoint. + */ +const ALLOWLIST: readonly string[] = readFileSync( + join(__dirname, '__fixtures__', 'windows-console-visibility-allowlist.txt'), + 'utf8' +) + .split('\n') + .map((line) => line.trim()) + .filter((line) => line.length > 0 && !line.startsWith('#')) + +const CHILD_PROCESS_IMPORT = + /from\s+['"](?:node:)?child_process['"]|require\(\s*['"](?:node:)?child_process['"]/ +const SPAWN_CALL = /\b(?:spawn|spawnSync|execFile|execFileSync|exec|execSync)\s*\(/g +const IGNORED_DIRECTORIES = new Set([ + 'node_modules', + 'dist', + 'out', + 'build', + '.git', + '__fixtures__' +]) +const SOURCE_ROOT = resolve(__dirname, '../..') + +/** + * Blank out comments before scanning. + * + * Why: `runner.ts` documents its wrapper as `execFileSync('git', args, {...})` + * in a doc comment. Counted as a call it can never be satisfied, so the file + * would sit on the allowlist forever and a genuine regression in it would be + * pre-approved -- a ratchet that only looks like one. + */ +function stripComments(source: string): string { + return source.replace(/\/\*[\s\S]*?\*\//g, ' ').replace(/(^|[^:])\/\/[^\n]*/g, '$1') +} + +function isTestFile(path: string): boolean { + return ( + /\.(?:test|spec)\.tsx?$/.test(path) || + /(?:test-harness|test-utils|test-setup|test-fixture|repro)/.test(path) || + path.includes('/__tests__/') + ) +} + +function collectSourceFiles(root: string): string[] { + const found: string[] = [] + for (const entry of readdirSync(root)) { + if (IGNORED_DIRECTORIES.has(entry)) { + continue + } + const path = join(root, entry) + if (statSync(path).isDirectory()) { + found.push(...collectSourceFiles(path)) + continue + } + if (/\.tsx?$/.test(entry)) { + found.push(path) + } + } + return found +} + +/** The call's argument text, brace-matched so a nested options literal stays whole. */ +function readCallArguments(source: string, openParenIndex: number): string { + let depth = 0 + for (let index = openParenIndex; index < source.length; index += 1) { + if (source[index] === '(') { + depth += 1 + } else if (source[index] === ')') { + depth -= 1 + if (depth === 0) { + return source.slice(openParenIndex, index) + } + } + } + return source.slice(openParenIndex) +} + +function findOffenders(): string[] { + const offenders = new Set() + for (const path of collectSourceFiles(SOURCE_ROOT)) { + const relativePath = relative(SOURCE_ROOT, path).replace(/\\/g, '/') + if (isTestFile(relativePath)) { + continue + } + const source = stripComments(readFileSync(path, 'utf8')) + if (!CHILD_PROCESS_IMPORT.test(source)) { + continue + } + for (const match of source.matchAll(SPAWN_CALL)) { + if (!readCallArguments(source, match.index + match[0].length - 1).includes('windowsHide')) { + offenders.add(relativePath) + } + } + } + return [...offenders].sort() +} + +describe('direct child-process calls hide the Windows console', () => { + const offenders = findOffenders() + + it('scans a realistic number of files', () => { + // Guards against an import-pattern change quietly emptying the scan, which + // would make every assertion below pass without checking anything. + expect(offenders.length + ALLOWLIST.length).toBeGreaterThan(50) + }) + + it('adds no new file that spawns without windowsHide', () => { + expect(offenders.filter((path) => !ALLOWLIST.includes(path))).toEqual([]) + }) + + it('carries no stale allowlist entry', () => { + // A fixed file must leave the list, or the ratchet stops ratcheting. + expect(ALLOWLIST.filter((path) => !offenders.includes(path))).toEqual([]) + }) +}) diff --git a/src/shared/wsl-exec-mode-separator.test.ts b/src/shared/wsl-exec-mode-separator.test.ts index 69028f9aa98..51e4efbe633 100644 --- a/src/shared/wsl-exec-mode-separator.test.ts +++ b/src/shared/wsl-exec-mode-separator.test.ts @@ -21,6 +21,11 @@ const STRING_FORM = new RegExp(String.raw`wsl(?:\.exe)?\b[^\n]*?[^-]--\s+${GUEST const SCANNED_ROOTS = ['src', 'config', 'tests'] const SCANNED_EXTENSIONS = ['.ts', '.tsx', '.mjs', '.js'] const IGNORED_DIRECTORIES = new Set(['node_modules', 'dist', 'out', 'build', '.git']) +// Why: the cross-version e2e lane checks whole historical releases out under +// tests/e2e/.cross-version-checkouts/. Those are shipped code we cannot edit, so +// scanning them made this guard fail on every machine that had run that lane -- +// 21 "offenders", all of them copies of a past release. +const IGNORED_DIRECTORY_PREFIX = '.' function collectSourceFiles(root: string): string[] { let found: string[] = [] @@ -31,7 +36,7 @@ function collectSourceFiles(root: string): string[] { return found } for (const entry of entries) { - if (IGNORED_DIRECTORIES.has(entry)) { + if (IGNORED_DIRECTORIES.has(entry) || entry.startsWith(IGNORED_DIRECTORY_PREFIX)) { continue } const full = join(root, entry)