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
18 changes: 18 additions & 0 deletions docs/reference/remote-wire-compatibility.md
Original file line number Diff line number Diff line change
Expand Up @@ -245,6 +245,24 @@ The cross-version suite derives the old client's list by removing this capabilit
baseline's own list, per the rule above, so the downgrade stays exercised after a release ships
it.

## Worked example: `files.open` of a PDF

`files.open` used to classify `.pdf` as `binary` and return `opened: false` without opening a
tab. The desktop viewer now opens that file and the reply is `kind: 'pdf', opened: true`. The
frame shape is unchanged, but an already-installed mobile client treats `opened: true` as
"activate the synced file tab" and then `files.read`s it, which rejects the PDF. That is Rule 3.

The reply stays behind `files.open.pdf-desktop.v1`
(`FILES_PDF_DESKTOP_OPEN_RUNTIME_CAPABILITY`). The check is
`supportsFilesPdfDesktopOpen` in
`src/main/runtime/rpc/methods/files-open-pdf-desktop-capability.ts`, applied only to
`files.open`. A client that advertises it, and any in-process caller, gets the desktop viewer.
A negotiated client that does not gets the previous answer: `kind: 'binary'`, `opened: false`,
and no tab. CLI and mobile advertise it from `remoteRuntimeClientCapabilities`. The phone
build that advertises it also skips activating a `kind: 'pdf'` tab
(`mobile/src/session/mobile-file-tap-open.ts`). Delete the gate once no supported release
lacks the capability.

## Known debt: JSON-RPC errors drop Node's string code

An error raised on an SSH host crosses the relay as JSON-RPC, and
Expand Down
50 changes: 50 additions & 0 deletions mobile/src/session/mobile-file-tap-open.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,56 @@ describe('openMobileFileTap', () => {
expect(switchSessionTab).toHaveBeenCalledWith(openedTab)
})

it('does not activate the synced tab after the desktop opens a PDF', async () => {
const client = createClient([
ok({
worktree: 'wt-1',
relativePath: 'docs/example.pdf',
absolutePath: '/repo/docs/example.pdf',
exists: true,
isDirectory: false,
openTarget: {
kind: 'worktree-file',
provider: 'local',
relativePath: 'docs/example.pdf',
absolutePath: '/repo/docs/example.pdf'
}
}),
ok({ opened: true, kind: 'pdf', relativePath: 'docs/example.pdf' })
])
const switchSessionTab = vi.fn()
const scheduleDelayedAction = vi.fn()

openMobileFileTap({
client,
hostId: 'host-1',
worktreeId: 'wt-1',
pathText: 'docs/example.pdf',
line: null,
column: null,
pushPreviewRoute: vi.fn(),
openBrowser: vi.fn(),
triggerOpenFeedback: vi.fn(),
fetchSessionTabs: vi.fn(),
getSessionTabs: () => [{ id: 'tab-pdf', relativePath: 'docs/example.pdf' }],
getActiveSessionTabId: () => 'terminal-tab',
getActivationState: activeTerminalState,
switchSessionTab,
scheduleDelayedAction
})
await Promise.resolve()
await Promise.resolve()
await new Promise((resolve) => setTimeout(resolve, 0))

expect(client.sendRequest).toHaveBeenCalledWith(
'files.open',
{ worktree: 'id:wt-1', relativePath: 'docs/example.pdf' },
{ timeoutMs: 15_000 }
)
expect(scheduleDelayedAction).not.toHaveBeenCalled()
expect(switchSessionTab).not.toHaveBeenCalled()
})

