microsoft / microsoft/aspire

Deployment/CLI E2E `PullRequest` install mode never puts the PR CLI on PATH (silent false passes)

Open
#19,223 1 comment 0 reactions 0 assignees View on GitHub
area-engineering-systems area-testing triage:bot-seen
Dominant language
C#
Stars
6.3k
Forks
991
Avg merge
2d 15h
Merged PRs (30d)
196

Description

## Summary

`CliInstallMode.PullRequest` in the E2E test harness installs the PR CLI into `~/.aspire/dogfood/pr-/bin`, but the harness only ever prepends `~/.aspire/bin` to `PATH`. The PR binary is therefore **never** the `aspire` that the test invokes.

On a clean machine this fails loudly (`aspire: command not found`). On any developer machine with a pre-existing `~/.aspire/bin/aspire`, the test **silently runs that unrelated CLI and reports a pass** — a false green that claims PR coverage it does not have.

I hit this while running `Aspire.Deployment.EndToEnd.Tests` against PR #19219: the run went green in 10m11s while actually exercising `13.5.0+cfbf1c43` (release/13.5), which provably does not contain the change under test.

## Root cause

Three independent gaps combine:

**1. Install goes to the dogfood path** — `tests/Shared/CliInstallStrategy.cs` (~line 124):

```csharp
internal static string GetPullRequestInstallArgs(int prNumber)
{
return prNumber.ToString(CultureInfo.InvariantCulture);
}
```

No `--install-path`, so `get-aspire-cli-pr.sh` uses its default. The installer even prints:

```
Add to your shell profile: export PATH="$HOME/.aspire/dogfood/pr-19219/bin:$PATH"
```

**2. PATH only ever gets `~/.aspire/bin`** — `tests/Shared/CliInstallStrategy.cs:78-83`:

```csharp
internal static string GetSourceAspireEnvironmentCommand(bool includeBundlePath)
{
return includeBundlePath
? $"export PATH=~/.aspire/bin:~/.aspire:$PATH {CommonAspireEnvironmentAssignments}"
: $"export PATH=~/.aspire/bin:$PATH {CommonAspireEnvironmentAssignments}";
}
```

This is unconditional — it does not consider the install mode, so the dogfood directory is never added. Sole caller: `tests/Shared/Hex1bAutomatorTestHelpers.cs:336` `SourceAspireEnvironmentAsync`.

**3. Nothing asserts the resulting version** — `tests/Shared/CliInstallStrategy.cs:337`:

```csharp
public static CliInstallStrategy FromPullRequest()
{
// ... validates GITHUB_PR_NUMBER / GITHUB_PR_HEAD_SHA are present ...
return new CliInstallStrategy(CliInstallMode.PullRequest); // no expectedVersion
}
```

`ExpectedVersion` stays `null`, so `CliE2EAutomatorHelpers.VerifyAspireCliVersionAsync` (line 466) takes its `expectedVersion is null` branch and only *logs* the version. The `Assert.Fail` guard on line 476 is scoped to `CliInstallMode.LocalArchive`, so PR mode is exempt. `Aspire.Deployment.EndToEnd.Tests` is weaker still — `DeploymentE2EAutomatorHelpers.cs:98` calls `LogAspireCliVersionAsync`, which never asserts anything.

Notably, `FromPullRequest()` reads `GITHUB_PR_HEAD_SHA` purely to validate it is non-empty and then discards it — the one value that would make verification trivial is thrown away.

## Impact

- **CI / clean machine:** hard failure at first `aspire` invocation. Noisy but safe.
- **Developer machine with an existing `~/.aspire/bin/aspire`:** silent false pass. The test appears to validate the PR while exercising whatever CLI happens to be installed. This is the dangerous case — it actively misleads.

Both `Aspire.Cli.EndToEnd.Tests` and `Aspire.Deployment.EndToEnd.Tests` route through the same helper, so both modes are affected.

## Repro

```bash
# with a stable CLI already at ~/.aspire/bin/aspire
export GITHUB_PR_NUMBER=19219
export GITHUB_PR_HEAD_SHA=
dotnet test --project tests/Aspire.Deployment.EndToEnd.Tests/... \
-- --filter-method "*.DeployAksWithAzureResources"
```

Then inspect the recorded `.cast` under
`artifacts/bin/Aspire.Deployment.EndToEnd.Tests/Debug/net10.0/TestResults/recordings/aspire-deployment-e2e/`
— `aspire --version` reports the pre-existing stable CLI, not `13.6.0-pr.19219.*`.

## Suggested fix

1. Make the PATH helper mode-aware so `PullRequest` prepends `~/.aspire/dogfood/pr-/bin`; or pass `--install-path`/`--skip-path` so the PR CLI lands where the harness already looks.
2. Populate `ExpectedVersion` (or at least the head SHA) in `FromPullRequest()` and assert it, so a mismatch fails the test instead of logging.
3. Consider extending the `Assert.Fail` guard beyond `LocalArchive` — any mode that cannot verify what it installed should fail on CI rather than pass quietly.

Item 2 alone would have converted my false green into an immediate, obvious failure.

## Open question

It is not clear whether `PullRequest` mode is exercised in CI at all, or is effectively dead code. Worth confirming as part of the fix — if it is dead, that would explain how the gap survived.

## Workaround

Back up `~/.aspire/bin/aspire`, copy the dogfood binary over it, run, then restore. This is sufficient because the PR CLI writes its own hive into the generated project's `nuget.config`, so `aspire add` resolves PR packages without extra flags.

Contributor guide

Open the contributing guide

Research direction

Start with tests/Shared/CliInstallStrategy.cs, especially GetPullRequestInstallArgs, GetSourceAspireEnvironmentCommand, and FromPullRequest; then trace callers in Hex1bAutomatorTestHelpers.cs and version handling in CliE2EAutomatorHelpers.cs and DeploymentE2EAutomatorHelpers.cs. Reproduce the deployment E2E case with an existing ~/.aspire/bin/aspire. Done means PullRequest mode invokes the installed PR CLI and a version or head-SHA mismatch fails rather than logging a false pass.

Written by the indexing model from the issue text.

Assessment

Tech stack
csharp
Domain
cli, testing-qa
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
55/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.