From db6e1775d5386f25d20afb6abc92451f0fda4c19 Mon Sep 17 00:00:00 2001 From: OrcaWin Date: Thu, 20 Aug 2026 19:11:51 -0700 Subject: [PATCH] fix(grok): stop the Windows status hook from spawning two interpreters per event (#15595) --- src/main/grok/hook-service.test.ts | 19 +- src/main/grok/hook-service.ts | 18 +- .../grok/windows-hook-launcher-chain.test.ts | 190 ++++++++++++++++++ 3 files changed, 215 insertions(+), 12 deletions(-) create mode 100644 src/main/grok/windows-hook-launcher-chain.test.ts diff --git a/src/main/grok/hook-service.test.ts b/src/main/grok/hook-service.test.ts index f08779aed02..57e6da47368 100644 --- a/src/main/grok/hook-service.test.ts +++ b/src/main/grok/hook-service.test.ts @@ -25,6 +25,13 @@ const GROK_SCRIPT_FILE_NAME = process.platform === 'win32' ? 'grok-hook.cmd' : ' const WINDOWS_POWERSHELL_LAUNCHER = /^[A-Za-z]:\/[^"]*\/System32\/WindowsPowerShell\/v1\.0\/powershell\.exe -NoProfile -ExecutionPolicy Bypass -WindowStyle Hidden -EncodedCommand \S+$/ +// Why (#14828): Windows registers the bare script path when it is cmd-safe and only falls back +// to the encoded launcher for a profile path that is not (#6078). windows-hook-launcher-chain +// .test.ts pins which branch applies; these cases only care that Orca's hook is present. +function registersManagedGrokScript(command: string): boolean { + return command.includes(GROK_SCRIPT_FILE_NAME) || WINDOWS_POWERSHELL_LAUNCHER.test(command) +} + type WindowsGrokHookRun = { status: number | null stderr: string @@ -253,9 +260,7 @@ describe('GrokHookService', () => { // Why: build the invalid pattern at runtime so static lint does not flag it. const bareStar = ['*', ''].join('') expect(() => new RegExp(bareStar)).toThrow() - expect(config.hooks.PreToolUse[0].hooks[0].command).toMatch( - process.platform === 'win32' ? WINDOWS_POWERSHELL_LAUNCHER : /grok-hook/ - ) + expect(registersManagedGrokScript(config.hooks.PreToolUse[0].hooks[0].command)).toBe(true) if (process.platform !== 'win32') { expect(config.hooks.PreToolUse[0].hooks[0].command).toContain(join(homeDir, '.orca')) } @@ -364,12 +369,6 @@ describe('GrokHookService', () => { definition.hooks.map((hook) => hook.command) ) expect(commands).toContain('/usr/local/bin/user-hook') - expect( - commands.some((command) => - process.platform === 'win32' - ? WINDOWS_POWERSHELL_LAUNCHER.test(command) - : command.includes(GROK_SCRIPT_FILE_NAME) - ) - ).toBe(true) + expect(commands.some(registersManagedGrokScript)).toBe(true) }) }) diff --git a/src/main/grok/hook-service.ts b/src/main/grok/hook-service.ts index 1b6e8bba165..41e1550fe28 100644 --- a/src/main/grok/hook-service.ts +++ b/src/main/grok/hook-service.ts @@ -9,7 +9,7 @@ import { readHooksJson, removeManagedCommands, wrapPosixHookCommand, - wrapWindowsHookCommand, + wrapWindowsCmdHookCommand, writeHooksJson, writeManagedScript, type HookDefinition @@ -102,11 +102,25 @@ function getManagedScriptPath(): string { } function getManagedCommand(scriptPath: string): string { + // Why (#14828): Grok runs a hook command containing a space as `pwsh -Command `, so the + // encoded PowerShell launcher cost two interpreters before reaching the script — + // `grok.exe -> pwsh.exe -> powershell.exe -> cmd.exe`, ~610ms per event, and the agent's hook + // console stays up for all of it. A cmd-safe bare path is spawned directly + // (`grok.exe -> cmd.exe`, ~110ms), the same shape Codex/Devin/Antigravity already register + // (#8430). Paths that are not cmd-safe still fall back to the encoded launcher (#6078). + // Tradeoff, as for those agents: a bare path cannot carry the launcher's missing-script + // guard, so a deleted script surfaces as a per-event `command not found` in the agent's log + // instead of a silent drain. Grok fails open — the tool still runs, including on PreToolUse. return process.platform === 'win32' - ? wrapWindowsHookCommand(scriptPath) + ? wrapWindowsCmdHookCommand(scriptPath) : wrapPosixHookCommand(scriptPath) } +/** Test seam: the command registered for `scriptPath` on the current platform. */ +export function getManagedCommandForTests(scriptPath: string): string { + return getManagedCommand(scriptPath) +} + function getManagedScript(target: 'local' | 'posix' = 'local'): string { if (target === 'local' && process.platform === 'win32') { return buildWindowsGrokHookScript() diff --git a/src/main/grok/windows-hook-launcher-chain.test.ts b/src/main/grok/windows-hook-launcher-chain.test.ts new file mode 100644 index 00000000000..ee503bd27a7 --- /dev/null +++ b/src/main/grok/windows-hook-launcher-chain.test.ts @@ -0,0 +1,190 @@ +// Why (#14828): Grok runs a hook `command` containing a space as `pwsh -Command `, so the +// encoded PowerShell launcher put two interpreters between the agent and the script — +// `grok.exe -> pwsh.exe -> powershell.exe -> cmd.exe` — and the console Grok allocates for a +// hook stays up for the whole chain. Measured against Grok 1.0.5's own dispatcher timing, the +// same script costs ~610ms behind the launcher and ~110ms as a bare path. These assertions pin +// the launch shape; the payload/exit-code contract stays covered by hook-service.test.ts. +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { existsSync, mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs' +import { tmpdir } from 'node:os' +import { dirname, join } from 'node:path' + +const { homedirMock } = vi.hoisted(() => ({ + homedirMock: vi.fn<() => string>() +})) + +vi.mock('os', async () => { + const actual = (await vi.importActual('os')) as Record + return { + ...actual, + homedir: homedirMock + } +}) + +import { getManagedCommandForTests, GrokHookService } from './hook-service' +import { wrapWindowsHookCommand } from '../agent-hooks/installer-utils' + +const GROK_EVENT_NAMES = [ + 'SessionStart', + 'UserPromptSubmit', + 'Stop', + 'StopFailure', + 'SessionEnd', + 'PreToolUse', + 'PostToolUse', + 'PostToolUseFailure', + 'Notification' +] as const + +type InstalledConfig = { + hooks: Record +} + +// Why: generate under a mocked win32 so the POSIX CI legs guard this too (#15117). +function withWin32(run: () => T): T { + const original = Object.getOwnPropertyDescriptor(process, 'platform') + Object.defineProperty(process, 'platform', { configurable: true, value: 'win32' }) + try { + return run() + } finally { + if (original) { + Object.defineProperty(process, 'platform', original) + } + } +} + +function readInstalledConfig(home: string): InstalledConfig { + return JSON.parse( + readFileSync(join(home, '.grok', 'hooks', 'orca-status.json'), 'utf8') + ) as InstalledConfig +} + +function registeredCommands(config: InstalledConfig): string[] { + return GROK_EVENT_NAMES.flatMap((eventName) => + (config.hooks[eventName] ?? []).flatMap((definition) => + definition.hooks.map((hook) => hook.command) + ) + ) +} + +describe('Windows Grok managed hook launch shape', () => { + // Why: only `process.platform` can be mocked, not `path.join`, so an install on a POSIX + // runner produces a `/`-separated path that is cmd-safe by no definition and routes to the + // #6078 fallback by design. These synthetic-path cases carry the invariant on every CI leg; + // the install-based ones below re-check it against a real generated path on Windows. + describe('registered command, from a synthetic Windows path', () => { + const scriptPath = 'C:\\Users\\dev\\.orca\\agent-hooks\\grok-hook.cmd' + + it('registers a cmd-safe script path as the command itself (#14828)', () => { + const command = withWin32(() => getManagedCommandForTests(scriptPath)) + + // Why (#8430): Grok spawns this as argv[0], so it must be exactly one spawnable token. + expect(command).toBe(scriptPath) + // Why: whitespace is the sole trigger for Grok's `pwsh -Command` wrapper, which is the + // interpreter this fix removes. Assert the trigger, not just the absence of the name. + expect(command, 'a space would reintroduce the pwsh wrapper').not.toMatch(/\s/) + expect(command).not.toMatch(/powershell|pwsh/i) + expect(command).not.toMatch(/-EncodedCommand/i) + }) + + it('still wraps a path cmd.exe would split or expand (#6078)', () => { + const spaced = 'C:\\Users\\Jane Doe\\.orca\\agent-hooks\\grok-hook.cmd' + const command = withWin32(() => getManagedCommandForTests(spaced)) + + expect(command).toMatch(/-EncodedCommand \S+$/) + // Why: the raw path must never reach a cmd.exe command line unquoted. + expect(command).not.toContain(spaced) + }) + }) + + describe('installed config', () => { + let home = '' + + beforeEach(() => { + home = mkdtempSync(join(tmpdir(), 'orca-grok-launcher-')) + homedirMock.mockReturnValue(home) + }) + + afterEach(() => { + vi.clearAllMocks() + rmSync(home, { recursive: true, force: true }) + home = '' + }) + + // win32-only: a real cmd-safe managed path needs backslashes; see the note above. + it.skipIf(process.platform !== 'win32')( + 'writes the generated script path itself to every managed event', + () => { + const scriptPath = join(home, '.orca', 'agent-hooks', 'grok-hook.cmd') + const commands = withWin32(() => { + expect(new GrokHookService().install().state).toBe('installed') + return registeredCommands(readInstalledConfig(home)) + }) + + expect(commands).toHaveLength(GROK_EVENT_NAMES.length) + for (const command of commands) { + expect(command).toBe(scriptPath) + expect(existsSync(command), 'registered command must name a real file').toBe(true) + } + } + ) + + it.skipIf(process.platform !== 'win32')( + 'replaces a previously installed encoded-PowerShell entry on reinstall', + () => { + const scriptPath = join(home, '.orca', 'agent-hooks', 'grok-hook.cmd') + const staleCommand = wrapWindowsHookCommand(scriptPath) + const configPath = join(home, '.grok', 'hooks', 'orca-status.json') + mkdirSync(dirname(configPath), { recursive: true }) + writeFileSync( + configPath, + `${JSON.stringify({ + hooks: { + Stop: [{ hooks: [{ type: 'command', command: staleCommand, timeout: 10 }] }], + // Why: a retired event must be swept too, or the old launcher keeps firing there. + SubagentStop: [{ hooks: [{ type: 'command', command: staleCommand, timeout: 10 }] }], + Notification: [{ hooks: [{ type: 'command', command: '/usr/local/bin/user-hook' }] }] + } + })}\n`, + 'utf8' + ) + + const config = withWin32(() => { + expect(new GrokHookService().install().state).toBe('installed') + return readInstalledConfig(home) + }) + + const all = Object.values(config.hooks).flatMap((definitions) => + definitions.flatMap((definition) => definition.hooks.map((hook) => hook.command)) + ) + expect(all).not.toContain(staleCommand) + expect(all.filter((command) => /-EncodedCommand/i.test(command))).toEqual([]) + expect(config.hooks.SubagentStop).toBeUndefined() + // Why: sweeping stale Orca entries must not touch hooks the user wrote. + expect(all).toContain('/usr/local/bin/user-hook') + expect(all.filter((command) => command === scriptPath)).toHaveLength( + GROK_EVENT_NAMES.length + ) + } + ) + + it('falls back to the encoded launcher when the profile path is not cmd-safe (#6078)', () => { + const spaceHome = mkdtempSync(join(tmpdir(), 'orca grok spaced ')) + homedirMock.mockReturnValue(spaceHome) + try { + const commands = withWin32(() => { + expect(new GrokHookService().install().state).toBe('installed') + return registeredCommands(readInstalledConfig(spaceHome)) + }) + + expect(commands.length).toBeGreaterThan(0) + for (const command of commands) { + expect(command).toMatch(/-EncodedCommand \S+$/) + expect(command).not.toContain(spaceHome) + } + } finally { + rmSync(spaceHome, { recursive: true, force: true }) + } + }) + }) +})