feat: implement cross-platform support with TCP fallback #7

observerw · 2026-01-12T06:30:37Z

Summary

This PR enables cross-platform support (specifically Windows) by introducing a flexible transport layer that can switch between Unix Domain Sockets (UDS) and TCP.

Key Changes

Transport Abstraction: Introduced ConnectionInfo to handle both UDS and TCP connection details.
TCP Fallback on Windows: Automatically uses TCP with dynamic port allocation on Windows, while maintaining UDS on Unix for performance.
Connection Metadata: Added manager.json to store current connection info, making server discovery transport-agnostic.
Enhanced CI: Added Windows and macOS to the CI matrix and updated tests to be cross-platform compatible.
Robust Discovery: Refactored connectivity checks to handle both UDS and TCP transparently.

Verification

All tests passed on macOS (local).
CI updated to verify on Linux, Windows, and macOS.

Copilot

Pull request overview

This PR enables cross-platform support for the LSP CLI tool by introducing a flexible transport layer that automatically uses Unix Domain Sockets (UDS) on Unix-like systems and TCP on Windows where UDS is not available.

Changes:

Introduced ConnectionInfo model to abstract connection details (UDS path vs TCP host/port)
Added automatic TCP fallback with dynamic port allocation on Windows
Created manager.json file for storing connection metadata instead of relying on socket file existence
Updated CI matrix to test on Linux, Windows, and macOS

Reviewed changes

Copilot reviewed 14 out of 14 changed files in this pull request and generated 17 comments.

Show a summary per file

File	Description
src/lsp_cli/manager/models.py	Added ConnectionInfo model with url property for transport abstraction
src/lsp_cli/manager/main.py	Added platform-specific server initialization with dynamic port allocation on Windows
src/lsp_cli/manager/client.py	Added TCP server support on Windows with dynamic port allocation
src/lsp_cli/manager/server.py	Updated to use ConnectionInfo for flexible transport selection
src/lsp_cli/manager/manager.py	Changed return type from Path to ConnectionInfo for create_client
src/lsp_cli/manager/init.py	Enhanced connect_manager to read connection info from manager.json
src/lsp_cli/cli/shared.py	Updated to use ConnectionInfo for transport and URL selection
src/lsp_cli/utils/socket.py	Refactored socket utilities to support both UDS and TCP connections
src/lsp_cli/settings.py	Added IS_WINDOWS flag and MANAGER_CONN_PATH constant
tests/test_server_management.py	Updated tests to work with ConnectionInfo and cross-platform servers
tests/test_language_support.py	Added retry logic for flaky server registration checks
tests/test_cli_integration.py	Added Windows-specific process termination logic
pyproject.toml	Added Windows to supported operating systems
.github/workflows/ci.yml	Expanded test matrix to include Windows and macOS

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-01-12T07:06:41Z

src/lsp_cli/manager/__main__.py

+            s.bind(("127.0.0.1", 0))
+            port = s.getsockname()[1]
+            assert isinstance(port, int)
+        conn = ConnectionInfo(host="127.0.0.1", port=port)


Missing directory creation: On Windows, MANAGER_CONN_PATH.write_text() is called without ensuring the parent directory exists. This will fail if RUNTIME_DIR doesn't exist yet. Add MANAGER_CONN_PATH.parent.mkdir(parents=True, exist_ok=True) before writing the file, similar to the Unix branch.

Suggested change

conn = ConnectionInfo(host="127.0.0.1", port=port)

conn = ConnectionInfo(host="127.0.0.1", port=port)

MANAGER_CONN_PATH.parent.mkdir(parents=True, exist_ok=True)

Copilot · 2026-01-12T07:06:42Z

tests/test_cli_integration.py

+        if os.name == "nt":
+            subprocess.run(
+                [
+                    "taskkill",
+                    "/F",
+                    "/IM",
+                    "python.exe",
+                    "/FI",
+                    "MODULE == lsp_cli.manager",
+                ],
+                capture_output=True,
+            )


The taskkill filter syntax is incorrect. The /FI flag with "MODULE == lsp_cli.manager" is not valid. Windows taskkill doesn't support filtering by module name in this way. Consider using "IMAGENAME eq python.exe" combined with manual process enumeration using psutil or a similar approach to identify the correct process, or accept that this will kill all python.exe processes which may be too broad.

Copilot · 2026-01-12T07:06:42Z

src/lsp_cli/manager/__main__.py

