Support optional connection token for TCP servers#1176
Support optional connection token for TCP servers#1176SteveSandersonMS wants to merge 6 commits intomainfrom
Conversation
This comment has been minimized.
This comment has been minimized.
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| if self._client: | ||
| try: | ||
| await self._client.stop() | ||
| except Exception: |
| finally: | ||
| try: | ||
| await wrong.force_stop() | ||
| except Exception: |
| finally: | ||
| try: | ||
| await no_token.force_stop() | ||
| except Exception: |
| } | ||
| finally | ||
| { | ||
| try { await wrongClient.ForceStopAsync(); } catch { } |
| } | ||
| finally | ||
| { | ||
| try { await noTokenClient.ForceStopAsync(); } catch { } |
| } | ||
| finally | ||
| { | ||
| try { await wrongClient.ForceStopAsync(); } catch { } |
| } | ||
| finally | ||
| { | ||
| try { await noTokenClient.ForceStopAsync(); } catch { } |
This comment has been minimized.
This comment has been minimized.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Generated by SDK Consistency Review Agent for issue #1176 · ● 1.5M
| if config.use_stdio: | ||
| self._effective_connection_token = None | ||
| else: | ||
| self._effective_connection_token = config.tcp_connection_token or uuid.uuid4().hex |
There was a problem hiding this comment.
Minor cross-SDK inconsistency: The or uuid.uuid4().hex expression silently replaces an empty-string tcp_connection_token with an auto-generated UUID (Python's falsy "" semantics). Node.js and .NET both raise an explicit error for empty strings:
- Node.js:
throw new Error("tcpConnectionToken must be a non-empty string") - .NET:
throw new ArgumentException("TcpConnectionToken must be a non-empty string")
To stay consistent, consider adding an explicit guard before this line:
if config.tcp_connection_token is not None and config.use_stdio:
raise ValueError("tcp_connection_token cannot be used with use_stdio=True")
if config.tcp_connection_token == "":
raise ValueError("tcp_connection_token must be a non-empty string")This is an edge case (nobody should pass ""), but worth aligning for predictable cross-SDK behaviour.
| lines.push(` self._session_id = session_id`); | ||
| for (const [groupName] of groups) { | ||
| lines.push(` self.${toSnakeCase(groupName)} = ${toPascalCase(groupName)}Api(client, session_id)`); | ||
| const prefix = classPrefix + (isSession ? "" : "Server"); |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Generated by SDK Consistency Review Agent for issue #1176 · ● 706.2K
| if config.use_stdio: | ||
| self._effective_connection_token = None | ||
| else: | ||
| self._effective_connection_token = config.tcp_connection_token or uuid.uuid4().hex |
There was a problem hiding this comment.
Cross-SDK consistency: The auto-generated token uses uuid.uuid4().hex which produces a compact 32-character hex string without dashes (e.g. a3f8b7d2e4c6f8a0b2d4e6f8a0b2d4e6), while the other three SDKs all produce standard UUID format with dashes:
- Node.js:
randomUUID()→a3f8b7d2-e4c6-f8a0-b2d4-e6f8a0b2d4e6 - Go:
uuid.NewString()→a3f8b7d2-e4c6-f8a0-b2d4-e6f8a0b2d4e6 - .NET:
Guid.NewGuid().ToString()→a3f8b7d2-e4c6-f8a0-b2d4-e6f8a0b2d4e6
This is also inconsistent with how the same file generates session IDs on line 1457, which correctly uses str(uuid.uuid4()).
Suggestion: replace uuid.uuid4().hex with str(uuid.uuid4()) for consistency across all SDKs.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Mirrors the Node SDK implementation in .NET, Go, and Python: - Add tcpConnectionToken option to client config; auto-generate a UUID when spawning the CLI in TCP mode and forward via COPILOT_CONNECTION_TOKEN. - Send the token via a new \connect\ RPC during the handshake; fall back to \ping\ against legacy servers without \connect\. - e2e coverage for explicit token, auto-generated token, wrong token, and missing token in each language. Codegen: fix scripts/codegen so quicktype's Python/Go renderers don't crash on boolean literal schemas (\const: true\/\�num: [true]\). Adds stripBooleanLiterals helper applied to the schema fed into quicktype. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The schema can now flag methods and types as internal. The codegen splits internal RPC methods into parallel structures so they don't appear on the public client API: - TypeScript: createInternalServerRpc / createInternalSessionRpc factories alongside the existing public ones; client.ts wires connect() through a private internalRpc getter. - C#: ConnectAsync and ConnectResult are emitted with the internal access modifier (real assembly-boundary access control). - Python: parallel InternalServerRpc / InternalSessionRpc classes with ':meta private:' docstrings. - Go: parallel InternalServerRpc / InternalSessionRpc types with their own unexported backing struct and NewInternalServerRpc constructor. - Internal type definitions get a per-language doc-comment marker. - New filterNodeByVisibility() helper in scripts/codegen/utils.ts. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
SteveSandersonMS
force-pushed
the
stevesa/sdk-server-connection-token
branch
from
b079ec3 to
5b23c6a
Compare
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Generated by SDK Consistency Review Agent for issue #1176 · ● 1.2M
| connect_result = await ServerRpc(self._client).connect( | ||
| ConnectRequest(token=self._effective_connection_token) | ||
| ) |
There was a problem hiding this comment.
🐛 Bug: wrong RPC class used — ServerRpc has no connect method
ServerRpc only has ping (and sub-APIs for models, tools, etc.). The newly generated InternalServerRpc is the class that exposes connect. As written, every call to start() will raise:
AttributeError: 'ServerRpc' object has no attribute 'connect'
making the Python SDK completely non-functional after this PR.
Fix — two changes needed:
- Add
InternalServerRpcto the import in thefrom .generated.rpc import (...)block (around line 35):
from .generated.rpc import (
ClientSessionApiHandlers,
ConnectRequest,
InternalServerRpc, # ← add this
ServerRpc,
register_client_session_api_handlers,
)- Replace
ServerRpcwithInternalServerRpchere:
connect_result = await InternalServerRpc(self._client).connect(
ConnectRequest(token=self._effective_connection_token)
)The pattern mirrors the other SDKs:
- Node.js uses
createInternalServerRpc(this.connection)for the handshake - Go uses
NewInternalServerRpc(...) - .NET calls
InvokeRpcAsync<ConnectResult>(connection.Rpc, "connect", ...)
SteveSandersonMS
changed the title
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Generated by SDK Consistency Review Agent for issue #1176 · ● 1M
| if config.use_stdio: | ||
| self._effective_connection_token = None | ||
| else: | ||
| self._effective_connection_token = config.tcp_connection_token or uuid.uuid4().hex |
There was a problem hiding this comment.
Cross-SDK consistency: UUID format and empty-string validation
Two small inconsistencies compared with the other SDK implementations:
1. Token format — uuid.uuid4().hex produces a 32-character lowercase hex string with no dashes (e.g. "a3b4c5d6e7f8..."). Every other SDK generates a standard UUID string with dashes:
| SDK | Expression | Format |
|---|---|---|
| Node.js | randomUUID() |
xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx |
| Go | uuid.NewString() |
xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx |
| .NET | Guid.NewGuid().ToString() |
xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx |
| Python | uuid.uuid4().hex |
xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx |
If the CLI server ever validates token format (or if users compare auto-generated tokens across SDKs), this discrepancy can cause confusion or failures. Consider using str(uuid.uuid4()) to stay consistent.
2. Empty-string validation — because of the or short-circuit, passing tcp_connection_token="" will silently fall through to auto-generation rather than raising a ValueError. Both Node.js and .NET reject an explicitly supplied empty string with an error:
# Node.js equivalent check:
if options.tcpConnectionToken is not None and options.tcpConnectionToken.length === 0:
throw new Error("tcpConnectionToken must be a non-empty string")
# .NET equivalent:
if TcpConnectionToken.Length == 0:
raise ArgumentException("TcpConnectionToken must be a non-empty string")Suggested fix covering both points:
token = config.tcp_connection_token
if token is not None:
if token == "":
raise ValueError("tcp_connection_token must be a non-empty string")
self._effective_connection_token = token
else:
self._effective_connection_token = str(uuid.uuid4())
Cross-SDK Consistency Review ✅ (with one minor note)This PR implements the TCP connection token feature across all four SDKs. Overall consistency is very good:
One minor inconsistency flagged (inline comment on
|
There was a problem hiding this comment.
Generated by SDK Consistency Review Agent for issue #1176 · ● 749.9K
| self._effective_connection_token: str | None = config.tcp_connection_token | ||
| else: | ||
| self._actual_port = None | ||
|
|
||
| if config.tcp_connection_token is not None and config.use_stdio: | ||
| raise ValueError("tcp_connection_token cannot be used with use_stdio=True") | ||
| if config.use_stdio: | ||
| self._effective_connection_token = None | ||
| else: | ||
| self._effective_connection_token = config.tcp_connection_token or uuid.uuid4().hex |
There was a problem hiding this comment.
Cross-SDK consistency: empty-string tcp_connection_token is handled differently here than in the Node.js and .NET SDKs.
Node.js and .NET both explicitly reject an empty-string token at construction time:
// Node.js
if (typeof options.tcpConnectionToken !== "string" || options.tcpConnectionToken.length === 0) {
throw new Error("tcpConnectionToken must be a non-empty string");
}// .NET
if (_options.TcpConnectionToken.Length == 0)
throw new ArgumentException("TcpConnectionToken must be a non-empty string");Python has two divergent behaviours:
-
ExternalServerConfigpath (line 900):self._effective_connection_token = config.tcp_connection_token— an empty string is assigned verbatim and then sent as thetokenfield in theconnectRPC, which the server would likely reject withAUTHENTICATION_FAILEDat runtime rather than at construction time. -
SubprocessConfigpath (line 909):config.tcp_connection_token or uuid.uuid4().hex— an empty string silently falls back to an auto-generated UUID, which is a quiet substitution rather than a clear error.
Suggestion: add a shared guard before the isinstance branch so both paths behave consistently with the other SDKs:
if config.tcp_connection_token is not None and len(config.tcp_connection_token) == 0:
raise ValueError("tcp_connection_token must be a non-empty string")(Go uses the idiomatic empty-string-as-absent pattern, so there's no mismatch there — but aligning Python with Node.js and .NET would be a small improvement.)
Adds opt-in support for connecting to a Copilot CLI TCP server that requires a shared connection token.
Behaviour
CopilotClientOptions.tcpConnectionToken. Only meaningful in TCP mode (useStdio: false); passing it together withuseStdio: truethrows.COPILOT_CONNECTION_TOKENenvironment variable. Callers who settcpConnectionTokenexplicitly override that.connectJSON-RPC method to authenticate. If the server returnsMethodNotFound(older runtime without the handler) the SDK falls back to the existingping-based protocol-version check, so this is fully backwards compatible.Tests
test/e2e/connection_token.test.ts— four end-to-end tests against a spawned CLI:Manual verification: a client built with these changes can still ping a legacy CLI server that has no
connecthandler.Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com