Skip to content

test: verify minProtocol and maxProtocol in connect request#102

Open
anschmieg wants to merge 11 commits intogrp06:mainfrom
anschmieg:fix/connect-protocol-params
Open

test: verify minProtocol and maxProtocol in connect request#102
anschmieg wants to merge 11 commits intogrp06:mainfrom
anschmieg:fix/connect-protocol-params

Conversation

@anschmieg
Copy link
Copy Markdown

@anschmieg anschmieg commented Mar 24, 2026

Problem

The OpenClawGatewayAdapter connect handshake sends minProtocol and maxProtocol in the connect params, but there is no test coverage verifying these fields are included in the actual WebSocket request.

Debugging Context

When Studio failed to connect with a generic "Disconnected" state, I added debug logging to capture the actual WebSocket frames. Without existing tests that verify the exact params structure, I had no reliable way to confirm whether the protocol fields were being sent correctly. The code appeared correct when reading the source, but without a test capturing the actual sent params, I couldn't rule out transpilation issues, bundler behavior, or subtle bugs.

Solution

Adds a test that captures and verifies the connect request params include minProtocol and maxProtocol set to the expected protocol version (3).

Test Coverage

The new test:

  • Creates a mock WebSocket server
  • Captures the actual params sent in the connect request
  • Verifies minProtocol is present and set to 3
  • Verifies maxProtocol is present and set to 3

Files Changed

  • tests/unit/openclawAdapter.test.ts

References

Copilot AI review requested due to automatic review settings March 24, 2026 12:02
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Adds missing unit-test coverage to ensure OpenClawGatewayAdapter includes the expected protocol negotiation fields in its WebSocket connect request params, preventing regressions in the connect handshake contract (closes #101).

Changes:

  • Added a unit test that inspects the outbound connect request and asserts params.minProtocol === 3 and params.maxProtocol === 3.

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

Comment on lines +545 to +549
ws.on("message", (raw) => {
const parsed = JSON.parse(String(raw ?? ""));
if (parsed?.method === "connect") {
observedMinProtocol = parsed.params?.minProtocol;
observedMaxProtocol = parsed.params?.maxProtocol;
Copy link

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

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

This test asserts params.minProtocol/params.maxProtocol are 3, but it doesn’t verify the PR’s stated requirement that these fields are not nested under a params.protocol object (or that no protocol object is present at all). Consider also asserting that parsed.params.protocol is undefined (or that the only protocol-related keys are minProtocol/maxProtocol) to fully cover the regression described in #101 and the PR description.

Copilot uses AI. Check for mistakes.
- Capture full params object to verify structure
- Assert minProtocol and maxProtocol are present
- Assert protocol field is undefined (not nested)

Addresses Copilot review feedback.
@anschmieg
Copy link
Copy Markdown
Author

anschmieg commented Mar 24, 2026

@copilot review the last commit that addressed the concerns raised in @copilot's comment

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.

Add test coverage for connect request protocol params

2 participants