+    if IS_WINDOWS:
+        with socket.socket(socket.AF_INET, socket.SOCK_STREAM) as s:
+            s.bind(("127.0.0.1", 0))
+            port = s.getsockname()[1]
+            assert isinstance(port, int)
+        conn = ConnectionInfo(host="127.0.0.1", port=port)
+        MANAGER_CONN_PATH.write_text(conn.model_dump_json())
+        uvicorn.run(app, host="127.0.0.1", port=port)


Potential security consideration: Binding to 127.0.0.1 on a dynamically allocated port without any authentication means any local process can connect to the manager. While this may be acceptable for a local development tool, consider documenting this security model or implementing basic authentication if the manager handles sensitive data.

Copilot · 2026-01-12T07:06:42Z

src/lsp_cli/utils/socket.py

+async def wait_for_server(
+    uds_path: Path | None = None,
+    host: str | None = None,
+    port: int | None = None,
+    timeout: float = 10.0,
+) -> None:
    async for attempt in AsyncRetrying(
        stop=stop_after_delay(timeout),
        wait=wait_fixed(0.1),
        reraise=True,
    ):
        with attempt:
-            try:
-                _ = await anyio.connect_unix(path)
-            except (OSError, RuntimeError):
-                raise OSError(f"Socket {path} not ready")
+            if uds_path:
+                try:
+                    af_unix = getattr(socket, "AF_UNIX", None)
+                    if af_unix is not None:
+                        _ = await anyio.connect_unix(uds_path)
+                        return
+                except (OSError, RuntimeError):
+                    pass
+            if host and port:
+                try:
+                    _ = await anyio.connect_tcp(host, port)
+                    return
+                except (OSError, RuntimeError):
+                    pass
+            raise OSError("Server not ready")


Logic issue: On Windows where AF_UNIX is not available, if only uds_path is provided (without host and port), the function will always raise "Server not ready" even after retries. The logic should either fail fast when uds_path is provided on Windows, or ensure that at least one valid connection method is available before entering the retry loop.

Copilot · 2026-01-12T07:06:42Z

src/lsp_cli/manager/models.py

+    port: int | None = None
+
+    @property
+    def url(self) -> str:


Missing documentation: The ConnectionInfo.url property returns "http://localhost" as a fallback when host and port are not set. This fallback behavior should be documented, as it may not be obvious when this occurs and could lead to connection issues if callers expect a valid connection URL.

Suggested change

def url(self) -> str:

def url(self) -> str:

"""

Return the HTTP URL for this connection.

If both ``host`` and ``port`` are set, this returns

``"http://{host}:{port}"``. If either value is missing, this

falls back to ``"http://localhost"``.

"""

Copilot · 2026-01-12T07:06:45Z

src/lsp_cli/manager/__init__.py

+            if MANAGER_CONN_PATH.exists():
+                try:
+                    conn = ConnectionInfo.model_validate_json(
+                        MANAGER_CONN_PATH.read_text()
+                    )
+                    if is_server_alive(
+                        uds_path=conn.uds_path, host=conn.host, port=conn.port
+                    ):
+                        break
+                except Exception:
+                    pass


Overly broad exception handling: Catching bare Exception and silently passing can hide legitimate errors during connection info parsing (e.g., JSON decode errors, permission errors). Consider catching more specific exceptions (e.g., json.JSONDecodeError, FileNotFoundError, ValidationError) or at least logging the error for debugging purposes.

Copilot · 2026-01-12T07:06:45Z

tests/test_server_management.py

+        if MANAGER_CONN_PATH.exists():
+            try:
+                conn = ConnectionInfo.model_validate_json(MANAGER_CONN_PATH.read_text())
+                if is_server_alive(
+                    uds_path=conn.uds_path, host=conn.host, port=conn.port
+                ):
+                    break
+            except Exception:
+                pass


Overly broad exception handling: Catching bare Exception and silently passing can hide legitimate errors during connection info parsing. Consider catching more specific exceptions or logging for debugging purposes.

Copilot · 2026-01-12T07:06:45Z

src/lsp_cli/manager/__init__.py

+    if MANAGER_CONN_PATH.exists():
+        try:
+            conn = ConnectionInfo.model_validate_json(MANAGER_CONN_PATH.read_text())
+        except Exception:


'except' clause does nothing but pass and there is no explanatory comment.

Copilot · 2026-01-12T07:06:45Z

src/lsp_cli/utils/socket.py

+                with socket.socket(af_unix, socket.SOCK_STREAM) as s:
+                    s.connect(str(uds_path))
+                    return True
+        except OSError:


'except' clause does nothing but pass and there is no explanatory comment.

