mirror of
https://github.com/stablyai/orca.git
synced 2026-09-22 08:02:28 +00:00
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 <help@stably.ai> * fix: cover Gemini hook migration for remote installs Co-authored-by: Orca <help@stably.ai> * test: cover local Gemini hook sweep preservation Co-authored-by: Orca <help@stably.ai> --------- Co-authored-by: ramzi <ramzi@upsys-consulting.com> Co-authored-by: Orca <help@stably.ai>
This commit is contained in:
Jinwoo Hong
co-authored by
Orca
ramzi
Orca
ramzi
parent
9348dd488f
commit
95862116aa
@@ -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<string, { hooks: { command: string }[] }[]>
|
||||
}
|
||||
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<string, { hooks?: { command: string }[] }[]>
|
||||
}
|
||||
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')
|
||||
|
||||
@@ -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' }
|
||||
}),
|
||||
|
||||
@@ -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<typeof osModule>()
|
||||
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)
|
||||
})
|
||||
})
|
||||
@@ -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<string>(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<string>(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] : []
|
||||
|
||||
@@ -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,
|
||||
|
||||
@@ -971,7 +971,12 @@ function extractGeminiToolFields(
|
||||
eventName: unknown,
|
||||
hookPayload: Record<string, unknown>
|
||||
): 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<string, unknown>
|
||||
): 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'
|
||||
|
||||
Reference in New Issue
Block a user