From f9ead94775451039f7fb02f0dbd7060a09c9e4d7 Mon Sep 17 00:00:00 2001 From: Joel Moss Date: Fri, 18 Sep 2026 17:10:39 +0100 Subject: [PATCH 01/13] feat(macapp): route local VCS writes through the agent wr-agent gains a generic, allowlisted exec service on the existing VCS envelope (argv/cwd/stdin/timeout in; exit code/stdout/stderr/timed_out/ signaled out), discriminated from a read Request by a "kind":"exec" field so the read wire format is untouched. CLIVCSWriter's own arg building, output parsing and failure classification stay exactly as they are -- only the StatusCommandRunning conformer it spawns through changes (AgentCommandRunner, forwarding to wr-agent instead of Process), so agent-routed and native writes classify identically by construction. The exec service deliberately never takes the JJ snapshot barrier itself: CLIVCSWriter's JJSnapshotGate already holds /.jj/workroom-vcs.lock for the whole gated write before any exec request goes out, and wr-agent's own SnapshotLock locks the identical file for its read-side snapshots -- acquiring it again on the write path would self-deadlock. Proven both in a Rust unit test (exec against a held SnapshotLock returns immediately) and a Swift integration test driving a real jj commit through the agent. RepositoryRouter.writer(for:) routes local writes through the same agent connection reads already use, falling back to a native CLIVCSWriter only when obtaining the writer itself fails (VCSError.backendVersion, a pre-upgrade agent with no write capability) -- never mid-write, matching the read path's existing fallback discipline exactly. Fixes from an independent code-review pass: - Always forward PATH to the exec child, not just for network commands: wr-agent is a long-lived daemon that is "negotiated with, never replaced," so its own inherited PATH can predate a tool install. - stdin round-trips via Latin-1 (a total bijection over every byte 0x00-0xFF), not UTF-8: a pathspec payload is NUL-separated and paths aren't guaranteed valid UTF-8, so decoding as UTF-8 would have silently corrupted the payload via lossy replacement. - Added integration coverage for the specific case where reads resolve through the agent but the write capability check alone falls back to a native writer. Known, documented limitations (not fixed here): a commit selecting many thousands of long paths could exceed the 1 MiB single-envelope request cap (would need real request chunking to lift); the 32-slot request pool is now shared between reads and a write that can hold a slot for minutes. Co-Authored-By: Claude Sonnet 5 --- docs/designs/remote-workrooms.md | 48 +- .../WorkroomApp/Core/AgentCommandRunner.swift | 119 ++++ .../WorkroomApp/Core/AgentVCSConnection.swift | 46 +- macapp/WorkroomApp/Core/LocalAgentVCS.swift | 11 +- .../WorkroomApp/Core/RepositoryLocation.swift | 22 +- .../AgentVCSIntegrationTests.swift | 189 +++++++ vcs/crates/wr-agent/src/vcs.rs | 533 +++++++++++++++++- 7 files changed, 952 insertions(+), 16 deletions(-) create mode 100644 macapp/WorkroomApp/Core/AgentCommandRunner.swift diff --git a/docs/designs/remote-workrooms.md b/docs/designs/remote-workrooms.md index 226010fbb..3d221ab09 100644 --- a/docs/designs/remote-workrooms.md +++ b/docs/designs/remote-workrooms.md @@ -13,8 +13,15 @@ New persistent local sessions use `wr-agent`; an attach-only Swift shim preserve sessions held by older daemons. This does **not** yet provide remote workrooms. Phase 2 preparation has landed: #184 added the VCS registry, #185 moved working status onto -`VCSProviding`, and #186 routed writers through the registry too. The host-aware foundation binds services to validated repository identities and captured ownership. -Agent-backed VCS, file and notification services remain unbuilt. +`VCSProviding`, and #186 routed writers through the registry too. The host-aware foundation binds services to validated repository identities and captured ownership +(#201, #202). Agent-backed VCS reads are implemented (#204, Next Steps item 2): exact patches and +line counts both come from owning-host Git, not gix, so the spike's 139/150 count drift never +ships. Agent-backed VCS writes are implemented (#205, Next Steps item 3): `wr-agent` gained a +generic, allowlisted exec service, and every write's argv building, output parsing and failure +classification stayed in `CLIVCSWriter` unmodified — only the `StatusCommandRunning` conformer that +actually spawns `git`/`jj` changed, so agent-routed and native writes classify identically by +construction. File and notification services remain unbuilt. All four PRs stay draft, stacked, +until the next stable release cuts from master. OQ1's gix measurements and OQ21's Swift driver decision are answered. The gix code remains a spike. Phases 3 and 4 — persistent remote transport, distribution, provisioning and UI — remain planned. The implementation sequence and remaining decisions are in **Next Steps**. @@ -1922,9 +1929,12 @@ disagreement passes every test on either side alone while presenting as an empty ## Next Steps -**Corrected 2026-09-17 against `master`.** Phase 1 is complete, OQ1's feasibility gate is lifted, -and the host-aware Phase 2 routing foundation is implemented. The remaining work is split into the -service milestones below so each layer can be reviewed and landed independently. +**Corrected 2026-09-18.** Phase 1 is complete, OQ1's feasibility gate is lifted, and items 1, 2 and +the VCS-writes half of item 3 are implemented on the draft stack (#201, #202, #204, #205) — the +exact-line-count decision item 1 demanded is delivered, not deferred. File access/notifications, +GitHub status and interchangeability (the rest of item 3) are the next milestones. The remaining +work is split into the service milestones below so each layer can be reviewed and landed +independently. 1. **Host-owned connection lifecycle implemented on the stacked branch.** The app-wide manager binds remote repository services to a connection generation, fails pending calls on disconnect, @@ -1969,8 +1979,32 @@ service milestones below so each layer can be reviewed and landed independently. persisted remote descriptors and remote UI remain later milestones. 3. **Deliver the remaining Phase 2 services as separately reviewable milestones.** - - VCS writes: preserve typed failures and Retry/Abort behavior; account for remote clones whose - project root and workroom root are the same repository. + + **VCS writes are implemented (#205).** `wr-agent` gained a generic exec service on the same + `Service::Vcs` envelope reads use, discriminated from a read `Request` by a `"kind":"exec"` field + so the existing read wire format is untouched: argv (allowlisted to `git`/`jj`), cwd, stdin and a + timeout in; exit code, stdout, stderr, `timed_out` and `signaled` out, mirroring + `StatusCommandRunner`'s own SIGTERM-then-grace-then-SIGKILL shape. `CLIVCSWriter`'s existing arg + building, output parsing and failure classification are untouched — only the + `StatusCommandRunning` conformer it spawns through changed (`AgentCommandRunner`, forwarding to + the new exec service instead of `Process`), so agent-routed and native writes classify identically + by construction rather than by keeping two implementations in sync. The exec service deliberately + never takes the JJ snapshot barrier itself: `CLIVCSWriter`'s `JJSnapshotGate` already holds + `/.jj/workroom-vcs.lock` for the whole gated write before any exec request goes out, and + wr-agent's own `SnapshotLock` locks the identical file for its read-side snapshots — acquiring it + again on the write path would self-deadlock. `RepositoryRouter.writer(for:)` falls back to a + native `CLIVCSWriter` only when obtaining the writer itself fails (`VCSError.backendVersion`, a + pre-upgrade agent with no write capability) — never mid-write, matching the read path's existing + fallback discipline exactly. Network commands (fetch/push/pull) forward a small allowlisted set of + auth env vars (`SSH_AUTH_SOCK`, `GIT_SSH_COMMAND`, etc.), resolved client-side via the same + `ShellEnvironment` probe a native push already depends on, rather than trusting wr-agent's own + (possibly stale, possibly launchd-minimal) inherited environment. A remote clone whose project + root and workroom root are the same repository needs no special handling: `SnapshotLock`/ + `JJProcessBarrier` already canonicalize and compare the two, so equal roots resolve to one lock + file. Filesystem probes `CLIVCSWriter` still runs locally (`sequencerState`, `worktreeGitDir`, + `gitLastFetch`, `existingLockFile`) are the one deliberate remaining gap — correct for today's + local-only agent, and exactly the piece Phase 3's remote transport will need to move host-side. + - File access and notifications: move directory listing, raw reads and watches behind the agent; preserve coalescing and account for the different local-worktree and remote-clone watch layouts. Reconnect must refresh state and restore subscriptions without leaving stale panels. diff --git a/macapp/WorkroomApp/Core/AgentCommandRunner.swift b/macapp/WorkroomApp/Core/AgentCommandRunner.swift new file mode 100644 index 000000000..8d69b96a2 --- /dev/null +++ b/macapp/WorkroomApp/Core/AgentCommandRunner.swift @@ -0,0 +1,119 @@ +import Foundation + +/// Forwards `StatusCommandRunning` calls to wr-agent's exec service instead of spawning `git`/`jj` +/// locally. This is the whole seam `AgentVCSConnection.writer` needs: `CLIVCSWriter`'s arg +/// building, stdout/stderr parsing and failure classification are untouched Swift, and see exactly +/// the bytes a host-executed command produced — see `vcs.rs`'s `ExecRequest` doc for the design. +/// +/// Never throws: `StatusCommandRunning.run` doesn't, so any transport or decode failure here +/// degrades to `CommandResult.launchFailed`, the same fact `StatusCommandRunner` reports when +/// `Process.run()` itself fails — `CLIVCSWriter.classify`/`.classifyCommit` already handle it. +struct AgentCommandRunner: StatusCommandRunning, Sendable { + let connection: AgentVCSConnection + + func run(_ executable: String, _ args: [String], in directory: String, timeout: TimeInterval) + async -> CommandResult + { + await exec(executable, args, in: directory, timeout: timeout, stdin: nil, network: false) + } + + func run( + _ executable: String, _ args: [String], in directory: String, timeout: TimeInterval, + stdin: Data? + ) async -> CommandResult { + await exec(executable, args, in: directory, timeout: timeout, stdin: stdin, network: false) + } + + func runNetwork( + _ executable: String, _ args: [String], in directory: String, timeout: TimeInterval + ) async -> CommandResult { + await exec(executable, args, in: directory, timeout: timeout, stdin: nil, network: true) + } + + private func exec( + _ executable: String, _ args: [String], in directory: String, timeout: TimeInterval, + stdin: Data?, network: Bool + ) async -> CommandResult { + // Always forwarded, network or not: wr-agent is a long-lived daemon "negotiated with, never + // replaced" (`LocalAgentVCS`), so its own inherited PATH at spawn time can predate a tool + // install and is never refreshed — without this, a write that would succeed natively could + // fail to find `git`/`jj` through an old agent. + var env = ["PATH": ShellEnvironment.path()] + if network { env.merge(Self.networkEnv()) { _, replacement in replacement } } + let request = AgentExecRequest( + executable: executable, args: args, dir: directory, + timeoutMs: Int((timeout * 1000).rounded(.up)), + // Latin-1, not UTF-8: a pathspec payload is NUL-separated and paths are not guaranteed + // valid UTF-8, and Latin-1 is a total bijection over every byte 0x00-0xFF, so this never + // fails and `vcs.rs`'s `latin1_bytes` is its exact inverse — see `ExecRequest.stdin`'s doc. + // Far more compact on the wire than a JSON byte-number array for ordinary text. + stdin: stdin.map { String(data: $0, encoding: .isoLatin1) ?? "" }, + env: env) + do { + // Slack above the command's own timeout: the round trip and the agent's own bookkeeping + // must not race the command's own timeout into a spurious `.launchFailed`. + let reply = try await connection.request(request, timeout: timeout + 15) + let result = try AgentVCSReply.decode(reply) + return CommandResult( + stdout: result.stdout, stderr: result.stderr, exitCode: result.exitCode, + timedOut: result.timedOut, signaled: result.signaled) + } catch { + return CommandResult( + stdout: "", stderr: "\(error)", exitCode: CommandResult.launchFailed, timedOut: false) + } + } + + /// The same forwarded-auth-key computation `StatusCommandRunner.networkEnvironment` does for a + /// native network command, as a standalone delta rather than a merged base environment: the + /// agent process's OWN inherited environment may predate this request and carry a stale or + /// absent `SSH_AUTH_SOCK` (see `ShellEnvironment`'s whole reason for existing), so the relevant + /// keys are resolved here, in the caller's process, and sent rather than re-derived agent-side. + private static func networkEnv() -> [String: String] { + StatusCommandRunner.networkEnvironment(base: [:], probed: ShellEnvironment.environment()) + } +} + +struct AgentExecRequest: Encodable, Sendable { + var version = 1 + var kind = "exec" + let executable: String + let args: [String] + let dir: String + let timeoutMs: Int + var stdin: String? + var env: [String: String] +} + +struct AgentExecResult: Decodable, Sendable { + let stdout: String + let stderr: String + let exitCode: Int32 + let timedOut: Bool + let signaled: Bool +} + +/// Feeds `CLIVCSWriter.remoteState`'s `currentRef` read through the agent-routed reader that +/// `AgentVCSConnection.writer` already has, instead of shelling a native process for it. +/// `CLIVCSWriter`'s own doc on `makeProvider` says the seam exists "for `currentRef` only"; every +/// other method here enforces that by throwing rather than silently answering something wrong. +struct AgentCurrentRefProvider: LocalVCSProviding { + let reader: VCSProviding + + func currentRef(root: URL) async throws -> VCSRef { try await reader.currentRef() } + + func log(root: URL, limit: Int) throws -> VCSHistoryPage { throw Self.unused } + func changeset(root: URL, commitID: String) async throws -> VCSChangeset { throw Self.unused } + func fileDiff(root: URL, commitID: String, path: String) async throws -> String { + throw Self.unused + } + func workingFileDiff(root: URL, path: String, base: VCSWorkingDiffBase) async throws -> String { + throw Self.unused + } + func fileContent(root: URL, rev: String, path: String) async throws -> String? { + throw Self.unused + } + + private static var unused: VCSError { + .unsupportedRepo("AgentCurrentRefProvider only answers currentRef") + } +} diff --git a/macapp/WorkroomApp/Core/AgentVCSConnection.swift b/macapp/WorkroomApp/Core/AgentVCSConnection.swift index 564dcd96c..746e3a878 100644 --- a/macapp/WorkroomApp/Core/AgentVCSConnection.swift +++ b/macapp/WorkroomApp/Core/AgentVCSConnection.swift @@ -22,6 +22,16 @@ final class AgentVCSConnection: HostServiceConnection, @unchecked Sendable { /// arrive. Tracked so a late reply drains harmlessly instead of `receive()` treating an unknown /// stream id as a protocol violation and tearing down every OTHER in-flight request too. private var abandoned: Set = [] + /// Negotiated once in `connect()`, before this connection is shared with any other caller. + /// `writes` is absent on a still-running pre-upgrade agent that answers `reads` but has no VCS + /// write service at all — `writer(context:reader:)` treats that as `VCSError.backendVersion`, + /// the same signal `RepositoryRouter` already falls back to native writes on. + private var _capabilities: AgentVCSCapabilities? + private var capabilities: AgentVCSCapabilities? { lock.withLock { _capabilities } } + /// The write methods `LocalVCSWriting` declares — matches wr-agent's `"writes"` capability count + /// (`vcs.rs`'s `capabilities` reply). Kept as one literal so a protocol change updates both ends + /// deliberately rather than by coincidence. + private static let writeMethodCount = 8 private init(host: HostID, descriptor: Int32) { self.host = host @@ -95,6 +105,9 @@ final class AgentVCSConnection: HostServiceConnection, @unchecked Sendable { guard capabilities.version == 1, capabilities.reads == 9 else { throw HostConnectionError.serviceUnavailable("Agent does not support these VCS reads.") } + // Not yet shared with any other caller, so a plain lock-guarded write is enough — no + // concurrent reader can observe a half-set value. + connection.lock.withLock { connection._capabilities = capabilities } return connection } catch { await connection.close() @@ -108,22 +121,49 @@ final class AgentVCSConnection: HostServiceConnection, @unchecked Sendable { return AgentVCSReader(context: context, connection: self) } + /// `reader` is the SAME agent-routed `VCSProviding` `HostConnectionManager.writer(context:)` just + /// built — threaded through rather than reconstructed so `CLIVCSWriter.remoteState`'s `currentRef` + /// read goes through the agent too (`AgentCurrentRefProvider`), not a fresh native process. A + /// still-running pre-upgrade agent has no write service at all; that is reported the same way an + /// unsupported read version is, so `RepositoryRouter` falls back to native writes rather than + /// leaving the repository unwritable. func writer(context: RepositoryContext, reader: VCSProviding) throws -> VCSWriting { - throw RepositoryRoutingError.unavailable(context.location.host) + guard context.location.host == host else { throw HostConnectionError.mismatchedContext } + guard lock.withLock({ !closed }) else { throw HostConnectionError.connectionLost } + guard capabilities?.writes == Self.writeMethodCount else { + throw VCSError.backendVersion("Agent does not support VCS writes.") + } + let engine = CLIVCSWriter( + vcs: context.backend.rawValue, runner: AgentCommandRunner(connection: self), + makeProvider: { _ in AgentCurrentRefProvider(reader: reader) }, gate: .shared) + return try BoundLocalWriter(context: context, reader: reader, writer: engine) } func close() async { fail(HostConnectionError.connectionLost) } - func request(_ request: AgentVCSRequest, timeout: TimeInterval = 30) async throws -> Data { + func request(_ request: Request, timeout: TimeInterval = 30) async throws + -> Data + { try Task.checkCancellation() let encoder = JSONEncoder() encoder.keyEncodingStrategy = .convertToSnakeCase let bytes = try encoder.encode(request) + // Matches the protocol's own per-envelope ceiling (`MAX_ENVELOPE_PAYLOAD` in + // `protocol/envelope.rs`) — requests are sent as ONE envelope, never chunked the way replies + // are, so raising this alone would only trade a typed `.partialData` failure here for a raw + // protocol violation there. A commit selecting many thousands of long paths (an `AgentExecRequest` + // stdin payload, `CLIVCSWriter`'s NUL-separated pathspec) could in principle exceed this; that + // would need real request chunking to lift, and is accepted as a known limit for now. guard bytes.count <= 1 << 20 else { throw VCSError.partialData("VCS request is too large.") } let cancellation = RequestCancellationBox() return try await withTaskCancellationHandler { try await withCheckedThrowingContinuation { continuation in let stream: UInt32? = lock.withLock { + // ponytail: one 32-slot pool shared by every read AND write on this connection, app-wide. + // A write can now legitimately hold a slot for minutes (`commitTimeout` = 600s), where only + // reads (seconds at most) used to compete for these slots. Exhausting the pool degrades to + // `.connectionLost` for the next caller rather than a distinct backpressure signal. Split + // reads and writes onto separate pools/connections if this is ever observed in practice. guard !closed, nextStream < UInt32.max, pending.count < 32 else { return nil } let stream = nextStream nextStream += 1 @@ -258,6 +298,8 @@ final class AgentVCSConnection: HostServiceConnection, @unchecked Sendable { struct AgentVCSCapabilities: Decodable { let version: Int let reads: Int + /// Absent on a still-running pre-upgrade agent that predates the VCS write service. + let writes: Int? } struct AgentVCSRequest: Encodable, Sendable { diff --git a/macapp/WorkroomApp/Core/LocalAgentVCS.swift b/macapp/WorkroomApp/Core/LocalAgentVCS.swift index 1d31232e2..4856b2df7 100644 --- a/macapp/WorkroomApp/Core/LocalAgentVCS.swift +++ b/macapp/WorkroomApp/Core/LocalAgentVCS.swift @@ -29,6 +29,16 @@ actor LocalAgentVCS { } func reader(context: RepositoryContext) async throws -> VCSProviding { + try await ensureConnected(for: context) + return try await manager.reader(context: context) + } + + func writer(context: RepositoryContext) async throws -> VCSWriting { + try await ensureConnected(for: context) + return try await manager.writer(context: context) + } + + private func ensureConnected(for context: RepositoryContext) async throws { guard context.location.host == .local else { throw HostConnectionError.mismatchedContext } try Task.checkCancellation() // Captured as local lets: plain Sendable values, so the nested closures below (some running on @@ -91,6 +101,5 @@ actor LocalAgentVCS { } } try Task.checkCancellation() - return try await manager.reader(context: context) } } diff --git a/macapp/WorkroomApp/Core/RepositoryLocation.swift b/macapp/WorkroomApp/Core/RepositoryLocation.swift index cf620f9ff..298274f22 100644 --- a/macapp/WorkroomApp/Core/RepositoryLocation.swift +++ b/macapp/WorkroomApp/Core/RepositoryLocation.swift @@ -123,7 +123,9 @@ final class RepositoryRouter: @unchecked Sendable { return RepositoryRouter() } #endif - return RepositoryRouter(localReader: { try await LocalAgentVCS.shared.reader(context: $0) }) + return RepositoryRouter( + localReader: { try await LocalAgentVCS.shared.reader(context: $0) }, + localWriter: { try await LocalAgentVCS.shared.writer(context: $0) }) }() private let lock = NSLock() private var entries: [RepositoryLocation: Entry] = [:] @@ -134,15 +136,20 @@ final class RepositoryRouter: @unchecked Sendable { let remoteWriter: @Sendable (RepositoryContext, VCSProviding) throws -> VCSWriting private let connections: HostConnectionManager? private let localReader: (@Sendable (RepositoryContext) async throws -> VCSProviding)? + /// nil in every test router that doesn't opt in (the vast majority, read-focused) — `writer(for:)` + /// then goes straight to the native `CLIVCSWriter` fallback it always has, unchanged. + private let localWriter: (@Sendable (RepositoryContext) async throws -> VCSWriting)? /// Production routers share app-wide host connections. Tests can retain native local providers /// or inject an isolated agent without starting a service against the user's session socket. init( connections: HostConnectionManager = .shared, - localReader: (@Sendable (RepositoryContext) async throws -> VCSProviding)? = nil + localReader: (@Sendable (RepositoryContext) async throws -> VCSProviding)? = nil, + localWriter: (@Sendable (RepositoryContext) async throws -> VCSWriting)? = nil ) { self.connections = connections self.localReader = localReader + self.localWriter = localWriter self.remoteReader = { throw RepositoryRoutingError.unavailable($0.location.host) } self.remoteWriter = { context, _ in throw RepositoryRoutingError.unavailable(context.location.host) @@ -157,6 +164,7 @@ final class RepositoryRouter: @unchecked Sendable { ) { self.connections = nil self.localReader = nil + self.localWriter = nil self.remoteReader = remoteReader self.remoteWriter = remoteWriter } @@ -293,6 +301,16 @@ final class RepositoryRouter: @unchecked Sendable { reader = try self.reader(context: context) } guard location.host == .local else { return try remoteWriter(context, reader) } + // Local writes route through the same agent connection reads already use — mirroring + // `reader(for:)` above — and fall back to a native `CLIVCSWriter` only when the agent predates + // the write service (`VCSError.backendVersion`, from `AgentVCSConnection.writer`'s capability + // check). The fallback only ever fires before any write is attempted, never mid-operation: + // obtaining a writer here does not execute anything. + if let localWriter { + do { + return try await localWriter(context) + } catch VCSError.backendVersion(_) {} + } let provider: LocalVCSProviding = context.backend == .jj ? RustJJProvider() : GitProvider() let writer = CLIVCSWriter( vcs: context.backend.rawValue, runner: StatusCommandRunner(), diff --git a/macapp/WorkroomAppTests/AgentVCSIntegrationTests.swift b/macapp/WorkroomAppTests/AgentVCSIntegrationTests.swift index 9432f7227..31ea2ff8a 100644 --- a/macapp/WorkroomAppTests/AgentVCSIntegrationTests.swift +++ b/macapp/WorkroomAppTests/AgentVCSIntegrationTests.swift @@ -79,6 +79,20 @@ final class AgentVCSIntegrationTests: XCTestCase { try router.register(.init(location: location, backend: backend, sharedLocation: location)) return (router, location) } + /// As `router(root:backend:connection:)`, but also wires `localWriter` — the write-path tests' + /// analogue of `reader(for:)`'s existing coverage. + private func writingRouter( + root: URL, backend: RepositoryBackend, connection: AgentVCSConnection + ) async throws -> (RepositoryRouter, RepositoryLocation) { + let location = try await RepositoryLocation.local(root.path) + let router = RepositoryRouter( + localReader: { try connection.reader(context: $0) }, + localWriter: { context in + try connection.writer(context: context, reader: try connection.reader(context: context)) + }) + try router.register(.init(location: location, backend: backend, sharedLocation: location)) + return (router, location) + } func testAllNineGitReadsMatchNativeProvider() async throws { let root = try gitRepo() @@ -505,4 +519,179 @@ final class AgentVCSIntegrationTests: XCTestCase { XCTAssertEqual( attempts.value(), 1, "four concurrent callers must coalesce onto one connect attempt") } + + // MARK: - Writes (#205) + + func testGitCommitRoutesThroughTheAgentAndMatchesNativeReads() async throws { + let root = try gitRepo() + try "second\n".write(to: root.appendingPathComponent("file"), atomically: true, encoding: .utf8) + let connection = try await connect() + let (router, location) = try await writingRouter( + root: root, backend: .git, connection: connection) + let writer = try await router.writer(for: location) + let result = await writer.commit( + request: VCSCommitRequest( + message: "agent commit", + files: [ChangedFile(path: "file", change: .modified, oldPath: nil)], + mode: .commit)) + guard case .ok(_, let revision) = result else { + XCTFail("commit failed: \(result)") + return + } + XCTAssertNotNil(revision) + let reader = try await router.reader(for: location) + let native = BoundLocalReader(context: reader.context, provider: GitProvider()) + let page = try await native.log(limit: 1) + XCTAssertEqual(page.commits.first?.summary, "agent commit") + XCTAssertEqual(page.commits.first?.commitID, revision) + await connection.close() + } + + /// The property the whole design leans on: the agent-backed writer's `jj commit` runs inside the + /// SAME `JJSnapshotGate`/`JJProcessBarrier` a native writer would, and the agent's own exec + /// service must never try to take that barrier a second time — see `vcs.rs`'s + /// `exec_never_contends_with_a_held_snapshot_barrier` for the Rust-side proof of the same + /// property. If this ever regressed, the commit below would hang for up to 30s and fail the test + /// on timeout instead of completing. + func testJJCommitRoutesThroughTheAgentWithoutContendingItsOwnSnapshotBarrier() async throws { + let root = try root() + try run("jj", ["git", "init", "--colocate"], at: root) + try "base\n".write(to: root.appendingPathComponent("file"), atomically: true, encoding: .utf8) + try run("jj", ["commit", "-m", "initial"], at: root) + try "changed\n".write( + to: root.appendingPathComponent("file"), atomically: true, encoding: .utf8) + let connection = try await connect() + let (router, location) = try await writingRouter( + root: root, backend: .jj, connection: connection) + let writer = try await router.writer(for: location) + let result = await writer.commit( + request: VCSCommitRequest(message: "agent jj commit", files: [], mode: .commit)) + guard case .ok = result else { + XCTFail("jj commit failed: \(result)") + return + } + let reader = try await router.reader(for: location) + let status = try await reader.workingStatus() + XCTAssertEqual(status.dirty, false) + let page = try await reader.log(limit: 5) + XCTAssertTrue(page.commits.contains { $0.summary == "agent jj commit" }) + await connection.close() + } + + func testPushAndPullRouteThroughTheAgentAgainstARealRemote() async throws { + let root = try gitRepo() + let bareRemote = try self.root() + try run("git", ["init", "--bare", "-b", "main"], at: bareRemote) + try run("git", ["remote", "add", "origin", bareRemote.path], at: root) + let connection = try await connect() + let (router, location) = try await writingRouter( + root: root, backend: .git, connection: connection) + let writer = try await router.writer(for: location) + let pushed = await writer.push( + current: VCSRef(name: "main", kind: .branch), remote: "origin", setUpstream: true, + anonymousRevision: "") + guard case .ok = pushed else { + XCTFail("push failed: \(pushed)") + return + } + let remoteHead = try run("git", ["rev-parse", "main"], at: bareRemote) + let localHead = try run("git", ["rev-parse", "HEAD"], at: root) + XCTAssertEqual(remoteHead, localHead) + + // A second clone pushes a new commit, so this workroom's `fetch`/`pullRebase` have something + // real to bring in. + let secondClone = try self.root() + try run("git", ["clone", bareRemote.path, "."], at: secondClone) + try run("git", ["config", "user.name", "Test"], at: secondClone) + try run("git", ["config", "user.email", "test@example.com"], at: secondClone) + try "from-second-clone\n".write( + to: secondClone.appendingPathComponent("other"), atomically: true, encoding: .utf8) + try run("git", ["add", "."], at: secondClone) + try run("git", ["commit", "-m", "from second clone"], at: secondClone) + try run("git", ["push", "origin", "main"], at: secondClone) + + let pulled = await writer.pullRebase( + current: VCSRef(name: "main", kind: .branch), remote: "origin", + tracking: VCSTracking(comparedTo: "origin/main", ahead: 0, behind: 1, gone: false)) + guard case .ok = pulled else { + XCTFail("pull failed: \(pulled)") + return + } + XCTAssertTrue(FileManager.default.fileExists(atPath: root.appendingPathComponent("other").path)) + await connection.close() + } + + /// Selecting a tracked file with no actual diff against `HEAD` classifies as `.nothingToCommit` — + /// proving a specific, named failure (not just success/failure) survives the wire byte-for-byte, + /// since it depends on git's own exit code AND stderr text reaching `CLIVCSWriter.classifyCommit` + /// unmodified. + func testAFailingAgentCommitClassifiesExactlyAsTheNativeWriterWould() async throws { + let root = try gitRepo() + let connection = try await connect() + let (router, location) = try await writingRouter( + root: root, backend: .git, connection: connection) + let writer = try await router.writer(for: location) + let result = await writer.commit( + request: VCSCommitRequest( + message: "nothing changed", + files: [ChangedFile(path: "file", change: .modified, oldPath: nil)], mode: .commit)) + guard case .failed(.nothingToCommit) = result else { + XCTFail("expected .nothingToCommit, got \(result)") + return + } + await connection.close() + } + + /// Mirrors `testAgentPredatingVCSSupportFallsBackToNativeReads`: a still-running pre-upgrade + /// agent answers `capabilities` with no `writes` field at all, and `RepositoryRouter` must serve + /// the write natively rather than leaving the repository unwritable. + func testAgentPredatingVCSWriteSupportFallsBackToNativeWrites() async throws { + let root = try gitRepo() + try "second\n".write(to: root.appendingPathComponent("file"), atomically: true, encoding: .utf8) + let location = try await RepositoryLocation.local(root.path) + let router = RepositoryRouter( + localReader: { _ in throw VCSError.backendVersion("Agent predates VCS support.") }, + localWriter: { _ in throw VCSError.backendVersion("Agent predates VCS write support.") }) + try router.register(.init(location: location, backend: .git, sharedLocation: location)) + let writer = try await router.writer(for: location) + let result = await writer.commit( + request: VCSCommitRequest( + message: "native fallback commit", + files: [ChangedFile(path: "file", change: .modified, oldPath: nil)], mode: .commit)) + guard case .ok = result else { + XCTFail("native fallback commit failed: \(result)") + return + } + } + + /// The realistic shape of `testAgentPredatingVCSWriteSupportFallsBackToNativeWrites`'s scenario: + /// a running agent whose `capabilities` reply has `reads: 9` but no `writes` (or a version the + /// `writes` count check rejects), so `reader(for:)` resolves through the REAL agent while + /// `writer(for:)`'s capability check alone fails — leaving the native `CLIVCSWriter` fallback + /// wired to an agent-sourced `reader`, not a freshly-native one (`RepositoryLocation.swift`'s + /// `writer(for:)`). A code-review pass flagged that the all-native version above never exercises + /// this reader/writer-origin mismatch. + func testWriteCapabilityFallbackWorksWithAnAgentSourcedReader() async throws { + let root = try gitRepo() + try "second\n".write(to: root.appendingPathComponent("file"), atomically: true, encoding: .utf8) + let connection = try await connect() + let location = try await RepositoryLocation.local(root.path) + let router = RepositoryRouter( + localReader: { try connection.reader(context: $0) }, + localWriter: { _ in throw VCSError.backendVersion("Agent predates VCS write support.") }) + try router.register(.init(location: location, backend: .git, sharedLocation: location)) + // Confirms the reader really is agent-backed, not incidentally also falling back. + let ref = try await router.reader(for: location).currentRef() + XCTAssertEqual(ref.name, "main") + let writer = try await router.writer(for: location) + let result = await writer.commit( + request: VCSCommitRequest( + message: "mixed-origin fallback commit", + files: [ChangedFile(path: "file", change: .modified, oldPath: nil)], mode: .commit)) + guard case .ok = result else { + XCTFail("mixed-origin fallback commit failed: \(result)") + return + } + await connection.close() + } } diff --git a/vcs/crates/wr-agent/src/vcs.rs b/vcs/crates/wr-agent/src/vcs.rs index 77d4f5d91..c320022da 100644 --- a/vcs/crates/wr-agent/src/vcs.rs +++ b/vcs/crates/wr-agent/src/vcs.rs @@ -5,18 +5,44 @@ use crate::protocol::envelope::{Envelope, Service, MAX_ENVELOPE_PAYLOAD}; use crate::session::SharedWriter; use serde::Deserialize; use serde_json::{json, Value}; -use std::io::Write; +use std::io::{Read, Write}; use std::os::fd::AsRawFd; use std::os::unix::fs::OpenOptionsExt; +use std::os::unix::process::{CommandExt, ExitStatusExt}; use std::path::{Path, PathBuf}; +use std::process::{Command, Stdio}; use std::sync::{ atomic::{AtomicUsize, Ordering}, Arc, }; +use std::time::{Duration, Instant}; use wr_vcs_model::{self as model, VcsError}; const MAX_RESPONSE: usize = 16 * 1024 * 1024; const MAX_HISTORY_LIMIT: usize = 10000; +/// `CLIVCSWriter.commitTimeout` (Swift) is 600s, the longest legitimate write timeout. Bounds a +/// hostile/buggy request from wedging an exec thread indefinitely; the 32-slot `ACTIVE` permit +/// already bounds concurrency, this bounds duration. +const MAX_EXEC_TIMEOUT_MS: u64 = 610_000; +/// Per-stream cap, matching `StatusCommandRunner.maxBytes`'s default on the Swift side. +const MAX_EXEC_STREAM: usize = 4 * 1024 * 1024; +/// Env vars an exec request may forward to the child, resolved by the client rather than re-derived +/// here — the agent's own inherited environment may predate the request and carry a stale value +/// (see `AgentCommandRunner.swift`). `PATH` is always sent, from the SAME `ShellEnvironment.path()` +/// the native writer uses via `/usr/bin/env`: wr-agent is a long-lived daemon that "is negotiated +/// with, never replaced" (`LocalAgentVCS`), so its own inherited PATH at spawn time can predate a +/// tool install and is never refreshed; without this, a write that would succeed natively could fail +/// to find `git`/`jj` through an old agent. The rest is `StatusCommandRunner.networkEnvironment`'s +/// set, sent only for a network write. Anything else in the request's `env` map is silently dropped. +const ALLOWED_EXEC_ENV_KEYS: &[&str] = &[ + "PATH", + "SSH_AUTH_SOCK", + "SSH_AGENT_PID", + "GIT_SSH_COMMAND", + "GIT_CONFIG_GLOBAL", + "XDG_CONFIG_HOME", + "SSH_ASKPASS_REQUIRE", +]; static ACTIVE: AtomicUsize = AtomicUsize::new(0); /// Whether any dispatched VCS request — including a JJ snapshot that owns the working-copy lock @@ -55,6 +81,61 @@ enum Backend { Jj, } +/// A write command, run to completion and reported back raw — argv, cwd, stdin and timeout in; +/// exit code, stdout, stderr, `timed_out`, `signaled` out. Every request-building, output-parsing +/// and failure-classification decision (`CLIVCSWriter.classify`/`.classifyCommit`, the pathspec +/// and refspec injection defenses, the retry/abort taxonomy) stays in Swift and sees exactly the +/// bytes it would see from a local `git`/`jj` — this is deliberately NOT a reimplementation of +/// commit/push/pull semantics, so criterion 2 ("typed failures... match pre-agent behavior +/// exactly") holds by construction rather than by keeping two classifiers in sync. +/// +/// **Never acquires `SnapshotLock`.** A caller that needs the JJ working-copy barrier for a +/// mutating command already holds it — `CLIVCSWriter`'s `gate: JJSnapshotGate` takes the same +/// `/.jj/workroom-vcs.lock` flock (`JJProcessBarrier`, shared by name with `SnapshotLock` +/// above) for the whole gated operation before any exec request goes out. Taking it again here +/// would self-deadlock the same actor for 30s and then fail as `LockContention`. +#[derive(Deserialize)] +#[serde(deny_unknown_fields)] +struct ExecRequest { + version: u32, + kind: ExecKind, + executable: Executable, + args: Vec, + dir: String, + timeout_ms: u64, + /// Latin-1 (ISO 8859-1), not UTF-8: a pathspec payload is NUL-separated and paths are not + /// guaranteed valid UTF-8, so decoding this as UTF-8 (or sending it as a JSON byte-number array, + /// ~3-4x larger on the wire for ordinary text) would either corrupt or bloat it. Latin-1 is a + /// total bijection over every byte 0x00-0xFF, so `AgentCommandRunner.swift`'s + /// `String(data:encoding:.isoLatin1)` never fails and `latin1_bytes` below is its exact inverse + /// — every byte round-trips, at roughly the original size for ordinary text. + #[serde(default)] + stdin: Option, + #[serde(default)] + env: std::collections::BTreeMap, +} + +#[derive(Deserialize)] +#[serde(rename_all = "snake_case")] +enum ExecKind { + Exec, +} + +#[derive(Clone, Copy, Deserialize)] +#[serde(rename_all = "snake_case")] +enum Executable { + Git, + Jj, +} +impl Executable { + fn as_str(self) -> &'static str { + match self { + Executable::Git => "git", + Executable::Jj => "jj", + } + } +} + fn io(error: impl std::fmt::Display) -> VcsError { VcsError::Io(error.to_string()) } @@ -176,15 +257,212 @@ fn content(root: &Path, rev: &str, path: &str) -> model::Result { } pub fn execute(bytes: &[u8]) -> Value { - let result = serde_json::from_slice::(bytes) + // `kind` distinguishes an exec request from the existing read `Request`; its absence (every + // read request in the wild, and every request before this field existed) means "read", so the + // wire format already spoken by `AgentVCSReader` is untouched. + let result = serde_json::from_slice::(bytes) .map_err(io) - .and_then(read); + .and_then(|value| { + if value.get("kind").and_then(Value::as_str) == Some("exec") { + serde_json::from_value::(value) + .map_err(io) + .and_then(exec) + } else { + serde_json::from_value::(value) + .map_err(io) + .and_then(read) + } + }); match result { Ok(result) => json!({"version": 1, "result": result}), Err(error) => json!({"version": 1, "error": error}), } } +struct Captured { + stdout: Vec, + stderr: Vec, + exit_code: i32, + timed_out: bool, + signaled: bool, +} + +/// Drain a pipe to EOF on its own thread (mirrors `StatusCommandRunner.readCapped`): a blocking +/// read is safe here because it runs off the thread that watches for timeout/exit, and keeps +/// draining past `cap` so the child can never block on a full pipe buffer. +fn drain_capped(mut source: impl Read, cap: usize) -> Vec { + let mut collected = Vec::new(); + let mut chunk = [0u8; 65536]; + loop { + match source.read(&mut chunk) { + Ok(0) => return collected, + Ok(count) => { + if collected.len() < cap { + let take = (cap - collected.len()).min(count); + collected.extend_from_slice(&chunk[..take]); + } + } + Err(error) if error.kind() == std::io::ErrorKind::Interrupted => continue, + Err(_) => return collected, + } + } +} + +/// Run `executable` to completion, capturing everything a `StatusCommandRunning` conformer needs +/// to classify the outcome exactly as the native path would. Mirrors `StatusCommandRunner.run`'s +/// SIGTERM-then-grace-then-SIGKILL shape (`CLIVCSWriter.commitTimeout`'s doc: "Killing a commit is +/// categorically more dangerous than killing a fetch" — an immediate SIGKILL denies a `post-commit` +/// hook the chance to exit cleanly and can leave `index.lock` behind). +fn run_exec( + dir: &Path, + executable: &str, + args: &[String], + timeout: Duration, + stdin: Option<&[u8]>, + env: &[(&str, &str)], +) -> model::Result { + let mut command = Command::new(executable); + command + .current_dir(dir) + .args(args) + .stdin(if stdin.is_some() { + Stdio::piped() + } else { + Stdio::null() + }) + .stdout(Stdio::piped()) + .stderr(Stdio::piped()) + .process_group(0) + // Same baseline as every other subprocess in this codebase (`StatusCommandRunner.run`, + // `wr_vcs_git::diff::run_bounded`): disable git's own lock-contention retries and terminal + // prompting, and pin the message locale so Swift's stderr-substring classifiers keep working. + .env("GIT_OPTIONAL_LOCKS", "0") + .env("GIT_TERMINAL_PROMPT", "0") + .env("LC_ALL", "C") + // A workroom can be a clone of an untrusted repo; never run an inherited external-diff/askpass + // helper or reach for a graphical prompt this headless agent has no display for. + .env_remove("GIT_EXTERNAL_DIFF") + .env_remove("SSH_ASKPASS") + .env_remove("DISPLAY") + .envs(env.iter().copied()); + for key in [ + "GIT_DIR", + "GIT_WORK_TREE", + "GIT_INDEX_FILE", + "GIT_COMMON_DIR", + "GIT_OBJECT_DIRECTORY", + "GIT_ALTERNATE_OBJECT_DIRECTORIES", + ] { + command.env_remove(key); + } + let mut child = command.spawn().map_err(io)?; + + if let Some(payload) = stdin { + let mut handle = child.stdin.take().expect("piped stdin"); + let payload = payload.to_vec(); + // Off the waiting thread: a payload larger than the pipe buffer blocks until the child + // drains it, and the child may not start reading until after argv parsing. + std::thread::spawn(move || { + let _ = handle.write_all(&payload); + // `handle` drops here, closing the write end so the child sees EOF. + }); + } + + let stdout_pipe = child.stdout.take().expect("piped stdout"); + let stderr_pipe = child.stderr.take().expect("piped stderr"); + let out = std::thread::spawn(move || drain_capped(stdout_pipe, MAX_EXEC_STREAM)); + let err = std::thread::spawn(move || drain_capped(stderr_pipe, MAX_EXEC_STREAM)); + + let pid = child.id() as i32; + let start = Instant::now(); + let mut timed_out = false; + let mut sent_term = false; + let status = 'wait: loop { + if let Ok(Some(status)) = child.try_wait() { + break 'wait status; + } + let elapsed = start.elapsed(); + if elapsed >= timeout { + timed_out = true; + if !sent_term { + sent_term = true; + unsafe { libc::kill(-pid, libc::SIGTERM) }; + } else if elapsed >= timeout + Duration::from_secs(2) { + unsafe { libc::kill(-pid, libc::SIGKILL) }; + break 'wait child.wait().map_err(io)?; + } + } + std::thread::sleep(Duration::from_millis(10)); + }; + let stdout = out.join().map_err(|_| io("stdout reader panicked"))?; + let stderr = err.join().map_err(|_| io("stderr reader panicked"))?; + Ok(Captured { + stdout, + stderr, + exit_code: status + .code() + .unwrap_or_else(|| status.signal().unwrap_or(-1)), + timed_out, + signaled: status.signal().is_some(), + }) +} + +/// Keep only the env vars an exec request is allowed to set — see `ALLOWED_EXEC_ENV_KEYS`. Pure, +/// so the allowlist itself is unit-testable without spawning anything. +fn filter_exec_env(env: &std::collections::BTreeMap) -> Vec<(&str, &str)> { + env.iter() + .filter(|(key, _)| ALLOWED_EXEC_ENV_KEYS.contains(&key.as_str())) + .map(|(key, value)| (key.as_str(), value.as_str())) + .collect() +} + +/// The exact inverse of `AgentCommandRunner.swift`'s `String(data:encoding:.isoLatin1)` — see +/// `ExecRequest.stdin`'s doc. Every `char` a Latin-1-decoded string can contain is U+0000-U+00FF +/// by construction; a value outside that range means the client sent something else, so this +/// rejects rather than silently truncates it. +fn latin1_bytes(text: &str) -> model::Result> { + text.chars() + .map(|c| u8::try_from(c as u32).map_err(|_| io("stdin is not Latin-1"))) + .collect() +} + +fn exec(request: ExecRequest) -> model::Result { + let ExecRequest { + version, + kind: ExecKind::Exec, + executable, + args, + dir, + timeout_ms, + stdin, + env, + } = request; + if version != 1 { + return Err(VcsError::BackendVersion( + "unsupported VCS service version".into(), + )); + } + let dir = absolute(&dir)?; + let timeout = Duration::from_millis(timeout_ms.clamp(1, MAX_EXEC_TIMEOUT_MS)); + let env = filter_exec_env(&env); + let stdin = stdin.as_deref().map(latin1_bytes).transpose()?; + let captured = run_exec( + &dir, + executable.as_str(), + &args, + timeout, + stdin.as_deref(), + &env, + )?; + Ok(json!({ + "stdout": String::from_utf8_lossy(&captured.stdout), + "stderr": String::from_utf8_lossy(&captured.stderr), + "exit_code": captured.exit_code, + "timed_out": captured.timed_out, + "signaled": captured.signaled, + })) +} + fn read(request: Request) -> model::Result { if request.version != 1 { return Err(VcsError::BackendVersion( @@ -192,7 +470,7 @@ fn read(request: Request) -> model::Result { )); } if request.method == "capabilities" { - return Ok(json!({"version": 1, "reads": 9})); + return Ok(json!({"version": 1, "reads": 9, "writes": 8})); } if request.limit.is_some_and(|limit| limit > MAX_HISTORY_LIMIT) { return Err(VcsError::PartialData(format!( @@ -450,4 +728,251 @@ mod tests { } std::fs::remove_dir_all(root).unwrap(); } + + /// Test-side inverse of `latin1_bytes`, standing in for `AgentCommandRunner.swift`'s + /// `String(data:encoding:.isoLatin1)`. + fn latin1_string(bytes: &[u8]) -> String { + bytes.iter().map(|&b| b as char).collect() + } + + #[test] + fn latin1_bytes_is_the_exact_inverse_of_latin1_string_for_every_byte_value() { + let all_bytes: Vec = (0..=255).collect(); + assert_eq!(latin1_bytes(&latin1_string(&all_bytes)).unwrap(), all_bytes); + } + + fn git_repo(name: &str) -> PathBuf { + let root = + std::env::temp_dir().join(format!("wr-vcs-exec-test-{name}-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&root); + std::fs::create_dir_all(&root).unwrap(); + for args in [ + vec!["init", "-q", "-b", "main"], + vec!["config", "user.name", "Test"], + vec!["config", "user.email", "test@example.com"], + ] { + assert!(Command::new("git") + .args(args) + .current_dir(&root) + .status() + .unwrap() + .success()); + } + root + } + + fn exec_request(dir: &Path, executable: &str, args: &[&str]) -> Vec { + serde_json::to_vec(&json!({ + "version": 1, + "kind": "exec", + "executable": executable, + "args": args, + "dir": dir.to_str().unwrap(), + "timeout_ms": 5000, + })) + .unwrap() + } + + #[test] + fn exec_runs_a_real_command_and_reports_its_own_output() { + let root = git_repo("basic"); + let reply = execute(&exec_request( + &root, + "git", + &["rev-parse", "--is-inside-work-tree"], + )); + let result = &reply["result"]; + assert_eq!(result["exit_code"], 0); + assert_eq!(result["timed_out"], false); + assert_eq!(result["signaled"], false); + assert!(result["stdout"].as_str().unwrap().trim() == "true"); + std::fs::remove_dir_all(&root).unwrap(); + } + + #[test] + fn exec_captures_a_failing_commands_exit_code_and_stderr() { + let root = git_repo("failing"); + let reply = execute(&exec_request( + &root, + "git", + &["rev-parse", "--verify", "bogus-ref"], + )); + let result = &reply["result"]; + assert_eq!(result["exit_code"], 128); + assert!(!result["stderr"].as_str().unwrap().is_empty()); + std::fs::remove_dir_all(&root).unwrap(); + } + + #[test] + fn exec_rejects_an_executable_outside_the_git_jj_allowlist() { + let root = git_repo("disallowed"); + let request = json!({ + "version": 1, "kind": "exec", "executable": "sh", "args": ["-c", "echo hi"], + "dir": root.to_str().unwrap(), "timeout_ms": 1000, + }); + let reply = execute(&serde_json::to_vec(&request).unwrap()); + assert!(reply.get("error").is_some()); + std::fs::remove_dir_all(&root).unwrap(); + } + + #[test] + fn exec_feeds_stdin_to_the_child() { + let root = git_repo("stdin"); + std::fs::write(root.join("file"), "one\n").unwrap(); + let command = json!({ + "version": 1, "kind": "exec", "executable": "git", + "args": ["hash-object", "--stdin"], "dir": root.to_str().unwrap(), "timeout_ms": 5000, + "stdin": latin1_string(b"two\n"), + }); + let reply = execute(&serde_json::to_vec(&command).unwrap()); + let result = &reply["result"]; + assert_eq!(result["exit_code"], 0); + // `git hash-object --stdin` prints the SHA of exactly what it read. + let expected = Command::new("git") + .args(["hash-object", "--stdin"]) + .current_dir(&root) + .stdin(Stdio::piped()) + .stdout(Stdio::piped()) + .spawn() + .and_then(|mut child| { + use std::io::Write as _; + child.stdin.take().unwrap().write_all(b"two\n")?; + child.wait_with_output() + }) + .unwrap(); + assert_eq!( + result["stdout"].as_str().unwrap().trim(), + String::from_utf8_lossy(&expected.stdout).trim() + ); + std::fs::remove_dir_all(&root).unwrap(); + } + + /// `stdin` round-trips via Latin-1 specifically so invalid-UTF-8 bytes (e.g. a raw pathspec + /// entry from a non-Mac-originated filename) survive the wire exactly — decoding as UTF-8 would + /// have silently replaced them with U+FFFD, corrupting the commit rather than failing loudly. + #[test] + fn exec_stdin_preserves_bytes_that_are_not_valid_utf8() { + let root = git_repo("stdin-binary"); + let payload: Vec = vec![0x66, 0x6f, 0x6f, 0xff, 0xfe, 0x00, 0x62, 0x61, 0x72]; + assert!( + String::from_utf8(payload.clone()).is_err(), + "fixture must be invalid UTF-8" + ); + let command = json!({ + "version": 1, "kind": "exec", "executable": "git", + "args": ["hash-object", "--stdin"], "dir": root.to_str().unwrap(), "timeout_ms": 5000, + "stdin": latin1_string(&payload), + }); + let reply = execute(&serde_json::to_vec(&command).unwrap()); + let result = &reply["result"]; + assert_eq!(result["exit_code"], 0); + let expected = Command::new("git") + .args(["hash-object", "--stdin"]) + .current_dir(&root) + .stdin(Stdio::piped()) + .stdout(Stdio::piped()) + .spawn() + .and_then(|mut child| { + use std::io::Write as _; + child.stdin.take().unwrap().write_all(&payload)?; + child.wait_with_output() + }) + .unwrap(); + assert_eq!( + result["stdout"].as_str().unwrap().trim(), + String::from_utf8_lossy(&expected.stdout).trim() + ); + std::fs::remove_dir_all(&root).unwrap(); + } + + #[test] + fn exec_times_out_and_kills_a_wedged_process_within_the_sigterm_grace() { + let root = git_repo("timeout"); + let start = Instant::now(); + let captured = run_exec( + &root, + "sleep", + &["30".into()], + Duration::from_millis(100), + None, + &[], + ) + .unwrap(); + assert!(captured.timed_out); + assert!(captured.signaled); + // SIGTERM at 100ms, grace to 2.1s, SIGKILL — must not run anywhere near the full 30s sleep. + assert!(start.elapsed() < Duration::from_secs(5)); + std::fs::remove_dir_all(&root).unwrap(); + } + + /// The one property the whole design leans on (see `ExecRequest`'s doc): a caller that already + /// holds the JJ snapshot barrier for a gated write must be able to run an exec request against + /// the SAME repository without the agent trying to take that barrier again. If `exec` ever + /// acquires `SnapshotLock`, this test hangs for 30s and then fails instead of returning quickly. + #[test] + fn exec_never_contends_with_a_held_snapshot_barrier() { + let root = + std::env::temp_dir().join(format!("wr-vcs-exec-barrier-test-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&root); + std::fs::create_dir_all(root.join(".jj/repo")).unwrap(); + let held = SnapshotLock::acquire(&root, root.to_str()).unwrap(); + let start = Instant::now(); + let reply = execute(&exec_request(&root, "jj", &["--version"])); + assert!(reply.get("result").is_some(), "exec failed: {reply:?}"); + assert!( + start.elapsed() < Duration::from_secs(5), + "exec waited on the snapshot barrier instead of running independently" + ); + drop(held); + std::fs::remove_dir_all(&root).unwrap(); + } + + #[test] + fn capabilities_reports_both_read_and_write_counts() { + let reply = execute(br#"{"version":1,"method":"capabilities"}"#); + assert_eq!(reply["result"]["reads"], 9); + assert_eq!(reply["result"]["writes"], 8); + } + + #[test] + fn filter_exec_env_drops_everything_outside_the_allowlist() { + let mut env = std::collections::BTreeMap::new(); + env.insert("SSH_AUTH_SOCK".to_string(), "/tmp/agent.sock".to_string()); + env.insert( + "WORKROOM_NOT_ALLOWED".to_string(), + "should-not-pass".to_string(), + ); + // PATH is allowlisted deliberately (see `ALLOWED_EXEC_ENV_KEYS`'s doc) so an agent-routed + // write can find a `git`/`jj` install its own long-lived process predates. + env.insert("PATH".to_string(), "/usr/local/bin:/usr/bin".to_string()); + let filtered = filter_exec_env(&env); + assert_eq!( + filtered, + vec![ + ("PATH", "/usr/local/bin:/usr/bin"), + ("SSH_AUTH_SOCK", "/tmp/agent.sock") + ] + ); + } + + #[test] + fn run_exec_only_applies_the_env_pairs_it_is_given() { + // `env` (not in the git/jj allowlist) is used directly via `run_exec`, bypassing `exec`'s + // executable restriction, specifically to observe what actually reaches the child — the + // allowlist filter itself is `filter_exec_env`'s job, covered above. + let root = git_repo("run-exec-env"); + let captured = run_exec( + &root, + "env", + &[], + Duration::from_secs(5), + None, + &[("SSH_AUTH_SOCK", "/tmp/allowed.sock")], + ) + .unwrap(); + let stdout = String::from_utf8_lossy(&captured.stdout); + assert!(stdout.contains("SSH_AUTH_SOCK=/tmp/allowed.sock")); + assert!(!stdout.contains("GIT_DIR=")); + std::fs::remove_dir_all(&root).unwrap(); + } } From f9711bbcdf5cdb00521539efefcdba3cc5da2144 Mon Sep 17 00:00:00 2001 From: Joel Moss Date: Fri, 18 Sep 2026 19:27:48 +0100 Subject: [PATCH 02/13] fix(macapp): make agent-routed writes behave like native ones An independent review found that "only the StatusCommandRunning conformer changes, so agent-routed and native writes classify identically by construction" was false. CLIVCSWriter is genuinely untouched -- the divergence was in what AgentCommandRunner FED it, which is harder to spot than two classifiers, because nothing looks out of sync. Five defects, all reproduced: - A descendant holding the child's pipe (ssh ControlPersist, a daemonising credential helper, gpg-agent from commit -S) wedged run_exec's unbounded join forever. That thread is the one dispatch spawned, so its Permit never dropped, one of 32 ACTIVE slots -- shared with every read -- leaked, and is_busy() stayed true so the daemon never idle-exited. 32 occurrences kill the VCS service until wr-agent is killed by hand. drain_capped now publishes into a shared sink and run_exec detaches the readers after a 2s deadline, mirroring StatusCommandRunner's own bounded drain.wait and its reasoning. - Every transport failure became CommandResult.launchFailed, whose doc reserves that value for "nothing ran". There is no cancel message in the protocol, so a cancelled or disconnected push completes host-side and was reported as never having launched -- inviting a retry that double-applies it. Split into neverRan (pre-send refusals, replies carrying an error) and outcomeUnknown, which carries the underlying error's own text; a new HostConnectionError.notDispatched keeps a local pool-full refusal on the honest side of that line. A closed connection deliberately still throws connectionLost, because connect()'s capabilities request routes through the same guard and LocalAgentVCS catches exactly that to spawn a stale-socket agent. - Cancelling a gated write released /.jj/workroom-vcs.lock while wr-agent kept writing. JJSnapshotGate's "a started native operation retains this tail until its actual completion" held only by accident: a Process inside runBlocking cannot observe Swift cancellation, an agent round trip can. The gate now shields its operation once it holds the flock -- there, not in the command runner, because the thing holding the lock is the thing that must outlive the child, and shielding in the runner also covered remoteState's ungated reads and stranded connection slots on every superseded refresh. - Non-network commands forwarded only PATH, so the child inherited GIT_AUTHOR_*, GIT_CONFIG_GLOBAL and HOME from whichever app launch first spawned the daemon -- silently authoring commits under a stale identity. No allowlist can close that; what git and jj read for identity, config, signing and hooks is open-ended. StatusCommandRunner.childEnvironment now builds one map for both paths and run_exec env_clear()s before applying it. - A missing git/jj raised an Io error, so a missing tool was reported as a deleted workroom -- the exact conflation launchFailed's doc warns about, in the mirror direction. run_exec spawns /usr/bin/env exactly as native does, making 127/126 env's real exit codes. Two follow-on rounds caught regressions in those fixes: env_clear() had dropped the GIT_DIR/GIT_WORK_TREE family scrub (a commit requested in one repository landed in another and reported success), and the timeout SIGKILL fired after normal exits and could then be skipped entirely. Both closed. The GIT_DIR scrub now lives in childEnvironment, so the NATIVE path gains it too -- it never had one, and was the unsafe side of that pair. Known gap, not closed here: an outcome-unknown result classifies as .other, which VCSSyncPresentation.retryAction still offers a Retry for. The message is truthful where it used to claim the command never ran, but suppressing the button needs a new VCSRemoteFailure case -- a user-visible taxonomy change. cargo test -p wr-agent 168 pass, clippy clean; make app-lint strict full-tree clean; make app-test APP_TEST_FLAGS= 2753 pass. Co-Authored-By: Claude Opus 5 (1M context) --- docs/designs/remote-workrooms.md | 28 +- .../WorkroomApp/Core/AgentCommandRunner.swift | 115 +++++-- .../WorkroomApp/Core/AgentVCSConnection.swift | 25 +- .../Core/HostConnectionManager.swift | 7 + macapp/WorkroomApp/Core/JJSnapshotGate.swift | 16 +- .../Core/StatusCommandRunner.swift | 86 +++-- .../AgentVCSProtocolTests.swift | 66 ++++ vcs/crates/wr-agent/src/vcs.rs | 301 ++++++++++++------ 8 files changed, 488 insertions(+), 156 deletions(-) diff --git a/docs/designs/remote-workrooms.md b/docs/designs/remote-workrooms.md index 3d221ab09..051e3d48d 100644 --- a/docs/designs/remote-workrooms.md +++ b/docs/designs/remote-workrooms.md @@ -1992,13 +1992,31 @@ independently. never takes the JJ snapshot barrier itself: `CLIVCSWriter`'s `JJSnapshotGate` already holds `/.jj/workroom-vcs.lock` for the whole gated write before any exec request goes out, and wr-agent's own `SnapshotLock` locks the identical file for its read-side snapshots — acquiring it - again on the write path would self-deadlock. `RepositoryRouter.writer(for:)` falls back to a + again on the write path would self-deadlock. That argument holds only while the client's lock + outlives the agent's child, so `JJSnapshotGate.run` shields the whole gated operation from task + cancellation once it holds the flock; unshielded, a cancelled commit released it while the agent + kept writing. The shield sits there rather than in the command runner deliberately: the thing + that holds the lock is the thing that must outlive the child, and shielding in the runner also + covered `remoteState`'s ungated reads, stranding connection slots on every superseded refresh. + Agent death mid-write is a known, documented residual (native has the same shape on app death); + closing it needs an operation-scoped agent-side lock with release-on-disconnect. + `RepositoryRouter.writer(for:)` falls back to a native `CLIVCSWriter` only when obtaining the writer itself fails (`VCSError.backendVersion`, a pre-upgrade agent with no write capability) — never mid-write, matching the read path's existing - fallback discipline exactly. Network commands (fetch/push/pull) forward a small allowlisted set of - auth env vars (`SSH_AUTH_SOCK`, `GIT_SSH_COMMAND`, etc.), resolved client-side via the same - `ShellEnvironment` probe a native push already depends on, rather than trusting wr-agent's own - (possibly stale, possibly launchd-minimal) inherited environment. A remote clone whose project + fallback discipline exactly. The child's environment is the app's OWN environment, sent + wholesale and applied over `env_clear()` agent-side (`StatusCommandRunner.childEnvironment` + builds the one map both paths use), not a key allowlist: wr-agent is "negotiated with, never + replaced", so its inherited environment is a snapshot of whichever app launch first spawned it, + and forwarding only `PATH` silently authored commits under that snapshot's stale + `GIT_AUTHOR_*`/`GIT_CONFIG_GLOBAL`/`HOME`. No allowlist could close that — what git and jj read + for identity, config, signing and hooks is open-ended. **That mechanism is local-only**: every + value in it (`SSH_AUTH_SOCK`, `GIT_SSH_COMMAND`, `HOME`, `XDG_CONFIG_HOME`) is a path into the + client's own filesystem, meaningless over a remote transport — how a remote host resolves + identity and auth is an open question for Phase 3, not a thing this milestone answers. Equally + local-only: the `git`/`jj` `executable` enum is not a containment boundary (argv is unvalidated, + and `git` with arbitrary argv is arbitrary code execution), which is acceptable only because the + transport is a same-user socket whose callers could spawn a shell anyway. Any Phase 3 transport + carrying this service must authenticate its peer to shell grade, not repository grade. A remote clone whose project root and workroom root are the same repository needs no special handling: `SnapshotLock`/ `JJProcessBarrier` already canonicalize and compare the two, so equal roots resolve to one lock file. Filesystem probes `CLIVCSWriter` still runs locally (`sequencerState`, `worktreeGitDir`, diff --git a/macapp/WorkroomApp/Core/AgentCommandRunner.swift b/macapp/WorkroomApp/Core/AgentCommandRunner.swift index 8d69b96a2..508ba29cc 100644 --- a/macapp/WorkroomApp/Core/AgentCommandRunner.swift +++ b/macapp/WorkroomApp/Core/AgentCommandRunner.swift @@ -5,9 +5,9 @@ import Foundation /// building, stdout/stderr parsing and failure classification are untouched Swift, and see exactly /// the bytes a host-executed command produced — see `vcs.rs`'s `ExecRequest` doc for the design. /// -/// Never throws: `StatusCommandRunning.run` doesn't, so any transport or decode failure here -/// degrades to `CommandResult.launchFailed`, the same fact `StatusCommandRunner` reports when -/// `Process.run()` itself fails — `CLIVCSWriter.classify`/`.classifyCommit` already handle it. +/// Never throws: `StatusCommandRunning.run` doesn't, so every failure here becomes a +/// `CommandResult` the existing classifiers already understand. Which one is NOT uniform, and the +/// distinction is load-bearing — see `neverRan` and `outcomeUnknown`. struct AgentCommandRunner: StatusCommandRunning, Sendable { let connection: AgentVCSConnection @@ -34,42 +34,105 @@ struct AgentCommandRunner: StatusCommandRunning, Sendable { _ executable: String, _ args: [String], in directory: String, timeout: TimeInterval, stdin: Data?, network: Bool ) async -> CommandResult { - // Always forwarded, network or not: wr-agent is a long-lived daemon "negotiated with, never - // replaced" (`LocalAgentVCS`), so its own inherited PATH at spawn time can predate a tool - // install and is never refreshed — without this, a write that would succeed natively could - // fail to find `git`/`jj` through an old agent. - var env = ["PATH": ShellEnvironment.path()] - if network { env.merge(Self.networkEnv()) { _, replacement in replacement } } + // The WHOLE environment native would use, not a key allowlist. wr-agent `env_clear()`s and + // adopts this map, so the child sees exactly what a native child sees. An allowlist could not + // work here: wr-agent is a long-lived daemon "negotiated with, never replaced" + // (`LocalAgentVCS`), so its own inherited environment is a snapshot of whichever app launch + // first spawned it, and the set of variables `git`/`jj` read for identity, config, signing and + // hooks is open-ended (`GIT_AUTHOR_*`, `GIT_CONFIG_GLOBAL`, `GIT_CONFIG_PARAMETERS`, `HOME`, + // `JJ_CONFIG`, `EMAIL`, `GNUPGHOME`, …). Forwarding only `PATH` silently authored commits under + // the daemon's stale identity — see `StatusCommandRunner.childEnvironment`'s doc. + // Latin-1, not UTF-8: a pathspec payload is NUL-separated and paths are not guaranteed valid + // UTF-8, and Latin-1 is a total bijection over every byte 0x00-0xFF, so this never fails and + // `vcs.rs`'s `latin1_bytes` is its exact inverse — see `ExecRequest.stdin`'s doc. Far more + // compact on the wire than a JSON byte-number array for ordinary text. + var payload: String? + if let stdin { + // Unreachable (Latin-1 decodes every byte), but the old `?? ""` fallback would have staged + // nothing or committed an empty message. Refuse rather than send a silent lie. + guard let encoded = String(data: stdin, encoding: .isoLatin1) else { + return Self.neverRan("stdin payload is not representable on the wire") + } + payload = encoded + } let request = AgentExecRequest( executable: executable, args: args, dir: directory, timeoutMs: Int((timeout * 1000).rounded(.up)), - // Latin-1, not UTF-8: a pathspec payload is NUL-separated and paths are not guaranteed - // valid UTF-8, and Latin-1 is a total bijection over every byte 0x00-0xFF, so this never - // fails and `vcs.rs`'s `latin1_bytes` is its exact inverse — see `ExecRequest.stdin`'s doc. - // Far more compact on the wire than a JSON byte-number array for ordinary text. - stdin: stdin.map { String(data: $0, encoding: .isoLatin1) ?? "" }, - env: env) + stdin: payload, + env: StatusCommandRunner.childEnvironment(network: network)) + let reply: Data do { // Slack above the command's own timeout: the round trip and the agent's own bookkeeping - // must not race the command's own timeout into a spurious `.launchFailed`. - let reply = try await connection.request(request, timeout: timeout + 15) + // must not race the command's own timeout. + // + // Cancellable on purpose. The write path's protection against a cancelled command releasing + // the JJ barrier early lives in `JJSnapshotGate.run`, which shields the whole gated operation + // once it holds the flock — see the comment there. Shielding HERE instead would also cover + // `remoteState`'s ungated reads, and a superseded `RemoteStateModel` refresh would then squat + // one of this connection's 32 shared slots until the agent answered. + reply = try await connection.request(request, timeout: timeout + 15) + } catch let error as VCSError { + // Raised before anything left this process — today only the 1 MiB single-envelope request + // ceiling. Nothing ran. + return Self.neverRan("\(error)") + } catch HostConnectionError.notDispatched { + // Refused locally with nothing written to the socket (closed connection, exhausted stream + // counter, full request pool). Definitively never ran. + return Self.neverRan(HostConnectionError.notDispatched.localizedDescription) + } catch { + // The request reached the socket and no reply came back: connection loss, the client-side + // deadline, or cancellation. The command may have completed host-side. + return Self.outcomeUnknown(error) + } + do { let result = try AgentVCSReply.decode(reply) return CommandResult( stdout: result.stdout, stderr: result.stderr, exitCode: result.exitCode, timedOut: result.timedOut, signaled: result.signaled) } catch { - return CommandResult( - stdout: "", stderr: "\(error)", exitCode: CommandResult.launchFailed, timedOut: false) + // A reply ARRIVED carrying an error. wr-agent answers one only for a request it refused + // before spawning (bad version, unusable `dir`, non-Latin-1 stdin) or for a reply too large + // to send — see `neverRan`'s caveat on that last case. + return Self.neverRan("\(error)") } } - /// The same forwarded-auth-key computation `StatusCommandRunner.networkEnvironment` does for a - /// native network command, as a standalone delta rather than a merged base environment: the - /// agent process's OWN inherited environment may predate this request and carry a stale or - /// absent `SSH_AUTH_SOCK` (see `ShellEnvironment`'s whole reason for existing), so the relevant - /// keys are resolved here, in the caller's process, and sent rather than re-derived agent-side. - private static func networkEnv() -> [String: String] { - StatusCommandRunner.networkEnvironment(base: [:], probed: ShellEnvironment.environment()) + /// The command never started. `CommandResult.launchFailed`'s own doc is strict about this value: + /// it means "nothing ran", which is a different fact from 127 ("env ran and searched PATH"), and + /// `CLIVCSWriter.classify` checks it before everything else to reach `.launchFailed`. + /// + /// Caveat: `vcs.rs`'s `send` replaces an over-16-MiB reply with a `PartialData` error, so a + /// command that ran and produced enormous output lands here too. That is the one remaining + /// misreport in this direction and is tracked separately from this fix. + static func neverRan(_ reason: String) -> CommandResult { + CommandResult( + stdout: "", stderr: reason, exitCode: CommandResult.launchFailed, timedOut: false) + } + + /// The command may or may not have completed; we stopped listening. Reported as a SIGTERM-signaled + /// result rather than `launchFailed`, because `launchFailed` asserts a falsehood: a cancelled or + /// disconnected `git push` DID run host-side (there is no cancel message in the protocol), and + /// telling the user it never launched invites a retry that double-applies it. + /// + /// It classifies as `.other(reason)`, carrying the underlying error's own description — + /// `HostConnectionError.connectionLost`'s is already exactly right ("An operation may have + /// completed; refresh before retrying"), and used to be discarded. Not the `"\(tool) was + /// interrupted"` branch: that one requires EMPTY stderr, and the message is worth more here. + /// `signaled: true` is therefore descriptive rather than load-bearing, and `timedOut` stays false + /// so this can never be mistaken for a command that ran and exceeded its own deadline. + /// + /// KNOWN GAP: `.other` is retryable (`VCSSyncPresentation.retryAction`), so the user is still + /// offered a Retry for an operation that may already have landed. Telling the truth in the + /// message is strictly better than the old `launchFailed` ("never ran"), but suppressing the + /// button needs its own `VCSRemoteFailure`/`VCSCommitFailure` case — that switch is exhaustive on + /// purpose, and adding a case is a user-visible taxonomy change, not a drive-by fix. + static func outcomeUnknown(_ error: Error) -> CommandResult { + let reason = + (error as? LocalizedError)?.errorDescription + ?? (error is CancellationError ? "The operation was cancelled; it may have completed." : nil) + ?? "\(error)" + return CommandResult( + stdout: "", stderr: reason, exitCode: 15, timedOut: false, signaled: true) } } diff --git a/macapp/WorkroomApp/Core/AgentVCSConnection.swift b/macapp/WorkroomApp/Core/AgentVCSConnection.swift index 746e3a878..c344d176a 100644 --- a/macapp/WorkroomApp/Core/AgentVCSConnection.swift +++ b/macapp/WorkroomApp/Core/AgentVCSConnection.swift @@ -158,20 +158,29 @@ final class AgentVCSConnection: HostServiceConnection, @unchecked Sendable { let cancellation = RequestCancellationBox() return try await withTaskCancellationHandler { try await withCheckedThrowingContinuation { continuation in - let stream: UInt32? = lock.withLock { + // Two refusals, two errors. Both happen before a byte reaches the socket, but they must + // NOT collapse: `connect()` issues its own `capabilities` request through here, and + // `LocalAgentVCS` catches exactly `.connectionLost` from it to spawn the agent and retry — + // the stale-`session.sock` case, which is common, not exotic. Reporting a closed connection + // as anything else would silently stop the agent ever being started. + let refusal: (stream: UInt32?, error: HostConnectionError) = lock.withLock { + if closed { return (nil, .connectionLost) } // ponytail: one 32-slot pool shared by every read AND write on this connection, app-wide. // A write can now legitimately hold a slot for minutes (`commitTimeout` = 600s), where only - // reads (seconds at most) used to compete for these slots. Exhausting the pool degrades to - // `.connectionLost` for the next caller rather than a distinct backpressure signal. Split - // reads and writes onto separate pools/connections if this is ever observed in practice. - guard !closed, nextStream < UInt32.max, pending.count < 32 else { return nil } + // reads (seconds at most) used to compete for these slots. Split reads and writes onto + // separate pools/connections if this is ever observed in practice. + // + // `.notDispatched` rather than `.connectionLost`: backpressure is not a lost connection, + // and a WRITE caller can say "this definitely did not run" — the difference between a safe + // retry and one that double-applies a commit or a push. See `AgentCommandRunner.neverRan`. + guard nextStream < UInt32.max, pending.count < 32 else { return (nil, .notDispatched) } let stream = nextStream nextStream += 1 pending[stream] = Pending(continuation: continuation) - return stream + return (stream, .notDispatched) } - guard let stream else { - continuation.resume(throwing: HostConnectionError.connectionLost) + guard let stream = refusal.stream else { + continuation.resume(throwing: refusal.error) return } // Registered only after `stream` is in `pending`, so an already-cancelled caller (Swift diff --git a/macapp/WorkroomApp/Core/HostConnectionManager.swift b/macapp/WorkroomApp/Core/HostConnectionManager.swift index 8156b10ed..684dcd871 100644 --- a/macapp/WorkroomApp/Core/HostConnectionManager.swift +++ b/macapp/WorkroomApp/Core/HostConnectionManager.swift @@ -12,6 +12,11 @@ protocol HostServiceConnection: Sendable { enum HostConnectionError: Error, Equatable, Sendable, LocalizedError, CustomStringConvertible { case connectionLost + /// Refused locally, before a single byte reached the socket — the connection was already closed, + /// the stream counter is exhausted, or the 32-slot request pool is full. Distinct from + /// `connectionLost` because the caller can say "this definitely did not run", which for a WRITE + /// is the difference between a safe retry and one that double-applies a commit or a push. + case notDispatched case staleGeneration case mismatchedContext case serviceUnavailable(String) @@ -22,6 +27,8 @@ enum HostConnectionError: Error, Equatable, Sendable, LocalizedError, CustomStri switch self { case .connectionLost: return "Host connection lost. An operation may have completed; refresh before retrying." + case .notDispatched: + return "Host service is busy; the request was not sent." case .staleGeneration: return "Host connection changed. Refresh repository data before retrying." case .mismatchedContext: diff --git a/macapp/WorkroomApp/Core/JJSnapshotGate.swift b/macapp/WorkroomApp/Core/JJSnapshotGate.swift index 38caa88c3..9d3e47cca 100644 --- a/macapp/WorkroomApp/Core/JJSnapshotGate.swift +++ b/macapp/WorkroomApp/Core/JJSnapshotGate.swift @@ -94,7 +94,21 @@ actor JJSnapshotGate { let barrier = try await runBlocking { try JJProcessBarrier.acquire(repository) } defer { barrier?.release() } try Task.checkCancellation() - return try await operation() + // Shielded once the barrier is held. This is what makes the doc above — "a started native + // operation retains this tail until its actual completion" — true for EVERY backend rather + // than only by accident. It held for a native writer because a `Process` inside `runBlocking` + // cannot observe Swift cancellation and simply runs on. An agent-routed write does observe + // it: cancelling returned immediately, `defer` released the flock, and wr-agent's `jj commit` + // kept running with the barrier gone — letting another instance or a read-side snapshot enter + // the operation this gate exists to protect. + // + // Deliberately here and not in `AgentCommandRunner`: the thing that holds the lock is the + // thing that must outlive the child. Shielding inside the runner instead covered every + // command it forwards, including `remoteState`'s ungated reads, so a superseded refresh + // squatted one of the connection's 32 shared slots until the agent answered. + return try await Task.detached(priority: Task.currentPriority) { + try await operation() + }.value } tails[repository] = Task { _ = try? await task.value } return try await withTaskCancellationHandler { diff --git a/macapp/WorkroomApp/Core/StatusCommandRunner.swift b/macapp/WorkroomApp/Core/StatusCommandRunner.swift index ecec2a7b6..143f9ca44 100644 --- a/macapp/WorkroomApp/Core/StatusCommandRunner.swift +++ b/macapp/WorkroomApp/Core/StatusCommandRunner.swift @@ -174,6 +174,65 @@ struct StatusCommandRunner: StatusCommandRunning, Sendable { return env } + /// The complete environment a `git`/`jj` child gets, native or agent-routed. + /// + /// Extracted from `run` and made static so `AgentCommandRunner` can send the SAME map to + /// wr-agent, which `env_clear()`s and replaces its own environment with it. Before this existed + /// the agent forwarded a small allowlist (`PATH`, plus auth keys on a network command) and the + /// child inherited everything else from the DAEMON — a process "negotiated with, never replaced" + /// (`LocalAgentVCS`) whose environment is a snapshot of whichever app launch first spawned it. + /// That silently authored commits under a stale `GIT_AUTHOR_*`/`GIT_CONFIG_GLOBAL`/`HOME`, and no + /// allowlist could fix it: the set of variables git and jj read for identity, config, signing and + /// hooks is open-ended (`GIT_CONFIG_PARAMETERS`, `JJ_CONFIG`, `EMAIL`, `GNUPGHOME`, …). Sending + /// the whole environment is the only construction under which the two paths agree. + /// + /// Pure, so `StatusCommandRunnerEnvironmentTests` can assert native/agent parity without spawning. + static func childEnvironment(network: Bool) -> [String: String] { + var env = ProcessInfo.processInfo.environment + env["PATH"] = ShellEnvironment.path() + env["GIT_OPTIONAL_LOCKS"] = "0" + env["GIT_TERMINAL_PROMPT"] = "0" + // Pin the message locale: EVERY consumer of this runner classifies failures by matching English + // substrings of git's stderr (`CLIVCSWriter.classify`, `WorkroomStatusResolver`), and git ships + // translated message catalogs. Homebrew git 2.55 under a French locale answers + // `erreur : le spécificateur de chemin …`, so without this a non-English user loses the ENTIRE + // failure taxonomy at once — auth, host-key, rejected-push, dirty-tree and leftover-lock all + // collapse to `.other(rawStderr)` with no recovery offered. + // + // `LC_ALL`, not `LC_MESSAGES`: this env is seeded from `ProcessInfo.environment`, so the user's own + // `LC_ALL` may be inherited, and it OUTRANKS `LC_MESSAGES` — setting the narrower variable is + // silently defeated (measured: `LC_ALL=fr_FR.UTF-8 LC_MESSAGES=C git …` still answers in French). + // + // Safe for paths despite forcing the C charset: git writes pathnames as raw bytes (and quotes + // non-ASCII per `core.quotePath` regardless of locale), so `LC_ALL=C` renders a `café-ünï.txt` + // byte-identically to the user's own locale — verified, not assumed. jj and gh are unaffected + // either way; neither localizes. + env["LC_ALL"] = "C" + // A workroom can be a clone of an *untrusted* repo, and the status sweep runs git automatically + // on load/focus/selection. `git diff` would otherwise run an inherited external-diff program; + // unset it so only the explicit `--no-ext-diff` flag (see WorkroomStatusResolver) governs diffs. + env.removeValue(forKey: "GIT_EXTERNAL_DIFF") + // Repository isolation: these OUTRANK the process's working directory, so an inherited + // `GIT_DIR`/`GIT_WORK_TREE` silently redirects a command aimed at one repository into another — + // a commit requested in workroom A landing in repo B, reported as success. Every caller here + // addresses a repository by `directory`, never by these, so inheriting them can only ever be + // wrong. A user who exports them in their shell (worktree tooling commonly does) would + // otherwise poison every command the app runs. + // + // Removed for the NATIVE path too, not just the agent's. wr-agent already stripped them; the + // app did not, so the two paths disagreed and the app was the unsafe one. `GIT_INDEX_FILE` is + // on the list deliberately: its one planned consumer is temp-index commits, which will pass it + // per-call rather than inherit it (see `StatusCommandRunning.run(stdin:)`'s doc). + for key in [ + "GIT_DIR", "GIT_WORK_TREE", "GIT_INDEX_FILE", "GIT_COMMON_DIR", "GIT_OBJECT_DIRECTORY", + "GIT_ALTERNATE_OBJECT_DIRECTORIES", + ] { + env.removeValue(forKey: key) + } + if network { env = Self.networkEnvironment(base: env, probed: ShellEnvironment.environment()) } + return env + } + func run(_ executable: String, _ args: [String], in directory: String, timeout: TimeInterval) async -> CommandResult { @@ -203,32 +262,7 @@ struct StatusCommandRunner: StatusCommandRunning, Sendable { proc.arguments = [executable] + args proc.currentDirectoryURL = URL(fileURLWithPath: directory) - var env = ProcessInfo.processInfo.environment - env["PATH"] = ShellEnvironment.path() - env["GIT_OPTIONAL_LOCKS"] = "0" - env["GIT_TERMINAL_PROMPT"] = "0" - // Pin the message locale: EVERY consumer of this runner classifies failures by matching English - // substrings of git's stderr (`CLIVCSWriter.classify`, `WorkroomStatusResolver`), and git ships - // translated message catalogs. Homebrew git 2.55 under a French locale answers - // `erreur : le spécificateur de chemin …`, so without this a non-English user loses the ENTIRE - // failure taxonomy at once — auth, host-key, rejected-push, dirty-tree and leftover-lock all - // collapse to `.other(rawStderr)` with no recovery offered. - // - // `LC_ALL`, not `LC_MESSAGES`: this env is seeded from `ProcessInfo.environment`, so the user's own - // `LC_ALL` may be inherited, and it OUTRANKS `LC_MESSAGES` — setting the narrower variable is - // silently defeated (measured: `LC_ALL=fr_FR.UTF-8 LC_MESSAGES=C git …` still answers in French). - // - // Safe for paths despite forcing the C charset: git writes pathnames as raw bytes (and quotes - // non-ASCII per `core.quotePath` regardless of locale), so `LC_ALL=C` renders a `café-ünï.txt` - // byte-identically to the user's own locale — verified, not assumed. jj and gh are unaffected - // either way; neither localizes. - env["LC_ALL"] = "C" - // A workroom can be a clone of an *untrusted* repo, and the status sweep runs git automatically - // on load/focus/selection. `git diff` would otherwise run an inherited external-diff program; - // unset it so only the explicit `--no-ext-diff` flag (see WorkroomStatusResolver) governs diffs. - env.removeValue(forKey: "GIT_EXTERNAL_DIFF") - if network { env = Self.networkEnvironment(base: env, probed: ShellEnvironment.environment()) } - proc.environment = env + proc.environment = Self.childEnvironment(network: network) let outPipe = Pipe() let errPipe = Pipe() diff --git a/macapp/WorkroomAppTests/AgentVCSProtocolTests.swift b/macapp/WorkroomAppTests/AgentVCSProtocolTests.swift index 431da4c84..6cfdffcab 100644 --- a/macapp/WorkroomAppTests/AgentVCSProtocolTests.swift +++ b/macapp/WorkroomAppTests/AgentVCSProtocolTests.swift @@ -37,4 +37,70 @@ final class AgentVCSProtocolTests: XCTestCase { } } } + + /// `launchFailed` means "the command never ran", and `CLIVCSWriter.classify` checks it before + /// everything else. A transport failure does NOT know that: there is no cancel message in the + /// protocol, so a cancelled or disconnected `git push` runs to completion host-side. Reporting it + /// as `launchFailed` told the user nothing had happened and invited a retry that double-applies. + func testTransportFailureIsNotReportedAsNeverHavingRun() { + let lost = AgentCommandRunner.outcomeUnknown(HostConnectionError.connectionLost) + XCTAssertNotEqual(lost.exitCode, CommandResult.launchFailed) + XCTAssertTrue(lost.signaled) + // Not `timedOut`: that offers Retry, and nobody knows the outcome. + XCTAssertFalse(lost.timedOut) + // The existing error already says the right thing; it used to be discarded. + XCTAssertTrue(lost.stderr.contains("may have completed")) + + let cancelled = AgentCommandRunner.outcomeUnknown(CancellationError()) + XCTAssertNotEqual(cancelled.exitCode, CommandResult.launchFailed) + XCTAssertTrue(cancelled.signaled) + XCTAssertTrue(cancelled.stderr.contains("may have completed")) + + // A request refused before it left this process genuinely never ran, and keeps the old value. + XCTAssertEqual( + AgentCommandRunner.neverRan("too large").exitCode, CommandResult.launchFailed) + } + + /// Both paths must hand the child the SAME environment. Forwarding only `PATH` let the child + /// inherit the rest from a daemon "negotiated with, never replaced", so an agent-routed commit + /// could be authored under the identity of whichever app launch first spawned it. + func testAgentAndNativeSendTheSameChildEnvironment() { + for network in [false, true] { + let env = StatusCommandRunner.childEnvironment(network: network) + // The subprocess baseline every consumer's stderr classification depends on. + XCTAssertEqual(env["LC_ALL"], "C") + XCTAssertEqual(env["GIT_TERMINAL_PROMPT"], "0") + XCTAssertEqual(env["GIT_OPTIONAL_LOCKS"], "0") + XCTAssertNil(env["GIT_EXTERNAL_DIFF"]) + XCTAssertEqual(env["PATH"], ShellEnvironment.path()) + // Identity/config carriers reach the child on EVERY command, not just network ones. + for key in ["HOME", "USER"] where ProcessInfo.processInfo.environment[key] != nil { + XCTAssertEqual(env[key], ProcessInfo.processInfo.environment[key]) + } + } + // The fail-fast ssh invariant is still network-only. + XCTAssertNil(StatusCommandRunner.childEnvironment(network: false)["SSH_ASKPASS_REQUIRE"]) + XCTAssertEqual( + StatusCommandRunner.childEnvironment(network: true)["SSH_ASKPASS_REQUIRE"], "never") + } + + /// These OUTRANK the working directory, so an inherited one silently redirects a command aimed at + /// one repository into another — a commit requested in workroom A landing in repo B, reported as + /// success. Every caller addresses a repository by `directory`, so inheriting them is only ever + /// wrong. Asserted for both paths because they now share one environment: sending the app's whole + /// environment to the agent would otherwise have carried these across too. + func testRepositoryOverridesNeverReachTheChild() { + let keys = [ + "GIT_DIR", "GIT_WORK_TREE", "GIT_INDEX_FILE", "GIT_COMMON_DIR", "GIT_OBJECT_DIRECTORY", + "GIT_ALTERNATE_OBJECT_DIRECTORIES", + ] + for key in keys { setenv(key, "/tmp/some-other-repo", 1) } + defer { for key in keys { unsetenv(key) } } + for network in [false, true] { + let env = StatusCommandRunner.childEnvironment(network: network) + for key in keys { + XCTAssertNil(env[key], "\(key) reached the child (network: \(network))") + } + } + } } diff --git a/vcs/crates/wr-agent/src/vcs.rs b/vcs/crates/wr-agent/src/vcs.rs index c320022da..816680a44 100644 --- a/vcs/crates/wr-agent/src/vcs.rs +++ b/vcs/crates/wr-agent/src/vcs.rs @@ -13,7 +13,7 @@ use std::path::{Path, PathBuf}; use std::process::{Command, Stdio}; use std::sync::{ atomic::{AtomicUsize, Ordering}, - Arc, + Arc, Mutex, }; use std::time::{Duration, Instant}; use wr_vcs_model::{self as model, VcsError}; @@ -26,23 +26,6 @@ const MAX_HISTORY_LIMIT: usize = 10000; const MAX_EXEC_TIMEOUT_MS: u64 = 610_000; /// Per-stream cap, matching `StatusCommandRunner.maxBytes`'s default on the Swift side. const MAX_EXEC_STREAM: usize = 4 * 1024 * 1024; -/// Env vars an exec request may forward to the child, resolved by the client rather than re-derived -/// here — the agent's own inherited environment may predate the request and carry a stale value -/// (see `AgentCommandRunner.swift`). `PATH` is always sent, from the SAME `ShellEnvironment.path()` -/// the native writer uses via `/usr/bin/env`: wr-agent is a long-lived daemon that "is negotiated -/// with, never replaced" (`LocalAgentVCS`), so its own inherited PATH at spawn time can predate a -/// tool install and is never refreshed; without this, a write that would succeed natively could fail -/// to find `git`/`jj` through an old agent. The rest is `StatusCommandRunner.networkEnvironment`'s -/// set, sent only for a network write. Anything else in the request's `env` map is silently dropped. -const ALLOWED_EXEC_ENV_KEYS: &[&str] = &[ - "PATH", - "SSH_AUTH_SOCK", - "SSH_AGENT_PID", - "GIT_SSH_COMMAND", - "GIT_CONFIG_GLOBAL", - "XDG_CONFIG_HOME", - "SSH_ASKPASS_REQUIRE", -]; static ACTIVE: AtomicUsize = AtomicUsize::new(0); /// Whether any dispatched VCS request — including a JJ snapshot that owns the working-copy lock @@ -86,14 +69,44 @@ enum Backend { /// and failure-classification decision (`CLIVCSWriter.classify`/`.classifyCommit`, the pathspec /// and refspec injection defenses, the retry/abort taxonomy) stays in Swift and sees exactly the /// bytes it would see from a local `git`/`jj` — this is deliberately NOT a reimplementation of -/// commit/push/pull semantics, so criterion 2 ("typed failures... match pre-agent behavior -/// exactly") holds by construction rather than by keeping two classifiers in sync. +/// commit/push/pull semantics. +/// +/// One untouched classifier is necessary for criterion 2 ("typed failures... match pre-agent +/// behavior exactly") and was not sufficient: a review found it reached OPPOSITE verdicts because +/// the two paths fed it different input. Identical output needs three things, all now true — +/// the child sees the same environment (`env_clear` below), a missing tool exits 127 rather than +/// failing to spawn (`/usr/bin/env`), and a transport failure is not reported as "never ran" +/// (`AgentCommandRunner.outcomeUnknown`). One documented gap remains: an outcome-unknown result is +/// still offered a Retry, because suppressing it needs a new `VCSRemoteFailure` case. /// /// **Never acquires `SnapshotLock`.** A caller that needs the JJ working-copy barrier for a /// mutating command already holds it — `CLIVCSWriter`'s `gate: JJSnapshotGate` takes the same /// `/.jj/workroom-vcs.lock` flock (`JJProcessBarrier`, shared by name with `SnapshotLock` /// above) for the whole gated operation before any exec request goes out. Taking it again here /// would self-deadlock the same actor for 30s and then fail as `LockContention`. +/// +/// That argument holds only while the CLIENT's lock outlives this child, which is why +/// `JJSnapshotGate.run` shields the whole gated operation from task cancellation once it holds the +/// flock: unshielded, a cancelled commit returned at once and released it while this `jj commit` +/// kept running, letting another instance or a read-side snapshot enter the supposedly protected +/// operation. Known residual: if the AGENT dies mid-write the gate is released with the child +/// potentially still alive. Native has the same shape on app death. Closing it needs an +/// operation-scoped lock here with release-on-disconnect, which is a protocol change, not a patch. +/// +/// **The child's environment is the request's `env`, wholesale** (`env_clear` in `run_exec`), never +/// this daemon's own. wr-agent is "negotiated with, never replaced", so its inherited environment is +/// a snapshot of whichever app launch first spawned it; an agent-routed commit was picking up that +/// snapshot's `GIT_AUTHOR_*`/`GIT_CONFIG_GLOBAL`/`HOME` and authoring under a stale identity. A key +/// allowlist could not fix it — what git and jj read for identity, config, signing and hooks is +/// open-ended — so `StatusCommandRunner.childEnvironment` builds one map for both paths. +/// +/// **`args` is unvalidated and the `git`/`jj` `executable` enum is NOT a containment boundary.** +/// `git` with arbitrary argv is arbitrary code execution (`-c alias.x='!…'`, `-c core.hooksPath=…`, +/// `--exec-path`), the request's `PATH` decides which binary resolves, and `GIT_SSH_COMMAND` and the +/// config paths in `env` are further routes. That is acceptable only because the sole transport +/// today is a same-user unix socket, where every reachable caller could already spawn a shell +/// itself. Any transport that is not that — Phase 3's remote relay above all — must authenticate its +/// peer to SHELL grade, not repository grade, before carrying this service. #[derive(Deserialize)] #[serde(deny_unknown_fields)] struct ExecRequest { @@ -290,20 +303,26 @@ struct Captured { /// Drain a pipe to EOF on its own thread (mirrors `StatusCommandRunner.readCapped`): a blocking /// read is safe here because it runs off the thread that watches for timeout/exit, and keeps /// draining past `cap` so the child can never block on a full pipe buffer. -fn drain_capped(mut source: impl Read, cap: usize) -> Vec { - let mut collected = Vec::new(); +/// +/// Publishes into a shared sink rather than returning, so `run_exec` can take what has drained so +/// far WITHOUT joining this thread. That matters because this thread can block forever: a +/// descendant that inherited the pipe (an ssh `ControlPersist` master, a daemonising credential +/// helper, gpg-agent auto-launched by `commit -S`) holds the write end open after the child is +/// reaped, and `setsid` puts it outside the process group `run_exec` signals. +fn drain_capped(mut source: impl Read, cap: usize, sink: &Mutex>) { let mut chunk = [0u8; 65536]; loop { match source.read(&mut chunk) { - Ok(0) => return collected, + Ok(0) => return, Ok(count) => { + let mut collected = sink.lock().unwrap_or_else(|e| e.into_inner()); if collected.len() < cap { let take = (cap - collected.len()).min(count); collected.extend_from_slice(&chunk[..take]); } } Err(error) if error.kind() == std::io::ErrorKind::Interrupted => continue, - Err(_) => return collected, + Err(_) => return, } } } @@ -321,8 +340,19 @@ fn run_exec( stdin: Option<&[u8]>, env: &[(&str, &str)], ) -> model::Result { - let mut command = Command::new(executable); + // `/usr/bin/env `, byte-for-byte what native does + // (`StatusCommandRunner.run`: `proc.executableURL = /usr/bin/env`). Not a style choice — it is + // what makes a MISSING tool exit 127 (`env` ran and searched PATH) instead of failing to spawn. + // Spawning the program directly made that an Io error, which the client reported as + // `launchFailed`, the value whose own doc reserves it for "nothing ran" and warns it would + // "misdiagnose a deleted workroom as a missing git/jj/gh". Mapping `ErrorKind::NotFound` to 127 + // by hand could not fix it either: `posix_spawn` returns ENOENT for a missing cwd AND a missing + // executable, so the two stay indistinguishable. Letting `env` do the lookup makes 127/126 its + // real exit codes, and narrows a spawn failure here to the cases that genuinely never ran — a + // vanished cwd, or `/usr/bin/env` itself being absent. + let mut command = Command::new("/usr/bin/env"); command + .arg(executable) .current_dir(dir) .args(args) .stdin(if stdin.is_some() { @@ -333,28 +363,17 @@ fn run_exec( .stdout(Stdio::piped()) .stderr(Stdio::piped()) .process_group(0) - // Same baseline as every other subprocess in this codebase (`StatusCommandRunner.run`, - // `wr_vcs_git::diff::run_bounded`): disable git's own lock-contention retries and terminal - // prompting, and pin the message locale so Swift's stderr-substring classifiers keep working. - .env("GIT_OPTIONAL_LOCKS", "0") - .env("GIT_TERMINAL_PROMPT", "0") - .env("LC_ALL", "C") - // A workroom can be a clone of an untrusted repo; never run an inherited external-diff/askpass - // helper or reach for a graphical prompt this headless agent has no display for. - .env_remove("GIT_EXTERNAL_DIFF") - .env_remove("SSH_ASKPASS") - .env_remove("DISPLAY") + // `env_clear` first, then adopt the request's map wholesale: the child must see EXACTLY the + // environment `StatusCommandRunner.childEnvironment` built in the app, never this daemon's + // own. wr-agent is "negotiated with, never replaced", so its inherited environment is a + // snapshot of whichever app launch first spawned it — an agent-routed commit was picking up + // that snapshot's `GIT_AUTHOR_*`/`GIT_CONFIG_GLOBAL`/`HOME` and authoring under a stale + // identity. A key allowlist could not fix that: what git and jj read for identity, config, + // signing and hooks is open-ended. That one function also applies this codebase's subprocess + // baseline (`GIT_OPTIONAL_LOCKS`, `GIT_TERMINAL_PROMPT`, `LC_ALL=C`, and the removal of + // `GIT_EXTERNAL_DIFF` and the `GIT_DIR` family) so both paths get it from one place. + .env_clear() .envs(env.iter().copied()); - for key in [ - "GIT_DIR", - "GIT_WORK_TREE", - "GIT_INDEX_FILE", - "GIT_COMMON_DIR", - "GIT_OBJECT_DIRECTORY", - "GIT_ALTERNATE_OBJECT_DIRECTORIES", - ] { - command.env_remove(key); - } let mut child = command.spawn().map_err(io)?; if let Some(payload) = stdin { @@ -370,13 +389,22 @@ fn run_exec( let stdout_pipe = child.stdout.take().expect("piped stdout"); let stderr_pipe = child.stderr.take().expect("piped stderr"); - let out = std::thread::spawn(move || drain_capped(stdout_pipe, MAX_EXEC_STREAM)); - let err = std::thread::spawn(move || drain_capped(stderr_pipe, MAX_EXEC_STREAM)); + let out_sink = Arc::new(Mutex::new(Vec::new())); + let err_sink = Arc::new(Mutex::new(Vec::new())); + let out = { + let sink = Arc::clone(&out_sink); + std::thread::spawn(move || drain_capped(stdout_pipe, MAX_EXEC_STREAM, &sink)) + }; + let err = { + let sink = Arc::clone(&err_sink); + std::thread::spawn(move || drain_capped(stderr_pipe, MAX_EXEC_STREAM, &sink)) + }; let pid = child.id() as i32; let start = Instant::now(); let mut timed_out = false; let mut sent_term = false; + let mut sent_kill = false; let status = 'wait: loop { if let Ok(Some(status)) = child.try_wait() { break 'wait status; @@ -388,34 +416,68 @@ fn run_exec( sent_term = true; unsafe { libc::kill(-pid, libc::SIGTERM) }; } else if elapsed >= timeout + Duration::from_secs(2) { + sent_kill = true; unsafe { libc::kill(-pid, libc::SIGKILL) }; break 'wait child.wait().map_err(io)?; } } std::thread::sleep(Duration::from_millis(10)); }; - let stdout = out.join().map_err(|_| io("stdout reader panicked"))?; - let stderr = err.join().map_err(|_| io("stderr reader panicked"))?; + // Two independent jobs, deliberately not fused. Fusing them (escalate only while a reader is + // still running) let a timed-out descendant survive: if it redirects stdout/stderr, both readers + // finish, the drain loop exits, and the SIGKILL never fires — leaving it free to keep modifying + // the repository after this result releases the caller's gate. + // + // 1. Timeout escalation. The wait loop above already SIGKILLs when the LEADER outlives the + // grace; this covers the leader exiting promptly on SIGTERM while group members do not. The + // remaining grace is honoured rather than skipped — `git`/`jj` can exit on SIGTERM while a + // hook is still cleaning up, and cutting that short can leave hook-owned locks behind + // (`CLIVCSWriter.commitTimeout`: "Killing a commit is categorically more dangerous than + // killing a fetch"). Never on a normal exit: killing the group because a reader thread has + // not been scheduled yet would kill a hook's legitimate background work, measured at ~1.5% of + // successful large-output runs. + // + // Like native's `ProcessTree.killTree` at `timeout + 2`, this signals a pid that has already + // been reaped, so it carries the same (small, unavoidable) PID-reuse window. It reaches only + // the process group; a descendant that `setsid`s escapes both this and native. + if timed_out && !sent_kill { + let kill_at = start + timeout + Duration::from_secs(2); + while Instant::now() < kill_at { + std::thread::sleep(Duration::from_millis(10)); + } + unsafe { libc::kill(-pid, libc::SIGKILL) }; + } + // 2. Drain bound, mirroring `StatusCommandRunner`'s `_ = drain.wait(timeout: .now() + 2)` and + // for the reason its comment gives: a descendant that inherited the pipe can hold the write + // end open after the child is reaped, so joining unconditionally would block forever. Forever + // is not merely a hung request here — this thread is the one `dispatch` spawned, so its + // `Permit` never drops, one of the 32 `ACTIVE` slots shared with every read leaks, and + // `is_busy()` stays true so the daemon never idle-exits either. After the deadline we take + // what drained and DETACH the readers; each exits on its own when the descendant finally + // closes the pipe, writing into a sink nobody reads. That costs a thread and up to 4 MiB per + // occurrence, which is bounded per call and vastly better than leaking a permit. + let drained_by = Instant::now() + Duration::from_secs(2); + while !(out.is_finished() && err.is_finished()) && Instant::now() < drained_by { + std::thread::sleep(Duration::from_millis(10)); + } + let take = |sink: &Mutex>| { + std::mem::take(&mut *sink.lock().unwrap_or_else(|e| e.into_inner())) + }; Ok(Captured { - stdout, - stderr, + stdout: take(&out_sink), + stderr: take(&err_sink), + // Never -1: that value IS `CommandResult.launchFailed` in Swift ("nothing ran"), and a + // process that got this far demonstrably ran. `code()` is None only when signaled, and a + // signaled status always carries a signal number, so the fallback is unreachable — pinned + // to 128+SIGKILL (the shell's own convention) rather than to a Swift sentinel. exit_code: status .code() - .unwrap_or_else(|| status.signal().unwrap_or(-1)), + .unwrap_or_else(|| status.signal().unwrap_or(137)), timed_out, signaled: status.signal().is_some(), }) } -/// Keep only the env vars an exec request is allowed to set — see `ALLOWED_EXEC_ENV_KEYS`. Pure, -/// so the allowlist itself is unit-testable without spawning anything. -fn filter_exec_env(env: &std::collections::BTreeMap) -> Vec<(&str, &str)> { - env.iter() - .filter(|(key, _)| ALLOWED_EXEC_ENV_KEYS.contains(&key.as_str())) - .map(|(key, value)| (key.as_str(), value.as_str())) - .collect() -} - /// The exact inverse of `AgentCommandRunner.swift`'s `String(data:encoding:.isoLatin1)` — see /// `ExecRequest.stdin`'s doc. Every `char` a Latin-1-decoded string can contain is U+0000-U+00FF /// by construction; a value outside that range means the client sent something else, so this @@ -444,7 +506,7 @@ fn exec(request: ExecRequest) -> model::Result { } let dir = absolute(&dir)?; let timeout = Duration::from_millis(timeout_ms.clamp(1, MAX_EXEC_TIMEOUT_MS)); - let env = filter_exec_env(&env); + let env: Vec<(&str, &str)> = env.iter().map(|(k, v)| (k.as_str(), v.as_str())).collect(); let stdin = stdin.as_deref().map(latin1_bytes).transpose()?; let captured = run_exec( &dir, @@ -934,32 +996,15 @@ mod tests { assert_eq!(reply["result"]["writes"], 8); } + /// The child's environment is EXACTLY the request's map — nothing of the daemon's own leaks + /// through. This is the property that keeps an agent-routed commit from being authored under + /// the stale identity of whichever app launch first spawned this daemon; a key allowlist could + /// not provide it, because what git and jj read for identity, config, signing and hooks is + /// open-ended. `env` (not in the git/jj allowlist) goes through `run_exec` directly, bypassing + /// `exec`'s executable restriction, specifically to observe what reaches the child. #[test] - fn filter_exec_env_drops_everything_outside_the_allowlist() { - let mut env = std::collections::BTreeMap::new(); - env.insert("SSH_AUTH_SOCK".to_string(), "/tmp/agent.sock".to_string()); - env.insert( - "WORKROOM_NOT_ALLOWED".to_string(), - "should-not-pass".to_string(), - ); - // PATH is allowlisted deliberately (see `ALLOWED_EXEC_ENV_KEYS`'s doc) so an agent-routed - // write can find a `git`/`jj` install its own long-lived process predates. - env.insert("PATH".to_string(), "/usr/local/bin:/usr/bin".to_string()); - let filtered = filter_exec_env(&env); - assert_eq!( - filtered, - vec![ - ("PATH", "/usr/local/bin:/usr/bin"), - ("SSH_AUTH_SOCK", "/tmp/agent.sock") - ] - ); - } - - #[test] - fn run_exec_only_applies_the_env_pairs_it_is_given() { - // `env` (not in the git/jj allowlist) is used directly via `run_exec`, bypassing `exec`'s - // executable restriction, specifically to observe what actually reaches the child — the - // allowlist filter itself is `filter_exec_env`'s job, covered above. + fn run_exec_replaces_the_daemons_environment_with_the_requests() { + std::env::set_var("WORKROOM_DAEMON_ONLY", "stale-daemon-value"); let root = git_repo("run-exec-env"); let captured = run_exec( &root, @@ -967,12 +1012,88 @@ mod tests { &[], Duration::from_secs(5), None, - &[("SSH_AUTH_SOCK", "/tmp/allowed.sock")], + &[ + ("PATH", "/usr/local/bin:/usr/bin:/bin"), + ("SSH_AUTH_SOCK", "/tmp/allowed.sock"), + ("GIT_AUTHOR_EMAIL", "fresh@example.com"), + ], ) .unwrap(); let stdout = String::from_utf8_lossy(&captured.stdout); assert!(stdout.contains("SSH_AUTH_SOCK=/tmp/allowed.sock")); - assert!(!stdout.contains("GIT_DIR=")); + assert!(stdout.contains("GIT_AUTHOR_EMAIL=fresh@example.com")); + // The daemon's own environment must not reach the child at all. + assert!(!stdout.contains("WORKROOM_DAEMON_ONLY")); + std::env::remove_var("WORKROOM_DAEMON_ONLY"); + std::fs::remove_dir_all(&root).unwrap(); + } + + /// The regression that motivated bounding the drain: a child that exits promptly while leaving + /// a BACKGROUND descendant holding its stdout must not hold the exec thread (and with it one of + /// the 32 `ACTIVE` permits) for the descendant's lifetime. Before the bound, this returned in + /// ~4s instead of ~0s, and each occurrence leaked a permit permanently. + #[test] + fn exec_does_not_wait_on_a_descendant_that_outlives_the_child() { + let root = git_repo("run-exec-straggler"); + let started = Instant::now(); + let captured = run_exec( + &root, + "sh", + &["-c".to_string(), "sleep 5 & echo done; exit 0".to_string()], + Duration::from_secs(30), + None, + &[("PATH", "/usr/bin:/bin")], + ) + .unwrap(); + let elapsed = started.elapsed(); + assert_eq!(captured.exit_code, 0); + assert!(!captured.timed_out); + assert!(String::from_utf8_lossy(&captured.stdout).contains("done")); + assert!( + elapsed < Duration::from_secs(4), + "run_exec waited {elapsed:?} on a background descendant" + ); + std::fs::remove_dir_all(&root).unwrap(); + } + + /// Native spawns `/usr/bin/env git`, so a missing tool exits 127 and `CLIVCSWriter.classify` + /// answers `.toolMissing`. Spawning the program directly used to raise an Io error instead, + /// which the client reported as `launchFailed` — the value whose own doc reserves it for + /// "nothing ran" and warns it would "misdiagnose a deleted workroom as a missing git/jj/gh". + #[test] + fn exec_reports_a_missing_tool_as_127_not_a_service_error() { + let root = git_repo("run-exec-missing"); + let empty = root.join("empty-bin"); + std::fs::create_dir_all(&empty).unwrap(); + let captured = run_exec( + &root, + "git", + &["status".to_string()], + Duration::from_secs(5), + None, + &[("PATH", empty.to_str().unwrap())], + ) + .unwrap(); + assert_eq!(captured.exit_code, 127); + assert!(!captured.signaled); + std::fs::remove_dir_all(&root).unwrap(); + } + + /// A vanished working directory stays a service error — that IS "nothing ran", and conflating + /// it with 127 is the mirror of the bug above. + #[test] + fn exec_reports_a_vanished_directory_as_an_error_not_127() { + let root = git_repo("run-exec-gone"); + let missing = root.join("not-there"); + let failed = run_exec( + &missing, + "git", + &["status".to_string()], + Duration::from_secs(5), + None, + &[("PATH", "/usr/bin:/bin")], + ); + assert!(failed.is_err()); std::fs::remove_dir_all(&root).unwrap(); } } From 4607c5562db78a579b2b561e79dffa7026f3cbb4 Mon Sep 17 00:00:00 2001 From: Joel Moss Date: Fri, 18 Sep 2026 19:31:11 +0100 Subject: [PATCH 03/13] docs: record the agent-VCS-writes review outcome MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The design doc claimed an untouched `CLIVCSWriter` made agent-routed and native writes "classify identically by construction". A three-cycle review falsified that, so the claim is replaced rather than softened: one classifier fed divergent input reached opposite verdicts, and parity is now stated as the three properties that actually produce it (same child environment, same exit-code semantics for a missing tool, an error partition that separates "never ran" from "outcome unknown"). Criterion 2 is recorded as met with one named exception. TODOS gains the standing "findings the /review pass didn't fix" entry the two earlier VCS reviews already use: the retry affordance on an outcome-unknown write, the unbounded `child.wait()` after SIGKILL, process-group vs process-tree kill, the `writes: 8` pseudo-capability, the MAX_EXEC_STREAM / MAX_RESPONSE mismatch, the 1 MiB request ceiling, read/write pool sharing, a capabilities-negotiation disconnect missing the agent respawn, and seven test gaps. The P1s are fixed in ed51faa9; none of what remains is silent data loss. Next Steps item 3 also records the two questions this milestone opened rather than answered, both because the exec service is the thing Phase 3 moves host-side: how a remote host resolves auth (every forwarded value is a path into the CLIENT's filesystem), and that a transport carrying exec must authenticate its peer to shell grade, not repository grade — there is no code guard for that today. macapp/AGENTS.md's working rules collect the failure that produced each rule, so both instances go there: rule 1 gains the "X is unchanged, therefore behaviour is unchanged" premise, whose fix is to verify everything reaching X; rule 2 gains the three review-fix regressions (a dropped GIT_DIR scrub landing a commit in the wrong repository, a cancellation shield stranding connection slots, a SIGKILL guard that opened a never-kill path). Co-Authored-By: Claude Opus 5 (1M context) --- TODOS.md | 81 ++++++++++++++++++++++++++++++++ docs/designs/remote-workrooms.md | 48 ++++++++++++++++--- macapp/AGENTS.md | 15 ++++++ 3 files changed, 138 insertions(+), 6 deletions(-) diff --git a/TODOS.md b/TODOS.md index bd6b9c806..d5b49210d 100644 --- a/TODOS.md +++ b/TODOS.md @@ -1119,6 +1119,87 @@ deferred on its own merits. **Priority:** done. +### Agent-routed VCS writes: the findings the `/review` pass didn't fix (macapp) — issue #205 + +**What:** What the writes review (#205, PR pending) verified and left standing. The five P1s it found +are fixed in `ed51faa9`; each entry below was reproduced or read off the code, none is speculative. + +1. **An outcome-unknown write still offers Retry.** `AgentCommandRunner.outcomeUnknown` stopped + claiming a cancelled or disconnected write "never ran", but it classifies as `.other`, and + `VCSSyncPresentation.retryAction` returns `lastAction` for `.other` — so the user is still one + click from re-running a push that may already have landed. The message is honest now; the button + is not. Closing it needs a new `VCSRemoteFailure`/`VCSCommitFailure` case (that switch is + exhaustive on purpose, so adding one is a compile-time-checked change) plus a `retryAction` arm + returning nil, and a matching tier in the presenter. User-visible taxonomy change, hence deferred + rather than folded into a review fix. + +2. **`child.wait()` after SIGKILL is unbounded.** `run_exec`'s timeout path ends + `break 'wait child.wait().map_err(io)?`. `kill(-pid, …)` only reaches the process group, so a + descendant that `setsid`s away (or a wrapper that re-parents) leaves that wait blocking. Codex + reproduced 4.1s on a 100ms timeout via `os.setpgid`. The drain bound means this no longer leaks an + `ACTIVE` permit, but the wait itself has no ceiling. Predates the agent-writes work. + +3. **The agent kills a process GROUP where native kills a process TREE.** `StatusCommandRunner` + escalates through `ProcessTree.killTree`, which walks real ppid lineage via `pgrep -P` — that file + explicitly rejected group-based kill because "helpers spawned by git/gh can outlive the parent". + `run_exec` has only `kill(-pid, …)`. Same gap as (2), and the two share a fix. + +4. **`writes: 8` is not a capability.** `AgentVCSConnection.writeMethodCount` counts methods on the + client-only `LocalVCSWriting` protocol; wr-agent implements ONE generic exec service and does + nothing the number could describe. Adding a ninth method — a Swift-only change the passthrough + already supports — silently drops every user to native writes on a fully capable agent, with no + log line. Gate on the exec service's presence/version instead. (`reads: 9` is fine; those are nine + real agent-side methods.) + +5. **Two caps that can't both be satisfied.** `MAX_EXEC_STREAM` is 4 MiB per stream, so two capped + streams JSON-escaped control-heavy can exceed `MAX_RESPONSE` (16 MiB); `send` then replaces the + whole reply with `PartialData`, discarding a completed command's exit status. Codex reproduced it + with 3 MiB of `0x01`. Native truncates and still classifies. Size the wire form for worst-case + expansion, or truncate while preserving status. + +6. **The 1 MiB request cap regresses large selective commits.** `StatusCommandRunning.run(stdin:)` + exists partly to sidestep `E2BIG`; routing through a single un-chunked envelope reintroduces a + ceiling, and a lower one (each NUL escapes to 6 bytes). "Select all and commit" in a large repo + fails through the agent where it succeeds natively. Needs real request chunking. + +7. **The 32-slot request pool is shared between reads and writes.** A write can hold a slot for + `commitTimeout` (600s) where only seconds-long reads used to compete. Exhaustion now degrades to + a typed `.notDispatched`, so it is no longer misreported, but the contention is real. Split reads + and writes onto separate pools or connections if it shows up in practice. Marked `ponytail:` in + `AgentVCSConnection.request`. + +8. **A capabilities-negotiation disconnect never reaches the agent respawn.** + `AgentVCSConnection.connect` wraps every negotiation failure as + `serviceUnavailable("VCS negotiation failed: …")`, but `LocalAgentVCS` catches exactly + `connectionLost` to spawn wr-agent and retry. A connection that drops mid-`capabilities` therefore + fails without the stale-socket recovery. Predates this work. + +9. **Seven test gaps, all with stubs available.** Nothing covers: the exec timeout clamp boundaries; + the `version != 1` guard; `drain_capped`'s 4 MiB cap and its drain-past-cap property; + `latin1_bytes`' rejection branch; a wrong-but-present `writes` count (only absent is tested); + `writer(context:reader:)`'s host-mismatch and closed-connection guards; and the write path's + transport-failure mapping. Items (2), (5) and the two fixed env/127 defects all lived in exactly + this untested region, which is the argument for closing it. + +**Also worth knowing, not a defect:** `serve::tests::instance_lock_is_exclusive` flaked once under the +full parallel `cargo test -p wr-agent` run and passes in isolation and on re-run. Left alone; if it +recurs, the exec tests' spawned processes are the likely interference. + +**Why:** These are the residue of a three-cycle review (critical pass + 6 specialists + two Claude +adversarial passes + three Codex passes, one structured). Its P1s — a leaked concurrency permit that +kills the VCS service, a completed push reported as never run, a cancelled write releasing the jj +barrier, commits authored under a stale identity, and a missing tool reported as a deleted workroom — +are fixed. Nothing left here is silent data loss; (1) is the closest, and it needs a UI decision. + +**How to start:** (1) is self-contained and the most user-visible. (2)+(3) are one change in +`run_exec`. (4) is a two-line protocol simplification. (9) is mechanical and makes the rest safe to +touch. + +**Depends on / blocked by:** nothing. All of it sits on the stacked branch (#201 → #202 → #204 → +#205), so land the stack first. + +**Priority:** P2. (1) is the only one worth pulling forward. + ### VCS toolbar: the findings the `/review` pass didn't fix (macapp) **What:** Everything the toolbar review verified but left standing. Each was reproduced or read off the diff --git a/docs/designs/remote-workrooms.md b/docs/designs/remote-workrooms.md index 051e3d48d..54d17989c 100644 --- a/docs/designs/remote-workrooms.md +++ b/docs/designs/remote-workrooms.md @@ -17,10 +17,13 @@ Phase 2 preparation has landed: #184 added the VCS registry, #185 moved working (#201, #202). Agent-backed VCS reads are implemented (#204, Next Steps item 2): exact patches and line counts both come from owning-host Git, not gix, so the spike's 139/150 count drift never ships. Agent-backed VCS writes are implemented (#205, Next Steps item 3): `wr-agent` gained a -generic, allowlisted exec service, and every write's argv building, output parsing and failure -classification stayed in `CLIVCSWriter` unmodified — only the `StatusCommandRunning` conformer that -actually spawns `git`/`jj` changed, so agent-routed and native writes classify identically by -construction. File and notification services remain unbuilt. All four PRs stay draft, stacked, +generic exec service, and every write's argv building, output parsing and failure classification +stayed in `CLIVCSWriter` unmodified — only the `StatusCommandRunning` conformer that actually spawns +`git`/`jj` changed. Keeping one classifier proved necessary but not sufficient for parity: a review +found it reaching opposite verdicts on the two paths because they fed it different input (stale +environment, a different exit code for a missing tool, and transport failures reported as "never +ran"). Parity is now three explicit properties rather than an architectural claim — see Next Steps +item 3. File and notification services remain unbuilt. All four PRs stay draft, stacked, until the next stable release cuts from master. OQ1's gix measurements and OQ21's Swift driver decision are answered. The gix code remains a spike. Phases 3 and 4 — persistent remote transport, distribution, provisioning and UI — remain planned. @@ -1987,8 +1990,25 @@ independently. `StatusCommandRunner`'s own SIGTERM-then-grace-then-SIGKILL shape. `CLIVCSWriter`'s existing arg building, output parsing and failure classification are untouched — only the `StatusCommandRunning` conformer it spawns through changed (`AgentCommandRunner`, forwarding to - the new exec service instead of `Process`), so agent-routed and native writes classify identically - by construction rather than by keeping two implementations in sync. The exec service deliberately + the new exec service instead of `Process`). + + **One untouched classifier turned out to be necessary but not sufficient, and that is the main + thing this milestone learned.** The first draft argued that because `CLIVCSWriter` is untouched, + the two paths "classify identically by construction". A three-cycle review falsified it: the same + classifier reached OPPOSITE verdicts because the two paths fed it different input — which is + harder to catch than two classifiers, because nothing looks out of sync. Three defects came from + that one root, all reproduced: the child inherited the daemon's stale environment (commits + authored under the wrong identity), a missing tool failed to spawn instead of exiting 127 via + `/usr/bin/env` (a missing `git` reported as a deleted workroom), and every transport failure + became `launchFailed`, "the command never ran" (a push that completed host-side reported as never + started, inviting a retry that double-applies it). Parity needed three explicit properties, not an + architectural claim: the same child environment, the same exit-code semantics for a missing tool, + and an error partition that distinguishes "never ran" from "outcome unknown". Issue #205's + criterion 2 ("typed failures and Retry/Abort behavior match pre-agent behavior exactly") is met + with one recorded exception — an outcome-unknown result classifies as `.other` and is still + offered a Retry; suppressing that needs a new `VCSRemoteFailure` case, tracked in `TODOS.md`. + + The exec service deliberately never takes the JJ snapshot barrier itself: `CLIVCSWriter`'s `JJSnapshotGate` already holds `/.jj/workroom-vcs.lock` for the whole gated write before any exec request goes out, and wr-agent's own `SnapshotLock` locks the identical file for its read-side snapshots — acquiring it @@ -2033,6 +2053,22 @@ independently. network responses. Define hysteresis, explicit activity handling and the awake-ceiling policy before declaring the wakefulness service ready. OQ19 remains open until those results exist. + **Two Phase 3 questions this milestone opened rather than answered**, both consequences of the + exec service being the thing Phase 3 moves host-side. *Auth resolution*: the child environment is + resolved entirely client-side, and every value in it is a path into the client's filesystem — + `SSH_AUTH_SOCK` a socket in the client's namespace, `GIT_SSH_COMMAND` a command line valid on the + client's disk. Over a remote transport those are either dangling (a push failing in a way + `CLIVCSWriter.classify` will misattribute) or, if the transport forwards the agent socket to make + them meaningful, they hand the user's SSH agent to the remote host for the connection's lifetime. + Decide per transport: host-resolved credentials, explicitly consented agent forwarding, or + neither. Note also that the fail-fast ssh invariant (`-o BatchMode=yes`, `SSH_ASKPASS_REQUIRE`) is + appended client-side today, so the agent has no guarantee any request carries it — a remote agent + should enforce it itself. *Peer authentication*: the `git`/`jj` executable enum is not a + containment boundary, so a transport carrying exec must authenticate its peer to SHELL grade, not + repository grade. There is currently no code guard — `childEnvironment` takes no host parameter + and `AgentCommandRunner` is constructible for any `HostID` — so the first remote wiring will + silently ship a Mac `HOME`/`PATH`/`SSH_AUTH_SOCK` across the wire unless one is added. + 4. **Phase 3: publish Linux agents and prove persistent remote transport.** Establish Linux artifacts and the bootstrap/version-compatibility policy, then use the container driver to connect to a supervised agent. Do not use connection-scoped `serve --stdio` as the persistent diff --git a/macapp/AGENTS.md b/macapp/AGENTS.md index b32476064..8bf777595 100644 --- a/macapp/AGENTS.md +++ b/macapp/AGENTS.md @@ -187,6 +187,13 @@ being disproved: - *"`WRITE_TIMEOUT` bounds the repaint."* It is a **no-progress** bound, reset per `write` call (`transport.rs`). Reused as a total-transfer budget it became a throughput floor, so a healthy peer on a slow link was permanently unattachable. +- *"`CLIVCSWriter` is untouched, so agent-routed and native writes classify identically by + construction."* Both halves of the premise were true and the conclusion was false. The classifier + really was untouched; it reached opposite verdicts because the two paths fed it different INPUT — + a stale child environment, a different exit code for a missing tool, and transport failures + reported as "the command never ran". One classifier fed divergent input is harder to catch than + two classifiers, because nothing looks out of sync. Generalised: when a claim is "X is unchanged, + therefore behaviour is unchanged", the thing to verify is everything that reaches X. The standard is the one worth applying to a reviewer's finding: quote the line, or drop the confidence. It applies to your own premises first. @@ -199,6 +206,14 @@ Three rounds of this on one branch (#188): chunking a repaint fixed a panic and timeout into N; bounding that with a `break` left the client's parser stranded mid-escape-sequence; the bound itself became the throughput floor above. Each fix was correct about its target. +The same shape again on #205's review fixes, three times in two rounds: replacing an env allowlist +with `env_clear()` + the app's own environment fixed a stale-identity bug and silently dropped the +`GIT_DIR`/`GIT_WORK_TREE` scrub, so a commit requested in one repository landed in another and +reported success; putting a cancellation shield in the command runner protected the jj flock and +stranded connection slots on every superseded read; gating the resulting SIGKILL on `timed_out` +stopped it firing after normal exits and opened a path where it never fired at all. Each fix was +correct about its target. Each was caught by a reviewer that had not written it. + So: name the neighbouring property before pushing (the lock hold, the client's parser state, what a caller now does with an error it never saw before), and test it. From b28c3574d506f519c29c70d440c963a6876521bf Mon Sep 17 00:00:00 2001 From: Joel Moss Date: Fri, 18 Sep 2026 19:50:49 +0100 Subject: [PATCH 04/13] fix(macapp): stop offering a retry for a write that may have landed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `AgentCommandRunner.outcomeUnknown` already told the truth — a cancelled or disconnected write "may have completed" rather than "never ran" — but it returned a SIGTERM-shaped `CommandResult`, which is indistinguishable from a genuinely signaled `git`. So it classified as `.other`, and `.other`'s recovery is a retry of the action that failed. An honest message with a button one click from pushing twice. The distinction had nowhere to live, so it gets a value: `CommandResult.outcomeUnknown`, a sentinel exit code beside `launchFailed` and for the same reason. That completes the partition every write classifier depends on — `launchFailed` asserts nothing ran, a real exit code asserts a known outcome, and this asserts neither. It is a state the native path cannot produce: a `Process` either runs or fails to launch, and either way the answer arrives. `VCSRemoteFailure.outcomeUnknown` and `VCSCommitFailure.outcomeUnknown` carry it to the UI, both classified before any output matching — the stderr is the transport's own description, so matching git's prose against it could only misfire. `retryAction` answers `.fetch`, not nil. Nil would have left the failure tier dead on a transient state; fetch is idempotent, the ahead/behind it returns is exactly what resolves the unknown, and a successful one clears `lastFailure`. Same shape as `.rejected → .pull`, where the recovery is deliberately not the failed verb and the button is labelled with what it does. Conditional on `lastAction`, which is the neighbouring property this broke first: tier [13b] passes nil to ask what a READ failure needs on its own, and a read that lost contact wrote nothing — answering `.fetch` there replaced its "Try Again" re-read with a network action nobody asked for. For a commit the doubt is usually resolved before the user sees it: `commit()` compares the revision before and after, so a moved ref still reports `.committedThenFailed`. The failure case is reached only when the repository could not be re-read either, and its copy says so. TODOS' standing entry for this is marked fixed, and the design doc's criterion 2 no longer records an exception. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01XLpTyiW62gUJERhjwjcFFs --- TODOS.md | 18 ++++--- docs/designs/remote-workrooms.md | 15 ++++-- .../WorkroomApp/Core/AgentCommandRunner.swift | 31 ++++++------ .../Core/StatusCommandRunner.swift | 14 ++++++ .../Core/VCSSyncPresentation.swift | 48 ++++++++++++++++-- macapp/WorkroomApp/Core/VCSWriting.swift | 32 ++++++++++++ .../AgentVCSProtocolTests.swift | 49 +++++++++++++++++-- 7 files changed, 173 insertions(+), 34 deletions(-) diff --git a/TODOS.md b/TODOS.md index d5b49210d..384a5b9db 100644 --- a/TODOS.md +++ b/TODOS.md @@ -1124,14 +1124,16 @@ deferred on its own merits. **What:** What the writes review (#205, PR pending) verified and left standing. The five P1s it found are fixed in `ed51faa9`; each entry below was reproduced or read off the code, none is speculative. -1. **An outcome-unknown write still offers Retry.** `AgentCommandRunner.outcomeUnknown` stopped - claiming a cancelled or disconnected write "never ran", but it classifies as `.other`, and - `VCSSyncPresentation.retryAction` returns `lastAction` for `.other` — so the user is still one - click from re-running a push that may already have landed. The message is honest now; the button - is not. Closing it needs a new `VCSRemoteFailure`/`VCSCommitFailure` case (that switch is - exhaustive on purpose, so adding one is a compile-time-checked change) plus a `retryAction` arm - returning nil, and a matching tier in the presenter. User-visible taxonomy change, hence deferred - rather than folded into a review fix. +1. ~~**An outcome-unknown write still offers Retry.**~~ **Fixed.** `AgentCommandRunner.outcomeUnknown` + had stopped claiming a cancelled or disconnected write "never ran", but it still classified as + `.other`, whose `retryAction` is `lastAction` — an honest message with a button one click from + re-running a push that may already have landed. Closed with the taxonomy change it needed: + `CommandResult.outcomeUnknown` (a sentinel exit code beside `launchFailed`, since the old + SIGTERM-shaped result was indistinguishable from a genuinely signaled git), a + `VCSRemoteFailure`/`VCSCommitFailure` case each, and a `retryAction` arm answering `.fetch` — + not nil, as this entry originally proposed. Nil would have left the failure tier dead on a + *transient* state; fetch is idempotent, it is what resolves the unknown, and a successful one + clears `lastFailure`. Same shape as `.rejected → .pull`. 2. **`child.wait()` after SIGKILL is unbounded.** `run_exec`'s timeout path ends `break 'wait child.wait().map_err(io)?`. `kill(-pid, …)` only reaches the process group, so a diff --git a/docs/designs/remote-workrooms.md b/docs/designs/remote-workrooms.md index 54d17989c..ac93f8c1d 100644 --- a/docs/designs/remote-workrooms.md +++ b/docs/designs/remote-workrooms.md @@ -2004,9 +2004,18 @@ independently. started, inviting a retry that double-applies it). Parity needed three explicit properties, not an architectural claim: the same child environment, the same exit-code semantics for a missing tool, and an error partition that distinguishes "never ran" from "outcome unknown". Issue #205's - criterion 2 ("typed failures and Retry/Abort behavior match pre-agent behavior exactly") is met - with one recorded exception — an outcome-unknown result classifies as `.other` and is still - offered a Retry; suppressing that needs a new `VCSRemoteFailure` case, tracked in `TODOS.md`. + criterion 2 ("typed failures and Retry/Abort behavior match pre-agent behavior exactly") is met. + + That third partition is a value the native path never needed and could never produce: a `Process` + either runs or fails to launch, and either way the answer arrives. Only a round trip can be + dispatched and then go unanswered, so `CommandResult.outcomeUnknown` and the matching + `VCSRemoteFailure`/`VCSCommitFailure` cases are the one place the agent path legitimately has a + state native does not. Its whole purpose is the recovery: `retryAction` answers `.fetch` rather + than the action that failed, because re-running a push that may have landed is the one thing this + state must not offer, and a fetch is both idempotent and the thing that resolves the doubt. For a + commit the doubt is usually resolved before the user sees it — `commit()` compares the revision + before and after, so a moved ref reports `.committedThenFailed` — and the failure case is reached + only when the repository could not be re-read either. The exec service deliberately never takes the JJ snapshot barrier itself: `CLIVCSWriter`'s `JJSnapshotGate` already holds diff --git a/macapp/WorkroomApp/Core/AgentCommandRunner.swift b/macapp/WorkroomApp/Core/AgentCommandRunner.swift index 508ba29cc..56ca3c7bf 100644 --- a/macapp/WorkroomApp/Core/AgentCommandRunner.swift +++ b/macapp/WorkroomApp/Core/AgentCommandRunner.swift @@ -109,30 +109,29 @@ struct AgentCommandRunner: StatusCommandRunning, Sendable { stdout: "", stderr: reason, exitCode: CommandResult.launchFailed, timedOut: false) } - /// The command may or may not have completed; we stopped listening. Reported as a SIGTERM-signaled - /// result rather than `launchFailed`, because `launchFailed` asserts a falsehood: a cancelled or - /// disconnected `git push` DID run host-side (there is no cancel message in the protocol), and - /// telling the user it never launched invites a retry that double-applies it. + /// The command may or may not have completed; we stopped listening. Carries + /// `CommandResult.outcomeUnknown` rather than `launchFailed`, because `launchFailed` asserts a + /// falsehood: a cancelled or disconnected `git push` DID run host-side (there is no cancel message + /// in the protocol), and telling the user it never launched invites a retry that double-applies it. /// - /// It classifies as `.other(reason)`, carrying the underlying error's own description — - /// `HostConnectionError.connectionLost`'s is already exactly right ("An operation may have - /// completed; refresh before retrying"), and used to be discarded. Not the `"\(tool) was - /// interrupted"` branch: that one requires EMPTY stderr, and the message is worth more here. - /// `signaled: true` is therefore descriptive rather than load-bearing, and `timedOut` stays false - /// so this can never be mistaken for a command that ran and exceeded its own deadline. + /// It classifies as `VCSRemoteFailure.outcomeUnknown` / `VCSCommitFailure.outcomeUnknown`, carrying + /// the underlying error's own description — `HostConnectionError.connectionLost`'s is already + /// exactly right ("An operation may have completed; refresh before retrying"), and used to be + /// discarded. /// - /// KNOWN GAP: `.other` is retryable (`VCSSyncPresentation.retryAction`), so the user is still - /// offered a Retry for an operation that may already have landed. Telling the truth in the - /// message is strictly better than the old `launchFailed` ("never ran"), but suppressing the - /// button needs its own `VCSRemoteFailure`/`VCSCommitFailure` case — that switch is exhaustive on - /// purpose, and adding a case is a user-visible taxonomy change, not a drive-by fix. + /// A sentinel exit code rather than the SIGTERM-shaped result this used to return. That one was + /// indistinguishable from a genuinely signaled command, so it classified as `.other` — which + /// `VCSSyncPresentation.retryAction` still offers a Retry for, i.e. the honest message came with + /// a button that could double-apply the write. `signaled` is now false: its doc makes `exitCode` + /// the signal number whenever it's true, and -2 isn't one. `timedOut` stays false so this can + /// never be mistaken for a command that ran and exceeded its own deadline. static func outcomeUnknown(_ error: Error) -> CommandResult { let reason = (error as? LocalizedError)?.errorDescription ?? (error is CancellationError ? "The operation was cancelled; it may have completed." : nil) ?? "\(error)" return CommandResult( - stdout: "", stderr: reason, exitCode: 15, timedOut: false, signaled: true) + stdout: "", stderr: reason, exitCode: CommandResult.outcomeUnknown, timedOut: false) } } diff --git a/macapp/WorkroomApp/Core/StatusCommandRunner.swift b/macapp/WorkroomApp/Core/StatusCommandRunner.swift index 143f9ca44..e8428c80a 100644 --- a/macapp/WorkroomApp/Core/StatusCommandRunner.swift +++ b/macapp/WorkroomApp/Core/StatusCommandRunner.swift @@ -38,6 +38,20 @@ struct CommandResult: Sendable, Equatable { /// "tool not installed" would otherwise misdiagnose a deleted workroom as a missing git/jj/gh. /// Negative and outside 0-255, so it can never collide with a real exit code or a signal number. static let launchFailed: Int32 = -1 + /// The command's outcome is **unknown**: it was dispatched and we stopped listening before an + /// answer came back. Only reachable on the agent-routed path (`AgentCommandRunner.outcomeUnknown`), + /// where a connection loss, a client-side deadline or a cancellation ends the round trip while the + /// host-side command keeps running — there is no cancel message in the protocol. + /// + /// The third value in a partition every write classifier depends on: `launchFailed` asserts nothing + /// ran, a real exit code asserts a known outcome, and this asserts neither. Without it a + /// disconnected `git push` had to borrow one of the other two, and both are lies that cost + /// something: "never ran" invites a retry that double-applies it, and a signal number invites + /// `.other`, which offers the same retry. + /// + /// Negative and outside 0-255 for `launchFailed`'s reason, and distinct from it because "nothing + /// happened" and "something may have happened" need opposite recoveries. + static let outcomeUnknown: Int32 = -2 /// Written out rather than synthesized: a `let` with an initial value is EXCLUDED from the /// memberwise init entirely, so `let signaled = false` would compile at all ~60 construction diff --git a/macapp/WorkroomApp/Core/VCSSyncPresentation.swift b/macapp/WorkroomApp/Core/VCSSyncPresentation.swift index d0c2cb5a2..f53ace230 100644 --- a/macapp/WorkroomApp/Core/VCSSyncPresentation.swift +++ b/macapp/WorkroomApp/Core/VCSSyncPresentation.swift @@ -389,6 +389,19 @@ enum VCSSyncPresenter { // The workroom's folder is gone. Retrying spawns the identical command against the identical // missing path — there is no "later" where this changes, unlike ordinary contention. case .launchFailed: return nil + // The action may ALREADY HAVE HAPPENED, so `lastAction` is the one thing that must not be + // offered — a re-run of a push that landed pushes twice. Fetch instead: it is idempotent, + // it is what resolves the unknown (the ahead/behind it brings back says whether the push + // landed), and a successful one clears `lastFailure`, so the tier isn't a dead end. + // + // Same shape as `.rejected → .pull`: the recovery is deliberately not the failed verb, and the + // button is labelled with the recovery, so it can't lie about what it does. + // + // Conditional on `lastAction` because tier [13b] passes nil to ask "what does this failure need + // on its own?" — and a READ that lost contact wrote nothing, so there is no doubt for a fetch to + // resolve. Answering `.fetch` unconditionally would have replaced that tier's "Try Again" + // re-read with a network action nobody asked for. `readRetryIsWorthwhile` handles that half. + case .outcomeUnknown: return lastAction == nil ? nil : .fetch case .timedOut, .authRequired, .hostKeyUnverified, .dirtyWorkingTree, .other: return lastAction // A LOCATED lock file offers nothing, for `rebaseInProgress`'s reason: the file is sitting there, so @@ -417,9 +430,12 @@ enum VCSSyncPresenter { case .toolMissing, .noRemote, .needsDescription, .immutableHistory: return false // The workroom's folder is gone — re-reading the same missing path can't succeed. case .launchFailed: return false - // Both carry an ACTION recovery, so this is only reached if that path is ever changed — and neither - // is fixed by re-reading. + // All three carry an ACTION recovery, so this is only reached if that path is ever changed. + // `.outcomeUnknown` is the one that would answer differently here: a READ has no side effect, so + // re-running it is always safe — the danger is re-running the WRITE, which is `retryAction`'s + // problem and already handled there. case .rejected, .rebaseInProgress: return false + case .outcomeUnknown: return true } } @@ -469,6 +485,10 @@ enum VCSSyncPresenter { return "Describe the change before pushing it (jj describe)." case .immutableHistory: return "Pull would rewrite shared history here, so it can’t run." + case .outcomeUnknown: + // Never "failed": the single most expensive mistake here is a user who reads this as "it + // didn't happen" and repeats the action. The line has to carry the doubt, not the verdict. + return "Lost contact — this may have completed. Fetch to check." case .other(let message): return message.split(whereSeparator: \.isNewline).first.map(String.init) ?? "Failed." } @@ -565,6 +585,15 @@ enum VCSSyncPresenter { The workroom may have been deleted, or its folder moved or removed outside Workroom. \ Refresh the sidebar, or recreate the workroom if it's genuinely gone. """ + case .outcomeUnknown: + return """ + The command was sent and the connection dropped before an answer came back, so it may have \ + completed anyway — there is no way to cancel one once it has been sent. + + Fetch first and read the result. If the push landed, the counts will say so and there is \ + nothing left to do; if it didn't, push again from there. Repeating the action before \ + checking is what applies it twice. + """ // `.locked` is answered by `explain` in full; `.other` is raw tool output we have no advice for. case .locked, .other: return nil @@ -578,7 +607,7 @@ enum VCSSyncPresenter { switch failure { case .timedOut(_, let m), .authRequired(let m), .hostKeyUnverified(let m), .rejected(let m), .dirtyWorkingTree(let m), - .immutableHistory(let m), .needsDescription(let m), .other(let m): + .immutableHistory(let m), .needsDescription(let m), .outcomeUnknown(let m), .other(let m): raw = m case .toolMissing, .noRemote, .rebaseInProgress, .locked, .launchFailed: raw = nil @@ -665,6 +694,8 @@ enum VCSSyncPresenter { guard let file else { return "The repository was busy. Try again." } return "A leftover \(file.filename) is blocking git." case .unsupportedMode: return "That action isn’t available for this repository." + case .outcomeUnknown: + return "Lost contact — this commit may have been written." case .other(let message): return message.split(whereSeparator: \.isNewline).first.map(String.init) ?? "Commit failed." } @@ -725,6 +756,15 @@ enum VCSSyncPresenter { The workroom may have been deleted, or its folder moved or removed outside Workroom. \ Refresh the sidebar, or recreate the workroom if it's genuinely gone. """ + case .outcomeUnknown: + return """ + The commit was sent and the connection dropped before an answer came back. Workroom also \ + couldn’t re-read the repository afterwards, so it can’t tell you whether the commit was \ + written — when it can, it says so instead of showing this. + + Check the history before committing again. Committing a second time over a commit that did \ + land leaves you with two. + """ case .unsupportedMode, .other: return nil } @@ -735,7 +775,7 @@ enum VCSSyncPresenter { let raw: String? switch failure { case .identityMissing(let m), .signingFailed(let m), .hookRejected(let m), - .unmergedFiles(let m), .other(let m): + .unmergedFiles(let m), .outcomeUnknown(let m), .other(let m): raw = m case .toolMissing, .launchFailed, .timedOut, .nothingToCommit, .sequencerInProgress, .locked, .unsupportedMode: diff --git a/macapp/WorkroomApp/Core/VCSWriting.swift b/macapp/WorkroomApp/Core/VCSWriting.swift index bb4ca200d..c68aded10 100644 --- a/macapp/WorkroomApp/Core/VCSWriting.swift +++ b/macapp/WorkroomApp/Core/VCSWriting.swift @@ -77,6 +77,17 @@ enum VCSRemoteFailure: Equatable, Sendable { /// every retry fails identically. The real fix is remembering each workroom's own base instead of /// guessing `trunk()` — filed, not built. case immutableHistory(String) + /// The command was dispatched and we never heard back — `CommandResult.outcomeUnknown`. Only + /// reachable on the agent-routed path, where a lost connection, a client-side deadline or a + /// cancellation ends the round trip while the host-side `git`/`jj` keeps running to completion. + /// + /// The whole point of the case is the recovery. This is NOT `.other`: that one's recovery is a + /// retry of the action that failed, and retrying a push that may already have landed is the one + /// thing this state must not offer. `retryAction` answers `.fetch` instead — idempotent, and the + /// ahead/behind it brings back is exactly the fact that resolves the unknown. + /// + /// It is also NOT `.launchFailed`: that asserts nothing ran, which is the opposite falsehood. + case outcomeUnknown(String) /// jj refuses to push a commit with an empty description, changes or not. This is the state a workroom /// sits in the moment you edit a file and before you write a message, so it is the most reachable /// failure on the push path, not an edge case. Measured: `Error: Won't push commit 050e657d3c36 since @@ -212,6 +223,13 @@ enum VCSCommitFailure: Equatable, Sendable { /// are invalid during several of these, and finishing the sequencer is the user's call. case sequencerInProgress(String) case locked(VCSLockFile?) + /// The commit was dispatched and we never heard back — see `VCSRemoteFailure.outcomeUnknown`. + /// + /// Reached ONLY when the ref could not be re-read either: `commit()` compares the revision before + /// and after, and a moved ref answers the question outright as `.committedThenFailed`. So this is + /// specifically "the write is unknown AND the repository is unreachable", which is why its copy + /// sends the user to check the history rather than offering anything. + case outcomeUnknown(String) /// The verb doesn't exist for this backend (`.amendMessage` on jj, `.describe` on git). case unsupportedMode case other(String) @@ -1135,6 +1153,12 @@ struct CLIVCSWriter: LocalVCSWriting, Sendable { // Checked BEFORE commandNotFound: launchFailed means the process never ran at all (dominated by // a vanished cwd), which is a different fact from commandNotFound's "env ran and searched PATH". if result.exitCode == CommandResult.launchFailed { return .launchFailed } + // Before every output check, for `launchFailed`'s reason: this is a fact about whether we heard + // an answer, not about what the answer said. There is no output to match on anyway — the stderr + // is the transport's own description, and matching git's prose against it could only misfire. + if result.exitCode == CommandResult.outcomeUnknown { + return .outcomeUnknown(result.stderr.trimmingCharacters(in: .whitespacesAndNewlines)) + } if result.exitCode == CommandResult.commandNotFound { return .toolMissing(tool) } let err = result.stderr + "\n" + result.stdout // A timed-out pull may have left a rebase behind; that reads better than "timed out". @@ -1264,6 +1288,10 @@ struct CLIVCSWriter: LocalVCSWriting, Sendable { /// perfectly good commit behind a non-zero exit. static func classifyCommit(_ result: CommandResult, tool: String) -> VCSCommitFailure? { if result.exitCode == CommandResult.launchFailed { return .launchFailed } + // See `classify` — a fact about the round trip, not about the command's output. + if result.exitCode == CommandResult.outcomeUnknown { + return .outcomeUnknown(result.stderr.trimmingCharacters(in: .whitespacesAndNewlines)) + } if result.exitCode == CommandResult.commandNotFound { return .toolMissing(tool) } // jj says "Nothing changed." and exits ZERO, so its no-op has to be read BEFORE the success // guard — an untouched working copy must not be reported as a commit that happened. @@ -1975,6 +2003,10 @@ struct CLIVCSWriter: LocalVCSWriting, Sendable { case .toolMissing(let m), .identityMissing(let m), .signingFailed(let m), .hookRejected(let m), .unmergedFiles(let m), .sequencerInProgress(let m), .other(let m): return m + // Reached when contact was lost AND the ref moved anyway — so the doubt is already resolved, + // and this says which half of it survived rather than repeating the "may have completed" copy. + case .outcomeUnknown(let m): + return "Lost contact after the commit was written: \(m)" case .timedOut: return "The command was stopped at its time limit." case .nothingToCommit: return "Nothing to commit." case .locked(let file): diff --git a/macapp/WorkroomAppTests/AgentVCSProtocolTests.swift b/macapp/WorkroomAppTests/AgentVCSProtocolTests.swift index 6cfdffcab..a39558818 100644 --- a/macapp/WorkroomAppTests/AgentVCSProtocolTests.swift +++ b/macapp/WorkroomAppTests/AgentVCSProtocolTests.swift @@ -44,16 +44,18 @@ final class AgentVCSProtocolTests: XCTestCase { /// as `launchFailed` told the user nothing had happened and invited a retry that double-applies. func testTransportFailureIsNotReportedAsNeverHavingRun() { let lost = AgentCommandRunner.outcomeUnknown(HostConnectionError.connectionLost) + XCTAssertEqual(lost.exitCode, CommandResult.outcomeUnknown) XCTAssertNotEqual(lost.exitCode, CommandResult.launchFailed) - XCTAssertTrue(lost.signaled) + // `signaled` makes `exitCode` a signal number, and the sentinel isn't one. It used to be true, + // which made this indistinguishable from a genuinely SIGTERMed git and sent it to `.other`. + XCTAssertFalse(lost.signaled) // Not `timedOut`: that offers Retry, and nobody knows the outcome. XCTAssertFalse(lost.timedOut) // The existing error already says the right thing; it used to be discarded. XCTAssertTrue(lost.stderr.contains("may have completed")) let cancelled = AgentCommandRunner.outcomeUnknown(CancellationError()) - XCTAssertNotEqual(cancelled.exitCode, CommandResult.launchFailed) - XCTAssertTrue(cancelled.signaled) + XCTAssertEqual(cancelled.exitCode, CommandResult.outcomeUnknown) XCTAssertTrue(cancelled.stderr.contains("may have completed")) // A request refused before it left this process genuinely never ran, and keeps the old value. @@ -61,6 +63,47 @@ final class AgentVCSProtocolTests: XCTestCase { AgentCommandRunner.neverRan("too large").exitCode, CommandResult.launchFailed) } + /// The sentinel has to survive the whole way to the button, on both write taxonomies. This is the + /// end-to-end form of the defect: an honest message with a Retry beside it is still a retry that + /// double-applies the push. + func testAnUnknownOutcomeNeverOffersARetryOfTheActionThatFailed() { + let lost = AgentCommandRunner.outcomeUnknown(HostConnectionError.connectionLost) + + guard case .outcomeUnknown(let reason) = CLIVCSWriter.classify(lost, action: .push, tool: "git") + else { return XCTFail("push did not classify as outcomeUnknown") } + XCTAssertTrue(reason.contains("may have completed")) + + guard case .outcomeUnknown = CLIVCSWriter.classifyCommit(lost, tool: "git") else { + return XCTFail("commit did not classify as outcomeUnknown") + } + + // Fetch, never Push: idempotent, and the ahead/behind it returns is what resolves the unknown. + XCTAssertEqual( + VCSSyncPresenter.retryAction(for: .outcomeUnknown(reason), lastAction: .push), .fetch) + XCTAssertEqual( + VCSSyncPresenter.retryAction(for: .outcomeUnknown(reason), lastAction: .pull), .fetch) + // A READ that lost contact wrote nothing, so tier [13b]'s `lastAction: nil` question must fall + // through to its own "Try Again" re-read rather than being handed a network action. + XCTAssertNil(VCSSyncPresenter.retryAction(for: .outcomeUnknown(reason), lastAction: nil)) + XCTAssertTrue(VCSSyncPresenter.readRetryIsWorthwhile(.outcomeUnknown(reason))) + // The copy must carry the doubt. A user who reads "failed" repeats the action. + XCTAssertTrue(VCSSyncPresenter.describe(.outcomeUnknown(reason)).contains("may have")) + XCTAssertTrue( + VCSSyncPresenter.describeCommit(.outcomeUnknown(reason)).contains("may have")) + } + + /// The sentinel is a claim about the round trip, so it must beat every output-shaped check — a + /// transport message that happened to contain git's prose must not be classified as git's failure. + func testTheUnknownSentinelOutranksOutputMatching() { + let misleading = CommandResult( + stdout: "", stderr: "Updates were rejected; the host key verification failed", + exitCode: CommandResult.outcomeUnknown, timedOut: false) + guard case .outcomeUnknown = CLIVCSWriter.classify(misleading, action: .push, tool: "git") + else { + return XCTFail("output matching won over the sentinel") + } + } + /// Both paths must hand the child the SAME environment. Forwarding only `PATH` let the child /// inherit the rest from a daemon "negotiated with, never replaced", so an agent-routed commit /// could be authored under the identity of whichever app launch first spawned it. From 783ebc6cb8772f3d40f0814cfaed6321d5333ba9 Mon Sep 17 00:00:00 2001 From: Joel Moss Date: Fri, 18 Sep 2026 20:11:15 +0100 Subject: [PATCH 05/13] fix(macapp): address the adversarial review of the outcome-unknown fix MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The sentinel and its partition survived review; six of the surfaces it reaches did not. Rule 2 again — each finding is in the property next door, and none is in the hunk that introduced it. **The same defect, on the sibling surface.** `CommitSheet.isSpent` was true only for `.landedThenFailed`, so `.failed(.outcomeUnknown)` left the Commit button live under a notice telling the user to check the history first — one click from the second commit, on the very taxonomy the last commit added a case to name. `.outcomeUnknown` becomes its own `Phase`, for `.landedThenFailed`'s stated reason: the button is the hazard, not the wording. Not folded INTO `.landedThenFailed`, which asserts the commit was recorded. **`.fetch` was generalised from one verb.** The recovery rule is IDEMPOTENCE, not "never the failed verb". `.fetch` and `.abortRebase` are idempotent (a second `git rebase --abort` exits "No rebase in progress"), so they are offered back; only `.push`/`.pull` are swapped for a fetch. A lost abort was being answered with Fetch, which resolves nothing about a parked rebase — and `perform` allows an abort with no remote, so that click replaced the doubt with an unrelated `.noRemote`. **The disk is not output.** Returning the sentinel before the `.pull` + `rebaseInProgress` check discarded the one piece of positive evidence available: a lost pull left the same parked `rebase-merge` a killed one does, no read path checks for it, and the offered Fetch then cleared `lastFailure` and restored an ordinary toolbar over a workroom parked mid-rebase. It now asks the disk in the same order the `timedOut` branch does, and accepts the same race for the same reason. `commit()` already resolved its own unknown off the ref this way. **Copy written for push, rendered for everything.** The remedy said "if the push landed… push again", and tier [13b] routes READ failures through the same `describe`/`remedy` pair — so a lost pull was told to push, and so was a failed read. Now verb-neutral. **Two surfaces stated the verdict the case exists to withhold.** The dialog title and the spoken label both hardcoded "\(verb) failed" over a body saying it may have succeeded; for a VoiceOver user that sentence IS the message. Both go through one `headline`. **An intent-to-add failure claimed a commit was sent.** `guard ita.ok else { return ita }` returned the staging result to be classified as a commit, and skipped the rollback entirely — pre-existing, but the lost-contact case is the one where those index entries may actually have landed, which is precisely the `git stash` breakage that rollback exists to prevent. The rollback is now a local function both exits use, and the staging failure is labelled as its own step. Also: `VCSCommitFailure.outcomeUnknown`'s doc and copy claimed the ref "could not be re-read", which `commit()` does not enforce (a ref that read back unchanged lands here too); `readRetryIsWorthwhile`'s comment called the new arm unreachable when tier [13b] makes it the live path. Tests: four new cases covering the idempotence rule, the parked rebase, the heading, and the verb-neutral remedy — the gap that would have caught the rebase finding was one missing `action: .pull` case. The existing sentinel-precedence test matched a marker that never fired (lowercase); fixed. Serial suite: 2759 passed, 1 skipped, 0 failures. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01XLpTyiW62gUJERhjwjcFFs --- .../Core/VCSSyncPresentation.swift | 83 ++++++++++++------- macapp/WorkroomApp/Core/VCSWriting.swift | 69 ++++++++++----- macapp/WorkroomApp/Views/CommitSheet.swift | 24 +++++- .../AgentVCSProtocolTests.swift | 64 +++++++++++++- 4 files changed, 187 insertions(+), 53 deletions(-) diff --git a/macapp/WorkroomApp/Core/VCSSyncPresentation.swift b/macapp/WorkroomApp/Core/VCSSyncPresentation.swift index f53ace230..61f46708d 100644 --- a/macapp/WorkroomApp/Core/VCSSyncPresentation.swift +++ b/macapp/WorkroomApp/Core/VCSSyncPresentation.swift @@ -175,7 +175,10 @@ enum VCSSyncPresenter { // The tooltip is where a failure gets room to explain itself — one line fits the bar, the // remedy usually doesn't. tone: .failure, help: explain(failure, now: now), - accessibility: "\(lastAction?.label ?? "Action") failed. \(explain(failure, now: now))", + // Via `headline`, not a literal "failed": VoiceOver gets no visual text, so this sentence IS + // the verdict for that user, and asserting one over `.outcomeUnknown` is the mistake the + // whole case exists to avoid. + accessibility: "\(headline(failure, action: lastAction)). \(explain(failure, now: now))", lockPath: lockPath(of: failure)) } @@ -389,19 +392,27 @@ enum VCSSyncPresenter { // The workroom's folder is gone. Retrying spawns the identical command against the identical // missing path — there is no "later" where this changes, unlike ordinary contention. case .launchFailed: return nil - // The action may ALREADY HAVE HAPPENED, so `lastAction` is the one thing that must not be - // offered — a re-run of a push that landed pushes twice. Fetch instead: it is idempotent, - // it is what resolves the unknown (the ahead/behind it brings back says whether the push - // landed), and a successful one clears `lastFailure`, so the tier isn't a dead end. + // The action may ALREADY HAVE HAPPENED, so the question is not "can this be retried?" but "is + // this verb IDEMPOTENT?" — re-running one that is costs nothing whether or not the first landed. // - // Same shape as `.rejected → .pull`: the recovery is deliberately not the failed verb, and the - // button is labelled with the recovery, so it can't lie about what it does. + // `.fetch` and `.abortRebase` are: a second fetch re-reads refs, and `git rebase --abort` with + // no rebase parked exits with a harmless "No rebase in progress". So they are offered back. // - // Conditional on `lastAction` because tier [13b] passes nil to ask "what does this failure need - // on its own?" — and a READ that lost contact wrote nothing, so there is no doubt for a fetch to - // resolve. Answering `.fetch` unconditionally would have replaced that tier's "Try Again" - // re-read with a network action nobody asked for. `readRetryIsWorthwhile` handles that half. - case .outcomeUnknown: return lastAction == nil ? nil : .fetch + // `.push` and `.pull` are not, so they get `.fetch` instead — the same shape as `.rejected → + // .pull`, where the recovery is deliberately not the failed verb and the button is labelled with + // the recovery so it can't lie about what it does. Fetch is what RESOLVES the doubt for those + // two: the ahead/behind it brings back is precisely the fact that says whether the push landed. + // + // Not `.fetch` for everything, which is what this arm did first. It buried a lost `.abortRebase` + // (a fetch says nothing about `rebase-merge`, and `perform` allows an abort with no remote — so + // the offered Fetch would have replaced the doubt with an unrelated `.noRemote`). + // + // Nil for a nil `lastAction`, because tier [13b] passes nil to ask what this failure needs on its + // own — and a READ that lost contact wrote nothing, so there is no doubt for a fetch to resolve. + // That tier's own "Try Again" re-read is the right offer; `readRetryIsWorthwhile` gives it. + case .outcomeUnknown: + guard let lastAction else { return nil } + return lastAction == .push || lastAction == .pull ? .fetch : lastAction case .timedOut, .authRequired, .hostKeyUnverified, .dirtyWorkingTree, .other: return lastAction // A LOCATED lock file offers nothing, for `rebaseInProgress`'s reason: the file is sitting there, so @@ -430,11 +441,13 @@ enum VCSSyncPresenter { case .toolMissing, .noRemote, .needsDescription, .immutableHistory: return false // The workroom's folder is gone — re-reading the same missing path can't succeed. case .launchFailed: return false - // All three carry an ACTION recovery, so this is only reached if that path is ever changed. - // `.outcomeUnknown` is the one that would answer differently here: a READ has no side effect, so - // re-running it is always safe — the danger is re-running the WRITE, which is `retryAction`'s - // problem and already handled there. + // Both carry an ACTION recovery, so this is only reached if that path is ever changed — and + // neither is fixed by re-reading. case .rejected, .rebaseInProgress: return false + // NOT that: `retryAction` answers nil for a nil `lastAction`, which is exactly what tier [13b] + // passes, so this is the LIVE path for a read that lost contact — not a leftover branch. A read + // has no side effect, so re-running it is always safe; the danger is re-running the WRITE, and + // that is `retryAction`'s problem. case .outcomeUnknown: return true } } @@ -450,6 +463,18 @@ enum VCSSyncPresenter { lastPullConflicted && statusConflicted } + /// The verdict line — the dialog's title and the spoken label both open with it. + /// + /// One function rather than the literal `"\(action) failed"` those two used, because exactly one + /// failure is not a verdict: `.outcomeUnknown` says the command may well have succeeded, and a + /// heading that calls it a failure contradicts the body underneath it (and, for VoiceOver, IS the + /// body — that user hears the heading and nothing else). + static func headline(_ failure: VCSRemoteFailure, action: VCSRemoteAction?) -> String { + let verb = action?.label ?? "The last action" + if case .outcomeUnknown = failure { return "\(verb) may not have completed" } + return "\(verb) failed" + } + /// One-line, actionable copy per failure. The raw stderr is legible for some cases and baffling for /// others, so the baffling ones get written copy. static func describe(_ failure: VCSRemoteFailure) -> String { @@ -514,8 +539,7 @@ enum VCSSyncPresenter { separator: "\n\n") } return VCSFailureDialog( - title: isRead - ? "Couldn’t read the repository" : "\(action?.label ?? "The last action") failed", + title: isRead ? "Couldn’t read the repository" : headline(failure, action: action), subtitle: workroom.map { "in \($0)" }, message: message, details: rawOutput(of: failure), @@ -586,13 +610,16 @@ enum VCSSyncPresenter { Refresh the sidebar, or recreate the workroom if it's genuinely gone. """ case .outcomeUnknown: + // Verb-neutral on purpose. This is reached for push, pull, fetch, a rebase abort AND for a + // read (tier [13b] describes a read failure through this same function), so naming one of + // them was wrong advice for the rest — telling someone whose rebase may be half-applied to + // "push again" was the worst of them. return """ - The command was sent and the connection dropped before an answer came back, so it may have \ - completed anyway — there is no way to cancel one once it has been sent. + Workroom sent the command and the connection dropped before an answer came back, so it may \ + have completed anyway — once sent, there is no way to call one back. - Fetch first and read the result. If the push landed, the counts will say so and there is \ - nothing left to do; if it didn't, push again from there. Repeating the action before \ - checking is what applies it twice. + Check the current state before repeating it: the toolbar's counts after a fetch, or the \ + history and `git status` in a terminal. Repeating it blind is what applies it twice. """ // `.locked` is answered by `explain` in full; `.other` is raw tool output we have no advice for. case .locked, .other: @@ -758,12 +785,12 @@ enum VCSSyncPresenter { """ case .outcomeUnknown: return """ - The commit was sent and the connection dropped before an answer came back. Workroom also \ - couldn’t re-read the repository afterwards, so it can’t tell you whether the commit was \ - written — when it can, it says so instead of showing this. + Workroom sent the command and the connection dropped before an answer came back, and it \ + could not confirm afterwards whether the commit was written. When it can confirm, it says \ + so instead of showing this. - Check the history before committing again. Committing a second time over a commit that did \ - land leaves you with two. + Check the history before committing again. Committing a second time over one that did land \ + leaves you with two. """ case .unsupportedMode, .other: return nil diff --git a/macapp/WorkroomApp/Core/VCSWriting.swift b/macapp/WorkroomApp/Core/VCSWriting.swift index c68aded10..218257f7a 100644 --- a/macapp/WorkroomApp/Core/VCSWriting.swift +++ b/macapp/WorkroomApp/Core/VCSWriting.swift @@ -225,10 +225,11 @@ enum VCSCommitFailure: Equatable, Sendable { case locked(VCSLockFile?) /// The commit was dispatched and we never heard back — see `VCSRemoteFailure.outcomeUnknown`. /// - /// Reached ONLY when the ref could not be re-read either: `commit()` compares the revision before - /// and after, and a moved ref answers the question outright as `.committedThenFailed`. So this is - /// specifically "the write is unknown AND the repository is unreachable", which is why its copy - /// sends the user to check the history rather than offering anything. + /// `commit()` compares the revision before and after, so a MOVED ref answers the question outright + /// as `.committedThenFailed` and never reaches here. What reaches here is everything else: the ref + /// could not be re-read (the common case — a dead socket fails that request too), or it read back + /// unchanged, which is not proof of anything because a commit host-side may simply not have + /// finished. Hence copy that sends the user to check the history rather than asserting either way. case outcomeUnknown(String) /// The verb doesn't exist for this backend (`.amendMessage` on jj, `.describe` on git). case unsupportedMode @@ -1156,7 +1157,17 @@ struct CLIVCSWriter: LocalVCSWriting, Sendable { // Before every output check, for `launchFailed`'s reason: this is a fact about whether we heard // an answer, not about what the answer said. There is no output to match on anyway — the stderr // is the transport's own description, and matching git's prose against it could only misfire. + // + // Except the DISK, which is not output and does not depend on having heard back. A pull whose + // reply was lost after git wrote `rebase-merge` leaves the same parked rebase as one we killed + // at its timeout, and the remedy is the same Abort — so this asks the same question the + // `timedOut` branch below does, in the same order, rather than discarding the one piece of + // positive evidence available. `commit()` resolves its own unknown the same way, off the ref. + // + // It races a host-side git that is still rebasing, and that is accepted here for the reason the + // `timedOut` branch already accepts it: a parked rebase the user cannot see is the worse state. if result.exitCode == CommandResult.outcomeUnknown { + if action == .pull, rebaseInProgress(gitDir: gitDir) { return .rebaseInProgress } return .outcomeUnknown(result.stderr.trimmingCharacters(in: .whitespacesAndNewlines)) } if result.exitCode == CommandResult.commandNotFound { return .toolMissing(tool) } @@ -1848,18 +1859,6 @@ struct CLIVCSWriter: LocalVCSWriting, Sendable { timeout: commitTimeout) } - // New sides only — see `gitPathspecPayload(literalPaths:)`. - let unknown = Self.pathsGitMayNotKnow(in: request.files) - if !unknown.isEmpty { - let ita = await runner.run( - vcs, Self.gitIntentToAddArgs(), in: path, timeout: refTimeout, - stdin: Self.gitPathspecPayload(literalPaths: unknown)) - guard ita.ok else { return ita } - } - let result = await runner.run( - vcs, Self.gitCommitOnlyArgs(message: request.message), in: path, timeout: commitTimeout, - stdin: Self.gitPathspecPayload(request.files)) - // The commit failed, so undo the index entries we just made. Left behind, an intent-to-add marker // is not the harmless residue it looks like: it breaks the user's own `git stash` in the terminal // ("Entry 'x' not uptodate. Cannot merge.") until they find and reverse a change they never made. @@ -1868,14 +1867,42 @@ struct CLIVCSWriter: LocalVCSWriting, Sendable { // `rm --cached` reverses exactly our own step. A rename's new side may already be a real staged // entry (`git mv`), and unstaging that would destroy work the user did themselves — the wrong // trade for tidiness. - if !result.ok { + func rollBackIntentToAdd() async { let added = Self.pathsAddedToTheIndex(in: request.files) - if !added.isEmpty { - _ = await runner.run( - vcs, Self.gitUnstageArgs(), in: path, timeout: refTimeout, - stdin: Self.gitPathspecPayload(literalPaths: added)) + guard !added.isEmpty else { return } + _ = await runner.run( + vcs, Self.gitUnstageArgs(), in: path, timeout: refTimeout, + stdin: Self.gitPathspecPayload(literalPaths: added)) + } + + // New sides only — see `gitPathspecPayload(literalPaths:)`. + let unknown = Self.pathsGitMayNotKnow(in: request.files) + if !unknown.isEmpty { + let ita = await runner.run( + vcs, Self.gitIntentToAddArgs(), in: path, timeout: refTimeout, + stdin: Self.gitPathspecPayload(literalPaths: unknown)) + guard ita.ok else { + // Rolled back HERE too, not only after a failed commit. A killed intent-to-add writes + // nothing, but a `CommandResult.outcomeUnknown` one may have written every entry before the + // connection dropped — so the branch that skipped the rollback was the one that most needed + // it, leaving exactly the `git stash` breakage the comment above documents. + await rollBackIntentToAdd() + // Relabelled as its own step, so the copy downstream ("The commit was sent…") cannot claim + // a commit was attempted when only the staging was. The exit code is carried through + // unchanged, so `classifyCommit` still reaches the same case. + return CommandResult( + stdout: ita.stdout, + stderr: ita.stderr.isEmpty + ? "git could not stage the new files" + : "Staging the new files failed: \(ita.stderr)", + exitCode: ita.exitCode, timedOut: ita.timedOut, signaled: ita.signaled) } } + let result = await runner.run( + vcs, Self.gitCommitOnlyArgs(message: request.message), in: path, timeout: commitTimeout, + stdin: Self.gitPathspecPayload(request.files)) + + if !result.ok { await rollBackIntentToAdd() } return result } diff --git a/macapp/WorkroomApp/Views/CommitSheet.swift b/macapp/WorkroomApp/Views/CommitSheet.swift index 108ea84d7..e8ace4e5f 100644 --- a/macapp/WorkroomApp/Views/CommitSheet.swift +++ b/macapp/WorkroomApp/Views/CommitSheet.swift @@ -54,12 +54,24 @@ struct CommitSheet: View { /// Distinct from `.failed` because retrying here would record the work a second time, and the /// notice says so — so the buttons that would do it are disabled rather than merely discouraged. case landedThenFailed(VCSFailureDialog) + /// Contact was lost mid-commit and the ref could not be re-read, so whether the work landed is + /// unknown — `VCSCommitFailure.outcomeUnknown`. + /// + /// Its own case rather than a `.failed`, for `.landedThenFailed`'s exact reason: the button is + /// the hazard, not the wording. A `.failed` leaves Commit live, and one click on a commit that + /// did land records it twice — the same defect, on the taxonomy that added a case to name it. + /// Not folded INTO `.landedThenFailed` either: that one asserts the commit was recorded, and + /// asserting the verdict is what this whole state exists to avoid. + case outcomeUnknown(VCSFailureDialog) } - /// The commit is on disk; nothing in this dialog can be pressed again without duplicating it. + /// Pressing again could duplicate work — either because the commit is on disk, or because nobody + /// knows whether it is. Both disable the buttons that would do it. private var isSpent: Bool { - if case .landedThenFailed = phase { return true } - return false + switch phase { + case .landedThenFailed, .outcomeUnknown: return true + default: return false + } } /// Hard cap on rendered rows, matching `ChangesPanel`'s. See `fileSection`. @@ -111,6 +123,7 @@ struct CommitSheet: View { if case .confirmingStagedLoss(let paths) = phase { stagedLossNotice(paths) } if case .failed(let dialog) = phase { CommitFailureNotice(dialog: dialog) } if case .landedThenFailed(let dialog) = phase { CommitFailureNotice(dialog: dialog) } + if case .outcomeUnknown(let dialog) = phase { CommitFailureNotice(dialog: dialog) } Divider() buttons } @@ -529,6 +542,11 @@ struct CommitSheet: View { "The commit was recorded. A step that runs after it — usually a post-commit hook — " + "then failed. Do not commit again; the change is already saved.", details: detail, recovery: nil, lockPath: nil)) + case .failed(.outcomeUnknown(let reason)): + // Routed away from `.failed` so `isSpent` disables Commit: the work may be on disk, and the + // notice telling the user to check the history is worth nothing beside a live button. + phase = .outcomeUnknown( + VCSSyncPresenter.commitFailureDialog(.outcomeUnknown(reason), mode: mode)) case .failed(let failure): phase = .failed(VCSSyncPresenter.commitFailureDialog(failure, mode: mode)) } diff --git a/macapp/WorkroomAppTests/AgentVCSProtocolTests.swift b/macapp/WorkroomAppTests/AgentVCSProtocolTests.swift index a39558818..97b2ab88e 100644 --- a/macapp/WorkroomAppTests/AgentVCSProtocolTests.swift +++ b/macapp/WorkroomAppTests/AgentVCSProtocolTests.swift @@ -92,11 +92,73 @@ final class AgentVCSProtocolTests: XCTestCase { VCSSyncPresenter.describeCommit(.outcomeUnknown(reason)).contains("may have")) } + /// The recovery rule is IDEMPOTENCE, not "never the failed verb". A lost `abortRebase` used to be + /// answered with Fetch, which resolves nothing about a parked rebase — and `perform` allows an + /// abort with no remote, so that click would have replaced the doubt with an unrelated `.noRemote`. + func testOnlyNonIdempotentVerbsAreSwappedForFetch() { + let reason = "An operation may have completed; refresh before retrying" + for verb in [VCSRemoteAction.fetch, .abortRebase] { + XCTAssertEqual( + VCSSyncPresenter.retryAction(for: .outcomeUnknown(reason), lastAction: verb), verb, + "\(verb.label) is idempotent and should be offered back") + } + for verb in [VCSRemoteAction.push, .pull] { + XCTAssertEqual( + VCSSyncPresenter.retryAction(for: .outcomeUnknown(reason), lastAction: verb), .fetch, + "\(verb.label) is not idempotent and must not be offered back") + } + } + + /// The disk is not output, so it survives never hearing back. A pull whose reply was lost after git + /// wrote `rebase-merge` leaves the same parked rebase a killed one does, and the same Abort fixes + /// it — checking the sentinel first discarded the only positive evidence available. + func testALostPullStillFindsAParkedRebaseOnDisk() throws { + let dir = URL(fileURLWithPath: NSTemporaryDirectory()) + .appendingPathComponent("wr-rebase-\(UUID().uuidString)") + try FileManager.default.createDirectory( + at: dir.appendingPathComponent("rebase-merge"), withIntermediateDirectories: true) + defer { try? FileManager.default.removeItem(at: dir) } + + let lost = AgentCommandRunner.outcomeUnknown(HostConnectionError.connectionLost) + guard + case .rebaseInProgress = CLIVCSWriter.classify( + lost, action: .pull, tool: "git", gitDir: dir) + else { return XCTFail("the parked rebase was discarded") } + + // Only for a pull, and only when the directory is actually there. + guard + case .outcomeUnknown = CLIVCSWriter.classify( + lost, action: .push, tool: "git", gitDir: dir) + else { return XCTFail("a push must not be upgraded to rebaseInProgress") } + } + + /// `describe`'s own rule — never "failed" over a state that may have succeeded — has to hold for the + /// heading too. For a VoiceOver user the heading IS the whole message. + func testTheHeadingDoesNotAssertAVerdictItDoesNotHave() { + let unknown = VCSRemoteFailure.outcomeUnknown("dropped") + XCTAssertEqual( + VCSSyncPresenter.headline(unknown, action: .push), "Push may not have completed") + XCTAssertFalse(VCSSyncPresenter.headline(unknown, action: .push).contains("failed")) + // Every other failure keeps the plain verdict. + XCTAssertEqual(VCSSyncPresenter.headline(.noRemote, action: .push), "Push failed") + } + + /// The remedy is rendered for push, pull, fetch, an abort AND for read failures (tier [13b] routes + /// a read through the same `describe`/`remedy` pair), so naming one verb was wrong advice for the + /// rest — "push again" to someone whose rebase may be half-applied most of all. + func testTheUnknownOutcomeRemedyNamesNoVerb() throws { + let remedy = try XCTUnwrap(VCSSyncPresenter.remedy(for: .outcomeUnknown("dropped"))) + for verb in ["push again", "pull again", "the push landed"] { + XCTAssertFalse(remedy.lowercased().contains(verb), "remedy assumes a verb: \(verb)") + } + } + /// The sentinel is a claim about the round trip, so it must beat every output-shaped check — a /// transport message that happened to contain git's prose must not be classified as git's failure. func testTheUnknownSentinelOutranksOutputMatching() { let misleading = CommandResult( - stdout: "", stderr: "Updates were rejected; the host key verification failed", + stdout: "", + stderr: "Updates were rejected. Host key verification failed. fatal: Cannot autostash", exitCode: CommandResult.outcomeUnknown, timedOut: false) guard case .outcomeUnknown = CLIVCSWriter.classify(misleading, action: .push, tool: "git") else { From e1c308fd7a72cad80d740456f7e71a48783cc91b Mon Sep 17 00:00:00 2001 From: Joel Moss Date: Fri, 18 Sep 2026 21:09:45 +0100 Subject: [PATCH 06/13] test(macapp): close the seven untested gaps around the exec service MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four of the writes review's defects — the unbounded join, the two caps that can't both be satisfied, the stale child environment and the missing tool reported as a deleted workroom — all lived in exactly this region. That is a measurement, not a hunch, and the remaining TODOS items all edit code inside it, so the coverage goes in first. Rust (`vcs.rs`): - The exec timeout clamp at both ends. 0 would kill a child before it started and an unbounded value would park an `ACTIVE` permit for as long as a caller asked. The ceiling's relation to `commitTimeout` moves to a `const _: () = assert!(…)`, so lowering it under 600s is a build error rather than a test someone runs later — a slow `pre-commit` hook must be cut short by its own limit, never by the transport. - The exec envelope's `version != 1` guard, which nothing covered (the read path's is covered). It asserts `BackendVersion` specifically, because `RepositoryRouter` keys its fall-back-to-native decision on that case and any other error strands the user. The request asks for `git init spawned-anyway` and the test asserts that directory does not exist, so a guard that ever moves below the spawn fails here rather than passing on an error string. - `drain_capped`'s cap AND its drain-past-cap property, the second through a REAL pipe. The cap bounds what is kept; refusing to read past it would block the child on a full pipe buffer forever — a hang, not a truncation. The test's writer thread only completes if the reader kept going. - `latin1_bytes`' rejection branch, unreachable from the existing round trip (every byte 0x00-0xFF is representable), plus the boundary either side. Swift (`AgentVCSIntegrationTests`): - Both `writer(context:reader:)` guards, driven through `RepositoryRouter` — the only thing that can mint a `RepositoryContext`, and the real caller, so this also pins that the router does not swallow either error. The closed connection must report `connectionLost` and specifically NOT `VCSError.backendVersion`, the one error `writer(for:)` falls back to native writes on: a closed connection is not a pre-upgrade agent, and writing natively there would route around a connection the caller believes it is using. - A capable agent actually ROUTING a write, asserted by reading the commit back out of the repository. Deliberately pins the DECISION rather than the `writes` count that currently encodes it, so replacing that count with an exec-service check (TODOS item 4) will not invalidate it. - The write path's transport-failure mapping end to end through a real `VCSWriting` — the property `CommandResult.outcomeUnknown` exists for, which until now was only exercised against hand-built `CommandResult`s that cannot catch the writer losing the distinction on the way through. cargo test -p wr-agent: 19 vcs tests pass, clippy and fmt clean. AgentVCSIntegrationTests: 19 pass. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01XLpTyiW62gUJERhjwjcFFs --- .../AgentVCSIntegrationTests.swift | 112 ++++++++++++++++++ vcs/crates/wr-agent/src/vcs.rs | 101 ++++++++++++++++ 2 files changed, 213 insertions(+) diff --git a/macapp/WorkroomAppTests/AgentVCSIntegrationTests.swift b/macapp/WorkroomAppTests/AgentVCSIntegrationTests.swift index 31ea2ff8a..fce115e40 100644 --- a/macapp/WorkroomAppTests/AgentVCSIntegrationTests.swift +++ b/macapp/WorkroomAppTests/AgentVCSIntegrationTests.swift @@ -694,4 +694,116 @@ final class AgentVCSIntegrationTests: XCTestCase { } await connection.close() } + + /// Both `writer(context:reader:)` guards, which nothing reached before: they are the two ways a + /// writer can be refused BEFORE any command is built, and each has a distinct caller contract. + /// Driven through `RepositoryRouter`, which is the only thing that can mint a `RepositoryContext` + /// — and is the real caller, so this also pins that the router does not swallow either error. + /// + /// The host check is the one that matters for correctness rather than tidiness. The router keys on + /// host-qualified identity precisely so a remote repository cannot collide with a local one at the + /// same path (#201); a connection answering for another host's context would write to the wrong + /// machine's repository at a path that exists on both. + func testAWriterIsRefusedForAnotherHostAndForAClosedConnection() async throws { + let root = try gitRepo() + let connection = try await connect() + + // This connection's host is `.local`, so a REMOTE context handed to it must be refused. Wired as + // the remote factory to get one built at all. + let elsewhere = try RepositoryLocation.remote(host: UUID(), path: root.path) + let remoteRouter = RepositoryRouter( + remoteReader: { try connection.reader(context: $0) }, + remoteWriter: { try connection.writer(context: $0, reader: $1) }) + try remoteRouter.register( + .init(location: elsewhere, backend: .git, sharedLocation: elsewhere)) + do { + _ = try await remoteRouter.writer(for: elsewhere) + XCTFail("a writer was issued for another host's context") + } catch { + XCTAssertEqual(error as? HostConnectionError, .mismatchedContext, "got \(error)") + } + + // Closed: `connectionLost`, NOT `notDispatched`. The distinction is the one P1 #2 established — + // the refusal happens before anything reaches the socket, but the caller learns it by asking for + // a writer rather than by a request coming back, so it is a lost connection. + // + // It must also NOT be `VCSError.backendVersion`, the one error `writer(for:)` falls back to + // native writes on: a closed connection is not a pre-upgrade agent, and silently writing + // natively here would route around a connection the caller believes it is using. + let location = try await RepositoryLocation.local(root.path) + let localRouter = RepositoryRouter( + localReader: { try connection.reader(context: $0) }, + localWriter: { try connection.writer(context: $0, reader: connection.reader(context: $0)) }) + try localRouter.register(.init(location: location, backend: .git, sharedLocation: location)) + await connection.close() + do { + _ = try await localRouter.writer(for: location) + XCTFail("a writer was issued on a closed connection") + } catch { + XCTAssertEqual(error as? HostConnectionError, .connectionLost, "got \(error)") + } + } + + /// A live agent must actually ROUTE writes, not merely fail to refuse them. Paired with the two + /// fallback tests above, which cover the refusal: together they pin the DECISION rather than the + /// capability number that currently encodes it, so replacing that number with an exec-service + /// check (TODOS item 4) does not invalidate this. + /// + /// `localWriter` is the only writer wired and it cannot throw `backendVersion`, so a fallback here + /// is impossible — the commit below is the agent's own child process or nothing. + func testACapableAgentRoutesWritesThroughTheAgentRatherThanFallingBack() async throws { + let root = try gitRepo() + try "second\n".write(to: root.appendingPathComponent("file"), atomically: true, encoding: .utf8) + let connection = try await connect() + let location = try await RepositoryLocation.local(root.path) + let router = RepositoryRouter( + localReader: { try connection.reader(context: $0) }, + localWriter: { try connection.writer(context: $0, reader: connection.reader(context: $0)) }) + try router.register(.init(location: location, backend: .git, sharedLocation: location)) + + let result = await (try await router.writer(for: location)).commit( + request: VCSCommitRequest( + message: "agent-routed commit", + files: [ChangedFile(path: "file", change: .modified, oldPath: nil)], mode: .commit)) + guard case .ok = result else { return XCTFail("agent-routed commit failed: \(result)") } + + let log = try run("git", ["log", "-1", "--format=%s"], at: root) + XCTAssertEqual(log.trimmingCharacters(in: .whitespacesAndNewlines), "agent-routed commit") + await connection.close() + } + + /// The write path's transport-failure mapping, end to end through the real client: a write issued + /// on a connection that dies UNDER it must classify as `.outcomeUnknown` and must not offer a + /// retry of itself. + /// + /// This is the property the whole `CommandResult.outcomeUnknown` partition exists for, and nothing + /// exercised it through an actual `VCSWriting` before — the unit tests build the `CommandResult` + /// by hand, which cannot catch the writer losing the distinction on the way through. + func testAWriteOnALostConnectionReportsAnUnknownOutcomeAndOffersNoSelfRetry() async throws { + let root = try gitRepo() + let connection = try await connect() + let location = try await RepositoryLocation.local(root.path) + let router = RepositoryRouter( + localReader: { try connection.reader(context: $0) }, + localWriter: { try connection.writer(context: $0, reader: connection.reader(context: $0)) }) + try router.register(.init(location: location, backend: .git, sharedLocation: location)) + // Obtained while the connection is live, so both guards above pass and the failure can only come + // from the write itself — which is the path under test. + let writer = try await router.writer(for: location) + await connection.close() + + let result = await writer.fetch(remote: "origin") + guard case .failed(let failure) = result else { + return XCTFail("a write on a closed connection reported success: \(result)") + } + // Never `.launchFailed`: that asserts nothing ran, and a dispatched command may well have. + // Never `.other`: that offers a retry of the verb that failed. + guard case .outcomeUnknown = failure else { + return XCTFail("transport failure classified as \(failure)") + } + // Fetch is idempotent, so it is offered back — but never escalated to a write. + XCTAssertEqual(VCSSyncPresenter.retryAction(for: failure, lastAction: .fetch), .fetch) + XCTAssertEqual(VCSSyncPresenter.retryAction(for: failure, lastAction: .push), .fetch) + XCTAssertNil(VCSSyncPresenter.retryAction(for: failure, lastAction: nil)) + } } diff --git a/vcs/crates/wr-agent/src/vcs.rs b/vcs/crates/wr-agent/src/vcs.rs index 816680a44..05f235758 100644 --- a/vcs/crates/wr-agent/src/vcs.rs +++ b/vcs/crates/wr-agent/src/vcs.rs @@ -24,6 +24,11 @@ const MAX_HISTORY_LIMIT: usize = 10000; /// hostile/buggy request from wedging an exec thread indefinitely; the 32-slot `ACTIVE` permit /// already bounds concurrency, this bounds duration. const MAX_EXEC_TIMEOUT_MS: u64 = 610_000; +/// The ceiling must stay above `CLIVCSWriter.commitTimeout` (600s), or the transport would cut a +/// legitimate commit short — one with a slow `pre-commit` hook — before its own limit applied, and +/// the failure would read as a transport fault rather than the timeout it is. Checked at COMPILE +/// time: a lowered ceiling is a build error, not a test that someone runs later. +const _: () = assert!(MAX_EXEC_TIMEOUT_MS > 600_000); /// Per-stream cap, matching `StatusCommandRunner.maxBytes`'s default on the Swift side. const MAX_EXEC_STREAM: usize = 4 * 1024 * 1024; static ACTIVE: AtomicUsize = AtomicUsize::new(0); @@ -1096,4 +1101,100 @@ mod tests { assert!(failed.is_err()); std::fs::remove_dir_all(&root).unwrap(); } + + /// The clamp is the only thing between a client's `timeout_ms` and a thread that waits on it, + /// and both ends of it matter: 0 would make every command time out before it started, and an + /// unbounded value would park an `ACTIVE` permit for as long as the caller asked. Asserted + /// through the clamp expression `exec` actually uses, so a changed bound fails here. + #[test] + fn the_exec_timeout_clamp_holds_at_both_ends() { + let clamp = |ms: u64| Duration::from_millis(ms.clamp(1, MAX_EXEC_TIMEOUT_MS)); + // Zero is raised to 1ms, never passed through: a 0ms deadline kills the child immediately. + assert_eq!(clamp(0), Duration::from_millis(1)); + assert_eq!(clamp(1), Duration::from_millis(1)); + assert_eq!(clamp(u64::MAX), Duration::from_millis(MAX_EXEC_TIMEOUT_MS)); + assert_eq!( + clamp(MAX_EXEC_TIMEOUT_MS + 1), + Duration::from_millis(MAX_EXEC_TIMEOUT_MS) + ); + // In range, it is the caller's value untouched. + assert_eq!(clamp(5_000), Duration::from_millis(5_000)); + } + + /// The exec envelope carries its OWN version, and it is checked before anything is spawned. The + /// read path's guard is covered by `version_and_path_errors_are_explicit`; this is the write + /// path's, and it has to fail as `BackendVersion` specifically — `RepositoryRouter` keys its + /// fall-back-to-native decision on that case, so any other error would strand the user instead. + #[test] + fn an_exec_request_with_an_unsupported_version_is_refused_before_spawning() { + let root = git_repo("exec-version"); + let request = json!({ + "version": 2, + "kind": "exec", + "executable": "git", + // A command with an observable side effect: if the guard ever moves after the spawn, + // this file appears and the assertion below catches it. + "args": ["init", "-q", "spawned-anyway"], + "dir": root.to_str().unwrap(), + "timeout_ms": 5000, + }); + let reply = execute(&serde_json::to_vec(&request).unwrap()); + assert!( + reply["error"]["BackendVersion"].as_str().is_some(), + "expected BackendVersion, got {reply}" + ); + assert!( + !root.join("spawned-anyway").exists(), + "the child ran anyway" + ); + std::fs::remove_dir_all(&root).unwrap(); + } + + /// Two separate properties, and the second is the one that matters. The cap bounds what is + /// KEPT; the loop must go on reading past it, because the child writes into a pipe whose buffer + /// is ~64 KiB — stop reading and the child blocks on write forever, which is a hang, not a + /// truncation. `StatusCommandRunner.readCapped` has the same shape for the same reason. + #[test] + fn drain_capped_bounds_what_it_keeps_and_still_reads_to_eof() { + // A source larger than the cap AND larger than any pipe buffer. + let cap = 1024; + let source = vec![b'x'; 256 * 1024]; + let sink = Mutex::new(Vec::new()); + drain_capped(source.as_slice(), cap, &sink); + assert_eq!(sink.lock().unwrap().len(), cap, "the cap did not hold"); + + // Drained to EOF through a REAL pipe, which is where refusing to read past the cap would + // deadlock rather than merely truncate. The writer completes only if the reader kept going. + let (reader, mut writer) = std::io::pipe().unwrap(); + let payload = vec![b'y'; 512 * 1024]; + let expected = payload.len(); + let pump = std::thread::spawn(move || { + std::io::Write::write_all(&mut writer, &payload).unwrap(); + expected + }); + let piped = Mutex::new(Vec::new()); + drain_capped(reader, cap, &piped); + assert_eq!( + pump.join().unwrap(), + expected, + "the writer never finished, so the reader stopped short of EOF" + ); + assert_eq!(piped.lock().unwrap().len(), cap); + } + + /// The rejection branch, which the round-trip test above cannot reach: every byte 0x00-0xFF is + /// representable, so only a client that sent something OTHER than a Latin-1-decoded string gets + /// here. Refusing is the point — truncating a pathspec payload would stage the wrong files. + #[test] + fn latin1_bytes_rejects_a_character_outside_the_byte_range() { + for text in ["\u{0100}", "é\u{20AC}", "ok-then-\u{1F600}"] { + assert!( + latin1_bytes(text).is_err(), + "accepted a non-Latin-1 payload: {text:?}" + ); + } + // The boundary either side, to pin where the rejection starts. + assert!(latin1_bytes("\u{00FF}").is_ok()); + assert!(latin1_bytes("\u{0100}").is_err()); + } } From f102e271f70c4bead2884990c42ceecf68dbbc6e Mon Sep 17 00:00:00 2001 From: Joel Moss Date: Fri, 18 Sep 2026 21:16:33 +0100 Subject: [PATCH 07/13] fix(wr-agent): kill the descendant tree, and never wait unbounded on a kill MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two TODOS items, one change, because they share a root: `kill(-pid, …)` reaches a process GROUP, and a descendant that calls `setsid` has left it. **The tree.** Native has covered this since `ProcessTree` was written — its own comment says "helpers spawned by git/gh can outlive the parent" — and the agent had only the group kill. `descendants`/`kill_recorded` are the Rust half, via `pgrep -P` for the reason that file records: `proc_listchildpids`' return value is ambiguous across sources, and mis-reading it would aim a SIGKILL at the wrong pid, which is worse than missing a child. **The part native gets wrong too, and this does not.** The walk cannot happen at kill time. A `setsid` descendant keeps its real ppid until its parent dies, so `pgrep -P` finds it right up to that moment — and not after, because the orphan re-parents to init and both handles are gone at once: the group it left, and a lineage that no longer leads back to us. `killTree` is called at `timeout + 2`, by which point a `git` that exited on SIGTERM has long been reaped, so it walks an empty tree. Here the tree is SNAPSHOTTED when SIGTERM is sent and the recorded pids are what gets SIGKILLed later. The group kill is kept alongside rather than replaced: the tree misses a process that re-parented before the snapshot, the group misses one that changed group, and the extra call is one syscall. The trade is a slightly wider PID-reuse window than native's, since the pids were read earlier. Native accepted the same trade at `timeout + 2` for the same reason — the alternative is leaving a process running against the user's repository after the call that started it has returned. **The wait.** `break 'wait child.wait()` was unbounded. The loop now polls to a `REAP_GRACE` deadline after the SIGKILL and reports rather than blocking: `timed_out` is already true on that path, so the client classifies it exactly as it would a child we did reap — the outcome is the same fact, and the only difference was whether this thread waited forever to state it. Deliberately not `outcomeUnknown`: we know what happened to this command. The handle moves to a detached thread, because Rust's `Child::drop` does not wait and dropping it would leave a zombie nothing ever reaps. Tests: a `setsid` grandchild that writes a marker after the whole escalation window, asserted absent — it fails against the group-only kill, and it failed against the first draft of this fix too, which walked the tree at kill time. That is what turned up the snapshot requirement. Plus the walk's init floor and termination. The new test needs a real process table: under a sandbox that hides other processes `pgrep` returns nothing and it fails indistinguishably from the bug. Recorded in its doc comment, since it cost half an hour to diagnose once. cargo test -p wr-agent: 174 pass, clippy and fmt clean. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01XLpTyiW62gUJERhjwjcFFs --- vcs/crates/wr-agent/src/vcs.rs | 192 ++++++++++++++++++++++++++++++++- 1 file changed, 188 insertions(+), 4 deletions(-) diff --git a/vcs/crates/wr-agent/src/vcs.rs b/vcs/crates/wr-agent/src/vcs.rs index 05f235758..2f927837d 100644 --- a/vcs/crates/wr-agent/src/vcs.rs +++ b/vcs/crates/wr-agent/src/vcs.rs @@ -332,6 +332,76 @@ fn drain_capped(mut source: impl Read, cap: usize, sink: &Mutex>) { } } +/// Direct children of `pid`, via `/usr/bin/pgrep -P` — the exact mechanism (and the exact reasoning) +/// of native's `ProcessTree.childPids`: `proc_listchildpids`' return value is ambiguous across +/// sources (bytes vs count), and mis-reading it would target the wrong pid for a SIGKILL, which is a +/// far worse failure than missing a child. Empty on any error. +fn child_pids(pid: i32) -> Vec { + let Ok(output) = Command::new("/usr/bin/pgrep") + .args(["-P", &pid.to_string()]) + .stderr(Stdio::null()) + .output() + else { + return Vec::new(); + }; + String::from_utf8_lossy(&output.stdout) + .split_whitespace() + .filter_map(|field| field.parse::().ok()) + .filter(|&child| child > 1) + .collect() +} + +/// Every descendant of `pid`, breadth-first, nearest first. The Rust half of +/// `ProcessTree.descendants`, and split from the killing for a reason that file did not need to +/// face: this must be SNAPSHOTTED WHILE THE PARENT IS STILL ALIVE. +/// +/// A descendant that calls `setsid` leaves the process group, so `kill(-pid, …)` never reaches it — +/// but it keeps its real ppid until its parent dies, so `pgrep -P` finds it right up to that moment. +/// Once the parent is reaped the orphan re-parents to init and both handles are gone: the group it +/// left, and a lineage that no longer leads back to us. Walking the tree at KILL time therefore +/// finds nothing, which is the hole native's `killTree` also has — it is called at `timeout + 2`, +/// by which point a `git` that exited on SIGTERM has long been reaped. +/// +/// So the walk happens when SIGTERM is sent, and the recorded pids are what gets SIGKILLed later. +fn descendants(pid: i32) -> Vec { + let mut collected: Vec = Vec::new(); + if pid <= 1 { + return collected; + } + let mut seen: Vec = vec![pid]; + let mut queue = child_pids(pid); + while !queue.is_empty() { + let next = queue.remove(0); + if next <= 1 || seen.contains(&next) { + continue; + } + seen.push(next); + collected.push(next); + queue.extend(child_pids(next)); + } + collected +} + +/// SIGKILL the recorded descendants deepest first, so a parent cannot observe a child's death and +/// respawn before it is itself killed — `ProcessTree.killTree`'s ordering, for its reason. +/// +/// Carries the same small, unavoidable PID-reuse window native does, and slightly more of it because +/// these pids were read earlier: by the time this runs some may have exited, and a pid could in +/// principle have been reused. Native accepted that trade at `timeout + 2` for the same reason — +/// the alternative is leaving a process running against the user's repository after the call that +/// started it has returned. +fn kill_recorded(descendants: &[i32]) { + for pid in descendants.iter().rev() { + unsafe { libc::kill(*pid, libc::SIGKILL) }; + } +} + +/// How long to keep trying to reap a process we have SIGKILLed before giving up on it. SIGKILL +/// cannot be caught, so this is generous for the normal case; it exists for the abnormal one, where +/// the leader is unkillable (uninterruptible I/O) and the alternative is blocking this thread — and +/// therefore holding one of the 32 shared `ACTIVE` permits — forever. +const REAP_GRACE: Duration = Duration::from_secs(2); + /// Run `executable` to completion, capturing everything a `StatusCommandRunning` conformer needs /// to classify the outcome exactly as the native path would. Mirrors `StatusCommandRunner.run`'s /// SIGTERM-then-grace-then-SIGKILL shape (`CLIVCSWriter.commitTimeout`'s doc: "Killing a commit is @@ -410,20 +480,38 @@ fn run_exec( let mut timed_out = false; let mut sent_term = false; let mut sent_kill = false; + let mut killed_at: Option = None; + // Recorded at SIGTERM time, used at SIGKILL time — see `descendants`' doc for why it cannot be + // walked at the point of the kill. + let mut recorded: Vec = Vec::new(); + // `None` ⇒ we killed it and it never became reapable within `REAP_GRACE`. Reported rather than + // waited on: this used to be `break 'wait child.wait()`, an UNBOUNDED wait, and a child that had + // left the process group never received the SIGKILL above it, so that wait had nothing to wait + // for. Codex reproduced 4.1s on a 100ms timeout with one `os.setpgid`. let status = 'wait: loop { if let Ok(Some(status)) = child.try_wait() { - break 'wait status; + break 'wait Some(status); } let elapsed = start.elapsed(); if elapsed >= timeout { timed_out = true; if !sent_term { sent_term = true; + // Snapshot BEFORE signalling: SIGTERM may reap the leader within microseconds, and + // an orphaned `setsid` descendant is unreachable from that moment on. + recorded = descendants(pid); unsafe { libc::kill(-pid, libc::SIGTERM) }; } else if elapsed >= timeout + Duration::from_secs(2) { - sent_kill = true; - unsafe { libc::kill(-pid, libc::SIGKILL) }; - break 'wait child.wait().map_err(io)?; + if !sent_kill { + sent_kill = true; + // Recorded tree first, then group: the tree reaches a descendant that left the + // group, the group reaches one that spawned after the snapshot. + kill_recorded(&recorded); + unsafe { libc::kill(-pid, libc::SIGKILL) }; + killed_at = Some(Instant::now()); + } else if killed_at.is_some_and(|at| at.elapsed() >= REAP_GRACE) { + break 'wait None; + } } } std::thread::sleep(Duration::from_millis(10)); @@ -450,6 +538,10 @@ fn run_exec( while Instant::now() < kill_at { std::thread::sleep(Duration::from_millis(10)); } + // The recorded set, not a fresh walk: the leader is already reaped on this path, so + // `pgrep -P` would find nothing — the orphan re-parented to init the moment its parent died. + // This is the branch the snapshot exists for. + kill_recorded(&recorded); unsafe { libc::kill(-pid, libc::SIGKILL) }; } // 2. Drain bound, mirroring `StatusCommandRunner`'s `_ = drain.wait(timeout: .now() + 2)` and @@ -468,6 +560,27 @@ fn run_exec( let take = |sink: &Mutex>| { std::mem::take(&mut *sink.lock().unwrap_or_else(|e| e.into_inner())) }; + // Unreapable after SIGKILL: report what a killed process reports rather than blocking on it. + // `timed_out` is already true on this path (nothing else reaches it), so the client classifies + // this as `VCSRemoteFailure.timedOut` exactly as it would a child we did manage to reap — the + // outcome is the same fact, and the ONLY difference is whether this thread waited forever to + // state it. Deliberately not `outcomeUnknown`: we know what happened to this command. + // + // The handle is moved to a detached thread rather than dropped: Rust's `Child::drop` does not + // wait, so dropping it here would leave a zombie nothing ever reaps. Mirrors the drain detach + // below — one parked thread that ends whenever the process finally does. + let Some(status) = status else { + std::thread::spawn(move || { + let _ = child.wait(); + }); + return Ok(Captured { + stdout: take(&out_sink), + stderr: take(&err_sink), + exit_code: 137, + timed_out: true, + signaled: true, + }); + }; Ok(Captured { stdout: take(&out_sink), stderr: take(&err_sink), @@ -1197,4 +1310,75 @@ mod tests { assert!(latin1_bytes("\u{00FF}").is_ok()); assert!(latin1_bytes("\u{0100}").is_err()); } + + /// The gap against native, which `ProcessTree.killTree` has covered since it was written ("helpers + /// spawned by git/gh can outlive the parent") and the agent did not: a DESCENDANT that calls + /// `setsid` leaves the process group, so `kill(-pid, …)` never reaches it. It then goes on + /// running — and, if it inherited the pipes, holding this request's readers open — after the + /// result has released the caller's gate. + /// + /// The leader itself cannot escape: `run_exec` sets `process_group(0)`, which makes it a group + /// leader, and `setsid` is EPERM for a group leader. So the descendant is the whole of the gap, + /// and asserting the leader's timing would prove nothing. + /// + /// ENVIRONMENT: needs a real process table. Under a sandbox that hides other processes, `pgrep` + /// returns nothing, the snapshot comes back empty and this fails in a way indistinguishable from + /// the bug — measured, not guessed. If it fails, check `descendants()` sees anything at all + /// before believing the kill path is broken. + #[test] + fn exec_kills_a_descendant_that_escaped_the_process_group() { + let root = git_repo("setsid-escape"); + let marker = root.join("still-alive"); + // The grandchild leaves the group, waits out the whole escalation, and only THEN writes. + // If it is still alive at that point the marker appears, which is the failure this catches. + let program = format!( + "import os,sys,time\n\ + if os.fork() == 0:\n\ + \x20 os.setsid()\n\ + \x20 time.sleep(6)\n\ + \x20 open({:?}, 'w').close()\n\ + \x20 sys.exit(0)\n\ + time.sleep(30)\n", + marker.to_str().unwrap() + ); + let captured = run_exec( + &root, + "python3", + &["-c".into(), program], + // Not 100ms: python's own start-up is ~50ms and the fork follows it, so a tighter + // deadline races the grandchild into existence and the snapshot finds an empty tree — + // which looks exactly like the bug this asserts against. + Duration::from_millis(1500), + None, + &[("PATH", "/usr/bin:/bin")], + ) + .unwrap(); + assert!(captured.timed_out, "the timeout was not reported"); + + // Past the grandchild's own sleep, so its write would have happened by now if it survived. + std::thread::sleep(Duration::from_secs(6)); + assert!( + !marker.exists(), + "a setsid descendant outlived the kill and kept running" + ); + std::fs::remove_dir_all(&root).unwrap(); + } + + /// `descendants` walks parsed `pgrep` output, so it must terminate on shapes a process table + /// cannot produce but a parse can — and it must never reach init. A SIGKILL aimed at pid 1 is + /// the worst outcome a bug here could have, so the floor is enforced by value. + #[test] + fn the_descendant_walk_refuses_init_and_terminates() { + assert!(descendants(0).is_empty()); + assert!(descendants(1).is_empty()); + assert!(descendants(-1).is_empty()); + // An impossible pid has no children, so the walk ends immediately rather than looping. + assert!( + child_pids(i32::MAX).is_empty(), + "pgrep invented children for an impossible pid" + ); + assert!(descendants(i32::MAX).is_empty()); + // Killing an empty set is a no-op, not a signal to the current process group. + kill_recorded(&[]); + } } From 0a344f0b78f30edc65344b8f1f3e7c70fe76d88a Mon Sep 17 00:00:00 2001 From: Joel Moss Date: Fri, 18 Sep 2026 21:19:47 +0100 Subject: [PATCH 08/13] fix(macapp): gate agent writes on the exec service, not a method count MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `writes: 8` described nothing on the side that answered it. The number counted methods on `LocalVCSWriting`, a protocol that exists only in the app; wr-agent implements one generic exec service and has never had eight write methods to report. Nothing could keep the two in step, and the client compared them for equality — so adding a ninth method to that Swift protocol, a change the exec passthrough fully supports, would have made a completely capable agent look incapable and silently dropped every user to native writes, with no log line. The agent now reports `exec`, its exec service's own wire version, and the client requires `>=` the version it sends. `>=` rather than `==` so an agent that gains a version 2 does not refuse a client speaking 1: whether it still accepts that request is the agent's call, and it answers it on the request itself (`exec`'s `version != 1` guard), which is the only place with the information to decide. Nothing has shipped with `writes`, so it is removed rather than accepted alongside — the whole stack is still draft. The integration test written for this yesterday asserts the ROUTING decision (a capable agent commits through the agent; an incapable one falls back) rather than the number that encoded it, and passes unchanged across the swap. That was the point of writing it that way. cargo test -p wr-agent: 21 vcs tests pass. AgentVCS{Integration,Protocol}Tests: 31 pass. app-lint clean. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01XLpTyiW62gUJERhjwjcFFs --- TODOS.md | 14 +++++----- .../WorkroomApp/Core/AgentVCSConnection.swift | 27 +++++++++++++------ vcs/crates/wr-agent/src/vcs.rs | 24 ++++++++++++++--- 3 files changed, 48 insertions(+), 17 deletions(-) diff --git a/TODOS.md b/TODOS.md index 384a5b9db..2ee52b066 100644 --- a/TODOS.md +++ b/TODOS.md @@ -1146,12 +1146,14 @@ are fixed in `ed51faa9`; each entry below was reproduced or read off the code, n explicitly rejected group-based kill because "helpers spawned by git/gh can outlive the parent". `run_exec` has only `kill(-pid, …)`. Same gap as (2), and the two share a fix. -4. **`writes: 8` is not a capability.** `AgentVCSConnection.writeMethodCount` counts methods on the - client-only `LocalVCSWriting` protocol; wr-agent implements ONE generic exec service and does - nothing the number could describe. Adding a ninth method — a Swift-only change the passthrough - already supports — silently drops every user to native writes on a fully capable agent, with no - log line. Gate on the exec service's presence/version instead. (`reads: 9` is fine; those are nine - real agent-side methods.) +4. ~~**`writes: 8` is not a capability.**~~ **Fixed.** The count described a CLIENT-side Swift + protocol (`LocalVCSWriting`); wr-agent implements one generic exec service and never had eight + write methods to report, so nothing could keep the number true — and the client compared it for + equality, so adding a ninth method there would have dropped every user to native writes on a + fully capable agent with no log line. Replaced by `exec`, the exec service's own wire version, + compared `>=` so a future version 2 doesn't refuse a client speaking 1 (the agent answers that on + the request itself, which is the only place that can know). (`reads: 9` is fine and stays; those + are nine real agent-side methods.) 5. **Two caps that can't both be satisfied.** `MAX_EXEC_STREAM` is 4 MiB per stream, so two capped streams JSON-escaped control-heavy can exceed `MAX_RESPONSE` (16 MiB); `send` then replaces the diff --git a/macapp/WorkroomApp/Core/AgentVCSConnection.swift b/macapp/WorkroomApp/Core/AgentVCSConnection.swift index c344d176a..a353553bd 100644 --- a/macapp/WorkroomApp/Core/AgentVCSConnection.swift +++ b/macapp/WorkroomApp/Core/AgentVCSConnection.swift @@ -23,15 +23,14 @@ final class AgentVCSConnection: HostServiceConnection, @unchecked Sendable { /// stream id as a protocol violation and tearing down every OTHER in-flight request too. private var abandoned: Set = [] /// Negotiated once in `connect()`, before this connection is shared with any other caller. - /// `writes` is absent on a still-running pre-upgrade agent that answers `reads` but has no VCS + /// `exec` is absent on a still-running pre-upgrade agent that answers `reads` but has no VCS /// write service at all — `writer(context:reader:)` treats that as `VCSError.backendVersion`, /// the same signal `RepositoryRouter` already falls back to native writes on. private var _capabilities: AgentVCSCapabilities? private var capabilities: AgentVCSCapabilities? { lock.withLock { _capabilities } } - /// The write methods `LocalVCSWriting` declares — matches wr-agent's `"writes"` capability count - /// (`vcs.rs`'s `capabilities` reply). Kept as one literal so a protocol change updates both ends - /// deliberately rather than by coincidence. - private static let writeMethodCount = 8 + /// The exec wire version this client speaks — the literal `AgentExecRequest.version` it sends, so + /// the two cannot drift. + private static let execVersion = 1 private init(host: HostID, descriptor: Int32) { self.host = host @@ -130,7 +129,11 @@ final class AgentVCSConnection: HostServiceConnection, @unchecked Sendable { func writer(context: RepositoryContext, reader: VCSProviding) throws -> VCSWriting { guard context.location.host == host else { throw HostConnectionError.mismatchedContext } guard lock.withLock({ !closed }) else { throw HostConnectionError.connectionLost } - guard capabilities?.writes == Self.writeMethodCount else { + // Presence and version of the SERVICE, not a count of the caller's own methods. `>=` rather + // than `==` so an agent that gains a version 2 does not refuse a client speaking 1 — the agent + // is what decides whether it still accepts this request, and it answers that on the request + // itself (`exec`'s `version != 1` guard), which is the only place that can know. + guard let exec = capabilities?.exec, exec >= Self.execVersion else { throw VCSError.backendVersion("Agent does not support VCS writes.") } let engine = CLIVCSWriter( @@ -307,8 +310,16 @@ final class AgentVCSConnection: HostServiceConnection, @unchecked Sendable { struct AgentVCSCapabilities: Decodable { let version: Int let reads: Int - /// Absent on a still-running pre-upgrade agent that predates the VCS write service. - let writes: Int? + /// The exec service's wire version, or nil on a still-running pre-upgrade agent that predates the + /// service entirely. + /// + /// Replaces a `writes` COUNT, which counted methods on `LocalVCSWriting` — a protocol that exists + /// only in this process. wr-agent implements one generic exec service and never had eight write + /// methods to report, so the number described nothing on the answering side and nothing could keep + /// it true: adding a ninth method here, a change the passthrough fully supports, made the equality + /// check below fail against a completely capable agent and silently dropped every user to native + /// writes with no log line. + let exec: Int? } struct AgentVCSRequest: Encodable, Sendable { diff --git a/vcs/crates/wr-agent/src/vcs.rs b/vcs/crates/wr-agent/src/vcs.rs index 2f927837d..855d469c9 100644 --- a/vcs/crates/wr-agent/src/vcs.rs +++ b/vcs/crates/wr-agent/src/vcs.rs @@ -23,6 +23,9 @@ const MAX_HISTORY_LIMIT: usize = 10000; /// `CLIVCSWriter.commitTimeout` (Swift) is 600s, the longest legitimate write timeout. Bounds a /// hostile/buggy request from wedging an exec thread indefinitely; the 32-slot `ACTIVE` permit /// already bounds concurrency, this bounds duration. +/// The exec service's wire version, reported in `capabilities` so a client can tell a capable agent +/// from one that predates the service. A version, not a count — see the `capabilities` reply. +const EXEC_SERVICE_VERSION: u32 = 1; const MAX_EXEC_TIMEOUT_MS: u64 = 610_000; /// The ceiling must stay above `CLIVCSWriter.commitTimeout` (600s), or the transport would cut a /// legitimate commit short — one with a slow `pre-commit` hook — before its own limit applied, and @@ -650,7 +653,16 @@ fn read(request: Request) -> model::Result { )); } if request.method == "capabilities" { - return Ok(json!({"version": 1, "reads": 9, "writes": 8})); + // `exec` is the exec service's own version, NOT a method count. The count it replaced + // (`writes: 8`) described a CLIENT-side Swift protocol: wr-agent implements one generic + // exec service and has never had eight write methods to report. Nothing here could keep + // that number true, and the client compared it for equality — so adding a ninth method to + // that Swift protocol, a change this side fully supports, silently dropped every user to + // native writes with no log line. + // + // The highest version this agent speaks. `exec` rejects anything but 1 today; a future + // version bumps this and decides then whether it still accepts 1. + return Ok(json!({"version": 1, "reads": 9, "exec": EXEC_SERVICE_VERSION})); } if request.limit.is_some_and(|limit| limit > MAX_HISTORY_LIMIT) { return Err(VcsError::PartialData(format!( @@ -1108,10 +1120,16 @@ mod tests { } #[test] - fn capabilities_reports_both_read_and_write_counts() { + fn capabilities_reports_the_read_count_and_the_exec_service_version() { let reply = execute(br#"{"version":1,"method":"capabilities"}"#); assert_eq!(reply["result"]["reads"], 9); - assert_eq!(reply["result"]["writes"], 8); + // A VERSION, not a count of anything. The client requires `>=` its own, so this must never + // become a number that describes the caller's side — that was the defect it replaced. + assert_eq!(reply["result"]["exec"], EXEC_SERVICE_VERSION); + assert!( + reply["result"].get("writes").is_none(), + "the method count is gone; nothing here could keep it true" + ); } /// The child's environment is EXACTLY the request's map — nothing of the daemon's own leaks From e1db4999472b25b1e62dd9beae5f76f835e4cd0f Mon Sep 17 00:00:00 2001 From: Joel Moss Date: Fri, 18 Sep 2026 21:22:09 +0100 Subject: [PATCH 09/13] fix(wr-agent): truncate an oversized exec reply instead of voiding it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `MAX_EXEC_STREAM` allows 4 MiB per stream and `MAX_RESPONSE` ceilings the reply at 16 MiB, but a control byte JSON-escapes to six — so two full streams reach 48 MiB of wire form and `send` replaced the ENTIRE reply with `PartialData`. That discards the exit status of a command that ran to completion: a known outcome reported as an unknown one, which is the failure class the `outcomeUnknown` partition was added to stop inventing. Codex reproduced it with 3 MiB of `0x01`. Truncated in `exec` now, so `send`'s blanket replacement is never reached. That replacement stays as-is for reads, where it is right — half a history is not a history — and this is the one reply where a partial answer is strictly better than none, because the exit code is the answer and the output is evidence. By MEASURED escaped length, not by lowering the stream cap. Sizing `MAX_EXEC_STREAM` for worst-case expansion would mean ~1.3 MiB per stream to survive a case that essentially never occurs, permanently cutting ordinary output to buy nothing; measuring costs one pass over the text and keeps everything that fits. stderr gets first call on the budget, capped at half, with stdout taking the slack. It is what `CLIVCSWriter.classify` matches on, so losing it turns a diagnosable failure into `.other` with git's own words missing — and on every ordinary command it is tiny, so stdout gets nearly all of the budget anyway. Head-truncated and silent, matching `drain_capped` and native's `readCapped`: git and jj put the classifying line first, and injecting a "truncated" notice into this text could only create a false positive against the markers that classifier matches. Tests: Codex's 3 MiB fixture, asserting the exit code survives and the serialized reply lands under the ceiling, with a guard that fails if the fixture ever stops exceeding the budget. Plus the accounting itself checked against serde's own output, and truncation on a character boundary (a byte-wise cut would panic on the slice). cargo test -p wr-agent: 23 vcs tests pass, clippy and fmt clean. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01XLpTyiW62gUJERhjwjcFFs --- TODOS.md | 13 +-- vcs/crates/wr-agent/src/vcs.rs | 154 ++++++++++++++++++++++++++++++++- 2 files changed, 160 insertions(+), 7 deletions(-) diff --git a/TODOS.md b/TODOS.md index 2ee52b066..bf37426fe 100644 --- a/TODOS.md +++ b/TODOS.md @@ -1155,11 +1155,14 @@ are fixed in `ed51faa9`; each entry below was reproduced or read off the code, n the request itself, which is the only place that can know). (`reads: 9` is fine and stays; those are nine real agent-side methods.) -5. **Two caps that can't both be satisfied.** `MAX_EXEC_STREAM` is 4 MiB per stream, so two capped - streams JSON-escaped control-heavy can exceed `MAX_RESPONSE` (16 MiB); `send` then replaces the - whole reply with `PartialData`, discarding a completed command's exit status. Codex reproduced it - with 3 MiB of `0x01`. Native truncates and still classifies. Size the wire form for worst-case - expansion, or truncate while preserving status. +5. ~~**Two caps that can't both be satisfied.**~~ **Fixed.** `MAX_EXEC_STREAM` (4 MiB/stream) against + `MAX_RESPONSE` (16 MiB), with a control byte escaping to six — so `send` replaced the whole reply + with `PartialData` and discarded a completed command's exit status, reporting a known outcome as + an unknown one. Now truncated in `exec` itself, by MEASURED escaped length rather than by lowering + the stream cap (which would have dropped ordinary output to ~1.3 MiB to survive a worst case that + almost never occurs). stderr gets first call on the budget — it is what `classify` matches on — + and stdout takes the slack, which is nearly all of it in practice. Head-truncated and silent, + matching `drain_capped` and native's `readCapped`. 6. **The 1 MiB request cap regresses large selective commits.** `StatusCommandRunning.run(stdin:)` exists partly to sidestep `E2BIG`; routing through a single un-chunked envelope reintroduces a diff --git a/vcs/crates/wr-agent/src/vcs.rs b/vcs/crates/wr-agent/src/vcs.rs index 855d469c9..c54b43b54 100644 --- a/vcs/crates/wr-agent/src/vcs.rs +++ b/vcs/crates/wr-agent/src/vcs.rs @@ -599,6 +599,49 @@ fn run_exec( }) } +/// Headroom left for everything in an exec reply that is not the two captured streams: the keys, +/// the exit code, the two booleans, the envelope framing, and JSON's own punctuation. Generous on +/// purpose — the cost of over-reserving is a few KiB of output, the cost of under-reserving is the +/// whole reply being replaced by an error. +const EXEC_REPLY_RESERVE: usize = 64 * 1024; + +/// How many bytes this text costs once JSON-escaped, which is what `MAX_RESPONSE` actually bounds. +/// +/// The expansion is why the two caps could not both be satisfied: a control byte serializes as +/// `\u0001`, six bytes for one, so two streams at `MAX_EXEC_STREAM` (4 MiB each) reach 48 MiB of +/// wire form against a 16 MiB ceiling. Codex reproduced it with 3 MiB of `0x01`. +fn escaped_len(text: &str) -> usize { + text.chars() + .map(|c| match c { + '"' | '\\' | '\n' | '\r' | '\t' => 2, + c if (c as u32) < 0x20 => 6, + c => c.len_utf8(), + }) + .sum() +} + +/// The longest prefix of `text` whose escaped form fits `budget`, cut on a character boundary. +/// +/// Head rather than tail, matching `drain_capped` and native's `StatusCommandRunner.readCapped`: +/// git and jj put the line that classifies a failure first, and a tail-truncated stderr would lose +/// it. Silent, also matching those two — the markers `CLIVCSWriter.classify` matches live in this +/// text, so injecting a "truncated" notice here could only create a false positive. +fn truncate_to_escaped_budget(text: &str, budget: usize) -> &str { + let mut used = 0; + for (index, c) in text.char_indices() { + let cost = match c { + '"' | '\\' | '\n' | '\r' | '\t' => 2, + c if (c as u32) < 0x20 => 6, + c => c.len_utf8(), + }; + if used + cost > budget { + return &text[..index]; + } + used += cost; + } + text +} + /// The exact inverse of `AgentCommandRunner.swift`'s `String(data:encoding:.isoLatin1)` — see /// `ExecRequest.stdin`'s doc. Every `char` a Latin-1-decoded string can contain is U+0000-U+00FF /// by construction; a value outside that range means the client sent something else, so this @@ -637,9 +680,29 @@ fn exec(request: ExecRequest) -> model::Result { stdin.as_deref(), &env, )?; + let stdout = String::from_utf8_lossy(&captured.stdout); + let stderr = String::from_utf8_lossy(&captured.stderr); + // Truncate HERE rather than letting `send` refuse the reply. `send`'s blanket + // `PartialData` replacement is right for a read — half a history is not a history — but for an + // exec it threw away the exit status of a command that ran to completion, reporting a known + // outcome as an unknown one. Native truncates the output and still classifies; so does this. + // + // stderr gets first call on the budget: it is what `CLIVCSWriter.classify` matches on, so losing + // it turns a diagnosable failure into `.other` with git's own words missing. stdout takes the + // slack, which in practice is nearly all of it — stderr is tiny on every ordinary command. + let budget = MAX_RESPONSE - EXEC_REPLY_RESERVE; + let (stdout, stderr) = if escaped_len(&stdout) + escaped_len(&stderr) > budget { + let err_budget = escaped_len(&stderr).min(budget / 2); + ( + truncate_to_escaped_budget(&stdout, budget - err_budget), + truncate_to_escaped_budget(&stderr, err_budget), + ) + } else { + (stdout.as_ref(), stderr.as_ref()) + }; Ok(json!({ - "stdout": String::from_utf8_lossy(&captured.stdout), - "stderr": String::from_utf8_lossy(&captured.stderr), + "stdout": stdout, + "stderr": stderr, "exit_code": captured.exit_code, "timed_out": captured.timed_out, "signaled": captured.signaled, @@ -1399,4 +1462,91 @@ mod tests { // Killing an empty set is a no-op, not a signal to the current process group. kill_recorded(&[]); } + + /// The two caps could not both be satisfied: `MAX_EXEC_STREAM` allows 4 MiB per stream, and a + /// control byte JSON-escapes to six, so two full streams reach 48 MiB against `MAX_RESPONSE`'s + /// 16 MiB ceiling. `send` then replaced the WHOLE reply with `PartialData` — discarding the exit + /// status of a command that ran to completion and reporting a known outcome as an unknown one. + /// + /// Codex's repro, 3 MiB of `0x01`. + #[test] + fn a_huge_control_heavy_reply_keeps_its_exit_status() { + let root = git_repo("huge-output"); + // 3 MiB of 0x01 on stdout, then a non-zero exit — the two facts that must both survive. + let program = "import sys; sys.stdout.write('\\x01' * (3 * 1024 * 1024)); sys.exit(3)"; + // `run_exec` directly rather than `execute`: the executable allowlist is git/jj only, and + // neither will emit 3 MiB of control bytes on demand. The reply's wire size is then asserted + // against the same budget `exec` applies, which is the contract under test. + let captured = run_exec( + &root, + "python3", + &["-c".into(), program.into()], + Duration::from_secs(30), + None, + &[("PATH", "/usr/bin:/bin")], + ) + .unwrap(); + assert_eq!(captured.exit_code, 3, "the child's own exit code was lost"); + assert_eq!(captured.stdout.len(), 3 * 1024 * 1024); + + // The reply `exec` would build for that capture must fit the wire ceiling, so `send` never + // reaches its blanket replacement. + let stdout = String::from_utf8_lossy(&captured.stdout); + let stderr = String::from_utf8_lossy(&captured.stderr); + let budget = MAX_RESPONSE - EXEC_REPLY_RESERVE; + assert!( + escaped_len(&stdout) > budget, + "the fixture no longer exceeds the budget, so this proves nothing" + ); + let err_budget = escaped_len(&stderr).min(budget / 2); + let kept = truncate_to_escaped_budget(&stdout, budget - err_budget); + let reply = json!({ + "version": 1, + "result": { + "stdout": kept, + "stderr": truncate_to_escaped_budget(&stderr, err_budget), + "exit_code": captured.exit_code, + "timed_out": captured.timed_out, + "signaled": captured.signaled, + } + }); + let wire = serde_json::to_vec(&reply).unwrap(); + assert!( + wire.len() <= MAX_RESPONSE, + "reply is {} bytes, over the {MAX_RESPONSE} ceiling", + wire.len() + ); + // Truncated, not emptied: the user still gets everything that fits. + assert!(!kept.is_empty(), "truncation threw the whole stream away"); + std::fs::remove_dir_all(&root).unwrap(); + } + + /// The budget accounting itself, away from multi-MiB fixtures. + #[test] + fn escaped_length_and_truncation_agree_on_the_expansion() { + // A control byte costs six on the wire, a quote two, plain ASCII one. + assert_eq!(escaped_len("\u{0001}"), 6); + assert_eq!(escaped_len("\""), 2); + assert_eq!(escaped_len("\n"), 2); + assert_eq!(escaped_len("abc"), 3); + // And that is what serde actually produces, minus the two surrounding quotes. + for sample in ["\u{0001}\u{0002}", "a\"b\\c", "plain", "tab\there", "é"] { + assert_eq!( + escaped_len(sample), + serde_json::to_string(sample).unwrap().len() - 2, + "escaped_len disagrees with serde for {sample:?}" + ); + } + + // Truncation never exceeds the budget and never splits a character. + assert_eq!( + truncate_to_escaped_budget("\u{0001}\u{0001}", 6), + "\u{0001}" + ); + assert_eq!(truncate_to_escaped_budget("\u{0001}", 5), ""); + assert_eq!(truncate_to_escaped_budget("abc", 99), "abc"); + // A multi-byte character is kept whole or dropped whole — a split would panic on the slice. + assert_eq!(truncate_to_escaped_budget("é", 1), ""); + assert_eq!(truncate_to_escaped_budget("aé", 2), "a"); + } } From 962d8cac65f249e93662f853f5fa9e4086baa207 Mon Sep 17 00:00:00 2001 From: Joel Moss Date: Fri, 18 Sep 2026 21:24:56 +0100 Subject: [PATCH 10/13] fix(macapp): keep a dropped handshake recoverable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `AgentVCSConnection.connect` wrapped every negotiation failure as `serviceUnavailable`, but `LocalAgentVCS` catches exactly `connectionLost` to spawn wr-agent and retry. So a connection that dropped mid-`capabilities` was routed around the stale-socket recovery and left the VCS service dead until the app was restarted — over a stale socket, which is the common case rather than an exotic one: the daemon leaves `session.sock` behind on any `pkill`. `connectionLost` is now rethrown unchanged. Only that case: a `capabilities` reply that arrives and reports an incompatible agent, and a handshake that times out against a HUNG one, both stay `serviceUnavailable` — a respawn cannot fix either, since the second candidate exits without binding while the first holds the single-instance flock. Same shape as the error-partition work earlier in this branch: a real distinction collapsed into one value, and the caller downstream cannot act on what it cannot see. Test drives a socket that ACCEPTS and then hangs up, which is what a dead agent's leftover socket does — a socket nothing listens on fails at connect and never reached this wrapper, which is why nothing caught this before. AgentVCSIntegrationTests: 20 pass. app-lint clean. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01XLpTyiW62gUJERhjwjcFFs --- TODOS.md | 12 ++--- .../WorkroomApp/Core/AgentVCSConnection.swift | 13 ++++++ .../AgentVCSIntegrationTests.swift | 45 +++++++++++++++++++ 3 files changed, 65 insertions(+), 5 deletions(-) diff --git a/TODOS.md b/TODOS.md index bf37426fe..e226a0a49 100644 --- a/TODOS.md +++ b/TODOS.md @@ -1175,11 +1175,13 @@ are fixed in `ed51faa9`; each entry below was reproduced or read off the code, n and writes onto separate pools or connections if it shows up in practice. Marked `ponytail:` in `AgentVCSConnection.request`. -8. **A capabilities-negotiation disconnect never reaches the agent respawn.** - `AgentVCSConnection.connect` wraps every negotiation failure as - `serviceUnavailable("VCS negotiation failed: …")`, but `LocalAgentVCS` catches exactly - `connectionLost` to spawn wr-agent and retry. A connection that drops mid-`capabilities` therefore - fails without the stale-socket recovery. Predates this work. +8. ~~**A capabilities-negotiation disconnect never reaches the agent respawn.**~~ **Fixed.** + `AgentVCSConnection.connect` flattened every negotiation failure into `serviceUnavailable`, but + `LocalAgentVCS` catches exactly `connectionLost` to spawn wr-agent and retry — so a dropped + handshake was routed around the stale-socket recovery and left the VCS service dead until the app + restarted. `connectionLost` is now rethrown unchanged; an incompatible reply or a hung agent stays + `serviceUnavailable`, since a respawn cannot fix either (the second candidate exits without + binding while the first holds the flock). 9. **Seven test gaps, all with stubs available.** Nothing covers: the exec timeout clamp boundaries; the `version != 1` guard; `drain_capped`'s 4 MiB cap and its drain-past-cap property; diff --git a/macapp/WorkroomApp/Core/AgentVCSConnection.swift b/macapp/WorkroomApp/Core/AgentVCSConnection.swift index a353553bd..42266626c 100644 --- a/macapp/WorkroomApp/Core/AgentVCSConnection.swift +++ b/macapp/WorkroomApp/Core/AgentVCSConnection.swift @@ -108,6 +108,19 @@ final class AgentVCSConnection: HostServiceConnection, @unchecked Sendable { // concurrent reader can observe a half-set value. connection.lock.withLock { connection._capabilities = capabilities } return connection + } catch HostConnectionError.connectionLost { + // Rethrown UNCHANGED, not wrapped. `LocalAgentVCS` catches exactly this case to spawn wr-agent + // and retry, and flattening it into `serviceUnavailable` routed a dropped handshake around + // that recovery entirely — leaving the VCS service dead until the app was restarted, over a + // stale socket, which is the common case rather than an exotic one (the daemon leaves + // `session.sock` behind on any `pkill`). + // + // Only this case. A `capabilities` reply that arrives and says the agent is incompatible, or a + // handshake that times out against a HUNG agent, are both still `serviceUnavailable`: a + // respawn cannot fix either, since the second candidate exits without binding while the first + // holds the single-instance flock. + await connection.close() + throw HostConnectionError.connectionLost } catch { await connection.close() throw HostConnectionError.serviceUnavailable("VCS negotiation failed: \(error)") diff --git a/macapp/WorkroomAppTests/AgentVCSIntegrationTests.swift b/macapp/WorkroomAppTests/AgentVCSIntegrationTests.swift index fce115e40..cfdf9169a 100644 --- a/macapp/WorkroomAppTests/AgentVCSIntegrationTests.swift +++ b/macapp/WorkroomAppTests/AgentVCSIntegrationTests.swift @@ -806,4 +806,49 @@ final class AgentVCSIntegrationTests: XCTestCase { XCTAssertEqual(VCSSyncPresenter.retryAction(for: failure, lastAction: .push), .fetch) XCTAssertNil(VCSSyncPresenter.retryAction(for: failure, lastAction: nil)) } + + /// A handshake that DROPS must stay `connectionLost` all the way out, because `LocalAgentVCS` + /// catches exactly that case to spawn wr-agent and retry. Flattening it into `serviceUnavailable` + /// routed a dropped handshake around the stale-socket recovery entirely — and a stale socket is + /// the common case, not an exotic one: the daemon leaves `session.sock` behind on any `pkill`. + /// + /// Driven against a socket that ACCEPTS and then closes without answering, which is the shape of a + /// dead agent's leftover socket. A socket nothing is listening on takes a different path (the + /// connect itself fails) and never reached the wrapper this covers. + func testADroppedHandshakeStaysRecoverableRatherThanBecomingUnavailable() async throws { + let dir = try root() + let path = dir.appendingPathComponent("dead.sock").path + + let fd = socket(AF_UNIX, SOCK_STREAM, 0) + XCTAssertGreaterThanOrEqual(fd, 0) + var address = sockaddr_un() + address.sun_family = sa_family_t(AF_UNIX) + let bytes = Array(path.utf8) + withUnsafeMutableBytes(of: &address.sun_path) { $0.copyBytes(from: bytes) } + let bound = withUnsafePointer(to: &address) { + $0.withMemoryRebound(to: sockaddr.self, capacity: 1) { + Darwin.bind(fd, $0, socklen_t(MemoryLayout.size)) + } + } + XCTAssertEqual(bound, 0, "could not bind the fixture socket") + XCTAssertEqual(Darwin.listen(fd, 1), 0) + // Accept one connection and immediately hang up, mid-handshake. + let accepting = Task.detached { + let peer = Darwin.accept(fd, nil, nil) + if peer >= 0 { Darwin.close(peer) } + } + defer { + accepting.cancel() + Darwin.close(fd) + } + + do { + _ = try await AgentVCSConnection.connect(host: .local, socketPath: path) + XCTFail("a connection was negotiated against a socket that hung up") + } catch { + XCTAssertEqual( + error as? HostConnectionError, .connectionLost, + "a dropped handshake must stay recoverable, got \(error)") + } + } } From 87ecde6f58df493f0baeedf7a6539bb93b5b36bd Mon Sep 17 00:00:00 2001 From: Joel Moss Date: Fri, 18 Sep 2026 21:40:04 +0100 Subject: [PATCH 11/13] feat(wr-agent): chunk oversized requests across envelopes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `StatusCommandRunning.run(stdin:)` exists partly to sidestep `E2BIG`, and routing it through a single un-chunked envelope put the ceiling straight back — a lower one, since every NUL in `CLIVCSWriter`'s pathspec escapes to six bytes on the wire. "Select all and commit" in a large repository worked natively and failed through the agent, which is a plain regression against the path the agent replaced. Requests now chunk across envelopes on one stream, mirroring how replies always have. Framed `[0x02][isFinal][bytes]` — a marker byte rather than a header on every request, because a whole request is JSON and always starts `{`, so the two are unambiguous and an UNCHUNKED request stays byte-for-byte what shipped before. The common path pays nothing for this. Gated on the agent's `exec >= 2` (bumped here): the framing is a wire change, and a pre-chunking agent would read the marker as the start of a JSON document and answer with a parse error. Below the ceiling the gate never applies. Three neighbouring properties, each found by asking what this made worse rather than whether it worked: - **A refused request must poison its stream.** Chunks keep arriving after a rejection, and reassembling that tail as a fresh request would put a SECOND reply on a stream the client has already completed — which it treats as an unknown stream id, a protocol violation that tears down every OTHER in-flight request on the connection. Poisoned streams swallow their remainder silently and clear on the final chunk. - **`is_busy()` must count partial requests.** A chunked request deliberately holds no `ACTIVE` permit until its last chunk lands, so a slow upload cannot occupy one of the 32 slots — but that left a client three chunks into five looking idle, and the daemon's idle-exit would drop the connection under it. - **Per-stream caps alone are not a bound.** 32 streams × 16 MiB is 512 MiB of peer-controlled memory. A total cap (32 MiB) sits over the per-stream one. Tests: a real 6000-file commit whose pathspec clears 1 MiB, driven through the real client and the bundled agent, asserting the commit recorded every file — a truncated payload would commit a prefix and still report ok. Plus reassembly, interleaved streams, the poisoning, and a malformed header. The fixture bug worth recording: the first draft used `.added` for the new files, but a new file on disk reports `.untracked`, and only that routes through intent-to-add. The commit then failed on pathspecs git had no index entry for — which looks exactly like a truncated payload. The `PARTIAL` tests serialize against each other on a test-local mutex: the map is process-global, and they passed alone and failed in the parallel run. For the same reason emptiness is asserted on the buffer rather than through `is_busy()`, which ORs in the dispatch count that other tests are using. cargo test -p wr-agent: 180 pass, clippy and fmt clean. make app-test (serial): 2764 pass, 1 skipped. app-lint clean. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01XLpTyiW62gUJERhjwjcFFs --- TODOS.md | 14 +- .../WorkroomApp/Core/AgentVCSConnection.swift | 81 +++++-- .../AgentVCSIntegrationTests.swift | 47 ++++ vcs/crates/wr-agent/src/vcs.rs | 220 +++++++++++++++++- 4 files changed, 342 insertions(+), 20 deletions(-) diff --git a/TODOS.md b/TODOS.md index e226a0a49..9e2867792 100644 --- a/TODOS.md +++ b/TODOS.md @@ -1164,10 +1164,16 @@ are fixed in `ed51faa9`; each entry below was reproduced or read off the code, n and stdout takes the slack, which is nearly all of it in practice. Head-truncated and silent, matching `drain_capped` and native's `readCapped`. -6. **The 1 MiB request cap regresses large selective commits.** `StatusCommandRunning.run(stdin:)` - exists partly to sidestep `E2BIG`; routing through a single un-chunked envelope reintroduces a - ceiling, and a lower one (each NUL escapes to 6 bytes). "Select all and commit" in a large repo - fails through the agent where it succeeds natively. Needs real request chunking. +6. ~~**The 1 MiB request cap regresses large selective commits.**~~ **Fixed.** Requests now chunk + across envelopes on one stream, mirroring how replies always have. Framed with a marker byte + (`0x02`) rather than a header on every request, so an UNCHUNKED request stays byte-identical and + the common path pays nothing; gated on the agent's `exec >= 2`, since a pre-chunking agent would + read the marker as the start of a JSON document. Reassembly is bounded per stream (16 MiB), in + total (32 MiB) and by stream count (32), and a refused request POISONS its stream so its + remaining chunks are swallowed — without that, the tail would reassemble as a fresh request and + put a second reply on a completed stream, which the client treats as a protocol violation that + tears down every other request on the connection. `is_busy()` counts partial requests too, or the + daemon could idle-exit under a half-sent one. 7. **The 32-slot request pool is shared between reads and writes.** A write can hold a slot for `commitTimeout` (600s) where only seconds-long reads used to compete. Exhaustion now degrades to diff --git a/macapp/WorkroomApp/Core/AgentVCSConnection.swift b/macapp/WorkroomApp/Core/AgentVCSConnection.swift index 42266626c..6759f6435 100644 --- a/macapp/WorkroomApp/Core/AgentVCSConnection.swift +++ b/macapp/WorkroomApp/Core/AgentVCSConnection.swift @@ -31,6 +31,17 @@ final class AgentVCSConnection: HostServiceConnection, @unchecked Sendable { /// The exec wire version this client speaks — the literal `AgentExecRequest.version` it sends, so /// the two cannot drift. private static let execVersion = 1 + /// The first exec service version that reassembles chunked requests. Separate from `execVersion` + /// because it gates a FRAMING capability, not the request body: a version-1 agent speaks the same + /// `AgentExecRequest` and is fully usable, it just cannot be sent one in pieces. + private static let chunkedRequestVersion = 2 + /// `MAX_ENVELOPE_PAYLOAD` in `protocol/envelope.rs`. + private static let maxEnvelopePayload = 1 << 20 + /// `MAX_REQUEST` in `vcs.rs` — the reassembled ceiling, mirroring the reply side's. + private static let maxRequest = 16 * 1024 * 1024 + /// Marks a chunked request envelope. A whole request is JSON and starts `{`, so the two are + /// unambiguous — see `REQUEST_CHUNK_MARKER`. + private static let requestChunkMarker: UInt8 = 0x02 private init(host: HostID, descriptor: Int32) { self.host = host @@ -155,6 +166,37 @@ final class AgentVCSConnection: HostServiceConnection, @unchecked Sendable { return try BoundLocalWriter(context: context, reader: reader, writer: engine) } + /// One envelope: the `Service.vcs` byte, the stream, the payload length, then the payload. + private static func envelope(stream: UInt32, payload: Data) -> Data { + var envelope = Data([2]) + for value in [stream, UInt32(payload.count)] { + var value = value.bigEndian + withUnsafeBytes(of: &value) { envelope.append(contentsOf: $0) } + } + envelope.append(payload) + return envelope + } + + /// The payloads to send for one request: exactly one, byte-identical to what shipped before, or a + /// marker-framed sequence when the body exceeds one envelope. + /// + /// Each chunk is `[marker][isFinal][bytes]`, so the agent knows both that this is a chunked + /// request and when it has all of it, without a length header it would have to trust. + private static func payloads(for bytes: Data, chunked: Bool) -> [Data] { + guard chunked else { return [bytes] } + let limit = maxEnvelopePayload - 2 + var payloads: [Data] = [] + var index = bytes.startIndex + while index < bytes.endIndex { + let end = bytes.index(index, offsetBy: limit, limitedBy: bytes.endIndex) ?? bytes.endIndex + var payload = Data([requestChunkMarker, end == bytes.endIndex ? 1 : 0]) + payload.append(bytes[index..(_ request: Request, timeout: TimeInterval = 30) async throws @@ -164,13 +206,26 @@ final class AgentVCSConnection: HostServiceConnection, @unchecked Sendable { let encoder = JSONEncoder() encoder.keyEncodingStrategy = .convertToSnakeCase let bytes = try encoder.encode(request) - // Matches the protocol's own per-envelope ceiling (`MAX_ENVELOPE_PAYLOAD` in - // `protocol/envelope.rs`) — requests are sent as ONE envelope, never chunked the way replies - // are, so raising this alone would only trade a typed `.partialData` failure here for a raw - // protocol violation there. A commit selecting many thousands of long paths (an `AgentExecRequest` - // stdin payload, `CLIVCSWriter`'s NUL-separated pathspec) could in principle exceed this; that - // would need real request chunking to lift, and is accepted as a known limit for now. - guard bytes.count <= 1 << 20 else { throw VCSError.partialData("VCS request is too large.") } + // A request larger than the protocol's per-envelope ceiling (`MAX_ENVELOPE_PAYLOAD`) is split + // across envelopes on one stream — the mirror of how replies have always been chunked. What + // makes this reachable at all is `CLIVCSWriter`'s NUL-separated pathspec, sent as an + // `AgentExecRequest` stdin payload: "select all and commit" in a large repository produced a + // payload that worked natively (stdin exists precisely to sidestep `E2BIG`) and failed through + // the agent, which is a plain regression against the path it replaced. + // + // Gated on the agent's own exec version, because the framing is a wire change: a pre-chunking + // agent would read the marker byte as the start of a JSON document and answer with a parse + // error. Below the ceiling nothing changes — the envelope is byte-identical to what shipped + // before, so the common path pays nothing for this. + let chunked = bytes.count > Self.maxEnvelopePayload + if chunked { + guard let exec = capabilities?.exec, exec >= Self.chunkedRequestVersion else { + throw VCSError.partialData("VCS request is too large for this agent.") + } + guard bytes.count <= Self.maxRequest else { + throw VCSError.partialData("VCS request is too large.") + } + } let cancellation = RequestCancellationBox() return try await withTaskCancellationHandler { try await withCheckedThrowingContinuation { continuation in @@ -208,13 +263,13 @@ final class AgentVCSConnection: HostServiceConnection, @unchecked Sendable { } writes.async { [self] in guard lock.withLock({ !closed }) else { return } - var envelope = Data([2]) - for value in [stream, UInt32(bytes.count)] { - var value = value.bigEndian - withUnsafeBytes(of: &value) { envelope.append(contentsOf: $0) } + do { + for payload in Self.payloads(for: bytes, chunked: chunked) { + try Self.send(descriptor, Self.envelope(stream: stream, payload: payload)) + } + } catch { + fail(error) } - envelope.append(bytes) - do { try Self.send(descriptor, envelope) } catch { fail(error) } } DispatchQueue.global().asyncAfter(deadline: .now() + timeout) { [weak self] in self?.timeoutStream(stream, error: HostConnectionError.connectionLost) diff --git a/macapp/WorkroomAppTests/AgentVCSIntegrationTests.swift b/macapp/WorkroomAppTests/AgentVCSIntegrationTests.swift index cfdf9169a..7aa21c324 100644 --- a/macapp/WorkroomAppTests/AgentVCSIntegrationTests.swift +++ b/macapp/WorkroomAppTests/AgentVCSIntegrationTests.swift @@ -851,4 +851,51 @@ final class AgentVCSIntegrationTests: XCTestCase { "a dropped handshake must stay recoverable, got \(error)") } } + + /// The regression this closes: `StatusCommandRunning.run(stdin:)` exists partly to sidestep + /// `E2BIG`, and routing through a single un-chunked envelope reintroduced a ceiling — a LOWER one, + /// since every NUL in the pathspec escapes to six bytes on the wire. "Select all and commit" in a + /// large repository therefore worked natively and failed through the agent. + /// + /// Drives a real commit whose pathspec payload exceeds one envelope, through the real client and + /// the bundled agent, and asserts the commit actually recorded every file. + func testACommitWhosePathspecExceedsOneEnvelopeStillCommits() async throws { + let root = try gitRepo() + // Long names so the payload clears 1 MiB well before the file count gets slow to create. + let padding = String(repeating: "p", count: 180) + var files: [ChangedFile] = [] + for index in 0..<6000 { + let name = "\(padding)-\(index).txt" + try "x\n".write( + to: root.appendingPathComponent(name), atomically: true, encoding: .utf8) + // `.untracked`, not `.added`: that is what a new file on disk reports, and it is what routes + // this through the intent-to-add step — whose payload is the SECOND oversized request this + // exercises, since it carries the same paths. + files.append(ChangedFile(path: name, change: .untracked, oldPath: nil)) + } + // The payload `CLIVCSWriter` will send, measured the way the wire measures it. + let payloadBytes = files.map(\.path).joined(separator: "\0").utf8.count + XCTAssertGreaterThan( + payloadBytes, 1 << 20, + "the fixture no longer exceeds one envelope, so this proves nothing") + + let connection = try await connect() + let location = try await RepositoryLocation.local(root.path) + let router = RepositoryRouter( + localReader: { try connection.reader(context: $0) }, + localWriter: { try connection.writer(context: $0, reader: connection.reader(context: $0)) }) + try router.register(.init(location: location, backend: .git, sharedLocation: location)) + + let result = await (try await router.writer(for: location)).commit( + request: VCSCommitRequest(message: "chunked commit", files: files, mode: .commit)) + guard case .ok = result else { return XCTFail("chunked commit failed: \(result)") } + + // Recorded, and recorded IN FULL: a truncated payload would commit a prefix and still report ok. + let subject = try run("git", ["log", "-1", "--format=%s"], at: root) + XCTAssertEqual(subject.trimmingCharacters(in: .whitespacesAndNewlines), "chunked commit") + let counted = try run("git", ["show", "--name-only", "--format=", "HEAD"], at: root) + .split(whereSeparator: \.isNewline).count + XCTAssertEqual(counted, files.count, "the commit recorded a truncated pathspec") + await connection.close() + } } diff --git a/vcs/crates/wr-agent/src/vcs.rs b/vcs/crates/wr-agent/src/vcs.rs index c54b43b54..e3cfcfc40 100644 --- a/vcs/crates/wr-agent/src/vcs.rs +++ b/vcs/crates/wr-agent/src/vcs.rs @@ -19,13 +19,37 @@ use std::time::{Duration, Instant}; use wr_vcs_model::{self as model, VcsError}; const MAX_RESPONSE: usize = 16 * 1024 * 1024; +/// The reassembled ceiling for a CHUNKED request, mirroring `MAX_RESPONSE` on the reply side. A +/// single-envelope request is still bounded by `MAX_ENVELOPE_PAYLOAD` (1 MiB) as before. +const MAX_REQUEST: usize = 16 * 1024 * 1024; +/// First byte of a chunked request envelope. A whole request is JSON and therefore always starts +/// `{`, so this is unambiguous — and it is what keeps an UNCHUNKED request byte-identical to what +/// the previous protocol sent, rather than adding a header to every request to serve the rare one. +const REQUEST_CHUNK_MARKER: u8 = 0x02; +/// Every byte buffered across ALL partial requests. Per-stream caps alone would still allow +/// 32 × `MAX_REQUEST` = 512 MiB of peer-controlled memory in this process, which is not a bound +/// worth having on a background daemon. +const MAX_PARTIAL_TOTAL: usize = 32 * 1024 * 1024; + +/// In-progress chunked requests, keyed by stream. +/// +/// `None` buffer ⇒ POISONED: the request was already refused (too large, too many streams) and its +/// remaining chunks must be swallowed rather than starting a fresh buffer. Without that, the tail of +/// a rejected request would be reassembled as a new one, fail to parse, and produce a SECOND reply +/// on a stream the client has already completed — and an unknown stream id is a protocol violation +/// that tears down every other in-flight request on the connection. +/// +/// A client that vanishes mid-request leaves its buffer behind. Bounded by the caps here and by the +/// agent's own lifetime, so it is accepted rather than given a connection-teardown hook that `vcs` +/// does not currently have. +static PARTIAL: Mutex>>>> = Mutex::new(None); const MAX_HISTORY_LIMIT: usize = 10000; /// `CLIVCSWriter.commitTimeout` (Swift) is 600s, the longest legitimate write timeout. Bounds a /// hostile/buggy request from wedging an exec thread indefinitely; the 32-slot `ACTIVE` permit /// already bounds concurrency, this bounds duration. /// The exec service's wire version, reported in `capabilities` so a client can tell a capable agent /// from one that predates the service. A version, not a count — see the `capabilities` reply. -const EXEC_SERVICE_VERSION: u32 = 1; +const EXEC_SERVICE_VERSION: u32 = 2; const MAX_EXEC_TIMEOUT_MS: u64 = 610_000; /// The ceiling must stay above `CLIVCSWriter.commitTimeout` (600s), or the transport would cut a /// legitimate commit short — one with a slow `pre-commit` hook — before its own limit applied, and @@ -41,7 +65,17 @@ static ACTIVE: AtomicUsize = AtomicUsize::new(0); /// process exit while this is nonzero would abort a repo-level transaction mid-flight, not just /// drop a socket. pub fn is_busy() -> bool { - ACTIVE.load(Ordering::Acquire) > 0 + if ACTIVE.load(Ordering::Acquire) > 0 { + return true; + } + // A request arriving in chunks holds no `ACTIVE` permit until its last one lands — deliberately, + // so a slow upload cannot occupy one of the 32 slots. But that left a window where a client + // three chunks into a five-chunk commit looked idle, and the daemon's idle-exit would drop the + // connection under it. In flight is in flight, whichever half of the round trip it is in. + PARTIAL + .lock() + .map(|guard| guard.as_ref().is_some_and(|map| !map.is_empty())) + .unwrap_or(false) } #[derive(Deserialize)] @@ -862,10 +896,91 @@ fn send(writer: &SharedWriter, stream: u32, value: Value) { } } +/// Take the complete request bytes for this envelope, or `None` when more chunks are still coming. +/// +/// `Err` is a request that broke the framing contract and gets a typed reply rather than silence. +fn reassemble(envelope: &Envelope) -> Result>, VcsError> { + let payload = &envelope.payload; + if payload.first() != Some(&REQUEST_CHUNK_MARKER) { + // The overwhelmingly common case: one envelope, one request, no copy and no bookkeeping. + return Ok(Some(payload.clone())); + } + let Some(&is_final) = payload.get(1) else { + return Err(VcsError::PartialData("truncated request chunk".into())); + }; + let mut guard = PARTIAL.lock().unwrap_or_else(|e| e.into_inner()); + let partials = guard.get_or_insert_with(std::collections::HashMap::new); + + // Already refused: swallow the rest in silence and clear on the last chunk. Replying again would + // put a second reply on a stream the client has finished with. + if let Some(None) = partials.get(&envelope.stream) { + if is_final != 0 { + partials.remove(&envelope.stream); + } + return Ok(None); + } + + let refuse = |partials: &mut std::collections::HashMap>>, error| { + // Poison unless this WAS the last chunk, in which case there is nothing left to swallow. + if is_final == 0 { + partials.insert(envelope.stream, None); + } else { + partials.remove(&envelope.stream); + } + Err(error) + }; + + let incoming = payload.len() - 2; + let buffered: usize = partials + .values() + .map(|buffer| buffer.as_ref().map_or(0, Vec::len)) + .sum(); + if buffered + incoming > MAX_PARTIAL_TOTAL { + return refuse( + partials, + VcsError::PartialData("too many large VCS requests in flight".into()), + ); + } + // Checked before inserting a NEW stream, so a client cannot open unbounded partial requests. + if !partials.contains_key(&envelope.stream) && partials.len() >= 32 { + return refuse(partials, VcsError::LockContention); + } + let buffer = partials + .entry(envelope.stream) + .or_insert_with(|| Some(Vec::new())) + .get_or_insert_with(Vec::new); + if buffer.len() + incoming > MAX_REQUEST { + return refuse( + partials, + VcsError::PartialData("VCS request exceeds 16 MiB".into()), + ); + } + buffer.extend_from_slice(&payload[2..]); + if is_final == 0 { + return Ok(None); + } + Ok(partials.remove(&envelope.stream).flatten()) +} + pub fn dispatch(envelope: &Envelope, writer: &SharedWriter) { if envelope.stream == 0 { return; } + // Before the permit: a non-final chunk does no work and must not hold one of the 32 slots for + // however long the rest of the request takes to arrive. The permit is taken when the request is + // COMPLETE, which is when it starts costing something. + let bytes = match reassemble(envelope) { + Ok(Some(bytes)) => bytes, + Ok(None) => return, + Err(error) => { + send( + writer, + envelope.stream, + json!({"version": 1, "error": error}), + ); + return; + } + }; if ACTIVE .fetch_update(Ordering::AcqRel, Ordering::Acquire, |count| { (count < 32).then_some(count + 1) @@ -880,7 +995,6 @@ pub fn dispatch(envelope: &Envelope, writer: &SharedWriter) { return; } let writer = Arc::clone(writer); - let bytes = envelope.payload.clone(); let stream = envelope.stream; std::thread::spawn(move || { struct Permit; @@ -1549,4 +1663,104 @@ mod tests { assert_eq!(truncate_to_escaped_budget("é", 1), ""); assert_eq!(truncate_to_escaped_budget("aé", 2), "a"); } + + fn chunk(stream: u32, body: &[u8], is_final: bool) -> Envelope { + let mut payload = vec![REQUEST_CHUNK_MARKER, u8::from(is_final)]; + payload.extend_from_slice(body); + Envelope::new(Service::Vcs, stream, payload) + } + + /// `PARTIAL` is process-global and these tests assert on its emptiness (via `is_busy`), so they + /// serialize against each other. Without this they pass alone and fail in the parallel run — + /// measured, and exactly the shape of flake that gets blamed on the code under test. + static PARTIAL_TESTS: Mutex<()> = Mutex::new(()); + + /// Take the serialization lock and start from an empty map. The guard is returned so it is held + /// for the caller's whole test. + fn partial_count() -> usize { + PARTIAL + .lock() + .unwrap_or_else(|e| e.into_inner()) + .as_ref() + .map_or(0, |map| map.len()) + } + + fn partial_test() -> std::sync::MutexGuard<'static, ()> { + let guard = PARTIAL_TESTS.lock().unwrap_or_else(|e| e.into_inner()); + PARTIAL.lock().unwrap_or_else(|e| e.into_inner()).take(); + guard + } + + /// A request split across envelopes must reassemble to exactly the bytes that were sent, and a + /// single-envelope request must still be handled byte-identically — the chunk marker exists so + /// the common path pays nothing. + #[test] + fn a_chunked_request_reassembles_and_an_unchunked_one_is_untouched() { + let _serialized = partial_test(); + let whole = br#"{"version":1,"method":"capabilities"}"#.to_vec(); + + // Unchunked: returned as-is, nothing buffered. + let got = reassemble(&Envelope::new(Service::Vcs, 7, whole.clone())).unwrap(); + assert_eq!(got, Some(whole.clone())); + assert_eq!(partial_count(), 0, "an unchunked request left state behind"); + + // Chunked: nothing until the final chunk, then the exact original. + assert_eq!(reassemble(&chunk(9, &whole[..10], false)).unwrap(), None); + assert_eq!(reassemble(&chunk(9, &whole[10..20], false)).unwrap(), None); + // In flight between chunks — the daemon must not idle-exit under a half-sent request. + // `is_busy` is an OR with the dispatch count, so asserting it TRUE here is race-free even + // while other tests run; asserting it false would not be, hence `partial_count` for that. + assert!(is_busy(), "a partial request did not count as in flight"); + assert_eq!( + reassemble(&chunk(9, &whole[20..], true)).unwrap(), + Some(whole) + ); + assert_eq!(partial_count(), 0, "the buffer outlived its request"); + } + + /// Two streams interleaved, because that is what a busy connection actually does — the buffers + /// are keyed by stream and must not bleed into one another. + #[test] + fn interleaved_chunked_requests_do_not_mix() { + let _serialized = partial_test(); + assert_eq!(reassemble(&chunk(1, b"AAA", false)).unwrap(), None); + assert_eq!(reassemble(&chunk(2, b"BBB", false)).unwrap(), None); + assert_eq!( + reassemble(&chunk(1, b"aaa", true)).unwrap(), + Some(b"AAAaaa".to_vec()) + ); + assert_eq!( + reassemble(&chunk(2, b"bbb", true)).unwrap(), + Some(b"BBBbbb".to_vec()) + ); + assert_eq!(partial_count(), 0); + } + + /// The hazard the poisoning exists for: a refused request keeps arriving, and reassembling its + /// tail as a fresh request would produce a SECOND reply on a stream the client has already + /// completed — which the client treats as an unknown stream id, a protocol violation that tears + /// down every other in-flight request on that connection. + #[test] + fn a_refused_chunked_request_swallows_its_remaining_chunks() { + let _serialized = partial_test(); + // Fill past the total cap in one chunk, on a request that is NOT final. + let huge = vec![b'x'; MAX_PARTIAL_TOTAL + 1]; + assert!( + reassemble(&chunk(4, &huge, false)).is_err(), + "the cap did not hold" + ); + // Every later chunk is silent — no value to execute, and crucially no second error. + assert_eq!(reassemble(&chunk(4, b"more", false)).unwrap(), None); + assert_eq!(reassemble(&chunk(4, b"last", true)).unwrap(), None); + // And the poison is cleared by that final chunk rather than leaking. + assert_eq!(partial_count(), 0, "the poisoned stream was never cleared"); + } + + /// A chunk with no continuation byte at all is malformed, not an empty request. + #[test] + fn a_truncated_chunk_header_is_refused() { + let _serialized = partial_test(); + let envelope = Envelope::new(Service::Vcs, 5, vec![REQUEST_CHUNK_MARKER]); + assert!(reassemble(&envelope).is_err()); + } } From f20f8eae33bf8e19cc907645458b7276c31d6e18 Mon Sep 17 00:00:00 2001 From: Joel Moss Date: Fri, 18 Sep 2026 22:05:39 +0100 Subject: [PATCH 12/13] fix(wr-agent): address the adversarial review of the six tracked fixes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Six findings, three of them one root cause, and that root cause was mine: the chunking commit made `PARTIAL` a process-global keyed by stream id. Wrong three ways at once. **It could make the daemon immortal.** `is_busy()` consulted it and `serve`'s idle check ORs `is_busy()`, so a single abandoned entry stopped `idle_since` ever advancing. A client quitting after chunk 2 of 3 left `wr-agent serve` resident until reboot — strictly worse than before the commit, where a vanished client cost nothing. Its own doc claimed the leak was "bounded by the agent's own lifetime", which is exactly what it unbounded. **Stream ids are per connection.** The client resets `nextStream` to 1 on every connect while this agent deliberately outlives the app, so one launch's abandoned buffer could prepend itself to the next launch's identically-numbered request — a parse error on the user's commit — or, if the stale entry was a poison, swallow a live request whole and answer nothing, which the client waits out and reports as `outcomeUnknown` for a command that provably never ran. **The stream-count cap added an entry every time it refused one.** Poisoning is an insert, so refusing the 33rd stream recorded a 33rd entry. All three go away by making the map per-connection state, threaded through `dispatch` exactly as `attached` and `token` already are. It dies with the connection, ids cannot collide across connections, and `is_busy()` reverts — `serve`'s own `connections > 0` already counts a live connection busy, so the partial-request accounting was never needed. The stream-count cap now runs BEFORE the byte caps, so a poison is only ever recorded for a stream there is room to hold. **A hung agent took the respawn path.** The `connectionLost` rethrow commit claimed a handshake timing out against a HUNG agent stays `serviceUnavailable`; `request`'s deadline resumed with `.connectionLost`, so it did not. New `HostConnectionError.requestTimedOut` separates "the peer did not answer" from "the connection died" — they want opposite recoveries, and a respawn cannot help a wedged agent that still holds the flock. The greeting-read deadline stays `connectionLost` on purpose: an agent that has not greeted within 2s is usually one still starting up, which is what the retry loop is for. **Two tests passed against their own reverted fix.** - The dropped-handshake test's fixture accepted and closed immediately, which fails at the greeting read inside `connect`'s `runBlocking` closure — before the `catch` under test. It now greets with a valid `Hello` and then hangs up. - The truncation test re-implemented the budget arithmetic instead of calling it. `bound_exec_streams` and `exec_timeout` are extracted so `exec` and the tests share one expression; deleting either from `exec` now reddens its test. The clamp test had the same defect and the same fix. **Premise errors, which is what rule 1 is for.** `EXEC_SERVICE_VERSION` had been inserted between `MAX_EXEC_TIMEOUT_MS`'s doc comment and its declaration, so a paragraph about commit timeouts documented a version constant. `escaped_len` is an upper BOUND — serde emits `\b` and `\f` as two bytes where it charges six — but its test was named "agree" and sampled around exactly those two characters; it now asserts equality where the two agree and never-under elsewhere. `execVersion`'s doc claimed it "cannot drift" from `AgentExecRequest.version`; they are separate declarations, so there is a test instead. The stale `AgentCommandRunner.neverRan` caveat about oversized replies is gone — that is the defect the truncation commit fixed. Also recorded, not fixed: `>=` on the exec version is justified by a native fallback that is not wired up. A `BackendVersion` error arriving in a REPLY becomes `neverRan`/`launchFailed`, not the `VCSError.backendVersion` that `RepositoryRouter` falls back on, so a future agent that DROPS version 1 would fail every write as "launch failed". Unreachable today; noted where the `>=` is, because that is where someone will need it. cargo test -p wr-agent: 182 pass, clippy and fmt clean. make app-test (serial): 2765 pass, 1 skipped. app-lint clean. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01XLpTyiW62gUJERhjwjcFFs --- .../WorkroomApp/Core/AgentCommandRunner.swift | 6 +- .../WorkroomApp/Core/AgentVCSConnection.swift | 26 +- .../Core/HostConnectionManager.swift | 12 + .../AgentVCSIntegrationTests.swift | 16 +- .../AgentVCSProtocolTests.swift | 10 + vcs/crates/wr-agent/src/serve.rs | 8 +- vcs/crates/wr-agent/src/vcs.rs | 348 ++++++++++++------ 7 files changed, 287 insertions(+), 139 deletions(-) diff --git a/macapp/WorkroomApp/Core/AgentCommandRunner.swift b/macapp/WorkroomApp/Core/AgentCommandRunner.swift index 56ca3c7bf..ab1cf271d 100644 --- a/macapp/WorkroomApp/Core/AgentCommandRunner.swift +++ b/macapp/WorkroomApp/Core/AgentCommandRunner.swift @@ -101,9 +101,9 @@ struct AgentCommandRunner: StatusCommandRunning, Sendable { /// it means "nothing ran", which is a different fact from 127 ("env ran and searched PATH"), and /// `CLIVCSWriter.classify` checks it before everything else to reach `.launchFailed`. /// - /// Caveat: `vcs.rs`'s `send` replaces an over-16-MiB reply with a `PartialData` error, so a - /// command that ran and produced enormous output lands here too. That is the one remaining - /// misreport in this direction and is tracked separately from this fix. + /// This no longer covers a command that merely produced too much output: `vcs.rs`'s `exec` bounds + /// the two streams to what one reply can carry (`bound_exec_streams`), so an enormous but + /// successful command is truncated and still reports its real exit code, as native does. static func neverRan(_ reason: String) -> CommandResult { CommandResult( stdout: "", stderr: reason, exitCode: CommandResult.launchFailed, timedOut: false) diff --git a/macapp/WorkroomApp/Core/AgentVCSConnection.swift b/macapp/WorkroomApp/Core/AgentVCSConnection.swift index 6759f6435..8b7c18b03 100644 --- a/macapp/WorkroomApp/Core/AgentVCSConnection.swift +++ b/macapp/WorkroomApp/Core/AgentVCSConnection.swift @@ -28,8 +28,9 @@ final class AgentVCSConnection: HostServiceConnection, @unchecked Sendable { /// the same signal `RepositoryRouter` already falls back to native writes on. private var _capabilities: AgentVCSCapabilities? private var capabilities: AgentVCSCapabilities? { lock.withLock { _capabilities } } - /// The exec wire version this client speaks — the literal `AgentExecRequest.version` it sends, so - /// the two cannot drift. + /// The exec wire version this client speaks. `AgentExecRequest.version` carries the same number on + /// the wire and is declared separately, so this is a claim the compiler does not check — asserted + /// in `AgentVCSProtocolTests` instead. private static let execVersion = 1 /// The first exec service version that reassembles chunked requests. Separate from `execVersion` /// because it gates a FRAMING capability, not the request body: a version-1 agent speaks the same @@ -126,10 +127,11 @@ final class AgentVCSConnection: HostServiceConnection, @unchecked Sendable { // stale socket, which is the common case rather than an exotic one (the daemon leaves // `session.sock` behind on any `pkill`). // - // Only this case. A `capabilities` reply that arrives and says the agent is incompatible, or a - // handshake that times out against a HUNG agent, are both still `serviceUnavailable`: a - // respawn cannot fix either, since the second candidate exits without binding while the first - // holds the single-instance flock. + // Only this case. A `capabilities` reply that arrives and says the agent is incompatible, and a + // handshake that times out against a HUNG agent (`.requestTimedOut` — which is the reason that + // case exists; it used to arrive here as `.connectionLost` and take the respawn path), are both + // still `serviceUnavailable`: a respawn cannot fix either, since the second candidate exits + // without binding while the first holds the single-instance flock. await connection.close() throw HostConnectionError.connectionLost } catch { @@ -154,9 +156,17 @@ final class AgentVCSConnection: HostServiceConnection, @unchecked Sendable { guard context.location.host == host else { throw HostConnectionError.mismatchedContext } guard lock.withLock({ !closed }) else { throw HostConnectionError.connectionLost } // Presence and version of the SERVICE, not a count of the caller's own methods. `>=` rather - // than `==` so an agent that gains a version 2 does not refuse a client speaking 1 — the agent + // than `==` so an agent that gains a version 3 does not refuse a client speaking 1 — the agent // is what decides whether it still accepts this request, and it answers that on the request - // itself (`exec`'s `version != 1` guard), which is the only place that can know. + // itself (`exec`'s version guard), which is the only place that can know. + // + // KNOWN GAP in that reasoning: a per-request refusal does NOT reach the native fallback. A + // `BackendVersion` error arriving in a REPLY is decoded in `AgentCommandRunner.exec`, throws, + // and becomes `neverRan`/`launchFailed` — not the `VCSError.backendVersion` that + // `RepositoryRouter.writer(for:)` falls back on. So a future agent that DROPS version 1 would + // fail every write as "launch failed" rather than writing natively. Nothing can hit this today + // (no such agent exists, and 2 still accepts 1), but the `>=` is justified by a mechanism that + // is not wired up, and that is worth knowing before the first version that drops one. guard let exec = capabilities?.exec, exec >= Self.execVersion else { throw VCSError.backendVersion("Agent does not support VCS writes.") } diff --git a/macapp/WorkroomApp/Core/HostConnectionManager.swift b/macapp/WorkroomApp/Core/HostConnectionManager.swift index 684dcd871..916743251 100644 --- a/macapp/WorkroomApp/Core/HostConnectionManager.swift +++ b/macapp/WorkroomApp/Core/HostConnectionManager.swift @@ -17,6 +17,15 @@ enum HostConnectionError: Error, Equatable, Sendable, LocalizedError, CustomStri /// `connectionLost` because the caller can say "this definitely did not run", which for a WRITE /// is the difference between a safe retry and one that double-applies a commit or a push. case notDispatched + /// The request was sent and its deadline passed with no reply. Distinct from `connectionLost` + /// because the connection is fine — the PEER is not answering — and the two want opposite + /// recoveries: `LocalAgentVCS` catches `connectionLost` to spawn wr-agent and retry, which is + /// right for a dead agent and useless against a hung one, since the second candidate exits + /// without binding while the first still holds the single-instance flock. + /// + /// Like `connectionLost` and unlike `notDispatched`, the request DID leave this process, so a + /// write that fails this way has an unknown outcome. + case requestTimedOut case staleGeneration case mismatchedContext case serviceUnavailable(String) @@ -29,6 +38,9 @@ enum HostConnectionError: Error, Equatable, Sendable, LocalizedError, CustomStri return "Host connection lost. An operation may have completed; refresh before retrying." case .notDispatched: return "Host service is busy; the request was not sent." + case .requestTimedOut: + return "Host service did not answer in time. An operation may have completed; refresh before " + + "retrying." case .staleGeneration: return "Host connection changed. Refresh repository data before retrying." case .mismatchedContext: diff --git a/macapp/WorkroomAppTests/AgentVCSIntegrationTests.swift b/macapp/WorkroomAppTests/AgentVCSIntegrationTests.swift index 7aa21c324..2b237cc15 100644 --- a/macapp/WorkroomAppTests/AgentVCSIntegrationTests.swift +++ b/macapp/WorkroomAppTests/AgentVCSIntegrationTests.swift @@ -812,9 +812,10 @@ final class AgentVCSIntegrationTests: XCTestCase { /// routed a dropped handshake around the stale-socket recovery entirely — and a stale socket is /// the common case, not an exotic one: the daemon leaves `session.sock` behind on any `pkill`. /// - /// Driven against a socket that ACCEPTS and then closes without answering, which is the shape of a - /// dead agent's leftover socket. A socket nothing is listening on takes a different path (the - /// connect itself fails) and never reached the wrapper this covers. + /// The fixture must GREET and then hang up. An earlier version just accepted and closed, which + /// fails at the greeting read inside `connect`'s `runBlocking` closure and leaves the function + /// before the `catch` under test — so it passed against the reverted fix. The hello is + /// `"WRA1"` + a big-endian u16 version + a build-string length byte (`protocol/envelope.rs`). func testADroppedHandshakeStaysRecoverableRatherThanBecomingUnavailable() async throws { let dir = try root() let path = dir.appendingPathComponent("dead.sock").path @@ -832,10 +833,15 @@ final class AgentVCSIntegrationTests: XCTestCase { } XCTAssertEqual(bound, 0, "could not bind the fixture socket") XCTAssertEqual(Darwin.listen(fd, 1), 0) - // Accept one connection and immediately hang up, mid-handshake. + + // Greet with a version the client accepts (>= 2), no build string, then hang up mid-handshake — + // after the greeting, before answering `capabilities`. + let hello: [UInt8] = Array("WRA1".utf8) + [0x00, 0x02, 0x00] let accepting = Task.detached { let peer = Darwin.accept(fd, nil, nil) - if peer >= 0 { Darwin.close(peer) } + guard peer >= 0 else { return } + _ = hello.withUnsafeBytes { Darwin.send(peer, $0.baseAddress, $0.count, 0) } + Darwin.close(peer) } defer { accepting.cancel() diff --git a/macapp/WorkroomAppTests/AgentVCSProtocolTests.swift b/macapp/WorkroomAppTests/AgentVCSProtocolTests.swift index 97b2ab88e..180df77db 100644 --- a/macapp/WorkroomAppTests/AgentVCSProtocolTests.swift +++ b/macapp/WorkroomAppTests/AgentVCSProtocolTests.swift @@ -208,4 +208,14 @@ final class AgentVCSProtocolTests: XCTestCase { } } } + + /// `AgentVCSConnection.execVersion` and `AgentExecRequest.version` are the same number declared + /// twice — the capability the client checks, and the version it actually sends. Nothing in the + /// compiler ties them together, and a doc comment claiming they "cannot drift" was simply wrong. + func testTheAdvertisedExecVersionIsTheOneActuallySent() { + let request = AgentExecRequest( + executable: "git", args: [], dir: "/tmp", timeoutMs: 1000, stdin: nil, env: [:]) + XCTAssertEqual(request.version, 1) + XCTAssertEqual(request.kind, "exec") + } } diff --git a/vcs/crates/wr-agent/src/serve.rs b/vcs/crates/wr-agent/src/serve.rs index 7d79380ca..1de5f078b 100644 --- a/vcs/crates/wr-agent/src/serve.rs +++ b/vcs/crates/wr-agent/src/serve.rs @@ -197,6 +197,10 @@ pub fn handle_connection( let mut decoder = EnvelopeDecoder::new(); let mut buffer = [0u8; 8192]; let mut attached: Option = None; + // Per connection, so it dies with the connection: stream ids restart at 1 on every connect and + // this agent outlives the app, so a shared map would let one launch's abandoned chunks corrupt + // the next launch's identically-numbered request. See `PartialRequests`. + let mut partial = crate::vcs::PartialRequests::default(); // Identifies THIS attachment, so ending this connection cannot detach a client that has since // taken the session over. let mut token = 0u64; @@ -218,6 +222,7 @@ pub fn handle_connection( &sessions, &mut attached, &mut token, + &mut partial, &writer, &send, ) { @@ -262,11 +267,12 @@ fn dispatch( sessions: &SessionStore, attached: &mut Option, token: &mut u64, + partial: &mut crate::vcs::PartialRequests, writer: &SharedWriter, send: &dyn Fn(&[u8]) -> bool, ) -> Option { if envelope.service == Service::Vcs { - crate::vcs::dispatch(envelope, writer); + crate::vcs::dispatch(partial, envelope, writer); return None; } if envelope.service != Service::Terminal && envelope.service != Service::Control { diff --git a/vcs/crates/wr-agent/src/vcs.rs b/vcs/crates/wr-agent/src/vcs.rs index e3cfcfc40..31070315c 100644 --- a/vcs/crates/wr-agent/src/vcs.rs +++ b/vcs/crates/wr-agent/src/vcs.rs @@ -26,30 +26,42 @@ const MAX_REQUEST: usize = 16 * 1024 * 1024; /// `{`, so this is unambiguous — and it is what keeps an UNCHUNKED request byte-identical to what /// the previous protocol sent, rather than adding a header to every request to serve the rare one. const REQUEST_CHUNK_MARKER: u8 = 0x02; -/// Every byte buffered across ALL partial requests. Per-stream caps alone would still allow -/// 32 × `MAX_REQUEST` = 512 MiB of peer-controlled memory in this process, which is not a bound -/// worth having on a background daemon. +/// Every byte buffered across all of ONE connection's partial requests. Per-stream caps alone would +/// still allow 32 × `MAX_REQUEST` = 512 MiB of peer-controlled memory, which is not a bound worth +/// having on a background daemon. const MAX_PARTIAL_TOTAL: usize = 32 * 1024 * 1024; -/// In-progress chunked requests, keyed by stream. +/// In-progress chunked requests for ONE connection, keyed by stream. /// -/// `None` buffer ⇒ POISONED: the request was already refused (too large, too many streams) and its -/// remaining chunks must be swallowed rather than starting a fresh buffer. Without that, the tail of -/// a rejected request would be reassembled as a new one, fail to parse, and produce a SECOND reply -/// on a stream the client has already completed — and an unknown stream id is a protocol violation -/// that tears down every other in-flight request on the connection. +/// Owned by `handle_connection` and threaded in, exactly as `attached` and `token` already are — +/// NOT a process-global, which the first version of this was and which was wrong three ways at once. +/// Stream ids restart at 1 on every connect (`AgentVCSConnection.nextStream`) while the agent is +/// deliberately long-lived ("negotiated with, never replaced"), so a global keyed by stream alone +/// let one app launch's abandoned buffer corrupt the next launch's identically-numbered request. A +/// global also had no teardown: a client that vanished mid-request left its entry behind forever, +/// and since `is_busy()` consulted it, that entry kept the daemon from ever idle-exiting. /// -/// A client that vanishes mid-request leaves its buffer behind. Bounded by the caps here and by the -/// agent's own lifetime, so it is accepted rather than given a connection-teardown hook that `vcs` -/// does not currently have. -static PARTIAL: Mutex>>>> = Mutex::new(None); +/// Per-connection state fixes all three by construction: the map dies with the connection, ids +/// cannot collide across connections, and a live connection is already counted busy by `serve`'s own +/// `connections > 0` check — so `is_busy()` does not need to know about partial requests at all. +#[derive(Default)] +pub struct PartialRequests { + /// `None` buffer ⇒ POISONED: the request was already refused and its remaining chunks must be + /// swallowed rather than starting a fresh buffer. Without that, the tail of a rejected request + /// would reassemble as a new one, fail to parse, and produce a SECOND reply on a stream the + /// client has already completed — and an unknown stream id is a protocol violation that tears + /// down every other in-flight request on the connection. + streams: std::collections::HashMap>>, +} const MAX_HISTORY_LIMIT: usize = 10000; -/// `CLIVCSWriter.commitTimeout` (Swift) is 600s, the longest legitimate write timeout. Bounds a -/// hostile/buggy request from wedging an exec thread indefinitely; the 32-slot `ACTIVE` permit -/// already bounds concurrency, this bounds duration. /// The exec service's wire version, reported in `capabilities` so a client can tell a capable agent /// from one that predates the service. A version, not a count — see the `capabilities` reply. +/// +/// 2 added chunked request reassembly; 1 accepted only single-envelope requests. const EXEC_SERVICE_VERSION: u32 = 2; +/// `CLIVCSWriter.commitTimeout` (Swift) is 600s, the longest legitimate write timeout. Bounds a +/// hostile/buggy request from wedging an exec thread indefinitely; the 32-slot `ACTIVE` permit +/// already bounds concurrency, this bounds duration. const MAX_EXEC_TIMEOUT_MS: u64 = 610_000; /// The ceiling must stay above `CLIVCSWriter.commitTimeout` (600s), or the transport would cut a /// legitimate commit short — one with a slow `pre-commit` hook — before its own limit applied, and @@ -65,17 +77,7 @@ static ACTIVE: AtomicUsize = AtomicUsize::new(0); /// process exit while this is nonzero would abort a repo-level transaction mid-flight, not just /// drop a socket. pub fn is_busy() -> bool { - if ACTIVE.load(Ordering::Acquire) > 0 { - return true; - } - // A request arriving in chunks holds no `ACTIVE` permit until its last one lands — deliberately, - // so a slow upload cannot occupy one of the 32 slots. But that left a window where a client - // three chunks into a five-chunk commit looked idle, and the daemon's idle-exit would drop the - // connection under it. In flight is in flight, whichever half of the round trip it is in. - PARTIAL - .lock() - .map(|guard| guard.as_ref().is_some_and(|map| !map.is_empty())) - .unwrap_or(false) + ACTIVE.load(Ordering::Acquire) > 0 } #[derive(Deserialize)] @@ -639,7 +641,12 @@ fn run_exec( /// whole reply being replaced by an error. const EXEC_REPLY_RESERVE: usize = 64 * 1024; -/// How many bytes this text costs once JSON-escaped, which is what `MAX_RESPONSE` actually bounds. +/// An UPPER BOUND on what this text costs once JSON-escaped, which is what `MAX_RESPONSE` bounds. +/// +/// A bound, not an exact count: serde emits `\b` (0x08) and `\f` (0x0C) as two bytes where this +/// charges six. Over-estimating is the safe direction — it truncates fractionally early and can +/// never overflow the ceiling — and getting it exact would mean tracking serde's escape table here, +/// which is a coupling worth more than the handful of bytes it buys. /// /// The expansion is why the two caps could not both be satisfied: a control byte serializes as /// `\u0001`, six bytes for one, so two streams at `MAX_EXEC_STREAM` (4 MiB each) reach 48 MiB of @@ -686,6 +693,32 @@ fn latin1_bytes(text: &str) -> model::Result> { .collect() } +/// Cut the two captured streams down to what fits one reply, returning them unchanged when they +/// already do — which is every ordinary command. +/// +/// A function for `exec_timeout`'s reason: the test that covers it must call the code the reply is +/// actually built from, not a copy of the arithmetic. +fn bound_exec_streams<'a>(stdout: &'a str, stderr: &'a str) -> (&'a str, &'a str) { + let budget = MAX_RESPONSE - EXEC_REPLY_RESERVE; + if escaped_len(stdout) + escaped_len(stderr) <= budget { + return (stdout, stderr); + } + let err_budget = escaped_len(stderr).min(budget / 2); + ( + truncate_to_escaped_budget(stdout, budget - err_budget), + truncate_to_escaped_budget(stderr, err_budget), + ) +} + +/// The deadline `run_exec` is given for a requested `timeout_ms`. +/// +/// A function rather than an inline `clamp` so a test can assert the REAL expression: a test that +/// rewrites the clamp for itself stays green when the clamp is deleted from here, which is exactly +/// what the first version of its test did. +fn exec_timeout(timeout_ms: u64) -> Duration { + Duration::from_millis(timeout_ms.clamp(1, MAX_EXEC_TIMEOUT_MS)) +} + fn exec(request: ExecRequest) -> model::Result { let ExecRequest { version, @@ -703,7 +736,7 @@ fn exec(request: ExecRequest) -> model::Result { )); } let dir = absolute(&dir)?; - let timeout = Duration::from_millis(timeout_ms.clamp(1, MAX_EXEC_TIMEOUT_MS)); + let timeout = exec_timeout(timeout_ms); let env: Vec<(&str, &str)> = env.iter().map(|(k, v)| (k.as_str(), v.as_str())).collect(); let stdin = stdin.as_deref().map(latin1_bytes).transpose()?; let captured = run_exec( @@ -724,16 +757,7 @@ fn exec(request: ExecRequest) -> model::Result { // stderr gets first call on the budget: it is what `CLIVCSWriter.classify` matches on, so losing // it turns a diagnosable failure into `.other` with git's own words missing. stdout takes the // slack, which in practice is nearly all of it — stderr is tiny on every ordinary command. - let budget = MAX_RESPONSE - EXEC_REPLY_RESERVE; - let (stdout, stderr) = if escaped_len(&stdout) + escaped_len(&stderr) > budget { - let err_budget = escaped_len(&stderr).min(budget / 2); - ( - truncate_to_escaped_budget(&stdout, budget - err_budget), - truncate_to_escaped_budget(&stderr, err_budget), - ) - } else { - (stdout.as_ref(), stderr.as_ref()) - }; + let (stdout, stderr) = bound_exec_streams(&stdout, &stderr); Ok(json!({ "stdout": stdout, "stderr": stderr, @@ -899,7 +923,10 @@ fn send(writer: &SharedWriter, stream: u32, value: Value) { /// Take the complete request bytes for this envelope, or `None` when more chunks are still coming. /// /// `Err` is a request that broke the framing contract and gets a typed reply rather than silence. -fn reassemble(envelope: &Envelope) -> Result>, VcsError> { +fn reassemble( + partial: &mut PartialRequests, + envelope: &Envelope, +) -> Result>, VcsError> { let payload = &envelope.payload; if payload.first() != Some(&REQUEST_CHUNK_MARKER) { // The overwhelmingly common case: one envelope, one request, no copy and no bookkeeping. @@ -908,68 +935,76 @@ fn reassemble(envelope: &Envelope) -> Result>, VcsError> { let Some(&is_final) = payload.get(1) else { return Err(VcsError::PartialData("truncated request chunk".into())); }; - let mut guard = PARTIAL.lock().unwrap_or_else(|e| e.into_inner()); - let partials = guard.get_or_insert_with(std::collections::HashMap::new); + let streams = &mut partial.streams; // Already refused: swallow the rest in silence and clear on the last chunk. Replying again would // put a second reply on a stream the client has finished with. - if let Some(None) = partials.get(&envelope.stream) { + if let Some(None) = streams.get(&envelope.stream) { if is_final != 0 { - partials.remove(&envelope.stream); + streams.remove(&envelope.stream); } return Ok(None); } - let refuse = |partials: &mut std::collections::HashMap>>, error| { - // Poison unless this WAS the last chunk, in which case there is nothing left to swallow. - if is_final == 0 { - partials.insert(envelope.stream, None); - } else { - partials.remove(&envelope.stream); - } - Err(error) - }; + // Poison unless this WAS the last chunk, in which case there is nothing left to swallow. + // + // Poisoning is itself an insert, so it must never be the thing that breaks a cap: the stream-count + // refusal below would otherwise add a 33rd entry in the act of enforcing a limit of 32. Refusals + // that cannot record a poison simply drop the stream and let the tail be refused the same way. + let refuse = + |streams: &mut std::collections::HashMap>>, error, may_poison: bool| { + if is_final == 0 && may_poison { + streams.insert(envelope.stream, None); + } else { + streams.remove(&envelope.stream); + } + Err(error) + }; let incoming = payload.len() - 2; - let buffered: usize = partials + // The stream-count cap goes FIRST, because poisoning is itself an insert: refused after it, this + // stream is either already known or there is room for it, so recording a poison can never be the + // thing that breaks a cap. (A poison holds no bytes, so it cannot break the byte caps at all.) + if !streams.contains_key(&envelope.stream) && streams.len() >= 32 { + return refuse(streams, VcsError::LockContention, false); + } + let buffered: usize = streams .values() .map(|buffer| buffer.as_ref().map_or(0, Vec::len)) .sum(); if buffered + incoming > MAX_PARTIAL_TOTAL { return refuse( - partials, + streams, VcsError::PartialData("too many large VCS requests in flight".into()), + true, ); } - // Checked before inserting a NEW stream, so a client cannot open unbounded partial requests. - if !partials.contains_key(&envelope.stream) && partials.len() >= 32 { - return refuse(partials, VcsError::LockContention); - } - let buffer = partials + let buffer = streams .entry(envelope.stream) .or_insert_with(|| Some(Vec::new())) .get_or_insert_with(Vec::new); if buffer.len() + incoming > MAX_REQUEST { return refuse( - partials, + streams, VcsError::PartialData("VCS request exceeds 16 MiB".into()), + true, ); } buffer.extend_from_slice(&payload[2..]); if is_final == 0 { return Ok(None); } - Ok(partials.remove(&envelope.stream).flatten()) + Ok(streams.remove(&envelope.stream).flatten()) } -pub fn dispatch(envelope: &Envelope, writer: &SharedWriter) { +pub fn dispatch(partial: &mut PartialRequests, envelope: &Envelope, writer: &SharedWriter) { if envelope.stream == 0 { return; } // Before the permit: a non-final chunk does no work and must not hold one of the 32 slots for // however long the rest of the request takes to arrive. The permit is taken when the request is // COMPLETE, which is when it starts costing something. - let bytes = match reassemble(envelope) { + let bytes = match reassemble(partial, envelope) { Ok(Some(bytes)) => bytes, Ok(None) => return, Err(error) => { @@ -1412,11 +1447,12 @@ mod tests { /// The clamp is the only thing between a client's `timeout_ms` and a thread that waits on it, /// and both ends of it matter: 0 would make every command time out before it started, and an - /// unbounded value would park an `ACTIVE` permit for as long as the caller asked. Asserted - /// through the clamp expression `exec` actually uses, so a changed bound fails here. + /// unbounded value would park an `ACTIVE` permit for as long as the caller asked. Calls + /// `exec_timeout`, the function `exec` uses, so deleting the clamp reddens this. #[test] fn the_exec_timeout_clamp_holds_at_both_ends() { - let clamp = |ms: u64| Duration::from_millis(ms.clamp(1, MAX_EXEC_TIMEOUT_MS)); + // `exec_timeout` itself, not a copy of its arithmetic — the whole point of extracting it. + let clamp = exec_timeout; // Zero is raised to 1ms, never passed through: a 0ms deadline kills the child immediately. assert_eq!(clamp(0), Duration::from_millis(1)); assert_eq!(clamp(1), Duration::from_millis(1)); @@ -1603,22 +1639,21 @@ mod tests { assert_eq!(captured.exit_code, 3, "the child's own exit code was lost"); assert_eq!(captured.stdout.len(), 3 * 1024 * 1024); - // The reply `exec` would build for that capture must fit the wire ceiling, so `send` never - // reaches its blanket replacement. + // `bound_exec_streams` is what `exec` builds its reply from, so deleting the bounding from + // `exec` reddens this — the previous version re-implemented the arithmetic here and stayed + // green against its own reverted fix. let stdout = String::from_utf8_lossy(&captured.stdout); let stderr = String::from_utf8_lossy(&captured.stderr); - let budget = MAX_RESPONSE - EXEC_REPLY_RESERVE; assert!( - escaped_len(&stdout) > budget, + escaped_len(&stdout) > MAX_RESPONSE - EXEC_REPLY_RESERVE, "the fixture no longer exceeds the budget, so this proves nothing" ); - let err_budget = escaped_len(&stderr).min(budget / 2); - let kept = truncate_to_escaped_budget(&stdout, budget - err_budget); + let (kept, kept_err) = bound_exec_streams(&stdout, &stderr); let reply = json!({ "version": 1, "result": { "stdout": kept, - "stderr": truncate_to_escaped_budget(&stderr, err_budget), + "stderr": kept_err, "exit_code": captured.exit_code, "timed_out": captured.timed_out, "signaled": captured.signaled, @@ -1636,21 +1671,39 @@ mod tests { } /// The budget accounting itself, away from multi-MiB fixtures. + /// + /// `escaped_len` is an upper BOUND, so the assertions below are `>=` against serde except for + /// the classes where the two agree exactly. `\b` and `\f` are the whole of the difference and + /// are asserted as over-estimates rather than quietly omitted from the sample. #[test] - fn escaped_length_and_truncation_agree_on_the_expansion() { + fn escaped_length_bounds_the_expansion_it_is_used_to_budget() { // A control byte costs six on the wire, a quote two, plain ASCII one. assert_eq!(escaped_len("\u{0001}"), 6); assert_eq!(escaped_len("\""), 2); assert_eq!(escaped_len("\n"), 2); assert_eq!(escaped_len("abc"), 3); - // And that is what serde actually produces, minus the two surrounding quotes. - for sample in ["\u{0001}\u{0002}", "a\"b\\c", "plain", "tab\there", "é"] { + // Exact for everything but `\b`/`\f`, and never UNDER serde's real cost for anything — + // which is the only property the budget depends on. + for sample in [ + "\u{0001}\u{0002}", + "a\"b\\c", + "plain", + "tab\there", + "é", + "\u{007F}", + ] { assert_eq!( escaped_len(sample), serde_json::to_string(sample).unwrap().len() - 2, "escaped_len disagrees with serde for {sample:?}" ); } + for sample in ["\u{0008}", "\u{000C}", "a\u{0008}b"] { + assert!( + escaped_len(sample) >= serde_json::to_string(sample).unwrap().len() - 2, + "escaped_len UNDER-counts {sample:?}, which could overflow the ceiling" + ); + } // Truncation never exceeds the budget and never splits a character. assert_eq!( @@ -1670,70 +1723,85 @@ mod tests { Envelope::new(Service::Vcs, stream, payload) } - /// `PARTIAL` is process-global and these tests assert on its emptiness (via `is_busy`), so they - /// serialize against each other. Without this they pass alone and fail in the parallel run — - /// measured, and exactly the shape of flake that gets blamed on the code under test. - static PARTIAL_TESTS: Mutex<()> = Mutex::new(()); - - /// Take the serialization lock and start from an empty map. The guard is returned so it is held - /// for the caller's whole test. - fn partial_count() -> usize { - PARTIAL - .lock() - .unwrap_or_else(|e| e.into_inner()) - .as_ref() - .map_or(0, |map| map.len()) - } - - fn partial_test() -> std::sync::MutexGuard<'static, ()> { - let guard = PARTIAL_TESTS.lock().unwrap_or_else(|e| e.into_inner()); - PARTIAL.lock().unwrap_or_else(|e| e.into_inner()).take(); - guard - } - /// A request split across envelopes must reassemble to exactly the bytes that were sent, and a /// single-envelope request must still be handled byte-identically — the chunk marker exists so /// the common path pays nothing. #[test] fn a_chunked_request_reassembles_and_an_unchunked_one_is_untouched() { - let _serialized = partial_test(); + let mut partial = PartialRequests::default(); let whole = br#"{"version":1,"method":"capabilities"}"#.to_vec(); // Unchunked: returned as-is, nothing buffered. - let got = reassemble(&Envelope::new(Service::Vcs, 7, whole.clone())).unwrap(); + let got = reassemble(&mut partial, &Envelope::new(Service::Vcs, 7, whole.clone())).unwrap(); assert_eq!(got, Some(whole.clone())); - assert_eq!(partial_count(), 0, "an unchunked request left state behind"); + assert_eq!( + partial.streams.len(), + 0, + "an unchunked request left state behind" + ); // Chunked: nothing until the final chunk, then the exact original. - assert_eq!(reassemble(&chunk(9, &whole[..10], false)).unwrap(), None); - assert_eq!(reassemble(&chunk(9, &whole[10..20], false)).unwrap(), None); - // In flight between chunks — the daemon must not idle-exit under a half-sent request. - // `is_busy` is an OR with the dispatch count, so asserting it TRUE here is race-free even - // while other tests run; asserting it false would not be, hence `partial_count` for that. - assert!(is_busy(), "a partial request did not count as in flight"); assert_eq!( - reassemble(&chunk(9, &whole[20..], true)).unwrap(), + reassemble(&mut partial, &chunk(9, &whole[..10], false)).unwrap(), + None + ); + assert_eq!( + reassemble(&mut partial, &chunk(9, &whole[10..20], false)).unwrap(), + None + ); + assert_eq!(partial.streams.len(), 1, "the buffer was not retained"); + assert_eq!( + reassemble(&mut partial, &chunk(9, &whole[20..], true)).unwrap(), Some(whole) ); - assert_eq!(partial_count(), 0, "the buffer outlived its request"); + assert_eq!(partial.streams.len(), 0, "the buffer outlived its request"); } /// Two streams interleaved, because that is what a busy connection actually does — the buffers /// are keyed by stream and must not bleed into one another. #[test] fn interleaved_chunked_requests_do_not_mix() { - let _serialized = partial_test(); - assert_eq!(reassemble(&chunk(1, b"AAA", false)).unwrap(), None); - assert_eq!(reassemble(&chunk(2, b"BBB", false)).unwrap(), None); + let mut partial = PartialRequests::default(); + assert_eq!( + reassemble(&mut partial, &chunk(1, b"AAA", false)).unwrap(), + None + ); assert_eq!( - reassemble(&chunk(1, b"aaa", true)).unwrap(), + reassemble(&mut partial, &chunk(2, b"BBB", false)).unwrap(), + None + ); + assert_eq!( + reassemble(&mut partial, &chunk(1, b"aaa", true)).unwrap(), Some(b"AAAaaa".to_vec()) ); assert_eq!( - reassemble(&chunk(2, b"bbb", true)).unwrap(), + reassemble(&mut partial, &chunk(2, b"bbb", true)).unwrap(), Some(b"BBBbbb".to_vec()) ); - assert_eq!(partial_count(), 0); + assert_eq!(partial.streams.len(), 0); + } + + /// Two connections both number their first stream 1 — the client resets `nextStream` on every + /// connect while this agent deliberately outlives the app. A shared map would have let an + /// abandoned buffer from one launch prepend itself to the next launch's request, or, if that + /// entry were a poison, swallow a live request whole and answer nothing. + #[test] + fn one_connections_abandoned_chunks_cannot_reach_another() { + let mut first = PartialRequests::default(); + assert_eq!( + reassemble(&mut first, &chunk(1, b"ABANDONED", false)).unwrap(), + None + ); + // That connection ends here; its state goes with it. + drop(first); + + let mut second = PartialRequests::default(); + let whole = br#"{"version":1,"method":"capabilities"}"#.to_vec(); + assert_eq!( + reassemble(&mut second, &chunk(1, &whole, true)).unwrap(), + Some(whole), + "a previous connection's bytes reached this one" + ); } /// The hazard the poisoning exists for: a refused request keeps arriving, and reassembling its @@ -1742,25 +1810,61 @@ mod tests { /// down every other in-flight request on that connection. #[test] fn a_refused_chunked_request_swallows_its_remaining_chunks() { - let _serialized = partial_test(); + let mut partial = PartialRequests::default(); // Fill past the total cap in one chunk, on a request that is NOT final. let huge = vec![b'x'; MAX_PARTIAL_TOTAL + 1]; assert!( - reassemble(&chunk(4, &huge, false)).is_err(), + reassemble(&mut partial, &chunk(4, &huge, false)).is_err(), "the cap did not hold" ); // Every later chunk is silent — no value to execute, and crucially no second error. - assert_eq!(reassemble(&chunk(4, b"more", false)).unwrap(), None); - assert_eq!(reassemble(&chunk(4, b"last", true)).unwrap(), None); + assert_eq!( + reassemble(&mut partial, &chunk(4, b"more", false)).unwrap(), + None + ); + assert_eq!( + reassemble(&mut partial, &chunk(4, b"last", true)).unwrap(), + None + ); // And the poison is cleared by that final chunk rather than leaking. - assert_eq!(partial_count(), 0, "the poisoned stream was never cleared"); + assert_eq!( + partial.streams.len(), + 0, + "the poisoned stream was never cleared" + ); + } + + /// Enforcing the stream-count cap must not itself break it. Poisoning is an insert, so refusing + /// the 33rd stream by recording a poison for it would grow the map to 33 — a limit that adds an + /// entry every time it is hit is not a limit. + #[test] + fn refusing_the_stream_cap_does_not_add_another_stream() { + let mut partial = PartialRequests::default(); + for stream in 0..32u32 { + assert_eq!( + reassemble(&mut partial, &chunk(stream, b"x", false)).unwrap(), + None + ); + } + assert_eq!(partial.streams.len(), 32); + for stream in 100..110u32 { + assert!( + reassemble(&mut partial, &chunk(stream, b"x", false)).is_err(), + "the 33rd stream was accepted" + ); + } + assert_eq!( + partial.streams.len(), + 32, + "refusals grew the map they were capping" + ); } /// A chunk with no continuation byte at all is malformed, not an empty request. #[test] fn a_truncated_chunk_header_is_refused() { - let _serialized = partial_test(); + let mut partial = PartialRequests::default(); let envelope = Envelope::new(Service::Vcs, 5, vec![REQUEST_CHUNK_MARKER]); - assert!(reassemble(&envelope).is_err()); + assert!(reassemble(&mut partial, &envelope).is_err()); } } From 317b47f1cdc2287ac5c9ad52b855f0dcf330d799 Mon Sep 17 00:00:00 2001 From: Joel Moss Date: Fri, 18 Sep 2026 23:15:02 +0100 Subject: [PATCH 13/13] docs(todos): record #211 eng-review follow-ups Four P3 entries from the #211 engineering review: rename AgentVCSConnection once it carries files and events, extract the shared service plumbing out of vcs.rs, share OS watchers across subscriptions (accepted shortcut, upgrade at the Linux agent), and raise or paginate the 4 MiB listing cap. Co-Authored-By: Claude Sonnet 5 --- TODOS.md | 79 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 79 insertions(+) diff --git a/TODOS.md b/TODOS.md index 9e2867792..a5e64eeff 100644 --- a/TODOS.md +++ b/TODOS.md @@ -1372,6 +1372,85 @@ enumerate remote refs cheaply, and its doc explains why the symref row must be d ## P3 — VCS engine, diffs, and status +### Rename `AgentVCSConnection` once it carries files and events (macapp) — #211 follow-up + +**What:** rename `AgentVCSConnection` to `AgentConnection` (and `AgentVCSReader` / `LocalAgentVCS` where +the name is now too narrow), and update its doc comment and error strings. + +**Why:** after #211 the class owns the File service and an always-open events stream as well as VCS. +A name that says VCS on a class that carries three services misleads the next reader. #211 itself +fixes only the comment and the strings that became false (`Invalid VCS envelope`). + +**Pros:** the name matches what the class does. **Cons:** pure churn across `LocalAgentVCS`, +`HostConnectionManager`, `AgentVCSIntegrationTests` and `AgentVCSProtocolTests`; done inside #211 it +would bury a security-sensitive change in mechanical noise. + +**How to start:** one pure-rename commit right after #211 merges, so `git diff --color-moved` verifies it. + +**Depends on:** #211 merged. + +**Priority:** P3. + +### Extract the agent's shared service plumbing out of `vcs.rs` (wr-agent) — #211 follow-up + +**What:** move `send` (chunked reply writer, parameterized by service byte), `Permit` / `ACTIVE` / +`is_busy`, and the exec runner from `vcs.rs` into `service.rs`; `vcs.rs`, `file.rs` and the watcher import it. + +**Why:** #211 chose `pub(crate)` reuse over an extraction to keep its diff on new behavior. Three +services (VCS, File, watch) now share this code, `vcs.rs` is ~1900 lines, and the helpers are the +agent's service layer in all but name. + +**Pros:** honest module boundary; `vcs.rs` becomes the VCS service again. **Cons:** ~300 moved lines, +in a crate whose changes need the mandatory adversarial review (`macapp/AGENTS.md` rule 3). + +**How to start:** one pure-move commit with no behavior change, verified by `git diff --color-moved` +and the existing suite, immediately after #211 merges so the review does not re-read moved code. + +**Depends on:** #211 merged. + +**Priority:** P3. + +### One OS watcher per subscription duplicates watches across windows (wr-agent) — #211 accepted shortcut + +**What:** share one OS watcher per canonical root across subscriptions and connections, fanning events +into per-subscription coalescers. + +**Why:** #211 creates one `notify` watcher per subscription. `AppStore` is per window, so N windows on +the same workroom, plus one `.git`/`.jj` watch per project per window, duplicate identical recursive +watches. Cheap on macOS FSEvents; on Linux inotify needs one watch per directory per watcher and +duplicates multiply toward `fs.inotify.max_user_watches`. + +**Pros:** correct on both platforms with one OS watch per tree. **Cons:** a cross-connection registry +(refcounts, canonical-path keys, start/stop races) in the daemon that owns terminals, and a +shutdown-ordering case against per-connection teardown. + +**How to start:** find the `gstack-shortcut(dec-8742d8a5-42ac-4348-827b-9c15d9330f5a)` marker at the +watcher creation site. Measure FSEvents duplication first; build the shared registry when the Linux +agent lands. + +**Depends on:** Phase 3 Linux agent, or a measured FSEvents problem. + +**Priority:** P3 — no cost on the only platform it ships on today. + +### Listings above the 4 MiB capture ceiling fail instead of paginating (macapp + wr-agent) — #211 follow-up + +**What:** raise the listing-specific cap (the reply ceiling is 16 MiB) or paginate `list`, so very +large repositories get a tree instead of a "too many files to list" failure. + +**Why:** #211 makes an over-cap listing an explicit typed failure on both paths. Before, both paths +silently returned a stdout cut at an arbitrary byte, possibly mid-filename. A repository above about 70k +paths now sees an error where it used to see a truncated tree with a bogus last entry. + +**Pros:** big monorepos get a usable Files panel. **Cons:** more memory per listing on the agent +(bounded by the shared `ACTIVE` pool) and, for pagination, a stateful protocol. + +**How to start:** confirm users actually hit the error, then raise the cap for `list` only, keeping the +16 MiB `MAX_RESPONSE` as the hard bound and re-running the differential and boundary tests. + +**Depends on:** #211 merged; evidence that the cap is reached. + +**Priority:** P3. + ### `XDG_CONFIG_HOME` never reaches the `gh` auth probe (macapp) — gh-flap eng-review follow-up **What:** decide whether `XDG_CONFIG_HOME` (or another way of resolving gh's config dir) should reach