it('opens a sibling terminal path through the resolved owning worktree', async () => {
const client = createClient([
ok({
Expand Down
5 changes: 5 additions & 0 deletions mobile/src/session/mobile-file-tap-open.ts
Original file line number Diff line number Diff line change
Expand Up @@ -187,6 +187,11 @@ async function openMobileFileTapAsync<T extends FileTapSessionTab>(
reportOpenFailure(options)
return
}
// The desktop PDF viewer is already open. Activating the synced tab would
// files.read the PDF and fail with binary_file.
if (opened.value.kind === 'pdf') {
return
}
Comment thread
innocarpe marked this conversation as resolved.
scheduleOpenedWorktreeTabActivation(options, openedPath)
}

Expand Down
7 changes: 6 additions & 1 deletion mobile/src/session/session-launch-reply-schema.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,12 @@ import { salvagedOptional } from '../../../src/shared/zod-salvage'
* that declined the open. An incompatible reply reaches the same `reportOpenFailure` through
* openMobileFileTap's own catch.
*/
export const fileTapOpenedSchema = z.looseObject({ opened: z.boolean() })
export const fileTapOpenedSchema = z.looseObject({
opened: z.boolean(),
// Optional so an older host that only sends `opened` still activates the tab.
// `pdf` is the desktop viewer; the phone must not load that tab through files.read.
kind: z.string().optional()
})

/**
* The browser tab a user opens from the tab strip.
Expand Down
1 change: 1 addition & 0 deletions mobile/src/session/session-reply-schema.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -203,6 +203,7 @@ describe('required members', () => {

it('requires the open verdict the tap routes on', () => {
expect(reads(fileTapOpenedSchema, { opened: true }).opened).toBe(true)
expect(reads(fileTapOpenedSchema, { opened: true, kind: 'pdf' }).kind).toBe('pdf')
expect(refuses(fileTapOpenedSchema, {})).toBe(true)
expect(refuses(fileTapOpenedSchema, { opened: 1 })).toBe(true)
})
Expand Down
49 changes: 49 additions & 0 deletions src/cli/handlers/file.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,52 @@ describe('orca file CLI handlers', () => {
expect(vi.mocked(console.log).mock.calls[0][0]).toBe('Opened src/App.tsx.')
})

it('rejects a direct open the runtime declines', async () => {
queueFixtures(
callMock,
worktreeListFixture([buildWorktree('/tmp/repo', 'feature')]),
okFixture('req_open', {
worktree: 'wt-1',
relativePath: 'dist/bundle.zip',
kind: 'binary',
opened: false
})
)

await main(['file', 'open', 'dist/bundle.zip'], '/tmp/repo')

expect(vi.mocked(console.log)).not.toHaveBeenCalled()
expect(vi.mocked(console.error).mock.calls[0][0]).toBe(
'Did not open dist/bundle.zip: binary file.'
)
expect(process.exitCode).toBe(1)
})

it('reports a declined direct open as unsupported-type in json', async () => {
queueFixtures(
callMock,
worktreeListFixture([buildWorktree('/tmp/repo', 'feature')]),
okFixture('req_open', {
worktree: 'wt-1',
relativePath: 'dist/bundle.zip',
kind: 'binary',
opened: false
})
)

await main(['file', 'open', 'dist/bundle.zip', '--json'], '/tmp/repo')

const output = JSON.parse(vi.mocked(console.log).mock.calls[0][0])
expect(output).toMatchObject({
ok: false,
error: {
code: 'unsupported-type',
message: 'Did not open dist/bundle.zip: binary file.'
}
})
expect(process.exitCode).toBe(1)
})

it('opens a staged diff for an explicit worktree without cwd inference', async () => {
queueFixtures(
callMock,
Expand Down Expand Up @@ -135,6 +181,7 @@ describe('orca file CLI handlers', () => {
expect(vi.mocked(console.log).mock.calls[0][0]).toBe(
'Did not open diff for assets/logo.png: binary file.'
)
expect(process.exitCode).toBeUndefined()
})

it('rejects --worktree without a value before cwd inference or RPC calls', async () => {
Expand Down Expand Up @@ -225,6 +272,8 @@ describe('orca file CLI handlers', () => {
await main(['file', 'open-changed', '--worktree', 'id:wt-1', '--json'], '/tmp/elsewhere')

const output = JSON.parse(vi.mocked(console.log).mock.calls[0][0])
expect(output.ok).toBe(true)
expect(process.exitCode).toBeUndefined()
expect(output.result.opened).toEqual([])
expect(output.result.skipped).toEqual([
{
Expand Down
7 changes: 6 additions & 1 deletion src/cli/handlers/file.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import { isWslUncPath, parseWslUncPath, toWindowsWslPath } from '../../shared/ws
import type { CommandHandler, HandlerContext } from '../dispatch'
import { getOptionalStringFlag, getRequiredStringFlag } from '../flags'
import { printResult } from '../format'
import { RuntimeClientError } from '../runtime-client'
import { RuntimeClientError } from '../runtime/types'
import { getOptionalWorktreeSelector, resolveCurrentWorktreeSelector } from '../selectors'

type FileOpenMode = 'edit' | 'diff'
Expand Down Expand Up @@ -213,6 +213,11 @@ export const FILE_HANDLERS: Record<string, CommandHandler> = {
relativePath,
navigation: getFileOpenNavigation(ctx.flags)
})
// Why: printResult keeps ok:true. Agents read that as success, so a binary the runtime
// declined must be a real error here. file diff and file open-changed keep the skipped row.
if (!result.result.opened) {
throw new RuntimeClientError('unsupported-type', formatFileOpen(result.result))
}
printResult(result, ctx.json, formatFileOpen)
},
'file diff': async (ctx) => {
Expand Down
4 changes: 3 additions & 1 deletion src/cli/runtime/websocket-transport.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ import {
WORKTREE_VISIBILITY_SOURCE_DEFAULTS_RUNTIME_CAPABILITY
} from '../../shared/protocol-version'
import { AGENT_SESSION_BACKGROUND_TASK_CHILD_VIEWS_CAPABILITY } from '../../shared/agent-session-background-task-child-views-capability'
import { FILES_PDF_DESKTOP_OPEN_RUNTIME_CAPABILITY } from '../../shared/files-pdf-desktop-open-capability'

vi.mock('./launch', () => ({
launchOrcaApp: vi.fn()
Expand Down Expand Up @@ -85,7 +86,8 @@ describe('CLI remote WebSocket transport', () => {
WORKTREE_GITHUB_PR_SUPPRESSION_RUNTIME_CAPABILITY,
WORKTREE_VISIBILITY_DEFAULTS_RUNTIME_CAPABILITY,
WORKTREE_VISIBILITY_SOURCE_DEFAULTS_RUNTIME_CAPABILITY,
AUTOMATION_OWNER_FENCING_RUNTIME_CAPABILITY
AUTOMATION_OWNER_FENCING_RUNTIME_CAPABILITY,
FILES_PDF_DESKTOP_OPEN_RUNTIME_CAPABILITY
]
})
)
Expand Down
77 changes: 77 additions & 0 deletions src/main/runtime/orca-runtime-files-mobile-explorer-reads.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,45 @@ describe('RuntimeFileCommands', () => {
)
})

it('opens a PDF through the same renderer host as the desktop viewer', async () => {
const openFile = vi.fn()
const { commands } = createRuntimeFileCommands({ openFile })
resolveAuthorizedPathMock.mockResolvedValue('/repo/docs/example.pdf')
statMock.mockResolvedValue({ isDirectory: () => false })

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

expect(openFile).toHaveBeenCalledWith(
'wt-1',
'/repo/docs/example.pdf',
'docs/example.pdf',
undefined,
undefined
)
expect(result).toEqual({
worktree: 'wt-1',
relativePath: 'docs/example.pdf',
kind: 'pdf',
opened: true
})
})

it('answers a PDF as an unopened binary when the caller would activate the file tab', async () => {
const openFile = vi.fn()
const { commands } = createRuntimeFileCommands({ openFile })

const result = await commands.openMobileFile('id:wt-1', 'docs/example.pdf', undefined, false)

expect(openFile).not.toHaveBeenCalled()
expect(statMock).not.toHaveBeenCalled()
expect(result).toEqual({
worktree: 'wt-1',
relativePath: 'docs/example.pdf',
kind: 'binary',
opened: false
})
})

it('leaves non-previewable binaries unavailable on mobile', async () => {
const openFile = vi.fn()
const { commands } = createRuntimeFileCommands({ openFile })
Expand All @@ -156,6 +195,44 @@ describe('RuntimeFileCommands', () => {
})
})

it('rejects a directory named like a PDF without creating an editor tab', async () => {
const openFile = vi.fn()
const { commands } = createRuntimeFileCommands({ openFile })
resolveAuthorizedPathMock.mockResolvedValue('/repo/docs/notes.pdf')
statMock.mockResolvedValue({ isDirectory: () => true })

await expect(commands.openMobileFile('id:wt-1', 'docs/notes.pdf')).rejects.toThrow(
"EISDIR: illegal operation on a directory, open '/repo/docs/notes.pdf'"
)
expect(openFile).not.toHaveBeenCalled()
})

it('rejects a remote directory named like a PDF without creating an editor tab', async () => {
const openFile = vi.fn()
const resolveRuntimeFileTarget = vi.fn(async () => ({
worktree: {
id: 'wt-1',
repoId: 'repo-1',
path: '/remote/repo'
},
executionHostId: 'ssh:ssh-1'
}))
const { commands } = createRuntimeFileCommands({
openFile,
path: '/remote/repo',
resolveRuntimeFileTarget
})
// oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: the command only reads `stat`.
vi.mocked(getSshFilesystemProvider).mockReturnValue({
stat: vi.fn().mockResolvedValue({ type: 'directory', size: 0, mtime: 0 })
} as never)
Comment thread
innocarpe marked this conversation as resolved.

await expect(commands.openMobileFile('id:wt-1', 'docs/notes.pdf')).rejects.toThrow(
"EISDIR: illegal operation on a directory, open '/remote/repo/docs/notes.pdf'"
)
expect(openFile).not.toHaveBeenCalled()
})

it('rejects missing local files without creating an editor tab', async () => {
const openFile = vi.fn()
const { commands } = createRuntimeFileCommands({ openFile })
Expand Down
Loading
Loading