Copilot · 2026-01-12T07:06:46Z

src/lsp_cli/utils/socket.py

+                    if af_unix is not None:
+                        _ = await anyio.connect_unix(uds_path)
+                        return
+                except (OSError, RuntimeError):


'except' clause does nothing but pass and there is no explanatory comment.

observerw · 2026-01-12T07:09:34Z

@copilot open a new pull request to apply changes based on the comments in this thread

Copilot · 2026-01-12T07:09:43Z

@observerw I've opened a new pull request, #8, to work on those changes. Once the pull request is ready, I'll request review from you.

* Initial plan * fix: address review comments for cross-platform support - Add directory creation for MANAGER_CONN_PATH.parent on Windows - Fix Windows process killing in tests (use WMIC instead of invalid taskkill filter) - Add comprehensive security documentation for TCP connections - Add fast-fail logic for unsupported UDS on Windows - Document ConnectionInfo.url fallback behavior - Fix resource leak by properly closing httpx.AsyncClient in ManagerServer - Extract port allocation into reusable utility function - Address race conditions by passing socket fd to uvicorn - Add port validation in ManagedClient.conn property - Make exception handling more specific with explanatory comments - Add proper error handling in test helpers Co-authored-by: observerw <[email protected]> * fix: address code review feedback on resource management - Close socket properly in ManagedClient cleanup on Windows - Add error handling for WMIC command (deprecated in newer Windows) - Add PowerShell fallback for process termination in tests Co-authored-by: observerw <[email protected]> --------- Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: observerw <[email protected]>

- Move _ready_event.set() from run() to lifespan after LSP client initialization - Add _startup_error field to capture and propagate initialization failures - Update wait_ready() to raise startup errors to prevent indefinite waiting - Properly handle both success and failure cases in lifespan context manager This fixes the race condition where tests would timeout waiting for a server that failed to initialize, particularly on Windows where initialization can take longer.

observerw · 2026-01-12T17:16:14Z

@copilot 当前 CI 全部失败，排查原因并修复

Copilot · 2026-01-12T17:16:24Z

@observerw I've opened a new pull request, #9, to work on those changes. Once the pull request is ready, I'll request review from you.

…eouts (#9) * Initial plan * fix: restore ready event signaling before server start to prevent timeouts Co-authored-by: observerw <[email protected]> --------- Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: observerw <[email protected]>

observerw added 3 commits January 12, 2026 14:30

feat: implement cross-platform support with TCP fallback and enhanced CI

8b75c43

fix: address lint issues in manager and settings

beafa67

fix: address cross-platform type issues and improve test stability

9a5059a

observerw requested a review from Copilot January 12, 2026 06:58

observerw added the enhancement New feature or request label Jan 12, 2026

Copilot started reviewing on behalf of observerw January 12, 2026 06:59 View session

Copilot AI reviewed Jan 12, 2026

View reviewed changes

Copilot AI mentioned this pull request Jan 12, 2026

[WIP] Implement cross-platform support with TCP fallback #8

Merged

16 tasks

Copilot AI and others added 4 commits January 12, 2026 15:20

fix: address cross-platform CI issues on Windows

ee396be

fix: resolve race condition in manager client startup on Windows

e76a991

Copilot AI mentioned this pull request Jan 12, 2026

fix: restore ready event signaling before server start to prevent timeouts #9

Merged

	conn = ConnectionInfo(host="127.0.0.1", port=port)
	conn = ConnectionInfo(host="127.0.0.1", port=port)
	MANAGER_CONN_PATH.parent.mkdir(parents=True, exist_ok=True)

-    def url(self) -> str:
+    def url(self) -> str:
+        """
+        Return the HTTP URL for this connection.
+        If both ``host`` and ``port`` are set, this returns
+        ``"http://{host}:{port}"``. If either value is missing, this
+        falls back to ``"http://localhost"``.
+        """

feat: implement cross-platform support with TCP fallback #7

Are you sure you want to change the base?

feat: implement cross-platform support with TCP fallback #7

Uh oh!

Conversation

observerw commented Jan 12, 2026

Summary

Key Changes

Verification

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Jan 12, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 12, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 12, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 12, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 12, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 12, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 12, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 12, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 12, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 12, 2026

Choose a reason for hiding this comment

Uh oh!

observerw commented Jan 12, 2026

Uh oh!

Copilot AI commented Jan 12, 2026

Uh oh!

observerw commented Jan 12, 2026

Uh oh!

Copilot AI commented Jan 12, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants