Repository navigation
feat(terminal): upload local files in direct SSH panes - #198
Conversation
f377f89 to
e30c8d3
Compare
A pasted screenshot or a dropped file used to insert a local Windows path. Inside an ssh pane that path does not exist on the far side, so the paste was silently useless — and an agent running there could not read it. Detect that a pane is inside an interactive ssh session, scp the file to /tmp/wmux-drop-<uuid>.<ext> on that host, and type the shell-escaped remote path instead. Shift inverts on drop; `[remote]` in config.toml turns it off. Detection has three sources feeding one per-surface cache: the `wmux ssh` shell spec, a shell-integration preexec report of a typed ssh, and a background Win32_Process probe for the nested cases. The first two are latency caches over the third, which costs ~550ms per sweep on Windows where cmux gets the same answer from ps in milliseconds. Clipboard paste also handles a copied FILE, not just a bitmap: Explorer's Ctrl+C puts a file reference on the clipboard with no image and no text, so that paste previously did nothing at all while dragging the same file worked. Ported from cmux (manaflow-ai/cmux), including its scp argv composition, the IPv6 bracketing fix (PR #6874) and the all-or-nothing rollback. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The guard skipped recording the ssh session on a reused PTY because the renderer had by then overwritten SurfaceRef.shell with the resolved executable, so a remount re-reported a destination-less `…\ssh.exe` and downgraded a correctly detected pane to "local". That write now targets resolvedShell, so every call carries the requested spec and the special case has nothing left to defend against. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The orphan reaper and the ssh probe had each grown their own copy of the same invocation: absolute powershell.exe, -NoProfile -NonInteractive, windowsHide, pipe-delimited Write-Output, resolve-empty on failure. The security-relevant part — resolving the interpreter by absolute path so a writeable PATH directory cannot shadow it — was therefore stated twice, and `systemRoot()` four times across src/main. Extract queryWin32Processes(). Fields are PowerShell expressions rather than property names because both callers need computed columns: an epoch from CreationDate, and a CommandLine with newlines flattened. pty-ledger keeps queryProcesses / parseProcessLines / PROBE_TIMEOUT_MS as its public surface — tests pin all three — and delegates. Also resolves the name collision where ssh-detect declared a private PROBE_TIMEOUT_MS at 20s beside pty-ledger's exported 60s; the sweep's is now SWEEP_TIMEOUT_MS. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Every input to "what should this paste type" already lived in main — the clipboard, the ssh detector, the filesystem, scp, the user config — so the renderer was making up to four IPC round trips per Ctrl+V transcribing main's own data back to it, including one on every plain text paste that could never succeed. Main now answers the whole question. remote:resolve-paste reads its own clipboard; remote:resolve-drop takes the paths webUtils already resolved. Both return the finished text, so the renderer's Ctrl+V is one call and a terminal.paste(). - remote-file-insert.ts moves to main as remote-insert.ts, and shell-quote leaves shared/ for main/ — both were only in those places because the renderer had been given the job. The injected FileInsertionDeps become direct calls, so the tests need no window.wmux shims. - clipboard:paste-image and clipboard:read-files stop being IPC handlers and become private functions behind the resolvers. read-text / write-text stay: Ctrl+C and the Ctrl+Shift+V text path still use them. - REMOTE_DETECT is deleted rather than replaced by a push over METADATA_UPDATE. With the decision in main the renderer never asks whether a pane is remote, so there is no paste-time await left for a push to remove; the only remaining use would be a tab-bar affordance. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The upload failure notification was hardcoded English while useTerminal.ts
imports useT and uses it forty lines away for the bell.
Main hands back the host and the transport's own complaint rather than a
finished sentence, so the renderer — the only side that knows the user's
language — composes it. Placeholders follow the existing convention
(palette.newWorkspaceWithLayout): {host} and {reason} substituted after t().
English only. The non-English locales already sit ~18 keys behind en.ts, and
t()'s fallback renders those in English, so this matches the repo's state
rather than inventing seventeen translations.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Fixes from a reuse/simplification/efficiency/altitude pass over C1-C7. Efficiency, and the one real regression: resolve-paste armed the ssh probe before reading the clipboard, so a plain TEXT paste woke a ~550ms enumeration of every process on the machine — and since start() resets the idle counter, pasting more than once per 15s kept that sweep alive indefinitely. The probe is now armed only when the paste actually holds files. Also, the shared Win32_Process helper returned a split array that both callers immediately re-joined and re-split; it hands back raw stdout now, which is what the two line parsers already wanted. Reuse: the "wmux uses Windows' own OpenSSH, never PATH's" rule was stated in three places. It lives once in system32.opensshPath, which both the spawn path and the upload path call. The unreachable `scp` arm is gone (nothing sets a workspace shell to scp), as is the last hand-built System32 path in src/main — in the very file this branch taught to import the helper. Simplification: resolveInsertion took a policy object, a mode and an invert flag to compute one boolean; it takes the boolean. The config mapping moves to uploadEnabled() beside it, which keeps the two-toggle independence testable rather than stranding it in an IPC handler. InsertionResult was declared three times and agreed only structurally; UploadResult no longer crosses the boundary and moves to remote-upload. Two stale docblocks that outlived the handlers they described are gone. Altitude: updateSurface and patchSurface no longer accept `shell`, so the respawn spec cannot be overwritten again by construction rather than by comment — which is what C3 now stakes ssh detection on. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
wmux has two paste paths for historical reasons: Ctrl+V is intercepted by xterm's key handler, while the configurable `paste` action (Ctrl+Shift+V by default) arrives as a wmux:paste-terminal event on a separate effect. The second one read only text. That was survivable while both were dumb. Once Ctrl+V learned to upload images and Explorer-copied files, the two bindings quietly meant different things: with a screenshot on the clipboard, Ctrl+V uploaded it and typed the remote path while Ctrl+Shift+V did nothing at all — no error, no paste. Both now go through the same resolver. applyInsertion moves to module scope since two effects need it, which is also why the divergence existed. Shift is not an invert modifier here and cannot be — it is part of the binding — so both are plain "paste whatever is on the clipboard". Shift-invert remains drag-and-drop only. Verified against a real SSH host: a copied image file through Ctrl+Shift+V uploads (0 -> 1, remote file is the expected 120x50 PNG); plain text through the same binding uploads nothing and still types the text. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
e30c8d3 to
37d8371
Compare
|
Merged. I rebased the branch onto master myself once #196 and #197 landed (the squash-merge changed their SHAs, so the branch went This is an unusually well-argued contribution and I want to be specific about why I took it at this size. The Windows safety boundary is the whole thing, and you got it right. The precedence rule in Other things I checked and liked:
One follow-up I pushed on top (
That change made your Also in that commit: Two notes for anyone reading later, neither blocking:
Shipping in v1.12.0. Thanks — this raised the bar. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e30c8d3cbd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| IPC_CHANNELS.REMOTE_RESOLVE_DROP, | ||
| (event, surfaceId: SurfaceId, localPaths: unknown, invert: boolean) => { | ||
| if (!ownsLiveSurface(surfaceId, event.sender)) return { text: null }; | ||
| const source: PasteSource = { kind: 'files', localPaths: regularFilePaths(localPaths) }; |
There was a problem hiding this comment.
When a user drops a directory into any terminal, regularFilePaths removes it before resolveInsertion can choose the local-path fallback, leaving an empty source and inserting nothing. This also affects remote panes when upload is disabled or Shift is held, and regresses the previous drop handler, which inserted every path returned by webUtils; pass the authenticated dropped paths through and let resolveInsertion reject non-regular files for upload.
Useful? React with 👍 / 👎.
| # Accept bare ssh, quoted paths (including spaces), unquoted absolute paths, | ||
| # and PowerShell's call operator. Keep the executable token exact so words | ||
| # such as `myssh.exe` are not mistaken for the OpenSSH client. | ||
| if ($line -notmatch '^\s*(?:&\s*"ssh(?:\.exe)?"|(?:&\s*)?"[^"]*[\\/]ssh(?:\.exe)?"|(?:&\s*)?[^\s"]*[\\/]ssh(?:\.exe)?|(?:&\s*)?ssh(?:\.exe)?)(?:\s|$)') { return } |
There was a problem hiding this comment.
When SSH is invoked through PowerShell's call operator, such as & "C:\Program Files\OpenSSH\ssh.exe" user@host, this pattern accepts and reports the full line beginning with &, but sshSessionFrom only accepts command lines whose first parsed argument normalizes to ssh. The report is therefore discarded, and because refresh refuses probe-only evidence, paste/drop falls back to a local path rather than uploading; normalize away the call operator before sending or parsing the report.
Useful? React with 👍 / 👎.
|
Released: v1.12.0. CI green end to end — 1,536 tests, resource verification, electron-builder package, portable zip and |
Summary
Fileconversion, lifecycle report ordering, nested-session ambiguity, and configuration validationThe design follows cmux's user-facing behavior where Windows provides equivalent evidence. Windows has no reliable tty foreground process group, so process ancestry is deliberately corroborating evidence only and can never independently select an upload destination. Unknown or contradictory destinations fail closed.
Dependencies
fix/ssh-path-resolution)fix/preserve-shell-spec)This PR is opened as a draft because its branch currently contains those two foundation fixes. Once they merge, the branch can be rebased and the resulting feature-only diff marked ready.
Validation
- automated SSH/SCP upload tests cover argument fidelity, matching client selection, private remote-directory setup, multi-file ordering, cleanup, failures, and cancellation
- detector tests cover managed and shell-reported sessions, stale/out-of-order reports, nested ambiguity, and the Windows background-SSH fail-safe
- renderer/preload tests cover genuine DOM
- PowerShell integration parser check
8000npm test: 1,530 passed, 11 skippedFileconversion, surface ownership, queued insertion, remounts, and teardownnpm run typechecknpm run build:mainnpm run build:renderernpm run verify:resourcesgit diff --checkLive end-to-end verification
The feature was also exercised through a running wmux/Claude Code terminal against a real SSH host using Windows OpenSSH:
Ctrl+Shift+Vincreased the remote upload count from 0 to 1The subsequent v1.11 rebase and final private-directory, ownership, cancellation, and fail-safe hardening were covered by the automated tests above. That final hardened revision was not separately re-smoked against the host.
Repository-wide lint remains at the existing upstream baseline of 14 errors and 13 warnings; this branch adds no new lint findings.
Closes #195