mirror of
https://github.com/stablyai/orca.git
synced 2026-10-23 16:03:49 +00:00
fix(grok): stop the Windows status hook from spawning two interpreters per event (#15595)
This commit is contained in:
@@ -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)
|
||||
})
|
||||
})
|
||||
|
||||
@@ -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 <cmd>`, 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()
|
||||
|
||||
@@ -0,0 +1,190 @@
|
||||
// Why (#14828): Grok runs a hook `command` containing a space as `pwsh -Command <cmd>`, 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<string, unknown>
|
||||
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<string, { matcher?: string; hooks: { command: string }[] }[]>
|
||||
}
|
||||
|
||||
// Why: generate under a mocked win32 so the POSIX CI legs guard this too (#15117).
|
||||
function withWin32<T>(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 })
|
||||
}
|
||||
})
|
||||
})
|
||||
})
|
||||
Reference in New Issue
Block a user