mirror of
https://github.com/stablyai/orca.git
synced 2026-09-22 08:02:28 +00:00
fix(win32): hide the console window for agent-browser and git helpers (#15887)
* fix(win32): hide the console window for agent-browser and git helpers W1 routed most child processes through `runProcess`, which always sets `windowsHide`. Six call sites still spawn directly, so each one opens a real console window on Windows: it flashes and steals foreground. For the git status poll, that is once per poll (#10488). A ratchet now scans every file that imports `child_process` and fails on a call without the flag. Its allowlist starts at the 76 files that still offend and can only shrink — it doubles as the worklist for routing them through the chokepoint, which is where the flag stops being a per-call-site decision at all. Diagnosed in #14589; the SSH and cookie-import sites it also covered are already fixed on main by the W1 migration. Co-authored-by: OrcaWin <orcawin@users.noreply.github.com> * test(wsl): stop the exec-mode guard scanning historical release checkouts The cross-version e2e lane checks whole past releases out under `tests/e2e/.cross-version-checkouts/`. The guard walked into them, so on any machine that had run that lane it reported 21 offenders -- every one a copy of shipped code we cannot edit -- and failed. Skip dot-directories; the >500-file vacuity assertion still holds. --------- Co-authored-by: OrcaWin <orcawin@users.noreply.github.com>
This commit is contained in:
@@ -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
|
||||
|
||||
@@ -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)
|
||||
|
||||
@@ -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
|
||||
@@ -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<string>()
|
||||
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([])
|
||||
})
|
||||
})
|
||||
@@ -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)
|
||||
|
||||
Reference in New Issue
Block a user