diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 48d24f363..69ec05ba2 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -67,7 +67,7 @@ - Fixed active goal runs that successfully call `yield` and then receive a trailing empty assistant `stop` skipping threshold compaction; post-yield empty-stop suppression now still anchors active-goal compaction on the yield-bearing assistant turn, so long-running tasks continue after maintenance instead of settling early. ### Added -- Added `ssh://host/path` support to `read`, `search`, and `write` for single text files on pre-configured SSH hosts (or `~/.ssh/config` aliases); UTF-8 text only, up to 1 MiB. Requires the same approval as the `ssh` tool, rejects argument-injecting hosts/usernames (leading `-`), validates the entire file as UTF-8, peels read selectors so `write` targets the same file `read` does, and replaces (rather than writes through) a symlinked write destination via a uniquely named remote temp. +- Added `ssh://host/path` support to `read`, `search`, and `write` for single text files on pre-configured SSH hosts (or `~/.ssh/config` aliases); UTF-8 text only, up to 1 MiB. Requires the same approval as the `ssh` tool, rejects argument-injecting hosts/usernames (leading `-`), validates the entire file as UTF-8, peels read selectors so `write` targets the same file `read` does, and replaces (rather than writes through) a symlinked write destination via a uniquely named remote temp. `read` also lists an `ssh://` directory one level deep (dotfiles included, directories first; `ssh://host/` lists the remote root), while `search` refuses an `ssh://` directory and `write` refuses to overwrite one. ## [16.1.19] - 2026-06-25 diff --git a/packages/coding-agent/src/internal-urls/__tests__/ssh-protocol.test.ts b/packages/coding-agent/src/internal-urls/__tests__/ssh-protocol.test.ts index 8d2abeb72..f324965c4 100644 --- a/packages/coding-agent/src/internal-urls/__tests__/ssh-protocol.test.ts +++ b/packages/coding-agent/src/internal-urls/__tests__/ssh-protocol.test.ts @@ -69,10 +69,15 @@ describe("SshProtocolHandler", () => { expect(spy.mock.calls[0]?.[0]).toMatchObject({ name: "bob@h1:2222", host: "h1", username: "bob", port: 2222 }); }); - it("rejects a host-only URL with no file path", async () => { + it("lists the remote root directory for ssh://host/", async () => { mockHosts(); - mockReadBytes("x"); - await expect(handler.resolve(parseInternalUrl("ssh://icaro/"))).rejects.toThrow(/absolute file path/); + vi.spyOn(fileTransfer, "readRemoteFile").mockRejectedValue(new Error("Is a directory")); + vi.spyOn(fileTransfer, "statRemotePath").mockResolvedValue("directory"); + const listSpy = vi.spyOn(fileTransfer, "listRemoteDir").mockResolvedValue([{ name: "etc", isDirectory: true }]); + const res = await handler.resolve(parseInternalUrl("ssh://icaro/")); + expect(res.isDirectory).toBe(true); + expect(res.content).toBe("etc/"); + expect(listSpy.mock.calls[0]?.[1]).toBe("/"); }); it("rejects a binary / non-UTF-8 file instead of returning a resource", async () => { @@ -111,4 +116,42 @@ describe("SshProtocolHandler", () => { expect(spy).toHaveBeenCalledTimes(1); expect(spy.mock.calls[0]?.[2]).toEqual(new TextEncoder().encode("hi\n\t!\n")); }); + + it("lists a remote directory when the path is not a readable file", async () => { + mockHosts(); + vi.spyOn(fileTransfer, "readRemoteFile").mockRejectedValue( + new Error("head: error reading '/etc': Is a directory"), + ); + vi.spyOn(fileTransfer, "statRemotePath").mockResolvedValue("directory"); + const listSpy = vi.spyOn(fileTransfer, "listRemoteDir").mockResolvedValue([ + { name: "conf.d", isDirectory: true }, + { name: "hosts", isDirectory: false }, + ]); + const res = await handler.resolve(parseInternalUrl("ssh://icaro/etc")); + expect(res.isDirectory).toBe(true); + expect(res.immutable).toBe(true); + expect(res.sourcePath).toBeUndefined(); + expect(res.content).toBe("conf.d/\nhosts"); + // read fail → stat → list must target the same remote path, not a peeled/normalized variant. + expect(listSpy.mock.calls[0]?.[1]).toBe("/etc"); + }); + + it("renders an empty remote directory", async () => { + mockHosts(); + vi.spyOn(fileTransfer, "readRemoteFile").mockRejectedValue(new Error("Is a directory")); + vi.spyOn(fileTransfer, "statRemotePath").mockResolvedValue("directory"); + vi.spyOn(fileTransfer, "listRemoteDir").mockResolvedValue([]); + const res = await handler.resolve(parseInternalUrl("ssh://icaro/empty")); + expect(res.content).toBe("(empty directory)"); + expect(res.isDirectory).toBe(true); + }); + + it("rethrows the original read error when the path is missing, not a directory", async () => { + mockHosts(); + vi.spyOn(fileTransfer, "readRemoteFile").mockRejectedValue( + new Error("head: cannot open '/nope': No such file or directory"), + ); + vi.spyOn(fileTransfer, "statRemotePath").mockResolvedValue("missing"); + await expect(handler.resolve(parseInternalUrl("ssh://icaro/nope"))).rejects.toThrow(/No such file or directory/); + }); }); diff --git a/packages/coding-agent/src/internal-urls/ssh-protocol.ts b/packages/coding-agent/src/internal-urls/ssh-protocol.ts index d15f88ef5..ab4bb5526 100644 --- a/packages/coding-agent/src/internal-urls/ssh-protocol.ts +++ b/packages/coding-agent/src/internal-urls/ssh-protocol.ts @@ -1,15 +1,16 @@ /** * Protocol handler for `ssh://host/path` URLs. * - * Resolves a single remote text file on a pre-configured SSH host — or any + * Resolves a remote text file or directory listing on a pre-configured SSH host — or any * destination OpenSSH can resolve itself (e.g. a `~/.ssh/config` alias) — for * the read, search, and write tools, reusing the shared ControlMaster * connection in `../ssh/connection-manager`. * - * Text only. Binary/non-UTF-8 files and files larger than 1 MiB are rejected - * with an explicit error rather than returned as a resource: this handler - * exposes no `sourcePath`, so a note-style resource would make `search` grep the - * note text and report "No matches found", hiding the real condition. + * A remote path resolves to a UTF-8 text file (≤ 1 MiB) or, when it is a + * directory, a one-level listing. Binary/non-UTF-8 or oversized files are + * rejected with an explicit error. This handler exposes no `sourcePath`; + * directory listings carry `isDirectory` so `search` refuses to grep the + * listing text instead of the directory's real contents. * * `loadCapability` is imported from `../capability` (not the `../discovery` * barrel) on purpose — pulling the barrel here would route @@ -21,7 +22,14 @@ import * as capability from "../capability"; import { type SSHHost, sshCapability } from "../capability/ssh"; import type { SSHConnectionTarget } from "../ssh/connection-manager"; -import { readRemoteFile, writeRemoteFile } from "../ssh/file-transfer"; +import { + listRemoteDir, + type RemoteDirEntry, + type RemotePathKind, + readRemoteFile, + statRemotePath, + writeRemoteFile, +} from "../ssh/file-transfer"; import type { InternalResource, InternalUrl, ProtocolHandler, ResolveContext, WriteContext } from "./types"; /** Largest remote text file `ssh://` will materialize (mirrors the local:// cap). */ @@ -62,8 +70,10 @@ function remotePathFromUrl(url: InternalUrl): string { } catch { throw new Error(`Invalid URL encoding in ssh:// path: ${url.href}`); } - if (!decoded || decoded === "/") { - throw new Error("ssh:// requires an absolute file path, e.g. ssh://host/etc/hosts"); + if (!decoded) { + throw new Error( + "ssh:// requires an absolute path, e.g. ssh://host/etc/hosts or ssh://host/ for the root directory", + ); } return decoded; } @@ -107,6 +117,12 @@ async function resolveTarget(url: InternalUrl, cwd?: string): Promise `${entry.name}${entry.isDirectory ? "/" : ""}`).join("\n"); +} + export class SshProtocolHandler implements ProtocolHandler { readonly scheme = "ssh"; readonly immutable = false; @@ -114,16 +130,31 @@ export class SshProtocolHandler implements ProtocolHandler { async resolve(url: InternalUrl, context?: ResolveContext): Promise { const target = await resolveTarget(url, context?.cwd); const remotePath = remotePathFromUrl(url); - const { bytes, truncated } = await readRemoteFile(target, remotePath, { - maxBytes: SSH_TEXT_MAX_BYTES, - signal: context?.signal, - }); - if (truncated) { + let fileResult: { bytes: Uint8Array; truncated: boolean }; + try { + fileResult = await readRemoteFile(target, remotePath, { + maxBytes: SSH_TEXT_MAX_BYTES, + signal: context?.signal, + }); + } catch (err) { + // `head` fails on a directory (and on a missing/unreadable path). Re-classify: + // only a real directory becomes a listing; anything else rethrows the original + // error, which carries the remote stderr (e.g. "No such file or directory"). + let kind: RemotePathKind | undefined; + try { + kind = await statRemotePath(target, remotePath, { signal: context?.signal }); + } catch { + // Re-stat failed too (host/connection issue) — the original read error is clearer. + } + if (kind === "directory") return this.#resolveDirectory(target, remotePath, url, context?.signal); + throw err; + } + if (fileResult.truncated) { throw new Error( `ssh://: ${remotePath} exceeds the 1 MiB limit; ssh:// supports text files up to 1 MiB — use an sshfs mount for larger files`, ); } - const content = decodeUtf8Text(bytes); + const content = decodeUtf8Text(fileResult.bytes); if (content === null) { throw new Error( `ssh://: ${remotePath} is a binary or non-UTF-8 file; ssh:// supports UTF-8 text only — use the ssh tool or an sshfs mount`, @@ -135,7 +166,25 @@ export class SshProtocolHandler implements ProtocolHandler { url: url.href, content, contentType: contentTypeFor(remotePath), - size: bytes.length, + size: fileResult.bytes.length, + }; + } + + /** Resolve a remote directory to a one-level listing (no `sourcePath`; `isDirectory` so search refuses it; immutable). */ + async #resolveDirectory( + target: SSHConnectionTarget, + remotePath: string, + url: InternalUrl, + signal?: AbortSignal, + ): Promise { + const content = formatDirListing(await listRemoteDir(target, remotePath, { signal })); + return { + url: url.href, + content, + contentType: "text/plain", + size: Buffer.byteLength(content, "utf-8"), + immutable: true, + isDirectory: true, }; } diff --git a/packages/coding-agent/src/internal-urls/types.ts b/packages/coding-agent/src/internal-urls/types.ts index bfdd76b0c..fe11a17b0 100644 --- a/packages/coding-agent/src/internal-urls/types.ts +++ b/packages/coding-agent/src/internal-urls/types.ts @@ -33,6 +33,13 @@ export interface InternalResource { * resources. Mutable resources (e.g. local://) behave like editable files. */ immutable?: boolean; + /** + * True when the resource is a directory listing rather than file content. + * `search` refuses to grep such a resource when it has no `sourcePath` — a + * remote `ssh://` listing has no local path to recurse, so its listing text + * must never be mistaken for the directory's contents. + */ + isDirectory?: boolean; } /** diff --git a/packages/coding-agent/src/prompts/tools/read.md b/packages/coding-agent/src/prompts/tools/read.md index 85013df59..6886e3a2f 100644 --- a/packages/coding-agent/src/prompts/tools/read.md +++ b/packages/coding-agent/src/prompts/tools/read.md @@ -69,7 +69,7 @@ For `.sqlite`, `.sqlite3`, `.db`, `.db3`: All URI schemes take the same line selectors. `artifact://` recovers full output a bash/eval/tool result spilled or truncated. `history://` = agent transcript; bare `history://` lists agents. -`ssh://host/` reads a single text file on a pre-configured SSH host or `~/.ssh/config` alias (UTF-8 text only, ≤1 MiB; no binary/recursive/glob). Also writable via `write` and searchable via `search`. A literal `:` in the remote path must be percent-encoded as `%3A`, since a trailing `:sel` is read as a line selector. +`ssh://host/` reads a remote text file (UTF-8, ≤1 MiB) or lists a directory one level deep, on a pre-configured SSH host or `~/.ssh/config` alias; `ssh://host/` lists the remote root. Files are also writable via `write` and searchable via `search`; a directory only lists (`search` refuses a directory, `write` refuses to overwrite one). A literal `:` in the remote path must be percent-encoded as `%3A`, since a trailing `:sel` is read as a line selector. - Line ranges go in the selector: `path="src/foo.ts:50-200"`. 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 new file mode 100644 index 000000000..cae09d50a --- /dev/null +++ b/packages/coding-agent/src/ssh/__tests__/file-transfer-posix-guard.test.ts @@ -0,0 +1,29 @@ +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"; + +describe("ssh file-transfer POSIX guard", () => { + afterEach(() => { + vi.restoreAllMocks(); + }); + + it("rejects a confirmed Windows remote before running any POSIX command", async () => { + // Stub BOTH the connection and the host-info probe so the guard is reached + // without opening a real SSH connection and before any command is spawned. + const ensureConnectionSpy = vi.spyOn(connectionManager, "ensureConnection").mockResolvedValue(undefined); + const ensureHostInfoSpy = vi.spyOn(connectionManager, "ensureHostInfo").mockResolvedValue({ + version: 2, + os: "windows", + shell: "powershell", + compatEnabled: false, + }); + const target: SSHConnectionTarget = { name: "winbox", host: "winbox" }; + await expect(readRemoteFile(target, "C:/x.txt", { maxBytes: 1024 })).rejects.toThrow(/Windows host/); + await expect(writeRemoteFile(target, "C:/x.txt", new Uint8Array([1]), {})).rejects.toThrow(/Windows host/); + // Prove the guard ran through the stubbed transport rather than failing early + // for an unrelated reason (e.g. a future import refactor bypassing the mocks). + expect(ensureConnectionSpy).toHaveBeenCalled(); + expect(ensureHostInfoSpy).toHaveBeenCalled(); + }); +}); diff --git a/packages/coding-agent/src/ssh/file-transfer.ts b/packages/coding-agent/src/ssh/file-transfer.ts index 855c5b4b1..9e662d7f2 100644 --- a/packages/coding-agent/src/ssh/file-transfer.ts +++ b/packages/coding-agent/src/ssh/file-transfer.ts @@ -7,12 +7,26 @@ * and final newlines are preserved. */ import { ptree } from "@oh-my-pi/pi-utils"; -import { buildRemoteCommand, ensureConnection, type SSHConnectionTarget } from "./connection-manager"; +import { buildRemoteCommand, ensureConnection, ensureHostInfo, type SSHConnectionTarget } from "./connection-manager"; import { quotePosixPath } 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 reject non-POSIX (Windows) remotes, + * which the POSIX `head`/`cat`/`mv`/`ls` commands in this module cannot drive. + */ +async function ensurePosixRemote(target: SSHConnectionTarget): Promise { + await ensureConnection(target); + const info = await ensureHostInfo(target); + if (info.os === "windows") { + throw new Error( + `ssh://: ${target.name} is a Windows host; ssh:// supports POSIX remotes only (head/cat/mv) — use the ssh tool for Windows hosts`, + ); + } +} + export interface RemoteFileReadOptions { /** Maximum bytes to materialize; the helper fetches one extra byte to detect truncation. */ maxBytes: number; @@ -44,7 +58,7 @@ export async function readRemoteFile( remotePath: string, opts: RemoteFileReadOptions, ): Promise { - await ensureConnection(target); + await ensurePosixRemote(target); const command = `head -c ${opts.maxBytes + 1} ${quotePosixPath(remotePath)}`; const args = await buildRemoteCommand(target, command); using child = ptree.spawn(["ssh", ...args], { @@ -58,12 +72,26 @@ export async function readRemoteFile( } /** - * Write `content` to a remote file byte-exact. Streams stdin into a uniquely - * named temp file in the destination directory, then atomically renames it into - * place so a partial transfer never clobbers the destination and concurrent - * writers cannot collide on the temp name. The rename REPLACES a symlink at the - * destination with a regular file rather than writing through it (resolving the - * link target is not portable across the macOS/Linux hosts this stack supports). + * Write `content` to a remote file byte-exact. Stdin is always staged first into + * a uniquely named temp in the destination directory (so the remote never blocks + * on an unread pipe and a dropped connection lands in the temp, never the + * destination). The destination then dictates the commit: + * - a directory — or a symlink to one, since the `-d` test follows links — is + * refused (a plain `mv tmp dir` would move the temp INTO it). + * - an existing non-symlink regular file is rewritten IN PLACE from the staged + * temp, preserving its inode and therefore its ordinary permission bits (a + * `0600` secret stays `0600` on overwrite), ACLs, xattrs, and hardlinks. The + * setuid/setgid bits may be cleared by the write (per POSIX). This commit is + * not fully atomic — a remote-side failure during the local temp->dest copy + * (e.g. the disk filling) can truncate the destination — but the slow network + * transfer has already landed in the temp, and the temp is removed on failure. + * It also needs write permission on the file itself (a read-only file is + * refused, not silently replaced). + * - an existing special file (FIFO/socket/device) is refused, not replaced. + * - anything else (a new path, a symlink to a non-directory, a dangling symlink) + * is committed with an atomic rename, which REPLACES a symlink with a regular + * file rather than writing through it (resolving the link target is not + * portable across the macOS/Linux hosts this stack supports). * Throws `ptree.NonZeroExitError` when the remote path is unwritable or the host * is unreachable. */ @@ -73,10 +101,27 @@ export async function writeRemoteFile( content: Uint8Array, opts: RemoteFileWriteOptions, ): Promise { - await ensureConnection(target); + await ensurePosixRemote(target); const dest = quotePosixPath(remotePath); const tmp = quotePosixPath(`${remotePath}.omp-tmp.${crypto.randomUUID()}`); - const command = `cat > ${tmp} && mv ${tmp} ${dest}`; + // Stage stdin into the temp first (so the remote never blocks on an unread + // pipe and a dropped connection lands in the temp, never the destination). + // An EXIT trap removes the staged temp on every exit path (staging failure, + // in-place success, refuse branches, or a failed rename). Commit by + // destination kind: a directory (or symlink to one; `-d` follows links) is + // refused; an existing non-symlink regular file is rewritten IN PLACE + // (preserving inode, permission bits, ACLs, xattrs, hardlinks; setuid/setgid + // may clear); an existing special file (FIFO/socket/device) is refused; + // anything else (a new path or a symlink to a non-directory) uses temp+rename, + // replacing such a symlink rather than writing through it. + const command = + `t=${tmp}; trap 'rm -f -- "$t"' 0; ` + + `cat > "$t" && { ` + + `if [ -d ${dest} ]; then echo 'ssh://: destination is a directory' >&2; exit 1; ` + + `elif [ -f ${dest} ] && [ ! -L ${dest} ]; then cat "$t" > ${dest} || exit 1; ` + + `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 }); using child = ptree.spawn(["ssh", ...args], { stdin: content, @@ -84,3 +129,68 @@ export async function writeRemoteFile( }); await child.exitedCleanly; } + +/** Classification of a remote path, used by the read handler's directory dispatch. */ +export type RemotePathKind = "file" | "directory" | "other" | "missing"; + +/** + * Classify a remote path with POSIX `test` (portable across Linux/BSD/macOS): + * `directory`, regular `file`, `other` (special file), or `missing`. + */ +export async function statRemotePath( + target: SSHConnectionTarget, + remotePath: string, + opts: { signal?: AbortSignal; timeoutMs?: number } = {}, +): Promise { + 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); + using child = ptree.spawn(["ssh", ...args], { + signal: ptree.combineSignals(opts.signal, opts.timeoutMs ?? DEFAULT_TIMEOUT_MS), + }); + const out = new TextDecoder().decode(await child.bytes()).trim(); + await child.exitedCleanly; + return out === "directory" || out === "file" || out === "other" ? out : "missing"; +} + +/** A single entry in a remote directory listing. */ +export interface RemoteDirEntry { + /** Entry name (no path component), trailing `/` stripped. */ + name: string; + /** True when the entry is a directory. */ + isDirectory: boolean; +} + +/** + * List a remote directory one level deep with `ls -1Ap` (one per line; all + * entries incl. dotfiles but not `.`/`..`; trailing `/` marks directories). + * Plain `ls` (no `| head`) so a permission/race failure surfaces as a non-zero + * exit instead of being masked as an empty listing. Entries are returned in + * full, sorted directories-first then by name to mirror the local + * directory-resource contract, so the read tool can paginate the listing. + */ +export async function listRemoteDir( + target: SSHConnectionTarget, + remotePath: string, + opts: { signal?: AbortSignal; timeoutMs?: number } = {}, +): Promise { + await ensurePosixRemote(target); + const command = `LC_ALL=C ls -1Ap -- ${quotePosixPath(remotePath)}`; + const args = await buildRemoteCommand(target, command); + using child = ptree.spawn(["ssh", ...args], { + signal: ptree.combineSignals(opts.signal, opts.timeoutMs ?? DEFAULT_TIMEOUT_MS), + }); + const text = new TextDecoder().decode(await child.bytes()); + await child.exitedCleanly; + const entries = text + .split("\n") + .filter(line => line.length > 0) + .map(line => { + const isDirectory = line.endsWith("/"); + return { name: isDirectory ? line.slice(0, -1) : line, isDirectory }; + }); + // JS sort is the order contract (mirrors buildDirectoryResource): dirs first, then by name. + entries.sort((a, b) => Number(b.isDirectory) - Number(a.isDirectory) || a.name.localeCompare(b.name)); + return entries; +} diff --git a/packages/coding-agent/src/tools/search.ts b/packages/coding-agent/src/tools/search.ts index 3a057a6f6..fd8e55ae7 100644 --- a/packages/coding-agent/src/tools/search.ts +++ b/packages/coding-agent/src/tools/search.ts @@ -123,6 +123,27 @@ interface SearchPathSpec { ranges?: [LineRange, ...LineRange[]]; } +/** + * Mirror of read's `parseSel` selector grammar (`read.ts`) so `search` accepts + * exactly the internal-URL selectors `read`/`write` do: a single chunk that is a + * line range, `raw`, or `conflicts`; or a two-chunk compound of exactly one `raw` + * plus one line range. Everything else (`:-10`, `:1-1:1-2`, `:conflicts:1-1`, + * `:raw:conflicts`) is rejected. Keep in sync with read.parseSel. + */ +function isReadSelectorGrammar(sel: string): boolean { + if (sel.includes(":")) { + const chunks = sel.split(":"); + if (chunks.length !== 2) return false; + const [a, b] = chunks as [string, string]; + const aIsRaw = a.toLowerCase() === "raw"; + const bIsRaw = b.toLowerCase() === "raw"; + const rangeChunk = aIsRaw ? b : bIsRaw ? a : null; + return rangeChunk !== null && parseLineRanges(rangeChunk) !== null; + } + const lower = sel.toLowerCase(); + return lower === "raw" || lower === "conflicts" || parseLineRanges(sel) !== null; +} + function parsePathSpecs(rawEntries: readonly string[]): SearchPathSpec[] { const specs: SearchPathSpec[] = []; for (const entry of rawEntries) { @@ -134,11 +155,14 @@ function parsePathSpecs(rawEntries: readonly string[]): SearchPathSpec[] { // still honor any embedded line range as a match filter. const internalSplit = splitInternalUrlSel(entry); if (internalSplit.sel !== undefined) { - specs.push({ - original: entry, - clean: internalSplit.path, - ranges: selectorLineRanges(internalSplit.sel), - }); + // Reject selectors read's parseSel would reject (`:-10`, `:1-1:1-2`, + // `:conflicts:1-1`) instead of silently widening the search or dropping a chunk. + if (!isReadSelectorGrammar(internalSplit.sel)) { + throw new ToolError( + `paths entry "${entry}" has an invalid selector ":${internalSplit.sel}" — use ":N-M" line ranges, ":raw"/":conflicts", a range plus ":raw", or percent-encode a literal ":" as %3A`, + ); + } + specs.push({ original: entry, clean: internalSplit.path, ranges: selectorLineRanges(internalSplit.sel) }); continue; } const split = splitPathAndSel(entry); @@ -162,6 +186,43 @@ function parsePathSpecs(rawEntries: readonly string[]): SearchPathSpec[] { return specs; } +/** + * Validate a search pattern against the native RE2 dialect by probing a scratch + * byte with `grep`. Used before a pure-virtual search (where no native grep ran + * to validate it), so an RE2-unsupported pattern (lookbehind, backreference) is + * rejected consistently instead of silently matching via JS `RegExp`. Throws the + * native `regex parse error`, which the caller normalizes to a `ToolError`. + */ +async function probeRegexDialect( + pattern: string, + ignoreCase: boolean, + multiline: boolean, + signal?: AbortSignal, +): Promise { + const dir = await mkdtemp(path.join(tmpdir(), "omp-search-probe-")); + try { + const probeFile = path.join(dir, "probe"); + await writeFile(probeFile, "\n"); + await grep( + { + pattern, + path: probeFile, + ignoreCase, + multiline, + hidden: true, + gitignore: false, + maxCount: 1, + mode: GrepOutputMode.Content, + signal, + timeoutMs: SEARCH_GREP_TIMEOUT_MS, + }, + undefined, + ); + } finally { + await rm(dir, { recursive: true, force: true }).catch(() => {}); + } +} + function mergeRangesInto(map: Map, absKey: string, ranges: readonly LineRange[]): void { // Concat-without-merge is correct: `isLineInRanges` scans linearly, so // duplicates/overlaps only cost a few extra comparisons per match. @@ -612,6 +673,14 @@ async function resolveInternalSearchInputs(opts: { throw new ToolError(`Glob patterns are not supported for internal URLs: ${rawPath}`); } const resource = await internalRouter.resolve(rawPath, context); + // A directory listing with no backing local path (e.g. a remote ssh:// dir) + // has no real contents to grep — searching its listing text would be + // misleading. Local/skill/vault dir resources set `sourcePath` and skip this. + if (resource.isDirectory && !resource.sourcePath) { + throw new ToolError( + `search cannot recurse the directory listing at ${rawPath}; search a specific file under it (e.g. ${rawPath.replace(/\/+$/, "")}/) or read ${rawPath} to list its entries`, + ); + } if (resource.sourcePath) { paths[idx] = resource.sourcePath; if (resource.immutable) { @@ -965,15 +1034,33 @@ export class SearchTool implements AgentTool 0) { + await probeRegexDialect(normalizedPattern, ignoreCase, effectiveMultiline, signal); + } + virtualResult = searchVirtualResources( + virtualResources, + normalizedPattern, + ignoreCase, + effectiveMultiline, + normalizedContextBefore, + normalizedContextAfter, + INTERNAL_TOTAL_CAP, + ); + } catch (err) { + if (err instanceof Error && /^regex(?: parse)? error/i.test(err.message)) { + throw new ToolError(err.message.replace(/^regex(?: parse)? error:?\s*/i, "Invalid regex: ")); + } + if (err instanceof SyntaxError) { + throw new ToolError(`Invalid regex: ${err.message}`); + } + throw err; + } result = mergeGrepResults(result, virtualResult, INTERNAL_TOTAL_CAP); if (rangesByAbsPath.size > 0) { const filteredMatches: GrepMatch[] = []; diff --git a/packages/coding-agent/test/tools/search-internal-urls.test.ts b/packages/coding-agent/test/tools/search-internal-urls.test.ts index 957ca1999..975dd6a8f 100644 --- a/packages/coding-agent/test/tools/search-internal-urls.test.ts +++ b/packages/coding-agent/test/tools/search-internal-urls.test.ts @@ -244,6 +244,36 @@ describe("SearchTool internal URL resolution", () => { expect(text).not.toContain("needle outside range"); }); + it("rejects a malformed selector on a selector-capable internal URL instead of widening the search", async () => { + const session = createSession(); + const tool = new SearchTool(session); + await expect(tool.execute("bad-sel", { pattern: "needle", paths: ["artifact://5:-10"] })).rejects.toThrow( + /invalid selector/i, + ); + await expect(tool.execute("bad-mixed", { pattern: "needle", paths: ["artifact://5:1-1:-10"] })).rejects.toThrow( + /invalid selector/i, + ); + // Multi-range colon compounds are rejected by read's parseSel; search must match. + await expect(tool.execute("bad-multi", { pattern: "needle", paths: ["artifact://5:1-1:1-2"] })).rejects.toThrow( + /invalid selector/i, + ); + // A `conflicts` display chunk is not valid in a range compound (only `raw` is). + await expect( + tool.execute("bad-conflicts", { pattern: "needle", paths: ["artifact://5:conflicts:1-1"] }), + ).rejects.toThrow(/invalid selector/i); + }); + + it("rejects an RE2-unsupported pattern on a pure-virtual search (dialect parity)", async () => { + registerVirtualDocs(new Map([["doc.md", "alpha line\nbeta line\n"]])); + const session = createSession(); + const tool = new SearchTool(session); + // Lookbehind is valid JS RegExp but unsupported by the native RE2 dialect; + // the pure-virtual probe must reject it consistently with native search. + await expect(tool.execute("re2", { pattern: "(?<=alpha)line", paths: ["virtual://doc.md"] })).rejects.toThrow( + /Invalid regex/i, + ); + }); + it("expands omp:// root to grep embedded documentation files", async () => { const session = createSession(); const tool = new SearchTool(session); @@ -466,4 +496,20 @@ describe("SearchTool internal URL resolution", () => { expect(text).toContain("2 files total"); expect(text).not.toContain("No matches found"); }); + + it("refuses to search a directory listing that has no backing local path", async () => { + // A directory resource with no sourcePath (e.g. a remote ssh:// listing) must + // not be virtual-grepped — its listing text is not the directory's contents. + InternalUrlRouter.instance().register({ + scheme: "dirstub", + immutable: true, + async resolve(url: InternalUrl): Promise { + return { url: url.href, content: "sub/\nfile.txt", contentType: "text/plain", isDirectory: true }; + }, + }); + const tool = new SearchTool(createSession()); + await expect(tool.execute("dir-search", { pattern: "x", paths: ["dirstub://host/dir"] })).rejects.toThrow( + /directory listing|cannot recurse/, + ); + }); }); diff --git a/packages/coding-agent/test/tools/ssh-url-localhost-e2e.test.ts b/packages/coding-agent/test/tools/ssh-url-localhost-e2e.test.ts index dc104d2b2..9f9036d61 100644 --- a/packages/coding-agent/test/tools/ssh-url-localhost-e2e.test.ts +++ b/packages/coding-agent/test/tools/ssh-url-localhost-e2e.test.ts @@ -1,9 +1,16 @@ import { afterAll, afterEach, beforeAll, describe, expect, it, vi } from "bun:test"; +import * as os from "node:os"; import * as capability from "@oh-my-pi/pi-coding-agent/capability"; import type { SSHHost } from "@oh-my-pi/pi-coding-agent/capability/ssh"; import type { CapabilityResult } from "@oh-my-pi/pi-coding-agent/capability/types"; +import { Settings } from "@oh-my-pi/pi-coding-agent/config/settings"; import { parseInternalUrl } from "@oh-my-pi/pi-coding-agent/internal-urls/parse"; +import { InternalUrlRouter } from "@oh-my-pi/pi-coding-agent/internal-urls/router"; import { SshProtocolHandler } from "@oh-my-pi/pi-coding-agent/internal-urls/ssh-protocol"; +import type { ToolSession } from "@oh-my-pi/pi-coding-agent/tools"; +import { ReadTool } from "@oh-my-pi/pi-coding-agent/tools/read"; +import { SearchTool } from "@oh-my-pi/pi-coding-agent/tools/search"; +import { WriteTool } from "@oh-my-pi/pi-coding-agent/tools/write"; // Live integration against `ssh localhost`. Skips automatically where key-based // localhost SSH is unavailable (CI without sshd). Capability lookup is mocked @@ -94,4 +101,139 @@ describe.skipIf(!SSH_OK)("ssh:// handler against a real localhost ssh", () => { const back = await handler.resolve(parseInternalUrl(`ssh://localhost${TMP}/sym-link`)); expect(back.content).toBe("replaced\n"); }); + + it("lists a real remote directory (dirs first, dotfiles included, no sourcePath)", async () => { + mockEmptyHosts(); + await sh(`mkdir -p ${TMP}/listdir/sub && printf x > ${TMP}/listdir/a.txt && printf y > ${TMP}/listdir/.hidden`); + const res = await handler.resolve(parseInternalUrl(`ssh://localhost${TMP}/listdir`)); + expect(res.isDirectory).toBe(true); + expect(res.immutable).toBe(true); // listings are never editable + expect(res.contentType).toBe("text/plain"); + expect(res.sourcePath).toBeUndefined(); + const lines = res.content.split("\n"); + expect(lines[0]).toBe("sub/"); // directories sort first + expect(lines).toContain(".hidden"); // dotfiles included via ls -A + expect(lines).toContain("a.txt"); + expect(lines).toHaveLength(3); + }); + + it("refuses to write to a directory and cleans up its temp", async () => { + mockEmptyHosts(); + await sh(`mkdir -p ${TMP}/wdir`); + await expect(handler.write(parseInternalUrl(`ssh://localhost${TMP}/wdir`), "x")).rejects.toThrow(); + const kind = await Bun.$`ssh -o BatchMode=yes localhost test -d ${TMP}/wdir && echo dir || echo notdir`.text(); + expect(kind.trim()).toBe("dir"); // directory intact, not clobbered into a file + // The dir-error path must remove the temp it created beside the destination. + const leftovers = + await Bun.$`ssh -o BatchMode=yes localhost ls -A ${TMP} | grep -c "wdir.omp-tmp" || true`.text(); + expect(leftovers.trim()).toBe("0"); + }); + + // GNU `stat -c` is Linux-only; `ssh localhost` targets this same machine, so + // gate on the local platform. Defends finding 2: overwriting an existing + // regular file preserves its ordinary permission bits and inode (in place), + // rather than resetting them via an inode-replacing temp+rename. + it.skipIf(process.platform !== "linux")( + "overwrites an existing regular file in place, preserving its mode and inode", + async () => { + mockEmptyHosts(); + const dest = `${TMP}/perm.txt`; + await sh(`printf orig > ${dest}; chmod 600 ${dest}`); + const inode = async () => (await Bun.$`ssh -o BatchMode=yes localhost stat -c %i ${dest}`.text()).trim(); + const inodeBefore = await inode(); + await handler.write(parseInternalUrl(`ssh://localhost${dest}`), "new\n"); + const mode = (await Bun.$`ssh -o BatchMode=yes localhost stat -c %a ${dest}`.text()).trim(); + expect(mode).toBe("600"); + expect(await inode()).toBe(inodeBefore); + const back = await handler.resolve(parseInternalUrl(`ssh://localhost${dest}`)); + expect(back.content).toBe("new\n"); + }, + ); +}); + +describe.skipIf(!SSH_OK)("ssh:// through the real read/search/write tools (localhost)", () => { + const TMP = `/tmp/omp-ssh-tools-e2e-${process.pid}`; + + function createSession(): ToolSession { + return { + cwd: os.tmpdir(), + hasUI: false, + getSessionFile: () => null, + getSessionSpawns: () => "*", + settings: Settings.isolated({ "search.contextBefore": 0, "search.contextAfter": 0 }), + }; + } + + function textOf(result: { content: Array<{ type: string; text?: string }> }): string { + return result.content + .filter(c => c.type === "text") + .map(c => c.text ?? "") + .join("\n"); + } + + beforeAll(async () => { + // Register built-in protocol handlers (incl. ssh) so the tools resolve ssh:// through the router. + InternalUrlRouter.resetForTests(); + await sh( + `mkdir -p ${TMP}; printf 'alpha\\n\\tbeta\\ngamma\\n' > ${TMP}/read.txt; awk 'BEGIN{print "ALPHALINE"; for(i=2;i<=9;i++)print "filler"i; print "OMEGALINE"}' > ${TMP}/range.txt`, + ); + }); + + afterAll(async () => { + await Bun.$`ssh -o BatchMode=yes localhost rm -rf ${TMP}`.nothrow().quiet(); + }); + + afterEach(() => { + vi.restoreAllMocks(); + }); + + it("ReadTool reads a remote file full and by range", async () => { + mockEmptyHosts(); + const tool = new ReadTool(createSession()); + const full = textOf(await tool.execute("r-full", { path: `ssh://localhost${TMP}/read.txt` })); + expect(full).toContain("alpha"); + expect(full).toContain("beta"); + expect(full).toContain("gamma"); + // `:1-1` plus the 3-line trailing context window covers lines 1-4; the + // line-10 OMEGALINE marker proves the bounded range actually sliced. + const range = textOf(await tool.execute("r-range", { path: `ssh://localhost${TMP}/range.txt:1-1` })); + expect(range).toContain("ALPHALINE"); + expect(range).not.toContain("OMEGALINE"); + }); + + it("SearchTool reports matches under the ssh:// URL with no scratch-temp leak", async () => { + mockEmptyHosts(); + const tool = new SearchTool(createSession()); + const result = await tool.execute("s", { pattern: "beta", paths: [`ssh://localhost${TMP}/read.txt`] }); + const out = textOf(result); + expect(out).toContain("beta"); + // The resource is reported under its ssh:// URL, not a local scratch path. + expect(result.details?.files).toContain(`ssh://localhost${TMP}/read.txt`); + // The pure-virtual RE2 probe's scratch dir must never leak into text or metadata. + const detailsJson = JSON.stringify(result.details ?? {}); + expect(out).not.toContain("omp-search-probe"); + expect(detailsJson).not.toContain("omp-search-probe"); + }); + + it("WriteTool round-trips a remote file byte-exact", async () => { + mockEmptyHosts(); + const dest = `ssh://localhost${TMP}/wtool.txt`; + await new WriteTool(createSession()).execute("w", { path: dest, content: "hi\n\t!\n" }); + const onDisk = await Bun.$`ssh -o BatchMode=yes localhost cat ${TMP}/wtool.txt`.text(); + expect(onDisk).toBe("hi\n\t!\n"); + const back = textOf(await new ReadTool(createSession()).execute("rb", { path: dest })); + expect(back).toContain("hi"); + }); + + it("WriteTool refuses to overwrite a remote special file (FIFO) and leaves it intact", async () => { + mockEmptyHosts(); + await sh(`rm -f ${TMP}/fifo; mkfifo ${TMP}/fifo`); + await expect( + new WriteTool(createSession()).execute("wf", { path: `ssh://localhost${TMP}/fifo`, content: "x" }), + ).rejects.toThrow(/special file/i); + const kind = ( + await Bun.$`ssh -o BatchMode=yes localhost test -p ${TMP}/fifo && echo fifo || echo other`.text() + ).trim(); + expect(kind).toBe("fifo"); + }); });