Fix aspire start --format json restart output#16228
Conversation
Suppress human-readable restart and status output when \ runs, and add an end-to-end regression that validates combined output stays valid JSON during restart.\n\nCo-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
sebastienros
requested review from
JamesNK,
davidfowl and
mitchdenny
as code owners
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 16228Or
iex "& { $(irm https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 16228" |
|
Isn't this already handled today by outputting human readable content to stderr and JSON to stdout? That's a common pattern in CLI apps when outputing structured data. |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Fixes aspire start --format json (detached/restart path) so machine-readable JSON output isn’t polluted by human-readable restart/stop/status messages, and adds an end-to-end regression test to prevent regressions.
Changes:
- Suppress human-readable “stopping previous instance” / “stopped successfully” messaging when JSON output is requested during detached launch/restart.
- Add an E2E regression that validates combined stdout/stderr remains valid JSON on restart.
- Update E2E test setup to use the install strategy flow for Docker runs.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| tests/Aspire.Cli.EndToEnd.Tests/MultipleAppHostTests.cs | Adds an E2E restart regression test and updates install flow usage. |
| src/Aspire.Cli/Projects/RunningInstanceManager.cs | Adds a switch to suppress human-readable stop progress/success messaging. |
| src/Aspire.Cli/Commands/AppHostLauncher.cs | Threads JSON-mode suppression into the detached launcher stop/restart flow. |
Update the restart-path E2E test to capture combined output without a tee pipeline, and rely on validating the parsed JSON shape instead of brittle English phrase checks.\n\nCo-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
JamesNK
left a comment
JamesNK
left a comment
There was a problem hiding this comment.
Isn't this already handled today by outputting human readable content to stderr and JSON to stdout? That's a common pattern in CLI apps when outputing structured data.
The stdout/stderr split is already there, but the issue in #15843 is that the detached restart path was still emitting human-readable stop/status lines before the final JSON payload. In practice that meant the combined command output shown in the issue was not valid JSON: That is a fine pattern when callers only consume stdout, but The change is scoped to that JSON detached-start path only. Normal |
The solution is to put the extra human text in stderr, no? I setup a system that as soon as Is the problem that |
|
I tested this scenario on main adn it seems to work how I would have expected. Only JSON is in stdout: 
|
Right - stdout-only already behaves that way, and that matches the global stderr routing you added. The case this PR is fixing is narrower: the detached restart path still surfaced extra human-readable stop/status text in the overall command output, which is what the issue screenshot shows. That's why the regression here validates the combined output with So this PR is not changing the existing stdout behavior. It's making the restart path consistent with the issue's expectation that |
|
I ran it again with a resource already running (that's what you mean by the restart path?) and still only got JSON in stdout: 
|
|
Although, I'm using 13.2. Let me retry with CLI daily build. |
|
With daily CLI I still see only JSON in stdout when restarting: 
|
Yep - that matches what I'd expect too. This PR is not trying to change stdout-only behavior. The issue is about the human-readable restart messages still being shown alongside the JSON result when That's the distinction behind the regression: it validates the restart path when the caller captures the full command output with So if the question is "does stdout already stay JSON-only on main?" then yes. If the question is "can |
|
In that case I think the premise of the PR is wrong. It's common practice in CLI apps to display human readable messages to stderr and structured content to stdout. This is what Aspire CLI already does. See https://chatgpt.com/share/69e042f8-57cc-8321-b76c-23a947a93f37 |
Revert the stderr-suppression behavior so detached start keeps the existing CLI contract of structured output on stdout and human-readable progress on stderr. Replace the restart-path regression with a stdout-only JSON assertion that covers restarting an existing instance.\n\nCo-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Agreed, and I've updated the branch to match that contract. The latest push removes the stderr-suppression behavior and narrows the change to a restart-path regression: So the PR is now about covering the existing stdout contract in the restart path rather than changing overall terminal-visible output semantics. |
JamesNK
left a comment
JamesNK
left a comment
There was a problem hiding this comment.
LGTM — the new test correctly validates the restart-path JSON contract, and the API migration to CliInstallStrategy.Detect() is clean. Two minor observations posted as comments (stale PR description, pre-existing weak validation in the first test).
Strengthen the existing detached JSON E2E validation to assert required keys and explicitly wait for the success marker, matching the restart-path regression.\n\nCo-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Switch the restart-path MultipleAppHostTests setup and cleanup to AspireStartAsync/AspireStopAsync so the test follows the shared end-to-end helper patterns.\n\nCo-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
🎬 CLI E2E Test Recordings — 72 recordings uploaded (commit View recordings
📹 Recordings uploaded automatically from CI run #24490202101 |
|
No documentation PR is required for this change. Reason: This PR is a bug fix that only modifies test files (
|
Description
Adds restart-path coverage for the existing
aspire start --format jsoncontract: stdout should remain a single JSON document even when an existing AppHost instance is restarted.The PR also updates
MultipleAppHostTeststo useCliInstallStrategy.Detect()so local archive runs exercise the current branch, and it tightens the existing detached JSON validation to assert the expected keys instead of only printing booleans.Related to #15843
Checklist
<remarks />and<code />elements on your triple slash comments?aspire.devissue: