Add websocket upstream bridge for /v1/responses and fix VS Code payload handling#110
Open
PenguinDOOM wants to merge 24 commits into
Open
Add websocket upstream bridge for /v1/responses and fix VS Code payload handling#110PenguinDOOM wants to merge 24 commits into
PenguinDOOM wants to merge 24 commits into
Conversation
- add CLI and env plumbing for responses websocket upstream mode - persist the resolved mode in app config and cover it with focused tests, including falsey env values
- add an importable websocket bridge stub for responses route patching - add route contracts for disabled mode, enabled mode, and explicit websocket failure paths
- route enabled responses requests through a dedicated websocket bridge - preserve SSE streaming, non-stream aggregation, and explicit error handling without connection reuse
- add flag, env var, and scope notes for HTTP /v1/responses websocket upstream - add manual verification steps for disabled and enabled runtime modes
- Added _build_upstream_request_event function to ensure proper payload structure for response.create events. - Updated _send_upstream_request to log and send the modified request event. - Enhanced unit tests to verify the correct behavior of the new request event structure and ensure existing payloads are preserved.
…store is always set to False - Removed the "truncation" key from the normalized payload. - Ensured "store" is set to False by default in the normalized responses payload.
- Removed unnecessary [DONE] sentinel from streaming events in responses_websocket_bridge.py. - Adjusted response preparation logic in session.py to ensure previous_response_id is handled correctly. - Added tests to validate normalization of responses payload and ensure store is always set to False.
Co-authored-by: Copilot <copilot@github.com>
Co-authored-by: Copilot <copilot@github.com>
- split default-off and stateful-on websocket bridge tests - add RED coverage for retained follow-up behavior - add CLI flag and env RED coverage for opt-in stateful mode
- add default-off stateful websocket bridge flags and env wiring - reject invalid startup config without websocket upstream - enable previous_response_id only in the stateful route branch
- add retained websocket registry with locking and bounded capacity - reuse upstream sockets across stateful follow-up requests - reject same-session contention and evict retained sockets on failure
- backfill completed response output from output_item events - apply retained-session capacity limits through the full route path - document stateful mode requirements, buffering behavior, and manual verification steps
- Seed headerless stateful HTTP contract tests - Preserve legacy websocket-upstream guard coverage - Characterize streaming invalid-marker as SSE error
- Remove header-gated stateful HTTP routing - Promote retained websocket ownership by response id - Green marker-driven route and registry tests
- Return previous_response_not_found for stale markers - Evict failed retained markers before retry - Preserve SSE characterization for streaming limitation
Co-authored-by: Copilot <copilot@github.com>
- Updated the error response structure in `responses_websocket_bridge.py` to include a specific error code and message for cases where a previous response is not found. - Modified the test in `test_routes.py` to assert the new error response format, ensuring the correct code and message are returned.
- add streaming and non-stream RED coverage for thin response.completed payloads - keep nearby /responses route and stateful regression checks green
…t bridge - preserve existing response.output lists in completed events - backfill completed output from collected response.output_item.done items - synthesize an empty output list when no completed items were observed
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR introduces an opt-in WebSocket upstream transport for the HTTP /v1/responses endpoint, with both stateless and stateful (retained connection) modes. It refactors WebSocket connection logic into shared utilities, adds a new responses-websocket bridge, retained session registry, and corresponding CLI flags, app config, and documentation.
Changes:
- New
responses_websocket_bridgemodule that proxies HTTP/v1/responsesrequests over an upstream websocket and aresponses_websocket_sessionsregistry to retain websocket leases byprevious_response_id. - New CLI flags / env vars (
--responses-websocket-upstream,--responses-websocket-upstream-stateful) wired throughcmd_serveandcreate_app, with a startup validation that stateful mode requires upstream mode. - Behavior changes in
normalize_responses_payload(forcesstore=False, dropstruncation) and improved completed-response output backfill fromresponse.output_item.doneevents; substantial new test coverage.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| chatmock/app.py | Adds new config flags and validates stateful requires upstream. |
| chatmock/cli.py | Adds CLI/env flags for the new modes and threads them to create_app. |
| chatmock/routes_openai.py | Routes /v1/responses through the websocket bridge when enabled, including stateful HTTP bridge handling. |
| chatmock/responses_api.py | Forces store=False, strips truncation, and backfills completed output items. |
| chatmock/responses_websocket_bridge.py | New bridge that talks to upstream over websocket and translates to JSON / SSE. |
| chatmock/responses_websocket_sessions.py | New retained-websocket registry with capacity / conflict / not-found semantics. |
| chatmock/session.py | Adds explicit_previous_response_id flag and refines final-response handling. |
| chatmock/upstream.py | Moves websocket connect helper here for reuse. |
| chatmock/websocket_routes.py | Removes duplicate websocket connect helper, imports the shared one. |
| tests/test_routes.py | Adds extensive websocket bridge and stateful contract tests. |
| tests/test_responses_websocket_sessions.py | Unit tests for the retained-session registry. |
| tests/test_cli.py | Tests CLI flag wiring for the new options. |
| README.md, DOCKER.md | Documents the new modes and manual verification steps. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+1075
to
+1087
| class FakeUpstreamWebsocket: | ||
| def __init__(self, messages: list[str]) -> None: | ||
| self.sent: list[str] = [] | ||
| self._messages = list(messages) | ||
|
|
||
| def send(self, message: str) -> None: | ||
| self.sent.append(message) | ||
|
|
||
| def recv(self) -> str: | ||
| return self._messages.pop(0) | ||
|
|
||
| def close(self) -> None: | ||
| return None |
Author
There was a problem hiding this comment.
Leaving this out of the must-fix follow-up scope. This pass was limited to correctness, concurrency, and streaming fixes; the local FakeUpstreamWebsocket cleanup can land separately as test-only cleanup.
Comment on lines
+1998
to
+2001
| class FakeUpstreamWebsocket: | ||
| def __init__(self) -> None: | ||
| self.sent: list[str] = [] | ||
| self._messages = [ |
Author
There was a problem hiding this comment.
Leaving this out of the must-fix follow-up scope. I kept the broader FakeUpstreamWebsocket deduplication out of this pass so the review follow-up stayed focused on the correctness and streaming fixes.
Comment on lines
+26
to
+30
| @dataclass(frozen=True) | ||
| class RetainedUpstreamWebsocketLease: | ||
| response_id: str | None | ||
| upstream_ws: Any | ||
| created: bool |
Author
There was a problem hiding this comment.
Leaving this out of the must-fix follow-up scope. The assertion is intentionally on the lease.created flag as written, but the naming/readability cleanup is separate from the must-fix behavior changes addressed here.
|
|
||
| if "store" not in normalized: | ||
| normalized["store"] = False | ||
| normalized["store"] = False |
Author
There was a problem hiding this comment.
Leaving this out of the must-fix follow-up scope. The current pass was limited to must-fix correctness, concurrency, and streaming items; warning/logging around forced store=false can be handled as a separate observability improvement.
- add explicit-empty-output follow-up contract coverage - add incremental stateful streaming contract coverage
- reserve capacity before anonymous upstream connect - roll back pending reservations on connect failure
- remove eager buffering from stateful SSE responses - release retained leases correctly on abort or error
- keep follow-up reuse from backfilling authoritative empty outputs - collapse duplicate websocket credential checks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Details
Testing
Notes