From ba13d7a04d3341f00ef6bfa10b8ba8f8e82d9ee2 Mon Sep 17 00:00:00 2001 From: Jinjing <6427696+AmethystLiang@users.noreply.github.com> Date: Thu, 23 Jul 2026 16:58:57 -0700 Subject: [PATCH] fix(review): consolidate SSH ownership fence and add fail-closed observability - move the SSH status==='connected' liveness gate and canonical host-id derivation into the shared buildFileMutationOwnership so web fails closed on a reconnecting connection the same way mobile does; reduce buildMobileFileMutationOwnership to a thin delegate (removes the undocumented override/divergence) [#1] - log a cause tag at every mobile fence rejection via runtimeChangedError() so a repeated 'Orca server changed' toast is diagnosable, mirroring the transport '[net]' convention [#2] - add tests for the post-create files.open failure, replacement-runtime reply, and transport-cutover-before-open branches [#3] - log ownership route mismatches in the server gate (assertRuntimeFileMutationExpectation), matching the file's other logged failure paths [#4] - add a web regression test for the non-connected SSH fail-closed path Co-authored-by: Orca --- .../files/mobile-file-mutation-ownership.ts | 47 +++++++------ .../files/mobile-markdown-note-create.test.ts | 67 +++++++++++++++++++ src/main/runtime/orca-runtime-files.ts | 8 +++ .../src/web/web-file-mutation-methods.test.ts | 15 +++++ src/shared/file-mutation-ownership.ts | 12 +++- 5 files changed, 126 insertions(+), 23 deletions(-) diff --git a/mobile/src/files/mobile-file-mutation-ownership.ts b/mobile/src/files/mobile-file-mutation-ownership.ts index 8e506063426..b71a272b9f8 100644 --- a/mobile/src/files/mobile-file-mutation-ownership.ts +++ b/mobile/src/files/mobile-file-mutation-ownership.ts @@ -3,8 +3,6 @@ import { assertFileMutationOwnershipCapability, buildFileMutationOwnership, FILE_MUTATION_OWNER_UNVERIFIED_MESSAGE, - FILE_MUTATION_RUNTIME_UNVERIFIED_MESSAGE, - FILE_MUTATION_SSH_UNVERIFIED_MESSAGE, type FileMutationOwnership } from '../../../src/shared/file-mutation-ownership' import { inferFolderWorkspacePathConnection } from '../../../src/shared/folder-workspace-path-connection' @@ -40,6 +38,13 @@ type MutationTargetHost = { runtimeIds: string[] } +// Why: all fence rejections share one generic user message, so log the distinguishing cause +// (mirroring the transport '[net]' convention) to keep a repeated failure diagnosable. +function runtimeChangedError(cause: string, detail?: Record): Error { + console.warn('[file-mutation-fence] runtime changed', { cause, ...detail }) + return new Error(RUNTIME_CHANGED_MESSAGE) +} + export function getMobileFileMutationFailureMessage(failure: RpcFailure): string { if ( failure.error.code === 'selector_not_found' || @@ -54,17 +59,9 @@ export function buildMobileFileMutationOwnership( worktreeHostId: string | null | undefined, sshState: SshConnectionState | null = null ): MobileFileMutationOwnership { - const host = parseExecutionHostId(worktreeHostId) - if (host?.kind === 'runtime') { - throw new Error(FILE_MUTATION_RUNTIME_UNVERIFIED_MESSAGE) - } - if (host?.kind === 'ssh' && sshState?.status !== 'connected') { - throw new Error(FILE_MUTATION_SSH_UNVERIFIED_MESSAGE) - } - const ownership = buildFileMutationOwnership(worktreeHostId, sshState) - return host?.kind === 'ssh' - ? { ...ownership, expectedExecutionHostId: toSshExecutionHostId(host.targetId) } - : ownership + // Ownership validation (runtime rejection, SSH liveness fence, canonical host id) lives in the + // shared builder so mobile and web stay identical. + return buildFileMutationOwnership(worktreeHostId, sshState) } export async function captureMobileFileMutationOwnership( @@ -82,7 +79,7 @@ export async function captureMobileFileMutationOwnership( throw normalizePreflightError(error) } } - throw new Error(RUNTIME_CHANGED_MESSAGE) + throw runtimeChangedError('preflight-retry-exhausted') } export function assertMobileFileMutationFenceCurrent( @@ -95,7 +92,10 @@ export function assertMobileFileMutationFenceCurrent( currentGeneration !== null && currentGeneration !== fence.transportGeneration ) { - throw new Error(RUNTIME_CHANGED_MESSAGE) + throw runtimeChangedError('transport-generation-changed', { + expected: fence.transportGeneration, + current: currentGeneration + }) } } @@ -103,8 +103,12 @@ export function assertMobileFileMutationResponseRuntime( fence: MobileFileMutationFence, response: RpcSuccess | RpcFailure ): void { - if (getResponseRuntimeId(response) !== fence.runtimeId) { - throw new Error(RUNTIME_CHANGED_MESSAGE) + const runtimeId = getResponseRuntimeId(response) + if (runtimeId !== fence.runtimeId) { + throw runtimeChangedError('mutation-runtime-mismatch', { + expected: fence.runtimeId, + actual: runtimeId + }) } } @@ -242,7 +246,7 @@ async function requestResult( function getResponseRuntimeId(response: RpcSuccess | RpcFailure): string { const runtimeId = response._meta?.runtimeId if (typeof runtimeId !== 'string' || !runtimeId.trim()) { - throw new Error(RUNTIME_CHANGED_MESSAGE) + throw runtimeChangedError('missing-response-runtime-id') } return runtimeId.trim() } @@ -268,7 +272,10 @@ function assertCaptureTransportCurrent( function assertSameRuntime(expectedRuntimeId: string, actualRuntimeId: string): void { if (actualRuntimeId !== expectedRuntimeId) { - throw new Error(RUNTIME_CHANGED_MESSAGE) + throw runtimeChangedError('capture-runtime-mismatch', { + expected: expectedRuntimeId, + actual: actualRuntimeId + }) } } @@ -292,7 +299,7 @@ function normalizePreflightError(error: unknown): Error { return new Error(PREFLIGHT_TIMEOUT_MESSAGE) } if (isLogicalClientCutoverError(error)) { - return new Error(RUNTIME_CHANGED_MESSAGE) + return runtimeChangedError('logical-cutover') } return error instanceof Error ? error : new Error(String(error)) } diff --git a/mobile/src/files/mobile-markdown-note-create.test.ts b/mobile/src/files/mobile-markdown-note-create.test.ts index d782c5f1edc..30f9cf242a7 100644 --- a/mobile/src/files/mobile-markdown-note-create.test.ts +++ b/mobile/src/files/mobile-markdown-note-create.test.ts @@ -66,6 +66,73 @@ describe('mobile markdown note creation fence', () => { ) }) + it('surfaces a files.open failure after a successful create', async () => { + const sendRequest = vi.fn(async (method: string) => { + if (method === 'status.get') { + return statusResponse() + } + if (method === 'worktree.show') { + return success({ worktree: { hostId: 'local' } }) + } + if (method === 'files.createFile') { + return success({ ok: true }) + } + if (method === 'files.open') { + return failure('Cannot open file') + } + throw new Error(`Unexpected RPC request: ${method}`) + }) + + await expect(createAndOpenMobileMarkdownNote({ sendRequest }, 'id:worktree-1')).rejects.toThrow( + 'Cannot open file' + ) + }) + + it('rejects a files.open reply from a replacement runtime', async () => { + const sendRequest = vi.fn(async (method: string) => { + if (method === 'status.get') { + return statusResponse() + } + if (method === 'worktree.show') { + return success({ worktree: { hostId: 'local' } }) + } + if (method === 'files.createFile') { + return success({ ok: true }) + } + if (method === 'files.open') { + return success({ opened: true }, 'runtime-new') + } + throw new Error(`Unexpected RPC request: ${method}`) + }) + + await expect(createAndOpenMobileMarkdownNote({ sendRequest }, 'id:worktree-1')).rejects.toThrow( + 'Orca server changed' + ) + }) + + it('stops before opening when the transport cuts over after a successful create', async () => { + let transportGeneration = 1 + const sendRequest = vi.fn(async (method: string) => { + if (method === 'status.get') { + return statusResponse() + } + if (method === 'worktree.show') { + return success({ worktree: { hostId: 'local' } }) + } + if (method === 'files.createFile') { + transportGeneration = 2 + return success({ ok: true }) + } + throw new Error(`Unexpected RPC request: ${method}`) + }) + const client = { sendRequest, getGeneration: () => transportGeneration } + + await expect(createAndOpenMobileMarkdownNote(client, 'id:worktree-1')).rejects.toThrow( + 'Orca server changed' + ) + expect(sendRequest.mock.calls.some(([method]) => method === 'files.open')).toBe(false) + }) + it('keeps one SSH ownership lease across name-collision retries', async () => { let createAttempts = 0 const sendRequest = vi.fn(async (method: string) => { diff --git a/src/main/runtime/orca-runtime-files.ts b/src/main/runtime/orca-runtime-files.ts index 47cc847e488..992e558e792 100644 --- a/src/main/runtime/orca-runtime-files.ts +++ b/src/main/runtime/orca-runtime-files.ts @@ -129,6 +129,14 @@ function assertRuntimeFileMutationExpectation( !declaredMatchesRoute || !executionHostMatchesFileRoute(expectedExecutionHostId, connectionId) ) { + // Why: this is the strict ownership gate; leave an audit trail for stale clients, reconnect + // races, and any spoofed-ownership attempt, matching this file's other logged failure paths. + console.warn('[runtime-files.mutation] ownership route mismatch', { + worktreeHostId, + connectionId, + expectedExecutionHostId, + declaredMatchesRoute + }) throw new Error('Workspace host changed; refresh and try again') } assertSshMutationExpectation(connectionId, expectedSshTargetId, expectedSshConnectionGeneration) diff --git a/src/renderer/src/web/web-file-mutation-methods.test.ts b/src/renderer/src/web/web-file-mutation-methods.test.ts index c6bdce30a26..61df031cc8c 100644 --- a/src/renderer/src/web/web-file-mutation-methods.test.ts +++ b/src/renderer/src/web/web-file-mutation-methods.test.ts @@ -231,6 +231,21 @@ describe('paired web file mutation methods', () => { expect(callRuntimeResult).not.toHaveBeenCalled() }) + it('fails closed before mutation when the SSH connection is not connected', async () => { + // Why: a reconnecting connection can still report a stale-but-defined generation; the shared + // builder must fence it the same way for web as it does for mobile. + getSshState.mockResolvedValueOnce({ + ...connectedSshState('hub-private-target', 17), + status: 'reconnecting' + }) + const methods = createWebFileMutationMethods({ captureSession }) + + await expect( + methods.writeFile({ filePath: '/ssh/repo/source.md', content: 'unsafe' }) + ).rejects.toThrow(FILE_MUTATION_SSH_UNVERIFIED_MESSAGE) + expect(callRuntimeResult).not.toHaveBeenCalled() + }) + it.each([ ['missing', undefined], ['invalid', 'runtime:' as Worktree['hostId']] diff --git a/src/shared/file-mutation-ownership.ts b/src/shared/file-mutation-ownership.ts index f23af1ccd17..e4747867e88 100644 --- a/src/shared/file-mutation-ownership.ts +++ b/src/shared/file-mutation-ownership.ts @@ -2,7 +2,7 @@ import { FILE_MUTATION_OWNERSHIP_RUNTIME_CAPABILITY, FILE_MUTATION_OWNERSHIP_UPDATE_REQUIRED_MESSAGE } from './protocol-version' -import { parseExecutionHostId } from './execution-host' +import { parseExecutionHostId, toSshExecutionHostId } from './execution-host' import type { RuntimeStatus } from './runtime-types' import type { SshConnectionState, SshMutationExpectation } from './ssh-types' @@ -39,11 +39,17 @@ export function buildFileMutationOwnership( if (host.kind === 'runtime') { throw new Error(FILE_MUTATION_RUNTIME_UNVERIFIED_MESSAGE) } - if (sshState?.targetId !== host.targetId || sshState.connectionGeneration === undefined) { + // Why: every client (mobile + web) must fence SSH mutations to a live, matching connection and + // a canonical host id — a reconnecting state with a stale-but-defined generation fails closed. + if ( + sshState?.status !== 'connected' || + sshState.targetId !== host.targetId || + sshState.connectionGeneration === undefined + ) { throw new Error(FILE_MUTATION_SSH_UNVERIFIED_MESSAGE) } return { - expectedExecutionHostId: host.id, + expectedExecutionHostId: toSshExecutionHostId(host.targetId), expectedSshTargetId: host.targetId, expectedSshConnectionGeneration: sshState.connectionGeneration }