mirror of
https://github.com/stablyai/orca.git
synced 2026-09-22 00:02:31 +00:00
fix(macos): stop the daemon blocking on its own folder read
The daemon reads the requested cwd before forking a shell to report whether it can list it. That read is the one macOS gates, so on a folder the daemon is refused it could hold the daemon's event loop behind a prompt. It is awaited now, which leaves the blocking enumerator with no callers.
This commit is contained in:
@@ -1,4 +1,4 @@
|
||||
import { opendirSync, type Dir } from 'node:fs'
|
||||
import type { Dir } from 'node:fs'
|
||||
import { opendir } from 'node:fs/promises'
|
||||
|
||||
/** `denied` is the only outcome that proves a permission refusal; `other` keeps unknown errors apart. */
|
||||
@@ -39,25 +39,3 @@ export async function enumerateDirectoryOnce(path: string): Promise<DirectoryEnu
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* The blocking variant, for a process that has nothing else to serve while it waits. Never call it
|
||||
* from the app: on macOS this read is what raises the TCC prompt, and the prompt holds the syscall
|
||||
* until the user answers, which would take the event loop down with it for the whole time.
|
||||
*/
|
||||
export function enumerateDirectoryOnceSync(path: string): DirectoryEnumerationOutcome {
|
||||
let dir: Dir | undefined
|
||||
try {
|
||||
dir = opendirSync(path)
|
||||
dir.readSync()
|
||||
return 'ok'
|
||||
} catch (error) {
|
||||
return outcomeForError(error)
|
||||
} finally {
|
||||
try {
|
||||
dir?.closeSync()
|
||||
} catch {
|
||||
// A handle we cannot close says nothing about readability.
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -2,18 +2,20 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
|
||||
import type { SubprocessHandle } from './session-subprocess-handle'
|
||||
import { TerminalHost, type TerminalHostOptions } from './terminal-host'
|
||||
|
||||
const { opendirSyncMock } = vi.hoisted(() => ({ opendirSyncMock: vi.fn() }))
|
||||
vi.mock('node:fs', async (importOriginal) => ({
|
||||
const { opendirMock } = vi.hoisted(() => ({ opendirMock: vi.fn() }))
|
||||
// Async on purpose: on macOS this read is what raises the TCC prompt, which holds the syscall for
|
||||
// as long as the user leaves the sheet up.
|
||||
vi.mock('node:fs/promises', async (importOriginal) => ({
|
||||
...(await importOriginal<Record<string, unknown>>()),
|
||||
opendirSync: opendirSyncMock
|
||||
opendir: opendirMock
|
||||
}))
|
||||
|
||||
vi.mock('../pty-descendant-termination', () => ({ killWithDescendantSweep: vi.fn() }))
|
||||
|
||||
const closeSync = vi.fn()
|
||||
const close = vi.fn(async () => {})
|
||||
|
||||
function dirReading(readSync: () => unknown): { readSync: () => unknown; closeSync: () => void } {
|
||||
return { readSync, closeSync }
|
||||
function dirReading(read: () => unknown): { read: () => unknown; close: () => Promise<void> } {
|
||||
return { read, close }
|
||||
}
|
||||
|
||||
function failWith(code: string): never {
|
||||
@@ -48,8 +50,8 @@ describe('TerminalHost cwd readability verdict', () => {
|
||||
let platformDescriptor: PropertyDescriptor | undefined
|
||||
|
||||
beforeEach(() => {
|
||||
closeSync.mockReset()
|
||||
opendirSyncMock.mockReset().mockReturnValue(dirReading(() => ({ name: 'entry' })))
|
||||
close.mockReset()
|
||||
opendirMock.mockReset().mockReturnValue(dirReading(() => ({ name: 'entry' })))
|
||||
platformDescriptor = Object.getOwnPropertyDescriptor(process, 'platform')
|
||||
Object.defineProperty(process, 'platform', { configurable: true, value: 'linux' })
|
||||
const spawnSubprocess: TerminalHostOptions['spawnSubprocess'] = () => createMockSubprocess()
|
||||
@@ -74,39 +76,39 @@ describe('TerminalHost cwd readability verdict', () => {
|
||||
|
||||
it('reports an enumerable cwd as readable, and closes the handle', async () => {
|
||||
expect((await create('readable', '/work/repo')).cwdReadableByDaemon).toBe(true)
|
||||
expect(opendirSyncMock).toHaveBeenCalledWith('/work/repo')
|
||||
expect(closeSync).toHaveBeenCalled()
|
||||
expect(opendirMock).toHaveBeenCalledWith('/work/repo')
|
||||
expect(close).toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it('reports an empty directory as readable', async () => {
|
||||
opendirSyncMock.mockReturnValue(dirReading(() => null))
|
||||
opendirMock.mockReturnValue(dirReading(() => null))
|
||||
expect((await create('empty', '/work/empty')).cwdReadableByDaemon).toBe(true)
|
||||
})
|
||||
|
||||
// The #17696 shape: TCC refuses the daemon, and only a refusal may read as denial.
|
||||
it('reports EPERM on open as denied', async () => {
|
||||
opendirSyncMock.mockImplementation(() => failWith('EPERM'))
|
||||
opendirMock.mockImplementation(() => failWith('EPERM'))
|
||||
expect((await create('eperm', '/Users/alice/Documents/repo')).cwdReadableByDaemon).toBe(false)
|
||||
})
|
||||
|
||||
it('reports EACCES on the first read as denied, and still closes the handle', async () => {
|
||||
opendirSyncMock.mockReturnValue(dirReading(() => failWith('EACCES')))
|
||||
opendirMock.mockReturnValue(dirReading(() => failWith('EACCES')))
|
||||
expect((await create('eacces', '/Users/alice/Desktop/repo')).cwdReadableByDaemon).toBe(false)
|
||||
expect(closeSync).toHaveBeenCalled()
|
||||
expect(close).toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it('reports a missing cwd as readable — absence is not a permission denial', async () => {
|
||||
opendirSyncMock.mockImplementation(() => failWith('ENOENT'))
|
||||
opendirMock.mockImplementation(() => failWith('ENOENT'))
|
||||
expect((await create('enoent', '/definitely/not/a/real/dir')).cwdReadableByDaemon).toBe(true)
|
||||
})
|
||||
|
||||
it('reports a non-directory cwd as readable', async () => {
|
||||
opendirSyncMock.mockImplementation(() => failWith('ENOTDIR'))
|
||||
opendirMock.mockImplementation(() => failWith('ENOTDIR'))
|
||||
expect((await create('enotdir', '/work/repo/file.txt')).cwdReadableByDaemon).toBe(true)
|
||||
})
|
||||
|
||||
it('reports an unexpected failure as readable — it must not masquerade as denial', async () => {
|
||||
opendirSyncMock.mockImplementation(() => {
|
||||
opendirMock.mockImplementation(() => {
|
||||
throw new TypeError('opendir is not a function')
|
||||
})
|
||||
expect((await create('unexpected', '/work/repo')).cwdReadableByDaemon).toBe(true)
|
||||
@@ -114,7 +116,7 @@ describe('TerminalHost cwd readability verdict', () => {
|
||||
|
||||
it('omits the verdict when no cwd was requested', async () => {
|
||||
expect((await create('no-cwd')).cwdReadableByDaemon).toBeUndefined()
|
||||
expect(opendirSyncMock).not.toHaveBeenCalled()
|
||||
expect(opendirMock).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it('omits the verdict on attach to an existing session', async () => {
|
||||
|
||||
@@ -1,7 +1,7 @@
|
||||
import { buildStartupCommandSubmission } from '../../shared/startup-command-submission'
|
||||
import { resolvePtyOwnerBackend } from '../../shared/pty-owner-backend'
|
||||
import { getDaemonSessionResultMetadata } from './daemon-create-or-attach-result'
|
||||
import { enumerateDirectoryOnceSync } from './directory-enumeration-probe'
|
||||
import { enumerateDirectoryOnce } from './directory-enumeration-probe'
|
||||
import { normalizePtySize } from './daemon-pty-size'
|
||||
import { Session } from './session'
|
||||
import { shellPathSupportsPtyStartupBarrier } from './shell-ready'
|
||||
@@ -110,7 +110,8 @@ async function spawnAndPublishSession(
|
||||
): Promise<CreateOrAttachResult> {
|
||||
const { size, wslDistro } = ctx
|
||||
// Why before the fork: the shell's own cwd may already have fallen back, so probe the requested path.
|
||||
const cwdReadableByDaemon = opts.cwd && !wslDistro ? isCwdReadableByThisProcess(opts.cwd) : null
|
||||
const cwdReadableByDaemon =
|
||||
opts.cwd && !wslDistro ? await isCwdReadableByThisProcess(opts.cwd) : null
|
||||
const subprocess = await deps.spawnSubprocess({
|
||||
sessionId: opts.sessionId,
|
||||
cols: size.cols,
|
||||
@@ -227,6 +228,6 @@ function createSessionExitHandler(
|
||||
// Why enumeration: a shell's cwd listing is what TCC withholds, and it can withhold it while
|
||||
// `access()` still passes. Only a proven permission refusal reads as denial — a missing path or an
|
||||
// unexpected error reads as readable so it can never masquerade as one.
|
||||
function isCwdReadableByThisProcess(cwd: string): boolean {
|
||||
return enumerateDirectoryOnceSync(cwd) !== 'denied'
|
||||
async function isCwdReadableByThisProcess(cwd: string): Promise<boolean> {
|
||||
return (await enumerateDirectoryOnce(cwd)) !== 'denied'
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user