feat(bridge): Windows support via platform-gated CLI transport#25
feat(bridge): Windows support via platform-gated CLI transport#25bwright2810 wants to merge 2 commits into
Conversation
Herdr's control socket on Windows is a named pipe, not a filesystem
AF_UNIX socket, so Bun.connect({unix}) can't reach it (a pipe-aware raw
client gets accepted then immediately EOF'd — the interprocess crate's
handshake is Herdr-internal). The only reliable local client for that
pipe is the same-version herdr binary itself.
herdr-client.ts is split into a HerdrClient interface with two
implementations chosen by createHerdrClient():
- SocketHerdrClient (mac/Linux) — the existing Unix-socket transport,
unchanged.
- CliHerdrClient (Windows) — spawns the herdr CLI once per RPC, emitting
identical JSON envelopes. events.subscribe has no CLI equivalent, so it
degrades to poll-only; StateEngine already treats events as a poke, not
a source of truth, so only poke latency changes, never correctness.
Config gains herdrBin (HERDR_BIN_PATH / COLLIE_HERDR_BIN, defaults to
`herdr` on PATH), consulted only on the Windows path. The manifest
declares the windows platform; README/ARCHITECTURE document the poll-only
degradation and the herdr-on-PATH requirement.
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
|
Hi I've been using a personal fork to get this running on Windows - would like to possibly get this merged upstream. Code and MR were written by Claude by I've reviewed and tested it myself. Thanks! |
AltanS
left a comment
There was a problem hiding this comment.
Nice work, and the interprocess diagnosis looks right. I checked two of your claims locally: SocketHerdrClient's body is byte-identical to the old class, and the CLI contract holds on 0.7.5 (errors on stderr, exit 1), so the exit-code gate is sound.
Three changes I want before this lands.
1. Move the seam down to the transport. Don't split HerdrClient into two implementations. The divergence here is transport, not semantics, and duplicating all 15 methods means the file that's meant to make a Herdr API change a one-file fix now needs that fix twice, with only one half ever exercised on any given machine. What I want:
interface Transport {
request<T>(method: string, params: Record<string, unknown>): Promise<T>;
subscribeEvents(opts): { close(): void } | null; // null = this transport can't stream
}One HerdrClient class keeps every method and its doc comments. SocketTransport is today's request(). CliTransport owns a method→argv table and synthesizes the envelope for pane.read — which is also where the truncated approximation belongs (right now it's hardcoded false, which kills the "load older lines" button, since agent-chat gates on that flag). Adding a Herdr method then means one method plus one case, not three edits across two impls, and consumers keep importing the same HerdrClient type.
2. CliTransport takes its runner as a constructor arg, defaulting to the Bun.spawn one, so the argv table is covered by bun test. Nothing in the CLI path is reachable from a test today, and argv is exactly the part neither of us can verify by reading.
3. COLLIE_HERDR_TRANSPORT=auto|cli|socket, defaulting to auto. I have no Windows box, so without it nobody who can merge this can run it.
Related: a transport that returns null from subscribeEvents shouldn't drive EventPoker into a permanent reconnect loop, and it shouldn't leave StateEngine pinned at COLLIE_POLL_MS. That path exists because "stream down" normally means transient; here it's the steady state, so Windows spawns herdr.exe every 1.5s forever. It needs its own state and its own knob — 1.5s is too many spawns, the 12s idle cadence is too stale. Something like 4s behind COLLIE_POLL_NO_EVENTS_MS.
Smaller: ENOENT out of Bun.spawn should say "herdr not found, set HERDR_BIN_PATH" instead of surfacing as a disconnect banner; isTransientPipeError has four clauses but two are substrings of the other two, so half of it is dead; make Config.herdrBin optional so it stops leaking into server.test.ts; drop the version bump, I'll cut the release commit.
Questions, since I can't reproduce any of this:
- did you reach it from a phone over tailscale, or just localhost in a browser? collie-ctl skips serve entirely when tailscale isn't on PATH
- how did you start it — the Herdr action buttons (which shell
bash collie-ctl.sh), or by hand? - is your herdr a real .exe or a .cmd shim? Bun runs shims through cmd.exe, which changes quoting for every arg, reply text included
- what have you actually typed through send-text? multi-line, quotes,
&,%, emoji - did you hit the pipe drop in practice? if so paste the stderr, that should drive the predicate rather than guesses
If you'd rather just get Windows working and not do the restructure, say so and I'll push it to your branch.
Addresses review on PR AltanS#25: move the platform divergence down to a transport, restore testability and observability, and fix the poll-only degradation path. - herdr-client.ts is now ONE HerdrClient class (every method + doc comment, written once) over a Transport interface (request + subscribeEvents|null). SocketTransport is the unchanged Unix-socket path; CliTransport owns a method->argv table (CLI_SPECS) and synthesizes the pane.read envelope. Adding a Herdr method is one method + one CLI_SPECS entry. - pane.read no longer hardcodes truncated:false (which killed agent-chat's "load older lines" button). approximateTruncated() reports truncated when a --lines N read returns >= N lines. - CliTransport takes its runner as a constructor arg (defaults to Bun.spawn), so the argv table is covered by bun test. New bridge/herdr-client.test.ts. - COLLIE_HERDR_TRANSPORT=auto|cli|socket (default auto) forces a transport, so the CLI path can be exercised off-Windows. - A null subscribeEvents no longer drives EventPoker into a permanent reconnect loop or pins StateEngine at COLLIE_POLL_MS. New three-value PollMode (streaming|fast|no-events); no-events polls on a dedicated COLLIE_POLL_NO_EVENTS_MS (default 4s) with no reconnect. - ENOENT from spawn throws an actionable "herdr not found, set HERDR_BIN_PATH" message. Dropped isTransientPipeError + the read-retry (never observed firing; half its clauses were dead substrings). - Config.herdrBin optional (out of server.test.ts). Reverted the version bump. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
|
Thanks for the detailed review — restructured to match. Rundown of what changed and answers to your questions. Changes1. Seam moved down to the transport. interface Transport {
request<T>(method: string, params?: Record<string, unknown>): Promise<T>;
subscribeEvents(opts: SubscribeOptions): StreamHandle | null; // null = can't stream
}
The 2. 3. Related — the poll-only degradation. A transport that returns Smaller items:
87 unit tests pass on the touched files; Your questions
One unrelated thing I noticed: multi-line reply text arrives flattened to one space-joined line. I traced it — the bridge receives it already flattened (the audit log shows Happy to split any of this into separate commits if you'd rather review it in pieces. |
|
Author of #27 here. Since the two PRs rest on directly contradictory transport claims, I had the central one re-verified independently (two separate agent reviews; the second re-ran the probe live against a running herd) before commenting — this is what both agreed on. The handshake claim doesn't reproduce on the current buildOn Windows 11 with herdr That matches the If the EOF was observed on an older herdr build, an exact version + endpoint + probe would let anyone reproduce it. Two gotchas worth ruling out first, since either produces exactly the reported symptom: dialing What CLI-spawn costs if the pipe does speak raw JSON-RPC
Where this PR is ahead of #27Packaging — manifest Suggested resolutionDirect named-pipe transport as the Windows default — from either PR, no ownership stake — with this PR's packaging and docs layered on top, and CLI-spawn retained as an explicit opt-in fallback for herdr builds demonstrated to be incompatible. 🤖 Generated with Claude Code, cross-checked by a second agent (Codex CLI); probe script available on request. |
…ization, APPDATA default, tests Review follow-ups from #25/#27 cross-review: - dialHerdr exposes onDial(cancel) so a caller timeout that fires mid-connect aborts the pending dial instead of leaking the OS handle (both request() and subscribeEvents() wire it up); a dial destroyed before connect now settles its promise instead of pending forever. - toPipeName() passes through already-prefixed pipe names (either slash direction) instead of double-prefixing. - defaultSocketPath() resolves %APPDATA%\herdr\herdr.sock on win32 so HERDR_SOCKET_PATH is no longer required there; pure and unit-tested. - New dial.test.ts: pure toPipeName cases everywhere, plus live named- pipe round-trip, mid-codepoint chunk split, connect-error, and cancel coverage on win32 (skipped elsewhere). - SockHandle.end() documented as immediate destroy, not a drained close. Co-Authored-By: Claude Fable 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01VutEkP9A8oY72ZQBfFJdSw
Builds on mikebenner's named-pipe dial shim with the parts of bwright's Windows PR (#25) that outlive its transport: a forced-dialer escape hatch, the envEnum config helper, and the packaging/docs it carried. COLLIE_HERDR_DIAL=auto|net|bun picks the dialer. `auto` is unchanged behaviour — node:net on Windows, Bun.connect elsewhere. `net` forces the Windows dialer anywhere, which matters because net.connect(path) opens an AF_UNIX socket on POSIX and a named pipe on win32: the same branch, the same code, reachable from a machine that can actually review it. bridge/dial.test.ts uses it, so the four live-dial tests that were describe.skip off win32 now run everywhere (2 pass + 4 skip -> 6 pass). Sabotaging net.connect fails them, so they are exercising the branch, not skipping past it. Verified against the live herd on Linux, both dialers, protocol 17: identical snapshots (7 panes, 5 agent), and events.subscribe ACKs over node:net. The event stream survives on this transport — that is the capability the competing CLI-spawn approach in #25 had to give up, and with it StateEngine's ability to see a blocked->done transition that resolves between two polls. There is no handshake to speak: herdr's `interprocess` local sockets state outright that the crate "never inserts its own message framing or any other type of metadata into the stream", and no handshake/greeting/preamble exists anywhere in its source. Recorded at the top of dial.ts so the next reader doesn't re-derive it. Config.dialMode is optional so it stays out of unrelated test fixtures — the same call made when reviewing #25's Config.herdrBin. herdr-plugin.toml still declares linux/macos only. The bridge runs on Windows; the launcher does not (no systemd unit, and the plugin actions shell out to bash, so they fire only when Git Bash is on PATH). Declaring the platform would advertise buttons that may not work. README documents the manual path instead, in the Variant C posture. Version bump and CHANGELOG are deferred to the release commit, per the land-features-first workflow. Co-Authored-By: Brandon Wright <[email protected]> Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
|
Thanks for this, and for the restructure. Your round-2 response was exactly what I asked for: the Transport seam, the injected runner, the forced-mode env var, and dropping The decision: I went with the direct named pipe transport (#27), so the CLI-spawn transport is not landing. Why. I read the That matters because keeping the event stream keeps correctness, not just latency. StateEngine derives transitions by comparing snapshots, so a blocked to done that resolves between two polls is missed outright on a poll-only transport, notification included. What landed from your PR, credited to you in the commit:
One thing I did not take: the manifest Also filing your multi-line flattening find separately. Good catch, and thanks for tracing it to the client rather than leaving it as a transport suspicion. Merged to main as dd6610d. Closing this one as superseded. |
|
Thank you both! |
What
Adds Windows support to Collie by giving
herdr-client.tsa second transport, selected by platform. mac/Linux are untouched.Why the socket path doesn't work on Windows
On Windows, Herdr doesn't expose a filesystem AF_UNIX socket — it maps the socket path onto a Windows named pipe (
\.\pipe\<path>) via the Rustinterprocesscrate, guarded by an in-crate handshake.Bun.connect({unix})targets native AF_UNIX and can't reach a named pipe; even a pipe-aware raw client (Nodenet, .NETNamedPipeClientStream) gets the connection accepted and then immediately EOF'd because it doesn't speak the crate's handshake. The only reliable local client for that pipe is the same-versionherdrbinary itself — which exposes every method Collie needs as a CLI subcommand and emits identical JSON envelopes.How
herdr-client.tsis refactored into aHerdrClientinterface with two implementations, chosen bycreateHerdrClient():SocketHerdrClient(mac/Linux) — the existing one-shot Unix-socket transport, moved verbatim. No behavior change on your platforms.CliHerdrClient(Windows) — spawnsherdr <subcommand>once per RPC (same one-request-per-invocation shape as the socket path), with a one-retry-on-transient-pipe-drop guard for idempotent reads.events.subscribehas no CLI equivalent, so on Windows it reports "down" and the engine stays on the fast poll cadence. SinceStateEnginealready treats events purely as a poke (the snapshot poll is the source of truth), this costs poke latency, not correctness.Supporting changes:
config.ts:herdrBin(HERDR_BIN_PATH→COLLIE_HERDR_BIN→herdron PATH), consulted only on the Windows path.herdr-plugin.toml:windowsadded to bothplatformsarrays.collie-ctl.shunder Git Bash.check-version.shpasses).Honest caveats
herdr.exebeing same-version and on PATH. If Herdr later exposes the Windows pipe in a way a socket client can speak, a socket-based Windows transport would be cleaner and this becomes legacy.main, so it carries the same behavior it always had, but I have no Mac/Linux box to re-run it on — a second set of eyes there is welcome.tsc --noEmitandweb tsc --noEmitare clean. The bridgebun testsuite has some pre-existing failures on Windows (Unix file-mode //tmp-path assumptions in the tests themselves, not the transport) — identical pass/fail count to a cleanmaincheckout on the same box.Happy to split, reshape, or gate this differently if you'd prefer a different approach to platform selection.