fix(editor): read out-of-worktree SSH paths without a stamped target id (#9743) - #10455
Merged
Conversation
…id (#9743) Route external absolute-path tabs by their resolved SSH connection instead of the externalSshTargetId stamp, which only the terminal-link open path sets. The stamp keeps its fail-closed role when present.
Contributor
📝 WalkthroughWalkthroughRefines restored external-file loading to derive ownership from the tab’s SSH target or resolved connection. Client-local external paths are authorized before reading, SSH-owned paths skip authorization, and unstamped external tabs in remote runtime workspaces produce a load error without reading runtime content. Tests add the authorization mock and cover all three behaviors. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…H host AI Vault "View Log" tabs are opened client-local by construction (readOnly + liveTail, no runtime env), so inferring the external SSH owner from the worktree connection made them read the remote host instead of the granted client path. Restrict that inference to non-live-tail tabs; an explicit externalSshTargetId stamp still routes remotely.
…2025/fix-ssh-absolute-path
This was referenced Jul 27, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
externalSshTargetIdstamp.authorizeExternalPathgrant unchanged.Closes #9743.
Root Cause
#10323 fixed this bug for terminal file links by stamping
externalSshTargetIdon the tab and using the stamp's presence as the "this path lives on the remote host" signal:But
terminal-file-open-routing.tsis the only producer of that stamp. Every other way an external absolute-path tab comes into existence gets no stamp and still hits the throw:externalSshTargetIdisundefined— the PR notes this as "may require reopening those tabs once after upgrade", but the tab errors forever until reopened from a terminal link)openFilecaller (floating-workspace tab creation, AI Vault log open, check-annotation open, IPC open-file, diff viewer)The stamp is used purely as a fail-closed expectation everywhere else in the codebase (
assertExternalSshReadOwnership,editor-file-operation-owner,editor-restored-tab-conflict-scan,useEditorExternalWatch,rich-markdown-image-context) — each of those resolves the connection independently viagetConnectionIdForFile.loadFileContentwas the one place that overloaded it as a routing signal, so it was the one place that broke.The fix resolves ownership the same way its siblings do:
externalSshTargetId?.trim() || connectionId. As #9743 documents,fs:readFilealready accepts host-absolute paths for an SSHconnectionId— the renderer was rejecting before the call.This is safe with respect to #10323's fail-closed intent: an unstamped external tab in an SSH workspace was never readable (the throw predates the stamp), so no client-local read is being reinterpreted. When a stamp is present,
assertExternalSshReadOwnershipstill rejects after a host change.Second Commit — Live-Tail Regression Fix
Owner inference from
connectionIdis correct for an ordinary external path, but it wrongly captured AI Vault View Log tabs, which are client-local by construction:ai-vault-session-log-open.tsis the only producer ofliveTail: truein the repo (the hits inlib/workspace-session.tsandstore/slices/editor.tsare persist/restore passthrough). It opens withreadOnly: true, liveTail: true, runtimeEnvironmentId: null, relativePath === filePath.canOpenAiVaultSessionLogInOrca→canUseLocalAiVaultSessionPathActions, which requiresnormalizeExecutionHostId(executionHostId) === LOCAL_EXECUTION_HOST_ID. Remote-host sessions never reachopenFile, and the opener's docstring states the invariant: "never redirects the open to a remote host".getConnectionIdForFileresolves the worktree's SSH target regardless of path (the per-file branch inconnection-owner-resolution.tsonly applies to folder workspaces).So with an SSH workspace active, the first commit made View Log read from the remote host — ENOENT normally, but the wrong host's file on a colliding path shape (mac→mac target, same username), with
includeLocalLogMetadatadropped on the SSH branch so live tail silently degrades. Worse,useLocalLogTail'sisLocalLiveLogdoes not checkconnectionId, so a remote read that returned afileIdentitywould have armed a local tail against a remote path. Onmainthis state was a hard error, so the first commit alone traded an error for silent wrong data.The fix restricts the inference and keeps an explicit stamp authoritative:
and after the client-local grant,
readConnectionId = undefinedkeeps the read off the SSH host.readConnectionIdis threaded into bothgetRuntimeFileReadScopeandreadRuntimeFileContentdeliberately, not incidentally:inFlightReadKeyis`${connectionId ?? ''}::${filePath}`, so leaving the scope onconnectionIdwould register the client-local read underssh-1::<path>— where it could attach to an already-registered remote promise for that path (or vice versa) and render the wrong host's bytes through the other door. Keying it::<path>is what makes the local read local; two local live-tail reads of the same path still dedupe. ThereadOnly && liveTailexpression is hoisted once and reused forincludeLocalLogMetadata(textually identical to the expression it replaced).A stamped live-tail tab keeps its remote route — the guard applies to owner inference only. Sending such a tab local while still passing
expectedExternalSshTargetIdwould tripassertExternalSshReadOwnership. No stamped live-tail tab exists today (the stamp comes only fromterminal-file-open-routing.ts, which never setsliveTail), so this is latent, but the ordering is correct now rather than accidentally correct.Screenshots
/work/reports/fresh-unstamped.md— an absolute path outside the SSH worktree, opened with noexternalSshTargetId, rendering remote content over the worktree's SSH connection:Testing
pnpm lintpnpm typecheckpnpm test— 1295 tests / 146 files acrosscomponents/editor,runtime-file-client,editor-file-operation-owner,terminal-link-handlers,store/slices/editor(1319 / 147 includinguseEditorExternalWatch)pnpm buildpnpm run check:max-lines-ratchet,git diff --checkFour new unit tests in
useEditorPanelContentState.test.tsx:mainauthorizeExternalPathReal SSH validation (
brennan-test-ssh)Throwaway Ubuntu 22.04 Docker container as the SSH target (not
ssh localhost), key-only auth, Node 22 for the relay; real git repo at/work/demo-projectregistered in Orca as a remote repo;pnpm build:relaybefore connecting; dev Orca launched from this worktree with isolatedORCA_DEV_USER_DATA_PATH+ CDP, identity verified assablefish @ brennanb2025/fix-ssh-absolute-path.Remote target state:
Main-process boundary already allowed the read (confirms the renderer was the blocker):
Before the fix — external tab with no stamp vs. the same tab with the #10323 stamp:
After the fix — unstamped open, and the same tab after a renderer reload (the session-restore case):
Console/app/relay logs inspected for read and relay errors — none related. Container, temp key, temp user-data, and the dev process spawned for this run were all removed; no test-prefixed remote artifacts remain.
AI Review Report
Reviewed every consumer of
externalSshTargetIdto confirm the routing overload was localized toloadFileContent:runtime-file-client.assertExternalSshReadOwnership,editor-file-operation-owner(mutation provenance),editor-restored-tab-conflict-scan,useEditorExternalWatch,rich-markdown-image-context/useLocalImageSrc, andmobile-markdown-bridgeall resolveconnectionIdindependently and treat the stamp as an expectation only — so writes, watches, images, conflict scans, and mobile reads already worked without a stamp and needed no change.Checked the branch ordering so message selection is preserved: remote-runtime workspaces still get "External local files are not available for remote workspaces."; a stamped tab whose host changed still gets "External SSH files are not available after the workspace host changes." from
assertExternalSshReadOwnership. Verified the unresolved-owner path (WORKTREE_OWNER_NOT_READY_ERROR, #6648) still runs first, so an SSH workspace that is still connecting fails retryably rather than resolving to a local read. Folder workspaces resolve through the samegetConnectionIdForFilepath as git worktrees (covered by the pre-existing folder-workspace test). No keyboard shortcut, path separator, or platform-conditional behavior is touched; the change is renderer-only and platform-neutral.Out of scope and left alone: the new-tab entry (
+→ absolute path) still hard-blocks absolute paths in SSH/runtime workspaces viaTAB_ENTRY_ABSOLUTE_PATH_REMOTE_BLOCKED_MESSAGE(isTabEntryAbsolutePathAllowed→isLocalPathOpenBlocked). That is a deliberate block with its own distinct message, not this bug — worth a separate change now that the read path supports it.Multi-round subagent review
Three independent Opus review rounds ran against this branch. Round 1 cleared the read-path fix (SSH provider routing, session restore, the save/autosave path, host-change fail-closed, perf) and independently confirmed the key test is red on
mainby swapping inorigin/main's source:1 failed | 19 passed. It logged the AI Vault live-tail hazard as an unacted observation; on review that was judged a real regression and fixed in the second commit.The final round enumerated every reachable combination of {external vs in-worktree} × {stamped vs unstamped} × {local / SSH / runtime-env owner} × {live-tail or not} and confirmed each route, verifying that only three rows change versus
main: the #9743 fix, the live-tail fix, and one message-only difference (below). It re-derived the red/green evidence itself, including that each half of the live-tail guard is independently load-bearing (1 failed | 20 passedwith either removed) and that the addedonLocalLogTailChangedmock is required rather than dead scaffolding (useLocalLogTailis unmocked in that file and arms its subscription for anyreadOnly && liveTailtab).Known and deliberately not changed:
connectionIdand an active runtime environment previously threw "External local files are not available for remote workspaces." and now throws the vaguer "Remote file is outside the owning runtime worktree" fromcanReadRelativeRuntimeFile. Verified still fail-closed with no read for every absolute path form (POSIX, drive-letter, UNC), and the!worktreeIdearly-return that would read remotely is unreachable here because a falsyworktreeIdimplies a falsyconnectionId. I could not demonstrate the state is reachable, so it is not worth reshaping the core condition on speculation.useEditorPanelContentState.test.tsxsits at 796 of its 800max-linescap (4 counted lines of headroom, measured empirically).max-linesdisables are forbidden byAGENTS.md, so that file needs a split before further tests land there. Filed as a follow-up rather than expanded into this PR.getRuntimeFileReadScopedouble returnsconnectionId ?? activeRuntimeEnvironmentId, inverting the real function's environment-first precedence. Harmless today (no test sets both) but it would mask a future regression in the message-only row above.Security Audit
No IPC surface, protocol, dependency, command construction, auth, or secret handling changed. Host-absolute reads still go through the SSH filesystem provider behind
fs:readFilewith an explicitconnectionId— the same boundary #9743 documents and the same one the terminal-link path has used since #10323. Client-local external paths still require the in-memoryfs:authorizeExternalPathgrant, and that grant is now not requested for remote-owned paths, which narrows the client-local authorization surface rather than widening it. Remote-runtime workspaces keep their worktree-scopedfiles.readrestriction. Mutations are unaffected:getEditorFileOperationContextstill fails closed on an owner change.Notes
useEditorPanelContentState.test.tsx(4 lines from themax-linescap), then add the in-flight dedupe-collision test; and open out-of-worktree absolute paths from the+new-tab entry,activateMarkdownLink, andMarkdownPreviewlinks, all of which still refuse paths the read path now supports.