fix(agents): fail fast instead of hanging on the agent-service picker in non-TTY contexts#9015
fix(agents): fail fast instead of hanging on the agent-service picker in non-TTY contexts#9015glharper wants to merge 1 commit into
Conversation
…TTY contexts The shared 'Select an agent service' picker (promptForAgentService) is drawn by the azd host and blocks on stdin. In a non-TTY context (CI, agent harness, or output piped through tee/tail) it was invisible and hung forever. It now detects a non-interactive terminal and fails fast with a structured, actionable error listing the available services, and prints a one-line banner to stderr before the picker in interactive mode so piped/CI logs show why azd is waiting. Fixes #8584 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
📋 Prioritization NoteThanks for the contribution! The linked issue isn't in the current milestone yet. |
There was a problem hiding this comment.
Pull request overview
This PR fixes a silent hang (issue #8584) in the azure.ai.agents extension. When an azd ai agent subcommand needs to resolve an ambiguous agent (a multi-service azure.yaml with more than one azure.ai.agent block), it renders an interactive "Select an agent service" picker via the azd host. In non-TTY contexts (CI, agent harnesses, piped output) the picker was invisible and blocked forever on stdin. All affected commands funnel through the shared helper promptForAgentService, so the fix is applied centrally there.
Changes:
- Add a
agentSelectionInteractiveTTY-check seam and fail fast with a structuredexterrors.Validationerror (new codenon_interactive_agent_selection) when stdin/stdout is not a terminal, listing available services and how to pass one. - Print a one-line hint banner to stderr before the interactive picker; extract a small
serviceNameshelper. - Add/adjust unit tests (using an overridable TTY seam, made non-parallel) and a CHANGELOG entry.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
cli/azd/extensions/azure.ai.agents/internal/exterrors/codes.go |
Adds the CodeNonInteractiveAgentSelection error code constant. |
cli/azd/extensions/azure.ai.agents/internal/cmd/helpers.go |
Adds the TTY-aware fail-fast guard, stderr banner, and serviceNames/agentSelectionInteractive helpers in promptForAgentService. |
cli/azd/extensions/azure.ai.agents/internal/cmd/helpers_test.go |
Adds tests for non-TTY fail-fast, --no-prompt fail-fast, and banner output; introduces the withInteractiveAgentSelection seam and removes parallelism where the seam is used. |
cli/azd/extensions/azure.ai.agents/CHANGELOG.md |
Documents the fix under the Unreleased section. |
| // Print a banner to stderr before the (interactive) picker. When the picker is visible this | ||
| // is harmless; when stdout is piped the banner still shows up in tee/tail/CI logs and | ||
| // explains why azd is blocked waiting for a selection. |
| return isTerminal(os.Stdin.Fd()) && isTerminal(os.Stdout.Fd()) | ||
| } | ||
|
|
||
| // serviceNames returns the sorted-order names of the given services. |
jongio
left a comment
jongio
left a comment
There was a problem hiding this comment.
One comment accuracy nit (already flagged): the banner on the stderr line is only reachable in fully interactive mode (both stdin+stdout are TTYs), so the inline comment's claim about showing up in piped/CI logs describes an unreachable path. Consider simplifying it to explain the banner provides context to the user while the picker loads.
RickWinter
left a comment
RickWinter
left a comment
There was a problem hiding this comment.
This converts the silent hang in promptForAgentService into a fast, structured error when there is no TTY, and prints a one-line stderr banner before the interactive picker. The approach is the right shape: the shared helper is the correct single chokepoint for every command that resolves an agent service, the TTY seam matches the existing isTerminal(os.Stdin/os.Stdout) pattern already used across this extension (nextstep_output.go, doctor.go, run.go), and requiring both stdin and stdout to be terminals is correct since the host draws the picker to stdout. Tests cover the non-TTY fail-fast, the --no-prompt path, and the banner, and the process-global test seam is documented as serial.
One thing worth resolving before merge is the banner rationale, noted inline: the comment claims a piped-log benefit the control flow cannot deliver. It is harmless either way, so nothing here blocks. Disposition: COMMENT.
| // Print a banner to stderr before the (interactive) picker. When the picker is visible this | ||
| // is harmless; when stdout is piped the banner still shows up in tee/tail/CI logs and | ||
| // explains why azd is blocked waiting for a selection. | ||
| fmt.Fprintln(os.Stderr, output.WithHintFormat("azd is waiting for your agent service selection below...")) |
There was a problem hiding this comment.
The comment above says this banner "still shows up in tee/tail/CI logs" when stdout is piped, but the non-TTY guard just above returns before we ever reach this line whenever stdout is not a terminal. So this only runs when both stdin and stdout are TTYs, which is exactly when the picker is already on screen. The banner is harmless, but its stated cross-piped-log value is unreachable as written. Either drop the piped-log justification from the comment, or, if you actually want the "why azd is waiting" hint to land in piped logs, emit it before the TTY guard.
| } | ||
|
|
||
| defaultIndex := int32(0) | ||
| resp, err := azdClient.Prompt().Select(ctx, &azdext.SelectRequest{ |
There was a problem hiding this comment.
From what I understand, non-tty contexts should automatically default to --no-prompt, causing this handling to default to the SelectedIndex, so I'm surprised we need any additional logic here (unless we don't want that default handling). @JeffreyCA @vhvb1989 can you confirm?
Ideally, we shouldn't duplicate some logic that is already built into azd, so even if this prompt doesn't work the way I think it should, everywhere else in the extension we've relied on the no-prompt flag to tell us when we're in a situation where we can't rely on interactive input, so we should be able to use that to simplify the above logic.
Summary
Fixes the silent hang described in #8584. Several
azd ai agentsubcommands render an interactive "Select an agent service" picker when the target agent is ambiguous (a multi-serviceazure.yamlwith more than oneazure.ai.agentblock). The picker is drawn by the azd host and blocks on stdin, so in a non-TTY context — CI, an agent harness, or output piped throughtee/tail— it was invisible and the process hung forever with no error, spinner, or banner; onlyCtrl+Crecovered.All of these commands funnel through a single shared helper,
promptForAgentService, which is the shared root cause. This fixes it centrally.Fixes #8584
What changed
promptForAgentService(internal/cmd/helpers.go):Fail fast in non-TTY contexts. When stdin/stdout is not a terminal, return a structured
exterrors.Validationerror (codenon_interactive_agent_selection) that lists the available services and explains how to pass one, instead of rendering a picker that hangs:with the suggestion to pass the agent service name explicitly (positional argument or
--agent-name), or run with--no-prompt.Banner before the interactive picker. Even in TTY mode, print a one-line banner to stderr (
azd is waiting for your agent service selection below...) before the picker. When the picker is visible this is harmless; when stdout is piped the banner still shows up intee/tail/CI logs and explains why azd is blocked.The existing
--no-promptfast-fail behavior is unchanged; the new guard covers the case where the user does not pass--no-promptbut has no TTY.Why this covers all the reported commands
invoke --local,files upload,delete,show,monitor,endpoint show,eval,optimize,update, etc. all resolve the agent throughresolveAgentService→promptForAgentService, so fixing the one helper turns the multi-minute silent hang into an immediate, diagnosable error everywhere.Testing
--no-promptfails fast; interactive mode prints the stderr banner and prompts once.agentSelectionInteractive).go build ./..., fullgo test ./...(all pass),gofmt,golangci-lint(0 issues),cspell(0 issues).Notes
The TTY check is exposed as a package-level function variable so tests can simulate a terminal; tests that use it run sequentially to avoid racing on the shared seam.