fix: remote cmd-click resolves .. on the host, through the symlink (#327)

TODOS.md

@@ -5,6 +5,55 @@
 > **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
+
+### A test-host process was killed for closing a guarded file descriptor (macapp) — #327 ship follow-up
+
+**What:** One full `make app-test` run lost a test worker to `EXC_GUARD` (`GUARD_TYPE_FD`, "CLOSE
+on file descriptor 25 (guarded with 0x08fd4dbfade2dead)"). The kill landed in
+`VCSProviderConformanceTests.testCommitRenameIsOneRow`, where `-[NSConcreteFileHandle dealloc]`
+closed the `Pipe` of its `sh` helper (`VCSProviderConformanceTests.swift:424`, called from
+`requireTool` at :339). Seen on `workroom/cyan-brush` (2026-10-04, crash report
+`~/Library/Logs/DiagnosticReports/Workroom Dev-2026-10-04-121020.ips`, pid 69069). The #327 diff
+opens and closes no descriptors, and an immediate re-run of the full suite passed.
+
+**Why:** The file handle closed a number it believed it owned, and the kernel found a guarded
+descriptor there. The suspected cause, not yet confirmed by the worker's trace: something earlier
+in that worker closed descriptor 25 without owning it (a double close), and a guarded descriptor
+reused the number. If so, and the double close is in app code rather than a test, the shipping
+app can intermittently close the wrong file.
+
+**How to start:** List what else ran in the same worker before the crash (the `xcresult`, or the
+log lines tagged with that worker's pid) and grep those tests and their code for raw `close(` and
+`FileHandle(fileDescriptor:closeOnDealloc: true)` on descriptors they also close by hand. Then run
+that worker's sequence alone, repeated.
+
+**Priority:** P0 (chosen at ship time on #327)
+
+### `ReverseForwardTests.testAReconnectWhileTheOldListenerHoldsThePortReopensOnceItLetsGo` is flaky (macapp) — #327 ship follow-up
+
+**What:** The test fails about 1 run in 5. Measured 2026-10-04 with `make app-test
+APP_TEST_FLAGS="-only-testing:WorkroomAppTests/ReverseForwardTests -test-iterations 10"`: 3 of 15
+runs failed on `workroom/cyan-brush`, and 2 of 20 on a clean `origin/master` worktree, so it is not
+the #327 branch. It fails at three places:
+- line 301, `XCTAssertEqual failed: ("0 bytes") is not equal to ("5 bytes")` in `roundTrip`;
+- line 285, "the port was never contended";
+- line 290, "never reopened".
+
+**Why:** It tests that the broker's reverse forward (`BrokerReverseForwards`) reopens after a
+reconnect while the old connection's listener still holds the port. If the race is in the
+registry rather than the test, a real reconnect can leave a workroom's broker forward dead. In a
+Debug build that forward is how the remote agent reaches the development Codaset
+(`BrokerReverseForwards`), so once the token it holds expires (up to an hour) it can't mint a new
+git credential. Release and Nightly agents reach codaset.dev directly and don't use it.
+
+**How to start:** Pin which step loses the race. The test leans on a fixed 3 s sleep (longer than
+the bind retries) and a 5 s reopen deadline. A `roundTrip` that reads 0 bytes means the port
+reopened but the forward didn't carry data, which points past the listener at the forward's
+target or at a second listener answering the port.
+
+**Priority:** P0 (chosen at ship time on #327)
+
 ## P1 — before GA
 
 ## P2 — perf, correctness, and the next VCS phase

docs/designs/remote-workrooms.md

@@ -1761,13 +1761,24 @@ 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. `..` 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.
+    65 CC 81), which a Linux host reads as another name. A `read` is never sent a `.` or `..`
+    component: the agent refuses one before it opens anything (`vcs::relative`), and sending `..`
+    to a read, as one review suggested, made every `../file` click open nothing (caught on the PR).
+    A `..` goes to the File service's `resolve` instead (#327), which the agent answers with the
+    path resolved on the host, through any symlink before the last `..`, so `link/../file.rb` is
+    the file beside the link's target; what follows the last `..` is kept as written, so a link
+    there keeps its own name, as a plain click would. `resolve` refuses a path that climbs above
+    the root as written before any lookup; the Mac resolves such a path lexically. So it does any
+    `..` path for an agent that predates `resolve`, recognised by its exact reply to an unknown
+    method; any other failure (busy, unreadable, a loop) opens nothing rather than the lexical
+    path, which is the wrong file whenever the host's answer would differ. Links before the last
+    `..` come back as their target. A `..` after a link out of the workroom can still tell
+    whether the directory it names exists, which the VCS service's `stat` already does for any
+    path; what follows the `..` is never looked up.
+    `resolve` shares `read`'s cap of four in flight, since a link can lead into a slow mount. 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
 

macapp/WorkroomApp/Core/AgentFileProvider.swift

@@ -4,6 +4,11 @@ import Foundation
 /// `vcs/crates/wr-agent/src/file.rs` and `watch.rs`; the two are kept in step by
 /// `AgentFileIntegrationTests`, which drives the real agent binary.
 struct AgentFileProvider: FileProviding {
+  /// What every agent with a File service answers a method it doesn't know with, as `Unsupported`
+  /// (`file.rs`, unchanged since the service was added). It is how the app tells an agent that
+  /// predates `resolve` (#327) from one that answered.
+  static let unknownMethod = "unsupported file method"
+
   let context: FileContext
   let connection: AgentVCSConnection
 
@@ -29,6 +34,11 @@ struct AgentFileProvider: FileProviding {
     return data
   }
 
+  func resolve(path: String) async throws -> String {
+    let request = AgentFileRequest(method: "resolve", root: context.location.path, path: path)
+    return try AgentFileReply<AgentFileResolved>.decode(await connection.fileRequest(request)).path
+  }
+
   func watch(root: String, onEvent: @escaping @Sendable (FileWatchEvent) -> Void) async throws
     -> FileWatchHandle?
   { try await connection.watch(root: root, onEvent: onEvent) }
@@ -62,6 +72,10 @@ private struct AgentFileListing: Decodable {
   let signaled: Bool
 }
 
+private struct AgentFileResolved: Decodable {
+  let path: String
+}
+
 private struct AgentFileContent: Decodable {
   let size: Int
   let content: String

macapp/WorkroomApp/Core/FileServices.swift

@@ -71,6 +71,10 @@ protocol FileProviding: Sendable {
   /// `maxBytes` is a ceiling the READ enforces (`.tooLarge`), not a hint.
   func read(path: String, symlinks: FileSymlinkPolicy, maxBytes: Int) async throws -> Data
 
+  /// The repository-relative path `path` names once resolved on the host, through any symlink on
+  /// the way (#327). `path` may hold `..`, but never one that climbs above the root as written.
+  func resolve(path: String) async throws -> String
+
   /// Subscribe to changes under `root`. `nil` means this provider cannot watch — the native one —
   /// and the caller uses local FSEvents instead.
   func watch(root: String, onEvent: @escaping @Sendable (FileWatchEvent) -> Void) async throws
@@ -98,6 +102,14 @@ struct NativeFileProvider: FileProviding {
     }
   }
 
+  /// Only a remote pane resolves on the host; a local pane's ⌘-click has this Mac's filesystem.
+  ///
+  /// ponytail: unimplemented natively, since nothing local calls it. Port the agent's
+  /// `resolve_path` here if a local caller ever needs it.
+  func resolve(path: String) async throws -> String {
+    throw FileServiceError.failed("Resolving a path is only done on a remote host.")
+  }
+
   func watch(root: String, onEvent: @escaping @Sendable (FileWatchEvent) -> Void) async throws
     -> FileWatchHandle?
   { nil }
@@ -215,6 +227,14 @@ struct LocalFallbackFileProvider: FileProviding {
     }
   }
 
+  func resolve(path: String) async throws -> String {
+    do {
+      return try await primary.resolve(path: path)
+    } catch is HostConnectionError {
+      return try await fallback.resolve(path: path)
+    }
+  }
+
   func watch(root: String, onEvent: @escaping @Sendable (FileWatchEvent) -> Void) async throws
     -> FileWatchHandle?
   { try await primary.watch(root: root, onEvent: onEvent) }
@@ -235,6 +255,7 @@ struct UnavailableFileProvider: FileProviding {
   func read(path: String, symlinks: FileSymlinkPolicy, maxBytes: Int) async throws -> Data {
     throw failure
   }
+  func resolve(path: String) async throws -> String { throw failure }
   func watch(root: String, onEvent: @escaping @Sendable (FileWatchEvent) -> Void) async throws
     -> FileWatchHandle?
   { throw failure }
@@ -257,6 +278,9 @@ struct HostFileProvider: FileProviding {
       try await service.read(path: path, symlinks: symlinks, maxBytes: maxBytes)
     }
   }
+  func resolve(path: String) async throws -> String {
+    try await manager.perform(on: lease) { try await service.resolve(path: path) }
+  }
   func watch(root: String, onEvent: @escaping @Sendable (FileWatchEvent) -> Void) async throws
     -> FileWatchHandle?
   {

macapp/WorkroomApp/Core/TerminalLinkOpener.swift

@@ -304,17 +304,30 @@ enum TerminalLinkOpener {
       guard !candidate.path.hasPrefix("~") else { return nil }
       let joined =
         candidate.path.hasPrefix("/") ? parts(candidate.path) : baseParts + parts(candidate.path)
-      // `..` 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.
+      // `..` is resolved here, lexically, for the containment check and for an agent that
+      // predates resolving. `link/../file.rb` names the file beside the link's TARGET, which only
+      // the host knows (#327), so a `..` that stays inside the workroom as written also goes to
+      // the host, as `onHost`. `read` itself never gets a `..`: the agent refuses one there
+      // before it opens anything (`vcs::relative`).
       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 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)
+      var onHost: String?
+      if joined.starts(with: rootParts) {
+        let written = joined.dropFirst(rootParts.count)
+        var depth = 0
+        for part in written {
+          depth += part == ".." ? -1 : 1
+          if depth < 0 { break }
+        }
+        if depth >= 0, written.contains("..") { onHost = written.joined(separator: "/") }
+      }
+      return PathCandidate(
+        path: relative, line: candidate.line, column: candidate.column, onHost: onHost)
     }
   }
 
@@ -384,6 +397,10 @@ enum TerminalLinkOpener {
     let path: String
     let line: Int?
     let column: Int?
+    /// A remote candidate written with a `..` (#327): the workroom-relative path with the `..`
+    /// still in it, for the host to resolve through any symlink before it. `path` is the same
+    /// path resolved here, lexically, for an agent that predates resolving.
+    var onHost: String? = nil
   }
 
   /// Candidate file paths to probe for `path`, in priority order: the literal first, then with the

macapp/WorkroomApp/Core/TerminalSessions.swift

@@ -2250,19 +2250,34 @@ final class TerminalSessions: ObservableObject {
 
   /// 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.
+  /// anything missing, refused or outside the workroom. A candidate written with a `..` is
+  /// resolved on the host first (#327), so `link/../file.rb` is the file beside the link's target.
   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 {
+      var path = candidate.path
       do {
-        _ = try await files.read(path: candidate.path, symlinks: .followWithinRoot, maxBytes: 1)
-        return candidate
-      } catch FileServiceError.tooLarge {
-        return candidate
+        if let onHost = candidate.onHost {
+          do {
+            path = try await files.resolve(path: onHost)
+          } catch FileServiceError.failed(let message)
+            where message == AgentFileProvider.unknownMethod
+          {
+            // An agent from before `resolve`: probe the lexical path, as every agent did before
+            // #327. Any other failure (busy, unreadable, a loop) skips the candidate below, since
+            // the lexical path is the wrong file whenever the host's answer would differ.
+          }
+        }
+        let found = TerminalLinkOpener.PathCandidate(
+          path: path, line: candidate.line, column: candidate.column)
+        do {
+          _ = try await files.read(path: path, symlinks: .followWithinRoot, maxBytes: 1)
+        } catch FileServiceError.tooLarge {}
+        return found
       } 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

macapp/WorkroomAppTests/AgentFileIntegrationTests.swift

@@ -433,6 +433,75 @@ final class AgentFileIntegrationTests: XCTestCase {
     XCTAssertTrue(empty.isEmpty)
   }
 
+  // Value: protects=the app's `resolve` request and the agent's reply agree on the wire, and each
+  // refusal lands as the error the remote cmd-click acts on (#327): a link is followed on the
+  // host; a path out of the root is `.refused`; a missing one is `.notFound`; a path the agent will
+  // not look up is `.failed`, which isn't the unknown-method reply, so the app skips it;
+  // fails_when=the method name, the `path` key, or the agent's error tags drift from what
+  // `AgentFileProvider` decodes; why_new=the real-agent round trip in RemoteHostIntegrationTests
+  // skips without the ssh fixture, and no other test sends `resolve` to the shipped agent;
+  // seam=none
+  func testAResolveFollowsALinkOnTheAgentAndItsRefusalsKeepTheirKind() async throws {
+    let (connection, _) = try await connect()
+    let root = try gitRepo()
+    let fm = FileManager.default
+    try fm.createDirectory(
+      at: root.appendingPathComponent("nested/dir"), withIntermediateDirectories: true)
+    try "x\n".write(
+      to: root.appendingPathComponent("nested/file.rb"), atomically: true, encoding: .utf8)
+    try "decoy\n".write(
+      to: root.appendingPathComponent("file.rb"), atomically: true, encoding: .utf8)
+    try fm.createSymbolicLink(
+      atPath: root.appendingPathComponent("link").path, withDestinationPath: "nested/dir")
+    let outside = root.deletingLastPathComponent().appendingPathComponent(
+      "outside-\(UUID().uuidString)")
+    try fm.createDirectory(
+      at: outside.appendingPathComponent("dir"), withIntermediateDirectories: true)
+    roots.append(outside)
+    try "secret".write(
+      to: outside.appendingPathComponent("secret"), atomically: true, encoding: .utf8)
+    try fm.createSymbolicLink(
+      atPath: root.appendingPathComponent("out").path,
+      withDestinationPath: outside.appendingPathComponent("dir").path)
+    let files = try await agentFiles(root, connection: connection)
+
+    let resolved = try await files.resolve(path: "link/../file.rb")
+    XCTAssertEqual(resolved, "nested/file.rb", "the file beside the link's target, not the decoy")
+
+    func failure(_ path: String) async -> FileServiceError? {
+      do {
+        _ = try await files.resolve(path: path)
+        return nil
+      } catch { return error as? FileServiceError }
+    }
+    guard case .refused = await failure("out/../secret") else {
+      return XCTFail("a link out of the root must be refused")
+    }
+    guard case .notFound = await failure("nope/../file.rb") else {
+      return XCTFail("a missing directory before the `..` must be not found")
+    }
+    guard case .failed = await failure("../file.rb") else {
+      return XCTFail("a path climbing the root as written must not be looked up")
+    }
+  }
+
+  // Value: protects=the app tells an agent that predates `resolve` from one that failed, by the
+  // agent's own reply to a method it doesn't know; fails_when=the agent's wording drifts from
+  // `AgentFileProvider.unknownMethod`, so an old agent's clicks stop falling back; why_new=the
+  // unit tests throw the constant itself, so only the real agent can pin it; seam=none
+  func testTheAgentAnswersAnUnknownFileMethodWithTheTextTheAppMatches() async throws {
+    let (connection, _) = try await connect()
+    let root = try gitRepo()
+    let request = AgentFileRequest(method: "no-such-method", root: root.path)
+    do {
+      _ = try AgentFileReply<AgentFileSubscription>.decode(
+        await connection.fileRequest(request))
+      XCTFail("an unknown method was answered")
+    } catch FileServiceError.failed(let message) {
+      XCTAssertEqual(message, AgentFileProvider.unknownMethod)
+    }
+  }
+
   /// One matrix, run against BOTH providers: a check that exists on one path and not the other is the
   /// bug (the native path used to check a path and then open it, and never guarded against a FIFO).
   func testTheContainmentMatrixHoldsOnTheAgentPath() async throws {

macapp/WorkroomAppTests/LocalFileFallbackTests.swift

@@ -21,6 +21,9 @@ final class LocalFileFallbackTests: XCTestCase {
     func read(path: String, symlinks: FileSymlinkPolicy, maxBytes: Int) async throws -> Data {
       try content.get()
     }
+    func resolve(path: String) async throws -> String {
+      String(decoding: try content.get(), as: UTF8.self)
+    }
     func watch(root: String, onEvent: @escaping @Sendable (FileWatchEvent) -> Void) async throws
       -> FileWatchHandle?
     { nil }
@@ -46,6 +49,8 @@ final class LocalFileFallbackTests: XCTestCase {
       XCTAssertEqual(listing.stdout, "native", "\(failure)")
       let data = try await files.read(path: "a", symlinks: .refuse, maxBytes: 1)
       XCTAssertEqual(data, Data("native".utf8), "\(failure)")
+      let resolved = try await files.resolve(path: "a/../b")
+      XCTAssertEqual(resolved, "native", "\(failure)")
     }
   }
 
@@ -70,6 +75,10 @@ final class LocalFileFallbackTests: XCTestCase {
         _ = try await files.read(path: "a", symlinks: .refuse, maxBytes: 1)
         XCTFail("\(answer) must propagate from read")
       } catch { XCTAssertEqual("\(error)", "\(answer)") }
+      do {
+        _ = try await files.resolve(path: "a/../b")
+        XCTFail("\(answer) must propagate from resolve")
+      } catch { XCTAssertEqual("\(error)", "\(answer)") }
     }
   }