Skip to content

Fix voice agent WebSocket configuration - #50532

Open
Mike Guo (guotuofeng) wants to merge 7 commits into
mainfrom
myguo/websocket_fix
Open

Mike Guo (guotuofeng) wants to merge 7 commits into
mainfrom
myguo/websocket_fix

Conversation

@guotuofeng

@guotuofeng Mike Guo (guotuofeng) commented Sep 20, 2026

Copy link
Copy Markdown
Member

Description

Addresses follow-up feedback from #50408 and #50519 for the voice-agent WebSocket clients:

  • reuses supported AgentsClientBuilder configuration and rejects unsupported HTTP pipeline settings instead of silently ignoring them
  • narrows VoiceAgentWebSocketConnectionOptions to session-specific behavior
  • enforces the synchronous receive stream's single-iterator contract
  • aligns synchronous proxy bypass behavior with the asynchronous transport
  • protects SDK-managed WebSocket handshake headers and verifies application-ID precedence
  • separates synchronous and asynchronous WebSocket session tests into dedicated source files
  • preserves isolated, length-safe names for live telephony resources
  • adds failure-safe telephony binding cleanup in live tests

Testing

  • Focused WebSocket/configuration/telephony contract tests: 69 passed
  • Checkstyle: 0 violations
  • Test compilation: passed

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 1 pipeline(s).
35 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

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.

Copilot review overview

🟡 Changes recommended

Three moderate issues remain regarding proxy matching, cleanup safety, and retry-setting test coverage.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 2 Medium severity

Open (2)
What changed in this PR

Fixes voice-agent WebSocket configuration, session behavior, proxy handling, and telephony test cleanup.

Changes:

  • Reuses supported builder configuration and rejects unsupported settings.
  • Narrows session options and enforces single synchronous iteration.
  • Updates proxy behavior, tests, documentation, and changelog.
File Summary
sdk/​ai/​azure-ai-agents/​src/​test/​java/​com/​azure/​ai/​agents/​voice/​VoiceAgentWebSocketSessionTests.java Updates handshake and iterator tests.
sdk/​ai/​azure-ai-agents/​src/​test/​java/​com/​azure/​ai/​agents/​voice/​VoiceAgentTelephonyLiveTests.java Adds binding cleanup tracking. Moderate issue: cleanup can fail when bindings are already absent.
sdk/​ai/​azure-ai-agents/​src/​test/​java/​com/​azure/​ai/​agents/​FoundryFeaturesHeaderVerificationTest.java Tests unsupported builder settings.
sdk/​ai/​azure-ai-agents/​src/​test/​java/​com/​azure/​ai/​agents/​BetaVoiceAgentWebSocketSessionClientTests.java Tests proxy bypass behavior.
sdk/​ai/​azure-ai-agents/​src/​main/​java/​com/​azure/​ai/​agents/​models/​VoiceAgentWebSocketConnectionOptions.java Narrows options to session behavior.
sdk/​ai/​azure-ai-agents/​src/​main/​java/​com/​azure/​ai/​agents/​implementation/​realtime/​VoiceAgentWebSocketUtils.java Applies SDK-managed WebSocket settings.
sdk/​ai/​azure-ai-agents/​src/​main/​java/​com/​azure/​ai/​agents/​BetaVoiceAgentWebSocketSessionClient.java Enforces iteration and proxy behavior. Moderate issue: proxy bypass matching is case-sensitive.
sdk/​ai/​azure-ai-agents/​src/​main/​java/​com/​azure/​ai/​agents/​BetaVoiceAgentWebSocketSessionAsyncClient.java Uses the fixed token scope.
sdk/​ai/​azure-ai-agents/​src/​main/​java/​com/​azure/​ai/​agents/​AgentsClientBuilder.java Validates WebSocket builder configuration. Moderate issue: retry option and policy coverage is missing for sync and async clients.
sdk/​ai/​azure-ai-agents/​README.md Documents the revised configuration model.
sdk/​ai/​azure-ai-agents/​CHANGELOG.md Records the behavioral changes.

💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.

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.

Copilot review overview

🟢 Approval recommended

No unresolved issues were identified that would block approval.

Review effort: Lite
Findings: None

Resolved since last review (2)

@kaylieee

Copy link
Copy Markdown
Member

Reuse the existing client configuration machinery rather than creating a parallel builder path. In createVoiceAgentWebSocketConfiguration, we reconstruct configuration/User-Agent/header handling, but the Azure HTTP client, pipeline, and custom policies configured on that same builder are not carried over. For example, a policy-added handshake header or TLS configuration on the supplied HTTP client will not apply. Please avoid a separate/custom builder unless there is a demonstrated requirement that the existing machinery cannot support. Endpoint configuration and User-Agent/header construction should primarily be a plumbing/reuse job, with only the necessary WebSocket-specific adaptation. I am not suggesting that an arbitrary Azure HTTP transport automatically supports WebSocket upgrades; where settings genuinely cannot apply, make that limitation explicit rather than silently accepting them.

