From 95862116aab4dbaffc21353e151571288e52080c Mon Sep 17 00:00:00 2001 From: Jinwoo Hong <73622457+Jinwoo-H@users.noreply.github.com> Date: Thu, 28 May 2026 17:10:57 -0400 Subject: [PATCH] fix: use Gemini BeforeTool hook for status (#2922) * fix: disable Gemini PreToolUse hook install * fix: use Gemini BeforeTool hook for status - install Gemini's native BeforeTool hook instead of invalid PreToolUse\n- keep sweeping stale managed PreToolUse entries during reinstall\n- normalize Gemini BeforeTool payloads into in-flight tool status Co-authored-by: Orca * fix: cover Gemini hook migration for remote installs Co-authored-by: Orca * test: cover local Gemini hook sweep preservation Co-authored-by: Orca --------- Co-authored-by: ramzi Co-authored-by: Orca --- .../remote-hook-service-installers.test.ts | 51 +++++- src/main/agent-hooks/server.test.ts | 8 +- src/main/gemini/hook-service.test.ts | 152 ++++++++++++++++++ src/main/gemini/hook-service.ts | 49 +++++- src/shared/agent-hook-listener.test.ts | 21 +++ src/shared/agent-hook-listener.ts | 10 +- 6 files changed, 279 insertions(+), 12 deletions(-) create mode 100644 src/main/gemini/hook-service.test.ts 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 d87b93b5884..86eb35a9f83 100644 --- a/src/main/agent-hooks/remote-hook-service-installers.test.ts +++ b/src/main/agent-hooks/remote-hook-service-installers.test.ts @@ -228,11 +228,12 @@ describe('remote hook service installers', () => { const geminiConfig = JSON.parse(gemini.fs.files.get('/home/dev/.gemini/settings.json')!) as { hooks: Record } - for (const eventName of ['BeforeAgent', 'AfterAgent', 'AfterTool', 'PreToolUse']) { + for (const eventName of ['BeforeAgent', 'AfterAgent', 'AfterTool', 'BeforeTool']) { const command = geminiConfig.hooks[eventName]?.[0]?.hooks?.[0]?.command expect(command).toContain('/home/dev/.orca/agent-hooks/gemini-hook.sh') expect(command).toMatch(/^if \[ -x /) } + expect(geminiConfig.hooks.PreToolUse).toBeUndefined() const antigravityConfig = JSON.parse( antigravity.fs.files.get('/home/dev/.gemini/config/hooks.json')! @@ -361,6 +362,54 @@ describe('remote hook service installers', () => { expect(postToolCommands.some((command) => command.includes('antigravity-hook.sh'))).toBe(true) }) + it('removes stale remote Gemini PreToolUse hooks while preserving user-authored hooks', async () => { + const { sftp, fs } = createFakeSftp() + fs.files.set( + '/home/dev/.gemini/settings.json', + `${JSON.stringify( + { + hooks: { + PreToolUse: [ + { + hooks: [ + { + type: 'command', + command: + "if [ -x '/tmp/old/agent-hooks/gemini-hook.sh' ]; then /bin/sh '/tmp/old/agent-hooks/gemini-hook.sh'; fi" + } + ] + }, + { + hooks: [ + { + type: 'command', + command: 'echo user-authored' + } + ] + } + ] + } + }, + null, + 2 + )}\n` + ) + + await new GeminiHookService().installRemote(sftp, '/home/dev') + + const config = JSON.parse(fs.files.get('/home/dev/.gemini/settings.json')!) as { + hooks: Record + } + const preToolCommands = config.hooks.PreToolUse.flatMap((definition) => + (definition.hooks ?? []).map((hook) => hook.command) + ) + expect(preToolCommands).toEqual(['echo user-authored']) + const beforeToolCommands = config.hooks.BeforeTool.flatMap((definition) => + (definition.hooks ?? []).map((hook) => hook.command) + ) + expect(beforeToolCommands.some((command) => command.includes('gemini-hook.sh'))).toBe(true) + }) + it('installs remote Copilot hooks under the user-level hooks directory', async () => { const { sftp, fs } = createFakeSftp() fs.dirs.add('/home/dev/.copilot') diff --git a/src/main/agent-hooks/server.test.ts b/src/main/agent-hooks/server.test.ts index 5c265cdbf2e..3f4c2d6449f 100644 --- a/src/main/agent-hooks/server.test.ts +++ b/src/main/agent-hooks/server.test.ts @@ -2655,11 +2655,11 @@ describe('Codex hook normalization', () => { }) describe('Gemini hook normalization', () => { - it('PreToolUse surfaces toolName + toolInput', () => { + it('BeforeTool surfaces toolName + toolInput', () => { const result = _internals.normalizeHookPayload( 'gemini', buildBody({ - hook_event_name: 'PreToolUse', + hook_event_name: 'BeforeTool', tool_name: 'read_file', tool_input: { path: '/src/index.ts' } }), @@ -2674,7 +2674,7 @@ describe('Gemini hook normalization', () => { const result = _internals.normalizeHookPayload( 'gemini', buildBody({ - hook_event_name: 'PreToolUse', + hook_event_name: 'BeforeTool', tool_name: 'run_shell_command', args: { command: 'git status' } }), @@ -2688,7 +2688,7 @@ describe('Gemini hook normalization', () => { _internals.normalizeHookPayload( 'gemini', buildBody({ - hook_event_name: 'PreToolUse', + hook_event_name: 'BeforeTool', tool_name: 'read_file', tool_input: { path: '/stale.ts' } }), diff --git a/src/main/gemini/hook-service.test.ts b/src/main/gemini/hook-service.test.ts new file mode 100644 index 00000000000..7490a1b8434 --- /dev/null +++ b/src/main/gemini/hook-service.test.ts @@ -0,0 +1,152 @@ +import { afterAll, beforeAll, describe, expect, it, vi } from 'vitest' +import { mkdtempSync, mkdirSync, readFileSync, rmSync, writeFileSync } from 'fs' +import { tmpdir } from 'os' +import { join } from 'path' +import type * as osModule from 'os' + +const { getPathMock, homedirMock } = vi.hoisted(() => ({ + getPathMock: vi.fn<(name: string) => string>(), + homedirMock: vi.fn<() => string>() +})) + +vi.mock('electron', () => ({ + app: { + getPath: getPathMock + } +})) + +vi.mock('os', async (importOriginal) => { + const actual = await importOriginal() + return { + ...actual, + homedir: homedirMock + } +}) + +import { GeminiHookService } from './hook-service' + +describe('GeminiHookService', () => { + let homeDir: string + let userDataDir: string + + beforeAll(() => { + homeDir = mkdtempSync(join(tmpdir(), 'orca-gemini-home-')) + userDataDir = mkdtempSync(join(tmpdir(), 'orca-gemini-userdata-')) + homedirMock.mockReturnValue(homeDir) + getPathMock.mockImplementation((name: string) => { + if (name === 'userData') { + return userDataDir + } + throw new Error(`unexpected getPath(${name})`) + }) + }) + + afterAll(() => { + rmSync(homeDir, { recursive: true, force: true }) + rmSync(userDataDir, { recursive: true, force: true }) + }) + + it('removes stale PreToolUse hooks when reinstalling managed Gemini hooks', () => { + const managedHookFileName = process.platform === 'win32' ? 'gemini-hook.cmd' : 'gemini-hook.sh' + const staleManagedHookPath = + process.platform === 'win32' + ? `C:\\Users\\ramzi\\.orca\\agent-hooks\\${managedHookFileName}` + : `/Users/ramzi/.orca/agent-hooks/${managedHookFileName}` + const staleManagedCommand = + process.platform === 'win32' + ? staleManagedHookPath + : `if [ -x '${staleManagedHookPath}' ]; then /bin/sh '${staleManagedHookPath}'; fi` + const managedHookPath = join(homeDir, '.orca', 'agent-hooks', managedHookFileName) + const configDir = join(homeDir, '.gemini') + mkdirSync(configDir, { recursive: true }) + writeFileSync( + join(configDir, 'settings.json'), + JSON.stringify( + { + hooks: { + BeforeAgent: [ + { + hooks: [{ type: 'command', command: 'echo user-before-agent' }] + } + ], + PreToolUse: [ + { + hooks: [ + { + type: 'command', + command: staleManagedCommand + } + ] + } + ] + } + }, + null, + 2 + ) + ) + + const service = new GeminiHookService() + const status = service.install() + const config = JSON.parse(readFileSync(join(configDir, 'settings.json'), 'utf8')) + + expect(status.state).toBe('installed') + expect(Object.keys(config.hooks).sort()).toEqual([ + 'AfterAgent', + 'AfterTool', + 'BeforeAgent', + 'BeforeTool' + ]) + expect(config.hooks.PreToolUse).toBeUndefined() + expect(config.hooks.BeforeAgent).toHaveLength(2) + expect(config.hooks.BeforeAgent[0].hooks[0].command).toBe('echo user-before-agent') + expect(config.hooks.BeforeAgent[1].hooks[0].command).toContain(managedHookPath) + expect(config.hooks.AfterAgent[0].hooks[0].command).toContain(managedHookPath) + expect(config.hooks.AfterTool[0].hooks[0].command).toContain(managedHookPath) + expect(config.hooks.BeforeTool[0].hooks[0].command).toContain(managedHookPath) + }) + + it('preserves user-authored PreToolUse hooks while sweeping stale managed Gemini hooks', () => { + const managedHookFileName = process.platform === 'win32' ? 'gemini-hook.cmd' : 'gemini-hook.sh' + const staleManagedHookPath = + process.platform === 'win32' + ? `C:\\Users\\ramzi\\.orca\\agent-hooks\\${managedHookFileName}` + : `/Users/ramzi/.orca/agent-hooks/${managedHookFileName}` + const staleManagedCommand = + process.platform === 'win32' + ? staleManagedHookPath + : `if [ -x '${staleManagedHookPath}' ]; then /bin/sh '${staleManagedHookPath}'; fi` + const configDir = join(homeDir, '.gemini') + mkdirSync(configDir, { recursive: true }) + writeFileSync( + join(configDir, 'settings.json'), + JSON.stringify( + { + hooks: { + PreToolUse: [ + { + hooks: [{ type: 'command', command: staleManagedCommand }] + }, + { + hooks: [{ type: 'command', command: 'echo user-authored' }] + } + ] + } + }, + null, + 2 + ) + ) + + const status = new GeminiHookService().install() + const config = JSON.parse(readFileSync(join(configDir, 'settings.json'), 'utf8')) + const preToolCommands = config.hooks.PreToolUse.flatMap( + (definition: { hooks?: { command: string }[] }) => + (definition.hooks ?? []).map((hook) => hook.command) + ) + + expect(status.state).toBe('installed') + expect(preToolCommands).toEqual(['echo user-authored']) + expect(config.hooks.BeforeTool[0].hooks[0].command).toContain(managedHookFileName) + }) +}) diff --git a/src/main/gemini/hook-service.ts b/src/main/gemini/hook-service.ts index 8247ca43365..4ff71a10eec 100644 --- a/src/main/gemini/hook-service.ts +++ b/src/main/gemini/hook-service.ts @@ -25,12 +25,10 @@ import { // (approvals flow through inline UI), so Orca cannot surface a waiting state // for Gemini — that is an upstream limitation, not an Orca bug. // -// PreToolUse surfaces the current tool name + input preview (e.g. -// `read_file: src/foo.ts`) so long-running tool calls aren't a silent gap -// between BeforeAgent and AfterAgent. PostToolUse is intentionally omitted — -// AfterTool already signals "back to working" and the tool name from -// PreToolUse is what we show; PostToolUse would be a redundant fire. -const GEMINI_EVENTS = ['BeforeAgent', 'AfterAgent', 'AfterTool', 'PreToolUse'] as const +// Gemini's native pre-tool event is BeforeTool, not Claude/Codex's PreToolUse. +// Keep installing the pre-tool status hook, but sweep stale PreToolUse entries +// below so current Gemini CLI no longer warns about an invalid event bucket. +const GEMINI_EVENTS = ['BeforeAgent', 'AfterAgent', 'AfterTool', 'BeforeTool'] as const function getConfigPath(): string { return join(homedir(), '.gemini', 'settings.json') @@ -175,6 +173,27 @@ export class GeminiHookService { // accumulate duplicate hook entries pointing at defunct scripts. const isManagedCommand = createManagedCommandMatcher(getManagedScriptFileName()) + const managedEvents = new Set(GEMINI_EVENTS) + + // Why: when Orca stops subscribing to an event, install() must sweep the + // old managed entry out of any leftover event bucket. Otherwise a stale + // hook such as PreToolUse survives forever in ~/.gemini/settings.json and + // continues firing even though the current build no longer wants it. + for (const [eventName, definitions] of Object.entries(nextHooks)) { + if (managedEvents.has(eventName)) { + continue + } + if (!Array.isArray(definitions)) { + continue + } + const cleaned = removeManagedCommands(definitions, isManagedCommand) + if (cleaned.length === 0) { + delete nextHooks[eventName] + } else { + nextHooks[eventName] = cleaned + } + } + for (const eventName of GEMINI_EVENTS) { const current = Array.isArray(nextHooks[eventName]) ? nextHooks[eventName] : [] const cleaned = removeManagedCommands(current, isManagedCommand) @@ -213,6 +232,24 @@ export class GeminiHookService { const command = wrapPosixHookCommand(remoteScriptPath) const nextHooks = { ...config.hooks } const isManagedCommand = createManagedCommandMatcher('gemini-hook.sh') + const managedEvents = new Set(GEMINI_EVENTS) + + // Why: remote installs must sweep legacy managed event buckets too. + // Otherwise stale PreToolUse entries keep warning in SSH Gemini sessions. + for (const [eventName, definitions] of Object.entries(nextHooks)) { + if (managedEvents.has(eventName)) { + continue + } + if (!Array.isArray(definitions)) { + continue + } + const cleaned = removeManagedCommands(definitions, isManagedCommand) + if (cleaned.length === 0) { + delete nextHooks[eventName] + } else { + nextHooks[eventName] = cleaned + } + } for (const eventName of GEMINI_EVENTS) { const current = Array.isArray(nextHooks[eventName]) ? nextHooks[eventName] : [] diff --git a/src/shared/agent-hook-listener.test.ts b/src/shared/agent-hook-listener.test.ts index 0b591aaa35b..669da848010 100644 --- a/src/shared/agent-hook-listener.test.ts +++ b/src/shared/agent-hook-listener.test.ts @@ -80,6 +80,27 @@ describe('shared agent-hook-listener', () => { expect(event!.payload.agentType).toBe('claude') }) + it('normalizes Gemini BeforeTool to working with tool fields', () => { + const event = normalizeHookPayload( + state, + 'gemini', + { + paneKey: PANE_KEY, + payload: { + hook_event_name: 'BeforeTool', + tool_name: 'read_file', + args: { file_path: 'src/index.ts' } + } + }, + 'production' + ) + + expect(event?.payload.state).toBe('working') + expect(event?.payload.agentType).toBe('gemini') + expect(event?.payload.toolName).toBe('read_file') + expect(event?.payload.toolInput).toBe('src/index.ts') + }) + it('normalizes OMP Pi-compatible hooks with OMP attribution', () => { const event = normalizeHookPayload( state, diff --git a/src/shared/agent-hook-listener.ts b/src/shared/agent-hook-listener.ts index 610148cc716..e29f9d61e8c 100644 --- a/src/shared/agent-hook-listener.ts +++ b/src/shared/agent-hook-listener.ts @@ -971,7 +971,12 @@ function extractGeminiToolFields( eventName: unknown, hookPayload: Record ): ToolSnapshot { - if (eventName === 'PreToolUse' || eventName === 'PostToolUse' || eventName === 'AfterTool') { + if ( + eventName === 'BeforeTool' || + eventName === 'AfterTool' || + eventName === 'PreToolUse' || + eventName === 'PostToolUse' + ) { const toolName = readString(hookPayload, 'tool_name') ?? readString(hookPayload, 'name') const toolInput = deriveToolInputPreview(toolName, hookPayload.tool_input) ?? @@ -1756,8 +1761,11 @@ function normalizeGeminiEvent( paneKey: string, hookPayload: Record ): ParsedAgentStatusPayload | null { + // Why: Gemini CLI's native pre-tool event is BeforeTool. PreToolUse/PostToolUse + // remain accepted for legacy Antigravity-compatible payloads on this endpoint. const stateName = eventName === 'BeforeAgent' || + eventName === 'BeforeTool' || eventName === 'AfterTool' || eventName === 'PreToolUse' || eventName === 'PostToolUse'