From c375f403efd7c2d87f827b983d158784f400448b Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Thu, 7 May 2026 13:05:30 -0700 Subject: [PATCH] fix(agent-hooks): silence exit-127 spam from stale managed hook entries (#1536) Co-authored-by: Orca --- src/main/agent-hooks/installer-utils.test.ts | 73 +++++++++++++++++++- src/main/agent-hooks/installer-utils.ts | 16 +++++ src/main/claude/hook-service.ts | 18 +++-- src/main/codex/hook-service.ts | 3 +- src/main/cursor/hook-service.ts | 3 +- src/main/gemini/hook-service.ts | 3 +- 6 files changed, 104 insertions(+), 12 deletions(-) diff --git a/src/main/agent-hooks/installer-utils.test.ts b/src/main/agent-hooks/installer-utils.test.ts index 906bceec4..a074db740 100644 --- a/src/main/agent-hooks/installer-utils.test.ts +++ b/src/main/agent-hooks/installer-utils.test.ts @@ -6,11 +6,18 @@ import { readFileSync, readdirSync, rmSync, - writeFileSync + writeFileSync, + chmodSync } from 'fs' import { tmpdir } from 'os' import { join } from 'path' -import { createManagedCommandMatcher, writeHooksJson, type HooksConfig } from './installer-utils' +import { spawnSync } from 'child_process' +import { + createManagedCommandMatcher, + wrapPosixHookCommand, + writeHooksJson, + type HooksConfig +} from './installer-utils' let tmpDir: string let configPath: string @@ -141,4 +148,66 @@ describe('createManagedCommandMatcher', () => { it('does not match hooks for a different agent', () => { expect(match('/bin/sh "/path/agent-hooks/gemini-hook.sh"')).toBe(false) }) + + it('matches the guarded launcher form so wrapped commands sweep correctly', () => { + // Why: wrapPosixHookCommand wraps the launcher in `if [ -x ... ]; then ...; fi` + // so a stale entry no-ops instead of returning exit 127. The sweep on + // install() must still recognize the guarded form as managed, otherwise + // repeated installs would accumulate one guarded + one unguarded entry. + expect( + match( + 'if [ -x "/Users/alice/Library/Application Support/Orca/agent-hooks/claude-hook.sh" ]; then /bin/sh "/Users/alice/Library/Application Support/Orca/agent-hooks/claude-hook.sh"; fi' + ) + ).toBe(true) + }) +}) + +describe('wrapPosixHookCommand', () => { + it('produces a guarded command that no-ops when the script is missing', () => { + const cmd = wrapPosixHookCommand('/does/not/exist.sh') + expect(cmd).toBe("if [ -x '/does/not/exist.sh' ]; then /bin/sh '/does/not/exist.sh'; fi") + }) + + it('preserves spaces in the script path (Library/Application Support case)', () => { + // Why: Electron's userData on macOS lives under "Application Support" with + // a space. The guard must keep the path quoted so `[ -x ]` and `/bin/sh` + // each see one argument. + const cmd = wrapPosixHookCommand('/Users/a/Library/Application Support/Orca/agent-hooks/x.sh') + expect(cmd).toContain("'/Users/a/Library/Application Support/Orca/agent-hooks/x.sh'") + }) + + it('escapes embedded single quotes so the wrapped command stays well-formed', () => { + // Why: POSIX single-quote escape renders ' as '\''. Verify a path with an + // embedded quote does not break out of the quoting and instead reaches + // /bin/sh as a single argument. + const cmd = wrapPosixHookCommand("/path/with'quote/x.sh") + expect(cmd).toBe( + "if [ -x '/path/with'\\''quote/x.sh' ]; then /bin/sh '/path/with'\\''quote/x.sh'; fi" + ) + }) + + it.skipIf(process.platform === 'win32')( + 'returns exit code 0 when the script does not exist (no-op)', + () => { + const cmd = wrapPosixHookCommand('/does/not/exist.sh') + const result = spawnSync('/bin/sh', ['-c', cmd]) + expect(result.status).toBe(0) + } + ) + + // Why: commit 4d618795 explicitly switched from `&& ... || true` (which + // swallowed non-zero exits) to `if ... then ... fi` (which preserves the + // script's exit code). This test guards against a future regression that + // re-introduces the swallowing form. + it.skipIf(process.platform === 'win32')( + 'propagates the script exit code when the script runs and fails', + () => { + const scriptPath = join(tmpDir, 'fails.sh') + writeFileSync(scriptPath, '#!/bin/sh\nexit 7\n', 'utf-8') + chmodSync(scriptPath, 0o755) + const cmd = wrapPosixHookCommand(scriptPath) + const result = spawnSync('/bin/sh', ['-c', cmd]) + expect(result.status).toBe(7) + } + ) }) diff --git a/src/main/agent-hooks/installer-utils.ts b/src/main/agent-hooks/installer-utils.ts index eb1d028eb..a63528500 100644 --- a/src/main/agent-hooks/installer-utils.ts +++ b/src/main/agent-hooks/installer-utils.ts @@ -65,6 +65,22 @@ export function createManagedCommandMatcher( } } +// Why: a stale managed hook entry (left over after the user wiped userData, +// switched dev↔prod installs, or had a partial install fail) used to fire +// `/bin/sh ""` on every tool call, which exits 127 and surfaces +// as `PreToolUse hook (failed) error: hook exited with code 127` in the agent +// transcript. Wrapping the launcher in `if [ -x ... ]; then ...; fi` makes a +// missing/non-executable script a silent no-op so a broken install never +// poisons the user's session. Failures inside the script itself are +// unaffected — only the missing-script case short-circuits. +export function wrapPosixHookCommand(scriptPath: string): string { + // Why: POSIX single-quote escape so $, `, ", and \ in scriptPath are taken + // literally — avoids a shell-injection footgun if a future caller passes an + // arbitrary path. + const quoted = `'${scriptPath.replaceAll("'", "'\\''")}'` + return `if [ -x ${quoted} ]; then /bin/sh ${quoted}; fi` +} + export function removeManagedCommands( definitions: HookDefinition[], isManagedCommand: (command: string | undefined) => boolean diff --git a/src/main/claude/hook-service.ts b/src/main/claude/hook-service.ts index a78445ec1..d8b85e3ad 100644 --- a/src/main/claude/hook-service.ts +++ b/src/main/claude/hook-service.ts @@ -6,6 +6,7 @@ import { createManagedCommandMatcher, readHooksJson, removeManagedCommands, + wrapPosixHookCommand, writeHooksJson, writeManagedScript, type HookDefinition @@ -48,13 +49,16 @@ function getManagedScriptPath(): string { } function getManagedCommand(scriptPath: string): string { - // Why: on Windows, Claude Code runs hooks through Git Bash (`/usr/bin/bash`). - // A path with single backslashes (e.g. `C:\Users\…\claude-hook.cmd`) is - // interpreted by bash as a string with escape sequences, so `\U`, `\A`, etc. - // collapse and the launcher fails with `command not found`. Emit forward - // slashes — Windows accepts them in path arguments and bash leaves them - // intact, so the same JSON value works through every shell layer. - return process.platform === 'win32' ? scriptPath.replaceAll('\\', '/') : `/bin/sh "${scriptPath}"` + if (process.platform === 'win32') { + // Why: on Windows, Claude Code runs hooks through Git Bash (`/usr/bin/bash`). + // A path with single backslashes (e.g. `C:\Users\…\claude-hook.cmd`) is + // interpreted by bash as a string with escape sequences, so `\U`, `\A`, etc. + // collapse and the launcher fails with `command not found`. Emit forward + // slashes — Windows accepts them in path arguments and bash leaves them + // intact, so the same JSON value works through every shell layer. + return scriptPath.replaceAll('\\', '/') + } + return wrapPosixHookCommand(scriptPath) } function getManagedScript(): string { diff --git a/src/main/codex/hook-service.ts b/src/main/codex/hook-service.ts index d57555c06..c7ed872f3 100644 --- a/src/main/codex/hook-service.ts +++ b/src/main/codex/hook-service.ts @@ -6,6 +6,7 @@ import { createManagedCommandMatcher, readHooksJson, removeManagedCommands, + wrapPosixHookCommand, writeHooksJson, writeManagedScript, type HookDefinition @@ -39,7 +40,7 @@ function getManagedScriptPath(): string { } function getManagedCommand(scriptPath: string): string { - return process.platform === 'win32' ? scriptPath : `/bin/sh "${scriptPath}"` + return process.platform === 'win32' ? scriptPath : wrapPosixHookCommand(scriptPath) } function getManagedScript(): string { diff --git a/src/main/cursor/hook-service.ts b/src/main/cursor/hook-service.ts index ed7d479c7..9d45681d5 100644 --- a/src/main/cursor/hook-service.ts +++ b/src/main/cursor/hook-service.ts @@ -6,6 +6,7 @@ import { createManagedCommandMatcher, readHooksJson, removeManagedCommands, + wrapPosixHookCommand, writeHooksJson, writeManagedScript, type HookDefinition @@ -51,7 +52,7 @@ function getManagedScriptPath(): string { } function getManagedCommand(scriptPath: string): string { - return process.platform === 'win32' ? scriptPath : `/bin/sh "${scriptPath}"` + return process.platform === 'win32' ? scriptPath : wrapPosixHookCommand(scriptPath) } function getManagedScript(): string { diff --git a/src/main/gemini/hook-service.ts b/src/main/gemini/hook-service.ts index 4239460bd..40e4fb5d2 100644 --- a/src/main/gemini/hook-service.ts +++ b/src/main/gemini/hook-service.ts @@ -6,6 +6,7 @@ import { createManagedCommandMatcher, readHooksJson, removeManagedCommands, + wrapPosixHookCommand, writeHooksJson, writeManagedScript, type HookDefinition @@ -37,7 +38,7 @@ function getManagedScriptPath(): string { } function getManagedCommand(scriptPath: string): string { - return process.platform === 'win32' ? scriptPath : `/bin/sh "${scriptPath}"` + return process.platform === 'win32' ? scriptPath : wrapPosixHookCommand(scriptPath) } function getManagedScript(): string {