From 44a20cef2ff0132e2ade6c4388c7e4182f6cf069 Mon Sep 17 00:00:00 2001 From: Joel Moss Date: Sun, 4 Oct 2026 01:31:58 +0100 Subject: [PATCH 01/14] feat(macapp): forward ports from a remote workroom's host (#254) The Ports section reached only this Mac's agent: `PortForwardingModel` was one shared model on the `.local` host. It is now one model per host, kept for the launch, and the section shows the selected workroom's host. A remote workroom is its own host, so its ports are forwarded from there. A remote workroom the app can't reach shows no Ports section, rather than this Mac's. A forward still never connects a host. Selecting the workroom connects it, and connecting from `+` could start a container the user stopped. Closes the last criterion of #208. Co-Authored-By: Claude Opus 5.5 --- .../Core/AgentPortForwarding.swift | 59 ++++++++++++++----- macapp/WorkroomApp/Views/ChangesPanel.swift | 38 ++++++++---- .../AgentPortForwardingTests.swift | 41 +++++++++++++ .../RemoteHostIntegrationTests.swift | 23 ++++++++ 4 files changed, 133 insertions(+), 28 deletions(-) diff --git a/macapp/WorkroomApp/Core/AgentPortForwarding.swift b/macapp/WorkroomApp/Core/AgentPortForwarding.swift index 6c4cf2131..a14e2bdc6 100644 --- a/macapp/WorkroomApp/Core/AgentPortForwarding.swift +++ b/macapp/WorkroomApp/Core/AgentPortForwarding.swift @@ -894,33 +894,56 @@ private final class ForwardedConnection: @unchecked Sendable { } } -/// The forwards the user has asked for on the local box, and the listeners serving them. +/// The forwards the user has asked for on one host, and the listeners serving them. /// -/// One list, not one per host: only the local host has an agent today, and `wakefulness()` is shared -/// the same way. Forwards are per HOST rather than per workroom, so two workrooms on this box see -/// the same list. +/// One model per HOST (`model(for:)`), not per workroom: a forward runs on the host's agent +/// connection, so two workrooms on one host see the same list. A remote workroom is its own host +/// (#254), so in practice each remote workroom has its own list and every local one shares this +/// Mac's. @MainActor final class PortForwardingModel: ObservableObject { - static let shared = PortForwardingModel() + /// Each host's model, made on first use and kept for the launch: its listeners must outlive the + /// Ports panel showing another workroom and coming back. A host that goes away takes its + /// forwards with its connection (`watch()`), leaving an empty model behind. + private static var models: [HostID: PortForwardingModel] = [:] + + static func model(for host: HostID) -> PortForwardingModel { + if let model = models[host] { return model } + let model = PortForwardingModel(host: host, transport: .live(host)) + models[host] = model + return model + } /// How the model reaches the agent, so a test can hand it a scripted agent and a connection /// stream it controls — the shape `WakefulnessModel.Transport` set. struct Transport { - /// The service and the lease of the connection it runs on. Never spawns an agent; the reasoning - /// is on `LocalAgentVCS.forwarding()`, the decision point. + /// The service and the lease of the connection it runs on. Never spawns an agent or connects a + /// host; the reasoning is on `LocalAgentVCS.forwarding()`, the decision point. var forwarding: @Sendable () async throws -> (HostConnectionManager.Lease, AgentForwardService) var updates: @Sendable () async -> AsyncStream /// The connected lease right now, or nil. What `add()` reconciles against: the watch's last /// snapshot can lag a reconnect that `forwarding` has already seen. var current: @Sendable () async -> HostConnectionManager.Lease? - static let live = Transport( - forwarding: { try await LocalAgentVCS.shared.forwarding() }, - updates: { await HostConnectionManager.shared.updates(for: .local) }, - current: { - let snapshot = await HostConnectionManager.shared.snapshot(for: .local) - return snapshot.status == .connected ? snapshot.lease : nil - }) + static func live(_ host: HostID) -> Transport { + var transport = on(host, manager: .shared) + if host == .local { transport.forwarding = { try await LocalAgentVCS.shared.forwarding() } } + return transport + } + + /// `host`'s connection in `manager`. A remote host's connection is made by selecting its + /// workroom (`RemoteHosts.ensureConnected`, through the inspector's reads), so a forward asks + /// for the one there is and never makes one: the local reasoning in `LocalAgentVCS.forwarding()` + /// holds for a host too, and connecting one could start a container the user stopped. + static func on(_ host: HostID, manager: HostConnectionManager) -> Transport { + Transport( + forwarding: { try await manager.forwarding(host: host) }, + updates: { await manager.updates(for: host) }, + current: { + let snapshot = await manager.snapshot(for: host) + return snapshot.status == .connected ? snapshot.lease : nil + }) + } } struct Entry: Identifiable, Equatable { @@ -943,11 +966,12 @@ final class PortForwardingModel: ObservableObject { @Published private(set) var forwards: [Entry] = [] /// What went wrong adding one, or why the last forwards went away. Cleared by the next attempt. @Published private(set) var message: String? - /// Whether the local agent connection is up, for the row's caption: the controls are useless + /// Whether the host's agent connection is up, for the row's caption: the controls are useless /// without one, and a caption says so before the user finds out from `+`. @Published private(set) var connected = false @Published var draft = "" + let host: HostID private let transport: Transport private var listeners: [UUID: PortForward] = [:] private var watching = false @@ -957,7 +981,8 @@ final class PortForwardingModel: ObservableObject { /// How many snapshots the watch has consumed, for the tests. private(set) var snapshotsSeen = 0 - init(transport: Transport = .live) { + init(host: HostID = .local, transport: Transport) { + self.host = host self.transport = transport } @@ -1003,6 +1028,8 @@ final class PortForwardingModel: ObservableObject { /// `VCSError` is not `LocalizedError`, so interpolating it prints its Swift case. static func describe(_ error: Error) -> String { switch error { + case RepositoryRoutingError.unavailable(.remote): + return "This workroom's host is not connected." case RepositoryRoutingError.unavailable: return "No agent is running on this Mac. Open a workroom to start one." case VCSError.backendVersion(let detail): diff --git a/macapp/WorkroomApp/Views/ChangesPanel.swift b/macapp/WorkroomApp/Views/ChangesPanel.swift index 214356357..7280178f6 100644 --- a/macapp/WorkroomApp/Views/ChangesPanel.swift +++ b/macapp/WorkroomApp/Views/ChangesPanel.swift @@ -477,16 +477,19 @@ struct WakefulnessBadge: View { } } -/// Ports forwarded from the agent's box to this Mac (issue #208). +/// Ports forwarded from a host's agent to this Mac (issues #208, #254). /// -/// Per HOST, not per workroom — two workrooms on the same box share this list, which is why it sits -/// above the change list rather than inside the workroom-dependent part of it. Nothing is persisted -/// across launches: the agent closes every forwarded socket when the client detaches, so a list -/// restored at launch would name addresses that no longer answer. +/// Per HOST, not per workroom — two local workrooms share this Mac's list, which is why it sits +/// above the change list rather than inside the workroom-dependent part of it; a remote workroom is +/// its own host and shows its own. Nothing is persisted across launches: the agent closes every +/// forwarded socket when the client detaches, so a list restored at launch would name addresses +/// that no longer answer. private struct PortsSection: View { - @ObservedObject private var model = PortForwardingModel.shared + @ObservedObject var model: PortForwardingModel private let theme = ThemeService.shared + private var machine: String { model.host == .local ? "this Mac" : "this workroom's host" } + var body: some View { VStack(alignment: .leading, spacing: 6) { HStack(spacing: 6) { @@ -500,8 +503,7 @@ private struct PortsSection: View { .frame(width: 48) .onSubmit { Task { await model.add() } } .accessibilityIdentifier("ports.field") - InspectorHeaderButton(systemImage: "plus", help: "Forward a port from this machine's agent") - { + InspectorHeaderButton(systemImage: "plus", help: "Forward a port from \(machine)") { Task { await model.add() } } .accessibilityIdentifier("ports.add") @@ -519,7 +521,9 @@ private struct PortsSection: View { } .padding(.horizontal, 12) .padding(.vertical, 8) - .task { model.watch() } + // Keyed by host: the section is the same view across workrooms, and each host's model is + // watched from the first time it is shown. + .task(id: model.host) { model.watch() } } @ViewBuilder @@ -884,9 +888,11 @@ struct ChangesPanel: View { var body: some View { VStack(alignment: .leading, spacing: 0) { - // Per box, so it must not vanish when no workroom is selected; see `PortsSection`. - PortsSection() - Divider() + // Per host, so it must not vanish when no workroom is selected; see `PortsSection`. + if let host = portsHost { + PortsSection(model: .model(for: host)) + Divider() + } Group { if let sid = store.inspectorTargetID, sid.isStatusable { content(for: sid) @@ -898,6 +904,14 @@ struct ChangesPanel: View { .frame(maxWidth: .infinity, alignment: .leading) } + /// The host whose ports the section forwards: a remote workroom's own host (#254), else this + /// Mac's. Nil for a remote workroom this app can't reach, whose ports this Mac's list would only + /// misname. + private var portsHost: HostID? { + guard let workroom = store.selectedWorkroom, workroom.isRemote else { return .local } + return workroom.reachableHost.map(HostID.remote) + } + @ViewBuilder private func content(for sid: SidebarID) -> some View { let target = store.target(for: sid) diff --git a/macapp/WorkroomAppTests/AgentPortForwardingTests.swift b/macapp/WorkroomAppTests/AgentPortForwardingTests.swift index b4f3b2f15..e1e18c807 100644 --- a/macapp/WorkroomAppTests/AgentPortForwardingTests.swift +++ b/macapp/WorkroomAppTests/AgentPortForwardingTests.swift @@ -944,6 +944,47 @@ final class PortForwardingModelTests: XCTestCase { old.draft = "5173" await old.add() XCTAssertEqual(old.message, "Agent does not support port forwarding.") + + // A remote workroom's host is not "this Mac" (#254). + let remote = try await model( + lease: lease(), forwarding: { throw RepositoryRoutingError.unavailable(.remote(UUID())) }) + remote.draft = "5173" + await remote.add() + XCTAssertEqual(remote.message, "This workroom's host is not connected.") + } + + /// One model per host, kept: the Ports section going to another workroom and back finds its + /// forwards where it left them, and two hosts never share a list (#254). + func testEachHostHasItsOwnModelForTheLaunch() { + let host = HostID.remote(UUID()) + XCTAssertTrue(PortForwardingModel.model(for: host) === PortForwardingModel.model(for: host)) + XCTAssertTrue(PortForwardingModel.model(for: .local) === PortForwardingModel.model(for: .local)) + XCTAssertFalse(PortForwardingModel.model(for: host) === PortForwardingModel.model(for: .local)) + XCTAssertEqual(PortForwardingModel.model(for: host).host, host) + } + + /// A remote host's transport reads that host's connection in the manager and never another's. + /// With only this Mac connected, a forward on the host is refused as the host's, not served by + /// the local agent. + func testARemoteHostsTransportAsksForThatHostsConnection() async throws { + let agent = try FakeAgent(version: 5, forward: true) + fakes.append(agent) + let manager = HostConnectionManager() + _ = try await manager.connect(host: .local) { + try await AgentVCSConnection.connect(host: .local, socketPath: agent.socketPath) + } + let host = HostID.remote(UUID()) + let transport = PortForwardingModel.Transport.on(host, manager: manager) + do { + _ = try await transport.forwarding() + XCTFail("a remote host with no connection was served") + } catch RepositoryRoutingError.unavailable(let refused) { + XCTAssertEqual(refused, host) + } + let current = await transport.current() + XCTAssertNil(current) + let local = await PortForwardingModel.Transport.on(.local, manager: manager).current() + XCTAssertNotNil(local) } /// A refusal on a row is a fact about one connection — the dev server was not up yet — and the diff --git a/macapp/WorkroomAppTests/RemoteHostIntegrationTests.swift b/macapp/WorkroomAppTests/RemoteHostIntegrationTests.swift index c2f03719d..d69bd9833 100644 --- a/macapp/WorkroomAppTests/RemoteHostIntegrationTests.swift +++ b/macapp/WorkroomAppTests/RemoteHostIntegrationTests.swift @@ -156,6 +156,29 @@ final class RemoteHostIntegrationTests: XCTestCase { XCTAssertEqual(banner, "SSH-2.0-") } + /// The Ports panel's model on a remote workroom's host (#254, the last of #208): a port added + /// there is the HOST's port, and a client on this Mac reaches the process listening on it. The + /// fixture's sshd is that process; it speaks first, so the banner proves the bytes came from the + /// host and not from anything on this Mac. + @MainActor + func testThePortsModelForwardsAPortFromARemoteHost() async throws { + let fixture = try 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 model = PortForwardingModel(host: host, transport: .on(host, manager: manager)) + model.draft = "22" + await model.add() + XCTAssertNil(model.message) + let entry = try XCTUnwrap(model.forwards.first) + defer { model.remove(entry.id) } + let client = try TCPClient(port: entry.localPort) + defer { client.close() } + let banner = String(decoding: try client.read(8, timeout: 10), as: UTF8.self) + 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. From c4e3dcc2009d3a281e0e6e4dce1cd659dfc5480b Mon Sep 17 00:00:00 2001 From: Joel Moss Date: Sun, 4 Oct 2026 01:35:38 +0100 Subject: [PATCH 02/14] feat(macapp): show each remote workroom's busy or idle state in the sidebar (#254) A remote workroom is its own box, so its wakefulness verdict is its own. Its sidebar row now carries the inspector's wakefulness badge, backed by one `WakefulnessModel` per remote host. The badge polls only while the row is on screen and never connects the host: a disconnected host shows nothing. A remote agent was never started with this Mac's wakefulness settings, so a remote badge's tooltip leaves out the settings-mismatch sentence. The Linux agent always runs the classifier, so no agent or host-setup change is needed. Co-Authored-By: Claude Opus 5.5 --- .../WorkroomApp/Core/AgentWakefulness.swift | 21 +++++++++++ macapp/WorkroomApp/Views/ChangesPanel.swift | 23 ++++++++---- macapp/WorkroomApp/Views/ProjectSidebar.swift | 7 ++++ .../AgentWakefulnessTests.swift | 37 +++++++++++++++++++ .../RemoteHostIntegrationTests.swift | 15 ++++++++ 5 files changed, 95 insertions(+), 8 deletions(-) diff --git a/macapp/WorkroomApp/Core/AgentWakefulness.swift b/macapp/WorkroomApp/Core/AgentWakefulness.swift index 2b580b0a9..d0b2efb6f 100644 --- a/macapp/WorkroomApp/Core/AgentWakefulness.swift +++ b/macapp/WorkroomApp/Core/AgentWakefulness.swift @@ -418,6 +418,16 @@ final class WakefulnessModel: ObservableObject { status: { try await LocalAgentVCS.shared.wakefulness(connecting: .never).status() }, keep: { try await LocalAgentVCS.shared.wakefulness(connecting: .spawn).keep() }, prompts: { try await LocalAgentVCS.shared.wakefulness(connecting: .reconnect).prompts }) + + /// A remote host's service in `manager` (#254). None of the three connects the host: a poll + /// never does, and the host's connection is made by selecting its workroom or by the status + /// sweep, either of which a remote "keep" or watch can wait for. + static func on(_ host: HostID, manager: HostConnectionManager) -> Transport { + Transport( + status: { try await manager.wakefulness(host: host).status() }, + keep: { try await manager.wakefulness(host: host).keep() }, + prompts: { try await manager.wakefulness(host: host).prompts }) + } } private let transport: Transport @@ -426,6 +436,17 @@ final class WakefulnessModel: ObservableObject { self.transport = transport } + /// Each remote host's model, made on first use and kept for the launch, as `PortForwardingModel`'s + /// are. This Mac's is `shared`, whose transport may start an agent. + private static var hosts: [UUID: WakefulnessModel] = [:] + + static func model(forHost id: UUID) -> WakefulnessModel { + if let model = hosts[id] { return model } + let model = WakefulnessModel(transport: .on(.remote(id), manager: .shared)) + hosts[id] = model + return model + } + /// Nil until a poll succeeds, and again once one fails: no agent, an agent that predates the /// service, or a macOS agent (which answers `running: false`) all show nothing rather than a /// guess. diff --git a/macapp/WorkroomApp/Views/ChangesPanel.swift b/macapp/WorkroomApp/Views/ChangesPanel.swift index 7280178f6..09368172e 100644 --- a/macapp/WorkroomApp/Views/ChangesPanel.swift +++ b/macapp/WorkroomApp/Views/ChangesPanel.swift @@ -385,11 +385,11 @@ private struct ChangedFileCountBadge: View { } } -/// Whether this box is busy, as the agent's wakefulness service reports it (issue #208). +/// Whether a box is busy, as its agent's wakefulness service reports it (issues #208, #254). /// -/// In the Changes header rather than in the sidebar row, matching the PR badge it sits beside: the -/// verdict is per BOX, not per workroom, so every sidebar row would carry the same glyph — the -/// inspector shows it once, for whatever is selected. +/// This Mac's is in the Changes header rather than in the sidebar rows, matching the PR badge it +/// sits beside: the verdict is per BOX, not per workroom, so every local row would carry the same +/// glyph. A remote workroom is its own box, so its row carries its own (`ProjectSidebar`). /// /// It owns its polling through `.task`, which is what makes "no polling while hidden" true rather /// than merely intended: closing the inspector or the window unmounts this view and cancels the poll. @@ -397,6 +397,9 @@ private struct ChangedFileCountBadge: View { /// classifier does not run), so this is invisible on a local-only setup. struct WakefulnessBadge: View { @ObservedObject var model: WakefulnessModel = .shared + /// False for a remote host's badge, whose agent was not started with this Mac's settings. + var isLocal = true + var identifier = "changes.wakefulness" private let theme = ThemeService.shared var body: some View { @@ -425,9 +428,9 @@ struct WakefulnessBadge: View { image } } - .help(Self.help(for: status, settings: .current)) + .help(Self.help(for: status, settings: isLocal ? .current : nil)) .accessibilityLabel(glyph.label) - .accessibilityIdentifier("changes.wakefulness") + .accessibilityIdentifier(identifier) } } .task { await model.poll() } @@ -453,7 +456,9 @@ struct WakefulnessBadge: View { } } - static func help(for status: AgentWakefulness, settings: AgentWakefulnessSettings) -> String { + /// `settings` is this Mac's, to name a running agent's that differ; nil for a remote host's + /// agent, which this Mac's settings never started. + static func help(for status: AgentWakefulness, settings: AgentWakefulnessSettings?) -> String { let awake = wakefulnessDuration(status.awakeSeconds).formatted( .units(allowed: [.hours, .minutes], width: .narrow)) var text: String @@ -472,7 +477,9 @@ struct WakefulnessBadge: View { : "This machine is busy, but the agent could not write its verdict, so nothing is keeping " + "it awake. Click to keep it awake." } - if let mismatch = status.settingsMismatch(against: settings) { text += " " + mismatch } + if let settings, let mismatch = status.settingsMismatch(against: settings) { + text += " " + mismatch + } return text } } diff --git a/macapp/WorkroomApp/Views/ProjectSidebar.swift b/macapp/WorkroomApp/Views/ProjectSidebar.swift index 80994736c..1678dae52 100644 --- a/macapp/WorkroomApp/Views/ProjectSidebar.swift +++ b/macapp/WorkroomApp/Views/ProjectSidebar.swift @@ -431,6 +431,13 @@ struct ProjectSidebar: View { .foregroundStyle(.yellow) .help(warning.message) } + // A remote workroom is its own box, so its busy or idle verdict is its own (#254). Nothing + // shows while its host isn't connected: the poll never connects one. + if let host = workroom.reachableHost { + WakefulnessBadge( + model: .model(forHost: host), isLocal: false, + identifier: "sidebar.workroom.\(workroom.name).wakefulness") + } // Spinner/delete slot: a progress spinner while the workroom's setup runs (issue #116) or a // command runs (issue #28), swapped for the delete button on hover — so a workroom stays // deletable even mid-run. The delete button is always laid out (it reveals via opacity), so it diff --git a/macapp/WorkroomAppTests/AgentWakefulnessTests.swift b/macapp/WorkroomAppTests/AgentWakefulnessTests.swift index a4676748e..6f2b8e600 100644 --- a/macapp/WorkroomAppTests/AgentWakefulnessTests.swift +++ b/macapp/WorkroomAppTests/AgentWakefulnessTests.swift @@ -166,6 +166,43 @@ final class AgentWakefulnessTests: XCTestCase { XCTAssertNil(older.settingsMismatch(against: AgentWakefulnessSettings(ceiling: 1, ask: false))) } + /// A remote host's agent was never started with this Mac's settings, so its badge names no + /// mismatch with them (#254); this Mac's badge still does. + func testOnlyThisMacsBadgeComparesThisMacsSettings() throws { + let status = try status() + let settings = AgentWakefulnessSettings(ceiling: 7200, ask: false) + XCTAssertTrue( + WakefulnessBadge.help(for: status, settings: settings).contains("when it next starts")) + XCTAssertFalse( + WakefulnessBadge.help(for: status, settings: nil).contains("when it next starts")) + } + + /// One model per remote host, kept for the launch, and never this Mac's (#254). + @MainActor + func testEachRemoteHostHasItsOwnWakefulnessModel() { + let id = UUID() + XCTAssertTrue(WakefulnessModel.model(forHost: id) === WakefulnessModel.model(forHost: id)) + XCTAssertFalse(WakefulnessModel.model(forHost: id) === WakefulnessModel.model(forHost: UUID())) + XCTAssertFalse(WakefulnessModel.model(forHost: id) === WakefulnessModel.shared) + } + + /// A remote host's poll reads that host's connection and never connects one: with only this Mac + /// connected, it is refused as the host's, so the row shows nothing rather than this Mac's verdict. + @MainActor + func testARemoteHostsPollAsksForThatHostsConnection() async throws { + let manager = HostConnectionManager() + let host = HostID.remote(UUID()) + do { + _ = try await WakefulnessModel.Transport.on(host, manager: manager).status() + XCTFail("a remote host with no connection answered") + } catch RepositoryRoutingError.unavailable(let refused) { + XCTAssertEqual(refused, host) + } + let model = WakefulnessModel(transport: .on(host, manager: manager)) + await model.refresh() + XCTAssertNil(model.status) + } + func testAnErrorReplyIsAServiceFailure() { let json = #"{"version":1,"error":{"unsupported":"status requests are ..."}}"# XCTAssertThrowsError(try decode(json)) { error in diff --git a/macapp/WorkroomAppTests/RemoteHostIntegrationTests.swift b/macapp/WorkroomAppTests/RemoteHostIntegrationTests.swift index d69bd9833..1b4be8bcb 100644 --- a/macapp/WorkroomAppTests/RemoteHostIntegrationTests.swift +++ b/macapp/WorkroomAppTests/RemoteHostIntegrationTests.swift @@ -179,6 +179,21 @@ final class RemoteHostIntegrationTests: XCTestCase { XCTAssertEqual(banner, "SSH-2.0-") } + /// A remote workroom's sidebar badge (#254): the host's agent runs its classifier (Linux always + /// does), and the model reads its verdict through that host's connection. + @MainActor + func testTheWakefulnessModelReadsARemoteHostsVerdict() async throws { + let fixture = try 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 model = WakefulnessModel(transport: .on(host, manager: manager)) + await model.refresh() + let status = try XCTUnwrap(model.status, "no verdict from the host") + XCTAssertTrue(status.running, "the host's classifier is not running") + } + /// 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. From 4f9b4828504e7e1e76d16f2cc91c7db3fc19be77 Mon Sep 17 00:00:00 2001 From: Joel Moss Date: Sun, 4 Oct 2026 01:42:39 +0100 Subject: [PATCH 03/14] feat(macapp): resolve a remote pane's cmd-click on its host (#254, C7) A cmd-clicked path in a remote pane was resolved against the pane's directory on THIS Mac, so it opened nothing, or another file at the same path. It now resolves on the host: against the shell's directory there, as the host's agent reports it (`hostCwd`), and through the host's file service. The file opens in the in-app viewer, since no editor on this Mac can open a file on another machine. `TerminalLinkOpener.remoteCandidates` turns the link into workroom-relative candidates, keeping the local parsing (the `:line[:col]` and trailing-dot forms) and dropping what the host's file service can't read: paths outside the workroom, and `~`, which is this Mac's home. Each candidate is probed with a one-byte read, which the agent answers from the file's size without sending it. The cmd-hover cursor is answered from a cache that the first hover over a word fills. Web URLs still open in this Mac's browser. Co-Authored-By: Claude Opus 5.5 --- .../WorkroomApp/Core/TerminalLinkOpener.swift | 35 ++++++ .../WorkroomApp/Core/TerminalSessions.swift | 93 ++++++++++++++ .../RemoteHostIntegrationTests.swift | 20 +++ .../TerminalLinkOpenerTests.swift | 114 ++++++++++++++++++ 4 files changed, 262 insertions(+) diff --git a/macapp/WorkroomApp/Core/TerminalLinkOpener.swift b/macapp/WorkroomApp/Core/TerminalLinkOpener.swift index 02c441680..eed80677d 100644 --- a/macapp/WorkroomApp/Core/TerminalLinkOpener.swift +++ b/macapp/WorkroomApp/Core/TerminalLinkOpener.swift @@ -279,6 +279,41 @@ enum TerminalLinkOpener { return true } + // MARK: Remote panes (#254, C7) + + /// The workroom-relative files a remote pane's ⌘-clicked `word` could name, in the order to probe + /// them on the host. `cwd` and `root` are paths on the HOST: the pane's working directory as its + /// agent reports it, and the workroom's root. Nothing here touches this Mac's filesystem, which is + /// the point (C7): on this Mac the same path is another file, or none. + /// + /// A candidate outside `root` is dropped, because the host's file service reads only under the + /// workroom's root. So is a `~` path: `~` is this Mac's home, not the host's. A link that names a + /// URL rather than a path gives none, through the same `filePath(from:)` a local pane uses. + static func remoteCandidates(for word: String, cwd: String?, root: String) -> [PathCandidate] { + guard let path = filePath(from: word), root.hasPrefix("/") else { return [] } + let base = cwd.flatMap { $0.hasPrefix("/") ? $0 : nil } ?? root + let prefix = root.hasSuffix("/") ? root : root + "/" + var seen = Set() + return pathCandidates(from: path).compactMap { candidate in + guard !candidate.path.hasPrefix("~") else { return nil } + let joined = candidate.path.hasPrefix("/") ? candidate.path : base + "/" + candidate.path + // Lexical only: `isDirectory: false` keeps `URL` from probing this Mac's disk for the path. + let absolute = URL(fileURLWithPath: joined, isDirectory: false).standardized.path + guard absolute.hasPrefix(prefix) else { return nil } + let relative = String(absolute.dropFirst(prefix.count)) + guard !relative.isEmpty, seen.insert(relative).inserted else { return nil } + return PathCandidate(path: relative, line: candidate.line, column: candidate.column) + } + } + + /// The text of an open-URL link to resolve as a path in a remote pane: a `file:` URL's path, else + /// the link as written. Only after `isSystemHandledURL` has turned it down, as locally. + static func remoteLink(from url: URL) -> String { + if url.isFileURL { return url.path } + let link = url.absoluteString + return link.removingPercentEncoding ?? link + } + /// Does an installed app actually claim `url`'s scheme (`https:`, `mailto:`, `vscode:`, …)? /// /// Only then may it reach `NSWorkspace.open`; everything else is a silent no-op. libghostty's link diff --git a/macapp/WorkroomApp/Core/TerminalSessions.swift b/macapp/WorkroomApp/Core/TerminalSessions.swift index c83ad9a0c..98bc0e4c4 100644 --- a/macapp/WorkroomApp/Core/TerminalSessions.swift +++ b/macapp/WorkroomApp/Core/TerminalSessions.swift @@ -2162,6 +2162,10 @@ final class TerminalSessions: ObservableObject { self.onSurfaceFocused?(targetID) } + if target.remoteHost != nil { + wireRemoteLinks(view, tab: tabID, target: target) + return tab + } let projectPath = target.path view.onCmdClickFile = { [weak view] word in TerminalLinkOpener.handleCmdClickFile(word, cwd: view?.lastKnownCwd ?? projectPath) @@ -2175,6 +2179,95 @@ final class TerminalSessions: ObservableObject { return tab } + // MARK: Remote pane links (#254, C7) + + /// How a remote pane reaches its host's files. Swapped by tests. + var remoteFiles: (RepositoryLocation) async throws -> FileProviding = { + try await RepositoryRouter.shared.files(for: $0) + } + + /// What ⌘-hover found for a word in a remote pane, keyed by target, host cwd and word. The probe + /// is a round trip and the hover callback is synchronous, so the first hover over a word starts + /// one and the cursor follows on the next move. + // ponytail: cleared whole at 256 entries, and a file made after its word was hovered keeps the + // plain cursor until then; the click itself always asks again. + private var remoteHoverAnswers: [String: Bool] = [:] + + /// ⌘-click in a remote pane resolves on the HOST (C7): against the shell's directory there, as + /// its agent reports it (`hostCwd`), and through the host's file service. The file opens in the + /// in-app viewer, since no editor on this Mac can open a file on another machine. Web URLs still + /// go to this Mac's browser. + private func wireRemoteLinks( + _ view: GhosttySurfaceView, tab tabID: TerminalTab.ID, target: TerminalTarget + ) { + view.onCmdClickFile = { [weak self] word in + self?.openRemoteFile(word, tab: tabID, target: target) + } + view.resolveCmdHoverFile = { [weak self] word in + self?.remoteFileExists(word, tab: tabID, target: target) ?? false + } + view.onOpenURL = { [weak self] url in + if TerminalLinkOpener.isSystemHandledURL(url) { + NSWorkspace.shared.open(url) + } else { + self?.openRemoteFile(TerminalLinkOpener.remoteLink(from: url), tab: tabID, target: target) + } + return true + } + } + + private func hostCwd(of tabID: TerminalTab.ID, in target: TerminalTarget.ID) -> String? { + guard case .terminal(let state) = tabsByTarget[target]?[tabID]?.content else { return nil } + return state.hostCwd + } + + func openRemoteFile(_ word: String, tab tabID: TerminalTab.ID, target: TerminalTarget) { + let cwd = hostCwd(of: tabID, in: target.id) + Task { [weak self] in + guard let self, let file = await self.remoteFile(word, cwd: cwd, target: target) else { + return + } + self.openFilePreview(FileDescriptor(path: file.path, isPreview: true), for: target) + } + } + + private func remoteFileExists( + _ word: String, tab tabID: TerminalTab.ID, target: TerminalTarget + ) -> Bool { + let cwd = hostCwd(of: tabID, in: target.id) + let key = [target.id, cwd ?? "", word].joined(separator: "\u{0}") + if let known = remoteHoverAnswers[key] { return known } + if remoteHoverAnswers.count >= 256 { remoteHoverAnswers.removeAll() } + remoteHoverAnswers[key] = false + Task { [weak self] in + let found = await self?.remoteFile(word, cwd: cwd, target: target) != nil + self?.remoteHoverAnswers[key] = found + } + return false + } + + /// The first of `word`'s candidates that is a file on the host, or nil. A read capped at one byte + /// is the probe: it answers `.tooLarge` for any longer file without sending it, and fails for + /// anything missing, refused or outside the workroom. + func remoteFile(_ word: String, cwd: String?, target: TerminalTarget) async + -> TerminalLinkOpener.PathCandidate? + { + guard let location = target.remoteLocation else { return nil } + let candidates = TerminalLinkOpener.remoteCandidates(for: word, cwd: cwd, root: target.path) + guard !candidates.isEmpty, let files = try? await remoteFiles(location) else { return nil } + for candidate in candidates { + do { + _ = try await files.read(path: candidate.path, symlinks: .followWithinRoot, maxBytes: 1) + return candidate + } catch FileServiceError.tooLarge { + return candidate + } catch { + continue + } + } + return nil + } + /// Reconstruct a minimal `TerminalTarget` from its id for the `onFocused` callback (which only /// carries ids). `focus` keys off the id alone, so a minimal target is sufficient. private func target(forID id: TerminalTarget.ID) -> TerminalTarget? { diff --git a/macapp/WorkroomAppTests/RemoteHostIntegrationTests.swift b/macapp/WorkroomAppTests/RemoteHostIntegrationTests.swift index 1b4be8bcb..04ba09e65 100644 --- a/macapp/WorkroomAppTests/RemoteHostIntegrationTests.swift +++ b/macapp/WorkroomAppTests/RemoteHostIntegrationTests.swift @@ -194,6 +194,26 @@ final class RemoteHostIntegrationTests: XCTestCase { XCTAssertTrue(status.running, "the host's classifier is not running") } + /// ⌘-click in a remote pane (#254, C7) finds a file on the host through its file service, + /// relative to the shell's directory there, with its line. The path does not exist on this Mac. + @MainActor + func testACmdClickedPathResolvesOnTheHost() async throws { + let fixture = try fixture() + let path = try repository(fixture) + try onHost(fixture, "mkdir -p \(path)/lib && echo hi > \(path)/lib/only-here.rb") + let (connection, id) = try await connect(fixture.host) + let target = TerminalTarget( + id: "remote", title: "remote", path: path, unavailability: .remote, remoteHost: id) + let sessions = TerminalSessions() + sessions.remoteFiles = { location in + try connection.files(context: FileContext(location: location, sharedLocation: nil)) + } + let found = await sessions.remoteFile("only-here.rb:1", cwd: "\(path)/lib", target: target) + XCTAssertEqual(found, .init(path: "lib/only-here.rb", line: 1, column: nil)) + let missing = await sessions.remoteFile("nowhere.rb", cwd: "\(path)/lib", target: target) + XCTAssertNil(missing) + } + /// 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. diff --git a/macapp/WorkroomAppTests/TerminalLinkOpenerTests.swift b/macapp/WorkroomAppTests/TerminalLinkOpenerTests.swift index 68b7fc099..e818eea43 100644 --- a/macapp/WorkroomAppTests/TerminalLinkOpenerTests.swift +++ b/macapp/WorkroomAppTests/TerminalLinkOpenerTests.swift @@ -530,4 +530,118 @@ final class TerminalLinkOpenerTests: XCTestCase { XCTAssertEqual(resolved?.line, line, "\(link) should carry its line") } } + + // MARK: Remote panes (#254, C7) + + /// A remote pane's link resolves against the HOST's paths, to a workroom-relative file, in the + /// local order: the literal first, then the decoration-stripped forms carrying their line. + func testARemoteLinkResolvesAgainstTheHostsDirectory() { + let root = "/home/workroom/repo" + let candidates = TerminalLinkOpener.remoteCandidates( + for: "app/user.rb:5:2", cwd: "\(root)/lib", root: root) + XCTAssertEqual( + candidates, + [ + .init(path: "lib/app/user.rb:5:2", line: nil, column: nil), + .init(path: "lib/app/user.rb", line: 5, column: 2), + ]) + // No cwd yet: the workroom's root. + XCTAssertEqual( + TerminalLinkOpener.remoteCandidates(for: "README.md", cwd: nil, root: root).map(\.path), + ["README.md"]) + // An absolute path inside the workroom, and `..` back into it. + XCTAssertEqual( + TerminalLinkOpener.remoteCandidates(for: "\(root)/a/b.txt", cwd: "/", root: root).map( + \.path), ["a/b.txt"]) + XCTAssertEqual( + TerminalLinkOpener.remoteCandidates(for: "../x.txt", cwd: "\(root)/lib", root: root).map( + \.path), ["x.txt"]) + } + + /// Only what the host's file service can read: nothing outside the workroom's root, no `~` (this + /// Mac's home), and no URL. + func testARemoteLinkOutsideTheWorkroomOrNotAPathGivesNothing() { + let root = "/home/workroom/repo" + for link in [ + "/etc/passwd", "../../other/file.txt", "~/notes.txt", "https://example.com/a.rb", + "javascript:payload.command", "/home/workroom/repository/file", + ] { + XCTAssertEqual( + TerminalLinkOpener.remoteCandidates(for: link, cwd: root, root: root), [], link) + } + } + + private struct HostFiles: FileProviding { + let context: FileContext + let files: [String: Data] + + func list() async throws -> CommandResult { + CommandResult(stdout: "", stderr: "", exitCode: 0, timedOut: false) + } + func read(path: String, symlinks: FileSymlinkPolicy, maxBytes: Int) async throws -> Data { + guard let data = files[path] else { throw FileServiceError.notFound(path) } + guard data.count <= maxBytes else { throw FileServiceError.tooLarge } + return data + } + func watch(root: String, onEvent: @escaping @Sendable (FileWatchEvent) -> Void) async throws + -> FileWatchHandle? + { nil } + } + + /// The acceptance test for C7: the workroom's path exists on THIS Mac too, holding a different + /// file. ⌘-click resolves only what the host has, and a file only this Mac has resolves to + /// nothing. + @MainActor + func testARemotePaneOpensTheHostsFileNotTheMacs() async throws { + let fm = FileManager.default + let root = fm.temporaryDirectory.appendingPathComponent(UUID().uuidString) + try fm.createDirectory(at: root, withIntermediateDirectories: true) + defer { try? fm.removeItem(at: root) } + fm.createFile(atPath: root.appendingPathComponent("only-on-mac.txt").path, contents: Data()) + + let host = UUID() + let target = TerminalTarget( + id: "remote", title: "remote", path: root.path, unavailability: .remote, remoteHost: host) + let location = try XCTUnwrap(target.remoteLocation) + let sessions = TerminalSessions() + sessions.remoteFiles = { location in + HostFiles( + context: FileContext(location: location, sharedLocation: nil), + files: ["only-on-host.rb": Data("puts 1\n".utf8)]) + } + + let found = await sessions.remoteFile("only-on-host.rb:3", cwd: nil, target: target) + XCTAssertEqual(found, .init(path: "only-on-host.rb", line: 3, column: nil)) + let onMac = await sessions.remoteFile("only-on-mac.txt", cwd: nil, target: target) + XCTAssertNil(onMac, "a file that is only on this Mac was taken for the host's") + XCTAssertEqual(location.host, .remote(host)) + } + + /// The wiring: a remote target's pane routes ⌘-click to the host and opens the file it finds as + /// the workroom's preview tab, in the in-app viewer. + @MainActor + func testCmdClickInARemotePaneOpensTheHostsFileInTheViewer() async throws { + let target = TerminalTarget( + id: "remote", title: "remote", path: "/home/workroom/repo", unavailability: .remote, + remoteHost: UUID()) + let sessions = TerminalSessions() + sessions.makeView = { _, cwd, _ in GhosttySurfaceView(workingDirectory: cwd) } + sessions.recordUnrecognizedTool = { _ in } + sessions.remoteFiles = { location in + HostFiles( + context: FileContext(location: location, sharedLocation: nil), + files: ["lib/user.rb": Data("class User; end\n".utf8)]) + } + let tab = sessions.addTab(for: target) + let view = try XCTUnwrap(sessions.view(forTab: tab.id, inTarget: target.id)) + view.onCmdClickFile?("lib/user.rb:1") + func opened() -> FileDescriptor? { + for tab in sessions.tabs(for: target) { + if case .file(let file) = tab.content { return file } + } + return nil + } + for _ in 0..<200 where opened() == nil { try await Task.sleep(for: .milliseconds(10)) } + XCTAssertEqual(opened()?.path, "lib/user.rb") + } } From b59342da130de591625775b08d31aaf9b084eb40 Mon Sep 17 00:00:00 2001 From: Joel Moss Date: Sun, 4 Oct 2026 01:45:09 +0100 Subject: [PATCH 04/14] docs(remote-workrooms): record #254 as built, with its limits Co-Authored-By: Claude Opus 5.5 --- docs/designs/remote-workrooms.md | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/docs/designs/remote-workrooms.md b/docs/designs/remote-workrooms.md index 1641cb10f..98f725f52 100644 --- a/docs/designs/remote-workrooms.md +++ b/docs/designs/remote-workrooms.md @@ -1712,6 +1712,32 @@ these are the subsystems that actually gate "a remote workroom is a real workroo It is a broker grant, not a deploy key (OQ20). - Idle state per remote workroom in the sidebar. **Cost display is deliberately deferred** — it needs per-provider billing APIs and there is no spending policy yet (open question 7). +- **As built (#254, remote pane parity).** Pane close was done first, in #306 (#283). The rest: + - **Cmd-click (C7).** A remote pane's links never reach `TerminalLinkOpener`'s local resolver. + `TerminalSessions.wireRemoteLinks` resolves a clicked path against the shell's directory on the + host, as its agent reports it (`hostCwd`, #245), or the workroom root before the first report. + `TerminalLinkOpener.remoteCandidates` keeps the local parsing (`:line[:col]`, trailing dots) + and drops what the host's file service can't read: anything outside the workroom root, and + `~`, which is this Mac's home. Each candidate is probed with a one-byte read, which the agent + answers from the file's size without sending it (`file.rs`, `read_file`). The first that exists + opens in the in-app viewer as the workroom's preview tab, since no editor on the Mac can open + a file on another machine. The hand cursor on cmd-hover comes from a cache that the first hover + over a word fills, so it shows from the second move on. Web URLs still go to the Mac's browser. + - **Ports.** `PortForwardingModel` is one model per host (`model(for:)`), kept for the launch, + and the inspector's Ports section shows the selected workroom's host. A remote workroom the + app can't reach shows no Ports section, rather than the Mac's list. A forward never connects a + host; selecting the workroom does. This closed #208's last criterion. + - **Idle state.** A remote workroom's sidebar row carries the wakefulness badge, backed by one + `WakefulnessModel` per remote host (`model(forHost:)`). It polls only while the row is shown + and never connects the host. The Linux agent runs the classifier unconditionally (`serve.rs`), + so neither the agent nor host setup changed. A remote badge's tooltip leaves out the + settings-mismatch sentence: that agent was never started with the Mac's settings. + - **Known limits.** The viewer opens a cmd-clicked file at the top: `FileDescriptor` has no line. + A path outside the workroom (`/etc/hosts`, a sibling checkout) opens nothing. The ceiling prompt + is still raised only for the Mac's agent: a remote host's "Keep awake" works from its badge, but + no prompt card is shown for it. `hostCwd` is asked for again each time a command finishes, one + round trip later, so a click inside that round trip after a `cd` resolves against the previous + directory. ## Phase 0 Results From 9309188aaafb3dfb01b485635243af1a15e9a908 Mon Sep 17 00:00:00 2001 From: Joel Moss Date: Sun, 4 Oct 2026 01:58:47 +0100 Subject: [PATCH 05/14] fix(macapp): let a remote pane's cmd-click through while the host is asked (#254) `GhosttySurfaceView.mouseDown` passes a cmd-click on only when the synchronous `resolveCmdHoverFile` says the word is a file. In a remote pane that answer is a round trip to the host, and the hover cache answered "no" until it came back. Every click made straight after pressing cmd was lost, and a "no" stood until 256 other words had been hovered. A word that could name a file in the workroom now passes while its probe is out, and the click asks the host itself. An answer stands for 3 s. URLs and paths outside the workroom are never candidates, so they still reach libghostty. Also from review: - Slashes are collapsed before the containment check. A root with a trailing slash, or a link with `//`, resolved to a path the host reads as absolute, so it opened nothing. - A remote link that opens nothing is logged, as a local one is. - A transport failure stops the probe instead of trying the next candidate. Co-Authored-By: Claude Opus 5.5 --- docs/designs/remote-workrooms.md | 11 +++- .../WorkroomApp/Core/TerminalLinkOpener.swift | 11 +++- .../WorkroomApp/Core/TerminalSessions.swift | 49 ++++++++++----- .../TerminalLinkOpenerTests.swift | 60 +++++++++++++++++++ 4 files changed, 112 insertions(+), 19 deletions(-) diff --git a/docs/designs/remote-workrooms.md b/docs/designs/remote-workrooms.md index 98f725f52..c307269cf 100644 --- a/docs/designs/remote-workrooms.md +++ b/docs/designs/remote-workrooms.md @@ -1721,8 +1721,15 @@ these are the subsystems that actually gate "a remote workroom is a real workroo `~`, which is this Mac's home. Each candidate is probed with a one-byte read, which the agent answers from the file's size without sending it (`file.rs`, `read_file`). The first that exists opens in the in-app viewer as the workroom's preview tab, since no editor on the Mac can open - a file on another machine. The hand cursor on cmd-hover comes from a cache that the first hover - over a word fills, so it shows from the second move on. Web URLs still go to the Mac's browser. + a file on another machine. Web URLs still go to the Mac's browser. + - **The ⌘-click gate.** `GhosttySurfaceView.mouseDown` passes a ⌘-click on only when the + synchronous `resolveCmdHoverFile` says the word is a file, and the host's answer is a round + trip. So a word that could name a file in the workroom passes while its probe is out, and the + click then asks the host itself. A first version answered "no" until the probe came back, + which lost every click made straight after pressing ⌘ (found in review). An answer stands for + 3 s (`remoteHoverTTL`), so a file made after its word was hovered becomes clickable. The cost: + a ⌘-click on a plain word before its answer arrives is consumed and opens nothing. A URL or a + path outside the workroom is never a candidate, so it still reaches libghostty. - **Ports.** `PortForwardingModel` is one model per host (`model(for:)`), kept for the launch, and the inspector's Ports section shows the selected workroom's host. A remote workroom the app can't reach shows no Ports section, rather than the Mac's list. A forward never connects a diff --git a/macapp/WorkroomApp/Core/TerminalLinkOpener.swift b/macapp/WorkroomApp/Core/TerminalLinkOpener.swift index eed80677d..ca19f5517 100644 --- a/macapp/WorkroomApp/Core/TerminalLinkOpener.swift +++ b/macapp/WorkroomApp/Core/TerminalLinkOpener.swift @@ -291,12 +291,19 @@ enum TerminalLinkOpener { /// URL rather than a path gives none, through the same `filePath(from:)` a local pane uses. static func remoteCandidates(for word: String, cwd: String?, root: String) -> [PathCandidate] { guard let path = filePath(from: word), root.hasPrefix("/") else { return [] } + // `standardized` keeps `//`, and the host joins a relative path whose first character is `/` + // as an absolute one, so slashes are collapsed first, the root's trailing one with them. + func collapsed(_ path: String) -> String { + path.replacingOccurrences(of: "/+", with: "/", options: .regularExpression) + } + let root = collapsed(root + "/") let base = cwd.flatMap { $0.hasPrefix("/") ? $0 : nil } ?? root - let prefix = root.hasSuffix("/") ? root : root + "/" + let prefix = root var seen = Set() return pathCandidates(from: path).compactMap { candidate in guard !candidate.path.hasPrefix("~") else { return nil } - let joined = candidate.path.hasPrefix("/") ? candidate.path : base + "/" + candidate.path + let joined = collapsed( + candidate.path.hasPrefix("/") ? candidate.path : base + "/" + candidate.path) // Lexical only: `isDirectory: false` keeps `URL` from probing this Mac's disk for the path. let absolute = URL(fileURLWithPath: joined, isDirectory: false).standardized.path guard absolute.hasPrefix(prefix) else { return nil } diff --git a/macapp/WorkroomApp/Core/TerminalSessions.swift b/macapp/WorkroomApp/Core/TerminalSessions.swift index 98bc0e4c4..9db5cb0ca 100644 --- a/macapp/WorkroomApp/Core/TerminalSessions.swift +++ b/macapp/WorkroomApp/Core/TerminalSessions.swift @@ -2186,12 +2186,14 @@ final class TerminalSessions: ObservableObject { try await RepositoryRouter.shared.files(for: $0) } - /// What ⌘-hover found for a word in a remote pane, keyed by target, host cwd and word. The probe - /// is a round trip and the hover callback is synchronous, so the first hover over a word starts - /// one and the cursor follows on the next move. - // ponytail: cleared whole at 256 entries, and a file made after its word was hovered keeps the - // plain cursor until then; the click itself always asks again. - private var remoteHoverAnswers: [String: Bool] = [:] + /// What the host said about a word in a remote pane, keyed by target, host cwd and word: nil + /// while its probe is out. The hover callback is synchronous and is also the ⌘-click's gate + /// (`GhosttySurfaceView.mouseDown`), and the probe is a round trip, so an answer can't wait. + // ponytail: cleared whole at 256 entries; an LRU if hovering ever thrashes it. + private var remoteHoverAnswers: [String: (found: Bool?, at: ContinuousClock.Instant)] = [:] + /// How long a host's answer stands before a hover asks again: a file can appear, and a probe can + /// fail on a passing disconnect. Swapped by tests. + var remoteHoverTTL: Duration = .seconds(3) /// ⌘-click in a remote pane resolves on the HOST (C7): against the shell's directory there, as /// its agent reports it (`hostCwd`), and through the host's file service. The file opens in the @@ -2224,26 +2226,39 @@ final class TerminalSessions: ObservableObject { func openRemoteFile(_ word: String, tab tabID: TerminalTab.ID, target: TerminalTarget) { let cwd = hostCwd(of: tabID, in: target.id) Task { [weak self] in - guard let self, let file = await self.remoteFile(word, cwd: cwd, target: target) else { + guard let self else { return } + guard let file = await self.remoteFile(word, cwd: cwd, target: target) else { + // As `TerminalLinkOpener.handleOpenURL` does locally: a dropped click is otherwise + // indistinguishable from a broken app. + NSLog("Workroom: no file on the host for terminal link %@", word) return } self.openFilePreview(FileDescriptor(path: file.path, isPreview: true), for: target) } } - private func remoteFileExists( - _ word: String, tab tabID: TerminalTab.ID, target: TerminalTarget - ) -> Bool { + /// The ⌘-hover cursor and the ⌘-click gate. A word the host has answered for gets that answer + /// while it is fresh. Any other word that could name a file in the workroom counts as one while + /// its probe is out, so a ⌘-click made before the answer reaches `openRemoteFile`, which asks the + /// host again; answering "no" until then lost every click made straight after pressing ⌘. The + /// cost: such a click on a word that turns out to be plain text is consumed and opens nothing. + /// A URL or a path outside the workroom is never a candidate, so it still reaches libghostty. + func remoteFileExists(_ word: String, tab tabID: TerminalTab.ID, target: TerminalTarget) -> Bool { let cwd = hostCwd(of: tabID, in: target.id) + guard !TerminalLinkOpener.remoteCandidates(for: word, cwd: cwd, root: target.path).isEmpty + else { return false } let key = [target.id, cwd ?? "", word].joined(separator: "\u{0}") - if let known = remoteHoverAnswers[key] { return known } + let now = ContinuousClock.now + if let answer = remoteHoverAnswers[key], now - answer.at < remoteHoverTTL { + return answer.found ?? true + } if remoteHoverAnswers.count >= 256 { remoteHoverAnswers.removeAll() } - remoteHoverAnswers[key] = false + remoteHoverAnswers[key] = (nil, now) Task { [weak self] in let found = await self?.remoteFile(word, cwd: cwd, target: target) != nil - self?.remoteHoverAnswers[key] = found + self?.remoteHoverAnswers[key] = (found, .now) } - return false + return true } /// The first of `word`'s candidates that is a file on the host, or nil. A read capped at one byte @@ -2261,8 +2276,12 @@ final class TerminalSessions: ObservableObject { return candidate } catch FileServiceError.tooLarge { return candidate - } catch { + } catch FileServiceError.notFound, FileServiceError.refused { continue + } catch { + // The host, not the candidate: the next one would fail the same way. + NSLog("Workroom: probing the host for %@: %@", candidate.path, "\(error)") + return nil } } return nil diff --git a/macapp/WorkroomAppTests/TerminalLinkOpenerTests.swift b/macapp/WorkroomAppTests/TerminalLinkOpenerTests.swift index e818eea43..4587d2bb8 100644 --- a/macapp/WorkroomAppTests/TerminalLinkOpenerTests.swift +++ b/macapp/WorkroomAppTests/TerminalLinkOpenerTests.swift @@ -644,4 +644,64 @@ final class TerminalLinkOpenerTests: XCTestCase { for _ in 0..<200 where opened() == nil { try await Task.sleep(for: .milliseconds(10)) } XCTAssertEqual(opened()?.path, "lib/user.rb") } + + /// A root recorded with a trailing slash, or a link with a doubled one, still resolves inside + /// the workroom: `standardized` keeps `//`, and the host reads `/src/a.rb` as an absolute path. + func testARemoteLinkSurvivesDoubledAndTrailingSlashes() { + XCTAssertEqual( + TerminalLinkOpener.remoteCandidates(for: "src/a.rb", cwd: nil, root: "/repo/").map(\.path), + ["src/a.rb"]) + XCTAssertEqual( + TerminalLinkOpener.remoteCandidates(for: "/repo//src/a.rb", cwd: nil, root: "/repo").map( + \.path), ["src/a.rb"]) + XCTAssertEqual( + TerminalLinkOpener.remoteCandidates(for: "a.rb", cwd: "/repo//lib/", root: "/repo").map( + \.path), ["lib/a.rb"]) + } + + /// What the host holds, changeable mid-test. + private final class HostDisk: @unchecked Sendable { + private let lock = NSLock() + private var files: [String: Data] = [:] + func put(_ path: String) { lock.withLock { files[path] = Data("x".utf8) } } + var snapshot: [String: Data] { lock.withLock { files } } + } + + /// The ⌘-click gate (`GhosttySurfaceView.mouseDown` asks `resolveCmdHoverFile` synchronously): + /// a word that could be a file in the workroom passes while its probe is out, so a click made + /// straight after pressing ⌘ is not lost. A "no" stands only while it is fresh, so a file made + /// after its word was hovered is found. URLs and paths outside the workroom never pass. + @MainActor + func testTheRemoteClickGatePassesAWordWhileTheHostIsAskedAndForgetsAStaleNo() async throws { + let target = TerminalTarget( + id: "remote", title: "remote", path: "/home/workroom/repo", unavailability: .remote, + remoteHost: UUID()) + let disk = HostDisk() + let sessions = TerminalSessions() + sessions.makeView = { _, cwd, _ in GhosttySurfaceView(workingDirectory: cwd) } + sessions.recordUnrecognizedTool = { _ in } + sessions.remoteHoverTTL = .milliseconds(200) + sessions.remoteFiles = { location in + HostFiles( + context: FileContext(location: location, sharedLocation: nil), files: disk.snapshot) + } + let tab = sessions.addTab(for: target) + let gate = { (word: String) in sessions.remoteFileExists(word, tab: tab.id, target: target) } + + XCTAssertFalse(gate("https://example.com/a.rb")) + XCTAssertFalse(gate("/etc/passwd")) + XCTAssertTrue(gate("later.rb"), "a click before the host answered would be lost") + // The host says no: the gate follows once the answer is in. + for _ in 0..<200 where gate("later.rb") { try await Task.sleep(for: .milliseconds(5)) } + XCTAssertFalse(gate("later.rb")) + // The file appears; once the "no" is stale, the host is asked again and says yes. + disk.put("later.rb") + try await Task.sleep(for: .milliseconds(250)) + XCTAssertTrue(gate("later.rb")) + for _ in 0..<100 { + _ = gate("later.rb") + try await Task.sleep(for: .milliseconds(5)) + } + XCTAssertTrue(gate("later.rb"), "a file made after its word was hovered stayed unclickable") + } } From 6ed9a710626f2c8d671f43e1b0550880324e10fb Mon Sep 17 00:00:00 2001 From: Joel Moss Date: Sun, 4 Oct 2026 02:03:31 +0100 Subject: [PATCH 06/14] test(macapp): drive the remote click gate through the view's own closure (#254) Co-Authored-By: Claude Opus 5.5 --- docs/designs/remote-workrooms.md | 2 +- macapp/WorkroomAppTests/TerminalLinkOpenerTests.swift | 4 +++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/docs/designs/remote-workrooms.md b/docs/designs/remote-workrooms.md index c307269cf..8b104683e 100644 --- a/docs/designs/remote-workrooms.md +++ b/docs/designs/remote-workrooms.md @@ -1707,7 +1707,7 @@ these are the subsystems that actually gate "a remote workroom is a real workroo hostname. From a remote agent that is a *remote* path labelled local, and `TerminalLinkOpener.absolutePath(for:cwd:)` (`TerminalLinkOpener.swift:27-31,202`) will then resolve a cmd-clicked file against it **on the Mac**. Week-one bug; needs a host-tagged cwd and a - link opener that routes through the file service. + link opener that routes through the file service. Done: see "As built (#254)" below. - Per-workroom credential: enrol on create, revoke on destroy, and revoke on the failure paths too. It is a broker grant, not a deploy key (OQ20). - Idle state per remote workroom in the sidebar. **Cost display is deliberately deferred** — diff --git a/macapp/WorkroomAppTests/TerminalLinkOpenerTests.swift b/macapp/WorkroomAppTests/TerminalLinkOpenerTests.swift index 4587d2bb8..becaab3b0 100644 --- a/macapp/WorkroomAppTests/TerminalLinkOpenerTests.swift +++ b/macapp/WorkroomAppTests/TerminalLinkOpenerTests.swift @@ -686,7 +686,9 @@ final class TerminalLinkOpenerTests: XCTestCase { context: FileContext(location: location, sharedLocation: nil), files: disk.snapshot) } let tab = sessions.addTab(for: target) - let gate = { (word: String) in sessions.remoteFileExists(word, tab: tab.id, target: target) } + // Through the view's own closure, which is what the real gate reads. + let view = try XCTUnwrap(sessions.view(forTab: tab.id, inTarget: target.id)) + let gate = { (word: String) in view.resolveCmdHoverFile?(word) == true } XCTAssertFalse(gate("https://example.com/a.rb")) XCTAssertFalse(gate("/etc/passwd")) From 4f6ae46666fa94d75b6f37441f8a76abfa1c3d17 Mon Sep 17 00:00:00 2001 From: Joel Moss Date: Sun, 4 Oct 2026 09:25:53 +0100 Subject: [PATCH 07/14] fix(macapp): pre-landing review fixes for remote pane parity (#254) From the ship review (specialists and red team): - A remote host's "Keep awake" connects the host first, as this Mac's spawns an agent. Before, a click while the connection was down failed silently and the box could sleep under a running job. - Quitting drains every host's in-flight keep, not only this Mac's. - A cmd-hover probe never connects the host; only the click's does. - A probe that gets no answer (no connection, or the agent busy past its concurrent-read cap) is no longer cached as "no file". - WakefulnessModel's doc comment no longer says there is one instance. - The hover cache limit is a named constant; a redundant alias is gone. Tests: the click-gate test no longer races a 200 ms TTL, and it now fails if the host's "yes" never arrives. New tests cover the hover/click connect split, the remote keep connecting first, the probe's no-answer case, a URL link and the pane's host cwd reaching a click. The two host-file fakes are now one. Co-Authored-By: Claude Opus 5.5 --- docs/designs/remote-workrooms.md | 10 +- .../WorkroomApp/Core/AgentWakefulness.swift | 37 +++- .../WorkroomApp/Core/TerminalLinkOpener.swift | 5 +- .../WorkroomApp/Core/TerminalSessions.swift | 67 ++++-- macapp/WorkroomApp/WorkroomApp.swift | 2 +- .../AgentWakefulnessTests.swift | 32 +++ .../RemoteHostIntegrationTests.swift | 2 +- .../TerminalLinkOpenerTests.swift | 206 +++++++++++++++--- .../TerminalSessionsTests.swift | 41 ++++ 9 files changed, 341 insertions(+), 61 deletions(-) diff --git a/docs/designs/remote-workrooms.md b/docs/designs/remote-workrooms.md index 8b104683e..41f90edec 100644 --- a/docs/designs/remote-workrooms.md +++ b/docs/designs/remote-workrooms.md @@ -1729,14 +1729,20 @@ these are the subsystems that actually gate "a remote workroom is a real workroo which lost every click made straight after pressing ⌘ (found in review). An answer stands for 3 s (`remoteHoverTTL`), so a file made after its word was hovered becomes clickable. The cost: a ⌘-click on a plain word before its answer arrives is consumed and opens nothing. A URL or a - path outside the workroom is never a candidate, so it still reaches libghostty. + path outside the workroom is never a candidate, so it still reaches libghostty. A hover's probe + never connects the host, as no passive read of a host does; only the click's probe goes + through `RepositoryRouter`, which connects it. A probe that gets no answer (no connection, or + the agent turning the read away as busy past `MAX_CONCURRENT_READS`) is not cached as "no + file": the next hover asks again. - **Ports.** `PortForwardingModel` is one model per host (`model(for:)`), kept for the launch, and the inspector's Ports section shows the selected workroom's host. A remote workroom the app can't reach shows no Ports section, rather than the Mac's list. A forward never connects a host; selecting the workroom does. This closed #208's last criterion. - **Idle state.** A remote workroom's sidebar row carries the wakefulness badge, backed by one `WakefulnessModel` per remote host (`model(forHost:)`). It polls only while the row is shown - and never connects the host. The Linux agent runs the classifier unconditionally (`serve.rs`), + and never connects the host. Its "Keep awake" click does connect it first + (`RemoteHosts.ensureConnected`), as the Mac's own spawns an agent, and quitting drains every + host's in-flight keep, not only the Mac's (`WakefulnessModel.drainAllKeeps`). The Linux agent runs the classifier unconditionally (`serve.rs`), so neither the agent nor host setup changed. A remote badge's tooltip leaves out the settings-mismatch sentence: that agent was never started with the Mac's settings. - **Known limits.** The viewer opens a cmd-clicked file at the top: `FileDescriptor` has no line. diff --git a/macapp/WorkroomApp/Core/AgentWakefulness.swift b/macapp/WorkroomApp/Core/AgentWakefulness.swift index d0b2efb6f..defecaf21 100644 --- a/macapp/WorkroomApp/Core/AgentWakefulness.swift +++ b/macapp/WorkroomApp/Core/AgentWakefulness.swift @@ -397,8 +397,9 @@ struct AwakeCeilingPromptState: Equatable { } } -/// What the UI reads: the local box's wakefulness, polled while it is on screen, plus the ceiling -/// prompt. One instance, because the verdict is per BOX — every workroom on this machine shares it. +/// What the UI reads: a box's wakefulness, polled while it is on screen, plus the ceiling prompt. +/// One instance per BOX, because the verdict is per box: `shared` is this Mac's, which every local +/// workroom shares, and each remote host has its own (`model(forHost:)`, #254). @MainActor final class WakefulnessModel: ObservableObject { static let shared = WakefulnessModel() @@ -419,13 +420,20 @@ final class WakefulnessModel: ObservableObject { keep: { try await LocalAgentVCS.shared.wakefulness(connecting: .spawn).keep() }, prompts: { try await LocalAgentVCS.shared.wakefulness(connecting: .reconnect).prompts }) - /// A remote host's service in `manager` (#254). None of the three connects the host: a poll - /// never does, and the host's connection is made by selecting its workroom or by the status - /// sweep, either of which a remote "keep" or watch can wait for. - static func on(_ host: HostID, manager: HostConnectionManager) -> Transport { + /// A remote host's service in `manager` (#254). A poll and the prompt watch never connect + /// the host: a badge is not a reason to reach it. A "keep" runs `connect` first, as the local + /// transport spawns, because a click is the one caller for which a dropped connection is not an + /// answer (`keep()`). + static func on( + _ host: HostID, manager: HostConnectionManager, + connect: @escaping @Sendable (HostID) async throws -> Void = { _ in } + ) -> Transport { Transport( status: { try await manager.wakefulness(host: host).status() }, - keep: { try await manager.wakefulness(host: host).keep() }, + keep: { + try await connect(host) + try await manager.wakefulness(host: host).keep() + }, prompts: { try await manager.wakefulness(host: host).prompts }) } } @@ -442,11 +450,24 @@ final class WakefulnessModel: ObservableObject { static func model(forHost id: UUID) -> WakefulnessModel { if let model = hosts[id] { return model } - let model = WakefulnessModel(transport: .on(.remote(id), manager: .shared)) + let model = WakefulnessModel( + transport: .on(.remote(id), manager: .shared) { + try await RemoteHosts.shared.ensureConnected($0) + }) hosts[id] = model return model } + /// Every model's in-flight "Keep awake", this Mac's and each remote host's, for quit. At once, so + /// the quit waits for the slowest, not their sum. A remote host's badge is its only keep control. + static func drainAllKeeps() async { + await withTaskGroup(of: Void.self) { group in + for model in [shared] + Array(hosts.values) { + group.addTask { await model.drainKeep() } + } + } + } + /// Nil until a poll succeeds, and again once one fails: no agent, an agent that predates the /// service, or a macOS agent (which answers `running: false`) all show nothing rather than a /// guess. diff --git a/macapp/WorkroomApp/Core/TerminalLinkOpener.swift b/macapp/WorkroomApp/Core/TerminalLinkOpener.swift index ca19f5517..b73c2ee65 100644 --- a/macapp/WorkroomApp/Core/TerminalLinkOpener.swift +++ b/macapp/WorkroomApp/Core/TerminalLinkOpener.swift @@ -296,9 +296,8 @@ enum TerminalLinkOpener { func collapsed(_ path: String) -> String { path.replacingOccurrences(of: "/+", with: "/", options: .regularExpression) } - let root = collapsed(root + "/") - let base = cwd.flatMap { $0.hasPrefix("/") ? $0 : nil } ?? root - let prefix = root + let prefix = collapsed(root + "/") + let base = cwd.flatMap { $0.hasPrefix("/") ? $0 : nil } ?? prefix var seen = Set() return pathCandidates(from: path).compactMap { candidate in guard !candidate.path.hasPrefix("~") else { return nil } diff --git a/macapp/WorkroomApp/Core/TerminalSessions.swift b/macapp/WorkroomApp/Core/TerminalSessions.swift index 9db5cb0ca..8b1354d92 100644 --- a/macapp/WorkroomApp/Core/TerminalSessions.swift +++ b/macapp/WorkroomApp/Core/TerminalSessions.swift @@ -2181,15 +2181,23 @@ final class TerminalSessions: ObservableObject { // MARK: Remote pane links (#254, C7) - /// How a remote pane reaches its host's files. Swapped by tests. - var remoteFiles: (RepositoryLocation) async throws -> FileProviding = { - try await RepositoryRouter.shared.files(for: $0) + /// How a remote pane reaches its host's files, connecting the host when `connect` says so: a + /// click does, as a deliberate act; a hover never does, as no passive read of a host does + /// (`HostConnectionManager.workingDirectory`). Swapped by tests. + var remoteFiles: (RepositoryLocation, _ connect: Bool) async throws -> FileProviding = { + location, connect in + guard connect else { + return try await HostConnectionManager.shared.files( + context: FileContext(location: location, sharedLocation: nil)) + } + return try await RepositoryRouter.shared.files(for: location) } /// What the host said about a word in a remote pane, keyed by target, host cwd and word: nil /// while its probe is out. The hover callback is synchronous and is also the ⌘-click's gate /// (`GhosttySurfaceView.mouseDown`), and the probe is a round trip, so an answer can't wait. - // ponytail: cleared whole at 256 entries; an LRU if hovering ever thrashes it. + // ponytail: cleared whole at `remoteHoverLimit` entries; an LRU if hovering ever thrashes it. + private static let remoteHoverLimit = 256 private var remoteHoverAnswers: [String: (found: Bool?, at: ContinuousClock.Instant)] = [:] /// How long a host's answer stands before a hover asks again: a file can appear, and a probe can /// fail on a passing disconnect. Swapped by tests. @@ -2252,39 +2260,64 @@ final class TerminalSessions: ObservableObject { if let answer = remoteHoverAnswers[key], now - answer.at < remoteHoverTTL { return answer.found ?? true } - if remoteHoverAnswers.count >= 256 { remoteHoverAnswers.removeAll() } + if remoteHoverAnswers.count >= Self.remoteHoverLimit { remoteHoverAnswers.removeAll() } remoteHoverAnswers[key] = (nil, now) Task { [weak self] in - let found = await self?.remoteFile(word, cwd: cwd, target: target) != nil - self?.remoteHoverAnswers[key] = (found, .now) + guard let probe = await self?.probeRemote(word, cwd: cwd, target: target, connect: false) + else { return } + switch probe { + case .found: self?.remoteHoverAnswers[key] = (true, .now) + case .absent: self?.remoteHoverAnswers[key] = (false, .now) + // No answer (no connection, an agent busy with other reads) is not "no file": the next + // hover asks again rather than holding a wrong "no" for `remoteHoverTTL`. + case .unknown: self?.remoteHoverAnswers[key] = nil + } } return true } - /// The first of `word`'s candidates that is a file on the host, or nil. A read capped at one byte - /// is the probe: it answers `.tooLarge` for any longer file without sending it, and fails for - /// anything missing, refused or outside the workroom. + /// What a probe of the host found: a file, no file among the candidates, or no answer. + enum RemoteProbe: Equatable { + case found(TerminalLinkOpener.PathCandidate) + case absent + case unknown + } + + /// The first of `word`'s candidates that is a file on the host, or nil, connecting the host if it + /// must: the click's question. func remoteFile(_ word: String, cwd: String?, target: TerminalTarget) async -> TerminalLinkOpener.PathCandidate? { - guard let location = target.remoteLocation else { return nil } + guard case .found(let file) = await probeRemote(word, cwd: cwd, target: target, connect: true) + else { return nil } + return file + } + + /// A read capped at one byte is the probe: it answers `.tooLarge` for any longer file without + /// sending it, and fails for anything missing, refused or outside the workroom. + func probeRemote(_ word: String, cwd: String?, target: TerminalTarget, connect: Bool) async + -> RemoteProbe + { + guard let location = target.remoteLocation else { return .absent } let candidates = TerminalLinkOpener.remoteCandidates(for: word, cwd: cwd, root: target.path) - guard !candidates.isEmpty, let files = try? await remoteFiles(location) else { return nil } + guard !candidates.isEmpty else { return .absent } + guard let files = try? await remoteFiles(location, connect) else { return .unknown } for candidate in candidates { do { _ = try await files.read(path: candidate.path, symlinks: .followWithinRoot, maxBytes: 1) - return candidate + return .found(candidate) } catch FileServiceError.tooLarge { - return candidate + return .found(candidate) } catch FileServiceError.notFound, FileServiceError.refused { continue } catch { - // The host, not the candidate: the next one would fail the same way. + // The host, not the candidate: the next one would fail the same way. That includes an + // agent turning the read away as busy (`file.rs`, `MAX_CONCURRENT_READS`). NSLog("Workroom: probing the host for %@: %@", candidate.path, "\(error)") - return nil + return .unknown } } - return nil + return .absent } /// Reconstruct a minimal `TerminalTarget` from its id for the `onFocused` callback (which only diff --git a/macapp/WorkroomApp/WorkroomApp.swift b/macapp/WorkroomApp/WorkroomApp.swift index 2295ff158..eed66fcab 100644 --- a/macapp/WorkroomApp/WorkroomApp.swift +++ b/macapp/WorkroomApp/WorkroomApp.swift @@ -759,7 +759,7 @@ final class AppDelegate: NSObject, NSApplicationDelegate, UNUserNotificationCent // two seconds of a quit. group.enter() Task { - await WakefulnessModel.shared.drainKeep() + await WakefulnessModel.drainAllKeeps() group.leave() } group.notify(queue: .main) { sender.reply(toApplicationShouldTerminate: true) } diff --git a/macapp/WorkroomAppTests/AgentWakefulnessTests.swift b/macapp/WorkroomAppTests/AgentWakefulnessTests.swift index 6f2b8e600..72fec68ca 100644 --- a/macapp/WorkroomAppTests/AgentWakefulnessTests.swift +++ b/macapp/WorkroomAppTests/AgentWakefulnessTests.swift @@ -203,6 +203,38 @@ final class AgentWakefulnessTests: XCTestCase { XCTAssertNil(model.status) } + /// A remote host's "Keep awake" connects the host first, as this Mac's spawns an agent: a click + /// is the one caller for which a dropped connection is not an answer. The prompt watch never + /// connects, as the poll never does. + @MainActor + func testOnlyARemoteKeepConnectsTheHost() async throws { + let manager = HostConnectionManager() + let host = HostID.remote(UUID()) + let connects = Connects() + let transport = WakefulnessModel.Transport.on(host, manager: manager) { connects.add($0) } + do { + _ = try await transport.prompts() + XCTFail("a remote host with no connection gave a prompt stream") + } catch RepositoryRoutingError.unavailable(let refused) { + XCTAssertEqual(refused, host) + } + XCTAssertEqual(connects.hosts, [], "the prompt watch connected the host") + do { + try await transport.keep() + XCTFail("a keep with no connection after connecting reported success") + } catch RepositoryRoutingError.unavailable(let refused) { + XCTAssertEqual(refused, host) + } + XCTAssertEqual(connects.hosts, [host], "a keep did not connect the host first") + } + + private final class Connects: @unchecked Sendable { + private let lock = NSLock() + private var asked: [HostID] = [] + func add(_ host: HostID) { lock.withLock { asked.append(host) } } + var hosts: [HostID] { lock.withLock { asked } } + } + func testAnErrorReplyIsAServiceFailure() { let json = #"{"version":1,"error":{"unsupported":"status requests are ..."}}"# XCTAssertThrowsError(try decode(json)) { error in diff --git a/macapp/WorkroomAppTests/RemoteHostIntegrationTests.swift b/macapp/WorkroomAppTests/RemoteHostIntegrationTests.swift index 04ba09e65..6a4b63cda 100644 --- a/macapp/WorkroomAppTests/RemoteHostIntegrationTests.swift +++ b/macapp/WorkroomAppTests/RemoteHostIntegrationTests.swift @@ -205,7 +205,7 @@ final class RemoteHostIntegrationTests: XCTestCase { let target = TerminalTarget( id: "remote", title: "remote", path: path, unavailability: .remote, remoteHost: id) let sessions = TerminalSessions() - sessions.remoteFiles = { location in + sessions.remoteFiles = { location, _ in try connection.files(context: FileContext(location: location, sharedLocation: nil)) } let found = await sessions.remoteFile("only-here.rb:1", cwd: "\(path)/lib", target: target) diff --git a/macapp/WorkroomAppTests/TerminalLinkOpenerTests.swift b/macapp/WorkroomAppTests/TerminalLinkOpenerTests.swift index becaab3b0..f1d04fc69 100644 --- a/macapp/WorkroomAppTests/TerminalLinkOpenerTests.swift +++ b/macapp/WorkroomAppTests/TerminalLinkOpenerTests.swift @@ -569,23 +569,16 @@ final class TerminalLinkOpenerTests: XCTestCase { XCTAssertEqual( TerminalLinkOpener.remoteCandidates(for: link, cwd: root, root: root), [], link) } - } - - private struct HostFiles: FileProviding { - let context: FileContext - let files: [String: Data] - - func list() async throws -> CommandResult { - CommandResult(stdout: "", stderr: "", exitCode: 0, timedOut: false) - } - func read(path: String, symlinks: FileSymlinkPolicy, maxBytes: Int) async throws -> Data { - guard let data = files[path] else { throw FileServiceError.notFound(path) } - guard data.count <= maxBytes else { throw FileServiceError.tooLarge } - return data - } - func watch(root: String, onEvent: @escaping @Sendable (FileWatchEvent) -> Void) async throws - -> FileWatchHandle? - { nil } + // Value: protects=a remote link never resolves against this Mac's cwd; + // fails_when=the absolute-cwd or absolute-root guard is dropped; + // why_new=rows above only use absolute roots and cwds; seam=none + // A cwd that is not absolute is ignored for the root, and a root that is not absolute gives + // nothing: `URL` would otherwise resolve either one against THIS Mac's working directory. + XCTAssertEqual( + TerminalLinkOpener.remoteCandidates(for: "a.rb", cwd: "lib", root: root).map(\.path), ["a.rb"] + ) + XCTAssertEqual( + TerminalLinkOpener.remoteCandidates(for: "a.rb", cwd: nil, root: "relative/repo"), []) } /// The acceptance test for C7: the workroom's path exists on THIS Mac too, holding a different @@ -604,7 +597,7 @@ final class TerminalLinkOpenerTests: XCTestCase { id: "remote", title: "remote", path: root.path, unavailability: .remote, remoteHost: host) let location = try XCTUnwrap(target.remoteLocation) let sessions = TerminalSessions() - sessions.remoteFiles = { location in + sessions.remoteFiles = { location, _ in HostFiles( context: FileContext(location: location, sharedLocation: nil), files: ["only-on-host.rb": Data("puts 1\n".utf8)]) @@ -627,7 +620,7 @@ final class TerminalLinkOpenerTests: XCTestCase { let sessions = TerminalSessions() sessions.makeView = { _, cwd, _ in GhosttySurfaceView(workingDirectory: cwd) } sessions.recordUnrecognizedTool = { _ in } - sessions.remoteFiles = { location in + sessions.remoteFiles = { location, _ in HostFiles( context: FileContext(location: location, sharedLocation: nil), files: ["lib/user.rb": Data("class User; end\n".utf8)]) @@ -645,6 +638,116 @@ final class TerminalLinkOpenerTests: XCTestCase { XCTAssertEqual(opened()?.path, "lib/user.rb") } + /// What the probe makes of the host's answers, one candidate at a time. A refused candidate (a + /// link out of the root, a directory) is skipped for the next, as a missing one is. Any other + /// failure is the HOST's (it dropped, the agent errored), so the next candidate would fail the + /// same way and the probe gives up with nothing, rather than reporting a file it never read. + // Value: protects=the host probe skips refused candidates but stops on a host failure; + // fails_when=refused stops the probe, or a host failure continues to a later candidate; + // why_new=existing tests only reach found, tooLarge and notFound; seam=none + @MainActor + func testTheRemoteProbeSkipsRefusedCandidatesAndStopsOnAHostFailure() async throws { + let target = TerminalTarget( + id: "remote", title: "remote", path: "/home/workroom/repo", unavailability: .remote, + remoteHost: UUID()) + let sessions = TerminalSessions() + let files: [String: Data] = ["a.rb": Data("x\n".utf8)] + func provider(failing: [String: FileServiceError]) -> (RepositoryLocation, Bool) async throws + -> FileProviding + { + { location, _ in + HostFiles( + context: FileContext(location: location, sharedLocation: nil), files: files, + failing: failing) + } + } + + // `a.rb:3` probes the literal first, then `a.rb` carrying line 3. + sessions.remoteFiles = provider(failing: ["a.rb:3": .refused("not a regular file")]) + let skipped = await sessions.remoteFile("a.rb:3", cwd: nil, target: target) + XCTAssertEqual(skipped, .init(path: "a.rb", line: 3, column: nil)) + + sessions.remoteFiles = provider(failing: ["a.rb:3": .failed("agent dropped")]) + let stopped = await sessions.remoteFile("a.rb:3", cwd: nil, target: target) + XCTAssertNil(stopped, "a host failure went on to probe a later candidate") + // ...and it is no answer, not "no file", so a hover asks again rather than caching a "no". + let busy = await sessions.probeRemote("a.rb:3", cwd: nil, target: target, connect: false) + XCTAssertEqual(busy, .unknown) + let missing = await sessions.probeRemote("b.rb", cwd: nil, target: target, connect: false) + XCTAssertEqual(missing, .absent) + + // The host's service itself unreachable: nothing to probe, nothing found, and no answer. + sessions.remoteFiles = { _, _ in throw RepositoryRoutingError.unavailable(.remote(UUID())) } + let unreachable = await sessions.remoteFile("a.rb", cwd: nil, target: target) + XCTAssertNil(unreachable) + let unanswered = await sessions.probeRemote("a.rb", cwd: nil, target: target, connect: false) + XCTAssertEqual(unanswered, .unknown) + } + + /// A hover never connects the host and a click does (#254): a cursor shape is not a reason to + /// reach a host, and a ⌘-click is a deliberate act, like opening the workroom. + @MainActor + func testOnlyAClickMayConnectARemotePanesHost() async throws { + let target = TerminalTarget( + id: "remote", title: "remote", path: "/home/workroom/repo", unavailability: .remote, + remoteHost: UUID()) + let asked = ConnectRequests() + let sessions = TerminalSessions() + sessions.makeView = { _, cwd, _ in GhosttySurfaceView(workingDirectory: cwd) } + sessions.recordUnrecognizedTool = { _ in } + sessions.remoteFiles = { location, connect in + asked.record(connect) + return HostFiles( + context: FileContext(location: location, sharedLocation: nil), + files: ["a.rb": Data("x\n".utf8)]) + } + let tab = sessions.addTab(for: target) + let view = try XCTUnwrap(sessions.view(forTab: tab.id, inTarget: target.id)) + _ = view.resolveCmdHoverFile?("a.rb") + for _ in 0..<200 where asked.all.isEmpty { try await Task.sleep(for: .milliseconds(5)) } + XCTAssertEqual(asked.all, [false], "a hover connected the host") + view.onCmdClickFile?("a.rb") + for _ in 0..<200 where asked.all.count < 2 { try await Task.sleep(for: .milliseconds(5)) } + XCTAssertEqual(asked.all, [false, true], "a click did not connect the host") + } + + /// A link libghostty hands over as a URL (OSC 8, or a printed `file:` URL) reaches the host's + /// file in a remote pane, whether it is a `file:` URL or a bare path: through `remoteLink`, which + /// decodes percent-escapes and takes a `file:` URL's path. + // Value: protects=a remote pane's URL links open the host's file, decoded; + // fails_when=onOpenURL stops routing to the host or remoteLink stops decoding; + // why_new=only the word-based click path is tested; seam=none + @MainActor + func testAnOpenURLLinkInARemotePaneOpensTheHostsFile() async throws { + let target = TerminalTarget( + id: "remote", title: "remote", path: "/home/workroom/repo", unavailability: .remote, + remoteHost: UUID()) + let sessions = TerminalSessions() + sessions.makeView = { _, cwd, _ in GhosttySurfaceView(workingDirectory: cwd) } + sessions.recordUnrecognizedTool = { _ in } + sessions.remoteFiles = { location, _ in + HostFiles( + context: FileContext(location: location, sharedLocation: nil), + files: ["lib/user.rb": Data("class User; end\n".utf8), "my file.rb": Data("x\n".utf8)]) + } + let tab = sessions.addTab(for: target) + let view = try XCTUnwrap(sessions.view(forTab: tab.id, inTarget: target.id)) + func opened() -> String? { + for tab in sessions.tabs(for: target) { + if case .file(let file) = tab.content { return file.path } + } + return nil + } + func click(_ link: String, opens path: String) async throws { + let url = try XCTUnwrap(URL(string: link), link) + XCTAssertEqual(view.onOpenURL?(url), true, "the click was not consumed: \(link)") + for _ in 0..<200 where opened() != path { try await Task.sleep(for: .milliseconds(10)) } + XCTAssertEqual(opened(), path, link) + } + try await click("file:///home/workroom/repo/lib/user.rb", opens: "lib/user.rb") + try await click("my%20file.rb", opens: "my file.rb") + } + /// A root recorded with a trailing slash, or a link with a doubled one, still resolves inside /// the workroom: `standardized` keeps `//`, and the host reads `/src/a.rb` as an absolute path. func testARemoteLinkSurvivesDoubledAndTrailingSlashes() { @@ -659,12 +762,28 @@ final class TerminalLinkOpenerTests: XCTestCase { \.path), ["lib/a.rb"]) } - /// What the host holds, changeable mid-test. + /// What the host holds, changeable mid-test, and how many probes asked it. private final class HostDisk: @unchecked Sendable { private let lock = NSLock() private var files: [String: Data] = [:] + private var asked = 0 func put(_ path: String) { lock.withLock { files[path] = Data("x".utf8) } } - var snapshot: [String: Data] { lock.withLock { files } } + /// The files as one probe sees them, counted as that probe. + func probe() -> [String: Data] { + lock.withLock { + asked += 1 + return files + } + } + var probes: Int { lock.withLock { asked } } + } + + /// Each `remoteFiles` call's `connect` flag, in order. + private final class ConnectRequests: @unchecked Sendable { + private let lock = NSLock() + private var flags: [Bool] = [] + func record(_ connect: Bool) { lock.withLock { flags.append(connect) } } + var all: [Bool] { lock.withLock { flags } } } /// The ⌘-click gate (`GhosttySurfaceView.mouseDown` asks `resolveCmdHoverFile` synchronously): @@ -680,10 +799,11 @@ final class TerminalLinkOpenerTests: XCTestCase { let sessions = TerminalSessions() sessions.makeView = { _, cwd, _ in GhosttySurfaceView(workingDirectory: cwd) } sessions.recordUnrecognizedTool = { _ in } - sessions.remoteHoverTTL = .milliseconds(200) - sessions.remoteFiles = { location in + // Long enough that no answer goes stale mid-test, so nothing here races a clock. + sessions.remoteHoverTTL = .seconds(3600) + sessions.remoteFiles = { location, _ in HostFiles( - context: FileContext(location: location, sharedLocation: nil), files: disk.snapshot) + context: FileContext(location: location, sharedLocation: nil), files: disk.probe()) } let tab = sessions.addTab(for: target) // Through the view's own closure, which is what the real gate reads. @@ -693,17 +813,45 @@ final class TerminalLinkOpenerTests: XCTestCase { XCTAssertFalse(gate("https://example.com/a.rb")) XCTAssertFalse(gate("/etc/passwd")) XCTAssertTrue(gate("later.rb"), "a click before the host answered would be lost") - // The host says no: the gate follows once the answer is in. + // The host says no: the gate follows once the answer is in, and holds it while it is fresh. for _ in 0..<200 where gate("later.rb") { try await Task.sleep(for: .milliseconds(5)) } XCTAssertFalse(gate("later.rb")) - // The file appears; once the "no" is stale, the host is asked again and says yes. + XCTAssertEqual(disk.probes, 1) + // The file appears. Once the "no" is stale the host is asked again: exactly one more probe, + // whose "yes" must then hold. A pending answer also reads true, so the test waits for the + // probe and then requires the gate never to read false. disk.put("later.rb") - try await Task.sleep(for: .milliseconds(250)) + sessions.remoteHoverTTL = .zero XCTAssertTrue(gate("later.rb")) + sessions.remoteHoverTTL = .seconds(3600) + for _ in 0..<200 where disk.probes < 2 { try await Task.sleep(for: .milliseconds(5)) } + XCTAssertEqual(disk.probes, 2) for _ in 0..<100 { - _ = gate("later.rb") + XCTAssertTrue(gate("later.rb"), "a file made after its word was hovered stayed unclickable") try await Task.sleep(for: .milliseconds(5)) } - XCTAssertTrue(gate("later.rb"), "a file made after its word was hovered stayed unclickable") + XCTAssertEqual(disk.probes, 2, "a fresh answer was asked again") } } + +/// A remote host's file service for the remote-pane tests (#254): `files` by workroom-relative path, +/// reads capped as the agent caps them. Shared with `RemotePaneFooterTests`. +struct HostFiles: FileProviding { + let context: FileContext + let files: [String: Data] + /// Paths whose read fails with this error, before `files` is consulted. + var failing: [String: FileServiceError] = [:] + + func list() async throws -> CommandResult { + CommandResult(stdout: "", stderr: "", exitCode: 0, timedOut: false) + } + func read(path: String, symlinks: FileSymlinkPolicy, maxBytes: Int) async throws -> Data { + if let error = failing[path] { throw error } + guard let data = files[path] else { throw FileServiceError.notFound(path) } + guard data.count <= maxBytes else { throw FileServiceError.tooLarge } + return data + } + func watch(root: String, onEvent: @escaping @Sendable (FileWatchEvent) -> Void) async throws + -> FileWatchHandle? + { nil } +} diff --git a/macapp/WorkroomAppTests/TerminalSessionsTests.swift b/macapp/WorkroomAppTests/TerminalSessionsTests.swift index f5e1bd039..144333206 100644 --- a/macapp/WorkroomAppTests/TerminalSessionsTests.swift +++ b/macapp/WorkroomAppTests/TerminalSessionsTests.swift @@ -1877,6 +1877,47 @@ final class RemotePaneFooterTests: XCTestCase { XCTAssertEqual(connection.asked, [session], "asked once, when the host connected") } + /// ⌘-click in a remote pane resolves a relative path against the directory the host's agent + /// reports for the shell (`hostCwd`), not the workroom's root and not this Mac's directory for + /// the pane (#254, C7). + // Value: protects=a remote ⌘-click resolves against the shell's directory on the host; + // fails_when=the pane's hostCwd stops reaching the click, so it resolves from the root; + // why_new=other C7 tests pass the cwd to remoteFile directly, never via the pane; seam=none + func testACmdClickInARemotePaneResolvesAgainstTheHostsDirectory() async throws { + let hostID = UUID() + let session = UUID() + registerRemote(session, on: hostID) + let connection = HostCwdConnection(directory: "/home/w/lib") + let s = try await makeSessions(connection, on: .remote(hostID)) + s.remoteFiles = { location, _ in + HostFiles( + context: FileContext(location: location, sharedLocation: nil), + files: ["lib/user.rb": Data("class User; end\n".utf8)]) + } + let remote = TerminalTarget( + id: target.id, title: "r", path: "/home/w", unavailability: .remote, remoteHost: hostID) + s.addTab(for: remote, sessionID: session) + let view = try XCTUnwrap(s.tabs(for: remote).first?.surface) + view.persistentSessionID = session + view.onTitleChange?("~") + func hostCwd() -> String? { + guard case .terminal(let state)? = s.tabs(for: remote).first?.content else { return nil } + return state.hostCwd + } + try await waitFor { hostCwd() == "/home/w/lib" } + XCTAssertEqual(hostCwd(), "/home/w/lib") + + view.onCmdClickFile?("user.rb") + func opened() -> String? { + for tab in s.tabs(for: remote) { + if case .file(let file) = tab.content { return file.path } + } + return nil + } + try await waitFor { opened() != nil } + XCTAssertEqual(opened(), "lib/user.rb") + } + func testALocalPaneAndAHostWithNoConnectionAskNothing() async throws { let connection = HostCwdConnection(directory: "/home/w") let s = try await makeSessions(connection, on: .remote(UUID())) From 664a57585857abb22a959a0bd8a2ac364e4df57e Mon Sep 17 00:00:00 2001 From: Joel Moss Date: Sun, 4 Oct 2026 09:25:53 +0100 Subject: [PATCH 08/14] docs(todos): P0 for the CredentialRelay test that fails under load (#322) Co-Authored-By: Claude Opus 5.5 --- TODOS.md | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/TODOS.md b/TODOS.md index 8fa16cc39..ee3216ea1 100644 --- a/TODOS.md +++ b/TODOS.md @@ -5,6 +5,27 @@ > **Recently done** at the bottom — including the traps found while doing it, which are the parts worth > reading before touching the same code. Full write-ups for finished items live in git history. +## P0 — before the next release + +### `CredentialRelayTests.testIdleStrangersDoNotBlockAKnownWorkroom` fails under full-suite load (macapp) — #309 follow-up, issue #322 + +**What:** The test failed once in a full `make app-test` run with `failed - connect 54` +(`ECONNRESET`): one of its `maxConcurrent + 4` idle "stranger" connects to the relay's loopback +listener, or the final real request's connect, was reset rather than accepted. Seen on +`feat/254-remote-pane-parity` (2026-10-04, xcresult `Test-WorkroomApp-2026.10.04_08-52-46`). The +branch doesn't touch `CredentialRelay`. + +**Why:** It passed 5 times out of 5 run alone, and in two other full runs the same day, so it looks +load-dependent. That is either a flaky test or a real limit: a listen backlog that the parallel +test workers' load overflows would also turn real git credential requests away while a box is +busy. + +**How to start:** Find which connect fails (the strangers' or the real one) and the listener's +`listen` backlog in `CredentialRelay`. Then reproduce under load, for example with the suite's +parallel workers, or with many more strangers than `maxConcurrent`. + +**Priority:** P0 (chosen at ship time on #254) + ## P1 — before GA ## P2 — perf, correctness, and the next VCS phase From 1a973e57ec7c5a9173479ccff134c66e363f1276 Mon Sep 17 00:00:00 2001 From: Joel Moss Date: Sun, 4 Oct 2026 09:42:18 +0100 Subject: [PATCH 09/14] fix(macapp): an unreachable host no longer holds every remote cmd-click (#254) From the second review pass: - The first pass stopped caching an unanswered hover probe. With hover no longer connecting the host, that kept the cmd-click gate at "yes" for a whole outage, so every cmd-click on any word was consumed and the host was probed on every pointer move (measured: 200 probes). An unanswered probe is now held as "no" for 0.5 s, then the host is asked again. - A remote keep may have to reconnect over ssh first, so quitting waits up to 15 s for it, not this Mac's 6 s. The doc says a slower keep can still miss the quit. - The hover cache's doc comment sits on the cache again; the design doc's idle-state item is marked done and its paragraph is re-wrapped. Co-Authored-By: Claude Opus 5.5 --- docs/designs/remote-workrooms.md | 18 ++++--- .../WorkroomApp/Core/AgentWakefulness.swift | 9 +++- .../WorkroomApp/Core/TerminalSessions.swift | 37 ++++++++------ .../TerminalLinkOpenerTests.swift | 51 +++++++++++++++++++ 4 files changed, 90 insertions(+), 25 deletions(-) diff --git a/docs/designs/remote-workrooms.md b/docs/designs/remote-workrooms.md index 41f90edec..ee8fc9173 100644 --- a/docs/designs/remote-workrooms.md +++ b/docs/designs/remote-workrooms.md @@ -1711,7 +1711,8 @@ these are the subsystems that actually gate "a remote workroom is a real workroo - Per-workroom credential: enrol on create, revoke on destroy, and revoke on the failure paths too. It is a broker grant, not a deploy key (OQ20). - Idle state per remote workroom in the sidebar. **Cost display is deliberately deferred** — - it needs per-provider billing APIs and there is no spending policy yet (open question 7). + it needs per-provider billing APIs and there is no spending policy yet (open question 7). Idle + state done: see "As built (#254)" below. - **As built (#254, remote pane parity).** Pane close was done first, in #306 (#283). The rest: - **Cmd-click (C7).** A remote pane's links never reach `TerminalLinkOpener`'s local resolver. `TerminalSessions.wireRemoteLinks` resolves a clicked path against the shell's directory on the @@ -1732,8 +1733,9 @@ these are the subsystems that actually gate "a remote workroom is a real workroo path outside the workroom is never a candidate, so it still reaches libghostty. A hover's probe never connects the host, as no passive read of a host does; only the click's probe goes through `RepositoryRouter`, which connects it. A probe that gets no answer (no connection, or - the agent turning the read away as busy past `MAX_CONCURRENT_READS`) is not cached as "no - file": the next hover asks again. + the agent turning the read away as busy past `MAX_CONCURRENT_READS`) is held as "no" for only + 0.5 s (`remoteHoverRetry`): long enough that a host out of reach makes the gate fall through + rather than take every ⌘-click, short enough that the next hover soon asks again. - **Ports.** `PortForwardingModel` is one model per host (`model(for:)`), kept for the launch, and the inspector's Ports section shows the selected workroom's host. A remote workroom the app can't reach shows no Ports section, rather than the Mac's list. A forward never connects a @@ -1741,10 +1743,12 @@ these are the subsystems that actually gate "a remote workroom is a real workroo - **Idle state.** A remote workroom's sidebar row carries the wakefulness badge, backed by one `WakefulnessModel` per remote host (`model(forHost:)`). It polls only while the row is shown and never connects the host. Its "Keep awake" click does connect it first - (`RemoteHosts.ensureConnected`), as the Mac's own spawns an agent, and quitting drains every - host's in-flight keep, not only the Mac's (`WakefulnessModel.drainAllKeeps`). The Linux agent runs the classifier unconditionally (`serve.rs`), - so neither the agent nor host setup changed. A remote badge's tooltip leaves out the - settings-mismatch sentence: that agent was never started with the Mac's settings. + (`RemoteHosts.ensureConnected`), as the Mac's own spawns an agent. Quitting drains every host's + in-flight keep, not only the Mac's (`WakefulnessModel.drainAllKeeps`), waiting up to 15 s for a + remote one, which may have to reconnect over ssh first; a keep slower than that can miss the + quit. The Linux agent runs the classifier unconditionally (`serve.rs`), so neither the agent nor + host setup changed. A remote badge's tooltip leaves out the settings-mismatch sentence: that + agent was never started with the Mac's settings. - **Known limits.** The viewer opens a cmd-clicked file at the top: `FileDescriptor` has no line. A path outside the workroom (`/etc/hosts`, a sibling checkout) opens nothing. The ceiling prompt is still raised only for the Mac's agent: a remote host's "Keep awake" works from its badge, but diff --git a/macapp/WorkroomApp/Core/AgentWakefulness.swift b/macapp/WorkroomApp/Core/AgentWakefulness.swift index defecaf21..7c303817e 100644 --- a/macapp/WorkroomApp/Core/AgentWakefulness.swift +++ b/macapp/WorkroomApp/Core/AgentWakefulness.swift @@ -460,10 +460,15 @@ final class WakefulnessModel: ObservableObject { /// Every model's in-flight "Keep awake", this Mac's and each remote host's, for quit. At once, so /// the quit waits for the slowest, not their sum. A remote host's badge is its only keep control. + /// + /// A remote keep may have to reconnect first, over ssh (`ConnectTimeout 10`), so it gets longer + /// than this Mac's. Longer still is possible (a relayed workroom's relay install runs on that + /// connect), and such a keep can miss the quit; the bound is what a quit is worth. static func drainAllKeeps() async { await withTaskGroup(of: Void.self) { group in - for model in [shared] + Array(hosts.values) { - group.addTask { await model.drainKeep() } + group.addTask { await shared.drainKeep() } + for model in hosts.values { + group.addTask { await model.drainKeep(timeout: .seconds(15)) } } } } diff --git a/macapp/WorkroomApp/Core/TerminalSessions.swift b/macapp/WorkroomApp/Core/TerminalSessions.swift index 8b1354d92..9c129db43 100644 --- a/macapp/WorkroomApp/Core/TerminalSessions.swift +++ b/macapp/WorkroomApp/Core/TerminalSessions.swift @@ -2193,15 +2193,21 @@ final class TerminalSessions: ObservableObject { return try await RepositoryRouter.shared.files(for: location) } - /// What the host said about a word in a remote pane, keyed by target, host cwd and word: nil - /// while its probe is out. The hover callback is synchronous and is also the ⌘-click's gate - /// (`GhosttySurfaceView.mouseDown`), and the probe is a round trip, so an answer can't wait. // ponytail: cleared whole at `remoteHoverLimit` entries; an LRU if hovering ever thrashes it. private static let remoteHoverLimit = 256 - private var remoteHoverAnswers: [String: (found: Bool?, at: ContinuousClock.Instant)] = [:] - /// How long a host's answer stands before a hover asks again: a file can appear, and a probe can - /// fail on a passing disconnect. Swapped by tests. + /// What the host said about a word in a remote pane, keyed by target, host cwd and word: nil + /// while its probe is out, and `unanswered` when the probe got no answer. The hover callback is + /// synchronous and is also the ⌘-click's gate (`GhosttySurfaceView.mouseDown`), and the probe is + /// a round trip, so an answer can't wait. + private var remoteHoverAnswers: + [String: (found: Bool?, at: ContinuousClock.Instant, unanswered: Bool)] = [:] + /// How long a host's answer stands before a hover asks again: a file can appear. Swapped by + /// tests. var remoteHoverTTL: Duration = .seconds(3) + /// How long a probe that got no answer (no connection, an agent busy with other reads) is held + /// as "no": briefly, since it says nothing about the file, but held, so a host that is out of + /// reach makes the gate fall through rather than take every ⌘-click. + var remoteHoverRetry: Duration = .milliseconds(500) /// ⌘-click in a remote pane resolves on the HOST (C7): against the shell's directory there, as /// its agent reports it (`hostCwd`), and through the host's file service. The file opens in the @@ -2256,21 +2262,20 @@ final class TerminalSessions: ObservableObject { guard !TerminalLinkOpener.remoteCandidates(for: word, cwd: cwd, root: target.path).isEmpty else { return false } let key = [target.id, cwd ?? "", word].joined(separator: "\u{0}") - let now = ContinuousClock.now - if let answer = remoteHoverAnswers[key], now - answer.at < remoteHoverTTL { + if let answer = remoteHoverAnswers[key], + ContinuousClock.now - answer.at < (answer.unanswered ? remoteHoverRetry : remoteHoverTTL) + { return answer.found ?? true } if remoteHoverAnswers.count >= Self.remoteHoverLimit { remoteHoverAnswers.removeAll() } - remoteHoverAnswers[key] = (nil, now) + remoteHoverAnswers[key] = (nil, .now, false) Task { [weak self] in - guard let probe = await self?.probeRemote(word, cwd: cwd, target: target, connect: false) - else { return } + guard let self else { return } + let probe = await probeRemote(word, cwd: cwd, target: target, connect: false) switch probe { - case .found: self?.remoteHoverAnswers[key] = (true, .now) - case .absent: self?.remoteHoverAnswers[key] = (false, .now) - // No answer (no connection, an agent busy with other reads) is not "no file": the next - // hover asks again rather than holding a wrong "no" for `remoteHoverTTL`. - case .unknown: self?.remoteHoverAnswers[key] = nil + case .found: remoteHoverAnswers[key] = (true, .now, false) + case .absent: remoteHoverAnswers[key] = (false, .now, false) + case .unknown: remoteHoverAnswers[key] = (false, .now, true) } } return true diff --git a/macapp/WorkroomAppTests/TerminalLinkOpenerTests.swift b/macapp/WorkroomAppTests/TerminalLinkOpenerTests.swift index f1d04fc69..9c47dc9be 100644 --- a/macapp/WorkroomAppTests/TerminalLinkOpenerTests.swift +++ b/macapp/WorkroomAppTests/TerminalLinkOpenerTests.swift @@ -762,6 +762,57 @@ final class TerminalLinkOpenerTests: XCTestCase { \.path), ["lib/a.rb"]) } + /// A host that gives no answer (its connection is down, its agent busy) makes the gate fall + /// through rather than take every ⌘-click: the unanswered probe is held as "no", briefly, and + /// then the host is asked again. Holding nothing kept the gate at "yes" for a whole outage. + @MainActor + func testAnUnansweredHoverProbeFallsThroughThenAsksAgain() async throws { + let target = TerminalTarget( + id: "remote", title: "remote", path: "/home/workroom/repo", unavailability: .remote, + remoteHost: UUID()) + let disk = HostDisk() + disk.put("a.rb") + let reachable = Flag() + let sessions = TerminalSessions() + sessions.makeView = { _, cwd, _ in GhosttySurfaceView(workingDirectory: cwd) } + sessions.recordUnrecognizedTool = { _ in } + sessions.remoteHoverTTL = .seconds(3600) + sessions.remoteHoverRetry = .seconds(3600) + sessions.remoteFiles = { location, _ in + let files = disk.probe() + guard reachable.value else { throw RepositoryRoutingError.unavailable(location.host) } + return HostFiles(context: FileContext(location: location, sharedLocation: nil), files: files) + } + let tab = sessions.addTab(for: target) + let view = try XCTUnwrap(sessions.view(forTab: tab.id, inTarget: target.id)) + let gate = { (word: String) in view.resolveCmdHoverFile?(word) == true } + + XCTAssertTrue(gate("a.rb"), "pending") + for _ in 0..<200 where gate("a.rb") { try await Task.sleep(for: .milliseconds(5)) } + XCTAssertFalse(gate("a.rb"), "an unreachable host held every ⌘-click") + XCTAssertEqual(disk.probes, 1) + // Once the short hold is over, the host is asked again; reachable now, it says yes. + reachable.value = true + sessions.remoteHoverRetry = .zero + XCTAssertTrue(gate("a.rb")) + sessions.remoteHoverRetry = .seconds(3600) + for _ in 0..<200 where disk.probes < 2 { try await Task.sleep(for: .milliseconds(5)) } + XCTAssertEqual(disk.probes, 2) + for _ in 0..<100 { + XCTAssertTrue(gate("a.rb")) + try await Task.sleep(for: .milliseconds(5)) + } + } + + private final class Flag: @unchecked Sendable { + private let lock = NSLock() + private var flag = false + var value: Bool { + get { lock.withLock { flag } } + set { lock.withLock { flag = newValue } } + } + } + /// What the host holds, changeable mid-test, and how many probes asked it. private final class HostDisk: @unchecked Sendable { private let lock = NSLock() From 1646ef505fb0eb1757b6e4e60b6d9f13a1ac1a70 Mon Sep 17 00:00:00 2001 From: Joel Moss Date: Sun, 4 Oct 2026 09:58:46 +0100 Subject: [PATCH 10/14] fix(macapp): decide a remote pane's cmd-click gate without the host (#254) The gate on a remote cmd-click (`resolveCmdHoverFile`, read synchronously by `GhosttySurfaceView.mouseDown`) was answered from a cache of the host's replies. Each of three review passes found a new failure in that cache: a click made straight after pressing cmd was lost, every click was taken during an outage, and probes against a dead link piled up while the gate held "yes". The gate now decides from the word alone: a candidate in the workroom with a `/`, a `name.ext` or a `:line` passes, and only the click asks the host, connecting it if it must. A hover makes no request at all. The cache, its TTLs and the hover/click connect split are gone. The costs, which the owner accepted: a path-shaped word with no file behind it shows the hand and its click opens nothing (logged), and a dotless bare name like `Gemfile` is not clickable in a remote pane. Also from the third pass: the quit comment no longer says "two seconds", and the remote keep drain's 15 s is a named constant. Co-Authored-By: Claude Opus 5.5 --- docs/designs/remote-workrooms.md | 23 +- .../WorkroomApp/Core/AgentWakefulness.swift | 13 +- .../WorkroomApp/Core/TerminalLinkOpener.swift | 14 ++ .../WorkroomApp/Core/TerminalSessions.swift | 106 ++------- macapp/WorkroomApp/WorkroomApp.swift | 7 +- .../RemoteHostIntegrationTests.swift | 2 +- .../TerminalLinkOpenerTests.swift | 212 +++++------------- .../TerminalSessionsTests.swift | 2 +- 8 files changed, 116 insertions(+), 263 deletions(-) diff --git a/docs/designs/remote-workrooms.md b/docs/designs/remote-workrooms.md index ee8fc9173..48e9c8b05 100644 --- a/docs/designs/remote-workrooms.md +++ b/docs/designs/remote-workrooms.md @@ -1724,18 +1724,17 @@ these are the subsystems that actually gate "a remote workroom is a real workroo opens in the in-app viewer as the workroom's preview tab, since no editor on the Mac can open a file on another machine. Web URLs still go to the Mac's browser. - **The ⌘-click gate.** `GhosttySurfaceView.mouseDown` passes a ⌘-click on only when the - synchronous `resolveCmdHoverFile` says the word is a file, and the host's answer is a round - trip. So a word that could name a file in the workroom passes while its probe is out, and the - click then asks the host itself. A first version answered "no" until the probe came back, - which lost every click made straight after pressing ⌘ (found in review). An answer stands for - 3 s (`remoteHoverTTL`), so a file made after its word was hovered becomes clickable. The cost: - a ⌘-click on a plain word before its answer arrives is consumed and opens nothing. A URL or a - path outside the workroom is never a candidate, so it still reaches libghostty. A hover's probe - never connects the host, as no passive read of a host does; only the click's probe goes - through `RepositoryRouter`, which connects it. A probe that gets no answer (no connection, or - the agent turning the read away as busy past `MAX_CONCURRENT_READS`) is held as "no" for only - 0.5 s (`remoteHoverRetry`): long enough that a host out of reach makes the gate fall through - rather than take every ⌘-click, short enough that the next hover soon asks again. + synchronous `resolveCmdHoverFile` says the word is a file, and only the host knows, a round + trip away. So the gate decides from the word alone (`TerminalLinkOpener.looksLikeRemotePath`): + a candidate in the workroom that has a `/`, a `name.ext` or a `:line` passes, and the click + asks the host, connecting it if need be (`RepositoryRouter`). A hover never asks the host. + The costs: a path-shaped word with no file behind it shows the hand and its click opens nothing + (logged), and a bare name with none of the three (`Gemfile`) isn't clickable in a remote pane. + A URL or a path outside the workroom never passes, so it still reaches libghostty. This was + first built as a cache of the host's answers behind the gate, and each of the three review + passes found a new failure in it: a click made straight after pressing ⌘ lost, every click + taken during an outage, and probes against a dead link piling up while the gate held "yes". + The owner chose the gate that needs no host (decided 2026-10-04). - **Ports.** `PortForwardingModel` is one model per host (`model(for:)`), kept for the launch, and the inspector's Ports section shows the selected workroom's host. A remote workroom the app can't reach shows no Ports section, rather than the Mac's list. A forward never connects a diff --git a/macapp/WorkroomApp/Core/AgentWakefulness.swift b/macapp/WorkroomApp/Core/AgentWakefulness.swift index 7c303817e..6e03750cb 100644 --- a/macapp/WorkroomApp/Core/AgentWakefulness.swift +++ b/macapp/WorkroomApp/Core/AgentWakefulness.swift @@ -461,18 +461,23 @@ final class WakefulnessModel: ObservableObject { /// Every model's in-flight "Keep awake", this Mac's and each remote host's, for quit. At once, so /// the quit waits for the slowest, not their sum. A remote host's badge is its only keep control. /// - /// A remote keep may have to reconnect first, over ssh (`ConnectTimeout 10`), so it gets longer - /// than this Mac's. Longer still is possible (a relayed workroom's relay install runs on that - /// connect), and such a keep can miss the quit; the bound is what a quit is worth. + /// A remote keep may have to reconnect first, over ssh (`ConnectTimeout 10`), so it gets + /// `remoteKeepDrain`, longer than this Mac's. Longer still is possible (a relayed workroom's relay + /// install runs on that connect), and such a keep can miss the quit; the bound is what a quit is + /// worth. static func drainAllKeeps() async { await withTaskGroup(of: Void.self) { group in group.addTask { await shared.drainKeep() } for model in hosts.values { - group.addTask { await model.drainKeep(timeout: .seconds(15)) } + group.addTask { await model.drainKeep(timeout: remoteKeepDrain) } } } } + /// How long quitting waits for a remote host's in-flight keep: ssh's 10 s connect timeout, plus + /// the agent's greeting and the request. + static let remoteKeepDrain: Duration = .seconds(15) + /// Nil until a poll succeeds, and again once one fails: no agent, an agent that predates the /// service, or a macOS agent (which answers `running: false`) all show nothing rather than a /// guess. diff --git a/macapp/WorkroomApp/Core/TerminalLinkOpener.swift b/macapp/WorkroomApp/Core/TerminalLinkOpener.swift index b73c2ee65..b04b4505d 100644 --- a/macapp/WorkroomApp/Core/TerminalLinkOpener.swift +++ b/macapp/WorkroomApp/Core/TerminalLinkOpener.swift @@ -312,6 +312,20 @@ enum TerminalLinkOpener { } } + /// Whether a remote pane's ⌘-hover cursor and ⌘-click gate pass `word` (#254), decided without + /// the host: it names a candidate in the workroom (`remoteCandidates`) and looks like a path, + /// with a `/`, a `name.ext` or a `:line`. The click then asks the host. The costs: a path-shaped + /// word with no file behind it shows the hand and its click opens nothing, and a bare name with + /// none of the three (`Gemfile`) isn't clickable in a remote pane. + static func looksLikeRemotePath(_ word: String, cwd: String?, root: String) -> Bool { + guard let path = filePath(from: word), + !remoteCandidates(for: word, cwd: cwd, root: root).isEmpty + else { return false } + return path.contains("/") + || path.range(of: "[A-Za-z0-9_-]\\.[A-Za-z0-9]", options: .regularExpression) != nil + || path.range(of: ":[0-9]", options: .regularExpression) != nil + } + /// The text of an open-URL link to resolve as a path in a remote pane: a `file:` URL's path, else /// the link as written. Only after `isSystemHandledURL` has turned it down, as locally. static func remoteLink(from url: URL) -> String { diff --git a/macapp/WorkroomApp/Core/TerminalSessions.swift b/macapp/WorkroomApp/Core/TerminalSessions.swift index 9c129db43..d05508b38 100644 --- a/macapp/WorkroomApp/Core/TerminalSessions.swift +++ b/macapp/WorkroomApp/Core/TerminalSessions.swift @@ -2181,38 +2181,22 @@ final class TerminalSessions: ObservableObject { // MARK: Remote pane links (#254, C7) - /// How a remote pane reaches its host's files, connecting the host when `connect` says so: a - /// click does, as a deliberate act; a hover never does, as no passive read of a host does - /// (`HostConnectionManager.workingDirectory`). Swapped by tests. - var remoteFiles: (RepositoryLocation, _ connect: Bool) async throws -> FileProviding = { - location, connect in - guard connect else { - return try await HostConnectionManager.shared.files( - context: FileContext(location: location, sharedLocation: nil)) - } - return try await RepositoryRouter.shared.files(for: location) - } - - // ponytail: cleared whole at `remoteHoverLimit` entries; an LRU if hovering ever thrashes it. - private static let remoteHoverLimit = 256 - /// What the host said about a word in a remote pane, keyed by target, host cwd and word: nil - /// while its probe is out, and `unanswered` when the probe got no answer. The hover callback is - /// synchronous and is also the ⌘-click's gate (`GhosttySurfaceView.mouseDown`), and the probe is - /// a round trip, so an answer can't wait. - private var remoteHoverAnswers: - [String: (found: Bool?, at: ContinuousClock.Instant, unanswered: Bool)] = [:] - /// How long a host's answer stands before a hover asks again: a file can appear. Swapped by - /// tests. - var remoteHoverTTL: Duration = .seconds(3) - /// How long a probe that got no answer (no connection, an agent busy with other reads) is held - /// as "no": briefly, since it says nothing about the file, but held, so a host that is out of - /// reach makes the gate fall through rather than take every ⌘-click. - var remoteHoverRetry: Duration = .milliseconds(500) + /// How a remote pane reaches its host's files: through the router, which connects the host, since + /// only a click asks and a click is a deliberate act. Swapped by tests. + var remoteFiles: (RepositoryLocation) async throws -> FileProviding = { + try await RepositoryRouter.shared.files(for: $0) + } /// ⌘-click in a remote pane resolves on the HOST (C7): against the shell's directory there, as /// its agent reports it (`hostCwd`), and through the host's file service. The file opens in the /// in-app viewer, since no editor on this Mac can open a file on another machine. Web URLs still /// go to this Mac's browser. + /// + /// The ⌘-hover cursor is also the ⌘-click's gate (`GhosttySurfaceView.mouseDown`), and it is + /// synchronous, so it can't ask the host. It decides from the word alone + /// (`TerminalLinkOpener.looksLikeRemotePath`), and the click asks the host. Gating on a cache + /// of the host's answers regressed on every review of #254: it lost the click made straight + /// after pressing ⌘, took every click during an outage, and let hung probes pile up. private func wireRemoteLinks( _ view: GhosttySurfaceView, tab tabID: TerminalTab.ID, target: TerminalTarget ) { @@ -2220,7 +2204,8 @@ final class TerminalSessions: ObservableObject { self?.openRemoteFile(word, tab: tabID, target: target) } view.resolveCmdHoverFile = { [weak self] word in - self?.remoteFileExists(word, tab: tabID, target: target) ?? false + TerminalLinkOpener.looksLikeRemotePath( + word, cwd: self?.hostCwd(of: tabID, in: target.id), root: target.path) } view.onOpenURL = { [weak self] url in if TerminalLinkOpener.isSystemHandledURL(url) { @@ -2251,78 +2236,31 @@ final class TerminalSessions: ObservableObject { } } - /// The ⌘-hover cursor and the ⌘-click gate. A word the host has answered for gets that answer - /// while it is fresh. Any other word that could name a file in the workroom counts as one while - /// its probe is out, so a ⌘-click made before the answer reaches `openRemoteFile`, which asks the - /// host again; answering "no" until then lost every click made straight after pressing ⌘. The - /// cost: such a click on a word that turns out to be plain text is consumed and opens nothing. - /// A URL or a path outside the workroom is never a candidate, so it still reaches libghostty. - func remoteFileExists(_ word: String, tab tabID: TerminalTab.ID, target: TerminalTarget) -> Bool { - let cwd = hostCwd(of: tabID, in: target.id) - guard !TerminalLinkOpener.remoteCandidates(for: word, cwd: cwd, root: target.path).isEmpty - else { return false } - let key = [target.id, cwd ?? "", word].joined(separator: "\u{0}") - if let answer = remoteHoverAnswers[key], - ContinuousClock.now - answer.at < (answer.unanswered ? remoteHoverRetry : remoteHoverTTL) - { - return answer.found ?? true - } - if remoteHoverAnswers.count >= Self.remoteHoverLimit { remoteHoverAnswers.removeAll() } - remoteHoverAnswers[key] = (nil, .now, false) - Task { [weak self] in - guard let self else { return } - let probe = await probeRemote(word, cwd: cwd, target: target, connect: false) - switch probe { - case .found: remoteHoverAnswers[key] = (true, .now, false) - case .absent: remoteHoverAnswers[key] = (false, .now, false) - case .unknown: remoteHoverAnswers[key] = (false, .now, true) - } - } - return true - } - - /// What a probe of the host found: a file, no file among the candidates, or no answer. - enum RemoteProbe: Equatable { - case found(TerminalLinkOpener.PathCandidate) - case absent - case unknown - } - - /// The first of `word`'s candidates that is a file on the host, or nil, connecting the host if it - /// must: the click's question. + /// The first of `word`'s candidates that is a file on the host, or nil. A read capped at one byte + /// is the probe: it answers `.tooLarge` for any longer file without sending it, and fails for + /// anything missing, refused or outside the workroom. func remoteFile(_ word: String, cwd: String?, target: TerminalTarget) async -> TerminalLinkOpener.PathCandidate? { - guard case .found(let file) = await probeRemote(word, cwd: cwd, target: target, connect: true) - else { return nil } - return file - } - - /// A read capped at one byte is the probe: it answers `.tooLarge` for any longer file without - /// sending it, and fails for anything missing, refused or outside the workroom. - func probeRemote(_ word: String, cwd: String?, target: TerminalTarget, connect: Bool) async - -> RemoteProbe - { - guard let location = target.remoteLocation else { return .absent } + guard let location = target.remoteLocation else { return nil } let candidates = TerminalLinkOpener.remoteCandidates(for: word, cwd: cwd, root: target.path) - guard !candidates.isEmpty else { return .absent } - guard let files = try? await remoteFiles(location, connect) else { return .unknown } + guard !candidates.isEmpty, let files = try? await remoteFiles(location) else { return nil } for candidate in candidates { do { _ = try await files.read(path: candidate.path, symlinks: .followWithinRoot, maxBytes: 1) - return .found(candidate) + return candidate } catch FileServiceError.tooLarge { - return .found(candidate) + return candidate } catch FileServiceError.notFound, FileServiceError.refused { continue } catch { // The host, not the candidate: the next one would fail the same way. That includes an // agent turning the read away as busy (`file.rs`, `MAX_CONCURRENT_READS`). NSLog("Workroom: probing the host for %@: %@", candidate.path, "\(error)") - return .unknown + return nil } } - return .absent + return nil } /// Reconstruct a minimal `TerminalTarget` from its id for the `onFocused` callback (which only diff --git a/macapp/WorkroomApp/WorkroomApp.swift b/macapp/WorkroomApp/WorkroomApp.swift index eed66fcab..774a7cf49 100644 --- a/macapp/WorkroomApp/WorkroomApp.swift +++ b/macapp/WorkroomApp/WorkroomApp.swift @@ -754,9 +754,10 @@ final class AppDelegate: NSObject, NSApplicationDelegate, UNUserNotificationCent // Stop every window's run commands, not just the focused one's. registry.gracefullyStopAllWindows(timeout: 5) { group.leave() } } - // A "Keep awake" clicked moments before quitting (issue #208): the card cleared on the click, - // the request may still be reconnecting. Bounded; a box the user asked to keep awake is worth - // two seconds of a quit. + // A "Keep awake" clicked moments before quitting (issues #208, #254): the card cleared on the + // click, the request may still be reconnecting. Bounded, this Mac's and each remote host's at + // once (`WakefulnessModel.drainAllKeeps`): a box the user asked to keep awake is worth a few + // seconds of a quit. group.enter() Task { await WakefulnessModel.drainAllKeeps() diff --git a/macapp/WorkroomAppTests/RemoteHostIntegrationTests.swift b/macapp/WorkroomAppTests/RemoteHostIntegrationTests.swift index 6a4b63cda..04ba09e65 100644 --- a/macapp/WorkroomAppTests/RemoteHostIntegrationTests.swift +++ b/macapp/WorkroomAppTests/RemoteHostIntegrationTests.swift @@ -205,7 +205,7 @@ final class RemoteHostIntegrationTests: XCTestCase { let target = TerminalTarget( id: "remote", title: "remote", path: path, unavailability: .remote, remoteHost: id) let sessions = TerminalSessions() - sessions.remoteFiles = { location, _ in + sessions.remoteFiles = { location in try connection.files(context: FileContext(location: location, sharedLocation: nil)) } let found = await sessions.remoteFile("only-here.rb:1", cwd: "\(path)/lib", target: target) diff --git a/macapp/WorkroomAppTests/TerminalLinkOpenerTests.swift b/macapp/WorkroomAppTests/TerminalLinkOpenerTests.swift index 9c47dc9be..4d83c531e 100644 --- a/macapp/WorkroomAppTests/TerminalLinkOpenerTests.swift +++ b/macapp/WorkroomAppTests/TerminalLinkOpenerTests.swift @@ -597,7 +597,7 @@ final class TerminalLinkOpenerTests: XCTestCase { id: "remote", title: "remote", path: root.path, unavailability: .remote, remoteHost: host) let location = try XCTUnwrap(target.remoteLocation) let sessions = TerminalSessions() - sessions.remoteFiles = { location, _ in + sessions.remoteFiles = { location in HostFiles( context: FileContext(location: location, sharedLocation: nil), files: ["only-on-host.rb": Data("puts 1\n".utf8)]) @@ -620,7 +620,7 @@ final class TerminalLinkOpenerTests: XCTestCase { let sessions = TerminalSessions() sessions.makeView = { _, cwd, _ in GhosttySurfaceView(workingDirectory: cwd) } sessions.recordUnrecognizedTool = { _ in } - sessions.remoteFiles = { location, _ in + sessions.remoteFiles = { location in HostFiles( context: FileContext(location: location, sharedLocation: nil), files: ["lib/user.rb": Data("class User; end\n".utf8)]) @@ -652,10 +652,10 @@ final class TerminalLinkOpenerTests: XCTestCase { remoteHost: UUID()) let sessions = TerminalSessions() let files: [String: Data] = ["a.rb": Data("x\n".utf8)] - func provider(failing: [String: FileServiceError]) -> (RepositoryLocation, Bool) async throws + func provider(failing: [String: FileServiceError]) -> (RepositoryLocation) async throws -> FileProviding { - { location, _ in + { location in HostFiles( context: FileContext(location: location, sharedLocation: nil), files: files, failing: failing) @@ -670,45 +670,11 @@ final class TerminalLinkOpenerTests: XCTestCase { sessions.remoteFiles = provider(failing: ["a.rb:3": .failed("agent dropped")]) let stopped = await sessions.remoteFile("a.rb:3", cwd: nil, target: target) XCTAssertNil(stopped, "a host failure went on to probe a later candidate") - // ...and it is no answer, not "no file", so a hover asks again rather than caching a "no". - let busy = await sessions.probeRemote("a.rb:3", cwd: nil, target: target, connect: false) - XCTAssertEqual(busy, .unknown) - let missing = await sessions.probeRemote("b.rb", cwd: nil, target: target, connect: false) - XCTAssertEqual(missing, .absent) - - // The host's service itself unreachable: nothing to probe, nothing found, and no answer. - sessions.remoteFiles = { _, _ in throw RepositoryRoutingError.unavailable(.remote(UUID())) } + + // The host's service itself unreachable: nothing to probe, nothing found. + sessions.remoteFiles = { _ in throw RepositoryRoutingError.unavailable(.remote(UUID())) } let unreachable = await sessions.remoteFile("a.rb", cwd: nil, target: target) XCTAssertNil(unreachable) - let unanswered = await sessions.probeRemote("a.rb", cwd: nil, target: target, connect: false) - XCTAssertEqual(unanswered, .unknown) - } - - /// A hover never connects the host and a click does (#254): a cursor shape is not a reason to - /// reach a host, and a ⌘-click is a deliberate act, like opening the workroom. - @MainActor - func testOnlyAClickMayConnectARemotePanesHost() async throws { - let target = TerminalTarget( - id: "remote", title: "remote", path: "/home/workroom/repo", unavailability: .remote, - remoteHost: UUID()) - let asked = ConnectRequests() - let sessions = TerminalSessions() - sessions.makeView = { _, cwd, _ in GhosttySurfaceView(workingDirectory: cwd) } - sessions.recordUnrecognizedTool = { _ in } - sessions.remoteFiles = { location, connect in - asked.record(connect) - return HostFiles( - context: FileContext(location: location, sharedLocation: nil), - files: ["a.rb": Data("x\n".utf8)]) - } - let tab = sessions.addTab(for: target) - let view = try XCTUnwrap(sessions.view(forTab: tab.id, inTarget: target.id)) - _ = view.resolveCmdHoverFile?("a.rb") - for _ in 0..<200 where asked.all.isEmpty { try await Task.sleep(for: .milliseconds(5)) } - XCTAssertEqual(asked.all, [false], "a hover connected the host") - view.onCmdClickFile?("a.rb") - for _ in 0..<200 where asked.all.count < 2 { try await Task.sleep(for: .milliseconds(5)) } - XCTAssertEqual(asked.all, [false, true], "a click did not connect the host") } /// A link libghostty hands over as a URL (OSC 8, or a printed `file:` URL) reaches the host's @@ -725,7 +691,7 @@ final class TerminalLinkOpenerTests: XCTestCase { let sessions = TerminalSessions() sessions.makeView = { _, cwd, _ in GhosttySurfaceView(workingDirectory: cwd) } sessions.recordUnrecognizedTool = { _ in } - sessions.remoteFiles = { location, _ in + sessions.remoteFiles = { location in HostFiles( context: FileContext(location: location, sharedLocation: nil), files: ["lib/user.rb": Data("class User; end\n".utf8), "my file.rb": Data("x\n".utf8)]) @@ -748,141 +714,71 @@ final class TerminalLinkOpenerTests: XCTestCase { try await click("my%20file.rb", opens: "my file.rb") } - /// A root recorded with a trailing slash, or a link with a doubled one, still resolves inside - /// the workroom: `standardized` keeps `//`, and the host reads `/src/a.rb` as an absolute path. - func testARemoteLinkSurvivesDoubledAndTrailingSlashes() { - XCTAssertEqual( - TerminalLinkOpener.remoteCandidates(for: "src/a.rb", cwd: nil, root: "/repo/").map(\.path), - ["src/a.rb"]) - XCTAssertEqual( - TerminalLinkOpener.remoteCandidates(for: "/repo//src/a.rb", cwd: nil, root: "/repo").map( - \.path), ["src/a.rb"]) - XCTAssertEqual( - TerminalLinkOpener.remoteCandidates(for: "a.rb", cwd: "/repo//lib/", root: "/repo").map( - \.path), ["lib/a.rb"]) + /// The remote ⌘-click gate decides from the word alone (#254): a path-shaped candidate passes + /// (a `/`, a `name.ext` or a `:line`), and anything else falls through to the terminal: a plain + /// word, a dotless bare name, a URL, a path outside the workroom. + func testTheRemoteClickGateTakesPathShapedWordsOnly() { + let root = "/home/workroom/repo" + for word in ["lib/user.rb", "user.rb", "user.rb:5", "Makefile:12", "./bin/dev", "a.b.c"] { + XCTAssertTrue(TerminalLinkOpener.looksLikeRemotePath(word, cwd: root, root: root), word) + } + for word in [ + "hello", "Gemfile", "done.", "https://example.com/a.rb", "/etc/passwd", "~/notes.txt", + "../../elsewhere/a.rb", + ] { + XCTAssertFalse(TerminalLinkOpener.looksLikeRemotePath(word, cwd: root, root: root), word) + } } - /// A host that gives no answer (its connection is down, its agent busy) makes the gate fall - /// through rather than take every ⌘-click: the unanswered probe is held as "no", briefly, and - /// then the host is asked again. Holding nothing kept the gate at "yes" for a whole outage. + /// The gate never asks the host: a hover in a remote pane, ⌘ held, makes no request at all, + /// so a host that is slow, down or hung can't lose, take or pile up clicks. Only the click asks. @MainActor - func testAnUnansweredHoverProbeFallsThroughThenAsksAgain() async throws { + func testARemotePanesGateNeverAsksTheHost() async throws { let target = TerminalTarget( id: "remote", title: "remote", path: "/home/workroom/repo", unavailability: .remote, remoteHost: UUID()) - let disk = HostDisk() - disk.put("a.rb") - let reachable = Flag() + let asked = Counter() let sessions = TerminalSessions() sessions.makeView = { _, cwd, _ in GhosttySurfaceView(workingDirectory: cwd) } sessions.recordUnrecognizedTool = { _ in } - sessions.remoteHoverTTL = .seconds(3600) - sessions.remoteHoverRetry = .seconds(3600) - sessions.remoteFiles = { location, _ in - let files = disk.probe() - guard reachable.value else { throw RepositoryRoutingError.unavailable(location.host) } - return HostFiles(context: FileContext(location: location, sharedLocation: nil), files: files) + sessions.remoteFiles = { location in + asked.bump() + return HostFiles( + context: FileContext(location: location, sharedLocation: nil), + files: ["a.rb": Data("x\n".utf8)]) } let tab = sessions.addTab(for: target) let view = try XCTUnwrap(sessions.view(forTab: tab.id, inTarget: target.id)) - let gate = { (word: String) in view.resolveCmdHoverFile?(word) == true } - - XCTAssertTrue(gate("a.rb"), "pending") - for _ in 0..<200 where gate("a.rb") { try await Task.sleep(for: .milliseconds(5)) } - XCTAssertFalse(gate("a.rb"), "an unreachable host held every ⌘-click") - XCTAssertEqual(disk.probes, 1) - // Once the short hold is over, the host is asked again; reachable now, it says yes. - reachable.value = true - sessions.remoteHoverRetry = .zero - XCTAssertTrue(gate("a.rb")) - sessions.remoteHoverRetry = .seconds(3600) - for _ in 0..<200 where disk.probes < 2 { try await Task.sleep(for: .milliseconds(5)) } - XCTAssertEqual(disk.probes, 2) - for _ in 0..<100 { - XCTAssertTrue(gate("a.rb")) - try await Task.sleep(for: .milliseconds(5)) - } - } - - private final class Flag: @unchecked Sendable { - private let lock = NSLock() - private var flag = false - var value: Bool { - get { lock.withLock { flag } } - set { lock.withLock { flag = newValue } } - } + XCTAssertEqual(view.resolveCmdHoverFile?("a.rb"), true) + XCTAssertEqual(view.resolveCmdHoverFile?("hello"), false) + try await Task.sleep(for: .milliseconds(100)) + XCTAssertEqual(asked.count, 0, "a hover asked the host") + view.onCmdClickFile?("a.rb") + for _ in 0..<200 where asked.count == 0 { try await Task.sleep(for: .milliseconds(5)) } + XCTAssertEqual(asked.count, 1, "the click did not ask the host") } - /// What the host holds, changeable mid-test, and how many probes asked it. - private final class HostDisk: @unchecked Sendable { + private final class Counter: @unchecked Sendable { private let lock = NSLock() - private var files: [String: Data] = [:] - private var asked = 0 - func put(_ path: String) { lock.withLock { files[path] = Data("x".utf8) } } - /// The files as one probe sees them, counted as that probe. - func probe() -> [String: Data] { - lock.withLock { - asked += 1 - return files - } - } - var probes: Int { lock.withLock { asked } } + private var value = 0 + func bump() { lock.withLock { value += 1 } } + var count: Int { lock.withLock { value } } } - /// Each `remoteFiles` call's `connect` flag, in order. - private final class ConnectRequests: @unchecked Sendable { - private let lock = NSLock() - private var flags: [Bool] = [] - func record(_ connect: Bool) { lock.withLock { flags.append(connect) } } - var all: [Bool] { lock.withLock { flags } } + /// A root recorded with a trailing slash, or a link with a doubled one, still resolves inside + /// the workroom: `standardized` keeps `//`, and the host reads `/src/a.rb` as an absolute path. + func testARemoteLinkSurvivesDoubledAndTrailingSlashes() { + XCTAssertEqual( + TerminalLinkOpener.remoteCandidates(for: "src/a.rb", cwd: nil, root: "/repo/").map(\.path), + ["src/a.rb"]) + XCTAssertEqual( + TerminalLinkOpener.remoteCandidates(for: "/repo//src/a.rb", cwd: nil, root: "/repo").map( + \.path), ["src/a.rb"]) + XCTAssertEqual( + TerminalLinkOpener.remoteCandidates(for: "a.rb", cwd: "/repo//lib/", root: "/repo").map( + \.path), ["lib/a.rb"]) } - /// The ⌘-click gate (`GhosttySurfaceView.mouseDown` asks `resolveCmdHoverFile` synchronously): - /// a word that could be a file in the workroom passes while its probe is out, so a click made - /// straight after pressing ⌘ is not lost. A "no" stands only while it is fresh, so a file made - /// after its word was hovered is found. URLs and paths outside the workroom never pass. - @MainActor - func testTheRemoteClickGatePassesAWordWhileTheHostIsAskedAndForgetsAStaleNo() async throws { - let target = TerminalTarget( - id: "remote", title: "remote", path: "/home/workroom/repo", unavailability: .remote, - remoteHost: UUID()) - let disk = HostDisk() - let sessions = TerminalSessions() - sessions.makeView = { _, cwd, _ in GhosttySurfaceView(workingDirectory: cwd) } - sessions.recordUnrecognizedTool = { _ in } - // Long enough that no answer goes stale mid-test, so nothing here races a clock. - sessions.remoteHoverTTL = .seconds(3600) - sessions.remoteFiles = { location, _ in - HostFiles( - context: FileContext(location: location, sharedLocation: nil), files: disk.probe()) - } - let tab = sessions.addTab(for: target) - // Through the view's own closure, which is what the real gate reads. - let view = try XCTUnwrap(sessions.view(forTab: tab.id, inTarget: target.id)) - let gate = { (word: String) in view.resolveCmdHoverFile?(word) == true } - - XCTAssertFalse(gate("https://example.com/a.rb")) - XCTAssertFalse(gate("/etc/passwd")) - XCTAssertTrue(gate("later.rb"), "a click before the host answered would be lost") - // The host says no: the gate follows once the answer is in, and holds it while it is fresh. - for _ in 0..<200 where gate("later.rb") { try await Task.sleep(for: .milliseconds(5)) } - XCTAssertFalse(gate("later.rb")) - XCTAssertEqual(disk.probes, 1) - // The file appears. Once the "no" is stale the host is asked again: exactly one more probe, - // whose "yes" must then hold. A pending answer also reads true, so the test waits for the - // probe and then requires the gate never to read false. - disk.put("later.rb") - sessions.remoteHoverTTL = .zero - XCTAssertTrue(gate("later.rb")) - sessions.remoteHoverTTL = .seconds(3600) - for _ in 0..<200 where disk.probes < 2 { try await Task.sleep(for: .milliseconds(5)) } - XCTAssertEqual(disk.probes, 2) - for _ in 0..<100 { - XCTAssertTrue(gate("later.rb"), "a file made after its word was hovered stayed unclickable") - try await Task.sleep(for: .milliseconds(5)) - } - XCTAssertEqual(disk.probes, 2, "a fresh answer was asked again") - } } /// A remote host's file service for the remote-pane tests (#254): `files` by workroom-relative path, diff --git a/macapp/WorkroomAppTests/TerminalSessionsTests.swift b/macapp/WorkroomAppTests/TerminalSessionsTests.swift index 144333206..91508c3d3 100644 --- a/macapp/WorkroomAppTests/TerminalSessionsTests.swift +++ b/macapp/WorkroomAppTests/TerminalSessionsTests.swift @@ -1889,7 +1889,7 @@ final class RemotePaneFooterTests: XCTestCase { registerRemote(session, on: hostID) let connection = HostCwdConnection(directory: "/home/w/lib") let s = try await makeSessions(connection, on: .remote(hostID)) - s.remoteFiles = { location, _ in + s.remoteFiles = { location in HostFiles( context: FileContext(location: location, sharedLocation: nil), files: ["lib/user.rb": Data("class User; end\n".utf8)]) From 1908efc33078f5b5199d972f4039ad1bd9f0e154 Mon Sep 17 00:00:00 2001 From: Joel Moss Date: Sun, 4 Oct 2026 10:28:08 +0100 Subject: [PATCH 11/14] fix(macapp): let dotfiles through a remote pane's cmd-click gate (#254) The gate's name.ext rule needed a character before the dot, so .gitignore, .env and .envrc were never clickable in a remote pane. A leading dot now counts. Found by a read-only review after the review cap; the owner approved the fix. Co-Authored-By: Claude Opus 5.5 --- docs/designs/remote-workrooms.md | 5 +++-- macapp/WorkroomApp/Core/TerminalLinkOpener.swift | 4 ++-- macapp/WorkroomAppTests/TerminalLinkOpenerTests.swift | 5 ++++- 3 files changed, 9 insertions(+), 5 deletions(-) diff --git a/docs/designs/remote-workrooms.md b/docs/designs/remote-workrooms.md index 48e9c8b05..5893380a7 100644 --- a/docs/designs/remote-workrooms.md +++ b/docs/designs/remote-workrooms.md @@ -1726,8 +1726,9 @@ these are the subsystems that actually gate "a remote workroom is a real workroo - **The ⌘-click gate.** `GhosttySurfaceView.mouseDown` passes a ⌘-click on only when the synchronous `resolveCmdHoverFile` says the word is a file, and only the host knows, a round trip away. So the gate decides from the word alone (`TerminalLinkOpener.looksLikeRemotePath`): - a candidate in the workroom that has a `/`, a `name.ext` or a `:line` passes, and the click - asks the host, connecting it if need be (`RepositoryRouter`). A hover never asks the host. + a candidate in the workroom that has a `/`, a `name.ext` (dotfiles included) or a `:line` + passes, and the click asks the host, connecting it if need be (`RepositoryRouter`). A hover + never asks the host. The costs: a path-shaped word with no file behind it shows the hand and its click opens nothing (logged), and a bare name with none of the three (`Gemfile`) isn't clickable in a remote pane. A URL or a path outside the workroom never passes, so it still reaches libghostty. This was diff --git a/macapp/WorkroomApp/Core/TerminalLinkOpener.swift b/macapp/WorkroomApp/Core/TerminalLinkOpener.swift index b04b4505d..d039a5d44 100644 --- a/macapp/WorkroomApp/Core/TerminalLinkOpener.swift +++ b/macapp/WorkroomApp/Core/TerminalLinkOpener.swift @@ -314,7 +314,7 @@ enum TerminalLinkOpener { /// Whether a remote pane's ⌘-hover cursor and ⌘-click gate pass `word` (#254), decided without /// the host: it names a candidate in the workroom (`remoteCandidates`) and looks like a path, - /// with a `/`, a `name.ext` or a `:line`. The click then asks the host. The costs: a path-shaped + /// with a `/`, a `name.ext` (a dotfile such as `.env` counts) or a `:line`. The click then asks the host. The costs: a path-shaped /// word with no file behind it shows the hand and its click opens nothing, and a bare name with /// none of the three (`Gemfile`) isn't clickable in a remote pane. static func looksLikeRemotePath(_ word: String, cwd: String?, root: String) -> Bool { @@ -322,7 +322,7 @@ enum TerminalLinkOpener { !remoteCandidates(for: word, cwd: cwd, root: root).isEmpty else { return false } return path.contains("/") - || path.range(of: "[A-Za-z0-9_-]\\.[A-Za-z0-9]", options: .regularExpression) != nil + || path.range(of: "(^|[A-Za-z0-9_-])\\.[A-Za-z0-9]", options: .regularExpression) != nil || path.range(of: ":[0-9]", options: .regularExpression) != nil } diff --git a/macapp/WorkroomAppTests/TerminalLinkOpenerTests.swift b/macapp/WorkroomAppTests/TerminalLinkOpenerTests.swift index 4d83c531e..c4be67f34 100644 --- a/macapp/WorkroomAppTests/TerminalLinkOpenerTests.swift +++ b/macapp/WorkroomAppTests/TerminalLinkOpenerTests.swift @@ -719,7 +719,10 @@ final class TerminalLinkOpenerTests: XCTestCase { /// word, a dotless bare name, a URL, a path outside the workroom. func testTheRemoteClickGateTakesPathShapedWordsOnly() { let root = "/home/workroom/repo" - for word in ["lib/user.rb", "user.rb", "user.rb:5", "Makefile:12", "./bin/dev", "a.b.c"] { + for word in [ + "lib/user.rb", "user.rb", "user.rb:5", "Makefile:12", "./bin/dev", "a.b.c", ".gitignore", + ".env", + ] { XCTAssertTrue(TerminalLinkOpener.looksLikeRemotePath(word, cwd: root, root: root), word) } for word in [ From 6ce0a1c9d6a20214cf0085d000ac4c03e136bb33 Mon Sep 17 00:00:00 2001 From: Joel Moss Date: Sun, 4 Oct 2026 11:01:42 +0100 Subject: [PATCH 12/14] fix(macapp): remote cmd-click keeps filename bytes, lets the host resolve `..`, drops stale clicks (#254) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit From the final adversarial reviews (Claude, and Codex's structured review), fixed past the review cap with the owner's approval: - Remote paths were normalised through `URL.standardized`, which decomposes Unicode filenames (`é` C3 A9 became `e` + U+0301), so a Linux host never found them. Paths are now split into components and sent byte for byte. - `link/../file.rb` was resolved on the Mac, naming a different file when `link` is a symlink. Containment is still judged on the lexical path, but the host is sent the path with its `..` and resolves it itself. - Each remote click started an unstructured task, so a slow answer could retarget the preview after a faster click, or reopen a tab in a workroom whose panes had all closed. A newer click now cancels the one before it, and an answer for a workroom with no panes opens nothing. - An error reading one path (too long, unreadable) stopped the probe before `file.rb` was tried from `file.rb:12`. Only a transport failure stops it. Documented and deferred by the owner: remote forwards reachable by every account on the Mac (#323), a failed remote keep shows nothing (#324), relative clicks where the root goes through a symlink (#325). Co-Authored-By: Claude Opus 5.5 --- docs/designs/remote-workrooms.md | 17 +++- .../WorkroomApp/Core/TerminalLinkOpener.swift | 34 ++++--- .../WorkroomApp/Core/TerminalSessions.swift | 23 ++++- .../TerminalLinkOpenerTests.swift | 93 ++++++++++++++++++- 4 files changed, 142 insertions(+), 25 deletions(-) diff --git a/docs/designs/remote-workrooms.md b/docs/designs/remote-workrooms.md index 5893380a7..1ec8bdae2 100644 --- a/docs/designs/remote-workrooms.md +++ b/docs/designs/remote-workrooms.md @@ -1752,9 +1752,20 @@ these are the subsystems that actually gate "a remote workroom is a real workroo - **Known limits.** The viewer opens a cmd-clicked file at the top: `FileDescriptor` has no line. A path outside the workroom (`/etc/hosts`, a sibling checkout) opens nothing. The ceiling prompt is still raised only for the Mac's agent: a remote host's "Keep awake" works from its badge, but - no prompt card is shown for it. `hostCwd` is asked for again each time a command finishes, one - round trip later, so a click inside that round trip after a `cd` resolves against the previous - directory. + no prompt card is shown for it, and a remote keep that fails shows nothing either (#324). + `hostCwd` is asked for again each time a command finishes, one round trip later, so a click + inside that round trip after a `cd` resolves against the previous directory. `hostCwd` is the + host's resolved path while the workroom root is the recorded one, so where the root goes + through a symlink (`/home` → `/var/home`) relative cmd-clicks open nothing (#325; the + first-party container image is not affected). A remote forward is reachable by every account on + the Mac, as any `127.0.0.1` forward is; accepted for now, with peer-uid scoping tracked in #323. + - **Path handling, from the final review.** A remote path is split into components, never run + through `URL`: Foundation decomposes Unicode filenames (`é`, C3 A9, becomes `e` + U+0301, + 65 CC 81), which a Linux host reads as another name. Containment is judged on the lexically + resolved path, but the host is sent the path with its `..` still in it, so `link/../file.rb` + is resolved through the link on the host. A newer click replaces the one before it, so a slow + answer can't retarget the preview or open a tab in a workroom that has gone. An error reading + one path moves on to the next candidate; only a transport failure stops the probe. ## Phase 0 Results diff --git a/macapp/WorkroomApp/Core/TerminalLinkOpener.swift b/macapp/WorkroomApp/Core/TerminalLinkOpener.swift index d039a5d44..ca0bc40cb 100644 --- a/macapp/WorkroomApp/Core/TerminalLinkOpener.swift +++ b/macapp/WorkroomApp/Core/TerminalLinkOpener.swift @@ -291,22 +291,32 @@ enum TerminalLinkOpener { /// URL rather than a path gives none, through the same `filePath(from:)` a local pane uses. static func remoteCandidates(for word: String, cwd: String?, root: String) -> [PathCandidate] { guard let path = filePath(from: word), root.hasPrefix("/") else { return [] } - // `standardized` keeps `//`, and the host joins a relative path whose first character is `/` - // as an absolute one, so slashes are collapsed first, the root's trailing one with them. - func collapsed(_ path: String) -> String { - path.replacingOccurrences(of: "/+", with: "/", options: .regularExpression) + // Split into components, never through `URL`: Foundation decomposes Unicode filenames + // (`é` → `e` + U+0301), which a Linux host reads as another name. Empty and `.` components + // go, which also absorbs a trailing or doubled slash. + func parts(_ path: String) -> [Substring] { + path.split(separator: "/").filter { $0 != "." } } - let prefix = collapsed(root + "/") - let base = cwd.flatMap { $0.hasPrefix("/") ? $0 : nil } ?? prefix + let rootParts = parts(root) + let baseParts = cwd.flatMap { $0.hasPrefix("/") ? parts($0) : nil } ?? rootParts var seen = Set() return pathCandidates(from: path).compactMap { candidate in guard !candidate.path.hasPrefix("~") else { return nil } - let joined = collapsed( - candidate.path.hasPrefix("/") ? candidate.path : base + "/" + candidate.path) - // Lexical only: `isDirectory: false` keeps `URL` from probing this Mac's disk for the path. - let absolute = URL(fileURLWithPath: joined, isDirectory: false).standardized.path - guard absolute.hasPrefix(prefix) else { return nil } - let relative = String(absolute.dropFirst(prefix.count)) + let joined = + candidate.path.hasPrefix("/") ? parts(candidate.path) : baseParts + parts(candidate.path) + // Containment is judged on the lexically resolved path. The host is sent the path with its + // `..` still in it, so it resolves them itself, through any symlink on the way (resolving + // `link/..` here names a different file), and its file service checks containment on the + // file it opens. + var resolved: [Substring] = [] + for part in joined { + if part == ".." { _ = resolved.popLast() } else { resolved.append(part) } + } + guard resolved.count > rootParts.count, resolved.starts(with: rootParts) else { return nil } + let sent = + joined.starts(with: rootParts) + ? joined.dropFirst(rootParts.count) : resolved.dropFirst(rootParts.count) + let relative = sent.joined(separator: "/") guard !relative.isEmpty, seen.insert(relative).inserted else { return nil } return PathCandidate(path: relative, line: candidate.line, column: candidate.column) } diff --git a/macapp/WorkroomApp/Core/TerminalSessions.swift b/macapp/WorkroomApp/Core/TerminalSessions.swift index d05508b38..fda56007d 100644 --- a/macapp/WorkroomApp/Core/TerminalSessions.swift +++ b/macapp/WorkroomApp/Core/TerminalSessions.swift @@ -2222,16 +2222,27 @@ final class TerminalSessions: ObservableObject { return state.hostCwd } + /// Each workroom's click in flight. The host can take seconds to answer (a reconnect, then a + /// probe per candidate), so a newer click replaces the one before it: a slow answer must not + /// retarget the preview after a faster one, or open a tab in a workroom that has gone. + private var remoteOpens: [TerminalTarget.ID: Task] = [:] + func openRemoteFile(_ word: String, tab tabID: TerminalTab.ID, target: TerminalTarget) { let cwd = hostCwd(of: tabID, in: target.id) - Task { [weak self] in + remoteOpens[target.id]?.cancel() + remoteOpens[target.id] = Task { [weak self] in guard let self else { return } - guard let file = await self.remoteFile(word, cwd: cwd, target: target) else { + let file = await self.remoteFile(word, cwd: cwd, target: target) + guard !Task.isCancelled else { return } + self.remoteOpens[target.id] = nil + guard let file else { // As `TerminalLinkOpener.handleOpenURL` does locally: a dropped click is otherwise // indistinguishable from a broken app. NSLog("Workroom: no file on the host for terminal link %@", word) return } + // The workroom's panes may all have closed while the host answered. + guard self.tabsByTarget[target.id]?.isEmpty == false else { return } self.openFilePreview(FileDescriptor(path: file.path, isPreview: true), for: target) } } @@ -2251,11 +2262,13 @@ final class TerminalSessions: ObservableObject { return candidate } catch FileServiceError.tooLarge { return candidate - } catch FileServiceError.notFound, FileServiceError.refused { + } catch is FileServiceError { + // About this path: missing, refused, or an error reading it (too long, not a directory + // on the way). The next candidate, `file.rb` from `file.rb:12`, may still be there. An + // agent turning reads away as busy lands here too, and costs only the remaining probes. continue } catch { - // The host, not the candidate: the next one would fail the same way. That includes an - // agent turning the read away as busy (`file.rs`, `MAX_CONCURRENT_READS`). + // The host or its connection, not the candidate: the next one would fail the same way. NSLog("Workroom: probing the host for %@: %@", candidate.path, "\(error)") return nil } diff --git a/macapp/WorkroomAppTests/TerminalLinkOpenerTests.swift b/macapp/WorkroomAppTests/TerminalLinkOpenerTests.swift index c4be67f34..964660cfe 100644 --- a/macapp/WorkroomAppTests/TerminalLinkOpenerTests.swift +++ b/macapp/WorkroomAppTests/TerminalLinkOpenerTests.swift @@ -553,9 +553,32 @@ final class TerminalLinkOpenerTests: XCTestCase { XCTAssertEqual( TerminalLinkOpener.remoteCandidates(for: "\(root)/a/b.txt", cwd: "/", root: root).map( \.path), ["a/b.txt"]) + // `..` stays in what the host is sent: it resolves it through any symlink on the way. XCTAssertEqual( TerminalLinkOpener.remoteCandidates(for: "../x.txt", cwd: "\(root)/lib", root: root).map( - \.path), ["x.txt"]) + \.path), ["lib/../x.txt"]) + } + + /// A remote path reaches the host with the bytes it was written in. Foundation's URL + /// normalisation decomposes `é` (C3 A9) into `e` + U+0301 (65 CC 81), which a Linux host reads as + /// another filename; Swift's `==` calls the two equal, so the bytes are compared. + func testARemoteLinkKeepsItsFilenameBytes() throws { + let name = "caf\u{e9}.txt" + let found = try XCTUnwrap( + TerminalLinkOpener.remoteCandidates(for: name, cwd: nil, root: "/home/workroom/repo").first) + XCTAssertEqual(Array(found.path.utf8), Array(name.utf8)) + } + + /// `link/../file.rb` names the file beside the link's TARGET, which only the host can resolve, + /// so the `..` goes to the host; resolving it here opened `file.rb` at the root. Containment is + /// still judged here, on the lexical path, and again by the host on the file it opens. + func testARemoteLinkLeavesParentTraversalsToTheHost() { + let root = "/home/workroom/repo" + XCTAssertEqual( + TerminalLinkOpener.remoteCandidates(for: "link/../file.rb", cwd: root, root: root).map( + \.path), ["link/../file.rb"]) + XCTAssertEqual( + TerminalLinkOpener.remoteCandidates(for: "link/../../../out.rb", cwd: root, root: root), []) } /// Only what the host's file service can read: nothing outside the workroom's root, no `~` (this @@ -652,7 +675,7 @@ final class TerminalLinkOpenerTests: XCTestCase { remoteHost: UUID()) let sessions = TerminalSessions() let files: [String: Data] = ["a.rb": Data("x\n".utf8)] - func provider(failing: [String: FileServiceError]) -> (RepositoryLocation) async throws + func provider(failing: [String: Error]) -> (RepositoryLocation) async throws -> FileProviding { { location in @@ -663,11 +686,20 @@ final class TerminalLinkOpenerTests: XCTestCase { } // `a.rb:3` probes the literal first, then `a.rb` carrying line 3. - sessions.remoteFiles = provider(failing: ["a.rb:3": .refused("not a regular file")]) + sessions.remoteFiles = provider(failing: [ + "a.rb:3": FileServiceError.refused("not a regular file") + ]) let skipped = await sessions.remoteFile("a.rb:3", cwd: nil, target: target) XCTAssertEqual(skipped, .init(path: "a.rb", line: 3, column: nil)) - sessions.remoteFiles = provider(failing: ["a.rb:3": .failed("agent dropped")]) + // An error reading one path (too long, unreadable) moves on to the next candidate. + sessions.remoteFiles = provider(failing: [ + "a.rb:3": FileServiceError.failed("File name too long (os error 63)") + ]) + let pathError = await sessions.remoteFile("a.rb:3", cwd: nil, target: target) + XCTAssertEqual(pathError, .init(path: "a.rb", line: 3, column: nil)) + + sessions.remoteFiles = provider(failing: ["a.rb:3": HostConnectionError.connectionLost]) let stopped = await sessions.remoteFile("a.rb:3", cwd: nil, target: target) XCTAssertNil(stopped, "a host failure went on to probe a later candidate") @@ -768,6 +800,57 @@ final class TerminalLinkOpenerTests: XCTestCase { var count: Int { lock.withLock { value } } } + /// A newer remote ⌘-click replaces the one before it (#254): the host can take seconds, and a + /// slow answer must not retarget the preview after a faster one. Nor may an answer open a tab in + /// a workroom whose panes have all closed meanwhile. + @MainActor + func testARemoteClickIsReplacedByTheNextAndDroppedForAClosedWorkroom() async throws { + let target = TerminalTarget( + id: "remote", title: "remote", path: "/home/workroom/repo", unavailability: .remote, + remoteHost: UUID()) + let sessions = TerminalSessions() + sessions.makeView = { _, cwd, _ in GhosttySurfaceView(workingDirectory: cwd) } + sessions.recordUnrecognizedTool = { _ in } + sessions.remoteFiles = { location in + SlowHostFiles( + inner: HostFiles( + context: FileContext(location: location, sharedLocation: nil), + files: ["slow.rb": Data("x\n".utf8), "fast.rb": Data("x\n".utf8)])) + } + let tab = sessions.addTab(for: target) + let view = try XCTUnwrap(sessions.view(forTab: tab.id, inTarget: target.id)) + func opened() -> String? { + for tab in sessions.tabs(for: target) { + if case .file(let file) = tab.content { return file.path } + } + return nil + } + view.onCmdClickFile?("slow.rb") + view.onCmdClickFile?("fast.rb") + try await Task.sleep(for: .milliseconds(800)) + XCTAssertEqual(opened(), "fast.rb", "a slower, older click retargeted the preview") + + // Every pane closes while the host is still answering: nothing opens. + view.onCmdClickFile?("slow.rb") + for tab in sessions.tabs(for: target) { sessions.closeTab(tab.id, for: target) } + try await Task.sleep(for: .milliseconds(800)) + XCTAssertTrue(sessions.tabs(for: target).isEmpty, "an answer reopened a closed workroom") + } + + /// `HostFiles` whose `slow.rb` takes half a second to answer. + private struct SlowHostFiles: FileProviding { + let inner: HostFiles + var context: FileContext { inner.context } + func list() async throws -> CommandResult { try await inner.list() } + func read(path: String, symlinks: FileSymlinkPolicy, maxBytes: Int) async throws -> Data { + if path == "slow.rb" { try await Task.sleep(for: .milliseconds(500)) } + return try await inner.read(path: path, symlinks: symlinks, maxBytes: maxBytes) + } + func watch(root: String, onEvent: @escaping @Sendable (FileWatchEvent) -> Void) async throws + -> FileWatchHandle? + { nil } + } + /// A root recorded with a trailing slash, or a link with a doubled one, still resolves inside /// the workroom: `standardized` keeps `//`, and the host reads `/src/a.rb` as an absolute path. func testARemoteLinkSurvivesDoubledAndTrailingSlashes() { @@ -790,7 +873,7 @@ struct HostFiles: FileProviding { let context: FileContext let files: [String: Data] /// Paths whose read fails with this error, before `files` is consulted. - var failing: [String: FileServiceError] = [:] + var failing: [String: Error] = [:] func list() async throws -> CommandResult { CommandResult(stdout: "", stderr: "", exitCode: 0, timedOut: false) From a9d4f605e22219058ac6e30be360485536290ed1 Mon Sep 17 00:00:00 2001 From: Joel Moss Date: Sun, 4 Oct 2026 11:15:55 +0100 Subject: [PATCH 13/14] fix(macapp): rewrap doc comments to the 100-column limit (#254) CodeRabbit flagged one; five more lines this branch added ran to 101-102 columns. swift-format's lint does not wrap comments, so make app-lint passed them. Co-Authored-By: Claude Opus 5.5 --- macapp/WorkroomApp/Core/AgentPortForwarding.swift | 5 +++-- macapp/WorkroomApp/Core/AgentWakefulness.swift | 10 +++++----- macapp/WorkroomApp/Core/TerminalLinkOpener.swift | 11 ++++++----- macapp/WorkroomAppTests/AgentWakefulnessTests.swift | 3 ++- macapp/WorkroomAppTests/TerminalLinkOpenerTests.swift | 4 ++-- 5 files changed, 18 insertions(+), 15 deletions(-) diff --git a/macapp/WorkroomApp/Core/AgentPortForwarding.swift b/macapp/WorkroomApp/Core/AgentPortForwarding.swift index a14e2bdc6..eb53df26c 100644 --- a/macapp/WorkroomApp/Core/AgentPortForwarding.swift +++ b/macapp/WorkroomApp/Core/AgentPortForwarding.swift @@ -933,8 +933,9 @@ final class PortForwardingModel: ObservableObject { /// `host`'s connection in `manager`. A remote host's connection is made by selecting its /// workroom (`RemoteHosts.ensureConnected`, through the inspector's reads), so a forward asks - /// for the one there is and never makes one: the local reasoning in `LocalAgentVCS.forwarding()` - /// holds for a host too, and connecting one could start a container the user stopped. + /// for the one there is and never makes one: the local reasoning in + /// `LocalAgentVCS.forwarding()` holds for a host too, and connecting one could start a + /// container the user stopped. static func on(_ host: HostID, manager: HostConnectionManager) -> Transport { Transport( forwarding: { try await manager.forwarding(host: host) }, diff --git a/macapp/WorkroomApp/Core/AgentWakefulness.swift b/macapp/WorkroomApp/Core/AgentWakefulness.swift index 6e03750cb..49f40586f 100644 --- a/macapp/WorkroomApp/Core/AgentWakefulness.swift +++ b/macapp/WorkroomApp/Core/AgentWakefulness.swift @@ -444,8 +444,8 @@ final class WakefulnessModel: ObservableObject { self.transport = transport } - /// Each remote host's model, made on first use and kept for the launch, as `PortForwardingModel`'s - /// are. This Mac's is `shared`, whose transport may start an agent. + /// Each remote host's model, made on first use and kept for the launch, as + /// `PortForwardingModel`'s are. This Mac's is `shared`, whose transport may start an agent. private static var hosts: [UUID: WakefulnessModel] = [:] static func model(forHost id: UUID) -> WakefulnessModel { @@ -462,9 +462,9 @@ final class WakefulnessModel: ObservableObject { /// the quit waits for the slowest, not their sum. A remote host's badge is its only keep control. /// /// A remote keep may have to reconnect first, over ssh (`ConnectTimeout 10`), so it gets - /// `remoteKeepDrain`, longer than this Mac's. Longer still is possible (a relayed workroom's relay - /// install runs on that connect), and such a keep can miss the quit; the bound is what a quit is - /// worth. + /// `remoteKeepDrain`, longer than this Mac's. Longer still is possible (a relayed workroom's + /// relay install runs on that connect), and such a keep can miss the quit; the bound is what a + /// quit is worth. static func drainAllKeeps() async { await withTaskGroup(of: Void.self) { group in group.addTask { await shared.drainKeep() } diff --git a/macapp/WorkroomApp/Core/TerminalLinkOpener.swift b/macapp/WorkroomApp/Core/TerminalLinkOpener.swift index ca0bc40cb..afd6bf17f 100644 --- a/macapp/WorkroomApp/Core/TerminalLinkOpener.swift +++ b/macapp/WorkroomApp/Core/TerminalLinkOpener.swift @@ -283,8 +283,8 @@ enum TerminalLinkOpener { /// The workroom-relative files a remote pane's ⌘-clicked `word` could name, in the order to probe /// them on the host. `cwd` and `root` are paths on the HOST: the pane's working directory as its - /// agent reports it, and the workroom's root. Nothing here touches this Mac's filesystem, which is - /// the point (C7): on this Mac the same path is another file, or none. + /// agent reports it, and the workroom's root. Nothing here touches this Mac's filesystem, which + /// is the point (C7): on this Mac the same path is another file, or none. /// /// A candidate outside `root` is dropped, because the host's file service reads only under the /// workroom's root. So is a `~` path: `~` is this Mac's home, not the host's. A link that names a @@ -324,9 +324,10 @@ enum TerminalLinkOpener { /// Whether a remote pane's ⌘-hover cursor and ⌘-click gate pass `word` (#254), decided without /// the host: it names a candidate in the workroom (`remoteCandidates`) and looks like a path, - /// with a `/`, a `name.ext` (a dotfile such as `.env` counts) or a `:line`. The click then asks the host. The costs: a path-shaped - /// word with no file behind it shows the hand and its click opens nothing, and a bare name with - /// none of the three (`Gemfile`) isn't clickable in a remote pane. + /// with a `/`, a `name.ext` (a dotfile such as `.env` counts) or a `:line`. The click then asks + /// the host. The costs: a path-shaped word with no file behind it shows the hand and its click + /// opens nothing, and a bare name with none of the three (`Gemfile`) isn't clickable in a remote + /// pane. static func looksLikeRemotePath(_ word: String, cwd: String?, root: String) -> Bool { guard let path = filePath(from: word), !remoteCandidates(for: word, cwd: cwd, root: root).isEmpty diff --git a/macapp/WorkroomAppTests/AgentWakefulnessTests.swift b/macapp/WorkroomAppTests/AgentWakefulnessTests.swift index 72fec68ca..6728e24e6 100644 --- a/macapp/WorkroomAppTests/AgentWakefulnessTests.swift +++ b/macapp/WorkroomAppTests/AgentWakefulnessTests.swift @@ -187,7 +187,8 @@ final class AgentWakefulnessTests: XCTestCase { } /// A remote host's poll reads that host's connection and never connects one: with only this Mac - /// connected, it is refused as the host's, so the row shows nothing rather than this Mac's verdict. + /// connected, it is refused as the host's, so the row shows nothing rather than this Mac's + /// verdict. @MainActor func testARemoteHostsPollAsksForThatHostsConnection() async throws { let manager = HostConnectionManager() diff --git a/macapp/WorkroomAppTests/TerminalLinkOpenerTests.swift b/macapp/WorkroomAppTests/TerminalLinkOpenerTests.swift index 964660cfe..066bcc1be 100644 --- a/macapp/WorkroomAppTests/TerminalLinkOpenerTests.swift +++ b/macapp/WorkroomAppTests/TerminalLinkOpenerTests.swift @@ -867,8 +867,8 @@ final class TerminalLinkOpenerTests: XCTestCase { } -/// A remote host's file service for the remote-pane tests (#254): `files` by workroom-relative path, -/// reads capped as the agent caps them. Shared with `RemotePaneFooterTests`. +/// A remote host's file service for the remote-pane tests (#254): `files` by workroom-relative +/// path, reads capped as the agent caps them. Shared with `RemotePaneFooterTests`. struct HostFiles: FileProviding { let context: FileContext let files: [String: Data] From 11631c6a476222c72c5aefed5e5e2ef9bf691d7b Mon Sep 17 00:00:00 2001 From: Joel Moss Date: Sun, 4 Oct 2026 11:22:40 +0100 Subject: [PATCH 14/14] fix(macapp): resolve `..` before asking the host; drop a closed pane's click (#254) From Greptile's review of #326: - 6ce0a1c9 sent `..` to the host so it could resolve symlinks, but the agent refuses any path with a `.` or `..` component before opening anything (`vcs::relative`). Every `../file` click in a remote pane opened nothing. `..` is resolved on the Mac again, lexically, keeping the byte-preserving component split. The symlink-before-`..` case is a documented limit. A fixture test now sends a parent traversal to the real agent. - A remote click's answer was dropped only when the whole workroom had no panes. It is now dropped when the pane that asked has closed, even with other panes open. Tab IDs are never reused, so a reopened workroom can't revive it. Co-Authored-By: Claude Opus 5.5 --- docs/designs/remote-workrooms.md | 12 +++--- .../WorkroomApp/Core/TerminalLinkOpener.swift | 12 ++---- .../WorkroomApp/Core/TerminalSessions.swift | 5 ++- .../RemoteHostIntegrationTests.swift | 3 ++ .../TerminalLinkOpenerTests.swift | 40 ++++++++++++------- 5 files changed, 42 insertions(+), 30 deletions(-) diff --git a/docs/designs/remote-workrooms.md b/docs/designs/remote-workrooms.md index 1ec8bdae2..8deb1a955 100644 --- a/docs/designs/remote-workrooms.md +++ b/docs/designs/remote-workrooms.md @@ -1761,11 +1761,13 @@ these are the subsystems that actually gate "a remote workroom is a real workroo the Mac, as any `127.0.0.1` forward is; accepted for now, with peer-uid scoping tracked in #323. - **Path handling, from the final review.** A remote path is split into components, never run through `URL`: Foundation decomposes Unicode filenames (`é`, C3 A9, becomes `e` + U+0301, - 65 CC 81), which a Linux host reads as another name. Containment is judged on the lexically - resolved path, but the host is sent the path with its `..` still in it, so `link/../file.rb` - is resolved through the link on the host. A newer click replaces the one before it, so a slow - answer can't retarget the preview or open a tab in a workroom that has gone. An error reading - one path moves on to the next candidate; only a transport failure stops the probe. + 65 CC 81), which a Linux host reads as another name. `..` is resolved here, lexically, because + the agent refuses any path with a `.` or `..` component before it opens anything + (`vcs::relative`); sending `..` through, as one review suggested, made every `../file` click + open nothing (caught on the PR). The cost: `link/../file.rb` names the file beside the link, + not beside its target. A newer click replaces the one before it, and an answer whose pane has + closed opens nothing. An error reading one path moves on to the next candidate; only a + transport failure stops the probe. ## Phase 0 Results diff --git a/macapp/WorkroomApp/Core/TerminalLinkOpener.swift b/macapp/WorkroomApp/Core/TerminalLinkOpener.swift index afd6bf17f..0940caa89 100644 --- a/macapp/WorkroomApp/Core/TerminalLinkOpener.swift +++ b/macapp/WorkroomApp/Core/TerminalLinkOpener.swift @@ -304,19 +304,15 @@ enum TerminalLinkOpener { guard !candidate.path.hasPrefix("~") else { return nil } let joined = candidate.path.hasPrefix("/") ? parts(candidate.path) : baseParts + parts(candidate.path) - // Containment is judged on the lexically resolved path. The host is sent the path with its - // `..` still in it, so it resolves them itself, through any symlink on the way (resolving - // `link/..` here names a different file), and its file service checks containment on the - // file it opens. + // `..` is resolved here, lexically: the agent refuses any path with a `.` or `..` + // component before it opens anything (`vcs::relative`). The cost is a symlink before the + // `..`: `link/../file.rb` names the file beside the link, not beside its target. var resolved: [Substring] = [] for part in joined { if part == ".." { _ = resolved.popLast() } else { resolved.append(part) } } guard resolved.count > rootParts.count, resolved.starts(with: rootParts) else { return nil } - let sent = - joined.starts(with: rootParts) - ? joined.dropFirst(rootParts.count) : resolved.dropFirst(rootParts.count) - let relative = sent.joined(separator: "/") + let relative = resolved.dropFirst(rootParts.count).joined(separator: "/") guard !relative.isEmpty, seen.insert(relative).inserted else { return nil } return PathCandidate(path: relative, line: candidate.line, column: candidate.column) } diff --git a/macapp/WorkroomApp/Core/TerminalSessions.swift b/macapp/WorkroomApp/Core/TerminalSessions.swift index fda56007d..03e3276d4 100644 --- a/macapp/WorkroomApp/Core/TerminalSessions.swift +++ b/macapp/WorkroomApp/Core/TerminalSessions.swift @@ -2241,8 +2241,9 @@ final class TerminalSessions: ObservableObject { NSLog("Workroom: no file on the host for terminal link %@", word) return } - // The workroom's panes may all have closed while the host answered. - guard self.tabsByTarget[target.id]?.isEmpty == false else { return } + // The pane that asked may have closed while the host answered. Tab IDs are never reused, + // so a reopened workroom doesn't revive it. + guard self.tabsByTarget[target.id]?[tabID] != nil else { return } self.openFilePreview(FileDescriptor(path: file.path, isPreview: true), for: target) } } diff --git a/macapp/WorkroomAppTests/RemoteHostIntegrationTests.swift b/macapp/WorkroomAppTests/RemoteHostIntegrationTests.swift index 04ba09e65..02a019e8b 100644 --- a/macapp/WorkroomAppTests/RemoteHostIntegrationTests.swift +++ b/macapp/WorkroomAppTests/RemoteHostIntegrationTests.swift @@ -212,6 +212,9 @@ final class RemoteHostIntegrationTests: XCTestCase { XCTAssertEqual(found, .init(path: "lib/only-here.rb", line: 1, column: nil)) let missing = await sessions.remoteFile("nowhere.rb", cwd: "\(path)/lib", target: target) XCTAssertNil(missing) + // A parent traversal reaches the real agent as a path it accepts: it refuses any `..`. + let parent = await sessions.remoteFile("../file", cwd: "\(path)/lib", target: target) + XCTAssertEqual(parent, .init(path: "file", line: nil, column: nil)) } /// A failed remote write is classified from the HOST's disk (#229): a leftover `index.lock` there diff --git a/macapp/WorkroomAppTests/TerminalLinkOpenerTests.swift b/macapp/WorkroomAppTests/TerminalLinkOpenerTests.swift index 066bcc1be..56ee26400 100644 --- a/macapp/WorkroomAppTests/TerminalLinkOpenerTests.swift +++ b/macapp/WorkroomAppTests/TerminalLinkOpenerTests.swift @@ -553,10 +553,9 @@ final class TerminalLinkOpenerTests: XCTestCase { XCTAssertEqual( TerminalLinkOpener.remoteCandidates(for: "\(root)/a/b.txt", cwd: "/", root: root).map( \.path), ["a/b.txt"]) - // `..` stays in what the host is sent: it resolves it through any symlink on the way. XCTAssertEqual( TerminalLinkOpener.remoteCandidates(for: "../x.txt", cwd: "\(root)/lib", root: root).map( - \.path), ["lib/../x.txt"]) + \.path), ["x.txt"]) } /// A remote path reaches the host with the bytes it was written in. Foundation's URL @@ -569,14 +568,22 @@ final class TerminalLinkOpenerTests: XCTestCase { XCTAssertEqual(Array(found.path.utf8), Array(name.utf8)) } - /// `link/../file.rb` names the file beside the link's TARGET, which only the host can resolve, - /// so the `..` goes to the host; resolving it here opened `file.rb` at the root. Containment is - /// still judged here, on the lexical path, and again by the host on the file it opens. - func testARemoteLinkLeavesParentTraversalsToTheHost() { + /// What the host is sent never has a `.` or `..` component: the agent refuses such a path + /// before it opens anything (`vcs::relative`), so a parent traversal is resolved here. Sent as + /// written, every `../file.rb` click opened nothing (found in review). + func testARemoteLinkNeverSendsTheHostAParentComponent() { let root = "/home/workroom/repo" - XCTAssertEqual( - TerminalLinkOpener.remoteCandidates(for: "link/../file.rb", cwd: root, root: root).map( - \.path), ["link/../file.rb"]) + for (link, cwd) in [ + ("../x.txt", "\(root)/lib"), ("link/../file.rb", root), ("./a/./b.rb", root), + ("../lib/../y.rb:3", "\(root)/lib/deep"), + ] { + let candidates = TerminalLinkOpener.remoteCandidates(for: link, cwd: cwd, root: root) + XCTAssertFalse(candidates.isEmpty, link) + for candidate in candidates { + let parts = candidate.path.split(separator: "/", omittingEmptySubsequences: false) + XCTAssertFalse(parts.contains { $0 == ".." || $0 == "." || $0.isEmpty }, candidate.path) + } + } XCTAssertEqual( TerminalLinkOpener.remoteCandidates(for: "link/../../../out.rb", cwd: root, root: root), []) } @@ -801,10 +808,10 @@ final class TerminalLinkOpenerTests: XCTestCase { } /// A newer remote ⌘-click replaces the one before it (#254): the host can take seconds, and a - /// slow answer must not retarget the preview after a faster one. Nor may an answer open a tab in - /// a workroom whose panes have all closed meanwhile. + /// slow answer must not retarget the preview after a faster one. Nor may an answer open anything + /// once the pane that asked has closed, even with other panes still open in the workroom. @MainActor - func testARemoteClickIsReplacedByTheNextAndDroppedForAClosedWorkroom() async throws { + func testARemoteClickIsReplacedByTheNextAndDroppedForAClosedPane() async throws { let target = TerminalTarget( id: "remote", title: "remote", path: "/home/workroom/repo", unavailability: .remote, remoteHost: UUID()) @@ -830,11 +837,14 @@ final class TerminalLinkOpenerTests: XCTestCase { try await Task.sleep(for: .milliseconds(800)) XCTAssertEqual(opened(), "fast.rb", "a slower, older click retargeted the preview") - // Every pane closes while the host is still answering: nothing opens. + // The pane that asked closes while the host is still answering, with another pane still + // open in the workroom: nothing opens there. + let other = sessions.addTab(for: target) view.onCmdClickFile?("slow.rb") - for tab in sessions.tabs(for: target) { sessions.closeTab(tab.id, for: target) } + sessions.closeTab(tab.id, for: target) try await Task.sleep(for: .milliseconds(800)) - XCTAssertTrue(sessions.tabs(for: target).isEmpty, "an answer reopened a closed workroom") + XCTAssertNotEqual(opened(), "slow.rb", "a closed pane's click opened a preview") + XCTAssertNotNil(sessions.tab(other.id, for: target)) } /// `HostFiles` whose `slow.rb` takes half a second to answer.