diff --git a/TODOS.md b/TODOS.md index bd6b9c806..a5e64eeff 100644 --- a/TODOS.md +++ b/TODOS.md @@ -1119,6 +1119,102 @@ 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.**~~ **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 + 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.**~~ **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.**~~ **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.**~~ **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 + 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.**~~ **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; + `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 @@ -1276,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 diff --git a/docs/designs/remote-workrooms.md b/docs/designs/remote-workrooms.md index 226010fbb..ac93f8c1d 100644 --- a/docs/designs/remote-workrooms.md +++ b/docs/designs/remote-workrooms.md @@ -13,8 +13,18 @@ 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 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. The implementation sequence and remaining decisions are in **Next Steps**. @@ -1922,9 +1932,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 +1982,76 @@ 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`). + + **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. + + 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 + `/.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. 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. 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`, + `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. @@ -1981,6 +2062,22 @@ service milestones below so each layer can be reviewed and landed 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. diff --git a/macapp/WorkroomApp/Core/AgentCommandRunner.swift b/macapp/WorkroomApp/Core/AgentCommandRunner.swift new file mode 100644 index 000000000..ab1cf271d --- /dev/null +++ b/macapp/WorkroomApp/Core/AgentCommandRunner.swift @@ -0,0 +1,181 @@ +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 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 + + 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 { + // 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)), + 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. + // + // 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 { + // 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 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`. + /// + /// 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) + } + + /// 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 `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. + /// + /// 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: CommandResult.outcomeUnknown, timedOut: false) + } +} + +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..8b7c18b03 100644 --- a/macapp/WorkroomApp/Core/AgentVCSConnection.swift +++ b/macapp/WorkroomApp/Core/AgentVCSConnection.swift @@ -22,6 +22,27 @@ 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. + /// `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 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 + /// `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 @@ -95,7 +116,24 @@ 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 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, 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 { await connection.close() throw HostConnectionError.serviceUnavailable("VCS negotiation failed: \(error)") @@ -108,30 +146,122 @@ 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 } + // Presence and version of the SERVICE, not a count of the caller's own methods. `>=` rather + // than `==` so an agent that gains a version 3 does not refuse a client speaking 1 — the agent + // is what decides whether it still accepts this request, and it answers that on the request + // 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.") + } + 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) + } + + /// 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.. 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) - 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 - let stream: UInt32? = lock.withLock { - guard !closed, nextStream < UInt32.max, pending.count < 32 else { return nil } + // 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. 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 @@ -143,13 +273,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) @@ -258,6 +388,16 @@ final class AgentVCSConnection: HostServiceConnection, @unchecked Sendable { struct AgentVCSCapabilities: Decodable { let version: Int let reads: 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/macapp/WorkroomApp/Core/HostConnectionManager.swift b/macapp/WorkroomApp/Core/HostConnectionManager.swift index 8156b10ed..916743251 100644 --- a/macapp/WorkroomApp/Core/HostConnectionManager.swift +++ b/macapp/WorkroomApp/Core/HostConnectionManager.swift @@ -12,6 +12,20 @@ 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 + /// 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) @@ -22,6 +36,11 @@ 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 .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/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/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/WorkroomApp/Core/StatusCommandRunner.swift b/macapp/WorkroomApp/Core/StatusCommandRunner.swift index ecec2a7b6..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 @@ -174,6 +188,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 +276,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/WorkroomApp/Core/VCSSyncPresentation.swift b/macapp/WorkroomApp/Core/VCSSyncPresentation.swift index d0c2cb5a2..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,6 +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 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. + // + // `.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. + // + // `.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 @@ -417,9 +441,14 @@ 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. + // 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 } } @@ -434,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 { @@ -469,6 +510,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." } @@ -494,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), @@ -565,6 +609,18 @@ 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: + // 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 """ + 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. + + 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: return nil @@ -578,7 +634,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 +721,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 +783,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 """ + 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 one that did land \ + leaves you with two. + """ case .unsupportedMode, .other: return nil } @@ -735,7 +802,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..218257f7a 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,14 @@ 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`. + /// + /// `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 case other(String) @@ -1135,6 +1154,22 @@ 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. + // + // 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) } 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 +1299,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. @@ -1820,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. @@ -1840,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 } @@ -1975,6 +2030,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/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/AgentVCSIntegrationTests.swift b/macapp/WorkroomAppTests/AgentVCSIntegrationTests.swift index 9432f7227..2b237cc15 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,389 @@ 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() + } + + /// 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)) + } + + /// 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`. + /// + /// 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 + + 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) + + // 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) + guard peer >= 0 else { return } + _ = hello.withUnsafeBytes { Darwin.send(peer, $0.baseAddress, $0.count, 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)") + } + } + + /// 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/macapp/WorkroomAppTests/AgentVCSProtocolTests.swift b/macapp/WorkroomAppTests/AgentVCSProtocolTests.swift index 431da4c84..180df77db 100644 --- a/macapp/WorkroomAppTests/AgentVCSProtocolTests.swift +++ b/macapp/WorkroomAppTests/AgentVCSProtocolTests.swift @@ -37,4 +37,185 @@ 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) + XCTAssertEqual(lost.exitCode, CommandResult.outcomeUnknown) + XCTAssertNotEqual(lost.exitCode, CommandResult.launchFailed) + // `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()) + 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. + XCTAssertEqual( + 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 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. Host key verification failed. fatal: Cannot autostash", + 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. + 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))") + } + } + } + + /// `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 77d4f5d91..31070315c 100644 --- a/vcs/crates/wr-agent/src/vcs.rs +++ b/vcs/crates/wr-agent/src/vcs.rs @@ -5,18 +5,71 @@ 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, + Arc, Mutex, }; +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 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 for ONE connection, keyed by stream. +/// +/// 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. +/// +/// 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; +/// 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 +/// 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); /// Whether any dispatched VCS request — including a JJ snapshot that owns the working-copy lock @@ -55,6 +108,91 @@ 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. +/// +/// 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 { + 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 +314,459 @@ 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. +/// +/// 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, + 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, + } + } +} + +/// 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 +/// 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 { + // `/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() { + Stdio::piped() + } else { + Stdio::null() + }) + .stdout(Stdio::piped()) + .stderr(Stdio::piped()) + .process_group(0) + // `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()); + 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_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 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 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) { + 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)); + }; + // 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)); + } + // 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 + // 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())) + }; + // 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), + // 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(137)), + timed_out, + signaled: status.signal().is_some(), + }) +} + +/// 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; + +/// 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 +/// 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 +/// 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() +} + +/// 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, + 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 = 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( + &dir, + executable.as_str(), + &args, + timeout, + 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 (stdout, stderr) = bound_exec_streams(&stdout, &stderr); + Ok(json!({ + "stdout": stdout, + "stderr": 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 +774,16 @@ fn read(request: Request) -> model::Result { )); } if request.method == "capabilities" { - return Ok(json!({"version": 1, "reads": 9})); + // `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!( @@ -329,10 +920,102 @@ fn send(writer: &SharedWriter, stream: u32, value: Value) { } } -pub fn dispatch(envelope: &Envelope, writer: &SharedWriter) { +/// 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( + 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. + return Ok(Some(payload.clone())); + } + let Some(&is_final) = payload.get(1) else { + return Err(VcsError::PartialData("truncated request chunk".into())); + }; + 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) = streams.get(&envelope.stream) { + if is_final != 0 { + streams.remove(&envelope.stream); + } + return Ok(None); + } + + // 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; + // 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( + streams, + VcsError::PartialData("too many large VCS requests in flight".into()), + true, + ); + } + 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( + streams, + VcsError::PartialData("VCS request exceeds 16 MiB".into()), + true, + ); + } + buffer.extend_from_slice(&payload[2..]); + if is_final == 0 { + return Ok(None); + } + Ok(streams.remove(&envelope.stream).flatten()) +} + +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(partial, 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) @@ -347,7 +1030,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; @@ -450,4 +1132,739 @@ 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_the_read_count_and_the_exec_service_version() { + let reply = execute(br#"{"version":1,"method":"capabilities"}"#); + assert_eq!(reply["result"]["reads"], 9); + // 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 + /// 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 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, + "env", + &[], + Duration::from_secs(5), + None, + &[ + ("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_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(); + } + + /// 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. Calls + /// `exec_timeout`, the function `exec` uses, so deleting the clamp reddens this. + #[test] + fn the_exec_timeout_clamp_holds_at_both_ends() { + // `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)); + 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()); + } + + /// 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(&[]); + } + + /// 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); + + // `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); + assert!( + escaped_len(&stdout) > MAX_RESPONSE - EXEC_REPLY_RESERVE, + "the fixture no longer exceeds the budget, so this proves nothing" + ); + let (kept, kept_err) = bound_exec_streams(&stdout, &stderr); + let reply = json!({ + "version": 1, + "result": { + "stdout": kept, + "stderr": kept_err, + "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. + /// + /// `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_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); + // 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!( + 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"); + } + + 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) + } + + /// 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 mut partial = PartialRequests::default(); + let whole = br#"{"version":1,"method":"capabilities"}"#.to_vec(); + + // Unchunked: returned as-is, nothing buffered. + let got = reassemble(&mut partial, &Envelope::new(Service::Vcs, 7, whole.clone())).unwrap(); + assert_eq!(got, Some(whole.clone())); + 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(&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.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 mut partial = PartialRequests::default(); + assert_eq!( + reassemble(&mut partial, &chunk(1, b"AAA", false)).unwrap(), + None + ); + assert_eq!( + 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(&mut partial, &chunk(2, b"bbb", true)).unwrap(), + Some(b"BBBbbb".to_vec()) + ); + 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 + /// 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 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(&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(&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.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 mut partial = PartialRequests::default(); + let envelope = Envelope::new(Service::Vcs, 5, vec![REQUEST_CHUNK_MARKER]); + assert!(reassemble(&mut partial, &envelope).is_err()); + } }