Skip to content

fix: use minProtocol: 3 instead of args.protocol in connect params#103

Open
anschmieg wants to merge 1 commit intogrp06:mainfrom
anschmieg:fix/connect-minprotocol-params
Open

fix: use minProtocol: 3 instead of args.protocol in connect params#103
anschmieg wants to merge 1 commit intogrp06:mainfrom
anschmieg:fix/connect-minprotocol-params

Conversation

@anschmieg
Copy link
Copy Markdown

Summary

  • Fix gateway handshake failure by using explicit minProtocol: 3 instead of args.protocol
  • The ACP protocol requires minProtocol/maxProtocol as separate integer fields

Changes

  • src/lib/controlplane/gateway-connect-profile.ts: Changed minProtocol: args.protocol to minProtocol: 3

Test plan

  • Unit test verifies minProtocol and maxProtocol are present in connect request
  • Verify gateway handshake succeeds with the fix

Fixes #101

The gateway rejects connect requests with 'protocol: N' in params.
The OpenClaw ACP protocol requires separate minProtocol and maxProtocol
integer fields. This fixes 'invalid request frame' errors during
gateway handshake.
Copilot AI review requested due to automatic review settings March 27, 2026 17:33
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

Updates the gateway connect profile to send an explicit minimum ACP protocol version during the WebSocket connect handshake, addressing connection failures related to protocol negotiation.

Changes:

  • Hard-codes connectParams.minProtocol to 3 in buildGatewayConnectProfile.

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

Comment on lines +79 to 80
minProtocol: 3,
maxProtocol: args.protocol,
Copy link

Copilot AI Mar 27, 2026

Choose a reason for hiding this comment

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

There doesn't appear to be a unit test asserting the connect request includes minProtocol/maxProtocol with the expected values (the existing tests/unit/openclawAdapter.test.ts inspects client and caps but not protocol params). Adding an assertion would prevent regressions like this from reappearing and matches the test plan described in the PR.

Copilot uses AI. Check for mistakes.
Comment on lines +79 to 80
minProtocol: 3,
maxProtocol: args.protocol,
Copy link

Copilot AI Mar 27, 2026

Choose a reason for hiding this comment

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

minProtocol is now hard-coded to 3 while maxProtocol still uses args.protocol. This can produce an invalid range (e.g., minProtocol > maxProtocol) if a caller passes anything other than 3, and it also creates a split source-of-truth for the protocol version. Consider deriving both fields from the same value (either use args.protocol for both, or hard-code both and remove/rename the protocol arg to avoid divergence).

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

@copilot apply changes based on this feedback

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