mirror of
https://github.com/stablyai/orca.git
synced 2026-09-22 00:02:31 +00:00
fix(agent-hooks): default-form managed hook vars so a static precheck cannot reject them (#14994)
The managed hook command embedded a bare $SYSTEMROOT. Grok loads Claude's
settings.json hooks and statically prechecks env vars across the whole command
string, so the reference inside the never-taken Windows branch made it refuse
the hook on macOS on every event:
hook not executed: required env var(s) not set: ${SYSTEMROOT}
Grok fails these open, and Orca installs Grok's native hook separately, so no
status was lost -- the symptom is a swallowed failure line per tool call.
$VAR and ${VAR-} expand identically in POSIX shells absent set -u, so this has
no execution-time effect; only the static precheck observes it. Verified in Git
Bash on Windows that both guard forms resolve identically ($SYSTEMROOT is set
there and uppercase is the correct spelling -- $SystemRoot is undefined).
Also converts the three bare $HOME references so the regression test can assert
zero bare variable references with no exemption. A $SYSTEMROOT-specific check
would not have caught this class of bug being introduced elsewhere.
This commit is contained in:
@@ -320,7 +320,7 @@ async function verifyInstalledLauncher(home, payload) {
|
||||
)
|
||||
if (
|
||||
!command ||
|
||||
!command.includes('"$HOME/.orca/agent-hooks/claude-hook.sh"') ||
|
||||
!command.includes('"${HOME-}/.orca/agent-hooks/claude-hook.sh"') ||
|
||||
!command.includes('] && [ -r ') ||
|
||||
!command.includes('else { command -p cat')
|
||||
) {
|
||||
|
||||
@@ -270,6 +270,16 @@ describe('createManagedCommandMatcher', () => {
|
||||
expect(match(command)).toBe(true)
|
||||
})
|
||||
|
||||
it('matches the pre-default-form launcher so upgrades replace it instead of duplicating', () => {
|
||||
// Why: installs before the ${VAR-} conversion emitted bare $HOME/$SYSTEMROOT. The sweep must
|
||||
// still recognize them, or an upgrade would leave the stale entry beside the new one.
|
||||
expect(
|
||||
match(
|
||||
'if [ -z "$HOME" ]; then :; else if [ -f "$HOME/.orca/agent-hooks/claude-hook.sh" ]; then /bin/sh "$HOME/.orca/agent-hooks/claude-hook.sh"; fi; fi'
|
||||
)
|
||||
).toBe(true)
|
||||
})
|
||||
|
||||
it('matches PowerShell and POSIX variants across Copilot platform switches', () => {
|
||||
const matchPosix = createManagedCommandMatcher('copilot-hook.sh')
|
||||
const matchPowerShell = createManagedCommandMatcher('copilot-hook.ps1')
|
||||
@@ -678,13 +688,29 @@ describe('wrapRuntimeHomeHookCommand', () => {
|
||||
const command = wrapRuntimeHomeHookCommand('claude-hook')
|
||||
|
||||
expect(command).toContain('case "${OSTYPE-}" in msys*|cygwin*|win32*)')
|
||||
expect(command).toContain('case "$HOME" in *\\&*|*\\^*|*\\(*|*\\)*|*\\;*|*,*|*=*|*%*|*\\!*)')
|
||||
expect(command).toContain('case "${HOME-}" in *\\&*|*\\^*|*\\(*|*\\)*|*\\;*|*,*|*=*|*%*|*\\!*)')
|
||||
expect(command).not.toContain('uname')
|
||||
expect(command).toContain('"$HOME/.orca/agent-hooks/claude-hook.cmd"')
|
||||
expect(command).toContain('/bin/sh "$HOME/.orca/agent-hooks/claude-hook.sh"')
|
||||
expect(command).toContain('"${HOME-}/.orca/agent-hooks/claude-hook.cmd"')
|
||||
expect(command).toContain('/bin/sh "${HOME-}/.orca/agent-hooks/claude-hook.sh"')
|
||||
expect(command).not.toMatch(/[A-Z]:[\\/]|\/Users\/|\/home\//)
|
||||
})
|
||||
|
||||
// Why: a static hook precheck (Grok) rejects the whole command on any bare reference it cannot
|
||||
// resolve, including one in a branch that platform never takes.
|
||||
it.each([
|
||||
['default', undefined],
|
||||
['neutral-json', { neutralJsonWhenMissing: true }]
|
||||
])(
|
||||
'references every variable in default form (%s) so a static precheck cannot reject it',
|
||||
(_label, options) => {
|
||||
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_]*\}/)
|
||||
}
|
||||
)
|
||||
|
||||
it('rejects a script base name that could inject shell syntax', () => {
|
||||
expect(() => wrapRuntimeHomeHookCommand('claude-hook; echo injected')).toThrow(
|
||||
'Invalid managed script base name'
|
||||
|
||||
@@ -14,18 +14,20 @@ export function wrapRuntimeHomeHookCommand(
|
||||
if (!MANAGED_SCRIPT_BASE_NAME.test(scriptBaseName)) {
|
||||
throw new Error(`Invalid managed script base name: ${scriptBaseName}`)
|
||||
}
|
||||
const windowsScript = `"$HOME/.orca/agent-hooks/${scriptBaseName}.cmd"`
|
||||
const posixScript = `"$HOME/.orca/agent-hooks/${scriptBaseName}.sh"`
|
||||
// Why: default-form every var — a static hook precheck (Grok) rejects the whole command on a bare
|
||||
// reference it cannot resolve, even in a branch that platform never takes.
|
||||
const windowsScript = `"\${HOME-}/.orca/agent-hooks/${scriptBaseName}.cmd"`
|
||||
const posixScript = `"\${HOME-}/.orca/agent-hooks/${scriptBaseName}.sh"`
|
||||
const drain = POSIX_HOOK_STDIN_DRAIN_COMMAND
|
||||
const missingScriptFallback = options.neutralJsonWhenMissing ? `${drain}; printf '{}\\n'` : drain
|
||||
const powershell = '"$SYSTEMROOT/System32/WindowsPowerShell/v1.0/powershell.exe"'
|
||||
const powershell = '"${SYSTEMROOT-}/System32/WindowsPowerShell/v1.0/powershell.exe"'
|
||||
const powershellFallback = options.neutralJsonWhenMissing ? "; Write-Output '{}'" : ''
|
||||
const powershellCommand = `$homePath = $env:HOME -replace '^/([A-Za-z])/', '$1:/'; $scriptPath = Join-Path $homePath '.orca\\agent-hooks\\${scriptBaseName}.cmd'; if (Test-Path -LiteralPath $scriptPath -PathType Leaf) { & $scriptPath; exit $LASTEXITCODE }; [Console]::In.ReadToEnd() | Out-Null${powershellFallback}; exit 0`
|
||||
const encodedCommand = encodeWindowsPowerShellHookCommand(powershellCommand)
|
||||
// Why: the Git Bash and native Windows launchers must suppress windows identically (#14815).
|
||||
const powershellInvocation = `${powershell} ${WINDOWS_POWERSHELL_HOOK_SWITCHES} -EncodedCommand ${encodedCommand}`
|
||||
const encodedWindowsBranch = `if [ -f ${powershell} ]; then ${powershellInvocation}; else ${missingScriptFallback}; fi`
|
||||
const windowsBranch = `if [ -f ${windowsScript} ]; then case "$HOME" in ${WINDOWS_GIT_BASH_RUNTIME_HOME_UNSAFE}) ${encodedWindowsBranch} ;; *) ${windowsScript} ;; esac; else ${missingScriptFallback}; fi`
|
||||
const windowsBranch = `if [ -f ${windowsScript} ]; then case "\${HOME-}" in ${WINDOWS_GIT_BASH_RUNTIME_HOME_UNSAFE}) ${encodedWindowsBranch} ;; *) ${windowsScript} ;; esac; else ${missingScriptFallback}; fi`
|
||||
const posixBranch = `if [ -f ${posixScript} ] && [ -r ${posixScript} ] && [ -x ${posixScript} ]; then /bin/sh ${posixScript}; else ${missingScriptFallback}; fi`
|
||||
// Why: OSTYPE is shell-owned, so platform selection adds no process to every hook invocation.
|
||||
return `if [ -z "\${HOME-}" ]; then ${missingScriptFallback}; else case "\${OSTYPE-}" in msys*|cygwin*|win32*) ${windowsBranch} ;; *) ${posixBranch} ;; esac; fi`
|
||||
|
||||
@@ -242,10 +242,10 @@ describe('ClaudeHookService.install', () => {
|
||||
) as { statusLine?: { type: string; command: string } }
|
||||
expect(settings.statusLine?.type).toBe('command')
|
||||
expect(settings.statusLine?.command).toContain(
|
||||
'"$HOME/.orca/agent-hooks/claude-statusline.cmd"'
|
||||
'"${HOME-}/.orca/agent-hooks/claude-statusline.cmd"'
|
||||
)
|
||||
expect(settings.statusLine?.command).toContain(
|
||||
'"$HOME/.orca/agent-hooks/claude-statusline.sh"'
|
||||
'"${HOME-}/.orca/agent-hooks/claude-statusline.sh"'
|
||||
)
|
||||
expect(settings.statusLine?.command).not.toContain(tmpHome.replaceAll('\\', '/'))
|
||||
|
||||
@@ -454,7 +454,7 @@ describe('ClaudeHookService.installRemote', () => {
|
||||
]) {
|
||||
expect(parsed.hooks[event]).toBeTruthy()
|
||||
const cmd = parsed.hooks[event][0].hooks[0].command as string
|
||||
expect(cmd).toContain('"$HOME/.orca/agent-hooks/claude-hook.sh"')
|
||||
expect(cmd).toContain('"${HOME-}/.orca/agent-hooks/claude-hook.sh"')
|
||||
expect(cmd).not.toContain('/home/dev/.orca/agent-hooks/claude-hook.sh')
|
||||
}
|
||||
// Managed script body
|
||||
@@ -551,8 +551,8 @@ describe('OpenClaudeHookService-compatible install', () => {
|
||||
for (const event of ['UserPromptSubmit', 'Stop', 'StopFailure']) {
|
||||
const command = parsed.hooks[event][0].hooks[0].command as string
|
||||
expect(isOpenClaudeManagedCommand(command)).toBe(true)
|
||||
expect(command).toContain('"$HOME/.orca/agent-hooks/openclaude-hook.cmd"')
|
||||
expect(command).toContain('"$HOME/.orca/agent-hooks/openclaude-hook.sh"')
|
||||
expect(command).toContain('"${HOME-}/.orca/agent-hooks/openclaude-hook.cmd"')
|
||||
expect(command).toContain('"${HOME-}/.orca/agent-hooks/openclaude-hook.sh"')
|
||||
expect(command).not.toContain(tmpHome.replaceAll('\\', '/'))
|
||||
}
|
||||
expect(
|
||||
@@ -582,7 +582,7 @@ describe('OpenClaudeHookService-compatible install', () => {
|
||||
})
|
||||
const parsed = JSON.parse(fs.files.get('/home/dev/.openclaude/settings.json')!)
|
||||
const command = parsed.hooks.StopFailure[0].hooks[0].command as string
|
||||
expect(command).toContain('"$HOME/.orca/agent-hooks/openclaude-hook.sh"')
|
||||
expect(command).toContain('"${HOME-}/.orca/agent-hooks/openclaude-hook.sh"')
|
||||
expect(command).not.toContain('/home/dev/.orca/agent-hooks/openclaude-hook.sh')
|
||||
expect(fs.files.get('/home/dev/.orca/agent-hooks/openclaude-hook.sh')).toContain('/hook/claude')
|
||||
})
|
||||
|
||||
Reference in New Issue
Block a user