From 4784fa0087ad60ea0a2b1ece5de19496440cc4fb Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Sun, 13 Sep 2026 18:43:08 -0700 Subject: [PATCH] fix(grok): defer managed hook pane guard expansion to shell (#20534) --- src/main/agent-hooks/installer-utils.test.ts | 4 +- .../managed-hook-command-contract.test.ts | 220 ++++++++++++++++++ .../managed-hook-command-env.test-fixture.ts | 19 ++ src/main/agent-hooks/posix-hook-command.ts | 9 +- .../remote-hook-service-installers.test.ts | 2 +- src/main/grok/hook-service.test.ts | 2 +- 6 files changed, 246 insertions(+), 10 deletions(-) create mode 100644 src/main/agent-hooks/managed-hook-command-contract.test.ts create mode 100644 src/main/agent-hooks/managed-hook-command-env.test-fixture.ts diff --git a/src/main/agent-hooks/installer-utils.test.ts b/src/main/agent-hooks/installer-utils.test.ts index cbe29ee1ca1..e68e3cc3554 100644 --- a/src/main/agent-hooks/installer-utils.test.ts +++ b/src/main/agent-hooks/installer-utils.test.ts @@ -36,6 +36,7 @@ import { WINDOWS_POWERSHELL_HOOK_ENVIRONMENT_GUARD } from './hook-stdin-contract' import { wrapRuntimeHomeHookCommand } from './runtime-home-hook-command' +import { findBareHookCommandVariables } from './managed-hook-command-env.test-fixture' let tmpDir: string let configPath: string @@ -765,8 +766,7 @@ describe('wrapRuntimeHomeHookCommand', () => { const command = wrapRuntimeHomeHookCommand('claude-hook', options) expect(command).toContain('"${SYSTEMROOT-}/System32/WindowsPowerShell/v1.0/powershell.exe"') - expect(command).not.toMatch(/\$(?!\{)[A-Za-z_]/) - expect(command).not.toMatch(/\$\{[A-Za-z_][A-Za-z0-9_]*\}/) + expect(findBareHookCommandVariables(command)).toEqual([]) } ) diff --git a/src/main/agent-hooks/managed-hook-command-contract.test.ts b/src/main/agent-hooks/managed-hook-command-contract.test.ts new file mode 100644 index 00000000000..72b522e8ff5 --- /dev/null +++ b/src/main/agent-hooks/managed-hook-command-contract.test.ts @@ -0,0 +1,220 @@ +import { afterEach, describe, expect, it, vi } from 'vitest' +import { + CLAUDE_HOOK_SETTINGS, + OPENCLAUDE_HOOK_SETTINGS, + getManagedLifecycleHook, + getRemoteManagedCommand as getClaudeRemoteCommand +} from '../claude/hook-settings' +import { + getManagedCommand as getCodexCommand, + wrapReadablePosixHookCommand +} from '../codex/codex-hook-definition' +import { ANTIGRAVITY_EVENTS, ANTIGRAVITY_PRE_TOOL_USE_DECISION } from '../antigravity/hook-events' +import { CURSOR_EVENTS } from '../cursor/hook-events' +import { + getManagedCommand as getCursorCommand, + getPosixManagedCommand as getCursorRemoteCommand +} from '../cursor/hook-script' +import { + COPILOT_EVENTS, + getManagedCommand as getCopilotCommand +} from '../copilot/copilot-managed-hook-definitions' +import { getDevinManagedCommand, getDevinRemoteManagedCommand } from '../devin/hook-settings' +import { getGrokManagedCommand } from '../grok/grok-hook-script' +import { + wrapPosixHookCommand, + wrapWindowsCmdHookCommand, + wrapWindowsHookCommand +} from './installer-utils' +import { MANAGED_AGENT_HOOK_INSTALLERS } from './managed-agent-hook-registry' +import { REMOTE_MANAGED_HOOK_INSTALLER_AGENTS } from './remote-managed-hook-installers' +import { + findBareHookCommandVariables, + GROK_PROVIDED_HOOK_VARIABLES +} from './managed-hook-command-env.test-fixture' + +vi.mock('electron', () => ({ app: { getPath: () => process.cwd() } })) + +afterEach(() => vi.restoreAllMocks()) + +type CommandBuilders = { + local: (scriptPath: string) => string[] + remote: (scriptPath: string) => string[] +} + +// Why: Gemini/Droid/Command Code keep their thin builders private; exercise the wrappers they call. +const standardCommands: CommandBuilders = { + local: (path) => [ + process.platform === 'win32' ? wrapWindowsHookCommand(path) : wrapPosixHookCommand(path) + ], + remote: (path) => [wrapPosixHookCommand(path)] +} + +function antigravityPosixCommands(path: string): string[] { + return ANTIGRAVITY_EVENTS.map((event) => + wrapPosixHookCommand( + path, + { ORCA_ANTIGRAVITY_EVENT: event.eventName }, + event.eventName === 'PreToolUse' ? { fallbackStdout: ANTIGRAVITY_PRE_TOOL_USE_DECISION } : {} + ) + ) +} + +const buildersByAgent = new Map([ + [ + 'claude', + { + local: (path) => + [true, false].map( + (gitBashAvailable) => + getManagedLifecycleHook(path, CLAUDE_HOOK_SETTINGS, { gitBashAvailable }).command + ), + remote: (path) => [getClaudeRemoteCommand(path)] + } + ], + [ + 'openclaude', + { + local: (path) => [getManagedLifecycleHook(path, OPENCLAUDE_HOOK_SETTINGS).command], + remote: (path) => [getClaudeRemoteCommand(path)] + } + ], + [ + 'codex', + { + local: (path) => [getCodexCommand(path), wrapReadablePosixHookCommand(path)], + remote: (path) => [wrapPosixHookCommand(path), wrapReadablePosixHookCommand(path)] + } + ], + ['gemini', standardCommands], + [ + 'antigravity', + { + local: (path) => + process.platform === 'win32' + ? ANTIGRAVITY_EVENTS.map((event) => + wrapWindowsCmdHookCommand( + path.replace('antigravity-hook.cmd', event.windowsWrapperFileName) + ) + ) + : antigravityPosixCommands(path), + remote: antigravityPosixCommands + } + ], + [ + 'cursor', + { + local: (path) => CURSOR_EVENTS.map((event) => getCursorCommand(path, event)), + remote: (path) => CURSOR_EVENTS.map((event) => getCursorRemoteCommand(path, event)) + } + ], + ['droid', standardCommands], + ['command-code', standardCommands], + [ + 'grok', + { + local: (path) => [getGrokManagedCommand(path)], + // Why: grok-hook-remote-install.ts calls this wrapper directly, with the pane guard. + remote: (path) => [wrapPosixHookCommand(path, {}, { requiredEnvVar: 'ORCA_PANE_KEY' })] + } + ], + [ + 'copilot', + { + local: (path) => COPILOT_EVENTS.map((event) => getCopilotCommand(path, event)), + remote: (path) => + COPILOT_EVENTS.map((event) => + wrapPosixHookCommand(path, { ORCA_COPILOT_HOOK_EVENT: event }) + ) + } + ], + [ + 'devin', + { + local: (path) => [getDevinManagedCommand(path)], + remote: (path) => [getDevinRemoteManagedCommand(path)] + } + ], + [ + 'kimi', + { + local: (path) => [wrapPosixHookCommand(path.replaceAll('\\', '/'))], + remote: (path) => [wrapPosixHookCommand(path)] + } + ] +]) + +// Why: as in MANAGED_AGENT_HOOK_SCRIPT_REFRESHERS, native plugin source has no shell command to scan. +const exemptionsByAgent = new Map([ + ['amp', 'Native TypeScript plugin source; no shell hook command'], + ['hermes', 'Native Python plugin source; no shell hook command'] +]) + +describe('managed hook command contract', () => { + it.each([ + ['local', MANAGED_AGENT_HOOK_INSTALLERS.map(([agent]) => agent)], + ['remote', REMOTE_MANAGED_HOOK_INSTALLER_AGENTS] + ] as const)('covers the %s installer registry in both directions', (_target, agents) => { + // Why: mirror the remote installer coverage ratchet (#7253); a new provider cannot opt out silently. + for (const agent of agents) { + expect( + Number(buildersByAgent.has(agent)) + Number(exemptionsByAgent.has(agent)), + `${agent} needs exactly one command builder or documented native-plugin exemption` + ).toBe(1) + } + const registered = new Set(agents) + for (const agent of [...buildersByAgent.keys(), ...exemptionsByAgent.keys()]) { + expect(registered.has(agent), `${agent} is absent from the installer registry`).toBe(true) + } + }) + + describe.each(['darwin', 'linux', 'win32'] as const)('%s host', (platform) => { + it.each([...buildersByAgent])('%s emits no bare variable references', (agent, builders) => { + vi.spyOn(process, 'platform', 'get').mockReturnValue(platform) + const extension = platform === 'win32' && agent !== 'kimi' ? 'cmd' : 'sh' + const homes = + platform === 'win32' ? ['C:/Users/test', 'C:/Users/test user'] : ['/home/test user'] + const paths = homes.map((home) => `${home}/.orca/agent-hooks/${agent}-hook.${extension}`) + const commands = [ + ...paths.flatMap((path) => builders.local(path)), + ...builders.remote(`/home/remote user/.orca/agent-hooks/${agent}-hook.sh`) + ] + expect(commands.length).toBeGreaterThan(0) + for (const command of commands) { + expect(command.length).toBeGreaterThan(0) + expect(findBareHookCommandVariables(command), command).toEqual([]) + } + }) + }) +}) + +describe('Grok variable scanner contract', () => { + it.each(['$NAME', '${NAME}', "'$NAME'", "'${NAME}'", '\\$NAME', '$lower_9', '${_NAME9}'])( + 'rejects bare references without shell quoting state: %s', + (command) => expect(findBareHookCommandVariables(command)).toHaveLength(1) + ) + + it.each([ + '${NAME-}', + '${NAME:-}', + '${NAME:+}', + '${NAME#x}', + '${NAME:0:5}', + '${NAME+x}', + '${NAME=x}', + '${NAME?x}', + '${NAME%x}', + '${NAME/x/y}', + '$1', + '$$', + '$?', + '$(true)' + ])('allows modifiers and non-variable dollar forms: %s', (command) => { + expect(findBareHookCommandVariables(command)).toEqual([]) + }) + + it.each(GROK_PROVIDED_HOOK_VARIABLES)('exempts only the exact provided name %s', (name) => { + expect(findBareHookCommandVariables(`$${name} \${${name}}`)).toEqual([]) + expect(findBareHookCommandVariables(`$${name}_OTHER \${${name}_OTHER}`)).toHaveLength(2) + }) +}) diff --git a/src/main/agent-hooks/managed-hook-command-env.test-fixture.ts b/src/main/agent-hooks/managed-hook-command-env.test-fixture.ts new file mode 100644 index 00000000000..77c73534c3f --- /dev/null +++ b/src/main/agent-hooks/managed-hook-command-env.test-fixture.ts @@ -0,0 +1,19 @@ +export const GROK_PROVIDED_HOOK_VARIABLES = [ + 'GROK_HOOK_EVENT', + 'GROK_HOOK_NAME', + 'GROK_SESSION_ID', + 'GROK_WORKSPACE_ROOT', + 'CLAUDE_PROJECT_DIR' +] as const + +const providedVariables = new Set(GROK_PROVIDED_HOOK_VARIABLES) + +export function findBareHookCommandVariables(command: string): string[] { + // Why: Grok scans dollar bytes without shell quoting state, even inside single quotes. + // These are the two runtime-home assertions from installer-utils.test.ts, shared across builders. + const references = [ + ...command.matchAll(/\$(?!\{)([A-Za-z_][A-Za-z0-9_]*)/g), + ...command.matchAll(/\$\{([A-Za-z_][A-Za-z0-9_]*)\}/g) + ] + return references.filter((match) => !providedVariables.has(match[1])).map((match) => match[0]) +} diff --git a/src/main/agent-hooks/posix-hook-command.ts b/src/main/agent-hooks/posix-hook-command.ts index 728fc30bf18..23d8f1acba8 100644 --- a/src/main/agent-hooks/posix-hook-command.ts +++ b/src/main/agent-hooks/posix-hook-command.ts @@ -21,13 +21,10 @@ export function wrapPosixHookCommand( options.fallbackStdout === undefined ? POSIX_HOOK_STDIN_DRAIN_COMMAND : `printf '%s\\n' ${quotePosixShellString(options.fallbackStdout)}; ${POSIX_HOOK_STDIN_DRAIN_COMMAND}` - // Why an env guard and not just a file test: the managed script always exists, so without this - // the agent spawns a shell for it on every event and the script only then discovers Orca is not - // listening and exits. The spawn has already happened by that point, which is the whole cost a - // standalone session was paying. `requiredEnvVar` names a variable Orca sets on the panes it - // launches, so a session Orca did not start short-circuits before spawning anything. + // Why: default form avoids Grok rejecting unset vars or splicing values into shell quotes at load + // time; the child shell checks the current pane env before spawning the managed script. const guards = [ - ...(options.requiredEnvVar ? [`[ -n "$${options.requiredEnvVar}" ]`] : []), + ...(options.requiredEnvVar ? [`[ -n "\${${options.requiredEnvVar}-}" ]`] : []), `[ -f ${quoted} ]`, `[ -r ${quoted} ]`, `[ -x ${quoted} ]` diff --git a/src/main/agent-hooks/remote-hook-service-installers.test.ts b/src/main/agent-hooks/remote-hook-service-installers.test.ts index a1168326ac2..4d5e3511114 100644 --- a/src/main/agent-hooks/remote-hook-service-installers.test.ts +++ b/src/main/agent-hooks/remote-hook-service-installers.test.ts @@ -420,7 +420,7 @@ describe('remote hook service installers', () => { const definition = grokConfig.hooks[eventName]?.[0] const command = definition?.hooks?.[0]?.command expect(command).toContain('/home/dev/.orca/agent-hooks/grok-hook.sh') - expect(command).toMatch(/^if \[ -n "\$ORCA_PANE_KEY" \] && /) + expect(command).toMatch(/^if \[ -n "\$\{ORCA_PANE_KEY-\}" \] && /) } // Why: Grok tool matchers are real regexes; bare `*` is invalid match-all. expect(grokConfig.hooks.PreToolUse?.[0]?.matcher).toBe('.*') diff --git a/src/main/grok/hook-service.test.ts b/src/main/grok/hook-service.test.ts index 251ae64fa33..49fd8ae5b4e 100644 --- a/src/main/grok/hook-service.test.ts +++ b/src/main/grok/hook-service.test.ts @@ -279,7 +279,7 @@ describe('GrokHookService', () => { expect(command).toContain(join(homeDir, '.orca')) // Why: with no Orca pane in the environment the guard short-circuits, so a standalone Grok // session never spawns a shell for the managed script at all. - expect(command).toMatch(/^if \[ -n "\$ORCA_PANE_KEY" \] && /) + expect(command).toMatch(/^if \[ -n "\$\{ORCA_PANE_KEY-\}" \] && /) } const script = readFileSync(