fix(util): dial herdr's Windows named pipe instead of a TCP port - #231
fix(util): dial herdr's Windows named pipe instead of a TCP port#231SandroHub013 wants to merge 1 commit into
Conversation
|
This PR doesn't fully meet our contributing guidelines and PR template. What needs to be fixed:
Please edit this PR description to address the above within 2 hours, or it will be automatically closed. If you believe this was flagged incorrectly, please let a maintainer know. |
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| } | ||
| return { path: socketPath } | ||
| export function socketEndpoint(socketPath: string): string { | ||
| return platform() === "win32" ? `\\\\.\\pipe\\${socketPath}` : socketPath |
There was a problem hiding this comment.
Minor style nit: the rest of packages/util/src uses process.platform === "win32" (see global.ts:14, filesystem.ts:57,78,228, win32.ts). The new helper introduces platform() from node:os, which means this file now mixes both styles. Consider process.platform === "win32" for consistency with the package, and the import { platform } from "node:os" line can be dropped.
export function socketEndpoint(socketPath: string): string {
return process.platform === "win32" ? `\\\\.\\pipe\\${socketPath}` : socketPath
}
|
This pull request has been automatically closed because it was not updated to meet our contributing guidelines within the 2-hour window. Feel free to open a new pull request that follows our guidelines. |
socketOptions() returned `{ host: "\\.\pipe", port: socketPath }` on win32.
Node validates `port` as a number, so every bridge call threw "Port should be
>= 0 and < 65536" before opening a connection and a nikcli pane never appeared
as a herdr agent on Windows.
herdr names its pipe after the full .sock path, so the endpoint is
`\\.\pipe\C:\Users\...\herdr.sock` and both platforms want `{ path }`.
socketEndpoint() is exported so the wire-contract test listens on the same
endpoint the bridge dials; that suite went from 1 pass / 9 fail to 10 pass
on Windows.
Fixes #230
da697b7 to
94cea4e
Compare
|
lgtm |
|
Reopened — the compliance bot closed this before anyone looked at it, because the description was missing the template sections. Fixed and Rebased on The 4 red checks are pre-existing, not from this change:
What this fixes: on Windows the herdr bridge threw |
1 participant
Issue for this PR
Closes #230
Type of change
What does this PR do?
socketOptions()passed the herdr socket path as a TCPporton Windows, so Node threwPort should be >= 0 and < 65536before it opened anything. A nikcli pane never showed up as a herdr agent there.herdr names its Windows pipe after the full
.sockpath, so the endpoint is\\.\pipe\C:\Users\...\herdr.sock— a path, not a host/port pair. Both platforms want{ path }, so the branch collapses into onesocketEndpoint()helper. It's exported because the wire-contract test's fake server was listening on the bare path too — that was the second half of the bug.No behaviour change on macOS/Linux:
socketEndpoint()returns the path unmodified there.How did you verify your code works?
bun test packages/nikcli/test/plugin/herdr/report-protocol.test.tson Windows 11: 1 pass / 9 fail → 10 pass. Wholepackages/nikcli/test/plugin/herdr/dir: 39 pass, 6 skip, 0 fail.net.createConnection("\\\\.\\pipe\\" + HERDR_SOCKET_PATH)+session.snapshotreturns real data; the old host/port form throws.herdr pane get <id>on a nikcli pane reports"agent":"nikcli","agent_status":"idle", and the pane appears inherdr agent list.To see the old failure, check out
live-mainand run the same test file on Windows.Screenshots / recordings
Not a UI change.
Checklist