From e7fd52f8a880678a4d96de11510de098c3918623 Mon Sep 17 00:00:00 2001 From: Joel Moss Date: Thu, 24 Sep 2026 20:46:34 +0100 Subject: [PATCH 1/7] feat(macapp): reach a remote agent through a HostDriver's ssh stream The first half of #229: the app's services reach a remote wr-agent through `ssh host wr-agent relay`, with the same connection, manager and router as a local agent. - HostDriver: create, deriveFromBase, destroy, openStream, plus the declared traits. Every driver must authenticate its peer to shell grade. - ContainerHostDriver: the first driver and the test fixture. openStream runs ssh with its own -F config and an empty environment: BatchMode, the pinned host key as the only known key, no agent or port forwarding, and ServerAliveInterval to bound a dead link. - HostStream: a socketpair whose other end is ssh's stdin and stdout. The connection owns it and ends ssh when it closes, and ssh exiting is the connection ending. A handshake that never completes is serviceUnavailable with what ssh or the relay said, not connectionLost. - AgentVCSConnection.connect splits into socket and stream paths. SO_NOSIGPIPE is now set when the socket is created: after the split's async hop, setsockopt failed on a peer that had already hung up, and the hello's send raised SIGPIPE. - No Mac environment reaches a remote exec request: it carries only policy pins and host_environment, and the agent builds the child's environment from its own, never setting GIT_SSH_COMMAND over a repository's core.sshCommand. - Writes to a remote host are refused by name until they have host-side failure probes and an agent-held jj barrier. - run.sh runs any command against the fixture, which now starts its agent with env -i. The design doc records the host-key policy, auth resolution and the shell-grade requirement. Co-Authored-By: Claude Opus 5.5 --- docs/designs/remote-workrooms.md | 39 +++- .../WorkroomApp/Core/AgentCommandRunner.swift | 38 ++- .../WorkroomApp/Core/AgentVCSConnection.swift | 116 +++++++--- .../Core/ContainerHostDriver.swift | 134 +++++++++++ macapp/WorkroomApp/Core/HostDriver.swift | 194 ++++++++++++++++ .../Core/StatusCommandRunner.swift | 7 + .../AgentVCSProtocolTests.swift | 27 +++ macapp/WorkroomAppTests/HostStreamTests.swift | 133 +++++++++++ .../RemoteHostIntegrationTests.swift | 216 ++++++++++++++++++ vcs/crates/wr-agent/src/file.rs | 2 +- vcs/crates/wr-agent/src/vcs.rs | 100 ++++++++ vcs/scripts/ssh-fixture/entrypoint.sh | 5 +- vcs/scripts/ssh-fixture/run.sh | 26 ++- 13 files changed, 994 insertions(+), 43 deletions(-) create mode 100644 macapp/WorkroomApp/Core/ContainerHostDriver.swift create mode 100644 macapp/WorkroomApp/Core/HostDriver.swift create mode 100644 macapp/WorkroomAppTests/HostStreamTests.swift create mode 100644 macapp/WorkroomAppTests/RemoteHostIntegrationTests.swift diff --git a/docs/designs/remote-workrooms.md b/docs/designs/remote-workrooms.md index d979de7ae..53f01a08e 100644 --- a/docs/designs/remote-workrooms.md +++ b/docs/designs/remote-workrooms.md @@ -11,8 +11,9 @@ Mode: Builder **Phase 2 is merged to master** (2026-09-23, #201 through #218; review follow-ups #220–#226), and **Phase 3 has started.** Its milestones are sub-issues of #154: the Linux agent artifact (#227, merged; its first Nightly DMG is still to be checked), the supervised far-side agent with its stdio -relay and container fixture (#228, in progress), the -app-side transport with `HostDriver` and the container driver (#229), `execve` hand-off (#230), +relay and container fixture (#228, merged), the +app-side transport with `HostDriver` and the container driver (#229, in progress: services first, +terminal panes second), `execve` hand-off (#230), push-on-first-connect bootstrap (#231) and stop-and-reboot screen restoration (#232). The rest of this section is the 2026-09-17 status, kept for the Phase 2 detail it records and corrected where it had gone stale. @@ -993,6 +994,14 @@ these are the subsystems that actually gate "a remote workroom is a real workroo not die with it: their jobs run on, orphaned, while the replacement agent reports no sessions. systemd's default `KillMode=control-group` does this. The fixture's loop does not, which is fine for a test box and wrong for a host with real work on it. + **App side, services built (#229):** `HostDriver` (`Core/HostDriver.swift`) has the four methods + and the declared traits, and `ContainerHostDriver` implements `openStream` as + `ssh -F wr-agent relay --socket `. The stream is one end of a + socketpair whose other end is ssh's stdin and stdout, and `AgentVCSConnection.connect(host:stream:)` + runs the same negotiation over it as over a local socket, so VCS, File, Status and Forward need + no change. ssh exiting is the connection ending, through the same generation and + fail-pending-calls rules as a dead local agent. Terminal panes through the driver are the second + half of #229. - **Local needs no supervisor at all. DECIDED — and an earlier claim in this document was wrong.** A previous revision asserted that `SessionDaemon.swift:97-107`'s self-exit (`sessions.isEmpty && connections.isEmpty`) had to go, because the agent would also own VCS reads @@ -1023,8 +1032,30 @@ these are the subsystems that actually gate "a remote workroom is a real workroo product uses everywhere, the container fixture exercises the real path rather than a fallback nobody runs. What remains uncovered by CI is boxd's *live-process* derivation specifically — a narrower gap than the whole fork path, and one a nightly boxd job can cover if it earns it. -- **State a host-key policy** (see Provider Decision). Fresh-key-per-fork plus `BatchMode=yes` cannot prompt, so - the driver has to deliver the expected host key out of band or pin the base's. +- **Host-key policy: pinned and delivered out of band. DECIDED (#229).** The driver writes its own + `ssh_config` and passes it with `-F`, so nothing in `~/.ssh/config` or `/etc/ssh/ssh_config` + applies. The host's key is the only known key (`GlobalKnownHostsFile /dev/null`), with + `StrictHostKeyChecking yes` and `BatchMode yes`: a mismatched key fails at once with "Host key + verification failed." and there is no prompt to hang on. A fresh-key-per-fork driver delivers each + instance's key the same way, from its provider's API. +- **Auth resolution for the ssh transport: host-resolved credentials, no forwarding. DECIDED + (#229).** Nothing from the Mac's environment crosses. A remote exec request carries only policy + pins (`GIT_OPTIONAL_LOCKS`, `GIT_TERMINAL_PROMPT`, `LC_ALL`), and the agent builds the rest from + its own environment (`host_environment` in `vcs.rs`), less variables that redirect git to + another repository. That makes the supervisor's environment what every remote git gets, so a + supervisor should start the agent from a clean one. ssh is kept from prompting without setting + `GIT_SSH_COMMAND`, which would outrank a repository's own `core.sshCommand`: no askpass helper, + and a supervised agent has no terminal. ssh itself runs with an empty environment, + `ForwardAgent no` and `ClearAllForwardings yes`. git on the host authenticates with what the host + has, which in production is a per-workroom deploy key minted on the box (Decisions Made; Phase 4). + Agent forwarding stays rejected (premise 6). The container driver also logs in with a key file + and `IdentityAgent none`; whether a real driver may use the Mac's ssh agent to LOG IN, which is + not forwarding it, is that driver's decision. +- **Peer authentication to shell grade, on every driver. DECIDED (#229).** The exec service runs + `git` with arbitrary arguments, which is arbitrary code execution, so a stream that reaches an + agent is a shell whatever it is called. ssh authenticates to that grade. Every later driver (SDK + exec, WebSocket) must show that it does too before it counts as a driver; `HostDriver`'s doc + carries the requirement. - **Agent bootstrap over the transport.** Workroom.app ships the Linux agent binary and pushes it on first connect rather than baking provider images. One source of truth, no image maintenance, and it works for SDK-transport providers with no image concept. (This is the one piece of Coder's and diff --git a/macapp/WorkroomApp/Core/AgentCommandRunner.swift b/macapp/WorkroomApp/Core/AgentCommandRunner.swift index 645154730..79d9a4bd1 100644 --- a/macapp/WorkroomApp/Core/AgentCommandRunner.swift +++ b/macapp/WorkroomApp/Core/AgentCommandRunner.swift @@ -55,11 +55,9 @@ struct AgentCommandRunner: StatusCommandRunning, Sendable { } payload = encoded } - let request = AgentExecRequest( - executable: executable, args: args, dir: directory, - timeoutMs: Int((timeout * 1000).rounded(.up)), - stdin: payload, - env: StatusCommandRunner.childEnvironment(network: network)) + let request = Self.request( + executable, args, in: directory, timeout: timeout, stdin: payload, network: network, + host: connection.host) let reply: Data do { // Slack above the command's own timeout: the round trip and the agent's own bookkeeping @@ -163,6 +161,33 @@ struct AgentCommandRunner: StatusCommandRunning, Sendable { } } +extension AgentCommandRunner { + /// The exec request for `host`. **Nothing from the Mac's environment goes to a remote host.** + /// + /// Locally the request carries the app's whole child environment, which the agent adopts in place + /// of its own stale one (see `StatusCommandRunner.childEnvironment`). A remote host is a different + /// machine: the Mac's `HOME` and `PATH` name nothing there, and its `SSH_AUTH_SOCK`, git identity + /// and credential variables are the user's Mac credentials, which must not cross (design doc, + /// premise 6: agent forwarding was rejected, and git on a remote host authenticates with what the + /// host has). So a remote request sends only `remoteEnvironment`'s policy pins and asks the agent + /// to build the rest from the host's own environment (`host_environment` in `vcs.rs`), which also + /// pins ssh to fail rather than prompt, as `networkEnvironment` does here. + static func request( + _ executable: String, _ args: [String], in directory: String, timeout: TimeInterval, + stdin: String?, network: Bool, host: HostID + ) -> AgentExecRequest { + let remote = host != .local + return AgentExecRequest( + executable: executable, args: args, dir: directory, + timeoutMs: Int((timeout * 1000).rounded(.up)), + stdin: stdin, + env: remote + ? StatusCommandRunner.remoteEnvironment + : StatusCommandRunner.childEnvironment(network: network), + hostEnvironment: remote ? true : nil) + } +} + struct AgentExecRequest: Encodable, Sendable { var version = 1 var kind = "exec" @@ -172,6 +197,9 @@ struct AgentExecRequest: Encodable, Sendable { let timeoutMs: Int var stdin: String? var env: [String: String] + /// Set only for a remote host, and omitted otherwise: an agent that predates the field rejects + /// any request carrying it (`deny_unknown_fields`), and a local agent may be one. + var hostEnvironment: Bool? } struct AgentExecResult: Decodable, Sendable { diff --git a/macapp/WorkroomApp/Core/AgentVCSConnection.swift b/macapp/WorkroomApp/Core/AgentVCSConnection.swift index 1ea161c76..5184aa319 100644 --- a/macapp/WorkroomApp/Core/AgentVCSConnection.swift +++ b/macapp/WorkroomApp/Core/AgentVCSConnection.swift @@ -90,11 +90,15 @@ final class AgentVCSConnection: HostServiceConnection, @unchecked Sendable { /// per box, so there is nothing to key subscriptions by. let ceilingPrompts: AsyncStream private let ceilingPrompt: AsyncStream.Continuation + /// The local process carrying a driver's stream (`ssh host wr-agent relay`), or nil for a local + /// agent's socket. Ended when the connection fails, so a closed connection leaves no ssh behind. + private let carrier: HostStream? - private init(host: HostID, descriptor: Int32, helloVersion: UInt16) { + private init(host: HostID, descriptor: Int32, helloVersion: UInt16, carrier: HostStream?) { self.host = host self.descriptor = descriptor self.helloVersion = helloVersion + self.carrier = carrier (disconnection, disconnected) = AsyncStream.makeStream() // Newest-only: a prompt the app never got round to reading is superseded by the next one, and // the deadline in a stale one has passed by definition. @@ -105,32 +109,77 @@ final class AgentVCSConnection: HostServiceConnection, @unchecked Sendable { deinit { Darwin.close(descriptor) } static func connect(host: HostID, socketPath: String) async throws -> AgentVCSConnection { - let connection = try await runBlocking { + let fd = try await runBlocking { let fd = socket(AF_UNIX, SOCK_STREAM, 0) guard fd >= 0 else { throw HostConnectionError.connectionLost } - do { - var address = sockaddr_un() - address.sun_family = sa_family_t(AF_UNIX) - let path = Array(socketPath.utf8) - guard !path.contains(0), path.count < MemoryLayout.size(ofValue: address.sun_path) else { - throw VCSError.io("Agent socket path is invalid.") - } - withUnsafeMutableBytes(of: &address.sun_path) { $0.copyBytes(from: path) } - let connected = withUnsafePointer(to: &address) { - $0.withMemoryRebound(to: sockaddr.self, capacity: 1) { - Darwin.connect(fd, $0, socklen_t(MemoryLayout.size)) - } + noSignalOnWrite(fd) + var address = sockaddr_un() + address.sun_family = sa_family_t(AF_UNIX) + let path = Array(socketPath.utf8) + guard !path.contains(0), path.count < MemoryLayout.size(ofValue: address.sun_path) else { + Darwin.close(fd) + throw VCSError.io("Agent socket path is invalid.") + } + withUnsafeMutableBytes(of: &address.sun_path) { $0.copyBytes(from: path) } + let connected = withUnsafePointer(to: &address) { + $0.withMemoryRebound(to: sockaddr.self, capacity: 1) { + Darwin.connect(fd, $0, socklen_t(MemoryLayout.size)) } - guard connected == 0 else { throw HostConnectionError.connectionLost } - var noSignal: Int32 = 1 - setsockopt(fd, SOL_SOCKET, SO_NOSIGPIPE, &noSignal, socklen_t(MemoryLayout.size)) - var timeout = timeval(tv_sec: 2, tv_usec: 0) + } + guard connected == 0 else { + Darwin.close(fd) + throw HostConnectionError.connectionLost + } + return fd + } + return try await negotiate(host: host, descriptor: fd, carrier: nil, handshakeTimeout: 2) + } + + /// Over a driver's stream (`HostDriver.openStream`), which this connection now owns: its + /// descriptor is closed and its process ended with the connection, including when this throws. + /// + /// A handshake the stream never answered is `serviceUnavailable` with what the carrier said, not + /// `connectionLost`. There is nothing to respawn on a remote host, and "Host key verification + /// failed." or the relay's "no agent listening" is the whole diagnosis. + static func connect(host: HostID, stream: HostStream) async throws -> AgentVCSConnection { + do { + return try await negotiate( + host: host, descriptor: stream.handOff(), carrier: stream, + handshakeTimeout: stream.handshakeTimeout) + } catch HostConnectionError.connectionLost { + throw HostConnectionError.serviceUnavailable(await stream.failure()) + } catch AgentControlClient.AgentProtocolError.notAnAgent { + // Commonly a shell startup file on the host that prints to stdout ahead of the relay. Not + // `failure()`: the carrier answered, and saying it did not would contradict this. + throw HostConnectionError.serviceUnavailable( + "The host sent something other than an agent's greeting (does a shell startup file print?)." + ) + } + } + + /// `SO_NOSIGPIPE`, set as soon as the socket exists: a write to a peer that has hung up then + /// fails with `EPIPE` rather than killing the app. Not later: once the peer has gone, `setsockopt` + /// itself fails, and a peer that greets and hangs up before the first write left the flag unset + /// and the hello's `send` raising SIGPIPE. + static func noSignalOnWrite(_ fd: Int32) { + var on: Int32 = 1 + setsockopt(fd, SOL_SOCKET, SO_NOSIGPIPE, &on, socklen_t(MemoryLayout.size)) + } + + private static func negotiate( + host: HostID, descriptor fd: Int32, carrier: HostStream?, handshakeTimeout: TimeInterval + ) async throws -> AgentVCSConnection { + // `SO_NOSIGPIPE` is already set: by `connect(host:socketPath:)` or `HostStream.spawn`, when + // the socket was made (see `noSignalOnWrite`). + let connection = try await runBlocking { + do { + var timeout = timeval(tv_sec: max(1, Int(handshakeTimeout.rounded(.up))), tv_usec: 0) setsockopt(fd, SOL_SOCKET, SO_RCVTIMEO, &timeout, socklen_t(MemoryLayout.size)) setsockopt(fd, SOL_SOCKET, SO_SNDTIMEO, &timeout, socklen_t(MemoryLayout.size)) try send(fd, Data(AgentControlClient.encodeHello(build: "Workroom VCS"))) var hello: [UInt8] = [] var helloVersion: UInt16 = 0 - let deadline = Date().addingTimeInterval(2) + let deadline = Date().addingTimeInterval(handshakeTimeout) while true { if let greeting = try AgentControlClient.decodeHello(hello) { helloVersion = greeting.version @@ -152,20 +201,22 @@ final class AgentVCSConnection: HostServiceConnection, @unchecked Sendable { } timeout = timeval(tv_sec: 0, tv_usec: 0) setsockopt(fd, SOL_SOCKET, SO_RCVTIMEO, &timeout, socklen_t(MemoryLayout.size)) - // The 2s handshake bound must not linger onto ordinary request sends — a legitimately busy - // agent (mid VCS request on another thread) can leave the socket buffer full past 2s with + // The handshake bound must not linger onto ordinary request sends — a legitimately busy + // agent (mid VCS request on another thread) can leave the socket buffer full past it with // no fault of its own; a lingering SO_SNDTIMEO would then report that as connection loss. setsockopt(fd, SOL_SOCKET, SO_SNDTIMEO, &timeout, socklen_t(MemoryLayout.size)) - return AgentVCSConnection(host: host, descriptor: fd, helloVersion: helloVersion) + return AgentVCSConnection( + host: host, descriptor: fd, helloVersion: helloVersion, carrier: carrier) } catch { Darwin.close(fd) + carrier?.end() throw error } } DispatchQueue.global(qos: .userInitiated).async { connection.receive() } do { let reply = try await connection.request( - AgentVCSRequest(method: "capabilities"), timeout: 2) + AgentVCSRequest(method: "capabilities"), timeout: handshakeTimeout) let capabilities = try AgentVCSReply.decode(reply) // `backendVersion`, not `serviceUnavailable`: an agent that answers but speaks different reads // (a newer one left running across an app rollback — it outlives the app and may own @@ -176,7 +227,7 @@ final class AgentVCSConnection: HostServiceConnection, @unchecked Sendable { // Not yet shared with any other caller, so a plain lock-guarded write is enough — no // concurrent reader can observe a half-set value. connection.lock.withLock { connection._capabilities = capabilities } - await connection.negotiateFiles() + await connection.negotiateFiles(timeout: handshakeTimeout) return connection } catch HostConnectionError.connectionLost, HostConnectionError.notDispatched { // `notDispatched` too: `request` refuses with it once the connection is closed, and a fresh @@ -219,6 +270,18 @@ final class AgentVCSConnection: HostServiceConnection, @unchecked Sendable { func writer(context: RepositoryContext, reader: VCSProviding) throws -> VCSWriting { guard context.location.host == host else { throw HostConnectionError.mismatchedContext } guard lock.withLock({ !closed }) else { throw HostConnectionError.connectionLost } + // ponytail: no writes to a remote host yet. Refused here, by name, because two parts of the write + // path are local-only in ways that would not fail but quietly be wrong. `CLIVCSWriter` classifies + // failures by reading the repository's `.git` on THIS disk (rebase in progress, a leftover + // `index.lock`), which for a remote path is a different machine or nothing at all. And a jj + // write relies on the caller holding the working-copy barrier for the whole operation + // (`JJSnapshotGate`), which a Mac cannot flock on another host, while the agent's exec service + // deliberately never takes it. Upgrade path: host-side probes, and an exec request that asks the + // agent to hold the barrier itself. + guard host == .local else { + throw HostConnectionError.serviceUnavailable( + "Changes to a remote repository are not supported yet.") + } // Presence and version of the SERVICE, not a count of the caller's own methods. `>=` rather // than `==` so an agent that gains a version 3 does not refuse a client speaking 1 — the agent // is what decides whether it still accepts this request, and it answers that on the request @@ -249,14 +312,14 @@ final class AgentVCSConnection: HostServiceConnection, @unchecked Sendable { /// /// Probed on `Service::File` itself, never through the VCS `capabilities` reply, whose `reads` /// count is compared for equality by every existing client. - private func negotiateFiles() async { + private func negotiateFiles(timeout: TimeInterval) async { // Against the peer's RAW greeting, before any File envelope: a protocol-2 agent silently drops // them, so the probe would otherwise wait out its whole timeout. guard helloVersion >= AgentControlClient.minFileVersion else { return } let outcome: FileNegotiation do { let reply = try await request( - AgentFileRequest(method: "capabilities"), timeout: 2, service: Self.fileService) + AgentFileRequest(method: "capabilities"), timeout: timeout, service: Self.fileService) let capabilities = try AgentFileReply.decode(reply) outcome = capabilities.version == 1 ? .ready : .unsupported } catch { @@ -777,6 +840,7 @@ final class AgentVCSConnection: HostServiceConnection, @unchecked Sendable { return (values, watchers, forwards) } guard let failed else { return } + carrier?.end() ceilingPrompt.finish() for operation in failed.pending { operation.continuation.resume(throwing: error) } // Outside the lock: a handler is caller code. Every subscription learns its watch is gone, so it diff --git a/macapp/WorkroomApp/Core/ContainerHostDriver.swift b/macapp/WorkroomApp/Core/ContainerHostDriver.swift new file mode 100644 index 000000000..986f1f99f --- /dev/null +++ b/macapp/WorkroomApp/Core/ContainerHostDriver.swift @@ -0,0 +1,134 @@ +import Foundation + +/// The first `HostDriver`: a Linux container running sshd and a supervised `wr-agent` +/// (`vcs/scripts/ssh-fixture`, #228), reached over plain ssh-stdio. It is the test fixture for +/// the remote path, and its `openStream` is the ssh transport a real ssh-reachable driver (boxd) +/// will share. +/// +/// Provisioning is Phase 4, so `create`, `deriveFromBase` and `destroy` say so rather than pretend. +/// Which hosts exist is whatever the caller hands in; nothing here persists. +struct ContainerHostDriver: HostDriver { + struct Host: Sendable { + let address: String + let port: Int + let user: String + /// The private key ssh authenticates with. The only credential used: `IdentitiesOnly` and + /// `IdentityAgent none` keep the Mac's ssh agent and default keys out of it. + let identityFile: String + /// The host's public key, ` `, delivered out of band. It is the only key ssh + /// accepts, and a mismatch fails closed: `BatchMode` cannot prompt to accept a new one. + let hostKey: String + /// The supervised agent's socket on the host. + let agentSocket: String + } + + let traits = HostDriverTraits( + transport: .sshStdio, deriveSpeed: nil, deriveCarriesLiveProcesses: false, + durableDisk: false, maxLifetime: nil) + + let hosts: [UUID: Host] + /// Where each host's `ssh_config` and `known_hosts` are written. + let directory: URL + + func create() async throws -> HostID { throw HostDriverError.notImplemented("Creating a base") } + + func deriveFromBase(_ base: HostID) async throws -> HostID { + throw HostDriverError.notImplemented("Deriving a workroom instance") + } + + func destroy(_ host: HostID) async throws { + throw HostDriverError.notImplemented("Destroying a host") + } + + func openStream(to host: HostID) async throws -> HostStream { + guard case .remote(let id) = host, let target = hosts[id] else { + throw HostDriverError.unknownHost(host) + } + let config = try Self.writeConfiguration(for: target, in: directory.appendingPathComponent(id.uuidString)) + return try HostStream.spawn( + URL(fileURLWithPath: "/usr/bin/ssh"), + ["-F", config.path, Self.alias, Self.relayCommand(socket: target.agentSocket)], + // Nothing from the app's own environment: ssh needs none of it with this configuration, and + // `SSH_AUTH_SOCK` in particular is a Mac credential. `-F` also keeps `/etc/ssh/ssh_config` + // out, whose `SendEnv LANG LC_*` would send the Mac's locale across. + environment: [:], + // ssh connects and authenticates before the agent can greet. `ConnectTimeout` bounds the + // connect; this leaves room for the rest. + handshakeTimeout: 20) + } + + static let alias = "workroom-host" + + /// The command ssh runs on the host, quoted for the remote shell. + static func relayCommand(socket: String) -> String { + "wr-agent relay --socket " + shellQuoted(socket) + } + + /// Writes the host's `ssh_config` and `known_hosts`, and returns the config's path. + /// + /// The whole policy is here, so nothing in `~/.ssh/config` can change it: + /// - **It cannot prompt.** `BatchMode` turns a password, passphrase or unknown-host prompt into a + /// failure, and `StrictHostKeyChecking` with only the pinned key known makes any other key one + /// of those failures. `HostKeyAlgorithms` is deliberately not pinned to the key's type: it + /// names signature algorithms, so for an RSA key it would pin SHA-1 `ssh-rsa`, which OpenSSH + /// 8.8+ servers no longer offer. + /// - **No Mac credential crosses.** No agent forwarding, no X11, no port forwards, and the Mac's + /// ssh agent is not consulted for keys. + /// - **A dead link is noticed.** `ServerAliveInterval` ends ssh after about 45s of silence from a + /// host that stopped answering, which ends the stream and so the connection. It is what bounds + /// a link that dies with nothing to send (#228). + static func writeConfiguration(for host: Host, in directory: URL) throws -> URL { + for (name, value) in [ + ("address", host.address), ("user", host.user), ("identity file", host.identityFile), + ("host key", host.hostKey), ("agent socket", host.agentSocket), + ] { + guard !value.isEmpty, !value.contains(where: { $0.isNewline || $0 == "\"" || $0 == "\0" }) + else { + throw HostDriverError.invalidConfiguration("\(name) is empty or has a quote or newline") + } + } + guard (1...65535).contains(host.port) else { + throw HostDriverError.invalidConfiguration("port \(host.port) is out of range") + } + let key = host.hostKey.split(separator: " ") + guard key.count >= 2 else { + throw HostDriverError.invalidConfiguration("host key must be ` `") + } + try FileManager.default.createDirectory(at: directory, withIntermediateDirectories: true) + let knownHosts = directory.appendingPathComponent("known_hosts") + let name = host.port == 22 ? host.address : "[\(host.address)]:\(host.port)" + try "\(name) \(key[0]) \(key[1])\n".write(to: knownHosts, atomically: true, encoding: .utf8) + let config = directory.appendingPathComponent("ssh_config") + try """ + Host \(alias) + HostName "\(host.address)" + Port \(host.port) + User "\(host.user)" + IdentityFile "\(host.identityFile)" + IdentitiesOnly yes + IdentityAgent none + BatchMode yes + StrictHostKeyChecking yes + UserKnownHostsFile "\(knownHosts.path)" + GlobalKnownHostsFile /dev/null + UpdateHostKeys no + ForwardAgent no + ForwardX11 no + ClearAllForwardings yes + ControlMaster no + ControlPath none + RequestTTY no + ServerAliveInterval 15 + ServerAliveCountMax 3 + ConnectTimeout 10 + LogLevel ERROR + + """.write(to: config, atomically: true, encoding: .utf8) + return config + } + + /// One POSIX shell word, whatever `text` holds. + static func shellQuoted(_ text: String) -> String { + "'" + text.replacingOccurrences(of: "'", with: "'\\''") + "'" + } +} diff --git a/macapp/WorkroomApp/Core/HostDriver.swift b/macapp/WorkroomApp/Core/HostDriver.swift new file mode 100644 index 000000000..ca3c3e8d7 --- /dev/null +++ b/macapp/WorkroomApp/Core/HostDriver.swift @@ -0,0 +1,194 @@ +import Darwin +import Foundation + +/// How a provider's hosts are reached and what they can do, declared so a provider can be gated +/// when it is chosen (design doc, Phase 3 "the stream drivers", and Premise 8). +struct HostDriverTraits: Equatable, Sendable { + enum Transport: Equatable, Sendable { case sshStdio, sdkExec, webSocket } + + let transport: Transport + /// Roughly how long `deriveFromBase` takes, or nil for a driver that cannot derive yet. + let deriveSpeed: Duration? + /// Whether a derived instance carries the base's running processes (a live fork), or only its + /// disk. + let deriveCarriesLiveProcesses: Bool + let durableDisk: Bool + /// The wall clock an instance dies on, or nil for one that lives until destroyed. + let maxLifetime: Duration? +} + +/// One provider. Four methods, and the only one that is not provisioning is `openStream` +/// (design doc, Phase 3). `deriveFromBase` is the ONLY way a workroom instance comes to exist; +/// `create` makes the base it derives from. +/// +/// **Every driver must authenticate its peer as strongly as a shell login.** The agent's exec +/// service runs `git` with arbitrary arguments, which is arbitrary code execution, so a stream +/// that reaches an agent is a shell whatever it is called. ssh meets that bar; an SDK exec or a +/// WebSocket driver must show it does before it counts as a driver. +protocol HostDriver: Sendable { + var traits: HostDriverTraits { get } + /// Provisions a base. + func create() async throws -> HostID + /// Provisions a workroom instance from `base`. + func deriveFromBase(_ base: HostID) async throws -> HostID + /// Takes down a base or an instance. + func destroy(_ host: HostID) async throws + /// A byte stream to the agent on `host`, base or instance, for + /// `AgentVCSConnection.connect(host:stream:)`. + func openStream(to host: HostID) async throws -> HostStream +} + +enum HostDriverError: Error, Equatable, Sendable, LocalizedError { + case unknownHost(HostID) + /// Provisioning lands in Phase 4, which is what says how a base and an instance come to exist. + case notImplemented(String) + case invalidConfiguration(String) + + var errorDescription: String? { + switch self { + case .unknownHost: return "This driver has no such host." + case .notImplemented(let what): return "\(what) is not implemented yet." + case .invalidConfiguration(let detail): return "Invalid host configuration: \(detail)" + } + } +} + +/// A bidirectional byte stream to the agent on a far-side host: one end of a socketpair, whose +/// other end is the stdin and stdout of the local process carrying it (`ssh host wr-agent relay`). +/// +/// Whoever connects over it owns both: `AgentVCSConnection` closes the descriptor and ends the +/// process with the connection. The carrier's exit is also how a lost link is noticed. The child +/// holds the only copy of its end of the pair, so when it exits, that end closes and the next read +/// on this one sees EOF. +final class HostStream: @unchecked Sendable { + let descriptor: Int32 + /// How long the agent's greeting may take: a local relay answers at once, ssh must connect and + /// authenticate first. + let handshakeTimeout: TimeInterval + private let process: Process + private let lock = NSLock() + private var errors = Data() + /// stderr reached EOF: everything the carrier said is in `errors`. + private var errorsClosed = false + /// `end()` stopped a carrier that was still running, so its exit status is our own signal. + private var endedByUs = false + /// A connection took the descriptor and the process (`handOff`), so `deinit` leaves them alone. + private var handedOff = false + + var processIdentifier: Int32 { process.processIdentifier } + + private init(descriptor: Int32, process: Process, handshakeTimeout: TimeInterval) { + self.descriptor = descriptor + self.process = process + self.handshakeTimeout = handshakeTimeout + } + + /// A stream nothing connected over is still this object's: an ssh whose stdin never reads EOF + /// holds a healthy link open for good. + deinit { + guard !lock.withLock({ handedOff }) else { return } + Darwin.close(descriptor) + end() + } + + /// Gives the descriptor and the process to the connection made over them, which closes and ends + /// them from then on. + func handOff() -> Int32 { + lock.withLock { handedOff = true } + return descriptor + } + + /// Runs `executable` with a socketpair end as both its stdin and its stdout. + /// + /// `environment` is the carrier's WHOLE environment, never merged with the app's: whatever it + /// holds can reach the far side, so a driver passes exactly what its transport needs. + static func spawn( + _ executable: URL, _ arguments: [String], environment: [String: String], + handshakeTimeout: TimeInterval + ) throws -> HostStream { + var pair: [Int32] = [0, 0] + guard socketpair(AF_UNIX, SOCK_STREAM, 0, &pair) == 0 else { + throw HostConnectionError.serviceUnavailable("socketpair failed: errno \(errno)") + } + let (ours, theirs) = (pair[0], pair[1]) + AgentVCSConnection.noSignalOnWrite(ours) + // Close-on-exec on both, so a process spawned later cannot inherit an end: a stray copy of the + // child's end would keep it open after the carrier exits, and the lost link would never read + // as EOF here. There is still a window between `socketpair` and these two calls, which Darwin + // cannot close (it has no `SOCK_CLOEXEC`); Foundation's own spawns close every descriptor they + // were not given, so only a raw `fork` in that window could catch one. The spawn below dup2s + // the child's end onto 0 and 1, which clears the flag on those copies only. + _ = fcntl(ours, F_SETFD, FD_CLOEXEC) + _ = fcntl(theirs, F_SETFD, FD_CLOEXEC) + + let process = Process() + process.executableURL = executable + process.arguments = arguments + process.environment = environment + let child = FileHandle(fileDescriptor: theirs, closeOnDealloc: false) + process.standardInput = child + process.standardOutput = child + let errors = Pipe() + process.standardError = errors + let stream = HostStream( + descriptor: ours, process: process, handshakeTimeout: handshakeTimeout) + errors.fileHandleForReading.readabilityHandler = { [weak stream] handle in + let data = handle.availableData + guard !data.isEmpty else { + handle.readabilityHandler = nil + stream?.lock.withLock { stream?.errorsClosed = true } + return + } + stream?.collect(data) + } + do { + try process.run() + } catch { + // Only the child's end: `ours` is the stream's, and its `deinit` closes it on the way out. + Darwin.close(theirs) + errors.fileHandleForReading.readabilityHandler = nil + throw HostConnectionError.serviceUnavailable( + "Could not start \(executable.lastPathComponent): \(error.localizedDescription)") + } + // The child now holds the only copy. Keeping this one would hide the child's exit. + Darwin.close(theirs) + return stream + } + + /// Ends the carrier. Idempotent, and safe after it has exited by itself. + func end() { + guard process.isRunning else { return } + lock.withLock { endedByUs = true } + process.terminate() + } + + /// What the carrier said about why the stream ended, for a handshake it never answered: its + /// stderr, or failing that how it ended. Waits for stderr to reach EOF rather than for the + /// process to be reaped: the two are separate events, and the reason ("Host key verification + /// failed.") is in the first. + func failure() async -> String { + for _ in 0..<40 where !lock.withLock({ errorsClosed }) || process.isRunning { + try? await Task.sleep(for: .milliseconds(50)) + } + let (said, endedByUs) = lock.withLock { + (String(decoding: errors, as: UTF8.self), self.endedByUs) + } + let reason = said.trimmingCharacters(in: .whitespacesAndNewlines) + if !reason.isEmpty { return reason } + let name = process.executableURL?.lastPathComponent ?? "carrier" + // Our own SIGTERM, from `end()`: the carrier was still running when the handshake gave up. + let stoppedByUs = + endedByUs && process.terminationReason == .uncaughtSignal + && process.terminationStatus == SIGTERM + if process.isRunning || stoppedByUs { return "\(name) did not answer in time" } + return "\(name) exited with status \(process.terminationStatus)" + } + + /// Keeps the first 16 KiB of stderr: enough for any diagnosis, and bounded against a chatty one. + private func collect(_ data: Data) { + lock.withLock { + guard errors.count < 16 * 1024 else { return } + errors.append(data.prefix(16 * 1024 - errors.count)) + } + } +} diff --git a/macapp/WorkroomApp/Core/StatusCommandRunner.swift b/macapp/WorkroomApp/Core/StatusCommandRunner.swift index 230cdcfc0..c84e9b798 100644 --- a/macapp/WorkroomApp/Core/StatusCommandRunner.swift +++ b/macapp/WorkroomApp/Core/StatusCommandRunner.swift @@ -207,6 +207,13 @@ struct StatusCommandRunner: StatusCommandRunning, Sendable { return env } + /// The whole environment a REMOTE exec request carries: the pins that are policy, not identity. + /// The remote agent builds everything else from the host's own environment. See + /// `AgentCommandRunner.request` for why nothing else crosses. + static let remoteEnvironment = [ + "GIT_OPTIONAL_LOCKS": "0", "GIT_TERMINAL_PROMPT": "0", "LC_ALL": "C", + ] + /// The complete environment a `git`/`jj` child gets, native or agent-routed. /// /// Extracted from `run` and made static so `AgentCommandRunner` can send the SAME map to diff --git a/macapp/WorkroomAppTests/AgentVCSProtocolTests.swift b/macapp/WorkroomAppTests/AgentVCSProtocolTests.swift index 9d3bf2618..dddf5ba32 100644 --- a/macapp/WorkroomAppTests/AgentVCSProtocolTests.swift +++ b/macapp/WorkroomAppTests/AgentVCSProtocolTests.swift @@ -260,6 +260,33 @@ final class AgentVCSProtocolTests: XCTestCase { XCTAssertEqual(request.kind, "exec") } + /// Nothing from the Mac's environment reaches a remote host's exec request (#229): its `HOME` and + /// `PATH` name nothing there, and its `SSH_AUTH_SOCK` is a Mac credential. Checked on the encoded + /// wire bytes, which are what crosses. + func testARemoteExecRequestCarriesNothingFromTheMacsEnvironment() throws { + let encoder = JSONEncoder() + encoder.keyEncodingStrategy = .convertToSnakeCase + for network in [false, true] { + let local = AgentCommandRunner.request( + "git", ["push"], in: "/r", timeout: 1, stdin: nil, network: network, host: .local) + // The control: locally the Mac's environment is the point. + XCTAssertNotNil(local.env["PATH"]) + let localWire = String(decoding: try encoder.encode(local), as: UTF8.self) + // An agent that predates the field rejects a request carrying it, and a local one may. + XCTAssertFalse(localWire.contains("host_environment"), localWire) + + let remote = AgentCommandRunner.request( + "git", ["push"], in: "/r", timeout: 1, stdin: nil, network: network, + host: .remote(UUID())) + XCTAssertEqual(remote.env, StatusCommandRunner.remoteEnvironment) + let wire = String(decoding: try encoder.encode(remote), as: UTF8.self) + for key in ["HOME", "PATH", "SSH_AUTH_SOCK", "USER", "GIT_SSH_COMMAND"] { + XCTAssertFalse(wire.contains("\"\(key)\""), "\(key) crossed (network: \(network)): \(wire)") + } + XCTAssertTrue(wire.contains("\"host_environment\":true"), wire) + } + } + // MARK: File service (#211) /// The client's five version constants are the agent's `PROTOCOL_VERSION`, `MIN_VCS_VERSION`, diff --git a/macapp/WorkroomAppTests/HostStreamTests.swift b/macapp/WorkroomAppTests/HostStreamTests.swift new file mode 100644 index 000000000..05e008ac1 --- /dev/null +++ b/macapp/WorkroomAppTests/HostStreamTests.swift @@ -0,0 +1,133 @@ +import Darwin +import XCTest + +@testable import Workroom + +/// A driver's stream with no ssh in it: `wr-agent relay` to a local agent, carried over a +/// socketpair exactly as ssh carries it. Runs wherever `make app-test` does, so CI covers the +/// transport's plumbing; the ssh leg is `RemoteHostIntegrationTests`, against the container. +final class HostStreamTests: XCTestCase { + private var agents: [AgentHarness] = [] + + override func tearDown() { + for agent in agents { agent.stop() } + agents.removeAll() + super.tearDown() + } + + private func agent() throws -> AgentHarness { + let agent = try AgentHarness.start() + agents.append(agent) + return agent + } + + private func relay(to socket: String) throws -> HostStream { + try HostStream.spawn( + try AgentHarness.binaryURL(), ["relay", "--socket", socket], environment: [:], + handshakeTimeout: 5) + } + + private func eventually(_ condition: () async -> Bool) async -> Bool { + for _ in 0..<100 { + if await condition() { return true } + try? await Task.sleep(for: .milliseconds(50)) + } + return await condition() + } + + private static func exited(_ pid: Int32) -> Bool { kill(pid, 0) == -1 && errno == ESRCH } + + func testRequestsAnswerThroughARelayedStream() async throws { + let socket = try agent().socketPath + let host = HostID.remote(UUID()) + let connection = try await AgentVCSConnection.connect(host: host, stream: try relay(to: socket)) + // `connect` already negotiated; this proves the stream keeps carrying traffic after it. + let reply = try await connection.request(AgentVCSRequest(method: "capabilities"), timeout: 5) + XCTAssertEqual(try AgentVCSReply.decode(reply).version, 1) + await connection.close() + } + + /// The carrier dying is the link dying: the manager must see the connection go, with no request + /// in flight to notice it. Without the parent closing its copy of the child's end, the pair never + /// reads EOF and this waits forever. + func testACarrierThatDiesIsALostConnection() async throws { + let socket = try agent().socketPath + let host = HostID.remote(UUID()) + let stream = try relay(to: socket) + let manager = HostConnectionManager() + _ = try await manager.connect(host: host) { + try await AgentVCSConnection.connect(host: host, stream: stream) + } + let connected = await manager.snapshot(for: host).status + XCTAssertEqual(connected, .connected) + kill(stream.processIdentifier, SIGKILL) + let lost = await eventually { await manager.snapshot(for: host).status == .disconnected } + XCTAssertTrue(lost, "the manager never noticed the carrier die") + } + + /// A closed connection must not leave its ssh behind. + func testClosingTheConnectionEndsTheCarrier() async throws { + let socket = try agent().socketPath + let stream = try relay(to: socket) + let connection = try await AgentVCSConnection.connect(host: .remote(UUID()), stream: stream) + let pid = stream.processIdentifier + await connection.close() + let ended = await eventually { Self.exited(pid) } + XCTAssertTrue(ended, "the carrier outlived its connection") + } + + /// No agent is the relay's own diagnosis, surfaced as it said it: nothing on a remote host can + /// be respawned, so `connectionLost` (which `LocalAgentVCS` answers by spawning an agent) would + /// be the wrong error and would lose the reason. + func testARelayWithNoAgentFailsWithItsOwnReason() async throws { + let missing = "/tmp/wr-none-\(UUID().uuidString.prefix(8)).sock" + let stream = try relay(to: missing) + do { + _ = try await AgentVCSConnection.connect(host: .remote(UUID()), stream: stream) + XCTFail("connected to nothing") + } catch HostConnectionError.serviceUnavailable(let detail) { + XCTAssertTrue(detail.contains("no agent listening"), detail) + } + let ended = await eventually { Self.exited(stream.processIdentifier) } + XCTAssertTrue(ended) + } + + func testTheSSHConfigurationCannotPromptOrForwardAnything() throws { + let directory = FileManager.default.temporaryDirectory.appendingPathComponent( + UUID().uuidString) + defer { try? FileManager.default.removeItem(at: directory) } + let host = ContainerHostDriver.Host( + address: "127.0.0.1", port: 2222, user: "workroom", identityFile: "/keys/id", + hostKey: "ssh-ed25519 AAAAC3Nza", agentSocket: "/run/workroom/agent.sock") + let config = try String( + contentsOf: ContainerHostDriver.writeConfiguration(for: host, in: directory), + encoding: .utf8) + for line in [ + "BatchMode yes", "StrictHostKeyChecking yes", "GlobalKnownHostsFile /dev/null", + "IdentitiesOnly yes", "IdentityAgent none", + "ForwardAgent no", "ClearAllForwardings yes", "ServerAliveInterval 15", + ] { + XCTAssertTrue(config.contains(" \(line)\n"), "missing \(line)") + } + XCTAssertFalse(config.contains("SendEnv")) + let known = try String( + contentsOf: directory.appendingPathComponent("known_hosts"), encoding: .utf8) + XCTAssertEqual(known, "[127.0.0.1]:2222 ssh-ed25519 AAAAC3Nza\n") + } + + /// A value is written into `ssh_config`, where a newline would start a directive of its own. + func testAConfigurationValueCannotInjectADirective() { + let host = ContainerHostDriver.Host( + address: "127.0.0.1\n ProxyCommand touch /tmp/owned", port: 22, user: "workroom", + identityFile: "/keys/id", hostKey: "ssh-ed25519 AAAA", agentSocket: "/run/a.sock") + XCTAssertThrowsError( + try ContainerHostDriver.writeConfiguration( + for: host, in: FileManager.default.temporaryDirectory)) + } + + func testTheRelayCommandQuotesItsSocketForTheRemoteShell() { + XCTAssertEqual( + ContainerHostDriver.relayCommand(socket: "/run/it's here/a.sock"), + "wr-agent relay --socket '/run/it'\\''s here/a.sock'") + } +} diff --git a/macapp/WorkroomAppTests/RemoteHostIntegrationTests.swift b/macapp/WorkroomAppTests/RemoteHostIntegrationTests.swift new file mode 100644 index 000000000..d2ace3660 --- /dev/null +++ b/macapp/WorkroomAppTests/RemoteHostIntegrationTests.swift @@ -0,0 +1,216 @@ +import Darwin +import XCTest + +@testable import Workroom + +/// The app's half of the remote path against a real host (#229): `ContainerHostDriver` over ssh +/// into the container fixture, driven by the real Swift clients. Skipped unless run through the +/// fixture script, which starts the container and says where it is: +/// +/// vcs/scripts/ssh-fixture/run.sh \ +/// make app-test APP_TEST_FLAGS=-only-testing:WorkroomAppTests/RemoteHostIntegrationTests +/// +/// The macOS CI runners have no Docker, so CI covers the stream without ssh (`HostStreamTests`) +/// and the ssh leg from Rust (`remote_transport.rs`, on the Linux runner). +final class RemoteHostIntegrationTests: XCTestCase { + private var directory: URL! + private var connections: [AgentVCSConnection] = [] + + override func setUp() { + super.setUp() + directory = FileManager.default.temporaryDirectory.appendingPathComponent( + "wr-remote-\(UUID().uuidString.prefix(8))") + } + + override func tearDown() async throws { + for connection in connections { await connection.close() } + connections.removeAll() + try? FileManager.default.removeItem(at: directory) + try await super.tearDown() + } + + private struct Fixture { + let config: String + let host: ContainerHostDriver.Host + } + + private func fixture() throws -> Fixture { + let environment = ProcessInfo.processInfo.environment + func need(_ name: String) throws -> String { + guard let value = environment[name], !value.isEmpty else { + throw XCTSkip("\(name) is unset; run these through vcs/scripts/ssh-fixture/run.sh") + } + return value + } + return Fixture( + config: try need("WR_SSH_FIXTURE_CONFIG"), + host: ContainerHostDriver.Host( + address: try need("WR_SSH_FIXTURE_ADDRESS"), + port: Int(try need("WR_SSH_FIXTURE_PORT")) ?? 0, + user: try need("WR_SSH_FIXTURE_USER"), + identityFile: try need("WR_SSH_FIXTURE_IDENTITY"), + hostKey: try need("WR_SSH_FIXTURE_HOST_KEY"), + agentSocket: try need("WR_SSH_FIXTURE_SOCKET"))) + } + + private func connect(_ host: ContainerHostDriver.Host) async throws -> ( + AgentVCSConnection, UUID + ) { + let id = UUID() + let driver = ContainerHostDriver(hosts: [id: host], directory: directory) + let connection = try await AgentVCSConnection.connect( + host: .remote(id), stream: try await driver.openStream(to: .remote(id))) + connections.append(connection) + return (connection, id) + } + + /// Runs `command` on the fixture over the script's own ssh, to set the host up. Not through the + /// driver, so a setup step can never be the thing under test. + @discardableResult + private func onHost(_ fixture: Fixture, _ command: String) throws -> String { + let process = Process() + process.executableURL = URL(fileURLWithPath: "/usr/bin/ssh") + process.arguments = ["-F", fixture.config, "fixture", command] + let output = Pipe() + process.standardOutput = output + process.standardError = output + try process.run() + let data = output.fileHandleForReading.readDataToEndOfFile() + process.waitUntilExit() + let text = String(decoding: data, as: UTF8.self) + guard process.terminationStatus == 0 else { + XCTFail("on the host, \(command) failed: \(text)") + throw VCSError.io(text) + } + return text.trimmingCharacters(in: .whitespacesAndNewlines) + } + + /// A git repository on the host, with its identity in the repository's own config: the host has + /// no global one, and nothing from the Mac supplies it. + private func repository(_ fixture: Fixture) throws -> String { + let path = "/home/workroom/repo-\(UUID().uuidString.prefix(8))" + try onHost( + fixture, + """ + set -e; git init -q -b main \(path); cd \(path) + git config user.name Remote; git config user.email remote@example.com + echo base > file; git add .; git commit -qm initial + """) + return path + } + + /// VCS reads, File, Status and Forward, all through one ssh-stdio connection and the same manager + /// and router the app uses. + func testEveryServiceAnswersThroughTheDriversStream() async throws { + let fixture = try fixture() + let path = try repository(fixture) + let (connection, id) = try await connect(fixture.host) + let host = HostID.remote(id) + let manager = HostConnectionManager() + _ = try await manager.connect(host: host) { connection } + let router = RepositoryRouter(connections: manager) + let location = try RepositoryLocation.remote(host: id, path: path) + try router.register(.init(location: location, backend: .git, sharedLocation: location)) + + // VCS reads. + let reader = try await router.reader(for: location) + let ref = try await reader.currentRef() + XCTAssertEqual(ref, VCSRef(name: "main", kind: .branch)) + let page = try await reader.log(limit: 5) + XCTAssertEqual(page.commits.map(\.summary), ["initial"]) + + // Writes are refused by name, not attempted: see `AgentVCSConnection.writer`. + do { + _ = try await router.writer(for: location) + XCTFail("a remote writer was handed out") + } catch HostConnectionError.serviceUnavailable(let detail) { + XCTAssertTrue(detail.contains("remote repository"), detail) + } + + // File. + let files = try connection.files( + context: FileContext(location: location, sharedLocation: nil)) + let listing = try await files.list(.git) + XCTAssertEqual(FileListing.parse(listing.stdout, vcs: .git), ["file"]) + let data = try await files.read(path: "file", symlinks: .refuse, maxBytes: 100) + XCTAssertEqual(data, Data("base\n".utf8)) + + // Status. + _ = try await connection.wakefulness().status() + + // Forward, to a process listening in the container: its own sshd, which speaks first. + let forward = try connection.forwarding().listen(remotePort: 22) { _ in } + defer { forward.stop() } + let client = try TCPClient(port: forward.localPort) + defer { client.close() } + let banner = String(decoding: try client.read(8, timeout: 10), as: UTF8.self) + XCTAssertEqual(banner, "SSH-2.0-") + } + + /// Nothing from the Mac's environment reaches a remote git (#229): the child runs with the + /// host's `HOME` and `PATH`, no `SSH_AUTH_SOCK`, and no way for ssh to prompt. Read back from the + /// child itself, through a git alias that prints its environment. + func testARemoteCommandRunsInTheHostsEnvironmentNotTheMacs() async throws { + let fixture = try fixture() + let path = try repository(fixture) + let (connection, _) = try await connect(fixture.host) + let result = await AgentCommandRunner(connection: connection).runNetwork( + "git", ["-c", "alias.environment=!env", "environment"], in: path, timeout: 20) + XCTAssertEqual(result.exitCode, 0, result.stderr) + let environment = Dictionary( + result.stdout.split(separator: "\n").compactMap { line -> (String, String)? in + guard let equals = line.firstIndex(of: "=") else { return nil } + return (String(line[.. Vec<(String, String)> { /// `listing_environment` over an explicit variable set, so the scrub and the pins can be tested /// without mutating this process's own environment — which other tests' child processes inherit. -fn scrubbed_environment( +pub(crate) fn scrubbed_environment( vars: impl Iterator, ) -> Vec<(String, String)> { const SCRUBBED: [&str; 7] = [ diff --git a/vcs/crates/wr-agent/src/vcs.rs b/vcs/crates/wr-agent/src/vcs.rs index af3b9b991..17a132b52 100644 --- a/vcs/crates/wr-agent/src/vcs.rs +++ b/vcs/crates/wr-agent/src/vcs.rs @@ -198,6 +198,44 @@ struct ExecRequest { stdin: Option, #[serde(default)] env: std::collections::BTreeMap, + /// Set by the app for a REMOTE host (#229): `env` is then only policy pins, and the child's + /// environment is built from this agent's own, which is the host's (`host_environment`). The + /// app sends nothing from the Mac there: its `HOME` and `PATH` name nothing on this machine, and + /// its `SSH_AUTH_SOCK` and git identity are the user's Mac credentials. + #[serde(default)] + host_environment: bool, +} + +/// The child environment for a remote host: this agent's own, which is the host's, with the file +/// service's scrub and pins (`scrubbed_environment`, so the two host-side paths agree), no graphical +/// prompt helper, and the request's pins on top. +/// +/// ssh is kept from prompting WITHOUT setting `GIT_SSH_COMMAND`, which would outrank a repository's +/// own `core.sshCommand`, the usual home of a deploy key, and so replace the host's ssh for every +/// repository on it. It cannot prompt anyway: `SSH_ASKPASS_REQUIRE=never` rules out a helper, and a +/// supervised agent has no terminal for one. A `GIT_SSH_COMMAND` the host sets itself is kept and +/// gets `BatchMode`, as the app does locally (`StatusCommandRunner.networkEnvironment`). +/// +/// Built from `vars_os`, not `vars`, which panics on one variable that is not UTF-8 and would take +/// the exec thread down before it replied. +fn host_environment( + own: impl Iterator, + pins: std::collections::BTreeMap, +) -> std::collections::BTreeMap { + let mut env: std::collections::BTreeMap = + crate::file::scrubbed_environment(own).into_iter().collect(); + env.remove("SSH_ASKPASS"); + env.remove("DISPLAY"); + env.extend(pins); + if let Some(ssh) = env + .get("GIT_SSH_COMMAND") + .map(|command| command.trim().to_string()) + .filter(|command| !command.is_empty()) + { + env.insert("GIT_SSH_COMMAND".into(), format!("{ssh} -o BatchMode=yes")); + } + env.insert("SSH_ASKPASS_REQUIRE".into(), "never".into()); + env } #[derive(Deserialize)] @@ -795,6 +833,7 @@ fn exec(request: ExecRequest) -> model::Result { timeout_ms, stdin, env, + host_environment: from_host, } = request; if version != 1 { return Err(VcsError::BackendVersion( @@ -803,6 +842,11 @@ fn exec(request: ExecRequest) -> model::Result { } let dir = absolute(&dir)?; let timeout = exec_timeout(timeout_ms); + let env = if from_host { + host_environment(std::env::vars_os(), env) + } else { + env + }; let env: Vec<(&str, &str)> = env.iter().map(|(k, v)| (k.as_str(), v.as_str())).collect(); let stdin = stdin.as_deref().map(latin1_bytes).transpose()?; let captured = run_exec( @@ -1532,6 +1576,62 @@ mod tests { std::fs::remove_dir_all(&root).unwrap(); } + /// A remote host's child environment (#229): the host's own, less what could point git at + /// another repository or let ssh prompt, with the app's pins on top. + #[test] + fn a_host_environment_is_the_hosts_own_with_the_pins_on_top_and_ssh_unable_to_prompt() { + let pairs = |pairs: &[(&str, &str)]| { + pairs + .iter() + .map(|(k, v)| (k.to_string(), v.to_string())) + .collect::>() + }; + let os = |pairs: Vec<(String, String)>| { + pairs + .into_iter() + .map(|(k, v)| (k.into(), v.into())) + .collect::>() + }; + let own = pairs(&[ + ("HOME", "/home/workroom"), + ("PATH", "/usr/bin:/bin"), + ("GIT_DIR", "/elsewhere/.git"), + ("SSH_ASKPASS", "/usr/bin/ask"), + ("LC_ALL", "fr_FR.UTF-8"), + ("GIT_SSH_COMMAND", "ssh -i /home/workroom/.ssh/deploy"), + ]); + let pins = pairs(&[("LC_ALL", "C"), ("GIT_TERMINAL_PROMPT", "0")]) + .into_iter() + .collect(); + let env = host_environment(os(own).into_iter(), pins); + assert_eq!(env["HOME"], "/home/workroom"); + assert_eq!(env["PATH"], "/usr/bin:/bin"); + assert!(!env.contains_key("GIT_DIR")); + assert!(!env.contains_key("SSH_ASKPASS")); + assert_eq!(env["LC_ALL"], "C", "a pin outranks the host's value"); + assert_eq!(env["GIT_TERMINAL_PROMPT"], "0"); + assert_eq!( + env["GIT_SSH_COMMAND"], + "ssh -i /home/workroom/.ssh/deploy -o BatchMode=yes" + ); + assert_eq!(env["SSH_ASKPASS_REQUIRE"], "never"); + + // With none of its own, it gets none: set, it would outrank a repository's + // `core.sshCommand`, where a deploy key usually lives. + let bare = host_environment(std::iter::empty(), Default::default()); + assert!(!bare.contains_key("GIT_SSH_COMMAND")); + assert_eq!(bare["SSH_ASKPASS_REQUIRE"], "never"); + + // A variable that is not UTF-8 is dropped, not a panic. + use std::os::unix::ffi::OsStringExt; + let latin1 = vec![( + std::ffi::OsString::from("LEGACY"), + std::ffi::OsString::from_vec(vec![0xe9]), + )]; + let env = host_environment(latin1.into_iter(), Default::default()); + assert!(!env.contains_key("LEGACY")); + } + /// The regression that motivated bounding the drain: a child that exits promptly while leaving /// a BACKGROUND descendant holding its stdout must not hold the exec thread (and with it one of /// the 32 `ACTIVE` permits) for the descendant's lifetime. Before the bound, this returned in diff --git a/vcs/scripts/ssh-fixture/entrypoint.sh b/vcs/scripts/ssh-fixture/entrypoint.sh index 9e6de2e34..fa9d2891b 100755 --- a/vcs/scripts/ssh-fixture/entrypoint.sh +++ b/vcs/scripts/ssh-fixture/entrypoint.sh @@ -17,11 +17,12 @@ chmod 600 /home/workroom/.ssh/authorized_keys # `--idle-timeout never`: a remote agent must keep running with no client attached, because its # BUSY/IDLE reports have to keep flowing while the Mac sleeps. Run as the ssh user, so the relay -# that user runs can reach the socket. +# that user runs can reach the socket. `env -i`, because the agent's environment is what every git +# it runs gets (the app sends none from the Mac), and this script's own holds AUTHORIZED_KEY. ( while :; do setpriv --reuid=workroom --regid=workroom --init-groups \ - env HOME=/home/workroom USER=workroom SHELL=/bin/bash \ + env -i HOME=/home/workroom USER=workroom SHELL=/bin/bash PATH=/usr/local/bin:/usr/bin:/bin \ wr-agent serve --socket /run/workroom/agent.sock --idle-timeout never || true sleep 1 done diff --git a/vcs/scripts/ssh-fixture/run.sh b/vcs/scripts/ssh-fixture/run.sh index a79926406..284dfa7b7 100755 --- a/vcs/scripts/ssh-fixture/run.sh +++ b/vcs/scripts/ssh-fixture/run.sh @@ -2,7 +2,7 @@ # # Runs the remote-path tests over real ssh, into a container (issue #228). # -# vcs/scripts/ssh-fixture/run.sh +# vcs/scripts/ssh-fixture/run.sh [command...] # # The argument is a Linux wr-agent for the CONTAINER's architecture, built the way the app bundles # it: static musl, with terminal-state. CI passes the one its `agent-linux` job just built. On an @@ -17,6 +17,14 @@ # pins the container's host key out of band, and runs the ignored `over_ssh_*` tests in # remote_transport.rs against it with BatchMode ssh. The tests run on THIS machine; only the agent # and sshd are in the container, as they will be on a real remote host. +# +# Given a command, it runs that instead, with the fixture's whereabouts in WR_SSH_FIXTURE_*. The +# app's tests take them from there, run from the repository root: +# +# vcs/scripts/ssh-fixture/run.sh \ +# make app-test APP_TEST_FLAGS=-only-testing:WorkroomAppTests/RemoteHostIntegrationTests +# +# xcodebuild hands a test only the variables prefixed TEST_RUNNER_, so each is exported twice. set -euo pipefail AGENT="${1:?usage: run.sh }" @@ -87,10 +95,18 @@ for attempt in $(seq 1 100); do sleep 0.2 done -# One thread: the tests share one agent, and the supervisor test kills it. -cd "$VCS_DIR" -if ! WR_SSH_FIXTURE_CONFIG="$STAGE/ssh_config" WR_SSH_FIXTURE_SOCKET="$SOCKET" \ - cargo test -p wr-agent --test remote_transport -- --ignored --test-threads=1; then +for pair in "CONFIG=$STAGE/ssh_config" "SOCKET=$SOCKET" "ADDRESS=127.0.0.1" "PORT=$PORT" \ + "USER=workroom" "IDENTITY=$STAGE/id_ed25519" "HOST_KEY=$HOST_KEY"; do + export "WR_SSH_FIXTURE_$pair" "TEST_RUNNER_WR_SSH_FIXTURE_$pair" +done + +shift +if [ "$#" -eq 0 ]; then + # One thread: the tests share one agent, and the supervisor test kills it. + cd "$VCS_DIR" + set -- cargo test -p wr-agent --test remote_transport -- --ignored --test-threads=1 +fi +if ! "$@"; then echo "error: the ssh fixture tests failed; the container's log:" >&2 "$RUNTIME" logs "$NAME" >&2 exit 1 From 3ceb27e06ec5e41230f276ebe283db73601a90ad Mon Sep 17 00:00:00 2001 From: Joel Moss Date: Thu, 24 Sep 2026 20:49:21 +0100 Subject: [PATCH 2/7] fix(macapp): suppress SIGPIPE on every agent socket before it can fail Three more sockets could raise SIGPIPE and kill the app when the peer hung up first, the same shape as the handshake crash fixed in the previous commit: - AgentControlClient.transact never set SO_NOSIGPIPE. - PersistentSessionControlClient.exchange wrote with no suppression at all. - A port forward set it after accept, and on a client that had already reset, setsockopt fails with EINVAL. The pump's first write then raised SIGPIPE. The two clients now set it right after socket(), before connect. The forward checks the setsockopt result and drops a client that is already gone. Co-Authored-By: Claude Opus 5.5 --- macapp/WorkroomApp/Core/AgentPortForwarding.swift | 12 +++++++++++- .../Core/Session/AgentControlClient.swift | 2 ++ .../Session/PersistentSessionControlClient.swift | 3 +++ 3 files changed, 16 insertions(+), 1 deletion(-) diff --git a/macapp/WorkroomApp/Core/AgentPortForwarding.swift b/macapp/WorkroomApp/Core/AgentPortForwarding.swift index 5fd59cc93..349a7799f 100644 --- a/macapp/WorkroomApp/Core/AgentPortForwarding.swift +++ b/macapp/WorkroomApp/Core/AgentPortForwarding.swift @@ -278,7 +278,17 @@ final class PortForward: @unchecked Sendable { // blocking `read` is the whole design. _ = fcntl(client, F_SETFL, fcntl(client, F_GETFL) & ~O_NONBLOCK) var enabled: Int32 = 1 - setsockopt(client, SOL_SOCKET, SO_NOSIGPIPE, &enabled, socklen_t(MemoryLayout.size)) + // Checked, because it can only be set here, after `accept`: a client that has already reset + // leaves the socket shut down, `setsockopt` fails with EINVAL, and the pump's first write would + // then raise SIGPIPE and kill the app. Such a client is gone, so there is nothing to forward. + // The slot is released as it goes out of scope. + guard + setsockopt(client, SOL_SOCKET, SO_NOSIGPIPE, &enabled, socklen_t(MemoryLayout.size)) + == 0 + else { + Darwin.close(client) + return + } // What a forward carries is small writes in both directions (chunked HTTP, HMR frames, a wire // protocol); Nagle plus delayed ACK on a loopback hop is latency for nothing. setsockopt(client, IPPROTO_TCP, TCP_NODELAY, &enabled, socklen_t(MemoryLayout.size)) diff --git a/macapp/WorkroomApp/Core/Session/AgentControlClient.swift b/macapp/WorkroomApp/Core/Session/AgentControlClient.swift index 519ad4f58..69c029ceb 100644 --- a/macapp/WorkroomApp/Core/Session/AgentControlClient.swift +++ b/macapp/WorkroomApp/Core/Session/AgentControlClient.swift @@ -159,6 +159,8 @@ struct AgentControlClient: SessionControlPlane { let descriptor = socket(AF_UNIX, SOCK_STREAM, 0) guard descriptor >= 0 else { return nil } defer { close(descriptor) } + // Before `connect`, not after: see `AgentVCSConnection.noSignalOnWrite`. + AgentVCSConnection.noSignalOnWrite(descriptor) var address = sockaddr_un() address.sun_family = sa_family_t(AF_UNIX) diff --git a/macapp/WorkroomApp/Core/Session/PersistentSessionControlClient.swift b/macapp/WorkroomApp/Core/Session/PersistentSessionControlClient.swift index bdf68f55c..099e219d7 100644 --- a/macapp/WorkroomApp/Core/Session/PersistentSessionControlClient.swift +++ b/macapp/WorkroomApp/Core/Session/PersistentSessionControlClient.swift @@ -125,6 +125,9 @@ struct PersistentSessionControlClient { let descriptor = socket(AF_UNIX, SOCK_STREAM, 0) guard descriptor >= 0 else { return .silent } defer { close(descriptor) } + // A daemon that hangs up mid-write must fail the write, not kill the app. Set before `connect`: + // see `AgentVCSConnection.noSignalOnWrite`. + AgentVCSConnection.noSignalOnWrite(descriptor) guard var address = Self.address(for: socketPath) else { return .noListener } let connected = withUnsafePointer(to: &address) { pointer in From b03686578a5c47d57f86ce81133c5050dc181713 Mon Sep 17 00:00:00 2001 From: Joel Moss Date: Thu, 24 Sep 2026 21:24:24 +0100 Subject: [PATCH 3/7] feat(macapp): write to a repository on a remote host Remote writes were refused because two parts of the write path were local-only. Both now reach the host, and every decision stays in the Swift writer (CLIVCSWriter). - The failure classifier's disk: it reads a repository's git directory to tell a parked rebase, a leftover lock or the last fetch. Its probes now read a RepositoryDisk: this Mac's (LocalDisk, as before) or a snapshot the host's agent reports through a new `stat` request, read after the command it explains. A lock file on a host is no longer offered for Reveal in Finder on the Mac. - The jj working-copy barrier: a Mac cannot flock a file on another host, so an exec request names the shared root (`barrier_root`) and the agent takes the barrier around that command, handing it to the child as it does for snapshots. It is per command, where the local gate holds it per operation; the design doc records that ceiling. Reads that cannot snapshot and network commands skip it. The write gate keys a remote repository by its remote location, and BoundLocalWriter binds a remote context only to a host-aware writer. Exec service version 3 carries host_environment, barrier_root and stat, and the app refuses a remote write to an older agent. Co-Authored-By: Claude Opus 5.5 --- docs/designs/remote-workrooms.md | 23 ++ .../WorkroomApp/Core/AgentCommandRunner.swift | 17 +- .../WorkroomApp/Core/AgentVCSConnection.swift | 74 ++++- macapp/WorkroomApp/Core/RepositoryDisk.swift | 63 +++++ .../WorkroomApp/Core/RepositoryServices.swift | 11 +- .../Core/VCSSyncPresentation.swift | 11 +- macapp/WorkroomApp/Core/VCSWriting.swift | 253 ++++++++++++------ .../AgentVCSIntegrationTests.swift | 39 +++ .../AgentVCSProtocolTests.swift | 18 ++ .../RemoteHostIntegrationTests.swift | 51 +++- .../VCSSyncPresentationTests.swift | 10 + vcs/crates/wr-agent/src/vcs.rs | 235 +++++++++++++++- 12 files changed, 681 insertions(+), 124 deletions(-) create mode 100644 macapp/WorkroomApp/Core/RepositoryDisk.swift diff --git a/docs/designs/remote-workrooms.md b/docs/designs/remote-workrooms.md index 53f01a08e..43a164643 100644 --- a/docs/designs/remote-workrooms.md +++ b/docs/designs/remote-workrooms.md @@ -1002,6 +1002,29 @@ these are the subsystems that actually gate "a remote workroom is a real workroo no change. ssh exiting is the connection ending, through the same generation and fail-pending-calls rules as a dead local agent. Terminal panes through the driver are the second half of #229. + **Remote writes (#229)** keep every decision in the Swift writer (`CLIVCSWriter`) and move only + two things to the host: + - *The disk the failure classifier reads.* It decides a parked rebase, a leftover lock or the last + fetch from the repository's git directory, and on a remote host that directory is there. The + agent's `stat` request reports the facts (existence, directory, modification time, a worktree's + `.git` text) for the paths the classifier names, read after the command they explain. + - *The jj working-copy barrier.* Locally the app's gate holds it for a whole operation, and exec + never takes it. A Mac cannot flock a file on another host, so an exec request for a remote jj + repository names its shared root (`barrier_root`) and the agent takes the barrier around that + command. **Known ceiling:** that is per command, where the local gate is per operation, so a + remote snapshot can land between two commands of one write. The upgrade is an + operation-scoped barrier the client acquires and releases, held by the connection and dropped + when it goes. Two kinds of command skip it: + - A read that cannot snapshot (`--ignore-working-copy`), which has nothing to protect. + - A network command (`jj git fetch`/`push`), because the barrier is handed to the child, and + git's detached helpers would inherit it and hold it after jj exits. jj's op log reconciles a + snapshot that runs alongside one. + Both came with exec service version 3, and the app refuses a remote write to an older agent. + **Known residual, wider remotely:** a jj commit tells "committed, then failed" from "failed" by + whether the op head moved, and a snapshot also moves it. A snapshot in the gap between the + before-read and the commit can make a failed commit read as one that landed. Locally the after-read + already sits outside the gate; the per-command barrier widens the gap. The fix is to compare + `@-`, which a snapshot never rewrites, for jj's `.commit` mode. - **Local needs no supervisor at all. DECIDED — and an earlier claim in this document was wrong.** A previous revision asserted that `SessionDaemon.swift:97-107`'s self-exit (`sessions.isEmpty && connections.isEmpty`) had to go, because the agent would also own VCS reads diff --git a/macapp/WorkroomApp/Core/AgentCommandRunner.swift b/macapp/WorkroomApp/Core/AgentCommandRunner.swift index 79d9a4bd1..c3182733a 100644 --- a/macapp/WorkroomApp/Core/AgentCommandRunner.swift +++ b/macapp/WorkroomApp/Core/AgentCommandRunner.swift @@ -10,6 +10,10 @@ import Foundation /// distinction is load-bearing — see `neverRan` and `outcomeUnknown`. struct AgentCommandRunner: StatusCommandRunning, Sendable { let connection: AgentVCSConnection + /// The shared root of a jj repository on a REMOTE host, whose working-copy barrier the agent + /// takes around each command (`barrier_root` in `vcs.rs`). Nil locally, where the caller's gate + /// holds it for the whole operation and the agent must never take it again. + var barrierRoot: String? func run(_ executable: String, _ args: [String], in directory: String, timeout: TimeInterval) async -> CommandResult @@ -57,7 +61,7 @@ struct AgentCommandRunner: StatusCommandRunning, Sendable { } let request = Self.request( executable, args, in: directory, timeout: timeout, stdin: payload, network: network, - host: connection.host) + host: connection.host, barrierRoot: barrierRoot) let reply: Data do { // Slack above the command's own timeout: the round trip and the agent's own bookkeeping @@ -174,7 +178,7 @@ extension AgentCommandRunner { /// pins ssh to fail rather than prompt, as `networkEnvironment` does here. static func request( _ executable: String, _ args: [String], in directory: String, timeout: TimeInterval, - stdin: String?, network: Bool, host: HostID + stdin: String?, network: Bool, host: HostID, barrierRoot: String? = nil ) -> AgentExecRequest { let remote = host != .local return AgentExecRequest( @@ -184,7 +188,12 @@ extension AgentCommandRunner { env: remote ? StatusCommandRunner.remoteEnvironment : StatusCommandRunner.childEnvironment(network: network), - hostEnvironment: remote ? true : nil) + hostEnvironment: remote ? true : nil, + // Not for a network command (`jj git fetch`/`push`): the barrier is handed to the child, and + // git's detached helpers (`git maintenance --auto`, a credential cache daemon) would inherit + // it and hold it long after jj exits. jj's own op log reconciles a snapshot that runs + // alongside one of those. + barrierRoot: remote && !network ? barrierRoot : nil) } } @@ -200,6 +209,8 @@ struct AgentExecRequest: Encodable, Sendable { /// Set only for a remote host, and omitted otherwise: an agent that predates the field rejects /// any request carrying it (`deny_unknown_fields`), and a local agent may be one. var hostEnvironment: Bool? + /// Remote only, for the same reason: see `AgentCommandRunner.barrierRoot`. + var barrierRoot: String? } struct AgentExecResult: Decodable, Sendable { diff --git a/macapp/WorkroomApp/Core/AgentVCSConnection.swift b/macapp/WorkroomApp/Core/AgentVCSConnection.swift index 5184aa319..a1d766a08 100644 --- a/macapp/WorkroomApp/Core/AgentVCSConnection.swift +++ b/macapp/WorkroomApp/Core/AgentVCSConnection.swift @@ -65,6 +65,9 @@ final class AgentVCSConnection: HostServiceConnection, @unchecked Sendable { /// because it gates a FRAMING capability, not the request body: a version-1 agent speaks the same /// `AgentExecRequest` and is fully usable, it just cannot be sent one in pieces. private static let chunkedRequestVersion = 2 + /// The first exec service version a remote host's writes can use: it takes `host_environment` + /// and `barrier_root`, and answers `stat`. + private static let remoteWriteVersion = 3 /// `MAX_ENVELOPE_PAYLOAD` in `protocol/envelope.rs`. private static let maxEnvelopePayload = 1 << 20 /// `MAX_REQUEST` in `vcs.rs` — the reassembled ceiling, mirroring the reply side's. @@ -270,17 +273,15 @@ final class AgentVCSConnection: HostServiceConnection, @unchecked Sendable { func writer(context: RepositoryContext, reader: VCSProviding) throws -> VCSWriting { guard context.location.host == host else { throw HostConnectionError.mismatchedContext } guard lock.withLock({ !closed }) else { throw HostConnectionError.connectionLost } - // ponytail: no writes to a remote host yet. Refused here, by name, because two parts of the write - // path are local-only in ways that would not fail but quietly be wrong. `CLIVCSWriter` classifies - // failures by reading the repository's `.git` on THIS disk (rebase in progress, a leftover - // `index.lock`), which for a remote path is a different machine or nothing at all. And a jj - // write relies on the caller holding the working-copy barrier for the whole operation - // (`JJSnapshotGate`), which a Mac cannot flock on another host, while the agent's exec service - // deliberately never takes it. Upgrade path: host-side probes, and an exec request that asks the - // agent to hold the barrier itself. - guard host == .local else { - throw HostConnectionError.serviceUnavailable( - "Changes to a remote repository are not supported yet.") + // A remote host's writes need what exec version 3 added (#229): the host's own environment, + // the jj barrier taken for each command, and `stat` for the classifier's disk facts. An older + // agent there fails closed rather than writing on Mac-side assumptions. + let remote = host != .local + if remote { + guard let exec = capabilities?.exec, exec >= Self.remoteWriteVersion else { + throw HostConnectionError.serviceUnavailable( + "The host's agent is too old to change a repository there.") + } } // Presence and version of the SERVICE, not a count of the caller's own methods. `>=` rather // than `==` so an agent that gains a version 3 does not refuse a client speaking 1 — the agent @@ -297,12 +298,40 @@ final class AgentVCSConnection: HostServiceConnection, @unchecked Sendable { guard let exec = capabilities?.exec, exec >= Self.execVersion else { throw VCSError.backendVersion("Agent does not support VCS writes.") } - let engine = CLIVCSWriter( - vcs: context.backend.rawValue, runner: AgentCommandRunner(connection: self), + // A Mac cannot flock the jj barrier on another host, so there the agent takes it for each + // command, under the shared root the local gate would have locked. + let barrierRoot = remote && context.backend == .jj ? try context.requireOwnership().path : nil + var engine = CLIVCSWriter( + vcs: context.backend.rawValue, + runner: AgentCommandRunner(connection: self, barrierRoot: barrierRoot), makeProvider: { _ in AgentCurrentRefProvider(reader: reader) }, gate: .shared) + if remote { + engine.host = host + engine.stat = { [self] paths, read in try await stat(paths: paths, read: read) } + } return try BoundLocalWriter(context: context, reader: reader, writer: engine) } + /// Facts from the host's disk for `CLIVCSWriter`'s failure classifier (`stat` in `vcs.rs`): + /// whether each path exists, is a directory and when it changed, and the text of those in `read`. + func stat(paths: [String], read: [String]) async throws -> DiskSnapshot { + let reply = try await request(AgentStatRequest(paths: paths, read: read), timeout: 15) + var entries: [String: DiskEntry?] = [:] + var texts: [String: String] = [:] + for entry in try AgentVCSReply.decode(reply).entries { + let found = + entry.exists + ? DiskEntry( + isDirectory: entry.isDir, + modifiedAt: entry.modifiedAt.map { Date(timeIntervalSince1970: $0) }) + : nil + // `.some(nil)`: asked about and absent, which a snapshot tells apart from never asked. + entries[entry.path] = .some(found) + if let text = entry.text { texts[entry.path] = text } + } + return DiskSnapshot(entries: entries, texts: texts) + } + /// Probe the File service, if the peer's greeting says it exists. Runs once in `connect()`, before /// this connection is shared, and NEVER fails it: an agent that predates the service, or one whose /// probe is unanswered or unreadable, leaves VCS fully working and just has no file service — @@ -868,6 +897,25 @@ struct AgentVCSCapabilities: Decodable { let exec: Int? } +/// `StatRequest` in `vcs.rs`, which rejects unknown fields. +struct AgentStatRequest: Encodable, Sendable { + var version = 1 + var kind = "stat" + let paths: [String] + let read: [String] +} + +struct AgentStatReply: Decodable { + struct Entry: Decodable { + let path: String + let exists: Bool + let isDir: Bool + let modifiedAt: Double? + let text: String? + } + let entries: [Entry] +} + struct AgentVCSRequest: Encodable, Sendable { var version = 1 var root: String? diff --git a/macapp/WorkroomApp/Core/RepositoryDisk.swift b/macapp/WorkroomApp/Core/RepositoryDisk.swift new file mode 100644 index 000000000..9215270ed --- /dev/null +++ b/macapp/WorkroomApp/Core/RepositoryDisk.swift @@ -0,0 +1,63 @@ +import Foundation + +/// What `CLIVCSWriter`'s failure classifier reads from a repository's disk: whether a path exists, +/// whether it is a directory, when it was modified, and the text of a small file (a worktree's +/// `.git`). Every decision stays in the classifier; only where these facts come from changes. For a +/// local repository they are this Mac's (`LocalDisk`); for one on a remote host, what that host's +/// agent reported (`DiskSnapshot`, from `AgentVCSConnection.stat`), since reading this disk for a +/// path on another machine answers nothing, or worse, the wrong thing. +protocol RepositoryDisk: Sendable { + func entry(_ path: String) -> DiskEntry? + func text(_ path: String) -> String? +} + +struct DiskEntry: Equatable, Sendable { + let isDirectory: Bool + let modifiedAt: Date? +} + +/// This Mac's disk, read live, exactly as the classifier always read it. +struct LocalDisk: RepositoryDisk { + func entry(_ path: String) -> DiskEntry? { + var isDirectory: ObjCBool = false + guard FileManager.default.fileExists(atPath: path, isDirectory: &isDirectory) else { + return nil + } + let modified = + (try? FileManager.default.attributesOfItem(atPath: path))?[.modificationDate] as? Date + return DiskEntry(isDirectory: isDirectory.boolValue, modifiedAt: modified) + } + + func text(_ path: String) -> String? { try? String(contentsOfFile: path, encoding: .utf8) } +} + +/// Facts a remote host reported for a fixed set of paths, read after the command they explain. +/// +/// A path the snapshot was not asked about is a bug in `CLIVCSWriter.classificationPaths`, not a +/// missing file, so it traps in debug builds rather than quietly reading as absent. +struct DiskSnapshot: RepositoryDisk { + let entries: [String: DiskEntry?] + let texts: [String: String] + + /// For a host that could not be asked: every path reads as absent, which is the classifier's + /// "no evidence" answer (no parked rebase, no lock file to name). + static let unknown = DiskSnapshot(entries: [:], texts: [:], complete: false) + + private let complete: Bool + + init(entries: [String: DiskEntry?], texts: [String: String], complete: Bool = true) { + self.entries = entries + self.texts = texts + self.complete = complete + } + + func entry(_ path: String) -> DiskEntry? { + guard let known = entries[path] else { + assert(!complete, "\(path) was not in the snapshot") + return nil + } + return known + } + + func text(_ path: String) -> String? { texts[path] } +} diff --git a/macapp/WorkroomApp/Core/RepositoryServices.swift b/macapp/WorkroomApp/Core/RepositoryServices.swift index 8fc4bfae6..28e690a89 100644 --- a/macapp/WorkroomApp/Core/RepositoryServices.swift +++ b/macapp/WorkroomApp/Core/RepositoryServices.swift @@ -90,8 +90,17 @@ struct BoundLocalWriter: VCSWriting { private var path: String { context.location.path } private let projectRoot: String + /// For a local repository, or a remote one whose writer reads that host's disk and holds its + /// barrier through the agent (`AgentVCSConnection.writer`). The paths here are only ever handed to + /// the writer as strings, never resolved against this disk. init(context: RepositoryContext, reader: VCSProviding, writer: LocalVCSWriting) throws { - _ = try context.location.requireLocalURL() + // A remote repository only with a writer that knows it is one; anything else would run git on + // this Mac against a path on another machine. + if context.location.host != .local { + guard let engine = writer as? CLIVCSWriter, engine.host == context.location.host else { + throw RepositoryRoutingError.unavailable(context.location.host) + } + } projectRoot = try context.requireOwnership().path self.context = context self.reader = reader diff --git a/macapp/WorkroomApp/Core/VCSSyncPresentation.swift b/macapp/WorkroomApp/Core/VCSSyncPresentation.swift index f76b85256..2d1b8fa38 100644 --- a/macapp/WorkroomApp/Core/VCSSyncPresentation.swift +++ b/macapp/WorkroomApp/Core/VCSSyncPresentation.swift @@ -644,9 +644,12 @@ enum VCSSyncPresenter { } /// The blocking lock file's path, when the failure has one. + /// Nil for a lock file on a remote host too: both things offered for the path, Reveal in Finder + /// and Copy Lock File Path, are for a file on this Mac. git's own message, in the failure's + /// details, still names it. static func lockPath(of failure: VCSRemoteFailure) -> String? { - guard case .locked(let file) = failure else { return nil } - return file?.path + guard case .locked(let file) = failure, let file, file.isOnThisMac else { return nil } + return file.path } /// The tooltip form of a failure: `describe`'s one-liner plus, where there is one, the remedy. @@ -696,8 +699,8 @@ enum VCSSyncPresenter { details: commitRawOutput(of: failure), recovery: nil, lockPath: { - guard case .locked(let file) = failure else { return nil } - return file?.path + guard case .locked(let file) = failure, let file, file.isOnThisMac else { return nil } + return file.path }()) } diff --git a/macapp/WorkroomApp/Core/VCSWriting.swift b/macapp/WorkroomApp/Core/VCSWriting.swift index 7755377c8..180958b75 100644 --- a/macapp/WorkroomApp/Core/VCSWriting.swift +++ b/macapp/WorkroomApp/Core/VCSWriting.swift @@ -139,6 +139,9 @@ struct VCSFailureReport: Identifiable, Equatable, Sendable { struct VCSLockFile: Equatable, Sendable { let path: String let modifiedAt: Date + /// False for a lock file on a remote host: its path names nothing on this Mac, so there is nothing + /// here to reveal in Finder (`VCSSyncPresenter.lockPath`). + var isOnThisMac = true var filename: String { (path as NSString).lastPathComponent } } @@ -384,6 +387,12 @@ struct CLIVCSWriter: LocalVCSWriting, Sendable { /// `git worktree add` worktrees sharing one `.git`, so a lock lost mid-`pull --rebase` can leave a /// workroom wedged in a rebase. let gate: JJSnapshotGate + /// The host the repository is on. It keys the write gate (`gated`), so a remote path is never + /// resolved against this disk, nor shares a queue with a local repository at the same path. + var host: HostID = .local + /// Where the classifier's disk facts come from for a repository on a remote host: that host's + /// agent (`AgentVCSConnection.stat`). Nil for a local repository, which reads this Mac's disk. + var stat: (@Sendable (_ paths: [String], _ read: [String]) async throws -> DiskSnapshot)? var refTimeout: TimeInterval = 5 var fetchTimeout: TimeInterval = 120 @@ -441,15 +450,12 @@ struct CLIVCSWriter: LocalVCSWriting, Sendable { /// /// Pure string work, no subprocess: `git rev-parse --git-common-dir` is authoritative but costs a /// process for something two file reads answer. - static func commonGitDir(at path: String) -> URL? { + static func commonGitDir(at path: String, disk: any RepositoryDisk = LocalDisk()) -> URL? { let dotGit = URL(fileURLWithPath: path, isDirectory: true).appendingPathComponent(".git") - var isDir: ObjCBool = false - guard FileManager.default.fileExists(atPath: dotGit.path, isDirectory: &isDir) else { - return nil - } - if isDir.boolValue { return dotGit } + guard let entry = disk.entry(dotGit.path) else { return nil } + if entry.isDirectory { return dotGit } // A worktree's `.git` file: `gitdir: /abs/or/relative/path`. - guard let contents = try? String(contentsOf: dotGit, encoding: .utf8) else { return nil } + guard let contents = disk.text(dotGit.path) else { return nil } let line = contents.split(whereSeparator: \.isNewline).first.map(String.init) ?? "" guard line.hasPrefix("gitdir:") else { return nil } let raw = String(line.dropFirst("gitdir:".count)).trimmingCharacters(in: .whitespaces) @@ -466,14 +472,13 @@ struct CLIVCSWriter: LocalVCSWriting, Sendable { /// `FETCH_HEAD`'s modification time. git rewrites it on **every** fetch, no-ops included (verified), /// so its mtime is a complete record of git fetches — including one the user ran in a terminal. - static func gitLastFetch(commonGitDir: URL?) -> VCSLastFetch { + static func gitLastFetch(commonGitDir: URL?, disk: any RepositoryDisk = LocalDisk()) + -> VCSLastFetch + { guard let dir = commonGitDir else { return .unknown } let head = dir.appendingPathComponent("FETCH_HEAD") - guard FileManager.default.fileExists(atPath: head.path) else { return .never } - guard - let attrs = try? FileManager.default.attributesOfItem(atPath: head.path), - let date = attrs[.modificationDate] as? Date - else { return .unknown } + guard let entry = disk.entry(head.path) else { return .never } + guard let date = entry.modifiedAt else { return .unknown } return .at(date) } @@ -1155,7 +1160,7 @@ struct CLIVCSWriter: LocalVCSWriting, Sendable { /// that state must offer Abort, not Retry. static func classify( _ result: CommandResult, action: VCSRemoteAction, tool: String, - gitDir: URL? = nil + gitDir: URL? = nil, disk: any RepositoryDisk = LocalDisk() ) -> VCSRemoteFailure? { // Checked BEFORE commandNotFound: launchFailed means the process never ran at all (dominated by // a vanished cwd), which is a different fact from commandNotFound's "env ran and searched PATH". @@ -1176,14 +1181,14 @@ struct CLIVCSWriter: LocalVCSWriting, Sendable { // It races a host-side git that is still rebasing, and that is accepted here for the reason the // `timedOut` branch already accepts it: a parked rebase the user cannot see is the worse state. if result.exitCode == CommandResult.outcomeUnknown { - if action == .pull, rebaseInProgress(gitDir: gitDir) { return .rebaseInProgress } + if action == .pull, rebaseInProgress(gitDir: gitDir, disk: disk) { return .rebaseInProgress } return .outcomeUnknown(result.stderr.trimmingCharacters(in: .whitespacesAndNewlines)) } if result.exitCode == CommandResult.commandNotFound { return .toolMissing(tool) } let err = result.stderr + "\n" + result.stdout // A timed-out pull may have left a rebase behind; that reads better than "timed out". if result.timedOut { - if action == .pull, rebaseInProgress(gitDir: gitDir) { return .rebaseInProgress } + if action == .pull, rebaseInProgress(gitDir: gitDir, disk: disk) { return .rebaseInProgress } return .timedOut(action, err) } guard !result.ok else { return nil } @@ -1224,9 +1229,9 @@ struct CLIVCSWriter: LocalVCSWriting, Sendable { && (err.contains("could not be obtained") || err.contains("File exists") || err.contains("Unable to create"))) { - return .locked(lockFile(in: err)) + return .locked(lockFile(in: err, disk: disk)) } - if action == .pull, rebaseInProgress(gitDir: gitDir) { return .rebaseInProgress } + if action == .pull, rebaseInProgress(gitDir: gitDir, disk: disk) { return .rebaseInProgress } // A lock failure that never names a lock. git's message depends on WHICH internal step hit the lock: // a fast-forward pull reports `Unable to create '': File exists.`, but a pull that must really // rebase fails in autostash first and says only `error: could not write index` / `fatal: Cannot @@ -1236,7 +1241,7 @@ struct CLIVCSWriter: LocalVCSWriting, Sendable { // So when the symptoms are lock-shaped, ask the DISK instead of the message. Deliberately last: any // failure git explains properly keeps its own classification, and this only speaks for the ones it // doesn't. - if lockSymptom(err), let lock = existingLockFile(gitDir: gitDir) { return .locked(lock) } + if lockSymptom(err), let lock = existingLockFile(gitDir: gitDir, disk: disk) { return .locked(lock) } let trimmed = err.trimmingCharacters(in: .whitespacesAndNewlines) // Killed rather than finished, with nothing to say for itself: `exitCode` is the SIGNAL number, // so the fallback below would render "git exited 15" — literally the dialog `a64e4269` ("stop @@ -1306,7 +1311,9 @@ struct CLIVCSWriter: LocalVCSWriting, Sendable { /// commit. The reachable case is a `post-commit` hook: git has already written the commit and /// moved `HEAD` by the time it runs, so a hook that fails or runs past the timeout leaves a /// perfectly good commit behind a non-zero exit. - static func classifyCommit(_ result: CommandResult, tool: String) -> VCSCommitFailure? { + static func classifyCommit( + _ result: CommandResult, tool: String, disk: any RepositoryDisk = LocalDisk() + ) -> VCSCommitFailure? { if result.exitCode == CommandResult.launchFailed { return .launchFailed } // See `classify`. if result.exitCode == CommandResult.refused { return .other(result.stderr) } @@ -1347,7 +1354,7 @@ struct CLIVCSWriter: LocalVCSWriting, Sendable { && (err.contains("could not be obtained") || err.contains("File exists") || err.contains("Unable to create"))) { - return .locked(lockFile(in: err)) + return .locked(lockFile(in: err, disk: disk)) } let trimmed = err.trimmingCharacters(in: .whitespacesAndNewlines) // Same reasoning as `classify`: a signal number is not an exit status, so don't print it as one. @@ -1357,11 +1364,10 @@ struct CLIVCSWriter: LocalVCSWriting, Sendable { /// Whether a rebase is parked in this worktree. Both directory names are checked: `rebase-merge` for /// an interactive/merge rebase, `rebase-apply` for the am-based one. - static func rebaseInProgress(gitDir: URL?) -> Bool { + static func rebaseInProgress(gitDir: URL?, disk: any RepositoryDisk = LocalDisk()) -> Bool { guard let gitDir else { return false } - let fm = FileManager.default - return fm.fileExists(atPath: gitDir.appendingPathComponent("rebase-merge").path) - || fm.fileExists(atPath: gitDir.appendingPathComponent("rebase-apply").path) + return disk.entry(gitDir.appendingPathComponent("rebase-merge").path) != nil + || disk.entry(gitDir.appendingPathComponent("rebase-apply").path) != nil } /// Which multi-step git operation is parked in this worktree, named for the user, or nil if none. @@ -1373,23 +1379,23 @@ struct CLIVCSWriter: LocalVCSWriting, Sendable { /// /// Ordered by specificity: a cherry-pick and a revert both also leave `MERGE_HEAD` behind, so they /// are tested first or every one of them would report "merge". - static func sequencerState(gitDir: URL?) -> String? { + static func sequencerState(gitDir: URL?, disk: any RepositoryDisk = LocalDisk()) -> String? { guard let gitDir else { return nil } - let fm = FileManager.default - let markers: [(String, String)] = [ + for (file, label) in sequencerMarkers + where disk.entry(gitDir.appendingPathComponent(file).path) != nil { + return label + } + return nil + } + + static let sequencerMarkers: [(String, String)] = [ ("CHERRY_PICK_HEAD", "cherry-pick"), ("REVERT_HEAD", "revert"), ("rebase-merge", "rebase"), ("rebase-apply", "rebase"), ("BISECT_LOG", "bisect"), ("MERGE_HEAD", "merge"), - ] - for (file, label) in markers - where fm.fileExists(atPath: gitDir.appendingPathComponent(file).path) { - return label - } - return nil - } + ] /// The lock file a failure is complaining about, or nil if we can't point at one. /// @@ -1401,12 +1407,11 @@ struct CLIVCSWriter: LocalVCSWriting, Sendable { /// Returns nil for jj's import/export lock, whose message carries no path, and — deliberately — when /// the named file no longer exists: a lock that cleared between the failure and this check WAS /// transient contention, which is exactly the case where Retry is the right offer. - static func lockFile(in stderr: String) -> VCSLockFile? { - guard let path = parseLockPath(stderr) else { return nil } - guard let attrs = try? FileManager.default.attributesOfItem(atPath: path), - let modified = attrs[.modificationDate] as? Date - else { return nil } - return VCSLockFile(path: path, modifiedAt: modified) + static func lockFile(in stderr: String, disk: any RepositoryDisk = LocalDisk()) -> VCSLockFile? { + guard let path = parseLockPath(stderr), let modified = disk.entry(path)?.modifiedAt else { + return nil + } + return VCSLockFile(path: path, modifiedAt: modified, isOnThisMac: disk is LocalDisk) } /// Whether a failure LOOKS like it was caused by a lock, without saying so. @@ -1426,25 +1431,43 @@ struct CLIVCSWriter: LocalVCSWriting, Sendable { /// The first lock file actually present in this repo, checking both the worktree's git dir and the /// common one — a workroom is a `git worktree`, so its `index.lock` and the repo's `packed-refs.lock` /// are in different directories. - static func existingLockFile(gitDir: URL?) -> VCSLockFile? { - guard let gitDir else { return nil } - let fm = FileManager.default + static func existingLockFile(gitDir: URL?, disk: any RepositoryDisk = LocalDisk()) + -> VCSLockFile? + { + for candidate in lockCandidates(gitDir: gitDir) { + guard let modified = disk.entry(candidate.path)?.modifiedAt else { continue } + return VCSLockFile( + path: candidate.path, modifiedAt: modified, isOnThisMac: disk is LocalDisk) + } + return nil + } + + /// Where a lock file can be, in the order `existingLockFile` checks. + static func lockCandidates(gitDir: URL?) -> [URL] { + guard let gitDir else { return [] } var dirs = [gitDir] // `/worktrees/` → ``. Cheap and string-only; `commonGitDir` does the same trip // from a path rather than from an already-resolved git dir. if gitDir.deletingLastPathComponent().lastPathComponent == "worktrees" { dirs.append(gitDir.deletingLastPathComponent().deletingLastPathComponent()) } - for dir in dirs { - for name in knownLockNames { - let candidate = dir.appendingPathComponent(name) - guard let attrs = try? fm.attributesOfItem(atPath: candidate.path), - let modified = attrs[.modificationDate] as? Date - else { continue } - return VCSLockFile(path: candidate.path, modifiedAt: modified) - } + return dirs.flatMap { dir in knownLockNames.map { dir.appendingPathComponent($0) } } + } + + /// Every path the classifier can read for a command run in the worktree whose git directories + /// these are, and whose stderr this is: the set a remote host's `stat` must report, so a + /// `DiskSnapshot` answers each probe above. A probe that reads a path not listed here traps in + /// a debug build. + static func classificationPaths(gitDir: URL?, commonGitDir: URL?, stderr: String) -> [String] { + var paths: [URL] = lockCandidates(gitDir: gitDir) + if let gitDir { + paths += sequencerMarkers.map { gitDir.appendingPathComponent($0.0) } } - return nil + if let commonGitDir { paths.append(commonGitDir.appendingPathComponent("FETCH_HEAD")) } + var unique = paths.map(\.path) + if let lock = parseLockPath(stderr) { unique.append(lock) } + var seen = Set() + return unique.filter { seen.insert($0).inserted } } /// The quoted path out of git's lock errors. Pure, so the parsing is testable without a repo. @@ -1466,14 +1489,11 @@ struct CLIVCSWriter: LocalVCSWriting, Sendable { /// This worktree's OWN git directory — `/worktrees/` for a workroom, the same as the /// common dir for a project root. Rebase state (`rebase-merge`), `HEAD` and `index` live here, unlike /// `FETCH_HEAD` which `commonGitDir` deliberately resolves to the shared copy. - static func worktreeGitDir(at path: String) -> URL? { + static func worktreeGitDir(at path: String, disk: any RepositoryDisk = LocalDisk()) -> URL? { let dotGit = URL(fileURLWithPath: path, isDirectory: true).appendingPathComponent(".git") - var isDir: ObjCBool = false - guard FileManager.default.fileExists(atPath: dotGit.path, isDirectory: &isDir) else { - return nil - } - if isDir.boolValue { return dotGit } - guard let contents = try? String(contentsOf: dotGit, encoding: .utf8) else { return nil } + guard let entry = disk.entry(dotGit.path) else { return nil } + if entry.isDirectory { return dotGit } + guard let contents = disk.text(dotGit.path) else { return nil } let line = contents.split(whereSeparator: \.isNewline).first.map(String.init) ?? "" guard line.hasPrefix("gitdir:") else { return nil } let raw = String(line.dropFirst("gitdir:".count)).trimmingCharacters(in: .whitespaces) @@ -1504,7 +1524,72 @@ struct CLIVCSWriter: LocalVCSWriting, Sendable { private func gated( _ projectRoot: String, _ body: @Sendable @escaping () async -> T ) async -> T? { - try? await gate.run(projectRoot: projectRoot, body) + guard case .remote(let id) = host else { + return try? await gate.run(projectRoot: projectRoot, body) + } + // Keyed as the remote location it is: validated, not resolved against this disk. The gate takes + // no process barrier for it, having none to take on another host (`JJProcessBarrier`); the + // agent takes jj's for each command instead (`AgentCommandRunner.barrierRoot`). + guard let repository = try? RepositoryLocation.remote(host: id, path: projectRoot) else { + return nil + } + return try? await gate.run(repository: repository, body) + } + + // MARK: - Disk facts + + /// The disk a classification reads, and the worktree's git directory on it, read AFTER the command + /// it explains: a parked rebase or a leftover lock exists only once that command has failed. + /// + /// Locally, this Mac's disk, live. On a remote host, two round trips to its agent: the worktree's + /// `.git` says where its git directories are, then everything in them at once. A host that cannot + /// be asked reads as no evidence (`DiskSnapshot.unknown`), as a missing file would. + /// + /// The git directories come from the FIRST read, and are what the second was built for: resolved + /// again from the second, a `.git` that changed in between would name paths it never asked about. + private func disk(at path: String, stderr: String = "") async -> ( + disk: any RepositoryDisk, gitDir: URL?, commonGitDir: URL? + ) { + guard let stat else { + return (LocalDisk(), Self.worktreeGitDir(at: path), Self.commonGitDir(at: path)) + } + let dotGit = URL(fileURLWithPath: path, isDirectory: true).appendingPathComponent(".git").path + guard let pointer = try? await stat([dotGit], [dotGit]) else { + return (DiskSnapshot.unknown, nil, nil) + } + let gitDir = Self.worktreeGitDir(at: path, disk: pointer) + let commonGitDir = Self.commonGitDir(at: path, disk: pointer) + let paths = + [dotGit] + + Self.classificationPaths(gitDir: gitDir, commonGitDir: commonGitDir, stderr: stderr) + guard let facts = try? await stat(paths, [dotGit]) else { + return (DiskSnapshot.unknown, gitDir, commonGitDir) + } + return (facts, gitDir, commonGitDir) + } + + private func rebaseParked(at path: String) async -> Bool { + let facts = await disk(at: path) + return Self.rebaseInProgress(gitDir: facts.gitDir, disk: facts.disk) + } + + private func parkedOperation(at path: String) async -> String? { + let facts = await disk(at: path) + return Self.sequencerState(gitDir: facts.gitDir, disk: facts.disk) + } + + /// `Self.classify`, reading the disk of wherever this repository lives. `withGitDir: false` for a + /// command whose classification never consulted the worktree's git directory. + private func classify( + _ result: CommandResult, action: VCSRemoteAction, tool: String, at path: String, + withGitDir: Bool = true + ) async -> VCSRemoteFailure? { + // Nothing on disk explains a success, so a remote host is not asked about one. + if result.ok { return Self.classify(result, action: action, tool: tool) } + let facts = await disk(at: path, stderr: result.stderr + "\n" + result.stdout) + return Self.classify( + result, action: action, tool: tool, gitDir: withGitDir ? facts.gitDir : nil, + disk: facts.disk) } // MARK: - Remote state @@ -1524,7 +1609,9 @@ struct CLIVCSWriter: LocalVCSWriting, Sendable { private func gitRemoteState(path: String, current: VCSRef) async -> VCSRemoteResolution { let refs = await run(Self.gitRemoteRefsArgs(), in: path, timeout: refTimeout) - if let failure = Self.classify(refs, action: .fetch, tool: "git") { + if let failure = await classify( + refs, action: .fetch, tool: "git", at: path, withGitDir: false) + { // A blip must not blank a good toolbar; a missing tool or a real error should show. if case .timedOut = failure { return .keepPrior } return .failed(failure) @@ -1553,10 +1640,11 @@ struct CLIVCSWriter: LocalVCSWriting, Sendable { tracking = VCSTracking(comparedTo: counterpart, ahead: nil, behind: nil, gone: true) } } + let facts = await disk(at: path) return .state( VCSRemoteState( current: current, tracking: tracking, remotes: remotes, primaryRemote: primary, - lastFetch: Self.gitLastFetch(commonGitDir: Self.commonGitDir(at: path)), + lastFetch: Self.gitLastFetch(commonGitDir: facts.commonGitDir, disk: facts.disk), resolvedAt: Date())) } @@ -1565,7 +1653,8 @@ struct CLIVCSWriter: LocalVCSWriting, Sendable { { // Reads take no lock, so they stay ungated. let list = await run(Self.jjBookmarkListArgs(), in: path, timeout: refTimeout) - if let failure = Self.classify(list, action: .fetch, tool: "jj") { + if let failure = await classify(list, action: .fetch, tool: "jj", at: path, withGitDir: false) + { if case .timedOut = failure { return .keepPrior } return .failed(failure) } @@ -1663,9 +1752,7 @@ struct CLIVCSWriter: LocalVCSWriting, Sendable { await run(args, in: dir, timeout: fetchTimeout, network: true) }) else { return .failed(.other("fetch was cancelled")) } - if let failure = Self.classify( - result, action: .fetch, tool: vcs, gitDir: Self.worktreeGitDir(at: path)) - { + if let failure = await classify(result, action: .fetch, tool: vcs, at: path) { return .failed(failure) } return .ok(summary: "Fetched \(remote)") @@ -1697,9 +1784,7 @@ struct CLIVCSWriter: LocalVCSWriting, Sendable { await run(args, in: dir, timeout: pushTimeout, network: true) }) else { return .failed(.other("push was cancelled")) } - if let failure = Self.classify( - result, action: .push, tool: vcs, gitDir: Self.worktreeGitDir(at: path)) - { + if let failure = await classify(result, action: .push, tool: vcs, at: path) { return .failed(failure) } return .ok(summary: "Pushed to \(remote)") @@ -1712,8 +1797,6 @@ struct CLIVCSWriter: LocalVCSWriting, Sendable { // Both backends fetch first, at the project root, for the reasons `opDirectory` documents. For git // `pull` would fetch too, but doing it explicitly at the root keeps `FETCH_HEAD` — and so the // "last fetched" label — correct for every workroom of the project. - // Hoisted above the fetch: BOTH steps classify against it, since a lock can block either one. - let gitDir = Self.worktreeGitDir(at: path) let fetchDir = Self.opDirectory(.fetch, path: path, projectRoot: projectRoot) let fetchArgs = vcs == "jj" ? Self.jjFetchArgs(remote: remote) : Self.gitFetchArgs(remote: remote) @@ -1724,7 +1807,8 @@ struct CLIVCSWriter: LocalVCSWriting, Sendable { await run(fetchArgs, in: fetchDir, timeout: fetchTimeout, network: true) }) else { return .failed(.other("pull was cancelled")) } - if let failure = Self.classify(fetched, action: .pull, tool: vcs, gitDir: gitDir) { + // Both steps classify against the worktree's git directory, since a lock can block either one. + if let failure = await classify(fetched, action: .pull, tool: vcs, at: path) { return .failed(failure) } @@ -1750,7 +1834,7 @@ struct CLIVCSWriter: LocalVCSWriting, Sendable { await run(args, in: dir, timeout: pullTimeout, network: true) }) else { return .failed(.other("pull was cancelled")) } - if let failure = Self.classify(result, action: .pull, tool: vcs, gitDir: gitDir) { + if let failure = await classify(result, action: .pull, tool: vcs, at: path) { return .failed(failure) } return .ok(summary: "Pulled from \(remote)") @@ -1766,7 +1850,7 @@ struct CLIVCSWriter: LocalVCSWriting, Sendable { } func abortRebase(path: String, projectRoot: String) async -> VCSRemoteActionResult { - if vcs == "jj", !Self.rebaseInProgress(gitDir: Self.worktreeGitDir(at: path)) { + if vcs == "jj", !(await rebaseParked(at: path)) { // jj's own rebase is atomic and undoable — there is no half-finished state to abort. But a // COLOCATED jj root still has a real `.git`, and a `git rebase` run in that root's terminal // can leave a real `rebase-merge`/`rebase-apply` behind — exactly what `classify` checks for @@ -1790,7 +1874,9 @@ struct CLIVCSWriter: LocalVCSWriting, Sendable { else { return .failed(.other("abort was cancelled")) } // Always "git" here too, never `vcs`, for the same reason: attributing a failure to the wrong // tool would misdirect ".toolMissing"/"exited N" messages. - if let failure = Self.classify(result, action: .abortRebase, tool: "git") { + if let failure = await classify( + result, action: .abortRebase, tool: "git", at: path, withGitDir: false) + { return .failed(failure) } return .ok(summary: "Rebase aborted") @@ -1801,10 +1887,9 @@ struct CLIVCSWriter: LocalVCSWriting, Sendable { func commit(path: String, projectRoot: String, request: VCSCommitRequest) async -> VCSCommitResult { guard Self.supports(mode: request.mode, vcs: vcs) else { return .failed(.unsupportedMode) } - let gitDir = Self.worktreeGitDir(at: path) // A parked merge/cherry-pick/rebase/bisect makes a path-limited commit outright invalid, and // finishing it is the user's call. Checked before the ref snapshot so nothing is spawned at all. - if vcs != "jj", let sequencer = Self.sequencerState(gitDir: gitDir) { + if vcs != "jj", let sequencer = await parkedOperation(at: path) { return .failed(.sequencerInProgress(sequencer)) } @@ -1828,7 +1913,15 @@ struct CLIVCSWriter: LocalVCSWriting, Sendable { guard let attempt = outcome else { return .failed(.other("commit was cancelled")) } let (before, result) = (attempt.before, attempt.result) - if let failure = Self.classifyCommit(result, tool: vcs) { + // Read only for a failure, as `classify` does: jj's "Nothing changed." exits 0 and is classified + // here too, but no probe reads the disk for it. + let commitDisk: any RepositoryDisk = + stat == nil + ? LocalDisk() + : result.ok + ? DiskSnapshot.unknown + : await disk(at: path, stderr: result.stderr + "\n" + result.stdout).disk + if let failure = Self.classifyCommit(result, tool: vcs, disk: commitDisk) { // The command failed, but did the ref move anyway? A `post-commit` hook runs AFTER git has // written the commit and moved HEAD, so a hook that fails — or that we killed at the timeout — // leaves a real commit behind a non-zero exit. Reporting that as a plain failure invites a @@ -2012,7 +2105,7 @@ struct CLIVCSWriter: LocalVCSWriting, Sendable { let subject = head.ok ? head.stdout.trimmingCharacters(in: .whitespacesAndNewlines) : "" return VCSCommitPreflight( - sequencer: Self.sequencerState(gitDir: Self.worktreeGitDir(at: path)), + sequencer: await parkedOperation(at: path), // Empty on an unborn branch — a repo with no commits has nothing to amend. amendTarget: subject.isEmpty ? nil : subject, currentMessage: nil) diff --git a/macapp/WorkroomAppTests/AgentVCSIntegrationTests.swift b/macapp/WorkroomAppTests/AgentVCSIntegrationTests.swift index ffd1126df..19e035252 100644 --- a/macapp/WorkroomAppTests/AgentVCSIntegrationTests.swift +++ b/macapp/WorkroomAppTests/AgentVCSIntegrationTests.swift @@ -555,6 +555,45 @@ final class AgentVCSIntegrationTests: XCTestCase { /// `exec_never_contends_with_a_held_snapshot_barrier` for the Rust-side proof of the same /// property. If this ever regressed, the commit below would hang for up to 30s and fail the test /// on timeout instead of completing. + /// A remote host's jj writes run under the working-copy barrier, which the agent takes around each + /// command because a Mac cannot flock a file on another host (#229). Driven through a local agent + /// under a remote host id, which takes that path end to end: while this test holds the barrier the + /// commit waits, and it lands once the barrier is released. + func testARemoteJJCommitWaitsOnTheBarrierTheAgentTakes() async throws { + let root = try root() + try run("jj", ["git", "init", "--colocate"], at: root) + try "base\n".write(to: root.appendingPathComponent("file"), atomically: true, encoding: .utf8) + try run("jj", ["commit", "-m", "initial"], at: root) + try "changed\n".write( + to: root.appendingPathComponent("file"), atomically: true, encoding: .utf8) + let hostID = UUID() + let host = HostID.remote(hostID) + let manager = HostConnectionManager() + let connection = try await connect(host: host) + _ = try await manager.connect(host: host) { connection } + let router = RepositoryRouter(connections: manager) + let location = try RepositoryLocation.remote(host: hostID, path: root.path) + try router.register(.init(location: location, backend: .jj, sharedLocation: location)) + let writer = try await router.writer(for: location) + + let local = try await RepositoryLocation.local(root.path) + let held = try XCTUnwrap(JJProcessBarrier.acquire(local)) + let release = Task { + try? await Task.sleep(for: .milliseconds(1500)) + held.release() + } + let started = ContinuousClock.now + let result = await writer.commit( + request: VCSCommitRequest(message: "remote jj commit", files: [], mode: .commit)) + let waited = ContinuousClock.now - started + await release.value + guard case .ok = result else { return XCTFail("jj commit failed: \(result)") } + XCTAssertGreaterThanOrEqual(waited, .milliseconds(1200), "the commit ran without the barrier") + let log = try run("jj", ["log", "--no-graph", "-r", "@-", "-T", "description"], at: root) + XCTAssertTrue(log.contains("remote jj commit"), log) + await connection.close() + } + func testJJCommitRoutesThroughTheAgentWithoutContendingItsOwnSnapshotBarrier() async throws { let root = try root() try run("jj", ["git", "init", "--colocate"], at: root) diff --git a/macapp/WorkroomAppTests/AgentVCSProtocolTests.swift b/macapp/WorkroomAppTests/AgentVCSProtocolTests.swift index dddf5ba32..fb1a18142 100644 --- a/macapp/WorkroomAppTests/AgentVCSProtocolTests.swift +++ b/macapp/WorkroomAppTests/AgentVCSProtocolTests.swift @@ -285,6 +285,24 @@ final class AgentVCSProtocolTests: XCTestCase { } XCTAssertTrue(wire.contains("\"host_environment\":true"), wire) } + // The jj barrier the agent takes for a remote command must never be asked of a local agent, + // whose caller already holds it for the whole operation: taken again, it would deadlock. + let local = AgentCommandRunner.request( + "jj", ["commit"], in: "/r", timeout: 1, stdin: nil, network: false, host: .local, + barrierRoot: "/r") + XCTAssertFalse( + String(decoding: try encoder.encode(local), as: UTF8.self).contains("barrier_root")) + let remote = AgentCommandRunner.request( + "jj", ["commit"], in: "/r", timeout: 1, stdin: nil, network: false, host: .remote(UUID()), + barrierRoot: "/r") + XCTAssertTrue( + String(decoding: try encoder.encode(remote), as: UTF8.self).contains( + "\"barrier_root\":\"\\/r\"")) + // Nor for a network command, whose detached git helpers would inherit it. + let fetch = AgentCommandRunner.request( + "jj", ["git", "fetch"], in: "/r", timeout: 1, stdin: nil, network: true, + host: .remote(UUID()), barrierRoot: "/r") + XCTAssertNil(fetch.barrierRoot) } // MARK: File service (#211) diff --git a/macapp/WorkroomAppTests/RemoteHostIntegrationTests.swift b/macapp/WorkroomAppTests/RemoteHostIntegrationTests.swift index d2ace3660..d6816b77f 100644 --- a/macapp/WorkroomAppTests/RemoteHostIntegrationTests.swift +++ b/macapp/WorkroomAppTests/RemoteHostIntegrationTests.swift @@ -99,8 +99,8 @@ final class RemoteHostIntegrationTests: XCTestCase { return path } - /// VCS reads, File, Status and Forward, all through one ssh-stdio connection and the same manager - /// and router the app uses. + /// VCS reads and writes, File, Status and Forward, all through one ssh-stdio connection and the + /// same manager and router the app uses. func testEveryServiceAnswersThroughTheDriversStream() async throws { let fixture = try fixture() let path = try repository(fixture) @@ -119,13 +119,18 @@ final class RemoteHostIntegrationTests: XCTestCase { let page = try await reader.log(limit: 5) XCTAssertEqual(page.commits.map(\.summary), ["initial"]) - // Writes are refused by name, not attempted: see `AgentVCSConnection.writer`. - do { - _ = try await router.writer(for: location) - XCTFail("a remote writer was handed out") - } catch HostConnectionError.serviceUnavailable(let detail) { - XCTAssertTrue(detail.contains("remote repository"), detail) - } + // A VCS write, through the exec service in the host's environment. + try onHost(fixture, "echo next > \(path)/file") + let writer = try await router.writer(for: location) + let result = await writer.commit( + request: VCSCommitRequest( + message: "remote commit", + files: [ChangedFile(path: "file", change: .modified, oldPath: nil)], + mode: .commit)) + guard case .ok = result else { return XCTFail("commit failed: \(result)") } + XCTAssertEqual( + try onHost(fixture, "git -C \(path) log -1 --format='%s %ae'"), + "remote commit remote@example.com") // File. let files = try connection.files( @@ -133,7 +138,7 @@ final class RemoteHostIntegrationTests: XCTestCase { let listing = try await files.list(.git) XCTAssertEqual(FileListing.parse(listing.stdout, vcs: .git), ["file"]) let data = try await files.read(path: "file", symlinks: .refuse, maxBytes: 100) - XCTAssertEqual(data, Data("base\n".utf8)) + XCTAssertEqual(data, Data("next\n".utf8)) // Status. _ = try await connection.wakefulness().status() @@ -147,6 +152,32 @@ final class RemoteHostIntegrationTests: XCTestCase { XCTAssertEqual(banner, "SSH-2.0-") } + /// A failed remote write is classified from the HOST's disk (#229): a leftover `index.lock` there + /// is named by its path on the host. Read from this Mac's disk, that path is not there, and the + /// failure would name no lock at all. + func testARemoteCommitBlockedByALockNamesTheLockOnTheHost() async throws { + let fixture = try fixture() + let path = try repository(fixture) + try onHost(fixture, "echo next > \(path)/file && touch \(path)/.git/index.lock") + let (connection, id) = try await connect(fixture.host) + let host = HostID.remote(id) + let manager = HostConnectionManager() + _ = try await manager.connect(host: host) { connection } + let router = RepositoryRouter(connections: manager) + let location = try RepositoryLocation.remote(host: id, path: path) + try router.register(.init(location: location, backend: .git, sharedLocation: location)) + let writer = try await router.writer(for: location) + let result = await writer.commit( + request: VCSCommitRequest( + message: "blocked", + files: [ChangedFile(path: "file", change: .modified, oldPath: nil)], + mode: .commit)) + guard case .failed(.locked(let lock)) = result else { + return XCTFail("expected a lock failure, got \(result)") + } + XCTAssertEqual(lock?.path, "\(path)/.git/index.lock") + } + /// Nothing from the Mac's environment reaches a remote git (#229): the child runs with the /// host's `HOME` and `PATH`, no `SSH_AUTH_SOCK`, and no way for ssh to prompt. Read back from the /// child itself, through a git alias that prints its environment. diff --git a/macapp/WorkroomAppTests/VCSSyncPresentationTests.swift b/macapp/WorkroomAppTests/VCSSyncPresentationTests.swift index a53999da2..2fa294018 100644 --- a/macapp/WorkroomAppTests/VCSSyncPresentationTests.swift +++ b/macapp/WorkroomAppTests/VCSSyncPresentationTests.swift @@ -32,6 +32,16 @@ final class VCSSyncPresentationTests: XCTestCase { // MARK: States + /// A lock file on a remote host names nothing on this Mac, so neither Reveal in Finder nor Copy + /// Lock File Path is offered for it (#229). + func testALockFileOnARemoteHostIsNotOfferedAsAPathOnThisMac() { + let local = VCSLockFile(path: "/repo/.git/index.lock", modifiedAt: Date()) + var remote = local + remote.isOnThisMac = false + XCTAssertEqual(VCSSyncPresenter.lockPath(of: .locked(local)), "/repo/.git/index.lock") + XCTAssertNil(VCSSyncPresenter.lockPath(of: .locked(remote))) + } + func testNoTargetIsDisabled() { let p = VCSSyncPresenter.make(state: nil, hasTarget: false, now: now) XCTAssertEqual(p.subtitle, "No workroom selected") diff --git a/vcs/crates/wr-agent/src/vcs.rs b/vcs/crates/wr-agent/src/vcs.rs index 17a132b52..b3e213b91 100644 --- a/vcs/crates/wr-agent/src/vcs.rs +++ b/vcs/crates/wr-agent/src/vcs.rs @@ -57,8 +57,9 @@ const MAX_HISTORY_LIMIT: usize = 10000; /// The exec service's wire version, reported in `capabilities` so a client can tell a capable agent /// from one that predates the service. A version, not a count — see the `capabilities` reply. /// -/// 2 added chunked request reassembly; 1 accepted only single-envelope requests. -const EXEC_SERVICE_VERSION: u32 = 2; +/// 3 added what a REMOTE host's writes need (#229): `host_environment`, `barrier_root`, and the +/// `stat` request. 2 added chunked request reassembly; 1 accepted only single-envelope requests. +const EXEC_SERVICE_VERSION: u32 = 3; /// `CLIVCSWriter.commitTimeout` (Swift) is 600s, the longest legitimate write timeout. Bounds a /// hostile/buggy request from wedging an exec thread indefinitely; the 32-slot `ACTIVE` permit /// already bounds concurrency, this bounds duration. @@ -204,6 +205,17 @@ struct ExecRequest { /// its `SSH_AUTH_SOCK` and git identity are the user's Mac credentials. #[serde(default)] host_environment: bool, + /// The shared root of a jj repository whose working-copy barrier this command must run under, + /// set by the app for a REMOTE host only. Locally the app holds that barrier itself + /// (`JJSnapshotGate`) for the whole operation, which is why exec never takes it; a Mac cannot + /// flock a file on another host, so there the agent takes it for each command instead. + /// + /// ponytail: per command, where the local gate holds it for a whole operation (a commit's + /// several commands under one acquisition). A snapshot can land between two commands of one + /// remote write. Upgrade path: an operation-scoped barrier the client acquires and releases, + /// held by this connection and dropped when it goes. + #[serde(default)] + barrier_root: Option, } /// The child environment for a remote host: this agent's own, which is the host's, with the file @@ -244,6 +256,83 @@ enum ExecKind { Exec, } +/// Filesystem facts for the app's write-failure classifier (`CLIVCSWriter`), which reads a +/// repository's git directory to tell a parked rebase, a leftover lock file or the last fetch. On a +/// remote host that directory is here, so the app asks for the facts and keeps every decision on +/// its own side (#229): one classifier, fed the same facts either way. Added with exec version 3. +#[derive(Deserialize)] +#[serde(deny_unknown_fields)] +struct StatRequest { + version: u32, + kind: StatKind, + /// Absolute paths. What exists there, whether it is a directory, and when it was modified. + paths: Vec, + /// Paths among `paths` whose text to return as well, when a small regular file: a worktree's + /// `.git` is a file that names its git directory. + #[serde(default)] + read: Vec, +} + +#[derive(Deserialize)] +#[serde(rename_all = "snake_case")] +enum StatKind { + Stat, +} + +/// Far more than the dozen paths one classification asks about, and small enough that a request +/// cannot turn this into a filesystem walk. +const MAX_STAT_PATHS: usize = 64; +/// A worktree's `.git` file is one line. +const MAX_STAT_TEXT: u64 = 4096; + +fn stat(request: StatRequest) -> model::Result { + let StatRequest { + version, + kind: StatKind::Stat, + paths, + read, + } = request; + if version != 1 { + return Err(VcsError::BackendVersion( + "unsupported VCS service version".into(), + )); + } + if paths.len() > MAX_STAT_PATHS { + return Err(io("too many paths in one stat request")); + } + let entries = paths + .iter() + .map(|path| { + // One path this cannot take (relative, or with a `..` from an unstandardized `gitdir:` + // or a hook's message) reads as absent, as a missing file would, rather than failing the + // whole request and taking every other fact with it. + let Ok(target) = absolute(path) else { + return Ok(json!({ "path": path, "exists": false, "is_dir": false, + "modified_at": null, "text": null })); + }; + // Follows links, as `FileManager.fileExists` does for the same question in the app. + let metadata = std::fs::metadata(&target).ok(); + let text = metadata + .as_ref() + .filter(|m| read.contains(path) && m.is_file() && m.len() <= MAX_STAT_TEXT) + .and_then(|_| std::fs::read_to_string(&target).ok()); + let modified = metadata + .as_ref() + .and_then(|m| m.modified().ok()) + .and_then(|time| time.duration_since(std::time::UNIX_EPOCH).ok()) + .map(|since| since.as_secs_f64()); + Ok(json!({ + "path": path, + "exists": metadata.is_some(), + "is_dir": metadata.as_ref().is_some_and(|m| m.is_dir()), + "modified_at": modified, + "text": text, + })) + }) + .collect::>>()?; + Ok(json!({ "entries": entries })) +} + #[derive(Clone, Copy, Deserialize)] #[serde(rename_all = "snake_case")] enum Executable { @@ -390,16 +479,16 @@ pub fn execute(bytes: &[u8]) -> Value { // wire format already spoken by `AgentVCSReader` is untouched. let result = serde_json::from_slice::(bytes) .map_err(io) - .and_then(|value| { - if value.get("kind").and_then(Value::as_str) == Some("exec") { - serde_json::from_value::(value) - .map_err(io) - .and_then(exec) - } else { - serde_json::from_value::(value) - .map_err(io) - .and_then(read) - } + .and_then(|value| match value.get("kind").and_then(Value::as_str) { + Some("exec") => serde_json::from_value::(value) + .map_err(io) + .and_then(exec), + Some("stat") => serde_json::from_value::(value) + .map_err(io) + .and_then(stat), + _ => serde_json::from_value::(value) + .map_err(io) + .and_then(read), }); match result { Ok(result) => json!({"version": 1, "result": result}), @@ -508,6 +597,7 @@ const REAP_GRACE: Duration = Duration::from_secs(2); /// SIGTERM-then-grace-then-SIGKILL shape (`CLIVCSWriter.commitTimeout`'s doc: "Killing a commit is /// categorically more dangerous than killing a fetch" — an immediate SIGKILL denies a `post-commit` /// hook the chance to exit cleanly and can leave `index.lock` behind). +#[cfg(test)] fn run_exec( dir: &Path, executable: &str, @@ -834,6 +924,7 @@ fn exec(request: ExecRequest) -> model::Result { stdin, env, host_environment: from_host, + barrier_root, } = request; if version != 1 { return Err(VcsError::BackendVersion( @@ -842,6 +933,19 @@ fn exec(request: ExecRequest) -> model::Result { } let dir = absolute(&dir)?; let timeout = exec_timeout(timeout_ms); + // Held until the child exits, and handed to it, so an agent that dies mid-command leaves the + // barrier held by the child rather than released under it. No `.jj` there, no barrier: a git + // repository has none to take, as `JJProcessBarrier.acquire` decides in the app. + // + // Nor for a command that cannot snapshot (`--ignore-working-copy`): there is nothing to protect, + // and a read waiting out a long push's barrier would time out as a failure the toolbar shows. + let snapshots = !args.iter().any(|arg| arg == "--ignore-working-copy"); + let barrier = match barrier_root.as_deref() { + Some(shared) if snapshots && absolute(shared)?.join(".jj").exists() => { + Some(SnapshotLock::acquire(&dir, Some(shared))?) + } + _ => None, + }; let env = if from_host { host_environment(std::env::vars_os(), env) } else { @@ -849,14 +953,16 @@ fn exec(request: ExecRequest) -> model::Result { }; let env: Vec<(&str, &str)> = env.iter().map(|(k, v)| (k.as_str(), v.as_str())).collect(); let stdin = stdin.as_deref().map(latin1_bytes).transpose()?; - let captured = run_exec( + let captured = run_exec_with( &dir, executable.as_str(), &args, timeout, stdin.as_deref(), &env, + barrier.as_ref().map(SnapshotLock::fd), )?; + drop(barrier); let stdout = String::from_utf8_lossy(&captured.stdout); let stderr = String::from_utf8_lossy(&captured.stderr); // Truncate HERE rather than letting `send` refuse the reply. `send`'s blanket @@ -1531,6 +1637,109 @@ mod tests { std::fs::remove_dir_all(&root).unwrap(); } + /// The remote half of that property (#229): a Mac cannot flock the barrier on another host, so + /// an exec request that names a `barrier_root` must run under the barrier, taken here. Held by + /// another holder, the command waits for it rather than running alongside. + #[test] + fn exec_with_a_barrier_root_waits_on_a_held_barrier() { + let root = + std::env::temp_dir().join(format!("wr-vcs-exec-barrier-root-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&root); + std::fs::create_dir_all(root.join(".jj/repo")).unwrap(); + let held = SnapshotLock::acquire(&root, root.to_str()).unwrap(); + let releaser = std::thread::spawn(move || { + std::thread::sleep(Duration::from_millis(700)); + drop(held); + }); + let mut request: Value = + serde_json::from_slice(&exec_request(&root, "git", &["--version"])).unwrap(); + request["barrier_root"] = json!(root.to_str().unwrap()); + let start = Instant::now(); + let reply = execute(&serde_json::to_vec(&request).unwrap()); + assert!(reply.get("result").is_some(), "exec failed: {reply:?}"); + assert!( + start.elapsed() >= Duration::from_millis(600), + "the command ran without waiting for the barrier" + ); + releaser.join().unwrap(); + std::fs::remove_dir_all(&root).unwrap(); + } + + /// A command that cannot snapshot has nothing for the barrier to protect, so it does not wait. + #[test] + fn a_command_that_cannot_snapshot_skips_the_barrier() { + let root = + std::env::temp_dir().join(format!("wr-vcs-exec-barrier-read-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&root); + std::fs::create_dir_all(root.join(".jj/repo")).unwrap(); + let held = SnapshotLock::acquire(&root, root.to_str()).unwrap(); + let mut request: Value = serde_json::from_slice(&exec_request( + &root, + "git", + &["--version", "--ignore-working-copy"], + )) + .unwrap(); + request["barrier_root"] = json!(root.to_str().unwrap()); + let start = Instant::now(); + let _ = execute(&serde_json::to_vec(&request).unwrap()); + assert!( + start.elapsed() < Duration::from_secs(5), + "a read waited on the barrier" + ); + drop(held); + std::fs::remove_dir_all(&root).unwrap(); + } + + /// A git repository has no barrier to take, and naming one is not an error: the app sends the + /// shared root for every remote jj command, and decides nothing about git from it. + #[test] + fn a_barrier_root_without_jj_runs_the_command_unlocked() { + let root = git_repo("barrier-root-git"); + let mut request: Value = + serde_json::from_slice(&exec_request(&root, "git", &["status"])).unwrap(); + request["barrier_root"] = json!(root.to_str().unwrap()); + let reply = execute(&serde_json::to_vec(&request).unwrap()); + assert_eq!(reply["result"]["exit_code"], 0, "{reply}"); + std::fs::remove_dir_all(&root).unwrap(); + } + + /// The facts the app's classifier reads from a git directory (#229), for a path that is a + /// directory, a file whose text was asked for, and one that is not there. + #[test] + fn stat_reports_what_is_at_each_path() { + let root = git_repo("stat"); + let git = root.join(".git"); + std::fs::write(root.join("pointer"), "gitdir: /elsewhere\n").unwrap(); + let request = json!({ + "version": 1, + "kind": "stat", + "paths": [git, root.join("pointer"), git.join("rebase-merge")], + "read": [root.join("pointer")], + }); + let reply = execute(&serde_json::to_vec(&request).unwrap()); + let entries = reply["result"]["entries"].as_array().expect("entries"); + assert_eq!(entries[0]["exists"], true); + assert_eq!(entries[0]["is_dir"], true); + assert!(entries[0]["modified_at"].as_f64().unwrap() > 0.0); + assert_eq!(entries[0]["text"], Value::Null, "text was not asked for"); + assert_eq!(entries[1]["is_dir"], false); + assert_eq!(entries[1]["text"], "gitdir: /elsewhere\n"); + assert_eq!(entries[2]["exists"], false); + assert_eq!(entries[2]["modified_at"], Value::Null); + + let relative = json!({"version": 1, "kind": "stat", "paths": ["relative/path", git]}); + let reply = execute(&serde_json::to_vec(&relative).unwrap()); + assert_eq!(reply["result"]["entries"][0]["exists"], false, "{reply}"); + assert_eq!( + reply["result"]["entries"][1]["exists"], true, + "one bad path spoiled the rest" + ); + let many: Vec = (0..=MAX_STAT_PATHS).map(|i| format!("/tmp/{i}")).collect(); + let too_many = json!({"version": 1, "kind": "stat", "paths": many}); + assert!(execute(&serde_json::to_vec(&too_many).unwrap())["error"].is_object()); + std::fs::remove_dir_all(&root).unwrap(); + } + #[test] fn capabilities_reports_the_read_count_and_the_exec_service_version() { let reply = execute(br#"{"version":1,"method":"capabilities"}"#); From 1d7ed00b7fed444fcd79d1a2edf12889426dd0bf Mon Sep 17 00:00:00 2001 From: Joel Moss Date: Thu, 24 Sep 2026 21:54:41 +0100 Subject: [PATCH 4/7] feat(macapp): attach a terminal pane to a session on a remote host The second half of #229. A pane is a process libghostty spawns on its own pty, so it cannot use openStream's socketpair. A HostTerminalDriver hands it a command instead: for ssh, `ssh -t 'env TERM=xterm-256color WORKROOM_SESSION_{ID,SOCKET,CWD}=... wr-agent attach --no-spawn [--no-create]'`, with EscapeChar none so `~.` is the user's. wr-agent attach: - --no-spawn: with no agent listening, a shell rather than a spawned agent. Only the supervisor starts one on a remote host. - --no-create (a restored pane): the attach request carries CREATE=0, and the agent refuses a session it does not hold instead of creating one. The client reads that answer before raw mode and its relay threads, and becomes a shell saying the session ended. One request, so the answer cannot go stale the way a separate ownership probe could. - A remote attach reports a session's own exit 255 as 254, so that 255 means ssh lost the link. App: - PersistentSessionService.registerRemoteSession routes a session to its driver: no local helper is probed about it or asked to kill it, and its pane carries no local session variables. A host that cannot be reached gets a pane that says so, not a shell on the Mac. - A remote pane whose ssh exits 255 reattaches as a restored pane, backing off to 30s and stopping after five quick failures so a permanent error stays on screen. Co-Authored-By: Claude Opus 5.5 --- docs/designs/remote-workrooms.md | 32 +++ .../Core/ContainerHostDriver.swift | 48 ++++- .../WorkroomApp/Core/GhosttySurfaceView.swift | 46 ++++- macapp/WorkroomApp/Core/HostDriver.swift | 16 ++ .../Session/PersistentSessionService.swift | 61 +++++- macapp/WorkroomAppTests/HostStreamTests.swift | 82 ++++++++ .../RemoteHostIntegrationTests.swift | 92 +++++++++ vcs/crates/wr-agent/src/main.rs | 100 ++++++++- vcs/crates/wr-agent/src/serve.rs | 21 ++ .../tests/attach_falls_back_to_a_shell.rs | 190 +++++++++++++++++- 10 files changed, 677 insertions(+), 11 deletions(-) diff --git a/docs/designs/remote-workrooms.md b/docs/designs/remote-workrooms.md index 43a164643..10b230a1b 100644 --- a/docs/designs/remote-workrooms.md +++ b/docs/designs/remote-workrooms.md @@ -1025,6 +1025,38 @@ these are the subsystems that actually gate "a remote workroom is a real workroo before-read and the commit can make a failed commit read as one that landed. Locally the after-read already sits outside the gate; the per-command barrier widens the gap. The fix is to compare `@-`, which a snapshot never rewrites, for jj's `.commit` mode. + **Terminal panes (#229).** A pane is a process libghostty spawns on its own pty, which a socketpair + made in the app cannot become, so a pane does not use `openStream`. A driver that can host panes + implements `HostTerminalDriver.attachCommand`. For ssh that runs + `ssh -t 'env TERM=xterm-256color WORKROOM_SESSION_{ID,SOCKET,CWD}=… wr-agent attach + --no-spawn [--no-create]'`, the far side's attach client in the pty ssh allocates there. + - *`--no-spawn`:* with no agent listening, a shell rather than a spawned agent. Only the + supervisor starts one there. + - *`--no-create` (a restored pane):* the agent refuses a session it does not hold (`CREATE=0` in + the attach request, which older agents ignore). The pane becomes a shell that says the session + ended. Locally the app asks first (`confirmBeforeAttach`); remotely one request answers and + attaches, so the answer cannot go stale in between. + - *A dropped link reconnects.* ssh's own failure (exit 255, as after sleep or a network change) + reattaches the pane as a restored one, backing off to 30s while the host stays unreachable, + and stopping after five quick failures in a row so a permanent one (a mismatched host key) + leaves its message on screen. A session that itself exited 255 is reported as 254 by the + remote attach, so it never reads as a dropped link. The pane's ssh has `EscapeChar none`: what + the user types is theirs, and `~.` would otherwise disconnect it. + - *Closing a remote pane does not end its session yet.* Nothing on the service connection can + ask the host's agent to, so the session is left running and reported as not killed. That + belongs with the host's lifecycle (Phase 4). + - *`CREATE=0` is only as good as the agent that reads it.* An agent older than the flag ignores + it and creates the session. The version hand-off (#230) and the bootstrap (#231) keep the + host's agent current; until they land, a host running an older agent can still hand a + restored pane a fresh shell. + - *Degraded until the agent bootstrap (#231):* `TERM` is `xterm-256color`, since a host rarely + has `xterm-ghostty` terminfo, and there is no shell integration there (its resources are in the + Mac's app bundle), so the title and footer do not follow the shell. + - *Not persisted:* a session is marked remote in memory (`registerRemoteSession`). Phase 4's + remote workrooms re-register their panes on relaunch. + - *An SDK-exec driver has no command to hand libghostty.* It needs a local bridge process, which + is a Phase 4 question. + - **Local needs no supervisor at all. DECIDED — and an earlier claim in this document was wrong.** A previous revision asserted that `SessionDaemon.swift:97-107`'s self-exit (`sessions.isEmpty && connections.isEmpty`) had to go, because the agent would also own VCS reads diff --git a/macapp/WorkroomApp/Core/ContainerHostDriver.swift b/macapp/WorkroomApp/Core/ContainerHostDriver.swift index 986f1f99f..bc49ee9e1 100644 --- a/macapp/WorkroomApp/Core/ContainerHostDriver.swift +++ b/macapp/WorkroomApp/Core/ContainerHostDriver.swift @@ -7,7 +7,7 @@ import Foundation /// /// Provisioning is Phase 4, so `create`, `deriveFromBase` and `destroy` say so rather than pretend. /// Which hosts exist is whatever the caller hands in; nothing here persists. -struct ContainerHostDriver: HostDriver { +struct ContainerHostDriver: HostTerminalDriver { struct Host: Sendable { let address: String let port: Int @@ -57,6 +57,48 @@ struct ContainerHostDriver: HostDriver { handshakeTimeout: 20) } + func attachCommand( + to host: HostID, session: UUID, workingDirectory: String, restored: Bool + ) throws -> String { + guard case .remote(let id) = host, let target = hosts[id] else { + throw HostDriverError.unknownHost(host) + } + let config = try Self.writeConfiguration( + for: target, in: directory.appendingPathComponent(id.uuidString)) + // `-t`: the attach client on the far side wants a terminal, for raw mode and the pane's size, + // and ssh forwards the pane's resizes to it. It outranks the config's `RequestTTY no`, which + // is right for the service stream. + return ["/usr/bin/ssh", "-F", config.path, "-t", Self.alias] + .map(Self.shellQuoted).joined(separator: " ") + + " " + + Self.shellQuoted( + Self.remoteAttachCommand( + session: session, socket: target.agentSocket, workingDirectory: workingDirectory, + restored: restored)) + } + + /// What runs on the host, in the pty ssh allocates there. + /// + /// The session contract is the environment `wr-agent attach` reads, set with `env` because ssh + /// carries none of the pane's own. Deliberately absent: the shell (the host's own login shell + /// applies), Ghostty's resources directory (a path in this Mac's app bundle), and the wakefulness + /// settings (they configure an agent the attach starts, and `--no-spawn` never starts one). + /// `TERM` is `xterm-256color`, not the pane's `xterm-ghostty`, which a host rarely has terminfo + /// for; shipping that terminfo belongs to the agent bootstrap (#231). + static func remoteAttachCommand( + session: UUID, socket: String, workingDirectory: String, restored: Bool + ) -> String { + let variables = [ + "TERM=xterm-256color", + "WORKROOM_SESSION_ID=\(session.uuidString)", + "WORKROOM_SESSION_SOCKET=\(socket)", + "WORKROOM_SESSION_CWD=\(workingDirectory)", + ] + return (["env"] + variables + ["wr-agent", "attach", "--no-spawn"] + + (restored ? ["--no-create"] : [])) + .map(shellQuoted).joined(separator: " ") + } + static let alias = "workroom-host" /// The command ssh runs on the host, quoted for the remote shell. @@ -74,6 +116,9 @@ struct ContainerHostDriver: HostDriver { /// 8.8+ servers no longer offer. /// - **No Mac credential crosses.** No agent forwarding, no X11, no port forwards, and the Mac's /// ssh agent is not consulted for keys. + /// - **What a pane types is only ever the user's.** A pane's ssh has a terminal, and ssh reads + /// `~.` and friends from one as its own commands (`~.` disconnects, `~^Z` suspends ssh and + /// freezes the pane), so `EscapeChar none`. /// - **A dead link is noticed.** `ServerAliveInterval` ends ssh after about 45s of silence from a /// host that stopped answering, which ends the stream and so the connection. It is what bounds /// a link that dies with nothing to send (#228). @@ -117,6 +162,7 @@ struct ContainerHostDriver: HostDriver { ClearAllForwardings yes ControlMaster no ControlPath none + EscapeChar none RequestTTY no ServerAliveInterval 15 ServerAliveCountMax 3 diff --git a/macapp/WorkroomApp/Core/GhosttySurfaceView.swift b/macapp/WorkroomApp/Core/GhosttySurfaceView.swift index 0b063641a..c4bd33c07 100644 --- a/macapp/WorkroomApp/Core/GhosttySurfaceView.swift +++ b/macapp/WorkroomApp/Core/GhosttySurfaceView.swift @@ -458,7 +458,7 @@ final class GhosttySurfaceView: NSView { // the owning backend has no binary, so the global check bought nothing on this path. guard let attach = PersistentSessionService.shared.attachCommand( - forSession: persistentSessionID), + forSession: persistentSessionID, restored: persistentSessionIsRestored), let attachPointer = strdup(attach) else { // A pane that expected a persisted session fell back to a plain login shell. Whatever the @@ -519,8 +519,37 @@ final class GhosttySurfaceView: NSView { return "/bin/sh -c \(shellQuoted(script))" } + /// A pane on a remote host whose ssh lost the link reattaches, rather than sitting on a dead + /// process while its session runs on: 255 is ssh's own failure, as after laptop sleep, a network + /// change or the host rebooting. It reattaches as a restored pane, so a session that ended + /// meanwhile becomes a shell that says so. Backs off to 30s while the host stays unreachable. + private func reconnectIfTheLinkDropped(exitCode: UInt32) { + guard exitCode == 255, let session = persistentSessionID, !isTornDown, + PersistentSessionService.shared.isRemote(session) + else { return } + if Date().timeIntervalSince(lastAttachAt) > 30 { + remoteReconnectDelay = 1 + failedReconnects = 0 + } else { + failedReconnects += 1 + guard failedReconnects < Self.maxFailedReconnects else { return } + } + let delay = remoteReconnectDelay + remoteReconnectDelay = min(delay * 2, 30) + let reconnect = DispatchWorkItem { [weak self] in + guard let self, !self.isTornDown, self.persistentSessionID == session else { return } + self.persistentSessionIsRestored = true + self.reattachPersistentSession() + } + pendingReconnect = reconnect + DispatchQueue.main.asyncAfter(deadline: .now() + delay, execute: reconnect) + } + func reattachPersistentSession() { guard persistentSessionID != nil, !isTornDown else { return } + pendingReconnect?.cancel() + pendingReconnect = nil + lastAttachAt = Date() destroySurface() if bounds.width <= 0 || bounds.height <= 0 { setFrameSize(CGSize(width: 800, height: 480)) @@ -883,7 +912,22 @@ final class GhosttySurfaceView: NSView { /// Called by `GhosttyRuntimeAdapter` on `GHOSTTY_ACTION_SHOW_CHILD_EXITED` — the surface's child /// process exited. Forwards to the host; only run tabs act on it (issue #7). + /// How long the next reconnect of a remote pane waits, doubling while the host stays unreachable. + private var remoteReconnectDelay: TimeInterval = 1 + /// When this pane last attached, to tell a link that dropped after a while (start the backoff + /// again) from one that never came up (keep backing off). + private var lastAttachAt = Date.distantPast + /// Reconnects in a row that failed within 30s. Past `maxFailedReconnects` the pane stops and + /// leaves ssh's last error on screen: a mismatched host key or a destroyed host does not heal, + /// and each retry would wipe the message that says so. + private var failedReconnects = 0 + private static let maxFailedReconnects = 5 + /// The reconnect waiting out its delay, cancelled by any other reattach so the pane does not + /// attach twice. + private var pendingReconnect: DispatchWorkItem? + func handleChildExited(exitCode: UInt32) { + reconnectIfTheLinkDropped(exitCode: exitCode) onChildExited?(exitCode) } diff --git a/macapp/WorkroomApp/Core/HostDriver.swift b/macapp/WorkroomApp/Core/HostDriver.swift index ca3c3e8d7..e4fba9c77 100644 --- a/macapp/WorkroomApp/Core/HostDriver.swift +++ b/macapp/WorkroomApp/Core/HostDriver.swift @@ -38,6 +38,22 @@ protocol HostDriver: Sendable { func openStream(to host: HostID) async throws -> HostStream } +/// A driver whose hosts a terminal pane can attach to. +/// +/// Not a fifth `HostDriver` method, and not `openStream`: a pane is a process libghostty spawns and +/// wires to its own pty, and a socketpair made in this process cannot become one. So the driver +/// hands over a command instead. For ssh that is `ssh -t wr-agent attach …`, the far side's +/// attach client running in the pty ssh allocates there. An SDK-exec driver has no such command, +/// and would need a local bridge process: a Phase 4 question (design doc, Phase 3). +protocol HostTerminalDriver: HostDriver { + /// The command a pane runs to attach to `session` on `host`, starting in `workingDirectory` if + /// the session is new. `restored` for a pane reattaching after a relaunch or a lost link: the + /// session must already exist there, and if it has ended the pane gets a shell that says so + /// rather than a fresh one that looks like it (`wr-agent attach --no-create`). + func attachCommand(to host: HostID, session: UUID, workingDirectory: String, restored: Bool) + throws -> String +} + enum HostDriverError: Error, Equatable, Sendable, LocalizedError { case unknownHost(HostID) /// Provisioning lands in Phase 4, which is what says how a base and an instance come to exist. diff --git a/macapp/WorkroomApp/Core/Session/PersistentSessionService.swift b/macapp/WorkroomApp/Core/Session/PersistentSessionService.swift index b3d6781c8..7491ccebe 100644 --- a/macapp/WorkroomApp/Core/Session/PersistentSessionService.swift +++ b/macapp/WorkroomApp/Core/Session/PersistentSessionService.swift @@ -57,6 +57,17 @@ final class PersistentSessionService { /// Which helper owns each session, resolved once. See `backend(forSession:)` for why one answer /// per session rather than one per call — a pane asks twice and the two must agree. private var owners: [UUID: SessionBackend] = [:] + /// Sessions on a remote host, and the driver that reaches it (#229). Checked before any local + /// owner: a remote session's owner is known by construction, and no local socket can answer for + /// it. Registered by whoever makes a pane for a remote workroom; nothing persists it yet, so a + /// relaunch re-registers it (Phase 4). + private var remoteSessions: [UUID: RemoteSession] = [:] + + private struct RemoteSession { + let host: HostID + let driver: any HostTerminalDriver + let workingDirectory: String + } /// How the agent's health is measured. Injected so a test can drive the unhealthy path without /// a real `wr-agent` to break. @@ -326,6 +337,9 @@ final class PersistentSessionService { // and asking would refuse every new pane. Only an id carried over from a previous launch can // name a session that has since died. guard wasRestored else { return .attachable } + // Asked of the host's agent in the attach itself (`--no-create`): one request, so the answer + // cannot go stale between the question and the attach. + if isRemote(sessionID) { return .attachable } guard let owner = backend(forSession: sessionID) else { return .attachable } switch ownership(of: sessionID, in: owner) { case .owned, .unreachable: @@ -351,7 +365,36 @@ final class PersistentSessionService { /// empty environment: `workroom-session attach` then started with no `WORKROOM_SESSION_ID` or /// `_SOCKET` and exited 2 immediately, and because the plain-shell fallback is decided BEFORE the /// command is spawned, the pane died rather than degrading. Refusing here degrades properly. - func attachCommand(forSession sessionID: UUID) -> String? { + /// Points `sessionID` at `host`: its pane attaches there, through `driver`, starting in + /// `workingDirectory` (a path on that host) if the session is new. + func registerRemoteSession( + _ sessionID: UUID, on host: HostID, via driver: any HostTerminalDriver, + workingDirectory: String + ) { + remoteSessions[sessionID] = RemoteSession( + host: host, driver: driver, workingDirectory: workingDirectory) + } + + func isRemote(_ sessionID: UUID) -> Bool { remoteSessions[sessionID] != nil } + + func attachCommand(forSession sessionID: UUID, restored: Bool = false) -> String? { + if let remote = remoteSessions[sessionID] { + do { + return try remote.driver.attachCommand( + to: remote.host, session: sessionID, workingDirectory: remote.workingDirectory, + restored: restored) + } catch { + logger.error( + "no attach command for remote session \(sessionID.uuidString, privacy: .public): \(error)") + // Not nil: nil opens a plain shell, which for this pane would run on the Mac while it + // reads as the remote workroom's. A pane that says why it could not reach the host is + // honest; one on the wrong machine is not. + let notice = "Could not reach this terminal's host: \(error.localizedDescription)" + return "/bin/sh -c " + + ContainerHostDriver.shellQuoted( + "printf '%s\\n' " + ContainerHostDriver.shellQuoted(notice)) + } + } // Nil owner ⇒ nil command ⇒ the caller opens a plain shell rather than guessing a helper. guard let owner = backend(forSession: sessionID), let path = binaryPath(for: owner), @@ -369,6 +412,10 @@ final class PersistentSessionService { .defaultShell, resourcesDirectory: String? = GhosttyResources.bundledURL?.path ) -> [(key: String, value: String)] { + // None for a pane on a remote host: its command carries the session's variables to the host + // itself (`HostTerminalDriver.attachCommand`), and a `WORKROOM_SESSION_*` left in this pane's + // environment would point a local `wr-agent` typed there at a session it cannot reach. + if isRemote(sessionID) { return [] } // Socket and binary must both come from the helper that owns THIS session, or the relay is // pointed at one implementation while being told to use the other's socket. // `WORKROOM_SESSION_BINARY` used to be set here. Its only reader was the attach client's @@ -449,6 +496,9 @@ final class PersistentSessionService { } func lookup(sessionID: UUID) async -> PersistentSessionLookup { + // A remote session is not on any local helper, and asking one would probe (and wait on) a + // socket that cannot know it. Its host answers for it; this build has no channel for that yet. + if isRemote(sessionID) { return .unreachable } // Routed by OWNER, with no gate on the preferred backend's socket. That gate was left over // from before per-session routing and inverted the drain: with the agent preferred but no // `agent.sock` yet, every session the daemon still owned reported `.unreachable` — and the @@ -470,6 +520,15 @@ final class PersistentSessionService { @discardableResult func endSession(sessionID: UUID) async -> Bool { + // Never routed to a local helper, which would be asked to kill an id it does not hold. Ending + // a session on its host belongs to that host's lifecycle (Phase 4); until then it is reported + // as not killed, which is true, and nothing that gates on this deletes a local directory for + // it. + if remoteSessions.removeValue(forKey: sessionID) != nil { + logger.error( + "remote session \(sessionID.uuidString, privacy: .public) left running on its host") + return false + } // An UNKNOWN owner must report failure, not success. `reap` gates deleting the workroom's // directory on this (issue #7), so answering "killed" for a session we could not even route to // would delete a worktree out from under a live shell. A malformed id or a missing socket is diff --git a/macapp/WorkroomAppTests/HostStreamTests.swift b/macapp/WorkroomAppTests/HostStreamTests.swift index 05e008ac1..5532051cf 100644 --- a/macapp/WorkroomAppTests/HostStreamTests.swift +++ b/macapp/WorkroomAppTests/HostStreamTests.swift @@ -106,6 +106,7 @@ final class HostStreamTests: XCTestCase { "BatchMode yes", "StrictHostKeyChecking yes", "GlobalKnownHostsFile /dev/null", "IdentitiesOnly yes", "IdentityAgent none", "ForwardAgent no", "ClearAllForwardings yes", "ServerAliveInterval 15", + "EscapeChar none", ] { XCTAssertTrue(config.contains(" \(line)\n"), "missing \(line)") } @@ -125,6 +126,87 @@ final class HostStreamTests: XCTestCase { for: host, in: FileManager.default.temporaryDirectory)) } + /// A remote pane's command (#229): the session contract rides in `env` on the host, and the + /// Mac-only parts of it (the shell, the app bundle's resources) stay behind. + func testARemotePanesCommandCarriesTheSessionAndNothingOfTheMacs() throws { + let session = UUID() + let fresh = ContainerHostDriver.remoteAttachCommand( + session: session, socket: "/run/workroom/agent.sock", + workingDirectory: "/home/w/it's here", restored: false) + XCTAssertEqual( + fresh, + "'env' 'TERM=xterm-256color' 'WORKROOM_SESSION_ID=\(session.uuidString)' " + + "'WORKROOM_SESSION_SOCKET=/run/workroom/agent.sock' " + + "'WORKROOM_SESSION_CWD=/home/w/it'\\''s here' 'wr-agent' 'attach' '--no-spawn'") + let restored = ContainerHostDriver.remoteAttachCommand( + session: session, socket: "/s", workingDirectory: "/w", restored: true) + XCTAssertTrue(restored.hasSuffix("'--no-spawn' '--no-create'"), restored) + for absent in ["WORKROOM_SESSION_SHELL", "WORKROOM_SESSION_RESOURCES", "AWAKE"] { + XCTAssertFalse(fresh.contains(absent), absent) + } + + let directory = FileManager.default.temporaryDirectory.appendingPathComponent( + UUID().uuidString) + defer { try? FileManager.default.removeItem(at: directory) } + let id = UUID() + let driver = ContainerHostDriver( + hosts: [ + id: .init( + address: "127.0.0.1", port: 2222, user: "workroom", identityFile: "/keys/id", + hostKey: "ssh-ed25519 AAAA", agentSocket: "/s") + ], directory: directory) + let command = try driver.attachCommand( + to: .remote(id), session: session, workingDirectory: "/w", restored: true) + XCTAssertTrue(command.hasPrefix("'/usr/bin/ssh' '-F' "), command) + XCTAssertTrue(command.contains(" '-t' 'workroom-host' "), command) + } + + /// A remote session is routed to its driver, not to a local helper: its command is the driver's, + /// the pane's own environment carries no session variables, and a restored pane is not asked + /// about locally (the host's agent answers that, in the attach). + @MainActor + func testARemoteSessionIsRoutedToItsDriverNotALocalHelper() async throws { + let service = PersistentSessionService( + probe: { _ in .unhealthy(reason: "none here") }, + ownership: { _ in + XCTFail("a local helper was asked about a remote session") + return .notOwned + }) + let directory = FileManager.default.temporaryDirectory.appendingPathComponent( + UUID().uuidString) + defer { try? FileManager.default.removeItem(at: directory) } + let host = UUID() + let driver = ContainerHostDriver( + hosts: [ + host: .init( + address: "127.0.0.1", port: 2222, user: "workroom", identityFile: "/keys/id", + hostKey: "ssh-ed25519 AAAA", agentSocket: "/s") + ], directory: directory) + let session = UUID() + service.registerRemoteSession( + session, on: .remote(host), via: driver, workingDirectory: "/home/w") + + XCTAssertEqual(service.confirmBeforeAttach(sessionID: session, wasRestored: true), .attachable) + let command = try XCTUnwrap(service.attachCommand(forSession: session, restored: true)) + XCTAssertTrue(command.contains("/usr/bin/ssh"), command) + XCTAssertTrue(command.contains("--no-create"), command) + XCTAssertTrue(service.launchEnvironment(sessionID: session, workingDirectory: "/w").isEmpty) + + // A host the driver cannot reach gets a pane that says so, never a plain shell on this Mac. + let stranded = UUID() + service.registerRemoteSession( + stranded, on: .remote(UUID()), via: driver, workingDirectory: "/home/w") + let notice = try XCTUnwrap(service.attachCommand(forSession: stranded)) + XCTAssertTrue(notice.hasPrefix("/bin/sh -c "), notice) + XCTAssertTrue(notice.contains("Could not reach this terminal"), notice) + + // Closing it reaches no local helper (the ownership closure above fails the test if asked), + // and reports it not killed, which is true. + let killed = await service.endSession(sessionID: session) + XCTAssertFalse(killed) + XCTAssertFalse(service.isRemote(session)) + } + func testTheRelayCommandQuotesItsSocketForTheRemoteShell() { XCTAssertEqual( ContainerHostDriver.relayCommand(socket: "/run/it's here/a.sock"), diff --git a/macapp/WorkroomAppTests/RemoteHostIntegrationTests.swift b/macapp/WorkroomAppTests/RemoteHostIntegrationTests.swift index d6816b77f..de033aa46 100644 --- a/macapp/WorkroomAppTests/RemoteHostIntegrationTests.swift +++ b/macapp/WorkroomAppTests/RemoteHostIntegrationTests.swift @@ -178,6 +178,98 @@ final class RemoteHostIntegrationTests: XCTestCase { XCTAssertEqual(lock?.path, "\(path)/.git/index.lock") } + /// A pane's process, running the command the driver hands libghostty, with its output collected. + /// Run through `/bin/sh -c`, as libghostty runs a pane's command. No local terminal, so ssh + /// allocates none on the host either, and the attach there relays over pipes, as the Rust + /// harnesses' do. + private final class Pane: @unchecked Sendable { + let process = Process() + private let input = Pipe() + private let lock = NSLock() + private var seen = "" + + init(command: String) throws { + process.executableURL = URL(fileURLWithPath: "/bin/sh") + process.arguments = ["-c", command] + process.environment = [:] + process.standardInput = input + let output = Pipe() + process.standardOutput = output + process.standardError = output + output.fileHandleForReading.readabilityHandler = { [weak self] handle in + let data = handle.availableData + guard !data.isEmpty else { + handle.readabilityHandler = nil + return + } + self?.lock.withLock { self?.seen += String(decoding: data, as: UTF8.self) } + } + try process.run() + } + + func type(_ text: String) { input.fileHandleForWriting.write(Data(text.utf8)) } + + func read(until needle: String, within seconds: TimeInterval = 20) -> String { + let deadline = Date().addingTimeInterval(seconds) + while Date() < deadline, !lock.withLock({ seen.contains(needle) }) { + Thread.sleep(forTimeInterval: 0.05) + } + return lock.withLock { seen } + } + + /// The link dropping hard: ssh killed, with no goodbye to the host. + func dropLink() { + kill(process.processIdentifier, SIGKILL) + process.waitUntilExit() + } + } + + /// The app half of the Phase 3 acceptance (#229): a pane attached through the driver keeps its + /// session across a dropped link, and the pane that comes back (a reconnect after sleep, or a + /// relaunch) reaches the SAME shell: its pid, and a variable set before the drop. + func testARemotePaneSurvivesADroppedLinkAndReachesTheSameShell() throws { + let fixture = try fixture() + let id = UUID() + let driver = ContainerHostDriver(hosts: [id: fixture.host], directory: directory) + let session = UUID() + + let first = try Pane( + command: driver.attachCommand( + to: .remote(id), session: session, workingDirectory: "/home/workroom", restored: false)) + Thread.sleep(forTimeInterval: 1) + // Quotes split each marker, so the terminal echoing the typed line cannot satisfy the wait. + first.type("MARK=kept; echo PID=$$; echo READ\"\"Y\n") + let before = first.read(until: "READY") + let pid = try XCTUnwrap( + before.range(of: #"PID=\d+"#, options: .regularExpression).map { String(before[$0]) }, + before) + first.dropLink() + + let second = try Pane( + command: driver.attachCommand( + to: .remote(id), session: session, workingDirectory: "/home/workroom", restored: true)) + defer { second.dropLink() } + Thread.sleep(forTimeInterval: 1) + second.type("echo VALUE=$MARK PID=$$ DO\"\"NE\n") + let after = second.read(until: "DONE") + XCTAssertTrue(after.contains("VALUE=kept \(pid) DONE"), after) + XCTAssertFalse(after.contains("has ended"), after) + } + + /// A restored pane whose session ended on the host gets a shell that says so, never a fresh + /// session passed off as the old one: the host's agent refuses to create it (`--no-create`). + func testARestoredPaneWhoseSessionEndedGetsAShellThatSaysSo() throws { + let fixture = try fixture() + let id = UUID() + let driver = ContainerHostDriver(hosts: [id: fixture.host], directory: directory) + let pane = try Pane( + command: driver.attachCommand( + to: .remote(id), session: UUID(), workingDirectory: "/home/workroom", restored: true)) + defer { pane.dropLink() } + let seen = pane.read(until: "has ended") + XCTAssertTrue(seen.contains("has ended"), seen) + } + /// Nothing from the Mac's environment reaches a remote git (#229): the child runs with the /// host's `HOME` and `PATH`, no `SSH_AUTH_SOCK`, and no way for ssh to prompt. Read back from the /// child itself, through a git alias that prints its environment. diff --git a/vcs/crates/wr-agent/src/main.rs b/vcs/crates/wr-agent/src/main.rs index ab944dc7b..432a46062 100644 --- a/vcs/crates/wr-agent/src/main.rs +++ b/vcs/crates/wr-agent/src/main.rs @@ -472,6 +472,12 @@ fn run_attach(args: &[String]) -> ExitCode { // shell: `fall_back_to_shell` reads the shell, cwd and resources the app exported, and those // arrive in the same environment. let mut request = serve::AttachRequest::from_env(); + // `--no-create`: attach only if the session exists (see `AttachRequest::existing_only`). + request.existing_only = args.iter().any(|arg| arg == "--no-create"); + // `--no-spawn`: never start an agent. On a remote host that is the supervisor's job, and an + // agent started here would be the idle-exit kind a remote host must not have, racing the + // supervised one for the socket (#228). + let no_spawn = args.iter().any(|arg| arg == "--no-spawn"); let give_up = |reason: &str| -> ExitCode { if invoked_by_the_app { @@ -509,6 +515,12 @@ fn run_attach(args: &[String]) -> ExitCode { // `wr-agent protocol` probe can still reach. let mut stream = match serve::connect(&socket) { Some(stream) => stream, + None if no_spawn => { + return fall_back_to_shell( + &request, + "no session agent is listening, and on a remote host only its supervisor starts one", + ); + } None => { let binary = std::env::current_exe().unwrap_or_else(|_| PathBuf::from("wr-agent")); if serve::spawn_agent(&binary, &socket).is_err() { @@ -549,6 +561,31 @@ fn run_attach(args: &[String]) -> ExitCode { ); } + let mut decoder = EnvelopeDecoder::new(); + let mut buffer = [0u8; 8192]; + + // A restored pane's attach (`--no-create`) is answered before raw mode and the relay threads: + // a session that ended becomes the notice-and-shell the app shows locally, and that has to + // start from a cooked terminal with nothing else running. Whatever follows `Attached` stays in + // `decoder` for the loop below. + if request.existing_only { + match await_attached(&mut stream, &mut decoder, &mut buffer) { + Some(true) => {} + Some(false) => { + return fall_back_to_shell( + &request, + "the terminal that was running here has ended, so this is a new shell", + ); + } + None => { + return fall_back_to_shell( + &request, + "the session agent closed before accepting the attach", + ); + } + } + } + // Before anything reads stdin: a relay that leaves its own tty cooked is not a relay. Held to // the end of this function so every `return` below restores the terminal. let _raw = RawMode::enter(); @@ -568,14 +605,9 @@ fn run_attach(args: &[String]) -> ExitCode { std::thread::spawn(move || relay_resizes(resize_stream, columns, rows)); } - let mut decoder = EnvelopeDecoder::new(); - let mut buffer = [0u8; 8192]; let mut stdout = std::io::stdout(); loop { - match stream.read(&mut buffer) { - Ok(0) | Err(_) => break, - Ok(n) => decoder.push(&buffer[..n]), - } + // What is already decoded first: `await_attached` may have left the repaint in `decoder`. loop { match decoder.next_envelope() { Ok(Some(envelope)) => { @@ -600,7 +632,16 @@ fn run_attach(args: &[String]) -> ExitCode { .map(|b| i32::from_be_bytes(b.try_into().unwrap())) .unwrap_or(0); // `ExitCode` is a byte; a shell status is already 0-255. - return ExitCode::from(code.clamp(0, 255) as u8); + let code = code.clamp(0, 255) as u8; + // On a remote host (`--no-spawn`) this status is ssh's, and 255 is + // what ssh reports for its OWN failure, which the app answers by + // reconnecting. A session that ended with 255 must not read as a + // dropped link, so it becomes 254. + return ExitCode::from(if no_spawn && code == 255 { + 254 + } else { + code + }); } // **Deliberately NOT a fallback**, and an earlier version of this made // it one. That was wrong in the worst available direction. @@ -620,6 +661,20 @@ fn run_attach(args: &[String]) -> ExitCode { // the app side: the one that looks like success. A visible error and a // non-zero exit is the honest answer, and it leaves the session where // the user can still reach it from the detached-sessions list. + // The one exception: a `--no-create` attach whose session exited + // between the agent's check and its attach. That is "ended", not a + // failure on a live session, so it gets the same shell as a check that + // came back negative. + FrameKind::Failure + if request.existing_only + && frame.payload.starts_with(b"no session ") => + { + drop(_raw); + return fall_back_to_shell( + &request, + "the terminal that was running here has ended, so this is a new shell", + ); + } FrameKind::Failure => { eprintln!("wr-agent: {}", String::from_utf8_lossy(&frame.payload)); return ExitCode::FAILURE; @@ -632,10 +687,41 @@ fn run_attach(args: &[String]) -> ExitCode { Err(_) => return ExitCode::FAILURE, } } + match stream.read(&mut buffer) { + Ok(0) | Err(_) => break, + Ok(n) => decoder.push(&buffer[..n]), + } } ExitCode::SUCCESS } +/// Reads until the agent answers an attach: `Some(true)` for `Attached`, `Some(false)` for a +/// `Failure` (for a `--no-create` attach, the only one before `Attached` is "session ended"), and +/// `None` when the stream ends first. Envelopes after `Attached` stay in `decoder`. +fn await_attached( + stream: &mut std::os::unix::net::UnixStream, + decoder: &mut EnvelopeDecoder, + buffer: &mut [u8], +) -> Option { + loop { + while let Ok(Some(envelope)) = decoder.next_envelope() { + let mut frames = FrameDecoder::new(); + frames.push(&envelope.payload); + while let Ok(Some(frame)) = frames.next_frame() { + match frame.kind { + FrameKind::Attached => return Some(true), + FrameKind::Failure => return Some(false), + _ => {} + } + } + } + match stream.read(buffer) { + Ok(0) | Err(_) => return None, + Ok(n) => decoder.push(&buffer[..n]), + } + } +} + /// The size of the terminal this process was forked into, or zero when there is none (a pipe, a /// test harness) so the agent applies its own default rather than creating a 0x0 pty. fn terminal_size() -> (u16, u16) { diff --git a/vcs/crates/wr-agent/src/serve.rs b/vcs/crates/wr-agent/src/serve.rs index e8dea019e..afba4b05d 100644 --- a/vcs/crates/wr-agent/src/serve.rs +++ b/vcs/crates/wr-agent/src/serve.rs @@ -385,6 +385,15 @@ fn dispatch( request.rows, ) }; + // Refused, not created, for a client that asked to attach only to what exists: see + // `AttachRequest::existing_only`. Before any `Attached`, so the client can tell it + // apart from a failure on a session it is already showing. + if request.existing_only && !sessions.contains(id) { + return reply(Frame::new( + FrameKind::Failure, + format!("session ended: {}", id.to_hyphenated()).into_bytes(), + )); + } // Create on first attach, reattach afterwards. One code path, so a client that // crashed and came back does not have to know which case it is in. let result = if sessions.contains(id) { @@ -670,6 +679,12 @@ pub struct AttachRequest { pub rows: u16, /// Passed to the child verbatim. The client's environment, not the agent's. pub env: Vec<(OsString, OsString)>, + /// Attach only if the session exists; never create it (`CREATE=0`). A restored pane sends it, + /// because creating would hand back a fresh shell that looks like the one that was running + /// there. The app asks first on a local host (`confirmBeforeAttach`); a pane on a remote host + /// has no such channel, and one request cannot race the answer. Inverted so the default, + /// and an older client that never sends it, keeps create-on-attach. + pub existing_only: bool, } impl AttachRequest { @@ -699,6 +714,9 @@ impl AttachRequest { } put("COLS", self.columns.to_string().as_bytes()); put("ROWS", self.rows.to_string().as_bytes()); + if self.existing_only { + put("CREATE", b"0"); + } for (key, value) in &self.env { let mut entry = b"ENV:".to_vec(); entry.extend_from_slice(key.as_bytes()); @@ -736,6 +754,7 @@ impl AttachRequest { b"ROWS" => { request.rows = value.to_string_lossy().parse().unwrap_or(0); } + b"CREATE" => request.existing_only = value == "0", _ => { if let Some(name) = key.strip_prefix(b"ENV:") { request.env.push((OsString::from_vec(name.to_vec()), value)); @@ -774,6 +793,7 @@ impl AttachRequest { columns: 0, rows: 0, env, + existing_only: false, } } } @@ -959,6 +979,7 @@ mod tests { (OsString::from("TERM"), OsString::from("xterm-ghostty")), (OsString::from("PATH"), OsString::from("/usr/bin:/bin")), ], + existing_only: true, }; let decoded = AttachRequest::decode(&request.encode()).expect("decode"); assert_eq!(decoded, request); diff --git a/vcs/crates/wr-agent/tests/attach_falls_back_to_a_shell.rs b/vcs/crates/wr-agent/tests/attach_falls_back_to_a_shell.rs index 45227d468..78414a470 100644 --- a/vcs/crates/wr-agent/tests/attach_falls_back_to_a_shell.rs +++ b/vcs/crates/wr-agent/tests/attach_falls_back_to_a_shell.rs @@ -44,8 +44,16 @@ fn attach_with_stdin(env: &[(&str, &str)], stdin: &str) -> (String, Option) } fn attach_inner(env: &[(&str, &str)], stdin: Option<&str>) -> (String, Option) { + attach_with_args(&[], env, stdin) +} + +fn attach_with_args( + args: &[&str], + env: &[(&str, &str)], + stdin: Option<&str>, +) -> (String, Option) { let mut command = Command::new(agent_binary()); - command.arg("attach"); + command.arg("attach").args(args); command.env_clear(); command.env("PATH", "/usr/bin:/bin"); for (key, value) in env { @@ -701,3 +709,183 @@ fn a_deleted_working_directory_falls_back_to_home_before_root() { {output:?}" ); } + +/// A healthy agent on `socket`, killed when the guard drops. +struct Agent(std::process::Child); + +impl Drop for Agent { + fn drop(&mut self) { + let _ = self.0.kill(); + let _ = self.0.wait(); + } +} + +fn start_agent(socket: &std::path::Path) -> Agent { + let agent = Agent( + Command::new(agent_binary()) + .args(["serve", "--socket"]) + .arg(socket) + .args(["--idle-timeout", "30"]) + .stdout(Stdio::null()) + .stderr(Stdio::null()) + .spawn() + .expect("spawn agent"), + ); + let deadline = std::time::Instant::now() + std::time::Duration::from_secs(5); + while !socket.exists() && std::time::Instant::now() < deadline { + std::thread::sleep(std::time::Duration::from_millis(20)); + } + assert!(socket.exists(), "the agent never bound its socket"); + agent +} + +/// `--no-spawn` (a pane on a remote host, #229): with no agent listening, a shell rather than a +/// spawned agent. On a remote host the supervisor starts the agent, and one started here would be +/// the idle-exit kind, racing the supervised one for the socket. +#[test] +fn no_spawn_falls_back_to_a_shell_instead_of_starting_an_agent() { + let dir = scratch("no-spawn"); + let socket = dir.join("a.sock"); + let (output, _) = attach_with_args( + &["--no-spawn"], + &[ + ( + "WORKROOM_SESSION_ID", + "6B9B968D-0BD7-4172-850A-A373DA73BC72", + ), + ("WORKROOM_SESSION_SOCKET", socket.to_str().unwrap()), + ("WORKROOM_SESSION_SHELL", "/bin/sh"), + ("WORKROOM_SESSION_CWD", dir.to_str().unwrap()), + ( + "WORKROOM_SESSION_COMMAND", + "echo FELL-BACK fb=[${WORKROOM_SESSION_FALLBACK:-unset}]", + ), + ], + None, + ); + std::thread::sleep(std::time::Duration::from_millis(500)); + let spawned = socket.exists(); + let _ = std::fs::remove_dir_all(&dir); + assert!( + output.contains("FELL-BACK fb=[1]"), + "expected the fallback shell. got: {output:?}" + ); + assert!(!spawned, "an agent was started despite --no-spawn"); +} + +/// `--no-create` (a restored pane): a session the agent does not hold is refused, not created, +/// and the pane becomes a shell that says so. Created instead, it would be a fresh shell passed off +/// as the one that was running there. +#[test] +fn no_create_turns_an_ended_session_into_a_shell_with_a_notice() { + let dir = scratch("no-create-ended"); + let socket = dir.join("a.sock"); + let _agent = start_agent(&socket); + let (output, _) = attach_with_args( + &["--no-create"], + &[ + ( + "WORKROOM_SESSION_ID", + "6B9B968D-0BD7-4172-850A-A373DA73BC73", + ), + ("WORKROOM_SESSION_SOCKET", socket.to_str().unwrap()), + ("WORKROOM_SESSION_SHELL", "/bin/sh"), + ("WORKROOM_SESSION_CWD", dir.to_str().unwrap()), + ( + "WORKROOM_SESSION_COMMAND", + "echo NEW-SHELL fb=[${WORKROOM_SESSION_FALLBACK:-unset}]", + ), + ], + None, + ); + let listed = Command::new(agent_binary()) + .args(["list", "--socket"]) + .arg(&socket) + .output() + .expect("list"); + let _ = std::fs::remove_dir_all(&dir); + assert!( + output.contains("has ended") && output.contains("NEW-SHELL fb=[1]"), + "expected the ended-session notice and a fallback shell. got: {output:?}" + ); + assert!( + !String::from_utf8_lossy(&listed.stdout).contains("6b9b968d"), + "the agent created the session anyway: {:?}", + String::from_utf8_lossy(&listed.stdout) + ); +} + +/// And a session that exists is reattached as usual: `--no-create` only refuses to make one. +#[test] +fn no_create_reattaches_a_session_that_exists() { + let dir = scratch("no-create-live"); + let socket = dir.join("a.sock"); + let _agent = start_agent(&socket); + let env = [ + ( + "WORKROOM_SESSION_ID", + "6B9B968D-0BD7-4172-850A-A373DA73BC74", + ), + ("WORKROOM_SESSION_SOCKET", socket.to_str().unwrap()), + ("WORKROOM_SESSION_SHELL", "/bin/sh"), + ("WORKROOM_SESSION_CWD", dir.to_str().unwrap()), + ( + "WORKROOM_SESSION_COMMAND", + // Its own `sh -c`: a session's command is run as `exec `, which would end at + // `sleep`. + "sh -c 'sleep 2; echo STILL-THE-SAME fb=[${WORKROOM_SESSION_FALLBACK:-unset}]'", + ), + ]; + // The first attach creates it; killed at once, it leaves the session running with no client. + let mut first = Command::new(agent_binary()); + first.arg("attach").env_clear().env("PATH", "/usr/bin:/bin"); + for (key, value) in env { + first.env(key, value); + } + let mut first = first + .stdin(Stdio::null()) + .stdout(Stdio::null()) + .stderr(Stdio::null()) + .spawn() + .expect("spawn first attach"); + std::thread::sleep(std::time::Duration::from_millis(500)); + let _ = first.kill(); + let _ = first.wait(); + + let (output, _) = attach_with_args(&["--no-create"], &env, None); + let _ = std::fs::remove_dir_all(&dir); + assert!( + output.contains("STILL-THE-SAME fb=[unset]"), + "expected the running session's own output. got: {output:?}" + ); + assert!(!output.contains("has ended"), "got: {output:?}"); +} + +/// On a remote host (`--no-spawn`) the attach's exit status is ssh's, and 255 is ssh's own +/// failure, which the app answers by reconnecting. So a session that ended with 255 exits 254 +/// there; locally it stays 255. +#[test] +fn a_remote_attach_never_exits_255_for_a_session_that_did() { + let dir = scratch("exit-255"); + let socket = dir.join("a.sock"); + let _agent = start_agent(&socket); + let run = |id: &str, args: &[&str]| { + attach_with_args( + args, + &[ + ("WORKROOM_SESSION_ID", id), + ("WORKROOM_SESSION_SOCKET", socket.to_str().unwrap()), + ("WORKROOM_SESSION_SHELL", "/bin/sh"), + ("WORKROOM_SESSION_CWD", dir.to_str().unwrap()), + ("WORKROOM_SESSION_COMMAND", "sh -c 'exit 255'"), + ], + None, + ) + .1 + }; + let local = run("6B9B968D-0BD7-4172-850A-A373DA73BC75", &[]); + let remote = run("6B9B968D-0BD7-4172-850A-A373DA73BC76", &["--no-spawn"]); + let _ = std::fs::remove_dir_all(&dir); + assert_eq!(local, Some(255)); + assert_eq!(remote, Some(254)); +} From 92d562929bd19e2db97ff38026775d5d8b278338 Mon Sep 17 00:00:00 2001 From: Joel Moss Date: Thu, 24 Sep 2026 22:31:33 +0100 Subject: [PATCH 5/7] style(macapp): swift-format the remote host changes `make app-lint` (the CI gate) flagged line lengths and wrapping in three files that per-file lint runs had passed. Formatter output only. Co-Authored-By: Claude Opus 5.5 --- .../Core/ContainerHostDriver.swift | 6 ++++-- .../Session/PersistentSessionService.swift | 3 ++- macapp/WorkroomApp/Core/VCSWriting.swift | 19 ++++++++++--------- 3 files changed, 16 insertions(+), 12 deletions(-) diff --git a/macapp/WorkroomApp/Core/ContainerHostDriver.swift b/macapp/WorkroomApp/Core/ContainerHostDriver.swift index bc49ee9e1..152a490b4 100644 --- a/macapp/WorkroomApp/Core/ContainerHostDriver.swift +++ b/macapp/WorkroomApp/Core/ContainerHostDriver.swift @@ -44,7 +44,8 @@ struct ContainerHostDriver: HostTerminalDriver { guard case .remote(let id) = host, let target = hosts[id] else { throw HostDriverError.unknownHost(host) } - let config = try Self.writeConfiguration(for: target, in: directory.appendingPathComponent(id.uuidString)) + let config = try Self.writeConfiguration( + for: target, in: directory.appendingPathComponent(id.uuidString)) return try HostStream.spawn( URL(fileURLWithPath: "/usr/bin/ssh"), ["-F", config.path, Self.alias, Self.relayCommand(socket: target.agentSocket)], @@ -94,7 +95,8 @@ struct ContainerHostDriver: HostTerminalDriver { "WORKROOM_SESSION_SOCKET=\(socket)", "WORKROOM_SESSION_CWD=\(workingDirectory)", ] - return (["env"] + variables + ["wr-agent", "attach", "--no-spawn"] + return + (["env"] + variables + ["wr-agent", "attach", "--no-spawn"] + (restored ? ["--no-create"] : [])) .map(shellQuoted).joined(separator: " ") } diff --git a/macapp/WorkroomApp/Core/Session/PersistentSessionService.swift b/macapp/WorkroomApp/Core/Session/PersistentSessionService.swift index 7491ccebe..a83031f4c 100644 --- a/macapp/WorkroomApp/Core/Session/PersistentSessionService.swift +++ b/macapp/WorkroomApp/Core/Session/PersistentSessionService.swift @@ -385,7 +385,8 @@ final class PersistentSessionService { restored: restored) } catch { logger.error( - "no attach command for remote session \(sessionID.uuidString, privacy: .public): \(error)") + "no attach command for remote session \(sessionID.uuidString, privacy: .public): \(error)" + ) // Not nil: nil opens a plain shell, which for this pane would run on the Mac while it // reads as the remote workroom's. A pane that says why it could not reach the host is // honest; one on the wrong machine is not. diff --git a/macapp/WorkroomApp/Core/VCSWriting.swift b/macapp/WorkroomApp/Core/VCSWriting.swift index 180958b75..e994b60f2 100644 --- a/macapp/WorkroomApp/Core/VCSWriting.swift +++ b/macapp/WorkroomApp/Core/VCSWriting.swift @@ -1241,7 +1241,9 @@ struct CLIVCSWriter: LocalVCSWriting, Sendable { // So when the symptoms are lock-shaped, ask the DISK instead of the message. Deliberately last: any // failure git explains properly keeps its own classification, and this only speaks for the ones it // doesn't. - if lockSymptom(err), let lock = existingLockFile(gitDir: gitDir, disk: disk) { return .locked(lock) } + if lockSymptom(err), let lock = existingLockFile(gitDir: gitDir, disk: disk) { + return .locked(lock) + } let trimmed = err.trimmingCharacters(in: .whitespacesAndNewlines) // Killed rather than finished, with nothing to say for itself: `exitCode` is the SIGNAL number, // so the fallback below would render "git exited 15" — literally the dialog `a64e4269` ("stop @@ -1389,12 +1391,12 @@ struct CLIVCSWriter: LocalVCSWriting, Sendable { } static let sequencerMarkers: [(String, String)] = [ - ("CHERRY_PICK_HEAD", "cherry-pick"), - ("REVERT_HEAD", "revert"), - ("rebase-merge", "rebase"), - ("rebase-apply", "rebase"), - ("BISECT_LOG", "bisect"), - ("MERGE_HEAD", "merge"), + ("CHERRY_PICK_HEAD", "cherry-pick"), + ("REVERT_HEAD", "revert"), + ("rebase-merge", "rebase"), + ("rebase-apply", "rebase"), + ("BISECT_LOG", "bisect"), + ("MERGE_HEAD", "merge"), ] /// The lock file a failure is complaining about, or nil if we can't point at one. @@ -1653,8 +1655,7 @@ struct CLIVCSWriter: LocalVCSWriting, Sendable { { // Reads take no lock, so they stay ungated. let list = await run(Self.jjBookmarkListArgs(), in: path, timeout: refTimeout) - if let failure = await classify(list, action: .fetch, tool: "jj", at: path, withGitDir: false) - { + if let failure = await classify(list, action: .fetch, tool: "jj", at: path, withGitDir: false) { if case .timedOut = failure { return .keepPrior } return .failed(failure) } From 4b83d94add081c32067904deec2ccf05c6c265b1 Mon Sep 17 00:00:00 2001 From: Joel Moss Date: Thu, 24 Sep 2026 22:35:19 +0100 Subject: [PATCH 6/7] fix(macapp): address #237 review findings - A remote jj command's request deadline now covers the up-to-30s barrier wait the agent does before starting it, so a slow barrier cannot turn a finished command into an unknown outcome. - Ending a remote session keeps its registration: the session still runs on its host, and without it a retry went to the local agent and a reattach opened a new session on the Mac. - The agent reads `sessions.contains` once for an attach, so a session that exits between the `--no-create` check and the choice to create cannot be created. Co-Authored-By: Claude Opus 5.5 --- macapp/WorkroomApp/Core/AgentCommandRunner.swift | 7 ++++++- .../Core/Session/PersistentSessionService.swift | 6 ++++-- macapp/WorkroomAppTests/HostStreamTests.swift | 7 ++++++- vcs/crates/wr-agent/src/serve.rs | 9 +++++++-- 4 files changed, 23 insertions(+), 6 deletions(-) diff --git a/macapp/WorkroomApp/Core/AgentCommandRunner.swift b/macapp/WorkroomApp/Core/AgentCommandRunner.swift index c3182733a..ced392544 100644 --- a/macapp/WorkroomApp/Core/AgentCommandRunner.swift +++ b/macapp/WorkroomApp/Core/AgentCommandRunner.swift @@ -72,7 +72,12 @@ struct AgentCommandRunner: StatusCommandRunning, Sendable { // once it holds the flock — see the comment there. Shielding HERE instead would also cover // `remoteState`'s ungated reads, and a superseded `RemoteStateModel` refresh would then squat // one of this connection's 32 shared slots until the agent answered. - reply = try await connection.request(request, timeout: timeout + 15) + // + // Plus the barrier's own wait where the agent takes one (a remote jj command): it can wait + // up to 30s for the barrier before the command starts (`SnapshotLock::acquire`), and a + // deadline that did not cover that would give up on a command still running. + let barrierWait: TimeInterval = request.barrierRoot == nil ? 0 : 30 + reply = try await connection.request(request, timeout: timeout + 15 + barrierWait) } catch let error as VCSError { // Raised before anything left this process — today only the 1 MiB single-envelope request // ceiling. Nothing ran, and the workroom is fine. diff --git a/macapp/WorkroomApp/Core/Session/PersistentSessionService.swift b/macapp/WorkroomApp/Core/Session/PersistentSessionService.swift index a83031f4c..8f616562a 100644 --- a/macapp/WorkroomApp/Core/Session/PersistentSessionService.swift +++ b/macapp/WorkroomApp/Core/Session/PersistentSessionService.swift @@ -524,8 +524,10 @@ final class PersistentSessionService { // Never routed to a local helper, which would be asked to kill an id it does not hold. Ending // a session on its host belongs to that host's lifecycle (Phase 4); until then it is reported // as not killed, which is true, and nothing that gates on this deletes a local directory for - // it. - if remoteSessions.removeValue(forKey: sessionID) != nil { + // it. The registration STAYS: the session is still running there, and without it a retry + // would ask the local agent (which "kills" an id it never held) and a reattach would open a + // new session on this Mac. + if isRemote(sessionID) { logger.error( "remote session \(sessionID.uuidString, privacy: .public) left running on its host") return false diff --git a/macapp/WorkroomAppTests/HostStreamTests.swift b/macapp/WorkroomAppTests/HostStreamTests.swift index 5532051cf..f955c9a94 100644 --- a/macapp/WorkroomAppTests/HostStreamTests.swift +++ b/macapp/WorkroomAppTests/HostStreamTests.swift @@ -204,7 +204,12 @@ final class HostStreamTests: XCTestCase { // and reports it not killed, which is true. let killed = await service.endSession(sessionID: session) XCTAssertFalse(killed) - XCTAssertFalse(service.isRemote(session)) + // Still remote: the session runs on, and a retry or a reattach must not go local. + XCTAssertTrue(service.isRemote(session)) + let again = await service.endSession(sessionID: session) + XCTAssertFalse(again) + XCTAssertTrue( + service.attachCommand(forSession: session, restored: true)?.contains("/usr/bin/ssh") == true) } func testTheRelayCommandQuotesItsSocketForTheRemoteShell() { diff --git a/vcs/crates/wr-agent/src/serve.rs b/vcs/crates/wr-agent/src/serve.rs index afba4b05d..3c8aaecbd 100644 --- a/vcs/crates/wr-agent/src/serve.rs +++ b/vcs/crates/wr-agent/src/serve.rs @@ -388,7 +388,12 @@ fn dispatch( // Refused, not created, for a client that asked to attach only to what exists: see // `AttachRequest::existing_only`. Before any `Attached`, so the client can tell it // apart from a failure on a session it is already showing. - if request.existing_only && !sessions.contains(id) { + // + // Read ONCE, for both this and the choice below: read twice, a session that exited in + // between would pass the refusal and then be created. With one read, that race ends in + // `attach()` finding nothing, which the client already takes as "ended". + let exists = sessions.contains(id); + if request.existing_only && !exists { return reply(Frame::new( FrameKind::Failure, format!("session ended: {}", id.to_hyphenated()).into_bytes(), @@ -396,7 +401,7 @@ fn dispatch( } // Create on first attach, reattach afterwards. One code path, so a client that // crashed and came back does not have to know which case it is in. - let result = if sessions.contains(id) { + let result = if exists { attach() } else { // Not `` with no arguments: see `shell::invocation`. Spawning the shell From f3b8f8b23bec84a09fd217d39ab81b5c418fa4c8 Mon Sep 17 00:00:00 2001 From: Joel Moss Date: Thu, 24 Sep 2026 23:03:59 +0100 Subject: [PATCH 7/7] fix(macapp): read a carrier's exit status only once it has exited HostStream.failure() read terminationReason before checking isRunning. Foundation raises an Objective-C exception for a process not yet reaped, which Swift cannot catch, so a carrier still exiting when the wait gave up would crash the app. From review on #237. Co-Authored-By: Claude Opus 5.5 --- macapp/WorkroomApp/Core/HostDriver.swift | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/macapp/WorkroomApp/Core/HostDriver.swift b/macapp/WorkroomApp/Core/HostDriver.swift index e4fba9c77..18a30e3a3 100644 --- a/macapp/WorkroomApp/Core/HostDriver.swift +++ b/macapp/WorkroomApp/Core/HostDriver.swift @@ -192,11 +192,16 @@ final class HostStream: @unchecked Sendable { let reason = said.trimmingCharacters(in: .whitespacesAndNewlines) if !reason.isEmpty { return reason } let name = process.executableURL?.lastPathComponent ?? "carrier" + // `isRunning` first: `terminationReason` and `terminationStatus` raise an Objective-C + // exception for a process that has not been reaped yet, which Swift cannot catch. A carrier + // stopped a moment ago can still be running once the wait above gives up. + if process.isRunning { return "\(name) did not answer in time" } // Our own SIGTERM, from `end()`: the carrier was still running when the handshake gave up. - let stoppedByUs = - endedByUs && process.terminationReason == .uncaughtSignal - && process.terminationStatus == SIGTERM - if process.isRunning || stoppedByUs { return "\(name) did not answer in time" } + if endedByUs, process.terminationReason == .uncaughtSignal, + process.terminationStatus == SIGTERM + { + return "\(name) did not answer in time" + } return "\(name) exited with status \(process.terminationStatus)" }