From fa738f30bbdaa0ffe70f35a28d6d7da8b2d396fb Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Sat, 17 Oct 2026 22:19:55 -0700 Subject: [PATCH] fix(notebook): run cells only when the resolved owner is this computer (#30187) A managed SSH or paired-server workspace has no SSH connection id, so the notebook read it as local and ran its cells in the desktop's own Python against a same-named local file. Derive the execution host from resolveOwner; refuse remote, ambiguous and unloaded owners. --- .../src/components/editor/IpynbViewer.tsx | 5 +- .../editor/ipynb-execution-host.test.ts | 49 +++++++++++ .../components/editor/ipynb-execution-host.ts | 22 +++++ .../editor/useIpynbCellExecution.test.tsx | 82 ++++++++++++++++--- .../editor/useIpynbCellExecution.ts | 43 +++++++--- src/renderer/src/i18n/locales/en.json | 3 +- 6 files changed, 178 insertions(+), 26 deletions(-) create mode 100644 src/renderer/src/components/editor/ipynb-execution-host.test.ts create mode 100644 src/renderer/src/components/editor/ipynb-execution-host.ts diff --git a/src/renderer/src/components/editor/IpynbViewer.tsx b/src/renderer/src/components/editor/IpynbViewer.tsx index c3f5aaf2efa..03beb68b122 100644 --- a/src/renderer/src/components/editor/IpynbViewer.tsx +++ b/src/renderer/src/components/editor/IpynbViewer.tsx @@ -17,7 +17,6 @@ import { translate } from '@/i18n/i18n' import { editorShortcutMatches } from './editor-shortcuts' import { getShortcutPlatform } from '@/lib/shortcut-platform' import { findWorktreeById } from '@/store/slices/worktree-helpers' -import { getConnectionId } from '@/lib/connection-context' import { IpynbCellToolbar, IpynbToolbarButton } from './IpynbCellToolbar' import { IpynbCellSource } from './IpynbCellEditor' import { IpynbCellRunOutputs, IpynbCellRunPrompt } from './IpynbCellRun' @@ -29,6 +28,7 @@ import { useIpynbDocumentEditing } from './useIpynbDocumentEditing' import { useIpynbCellExecution } from './useIpynbCellExecution' +import { resolveNotebookExecutionHost } from './ipynb-execution-host' import { useIpynbScrollRestoration } from './useIpynbScrollRestoration' type IpynbViewerProps = { @@ -54,6 +54,7 @@ export default function IpynbViewer({ }: IpynbViewerProps): React.JSX.Element { const editorFontZoomLevel = useAppStore((s) => s.editorFontZoomLevel) const rootPath = useAppStore((s) => findWorktreeById(s.worktreesByRepo, worktreeId)?.path ?? null) + const executionHost = useAppStore((s) => resolveNotebookExecutionHost(s, worktreeId)) const [editingCellKey, setEditingCellKey] = useState(null) const parsed = useMemo(() => { try { @@ -158,7 +159,7 @@ export default function IpynbViewer({ const { notebook } = parsed // Kernels are local Python only; other notebooks still render and edit. - const canRun = notebook.language === 'python' && !getConnectionId(worktreeId) + const canRun = notebook.language === 'python' && executionHost === 'local' return (
, + worktree: { + hostId?: `runtime:${string}` | `ssh:${string}` | 'local' + runtimeOwnerEnvironmentId?: string + } +): WorktreeOperationRouteState { + return { + repos: [{ id: 'repo-1', ...repo }], + worktreesByRepo: { 'repo-1': [{ id: WORKTREE_ID, repoId: 'repo-1', ...worktree }] } + } +} + +describe('resolveNotebookExecutionHost', () => { + it.each([ + ['a local workspace', rows({}, { hostId: 'local' }), 'local'], + ['a legacy unstamped local workspace', rows({}, {}), 'local'], + ['a direct SSH workspace', rows({ connectionId: 'box' }, { hostId: 'ssh:box' }), 'remote'], + [ + 'a managed SSH workspace, which has no connection id', + rows( + { executionHostId: 'runtime:env-orcad' }, + { hostId: 'runtime:env-orcad', runtimeOwnerEnvironmentId: 'env-orcad' } + ), + 'remote' + ], + ['unloaded rows', {}, 'unresolved'] + ] as const)('reads %s as %s', (_name, state, expected) => { + expect(resolveNotebookExecutionHost(state, WORKTREE_ID)).toBe(expected) + }) + + it('treats a local folder workspace as local and a server-owned one as remote', () => { + const folder = ( + executionHostId: 'local' | `runtime:${string}` + ): WorktreeOperationRouteState => ({ + folderWorkspaces: [ + { id: 'f1', projectGroupId: 'g1', connectionId: null, executionHostId, diffComments: [] } + ] + }) + expect(resolveNotebookExecutionHost(folder('local'), 'folder:f1')).toBe('local') + expect(resolveNotebookExecutionHost(folder('runtime:env-a'), 'folder:f1')).toBe('remote') + }) +}) diff --git a/src/renderer/src/components/editor/ipynb-execution-host.ts b/src/renderer/src/components/editor/ipynb-execution-host.ts new file mode 100644 index 00000000000..b620b83d4b4 --- /dev/null +++ b/src/renderer/src/components/editor/ipynb-execution-host.ts @@ -0,0 +1,22 @@ +import { LOCAL_EXECUTION_HOST_ID } from '../../../../shared/execution-host' +import { resolveOwner } from '@/lib/resolve-owner' +import type { WorktreeOperationRouteState } from '@/lib/worktree-operation-route' + +/** Where a notebook's kernel would run: kernels exist only on this computer. */ +export type NotebookExecutionHost = 'local' | 'remote' | 'unresolved' + +/** + * Why: a managed SSH or paired-server workspace has no SSH connection id, so "no connection id" + * cannot mean local; running its cells here would execute a same-named local file. + */ +export function resolveNotebookExecutionHost( + state: WorktreeOperationRouteState, + worktreeId: string +): NotebookExecutionHost { + const match = resolveOwner(state, { workspaceId: worktreeId }) + if (match.kind !== 'resolved') { + return 'unresolved' + } + const { owner } = match + return owner.endpoint.kind === 'self' && owner.at === LOCAL_EXECUTION_HOST_ID ? 'local' : 'remote' +} diff --git a/src/renderer/src/components/editor/useIpynbCellExecution.test.tsx b/src/renderer/src/components/editor/useIpynbCellExecution.test.tsx index 8dfb540ff5c..af50f86a6f4 100644 --- a/src/renderer/src/components/editor/useIpynbCellExecution.test.tsx +++ b/src/renderer/src/components/editor/useIpynbCellExecution.test.tsx @@ -3,8 +3,12 @@ import { act, renderHook, waitFor } from '@testing-library/react' import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import { parseIpynb } from './ipynb-parse' import type { KernelFrameEvent } from '../../../../shared/notebook-kernel-types' +import type { + WorktreeOperationOwnerRecord, + WorktreeOperationRouteState +} from '@/lib/worktree-operation-route' -const { getConnectionIdMock, notebookApi, toastError, frameListener } = vi.hoisted(() => { +const { storeState, notebookApi, toastError, frameListener } = vi.hoisted(() => { const frameListener = { current: (_event: KernelFrameEvent): void => {} } const notebookApi = { listPythonEnvironments: vi.fn(), @@ -18,23 +22,42 @@ const { getConnectionIdMock, notebookApi, toastError, frameListener } = vi.hoist } // The kernel session subscribes to kernel frames when it loads. Object.defineProperty(window, 'api', { configurable: true, value: { notebook: notebookApi } }) + const storeState: { current: WorktreeOperationRouteState } = { current: {} } return { - getConnectionIdMock: vi.fn((): string | null => null), + storeState, notebookApi, toastError: vi.fn(), frameListener } }) -vi.mock('@/lib/connection-context', () => ({ getConnectionId: getConnectionIdMock })) vi.mock('@/i18n/i18n', () => ({ translate: (_key: string, fallback: string) => fallback })) vi.mock('sonner', () => ({ toast: { error: toastError } })) -vi.mock('@/store', () => ({ useAppStore: { subscribe: () => () => {} } })) +vi.mock('@/store', () => ({ + useAppStore: { subscribe: () => () => {}, getState: () => storeState.current } +})) import { useIpynbCellExecution } from './useIpynbCellExecution' import { runCells, trustNotebook } from './ipynb-kernel-session' import { getSession } from './ipynb-kernel-store' +const WORKTREE_ID = 'repo-1::/repo' + +function ownerRows( + repo: Pick< + NonNullable[number], + 'connectionId' | 'executionHostId' + >, + worktree: Pick = {} +): WorktreeOperationRouteState { + return { + repos: [{ id: 'repo-1', ...repo }], + worktreesByRepo: { 'repo-1': [{ id: WORKTREE_ID, repoId: 'repo-1', ...worktree }] } + } +} + +const LOCAL_ROWS = ownerRows({}, { hostId: 'local' }) + function notebookContent(withIds: boolean): string { return JSON.stringify({ nbformat: 4, @@ -62,7 +85,7 @@ function renderExecution(filePath: string, applyContent = vi.fn(), withIds = tru const hook = renderHook(() => useIpynbCellExecution({ filePath, - worktreeId: 'worktree-a', + worktreeId: WORKTREE_ID, rootPath: '/repo', flushSourceDrafts: () => content, applyContent @@ -73,7 +96,7 @@ function renderExecution(filePath: string, applyContent = vi.fn(), withIds = tru beforeEach(() => { vi.clearAllMocks() - getConnectionIdMock.mockReturnValue(null) + storeState.current = LOCAL_ROWS notebookApi.listPythonEnvironments.mockResolvedValue({ workspace: [{ path: '/repo/.venv/bin/python', name: '.venv', version: '3.12.1' }], path: [] @@ -106,9 +129,22 @@ describe('notebook cell execution', () => { }) }) - it('refuses to run in SSH workspaces without touching the notebook or starting a kernel', () => { - getConnectionIdMock.mockReturnValue('ssh-connection') - const { hook, applyContent } = renderExecution('/remote/notebook.ipynb') + it.each([ + ['a direct SSH workspace', ownerRows({ connectionId: 'box' }, { hostId: 'ssh:box' })], + [ + 'a managed SSH workspace owned by its server', + ownerRows( + { executionHostId: 'runtime:env-orcad' }, + { hostId: 'runtime:env-orcad', runtimeOwnerEnvironmentId: 'env-orcad' } + ) + ], + [ + 'an SSH target behind a paired server', + ownerRows({}, { hostId: 'ssh:t', runtimeOwnerEnvironmentId: 'env-a' }) + ] + ])('refuses to run %s without touching the notebook or starting a kernel', (_name, rows) => { + storeState.current = rows + const { hook, applyContent } = renderExecution('/repo/notebook.ipynb') act(() => hook.result.current.runCell(1)) expect(toastError).toHaveBeenCalledWith( @@ -119,6 +155,32 @@ describe('notebook cell execution', () => { expect(notebookApi.startKernel).not.toHaveBeenCalled() }) + it.each<[string, WorktreeOperationRouteState]>([ + ['rows that have not loaded', {}], + [ + 'two hosts publishing the same workspace', + { + repos: [{ id: 'repo-1' }, { id: 'repo-1', executionHostId: 'runtime:env-a' }], + worktreesByRepo: { + 'repo-1': [ + { id: WORKTREE_ID, repoId: 'repo-1', hostId: 'local' }, + { id: WORKTREE_ID, repoId: 'repo-1', hostId: 'runtime:env-a' } + ] + } + } + ] + ])('refuses to run with %s instead of reading them as local', (_name, rows) => { + storeState.current = rows + const { hook, applyContent } = renderExecution('/repo/notebook.ipynb') + + act(() => hook.result.current.runCell(1)) + expect(toastError).toHaveBeenCalledWith( + "Orca can't tell which computer this notebook is on yet, so its cells can't run." + ) + expect(applyContent).not.toHaveBeenCalled() + expect(hook.result.current.pendingRun).toBeNull() + }) + it('gives id-less cells ids before queueing, so output follows a moved cell', () => { const { hook, content } = renderExecution('/repo/legacy.ipynb', vi.fn(), false) @@ -185,7 +247,7 @@ function mountFinishedNotebook(filePath: string, content: string) { const hook = renderHook(() => useIpynbCellExecution({ filePath, - worktreeId: 'worktree-a', + worktreeId: WORKTREE_ID, rootPath: '/repo', flushSourceDrafts, applyContent diff --git a/src/renderer/src/components/editor/useIpynbCellExecution.ts b/src/renderer/src/components/editor/useIpynbCellExecution.ts index 97bf5346d0f..e8949935ada 100644 --- a/src/renderer/src/components/editor/useIpynbCellExecution.ts +++ b/src/renderer/src/components/editor/useIpynbCellExecution.ts @@ -1,7 +1,8 @@ import { useEffect, useState } from 'react' import { toast } from 'sonner' import { translate } from '@/i18n/i18n' -import { getConnectionId } from '@/lib/connection-context' +import { useAppStore } from '@/store' +import { resolveNotebookExecutionHost, type NotebookExecutionHost } from './ipynb-execution-host' import { clearIpynbOutputs, updateIpynbCellRuns, withIpynbCellIds } from './ipynb-cell-mutations' import { toStoredOutputs } from './ipynb-kernel-outputs' import { @@ -19,6 +20,28 @@ import { import { parseIpynb } from './ipynb-parse' import { getIpynbCellKey } from './useIpynbDocumentEditing' +function executionRefusal(host: NotebookExecutionHost, language: string): string | null { + switch (host) { + case 'remote': + return translate( + 'auto.components.editor.IpynbViewer.localOnly', + 'Notebook cells can only run for files on this computer.' + ) + case 'unresolved': + return translate( + 'auto.components.editor.IpynbViewer.ownerUnresolved', + "Orca can't tell which computer this notebook is on yet, so its cells can't run." + ) + case 'local': + return language === 'python' + ? null + : translate( + 'auto.components.editor.IpynbViewer.pythonOnly', + 'Only Python notebooks can run in Orca.' + ) + } +} + type UseIpynbCellExecutionArgs = { filePath: string worktreeId: string @@ -83,18 +106,12 @@ export function useIpynbCellExecution({ if (codeIndexes.length === 0) { return } - if (getConnectionId(worktreeId) || notebook.language !== 'python') { - toast.error( - getConnectionId(worktreeId) - ? translate( - 'auto.components.editor.IpynbViewer.localOnly', - 'Notebook cells can only run for files on this computer.' - ) - : translate( - 'auto.components.editor.IpynbViewer.pythonOnly', - 'Only Python notebooks can run in Orca.' - ) - ) + const refusal = executionRefusal( + resolveNotebookExecutionHost(useAppStore.getState(), worktreeId), + notebook.language + ) + if (refusal) { + toast.error(refusal) return } // Why: id-less (nbformat 4.4) cells are keyed by index, so a move mid-run would misroute output. diff --git a/src/renderer/src/i18n/locales/en.json b/src/renderer/src/i18n/locales/en.json index eb8a17bc290..28f72994d41 100644 --- a/src/renderer/src/i18n/locales/en.json +++ b/src/renderer/src/i18n/locales/en.json @@ -15866,7 +15866,8 @@ "noPython": "Python was not found on this computer. Install it from [python.org](https://www.python.org/downloads/), then run the cell again.", "kernelDied": "The kernel died.", "localOnly": "Notebook cells can only run for files on this computer.", - "pythonOnly": "Only Python notebooks can run in Orca." + "pythonOnly": "Only Python notebooks can run in Orca.", + "ownerUnresolved": "Orca can't tell which computer this notebook is on yet, so its cells can't run." }, "MarkdownPreview": { "e4683f70c4": "Cancel",