diff --git a/config/reliability-gates.jsonc b/config/reliability-gates.jsonc index 0efe0887df9..bc1e8648e28 100644 --- a/config/reliability-gates.jsonc +++ b/config/reliability-gates.jsonc @@ -2596,7 +2596,7 @@ "invariant": "Startup, Settings, runtime RPC, offline CLI, and SSH enable paths must not mutate third-party managed hook config without positive CLI presence or a resolvable explicit command override; disabling hooks remains ungated so stale Orca-managed hooks can be removed.", "oracle": "Presence-gated install calls only installers with found local or remote CLI evidence; missing or unknown CLIs return skipped statuses and do not write config; status reads remain config-authoritative; SSH all-missing/unknown presence avoids opening SFTP.", "commands": [ - "pnpm exec vitest run --config config/vitest.config.ts src/main/agent-hooks/managed-agent-hook-manifest.test.ts src/main/agent-hooks/local-agent-cli-presence.test.ts src/main/agent-hooks/managed-agent-hook-controls.test.ts src/main/agent-hooks/install-telemetry.test.ts src/main/agent-hooks/remote-managed-hook-installers.test.ts src/main/ssh/ssh-relay-session-agent-hook-presence.test.ts src/relay/agent-hook-cli-presence.test.ts src/relay/agent-hook-cli-presence-timeout.test.ts src/cli/handlers/agent-hooks.test.ts src/main/runtime/rpc/methods/client-ui.test.ts src/main/runtime/orca-runtime.test.ts" + "pnpm exec vitest run --config config/vitest.config.ts src/main/agent-hooks/managed-agent-hook-manifest.test.ts src/main/agent-hooks/local-agent-cli-presence.test.ts src/main/agent-hooks/managed-agent-hook-controls.test.ts src/main/agent-hooks/install-telemetry.test.ts src/main/agent-hooks/remote-managed-hook-installers.test.ts src/main/ssh/ssh-relay-session-agent-hook-presence.test.ts src/relay/agent-hook-cli-presence.test.ts src/relay/agent-hook-cli-presence-timeout.test.ts src/cli/handlers/agent-hooks.test.ts src/main/runtime/rpc/methods/client-ui.test.ts src/main/runtime/orca-runtime.test.ts src/main/codex/codex-launch-hook-upkeep.test.ts" ], "testFiles": [ "src/main/agent-hooks/managed-agent-hook-manifest.test.ts", @@ -2609,7 +2609,8 @@ "src/relay/agent-hook-cli-presence-timeout.test.ts", "src/cli/handlers/agent-hooks.test.ts", "src/main/runtime/rpc/methods/client-ui.test.ts", - "src/main/runtime/orca-runtime.test.ts" + "src/main/runtime/orca-runtime.test.ts", + "src/main/codex/codex-launch-hook-upkeep.test.ts" ], "assertionRefs": [ { @@ -2642,7 +2643,7 @@ "date": "2026-07-05", "runner": "local", "platform": "macos", - "command": "pnpm exec vitest run --config config/vitest.config.ts src/main/agent-hooks/managed-agent-hook-manifest.test.ts src/main/agent-hooks/local-agent-cli-presence.test.ts src/main/agent-hooks/managed-agent-hook-controls.test.ts src/main/agent-hooks/install-telemetry.test.ts src/main/agent-hooks/remote-managed-hook-installers.test.ts src/main/ssh/ssh-relay-session-agent-hook-presence.test.ts src/relay/agent-hook-cli-presence.test.ts src/relay/agent-hook-cli-presence-timeout.test.ts src/cli/handlers/agent-hooks.test.ts src/main/runtime/rpc/methods/client-ui.test.ts src/main/runtime/orca-runtime.test.ts", + "command": "pnpm exec vitest run --config config/vitest.config.ts src/main/agent-hooks/managed-agent-hook-manifest.test.ts src/main/agent-hooks/local-agent-cli-presence.test.ts src/main/agent-hooks/managed-agent-hook-controls.test.ts src/main/agent-hooks/install-telemetry.test.ts src/main/agent-hooks/remote-managed-hook-installers.test.ts src/main/ssh/ssh-relay-session-agent-hook-presence.test.ts src/relay/agent-hook-cli-presence.test.ts src/relay/agent-hook-cli-presence-timeout.test.ts src/cli/handlers/agent-hooks.test.ts src/main/runtime/rpc/methods/client-ui.test.ts src/main/runtime/orca-runtime.test.ts src/main/codex/codex-launch-hook-upkeep.test.ts", "result": "passed", "durationSeconds": 12, "summary": "11 test files passed, 639 tests passed locally for managed hook CLI presence gating." diff --git a/src/main/agent-hooks/install-telemetry.test.ts b/src/main/agent-hooks/install-telemetry.test.ts index 9af8a321d4f..8e82a486dd6 100644 --- a/src/main/agent-hooks/install-telemetry.test.ts +++ b/src/main/agent-hooks/install-telemetry.test.ts @@ -1,7 +1,7 @@ -// Pins the contract between `runManagedHookInstallers` and the -// `agent_hook_install_failed` telemetry event: each catch must fire `track` -// with the correct agent label and a truncated error_message, and one -// installer's failure must not stop the others (fail-open semantics). +// Pins the contract of `recordManagedHookInstallFailure`: it fires the +// `agent_hook_install_failed` telemetry event with the correct agent label and a +// truncated error_message, and it swallows any throw from `track` so a broken +// telemetry client can never break the fail-open installer loop that calls it. import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' @@ -9,9 +9,9 @@ const { trackMock } = vi.hoisted(() => ({ trackMock: vi.fn() })) vi.mock('../telemetry/client', () => ({ track: trackMock })) -import { runManagedHookInstallers } from './install-telemetry' +import { recordManagedHookInstallFailure } from './install-telemetry' -describe('runManagedHookInstallers', () => { +describe('recordManagedHookInstallFailure', () => { beforeEach(() => { trackMock.mockReset() vi.spyOn(console, 'error').mockImplementation(() => {}) @@ -20,27 +20,8 @@ describe('runManagedHookInstallers', () => { vi.restoreAllMocks() }) - it('runs every installer when none throw and never calls track', () => { - const claude = vi.fn() - const codex = vi.fn() - runManagedHookInstallers([ - ['claude', claude], - ['codex', codex] - ]) - expect(claude).toHaveBeenCalledTimes(1) - expect(codex).toHaveBeenCalledTimes(1) - expect(trackMock).not.toHaveBeenCalled() - }) - - it('fires agent_hook_install_failed with the correct agent label when an installer throws', () => { - runManagedHookInstallers([ - [ - 'codex', - () => { - throw new Error('codex config malformed') - } - ] - ]) + it('fires agent_hook_install_failed with the correct agent label', () => { + recordManagedHookInstallFailure('codex', new Error('codex config malformed')) expect(trackMock).toHaveBeenCalledTimes(1) expect(trackMock).toHaveBeenCalledWith('agent_hook_install_failed', { @@ -49,52 +30,17 @@ describe('runManagedHookInstallers', () => { }) }) - it('continues running later installers after an earlier one throws (fail-open)', () => { - const codex = vi.fn() - const gemini = vi.fn() - runManagedHookInstallers([ - [ - 'claude', - () => { - throw new Error('claude failed') - } - ], - ['codex', codex], - ['gemini', gemini] - ]) - expect(codex).toHaveBeenCalledTimes(1) - expect(gemini).toHaveBeenCalledTimes(1) - expect(trackMock).toHaveBeenCalledTimes(1) - expect(trackMock).toHaveBeenCalledWith( - 'agent_hook_install_failed', - expect.objectContaining({ agent: 'claude' }) - ) - }) - it('truncates error_message to 200 chars', () => { - const longMessage = 'x'.repeat(500) - runManagedHookInstallers([ - [ - 'gemini', - () => { - throw new Error(longMessage) - } - ] - ]) + recordManagedHookInstallFailure('gemini', new Error('x'.repeat(500))) + expect(trackMock).toHaveBeenCalledTimes(1) const [, props] = trackMock.mock.calls[0] as [string, { error_message: string }] expect(props.error_message.length).toBe(200) }) it('handles non-Error throws', () => { - runManagedHookInstallers([ - [ - 'cursor', - () => { - throw 'cursor string failure' - } - ] - ]) + recordManagedHookInstallFailure('cursor', 'cursor string failure') + expect(trackMock).toHaveBeenCalledWith('agent_hook_install_failed', { agent: 'cursor', error_message: 'cursor string failure' @@ -102,14 +48,8 @@ describe('runManagedHookInstallers', () => { }) it('serializes thrown objects through JSON.stringify', () => { - runManagedHookInstallers([ - [ - 'cursor', - () => { - throw { code: 'EACCES', path: '/tmp' } - } - ] - ]) + recordManagedHookInstallFailure('cursor', { code: 'EACCES', path: '/tmp' }) + expect(trackMock).toHaveBeenCalledTimes(1) expect(trackMock).toHaveBeenCalledWith('agent_hook_install_failed', { agent: 'cursor', @@ -117,39 +57,20 @@ describe('runManagedHookInstallers', () => { }) }) - it('does not throw when an installer throws undefined (regression for JSON.stringify undefined return)', () => { - expect(() => - runManagedHookInstallers([ - [ - 'cursor', - () => { - throw undefined - } - ] - ]) - ).not.toThrow() + it('does not throw on an undefined error (regression for JSON.stringify undefined)', () => { + expect(() => recordManagedHookInstallFailure('cursor', undefined)).not.toThrow() expect(trackMock).toHaveBeenCalledTimes(1) const [eventName, props] = trackMock.mock.calls[0] as [string, { error_message: string }] expect(eventName).toBe('agent_hook_install_failed') expect(typeof props.error_message).toBe('string') }) - it('continues running later installers when track itself throws (telemetry must not break fail-open)', () => { - const codex = vi.fn() + it('swallows a throw from track so the caller stays fail-open', () => { trackMock.mockImplementationOnce(() => { throw new Error('telemetry blew up') }) expect(() => - runManagedHookInstallers([ - [ - 'claude', - () => { - throw new Error('claude failed') - } - ], - ['codex', codex] - ]) + recordManagedHookInstallFailure('claude', new Error('claude failed')) ).not.toThrow() - expect(codex).toHaveBeenCalledTimes(1) }) }) diff --git a/src/main/agent-hooks/install-telemetry.ts b/src/main/agent-hooks/install-telemetry.ts index 46afdfda62d..06a3bff4cb9 100644 --- a/src/main/agent-hooks/install-telemetry.ts +++ b/src/main/agent-hooks/install-telemetry.ts @@ -1,11 +1,10 @@ -// Per-agent managed-hook installer with fail-open semantics and PostHog -// attribution. Lifted out of `src/main/index.ts` so the loop is unit-testable -// without standing up the full Electron startup graph — the catch site needs -// the agent label to fire `agent_hook_install_failed`, and the previous -// closure-style loop lost it. +// PostHog attribution for managed-hook install failures. Kept out of the +// CLI-safe `managed-agent-hook-controls` module (and out of `src/main/index.ts`) +// because `track` pulls in the Electron telemetry client: main-process callers +// inject `recordManagedHookInstallFailure` into the installer loop, while the +// offline CLI path leaves it unset so it never imports Electron. import type { HookInstallAgent } from '../../shared/telemetry-events' -import type { AgentHookInstallStatus } from '../../shared/agent-hook-types' import { track } from '../telemetry/client' // Why: install errors are about config-file shape (malformed JSON, ACL @@ -15,7 +14,6 @@ import { track } from '../telemetry/client' // so truncation must happen here at the call site. const ERROR_MESSAGE_MAX_LEN = 200 -export type ManagedHookInstaller = readonly [HookInstallAgent, () => AgentHookInstallStatus] export type ManagedHookInstallErrorRecorder = (agent: HookInstallAgent, error: unknown) => void function describeError(error: unknown): string { @@ -33,20 +31,10 @@ function describeError(error: unknown): string { } } -function errorStatus(agent: HookInstallAgent, error: unknown): AgentHookInstallStatus { - return { - agent, - state: 'error', - configPath: '', - managedHooksPresent: false, - detail: describeError(error) - } -} - export function recordManagedHookInstallFailure(agent: HookInstallAgent, error: unknown): void { // Why: telemetry must not break fail-open. A throw inside `track` (e.g. // a corrupted settings store the resolveConsent path reads from) would - // otherwise abort the for-loop and skip later agents' installers. + // otherwise abort the installer loop and skip later agents' installers. try { track('agent_hook_install_failed', { agent, @@ -56,24 +44,3 @@ export function recordManagedHookInstallFailure(agent: HookInstallAgent, error: console.error('[agent-hooks] Failed to record install-failure telemetry:', telemetryError) } } - -export function runManagedHookInstallers( - installers: readonly ManagedHookInstaller[], - onInstallError: ManagedHookInstallErrorRecorder = recordManagedHookInstallFailure -): AgentHookInstallStatus[] { - const results: AgentHookInstallStatus[] = [] - for (const [agent, install] of installers) { - try { - results.push(install()) - } catch (error) { - console.error(`[agent-hooks] Failed to install ${agent} managed hooks:`, error) - try { - onInstallError(agent, error) - } catch (telemetryError) { - console.error('[agent-hooks] Failed to record install-failure telemetry:', telemetryError) - } - results.push(errorStatus(agent, error)) - } - } - return results -} diff --git a/src/main/agent-hooks/managed-agent-hook-controls.test.ts b/src/main/agent-hooks/managed-agent-hook-controls.test.ts index 6699e1bc876..f84cdc986d6 100644 --- a/src/main/agent-hooks/managed-agent-hook-controls.test.ts +++ b/src/main/agent-hooks/managed-agent-hook-controls.test.ts @@ -1,4 +1,4 @@ -import { beforeEach, describe, expect, it, vi } from 'vitest' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' const { installCodex, installClaude, removeCodex, removeClaude, statusCodex, statusClaude } = vi.hoisted(() => ({ @@ -47,6 +47,8 @@ import { describe('managed agent hook controls', () => { beforeEach(() => { vi.clearAllMocks() + // Fail-open paths log to console.error on install failure; keep test output clean. + vi.spyOn(console, 'error').mockImplementation(() => {}) installCodex.mockReturnValue({ agent: 'codex', state: 'installed', @@ -91,6 +93,10 @@ describe('managed agent hook controls', () => { }) }) + afterEach(() => { + vi.restoreAllMocks() + }) + it('installs only agents with launchable CLIs and returns skipped attempt results', async () => { detectLocalManagedAgentCliPresenceMock.mockResolvedValue({ codex: { state: 'found' }, @@ -155,6 +161,47 @@ describe('managed agent hook controls', () => { expect(results).toEqual([expect.objectContaining({ agent: 'codex', state: 'installed' })]) }) + it('is fail-open: an installer that throws still runs the others and reports an error status', async () => { + detectLocalManagedAgentCliPresenceMock.mockResolvedValue({ + codex: { state: 'found' }, + claude: { state: 'found' } + }) + installCodex.mockImplementation(() => { + throw new Error('codex config malformed') + }) + const onInstallError = vi.fn() + + const results = await installManagedAgentHooks({ agentCmdOverrides: {} }, { onInstallError }) + + expect(installClaude).toHaveBeenCalledTimes(1) + expect(onInstallError).toHaveBeenCalledWith('codex', expect.any(Error)) + expect(results).toEqual([ + expect.objectContaining({ agent: 'codex', state: 'error', detail: 'codex config malformed' }), + expect.objectContaining({ agent: 'claude', state: 'installed' }) + ]) + }) + + it('keeps installing later agents even when the error recorder itself throws', async () => { + detectLocalManagedAgentCliPresenceMock.mockResolvedValue({ + codex: { state: 'found' }, + claude: { state: 'found' } + }) + installCodex.mockImplementation(() => { + throw new Error('codex failed') + }) + const onInstallError = vi.fn(() => { + throw new Error('telemetry blew up') + }) + + await expect( + installManagedAgentHooks({ agentCmdOverrides: {} }, { onInstallError }) + ).resolves.toEqual([ + expect.objectContaining({ agent: 'codex', state: 'error' }), + expect.objectContaining({ agent: 'claude', state: 'installed' }) + ]) + expect(installClaude).toHaveBeenCalledTimes(1) + }) + it('keeps status reads config-authoritative instead of returning skipped states', () => { expect(getManagedAgentHookStatuses()).toEqual([ expect.objectContaining({ agent: 'codex', state: 'installed' }), diff --git a/src/main/codex/codex-launch-hook-upkeep.test.ts b/src/main/codex/codex-launch-hook-upkeep.test.ts new file mode 100644 index 00000000000..c515765b1b6 --- /dev/null +++ b/src/main/codex/codex-launch-hook-upkeep.test.ts @@ -0,0 +1,26 @@ +import { describe, expect, it } from 'vitest' +import type { AgentHookInstallState } from '../../shared/agent-hook-types' +import { codexLaunchNeedsDirectApprovalPromotion } from './codex-launch-hook-upkeep' + +function status(state: AgentHookInstallState): { state: AgentHookInstallState } { + return { state } +} + +describe('codexLaunchNeedsDirectApprovalPromotion', () => { + it('promotes directly when the managed install skipped codex (presence-gated out)', () => { + // Regression: a false-'missing' PATH probe or relative override must not drop + // #7896 approval promotion on a real Codex launch. + expect(codexLaunchNeedsDirectApprovalPromotion(status('skipped'))).toBe(true) + }) + + it('promotes directly when no codex status came back at all', () => { + expect(codexLaunchNeedsDirectApprovalPromotion(undefined)).toBe(true) + }) + + it.each(['installed', 'partial', 'error', 'not_installed'])( + 'does not double-promote when install() already ran (state=%s)', + (state) => { + expect(codexLaunchNeedsDirectApprovalPromotion(status(state))).toBe(false) + } + ) +}) diff --git a/src/main/codex/codex-launch-hook-upkeep.ts b/src/main/codex/codex-launch-hook-upkeep.ts new file mode 100644 index 00000000000..7ea003f4afb --- /dev/null +++ b/src/main/codex/codex-launch-hook-upkeep.ts @@ -0,0 +1,15 @@ +import type { AgentHookInstallStatus } from '../../shared/agent-hook-types' + +// Why: the per-launch Codex path runs the presence-gated managed install, but +// that install can be skipped (a false-'missing' PATH probe or a relative +// command override reporting 'unknown'). #7896 approval promotion must still run +// on every launch, so when the managed install did not run install() itself the +// caller must promote directly. A completed install() (installed/partial/error) +// already promoted before touching config, so promoting again would be wasted +// work — and falling back to refreshRuntimeUserHooks() would wrongly strip the +// managed hooks a false-'missing' can't see. +export function codexLaunchNeedsDirectApprovalPromotion( + codexStatus: Pick | undefined +): boolean { + return !codexStatus || codexStatus.state === 'skipped' +} diff --git a/src/main/codex/hook-service.ts b/src/main/codex/hook-service.ts index ef812ab7ff0..00a2ad8b8e5 100644 --- a/src/main/codex/hook-service.ts +++ b/src/main/codex/hook-service.ts @@ -838,6 +838,14 @@ export class CodexHookService { return { agent: 'codex', state, configPath, managedHooksPresent, detail } } + // Why: exposes only the #7896 approval-promotion side effect so the per-launch + // path can guarantee it runs even when presence-gating skips the managed + // install. Promotion is idempotent, so callers that also run install() (which + // promotes internally) stay correct without double work. + promoteRuntimeHookApprovals(): void { + promoteCodexRuntimeHookApprovalsToSystem() + } + install(): AgentHookInstallStatus { const configPath = getConfigPath() const scriptPath = getManagedScriptPath() diff --git a/src/main/index.ts b/src/main/index.ts index d25df1304de..69f09b63570 100644 --- a/src/main/index.ts +++ b/src/main/index.ts @@ -36,6 +36,7 @@ import { removeManagedAgentHooks } from './agent-hooks/managed-agent-hook-controls' import { recordManagedHookInstallFailure } from './agent-hooks/install-telemetry' +import type { AgentHookInstallStatus } from '../shared/agent-hook-types' import { initCohortClassifier } from './telemetry/cohort-classifier' import { initOnboardingCohortClassifier } from './telemetry/onboarding-cohort-classifier' import { resolveConsent } from './telemetry/consent' @@ -122,6 +123,7 @@ import { } from './codex-accounts/runtime-selection' import { normalizeClaudeRuntimeSelection } from './claude-accounts/runtime-selection' import { codexHookService } from './codex/hook-service' +import { codexLaunchNeedsDirectApprovalPromotion } from './codex/codex-launch-hook-upkeep' import { ClaudeAccountService } from './claude-accounts/service' import { ClaudeRuntimeAuthService } from './claude-accounts/runtime-auth-service' import { @@ -699,20 +701,29 @@ async function startServeAgentHookServer(): Promise { async function maintainCodexLaunchHooks(): Promise { const hooksEnabled = isAgentStatusHooksEnabled(store?.getSettings()) try { - const statuses = hooksEnabled - ? await installManagedAgentHooks(store?.getSettings(), { - shouldHydrateShellPath: app.isPackaged && process.platform !== 'win32', - onInstallError: recordManagedHookInstallFailure, - agents: ['codex'] - }) - : [codexHookService.refreshRuntimeUserHooks()] - const failedStatus = statuses.find((status) => status.state === 'error') - if (failedStatus) { + let launchStatus: AgentHookInstallStatus | undefined + if (hooksEnabled) { + const statuses = await installManagedAgentHooks(store?.getSettings(), { + shouldHydrateShellPath: app.isPackaged && process.platform !== 'win32', + onInstallError: recordManagedHookInstallFailure, + agents: ['codex'] + }) + launchStatus = statuses.find((status) => status.agent === 'codex') + // Why: presence-gating can skip the managed install; #7896 approval + // promotion must still run on every Codex launch, so promote directly when + // install() itself never ran (a completed install() already promoted). + if (codexLaunchNeedsDirectApprovalPromotion(launchStatus)) { + codexHookService.promoteRuntimeHookApprovals() + } + } else { + launchStatus = codexHookService.refreshRuntimeUserHooks() + } + if (launchStatus?.state === 'error') { console.warn( `[codex-hook-service] failed to ${ hooksEnabled ? 'refresh' : 'refresh user' } runtime hooks before launch`, - failedStatus.detail + launchStatus.detail ) } } catch (error) { diff --git a/src/relay/agent-hook-cli-presence.ts b/src/relay/agent-hook-cli-presence.ts index 7d40d3382c8..78339fd90f7 100644 --- a/src/relay/agent-hook-cli-presence.ts +++ b/src/relay/agent-hook-cli-presence.ts @@ -217,8 +217,13 @@ export async function detectAgentHookCliPresence( const request = parseRequest(params) const presence: AgentHookCliPresenceResponse['presence'] = {} if (process.platform === 'win32') { + // Why: the probe below is POSIX-only (login-shell PATH + X_OK), so on a + // Windows relay we can't determine presence — report 'unknown' rather than a + // false 'missing', which would wrongly claim the CLI is absent. Callers still + // skip install (only positive 'found' installs). Reachable only defensively: + // SSH sessions short-circuit Windows remotes before requesting presence. for (const agent of request.agents) { - presence[agent] = { state: 'missing' } + presence[agent] = { state: 'unknown' } } return { presence } } diff --git a/src/shared/agent-hook-types.ts b/src/shared/agent-hook-types.ts index fc9f0256520..8d0f30b2c10 100644 --- a/src/shared/agent-hook-types.ts +++ b/src/shared/agent-hook-types.ts @@ -28,7 +28,6 @@ export type AgentHookInstallSkipReason = | 'cli_presence_unknown' | 'hooks_disabled' | 'remote_presence_unavailable' - | 'unsupported_remote_platform' | 'remote_hook_unsupported' export type AgentHookInstallStatus = {