sdk/openai/azure-ai-openai-realtime is a useful reference for the encapsulation boundary: the public client privately owns its WebSocket client/session, and endpoint/transport implementation types stay internal. Please use that separation as guidance, rather than copying its lifecycle implementation wholesale.

Above is Jose's comment from the earlier PR. The unsupported settings are now documented/rejected but the parallel builder path concern remains

@guotuofeng

Copy link
Copy Markdown
Member Author

builder

createHttpPipeline is emitted by the TypeSpec Java generator and may change when the SDK is regenerated or the emitter is upgraded. Customizing its local variables would tightly couple this library to the generator’s current implementation details and could create regeneration failures or maintenance overhead.

The separate WebSocket path is justified because Azure Core’s HttpClient and HttpPipeline abstractions do not expose WebSocket upgrade functionality. The synchronous and asynchronous WebSocket clients therefore require native OkHttp and Reactor Netty transports.

The current implementation handles this limitation explicitly:

Compatible settings such as endpoint, credential, service version, client headers, User-Agent, and proxy configuration are propagated.
Incompatible settings such as custom HTTP clients, pipelines, policies, and retries are rejected rather than silently ignored.
Tests verify the supported configuration behavior.
Given these constraints, retaining the separate WebSocket configuration path is safer and more maintainable than customizing generated pipeline internals.

the source code of emitter used to generate the createHttpPipeline is
https://github.com/microsoft/typespec/blob/main/packages/http-client-java/generator/http-client-generator-core/src/main/java/com/microsoft/typespec/http/client/generator/core/template/TemplateHelper.java#L137C1-L176C50

@jpalvarezl

Copy link
Copy Markdown
Member

Thanks for following through on the earlier feedback. Removing the public protocol/transport overrides and adding the guard against a second iterator both address the issues raised. I have two follow-ups before considering the remaining concerns closed.

Test resource naming and recording modes

Please replace the nextInt(100) resource-name suffix. It leaves only 100 names per prefix. A concurrent or interrupted run can already own that name; creating another version does not establish ownership of the whole agent, yet cleanup deletes the entire agent.

The replacement should respect our test modes, not simply switch to an untracked UUID.randomUUID(). In recording-capable tests, use the existing testResourceNamer.randomName(...) / randomUuid() through the test framework: TestResourceNamer generates and persists values in RECORD and retrieves those same values in PLAYBACK. Fresh uncontrolled randomness in request paths or bodies can make playback fail.

These two methods are currently gated on AZURE_TEST_MODE=LIVE and the class does not extend the recording test base, so I am not claiming an existing playback failure in this class. The collision/cleanup issue applies today. Please also clarify why the REST-only binding lifecycle case is live-only rather than covered through the usual record/playback harness; the real PSTN-call scenario can legitimately remain live-only. For any test intended to support recording, preserve both replayability and run isolation rather than choosing between fixed shared names and uncontrolled randomness.

Shared builder configuration

Rejecting unsupported settings addresses the silent-ignore bug, but does not by itself settle the shared-configuration concern. I agree that Azure Core's HTTP transport does not expose a WebSocket session API, and I am not requesting a cloned HTTP pipeline or AST patches to generated local variables.

The adapter already uses CoreUtils, UserAgentUtil, and ProxyOptions, which is the right direction. Please make the remaining boundary explicit: reuse the existing shared derivation for supported settings wherever possible, keep only WebSocket-specific adaptation here, and identify anything that genuinely cannot be shared. Add focused compatibility coverage for the supported settings that can drift, especially application ID/User-Agent precedence and client headers, alongside the proxy coverage. The documented requirement to keep this path aligned when the emitter changes should be backed by that coverage, not just a maintenance comment.

@guotuofeng

Copy link
Copy Markdown
Member Author

Addressed the latest review follow-ups in dc79786:

  • Replaced the 0-99 live agent suffix with 12 UUID hex characters. This preserves practical live-run isolation while staying below the ARM-style 256-character resource ID limit that full UUID names exceeded.
  • Documented why the binding lifecycle remains LIVE-only: it validates an actual Twilio connection/number; VoiceAgentTelephonyTests covers the HTTP contracts without provider resources.
  • Protected the SDK-managed User-Agent from custom header overrides.
  • Added explicit coverage for ClientOptions application ID precedence, HttpLogOptions fallback, ordinary custom headers, and protected handshake headers.
  • Moved the configuration compatibility coverage into separate sync and async source files with shared resource-locked TLS assertions.

Validation: 66 focused tests passed; checkstyle reports 0 violations.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants