mirror of
https://github.com/stablyai/orca.git
synced 2026-09-22 08:02:28 +00:00
Fix remote-host file open denied during SSH hydration (#6648)
Opening a file to edit on a remote SSH host could fail with "Access denied: path resolves outside allowed directories" and stay stuck. Right after a session restore the SSH repo has not hydrated, so the owner lookup returns undefined ("owner unknown"); the editor treated that like null ("local") and read the remote path off the local filesystem, then the retry gate excluded "access denied" so the error latched permanently.
Distinguish "owner not yet known" from "definitely local": when the worktree's backing repo has not hydrated, fail with a retryable owner-not-ready error instead of a local read, and retry it on a steady cadence bounded to ~2 min. If the host never connects, surface a truthful terminal message with a Retry button rather than retrying forever; a successful read at any point recovers immediately. Hydrated local repos resolve to null and are unaffected. The defect is platform-agnostic; Windows just surfaces the race most often.
Closes #6648
This commit is contained in:
@@ -1,5 +1,23 @@
|
||||
import type { GitDiffResult } from '../../../../shared/types'
|
||||
|
||||
/**
|
||||
* Thrown when a worktree's host owner is not yet known (the backing repo has
|
||||
* not hydrated). The retry gate treats this as transient so the read recovers
|
||||
* once the SSH connection finishes establishing, instead of latching a local
|
||||
* "access denied" for a remote path (#6648).
|
||||
*/
|
||||
export const WORKTREE_OWNER_NOT_READY_ERROR =
|
||||
'Connecting to the remote host… retrying once the workspace is ready.'
|
||||
|
||||
/**
|
||||
* Terminal message shown once the owner-not-ready retry budget is exhausted —
|
||||
* the remote host never finished connecting. Truthful (no longer claims it is
|
||||
* still retrying) and points the user at the Retry button, which starts a fresh
|
||||
* budget (#6648).
|
||||
*/
|
||||
export const WORKTREE_OWNER_UNREACHABLE_ERROR =
|
||||
"Couldn't reach the remote host. Check the connection, then retry."
|
||||
|
||||
export type FileContent = {
|
||||
content: string
|
||||
isBinary: boolean
|
||||
|
||||
@@ -12,6 +12,7 @@ const mocks = vi.hoisted(() => ({
|
||||
getRuntimeGitDiff: vi.fn(),
|
||||
getConnectionId: vi.fn(),
|
||||
getConnectionIdForFile: vi.fn(),
|
||||
isWorktreeConnectionResolved: vi.fn(() => true),
|
||||
getState: vi.fn()
|
||||
}))
|
||||
|
||||
@@ -35,7 +36,8 @@ vi.mock('@/runtime/runtime-git-client', () => ({
|
||||
|
||||
vi.mock('@/lib/connection-context', () => ({
|
||||
getConnectionId: mocks.getConnectionId,
|
||||
getConnectionIdForFile: mocks.getConnectionIdForFile
|
||||
getConnectionIdForFile: mocks.getConnectionIdForFile,
|
||||
isWorktreeConnectionResolved: mocks.isWorktreeConnectionResolved
|
||||
}))
|
||||
|
||||
vi.mock('@/store', () => ({
|
||||
@@ -130,6 +132,8 @@ describe('useEditorPanelContentState', () => {
|
||||
mocks.getConnectionId.mockReturnValue(undefined)
|
||||
mocks.getConnectionIdForFile.mockReset()
|
||||
mocks.getConnectionIdForFile.mockReturnValue(undefined)
|
||||
mocks.isWorktreeConnectionResolved.mockReset()
|
||||
mocks.isWorktreeConnectionResolved.mockReturnValue(true)
|
||||
mocks.getState.mockReset()
|
||||
mocks.getState.mockReturnValue({ settings: null })
|
||||
})
|
||||
@@ -177,6 +181,50 @@ describe('useEditorPanelContentState', () => {
|
||||
)
|
||||
})
|
||||
|
||||
it('does not read locally while a remote host worktree owner is still hydrating (#6648)', async () => {
|
||||
const activeFile = createOpenFile({
|
||||
filePath: '/home/user/project/src/index.ts',
|
||||
relativePath: 'src/index.ts',
|
||||
worktreeId: 'repo-ssh::/home/user/project'
|
||||
})
|
||||
// Owner unknown (SSH repo not hydrated): connection unresolved + not ready.
|
||||
mocks.getConnectionIdForFile.mockReturnValue(undefined)
|
||||
mocks.isWorktreeConnectionResolved.mockReturnValue(false)
|
||||
|
||||
container = document.createElement('div')
|
||||
document.body.appendChild(container)
|
||||
root = createRoot(container)
|
||||
|
||||
await act(async () => {
|
||||
root?.render(<HookProbe activeFile={activeFile} openFiles={[activeFile]} />)
|
||||
})
|
||||
|
||||
// Surfaces a retryable owner-not-ready error instead of a terminal local
|
||||
// "access denied", and never attempts the bad local read.
|
||||
await vi.waitFor(() => expect(latestFileContents[activeFile.id]?.loadError).toBeTruthy())
|
||||
expect(latestFileContents[activeFile.id]?.loadError).not.toMatch(/access denied/i)
|
||||
expect(mocks.readRuntimeFileContent).not.toHaveBeenCalled()
|
||||
|
||||
// The SSH repo finishes hydrating: the worktree owner resolves to its
|
||||
// target. We do NOT bump the reload nonce here — the retry hook must
|
||||
// re-attempt the read on its own once the owner-not-ready error clears.
|
||||
mocks.isWorktreeConnectionResolved.mockReturnValue(true)
|
||||
mocks.getConnectionIdForFile.mockReturnValue('ssh-target-1')
|
||||
mocks.readRuntimeFileContent.mockResolvedValue({ content: 'remote', isBinary: false })
|
||||
|
||||
// Driven purely by the automatic retry (no re-render, no forced reload).
|
||||
await vi.waitFor(() => expect(latestFileContents[activeFile.id]?.content).toBe('remote'), {
|
||||
timeout: 3000
|
||||
})
|
||||
expect(mocks.readRuntimeFileContent).toHaveBeenCalledWith(
|
||||
expect.objectContaining({
|
||||
filePath: '/home/user/project/src/index.ts',
|
||||
worktreeId: 'repo-ssh::/home/user/project',
|
||||
connectionId: 'ssh-target-1'
|
||||
})
|
||||
)
|
||||
})
|
||||
|
||||
it('reloads a clean file when its file content reload nonce changes', async () => {
|
||||
const activeFile = createOpenFile()
|
||||
mocks.readRuntimeFileContent
|
||||
|
||||
@@ -3,7 +3,11 @@
|
||||
make the hook coordination harder to audit. */
|
||||
import { useCallback, useEffect, useMemo, useRef, useState } from 'react'
|
||||
import type { OpenFile } from '@/store/slices/editor'
|
||||
import { getConnectionId, getConnectionIdForFile } from '@/lib/connection-context'
|
||||
import {
|
||||
getConnectionId,
|
||||
getConnectionIdForFile,
|
||||
isWorktreeConnectionResolved
|
||||
} from '@/lib/connection-context'
|
||||
import { joinPath } from '@/lib/path'
|
||||
import { useAppStore } from '@/store'
|
||||
import { getRuntimeFileReadScope, readRuntimeFileContent } from '@/runtime/runtime-file-client'
|
||||
@@ -14,7 +18,11 @@ import {
|
||||
getRuntimeGitDiff,
|
||||
getRuntimeGitScope
|
||||
} from '@/runtime/runtime-git-client'
|
||||
import type { DiffContent, FileContent } from './editor-panel-content-types'
|
||||
import {
|
||||
WORKTREE_OWNER_NOT_READY_ERROR,
|
||||
type DiffContent,
|
||||
type FileContent
|
||||
} from './editor-panel-content-types'
|
||||
import { canUseChangesModeForFile } from './editor-panel-file-mode'
|
||||
import {
|
||||
isReloadableSingleFileDiffTab,
|
||||
@@ -105,13 +113,24 @@ export function useEditorPanelContentState({
|
||||
fileReadGenerationCounterRef.current = generation
|
||||
fileReadGenerationRef.current[id] = generation
|
||||
try {
|
||||
const connectionId = getConnectionIdForFile(worktreeId ?? null, filePath) ?? undefined
|
||||
const resolvedConnectionId = getConnectionIdForFile(worktreeId ?? null, filePath)
|
||||
const connectionId = resolvedConnectionId ?? undefined
|
||||
const restoredOpenFile = openFilesRef.current.find((file) => file.id === id)
|
||||
const activeSettings = useAppStore.getState().settings
|
||||
const readSettings = settingsForRuntimeOwner(
|
||||
activeSettings,
|
||||
restoredOpenFile?.runtimeEnvironmentId
|
||||
)
|
||||
if (
|
||||
resolvedConnectionId === undefined &&
|
||||
!readSettings?.activeRuntimeEnvironmentId?.trim() &&
|
||||
!isWorktreeConnectionResolved(worktreeId ?? null)
|
||||
) {
|
||||
// Why: the backing repo hasn't hydrated yet (SSH still connecting), so
|
||||
// we can't tell local from remote. Reading locally would deny a remote
|
||||
// path with a terminal "access denied" (#6648); fail retryably instead.
|
||||
throw new Error(WORKTREE_OWNER_NOT_READY_ERROR)
|
||||
}
|
||||
if (restoredOpenFile?.filePath === filePath && restoredOpenFile.relativePath === filePath) {
|
||||
if (readSettings?.activeRuntimeEnvironmentId?.trim() || connectionId) {
|
||||
// Why: restored external-file tabs contain client-local absolute
|
||||
|
||||
@@ -0,0 +1,203 @@
|
||||
// @vitest-environment happy-dom
|
||||
|
||||
import { act } from 'react'
|
||||
import { createRoot, type Root } from 'react-dom/client'
|
||||
import { afterEach, beforeEach, describe, expect, it, vi, type MockInstance } from 'vitest'
|
||||
import type { OpenFile } from '@/store/slices/editor'
|
||||
import {
|
||||
WORKTREE_OWNER_NOT_READY_ERROR,
|
||||
WORKTREE_OWNER_UNREACHABLE_ERROR,
|
||||
type FileContent
|
||||
} from './editor-panel-content-types'
|
||||
import {
|
||||
OWNER_NOT_READY_RETRY_LIMIT,
|
||||
shouldRetryFileLoadError,
|
||||
useEditorPanelFileLoadRetry
|
||||
} from './useEditorPanelFileLoadRetry'
|
||||
|
||||
// Why: the real setFileContents replaces the map; a key the hook deletes must
|
||||
// disappear. A merge (Object.assign) would silently keep a stale loadError.
|
||||
function replaceFileContents(
|
||||
target: Record<string, FileContent>,
|
||||
next: Record<string, FileContent>
|
||||
): void {
|
||||
for (const key of Object.keys(target)) {
|
||||
delete target[key]
|
||||
}
|
||||
Object.assign(target, next)
|
||||
}
|
||||
|
||||
function makeFile(overrides: Partial<OpenFile> = {}): OpenFile {
|
||||
return {
|
||||
id: 'tab-1',
|
||||
filePath: '/home/user/project/src/index.ts',
|
||||
relativePath: 'src/index.ts',
|
||||
worktreeId: 'repo-ssh::/home/user/project',
|
||||
language: 'typescript',
|
||||
isDirty: false,
|
||||
mode: 'edit',
|
||||
...overrides
|
||||
}
|
||||
}
|
||||
|
||||
// Drives the real hook with a controllable fileContents store, mirroring how
|
||||
// useEditorPanelContentState wires it up.
|
||||
function Harness({
|
||||
file,
|
||||
fileContents,
|
||||
attemptsRef,
|
||||
loadFileContent,
|
||||
setFileContents
|
||||
}: {
|
||||
file: OpenFile
|
||||
fileContents: Record<string, FileContent>
|
||||
attemptsRef: { current: Record<string, number> }
|
||||
loadFileContent: (filePath: string, id: string) => Promise<void>
|
||||
setFileContents: (
|
||||
updater: (prev: Record<string, FileContent>) => Record<string, FileContent>
|
||||
) => void
|
||||
}): null {
|
||||
useEditorPanelFileLoadRetry({
|
||||
activeFile: file,
|
||||
fileContents,
|
||||
fileLoadRetryAttemptsRef: attemptsRef,
|
||||
loadFileContent: loadFileContent as never,
|
||||
openFilesRef: { current: [file] },
|
||||
setFileContents: setFileContents as never
|
||||
})
|
||||
return null
|
||||
}
|
||||
|
||||
describe('useEditorPanelFileLoadRetry — owner-not-ready bounding (#6648)', () => {
|
||||
let container: HTMLDivElement | null = null
|
||||
let root: Root | null = null
|
||||
let setTimeoutSpy: MockInstance
|
||||
|
||||
beforeEach(() => {
|
||||
vi.useFakeTimers()
|
||||
// Run scheduled retries immediately so we can exhaust the budget without
|
||||
// waiting ~2 minutes of real time.
|
||||
setTimeoutSpy = vi.spyOn(window, 'setTimeout').mockImplementation(((fn: () => void) => {
|
||||
fn()
|
||||
return 0 as unknown as ReturnType<typeof setTimeout>
|
||||
}) as typeof window.setTimeout)
|
||||
})
|
||||
|
||||
afterEach(() => {
|
||||
if (root) {
|
||||
act(() => root?.unmount())
|
||||
}
|
||||
container?.remove()
|
||||
container = null
|
||||
root = null
|
||||
setTimeoutSpy.mockRestore()
|
||||
vi.useRealTimers()
|
||||
})
|
||||
|
||||
it('classifies retryable vs terminal errors', () => {
|
||||
expect(shouldRetryFileLoadError(WORKTREE_OWNER_NOT_READY_ERROR)).toBe(true)
|
||||
expect(shouldRetryFileLoadError(WORKTREE_OWNER_UNREACHABLE_ERROR)).toBe(false)
|
||||
expect(shouldRetryFileLoadError('Access denied: outside allowed directories')).toBe(false)
|
||||
})
|
||||
|
||||
it('stops after the budget and shows a truthful terminal message, then Retry re-arms', () => {
|
||||
const file = makeFile()
|
||||
const attemptsRef = { current: {} as Record<string, number> }
|
||||
// The owner never hydrates: every retry re-fails with owner-not-ready.
|
||||
const fileContents: Record<string, FileContent> = {
|
||||
[file.id]: { content: '', isBinary: false, loadError: WORKTREE_OWNER_NOT_READY_ERROR }
|
||||
}
|
||||
const setFileContents = (
|
||||
updater: (prev: Record<string, FileContent>) => Record<string, FileContent>
|
||||
): void => {
|
||||
replaceFileContents(fileContents, updater(fileContents))
|
||||
}
|
||||
// loadFileContent (the retry callback) clears then re-fails as owner-not-ready.
|
||||
const loadFileContent = vi.fn(async (_filePath: string, id: string) => {
|
||||
fileContents[id] = { content: '', isBinary: false, loadError: WORKTREE_OWNER_NOT_READY_ERROR }
|
||||
})
|
||||
|
||||
container = document.createElement('div')
|
||||
document.body.appendChild(container)
|
||||
root = createRoot(container)
|
||||
|
||||
// Re-render the hook repeatedly; each render that still sees the
|
||||
// owner-not-ready error schedules (and our spy immediately runs) one retry.
|
||||
for (let i = 0; i < OWNER_NOT_READY_RETRY_LIMIT + 2; i++) {
|
||||
act(() => {
|
||||
root?.render(
|
||||
<Harness
|
||||
file={file}
|
||||
fileContents={{ ...fileContents }}
|
||||
attemptsRef={attemptsRef}
|
||||
loadFileContent={loadFileContent}
|
||||
setFileContents={setFileContents}
|
||||
/>
|
||||
)
|
||||
})
|
||||
if (fileContents[file.id]?.loadError === WORKTREE_OWNER_UNREACHABLE_ERROR) {
|
||||
break
|
||||
}
|
||||
}
|
||||
|
||||
// Budget honored: retried at most the limit, then went terminal.
|
||||
expect(loadFileContent.mock.calls.length).toBeLessThanOrEqual(OWNER_NOT_READY_RETRY_LIMIT)
|
||||
expect(fileContents[file.id]?.loadError).toBe(WORKTREE_OWNER_UNREACHABLE_ERROR)
|
||||
|
||||
// The terminal error is not auto-retried.
|
||||
const callsAfterTerminal = loadFileContent.mock.calls.length
|
||||
act(() => {
|
||||
root?.render(
|
||||
<Harness
|
||||
file={file}
|
||||
fileContents={{ ...fileContents }}
|
||||
attemptsRef={attemptsRef}
|
||||
loadFileContent={loadFileContent}
|
||||
setFileContents={setFileContents}
|
||||
/>
|
||||
)
|
||||
})
|
||||
expect(loadFileContent.mock.calls.length).toBe(callsAfterTerminal)
|
||||
|
||||
// Retry (reloadFileContent) clears the attempt budget for a fresh start.
|
||||
delete attemptsRef.current[file.id]
|
||||
expect(attemptsRef.current[file.id]).toBeUndefined()
|
||||
})
|
||||
|
||||
it('stops immediately once the read succeeds (no terminal message)', () => {
|
||||
const file = makeFile()
|
||||
const attemptsRef = { current: {} as Record<string, number> }
|
||||
const fileContents: Record<string, FileContent> = {
|
||||
[file.id]: { content: '', isBinary: false, loadError: WORKTREE_OWNER_NOT_READY_ERROR }
|
||||
}
|
||||
const setFileContents = (
|
||||
updater: (prev: Record<string, FileContent>) => Record<string, FileContent>
|
||||
): void => {
|
||||
replaceFileContents(fileContents, updater(fileContents))
|
||||
}
|
||||
// The repo hydrates on the first retry: the read now succeeds.
|
||||
const loadFileContent = vi.fn(async (_filePath: string, id: string) => {
|
||||
fileContents[id] = { content: 'remote', isBinary: false }
|
||||
})
|
||||
|
||||
container = document.createElement('div')
|
||||
document.body.appendChild(container)
|
||||
root = createRoot(container)
|
||||
|
||||
act(() => {
|
||||
root?.render(
|
||||
<Harness
|
||||
file={file}
|
||||
fileContents={{ ...fileContents }}
|
||||
attemptsRef={attemptsRef}
|
||||
loadFileContent={loadFileContent}
|
||||
setFileContents={setFileContents}
|
||||
/>
|
||||
)
|
||||
})
|
||||
|
||||
expect(loadFileContent).toHaveBeenCalledTimes(1)
|
||||
expect(fileContents[file.id]?.loadError).toBeUndefined()
|
||||
expect(fileContents[file.id]?.content).toBe('remote')
|
||||
})
|
||||
})
|
||||
@@ -1,8 +1,23 @@
|
||||
import { useEffect, type Dispatch, type MutableRefObject, type SetStateAction } from 'react'
|
||||
import type { OpenFile } from '@/store/slices/editor'
|
||||
import type { FileContent } from './editor-panel-content-types'
|
||||
import {
|
||||
WORKTREE_OWNER_NOT_READY_ERROR,
|
||||
WORKTREE_OWNER_UNREACHABLE_ERROR,
|
||||
type FileContent
|
||||
} from './editor-panel-content-types'
|
||||
|
||||
const FILE_LOAD_RETRY_DELAYS_MS = [250, 1000, 2500]
|
||||
// Why: a remote host can take a while to finish connecting. The owner-not-ready
|
||||
// check is a pure local store read (it throws before any network call until the
|
||||
// SSH repo hydrates), so poll it at a steady cadence — but cap the wait so a
|
||||
// host that never connects ends in a truthful terminal message instead of
|
||||
// retrying forever. ~2 min covers any realistic connect; Retry re-arms it (#6648).
|
||||
export const OWNER_NOT_READY_RETRY_DELAY_MS = 750
|
||||
export const OWNER_NOT_READY_RETRY_LIMIT = 160
|
||||
|
||||
function isOwnerNotReadyError(message: string): boolean {
|
||||
return message === WORKTREE_OWNER_NOT_READY_ERROR
|
||||
}
|
||||
|
||||
type UseEditorPanelFileLoadRetryParams = {
|
||||
activeFile: OpenFile | null
|
||||
@@ -18,7 +33,12 @@ type UseEditorPanelFileLoadRetryParams = {
|
||||
setFileContents: Dispatch<SetStateAction<Record<string, FileContent>>>
|
||||
}
|
||||
|
||||
function shouldRetryFileLoadError(message: string): boolean {
|
||||
export function shouldRetryFileLoadError(message: string): boolean {
|
||||
// Terminal: the owner-not-ready budget is spent; only an explicit Retry should
|
||||
// restart it, never the automatic backoff.
|
||||
if (message === WORKTREE_OWNER_UNREACHABLE_ERROR) {
|
||||
return false
|
||||
}
|
||||
const lower = message.toLowerCase()
|
||||
return (
|
||||
!lower.includes('access denied') &&
|
||||
@@ -49,11 +69,35 @@ export function useEditorPanelFileLoadRetry({
|
||||
) {
|
||||
return
|
||||
}
|
||||
const ownerNotReady = isOwnerNotReadyError(activeFileLoadError)
|
||||
const retryCount = fileLoadRetryAttemptsRef.current[activeFileLoadRetryId] ?? 0
|
||||
if (retryCount >= FILE_LOAD_RETRY_DELAYS_MS.length) {
|
||||
const retryLimit = ownerNotReady
|
||||
? OWNER_NOT_READY_RETRY_LIMIT
|
||||
: FILE_LOAD_RETRY_DELAYS_MS.length
|
||||
if (retryCount >= retryLimit) {
|
||||
// Why: the remote host never finished connecting. Replace the transient
|
||||
// "still connecting" text with a truthful terminal message so it does not
|
||||
// look like it is still retrying; Retry starts a fresh budget (#6648).
|
||||
if (ownerNotReady) {
|
||||
setFileContents((prev) => {
|
||||
if (prev[activeFileLoadRetryId]?.loadError !== activeFileLoadError) {
|
||||
return prev
|
||||
}
|
||||
return {
|
||||
...prev,
|
||||
[activeFileLoadRetryId]: {
|
||||
content: '',
|
||||
isBinary: false,
|
||||
loadError: WORKTREE_OWNER_UNREACHABLE_ERROR
|
||||
}
|
||||
}
|
||||
})
|
||||
}
|
||||
return
|
||||
}
|
||||
const delayMs = FILE_LOAD_RETRY_DELAYS_MS[retryCount] ?? FILE_LOAD_RETRY_DELAYS_MS[0]
|
||||
const delayMs = ownerNotReady
|
||||
? OWNER_NOT_READY_RETRY_DELAY_MS
|
||||
: (FILE_LOAD_RETRY_DELAYS_MS[retryCount] ?? FILE_LOAD_RETRY_DELAYS_MS[0])
|
||||
fileLoadRetryAttemptsRef.current[activeFileLoadRetryId] = retryCount + 1
|
||||
const timeoutId = window.setTimeout(() => {
|
||||
const currentFile = openFilesRef.current.find((file) => file.id === activeFileLoadRetryId)
|
||||
|
||||
@@ -1,7 +1,11 @@
|
||||
import { afterEach, describe, expect, it } from 'vitest'
|
||||
import type { Repo } from '../../../shared/types'
|
||||
import { useAppStore } from '@/store'
|
||||
import { getConnectionId, getConnectionIdForFile } from './connection-context'
|
||||
import {
|
||||
getConnectionId,
|
||||
getConnectionIdForFile,
|
||||
isWorktreeConnectionResolved
|
||||
} from './connection-context'
|
||||
import { folderWorkspaceKey } from '../../../shared/workspace-scope'
|
||||
|
||||
const initialState = useAppStore.getInitialState()
|
||||
@@ -317,6 +321,25 @@ describe('getConnectionId', () => {
|
||||
expect(getConnectionId(folderWorkspaceKey('folder-workspace-1'))).toBe('ssh-1')
|
||||
})
|
||||
|
||||
it('reports a worktree owner as unresolved until its backing repo hydrates (#6648)', () => {
|
||||
useAppStore.setState({ repos: [], worktreesByRepo: {} })
|
||||
// SSH repo not yet in the store -> owner unknown, must not read locally.
|
||||
expect(isWorktreeConnectionResolved('repo-ssh::/home/neil/repo')).toBe(false)
|
||||
|
||||
useAppStore.setState({
|
||||
repos: [makeRepo({ id: 'repo-ssh', connectionId: 'ssh-1' })],
|
||||
worktreesByRepo: {}
|
||||
})
|
||||
expect(isWorktreeConnectionResolved('repo-ssh::/home/neil/repo')).toBe(true)
|
||||
})
|
||||
|
||||
it('treats null worktrees and folder workspaces as resolved owners', () => {
|
||||
useAppStore.setState({ repos: [], worktreesByRepo: {} })
|
||||
expect(isWorktreeConnectionResolved(null)).toBe(true)
|
||||
// Folder workspaces resolve per-file via getConnectionIdForFile.
|
||||
expect(isWorktreeConnectionResolved(folderWorkspaceKey('folder-workspace-1'))).toBe(true)
|
||||
})
|
||||
|
||||
it('keeps normalized same-path folder repo ambiguity when resolving files', () => {
|
||||
const workspaceKey = folderWorkspaceKey('folder-workspace-1')
|
||||
useAppStore.setState({
|
||||
|
||||
@@ -39,6 +39,28 @@ export function getConnectionId(worktreeId: string | null): string | null | unde
|
||||
return repo.connectionId ?? null
|
||||
}
|
||||
|
||||
/**
|
||||
* True when we can determine the owning host (local vs. a specific SSH target)
|
||||
* for a worktree. False means the backing repo has not landed in the store yet
|
||||
* — e.g. right after a session restore while the SSH connection is still
|
||||
* establishing. Callers must not fall back to a LOCAL read of a remote path in
|
||||
* that window; doing so denies the path with a terminal "access denied" (#6648).
|
||||
*/
|
||||
export function isWorktreeConnectionResolved(worktreeId: string | null): boolean {
|
||||
if (!worktreeId) {
|
||||
return true
|
||||
}
|
||||
const parsedWorkspaceKey = parseWorkspaceKey(worktreeId)
|
||||
if (parsedWorkspaceKey?.type === 'folder') {
|
||||
// Folder workspaces resolve per-file; treat them as resolved here and let
|
||||
// getConnectionIdForFile decide ownership for the concrete path.
|
||||
return true
|
||||
}
|
||||
// Why: getConnectionId returns undefined only when the backing repo is absent;
|
||||
// any found repo yields a string or null, so this mirrors "repo has hydrated".
|
||||
return getConnectionId(worktreeId) !== undefined
|
||||
}
|
||||
|
||||
export function getConnectionIdForFile(
|
||||
worktreeId: string | null,
|
||||
filePath: string
|
||||
|
||||
@@ -0,0 +1,174 @@
|
||||
/**
|
||||
* Reproduction harness for issue #6648:
|
||||
* "Remote host attempting to view a file results in error"
|
||||
*
|
||||
* On Windows, opening a file to edit it on a remote (SSH) host fails with:
|
||||
* "Error invoking remote method 'fs:readFile': Error: Access denied: path
|
||||
* resolves outside allowed directories."
|
||||
*
|
||||
* That message (PATH_ACCESS_DENIED_MESSAGE) is only produced by the LOCAL
|
||||
* filesystem resolver, which means the remote file path reached the local
|
||||
* read path WITHOUT a connectionId. This harness drives the real renderer
|
||||
* code paths to show how that happens and stays latched:
|
||||
*
|
||||
* 1. A connected SSH repo's connectionId is resolved from state.repos. While
|
||||
* the SSH repo is still hydrating (e.g. right after a session restore on a
|
||||
* slow Windows SSH connect), getConnectionId returns `undefined`.
|
||||
* 2. readRuntimeFileContent treats `undefined`/`null` connectionId identically
|
||||
* and falls back to a LOCAL fs.readFile of the remote POSIX path.
|
||||
* 3. The local resolver denies the remote path -> PATH_ACCESS_DENIED_MESSAGE.
|
||||
* 4. The editor's retry gate refuses to retry "access denied", so the error
|
||||
* latches permanently even after the SSH repo finishes hydrating.
|
||||
*/
|
||||
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
|
||||
import type { Repo } from '../../../shared/types'
|
||||
import { useAppStore } from '@/store'
|
||||
import { getConnectionIdForFile, isWorktreeConnectionResolved } from '@/lib/connection-context'
|
||||
import {
|
||||
WORKTREE_OWNER_NOT_READY_ERROR,
|
||||
WORKTREE_OWNER_UNREACHABLE_ERROR
|
||||
} from '@/components/editor/editor-panel-content-types'
|
||||
import { shouldRetryFileLoadError } from '@/components/editor/useEditorPanelFileLoadRetry'
|
||||
import { readRuntimeFileContent } from './runtime-file-client'
|
||||
|
||||
// Mirrors src/main/ipc/filesystem-auth.ts PATH_ACCESS_DENIED_MESSAGE.
|
||||
const PATH_ACCESS_DENIED_MESSAGE =
|
||||
'Access denied: path resolves outside allowed directories. If this blocks a legitimate workflow, please file a GitHub issue.'
|
||||
|
||||
const REMOTE_REPO_ROOT = '/home/user/project'
|
||||
const REMOTE_FILE_PATH = '/home/user/project/src/index.ts'
|
||||
const SSH_TARGET_ID = 'ssh-target-1'
|
||||
const SSH_REPO_ID = 'repo-ssh'
|
||||
const REMOTE_WORKTREE_ID = `${SSH_REPO_ID}::${REMOTE_REPO_ROOT}`
|
||||
|
||||
const initialState = useAppStore.getInitialState()
|
||||
|
||||
const fsReadFile = vi.fn()
|
||||
|
||||
/**
|
||||
* Stand-in for the main-process `fs:readFile` IPC handler. It mirrors the real
|
||||
* routing in src/main/ipc/filesystem.ts: when a connectionId is supplied the
|
||||
* read is served by the SSH provider; otherwise it goes through the local
|
||||
* authorized-path resolver, which denies any path outside the local allowed
|
||||
* roots (SSH repo roots are intentionally excluded — see getLocalRepos).
|
||||
*/
|
||||
function mainProcessReadFile(args: { filePath: string; connectionId?: string }) {
|
||||
if (args.connectionId) {
|
||||
return Promise.resolve({ content: 'export const remote = true\n', isBinary: false })
|
||||
}
|
||||
// Local resolver: the remote POSIX path is not under any local allowed root.
|
||||
return Promise.reject(new Error(PATH_ACCESS_DENIED_MESSAGE))
|
||||
}
|
||||
|
||||
function makeRepo(overrides: Partial<Repo> & { id: string }): Repo {
|
||||
return {
|
||||
path: REMOTE_REPO_ROOT,
|
||||
displayName: 'project',
|
||||
badgeColor: '#000',
|
||||
addedAt: 0,
|
||||
...overrides
|
||||
}
|
||||
}
|
||||
|
||||
beforeEach(() => {
|
||||
fsReadFile.mockReset()
|
||||
fsReadFile.mockImplementation(mainProcessReadFile)
|
||||
vi.stubGlobal('window', {
|
||||
api: {
|
||||
fs: { readFile: fsReadFile },
|
||||
runtime: { call: vi.fn() },
|
||||
runtimeEnvironments: { call: vi.fn(), subscribe: vi.fn() }
|
||||
}
|
||||
})
|
||||
})
|
||||
|
||||
afterEach(() => {
|
||||
useAppStore.setState(initialState, true)
|
||||
vi.unstubAllGlobals()
|
||||
})
|
||||
|
||||
// Replays the editor read path from useEditorPanelContentState.loadFileContent,
|
||||
// including the owner-not-ready guard added for #6648.
|
||||
async function openRemoteFileInEditor() {
|
||||
const resolvedConnectionId = getConnectionIdForFile(REMOTE_WORKTREE_ID, REMOTE_FILE_PATH)
|
||||
const connectionId = resolvedConnectionId ?? undefined
|
||||
const readSettings: { activeRuntimeEnvironmentId: string | null } = {
|
||||
activeRuntimeEnvironmentId: null
|
||||
}
|
||||
if (
|
||||
resolvedConnectionId === undefined &&
|
||||
!readSettings.activeRuntimeEnvironmentId?.trim() &&
|
||||
!isWorktreeConnectionResolved(REMOTE_WORKTREE_ID)
|
||||
) {
|
||||
throw new Error(WORKTREE_OWNER_NOT_READY_ERROR)
|
||||
}
|
||||
return readRuntimeFileContent({
|
||||
settings: readSettings,
|
||||
filePath: REMOTE_FILE_PATH,
|
||||
relativePath: 'src/index.ts',
|
||||
worktreeId: REMOTE_WORKTREE_ID,
|
||||
connectionId
|
||||
})
|
||||
}
|
||||
|
||||
describe('issue #6648: opening a remote-host file in the editor', () => {
|
||||
it('succeeds once the SSH repo is hydrated with its connectionId', async () => {
|
||||
useAppStore.setState({
|
||||
repos: [makeRepo({ id: SSH_REPO_ID, connectionId: SSH_TARGET_ID })],
|
||||
worktreesByRepo: {}
|
||||
})
|
||||
|
||||
await expect(openRemoteFileInEditor()).resolves.toEqual({
|
||||
content: 'export const remote = true\n',
|
||||
isBinary: false
|
||||
})
|
||||
expect(fsReadFile).toHaveBeenCalledWith({
|
||||
filePath: REMOTE_FILE_PATH,
|
||||
connectionId: SSH_TARGET_ID
|
||||
})
|
||||
})
|
||||
|
||||
it('FIXED: fails retryably (not a local access-denied) while the SSH repo hydrates', async () => {
|
||||
// Session restore reopened the remote tab, but the SSH repo has not landed
|
||||
// in state.repos yet (relay/connection still establishing — slower on
|
||||
// Windows). getConnectionIdForFile returns undefined for the unknown repo.
|
||||
useAppStore.setState({ repos: [], worktreesByRepo: {} })
|
||||
|
||||
expect(getConnectionIdForFile(REMOTE_WORKTREE_ID, REMOTE_FILE_PATH)).toBeUndefined()
|
||||
expect(isWorktreeConnectionResolved(REMOTE_WORKTREE_ID)).toBe(false)
|
||||
|
||||
// The owner-not-ready guard prevents the bad local read entirely.
|
||||
await expect(openRemoteFileInEditor()).rejects.toThrow(WORKTREE_OWNER_NOT_READY_ERROR)
|
||||
await expect(openRemoteFileInEditor()).rejects.not.toThrow(PATH_ACCESS_DENIED_MESSAGE)
|
||||
expect(fsReadFile).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it('FIXED: recovers once the SSH repo finishes hydrating', async () => {
|
||||
useAppStore.setState({ repos: [], worktreesByRepo: {} })
|
||||
await expect(openRemoteFileInEditor()).rejects.toThrow(WORKTREE_OWNER_NOT_READY_ERROR)
|
||||
|
||||
// Relay discovery completes and the SSH repo lands in the store.
|
||||
useAppStore.setState({
|
||||
repos: [makeRepo({ id: SSH_REPO_ID, connectionId: SSH_TARGET_ID })],
|
||||
worktreesByRepo: {}
|
||||
})
|
||||
|
||||
await expect(openRemoteFileInEditor()).resolves.toEqual({
|
||||
content: 'export const remote = true\n',
|
||||
isBinary: false
|
||||
})
|
||||
expect(fsReadFile).toHaveBeenCalledWith({
|
||||
filePath: REMOTE_FILE_PATH,
|
||||
connectionId: SSH_TARGET_ID
|
||||
})
|
||||
})
|
||||
|
||||
it('retry gate retries owner-not-ready but not access-denied or the terminal message', () => {
|
||||
// owner-not-ready auto-retries (while connecting)...
|
||||
expect(shouldRetryFileLoadError(WORKTREE_OWNER_NOT_READY_ERROR)).toBe(true)
|
||||
// ...but a genuine access-denied and the budget-exhausted terminal message
|
||||
// are NOT auto-retried (the terminal one only restarts via the Retry button).
|
||||
expect(shouldRetryFileLoadError(PATH_ACCESS_DENIED_MESSAGE)).toBe(false)
|
||||
expect(shouldRetryFileLoadError(WORKTREE_OWNER_UNREACHABLE_ERROR)).toBe(false)
|
||||
})
|
||||
})
|
||||
Reference in New Issue
Block a user