mirror of
https://github.com/stablyai/orca.git
synced 2026-10-17 16:02:55 +00:00
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 <help@stably.ai>
This commit is contained in:
committed by
Brennan Benson
co-authored by
Orca
parent
20fafda6cd
commit
a74bb7278e
@@ -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(
|
||||
|
||||
@@ -131,13 +131,8 @@ async function probePathCandidate(
|
||||
return false
|
||||
}
|
||||
|
||||
async function probeOverridePath(
|
||||
token: string,
|
||||
platform: NodeJS.Platform,
|
||||
homeDir: string,
|
||||
fileProbe: FileProbe
|
||||
): Promise<boolean> {
|
||||
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<void> {
|
||||
@@ -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
|
||||
|
||||
@@ -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' }
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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(() => {})
|
||||
|
||||
@@ -717,6 +717,11 @@ export class SshRelaySession {
|
||||
let sftp: Awaited<ReturnType<SshConnection['sftp']>> | 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(
|
||||
|
||||
Reference in New Issue
Block a user