Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
67 changes: 55 additions & 12 deletions src/main/ipc/ssh-browse.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,13 @@ function createMockChannel(): EventEmitter & { stderr: EventEmitter } {
})
}

// The POSIX listing emits `<l|-><d|->/name` lines: `[ -d ]` (which follows
// symlinks, unlike `ls -p`) supplies the directory flag and `[ -h ]` the
// symlink flag. Names cannot contain `/`, so the first `/` ends the prefix.
function posixBrowseCommand(cdTarget: string): string {
return `cd ${cdTarget} && pwd && entries=$(command ls -1A) && printf '%s\\n' "$entries" | while IFS= read -r f; do if [ -h "$f" ]; then t=l; else t=-; fi; if [ -d "$f" ]; then d=d; else d=-; fi; printf '%s%s/%s\\n' "$t" "$d" "$f"; done`
}

// Recover the PowerShell script from a `powershell.exe ... -EncodedCommand <b64>`
// command so tests can assert on the actual (UTF-16LE) payload sent to the host.
function decodeEncodedCommand(command: string): string {
Expand Down Expand Up @@ -56,19 +63,19 @@ describe('registerSshBrowseHandler', () => {

const resultPromise = handler(null, { targetId: 'ssh-1', dirPath: '~' })
await Promise.resolve()
channel.emit('data', Buffer.from('/home/user\nsrc/\nREADME.md\nnotes file.txt\n'))
channel.emit('data', Buffer.from('/home/user\n-d/src\n--/README.md\n--/notes file.txt\n'))
channel.emit('exit', 0)
channel.emit('close')

await expect(resultPromise).resolves.toEqual({
resolvedPath: '/home/user',
entries: [
{ name: 'src', isDirectory: true },
{ name: 'notes file.txt', isDirectory: false },
{ name: 'README.md', isDirectory: false }
{ name: 'src', isDirectory: true, isSymlink: false },
{ name: 'notes file.txt', isDirectory: false, isSymlink: false },
{ name: 'README.md', isDirectory: false, isSymlink: false }
]
})
expect(exec).toHaveBeenCalledWith('cd "$HOME" && pwd && command ls -1Ap')
expect(exec).toHaveBeenCalledWith(posixBrowseCommand('"$HOME"'))
expect(channel.listenerCount('data')).toBe(0)
expect(channel.listenerCount('exit')).toBe(0)
expect(channel.listenerCount('close')).toBe(0)
Expand All @@ -77,6 +84,37 @@ describe('registerSshBrowseHandler', () => {
expect(channel.stderr.listenerCount('error')).toBe(0)
})

it('classifies remote symlinked directories as directories', async () => {
const channel = createMockChannel()
const exec = vi.fn().mockResolvedValue(channel)
const getConnectionManager = () => ({
getConnection: () => ({ exec })
})
registerSshBrowseHandler(getConnectionManager as never)

const resultPromise = handler(null, { targetId: 'ssh-1', dirPath: '/srv' })
await Promise.resolve()
// `[ -d ]` follows symlinks, so the remote loop flags `linked-dir` as a
// directory even though it is a symlink; `[ -h ]` marks the link itself.
channel.emit(
'data',
Buffer.from('/srv\nld/linked-dir\n-d/real-dir\nl-/broken-link\nl-/linked-file\n')
)
channel.emit('exit', 0)
channel.emit('close')

await expect(resultPromise).resolves.toEqual({
resolvedPath: '/srv',
entries: [
{ name: 'linked-dir', isDirectory: true, isSymlink: true },
{ name: 'real-dir', isDirectory: true, isSymlink: false },
{ name: 'broken-link', isDirectory: false, isSymlink: true },
{ name: 'linked-file', isDirectory: false, isSymlink: true }
]
})
expect(exec).toHaveBeenCalledWith(posixBrowseCommand("'/srv'"))
})

it('escapes remote browse paths before invoking command ls', async () => {
const channel = createMockChannel()
const exec = vi.fn().mockResolvedValue(channel)
Expand All @@ -95,7 +133,7 @@ describe('registerSshBrowseHandler', () => {
resolvedPath: "/tmp/it's here",
entries: []
})
expect(exec).toHaveBeenCalledWith("cd '/tmp/it'\\''s here' && pwd && command ls -1Ap")
expect(exec).toHaveBeenCalledWith(posixBrowseCommand("'/tmp/it'\\''s here'"))
})

it('falls back to PowerShell when a Windows SSH shell rejects POSIX exec', async () => {
Expand All @@ -122,19 +160,19 @@ describe('registerSshBrowseHandler', () => {
// aren't misclassified as files with a stray carriage return in the name.
// The script emits a forward-slash resolvedPath (the -replace '\\','/' line)
// so the renderer's parentPath/joinPath, which only split on `/`, still work.
windowsChannel.emit('data', Buffer.from('C:/Users/alice\r\nDesktop/\r\nnotes.txt\r\n'))
windowsChannel.emit('data', Buffer.from('C:/Users/alice\r\n-d/Desktop\r\n--/notes.txt\r\n'))
windowsChannel.emit('exit', 0)
windowsChannel.emit('close')

await expect(resultPromise).resolves.toEqual({
resolvedPath: 'C:/Users/alice',
entries: [
{ name: 'Desktop', isDirectory: true },
{ name: 'notes.txt', isDirectory: false }
{ name: 'Desktop', isDirectory: true, isSymlink: false },
{ name: 'notes.txt', isDirectory: false, isSymlink: false }
]
})
expect(exec).toHaveBeenCalledTimes(2)
expect(exec).toHaveBeenNthCalledWith(1, "cd 'C:/Users/alice' && pwd && command ls -1Ap")
expect(exec).toHaveBeenNthCalledWith(1, posixBrowseCommand("'C:/Users/alice'"))
expect(exec.mock.calls[1]?.[0]).toMatch(/^powershell\.exe /)
expect(exec.mock.calls[1]?.[1]).toEqual({ wrapCommand: false })

Expand All @@ -148,6 +186,11 @@ describe('registerSshBrowseHandler', () => {
// resolvedPath must be emitted with forward slashes so the renderer's
// parentPath/joinPath (which only split on `/`) keep working on Windows.
expect(script).toContain("Write-Output ($resolved -replace '\\\\', '/')")
// Use LinkType (populated only for symlinks/junctions) rather than the
// generic ReparsePoint attribute, which is also set on OneDrive/cloud
// placeholders and would mislabel ordinary files as links.
expect(script).toContain('$_.LinkType')
expect(script).not.toContain('ReparsePoint')
})

it('falls back for a non-English cmd.exe reject (exit 1, localized stderr)', async () => {
Expand Down Expand Up @@ -176,13 +219,13 @@ describe('registerSshBrowseHandler', () => {
await vi.waitFor(() => {
expect(windowsChannel.listenerCount('close')).toBe(1)
})
windowsChannel.emit('data', Buffer.from('C:/Users\r\nAdmin/\r\n'))
windowsChannel.emit('data', Buffer.from('C:/Users\r\n-d/Admin\r\n'))
windowsChannel.emit('exit', 0)
windowsChannel.emit('close')

await expect(resultPromise).resolves.toEqual({
resolvedPath: 'C:/Users',
entries: [{ name: 'Admin', isDirectory: true }]
entries: [{ name: 'Admin', isDirectory: true, isSymlink: false }]
})
expect(exec).toHaveBeenCalledTimes(2)
})
Expand Down
42 changes: 30 additions & 12 deletions src/main/ipc/ssh-browse.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,14 @@ import { powerShellCommand, powerShellLiteral } from '../ssh/ssh-remote-powershe
export type RemoteDirEntry = {
name: string
isDirectory: boolean
isSymlink: boolean
}

// Listing lines are `<l|-><d|->/name`: symlink flag, directory flag, then the
// entry name. Names cannot contain `/`, so the first `/` always ends the
// prefix; anything else on stdout (motd noise, blank lines) is skipped.
const BROWSE_ENTRY_LINE = /^([l-])([d-])\/(.*)$/

const SSH_BROWSE_TIMEOUT_MS = 15_000

// Why: a POSIX login shell that can't find powershell.exe exits 127 (the POSIX
Expand Down Expand Up @@ -95,12 +101,19 @@ function browseWithPosixShell(
): Promise<{ entries: RemoteDirEntry[]; resolvedPath: string }> {
// Why: using one line per entry preserves filenames containing spaces.
// `command ls` bypasses user aliases/functions like `ls='eza ...'`.
// The -1 flag outputs one entry per line. The -p flag appends / to directories.
// We resolve ~ and get the absolute path via `cd <path> && pwd`.
// `cd` and `ls` are chained with `&&` so a failing `ls` (e.g. permission
// denied after a readable `cd ... && pwd`) propagates as a non-zero exit
// code rather than being indistinguishable from an empty directory.
return runBrowseCommand(conn, `cd ${shellEscape(dirPath)} && pwd && command ls -1Ap`)
// Each entry is emitted as `<l|-><d|->/name`: the directory flag comes from
// `[ -d ]`, which follows symlinks (unlike `ls -p`, which would list a
// symlinked directory as a file), and `[ -h ]` flags the link itself so the
// renderer can show a symlink indicator. Capturing `ls` output into a
// variable keeps a failing `ls` (e.g. permission denied after a readable
// `cd ... && pwd`) propagating through the `&&` chain as a non-zero exit
// code rather than being masked by the while-loop pipeline and misread as
// an empty directory.
return runBrowseCommand(
conn,
`cd ${shellEscape(dirPath)} && pwd && entries=$(command ls -1A) && printf '%s\\n' "$entries" | while IFS= read -r f; do if [ -h "$f" ]; then t=l; else t=-; fi; if [ -d "$f" ]; then d=d; else d=-; fi; printf '%s%s/%s\\n' "$t" "$d" "$f"; done`
)
}

function browseWithWindowsPowerShell(
Expand All @@ -122,7 +135,12 @@ function browseWithWindowsPowerShell(
// native $resolved for Get-ChildItem -LiteralPath.
"Write-Output ($resolved -replace '\\\\', '/')",
'Get-ChildItem -LiteralPath $resolved -Force | ForEach-Object {',
" if ($_.PSIsContainer) { Write-Output ($_.Name + '/') } else { Write-Output $_.Name }",
// Why: LinkType is populated only for symlinks/junctions, matching Node's
// isSymbolicLink() on the local/server path. The generic ReparsePoint
// attribute would also flag OneDrive/cloud placeholders as links.
" $t = if ($_.LinkType) { 'l' } else { '-' }",
" $d = if ($_.PSIsContainer) { 'd' } else { '-' }",
" Write-Output ($t + $d + '/' + $_.Name)",
'}'
].join('; ')

Expand Down Expand Up @@ -240,15 +258,15 @@ async function runBrowseCommand(
const entries: RemoteDirEntry[] = []

for (let i = 1; i < lines.length; i++) {
const line = lines[i]
if (!line || line === './' || line === '../') {
const match = BROWSE_ENTRY_LINE.exec(lines[i])
if (!match) {
continue
}
if (line.endsWith('/')) {
entries.push({ name: line.slice(0, -1), isDirectory: true })
} else {
entries.push({ name: line, isDirectory: false })
const name = match[3]
if (!name || name === '.' || name === '..') {
continue
}
entries.push({ name, isDirectory: match[2] === 'd', isSymlink: match[1] === 'l' })
}

// Sort: directories first, then alphabetical
Expand Down
30 changes: 29 additions & 1 deletion src/main/runtime/orca-runtime.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import { EventEmitter } from 'node:events'
import { randomUUID } from 'node:crypto'
import { execFileSync } from 'node:child_process'
import { mkdirSync } from 'node:fs'
import { lstat, mkdir, mkdtemp, rm, writeFile } from 'node:fs/promises'
import { lstat, mkdir, mkdtemp, rm, symlink, writeFile } from 'node:fs/promises'
import { homedir, tmpdir } from 'node:os'
import { join, win32 } from 'node:path'
import { ipcMain } from 'electron'
Expand Down Expand Up @@ -5869,6 +5869,34 @@ describe('OrcaRuntimeService', () => {
}
})

it('classifies symlinked directories as directories when browsing server dirs', async () => {
const tempRoot = await mkdtemp(join(tmpdir(), 'orca-runtime-browse-symlink-'))
try {
await mkdir(join(tempRoot, 'real-dir'))
await writeFile(join(tempRoot, 'real-file.txt'), 'contents\n')
await symlink(
join(tempRoot, 'real-dir'),
join(tempRoot, 'linked-dir'),
process.platform === 'win32' ? 'junction' : 'dir'
)
await symlink(join(tempRoot, 'real-file.txt'), join(tempRoot, 'linked-file'))
await symlink(join(tempRoot, 'missing-target'), join(tempRoot, 'broken-link'))
const runtime = new OrcaRuntimeService(store)

const result = await runtime.browseServerDir(tempRoot)

expect(result.entries).toEqual([
{ name: 'linked-dir', isDirectory: true, isSymlink: true },
{ name: 'real-dir', isDirectory: true, isSymlink: false },
{ name: 'broken-link', isDirectory: false, isSymlink: true },
{ name: 'linked-file', isDirectory: false, isSymlink: true },
{ name: 'real-file.txt', isDirectory: false, isSymlink: false }
])
} finally {
await rm(tempRoot, { recursive: true, force: true })
}
})

it('defaults runtime addRepo badgeColor to DEFAULT_REPO_BADGE_COLOR', async () => {
const added: Record<string, unknown>[] = []
const colorStore = {
Expand Down
35 changes: 28 additions & 7 deletions src/main/runtime/orca-runtime.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1902,6 +1902,25 @@ async function pathExists(pathValue: string): Promise<boolean> {
}
}

async function isServerBrowseEntryDirectory(
dirPath: string,
entry: { name: string; isDirectory(): boolean; isSymbolicLink(): boolean }
): Promise<boolean> {
if (entry.isDirectory()) {
return true
}
if (!entry.isSymbolicLink()) {
return false
}
try {
// Why: the Add Project browser must let users descend into symlinked
// directories, so classify links by their target type like relay fs.readDir.
return (await stat(join(dirPath, entry.name))).isDirectory()
} catch {
return false
}
}

function resolveServerBrowsePath(pathValue: string): string {
const trimmed = pathValue.trim() || '~'
if (trimmed.includes('\0')) {
Expand Down Expand Up @@ -12337,13 +12356,15 @@ export class OrcaRuntimeService {
throw new Error(`${dirPath} is not a directory`)
}
const entries = await readdir(dirPath, { withFileTypes: true })
const mapped = entries
.filter((entry) => entry.name !== '.' && entry.name !== '..')
.map((entry) => ({
name: entry.name,
isDirectory: entry.isDirectory(),
isSymlink: entry.isSymbolicLink()
}))
const mapped = await Promise.all(
entries
.filter((entry) => entry.name !== '.' && entry.name !== '..')
.map(async (entry) => ({
name: entry.name,
isDirectory: await isServerBrowseEntryDirectory(dirPath, entry),
isSymlink: entry.isSymbolicLink()
}))
)
Comment on lines +12359 to +12367

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

Limit concurrency to prevent EMFILE errors on directories with many symlinks.

Using an unbounded Promise.all to stat all symlinks concurrently can trigger EMFILE (too many open files) errors if the directory contains a large number of symbolic links. Because isServerBrowseEntryDirectory catches all errors and silently returns false, affected symlinked directories will incorrectly appear as non-directories in the UI.

Consider processing the entries sequentially to cap concurrent file descriptors.

💡 Proposed fix using sequential processing
-    const mapped = await Promise.all(
-      entries
-        .filter((entry) => entry.name !== '.' && entry.name !== '..')
-        .map(async (entry) => ({
-          name: entry.name,
-          isDirectory: await isServerBrowseEntryDirectory(dirPath, entry),
-          isSymlink: entry.isSymbolicLink()
-        }))
-    )
+    const mapped: DirEntry[] = []
+    for (const entry of entries) {
+      if (entry.name === '.' || entry.name === '..') {
+        continue
+      }
+      mapped.push({
+        name: entry.name,
+        isDirectory: await isServerBrowseEntryDirectory(dirPath, entry),
+        isSymlink: entry.isSymbolicLink()
+      })
+    }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const mapped = await Promise.all(
entries
.filter((entry) => entry.name !== '.' && entry.name !== '..')
.map(async (entry) => ({
name: entry.name,
isDirectory: await isServerBrowseEntryDirectory(dirPath, entry),
isSymlink: entry.isSymbolicLink()
}))
)
const mapped: DirEntry[] = []
for (const entry of entries) {
if (entry.name === '.' || entry.name === '..') {
continue
}
mapped.push({
name: entry.name,
isDirectory: await isServerBrowseEntryDirectory(dirPath, entry),
isSymlink: entry.isSymbolicLink()
})
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This intentionally mirrors the existing fs.readDir handler in src/relay/fs-handler.ts (lines 141–147), which uses the same unbounded Promise.all over isDirectoryEntry and is test-locked — browseServerDir was written to match that behavior.

The concurrency here is bounded by the number of symlinked entries in a single browsed directory (non-symlinks skip stat entirely), so for a project-picker listing it isn't a realistic EMFILE trigger. Adding a concurrency cap to only this side would diverge from the relay twin it deliberately matches; if the fd concern is worth addressing, it's really a pre-existing, codebase-wide item that should be a separate change applied to both handlers rather than a divergence introduced here.

Leaving as-is for parity with the established handler.

mapped.sort((a, b) => {
if (a.isDirectory !== b.isDirectory) {
return a.isDirectory ? -1 : 1
Expand Down
2 changes: 1 addition & 1 deletion src/preload/api-types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3107,7 +3107,7 @@ export type PreloadApi = {
callback: (data: { targetId: string; ports: EnrichedDetectedPort[] }) => void
) => () => void
browseDir: (args: { targetId: string; dirPath: string }) => Promise<{
entries: { name: string; isDirectory: boolean }[]
entries: { name: string; isDirectory: boolean; isSymlink: boolean }[]
resolvedPath: string
}>
onCredentialRequest: (
Expand Down
2 changes: 1 addition & 1 deletion src/preload/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4219,7 +4219,7 @@ const api = {
targetId: string
dirPath: string
}): Promise<{
entries: { name: string; isDirectory: boolean }[]
entries: { name: string; isDirectory: boolean; isSymlink: boolean }[]
resolvedPath: string
}> => ipcRenderer.invoke('ssh:browseDir', args),

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,8 @@ type BrowseDirArgs = {

const browseDir = vi.fn(async ({ dirPath }: BrowseDirArgs) => ({
entries: [
{ name: 'src', isDirectory: true },
{ name: 'README.md', isDirectory: false }
{ name: 'src', isDirectory: true, isSymlink: false },
{ name: 'README.md', isDirectory: false, isSymlink: false }
],
resolvedPath: dirPath === '~' ? '/home/alice' : dirPath
}))
Expand Down
Loading