Skip to content

Clean up HTTP passthrough API#1784

Merged
SteveSandersonMS merged 5 commits into
mainfrom
stevesa/http-interception-refinement
Jun 24, 2026
Merged

Clean up HTTP passthrough API#1784
SteveSandersonMS merged 5 commits into
mainfrom
stevesa/http-interception-refinement

Conversation

@SteveSandersonMS

@SteveSandersonMS SteveSandersonMS commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

The built-in types were sourcing url/header information from two different places which reduced usability of the APIs.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@SteveSandersonMS SteveSandersonMS marked this pull request as ready for review June 24, 2026 11:31
@SteveSandersonMS SteveSandersonMS requested a review from a team as a code owner June 24, 2026 11:31
Copilot AI review requested due to automatic review settings June 24, 2026 11:31

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR cleans up the “HTTP/WebSocket passthrough” request-handler API across SDKs by removing per-forwarder URL override parameters and standardizing on rewriting via the per-request context instead.

Changes:

  • Remove CopilotWebSocketForwarder constructor URL overrides (and equivalent) so forwarders always connect using the URL in the request context.
  • Update E2E tests to rewrite WebSocket URLs by updating/copying the request context (language-appropriate: mutation in Python/Node, copy helpers in .NET/Java).
  • Add context-copy APIs where needed (Java withUrl/withHeaders, .NET copy constructor).
Show a summary per file
File Description
python/e2e/test_copilot_request_handler_e2e.py Updates Python E2E test to rewrite WS URL by mutating ctx.url before returning a forwarder.
python/copilot/copilot_request_handler.py Simplifies Python WS forwarder to always use context.url (removes separate url field/arg).
nodejs/test/e2e/copilot_request_handler.e2e.test.ts Updates Node E2E test to rewrite ctx.url and count messages via a forwarder subclass.
nodejs/src/copilotRequestHandler.ts Makes CopilotRequestContext.url/headers writable and removes URL override from the WS forwarder constructor.
java/src/test/java/com/github/copilot/CopilotRequestHandlerE2ETest.java Updates Java E2E test to pass rctx.withUrl(...) to the forwarder rather than a separate URL arg.
java/src/main/java/com/github/copilot/CopilotWebSocketForwarder.java Removes URL/header override constructors; forwarder now reads URL/headers from CopilotRequestContext.
java/src/main/java/com/github/copilot/CopilotRequestContext.java Adds withUrl / withHeaders copy helpers to support context-based rewriting.
dotnet/test/E2E/CopilotRequestWebSocketE2ETests.cs Updates .NET E2E test to rewrite URL via new CopilotRequestContext(ctx) { Url = ... }.
dotnet/src/CopilotRequestHandler.cs Adds copy constructor + internal ctor for CopilotRequestContext; WS forwarder now uses Context.Url/Headers.

Copilot's findings

  • Files reviewed: 9/9 changed files
  • Comments generated: 2

Comment thread nodejs/test/e2e/copilot_request_handler.e2e.test.ts
Comment thread dotnet/src/CopilotRequestHandler.cs
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@github-actions

Copy link
Copy Markdown
Contributor

Cross-SDK Consistency Review ✅

I reviewed this PR across all six SDK implementations for consistency.

Changes Covered (Node.js, Python, Java, .NET)

The PR removes the optional url/headers parameters from CopilotWebSocketForwarder constructors and instead relies on a mutable/copyable CopilotRequestContext:

SDK URL override mechanism (after PR)
Node.js Directly mutate: ctx.url = newUrl (field made non-readonly)
Python Directly mutate: ctx.url = newUrl (Python allows this already)
Java Copy method: ctx.withUrl(newUrl) (returns new instance)
.NET Copy constructor: new CopilotRequestContext(ctx) { Url = newUrl }

This is consistent across the four OOP SDKs — each follows its language's idiomatic pattern for context mutation.

Go and Rust (not modified — already consistent)

Both remaining SDKs have structurally different APIs that are already aligned with the spirit of this change:

  • Go: CopilotWebSocketForwarder is not a base class — NewCopilotWebSocketForwarder(url, headers) takes URL/headers directly. CopilotRequestContext.URL is already a mutable public field. No changes needed.
  • Rust: CopilotWebSocketForwarder::builder(url, headers) also takes explicit URL/headers. CopilotRequestContext fields are pub (mutable by owner), and open_websocket can pass any URL to the builder. No changes needed.

Minor observation (not a blocking issue)

There's a subtle semantic difference: Node.js and Python mutate the context in-place, while Java and .NET return a new copy. This follows each language's idiomatic approach to "value-like" objects, so it's expected and acceptable. In both cases the original context handed to openWebSocket/open_websocket is either the one modified or the one to copy from — callers don't hold overlapping references during the forwarder construction lifecycle.

Verdict

The PR maintains good cross-SDK consistency. The four OOP SDKs are updated in parallel, and Go/Rust were already designed for this pattern. No additional SDKs need updating.

Generated by SDK Consistency Review Agent for issue #1784 · sonnet46 1.4M ·

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