Skip to content
Closed
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
24 changes: 24 additions & 0 deletions src/cli/handlers/file.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ vi.mock('../runtime-client', () => {
})

import { main } from '../index'
import { RuntimeRpcFailureError } from '../runtime-client'
import { buildWorktree, okFixture, queueFixtures, worktreeListFixture } from '../test-fixtures'

describe('orca file CLI handlers', () => {
Expand Down Expand Up @@ -87,6 +88,29 @@ describe('orca file CLI handlers', () => {
expect(vi.mocked(console.log).mock.calls[0][0]).toBe('Opened src/App.tsx.')
})

it('reports a missing file as a JSON failure with a nonzero exit code', async () => {
const priorExitCode = process.exitCode
queueFixtures(callMock, worktreeListFixture([buildWorktree('/tmp/repo', 'feature')]))
callMock.mockRejectedValueOnce(
new RuntimeRpcFailureError({
id: 'req_open',
ok: false,
error: { code: 'runtime_error', message: 'File not found: docs/missing.md' },
_meta: { runtimeId: 'runtime-1' }
})
)

await main(['file', 'open', 'docs/missing.md', '--json'], '/tmp/repo')

expect(JSON.parse(String(vi.mocked(console.log).mock.calls[0][0]))).toMatchObject({
ok: false,
error: { code: 'runtime_error', message: 'File not found: docs/missing.md' }
})
expect(process.exitCode).toBe(1)

process.exitCode = priorExitCode
})

Comment on lines +91 to +113

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.

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Use a try...finally block to ensure process.exitCode is restored.

If an assertion (like expect) fails during the test, it throws an error and halts the test execution. Without a try...finally block, process.exitCode will not be restored to priorExitCode, which can cause subsequent tests to misbehave or the test runner process to exit with an incorrect status code.

🛡️ Proposed fix to ensure cleanup
   it('reports a missing file as a JSON failure with a nonzero exit code', async () => {
     const priorExitCode = process.exitCode
-    queueFixtures(callMock, worktreeListFixture([buildWorktree('/tmp/repo', 'feature')]))
-    callMock.mockRejectedValueOnce(
-      new RuntimeRpcFailureError({
-        id: 'req_open',
-        ok: false,
-        error: { code: 'runtime_error', message: 'File not found: docs/missing.md' },
-        _meta: { runtimeId: 'runtime-1' }
-      })
-    )
-
-    await main(['file', 'open', 'docs/missing.md', '--json'], '/tmp/repo')
-
-    expect(JSON.parse(String(vi.mocked(console.log).mock.calls[0][0]))).toMatchObject({
-      ok: false,
-      error: { code: 'runtime_error', message: 'File not found: docs/missing.md' }
-    })
-    expect(process.exitCode).toBe(1)
-
-    process.exitCode = priorExitCode
+    try {
+      queueFixtures(callMock, worktreeListFixture([buildWorktree('/tmp/repo', 'feature')]))
+      callMock.mockRejectedValueOnce(
+        new RuntimeRpcFailureError({
+          id: 'req_open',
+          ok: false,
+          error: { code: 'runtime_error', message: 'File not found: docs/missing.md' },
+          _meta: { runtimeId: 'runtime-1' }
+        })
+      )
+
+      await main(['file', 'open', 'docs/missing.md', '--json'], '/tmp/repo')
+
+      expect(JSON.parse(String(vi.mocked(console.log).mock.calls[0][0]))).toMatchObject({
+        ok: false,
+        error: { code: 'runtime_error', message: 'File not found: docs/missing.md' }
+      })
+      expect(process.exitCode).toBe(1)
+    } finally {
+      process.exitCode = priorExitCode
+    }
   })
📝 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
it('reports a missing file as a JSON failure with a nonzero exit code', async () => {
const priorExitCode = process.exitCode
queueFixtures(callMock, worktreeListFixture([buildWorktree('/tmp/repo', 'feature')]))
callMock.mockRejectedValueOnce(
new RuntimeRpcFailureError({
id: 'req_open',
ok: false,
error: { code: 'runtime_error', message: 'File not found: docs/missing.md' },
_meta: { runtimeId: 'runtime-1' }
})
)
await main(['file', 'open', 'docs/missing.md', '--json'], '/tmp/repo')
expect(JSON.parse(String(vi.mocked(console.log).mock.calls[0][0]))).toMatchObject({
ok: false,
error: { code: 'runtime_error', message: 'File not found: docs/missing.md' }
})
expect(process.exitCode).toBe(1)
process.exitCode = priorExitCode
})
it('reports a missing file as a JSON failure with a nonzero exit code', async () => {
const priorExitCode = process.exitCode
try {
queueFixtures(callMock, worktreeListFixture([buildWorktree('/tmp/repo', 'feature')]))
callMock.mockRejectedValueOnce(
new RuntimeRpcFailureError({
id: 'req_open',
ok: false,
error: { code: 'runtime_error', message: 'File not found: docs/missing.md' },
_meta: { runtimeId: 'runtime-1' }
})
)
await main(['file', 'open', 'docs/missing.md', '--json'], '/tmp/repo')
expect(JSON.parse(String(vi.mocked(console.log).mock.calls[0][0]))).toMatchObject({
ok: false,
error: { code: 'runtime_error', message: 'File not found: docs/missing.md' }
})
expect(process.exitCode).toBe(1)
} finally {
process.exitCode = priorExitCode
}
})

it('opens a staged diff for an explicit worktree without cwd inference', async () => {
queueFixtures(
callMock,
Expand Down
43 changes: 43 additions & 0 deletions src/main/runtime/orca-runtime-files.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -248,6 +248,8 @@ describe('RuntimeFileCommands', () => {
it('opens text files through the renderer host (inheriting active runtime env)', async () => {
const openFile = vi.fn()
const { commands } = createRuntimeFileCommands({ openFile })
resolveAuthorizedPathMock.mockImplementation(async (path: string) => path)
statMock.mockResolvedValue(mockStats(12, 3))

const result = await commands.openMobileFile('id:wt-1', 'docs/readme.md')

Expand All @@ -268,6 +270,8 @@ describe('RuntimeFileCommands', () => {
it('opens previewable images through the renderer host as an image tab', async () => {
const openFile = vi.fn()
const { commands } = createRuntimeFileCommands({ openFile })
resolveAuthorizedPathMock.mockImplementation(async (path: string) => path)
statMock.mockResolvedValue(mockStats(12, 3))

const result = await commands.openMobileFile('id:wt-1', 'assets/logo.png')

Expand All @@ -288,6 +292,8 @@ describe('RuntimeFileCommands', () => {
it('leaves non-previewable binaries unavailable on mobile', async () => {
const openFile = vi.fn()
const { commands } = createRuntimeFileCommands({ openFile })
resolveAuthorizedPathMock.mockImplementation(async (path: string) => path)
statMock.mockResolvedValue(mockStats(12, 3))

const result = await commands.openMobileFile('id:wt-1', 'dist/bundle.zip')

Expand All @@ -300,6 +306,43 @@ describe('RuntimeFileCommands', () => {
})
})

it('rejects a missing local file before opening a renderer tab', async () => {
const openFile = vi.fn()
const { commands } = createRuntimeFileCommands({ openFile })
resolveAuthorizedPathMock.mockImplementation(async (path: string) => path)
statMock.mockRejectedValue(enoent())

await expect(commands.openMobileFile('id:wt-1', 'docs/missing.md')).rejects.toThrow(
'File not found: docs/missing.md'
)

expect(openFile).not.toHaveBeenCalled()
})

it('rejects a missing SSH file before opening a renderer tab', async () => {
const openFile = vi.fn()
const stat = vi.fn().mockRejectedValue(new Error('ENOENT: no such file'))
const { commands } = createRuntimeFileCommands({
openFile,
resolveRuntimeFileTarget: vi.fn(async () => ({
worktree: {
id: 'wt-1',
repoId: 'repo-1',
path: '/repo'
},
connectionId: 'ssh-1'
}))
})
vi.mocked(getSshFilesystemProvider).mockReturnValue({ stat } as never)

await expect(commands.openMobileFile('id:wt-1', 'docs/missing.md')).rejects.toThrow(
'File not found: docs/missing.md'
)

expect(stat).toHaveBeenCalledWith('/repo/docs/missing.md')
expect(openFile).not.toHaveBeenCalled()
})

it('does not follow symlinks when reading runtime-local file explorer dirs', async () => {
const { commands } = createRuntimeFileCommands()
resolveAuthorizedPathMock.mockResolvedValue('/repo')
Expand Down
31 changes: 29 additions & 2 deletions src/main/runtime/orca-runtime-files.ts
Original file line number Diff line number Diff line change
Expand Up @@ -248,10 +248,12 @@ export class RuntimeFileCommands {
worktreeSelector: string,
relativePath: string
): Promise<RuntimeFileOpenResult> {
const { worktree } = await this.host.resolveRuntimeFileTarget(worktreeSelector)
const { worktree, connectionId } = await this.host.resolveRuntimeFileTarget(worktreeSelector)
if (!isSafeMobileRelativePath(relativePath)) {
throw new Error('invalid_relative_path')
}
const filePath = joinWorktreeRelativePath(worktree.path, relativePath)
await this.assertMobileFileExists(filePath, relativePath, connectionId)
// Previewable images open like text (the mobile viewer renders them via
// files.readPreview); other binaries stay unavailable on mobile.
const kind = isMobilePreviewableImagePath(relativePath)
Expand All @@ -264,7 +266,6 @@ export class RuntimeFileCommands {
if (kind === 'binary') {
return { worktree: worktree.id, relativePath, kind, opened: false }
}
const filePath = joinWorktreeRelativePath(worktree.path, relativePath)
// Why: the service's internal runtimeId is not a registered runtime env selector
// (those live in orca-environments.json). Passing it caused Unknown environment
// errors on content load for CLI-initiated opens (via files.open from orca cli
Expand All @@ -275,6 +276,32 @@ export class RuntimeFileCommands {
return { worktree: worktree.id, relativePath, kind, opened: true }
}

private async assertMobileFileExists(
filePath: string,
relativePath: string,
connectionId?: string
): Promise<void> {
try {
if (connectionId) {
const provider = getSshFilesystemProvider(connectionId)
if (!provider) {
throw new Error(SSH_FILESYSTEM_PROVIDER_UNAVAILABLE_MESSAGE)
}
await provider.stat(filePath)
return
}
await stat(await resolveAuthorizedPath(filePath, this.host.requireStore()))
} catch (error) {
if (
isENOENT(error) ||
(connectionId && RuntimeFileCommands.isRemoteNotFoundErrorMessage(error))
) {
throw new Error(`File not found: ${relativePath}`)
}
throw error
}
}

async openMobileDiff(
worktreeSelector: string,
relativePath: string,
Expand Down
Loading