Avoid live CLI rendering when stdout is redirected - #19673
Ella Hathaway (ellahathaway) merged 5 commits into
Conversation
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 19673Or
iex "& { $(irm https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 19673" |
|
Copilot review |
There was a problem hiding this comment.
Pull request overview
Prevents Spectre live rendering when CLI stdout is redirected, avoiding Remote SSH crashes while preserving interactive terminal behavior.
Changes:
- Detects redirected stdout as non-interactive.
- Falls back to static rendering for live updates.
- Adds focused unit coverage.
Show a summary per file
| File | Description |
|---|---|
src/Aspire.Cli/Utils/CliHostEnvironment.cs |
Detects redirected stdout. |
src/Aspire.Cli/Interaction/ConsoleInteractionService.cs |
Adds static rendering fallback. |
tests/Aspire.Cli.Tests/Utils/CliHostEnvironmentTests.cs |
Tests redirected-output detection. |
tests/Aspire.Cli.Tests/Interaction/ConsoleInteractionServiceTests.cs |
Tests static live-update output. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Balanced
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Review details
Suppressed comments (1)
src/Aspire.Cli/Utils/CliHostEnvironment.cs:96
isOutputRedirectedis only checked in this final branch, soASPIRE_PLAYGROUND=truestill hard-codesSupportsInteractiveOutputtotrue.BuildAnsiConsolethen forcesInteractionSupport.Yes, meaning redirecting stdout in playground mode can still enterDisplayLiveAsync's Spectre live path and reproduce the same crash. Redirected output should take precedence for output capability (while playground may still force input/ANSI), with the playground tests updated to cover both redirected and terminal output.
SupportsInteractiveOutput = DetectInteractiveOutput(configuration, isOutputRedirected);
- Files reviewed: 5/5 changed files
- Comments generated: 0 new
- Review effort level: Balanced
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
James Newton-King (JamesNK)
left a comment
There was a problem hiding this comment.
This fixes the error by making data no longer live. But it was live for a reason. What is the new behavior?
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Good call. Interactive terminals still redraw endpoint updates in place with Spectre Live. Redirected/non-TTY output skips Live, but appends the initial state and each update so progress and the final snapshot remain in logs. I added coverage for both paths and made redirection win over the playground override. |
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: cf0deea0-a8ac-4680-a792-b0122657e423
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
PR Testing ReportPR Information
Artifact Version Verification
Changes AnalyzedFiles Changed
Change Categories
Test Scenarios ExecutedScenario 1: Exact PR artifact installation and identityObjective: Install the current PR dogfood CLI in an isolated Windows temp directory and prove it matches the latest PR commit. Coverage Type: Artifact boundary Status: Passed Evidence:
Scenario 2: Fresh project creation from the PR hiveObjective: Verify the PR CLI and PR package hive can create a fresh starter application non-interactively. Coverage Type: Happy path Status: Passed Observations:
Scenario 3: Redirected Remote SSH runObjective: Reproduce the reported redirected Remote SSH boundary with playground mode forced so stdout redirection is the decisive non-interactive capability. Coverage Type: Regression and unhappy-path boundary Status: Passed Observations:
Scenario 4: Build and focused boundary testsObjective: Verify the source-level redirected detection, explicit non-interactive live-rendering guard, incremental static path, and preserved interactive live path. Coverage Type: Build, unit, and command integration Status: Passed Commands: dotnet build src\Aspire.Cli\Aspire.Cli.csproj --no-restore
dotnet test --project tests\Aspire.Cli.Tests\Aspire.Cli.Tests.csproj --no-launch-profile -- `
--filter-class "*.RunCommandTests" `
--filter-class "*.ConsoleInteractionServiceTests" `
--filter-class "*.CliHostEnvironmentTests" `
--filter-not-trait "quarantined=true" `
--filter-not-trait "outerloop=true"Results:
EvidenceSanitized evidence was retained locally and was not uploaded to GitHub.
No screenshots were captured because the changed surface is redirected terminal output rather than dashboard UI. Summary
Overall ResultPR #19673 verified successfully. |
Comments have been addressed, and James is unavailable for a re-review
a7c95f8
into
microsoft:main
|
The CI build failed due to test failure(s) that appear unrelated to the PR changes. These may be flaky tests. Suspected flaky test(s):
Suggested actions:
You can re-run the failed jobs from the workflow run page. |
Description
aspire runcould enter Spectre live rendering in a Remote SSH session even when stdout was redirected. Resource endpoint updates then crashed the CLI withSystem.ArgumentExceptionfromLiveRenderable.Render, which also stopped the AppHost.This change treats redirected stdout as non-interactive. Real terminals keep the existing Spectre live display, which replaces a cumulative endpoint snapshot in place. Redirected or otherwise non-interactive remote runs bypass
DisplayLiveAsync: they print the CTRL+C hint once and append one line per newly observed endpoint, avoiding both cursor manipulation and duplicate snapshots.DisplayLiveAsyncnow rejects non-interactive calls so future callers must define appropriate static semantics.User-facing usage
The reported path now stays running and writes plain endpoint updates:
Before this change, the command exited with code 2 when the first endpoint updated:
After this change, stdout contains one static record per endpoint transition with no carriage-return live redraws, stderr has no Spectre exception, and the AppHost remains running until stopped. The same Remote SSH command under a real PTY continues to use live rendering.
Screenshots / Recordings
The affected surface is redirected stdout rather than a visual terminal layout, so the exact before/after command and output are included above. Interactive PTY rendering was verified separately and remains unchanged.
Validation
RunCommandTests,ConsoleInteractionServiceTests, andCliHostEnvironmentTests: 234 passedSmokeTests.RedirectedRemoteSshRunUsesStaticOutput: 1 passed against a locally built Linux arm64 archive in 2m06sdotnet build src/Aspire.Cli/Aspire.Cli.csproj --no-restore: 0 warnings, 0 errorsResolves #19290
Checklist
<remarks />and<code />elements on your triple slash comments?