From 207734e91557d4280ea1f9ff00a2fd98a6318acf Mon Sep 17 00:00:00 2001 From: roboomp Date: Sun, 28 Jun 2026 11:52:44 +0000 Subject: [PATCH] fix(ssh): dispatch transfer snippets through verified transferShell The previous fix gated on a verified `transferShell` but still let OpenSSH hand the snippet to whatever `$SHELL` happens to be on the remote. On a fish/csh/tcsh host the new gate would accept the host, then fail anyway because the login shell can't parse `if [ ... ]; then ...` (P1 from PR #3722 review). Each transfer command (read, write, stat, list) is now wrapped in ` -c '...'` via a shared `wrapInPosixShell` helper, so the snippet is parsed by the same shell OMP's capability probe verified can run it. `ensurePosixRemote` returns the verified shell so each call site can do the wrap. `ssh-executor.ts`'s identical Windows-compat helper is consolidated onto the same primitive (`buildCompatCommand` / `quoteForCompatShell` removed). Stays POSIX-clean across all four call sites; `-c` (not `-lc`) since the snippets only call absolute builtins and don't need login-profile setup. Capability *probing* still uses `-lc` to mirror the user env. Test additions: one case asserts every dispatch starts with `bash -c '...'` and embeds the original POSIX snippet (read/write/stat/list) when transferShell is bash and login shell is unknown; another covers the `sh -c` happy path. --- packages/coding-agent/CHANGELOG.md | 2 +- .../file-transfer-posix-guard.test.ts | 38 ++++++++++++------- .../coding-agent/src/ssh/file-transfer.ts | 36 +++++++++--------- packages/coding-agent/src/ssh/ssh-executor.ts | 15 +------- packages/coding-agent/src/ssh/utils.ts | 19 ++++++++++ 5 files changed, 65 insertions(+), 45 deletions(-) diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index ff7ed37aa..8c6c506af 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -39,7 +39,7 @@ - Fixed reasoning streaming being locked off for OpenAI-compatible providers that stream reasoning content without advertising reasoning support in model metadata. - Fixed `/shake` and other mid-stream chat rebuilds erasing live LLM output by preserving the in-flight streaming components and pending tools. - Fixed the `time_spent` status-line segment ticking continuously during idle sessions by ensuring it only accumulates active agent execution windows and resets correctly across session switches. -- Fixed `ssh://` rejecting POSIX-capable remotes whose login-shell classification was ambiguous (`shell: "unknown"`), by verifying a working transfer shell directly (`sh -lc`/`bash -lc`/`zsh -lc`) and gating transfers on that capability instead of the self-reported login-shell name. The host probe is now marker-framed to ignore login-banner noise, and ambiguous non-Windows cache entries are treated as stale. ([#3719](https://github.com/can1357/oh-my-pi/issues/3719)) +- Fixed `ssh://` rejecting POSIX-capable remotes whose login-shell classification was ambiguous (`shell: "unknown"`), by verifying a working transfer shell directly (`sh -lc`/`bash -lc`/`zsh -lc`) and gating transfers on that capability instead of the self-reported login-shell name. Transfer commands are now dispatched through the verified shell (` -c '…'`) so a remote whose login shell is fish/csh/tcsh still parses the POSIX snippets correctly. The host probe is also marker-framed to ignore login-banner noise, and ambiguous non-Windows cache entries are treated as stale. ([#3719](https://github.com/can1357/oh-my-pi/issues/3719)) ## [16.2.2] - 2026-06-27 diff --git a/packages/coding-agent/src/ssh/__tests__/file-transfer-posix-guard.test.ts b/packages/coding-agent/src/ssh/__tests__/file-transfer-posix-guard.test.ts index 970ef3626..f0dbd8e7e 100644 --- a/packages/coding-agent/src/ssh/__tests__/file-transfer-posix-guard.test.ts +++ b/packages/coding-agent/src/ssh/__tests__/file-transfer-posix-guard.test.ts @@ -1,7 +1,7 @@ import { afterEach, describe, expect, it, vi } from "bun:test"; import type { SSHConnectionTarget } from "../connection-manager"; import * as connectionManager from "../connection-manager"; -import { readRemoteFile, writeRemoteFile } from "../file-transfer"; +import { listRemoteDir, readRemoteFile, statRemotePath, writeRemoteFile } from "../file-transfer"; describe("ssh file-transfer POSIX guard", () => { afterEach(() => { @@ -46,15 +46,18 @@ describe("ssh file-transfer POSIX guard", () => { ); }); - it("allows a non-Windows remote whose transferShell is verified even when login shell is unknown", async () => { - // The bug fix: ssh:// must accept a POSIX-capable host even when the - // first-line probe couldn't classify the login shell, as long as a - // capability probe verified sh/bash/zsh works. Stop at buildRemoteCommand - // so we capture the dispatch without spawning a real ssh. + it("dispatches transfer commands through the verified transferShell, not the login shell", async () => { + // The bug fix: if the login shell is fish/csh/tcsh, the legacy guard + // would refuse the host — but allowing it isn't enough on its own. + // OpenSSH still hands our snippets to `$SHELL -c`, so a fish login + // shell would choke on `if [ … ]; then …`. Every transfer command + // must be wrapped in ` -c '…'` to force parsing + // under the shell we verified can run it (#3719). vi.spyOn(connectionManager, "ensureConnection").mockResolvedValue(undefined); vi.spyOn(connectionManager, "ensureHostInfo").mockResolvedValue({ version: 4, os: "linux", + // Login shell is fish; only `transferShell` indicates a working POSIX shell. shell: "unknown", transferShell: "bash", compatEnabled: false, @@ -62,20 +65,27 @@ describe("ssh file-transfer POSIX guard", () => { const buildSpy = vi .spyOn(connectionManager, "buildRemoteCommand") .mockRejectedValue(new Error("stop-before-spawn")); - const target: SSHConnectionTarget = { name: "shellnoise", host: "shellnoise" }; + const target: SSHConnectionTarget = { name: "fishbox", host: "fishbox" }; await expect(readRemoteFile(target, "/etc/hosts", { maxBytes: 1024 })).rejects.toThrow(/stop-before-spawn/); await expect(writeRemoteFile(target, "/tmp/x", new Uint8Array([1]), {})).rejects.toThrow(/stop-before-spawn/); + await expect(statRemotePath(target, "/etc/hosts")).rejects.toThrow(/stop-before-spawn/); + await expect(listRemoteDir(target, "/etc")).rejects.toThrow(/stop-before-spawn/); - // Reached buildRemoteCommand → the guard let us through despite the - // login-shell being "unknown". Commands are still sent verbatim (no - // `sh -c` wrapper) and write keeps its stdin staging. - expect(buildSpy.mock.calls[0]?.[1]).toContain("head -c 1025"); + // Each dispatch must start with `bash -c '…'` and embed the original + // POSIX snippet inside the quoted command. Read also drops `-n` + // (allowStdin: true) because cat-staging needs stdin streaming. + const dispatches = buildSpy.mock.calls.map(call => call[1] as string); + expect(dispatches[0]).toMatch(/^bash -c '.*head -c 1025/); + expect(dispatches[1]).toMatch(/^bash -c '.*cat > /); expect(buildSpy.mock.calls[1]?.[2]).toMatchObject({ allowStdin: true }); + expect(dispatches[2]).toMatch(/^bash -c '.*if \[ -d /); + expect(dispatches[3]).toMatch(/^bash -c '.*LC_ALL=C ls -1Ap /); }); - it("allows a POSIX login shell when transferShell is also set", async () => { - // Belt-and-suspenders: the common happy path where both fields agree. + it("uses sh -c when transferShell is sh (the most universal POSIX fallback)", async () => { + // Belt-and-suspenders: the common happy path with a sh-family login + // shell still routes through `sh -c` to keep one dispatch shape. vi.spyOn(connectionManager, "ensureConnection").mockResolvedValue(undefined); vi.spyOn(connectionManager, "ensureHostInfo").mockResolvedValue({ version: 4, @@ -90,6 +100,6 @@ describe("ssh file-transfer POSIX guard", () => { const target: SSHConnectionTarget = { name: "shbox", host: "shbox" }; await expect(readRemoteFile(target, "/etc/hosts", { maxBytes: 1024 })).rejects.toThrow(/stop-before-spawn/); - expect(buildSpy.mock.calls[0]?.[1]).toContain("head -c 1025"); + expect(buildSpy.mock.calls[0]?.[1]).toMatch(/^sh -c '.*head -c 1025/); }); }); diff --git a/packages/coding-agent/src/ssh/file-transfer.ts b/packages/coding-agent/src/ssh/file-transfer.ts index d755ea995..90bafd11f 100644 --- a/packages/coding-agent/src/ssh/file-transfer.ts +++ b/packages/coding-agent/src/ssh/file-transfer.ts @@ -8,23 +8,24 @@ */ import { ptree } from "@oh-my-pi/pi-utils"; import { buildRemoteCommand, ensureConnection, ensureHostInfo, type SSHConnectionTarget } from "./connection-manager"; -import { quotePosixPath } from "./utils"; +import { quotePosixPath, wrapInPosixShell } from "./utils"; /** Per-operation timeout for remote transfers (matches the ssh tool's grep window). */ const DEFAULT_TIMEOUT_MS = 30_000; /** - * Ensure the ControlMaster connection and restrict transfers to remotes where - * OMP has verified a working POSIX transfer shell (`info.transferShell`). + * Ensure the ControlMaster connection and pick the verified POSIX shell to + * run transfer commands under. Returns the shell name so the caller can + * wrap its snippet in ` -c '…'`; OpenSSH otherwise hands the command + * to the user's login shell, which on fish/csh/tcsh hosts can't parse our + * `if [ … ]; then …` constructs (#3719). * * Windows hosts are refused up front — `ssh://` runs `head`/`cat`/`mv`/`test` - * directly, and cmd/powershell can't drive those. Everywhere else, we trust - * the capability probe (`sh -lc` / `bash -lc` / `zsh -lc` against the remote) - * over the self-reported login-shell name, because login-shell classification - * can be wrong or noisy (#3719). When the probe couldn't verify any candidate, - * `transferShell` is `undefined` and the guard rejects. + * directly and cmd/powershell can't drive those. Everywhere else, we require + * a non-empty `transferShell` (set by `probeHostInfo` after `sh -lc` / + * `bash -lc` / `zsh -lc` round-trips a marker against the remote). */ -async function ensurePosixRemote(target: SSHConnectionTarget): Promise { +async function ensurePosixRemote(target: SSHConnectionTarget): Promise<"sh" | "bash" | "zsh"> { await ensureConnection(target); const info = await ensureHostInfo(target); if (info.os === "windows") { @@ -37,6 +38,7 @@ async function ensurePosixRemote(target: SSHConnectionTarget): Promise { `ssh://: ${target.name} has no verified POSIX shell for ssh:// read/write — none of sh/bash/zsh round-tripped a capability probe (use the ssh tool for this host)`, ); } + return info.transferShell; } export interface RemoteFileReadOptions { @@ -70,9 +72,9 @@ export async function readRemoteFile( remotePath: string, opts: RemoteFileReadOptions, ): Promise { - await ensurePosixRemote(target); + const shell = await ensurePosixRemote(target); const command = `head -c ${opts.maxBytes + 1} ${quotePosixPath(remotePath)}`; - const args = await buildRemoteCommand(target, command); + const args = await buildRemoteCommand(target, wrapInPosixShell(shell, command)); using child = ptree.spawn(["ssh", ...args], { signal: ptree.combineSignals(opts.signal, opts.timeoutMs ?? DEFAULT_TIMEOUT_MS), }); @@ -113,7 +115,7 @@ export async function writeRemoteFile( content: Uint8Array, opts: RemoteFileWriteOptions, ): Promise { - await ensurePosixRemote(target); + const shell = await ensurePosixRemote(target); if (remotePath.endsWith("/")) { throw new Error("ssh://: destination is a directory path (trailing '/'); ssh:// write requires a file path"); } @@ -138,7 +140,7 @@ export async function writeRemoteFile( `elif [ -e ${dest} ] && [ ! -L ${dest} ]; then echo 'ssh://: destination is a special file (not a regular file)' >&2; exit 1; ` + `else mv "$t" ${dest}; fi; ` + `}`; - const args = await buildRemoteCommand(target, command, { allowStdin: true }); + const args = await buildRemoteCommand(target, wrapInPosixShell(shell, command), { allowStdin: true }); using child = ptree.spawn(["ssh", ...args], { stdin: content, signal: ptree.combineSignals(opts.signal, opts.timeoutMs ?? DEFAULT_TIMEOUT_MS), @@ -158,10 +160,10 @@ export async function statRemotePath( remotePath: string, opts: { signal?: AbortSignal; timeoutMs?: number } = {}, ): Promise { - await ensurePosixRemote(target); + const shell = await ensurePosixRemote(target); const p = quotePosixPath(remotePath); const command = `if [ -d ${p} ]; then echo directory; elif [ -f ${p} ]; then echo file; elif [ -e ${p} ]; then echo other; else echo missing; fi`; - const args = await buildRemoteCommand(target, command); + const args = await buildRemoteCommand(target, wrapInPosixShell(shell, command)); using child = ptree.spawn(["ssh", ...args], { signal: ptree.combineSignals(opts.signal, opts.timeoutMs ?? DEFAULT_TIMEOUT_MS), }); @@ -191,9 +193,9 @@ export async function listRemoteDir( remotePath: string, opts: { signal?: AbortSignal; timeoutMs?: number } = {}, ): Promise { - await ensurePosixRemote(target); + const shell = await ensurePosixRemote(target); const command = `LC_ALL=C ls -1Ap -- ${quotePosixPath(remotePath)}`; - const args = await buildRemoteCommand(target, command); + const args = await buildRemoteCommand(target, wrapInPosixShell(shell, command)); using child = ptree.spawn(["ssh", ...args], { signal: ptree.combineSignals(opts.signal, opts.timeoutMs ?? DEFAULT_TIMEOUT_MS), }); diff --git a/packages/coding-agent/src/ssh/ssh-executor.ts b/packages/coding-agent/src/ssh/ssh-executor.ts index dbe553cd6..a8b92842c 100644 --- a/packages/coding-agent/src/ssh/ssh-executor.ts +++ b/packages/coding-agent/src/ssh/ssh-executor.ts @@ -4,6 +4,7 @@ import { OutputSink } from "../session/streaming-output"; import { resolveOutputMaxColumns, resolveOutputSinkHeadBytes } from "../tools/output-meta"; import { buildRemoteCommand, ensureConnection, ensureHostInfo, type SSHConnectionTarget } from "./connection-manager"; import { hasSshfs, mountRemote } from "./sshfs-mount"; +import { wrapInPosixShell } from "./utils"; export interface SSHExecutorOptions { /** Timeout in milliseconds */ @@ -78,18 +79,6 @@ function createAbortWaiter( return { promise, cleanup: () => signal.removeEventListener("abort", onAbort) }; } -function quoteForCompatShell(command: string): string { - if (command.length === 0) { - return "''"; - } - const escaped = command.replace(/'/g, "'\\''"); - return `'${escaped}'`; -} - -function buildCompatCommand(shell: "bash" | "sh", command: string): string { - return `${shell} -c ${quoteForCompatShell(command)}`; -} - export async function executeSSH( host: SSHConnectionTarget, command: string, @@ -108,7 +97,7 @@ export async function executeSSH( if (options?.compatEnabled) { const info = await ensureHostInfo(host); if (info.compatShell) { - resolvedCommand = buildCompatCommand(info.compatShell, command); + resolvedCommand = wrapInPosixShell(info.compatShell, command); } else { logger.warn("SSH compat enabled without detected compat shell", { host: host.name }); } diff --git a/packages/coding-agent/src/ssh/utils.ts b/packages/coding-agent/src/ssh/utils.ts index d4d131ec4..72e3e7f1d 100644 --- a/packages/coding-agent/src/ssh/utils.ts +++ b/packages/coding-agent/src/ssh/utils.ts @@ -30,3 +30,22 @@ export function quotePosixPath(value: string): string { if (value.length === 0) return "''"; return `'${value.replace(/'/g, "'\\''")}'`; } + +/** + * Wrap a POSIX command in ` -c ''` so it runs under the + * named shell rather than whatever `$SHELL` happens to be on the remote. + * + * Used by the `ssh://` transfer helpers and the Windows compat dispatch: + * OpenSSH passes our snippets to ` -c`, so a remote whose + * login shell is fish/csh/tcsh (or cmd/powershell on Windows compat) + * can't parse `if [ … ]; then …`. Wrapping forces parsing under the + * shell OMP actually verified can run the snippet. + * + * `-c` (not `-lc`): the transfer snippets only call absolute POSIX + * builtins (`head`/`cat`/`mv`/`test`/`ls`/`mkdir`/`rm`/`dirname`) and + * don't need login-profile setup. Capability *probing* still uses + * `-lc` to mirror the user's real environment. + */ +export function wrapInPosixShell(shell: "sh" | "bash" | "zsh", command: string): string { + return `${shell} -c ${quotePosixPath(command)}`; +}