Skip to content

fix(editor): read out-of-worktree SSH paths without a stamped target id (#9743) - #10455

Merged
brennanb2025 merged 3 commits into
mainfrom
brennanb2025/fix-ssh-absolute-path
Jul 25, 2026
Merged

fix(editor): read out-of-worktree SSH paths without a stamped target id (#9743)#10455
brennanb2025 merged 3 commits into
mainfrom
brennanb2025/fix-ssh-absolute-path

Conversation

@brennanb2025

@brennanb2025 brennanb2025 commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Route external (out-of-worktree) absolute-path editor tabs by their resolved SSH connection, not by the externalSshTargetId stamp.
  • Keep the stamp's real job: fail closed when a stamped tab's host changes.
  • Keep the remote-runtime rejection and the client-local authorizeExternalPath grant unchanged.
  • Second commit: exempt client-local live-tail (AI Vault "View Log") tabs from that owner inference, so they keep reading the granted client path inside an SSH workspace.

Closes #9743.

Root Cause

#10323 fixed this bug for terminal file links by stamping externalSshTargetId on the tab and using the stamp's presence as the "this path lives on the remote host" signal:

if (!externalSshTargetId && (runtimeActive || connectionId)) {
  throw new Error('External local files are not available for remote workspaces.')
}

But terminal-file-open-routing.ts is 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:

  • session restore of a tab persisted before the field existed (externalSshTargetId is undefined — the PR notes this as "may require reopening those tabs once after upgrade", but the tab errors forever until reopened from a terminal link)
  • any other openFile caller (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 via getConnectionIdForFile. loadFileContent was 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:readFile already accepts host-absolute paths for an SSH connectionId — 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, assertExternalSshReadOwnership still rejects after a host change.

Second Commit — Live-Tail Regression Fix

Owner inference from connectionId is 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.ts is the only producer of liveTail: true in the repo (the hits in lib/workspace-session.ts and store/slices/editor.ts are persist/restore passthrough). It opens with readOnly: true, liveTail: true, runtimeEnvironmentId: null, relativePath === filePath.
  • It is gated by canOpenAiVaultSessionLogInOrcacanUseLocalAiVaultSessionPathActions, which requires normalizeExecutionHostId(executionHostId) === LOCAL_EXECUTION_HOST_ID. Remote-host sessions never reach openFile, and the opener's docstring states the invariant: "never redirects the open to a remote host".
  • getConnectionIdForFile resolves the worktree's SSH target regardless of path (the per-file branch in connection-owner-resolution.ts only 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 includeLocalLogMetadata dropped on the SSH branch so live tail silently degrades. Worse, useLocalLogTail's isLocalLiveLog does not check connectionId, so a remote read that returned a fileIdentity would have armed a local tail against a remote path. On main this 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:

const isLiveTailLogTab = restoredOpenFile?.readOnly === true && restoredOpenFile.liveTail === true
const externalSshOwnerId =
  restoredOpenFile.externalSshTargetId?.trim() || (isLiveTailLogTab ? undefined : connectionId)

and after the client-local grant, readConnectionId = undefined keeps the read off the SSH host. readConnectionId is threaded into both getRuntimeFileReadScope and readRuntimeFileContent deliberately, not incidentally: inFlightReadKey is `${connectionId ?? ''}::${filePath}`, so leaving the scope on connectionId would register the client-local read under ssh-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. The readOnly && liveTail expression is hoisted once and reused for includeLocalLogMetadata (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 expectedExternalSshTargetId would trip assertExternalSshReadOwnership. No stamped live-tail tab exists today (the stamp comes only from terminal-file-open-routing.ts, which never sets liveTail), 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 no externalSshTargetId, rendering remote content over the worktree's SSH connection:

fixed

Testing

  • pnpm lint
  • pnpm typecheck
  • pnpm test — 1295 tests / 146 files across components/editor, runtime-file-client, editor-file-operation-owner, terminal-link-handlers, store/slices/editor (1319 / 147 including useEditorExternalWatch)
  • pnpm build
  • Added or updated high-quality tests that would catch regressions
  • pnpm run check:max-lines-ratchet, git diff --check

Four new unit tests in useEditorPanelContentState.test.tsx:

test red without fix
unstamped external SSH tab reads through the resolved connection (and does not request the client-local grant) ✅ fails on main
client-local external tab still calls authorizeExternalPath guard (passes both ways)
unstamped external tab in a remote runtime workspace still rejects guard (passes both ways)
client-local live-tail log tab stays on the client inside an SSH workspace ✅ fails with either half of the live-tail guard removed

Real 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-project registered in Orca as a remote repo; pnpm build:relay before connecting; dev Orca launched from this worktree with isolated ORCA_DEV_USER_DATA_PATH + CDP, identity verified as sablefish @ brennanb2025/fix-ssh-absolute-path.

Remote target state:

sshState: {"status":"connected","remotePlatform":"linux","supportsFolderDownload":true}
repo:     {"path":"/work/demo-project","connectionId":"ssh-1784933456972-5vj5dy"}
$ ssh ... 'git -C /work/demo-project status --short --branch'  →  ## main

Main-process boundary already allowed the read (confirms the renderer was the blocker):

rawSshRead: {"ok":true,"content":"# External Report\n\nabsolute-path payload OK\n"}

Before the fix — external tab with no stamp vs. the same tab with the #10323 stamp:

A.tab: {"externalSshTargetId":null}          A.errorTextVisible: true    ← bug
B.tab: {"externalSshTargetId":"ssh-1784..."} B.errorTextVisible: false   B.payloadVisible: true

After the fix — unstamped open, and the same tab after a renderer reload (the session-restore case):

tab:         {"externalSshTargetId":null}   errorTextVisible: false   payloadVisible: true
restoredTab: {"externalSshTargetId":null}   errorTextVisible: false   payloadVisible: true

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 externalSshTargetId to confirm the routing overload was localized to loadFileContent: runtime-file-client.assertExternalSshReadOwnership, editor-file-operation-owner (mutation provenance), editor-restored-tab-conflict-scan, useEditorExternalWatch, rich-markdown-image-context / useLocalImageSrc, and mobile-markdown-bridge all resolve connectionId independently 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 same getConnectionIdForFile path 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 via TAB_ENTRY_ABSOLUTE_PATH_REMOTE_BLOCKED_MESSAGE (isTabEntryAbsolutePathAllowedisLocalPathOpenBlocked). 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 main by swapping in origin/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 passed with either removed) and that the added onLocalLogTailChanged mock is required rather than dead scaffolding (useLocalLogTail is unmocked in that file and arms its subscription for any readOnly && liveTail tab).

Known and deliberately not changed:

  • Message-only difference. A tab with both an SSH connectionId and 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" from canReadRelativeRuntimeFile. Verified still fail-closed with no read for every absolute path form (POSIX, drive-letter, UNC), and the !worktreeId early-return that would read remotely is unreachable here because a falsy worktreeId implies a falsy connectionId. I could not demonstrate the state is reachable, so it is not worth reshaping the core condition on speculation.
  • No test pins the in-flight dedupe reasoning, and no test covers a stamped live-tail tab or the row above — all three are unreachable with today's producers, and useEditorPanelContentState.test.tsx sits at 796 of its 800 max-lines cap (4 counted lines of headroom, measured empirically). max-lines disables are forbidden by AGENTS.md, so that file needs a split before further tests land there. Filed as a follow-up rather than expanded into this PR.
  • The test file's getRuntimeFileReadScope double returns connectionId ?? 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:readFile with an explicit connectionId — 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-memory fs:authorizeExternalPath grant, 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-scoped files.read restriction. Mutations are unaffected: getEditorFileOperationContext still fails closed on an owner change.

Notes

…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.
@coderabbitai

coderabbitai Bot commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

Refines 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)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed The changes satisfy #9743 by routing unstamped SSH external paths through the resolved connection while preserving remote-runtime rejection and local authorization.
Out of Scope Changes check ✅ Passed The changes are focused on the editor load path and tests, with no clear unrelated or out-of-scope additions.
Title check ✅ Passed The title is concise and accurately summarizes the main change: routing unstamped SSH out-of-worktree paths.
Description check ✅ Passed The description covers all required template sections with substantive details for summary, screenshots, testing, review, security, and notes.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

…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.
@brennanb2025
brennanb2025 merged commit b9a7c00 into main Jul 25, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant