[Agents Extension] Better handling for protocol flag#7725
Conversation
Signed-off-by: trangevi <trangevi@microsoft.com>
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Improves agent invocation protocol resolution by surfacing actionable errors when a protocol can’t be determined and fixing incorrect selection behavior when multiple protocols are declared.
Changes:
- Makes
resolveProtocol/resolveAgentProtocolreturn errors instead of silently defaulting. - Consolidates protocol auto-detection logic to use a single azd client path for local/remote resolution.
- Updates unit test(s) to handle the new
(protocol, error)return shape.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| cli/azd/extensions/azure.ai.agents/internal/cmd/invoke.go | Propagates protocol resolution errors and consolidates local/remote protocol detection into one flow. |
| cli/azd/extensions/azure.ai.agents/internal/cmd/helpers.go | Changes protocol resolution to return contextual validation errors when agent.yaml can’t be read/parsed or is ambiguous. |
| cli/azd/extensions/azure.ai.agents/internal/cmd/invoke_test.go | Updates existing test to handle the new error-returning API. |
Signed-off-by: trangevi <trangevi@microsoft.com>
jongio
left a comment
There was a problem hiding this comment.
The approach is solid - converting silent defaults to explicit exterrors.Validation errors is the right fix. Users with misconfigured agent.yaml now get clear guidance instead of mysterious wrong-protocol behavior.
The resolveLocalProtocol/resolveRemoteProtocol consolidation is clean. They only differed on the name parameter, so the single if/else on a.flags.local is simpler and correct.
Two things before this merges:
-
The
case 1path should validate thathosted.Protocols[0].Protocolisn't empty - aprotocols: [{ version: v1 }]entry (missing protocol field) returns""with nil error, which silently falls through to responses downstream. The bot's suggestion here is valid. -
The five new error scenarios in
resolveAgentProtocolare the core value of this PR and need test coverage. This is non-trivial since it requires mocking the azdClient, but the existingTestResolveProtocol_ExplicitFlagpattern could be extended with a test fixture that writes temp agent.yaml files.
Side note: the author's rejection of the error-wrapping suggestion on invoke.go:167 is correct. The exterrors package docs say not to wrap structured errors with fmt.Errorf, and the fmt.Errorf("failed to create azd client: %%w", err) pattern on line 205 matches 7+ other call sites in this extension.
📋 Prioritization NoteThanks for the contribution! The linked issue isn't in the current milestone yet. |
* Better handling for protocol flag Signed-off-by: trangevi <trangevi@microsoft.com> * Pr comments Signed-off-by: trangevi <trangevi@microsoft.com> --------- Signed-off-by: trangevi <trangevi@microsoft.com>
Handles silent failure due to missing protocol. Also addresses bug where multiple protocols would always use the first one if no flag was provided