Skip to content

Add Windows Named Pipe transport - #3

Open
kengwang wants to merge 3 commits into
ltlly:masterfrom
kengwang:codex/windows-named-pipe
Open

Add Windows Named Pipe transport#3
kengwang wants to merge 3 commits into
ltlly:masterfrom
kengwang:codex/windows-named-pipe

Conversation

@kengwang

Copy link
Copy Markdown

Summary

  • make authenticated Windows Named Pipes the default bridge transport
  • retain loopback TCP as an explicit compatibility mode via BN_BRIDGE_TRANSPORT
  • add pipe registry discovery, retries, timeouts, concurrent server handling, and graceful shutdown
  • update CLI endpoint reporting and bundled documentation

Tests

  • uv run pytest
  • 247 passed, 9 skipped on Windows

@ltlly ltlly left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this! I reviewed the diff and ran the test suite on Linux. Overall the design is solid (token auth, pipe-name validation, explicit platform errors), but I found two issues to address before merge:

1. tests/test_bridge.py::test_explicit_transport_environment_overrides_gui_setting fails on Linux (breaks CI)

It sets BN_BRIDGE_TRANSPORT=pipe without patching platform.system(). On Linux bridge_transport() raises ValueError: BN_BRIDGE_TRANSPORT=pipe is only supported on Windows, so the test errors out:

258 passed, 2 skipped, 1 failed
FAILED tests/test_bridge.py::test_explicit_transport_environment_overrides_gui_setting
ValueError: BN_BRIDGE_TRANSPORT=pipe is only supported on Windows

Fix options: use BN_BRIDGE_TRANSPORT=tcp (valid on all platforms and still proves "explicit env overrides GUI setting"), or add monkeypatch.setattr("bn.paths.platform.system", lambda: "Windows").

2. _load_instance purges the registry on transient pipe errors

In src/bn/transport.py (_load_instance, the probe block around L232-239), any probe error other than DENIED is treated as stale and calls _purge_stale_registry. But for pipes, ERROR_SEM_TIMEOUT (121) and ERROR_PIPE_BUSY (231) are transient — the same codes _send_request_to_instance retries via TRANSIENT_PIPE_WINERRORS. The probe timeout is 0.2s, so when the bridge is briefly busy, list_instances() will delete the registry file and the CLI reports "No running bridge instances found" even though the daemon is still alive (registry is only written once at start). Suggest keeping the instance and recording the probe error for winerror in TRANSIENT_PIPE_WINERRORS, and only purging on terminal errors like 2/53.

Minor / nits

  • backlog=64 is a no-op for the AF_PIPE listener — CPython's PipeListener ignores backlog (instances are PIPE_UNLIMITED_INSTANCES). Maybe drop it or add a comment.
  • Unbounded per-connection threads + PIPE_UNLIMITED_INSTANCES + a fixed well-known pipe name means a local process can open connections that never send data and pin bridge threads on recv_bytes(). A recv timeout or max-connection cap would harden this. (Pre-existing pattern on TCP, but worth noting for pipes.)
  • _set_gui_port_command calls _configured_transport("gui") without the try/except ValueError that _restart_bridge_command has — an invalid BN_BRIDGE_TRANSPORT would raise unhandled in the plugin command callback.
  • Please smoke-test the enum / enumDescriptions / requiresRestart schema keys against a real BN install — _FakePluginSettings can't catch schema rejection.

Also, thanks for the _fake_headless_instance BN_CACHE_DIR fix — it fixes a real test-isolation issue on master (I hit it locally with a stale registry).

Everything else looks good: the _process_bridge_request refactor also fixes the pre-existing crash when a payload is a JSON list, and the pipe-name validation correctly rejects remote/nested names.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants