From a74bb7278e665ffd2060d4edcc70b42780c47076 Mon Sep 17 00:00:00 2001 From: brennanb2025 Date: Sun, 5 Jul 2026 19:53:54 -0700 Subject: [PATCH] Address CodeRabbit review: presence-classification and cancellation fixes - Report agents omitted from the remote presence map as remote_presence_unavailable instead of cli_not_found; only a positive 'missing' probe means the CLI is absent. - Re-check shouldContinue after the SFTP open await in installManagedHooksOnRemote so a disposed/replaced session cannot mutate remote agent config. - Treat relative (non-home, non-absolute) command overrides as unknown presence instead of probing them against the main process CWD. Co-authored-by: Orca --- .../local-agent-cli-presence.test.ts | 17 ++++++++++ .../agent-hooks/local-agent-cli-presence.ts | 19 ++++++----- .../remote-managed-hook-installers.test.ts | 15 +++++++++ .../remote-managed-hook-installers.ts | 11 ++++--- src/main/ssh/ssh-relay-session.test.ts | 33 +++++++++++++++++++ src/main/ssh/ssh-relay-session.ts | 5 +++ 6 files changed, 88 insertions(+), 12 deletions(-) diff --git a/src/main/agent-hooks/local-agent-cli-presence.test.ts b/src/main/agent-hooks/local-agent-cli-presence.test.ts index 8c31ee8d776..ae0e248bd00 100644 --- a/src/main/agent-hooks/local-agent-cli-presence.test.ts +++ b/src/main/agent-hooks/local-agent-cli-presence.test.ts @@ -102,6 +102,23 @@ describe('detectLocalManagedAgentCliPresence', () => { expect(probe).toHaveBeenCalledWith('/home/orca/bin/codex') }) + it('reports relative override paths as unknown instead of probing the process CWD', async () => { + const probe = vi.fn(async () => true) + const result = await detectLocalManagedAgentCliPresence( + [codexTarget], + { agentCmdOverrides: { codex: 'bin/codex --profile work' } }, + { + pathEnv: '', + pathDelimiter: ':', + fileProbe: { isExecutableFile: probe }, + platform: 'linux' + } + ) + + expect(result.codex).toEqual({ state: 'unknown' }) + expect(probe).not.toHaveBeenCalled() + }) + it('honors PATHEXT for Windows PATH candidates', async () => { const probe = vi.fn(async (filePath: string) => filePath === 'C:\\Tools/codex.CMD') const result = await detectLocalManagedAgentCliPresence( diff --git a/src/main/agent-hooks/local-agent-cli-presence.ts b/src/main/agent-hooks/local-agent-cli-presence.ts index 3e89774f545..23611c30c98 100644 --- a/src/main/agent-hooks/local-agent-cli-presence.ts +++ b/src/main/agent-hooks/local-agent-cli-presence.ts @@ -131,13 +131,8 @@ async function probePathCandidate( return false } -async function probeOverridePath( - token: string, - platform: NodeJS.Platform, - homeDir: string, - fileProbe: FileProbe -): Promise { - return await fileProbe.isExecutableFile(expandHomePathToken(token, platform, homeDir)) +function isPlatformAbsolutePath(candidate: string, platform: NodeJS.Platform): boolean { + return platform === 'win32' ? path.win32.isAbsolute(candidate) : path.posix.isAbsolute(candidate) } async function maybeHydrateShellPath(options: DetectOptions): Promise { @@ -189,7 +184,15 @@ export async function detectLocalManagedAgentCliPresence( for (const target of targets) { const overrideToken = overrideTokenForAgent(settings, target, platform) if (overrideToken && hasPathSeparatorToken(overrideToken)) { - result[target.agent] = (await probeOverridePath(overrideToken, platform, homeDir, fileProbe)) + const expandedOverride = expandHomePathToken(overrideToken, platform, homeDir) + // Why: a relative override resolves against whichever CWD the agent is + // later launched from, so probing it against the main process's CWD + // would report presence for an unrelated directory. + if (!isPlatformAbsolutePath(expandedOverride, platform)) { + result[target.agent] = { state: 'unknown' } + continue + } + result[target.agent] = (await fileProbe.isExecutableFile(expandedOverride)) ? { state: 'found' } : { state: 'missing' } continue diff --git a/src/main/agent-hooks/remote-managed-hook-installers.test.ts b/src/main/agent-hooks/remote-managed-hook-installers.test.ts index 5eee9c44beb..aa2e7767013 100644 --- a/src/main/agent-hooks/remote-managed-hook-installers.test.ts +++ b/src/main/agent-hooks/remote-managed-hook-installers.test.ts @@ -73,6 +73,21 @@ describe('remote managed hook installers', () => { ]) }) + it('reports agents omitted from the presence map as unavailable, not missing', async () => { + const results = await installRemoteManagedAgentHooks({} as never, '/home/orca', { + codex: { state: 'found' } + }) + + expect(installClaudeRemote).not.toHaveBeenCalled() + expect(results).toContainEqual( + expect.objectContaining({ + agent: 'claude', + state: 'skipped', + skipReason: 'remote_presence_unavailable' + }) + ) + }) + it('skips unknown presence without mutating remote config', async () => { const results = await installRemoteManagedAgentHooks({} as never, '/home/orca', { codex: { state: 'unknown' } diff --git a/src/main/agent-hooks/remote-managed-hook-installers.ts b/src/main/agent-hooks/remote-managed-hook-installers.ts index 8379df55116..4b1c9435d82 100644 --- a/src/main/agent-hooks/remote-managed-hook-installers.ts +++ b/src/main/agent-hooks/remote-managed-hook-installers.ts @@ -65,14 +65,17 @@ export async function installRemoteManagedAgentHooks( } const presence = presenceByAgent[agent] if (presence?.state !== 'found') { + // Why: an omitted entry means the relay never reported a result, not + // that the CLI was positively absent — only 'missing' is a real miss. + const cliMissing = presence?.state === 'missing' results.push( skippedStatus( agent, remoteHome, - presence?.state === 'unknown' ? 'remote_presence_unavailable' : 'cli_not_found', - presence?.state === 'unknown' - ? 'Remote CLI presence unknown; managed hook install skipped.' - : 'Remote CLI not found; managed hook install skipped.' + cliMissing ? 'cli_not_found' : 'remote_presence_unavailable', + cliMissing + ? 'Remote CLI not found; managed hook install skipped.' + : 'Remote CLI presence unavailable; managed hook install skipped.' ) ) continue diff --git a/src/main/ssh/ssh-relay-session.test.ts b/src/main/ssh/ssh-relay-session.test.ts index bfb5383495e..a87df5c148f 100644 --- a/src/main/ssh/ssh-relay-session.test.ts +++ b/src/main/ssh/ssh-relay-session.test.ts @@ -230,6 +230,39 @@ describe('SshRelaySession', () => { expect(sftp.end).toHaveBeenCalledTimes(1) }) + it('does not mutate remote config when the session is disposed while SFTP opens', async () => { + process.env.ORCA_FEATURE_REMOTE_AGENT_HOOKS = '1' + let resolvePresence!: (value: { presence: { claude: { state: 'found' } } }) => void + const presencePromise = new Promise<{ presence: { claude: { state: 'found' } } }>((resolve) => { + resolvePresence = resolve + }) + muxRequestMock.mockImplementation(async (method: string) => { + if (method === REMOTE_AGENT_HOOK_CLI_PRESENCE_METHOD) { + return await presencePromise + } + if (method === 'session.resolveHome') { + return { resolvedPath: '/home/orca' } + } + return { ok: true } + }) + const sftp = { end: vi.fn() } + const { mockStore, mockPortForward, getMainWindow } = createMockDeps() + let session!: SshRelaySession + const mockConn = { + sftp: vi.fn().mockImplementation(async () => { + session.dispose() + return sftp + }) + } as unknown as SshConnection + session = new SshRelaySession('target-1', getMainWindow, mockStore, mockPortForward) + + await session.establish(mockConn) + resolvePresence({ presence: { claude: { state: 'found' } } }) + + await vi.waitFor(() => expect(sftp.end).toHaveBeenCalledTimes(1)) + expect(installRemoteManagedAgentHooksMock).not.toHaveBeenCalled() + }) + it('keeps SSH providers registered when remote CLI presence is unavailable', async () => { process.env.ORCA_FEATURE_REMOTE_AGENT_HOOKS = '1' const warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}) diff --git a/src/main/ssh/ssh-relay-session.ts b/src/main/ssh/ssh-relay-session.ts index 84bb6e470aa..9c0a96ea0c8 100644 --- a/src/main/ssh/ssh-relay-session.ts +++ b/src/main/ssh/ssh-relay-session.ts @@ -717,6 +717,11 @@ export class SshRelaySession { let sftp: Awaited> | null = null try { sftp = await this.requireReadyConnection().sftp() + // Why: the session can be disposed or replaced while sftp() is in + // flight; re-check before mutating remote agent config. + if (shouldContinue && !shouldContinue()) { + return + } await installRemoteManagedAgentHooks(sftp, remoteHome, presenceByAgent) } catch (error) { console.warn(